diff --git a/docs/QA.md b/docs/QA.md index a50243f..a687dd0 100644 --- a/docs/QA.md +++ b/docs/QA.md @@ -297,6 +297,10 @@ The refresh queue is the most critical path: a 401 triggers a single token refre Flagging a credential endpoint `isPublic` sends every rejected attempt twice and then reports the session as expired — see `auth-api.test.ts` for the four regressions that guard against it. +**Refresh is single-flight, and two tests keep it that way.** Every caller that needs a renewed session shares one in-flight `POST /auth/refresh`. The public branch used to call refresh directly and so escaped the queue the authenticated path uses: opening an article fires several public reads at once — the article, its comments, the trending rail — and with a stale token each one asked for its own. That spends a five-a-minute budget three at a time, and where the refresh token rotates the later calls present one the first has already consumed, fail, and sign the reader out. + +The second test covers the guest case: a public request that carried **no** token is already anonymous, so its 401 says nothing about a session. Renewing one that was never opened ends by reporting it expired, which puts the sign-in modal in front of a reader who never signed in. + #### `authApi` (`src/features/auth/api/auth-api.test.ts`) 7 tests. The only `*.api.ts` spec in the suite, because these thunks are the one place where the _choice_ of client flag is itself the behaviour under test. diff --git a/src/core/api/client.test.ts b/src/core/api/client.test.ts index 0236f31..750b482 100644 --- a/src/core/api/client.test.ts +++ b/src/core/api/client.test.ts @@ -251,6 +251,72 @@ describe("apiClient", () => { expect(result).toEqual([{ id: "public-1" }]); }); + // Opening an article fires several public reads at once — the article, + // its comments, the trending rail. With a stale token each one takes + // this branch, and each used to call refresh directly, bypassing the + // single-flight guard the authenticated path uses. Refresh is limited + // to five a minute, and a rotating refresh token means the second and + // third present one the first has already spent: they fail, and a + // failed refresh signs the reader out. + it("refreshes once for several concurrent public 401s", async () => { + localStorage.setItem("access_token", "expired-token"); + let refreshes = 0; + const seen = new Set(); + + server.use( + http.get(`${BASE}/articles/:slug`, ({ request }) => { + const key = new URL(request.url).pathname; + if (!seen.has(key)) { + seen.add(key); + return new HttpResponse(null, { status: 401 }); + } + return HttpResponse.json({ data: { id: "a" } }); + }), + http.post(`${BASE}/auth/refresh`, async () => { + refreshes += 1; + await new Promise((resolve) => setTimeout(resolve, 20)); + return HttpResponse.json({ + data: { accessToken: "fresh" }, + }); + }), + ); + + await Promise.all([ + api.get("/articles/one", { isPublic: true }), + api.get("/articles/two", { isPublic: true }), + api.get("/articles/three", { isPublic: true }), + ]); + await new Promise((resolve) => setTimeout(resolve, 60)); + + expect(refreshes).toBe(1); + }); + + // A reader who never signed in has no session to renew. Asking anyway + // spends a request and then reports the session as expired, which + // opens the sign-in modal at someone who never signed in. + it("does not try to refresh when no token was sent", async () => { + let refreshes = 0; + const onExpired = vi.fn(); + registerSessionExpiredHandler(onExpired); + + server.use( + http.get( + `${BASE}/articles`, + () => new HttpResponse(null, { status: 401 }), + ), + http.post(`${BASE}/auth/refresh`, () => { + refreshes += 1; + return new HttpResponse(null, { status: 401 }); + }), + ); + + await api.get("/articles", { isPublic: true }).catch(() => {}); + await new Promise((resolve) => setTimeout(resolve, 40)); + + expect(refreshes).toBe(0); + expect(onExpired).not.toHaveBeenCalled(); + }); + it("reports a dropped connection on the retry as a NetworkError", async () => { localStorage.setItem("access_token", "expired-token"); let hit = 0; diff --git a/src/core/api/client.ts b/src/core/api/client.ts index 7e7f7ce..eca16fc 100644 --- a/src/core/api/client.ts +++ b/src/core/api/client.ts @@ -36,6 +36,19 @@ interface ApiOptions extends RequestInit { } let isRefreshing = false; + +/** + * The in-flight refresh, shared by every caller that needs one. + * + * The queue below serialises *authenticated* retries, but the public branch + * used to call `attemptTokenRefresh` directly and so escaped it entirely. + * Opening an article fires several public reads at once — the article, its + * comments, the trending rail — and with a stale token each one asked for its + * own refresh. That spends a five-a-minute budget three at a time, and where + * the refresh token rotates, the later calls present one the first has + * already consumed: they fail, and a failed refresh signs the reader out. + */ +let refreshPromise: Promise | null = null; let failedQueue: Array<{ resolve: (token: string | null) => void; reject: (error: unknown) => void; @@ -102,6 +115,15 @@ const attemptTokenRefresh = async (): Promise => { return null; }; +const refreshOnce = (): Promise => { + if (!refreshPromise) { + refreshPromise = attemptTokenRefresh().finally(() => { + refreshPromise = null; + }); + } + return refreshPromise; +}; + export const apiClient = async ( endpoint: string, options: ApiOptions = {}, @@ -134,10 +156,21 @@ export const apiClient = async ( headers, }); + // A public request that carried no token was already anonymous, so its + // 401 is the endpoint's own answer rather than a stale session. Retrying + // it unchanged only repeats the same failure, and renewing a session that + // was never opened ends by reporting it expired — which puts the sign-in + // modal in front of a reader who never signed in. + const isStaleSession = + response.status === 401 && + !_retry && + !isAnonymous && + !(isPublic && !token); + // `isAnonymous` endpoints answer 401 to mean "wrong credentials", so the // whole recovery apparatus below is skipped and the problem document // falls through to the caller. - if (response.status === 401 && !_retry && !isAnonymous) { + if (isStaleSession) { // Public endpoints: retry without token so the request succeeds // as unauthenticated, then attempt a background refresh. if (isPublic) { @@ -147,8 +180,10 @@ export const apiClient = async ( headers, }); - // Background refresh so subsequent authenticated calls work - attemptTokenRefresh().then((newToken) => { + // Background refresh so subsequent authenticated calls work. + // Shared with every other caller, so a page full of public reads + // renews the session once rather than once each. + refreshOnce().then((newToken) => { if (!newToken) { localStorage.removeItem("access_token"); _onSessionExpired?.(); @@ -172,7 +207,7 @@ export const apiClient = async ( isRefreshing = true; - const newToken = await attemptTokenRefresh(); + const newToken = await refreshOnce(); if (newToken) { processQueue(null, newToken); isRefreshing = false;