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