From cf68df9470b65eb5a3190806f3fdc7677b725751 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Fri, 17 Jul 2026 17:31:45 -0700 Subject: [PATCH 1/4] improvement(mcp): align tool discovery timeouts and retries with MCP protocol standard - Raise tools/list idle timeout 10s -> 30s (derived from per-server config, clamped to max execution timeout) with a 60s hard cap via maxTotalTimeout, matching the SDK's DEFAULT_REQUEST_TIMEOUT_MSEC. Enable resetTimeoutOnProgress with an onprogress handler so slow-but-alive servers are not spuriously failed. - Retry read-only tools/list on transient transport errors (timeout, connection-closed, 429/5xx, session 404/400, network) with jittered backoff. tools/call stays conservative (session-error retry only) since it is not idempotent. The MCP SDK does not retry POST requests, so the app owns this. - Wire client.onerror so out-of-band transport failures are observed, not silently dropped. - Map timeouts to a user-facing message and surface refresh/remove failures via the standard emcn toast instead of swallowing them. --- .../settings/components/mcp/mcp.tsx | 4 +- apps/sim/lib/mcp/client.test.ts | 26 ++++++++- apps/sim/lib/mcp/client.ts | 38 ++++++++++++- apps/sim/lib/mcp/service.test.ts | 25 +++++++-- apps/sim/lib/mcp/service.ts | 53 +++++++++++++++++-- apps/sim/lib/mcp/utils.ts | 15 +++++- 6 files changed, 150 insertions(+), 11 deletions(-) diff --git a/apps/sim/app/workspace/[workspaceId]/settings/components/mcp/mcp.tsx b/apps/sim/app/workspace/[workspaceId]/settings/components/mcp/mcp.tsx index 35956d38b17..c2769a36e54 100644 --- a/apps/sim/app/workspace/[workspaceId]/settings/components/mcp/mcp.tsx +++ b/apps/sim/app/workspace/[workspaceId]/settings/components/mcp/mcp.tsx @@ -1,7 +1,7 @@ 'use client' import { useEffect, useRef, useState } from 'react' -import { Badge, Button, Chip, ChipConfirmModal, cn, Tooltip } from '@sim/emcn' +import { Badge, Button, Chip, ChipConfirmModal, cn, Tooltip, toast } from '@sim/emcn' import { ArrowLeft } from '@sim/emcn/icons' import { createLogger } from '@sim/logger' import { getErrorMessage } from '@sim/utils/errors' @@ -224,6 +224,7 @@ export function MCP() { logger.info(`Removed MCP server: ${serverId}`) } catch (error) { logger.error('Failed to remove MCP server:', error) + toast.error('Failed to remove MCP server', { description: getErrorMessage(error) }) } finally { setDeletingServers((prev) => { const newSet = new Set(prev) @@ -301,6 +302,7 @@ export function MCP() { } } catch (error) { logger.error('Failed to refresh MCP server:', error) + toast.error('Failed to refresh MCP server', { description: getErrorMessage(error) }) } } diff --git a/apps/sim/lib/mcp/client.test.ts b/apps/sim/lib/mcp/client.test.ts index de722343e48..f8449b240a8 100644 --- a/apps/sim/lib/mcp/client.test.ts +++ b/apps/sim/lib/mcp/client.test.ts @@ -3,7 +3,7 @@ */ import { beforeEach, describe, expect, it, vi } from 'vitest' -const { mockLogger, mockSdkConnect } = vi.hoisted(() => ({ +const { mockLogger, mockSdkConnect, mockSdkListTools } = vi.hoisted(() => ({ mockLogger: { debug: vi.fn(), error: vi.fn(), @@ -11,6 +11,7 @@ const { mockLogger, mockSdkConnect } = vi.hoisted(() => ({ warn: vi.fn(), }, mockSdkConnect: vi.fn().mockResolvedValue(undefined), + mockSdkListTools: vi.fn().mockResolvedValue({ tools: [] }), })) vi.mock('@sim/logger', () => ({ @@ -37,7 +38,7 @@ vi.mock('@modelcontextprotocol/sdk/client/index.js', () => ({ .mockImplementation((_schema: unknown, handler: () => Promise) => { capturedNotificationHandler = handler }), - listTools: vi.fn().mockResolvedValue({ tools: [] }), + listTools: mockSdkListTools, }) } } @@ -80,6 +81,7 @@ describe('McpClient notification handler', () => { capturedNotificationHandler = null vi.clearAllMocks() mockSdkConnect.mockResolvedValue(undefined) + mockSdkListTools.mockResolvedValue({ tools: [] }) }) it('fires onToolsChanged when a notification arrives while connected', async () => { @@ -152,6 +154,26 @@ describe('McpClient notification handler', () => { expect(mockSdkConnect).toHaveBeenCalledWith(expect.anything(), { timeout: 30_000 }) }) + it('bounds tools/list with an idle timeout, hard cap, and progress reset', async () => { + const client = new McpClient({ + config: createConfig(), + securityPolicy: { requireConsent: false, auditLevel: 'basic' }, + }) + + await client.connect() + await client.listTools() + + expect(mockSdkListTools).toHaveBeenCalledWith( + undefined, + expect.objectContaining({ + timeout: 30_000, + maxTotalTimeout: 60_000, + resetTimeoutOnProgress: true, + onprogress: expect.any(Function), + }) + ) + }) + it('logs connection diagnostics without header values', async () => { const client = new McpClient({ config: { diff --git a/apps/sim/lib/mcp/client.ts b/apps/sim/lib/mcp/client.ts index f9e1485c824..1ceff9cc477 100644 --- a/apps/sim/lib/mcp/client.ts +++ b/apps/sim/lib/mcp/client.ts @@ -104,6 +104,17 @@ export class McpClient { capabilities: {}, } ) + + // The SDK invokes onerror for out-of-band transport failures (SSE + // disconnects, exhausted reconnects) that are not tied to an in-flight + // request. Without a handler these are silently dropped, so wire one for + // observability. See @modelcontextprotocol/sdk Protocol.onerror. + this.client.onerror = (error) => { + logger.warn(`MCP transport error for ${this.config.name}`, { + serverId: this.config.id, + error: sanitizeForLogging(getErrorMessage(error, 'Unknown transport error'), 200), + }) + } } /** @@ -217,9 +228,34 @@ export class McpClient { throw new McpConnectionError('Not connected to server', this.config.name) } + const configuredTimeout = this.config.timeout + const idleTimeoutMs = Math.min( + configuredTimeout !== undefined && Number.isFinite(configuredTimeout) && configuredTimeout > 0 + ? Math.floor(configuredTimeout) + : MCP_CLIENT_CONSTANTS.LIST_TOOLS_TIMEOUT_MS, + getMaxExecutionTimeout() + ) + const maxTotalTimeoutMs = Math.max( + idleTimeoutMs, + MCP_CLIENT_CONSTANTS.LIST_TOOLS_MAX_TOTAL_TIMEOUT_MS + ) + try { const result: ListToolsResult = await this.client.listTools(undefined, { - timeout: MCP_CLIENT_CONSTANTS.LIST_TOOLS_TIMEOUT_MS, + // `timeout` bounds silence between progress notifications; `maxTotalTimeout` + // is the hard ceiling regardless of progress. `resetTimeoutOnProgress` only + // takes effect when an `onprogress` callback is supplied (the SDK sets the + // request progressToken only then), so both are passed together. + timeout: idleTimeoutMs, + maxTotalTimeout: maxTotalTimeoutMs, + resetTimeoutOnProgress: true, + onprogress: (progress) => { + logger.debug(`Tool discovery progress from ${this.config.name}`, { + serverId: this.config.id, + progress: progress.progress, + total: progress.total, + }) + }, }) if (!result.tools || !Array.isArray(result.tools)) { diff --git a/apps/sim/lib/mcp/service.test.ts b/apps/sim/lib/mcp/service.test.ts index 49723bfc81f..7349b39e71f 100644 --- a/apps/sim/lib/mcp/service.test.ts +++ b/apps/sim/lib/mcp/service.test.ts @@ -400,7 +400,11 @@ describe('McpService.discoverTools per-server caching', () => { it('successful discoverServerTools clears the negative cache', async () => { mockGetWorkspaceServersRows.mockResolvedValue([dbRow('mcp-a', 'A')]) - mockListTools.mockRejectedValueOnce(new Error('Request timed out')) + // A timeout is transient/retryable, so it must fail every attempt to reach + // the persisted-failure path. + mockListTools + .mockRejectedValueOnce(new Error('Request timed out')) + .mockRejectedValueOnce(new Error('Request timed out')) await expect(mcpService.discoverServerTools(USER_ID, 'mcp-a', WORKSPACE_ID)).rejects.toThrow( 'Request timed out' @@ -450,7 +454,9 @@ describe('McpService.discoverTools per-server caching', () => { statusConfig: { consecutiveFailures: 0, lastSuccessfulDiscovery: null }, }), ]) - mockListTools.mockRejectedValueOnce(new Error('Request timed out')) + mockListTools + .mockRejectedValueOnce(new Error('Request timed out')) + .mockRejectedValueOnce(new Error('Request timed out')) await expect(mcpService.discoverServerTools(USER_ID, 'mcp-a', WORKSPACE_ID)).rejects.toThrow( 'Request timed out' @@ -459,12 +465,25 @@ describe('McpService.discoverTools per-server caching', () => { expect(mockUpdateSet).toHaveBeenCalledWith( expect.objectContaining({ connectionStatus: 'disconnected', - lastError: 'Request timed out', + // Raw SDK timeout text is mapped to a user-facing message before persisting. + lastError: 'The MCP server took too long to respond and timed out', statusConfig: { consecutiveFailures: 1, lastSuccessfulDiscovery: null }, }) ) }) + it('retries a transient tools/list timeout and succeeds on the second attempt', async () => { + mockGetWorkspaceServersRows.mockResolvedValue([dbRow('mcp-a', 'A')]) + mockListTools + .mockRejectedValueOnce(new Error('Request timed out')) + .mockResolvedValueOnce([tool('a1', 'mcp-a')]) + + const tools = await mcpService.discoverServerTools(USER_ID, 'mcp-a', WORKSPACE_ID) + + expect(tools.map((t) => t.name)).toEqual(['a1']) + expect(mockListTools).toHaveBeenCalledTimes(2) + }) + it('persists and negative-caches per-server UnauthorizedError for headers auth', async () => { const reflectedCredential = 'Bearer static-secret-for-server-discovery' mockGetWorkspaceServersRows.mockResolvedValue([ diff --git a/apps/sim/lib/mcp/service.ts b/apps/sim/lib/mcp/service.ts index 3b9e473efe5..4a405414946 100644 --- a/apps/sim/lib/mcp/service.ts +++ b/apps/sim/lib/mcp/service.ts @@ -1,10 +1,12 @@ import { UnauthorizedError } from '@modelcontextprotocol/sdk/client/auth.js' import { StreamableHTTPError } from '@modelcontextprotocol/sdk/client/streamableHttp.js' +import { ErrorCode, McpError } from '@modelcontextprotocol/sdk/types.js' import { db } from '@sim/db' import { mcpServers } from '@sim/db/schema' import { createLogger } from '@sim/logger' import { getErrorMessage } from '@sim/utils/errors' import { sleep } from '@sim/utils/helpers' +import { backoffWithJitter } from '@sim/utils/retry' import { and, eq, isNull, lte, or } from 'drizzle-orm' import { isTest } from '@/lib/core/config/env-flags' import { generateRequestId } from '@/lib/core/utils/request' @@ -85,9 +87,54 @@ function getDiscoveryFailureMessage( if (authType !== 'oauth' && error instanceof UnauthorizedError) { return 'Authentication failed' } + if (isTimeoutError(error)) { + return 'The MCP server took too long to respond and timed out' + } return getErrorMessage(error, fallback) } +function isTimeoutError(error: unknown): boolean { + if (error instanceof McpError && error.code === ErrorCode.RequestTimeout) { + return true + } + return getErrorMessage(error, '').toLowerCase().includes('timed out') +} + +/** + * Transient transport failures that a read-only `tools/list` may safely retry. + * `tools/list` is idempotent, so re-issuing it after a timeout, dropped + * connection, or 5xx/429 cannot double-apply side effects — unlike `tools/call`, + * which is never retried on these. OAuth-authorization and terminal 4xx errors + * are intentionally excluded: they need re-auth or a config fix, not a retry. + * The MCP TypeScript SDK does not retry POST requests itself (only SSE streams), + * so the app owns this. + */ +function isRetryableDiscoveryError(error: unknown): boolean { + if (isTimeoutError(error)) return true + if (error instanceof McpError) { + return error.code === ErrorCode.ConnectionClosed + } + if (error instanceof StreamableHTTPError) { + // 404/400 = stale/malformed session id: retrying rebuilds the client with no + // session id, which re-initializes per MCP spec. 429/5xx = transient upstream. + const code = error.code + return ( + code === 404 || + code === 400 || + code === 429 || + (typeof code === 'number' && code >= 500 && code <= 599) + ) + } + const message = getErrorMessage(error, '').toLowerCase() + return ( + message.includes('econnreset') || + message.includes('socket hang up') || + message.includes('etimedout') || + message.includes('fetch failed') || + message.includes('network') + ) +} + class McpService { private cacheAdapter: McpCacheStorageAdapter private readonly cacheTimeout = MCP_CONSTANTS.CACHE_TIMEOUT @@ -772,12 +819,12 @@ class McpService { await client.disconnect() } } catch (error) { - if (this.isSessionError(error) && attempt < maxRetries - 1) { + if (isRetryableDiscoveryError(error) && attempt < maxRetries - 1) { logger.warn( - `[${requestId}] Session error discovering tools from server ${serverId}, retrying (attempt ${attempt + 1}):`, + `[${requestId}] Transient error discovering tools from server ${serverId}, retrying (attempt ${attempt + 1}):`, error ) - await sleep(100) + await sleep(backoffWithJitter(attempt + 1, null, { baseMs: 250, maxMs: 2000 })) continue } // Drop positive cache so a follow-up doesn't return stale tools. diff --git a/apps/sim/lib/mcp/utils.ts b/apps/sim/lib/mcp/utils.ts index e5c2f9db22f..2455fff77d9 100644 --- a/apps/sim/lib/mcp/utils.ts +++ b/apps/sim/lib/mcp/utils.ts @@ -51,7 +51,20 @@ export function sanitizeHeaders( export const MCP_CLIENT_CONSTANTS = { CLIENT_TIMEOUT: DEFAULT_EXECUTION_TIMEOUT_MS, AUTO_REFRESH_INTERVAL: 5 * 60 * 1000, - LIST_TOOLS_TIMEOUT_MS: 10_000, + /** + * Idle timeout for a `tools/list` request. Raised from the former aggressive + * 10s toward the MCP TypeScript SDK's 60s default (`DEFAULT_REQUEST_TIMEOUT_MSEC`) + * so a legitimately slow-to-enumerate server is not spuriously failed with + * `McpError -32001`. Combined with `resetTimeoutOnProgress`, this is the gap + * allowed between progress notifications, not the absolute ceiling. + */ + LIST_TOOLS_TIMEOUT_MS: 30_000, + /** + * Absolute ceiling for a `tools/list` request regardless of progress, mirroring + * the SDK's `maxTotalTimeout` safeguard. Matches the SDK's 60s default so a + * server emitting continuous progress still cannot hang discovery indefinitely. + */ + LIST_TOOLS_MAX_TOTAL_TIMEOUT_MS: 60_000, FAILURE_CACHE_TTL_MS: 120_000, } as const From 0c05adf022705b8585fa74fba1bb6d436775bebc Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Fri, 17 Jul 2026 17:36:05 -0700 Subject: [PATCH 2/4] chore(mcp): trim comments to match codebase density --- apps/sim/lib/mcp/client.ts | 10 ++-------- apps/sim/lib/mcp/service.ts | 13 ++----------- apps/sim/lib/mcp/utils.ts | 14 ++------------ 3 files changed, 6 insertions(+), 31 deletions(-) diff --git a/apps/sim/lib/mcp/client.ts b/apps/sim/lib/mcp/client.ts index 1ceff9cc477..3273dbe4d18 100644 --- a/apps/sim/lib/mcp/client.ts +++ b/apps/sim/lib/mcp/client.ts @@ -105,10 +105,7 @@ export class McpClient { } ) - // The SDK invokes onerror for out-of-band transport failures (SSE - // disconnects, exhausted reconnects) that are not tied to an in-flight - // request. Without a handler these are silently dropped, so wire one for - // observability. See @modelcontextprotocol/sdk Protocol.onerror. + // Observe out-of-band transport errors the SDK would otherwise drop silently. this.client.onerror = (error) => { logger.warn(`MCP transport error for ${this.config.name}`, { serverId: this.config.id, @@ -242,10 +239,7 @@ export class McpClient { try { const result: ListToolsResult = await this.client.listTools(undefined, { - // `timeout` bounds silence between progress notifications; `maxTotalTimeout` - // is the hard ceiling regardless of progress. `resetTimeoutOnProgress` only - // takes effect when an `onprogress` callback is supplied (the SDK sets the - // request progressToken only then), so both are passed together. + // resetTimeoutOnProgress only takes effect when onprogress is supplied. timeout: idleTimeoutMs, maxTotalTimeout: maxTotalTimeoutMs, resetTimeoutOnProgress: true, diff --git a/apps/sim/lib/mcp/service.ts b/apps/sim/lib/mcp/service.ts index 4a405414946..960007bf868 100644 --- a/apps/sim/lib/mcp/service.ts +++ b/apps/sim/lib/mcp/service.ts @@ -100,23 +100,14 @@ function isTimeoutError(error: unknown): boolean { return getErrorMessage(error, '').toLowerCase().includes('timed out') } -/** - * Transient transport failures that a read-only `tools/list` may safely retry. - * `tools/list` is idempotent, so re-issuing it after a timeout, dropped - * connection, or 5xx/429 cannot double-apply side effects — unlike `tools/call`, - * which is never retried on these. OAuth-authorization and terminal 4xx errors - * are intentionally excluded: they need re-auth or a config fix, not a retry. - * The MCP TypeScript SDK does not retry POST requests itself (only SSE streams), - * so the app owns this. - */ +/** Transient failures a read-only `tools/list` may safely retry (idempotent, unlike `tools/call`); excludes OAuth and terminal 4xx. */ function isRetryableDiscoveryError(error: unknown): boolean { if (isTimeoutError(error)) return true if (error instanceof McpError) { return error.code === ErrorCode.ConnectionClosed } if (error instanceof StreamableHTTPError) { - // 404/400 = stale/malformed session id: retrying rebuilds the client with no - // session id, which re-initializes per MCP spec. 429/5xx = transient upstream. + // 404/400 = stale session (retry re-initializes); 429/5xx = transient upstream. const code = error.code return ( code === 404 || diff --git a/apps/sim/lib/mcp/utils.ts b/apps/sim/lib/mcp/utils.ts index 2455fff77d9..2f04d3039c3 100644 --- a/apps/sim/lib/mcp/utils.ts +++ b/apps/sim/lib/mcp/utils.ts @@ -51,19 +51,9 @@ export function sanitizeHeaders( export const MCP_CLIENT_CONSTANTS = { CLIENT_TIMEOUT: DEFAULT_EXECUTION_TIMEOUT_MS, AUTO_REFRESH_INTERVAL: 5 * 60 * 1000, - /** - * Idle timeout for a `tools/list` request. Raised from the former aggressive - * 10s toward the MCP TypeScript SDK's 60s default (`DEFAULT_REQUEST_TIMEOUT_MSEC`) - * so a legitimately slow-to-enumerate server is not spuriously failed with - * `McpError -32001`. Combined with `resetTimeoutOnProgress`, this is the gap - * allowed between progress notifications, not the absolute ceiling. - */ + /** Idle timeout for tools/list (gap between progress events); raised from 10s toward the SDK's 60s default. */ LIST_TOOLS_TIMEOUT_MS: 30_000, - /** - * Absolute ceiling for a `tools/list` request regardless of progress, mirroring - * the SDK's `maxTotalTimeout` safeguard. Matches the SDK's 60s default so a - * server emitting continuous progress still cannot hang discovery indefinitely. - */ + /** Hard ceiling for tools/list regardless of progress (SDK maxTotalTimeout safeguard). */ LIST_TOOLS_MAX_TOTAL_TIMEOUT_MS: 60_000, FAILURE_CACHE_TTL_MS: 120_000, } as const From 6e24e02993078505ad14d4e839bd0b8f7c74ac19 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Fri, 17 Jul 2026 17:41:14 -0700 Subject: [PATCH 3/4] fix(mcp): don't fail refresh with 500 when post-discovery workflow sync throws Discovery already persists status and caches tools; a syncToolSchemasToWorkflows failure was escaping the refactored try/catch and returning 500 despite the refresh having succeeded. Guard the secondary sync so it degrades to zero workflows updated instead. --- .../mcp/servers/[id]/refresh/route.test.ts | 33 +++++++++++++++++++ .../app/api/mcp/servers/[id]/refresh/route.ts | 25 +++++++++----- 2 files changed, 50 insertions(+), 8 deletions(-) diff --git a/apps/sim/app/api/mcp/servers/[id]/refresh/route.test.ts b/apps/sim/app/api/mcp/servers/[id]/refresh/route.test.ts index 06329a1ada7..e3f217fbdde 100644 --- a/apps/sim/app/api/mcp/servers/[id]/refresh/route.test.ts +++ b/apps/sim/app/api/mcp/servers/[id]/refresh/route.test.ts @@ -164,4 +164,37 @@ describe('MCP server refresh route', () => { ) expect(mockClearCache).toHaveBeenCalledWith('workspace-1') }) + + it('does not 500 when workflow sync fails after a successful discovery', async () => { + mockDiscoverServerTools.mockResolvedValueOnce([ + { + name: 'search', + description: 'Search tool', + inputSchema: {}, + serverId: 'server-1', + serverName: 'OAuth Server', + }, + ]) + // The route's server lookup consumes the first select (beforeEach). The sync's + // workflow select is left unmocked, so it throws — exercising the guard that + // keeps a secondary sync failure from turning a successful refresh into a 500. + mockUpdateSet.mockReturnValueOnce({ + where: vi.fn().mockReturnValue({ returning: vi.fn().mockResolvedValue([initialServer]) }), + }) + + const request = new Request('http://localhost/api/mcp/servers/server-1/refresh', { + method: 'POST', + }) as NextRequest + const response = await POST(request, { params: Promise.resolve({ id: 'server-1' }) }) + const body = await response.json() + + expect(response.status).toBe(200) + expect(body.data).toEqual( + expect.objectContaining({ + status: 'connected', + workflowsUpdated: 0, + updatedWorkflowIds: [], + }) + ) + }) }) diff --git a/apps/sim/app/api/mcp/servers/[id]/refresh/route.ts b/apps/sim/app/api/mcp/servers/[id]/refresh/route.ts index e778c912075..26bb6ddaaef 100644 --- a/apps/sim/app/api/mcp/servers/[id]/refresh/route.ts +++ b/apps/sim/app/api/mcp/servers/[id]/refresh/route.ts @@ -1,7 +1,7 @@ import { db } from '@sim/db' import { mcpServers, workflow, workflowBlocks } from '@sim/db/schema' import { createLogger } from '@sim/logger' -import { toError } from '@sim/utils/errors' +import { getErrorMessage, toError } from '@sim/utils/errors' import { truncate } from '@sim/utils/string' import { and, eq, inArray, isNull } from 'drizzle-orm' import type { NextRequest } from 'next/server' @@ -209,13 +209,22 @@ export const POST = withRouteHandler( } if (discoveryError === null) { - syncResult = await syncToolSchemasToWorkflows( - workspaceId, - serverId, - discoveredTools, - requestId, - { url: server.url ?? undefined, name: server.name ?? undefined } - ) + try { + syncResult = await syncToolSchemasToWorkflows( + workspaceId, + serverId, + discoveredTools, + requestId, + { url: server.url ?? undefined, name: server.name ?? undefined } + ) + } catch (error) { + // Discovery already persisted status and cached tools; a workflow-sync + // failure is a secondary propagation and must not fail the refresh with + // a 500. Surface it as zero workflows updated instead. + logger.warn(`[${requestId}] Tool schema sync failed after successful discovery`, { + error: getErrorMessage(error), + }) + } } const now = new Date() From 561a32f809e9a05230e152b24191bbdd9da7a998 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Fri, 17 Jul 2026 17:46:44 -0700 Subject: [PATCH 4/4] fix(mcp): keep tools/list absolute timeout ceiling hard A configured per-server timeout above 60s was expanding maxTotalTimeout past the intended absolute discovery ceiling. Clamp the idle timeout to the ceiling too so tools/list can never hang the UI beyond it; connect() and callTool() still honor the full configured/execution timeout. --- apps/sim/lib/mcp/client.test.ts | 20 ++++++++++++++++++++ apps/sim/lib/mcp/client.ts | 8 ++++---- 2 files changed, 24 insertions(+), 4 deletions(-) diff --git a/apps/sim/lib/mcp/client.test.ts b/apps/sim/lib/mcp/client.test.ts index f8449b240a8..c568f425d61 100644 --- a/apps/sim/lib/mcp/client.test.ts +++ b/apps/sim/lib/mcp/client.test.ts @@ -64,6 +64,7 @@ vi.mock('@/lib/core/execution-limits', () => ({ })) import { StreamableHTTPClientTransport } from '@modelcontextprotocol/sdk/client/streamableHttp.js' +import { getMaxExecutionTimeout } from '@/lib/core/execution-limits' import { McpClient } from './client' import type { McpClientOptions, McpServerConfig } from './types' @@ -82,6 +83,9 @@ describe('McpClient notification handler', () => { vi.clearAllMocks() mockSdkConnect.mockResolvedValue(undefined) mockSdkListTools.mockResolvedValue({ tools: [] }) + // clearAllMocks resets call history but not implementations; re-establish the + // default so a per-test override can't bleed into later tests. + vi.mocked(getMaxExecutionTimeout).mockReturnValue(30_000) }) it('fires onToolsChanged when a notification arrives while connected', async () => { @@ -174,6 +178,22 @@ describe('McpClient notification handler', () => { ) }) + it('clamps a configured tools/list timeout to the absolute discovery ceiling', async () => { + vi.mocked(getMaxExecutionTimeout).mockReturnValue(120_000) + const client = new McpClient({ + config: { ...createConfig(), timeout: 300_000 }, + securityPolicy: { requireConsent: false, auditLevel: 'basic' }, + }) + + await client.connect() + await client.listTools() + + expect(mockSdkListTools).toHaveBeenCalledWith( + undefined, + expect.objectContaining({ timeout: 60_000, maxTotalTimeout: 60_000 }) + ) + }) + it('logs connection diagnostics without header values', async () => { const client = new McpClient({ config: { diff --git a/apps/sim/lib/mcp/client.ts b/apps/sim/lib/mcp/client.ts index 3273dbe4d18..50a90355c14 100644 --- a/apps/sim/lib/mcp/client.ts +++ b/apps/sim/lib/mcp/client.ts @@ -226,16 +226,16 @@ export class McpClient { } const configuredTimeout = this.config.timeout + // Idle timeout honors the per-server config but never exceeds the absolute + // discovery ceiling, so tools/list can't hang the UI past that cap. const idleTimeoutMs = Math.min( configuredTimeout !== undefined && Number.isFinite(configuredTimeout) && configuredTimeout > 0 ? Math.floor(configuredTimeout) : MCP_CLIENT_CONSTANTS.LIST_TOOLS_TIMEOUT_MS, - getMaxExecutionTimeout() - ) - const maxTotalTimeoutMs = Math.max( - idleTimeoutMs, + getMaxExecutionTimeout(), MCP_CLIENT_CONSTANTS.LIST_TOOLS_MAX_TOTAL_TIMEOUT_MS ) + const maxTotalTimeoutMs = MCP_CLIENT_CONSTANTS.LIST_TOOLS_MAX_TOTAL_TIMEOUT_MS try { const result: ListToolsResult = await this.client.listTools(undefined, {