fix: prevent disabled MCP servers from starting processes and show correct status #6037
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…rrect status - Add disabled check in updateServerConnections() before connecting to servers - Disconnect servers immediately when toggling to disabled state - Update UI to show grey/disabled color for disabled servers - Add comprehensive tests for disabled server behavior Fixes #6036
dosubot
bot
added
size:L
hannesrudolph
added
the
Issue/PR - Triage
- Centralize disabled state validation in connectToServer() method - Add protection against race conditions in toggleServerDisabled() - Remove redundant grey color indicator (keep only opacity) - Remove unnecessary status checks in toggle logic - Add comprehensive test coverage for edge cases - Fix failing test by ensuring correct disabled state in config
- Modified getServers() to filter out disabled servers (for prompt system) - getAllServers() returns all servers including disabled (for UI) - Updated updateServerConnections to create connection objects for disabled servers - Fixed toggleServerDisabled to properly handle re-enabling servers - Added test coverage for getAllServers() method This ensures disabled servers appear in the UI with visual indicators while preventing their use in prompts and tool calls.
dosubot
bot
added
size:XXL
…om restarting - Removed redundant refCount logic from McpHub that conflicted with singleton pattern - Made registerClient/unregisterClient no-ops to maintain API compatibility - Added checks in restartConnection and setupFileWatcher to skip disabled servers - Added comprehensive test coverage for disabled server restart prevention - This fixes the issue where moving the Roo view between panes could disconnect all MCP servers
hannesrudolph
force-pushed
the
fix/disabled-mcp-servers-issue-6036
branch
from
67635d3 to
b4e1337
Compare
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
|
|
||
| // Create a placeholder connection for disabled servers | ||
| // This is needed so the server still appears in the UI | ||
| const parsedConfig = JSON.parse(storedConfig) |
Contributor
There was a problem hiding this comment.
Consider wrapping the JSON.parse call on storedConfig in a try/catch block to guard against invalid JSON and log parsing errors for easier debugging.
This comment was generated because it violated a code review rule: irule_PTI8rjtnhwrWq6jS.
dosubot
bot
added
size:XL
- Add isInitializing flag to prevent file watchers from creating duplicate connections - Refactor constructor to use async initialization method - Skip config change handling during initialization phase - Add test to verify no duplicate connections are created - Fixes issue where Docker (MCP) processes were duplicated on startup
dosubot
bot
added
size:XXL
…watcher - Added check in handleConfigFileChange to ignore file change events for servers with active toggle operations - This prevents the race condition where disabling a server triggers a config write, which triggers the file watcher, which would restart the server - Fixes the issue where disabled servers would immediately restart after being shut down
…rvers - Added null checks in deleteConnection to prevent errors when closing disabled servers - Disabled servers have null client and transport, so we need to check before calling close() - Updated test expectations for concurrent toggle operations (now allows multiple executions) - Fixes the 'Cannot read properties of null' error when toggling server state
…state - Use configChangeDebounceTimers map to flag programmatic config changes - Set null timer value before updating config to signal file watcher to skip - File watcher checks for null flag and skips processing if found - Automatically clear flag after 1 second to restore normal operation - Added debug logging to trace execution flow - Fixes issue where both toggleServerDisabled and file watcher would call updateServerConnections
hannesrudolph
added
PR - Draft / In Progress
and removed
Issue/PR - Triage
github-project-automation
bot
moved this from PR [Draft / In Progress]
to Done
in Roo Code Roadmap
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #6036
This PR fixes the issue where disabled MCP servers were still starting processes and showing incorrect status indicators in the UI.
Changes:
All existing tests pass and new tests verify the fix works correctly.
Important
Fixes issue where disabled MCP servers incorrectly start processes and show incorrect status by updating server connection logic and UI.
updateServerConnections()inMcpHub.ts.toggleServerDisabled()inMcpHub.tsto disconnect servers when disabling and reconnect when enabling.McpView.tsxto show grey/disabled color for disabled servers.McpHub.spec.ts, including tests for initialization, toggling, and error handling.McpHub.tsto prevent premature disposal.This description was created by
for a1f24ec. You can customize this summary. It will automatically update as commits are pushed.