Skip to content

Commit 5e975cd

Browse files
committed
Align MCP refresh responses with publication order (#5754)
Expose discovery publication metadata to refresh callers, report live tools during cache degradation, and compare lastToolsRefresh mutation tokens instead of wall clocks when preserving a newer success.
1 parent 0a12ca9 commit 5e975cd

4 files changed

Lines changed: 104 additions & 24 deletions

File tree

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

Lines changed: 45 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,7 @@ vi.mock('@/lib/mcp/middleware', () => ({
4343
vi.mock('@/lib/mcp/service', () => ({
4444
mcpService: {
4545
clearCache: mockClearCache,
46-
discoverServerTools: mockDiscoverServerTools,
46+
discoverServerToolsWithMetadata: mockDiscoverServerTools,
4747
},
4848
}))
4949

@@ -57,6 +57,7 @@ const initialServer = {
5757
connectionStatus: 'connected',
5858
lastError: null,
5959
lastConnected: new Date('2026-01-01T00:00:00.000Z'),
60+
lastToolsRefresh: new Date('2026-01-01T00:00:00.000Z'),
6061
toolCount: 4,
6162
statusConfig: { consecutiveFailures: 0, lastSuccessfulDiscovery: null },
6263
}
@@ -139,6 +140,7 @@ describe('MCP server refresh route', () => {
139140
const newerSuccessfulServer = {
140141
...initialServer,
141142
lastConnected: new Date(Date.now() + 60_000),
143+
lastToolsRefresh: new Date(Date.now() + 60_000),
142144
toolCount: 7,
143145
}
144146
mockSelect.mockReturnValueOnce(selectRows([newerSuccessfulServer]))
@@ -161,15 +163,18 @@ describe('MCP server refresh route', () => {
161163
})
162164

163165
it('does not 500 when workflow sync fails after a successful discovery', async () => {
164-
mockDiscoverServerTools.mockResolvedValueOnce([
165-
{
166-
name: 'search',
167-
description: 'Search tool',
168-
inputSchema: {},
169-
serverId: 'server-1',
170-
serverName: 'OAuth Server',
171-
},
172-
])
166+
mockDiscoverServerTools.mockResolvedValueOnce({
167+
tools: [
168+
{
169+
name: 'search',
170+
description: 'Search tool',
171+
inputSchema: {},
172+
serverId: 'server-1',
173+
serverName: 'OAuth Server',
174+
},
175+
],
176+
state: 'published',
177+
})
173178
// The route's server lookup consumes the first select (beforeEach). The sync's
174179
// workflow select is left unmocked, so it throws — exercising the guard that
175180
// keeps a secondary sync failure from turning a successful refresh into a 500.
@@ -190,4 +195,34 @@ describe('MCP server refresh route', () => {
190195
})
191196
)
192197
})
198+
199+
it('reports live tools when cache degradation prevents status publication', async () => {
200+
mockDiscoverServerTools.mockResolvedValueOnce({
201+
tools: [
202+
{
203+
name: 'search',
204+
description: 'Search tool',
205+
inputSchema: {},
206+
serverId: 'server-1',
207+
serverName: 'OAuth Server',
208+
},
209+
],
210+
state: 'unavailable',
211+
})
212+
mockSelect.mockReturnValueOnce(selectRows([persistedServer]))
213+
214+
const request = new Request('http://localhost/api/mcp/servers/server-1/refresh', {
215+
method: 'POST',
216+
}) as NextRequest
217+
const response = await POST(request, { params: Promise.resolve({ id: 'server-1' }) })
218+
const body = await response.json()
219+
220+
expect(body.data).toEqual(
221+
expect.objectContaining({
222+
status: 'connected',
223+
error: null,
224+
toolCount: 1,
225+
})
226+
)
227+
})
193228
})

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

Lines changed: 18 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ import { mcpServerIdParamsSchema } from '@/lib/api/contracts/mcp'
99
import { validationErrorResponse } from '@/lib/api/server'
1010
import { withRouteHandler } from '@/lib/core/utils/with-route-handler'
1111
import { withMcpAuth } from '@/lib/mcp/middleware'
12-
import { mcpService } from '@/lib/mcp/service'
12+
import { type McpServerDiscoveryState, mcpService } from '@/lib/mcp/service'
1313
import type { McpTool, McpToolSchema } from '@/lib/mcp/types'
1414
import {
1515
categorizeError,
@@ -188,16 +188,18 @@ export const POST = withRouteHandler(
188188

189189
let syncResult: SyncResult = { updatedCount: 0, updatedWorkflowIds: [] }
190190
let discoveredTools: McpTool[] = []
191+
let discoveryState: McpServerDiscoveryState | null = null
191192
let discoveryError: string | null = null
192-
const discoveryStartedAt = new Date()
193193

194194
try {
195-
discoveredTools = await mcpService.discoverServerTools(
195+
const discovery = await mcpService.discoverServerToolsWithMetadata(
196196
userId,
197197
serverId,
198198
workspaceId,
199199
true
200200
)
201+
discoveredTools = discovery.tools
202+
discoveryState = discovery.state
201203
logger.info(
202204
`[${requestId}] Discovered ${discoveredTools.length} tools from server ${serverId}`
203205
)
@@ -214,6 +216,7 @@ export const POST = withRouteHandler(
214216
url: mcpServers.url,
215217
connectionStatus: mcpServers.connectionStatus,
216218
lastConnected: mcpServers.lastConnected,
219+
lastToolsRefresh: mcpServers.lastToolsRefresh,
217220
lastError: mcpServers.lastError,
218221
toolCount: mcpServers.toolCount,
219222
})
@@ -253,10 +256,20 @@ export const POST = withRouteHandler(
253256
let lastError = refreshedServer ? refreshedServer.lastError : discoveryError
254257
let toolCount = refreshedServer?.toolCount ?? discoveredTools.length
255258

259+
if (
260+
discoveryError === null &&
261+
(discoveryState === 'unavailable' || discoveryState === 'winner-cache')
262+
) {
263+
connectionStatus = 'connected'
264+
lastError = null
265+
toolCount = discoveredTools.length
266+
}
267+
256268
if (discoveryError !== null && connectionStatus === 'connected') {
257269
const newerSuccessWonRace =
258-
refreshedServer?.lastConnected != null &&
259-
refreshedServer.lastConnected > discoveryStartedAt
270+
refreshedServer?.lastToolsRefresh != null &&
271+
(server.lastToolsRefresh == null ||
272+
refreshedServer.lastToolsRefresh > server.lastToolsRefresh)
260273

261274
if (!newerSuccessWonRace) {
262275
connectionStatus = 'disconnected'

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

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -548,15 +548,18 @@ describe('McpService.discoverTools per-server caching', () => {
548548
)
549549
.mockResolvedValueOnce([tool('new-tool', 'mcp-a')])
550550

551-
const older = mcpService.discoverServerTools(USER_ID, 'mcp-a', WORKSPACE_ID, false)
551+
const older = mcpService.discoverServerToolsWithMetadata(USER_ID, 'mcp-a', WORKSPACE_ID, false)
552552
await vi.waitFor(() => expect(mockListTools).toHaveBeenCalledTimes(1))
553553

554554
mockCacheAdapter.beginMutation.mockRejectedValueOnce(new Error('transient ordering failure'))
555555
const newer = mcpService.discoverServerTools(USER_ID, 'mcp-a', WORKSPACE_ID, true)
556556
await expect(newer).resolves.toEqual([tool('new-tool', 'mcp-a')])
557557

558558
resolveOlder?.([tool('old-tool', 'mcp-a')])
559-
await expect(older).resolves.toEqual([tool('new-tool', 'mcp-a')])
559+
await expect(older).resolves.toEqual({
560+
tools: [tool('new-tool', 'mcp-a')],
561+
state: 'winner-cache',
562+
})
560563

561564
expect(mockCacheAdapter.beginMutation).toHaveBeenCalledTimes(3)
562565
expect(cacheStore.get(serverKey)?.tools).toEqual([tool('new-tool', 'mcp-a')])
@@ -632,8 +635,11 @@ describe('McpService.discoverTools per-server caching', () => {
632635
mockCacheAdapter.beginMutation
633636
.mockRejectedValueOnce(new Error('ordering unavailable'))
634637
.mockRejectedValueOnce(new Error('ordering unavailable'))
635-
const newer = mcpService.discoverServerTools(USER_ID, 'mcp-a', WORKSPACE_ID, true)
636-
await expect(newer).resolves.toEqual([tool('unowned-new-tool', 'mcp-a')])
638+
const newer = mcpService.discoverServerToolsWithMetadata(USER_ID, 'mcp-a', WORKSPACE_ID, true)
639+
await expect(newer).resolves.toEqual({
640+
tools: [tool('unowned-new-tool', 'mcp-a')],
641+
state: 'unavailable',
642+
})
637643

638644
resolveOlder?.([tool('owned-old-tool', 'mcp-a')])
639645
await expect(older).resolves.toEqual([tool('owned-old-tool', 'mcp-a')])

apps/sim/lib/mcp/service.ts

Lines changed: 31 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -72,6 +72,18 @@ const STATUS_UPDATE_CAS_ATTEMPTS = 3
7272
type CacheMutationResult = 'applied' | 'superseded' | 'unavailable'
7373
type SuccessfulPublicationResult = 'published' | Exclude<CacheMutationResult, 'applied'>
7474

75+
export type McpServerDiscoveryState =
76+
| 'cached'
77+
| 'published'
78+
| 'winner-cache'
79+
| 'superseded'
80+
| 'unavailable'
81+
82+
export interface McpServerDiscoveryResult {
83+
tools: McpTool[]
84+
state: McpServerDiscoveryState
85+
}
86+
7587
type DiscoveryOutcome =
7688
| { kind: 'cached'; tools: McpTool[] }
7789
| {
@@ -188,7 +200,7 @@ class McpService {
188200
private readonly cacheTimeout = MCP_CONSTANTS.CACHE_TIMEOUT
189201
private unsubscribeConnectionManager?: () => void
190202
// Keyed on (workspaceId, serverId, userId) — OAuth-scoped tokens vary per user.
191-
private inflightServerDiscovery = new Map<string, Promise<McpTool[]>>()
203+
private inflightServerDiscovery = new Map<string, Promise<McpServerDiscoveryResult>>()
192204

193205
constructor() {
194206
this.cacheAdapter = createMcpCacheAdapter()
@@ -942,6 +954,16 @@ class McpService {
942954
workspaceId: string,
943955
forceRefresh = false
944956
): Promise<McpTool[]> {
957+
return (await this.discoverServerToolsWithMetadata(userId, serverId, workspaceId, forceRefresh))
958+
.tools
959+
}
960+
961+
async discoverServerToolsWithMetadata(
962+
userId: string,
963+
serverId: string,
964+
workspaceId: string,
965+
forceRefresh = false
966+
): Promise<McpServerDiscoveryResult> {
945967
const inflightKey = `${workspaceId}:${serverId}:${userId}:${forceRefresh ? 'force' : 'cache'}`
946968
const existing = this.inflightServerDiscovery.get(inflightKey)
947969
if (existing) return existing
@@ -963,7 +985,7 @@ class McpService {
963985
serverId: string,
964986
workspaceId: string,
965987
forceRefresh: boolean
966-
): Promise<McpTool[]> {
988+
): Promise<McpServerDiscoveryResult> {
967989
const requestId = generateRequestId()
968990
const maxRetries = 2
969991

@@ -972,7 +994,7 @@ class McpService {
972994
const cached = await this.cacheAdapter.get(serverCacheKey(workspaceId, serverId))
973995
if (cached) {
974996
logger.debug(`[${requestId}] Cache hit for server ${serverId}`)
975-
return cached.tools
997+
return { tools: cached.tools, state: 'cached' }
976998
}
977999
} catch (error) {
9781000
logger.warn(`[${requestId}] Cache read failed for server ${serverId}:`, error)
@@ -1021,10 +1043,14 @@ class McpService {
10211043
reason: publication,
10221044
})
10231045
if (publication === 'superseded') {
1024-
return (await this.getCurrentCachedTools(workspaceId, serverId)) ?? []
1046+
const winner = await this.getCurrentCachedTools(workspaceId, serverId)
1047+
return winner
1048+
? { tools: winner, state: 'winner-cache' }
1049+
: { tools: [], state: 'superseded' }
10251050
}
1051+
return { tools, state: 'unavailable' }
10261052
}
1027-
return tools
1053+
return { tools, state: 'published' }
10281054
} finally {
10291055
await client.disconnect()
10301056
}

0 commit comments

Comments
 (0)