diff --git a/CHANGELOG.md b/CHANGELOG.md index 380b9ada..44f306fa 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -29,6 +29,16 @@ parallel copies under `docs/` or `scripts/notes/`. At cut time: rename the subtree-scoping rule for those is written and tested but not yet wired to a live call site. `task()` is unchanged and still the only spawn verb. +- **Retry recovery no longer multiplies with the harness's own retries.** The + director's inference-recovery layer previously re-issued a full-context + `infer()` call for `timeout`/`retryable` errors even though the harness's + own retry policy already retries and exhausts those categories before + surfacing them — compounding to up to 9 identical full-context sends per + logical turn in the worst case. The director now only recovers + internal-recovery aborts (a category the harness never retries on its + own), so the two layers no longer multiply. Attempt counts are now logged + on each recovery so retry storms are visible in traces. + ### Fixed - **Interrupting a turn no longer risks a startup crash.** If an interrupt hit diff --git a/src/agent/director.test.ts b/src/agent/director.test.ts index 76460d40..11af0ea4 100644 --- a/src/agent/director.test.ts +++ b/src/agent/director.test.ts @@ -1083,3 +1083,155 @@ describe("ChatDirector tool-only loop protection", () => { }); }); }); + +// CL-6910: the harness's own retry policy (vendor/intx-inference/src/ +// retry-policy.ts) already owns `timeout`/`retryable`/`quota_exhausted` and +// exhausts its full attempt budget (3 attempts) before an `inference.error` +// of one of those categories ever reaches the director. The director must +// not re-wrap those categories in another `capabilities.infer()` call — that +// multiplied the two layers' attempt budgets (up to 9 identical full-context +// sends per turn) instead of composing them. `aborted` (internal-recovery) +// is the one category the harness never retries at all, so it remains the +// director's to recover, and that recovery does not compound with harness +// attempts. +function inferenceErrorEvent( + category: "retryable" | "timeout" | "aborted" | "quota_exhausted", + raw?: unknown, +): ReactorInboundEvent { + return { + type: "inference.error", + error: { category, message: "boom", raw }, + } as unknown as ReactorInboundEvent; +} + +describe("ChatDirector inference-error recovery (CL-6910)", () => { + const providerlessPolicy = { providerName: "test-provider" }; + + test.each(["retryable", "timeout", "quota_exhausted"] as const)( + "does not re-issue inference for a %s error already exhausted by the harness", + async (category) => { + const director = createChatDirector("system", [], { + onTasksChange: () => {}, + provider: providerlessPolicy, + }); + const capabilities = makeCapabilities(); + + const actions = actionsArray( + await director.decide(inferenceErrorEvent(category), mockState, capabilities), + ); + + // No additional full-context send: the base director's terminal + // checkpoint + reply is the only outcome, not another `infer`. + expect(actions.some((a) => a.type === "infer")).toBe(false); + expect(actions.some((a) => a.type === "reply")).toBe(true); + }, + ); + + test("still recovers on internal-recovery abort, bounded by MAX_INFERENCE_RECOVERIES", async () => { + const director = createChatDirector("system", [], { + onTasksChange: () => {}, + provider: providerlessPolicy, + }); + const capabilities = makeCapabilities(); + const internalAbort = inferenceErrorEvent("aborted", { origin: "internal-recovery" }); + + // Recovery 1 of 2: re-issues inference. + const first = actionsArray(await director.decide(internalAbort, mockState, capabilities)); + expect(first.some((a) => a.type === "infer")).toBe(true); + + // Recovery 2 of 2: re-issues inference. + const second = actionsArray(await director.decide(internalAbort, mockState, capabilities)); + expect(second.some((a) => a.type === "infer")).toBe(true); + + // Budget exhausted: no further infer, terminal reply instead. + const third = actionsArray(await director.decide(internalAbort, mockState, capabilities)); + expect(third.some((a) => a.type === "infer")).toBe(false); + expect(third.some((a) => a.type === "reply")).toBe(true); + }); + + test("an unrelated aborted error (not internal-recovery) is not recovered by the director", async () => { + const director = createChatDirector("system", [], { + onTasksChange: () => {}, + provider: providerlessPolicy, + }); + const capabilities = makeCapabilities(); + + const actions = actionsArray( + await director.decide( + inferenceErrorEvent("aborted", { origin: "user-stop" }), + mockState, + capabilities, + ), + ); + expect(actions.some((a) => a.type === "infer")).toBe(false); + }); + + test("inference-recovery budget resets at the next turn boundary", async () => { + const director = createChatDirector("system", [], { + onTasksChange: () => {}, + provider: providerlessPolicy, + }); + const capabilities = makeCapabilities(); + const internalAbort = inferenceErrorEvent("aborted", { origin: "internal-recovery" }); + + await director.decide(internalAbort, mockState, capabilities); + await director.decide(internalAbort, mockState, capabilities); + // Budget exhausted for this turn. + const exhausted = actionsArray(await director.decide(internalAbort, mockState, capabilities)); + expect(exhausted.some((a) => a.type === "infer")).toBe(false); + + // A fresh turn boundary (inference.done) resets the budget. + await director.decide(toolOnlyTurn("post-boundary"), mockState, capabilities); + const afterBoundary = actionsArray( + await director.decide(internalAbort, mockState, capabilities), + ); + expect(afterBoundary.some((a) => a.type === "infer")).toBe(true); + }); + + // Bounds the worst-case number of on-wire full-context sends per logical + // turn across the two layers that can legitimately fire: the harness's + // own retry policy (up to 3 attempts per `infer()` call — see + // vendor/intx-inference/src/retry-policy.ts MAX_ATTEMPTS) and the + // director's internal-recovery-only budget (up to 2 extra `infer()` + // calls). Before this fix, `retryable`/`timeout` re-entered this same + // director budget on top of the harness's exhausted 3, multiplying to 9. + // After this fix, `retryable`/`timeout`/`quota_exhausted` are harness-only + // (bounded at 3, asserted against createDefaultRetryPolicy behavior in + // retry-policy.test.ts), and `aborted` is director-only: each of the + // director's up-to-3 infer() calls (1 initial + 2 recoveries) is a single + // harness attempt because the harness's own policy never retries + // `aborted`. Worst case across a turn that alternates categories is + // bounded, not open-ended, and never reaches 9. + test("worst case: director-owned recovery path issues at most 1 + MAX_INFERENCE_RECOVERIES infer calls", async () => { + const director = createChatDirector("system", [], { + onTasksChange: () => {}, + provider: providerlessPolicy, + }); + const capabilities = makeCapabilities(); + const internalAbort = inferenceErrorEvent("aborted", { origin: "internal-recovery" }); + + let inferCount = 0; + for (let i = 0; i < 10; i++) { + const actions = actionsArray(await director.decide(internalAbort, mockState, capabilities)); + if (actions.some((a) => a.type === "infer")) inferCount++; + else break; + } + expect(inferCount).toBe(2); // MAX_INFERENCE_RECOVERIES + }); + + test("timeout category produces the timeout preamble, not the fatal fallback", async () => { + const director = createChatDirector("system", [], { + onTasksChange: () => {}, + provider: providerlessPolicy, + }); + const capabilities = makeCapabilities(); + + const actions = actionsArray( + await director.decide(inferenceErrorEvent("timeout"), mockState, capabilities), + ); + const reply = actions.find((a) => a.type === "reply"); + expect(reply).toBeDefined(); + expect((reply as { content: string }).content).toContain("did not respond in time"); + expect((reply as { content: string }).content).not.toContain("unrecoverable inference error"); + }); +}); diff --git a/src/agent/director.ts b/src/agent/director.ts index 085e7091..7fc147b5 100644 --- a/src/agent/director.ts +++ b/src/agent/director.ts @@ -677,22 +677,55 @@ class ChatDirectorImpl extends DefaultDirector { const recovery = this.compaction.interceptOverflow(event, capabilities); if (recovery !== null) return recovery; + // Only `aborted` (internal-recovery-abort) lands here: the harness's own + // retry policy already owns `timeout`/`retryable`/`quota_exhausted` and + // has exhausted its own attempt budget (up to MAX_ATTEMPTS full-context + // sends, see vendor/intx-inference/src/retry-policy.ts) before an + // `inference.error` of one of those categories ever reaches the + // director. Re-wrapping an already-exhausted harness retry in another + // `capabilities.infer()` call multiplied the two budgets instead of + // composing them (up to 9 identical full-context sends per turn, + // CL-6910) without recovering anything the harness had not already + // tried. Internal-recovery-abort is different: the harness's default + // policy never retries `aborted` at all, so this remains the only + // layer that owns that category, and it does not compound with the + // harness's own attempts. if ( event.type === "inference.error" && - (event.error.category === "timeout" || - event.error.category === "retryable" || - (event.error.category === "aborted" && isInternalRecoveryAbort(event))) + event.error.category === "aborted" && + isInternalRecoveryAbort(event) ) { if (this.inferenceRecoveries < MAX_INFERENCE_RECOVERIES) { this.inferenceRecoveries++; + logger.warn`inference-recovery attempt=${String(this.inferenceRecoveries)} max=${String(MAX_INFERENCE_RECOVERIES)} category=${event.error.category}`; return [capabilities.checkpoint("inference-recovery"), capabilities.infer()]; } + logger.warn`inference-recovery-exhausted max=${String(MAX_INFERENCE_RECOVERIES)} category=${event.error.category}`; return [ capabilities.checkpoint("inference-recovery-exhausted"), capabilities.reply("The request could not recover. Send a message to resume."), ]; } + // The vendored DefaultDirector's inference.error preamble map + // (vendor/intx-inference/src/default-director.ts, ERROR_PREAMBLE) has no + // `timeout` entry, so it falls back to the `fatal` wording ("... + // unrecoverable inference error"). Before CL-6910, a `timeout` reaching + // the director was rare (the harness retried it first, then the director + // recovered it again — see the block above), so operators almost never + // saw that fallback text. Now an exhausted `timeout` routinely lands here + // as a terminal reply, so the misleading "unrecoverable" wording would + // become the routine message for an ordinary timeout. Intercept it here + // with accurate, calm wording rather than patching the vendored map. + if (event.type === "inference.error" && event.error.category === "timeout") { + return [ + capabilities.checkpoint("inference-error"), + capabilities.reply( + "This agent's request timed out because the inference provider did not respond in time. The request was retried and gave up.", + ), + ]; + } + // Both nudge budgets are monotonic per inbound user message rather than // resetting on "real" tool work. Classifying a tool call as progress is // gameable: a weak model learns that any tool call (including a no-op diff --git a/src/agent/retry-policy.test.ts b/src/agent/retry-policy.test.ts index c9a4391c..4efd459a 100644 --- a/src/agent/retry-policy.test.ts +++ b/src/agent/retry-policy.test.ts @@ -129,6 +129,57 @@ describe("createCorbitsRetryPolicy", () => { expect(await policy(bare429)).toEqual({ kind: "retry", delayMs: 500 }); }); + // CL-6910: the harness only surfaces `inference.error` to the director + // once this policy returns `abort` — so the attempt cap here IS the + // on-wire send cap for these categories (the director no longer re-wraps + // them, see director.test.ts). Bound at 3 sends for each error class the + // ticket names: rate limit (quota_exhausted), gateway error and malformed + // response (both normalized to retryable/protocol_mismatch here). + test("rate limit (quota_exhausted) aborts by the 3rd attempt — bounds harness sends to 3", async () => { + const policy = createCorbitsRetryPolicy(); + const situation = (attempt: number) => ({ + attempt, + elapsedMs: 0, + error: { + category: "quota_exhausted" as const, + message: "Too Many Requests", + statusCode: 429, + retryAfterMs: 10, + }, + }); + expect(await policy(situation(1))).toEqual({ kind: "retry", delayMs: 10 }); + expect(await policy(situation(2))).toEqual({ kind: "retry", delayMs: 10 }); + expect(await policy(situation(3))).toEqual({ kind: "abort" }); + }); + + test("gateway error (retryable) aborts by the 3rd attempt — bounds harness sends to 3", async () => { + const policy = createCorbitsRetryPolicy(); + const situation = (attempt: number) => ({ + attempt, + elapsedMs: 0, + error: { category: "retryable" as const, message: "gateway timeout" }, + }); + expect(await policy(situation(1))).toEqual({ kind: "retry", delayMs: 500 }); + expect(await policy(situation(2))).toEqual({ kind: "retry", delayMs: 1000 }); + expect(await policy(situation(3))).toEqual({ kind: "abort" }); + }); + + test("malformed response (HTML gateway page) aborts by the 3rd attempt — bounds harness sends to 3", async () => { + const policy = createCorbitsRetryPolicy(); + const situation = (attempt: number) => ({ + attempt, + elapsedMs: 0, + error: { + category: "protocol_mismatch" as const, + message: "malformed JSON in SSE data payload", + raw: HTML_503, + }, + }); + expect(await policy(situation(1))).toEqual({ kind: "retry", delayMs: 500 }); + expect(await policy(situation(2))).toEqual({ kind: "retry", delayMs: 1000 }); + expect(await policy(situation(3))).toEqual({ kind: "abort" }); + }); + test("live providerId getter: xAI → non-xAI stops remapping bare 429", async () => { let current: string | undefined = "xai/thegreataxios"; const policy = createCorbitsRetryPolicy({ providerId: () => current }); diff --git a/src/director.test.ts b/src/director.test.ts index 281969a5..5095c8fb 100644 --- a/src/director.test.ts +++ b/src/director.test.ts @@ -460,23 +460,23 @@ describe("chatDirector compaction", () => { expect(resumed.some((a) => a.type === "infer")).toBe(true); }); - test("retries recoverable inference failures within a bounded budget", async () => { + // CL-6910: `timeout`/`retryable` are owned entirely by the harness's own + // retry policy, which already retries and exhausts them before an + // `inference.error` of one of those categories ever reaches the director. + // The director re-issuing another `infer()` here used to multiply with + // the harness's own attempts (up to 9 identical full-context sends per + // turn); it now falls through to the base director's terminal + // checkpoint + reply instead of recovering. + test("does not re-issue inference for a timeout already exhausted by the harness", async () => { const director = chatDirectorWithContinuation(); const timeout = { type: "inference.error", error: { category: "timeout", message: "request timed out" }, } as unknown as ReactorInboundEvent; - for (let i = 0; i < 2; i++) { - const actions = actionsArray(await director.decide(timeout, longState, mockCapabilities)); - expect(actions.some((action) => action.type === "infer")).toBe(true); - } - - const exhausted = actionsArray(await director.decide(timeout, longState, mockCapabilities)); - expect(exhausted).toEqual([ - { type: "checkpoint", message: "inference-recovery-exhausted" }, - { type: "reply", content: "The request could not recover. Send a message to resume." }, - ]); + const actions = actionsArray(await director.decide(timeout, longState, mockCapabilities)); + expect(actions.some((action) => action.type === "infer")).toBe(false); + expect(actions.some((action) => action.type === "reply")).toBe(true); }); test("recovers an internally aborted inference but keeps explicit abort terminal", async () => {