From b2b5e1ffa64166fe757cc6a7cf2e7f63c94bd466 Mon Sep 17 00:00:00 2001 From: MuriloFP Date: Fri, 7 Feb 2025 15:51:30 -0300 Subject: [PATCH 1/3] fix: Implement singleton pattern for MCP server management to prevent multiple instances Problem: - Multiple instances of the Roo Code application were launching separate MCP server instances - This led to unnecessary resource consumption and potential conflicts between instances Solution: 1. Created new McpServerManager singleton class to manage MCP server instances: - Static getInstance() method ensures only one McpHub instance exists - Tracks registered ClineProvider instances - Handles cleanup on extension deactivation 2. Modified ClineProvider class: - Changed mcpHub from private to protected - Added getMcpHub() public getter method - Updated initialization to use McpServerManager - Added unregister logic in dispose() 3. Updated extension.ts to handle cleanup: - Added McpServerManager cleanup in deactivate() Technical Implementation: - Uses WeakRef for provider tracking to allow proper garbage collection - Maintains global state to track instance IDs - Implements proper cleanup of resources on disposal - Ensures backward compatibility with existing code This change significantly improves resource usage and prevents potential conflicts between multiple instances of the application while maintaining all existing functionality. --- src/core/Cline.ts | 10 +++-- src/core/webview/ClineProvider.ts | 21 +++++++++- src/extension.ts | 9 ++++- src/services/mcp/McpServerManager.ts | 60 ++++++++++++++++++++++++++++ 4 files changed, 92 insertions(+), 8 deletions(-) create mode 100644 src/services/mcp/McpServerManager.ts diff --git a/src/core/Cline.ts b/src/core/Cline.ts index 2c36dd5495..0f2e796132 100644 --- a/src/core/Cline.ts +++ b/src/core/Cline.ts @@ -832,7 +832,7 @@ export class Cline { this.lastApiRequestTime = Date.now() if (mcpEnabled ?? true) { - mcpHub = this.providerRef.deref()?.mcpHub + mcpHub = this.providerRef.deref()?.getMcpHub() if (!mcpHub) { throw new Error("MCP hub not available") } @@ -1013,7 +1013,7 @@ export class Cline { // (have to do this for partial and complete since sending content in thinking tags to markdown renderer will automatically be removed) // Remove end substrings of (with optional line break after) and (with optional line break before) // - Needs to be separate since we dont want to remove the line break before the first tag // - Needs to happen before the xml parsing below @@ -2267,7 +2267,8 @@ export class Cline { await this.say("mcp_server_request_started") // same as browser_action_result const toolResult = await this.providerRef .deref() - ?.mcpHub?.callTool(server_name, tool_name, parsedArguments) + ?.getMcpHub() + ?.callTool(server_name, tool_name, parsedArguments) // TODO: add progress indicator and ability to parse images and non-text responses const toolResultPretty = @@ -2335,7 +2336,8 @@ export class Cline { await this.say("mcp_server_request_started") const resourceResult = await this.providerRef .deref() - ?.mcpHub?.readResource(server_name, uri) + ?.getMcpHub() + ?.readResource(server_name, uri) const resourceResultPretty = resourceResult?.contents .map((item) => { diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index 9326c83350..7b4d4b2047 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -36,6 +36,7 @@ import { EXPERIMENT_IDS, experiments as Experiments, experimentDefault, Experime import { CustomSupportPrompts, supportPrompt } from "../../shared/support-prompt" import { ACTION_NAMES } from "../CodeActionProvider" +import { McpServerManager } from "../../services/mcp/McpServerManager" /* https://github.com/microsoft/vscode-webview-ui-toolkit-samples/blob/main/default/weather-webview/src/providers/WeatherViewProvider.ts @@ -136,7 +137,7 @@ export class ClineProvider implements vscode.WebviewViewProvider { private isViewLaunched = false private cline?: Cline private workspaceTracker?: WorkspaceTracker - mcpHub?: McpHub + protected mcpHub?: McpHub // Change from private to protected private latestAnnouncementId = "jan-21-2025-custom-modes" // update to some unique identifier when we add a new announcement configManager: ConfigManager customModesManager: CustomModesManager @@ -148,11 +149,19 @@ export class ClineProvider implements vscode.WebviewViewProvider { this.outputChannel.appendLine("ClineProvider instantiated") ClineProvider.activeInstances.add(this) this.workspaceTracker = new WorkspaceTracker(this) - this.mcpHub = new McpHub(this) this.configManager = new ConfigManager(this.context) this.customModesManager = new CustomModesManager(this.context, async () => { await this.postStateToWebview() }) + + // Initialize MCP Hub through the singleton manager + McpServerManager.getInstance(this.context, this) + .then((hub) => { + this.mcpHub = hub + }) + .catch((error) => { + this.outputChannel.appendLine(`Failed to initialize MCP Hub: ${error}`) + }) } /* @@ -181,6 +190,9 @@ export class ClineProvider implements vscode.WebviewViewProvider { this.customModesManager?.dispose() this.outputChannel.appendLine("Disposed all disposables") ClineProvider.activeInstances.delete(this) + + // Unregister from McpServerManager + McpServerManager.unregisterProvider(this) } public static getVisibleInstance(): ClineProvider | undefined { @@ -2538,4 +2550,9 @@ export class ClineProvider implements vscode.WebviewViewProvider { get messages() { return this.cline?.clineMessages || [] } + + // Add public getter + public getMcpHub(): McpHub | undefined { + return this.mcpHub + } } diff --git a/src/extension.ts b/src/extension.ts index 8ca7f3312c..a05afa4651 100644 --- a/src/extension.ts +++ b/src/extension.ts @@ -6,6 +6,7 @@ import "./utils/path" // Necessary to have access to String.prototype.toPosix. import { CodeActionProvider } from "./core/CodeActionProvider" import { DIFF_VIEW_URI_SCHEME } from "./integrations/editor/DiffViewProvider" import { handleUri, registerCommands, registerCodeActions, registerTerminalActions } from "./activate" +import { McpServerManager } from "./services/mcp/McpServerManager" /** * Built using https://github.com/microsoft/vscode-webview-ui-toolkit @@ -16,10 +17,12 @@ import { handleUri, registerCommands, registerCodeActions, registerTerminalActio */ let outputChannel: vscode.OutputChannel +let extensionContext: vscode.ExtensionContext // This method is called when your extension is activated. // Your extension is activated the very first time the command is executed. export function activate(context: vscode.ExtensionContext) { + extensionContext = context outputChannel = vscode.window.createOutputChannel("Roo-Code") context.subscriptions.push(outputChannel) outputChannel.appendLine("Roo-Code extension activated") @@ -83,7 +86,9 @@ export function activate(context: vscode.ExtensionContext) { return createClineAPI(outputChannel, sidebarProvider) } -// This method is called when your extension is deactivated. -export function deactivate() { +// This method is called when your extension is deactivated +export async function deactivate() { outputChannel.appendLine("Roo-Code extension deactivated") + // Clean up MCP server manager + await McpServerManager.cleanup(extensionContext) } diff --git a/src/services/mcp/McpServerManager.ts b/src/services/mcp/McpServerManager.ts new file mode 100644 index 0000000000..20ed2b8322 --- /dev/null +++ b/src/services/mcp/McpServerManager.ts @@ -0,0 +1,60 @@ +import * as vscode from "vscode" +import { McpHub } from "./McpHub" +import { ClineProvider } from "../../core/webview/ClineProvider" + +/** + * Singleton manager for MCP server instances. + * Ensures only one set of MCP servers runs across all webviews. + */ +export class McpServerManager { + private static instance: McpHub | null = null + private static readonly GLOBAL_STATE_KEY = "mcpHubInstanceId" + private static providers: Set = new Set() + + /** + * Get the singleton McpHub instance. + * Creates a new instance if one doesn't exist. + */ + static async getInstance(context: vscode.ExtensionContext, provider: ClineProvider): Promise { + // Register the provider + this.providers.add(provider) + + if (!this.instance) { + this.instance = new McpHub(provider) + // Store a unique identifier in global state to track the primary instance + await context.globalState.update(this.GLOBAL_STATE_KEY, Date.now().toString()) + } + return this.instance + } + + /** + * Remove a provider from the tracked set. + * This is called when a webview is disposed. + */ + static unregisterProvider(provider: ClineProvider): void { + this.providers.delete(provider) + } + + /** + * Notify all registered providers of server state changes. + */ + static notifyProviders(message: any): void { + this.providers.forEach((provider) => { + provider.postMessageToWebview(message).catch((error) => { + console.error("Failed to notify provider:", error) + }) + }) + } + + /** + * Clean up the singleton instance and all its resources. + */ + static async cleanup(context: vscode.ExtensionContext): Promise { + if (this.instance) { + await this.instance.dispose() + this.instance = null + await context.globalState.update(this.GLOBAL_STATE_KEY, undefined) + } + this.providers.clear() + } +} From ef95562dfe5166b2c66c5c55a4fe264e12e57ffd Mon Sep 17 00:00:00 2001 From: MuriloFP Date: Fri, 7 Feb 2025 16:57:21 -0300 Subject: [PATCH 2/3] fix: ensure MCP server list appears in settings when server starts first When the MCP server was initialized before opening RooCode, the server list would not appear in the settings. This was fixed by: 1. Adding proper server list initialization in ClineProvider.ts when the webview launches, checking if mcpHub exists and sending its current servers to the webview: ```typescript if (this.mcpHub) { this.postMessageToWebview({ type: "mcpServers", mcpServers: this.mcpHub.getServers() }) } ``` 2. Using the public getServers() method from McpHub instead of relying on internal state updates, ensuring consistent server list state across the application. The fix maintains clean separation of concerns and follows existing patterns for state management between the extension and webview. --- src/core/webview/ClineProvider.ts | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index 7b4d4b2047..23346d945c 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -613,6 +613,15 @@ export class ClineProvider implements vscode.WebviewViewProvider { this.postMessageToWebview({ type: "openRouterModels", openRouterModels }) } }) + + // If MCP Hub is already initialized, update the webview with current server list + if (this.mcpHub) { + this.postMessageToWebview({ + type: "mcpServers", + mcpServers: this.mcpHub.getServers(), + }) + } + // gui relies on model info to be up-to-date to provide the most accurate pricing, so we need to fetch the latest details on launch. // we do this for all users since many users switch between api providers and if they were to switch back to openrouter it would be showing outdated model info if we hadn't retrieved the latest at this point // (see normalizeApiConfiguration > openrouter) @@ -2115,6 +2124,7 @@ export class ClineProvider implements vscode.WebviewViewProvider { autoApprovalEnabled: autoApprovalEnabled ?? false, customModes: await this.customModesManager.getCustomModes(), experiments: experiments ?? experimentDefault, + mcpServers: this.mcpHub?.getServers() ?? [], } } From c9be470319704782523d913a0b2444b687a17e95 Mon Sep 17 00:00:00 2001 From: MuriloFP Date: Fri, 7 Feb 2025 17:12:39 -0300 Subject: [PATCH 3/3] fix: prevent race conditions in McpServerManager singleton initialization Added thread-safe initialization to the McpServerManager singleton pattern to prevent potential race conditions when getInstance is called concurrently. The changes include: 1. Added initializationPromise to track ongoing initialization 2. Implemented double-checked locking pattern: - First check: Return existing instance if available - Second check: Wait for any ongoing initialization - Third check: Double-check inside initialization block 3. Added proper cleanup in finally block to prevent deadlocks This ensures that: - Only one McpHub instance is ever created - Concurrent calls wait for initialization to complete - Resources are properly initialized and cleaned up - No memory leaks from incomplete initialization The fix maintains the existing functionality while making it safe for concurrent access in VS Code's multi-window environment. --- src/services/mcp/McpServerManager.ts | 33 +++++++++++++++++++++++----- 1 file changed, 28 insertions(+), 5 deletions(-) diff --git a/src/services/mcp/McpServerManager.ts b/src/services/mcp/McpServerManager.ts index 20ed2b8322..e15f9db0a7 100644 --- a/src/services/mcp/McpServerManager.ts +++ b/src/services/mcp/McpServerManager.ts @@ -10,21 +10,44 @@ export class McpServerManager { private static instance: McpHub | null = null private static readonly GLOBAL_STATE_KEY = "mcpHubInstanceId" private static providers: Set = new Set() + private static initializationPromise: Promise | null = null /** * Get the singleton McpHub instance. * Creates a new instance if one doesn't exist. + * Thread-safe implementation using a promise-based lock. */ static async getInstance(context: vscode.ExtensionContext, provider: ClineProvider): Promise { // Register the provider this.providers.add(provider) - if (!this.instance) { - this.instance = new McpHub(provider) - // Store a unique identifier in global state to track the primary instance - await context.globalState.update(this.GLOBAL_STATE_KEY, Date.now().toString()) + // If we already have an instance, return it + if (this.instance) { + return this.instance } - return this.instance + + // If initialization is in progress, wait for it + if (this.initializationPromise) { + return this.initializationPromise + } + + // Create a new initialization promise + this.initializationPromise = (async () => { + try { + // Double-check instance in case it was created while we were waiting + if (!this.instance) { + this.instance = new McpHub(provider) + // Store a unique identifier in global state to track the primary instance + await context.globalState.update(this.GLOBAL_STATE_KEY, Date.now().toString()) + } + return this.instance + } finally { + // Clear the initialization promise after completion or error + this.initializationPromise = null + } + })() + + return this.initializationPromise } /**