fix: Fail fast on transport errors a retry cannot fix (#1019)

Every `impit.HTTPError` counted as transient, so a transport failure a
retry cannot fix burned the whole backoff before surfacing. The
permanent classes now fail on the first attempt instead:

- `LocalProtocolError` - a request Impit rejects before sending it, e.g.
one carrying an invalid header value.
- `TooManyRedirects` - a routing loop, which repeating the request
cannot break.
- `UnsupportedProtocol` - the class Impit declares for a scheme it
refuses to speak, but does not raise today; an unsupported scheme
arrives as `impit.InvalidURL`, outside the `impit.HTTPError` tree and
non-retryable already. Listed so the classifier stays right if Impit
switches over.
- `HTTPStatusError` - only `Response.raise_for_status()` raises it and
the client never calls it; `_make_request` decides on status codes from
the response itself.

Everything else in the `impit.HTTPError` tree stays retryable, including
a bare `HTTPError` - Impit wraps a failure its internal HTTP library did
not classify in one, e.g. a non-HTTP response or a connection reset -
and a `ProxyError`, which covers a rejected CONNECT tunnel and a 407
alike, so a transient case cannot be told from a permanent one.

Also drops `InvalidResponseBodyError` from the retry check. Only
`key_value_store.py` raises it, once `call` has already returned, so it
can never reach the classifier. Its docstring no longer claims the
client retries such requests. The duplicate `is_retryable_error` in
`_utils/errors.py`, which carried the old policy, is gone.

Split out of #1006 and re-targeted at `master`, so the retry fixes can
ship in a patch release ahead of the pipeline refactor in #1011.

*✍️ Drafted by Claude Code*

Author: vdusek

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:

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')

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(