fix(server): handle ECONNRESET on the WS upgrade socket#22556
Open
sashanicolas wants to merge 2 commits into
Open
fix(server): handle ECONNRESET on the WS upgrade socket#22556sashanicolas wants to merge 2 commits into
sashanicolas wants to merge 2 commits into
Conversation
The HMR WebSocket upgrade handler delegates straight to wss.handleUpgrade
without attaching an 'error' listener on the raw socket first. If the
client RSTs the TCP connection during the upgrade handshake (e.g. a
browser tab closing while vite is pushing an HMR update), the underlying
socket emits 'error' with ECONNRESET while no listener is attached yet,
and Node crashes the dev server with:
Error: read ECONNRESET
at TCP.onStreamRead (node:internal/stream_base_commons:216:20)
Emitted 'error' event on Socket instance at ...
This bug is exclusively in vite/ws.ts, but several downstream forks have
hit the same stack trace and patched around it locally — see
cloudflare/workers-sdk#12047, cloudflare/vinext#905,
livestorejs/livestore#982, nuxt/cli#994, cypress-io/cypress#28688.
The fix attaches a logger-backed error listener on the raw socket
before calling wss.handleUpgrade. The ws library attaches its own as
part of the handshake, so the two coexist for the remainder of the
connection lifetime. The added unit test asserts that vite's listener
is present alongside the ws library's after the upgrade event.
The previous commit logged every error on the upgrade socket, including the benign client-disconnect codes (ECONNRESET, EPIPE) that fire on normal tab close / page navigation. This broke playground/ssr's 'should restart ssr' test, which asserts the server logs contain no 'error' string after restart, and added noise to legitimate test runs (see playground/ssr-deps and ssr-noexternal output in CI). The client is already gone by the time the error fires, so there's no actionable recovery — silently absorb the benign codes and only log unexpected ones. The unit test still passes because it counts socket 'error' listeners, not log output.
Author
|
The two red checks on this PR look pre-existing and unrelated to the diff:
Happy to rebase, narrow the patch, or address anything specific if reviewers spot a real concern in the diff. |
sapphi-red
added
p2-edge-case
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Vite's HMR upgrade handler delegates straight to
wss.handleUpgradewithout attaching an'error'listener on the raw socket. If a client RSTs the TCP connection during the upgrade handshake (e.g. a browser tab closing mid-HMR), the underlying socket emits'error'withECONNRESETwhile no listener is attached yet, and Node aborts the dev server with:Evidence
The gap is in
packages/vite/src/node/server/ws.ts(the upgrade handler funnels throughhandleUpgrade, which callswss.handleUpgradedirectly). Several downstream forks have hit the same stack trace and patched around it locally:coupleWebSocketPR #12007 closed a related crash class on the
wss'connection'path, but the upgrade-window path was missed.Fix
Attach a logger-backed
'error'listener on the raw socket before callingwss.handleUpgrade. Thewslibrary attaches its own as part of the handshake (websocket-server.jsline 239), so the two coexist for the rest of the connection lifetime.Test
Added a unit test in
packages/vite/src/node/server/__tests__/ws.spec.tsthat opens an upgrade connection and asserts the socket has at least 2'error'listeners after the upgrade event (vite's + ws library's). The test deterministically fails without the fix and passes with it.The full race condition requires precise timing and is hard to trigger on demand, which is why the test verifies the safety net is present rather than trying to provoke the crash.
Notes
packages/vitepass (756 tests, 4 skipped).