From 2071bfda7fd632bd452f207fb4e9921003e41add Mon Sep 17 00:00:00 2001 From: Mohamed Boudra Date: Sun, 8 Nov 2026 10:00:11 +0700 Subject: [PATCH] fix(client): close abandoned browser connections immediately (#9067) * fix(client): close abandoned browser connections immediately When the client gave up on a connection (liveness timeout, connect timeout, reconnect, subscription failure) it closed the WebSocket with 1001. Browsers, Electron and Node's global WebSocket accept only 1000 or 3000-4999 and throw InvalidAccessError otherwise; the throw was swallowed and the reference dropped, so the physical socket stayed open and the daemon kept streaming into it until its 45 s lease expired. The WebSocket adapter is the only code that calls WebSocket.close, so it now sends 1000 for codes the API rejects and trims reasons to the 123-byte limit. Callers keep their reasons, and Node's `ws` sends the same frame as a browser, so Node tests now catch this class of bug. * test(client): drop timing-dependent hello count from abandoned-close test Handshake retries after close are owned and tested by the relay's handshake-lifetime test; this test only checks that the abandoned socket closes. --- .../src/daemon-client-transport.test.ts | 104 ++++++++++++++++++ .../src/daemon-client-websocket-transport.ts | 28 ++++- 2 files changed, 131 insertions(+), 1 deletion(-) diff --git a/packages/client/src/daemon-client-transport.test.ts b/packages/client/src/daemon-client-transport.test.ts index c95c534129..b9ea5b0454 100644 --- a/packages/client/src/daemon-client-transport.test.ts +++ b/packages/client/src/daemon-client-transport.test.ts @@ -1,5 +1,9 @@ import { EventEmitter } from "node:events"; import { describe, expect, test, vi } from "vitest"; +import { WebSocketServer, type WebSocket as ServerSocket } from "ws"; +import { exportPublicKey, generateKeyPair } from "@getpaseo/relay/e2ee"; +import { DaemonClient } from "./daemon-client.js"; +import { defaultWebSocketFactory } from "./daemon-client-websocket-transport.js"; import { createEncryptedTransport, createWebSocketTransportFactory, @@ -190,3 +194,103 @@ test("native drain capability survives WebSocket and encryption wrappers", async }); expect(encrypted.supportsDrain).toBe(false); }); + +interface ObservedSocket { + closeFrame: { code: number; reason: string } | null; +} + +/** A daemon endpoint that accepts sockets and never answers, so every attempt times out. */ +async function silentDaemon() { + const server = new WebSocketServer({ host: "127.0.0.1", port: 0 }); + await new Promise((resolve) => server.once("listening", () => resolve())); + const sockets: ObservedSocket[] = []; + server.on("connection", (socket: ServerSocket) => { + const observed: ObservedSocket = { closeFrame: null }; + sockets.push(observed); + socket.on("close", (code, reason) => { + observed.closeFrame = { code, reason: reason.toString() }; + }); + }); + const address = server.address(); + if (!address || typeof address === "string") throw new Error("Missing port"); + return { + url: `ws://127.0.0.1:${address.port}/ws`, + sockets, + openSockets: () => sockets.filter((entry) => entry.closeFrame === null), + stop: () => + new Promise((resolve) => { + for (const client of server.clients) client.terminate(); + server.close(() => resolve()); + }), + }; +} + +// Node's global WebSocket applies the WHATWG close rules Chromium and Electron apply: +// close() accepts only 1000 or 3000-4999 and a reason of at most 123 UTF-8 bytes. +describe("abandoned connections close their WebSocket", () => { + test.each([ + { name: "direct", e2ee: false }, + { name: "relay E2EE", e2ee: true }, + ])( + "$name: a timed-out attempt sends a close frame before the client reconnects", + async ({ e2ee }) => { + const daemon = await silentDaemon(); + const client = new DaemonClient({ + url: daemon.url, + clientId: "cid_abandoned_close_test", + webSocketFactory: defaultWebSocketFactory, + connectTimeoutMs: 1_500, + reconnect: { enabled: true, baseDelayMs: 50, maxDelayMs: 50 }, + logger: { debug() {}, info() {}, warn() {}, error() {} }, + ...(e2ee + ? { + e2ee: { + enabled: true, + daemonPublicKeyB64: exportPublicKey(generateKeyPair().publicKey), + }, + } + : {}), + }); + try { + void client.connect().catch(() => {}); + await vi.waitFor(() => expect(daemon.sockets.length).toBeGreaterThanOrEqual(2), { + timeout: 4_000, + }); + const [abandoned] = daemon.sockets; + await vi.waitFor(() => expect(abandoned?.closeFrame).not.toBeNull(), { timeout: 500 }); + expect(abandoned?.closeFrame?.reason).toBe("Connection timed out"); + + // The newest attempt may still be connecting; aborting it closes without a frame. + await client.close(); + await vi.waitFor(() => expect(daemon.openSockets()).toEqual([]), { timeout: 500 }); + } finally { + await client.close(); + await daemon.stop(); + } + }, + 10_000, + ); + + test("a code or reason the WebSocket API rejects still closes the socket", async () => { + const daemon = await silentDaemon(); + try { + const transport = createWebSocketTransportFactory(defaultWebSocketFactory)({ + url: daemon.url, + }); + await new Promise((resolve) => transport.onOpen(() => resolve())); + const reason = `Decryption failed: ${"é".repeat(100)}`; + + expect(() => transport.close(1011, reason)).not.toThrow(); + + await vi.waitFor(() => expect(daemon.sockets[0]?.closeFrame).not.toBeNull(), { + timeout: 500, + }); + const frame = daemon.sockets[0]?.closeFrame; + expect(frame?.code).toBe(1000); + expect(reason.startsWith(frame?.reason ?? "")).toBe(true); + expect(Buffer.byteLength(frame?.reason ?? "")).toBe(123); + } finally { + await daemon.stop(); + } + }); +}); diff --git a/packages/client/src/daemon-client-websocket-transport.ts b/packages/client/src/daemon-client-websocket-transport.ts index c7ec87555d..5331aea9f4 100644 --- a/packages/client/src/daemon-client-websocket-transport.ts +++ b/packages/client/src/daemon-client-websocket-transport.ts @@ -82,7 +82,7 @@ export function createWebSocketTransportFactory(factory: WebSocketFactory): Daem // connect timeouts does not crash the CLI with an unhandled error event. const suppressEarlyCloseError = bindTemporaryEarlyCloseErrorHandler(ws); try { - ws.close(code, reason); + ws.close(toSendableCloseCode(code), toSendableCloseReason(reason)); } finally { if (typeof ws.on !== "function" && typeof ws.addEventListener !== "function") { suppressEarlyCloseError(); @@ -97,6 +97,32 @@ export function createWebSocketTransportFactory(factory: WebSocketFactory): Daem }; } +// WHATWG WebSocket.close throws for a code other than 1000 or 3000-4999 and for a +// reason over 123 UTF-8 bytes, and the throw leaves the socket open: the peer keeps +// streaming into a connection nobody reads. Browsers, Electron and Node's global +// WebSocket enforce this; Node's `ws` accepts more. Translate once here so callers +// keep their reasons and every runtime sends the same close frame. +const MAX_CLOSE_REASON_BYTES = 123; + +function toSendableCloseCode(code: number | undefined): number { + if (code === 1000 || (code !== undefined && code >= 3000 && code <= 4999)) return code; + return 1000; +} + +function toSendableCloseReason(reason: string | undefined): string | undefined { + if (reason === undefined) return undefined; + const encoder = new TextEncoder(); + if (encoder.encode(reason).byteLength <= MAX_CLOSE_REASON_BYTES) return reason; + let truncated = ""; + let bytes = 0; + for (const character of reason) { + bytes += encoder.encode(character).byteLength; + if (bytes > MAX_CLOSE_REASON_BYTES) break; + truncated += character; + } + return truncated; +} + function bindWsMessageHandler( ws: WebSocketLike, handler: (data: unknown, isBinary: boolean) => void,