fix(web): resolve the rollback baseline at failure time, not at issue

Review round 3 on #2091. The baseline was still captured when the write
went out, so overlapping toggles reproduced the original bug through the
new machinery: toggle 1 PUTs `true` while its reload is pending, toggle 2
reads the baseline and sees `false`, toggle 1 lands `true`, toggle 2
fails — and its catch pushed the `false` it had closed over, contradicting
both disk and the hook.

`resolve` now reads the record and the list live and takes no fallback,
returning `undefined` when neither has anything to say, so calling it in
the failure handler answers for the state at failure time. Both the
value being written and the rollback go through it, each at the moment
it is needed. The hook says explicitly not to hoist the result.

The regression test drives the exact order — issue 1, issue 2, land 1,
fail 2 — with deferred promises rather than timing, and fails on the
issue-time capture.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UL9retfvAXRgvi6EWk4SY4
Signed-off-by: cliffhall <cliff@futurescale.com>

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<void>((resolve) => {
+      landFirst = resolve;
+    });
+    const second = new Promise<void>((_resolve, reject) => {
+      failSecond = (err) => {
+        reject(err);
+      };
+    });
+    updateServerSettingsSpy
+      .mockImplementationOnce(() => first)
+      .mockImplementationOnce(() => second);
+
+    renderWithMantine(<App />);
+    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<typeof vi.fn>;
+    };
+    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

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
+      // was *before* the writes made since. Build on the last write known to
-      // to have landed while that is the fresher account (#2089).
+      // have landed while that is the fresher account (#2089).
-      const prevSettings = lastPersistedSettings.resolve(
-        activeServerId,
-        current.settings ?? EMPTY_SETTINGS,
-      );
       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
+          // actually on disk rather than the failed edit (#1721).
-          // is set to the baseline rather than cleared: clearing it falls back
+          //
-          // to `persistedPaginatedLists`, read from the same `servers` entry
+          // The baseline is resolved *here*, not captured when this write was
-          // that may be stale, which would show the same wrong value from the
+          // issued: another toggle can land in between, and its value is what
-          // other direction (#2089).
+          // disk holds by the time this one fails. The override is set to that
-          setPaginatedListsOverride(prevSettings.paginatedLists ?? false);
+          // baseline rather than cleared, because clearing it falls back to
-          inspectorClient?.setServerSettings(prevSettings);
+          // `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),

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")).toBeUndefined();
-    expect(api.resolve("A", fallback)).toBe(fallback);
   });
 
   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")).toBe(onDiskB);
-    expect(api.resolve("B", fallback)).toBe(fallback);
   });
 
   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", () => {
+  it("returns nothing when the server has left the list entirely", () => {
-    const { api, setServers } = harness([entry("A")]);
+    const { api, setServers } = harness([entry("A", settings())]);
     api.begin("A").landed(settings({ paginatedLists: true }));
     setServers([]);
-    const fallback = settings();
+    expect(api.resolve("A")).toBeUndefined();
-    expect(api.resolve("A", fallback)).toBe(fallback);
   });
 });

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
+   * The best available answer to "what is on disk for this server", **as of the
-   * recorded write while the list still carries the entry that write was paired
+   * moment it is called**. Returns the recorded write while the list still
-   * with, and `fallback` (the caller's read of the entry) once a fresh list read
+   * carries the entry that write was paired with, otherwise that server's
-   * has replaced it, or when no write has landed for that server this session.
+   * 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: (
+  resolve: (serverId: string) => InspectorServerSettings | undefined;
-    serverId: string,
-    fallback: InspectorServerSettings,
-  ) => InspectorServerSettings;
 }
 
 export interface SettingsWrite {
@@ -130,20 +134,17 @@ export function useLastPersistedSettings(
   }, []);
 
   const resolve = useCallback(
-    (
+    (serverId: string): InspectorServerSettings | undefined => {
-      serverId: string,
-      fallback: InspectorServerSettings,
-    ): InspectorServerSettings => {
-      const current = recordsRef.current.get(serverId);
-      if (!current) return fallback;
       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;
     },