fix(ext/fetch): retry on stale pooled HTTP/1.1 connections#32566
Merged
bartlomieju merged 3 commits intodenoland:mainfrom Mar 11, 2026
Merged
fix(ext/fetch): retry on stale pooled HTTP/1.1 connections#32566bartlomieju merged 3 commits intodenoland:mainfrom
bartlomieju merged 3 commits intodenoland:mainfrom
Conversation
When a server shuts down and restarts on the same port, the fetch client's connection pool may still hold a stale keep-alive connection. Reusing it causes "connection closed before message completed" errors. Extend `is_error_retryable()` to also retry on: - `hyper::Error::is_incomplete_message()` (connection closed mid-request) - `io::ErrorKind::ConnectionReset` (TCP RST on stale connection) These are safe to retry because the server never received the request. Closes denoland#31955 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Fix clippy collapsible_if lint errors - Handle ConnectionAborted (Windows os error 10053) in addition to ConnectionReset for cross-platform retry support - Fix errorMessageIncludesUrlAndDetailsWithTcpInfo unit test to accept connections in a loop so retries hit the same error Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
kajukitli
approved these changes
Mar 10, 2026
Contributor
kajukitli
left a comment
There was a problem hiding this comment.
lgtm, good fix for a frustrating issue
the retry logic is sound:
is_incomplete_message()catches the "connection closed before message completed" caseConnectionReset/ConnectionAbortedcovers TCP RST from stale connections- both are safe to retry since the server never processed the request
the existing Retried marker ensures we don't retry infinitely, and the body-clonability check prevents retrying streaming requests.
nice that you also fixed the unit test to accept connections in a loop — otherwise the retry would hit a closed listener and fail differently.
one thought: might be worth adding a brief comment in the test explaining why we loop on accept (so future readers understand it's for retry coverage, not just cleanup).
nathanwhit
approved these changes
Mar 11, 2026
kajukitli
approved these changes
Mar 18, 2026
Contributor
kajukitli
left a comment
There was a problem hiding this comment.
Reviewed the changes with full repo context. No issues found.
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
3 participants
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.
Summary
Fixes #31955
When a server shuts down and restarts on the same port, the fetch client's
connection pool may still hold a stale keep-alive connection. Reusing it causes
connection closed before message completederrors on every other request.The existing
is_error_retryable()only handled HTTP/2 GOAWAY andREFUSED_STREAM. This PR extends it to also retry on:
hyper::Error::is_incomplete_message()— connection closed before themessage completed (the main error from the issue)
io::ErrorKind::ConnectionReset— TCP RST from a stale pooled connectionBoth are safe to retry because the server never received/processed the request.
The retry is limited to one attempt (enforced by the existing
Retriedmarkerextension) and only applies to requests with clonable bodies (not streaming).
Note: despite being reported as a regression in 2.6.6, this issue reproduces all
the way back to Deno 1.46.0 — it was a long-standing bug, not a regression.