Skip to content

fix(core): shorten MCP tools/list discovery timeout so it fails fast #28410

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
sahilempire wants to merge 1 commit into
base: main
Choose a base branch
from
+138 −5
Open

fix(core): shorten MCP tools/list discovery timeout so it fails fast #28410

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
127 changes: 126 additions & 1 deletion packages/core/src/tools/mcp-client.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 }),
);
});

Expand Down Expand Up @@ -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,
}),
);

Expand Down
16 changes: 12 additions & 4 deletions packages/core/src/tools/mcp-client.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<string, unknown>) => Promise<GetPromptResult>;
Expand Down Expand Up @@ -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,
},
);
Expand Down Expand Up @@ -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
Expand Down
Loading