fix(mcp): preserve discovery failure state

j15z · j15z · commit 23320ee439fb · 2026-07-14T11:17:07.000-07:00
Treat static-header 401s as credential failures, keep OAuth failures pending, and prevent failed refreshes or reflected upstream errors from masquerading as connected state.
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
@@ -4,7 +4,8 @@
 import type { NextRequest } from 'next/server'
 import { beforeEach, describe, expect, it, vi } from 'vitest'
 
-const { mockDiscoverServerTools, mockSelect, mockUpdateSet } = vi.hoisted(() => ({
+const { mockClearCache, mockDiscoverServerTools, mockSelect, mockUpdateSet } = vi.hoisted(() => ({
+  mockClearCache: vi.fn(),
   mockDiscoverServerTools: vi.fn(),
   mockSelect: vi.fn(),
   mockUpdateSet: vi.fn(),
@@ -41,7 +42,7 @@ vi.mock('@/lib/mcp/middleware', () => ({
 
 vi.mock('@/lib/mcp/service', () => ({
   mcpService: {
-    clearCache: vi.fn(),
+    clearCache: mockClearCache,
     discoverServerTools: mockDiscoverServerTools,
   },
 }))
@@ -105,4 +106,62 @@ describe('MCP server refresh route', () => {
       expect.objectContaining({ connectionStatus: expect.anything() })
     )
   })
+
+  it('reports the discovery failure when status persistence leaves a stale connected row', async () => {
+    const reflectedSecret = 'Bearer reflected-static-token'
+    mockDiscoverServerTools.mockRejectedValueOnce(
+      new Error(`Upstream reflected ${reflectedSecret}`)
+    )
+    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(body.data).toEqual(
+      expect.objectContaining({
+        status: 'disconnected',
+        error: 'Internal server error',
+        workflowsUpdated: 0,
+      })
+    )
+    expect(JSON.stringify(body)).not.toContain(reflectedSecret)
+    expect(mockClearCache).not.toHaveBeenCalled()
+  })
+
+  it('preserves a connected status from a newer successful discovery', async () => {
+    mockDiscoverServerTools.mockRejectedValueOnce(new Error('Connection failed'))
+    const newerSuccessfulServer = {
+      ...initialServer,
+      lastConnected: new Date(Date.now() + 60_000),
+      toolCount: 7,
+    }
+    mockUpdateSet.mockReturnValueOnce({
+      where: vi.fn().mockReturnValue({
+        returning: vi.fn().mockResolvedValue([newerSuccessfulServer]),
+      }),
+    })
+
+    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(body.data).toEqual(
+      expect.objectContaining({
+        status: 'connected',
+        error: null,
+        toolCount: 7,
+        workflowsUpdated: 0,
+      })
+    )
+    expect(mockClearCache).toHaveBeenCalledWith('workspace-1')
+  })
 })
diff --git 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 { getErrorMessage, toError } from '@sim/utils/errors'
+import { 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'
@@ -12,6 +12,7 @@ import { withMcpAuth } from '@/lib/mcp/middleware'
 import { mcpService } from '@/lib/mcp/service'
 import type { McpTool, McpToolSchema } from '@/lib/mcp/types'
 import {
+  categorizeError,
   createMcpErrorResponse,
   createMcpSuccessResponse,
   MCP_TOOL_CORE_PARAMS,
@@ -188,6 +189,7 @@ export const POST = withRouteHandler(
         let syncResult: SyncResult = { updatedCount: 0, updatedWorkflowIds: [] }
         let discoveredTools: McpTool[] = []
         let discoveryError: string | null = null
+        const discoveryStartedAt = new Date()
 
         try {
           discoveredTools = await mcpService.discoverServerTools(
@@ -200,12 +202,10 @@ export const POST = withRouteHandler(
             `[${requestId}] Discovered ${discoveredTools.length} tools from server ${serverId}`
           )
         } catch (error) {
-          discoveryError = truncate(
-            getErrorMessage(error, 'Connection failed').split('\n')[0],
-            200,
-            ''
-          )
-          logger.warn(`[${requestId}] Failed to connect to server ${serverId}:`, error)
+          discoveryError = truncate(categorizeError(error).message, 200, '')
+          logger.warn(`[${requestId}] Failed to connect to server ${serverId}`, {
+            error: discoveryError,
+          })
         }
 
         if (discoveryError === null) {
@@ -240,10 +240,21 @@ export const POST = withRouteHandler(
             toolCount: mcpServers.toolCount,
           })
 
-        const connectionStatus = refreshedServer?.connectionStatus ?? 'error'
-        const lastError = refreshedServer ? refreshedServer.lastError : discoveryError
+        let connectionStatus = refreshedServer?.connectionStatus ?? 'error'
+        let lastError = refreshedServer ? refreshedServer.lastError : discoveryError
         const toolCount = refreshedServer?.toolCount ?? discoveredTools.length
 
+        if (discoveryError !== null && connectionStatus === 'connected') {
+          const newerSuccessWonRace =
+            refreshedServer?.lastConnected != null &&
+            refreshedServer.lastConnected > discoveryStartedAt
+
+          if (!newerSuccessWonRace) {
+            connectionStatus = 'disconnected'
+            lastError = discoveryError
+          }
+        }
+
         if (connectionStatus === 'connected') {
           await mcpService.clearCache(workspaceId)
         }
diff --git a/apps/sim/lib/mcp/service.test.ts b/apps/sim/lib/mcp/service.test.ts
@@ -1,6 +1,9 @@
 /**
  * @vitest-environment node
  */
+
+import { UnauthorizedError } from '@modelcontextprotocol/sdk/client/auth.js'
+import { loggerMock } from '@sim/testing'
 import { beforeEach, describe, expect, it, vi } from 'vitest'
 
 const {
@@ -26,24 +29,24 @@ const {
   // an expiry timestamp so negative-cache assertions behave like production.
   const cacheStore = new Map<string, { tools: unknown[]; expiry: number }>()
   const mockCacheAdapter = {
-    get: async (key: string) => {
+    get: vi.fn(async (key: string) => {
       const entry = cacheStore.get(key)
       if (!entry) return null
       if (entry.expiry <= Date.now()) {
         cacheStore.delete(key)
         return null
       }
       return entry
-    },
-    set: async (key: string, tools: unknown[], ttlMs: number) => {
+    }),
+    set: vi.fn(async (key: string, tools: unknown[], ttlMs: number) => {
       cacheStore.set(key, { tools, expiry: Date.now() + ttlMs })
-    },
-    delete: async (key: string) => {
+    }),
+    delete: vi.fn(async (key: string) => {
       cacheStore.delete(key)
-    },
-    clear: async () => {
+    }),
+    clear: vi.fn(async () => {
       cacheStore.clear()
-    },
+    }),
     dispose: () => {},
   }
   return {
@@ -132,6 +135,8 @@ vi.mock('@/lib/mcp/storage', () => ({
 import { mcpService } from '@/lib/mcp/service'
 import { McpOauthAuthorizationRequiredError } from '@/lib/mcp/types'
 
+const mockLogger = vi.mocked(loggerMock.createLogger).mock.results.at(-1)?.value
+
 const WORKSPACE_ID = 'workspace-test'
 const USER_ID = 'user-test'
 
@@ -329,6 +334,70 @@ describe('McpService.discoverTools per-server caching', () => {
     expect(mockListTools).not.toHaveBeenCalled()
   })
 
+  it('persists and negative-caches UnauthorizedError for a headers-auth server', async () => {
+    const reflectedCredential = 'Bearer static-secret-for-bulk-discovery'
+    mockGetWorkspaceServersRows.mockResolvedValue([
+      dbRow('mcp-a', 'A', {
+        statusConfig: { consecutiveFailures: 0, lastSuccessfulDiscovery: null },
+      }),
+    ])
+    mockListTools.mockRejectedValueOnce(
+      new UnauthorizedError(`Rejected Authorization: ${reflectedCredential}`)
+    )
+
+    const first = await mcpService.discoverTools(USER_ID, WORKSPACE_ID)
+    expect(first).toEqual([])
+
+    await vi.waitFor(() => {
+      expect(mockUpdateSet).toHaveBeenCalledWith(
+        expect.objectContaining({
+          connectionStatus: 'disconnected',
+          lastError: 'Authentication failed',
+          statusConfig: { consecutiveFailures: 1, lastSuccessfulDiscovery: null },
+        })
+      )
+      expect(mockCacheAdapter.set).toHaveBeenCalledWith(
+        `workspace:${WORKSPACE_ID}:server:mcp-a:failure`,
+        [],
+        expect.any(Number)
+      )
+    })
+    expect(JSON.stringify(mockUpdateSet.mock.calls)).not.toContain(reflectedCredential)
+    expect(JSON.stringify(mockCacheAdapter.set.mock.calls)).not.toContain(reflectedCredential)
+    expect(JSON.stringify(mockLogger?.warn.mock.calls)).not.toContain(reflectedCredential)
+
+    mockListTools.mockClear()
+    const second = await mcpService.discoverTools(USER_ID, WORKSPACE_ID)
+    expect(second).toEqual([])
+    expect(mockListTools).not.toHaveBeenCalled()
+  })
+
+  it('keeps UnauthorizedError soft-pending for an OAuth server', async () => {
+    mockGetWorkspaceServersRows.mockResolvedValue([dbRow('mcp-a', 'A', { authType: 'oauth' })])
+    mockResolveEnvVars.mockRejectedValue(new UnauthorizedError('OAuth token rejected'))
+
+    const first = await mcpService.discoverTools(USER_ID, WORKSPACE_ID)
+    expect(first).toEqual([])
+
+    await vi.waitFor(() => {
+      expect(mockUpdateSet).toHaveBeenCalledWith(
+        expect.objectContaining({
+          connectionStatus: 'disconnected',
+          lastError: null,
+        })
+      )
+    })
+    expect(mockCacheAdapter.set).not.toHaveBeenCalledWith(
+      `workspace:${WORKSPACE_ID}:server:mcp-a:failure`,
+      [],
+      expect.any(Number)
+    )
+
+    mockResolveEnvVars.mockClear()
+    await mcpService.discoverTools(USER_ID, WORKSPACE_ID)
+    expect(mockResolveEnvVars).toHaveBeenCalledTimes(1)
+  })
+
   it('successful discoverServerTools clears the negative cache', async () => {
     mockGetWorkspaceServersRows.mockResolvedValue([dbRow('mcp-a', 'A')])
     mockListTools.mockRejectedValueOnce(new Error('Request timed out'))
@@ -396,6 +465,66 @@ describe('McpService.discoverTools per-server caching', () => {
     )
   })
 
+  it('persists and negative-caches per-server UnauthorizedError for headers auth', async () => {
+    const reflectedCredential = 'Bearer static-secret-for-server-discovery'
+    mockGetWorkspaceServersRows.mockResolvedValue([
+      dbRow('mcp-a', 'A', {
+        statusConfig: { consecutiveFailures: 0, lastSuccessfulDiscovery: null },
+      }),
+    ])
+    mockListTools.mockRejectedValueOnce(
+      new UnauthorizedError(`Rejected Authorization: ${reflectedCredential}`)
+    )
+
+    await expect(mcpService.discoverServerTools(USER_ID, 'mcp-a', WORKSPACE_ID)).rejects.toThrow(
+      reflectedCredential
+    )
+
+    expect(mockUpdateSet).toHaveBeenCalledWith(
+      expect.objectContaining({
+        connectionStatus: 'disconnected',
+        lastError: 'Authentication failed',
+        statusConfig: { consecutiveFailures: 1, lastSuccessfulDiscovery: null },
+      })
+    )
+    expect(JSON.stringify(mockUpdateSet.mock.calls)).not.toContain(reflectedCredential)
+    expect(JSON.stringify(mockCacheAdapter.set.mock.calls)).not.toContain(reflectedCredential)
+    expect(JSON.stringify(mockLogger?.warn.mock.calls)).not.toContain(reflectedCredential)
+
+    mockListTools.mockClear()
+    await expect(mcpService.discoverServerTools(USER_ID, 'mcp-a', WORKSPACE_ID)).rejects.toThrow(
+      'cooldown'
+    )
+    expect(mockListTools).not.toHaveBeenCalled()
+  })
+
+  it('keeps per-server UnauthorizedError soft-pending for OAuth auth', async () => {
+    mockGetWorkspaceServersRows.mockResolvedValue([dbRow('mcp-a', 'A', { authType: 'oauth' })])
+    mockResolveEnvVars.mockRejectedValue(new UnauthorizedError('OAuth token rejected'))
+
+    await expect(mcpService.discoverServerTools(USER_ID, 'mcp-a', WORKSPACE_ID)).rejects.toThrow(
+      'OAuth token rejected'
+    )
+
+    expect(mockUpdateSet).toHaveBeenCalledWith(
+      expect.objectContaining({
+        connectionStatus: 'disconnected',
+        lastError: null,
+      })
+    )
+    expect(mockCacheAdapter.set).not.toHaveBeenCalledWith(
+      `workspace:${WORKSPACE_ID}:server:mcp-a:failure`,
+      [],
+      expect.any(Number)
+    )
+
+    mockResolveEnvVars.mockClear()
+    await expect(mcpService.discoverServerTools(USER_ID, 'mcp-a', WORKSPACE_ID)).rejects.toThrow(
+      'OAuth token rejected'
+    )
+    expect(mockResolveEnvVars).toHaveBeenCalledTimes(1)
+  })
+
   it('promotes the persisted server status to error on the third consecutive failure', async () => {
     mockGetWorkspaceServersRows.mockResolvedValue([
       dbRow('mcp-a', 'A', {
diff --git a/apps/sim/lib/mcp/service.ts b/apps/sim/lib/mcp/service.ts
