fix(ext/fetch): only retry transport errors on pooled connections - #36415
Conversation
Retrying `incomplete_message` and `ECONNRESET` unconditionally resends requests that the server may have already processed. Gate those retries on the connection having been reused, so a failure on a freshly established connection surfaces instead of being retried.
bartlomieju
left a comment
bartlomieju
left a comment
There was a problem hiding this comment.
The premise is right and the layering is clean — gating the two ambiguous transport errors on pool provenance, while leaving GOAWAY/REFUSED_STREAM unconditional, is the correct split. fetch_retry_stale_connection still covers the positive case and the new spec test can't pass spuriously (the serialized accept loop means a retry would deterministically show 2).
One substantive thing: the HTTP/2 comment describes the opposite of what the code does (see inline). A few smaller notes on test cleanup, an invisible failure mode in the tracker, and readability.
Correct the HTTP/2 comment to describe the actual trade-off of bumping the
usage counter at checkout, and record the multiplexing false positive as a
known limitation instead of claiming it is avoided.
Replace the bare `bool` threaded into `is_error_retryable` with a
`ConnectionKind::{Pooled, Fresh}` enum, relax the tracker's atomics to
`Relaxed`, skip provenance tracking for streaming bodies (which can never be
cloned and so never retried), and guard the tracker's passthrough with a
`debug_assert!` so a future entry point that forgets the extension fails loudly
rather than silently disabling every transport retry.
| mut captured: CaptureConnection, | ||
| reused: ConnectionReused, | ||
| ) { | ||
| let metadata = captured.wait_for_connection_metadata().await; |
There was a problem hiding this comment.
P2: Track the connection used by the final internal attempt. If the request first checks out a stale pooled connection, hyper-util can internally recover the unstarted request and resend it on a fresh connection before returning to tower. The capture extension replaces its metadata on every checkout, but this future stops after the first value, so the request remains classified as pooled. If the fresh internal attempt is then accepted and reset, tower retries it again and may duplicate a non-idempotent request. Please track the latest checkout through completion so the classification belongs to the attempt that produced the error.
One `tower` attempt can span several internal `hyper_util` ones: its client loops on `TrySendError::Retryable`, so a request that checks out a stale pooled connection is recovered unstarted and resent on a fresh one, replacing the capture slot. The recorder awaited `wait_for_connection_metadata` once and stopped, so the request stayed classified as `Pooled` even though the failing attempt ran on a fresh connection — and a reset there would let `FetchRetry` resend a non-idempotent request. `CaptureConnection` exposes no change signal (only a one-shot wait), so instead sample the slot after every poll of the request future — `hyper_util` performs the checkout while polling it, so every checkout is observed. `CheckoutTracker` dedupes by connection identity so each checkout is counted once no matter how often the future is polled.
1 participant
fetchretries onincomplete_messageandECONNRESETon the premise thatthe server never received the request. That only holds for stale pooled
connections. On a freshly established connection the same errors mean the
server accepted the request, possibly ran it, and then died, so the retry
delivers a non-idempotent request twice. Node sends it once.
Transport-level retries are now gated on the connection having come out of
the pool. Since hyper-util doesn't expose connection reuse, the connector
tags every connection it creates with a request counter, and a small service
inside the retry layer records, per attempt, whether that connection had
already served a request. The HTTP/2 GOAWAY and REFUSED_STREAM retries are
protocol-level guarantees and stay unconditional.
The counter is bumped at checkout rather than on response completion, so a
request that never gets a connection is not misread as pooled. On HTTP/2 that
leaves a known false positive: the counter is per connection rather than per
stream, so a request multiplexed onto a connection a concurrent sibling
established a moment earlier reads as pooled and stays retryable even though
that connection was never in the pool. Most h2 transport failures are decided
by the
h2::Errorbranch before provenance is consulted, so the window isnarrow. Closing it properly needs a checkout-time snapshot distinguishing
"created for me" from "created for a concurrent sibling", which the current
design can't express.
Fixes #35610