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 68353b1d..8334da4c 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,31 @@ logger = logging.getLogger(logger_name) +_PERMANENT_ERRORS = ( + # 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, + # 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, +) + + 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` 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, - ( - 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..213bb38d 100644 --- a/tests/unit/test_http_clients.py +++ b/tests/unit/test_http_clients.py @@ -309,18 +309,82 @@ def test_parse_params_mixed() -> None: } -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 - 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', + [ + # 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'), + ], +) +def test_is_retryable_error(exc: Exception) -> None: + """A transient transport failure is retried.""" + assert _is_retryable_error(exc) + + +@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.LocalProtocolError('invalid header value')) + client._impit_client = Mock(request=request) + + with pytest.raises(impit.LocalProtocolError): + client.call(method='GET', url='https://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.LocalProtocolError('invalid header value')) + client._impit_async_client = Mock(request=request) + + 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( 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'), [