http2: fix onread assert when destroying session from stream handler

[sankalpsthakur]

When session.destroy() is called from a 'stream' handler, MakeCallback drains nextTick while nghttp2 is still inside mem_recv. Session close is deferred for that window (from #64166), so later HEADERS in the same receive buffer created C++ streams without a JS wrapper / onread. Subsequent DATA delivery hit:

Assertion failed: onread->IsFunction()
<- StreamBase::CallJSOnreadMethod
<- Http2StreamListener::OnStreamRead
<- Http2Session::OnDataChunkReceived

Changes

1. Do not create streams while the session is closing (OnBeginHeadersCallback) — RST with NGHTTP2_CANCEL instead.
2. If on_headers runs after JS destroy, destroy the C++ handle (mirrors the existing session.closed path).
3. Defensive: drop DATA when onread is not a function rather than aborting.
4. Regression test exercising concurrent POSTs with session.destroy() from the stream handler.

Test plan

• New test/parallel/test-http2-session-destroy-stream-handler.js (40 rounds × 8 concurrent POSTs)

Fixes#64850

AI/LLM disclosure

• AI coding tools (including Grok and/or Codex agent-assisted editing) were used to help draft or modify code and this PR description.
• I reviewed the complete change, understand the reasoning, and added the regression test listed above.
• This submission is original work of authorship under the project CLA / contributor terms; AI output was not pasted unreviewed.

[nodejs-github-bot]

Review requested:

• @nodejs/http2
• @nodejs/net

[mcollina]

lgtm

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/76753/

[codecov]

Codecov Report

❌ Patch coverage is 66.66667% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.98%. Comparing base (2befec5) to head (eaa9b7a).
⚠️ Report is 5 commits behind head on main.

Files with missing lines	Patch %	Lines
lib/internal/http2/core.js	50.00%	4 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #65116 +/- ##
==========================================
- Coverage 89.99% 89.98% -0.02% 
==========================================
Files 757 757 Lines 257739 257750 +11 Branches 48881 48879 -2 ==========================================
- Hits 231961 231943 -18 - Misses 16861 16899 +38 + Partials 8917 8908 -9 

Files with missing lines	Coverage Δ
src/node_http2.cc	81.83% <100.00%> (-0.11%)	⬇️
lib/internal/http2/core.js	94.99% <50.00%> (-0.10%)	⬇️

... and 36 files with indirect coverage changes

🚀 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.

[pimterry]

Thanks @sankalpsthakur!

Mostly looks good. The 3 fixes seem to all separately solve the same issue redundantly, is that right? I think for the first two that's OK and they're independently valuable (with minor tweaks) but we should drop the 3rd chunk to avoid missing major issues in future.

There's also a failing lint here, you can fix this locally with make format-cpp.

[mcollina]

@pimterry ptal

[mcollina]

lgtm

[pimterry]

LGTM, thanks for working through those fixes @sankalpsthakur!

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/76908/

[sankalpsthakur]

When session.destroy() is called from a 'stream' handler, MakeCallback drains nextTick while nghttp2 is still inside mem_recv. Session close is deferred for that window (from #64166), so later HEADERS in the same receive buffer created C++ streams without a JS wrapper / onread. Subsequent DATA delivery hit:

Assertion failed: onread->IsFunction()
<- StreamBase::CallJSOnreadMethod
<- Http2StreamListener::OnStreamRead
<- Http2Session::OnDataChunkReceived

Changes

1. Do not create streams while the session is closing (OnBeginHeadersCallback) — RST with NGHTTP2_CANCEL instead.
2. If on_headers runs after JS destroy, destroy the C++ handle (mirrors the existing session.closed path).
3. Defensive: drop DATA when onread is not a function rather than aborting.
4. Regression test exercising concurrent POSTs with session.destroy() from the stream handler.

Test plan

• New test/parallel/test-http2-session-destroy-stream-handler.js (40 rounds × 8 concurrent POSTs)

Fixes#64850

AI/LLM disclosure

• AI coding tools (including Grok and/or Codex agent-assisted editing) were used to help draft or modify code and this PR description.
• I reviewed the complete change, understand the reasoning, and added the regression test listed above.
• This submission is original work of authorship under the project CLA / contributor terms; AI output was not pasted unreviewed.

[nodejs-github-bot]

Review requested:

• @nodejs/http2
• @nodejs/net

[mcollina]

lgtm

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/76753/

[codecov]

Codecov Report

❌ Patch coverage is 66.66667% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.98%. Comparing base (2befec5) to head (eaa9b7a).
⚠️ Report is 5 commits behind head on main.

Files with missing lines	Patch %	Lines
lib/internal/http2/core.js	50.00%	4 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #65116 +/- ##
==========================================
- Coverage 89.99% 89.98% -0.02% 
==========================================
Files 757 757 Lines 257739 257750 +11 Branches 48881 48879 -2 ==========================================
- Hits 231961 231943 -18 - Misses 16861 16899 +38 + Partials 8917 8908 -9 

Files with missing lines	Coverage Δ
src/node_http2.cc	81.83% <100.00%> (-0.11%)	⬇️
lib/internal/http2/core.js	94.99% <50.00%> (-0.10%)	⬇️

... and 36 files with indirect coverage changes

🚀 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.

[pimterry]

Thanks @sankalpsthakur!

Mostly looks good. The 3 fixes seem to all separately solve the same issue redundantly, is that right? I think for the first two that's OK and they're independently valuable (with minor tweaks) but we should drop the 3rd chunk to avoid missing major issues in future.

There's also a failing lint here, you can fix this locally with make format-cpp.

[mcollina]

@pimterry ptal

[mcollina]

lgtm

[pimterry]

LGTM, thanks for working through those fixes @sankalpsthakur!

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/76908/

[sankalpsthakur]

When session.destroy() is called from a 'stream' handler, MakeCallback drains nextTick while nghttp2 is still inside mem_recv. Session close is deferred for that window (from #64166), so later HEADERS in the same receive buffer created C++ streams without a JS wrapper / onread. Subsequent DATA delivery hit:

Assertion failed: onread->IsFunction()
<- StreamBase::CallJSOnreadMethod
<- Http2StreamListener::OnStreamRead
<- Http2Session::OnDataChunkReceived

Changes

1. Do not create streams while the session is closing (OnBeginHeadersCallback) — RST with NGHTTP2_CANCEL instead.
2. If on_headers runs after JS destroy, destroy the C++ handle (mirrors the existing session.closed path).
3. Defensive: drop DATA when onread is not a function rather than aborting.
4. Regression test exercising concurrent POSTs with session.destroy() from the stream handler.

Test plan

• New test/parallel/test-http2-session-destroy-stream-handler.js (40 rounds × 8 concurrent POSTs)

Fixes#64850

AI/LLM disclosure

• AI coding tools (including Grok and/or Codex agent-assisted editing) were used to help draft or modify code and this PR description.
• I reviewed the complete change, understand the reasoning, and added the regression test listed above.
• This submission is original work of authorship under the project CLA / contributor terms; AI output was not pasted unreviewed.

[nodejs-github-bot]

Review requested:

• @nodejs/http2
• @nodejs/net

[mcollina]

lgtm

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/76753/

[codecov]

Codecov Report

❌ Patch coverage is 66.66667% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.98%. Comparing base (2befec5) to head (eaa9b7a).
⚠️ Report is 5 commits behind head on main.

Files with missing lines	Patch %	Lines
lib/internal/http2/core.js	50.00%	4 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #65116 +/- ##
==========================================
- Coverage 89.99% 89.98% -0.02% 
==========================================
Files 757 757 Lines 257739 257750 +11 Branches 48881 48879 -2 ==========================================
- Hits 231961 231943 -18 - Misses 16861 16899 +38 + Partials 8917 8908 -9 

Files with missing lines	Coverage Δ
src/node_http2.cc	81.83% <100.00%> (-0.11%)	⬇️
lib/internal/http2/core.js	94.99% <50.00%> (-0.10%)	⬇️

... and 36 files with indirect coverage changes

🚀 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.

[pimterry]

Thanks @sankalpsthakur!

Mostly looks good. The 3 fixes seem to all separately solve the same issue redundantly, is that right? I think for the first two that's OK and they're independently valuable (with minor tweaks) but we should drop the 3rd chunk to avoid missing major issues in future.

There's also a failing lint here, you can fix this locally with make format-cpp.

[mcollina]

@pimterry ptal

[mcollina]

lgtm

[pimterry]

LGTM, thanks for working through those fixes @sankalpsthakur!

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/76908/

[sankalpsthakur]

When session.destroy() is called from a 'stream' handler, MakeCallback drains nextTick while nghttp2 is still inside mem_recv. Session close is deferred for that window (from #64166), so later HEADERS in the same receive buffer created C++ streams without a JS wrapper / onread. Subsequent DATA delivery hit:

Assertion failed: onread->IsFunction()
<- StreamBase::CallJSOnreadMethod
<- Http2StreamListener::OnStreamRead
<- Http2Session::OnDataChunkReceived

Changes

1. Do not create streams while the session is closing (OnBeginHeadersCallback) — RST with NGHTTP2_CANCEL instead.
2. If on_headers runs after JS destroy, destroy the C++ handle (mirrors the existing session.closed path).
3. Defensive: drop DATA when onread is not a function rather than aborting.
4. Regression test exercising concurrent POSTs with session.destroy() from the stream handler.

Test plan

• New test/parallel/test-http2-session-destroy-stream-handler.js (40 rounds × 8 concurrent POSTs)

Fixes#64850

AI/LLM disclosure

• AI coding tools (including Grok and/or Codex agent-assisted editing) were used to help draft or modify code and this PR description.
• I reviewed the complete change, understand the reasoning, and added the regression test listed above.
• This submission is original work of authorship under the project CLA / contributor terms; AI output was not pasted unreviewed.

[nodejs-github-bot]

Review requested:

• @nodejs/http2
• @nodejs/net

[mcollina]

lgtm

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/76753/

[codecov]

Codecov Report

❌ Patch coverage is 66.66667% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.98%. Comparing base (2befec5) to head (eaa9b7a).
⚠️ Report is 5 commits behind head on main.

Files with missing lines	Patch %	Lines
lib/internal/http2/core.js	50.00%	4 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #65116 +/- ##
==========================================
- Coverage 89.99% 89.98% -0.02% 
==========================================
Files 757 757 Lines 257739 257750 +11 Branches 48881 48879 -2 ==========================================
- Hits 231961 231943 -18 - Misses 16861 16899 +38 + Partials 8917 8908 -9 

Files with missing lines	Coverage Δ
src/node_http2.cc	81.83% <100.00%> (-0.11%)	⬇️
lib/internal/http2/core.js	94.99% <50.00%> (-0.10%)	⬇️

... and 36 files with indirect coverage changes

🚀 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.

[pimterry]

Thanks @sankalpsthakur!

Mostly looks good. The 3 fixes seem to all separately solve the same issue redundantly, is that right? I think for the first two that's OK and they're independently valuable (with minor tweaks) but we should drop the 3rd chunk to avoid missing major issues in future.

There's also a failing lint here, you can fix this locally with make format-cpp.

[mcollina]

@pimterry ptal

[mcollina]

lgtm

[pimterry]

LGTM, thanks for working through those fixes @sankalpsthakur!

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/76908/

[sankalpsthakur]

When session.destroy() is called from a 'stream' handler, MakeCallback drains nextTick while nghttp2 is still inside mem_recv. Session close is deferred for that window (from #64166), so later HEADERS in the same receive buffer created C++ streams without a JS wrapper / onread. Subsequent DATA delivery hit:

Assertion failed: onread->IsFunction()
<- StreamBase::CallJSOnreadMethod
<- Http2StreamListener::OnStreamRead
<- Http2Session::OnDataChunkReceived

Changes

1. Do not create streams while the session is closing (OnBeginHeadersCallback) — RST with NGHTTP2_CANCEL instead.
2. If on_headers runs after JS destroy, destroy the C++ handle (mirrors the existing session.closed path).
3. Defensive: drop DATA when onread is not a function rather than aborting.
4. Regression test exercising concurrent POSTs with session.destroy() from the stream handler.

Test plan

• New test/parallel/test-http2-session-destroy-stream-handler.js (40 rounds × 8 concurrent POSTs)

Fixes#64850

AI/LLM disclosure

• AI coding tools (including Grok and/or Codex agent-assisted editing) were used to help draft or modify code and this PR description.
• I reviewed the complete change, understand the reasoning, and added the regression test listed above.
• This submission is original work of authorship under the project CLA / contributor terms; AI output was not pasted unreviewed.

[nodejs-github-bot]

Review requested:

• @nodejs/http2
• @nodejs/net

[mcollina]

lgtm

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/76753/

[codecov]

Codecov Report

❌ Patch coverage is 66.66667% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.98%. Comparing base (2befec5) to head (eaa9b7a).
⚠️ Report is 5 commits behind head on main.

Files with missing lines	Patch %	Lines
lib/internal/http2/core.js	50.00%	4 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #65116 +/- ##
==========================================
- Coverage 89.99% 89.98% -0.02% 
==========================================
Files 757 757 Lines 257739 257750 +11 Branches 48881 48879 -2 ==========================================
- Hits 231961 231943 -18 - Misses 16861 16899 +38 + Partials 8917 8908 -9 

Files with missing lines	Coverage Δ
src/node_http2.cc	81.83% <100.00%> (-0.11%)	⬇️
lib/internal/http2/core.js	94.99% <50.00%> (-0.10%)	⬇️

... and 36 files with indirect coverage changes

🚀 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.

[pimterry]

Thanks @sankalpsthakur!

Mostly looks good. The 3 fixes seem to all separately solve the same issue redundantly, is that right? I think for the first two that's OK and they're independently valuable (with minor tweaks) but we should drop the 3rd chunk to avoid missing major issues in future.

There's also a failing lint here, you can fix this locally with make format-cpp.

[mcollina]

@pimterry ptal

[mcollina]

lgtm

[pimterry]

LGTM, thanks for working through those fixes @sankalpsthakur!

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/76908/

[sankalpsthakur]

When session.destroy() is called from a 'stream' handler, MakeCallback drains nextTick while nghttp2 is still inside mem_recv. Session close is deferred for that window (from #64166), so later HEADERS in the same receive buffer created C++ streams without a JS wrapper / onread. Subsequent DATA delivery hit:

Assertion failed: onread->IsFunction()
<- StreamBase::CallJSOnreadMethod
<- Http2StreamListener::OnStreamRead
<- Http2Session::OnDataChunkReceived

Changes

1. Do not create streams while the session is closing (OnBeginHeadersCallback) — RST with NGHTTP2_CANCEL instead.
2. If on_headers runs after JS destroy, destroy the C++ handle (mirrors the existing session.closed path).
3. Defensive: drop DATA when onread is not a function rather than aborting.
4. Regression test exercising concurrent POSTs with session.destroy() from the stream handler.

Test plan

• New test/parallel/test-http2-session-destroy-stream-handler.js (40 rounds × 8 concurrent POSTs)

Fixes#64850

AI/LLM disclosure

• AI coding tools (including Grok and/or Codex agent-assisted editing) were used to help draft or modify code and this PR description.
• I reviewed the complete change, understand the reasoning, and added the regression test listed above.
• This submission is original work of authorship under the project CLA / contributor terms; AI output was not pasted unreviewed.

[nodejs-github-bot]

Review requested:

• @nodejs/http2
• @nodejs/net

[mcollina]

lgtm

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/76753/

[codecov]

Codecov Report

❌ Patch coverage is 66.66667% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.98%. Comparing base (2befec5) to head (eaa9b7a).
⚠️ Report is 5 commits behind head on main.

Files with missing lines	Patch %	Lines
lib/internal/http2/core.js	50.00%	4 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #65116 +/- ##
==========================================
- Coverage 89.99% 89.98% -0.02% 
==========================================
Files 757 757 Lines 257739 257750 +11 Branches 48881 48879 -2 ==========================================
- Hits 231961 231943 -18 - Misses 16861 16899 +38 + Partials 8917 8908 -9 

Files with missing lines	Coverage Δ
src/node_http2.cc	81.83% <100.00%> (-0.11%)	⬇️
lib/internal/http2/core.js	94.99% <50.00%> (-0.10%)	⬇️

... and 36 files with indirect coverage changes

🚀 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.

[pimterry]

Thanks @sankalpsthakur!

Mostly looks good. The 3 fixes seem to all separately solve the same issue redundantly, is that right? I think for the first two that's OK and they're independently valuable (with minor tweaks) but we should drop the 3rd chunk to avoid missing major issues in future.

There's also a failing lint here, you can fix this locally with make format-cpp.

[mcollina]

@pimterry ptal

[mcollina]

lgtm

[pimterry]

LGTM, thanks for working through those fixes @sankalpsthakur!

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/76908/

[sankalpsthakur]

When session.destroy() is called from a 'stream' handler, MakeCallback drains nextTick while nghttp2 is still inside mem_recv. Session close is deferred for that window (from #64166), so later HEADERS in the same receive buffer created C++ streams without a JS wrapper / onread. Subsequent DATA delivery hit:

Assertion failed: onread->IsFunction()
<- StreamBase::CallJSOnreadMethod
<- Http2StreamListener::OnStreamRead
<- Http2Session::OnDataChunkReceived

Changes

1. Do not create streams while the session is closing (OnBeginHeadersCallback) — RST with NGHTTP2_CANCEL instead.
2. If on_headers runs after JS destroy, destroy the C++ handle (mirrors the existing session.closed path).
3. Defensive: drop DATA when onread is not a function rather than aborting.
4. Regression test exercising concurrent POSTs with session.destroy() from the stream handler.

Test plan

• New test/parallel/test-http2-session-destroy-stream-handler.js (40 rounds × 8 concurrent POSTs)

Fixes#64850

AI/LLM disclosure

• AI coding tools (including Grok and/or Codex agent-assisted editing) were used to help draft or modify code and this PR description.
• I reviewed the complete change, understand the reasoning, and added the regression test listed above.
• This submission is original work of authorship under the project CLA / contributor terms; AI output was not pasted unreviewed.

[nodejs-github-bot]

Review requested:

• @nodejs/http2
• @nodejs/net

[mcollina]

lgtm

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/76753/

[codecov]

Codecov Report

❌ Patch coverage is 66.66667% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.98%. Comparing base (2befec5) to head (eaa9b7a).
⚠️ Report is 5 commits behind head on main.

Files with missing lines	Patch %	Lines
lib/internal/http2/core.js	50.00%	4 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #65116 +/- ##
==========================================
- Coverage 89.99% 89.98% -0.02% 
==========================================
Files 757 757 Lines 257739 257750 +11 Branches 48881 48879 -2 ==========================================
- Hits 231961 231943 -18 - Misses 16861 16899 +38 + Partials 8917 8908 -9 

Files with missing lines	Coverage Δ
src/node_http2.cc	81.83% <100.00%> (-0.11%)	⬇️
lib/internal/http2/core.js	94.99% <50.00%> (-0.10%)	⬇️

... and 36 files with indirect coverage changes

🚀 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.

[pimterry]

Thanks @sankalpsthakur!

Mostly looks good. The 3 fixes seem to all separately solve the same issue redundantly, is that right? I think for the first two that's OK and they're independently valuable (with minor tweaks) but we should drop the 3rd chunk to avoid missing major issues in future.

There's also a failing lint here, you can fix this locally with make format-cpp.

[mcollina]

@pimterry ptal

[mcollina]

lgtm

[pimterry]

LGTM, thanks for working through those fixes @sankalpsthakur!

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/76908/

[sankalpsthakur]

When session.destroy() is called from a 'stream' handler, MakeCallback drains nextTick while nghttp2 is still inside mem_recv. Session close is deferred for that window (from #64166), so later HEADERS in the same receive buffer created C++ streams without a JS wrapper / onread. Subsequent DATA delivery hit:

Assertion failed: onread->IsFunction()
<- StreamBase::CallJSOnreadMethod
<- Http2StreamListener::OnStreamRead
<- Http2Session::OnDataChunkReceived

Changes

1. Do not create streams while the session is closing (OnBeginHeadersCallback) — RST with NGHTTP2_CANCEL instead.
2. If on_headers runs after JS destroy, destroy the C++ handle (mirrors the existing session.closed path).
3. Defensive: drop DATA when onread is not a function rather than aborting.
4. Regression test exercising concurrent POSTs with session.destroy() from the stream handler.

Test plan

• New test/parallel/test-http2-session-destroy-stream-handler.js (40 rounds × 8 concurrent POSTs)

Fixes#64850

AI/LLM disclosure

• AI coding tools (including Grok and/or Codex agent-assisted editing) were used to help draft or modify code and this PR description.
• I reviewed the complete change, understand the reasoning, and added the regression test listed above.
• This submission is original work of authorship under the project CLA / contributor terms; AI output was not pasted unreviewed.

[nodejs-github-bot]

Review requested:

• @nodejs/http2
• @nodejs/net

[mcollina]

lgtm

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/76753/

[codecov]

Codecov Report

❌ Patch coverage is 66.66667% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.98%. Comparing base (2befec5) to head (eaa9b7a).
⚠️ Report is 5 commits behind head on main.

Files with missing lines	Patch %	Lines
lib/internal/http2/core.js	50.00%	4 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #65116 +/- ##
==========================================
- Coverage 89.99% 89.98% -0.02% 
==========================================
Files 757 757 Lines 257739 257750 +11 Branches 48881 48879 -2 ==========================================
- Hits 231961 231943 -18 - Misses 16861 16899 +38 + Partials 8917 8908 -9 

Files with missing lines	Coverage Δ
src/node_http2.cc	81.83% <100.00%> (-0.11%)	⬇️
lib/internal/http2/core.js	94.99% <50.00%> (-0.10%)	⬇️

... and 36 files with indirect coverage changes

🚀 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.

[pimterry]

Thanks @sankalpsthakur!

Mostly looks good. The 3 fixes seem to all separately solve the same issue redundantly, is that right? I think for the first two that's OK and they're independently valuable (with minor tweaks) but we should drop the 3rd chunk to avoid missing major issues in future.

There's also a failing lint here, you can fix this locally with make format-cpp.

[mcollina]

@pimterry ptal

[mcollina]

lgtm

[pimterry]

LGTM, thanks for working through those fixes @sankalpsthakur!

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/76908/