fix(streaming): wrap mid-stream request errors - #3827
Conversation
![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: f62ba8948f
ℹ️ 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".
| except APITimeoutError as exc: | ||
| error = _request_error_from_api_error(exc) | ||
| self.on_timeout() | ||
| self.on_exception(error or exc) |
There was a problem hiding this comment.
When a user hook called by _emit_sse_event makes another SDK request that fails with APITimeoutError or APIConnectionError, this handler also catches that exception; because its cause is an HTTPX RequestError, _request_error_from_api_error mistakes it for a failure from for event in stream, passes the raw HTTPX exception to on_exception, and subsequently re-raises that raw exception (while the timeout case also invokes on_timeout). This unintentionally changes the callback's exception contract rather than only preserving compatibility for transport failures from the streamed response; iterator advancement and hook execution need separate exception handling, including in the mirrored async branch.
AGENTS.md reference: AGENTS.md:L5-L8
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 80686f3624
ℹ️ 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".
| except timeout_exceptions() as err: | ||
| raise APITimeoutError(request=response.request) from err | ||
| except request_exceptions() as err: | ||
| raise APIConnectionError(request=response.request) from err |
There was a problem hiding this comment.
Although the Assistants hook handling is now separated, the fresh core loop still places process_data(...) inside the new catch scope. When a custom ModelBuilderProtocol.build implementation or Pydantic validator makes an SDK call that raises APITimeoutError/APIConnectionError with an HTTPX RequestError cause, these handlers catch and replace it with a new error associated with the streaming request; this changes the application callback's exception identity and request context in both sync and async streams. Restrict these catches to iterator advancement, as was done for the Assistants layer.
AGENTS.md reference: AGENTS.md:L5-L8
Useful? React with 👍 / 👎.
|
@codex review |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
|
Codex Review: Didn't find any major issues. Swish! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
|
Looks ready now :) Codex re-re-reviewed the latest commit and the security review is clean. Thanks for taking a look! |
1 participant
Changes being requested
Fixes #3811.
HTTPX request errors raised while consuming a streaming response currently escape as raw HTTPX/HTTPX2 exceptions instead of the SDK's
APIErrorhierarchy.This change makes streaming error handling consistent with the initial request path:
APITimeoutErrorRequestErrors are wrapped asAPIConnectionError__cause__The compatibility helper covers both
httpx2and a loaded legacyhttpxmodule.Using
RequestErrorrather than onlyTransportErroralso covers decoding failures such asDecodingError, while avoiding a broadexcept Exceptionthat would incorrectly convert unrelated parsing or application errors intoAPIConnectionError.The Assistants streaming helpers have an existing compatibility contract where timeout callbacks receive and re-raise the underlying HTTPX exception. This patch preserves that behavior by unwrapping the request error from the SDK exception only in the Assistants event-handler layer.
This change intentionally does not retry partially consumed streams, since retry/resumption semantics after chunks have already been emitted are a separate concern.
Tests
Added regression coverage for sync and async streams for:
ReadTimeout→APITimeoutErrorRemoteProtocolError→APIConnectionErrorDecodingError→APIConnectionErrorAlso verified the existing Assistants timeout callback behavior remains unchanged.
Validation completed locally:
./scripts/lint: passedgit diff --check: passedAdditional context & links
This overlaps with #3813, #3814, and #3818.
This version incorporates issues identified in the existing review discussions:
preserves the existing Assistants timeout callback and raw HTTPX exception behavior:
fix(streaming): wrap mid-stream transport errors as APITimeoutError/APIConnectionError #3818 (comment)
limits wrapping to HTTPX request failures rather than arbitrary exceptions:
fix(streaming): wrap mid-stream transport errors as APITimeoutError/APIConnectionError #3818 (comment)
covers HTTP decoding failures that are
RequestErrors but notTransportErrors:fix: wrap transport failures while consuming a stream #3814 (comment)