fix(minions): handleTimeouts counts the timed-out run as a spent attempt (#1737)

The per-job timeout_at dead-letter (handleTimeouts) set status='dead' without
incrementing attempts_made, unlike the wall-clock and stall dead-letter siblings.
It is the FIRST killer to fire for the long-lane handlers (subagent / embed-backfill
/ autopilot-cycle) because timeout_ms is stamped at submit, so a timed-out long job
reported `attempts: 0/N (started: N)`. Mirror the siblings with attempts_made + 1
(terminal, no retry). Safe against double-count: the worker sweep runs handleStalled
-> handleTimeouts -> handleWallClockTimeouts sequentially and awaited, each guarded on
status='active', so the first to dead-letter excludes the row from the rest.

Regression assertions added (test/minions.test.ts + e2e/minions-resilience.test.ts)
so the increment can't be silently dropped.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
Garry Tan
2026-06-18 00:57:08 -07:00
co-authored by Claude Opus 4.8
parent f4a0f899ad
commit bbde573ca6
3 changed files with 19 additions and 0 deletions
+9
View File
@@ -647,9 +647,18 @@ export class MinionQueue {
async handleTimeouts(): Promise<MinionJob[]> {
return this.engine.transaction(async (tx) => {
const rows = await tx.executeRaw<Record<string, unknown>>(
// #1737: count the timed-out run as a spent attempt (terminal, no retry),
// mirroring handleWallClockTimeouts + handleStalled. handleTimeouts is the
// FIRST killer to fire for the long-lane handlers (timeout_ms stamped at
// submit), so without this the job reads `attempts: 0/N (started: N)`.
// Safe against double-count: the worker sweep runs handleStalled ->
// handleTimeouts -> handleWallClockTimeouts sequentially and awaited, and
// each guards on `status = 'active'`, so the first to set status='dead'
// excludes the row from the later sweeps.
`UPDATE minion_jobs SET
status = 'dead',
error_text = 'timeout exceeded',
attempts_made = attempts_made + 1,
lock_token = NULL,
lock_until = NULL,
finished_at = now(),
+4
View File
@@ -138,6 +138,10 @@ describeE2E('E2E: Minions resilience (OpenClaw real-world patterns)', () => {
const final = await queue.getJob(job.id);
expect(final?.error_text).toMatch(/timeout exceeded/i);
// #1737 regression: the runaway run is counted as a spent attempt by
// handleTimeouts (it's the first killer to fire), so accounting reads
// honestly instead of `attempts: 0/1 (started: 1)`.
expect(final?.attempts_made).toBe(1);
} finally {
await a.disconnect();
await b.disconnect();
+6
View File
@@ -1300,6 +1300,12 @@ describe('MinionQueue: handleTimeouts', () => {
const dead = await queue.getJob(job.id);
expect(dead!.status).toBe('dead');
expect(dead!.error_text).toBe('timeout exceeded');
// #1737 regression: the timed-out run counts as a spent attempt, mirroring
// the wall-clock + stall dead-letter paths. Without this the job reads
// `attempts: 0/N (started: N)`. Asserted on both the RETURNING row and the
// persisted row so a future refactor can't silently drop the increment.
expect(timedOut[0].attempts_made).toBe(1);
expect(dead!.attempts_made).toBe(1);
});
test('handleTimeouts ignores stalled jobs (lock_until > now guard)', async () => {