From 774ae220bea45917f2890b2c1b6fd186eb47459f Mon Sep 17 00:00:00 2001 From: Himanshu Soni Date: Tue, 24 Feb 2026 00:05:31 +0530 Subject: [PATCH] fix(core): prevent state corruption in McpClientManager during collis (#19782) --- .../core/src/tools/mcp-client-manager.test.ts | 30 +++++++++++++++++++ packages/core/src/tools/mcp-client-manager.ts | 25 +++++++++------- 2 files changed, 44 insertions(+), 11 deletions(-) diff --git a/packages/core/src/tools/mcp-client-manager.test.ts b/packages/core/src/tools/mcp-client-manager.test.ts index 352c2c12f0..6592b01f01 100644 --- a/packages/core/src/tools/mcp-client-manager.test.ts +++ b/packages/core/src/tools/mcp-client-manager.test.ts @@ -381,6 +381,36 @@ describe('McpClientManager', () => { expect(manager.getMcpServers()).not.toHaveProperty('test-server'); }); + it('should ignore an extension attempting to register a server with an existing name', async () => { + const manager = new McpClientManager('0.0.1', toolRegistry, mockConfig); + const userConfig = { command: 'node', args: ['user-server.js'] }; + + mockConfig.getMcpServers.mockReturnValue({ + 'test-server': userConfig, + }); + mockedMcpClient.getServerConfig.mockReturnValue(userConfig); + + await manager.startConfiguredMcpServers(); + expect(mockedMcpClient.connect).toHaveBeenCalledTimes(1); + + const extension: GeminiCLIExtension = { + name: 'test-extension', + mcpServers: { + 'test-server': { command: 'node', args: ['ext-server.js'] }, + }, + isActive: true, + version: '1.0.0', + path: '/some-path', + contextFiles: [], + id: '123', + }; + + await manager.startExtension(extension); + + expect(mockedMcpClient.disconnect).not.toHaveBeenCalled(); + expect(mockedMcpClient.connect).toHaveBeenCalledTimes(1); + }); + it('should remove servers from blockedMcpServers when stopExtension is called', async () => { mockConfig.getBlockedMcpServers.mockReturnValue(['blocked-server']); const manager = new McpClientManager('0.0.1', toolRegistry, mockConfig); diff --git a/packages/core/src/tools/mcp-client-manager.ts b/packages/core/src/tools/mcp-client-manager.ts index a6783788e8..f9e377f038 100644 --- a/packages/core/src/tools/mcp-client-manager.ts +++ b/packages/core/src/tools/mcp-client-manager.ts @@ -176,6 +176,20 @@ export class McpClientManager { name: string, config: MCPServerConfig, ): Promise { + const existing = this.clients.get(name); + if ( + existing && + existing.getServerConfig().extension?.id !== config.extension?.id + ) { + const extensionText = config.extension + ? ` from extension "${config.extension.name}"` + : ''; + debugLogger.warn( + `Skipping MCP config for server with name "${name}"${extensionText} as it already exists.`, + ); + return; + } + // Always track server config for UI display this.allServerConfigs.set(name, config); @@ -191,7 +205,6 @@ export class McpClientManager { } // User-disabled servers: disconnect if running, don't start if (await this.isDisabledByUser(name)) { - const existing = this.clients.get(name); if (existing) { await this.disconnectClient(name); } @@ -203,16 +216,6 @@ export class McpClientManager { if (config.extension && !config.extension.isActive) { return; } - const existing = this.clients.get(name); - if (existing && existing.getServerConfig().extension !== config.extension) { - const extensionText = config.extension - ? ` from extension "${config.extension.name}"` - : ''; - debugLogger.warn( - `Skipping MCP config for server with name "${name}"${extensionText} as it already exists.`, - ); - return; - } const currentDiscoveryPromise = new Promise((resolve, reject) => { (async () => {