From 731203bdf4dbe145c770136d3232b0194230839e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Thu, 6 Aug 2026 20:45:07 +0200 Subject: [PATCH] fix(02-12.1): reconcile without firing the reads it is holding - keep the record entries until /api/auth/me has answered: clearing the cache under a mounted observer made the held tab refetch at once - exclude the account cell from the cancellation, and swallow a repeat trigger, so a burst of 409s cannot kill the request that ends the hold - read the account payload network-only; the default staleTime would have returned the very context the browser was told is wrong --- .../__tests__/record-fence-reconciler.test.ts | 195 ++++++++++++++++++ .../layout/record-session-fence-bridge.tsx | 107 ++++++++-- 2 files changed, 284 insertions(+), 18 deletions(-) create mode 100644 src/components/layout/__tests__/record-fence-reconciler.test.ts diff --git a/src/components/layout/__tests__/record-fence-reconciler.test.ts b/src/components/layout/__tests__/record-fence-reconciler.test.ts new file mode 100644 index 000000000..d803a8b10 --- /dev/null +++ b/src/components/layout/__tests__/record-fence-reconciler.test.ts @@ -0,0 +1,195 @@ +/** + * Reconciling a moved record context without firing the reads it is holding. + * + * Both defects this pins were found in a real browser rather than reasoned + * about, and both look harmless in the source: + * + * * `queryClient.clear()` while the shell is still mounted drops every entry, + * and a mounted observer whose entry disappears refetches at once. The tab + * that had just been told to hold issued `/api/dashboard/snapshot` and + * `/api/medications` under the very context it could not prove; + * * an unfiltered `cancelQueries()` cancels the `/api/auth/me` a previous + * reconcile started. A stale tab produces one 409 per errored query, so + * each new trigger killed the request that would have ended the hold and + * the tab never recovered. + * + * Run against a real `QueryClient`, for the reason `grant-loss.test.ts` gives: + * "the effect calls subscribe" is a claim about source, and this is a claim + * about what a cache full of live observers actually does. + * + * Mutation checks, run: put the `removeQueries` back before the `/me` await → + * "does not refetch a record query it is holding" goes red. Drop the in-flight + * guard → "does not cancel the /me a previous reconcile started" goes red. + */ +import { describe, expect, it, vi, beforeEach, afterEach } from "vitest"; +import { QueryClient, QueryObserver } from "@tanstack/react-query"; + +import { createRecordFenceReconciler } from "@/components/layout/record-session-fence-bridge"; +import { queryKeys } from "@/lib/query-keys"; +import { __resetRecordSessionTransitionForTests } from "@/lib/query-keys/record-session-transition"; + +function client(): QueryClient { + return new QueryClient({ + defaultOptions: { + queries: { retry: false, gcTime: Infinity, staleTime: Infinity }, + }, + }); +} + +/** A record cell with a live observer, exactly as a mounted page holds one. */ +async function mountedRecordQuery( + qc: QueryClient, + fetcher: () => Promise, +) { + const queryKey = ["dashboard", "snapshot"]; + // A real `QueryObserver` with a live subscriber, which is what a mounted page + // holds. Without the subscription the query is inactive, removal can never + // trigger a refetch, and this whole file would pass vacuously. + const observer = new QueryObserver(qc, { queryKey, queryFn: fetcher }); + const unsubscribe = observer.subscribe(() => {}); + await vi.waitFor(() => + expect(observer.getCurrentResult().data).toBeDefined(), + ); + return { queryKey, unsubscribe }; +} + +beforeEach(() => { + __resetRecordSessionTransitionForTests(); +}); + +afterEach(() => { + __resetRecordSessionTransitionForTests(); +}); + +describe("createRecordFenceReconciler", () => { + it("keeps the record entries alive until the account payload has answered", async () => { + // The sequencing IS the fix, and it is what this asserts directly. + // + // The defect was `queryClient.clear()` before the `/me` await: a mounted + // observer whose entry disappears refetches, so the tab that had just been + // told to hold fired its record reads under the context it could not + // prove. A node test has no React render loop, so it cannot observe that + // refetch — asserting "the fetcher was not called again" here would be a + // check that cannot fail. What it CAN observe, deterministically, is the + // ordering the refetch depends on: the entry must still exist while `/me` + // is in flight, and be gone once it has answered. + const qc = client(); + const { queryKey, unsubscribe } = await mountedRecordQuery( + qc, + async () => ({ + value: 1, + }), + ); + + let releaseMe!: () => void; + const meGate = new Promise((resolve) => { + releaseMe = resolve; + }); + let presentWhileInFlight: boolean | null = null; + const refetchAccount = vi.fn(async () => { + // Sampled from INSIDE the request, which is the only moment that matters: + // this is precisely when a mounted observer would react to a removal. + presentWhileInFlight = + qc.getQueryCache().find({ queryKey }) !== undefined; + await meGate; + return { id: "u-1" }; + }); + + createRecordFenceReconciler(qc, refetchAccount)(); + await vi.waitFor(() => expect(refetchAccount).toHaveBeenCalledTimes(1)); + + expect(presentWhileInFlight).toBe(true); + + releaseMe(); + await vi.waitFor(() => + expect(qc.getQueryCache().find({ queryKey })).toBeUndefined(), + ); + unsubscribe(); + }); + + it("removes the stale record entries once the account payload lands", async () => { + // The positive control for the assertion above: a reconciler that simply + // never removed anything would pass it. + const qc = client(); + const { queryKey, unsubscribe } = await mountedRecordQuery( + qc, + async () => ({ + value: 1, + }), + ); + unsubscribe(); + + const refetchAccount = vi.fn(async () => ({ id: "u-1" })); + createRecordFenceReconciler(qc, refetchAccount)(); + + await vi.waitFor(() => + expect(qc.getQueryCache().find({ queryKey })).toBeUndefined(), + ); + // The account cell itself survives — it is the one thing that releases the + // hold, and removing it would be removing the answer. + expect( + qc.getQueryCache().find({ queryKey: queryKeys.authMe() }), + ).toBeDefined(); + }); + + it("does not cancel the /me a previous reconcile started", async () => { + const qc = client(); + let releaseMe!: () => void; + const meGate = new Promise((resolve) => { + releaseMe = resolve; + }); + const refetchAccount = vi.fn(async () => { + await meGate; + return { id: "u-1" }; + }); + + const reconcile = createRecordFenceReconciler(qc, refetchAccount); + // A burst, which is what a stale tab produces: one 409 per errored query. + reconcile(); + reconcile(); + reconcile(); + reconcile(); + + // One request, not four, and not zero — a reconciler that restarted on + // every trigger would cancel the request that ends the hold. + expect(refetchAccount).toHaveBeenCalledTimes(1); + + releaseMe(); + await vi.waitFor(() => + expect( + qc.getQueryCache().find({ queryKey: queryKeys.authMe() })?.state.data, + ).toEqual({ id: "u-1" }), + ); + }); + + it("accepts a fresh trigger once the previous reconcile settled", async () => { + // The in-flight guard must be a latch, not a one-shot: a later, genuine + // disagreement has to be reconciled too. + const qc = client(); + const refetchAccount = vi.fn(async () => ({ id: "u-1" })); + const reconcile = createRecordFenceReconciler(qc, refetchAccount); + + reconcile(); + await vi.waitFor(() => expect(refetchAccount).toHaveBeenCalledTimes(1)); + await new Promise((r) => setTimeout(r, 10)); + reconcile(); + await vi.waitFor(() => expect(refetchAccount).toHaveBeenCalledTimes(2)); + }); + + it("stays held when the account payload cannot be fetched", async () => { + const qc = client(); + const refetchAccount = vi.fn(async () => { + throw new TypeError("offline"); + }); + const reconcile = createRecordFenceReconciler(qc, refetchAccount); + + reconcile(); + await vi.waitFor(() => expect(refetchAccount).toHaveBeenCalledTimes(1)); + await new Promise((r) => setTimeout(r, 10)); + + // The latch released, so a retry is possible — being stuck offline must not + // also mean being stuck unable to try again. + reconcile(); + await vi.waitFor(() => expect(refetchAccount).toHaveBeenCalledTimes(2)); + }); +}); diff --git a/src/components/layout/record-session-fence-bridge.tsx b/src/components/layout/record-session-fence-bridge.tsx index 2630761fd..e6f66f074 100644 --- a/src/components/layout/record-session-fence-bridge.tsx +++ b/src/components/layout/record-session-fence-bridge.tsx @@ -1,6 +1,6 @@ "use client"; -import { useQueryClient } from "@tanstack/react-query"; +import { useQueryClient, type QueryClient } from "@tanstack/react-query"; import { useEffect } from "react"; import { onRecordFenceMismatch } from "@/lib/api/record-fence"; @@ -35,27 +35,98 @@ import { holdForRecordSessionReconcile } from "@/lib/query-keys/record-session-t * this is"), and a single subscriber with a mode flag is how they start * answering each other's. */ + +/** Is this the account payload's cell? */ +function isAccountQuery(queryKey: readonly unknown[]): boolean { + const authMe = queryKeys.authMe() as readonly unknown[]; + return ( + queryKey.length === authMe.length && + queryKey.every((part, i) => part === authMe[i]) + ); +} + +/** + * The reconciliation itself, as a plain function over a `QueryClient`. + * + * Extracted from the effect body for the reason `subscribeToGrantLoss` is: a + * test can drive a real cache through it and check what happened, which is the + * only way to tell a reconciler that holds from one that quietly refetches the + * record it is supposed to be holding. + * + * ## The two things the first version got wrong + * + * **It cleared the cache while the shell was still mounted.** + * `queryClient.clear()` drops every entry, and a mounted observer whose entry + * disappears refetches immediately — so the tab that had just been told to hold + * fired `/api/dashboard/snapshot` and `/api/medications` under the very context + * it could not prove. The removal therefore waits until AFTER `/api/auth/me` + * has answered: by then the hold has unmounted the protected children, so there + * is no observer left to trigger anything. + * + * **It cancelled every in-flight query, including the `/me` a previous + * reconcile had started.** Under a burst of 409s — which is exactly what a + * stale tab produces, one per errored query — each new reconcile cancelled the + * request that would have ended the hold, and the tab never recovered. Two + * fixes, both needed: the cancellation excludes the account cell, and a + * reconcile already in flight swallows a second trigger rather than restarting. + */ +export function createRecordFenceReconciler( + queryClient: QueryClient, + refetchAccount: () => Promise = fetchMe, +): () => void { + let inFlight = false; + + return () => { + // A burst of 409s is one signal, not many. Restarting on each would cancel + // the request that ends the hold and livelock the tab. + if (inFlight) return; + inFlight = true; + + // Hold FIRST. This is what unmounts the protected children, and everything + // below depends on them being gone. + holdForRecordSessionReconcile(); + + // Cancel record traffic, never the account payload: the `/me` below is the + // only thing that can release the hold. + void queryClient.cancelQueries({ + predicate: (query) => !isAccountQuery(query.queryKey), + }); + + void queryClient + .fetchQuery({ + queryKey: queryKeys.authMe(), + queryFn: refetchAccount, + // NETWORK-ONLY, and this is load-bearing rather than a precaution. The + // app's default `staleTime` is not zero, so a plain `fetchQuery` would + // hand back the cached account payload — the one carrying the context + // this browser has just been told is wrong — and release the hold + // without ever asking the server. The whole reconciliation would then + // be a no-op that looked like a success. + staleTime: 0, + }) + .then(() => { + // Now, and not before: the children are unmounted, so removing their + // entries cannot remount an observer that refetches. + queryClient.removeQueries({ + predicate: (query) => !isAccountQuery(query.queryKey), + }); + }) + .catch(() => { + // Offline, or the server is down. The hold stays, which is the correct + // place to be stuck: nothing renders under a context this browser + // cannot prove, and the next successful `/me` releases it. + }) + .finally(() => { + inFlight = false; + }); + }; +} + export function RecordSessionFenceBridge() { const queryClient = useQueryClient(); useEffect(() => { - /** - * Held, then reconciled. `fetchMe` adopts the context and settles the - * transition itself, so nothing here decides what the browser is allowed - * to render next — the server's answer does. - */ - const reconcile = () => { - holdForRecordSessionReconcile(); - void queryClient.cancelQueries(); - queryClient.clear(); - void queryClient - .fetchQuery({ queryKey: queryKeys.authMe(), queryFn: fetchMe }) - .catch(() => { - // Offline, or the server is down. The hold stays, which is the - // correct place to be stuck: nothing renders under a context this - // browser cannot prove, and the next successful `/me` releases it. - }); - }; + const reconcile = createRecordFenceReconciler(queryClient); // The in-flight / cached-response arm, raised by the transport wrapper. const disposeMismatch = onRecordFenceMismatch(reconcile);