From 7da74083be1d61e679c5cdf780af07ebc61ec888 Mon Sep 17 00:00:00 2001 From: jsboige Date: Fri, 21 Aug 2026 12:18:47 +0200 Subject: [PATCH] fix(task): bound the auto-approval retry loop in attemptApiRequest MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The autoApprovalEnabled path of attemptApiRequest recursed with no cap — only abort stopped it. On a persistent API error (e.g. HTTP 429 fair usage, a whole-account rate limit), each retry is charged against the account and worsens the condition; observed 17 retries (~2h50) and 48 (~8h) in production. Add MAX_AUTO_APPROVAL_RETRIES = 3 (same convention as MAX_CONTEXT_WINDOW_RETRIES) checked before backoffAndAnnounce so the refused request never sleeps on a backoff that cannot succeed, and stop loudly with an Error naming the cap and the last underlying error. The context-window and interactive retry paths are untouched. Add a mutation-checked spec: always-failing stream + autoApprovalEnabled must throw after MAX+1 total attempts; the in-mock guard fails fast if the cap is removed. Co-Authored-By: Claude-Code --- src/core/task/Task.ts | 13 +++++ src/core/task/__tests__/Task.spec.ts | 75 ++++++++++++++++++++++++++++ 2 files changed, 88 insertions(+) diff --git a/src/core/task/Task.ts b/src/core/task/Task.ts index 4be087394e..8c42440744 100644 --- a/src/core/task/Task.ts +++ b/src/core/task/Task.ts @@ -141,6 +141,7 @@ const MAX_EXPONENTIAL_BACKOFF_SECONDS = 600 // 10 minutes const DEFAULT_USAGE_COLLECTION_TIMEOUT_MS = 5000 // 5 seconds const FORCED_CONTEXT_REDUCTION_PERCENT = 75 // Keep 75% of context (remove 25%) on context window errors const MAX_CONTEXT_WINDOW_RETRIES = 3 // Maximum retries for context window errors +const MAX_AUTO_APPROVAL_RETRIES = 3 // Bounds the auto-approval retry loop (persistent API errors, e.g. HTTP 429) export interface TaskOptions extends CreateTaskOptions { provider: ClineProvider @@ -4425,6 +4426,18 @@ export class Task extends EventEmitter implements TaskLike { // note that this api_req_failed ask is unique in that we only present this option if the api hasn't streamed any content yet (ie it fails on the first chunk due), as it would allow them to hit a retry button. However if the api failed mid-stream, it could be in any arbitrary state where some tools may have executed, so that error is handled differently and requires cancelling the task entirely. if (autoApprovalEnabled) { + // Bound the retry loop before backoff: a persistent API error (e.g. HTTP 429 fair usage, + // a rate limit on the whole account) is not going to resolve by retrying harder — each + // attempt is charged against the account and postpones recovery. Stop loudly instead of + // recursing until abort. + if (retryAttempt >= MAX_AUTO_APPROVAL_RETRIES) { + throw new Error( + `[Task#attemptApiRequest] task ${this.taskId}.${this.instanceId} aborted after ` + + `${MAX_AUTO_APPROVAL_RETRIES} auto-approval retries — persistent API error ` + + `(last: ${error.message ?? JSON.stringify(serializeError(error))}). Retry loop capped (roo-extensions#3195).`, + ) + } + // Apply shared exponential backoff and countdown UX await this.backoffAndAnnounce(retryAttempt, error) diff --git a/src/core/task/__tests__/Task.spec.ts b/src/core/task/__tests__/Task.spec.ts index 37e228f887..7298df67b8 100644 --- a/src/core/task/__tests__/Task.spec.ts +++ b/src/core/task/__tests__/Task.spec.ts @@ -30,6 +30,7 @@ import type { ApiMessage } from "../../task-persistence" type TaskTestAccess = { getSystemPrompt: () => Promise + backoffAndAnnounce: (retryAttempt: number, error: unknown) => Promise getEnabledMcpToolsCount: () => Promise<{ enabledToolCount: number; enabledServerCount: number }> initiateTaskLoop: (userContent: Anthropic.Messages.ContentBlockParam[]) => Promise startTask: (task?: string, images?: string[]) => Promise @@ -943,6 +944,80 @@ describe("Cline", () => { expect(mockDelay).toHaveBeenCalledWith(1000) }) + it("should cap the auto-approval retry loop on a persistent API error", async () => { + const cline = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + task: "test task", + startTask: false, + }) + vi.spyOn(getTaskTestAccess(cline), "getSystemPrompt").mockResolvedValue("mock system prompt") + + // Mock delay to keep the backoff instant + const mockDelay = vi.fn().mockResolvedValue(undefined) + vi.spyOn(await import("delay"), "default").mockImplementation(mockDelay) + + const saySpy = vi.spyOn(cline, "say") + + // A stream that errors on every access — the API never succeeds. + const mockError = new Error("API Error") + const mockFailedStream = { + // eslint-disable-next-line require-yield + async *[Symbol.asyncIterator]() { + throw mockError + }, + async next() { + throw mockError + }, + async return() { + return { done: true, value: undefined } + }, + async throw(error: unknown) { + throw error + }, + async [Symbol.asyncDispose]() { + // Cleanup + }, + } as AsyncGenerator + + const providerState = await mockProvider.getState() + vi.spyOn(mockProvider, "getState").mockResolvedValue({ + ...providerState, + apiConfiguration: mockApiConfig, + autoApprovalEnabled: true, + requestDelaySeconds: 3, + }) + + let attemptCount = 0 + const createMessageSpy = vi.spyOn(cline.api, "createMessage").mockImplementation(() => { + attemptCount++ + // Fail fast if the retry loop is unbounded — guards against a hang if the cap is removed. + expect(attemptCount).toBeLessThanOrEqual(4) + return mockFailedStream + }) + + // One backoff per retry, and the cap must refuse to back off again once hit. + const backoffSpy = vi.spyOn(getTaskTestAccess(cline), "backoffAndAnnounce").mockResolvedValue(undefined) + + // 1 initial attempt + MAX_AUTO_APPROVAL_RETRIES(3) retries, then the loop must throw. + const iterator = cline.attemptApiRequest(0) + let thrown: unknown + try { + await iterator.next() + } catch (e) { + thrown = e + } + + // The stop is loud and names the last underlying error and the cap. + expect(thrown).toBeInstanceOf(Error) + expect((thrown as Error).message).toMatch(/capped.*roo-extensions#3195/) + expect((thrown as Error).message).toContain("API Error") + expect(attemptCount).toBe(4) + expect(createMessageSpy).toHaveBeenCalledTimes(4) + // Exactly as many backoffs as retries — the request that finally threw never slept. + expect(backoffSpy).toHaveBeenCalledTimes(3) + }) + it("uses the task rate limit in retry backoff when focused provider state differs", async () => { const clock = createRateLimitClock() const rateLimitConfig = {