Skip to content

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

Merged
vdusek merged 2 commits into
masterfrom
fix-permanent-transport-errors
Aug 18, 2026
Merged

fix: Fail fast on transport errors a retry cannot fix#1019
vdusek merged 2 commits into
masterfrom
fix-permanent-transport-errors

Conversation

@vdusek

@vdusekvdusek commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

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

@vdusekvdusek added adhoc Ad-hoc unplanned task added during the sprint. t-tooling Issues with this label are in the ownership of the tooling team. labels Aug 18, 2026
@vdusekvdusek self-assigned this Aug 18, 2026
@github-actionsgithub-actionsBot added the tested Temporary label used only programatically for some analytics. label Aug 18, 2026
@codecov

codecovBot commented Aug 18, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.85%. Comparing base (334e131) to head (fbc7216).

Additional details and impacted files
@@ Coverage Diff @@## master #1019 +/- ##
==========================================
- Coverage 94.87% 94.85% -0.03% 
==========================================
Files 58 58 Lines 5367 5365 -2 ==========================================
- Hits 5092 5089 -3 - Misses 275 276 +1 
FlagCoverage Δ
integration91.94% <75.00%> (-0.27%)⬇️
unit85.14% <100.00%> (-0.01%)⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@vdusek
vdusek requested a review from PijukatelAugust 18, 2026 13:52
@vdusek
vdusek marked this pull request as ready for review August 18, 2026 13:52
vdusek added a commit that referenced this pull request Aug 18, 2026
`_utils/errors.py::is_retryable_error` is a stale copy of
`http_clients/_impit.py::_is_retryable_error`. No production code calls
it — only its own tests did — and it sits in a private module, so
nothing depends on it. Once #1019 narrows the real policy, keeping a
second copy that still claims every `impit.HTTPError` is transient is
actively misleading.
Removes the function, its two test cases, and the imports they alone
needed.
Split out of #1006.
*✍️ Drafted by Claude Code*
@vdusek
vdusek merged commit ab63eff into masterAug 18, 2026
30 checks passed
@vdusek
vdusek deleted the fix-permanent-transport-errors branch August 18, 2026 14:03
vdusek added a commit that referenced this pull request Aug 19, 2026
Moves `call`, the retry loop, and the per-attempt request handling from
`ImpitHttpClient` and `ImpitHttpClientAsync` into `HttpClient` and
`HttpClientAsync`, leaving Impit as a thin adapter over the transport
hooks: `send_request`, `is_retryable_transport_error`,
`is_timeout_error`, and `close()` / `aclose()` plus context managers. A
custom client that overrides `call` keeps working unchanged and opts out
of the shared pipeline. `StreamedLog` classifies timeouts through the
transport-neutral `is_timeout_error` hook instead of importing Impit.
No behavior change for the built-in clients: the retry policy shipped in
#1019 (permanent transport errors fail fast) and #1020 (a failed read of
a streamed error body is classified like a failed send and the response
is closed) is preserved — the classification now lives in the
`is_retryable_transport_error` hook, and the error-body read handling in
the shared pipeline.
Supersedes #1011, rebased onto current master and squashed. Originally
split out of #1006.
*✍️ Drafted by Claude Code*
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

adhocAd-hoc unplanned task added during the sprint.t-toolingIssues with this label are in the ownership of the tooling team.testedTemporary label used only programatically for some analytics.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@vdusek@Pijukatel@apify-service-account