Skip to content

fix(selfhost): guard the dead-letter revive interval against unhandled errors#2597

Merged
JSONbored merged 1 commit into
mainfrom
fix/selfhost-dead-letter-revive-interval-crash
Jul 2, 2026
Merged

fix(selfhost): guard the dead-letter revive interval against unhandled errors#2597
JSONbored merged 1 commit into
mainfrom
fix/selfhost-dead-letter-revive-interval-crash

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

Summary

Follow-up to #2581 (already merged), which shipped automatic dead-letter job revival for both self-host queue backends. A later review pass on that PR flagged a second real defect that landed after the PR's blocking race-condition issue was already fixed and merged, so this is a fresh PR against current main rather than an amendment to the closed one.

The defect: both backends scheduled reviveDeadLetterJobs directly from setInterval with no error handler of its own:

// pg-queue.ts
deadLetterReviveTimer = setInterval(() => void reviveDeadLetterJobs(), queueDeadLetterReviveIntervalMs());
// sqlite-queue.ts
deadLetterReviveTimer = setInterval(reviveDeadLetterJobs, queueDeadLetterReviveIntervalMs());

A transient pool/driver/metric failure on that background tick surfaces as an unhandled promise rejection (Postgres — the function is async and its rejection is discarded via void) or an uncaught exception (SQLite — the function is synchronous). Either can terminate the self-host process, turning a feature meant to bound retry storms into a process-crash vector of its own.

What changed

Both backends now wrap the interval callback in a new reviveDeadLetterJobsSafely() that mirrors the exact try/catch + structured console.error + captureError pattern pump() already uses for the main poll loop (see selfhost_queue_pump_crashed — this fix adds the analogous selfhost_queue_dead_letter_revive_crashed). A failed revive tick now just waits for the next interval, same as a failed poll tick waits for the next poll.

The public reviveDeadLetterJobs() — the one tests and any future operator-triggered repair path call directly — is untouched; only the internal setInterval callback is rewired to the safe wrapper, so direct callers still see real errors/rejections if they want to handle them themselves.

Tests

Added one regression test per backend that injects a pool/driver failure specifically on the revive interval's own tick (via fake timers + vi.advanceTimersByTimeAsync) and asserts the process survives and the failure is logged — mirroring the existing "pump absorbs a ... failure instead of crashing" tests already in both suites for the main poll loop.

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.
  • No issue is linked — this is a direct maintainer follow-up from a review on the now-merged feat(selfhost): add automatic dead-letter job retry for self-host queues #2581, not tied to a filed issue.

Validation

  • git diff --check
  • npm run actionlint
  • npm run typecheck
  • npm run test:coverage locally — both changed src/** files (pg-queue.ts, sqlite-queue.ts) are 100% line-covered; the remaining uncovered branches are pre-existing and unrelated (cross-checked directly against the diff).
  • 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

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 changes.
  • API/OpenAPI/MCP behavior is updated and tested where needed. — no OpenAPI/MCP surface touched; ui:openapi:check confirms no drift.
  • UI changes use live API data or real empty/error/loading states, not production mock/demo fallbacks. — N/A, backend-only change.
  • Visible UI changes include a UI Evidence section below with screenshots. — N/A, backend-only change.
  • Public docs/changelogs are updated where needed. — N/A, no user-facing docs affected.

…d errors

The AI review flagged a second defect: both backends scheduled
reviveDeadLetterJobs directly from setInterval with no error handler
of its own. A transient pool/driver/metric failure on that background
tick would surface as an unhandled promise rejection (Postgres, async)
or an uncaught exception (SQLite, sync), either of which can terminate
the self-host process -- turning a bounded revival feature into a
process-crash vector.

Both backends now wrap the interval callback (reviveDeadLetterJobsSafely)
in the exact same try/catch + structured console.error + captureError
pattern pump() already uses for the main poll loop, so a failed revive
tick just waits for the next interval instead of taking the process
down. The public reviveDeadLetterJobs() the tests and any future
operator-triggered repair path call directly is untouched -- only the
internal timer callback is wrapped.

Added a regression test per backend that injects a pool/driver failure
specifically on the revive interval's own tick (via fake timers) and
asserts the process survives and the failure is logged, mirroring the
existing "pump absorbs a ... failure instead of crashing" tests for the
main poll loop.
@dosubot dosubot Bot added the size:M label Jul 2, 2026
@loopover-orb

loopover-orb Bot commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

Tip

🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩

✅ Gittensory review result - approve/merge recommended

Review updated: 2026-07-02 20:15:21 UTC

4 files · 1 AI reviewer · no blockers · readiness 91/100 · CI green · clean

✅ Suggested Action - Approve/Merge

  • safe to merge

Review summary
The change correctly moves the recurring dead-letter revive work behind a backend-local safe wrapper, so interval-triggered failures are logged and reported instead of escaping the timer callback. The public revive functions remain unchanged for direct callers, and the added regression tests exercise the real interval path for both queue backends. The notable remaining issue is test cleanup hygiene around fake timers/environment state, not the production behavior.

Nits — 5 non-blocking
  • nit: `test/unit/selfhost-sqlite-queue.test.ts:1315` sets `QUEUE_DEAD_LETTER_REVIVE_INTERVAL_MS` but does not delete it in a local `finally`, so this test relies on outer cleanup that is not visible in the diff.
  • nit: `test/unit/selfhost-pg-queue.test.ts:1012` and `test/unit/selfhost-sqlite-queue.test.ts:1316` switch to fake timers without locally restoring real timers, which makes the tests more fragile if they are moved or copied outside the current harness.
  • nit: `src/selfhost/pg-queue.ts:253` and `src/selfhost/sqlite-queue.ts:187` add large block comments that duplicate the PR narrative; the operationally useful part could be shorter now that the wrapper name and event make the intent clear.
  • In `test/unit/selfhost-sqlite-queue.test.ts:1315`, wrap the test body in `try/finally` like the pg test and delete `QUEUE_DEAD_LETTER_REVIVE_INTERVAL_MS` there.
  • In both new tests, put `await q.stop()` in cleanup so a failed assertion does not leave an interval behind.
Signal Result Evidence
Code review ✅ No blockers 1 reviewer
Linked issue ⚠️ Missing No linked issue or no-issue rationale found.
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 (size label size:M; no linked issue context).
Validation posture ✅ 25/25 PR body includes validation/test evidence.
Contributor workload ✅ 10/10 Author activity: 64 registered-repo PR(s), 55 merged, 526 issue(s).
Contributor context ✅ Confirmed Gittensor contributor JSONbored; Gittensor profile; 64 PR(s), 526 issue(s).
Gate result ✅ Passing No configured blocker found.
Review context
  • Author: JSONbored
  • Role context: owner (maintainer lane)
  • 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: 64 PR(s), 526 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Treat this as maintainer-lane context rather than normal contributor-lane activity.
  • Explain no-issue PR.
  • No action.
  • Link the issue being solved, or explicitly explain why this is a no-issue PR.
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 Jul 2, 2026
@codecov

codecov Bot commented Jul 2, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 96.02%. Comparing base (2af7092) to head (8ec875c).
⚠️ Report is 10 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2597      +/-   ##
==========================================
+ Coverage   95.99%   96.02%   +0.02%     
==========================================
  Files         230      233       +3     
  Lines       25887    26081     +194     
  Branches     9410     9474      +64     
==========================================
+ Hits        24851    25045     +194     
  Misses        425      425              
  Partials      611      611              
Files with missing lines Coverage Δ
src/selfhost/sqlite-queue.ts 99.31% <100.00%> (+0.05%) ⬆️
🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@JSONbored JSONbored self-assigned this Jul 2, 2026
@JSONbored
JSONbored merged commit 6f46056 into main Jul 2, 2026
13 checks passed
@JSONbored
JSONbored deleted the fix/selfhost-dead-letter-revive-interval-crash branch July 2, 2026 20:20
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.

Development

Successfully merging this pull request may close these issues.

1 participant