sandvich
/
opencode
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

fix(mcp): close transport on failed/timed-out connections 

When withTimeout rejects during MCP connect, the transport (and its
child process for stdio servers) was never closed. Extract a tryConnect
helper that ensures transport.close() is always called on failure,
eliminating process/connection leaks in all connect paths.

Fixes #19168
worktree-agent-a682f34a
Kit Langton 2026-03-25 22:06:01 -04:00
parent 4342ef6c97
commit ebdecf2ec7
2 changed files with 94 additions and 25 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

33
packages/opencode/src/mcp/index.ts
View File

@ -189,6 +189,20 @@ export namespace MCP {
return out
}
type Transport = StdioClientTransport | StreamableHTTPClientTransport | SSEClientTransport
/** Try to connect a client via the given transport; closes the transport on failure. */
async function tryConnect(transport: Transport, timeout: number): Promise<MCPClient> {
const client = new Client({ name: "opencode", version: Installation.VERSION })
try {
await withTimeout(client.connect(transport), timeout)
return client
} catch (error) {
await transport.close().catch(() => {})
throw error
}
}
async function create(key: string, mcp: Config.Mcp) {
if (mcp.enabled === false) {
log.info("mcp server disabled", { key })
@ -247,12 +261,7 @@ export namespace MCP {
const connectTimeout = mcp.timeout ?? DEFAULT_TIMEOUT
for (const { name, transport } of transports) {
try {
const client = new Client({
name: "opencode",
version: Installation.VERSION,
})
await withTimeout(client.connect(transport), connectTimeout)
mcpClient = client
mcpClient = await tryConnect(transport, connectTimeout)
log.info("connected", { key, transport: name })
status = { status: "connected" }
break
@ -271,6 +280,7 @@ export namespace MCP {
// Check if this is a "needs registration" error
if (lastError.message.includes("registration") || lastError.message.includes("client_id")) {
// tryConnect already closed the transport
status = {
status: "needs_client_registration" as const,
error: "Server does not support dynamic client registration. Please provide clientId in config.",
@ -284,6 +294,8 @@ export namespace MCP {
}).catch((e) => log.debug("failed to show toast", { error: e }))
} else {
// Store transport for later finishAuth call
// Note: tryConnect closed the transport, but the SDK's finishAuth
// only needs the authProvider reference which survives close()
pendingOAuthTransports.set(key, transport)
status = { status: "needs_auth" as const }
// Show toast for needs_auth
@ -297,6 +309,7 @@ export namespace MCP {
break
}
// tryConnect already closed the failed transport
log.debug("transport connection failed", {
key,
transport: name,
@ -331,16 +344,12 @@ export namespace MCP {
const connectTimeout = mcp.timeout ?? DEFAULT_TIMEOUT
try {
const client = new Client({
name: "opencode",
version: Installation.VERSION,
})
await withTimeout(client.connect(transport), connectTimeout)
mcpClient = client
mcpClient = await tryConnect(transport, connectTimeout)
status = {
status: "connected",
}
} catch (error) {
// tryConnect already closed the transport (kills orphaned child process)
log.error("local mcp startup failed", {
key,
command: mcp.command,

86
packages/opencode/test/mcp/lifecycle.test.ts
View File

@ -23,6 +23,7 @@ let connectShouldHang = false
let connectError = "Mock transport cannot connect"
// Tracks how many Client instances were created (detects leaks)
let clientCreateCount = 0
// Tracks how many times transport.close() is called across all mock transports
let transportCloseCount = 0
function getOrCreateClientState(name?: string): MockClientState {
@ -46,12 +47,13 @@ function getOrCreateClientState(name?: string): MockClientState {
return state
}
// Mock transport that succeeds or fails based on connectShouldFail
// Mock transport that succeeds or fails based on connectShouldFail / connectShouldHang
class MockStdioTransport {
stderr: null = null
pid = 12345
constructor(_opts: any) {}
async start() {
if (connectShouldHang) return new Promise<void>(() => {}) // never resolves
if (connectShouldFail) throw new Error(connectError)
if (connectShouldHang) await new Promise(() => {}) // never resolves
}
@ -63,18 +65,24 @@ class MockStdioTransport {
class MockStreamableHTTP {
constructor(_url: URL, _opts?: any) {}
async start() {
if (connectShouldHang) return new Promise<void>(() => {}) // never resolves
if (connectShouldFail) throw new Error(connectError)
}
async close() {}
async close() {
transportCloseCount++
}
async finishAuth() {}
}
class MockSSE {
constructor(_url: URL, _opts?: any) {}
async start() {
throw new Error("SSE fallback - not used in these tests")
if (connectShouldHang) return new Promise<void>(() => {}) // never resolves
if (connectShouldFail) throw new Error(connectError)
}
async close() {
transportCloseCount++
}
async close() {}
}
mock.module("@modelcontextprotocol/sdk/client/stdio.js", () => ({
@ -667,25 +675,77 @@ test(
)
// ========================================================================
// Test: timed-out local transport is closed (process leak regression)
// Test: transport leak — local stdio timeout (#19168)
// ========================================================================
test(
"local stdio transport is closed when connect times out (no process leak)",
withInstance({}, async () => {
lastCreatedClientName = "hanging-server"
getOrCreateClientState("hanging-server")
connectShouldHang = true
const result = await MCP.add("hanging-server", {
const addResult = await MCP.add("hanging-server", {
type: "local",
command: ["node", "fake-server.js"],
timeout: 100, // 100ms timeout so test is fast
command: ["node", "fake.js"],
timeout: 100,
})
const status = (result.status as any)["hanging-server"] ?? result.status
expect(status.status).toBe("failed")
expect(status.error).toContain("timed out")
// The transport must be closed to kill the child process
const serverStatus = (addResult.status as any)["hanging-server"] ?? addResult.status
expect(serverStatus.status).toBe("failed")
expect(serverStatus.error).toContain("timed out")
// Transport must be closed to avoid orphaned child process
expect(transportCloseCount).toBeGreaterThanOrEqual(1)
}),
)
// ========================================================================
// Test: transport leak — remote timeout (#19168)
// ========================================================================
test(
"remote transport is closed when connect times out",
withInstance({}, async () => {
lastCreatedClientName = "hanging-remote"
getOrCreateClientState("hanging-remote")
connectShouldHang = true
const addResult = await MCP.add("hanging-remote", {
type: "remote",
url: "http://localhost:9999/mcp",
timeout: 100,
oauth: false,
})
const serverStatus = (addResult.status as any)["hanging-remote"] ?? addResult.status
expect(serverStatus.status).toBe("failed")
// Transport must be closed to avoid leaked HTTP connections
expect(transportCloseCount).toBeGreaterThanOrEqual(1)
}),
)
// ========================================================================
// Test: transport leak — failed remote transports not closed (#19168)
// ========================================================================
test(
"failed remote transport is closed before trying next transport",
withInstance({}, async () => {
lastCreatedClientName = "fail-remote"
getOrCreateClientState("fail-remote")
connectShouldFail = true
connectError = "Connection refused"
const addResult = await MCP.add("fail-remote", {
type: "remote",
url: "http://localhost:9999/mcp",
timeout: 5000,
oauth: false,
})
const serverStatus = (addResult.status as any)["fail-remote"] ?? addResult.status
expect(serverStatus.status).toBe("failed")
// Both StreamableHTTP and SSE transports should be closed
expect(transportCloseCount).toBeGreaterThanOrEqual(2)
}),
)