fix: prevent MCP server restart when toggling tool permissions #8633
+101
−6
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
Add isProgrammaticUpdate flag to distinguish between programmatic config updates and user-initiated file changes. Skip file watcher processing during programmatic updates to prevent unnecessary server restarts.
Fixed bug where MCP servers would reconnect instead of staying disabled when toggled off. The issue was that toggleServerDisabled() used stale in-memory config instead of reading the fresh config from disk after writing the disabled flag. Changes: Added readServerConfigFromFile() helper to read and validate server config from disk Updated disable path to read fresh config before calling connectToServer() Updated enable path to read fresh config before calling connectToServer() This ensures the disabled: true flag is properly read, causing connectToServer() to create a disabled placeholder connection instead of actually connecting the server. + refactor(mcp): use safeWriteJson for atomic config writes Replace JSON.stringify + fs.writeFile with safeWriteJson in McpHub.ts to prevent data corruption through atomic writes with file locking.
Replace multiple independent reset timers with a single timer that gets cleared and rescheduled on each programmatic config update. This prevents the flag from being reset prematurely when multiple rapid updates occur, which could cause unwanted server restarts during the file watcher's debounce period. + fix(mcp): ensure isProgrammaticUpdate flag cleanup with try-finally Wrap safeWriteJson() calls in try-finally blocks to guarantee the isProgrammaticUpdate flag is always reset, even if the write operation fails. This prevents the flag from being stuck at true indefinitely, which would cause subsequent user-initiated config changes to be silently ignored.
|
While testing these fixes I discovered a bug (not caused by my changes): Steps to reproduce:
Should I create a new issue for this bug, or try to fix this behavior as part of my PR? |
|
Thanks for the detailed repro steps. This appears to be a cross-tab state sync bug in the MCP settings flow, where changes made in a second Roo tab are applied to the first tab process and do not persist locally. To keep this PR focused on preventing unintended server restarts, I suggest we track the multi-tab persistence issue separately. I will open a new issue with your steps and link it here next. |
|
Summary:
No further changes planned in this PR for that bug. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Related GitHub Issue
Closes: #8231
Description
Problem
Toggling the "Always allow" checkbox for MCP tools in the chat view was causing unnecessary server restarts, disrupting active sessions and chat flow. This occurred because programmatic configuration file updates triggered the file watcher, which then restarted the server as if the user had manually edited the config.
Implementation
Implemented a flag-based mechanism to distinguish between programmatic configuration updates and user-initiated file changes:
isProgrammaticUpdateboolean flag toMcpHubclassdebounceConfigChange()to skip processing when flag is setupdateServerToolList()andupdateServerConfig()with flag protectionChanges Made
isProgrammaticUpdateflagupdateServerToolList()file writes with flagupdateServerConfig()file writes with flagTest Procedure
Manually tested by toggling "Always allow" for MCP tools in the chatview and verifying:
Pre-Submission Checklist
Documentation Updates
Additional Notes
Get in Touch
Discord:
@ocean.smithImportant
Prevents unnecessary MCP server restarts by using a flag to differentiate programmatic updates from user-initiated changes in
McpHub.isProgrammaticUpdateflag inMcpHub.debounceConfigChange()whenisProgrammaticUpdateis true.isProgrammaticUpdateflag toMcpHub.updateServerToolList()andupdateServerConfig()withisProgrammaticUpdateprotection.isProgrammaticUpdateflag during disposal indispose()method.This description was created by
for 9db94d3. You can customize this summary. It will automatically update as commits are pushed.