From 36ca8de99828ab8cd19a1dc1efe4407a269b2e17 Mon Sep 17 00:00:00 2001 From: Bhumika Date: Sat, 5 Sep 2026 18:18:26 +0530 Subject: [PATCH] fix: only retry transport-level exceptions, not arbitrary ones 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 (#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 #2737 --- src/openai/_base_client.py | 5 +-- src/openai/_httpx2.py | 11 ++++++ tests/test_client.py | 72 ++++++++++++++++++++++++++++++++++++-- 3 files changed, 83 insertions(+), 5 deletions(-) diff --git a/src/openai/_base_client.py b/src/openai/_base_client.py index 7f92a61a86..6377ce5b77 100644 --- a/src/openai/_base_client.py +++ b/src/openai/_base_client.py @@ -65,6 +65,7 @@ from ._compat import PYDANTIC_V1, model_copy from ._httpx2 import ( status_exceptions, + request_exceptions, timeout_exceptions, http_response_types, normalize_httpx_url, @@ -1095,7 +1096,7 @@ def request( except OpenAIError as err: # Propagate OpenAIErrors as-is, without retrying or wrapping in APIConnectionError raise err - except Exception as err: + except request_exceptions() as err: log.debug("Encountered exception: %s", type(err).__name__) if remaining_retries > 0: @@ -1718,7 +1719,7 @@ async def request( except OpenAIError as err: # Propagate OpenAIErrors as-is, without retrying or wrapping in APIConnectionError raise err - except Exception as err: + except request_exceptions() as err: log.debug("Encountered exception: %s", type(err).__name__) if remaining_retries > 0: diff --git a/src/openai/_httpx2.py b/src/openai/_httpx2.py index 491398b43c..3ae327e1c2 100644 --- a/src/openai/_httpx2.py +++ b/src/openai/_httpx2.py @@ -17,6 +17,7 @@ class _LegacyHttpxModule(Protocol): Timeout: type[httpx2.Timeout] Limits: type[httpx2.Limits] TimeoutException: type[httpx2.TimeoutException] + RequestError: type[httpx2.RequestError] HTTPStatusError: type[httpx2.HTTPStatusError] StreamConsumed: type[httpx2.StreamConsumed] RequestNotRead: type[httpx2.RequestNotRead] @@ -99,6 +100,16 @@ def timeout_exceptions() -> tuple[type[httpx2.TimeoutException], ...]: return (httpx2.TimeoutException,) if module is None else (httpx2.TimeoutException, module.TimeoutException) +def request_exceptions() -> tuple[type[httpx2.RequestError], ...]: + """Exceptions raised by the transport for a single request: connection failures, protocol + errors, timeouts, and the like. Deliberately narrower than `Exception` so that unrelated + errors (e.g. a task-cancellation signal raised inside a custom transport) are not silently + retried and reported as an `APIConnectionError`. + """ + module = _loaded_legacy_httpx() + return (httpx2.RequestError,) if module is None else (httpx2.RequestError, module.RequestError) + + def status_exceptions() -> tuple[type[httpx2.HTTPStatusError], ...]: module = _loaded_legacy_httpx() return (httpx2.HTTPStatusError,) if module is None else (httpx2.HTTPStatusError, module.HTTPStatusError) diff --git a/tests/test_client.py b/tests/test_client.py index d82c39e616..418edab0cb 100644 --- a/tests/test_client.py +++ b/tests/test_client.py @@ -23,7 +23,7 @@ from openai._utils import asyncify from openai._models import BaseModel, FinalRequestOptions from openai._streaming import Stream, AsyncStream -from openai._exceptions import APIStatusError, APITimeoutError, APIResponseValidationError +from openai._exceptions import APIStatusError, APITimeoutError, APIConnectionError, APIResponseValidationError from openai._base_client import ( DEFAULT_TIMEOUT, HTTPX_DEFAULT_TIMEOUT, @@ -1208,7 +1208,7 @@ def retry_handler(_request: httpx2.Request) -> httpx2.Response: if nb_retries < failures_before_success: nb_retries += 1 if failure_mode == "exception": - raise RuntimeError("oops") + raise httpx2.ConnectError("oops") return httpx2.Response(500) return httpx2.Response(200) @@ -1227,6 +1227,40 @@ def retry_handler(_request: httpx2.Request) -> httpx2.Response: assert response.retries_taken == failures_before_success assert int(response.http_request.headers.get("x-stainless-retry-count")) == failures_before_success + @mock.patch("openai._base_client.BaseClient._calculate_retry_timeout", _low_retry_timeout) + @pytest.mark.respx2(base_url=base_url) + def test_non_transport_exceptions_are_not_retried(self, client: OpenAI, respx2_mock: MockRouter) -> None: + # Exceptions that aren't raised by the transport layer (connection failures, timeouts, + # protocol errors, ...) must propagate immediately, unmodified, and without being + # retried. Previously a bare `except Exception` treated *any* error - including ones + # unrelated to the HTTP request, such as Celery's `SoftTimeLimitExceeded` - as a + # retryable connection error, which silently discarded the original exception and any + # cleanup logic relying on it. + client = client.with_options(max_retries=4) + + nb_calls = 0 + + def raise_non_transport_error(_request: httpx2.Request) -> httpx2.Response: + nonlocal nb_calls + nb_calls += 1 + raise RuntimeError("not a transport error") + + respx2_mock.post("/chat/completions").mock(side_effect=raise_non_transport_error) + + with pytest.raises(RuntimeError, match="not a transport error") as exc_info: + client.chat.completions.create( + messages=[ + { + "content": "string", + "role": "developer", + } + ], + model="gpt-5.4", + ) + + assert not isinstance(exc_info.value, APIConnectionError) + assert nb_calls == 1 + @pytest.mark.parametrize("failures_before_success", [0, 2, 4]) @mock.patch("openai._base_client.BaseClient._calculate_retry_timeout", _low_retry_timeout) @pytest.mark.respx2(base_url=base_url) @@ -2505,7 +2539,7 @@ def retry_handler(_request: httpx2.Request) -> httpx2.Response: if nb_retries < failures_before_success: nb_retries += 1 if failure_mode == "exception": - raise RuntimeError("oops") + raise httpx2.ConnectError("oops") return httpx2.Response(500) return httpx2.Response(200) @@ -2524,6 +2558,38 @@ def retry_handler(_request: httpx2.Request) -> httpx2.Response: assert response.retries_taken == failures_before_success assert int(response.http_request.headers.get("x-stainless-retry-count")) == failures_before_success + @mock.patch("openai._base_client.BaseClient._calculate_retry_timeout", _low_retry_timeout) + @pytest.mark.respx2(base_url=base_url) + async def test_non_transport_exceptions_are_not_retried( + self, async_client: AsyncOpenAI, respx2_mock: MockRouter + ) -> None: + # See the sync counterpart above for context: a non-transport exception must propagate + # immediately, unmodified, and without being retried. + client = async_client.with_options(max_retries=4) + + nb_calls = 0 + + def raise_non_transport_error(_request: httpx2.Request) -> httpx2.Response: + nonlocal nb_calls + nb_calls += 1 + raise RuntimeError("not a transport error") + + respx2_mock.post("/chat/completions").mock(side_effect=raise_non_transport_error) + + with pytest.raises(RuntimeError, match="not a transport error") as exc_info: + await client.chat.completions.create( + messages=[ + { + "content": "string", + "role": "developer", + } + ], + model="gpt-5.4", + ) + + assert not isinstance(exc_info.value, APIConnectionError) + assert nb_calls == 1 + @pytest.mark.parametrize("failures_before_success", [0, 2, 4]) @mock.patch("openai._base_client.BaseClient._calculate_retry_timeout", _low_retry_timeout) @pytest.mark.respx2(base_url=base_url)