feat(streaming): normalize errors raised while reading streams - #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! |
marcuswood-oai
changed the title
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Castiron custom code✅ No new custom-code files detected. 43 mixed files remain; 0 existing customizations changed. Compared 43 existing customizations unchanged
3 more in the full report. A changed generated baseline means this report cannot reliably identify which handwritten lines changed. Inspect the custom-code diffDownload the exact patch produced by this run (requires repository access): gh run download 34538355473 --repo openai/openai-python \
--name castiron-custom-code-34538355473-1 --dir /tmp/castiron-custom-code-34538355473-1
git apply --stat /tmp/castiron-custom-code-34538355473-1/custom-code.patch
cat /tmp/castiron-custom-code-34538355473-1/custom-code.patchOr reproduce it from an SDK checkout containing the vendored reporter: git fetch --no-tags origin ae41bc46cdd53d17e948ac8a73e16bdbf34123a1 6982b4bb1fc01bddadd7ef8f62f15f5e5d1a691d
python3 scripts/castiron/custom_code_report.py report \
--base ae41bc46cdd53d17e948ac8a73e16bdbf34123a1 \
--head 6982b4bb1fc01bddadd7ef8f62f15f5e5d1a691d --fetch --require-head-hash --public \
--out /tmp/castiron-custom-code-6982b4bb1fc0
cat /tmp/castiron-custom-code-6982b4bb1fc0/custom-code.patchThis is the current full custom patch for mixed files, not an attribution of only the handwritten lines changed by this PR. |
|
Thanks for the fix and careful testing! Merged. |
|
Thank you! Glad the fix and tests were useful :) |
2 participants
Summary
A timeout or broken connection after streaming starts currently escapes as a raw HTTPX exception, bypassing
except openai.APIError. Wrap request failures at the event-read boundary, preserving the original exception as__cause__and closing the response.Keep parsing and callback errors unchanged. Preserve existing Assistants event-handler and raw byte-stream behavior. Partially consumed streams are not retried.
Fixes #3811. Supersedes the overlapping fixes in #3813, #3814, and #3818.
Release note
Release as a minor version.
StreamandAsyncStreamnow raiseopenai.APITimeoutErrorfor read timeouts andopenai.APIConnectionErrorfor other HTTPX request failures, including decoding errors. Applications catching rawhttpxorhttpx2exceptions during event streaming should catch these SDK exceptions instead. The underlying exception remains available through__cause__.Validation
API/mock-server and network-dependent suites were excluded from the broad local run. The separate optional legacy aiohttp adapter test could not run because
httpx_aiohttpis not installed.