[mcollina]

When a peer sends GOAWAY(NO_ERROR), core.js calls session.close(), which sets graceful_close_initiated_ and goaway_initiated_ without any application code. That disarmed the detection of nghttp2 terminating the session internally after a protocol error, leaving open streams and the connection hanging.

As suggested by @pimterry, this also treats it as an internal termination when nghttp2 no longer wants to read or write (want_read() == 0 && want_write() == 0) while we still track open streams. A test covers the peer GOAWAY followed by an oversized frame case.

---

AI generated, reviewed by me.

[nodejs-github-bot]

Review requested:

• @nodejs/http2
• @nodejs/net

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.50%. Comparing base (df2ea49) to head (1477f70).
⚠️ Report is 67 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #66641      +/-   ##
==========================================
- Coverage   90.51%   90.50%   -0.02%     
==========================================
  Files         797      797              
  Lines      278252   278263      +11     
  Branches    53472    53466       -6     
==========================================
- Hits       251868   251830      -38     
- Misses      16761    16808      +47     
- Partials     9623     9625       +2     

Files with missing lines	Coverage Δ
src/node_http2.cc	82.17% <100.00%> (+0.13%)	⬆️
src/node_http2.h	92.47% <ø> (+0.08%)	⬆️

... and 39 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]

All the tests are failing.

The current approach here has some clear false negatives, e.g. if we manually set the same error code that nghttp2 generates internally, we'd still hit the original issue. I'm not sure trying distinguish specific frames more precisely than "completely unexpected" is going to work well.

I think there's a better approach: nghttp2 says given want_read() == 0 && want_write() == 0, the application should drop the connection, and we could use that to detect this issue instead. In all of the cases we're looking at here, we'll reach that state unexpectedly. So in any scenario where we think we have an open stream on a session, but nghttp2 says the session is dead => an internal error must have happened somewhere.

Something like (AI gen):

bool Http2Session::HasStreamsAbandonedByNghttp2() const {
  if (HasPendingData()) return false;
  return std::ranges::any_of(streams_, [](const auto& entry) {
    return !entry.second->is_closed() && !entry.second->is_destroyed();
  });
}

and then make the check at the end of SendPendingData into:

if (internal_goaway_sent_ || HasStreamsAbandonedByNghttp2()) {
...

I'm surprised the description says this isn't testable - it looks very reachable I think, and Claude whipped up a few examples that seem to hit this immediately.

[pimterry]

Thanks! LGTM 👍

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[github-actions]

Failed to resume CI

✖ Failed to load PR CI job 78557: Unexpected token '<', ..."

Full Auto Start CI output

�[36m⠋�[39m Validating Jenkins credentials
�[36m⠋�[39m Validating Jenkins credentials
✔  Jenkins credentials valid
�[36m⠙�[39m Looking for CI runs for pull request 66641
�[36m⠙�[39m Looking for CI runs for pull request 66641
�[36m⠙�[39m Getting PR from nodejs/node/pull/66641
�[36m⠙�[39m Getting reviews from nodejs/node/pull/66641
�[36m⠙�[39m Getting comments from nodejs/node/pull/66641
✔  Found PR CI job 78557
�[36m⠹�[39m Querying data for job/node-test-pull-request/78557/
�[36m⠹�[39m Querying data for job/node-test-pull-request/78557/
�[36m⠹�[39m Querying API for job/node-test-pull-request/78557/

[DEBUG] [SyntaxError: Unexpected token '<', ..."    
  
  <!DOCTYPE "... is not valid JSON]
✖  Failed to load PR CI job 78557: Unexpected token '<', ..."    
  
  <!DOCTYPE "... is not valid JSON

View workflow run

[nodejs-github-bot]

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

[nodejs-github-bot]

Landed in 41b1e68