Skip to content

fix(h2): destroy aborted stream synchronously instead of via setImmediate#5559

Merged
mcollina merged 1 commit into
nodejs:mainfrom
staylor:fix/h2-abort-destroy-sync
Jul 14, 2026
Merged

fix(h2): destroy aborted stream synchronously instead of via setImmediate#5559
mcollina merged 1 commit into
nodejs:mainfrom
staylor:fix/h2-abort-destroy-sync

Conversation

@staylor

@staylor staylor commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

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

  • Destroy the stream synchronously after close() in the writeH2 abort path. close() has already queued the RST_STREAM frame on the native session, so it is still sent; teardown no longer depends on the event loop advancing.
  • Tighten the 'Should destroy the h2 stream on abort' regression test to assert the stream is destroyed synchronously on abort (drops the waitFor poll), so a future re-deferral fails in CI instead of in production.

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.

…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
@codecov-commenter

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.44%. Comparing base (a360781) to head (2c1dca1).

Additional details and impacted files
@@            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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@mcollina mcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@mcollina
mcollina merged commit 6010668 into nodejs:main Jul 14, 2026
36 checks passed
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
@github-actions github-actions Bot mentioned this pull request Jul 20, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants