fix(streaming): wrap mid-stream transport errors as APITimeoutError/APIConnectionError - #3818
fix(streaming): wrap mid-stream transport errors as APITimeoutError/APIConnectionError#3818adhavan18 wants to merge 1 commit into
Conversation
…PIConnectionError _base_client wraps the initial send so httpx transport failures surface as APIError subclasses, but Stream/AsyncStream.__stream__ iterated the response with no handling at all. A read timeout or dropped connection mid-stream escaped as a raw httpx exception, so `except openai.APIError` around a streaming call missed the most common streaming failure and max_retries was never consulted for it. Wraps the iteration in the same try/except pattern _base_client already uses: timeout_exceptions() -> APITimeoutError, an intentionally-raised OpenAIError (the in-stream error-event case) re-raised as-is, anything else -> APIConnectionError.
![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: b5502eb26b
ℹ️ 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".
| except timeout_exceptions() as err: | ||
| raise APITimeoutError(request=response.request) from err |
There was a problem hiding this comment.
When an Assistants stream raises httpx2.ReadTimeout, this conversion means AssistantEventHandler.__stream__ and its async counterpart no longer match their _timeout_exceptions() branches, so they invoke only on_exception() and skip on_timeout(). This also changes the exception exposed by until_done() and breaks the existing sync and async callback expectations in tests/test_httpx2.py::test_assistant_stream_timeout_callbacks_preserve_httpx2_family; recognize APITimeoutError in those handlers (or preserve the transport exception for that path).
Useful? React with 👍 / 👎.
| except Exception as err: | ||
| raise APIConnectionError(request=response.request) from err |
There was a problem hiding this comment.
This catch also converts exceptions unrelated to the connection into APIConnectionError: malformed SSE data can raise UnicodeDecodeError or JSONDecodeError, and custom stream implementations can raise ordinary application exceptions. That changes their semantics and already makes the sync and async interruption checks in tests/test_sse_framing.py fail because the expected RuntimeError is replaced; restrict this branch to the HTTPX/HTTPX2 transport-error families so only the intended mid-stream failures are wrapped.
Useful? React with 👍 / 👎.
1 participant
Changes being requested
_base_clientwraps the initial send —httpx.TimeoutExceptionbecomesAPITimeoutError, other transport errors becomeAPIConnectionError, and both go through the retry loop. Once the response is streaming,Stream.__stream__/AsyncStream.__stream__in_streaming.pyiterated the response with no exception handling at all, so a read timeout or a dropped connection mid-stream surfaced as the rawhttpx.ReadTimeout/httpx.RemoteProtocolError, not anopenai.APIError.That means
except openai.APIErroraround a streaming call misses the most common streaming failure, andmax_retriesis never consulted for it.The fix wraps the iteration loop in both
__stream__methods with the same pattern_base_client.pyalready uses for the initial request:timeout_exceptions()→APITimeoutError, an already-raisedOpenAIError(the in-streamerror-event case a few lines up) re-raised as-is so it isn't double-wrapped, anything else →APIConnectionError. Whether a partially-consumed stream can itself be retried is a separate question — this only fixes the catch side, matching what the non-streaming path already does.Also fixed in
anthropic-sdk-python(same generated base, same gap): anthropics/anthropic-sdk-python#1919Additional context & links
Closes #3811
Verified against the repro script from the issue (mock transport that yields one chunk then raises
ReadTimeout): before the fix,isinstance(e, openai.APIError)isFalse; after, it raisesAPITimeoutErrorandisinstanceisTrue.Added
test_sync_stream_wraps_mid_stream_transport_errorandtest_async_stream_wraps_mid_stream_transport_errortotests/test_streaming.py— both confirmed to fail on the unfixed code (rawhttpx2.ReadTimeoutescapes) and pass with the fix. Fulltests/test_streaming.pysuite (22 tests) passes. The broader test run has unrelated pre-existing collection errors from missing optional dev deps (rich,botocore, etc.) in this environment — not touched by this change.