fix(orb): stop the relay losing events without a trace or a retry row (#9471) - #9510
Conversation
|
Warning ⏸️ LoopOver review result - manual review recommendedReview updated: 2026-07-28 02:35:40 UTC
Review summary Nits — 6 non-blocking
Decision drivers
Context & advisory signals — never blocks the verdict
Linked issue satisfactionPartially addressed Review context
Contributor next steps
Signal definitions
🧪 Chat with LoopOverAsk LoopOver a question about this PR directly in a comment — grounded only in the same cached, public-safe facts shown above, never a new claim.
Full command reference: https://loopover.ai/docs/loopover-commands 🧪 Experimental — new and may change. Decision record
🟩 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 LoopOver, a quiet PR intelligence layer for OSS maintainers.
|
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
loopover-orb
Bot
added
the
gittensor:bug
…#9471) The receiver ACKs GitHub 202 and forwards in a deferred task, so anything that throws out of forwardOrbEvent is an event GitHub considers delivered and this deployment has silently lost. Only the PUSH branch sat inside forwardOrbEvent's try/catch: the enrollment SELECT and the entire PULL branch -- four D1 statements -- were outside it, and relayForward's catch was completely empty. A transient D1 error there, exactly what a near-cap database produces and this D1 has hit its 10GB ceiling twice, left the event recorded in orb_webhook_events but in NEITHER orb_relay_pending NOR orb_relay_failures, so no retry cron could ever see it and the review simply never ran. Both paths now degrade to "failed", which routes them into the same durable retry machinery the push path already relies on, and relayForward's catch logs and counts instead of swallowing in silence. The per-installation cap eviction was the last silent drop path: its two siblings each emit an alertable *_dropped error with samples, but this one deleted the oldest events with no log and without checking meta.changes. A pull-mode container down for hours on an active repo can exceed 500 pending events, and those deliveries vanished with zero trace. retryFailedRelays is documented as never throwing and job-dispatch relies on that, but forwardOrbEvent can throw -- so one bad row rejected the Promise.all, skipped every later chunk in the tick, and because finalize never ran left that row's attempts unadvanced. It stayed first-in-batch and immediately eligible, wedging the retry consumer's tail until its 1h TTL. Rows are now isolated individually and a throwing row still advances its attempt counter.
851c635 to
0a781fe
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #9510 +/- ##
==========================================
- Coverage 89.55% 88.65% -0.90%
==========================================
Files 843 843
Lines 110135 110148 +13
Branches 26207 26208 +1
==========================================
- Hits 98635 97657 -978
- Misses 10238 11520 +1282
+ Partials 1262 971 -291
Flags with carried forward coverage won't be shown. Click here to find out more.
|
1 participant
Summary
Closes the relay's silent event-loss paths. The receiver ACKs GitHub 202 and forwards in a deferred task, so anything that throws out of
forwardOrbEventis an event GitHub considers delivered and this deployment has lost.Closes #9471
The three paths
1. Only the push branch was guarded.
forwardOrbEvent'stry/catch(which degrades to"failed"and thereby persists toorb_relay_failuresfor the retry cron) wrapped only the push path. The enrollmentSELECTand the entire pull branch — four D1 statements — sat outside it. AndrelayForward's catch was completely empty: no log, no metric.So a transient D1 error in the pull path — precisely what a near-cap database produces, and this D1 has hit its 10 GB ceiling twice — left the event recorded in
orb_webhook_eventsbut in neitherorb_relay_pendingnororb_relay_failures. No retry cron could ever see it; the review simply never ran. Pull mode is the shapeedge-nl-01runs.Both paths now degrade to
"failed", routing into the same durable retry machinery the push path already relies on, andrelayForward's catch logs the delivery id and increments a counter.2. The cap eviction was the last silent drop.
pruneRelayPendingandretryFailedRelayseach emit an alertable*_droppederror with samples, but the per-installation cap deleted the oldest events with no log and without even checkingmeta.changes. A pull-mode container down for hours on an active repo can exceed 500 pending events (issue_comment+pull_request+check_suitetogether), and those deliveries vanished with zero trace — inconsistent with this file's own zero-trace-loss doctrine. It now samples before deleting and names the installation.3. One bad row could wedge the retry consumer.
retryFailedRelaysis documented as never throwing andjob-dispatchrelies on that, butforwardOrbEventcould throw — so one row rejected thePromise.all, skipped every later chunk in the tick, and (becausefinalizenever ran) left that row'sattempts/last_attempt_atunadvanced. It stayed first-in-batch and immediately eligible, wedging the tail until its 1-hour TTL. Rows are isolated individually now.Two notes on honesty of the fix
The retry-row isolation is now defence in depth, and is marked as such. Once the enrollment read and pull branch are guarded,
forwardOrbEventis total, andfinalizeRelayFailureRetryRowalready has its own catch — so nothing inretryRowcan currently throw. I kept the isolation anyway (the function's "never throws" contract is relied upon elsewhere, and a future unguarded path must degrade rather than wedge) and annotated it with that reasoning rather than writing a test that cannot fail.I dropped a nullish fallback rather than testing an unreachable arm.
installationId: args.installationId ?? nullin the new error log had a null arm that nothing can reach —forwardOrbEventshort-circuits before any IO for a null installation, so nothing throws.JSON.stringifyomitsundefinedand keeps explicitnull, so the fallback bought nothing.Validation
npx tsc --noEmit -p tsconfig.json— cleanorb-relay(integration) andselfhost-metricsRegressions, verified to fail against the unfixed code:
"failed"instead of throwing out offorwardOrbEvent(removing the guard makes it fail)."failed"rather than escaping unclassified.relayForwardlogs and counts a swallowed forward error, with the delivery id present so the event is identifiable.Invariants:
"failed"outcome always persists a durable retry row, so the event stays recoverable.Two new metrics are registered in
DEFAULT_METRIC_META(the drift guard enforces this).