From 92a6a7b729bfd30a1fb71c4207f926ff27e401cf Mon Sep 17 00:00:00 2001 From: Chen Tang Date: Mon, 10 Aug 2026 14:21:30 +0800 Subject: [PATCH] fix(mcp): close transports whose connection never reached the client registry MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit initialize() registers a connection with this.clients.push() on the try block's last line, and shutdown() only walks that array. A connect() or listTools() failure therefore left a started transport — an spawned stdio child or an open http connection — that nothing would ever close. On a resident box (SICLAW_AGENTBOX_IDLE_TIMEOUT=0) these accumulate for the pod's lifetime, one per session created while an MCP server is unreachable. - hoist client/transport out of the try so the catch can reach them; the old `let transport` was block-scoped inside it and unreachable from the handler - add discardUnregisteredConnection(): best-effort close of both handles, since which one owns the live resource depends on how far the handshake got - test both failure paths, the success path, and that a throwing cleanup neither escapes initialize() nor aborts discovery of the remaining servers --- src/core/mcp-client.test.ts | 105 +++++++++++++++++++++++++++++++++++- src/core/mcp-client.ts | 42 ++++++++++++++- 2 files changed, 144 insertions(+), 3 deletions(-) diff --git a/src/core/mcp-client.test.ts b/src/core/mcp-client.test.ts index 050fbdf79..e65014cc1 100644 --- a/src/core/mcp-client.test.ts +++ b/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 => {}, + 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 { + 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"]); + }); +}); diff --git a/src/core/mcp-client.ts b/src/core/mcp-client.ts index 7dcf4cbb3..b20931aa3 100644 --- a/src/core/mcp-client.ts +++ b/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 } | undefined, + transport: { close?: () => Promise } | undefined, + ): Promise { + 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. */