fix(providers): stop killing streaming responses at the total request timeout - #10620
Conversation
|
I think this will be necessary - wonder if it has to be as cross cutting as this (maybe it does). Looks tricky, but yes please. |
333dfb8 to
425dfee
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: 425dfeea74
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4bccd34ec9
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4ed3ad0ac0
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
4ed3ad0 to
d7ac23d
Compare
d7ac23d to
2cc9cdb
Compare
2cc9cdb to
a3e5fbe
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a3e5fbe526
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
a3e5fbe to
bedfd26
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bedfd26bac
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| let client = Client::builder() | ||
| .timeout(Duration::from_secs(DEFAULT_PROVIDER_TIMEOUT_SECS)) | ||
| .connect_timeout(Duration::from_secs(DEFAULT_CONNECT_TIMEOUT_SECS)) | ||
| .read_timeout(Duration::from_secs(DEFAULT_PROVIDER_TIMEOUT_SECS)) |
There was a problem hiding this comment.
For GCP Vertex streaming calls, this client now has only connect/read timeouts, while send_request_with_retry later awaits .send() directly. With a large prompt or image payload and a proxy/provider that accepts the connection but stops reading the request body, no read is in progress so read_timeout will not fire, and the turn can hang before any SSE body exists; before this diff the client-level total timeout bounded that phase. Please wrap the .send() phase in the configured deadline and only drop the total deadline after the response stream is opened.
Useful? React with 👍 / 👎.
| let client = Client::builder() | ||
| .timeout(StdDuration::from_secs(DEFAULT_PROVIDER_TIMEOUT_SECS)) | ||
| .connect_timeout(StdDuration::from_secs(DEFAULT_CONNECT_TIMEOUT_SECS)) | ||
| .read_timeout(StdDuration::from_secs(DEFAULT_PROVIDER_TIMEOUT_SECS)) |
There was a problem hiding this comment.
Kimi streaming requests use this client in post() and then await .send() without adding a per-request timeout. After replacing the previous total client timeout with only read_timeout, a large request body can hang indefinitely if the connection is accepted but the peer stops reading, because the stream body has not started and no read timeout applies. Please keep a total deadline around the request upload/time-to-headers phase and remove it only for the opened response stream.
Useful? React with 👍 / 👎.
| reqwest::Client::builder() | ||
| .timeout(Duration::from_secs(DEFAULT_PROVIDER_TIMEOUT_SECS)) | ||
| .connect_timeout(Duration::from_secs(DEFAULT_CONNECT_TIMEOUT_SECS)) | ||
| .read_timeout(Duration::from_secs(DEFAULT_PROVIDER_TIMEOUT_SECS)) |
There was a problem hiding this comment.
Gemini OAuth streaming posts use this shared client and later call .send() directly, so replacing the total timeout with only read_timeout leaves the pre-response phase unbounded. If a large payload is sent through a proxy/provider that accepts the connection but stops reading the body, the turn can stall before any SSE response exists; the old client-level timeout capped that. Please apply the configured deadline to .send() and only exempt the response body from the total timeout.
Useful? React with 👍 / 👎.
bedfd26 to
e3032f0
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e3032f0a92
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
|
||
| /// Streaming requests carry no total-request deadline, so error-body reads | ||
| /// must be bounded or a server that drips a non-2xx body stalls retries forever. | ||
| const ERROR_BODY_READ_TIMEOUT: Duration = Duration::from_secs(30); |
There was a problem hiding this comment.
In this revision the new ERROR_BODY_READ_TIMEOUT hard-codes a 30s status-body read for every handle_status caller. When a streaming request gets non-2xx headers and the user configured a shorter provider timeout (for example OPENAI_TIMEOUT=5 or timeout_seconds: 5), retries and failure still wait up to 30s here before returning, so the configured timeout no longer bounds the non-generation error path that was moved out from under reqwest's total deadline. Thread the client's configured timeout into this read instead of using a global cap.
Useful? React with 👍 / 👎.
… timeout reqwest's client-level timeout is a total-request deadline that includes the streamed body, so any model turn longer than 600s died mid-stream with a decode error. Apply the total deadline per request and exempt requests marked .streaming(true); streams are bounded by read_timeout (reset per chunk) and connect_timeout instead. Providers with hand-rolled clients (GCP Vertex, Kimi, Gemini OAuth, device flow, ChatGPT Codex, Bedrock Mantle) get the same connect/read bounds, plus per-request total deadlines on their non-streaming calls. Error-body reads on paths that serve streaming are bounded separately since those requests no longer carry a total deadline.
e3032f0 to
748e6e7
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5ba84157d6
ℹ️ 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".
| pub async fn handle_response( | ||
| response: Response, | ||
| timeout: Duration, | ||
| ) -> Result<Value, ProviderError> { |
There was a problem hiding this comment.
Changing handle_response to require a Duration also changes the re-exported handle_response_openai_compat, but several unchanged callers still invoke it with one argument, including azure_foundry.rs:276, openai_compatible.rs:188, and openai.rs:580. cargo check -p goose-providers --lib now fails with E0061, so the crate cannot build until those paths pass a timeout or the legacy alias preserves the old signature.
Useful? React with 👍 / 👎.
| pub fn timeout(&self) -> Duration { | ||
| self.timeout | ||
| } |
There was a problem hiding this comment.
This newly added timeout method duplicates the existing timeout method later in the same impl ApiClient block, and cargo check -p goose-providers --lib fails with E0592 before downstream crates can build. Remove one of the definitions before merging.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 585460f563
ℹ️ 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".
| ApiClient::with_timeout_and_tls( | ||
| format!("http://{}", addr), | ||
| AuthMethod::NoAuth, | ||
| Duration::from_millis(timeout_ms), | ||
| None, |
There was a problem hiding this comment.
In environments with HTTP_PROXY/HTTPS_PROXY set and NO_PROXY not covering 127.0.0.1, this helper sends the mock-server requests through the proxy instead of the local TcpListener. I ran cargo test -p goose-providers api_client::tests in this runner and the four new timeout tests failed by receiving proxy responses (403 from envoy or empty bodies); setting NO_PROXY=127.0.0.1,localhost makes the local-server path work. Please disable proxies for this test client or otherwise make the loopback tests hermetic.
Useful? React with 👍 / 👎.
| pub async fn read_error_body(response: Response, timeout: Duration) -> Option<String> { | ||
| tokio::time::timeout(timeout, response.text()) | ||
| .await |
There was a problem hiding this comment.
When a streaming request gets non-2xx headers near the configured deadline, send_bounded has already allowed up to the provider timeout for upload/time-to-headers; this line then starts another full timeout for the error body, so a setting like OPENAI_TIMEOUT=5 can spend nearly 10 seconds per attempt before retrying or failing. Fresh evidence in this revision is that the hard-coded error-body cap was replaced with this second configured timeout instead of sharing a single deadline across send plus status handling.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1c9c3242cc
ℹ️ 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".
|
|
||
| async fn send_bounded(&self, request: reqwest::RequestBuilder) -> Result<Response> { | ||
| if self.streaming { | ||
| Ok(crate::http_status::send_bounded(request, self.client.timeout).await?) |
There was a problem hiding this comment.
When a streaming request times out or fails before headers, http_status::send_bounded returns a ProviderError::NetworkError, but this ? wraps that typed error in anyhow::Error. The streaming providers then call response_post(...).await? inside ProviderError retry closures, and From<anyhow::Error> does not recover an embedded ProviderError, so the retryable network error becomes ExecutionError("Network error: ...") and skips the provider retry budget. Please avoid routing this path through anyhow or preserve embedded ProviderError during conversion.
Useful? React with 👍 / 👎.
4 participants
Problem
Model turns streaming longer than 10 minutes die with
Stream decode error: error decoding response body.In headless
goose runthe reply loop treats this as terminal and silently ends the session.Long turns are routine with high thinking effort, large single-file generations, and slow local models.
Root cause:
ApiClientsets reqwest's client-level.timeout(600s), a total request deadline that includes the streamed body, so it caps turn duration instead of connection health.Reproduced on Terminal-Bench 2.1: all 5 regex-chess trials died at exactly 600-601s.
Fix
The client uses
connect_timeout(30s)+read_timeout(timeout); the total deadline is applied per request only to non-streaming requests, and provider SSE call sites are marked.streaming(true).Streaming requests still bound everything before the response body (connect, request upload, first byte) with the configured timeout; only the streamed body itself is exempt.
Providers with their own reqwest clients (gcpvertexai, kimicode, gemini_oauth, OAuth device flow, ChatGPT Codex, Bedrock Mantle) get the same split.
Error-body reads on paths that lost the total deadline are bounded (30s), including 200-status JSON error payloads on the Tetrate and Snowflake streaming endpoints.
The Anthropic factories now honor
timeout_seconds/ANTHROPIC_TIMEOUT, matching openai.rs.Scope: this PR is transport-only.
An earlier revision also retried mid-stream network errors in the agent loop; that was split out so agent-layer retry has a single owner in #10533.
Validation
The A/B ran with the since-removed agent retry included; it fired on 1 of the 20 trials, and the other recoveries came from the transport fix alone.
Tradeoff: worst-case hung-stream detection stays 600s (of silence).
Fixes #9413