fix(providers): retry streaming requests dropped before first body bytes - #10779
fix(providers): retry streaming requests dropped before first body bytes#10779vincenzopalazzo wants to merge 2 commits into
Conversation
3cb2cf0 to
200ca24
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 200ca2404f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The with_retry scope in the OpenAI and OpenAI-compatible providers ended the moment response headers arrived, so an SSE connection cut before delivering any body (reqwest's "error decoding response body") surfaced as a mid-stream item error where retry is structurally unreachable, aborting the whole turn. Await the first body chunk inside the with_retry closure and prepend it to the reconstructed body stream, so pre-first-token failures — the common case with long-lived SSE connections — are transparently retried with the existing backoff. should_retry(NetworkError) is no longer dead code for this failure mode. Adds a regression test with a raw TCP server that drops the first connection after 200 headers and serves a full SSE response on retry.
200ca24 to
c0bdfbb
Compare
With connection-close body framing, a 200 response closed immediately after its headers makes Response::chunk() return Ok(None) instead of an error, so the retry scope completed successfully and the reconstructed stream yielded nothing. An empty body is never a valid answer to a streaming request — treat it as a retryable pre-first-token failure. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1c9d632a8d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| response: Response, | ||
| log: Option<Box<dyn RequestLogHandle>>, | ||
| ) -> Result<MessageStream, ProviderError> { | ||
| stream_openai_compat_with_prefix(response, None, log) |
There was a problem hiding this comment.
The compatibility wrapper still passes None, so existing callers other than OpenAiCompatibleProvider retain the original failure mode. For example, OpenRouter ends its with_retry after handle_status in crates/goose/src/providers/openrouter.rs:347-360 and then calls this wrapper at line 362; GitHub Copilot and both Databricks implementations follow the same ordering. For those providers, a 200 followed by a disconnect before any body bytes still surfaces outside the retry scope and aborts the turn. These callers should pre-read inside their retry closures and invoke the prefix variant as well.
Useful? React with 👍 / 👎.
|
Thank you for the work on this — it addresses a real reliability gap. We've captured the issue as #10926: the HTTP request can successfully establish a stream, ending the existing request retry scope, but reading or decoding that stream can still fail before the first response item reaches the agent. There are two open approaches to the same problem here and in #10534. Before choosing an implementation, we should agree in #10926 whether the retry boundary belongs around the first raw body chunk or the first parsed provider-stream item, along with replay-safety constraints and a verification plan. I've moved the issue to Accepted / design for that discussion. Per CONTRIBUTING.md, implementation should follow once the issue reaches Ready and the PR should link it. Please bring the design trade-offs and regression-test approach from this PR to the issue; they are useful input to that decision. |
3 participants
Summary
error decoding response body) bypassed goose's provider retry entirely:with_retrywrapped only the HTTP request andhandle_status(resp), so the retry scope ended the moment response headers arrived. A cut SSE connection surfaced as a per-itemErr(NetworkError)inside the body stream, long after any retry scope, and the agent's reply loop aborted the whole turn on the first stream error.should_retry(NetworkError) = truewas dead code for the one network error that actually occurs with SSE providers — it could only fire for pre-headers failures.first_body_chunk) inside thewith_retryclosure inopenai.rs(chat completions + responses API paths) andopenai_compatible.rs, then prepend it to the reconstructed body stream (stream_openai_compat_with_prefix/stream_responses_compat_with_prefix). Any failure before the first token — the common case — is now transparently retried with the existing backoff.Test plan
openai::tests::stream_retries_when_connection_drops_before_first_body_bytes: raw TCP server returns 200 headers with a promised 1024-byte body then drops the connection; retry serves a full SSE response. Verified the test fails without the fix and passes with it.cargo test -p goose-providers(87 passed)cargo clippy -p goose-providers -p goose --all-targets -- -D warnings