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", () => {