From 71bfcb06a71db45b0ee1825857ec80ad2448d45e Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Tue, 21 Jul 2026 23:10:24 -0700 Subject: [PATCH 1/2] fix(mcp): keep last-known-good tools instead of flashing red on a transient failure MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A connected server with cached tools was turning red 'Failed to discover' on a single transient discovery probe blip, even though the stored status stayed connected with tools — because useMcpToolsQuery dropped React Query's retained data on error, and the row's showDiscoveryError fired over a populated server. Now the aggregate keeps last-known-good tools across a failed refetch, and the row only hard-reds when there are genuinely no tools to show. A persistent failure still surfaces via the stored connectionStatus (gated by MAX_CONSECUTIVE_FAILURES), matching how Claude Code / LibreChat / OpenCode / VS Code handle a transient tools/list failure on a live connection. Validated against those clients' source + the MCP spec before changing; this supersedes the #5829 over-correction that showed the error even when tools existed. --- .../settings/components/mcp/mcp.tsx | 8 +-- apps/sim/hooks/queries/mcp.test.tsx | 49 ++++++++++++++++++- apps/sim/hooks/queries/mcp.ts | 6 ++- 3 files changed, 55 insertions(+), 8 deletions(-) diff --git a/apps/sim/app/workspace/[workspaceId]/settings/components/mcp/mcp.tsx b/apps/sim/app/workspace/[workspaceId]/settings/components/mcp/mcp.tsx index 32cc51cc2f9..5558e23fb35 100644 --- a/apps/sim/app/workspace/[workspaceId]/settings/components/mcp/mcp.tsx +++ b/apps/sim/app/workspace/[workspaceId]/settings/components/mcp/mcp.tsx @@ -97,12 +97,12 @@ function ServerListItem({ server.lastError, server.authType ) - // A live discovery failure whose stored status hasn't caught up yet would otherwise read as - // "0 tools"; surface it directly so a failed row reads as failed, not empty. - // Shown even when cached tools exist: a present discoveryError means the LATEST - // discovery failed, and silently showing the stale tool count would hide that. + // Only hard-red when there are no last-known tools to show. A populated, connected server + // stays on its tool count through a transient probe failure; a persistent failure flips + // `connectionStatus` to error/disconnected and reads as failed through that path instead. const showDiscoveryError = Boolean(discoveryError) && + tools.length === 0 && server.connectionStatus !== 'error' && server.connectionStatus !== 'disconnected' const hasConnectionIssue = diff --git a/apps/sim/hooks/queries/mcp.test.tsx b/apps/sim/hooks/queries/mcp.test.tsx index 18617f45b67..657fdd50154 100644 --- a/apps/sim/hooks/queries/mcp.test.tsx +++ b/apps/sim/hooks/queries/mcp.test.tsx @@ -3,7 +3,7 @@ */ import { act, type ReactNode } from 'react' import { sleep } from '@sim/utils/helpers' -import { QueryClient, QueryClientProvider } from '@tanstack/react-query' +import { QueryClient, QueryClientProvider, useQueryClient } from '@tanstack/react-query' import { createRoot, type Root } from 'react-dom/client' import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' @@ -20,7 +20,12 @@ import { listMcpServersContract, type McpServer, } from '@/lib/api/contracts/mcp' -import { useForceRefreshMcpTools, useMcpServers, useMcpToolsQuery } from '@/hooks/queries/mcp' +import { + mcpKeys, + useForceRefreshMcpTools, + useMcpServers, + useMcpToolsQuery, +} from '@/hooks/queries/mcp' const WORKSPACE_ID = 'workspace-1' @@ -181,6 +186,46 @@ describe('useMcpToolsQuery', () => { unmount() }) + it('keeps last-known-good tools when a later discovery refetch fails', async () => { + let discoverCalls = 0 + mockRequestJson.mockImplementation(async (contract) => { + if (contract === listMcpServersContract) { + return { + success: true, + data: { servers: [server('s1', { authType: 'headers', connectionStatus: 'connected' })] }, + } + } + if (contract === discoverMcpToolsContract) { + discoverCalls++ + if (discoverCalls === 1) { + return { success: true, data: { tools: [{ name: 'tool-a', serverId: 's1' }] } } + } + throw new Error('transient stall') + } + throw new Error('Unexpected MCP request') + }) + + const { getResult, unmount } = renderHookWithClient(() => ({ + tools: useMcpToolsQuery(WORKSPACE_ID), + queryClient: useQueryClient(), + })) + await flush() + expect(getResult().tools.data).toHaveLength(1) + + // Force a refetch that fails; the last successful tools must survive. + await act(async () => { + await getResult().queryClient.invalidateQueries({ + queryKey: mcpKeys.serverToolsList(WORKSPACE_ID, 's1'), + }) + }) + await flush() + + expect(getResult().tools.data).toHaveLength(1) + expect(getResult().tools.toolsStateByServer.get('s1')?.error).toBeInstanceOf(Error) + + unmount() + }) + it('does not force-refresh disconnected OAuth servers', async () => { mockServers([ server('oauth-disconnected', { authType: 'oauth', connectionStatus: 'disconnected' }), diff --git a/apps/sim/hooks/queries/mcp.ts b/apps/sim/hooks/queries/mcp.ts index 2f0f2002080..cab3e5a5f66 100644 --- a/apps/sim/hooks/queries/mcp.ts +++ b/apps/sim/hooks/queries/mcp.ts @@ -188,8 +188,10 @@ export function useMcpToolsQuery(workspaceId: string) { >() for (let index = 0; index < results.length; index++) { const result = results[index] - // Drop stale data from servers whose latest refetch errored. - if (result.data && !result.isError) { + // Keep last-known-good tools when the latest refetch errored (React Query retains `data`); + // the per-server error rides in `toolsStateByServer`. Blanking a populated server on a + // transient discovery failure is what every reference MCP client avoids. + if (result.data) { tools.push(...result.data) hasData = true } From b65d49ce6808708ca89a12146fa75be87fdd23cf Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Tue, 21 Jul 2026 23:16:32 -0700 Subject: [PATCH 2/2] fix(mcp): drop stale tools once a server is persistently failed, keep on transient MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Sharpens the last-known-good behavior with the transient-vs-persistent distinction the reference clients make: keep cached tools through a transient discovery failure (stored status still healthy) so a populated server doesn't blank, but drop them once the stored connectionStatus crosses its failure threshold to error/disconnected — so the shared aggregate the workflow editor consumes stops offering a dead server's stale tool schemas. --- apps/sim/hooks/queries/mcp.test.tsx | 43 +++++++++++++++++++++++++++++ apps/sim/hooks/queries/mcp.ts | 16 +++++++---- 2 files changed, 53 insertions(+), 6 deletions(-) diff --git a/apps/sim/hooks/queries/mcp.test.tsx b/apps/sim/hooks/queries/mcp.test.tsx index 657fdd50154..ad6c120f516 100644 --- a/apps/sim/hooks/queries/mcp.test.tsx +++ b/apps/sim/hooks/queries/mcp.test.tsx @@ -226,6 +226,49 @@ describe('useMcpToolsQuery', () => { unmount() }) + it('drops stale tools once a non-OAuth server is persistently failed', async () => { + let discoverCalls = 0 + let listCalls = 0 + mockRequestJson.mockImplementation(async (contract) => { + if (contract === listMcpServersContract) { + listCalls++ + // Healthy on first read, error state after the failed refetch invalidates the list. + const connectionStatus = listCalls === 1 ? 'connected' : 'error' + return { + success: true, + data: { servers: [server('s1', { authType: 'headers', connectionStatus })] }, + } + } + if (contract === discoverMcpToolsContract) { + discoverCalls++ + if (discoverCalls === 1) { + return { success: true, data: { tools: [{ name: 'tool-a', serverId: 's1' }] } } + } + throw new Error('persistent failure') + } + throw new Error('Unexpected MCP request') + }) + + const { getResult, unmount } = renderHookWithClient(() => ({ + tools: useMcpToolsQuery(WORKSPACE_ID), + queryClient: useQueryClient(), + })) + await flush() + expect(getResult().tools.data).toHaveLength(1) + + await act(async () => { + await getResult().queryClient.invalidateQueries({ + queryKey: mcpKeys.serverToolsList(WORKSPACE_ID, 's1'), + }) + }) + await flush() + + // Stored status is now 'error' → the dead server's stale tools are dropped from the aggregate. + expect(getResult().tools.data).toHaveLength(0) + + unmount() + }) + it('does not force-refresh disconnected OAuth servers', async () => { mockServers([ server('oauth-disconnected', { authType: 'oauth', connectionStatus: 'disconnected' }), diff --git a/apps/sim/hooks/queries/mcp.ts b/apps/sim/hooks/queries/mcp.ts index cab3e5a5f66..8daab301235 100644 --- a/apps/sim/hooks/queries/mcp.ts +++ b/apps/sim/hooks/queries/mcp.ts @@ -182,23 +182,27 @@ export function useMcpToolsQuery(workspaceId: string) { let hasData = false let anyServerLoading = false let firstError: Error | null = null + const statusById = new Map(servers?.map((s) => [s.id, s.connectionStatus])) const toolsStateByServer = new Map< string, { isLoading: boolean; isFetching: boolean; error: Error | null } >() for (let index = 0; index < results.length; index++) { const result = results[index] - // Keep last-known-good tools when the latest refetch errored (React Query retains `data`); - // the per-server error rides in `toolsStateByServer`. Blanking a populated server on a - // transient discovery failure is what every reference MCP client avoids. - if (result.data) { + const serverId = serverIds[index] + const status = serverId ? statusById.get(serverId) : undefined + const persistentlyFailed = status === 'error' || status === 'disconnected' + // Keep last-known-good tools through a transient failure (React Query retains `data`, the + // stored status is still healthy) so a populated server doesn't blank — but drop them once + // the stored status crosses its failure threshold, so the workflow editor stops offering a + // dead server's stale tools. Matches how reference MCP clients treat transient vs. closed. + if (result.data && (!result.isError || !persistentlyFailed)) { tools.push(...result.data) hasData = true } if (result.isLoading) anyServerLoading = true if (!firstError && result.error instanceof Error) firstError = result.error - const serverId = serverIds[index] if (serverId) { toolsStateByServer.set(serverId, { isLoading: result.isLoading, @@ -215,7 +219,7 @@ export function useMcpToolsQuery(workspaceId: string) { error: hasData ? null : firstError, toolsStateByServer, } - }, [results, serversLoading, serverIds]) + }, [results, serversLoading, serverIds, servers]) } export function useForceRefreshMcpTools() {