Bug: MCP stdio transport leaks child process when client.connect() fails during handshake

[marlonbarreto-git]

Description

In McpHub.connectToServer(), when connecting to an MCP server of type stdio, the transport is started (spawning a child process) before client.connect(transport) is called. If the MCP handshake fails, the catch block marks the connection as "disconnected" but never calls transport.close(), leaving the child process running as an orphan.

Root Cause

File: src/services/mcp/McpHub.ts

// Line ~755: Transport is started (spawns child process)
await transport.start()

// Line ~875: Connection pushed to this.connections BEFORE connect()
this.connections.push(connection)

// Line ~878: If this throws, the catch block doesn't close the transport
await client.connect(transport)

// Lines ~887-895: Catch block
} catch (error) {
    // ❌ No transport.close() — child process stays alive
    connection.server.status = "disconnected"
    // ... error handling, but transport is abandoned
}

Impact

sequenceDiagram
    participant Hub as McpHub
    participant T as StdioTransport
    participant CP as Child Process
    participant Client as MCP Client

    Hub->>T: transport.start()
    T->>CP: spawn child process ✅
    Hub->>Hub: this.connections.push(connection)
    Hub->>Client: client.connect(transport)
    Client-->>Hub: throws (handshake failed)
    Hub->>Hub: catch: set status = "disconnected"
    Note over T,CP: transport.close() never called ⚠️
    Note over CP: Child process stays alive forever ⚠️

Loading

Each retry spawns a new orphan. If the MCP server binary starts successfully but fails the protocol handshake (e.g., prints output but doesn't speak MCP), every connection attempt leaks another child process. Over time, this causes:

• Accumulating zombie processes
• Memory and CPU waste
• Potential port/resource exhaustion if the child process binds resources

Reproduction

1. Configure an MCP server of type stdio pointing to a binary that starts but doesn't speak MCP (e.g., echo "hello" or a web server binary)
2. Observe that the child process remains alive after the connection error
3. Trigger a retry (automatic or manual) — another orphaned process spawns
4. Check with ps aux | grep <binary-name> — processes accumulate

Proposed Fix

Add transport.close() in the catch block:

} catch (error) {
    try {
        await transport.close()
    } catch {
        // Ignore close errors
    }
    connection.server.status = "disconnected"
    // ... rest of error handling
}

Or use a try/finally pattern to ensure cleanup regardless of success/failure:

let connected = false
try {
    await client.connect(transport)
    connected = true
} finally {
    if (!connected) {
        try { await transport.close() } catch {}
    }
}

Environment

• Roo-Code: latest
• File: src/services/mcp/McpHub.ts
• Affects: All stdio-type MCP server connections

I'm happy to open a PR for this fix if you'd like.

[roomote]

I've analyzed this issue and here's my proposed implementation plan:

Problem confirmed: In src/services/mcp/McpHub.ts, the connectToServer() method calls transport.start() (line ~755) which spawns a child process for stdio transports, but the catch block (lines ~887-895) never calls transport.close() when client.connect(transport) fails, leaking child processes.

Proposed fix -- add transport.close() in the catch block:

} catch (error) {
    // Clean up the transport to prevent child process leaks (especially for stdio transports)
    const connection = this.findConnection(name, source)
    if (connection && "transport" in connection && connection.transport) {
        try {
            await connection.transport.close()
        } catch {
            // Ignore close errors during cleanup
        }
    }
    // Update status with error
    if (connection) {
        connection.server.status = "disconnected"
        this.appendErrorMessage(connection, error instanceof Error ? error.message : `${error}`)
    }
    throw error
}

This approach:

1. Uses the connection's transport reference (already pushed to this.connections before .connect())
2. Wraps transport.close() in its own try/catch so close errors don't mask the original error
3. Cleans up before setting the disconnected status, so the onclose handler (which also sets status to disconnected) fires cleanly
4. Applies to all transport types (stdio, SSE, streamable-http), which is safe since .close() is a no-op or harmless on non-stdio transports

I'll also add a test to verify the transport is closed on connection failure.

Please mention @roomote in a comment saying that you approve this plan, and I'll proceed with the implementation.