refactor(opencode): name the retry cap for its cumulative scope

Merged
AL
3mo ago
+34-3

Follow-up to #137. Naming and documentation only — no behavior change.

The mismatch

maxAttempts lives on the per-error descriptor returned by retryable(), so it reads as "attempts allowed for this error". It is actually compared against Schedule.InputMetadata.attempt, which metadataFn implements as a single ++n per schedule instance (effect/dist/Schedule.js:109-127) — one counter per request, incremented on every step regardless of which reason produced it.

So the field is a total, not a per-reason budget. Renamed to maxTotalAttempts and documented at both sites.

Why it matters

processor.ts:733 has one Effect.retry wrapping the whole stream drain, so two reasons share that counter:

  • rate limit / overload, surfaced from llm.stream — no cap, retries indefinitely with backoff
  • finishReason === "length" thrown at processor.ts:524 — maxTotalAttempts: 3

429, 429, finishReason=length reaches the output-limit branch at attempt=3, so 3 >= 3 gives it zero resamples and the turn fails with "Model hit its output limit". The sharing is one-directional: rate limits set no cap, so they never lose budget to a prior output-limit attempt.

The homogeneous case — the one #137 was designed around — is unaffected and stays at 2 retries / 3 calls, byte-identical to the counter it replaced (++outputLengthRetries > 2 also retried errors 1 and 2 and gave up on 3).

Which behavior is right

I did not change it. Bounding total work on a request that has already failed three times is defensible, and the consequence when it fires is one lost resample on a request where the provider was already misbehaving — no data loss, and it is cheaper rather than more expensive. The problem was that the name claimed per-reason scope while the code did cumulative, and no test pinned either reading.

If independent per-reason budgets are actually wanted, that needs a Map<reason, count> closure in policy() — which is the mutable state #137 deliberately deleted, so it should be a conscious choice rather than a silent restoration. Happy to do that instead if you prefer.

Tests

Adds the mixed-reason case the suite was missing, so the cumulative semantics are now pinned either way. It fails against a per-reason implementation, which is the point.

bun test test/session/retry.test.ts → 35 pass. bun typecheck clean.

Unrelated, noticed while verifying: test/session/processor-effect.test.ts is 1 pass / 15 fail on origin/main with Model not found: test/test-model, identical before and after this branch. The only CI workflow is typecheck, so nothing runs the suite on main. Flagging separately, not touching it here.

Summary by cubic

Renamed the retry cap to maxTotalAttempts to match the shared, cumulative attempt counter across a request. Added inline docs and a mixed‑reason test; no behavior change.

  • Refactors

    • Renamed maxAttempts to maxTotalAttempts in retryable() and policy().
    • Documented that the cap is cumulative across reasons and measured against the per-request meta.attempt.

  • Tests

    • Added a case proving earlier rate-limit retries consume the budget for a later output-limit error.