fix: only let the newest OAuth state read write, and harden its tests

Copilot review round 1.

Refreshes are concurrent — an `oauthComplete` can start a second read while
the first is still in flight — and nothing makes them settle in order. The
new catch made that visible: a slow earlier read could reject *after* a
newer one succeeded and clear the fresh result. Guard both handlers with a
monotonically increasing sequence so only the newest read writes.

The two tests were also too weak to detect their own guards, as the review
pointed out. Both started from an undefined panel, so neither could tell a
correct handler from one that merely swallowed the error:

- The rejection test now seeds a successful read first, then rejects an
  `oauthComplete` refresh and asserts the already-loaded details clear.
- The post-cleanup test now reconnects with a replacement client that
  populates details, then rejects the stale read, and asserts the current
  details survive.
- A third test covers the ordering guard directly.

Each of the three guards has exactly one test that fails without it,
verified by mutation: dropping the sequence check, dropping the clear, and
dropping the `cancelled` check each break one test and no others.

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

clients/web/src/hooks/useOAuthRecovery.test.tsx

@@ -393,52 +393,85 @@ describe("useOAuthRecovery", () => {
       expect(h.api().connectionInfoOAuth).toBeUndefined();
     });
 
-    it("clears the details when the state read rejects", async () => {
-      const client = fakeClient({
-        getOAuthState: vi.fn().mockRejectedValue(new Error("backend down")),
-      });
-      const props: HarnessProps = {
-        servers: [entry("a")],
-        activeServerId: "a",
-        client,
-      };
-      const h = harness(props);
-      await waitFor(() => expect(client.getOAuthState).toHaveBeenCalled());
-      expect(h.api().connectionInfoOAuth).toBeUndefined();
+    it("clears already-loaded details when a refresh read rejects", async () => {
+      let read: () => Promise<unknown> = () =>
+        Promise.resolve({ tokens: { access_token: "t" } });
+      const client = fakeClient({ getOAuthState: vi.fn(() => read()) });
+      const h = harness({ servers: [entry("a")], activeServerId: "a", client });
+      await waitFor(() => expect(h.api().connectionInfoOAuth).toBeDefined());
+
+      // The panel reports the *current* state, so a failed read must clear the
+      // details it already holds rather than leave a stale answer on screen.
+      read = () => Promise.reject(new Error("backend down"));
+      await act(async () => {
+        client.emit("oauthComplete", {});
+      });
+      await waitFor(() => expect(h.api().connectionInfoOAuth).toBeUndefined());
+    });
+
+    it("ignores an earlier read that rejects after a newer one succeeded", async () => {
+      let failFirst: (reason: unknown) => void = () => {};
+      let call = 0;
+      const client = fakeClient({
+        getOAuthState: vi.fn((): Promise<unknown> => {
+          call += 1;
+          if (call === 1) {
+            return new Promise((_resolve, reject) => {
+              failFirst = reject;
+            });
+          }
+          return Promise.resolve({ tokens: { access_token: "t" } });
+        }),
+      });
+      const h = harness({ servers: [entry("a")], activeServerId: "a", client });
+      await waitFor(() =>
+        expect(client.getOAuthState).toHaveBeenCalledTimes(1),
+      );
 
-      // The rejection is terminated, not floated: a later resolving read still
-      // populates the panel, which an unhandled rejection would have prevented
-      // by failing the run.
-      client.getOAuthState = vi
-        .fn()
-        .mockResolvedValue({ tokens: { access_token: "t" } });
       await act(async () => {
         client.emit("oauthComplete", {});
       });
       await waitFor(() => expect(h.api().connectionInfoOAuth).toBeDefined());
+
+      // The stale read settles last. Without the sequence guard its catch
+      // would clear the newer read's result.
+      await act(async () => {
+        failFirst(new Error("backend down"));
+      });
+      expect(h.api().connectionInfoOAuth).toBeDefined();
     });
 
-    it("drops a rejected state read that lands after the session ended", async () => {
-      let fail: (reason: unknown) => void = () => {};
-      const client = fakeClient({
+    it("drops a rejected read that lands after the client was replaced", async () => {
+      let failStale: (reason: unknown) => void = () => {};
+      const stale = fakeClient({
         getOAuthState: vi.fn(
           () =>
             new Promise((_resolve, reject) => {
-              fail = reject;
+              failStale = reject;
             }),
         ),
       });
       const props: HarnessProps = {
         servers: [entry("a")],
         activeServerId: "a",
-        client,
+        client: stale,
       };
       const h = harness(props);
-      h.rerender({ ...props, connectionStatus: "disconnected" });
-      await act(async () => {
-        fail(new Error("backend down"));
+      await waitFor(() => expect(stale.getOAuthState).toHaveBeenCalled());
+
+      // Reconnect. The new client's details are what the panel must keep.
+      const fresh = fakeClient({
+        getOAuthState: vi
+          .fn()
+          .mockResolvedValue({ tokens: { access_token: "t" } }),
       });
-      expect(h.api().connectionInfoOAuth).toBeUndefined();
+      h.rerender({ ...props, client: fresh });
+      await waitFor(() => expect(h.api().connectionInfoOAuth).toBeDefined());
+
+      await act(async () => {
+        failStale(new Error("backend down"));
+      });
+      expect(h.api().connectionInfoOAuth).toBeDefined();
     });
 
     it("drops a state read that lands after the session ended", async () => {

clients/web/src/hooks/useOAuthRecovery.ts

@@ -405,13 +405,20 @@ export function useOAuthRecovery({
     }
     let cancelled = false;
 
+    // Reads are concurrent — an `oauthComplete` can start a second one while
+    // the first is still in flight — and nothing makes them settle in order.
+    // Only the newest read may write, so a slow earlier one cannot overwrite
+    // (or, on rejection, clear) a newer result.
+    let latest = 0;
+
     const refresh = (): void => {
+      const seq = ++latest;
       // void: a synchronous useEffect body cannot await. The chain is
       // terminated below, so the rejection is handled rather than discarded.
       void inspectorClient
         .getOAuthState()
         .then((state) => {
-          if (cancelled) return;
+          if (cancelled || seq !== latest) return;
           setConnectionInfoOAuthWhenConnected(
             state ? oauthDetailsFromConnectionState(state) : undefined,
           );
@@ -420,7 +427,7 @@ export function useOAuthRecovery({
           // The read failed (backend down, 401 on the API token, malformed
           // stored state). Clear rather than keep the last successful read —
           // a stale answer is indistinguishable from a fresh one in the panel.
-          if (cancelled) return;
+          if (cancelled || seq !== latest) return;
           setConnectionInfoOAuthWhenConnected(undefined);
         });
     };