From b9012da988cafddaa0f16cae422b30a3f13eeefe Mon Sep 17 00:00:00 2001 From: cliffhall Date: Fri, 28 Aug 2026 06:47:31 -0400 Subject: [PATCH] fix: close three stale-completion holes from the round-16 suppressed block (#2144) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copilot's round 16 reported "no new comments", but its suppressed block carried three real findings. - `AuthTab` released `clearInFlightRef` before checking whether the completion was still current, so server A settling after the user moved to B and started a clear there dropped B's lock and let a second B clear run concurrently against the same store entry. `current()` is now checked before either shared ref is touched: a stale clear owns nothing. - Switching servers retired the clear but left `ownClearRef` set. The retired clear returns before its `oauthRevision` bump, so the marker had no bump to skip and would swallow the first unrelated revision on the newly selected server, stranding that server's banner. - The web session check compared only the server id, which a disconnect/reconnect to the SAME server passes — that flow builds a replacement `InspectorClient`, so the old clear would have run its session-wide cleanup against the new session. The client identity is part of the check now. Both new tests were verified to fail with their fix reverted. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_0177mPHAdECD18nLLCTwR5rh Signed-off-by: cliffhall --- clients/tui/__tests__/AuthTab.test.tsx | 40 +++++++++++++++++ clients/tui/src/components/AuthTab.tsx | 13 +++++- .../web/src/hooks/useOAuthRecovery.test.tsx | 44 +++++++++++++++++++ clients/web/src/hooks/useOAuthRecovery.ts | 8 +++- 4 files changed, 102 insertions(+), 3 deletions(-) diff --git a/clients/tui/__tests__/AuthTab.test.tsx b/clients/tui/__tests__/AuthTab.test.tsx index a330cab8..213416c5 100644 --- a/clients/tui/__tests__/AuthTab.test.tsx +++ b/clients/tui/__tests__/AuthTab.test.tsx @@ -187,6 +187,46 @@ describe("AuthTab", () => { expect(onClearOAuth).toHaveBeenCalledTimes(2); }); + // A stale completion owns nothing: server A settling after the user moved to + // B and started a clear there must not drop B's lock, or a second B clear + // could run concurrently against the same store entry. + it("a stale completion does not release the current server's lock", async () => { + const settlers: Array<() => void> = []; + const onClearOAuth = vi.fn( + () => + new Promise((resolve) => { + settlers.push(resolve); + }), + ); + const props = (serverName: string) => ({ + ...baseProps, + serverName, + onClearOAuth, + inspectorClient: null, + oauthStatus: "idle" as const, + oauthMessage: null, + focused: true, + }); + const { stdin, rerender } = render(); + await tick(); + + stdin.write("s"); // A's clear starts + await tick(); + rerender(); + await tick(); + stdin.write("s"); // B's clear starts + await tick(); + expect(onClearOAuth).toHaveBeenCalledTimes(2); + + // A settles late. B's clear is still running, so its lock must hold. + settlers[0]!(); + await tick(); + stdin.write("s"); + await tick(); + + expect(onClearOAuth).toHaveBeenCalledTimes(2); + }); + // 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. diff --git a/clients/tui/src/components/AuthTab.tsx b/clients/tui/src/components/AuthTab.tsx index 00d713cd..d2160135 100644 --- a/clients/tui/src/components/AuthTab.tsx +++ b/clients/tui/src/components/AuthTab.tsx @@ -123,6 +123,10 @@ export function AuthTab({ useEffect(() => { clearAttemptRef.current++; clearInFlightRef.current = false; + // The retired clear returns before its `oauthRevision` bump, so the marker + // has no bump to skip. Left set, it would swallow the first unrelated + // revision on the server just selected and strand that server's banner. + ownClearRef.current = false; setClearState("idle"); setClearFailure(null); setLastClearDisconnected(false); @@ -231,10 +235,16 @@ export function AuthTab({ serverNameRef.current === attemptServer; void Promise.resolve(onClearOAuth()).then( () => { + // `current()` FIRST, before touching either shared ref. A stale + // completion — server A settling after the user moved to B and + // started a clear there — would otherwise drop B's lock and let a + // second B clear run concurrently. A stale clear owns nothing. + if (!current()) return; clearInFlightRef.current = false; - if (current()) setClearState("cleared"); + setClearState("cleared"); }, (err: unknown) => { + if (!current()) return; clearInFlightRef.current = false; // A rejection is NOT a revocation failure — those come back as // outcomes and are reported through the message line. This is the @@ -246,7 +256,6 @@ export function AuthTab({ // Left set, it would swallow the next *unrelated* revision change // and strand this banner after the OAuth state moved on. ownClearRef.current = false; - if (!current()) return; setClearFailure(err instanceof Error ? err.message : String(err)); setClearState("failed"); }, diff --git a/clients/web/src/hooks/useOAuthRecovery.test.tsx b/clients/web/src/hooks/useOAuthRecovery.test.tsx index fc7f4fa1..9eeed3c3 100644 --- a/clients/web/src/hooks/useOAuthRecovery.test.tsx +++ b/clients/web/src/hooks/useOAuthRecovery.test.tsx @@ -1409,6 +1409,50 @@ describe("useOAuthRecovery", () => { expect(client.disconnect).not.toHaveBeenCalled(); }); + // A disconnect/reconnect to the SAME server builds a replacement client, so + // an id-only check passes again and the old clear would run its + // session-wide cleanup — including the disconnect — against the new session. + it("does not disconnect a replacement client for the same server", async () => { + let settle: (r: { cleared: boolean }) => void = () => {}; + clearServerOAuthStateMock.mockImplementation( + () => + new Promise((resolve) => { + settle = resolve as typeof settle; + }), + ); + const original = fakeClient(); + const h = harness({ + servers: [entry("a")], + activeServerId: "a", + client: original, + }); + + let done: Promise; + await act(async () => { + done = h.api().clearServerOAuthAndDisconnect(entry("a")); + await Promise.resolve(); + }); + + // Same server, new client — a reconnect while the clear is pending. + const replacement = fakeClient(); + h.rerender({ + servers: [entry("a")], + activeServerId: "a", + client: replacement, + }); + + await act(async () => { + settle({ cleared: true }); + await done; + }); + + // Neither client is torn down: the replacement is the live session and + // must not be touched, and the original is already gone — disconnecting + // it would only drag `finalizeExplicitDisconnect` across the new one. + expect(original.disconnect).not.toHaveBeenCalled(); + expect(replacement.disconnect).not.toHaveBeenCalled(); + }); + it("clears the resume snapshot on an explicit disconnect", () => { writeOAuthResumeSnapshot({ version: 1, diff --git a/clients/web/src/hooks/useOAuthRecovery.ts b/clients/web/src/hooks/useOAuthRecovery.ts index d58afc40..d0d73cdc 100644 --- a/clients/web/src/hooks/useOAuthRecovery.ts +++ b/clients/web/src/hooks/useOAuthRecovery.ts @@ -1265,8 +1265,14 @@ export function useOAuthRecovery({ // The session ref is the live answer. Only the parts that touch the // *session* are gated on it; the store write already happened and the // toast still belongs to the server the user asked about. + // The client identity is part of the check, not just the server id: a + // disconnect/reconnect to the SAME server builds a replacement + // `InspectorClient`, so an id-only check passes again and the old clear + // would run its session-wide cleanup against the new session. const stillTargetsActiveSession = (): boolean => - isActive && sessionRef.current.activeServerId === server.id; + isActive && + sessionRef.current.activeServerId === server.id && + sessionRef.current.inspectorClient === client; const { cleared, revocation } = await clearServerOAuthState({ config: server.config,