fix(runtime): derive model retryability from the failure kind alone - #5318
Conversation
36c32b9 to
bb8453b
Compare
A 429 with no Retry-After failed the Turn immediately. A commandcode gateway
throttle ({code:'rate_limit_error', "Upstream model provider is temporarily
unavailable. Please try again."}) ended the Turn with retry
{decision:'declined', because:'policy'} even though the same response says to
retry.
The root cause is that retryMetadataFromFacts had grown into a second
classifier, re-deciding from status and code what classifyProviderFacts had
already decided:
- a numeric `status` fallback that retried 408/409/5xx behind the kind's back,
so the kind and the retry answer could disagree;
- three different meanings for a malformed Retry-After — ignored for
provider_capacity and 429 rate limits, fatal for 5xx and network, and the
sole retry evidence for a text-only rate limit;
- RUNTIME_RETRYABLE_ERROR_CODES, which made three runtime codes retryable
before classification ran, including one that classifies as `unknown`;
- `status === 409`, introduced by #1425 with no test and no provider evidence.
The classifier is now the only authority, through one exhaustive table.
MODEL_FAILURE_RETRY maps every ModelFailureKind to the ProviderRetryReason it
retries under, or to null: network, provider_capacity, provider_unavailable,
rate_limit, stream_truncated and timeout map to their same-named reason, the
other six to null. A new kind that forgets a row fails to compile. Two guards
survive because they are facts about the error rather than its class: an abort,
and a Codex edge rejection whose transport already spent its full
2/10/30-second budget.
Behavior changes:
- a bare 429 is retryable (the reported failure);
- a 5xx or transport failure with a malformed Retry-After is retryable rather
than fatal — the header is ignored and the local backoff paces it;
- a fetch timeout (kind timeout) is retryable, matching MODEL_STREAM_TIMEOUT,
which was retryable only through the code list;
- 409 is no longer retried; it classifies as request_rejected. The upstream AI
SDK's APICallError.isRetryable defaults to treating 409 as retryable and
#1425 most likely copied that, but this repo has no path that produces a 409
and no test that ever pinned the behavior, so it follows the table;
- OPENAI_RESPONSES_CONTINUATION_UNAVAILABLE now classifies as network like its
sibling websocket code, instead of being retryable while classified
`unknown`;
- FreeUsageLimitError is newly added to PROVIDER_BILLING_PROVIDER_CODES. On
main it classified as rate_limit and failed fast only by accident, through
the "a 429 with no Retry-After is not retryable" rule this commit deletes.
#3115's case (an exhausted OpenCode Zen free tier on 429) now fails fast by
an explicit rule instead, at the cost of a kind change from rate_limit to
provider_billing — the persisted errorClass and the user-facing guidance
change with it.
Known cost: a quota-exhaustion 429 that ships no structured billing code
(Gemini daily quota, OpenRouter free-models-per-day, Anthropic OAuth usage
limit) now spends the whole 10-attempt-per-step budget before the Turn reports
a terminal failure — about 159s of local backoff, up to about 199s with
jitter, so roughly three minutes. Not fixed by vetoing 429 with
USAGE_LIMIT_TEXT_PATTERNS: Gemini's per-minute throttle reads "Quota exceeded
for quota metric…" too, so that veto would fail genuine throttles on the first
attempt — exactly the behavior this commit repairs. Only a structured billing
code is an honest signal, and these bodies carry none.
Retry-After is still capped at MAX_SAFE_TIMER_DELAY_MS (~24.8 days) and a
longer delay is discarded rather than clamped, as on main for rate_limit; this
commit neither widens that bound nor adds a clamp. A Turn abort interrupts the
wait.
Consumer redundancy the table removes: ai-sdk-turn's `context_overflow` retry
branch duplicated the `!failure.retryable` branch immediately below it;
ai-sdk-turn's own providerRetryReason switch, a second hand-maintained copy of
the kind-to-reason mapping, is now the same table; and the Codex history
compactor's fallback gate tested `!diagnostic.retryable` on a request_rejected
diagnostic, which is now identically false.
Unchanged: the 10-attempt-per-step budget and its backoff, the abort and Codex
edge guards, durable retryable fields and protocol shapes (including the
ProviderRetryReason `unknown` member, now unreachable from this call site but
still accepted by the Turn protocol), and @maka/eval.
Generated-by: Claude Code
bb8453b to
1587cdd
Compare
jackwener
left a comment
There was a problem hiding this comment.
Independent agent review — scope ①: classifier contract and the per-member kind table. Reviewed at 1587cddc507c0e126182f3e00f10e25529e47777. I am an AI agent (executing seat @kabi-opus) publishing through a shared GitHub account; this is an automated review, not a human sign-off. Other scopes of this PR (the ai-sdk-turn retry loop, and downstream consumers) are being covered by a separate reviewer and are explicitly outside what I verified.
No P0–P2 findings in my scope. One disclosure gap is noted below as P3.
What I actually checked
I enumerated every behavioural difference between the old multi-source derivation and the new table in packages/runtime/src/provider-error-classification.ts:153 / :247, rather than reading the new table for plausibility:
| Removed branch | Effect on behaviour |
|---|---|
RUNTIME_RETRYABLE_ERROR_CODES.has(code) → true |
Equivalent. Those codes classify to network/timeout, both retryable in the table. |
rate_limit || status===429 requiring a valid Retry-After, else false |
The fix. A 429 without a parseable header is now retryable. |
retryAfterMs === null → false |
Improvement. A malformed header no longer vetoes an otherwise-retryable class. This is a second, distinct bug the PR closes; the description only names the 429 case. |
status === 408 |
Preserved via 408 → timeout. |
status >= 500 |
Preserved via 5xx → provider_unavailable. |
status === 409 |
Dropped — falls to 4xx → request_rejected → not retryable. See below. |
The 409 change is the only status whose retryability actually flips. I checked whether it was deliberate before writing it up, and it is: packages/runtime/src/__tests__/provider-error-classification.test.ts:323 pins the case with the comment "The AI SDK's own isRetryable flag calls 409 retryable; the kind decides." So this is a documented, tested design decision, not an oversight, and I am not reporting it as a defect.
I also confirmed the two deletions in ai-sdk-turn.ts match their table cells, since a table change and a branch deletion have to be correct simultaneously — otherwise you get "the table says retryable but the loop declines", or the reverse:
- The local
providerRetryReasonswitch is replaced by the exported table-backed function atpackages/runtime/src/ai-sdk-turn.ts:2087, with?? 'unknown'preserving the olddefault:arm. Equivalent. - The deleted
failure.kind === 'context_overflow'decline branch is redundant, not load-bearing: the table maps that kind tonull, so the earlier!retryablegate produces the samedeclined/policyoutcome. It does not touch the overflow recovery (compaction) path, which branches earlier.
Evidence
Built from source at the reviewed SHA and ran the runtime suites covering this file: 86 tests, 86 pass, 0 fail.
A passing suite proves nothing on its own, so I ablated the fix: flipping the rate_limit cell to null turned three tests red, including the two named after the contract itself — derives retryability from the failure kind alone and Retry-After only sets the delay, never the retryability. I confirmed the ablation marker reached the emitted dist/ artifact before trusting the run. The table is genuinely pinned by tests named after the invariant, which is the form that survives refactoring.
P3 — behaviour change not disclosed in the description
The description states the known cost of the 429 change (a quota 429 without a structured billing code now burns the full local backoff). It does not mention that 409 stops being retryable, and it does not mention that a malformed Retry-After no longer vetoes retry. Both are real behaviour changes; the 409 rationale currently lives only in a test comment. This is a documentation gap, not a code defect — worth a line in the description so the change is discoverable without reading the tests.
Boundaries of this review
I did not verify: the retry loop's interaction with the unified stream-recovery work, cumulative backoff behaviour under concurrent sub-agents, or downstream consumers of providerRetryReason. I did not call any paid provider API and did not re-run evals, so every claim above is about code and local suites — not observed provider behaviour. The author's own local results were treated as context, not as my evidence.
Code review and merge readiness are separate. At the reviewed SHA the two reported checks are green, mergeable=MERGEABLE, mergeStateStatus=BLOCKED (these are different fields and can hold simultaneously). This approval covers code in my scope only and is not a statement that the PR may be merged.
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed exact head 1587cddc507c0e126182f3e00f10e25529e47777. I found no P0-P3 issues in the downstream-consumer and retry-loop scope.
The change makes ModelFailureKind the retry authority, lets malformed or absent Retry-After fall back to local backoff, recognizes OpenCode FreeUsageLimitError as billing, and removes redundant retry derivation from AiSdkTurn and the Codex compactor.
I traced the changed semantics through Runtime Host projection, the live UI retry banner, canonical request telemetry, and eval admission/relay. None assumes that rate_limit is terminal: retry events remain nonterminal, terminal failure presentation still comes only from the final turn state, each physical attempt retains its own failure fingerprint, and eval admission continues to classify provider-response evidence rather than Runtime retry progress.
I also tested the apparent adapter bypass. A provider stream error shaped as {type: "model_failure", kind: "rate_limit", retryable: false} is not trusted through the production MockLanguageModelV4 -> ModelAdapter -> AiSdkBackend path: AI SDK wraps it, Maka classifies it as unknown/nonretryable, and the provider is called once. Only the direct internal translateChunk seam can preserve that forged object, so I did not count it as a reachable finding.
Verification: build:test, full typecheck/lint/format, ASF headers, diff check, Runtime 3467 pass / 13 skip, focused Runtime/UI/Eval suites 185/185, hosted test/label, and a clean merge tree with current main 2bc8a548e21184eac0706c19d3e2a79dcc81592d. Three independent ablations failed as intended: disabling the rate_limit table cell broke both classification and production-backend retries; removing FreeUsageLimitError broke the billing regression; restoring malformed-header veto broke both classification and backend recovery. Adding a synthetic future failure kind produced TS2741, confirming the table is compile-time exhaustive.
I did not call paid providers or rerun evaluations, so real gateway wording and timing remain outside this review.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
|
Retraction of the P3 in my review ( In my review I reported as a P3 that two behaviour changes — 409 no longer being retried, and a malformed Both are in the description's "Behavior changes" list, items 2 and 4, and the 409 entry carries a fuller rationale than the test comment I cited: it notes that the upstream AI SDK's The description's edit history shows its last revision preceded my review by over an hour, so this is not a case of the text arriving late — I asserted that something was missing from a document I had not read. Everything else in my review came from reading the diff, the tests and the built output; this one claim was a negative about the description itself, and a negative claim needs the same verification as a positive one. I did not give it any. The rest of the review stands as published. With this P3 withdrawn, my scope reports no findings at all. |
Summary
A 429 with no
Retry-Afterfailed the Turn immediately. A commandcode gateway throttle ({code:'rate_limit_error', message:"Upstream model provider is temporarily unavailable. Please try again."}) ended the Turn withretry {decision:'declined', because:'policy'}even though the same response says to retry.The root cause is that
retryMetadataFromFactshad grown into a second classifier, re-deciding from status and code whatclassifyProviderFactshad already decided:statusfallback that retried 408/409/5xx behind the kind's back, so the kind and the retry answer could disagree;Retry-After— ignored forprovider_capacityand 429 rate limits, fatal for 5xx and network, and the sole retry evidence for a text-only rate limit;RUNTIME_RETRYABLE_ERROR_CODES, which made three runtime codes retryable before classification ran, including one that classifies asunknown;status === 409, introduced by refactor(runtime): make a single provider step the ModelAdapter primitive #1425 with no test and no provider evidence.The new contract:
ModelFailureKindis the only authority for retryability, expressed as one exhaustive table.MODEL_FAILURE_RETRYmaps every kind to theProviderRetryReasonit retries under, or tonull:network,provider_capacity,provider_unavailable,rate_limit,stream_truncatedandtimeoutmap to their same-named reason, the other six tonull. A new kind that forgets a row fails to compile. Two guards survive because they are facts about the error rather than its class: an abort, and a Codex edge rejection whose transport already spent its full 2/10/30-second budget.Behavior changes:
Retry-Afteris retryable rather than fatal — the header is ignored and the local backoff paces it;timeout) is retryable, matchingMODEL_STREAM_TIMEOUT, which was retryable only through the code list;request_rejected. The upstream AI SDK'sAPICallError.isRetryabledefaults to treating 409 as retryable and refactor(runtime): make a single provider step the ModelAdapter primitive #1425 most likely copied that, but this repo has no path that produces a 409 and no test that ever pinned the behavior, so it follows the table;OPENAI_RESPONSES_CONTINUATION_UNAVAILABLEclassifies asnetworklike its sibling websocket code, instead of being retryable while classifiedunknown;FreeUsageLimitErroris newly added toPROVIDER_BILLING_PROVIDER_CODES. Onmainit classified asrate_limitand failed fast only by accident, through the "a 429 with noRetry-Afteris not retryable" rule this PR deletes. fix: fail bare rate limits and show retry waits #3115's case (an exhausted OpenCode Zen free tier on 429) now fails fast by an explicit rule instead, at the cost of a kind change fromrate_limittoprovider_billing— the persistederrorClassand the user-facing guidance change with it.Known cost: a quota-exhaustion 429 that ships no structured billing code (Gemini daily quota, OpenRouter free-models-per-day, Anthropic OAuth usage limit) now spends the whole 10-attempt-per-step budget before the Turn reports a terminal failure — about 159s of local backoff, up to about 199s with jitter, so roughly three minutes. Not fixed by vetoing 429 with
USAGE_LIMIT_TEXT_PATTERNS: Gemini's per-minute throttle reads"Quota exceeded for quota metric…"too, so that veto would fail genuine throttles on the first attempt — exactly the behavior this PR repairs. Only a structured billing code is an honest signal, and these bodies carry none.Retry-Afteris still capped atMAX_SAFE_TIMER_DELAY_MS(~24.8 days) and a longer delay is discarded rather than clamped, as onmainforrate_limit; this PR neither widens that bound nor adds a clamp. A Turn abort interrupts the wait.Consumer redundancy the table removes:
ai-sdk-turn'scontext_overflowretry branch duplicated the!failure.retryablebranch immediately below it;ai-sdk-turn's ownproviderRetryReasonswitch, a second hand-maintained copy of the kind-to-reason mapping, is now the same table; and the Codex history compactor's fallback gate tested!diagnostic.retryableon arequest_rejecteddiagnostic, which is now identically false.Unchanged: the 10-attempt-per-step budget and its backoff, the abort and Codex edge guards, durable
retryablefields and protocol shapes (including theProviderRetryReasonunknownmember, now unreachable from this call site but still accepted by the Turn protocol), and@maka/eval.Refs #3115
Verification
npm --workspace @maka/runtime run buildpackages/runtime:node --test --test-concurrency=4 dist/__tests__/{provider-error-classification,model-adapter,model-adapter-onerror,provider-request-telemetry,ai-sdk-backend,openai-codex-history-compactor,overflow-reactive-recovery,openai-responses-model-adapter,openai-responses-websocket}.test.js— 421 tests, 421 pass, 0 fail.provider-error-classification.test.js(19 tests): mappingrate_limittonull→ 3 failures; givingrequest_rejecteda retry reason → 1 failure; making a malformedRetry-Afterreturn a delay instead ofundefined→ 1 failure.npm run format,npm run lint— clean.AI use
Select exactly one:
Tool(s) and scope: Claude Code — classifier refactor, consumer cleanup, tests, and this description.
Checklist
Does this PR entail a change in behavior?