fix(auth): bound the whole teardown, and scope the TUI clear to its server (#2144)

Copilot review round 7.

- The five-second timeout was per request while grants are revoked
  sequentially, so a server with N issuer slots could block the disconnect for
  N x 5s — the "short timeout" bounded a request rather than anything the user
  feels. It is now one budget for the whole call: each grant gets the remaining
  time, and a grant reached after it is exhausted is reported as never
  attempted rather than starting a fresh five seconds. `--relogin` shares one
  budget across its two key spellings for the same reason.
- `AuthTab` called the same settle handler on rejection, so a failed local
  clear or disconnect announced "OAuth state cleared". A rejection is not a
  revocation failure — those come back as outcomes — so it now has its own
  path and reports the real error. The completion is also scoped: a clear
  carries the server it started on and an attempt number, and a switch retires
  it, so server A's confirmation cannot appear under B.
- `clearOAuthAttemptRef` is now retired by the server-switch effect alongside
  `disconnectAttemptRef`; without that an A -> B -> A round trip left both the
  token and the captured name matching again. `handleClearOAuth` also
  revalidates after `await disconnectInspector()`, since a switch during that
  second await would have put the revision bump on the new selection.

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

clients/cli/src/clear-stored-auth-for-relogin.ts

@@ -3,6 +3,7 @@ import {
   resetNodeOAuthStorageCache,
 } from "@inspector/core/auth/node/storage-node.js";
 import {
+  DEFAULT_REVOCATION_TIMEOUT_MS,
   revokeStoredOAuthTokens,
   type TokenRevocationOutcome,
 } from "@inspector/core/auth/revocation.js";
@@ -92,13 +93,26 @@ async function revokeStoredKeys(
   keys: string[],
 ): Promise<TokenRevocationOutcome | undefined> {
   const fetchFn = createProxyFetch() ?? fetch;
+  // One budget across both keys, for the same reason `revokeStoredOAuthTokens`
+  // shares one across grants: two keys would otherwise double the wait a user
+  // feels before `--relogin` gets on with the local delete.
+  const deadlineAt = Date.now() + DEFAULT_REVOCATION_TIMEOUT_MS;
   let reported: TokenRevocationOutcome | undefined;
   let lastSkip: TokenRevocationOutcome | undefined;
   for (const key of new Set(keys)) {
+    const remainingMs = deadlineAt - Date.now();
+    if (remainingMs <= 0) {
+      reported ??= {
+        status: "failed",
+        detail: `the ${DEFAULT_REVOCATION_TIMEOUT_MS}ms revocation budget was exhausted before "${key}" was attempted`,
+      };
+      continue;
+    }
     const outcome = await revokeStoredOAuthTokens({
       serverUrl: key,
       storage,
       fetchFn,
+      timeoutMs: remainingMs,
     });
     if (outcome.status === "skipped" && outcome.reason === "no_tokens") {
       lastSkip = outcome;

clients/tui/__tests__/AuthTab.test.tsx

@@ -138,10 +138,13 @@ describe("AuthTab", () => {
     expect(lastFrame() ?? "").toContain("OAuth state cleared");
   });
 
-  // A failed revocation still cleared local state, and the failure is reported
-  // through the message line — so the confirmation must not hang forever.
-  it("settles the confirmation even when the clear rejects", async () => {
-    const onClearOAuth = vi.fn(() => Promise.reject(new Error("nope")));
+  // A rejection is NOT a revocation failure — those come back as outcomes and
+  // are reported through the message line. This is the local clear or the
+  // disconnect itself failing, so reporting success would be a plain lie.
+  it("reports a rejected clear as a failure, not as success", async () => {
+    const onClearOAuth = vi.fn(() =>
+      Promise.reject(new Error("keychain locked")),
+    );
     const { lastFrame, stdin } = render(
       <AuthTab
         {...baseProps}
@@ -155,7 +158,57 @@ describe("AuthTab", () => {
     await tick();
     stdin.write("s");
     await tick();
-    expect(lastFrame() ?? "").toContain("OAuth state cleared");
+    const frame = lastFrame() ?? "";
+    expect(frame).toContain("Could not clear OAuth state");
+    expect(frame).toContain("keychain locked");
+    expect(frame).not.toContain("OAuth state cleared.");
+    expect(frame).not.toContain("Clearing OAuth state");
+  });
+
+  // A clear started on server A must not confirm under server B: it is a
+  // bounded network request now, so it can settle after the user has moved on.
+  it("does not confirm a clear that settles after the server changed", async () => {
+    let settle: () => void = () => {};
+    const onClearOAuth = vi.fn(
+      () =>
+        new Promise<void>((resolve) => {
+          settle = resolve;
+        }),
+    );
+    const { lastFrame, stdin, rerender } = render(
+      <AuthTab
+        {...baseProps}
+        serverName="a"
+        onClearOAuth={onClearOAuth}
+        inspectorClient={null}
+        oauthStatus="idle"
+        oauthMessage={null}
+        focused
+      />,
+    );
+    await tick();
+    stdin.write("s");
+    await tick();
+    expect(lastFrame() ?? "").toContain("Clearing OAuth state");
+
+    rerender(
+      <AuthTab
+        {...baseProps}
+        serverName="b"
+        onClearOAuth={onClearOAuth}
+        inspectorClient={null}
+        oauthStatus="idle"
+        oauthMessage={null}
+        focused
+      />,
+    );
+    await tick();
+    settle();
+    await tick();
+
+    const frame = lastFrame() ?? "";
+    expect(frame).not.toContain("OAuth state cleared");
+    expect(frame).not.toContain("Clearing OAuth state");
   });
 
   // #2144 — a revocation failure is a *partial* success: the local state really

clients/tui/src/App.tsx

@@ -464,6 +464,11 @@ function App({
     // server name matching again, so the stale rejection would land on the
     // re-selected A. Retiring the token on every switch is what closes that.
     disconnectAttemptRef.current++;
+    // Same reasoning for the clear (#2144): the A → B → A round trip would
+    // otherwise leave both its attempt token and the captured server name
+    // matching again, so a clear started on the first A could publish into the
+    // re-selected one.
+    clearOAuthAttemptRef.current++;
     setDisconnectError(null);
     const stepUp = pendingStepUpRef.current;
     if (stepUp && selectedServer && stepUp.serverName !== selectedServer) {
@@ -983,6 +988,14 @@ function App({
     setConnectError(null);
     if (inspectorStatus === "connected" || inspectorStatus === "connecting") {
       await disconnectInspector();
+      // Revalidate: the disconnect is a second await, and a switch during it
+      // would make the revision bump below land on the new selection.
+      if (
+        clearOAuthAttemptRef.current !== attempt ||
+        selectedServerRef.current !== attemptServer
+      ) {
+        return;
+      }
     }
     setOauthRevision((n) => n + 1);
   }, [

clients/tui/src/components/AuthTab.tsx

@@ -90,9 +90,16 @@ export function AuthTab({
   const [oauthState, setOauthState] = useState<
     OAuthConnectionState | undefined
   >(undefined);
-  const [clearState, setClearState] = useState<"idle" | "clearing" | "cleared">(
-    "idle",
-  );
+  const [clearState, setClearState] = useState<
+    "idle" | "clearing" | "cleared" | "failed"
+  >("idle");
+  const [clearFailure, setClearFailure] = useState<string | null>(null);
+  /**
+   * The server a clear was started for, and a sequence number. The clear is a
+   * bounded network request now, so it can settle after the user has moved on
+   * — at which point server A's confirmation must not appear under server B.
+   */
+  const clearAttemptRef = useRef(0);
   /**
    * Set synchronously when a clear starts, and consumed by the reset effect
    * below. The clear itself bumps `oauthRevision` on its way out, so without
@@ -100,6 +107,18 @@ export function AuthTab({
    * which of the two lands first is not something the ordering guarantees.
    */
   const ownClearRef = useRef(false);
+  const serverNameRef = useRef(serverName);
+  useEffect(() => {
+    serverNameRef.current = serverName;
+  }, [serverName]);
+  // A new selection retires any in-flight clear and drops the previous one's
+  // banner: neither belongs to the server now on screen.
+  useEffect(() => {
+    clearAttemptRef.current++;
+    setClearState("idle");
+    setClearFailure(null);
+    setLastClearDisconnected(false);
+  }, [serverName]);
   const [lastClearDisconnected, setLastClearDisconnected] = useState(false);
   const [stepUpChoiceIndex, setStepUpChoiceIndex] = useState(0);
 
@@ -123,6 +142,7 @@ export function AuthTab({
       return;
     }
     setClearState("idle");
+    setClearFailure(null);
     setLastClearDisconnected(false);
   }, [oauthRevision]);
 
@@ -189,13 +209,31 @@ export function AuthTab({
         // first over the same store entry, and the user cannot see that the
         // first is still running except by this state.
         if (clearState === "clearing") return;
+        const attempt = ++clearAttemptRef.current;
+        const attemptServer = serverName;
         setLastClearDisconnected(isLiveConnection);
+        setClearFailure(null);
         ownClearRef.current = true;
         setClearState("clearing");
-        const settle = () => setClearState("cleared");
-        // Settled either way: a failed revocation still cleared local state,
-        // and `handleClearOAuth` reports the failure through the message line.
-        void Promise.resolve(onClearOAuth()).then(settle, settle);
+        // A completion is only ours if nothing has superseded it and the
+        // selection has not moved on.
+        const current = () =>
+          clearAttemptRef.current === attempt &&
+          serverNameRef.current === attemptServer;
+        void Promise.resolve(onClearOAuth()).then(
+          () => {
+            if (current()) setClearState("cleared");
+          },
+          (err: unknown) => {
+            // A rejection is NOT a revocation failure — those come back as
+            // outcomes and are reported through the message line. This is the
+            // local clear or the disconnect itself failing, so announcing
+            // "OAuth state cleared" here would be a plain lie.
+            if (!current()) return;
+            setClearFailure(err instanceof Error ? err.message : String(err));
+            setClearState("failed");
+          },
+        );
       }
     },
     { isActive: focused },
@@ -371,6 +409,12 @@ export function AuthTab({
                   : "OAuth state cleared."}
               </Text>
             )}
+            {clearState === "failed" && (
+              <Text color="red">
+                Could not clear OAuth state
+                {clearFailure ? `: ${clearFailure}` : "."}
+              </Text>
+            )}
           </Box>
         </Box>
       </ScrollView>

clients/web/src/test/core/auth/revocation.test.ts

@@ -507,6 +507,43 @@ describe("revokeStoredOAuthTokens", () => {
     expect(fetchFn).toHaveBeenCalledTimes(2);
   });
 
+  // The timeout is a budget for the WHOLE teardown, not per request. `clear`
+  // deletes every issuer slot, so a per-request bound would let a server with
+  // N of them block the disconnect for N × the timeout — at which point the
+  // "short timeout" bounds a single request and nothing the user feels.
+  it("shares one deadline across grants instead of one per grant", async () => {
+    await storage.saveServerMetadata(
+      SERVER_URL,
+      metadata({ issuer: undefined }),
+    );
+    vi.spyOn(storage, "listIssuers").mockResolvedValue(["a", "b", "c"]);
+    vi.spyOn(storage, "getIssuerTokens").mockImplementation(
+      async (_url: string, issuer: string) => ({
+        access_token: `a-${issuer}`,
+        token_type: "Bearer",
+        refresh_token: `r-${issuer}`,
+      }),
+    );
+    // Every request hangs, so each one burns the whole remaining budget.
+    const fetchFn = vi.fn<typeof fetch>(() => new Promise<Response>(() => {}));
+
+    const started = Date.now();
+    const outcome = await revokeStoredOAuthTokens({
+      serverUrl: SERVER_URL,
+      storage,
+      fetchFn,
+      timeoutMs: 30,
+    });
+    const elapsed = Date.now() - started;
+
+    expect(outcome).toMatchObject({ status: "failed" });
+    // Three grants: with a per-grant bound this would be ~90ms. Generous upper
+    // bound so the assertion is about the shape, not the machine.
+    expect(elapsed).toBeLessThan(70);
+    // The first burned the budget; the rest are reported as never attempted.
+    expect(fetchFn).toHaveBeenCalledTimes(1);
+  });
+
   // A grant bound to an issuer the cached metadata does not describe cannot be
   // revoked — that endpoint belongs to a different authorization server, and
   // sending it another AS's token would hand a credential to a server that

core/auth/revocation.ts

@@ -36,6 +36,11 @@ import type { OAuthStorage } from "./storage.js";
  * committed to, so an unreachable or wedged authorization server must not hold
  * the teardown open — five seconds is long enough for a real endpoint on a slow
  * link and short enough that a dead one is not felt as a hang.
+ *
+ * It is a budget for the **whole** call, not per request: a server with several
+ * issuer-bound grants revokes them sequentially against one shared deadline, so
+ * the bound the user feels is this number regardless of how many grants the
+ * clear is about to delete.
  */
 export const DEFAULT_REVOCATION_TIMEOUT_MS = 5000;
 
@@ -525,6 +530,14 @@ async function computeOutcome(
 
     const supportedAuthMethods = revocationAuthMethods(metadata);
     const outcomes: TokenRevocationOutcome[] = [...failures];
+    // ONE deadline for the whole teardown, not one per grant. `clear` deletes
+    // every issuer slot, so a server with N of them would otherwise block the
+    // disconnect for N × the timeout — at which point the "short timeout"
+    // bounds a single request and nothing the user experiences. Grants are
+    // revoked sequentially on purpose (a burst of parallel requests to one
+    // authorization server is not a kindness), so the budget is shared instead.
+    const timeoutMs = params.timeoutMs ?? DEFAULT_REVOCATION_TIMEOUT_MS;
+    const deadlineAt = Date.now() + timeoutMs;
     for (const grant of grants) {
       // Metadata is cached once per server, not per issuer, so it describes
       // whichever authorization server was discovered last. Sending another
@@ -542,6 +555,17 @@ async function computeOutcome(
         });
         continue;
       }
+      const remainingMs = deadlineAt - Date.now();
+      if (remainingMs <= 0) {
+        outcomes.push({
+          status: "failed",
+          endpoint,
+          detail: `the ${timeoutMs}ms revocation budget was exhausted before this grant was attempted${
+            grant.issuer === undefined ? "" : ` (issuer ${grant.issuer})`
+          }`,
+        });
+        continue;
+      }
       outcomes.push(
         await revokeToken({
           endpoint,
@@ -550,7 +574,7 @@ async function computeOutcome(
           clientInformation: grant.clientInformation,
           supportedAuthMethods,
           fetchFn,
-          timeoutMs: params.timeoutMs,
+          timeoutMs: remainingMs,
         }),
       );
     }