fix(core): make "error" the canonical terminal status on a mid-session crash (#1490) (#1493)

* 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) <noreply@anthropic.com>

* 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) <noreply@anthropic.com>

* 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) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

core/mcp/inspectorClient.ts

@@ -189,6 +189,11 @@ export class InspectorClient extends InspectorClientEventTarget {
   private defaultMetadata: Record<string, string> | 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<void> {
-    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<unknown, unknown>;
-        };
-        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<unknown, unknown>;
+          };
+          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);

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;

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<ManagedRequesto
       void this.refresh();
     };
     const onStatusChange = (): void => {
-      if (this.client?.getStatus() === "disconnected") {
+      if (isTerminalStatus(this.client?.getStatus())) {
         this.tasks = [];
         this.dismissedTaskIds.clear();
         this.dispatchTypedEvent("tasksChange", []);

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<MessageLogStateEventMap> {
     };
 
     const onStatusChange = (): void => {
-      if (this.client?.getStatus() === "disconnected") {
+      if (isTerminalStatus(this.client?.getStatus())) {
         this.messages = [];
         this.pendingRequestEntries.clear();
         this.dispatchTypedEvent("messagesChange", []);

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<PagedPromptsStateEventMa
     super();
     this.client = client;
     const onStatusChange = (): void => {
-      if (this.client?.getStatus() === "disconnected") {
+      if (isTerminalStatus(this.client?.getStatus())) {
         this.prompts = [];
         this.dispatchTypedEvent("promptsChange", []);
       }

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<PagedRequestorTas
     super();
     this.client = client;
     const onStatusChange = (): void => {
-      if (this.client?.getStatus() === "disconnected") {
+      if (isTerminalStatus(this.client?.getStatus())) {
         this.tasks = [];
         this.nextCursor = undefined;
         this.dispatchTypedEvent("tasksChange", []);

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<PagedResourceT
     super();
     this.client = client;
     const onStatusChange = (): void => {
-      if (this.client?.getStatus() === "disconnected") {
+      if (isTerminalStatus(this.client?.getStatus())) {
         this.resourceTemplates = [];
         this.dispatchTypedEvent("resourceTemplatesChange", []);
       }

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<PagedResourcesStateEve
     super();
     this.client = client;
     const onStatusChange = (): void => {
-      if (this.client?.getStatus() === "disconnected") {
+      if (isTerminalStatus(this.client?.getStatus())) {
         this.resources = [];
         this.dispatchTypedEvent("resourcesChange", []);
       }

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<PagedToolsStateEventMap> {
     super();
     this.client = client;
     const onStatusChange = (): void => {
-      if (this.client?.getStatus() === "disconnected") {
+      if (isTerminalStatus(this.client?.getStatus())) {
         this.tools = [];
         this.dispatchTypedEvent("toolsChange", []);
       }

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<ResourceSubscri
     };
 
     const onStatusChange = (): void => {
-      if (this.client?.getStatus() === "disconnected") {
+      if (isTerminalStatus(this.client?.getStatus())) {
         this.subscribedUris = [];
         this.lastUpdatedByUri.clear();
         this.subscriptions = [];

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.