Skip to content

fix(orb): order enrollment lookup so relay forwarding targets the current container - #1789

Closed
jaso0n0818 wants to merge 1 commit into
JSONbored:mainfrom
jaso0n0818:fix/orb-relay-enrollment-order-1782774459
Closed

fix(orb): order enrollment lookup so relay forwarding targets the current container#1789
jaso0n0818 wants to merge 1 commit into
JSONbored:mainfrom
jaso0n0818:fix/orb-relay-enrollment-order-1782774459

Conversation

@jaso0n0818

Copy link
Copy Markdown
Contributor

Closes #1783.

The bug

forwardOrbEvent (src/orb/relay.ts) loads the brokered self-host relay target with a .first() query on orb_enrollments that has no ORDER BY. issueOrbEnrollment inserts a new enrolled row on every enrollment without revoking the prior rows for the same installation_id, so D1/SQLite can return an arbitrary enrolled row. After a maintainer re-enrolls and registers a new push relay URL, forwardOrbEvent may read an older row (stale relay URL, or none registered) and skip the event or POST the HMAC-signed payload to the wrong container.

The fix

Order the lookup by relay_registered_at DESC, enrolled_at DESC — the order the issue specifies — so the most recently registered enrollment wins and re-enrollment always targets the current self-host container. (A re-registered relay sets relay_registered_at = CURRENT_TIMESTAMP; enrolled_at breaks ties.)

Tests

Integration test per the issue's Validation spec: double-enroll one installation (row A stays enrolled with no relay), register a relay on the second secret only, then assert forwardOrbEvent forwards to the new URL instead of skipping on the stale row. All 63 orb-relay tests green; the changed query line is covered; OpenAPI unchanged.

…rent container

forwardOrbEvent loaded the brokered self-host relay target with a .first() query
on orb_enrollments that has no ORDER BY. issueOrbEnrollment inserts a new
'enrolled' row on every enrollment without revoking the prior rows for the same
installation_id, so D1/SQLite could return an arbitrary enrolled row: after a
maintainer re-enrolls and registers a new push relay URL, forwardOrbEvent might
read an older row (stale relay URL, or none registered) and skip or POST to the
wrong container.

Order the lookup by relay_registered_at DESC, then enrolled_at DESC, so the most
recently registered enrollment wins and re-enrollment always targets the current
self-host container. Adds an integration test: double-enroll one installation,
register a relay on the second secret only, and assert forwardOrbEvent forwards
to the new URL instead of skipping on the stale row.
@jaso0n0818
jaso0n0818 requested a review from JSONbored as a code owner June 29, 2026 23:08
@dosubot dosubot Bot added the size:XS label Jun 29, 2026
@loopover-orb

loopover-orb Bot commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

Warning

🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨

⏸️ Gittensory review result - manual review recommended

Review updated: 2026-06-30 09:05:32 UTC

2 files · 1 AI reviewer · no blockers · readiness 75/100 · CI green · dirty

⏸️ Suggested Action - Manual Review

Review summary
The change fixes the stale relay-target lookup by making `forwardOrbEvent` choose a deterministic enrolled row with a registered relay ahead of older unregistered enrollments, and the added integration test exercises the double-enrollment path described in the issue. The query-level fix is in the right layer for the forwarding path, and the test validates that the newer registered relay URL is used instead of skipping on the older row. The only maintainability concern is that the ordering still relies on timestamp precision for complete determinism when rows are created/registered in the same second.

Nits — 3 non-blocking
  • nit: `src/orb/relay.ts:302` still has no final deterministic tie-breaker after `relay_registered_at DESC, enrolled_at DESC`; add a stable column such as `enroll_id DESC` if same-second enroll/register events are possible under SQLite `CURRENT_TIMESTAMP` precision.
  • `src/orb/relay.ts:302`: consider extending the ORDER BY to include a stable final tie-breaker so the current-container selection cannot fall back to arbitrary row order on timestamp ties.
  • `test/integration/orb-relay.test.ts`: add a same-second/tie case if the fixtures can force equal timestamps, because that is the remaining place where this class of bug can reappear.
Signal Result Evidence
Code review ✅ No blockers 1 reviewer
Linked issue ✅ Linked #1783
Related work ✅ No active overlap found No same-issue or scoped active PR overlap found.
Change scope ✅ 20/20 Low review scope from cached public metadata (1 linked issue).
Validation posture ❌ 5/25 Preflight is holding this PR; address the blocker before review.
Contributor workload ✅ 10/10 Author activity: 619 registered-repo PR(s), 384 merged, 6 issue(s).
Contributor context ✅ Confirmed Gittensor contributor jaso0n0818; Gittensor profile; 619 PR(s), 6 issue(s).
Gate result ✅ Passing No configured blocker found.
Review context
  • Author: jaso0n0818
  • Role context: outside_contributor
  • Public audience mode: oss maintainer
  • Lane context: Repository registration is not available in the local Gittensory cache.
  • Public profile languages: not available
  • Official Gittensor activity: 619 PR(s), 6 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Fix the blocker.
  • Triage stale or unlinked PRs.
  • Refresh registry data or choose a registered active repo.
Signal definitions
  • Related work = same linked issue, overlapping active PRs, or title/path similarity.
  • Change scope = cached public metadata such as size labels, draft state, and review-burden hints.
  • Validation posture = whether the PR provides enough public validation/test evidence for maintainer review.
  • Contributor workload = public contributor activity and cleanup pressure, not a repo-wide quality failure.
  • Contributor context = public GitHub/Gittensor identity context; non-Gittensor status is not a blocker.

🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed


💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →.

Checked by Gittensory, a quiet PR intelligence layer for OSS maintainers.

  • Re-run Gittensory review

@loopover-orb loopover-orb Bot added gittensor gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. labels Jun 29, 2026
@jaso0n0818 jaso0n0818 closed this Jun 29, 2026
@jaso0n0818

Copy link
Copy Markdown
Contributor Author

Closing as a duplicate of #1785, which addresses #1783 with the same fix and was opened first.

@jaso0n0818 jaso0n0818 reopened this Jun 30, 2026

@JSONbored JSONbored left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

merge conflicts, closing.

This branch has conflicts that must be resolved
Use the [web editor](https://github.com/JSONbored/gittensory/pull/1789/conflicts) or the command line to resolve conflicts before continuing.

src/orb/relay.ts
test/integration/orb-relay.test.ts

@JSONbored JSONbored closed this Jun 30, 2026
@jaso0n0818
jaso0n0818 deleted the fix/orb-relay-enrollment-order-1782774459 branch June 30, 2026 10:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Orb relay forward picks an arbitrary enrollment when multiple enrolled rows exist

2 participants