Skip to content

fix(selfhost): catch unhandled pump() rejections in sqlite-queue/pg-queue#2544

Merged
JSONbored merged 1 commit into
mainfrom
fix/queue-pump-unhandled-rejection
Jul 2, 2026
Merged

fix(selfhost): catch unhandled pump() rejections in sqlite-queue/pg-queue#2544
JSONbored merged 1 commit into
mainfrom
fix/queue-pump-unhandled-rejection

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

Summary

  • kickOne()/kickAll() in sqlite-queue.ts/pg-queue.ts fire pump() via bare void pump() with no .catch(). claimNext()/reclaimExpiredProcessingJobs() run OUTSIDE processOne()'s own try/finally, so a raw driver failure (a locked SQLite file, a dropped Postgres connection/lock timeout) propagated out of pump() uncaught.
  • server.ts only registers process.on('unhandledRejection', ...) when SENTRY_DSN is set. Since Sentry is opt-in, a single transient DB error surfacing from claimNext()/reclaimExpiredProcessingJobs() could silently crash the entire self-host server — no metric bump, no Sentry event, no /ready degradation warning beforehand.
  • Wrapped pump()'s body in a try/catch that logs (selfhost_queue_pump_crashed) and calls captureError instead of letting the exception propagate, in both queue drivers.
  • Regression tests: mock the driver/pool to throw on the claimNext() query and separately on the reclaimExpiredProcessingJobs() query, in both sqlite-queue.ts and pg-queue.ts; assert drain() resolves cleanly (no unhandled rejection) and the failure is logged.

Closes #2498.

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 an issue, or this is small enough that the summary explains why an issue is not needed.

Validation

  • git diff --check
  • npm run actionlint
  • npm run typecheck
  • npm run test:coverage locally; codecov/patch requires ≥99% coverage of the lines AND branches you changed (aim for 100% on your diff so CI variance does not fail near the threshold). Global coverage is a non-blocking trend with a loose 90% backstop, not the gate.
  • 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/cookie/CORS changes.
  • API/OpenAPI/MCP behavior is updated and tested where needed. — N/A, no 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 changes.
  • Visible UI changes include a UI Evidence section below — N/A, no UI changes.
  • Public docs/changelogs are updated where needed; changelogs are only edited for release-prep PRs.

Notes

…ueue

claimNext()/reclaimExpiredProcessingJobs() run outside processOne()'s own
try/finally in both queue drivers, so a raw driver failure (a locked
SQLite file, a dropped Postgres connection) propagated out of pump()
uncaught. Every void pump() call site (kickOne/kickAll) is fire-and-forget,
so this surfaced as an unhandled promise rejection — fatal when
SENTRY_DSN is unset, since server.ts only installs the unhandledRejection
handler when Sentry is configured. A single transient DB error could
silently kill the whole self-host server.

Wrap pump()'s body in a try/catch that logs and captures the failure
instead of letting it propagate, matching the pattern already used
throughout processOne() for job-level errors.
@dosubot dosubot Bot added the size:S label Jul 2, 2026
@loopover-orb

loopover-orb Bot commented Jul 2, 2026

Copy link
Copy Markdown

Tip

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

✅ Gittensory review result - approve/merge recommended

Review updated: 2026-07-02 09:01:58 UTC

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

✅ Suggested Action - Approve/Merge

  • safe to merge

Review summary
The change wraps the queue pump loop in both self-host queue backends so driver failures from the pre-processing claim/reclaim path are logged and reported instead of escaping through fire-and-forget pump calls. The tests cover both claim and reclaim failures for SQLite and Postgres and assert that drain resolves after the injected backend error. The visible diff is narrowly scoped and fixes the described unhandled rejection path without changing normal job processing semantics.

Nits — 6 non-blocking
  • nit: src/selfhost/pg-queue.ts:557 and src/selfhost/sqlite-queue.ts:500 duplicate the same structured pump-crash logging block, so future changes to event shape or reporting context can drift between the two queue drivers.
  • nit: test/unit/selfhost-pg-queue.test.ts:1186 and test/unit/selfhost-sqlite-queue.test.ts:1764 match raw SQL substrings to inject failures; that is workable for this regression but brittle against harmless query formatting or column-order changes.
  • nit: test/unit/selfhost-pg-queue.test.ts:1198 and test/unit/selfhost-sqlite-queue.test.ts:1775 only assert substring presence in the emitted JSON line, so they do not verify the structured `level`, `event`, and `error` fields stay parseable.
  • Extract a small shared helper for the `selfhost_queue_pump_crashed` log/capture shape or at least keep the two driver implementations adjacent in review when changing it again.
  • In the regression tests, parse the logged JSON and assert `event === "selfhost_queue_pump_crashed"` plus the expected error text instead of relying only on `includes()`.
  • PR author also opened the linked issue — Link an issue that was opened by a different contributor, or provide a rationale for why this self-authored issue represents genuine discovery work.
Signal Result Evidence
Code review ✅ No blockers 1 reviewer
Linked issue ✅ Linked #2498
Related work ⚠️ 3 scoped overlaps Top overlaps are listed below; lower-confidence bulk is hidden.
Change scope ❌ 8/20 High review scope from cached public metadata (size label size:S; 1 linked issue).
Validation posture ✅ 25/25 PR body includes validation/test evidence.
Contributor workload ✅ 10/10 Author activity: 65 registered-repo PR(s), 55 merged, 548 issue(s).
Contributor context ✅ Confirmed Gittensor contributor JSONbored; Gittensor profile; 65 PR(s), 548 issue(s).
Gate result ✅ Passing No configured blocker found.
Review context
Contributor next steps
  • Treat this as maintainer-lane context rather than normal contributor-lane activity.
  • Review top overlaps.
  • Add a concise scope and risk note.
  • No action.
  • Check active issues and PRs before submitting.
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
@JSONbored JSONbored self-assigned this 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 95.97%. Comparing base (0288f5f) to head (c41827e).
⚠️ Report is 28 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2544      +/-   ##
==========================================
+ Coverage   95.95%   95.97%   +0.01%     
==========================================
  Files         226      228       +2     
  Lines       25423    25711     +288     
  Branches     9244     9349     +105     
==========================================
+ Hits        24395    24676     +281     
- Misses        417      425       +8     
+ Partials      611      610       -1     
Files with missing lines Coverage Δ
src/selfhost/sqlite-queue.ts 99.25% <100.00%> (+<0.01%) ⬆️

... and 2 files with indirect coverage changes

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@JSONbored
JSONbored merged commit a9d6e0c into main Jul 2, 2026
13 checks passed
@JSONbored
JSONbored deleted the fix/queue-pump-unhandled-rejection branch July 2, 2026 09:20
@github-project-automation github-project-automation Bot moved this from Todo to Done in gittensory - v1 roadmap Jul 2, 2026
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

No open projects
Status: Done

Development

Successfully merging this pull request may close these issues.

fix(selfhost): catch unhandled pump() rejections in sqlite-queue/pg-queue

1 participant