fix: prevent duplicate MCP server instances on refresh #6886
Conversation
- Add proper cleanup delays in restartConnection and refreshAllConnections - Ensure transport and client are properly closed before creating new connections - Fix race condition where new connections were created before old ones were fully cleaned up - Maintain backward compatibility with existing tests Fixes issue where refreshing MCP servers or individual server refresh was causing duplicate instances without cleaning up the old ones.
dosubot
bot
added
size:S
![roomote[bot]](https://avatars.githubusercontent.com/in/1546624?s=60&v=4){kind=small}
| connection.server.status = "connecting" | ||
| connection.server.error = "" | ||
| await this.notifyWebviewOfServerChanges() | ||
| await delay(500) // artificial delay to show user that server is restarting |
There was a problem hiding this comment.
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.
| await connection.transport.close() | ||
| await connection.client.close() | ||
| // Close transport first, then client | ||
| if (connection.transport) { |
There was a problem hiding this comment.
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?
| if (connection.type === "connected") { | ||
| await connection.transport.close() | ||
| await connection.client.close() | ||
| // Close transport first, then client |
There was a problem hiding this comment.
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'
| } | ||
|
|
||
| // Add small delay to ensure cleanup is complete | ||
| await delay(100) |
There was a problem hiding this comment.
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?
| await this.deleteConnection(serverName, serverSource) | ||
|
|
||
| // Add small delay to ensure cleanup is complete | ||
| await delay(100) |
There was a problem hiding this comment.
This 100ms delay value appears multiple times. Consider defining it as a constant at the top of the file for easier maintenance and tuning.
hannesrudolph
added
the
Issue/PR - Triage
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
Problem
When hitting the "refresh MCP servers" button or the refresh button on individual MCP server lines, new instances of the servers were being started without properly closing the old ones, resulting in duplicate server processes.
Root Cause
The issue was caused by a race condition in the
restartConnectionandrefreshAllConnectionsmethods where:Solution
restartConnectionandrefreshAllConnectionsmethodsChanges Made
deleteConnectionto properly close transport and client with null checksrestartConnectionto ensure complete cleanup with proper source trackingrefreshAllConnectionsto add cleanup delay before re-initializationTesting
Fixes issue reported via Slack where refreshing MCP servers was causing duplicate instances.
Important
Fixes race condition in
McpHub.tsto prevent duplicate MCP server instances by ensuring proper cleanup and adding delays inrestartConnectionandrefreshAllConnections.restartConnectionandrefreshAllConnectionsinMcpHub.tsto prevent duplicate server instances.deleteConnectionto close transport and client with null checks.restartConnectionto ensure complete cleanup with proper source tracking.refreshAllConnectionsto add cleanup delay before re-initialization.This description was created by
for 87e1203. You can customize this summary. It will automatically update as commits are pushed.