improvement(mcp): align tool discovery timeouts and retries with MCP protocol standard

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: [],
+      })
+    )
+  })
 })

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()

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) })
     }
   }
 

apps/sim/lib/mcp/client.test.ts

@@ -3,14 +3,15 @@
  */
 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(),
     info: vi.fn(),
     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<void>) => {
               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: {

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)) {

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([

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.

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