fix(node): do not retry cancelled HTTP requests - #36801
Open
boyam01 wants to merge 2 commits into
Open
Conversation
Respect ClientRequest.destroyed before retrying reused keep-alive sockets so cancellation cannot replay a POST or swallow its original error. Cover destroy(), abort(), AbortSignal, and timeout cancellation over HTTP and HTTPS, with fresh-socket controls. Fixes denoland#36786 AI assistance: OpenAI Codex.
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 #36786.
canRetryRequest()currently accepts a request that the caller has already destroyed. Socket close/error/end handling can then entermaybeRetryRequest(), resetdestroyed, and resend the buffered POST on a new connection.Exclude destroyed requests in the shared retry predicate. This also covers
abort(), AbortSignal cancellation, and timeout callbacks that calldestroy(), while leaving retries for genuinely stale pooled connections intact.The regression waits until the server has received the complete POST and asserts that the socket was actually reused before cancelling. It then checks that the body was received only once, no response was delivered, and the cancellation error was preserved. There are 10 cancellation cases across HTTP/HTTPS plus 2 fresh-socket controls, using the existing TLS fixtures and spec harness.
Testing:
specs::node::http_cases pass, including the 12 new cases and existing stale-socket retry, body preservation, streaming, and permission tests.unit_node::httppass through the official harness.The patched runtime has not been built/tested locally on Windows or macOS; the full compatibility suite has not been run locally.
AI disclosure: OpenAI Codex assisted with the implementation, tests, review, and this description. The results above come from executed local checks.