mirror of
https://github.com/RooVetGit/Roo-Code.git
synced 2026-10-08 03:07:53 +00:00
fix: prevent MCP server restart when toggling tool permissions (#8633)
* 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. * 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. * 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.
This commit is contained in:
Seth Miller
GitHub
parent
82b0b04b59
commit
f839d4c27b
1 changed files with 101 additions and 6 deletions
|
|
@ -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 = {
|
||||
|
|
@ -151,6 +152,8 @@ export class McpHub {
|
|||
isConnecting: boolean = false
|
||||
private refCount: number = 0 // Reference counter for active clients
|
||||
private configChangeDebounceTimers: Map<string, NodeJS.Timeout> = new Map()
|
||||
private isProgrammaticUpdate: boolean = false
|
||||
private flagResetTimer?: NodeJS.Timeout
|
||||
|
||||
constructor(provider: ClineProvider) {
|
||||
this.providerRef = new WeakRef(provider)
|
||||
|
|
@ -278,6 +281,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
|
||||
|
|
@ -1369,13 +1377,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)
|
||||
|
|
@ -1397,6 +1408,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<z.infer<typeof ServerConfigSchema>> {
|
||||
// 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
|
||||
|
|
@ -1463,7 +1525,20 @@ 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
|
||||
if (this.flagResetTimer) {
|
||||
clearTimeout(this.flagResetTimer)
|
||||
}
|
||||
this.isProgrammaticUpdate = true
|
||||
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(
|
||||
|
|
@ -1541,7 +1616,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)
|
||||
|
|
@ -1686,7 +1761,20 @@ 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
|
||||
if (this.flagResetTimer) {
|
||||
clearTimeout(this.flagResetTimer)
|
||||
}
|
||||
this.isProgrammaticUpdate = true
|
||||
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)
|
||||
|
|
@ -1796,6 +1884,13 @@ export class McpHub {
|
|||
}
|
||||
this.configChangeDebounceTimers.clear()
|
||||
|
||||
// Clear flag reset timer and reset programmatic update flag
|
||||
if (this.flagResetTimer) {
|
||||
clearTimeout(this.flagResetTimer)
|
||||
this.flagResetTimer = undefined
|
||||
}
|
||||
this.isProgrammaticUpdate = false
|
||||
|
||||
this.removeAllFileWatchers()
|
||||
for (const connection of this.connections) {
|
||||
try {
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue