fix(mcp): recover cleanly from OAuth failures (#5595)

* fix(mcp): improve OAuth failure recovery

* fix(mcp): address OAuth recovery review findings

* fix(mcp): prefer static bearer auth in connection tests

* fix(mcp): preserve discovery failure state

Treat static-header 401s as credential failures, keep OAuth failures pending, and prevent failed refreshes or reflected upstream errors from masquerading as connected state.

Author: j15z

apps/sim/app/api/mcp/servers/[id]/refresh/route.test.ts

@@ -0,0 +1,167 @@
+/**
+ * @vitest-environment node
+ */
+import type { NextRequest } from 'next/server'
+import { beforeEach, describe, expect, it, vi } from 'vitest'
+
+const { mockClearCache, mockDiscoverServerTools, mockSelect, mockUpdateSet } = vi.hoisted(() => ({
+  mockClearCache: vi.fn(),
+  mockDiscoverServerTools: vi.fn(),
+  mockSelect: vi.fn(),
+  mockUpdateSet: vi.fn(),
+}))
+
+vi.mock('@sim/db', () => ({
+  db: {
+    select: mockSelect,
+    update: vi.fn().mockReturnValue({ set: mockUpdateSet }),
+  },
+}))
+
+vi.mock('@/lib/core/utils/with-route-handler', () => ({
+  withRouteHandler: (handler: unknown) => handler,
+}))
+
+vi.mock('@/lib/mcp/middleware', () => ({
+  withMcpAuth:
+    () =>
+    (
+      handler: (
+        request: NextRequest,
+        context: { userId: string; workspaceId: string; requestId: string },
+        routeContext: { params: Promise<{ id: string }> }
+      ) => Promise<Response>
+    ) =>
+    (request: NextRequest, routeContext: { params: Promise<{ id: string }> }) =>
+      handler(
+        request,
+        { userId: 'user-1', workspaceId: 'workspace-1', requestId: 'request-1' },
+        routeContext
+      ),
+}))
+
+vi.mock('@/lib/mcp/service', () => ({
+  mcpService: {
+    clearCache: mockClearCache,
+    discoverServerTools: mockDiscoverServerTools,
+  },
+}))
+
+import { POST } from '@/app/api/mcp/servers/[id]/refresh/route'
+
+const initialServer = {
+  id: 'server-1',
+  workspaceId: 'workspace-1',
+  name: 'OAuth Server',
+  url: 'https://example.com/mcp',
+  connectionStatus: 'connected',
+  lastError: null,
+  lastConnected: new Date('2026-01-01T00:00:00.000Z'),
+  toolCount: 4,
+  statusConfig: { consecutiveFailures: 0, lastSuccessfulDiscovery: null },
+}
+
+const persistedServer = {
+  ...initialServer,
+  connectionStatus: 'disconnected',
+  lastError: null,
+  toolCount: 0,
+}
+
+function selectRows(rows: unknown[]) {
+  return {
+    from: vi.fn().mockReturnValue({
+      where: vi.fn().mockReturnValue({
+        limit: vi.fn().mockResolvedValue(rows),
+      }),
+    }),
+  }
+}
+
+describe('MCP server refresh route', () => {
+  beforeEach(() => {
+    vi.clearAllMocks()
+    mockSelect.mockReturnValueOnce(selectRows([initialServer]))
+    mockUpdateSet.mockReturnValue({
+      where: vi.fn().mockReturnValue({ returning: vi.fn().mockResolvedValue([persistedServer]) }),
+    })
+  })
+
+  it('preserves the service-persisted OAuth pending status', async () => {
+    mockDiscoverServerTools.mockRejectedValueOnce(new Error('OAuth authorization required'))
+
+    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: null,
+      })
+    )
+    expect(mockUpdateSet).not.toHaveBeenCalledWith(
+      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')
+  })
+})

apps/sim/app/api/mcp/servers/[id]/refresh/route.ts

@@ -2,15 +2,17 @@ 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 { truncate } from '@sim/utils/string'
 import { and, eq, inArray, isNull } from 'drizzle-orm'
 import type { NextRequest } from 'next/server'
 import { mcpServerIdParamsSchema } from '@/lib/api/contracts/mcp'
 import { validationErrorResponse } from '@/lib/api/server'
 import { withRouteHandler } from '@/lib/core/utils/with-route-handler'
 import { withMcpAuth } from '@/lib/mcp/middleware'
 import { mcpService } from '@/lib/mcp/service'
-import type { McpServerStatusConfig, McpTool, McpToolSchema } from '@/lib/mcp/types'
+import type { McpTool, McpToolSchema } from '@/lib/mcp/types'
 import {
+  categorizeError,
   createMcpErrorResponse,
   createMcpSuccessResponse,
   MCP_TOOL_CORE_PARAMS,
@@ -184,17 +186,10 @@ export const POST = withRouteHandler(
           )
         }
 
-        let connectionStatus: 'connected' | 'disconnected' | 'error' = 'error'
-        let toolCount = 0
-        let lastError: string | null = null
         let syncResult: SyncResult = { updatedCount: 0, updatedWorkflowIds: [] }
         let discoveredTools: McpTool[] = []
-
-        const currentStatusConfig: McpServerStatusConfig =
-          (server.statusConfig as McpServerStatusConfig | null) ?? {
-            consecutiveFailures: 0,
-            lastSuccessfulDiscovery: null,
-          }
+        let discoveryError: string | null = null
+        const discoveryStartedAt = new Date()
 
         try {
           discoveredTools = await mcpService.discoverServerTools(
@@ -203,48 +198,62 @@ export const POST = withRouteHandler(
             workspaceId,
             true
           )
-          connectionStatus = 'connected'
-          toolCount = discoveredTools.length
-          logger.info(`[${requestId}] Discovered ${toolCount} tools from server ${serverId}`)
+          logger.info(
+            `[${requestId}] Discovered ${discoveredTools.length} tools from server ${serverId}`
+          )
+        } catch (error) {
+          discoveryError = truncate(categorizeError(error).message, 200, '')
+          logger.warn(`[${requestId}] Failed to connect to server ${serverId}`, {
+            error: discoveryError,
+          })
+        }
 
+        if (discoveryError === null) {
           syncResult = await syncToolSchemasToWorkflows(
             workspaceId,
             serverId,
             discoveredTools,
             requestId,
             { url: server.url ?? undefined, name: server.name ?? undefined }
           )
-        } catch (error) {
-          connectionStatus = 'error'
-          lastError =
-            error instanceof Error
-              ? error.message.split('\n')[0].slice(0, 200)
-              : 'Connection failed'
-          logger.warn(`[${requestId}] Failed to connect to server ${serverId}:`, error)
         }
 
         const now = new Date()
-        const newStatusConfig =
-          connectionStatus === 'connected'
-            ? { consecutiveFailures: 0, lastSuccessfulDiscovery: now.toISOString() }
-            : {
-                consecutiveFailures: currentStatusConfig.consecutiveFailures + 1,
-                lastSuccessfulDiscovery: currentStatusConfig.lastSuccessfulDiscovery,
-              }
 
         const [refreshedServer] = await db
           .update(mcpServers)
           .set({
             lastToolsRefresh: now,
-            connectionStatus,
-            lastError,
-            lastConnected: connectionStatus === 'connected' ? now : server.lastConnected,
-            toolCount,
-            statusConfig: newStatusConfig,
             updatedAt: now,
           })
-          .where(eq(mcpServers.id, serverId))
-          .returning()
+          .where(
+            and(
+              eq(mcpServers.id, serverId),
+              eq(mcpServers.workspaceId, workspaceId),
+              isNull(mcpServers.deletedAt)
+            )
+          )
+          .returning({
+            connectionStatus: mcpServers.connectionStatus,
+            lastConnected: mcpServers.lastConnected,
+            lastError: mcpServers.lastError,
+            toolCount: mcpServers.toolCount,
+          })
+
+        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)