Skip to content

fix(orb): guard finalizeRelayFailureRetryRow DB writes against duplicate redelivery (#8332)#8462

Merged
loopover-orb[bot] merged 2 commits into
JSONbored:mainfrom
joaovictor91123:fix/orb-relay-finalize-retry-guard-8332
Jul 24, 2026
Merged

fix(orb): guard finalizeRelayFailureRetryRow DB writes against duplicate redelivery (#8332)#8462
loopover-orb[bot] merged 2 commits into
JSONbored:mainfrom
joaovictor91123:fix/orb-relay-finalize-retry-guard-8332

Conversation

@joaovictor91123

Copy link
Copy Markdown
Contributor

Summary

  • src/orb/relay.ts's retryFailedRelays doc comment promises "Never throws." Its per-row finalize step, finalizeRelayFailureRetryRow, issued its DELETE/UPDATE on orb_relay_failures with no try/catch — a transient D1 write failure right after a successful forward would reject out of the Promise.all batch, breaking that contract, and would leave the failure row un-deleted despite the event already having been forwarded (risking duplicate redelivery on the next retry tick). Wrapped both writes in a try/catch that logs an alertable structured error (delivery_id, event_name, outcome) and swallows, matching this file's existing console.error(JSON.stringify({...})) logging convention used elsewhere in the same file.

(Reopens the substance of #8441, which was auto-closed on a stale codecov/patch evaluation from a genuine gap in the original test — the error instanceof Error ? error.message : String(error) fallback branch was only ever exercised with real Error instances. Fixed here: one regression test now throws a non-Error value to cover the String(error) branch, verified locally at 99.11% branch coverage on src/orb/relay.ts, the only remaining gap being pre-existing unrelated code outside this diff.)

Scope

  • The PR title follows type(scope): short summary Conventional Commit format, for example fix(api): restore profile access checks.
  • This PR is focused and does not mix unrelated backend, UI, MCP, docs, dependency, and deploy changes.
  • This follows CONTRIBUTING.md and does not reintroduce GitHub Pages, VitePress, site/, or CNAME.
  • I linked a currently open issue this PR resolves (e.g. Closes #123) — a linked open issue is required for every contributor PR.

Closes #8332

Validation

  • git diff --check
  • npm run typecheck
  • npx vitest run test/integration/orb-relay.test.ts --coverage --coverage.include="src/orb/relay.ts" — all 85 tests pass; 99.11% branch coverage (112/113) on the changed file — the single remaining uncovered branch is pre-existing code outside this diff (enqueueConfigPushRelay, lines 385-392)
  • npm run actionlint
  • npm run test:workers
  • npm run build:mcp
  • npm run test:mcp-pack
  • npm run ui:openapi:check
  • npm run ui:lint
  • npm run ui:typecheck
  • npm run ui:build
  • npm audit --audit-level=moderate
  • New or changed behavior has unit/integration tests for new branches, fallback paths, and sanitizer boundaries

If any required check was skipped, explain why:

  • This change touches only src/orb/relay.ts and its integration test (no UI/MCP/worker/OpenAPI surface touched), so the UI/MCP/workers/OpenAPI-specific checks above were not run locally; they are unaffected by this diff and are still exercised by the full CI gate.

Safety

  • No secrets, wallet details, hotkeys, coldkeys, user PATs, private keys, raw trust scores, private rankings, or private maintainer evidence are exposed.
  • Public GitHub text stays sanitized, low-noise, and does not imply compensation guarantees or optimization tactics.
  • Auth, cookie, CORS, GitHub App, Cloudflare, or session changes include negative-path tests. (N/A — no auth/session/CORS code touched.)
  • API/OpenAPI/MCP behavior is updated and tested where needed. (Internal retry-cron behavior only; no external API/OpenAPI/MCP surface changed.)
  • UI changes use live API data or real empty/error/loading states, not production mock/demo fallbacks. (N/A — no UI touched.)
  • Visible UI changes include a UI Evidence section below. (N/A — no visible UI change.)
  • Public docs/changelogs are updated where needed. (N/A — no docs/changelog change needed.)

UI Evidence

N/A — this is a backend cron/retry-path change with no visible UI surface.

Notes

  • Only finalizeRelayFailureRetryRow's two DB writes are guarded; no other logic in retryFailedRelays or elsewhere in the file was touched, per the issue's own scope note.

@superagent-security

Copy link
Copy Markdown
Contributor

Superagent didn't find any vulnerabilities or security issues in this PR.

@codecov

codecov Bot commented Jul 24, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.64%. Comparing base (6c25eb6) to head (28abef7).
⚠️ Report is 16 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8462      +/-   ##
==========================================
+ Coverage   79.69%   89.64%   +9.95%     
==========================================
  Files         791       98     -693     
  Lines       79319    22847   -56472     
  Branches    23954     3912   -20042     
==========================================
- Hits        63216    20482   -42734     
+ Misses      13298     2187   -11111     
+ Partials     2805      178    -2627     
Flag Coverage Δ
shard-1 27.65% <0.00%> (?)
shard-2 97.16% <100.00%> (+46.05%) ⬆️
shard-3 26.95% <0.00%> (-26.48%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
src/orb/relay.ts 100.00% <100.00%> (+64.23%) ⬆️

... and 693 files with indirect coverage changes

@loopover-orb loopover-orb Bot added the gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. label Jul 24, 2026
@loopover-orb

loopover-orb Bot commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Tip

✅ LoopOver review result - approve/merge recommended

Review updated: 2026-07-24 14:47:52 UTC

2 files · 1 AI reviewer · no blockers · readiness 93/100 · CI green · clean

✅ Suggested Action - Approve/Merge

  • safe to merge

Review summary
This PR wraps finalizeRelayFailureRetryRow's DELETE/UPDATE writes in a try/catch to actually satisfy retryFailedRelays's documented 'Never throws' contract, logging a structured error with delivery_id/event_name/outcome when the write fails after a successful forward. The fix is correctly placed at the source (the row-level finalize function itself, not a caller wrapper), and both branches (DELETE-path and UPDATE-path failures, Error vs non-Error thrown values) are exercised by the two added regression tests, which mock env.DB.prepare to reject on the specific SQL string while leaving other prepared statements to hit the real DB. The early `return` inside the try block for the terminal DELETE case is correctly placed so the non-terminal logRelayTransientSkip call below the try/catch is skipped for terminal outcomes, preserving prior behavior.

Nits — 4 non-blocking
  • src/orb/relay.ts:122 — the console.error call is intentional structured logging matching the file's existing convention (not a debug leftover), but confirm this is exempt from any lint rule that flags bare console usage.
  • The two new tests replace `e.DB` entirely with a partial mock object cast via `as unknown as Env["DB"]` (test/integration/orb-relay.test.ts) rather than wrapping the existing prepare like other regression tests in this same file (e.g. the fix(orb): drainOrbRelay returns HTTP 500 repeatedly (872 Sentry events, escalating) #4995 tests) — for consistency, consider using the same realPrepare-wrapping pattern used elsewhere in this file.
  • Consider whether the swallowed error should also increment a metric/counter distinct from the log line, so an operator dashboard doesn't rely solely on grepping structured JSON logs for orb_relay_failure_finalize_write_failed.
  • src/orb/relay.ts:117-118 — the comment explaining the duplicate-redelivery risk is good context; consider linking to issue finalizeRelayFailureRetryRow's unguarded DB writes contradict retryFailedRelays's documented Never throws contract #8332 directly in the comment for easy traceability, matching the file's convention of citing issue numbers in nearby comments.

Decision drivers

  • ✅ Code review — No blockers (1 reviewer)
  • ✅ Gate result — Passing (No configured blocker found.)
Context & advisory signals — never blocks the verdict
Signal Result Evidence
Linked issue ✅ Linked #8332
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 ✅ 25/25 PR body includes validation/test evidence.
Contributor workload ✅ 10/10 Author activity: 183 registered-repo PR(s), 83 merged, 5 issue(s).
Contributor context ✅ Confirmed Gittensor contributor joaovictor91123; Gittensor profile; 183 PR(s), 5 issue(s).
Improvement ✅ Minor risk: clean · value: minor · LLM: moderate
Linked issue satisfaction

Addressed
The diff wraps both the DELETE and UPDATE writes in finalizeRelayFailureRetryRow in a try/catch that logs a structured, alertable console.error containing delivery_id, event_name, and outcome before swallowing the error, matching the requested logging convention. Two new regression tests cover both the DELETE and UPDATE failure paths (including both branches of the error-message ternary) and asser

Review context
  • Author: joaovictor91123
  • Role context: outside_contributor
  • Public audience mode: oss maintainer
  • Lane context: Repository is configured for direct PR review.
  • Public profile languages: Python, JavaScript, MDX, TypeScript, C++, CSS, Rust
  • Official Gittensor activity: 183 PR(s), 5 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Start here: Triage stale or unlinked PRs.
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.
🧪 Chat with LoopOver

Ask 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.

  • @loopover ask &lt;question&gt; answers contribution-quality Q&A with source citations and freshness.
  • @loopover chat &lt;question&gt; answers in natural prose from cached decision-pack facts via local inference (maintainer/collaborator; read-only).
  • A plain-language @loopover mention with a real question is routed to the closest matching read-only command automatically — no exact syntax required.

Full command reference: https://loopover.ai/docs/loopover-commands

🧪 Experimental — new and may change.

🟩 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.

  • Re-run LoopOver review

@loopover-orb loopover-orb Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LoopOver approves — the gate is satisfied and CI is green.

@loopover-orb
loopover-orb Bot merged commit 9b9924b into JSONbored:main Jul 24, 2026
12 checks passed
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.

finalizeRelayFailureRetryRow's unguarded DB writes contradict retryFailedRelays's documented Never throws contract

1 participant