From dc31c281024bb3a3207eadf44a660fc4b8ee5e7d Mon Sep 17 00:00:00 2001 From: Cliff Hall Date: Mon, 15 Jun 2026 10:15:29 -0400 Subject: [PATCH] fix(core): make "error" the canonical terminal status on a mid-session crash (#1490) (#1493) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(core): make "error" the canonical terminal status on a mid-session crash (#1490) On a real mid-session transport crash many SDK transports fire BOTH `onclose` and `onerror` in a transport-dependent order. The `onclose` guard (`if status !== "disconnected"`) let a trailing `onclose` downgrade a crash's `"error"` status back to `"disconnected"`, so the persistent terminal status differed by ordering: - onerror-then-onclose → "disconnected" - onclose-then-onerror → "error" `onclose` now treats `"error"` as terminal (only `"connected"`/ `"connecting"` → `"disconnected"`), so `"error"` is the canonical resting status in both orderings. It still emits the `disconnect` event exactly once so session-teardown consumers fire identically either way. Because the state managers gated list/message/subscription/task teardown on `getStatus() === "disconnected"`, holding the status at `"error"` would have left them stale in the onerror-first ordering — swapping one ordering-dependence for another. Introduce `isTerminalStatus()` (disconnected || error) and key every teardown guard off it so they clear identically regardless of crash ordering. Co-Authored-By: Claude Opus 4.8 (1M context) * fix(core): single disconnect event when explicitly disconnecting from "error" (#1490 re-review) Addresses the non-blocking observation from the PR review: explicitly calling disconnect() from a held-"error" status could fire the `disconnect` event twice. close() can trigger the transport's onclose synchronously; onclose (holding status at "error") fired `disconnect`, then disconnect()'s own guard saw status still != "disconnected" and fired it again — a timing-dependence on whether the SDK closes sync. Add a `disconnecting` ownership flag: onclose defers its status set + `disconnect` event to disconnect() while it is set, so the explicit-disconnect path emits exactly one `disconnect` regardless of sync/async close. Crash-path onclose (flag unset) is unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) * test(core): deterministically pin the sync-close disconnect double-fire (#1490 re-review) The real-stdio test asserts the single-disconnect invariant but may not exercise the sync-close branch (the one that double-fired) if the SDK stdio transport closes asynchronously. Add a test that overrides the SDK client's close() to invoke the transport's onclose synchronously, forcing the exact timing the `disconnecting` flag guards. Verified it asserts 2 (fails) without the flag and 1 (passes) with it. Co-Authored-By: Claude Opus 4.8 (1M context) --------- Co-authored-by: Claude Opus 4.8 (1M context) --- .../mcp/state/managedPromptsState.test.ts | 15 +- .../state/managedRequestorTasksState.test.ts | 15 +- .../managedResourceTemplatesState.test.ts | 15 +- .../mcp/state/managedResourcesState.test.ts | 15 +- .../core/mcp/state/managedToolsState.test.ts | 15 +- .../core/mcp/state/messageLogState.test.ts | 13 +- .../core/mcp/state/pagedPromptsState.test.ts | 12 +- .../state/pagedRequestorTasksState.test.ts | 10 + .../state/pagedResourceTemplatesState.test.ts | 12 +- .../mcp/state/pagedResourcesState.test.ts | 12 +- .../core/mcp/state/pagedToolsState.test.ts | 12 +- .../state/resourceSubscriptionsState.test.ts | 14 +- .../integration/mcp/inspectorClient.test.ts | 193 ++++++++++++++++++ core/mcp/inspectorClient.ts | 101 ++++++--- core/mcp/state/managedListState.ts | 3 +- core/mcp/state/managedRequestorTasksState.ts | 3 +- core/mcp/state/messageLogState.ts | 3 +- core/mcp/state/pagedPromptsState.ts | 3 +- core/mcp/state/pagedRequestorTasksState.ts | 3 +- core/mcp/state/pagedResourceTemplatesState.ts | 3 +- core/mcp/state/pagedResourcesState.ts | 3 +- core/mcp/state/pagedToolsState.ts | 3 +- core/mcp/state/resourceSubscriptionsState.ts | 3 +- core/mcp/types.ts | 17 ++ 24 files changed, 447 insertions(+), 51 deletions(-) diff --git a/clients/web/src/test/core/mcp/state/managedPromptsState.test.ts b/clients/web/src/test/core/mcp/state/managedPromptsState.test.ts index 9b660f19..2cc964c2 100644 --- a/clients/web/src/test/core/mcp/state/managedPromptsState.test.ts +++ b/clients/web/src/test/core/mcp/state/managedPromptsState.test.ts @@ -182,11 +182,24 @@ describe("ManagedPromptsState", () => { expect(state.getPrompts()).toEqual([]); }); - it("statusChange to other (non-disconnected) values does not clear prompts", async () => { + it("statusChange to error clears prompts (error is terminal, #1490)", async () => { client.setStatus("connected"); client.queuePromptPages({ prompts: [prompt("a")] }); await state.refresh(); + expect(state.getPrompts()).toHaveLength(1); + + const changePromise = waitForPromptsChange(state); client.setStatus("error"); + const next = await changePromise; + expect(next).toEqual([]); + expect(state.getPrompts()).toEqual([]); + }); + + it("statusChange to a non-terminal value (connecting) does not clear prompts", async () => { + client.setStatus("connected"); + client.queuePromptPages({ prompts: [prompt("a")] }); + await state.refresh(); + client.setStatus("connecting"); expect(state.getPrompts().map((p) => p.name)).toEqual(["a"]); }); diff --git a/clients/web/src/test/core/mcp/state/managedRequestorTasksState.test.ts b/clients/web/src/test/core/mcp/state/managedRequestorTasksState.test.ts index bcc5e07d..32212b43 100644 --- a/clients/web/src/test/core/mcp/state/managedRequestorTasksState.test.ts +++ b/clients/web/src/test/core/mcp/state/managedRequestorTasksState.test.ts @@ -136,11 +136,24 @@ describe("ManagedRequestorTasksState", () => { expect(state.getTasks()).toEqual([]); }); - it("statusChange to other values does not clear tasks", async () => { + it("statusChange to error clears tasks (error is terminal, #1490)", async () => { client.setStatus("connected"); client.queueTaskPages({ tasks: [task("t1")] }); await state.refresh(); + expect(state.getTasks()).toHaveLength(1); + + const changePromise = waitForChange(state); client.setStatus("error"); + const next = await changePromise; + expect(next).toEqual([]); + expect(state.getTasks()).toEqual([]); + }); + + it("statusChange to a non-terminal value (connecting) does not clear tasks", async () => { + client.setStatus("connected"); + client.queueTaskPages({ tasks: [task("t1")] }); + await state.refresh(); + client.setStatus("connecting"); expect(state.getTasks().map((t) => t.taskId)).toEqual(["t1"]); }); diff --git a/clients/web/src/test/core/mcp/state/managedResourceTemplatesState.test.ts b/clients/web/src/test/core/mcp/state/managedResourceTemplatesState.test.ts index ab13a646..502e5eed 100644 --- a/clients/web/src/test/core/mcp/state/managedResourceTemplatesState.test.ts +++ b/clients/web/src/test/core/mcp/state/managedResourceTemplatesState.test.ts @@ -196,11 +196,24 @@ describe("ManagedResourceTemplatesState", () => { expect(state.getResourceTemplates()).toEqual([]); }); - it("statusChange to other values does not clear templates", async () => { + it("statusChange to error clears templates (error is terminal, #1490)", async () => { client.setStatus("connected"); client.queueResourceTemplatePages({ resourceTemplates: [template("a")] }); await state.refresh(); + expect(state.getResourceTemplates()).toHaveLength(1); + + const changePromise = waitForChange(state); client.setStatus("error"); + const next = await changePromise; + expect(next).toEqual([]); + expect(state.getResourceTemplates()).toEqual([]); + }); + + it("statusChange to a non-terminal value (connecting) does not clear templates", async () => { + client.setStatus("connected"); + client.queueResourceTemplatePages({ resourceTemplates: [template("a")] }); + await state.refresh(); + client.setStatus("connecting"); expect(state.getResourceTemplates().map((t) => t.name)).toEqual(["a"]); }); diff --git a/clients/web/src/test/core/mcp/state/managedResourcesState.test.ts b/clients/web/src/test/core/mcp/state/managedResourcesState.test.ts index 729fb6eb..ea510fb2 100644 --- a/clients/web/src/test/core/mcp/state/managedResourcesState.test.ts +++ b/clients/web/src/test/core/mcp/state/managedResourcesState.test.ts @@ -189,11 +189,24 @@ describe("ManagedResourcesState", () => { expect(state.getResources()).toEqual([]); }); - it("statusChange to other (non-disconnected) values does not clear resources", async () => { + it("statusChange to error clears resources (error is terminal, #1490)", async () => { client.setStatus("connected"); client.queueResourcePages({ resources: [resource("a://1")] }); await state.refresh(); + expect(state.getResources()).toHaveLength(1); + + const changePromise = waitForResourcesChange(state); client.setStatus("error"); + const next = await changePromise; + expect(next).toEqual([]); + expect(state.getResources()).toEqual([]); + }); + + it("statusChange to a non-terminal value (connecting) does not clear resources", async () => { + client.setStatus("connected"); + client.queueResourcePages({ resources: [resource("a://1")] }); + await state.refresh(); + client.setStatus("connecting"); expect(state.getResources().map((r) => r.uri)).toEqual(["a://1"]); }); diff --git a/clients/web/src/test/core/mcp/state/managedToolsState.test.ts b/clients/web/src/test/core/mcp/state/managedToolsState.test.ts index b5cca48f..fd2e143c 100644 --- a/clients/web/src/test/core/mcp/state/managedToolsState.test.ts +++ b/clients/web/src/test/core/mcp/state/managedToolsState.test.ts @@ -242,11 +242,24 @@ describe("ManagedToolsState", () => { expect(state.getTools()).toEqual([]); }); - it("statusChange to other (non-disconnected) values does not clear tools", async () => { + it("statusChange to error clears tools (error is terminal, #1490)", async () => { client.setStatus("connected"); client.queueToolPages({ tools: [tool("a")] }); await state.refresh(); + expect(state.getTools()).toHaveLength(1); + + const changePromise = waitForToolsChange(state); client.setStatus("error"); + const next = await changePromise; + expect(next).toEqual([]); + expect(state.getTools()).toEqual([]); + }); + + it("statusChange to a non-terminal value (connecting) does not clear tools", async () => { + client.setStatus("connected"); + client.queueToolPages({ tools: [tool("a")] }); + await state.refresh(); + client.setStatus("connecting"); expect(state.getTools().map((t) => t.name)).toEqual(["a"]); }); diff --git a/clients/web/src/test/core/mcp/state/messageLogState.test.ts b/clients/web/src/test/core/mcp/state/messageLogState.test.ts index 3583cf6d..8ef02a7e 100644 --- a/clients/web/src/test/core/mcp/state/messageLogState.test.ts +++ b/clients/web/src/test/core/mcp/state/messageLogState.test.ts @@ -177,9 +177,20 @@ describe("MessageLogState", () => { expect(dispatched).toBe(true); }); - it("does not clear on non-disconnected status changes", () => { + it("clears on a mid-session crash (statusChange -> error, #1490)", () => { + client.setStatus("connected"); client.dispatchTypedEvent("message", notificationEntry()); + expect(state.getMessages()).toHaveLength(1); + let dispatched = false; + state.addEventListener("messagesChange", () => (dispatched = true)); client.setStatus("error"); + expect(state.getMessages()).toEqual([]); + expect(dispatched).toBe(true); + }); + + it("does not clear on a non-terminal status change (connecting)", () => { + client.dispatchTypedEvent("message", notificationEntry()); + client.setStatus("connecting"); expect(state.getMessages()).toHaveLength(1); }); diff --git a/clients/web/src/test/core/mcp/state/pagedPromptsState.test.ts b/clients/web/src/test/core/mcp/state/pagedPromptsState.test.ts index bf247e62..eaf32036 100644 --- a/clients/web/src/test/core/mcp/state/pagedPromptsState.test.ts +++ b/clients/web/src/test/core/mcp/state/pagedPromptsState.test.ts @@ -81,11 +81,21 @@ describe("PagedPromptsState", () => { expect(await changePromise).toEqual([]); }); - it("statusChange to non-disconnected values is a no-op", async () => { + it("statusChange to error clears prompts (error is terminal, #1490)", async () => { client.setStatus("connected"); client.queuePromptPages({ prompts: [prompt("a")] }); await state.loadPage(); + const changePromise = waitForChange(state); client.setStatus("error"); + expect(await changePromise).toEqual([]); + expect(state.getPrompts()).toEqual([]); + }); + + it("statusChange to a non-terminal value (connecting) is a no-op", async () => { + client.setStatus("connected"); + client.queuePromptPages({ prompts: [prompt("a")] }); + await state.loadPage(); + client.setStatus("connecting"); expect(state.getPrompts().map((p) => p.name)).toEqual(["a"]); }); diff --git a/clients/web/src/test/core/mcp/state/pagedRequestorTasksState.test.ts b/clients/web/src/test/core/mcp/state/pagedRequestorTasksState.test.ts index 095a3567..716bf993 100644 --- a/clients/web/src/test/core/mcp/state/pagedRequestorTasksState.test.ts +++ b/clients/web/src/test/core/mcp/state/pagedRequestorTasksState.test.ts @@ -148,6 +148,16 @@ describe("PagedRequestorTasksState", () => { expect(state.getNextCursor()).toBeUndefined(); }); + it("statusChange to error clears tasks and nextCursor (error is terminal, #1490)", async () => { + client.setStatus("connected"); + client.queueTaskPages({ tasks: [task("t1")], nextCursor: "c1" }); + await state.loadPage(); + const changePromise = waitForChange(state); + client.setStatus("error"); + expect(await changePromise).toEqual([]); + expect(state.getNextCursor()).toBeUndefined(); + }); + it("destroy stops listening and clears state", async () => { client.setStatus("connected"); client.queueTaskPages({ tasks: [task("t1")] }); diff --git a/clients/web/src/test/core/mcp/state/pagedResourceTemplatesState.test.ts b/clients/web/src/test/core/mcp/state/pagedResourceTemplatesState.test.ts index cd6a9332..30edf9e6 100644 --- a/clients/web/src/test/core/mcp/state/pagedResourceTemplatesState.test.ts +++ b/clients/web/src/test/core/mcp/state/pagedResourceTemplatesState.test.ts @@ -89,11 +89,21 @@ describe("PagedResourceTemplatesState", () => { expect(await changePromise).toEqual([]); }); - it("statusChange to non-disconnected values is a no-op", async () => { + it("statusChange to error clears templates (error is terminal, #1490)", async () => { client.setStatus("connected"); client.queueResourceTemplatePages({ resourceTemplates: [template("a")] }); await state.loadPage(); + const changePromise = waitForChange(state); client.setStatus("error"); + expect(await changePromise).toEqual([]); + expect(state.getResourceTemplates()).toEqual([]); + }); + + it("statusChange to a non-terminal value (connecting) is a no-op", async () => { + client.setStatus("connected"); + client.queueResourceTemplatePages({ resourceTemplates: [template("a")] }); + await state.loadPage(); + client.setStatus("connecting"); expect(state.getResourceTemplates().map((t) => t.name)).toEqual(["a"]); }); diff --git a/clients/web/src/test/core/mcp/state/pagedResourcesState.test.ts b/clients/web/src/test/core/mcp/state/pagedResourcesState.test.ts index adf811b1..03d6661e 100644 --- a/clients/web/src/test/core/mcp/state/pagedResourcesState.test.ts +++ b/clients/web/src/test/core/mcp/state/pagedResourcesState.test.ts @@ -83,11 +83,21 @@ describe("PagedResourcesState", () => { expect(await changePromise).toEqual([]); }); - it("statusChange to non-disconnected values is a no-op", async () => { + it("statusChange to error clears resources (error is terminal, #1490)", async () => { client.setStatus("connected"); client.queueResourcePages({ resources: [resource("a://1")] }); await state.loadPage(); + const changePromise = waitForChange(state); client.setStatus("error"); + expect(await changePromise).toEqual([]); + expect(state.getResources()).toEqual([]); + }); + + it("statusChange to a non-terminal value (connecting) is a no-op", async () => { + client.setStatus("connected"); + client.queueResourcePages({ resources: [resource("a://1")] }); + await state.loadPage(); + client.setStatus("connecting"); expect(state.getResources().map((r) => r.uri)).toEqual(["a://1"]); }); diff --git a/clients/web/src/test/core/mcp/state/pagedToolsState.test.ts b/clients/web/src/test/core/mcp/state/pagedToolsState.test.ts index 1f7fbb1e..4f4b51e0 100644 --- a/clients/web/src/test/core/mcp/state/pagedToolsState.test.ts +++ b/clients/web/src/test/core/mcp/state/pagedToolsState.test.ts @@ -77,11 +77,21 @@ describe("PagedToolsState", () => { expect(state.getTools()).toEqual([]); }); - it("statusChange to non-disconnected values is a no-op", async () => { + it("statusChange to error clears tools (error is terminal, #1490)", async () => { client.setStatus("connected"); client.queueToolPages({ tools: [tool("a")] }); await state.loadPage(); + const changePromise = waitForChange(state); client.setStatus("error"); + expect(await changePromise).toEqual([]); + expect(state.getTools()).toEqual([]); + }); + + it("statusChange to a non-terminal value (connecting) is a no-op", async () => { + client.setStatus("connected"); + client.queueToolPages({ tools: [tool("a")] }); + await state.loadPage(); + client.setStatus("connecting"); expect(state.getTools().map((t) => t.name)).toEqual(["a"]); }); diff --git a/clients/web/src/test/core/mcp/state/resourceSubscriptionsState.test.ts b/clients/web/src/test/core/mcp/state/resourceSubscriptionsState.test.ts index 44d2ddee..5dbd5659 100644 --- a/clients/web/src/test/core/mcp/state/resourceSubscriptionsState.test.ts +++ b/clients/web/src/test/core/mcp/state/resourceSubscriptionsState.test.ts @@ -191,10 +191,22 @@ describe("ResourceSubscriptionsState", () => { expect(state.getSubscriptions()).toEqual([]); }); - it("does not clear subscriptions on non-disconnected status changes", () => { + it("clears subscriptions on a mid-session crash (statusChange to error, #1490)", async () => { const state = new ResourceSubscriptionsState(client); client.dispatchTypedEvent("resourceSubscriptionsChange", ["file:///a"]); + expect(state.getSubscriptions()).toHaveLength(1); + + const changePromise = waitForSubscriptionsChange(state); client.setStatus("error"); + const next = await changePromise; + expect(next).toEqual([]); + expect(state.getSubscriptions()).toEqual([]); + }); + + it("does not clear subscriptions on a non-terminal status change (connecting)", () => { + const state = new ResourceSubscriptionsState(client); + client.dispatchTypedEvent("resourceSubscriptionsChange", ["file:///a"]); + client.setStatus("connecting"); expect(state.getSubscriptions()).toHaveLength(1); }); diff --git a/clients/web/src/test/integration/mcp/inspectorClient.test.ts b/clients/web/src/test/integration/mcp/inspectorClient.test.ts index 30e645d7..b369e556 100644 --- a/clients/web/src/test/integration/mcp/inspectorClient.test.ts +++ b/clients/web/src/test/integration/mcp/inspectorClient.test.ts @@ -2612,6 +2612,199 @@ describe("InspectorClient", () => { await client.disconnect(); }); + it("holds status at error when a trailing onclose lands after onerror (#1490)", async () => { + // The reverse ordering of the test above: onerror lands first (status + // "error"), then the trailing onclose arrives. A bare + // `if (status !== "disconnected")` onclose guard would downgrade "error" + // back to "disconnected", so the persistent terminal status differed by + // transport ordering. onclose must now treat "error" as terminal and + // leave it untouched, while still emitting `disconnect` so teardown + // consumers fire identically in both orderings. + client = new InspectorClient( + { + type: "stdio", + command: serverCommand.command, + args: serverCommand.args, + }, + { + environment: { transport: createTransportNode }, + }, + ); + + const errors: Error[] = []; + let disconnects = 0; + client.addEventListener("error", (event) => { + errors.push(event.detail); + }); + client.addEventListener("disconnect", () => { + disconnects++; + }); + + await client.connect(); + expect(client.getStatus()).toBe("connected"); + + const baseTransport = ( + client as unknown as { + baseTransport: { + onclose?: () => void; + onerror?: (error: Error) => void; + }; + } + ).baseTransport; + + // onerror first → status "error". + const failure = new Error("stdio subprocess crashed"); + baseTransport.onerror?.(failure); + expect(client.getStatus()).toBe("error"); + + // Trailing onclose must NOT downgrade "error" to "disconnected"... + baseTransport.onclose?.(); + expect(client.getStatus()).toBe("error"); + // ...but must still fire `disconnect` exactly once so session teardown + // (e.g. App resets the active server) happens regardless of ordering. + expect(disconnects).toBe(1); + expect(errors).toHaveLength(1); + expect(errors[0]).toBe(failure); + + await client.disconnect(); + }); + + it("emits disconnect on a mid-session crash regardless of onclose/onerror ordering (#1490)", async () => { + // Both real-world orderings must settle on the same observable outcome: + // final status "error" and exactly one `disconnect` event. + const run = async (order: "onclose-first" | "onerror-first") => { + const c = new InspectorClient( + { + type: "stdio", + command: serverCommand.command, + args: serverCommand.args, + }, + { environment: { transport: createTransportNode } }, + ); + let disconnects = 0; + c.addEventListener("disconnect", () => { + disconnects++; + }); + await c.connect(); + const bt = ( + c as unknown as { + baseTransport: { + onclose?: () => void; + onerror?: (error: Error) => void; + }; + } + ).baseTransport; + const failure = new Error("crash"); + if (order === "onclose-first") { + bt.onclose?.(); + bt.onerror?.(failure); + } else { + bt.onerror?.(failure); + bt.onclose?.(); + } + // Snapshot before the cleanup disconnect() below, which fires its own + // disconnect events as it tears the (still-live) real transport down. + const result = { status: c.getStatus(), disconnects }; + await c.disconnect(); + return result; + }; + + const closeFirst = await run("onclose-first"); + const errorFirst = await run("onerror-first"); + + expect(closeFirst).toEqual({ status: "error", disconnects: 1 }); + expect(errorFirst).toEqual({ status: "error", disconnects: 1 }); + }); + + it("fires disconnect exactly once when explicitly disconnecting from a crashed (error) state (#1490)", async () => { + // After a crash holds status at "error", close() can fire the transport's + // onclose synchronously. onclose would emit `disconnect` (status held at + // "error"), and then disconnect()'s own guard — seeing "error" still + // != "disconnected" — would emit it a second time. The `disconnecting` + // ownership flag must collapse that to exactly one event regardless of + // whether the SDK closes synchronously. + client = new InspectorClient( + { + type: "stdio", + command: serverCommand.command, + args: serverCommand.args, + }, + { environment: { transport: createTransportNode } }, + ); + await client.connect(); + + const baseTransport = ( + client as unknown as { + baseTransport: { onerror?: (error: Error) => void }; + } + ).baseTransport; + baseTransport.onerror?.(new Error("stdio subprocess crashed")); + expect(client.getStatus()).toBe("error"); + + // Listener attached AFTER the crash so it counts only the explicit + // disconnect()'s events. + let disconnects = 0; + client.addEventListener("disconnect", () => { + disconnects++; + }); + await client.disconnect(); + + expect(disconnects).toBe(1); + expect(client.getStatus()).toBe("disconnected"); + }); + + it("fires disconnect exactly once on a synchronous close() from error (deterministic sync-close pin, #1490)", async () => { + // The test above uses the real stdio transport, whose close() may invoke + // onclose asynchronously — so it asserts the invariant but does not + // deterministically exercise the sync-close branch that double-fired + // (onclose firing INSIDE close(), before disconnect()'s guard runs). + // Here we force that exact timing: override the SDK client's close() to + // invoke the transport's onclose synchronously. Without the + // `disconnecting` flag this asserts 2; with it, 1. + client = new InspectorClient( + { + type: "stdio", + command: serverCommand.command, + args: serverCommand.args, + }, + { environment: { transport: createTransportNode } }, + ); + await client.connect(); + + const baseTransport = ( + client as unknown as { + baseTransport: { + onclose?: () => void; + onerror?: (error: Error) => void; + }; + } + ).baseTransport; + // Put the client into the crashed "error" state. + baseTransport.onerror?.(new Error("stdio subprocess crashed")); + expect(client.getStatus()).toBe("error"); + + // Force close() to fire onclose synchronously (the sync-close branch), + // then delegate to the real close() so the subprocess is still torn down. + const sdkClient = ( + client as unknown as { client: { close: () => Promise } } + ).client; + const realClose = sdkClient.close.bind(sdkClient); + sdkClient.close = async () => { + baseTransport.onclose?.(); + await realClose(); + }; + + // Listener attached AFTER the crash so it counts only disconnect()'s events. + let disconnects = 0; + client.addEventListener("disconnect", () => { + disconnects++; + }); + await client.disconnect(); + + expect(disconnects).toBe(1); + expect(client.getStatus()).toBe("disconnected"); + }); + 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 diff --git a/core/mcp/inspectorClient.ts b/core/mcp/inspectorClient.ts index 0f1ab794..f3c74d31 100644 --- a/core/mcp/inspectorClient.ts +++ b/core/mcp/inspectorClient.ts @@ -189,6 +189,11 @@ export class InspectorClient extends InspectorClientEventTarget { private defaultMetadata: Record | undefined; private serverSettings: InspectorServerSettings | undefined; private status: ConnectionStatus = "disconnected"; + // True only while an explicit disconnect() owns the teardown. close() can + // trigger the transport's onclose synchronously, so this lets onclose defer + // the canonical status set + `disconnect` event to disconnect() and fire it + // exactly once (see onclose / disconnect()). + private disconnecting = false; // Server data (resources, resourceTemplates, prompts are in state managers) private capabilities?: ServerCapabilities; private serverInfo?: Implementation; @@ -429,11 +434,30 @@ export class InspectorClient extends InspectorClientEventTarget { private attachTransportListeners(baseTransport: Transport): void { baseTransport.onclose = () => { - if (this.status !== "disconnected") { + // An explicit disconnect() owns the teardown and will set the canonical + // status + fire `disconnect` itself. Defer to it so the event fires + // exactly once whether or not the SDK calls onclose synchronously inside + // close() — without this, an onclose that runs while status is held at + // "error" would fire `disconnect`, then disconnect()'s own guard would + // fire it again (#1490 re-review). + if (this.disconnecting) return; + // Already fully torn down — nothing to do (avoids a duplicate + // `disconnect` event after an explicit disconnect()). + if (this.status === "disconnected") return; + // Do NOT let a trailing `onclose` downgrade a crash's "error" status to + // "disconnected". On a real mid-session crash many SDK transports fire + // BOTH `onclose` and `onerror` in a transport-dependent order; with the + // old `!== "disconnected"` guard the final status differed by ordering + // ("disconnected" when onerror landed first, "error" when onclose did). + // Treating "error" as terminal here makes "error" the canonical resting + // status in both orderings (#1490). We still emit the `disconnect` event + // below so session-teardown consumers fire identically either way; only + // the persistent status value is held at "error". + if (this.status !== "error") { this.status = "disconnected"; this.dispatchTypedEvent("statusChange", this.status); - this.dispatchTypedEvent("disconnect"); } + this.dispatchTypedEvent("disconnect"); }; baseTransport.onerror = (error: Error) => { // Suppress ONLY the handshake case. These listeners are attached before @@ -1051,41 +1075,56 @@ export class InspectorClient extends InspectorClientEventTarget { * @param safeDisconnectTimeout If > 0, poll every 10ms until SDK _responseHandlers is empty or this many ms have elapsed, then close. Default 0 = close immediately. */ async disconnect(safeDisconnectTimeout = 0): Promise { - if (this.client) { - if (safeDisconnectTimeout > 0) { - // This is pretty creepy, but there are test cases where client calls return but there - // are still response handlers pending. Usually a single macrotask delay is enough to - // clear them, but not always (it's been >10ms in some cases). The pending handlers - // themselves get the error (and in cases where those aren't awaited, the errors fly - // out of the test). This workaround where we directly access the handlers (otherwise - // private member of the SDK client) is creepy, but the least ugly working solution. - // We will re-valuate this with the v2 SDK. Currenly only tests that do quick disconnects - // use this setting. - // - const protocol = this.client as unknown as { - _responseHandlers?: Map; - }; - const handlers = protocol._responseHandlers; - const deadline = Date.now() + safeDisconnectTimeout; - while ( - handlers?.size !== undefined && - handlers.size > 0 && - Date.now() < deadline - ) { - await new Promise((r) => setTimeout(r, 10)); + // Claim ownership of the teardown so a synchronous onclose (fired from + // within close() below) defers its status set + `disconnect` event to the + // canonical block at the end of this method. Reset before that block so + // its dispatch is unaffected; any later async onclose early-returns on the + // "disconnected" status guard. Guarantees a single `disconnect` event even + // when disconnecting from a held-"error" status (#1490 re-review). + this.disconnecting = true; + try { + if (this.client) { + if (safeDisconnectTimeout > 0) { + // This is pretty creepy, but there are test cases where client calls return but there + // are still response handlers pending. Usually a single macrotask delay is enough to + // clear them, but not always (it's been >10ms in some cases). The pending handlers + // themselves get the error (and in cases where those aren't awaited, the errors fly + // out of the test). This workaround where we directly access the handlers (otherwise + // private member of the SDK client) is creepy, but the least ugly working solution. + // We will re-valuate this with the v2 SDK. Currenly only tests that do quick disconnects + // use this setting. + // + const protocol = this.client as unknown as { + _responseHandlers?: Map; + }; + const handlers = protocol._responseHandlers; + const deadline = Date.now() + safeDisconnectTimeout; + while ( + handlers?.size !== undefined && + handlers.size > 0 && + Date.now() < deadline + ) { + await new Promise((r) => setTimeout(r, 10)); + } + } + try { + await this.client.close(); + } catch { + // Ignore errors on close } } - try { - await this.client.close(); - } catch { - // Ignore errors on close - } + } finally { + // Release ownership before the canonical dispatch below so it runs + // normally; the "disconnected" status it sets makes any later async + // onclose early-return. + this.disconnecting = false; } // Null out transport so next connect() creates a fresh one. this.baseTransport = null; this.transport = null; - // Update status - transport onclose handler will also fire and clear state - // But we also do it here in case disconnect() is called directly + // Update status - any onclose fired during close() above deferred to us + // (see `disconnecting`), so this is the single place the explicit-disconnect + // path settles the status and emits `disconnect`. if (this.status !== "disconnected") { this.status = "disconnected"; this.dispatchTypedEvent("statusChange", this.status); diff --git a/core/mcp/state/managedListState.ts b/core/mcp/state/managedListState.ts index 4faa4ed9..99471d18 100644 --- a/core/mcp/state/managedListState.ts +++ b/core/mcp/state/managedListState.ts @@ -14,6 +14,7 @@ import type { InspectorClientProtocol } from "../inspectorClientProtocol.js"; import type { InspectorClientEventMap } from "../inspectorClientEventTarget.js"; import type { ServerCapabilities } from "@modelcontextprotocol/sdk/types.js"; +import { isTerminalStatus } from "../types.js"; import { TypedEventTarget } from "../typedEventTarget.js"; const MAX_PAGES = 100; @@ -106,7 +107,7 @@ export abstract class ManagedListState< }, config.debounceMs); }; const onStatusChange = (): void => { - if (this.client?.getStatus() === "disconnected") { + if (isTerminalStatus(this.client?.getStatus())) { if (this.listChangedTimer !== null) { clearTimeout(this.listChangedTimer); this.listChangedTimer = null; diff --git a/core/mcp/state/managedRequestorTasksState.ts b/core/mcp/state/managedRequestorTasksState.ts index 190aa27d..42e71f00 100644 --- a/core/mcp/state/managedRequestorTasksState.ts +++ b/core/mcp/state/managedRequestorTasksState.ts @@ -9,6 +9,7 @@ import type { InspectorClientProtocol } from "../inspectorClientProtocol.js"; import type { Task } from "@modelcontextprotocol/sdk/types.js"; +import { isTerminalStatus } from "../types.js"; import type { InspectorClientEventMap } from "../inspectorClientEventTarget.js"; import { TypedEventTarget, @@ -55,7 +56,7 @@ export class ManagedRequestorTasksState extends TypedEventTarget { - if (this.client?.getStatus() === "disconnected") { + if (isTerminalStatus(this.client?.getStatus())) { this.tasks = []; this.dismissedTaskIds.clear(); this.dispatchTypedEvent("tasksChange", []); diff --git a/core/mcp/state/messageLogState.ts b/core/mcp/state/messageLogState.ts index 28a5d366..2cc6de7b 100644 --- a/core/mcp/state/messageLogState.ts +++ b/core/mcp/state/messageLogState.ts @@ -16,6 +16,7 @@ import type { InspectorClientProtocol } from "../inspectorClientProtocol.js"; import type { MessageEntry } from "../types.js"; +import { isTerminalStatus } from "../types.js"; import type { InspectorClientEventMap } from "../inspectorClientEventTarget.js"; import { TypedEventTarget, @@ -98,7 +99,7 @@ export class MessageLogState extends TypedEventTarget { }; const onStatusChange = (): void => { - if (this.client?.getStatus() === "disconnected") { + if (isTerminalStatus(this.client?.getStatus())) { this.messages = []; this.pendingRequestEntries.clear(); this.dispatchTypedEvent("messagesChange", []); diff --git a/core/mcp/state/pagedPromptsState.ts b/core/mcp/state/pagedPromptsState.ts index f0e046a5..0d49aed3 100644 --- a/core/mcp/state/pagedPromptsState.ts +++ b/core/mcp/state/pagedPromptsState.ts @@ -12,6 +12,7 @@ import type { InspectorClientProtocol } from "../inspectorClientProtocol.js"; import type { Prompt } from "@modelcontextprotocol/sdk/types.js"; +import { isTerminalStatus } from "../types.js"; import { TypedEventTarget } from "../typedEventTarget.js"; export interface PagedPromptsStateEventMap { @@ -32,7 +33,7 @@ export class PagedPromptsState extends TypedEventTarget { - if (this.client?.getStatus() === "disconnected") { + if (isTerminalStatus(this.client?.getStatus())) { this.prompts = []; this.dispatchTypedEvent("promptsChange", []); } diff --git a/core/mcp/state/pagedRequestorTasksState.ts b/core/mcp/state/pagedRequestorTasksState.ts index f68604ff..9092339a 100644 --- a/core/mcp/state/pagedRequestorTasksState.ts +++ b/core/mcp/state/pagedRequestorTasksState.ts @@ -9,6 +9,7 @@ import type { InspectorClientProtocol } from "../inspectorClientProtocol.js"; import type { Task } from "@modelcontextprotocol/sdk/types.js"; +import { isTerminalStatus } from "../types.js"; import type { InspectorClientEventMap } from "../inspectorClientEventTarget.js"; import { TypedEventTarget, @@ -35,7 +36,7 @@ export class PagedRequestorTasksState extends TypedEventTarget { - if (this.client?.getStatus() === "disconnected") { + if (isTerminalStatus(this.client?.getStatus())) { this.tasks = []; this.nextCursor = undefined; this.dispatchTypedEvent("tasksChange", []); diff --git a/core/mcp/state/pagedResourceTemplatesState.ts b/core/mcp/state/pagedResourceTemplatesState.ts index 35c4d633..0bdff4ae 100644 --- a/core/mcp/state/pagedResourceTemplatesState.ts +++ b/core/mcp/state/pagedResourceTemplatesState.ts @@ -13,6 +13,7 @@ import type { InspectorClientProtocol } from "../inspectorClientProtocol.js"; import type { ResourceTemplate } from "@modelcontextprotocol/sdk/types.js"; +import { isTerminalStatus } from "../types.js"; import { TypedEventTarget } from "../typedEventTarget.js"; export interface PagedResourceTemplatesStateEventMap { @@ -33,7 +34,7 @@ export class PagedResourceTemplatesState extends TypedEventTarget { - if (this.client?.getStatus() === "disconnected") { + if (isTerminalStatus(this.client?.getStatus())) { this.resourceTemplates = []; this.dispatchTypedEvent("resourceTemplatesChange", []); } diff --git a/core/mcp/state/pagedResourcesState.ts b/core/mcp/state/pagedResourcesState.ts index 2585173b..c90c08cc 100644 --- a/core/mcp/state/pagedResourcesState.ts +++ b/core/mcp/state/pagedResourcesState.ts @@ -12,6 +12,7 @@ import type { InspectorClientProtocol } from "../inspectorClientProtocol.js"; import type { Resource } from "@modelcontextprotocol/sdk/types.js"; +import { isTerminalStatus } from "../types.js"; import { TypedEventTarget } from "../typedEventTarget.js"; export interface PagedResourcesStateEventMap { @@ -32,7 +33,7 @@ export class PagedResourcesState extends TypedEventTarget { - if (this.client?.getStatus() === "disconnected") { + if (isTerminalStatus(this.client?.getStatus())) { this.resources = []; this.dispatchTypedEvent("resourcesChange", []); } diff --git a/core/mcp/state/pagedToolsState.ts b/core/mcp/state/pagedToolsState.ts index 7ce7a126..2bc93e2f 100644 --- a/core/mcp/state/pagedToolsState.ts +++ b/core/mcp/state/pagedToolsState.ts @@ -13,6 +13,7 @@ import type { InspectorClientProtocol } from "../inspectorClientProtocol.js"; import type { Tool } from "@modelcontextprotocol/sdk/types.js"; +import { isTerminalStatus } from "../types.js"; import { TypedEventTarget } from "../typedEventTarget.js"; export interface PagedToolsStateEventMap { @@ -33,7 +34,7 @@ export class PagedToolsState extends TypedEventTarget { super(); this.client = client; const onStatusChange = (): void => { - if (this.client?.getStatus() === "disconnected") { + if (isTerminalStatus(this.client?.getStatus())) { this.tools = []; this.dispatchTypedEvent("toolsChange", []); } diff --git a/core/mcp/state/resourceSubscriptionsState.ts b/core/mcp/state/resourceSubscriptionsState.ts index 419ed86f..7135e326 100644 --- a/core/mcp/state/resourceSubscriptionsState.ts +++ b/core/mcp/state/resourceSubscriptionsState.ts @@ -22,6 +22,7 @@ import type { ManagedResourcesStateEventMap, } from "./managedResourcesState.js"; import type { InspectorResourceSubscription } from "../types.js"; +import { isTerminalStatus } from "../types.js"; import type { Resource } from "@modelcontextprotocol/sdk/types.js"; import { TypedEventTarget, @@ -82,7 +83,7 @@ export class ResourceSubscriptionsState extends TypedEventTarget { - if (this.client?.getStatus() === "disconnected") { + if (isTerminalStatus(this.client?.getStatus())) { this.subscribedUris = []; this.lastUpdatedByUri.clear(); this.subscriptions = []; diff --git a/core/mcp/types.ts b/core/mcp/types.ts index 93b5d6c8..7741812e 100644 --- a/core/mcp/types.ts +++ b/core/mcp/types.ts @@ -149,6 +149,23 @@ export type ConnectionStatus = | "connected" | "error"; +/** + * True when a connection has settled into a non-live terminal state — either a + * clean `"disconnected"` or a crashed `"error"`. Both mean the session is over + * and any cached server state (tool/resource/prompt lists, message log, + * subscriptions) should be torn down. + * + * Session-teardown consumers must key off this predicate rather than branching + * on `status === "disconnected"` alone: on a real mid-session crash many SDK + * transports fire BOTH `onclose` and `onerror` in a transport-dependent order, + * and the canonical terminal status is now `"error"` regardless of ordering + * (see InspectorClient's `onclose` handler, #1490). A bare `=== "disconnected"` + * check would therefore tear down in one ordering but not the other. + */ +export function isTerminalStatus(status: ConnectionStatus | undefined): boolean { + return status === "disconnected" || status === "error"; +} + /** * Snapshot of a server's connection state, used by dumb components * that display status, retry count, and error details.