fix: close three stale-completion holes from the round-16 suppressed block (#2144)

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) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0177mPHAdECD18nLLCTwR5rh
Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
cliffhall 2026-08-28 06:47:31 -04:00
commit b9012da988
2 changed files with 102 additions and 3 deletions

View file

@ -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<void>((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(<AuthTab {...props("a")} />);
await tick();
stdin.write("s"); // A's clear starts
await tick();
rerender(<AuthTab {...props("b")} />);
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.

View file

@ -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");
},