Skip to content

Commit cdc3f46

Browse files
committed
fix(mcp): acquire a fresh cache mutation per discovery retry
A retried discovery took mutation ownership once before the retry loop, so a retry that succeeded after a concurrent clearCache published under a stale ownership id, lost the CAS, and dropped otherwise-valid tools (empty fail-closed result). Begin a fresh mutation per attempt so a retried result publishes under a current id. Adds a regression test.
1 parent 444f512 commit cdc3f46

2 files changed

Lines changed: 21 additions & 2 deletions

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 & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1069,10 +1069,12 @@ class McpService {
10691069
}
10701070
}
10711071

1072-
const mutation = await this.beginServerCacheMutation(workspaceId, serverId)
1073-
10741072
for (let attempt = 0; attempt < maxRetries; attempt++) {
10751073
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)
10761078
try {
10771079
logger.info(
10781080
`[${requestId}] Discovering tools from server ${serverId} for user ${userId}${attempt > 0 ? ` (attempt ${attempt + 1})` : ''}`

0 commit comments

Comments
 (0)