fix(h2): destroy aborted stream synchronously instead of via setImmediate - #5559
Merged
mcollina merged 1 commit intoJul 14, 2026
Merged
Conversation
Copy link
Copy Markdown
Contributor
Sorry, something went wrong.
…iate nodejs#5462 destroys the aborted HTTP/2 stream (rather than relying on the async 'close' event) to release the native Http2Stream deterministically, but it defers the destroy behind a setImmediate so the RST_STREAM frame queued by close() is written first. That reintroduces the same leak under a different trigger: while the event loop is stalled by a long synchronous task, the setImmediate callback never runs, so every stream aborted during the stall stays alive along with the request graph it pins. Under abort churn (timeouts, cancellations) — which tends to spike exactly when the loop is busy — this grows without bound until the process OOMs. Destroy the stream synchronously after close(). close() has already queued the RST_STREAM frame on the native session, so it is still sent, and teardown no longer depends on the event loop advancing. Tightens the regression test to assert the stream is destroyed synchronously on abort, so a future re-deferral fails here instead of in production. Refs: nodejs#5558 Signed-off-by: Scott Taylor <scott.c.taylor@mac.com> Assisted-By: devx/4833cb20-f964-4e22-abdf-50db58d074eb
Copy link
Copy Markdown
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## main #5559 +/- ##
==========================================
- Coverage 93.44% 93.44% -0.01%
==========================================
Files 110 110
Lines 37434 37434
==========================================
- Hits 34980 34979 -1
- Misses 2454 2455 +1 ☔ View full report in Codecov by Harness.
|
Sorry, something went wrong.
mcollina
approved these changes
Jul 14, 2026
mcollina
left a comment
Copy link
Copy Markdown
Member
There was a problem hiding this comment.
lgtm
Sorry, something went wrong.
staylor
added a commit
to staylor/undici
that referenced
this pull request
Jul 14, 2026
On a busy, long-lived multiplexed HTTP/2 session the native 'close' event can fail to fire. Since nodejs#5559 the abort path handles this, but normal completion still defers all cleanup (onResponseEnd, listener removal, kRequestStreamState reset, session open-stream decrement) to onRequestStreamClose, gated on 'close'. A completed-but-not-closed stream therefore pins its request graph and buffers for the session's life, leaking until OOM under sustained request volume. Extract that cleanup into completeRequestStream and also run it from onEnd, guarded by a null-state check so whichever of 'end'/'close' fires first wins and the other is a no-op. This completes the response properly (no premature-close of an in-flight body) rather than force-destroying the stream. Refs nodejs#5558. Assisted-By: devx/816ae76b-22d2-4002-873d-05d223e7eec2
staylor
added a commit
to staylor/undici
that referenced
this pull request
Jul 14, 2026
On a busy, long-lived multiplexed HTTP/2 session the native 'close' event can fail to fire. Since nodejs#5559 the abort path handles this, but normal completion still defers all cleanup (onResponseEnd, listener removal, kRequestStreamState reset, session open-stream decrement) to onRequestStreamClose, gated on 'close'. A completed-but-not-closed stream therefore pins its request graph and buffers for the session's life, leaking until OOM under sustained request volume. Extract that cleanup into completeRequestStream and also run it from onEnd, guarded by a null-state check so whichever of 'end'/'close' fires first wins and the other is a no-op. This completes the response properly (no premature-close of an in-flight body) rather than force-destroying the stream. Refs nodejs#5558. Assisted-By: devx/816ae76b-22d2-4002-873d-05d223e7eec2
staylor
added a commit
to staylor/undici
that referenced
this pull request
Jul 15, 2026
On a busy, long-lived multiplexed HTTP/2 session the native 'close' event can fail to fire. Since nodejs#5559 the abort path handles this, but normal completion still defers all cleanup (onResponseEnd, listener removal, kRequestStreamState reset, session open-stream decrement) to onRequestStreamClose, gated on 'close'. A completed-but-not-closed stream therefore pins its request graph and buffers for the session's life, leaking until OOM under sustained request volume. Extract that cleanup into completeRequestStream and also run it from onEnd, guarded by a null-state check so whichever of 'end'/'close' fires first wins and the other is a no-op. This completes the response properly (no premature-close of an in-flight body) rather than force-destroying the stream. Refs nodejs#5558. Assisted-By: devx/816ae76b-22d2-4002-873d-05d223e7eec2
staylor
added a commit
to staylor/undici
that referenced
this pull request
Jul 15, 2026
On a busy, long-lived multiplexed HTTP/2 session the native 'close' event can fail to fire. Since nodejs#5559 the abort path handles this, but normal completion still defers all cleanup (onResponseEnd, listener removal, kRequestStreamState reset, session open-stream decrement) to onRequestStreamClose, gated on 'close'. A completed-but-not-closed stream therefore pins its request graph and buffers for the session's life, leaking until OOM under sustained request volume. Extract that cleanup into completeRequestStream and also run it from onEnd, guarded by a null-state check so whichever of 'end'/'close' fires first wins and the other is a no-op. This completes the response properly (no premature-close of an in-flight body) rather than force-destroying the stream. Refs nodejs#5558. Assisted-By: devx/816ae76b-22d2-4002-873d-05d223e7eec2
Merged
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.
Follow-up to #5462.
#5462 fixed a leak where an aborted HTTP/2 request left teardown to the stream's async 'close' event, which can fail to fire on a busy, long-lived multiplexed session — pinning the native Http2Stream and the request graph it references. It did so by destroying the stream after close(), but deferred the destroy behind a setImmediate so the RST_STREAM frame queued by close() is written first.
That deferral reintroduces the same leak under a different trigger. setImmediate callbacks only run once the loop reaches the check phase, so while the event loop is stalled by a long synchronous task the deferred destroy never runs. Every stream aborted during the stall stays alive (plus the request graph each one pins), and abort churn — timeouts, cancellations — tends to spike precisely when the loop is busy. The result is an unbounded, positive-feedback leak until the process OOMs.
This was not hypothetical for us: a high-throughput fetch service (~100K req/s across the fleet) that periodically stalls its loop under bursty work saw memory roughly triple and the fleet enter a sustained OOMKilled crashloop immediately after taking stock 8.7.0 (which carries the setImmediate defer) in place of a local patch that destroyed synchronously. Details in #5558.
Change
Testing
test/http2-dispatcher.js (all 18) plus http2-abort, http2-goaway, http2-destroy-after-failed-stream, http2-response-after-completion, http2-timeout, and http2-stream pass locally. eslint clean.
If there is a concrete RST_STREAM-ordering scenario the setImmediate was guarding against that isn't covered by the current tests, I'm happy to add a case for it — but in practice close(code) submits the frame to the native handle synchronously, so a subsequent destroy() still delivers it.