From 3801b4ee3217094a8d8bc8d803cb45f43dca8816 Mon Sep 17 00:00:00 2001 From: can1357 Date: Mon, 25 May 2026 12:37:01 +0200 Subject: [PATCH] fix(coding-agent): added configurable retry delay cap and surfaced rate-limit failure state - Added `retry.maxDelayMs` to the settings schema and interfaces, with a default cap for provider backoff delays. - Updated session auto-retry logic to fail fast when a requested wait exceeds the cap without fallback, emitting terminal auto-retry failure state. - Propagated retry state and failure data into task progress and rendering so children show retry/wait details and reminder prompts stop after terminal errors. --- .../src/config/settings-schema.ts | 11 + .../coding-agent/src/session/agent-session.ts | 21 ++ packages/coding-agent/src/task/executor.ts | 31 +++ packages/coding-agent/src/task/index.ts | 2 + packages/coding-agent/src/task/render.ts | 31 ++- packages/coding-agent/src/task/types.ts | 34 ++++ .../test/agent-session-retry-cap.test.ts | 190 ++++++++++++++++++ .../coding-agent/test/issue-953-repro.test.ts | 1 + .../test/status-line-overflow.test.ts | 1 + .../test/status-line-path.test.ts | 1 + 10 files changed, 321 insertions(+), 2 deletions(-) create mode 100644 packages/coding-agent/test/agent-session-retry-cap.test.ts diff --git a/packages/coding-agent/src/config/settings-schema.ts b/packages/coding-agent/src/config/settings-schema.ts index 4a890e6d1..e9188bb0a 100644 --- a/packages/coding-agent/src/config/settings-schema.ts +++ b/packages/coding-agent/src/config/settings-schema.ts @@ -836,6 +836,16 @@ export const SETTINGS_SCHEMA = { }, "retry.baseDelayMs": { type: "number", default: 2000 }, + "retry.maxDelayMs": { + type: "number", + default: 5 * 60 * 1000, + ui: { + tab: "model", + label: "Max Retry Delay", + description: + "Maximum wait between retries, in ms. When the provider asks us to wait longer than this and no credential or model fallback succeeds, the request fails fast instead of sleeping (e.g. 3-hour Anthropic rate-limit windows).", + }, + }, "retry.fallbackChains": { type: "record", default: {} as Record }, "retry.fallbackRevertPolicy": { type: "enum", @@ -2859,6 +2869,7 @@ export interface RetrySettings { enabled: boolean; maxRetries: number; baseDelayMs: number; + maxDelayMs: number; } export interface MemoriesSettings { diff --git a/packages/coding-agent/src/session/agent-session.ts b/packages/coding-agent/src/session/agent-session.ts index e8e72d35a..b8c033b2d 100644 --- a/packages/coding-agent/src/session/agent-session.ts +++ b/packages/coding-agent/src/session/agent-session.ts @@ -7108,6 +7108,27 @@ export class AgentSession { } } + // Fail-fast cap: if the provider asks us to wait longer than + // retry.maxDelayMs and we have no fallback credential or model to + // switch to, surface the error instead of sleeping. Defends against + // 3-hour Anthropic rate-limit windows that would otherwise leave a + // subagent (or interactive session) silently hung. The original + // assistant error message is preserved in agent state so the caller + // can act on it. + const maxDelayMs = retrySettings.maxDelayMs; + if (maxDelayMs > 0 && delayMs > maxDelayMs && !switchedCredential && !switchedModel) { + const attempt = this.#retryAttempt; + this.#retryAttempt = 0; + await this.#emitSessionEvent({ + type: "auto_retry_end", + success: false, + attempt, + finalError: `Provider requested ${delayMs}ms wait, exceeds retry.maxDelayMs (${maxDelayMs}ms). Original error: ${errorMessage}`, + }); + this.#resolveRetry(); + return false; + } + await this.#emitSessionEvent({ type: "auto_retry_start", attempt: this.#retryAttempt, diff --git a/packages/coding-agent/src/task/executor.ts b/packages/coding-agent/src/task/executor.ts index b4c369cbc..a3c2ab6d5 100644 --- a/packages/coding-agent/src/task/executor.ts +++ b/packages/coding-agent/src/task/executor.ts @@ -1350,6 +1350,30 @@ export async function runSubprocess(options: ExecutorOptions): Promise { + if (event.type === "auto_retry_start") { + progress.retryState = { + attempt: event.attempt, + maxAttempts: event.maxAttempts, + delayMs: event.delayMs, + errorMessage: event.errorMessage, + startedAtMs: Date.now(), + }; + progress.retryFailure = undefined; + scheduleProgress(true); + return; + } + if (event.type === "auto_retry_end") { + const attempt = progress.retryState?.attempt ?? event.attempt; + progress.retryState = undefined; + if (!event.success) { + progress.retryFailure = { + attempt, + errorMessage: event.finalError ?? "Auto-retry failed", + }; + } + scheduleProgress(true); + return; + } if (isAgentEvent(event)) { try { processEvent(event); @@ -1385,6 +1409,12 @@ export async function runSubprocess(options: ExecutorOptions): Promise 0 ? `in ${formatDuration(remainingMs)}` : "now"; + const summary = + `retrying ${progress.retryState.attempt}/${progress.retryState.maxAttempts} ${waitLabel}: ` + + truncateToWidth(replaceTabs(progress.retryState.errorMessage), 60); + lines.push(`${continuePrefix}${theme.tree.hook} ${theme.fg("warning", summary)}`); + } else if (progress.retryFailure && progress.status !== "running") { + const summary = `auto-retry gave up after ${progress.retryFailure.attempt} attempt${ + progress.retryFailure.attempt === 1 ? "" : "s" + }: ${truncateToWidth(replaceTabs(progress.retryFailure.errorMessage), 80)}`; + lines.push(`${continuePrefix}${theme.tree.hook} ${theme.fg("error", summary)}`); + } + // Render extracted tool data inline (e.g., review findings) if (progress.extractedToolData) { // For completed tasks, check for review verdict from yield tool diff --git a/packages/coding-agent/src/task/types.ts b/packages/coding-agent/src/task/types.ts index 88a9a9827..1898ece3b 100644 --- a/packages/coding-agent/src/task/types.ts +++ b/packages/coding-agent/src/task/types.ts @@ -212,6 +212,30 @@ export interface AgentProgress { modelOverride?: string | string[]; /** Data extracted by registered subprocess tool handlers (keyed by tool name) */ extractedToolData?: Record; + /** + * Auto-retry state when the subagent is sleeping between provider retries + * (e.g. 429 rate-limit with retry-after). Cleared when the retry resolves + * or fails. Surfacing this to the parent prevents the task tool from + * looking indefinitely "in progress" when a child is actually blocked on + * provider quota. + */ + retryState?: { + attempt: number; + maxAttempts: number; + delayMs: number; + errorMessage: string; + startedAtMs: number; + }; + /** + * Terminal retry failure surfaced once the subagent gave up retrying + * (e.g. retry-after exceeded the cap, or all attempts exhausted). Carries + * the final error so the parent UI can render "blocked: rate-limited" + * instead of waiting for a status that never arrives. + */ + retryFailure?: { + attempt: number; + errorMessage: string; + }; } /** Result from a single agent execution */ @@ -251,6 +275,16 @@ export interface SingleResult { nestedPatches?: NestedRepoPatch[]; /** Data extracted by registered subprocess tool handlers (keyed by tool name) */ extractedToolData?: Record; + /** + * Terminal retry failure, when the subagent exited because the auto-retry + * loop gave up (retry-after exceeded the cap, or all attempts exhausted). + * Lets the parent task tool surface a "blocked: rate-limited" outcome + * instead of a generic failure. + */ + retryFailure?: { + attempt: number; + errorMessage: string; + }; /** Output metadata for agent:// URL integration */ outputMeta?: { lineCount: number; charCount: number }; } diff --git a/packages/coding-agent/test/agent-session-retry-cap.test.ts b/packages/coding-agent/test/agent-session-retry-cap.test.ts new file mode 100644 index 000000000..e8fd1f3a8 --- /dev/null +++ b/packages/coding-agent/test/agent-session-retry-cap.test.ts @@ -0,0 +1,190 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from "bun:test"; +import * as path from "node:path"; +import { scheduler } from "node:timers/promises"; +import { Agent } from "@oh-my-pi/pi-agent-core"; +import { type AssistantMessage, getBundledModel } from "@oh-my-pi/pi-ai"; +import { createMockModel } from "@oh-my-pi/pi-ai/providers/mock"; +import { ModelRegistry } from "@oh-my-pi/pi-coding-agent/config/model-registry"; +import { Settings } from "@oh-my-pi/pi-coding-agent/config/settings"; +import { AgentSession, type AgentSessionEvent } from "@oh-my-pi/pi-coding-agent/session/agent-session"; +import { AuthStorage } from "@oh-my-pi/pi-coding-agent/session/auth-storage"; +import { SessionManager } from "@oh-my-pi/pi-coding-agent/session/session-manager"; +import { TempDir } from "@oh-my-pi/pi-utils"; + +type AutoRetryEndEvent = Extract; +type AutoRetryStartEvent = Extract; + +function lastAssistant(session: AgentSession): AssistantMessage { + const message = session.agent.state.messages.at(-1); + if (!message || message.role !== "assistant") { + throw new Error("Expected trailing assistant message"); + } + return message as AssistantMessage; +} + +/** + * Contract: when the provider asks us to wait longer than `retry.maxDelayMs` + * and we have no credential/model fallback to switch to, the auto-retry + * loop MUST fail fast — preserving the terminal error message in agent + * state and skipping the long sleep entirely. + * + * Without this defense, an Anthropic `429 rate_limit_error` with + * `retry-after-ms=11180000` (≈3 hours) pinned a subagent in the retry + * sleep, leaving the parent task tool stuck on the review phase for hours + * (see GitHub issue #607). + */ +describe("AgentSession retry delay cap", () => { + let tempDir: TempDir; + let authStorage: AuthStorage; + let modelRegistry: ModelRegistry; + let session: AgentSession | undefined; + + beforeEach(async () => { + tempDir = TempDir.createSync("@pi-retry-cap-"); + authStorage = await AuthStorage.create(path.join(tempDir.path(), "testauth.db")); + authStorage.setRuntimeApiKey("anthropic", "anthropic-test-key"); + modelRegistry = new ModelRegistry(authStorage); + }); + + afterEach(async () => { + if (session) { + await session.dispose(); + session = undefined; + } + authStorage.close(); + tempDir.removeSync(); + vi.restoreAllMocks(); + }); + + it("bails immediately when retry-after exceeds retry.maxDelayMs", async () => { + const model = getBundledModel("anthropic", "claude-sonnet-4-5"); + if (!model) { + throw new Error("Expected bundled Anthropic test model to exist"); + } + + // 11.18M ms == ~3.1 hours, matching the report on the original incident. + const rateLimitError = + '429 {"type":"error","error":{"type":"rate_limit_error","message":"This request would exceed your account\'s rate limit. Please try again later."}} retry-after-ms=11180000'; + + const mock = createMockModel({ handler: () => ({ throw: rateLimitError }) }); + const requestedModels: string[] = []; + const agent = new Agent({ + getApiKey: provider => `${provider}-test-key`, + initialState: { + model, + systemPrompt: ["Test"], + tools: [], + messages: [], + }, + streamFn: (requestedModel, context, options) => { + requestedModels.push(`${requestedModel.provider}/${requestedModel.id}`); + return mock.stream(requestedModel, context, options); + }, + }); + + const settings = Settings.isolated({ + "compaction.enabled": false, + "retry.baseDelayMs": 5, + "retry.maxDelayMs": 100, + }); + settings.setModelRole("default", `${model.provider}/${model.id}`); + + session = new AgentSession({ + agent, + sessionManager: SessionManager.inMemory(), + settings, + modelRegistry, + }); + + // Spy after construction so the constructor's no-op work isn't intercepted. + const waitSpy = vi.spyOn(scheduler, "wait").mockResolvedValue(undefined); + const retryStartEvents: AutoRetryStartEvent[] = []; + const retryEndEvents: AutoRetryEndEvent[] = []; + session.subscribe(event => { + if (event.type === "auto_retry_start") retryStartEvents.push(event); + if (event.type === "auto_retry_end") retryEndEvents.push(event); + }); + + await session.prompt("Trigger rate limit with long retry-after"); + await session.waitForIdle(); + + // Only one model call: the auto-retry MUST NOT loop into a fresh attempt + // because the cap fired before scheduler.wait was even reached. + expect(requestedModels).toEqual([`${model.provider}/${model.id}`]); + expect(retryStartEvents).toHaveLength(0); + expect(retryEndEvents).toHaveLength(1); + expect(retryEndEvents[0]).toMatchObject({ success: false }); + expect(retryEndEvents[0].finalError).toContain("exceeds retry.maxDelayMs"); + expect(retryEndEvents[0].finalError).toContain("11180000"); + // No multi-hour (or any) sleep — the cap path skips scheduler.wait entirely. + for (const call of waitSpy.mock.calls) { + expect(call[0]).toBeLessThanOrEqual(100); + } + + // The terminal error stays as the last assistant message so the caller + // (interactive UI, parent task tool, SDK consumer) can act on it. + const last = lastAssistant(session); + expect(last.stopReason).toBe("error"); + expect(last.errorMessage).toContain("rate_limit_error"); + expect(session.isRetrying).toBe(false); + }); + + it("still retries normally when the delay is under retry.maxDelayMs", async () => { + // Sanity check: a small retry-after MUST still go through the retry + // loop so we don't regress the existing transient-error recovery. + const model = getBundledModel("anthropic", "claude-sonnet-4-5"); + if (!model) { + throw new Error("Expected bundled Anthropic test model to exist"); + } + + const mock = createMockModel({ + responses: [ + { throw: "503 service unavailable: overloaded_error retry-after-ms=50" }, + { content: ["recovered after short backoff"] }, + ], + }); + const agent = new Agent({ + getApiKey: provider => `${provider}-test-key`, + initialState: { + model, + systemPrompt: ["Test"], + tools: [], + messages: [], + }, + streamFn: mock.stream, + }); + + const settings = Settings.isolated({ + "compaction.enabled": false, + "retry.baseDelayMs": 5, + "retry.maxDelayMs": 5_000, + }); + settings.setModelRole("default", `${model.provider}/${model.id}`); + + session = new AgentSession({ + agent, + sessionManager: SessionManager.inMemory(), + settings, + modelRegistry, + }); + + const waitSpy = vi.spyOn(scheduler, "wait").mockResolvedValue(undefined); + const retryStartEvents: AutoRetryStartEvent[] = []; + const retryEndEvents: AutoRetryEndEvent[] = []; + session.subscribe(event => { + if (event.type === "auto_retry_start") retryStartEvents.push(event); + if (event.type === "auto_retry_end") retryEndEvents.push(event); + }); + + await session.prompt("Trigger transient with short retry-after"); + await session.waitForIdle(); + + expect(retryStartEvents).toHaveLength(1); + expect(retryStartEvents[0].delayMs).toBeLessThanOrEqual(5_000); + expect(retryEndEvents).toHaveLength(1); + expect(retryEndEvents[0]).toMatchObject({ success: true }); + expect(waitSpy).toHaveBeenCalled(); + const last = lastAssistant(session); + expect(last.stopReason).toBe("stop"); + }); +}); diff --git a/packages/coding-agent/test/issue-953-repro.test.ts b/packages/coding-agent/test/issue-953-repro.test.ts index db188f02b..7a6fb3585 100644 --- a/packages/coding-agent/test/issue-953-repro.test.ts +++ b/packages/coding-agent/test/issue-953-repro.test.ts @@ -40,6 +40,7 @@ function createCtx(usage: Partial): SegmentContext status: null, pr: null, }, + usage: null, }; } diff --git a/packages/coding-agent/test/status-line-overflow.test.ts b/packages/coding-agent/test/status-line-overflow.test.ts index 7ee0bd9fc..50c8a6112 100644 --- a/packages/coding-agent/test/status-line-overflow.test.ts +++ b/packages/coding-agent/test/status-line-overflow.test.ts @@ -64,6 +64,7 @@ function createCtx(overrides?: { pathMaxLength?: number; branch?: string | null status: null, pr: null, }, + usage: null, }; } diff --git a/packages/coding-agent/test/status-line-path.test.ts b/packages/coding-agent/test/status-line-path.test.ts index 6fd3fed1f..00e8a57ec 100644 --- a/packages/coding-agent/test/status-line-path.test.ts +++ b/packages/coding-agent/test/status-line-path.test.ts @@ -51,6 +51,7 @@ function createPathContext(): SegmentContext { status: null, pr: null, }, + usage: null, }; }