diff --git a/clients/web/src/App.test.tsx b/clients/web/src/App.test.tsx index b4e83147..d5dc2ce7 100644 --- a/clients/web/src/App.test.tsx +++ b/clients/web/src/App.test.tsx @@ -2465,6 +2465,62 @@ describe("App paginated list pagination toggle (#1721)", () => { }; expect(lastPush?.paginatedLists).toBe(true); }); + + it("rolls back to a write that landed while this one was in flight (#2089)", async () => { + // Overlapping toggles. The baseline cannot be captured when a write is + // issued: toggle 1 is still in flight when toggle 2 reads it, so toggle 2 + // would close over `false` — and by the time toggle 2 fails, toggle 1 has + // landed `true` on disk. Rolling back to the captured value contradicts + // disk exactly as the stale-list case does; the baseline has to be resolved + // at failure time. The two writes are driven by deferred promises so the + // order — issue 1, issue 2, land 1, fail 2 — is exact rather than timed. + const user = userEvent.setup(); + let landFirst: (() => void) | undefined; + let failSecond: ((err: Error) => void) | undefined; + const first = new Promise((resolve) => { + landFirst = resolve; + }); + const second = new Promise((_resolve, reject) => { + failSecond = (err) => { + reject(err); + }; + }); + updateServerSettingsSpy + .mockImplementationOnce(() => first) + .mockImplementationOnce(() => second); + + renderWithMantine(); + await user.click(screen.getByText("connect")); + await waitFor(() => expect(clientInstances).toHaveLength(1)); + + await user.click(screen.getByText("paginated-on")); + await user.click(screen.getByText("paginated-off")); + await waitFor(() => + expect(updateServerSettingsSpy).toHaveBeenCalledTimes(2), + ); + + await act(async () => { + landFirst?.(); + await first; + }); + await act(async () => { + failSecond?.(new Error("disk full")); + await second.catch(() => undefined); + }); + + // `true` is what the landed write put on disk — not the `false` the second + // toggle read before it got there. + await waitFor(() => + expect(screen.getByTestId("tools-paginated")).toHaveTextContent("true"), + ); + const client = clientInstances[0] as EventTarget & { + setServerSettings: ReturnType; + }; + const lastPush = client.setServerSettings.mock.calls.at(-1)?.[0] as { + paginatedLists?: boolean; + }; + expect(lastPush?.paginatedLists).toBe(true); + }); }); // A background command runs through `runCommandInBackground`, which owns the diff --git a/clients/web/src/App.tsx b/clients/web/src/App.tsx index 4d481efa..bf72364c 100644 --- a/clients/web/src/App.tsx +++ b/clients/web/src/App.tsx @@ -3737,14 +3737,10 @@ function App() { if (!current || activeServerId === undefined) return; // Not `current.settings` directly: that entry only advances on a // successful list read, so once one has failed it describes disk as it - // was *before* the writes made since. Roll back to the last write known - // to have landed while that is the fresher account (#2089). - const prevSettings = lastPersistedSettings.resolve( - activeServerId, - current.settings ?? EMPTY_SETTINGS, - ); + // was *before* the writes made since. Build on the last write known to + // have landed while that is the fresher account (#2089). const next: InspectorServerSettings = { - ...prevSettings, + ...(lastPersistedSettings.resolve(activeServerId) ?? EMPTY_SETTINGS), paginatedLists: value, }; inspectorClient?.setServerSettings(next); @@ -3787,13 +3783,18 @@ function App() { .catch((err: unknown) => { // Persist failed: revert the optimistic override and roll the live // client setting back, so the UI and client reflect the value that's - // actually on disk rather than the failed edit (#1721). The override - // is set to the baseline rather than cleared: clearing it falls back - // to `persistedPaginatedLists`, read from the same `servers` entry - // that may be stale, which would show the same wrong value from the - // other direction (#2089). - setPaginatedListsOverride(prevSettings.paginatedLists ?? false); - inspectorClient?.setServerSettings(prevSettings); + // actually on disk rather than the failed edit (#1721). + // + // The baseline is resolved *here*, not captured when this write was + // issued: another toggle can land in between, and its value is what + // disk holds by the time this one fails. The override is set to that + // baseline rather than cleared, because clearing it falls back to + // `persistedPaginatedLists` — read from a `servers` entry that may be + // stale, showing the same wrong value from the other side (#2089). + const baseline = + lastPersistedSettings.resolve(activeServerId) ?? EMPTY_SETTINGS; + setPaginatedListsOverride(baseline.paginatedLists ?? false); + inspectorClient?.setServerSettings(baseline); notifications.show({ title: "Failed to save pagination setting", message: err instanceof Error ? err.message : String(err), diff --git a/clients/web/src/hooks/useLastPersistedSettings.test.tsx b/clients/web/src/hooks/useLastPersistedSettings.test.tsx index 73f11d27..b7ea2308 100644 --- a/clients/web/src/hooks/useLastPersistedSettings.test.tsx +++ b/clients/web/src/hooks/useLastPersistedSettings.test.tsx @@ -67,24 +67,29 @@ function harness(initial: ServerEntry[]): Harness { } describe("useLastPersistedSettings", () => { - it("falls back to the caller's value when no write has been recorded", () => { + it("reads the list entry when no write has landed for that server", () => { + const onDisk = settings({ paginatedLists: true }); + const { api } = harness([entry("A", onDisk)]); + expect(api.resolve("A")).toBe(onDisk); + }); + + it("returns nothing when the list has no settings and nothing landed", () => { const { api } = harness([entry("A")]); - const fallback = settings({ paginatedLists: true }); - expect(api.resolve("A", fallback)).toBe(fallback); + expect(api.resolve("A")).toBeUndefined(); }); it("returns the recorded write while the entry has not been re-read", () => { - const { api } = harness([entry("A")]); + const { api } = harness([entry("A", settings())]); const written = settings({ paginatedLists: true }); api.begin("A").landed(written); - expect(api.resolve("A", settings())).toBe(written); + expect(api.resolve("A")).toBe(written); }); it("ignores a record made for a different server", () => { - const { api } = harness([entry("A"), entry("B")]); + const onDiskB = settings(); + const { api } = harness([entry("A"), entry("B", onDiskB)]); api.begin("A").landed(settings({ paginatedLists: true })); - const fallback = settings(); - expect(api.resolve("B", fallback)).toBe(fallback); + expect(api.resolve("B")).toBe(onDiskB); }); it("keeps each server's record, so a save on B doesn't discard A's", () => { @@ -92,42 +97,42 @@ describe("useLastPersistedSettings", () => { // this sequence is reachable: A's write lands while list reads are failing, // B is saved from the modal, and only then does a write on A fail. A single // slot would have lost A's baseline and reproduced #2089. - const { api } = harness([entry("A"), entry("B")]); + const { api } = harness([entry("A", settings()), entry("B", settings())]); const writtenA = settings({ paginatedLists: true }); api.begin("A").landed(writtenA); api.begin("B").landed(settings({ autoRefreshOnListChanged: true })); - expect(api.resolve("A", settings())).toBe(writtenA); + expect(api.resolve("A")).toBe(writtenA); }); it("stops trusting the record once a fresh list read replaces the entry", () => { - const { api, setServers } = harness([entry("A")]); + const { api, setServers } = harness([entry("A", settings())]); api.begin("A").landed(settings({ paginatedLists: true })); // A successful `GET /api/servers` rebuilds the list, so the entry is a new // object even when its values are unchanged. That is the signal the list // has caught up, and it is authoritative again from here. const fresh = settings(); setServers([entry("A", fresh)]); - expect(api.resolve("A", fresh)).toBe(fresh); + expect(api.resolve("A")).toBe(fresh); }); it("pairs the record with the entry as of completion, not of issue", () => { // A list read that lands while the write is in flight must not be mistaken // for one that happened after it. The pairing is taken in `landed`, so the // read below — which the write outlives — does not invalidate the record. - const { api, setServers } = harness([entry("A")]); + const { api, setServers } = harness([entry("A", settings())]); const write = api.begin("A"); - setServers([entry("A")]); + setServers([entry("A", settings())]); const written = settings({ paginatedLists: true }); write.landed(written); - expect(api.resolve("A", settings())).toBe(written); + expect(api.resolve("A")).toBe(written); }); it("keeps only the most recent write for a server", () => { - const { api } = harness([entry("A")]); + const { api } = harness([entry("A", settings())]); api.begin("A").landed(settings({ paginatedLists: true })); const later = settings({ autoRefreshOnListChanged: true }); api.begin("A").landed(later); - expect(api.resolve("A", settings())).toBe(later); + expect(api.resolve("A")).toBe(later); }); it("lets the later-issued write win however the two finish", () => { @@ -135,33 +140,32 @@ describe("useLastPersistedSettings", () => { // write with a slow read can complete last. Ordering by completion would // then record a value that a newer write has already replaced on disk, and // the next failed write would roll back to it. - const { api } = harness([entry("A")]); + const { api } = harness([entry("A", settings())]); const first = api.begin("A"); const second = api.begin("A"); const newest = settings({ paginatedLists: true }); second.landed(newest); first.landed(settings({ paginatedLists: false })); - expect(api.resolve("A", settings())).toBe(newest); + expect(api.resolve("A")).toBe(newest); }); it("does not let a straggler resurrect a record a fresh read dropped", () => { // The high-water mark outlives the record, so an earlier write reporting // after `resolve` invalidated things cannot reinstate a superseded value. - const { api, setServers } = harness([entry("A")]); + const { api, setServers } = harness([entry("A", settings())]); const first = api.begin("A"); api.begin("A").landed(settings({ paginatedLists: true })); const fresh = settings(); setServers([entry("A", fresh)]); - expect(api.resolve("A", fresh)).toBe(fresh); + expect(api.resolve("A")).toBe(fresh); first.landed(settings({ paginatedLists: false })); - expect(api.resolve("A", fresh)).toBe(fresh); + expect(api.resolve("A")).toBe(fresh); }); - it("falls back when the server has left the list entirely", () => { - const { api, setServers } = harness([entry("A")]); + it("returns nothing when the server has left the list entirely", () => { + const { api, setServers } = harness([entry("A", settings())]); api.begin("A").landed(settings({ paginatedLists: true })); setServers([]); - const fallback = settings(); - expect(api.resolve("A", fallback)).toBe(fallback); + expect(api.resolve("A")).toBeUndefined(); }); }); diff --git a/clients/web/src/hooks/useLastPersistedSettings.ts b/clients/web/src/hooks/useLastPersistedSettings.ts index 872711d3..dde1b6c4 100644 --- a/clients/web/src/hooks/useLastPersistedSettings.ts +++ b/clients/web/src/hooks/useLastPersistedSettings.ts @@ -68,15 +68,19 @@ export interface LastPersistedSettings { */ begin: (serverId: string) => SettingsWrite; /** - * The best available answer to "what is on disk for this server". Returns the - * recorded write while the list still carries the entry that write was paired - * with, and `fallback` (the caller's read of the entry) once a fresh list read - * has replaced it, or when no write has landed for that server this session. + * The best available answer to "what is on disk for this server", **as of the + * moment it is called**. Returns the recorded write while the list still + * carries the entry that write was paired with, otherwise that server's + * current entry settings, and `undefined` when the list has no such entry (or + * one carrying no settings) and no write has landed for it this session. + * + * Both the record and the list are read live, so calling this in a failure + * handler answers for the state at failure time. Do not hoist the result into + * a variable at the point the write is issued and reuse it on rejection: an + * overlapping write that landed in between would make it stale, which is the + * same class of bug as reading the baseline from a stale `servers` entry. */ - resolve: ( - serverId: string, - fallback: InspectorServerSettings, - ) => InspectorServerSettings; + resolve: (serverId: string) => InspectorServerSettings | undefined; } export interface SettingsWrite { @@ -130,20 +134,17 @@ export function useLastPersistedSettings( }, []); const resolve = useCallback( - ( - serverId: string, - fallback: InspectorServerSettings, - ): InspectorServerSettings => { - const current = recordsRef.current.get(serverId); - if (!current) return fallback; + (serverId: string): InspectorServerSettings | undefined => { const entry = serversRef.current.find((s) => s.id === serverId); + const current = recordsRef.current.get(serverId); + if (!current) return entry?.settings; // Identity, not deep equality: the question is whether the list has been // re-read since the write, not whether the values happen to match. if (current.entry !== entry) { // Superseded — drop it rather than re-checking a dead record on every // later write for this server. recordsRef.current.delete(serverId); - return fallback; + return entry?.settings; } return current.written; },