http: fix just-merged change that could silently truncate responses

[pimterry]

This fixes an issue introduced 2 days ago in #64278. With that change, errors that emit after HTTP response headers are received but before the response is completed are swallowed silently.

This PR preserves the core change there, but covers the "response set but not yet completed" window properly so errors here still fire as expected.

[nodejs-github-bot]

Review requested:

• @nodejs/http
• @nodejs/net

[github-actions]

Fast-track has been requested by @pimterry. Please 👍 to approve.

[bjohansebas]

LGTM!

[mcollina]

lgtm

[mcollina]

I'm also ok to revert #64278.

[nodejs-github-bot]

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

[pimterry]

I'm also ok to revert

I think we can roll forwards instead. The issue is real, and the fix is roughly correct, it just misses a couple of cases en route. AFAICT with some small changes this'll work as intended.

That said, I say "cases" plural because I've found another one: if the response is completed but the request is still going, this will now swallow errors we previously emitted on the still open request.

That can happen because request/response lifetimes are surprisingly independent, servers can finish responding while the request is still going. Most plausible example: the server could send an HTTP 202 to confirm an upload based on headers alone, before the body is fully delivered, expecting the request to keep writing it out even though the response side is closed. It's definitely weird, but it's valid HTTP AFAICT and I wouldn't be surprised at all if somebody does it and expects to get errors if it goes wrong.

Bonus fix for that incoming now.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.23%. Comparing base (3ac95f2) to head (30901e2).
⚠️ Report is 3 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #64507      +/-   ##
==========================================
- Coverage   90.23%   90.23%   -0.01%     
==========================================
  Files         741      739       -2     
  Lines      241692   241668      -24     
  Branches    45541    45547       +6     
==========================================
- Hits       218097   218073      -24     
- Misses      15113    15152      +39     
+ Partials     8482     8443      -39     

Files with missing lines	Coverage Δ
lib/_http_client.js	97.63% <100.00%> (-0.01%)	⬇️

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

[Archkon]

I need to metion that, I do these kind of check in my first version of commit ,but it would failed to pass macos tests and it seems that the macos platform has a bit difference.I'm sorry for that I don't deep dive this beacuse I don't have macos device to check the operating system real behavior. I hope this could be a kind of helpful reminder for you. I just think it maybe not easily to fix just check the req status

[Archkon]

how about replace using this ?

Suggested change

	assert.strictEqual(err.code, 'ECONNRESET');
	assert.strictEqual(err.syscall, 'write');
	switch (err.code) {
	case 'ECONNRESET':
	case 'ECONNABORTED':
	case 'EPIPE':
	break;
	default:
	assert.fail(`Unexpected error code ${err.code}`);
	}

[pimterry]

@Archkon I'm assuming from your comments on #64511 that you're happy to open a new PR for this once it's reverted? That would be great if so, I'll close this once that's open.

Just for reference, I think there's a few key things to cover:

• The original test was covering useful behaviour, but not the exact problem from the reported issue: it fails on the affected versions and previous Node versions (e.g. v24.15). I tried to tweak it further here, but I'm not sure the current state is right either. The test for this should cover the actual issue if possible, meaning it should pass on v24.15, fail on v24.16, and then pass with the fix applied.
• I think if read or write are still incomplete in any way (read not completed or writable not flushed) we do need to expose the error. Any failure at that stage is a real problem that should be exposed.
• The MacOS difference seems to be due to flushing timing. writableFinished resolves when the OS confirms that the full write has been flushed (as opposed to Ended which resolves immediately, but doesn't actually confirm the data was sent). The flush happens async and timing depends on OS & network behaviour. It is the right gate I think, but the test here was still wrong, it was a quick fix on my part and it needs more thought. That needs fixing, but I don't think the error code is the issue - the failing test isn't supposed to error at all! I think the problem is that the server doesn't read the request body, so the write doesn't fully flush in some environments - req.resume() on the server might resolve that.

I think the correct logic for the code is now clear enough, the tricky part is writing tests that carefully wait for the write events so they properly cover the original issue, and also validate the various different cases here. It turns out it's quite a complex set of behaviours! In terms of testing it, see what you can come up with to try to fix this, and then open the PR and we can run CI manually to validate it there.

Thanks for your work on this. Sorry for all the back and forth but hopefully we'll get something working nicely soon. Feel free to ping if you have questions or want any help.

[Archkon]

@Archkon I'm assuming from your comments on #64511 that you're happy to open a new PR for this once it's reverted? That would be great if so, I'll close this once that's open.

Just for reference, I think there's a few key things to cover:

• The original test was covering useful behaviour, but not the exact problem from the reported issue: it fails on the affected versions and previous Node versions (e.g. v24.15). I tried to tweak it further here, but I'm not sure the current state is right either. The test for this should cover the actual issue if possible, meaning it should pass on v24.15, fail on v24.16, and then pass with the fix applied.
• I think if read or write are still incomplete in any way (read not completed or writable not flushed) we do need to expose the error. Any failure at that stage is a real problem that should be exposed.
• The MacOS difference seems to be due to flushing timing. writableFinished resolves when the OS confirms that the full write has been flushed (as opposed to Ended which resolves immediately, but doesn't actually confirm the data was sent). The flush happens async and timing depends on OS & network behaviour. It is the right gate I think, but the test here was still wrong, it was a quick fix on my part and it needs more thought. That needs fixing, but I don't think the error code is the issue - the failing test isn't supposed to error at all! I think the problem is that the server doesn't read the request body, so the write doesn't fully flush in some environments - req.resume() on the server might resolve that.

I think the correct logic for the code is now clear enough, the tricky part is writing tests that carefully wait for the write events so they properly cover the original issue, and also validate the various different cases here. It turns out it's quite a complex set of behaviours! In terms of testing it, see what you can come up with to try to fix this, and then open the PR and we can run CI manually to validate it there.

Thanks for your work on this. Sorry for all the back and forth but hopefully we'll get something working nicely soon. Feel free to ping if you have questions or want any help.

@pimterry
I will but I don't completely understand that what do you mean by
the failing test isn't supposed to error at all

As you have said previously
I think if read or write are still incomplete in any way (read not completed or writable not flushed) we do need to expose the error. Any failure at that stage is a real problem that should be exposed.

and the test code is that

    assert.strictEqual(response.complete, true);
    assert.strictEqual(req.writableFinished, false);
    assert.strictEqual(err.code, 'ECONNRESET');

so why this should't be trigger at all ?

[pimterry]

I will but I don't completely understand that what do you mean by the failing test isn't supposed to error at all

Ah, there are at least two failing tests, that's the confusion. macOS fails on that error test, but aarch64-darwin fails here in the test-http-client-complete-response-reset.js test that shouldn't error at all, that's what I meant.

For the error test case, yes being flexible on the error codes if they're all reasonable is OK I think.

[Archkon]

I will but I don't completely understand that what do you mean by the failing test isn't supposed to error at all

Ah, there are at least two failing tests, that's the confusion. macOS fails on that error test, but aarch64-darwin fails here in the test-http-client-complete-response-reset.js test that shouldn't error at all, that's what I meant.

For the error test case, yes being flexible on the error codes if they're all reasonable is OK I think.

Thanks for clarify that but may i ask why you could tell that this would be casued by writableFinished ? not the complete ? I just try run test on my fork repo but just spot that
https://github.com/Archkon/node/actions/runs/29435487178/job/87420971349
the writableFinished would be true but complete would be undefined ?