fix: prevent duplicate MCP server instances on refresh/restart #6885
Conversation
- Added proper cleanup in restartConnection to ensure old instances are fully closed - Added safeguards in connectToServer to check for existing connections - Improved deleteConnection to handle null checks for transport and client - Added delays to ensure cleanup completes before creating new connections This fixes the issue where refreshing MCP servers or clicking the refresh button on individual servers was causing new instances to start without closing old ones.
dosubot
bot
added
size:M
![roomote[bot]](https://avatars.githubusercontent.com/in/1546624?s=60&v=4){kind=small}
| ): 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 |
There was a problem hiding this comment.
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.
| // Ensure complete cleanup of existing connection | ||
| await this.deleteConnection(name, source) | ||
| // Wait a moment to ensure cleanup is complete | ||
| await delay(100) |
There was a problem hiding this comment.
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.
| await connection.transport.close() | ||
| await connection.client.close() | ||
| // Close transport first | ||
| if (connection.transport) { |
There was a problem hiding this comment.
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 this.deleteConnection(serverName, serverSource) | ||
|
|
||
| // Wait a bit to ensure cleanup is complete | ||
| await delay(500) |
There was a problem hiding this comment.
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.
hannesrudolph
added
the
Issue/PR - Triage
daniel-lxs
moved this from PR [Needs Review]
to PR [Needs Prelim Review]
in Roo Code Roadmap
hannesrudolph
added
PR - Needs Preliminary Review
and removed
Issue/PR - Triage
|
Closing this PR as it doesn't address the root cause of the duplicate MCP server instances issue. This problem needs better scoping to understand what's actually happening before implementing a fix. The current approach of adding delays and cleanup checks is treating symptoms rather than the underlying issue. A proper investigation is needed to identify why duplicate instances are being created in the first place. |
github-project-automation
bot
moved this from PR [Needs Prelim Review]
to Done
in Roo Code Roadmap
This PR fixes an issue where refreshing MCP servers or clicking the refresh button on individual servers was causing new instances to start without properly closing the old ones.
Problem
When users hit the "refresh MCP servers" button or the refresh button on an individual MCP server line, a new instance of the server was being started without closing the old one, leading to duplicate server processes.
Solution
restartConnectionmethod: Added proper cleanup sequence with delays to ensure old instances are fully closed before creating new onesconnectToServermethod: Added safeguards to check for and clean up existing connections before creating new onesdeleteConnectionmethod: Added null checks for transport and client to handle edge cases gracefullyChanges Made
restartConnectionto store the server source before deletion and ensure proper cleanupconnectToServerwith cleanup if founddeleteConnectionfor transport and client objectsTesting
Fixes the issue reported via Slack about duplicate MCP server instances.
Important
Fixes duplicate MCP server instances by ensuring proper cleanup before starting new instances in
McpHub.McpHub.restartConnectionandconnectToServerto ensure cleanup completion.restartConnection: Stores server source before deletion, ensures cleanup, and adds delay.connectToServer: Checks for existing connections, cleans up if found, and adds delay.deleteConnection: Adds null checks fortransportandclientto handle edge cases.McpHub.spec.ts).This description was created by
for afc5e7b. You can customize this summary. It will automatically update as commits are pushed.