Fix HTTP/2 RST_STREAM behaviour, add auto-drain, deprecate 'aborted', fix related compat API issues

[pimterry]

This PR replaces #62923, which aims to fix #56627. The goal here is to make hard-shutdown (RST_STREAM messages) for HTTP/2 streams behave predictably and remove a bunch of footguns. This became a bit of a rabbit hole, but I think I have a good understanding of the problem and a much better behaviour that should be workable, although is definitely requires some breaking changes (though ~all affected code is broken in some ways today anyway).

To implement this, this PR does a few related things:

• Makes HTTP/2 streams emit 'error' when we receive RST_STREAM (hard-stopping the stream in both directions) in specific cases only:
  • Remote client sends any error reset before the stream read & write are fully done.
  • Remote client sends a non-error reset, before our readable is complete.
• To avoid spurious errors in cases where you never read at all, this also enables auto-drain behaviour for HTTP/2 streams, matching HTTP/1. We now drain & dump the stream readable if:
  • For the client, you don't register a 'response' handler before the response arrives.
  • For the server, if you write and fully finish your response without touching the readable (no read/'data'/pipe/etc).
  • I believe this is the same logic as in HTTP/1, and it seems so far it's fairly intuitive for everybody, AFAICT.
• Deprecates aborted event & field, while preserving their current behaviour for now. These are confusing to the point of being effectively broken, they're already deprecated in HTTP/1, and they're not necessary.
• Updates the server compat API with corresponding fixes: exposing stream errors on the request, if there is an error listener (same as HTTP/1), and ensuring 'finish' only fires if the writable actually finished.

This fixes a few major existing issues. Most notably, without this PR, if you have a stream where you're finished writing but you're only part way through reading (e.g. normal client request flow) and you get a non-error RST_STREAM (a remote stream cancellation) then we close the readable cleanly, with no errors. In effect, we currently truncate cancelled responses with no warning whatsoever. The existing 'aborted' event is only emitted for incomplete writes, not reads.

This resolves that issue for reads. For writes, it stays close to existing behaviour but relaxes one key case: if you've read everything, and receive a RST_STREAM cancellation while writing (most commonly: a client aborts a request while a server is responding) then the writable closes immediately but doesn't emit an 'error' event. Effectively, we treat this as a quiet shutdown. This roughly matches HTTP/1 behaviour, and avoid spurious errors server-side. Even though there's no 'error' event, it's still easy to detect this: if you get 'close' without 'finish' (without writableFinished being true) while writing then the client has cancelled the request.

We could make aborted match the error behaviour here, but I've left it as is to avoid churn there, since I think we should deprecate it regardless so there's no point making unnecessary breaking changes in the meantime.

In the compat API, there's two other related existing problems this fixes:

• The compat API always emits 'finish' when the stream closes - even if the write was cancelled (and so never actually finished) by either peer. We now emit finish only if the writable finished.
• The compat API swallowed internal errors - it seems basically anything breaking the underlying stream or session wasn't exposed as an error. This happened because in the past in http2: refactor error handling #14991 we moved this into a separate streamError event, to deal with people not handling 'error', and then in http2: refactor close/destroy for Http2Stream and Http2Session #17406 we removed streamError entirely. The HTTP/1 'aborted' event does fire in most (all?) of these cases, but that's deprecated already anyway.

As a happy side bonus, as part of this work I hunted down the underlying issue for #58252: buggy cleanup of reset streams. This includes a reliable repro for the underlying issue (test/parallel/test-http2-server-stream-destroy-after-reset.js) and fixes that test and the flaky one as well, through with better cleanup behaviour in onStreamClose.

I suspect there'll be some debate around this, and it is definitely a non-trivial breaking change, but on the flip side this does show some quite substantial major issues in the current APIs, including silent read & write data loss everywhere, so I think it's worthwhile.

[nodejs-github-bot]

Review requested:

• @nodejs/http
• @nodejs/http2
• @nodejs/net
• @nodejs/userland-migrations

[codecov]

Codecov Report

❌ Patch coverage is 97.84173% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.03%. Comparing base (0a60e90) to head (6932225).
⚠️ Report is 8 commits behind head on main.

Files with missing lines	Patch %	Lines
lib/internal/http2/core.js	98.26%	2 Missing ⚠️
src/node_http2.cc	88.88%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #63249      +/-   ##
==========================================
+ Coverage   90.02%   90.03%   +0.01%     
==========================================
  Files         713      713              
  Lines      224950   225026      +76     
  Branches    42530    42561      +31     
==========================================
+ Hits       202513   202604      +91     
+ Misses      14220    14192      -28     
- Partials     8217     8230      +13     

Files with missing lines	Coverage Δ
lib/internal/errors.js	97.65% <100.00%> (+<0.01%)	⬆️
lib/internal/http2/compat.js	96.96% <100.00%> (+<0.01%)	⬆️
src/node_http2.h	91.86% <100.00%> (+0.24%)	⬆️
src/node_http2.cc	82.10% <88.88%> (+0.01%)	⬆️
lib/internal/http2/core.js	94.92% <98.26%> (-0.27%)	⬇️

... and 34 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.

[mcollina]

lgtm

[pimterry]

Pushed updates to the deprecation docs, tightened the tests a little, and covered an extra case: it turns out respondWithFD was also broken in very similar ways (e.g. finish fired when you didn't actually finish writing) and that wasn't previously fixed with the rest. Now updated and fixed to match the same behaviour there too.