Make chat recover from transient failures instead of ending on them - #2556
Conversation
A provider transport blip reached persistence with no structured error code and was stored as `unknown`, which the client does not list as auto-recoverable — so the turn died where the identical failure carrying its real code resumes. Production over four days: `unknown` averaged exactly 1.00 runs per turn (174 runs / 174 turns — recovery was never attempted once), against 2.0 for `provider_network_error` and 1.5 for `http_429` on the same underlying errors. That single gap was 28% of all chat turns. Four copies of the connection-error predicate had drifted apart and disagreed on the exact string the AI SDK throws: `RetryError` reports "Failed after 2 attempts. Last error: Cannot connect to API: …", which the `startsWith` copy scored as unclassified while the `includes` copy scored as retryable. Collapse them into one exported classifier, match it against the full cause chain, and apply it both where the error event is built (the code the client reads) and where the terminal run code is persisted. Transport and capacity failures map to real codes; deterministic ones stay unmapped so a broken request still stops the chat rather than spiralling. Also stop sending `reasoning_effort` alongside function tools for GPT models on the Builder gateway, which routes them to Chat Completions — a combination OpenAI rejects outright, failing every agent turn on a gpt-5.x model. Only an explicit "none" clears it; omitting the field applies the model default and fails identically.
The sweep's redispatch asserted `payloadRef: true` without checking the row still carried a `dispatch_payload`. Sweep eligibility never implied one — a background row can reach the grace window having never had a payload at all — so the redispatched worker could not rehydrate a request body and failed the run as `dispatch_payload_missing`. 98 production runs died that way, every one a scheduled job, several after the original worker had already emitted 150-240 events. `listUnclaimedBackgroundRunRows` now reports payload presence per row. It reports rather than filters deliberately: a payload-less row must stay visible to the slow sweep or it would sit in `running` forever. The fast sweep (which never reaps) skips those rows, and the slow sweep sends them straight to its existing loud reap rather than waiting out the redispatch bound for a recovery that cannot happen. The run still fails — nothing can rehydrate it — but with its real cause, `background_worker_never_started`, which the client treats as recoverable.
A rebuild correctly refuses to apply a TRAILING `clear` — there is no successor chunk to re-emit what it wipes — but it only skipped the clear at the very last index. Each failed engine attempt emits its own `clear`, so three failures in a row is the ordinary shape, and the rebuild still applied the first two, splicing every text and reasoning part out of the run. Where the run had made no tool calls this emptied the content entirely and the builder returned null, so the user's message was persisted with no assistant reply at all — a chat that "just did nothing" on a run the table records as completed. Skip the whole trailing run of clears; a `clear` with real events after it still applies. Also make `terminal_reason` write-once once a terminal row has recorded one. Three writers in three isolates race on that column — the mid-run checkpoint, the run-manager's finalization, and the background worker's failure path — with no ordering between them, and last-writer-wins let a late checkpoint relabel a run another isolate had already finalized. That produced impossible rows (status='errored' carrying a continuation reason, no error_code, no terminal event) and misattributed 130 production runs to a failure mode they never hit. A row still `running` has no honest reason yet, so it stays writable.
`streamText` does not throw for a failed provider request — it emits an `error` part on `fullStream` — so provider HTTP failures had two arrival paths and only the thrown one was classified. The stream-part path built a bare stop event from the message alone, discarding the APICallError's `statusCode` and `isRetryable`. Downstream then had nothing structured to read: a 429 or 503 was retried only if its prose happened to contain "rate_limit" or "overloaded", and the run persisted `error_code = 'unknown'`. That is also why a 100%-reproducible config 400 ran for three days across five apps unnoticed — in the outcome tables it was indistinguishable from every other unclassified failure, so it had no signature to alert on. Both paths now share one `classifyProviderError`: status code -> `http_<status>`, transport failure -> `provider_network_error`, `isRetryable` passed through, and a message-based fallback when the provider sent nothing structured. Every ai-sdk provider gets correct classification at once instead of only the engines that happen to throw.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Visual recap — skippedThe visual recap job did not run for this pull request. This is informational only and does not block the PR. Recap skipped for |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Builder reviewed your changes and found 2 potential issues 🔴
Review Details
Code Review Summary
This incremental review covers the latest PR head, which centralizes provider-error classification across thrown and streamed AI SDK failures, improves bounded continuation/retry behavior, hardens background run claiming and payload rehydration, protects terminal metadata from racing writers, and adds model/provider compatibility and integration changes. The core approach is sound and the focused lifecycle/classification suites reported by agents pass; the prior explicit-none Builder GPT issue remains open and is intentionally not reposted.
Risk assessment: Standard — this is core agent execution, recovery, and provider integration logic with broad behavioral impact.
Key Findings
- 🔴 HIGH: Abort-marker database failures are coerced into successful user stops, silently dropping valid turns.
- 🟡 MEDIUM: Stream-delivered provider 401s are not recorded as credential failures before the success cleanup path clears markers.
🧪 Browser testing: Will run after this review (PR touches UI code)
| typeof requestTurnId === "string" && | ||
| requestTurnId && | ||
| (await isTurnAborted(threadId, requestTurnId)) | ||
| (await isTurnAborted(threadId, requestTurnId).catch(() => true)) |
There was a problem hiding this comment.
🔴 Do not treat an unreadable abort marker as a user stop
isTurnAborted can reject on transient database failures, but this catch maps every rejection to true and returns { ok: true, stopped: true }, silently dropping a submitted turn (and similarly affecting background paths). Preserve or surface the infrastructure failure so it can be retried; an unreadable marker must not be treated as a durable user abort.
Additional Info
Found by 2 of 4 review agents; confirmed against the surrounding handler and run-manager fail-closed behavior.
There was a problem hiding this comment.
Fixed in aeae0ec. Abort-marker reads now propagate infrastructure failures instead of treating every rejection as a user stop across foreground, background, and continuation checks. The regression now asserts the database error is surfaced; the focused production-agent suite passes.
| events.push({ | ||
| type: "stop", | ||
| reason: "error", | ||
| error: errMsg, | ||
| ...classifyProviderError(part.error), |
There was a problem hiding this comment.
🟡 Record credential failures from streamed 401 errors
For a provider 401 delivered as a fullStream error part, this preserves statusCode but the engine only calls recordProviderCredentialAuthFailure in its thrown-error catch path. The stream then reaches the unconditional credential-clear path, so an invalid key is not marked unusable and may be retried on every turn; handle streamed 401s before clearing the marker.
Additional Info
Confirmed by tracing aiSdkPartToEngineEvents into AISDKEngine.stream; add a stream-part 401 regression test.
There was a problem hiding this comment.
Fixed in aeae0ec. Stream error events with status 401 now record the credential-auth failure before the buffered terminal event, and the success cleanup skips clearing that marker. Added a regression covering the streamed 401 path; the focused AI SDK suite passes.
Deep dive into the last week of production chat logs across all 13 template databases, then fixes for what the data actually showed.
What the data said
Measured per logical turn (what a user experiences), interactive chats only, last 4 days — 566 turns:
doneerror:unknownaborted:userno_progressandrun_timeoutlook enormous at the run level (95 and 53) but collapse to 8 and 5 dead turns — continuation is working. Essentially zero completed runs produced no assistant text. One bucket was the story, and it was the one labelled "unknown".The decisive measurement:
unknownprovider_network_errorhttp_429Same underlying failures. The client auto-recovers a fixed list of transport codes;
unknownis not on it. Classification alone was the difference between a resumed turn and a dead chat.Inside those 160: 118 TLS resets, 15 provider-overloaded, 15 a deterministic config 400, 7 an app-specific auth error, 2 gateway stream truncation.
Fixes
Transient failures were unclassified, so they were never retried. Four copies of the connection-error predicate had drifted apart and disagreed on the exact string the AI SDK throws —
RetryErrorreports"Failed after 2 attempts. Last error: Cannot connect to API: …", which thestartsWithcopy scored as unclassified while theincludescopy scored as retryable. Split brain: the agent loop retried, the run persistedunknown, the client refused to resume. Now one exported classifier, matched against the full cause chain, applied both where the error event is built and where the terminal code is persisted.Provider failures arriving as a stream part were never classified at all.
streamTextemits anerrorpart rather than throwing, and that path discardedstatusCode/isRetryableentirely. It also meant a 100%-reproducible config 400 ran three days across five apps with no signature to alert on. Both paths now share oneclassifyProviderError.Every agent turn on a
gpt-5.xmodel failed deterministically. The gateway routes those to Chat Completions, which rejectsreasoning_effortalongside function tools. Omitting the field does not help — only an explicit"none"clears it. The ai-sdk engine already had this guard; the Builder gateway path had none.A retry storm deleted the answer the user had already read. The rebuild correctly refuses to apply a trailing
clear, but only skipped the one at the last index. Each failed attempt emits its ownclear, so three failures in a row — the ordinary shape — still applied two of them and spliced the text out. With no tool calls to keep content non-empty the builder returnednulland the user's message was persisted with no assistant reply at all, on a run the table records as completed.The unclaimed-run sweep destroyed the runs it exists to recover. Its redispatch asserted
payloadRef: truewithout checking the row still had adispatch_payload; the worker then could not rehydrate and killed the run asdispatch_payload_missing. 98 production runs, every one a scheduled job, several after the original worker had already emitted 150–240 events.terminal_reasonwas last-writer-wins across three racing isolates, letting a late checkpoint relabel an already-finalized run. That produced impossible rows (status='errored'carrying a continuation reason, noerror_code, no terminal event) and misattributed 130 runs to a failure they never hit. Now write-once once a terminal row has recorded one.Honest expectations
The 15 config-400 failures are eliminated outright. The ~135 transport failures move from "never retried" into the existing bounded retry machinery (12 continuations max, exponential backoff with jitter already applied for exactly this case). Observed recovery for correctly-classified transients is roughly half to two-thirds — on a small sample (11 turns), so treat the exact figure as uncertain. Expect failed chat turns to drop by somewhere around half to two-thirds, not the full 28 points.
Deliberately not fixed here, and worth their own change:
clearis attempt-scoped when sent and turn-scoped when applied (needs a scope field on the event, not a wider special case); the identical-tool-call spiral guard is scoped to one serverless chunk so it never fires in the multi-chunk turns that actually spiral; scheduled automations treat a soft-timeout chunk boundary as a hard failure with no continuation path; andbackground_continuation_dispatch_deferredrecordsstatus='completed'.Verification
Typecheck clean. 1427 agent + shared tests pass, all repo guards pass. Two unrelated failures (
guided-questions.flow.spec.tsx, and a flaky extension e2e under parallel load) reproduce on a clean tree.Findings came from a 40-agent investigation over the production databases and the agent source, with each root cause adversarially verified against the code before any fix was written; several proposed fixes were refuted in that pass and dropped.