Skip to content

fix: prevent duplicate MCP server instances on refresh #6886

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
wants to merge 1 commit into from
Closed

fix: prevent duplicate MCP server instances on refresh #6886

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
22 changes: 18 additions & 4 deletions 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,13 @@ export class McpHub {
for (const connection of connections) {
try {
if (connection.type === "connected") {
await connection.transport.close()
await connection.client.close()
// Close transport first, then client
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.

Good comment, but could you explain WHY the order matters? Something like: 'Close transport first to stop incoming messages, then client to clean up handlers - prevents race conditions where new messages arrive during cleanup'

if (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.

The cleanup pattern (close transport, close client, delay) appears in multiple places. Could we extract this into a helper method like cleanupConnection() to reduce duplication and ensure consistency?

await connection.transport.close()
}
if (connection.client) {
await connection.client.close()
}
}
} catch (error) {
console.error(`Failed to close transport for ${name}:`, error)
Expand Down Expand Up @@ -1188,15 +1193,21 @@ export class McpHub {
await this.notifyWebviewOfServerChanges()
await delay(500) // artificial delay to show user that server is restarting
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.

Is the inconsistency between 100ms (cleanup delay) and 500ms (user feedback delay) intentional? Consider standardizing these or extracting them as named constants like CLEANUP_DELAY_MS and USER_FEEDBACK_DELAY_MS for better maintainability.

try {
await this.deleteConnection(serverName, connection.server.source)
// Ensure complete cleanup before restarting
const serverSource = connection.server.source || "global"
await this.deleteConnection(serverName, serverSource)

// Add small delay to ensure cleanup is complete
await delay(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.

This 100ms delay value appears multiple times. Consider defining it as a constant at the top of the file for easier maintenance and tuning.


// Parse the config to validate it
const parsedConfig = JSON.parse(config)
try {
// Validate the config
const validatedConfig = this.validateServerConfig(parsedConfig, serverName)

// Try to connect again using validated config
await this.connectToServer(serverName, validatedConfig, connection.server.source || "global")
await this.connectToServer(serverName, validatedConfig, serverSource)
vscode.window.showInformationMessage(t("mcp:info.server_connected", { serverName }))
} catch (validationError) {
this.showErrorMessage(`Invalid configuration for MCP server "${serverName}"`, validationError)
Expand Down Expand Up @@ -1265,6 +1276,9 @@ export class McpHub {
await this.deleteConnection(conn.server.name, conn.server.source)
}

// Add small delay to ensure cleanup is complete
await delay(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.

If cleanup fails for some connections, we still apply the delay. Should we handle partial cleanup failures more gracefully here? Maybe track which connections failed and retry them?


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