diff --git a/src/core/minions/queue.ts b/src/core/minions/queue.ts index e21eb8a41..83ae17b67 100644 --- a/src/core/minions/queue.ts +++ b/src/core/minions/queue.ts @@ -471,12 +471,34 @@ export class MinionQueue { }); } - /** Re-queue a failed or dead job for retry. */ + /** + * Re-queue a failed or dead job for retry. + * + * #2783: an explicit `jobs retry` is an operator asserting "run this + * fresh" — so it clears `started_at` (re-stamped on re-claim via + * `claim()`'s `COALESCE(started_at, now())`, `queue.ts:620`) and resets + * `attempts_made`/`attempts_started` to 0. Without this, `started_at` + * kept the ORIGINAL first-claim time, so `handleWallClockTimeouts()` + * (anchored on `now() - started_at`, `queue.ts:729-749`) could measure + * from long before the retry — a retry issued more than `timeout_ms * 2` + * after the original claim was dead-lettered again in under a second, + * with `attempts_made` already past `max_attempts`. This made retry + * useless for exactly the case it exists for: recovering work after an + * outage that outlasted the job's timeout. + * + * Also resets `stalled_counter` (Codex review): `handleStalled()` + * dead-letters once `stalled_counter + 1 >= max_stalled` (`queue.ts:1190`). + * A job dead-lettered BY stall exhaustion, left un-reset, would hit that + * same threshold on its very first lock expiry after retry — a job + * killed by 3 stalls doesn't get a fresh stall budget, contradicting + * "run this fresh" the same way the unreset attempt counters did. + */ async retryJob(id: number): Promise { const rows = await this.engine.executeRaw>( `UPDATE minion_jobs SET status = 'waiting', error_text = NULL, lock_token = NULL, lock_until = NULL, delay_until = NULL, - finished_at = NULL, updated_at = now() + finished_at = NULL, started_at = NULL, attempts_made = 0, + attempts_started = 0, stalled_counter = 0, updated_at = now() WHERE id = $1 AND status IN ('failed', 'dead') RETURNING *`, [id] diff --git a/test/minions.test.ts b/test/minions.test.ts index 012e91bdd..b1d7d6da9 100644 --- a/test/minions.test.ts +++ b/test/minions.test.ts @@ -735,6 +735,99 @@ describe('MinionQueue: Cancel & Retry', () => { expect(retried!.status).toBe('waiting'); expect(retried!.error_text).toBeNull(); }); + + // #2783: retry must reset started_at/attempts_made/attempts_started/ + // stalled_counter — an explicit `jobs retry` is an operator asserting + // "run this fresh". + test('retry resets started_at/attempts_made/attempts_started/stalled_counter', async () => { + const job = await queue.add('sync', {}, { max_attempts: 3, max_stalled: 3 }); + await queue.claim('tok1', 30000, 'default', ['sync']); + await queue.failJob(job.id, 'tok1', 'error', 'dead'); + // Simulate the original claim having stamped started_at long ago, + // attempts already elevated, and a near-exhausted stall budget — + // matching what a real dead job (killed by wall-clock OR by stall + // exhaustion) looks like. + await engine.executeRaw( + "UPDATE minion_jobs SET started_at = now() - interval '1 hour', stalled_counter = 2 WHERE id = $1", + [job.id], + ); + const retried = await queue.retryJob(job.id); + expect(retried!.status).toBe('waiting'); + expect(retried!.started_at).toBeNull(); + expect(retried!.attempts_made).toBe(0); + expect(retried!.attempts_started).toBe(0); + expect(retried!.stalled_counter).toBe(0); + }); + + // #2783 repro: retry issued long after the original claim must NOT be + // immediately dead-lettered by the wall-clock sweep on re-claim. + test('retry survives handleWallClockTimeouts after re-claim, even long after the original attempt', async () => { + const job = await queue.add('sync', {}, { max_attempts: 3 }); + await engine.executeRaw('UPDATE minion_jobs SET timeout_ms = 1000 WHERE id = $1', [job.id]); + await queue.claim('tok1', 30000, 'default', ['sync']); + // Original attempt dies from a wall-clock timeout — matches the issue's + // repro (an outage that outlasts timeout_ms). + await engine.executeRaw( + "UPDATE minion_jobs SET started_at = now() - interval '10 seconds' WHERE id = $1", + [job.id], + ); + const firstDead = await queue.handleWallClockTimeouts(30000); + expect(firstDead.length).toBe(1); + expect(firstDead[0].status).toBe('dead'); + + // Outage clears; operator retries — LONG after the original claim time + // (this is the exact scenario that used to dead-letter in <1s: without + // the fix, started_at would still be the original claim's timestamp). + await queue.retryJob(job.id); + const reclaimed = await queue.claim('tok2', 30000, 'default', ['sync']); + expect(reclaimed).not.toBeNull(); + expect(reclaimed!.attempts_made).toBe(0); + + // The sweep must NOT kill it immediately this time — started_at was + // re-stamped fresh on re-claim (claim()'s COALESCE(started_at, now())). + const stillAlive = await queue.handleWallClockTimeouts(30000); + expect(stillAlive.length).toBe(0); + expect((await queue.getJob(job.id))!.status).toBe('active'); + }); + + // #2783 repro (stall side): a job dead-lettered by stall exhaustion must + // get a fresh stall budget on retry, not immediately re-die on its first + // stall after being re-claimed. + test('retry survives one stall after re-claim, even after the original stall budget was exhausted', async () => { + const job = await queue.add('sync', {}, { max_attempts: 3, max_stalled: 2 }); + + // Exhaust the stall budget the same way the existing stall test does: + // one requeue stall, then one dead-lettering stall. + await queue.claim('tok1', 30000, 'default', ['sync']); + await engine.executeRaw( + "UPDATE minion_jobs SET lock_until = now() - interval '1 second' WHERE id = $1", + [job.id], + ); + await queue.handleStalled(); + await queue.claim('tok2', 30000, 'default', ['sync']); + await engine.executeRaw( + "UPDATE minion_jobs SET lock_until = now() - interval '1 second' WHERE id = $1", + [job.id], + ); + const r2 = await queue.handleStalled(); + expect(r2.dead.length).toBe(1); + expect(r2.dead[0].status).toBe('dead'); + expect(r2.dead[0].stalled_counter).toBe(2); // == max_stalled — exhausted + + // Operator retries. Without the stalled_counter reset, the very next + // stall would immediately satisfy `stalled_counter + 1 >= max_stalled` + // and dead-letter again despite "run this fresh". + const retried = await queue.retryJob(job.id); + expect(retried!.stalled_counter).toBe(0); + await queue.claim('tok3', 30000, 'default', ['sync']); + await engine.executeRaw( + "UPDATE minion_jobs SET lock_until = now() - interval '1 second' WHERE id = $1", + [job.id], + ); + const r3 = await queue.handleStalled(); + expect(r3.requeued.length).toBe(1); // fresh budget — requeued, not dead + expect(r3.dead.length).toBe(0); + }); }); // --- Pause / Resume (5 tests) ---