fix: terminate the OAuth connection-details refresh promise chain by cliffhall · Pull Request #2236 · modelcontextprotocol/inspector

cliffhall · 2026-09-04T00:02:02Z

The bug

clients/web/src/hooks/useOAuthRecovery.ts, the connection-details refresh effect, voided inspectorClient.getOAuthState() with a lone .then and no rejection handler:

void inspectorClient.getOAuthState().then((state) => { … });

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 @typescript-eslint/no-floating-promises without terminating anything, so every such failure became an unhandled rejection: on the initial refresh, and on every oauthComplete refresh — the latter firing exactly when OAuth has just been exercised and a transient backend failure is most plausible.

Per AGENTS.md, void is only acceptable when the callee owns its failures, with a one-line comment saying so. getOAuthState() has no catch and surfaces no message, and there was no comment. This is the same class as #2165, whose five void-discarded OAuth recovery promises were fixed in #2190 — this call sits in the same file and was missed.

The fix

Terminate the chain with a .catch that clears the panel details rather than leaving the last successful read on screen. The panel’s job is to report the current OAuth state, so holding a stale answer makes it 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 justification AGENTS.md asks for — a synchronous useEffect body is one of the named cases where the caller genuinely cannot await.

Tests

Two cases in useOAuthRecovery.test.tsx:

clears the details when the state read rejects — the read rejects, the details stay undefined, and a later resolving read still populates the panel.
drops a rejected state read that lands after the session ended — the rejection settles after the effect cleanup ran; nothing is written.

Both fail against the unfixed source, and the failure is the bug itself:

⎯⎯⎯⎯ Unhandled Rejection ⎯⎯⎯⎯⎯
⎯⎯⎯⎯ Unhandled Rejection ⎯⎯⎯⎯⎯

An unhandled rejection fails the whole vitest run, which is the #1947 experience the rule exists to prevent.

Verification

npm run format clean, npm run local:gate passes end to end (exit 0) — 7,389 web tests, all four coverage gates, smokes and Storybook.

No screenshots: this is a failure-path fix behind a backend error with no reproducible visual change.

🤖 Generated with Claude Code

https://claude.ai/code/session_01GBkPzbuxgyqiz39ytmQzMB

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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GBkPzbuxgyqiz39ytmQzMB Signed-off-by: cliffhall <cliff@futurescale.com>

Copilot

🟡 Changes recommended

Out-of-order refreshes can clear newer state, and the added tests do not fully verify the intended state transitions.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Fixes unhandled OAuth state-refresh rejections and clears unavailable connection details.

Changes:

Terminates the refresh promise chain with rejection handling.
Adds rejection and effect-cleanup tests.

File summaries

File	Description
`useOAuthRecovery.ts`	Handles failed OAuth-state reads.
`useOAuthRecovery.test.tsx`	Adds rejection-path tests.

Review details

Files reviewed: 2/2 changed files
Comments generated: 3
Review effort level: Balanced

cliffhall · 2026-09-04T02:55:12Z

+          // 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);


Fixed — good catch, and it is a real bug the .catch newly exposed.

Reads are concurrent (an oauthComplete can start a second one while the first is in flight) and nothing ordered them. Added a monotonically increasing sequence, checked in both handlers:

let latest = 0; const refresh = (): void => { const seq = ++latest; void inspectorClient .getOAuthState() .then((state) => { if (cancelled || seq !== latest) return; … }) .catch(() => { if (cancelled || seq !== latest) return; setConnectionInfoOAuthWhenConnected(undefined); }); };

New test ignores an earlier read that rejects after a newer one succeeded covers exactly the sequence you described: read #1 hangs, oauthComplete starts read #2 which resolves, then read #1 rejects last. Without the guard that test fails.

cliffhall · 2026-09-04T02:55:13Z

+      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());


Fixed — you are right, that test could not tell a correct handler from one that just swallowed the error.

clears already-loaded details when a refresh read rejects now seeds a successful read, waits for connectionInfoOAuth to be defined, then makes the oauthComplete refresh reject and asserts the details go back to undefined. Verified by mutation: replacing the catch body with a bare return fails this test and only this test.

cliffhall · 2026-09-04T02:55:14Z

+      const h = harness(props);
+      h.rerender({ ...props, connectionStatus: "disconnected" });
+      await act(async () => {
+        fail(new Error("backend down"));
+      });
+      expect(h.api().connectionInfoOAuth).toBeUndefined();


Fixed, along the lines you suggested. drops a rejected read that lands after the client was replaced now:

starts with a client whose read hangs,

rerenders with a replacement client that resolves and populates the details,

rejects the stale read, and asserts the current details are still defined.

Verified by mutation: removing cancelled from the catch (leaving only the sequence check) fails this test and only this test — so it now detects the guard it exists for.

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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GBkPzbuxgyqiz39ytmQzMB Signed-off-by: cliffhall <cliff@futurescale.com>

cliffhall · 2026-09-04T02:55:31Z

Review round 1 — all three comments addressed

Inline replies will go outdated once this push lands, so mirroring here.

#	Finding	Response
1	Concurrent refreshes can settle out of order; a stale rejection could clear a newer result	Fixed — monotonic sequence guard, checked in both the `then` and the `catch`
2	The rejection test started from an undefined panel, so it passed even if the handler omitted the clear	Fixed — seeds a successful read first, then rejects, and asserts the loaded details clear
3	The post-cleanup test could not detect the `cancelled` check, since `connectionInfoOAuth` is undefined while disconnected anyway	Fixed — reconnects with a replacement client that populates details, then rejects the stale read and asserts they survive

Comment 1 was a genuine bug the new .catch exposed rather than introduced: the pre-existing .then had the same ordering hazard, it just silently kept a stale value instead of clearing one.

Each guard is now covered by exactly one test

Rather than assert the tests are stronger, I mutation-tested them — each guard removed in turn, full file re-run:

Mutation	Result
Drop the sequence check from both handlers	`× ignores an earlier read that rejects after a newer one succeeded` — 1 failed, 103 passed
Catch swallows without clearing	`× clears already-loaded details when a refresh read rejects` — 1 failed, 103 passed
Drop `cancelled` from the catch only	`× drops a rejected read that lands after the client was replaced` — 1 failed, 103 passed

One test per guard, no overlap and no redundancy.

Verification

npm run coverage:web passes clean with the change — 7,390/7,390.

⚠️ Note for anyone reading a red local run: this machine had several concurrent local:gate runs from sibling worktrees while I was verifying, which produces drifting userEvent/debounce timeouts in unrelated files (ServerSettingsModal, InspectorView, ServerImportJsonModal, AppsScreen). I ran a paired control to rule my change out — the unmodified baseline and this branch both pass clean when the machine is idle, and both fail with different, shifting sets of those same timing-shaped tests under load. Nothing in the failing set is in this diff, which touches two files.

Copilot

🟢 Approval recommended

The promise chain is safely terminated, stale async writes are guarded, and the relevant failure paths have focused regression coverage.

Review details

Files reviewed: 2/2 changed files
Comments generated: 0 new
Review effort level: Balanced

Copilot

🟢 Approval recommended

The promise chain is safely terminated, stale writes are guarded, and targeted tests cover the failure modes.

Review details

Files reviewed: 2/2 changed files
Comments generated: 0 new
Review effort level: Balanced

cliffhall · 2026-09-04T03:34:03Z

CI is green.

The one red run was clients/web/src/test/core/auth/revocation.test.ts > shares one deadline across grants instead of one per grant — a 30 ms real-timer budget assertion in core/auth, unrelated to this diff (which touches only useOAuthRecovery). It passed on rerun with no code change, so it is a CI-timing flake rather than a regression. Flagging it here in case it recurs often enough to be worth an issue of its own; not fixing it in this PR.

cliffhall added the v2 Issues and PRs for v2 label Sep 4, 2026

cliffhall requested a balanced review from Copilot September 4, 2026 00:02

Copilot started reviewing on behalf of cliffhall September 4, 2026 00:02 View session

Copilot AI reviewed Sep 4, 2026

View reviewed changes

cliffhall requested a balanced review from Copilot September 4, 2026 02:55

Copilot started reviewing on behalf of cliffhall September 4, 2026 02:56 View session

Copilot AI reviewed Sep 4, 2026

View reviewed changes

cliffhall requested a balanced review from Copilot September 4, 2026 02:57

Copilot started reviewing on behalf of cliffhall September 4, 2026 02:57 View session

Copilot AI reviewed Sep 4, 2026

View reviewed changes

Navigation Menu

Uh oh!

fix: terminate the OAuth connection-details refresh promise chain - #2236

fix: terminate the OAuth connection-details refresh promise chain#2236
cliffhall wants to merge 2 commits into
v2/mainfrom
v2/fix/2221-oauth-refresh-unhandled-rejection

cliffhall commented Sep 4, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

cliffhall Sep 4, 2026

Uh oh!

cliffhall Sep 4, 2026

Uh oh!

cliffhall Sep 4, 2026

Uh oh!

cliffhall commented Sep 4, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

cliffhall commented Sep 4, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

cliffhall commented Sep 4, 2026

The bug

The fix

Tests

Verification

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

cliffhall Sep 4, 2026

Choose a reason for hiding this comment

Uh oh!

cliffhall Sep 4, 2026

Choose a reason for hiding this comment

Uh oh!

cliffhall Sep 4, 2026

Choose a reason for hiding this comment

Uh oh!

cliffhall commented Sep 4, 2026

Review round 1 — all three comments addressed

Each guard is now covered by exactly one test

Verification

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟢 Approval recommended

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟢 Approval recommended

Uh oh!

cliffhall commented Sep 4, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants