From b84b58ef218894873f6cabd8ee34dcad3c1e714d Mon Sep 17 00:00:00 2001 From: MHSanaei Date: Fri, 8 May 2026 00:00:10 +0200 Subject: [PATCH] fix(websocket): guard stale events and disconnect race in JS client Two subtle race conditions in the browser WebSocket client: 1. Stale-event clobber. When connect() is called while the old socket is in CLOSING state, the readyState guard falls through and a new socket is assigned to this.ws. The old socket's queued close event then nulls out this.ws, silently breaking send() until the next reconnect. Same risk for delayed open/error/message handlers. 2. Reconnect-after-disconnect. clearTimeout() does not cancel a callback that has already fired but whose macrotask has not yet run. If disconnect() lands in that window, the queued reconnect callback still calls #openSocket() and resurrects the connection. Every event handler now bails out if this.ws no longer points at the socket that fired the event, and the reconnect timer callback re-checks shouldReconnect before opening a new socket. Co-Authored-By: Claude Opus 4.7 --- web/assets/js/websocket.js | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/web/assets/js/websocket.js b/web/assets/js/websocket.js index 64dd2769..f10507d5 100644 --- a/web/assets/js/websocket.js +++ b/web/assets/js/websocket.js @@ -104,15 +104,25 @@ class WebSocketClient { } this.ws = socket; + // Every handler must check `this.ws !== socket` first. A previous socket + // can still fire events (especially `close`) after we've moved on to a + // new one — e.g. connect() called while the old socket is in CLOSING + // state. Without the guard, a stale close would null out the freshly + // opened socket and silently break send(). socket.addEventListener('open', () => { + if (this.ws !== socket) return; this.isConnected = true; this.reconnectAttempts = 0; this.#emit('connected'); }); - socket.addEventListener('message', (event) => this.#onMessage(event)); + socket.addEventListener('message', (event) => { + if (this.ws !== socket) return; + this.#onMessage(event); + }); socket.addEventListener('error', (event) => { + if (this.ws !== socket) return; // Browsers fire 'error' before 'close' on failure. We surface it for // consumers (so polling fallbacks can engage) but don't log every blip // — bad networks would flood the console otherwise. @@ -120,6 +130,7 @@ class WebSocketClient { }); socket.addEventListener('close', () => { + if (this.ws !== socket) return; this.isConnected = false; this.ws = null; this.#emit('disconnected'); @@ -196,6 +207,10 @@ class WebSocketClient { this.reconnectTimer = setTimeout(() => { this.reconnectTimer = null; + // clearTimeout doesn't cancel a callback that has already fired but + // whose macrotask hasn't run yet — re-check shouldReconnect here so + // disconnect() called in that window can't be overridden. + if (!this.shouldReconnect) return; this.#openSocket(); }, delay); }