diff --git a/.changeset/reduce-ws-retry-storm.md b/.changeset/reduce-ws-retry-storm.md new file mode 100644 index 000000000..ba0c097a6 --- /dev/null +++ b/.changeset/reduce-ws-retry-storm.md @@ -0,0 +1,13 @@ +--- +"@knocklabs/client": patch +--- + +fix(KNO-13857): reduce websocket reconnect load when connections can't recover + +The Phoenix socket now escalates its reconnect backoff toward a 10-minute cap (previously a fixed 30s cap that Phoenix reset on every successful open), so a client that can never reconnect — for example after an API key rotation leaves stale credentials that get rejected on every upgrade — settles into a slow retry cadence instead of a tight loop. Backoff escalation persists through brief "connect then immediately drop" cycles and resets once a connection stays up for 30s. + +Additionally: + +- `teardown()` now always disconnects the socket (even mid-reconnect), so a reauth that replaces the client can't leak a socket that keeps retrying with stale credentials. +- Hidden background tabs now stop retrying: a socket that is mid-reconnect when the page is hidden is disconnected, not just connected ones, and resumes when the page becomes visible again. +- Reconnects promptly on the browser `online` event so recovery after a real network drop doesn't wait out the (now longer) backoff. diff --git a/packages/client/src/api.ts b/packages/client/src/api.ts index 9e9c39d46..db71e02de 100644 --- a/packages/client/src/api.ts +++ b/packages/client/src/api.ts @@ -3,6 +3,22 @@ import { Socket } from "phoenix"; import { exponentialBackoffFullJitter } from "./helpers"; import { PageVisibilityManager } from "./pageVisibility"; +// Ceiling for socket reconnect backoff. This is intentionally much larger than +// the channel rejoin cap: a client that can never reconnect (e.g. a rotated +// API key that now returns 403 on every upgrade) should settle into a slow +// retry cadence rather than hammering the API indefinitely. +const SOCKET_RECONNECT_MAX_DELAY_MS = 600_000; + +// A connection that stays open at least this long is treated as healthy, which +// resets the reconnect backoff escalation so a later transient drop reconnects +// quickly. +const STABLE_CONNECTION_THRESHOLD_MS = 30_000; + +// Upper bound for the escalation counter. Past this the reconnect delay is +// already pinned to SOCKET_RECONNECT_MAX_DELAY_MS, so there's no need to keep +// counting. +const MAX_UNSTABLE_CONNECTION_COUNT = 15; + type ApiClientOptions = { host: string; apiKey: string; @@ -69,6 +85,18 @@ class ApiClient { public socket: Socket | undefined; private pageVisibility: PageVisibilityManager | undefined; + // Count of consecutive connection attempts that failed or dropped before + // stabilizing. Drives reconnect backoff escalation and, unlike Phoenix's own + // attempt counter, is not reset each time a short-lived connection opens. + private unstableConnectionCount = 0; + + // Timestamp (ms) of the current connection's open, or null when not open. + private socketOpenedAt: number | null = null; + + // Whether the socket has ever attempted to connect. Used to avoid forcing a + // connection the consumer never asked for when connectivity returns. + private socketWasActive = false; + constructor(options: ApiClientOptions) { this.host = options.host; this.apiKey = options.apiKey; @@ -93,10 +121,18 @@ class ApiClient { branch_slug: this.branch, }, reconnectAfterMs: (tries: number) => { - return exponentialBackoffFullJitter(tries, { - baseDelayMs: 1000, - maxDelayMs: 30_000, - }); + // Escalate using whichever is larger: Phoenix's own attempt counter + // (which it resets on every successful open) or our count of + // consecutive unstable connections (which survives brief opens). The + // latter keeps the delay growing through "accept then immediately + // drop" cycles that would otherwise reset Phoenix's counter each time. + return exponentialBackoffFullJitter( + Math.max(tries, this.unstableConnectionCount), + { + baseDelayMs: 1000, + maxDelayMs: SOCKET_RECONNECT_MAX_DELAY_MS, + }, + ); }, rejoinAfterMs: (tries: number) => { return exponentialBackoffFullJitter(tries, { @@ -106,12 +142,78 @@ class ApiClient { }, }); + this.trackConnectionStability(this.socket); + + // Recover promptly when connectivity returns instead of waiting out a + // potentially long reconnect backoff window. + if (typeof window.addEventListener === "function") { + window.addEventListener("online", this.handleOnline); + } + if (options.disconnectOnPageHidden !== false) { this.pageVisibility = new PageVisibilityManager(this.socket); } } } + // Observes socket open/close events to drive reconnect backoff escalation. + // The escalation is what turns a permanent failure (e.g. a rejected upgrade + // after a key rotation) from a tight retry loop into a slow one. + private trackConnectionStability(socket: Socket) { + socket.onOpen(() => { + this.socketWasActive = true; + this.socketOpenedAt = Date.now(); + }); + + socket.onClose((event) => { + this.socketWasActive = true; + + const openedAt = this.socketOpenedAt; + this.socketOpenedAt = null; + + const stayedConnected = + openedAt !== null && + Date.now() - openedAt >= STABLE_CONNECTION_THRESHOLD_MS; + + if (stayedConnected) { + // A connection that stayed up long enough is considered healthy, so + // allow fast reconnects again after a transient drop. + this.unstableConnectionCount = 0; + } else if (!event?.wasClean) { + // A rejected upgrade (never opened) or a connection dropped shortly + // after opening keeps escalating the backoff. Clean closes — our own + // disconnect() or a graceful server close — are intentional, won't be + // retried by Phoenix, and so don't count. + this.unstableConnectionCount = Math.min( + this.unstableConnectionCount + 1, + MAX_UNSTABLE_CONNECTION_COUNT, + ); + } + }); + } + + private handleOnline = () => { + const socket = this.socket; + if (!socket) { + return; + } + + // Only act if the consumer previously opened a connection that isn't + // currently live. When the page is hidden, PageVisibilityManager owns the + // connection lifecycle, so leave it be. + if (!this.socketWasActive || socket.isConnected()) { + return; + } + + if (typeof document !== "undefined" && document.hidden) { + return; + } + + // Cancel any pending backoff and make a single immediate attempt. If it + // fails, the escalated schedule resumes (unstableConnectionCount persists). + socket.disconnect(() => socket.connect()); + }; + async makeRequest(req: ApiRequestConfig): Promise { try { const result = await this.requestWithRetries(req); @@ -307,9 +409,18 @@ class ApiClient { teardown() { this.pageVisibility?.teardown(); - if (this.socket?.isConnected()) { - this.socket.disconnect(); + if ( + typeof window !== "undefined" && + typeof window.removeEventListener === "function" + ) { + window.removeEventListener("online", this.handleOnline); } + + // Always disconnect, even when not currently connected: a socket that is + // mid-reconnect still holds a pending retry timer, and disconnect() is the + // only thing that cancels it. Otherwise a reauth that replaces this client + // would leak a socket that retries forever with stale credentials. + this.socket?.disconnect(); } private canRetryRequest(error: unknown) { diff --git a/packages/client/src/pageVisibility.ts b/packages/client/src/pageVisibility.ts index fd837a5b2..b3e8b2847 100644 --- a/packages/client/src/pageVisibility.ts +++ b/packages/client/src/pageVisibility.ts @@ -12,17 +12,28 @@ const DEFAULT_DISCONNECT_DELAY_MS = 30_000; */ export class PageVisibilityManager { private disconnectTimer: ReturnType | null = null; - private wasConnected = false; + private shouldReconnectOnVisible = false; + private socketWasActive = false; constructor( private socket: Socket, private disconnectDelayMs: number = DEFAULT_DISCONNECT_DELAY_MS, ) { + // Track whether the socket has ever attempted to connect so we only park + // (and later resume) sockets the consumer actually activated. Both + // callbacks fire only after connect() has been called at least once. + this.socket.onOpen(this.markActive); + this.socket.onClose(this.markActive); + if (typeof document !== "undefined") { document.addEventListener("visibilitychange", this.onVisibilityChange); } } + private markActive = () => { + this.socketWasActive = true; + }; + private onVisibilityChange = () => { if (document.hidden) { this.scheduleDisconnect(); @@ -37,8 +48,12 @@ export class PageVisibilityManager { this.disconnectTimer = setTimeout(() => { this.disconnectTimer = null; - if (this.socket.isConnected()) { - this.wasConnected = true; + // Disconnect even when the socket is mid-reconnect rather than fully + // connected: a socket retrying against, say, a rejected credential would + // otherwise keep looping in a hidden background tab. disconnect() also + // cancels Phoenix's pending reconnect timer. + if (this.socketWasActive) { + this.shouldReconnectOnVisible = true; this.socket.disconnect(); } }, this.disconnectDelayMs); @@ -47,8 +62,8 @@ export class PageVisibilityManager { private reconnect() { this.clearTimer(); - if (this.wasConnected) { - this.wasConnected = false; + if (this.shouldReconnectOnVisible) { + this.shouldReconnectOnVisible = false; this.socket.connect(); } } diff --git a/packages/client/test/api.test.ts b/packages/client/test/api.test.ts index 5ca53c745..da39fb48c 100644 --- a/packages/client/test/api.test.ts +++ b/packages/client/test/api.test.ts @@ -33,11 +33,14 @@ const skipRetryDelays = (apiClient: ApiClient) => { .mockResolvedValue(undefined); }; -vi.mock("phoenix", () => ({ - Socket: vi.fn().mockImplementation(() => ({ +const { createSocketMock } = vi.hoisted(() => ({ + createSocketMock: () => ({ connect: vi.fn(), disconnect: vi.fn(), isConnected: vi.fn().mockReturnValue(false), + onOpen: vi.fn(), + onClose: vi.fn(), + onError: vi.fn(), channel: vi.fn().mockReturnValue({ join: vi.fn(), leave: vi.fn(), @@ -45,12 +48,45 @@ vi.mock("phoenix", () => ({ on: vi.fn(), off: vi.fn(), }), - })), + }), })); +vi.mock("phoenix", () => ({ + Socket: vi.fn(createSocketMock), +})); + +type MockSocket = { + connect: ReturnType; + disconnect: ReturnType; + isConnected: ReturnType; + onOpen: ReturnType; + onClose: ReturnType; +}; + +// ApiClient registers its connection-stability handlers first (before +// PageVisibilityManager), so calls[0] is always the escalation handler. The new +// socket tests disable PageVisibilityManager anyway to isolate this behavior. +const getOnCloseHandler = (apiClient: ApiClient) => { + const socket = apiClient.socket as unknown as MockSocket; + return socket.onClose.mock.calls[0]![0] as (event: { + wasClean: boolean; + }) => void; +}; + +const getOnOpenHandler = (apiClient: ApiClient) => { + const socket = apiClient.socket as unknown as MockSocket; + return socket.onOpen.mock.calls[0]![0] as () => void; +}; + describe("API Client", () => { beforeEach(() => { vi.clearAllMocks(); + // afterEach's restoreAllMocks() strips the Socket mock's implementation, so + // re-establish it before each test (the ApiClient constructor now calls + // socket.onOpen/onClose). + vi.mocked(Socket).mockImplementation( + createSocketMock as unknown as () => Socket, + ); }); afterEach(() => { @@ -684,12 +720,138 @@ describe("API Client", () => { for (let i = 0; i < 50; i++) { const delay = reconnectAfterMs(100); expect(delay).toBeGreaterThanOrEqual(250); - expect(delay).toBeLessThanOrEqual(30_000); + expect(delay).toBeLessThanOrEqual(600_000); } (global as GlobalWithWindow).window = originalWindow; }); + test("reconnectAfterMs escalates as connections keep failing", () => { + const originalWindow = (global as GlobalWithWindow).window; + (global as GlobalWithWindow).window = {}; + + vi.mocked(Socket).mockClear(); + + const apiClient = new ApiClient({ + host: "https://api.knock.app", + apiKey: "pk_test_12345", + userToken: "user_token_456", + disconnectOnPageHidden: false, + }); + + const socketOpts = vi.mocked(Socket).mock.calls[0]![1] as Record< + string, + unknown + >; + const reconnectAfterMs = socketOpts.reconnectAfterMs as ( + tries: number, + ) => number; + const onClose = getOnCloseHandler(apiClient); + + // Before any failures, the delay for the first attempt stays within the + // original ~1s window. + const initialMax = Math.max( + ...Array.from({ length: 200 }, () => reconnectAfterMs(1)), + ); + expect(initialMax).toBeLessThanOrEqual(1000); + + // Simulate a run of connections that never open and close uncleanly, as + // happens when every upgrade is rejected (e.g. a rotated API key). + for (let i = 0; i < 20; i++) { + onClose({ wasClean: false }); + } + + // Even for Phoenix's "first" attempt, the escalation counter now pushes + // the ceiling well past the original 30s cap, up to the 10-minute cap. + const escalatedMax = Math.max( + ...Array.from({ length: 200 }, () => reconnectAfterMs(1)), + ); + expect(escalatedMax).toBeGreaterThan(30_000); + expect(escalatedMax).toBeLessThanOrEqual(600_000); + + (global as GlobalWithWindow).window = originalWindow; + }); + + test("a connection that stays up long enough resets the escalation", () => { + const originalWindow = (global as GlobalWithWindow).window; + (global as GlobalWithWindow).window = {}; + + const nowSpy = vi.spyOn(Date, "now").mockReturnValue(0); + vi.mocked(Socket).mockClear(); + + const apiClient = new ApiClient({ + host: "https://api.knock.app", + apiKey: "pk_test_12345", + userToken: "user_token_456", + disconnectOnPageHidden: false, + }); + + const socketOpts = vi.mocked(Socket).mock.calls[0]![1] as Record< + string, + unknown + >; + const reconnectAfterMs = socketOpts.reconnectAfterMs as ( + tries: number, + ) => number; + const onOpen = getOnOpenHandler(apiClient); + const onClose = getOnCloseHandler(apiClient); + + for (let i = 0; i < 20; i++) { + onClose({ wasClean: false }); + } + + // A connection opens and stays up beyond the stability threshold. + nowSpy.mockReturnValue(1_000); + onOpen(); + nowSpy.mockReturnValue(1_000 + 30_000); + onClose({ wasClean: false }); + + // Escalation is reset, so the first attempt is back in the ~1s window. + const resetMax = Math.max( + ...Array.from({ length: 200 }, () => reconnectAfterMs(1)), + ); + expect(resetMax).toBeLessThanOrEqual(1000); + + nowSpy.mockRestore(); + (global as GlobalWithWindow).window = originalWindow; + }); + + test("clean closes do not escalate the reconnect backoff", () => { + const originalWindow = (global as GlobalWithWindow).window; + (global as GlobalWithWindow).window = {}; + + vi.mocked(Socket).mockClear(); + + const apiClient = new ApiClient({ + host: "https://api.knock.app", + apiKey: "pk_test_12345", + userToken: "user_token_456", + disconnectOnPageHidden: false, + }); + + const socketOpts = vi.mocked(Socket).mock.calls[0]![1] as Record< + string, + unknown + >; + const reconnectAfterMs = socketOpts.reconnectAfterMs as ( + tries: number, + ) => number; + const onClose = getOnCloseHandler(apiClient); + + // Intentional (clean) closes — our own disconnect or a graceful server + // close — must not push the backoff up. + for (let i = 0; i < 20; i++) { + onClose({ wasClean: true }); + } + + const max = Math.max( + ...Array.from({ length: 200 }, () => reconnectAfterMs(1)), + ); + expect(max).toBeLessThanOrEqual(1000); + + (global as GlobalWithWindow).window = originalWindow; + }); + test("rejoinAfterMs returns values within expected bounds", () => { const originalWindow = (global as GlobalWithWindow).window; (global as GlobalWithWindow).window = {}; @@ -739,5 +901,77 @@ describe("API Client", () => { (global as GlobalWithWindow).window = originalWindow; }); + + test("teardown disconnects the socket even when it is not connected", () => { + const originalWindow = (global as GlobalWithWindow).window; + (global as GlobalWithWindow).window = {}; + + const apiClient = new ApiClient({ + host: "https://api.knock.app", + apiKey: "pk_test_12345", + userToken: undefined, + disconnectOnPageHidden: false, + }); + + const socket = apiClient.socket as unknown as MockSocket; + socket.isConnected.mockReturnValue(false); + + apiClient.teardown(); + + // A socket mid-reconnect must still be disconnected so its pending retry + // timer is cancelled and it doesn't keep looping after the client is torn + // down. + expect(socket.disconnect).toHaveBeenCalledOnce(); + + (global as GlobalWithWindow).window = originalWindow; + }); + + test("reconnects a single time when connectivity returns via the online event", () => { + const originalWindow = (global as GlobalWithWindow).window; + const addEventListener = vi.fn(); + const removeEventListener = vi.fn(); + (global as GlobalWithWindow).window = { + addEventListener, + removeEventListener, + }; + + const apiClient = new ApiClient({ + host: "https://api.knock.app", + apiKey: "pk_test_12345", + userToken: undefined, + disconnectOnPageHidden: false, + }); + + const onlineHandler = addEventListener.mock.calls.find( + (call) => call[0] === "online", + )![1] as () => void; + + const socket = apiClient.socket as unknown as MockSocket; + socket.isConnected.mockReturnValue(false); + + // Before any connection attempt, the online event is a no-op: we never + // force a connection the consumer didn't ask for. + onlineHandler(); + expect(socket.disconnect).not.toHaveBeenCalled(); + + // Simulate a prior failed attempt so the socket counts as "active". + getOnCloseHandler(apiClient)({ wasClean: false }); + + onlineHandler(); + expect(socket.disconnect).toHaveBeenCalledOnce(); + + // The disconnect callback triggers a single fresh connect. + const disconnectCallback = socket.disconnect.mock.calls[0]![0] as + | (() => void) + | undefined; + disconnectCallback?.(); + expect(socket.connect).toHaveBeenCalledOnce(); + + // Teardown unregisters the listener. + apiClient.teardown(); + expect(removeEventListener).toHaveBeenCalledWith("online", onlineHandler); + + (global as GlobalWithWindow).window = originalWindow; + }); }); }); diff --git a/packages/client/test/pageVisibility.test.ts b/packages/client/test/pageVisibility.test.ts index b7d36d399..8dbc1f352 100644 --- a/packages/client/test/pageVisibility.test.ts +++ b/packages/client/test/pageVisibility.test.ts @@ -5,15 +5,30 @@ import type { Socket } from "phoenix"; import { PageVisibilityManager } from "../src/pageVisibility"; +type MockSocket = Socket & { + isConnected: ReturnType; + connect: ReturnType; + disconnect: ReturnType; +}; + function createMockSocket() { - return { + const openCallbacks: Array<() => void> = []; + const closeCallbacks: Array<() => void> = []; + + const socket = { isConnected: vi.fn(() => true), connect: vi.fn(), disconnect: vi.fn(), - } as unknown as Socket & { - isConnected: ReturnType; - connect: ReturnType; - disconnect: ReturnType; + onOpen: vi.fn((cb: () => void) => openCallbacks.push(cb)), + onClose: vi.fn((cb: () => void) => closeCallbacks.push(cb)), + } as unknown as MockSocket; + + return { + socket, + // Simulates the socket successfully opening a connection. + fireOpen: () => openCallbacks.forEach((cb) => cb()), + // Simulates the socket's connection closing (e.g. a failed/dropped attempt). + fireClose: () => closeCallbacks.forEach((cb) => cb()), }; } @@ -36,8 +51,9 @@ describe("PageVisibilityManager", () => { }); test("disconnects the socket after the delay when page becomes hidden", () => { - const socket = createMockSocket(); + const { socket, fireOpen } = createMockSocket(); const manager = new PageVisibilityManager(socket); + fireOpen(); simulateVisibilityChange(true); @@ -49,8 +65,9 @@ describe("PageVisibilityManager", () => { }); test("reconnects the socket when page becomes visible after a disconnect", () => { - const socket = createMockSocket(); + const { socket, fireOpen } = createMockSocket(); const manager = new PageVisibilityManager(socket); + fireOpen(); simulateVisibilityChange(true); vi.advanceTimersByTime(30_000); @@ -61,9 +78,31 @@ describe("PageVisibilityManager", () => { manager.teardown(); }); + test("parks and resumes a socket that is mid-reconnect (not connected) while hidden", () => { + const { socket, fireClose } = createMockSocket(); + // Socket is retrying, so it is not currently connected. + socket.isConnected.mockReturnValue(false); + const manager = new PageVisibilityManager(socket); + // A failed/closed attempt still marks the socket as active. + fireClose(); + + simulateVisibilityChange(true); + vi.advanceTimersByTime(30_000); + + // The retrying socket is parked despite never being "connected", which + // stops the reconnect loop from running in a hidden background tab. + expect(socket.disconnect).toHaveBeenCalledOnce(); + + simulateVisibilityChange(false); + expect(socket.connect).toHaveBeenCalledOnce(); + + manager.teardown(); + }); + test("cancels the pending disconnect if the page becomes visible before the delay", () => { - const socket = createMockSocket(); + const { socket, fireOpen } = createMockSocket(); const manager = new PageVisibilityManager(socket); + fireOpen(); simulateVisibilityChange(true); vi.advanceTimersByTime(15_000); @@ -77,10 +116,11 @@ describe("PageVisibilityManager", () => { manager.teardown(); }); - test("does not reconnect if the socket was not connected when hidden", () => { - const socket = createMockSocket(); + test("does not park or reconnect a socket that was never activated", () => { + const { socket } = createMockSocket(); socket.isConnected.mockReturnValue(false); const manager = new PageVisibilityManager(socket); + // No fireOpen/fireClose: the consumer never opened a connection. simulateVisibilityChange(true); vi.advanceTimersByTime(30_000); @@ -94,8 +134,9 @@ describe("PageVisibilityManager", () => { }); test("respects a custom disconnect delay", () => { - const socket = createMockSocket(); + const { socket, fireOpen } = createMockSocket(); const manager = new PageVisibilityManager(socket, 5_000); + fireOpen(); simulateVisibilityChange(true); @@ -109,8 +150,9 @@ describe("PageVisibilityManager", () => { }); test("teardown removes the event listener and clears pending timers", () => { - const socket = createMockSocket(); + const { socket, fireOpen } = createMockSocket(); const manager = new PageVisibilityManager(socket); + fireOpen(); simulateVisibilityChange(true); manager.teardown();