fix(mcp): close transports whose connection never reached the client …

src/core/mcp-client.test.ts

@@ -1,4 +1,40 @@
-import { describe, it, expect } from "vitest";
+import { describe, it, expect, vi, beforeEach } from "vitest";
+
+// MCP SDK doubles. initialize() lazy-imports the SDK, so the connection lifecycle
+// is only reachable in a test by standing in for those four modules. Everything
+// else in this file exercises pure functions and is unaffected by these mocks.
+const sdk = vi.hoisted(() => {
+  const state = {
+    connect: async (_transport: unknown): Promise<void> => {},
+    listTools: async (): Promise<{ tools: unknown[] }> => ({ tools: [] }),
+    /** Ordered log of every close() the manager performed. */
+    closed: [] as string[],
+    /** When set, every transport close() rejects with it. */
+    transportCloseError: null as Error | null,
+  };
+  const transportDouble = (label: string) =>
+    class {
+      constructor(..._args: unknown[]) {}
+      async close(): Promise<void> {
+        state.closed.push(label);
+        if (state.transportCloseError) throw state.transportCloseError;
+      }
+    };
+  return { state, transportDouble };
+});
+
+vi.mock("@modelcontextprotocol/sdk/client/index.js", () => ({
+  Client: class {
+    constructor(..._args: unknown[]) {}
+    async connect(transport: unknown) { return sdk.state.connect(transport); }
+    async listTools() { return sdk.state.listTools(); }
+    async close() { sdk.state.closed.push("client"); }
+  },
+}));
+vi.mock("@modelcontextprotocol/sdk/client/stdio.js", () => ({ StdioClientTransport: sdk.transportDouble("stdio") }));
+vi.mock("@modelcontextprotocol/sdk/client/sse.js", () => ({ SSEClientTransport: sdk.transportDouble("sse") }));
+vi.mock("@modelcontextprotocol/sdk/client/streamableHttp.js", () => ({ StreamableHTTPClientTransport: sdk.transportDouble("http") }));
+
 import { jsonSchemaToTypebox, normalizeMcpInputSchema, buildMcpToolName, isMcpTool, MCP_TOOL_PREFIX, mcpContentToAgentContent, McpClientManager } from "./mcp-client.js";
 
 describe("jsonSchemaToTypebox", () => {
@@ -207,3 +243,70 @@ describe("normalizeMcpInputSchema", () => {
     expect(normalizeMcpInputSchema([1, 2]) as any).toMatchObject({ type: "object", properties: {} });
   });
 });
+
+describe("McpClientManager.initialize connection cleanup", () => {
+  // A connection that fails before `this.clients.push()` is invisible to
+  // shutdown(), which only walks that array. Without an explicit close in the
+  // catch, the started transport (stdio child process / open http connection)
+  // leaks — and since initialize() runs once per session on a resident box
+  // (SICLAW_AGENTBOX_IDLE_TIMEOUT=0), nothing ever reclaims it.
+  const stdioServer = { mcpServers: { probe: { command: "/bin/true" } } } as any;
+
+  beforeEach(() => {
+    sdk.state.closed = [];
+    sdk.state.connect = async () => {};
+    sdk.state.listTools = async () => ({ tools: [] });
+    sdk.state.transportCloseError = null;
+  });
+
+  it("closes the transport when connect() fails", async () => {
+    sdk.state.connect = async () => { throw new Error("ECONNREFUSED"); };
+    await new McpClientManager(stdioServer).initialize();
+    expect(sdk.state.closed).toContain("stdio");
+  });
+
+  it("closes the connection when listTools() fails after a completed handshake", async () => {
+    sdk.state.listTools = async () => { throw new Error("tools/list timed out"); };
+    await new McpClientManager(stdioServer).initialize();
+    // connect() returned, so the client owns a live transport: closing either
+    // handle releases it. Assert the connection was released, not which handle did it.
+    expect(sdk.state.closed.length).toBeGreaterThan(0);
+  });
+
+  it("leaves a successful connection open for shutdown() to close", async () => {
+    sdk.state.listTools = async () => ({ tools: [{ name: "ping" }] });
+    const manager = new McpClientManager(stdioServer);
+    await manager.initialize();
+
+    expect(sdk.state.closed).toEqual([]);
+    expect(manager.getTools()).toHaveLength(1);
+
+    await manager.shutdown();
+    expect(sdk.state.closed).toContain("client");
+  });
+
+  it("does not let a cleanup failure escape initialize()", async () => {
+    sdk.state.connect = async () => { throw new Error("ECONNREFUSED"); };
+    sdk.state.transportCloseError = new Error("close hung");
+    await expect(new McpClientManager(stdioServer).initialize()).resolves.toBeUndefined();
+  });
+
+  it("keeps initializing the remaining servers after one fails to clean up", async () => {
+    // The cleanup is awaited inside the loop, so a throwing close() must not
+    // abort the iteration and cost every later server its tools.
+    sdk.state.transportCloseError = new Error("close hung");
+    sdk.state.connect = async () => {
+      // Fail the first server only; the second completes its handshake.
+      sdk.state.connect = async () => {};
+      throw new Error("ECONNREFUSED");
+    };
+    sdk.state.listTools = async () => ({ tools: [{ name: "ping" }] });
+
+    const manager = new McpClientManager({
+      mcpServers: { broken: { command: "/bin/true" }, healthy: { url: "https://mcp.example/mcp" } },
+    } as any);
+    await manager.initialize();
+
+    expect(manager.getTools().map((t) => t.name)).toEqual(["mcp__healthy__ping"]);
+  });
+});

src/core/mcp-client.ts

@@ -213,12 +213,20 @@ export class McpClientManager {
     const { StreamableHTTPClientTransport } = await import("@modelcontextprotocol/sdk/client/streamableHttp.js");
 
     for (const [serverName, serverConfig] of entries) {
+      // Both handles live outside the try because the catch has to reach them.
+      // A connection that fails before the `this.clients.push()` on the try's last
+      // line is invisible to shutdown(), which only walks that array — so without
+      // an explicit close here nothing ever releases it. The leak is real, not
+      // theoretical: connect() can fail with the transport already started (stdio
+      // spawned a child, http opened a connection), and a listTools() failure means
+      // the handshake definitely completed.
+      let client: any;
+      let transport: any;
       try {
-        const client = new Client(
+        client = new Client(
           { name: `siclaw-mcp-${serverName}`, version: "1.0.0" },
         );
 
-        let transport: any;
         // Auto-detect transport when not explicitly set: url → streamable-http, command → stdio
         const cfg = serverConfig as any;
         const detectedTransport: string = cfg.transport
@@ -267,6 +275,7 @@ export class McpClientManager {
         this.clients.push({ serverName, client, transport });
       } catch (err) {
         console.error(`[mcp-client] Failed to connect to "${serverName}":`, err);
+        await this.discardUnregisteredConnection(serverName, client, transport);
       }
     }
 
@@ -303,6 +312,35 @@ export class McpClientManager {
     this.tools = [];
   }
 
+  /**
+   * Release a connection that failed before it was registered in `this.clients`.
+   *
+   * Both handles are tried because which one owns the live resource depends on how
+   * far the handshake got: once connect() has returned, Client.close() is what
+   * drives the transport down; if it threw, the raw transport may be all that was
+   * ever started. Closing an already-closed transport is a no-op, and Client.close()
+   * on a client that never connected has no transport to reach — so trying both is
+   * cheap and covers every exit point of the try block above.
+   *
+   * Best-effort by construction: this runs while an error is already being handled,
+   * and a cleanup failure must not mask the original one. That is the only reason
+   * these catches swallow — the real error was logged by the caller first.
+   */
+  private async discardUnregisteredConnection(
+    serverName: string,
+    client: { close?: () => Promise<void> } | undefined,
+    transport: { close?: () => Promise<void> } | undefined,
+  ): Promise<void> {
+    for (const handle of [client, transport]) {
+      if (typeof handle?.close !== "function") continue;
+      try {
+        await handle.close();
+      } catch (err) {
+        console.warn(`[mcp-client] Cleanup after the failed "${serverName}" connection did not complete:`, err);
+      }
+    }
+  }
+
   /**
    * Create a pi-agent ToolDefinition from an MCP tool descriptor.
    */