Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 22 additions & 1 deletion src/selfhost/pg-queue.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<void> {
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<number> {
const now = Date.now();
const res = await pool.query(
Expand Down Expand Up @@ -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;
Expand Down
22 changes: 21 additions & 1 deletion src/selfhost/sqlite-queue.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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;
Expand Down
26 changes: 26 additions & 0 deletions test/unit/selfhost-pg-queue.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 () => {
Expand Down
23 changes: 23 additions & 0 deletions test/unit/selfhost-sqlite-queue.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<?")) throw new Error("disk I/O error");
return realQuery(sql, params);
});
const errorSpy = vi.spyOn(console, "error").mockImplementation(() => 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 () => {
Expand Down
Loading