Ensure session close event fires before stream close callbacks

[DavidUmunna]

This pull request addresses a subtle but important issue in the HTTP/2 session shutdown process to ensure correct event ordering when a session is closed, particularly when the underlying socket is abruptly destroyed. The main change is to guarantee that the session's 'close' event is emitted before any stream 'close' events that observe the session as destroyed, aligning with expected user-facing behavior. The fix is validated with both a new regression test and a simulation to demonstrate the correct ordering.

HTTP/2 Session Close Event Ordering Fix:

• In lib/internal/http2/core.js, the order of operations in closeSession was changed so that the session handle is destroyed before any pending or open streams. This guarantees that the session's 'close' event is queued before any stream 'close' events, ensuring user code observes the correct session state.

Testing and Validation:

• Added test/parallel/test-http2-session-close-before-stream-close.js, a regression test that verifies the session 'close' event fires before any stream 'close' callback can observe session.closed/session.destroyed as true, preventing potential user-facing bugs.
• Added test/parallel/test-http2-session-close-order-simulation.js, a simulation test that models the nextTick event ordering to clearly demonstrate and validate the fix logic without requiring a compiled binary.

[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.03%. Comparing base (4ee7567) to head (e922ae8).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #63422      +/-   ##
==========================================
- Coverage   90.05%   90.03%   -0.02%     
==========================================
  Files         714      714              
  Lines      225876   225882       +6     
  Branches    42737    42732       -5     
==========================================
- Hits       203408   203371      -37     
- Misses      14244    14288      +44     
+ Partials     8224     8223       -1     

Files with missing lines	Coverage Δ
lib/internal/http2/core.js	95.22% <100.00%> (+0.03%)	⬆️

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

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.

[DavidUmunna]

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?