Skip to content

Commit 8899fdf

Browse files
committed
Merge remote-tracking branch 'origin/fix/mcp-discovery-races' into fix/mcp-discovery-races
2 parents 5940a20 + cdc3f46 commit 8899fdf

2 files changed

Lines changed: 21 additions & 1 deletion

File tree

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

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -517,6 +517,23 @@ describe('McpService.discoverTools per-server caching', () => {
517517
)
518518
})
519519

520+
it('acquires a fresh mutation on each discovery retry so a retried result still publishes', async () => {
521+
const serverKey = `workspace:${WORKSPACE_ID}:server:mcp-a`
522+
mockGetWorkspaceServersRows.mockResolvedValue([dbRow('mcp-a', 'A')])
523+
mockCacheAdapter.beginMutation.mockClear()
524+
mockListTools
525+
.mockRejectedValueOnce(new Error('Request timed out'))
526+
.mockResolvedValueOnce([tool('a1', 'mcp-a')])
527+
528+
const tools = await mcpService.discoverServerTools(USER_ID, 'mcp-a', WORKSPACE_ID, true)
529+
530+
expect(tools).toEqual([tool('a1', 'mcp-a')])
531+
expect(cacheStore.get(serverKey)?.tools).toEqual([tool('a1', 'mcp-a')])
532+
// One begin per attempt: the retry publishes under a current ownership id
533+
// instead of a stale pre-loop id that a concurrent clearCache could supersede.
534+
expect(mockCacheAdapter.beginMutation).toHaveBeenCalledTimes(2)
535+
})
536+
520537
it('keeps an older ordered publisher from superseding a retry-acquired mutation', async () => {
521538
const serverKey = `workspace:${WORKSPACE_ID}:server:mcp-a`
522539
mockGetWorkspaceServersRows.mockResolvedValue([dbRow('mcp-a', 'A')])

apps/sim/lib/mcp/service.ts

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1070,8 +1070,11 @@ class McpService {
10701070
}
10711071

10721072
for (let attempt = 0; attempt < maxRetries; attempt++) {
1073-
const mutation = await this.beginServerCacheMutation(workspaceId, serverId)
10741073
let config: McpServerConfig | null = null
1074+
// Begin a fresh mutation per attempt. A retry that succeeds after a
1075+
// concurrent clearCache must publish under a current ownership id — a
1076+
// stale pre-loop id would lose the CAS and drop otherwise-valid tools.
1077+
const mutation = await this.beginServerCacheMutation(workspaceId, serverId)
10751078
try {
10761079
logger.info(
10771080
`[${requestId}] Discovering tools from server ${serverId} for user ${userId}${attempt > 0 ? ` (attempt ${attempt + 1})` : ''}`

0 commit comments

Comments
 (0)