http: prevent reuse after incomplete request destruction

[dayun6530]

Refs: #49429

When a server IncomingMessage is consumed with an async iterator and iteration terminates before the request body is fully consumed, the stream destroy path detaches the request from its socket before destroying it. This leaves the underlying keep-alive connection open even though the request was not fully consumed.

Unread request body data can then reach the destroyed stream, pause the socket, and make a subsequent request that reuses the same connection stall.

This change prevents that connection from being reused:

• If the current response headers have not been sent yet, the response is marked non-keep-alive and as the last response on the connection. The current response can still complete, but the client receives Connection: close and the socket is not reused.
• If the response headers have already been sent, the response is destroyed. At that point the keep-alive decision has already been committed, so destroying the response/socket avoids reusing a connection whose request was not fully consumed.

The request stream is then detached and destroyed as before.

Regression tests cover both cases:

• headers not sent: the first response completes with Connection: close, and the next request uses a new socket;
• headers already sent: the first response is aborted with ECONNRESET, and the next request uses a new socket.

Tests:

• make lint-js
• python3 tools/test.py --mode=release test/parallel/test-http-server-for-await-keepalive.js test/parallel/test-http-server-for-await-keepalive-headers-sent.js
• python3 tools/test.py --mode=release test/parallel/test-stream-destroy.js test/parallel/test-http-server-incomingmessage-destroy.js test/parallel/test-http-incoming-pipelined-socket-destroy.js

[nodejs-github-bot]

Review requested:

• @nodejs/streams

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.37%. Comparing base (691cf62) to head (70234e9).
⚠️ Report is 20 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #65674      +/-   ##
==========================================
- Coverage   92.77%   90.37%   -2.40%     
==========================================
  Files         422      792     +370     
  Lines      192675   275576   +82901     
  Branches    29668    52826   +23158     
==========================================
+ Hits       178746   249059   +70313     
- Misses      13604    16914    +3310     
- Partials      325     9603    +9278     

Files with missing lines	Coverage Δ
lib/_http_incoming.js	97.70% <100.00%> (+0.04%)	⬆️

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

[ronag]

Something more fundamental is wrong here. The socket should not be re-used if destroyed without being fully consumed and anything else in the pipeline queue should also be cancelled.

[ronag]

@mcollina

[dayun6530]

Thanks for the review. I've updated the change so that an incompletely consumed request prevents the underlying connection from being reused, and added regression tests for both the headers-not-sent and headers-already-sent cases.

The remaining Test Shared libraries failure is from test-tick-processor-arguments.js timing out on x86_64-darwin, which appears unrelated to this change.

@ronag Could you please take another look when you have a chance? A re-run of the failed CI job would also be appreciated.

[dayun6530]

@ronag Just following up on the Sep 10 update.
I changed the fix so that an incompletely consumed request prevents the underlying connection from being reused, and added regression tests for both the headers-not-sent and headers-already-sent cases.
Could you please take another look when you have a chance? If the direction looks good, a CI rerun would also be appreciated. Thanks!

[mcollina]

My understanding is that the fix for this needs to happen in http, not in streams.

[dayun6530]

Thanks for the feedback. I've moved the fix out of the streams implementation and into HTTP handling in IncomingMessage._destroy().

The regression tests still cover both cases:

• before response headers are sent, the connection is marked non-keep-alive;
• after headers are sent, the response/socket is destroyed to prevent reuse.

I've also rebased onto the current main, and the relevant local tests and make lint-js pass.

@mcollina @ronag Could you please take another look when you have a chance? Thanks!