From b4716a14056c36202b239298c6ba32b0a99cca32 Mon Sep 17 00:00:00 2001 From: Cliff Hall Date: Mon, 15 Jun 2026 08:46:59 -0400 Subject: [PATCH] feat(core): surface mid-session transport failures via lastError (#1489) Closes #1323. Adds an `error`-event-driven `lastError` on useInspectorClient and a mid-session failure toast in App.tsx; the transport onerror is scoped to non-handshake (status !== connecting) so handshake failures stay on the connect() rejection path. Follow-up: #1490. --- clients/web/src/App.test.tsx | 54 ++++++ clients/web/src/App.tsx | 32 +++- .../core/react/useInspectorClient.test.tsx | 92 ++++++++++ .../integration/mcp/inspectorClient.test.ts | 169 ++++++++++++++++++ core/mcp/inspectorClient.ts | 25 ++- core/mcp/inspectorClientEventTarget.ts | 11 ++ core/react/useInspectorClient.ts | 22 +++ 7 files changed, 398 insertions(+), 7 deletions(-) diff --git a/clients/web/src/App.test.tsx b/clients/web/src/App.test.tsx index 68747ec3..daac033c 100644 --- a/clients/web/src/App.test.tsx +++ b/clients/web/src/App.test.tsx @@ -707,6 +707,60 @@ describe("App network-log body-dropped toast", () => { }); }); +describe("App mid-session error toast", () => { + beforeEach(() => { + clientInstances.length = 0; + notificationsMock.show.mockClear(); + vi.mocked(useInspectorClient).mockReturnValue(DEFAULT_USE_INSPECTOR_CLIENT); + }); + + afterEach(() => { + vi.mocked(useInspectorClient).mockReturnValue(DEFAULT_USE_INSPECTOR_CLIENT); + }); + + it("toasts the lastError with a generic title when no server is active", () => { + // `lastError` is set but nothing has been connected, so the active-server + // name ref is empty and the toast falls back to "Connection lost". + vi.mocked(useInspectorClient).mockReturnValue({ + ...DEFAULT_USE_INSPECTOR_CLIENT, + lastError: "stdio subprocess crashed", + }); + renderWithMantine(); + + expect(notificationsMock.show).toHaveBeenCalledTimes(1); + const shown = notificationsMock.show.mock.calls[0][0]; + expect(shown.title).toBe("Connection lost"); + expect(shown.message).toBe("stdio subprocess crashed"); + expect(shown.color).toBe("red"); + }); + + it("names the active server in the toast after a session has connected", async () => { + const user = userEvent.setup(); + renderWithMantine(); + + // Connect first so the active-server name ref is populated with SERVER_A. + await user.click(screen.getByText("connect")); + await waitFor(() => expect(clientInstances).toHaveLength(1)); + + // The transport now dies mid-session: `lastError` becomes set and the + // client's `disconnect` event clears the active server. The name ref + // survives, so the toast still names the server (PlotRocket). + vi.mocked(useInspectorClient).mockReturnValue({ + ...DEFAULT_USE_INSPECTOR_CLIENT, + lastError: "SSE stream dropped", + }); + act(() => { + clientInstances[0].dispatchEvent(new CustomEvent("disconnect")); + }); + + await waitFor(() => expect(notificationsMock.show).toHaveBeenCalled()); + const shown = notificationsMock.show.mock.calls.at(-1)?.[0]; + expect(shown.title).toBe('Connection to "PlotRocket" lost'); + expect(shown.message).toBe("SSE stream dropped"); + expect(shown.color).toBe("red"); + }); +}); + describe("App pending server-initiated request modal", () => { beforeEach(() => { clientInstances.length = 0; diff --git a/clients/web/src/App.tsx b/clients/web/src/App.tsx index ad7814de..b408557e 100644 --- a/clients/web/src/App.tsx +++ b/clients/web/src/App.tsx @@ -666,6 +666,7 @@ function App() { serverInfo, instructions, protocolVersion, + lastError, } = useInspectorClient(inspectorClient); const { tools, @@ -1085,6 +1086,30 @@ function App() { [servers, activeServerId], ); + // Mirror the active server's name into a ref so a mid-session failure toast + // can still name the server: a transport crash dispatches `disconnect`, + // which clears `activeServerId` (and thus `activeServer`) before the + // `lastError` effect below runs, so the ref is the only surviving handle. + const activeServerNameRef = useRef(undefined); + useEffect(() => { + if (activeServer) activeServerNameRef.current = activeServer.name; + }, [activeServer]); + + // Surface a mid-session transport failure (stdio crash, SSE drop, HTTP 5xx) + // as a toast. The handshake case is handled in `onToggleConnection`'s catch; + // this covers the `status: connected → error` transition that fires the + // client's `error` event without rejecting any awaited promise (#1323). + // `lastError` clears at the next connecting edge, so each failure toasts once. + useEffect(() => { + if (!lastError) return; + const name = activeServerNameRef.current; + notifications.show({ + title: name ? `Connection to "${name}" lost` : "Connection lost", + message: lastError, + color: "red", + }); + }, [lastError]); + // `config.type` is optional in the schema (a bare `command: ...` // entry implies stdio), so we materialize the default here rather // than at the render site — the modal's `transport` prop is a @@ -1470,9 +1495,10 @@ function App() { // same behavior by reading from `serverSettings` on the client. await client.connect(); } catch (err) { - // Handshake-only. A mid-session transport failure does not throw, - // so a future error event from InspectorClient is the right hook - // for surfacing those (TODO(#1323)). + // Handshake-only. A mid-session transport failure does not throw; the + // client's `error` event surfaces those, consumed via + // `useInspectorClient`'s `lastError` and toasted in the effect above + // (#1323). connectStartRef.current = undefined; // A 401 from an OAuth-protected server means we have no (valid) token diff --git a/clients/web/src/test/core/react/useInspectorClient.test.tsx b/clients/web/src/test/core/react/useInspectorClient.test.tsx index 7fddd5f2..c1c0dbf5 100644 --- a/clients/web/src/test/core/react/useInspectorClient.test.tsx +++ b/clients/web/src/test/core/react/useInspectorClient.test.tsx @@ -150,6 +150,98 @@ describe("useInspectorClient", () => { expect(result.current.status).toBe("connected"); }); + it("captures lastError from the client's error event", () => { + const client = new FakeInspectorClient({ status: "connected" }); + const { result } = renderHook(() => useInspectorClient(client)); + expect(result.current.lastError).toBeUndefined(); + act(() => { + client.dispatchTypedEvent("error", new Error("stdio subprocess crashed")); + }); + expect(result.current.lastError).toBe("stdio subprocess crashed"); + }); + + it("clears lastError when a new connection attempt begins (connecting)", () => { + const client = new FakeInspectorClient({ status: "connected" }); + const { result } = renderHook(() => useInspectorClient(client)); + act(() => { + client.dispatchTypedEvent("error", new Error("SSE stream dropped")); + }); + expect(result.current.lastError).toBe("SSE stream dropped"); + + // A reconnect drives status → "connecting", which clears the stale error. + act(() => { + client.setStatus("connecting"); + }); + expect(result.current.lastError).toBeUndefined(); + }); + + it("keeps lastError across the error status transition", () => { + const client = new FakeInspectorClient({ status: "connected" }); + const { result } = renderHook(() => useInspectorClient(client)); + // The transport onerror path fires statusChange("error") then error; the + // "error" status must not clear the message the error event carries. + act(() => { + client.setStatus("error"); + client.dispatchTypedEvent("error", new Error("HTTP 503")); + }); + expect(result.current.status).toBe("error"); + expect(result.current.lastError).toBe("HTTP 503"); + }); + + it("keeps lastError across a trailing disconnected transition", () => { + const client = new FakeInspectorClient({ status: "connected" }); + const { result } = renderHook(() => useInspectorClient(client)); + act(() => { + client.dispatchTypedEvent("error", new Error("stdio crashed")); + }); + expect(result.current.lastError).toBe("stdio crashed"); + + // A real crash often trails the error with an onclose → statusChange + // ("disconnected"). The toast effect depends on that NOT clearing + // lastError (only the next "connecting" edge does). + act(() => { + client.setStatus("disconnected"); + }); + expect(result.current.status).toBe("disconnected"); + expect(result.current.lastError).toBe("stdio crashed"); + }); + + it("resets lastError when the client prop changes", () => { + const a = new FakeInspectorClient({ status: "connected" }); + const b = new FakeInspectorClient({ status: "connected" }); + const { result, rerender } = renderHook( + ({ c }: { c: InspectorClientProtocol }) => useInspectorClient(c), + { initialProps: { c: a as InspectorClientProtocol } }, + ); + act(() => { + a.dispatchTypedEvent("error", new Error("boom")); + }); + expect(result.current.lastError).toBe("boom"); + + rerender({ c: b }); + expect(result.current.lastError).toBeUndefined(); + + // The old client's error event is ignored after the swap. + act(() => { + a.dispatchTypedEvent("error", new Error("late")); + }); + expect(result.current.lastError).toBeUndefined(); + }); + + it("resets lastError when client becomes null", () => { + const client = new FakeInspectorClient({ status: "connected" }); + const { result, rerender } = renderHook( + ({ c }: { c: InspectorClientProtocol | null }) => useInspectorClient(c), + { initialProps: { c: client as InspectorClientProtocol | null } }, + ); + act(() => { + client.dispatchTypedEvent("error", new Error("boom")); + }); + expect(result.current.lastError).toBe("boom"); + rerender({ c: null }); + expect(result.current.lastError).toBeUndefined(); + }); + it("reads clientCapabilities lazily from the client and defaults to {} when null", () => { const advertised: ClientCapabilities = { elicitation: { form: {} }, diff --git a/clients/web/src/test/integration/mcp/inspectorClient.test.ts b/clients/web/src/test/integration/mcp/inspectorClient.test.ts index 4772d6d5..30e645d7 100644 --- a/clients/web/src/test/integration/mcp/inspectorClient.test.ts +++ b/clients/web/src/test/integration/mcp/inspectorClient.test.ts @@ -2527,6 +2527,175 @@ describe("InspectorClient", () => { await client.disconnect(); expect(disconnectFired).toBe(true); }); + + it("emits an error event and sets status to error on a mid-session transport failure", async () => { + client = new InspectorClient( + { + type: "stdio", + command: serverCommand.command, + args: serverCommand.args, + }, + { + environment: { transport: createTransportNode }, + }, + ); + + const errors: Error[] = []; + client.addEventListener("error", (event) => { + errors.push(event.detail); + }); + + await client.connect(); + expect(client.getStatus()).toBe("connected"); + + // Simulate the transport dying mid-session (stdio crash / SSE drop / + // HTTP 5xx) by invoking the `onerror` the client attached to the base + // transport — the same callback the SDK fires on a real failure. + const baseTransport = ( + client as unknown as { + baseTransport: { onerror?: (error: Error) => void }; + } + ).baseTransport; + const failure = new Error("stdio subprocess crashed"); + baseTransport.onerror?.(failure); + + expect(client.getStatus()).toBe("error"); + expect(errors).toHaveLength(1); + expect(errors[0]).toBe(failure); + + await client.disconnect(); + }); + + it("still emits error when onclose lands before onerror on a mid-session crash", async () => { + // Many transports fire BOTH onclose and onerror on a real crash, in a + // transport-dependent order. When onclose lands first it flips status to + // "disconnected"; the guard must NOT swallow the trailing onerror's + // reason (its only surface). Regression lock for the #1489 re-review. + client = new InspectorClient( + { + type: "stdio", + command: serverCommand.command, + args: serverCommand.args, + }, + { + environment: { transport: createTransportNode }, + }, + ); + + const errors: Error[] = []; + client.addEventListener("error", (event) => { + errors.push(event.detail); + }); + + await client.connect(); + expect(client.getStatus()).toBe("connected"); + + const baseTransport = ( + client as unknown as { + baseTransport: { + onclose?: () => void; + onerror?: (error: Error) => void; + }; + } + ).baseTransport; + + // onclose first → status "disconnected"; then the trailing onerror. + baseTransport.onclose?.(); + expect(client.getStatus()).toBe("disconnected"); + const failure = new Error("stdio subprocess crashed"); + baseTransport.onerror?.(failure); + + expect(client.getStatus()).toBe("error"); + expect(errors).toHaveLength(1); + expect(errors[0]).toBe(failure); + + await client.disconnect(); + }); + + it("does not emit an error event when the handshake rejects connect()", async () => { + // A transport whose start() rejects makes connect() reject (handshake + // failure). The caller gets the reason via the rejected promise, so the + // client must NOT also dispatch the `error` event (which is reserved for + // non-awaited, mid-session failures). See #1323. + const failingFactory = () => ({ + transport: { + start: () => Promise.reject(new Error("handshake refused")), + send: async () => {}, + close: async () => {}, + onclose: undefined, + onerror: undefined, + onmessage: undefined, + sessionId: undefined, + } as unknown as import("@modelcontextprotocol/sdk/shared/transport.js").Transport, + }); + client = new InspectorClient( + { type: "streamable-http", url: "http://localhost:1/never" }, + { environment: { transport: failingFactory } }, + ); + + let errorFired = false; + client.addEventListener("error", () => { + errorFired = true; + }); + + await expect(client.connect()).rejects.toThrow(/handshake refused/); + expect(client.getStatus()).toBe("error"); + expect(errorFired).toBe(false); + }); + + it("does not emit error or flip to error when onerror fires during the handshake", async () => { + // The transport listeners are attached before the handshake runs, so a + // transport that reports a connect-time error via `onerror` (some SDK + // transports do this in addition to rejecting connect()) must NOT be + // treated as a mid-session failure: that would double-report a handshake + // error the awaited connect() rejection already surfaces. See #1323. + let rejectStart: (error: Error) => void = () => {}; + const startPromise = new Promise((_resolve, reject) => { + rejectStart = reject; + }); + const transport = { + start: () => startPromise, + send: async () => {}, + close: async () => {}, + onclose: undefined as undefined | (() => void), + onerror: undefined as undefined | ((error: Error) => void), + onmessage: undefined, + sessionId: undefined, + }; + const factory = () => ({ + transport: + transport as unknown as import("@modelcontextprotocol/sdk/shared/transport.js").Transport, + }); + client = new InspectorClient( + { type: "streamable-http", url: "http://localhost:1/never" }, + { environment: { transport: factory } }, + ); + + let errorFired = false; + client.addEventListener("error", () => { + errorFired = true; + }); + + // Kick off connect() without awaiting — it parks on the hanging start(). + // `status` is "connecting" and `onerror` is wired by this point. + const pending = client.connect().catch(() => {}); + expect(client.getStatus()).toBe("connecting"); + + // A connect-time transport error arrives via onerror. + transport.onerror?.(new Error("socket error mid-handshake")); + + // It must be ignored: still connecting, no error event dispatched. + expect(client.getStatus()).toBe("connecting"); + expect(errorFired).toBe(false); + + // Unblock the handshake so connect() settles (and tears down cleanly). + rejectStart(new Error("aborted")); + await pending; + // connect()'s catch transitions to "error" via the rejection path — + // still without dispatching the `error` event. + expect(client.getStatus()).toBe("error"); + expect(errorFired).toBe(false); + }); }); describe("Sampling Requests", () => { diff --git a/core/mcp/inspectorClient.ts b/core/mcp/inspectorClient.ts index 588f5740..0f1ab794 100644 --- a/core/mcp/inspectorClient.ts +++ b/core/mcp/inspectorClient.ts @@ -436,6 +436,22 @@ export class InspectorClient extends InspectorClientEventTarget { } }; baseTransport.onerror = (error: Error) => { + // Suppress ONLY the handshake case. These listeners are attached before + // the handshake runs (see connect()), so an SDK transport that reports a + // connect-time error via `onerror` — in addition to rejecting connect() + // — would otherwise dispatch the `error` event for a failure the awaited + // connect() rejection already surfaces, double-reporting it. "connecting" + // is precisely that state: the only one with a pending awaited connect() + // that will reject. + // + // We deliberately do NOT guard on `!== "connected"`: on a real + // mid-session crash many transports fire BOTH `onclose` and `onerror`, + // and the order is transport-dependent. If `onclose` lands first it flips + // status to "disconnected", so a "connected"-only guard would swallow the + // reason that the trailing `onerror` carries (its sole surface). Firing + // from any non-"connecting" state captures the reason regardless of + // ordering. + if (this.status === "connecting") return; this.status = "error"; this.dispatchTypedEvent("statusChange", this.status); this.dispatchTypedEvent("error", error); @@ -1021,10 +1037,11 @@ export class InspectorClient extends InspectorClientEventTarget { } catch (error) { this.status = "error"; this.dispatchTypedEvent("statusChange", this.status); - this.dispatchTypedEvent( - "error", - error instanceof Error ? error : new Error(String(error)), - ); + // Deliberately do NOT dispatch the `error` event here: this is the + // awaited `connect()` path, so re-throwing hands the reason straight to + // the caller. The `error` event is reserved for non-awaited transitions + // (the transport `onerror` above), where there is no promise to reject. + // Dispatching here too would double-report a handshake failure. throw error; } } diff --git a/core/mcp/inspectorClientEventTarget.ts b/core/mcp/inspectorClientEventTarget.ts index fd5d0147..934bc1c1 100644 --- a/core/mcp/inspectorClientEventTarget.ts +++ b/core/mcp/inspectorClientEventTarget.ts @@ -61,6 +61,17 @@ export interface InspectorClientEventMap { fetchRequest: FetchRequestEntry; /** Fired when an in-flight fetch's response body is read asynchronously. */ fetchRequestBodyUpdate: { id: string; responseBody: string }; + /** + * Fired whenever the client transitions `status` to `"error"` from a path + * that is NOT an awaited promise — i.e. a mid-session transport failure + * (stdio subprocess crash, SSE stream drop, HTTP 5xx) surfaced via the + * transport's `onerror`. The `Error` carries the reason (`.message`, + * optional `.cause`). Handshake failures are NOT dispatched here: they + * reject the awaited `connect()` promise, so the caller already has the + * error and a second surface would double-report it. Consumers that don't + * subscribe directly can read the last error via `useInspectorClient`'s + * `lastError`. + */ error: Error; resourceUpdated: { uri: string }; progressNotification: Progress & { progressToken?: ProgressToken }; diff --git a/core/react/useInspectorClient.ts b/core/react/useInspectorClient.ts index de873f7c..c104d41c 100644 --- a/core/react/useInspectorClient.ts +++ b/core/react/useInspectorClient.ts @@ -22,6 +22,14 @@ export interface UseInspectorClientResult { serverInfo?: Implementation; instructions?: string; protocolVersion?: string; + /** + * Message from the most recent mid-session transport failure (the client's + * `error` event — stdio crash, SSE drop, HTTP 5xx). Stays set until the next + * connection attempt (`status` → `"connecting"`) clears it, so consumers can + * render it without subscribing to the event directly. Handshake failures do + * NOT populate this — they reject the `connect()` promise instead. + */ + lastError?: string; appRendererClient: AppRendererClient | null; connect: () => Promise; disconnect: () => Promise; @@ -58,6 +66,7 @@ export function useInspectorClient( const [protocolVersion, setProtocolVersion] = useState( inspectorClient?.getProtocolVersion(), ); + const [lastError, setLastError] = useState(undefined); useEffect(() => { if (!inspectorClient) { @@ -66,6 +75,7 @@ export function useInspectorClient( setServerInfo(undefined); setInstructions(undefined); setProtocolVersion(undefined); + setLastError(undefined); return; } @@ -74,9 +84,18 @@ export function useInspectorClient( setServerInfo(inspectorClient.getServerInfo()); setInstructions(inspectorClient.getInstructions()); setProtocolVersion(inspectorClient.getProtocolVersion()); + setLastError(undefined); const onStatusChange = (event: TypedEvent<"statusChange">) => { setStatus(event.detail); + // A fresh connection attempt clears any stale error from the prior + // session so the UI doesn't keep showing why the last transport died. + if (event.detail === "connecting") { + setLastError(undefined); + } + }; + const onError = (event: TypedEvent<"error">) => { + setLastError(event.detail.message); }; const onCapabilitiesChange = (event: TypedEvent<"capabilitiesChange">) => { setCapabilities(event.detail); @@ -94,6 +113,7 @@ export function useInspectorClient( }; inspectorClient.addEventListener("statusChange", onStatusChange); + inspectorClient.addEventListener("error", onError); inspectorClient.addEventListener( "capabilitiesChange", onCapabilitiesChange, @@ -110,6 +130,7 @@ export function useInspectorClient( return () => { inspectorClient.removeEventListener("statusChange", onStatusChange); + inspectorClient.removeEventListener("error", onError); inspectorClient.removeEventListener( "capabilitiesChange", onCapabilitiesChange, @@ -152,6 +173,7 @@ export function useInspectorClient( serverInfo, instructions, protocolVersion, + lastError, appRendererClient: inspectorClient?.getAppRendererClient() ?? null, connect, disconnect,