fix(web): tear the session down on the two OAuth failures that hold "connecting"
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 <cliff@futurescale.com>
This commit is contained in:
parent
46fabeaf4d
commit
45f50be9f8
2 changed files with 30 additions and 1 deletions
|
|
@ -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<typeof vi.fn>;
|
||||
};
|
||||
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<typeof vi.fn>;
|
||||
};
|
||||
await waitFor(() => expect(client.disconnect).toHaveBeenCalled());
|
||||
});
|
||||
|
||||
// The callback leg can also die before the token exchange, if the persisted
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
Loading…
Reference in a new issue