fix(runtime): classify provider capacity errors - #3365
Conversation
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for this — classifying on the provider's structured code/type rather than on message text is the right foundation, and matching the existing CONTEXT_OVERFLOW_PROVIDER_CODES shape means this slots in without inventing a new mechanism. The copy and recovery-hint plumbing through to the desktop is complete and the tests assert the classification and the retry metadata rather than restating the implementation.
No [P0]/[P1]. One [P2] that is really a question about the problem definition, and two [P3]s.
The [P2]: resource-exhausted / resource_exhausted is not a single meaning across providers. In gRPC and the Google API error model, RESOURCE_EXHAUSTED (status 8) is the standard code for quota exhaustion — per-minute, per-day, or per-project — not for "the server is busy right now". The user-facing copy this PR routes it to is 模型服务暂时满载,请等待几分钟或切换模型后重试 / Wait a few minutes or switch models, and that advice is actively wrong for a daily quota: waiting a few minutes will not help, and the correct action is different.
The 429 branch above catches the common case, since a Gemini quota error usually carries HTTP 429 and returns RateLimit before reaching this check. So this is not a blanket misclassification. But that also means the capacity branch is reached precisely when the status code is absent or non-429 — which is the case where the code alone is carrying the whole meaning, and where it is most ambiguous.
Could you say which providers you observed emitting these two codes, and with which meaning? If the set is narrow and "server at capacity" is what they actually mean, then pinning that in the comment next to PROVIDER_CAPACITY_CODES resolves it and I have no further concern. If it turns out the same code also arrives for quota exhaustion, the classification is fine but the recovery copy needs to not promise that waiting works.
I want to be explicit that I am asking rather than asserting: I have not seen your traces, and you may well have picked these two spellings from concrete provider payloads that mean exactly what you say.
Review assisted by AI (Claude Opus 5). Findings were verified against the files at this head; the reviewer is accountable for them.
| ]); | ||
|
|
||
| /** Provider codes meaning the model is temporarily at capacity. */ | ||
| const PROVIDER_CAPACITY_CODES: ReadonlySet<string> = new Set([ |
There was a problem hiding this comment.
[P2] See the review body. Short version: in the gRPC / Google API error model RESOURCE_EXHAUSTED is the standard code for quota exhaustion, not "server temporarily busy". This branch is only reached when the error did not already classify as RateLimit via 429, i.e. exactly when the code is carrying the meaning by itself.
If these two spellings came from concrete payloads that genuinely mean "at capacity", a one-line note here naming the providers would settle it permanently — the next person to add a code to this set will need the same reasoning, and right now the comment says what the code means but not who sends it or how you established that.
| const errorClass = classifyProviderFacts(facts); | ||
| const retryAfterMs = parseRetryAfterMs(facts.responseHeaders ?? {}); | ||
| if (errorClass === 'ProviderCapacity') { | ||
| if (retryAfterMs === null) return { retryable: false }; |
There was a problem hiding this comment.
[P3] This treats an absent Retry-After as more retryable than a malformed one, which inverts the usual information ordering.
parseRetryAfterMs returns undefined when neither header is present, and null when a header is present but unusable (unparseable, <= 0, or beyond MAX_SAFE_TIMER_DELAY_MS). So: no header at all → retryable: true and the caller backs off on its own; Retry-After: 0 or a garbage value → retryable: false and the turn does not retry at all.
A provider that sends a broken header ends up strictly worse off than one that sends none, even though the underlying condition is identically transient. The RateLimit branch below collapses both to non-retryable, which is defensible there because the server's own window is the whole point — but capacity is retried with local backoff, so the malformed case has a sensible fallback available and does not use it.
Not a blocker: it fails closed, and the user can retry by hand.
| ); | ||
|
|
||
| assert.equal(classifyError(capacity), 'ProviderCapacity'); | ||
| assert.deepEqual(providerRetryMetadata(capacity), { retryable: true }); |
There was a problem hiding this comment.
[P3] Object.assign(capacity, { responseHeaders: ... }) mutates capacity in place rather than deriving a new error, so after this line the object asserted on above no longer has the shape it was asserted with. It happens to be harmless in the current order, but it makes the two assertions look independent when they are not — if anyone later reorders them or adds a case in between, the earlier assert.deepEqual(providerRetryMetadata(capacity), { retryable: true }) starts failing for a reason that has nothing to do with the code under test.
{ ...capacity, responseHeaders: ... } would not work here since these are Error instances, but building a second error the same way you built the first would.
Worth saying that the test is otherwise well-shaped: it pins both code spellings, covers the top-level code carrier as well as the nested data.error.code one, and asserts retryAfterMs rather than only the boolean.
| case 'rate_limit': | ||
| case 'timeout': | ||
| return kind; | ||
| case 'provider_capacity': |
There was a problem hiding this comment.
[P3] The new class is collapsed back to provider_unavailable here, so everything downstream of ProviderRetryReason loses the distinction this PR just introduced — including the retry banner copy, which will say 模型服务暂时不可用 / Model service temporarily unavailable rather than anything about capacity.
That may well be deliberate: ProviderRetryReason is a narrower vocabulary than ModelFailureKind and widening it touches the retry banner in both locales. If so, it is worth one line of comment here saying the narrowing is intentional, because as written it reads like the case was added only to satisfy the switch.
(Credit where due — this is the one thing the other reviewer on our side and I independently landed on, so it does stand out to a reader.)
There was a problem hiding this comment.
Thanks for calling this out. The narrowing was not intentional; it did lose the user-visible distinction in the live retry banner.\n\nFixed in commit d473c5e: ProviderRetryReason now includes provider_capacity, the Runtime mapping and Runtime Host decoder preserve it, the compatibility epoch was bumped to 30 for the wire-contract change, and both locales have capacity-specific retry copy. Focused Runtime, Runtime Host protocol, and UI projection tests cover the full path.
|
Heads up — this is currently conflicting with One thing worth knowing: #3397 landed on 2026-08-22 and added ASF license headers across ~2685 files, so the rebase will touch more than you'd expect, and any file you add now needs a header ( Ping me once it's rebased and I'll pick it up. |
d473c5e to
4f00034
Compare
|
@Astro-Han PR #3365 已完成 rebase,当前基于最新 main(f19eede03),远端 head 为 4f00034。冲突已解决,构建和相关功能测试已通过;当前 PR 状态为 MERGEABLE,剩余 BLOCKED 原因为 REVIEW_REQUIRED,请继续处理。 |
| if (lower === 'provider_billing' || lower === 'auth' || lower.includes('auth') || lower === '401' || lower === '403') { | ||
| return { action: 'check_connection', label: copy.connection }; | ||
| } | ||
| if (lower === 'provider_capacity') { |
There was a problem hiding this comment.
[P2] The capacity branch sits ahead of the partial-output and successful-tool guards, so a Turn that already produced work is told to retry — and retrying can re-run tool side effects.
if (lower === 'provider_capacity') return { action: 'retry', label: copy.capacity };
if (input.partialOutputRetained) return { action: 'continue', label: copy.partial };
if (input.toolActivityCount > 0) return { action: 'inspect_tool', label: copy.toolRecord };Only the errored-tool guard runs before capacity; the two guards that protect retained output and successful tool activity run after it.
That combination is reachable. A Turn completes an assistant/tool step, the next model request comes back resource-exhausted, and all 10 physical attempts for the current step fail before producing new output. Auto-retry only asks whether this attempt has observable output (ai-sdk-backend.ts:2530-2602), so the Turn can terminate with errorClass=provider_capacity while partialOutputRetained=true or toolActivityCount>0. Calling the presentation function directly on this head:
- capacity +
partialOutputRetained=true→retry / Wait a few minutes or switch models before retrying - capacity +
toolActivityCount=1→ the same retry prompt
Why the consequence is worse than a mislabel: the user follows that advice and clicks Regenerate, and session-manager.ts:4846-4903 resubmits the original Turn's user content — so tool side effects that already succeeded can happen a second time. "Continue" and "inspect tool" exist precisely to keep completed work from being redone.
The existing suite says as much: session-status-presentation.test.ts:60-80 explicitly treats these two as higher-priority prompts. The new capacity tests only cover the 0-output / 0-tool case, so the ordering regression passes unnoticed.
Minimal fix: move the capacity branch below the partial-output and successful-tool guards and above the generic output-free fallback, and add a regression for capacity with retained output or tool activity.
Independent line, bound to 4f000347. Verified at the gate 2026-08-23 13:12 UTC.
There was a problem hiding this comment.
已在 8ebbb02 修复:partialOutputRetained 和 toolActivityCount 现在先于 provider_capacity 处理,分别引导 continue / inspect_tool;新增这两个 mixed-state 回归断言。已通过 Desktop recovery 测试。
| if (statusCode === '429' || code === '429') return 'RateLimit'; | ||
| if (statusCode === '401' || statusCode === '403' || code === '401' || code === '403') | ||
| return 'Auth'; | ||
| if ( |
There was a problem hiding this comment.
[P2] Weak wrapper evidence outranks the exact capacity code, so the classification this PR is built on is lost whenever a transport layer wraps the error.
if (text.includes('abort')) return 'Abort';
if (statusCode === '402' || code === '402') return 'ProviderBilling';
if (statusCode === '429' || code === '429') return 'RateLimit';
if (statusCode === '401' || statusCode === '403' || ...) return 'Auth';
if (PROVIDER_CAPACITY_CODES.has(normalizedCode) || structuredCodes.some(...)) return 'ProviderCapacity';Free-text abort matching and the generic numeric fallbacks all run before the precise structured code. Probed directly on this head:
{statusCode: 429, data: {error: {code: 'resource-exhausted'}}}→RateLimit, and with noRetry-Afterpresent,retryable=false{message: 'request aborted because model at capacity', data: {error: {code: 'resource-exhausted'}}}→Abort
The same verified xAI code loses its capacity copy and its bounded local backoff purely because an SDK or gateway added an outer transport status or a descriptive phrase. The 429 case is the sharper one: it does not merely relabel, it can flip retryable to false — turning a condition this PR wants to back off from into one that does not retry at all.
The existing tests only exercise payloads with no outer status and no abort wording, so the precedence problem is invisible from inside them.
Why reordering is safe here specifically: this head already excludes the underscore form resource_exhausted, whose meaning is broader (in Google/gRPC it commonly means quota). Promoting the exact hyphenated code above weak wrapper evidence therefore does not re-blur the Google/gRPC quota case that exclusion was added to avoid.
Minimal fix: match the exact resource-exhausted structured/top-level code before free-text abort and the generic numeric fallbacks, and add a 429-wrapper regression. This is also the precedence #2521 already adopts — structured provider identifiers ahead of ambiguous transport facts — so the two changes agree on the principle.
Independent line, bound to 4f000347. Verified at the gate 2026-08-23 13:12 UTC.
There was a problem hiding this comment.
已在 8ebbb02 修复:保留显式 RetryError abort,但 provider 的精确 resource-exhausted 结构化证据现在优先于 free-text abort 和 402/429/401/403 fallback。新增 abort-text 与 429 混合载荷回归断言,并验证 capacity 仍可重试。
|
Independent review of
The bigger question is not in this diff: duplicate authority with #2521Inside this branch there is no second classifier — it plugs into the existing classification, retry, wire, and Desktop presentation chain. Of the 18 files, 7 are tests and the other 11 are real core-type / runtime / strict-wire / Desktop boundaries. Deleting files is not the simplification available here. The actual duplication is with #2521, which is open on the same baseline
That is not a third finding against this diff, but it is a merge-order and design gate: two implementations of the same concept should not both land. If #2521 is the intended authority, the low-entropy path is for this PR to rebase and contribute Candidates raised and withdrawn
State of the head
Blind line: provisional judgment sealed before any existing review was read. Reviewed at 2026-08-23 13:12 UTC. No overall verdict offered. |
4f00034 to
8ebbb02
Compare
|
Both [P2]s are fixed on the new head Presentation — capacity moved below the two guards it was jumping: if (input.partialOutputRetained) return { action: 'continue', label: copy.partial };
if (input.toolActivityCount > 0) return { action: 'inspect_tool', label: copy.toolRecord };
if (lower === 'provider_capacity') return { action: 'retry', label: copy.capacity };A Turn that already retained output or ran a tool successfully now gets continue / inspect tool instead of retry, so the path that could re-run completed tool side effects is closed. Classification — the exact structured code now precedes the weak wrapper evidence: if (PROVIDER_CAPACITY_CODES.has(normalizedCode) || structuredCodes.some(...)) return 'ProviderCapacity';
...
if (text.includes('abort')) return 'Abort';
if (statusCode === '429' || code === '429') return 'RateLimit';
Two things still open, neither of them mine to close:
I will re-check once CI reports. |
|
To clarify the integration recommendation for #2521 and this PR: These are not merely adjacent changes. #2521 establishes a broader provider-failure authority ( I recommend choosing one authority before merging:
Landing both independently would leave two classification authorities and makes divergent retry/recovery behavior and protocol conflicts likely. This is an architectural merge-order decision rather than a routine conflict to resolve after both land. |
|
Fixed in c001f6b.
|
Astro-Han
left a comment
There was a problem hiding this comment.
Approving c001f6b85adf6d4268b91e414b0d1a9d9036c1b2. Required test is completed / success bound to that exact SHA. No P0–P3.
Re-review at the current head. Both earlier [P2]s were re-derived here rather than accepted as fixed on the strength of the "Fixed in c001f6b" note.
Ordering fix, verified in place. In classifyProviderFacts, the PROVIDER_CAPACITY_CODES test against structuredCodes now sits above text.includes('abort'), above the 429 branch and above the 5xx branch. That is the whole point of the finding: a transport layer wrapping the error used to let weak textual evidence outrank the exact capacity code the PR is built on. It no longer can. providerFailureDiagnostic also retains ProviderCapacity rather than widening it back to RateLimit/ProviderUnavailable.
Guard ordering fix, verified in place. In session-status-presentation.ts, partialOutputRetained and toolActivityCount are handled before the capacity branch, so provider_capacity only reaches a Turn that retained no output and ran no tools. That matters beyond presentation: the old order could invite a retry on a Turn whose tool side effects had already happened.
One deliberate non-change worth recording. resource_exhausted with an underscore is still not treated as capacity. That is correct rather than an oversight — in the gRPC/Google error model RESOURCE_EXHAUSTED is the standard code for quota exhaustion, not "server temporarily busy", and folding it in would recreate the misclassification this PR exists to remove.
Coverage now spans abort text, a 429 wrapper and a 503 wrapper; focused suites 217/217.
Disclosure, because it changes what this approval is worth: this is an AI review. Under CONTRIBUTING.md §Review it does not count as the required independent human review. It means the code has been checked, not that the gate is open — merge still needs a committer other than the author to give LGTM and to decide.
|
Hi — thanks for Heads-up that this branch now conflicts with current
Worth knowing what that first conflict is, because it isn't mechanical: it's Resolving it means re-deriving your epoch bump on top of whatever Once the branch is conflict-free and CI is green on the new head, I'll pick the review back up. AI-assisted maintenance note, not a review. It does not count as the required human review under |
Fixes apache#3341 Generated-by: Codex
Keep xAI capacity errors distinct from quota exhaustion and fall back to bounded local retry when Retry-After is malformed. Generated-by: gpt-5.6-sol
Generated-by: gpt-5.6-sol
c001f6b to
569f7b0
Compare
|
Rebased and resolved in The branch is now based on current Verification after a clean workspace build:
The rebased head was pushed with |
Astro-Han
left a comment
There was a problem hiding this comment.
Approving at exact head 569f7b066dd8a9b2a8ab5963dfb484fb21f81772.
This head is the rebase of the previously-approved c001f6b85. I re-anchored rather than carrying the old approval forward: comparing the two ranges, the first two commits are byte-identical and the third differs only in how the protocol-epoch conflict was resolved — the test assertion moves from > 39 to > 41 and merges with main's compaction-epoch test. That is the expected resolution for this file, and it changes nothing about the reviewed behaviour.
CI is green on this exact head. Worth noting for the record that the run only happened after the fork workflow was approved — before that, this head had zero check-runs, so the earlier state was "never ran", not "passed".
No live [P0]–[P2] at this head.
…re authority Main's apache#3365 added ProviderCapacity classification while this branch was queued. Fold it into the unified provider-failure taxonomy: the class joins the core union, structured capacity codes rank above account-state codes, and capacity stays retryable through the shared retry metadata. Generated-by: maka
…re authority Main's apache#3365 added ProviderCapacity classification while this branch was queued. Fold it into the unified provider-failure taxonomy: the class joins the core union, structured capacity codes rank above account-state codes, and capacity stays retryable through the shared retry metadata. Generated-by: maka
Summary
Fixes #3341
Provider
resource-exhaustedfailures now retain a stableprovider_capacityclassification, use bounded retry metadata, and receive capacity-specific Desktop error and recovery guidance instead ofUnknown errorand direct-retry advice.Verification
npm installsucceeded and installed@ai-sdk/code-mode@1.0.23.npm run buildpassed for all workspaces.npm testwas attempted; it remains blocked by unrelated Windows environment failures including symlink permissions (EPERM), SQLite locks (EBUSY), missing Rive CLI, and workspace timeouts.AI use
Tool(s) and scope: Codex implemented the runtime classification, retry mapping, Desktop copy/recovery behavior, and regression tests. A
Generated-by: Codextrailer is present in the commit.Checklist
Does this PR entail a change in behavior?