Skip to content

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

Merged
TheGreatAxios merged 2 commits into
mainfrom
cl-6910-retry-amplification-up-to-9-identical-full-context-sends-per
Aug 24, 2026
Merged

Stop retry amplification: director no longer re-wraps harness-exhausted retries#595
TheGreatAxios merged 2 commits into
mainfrom
cl-6910-retry-amplification-up-to-9-identical-full-context-sends-per

Conversation

@TheGreatAxios

@TheGreatAxios TheGreatAxios commented Aug 24, 2026

Copy link
Copy Markdown
Collaborator

Closes CL-6910.

Guardrail 4 applies (director changes): needs-sawyer-review, no auto-merge.

Verified layer map (before)

Two layers, each contributing attempts to a single logical turn:

  1. Harness retry policy (vendor/intx-inference/src/retry-policy.ts, createDefaultRetryPolicy) — a per-infer()-call mechanical retry. For retryable/timeout/quota_exhausted, up to MAX_ATTEMPTS = 3 attempts with backoff before it returns { kind: "abort" }. Only then does the harness surface an inference.error event to the caller (confirmed in vendor/intx-inference/src/harness.ts:1479-1549 — the loop only yields inference.error after decision.kind === "abort").

  2. Director recovery (src/agent/director.ts, decideInner) — on receiving an inference.error of category timeout, retryable, or aborted+internal-recovery, called capabilities.infer() again, up to MAX_INFERENCE_RECOVERIES = 2 more times, resetting at each turn boundary (inference.done) and on message.received. Each recovery also fired a checkpoint (a git commit via contextStore.commit()).

The bug: for timeout/retryable, an inference.error only ever reaches the director after the harness has already exhausted its own 3-attempt budget. The director's recovery then started a brand-new infer() call with a fresh harness-level 3-attempt budget — up to 2 more times. Worst case: (1 initial + 2 director recoveries) * 3 harness attempts = 9 identical full-context sends for a single logical turn, each director recovery also committing a checkpoint. This matches the ticket's "up to 9" exactly.

quota_exhausted was never in the director's recovery trigger, before or after this change — its director-facing behavior is unchanged by this PR. It's included above only because it shares the harness-layer 3-attempt budget with timeout/retryable; separately, src/agent/retry-policy.ts's createCorbitsRetryPolicy already aborts quota_exhausted on the first attempt when error.retryAfterMs > MAX_BLIND_WAIT_MS (30s) — a deliberate short-circuit so the harness doesn't blind-wait through a long provider-declared backoff, and unaffected by this PR.

RetrySituation.elapsedMs is threaded through by the harness (vendor/intx-inference/src/harness.ts:1532) but never read by either retry policy (createDefaultRetryPolicy / createCorbitsRetryPolicy both destructure only error/attempt) — confirming "the retry policy receives elapsed time but ignores it."

What changed (structural, not a cap)

aborted+internal-recovery is the only category the harness's own policy never retries (it's in the harness's explicit "never retry" list). So it's the only category where director-level recovery was ever additive rather than duplicative.

src/agent/director.ts: the recovery trigger now only fires for aborted+internal-recovery-abort. timeout/retryable errors that reach the director are already-exhausted harness attempts, so they now fall through to the base DefaultDirector's existing inference.error handling (checkpoint + reply), instead of being re-wrapped in another infer() call. quota_exhausted was already excluded from the director's recovery trigger before this PR and is untouched by this change.

This removes the multiplication for timeout/retryable rather than capping it: the two layers no longer share those two error categories, so there is nothing left to compound for them. No retry threshold, backoff constant, or attempt count changed — MAX_ATTEMPTS (harness) and MAX_INFERENCE_RECOVERIES (director) are both untouched.

Worst-case send count after

  • timeout/retryable: bounded at the harness's own 3 attempts, director does not add to it (this is the fix — before this PR the director could add up to 2 more full 3-attempt cycles).
  • quota_exhausted: bounded at the harness's own 3 attempts (or aborted after attempt 1 if retryAfterMs exceeds the 30s blind-wait cap) — unchanged by this PR, since the director never recovered this category.
  • aborted/internal-recovery: bounded at 3 (1 initial + MAX_INFERENCE_RECOVERIES=2 director-triggered re-invocations), each a single harness attempt since the harness never retries aborted.
  • Because a turn can in principle transition between an internal-recovery abort and a distinct retryable/timeout failure across separate infer() calls, the true worst case across a whole turn is bounded (not open-ended) at up to 5 (e.g. abort, abort, then a final call that itself exhausts 3 harness attempts) — nowhere near the prior 9, and it cannot grow further because neither layer's own cap changed.

Before: up to 9 (via timeout/retryable compounding). After: up to 5, and 3 in the common single-category case.

Number I chose, and why

No retry threshold, backoff, or attempt count was changed. The only new number is 0 — no wall-clock ceiling was added. Tonight's eval matrix recorded a single legitimate grok-4.6 turn lasting 746 seconds; any ceiling picked without data risks aborting healthy turns like that one. Per the ticket's own guidance, de-compounding is implemented here and the wall-clock ceiling is left as a follow-up once there's data on what a genuinely stalled retry sequence looks like versus a slow-but-healthy one.

Also added: a logger.warn line on each director recovery attempt and on exhaustion, stating the attempt count and category, so a retry storm is visible in traces (outcome bullet 3).

UX: timeout preamble

An exhausted timeout now routinely reaches the director's terminal reply path (previously it was retried and rarely surfaced). The vendored DefaultDirector's ERROR_PREAMBLE map (vendor/intx-inference/src/default-director.ts) has no timeout entry, so it falls back to the fatal preamble ("...unrecoverable inference error"), which is misleading for an ordinary exhausted timeout. Rather than editing the vendored file directly (out of scope for this wave per scripts/verify-corbits-only-scope.sh), src/agent/director.ts now intercepts inference.error with category timeout before it falls through to the vendored handler, and replies with calm, accurate wording instead (the provider didn't respond in time; the request was retried and gave up).

Tests

  • src/agent/retry-policy.test.ts — three new tests proving the harness-layer policy aborts by the 3rd attempt for each error class named in the ticket: rate limit (quota_exhausted), gateway error (retryable), and malformed response (protocol_mismatch HTML gateway page, normalized to retryable).
  • src/agent/director.test.ts — new describe("ChatDirector inference-error recovery (CL-6910)") block: parametrized test proving retryable/timeout/quota_exhausted no longer produce an infer action from the director; internal-recovery-abort still recovers up to MAX_INFERENCE_RECOVERIES, then replies; an unrelated (user-stop) abort is never recovered; the recovery budget resets at the next turn boundary; and a worst-case-count test asserting the director-owned path issues exactly 1 + MAX_INFERENCE_RECOVERIES infer calls, never more.
  • src/director.test.ts — updated the pre-existing "retries recoverable inference failures" test (which asserted the old, buggy re-wrapping behavior for timeout) to assert the new pass-through behavior instead.
  • src/agent/director.test.ts — new test "timeout category produces the timeout preamble, not the fatal fallback" asserting the reply text for a timeout inference.error contains the new timeout wording and not the fatal preamble's "unrecoverable inference error".

All assertions are on action/send counts, not timing.

Gate

bun run check (lint, typecheck, build, test) green in the foreground.

@linear-code

linear-code Bot commented Aug 24, 2026

Copy link
Copy Markdown

CL-6910

@TheGreatAxios
TheGreatAxios force-pushed the cl-6910-retry-amplification-up-to-9-identical-full-context-sends-per branch from 701d314 to 761492b Compare August 24, 2026 05:13
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.
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.
@TheGreatAxios
TheGreatAxios force-pushed the cl-6910-retry-amplification-up-to-9-identical-full-context-sends-per branch from 761492b to fa80d56 Compare August 24, 2026 05:16
@TheGreatAxios
TheGreatAxios enabled auto-merge (squash) August 24, 2026 05:17
@TheGreatAxios
TheGreatAxios merged commit e52cac0 into main Aug 24, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant