fix(h2): release completed requests from queue by staylor · Pull Request #5569 · nodejs/undici

staylor · 2026-07-17T13:39:04Z

This relates to...

Rationale

The HTTP/2 in-order completion optimization introduced in #5483 advanced kRunningIdx without clearing the completed request's queue slot. Until the periodic queue compaction ran, each dead prefix slot retained its Request, whose controller retained the ClientHttp2Stream and the rest of the fetch graph. Under sustained HTTP/2 abort churn this retained native response buffers and caused memory growth. A 20,000-request reproduction at 50% aborts grew the heap by 10.6 MB before this change and 2.8 MB after it, matching the pre-#5483 baseline.

A stream can also close after its request has already been marked aborted. Terminal stream cleanup must still finalize that request even though onResponseEnd must not run.

Changes

Features

N/A

Bug Fixes

Clear an in-order completed HTTP/2 request's queue slot before advancing kRunningIdx, preserving the O(1) fast path and matching the existing HTTP/1 queue behavior.
Finalize terminal HTTP/2 streams independently of whether the response ended successfully.
Add regression coverage for an already-aborted request closing without response completion.

Breaking Changes and Deprecations

N/A

Status

The HTTP/2 in-order completion fast path advanced the running index but retained the completed Request in its queue slot until batched compaction. Under sustained abort churn, each retained Request pinned its stream and fetch graph, including native response buffers. Clear the completed slot before advancing, preserving the O(1) fast path, and finalize closed streams even when the Request was already marked aborted. Fixes: nodejs#5566 Signed-off-by: Scott Taylor <scott.c.taylor@mac.com> Assisted-By: devx/4ad398e1-8641-4e85-a62d-cb78992a7aae

codecov-commenter · 2026-07-17T13:56:28Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.47%. Comparing base (87270e4) to head (7ffd5c3).
⚠️ Report is 3 commits behind head on main.

Additional details and impacted files

@@ Coverage Diff @@## main #5569 +/- ##
=======================================
Coverage 93.47% 93.47% =======================================
Files 110 110 Lines 37507 37561 +54 =======================================
+ Hits 35059 35111 +52 - Misses 2448 2450 +2

☔ 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

lgtm

mcollina · 2026-07-18T18:27:49Z

I'm relatively concerned by this PR (and previous ones) that do not actually provide e2e tests.

Signed-off-by: Scott Taylor <scott.c.taylor@mac.com> Assisted-By: devx/7d65db46-a09e-471a-9e1c-abdaa5accba8

staylor · 2026-07-19T03:36:56Z

Thanks @mcollina — agreed. I’ve added an end-to-end regression test in 84ab33d. It uses a real TLS HTTP/2 server through the public fetch/Client APIs, aborts an in-flight response while keeping the session open and usable, and verifies that the aborted fetch graph is released. The test fails against the parent commit and passes with this fix.

metcoder95

small comment; rest lgtm

mcollina

lgtm

Co-authored-by: Carlos Fuentes <me@metcoder.dev>

Signed-off-by: Matteo Collina <hello@matteocollina.com>

mcollina

lgtm

…#5618) Backport of #5410 (and the fast path from #5569). HTTP/2 completes out of order, but every completion site advanced the running index blindly: client[kQueue][client[kRunningIdx]++] = null so whichever request happened to sit at the head was retired instead of the one that actually finished. With two streams in flight, completing the second one clears the first's slot while the second stays in the running window for good: after /second queue=[null, "/second"] runningIdx=1 pendingIdx=2 after /third queue=[null, null, "/third"] runningIdx=2 pendingIdx=3 The still-running /first is gone from the queue and two finished requests are counted as running forever, so kRunning never returns to zero. Since _resume() stops dispatching once kRunning reaches the concurrency limit, a client accumulating these eventually stops sending anything. Port completeRequest() from main: retire the request by identity, keeping the O(1) in-order fast path, and splice it out when it finished out of order. Cleared slots can now appear in the queue, so the paths that walk it skip them, as they do on main. Refs: #5404 Refs: #5410 Refs: #5569 Claude-Session: https://claude.ai/code/session_01A49JamgF2TkZHu5h58ChUM Signed-off-by: Matteo Collina <hello@matteocollina.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>

staylor · 2026-07-17T13:39:04Z

This relates to...

Fixes#5566.

Rationale

The HTTP/2 in-order completion optimization introduced in #5483 advanced kRunningIdx without clearing the completed request's queue slot. Until the periodic queue compaction ran, each dead prefix slot retained its Request, whose controller retained the ClientHttp2Stream and the rest of the fetch graph. Under sustained HTTP/2 abort churn this retained native response buffers and caused memory growth. A 20,000-request reproduction at 50% aborts grew the heap by 10.6 MB before this change and 2.8 MB after it, matching the pre-#5483 baseline.

A stream can also close after its request has already been marked aborted. Terminal stream cleanup must still finalize that request even though onResponseEnd must not run.

Changes

Features

N/A

Bug Fixes

Clear an in-order completed HTTP/2 request's queue slot before advancing kRunningIdx, preserving the O(1) fast path and matching the existing HTTP/1 queue behavior.
Finalize terminal HTTP/2 streams independently of whether the response ended successfully.
Add regression coverage for an already-aborted request closing without response completion.

Breaking Changes and Deprecations

N/A

Status

The HTTP/2 in-order completion fast path advanced the running index but retained the completed Request in its queue slot until batched compaction. Under sustained abort churn, each retained Request pinned its stream and fetch graph, including native response buffers. Clear the completed slot before advancing, preserving the O(1) fast path, and finalize closed streams even when the Request was already marked aborted. Fixes: nodejs#5566 Signed-off-by: Scott Taylor <scott.c.taylor@mac.com> Assisted-By: devx/4ad398e1-8641-4e85-a62d-cb78992a7aae

codecov-commenter · 2026-07-17T13:56:28Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.47%. Comparing base (87270e4) to head (7ffd5c3).
⚠️ Report is 3 commits behind head on main.

Additional details and impacted files

@@ Coverage Diff @@## main #5569 +/- ##
=======================================
Coverage 93.47% 93.47% =======================================
Files 110 110 Lines 37507 37561 +54 =======================================
+ Hits 35059 35111 +52 - Misses 2448 2450 +2

☔ 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

lgtm

mcollina · 2026-07-18T18:27:49Z

I'm relatively concerned by this PR (and previous ones) that do not actually provide e2e tests.

Signed-off-by: Scott Taylor <scott.c.taylor@mac.com> Assisted-By: devx/7d65db46-a09e-471a-9e1c-abdaa5accba8

staylor · 2026-07-19T03:36:56Z

Thanks @mcollina — agreed. I’ve added an end-to-end regression test in 84ab33d. It uses a real TLS HTTP/2 server through the public fetch/Client APIs, aborts an in-flight response while keeping the session open and usable, and verifies that the aborted fetch graph is released. The test fails against the parent commit and passes with this fix.

metcoder95

small comment; rest lgtm

mcollina

lgtm

Co-authored-by: Carlos Fuentes <me@metcoder.dev>

Signed-off-by: Matteo Collina <hello@matteocollina.com>

mcollina

lgtm

…#5618) Backport of #5410 (and the fast path from #5569). HTTP/2 completes out of order, but every completion site advanced the running index blindly: client[kQueue][client[kRunningIdx]++] = null so whichever request happened to sit at the head was retired instead of the one that actually finished. With two streams in flight, completing the second one clears the first's slot while the second stays in the running window for good: after /second queue=[null, "/second"] runningIdx=1 pendingIdx=2 after /third queue=[null, null, "/third"] runningIdx=2 pendingIdx=3 The still-running /first is gone from the queue and two finished requests are counted as running forever, so kRunning never returns to zero. Since _resume() stops dispatching once kRunning reaches the concurrency limit, a client accumulating these eventually stops sending anything. Port completeRequest() from main: retire the request by identity, keeping the O(1) in-order fast path, and splice it out when it finished out of order. Cleared slots can now appear in the queue, so the paths that walk it skip them, as they do on main. Refs: #5404 Refs: #5410 Refs: #5569 Claude-Session: https://claude.ai/code/session_01A49JamgF2TkZHu5h58ChUM Signed-off-by: Matteo Collina <hello@matteocollina.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>

staylor · 2026-07-17T13:39:04Z

This relates to...

Fixes#5566.

Rationale

The HTTP/2 in-order completion optimization introduced in #5483 advanced kRunningIdx without clearing the completed request's queue slot. Until the periodic queue compaction ran, each dead prefix slot retained its Request, whose controller retained the ClientHttp2Stream and the rest of the fetch graph. Under sustained HTTP/2 abort churn this retained native response buffers and caused memory growth. A 20,000-request reproduction at 50% aborts grew the heap by 10.6 MB before this change and 2.8 MB after it, matching the pre-#5483 baseline.

A stream can also close after its request has already been marked aborted. Terminal stream cleanup must still finalize that request even though onResponseEnd must not run.

Changes

Features

N/A

Bug Fixes

Clear an in-order completed HTTP/2 request's queue slot before advancing kRunningIdx, preserving the O(1) fast path and matching the existing HTTP/1 queue behavior.
Finalize terminal HTTP/2 streams independently of whether the response ended successfully.
Add regression coverage for an already-aborted request closing without response completion.

Breaking Changes and Deprecations

N/A

Status

The HTTP/2 in-order completion fast path advanced the running index but retained the completed Request in its queue slot until batched compaction. Under sustained abort churn, each retained Request pinned its stream and fetch graph, including native response buffers. Clear the completed slot before advancing, preserving the O(1) fast path, and finalize closed streams even when the Request was already marked aborted. Fixes: nodejs#5566 Signed-off-by: Scott Taylor <scott.c.taylor@mac.com> Assisted-By: devx/4ad398e1-8641-4e85-a62d-cb78992a7aae

codecov-commenter · 2026-07-17T13:56:28Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.47%. Comparing base (87270e4) to head (7ffd5c3).
⚠️ Report is 3 commits behind head on main.

Additional details and impacted files

@@ Coverage Diff @@## main #5569 +/- ##
=======================================
Coverage 93.47% 93.47% =======================================
Files 110 110 Lines 37507 37561 +54 =======================================
+ Hits 35059 35111 +52 - Misses 2448 2450 +2

☔ 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

lgtm

mcollina · 2026-07-18T18:27:49Z

I'm relatively concerned by this PR (and previous ones) that do not actually provide e2e tests.

Signed-off-by: Scott Taylor <scott.c.taylor@mac.com> Assisted-By: devx/7d65db46-a09e-471a-9e1c-abdaa5accba8

staylor · 2026-07-19T03:36:56Z

Thanks @mcollina — agreed. I’ve added an end-to-end regression test in 84ab33d. It uses a real TLS HTTP/2 server through the public fetch/Client APIs, aborts an in-flight response while keeping the session open and usable, and verifies that the aborted fetch graph is released. The test fails against the parent commit and passes with this fix.

metcoder95

small comment; rest lgtm

mcollina

lgtm

Co-authored-by: Carlos Fuentes <me@metcoder.dev>

Signed-off-by: Matteo Collina <hello@matteocollina.com>

mcollina

lgtm

…#5618) Backport of #5410 (and the fast path from #5569). HTTP/2 completes out of order, but every completion site advanced the running index blindly: client[kQueue][client[kRunningIdx]++] = null so whichever request happened to sit at the head was retired instead of the one that actually finished. With two streams in flight, completing the second one clears the first's slot while the second stays in the running window for good: after /second queue=[null, "/second"] runningIdx=1 pendingIdx=2 after /third queue=[null, null, "/third"] runningIdx=2 pendingIdx=3 The still-running /first is gone from the queue and two finished requests are counted as running forever, so kRunning never returns to zero. Since _resume() stops dispatching once kRunning reaches the concurrency limit, a client accumulating these eventually stops sending anything. Port completeRequest() from main: retire the request by identity, keeping the O(1) in-order fast path, and splice it out when it finished out of order. Cleared slots can now appear in the queue, so the paths that walk it skip them, as they do on main. Refs: #5404 Refs: #5410 Refs: #5569 Claude-Session: https://claude.ai/code/session_01A49JamgF2TkZHu5h58ChUM Signed-off-by: Matteo Collina <hello@matteocollina.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>

staylor · 2026-07-17T13:39:04Z

This relates to...

Fixes#5566.

Rationale

The HTTP/2 in-order completion optimization introduced in #5483 advanced kRunningIdx without clearing the completed request's queue slot. Until the periodic queue compaction ran, each dead prefix slot retained its Request, whose controller retained the ClientHttp2Stream and the rest of the fetch graph. Under sustained HTTP/2 abort churn this retained native response buffers and caused memory growth. A 20,000-request reproduction at 50% aborts grew the heap by 10.6 MB before this change and 2.8 MB after it, matching the pre-#5483 baseline.

A stream can also close after its request has already been marked aborted. Terminal stream cleanup must still finalize that request even though onResponseEnd must not run.

Changes

Features

N/A

Bug Fixes

Clear an in-order completed HTTP/2 request's queue slot before advancing kRunningIdx, preserving the O(1) fast path and matching the existing HTTP/1 queue behavior.
Finalize terminal HTTP/2 streams independently of whether the response ended successfully.
Add regression coverage for an already-aborted request closing without response completion.

Breaking Changes and Deprecations

N/A

Status

The HTTP/2 in-order completion fast path advanced the running index but retained the completed Request in its queue slot until batched compaction. Under sustained abort churn, each retained Request pinned its stream and fetch graph, including native response buffers. Clear the completed slot before advancing, preserving the O(1) fast path, and finalize closed streams even when the Request was already marked aborted. Fixes: nodejs#5566 Signed-off-by: Scott Taylor <scott.c.taylor@mac.com> Assisted-By: devx/4ad398e1-8641-4e85-a62d-cb78992a7aae

codecov-commenter · 2026-07-17T13:56:28Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.47%. Comparing base (87270e4) to head (7ffd5c3).
⚠️ Report is 3 commits behind head on main.

Additional details and impacted files

@@ Coverage Diff @@## main #5569 +/- ##
=======================================
Coverage 93.47% 93.47% =======================================
Files 110 110 Lines 37507 37561 +54 =======================================
+ Hits 35059 35111 +52 - Misses 2448 2450 +2

☔ 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

lgtm

mcollina · 2026-07-18T18:27:49Z

I'm relatively concerned by this PR (and previous ones) that do not actually provide e2e tests.

Signed-off-by: Scott Taylor <scott.c.taylor@mac.com> Assisted-By: devx/7d65db46-a09e-471a-9e1c-abdaa5accba8

staylor · 2026-07-19T03:36:56Z

Thanks @mcollina — agreed. I’ve added an end-to-end regression test in 84ab33d. It uses a real TLS HTTP/2 server through the public fetch/Client APIs, aborts an in-flight response while keeping the session open and usable, and verifies that the aborted fetch graph is released. The test fails against the parent commit and passes with this fix.

metcoder95

small comment; rest lgtm

mcollina

lgtm

Co-authored-by: Carlos Fuentes <me@metcoder.dev>

Signed-off-by: Matteo Collina <hello@matteocollina.com>

mcollina

lgtm

…#5618) Backport of #5410 (and the fast path from #5569). HTTP/2 completes out of order, but every completion site advanced the running index blindly: client[kQueue][client[kRunningIdx]++] = null so whichever request happened to sit at the head was retired instead of the one that actually finished. With two streams in flight, completing the second one clears the first's slot while the second stays in the running window for good: after /second queue=[null, "/second"] runningIdx=1 pendingIdx=2 after /third queue=[null, null, "/third"] runningIdx=2 pendingIdx=3 The still-running /first is gone from the queue and two finished requests are counted as running forever, so kRunning never returns to zero. Since _resume() stops dispatching once kRunning reaches the concurrency limit, a client accumulating these eventually stops sending anything. Port completeRequest() from main: retire the request by identity, keeping the O(1) in-order fast path, and splice it out when it finished out of order. Cleared slots can now appear in the queue, so the paths that walk it skip them, as they do on main. Refs: #5404 Refs: #5410 Refs: #5569 Claude-Session: https://claude.ai/code/session_01A49JamgF2TkZHu5h58ChUM Signed-off-by: Matteo Collina <hello@matteocollina.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>

staylor · 2026-07-17T13:39:04Z

This relates to...

Fixes#5566.

Rationale

The HTTP/2 in-order completion optimization introduced in #5483 advanced kRunningIdx without clearing the completed request's queue slot. Until the periodic queue compaction ran, each dead prefix slot retained its Request, whose controller retained the ClientHttp2Stream and the rest of the fetch graph. Under sustained HTTP/2 abort churn this retained native response buffers and caused memory growth. A 20,000-request reproduction at 50% aborts grew the heap by 10.6 MB before this change and 2.8 MB after it, matching the pre-#5483 baseline.

A stream can also close after its request has already been marked aborted. Terminal stream cleanup must still finalize that request even though onResponseEnd must not run.

Changes

Features

N/A

Bug Fixes

Clear an in-order completed HTTP/2 request's queue slot before advancing kRunningIdx, preserving the O(1) fast path and matching the existing HTTP/1 queue behavior.
Finalize terminal HTTP/2 streams independently of whether the response ended successfully.
Add regression coverage for an already-aborted request closing without response completion.

Breaking Changes and Deprecations

N/A

Status

The HTTP/2 in-order completion fast path advanced the running index but retained the completed Request in its queue slot until batched compaction. Under sustained abort churn, each retained Request pinned its stream and fetch graph, including native response buffers. Clear the completed slot before advancing, preserving the O(1) fast path, and finalize closed streams even when the Request was already marked aborted. Fixes: nodejs#5566 Signed-off-by: Scott Taylor <scott.c.taylor@mac.com> Assisted-By: devx/4ad398e1-8641-4e85-a62d-cb78992a7aae

codecov-commenter · 2026-07-17T13:56:28Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.47%. Comparing base (87270e4) to head (7ffd5c3).
⚠️ Report is 3 commits behind head on main.

Additional details and impacted files

@@ Coverage Diff @@## main #5569 +/- ##
=======================================
Coverage 93.47% 93.47% =======================================
Files 110 110 Lines 37507 37561 +54 =======================================
+ Hits 35059 35111 +52 - Misses 2448 2450 +2

☔ 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

lgtm

mcollina · 2026-07-18T18:27:49Z

I'm relatively concerned by this PR (and previous ones) that do not actually provide e2e tests.

Signed-off-by: Scott Taylor <scott.c.taylor@mac.com> Assisted-By: devx/7d65db46-a09e-471a-9e1c-abdaa5accba8

staylor · 2026-07-19T03:36:56Z

Thanks @mcollina — agreed. I’ve added an end-to-end regression test in 84ab33d. It uses a real TLS HTTP/2 server through the public fetch/Client APIs, aborts an in-flight response while keeping the session open and usable, and verifies that the aborted fetch graph is released. The test fails against the parent commit and passes with this fix.

metcoder95

small comment; rest lgtm

mcollina

lgtm

Co-authored-by: Carlos Fuentes <me@metcoder.dev>

Signed-off-by: Matteo Collina <hello@matteocollina.com>

mcollina

lgtm

…#5618) Backport of #5410 (and the fast path from #5569). HTTP/2 completes out of order, but every completion site advanced the running index blindly: client[kQueue][client[kRunningIdx]++] = null so whichever request happened to sit at the head was retired instead of the one that actually finished. With two streams in flight, completing the second one clears the first's slot while the second stays in the running window for good: after /second queue=[null, "/second"] runningIdx=1 pendingIdx=2 after /third queue=[null, null, "/third"] runningIdx=2 pendingIdx=3 The still-running /first is gone from the queue and two finished requests are counted as running forever, so kRunning never returns to zero. Since _resume() stops dispatching once kRunning reaches the concurrency limit, a client accumulating these eventually stops sending anything. Port completeRequest() from main: retire the request by identity, keeping the O(1) in-order fast path, and splice it out when it finished out of order. Cleared slots can now appear in the queue, so the paths that walk it skip them, as they do on main. Refs: #5404 Refs: #5410 Refs: #5569 Claude-Session: https://claude.ai/code/session_01A49JamgF2TkZHu5h58ChUM Signed-off-by: Matteo Collina <hello@matteocollina.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>

staylor · 2026-07-17T13:39:04Z

This relates to...

Fixes#5566.

Rationale

The HTTP/2 in-order completion optimization introduced in #5483 advanced kRunningIdx without clearing the completed request's queue slot. Until the periodic queue compaction ran, each dead prefix slot retained its Request, whose controller retained the ClientHttp2Stream and the rest of the fetch graph. Under sustained HTTP/2 abort churn this retained native response buffers and caused memory growth. A 20,000-request reproduction at 50% aborts grew the heap by 10.6 MB before this change and 2.8 MB after it, matching the pre-#5483 baseline.

A stream can also close after its request has already been marked aborted. Terminal stream cleanup must still finalize that request even though onResponseEnd must not run.

Changes

Features

N/A

Bug Fixes

Clear an in-order completed HTTP/2 request's queue slot before advancing kRunningIdx, preserving the O(1) fast path and matching the existing HTTP/1 queue behavior.
Finalize terminal HTTP/2 streams independently of whether the response ended successfully.
Add regression coverage for an already-aborted request closing without response completion.

Breaking Changes and Deprecations

N/A

Status

The HTTP/2 in-order completion fast path advanced the running index but retained the completed Request in its queue slot until batched compaction. Under sustained abort churn, each retained Request pinned its stream and fetch graph, including native response buffers. Clear the completed slot before advancing, preserving the O(1) fast path, and finalize closed streams even when the Request was already marked aborted. Fixes: nodejs#5566 Signed-off-by: Scott Taylor <scott.c.taylor@mac.com> Assisted-By: devx/4ad398e1-8641-4e85-a62d-cb78992a7aae

codecov-commenter · 2026-07-17T13:56:28Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.47%. Comparing base (87270e4) to head (7ffd5c3).
⚠️ Report is 3 commits behind head on main.

Additional details and impacted files

@@ Coverage Diff @@## main #5569 +/- ##
=======================================
Coverage 93.47% 93.47% =======================================
Files 110 110 Lines 37507 37561 +54 =======================================
+ Hits 35059 35111 +52 - Misses 2448 2450 +2

☔ 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

lgtm

mcollina · 2026-07-18T18:27:49Z

I'm relatively concerned by this PR (and previous ones) that do not actually provide e2e tests.

Signed-off-by: Scott Taylor <scott.c.taylor@mac.com> Assisted-By: devx/7d65db46-a09e-471a-9e1c-abdaa5accba8

staylor · 2026-07-19T03:36:56Z

Thanks @mcollina — agreed. I’ve added an end-to-end regression test in 84ab33d. It uses a real TLS HTTP/2 server through the public fetch/Client APIs, aborts an in-flight response while keeping the session open and usable, and verifies that the aborted fetch graph is released. The test fails against the parent commit and passes with this fix.

metcoder95

small comment; rest lgtm

mcollina

lgtm

Co-authored-by: Carlos Fuentes <me@metcoder.dev>

Signed-off-by: Matteo Collina <hello@matteocollina.com>

mcollina

lgtm

…#5618) Backport of #5410 (and the fast path from #5569). HTTP/2 completes out of order, but every completion site advanced the running index blindly: client[kQueue][client[kRunningIdx]++] = null so whichever request happened to sit at the head was retired instead of the one that actually finished. With two streams in flight, completing the second one clears the first's slot while the second stays in the running window for good: after /second queue=[null, "/second"] runningIdx=1 pendingIdx=2 after /third queue=[null, null, "/third"] runningIdx=2 pendingIdx=3 The still-running /first is gone from the queue and two finished requests are counted as running forever, so kRunning never returns to zero. Since _resume() stops dispatching once kRunning reaches the concurrency limit, a client accumulating these eventually stops sending anything. Port completeRequest() from main: retire the request by identity, keeping the O(1) in-order fast path, and splice it out when it finished out of order. Cleared slots can now appear in the queue, so the paths that walk it skip them, as they do on main. Refs: #5404 Refs: #5410 Refs: #5569 Claude-Session: https://claude.ai/code/session_01A49JamgF2TkZHu5h58ChUM Signed-off-by: Matteo Collina <hello@matteocollina.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>

staylor · 2026-07-17T13:39:04Z

This relates to...

Fixes#5566.

Rationale

The HTTP/2 in-order completion optimization introduced in #5483 advanced kRunningIdx without clearing the completed request's queue slot. Until the periodic queue compaction ran, each dead prefix slot retained its Request, whose controller retained the ClientHttp2Stream and the rest of the fetch graph. Under sustained HTTP/2 abort churn this retained native response buffers and caused memory growth. A 20,000-request reproduction at 50% aborts grew the heap by 10.6 MB before this change and 2.8 MB after it, matching the pre-#5483 baseline.

A stream can also close after its request has already been marked aborted. Terminal stream cleanup must still finalize that request even though onResponseEnd must not run.

Changes

Features

N/A

Bug Fixes

Clear an in-order completed HTTP/2 request's queue slot before advancing kRunningIdx, preserving the O(1) fast path and matching the existing HTTP/1 queue behavior.
Finalize terminal HTTP/2 streams independently of whether the response ended successfully.
Add regression coverage for an already-aborted request closing without response completion.

Breaking Changes and Deprecations

N/A

Status

The HTTP/2 in-order completion fast path advanced the running index but retained the completed Request in its queue slot until batched compaction. Under sustained abort churn, each retained Request pinned its stream and fetch graph, including native response buffers. Clear the completed slot before advancing, preserving the O(1) fast path, and finalize closed streams even when the Request was already marked aborted. Fixes: nodejs#5566 Signed-off-by: Scott Taylor <scott.c.taylor@mac.com> Assisted-By: devx/4ad398e1-8641-4e85-a62d-cb78992a7aae

codecov-commenter · 2026-07-17T13:56:28Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.47%. Comparing base (87270e4) to head (7ffd5c3).
⚠️ Report is 3 commits behind head on main.

Additional details and impacted files

@@ Coverage Diff @@## main #5569 +/- ##
=======================================
Coverage 93.47% 93.47% =======================================
Files 110 110 Lines 37507 37561 +54 =======================================
+ Hits 35059 35111 +52 - Misses 2448 2450 +2

☔ 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

lgtm

mcollina · 2026-07-18T18:27:49Z

I'm relatively concerned by this PR (and previous ones) that do not actually provide e2e tests.

Signed-off-by: Scott Taylor <scott.c.taylor@mac.com> Assisted-By: devx/7d65db46-a09e-471a-9e1c-abdaa5accba8

staylor · 2026-07-19T03:36:56Z

Thanks @mcollina — agreed. I’ve added an end-to-end regression test in 84ab33d. It uses a real TLS HTTP/2 server through the public fetch/Client APIs, aborts an in-flight response while keeping the session open and usable, and verifies that the aborted fetch graph is released. The test fails against the parent commit and passes with this fix.

metcoder95

small comment; rest lgtm

mcollina

lgtm

Co-authored-by: Carlos Fuentes <me@metcoder.dev>

Signed-off-by: Matteo Collina <hello@matteocollina.com>

mcollina

lgtm

…#5618) Backport of #5410 (and the fast path from #5569). HTTP/2 completes out of order, but every completion site advanced the running index blindly: client[kQueue][client[kRunningIdx]++] = null so whichever request happened to sit at the head was retired instead of the one that actually finished. With two streams in flight, completing the second one clears the first's slot while the second stays in the running window for good: after /second queue=[null, "/second"] runningIdx=1 pendingIdx=2 after /third queue=[null, null, "/third"] runningIdx=2 pendingIdx=3 The still-running /first is gone from the queue and two finished requests are counted as running forever, so kRunning never returns to zero. Since _resume() stops dispatching once kRunning reaches the concurrency limit, a client accumulating these eventually stops sending anything. Port completeRequest() from main: retire the request by identity, keeping the O(1) in-order fast path, and splice it out when it finished out of order. Cleared slots can now appear in the queue, so the paths that walk it skip them, as they do on main. Refs: #5404 Refs: #5410 Refs: #5569 Claude-Session: https://claude.ai/code/session_01A49JamgF2TkZHu5h58ChUM Signed-off-by: Matteo Collina <hello@matteocollina.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>

staylor · 2026-07-17T13:39:04Z

This relates to...

Fixes#5566.

Rationale

The HTTP/2 in-order completion optimization introduced in #5483 advanced kRunningIdx without clearing the completed request's queue slot. Until the periodic queue compaction ran, each dead prefix slot retained its Request, whose controller retained the ClientHttp2Stream and the rest of the fetch graph. Under sustained HTTP/2 abort churn this retained native response buffers and caused memory growth. A 20,000-request reproduction at 50% aborts grew the heap by 10.6 MB before this change and 2.8 MB after it, matching the pre-#5483 baseline.

A stream can also close after its request has already been marked aborted. Terminal stream cleanup must still finalize that request even though onResponseEnd must not run.

Changes

Features

N/A

Bug Fixes

Clear an in-order completed HTTP/2 request's queue slot before advancing kRunningIdx, preserving the O(1) fast path and matching the existing HTTP/1 queue behavior.
Finalize terminal HTTP/2 streams independently of whether the response ended successfully.
Add regression coverage for an already-aborted request closing without response completion.

Breaking Changes and Deprecations

N/A

Status

The HTTP/2 in-order completion fast path advanced the running index but retained the completed Request in its queue slot until batched compaction. Under sustained abort churn, each retained Request pinned its stream and fetch graph, including native response buffers. Clear the completed slot before advancing, preserving the O(1) fast path, and finalize closed streams even when the Request was already marked aborted. Fixes: nodejs#5566 Signed-off-by: Scott Taylor <scott.c.taylor@mac.com> Assisted-By: devx/4ad398e1-8641-4e85-a62d-cb78992a7aae

codecov-commenter · 2026-07-17T13:56:28Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.47%. Comparing base (87270e4) to head (7ffd5c3).
⚠️ Report is 3 commits behind head on main.

Additional details and impacted files

@@ Coverage Diff @@## main #5569 +/- ##
=======================================
Coverage 93.47% 93.47% =======================================
Files 110 110 Lines 37507 37561 +54 =======================================
+ Hits 35059 35111 +52 - Misses 2448 2450 +2

☔ 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

lgtm

mcollina · 2026-07-18T18:27:49Z

I'm relatively concerned by this PR (and previous ones) that do not actually provide e2e tests.

Signed-off-by: Scott Taylor <scott.c.taylor@mac.com> Assisted-By: devx/7d65db46-a09e-471a-9e1c-abdaa5accba8

staylor · 2026-07-19T03:36:56Z

Thanks @mcollina — agreed. I’ve added an end-to-end regression test in 84ab33d. It uses a real TLS HTTP/2 server through the public fetch/Client APIs, aborts an in-flight response while keeping the session open and usable, and verifies that the aborted fetch graph is released. The test fails against the parent commit and passes with this fix.

metcoder95

small comment; rest lgtm

mcollina

lgtm

Co-authored-by: Carlos Fuentes <me@metcoder.dev>

Signed-off-by: Matteo Collina <hello@matteocollina.com>

mcollina

lgtm

…#5618) Backport of #5410 (and the fast path from #5569). HTTP/2 completes out of order, but every completion site advanced the running index blindly: client[kQueue][client[kRunningIdx]++] = null so whichever request happened to sit at the head was retired instead of the one that actually finished. With two streams in flight, completing the second one clears the first's slot while the second stays in the running window for good: after /second queue=[null, "/second"] runningIdx=1 pendingIdx=2 after /third queue=[null, null, "/third"] runningIdx=2 pendingIdx=3 The still-running /first is gone from the queue and two finished requests are counted as running forever, so kRunning never returns to zero. Since _resume() stops dispatching once kRunning reaches the concurrency limit, a client accumulating these eventually stops sending anything. Port completeRequest() from main: retire the request by identity, keeping the O(1) in-order fast path, and splice it out when it finished out of order. Cleared slots can now appear in the queue, so the paths that walk it skip them, as they do on main. Refs: #5404 Refs: #5410 Refs: #5569 Claude-Session: https://claude.ai/code/session_01A49JamgF2TkZHu5h58ChUM Signed-off-by: Matteo Collina <hello@matteocollina.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>

mcollina requested a review from trivikr July 17, 2026 15:02

mcollina approved these changes Jul 17, 2026
View reviewed changes

mcollina requested a review from metcoder95 July 17, 2026 15:38

test(h2): cover aborted fetch queue retention
84ab33d
Signed-off-by: Scott Taylor <scott.c.taylor@mac.com> Assisted-By: devx/7d65db46-a09e-471a-9e1c-abdaa5accba8

metcoder95 reviewed Jul 19, 2026
View reviewed changes

Comment threadtest/fetch/issue-5566.js Outdated

mcollina approved these changes Jul 20, 2026
View reviewed changes

mcollinaand others added 2 commits July 20, 2026 04:07

Update test/fetch/issue-5566.js
e0e3ab7
Co-authored-by: Carlos Fuentes <me@metcoder.dev>

test(fetch): fix issue 5566 test declaration
7ffd5c3
Signed-off-by: Matteo Collina <hello@matteocollina.com>

mcollina approved these changes Jul 20, 2026
View reviewed changes

mcollina merged commit f87273e into nodejs:mainJul 20, 2026
38 checks passed

github-actionsBot mentioned this pull request Jul 20, 2026
[Release] v8.8.0 #5575
Merged

mcollina mentioned this pull request Jul 29, 2026
fix(h2): retire the request that completed, not the head of the queue #5618
Merged