diff --git a/changelog.d/tsk-ycfglg-decisions-load-race.md b/changelog.d/tsk-ycfglg-decisions-load-race.md new file mode 100644 index 000000000..eb1b6cc6e --- /dev/null +++ b/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 diff --git a/desktop/src/apps/DecisionsApp.test.tsx b/desktop/src/apps/DecisionsApp.test.tsx index 9643382f2..a4cd70bba 100644 --- a/desktop/src/apps/DecisionsApp.test.tsx +++ b/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(); + + // 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(); + + // 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; diff --git a/desktop/src/apps/DecisionsApp.tsx b/desktop/src/apps/DecisionsApp.tsx index aa888c4fe..538fbe309 100644 --- a/desktop/src/apps/DecisionsApp.tsx +++ b/desktop/src/apps/DecisionsApp.tsx @@ -590,8 +590,10 @@ export function DecisionsApp({ windowId: _windowId }: { windowId: string }) { const [answered, setAnswered] = useState([]); const [authRequests, setAuthRequests] = useState([]); 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); } }, []);