From d5197a53ece02b6effc0fb1215fb02a10251f22a Mon Sep 17 00:00:00 2001 From: Vlada Dusek Date: Tue, 18 Aug 2026 14:39:46 +0200 Subject: [PATCH 1/2] fix: Fail fast on transport errors a retry cannot fix --- src/apify_client/http_clients/_impit.py | 31 +++++++----- tests/unit/test_http_clients.py | 63 +++++++++++++++++++++---- 2 files changed, 74 insertions(+), 20 deletions(-) diff --git a/src/apify_client/http_clients/_impit.py b/src/apify_client/http_clients/_impit.py index 68353b1d..491d93b1 100644 --- a/src/apify_client/http_clients/_impit.py +++ b/src/apify_client/http_clients/_impit.py @@ -21,7 +21,7 @@ from apify_client._docs import docs_group from apify_client._logging import log_context, logger_name from apify_client._utils.time import to_seconds -from apify_client.errors import ApifyApiError, InvalidResponseBodyError +from apify_client.errors import ApifyApiError from apify_client.http_clients._base import HttpClient, HttpClientAsync if TYPE_CHECKING: @@ -37,20 +37,27 @@ logger = logging.getLogger(logger_name) +_PERMANENT_ERRORS = ( + # A bad URL scheme or a request Impit itself rejects cannot succeed on a retry. + impit.UnsupportedProtocol, + impit.LocalProtocolError, + # An over-long redirect chain is a routing loop, which repeating the request cannot break. + impit.TooManyRedirects, + # Status codes are decided by `_make_request` from the response itself, never as a transport error. + impit.HTTPStatusError, +) +"""Impit errors that a retry cannot fix. Everything else in the `impit.HTTPError` tree is treated as transient.""" + + def _is_retryable_error(exc: Exception) -> bool: - """Check if an exception represents a transient error that should be retried. + """Check if an exception represents a transient transport failure that should be retried. - All `impit.HTTPError` subclasses are considered retryable because they represent transport-level failures - (network issues, timeouts, protocol errors, body decoding errors) that are typically transient. HTTP status - code errors are handled separately in `_make_request` based on the response status code, not here. + Every error from Impit's own hierarchy counts as transient except the permanently-failing types listed in + `_PERMANENT_ERRORS`. Retrying is the default because Impit also reports genuinely transient failures through its + generic base class, e.g. a bare `impit.HTTPError` for a body that ends mid-chunk. HTTP status code errors are + handled separately in `_make_request` based on the response status code, not here. """ - return isinstance( - exc, - ( - InvalidResponseBodyError, - impit.HTTPError, - ), - ) + return isinstance(exc, impit.HTTPError) and not isinstance(exc, _PERMANENT_ERRORS) @docs_group('HTTP clients') diff --git a/tests/unit/test_http_clients.py b/tests/unit/test_http_clients.py index 34236887..f4033427 100644 --- a/tests/unit/test_http_clients.py +++ b/tests/unit/test_http_clients.py @@ -309,20 +309,67 @@ def test_parse_params_mixed() -> None: } +RETRYABLE_TRANSPORT_ERRORS = ( + # Impit raises a bare `HTTPError` for a body that ends mid-chunk, so even the generic base class is transient. + impit.HTTPError, + impit.TimeoutException, + impit.NetworkError, + impit.RemoteProtocolError, + impit.DecodingError, + # A proxy rejecting the tunnel is often transient, e.g. one that is overloaded or rate-limiting. + impit.ProxyError, +) +"""Transport errors that must stay retryable.""" + +NON_RETRYABLE_TRANSPORT_ERRORS = ( + impit.UnsupportedProtocol, + impit.LocalProtocolError, + impit.TooManyRedirects, + # No built-in client ever raises this one, because `_make_request` decides on status codes from the response. + impit.HTTPStatusError, +) +"""Transport errors that a retry cannot fix, so they must fail on the first attempt.""" + + def test_is_retryable_error() -> None: - """Test _is_retryable_error correctly identifies retryable errors.""" - mock_response = Mock() - assert _is_retryable_error(InvalidResponseBodyError(mock_response)) - assert _is_retryable_error(impit.NetworkError('test')) - assert _is_retryable_error(impit.TimeoutException('test')) - assert _is_retryable_error(impit.RemoteProtocolError('test')) - - # Non-retryable errors + """Transient transport failures are retried, and the ones a retry cannot fix are not.""" + for error_class in RETRYABLE_TRANSPORT_ERRORS: + assert _is_retryable_error(error_class('test')), error_class.__name__ + + for error_class in NON_RETRYABLE_TRANSPORT_ERRORS: + assert not _is_retryable_error(error_class('test')), error_class.__name__ + + # `InvalidResponseBodyError` is raised by a resource client once `call` has returned, never inside the retry loop. + assert not _is_retryable_error(InvalidResponseBodyError(Mock())) assert not _is_retryable_error(ValueError('test')) assert not _is_retryable_error(RuntimeError('test')) assert not _is_retryable_error(Exception('test')) +def test_permanent_transport_error_is_not_retried() -> None: + """A transport error a retry cannot fix fails on the first attempt instead of burning the whole backoff.""" + client = ImpitHttpClient(token='test_token', min_delay_between_retries=timedelta(0)) + request = Mock(side_effect=impit.UnsupportedProtocol('unsupported scheme')) + client._impit_client = Mock(request=request) + + with pytest.raises(impit.UnsupportedProtocol): + client.call(method='GET', url='ftp://api.test.com/endpoint') + + request.assert_called_once() + + +async def test_permanent_transport_error_is_not_retried_async() -> None: + """The async client applies the same policy, failing on the first attempt.""" + client = ImpitHttpClientAsync(token='test_token', min_delay_between_retries=timedelta(0)) + request = AsyncMock(side_effect=impit.UnsupportedProtocol('unsupported scheme')) + client._impit_async_client = Mock(request=request) + + with pytest.raises(impit.UnsupportedProtocol): + await client.call(method='GET', url='ftp://api.test.com/endpoint') + + request.assert_awaited_once() + + @pytest.fixture( params=[ pytest.param((GzipHttpCompressor(), 'gzip', gzip.decompress), id='gzip'), From fbc721672115581adb60f48128504a110fa7f4aa Mon Sep 17 00:00:00 2001 From: Vlada Dusek Date: Tue, 18 Aug 2026 15:51:31 +0200 Subject: [PATCH 2/2] refactor: Remove duplicate retry classifier and correct transport error docs --- src/apify_client/_utils/errors.py | 20 +----- src/apify_client/errors.py | 6 +- src/apify_client/http_clients/_impit.py | 18 +++-- tests/unit/test_http_clients.py | 93 +++++++++++++++---------- tests/unit/test_utils.py | 35 +--------- 5 files changed, 72 insertions(+), 100 deletions(-) diff --git a/src/apify_client/_utils/errors.py b/src/apify_client/_utils/errors.py index 989d3966..fb4ea9af 100644 --- a/src/apify_client/_utils/errors.py +++ b/src/apify_client/_utils/errors.py @@ -2,9 +2,7 @@ from typing import TYPE_CHECKING -import impit - -from apify_client.errors import InvalidResponseBodyError, NotFoundError +from apify_client.errors import NotFoundError if TYPE_CHECKING: from apify_client.errors import ApifyApiError @@ -33,19 +31,3 @@ def catch_not_found_for_resource_or_throw(exc: ApifyApiError, resource_id: str | if resource_id is None: raise exc catch_not_found_or_throw(exc) - - -def is_retryable_error(exc: Exception) -> bool: - """Check if the given error is retryable. - - All `impit.HTTPError` subclasses are considered retryable because they represent transport-level failures - (network issues, timeouts, protocol errors, body decoding errors) that are typically transient. HTTP status - code errors are handled separately in `_make_request` based on the response status code, not here. - """ - return isinstance( - exc, - ( - InvalidResponseBodyError, - impit.HTTPError, - ), - ) diff --git a/src/apify_client/errors.py b/src/apify_client/errors.py index b77d318a..7fdf432a 100644 --- a/src/apify_client/errors.py +++ b/src/apify_client/errors.py @@ -137,9 +137,9 @@ class ServerError(ApifyApiError): class InvalidResponseBodyError(ApifyClientError): """Error raised when a response body cannot be parsed. - This typically occurs when the API returns a partial or malformed JSON response, for example due to a network - interruption. The client retries such requests automatically, so this error is only raised after all retry - attempts have been exhausted. + This occurs when the API returns a body that does not match its content type, for example a malformed JSON + document. It is raised on a response the client already accepted, so it is not retried - a transfer that breaks + mid-body surfaces as a transport error inside the retry loop instead. """ def __init__(self, response: HttpResponse) -> None: diff --git a/src/apify_client/http_clients/_impit.py b/src/apify_client/http_clients/_impit.py index 491d93b1..8334da4c 100644 --- a/src/apify_client/http_clients/_impit.py +++ b/src/apify_client/http_clients/_impit.py @@ -38,24 +38,28 @@ _PERMANENT_ERRORS = ( - # A bad URL scheme or a request Impit itself rejects cannot succeed on a retry. - impit.UnsupportedProtocol, + # A request Impit rejects before sending it, e.g. one carrying an invalid header value. impit.LocalProtocolError, + # The class Impit declares for a scheme it refuses to speak, but does not raise today - it reports an unsupported + # scheme as `impit.InvalidURL`, which sits outside the `impit.HTTPError` tree and is non-retryable anyway. Listed + # so the classifier stays right if Impit switches over. + impit.UnsupportedProtocol, # An over-long redirect chain is a routing loop, which repeating the request cannot break. impit.TooManyRedirects, - # Status codes are decided by `_make_request` from the response itself, never as a transport error. + # Only `Response.raise_for_status()` raises this, and the client never calls it - `_make_request` decides on + # status codes from the response itself. impit.HTTPStatusError, ) -"""Impit errors that a retry cannot fix. Everything else in the `impit.HTTPError` tree is treated as transient.""" def _is_retryable_error(exc: Exception) -> bool: """Check if an exception represents a transient transport failure that should be retried. Every error from Impit's own hierarchy counts as transient except the permanently-failing types listed in - `_PERMANENT_ERRORS`. Retrying is the default because Impit also reports genuinely transient failures through its - generic base class, e.g. a bare `impit.HTTPError` for a body that ends mid-chunk. HTTP status code errors are - handled separately in `_make_request` based on the response status code, not here. + `_PERMANENT_ERRORS`. Retrying is the default because Impit also reports genuinely transient failures through + its generic base class, e.g. a bare `impit.HTTPError` wrapping a failure its internal HTTP library did not + classify. HTTP status code errors are handled separately in `_make_request` based on the response status code, + not here. """ return isinstance(exc, impit.HTTPError) and not isinstance(exc, _PERMANENT_ERRORS) diff --git a/tests/unit/test_http_clients.py b/tests/unit/test_http_clients.py index f4033427..213bb38d 100644 --- a/tests/unit/test_http_clients.py +++ b/tests/unit/test_http_clients.py @@ -309,51 +309,55 @@ def test_parse_params_mixed() -> None: } -RETRYABLE_TRANSPORT_ERRORS = ( - # Impit raises a bare `HTTPError` for a body that ends mid-chunk, so even the generic base class is transient. - impit.HTTPError, - impit.TimeoutException, - impit.NetworkError, - impit.RemoteProtocolError, - impit.DecodingError, - # A proxy rejecting the tunnel is often transient, e.g. one that is overloaded or rate-limiting. - impit.ProxyError, -) -"""Transport errors that must stay retryable.""" - -NON_RETRYABLE_TRANSPORT_ERRORS = ( - impit.UnsupportedProtocol, - impit.LocalProtocolError, - impit.TooManyRedirects, - # No built-in client ever raises this one, because `_make_request` decides on status codes from the response. - impit.HTTPStatusError, +@pytest.mark.parametrize( + 'exc', + [ + # Impit wraps a failure its internal HTTP library did not classify in a bare `HTTPError`, so even the + # generic base class is transient. + pytest.param(impit.HTTPError('unclassified failure'), id='bare HTTPError'), + pytest.param(impit.TimeoutException('timeout'), id='TimeoutException'), + pytest.param(impit.NetworkError('network error'), id='NetworkError'), + pytest.param(impit.RemoteProtocolError('remote protocol error'), id='RemoteProtocolError'), + pytest.param(impit.DecodingError('decoding error'), id='DecodingError'), + # One `ProxyError` covers both a proxy rejecting the CONNECT tunnel and a 407, so a transient case cannot + # be told from a permanent one - retrying is the safer default. + pytest.param(impit.ProxyError('proxy error'), id='ProxyError'), + ], ) -"""Transport errors that a retry cannot fix, so they must fail on the first attempt.""" +def test_is_retryable_error(exc: Exception) -> None: + """A transient transport failure is retried.""" + assert _is_retryable_error(exc) -def test_is_retryable_error() -> None: - """Transient transport failures are retried, and the ones a retry cannot fix are not.""" - for error_class in RETRYABLE_TRANSPORT_ERRORS: - assert _is_retryable_error(error_class('test')), error_class.__name__ - - for error_class in NON_RETRYABLE_TRANSPORT_ERRORS: - assert not _is_retryable_error(error_class('test')), error_class.__name__ - - # `InvalidResponseBodyError` is raised by a resource client once `call` has returned, never inside the retry loop. - assert not _is_retryable_error(InvalidResponseBodyError(Mock())) - assert not _is_retryable_error(ValueError('test')) - assert not _is_retryable_error(RuntimeError('test')) - assert not _is_retryable_error(Exception('test')) +@pytest.mark.parametrize( + 'exc', + [ + pytest.param(impit.LocalProtocolError('invalid header value'), id='LocalProtocolError'), + pytest.param(impit.UnsupportedProtocol('unsupported scheme'), id='UnsupportedProtocol'), + pytest.param(impit.TooManyRedirects('too many redirects'), id='TooManyRedirects'), + pytest.param(impit.HTTPStatusError('status error'), id='HTTPStatusError'), + # Impit reports a bad URL outside the `impit.HTTPError` tree entirely. + pytest.param(impit.InvalidURL('unsupported scheme'), id='InvalidURL'), + # `InvalidResponseBodyError` is raised by a resource client once `call` has returned, never in the retry loop. + pytest.param(InvalidResponseBodyError(Mock()), id='InvalidResponseBodyError'), + pytest.param(ValueError('value error'), id='ValueError'), + pytest.param(RuntimeError('runtime error'), id='RuntimeError'), + pytest.param(Exception('generic exception'), id='Exception'), + ], +) +def test_is_not_retryable_error(exc: Exception) -> None: + """A transport failure a retry cannot fix, and anything outside Impit's hierarchy, is not retried.""" + assert not _is_retryable_error(exc) def test_permanent_transport_error_is_not_retried() -> None: """A transport error a retry cannot fix fails on the first attempt instead of burning the whole backoff.""" client = ImpitHttpClient(token='test_token', min_delay_between_retries=timedelta(0)) - request = Mock(side_effect=impit.UnsupportedProtocol('unsupported scheme')) + request = Mock(side_effect=impit.LocalProtocolError('invalid header value')) client._impit_client = Mock(request=request) - with pytest.raises(impit.UnsupportedProtocol): - client.call(method='GET', url='ftp://api.test.com/endpoint') + with pytest.raises(impit.LocalProtocolError): + client.call(method='GET', url='https://api.test.com/endpoint') request.assert_called_once() @@ -361,15 +365,28 @@ def test_permanent_transport_error_is_not_retried() -> None: async def test_permanent_transport_error_is_not_retried_async() -> None: """The async client applies the same policy, failing on the first attempt.""" client = ImpitHttpClientAsync(token='test_token', min_delay_between_retries=timedelta(0)) - request = AsyncMock(side_effect=impit.UnsupportedProtocol('unsupported scheme')) + request = AsyncMock(side_effect=impit.LocalProtocolError('invalid header value')) client._impit_async_client = Mock(request=request) - with pytest.raises(impit.UnsupportedProtocol): - await client.call(method='GET', url='ftp://api.test.com/endpoint') + with pytest.raises(impit.LocalProtocolError): + await client.call(method='GET', url='https://api.test.com/endpoint') request.assert_awaited_once() +def test_transient_transport_error_is_retried() -> None: + """A transient transport failure keeps being retried until the attempts run out.""" + client = ImpitHttpClient(token='test_token', max_retries=2, min_delay_between_retries=timedelta(0)) + request = Mock(side_effect=impit.TimeoutException('timeout')) + client._impit_client = Mock(request=request) + + with pytest.raises(impit.TimeoutException): + client.call(method='GET', url='https://api.test.com/endpoint') + + # `max_retries` attempts inside the backoff loop, plus the final one it makes after the last delay. + assert request.call_count == 3 + + @pytest.fixture( params=[ pytest.param((GzipHttpCompressor(), 'gzip', gzip.decompress), id='gzip'), diff --git a/tests/unit/test_utils.py b/tests/unit/test_utils.py index aff75fc0..e1c73bcf 100644 --- a/tests/unit/test_utils.py +++ b/tests/unit/test_utils.py @@ -11,14 +11,13 @@ from typing import TYPE_CHECKING, Any from unittest.mock import Mock -import impit import pytest from apify_client._models import WebhookCondition, WebhookCreate from apify_client._resource_clients._resource_client import ResourceClientBase from apify_client._utils.crypto import create_hmac_signature, create_storage_content_signature, encode_base62 from apify_client._utils.encoding import encode_key_value_store_record_value, encode_webhooks_to_base64 -from apify_client._utils.errors import catch_not_found_or_throw, is_retryable_error +from apify_client._utils.errors import catch_not_found_or_throw from apify_client._utils.http import ( is_compressible_content_type, response_to_dict, @@ -26,7 +25,7 @@ to_safe_id, ) from apify_client._utils.try_import import FailedImport, try_import -from apify_client.errors import ApifyApiError, InvalidResponseBodyError +from apify_client.errors import ApifyApiError if TYPE_CHECKING: from apify_client._typeddicts import WebhookRepresentationDict @@ -187,36 +186,6 @@ def test_encode_webhooks_to_base64_from_dicts() -> None: assert result == result_from_models -@pytest.mark.parametrize( - 'exc', - [ - InvalidResponseBodyError(impit.Response(status_code=200)), - impit.HTTPError('generic http error'), - impit.NetworkError('network error'), - impit.TimeoutException('timeout'), - impit.RemoteProtocolError('remote protocol error'), - impit.ReadError('read error'), - impit.ConnectError('connect error'), - impit.WriteError('write error'), - impit.DecodingError('decoding error'), - ], -) -def test__is_retryable_error(exc: Exception) -> None: - assert is_retryable_error(exc) is True - - -@pytest.mark.parametrize( - 'exc', - [ - Exception('generic exception'), - ValueError('value error'), - RuntimeError('runtime error'), - ], -) -def test__is_not_retryable_error(exc: Exception) -> None: - assert is_retryable_error(exc) is False - - @pytest.mark.parametrize( ('status_code', 'error_type', 'should_suppress'), [