Review requested:

• @nodejs/http
• @nodejs/net
• @nodejs/streams

Codecov Report

❌ Patch coverage is 74.41860% with 11 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.28%. Comparing base (2ddade6) to head (dcbb130).
⚠️ Report is 5 commits behind head on main.

@@            Coverage Diff             @@
##             main   #64566      +/-   ##
==========================================
- Coverage   90.28%   90.28%   -0.01%     
==========================================
  Files         762      762              
  Lines      247646   247687      +41     
  Branches    46695    46705      +10     
==========================================
+ Hits       223596   223623      +27     
  Misses      15496    15496              
- Partials     8554     8568      +14     

... and 35 files with indirect coverage changes

• ❄️ 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.

Sorry I haven't reviewed this yet @Archkon. I think it does match the description before, but I did some digging and I think there's an existing bug with writableFinished, which means even though that's correct we can't enable this yet... We'll need to fix that as well to ship this. Demo:

'use strict';
const assert = require('assert');
const http = require('http');
const net = require('net');

// A server that never reads the request body and aborts the connection.
const server = net.createServer((socket) => socket.destroy());

server.listen(0, () => {
  const req = http.request({ port: server.address().port, method: 'POST' });
  let writeError = null;
  let finishEmitted = false;

  req.on('finish', () => { finishEmitted = true; });
  req.on('error', () => {});

  req.on('close', () => {
    server.close();

    // The write failed, so the body was never flushed to the socket.
    assert.strictEqual(writeError.code, 'EPIPE');

    assert.strictEqual(req.writableFinished, false,
                       'writableFinished is true after a failed write');
    assert.strictEqual(finishEmitted, false,
                       "'finish' was emitted after a failed write");
  });

  // 1 MiB, so the write cannot fit in the socket buffers and must fail.
  req.write(Buffer.alloc(1024 * 1024), (err) => { writeError = err; });
  req.end();
});

If you tweak your tests slightly you'll hit the same thing. This seems to be an existing bug, but it doesn't normally matter because error emits first and we don't base anything on writableFinished. Once we do, this will swallow write errors just like before. Do you have a little time to take a look?

I've opened a separate PR to fix writableFinished: #64847. Once that's merged, I think this will work correctly.

#64847 is now merging any second (just waiting for the commit queue). As soon as that's on main, you can rebase this and then hopefully the above example will now work correctly and we can get this merged too.

@Archkon This is quite confusingly written, so I'm not really clear what you mean... In future I'd suggest maybe providing examples or a larger explanation.

For now I can clarify the general invariant though, which might help: we want to preserve the error handling behaviour from 24.15.

For the current test, and any other test you can write, we should see the same error behaviour with this fix as we did in 24.15 before the regression.

If you find that the core fix behaves the same as 24.15 in every case for this issue then it's correct I think, we're happy with that original behaviour. We only need to change the core code if you find a difference between this and 24.15.

If the code is correct but the test here is failing, then that must mean the test was also failing in 24.15. Is that right? If so then yes, in that case we should fix the test.

Overall I think we need two tests (one for read errors and one for write errors) which both pass in 24.15, fail in 24.16, and then pass with this fix. They need a configuration that does that consistently on both Linux & Mac.

Does that help?