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() 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..c568f425d61 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, }) } } @@ -63,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' @@ -80,6 +82,10 @@ describe('McpClient notification handler', () => { capturedNotificationHandler = null 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 () => { @@ -152,6 +158,42 @@ 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('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 f9e1485c824..50a90355c14 100644 --- a/apps/sim/lib/mcp/client.ts +++ b/apps/sim/lib/mcp/client.ts @@ -104,6 +104,14 @@ export class McpClient { capabilities: {}, } ) + + // 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, + error: sanitizeForLogging(getErrorMessage(error, 'Unknown transport error'), 200), + }) + } } /** @@ -217,9 +225,31 @@ export class McpClient { throw new McpConnectionError('Not connected to server', this.config.name) } + 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(), + 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, { - timeout: MCP_CLIENT_CONSTANTS.LIST_TOOLS_TIMEOUT_MS, + // resetTimeoutOnProgress only takes effect when onprogress is supplied. + 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..960007bf868 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,45 @@ 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 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 session (retry re-initializes); 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 +810,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..2f04d3039c3 100644 --- a/apps/sim/lib/mcp/utils.ts +++ b/apps/sim/lib/mcp/utils.ts @@ -51,7 +51,10 @@ 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 tools/list (gap between progress events); raised from 10s toward the SDK's 60s default. */ + LIST_TOOLS_TIMEOUT_MS: 30_000, + /** 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