http: frame client requests that carry a payload

[zeexzeex]

A client request whose method defaults to no chunked encoding (GET, HEAD, DELETE, OPTIONS, TRACE) skipped both Content-Length and Transfer-Encoding even when a payload was written. The payload was still written to the socket, so the peer reads those bytes as the start of the next request.

const req = http.request({ method: 'DELETE', port });
req.write('PUT /admin HTTP/1.1\r\n\r\n');
req.end();

On the wire this produces a message with no framing headers followed by the payload, which the peer parses as a second request. _storeHeader() skipped header generation whenever useChunkedEncodingByDefault was false, regardless of whether a payload was actually written.

Such requests are now framed like any other: Content-Length when the length is known, a chunked Transfer-Encoding when it is not.

Three cases deliberately keep the previous behaviour:

• server responses, which have no method;
• CONNECT, whose payload is tunnelled data that must not be re-framed;
• requests where no payload write happened.

The last one needs more than _contentLength. Headers can be rendered before anything is written, for example when headers are passed as an array or an Expect header is set, and at that point the length is null just as it is for a write() of unknown length. A kPayloadPending flag records whether a payload write actually reached header rendering, so the two are no longer conflated.

Tests

test-http-clientrequest-end-contentlength and test-http-clientrequest-write-chunked now pass and move from known_issues to parallel. Both needed fixes unrelated to this change to be usable there: they asserted a response body for HEAD, which cannot carry one, and compared buffers with strictEqual.

test-http-request-method-delete-payload asserted the old behaviour and is updated to assert the payload arrives as a body.

test-http-clientrequest-end-empty-response-body stays in known_issues. It covers the Content-Length: 0 that end() adds for methods that do default to chunked encoding, which comes from a different code path and is unchanged here.

test/parallel/test-http-* and test/known_issues pass locally.

Fixes: #27880
Refs: #34066
Refs: #60718

[nodejs-github-bot]

Review requested:

• @nodejs/http
• @nodejs/net

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.15%. Comparing base (2dbf9d7) to head (8d8a358).
⚠️ Report is 6 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #65512      +/-   ##
==========================================
- Coverage   90.16%   90.15%   -0.02%     
==========================================
  Files         751      751              
  Lines      253465   253522      +57     
  Branches    47741    47756      +15     
==========================================
+ Hits       228537   228560      +23     
- Misses      16197    16212      +15     
- Partials     8731     8750      +19     

Files with missing lines	Coverage Δ
lib/_http_outgoing.js	97.79% <100.00%> (+0.01%)	⬆️

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

[zeexzeex]

CI is red on test/parallel/test-stream-pipeline.js. All 13 failing jobs report the same single failure, so this is one root cause rather than a flake. I pushed a commit updating the expectation, and I want to flag the behaviour change explicitly rather than let a test edit slide by unnoticed.

What happens

The test opens a client request, pipes a body into it, and destroys the source before EOF. It then asserts on the error the server side sees:

pipeline(req, res, common.mustCall((err) => {
  assert.strictEqual(err?.code, 'ERR_STREAM_PREMATURE_CLOSE');
}));

Before this PR the request carried neither Content-Length nor Transfer-Encoding, so the server treated it as having no body and an early close surfaced as ERR_STREAM_PREMATURE_CLOSE. With this PR the request is framed as chunked, so the server waits for the terminating chunk. That chunk never arrives, and the connection is reset instead:

GET / HTTP/1.1
Host: localhost:50988
Connection: keep-alive
Transfer-Encoding: chunked      <- added by this PR

Why I think the new code is correct

Once a request is declared chunked, aborting it mid-body is a transport-level failure, so ECONNRESET describes what actually happened. The old code only produced ERR_STREAM_PREMATURE_CLOSE because the request was unframed, which is the bug this PR fixes.

I ran the full parallel and sequential suites locally on arm64 macOS with no related failures.