From 45f50be9f8b875688ff63e3bf1e884323fbd3b89 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Mon, 24 Aug 2026 21:06:10 -0400 Subject: [PATCH] fix(web): tear the session down on the two OAuth failures that hold "connecting" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review round 3. Both catches report a failure without ending the attempt, and in each the preceding rejection is an auth-recovery error — which deliberately holds the status at "connecting" rather than moving it to "error". So the toggle spins and the active-server lock is never released: - the auth-recovery arm, when `checkAuthChallengeSatisfied` is what rejected (the retrying `connect()` does reach "error", so only this half was stuck); - the callback leg, whose `resumeAfterOAuth` carries a reconnect that can reject the same way. Disconnect ahead of the EMA guard there, since a stuck session is worth clearing whichever way the error classifies. The fetch log survives a disconnect, so the Network diagnostics this issue is about are unaffected. Both teardowns are asserted by the tests that already cover those arms. Also fixes a stale comment left by round 2: the arming-hooks type carries four hooks, not three. Signed-off-by: cliffhall --- clients/web/src/App.test.tsx | 15 ++++++++++++++- clients/web/src/App.tsx | 16 ++++++++++++++++ 2 files changed, 30 insertions(+), 1 deletion(-) diff --git a/clients/web/src/App.test.tsx b/clients/web/src/App.test.tsx index 83e5f198..1ac0f73f 100644 --- a/clients/web/src/App.test.tsx +++ b/clients/web/src/App.test.tsx @@ -718,7 +718,7 @@ const clientInstances = ( McpIndex as unknown as { __clientInstances: EventTarget[] } ).__clientInstances; -// The mock factory adds three test-only arming hooks to the module namespace. +// The mock factory adds four test-only arming hooks to the module namespace. // Intersecting with `typeof McpIndex` keeps the real module's shape checked and // narrows this to a single cast — `as unknown as` would discard the former and // is what AGENTS.md rules out. @@ -847,6 +847,13 @@ describe("App failed-connection card border (#1621)", () => { // No redirect was prepared: `prepareOAuthRedirect` persists a resume // snapshot before navigating, and nothing wrote one. expect(readOAuthResumeSnapshot()).toBeUndefined(); + // And the attempt was ended. The outer connect rejected with an + // auth-recovery error, which holds the status at "connecting", so without + // an explicit teardown here the toggle would spin forever. + const client = clientInstances[0] as EventTarget & { + disconnect: ReturnType; + }; + expect(client.disconnect).toHaveBeenCalled(); }); it("clears the flag when a new connection attempt starts", async () => { @@ -2597,6 +2604,12 @@ describe("App OAuth callback issuer-binding failures (#1808)", () => { await waitFor(() => expect(screen.getByTestId("errored-server")).toHaveTextContent("A"), ); + // `resumeAfterOAuth` carries the reconnect, which can reject while holding + // the status at "connecting", so the attempt is torn down explicitly. + const client = clientInstances[0] as EventTarget & { + disconnect: ReturnType; + }; + await waitFor(() => expect(client.disconnect).toHaveBeenCalled()); }); // The callback leg can also die before the token exchange, if the persisted diff --git a/clients/web/src/App.tsx b/clients/web/src/App.tsx index e0818b64..b66a92d9 100644 --- a/clients/web/src/App.tsx +++ b/clients/web/src/App.tsx @@ -2902,6 +2902,13 @@ function App() { }); } catch (err) { connectStartRef.current = undefined; + // `resumeAfterOAuth` carries the reconnect, and that reconnect can + // reject with an auth-recovery error — which holds the status at + // `"connecting"` instead of moving it to `"error"`. Nothing downstream + // of here ends the attempt, so without this the toggle is stuck and the + // active-server lock is never released. Before the EMA guard, since a + // stuck session is worth clearing whichever way the error classifies. + await client.disconnect().catch(() => {}); if (isEmaClientNotConfiguredError(err)) { notifications.show({ title: `Cannot connect to "${server.name}"`, @@ -3088,6 +3095,15 @@ function App() { // the same as the challenge being *unsatisfied*, and navigating the // whole page away on the strength of an error would bury it. connectStartRef.current = undefined; + // Tear the session down before reporting, as the sibling OAuth + // catch below does. The outer `connect()` rejected with an + // auth-recovery error, which deliberately holds the status at + // `"connecting"` rather than moving it to `"error"` — so if the + // challenge check is what rejected, nothing else ever ends the + // attempt and the toggle spins while the active-server lock is + // held. The fetch log survives a disconnect, so the Network + // diagnostics this issue is about are unaffected. + await client.disconnect().catch(() => {}); setFailedServerId(id); const message = recoveryErr instanceof Error