From 9db94d3bddc1e28e1b52fdd4f43f89a96e9256a1 Mon Sep 17 00:00:00 2001 From: heyseth Date: Sun, 12 Oct 2025 16:05:57 -0700 Subject: [PATCH 1/3] fix: prevent MCP server restart when toggling tool permissions 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. --- src/services/mcp/McpHub.ts | 29 +++++++++++++++++++++++++++-- 1 file changed, 27 insertions(+), 2 deletions(-) diff --git a/src/services/mcp/McpHub.ts b/src/services/mcp/McpHub.ts index caca5ddb392..54dc45107c2 100644 --- a/src/services/mcp/McpHub.ts +++ b/src/services/mcp/McpHub.ts @@ -151,6 +151,7 @@ export class McpHub { isConnecting: boolean = false private refCount: number = 0 // Reference counter for active clients private configChangeDebounceTimers: Map = new Map() + private isProgrammaticUpdate: boolean = false constructor(provider: ClineProvider) { this.providerRef = new WeakRef(provider) @@ -278,6 +279,11 @@ export class McpHub { * Debounced wrapper for handling config file changes */ private debounceConfigChange(filePath: string, source: "global" | "project"): void { + // Skip processing if this is a programmatic update to prevent unnecessary server restarts + if (this.isProgrammaticUpdate) { + return + } + const key = `${source}-${filePath}` // Clear existing timer if any @@ -1463,7 +1469,15 @@ export class McpHub { mcpServers: config.mcpServers, } - await fs.writeFile(configPath, JSON.stringify(updatedConfig, null, 2)) + // Set flag to prevent file watcher from triggering server restart + this.isProgrammaticUpdate = true + try { + await fs.writeFile(configPath, JSON.stringify(updatedConfig, null, 2)) + // Delay longer than debounce timer (500ms) to ensure watcher event is processed while flag is set + await delay(600) + } finally { + this.isProgrammaticUpdate = false + } } public async updateServerTimeout( @@ -1686,7 +1700,15 @@ export class McpHub { targetList.splice(toolIndex, 1) } - await fs.writeFile(normalizedPath, JSON.stringify(config, null, 2)) + // Set flag to prevent file watcher from triggering server restart + this.isProgrammaticUpdate = true + try { + await fs.writeFile(normalizedPath, JSON.stringify(config, null, 2)) + // Delay longer than debounce timer (500ms) to ensure watcher event is processed while flag is set + await delay(600) + } finally { + this.isProgrammaticUpdate = false + } if (connection) { connection.server.tools = await this.fetchToolsList(serverName, source) @@ -1796,6 +1818,9 @@ export class McpHub { } this.configChangeDebounceTimers.clear() + // Reset programmatic update flag + this.isProgrammaticUpdate = false + this.removeAllFileWatchers() for (const connection of this.connections) { try { From 6587316a926de517f9004e8dd9e993843a060d3c Mon Sep 17 00:00:00 2001 From: heyseth Date: Sun, 12 Oct 2025 17:26:28 -0700 Subject: [PATCH 2/3] fix(mcp): prevent server reconnection when toggling disabled state 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. --- src/services/mcp/McpHub.ts | 85 +++++++++++++++++++++++++++++++------- 1 file changed, 69 insertions(+), 16 deletions(-) diff --git a/src/services/mcp/McpHub.ts b/src/services/mcp/McpHub.ts index 54dc45107c2..3dd06bc4488 100644 --- a/src/services/mcp/McpHub.ts +++ b/src/services/mcp/McpHub.ts @@ -32,6 +32,7 @@ import { import { fileExistsAtPath } from "../../utils/fs" import { arePathsEqual, getWorkspacePath } from "../../utils/path" import { injectVariables } from "../../utils/config" +import { safeWriteJson } from "../../utils/safeWriteJson" // Discriminated union for connection states export type ConnectedMcpConnection = { @@ -1375,13 +1376,16 @@ export class McpHub { this.removeFileWatchersForServer(serverName) await this.deleteConnection(serverName, serverSource) // Re-add as a disabled connection - await this.connectToServer(serverName, JSON.parse(connection.server.config), serverSource) + // Re-read config from file to get updated disabled state + const updatedConfig = await this.readServerConfigFromFile(serverName, serverSource) + await this.connectToServer(serverName, updatedConfig, serverSource) } else if (!disabled && connection.server.status === "disconnected") { // If enabling a disabled server, connect it - const config = JSON.parse(connection.server.config) + // Re-read config from file to get updated disabled state + const updatedConfig = await this.readServerConfigFromFile(serverName, serverSource) await this.deleteConnection(serverName, serverSource) // When re-enabling, file watchers will be set up in connectToServer - await this.connectToServer(serverName, config, serverSource) + await this.connectToServer(serverName, updatedConfig, serverSource) } else if (connection.server.status === "connected") { // Only refresh capabilities if connected connection.server.tools = await this.fetchToolsList(serverName, serverSource) @@ -1403,6 +1407,57 @@ export class McpHub { } } + /** + * Helper method to read a server's configuration from the appropriate settings file + * @param serverName The name of the server to read + * @param source Whether to read from the global or project config + * @returns The validated server configuration + */ + private async readServerConfigFromFile( + serverName: string, + source: "global" | "project" = "global", + ): Promise> { + // Determine which config file to read + let configPath: string + if (source === "project") { + const projectMcpPath = await this.getProjectMcpPath() + if (!projectMcpPath) { + throw new Error("Project MCP configuration file not found") + } + configPath = projectMcpPath + } else { + configPath = await this.getMcpSettingsFilePath() + } + + // Ensure the settings file exists and is accessible + try { + await fs.access(configPath) + } catch (error) { + console.error("Settings file not accessible:", error) + throw new Error("Settings file not accessible") + } + + // Read and parse the config file + const content = await fs.readFile(configPath, "utf-8") + const config = JSON.parse(content) + + // Validate the config structure + if (!config || typeof config !== "object") { + throw new Error("Invalid config structure") + } + + if (!config.mcpServers || typeof config.mcpServers !== "object") { + throw new Error("No mcpServers section in config") + } + + if (!config.mcpServers[serverName]) { + throw new Error(`Server ${serverName} not found in config`) + } + + // Validate and return the server config + return this.validateServerConfig(config.mcpServers[serverName], serverName) + } + /** * Helper method to update a server's configuration in the appropriate settings file * @param serverName The name of the server to update @@ -1471,13 +1526,12 @@ export class McpHub { // Set flag to prevent file watcher from triggering server restart this.isProgrammaticUpdate = true - try { - await fs.writeFile(configPath, JSON.stringify(updatedConfig, null, 2)) - // Delay longer than debounce timer (500ms) to ensure watcher event is processed while flag is set - await delay(600) - } finally { + await safeWriteJson(configPath, updatedConfig) + + // Reset flag after watcher debounce period (non-blocking) + delay(600).then(() => { this.isProgrammaticUpdate = false - } + }) } public async updateServerTimeout( @@ -1555,7 +1609,7 @@ export class McpHub { mcpServers: config.mcpServers, } - await fs.writeFile(configPath, JSON.stringify(updatedConfig, null, 2)) + await safeWriteJson(configPath, updatedConfig) // Update server connections with the correct source await this.updateServerConnections(config.mcpServers, serverSource) @@ -1702,13 +1756,12 @@ export class McpHub { // Set flag to prevent file watcher from triggering server restart this.isProgrammaticUpdate = true - try { - await fs.writeFile(normalizedPath, JSON.stringify(config, null, 2)) - // Delay longer than debounce timer (500ms) to ensure watcher event is processed while flag is set - await delay(600) - } finally { + await safeWriteJson(normalizedPath, config) + + // Reset flag after watcher debounce period (non-blocking) + delay(600).then(() => { this.isProgrammaticUpdate = false - } + }) if (connection) { connection.server.tools = await this.fetchToolsList(serverName, source) From a2e795975ade7f243d4392d3acbb651008fab136 Mon Sep 17 00:00:00 2001 From: heyseth Date: Sun, 12 Oct 2025 17:53:48 -0700 Subject: [PATCH 3/3] fix(mcp): prevent race condition in isProgrammaticUpdate flag 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. --- src/services/mcp/McpHub.ts | 43 ++++++++++++++++++++++++++------------ 1 file changed, 30 insertions(+), 13 deletions(-) diff --git a/src/services/mcp/McpHub.ts b/src/services/mcp/McpHub.ts index 3dd06bc4488..f5ddf3e57b8 100644 --- a/src/services/mcp/McpHub.ts +++ b/src/services/mcp/McpHub.ts @@ -153,6 +153,7 @@ export class McpHub { private refCount: number = 0 // Reference counter for active clients private configChangeDebounceTimers: Map = new Map() private isProgrammaticUpdate: boolean = false + private flagResetTimer?: NodeJS.Timeout constructor(provider: ClineProvider) { this.providerRef = new WeakRef(provider) @@ -1525,13 +1526,19 @@ export class McpHub { } // Set flag to prevent file watcher from triggering server restart + if (this.flagResetTimer) { + clearTimeout(this.flagResetTimer) + } this.isProgrammaticUpdate = true - await safeWriteJson(configPath, updatedConfig) - - // Reset flag after watcher debounce period (non-blocking) - delay(600).then(() => { - this.isProgrammaticUpdate = false - }) + try { + await safeWriteJson(configPath, updatedConfig) + } finally { + // Reset flag after watcher debounce period (non-blocking) + this.flagResetTimer = setTimeout(() => { + this.isProgrammaticUpdate = false + this.flagResetTimer = undefined + }, 600) + } } public async updateServerTimeout( @@ -1755,13 +1762,19 @@ export class McpHub { } // Set flag to prevent file watcher from triggering server restart + if (this.flagResetTimer) { + clearTimeout(this.flagResetTimer) + } this.isProgrammaticUpdate = true - await safeWriteJson(normalizedPath, config) - - // Reset flag after watcher debounce period (non-blocking) - delay(600).then(() => { - this.isProgrammaticUpdate = false - }) + try { + await safeWriteJson(normalizedPath, config) + } finally { + // Reset flag after watcher debounce period (non-blocking) + this.flagResetTimer = setTimeout(() => { + this.isProgrammaticUpdate = false + this.flagResetTimer = undefined + }, 600) + } if (connection) { connection.server.tools = await this.fetchToolsList(serverName, source) @@ -1871,7 +1884,11 @@ export class McpHub { } this.configChangeDebounceTimers.clear() - // Reset programmatic update flag + // Clear flag reset timer and reset programmatic update flag + if (this.flagResetTimer) { + clearTimeout(this.flagResetTimer) + this.flagResetTimer = undefined + } this.isProgrammaticUpdate = false this.removeAllFileWatchers()