Skip to content

fix(sdk): recover mid-stream provider 5xx/429 like severed connections - #1174

Open
nordicnode wants to merge 1 commit into
CodebuffAI:mainfrom
nordicnode:oss/recover-provider-5xx-midstream-1155
Open

fix(sdk): recover mid-stream provider 5xx/429 like severed connections#1174
nordicnode wants to merge 1 commit into
CodebuffAI:mainfrom
nordicnode:oss/recover-provider-5xx-midstream-1155

Conversation

@nordicnode

Copy link
Copy Markdown

Summary

  • a provider-reported 500/429 arriving mid-stream — the openai-compatible shim enqueues it as an error part with finishReason='error' — no longer ends the entire run with an error
  • extend classifyThrownStreamRecovery (already consulted for both the thrown-iterator and error-chunk shapes) so retryable APICallErrors (429, any 5xx) take the same capped recovery path as a severed body: note injected into the conversation, retry step forced, capped at MAX_CONSECUTIVE_STREAM_RECOVERIES
  • the recovery message names the HTTP status instead of claiming the connection dropped
  • client-error statuses (400/401/402/403) are deterministic — retrying cannot help — so they stay fatal and still propagate to the run's error handling, including the existing 402 rethrow
  • add regression tests: 500 and 429 recover with a status-naming message, a wrapped 503 behind a RetryError cause chain recovers, client-error statuses and post-cancel 5xx stay fatal

This addresses the run-killing part of #1155: the recoverable class was "the connection failed to speak" (isTransientNetworkError, a socket-level allowlist) and the fatal class was "the provider reported a failure", which is backwards for flaky endpoints, where both are the same transient event.

Validation

Re-run on the new main after the history rewrite:

  • bun test sdk/src/__tests__/stream-interruption.test.ts — 19 pass, 0 fail
  • bunx tsc --noEmit -p sdk/tsconfig.json clean

Refs #1155


Replaces #1162 — auto-closed when the repo history was force-pushed (the original commits no longer exist in the new history). Rebased onto the new main per the maintainer's request; picking the review up here.

A provider-reported 500/429 arriving mid-stream — the openai-compatible
shim enqueues it as an error part with finishReason='error' — was thrown
straight out of the stream and ended the entire run with an error. The
same underlying transient event surfacing as a severed body instead
took the capped recovery path (note injected into the conversation,
retry step forced, capped at MAX_CONSECUTIVE_STREAM_RECOVERIES).

The recoverable class was 'the connection failed to speak' and the
fatal class was 'the provider reported a failure', which is backwards
for flaky endpoints, where both are the same transient event.

Route retryable APICallErrors (429, any 5xx) through the same capped
recovery path with a message naming the HTTP status. Client-error
statuses (400/401/402/403) are deterministic — retrying cannot help —
so they stay fatal and still propagate to the run's error handling.

Refs CodebuffAI#1155
@codebuff-team

Copy link
Copy Markdown
Contributor

Good, surgical change. classifyThrownStreamRecovery in sdk/src/impl/stream-interruption.ts previously only recovered from socket-level failures (isTransientNetworkError) and treated a provider-reported 500/429 mid-stream as fatal, which is backwards — both are the same transient upstream event. Adding the extractApiErrorDetails branch to route 429/5xx through the same capped stream-interrupted recovery path, while leaving 400/401/402/403 fatal (preserving the existing 402 rethrow), is the right fix and matches the reasoning in #1155.

The new tests in stream-interruption.test.ts cover the important cases: bare 500/429 recovery with a status-naming message, a RetryError-wrapped 503 behind a cause chain, client errors staying fatal, and post-cancellation 5xx staying fatal (no aborted bypass). That's a reasonable spread for a classifier function like this.

One thing worth double-checking before porting: extractApiErrorDetails isn't shown in this diff, so I'm trusting it already walks cause chains correctly for wrapped RetryErrors — the third test only asserts .source is 'stream-interrupted', not the exact status extracted, so a maintainer should confirm the message still names the right HTTP status (503) in that wrapped case, not undefined.

Scope and size are appropriate — small, targeted, single concern, no touches to forbidden paths. This looks portable as-is.

@codebuff-team codebuff-team added bot:triaged Classified by the community triage bot pr:port-candidate Worth porting into the private source tree labels Sep 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bot:triaged Classified by the community triage bot pr:port-candidate Worth porting into the private source tree

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants