From 3fbde607422dfbb29dd3eec37eb7e5ef4ab2ddda Mon Sep 17 00:00:00 2001 From: Johannes Millan Date: Wed, 20 May 2026 15:38:26 +0200 Subject: [PATCH] refactor(supersync): address round-2 review of WS reconnect cooldown fix MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Replace the `client.refusedChallengers = 0` post-summary reset with a dedicated `summaryLogged: boolean` flag. The reset muddied the field's documented "lifetime count" semantics for any future reader observing the closing socket. The flag dedupes the `ws.on('close')` re-entry without lying about the count. - Refresh the stale `RECONNECT_COOLDOWN_MS` doc that still described the non-sliding semantics from before fbdedf597e. - Short-circuit the WS-429 path normalize on `statusCode === 429` in setErrorHandler so the split+replace doesn't run on the 99%+ of error responses that aren't 429. - Drop the redundant `cid.length > 0` guard in `isValidClientId` — the trailing regex already requires `+` (≥1 char). - Route the websocket.routes spec simulator through `isValidClientId` so it can't drift from production validation order again. - Add a heartbeat-path regression test for the storm-summary dedup (the explicit-eviction path was already covered by fbdedf597e). --- packages/super-sync-server/src/server.ts | 8 +++-- .../services/websocket-connection.service.ts | 33 ++++++++++++------- .../super-sync-server/src/sync/sync.const.ts | 6 ++-- .../websocket-connection.service.spec.ts | 29 ++++++++++++++++ .../tests/websocket.routes.spec.ts | 12 +++---- 5 files changed, 65 insertions(+), 23 deletions(-) diff --git a/packages/super-sync-server/src/server.ts b/packages/super-sync-server/src/server.ts index 9aaa1c42a3..3888479ca6 100644 --- a/packages/super-sync-server/src/server.ts +++ b/packages/super-sync-server/src/server.ts @@ -159,9 +159,11 @@ export const createServer = ( // would flood the log without adding info. Downgrade those to debug. // Exact-match the WS route (allowing trailing slash + querystring) so // future sibling routes like /api/sync/ws-status do not silently - // inherit the debug-only behavior. - const path = req.url.split('?', 1)[0].replace(/\/+$/, ''); - const isWsRateLimit = statusCode === 429 && path === '/api/sync/ws'; + // inherit the debug-only behavior. statusCode gate short-circuits the + // path-normalize on the ~99% of error responses that aren't 429. + const isWsRateLimit = + statusCode === 429 && + req.url.split('?', 1)[0].replace(/\/+$/, '') === '/api/sync/ws'; if (statusCode >= 500) { Logger.error(logMessage, error.stack); } else if (isWsRateLimit) { diff --git a/packages/super-sync-server/src/sync/services/websocket-connection.service.ts b/packages/super-sync-server/src/sync/services/websocket-connection.service.ts index f48c6c9958..44b9aead9a 100644 --- a/packages/super-sync-server/src/sync/services/websocket-connection.service.ts +++ b/packages/super-sync-server/src/sync/services/websocket-connection.service.ts @@ -17,6 +17,14 @@ interface ConnectedClient { cooldownUntil: number; /** Count of challengers refused during this incumbent's lifetime. */ refusedChallengers: number; + /** + * Set true after `removeConnection` emits the storm-summary INFO. Guards + * against the inevitable `ws.on('close')` re-entry (triggered by + * removeConnection's own ws.close — eviction, heartbeat, closeAll) firing + * the summary a second time. Preferred over zeroing `refusedChallengers` + * because the count remains observably truthful for the life of the object. + */ + summaryLogged: boolean; } /** @@ -43,13 +51,15 @@ export class WebSocketConnectionService { /** Close code sent to a challenger socket refused during the reconnect cooldown */ private static readonly RECONNECT_COOLDOWN_CLOSE_CODE = 4008; /** - * If a still-OPEN socket for this clientId was accepted less than this long - * ago, a new socket from the same clientId is refused (the incumbent is kept, - * NOT evicted). Breaks the shared-clientId reconnect storm from pre-18.6.0 - * clients that reconnect immediately on the 4009 eviction: the incumbent is - * never evicted, so the server stops emitting 4009 and the loop loses its - * fuel. A genuinely dead/closing incumbent bypasses this (see addConnection), - * so a real network-blip reconnect still recovers within the cooldown. + * Sliding-window cooldown. While a still-OPEN incumbent's `cooldownUntil` is + * in the future, a new socket from the same clientId is refused (the + * incumbent is kept, NOT evicted) and `cooldownUntil` is extended by another + * RECONNECT_COOLDOWN_MS. Eviction only resumes after this long of QUIET (no + * challengers). Breaks the shared-clientId reconnect storm from pre-18.6.0 + * clients that reconnect immediately on the 4009 eviction: under sustained + * load the gate never expires, so the server stops emitting 4009 and the + * loop loses its fuel. A genuinely dead/closing incumbent bypasses this (see + * addConnection), so a real network-blip reconnect still recovers. */ private static readonly RECONNECT_COOLDOWN_MS = 5_000; @@ -145,6 +155,7 @@ export class WebSocketConnectionService { connectedAt: nowMs, cooldownUntil: nowMs + WebSocketConnectionService.RECONNECT_COOLDOWN_MS, refusedChallengers: 0, + summaryLogged: false, }; userSet.add(client); @@ -200,14 +211,14 @@ export class WebSocketConnectionService { // Storm summary: the first refusal logged a WARN; the rest were silent. // When the incumbent finally goes away, log the cumulative count so the // operator sees the scale of the storm without per-attempt log spam. - // Zero the counter so the eventual `ws.on('close')` re-entry (triggered by - // our own ws.close below) does not double-log this summary. - if (client.refusedChallengers > 0) { + // `summaryLogged` guards against the inevitable `ws.on('close')` re-entry + // (triggered by our own ws.close below) double-logging. + if (client.refusedChallengers > 0 && !client.summaryLogged) { const incumbentLifetimeMs = Date.now() - client.connectedAt; Logger.info( `[ws:user:${userId}:${client.clientId}] Refused ${client.refusedChallengers} reconnect challenger(s) over ${incumbentLifetimeMs}ms incumbent lifetime before removal`, ); - client.refusedChallengers = 0; + client.summaryLogged = true; } // Close the WebSocket if still open if ( diff --git a/packages/super-sync-server/src/sync/sync.const.ts b/packages/super-sync-server/src/sync/sync.const.ts index 0fed549a0e..7d93c55ce8 100644 --- a/packages/super-sync-server/src/sync/sync.const.ts +++ b/packages/super-sync-server/src/sync/sync.const.ts @@ -11,12 +11,12 @@ export { /** * Type-guard for clientId validation. Order matters: cheap length check first * so an attacker passing a multi-megabyte clientId is rejected before the - * regex scans it. Used by the WS route handler AND the rate-limit - * keyGenerator — keep both call sites in sync via this helper. + * regex scans it. The regex already requires `+` (≥1 char), so an explicit + * non-empty check is redundant. Used by the WS route handler AND the + * rate-limit keyGenerator — keep both call sites in sync via this helper. */ export const isValidClientId = (cid: unknown): cid is string => typeof cid === 'string' && - cid.length > 0 && cid.length <= SUPER_SYNC_MAX_CLIENT_ID_LENGTH && SUPER_SYNC_CLIENT_ID_REGEX.test(cid); diff --git a/packages/super-sync-server/tests/websocket-connection.service.spec.ts b/packages/super-sync-server/tests/websocket-connection.service.spec.ts index eb7ff4ad07..a85256813d 100644 --- a/packages/super-sync-server/tests/websocket-connection.service.spec.ts +++ b/packages/super-sync-server/tests/websocket-connection.service.spec.ts @@ -323,6 +323,35 @@ describe('WebSocketConnectionService', () => { expect(summaryCalls).toHaveLength(1); }); + it('should emit the storm summary exactly once when an incumbent with refusals is reaped by the heartbeat', () => { + // Regression: heartbeat-dead-connection removal also calls + // removeConnection -> ws.close -> 'close' event -> removeConnection + // re-entry. summaryLogged must dedupe that path too. + const incumbent = createMockWs(); + service.addConnection(1, 'client-a', incumbent as any); + + for (let i = 0; i < 4; i++) { + const challenger = createMockWs(); + service.addConnection(1, 'client-a', challenger as any); + } + vi.mocked(Logger.info).mockClear(); + + service.startHeartbeat(); + // Two ping cycles with no pong → heartbeat removes the incumbent. + vi.advanceTimersByTime(30_000); + vi.advanceTimersByTime(30_000); + // The heartbeat's ws.close() would in real ws fire the close handler, + // re-entering removeConnection. + incumbent._emitClose(); + + const summaryCalls = vi + .mocked(Logger.info) + .mock.calls.filter((c) => + String(c[0]).includes('Refused 4 reconnect challenger(s)'), + ); + expect(summaryCalls).toHaveLength(1); + }); + it('should not emit a storm summary when no challengers were refused', () => { const ws = createMockWs(); service.addConnection(1, 'client-a', ws as any); diff --git a/packages/super-sync-server/tests/websocket.routes.spec.ts b/packages/super-sync-server/tests/websocket.routes.spec.ts index 3d7102c23c..52a939e331 100644 --- a/packages/super-sync-server/tests/websocket.routes.spec.ts +++ b/packages/super-sync-server/tests/websocket.routes.spec.ts @@ -1,5 +1,9 @@ import { describe, it, expect, beforeEach, vi } from 'vitest'; -import { CLIENT_ID_REGEX, MAX_CLIENT_ID_LENGTH } from '../src/sync/sync.const'; +import { + CLIENT_ID_REGEX, + MAX_CLIENT_ID_LENGTH, + isValidClientId, +} from '../src/sync/sync.const'; import { WS_CONNECTION_RATE_LIMIT_MAX, WS_CONNECTION_RATE_LIMIT_WINDOW, @@ -59,11 +63,7 @@ async function simulateWsHandler( return 'rejected'; } - if ( - !clientId || - !CLIENT_ID_REGEX.test(clientId) || - clientId.length > MAX_CLIENT_ID_LENGTH - ) { + if (!isValidClientId(clientId)) { socket.close(4001, 'Invalid clientId'); return 'rejected'; }