fix(mcp): address bugbot/greptile review on per-server tool discovery

- Workspace-scoped `mcpKeys.serverToolsWorkspace(workspaceId)` prefix for bulk
  invalidations (create-server, refresh-all, SSE workspace fallback, OAuth
  fallback). The previous `mcpKeys.serverTools()` prefix was global and
  invalidated every workspace's tools cache.
- `useMcpToolsQuery` folds `useMcpServers().isLoading` into the aggregate
  `isLoading` so mounting no longer flashes an "empty tools" state during the
  servers-list fetch. Aggregate `error` is suppressed when any per-server
  query already returned data so one slow server can't blank out the others.
- `useForceRefreshMcpTools` invalidates the per-server query keys of servers
  whose force refresh failed, so stale tools don't linger.
- `DiscoveryOutcome` error variant carries the original error, restoring the
  OAuth-exemption check that `getErrorMessage(...)` previously erased.
- `discoverServerTools(userId, serverId, workspaceId, forceRefresh = false)`
  now consults the positive + negative cache by default. Per-server React
  Query refetches hit the cache instead of re-paying the listTools timeout;
  callers that explicitly bypass cache (refresh route, OAuth callback, bulk
  POST refresh) pass `forceRefresh: true`. Negative-cache hits throw a typed
  `McpConnectionError` so the route layer can surface a fast 503.

Author: waleedlatif1

apps/sim/app/api/mcp/oauth/callback/route.ts

@@ -168,8 +168,10 @@ export const GET = withRouteHandler(async (request: NextRequest) => {
 
     try {
       // discoverServerTools writes the result to this server's cache so the UI's
-      // immediate refetch hits it instead of re-fetching live.
-      await mcpService.discoverServerTools(session.user.id, server.id, server.workspaceId)
+      // immediate refetch hits it instead of re-fetching live. Bypass any
+      // stale positive/negative cache from before re-auth so we actually
+      // verify the new tokens.
+      await mcpService.discoverServerTools(session.user.id, server.id, server.workspaceId, true)
     } catch (e) {
       logger.warn('Post-auth tools refresh failed', toError(e).message)
     }

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

@@ -197,7 +197,12 @@ export const POST = withRouteHandler(
           }
 
         try {
-          discoveredTools = await mcpService.discoverServerTools(userId, serverId, workspaceId)
+          discoveredTools = await mcpService.discoverServerTools(
+            userId,
+            serverId,
+            workspaceId,
+            true
+          )
           connectionStatus = 'connected'
           toolCount = discoveredTools.length
           logger.info(`[${requestId}] Discovered ${toolCount} tools from server ${serverId}`)

apps/sim/app/api/mcp/tools/discover/route.ts

@@ -28,7 +28,7 @@ export const GET = withRouteHandler(
       logger.info(`[${requestId}] Discovering MCP tools`, { serverId, workspaceId, forceRefresh })
 
       const tools = serverId
-        ? await mcpService.discoverServerTools(userId, serverId, workspaceId)
+        ? await mcpService.discoverServerTools(userId, serverId, workspaceId, forceRefresh)
         : await mcpService.discoverTools(userId, workspaceId, forceRefresh)
 
       const byServer: Record<string, number> = {}
@@ -76,7 +76,7 @@ export const POST = withRouteHandler(
 
       const results = await Promise.allSettled(
         serverIds.map(async (serverId: string) => {
-          const tools = await mcpService.discoverServerTools(userId, serverId, workspaceId)
+          const tools = await mcpService.discoverServerTools(userId, serverId, workspaceId, true)
           return { serverId, toolCount: tools.length }
         })
       )

apps/sim/hooks/mcp/use-mcp-oauth-popup.ts

@@ -87,7 +87,9 @@ export function useMcpOauthPopup({ workspaceId }: UseMcpOauthPopupProps) {
             queryKey: mcpKeys.serverToolsList(workspaceId, data.serverId),
           })
         } else {
-          queryClient.invalidateQueries({ queryKey: mcpKeys.serverTools() })
+          queryClient.invalidateQueries({
+            queryKey: mcpKeys.serverToolsWorkspace(workspaceId),
+          })
         }
         queryClient.invalidateQueries({ queryKey: mcpKeys.storedToolsList(workspaceId) })
         toast.success('Server authorized')

apps/sim/hooks/mcp/use-mcp-tools.ts

@@ -65,7 +65,7 @@ export function useMcpTools(workspaceId: string): UseMcpToolsResult {
       logger.info('Refreshing MCP tools', { forceRefresh, workspaceId })
 
       await queryClient.invalidateQueries({
-        queryKey: mcpKeys.serverTools(),
+        queryKey: mcpKeys.serverToolsWorkspace(workspaceId),
         refetchType: forceRefresh ? 'active' : 'all',
       })
     },

apps/sim/hooks/queries/mcp.ts

@@ -48,8 +48,13 @@ export const mcpKeys = {
   tools: () => [...mcpKeys.all, 'tools'] as const,
   toolsList: (workspaceId?: string) => [...mcpKeys.tools(), workspaceId ?? ''] as const,
   serverTools: () => [...mcpKeys.all, 'serverTools'] as const,
+  // Workspace-scoped prefix so bulk invalidations (refresh-all, create-server)
+  // only touch the current workspace's per-server entries — not every
+  // workspace cached in the QueryClient.
+  serverToolsWorkspace: (workspaceId?: string) =>
+    [...mcpKeys.serverTools(), workspaceId ?? ''] as const,
   serverToolsList: (workspaceId?: string, serverId?: string) =>
-    [...mcpKeys.serverTools(), workspaceId ?? '', serverId ?? ''] as const,
+    [...mcpKeys.serverToolsWorkspace(workspaceId), serverId ?? ''] as const,
   storedTools: () => [...mcpKeys.all, 'storedTools'] as const,
   storedToolsList: (workspaceId?: string) => [...mcpKeys.storedTools(), workspaceId ?? ''] as const,
   allowedDomains: () => [...mcpKeys.all, 'allowedDomains'] as const,
@@ -149,7 +154,7 @@ export function useMcpServerTools(workspaceId: string, serverId?: string) {
  * fast servers populate immediately.
  */
 export function useMcpToolsQuery(workspaceId: string) {
-  const { data: servers } = useMcpServers(workspaceId)
+  const { data: servers, isLoading: serversLoading } = useMcpServers(workspaceId)
 
   const serverIds = useMemo(() => (servers ? servers.map((s) => s.id).sort() : []), [servers])
 
@@ -168,27 +173,31 @@ export function useMcpToolsQuery(workspaceId: string) {
   return useMemo(() => {
     const tools: McpTool[] = []
     let hasData = false
-    let isLoading = false
+    let anyServerLoading = false
     let firstError: Error | null = null
     for (const result of results) {
       if (result.data) {
         tools.push(...result.data)
         hasData = true
       }
-      if (result.isLoading) isLoading = true
+      if (result.isLoading) anyServerLoading = true
       if (!firstError && result.error instanceof Error) firstError = result.error
     }
     return {
       data: tools,
-      // Match the prior semantics: "loading" means we have nothing to show yet.
-      // Once any server has returned, the UI can render that and let slow
-      // neighbors fill in incrementally without keeping the spinner up.
-      isLoading: isLoading && !hasData,
-      isFetching: results.some((r) => r.isFetching),
-      error: firstError,
+      // "Loading" means we have nothing to render yet — including the gap
+      // between mount and the server-list arriving (otherwise consumers flash
+      // an empty state). Once any per-server query has returned, we drop the
+      // spinner and let slow neighbors fill in incrementally.
+      isLoading: (serversLoading || anyServerLoading) && !hasData,
+      isFetching: serversLoading || results.some((r) => r.isFetching),
+      // Only surface an aggregate error when no server returned anything —
+      // one slow/dead server should not blank out tools from the others.
+      // Per-server errors are still exposed via `perServer`.
+      error: hasData ? null : firstError,
       perServer: results,
     }
-  }, [results])
+  }, [results, serversLoading])
 }
 
 export function useForceRefreshMcpTools() {
@@ -207,13 +216,27 @@ export function useForceRefreshMcpTools() {
           return tools
         })
       )
+      // Invalidate the per-server keys of any server that failed so stale tools
+      // don't linger after a force-refresh — React Query will refetch and the
+      // negative cache (set server-side) will make that retry fail fast.
+      results.forEach((result, index) => {
+        if (result.status === 'rejected') {
+          const failedServer = servers[index]
+          if (failedServer) {
+            queryClient.invalidateQueries({
+              queryKey: mcpKeys.serverToolsList(workspaceId, failedServer.id),
+            })
+          }
+        }
+      })
       return results
         .filter((r): r is PromiseFulfilledResult<McpTool[]> => r.status === 'fulfilled')
         .flatMap((r) => r.value)
     },
     onSettled: (_data, _error, workspaceId) => {
-      // Per-server caches were already populated inside `mutationFn` via
-      // `setQueryData`, so we only need to refresh the dependent views.
+      // Successful per-server caches were already populated inside `mutationFn`
+      // via `setQueryData`; failed ones were invalidated above. Just refresh
+      // the dependent views.
       queryClient.invalidateQueries({ queryKey: mcpKeys.serversList(workspaceId) })
       queryClient.invalidateQueries({ queryKey: mcpKeys.storedToolsList(workspaceId) })
     },
@@ -263,7 +286,9 @@ export function useCreateMcpServer() {
     },
     onSettled: (_data, _error, variables) => {
       queryClient.invalidateQueries({ queryKey: mcpKeys.serversList(variables.workspaceId) })
-      queryClient.invalidateQueries({ queryKey: mcpKeys.serverTools() })
+      queryClient.invalidateQueries({
+        queryKey: mcpKeys.serverToolsWorkspace(variables.workspaceId),
+      })
     },
   })
 }
@@ -486,7 +511,7 @@ export function useMcpToolsEvents(workspaceId: string) {
           queryKey: mcpKeys.serverToolsList(workspaceId, serverId),
         })
       } else {
-        queryClient.invalidateQueries({ queryKey: mcpKeys.serverTools() })
+        queryClient.invalidateQueries({ queryKey: mcpKeys.serverToolsWorkspace(workspaceId) })
       }
       queryClient.invalidateQueries({ queryKey: mcpKeys.serversList(workspaceId) })
       queryClient.invalidateQueries({ queryKey: mcpKeys.storedToolsList(workspaceId) })

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

@@ -291,11 +291,18 @@ describe('McpService.discoverTools per-server caching', () => {
       'Request timed out'
     )
 
-    // Following the failure, the negative cache would short-circuit discoverTools.
-    // Reconnecting the server (e.g. after OAuth) should bring it back to live.
+    // After the failure the negative cache is set, so the next default call
+    // short-circuits without re-paying the listTools timeout.
     mockListTools.mockClear()
+    await expect(mcpService.discoverServerTools(USER_ID, 'mcp-a', WORKSPACE_ID)).rejects.toThrow(
+      'cooldown'
+    )
+    expect(mockListTools).not.toHaveBeenCalled()
+
+    // Reconnecting via the explicit-refresh path (refresh button / OAuth
+    // callback) bypasses both caches and brings the server back to live.
     mockListTools.mockResolvedValueOnce([tool('a1', 'mcp-a')])
-    const tools = await mcpService.discoverServerTools(USER_ID, 'mcp-a', WORKSPACE_ID)
+    const tools = await mcpService.discoverServerTools(USER_ID, 'mcp-a', WORKSPACE_ID, true)
     expect(tools.map((t) => t.name)).toEqual(['a1'])
 
     // discoverTools now sees the cleared negative cache + primed positive cache.

apps/sim/lib/mcp/service.ts

@@ -32,6 +32,7 @@ import {
   type McpCacheStorageAdapter,
 } from '@/lib/mcp/storage'
 import {
+  McpConnectionError,
   McpOauthAuthorizationRequiredError,
   type McpServerConfig,
   type McpServerStatusConfig,
@@ -69,7 +70,9 @@ type DiscoveryOutcome =
     }
   | { kind: 'oauth-pending' }
   | { kind: 'unhealthy' }
-  | { kind: 'error'; message: string }
+  // Carries the original error so `markServerUnhealthy` can apply its
+  // `instanceof` exemption — `getErrorMessage(...)` erases type info.
+  | { kind: 'error'; message: string; originalError: unknown }
 
 class McpService {
   private cacheAdapter: McpCacheStorageAdapter
@@ -519,7 +522,11 @@ class McpService {
             ) {
               return { kind: 'oauth-pending' }
             }
-            return { kind: 'error', message: getErrorMessage(error, 'Unknown error') }
+            return {
+              kind: 'error',
+              message: getErrorMessage(error, 'Unknown error'),
+              originalError: error,
+            }
           }
         })
       )
@@ -593,7 +600,7 @@ class McpService {
         )
         deferredSideEffects.push(
           this.updateServerStatus(server.id, workspaceId, false, outcome.message),
-          this.markServerUnhealthy(workspaceId, server.id, new Error(outcome.message))
+          this.markServerUnhealthy(workspaceId, server.id, outcome.originalError)
         )
       })
 
@@ -630,6 +637,12 @@ class McpService {
    * Discover tools from a specific server with retry logic for session errors.
    * Retries once on session-related errors (400, 404, session ID issues).
    *
+   * By default consults the positive + negative cache before going live, so
+   * a React Query refetch for a healthy server is served from cache and a
+   * recently-failed server short-circuits instead of re-paying the listTools
+   * timeout. Pass `forceRefresh: true` from explicit user-initiated paths
+   * (refresh button, OAuth callback) to bypass both caches.
+   *
    * Concurrent callers for the same `(workspaceId, serverId, userId)` share a
    * single in-flight upstream request — important when an OAuth callback
    * primes the cache while a UI refetch races against it, or when multiple
@@ -638,13 +651,19 @@ class McpService {
   async discoverServerTools(
     userId: string,
     serverId: string,
-    workspaceId: string
+    workspaceId: string,
+    forceRefresh = false
   ): Promise<McpTool[]> {
-    const inflightKey = `${workspaceId}:${serverId}:${userId}`
+    const inflightKey = `${workspaceId}:${serverId}:${userId}:${forceRefresh ? 'force' : 'cache'}`
     const existing = this.inflightServerDiscovery.get(inflightKey)
     if (existing) return existing
 
-    const promise = this.discoverServerToolsImpl(userId, serverId, workspaceId).finally(() => {
+    const promise = this.discoverServerToolsImpl(
+      userId,
+      serverId,
+      workspaceId,
+      forceRefresh
+    ).finally(() => {
       this.inflightServerDiscovery.delete(inflightKey)
     })
     this.inflightServerDiscovery.set(inflightKey, promise)
@@ -654,11 +673,35 @@ class McpService {
   private async discoverServerToolsImpl(
     userId: string,
     serverId: string,
-    workspaceId: string
+    workspaceId: string,
+    forceRefresh: boolean
   ): Promise<McpTool[]> {
     const requestId = generateRequestId()
     const maxRetries = 2
 
+    if (!forceRefresh) {
+      // Positive cache hit — same payload the aggregate fan-out would return,
+      // no upstream traffic.
+      try {
+        const cached = await this.cacheAdapter.get(serverCacheKey(workspaceId, serverId))
+        if (cached) {
+          logger.debug(`[${requestId}] Cache hit for server ${serverId}`)
+          return cached.tools
+        }
+      } catch (error) {
+        logger.warn(`[${requestId}] Cache read failed for server ${serverId}:`, error)
+      }
+      // Negative cache hit — fail fast with a typed error the caller can map
+      // to a 503-style response, rather than waiting out the listTools timeout.
+      if (await this.isServerUnhealthy(workspaceId, serverId)) {
+        logger.info(`[${requestId}] Skipping recently-failed server ${serverId} (negative-cache)`)
+        throw new McpConnectionError(
+          'Server recently failed and is in cooldown — try again shortly.',
+          serverId
+        )
+      }
+    }
+
     for (let attempt = 0; attempt < maxRetries; attempt++) {
       try {
         logger.info(