From 2a21ca385fb05d604d3667f43a54b1560834b42a Mon Sep 17 00:00:00 2001 From: unohee Date: Fri, 11 Sep 2026 08:42:39 +0900 Subject: [PATCH] fix(scheduler): idle-fill must not out-race a repeated infra_error (AGT-4305) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit AGT-4257's idle_fill unconditionally cleared a task's infra_error backoff whenever a free slot was available, with no cap on how many times the SAME issue could be bypassed for the SAME recurring infra_error. On vela, AX-1272's reviewer stage died to the same OpenRouter 360s timeout 9 of ~13 attempts over 4h+, retried every ~90s instead of waiting the intended 1h backoff, because it was the only runnable candidate — burning a full worker+tester+reviewer cycle each time with no forward progress. Adds consecutiveInfraErrorCounts (issueId -> streak), incremented on every infra_error result and reset at every existing terminal/recovery site. Both idle-fill bypass points (legacy failedTaskRetryTimes gate, and the durable- ledger RETRY_AT idleLiftable check) now refuse to bypass once an issue's CURRENT infra_error streak reaches 3, honoring the real 1h backoff instead. The durable-ledger check is additionally scoped to durableRun.lastErrorCode === 'infra_error' so a later, unrelated RETRY_AT (e.g. an ordinary rejection) on the same issue keeps normal idle-fill treatment. Two related gaps found during review are tracked separately rather than expanding this fix's scope: AGT-4306 (idle-fill also un-parks an INFRA_CIRCUIT_PARK_REASON NEEDS_HUMAN with no operator action) and AGT-4307 (a thrown/rejected executor error surfaces via a separate scheduler 'error' event that this streak never sees, so it bypasses this protection entirely). Co-Authored-By: Claude Sonnet 5 --- .../autonomousRunner.infraError.test.ts | 110 ++++++++++++++++++ src/automation/autonomousRunner.ts | 67 ++++++++++- 2 files changed, 173 insertions(+), 4 deletions(-) diff --git a/src/automation/autonomousRunner.infraError.test.ts b/src/automation/autonomousRunner.infraError.test.ts index a2732919..429b4e50 100644 --- a/src/automation/autonomousRunner.infraError.test.ts +++ b/src/automation/autonomousRunner.infraError.test.ts @@ -212,3 +212,113 @@ describe('AutonomousRunner infra_error handling (INT-2010)', () => { expect(history[0]).toMatchObject({ failureCause: 'timeout', finalStatus: 'infra_error' }); }); }); + +// AGT-4305: AGT-4257's idle_fill exists so free slots do not sit empty, but +// bypassing the 1h infra_error backoff instantly and forever, on the SAME +// issue, turns a transient provider hiccup into a worker+tester+reviewer +// spend loop when that issue is the only candidate around to fill slots with +// (observed on vela: AX-1272's reviewer call died to the same 360s OpenRouter +// timeout 9 of ~13 attempts over 4h+, retried within ~90s every time). +describe('idle-fill must not out-race a repeated infra_error on the same issue (AGT-4305)', () => { + type InternalRunner = { + scheduler: TaskScheduler; + filterAlreadyProcessed(tasks: TaskItem[]): TaskItem[]; + consecutiveInfraErrorCounts: Map; + failedTaskRetryTimes: Map; + }; + + const backoffTask = (): TaskItem => ({ + ...task(), + linearState: 'Todo', + linearProject: { id: 'project', name: 'Repo' }, + }); + + it('still idle-fills through the real 1h backoff below the threshold (control, unchanged AGT-4257 behavior)', async () => { + const source = mockTaskSource(); + runnerExecution.setTaskSource(source); + const runner = new AutonomousRunner(cfg()) as unknown as InternalRunner; + + await runN(runner.scheduler, 'infra_error', 2); // below MAX_CONSECUTIVE_INFRA_IDLE_FILL (3) + expect(runner.failedTaskRetryTimes.get('ISSUE-1')).toBeGreaterThan(Date.now()); + + const filtered = runner.filterAlreadyProcessed([backoffTask()]); + expect(filtered.map((t) => t.issueId)).toContain('ISSUE-1'); + }); + + it('stops bypassing the 1h backoff once the same issue has died to infra_error 3 times running', async () => { + const source = mockTaskSource(); + runnerExecution.setTaskSource(source); + const runner = new AutonomousRunner(cfg()) as unknown as InternalRunner; + + await runN(runner.scheduler, 'infra_error', 3); + expect(runner.consecutiveInfraErrorCounts.get('ISSUE-1')).toBe(3); + expect(runner.failedTaskRetryTimes.get('ISSUE-1')).toBeGreaterThan(Date.now()); + + const filtered = runner.filterAlreadyProcessed([backoffTask()]); + expect(filtered.map((t) => t.issueId)).not.toContain('ISSUE-1'); + }); + + it('resets the streak once the same issue reaches a real (non-infra) outcome', async () => { + const source = mockTaskSource(); + runnerExecution.setTaskSource(source); + const runner = new AutonomousRunner(cfg()) as unknown as InternalRunner; + + await runN(runner.scheduler, 'infra_error', 3); + expect(runner.consecutiveInfraErrorCounts.get('ISSUE-1')).toBe(3); + + const approved: PipelineResult = { ...result('approved'), success: true }; + runner.scheduler.startTask(task(), '/repo', async () => approved); + await new Promise((resolve) => setTimeout(resolve, 25)); + + expect(runner.consecutiveInfraErrorCounts.has('ISSUE-1')).toBe(false); + }); + + // The gate above only fires through `legacyIsAuthority` — false whenever a + // durable run already exists and the ledger is primary. Any issue that has + // ever failed once already has a durable row, so the durable-ledger + // idle-fill branch (`idleLiftable`, separate code above) needed its own copy + // of this gate. Without it, this whole describe block would be green while + // the actual production incident (AX-1272, durable/primary ledger) kept + // looping — caught in independent review before this test existed. + it('the durable-ledger RETRY_AT idle-fill branch also stops bypassing backoff after 3 consecutive infra_errors', async () => { + const LEDGER_TASK: TaskItem = { + id: 'ISSUE-1', issueId: 'ISSUE-1', issueIdentifier: 'ISSUE-1', + source: 'linear', title: 'reviewer keeps timing out', priority: 2, createdAt: 0, + linearState: 'Todo', linearProject: { id: 'project', name: 'Repo' }, + }; + const dbPath = join(tempDir, 'automation.db'); + + const source = mockTaskSource(); + runnerExecution.setTaskSource(source); + const runner = new AutonomousRunner(cfg({ automationLedgerMode: 'primary', automationDbPath: dbPath })) as unknown as InternalRunner & { + durableRuns: { + observeTask(task: TaskItem, repo: string): void; + getRun(id: string): { state: string } | null; + close(): void; + }; + }; + runner.durableRuns.observeTask(LEDGER_TASK, '/repo'); + + // Build the legacy-tracked streak the same way production does (the + // increment itself is unconditional — see the 'failed' handler). + await runN(runner.scheduler, 'infra_error', 3); + expect(runner.consecutiveInfraErrorCounts.get('ISSUE-1')).toBe(3); + + // Seed the durable row into RETRY_AT with a future retryAt, the way the + // real infra_error ledger transition leaves it (durableRunCoordinator.ts). + const { RunLedger } = await import('./runLedger.js'); + const ledger = new RunLedger(dbPath); + const claim = ledger.claimRun('ISSUE-1', { ownerInstanceId: 'seed', leaseMs: 60_000, maxActiveForProject: 1 }); + expect(claim).not.toBeNull(); + expect(ledger.transition(claim!, 'RETRY_AT', { + retryAt: Date.now() + 3_600_000, errorCode: 'infra_error', + })).toBe(true); + ledger.close(); + + const filtered = runner.filterAlreadyProcessed([LEDGER_TASK]); + expect(filtered.map((t) => t.issueId)).not.toContain('ISSUE-1'); + expect(runner.durableRuns.getRun('ISSUE-1')?.state).toBe('RETRY_AT'); // still parked, not lifted + + runner.durableRuns.close(); // this test opens its own primary-mode ledger handle + }); +}); diff --git a/src/automation/autonomousRunner.ts b/src/automation/autonomousRunner.ts index b27424da..e0bf52c0 100644 --- a/src/automation/autonomousRunner.ts +++ b/src/automation/autonomousRunner.ts @@ -422,6 +422,17 @@ export class AutonomousRunner { private completedTaskIds = new Set(); private failedTaskCounts = new Map(); private failedTaskRetryTimes = new Map(); // issueId → next retry timestamp (ms) + // issueId → consecutive infra_error count since its last cleared/terminal + // outcome (success, permanent block, operator park, or manual recovery — the + // same events that already clear failedTaskRetryTimes for this issueId). + // A non-infra failure that is retried again (rejection under the limit, + // 'superseded', etc.) leaves this untouched by design: it only needs to be + // conservative in one direction, never resetting is safe, silently resetting + // on the wrong event is not. Not persisted across restarts — a restart is + // itself a reasonable "give it a fresh run" signal, and losing the streak on + // restart only ever makes the gate below MORE permissive, never less safe. + // (AGT-4305) + private consecutiveInfraErrorCounts = new Map(); /** * Bring a durably backed-off run forward because its answer landed. @@ -438,6 +449,7 @@ export class AutonomousRunner { if (!this.answerArrivedFor(issueId)) return false; if (!this.durableRuns.readmitParkedRun(issueId, OPERATOR_PARK_REASON)) return false; clearRetryTime(issueId, this.failedTaskRetryTimes); + this.consecutiveInfraErrorCounts.delete(issueId); return true; } @@ -486,6 +498,10 @@ export class AutonomousRunner { // the reviewer already called out (INT-2474). Persisted; cleared on success. private lastFailureDetails = new Map(); private static readonly MAX_RETRY_COUNT = 4; // Increased from 2 to allow more retries with backoff + // Consecutive infra_error attempts (same issue, no intervening non-infra + // outcome) allowed to bypass backoff via idle-fill before the real 1h + // backoff is enforced instead. (AGT-4305) + private static readonly MAX_CONSECUTIVE_INFRA_IDLE_FILL = 3; // Rate-limit hold: epoch ms until which all task execution is paused. // Set when any adapter returns a 429 / usage_limit_reached response (INT-1906). @@ -607,6 +623,7 @@ export class AutonomousRunner { this.completedTaskIds.add(task.issueId); clearRejection(task.issueId); // Clear rejection count on success clearRetryTime(task.issueId, this.failedTaskRetryTimes); // Clear retry backoff time + this.consecutiveInfraErrorCounts.delete(task.issueId); this.lastFailureDetails.delete(task.issueId); // Stale feedback must not haunt future work this.saveTaskState(); // Track project-level pace (5h rolling window) @@ -623,6 +640,7 @@ export class AutonomousRunner { } if (result.success && task.issueId && this.durableRuns.isPrimary) { + this.consecutiveInfraErrorCounts.delete(task.issueId); await this.drainDurableOutbox().catch((error) => console.error('[Outbox] Completion delivery pass failed:', error)); const durableState = this.durableRuns.getRun(task.issueId)?.state; @@ -854,8 +872,10 @@ export class AutonomousRunner { // Fixed mid-range backoff — we intentionally don't bump failure counts, // so there's no attempt number to scale by. const nextRetryTime = setRetryTime(task.issueId, 3, this.failedTaskRetryTimes); + const infraStreak = (this.consecutiveInfraErrorCounts.get(task.issueId) ?? 0) + 1; + this.consecutiveInfraErrorCounts.set(task.issueId, infraStreak); this.saveTaskState(); - console.warn(`[Scheduler] Infra error for ${taskCtx} (NOT counted toward STUCK) — backoff retry ${formatRetryTime(nextRetryTime)}: ${detail}`); + console.warn(`[Scheduler] Infra error for ${taskCtx} (NOT counted toward STUCK, consecutive: ${infraStreak}) — backoff retry ${formatRetryTime(nextRetryTime)}: ${detail}`); } else { console.warn(`[Scheduler] Infra error for ${taskCtx} (NOT counted toward STUCK): ${detail}`); } @@ -892,6 +912,7 @@ export class AutonomousRunner { const { code, reason } = result.operatorPark; this.completedTaskIds.add(task.issueId); // no retry changes what the fence saw clearRetryTime(task.issueId, this.failedTaskRetryTimes); + this.consecutiveInfraErrorCounts.delete(task.issueId); recordLastFailureDetail(this.taskStateRef, task.issueId, reason); this.saveTaskState(); console.warn(`[Scheduler] ${taskCtx} parked for the operator (${code}): ${reason}`); @@ -934,6 +955,7 @@ export class AutonomousRunner { const attempts = getRejectionCount(task.issueId) + (this.failedTaskCounts.get(task.issueId) ?? 0) + 1; this.completedTaskIds.add(task.issueId); // no retry can move an environmental wall clearRetryTime(task.issueId, this.failedTaskRetryTimes); + this.consecutiveInfraErrorCounts.delete(task.issueId); clearRejection(task.issueId); recordLastFailureDetail(this.taskStateRef, task.issueId, infeasDetail); const ownsRun = parkRunForHuman( @@ -990,6 +1012,7 @@ export class AutonomousRunner { // Max rejections reached - permanently block this.completedTaskIds.add(task.issueId); // Prevent re-selection clearRetryTime(task.issueId, this.failedTaskRetryTimes); // Clear retry time + this.consecutiveInfraErrorCounts.delete(task.issueId); const ownsRun = parkRunForHuman( this.durableRuns, task.issueId, `Reviewer rejected ${rejectionCount} attempts: ${feedback}`, @@ -1057,6 +1080,7 @@ export class AutonomousRunner { // Max retries exceeded - permanently block this.completedTaskIds.add(task.issueId); // Prevent re-selection clearRetryTime(task.issueId, this.failedTaskRetryTimes); // Clear retry time + this.consecutiveInfraErrorCounts.delete(task.issueId); const ownsRun = parkRunForHuman( this.durableRuns, task.issueId, `Autonomous execution failed ${count} times: ${failureDetail}`, @@ -1227,7 +1251,29 @@ export class AutonomousRunner { // without spending budget. WAITING_EXTERNAL is a run whose published // effect is still pending, not a park: lifting it re-runs the task on // top of its own in-flight publish. - const idleLiftable = (durableRun.state === 'RETRY_AT' && (durableRun.retryAt ?? 0) > Date.now()) + // + // A RETRY_AT row can be parked there for infra_error same as any other + // reason, and this is the durable-ledger counterpart of the legacy + // idle-fill bypass gated below by `consecutiveInfraErrorCounts` — without + // it here too, an issue whose durable row already exists (true for + // anything that has ever failed once) never reaches that legacy gate at + // all, since `legacyIsAuthority` is false whenever this block ran and + // left a durable run in place. (AGT-4305 — this is the branch AX-1272 + // was actually looping through in production.) + // Only throttle a RETRY_AT that is CURRENTLY backed off for infra_error — + // `consecutiveInfraErrorCounts` does not reset on a later, unrelated + // rejection/failure retry for the same issue (by design; see the field + // comment), so without the lastErrorCode check a stale infra streak + // would keep throttling idle-fill for a RETRY_AT caused by an ordinary + // task-level rejection long after the infra episode ended. + const infraStreak = durableRun.lastErrorCode === 'infra_error' + ? this.consecutiveInfraErrorCounts.get(id) ?? 0 + : 0; + const idleLiftable = ( + durableRun.state === 'RETRY_AT' + && (durableRun.retryAt ?? 0) > Date.now() + && infraStreak < AutonomousRunner.MAX_CONSECUTIVE_INFRA_IDLE_FILL + ) || durableRun.state === 'NEEDS_SPEC' || durableRun.state === 'NEEDS_ENV'; if (idleLiftable && idleFillBudget > 0 && this.durableRuns.markReady(id)) { @@ -1298,6 +1344,7 @@ export class AutonomousRunner { this.failedTaskCounts.delete(id); clearRejection(id); // Clear rejection count on recovery clearRetryTime(id, this.failedTaskRetryTimes); // Clear retry backoff time + this.consecutiveInfraErrorCounts.delete(id); if (isStuck) toUnstick.push(id); // strip the stuck label so it is not re-skipped recovered++; return true; @@ -1317,6 +1364,7 @@ export class AutonomousRunner { this.failedTaskCounts.delete(id); clearRejection(id); clearRetryTime(id, this.failedTaskRetryTimes); + this.consecutiveInfraErrorCounts.delete(id); if (isStuck) toUnstick.push(id); recovered++; return true; @@ -1369,11 +1417,21 @@ export class AutonomousRunner { // an `ask_human` park, so left alone it makes the operator's reply land // up to two hours after they sent it. if (!canRetryNow(id, this.failedTaskRetryTimes)) { + const infraStreak = this.consecutiveInfraErrorCounts.get(id) ?? 0; if (this.answerArrivedFor(id)) { clearRetryTime(id, this.failedTaskRetryTimes); + this.consecutiveInfraErrorCounts.delete(id); answered++; - } else if (idleFillBudget > 0) { - // AGT-4257: free slots chew the backoff instead of sitting idle. + } else if (idleFillBudget > 0 && infraStreak < AutonomousRunner.MAX_CONSECUTIVE_INFRA_IDLE_FILL) { + // AGT-4257: free slots chew the backoff instead of sitting idle — but + // not when the same issue has died to infra_error (timeout/CLI + // failure, not a task failure) several times running with an idle + // fill each time. Retrying instantly with an unchanged payload just + // re-hits the same wall (AX-1272, 2026-09-10: same reviewer 360s + // timeout, 9 of ~13 attempts over 4h+, ~90s apart every time because + // this was the only candidate to fill idle slots with). Past the + // threshold, honor the real 1h backoff so a transient provider issue + // gets time to actually clear instead of being hammered. (AGT-4305) idleFillBudget--; clearRetryTime(id, this.failedTaskRetryTimes); recovered++; @@ -1772,6 +1830,7 @@ export class AutonomousRunner { this.completedTaskIds.add(issueId); clearRejection(issueId); clearRetryTime(issueId, this.failedTaskRetryTimes); + this.consecutiveInfraErrorCounts.delete(issueId); this.lastFailureDetails.delete(issueId); } if (finalized.size > 0) this.saveTaskState();