From 8ec875c2ae63c3d98fce28a8416bd07b22e407ea Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Thu, 2 Jul 2026 04:59:51 -0700 Subject: [PATCH] fix(selfhost): guard the dead-letter revive interval against unhandled 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. --- src/selfhost/pg-queue.ts | 23 +++++++++++++++++++++- src/selfhost/sqlite-queue.ts | 22 ++++++++++++++++++++- test/unit/selfhost-pg-queue.test.ts | 26 +++++++++++++++++++++++++ test/unit/selfhost-sqlite-queue.test.ts | 23 ++++++++++++++++++++++ 4 files changed, 92 insertions(+), 2 deletions(-) diff --git a/src/selfhost/pg-queue.ts b/src/selfhost/pg-queue.ts index e02990a1be..e5e152cf14 100644 --- a/src/selfhost/pg-queue.ts +++ b/src/selfhost/pg-queue.ts @@ -253,6 +253,27 @@ export function createPgQueue( return revived; } + /** Wraps reviveDeadLetterJobs() for the setInterval callback below, which has no rejection handler of its + * own -- a transient pool/driver/metric failure here would otherwise surface as an unhandled promise + * rejection and can terminate the process (fatal when SENTRY_DSN is unset, since server.ts only installs + * the handler when Sentry is configured), exactly the failure mode pump()'s own try/catch above guards + * against for the main poll loop. A failed revive tick just waits for the next interval, same as a failed + * poll tick waits for the next poll. */ + async function reviveDeadLetterJobsSafely(): Promise { + try { + await reviveDeadLetterJobs(); + } catch (error) { + console.error( + JSON.stringify({ + level: "error", + event: "selfhost_queue_dead_letter_revive_crashed", + error: errorMessageWithCause(error), + }), + ); + captureError(error, { kind: "queue_dead_letter_revive_crashed" }); + } + } + async function spreadDueJobsOnStartup(): Promise { const now = Date.now(); const res = await pool.query( @@ -666,7 +687,7 @@ export function createPgQueue( // Separate, much slower interval than the poll tick above -- reviving a dead job every second would // recreate the retry storm this feature exists to bound. The interval itself is the cooldown between // auto-retry rounds for any one job. - deadLetterReviveTimer = setInterval(() => void reviveDeadLetterJobs(), queueDeadLetterReviveIntervalMs()); + deadLetterReviveTimer = setInterval(() => void reviveDeadLetterJobsSafely(), queueDeadLetterReviveIntervalMs()); }, async stop() { running = false; diff --git a/src/selfhost/sqlite-queue.ts b/src/selfhost/sqlite-queue.ts index 53d272fe4e..e56696a23d 100644 --- a/src/selfhost/sqlite-queue.ts +++ b/src/selfhost/sqlite-queue.ts @@ -187,6 +187,26 @@ export function createSqliteQueue( return revived; } + /** Wraps reviveDeadLetterJobs() for the setInterval callback below, which has no error handler of its own -- + * a transient driver/metric failure here would otherwise surface as an uncaught exception and can terminate + * the process (fatal when SENTRY_DSN is unset, since server.ts only installs the handler when Sentry is + * configured), exactly the failure mode pump()'s own try/catch above guards against for the main poll loop. + * A failed revive tick just waits for the next interval, same as a failed poll tick waits for the next poll. */ + function reviveDeadLetterJobsSafely(): void { + try { + reviveDeadLetterJobs(); + } catch (error) { + console.error( + JSON.stringify({ + level: "error", + event: "selfhost_queue_dead_letter_revive_crashed", + error: errorMessageWithCause(error), + }), + ); + captureError(error, { kind: "queue_dead_letter_revive_crashed" }); + } + } + function enqueue(message: JobMessage, delaySeconds: number): void { const now = Date.now(); const payload = JSON.stringify(message); @@ -576,7 +596,7 @@ export function createSqliteQueue( // Separate, much slower interval than the poll tick above -- reviving a dead job every second would // recreate the retry storm this feature exists to bound. The interval itself is the cooldown between // auto-retry rounds for any one job. - deadLetterReviveTimer = setInterval(reviveDeadLetterJobs, queueDeadLetterReviveIntervalMs()); + deadLetterReviveTimer = setInterval(reviveDeadLetterJobsSafely, queueDeadLetterReviveIntervalMs()); }, async stop() { running = false; diff --git a/test/unit/selfhost-pg-queue.test.ts b/test/unit/selfhost-pg-queue.test.ts index 8f52ca9b56..3b5a465ef7 100644 --- a/test/unit/selfhost-pg-queue.test.ts +++ b/test/unit/selfhost-pg-queue.test.ts @@ -1006,6 +1006,32 @@ describe("createPgQueue (durable #977)", () => { ); expect(await renderMetrics()).toContain("gittensory_jobs_dead_letter_revived_total 1"); }); + + // REGRESSION (#2581 review defect): the revive interval had no error handler of its own, so a thrown + // pool/metric failure on that tick would surface as an unhandled promise rejection and could terminate the + // process -- exactly the failure mode pump()'s own try/catch already guards against for the main poll loop. + it("survives a reviveDeadLetterJobs() pool failure on the interval tick instead of crashing the process", async () => { + process.env.QUEUE_DEAD_LETTER_REVIVE_INTERVAL_MS = "1000"; + vi.useFakeTimers(); + try { + const fn = vi.fn().mockImplementation(async (sql: unknown) => { + if (String(sql).includes("WHERE status='dead' AND attempts<$1")) throw new Error("connection terminated unexpectedly"); + return { rows: [], rowCount: 0 }; + }); + const pool = { query: fn } as unknown as Pool; + const errorSpy = vi.spyOn(console, "error").mockImplementation(() => undefined); + const q = createPgQueue(pool, async () => undefined, { maxRetries: 1 }); + + q.start(); + await vi.advanceTimersByTimeAsync(1000); // the revive interval fires once + + const logged = errorSpy.mock.calls.map(([line]) => String(line)); + expect(logged.some((line) => line.includes("selfhost_queue_dead_letter_revive_crashed") && line.includes("connection terminated unexpectedly"))).toBe(true); + await q.stop(); + } finally { + delete process.env.QUEUE_DEAD_LETTER_REVIVE_INTERVAL_MS; + } + }); }); it("reschedules GitHub rate-limit failures without consuming the dead-letter budget", async () => { diff --git a/test/unit/selfhost-sqlite-queue.test.ts b/test/unit/selfhost-sqlite-queue.test.ts index b84f16119e..68dcaffc98 100644 --- a/test/unit/selfhost-sqlite-queue.test.ts +++ b/test/unit/selfhost-sqlite-queue.test.ts @@ -1310,6 +1310,29 @@ describe("createSqliteQueue (durable #980)", () => { expect(calls).toBe(1); // the auto-revived job was actually re-attempted await q.stop(); }); + + // REGRESSION (#2581 review defect): the revive interval had no error handler of its own, so a thrown + // driver/metric failure on that tick would surface as an uncaught exception and could terminate the + // process -- exactly the failure mode pump()'s own try/catch already guards against for the main poll loop. + it("survives a reviveDeadLetterJobs() driver failure on the interval tick instead of crashing the process", async () => { + process.env.QUEUE_DEAD_LETTER_REVIVE_INTERVAL_MS = "1000"; + vi.useFakeTimers(); + const driver = makeDriver(); + const realQuery = driver.query.bind(driver); + vi.spyOn(driver, "query").mockImplementation((sql: string, params: unknown[]) => { + if (sql.includes("WHERE status='dead' AND attempts undefined); + const q = createSqliteQueue(driver, async () => undefined, { maxRetries: 1 }); + + q.start(); + await vi.advanceTimersByTimeAsync(1000); // the revive interval fires once -- would throw here if uncaught + + const logged = errorSpy.mock.calls.map(([line]) => String(line)); + expect(logged.some((line) => line.includes("selfhost_queue_dead_letter_revive_crashed") && line.includes("disk I/O error"))).toBe(true); + await q.stop(); + }); }); it("reschedules GitHub rate-limit failures without consuming the dead-letter budget", async () => {