fix(server): survive client resets on WebSocket upgrade sockets#4570
fix(server): survive client resets on WebSocket upgrade sockets#4570Sipixer wants to merge 1 commit into
Conversation
Any client that opens a WebSocket upgrade request to /ws and then resets
the connection takes the whole server process down. Node throws an
uncaught exception when a socket emits 'error' with no listener, and
@effect/platform-node's upgrade handler wires a 'close' listener on the
raw upgrade socket but never an 'error' one.
Authentication does not protect against this. The upgrade event fires
before the route runs, so a client whose credentials are rejected still
reaches the unguarded socket: a rejected connection that resets kills
the server just as effectively as an accepted one. Every in-flight
session dies with the process, and on a deployment exposed beyond
loopback this is remotely reachable without credentials.
node:events:497
throw er; // Unhandled 'error' event
Error: read ECONNRESET
at TCP.onStreamRead (node:internal/stream_base_commons:216:20)
Attach a no-op 'error' listener to every upgrade socket so a reset
degrades to an ordinary disconnect. Only the Node factory needs this;
the Bun path does not expose raw upgrade sockets.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
ApprovabilityVerdict: Approved This is a straightforward defensive bug fix that prevents server crashes when clients reset WebSocket connections during handshake. The change follows an existing pattern in the codebase, is minimal in scope, and includes comprehensive tests. You can customize Macroscope's approvability policy. Learn more. |
What Changed
Attach a no-op
'error'listener to every WebSocket upgrade socket, so a client reset degrades to an ordinary disconnect instead of killing the server process.Only the Node factory in
server.tsis touched. The Bun path goes throughBun.serveand never hands out raw upgrade sockets, so it needs no guard.Why
A client that sends an upgrade request to
/wsand then resets the connection takes the whole server down. Node throws an uncaught exception when a socket emits'error'with no listener, and@effect/platform-node@4.0.0-beta.78wires a'close'listener on the raw upgrade socket inmakeUpgradeHandlerbut never an'error'one:Authentication is not a barrier. The
'upgrade'event fires before the route runs, so a client whose credentials are rejected has already passed through the unguarded socket — a rejected connection that resets kills the server exactly like an accepted one.Every in-flight session dies with the process, not just the one whose socket reset.
Trigger conditions. Two have to hold together. Probed against an unpatched
0.0.29-nightly.20260725.899:Unread bytes in the receive buffer make the kernel emit RST instead of FIN, and that RST lands on the unlistened socket. The same probe against a bare connection, partial headers, a completed
GET /, aPOST /mcpand an SSEGET /mcpleaves the server up — only the upgrade path is affected.Impact. Unlikely from ordinary traffic: browsers read their response and close cleanly, and a three-week journal on my deployment shows zero occurrences from normal client churn. The crashes I observed were self-inflicted by a scripted client that ignored the response. But it takes about ten lines and no credentials to trigger deliberately and repeatedly, so on any deployment listening beyond loopback it is an availability problem rather than a rare glitch.
Upstream. The gap is in
@effect/platform-nodeand arguably belongs there too. A guard here is still worth having: three lines, holds regardless of what upstream does, and keeps server liveness independent of a transitive dependency's socket handling.Checklist
Tests
apps/server/src/upgradeSocketGuard.test.tscovers that the listener is attached to every upgrade socket, and that ten consecutive resets leave the server answering requests with nouncaughtException.apps/serversuite green (1622 passed, 7 skipped)vp checkreports 0 errors (remaining 11 warnings are pre-existing inapps/web)vpr typecheckfailures are limited toscripts/lib/resolve-catalog.tsand reproduce on a clean checkoutvp packand re-ran the probe againstdist/bin.mjs: 20 consecutive resets on the previously fatal case, server alive, nothing fatal loggedNegative control: with the guard removed, the same sequence produces
UNCAUGHT: ECONNRESETand exit code 42.