Skip to content

Commit 625c0cb

Browse files
roomoteroomote
committed
fix: properly fix MCP server race condition instead of using retry logic
- Set up stderr handler BEFORE starting transport to avoid race condition - Remove unnecessary retry logic from both McpHub and useMcpToolTool - Fix the root cause: handlers were being set up after transport.start() - This ensures no events are missed between start() and handler setup - Applies to all stdio MCP servers, not just marketplace ones
1 parent c5c9d2f commit 625c0cb

File tree

3 files changed

+70
-146
lines changed

3 files changed

+70
-146
lines changed

‎src/core/tools/useMcpToolTool.ts‎

Lines changed: 17 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -127,50 +127,27 @@ async function executeToolAndProcessResult(
127127
toolName,
128128
})
129129

130-
// Try to execute the tool with retry logic for connection failures
130+
// Execute the tool
131131
let toolResult: any
132-
let retryCount = 0
133-
const maxRetries = 2
134-
const retryDelay = 1000 // 1 second
135-
136-
while (retryCount <= maxRetries) {
137-
try {
138-
const mcpHub = cline.providerRef.deref()?.getMcpHub()
139-
if (!mcpHub) {
140-
throw new Error("MCP Hub not available")
141-
}
142-
143-
toolResult = await mcpHub.callTool(serverName, toolName, parsedArguments)
144-
break // Success, exit the retry loop
145-
} catch (error: any) {
146-
const errorMessage = error?.message || String(error)
147-
148-
// Check if this is a connection closed error that might benefit from a retry
149-
if (errorMessage.includes("Connection closed") || errorMessage.includes("No connection found")) {
150-
retryCount++
151-
152-
if (retryCount <= maxRetries) {
153-
console.log(
154-
`MCP tool execution failed with connection error, retrying (${retryCount}/${maxRetries})...`,
155-
)
156-
157-
// Wait before retrying
158-
await new Promise((resolve) => setTimeout(resolve, retryDelay))
132+
try {
133+
const mcpHub = cline.providerRef.deref()?.getMcpHub()
134+
if (!mcpHub) {
135+
throw new Error("MCP Hub not available")
136+
}
159137

160-
// Send retry status
161-
await sendExecutionStatus(cline, {
162-
executionId,
163-
status: "output",
164-
response: `Connection error, retrying (${retryCount}/${maxRetries})...`,
165-
})
138+
toolResult = await mcpHub.callTool(serverName, toolName, parsedArguments)
139+
} catch (error: any) {
140+
const errorMessage = error?.message || String(error)
166141

167-
continue // Try again
168-
}
169-
}
142+
// Send error status
143+
await sendExecutionStatus(cline, {
144+
executionId,
145+
status: "error",
146+
error: errorMessage,
147+
})
170148

171-
// If we've exhausted retries or it's a different error, throw it
172-
throw error
173-
}
149+
// Re-throw to be handled by the caller
150+
throw error
174151
}
175152

176153
let toolResultPretty = "(No response)"

‎src/services/mcp/McpHub.ts‎

Lines changed: 47 additions & 100 deletions
Original file line numberDiff line numberDiff line change
@@ -797,112 +797,59 @@ export class McpHub {
797797
}
798798
this.connections.push(connection)
799799

800-
// For stdio transports, we need to handle the connection differently
800+
// For stdio transports, we need to handle the connection properly to avoid race conditions
801801
if (configInjected.type === "stdio") {
802-
// Store command and args for retry logic
803-
const isWindows = process.platform === "win32"
804-
const isAlreadyWrapped =
805-
configInjected.command.toLowerCase() === "cmd.exe" || configInjected.command.toLowerCase() === "cmd"
806-
const command = isWindows && !isAlreadyWrapped ? "cmd.exe" : configInjected.command
807-
const args =
808-
isWindows && !isAlreadyWrapped
809-
? ["/c", configInjected.command, ...(configInjected.args || [])]
810-
: configInjected.args
811-
812-
// Try to connect with retry logic for stdio servers
813-
let connected = false
814-
let retryCount = 0
815-
const maxRetries = 3
816-
const retryDelay = 1000 // 1 second
817-
818-
while (!connected && retryCount < maxRetries) {
819-
try {
820-
// Start the transport and connect
821-
await (transport as StdioClientTransport).start()
822-
823-
// Set up stderr handler after transport is started
824-
const stderrStream = (transport as StdioClientTransport).stderr
825-
if (stderrStream) {
826-
stderrStream.on("data", async (data: Buffer) => {
827-
const output = data.toString()
828-
// Check if output contains INFO level log
829-
const isInfoLog = /INFO/i.test(output)
830-
831-
if (isInfoLog) {
832-
// Log normal informational messages
833-
console.log(`Server "${name}" info:`, output)
834-
} else {
835-
// Treat as error log
836-
console.error(`Server "${name}" stderr:`, output)
837-
const connection = this.findConnection(name, source)
838-
if (connection) {
839-
this.appendErrorMessage(connection, output)
840-
if (connection.server.status === "disconnected") {
841-
await this.notifyWebviewOfServerChanges()
842-
}
843-
}
844-
}
845-
})
846-
}
847-
848-
// Now connect the client
849-
await client.connect(transport)
850-
connected = true
851-
connection.server.status = "connected"
852-
connection.server.error = ""
853-
} catch (connectError) {
854-
retryCount++
855-
console.error(
856-
`Failed to connect to MCP server "${name}" (attempt ${retryCount}/${maxRetries}):`,
857-
connectError,
858-
)
859-
860-
if (retryCount < maxRetries) {
861-
// Wait before retrying
862-
await new Promise((resolve) => setTimeout(resolve, retryDelay))
863-
864-
// Create a new transport for the retry
865-
if (configInjected.type === "stdio") {
866-
transport = new StdioClientTransport({
867-
command,
868-
args,
869-
cwd: configInjected.cwd,
870-
env: {
871-
...getDefaultEnvironment(),
872-
...(configInjected.env || {}),
873-
},
874-
stderr: "pipe",
875-
})
876-
877-
// Re-setup error handlers for the new transport
878-
transport.onerror = async (error) => {
879-
console.error(`Transport error for "${name}":`, error)
880-
const connection = this.findConnection(name, source)
881-
if (connection) {
882-
connection.server.status = "disconnected"
883-
this.appendErrorMessage(
884-
connection,
885-
error instanceof Error ? error.message : `${error}`,
886-
)
887-
}
888-
await this.notifyWebviewOfServerChanges()
889-
}
890-
891-
transport.onclose = async () => {
892-
const connection = this.findConnection(name, source)
893-
if (connection) {
894-
connection.server.status = "disconnected"
895-
}
802+
// Set up stderr handler BEFORE starting the transport to avoid race conditions
803+
const stderrStream = (transport as StdioClientTransport).stderr
804+
if (stderrStream) {
805+
stderrStream.on("data", async (data: Buffer) => {
806+
const output = data.toString()
807+
// Check if output contains INFO level log
808+
const isInfoLog = /INFO/i.test(output)
809+
810+
if (isInfoLog) {
811+
// Log normal informational messages
812+
console.log(`Server "${name}" info:`, output)
813+
} else {
814+
// Treat as error log
815+
console.error(`Server "${name}" stderr:`, output)
816+
const connection = this.findConnection(name, source)
817+
if (connection) {
818+
this.appendErrorMessage(connection, output)
819+
if (connection.server.status === "disconnected") {
896820
await this.notifyWebviewOfServerChanges()
897821
}
898-
899-
// Update the connection's transport reference
900-
connection.transport = transport
901822
}
902-
} else {
903-
throw connectError
904823
}
824+
})
825+
}
826+
827+
try {
828+
// Start the transport AFTER setting up handlers
829+
await (transport as StdioClientTransport).start()
830+
831+
// Now connect the client
832+
await client.connect(transport)
833+
connection.server.status = "connected"
834+
connection.server.error = ""
835+
} catch (connectError) {
836+
console.error(`Failed to connect to MCP server "${name}":`, connectError)
837+
838+
// Update status with error
839+
connection.server.status = "disconnected"
840+
this.appendErrorMessage(
841+
connection,
842+
connectError instanceof Error ? connectError.message : `${connectError}`,
843+
)
844+
845+
// Clean up on failure
846+
try {
847+
await transport.close()
848+
} catch (closeError) {
849+
console.error(`Failed to close transport after connection error:`, closeError)
905850
}
851+
852+
throw connectError
906853
}
907854
} else {
908855
// For non-stdio transports, connect normally

‎src/services/mcp/__tests__/McpHub.spec.ts‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -237,8 +237,8 @@ describe("McpHub", () => {
237237
if (connection && connection.type === "connected") {
238238
expect(connection.client).toBeDefined()
239239
expect(connection.transport).toBeDefined()
240-
// Status could be "connecting" or "connected" depending on timing
241-
expect(["connecting", "connected"]).toContain(connection.server.status)
240+
// Status should be "connected" after successful connection
241+
expect(connection.server.status).toBe("connected")
242242
} else {
243243
throw new Error("Connection should be of type 'connected'")
244244
}
@@ -1569,8 +1569,8 @@ describe("McpHub", () => {
15691569
// Verify server is connected
15701570
const connectedServer = mcpHub.connections.find((conn) => conn.server.name === "toggle-test-server")
15711571
expect(connectedServer).toBeDefined()
1572-
// Status could be "connecting" or "connected" depending on timing
1573-
expect(["connecting", "connected"]).toContain(connectedServer!.server.status)
1572+
// Status should be "connected" after successful connection
1573+
expect(connectedServer!.server.status).toBe("connected")
15741574
expect(connectedServer!.client).toBeDefined()
15751575
expect(connectedServer!.transport).toBeDefined()
15761576

@@ -1698,8 +1698,8 @@ describe("McpHub", () => {
16981698

16991699
// Verify that the server is connected
17001700
expect(enabledServer).toBeDefined()
1701-
// Status could be "connecting" or "connected" depending on timing
1702-
expect(["connecting", "connected"]).toContain(enabledServer!.server.status)
1701+
// Status should be "connected" after successful connection
1702+
expect(enabledServer!.server.status).toBe("connected")
17031703
expect(enabledServer!.client).toBeDefined()
17041704
expect(enabledServer!.transport).toBeDefined()
17051705

0 commit comments

Comments
 (0)