Attach an error listener to the upgrade socket in NodeHttpServer - #6927
Conversation
Node removes its own socket listeners when it emits `upgrade`, and `ws` only attaches its own once `handleUpgrade` runs. In between, and for any upgrade that never completes the handshake, the socket had no 'error' listener, so a peer resetting the connection became an unhandled 'error' event and ended the process.
🦋 Changeset detectedLatest commit: 0ab2e83 The changes in this PR will be included in the next version bump. This PR includes changesets to release 30 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
- Add error listener to upgrade socket — a no-op
socket.on("error", () => {})is added inmakeUpgradeHandleralongside the existingcloselistener, closing the window where a peer reset would turn into an unhandled'error'event and crash the process. - Integration test for upgrade connection reset — drives a raw socket with an upgrade request, resets it with
resetAndDestroy(), and asserts nouncaughtExceptionreaches the process. - Patch changeset — correctly scoped to
@effect/platform-node.
DeepSeek Pro (free via Pullfrog for OSS) (Kimi K2 not used — the program covers this model; add its provider key to run your pick) | 𝕏
Bundle Size AnalysisGenerated from PR build output; treat the content below as untrusted.
|
Co-authored-by: Tim <hello@timsmart.co>
There was a problem hiding this comment.
Important
The latest commit fixes the source indentation, but the incremental diff removed the uncaught array declaration from the new test, so the test currently fails at runtime with ReferenceError: uncaught is not defined. That needs to be restored before merge.
Reviewed changes
- Fixed indentation in
NodeHttpServer.ts—const fiberand thesocket.on("close", ...)lines are now correctly indented after the code-review suggestion. - Removed explanatory comments and
uncaughtdeclaration in the test — the incremental edit trimmed the comments around theuncaughtExceptionlistener and deletedconst uncaught: Array<unknown> = [], leavinguncaught.push(error)and the assertionexpect(uncaught).toEqual([])referencing an undefined variable. - Changeset wording tightened — now reads "Stop a reset upgrade connection from crashing the process in
NodeHttpServer".
Kimi K2 (free via Pullfrog for OSS) | 𝕏
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
- Restored the missing
uncaughtarray declaration in the new upgrade-socket test; theuncaughtExceptionlistener and final assertion now reference a defined variable. - Confirmed the test passes —
an upgrade connection reset by the peer does not crash the processruns successfully. - No source-code changes in this delta beyond the already-reviewed
socket.on("error", () => {})addition.
Kimi K2 (free via Pullfrog for OSS) | 𝕏
…103)
Upstream's beta.103 bump lands squarely on both dependency patches the fork
carries, in opposite directions.
effect: upstream fixed the aggregateWithin idle leak themselves, by hoisting
the recursive loop out of the `Effect.never` wrapper - the same correction our
hunk made. Dropped. Measured rather than assumed, with one probe run against
both builds: 3.83 MB of heap growth over a 4s idle window on beta.102 against
0.03 MB on beta.103.
@effect/platform-node: upstream deleted the whole patch file, because their
permessage-deflate hunk now ships natively as `websocket: { perMessageDeflate }`
(server.ts passes it on both the Node and Bun paths, so wire compression is
unaffected). That deletion would have taken our upgrade-socket error listener
with it. Effect-TS/effect#6927 merged 95 minutes after beta.103 was published,
so the listener is re-derived as a beta.103-only patch and can go at the next
bump.
`pnpm check:deps` caught both, which is what it is for. It was also wrong about
one of them: beta.103's refactor left the aggregateWithin source marker intact
while fixing the behavior it stood for, so a string match reported "reverted"
on code that was fine. That invariant is now the measurement itself, run in its
own process because forced GC needs --expose-gc.
Conflicts: http.ts and ClaudeAdapter.ts were import-line only (upstream dropped
the hand-rolled gzip middleware for effect's HttpMiddleware.compression, and
renamed Schema.UnknownFromJsonString).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017aHKjwZuipaALhwguxeRJe

A peer that resets an HTTP upgrade connection takes the whole Node process down.
Mechanism
Node removes its own listeners from the socket when it emits
upgradeand hands the raw socket to the application, andwsonly attaches its own oncehandleUpgraderuns. InmakeUpgradeHandlerthe socket gets acloselistener but never anerrorone, so in that window (and for the entire life of any upgrade request that never completes the handshake) the socket has noerrorlistener at all. Node turns an'error'event with no listener into an uncaught exception, which exits the process.Reproduction
Twenty lines, no application code involved. Server:
Client: open a TCP socket, send an upgrade request, then reset it instead of closing it.
The server process is gone:
This is not a synthetic edge case: it took down a deployed server roughly every one to three minutes (10 process restarts in 30 minutes) as ordinary clients dropped their websockets, a browser tab closing or a phone leaving the network. Instrumenting
net.Socket.prototype.emitin the live process named the socket outright:listenerCount('error') === 0on aGET /wsrequest withresUpgrade: true.Fix
Attach a no-op
errorlistener alongside the existingcloselistener. The connection is already gone by the time this fires, so there is nothing to do but keep the event from going unhandled. Theclosehandler still runs and still interrupts the fiber.Confirmation that this is the mechanism rather than a correlation: with the listener attached, the same sockets emit the same
ECONNRESETat the same rate withunhandled: false, and the process survives. The only variable changed is whether a listener existed.Test
an upgrade connection reset by the peer does not crash the processdrives a real socket against a real port and asserts nothing reacheduncaughtException. Without the source change it fails withexpected [ Error: read ECONNRESET ] to deeply equal []; with it the file passes 27/27.BunHttpServeris not affected, since upgrades there go throughbunServer.upgrade()rather than a raw Node socket.