diff --git a/.changeset/fix-mcp-restart-loop.md b/.changeset/fix-mcp-restart-loop.md new file mode 100644 index 00000000000..edd2d16fb9b --- /dev/null +++ b/.changeset/fix-mcp-restart-loop.md @@ -0,0 +1,5 @@ +--- +"kilo-code": patch +--- + +Fix MCP servers restarting repeatedly when saving settings diff --git a/src/services/mcp/McpHub.ts b/src/services/mcp/McpHub.ts index e0290ee8caa..ec2e65a294c 100644 --- a/src/services/mcp/McpHub.ts +++ b/src/services/mcp/McpHub.ts @@ -47,6 +47,7 @@ import { McpOAuthService, OAuthTokens } from "./oauth" // Discriminated union for connection states export type ConnectedMcpConnection = { type: "connected" + rawConfig: string // kilocode_change: pre-injection validated config for accurate deep-equal comparison server: McpServer client: Client transport: StdioClientTransport | SSEClientTransport | StreamableHTTPClientTransport @@ -54,6 +55,7 @@ export type ConnectedMcpConnection = { export type DisconnectedMcpConnection = { type: "disconnected" + rawConfig: string // kilocode_change: pre-injection validated config for accurate deep-equal comparison server: McpServer client: null transport: null @@ -188,6 +190,9 @@ export class McpHub { // kilocode_change start - MCP OAuth Authorization private oauthService?: McpOAuthService // kilocode_change end + // kilocode_change start - Intentional disconnect guard for auto-reconnect + private intentionalDisconnects: Set = new Set() + // kilocode_change end // kilocode_change start - Auto-reconnect on disconnect private reconnectAttempts: Map = new Map() private reconnectTimers: Map = new Map() @@ -587,6 +592,19 @@ export class McpHub { this.reconnectTimers.set(key, timer) } + /** + * Attempts to schedule a reconnect only if the disconnect was not intentional. + * This prevents auto-reconnect from firing when we programmatically close connections. + * @param serverName The name of the server + * @param source The server source (global or project) + */ + private scheduleReconnectIfNotIntentional(serverName: string, source: "global" | "project"): void { + const intentionalKey = `${source}-${serverName}` + if (!this.intentionalDisconnects.has(intentionalKey)) { + this.scheduleReconnect(serverName, source) + } + } + /** * Cancels any scheduled reconnect for a server. * @param serverName The name of the server @@ -747,6 +765,11 @@ export class McpHub { // Set new timer const timer = setTimeout(async () => { this.configChangeDebounceTimers.delete(key) + // kilocode_change: Re-check flag inside callback - it may have been set + // after the initial check but before the debounce timer fires + if (this.isProgrammaticUpdate) { + return + } await this.handleConfigFileChange(filePath, source) }, 500) // 500ms debounce @@ -1088,6 +1111,7 @@ export class McpHub { ): DisconnectedMcpConnection { return { type: "disconnected", + rawConfig: JSON.stringify(config), // kilocode_change: store pre-injection config server: { name, config: JSON.stringify(config), @@ -1203,8 +1227,8 @@ export class McpHub { this.appendErrorMessage(connection, error instanceof Error ? error.message : `${error}`) } await this.notifyWebviewOfServerChanges() - // kilocode_change - Schedule auto-reconnect on error - this.scheduleReconnect(name, source) + // kilocode_change - Schedule auto-reconnect on error (skip if intentional disconnect) + this.scheduleReconnectIfNotIntentional(name, source) } transport.onclose = async () => { @@ -1213,8 +1237,8 @@ export class McpHub { connection.server.status = "disconnected" } await this.notifyWebviewOfServerChanges() - // kilocode_change - Schedule auto-reconnect on close - this.scheduleReconnect(name, source) + // kilocode_change - Schedule auto-reconnect on close (skip if intentional disconnect) + this.scheduleReconnectIfNotIntentional(name, source) } // transport.stderr is only available after the process has been started. However we can't start it separately from the .connect() call because it also starts the transport. And we can't place this after the connect call since we need to capture the stderr stream before the connection is established, in order to capture errors during the connection process. @@ -1274,8 +1298,8 @@ export class McpHub { this.appendErrorMessage(connection, error instanceof Error ? error.message : `${error}`) } await this.notifyWebviewOfServerChanges() - // kilocode_change - Schedule auto-reconnect on error - this.scheduleReconnect(name, source) + // kilocode_change - Schedule auto-reconnect on error (skip if intentional disconnect) + this.scheduleReconnectIfNotIntentional(name, source) } transport.onclose = async () => { @@ -1284,8 +1308,8 @@ export class McpHub { connection.server.status = "disconnected" } await this.notifyWebviewOfServerChanges() - // kilocode_change - Schedule auto-reconnect on close - this.scheduleReconnect(name, source) + // kilocode_change - Schedule auto-reconnect on close (skip if intentional disconnect) + this.scheduleReconnectIfNotIntentional(name, source) } } else if (configInjected.type === "sse") { // SSE connection @@ -1336,8 +1360,8 @@ export class McpHub { this.appendErrorMessage(connection, error instanceof Error ? error.message : `${error}`) } await this.notifyWebviewOfServerChanges() - // kilocode_change - Schedule auto-reconnect on error - this.scheduleReconnect(name, source) + // kilocode_change - Schedule auto-reconnect on error (skip if intentional disconnect) + this.scheduleReconnectIfNotIntentional(name, source) } transport.onclose = async () => { @@ -1346,8 +1370,8 @@ export class McpHub { connection.server.status = "disconnected" } await this.notifyWebviewOfServerChanges() - // kilocode_change - Schedule auto-reconnect on close - this.scheduleReconnect(name, source) + // kilocode_change - Schedule auto-reconnect on close (skip if intentional disconnect) + this.scheduleReconnectIfNotIntentional(name, source) } } else { // Should not happen if validateServerConfig is correct @@ -1396,6 +1420,7 @@ export class McpHub { // Create a connected connection const connection: ConnectedMcpConnection = { type: "connected", + rawConfig: JSON.stringify(config), // kilocode_change: store pre-injection config for accurate comparison server: { name, config: JSON.stringify(configInjected), @@ -1420,6 +1445,9 @@ export class McpHub { connection.server.instructions = client.getInstructions() // kilocode_change - Reset reconnect attempts on successful connection this.resetReconnectAttempts(name, source) + // kilocode_change - Clear intentional disconnect flag on successful connection + const intentionalKey = `${source}-${name}` + this.intentionalDisconnects.delete(intentionalKey) this.kiloNotificationService.connect(name, connection.client) @@ -1692,6 +1720,10 @@ export class McpHub { for (const connection of connections) { try { if (connection.type === "connected") { + // kilocode_change: Mark as intentional disconnect to prevent onclose from scheduling auto-reconnect + const connSource = connection.server.source || "global" + const intentionalKey = `${connSource}-${name}` + this.intentionalDisconnects.add(intentionalKey) // kilocode_change start // Fire-and-forget: don't await close() calls as they can block // waiting for the subprocess to exit. The MCP SDK's transport.close() @@ -1774,7 +1806,8 @@ export class McpHub { } catch (error) { this.showErrorMessage(`Failed to connect to new MCP server ${name}`, error) } - } else if (!deepEqual(JSON.parse(currentConnection.server.config), config)) { + } else if (!deepEqual(JSON.parse(currentConnection.rawConfig), validatedConfig)) { + // kilocode_change: compare raw (pre-injection) configs // Existing server with changed config try { // Only setup file watcher for enabled servers @@ -2589,6 +2622,7 @@ export class McpHub { } this.reconnectTimers.clear() this.reconnectAttempts.clear() + this.intentionalDisconnects.clear() // kilocode_change end this.removeAllFileWatchers() diff --git a/src/services/mcp/__tests__/McpHub.spec.ts b/src/services/mcp/__tests__/McpHub.spec.ts index a79b1b78d87..2ed5250a6e6 100644 --- a/src/services/mcp/__tests__/McpHub.spec.ts +++ b/src/services/mcp/__tests__/McpHub.spec.ts @@ -307,6 +307,7 @@ describe("McpHub", () => { // Directly set up a connected connection const connectedConnection: ConnectedMcpConnection = { type: "connected", + rawConfig: "{}", server: { name: "test-server", config: JSON.stringify({ command: "node", args: ["test.js"] }), @@ -331,6 +332,7 @@ describe("McpHub", () => { // Now test with a disconnected connection const disconnectedConnection: DisconnectedMcpConnection = { type: "disconnected", + rawConfig: "{}", server: { name: "disabled-server", config: JSON.stringify({ command: "node", args: ["test.js"], disabled: true }), @@ -797,6 +799,7 @@ describe("McpHub", () => { // Set up mock connection without alwaysAllow const mockConnection: ConnectedMcpConnection = { type: "connected", + rawConfig: "{}", server: { name: "test-server", type: "stdio", @@ -846,6 +849,7 @@ describe("McpHub", () => { // Set up mock connection const mockConnection: ConnectedMcpConnection = { type: "connected", + rawConfig: "{}", server: { name: "test-server", type: "stdio", @@ -895,6 +899,7 @@ describe("McpHub", () => { // Set up mock connection const mockConnection: ConnectedMcpConnection = { type: "connected", + rawConfig: "{}", server: { name: "test-server", type: "stdio", @@ -941,6 +946,7 @@ describe("McpHub", () => { // Set up mock connection const mockConnection: ConnectedMcpConnection = { type: "connected", + rawConfig: "{}", server: { name: "test-server", config: "test-server-config", @@ -989,6 +995,7 @@ describe("McpHub", () => { // Set up mock connection const mockConnection: ConnectedMcpConnection = { type: "connected", + rawConfig: "{}", server: { name: "test-server", config: "test-server-config", @@ -1036,6 +1043,7 @@ describe("McpHub", () => { // Set up mock connection const mockConnection: ConnectedMcpConnection = { type: "connected", + rawConfig: "{}", server: { name: "test-server", config: "test-server-config", @@ -1087,6 +1095,7 @@ describe("McpHub", () => { // Set up mock connection const mockConnection: ConnectedMcpConnection = { type: "connected", + rawConfig: "{}", server: { name: "test-server", type: "stdio", @@ -1119,6 +1128,7 @@ describe("McpHub", () => { const mockConnections: McpConnection[] = [ { type: "connected", + rawConfig: "{}", server: { name: "enabled-server", config: "{}", @@ -1130,6 +1140,7 @@ describe("McpHub", () => { } as ConnectedMcpConnection, { type: "disconnected", + rawConfig: "{}", server: { name: "disabled-server", config: "{}", @@ -1152,6 +1163,7 @@ describe("McpHub", () => { const mockConnections: McpConnection[] = [ { type: "connected", + rawConfig: "{}", server: { name: "shared-server", config: '{"source":"global"}', @@ -1164,6 +1176,7 @@ describe("McpHub", () => { } as ConnectedMcpConnection, { type: "connected", + rawConfig: "{}", server: { name: "shared-server", config: '{"source":"project"}', @@ -1176,6 +1189,7 @@ describe("McpHub", () => { } as ConnectedMcpConnection, { type: "connected", + rawConfig: "{}", server: { name: "unique-global-server", config: "{}", @@ -1209,6 +1223,7 @@ describe("McpHub", () => { const mockConnections: McpConnection[] = [ { type: "connected", + rawConfig: "{}", server: { name: "global-only-server", config: "{}", @@ -1297,6 +1312,7 @@ describe("McpHub", () => { // Mock the connection with a minimal client implementation const mockConnection: ConnectedMcpConnection = { type: "connected", + rawConfig: "{}", server: { name: "test-server", config: JSON.stringify({}), @@ -1361,6 +1377,7 @@ describe("McpHub", () => { it("should use default timeout of 60 seconds if not specified", async () => { const mockConnection: ConnectedMcpConnection = { type: "connected", + rawConfig: "{}", server: { name: "test-server", config: JSON.stringify({ type: "stdio", command: "test" }), // No timeout specified @@ -1385,6 +1402,7 @@ describe("McpHub", () => { it("should apply configured timeout to tool calls", async () => { const mockConnection: ConnectedMcpConnection = { type: "connected", + rawConfig: "{}", server: { name: "test-server", config: JSON.stringify({ type: "stdio", command: "test", timeout: 120 }), // 2 minutes @@ -1426,6 +1444,7 @@ describe("McpHub", () => { // Set up mock connection const mockConnection: ConnectedMcpConnection = { type: "connected", + rawConfig: "{}", server: { name: "test-server", type: "stdio", @@ -1472,6 +1491,7 @@ describe("McpHub", () => { // Set up mock connection before updating const mockConnectionInitial: ConnectedMcpConnection = { type: "connected", + rawConfig: "{}", server: { name: "test-server", type: "stdio", @@ -1496,6 +1516,7 @@ describe("McpHub", () => { // Setup connection with invalid timeout const mockConnectionInvalid: ConnectedMcpConnection = { type: "connected", + rawConfig: "{}", server: { name: "test-server", config: JSON.stringify({ @@ -1542,6 +1563,7 @@ describe("McpHub", () => { // Set up mock connection const mockConnection: ConnectedMcpConnection = { type: "connected", + rawConfig: "{}", server: { name: "test-server", type: "stdio", @@ -1582,6 +1604,7 @@ describe("McpHub", () => { // Set up mock connection const mockConnection: ConnectedMcpConnection = { type: "connected", + rawConfig: "{}", server: { name: "test-server", type: "stdio",