test(uws): fix macOS EPIPE flake in the UDS 413 test - #2023
Conversation
…client
The 413-over-limit test posts an 8KB body against a 1KB maxBodyBytes server.
uWS correctly responds early and closes the connection without draining the
rest of the body; on macOS the small default UDS send-buffer size means the
client's outstanding write is still in flight when the socket closes, so it
completes with EPIPE after the response has already resolved.
Node's http client normally forwards any socket error straight to `req`'s
own 'error' event — except for the window right after a keep-alive response
finishes, where the client detaches its listener before handing the socket
back to the agent's free-connection pool (`responseKeepAlive` in Node's
`_http_client.js`, which carries its own upstream `// TODO(ronag): ... the
socket has no 'error' handler` comment on exactly this gap). A write that
fails in that window is unhandled and becomes an uncaught exception
attributed to whichever test happens to be running.
Confirmed against the real createUwsServer on Linux, not just inferred: with
the pre-fix helper, the exact 8KB-body/1KB-cap fixture this test already
uses rejects with a raw EPIPE roughly half the time (40 trials); with this
fix, 40/40 resolve cleanly to 413. So this isn't purely a macOS-only symptom
being tolerated blind — the same race is reproducible and now verifiably
fixed on the platform CI actually runs, using the test's existing fixture
size, no artificial inflation needed.
Fix udsRequest (the shared helper used by every test in this file):
- `agent: false` per request, so the pooled-connection gap above never
opens — this is the primary fix.
- A socket-level 'error' listener retained as a second layer, tolerating
EPIPE/ECONNRESET and otherwise routing to the same settle-once reject
path as every other error channel. Node's own forwarding to `req` always
sees a socket error first and settles the promise a moment before this
listener also observes the same event, so a `settledThisTick` flag
(cleared on the next microtask) distinguishes that expected echo from a
genuinely late arrival worth recording. A late, non-tolerated error
can't reject an already-settled promise either way, so it's collected in
`udsRequestWarnings` and asserted empty in this file's `after`, rather
than silently absorbed.
- Reject on 'close' if the promise never settled at all, so a response
that commits headers and then the connection aborts mid-body fails with
a diagnosable error instead of hanging until Mocha's timeout.
- A new test for a connection failure before any response.
Nine rounds of independent pre-push review (codex/gemini/grok + a Harper-
domain pass) drove this design; several earlier versions each reintroduced a
variant of the exact duplicate-error/uncaught-exception bug this change
exists to remove. No server behavior changes.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Reviewed; no blockers found. |
There was a problem hiding this comment.
Code Review
This pull request refactors the udsRequest helper in uwsServer.test.js to use agent: false and adds robust handling for socket-level errors (such as EPIPE and ECONNRESET races) to prevent uncaught exceptions and test flakes on macOS. It also adds a new test verifying that connection failures before a response are rejected with the underlying error. The reviewer noted that req.on('error', settleReject) will still reject the promise on EPIPE or ECONNRESET because Node's http.ClientRequest forwards socket errors to the request object. A suggestion was provided to guard against these error codes in the request-level error listener as well.
| settleReject(err); | ||
| }); | ||
| }); | ||
| req.on('error', settleReject); |
There was a problem hiding this comment.
Because Node's http.ClientRequest internally forwards socket errors to the request object, any socket-level EPIPE or ECONNRESET error will be forwarded and emitted as an 'error' event on req. Since req.on('error', settleReject) is registered directly, it will synchronously invoke settleReject and reject the promise with the raw EPIPE/ECONNRESET error before the response can be fully read/processed (if there is a race).
To prevent this and ensure that EPIPE and ECONNRESET are always tolerated (allowing either the response to finish or the 'close' event to reject with a descriptive error), we should also guard against these error codes in the req.on('error') listener.
| req.on('error', settleReject); | |
| req.on('error', (err) => { | |
| if (err.code === 'EPIPE' || err.code === 'ECONNRESET') return; | |
| settleReject(err); | |
| }); |
What / why
uWS UDS adapter (createUwsServer) rejects a body over maxBodyBytes with 413flaked on macOS: the 413 assertion passed, but an uncaughtEPIPElanded afterward and mocha attributed it to the test, failing the run (green on Linux CI).Root cause: the server correctly responds 413 and closes early without draining the rest of an over-limit body. The client's outstanding write can still be in flight when the socket closes, and Node's http client stops forwarding socket errors to the request in the window right after a keep-alive response completes (
responseKeepAlivein Node's own_http_client.js, which carries an upstream// TODO(ronag): ... the socket has no 'error' handlercomment on exactly this gap) — so the late write failure becomes an uncaught exception with nothing listening.Confirmed this reproduces on Linux too, not just inferred as macOS-only: against the real
createUwsServer, the pre-fix helper rejects with a rawEPIPEon roughly half of 40 trials using the test's existing 8KB-body/1KB-cap fixture (no artificial size inflation needed). With the fix, 40/40 resolve cleanly to 413.Fix (in the shared
udsRequesttest helper, so it protects every test in the file, not just the 413 case):agent: falseper request — the primary fix, avoids the pooled-connection gap entirely.errorlistener as a second layer, tolerating onlyEPIPE/ECONNRESET; asettledThisTickflag distinguishes Node's synchronous same-event echo (already handled viareq's own forwarding) from a genuinely late arrival worth recording. Anything else post-settlement is collected and asserted empty inafter(), rather than silently absorbed.closeif the promise never settled, so a truncated response fails with a diagnosable error instead of hanging to Mocha's timeout.No changes to
server/serverHelpers/uwsServer.ts— the server's behavior (respond early, don't drain an over-limit body) is correct per HTTP and is preserved untouched.Verification
npx mocha unitTests/server/serverHelpers/uwsServer.test.js: 21/21 passing, stable across repeated runs, noMaxListenersExceededWarning.unitTests/server/**/*.test.js: 679 passing.200 !== 413), then reverted.Review
Nine rounds of independent pre-push review (Codex/Gemini/Grok + a Harper-domain pass) drove the final design — several earlier iterations each reintroduced a variant of the exact duplicate-error/uncaught-exception bug this change removes (a competing listener installed too early causing double delivery, then a module-global listener-ownership slot that orphaned a socket when the file's second UDS connection was opened). Final round: verdict LGTM (Codex), no findings from Grok; Gemini's remaining concern (pre-response EPIPE could still defeat the fix) was tested directly against the real server and did not reproduce in 60 combined trials.
🤖 Generated with Claude Code