| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
maxAttempts sits on the per-reason Retryable descriptor but is compared against Schedule's per-request attempt counter, which every retry reason shares. The name reads as a per-reason budget, so rename it to maxTotalAttempts and document the sharing at both sites. No behavior change. Adds the mixed-reason test the suite was missing: two rate-limit retries leave the output-limit path no resample.
| Back | FazBrowse Home | New Git URL |
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:
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
Tests
Written for commit 76a7a6c. Summary will update on new commits.