DecisionsApp load(): stale response must not overwrite a newer one (last-write-wins race)

changelog.d/tsk-ycfglg-decisions-load-race.md

@@ -0,0 +1,2 @@
+### Fixed
+- DecisionsApp `load()` now guards each state update with a monotonically increasing request sequence, so a stale in-flight response can no longer overwrite newer data when mount, focus refresh, or SSE-driven reloads overlap

desktop/src/apps/DecisionsApp.test.tsx

@@ -440,6 +440,137 @@ describe("DecisionsApp", () => {
     });
   });
 
+  it("last-started load wins over an older stale response", async () => {
+    const olderPending = [singleSelect];
+    const newerPending: Decision[] = [];
+    let callCount = 0;
+    const heldResolvers: Array<() => void> = [];
+
+    const fetchMock = vi.fn().mockImplementation((input: string) => {
+      callCount++;
+      if (callCount <= 3) {
+        return new Promise((resolve) => {
+          heldResolvers.push(() =>
+            resolve({
+              ok: true,
+              status: 200,
+              json: () => {
+                if (input.includes("status=pending") && !input.includes("auth-requests"))
+                  return Promise.resolve(olderPending);
+                if (input.includes("status=answered"))
+                  return Promise.resolve([]);
+                return Promise.resolve({ requests: [] });
+              },
+            }),
+          );
+        });
+      }
+      return Promise.resolve({
+        ok: true,
+        status: 200,
+        json: () => {
+          if (input.includes("status=pending") && !input.includes("auth-requests"))
+            return Promise.resolve(newerPending);
+          if (input.includes("status=answered"))
+            return Promise.resolve([]);
+          return Promise.resolve({ requests: [] });
+        },
+      });
+    });
+
+    vi.stubGlobal("fetch", fetchMock);
+    render(<DecisionsApp windowId="w1" />);
+
+    // Mount load A starts but its fetches are held pending.
+    await act(async () => {
+      await new Promise((r) => setTimeout(r, 50));
+    });
+
+    // Focus refresh triggers load B after the 1 s debounce.
+    window.dispatchEvent(new Event("focus"));
+
+    // Wait for load B to land.
+    await act(async () => {
+      await new Promise((r) => setTimeout(r, 1200));
+    });
+
+    // Now release the held load A with older data.
+    await act(async () => {
+      heldResolvers.forEach((r) => r());
+      await new Promise((r) => setTimeout(r, 0));
+    });
+
+    // Guard against a vacuous pass: if the mount load's loss of the race left
+    // `loading` stuck true, the list is not rendered at all and the stale-data
+    // assert below would pass no matter what state holds.
+    expect(screen.queryByText("Loading...")).toBeNull();
+    // The rendered list must still reflect load B's newer data, not load A's stale data.
+    expect(screen.queryByText(singleSelect.question)).toBeNull();
+  });
+
+  it("stale response held at json() parse must not overwrite a newer load", async () => {
+    // Unlike the test above, load A's FETCHES resolve immediately — it is the
+    // awaited json() parse that is held. The seq guard has already been
+    // evaluated by then, so a check placed before the await cannot catch this.
+    const olderPending = [singleSelect];
+    const newerPending: Decision[] = [];
+    let callCount = 0;
+    const heldJsonResolvers: Array<() => void> = [];
+
+    const fetchMock = vi.fn().mockImplementation((input: string) => {
+      callCount++;
+      const bodyFor = (data: Decision[] | never[]) => {
+        if (input.includes("status=pending") && !input.includes("auth-requests")) return data;
+        if (input.includes("status=answered")) return [];
+        return { requests: [] };
+      };
+      if (callCount <= 3) {
+        return Promise.resolve({
+          ok: true,
+          status: 200,
+          json: () =>
+            new Promise((resolve) => {
+              heldJsonResolvers.push(() => resolve(bodyFor(olderPending)));
+            }),
+        });
+      }
+      return Promise.resolve({
+        ok: true,
+        status: 200,
+        json: () => Promise.resolve(bodyFor(newerPending)),
+      });
+    });
+
+    vi.stubGlobal("fetch", fetchMock);
+    render(<DecisionsApp windowId="w1" />);
+
+    // Load A's fetches resolve; A is now suspended inside await json().
+    await act(async () => {
+      await new Promise((r) => setTimeout(r, 50));
+    });
+
+    // Focus refresh triggers load B after the 1 s debounce; B completes fully.
+    window.dispatchEvent(new Event("focus"));
+    await act(async () => {
+      await new Promise((r) => setTimeout(r, 1200));
+    });
+
+    // Release A's held json() bodies with the older data. A awaits the three
+    // bodies sequentially, so each release lets it register the next held
+    // json(); drain until none remain so A runs to completion (incl. finally).
+    await act(async () => {
+      while (heldJsonResolvers.length > 0) {
+        heldJsonResolvers.splice(0).forEach((r) => r());
+        await new Promise((r) => setTimeout(r, 0));
+      }
+    });
+
+    // Same vacuity guard as above: the list must actually be rendered.
+    expect(screen.queryByText("Loading...")).toBeNull();
+    // B's newer (empty) list must survive; A's stale parse must not land.
+    expect(screen.queryByText(singleSelect.question)).toBeNull();
+  });
+
   it("refreshes the list live when a decision is answered from another surface", async () => {
     // Initially the decision is pending; after the SSE event it moves to answered.
     let answeredElsewhere = false;

desktop/src/apps/DecisionsApp.tsx

@@ -590,8 +590,10 @@ export function DecisionsApp({ windowId: _windowId }: { windowId: string }) {
   const [answered, setAnswered] = useState<Decision[]>([]);
   const [authRequests, setAuthRequests] = useState<AuthRequest[]>([]);
   const [loading, setLoading] = useState(true);
+  const latestSeq = useRef(0);
 
   const load = useCallback(async (opts?: { silent?: boolean }) => {
+    const seq = ++latestSeq.current;
     if (!opts?.silent) setLoading(true);
     try {
       const [pRes, aRes, rRes] = await Promise.all([
@@ -601,16 +603,29 @@ export function DecisionsApp({ windowId: _windowId }: { windowId: string }) {
       ]);
       // Only overwrite a list when its request actually succeeded; a transient
       // failure must not blank out decisions the user can still act on.
-      if (pRes.ok) setPending(asDecisionList(await pRes.json()));
-      if (aRes.ok) setAnswered(asDecisionList(await aRes.json()));
+      // The seq re-check must sit AFTER each awaited json() parse — a newer
+      // load can start while a body is still streaming, and a check placed
+      // before the await would let the stale body land anyway.
+      if (pRes.ok) {
+        const next = asDecisionList(await pRes.json());
+        if (seq === latestSeq.current) setPending(next);
+      }
+      if (aRes.ok) {
+        const next = asDecisionList(await aRes.json());
+        if (seq === latestSeq.current) setAnswered(next);
+      }
       if (rRes.ok) {
         const data = await rRes.json();
         const reqs = (data?.requests ?? data ?? []) as AuthRequest[];
-        setAuthRequests(Array.isArray(reqs) ? reqs : []);
+        if (seq === latestSeq.current) setAuthRequests(Array.isArray(reqs) ? reqs : []);
       }
     } catch {
       // Network error: keep whatever was last loaded in place.
     } finally {
+      // Deliberately NOT seq-guarded: loading tracks this non-silent call's
+      // own lifecycle. Only the mount load is non-silent; if a silent focus
+      // refresh outraces it, a guarded clear would leave "Loading..." stuck
+      // forever with data already on screen.
       if (!opts?.silent) setLoading(false);
     }
   }, []);