Repository navigation
test(artifacts): open a connection per raw request - #259
Conversation
Node before 24.20 emits 'finish' on a request whose body write failed, so a keep-alive agent takes the socket back and removes its error listener, and the write's EPIPE then goes unhandled (nodejs/node#64847). The oversized-body test hit it on runners that still cache Node 24.19 whenever the service closed the connection before the body was written. A connection per request keeps the error on the request. The new test reproduces it on Node 24.19 every time.
amigoer
left a comment
amigoer
left a comment
There was a problem hiding this comment.
The cause checks out: at v24.19.0, onFinish in _http_outgoing.js ignores the write error and emits 'finish', requestOnFinish then calls responseKeepAlive, which removes socketErrorListener, and the agent adds no 'error' listener of its own; nodejs/node#64847 shipped in 24.20.0 and Electron 44.9.2 runs Node 24.22.0, and no product code uses the node:http client. Without agent: false the new test fails on Node 24.19.0 in 3 of 3 runs with the CI stack and passes on 24.22.0; with it the file passes 10 of 10 runs on 24.19.0. The Release run passed on all six runners, and macos-15-intel ran Node 24.19.0 there with no unhandled error.
| const req = http.request(url, { | ||
| method: options.method ?? 'POST', | ||
| headers: options.headers, | ||
| agent: false, |
There was a problem hiding this comment.
agent: false also makes Node send Connection: close, so the service's server closes the socket as soon as it has answered instead of draining the body. The oversized-body test (line 287) now often gets the reset before the 413: it saw 413 in 18 of 40 local runs, against 40 of 40 before this change, so the 413 is checked only about half the time. The SDK refuses on the declared length before reading anything, so that request can send only the headers, headers(token, { 'Content-Length': String(BODY_BYTES + 1) }) with no body, and assert toBe(413); that got 413 in 100 of 100 runs on Node 24.19 and 24.22. It still needs agent: false, or a kept-alive socket would hand the unsent body to the next request.
There was a problem hiding this comment.
Fixed in 79e1dfe: the oversized request now declares BODY_BYTES + 1 and sends no body, so the test checks a 413 on every run (15 runs each on Node 24.19 and 24.22). agent: false stays.
| await new Promise<void>((resolve) => server.listen(0, '127.0.0.1', resolve)) | ||
| cleanups.push(() => new Promise<void>((resolve) => server.close(() => resolve()))) | ||
| const { port } = server.address() as net.AddressInfo | ||
| const reply = await send(`http://127.0.0.1:${port}/`, { body: 'x'.repeat(16 * 1024 * 1024) }) |
There was a problem hiding this comment.
Nit: the size matters. With a 1 MiB body the test no longer fails on Node 24.19 without the fix, because the loopback buffers take the whole write before the close. A few words on why 16 MiB would keep someone from shrinking it.
There was a problem hiding this comment.
Fixed in 79e1dfe: a comment says the body must outgrow the loopback buffers.
With a connection per request the service closes it right after the 413, so a real oversized body was often reset before the answer and the test checked the status only about half the time. The service refuses a declared length before reading, so none of the body is sent now and the 413 is checked every run.
The oversized-body test failed on macOS Intel with an unhandled EPIPE because Node before 24.20 lets a keep-alive agent take back a socket whose body write failed after the answer (nodejs/node#64847), and those runners still cache Node 24.19; the raw-request helper now opens a connection per request, and a new test reproduces the bug.
Closes #243
Checked on a verified Node 24.19.0 binary: the new test fails with the CI stack before the change and the service tests pass ten runs in a row after it;
pnpm testpasses on Node 24.22, and a Release run on this branch covers the six runners.