From 0b7a7daf6a3a8f3c6b9f9a8218c84d2390ec1c24 Mon Sep 17 00:00:00 2001 From: Andrew Han Date: Tue, 4 Aug 2026 17:53:06 -0400 Subject: [PATCH] fix(client): stop the user-token refresh from spinning `maybeScheduleUserTokenExpiration` computes its delay as `exp - timeBeforeExpirationInMs - now` and passes it straight to `setTimeout`. Whenever the client authenticates with a token that is already inside the refresh window that value is negative, `setTimeout` treats it as `0`, and the refresh runs on the next tick. If the refreshed token is also inside the window (a short-lived token, a stale token re-supplied by a re-render, a client whose clock runs ahead of the token issuer) the callback re-authenticates and the instance spins. Spinning is expensive rather than merely wasteful: `authenticate()` sees changed credentials, so every iteration tears down the API client and its socket, rebuilds them, re-identifies the user, and rejoins feed channels. Reproduced against this package with near-expiry tokens: 6,096 refreshes and 6,096 websocket opens in 10 seconds, which in a browser means a flood of 429s from `api.knock.app` and repeated "WebSocket is closed before the connection is established". Three changes: - Floor the timer delay at 1s so a token inside the refresh window schedules a real wait instead of an immediate refresh. - Clear the previous timer before scheduling. `authenticate()` with unchanged credentials does not tear down, so overwriting `tokenExpirationTimer` left the old timer live: it still woke up and called the app's refresh callback before failing the identity check. - Don't re-authenticate when the refreshed token expires no later than the token it replaces, or too soon to schedule another genuine wait. Log and stop, which leaves the socket in its normal reconnect backoff. Tokens with no readable expiration keep their current behaviour: they schedule nothing, so they cannot spin. --- .changeset/sweet-donkeys-refresh.md | 5 ++ packages/client/src/knock.ts | 126 ++++++++++++++++++++++------ packages/client/test/knock.test.ts | 91 ++++++++++++++++++++ 3 files changed, 196 insertions(+), 26 deletions(-) create mode 100644 .changeset/sweet-donkeys-refresh.md diff --git a/.changeset/sweet-donkeys-refresh.md b/.changeset/sweet-donkeys-refresh.md new file mode 100644 index 000000000..d222161f8 --- /dev/null +++ b/.changeset/sweet-donkeys-refresh.md @@ -0,0 +1,5 @@ +--- +"@knocklabs/client": patch +--- + +Stop the user-token refresh from spinning. The expiry timer is no longer scheduled with a negative delay when the client authenticates with a token that is already inside the refresh window, orphaned timers are cleared instead of left live, and a refresh whose token expires too soon to schedule another wait no longer re-authenticates (which would tear down and rebuild the API client and socket on every round trip). diff --git a/packages/client/src/knock.ts b/packages/client/src/knock.ts index 32bcc7dcc..8931a20b3 100644 --- a/packages/client/src/knock.ts +++ b/packages/client/src/knock.ts @@ -22,6 +22,14 @@ import { jwtDecode } from "./jwt"; const DEFAULT_HOST = "https://api.knock.app"; +/** + * Floor for the token-expiration timer. `exp - timeBeforeExpirationInMs - now` + * is negative whenever we authenticate with a token that is already inside the + * refresh window, and `setTimeout` treats a negative delay as `0`, so without a + * floor the refresh runs on the next tick. + */ +const MIN_TOKEN_EXPIRATION_DELAY_MS = 1_000; + class Knock { public host: string; private apiClient: ApiClient | null = null; @@ -289,33 +297,99 @@ class Knock { const nowMs = Date.now(); // Expiration is in the future - if (expiresAtMs && expiresAtMs > nowMs) { - // Check how long until the token should be regenerated - // | ----------------- | ----------------------- | - // ^ now ^ expiration offset ^ expires at - const msInFuture = expiresAtMs - timeBeforeExpirationInMs - nowMs; - - const timerId = setTimeout(async () => { - const newToken = await callbackFn(this.userToken as string, decoded); - - // If we were torn down (logout/unmount) or re-authenticated while the - // callback was awaiting, the timer reference will have changed (or been - // cleared). Bail so we don't resurrect a logged-out instance by - // re-authenticating and re-opening connections. - if (this.tokenExpirationTimer !== timerId) { - return; - } - - // Reauthenticate which will handle reinitializing sockets - if (typeof newToken === "string") { - this.authenticate(this.userId!, newToken, { - onUserTokenExpiring: callbackFn, - timeBeforeExpirationInMs: timeBeforeExpirationInMs, - }); - } - }, msInFuture); - this.tokenExpirationTimer = timerId; + if (!expiresAtMs || expiresAtMs <= nowMs) return; + + // Only ever keep one refresh timer per instance. `authenticate()` can be + // called repeatedly with unchanged credentials (a React effect re-running, + // for example), which does not tear down, so replacing the reference + // without clearing the old timer leaves it live: it still wakes up and + // calls the app's refresh callback before failing the identity check below. + if (this.tokenExpirationTimer) { + clearTimeout(this.tokenExpirationTimer); + this.tokenExpirationTimer = null; } + + // Check how long until the token should be regenerated + // | ----------------- | ----------------------- | + // ^ now ^ expiration offset ^ expires at + // + // Floored: a token that is already inside the refresh window produces a + // negative delay, which `setTimeout` runs on the next tick. + const msInFuture = Math.max( + expiresAtMs - timeBeforeExpirationInMs - nowMs, + MIN_TOKEN_EXPIRATION_DELAY_MS, + ); + + const timerId = setTimeout(async () => { + const newToken = await callbackFn(this.userToken as string, decoded); + + // If we were torn down (logout/unmount) or re-authenticated while the + // callback was awaiting, the timer reference will have changed (or been + // cleared). Bail so we don't resurrect a logged-out instance by + // re-authenticating and re-opening connections. + if (this.tokenExpirationTimer !== timerId) { + return; + } + + if (typeof newToken !== "string") { + return; + } + + // A refresh that does not push the expiration further out cannot be + // rescheduled sanely: re-authenticating would schedule another immediate + // refresh, and since changed credentials tear down and rebuild the API + // client, the instance would spin — hammering the app's token endpoint, + // re-identifying the user, and reconnecting the socket every round trip + // until the page is closed. Stop instead and let the caller notice. + if ( + !this.refreshMakesProgress( + newToken, + expiresAtMs, + timeBeforeExpirationInMs, + ) + ) { + this.log( + "Refreshed user token expires too soon to schedule another refresh; not re-authenticating", + true, + ); + return; + } + + // Reauthenticate which will handle reinitializing sockets + this.authenticate(this.userId!, newToken, { + onUserTokenExpiring: callbackFn, + timeBeforeExpirationInMs: timeBeforeExpirationInMs, + }); + }, msInFuture); + this.tokenExpirationTimer = timerId; + } + + /** + * Whether re-authenticating with `newToken` would buy real time: it has to + * expire later than the token it replaces, and late enough that the next + * refresh timer is a genuine wait rather than another immediate refresh. + * + * Tokens we cannot read an expiration from are treated as progress, leaving + * the behaviour to `authenticate()` as before — an unreadable or `exp`-less + * token schedules nothing, so it cannot spin. + */ + private refreshMakesProgress( + newToken: string, + previousExpiresAtMs: number, + timeBeforeExpirationInMs: number, + ): boolean { + let nextExpiresAtMs: number; + + try { + nextExpiresAtMs = (jwtDecode(newToken).exp ?? 0) * 1000; + } catch { + return true; + } + + if (!nextExpiresAtMs) return true; + if (nextExpiresAtMs <= previousExpiresAtMs) return false; + + return nextExpiresAtMs - timeBeforeExpirationInMs - Date.now() > 0; } /** diff --git a/packages/client/test/knock.test.ts b/packages/client/test/knock.test.ts index 4858ccec8..5495d304e 100644 --- a/packages/client/test/knock.test.ts +++ b/packages/client/test/knock.test.ts @@ -663,6 +663,97 @@ describe("Knock Client", () => { expect(authenticateSpy).not.toHaveBeenCalled(); expect(knock.isAuthenticated()).toBe(false); }); + + test("floors the refresh delay for a token already inside the expiration window", async () => { + const knock = new Knock("pk_test_12345"); + const onUserTokenExpiring = vi.fn().mockResolvedValue("token_new"); + + // 5s of life left, but we refresh 30s before expiry: the unfloored delay + // is -25s, which `setTimeout` would run on the next tick. + vi.mocked(jwtDecode).mockReturnValueOnce({ + exp: Math.floor((Date.now() + 5000) / 1000), + }); + + knock.authenticate("user_123", "token_abc", { + onUserTokenExpiring, + timeBeforeExpirationInMs: 30000, + }); + + await vi.advanceTimersByTimeAsync(0); + expect(onUserTokenExpiring).not.toHaveBeenCalled(); + + await vi.advanceTimersByTimeAsync(1000); + expect(onUserTokenExpiring).toHaveBeenCalledTimes(1); + }); + + test("does not re-authenticate when the refreshed token expires no later than the current one", async () => { + const knock = new Knock("pk_test_12345"); + const authenticateSpy = vi.spyOn(knock, "authenticate"); + const onUserTokenExpiring = vi.fn().mockResolvedValue("token_same_exp"); + + const exp = Math.floor((Date.now() + 60000) / 1000); + vi.mocked(jwtDecode) + .mockReturnValueOnce({ exp }) // the token we authenticate with + .mockReturnValueOnce({ exp }); // the refreshed token: no progress + + knock.authenticate("user_123", "token_abc", { + onUserTokenExpiring, + timeBeforeExpirationInMs: 10000, + }); + authenticateSpy.mockClear(); + + await vi.advanceTimersByTimeAsync(50000); + + expect(onUserTokenExpiring).toHaveBeenCalledTimes(1); + expect(authenticateSpy).not.toHaveBeenCalled(); + }); + + // A refreshed token that is itself inside the refresh window would schedule + // another refresh immediately. Re-authenticating on it spins: token endpoint + // flood, one identify and one socket reconnect per iteration, and 429s. + test("does not spin when the refreshed token is also inside the expiration window", async () => { + const knock = new Knock("pk_test_12345"); + const authenticateSpy = vi.spyOn(knock, "authenticate"); + const onUserTokenExpiring = vi + .fn() + .mockResolvedValue("token_also_near_expiry"); + + vi.mocked(jwtDecode) + // Authenticated with a token that has 5s of life left. + .mockReturnValueOnce({ exp: Math.floor((Date.now() + 5000) / 1000) }) + // The refresh returns a later token, but still inside the 30s window. + .mockReturnValueOnce({ exp: Math.floor((Date.now() + 10000) / 1000) }); + + knock.authenticate("user_123", "token_abc", { + onUserTokenExpiring, + timeBeforeExpirationInMs: 30000, + }); + authenticateSpy.mockClear(); + + await vi.advanceTimersByTimeAsync(60000); + + expect(onUserTokenExpiring).toHaveBeenCalledTimes(1); + expect(authenticateSpy).not.toHaveBeenCalled(); + }); + + test("keeps a single refresh timer when re-authenticating with unchanged credentials", async () => { + const knock = new Knock("pk_test_12345"); + const onUserTokenExpiring = vi.fn().mockResolvedValue("token_new"); + + const options = { + onUserTokenExpiring, + timeBeforeExpirationInMs: 10000, + }; + + // Same credentials twice: no teardown runs, so the first timer has to be + // cleared explicitly or it stays live alongside the second. + knock.authenticate("user_123", "token_abc", options); + knock.authenticate("user_123", "token_abc", options); + + await vi.advanceTimersByTimeAsync(60000); + + expect(onUserTokenExpiring).toHaveBeenCalledTimes(1); + }); }); describe("Authentication with reinitialize", () => {