Stop retry amplification: director no longer re-wraps harness-exhausted retries (#595)

* Stop director recovery from re-wrapping harness-exhausted retries

The harness's own retry policy already retries and exhausts
timeout/retryable/quota_exhausted errors (up to 3 attempts) before an
inference.error of one of those categories reaches the director. The
director's recovery layer re-issued another full-context infer() call
for the same categories, multiplying with the harness's own attempts
instead of composing with them -- up to 9 identical full-context sends
per logical turn with no wall-clock ceiling.

Director recovery now only handles internal-recovery-abort, the one
category the harness never retries on its own, so the two layers no
longer multiply. Attempt counts are logged on each recovery.

* Give an exhausted timeout its own reply preamble

The vendored DefaultDirector's ERROR_PREAMBLE map has no timeout entry
and falls back to the fatal ('unrecoverable inference error') wording.
CL-6910 makes an exhausted timeout the routine terminal state instead
of a rarity, so intercept it in ChatDirector and reply with accurate,
calm wording instead of patching the vendored map.

Author: TheGreatAxios

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

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");
+  });
+});

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

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 });

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 () => {