Skip to content

fix: prevent duplicate MCP server instances on refresh/restart #6885

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/restart #6885

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
34 changes: 27 additions & 7 deletions src/services/mcp/McpHub.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -618,8 +618,14 @@ export class McpHub {
config: z.infer<typeof ServerConfigSchema>,
source: "global" | "project" = "global",
): Promise<void> {
// Remove existing connection if it exists with the same source
await this.deleteConnection(name, source)
// Check if a connection already exists with the same name and source
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 this a potential race condition? If multiple calls to connectToServer happen simultaneously for the same server, they could both pass the existence check before either completes cleanup, potentially leading to duplicate connections. Consider implementing a connection lock or queue mechanism to prevent concurrent connection attempts for the same server.

const existingConnection = this.findConnection(name, source)
if (existingConnection) {
// Ensure complete cleanup of existing connection
await this.deleteConnection(name, source)
// Wait a moment 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.

The delay here is 100ms, but in restartConnection it's 500ms. Is this intentional? If so, could we add a comment explaining why different delays are needed? If not, consider using a consistent delay value.

}

// Check if MCP is globally enabled
const mcpEnabled = await this.isMcpEnabled()
Expand Down Expand Up @@ -1007,8 +1013,14 @@ export class McpHub {
for (const connection of connections) {
try {
if (connection.type === "connected") {
await connection.transport.close()
await connection.client.close()
// Close transport first
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.

While the null checks are good, errors are only logged to console. Should we track these in the server's error history for better debugging visibility?

await connection.transport.close()
}
// Then close client
if (connection.client) {
await connection.client.close()
}
}
} catch (error) {
console.error(`Failed to close transport for ${name}:`, error)
Expand Down Expand Up @@ -1186,17 +1198,25 @@ export class McpHub {
connection.server.status = "connecting"
connection.server.error = ""
await this.notifyWebviewOfServerChanges()
await delay(500) // artificial delay to show user that server is restarting

try {
await this.deleteConnection(serverName, connection.server.source)
// Store the source before deleting
const serverSource = connection.server.source || "global"

// Ensure complete cleanup of the old connection
await this.deleteConnection(serverName, serverSource)

// Wait a bit to ensure cleanup is complete
await delay(500)
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.

Consider extracting these delay values to named constants at the top of the file for better maintainability. For example, CONNECTION_RESTART_DELAY_MS = 500 and CONNECTION_CLEANUP_DELAY_MS = 100.


// 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
Loading