fix(web): scope the OAuth recovery restores to the attempt that made them

Copilot round 2, both findings valid:

- `resumePendingReauth`'s restore could resurrect a challenge whose
  session ended during the awaits — a server switch or disconnect clears
  the slot on purpose. It is now gated on the attempt still targeting
  the live session, client identity included (reconnecting to the same
  server builds a replacement client an id-only check would admit), and
  uses a functional update so a newer deferral queued in the same batch
  wins rather than being read past.
- `prepareOAuthRedirect` cleared whichever resume snapshot was current,
  not the one it wrote. Nothing single-flights that callback, so a
  redirect failing before navigation could delete a later attempt's
  callback-routing state. `writeOAuthResumeSnapshot` now returns the
  serialization it stored, and `clearOwnOAuthResumeSnapshot` removes it
  only while it is still the current one.

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

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

@@ -1142,6 +1142,43 @@ describe("useOAuthRecovery", () => {
       expect(client.handleAuthChallenge).toHaveBeenCalledTimes(1);
     });
 
+    it("does not resurrect a challenge whose session ended mid-resume", async () => {
+      // A server switch clears the slot on purpose. The in-flight attempt then
+      // rejects, finds it empty, and must NOT put its own challenge back — it
+      // belongs to a session that has ended (#2165).
+      let rejectResume: ((err: Error) => void) | undefined;
+      const client = fakeClient({
+        handleAuthChallenge: vi.fn(
+          () =>
+            new Promise((_resolve, reject) => {
+              rejectResume = reject;
+            }),
+        ),
+      });
+      const servers = [entry("a"), entry("b")];
+      const h = harness({ servers, activeServerId: "a", client });
+      await defer(client, h);
+
+      await act(async () => {
+        becomeVisible();
+      });
+      await waitFor(() => expect(rejectResume).toBeDefined());
+
+      h.rerender({ servers, activeServerId: "b", client });
+      await act(async () => {
+        rejectResume!(new Error("token endpoint unreachable"));
+      });
+      await waitFor(() =>
+        expect(toastTitles()).toContain("Could not continue authorization"),
+      );
+
+      // Nothing was restored, so returning to the tab resumes nothing.
+      await act(async () => {
+        becomeVisible();
+      });
+      expect(client.handleAuthChallenge).toHaveBeenCalledTimes(1);
+    });
+
     it("does not resume while disconnected, and resumes on the reconnect", async () => {
       const client = fakeClient({
         handleAuthChallenge: vi.fn().mockResolvedValue({ kind: "satisfied" }),

clients/web/src/hooks/useOAuthRecovery.ts

@@ -44,6 +44,7 @@ import {
   applyOAuthResumeUi,
   buildTabUiSnapshot,
   clearOAuthResumeSnapshot,
+  clearOwnOAuthResumeSnapshot,
   consumeOAuthResumeSnapshot,
   oauthResumeInsufficientScopeMessage,
   oauthResumeToastMessage,
@@ -529,7 +530,7 @@ export function useOAuthRecovery({
       onBeforeOAuthRedirect(authorizationUrl);
       // Write immediately before navigation so implicit client teardown (during
       // connect setup) cannot clear the snapshot after we persist it.
-      writeOAuthResumeSnapshot({
+      const snapshotToken = writeOAuthResumeSnapshot({
         version: 1,
         serverId,
         activeTab,
@@ -548,10 +549,15 @@ export function useOAuthRecovery({
       // read it as an *abandoned* redirect and offer that diagnosis, which is
       // wrong — and the failure has to be reported the way every sibling arm
       // reports one, with the red border (#1621) and the re-auth banner.
+      //
+      // Scoped to the snapshot *this* attempt wrote: nothing single-flights
+      // `prepareOAuthRedirect`, so a later attempt may already have replaced
+      // it, and deleting that one would strand the redirect that is actually
+      // in flight.
       oauthClient
         ?.beginInteractiveAuthorization(authorizationUrl)
         .catch((err: unknown) => {
-          clearOAuthResumeSnapshot();
+          clearOwnOAuthResumeSnapshot(snapshotToken);
           setFailedServerId(serverId);
           showReAuthBanner(serverId, err);
         });
@@ -851,13 +857,26 @@ export function useOAuthRecovery({
         // the recovery was delivered. It still is owed, so restore it and let
         // the next trigger retry rather than losing it silently (#2165).
         //
-        // Only into an empty slot, though: the tab can go hidden during the
+        // Only into the session this attempt belongs to, and only into an
-        // awaits above and a *newer* challenge defer itself into it, and that
+        // empty slot.
-        // one describes the session as it is now. Latest wins — putting this
+        //
-        // attempt's stale challenge back over it would retry authorization
+        // The awaits above can outlive the session entirely — a server switch
-        // data the server has already superseded.
+        // or a disconnect clears the slot on purpose, and restoring into that
-        if (sessionRef.current.pendingReauth === null) {
+        // would resurrect a challenge for a session that has ended. The client
-          setPendingReauth(pending);
+        // identity is part of the check, not just the server id: reconnecting
+        // to the SAME server builds a replacement `InspectorClient`, which an
+        // id-only check would wave through.
+        //
+        // And the tab can go hidden mid-flight with a *newer* challenge
+        // deferring itself into the slot; that one describes the session as it
+        // is now. The restore is a functional update so it sees the queued
+        // state rather than the render-time value, and yields to whatever is
+        // already there — latest wins.
+        if (
+          sessionRef.current.activeServerId === pending.serverId &&
+          sessionRef.current.inspectorClient === client
+        ) {
+          setPendingReauth((prev) => prev ?? pending);
         }
         notifications.show({
           title: "Could not continue authorization",

clients/web/src/lib/oauthResume.test.ts

@@ -11,6 +11,7 @@ import {
   readOAuthResumeSnapshot,
   restoreTabUiFromSnapshot,
   writeOAuthResumeSnapshot,
+  clearOwnOAuthResumeSnapshot,
   type OAuthResumeSnapshot,
 } from "./oauthResume.js";
 import {
@@ -349,6 +350,43 @@ describe("oauthResume", () => {
     }
   });
 
+  it("clearOwnOAuthResumeSnapshot removes only the snapshot it was given", () => {
+    const first = writeOAuthResumeSnapshot({
+      version: 1,
+      serverId: "a",
+      activeTab: "tools",
+      authKind: "reauth",
+      tabUi: {},
+    });
+    // A later attempt replaces it before the first one's failure lands.
+    writeOAuthResumeSnapshot({
+      version: 1,
+      serverId: "b",
+      activeTab: "prompts",
+      authKind: "reauth",
+      tabUi: {},
+    });
+
+    expect(clearOwnOAuthResumeSnapshot(first)).toBe(false);
+    expect(readOAuthResumeSnapshot()?.serverId).toBe("b");
+
+    const second = window.sessionStorage.getItem(OAUTH_RESUME_KEY)!;
+    expect(clearOwnOAuthResumeSnapshot(second)).toBe(true);
+    expect(window.sessionStorage.getItem(OAUTH_RESUME_KEY)).toBeNull();
+  });
+
+  it("clearOwnOAuthResumeSnapshot is a no-op without a token", () => {
+    writeOAuthResumeSnapshot({
+      version: 1,
+      serverId: "a",
+      activeTab: "tools",
+      authKind: "reauth",
+      tabUi: {},
+    });
+    expect(clearOwnOAuthResumeSnapshot(undefined)).toBe(false);
+    expect(readOAuthResumeSnapshot()?.serverId).toBe("a");
+  });
+
   it("writeOAuthResumeSnapshot swallows setItem failures", () => {
     vi.stubGlobal("sessionStorage", {
       getItem: () => null,
@@ -368,6 +406,17 @@ describe("oauthResume", () => {
     ).not.toThrow();
   });
 
+  it("clearOwnOAuthResumeSnapshot swallows storage failures", () => {
+    vi.stubGlobal("sessionStorage", {
+      getItem: () => {
+        throw new Error("blocked");
+      },
+      setItem: () => {},
+      removeItem: () => {},
+    });
+    expect(clearOwnOAuthResumeSnapshot("{}")).toBe(false);
+  });
+
   it("clearOAuthResumeSnapshot swallows removeItem failures", () => {
     vi.stubGlobal("sessionStorage", {
       getItem: () => null,
@@ -422,6 +471,10 @@ describe("oauthResume", () => {
     it("clearOAuthResumeSnapshot is a no-op", () => {
       expect(() => clearOAuthResumeSnapshot()).not.toThrow();
     });
+
+    it("clearOwnOAuthResumeSnapshot is a no-op", () => {
+      expect(clearOwnOAuthResumeSnapshot("{}")).toBe(false);
+    });
   });
 
   it("restoreTabUiFromSnapshot returns early when tabUi is undefined", () => {

clients/web/src/lib/oauthResume.ts

@@ -168,14 +168,26 @@ export function restoreTabUiFromSnapshot(
   }
 }
 
-export function writeOAuthResumeSnapshot(snapshot: OAuthResumeSnapshot): void {
+/**
+ * Persist the snapshot, returning the exact serialization stored — the
+ * "attempt token" {@link clearOwnOAuthResumeSnapshot} matches against (#2165).
+ *
+ * `undefined` when nothing was stored (no `window`, privacy mode, quota), so a
+ * caller holding one knows a clear is meaningful.
+ */
+export function writeOAuthResumeSnapshot(
+  snapshot: OAuthResumeSnapshot,
+): string | undefined {
   if (typeof window === "undefined") {
-    return;
+    return undefined;
   }
+  const serialized = JSON.stringify(snapshot);
   try {
-    window.sessionStorage.setItem(OAUTH_RESUME_KEY, JSON.stringify(snapshot));
+    window.sessionStorage.setItem(OAUTH_RESUME_KEY, serialized);
+    return serialized;
   } catch {
     // Best-effort — privacy mode / quota.
+    return undefined;
   }
 }
 
@@ -231,6 +243,39 @@ function readLegacyPendingServerSnapshot(): OAuthResumeSnapshot | undefined {
   }
 }
 
+/**
+ * Clear the snapshot **only if it is still the one this attempt wrote** (#2165).
+ *
+ * `prepareOAuthRedirect` has no single-flight guard, so a redirect that fails
+ * before navigating can reject after a *later* attempt has already written its
+ * own snapshot. An unconditional clear there would delete the newer attempt's
+ * callback-routing state, which is a worse failure than the one it is trying
+ * to avoid.
+ *
+ * The token is the stored serialization, which makes two byte-identical
+ * snapshots indistinguishable — deliberately: identical bytes describe the
+ * same redirect, for the same server, with the same shell state, so clearing
+ * "the other one" reaches the same end state.
+ *
+ * Returns whether anything was removed.
+ */
+export function clearOwnOAuthResumeSnapshot(
+  token: string | undefined,
+): boolean {
+  if (typeof window === "undefined" || token === undefined) {
+    return false;
+  }
+  try {
+    if (window.sessionStorage.getItem(OAUTH_RESUME_KEY) !== token) {
+      return false;
+    }
+    window.sessionStorage.removeItem(OAUTH_RESUME_KEY);
+    return true;
+  } catch {
+    return false;
+  }
+}
+
 export function clearOAuthResumeSnapshot(): void {
   if (typeof window === "undefined") {
     return;