v0.7.39: mcp oauth hardening, chat perf improvements

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

@@ -0,0 +1,200 @@
+/**
+ * @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')
+  })
+
+  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,16 +1,18 @@
 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'
 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,71 @@ export const POST = withRouteHandler(
             workspaceId,
             true
           )
-          connectionStatus = 'connected'
-          toolCount = discoveredTools.length
-          logger.info(`[${requestId}] Discovered ${toolCount} tools from server ${serverId}`)
-
-          syncResult = await syncToolSchemasToWorkflows(
-            workspaceId,
-            serverId,
-            discoveredTools,
-            requestId,
-            { url: server.url ?? undefined, name: server.name ?? undefined }
+          logger.info(
+            `[${requestId}] Discovered ${discoveredTools.length} tools from server ${serverId}`
           )
         } 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)
+          discoveryError = truncate(categorizeError(error).message, 200, '')
+          logger.warn(`[${requestId}] Failed to connect to server ${serverId}`, {
+            error: discoveryError,
+          })
+        }
+
+        if (discoveryError === null) {
+          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()
-        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)