Skip to content

feat: Unify the request pipeline across HTTP clients - #1011

Draft
vdusek wants to merge 1 commit into
masterfrom
move-http-pipeline-to-base
Draft

feat: Unify the request pipeline across HTTP clients#1011
vdusek wants to merge 1 commit into
masterfrom
move-http-pipeline-to-base

Conversation

@vdusek

@vdusek vdusek commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

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: retry classification stays exactly as on master (every impit.HTTPError is transient). The classification fixes follow in #1012.

Split out of #1006.

✍️ Drafted by Claude Code

@vdusek vdusek 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 17, 2026
@vdusek vdusek self-assigned this Aug 17, 2026
@codecov

codecov Bot commented Aug 17, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 97.57576% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 94.91%. Comparing base (334e131) to head (a4ae1e6).

Files with missing lines Patch % Lines
src/apify_client/_streamed_log.py 75.00% 2 Missing ⚠️
src/apify_client/http_clients/_base.py 98.38% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1011      +/-   ##
==========================================
+ Coverage   94.87%   94.91%   +0.03%     
==========================================
  Files          58       58              
  Lines        5367     5385      +18     
==========================================
+ Hits         5092     5111      +19     
+ Misses        275      274       -1     
Flag Coverage Δ
integration 91.81% <72.72%> (-0.41%) ⬇️
unit 85.23% <97.57%> (+0.08%) ⬆️

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.

Base automatically changed from fix-http-response-protocol to master August 17, 2026 17:24
@vdusek
vdusek force-pushed the move-http-pipeline-to-base branch from e6f24c5 to 4d7112b Compare August 17, 2026 20:30
@github-actions github-actions Bot added this to the 147th sprint - Tooling team milestone Aug 17, 2026
@github-actions github-actions Bot added the tested Temporary label used only programatically for some analytics. label Aug 17, 2026
@vdusek vdusek changed the title fix: Unify the request pipeline across HTTP clients feat: Unify the request pipeline across HTTP clients Aug 18, 2026
@vdusek
vdusek force-pushed the move-http-pipeline-to-base branch from eaeb849 to a4ae1e6 Compare August 18, 2026 09:20
vdusek added a commit that referenced this pull request Aug 18, 2026
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*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

adhoc Ad-hoc unplanned task added during the sprint. t-tooling Issues with this label are in the ownership of the tooling team. tested Temporary label used only programatically for some analytics.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants