fix: only retry transport-level exceptions, not arbitrary ones - #3804
Open
Bhumika-1432006 wants to merge 1 commit into
Open
fix: only retry transport-level exceptions, not arbitrary ones#3804Bhumika-1432006 wants to merge 1 commit into
Bhumika-1432006 wants to merge 1 commit into
Conversation
The request retry loop in _base_client.py caught bare `Exception`, which meant any error raised while a request was in flight - including ones with nothing to do with the HTTP request itself - was treated as a retryable connection error and eventually wrapped in APIConnectionError. In particular, running the client inside a Celery task with a soft time limit causes Celery's SoftTimeLimitExceeded (a plain Exception subclass) to be swallowed by this handler and retried instead of propagating, so task cleanup/shutdown logic relying on it never runs (openai#2737). Add request_exceptions() alongside the existing timeout_exceptions()/ status_exceptions() helpers in _httpx2.py, and use it to narrow the retry-on-exception branch to httpx2.RequestError (and the legacy httpx.RequestError, for users who inject a legacy AsyncClient) - covering connection failures, protocol errors, and other genuine transport errors, while letting unrelated exceptions propagate immediately and unmodified. Fixes openai#2737
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The request retry loop in
_base_client.pycatches bareExceptionand treats it exactly like a connection failure: it retries (if retries remain) and otherwise wraps it inAPIConnectionError. That means any exception raised while a request is in flight — even one that has nothing to do with the HTTP request — gets silently swallowed and retried.Concretely, this breaks graceful shutdown for callers that run the client inside a Celery task with a soft time limit: Celery's
SoftTimeLimitExceededis a plainExceptionsubclass, so it gets caught here, retried, and the task's cleanup/shutdown logic that depends on that exception propagating never runs.Fixes #2737.
Root cause
Both the sync (
SyncAPIClient._request) and async (AsyncAPIClient._request) retry loops had this issue.Fix
request_exceptions()tosrc/openai/_httpx2.py, following the same pattern as the existingtimeout_exceptions()/status_exceptions()helpers: it returns(httpx2.RequestError,), plus the legacyhttpx.RequestErrorwhen a legacyhttpx.AsyncClient/Clienthas been injected (per the documented escape hatch).httpx2.RequestError(mirroringhttpx's hierarchy) is the base class for all genuine transport-level failures — connection errors, protocol errors, proxy errors, timeouts, etc. — so this is the right level to retry on.except Exception as err:blocks withexcept request_exceptions() as err:.Unrelated exceptions (e.g.
SoftTimeLimitExceeded, or any other exception raised by code running underneath the transport) now propagate immediately, unmodified, and without being retried or wrapped inAPIConnectionError.Tests
tests/test_client.py: the existingtest_retries_taken[exception-*]parametrization (sync + async) simulated a retryable exception with a bareRuntimeError(...). Since that's no longer retried under this fix, I changed it tohttpx2.ConnectError(...)— a genuine transport error — so it still exercises the "retry on exception" path meaningfully.test_non_transport_exceptions_are_not_retried, which raises a plainRuntimeErrorfrom the mocked transport and asserts it (a) is not retried (the mock is only called once) and (b) propagates as the originalRuntimeError, not wrapped inAPIConnectionError.All of
tests/test_client.pypasses locally (198 passed, 2 pre-existing/unrelated failures intest_proxy_environment_variablesthat reproduce identically onmainwithout this change, 2 skipped).ruff check,ruff format --check, andmypyare clean on the changed files.Notes for reviewers
except OpenAIError(re-raise) andexcept timeout_exceptions()(dedicatedAPITimeoutErrorpath) branches above it are untouched.except Exception:blocks used for best-effort cleanup elsewhere in_base_client.py(e.g. around client teardown) — those are a distinct issue (Empty exception handler in _base_client.py #3428) with a different fix shape (logging rather than narrowing).