Review requested:

• @nodejs/http2
• @nodejs/net

Codecov Report

❌ Patch coverage is 94.44444% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 90.14%. Comparing base (3553a34) to head (a538ee7).
⚠️ Report is 17 commits behind head on main.

@@           Coverage Diff           @@
##             main   #63422   +/-   ##
=======================================
  Coverage   90.13%   90.14%           
=======================================
  Files         718      718           
  Lines      227914   227927   +13     
  Branches    42811    42811           
=======================================
+ Hits       205435   205468   +33     
+ Misses      14248    14244    -4     
+ Partials     8231     8215   -16     

... and 23 files with indirect coverage changes

• ❄️ 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.

Hi @DavidUmunna thanks for the PR! Can you explain more about why you want to do this? You describe it as 'correct' but I'm not really sure that's true, or why this ordering would be preferable.

Why should the session close before the streams within it do? I would normally assume the other way around: so that we clean up all the inner state, and then the outer state, and session.closed isn't true until the streams within are all closed as well. It seems we haven't formally documented the specific order, until now but the current state isn't obviously wrong to me.

Hi @DavidUmunna thanks for the PR! Can you explain more about why you want to do this? You describe it as 'correct' but I'm not really sure that's true, or why this ordering would be preferable.

Why should the session close before the streams within it do? I would normally assume the other way around: so that we clean up all the inner state, and then the outer state, and session.closed isn't true until the streams within are all closed as well. It seems we haven't formally documented the specific order, until now but the current state isn't obviously wrong to me.

Hi @pimterry , thanks for leaving a comment, the aim of the PR was to address an issue posted yesterday which had to do with a race in closeSession, where stream close events were emitted before the session close event, which allowed stream callbacks to observe session.destroyed==true and throw invalid session error before the application code had a chance to handle the session level signal,

shouldn't session events signal session stale before anything else?

Thanks. This does look like it is addressing the same race as #63412.

I do not think the current fix is safe as-is, though. Reordering closeSession() so handle.destroy() runs before stream destruction changes teardown behavior more broadly than the socket-already-closed case from the issue.

In particular, CI is showing a regression in test/parallel/test-http2-server-session-destroy.js with an unhandled ERR_HTTP2_STREAM_ERROR / NGHTTP2_REFUSED_STREAM, with the stack going through onStreamClose -> closeSession -> ClientHttp2Session.destroy.

That suggests the global reordering is affecting normal session teardown, not just the abrupt transport-close path.

I think the safer direction is to preserve the existing shutdown order and only queue the session 'close' event early when the transport is already closed. That narrows the change to the specific race from #63412 without perturbing other teardown paths.

Also, test-http2-session-close-order-simulation.js currently fails lint and probably is not needed if the regression test covers the real behavior.

Thanks @pimterry i saw the comment you made to @mcollina's PR #63414 ,based on that, I reverted what i did and just made sure that the caller doesnt crash and the session had time to fire and clear the cached session before the error arrives

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

@DavidUmunna Can you please rebase?

The merge commits don't work well with our tooling.

@trivikr no problem I’ll do that

Closing this — the underlying race (#63412) was already fixed and merged via #63414 (bac704d3d3), which takes a narrower approach (adjusting emitClose ordering and deferring session.request()'s synchronous throws) rather than reordering closeSession()'s stream/handle teardown. That reorder was the approach explored here, and @mcollina's review already flagged it as causing a regression in test/parallel/test-http2-server-session-destroy.js.

Thanks all for the review and discussion — closing as superseded by #63414.