fix: prevent MCP servers from restarting on every settings file re-read#5897
Conversation
The config comparison in updateServerConnections compared the stored validated+injected config against the raw config from the settings file. Since Zod validation applies defaults (timeout, alwaysAllow, disabledTools, cwd, type), the two sides were never deep-equal for typical configs that omit optional fields, causing every file re-read to trigger unnecessary server restarts. Fix by applying both validation and variable injection to the incoming config before comparing, so both sides of the deepEqual check are in the same normalized form. Fixes Kilo-Org#5781, Kilo-Org#5791
🦋 Changeset detectedLatest commit: 0a3fae1 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
There was a problem hiding this comment.
Pull request overview
This PR fixes an issue where MCP servers restart unnecessarily on every settings file re-read, even when the configuration hasn't actually changed. The root cause is an asymmetric comparison in McpHub.updateServerConnections where the stored config (which has been through Zod validation and variable injection) was being compared against raw config from the settings file (not validated or injected). The PR applies injectVariables to the validated config before comparison to normalize both sides.
Changes:
- Modified the config comparison logic in
McpHub.updateServerConnectionsto inject variables into the validated config before comparing with the stored config
| } else if ( | ||
| !deepEqual( | ||
| JSON.parse(currentConnection.server.config), | ||
| await injectVariables(validatedConfig, { | ||
| env: process.env, | ||
| workspaceFolder: vscode.workspace.workspaceFolders?.[0]?.uri.fsPath ?? "", | ||
| }), | ||
| ) | ||
| ) { |
There was a problem hiding this comment.
This code change should be marked with a kilocode_change comment. According to the project's guidelines for maintaining a fork of Roo, all changes should be marked to enable easier merging. Since this is a multi-line change, it should be wrapped with // kilocode_change start and // kilocode_change end comments.
| } else if ( | ||
| !deepEqual( | ||
| JSON.parse(currentConnection.server.config), | ||
| await injectVariables(validatedConfig, { | ||
| env: process.env, | ||
| workspaceFolder: vscode.workspace.workspaceFolders?.[0]?.uri.fsPath ?? "", | ||
| }), | ||
| ) | ||
| ) { |
There was a problem hiding this comment.
The comparison assumes that the stored config is always injected with variables, but this is only true for connected servers (line 1401). For placeholder connections created for disabled servers or when MCP is globally disabled (line 1093 in createPlaceholderConnection), the config is stored without variable injection. This creates an asymmetry: the stored config for placeholders is validated but not injected, while the new config being compared is both validated and injected. This means disabled servers will be incorrectly detected as having changed configs on every settings re-read, causing unnecessary deleteConnection and connectToServer calls. To fix this, either: (1) also inject variables when storing placeholder connection configs, or (2) conditionally inject variables in the comparison only for connected connections.
|
Thanks! this was a very annoying problem! |
Context
MCP servers restart every time a settings file is re-read, even when the configuration hasn't actually changed. This causes disruptive reconnection loops during normal usage (e.g. switching windows, saving unrelated files).
Fixes #5781, #5791
Implementation
The root cause is an asymmetric comparison in
McpHub.updateServerConnections. The stored config (currentConnection.server.config) has been through both Zod validation (which applies defaults fortimeout,alwaysAllow,disabledTools,cwd,type) andinjectVariables(which resolves${env:...}and${workspaceFolder}placeholders). The incoming config from the settings file is raw — neither validated nor injected.Since a typical user config omits optional fields like
timeoutandalwaysAllow, the Zod defaults alone guarantee the two sides ofdeepEqualcan never match, triggeringdeleteConnection+connectToServeron every re-read.The fix applies
injectVariablesto the already-validated config before comparison, so both sides are in the same normalized form:This is a single-line-to-8-line change in
src/services/mcp/McpHub.ts.Screenshots
N/A — backend logic change, no UI impact.
How to Test
${workspaceFolder}in a path, or omit optional fields liketimeout)Get in Touch
@evanjacobson on the Kilo Code Discord