Skip to content

fix: prevent MCP server restart when toggling tool permissions #8233

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

fix: prevent MCP server restart when toggling tool permissions #8233

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 40 additions & 2 deletions src/services/mcp/McpHub.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,7 @@ 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 // Flag to track programmatic config updates
Copy link
Contributor Author

@roomote roomote bot Sep 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing test coverage: Consider adding specific test cases for the isProgrammaticUpdate flag behavior to ensure file watchers are properly skipped during programmatic updates. This would help catch any regression in this critical functionality.


constructor(provider: ClineProvider) {
this.providerRef = new WeakRef(provider)
Expand Down Expand Up @@ -278,6 +279,11 @@ export class McpHub {
* Debounced wrapper for handling config file changes
*/
private debounceConfigChange(filePath: string, source: "global" | "project"): void {
// Skip if this is a programmatic update
if (this.isProgrammaticUpdate) {
Copy link
Contributor Author

@roomote roomote bot Sep 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Potential race condition: There's a risk that file system watcher events could fire between setting/unsetting the isProgrammaticUpdate flag and the actual file operations. Consider using a more robust synchronization mechanism, such as:

  • A promise-based queue for file operations
  • A unique operation ID system to track programmatic updates
  • Or increasing the debounce timeout in debounceConfigChange to be longer than the 100ms wait

return
}

const key = `${source}-${filePath}`

// Clear existing timer if any
Expand Down Expand Up @@ -1463,7 +1469,28 @@ export class McpHub {
mcpServers: config.mcpServers,
}

await fs.writeFile(configPath, JSON.stringify(updatedConfig, null, 2))
// Check if this is a minor update that shouldn't trigger a restart
const isMinorUpdate = Object.keys(configUpdate).every(
(key) => key === "alwaysAllow" || key === "disabledTools" || key === "timeout",
)

if (isMinorUpdate) {
// Set flag to indicate programmatic update
this.isProgrammaticUpdate = true
Copy link
Contributor Author

@roomote roomote bot Sep 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code duplication: This pattern of setting flag → writing → waiting → resetting flag is duplicated in updateServerToolList. Consider extracting into a helper method:

private async writeConfigWithoutTriggeringWatcher(
  configPath: string,
  config: any
): Promise<void> {
  this.isProgrammaticUpdate = true;
  try {
    await fs.writeFile(configPath, JSON.stringify(config, null, 2));
    await new Promise((resolve) => setTimeout(resolve, 100));
  } finally {
    this.isProgrammaticUpdate = false;
  }
}


try {
await fs.writeFile(configPath, JSON.stringify(updatedConfig, null, 2))

// Give file system watchers a moment to process
await new Promise((resolve) => setTimeout(resolve, 100))
Copy link
Contributor Author

@roomote roomote bot Sep 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hardcoded timeout concern: The 100ms timeout might not be sufficient on slower systems or under heavy I/O load. Consider:

  • Making this configurable via settings
  • Using file system events to confirm write completion
  • Or implementing exponential backoff if issues persist

} finally {
// Always reset the flag
this.isProgrammaticUpdate = false
}
} else {
// For major updates, allow normal file watcher behavior
await fs.writeFile(configPath, JSON.stringify(updatedConfig, null, 2))
}
}

public async updateServerTimeout(
Expand Down Expand Up @@ -1686,7 +1713,18 @@ export class McpHub {
targetList.splice(toolIndex, 1)
}

await fs.writeFile(normalizedPath, JSON.stringify(config, null, 2))
// Set flag to indicate programmatic update
this.isProgrammaticUpdate = true

try {
await fs.writeFile(normalizedPath, JSON.stringify(config, null, 2))

// Give file system watchers a moment to process
await new Promise((resolve) => setTimeout(resolve, 100))
} finally {
// Always reset the flag
this.isProgrammaticUpdate = false
}

if (connection) {
connection.server.tools = await this.fetchToolsList(serverName, source)
Expand Down
Loading