From 311161662041c7dda1b3da7c675fe2a35dc0d54a Mon Sep 17 00:00:00 2001 From: sahilempire Date: Thu, 16 Jul 2026 01:34:54 +0530 Subject: [PATCH] fix(core): shorten MCP tools/list discovery timeout so it fails fast The tools/list discovery request reused MCP_DEFAULT_TIMEOUT_MSEC (10 minutes), the default meant for long-running tool invocations. It was the only discovery call that passed that 10-minute default: when a server answers tools/list with a mismatched JSON-RPC id, the SDK ignores the response per spec, so the request stays pending for the full 10 minutes with no visible feedback, freezing startup discovery. Add MCP_DISCOVERY_TIMEOUT_MSEC (10s) and apply it as the default for the tools/list discovery request. The default is forwarded on every discovery call, including the tool-refresh path that also passes an abort signal, and an explicit per-server timeout still takes precedence. A server whose tools/list has not answered within the timeout is skipped for that session, and the failure is surfaced through the existing MCP diagnostic path within 10s instead of 10 minutes. Scope is limited to the tools/list discovery request, which is the one call that used the 10-minute default. prompts/list and resources/list already use the SDK default (60s), and the connection handshake and tool-invocation timeouts are intentionally unchanged. Fixes #28355 --- packages/core/src/tools/mcp-client.test.ts | 127 ++++++++++++++++++++- packages/core/src/tools/mcp-client.ts | 16 ++- 2 files changed, 138 insertions(+), 5 deletions(-) diff --git a/packages/core/src/tools/mcp-client.test.ts b/packages/core/src/tools/mcp-client.test.ts index 4d682c52c2b..b701aeefa49 100644 --- a/packages/core/src/tools/mcp-client.test.ts +++ b/packages/core/src/tools/mcp-client.test.ts @@ -190,7 +190,129 @@ describe('mcp-client', () => { }); expect(mockedClient.listTools).toHaveBeenCalledWith( {}, - expect.objectContaining({ timeout: 600000, progressReporter: client }), + expect.objectContaining({ timeout: 10000, progressReporter: client }), + ); + }); + + it('should use a short discovery timeout for tools/list when no per-server timeout is configured', async () => { + const mockedClient = { + connect: vi.fn(), + getStatus: vi.fn(), + registerCapabilities: vi.fn(), + setRequestHandler: vi.fn(), + setNotificationHandler: vi.fn(), + getServerCapabilities: vi.fn().mockReturnValue({ tools: {} }), + listTools: vi.fn().mockResolvedValue({ + tools: [ + { + name: 'testFunction', + inputSchema: { type: 'object', properties: {} }, + }, + ], + }), + listPrompts: vi.fn().mockResolvedValue({ prompts: [] }), + request: vi.fn().mockResolvedValue({}), + }; + vi.mocked(ClientLib.Client).mockReturnValue( + mockedClient as unknown as ClientLib.Client, + ); + vi.spyOn(SdkClientStdioLib, 'StdioClientTransport').mockReturnValue( + {} as SdkClientStdioLib.StdioClientTransport, + ); + const mockedToolRegistry = { + registerTool: vi.fn(), + sortTools: vi.fn(), + getToolsByServer: vi.fn().mockReturnValue([]), + getMessageBus: vi.fn().mockReturnValue(undefined), + } as unknown as ToolRegistry; + const promptRegistry = { + registerPrompt: vi.fn(), + getPromptsByServer: vi.fn().mockReturnValue([]), + removePromptsByServer: vi.fn(), + } as unknown as PromptRegistry; + const resourceRegistry = { + getResourcesByServer: vi.fn().mockReturnValue([]), + setResourcesForServer: vi.fn(), + removeResourcesByServer: vi.fn(), + } as unknown as ResourceRegistry; + const client = new McpClient( + 'test-server', + { command: 'test-command' }, + workspaceContext, + MOCK_CONTEXT, + false, + '0.0.1', + ); + await client.connect(); + await client.discoverInto(MOCK_CONTEXT, { + toolRegistry: mockedToolRegistry, + promptRegistry, + resourceRegistry, + }); + expect(mockedClient.listTools).toHaveBeenCalledWith( + {}, + expect.objectContaining({ timeout: 10000 }), + ); + }); + + it('should honor an explicit per-server timeout for tools/list discovery', async () => { + const mockedClient = { + connect: vi.fn(), + getStatus: vi.fn(), + registerCapabilities: vi.fn(), + setRequestHandler: vi.fn(), + setNotificationHandler: vi.fn(), + getServerCapabilities: vi.fn().mockReturnValue({ tools: {} }), + listTools: vi.fn().mockResolvedValue({ + tools: [ + { + name: 'testFunction', + inputSchema: { type: 'object', properties: {} }, + }, + ], + }), + listPrompts: vi.fn().mockResolvedValue({ prompts: [] }), + request: vi.fn().mockResolvedValue({}), + }; + vi.mocked(ClientLib.Client).mockReturnValue( + mockedClient as unknown as ClientLib.Client, + ); + vi.spyOn(SdkClientStdioLib, 'StdioClientTransport').mockReturnValue( + {} as SdkClientStdioLib.StdioClientTransport, + ); + const mockedToolRegistry = { + registerTool: vi.fn(), + sortTools: vi.fn(), + getToolsByServer: vi.fn().mockReturnValue([]), + getMessageBus: vi.fn().mockReturnValue(undefined), + } as unknown as ToolRegistry; + const promptRegistry = { + registerPrompt: vi.fn(), + getPromptsByServer: vi.fn().mockReturnValue([]), + removePromptsByServer: vi.fn(), + } as unknown as PromptRegistry; + const resourceRegistry = { + getResourcesByServer: vi.fn().mockReturnValue([]), + setResourcesForServer: vi.fn(), + removeResourcesByServer: vi.fn(), + } as unknown as ResourceRegistry; + const client = new McpClient( + 'test-server', + { command: 'test-command', timeout: 45000 }, + workspaceContext, + MOCK_CONTEXT, + false, + '0.0.1', + ); + await client.connect(); + await client.discoverInto(MOCK_CONTEXT, { + toolRegistry: mockedToolRegistry, + promptRegistry, + resourceRegistry, + }); + expect(mockedClient.listTools).toHaveBeenCalledWith( + {}, + expect.objectContaining({ timeout: 45000 }), ); }); @@ -1785,6 +1907,9 @@ describe('mcp-client', () => { expect.anything(), expect.objectContaining({ signal: expect.any(AbortSignal), + // The configured per-server timeout must still be forwarded on the + // refresh path, not discarded when a signal is also passed. + timeout: 50, }), ); diff --git a/packages/core/src/tools/mcp-client.ts b/packages/core/src/tools/mcp-client.ts index 695b8380eb6..ac5d56f816e 100644 --- a/packages/core/src/tools/mcp-client.ts +++ b/packages/core/src/tools/mcp-client.ts @@ -94,6 +94,12 @@ import { export const MCP_DEFAULT_TIMEOUT_MSEC = 10 * 60 * 1000; // default to 10 minutes +// Default timeout for the MCP tools/list discovery request. Discovery runs at +// startup and should fail fast so a slow or misbehaving server does not silently +// freeze the CLI for the full invocation timeout. This is only a default: an +// explicit per-server `timeout` still takes precedence. +export const MCP_DISCOVERY_TIMEOUT_MSEC = 10 * 1000; // default to 10 seconds + export type DiscoveredMCPPrompt = Prompt & { serverName: string; invoke: (params: Record) => Promise; @@ -338,9 +344,11 @@ export class McpClient implements McpProgressReporter { cliConfig, messageBus, { - ...(options ?? { - timeout: this.serverConfig.timeout ?? MCP_DEFAULT_TIMEOUT_MSEC, - }), + // Apply the discovery default first so it is used even when a caller + // (e.g. the refresh path) passes other options such as an abort signal. + // An explicit `timeout` in `options` still takes precedence. + timeout: this.serverConfig.timeout ?? MCP_DISCOVERY_TIMEOUT_MSEC, + ...options, progressReporter: this, }, ); @@ -1238,7 +1246,7 @@ export async function connectAndDiscover( mcpClient, cliConfig, toolRegistry.messageBus, - { timeout: mcpServerConfig.timeout ?? MCP_DEFAULT_TIMEOUT_MSEC }, + { timeout: mcpServerConfig.timeout ?? MCP_DISCOVERY_TIMEOUT_MSEC }, ); // If we have neither prompts nor tools, it's a failed discovery