Fix retry-budget exhaustion and correlated reused-connection death - #1354
Merged
Conversation
A client sending periodic bursts against a peer that silently discards idle pooled connections saw transient errors escape HTTP.request with default retry=true after ~50 burst cycles, permanently (#1353). Root cause: every armed high-level retry consumed 10 of the per-host RetryBucket's 500 tokens even when the retried attempt succeeded, the bucket never refilled, and once empty the denial was silent — so the budget was a fuse that burned out in ~50 recoveries, after which every transient failure surfaced raw. - RetryBucket: refund the reservation when a retried attempt reaches a non-retryable response (retryable 408/429/5xx responses keep partial cost, exception outcomes keep full cost), and credit 1 unit per successful non-retried request so a drained partition heals from healthy traffic. A depleted-partition counter keeps the per-request replenish check lock-free while all partitions are full. - Transport: retry a replayable idempotent request while failures keep landing on *reused* pooled connections (bounded by max_idle_per_host + 1 acquisitions) instead of exactly once, so a correlated-death batch is burned through down to a fresh dial without consuming high-level retry budget; classify TLSError on reused connections by its cause so dead reused TLS connections take this path too. - Add RetrySkippedEvent so a denied (:retry_bucket) or deadline- preempted (:deadline) retry is observable in request traces. - Add TLSTransportError for TLS I/O failures on established connections; handshake failures are now typed TLSHandshakeError at the dial sites instead of one blanket wrap that mislabeled mid-request read errors as handshake errors. isrecoverable unwraps both. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1354 +/- ##
==========================================
+ Coverage 88.56% 88.84% +0.28%
==========================================
Files 31 31
Lines 11974 12132 +158
==========================================
+ Hits 10605 10779 +174
+ Misses 1369 1353 -16 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Add tests for the handshake-phase TLSHandshakeError wrap on both the HTTP/1 transport dial and connect_h2!, the request-path (exception) RetrySkippedEvent emission, and the verbose trace formatting of RetrySkippedEvent. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Make retry-budget accounting exact across built-in and custom policies. Recover poisoned pooled connections without leaking slots or reusing a failed connection. Normalize established TLS failures across HTTP/1 and HTTP/2 public client boundaries. Add regression coverage for terminal accounting, trace failures, one-shot bodies, concurrent pool handoff, and truncated TLS records. Fixes #1353
Both retry classifiers matched Reseau's truncated-TLS-stream message with a duplicated string literal; hoist it to a single documented constant so a Reseau wording change is a one-line fix, and note that the end-to-end truncation tests pin the coupling. Also smooth the retry-budget CHANGELOG entry into readable prose. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #1353.
Root cause
The reported pattern — clean for ~50 burst cycles (~10 min), then 1–2
ParseError("unexpected EOF while reading HTTP/1 data")escapes per burst forever, despite defaultretry=true— is primarily a retry-budget accounting bug, with the issue's single-shot transport retry as the trigger:max_idle_per_host = 2default) die together between bursts (S3 discards them while parked). The unlucky request draws dead conn splitpath loses last character #1 and its single transport retry (attempt == 1gate) draws dead conn Basic Documentation #2.RetryBucket's 500 tokens, and the release call charged the full 10 even when the retried attempt succeeded with a 2xx. Nothing ever refilled the bucket.RetryDeniedErrorwas swallowed silently and the rawParseErrorrethrown, indistinguishable from "not retryable".Reproduced deterministically against a local TLS server (park 2 conns → kill both → 1 request, every 10 s): the bucket drains 10/cycle through 50 successful recoveries, then the exact reported error escapes every cycle, first escape at cycle 51.
Changes
Retry-budget accounting. A retry reservation is settled by the effective retry decision for the response it produced: refunded in full when the built-in policy (or a custom
retry_if) no longer wants a retry, partial cost (5) kept when the response is still classified as a failure. On the final attempt the built-in classification applies without invokingretry_if, and aretry_if-forced retry conservatively keeps cost on a non-2xx/3xx outcome. Each successful non-retried request restores one unit of previously consumed budget (AWS-style no-retry increment), tracked through a copy-on-write depleted-partition snapshot so healthy traffic never takes the bucket lock. Reservations and response connections are released even when a trace orretry_ifcallback throws, and the request deadline is rechecked after the backoff sleep.Transport recovery through a poisoned pool. The reused-connection retry loops while failures keep landing on reused connections: up to
max_idle_per_hostreused acquisitions, then the final attempt forces a fresh dial — an idle pool hit or concurrent waiter handoff on that attempt is closed and its counted slot transferred to the dial, somax_conns_per_hoststays exact. Slot release is now exactly-once via aslot_releasedflag (a connection closed early by a cancel callback no longer strands its slot), and a live connection drawn in the same pool pass that evicts stale peers is no longer dropped. PUT and DELETE get the same stale-connection recovery as the other idempotent methods (replayable-body gate unchanged). This makes correlated-death recovery free — no outer attempt, no bucket traffic, no backoff — and protectsretry=falsecallers and directroundtrip!users.TLS error classification and typing. Dead reused TLS connections are classified by cause (e.g. an RST as a wrapped
SystemError) and by Reseau's truncated-stream signature (a single documented constant; the coupling is pinned by end-to-end truncated-record tests). Handshake-phase failures are typedTLSHandshakeErrorat the dial sites (HTTP/1 and HTTP/2, includingTLS.clientsetup); TLS failures on established connections surface as the newTLSTransportErroracross all public boundaries —request,HTTP.open, streaming body reads,roundtrip!,h2_roundtrip!, and the HTTP/2 read-loop/window-update paths.isrecoverableunwraps both public wrappers plus HTTP/2ProtocolErrorconnection wrappers. Non-Exceptionthrows from user callbacks pass through unwrapped.Observability. New public
RetrySkippedEventtrace event fires when the policy wanted a retry but none was armed —reason = :retry_bucket(budget denied) or:deadline(deadline preempted the backoff). Previously a denied retry was indistinguishable from a non-retryable failure, which is what originally made this bug look like "retry=true doesn't retry".Validation
Issue-shaped end-to-end harness (local TLS server, both pooled conns killed every cycle, 60 cycles):
outer_retries = 0ParseErrorevery cycle from 51 onUnder close_notify, bare-FIN, and RST kill modes, recovery now happens entirely at the transport layer with zero bucket consumption. The poisoned-pool scenario is a regression test (red on master with the exact reported error), alongside coverage for terminal accounting,
retry_ifaccounting and suppression, trace/policy-callback failure cleanup (including pool-slot assertions), concurrent handoff underforce_fresh, one-shot bodies, truncated TLS records (HTTP/1 + HTTP/2), stale PUT/DELETE recovery, andRetrySkippedEventemission on both response and request paths.Behavior notes
TLSTransportErrorinstead ofTLSHandshakeError; callers matchingHTTPErroror usingisrecoverableare unaffected (CHANGELOG entry included).retry_ifis not invoked for the terminal response once retries are exhausted (matching the previous gate order) nor for unreplayable bodies.retry_bucket=false(or a large-capacityRetryBucket).🤖 Generated with Claude Code