From de058c50164beece3090aa64e28de68daf68e005 Mon Sep 17 00:00:00 2001 From: pt-act <211776491+pt-act@users.noreply.github.com> Date: Mon, 19 Oct 2026 13:48:08 +0100 Subject: [PATCH] fix(dashboard): discard superseded workspace reads (RACE-1) WorkspaceDrawer awaited a file read and then applied it unconditionally, so a slow read could land after a fast one and overwrite the viewer: click file A, then file B, and A's contents paint under B's name. Three sites had this shape - activateSelection, handleEdit, and the create-file branch - and the file had no generation counter of any kind. createReadGuard issues a monotonic ticket; only the newest ticket may apply. runExclusive was not reusable here: it *blocks* concurrent calls, which is right for double-submit (PR #14) and wrong for reads, where every request should proceed and only the latest result may win. In activateSelection the ticket is taken BEFORE the media/non-text early returns. Otherwise selecting an image would not invalidate an in-flight text read, and that text would land on the image viewer - the same bug through a different door. The isCurrent checks guard the error path and the loading flag too, not just setContent: a superseded read must not surface its failure as if it were the current file's, and its finally must not clear a spinner the newer read owns. Extracted as a plain function rather than a hook for the same reason runExclusive was: WorkspaceDrawer is ~1500 lines and imports pdfjs at module scope, which throws DOMMatrix in jsdom, so mounting it in a test hangs rather than fails. readGuard.test.ts drives the ordering directly. Verified: tsc -b clean; eslint 0 errors (the 3 exhaustive-deps warnings on this file pre-exist - confirmed against a stashed tree); 6 new cases in readGuard.test.ts; 20/20 passing across src/pages/Agent/Workspace. The decisive check: reproducing the pre-fix code in isolation - reads with no isCurrent guard, slow A then fast B - fails with "expected 'A' to be 'B'". That is the bug, and the test catches exactly it. --- .../Workspace/components/WorkspaceDrawer.tsx | 32 +++++++- .../Workspace/components/readGuard.test.ts | 82 +++++++++++++++++++ .../Agent/Workspace/components/readGuard.ts | 54 ++++++++++++ 3 files changed, 164 insertions(+), 4 deletions(-) create mode 100644 dashboard/src/pages/Agent/Workspace/components/readGuard.test.ts create mode 100644 dashboard/src/pages/Agent/Workspace/components/readGuard.ts diff --git a/dashboard/src/pages/Agent/Workspace/components/WorkspaceDrawer.tsx b/dashboard/src/pages/Agent/Workspace/components/WorkspaceDrawer.tsx index e73ecf4640..1df9c8a525 100644 --- a/dashboard/src/pages/Agent/Workspace/components/WorkspaceDrawer.tsx +++ b/dashboard/src/pages/Agent/Workspace/components/WorkspaceDrawer.tsx @@ -4,7 +4,7 @@ * without modifying the global active-agent selection. */ -import { useCallback, useEffect, useState } from "react"; +import { useCallback, useEffect, useRef, useState } from "react"; import { App, Tree, @@ -56,6 +56,7 @@ import { saveBlobAsFile } from "../../../../utils/saveBlobAsFile"; import AgentNotReadyScreen from "../../../Chat/components/AgentNotReadyScreen"; import { fileTreeIcon } from "../../../../utils/fileTreeIcon"; import { dedupeFileTreeInfos } from "../../../../utils/fileTreeNodes"; +import { createReadGuard } from "./readGuard"; import { workspaceEntryPath } from "../../../../utils/workspacePath"; import FileViewer from "./FileViewer"; import { @@ -266,6 +267,12 @@ export default function WorkspaceDrawer({ const [content, setContent] = useState(""); const [editMode, setEditMode] = useState(false); const [fileLoading, setFileLoading] = useState(false); + /** + * Monotonic ticket for content reads. See `readGuard.ts` — without it a slow + * read for file A can land after a fast read for file B and overwrite the + * viewer, so the user sees the wrong file's contents. + */ + const contentReadSeq = useRef(0); const [saving, setSaving] = useState(false); const [previewMode, setPreviewMode] = useState(true); const [archiveExporting, setArchiveExporting] = useState(false); @@ -533,6 +540,8 @@ export default function WorkspaceDrawer({ setContent(""); if (isMobile) setMobilePane("viewer"); if (!getMediaKind(dest) && isProbablyText(dest)) { + const guard = createReadGuard(contentReadSeq); + const token = guard.begin(); setFileLoading(true); try { const r = await request<{ content: string }>( @@ -542,9 +551,10 @@ export default function WorkspaceDrawer({ )}`, ), ); + if (!guard.isCurrent(token)) return; setContent(r.content); } finally { - setFileLoading(false); + if (guard.isCurrent(token)) setFileLoading(false); } } } @@ -750,6 +760,11 @@ export default function WorkspaceDrawer({ } setDirEntries([]); if (isMobile) setMobilePane("viewer"); + // Take the ticket BEFORE the early returns: selecting an image or a binary + // must still invalidate an in-flight text read, or that read lands later and + // paints text over the image viewer. + const guard = createReadGuard(contentReadSeq); + const token = guard.begin(); if (getMediaKind(path)) return; if (!isProbablyText(path)) return; setFileLoading(true); @@ -759,14 +774,18 @@ export default function WorkspaceDrawer({ `/agents/${agentId}/workspace/file?path=${encodeURIComponent(path)}`, ), ); + // A newer selection superseded this read; applying it would show the + // wrong file's contents under the newly selected name. + if (!guard.isCurrent(token)) return; setContent(r.content); } catch (err: unknown) { + if (!guard.isCurrent(token)) return; message.error( (err instanceof Error ? err.message : String(err)) || t("workspace.readFailed", "读取失败"), ); } finally { - setFileLoading(false); + if (guard.isCurrent(token)) setFileLoading(false); } }; @@ -883,6 +902,8 @@ export default function WorkspaceDrawer({ setEditMode(true); return; } + const guard = createReadGuard(contentReadSeq); + const token = guard.begin(); setFileLoading(true); try { const r = await request<{ content: string }>( @@ -890,15 +911,18 @@ export default function WorkspaceDrawer({ `/agents/${agentId}/workspace/doc?path=${encodeURIComponent(path)}`, ), ); + // Entering edit mode on a superseded read would edit the wrong file. + if (!guard.isCurrent(token)) return; setContent(r.content); setEditMode(true); } catch (err: unknown) { + if (!guard.isCurrent(token)) return; message.error( (err instanceof Error ? err.message : String(err)) || t("workspace.readFailed", "读取失败"), ); } finally { - setFileLoading(false); + if (guard.isCurrent(token)) setFileLoading(false); } }; diff --git a/dashboard/src/pages/Agent/Workspace/components/readGuard.test.ts b/dashboard/src/pages/Agent/Workspace/components/readGuard.test.ts new file mode 100644 index 0000000000..d2cf6527d9 --- /dev/null +++ b/dashboard/src/pages/Agent/Workspace/components/readGuard.test.ts @@ -0,0 +1,82 @@ +import { describe, expect, it } from "vitest"; +import { createReadGuard, type MutableRef } from "./readGuard"; + +/** + * RACE-1 regression guard. + * + * WorkspaceDrawer awaits a file read and then applies it unconditionally, so a + * slow read for file A can land after a fast read for file B and overwrite the + * viewer. The user then sees A's contents under B's name. + * + * These cases pin the ordering contract directly. The drawer is 1500 lines with + * pdfjs at module scope (which throws DOMMatrix in jsdom), so drive the guard + * rather than mounting it - the same reasoning behind extracting runExclusive. + */ + +function ref(): MutableRef { + return { current: 0 }; +} + +describe("createReadGuard (RACE-1)", () => { + it("treats the newest ticket as current", () => { + const guard = createReadGuard(ref()); + const token = guard.begin(); + expect(guard.isCurrent(token)).toBe(true); + }); + + it("invalidates the earlier ticket when a newer one is taken", () => { + // The core of the bug: B begins after A, so A must no longer apply. + const guard = createReadGuard(ref()); + const slowA = guard.begin(); + const fastB = guard.begin(); + expect(guard.isCurrent(slowA)).toBe(false); + expect(guard.isCurrent(fastB)).toBe(true); + }); + + it("keeps only the LAST of many concurrent reads current", () => { + const guard = createReadGuard(ref()); + const tokens = [guard.begin(), guard.begin(), guard.begin()]; + expect(tokens.map((t) => guard.isCurrent(t))).toEqual([false, false, true]); + }); + + it("never revalidates an old token as new ones arrive", () => { + const guard = createReadGuard(ref()); + const first = guard.begin(); + for (let i = 0; i < 5; i += 1) guard.begin(); + expect(guard.isCurrent(first)).toBe(false); + }); + + it("survives across separate guard instances sharing one ref", () => { + // The drawer recreates the guard on every render, so the counter - not the + // guard object - is what must carry the ordering. + const counter = ref(); + const a = createReadGuard(counter).begin(); + const b = createReadGuard(counter).begin(); + expect(createReadGuard(counter).isCurrent(a)).toBe(false); + expect(createReadGuard(counter).isCurrent(b)).toBe(true); + }); + + it("simulates the out-of-order landing the bug produces", async () => { + // Direct model of the failure: A resolves after B. + const guard = createReadGuard(ref()); + let viewer = ""; + + const readA = (async () => { + const token = guard.begin(); + await new Promise((r) => setTimeout(r, 30)); // slow + if (!guard.isCurrent(token)) return; // the fix + viewer = "A"; + })(); + + const readB = (async () => { + const token = guard.begin(); + await new Promise((r) => setTimeout(r, 5)); // fast + if (!guard.isCurrent(token)) return; + viewer = "B"; + })(); + + await Promise.all([readA, readB]); + // Without the guard this is "A" - the stale read wins and the viewer lies. + expect(viewer).toBe("B"); + }); +}); diff --git a/dashboard/src/pages/Agent/Workspace/components/readGuard.ts b/dashboard/src/pages/Agent/Workspace/components/readGuard.ts new file mode 100644 index 0000000000..64ea45ec69 --- /dev/null +++ b/dashboard/src/pages/Agent/Workspace/components/readGuard.ts @@ -0,0 +1,54 @@ +/** + * Latest-write-wins guard for async reads. + * + * A drawer that awaits a file read and then applies it unconditionally has no + * way to tell a fresh response from a stale one. Click slow file A, then fast + * file B: B renders, then A's response lands last and overwrites it. The user + * sees the wrong file's contents under the right file's name. + * + * `runExclusive` does not help here — it *blocks* concurrent calls, which is + * right for double-submit and wrong for reads. Reads should all proceed, and + * only the newest one may apply. + * + * This is deliberately not a React hook: the caller holds the counter in a ref + * so it survives re-renders, and keeping it a plain function makes the ordering + * testable without mounting a 1500-line drawer. + */ + +/** Minimal shape of a React `useRef` result — avoids importing React here. */ +export interface MutableRef { + current: T; +} + +/** A ticket issued by `beginRead`. */ +export interface ReadTicket { + /** Issues a ticket and marks it as the newest request. */ + begin: () => number; + /** True only if `token` is still the newest ticket issued. */ + isCurrent: (token: number) => boolean; +} + +/** + * Create a guard whose `isCurrent` is true only for the most recent `begin`. + * + * ```ts + * const guard = createReadGuard(seqRef); + * const token = guard.begin(); + * const r = await request(...); + * if (!guard.isCurrent(token)) return; // a newer read superseded this one + * setContent(r.content); + * ``` + * + * The counter only ever increases, so a token can never be revalidated by + * wrapping around, and a caller that forgets to check simply keeps the old + * (buggy) behaviour rather than introducing a new one. + */ +export function createReadGuard(ref: MutableRef): ReadTicket { + return { + begin: () => { + ref.current += 1; + return ref.current; + }, + isCurrent: (token: number) => ref.current === token, + }; +}