From 54b59965ed18ce1d33ca74be512b2eaed1169a4a Mon Sep 17 00:00:00 2001 From: dev_agent Date: Thu, 8 Oct 2026 11:27:18 -0400 Subject: [PATCH] Bound a Supply run toggle's session load and CSRF refresh by its deadline (Feature 17) - loadMe takes an optional abort signal; AuthBootstrap calls it with none. - Each toggle try loads GET /api/v1/me itself under its deadline when the session is not loaded, instead of joining a /me query already in flight that may never answer. - The /me refresh a csrf 403 makes is bounded by the request's signal: a request whose signal aborts stops waiting at once and the refresh is no longer shared, so the next csrf 403 starts a fresh /me. Requests without a signal still share it, and it is aborted only once no request waits. - Tests: a hung /me and a hung csrf refresh during a toggle exchange, both recovered on the next try in order; the client's refresh sharing and abort. Co-Authored-By: Claude Opus 5.5 --- web/src/AuthBootstrap.tsx | 2 +- web/src/api/client.test.ts | 72 ++++++++++++++++ web/src/api/client.ts | 67 ++++++++++++--- web/src/shopping/api.ts | 20 ++--- web/src/shopping/offlineQueue.test.tsx | 112 +++++++++++++++++++++++-- 5 files changed, 239 insertions(+), 34 deletions(-) diff --git a/web/src/AuthBootstrap.tsx b/web/src/AuthBootstrap.tsx index 774ca5c..c3d9522 100644 --- a/web/src/AuthBootstrap.tsx +++ b/web/src/AuthBootstrap.tsx @@ -20,7 +20,7 @@ export const meQueryKey = [ME_PATH] as const export function AuthBootstrap({ children }: { children: ReactNode }) { const api = useApi() const forbidden = useSyncExternalStore(api.subscribe, api.forbidden) - const me = useQuery({ queryKey: meQueryKey, queryFn: api.loadMe, staleTime: Infinity, networkMode: 'always' }) + const me = useQuery({ queryKey: meQueryKey, queryFn: () => api.loadMe(), staleTime: Infinity, networkMode: 'always' }) // fetch rejects with a TypeError only when the network fails. const savedPages = me.isError && me.error instanceof TypeError && api.session.verified() const { refetch } = me diff --git a/web/src/api/client.test.ts b/web/src/api/client.test.ts index 15390e7..8155382 100644 --- a/web/src/api/client.test.ts +++ b/web/src/api/client.test.ts @@ -125,6 +125,66 @@ describe('apiFetch', () => { expect(api.forbidden()).toBe(false) }) + it('shares a csrf refresh between requests, but one whose signal aborts stops waiting and the next refreshes afresh', async () => { + // The first refresh answers only when answerFirst is called; every other + // request answers at once. + let answerFirst!: (response: Response) => void + const mes: (AbortSignal | null | undefined)[] = [] + const fetchMock = vi.fn((input, init) => { + if (String(input) === '/api/v1/me') { + mes.push(init?.signal) + if (mes.length === 2) return new Promise((resolve) => (answerFirst = resolve)) + return Promise.resolve(jsonResponse(200, { ...ME, csrfToken: `token-${mes.length}` })) + } + const token = new Headers(init?.headers).get(CSRF_HEADER) + return Promise.resolve(token === 'token-1' ? errorResponse(403, 'csrf') : jsonResponse(201, { token })) + }) + vi.stubGlobal('fetch', fetchMock) + const api = createApiClient(fakeLocation(), createNetworkStatus(window), createVerifiedSession(localStorage)) + await api.loadMe() + + const unbounded = api.apiFetch('/api/v1/jobs', { method: 'POST', body: {} }) + const timedOut = new AbortController() + const bounded = api.apiFetch('/api/v1/jobs', { method: 'POST', body: {}, signal: timedOut.signal }) + await vi.waitFor(() => expect(mes).toHaveLength(2)) + timedOut.abort(new DOMException('no answer in time', 'TimeoutError')) + await expect(bounded).rejects.toMatchObject({ name: 'TimeoutError' }) + // A request without a signal still waits for it, so it is not aborted. + expect(mes[1]!.aborted).toBe(false) + + // A later csrf 403 does not wait on the refresh still in flight. + await expect( + api.apiFetch('/api/v1/jobs', { method: 'POST', body: {}, signal: new AbortController().signal }), + ).resolves.toEqual({ token: 'token-3' }) + expect(mes).toHaveLength(3) + + answerFirst(jsonResponse(200, { ...ME, csrfToken: 'token-2' })) + await expect(unbounded).resolves.toEqual({ token: 'token-2' }) + expect(mes).toHaveLength(3) + expect(fetchMock.mock.calls.filter(([, init]) => init?.method === 'POST')).toHaveLength(5) + }) + + it('aborts a csrf refresh once the only request waiting for it aborts, without counting it as offline', async () => { + const mes: AbortSignal[] = [] + const fetchMock = vi.fn((input, init) => { + if (String(input) !== '/api/v1/me') return Promise.resolve(errorResponse(403, 'csrf')) + if (mes.push(init!.signal!) === 1) return Promise.resolve(jsonResponse(200, ME)) + return new Promise((_, reject) => init!.signal!.addEventListener('abort', () => reject(init!.signal!.reason as Error))) + }) + vi.stubGlobal('fetch', fetchMock) + const network = createNetworkStatus(window) + const api = createApiClient(fakeLocation(), network, createVerifiedSession(localStorage)) + await api.loadMe() + + const timedOut = new AbortController() + const bounded = api.apiFetch('/api/v1/jobs', { method: 'POST', body: {}, signal: timedOut.signal }) + await vi.waitFor(() => expect(mes).toHaveLength(2)) + timedOut.abort(new DOMException('no answer in time', 'TimeoutError')) + await expect(bounded).rejects.toMatchObject({ name: 'TimeoutError' }) + expect(mes[1]!.aborted).toBe(true) + expect(network.offline()).toBe(false) + }) + it('marks any other 403 as not in this household and notifies subscribers', async () => { const fetchMock = stubFetch(errorResponse(403, 'forbidden')) const api = createApiClient(fakeLocation(), createNetworkStatus(window), createVerifiedSession(localStorage)) @@ -153,6 +213,18 @@ describe('apiFetch', () => { }) describe('loadMe', () => { + it('aborts /me with its signal', async () => { + vi.stubGlobal( + 'fetch', + vi.fn((_, init) => new Promise((_, reject) => init!.signal!.addEventListener('abort', () => reject(init!.signal!.reason as Error)))), + ) + const api = createApiClient(fakeLocation(), createNetworkStatus(window), createVerifiedSession(localStorage)) + const timedOut = new AbortController() + const me = api.loadMe(timedOut.signal) + timedOut.abort(new DOMException('no answer in time', 'TimeoutError')) + await expect(me).rejects.toMatchObject({ name: 'TimeoutError' }) + }) + it('rejects a /me response without a CSRF token', async () => { stubFetch(jsonResponse(200, { ...ME, csrfToken: '' })) const api = createApiClient(fakeLocation(), createNetworkStatus(window), createVerifiedSession(localStorage)) diff --git a/web/src/api/client.ts b/web/src/api/client.ts index 754edaf..ae6821b 100644 --- a/web/src/api/client.ts +++ b/web/src/api/client.ts @@ -41,8 +41,9 @@ export type ApiRequest = { method?: 'GET' | 'POST' | 'PUT' | 'PATCH' | 'DELETE' // Sent as JSON. A GET must not have one. body?: unknown - // Aborts the request; fetch then rejects with the signal's reason. No - // request has a deadline unless its caller aborts it. + // Aborts the request, including the /me refresh a csrf 403 makes it wait + // for; it then rejects with the signal's reason. No request has a deadline + // unless its caller aborts it. signal?: AbortSignal } @@ -58,8 +59,9 @@ export type ApiClient = { // apiRead is apiFetch's GET that also says whether the body is the service // worker's cached copy rather than the server's answer. apiRead(path: string): Promise> - // loadMe fetches GET /api/v1/me and caches its CSRF token. - loadMe(): Promise + // loadMe fetches GET /api/v1/me and caches its CSRF token; signal aborts + // it, as it does an apiFetch. + loadMe(signal?: AbortSignal): Promise // forbidden reports whether a request was refused because the signed-in // account is not in this household; subscribe is notified when it changes. forbidden(): boolean @@ -73,11 +75,16 @@ export type ApiClient = { session: VerifiedSession } +// Refresh is the /me request refreshMe shares: controller aborts it, waiting +// counts the requests with a signal still waiting for it, and unbounded says +// whether one without a signal is. +type Refresh = { me: Promise; controller: AbortController; waiting: number; unbounded: boolean } + // createApiClient builds the app's API client. location is window.location in // the app: a 401 sends it to /auth/login with the current path to return to. export function createApiClient(location: ApiLocation, network: NetworkStatus, session: VerifiedSession): ApiClient { let csrfToken: string | null = null - let refreshing: Promise | null = null + let refreshing: Refresh | null = null let forbidden = false const listeners = new Set<() => void>() @@ -115,12 +122,46 @@ export function createApiClient(location: ApiLocation, network: NetworkStatus, s } // refreshMe shares one in-flight /me request between every request that - // got a csrf 403 at the same time. - function refreshMe(): Promise { - refreshing ??= loadMe().finally(() => { - refreshing = null + // got a csrf 403 at the same time. A request whose signal aborts stops + // waiting for it at once, and it is no longer shared: the next csrf 403 + // starts a fresh /me rather than wait on one that may never answer. Once + // every request with a signal waiting for it has stopped, and none without + // one waits, it is aborted. + function refreshMe(signal: AbortSignal | undefined): Promise { + if (refreshing === null) { + const controller = new AbortController() + const refresh: Refresh = { me: loadMe(controller.signal), controller, waiting: 0, unbounded: false } + const done = () => { + if (refreshing === refresh) refreshing = null + } + refresh.me.then(done, done) + refreshing = refresh + } + const refresh = refreshing + if (signal === undefined) { + refresh.unbounded = true + return refresh.me + } + refresh.waiting++ + return new Promise((resolve, reject) => { + const stop = () => { + if (refreshing === refresh) refreshing = null + if (--refresh.waiting === 0 && !refresh.unbounded) refresh.controller.abort(signal.reason) + reject(signal.reason as Error) + } + if (signal.aborted) return stop() + signal.addEventListener('abort', stop, { once: true }) + refresh.me.then( + (me) => { + signal.removeEventListener('abort', stop) + resolve(me) + }, + (error: unknown) => { + signal.removeEventListener('abort', stop) + reject(error as Error) + }, + ) }) - return refreshing } async function apiFetch(path: string, request: ApiRequest = {}): Promise { @@ -147,7 +188,7 @@ export function createApiClient(location: ApiLocation, network: NetworkStatus, s // Only a non-GET carries the token, so only it can be refused for it. if (error.code !== 'csrf' || method === 'GET') throw error // The session's token changed: refresh it once and retry once. - await refreshMe() + await refreshMe(request.signal) response = await send(method, url, request.body, request.signal) if (response.status === 403) throw await parseForbidden(method, path, response) } @@ -179,8 +220,8 @@ export function createApiClient(location: ApiLocation, network: NetworkStatus, s return (await response.json()) as T } - async function loadMe(): Promise { - const me = await apiFetch(ME_PATH) + async function loadMe(signal?: AbortSignal): Promise { + const me = await apiFetch(ME_PATH, { signal }) if ( !isRecord(me) || typeof me.name !== 'string' || diff --git a/web/src/shopping/api.ts b/web/src/shopping/api.ts index fb3fbdb..8ea8afc 100644 --- a/web/src/shopping/api.ts +++ b/web/src/shopping/api.ts @@ -228,8 +228,9 @@ export type Toggles = { } // TOGGLE_DEADLINE is how long, in milliseconds, one try of a toggle — its -// GET /api/v1/me and its PATCH — may take before it counts as the server not -// answering. Only toggles have a deadline: the API client sets none. +// GET /api/v1/me, its PATCH and the /me refresh a csrf 403 to it makes — may +// take before it counts as the server not answering. Only toggles have a +// deadline: the API client sets none. export const TOGGLE_DEADLINE = 10_000 // RETRY_FIRST and RETRY_CEILING bound, in milliseconds, how long a toggle @@ -258,14 +259,6 @@ function reachable(network: NetworkStatus, delay: number): Promise { }) } -// beforeDeadline is promise, rejected with signal's reason once it aborts. -function beforeDeadline(promise: Promise, signal: AbortSignal): Promise { - return new Promise((resolve, reject) => { - signal.addEventListener('abort', () => reject(signal.reason as Error), { once: true }) - promise.then(resolve, reject) - }) -} - // toggleOptions is the mutation every toggle of the week's Supply run is, // made on the Supply run screen (useToggles) or replayed from the queue saved // on this device (useSupplyRunSync). @@ -313,8 +306,11 @@ function toggleOptions( const deadline = new AbortController() const timer = setTimeout(() => deadline.abort(new DOMException(`${where}: no answer in time`, 'TimeoutError')), TOGGLE_DEADLINE) try { - const me = queryClient.ensureQueryData({ queryKey: meQueryKey, queryFn: api.loadMe, networkMode: 'always' }) - await beforeDeadline(me, deadline.signal) + // Each try loads /me itself, under its deadline, rather than share + // a /me already in flight that may never answer. + if (queryClient.getQueryData(meQueryKey) === undefined) { + queryClient.setQueryData(meQueryKey, await api.loadMe(deadline.signal)) + } body = await api.apiFetch(path, { method: 'PATCH', body: { needed }, signal: deadline.signal }) break } catch (error) { diff --git a/web/src/shopping/offlineQueue.test.tsx b/web/src/shopping/offlineQueue.test.tsx index e6f0f0c..2f64ec5 100644 --- a/web/src/shopping/offlineQueue.test.tsx +++ b/web/src/shopping/offlineQueue.test.tsx @@ -68,14 +68,21 @@ function goOnline() { // stubServer is the server's Supply runs of this week and next: a PATCH // marks its item and answers with its week's counts, unless refuse answers // it instead; when hold is given, the item is marked at once but the answer -// waits for the promise hold returns, or until the request is aborted. This -// week's counts as archived once archive is called. -function stubServer(refuse: (id: string) => Response | undefined = () => undefined, hold?: () => Promise) { +// waits for the promise hold returns, or until the request is aborted; holdMe +// holds GET /api/v1/me's answer the same way. This week's counts as archived +// once archive is called. +function stubServer( + refuse: (id: string) => Response | undefined = () => undefined, + hold?: () => Promise, + holdMe?: () => Promise, +) { const needed = new Map(Object.values(WEEKS).flatMap((items) => items.map((item) => [item.id, item.needed] as const))) let status = 'current' // How many PATCHes are unanswered now, and the most there have been at once. let inFlight = 0 let mostInFlight = 0 + // How many GET /api/v1/me requests have been answered. + let meAnswered = 0 const counts = (week: string) => { const marks = itemsOf(week).map((item) => needed.get(item.id)!) return { needed_count: marks.filter(Boolean).length, total_count: marks.length } @@ -98,7 +105,11 @@ function stubServer(refuse: (id: string) => Response | undefined = () => undefin const fetchMock = vi.fn(async (input, init) => { if (!onLine || !reachable) throw new TypeError('Failed to fetch') const key = `${init?.method ?? 'GET'} ${String(input)}` - if (key === 'GET /api/v1/me') return jsonResponse(200, ME) + if (key === 'GET /api/v1/me') { + await answerAfter(init?.signal, holdMe) + meAnswered++ + return jsonResponse(200, ME) + } if (key === 'GET /api/v1/settings/household') return jsonResponse(200, HOUSEHOLD_SETTINGS) if (key === `GET ${THIS_WEEK}`) return jsonResponse(200, run('2026-10-05')) const week = Object.keys(WEEKS).find((w) => itemsOf(w).some((i) => key === `PATCH ${ITEM(i.id)}`)) @@ -112,10 +123,7 @@ function stubServer(refuse: (id: string) => Response | undefined = () => undefin inFlight++ mostInFlight = Math.max(mostInFlight, inFlight) try { - await new Promise((resolve, reject) => { - init?.signal?.addEventListener('abort', () => reject(init.signal!.reason as Error)) - void (hold?.() ?? Promise.resolve()).then(resolve) - }) + await answerAfter(init?.signal, hold) } finally { inFlight-- } @@ -126,12 +134,22 @@ function stubServer(refuse: (id: string) => Response | undefined = () => undefin fetchMock, run, mostInFlight: () => mostInFlight, + meAnswered: () => meAnswered, archive: () => { status = 'archived' }, } } +// answerAfter resolves once the promise hold returns does, at once without +// hold, or rejects with signal's reason once it aborts. +function answerAfter(signal: AbortSignal | null | undefined, hold?: () => Promise) { + return new Promise((resolve, reject) => { + signal?.addEventListener('abort', () => reject(signal.reason as Error)) + void (hold?.() ?? Promise.resolve()).then(resolve) + }) +} + // held is a hold for stubServer whose answers wait until release is called, // each one released in turn. function held() { @@ -143,6 +161,11 @@ function held() { } } +// mes counts the GET /api/v1/me requests sent. +function mes(fetchMock: Mock) { + return fetchMock.mock.calls.filter(([input, init]) => String(input) === '/api/v1/me' && (init?.method ?? 'GET') === 'GET').length +} + // patches lists the PATCHes sent, as "path body". function patches(fetchMock: Mock) { return fetchMock.mock.calls.filter(([, init]) => init?.method === 'PATCH').map(([input, init]) => `${String(input)} ${String(init?.body)}`) @@ -449,6 +472,79 @@ describe('offline toggle queue, replay', () => { expect(screen.queryByRole('alert')).not.toBeInTheDocument() }) + it('abandons a GET /api/v1/me that never answers at its deadline and loads it afresh before the toggles after it', async () => { + let tries = 0 + // The page loaded /me; the first toggle loads it again, and that /me never + // answers. Every later /me answers at once. + const server = stubServer(undefined, undefined, () => (++tries === 2 ? new Promise(() => {}) : Promise.resolve())) + await openPage() + expect(await screen.findByText('2 / 3')).toBeInTheDocument() + + fireEvent.click(checkbox('Taco seasoning')) + await waitFor(() => expect(tries).toBe(2)) + fireEvent.click(checkbox('Ground beef')) + expect(queue()).toEqual(['i3 true', 'i1 false']) + + // Past the deadline the hung /me is abandoned; both still wait, in order. + await advance(TOGGLE_DEADLINE) + expect(await within(row('Taco seasoning')).findByText('will sync')).toBeInTheDocument() + expect(within(row('Ground beef')).getByText('will sync')).toBeInTheDocument() + expect(patches(server.fetchMock)).toEqual([]) + + // The next try sends a fresh /me, which the server answers. + await advance(RETRY_FIRST) + await waitFor(() => expect(queue()).toEqual([])) + expect(mes(server.fetchMock)).toBe(3) + expect(patches(server.fetchMock)).toEqual([`${ITEM('i3')} {"needed":true}`, `${ITEM('i1')} {"needed":false}`]) + expect(server.mostInFlight()).toBe(1) + await waitFor(() => expect(screen.queryByText('will sync')).not.toBeInTheDocument()) + expect(screen.getByText('2 / 3')).toBeInTheDocument() + expect(screen.queryByRole('alert')).not.toBeInTheDocument() + }) + + it('abandons the /me refresh of a csrf 403 that never answers at its deadline and refreshes afresh on the next try', async () => { + let tries = 0 + // The session's token changed: each PATCH is refused for it until a /me + // after the first refusal has answered. The first such refresh never + // answers. + let refusedAt: number | null = null + const server = stubServer( + () => { + if (refusedAt !== null && server.meAnswered() > refusedAt) return undefined + refusedAt ??= server.meAnswered() + return errorResponse(403, 'csrf') + }, + undefined, + () => (refusedAt !== null && ++tries === 1 ? new Promise(() => {}) : Promise.resolve()), + ) + await openPage() + expect(await screen.findByText('2 / 3')).toBeInTheDocument() + + fireEvent.click(checkbox('Taco seasoning')) + await waitFor(() => expect(tries).toBe(1)) + fireEvent.click(checkbox('Ground beef')) + expect(patches(server.fetchMock)).toHaveLength(1) + + // Past the deadline the hung refresh is abandoned; both still wait, in order. + await advance(TOGGLE_DEADLINE) + expect(await within(row('Taco seasoning')).findByText('will sync')).toBeInTheDocument() + expect(within(row('Ground beef')).getByText('will sync')).toBeInTheDocument() + expect(queue()).toEqual(['i3 true', 'i1 false']) + + // The next try is refused again and sends a fresh refresh, which answers. + await advance(RETRY_FIRST) + await waitFor(() => expect(queue()).toEqual([])) + expect(patches(server.fetchMock)).toEqual([ + `${ITEM('i3')} {"needed":true}`, + `${ITEM('i3')} {"needed":true}`, + `${ITEM('i3')} {"needed":true}`, + `${ITEM('i1')} {"needed":false}`, + ]) + await waitFor(() => expect(screen.queryByText('will sync')).not.toBeInTheDocument()) + expect(screen.getByText('2 / 3')).toBeInTheDocument() + expect(screen.queryByRole('alert')).not.toBeInTheDocument() + }) + it('tries a toggle again once the server is back, with no online event from the browser', async () => { const { fetchMock } = stubServer() await openPage()