From 19a39235a2ce7c387bdb1b55cc1b05c5b5eb1a66 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Thu, 3 Sep 2026 20:01:38 -0400 Subject: [PATCH 1/2] fix: terminate the OAuth connection-details refresh promise chain MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The connection-details refresh effect in useOAuthRecovery voided inspectorClient.getOAuthState() with a lone .then and no rejection handler. In the browser that read goes through the remote OAuth store, so it is a network round trip that can reject — a backend that is down or restarting, a 401 on the API token, malformed stored state. void silences no-floating-promises without terminating anything, so every such failure became an unhandled rejection, on the initial refresh and on every oauthComplete refresh. Terminate the chain with a .catch that clears the panel details rather than leaving the last successful read on screen: the panel reports the current OAuth state, and a stale answer is indistinguishable from a fresh one. The cancelled guard is repeated on the catch so a rejection arriving after unmount does not write. The void stays, now with the one-line justification AGENTS.md asks for — a synchronous useEffect body cannot await. Two tests cover it. Both fail against the unfixed source with unhandled rejections, which is the failure mode itself: an unhandled rejection fails the whole vitest run. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01GBkPzbuxgyqiz39ytmQzMB Signed-off-by: cliffhall --- .../web/src/hooks/useOAuthRecovery.test.tsx | 48 +++++++++++++++++++ clients/web/src/hooks/useOAuthRecovery.ts | 23 ++++++--- 2 files changed, 65 insertions(+), 6 deletions(-) diff --git a/clients/web/src/hooks/useOAuthRecovery.test.tsx b/clients/web/src/hooks/useOAuthRecovery.test.tsx index 91b3f10d0..0ab47c70c 100644 --- a/clients/web/src/hooks/useOAuthRecovery.test.tsx +++ b/clients/web/src/hooks/useOAuthRecovery.test.tsx @@ -393,6 +393,54 @@ 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(); + + // 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()); + }); + + it("drops a rejected state read that lands after the session ended", async () => { + let fail: (reason: unknown) => void = () => {}; + const client = fakeClient({ + getOAuthState: vi.fn( + () => + new Promise((_resolve, reject) => { + fail = reject; + }), + ), + }); + const props: HarnessProps = { + servers: [entry("a")], + activeServerId: "a", + client, + }; + const h = harness(props); + h.rerender({ ...props, connectionStatus: "disconnected" }); + await act(async () => { + fail(new Error("backend down")); + }); + expect(h.api().connectionInfoOAuth).toBeUndefined(); + }); + it("drops a state read that lands after the session ended", async () => { let settle: (value: unknown) => void = () => {}; const client = fakeClient({ diff --git a/clients/web/src/hooks/useOAuthRecovery.ts b/clients/web/src/hooks/useOAuthRecovery.ts index 428352b77..c6b7d184c 100644 --- a/clients/web/src/hooks/useOAuthRecovery.ts +++ b/clients/web/src/hooks/useOAuthRecovery.ts @@ -406,12 +406,23 @@ export function useOAuthRecovery({ let cancelled = false; const refresh = (): void => { - void inspectorClient.getOAuthState().then((state) => { - if (cancelled) return; - setConnectionInfoOAuthWhenConnected( - state ? oauthDetailsFromConnectionState(state) : undefined, - ); - }); + // 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; + setConnectionInfoOAuthWhenConnected( + state ? oauthDetailsFromConnectionState(state) : undefined, + ); + }) + .catch(() => { + // 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; + setConnectionInfoOAuthWhenConnected(undefined); + }); }; const onAmbientAuthChallenge = (): void => { From 79751383da9fb07eafbc1b9881606854ee29dfd7 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Thu, 3 Sep 2026 22:54:56 -0400 Subject: [PATCH 2/2] fix: only let the newest OAuth state read write, and harden its tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) Claude-Session: https://claude.ai/code/session_01GBkPzbuxgyqiz39ytmQzMB Signed-off-by: cliffhall --- .../web/src/hooks/useOAuthRecovery.test.tsx | 81 +++++++++++++------ clients/web/src/hooks/useOAuthRecovery.ts | 11 ++- 2 files changed, 66 insertions(+), 26 deletions(-) diff --git a/clients/web/src/hooks/useOAuthRecovery.test.tsx b/clients/web/src/hooks/useOAuthRecovery.test.tsx index 0ab47c70c..fbe7e6cc5 100644 --- a/clients/web/src/hooks/useOAuthRecovery.test.tsx +++ b/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 () => { + it("clears already-loaded details when a refresh read rejects", async () => { + let read: () => Promise = () => + 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().mockRejectedValue(new Error("backend down")), + getOAuthState: vi.fn((): Promise => { + call += 1; + if (call === 1) { + return new Promise((_resolve, reject) => { + failFirst = reject; + }); + } + return Promise.resolve({ tokens: { access_token: "t" } }); + }), }); - const props: HarnessProps = { - servers: [entry("a")], - activeServerId: "a", - client, - }; - const h = harness(props); - await waitFor(() => expect(client.getOAuthState).toHaveBeenCalled()); - expect(h.api().connectionInfoOAuth).toBeUndefined(); + 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 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" } }), + }); + h.rerender({ ...props, client: fresh }); + await waitFor(() => expect(h.api().connectionInfoOAuth).toBeDefined()); + await act(async () => { - fail(new Error("backend down")); + failStale(new Error("backend down")); }); - expect(h.api().connectionInfoOAuth).toBeUndefined(); + expect(h.api().connectionInfoOAuth).toBeDefined(); }); it("drops a state read that lands after the session ended", async () => { diff --git a/clients/web/src/hooks/useOAuthRecovery.ts b/clients/web/src/hooks/useOAuthRecovery.ts index c6b7d184c..c26cb7d79 100644 --- a/clients/web/src/hooks/useOAuthRecovery.ts +++ b/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); }); };