From 3062859420a7a7e3cb778a3d3d5457facbc5a754 Mon Sep 17 00:00:00 2001 From: Masa <98894508+Masashi-Ono0611@users.noreply.github.com> Date: Sat, 1 Aug 2026 04:39:20 +0900 Subject: [PATCH] fix(autopilot): derive the full-cycle timeout floor from the handler anchors (#2781) (#3656) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adversarial review: survived a hostile reviewer plus two independent refuters, each told to assume the PR was broken and to default to refuting when uncertain. 19 of 62 PRs cleared that bar. Full-cycle maintenance jobs were stamped with the outer 600s timeout instead of the 30-minute handler anchor — a regression from #3338 that killed long cycles mid-run. Fixed with a named `fullCycleTimeoutMs` derived from the handler anchors, which now fail loudly rather than silently defaulting; reverting fails 3 of 8 tests. Verified before merge: the PR's own tests fail when the production change is reverted; typecheck clean; MERGEABLE/CLEAN on the current base after batches 1 and 2 landed, not a stale one. Known gap, recorded rather than hidden: the '38 dead cycles in 24h' figure from the description was not reproduced; the stamp arithmetic was verified by code inspection. --- src/commands/autopilot-timeout.ts | 35 ++++++++++- src/commands/autopilot.ts | 17 ++++-- test/autopilot-fanout-wiring.test.ts | 87 +++++++++++++++++++++++++++- 3 files changed, 132 insertions(+), 7 deletions(-) diff --git a/src/commands/autopilot-timeout.ts b/src/commands/autopilot-timeout.ts index 0ef6b5b59..c8189a9d3 100644 --- a/src/commands/autopilot-timeout.ts +++ b/src/commands/autopilot-timeout.ts @@ -1,9 +1,42 @@ +import { defaultTimeoutMsFor } from '../core/minions/handler-timeouts.ts'; + +// #2781: the full-cycle floor used to be a literal `1_800_000` that merely +// HAPPENED to match the 'autopilot-cycle' / 'autopilot-global-maintenance' +// handler anchors (`HANDLER_DEFAULT_TIMEOUT_MS`, #1737) instead of being +// derived from them. A duplicated literal can silently drift from the +// handler default it's supposed to track — which is exactly the bug class +// #2781 reported (an explicit `timeout_ms` stamp permanently overrides the +// handler default per `queue.ts`'s `opts?.timeout_ms ?? defaultTimeoutMsFor`, +// so a stale/lower literal here would starve a phase the handler default +// was sized for). Deriving the floor from `defaultTimeoutMsFor` for both +// full-cycle job names keeps the stamp coupled to its anchor by construction. +// Fail fast (not `?? 0`) if either handler ever loses its entry in +// HANDLER_DEFAULT_TIMEOUT_MS — silently falling back to "no floor" would +// reintroduce #2781 rather than surface the drift. +function requireHandlerAnchorMs(jobName: string): number { + const ms = defaultTimeoutMsFor(jobName); + if (ms === null) { + throw new Error( + `resolveAutopilotDispatchTimeoutMs: '${jobName}' has no entry in HANDLER_DEFAULT_TIMEOUT_MS ` + + '(handler-timeouts.ts) — the full-cycle timeout floor can no longer be derived from it. ' + + 'See #2781: a missing/removed anchor here silently reintroduces the interval-derived stamp ' + + 'permanently overriding the handler default.', + ); + } + return ms; +} + +const FULL_CYCLE_TIMEOUT_FLOOR_MS = Math.max( + requireHandlerAnchorMs('autopilot-cycle'), + requireHandlerAnchorMs('autopilot-global-maintenance'), +); + export function resolveAutopilotDispatchTimeoutMs( baseIntervalSeconds: number, fullCycle: boolean, ): number { const intervalDerivedTimeoutMs = Math.max(baseIntervalSeconds * 2 * 1000, 300_000); return fullCycle - ? Math.max(intervalDerivedTimeoutMs, 1_800_000) + ? Math.max(intervalDerivedTimeoutMs, FULL_CYCLE_TIMEOUT_FLOOR_MS) : intervalDerivedTimeoutMs; } diff --git a/src/commands/autopilot.ts b/src/commands/autopilot.ts index dd5598713..fcd15f53b 100644 --- a/src/commands/autopilot.ts +++ b/src/commands/autopilot.ts @@ -981,12 +981,21 @@ export async function runAutopilot(engine: BrainEngine, args: string[]) { // can't shrink throughput (codex #9/D5). autopilot-cycle jobs run on // the 'default' queue, so that's the concurrency we compare against. const fanoutMax = await resolveEffectiveFanoutMax(engine, 'default'); + // #2781: both 'autopilot-cycle' (per-source) and 'autopilot-global- + // maintenance' carry a 30-min handler anchor (handler-timeouts.ts) + // because a full cycle can outlive short daemon intervals — unlike + // the lighter interval-derived `timeoutMs` above (sync/freshness, + // extract-atoms-drain, targeted small-plan steps), which have no + // such anchor and are meant to stay interval-derived. Naming this + // separately (rather than reusing the outer `timeoutMs`) avoids + // the #2781 bug class: dispatchGlobalMaintenance previously reused + // the outer non-full-cycle `timeoutMs` by shorthand, silently + // dropping its own handler anchor. + const fullCycleTimeoutMs = resolveAutopilotDispatchTimeoutMs(baseInterval, true); const result = await dispatchPerSource(engine, queue, { repoPath, slot, - // Full cycles can outlive short daemon intervals. Keep lighter dispatches - // interval-derived while giving per-source consolidation enough time. - timeoutMs: resolveAutopilotDispatchTimeoutMs(baseInterval, true), + timeoutMs: fullCycleTimeoutMs, fanoutMax, jsonMode, }); @@ -997,7 +1006,7 @@ export async function runAutopilot(engine: BrainEngine, args: string[]) { // the per-source path (legacy single-source still runs everything). if (!result.legacy_fallback) { try { - await dispatchGlobalMaintenance(engine, queue, { repoPath, slot, timeoutMs, jsonMode }); + await dispatchGlobalMaintenance(engine, queue, { repoPath, slot, timeoutMs: fullCycleTimeoutMs, jsonMode }); } catch (e) { if (jsonMode) process.stderr.write(JSON.stringify({ event: 'global_maintenance_dispatch_failed', error: e instanceof Error ? e.message : String(e) }) + '\n'); } diff --git a/test/autopilot-fanout-wiring.test.ts b/test/autopilot-fanout-wiring.test.ts index 213b5d7c7..8bee073a7 100644 --- a/test/autopilot-fanout-wiring.test.ts +++ b/test/autopilot-fanout-wiring.test.ts @@ -16,12 +16,18 @@ import { describe, expect, test } from 'bun:test'; import { readFileSync } from 'fs'; import { join } from 'path'; import { resolveAutopilotDispatchTimeoutMs } from '../src/commands/autopilot-timeout.ts'; +import { defaultTimeoutMsFor } from '../src/core/minions/handler-timeouts.ts'; const AUTOPILOT_SRC = readFileSync( join(import.meta.dir, '..', 'src', 'commands', 'autopilot.ts'), 'utf8', ); +const AUTOPILOT_TIMEOUT_SRC = readFileSync( + join(import.meta.dir, '..', 'src', 'commands', 'autopilot-timeout.ts'), + 'utf8', +); + describe('autopilot.ts ↔ dispatchPerSource wiring', () => { test('imports dispatchPerSource from the fan-out helper', () => { expect(AUTOPILOT_SRC).toMatch( @@ -59,9 +65,33 @@ describe('autopilot.ts ↔ dispatchPerSource wiring', () => { expect(AUTOPILOT_SRC).toContain( 'const timeoutMs = resolveAutopilotDispatchTimeoutMs(baseInterval, false);', ); - expect(AUTOPILOT_SRC).toMatch( - /dispatchPerSource\(engine, queue, \{[\s\S]{0,300}timeoutMs: resolveAutopilotDispatchTimeoutMs\(baseInterval, true\)/, + expect(AUTOPILOT_SRC).toContain( + 'const fullCycleTimeoutMs = resolveAutopilotDispatchTimeoutMs(baseInterval, true);', ); + expect(AUTOPILOT_SRC).toMatch( + /dispatchPerSource\(engine, queue, \{[\s\S]{0,300}timeoutMs: fullCycleTimeoutMs/, + ); + }); + + test('#2781: dispatchGlobalMaintenance gets the full-cycle floor, not the outer (non-full-cycle) timeoutMs', () => { + // Live #2781 regression, found in review: dispatchGlobalMaintenance's + // call used the object-shorthand `timeoutMs`, which resolved to the + // OUTER `const timeoutMs = resolveAutopilotDispatchTimeoutMs(baseInterval, false)` + // declared earlier in the same function for the sync/freshness dispatch + // — not to the full-cycle value computed for dispatchPerSource a few + // lines above it. 'autopilot-global-maintenance' carries the same + // 30-min handler anchor as 'autopilot-cycle' (handler-timeouts.ts), so + // this silently starved brain-wide maintenance (embed/orphans/purge/…) + // at exactly the #2781 symptom (600s budget at the default 300s + // interval) even after the per-source path was fixed. Pin the correct + // wiring by source-shape: the call must pass the *full-cycle* variable. + const dispatchGlobalIdx = AUTOPILOT_SRC.indexOf('dispatchGlobalMaintenance(engine, queue'); + expect(dispatchGlobalIdx).toBeGreaterThan(-1); + const dispatchGlobalCall = AUTOPILOT_SRC.slice(dispatchGlobalIdx, dispatchGlobalIdx + 200); + expect(dispatchGlobalCall).toContain('timeoutMs: fullCycleTimeoutMs'); + // Guard against the exact regression: the shorthand `timeoutMs` (bare, + // no colon) resolving to the non-full-cycle outer const. + expect(dispatchGlobalCall).not.toMatch(/\{\s*repoPath,\s*slot,\s*timeoutMs,/); }); test('updates lastFullCycleAt on dispatch (so the 60-min floor is honored)', () => { @@ -70,6 +100,59 @@ describe('autopilot.ts ↔ dispatchPerSource wiring', () => { expect(AUTOPILOT_SRC).toMatch(/lastFullCycleAt\s*=\s*Date\.now\(\)/); }); + test('#2781: full-cycle floor is derived from BOTH handler anchors, not a duplicated literal', () => { + // #2781's root cause: autopilot stamped an explicit `timeout_ms` that was + // only a `Math.max(interval-derived, 300_000)`-shaped literal, so it + // silently overrode the 'autopilot-cycle' handler's own #1737 anchor + // (`queue.ts`: an explicit stamp always wins over `defaultTimeoutMsFor`). + // A prior fix (#2852) hardcoded a matching `1_800_000` floor for + // full-cycle dispatch, but a literal that merely happens to equal the + // handler anchor can drift from it again if the anchor is ever retuned + // in handler-timeouts.ts without a matching edit here — reintroducing + // the exact #2781 bug class. + // + // Prove the floor is *derived* (not just numerically coincidental) two + // ways: (a) it equals Math.max of BOTH job names' anchors — a bare + // duplicated literal could accidentally match a SINGLE anchor (as the + // prior #2852 fix did) but wiring `Math.max(cycle, global)` is what + // actually protects a future divergence between the two anchors; (b) a + // source-shape check that both job-name string literals reach + // `defaultTimeoutMsFor` (directly or via a thin wrapper), and that no + // bare numeric literal sits in the full-cycle branch. + const cycleAnchorMs = defaultTimeoutMsFor('autopilot-cycle'); + const globalAnchorMs = defaultTimeoutMsFor('autopilot-global-maintenance'); + if (cycleAnchorMs === null) throw new Error("expected a handler anchor for 'autopilot-cycle'"); + if (globalAnchorMs === null) throw new Error("expected a handler anchor for 'autopilot-global-maintenance'"); + const expectedFloorMs = Math.max(cycleAnchorMs, globalAnchorMs); + + // A short interval collapses the interval-derived component to its + // 300_000ms minimum, so the full-cycle result must equal the derived + // floor exactly. + expect(resolveAutopilotDispatchTimeoutMs(1, true)).toBe(expectedFloorMs); + // A regular (non-full-cycle) dispatch — e.g. the 'sync' freshness job, + // which has no long-job handler anchor — must NOT pick up the floor. + expect(resolveAutopilotDispatchTimeoutMs(1, false)).toBe(300_000); + + // Guard against reintroducing a hardcoded literal floor directly in the + // full-cycle branch instead of the derived FULL_CYCLE_TIMEOUT_FLOOR_MS. + expect(AUTOPILOT_TIMEOUT_SRC).not.toMatch(/fullCycle\s*\?\s*Math\.max\([^)]*,\s*1_?800_?000\)/); + // Pin the derivation END TO END in source shape (codex round-2): both + // job-name anchor lookups must participate in the floor's Math.max, and + // the full-cycle branch must consume that derived const — otherwise the + // floor could be swapped back to a bare literal while the wrapper, + // import, and job-name strings survive as dead code and the assertions + // above still pass. + expect(AUTOPILOT_TIMEOUT_SRC).toMatch( + /FULL_CYCLE_TIMEOUT_FLOOR_MS\s*=\s*Math\.max\(\s*requireHandlerAnchorMs\('autopilot-cycle'\),\s*requireHandlerAnchorMs\('autopilot-global-maintenance'\),?\s*\)/, + ); + expect(AUTOPILOT_TIMEOUT_SRC).toMatch( + /fullCycle\s*\?\s*Math\.max\(intervalDerivedTimeoutMs,\s*FULL_CYCLE_TIMEOUT_FLOOR_MS\)/, + ); + // The wrapper itself must consult defaultTimeoutMsFor (fail-loud on a + // missing anchor, never a numeric fallback). + expect(AUTOPILOT_TIMEOUT_SRC).toMatch(/requireHandlerAnchorMs[\s\S]{0,200}defaultTimeoutMsFor\(jobName\)/); + }); + test('does NOT regress to the single-job dispatch on the full-cycle path', () => { // Pre-PR: the shouldFullCycle branch did: // const job = await queue.add('autopilot-cycle', { repoPath }, {