Skip to content

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

Closed
joaovictor91123 wants to merge 1 commit into
JSONbored:mainfrom
joaovictor91123:fix/orb-relay-finalize-retry-guard-8332
Closed

fix(orb): guard finalizeRelayFailureRetryRow DB writes against duplicate redelivery (#8332)#8441
joaovictor91123 wants to merge 1 commit 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.

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; the two new/changed lines in finalizeRelayFailureRetryRow are fully exercised by two new regression tests (DELETE-path and UPDATE-path DB-write failure)
  • 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

❌ Patch coverage is 85.71429% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 89.64%. Comparing base (bf2270e) to head (032fc1b).
⚠️ Report is 16 commits behind head on main.

Files with missing lines Patch % Lines
src/orb/relay.ts 85.71% 0 Missing and 1 partial ⚠️

❌ Your patch check has failed because the patch coverage (85.71%) is below the target coverage (99.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8441      +/-   ##
==========================================
- Coverage   92.42%   89.64%   -2.79%     
==========================================
  Files         791       98     -693     
  Lines       79294    22847   -56447     
  Branches    23950     3912   -20038     
==========================================
- Hits        73291    20481   -52810     
+ Misses       4866     2187    -2679     
+ Partials     1137      179     -958     
Flag Coverage Δ
shard-1 27.65% <0.00%> (-30.11%) ⬇️
shard-2 96.45% <85.71%> (+48.77%) ⬆️
shard-3 26.95% <0.00%> (-29.63%) ⬇️

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

Files with missing lines Coverage Δ
src/orb/relay.ts 99.29% <85.71%> (-0.71%) ⬇️

... and 693 files with indirect coverage changes

@joaovictor91123
joaovictor91123 force-pushed the fix/orb-relay-finalize-retry-guard-8332 branch from 4e8a5c1 to 032fc1b Compare July 24, 2026 13:40
@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

Caution

🛑 LoopOver review result - fixes required

Review updated: 2026-07-24 14:04:27 UTC

2 files · 1 AI reviewer · no blockers · CI failing · unstable

🛑 Suggested Action - Fix Blockers

Review summary
This PR wraps finalizeRelayFailureRetryRow's DELETE/UPDATE calls in a try/catch so a transient D1 write failure after a successful forward is logged (with delivery_id, event_name, outcome) rather than throwing out of the Promise.all batch in retryFailedRelays, restoring the documented 'Never throws' contract and preserving the file's existing console.error(JSON.stringify(...)) logging convention. The fix is correctly placed at the actual failure site (the write itself), the reasoning about duplicate-redelivery risk is accurate given the row stays pending, and both new tests genuinely exercise the real DELETE-path and UPDATE-path failure branches by stubbing env.DB.prepare for the exact SQL string rather than fabricating an unreachable state. The console.error call flagged in the brief is the same logging convention already used elsewhere in this file (pruneRelayPending, retryFailedRelays) and is not a debug leftover.

Nits — 4 non-blocking
  • The codecov/patch check failed at 85.71% (target 99%) — likely because the `return;` on the catch's fallthrough path or one of the two write-outcome branches isn't independently branch-covered by the two new tests; worth checking coverage output to see which line/branch is still uncovered.
  • src/orb/relay.ts: the try/catch now wraps both the terminal DELETE and the non-terminal UPDATE, but note the `outcome === "skipped"` transient-skip log below the try block is only reached on the success path — worth a one-line comment noting that a write failure suppresses that secondary log too, since a reviewer might otherwise expect it always to fire on skip.
  • Consider whether the catch block should also record a metrics counter (in addition to the log) so an operator dashboard can alert on sustained finalize-write failures without grepping logs, though this may be out of scope for this narrow fix.
  • If codecov/patch failure turns out to be a real gap rather than a stale-branch artifact from the 10-commit lag, add a third test to cover whichever branch is missing (e.g. the isRelayFailureRetryTerminal===false path combined with a non-D1 thrown value).

CI checks failing

  • codecov/patch — 85.71% of diff hit (target 99.00%)

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: 176 registered-repo PR(s), 81 merged, 5 issue(s).
Contributor context ✅ Confirmed Gittensor contributor joaovictor91123; Gittensor profile; 176 PR(s), 5 issue(s).
Improvement ✅ Minor risk: clean · value: minor · LLM: moderate
Linked issue satisfaction

Addressed
The diff wraps both DELETE and UPDATE writes in finalizeRelayFailureRetryRow in a try/catch that logs a structured console.error with delivery_id, event_name, outcome, and swallows the error rather than propagating it, and adds two regression tests simulating DB write failures on the DELETE and UPDATE paths that assert retryFailedRelays resolves without throwing and logs the alertable message.

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: not available
  • Official Gittensor activity: 176 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 commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

LoopOver is closing this pull request on the maintainer's behalf (CI is failing (codecov/patch)). This is an automated maintenance action — to pursue this change, please open a new pull request with the issues resolved. Closed PRs may be analyzed later to improve review accuracy, but they are not automatically reopened or re-reviewed.

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