Address PR review feedback (#5754)

- Fall back safely when ordered MCP cache invalidation is unavailable
- Surface sanitized concrete refresh errors without masking OAuth-pending state

Author: j15z

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

@@ -3,6 +3,7 @@
  */
 import type { NextRequest } from 'next/server'
 import { beforeEach, describe, expect, it, vi } from 'vitest'
+import { McpOauthAuthorizationRequiredError } from '@/lib/mcp/types'
 
 const { mockClearCache, mockDiscoverServerTools, mockSelect, mockUpdate, mockUpdateSet } =
   vi.hoisted(() => ({
@@ -56,6 +57,7 @@ const initialServer = {
   workspaceId: 'workspace-1',
   name: 'OAuth Server',
   url: 'https://example.com/mcp',
+  authType: 'oauth',
   connectionStatus: 'connected',
   lastError: null,
   lastConnected: new Date('2026-01-01T00:00:00.000Z'),
@@ -93,7 +95,9 @@ describe('MCP server refresh route', () => {
   })
 
   it('preserves the service-persisted OAuth pending status', async () => {
-    mockDiscoverServerTools.mockRejectedValueOnce(new Error('OAuth authorization required'))
+    mockDiscoverServerTools.mockRejectedValueOnce(
+      new McpOauthAuthorizationRequiredError('server-1', 'OAuth Server')
+    )
 
     const request = new Request('http://localhost/api/mcp/servers/server-1/refresh', {
       method: 'POST',
@@ -112,6 +116,25 @@ describe('MCP server refresh route', () => {
     )
   })
 
+  it('reports a sanitized discovery timeout when persistence leaves disconnected without an error', async () => {
+    mockDiscoverServerTools.mockRejectedValueOnce(new Error('upstream request timeout'))
+
+    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: 'Request timed out',
+        toolCount: 0,
+        workflowsUpdated: 0,
+      })
+    )
+  })
+
   it('reports the discovery failure when status persistence leaves a stale connected row', async () => {
     const reflectedSecret = 'Bearer reflected-static-token'
     mockDiscoverServerTools.mockRejectedValueOnce(

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

@@ -1,3 +1,4 @@
+import { UnauthorizedError } from '@modelcontextprotocol/sdk/client/auth.js'
 import { db } from '@sim/db'
 import { mcpServers, workflow, workflowBlocks } from '@sim/db/schema'
 import { createLogger } from '@sim/logger'
@@ -10,7 +11,11 @@ import { validationErrorResponse } from '@/lib/api/server'
 import { withRouteHandler } from '@/lib/core/utils/with-route-handler'
 import { withMcpAuth } from '@/lib/mcp/middleware'
 import { type McpServerDiscoveryState, mcpService } from '@/lib/mcp/service'
-import type { McpTool, McpToolSchema } from '@/lib/mcp/types'
+import {
+  McpOauthAuthorizationRequiredError,
+  type McpTool,
+  type McpToolSchema,
+} from '@/lib/mcp/types'
 import {
   categorizeError,
   createMcpErrorResponse,
@@ -190,6 +195,7 @@ export const POST = withRouteHandler(
         let discoveredTools: McpTool[] = []
         let discoveryState: McpServerDiscoveryState | null = null
         let discoveryError: string | null = null
+        let oauthAuthorizationRequired = false
 
         try {
           const discovery = await mcpService.discoverServerToolsWithMetadata(
@@ -204,6 +210,10 @@ export const POST = withRouteHandler(
             `[${requestId}] Discovered ${discoveredTools.length} tools from server ${serverId}`
           )
         } catch (error) {
+          oauthAuthorizationRequired =
+            server.authType === 'oauth' &&
+            (error instanceof McpOauthAuthorizationRequiredError ||
+              error instanceof UnauthorizedError)
           discoveryError = truncate(categorizeError(error).message, 200, '')
           logger.warn(`[${requestId}] Failed to connect to server ${serverId}`, {
             error: discoveryError,
@@ -282,6 +292,14 @@ export const POST = withRouteHandler(
             lastError = discoveryError
             toolCount = 0
           }
+        } else if (
+          discoveryError !== null &&
+          connectionStatus === 'disconnected' &&
+          lastError === null &&
+          !oauthAuthorizationRequired
+        ) {
+          lastError = discoveryError
+          toolCount = 0
         }
 
         return createMcpSuccessResponse({

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

@@ -737,6 +737,49 @@ describe('McpService.discoverTools per-server caching', () => {
     expect(mockCacheAdapter.delete).toHaveBeenCalledWith(failureKey)
   })
 
+  it('best-effort deletes both cache keys and publishes the barrier when ordered invalidation fails', async () => {
+    const serverKey = `workspace:${WORKSPACE_ID}:server:mcp-a`
+    const failureKey = `${serverKey}:failure`
+    mockGetWorkspaceServersRows.mockResolvedValue([dbRow('mcp-a', 'A')])
+    cacheStore.set(serverKey, {
+      tools: [tool('stale-tool', 'mcp-a')],
+      expiry: Date.now() + 60_000,
+    })
+    cacheStore.set(failureKey, { tools: [], expiry: Date.now() + 60_000 })
+    mockCacheAdapter.applyMutationIfCurrent.mockRejectedValueOnce(
+      new Error('atomic invalidation unavailable')
+    )
+
+    await mcpService.clearCache(WORKSPACE_ID)
+
+    const mutationId = mockCacheAdapter.applyMutationIfCurrent.mock.calls[0][1]
+    expect(cacheStore.has(serverKey)).toBe(false)
+    expect(cacheStore.has(failureKey)).toBe(false)
+    expect(mockCacheAdapter.delete).toHaveBeenCalledWith(serverKey)
+    expect(mockCacheAdapter.delete).toHaveBeenCalledWith(failureKey)
+    expect(mockUpdateSet).toHaveBeenCalledWith({
+      toolCount: 0,
+      lastToolsRefresh: new Date(mutationId),
+    })
+  })
+
+  it('preserves a newer cache winner when invalidation is superseded', async () => {
+    const serverKey = `workspace:${WORKSPACE_ID}:server:mcp-a`
+    const failureKey = `${serverKey}:failure`
+    const winner = tool('winner-tool', 'mcp-a')
+    mockGetWorkspaceServersRows.mockResolvedValue([dbRow('mcp-a', 'A')])
+    cacheStore.set(serverKey, { tools: [winner], expiry: Date.now() + 60_000 })
+    cacheStore.set(failureKey, { tools: [], expiry: Date.now() + 60_000 })
+    mockCacheAdapter.applyMutationIfCurrent.mockResolvedValueOnce(false)
+
+    await mcpService.clearCache(WORKSPACE_ID)
+
+    expect(cacheStore.get(serverKey)?.tools).toEqual([winner])
+    expect(cacheStore.has(failureKey)).toBe(true)
+    expect(mockCacheAdapter.delete).not.toHaveBeenCalled()
+    expect(mockUpdateSet).not.toHaveBeenCalled()
+  })
+
   it('does not negative-cache OAuth-required errors', async () => {
     mockGetWorkspaceServersRows.mockResolvedValue([dbRow('mcp-a', 'A')])
     mockListTools.mockRejectedValueOnce(new McpOauthAuthorizationRequiredError('mcp-a', 'A'))

apps/sim/lib/mcp/service.ts

@@ -734,7 +734,10 @@ class McpService {
       null,
       [serverCacheKey(workspaceId, serverId), failureCacheKey(workspaceId, serverId)]
     )
-    if (cacheApplied !== 'applied') return
+    if (cacheApplied === 'superseded') return
+    if (cacheApplied === 'unavailable') {
+      await this.bestEffortInvalidateServerCache(workspaceId, serverId)
+    }
 
     await this.markServerCacheInvalidated(serverId, workspaceId, new Date(mutation.id))
   }