Skip to content

fix(mcp): properly terminate stdio processes when refreshing MCP servers #6887

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
roomote wants to merge 1 commit into from
Closed

fix(mcp): properly terminate stdio processes when refreshing MCP servers #6887

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 29 additions & 1 deletion src/services/mcp/McpHub.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1007,8 +1007,28 @@ export class McpHub {
for (const connection of connections) {
try {
if (connection.type === "connected") {
await connection.transport.close()
// First close the client to stop any ongoing operations
Copy link
Contributor Author

@roomote roomote bot Aug 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This critical process termination logic should have unit tests. Consider adding tests to verify:

  • Processes are properly terminated on refresh
  • Docker containers are cleaned up
  • Error handling works when processes are already dead
  • The delays are sufficient for cleanup

await connection.client.close()

// For stdio transports, we need to ensure the process is terminated
if (connection.transport && "proc" in connection.transport) {
Copy link
Contributor Author

@roomote roomote bot Aug 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Type safety concern: Using "proc" in connection.transport and casting to any bypasses TypeScript's type system. Could we add a proper type guard or extend the StdioClientTransport interface to include the proc property?

const proc = (connection.transport as any).proc
if (proc && !proc.killed) {
// Try to gracefully terminate the process first
proc.kill("SIGTERM")
Copy link
Contributor Author

@roomote roomote bot Aug 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing error handling for process termination. The proc.kill() calls could throw if the process has already exited. Should we wrap these in try-catch blocks?

Suggested change
proc.kill("SIGTERM")
// Try to gracefully terminate the process first
try {
proc.kill("SIGTERM")
} catch (error) {
// Process might have already exited
console.debug(`Process already terminated: ${error}`)
}

Copy link
Contributor Author

@roomote roomote bot Aug 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cross-platform concern: SIGTERM might not work properly on Windows. Should we use platform-specific termination methods or consider using a library like tree-kill for cross-platform process termination?


// Give it a moment to terminate gracefully
await new Promise((resolve) => setTimeout(resolve, 100))
Copy link
Contributor Author

@roomote roomote bot Aug 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Race condition risk here. The 100ms delay between SIGTERM and SIGKILL might not be sufficient for all processes, especially Docker containers. Consider:

Suggested change
await new Promise((resolve) => setTimeout(resolve, 100))
// Try to gracefully terminate the process first
proc.kill("SIGTERM")
// Give it more time to terminate gracefully (especially for Docker)
await new Promise((resolve) => setTimeout(resolve, 500))
// If still not killed, force kill it
if (!proc.killed) {
proc.kill("SIGKILL")
}

Alternatively, could we listen to the process 'exit' event for a more deterministic approach?


// If still not killed, force kill it
if (!proc.killed) {
proc.kill("SIGKILL")
}
}
}

// Now close the transport
await connection.transport.close()
}
} catch (error) {
console.error(`Failed to close transport for ${name}:`, error)
Expand Down Expand Up @@ -1188,7 +1208,12 @@ export class McpHub {
await this.notifyWebviewOfServerChanges()
await delay(500) // artificial delay to show user that server is restarting
try {
// Ensure complete cleanup before reconnecting
await this.deleteConnection(serverName, connection.server.source)

// Add a small delay to ensure the process is fully terminated
await delay(200)
Copy link
Contributor Author

@roomote roomote bot Aug 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These hardcoded delays (200ms here, 300ms in refreshAllConnections) seem arbitrary. Different systems and Docker containers might need different delays. Should we make these configurable or use a more deterministic approach like waiting for process exit events?


// Parse the config to validate it
const parsedConfig = JSON.parse(config)
try {
Expand Down Expand Up @@ -1265,6 +1290,9 @@ export class McpHub {
await this.deleteConnection(conn.server.name, conn.server.source)
}

// Add a delay to ensure all processes are fully terminated
await delay(300)

// Re-initialize all servers from scratch
// This ensures proper initialization including fetching tools, resources, etc.
await this.initializeMcpServers("global")
Expand Down
Loading