Skip to content

Commit 71bfcb0

Browse files
committed
fix(mcp): keep last-known-good tools instead of flashing red on a transient failure
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.
1 parent 93fbf58 commit 71bfcb0

3 files changed

Lines changed: 55 additions & 8 deletions

File tree

apps/sim/app/workspace/[workspaceId]/settings/components/mcp/mcp.tsx

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -97,12 +97,12 @@ function ServerListItem({
9797
server.lastError,
9898
server.authType
9999
)
100-
// A live discovery failure whose stored status hasn't caught up yet would otherwise read as
101-
// "0 tools"; surface it directly so a failed row reads as failed, not empty.
102-
// Shown even when cached tools exist: a present discoveryError means the LATEST
103-
// discovery failed, and silently showing the stale tool count would hide that.
100+
// Only hard-red when there are no last-known tools to show. A populated, connected server
101+
// stays on its tool count through a transient probe failure; a persistent failure flips
102+
// `connectionStatus` to error/disconnected and reads as failed through that path instead.
104103
const showDiscoveryError =
105104
Boolean(discoveryError) &&
105+
tools.length === 0 &&
106106
server.connectionStatus !== 'error' &&
107107
server.connectionStatus !== 'disconnected'
108108
const hasConnectionIssue =

apps/sim/hooks/queries/mcp.test.tsx

Lines changed: 47 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@
33
*/
44
import { act, type ReactNode } from 'react'
55
import { sleep } from '@sim/utils/helpers'
6-
import { QueryClient, QueryClientProvider } from '@tanstack/react-query'
6+
import { QueryClient, QueryClientProvider, useQueryClient } from '@tanstack/react-query'
77
import { createRoot, type Root } from 'react-dom/client'
88
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
99

@@ -20,7 +20,12 @@ import {
2020
listMcpServersContract,
2121
type McpServer,
2222
} from '@/lib/api/contracts/mcp'
23-
import { useForceRefreshMcpTools, useMcpServers, useMcpToolsQuery } from '@/hooks/queries/mcp'
23+
import {
24+
mcpKeys,
25+
useForceRefreshMcpTools,
26+
useMcpServers,
27+
useMcpToolsQuery,
28+
} from '@/hooks/queries/mcp'
2429

2530
const WORKSPACE_ID = 'workspace-1'
2631

@@ -181,6 +186,46 @@ describe('useMcpToolsQuery', () => {
181186
unmount()
182187
})
183188

189+
it('keeps last-known-good tools when a later discovery refetch fails', async () => {
190+
let discoverCalls = 0
191+
mockRequestJson.mockImplementation(async (contract) => {
192+
if (contract === listMcpServersContract) {
193+
return {
194+
success: true,
195+
data: { servers: [server('s1', { authType: 'headers', connectionStatus: 'connected' })] },
196+
}
197+
}
198+
if (contract === discoverMcpToolsContract) {
199+
discoverCalls++
200+
if (discoverCalls === 1) {
201+
return { success: true, data: { tools: [{ name: 'tool-a', serverId: 's1' }] } }
202+
}
203+
throw new Error('transient stall')
204+
}
205+
throw new Error('Unexpected MCP request')
206+
})
207+
208+
const { getResult, unmount } = renderHookWithClient(() => ({
209+
tools: useMcpToolsQuery(WORKSPACE_ID),
210+
queryClient: useQueryClient(),
211+
}))
212+
await flush()
213+
expect(getResult().tools.data).toHaveLength(1)
214+
215+
// Force a refetch that fails; the last successful tools must survive.
216+
await act(async () => {
217+
await getResult().queryClient.invalidateQueries({
218+
queryKey: mcpKeys.serverToolsList(WORKSPACE_ID, 's1'),
219+
})
220+
})
221+
await flush()
222+
223+
expect(getResult().tools.data).toHaveLength(1)
224+
expect(getResult().tools.toolsStateByServer.get('s1')?.error).toBeInstanceOf(Error)
225+
226+
unmount()
227+
})
228+
184229
it('does not force-refresh disconnected OAuth servers', async () => {
185230
mockServers([
186231
server('oauth-disconnected', { authType: 'oauth', connectionStatus: 'disconnected' }),

apps/sim/hooks/queries/mcp.ts

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -188,8 +188,10 @@ export function useMcpToolsQuery(workspaceId: string) {
188188
>()
189189
for (let index = 0; index < results.length; index++) {
190190
const result = results[index]
191-
// Drop stale data from servers whose latest refetch errored.
192-
if (result.data && !result.isError) {
191+
// Keep last-known-good tools when the latest refetch errored (React Query retains `data`);
192+
// the per-server error rides in `toolsStateByServer`. Blanking a populated server on a
193+
// transient discovery failure is what every reference MCP client avoids.
194+
if (result.data) {
193195
tools.push(...result.data)
194196
hasData = true
195197
}

0 commit comments

Comments
 (0)