From 0ea66ec71b3c22b5a2a4a6784f4c3c42cd141c66 Mon Sep 17 00:00:00 2001 From: Justin Blumencranz <96924014+j15z@users.noreply.github.com> Date: Fri, 17 Jul 2026 18:40:48 -0700 Subject: [PATCH 01/16] fix(mcp): stabilize authenticated tool discovery --- .../sim/app/api/mcp/oauth/start/route.test.ts | 17 + apps/sim/app/api/mcp/oauth/start/route.ts | 12 + .../mcp/servers/[id]/refresh/route.test.ts | 21 +- .../app/api/mcp/servers/[id]/refresh/route.ts | 53 +- apps/sim/lib/mcp/client.test.ts | 49 ++ apps/sim/lib/mcp/client.ts | 47 +- apps/sim/lib/mcp/service.test.ts | 158 +++++- apps/sim/lib/mcp/service.ts | 537 ++++++++++++------ apps/sim/lib/mcp/storage/adapter.ts | 14 + apps/sim/lib/mcp/storage/memory-cache.test.ts | 40 ++ apps/sim/lib/mcp/storage/memory-cache.ts | 34 ++ apps/sim/lib/mcp/storage/redis-cache.test.ts | 92 +++ apps/sim/lib/mcp/storage/redis-cache.ts | 108 ++++ 13 files changed, 944 insertions(+), 238 deletions(-) create mode 100644 apps/sim/lib/mcp/storage/redis-cache.test.ts diff --git a/apps/sim/app/api/mcp/oauth/start/route.test.ts b/apps/sim/app/api/mcp/oauth/start/route.test.ts index 68dd96b2bf3..555e2d2fa4d 100644 --- a/apps/sim/app/api/mcp/oauth/start/route.test.ts +++ b/apps/sim/app/api/mcp/oauth/start/route.test.ts @@ -162,6 +162,23 @@ describe('MCP OAuth start route', () => { expect(body.error).not.toContain('ECONNREFUSED') expect(body.error).not.toContain('internal-db-host') }) + + it('returns an actionable 4xx when the server does not support dynamic client registration', async () => { + mcpOauthMockFns.mockMcpAuthGuarded.mockRejectedValueOnce( + new Error('Incompatible auth server: does not support dynamic client registration') + ) + const request = new NextRequest( + 'http://localhost:3000/api/mcp/oauth/start?workspaceId=workspace-1&serverId=server-1' + ) + + const response = await GET(request) + const body = await response.json() + + expect(response.status).toBe(422) + expect(body.error).toBe( + "This server doesn't support OAuth client registration. Configure a token instead." + ) + }) }) describe('surfaceOauthError', () => { diff --git a/apps/sim/app/api/mcp/oauth/start/route.ts b/apps/sim/app/api/mcp/oauth/start/route.ts index edb17a0f1c9..d176fb6d673 100644 --- a/apps/sim/app/api/mcp/oauth/start/route.ts +++ b/apps/sim/app/api/mcp/oauth/start/route.ts @@ -25,6 +25,15 @@ import { createMcpErrorResponse } from '@/lib/mcp/utils' const logger = createLogger('McpOauthStartAPI') const OAUTH_START_TTL_MS = 10 * 60 * 1000 const MAX_SURFACED_ERROR_LENGTH = 250 +const DCR_UNSUPPORTED_MESSAGE = + "This server doesn't support OAuth client registration. Configure a token instead." + +function isDynamicClientRegistrationUnsupported(error: unknown): boolean { + return ( + error instanceof Error && + error.message.toLowerCase().includes('does not support dynamic client registration') + ) +} export function surfaceOauthError(error: unknown): string { // Spec-compliant OAuth servers throw typed subclasses with clean RFC 6749 fields. @@ -148,6 +157,9 @@ export const GET = withRouteHandler( authorizationUrl: e.authorizationUrl, }) } + if (isDynamicClientRegistrationUnsupported(e)) { + return createMcpErrorResponse(toError(e), DCR_UNSUPPORTED_MESSAGE, 422) + } throw e } } catch (error) { diff --git a/apps/sim/app/api/mcp/servers/[id]/refresh/route.test.ts b/apps/sim/app/api/mcp/servers/[id]/refresh/route.test.ts index e3f217fbdde..72ec98d0087 100644 --- a/apps/sim/app/api/mcp/servers/[id]/refresh/route.test.ts +++ b/apps/sim/app/api/mcp/servers/[id]/refresh/route.test.ts @@ -81,6 +81,8 @@ function selectRows(rows: unknown[]) { describe('MCP server refresh route', () => { beforeEach(() => { vi.clearAllMocks() + mockSelect.mockReset() + mockSelect.mockReturnValue(selectRows([persistedServer])) mockSelect.mockReturnValueOnce(selectRows([initialServer])) mockUpdateSet.mockReturnValue({ where: vi.fn().mockReturnValue({ returning: vi.fn().mockResolvedValue([persistedServer]) }), @@ -112,11 +114,7 @@ describe('MCP server refresh route', () => { mockDiscoverServerTools.mockRejectedValueOnce( new Error(`Upstream reflected ${reflectedSecret}`) ) - mockUpdateSet.mockReturnValueOnce({ - where: vi.fn().mockReturnValue({ - returning: vi.fn().mockResolvedValue([initialServer]), - }), - }) + mockSelect.mockReturnValueOnce(selectRows([initialServer])) const request = new Request('http://localhost/api/mcp/servers/server-1/refresh', { method: 'POST', @@ -128,6 +126,7 @@ describe('MCP server refresh route', () => { expect.objectContaining({ status: 'disconnected', error: 'Internal server error', + toolCount: 0, workflowsUpdated: 0, }) ) @@ -142,11 +141,7 @@ describe('MCP server refresh route', () => { lastConnected: new Date(Date.now() + 60_000), toolCount: 7, } - mockUpdateSet.mockReturnValueOnce({ - where: vi.fn().mockReturnValue({ - returning: vi.fn().mockResolvedValue([newerSuccessfulServer]), - }), - }) + mockSelect.mockReturnValueOnce(selectRows([newerSuccessfulServer])) const request = new Request('http://localhost/api/mcp/servers/server-1/refresh', { method: 'POST', @@ -162,7 +157,7 @@ describe('MCP server refresh route', () => { workflowsUpdated: 0, }) ) - expect(mockClearCache).toHaveBeenCalledWith('workspace-1') + expect(mockClearCache).not.toHaveBeenCalled() }) it('does not 500 when workflow sync fails after a successful discovery', async () => { @@ -178,9 +173,7 @@ describe('MCP server refresh route', () => { // The route's server lookup consumes the first select (beforeEach). The sync's // workflow select is left unmocked, so it throws — exercising the guard that // keeps a secondary sync failure from turning a successful refresh into a 500. - mockUpdateSet.mockReturnValueOnce({ - where: vi.fn().mockReturnValue({ returning: vi.fn().mockResolvedValue([initialServer]) }), - }) + mockSelect.mockReturnValueOnce(selectRows([initialServer])) const request = new Request('http://localhost/api/mcp/servers/server-1/refresh', { method: 'POST', diff --git a/apps/sim/app/api/mcp/servers/[id]/refresh/route.ts b/apps/sim/app/api/mcp/servers/[id]/refresh/route.ts index 26bb6ddaaef..87ca976ed9a 100644 --- a/apps/sim/app/api/mcp/servers/[id]/refresh/route.ts +++ b/apps/sim/app/api/mcp/servers/[id]/refresh/route.ts @@ -208,6 +208,25 @@ export const POST = withRouteHandler( }) } + const [refreshedServer] = await db + .select({ + name: mcpServers.name, + url: mcpServers.url, + connectionStatus: mcpServers.connectionStatus, + lastConnected: mcpServers.lastConnected, + lastError: mcpServers.lastError, + toolCount: mcpServers.toolCount, + }) + .from(mcpServers) + .where( + and( + eq(mcpServers.id, serverId), + eq(mcpServers.workspaceId, workspaceId), + isNull(mcpServers.deletedAt) + ) + ) + .limit(1) + if (discoveryError === null) { try { syncResult = await syncToolSchemasToWorkflows( @@ -215,7 +234,10 @@ export const POST = withRouteHandler( serverId, discoveredTools, requestId, - { url: server.url ?? undefined, name: server.name ?? undefined } + { + url: refreshedServer?.url ?? undefined, + name: refreshedServer?.name ?? undefined, + } ) } catch (error) { // Discovery already persisted status and cached tools; a workflow-sync @@ -227,31 +249,9 @@ export const POST = withRouteHandler( } } - const now = new Date() - - const [refreshedServer] = await db - .update(mcpServers) - .set({ - lastToolsRefresh: now, - updatedAt: now, - }) - .where( - and( - eq(mcpServers.id, serverId), - eq(mcpServers.workspaceId, workspaceId), - isNull(mcpServers.deletedAt) - ) - ) - .returning({ - connectionStatus: mcpServers.connectionStatus, - lastConnected: mcpServers.lastConnected, - lastError: mcpServers.lastError, - toolCount: mcpServers.toolCount, - }) - let connectionStatus = refreshedServer?.connectionStatus ?? 'error' let lastError = refreshedServer ? refreshedServer.lastError : discoveryError - const toolCount = refreshedServer?.toolCount ?? discoveredTools.length + let toolCount = refreshedServer?.toolCount ?? discoveredTools.length if (discoveryError !== null && connectionStatus === 'connected') { const newerSuccessWonRace = @@ -261,13 +261,10 @@ export const POST = withRouteHandler( if (!newerSuccessWonRace) { connectionStatus = 'disconnected' lastError = discoveryError + toolCount = 0 } } - if (connectionStatus === 'connected') { - await mcpService.clearCache(workspaceId) - } - return createMcpSuccessResponse({ status: connectionStatus, toolCount, diff --git a/apps/sim/lib/mcp/client.test.ts b/apps/sim/lib/mcp/client.test.ts index c568f425d61..d8dbd68e8d3 100644 --- a/apps/sim/lib/mcp/client.test.ts +++ b/apps/sim/lib/mcp/client.test.ts @@ -1,6 +1,7 @@ /** * @vitest-environment node */ +import { UnauthorizedError } from '@modelcontextprotocol/sdk/client/auth.js' import { beforeEach, describe, expect, it, vi } from 'vitest' const { mockLogger, mockSdkConnect, mockSdkListTools } = vi.hoisted(() => ({ @@ -265,6 +266,54 @@ describe('McpClient notification handler', () => { expect(JSON.stringify(mockLogger.error.mock.calls)).not.toContain(secret) }) + it('does not misclassify rejected static headers as an OAuth authorization flow', async () => { + mockSdkConnect.mockRejectedValueOnce(new UnauthorizedError('Static token rejected')) + const client = new McpClient({ + config: { + ...createConfig(), + authType: 'headers', + headers: { Authorization: 'Bearer rejected-static-token' }, + }, + securityPolicy: { requireConsent: false, auditLevel: 'basic' }, + }) + + await expect(client.connect()).rejects.toThrow('Static token rejected') + + expect(mockLogger.error).toHaveBeenCalledWith( + expect.stringContaining('Failed to connect'), + expect.objectContaining({ outcome: 'unauthorized' }) + ) + }) + + it('logs tools/list failures without echoed credentials or session identifiers', async () => { + const secret = 'opaque-tools-list-credential' + mockSdkListTools.mockRejectedValueOnce(new Error(`Upstream rejected ${secret}`)) + const client = new McpClient({ + config: { + ...createConfig(), + authType: 'headers', + headers: { 'X-Custom-Credential': secret }, + }, + securityPolicy: { requireConsent: false, auditLevel: 'basic' }, + }) + + await client.connect() + await expect(client.listTools()).rejects.toThrow(secret) + + expect(mockLogger.error).toHaveBeenCalledWith( + expect.stringContaining('Failed to list tools'), + expect.objectContaining({ + phase: 'tools/list', + serverId: 'server-1', + sessionIdPresent: true, + error: expect.objectContaining({ name: 'Error' }), + }) + ) + const logged = JSON.stringify(mockLogger.error.mock.calls) + expect(logged).not.toContain(secret) + expect(logged).not.toContain('test-session') + }) + it('passes configured headers for OAuth transports as well as header auth transports', () => { const authProvider = {} as unknown as NonNullable new McpClient({ diff --git a/apps/sim/lib/mcp/client.ts b/apps/sim/lib/mcp/client.ts index 50a90355c14..5473d755a6c 100644 --- a/apps/sim/lib/mcp/client.ts +++ b/apps/sim/lib/mcp/client.ts @@ -43,10 +43,16 @@ type ConnectionOutcome = | 'cancelled' | 'error' -function classifyConnectionOutcome(error: unknown): ConnectionOutcome { - if (error instanceof McpOauthRedirectRequired || error instanceof UnauthorizedError) { +function classifyConnectionOutcome( + error: unknown, + authType: McpServerConfig['authType'] +): ConnectionOutcome { + if (error instanceof McpOauthRedirectRequired) { return 'authorization_required' } + if (error instanceof UnauthorizedError) { + return authType === 'oauth' ? 'authorization_required' : 'unauthorized' + } const message = getErrorMessage(error, '').toLowerCase() if (message.includes('connection attempt cancelled')) return 'cancelled' if (message.includes('timeout') || message.includes('timed out')) return 'timeout' @@ -54,6 +60,16 @@ function classifyConnectionOutcome(error: unknown): ConnectionOutcome { return 'error' } +function getSafeErrorDiagnostics(error: unknown) { + const describedError = describeError(error) + return { + name: sanitizeForLogging(describedError.name, 100), + code: describedError.code ? sanitizeForLogging(describedError.code, 100) : undefined, + errno: describedError.errno ? sanitizeForLogging(describedError.errno, 100) : undefined, + syscall: describedError.syscall ? sanitizeForLogging(describedError.syscall, 100) : undefined, + } +} + interface McpClientConnectOptions { isCancelled?: () => boolean } @@ -109,7 +125,9 @@ export class McpClient { this.client.onerror = (error) => { logger.warn(`MCP transport error for ${this.config.name}`, { serverId: this.config.id, - error: sanitizeForLogging(getErrorMessage(error, 'Unknown transport error'), 200), + phase: 'transport', + sessionIdPresent: Boolean(this.transport.sessionId), + error: getSafeErrorDiagnostics(error), }) } } @@ -178,19 +196,11 @@ export class McpClient { } catch (error) { this.isConnected = false const errorMessage = getErrorMessage(error, 'Unknown error') - const describedError = describeError(error) - const outcome = classifyConnectionOutcome(error) + const outcome = classifyConnectionOutcome(error, this.config.authType) logger.error(`Failed to connect to MCP server ${this.config.name}`, { ...diagnostics, durationMs: Date.now() - startedAt, - error: { - name: sanitizeForLogging(describedError.name, 100), - code: describedError.code ? sanitizeForLogging(describedError.code, 100) : undefined, - errno: describedError.errno ? sanitizeForLogging(describedError.errno, 100) : undefined, - syscall: describedError.syscall - ? sanitizeForLogging(describedError.syscall, 100) - : undefined, - }, + error: getSafeErrorDiagnostics(error), outcome, }) if (outcome === 'authorization_required') { @@ -236,6 +246,7 @@ export class McpClient { MCP_CLIENT_CONSTANTS.LIST_TOOLS_MAX_TOTAL_TIMEOUT_MS ) const maxTotalTimeoutMs = MCP_CLIENT_CONSTANTS.LIST_TOOLS_MAX_TOTAL_TIMEOUT_MS + const startedAt = Date.now() try { const result: ListToolsResult = await this.client.listTools(undefined, { @@ -265,7 +276,15 @@ export class McpClient { serverName: this.config.name, })) } catch (error) { - logger.error(`Failed to list tools from server ${this.config.name}:`, error) + logger.error(`Failed to list tools from server ${this.config.name}`, { + serverId: this.config.id, + phase: 'tools/list', + durationMs: Date.now() - startedAt, + idleTimeoutMs, + maxTotalTimeoutMs, + sessionIdPresent: Boolean(this.transport.sessionId), + error: getSafeErrorDiagnostics(error), + }) throw error } } diff --git a/apps/sim/lib/mcp/service.test.ts b/apps/sim/lib/mcp/service.test.ts index 7349b39e71f..bed170d18f8 100644 --- a/apps/sim/lib/mcp/service.test.ts +++ b/apps/sim/lib/mcp/service.test.ts @@ -19,6 +19,7 @@ const { mockCacheAdapter, mockUpdateSet, mockUpdateReturning, + cacheStore, } = vi.hoisted(() => { const mockListTools = vi.fn() const mockConnect = vi.fn() @@ -28,6 +29,8 @@ const { // local .env points at (unreachable in CI/sandbox → hangs). Honors TTL via // an expiry timestamp so negative-cache assertions behave like production. const cacheStore = new Map() + const cacheMutations = new Map() + let nextMutationId = 0 const mockCacheAdapter = { get: vi.fn(async (key: string) => { const entry = cacheStore.get(key) @@ -44,12 +47,39 @@ const { delete: vi.fn(async (key: string) => { cacheStore.delete(key) }), + beginMutation: vi.fn(async (scopeKey: string) => { + const mutationId = ++nextMutationId + cacheMutations.set(scopeKey, mutationId) + return mutationId + }), + setIfCurrentMutation: vi.fn( + async ( + scopeKey: string, + mutationId: number, + key: string, + tools: unknown[], + ttlMs: number + ) => { + if (cacheMutations.get(scopeKey) !== mutationId) return false + cacheStore.set(key, { tools, expiry: Date.now() + ttlMs }) + return true + } + ), + deleteIfCurrentMutation: vi.fn(async (scopeKey: string, mutationId: number, key: string) => { + if (cacheMutations.get(scopeKey) !== mutationId) return false + cacheStore.delete(key) + return true + }), clear: vi.fn(async () => { cacheStore.clear() + for (const scopeKey of cacheMutations.keys()) { + cacheMutations.set(scopeKey, ++nextMutationId) + } }), dispose: () => {}, } return { + cacheStore, mockCacheAdapter, MockMcpClient: vi.fn().mockImplementation( class { @@ -356,14 +386,18 @@ describe('McpService.discoverTools per-server caching', () => { statusConfig: { consecutiveFailures: 1, lastSuccessfulDiscovery: null }, }) ) - expect(mockCacheAdapter.set).toHaveBeenCalledWith( + expect(mockCacheAdapter.setIfCurrentMutation).toHaveBeenCalledWith( + `workspace:${WORKSPACE_ID}:server:mcp-a`, + expect.any(Number), `workspace:${WORKSPACE_ID}:server:mcp-a:failure`, [], expect.any(Number) ) }) expect(JSON.stringify(mockUpdateSet.mock.calls)).not.toContain(reflectedCredential) - expect(JSON.stringify(mockCacheAdapter.set.mock.calls)).not.toContain(reflectedCredential) + expect(JSON.stringify(mockCacheAdapter.setIfCurrentMutation.mock.calls)).not.toContain( + reflectedCredential + ) expect(JSON.stringify(mockLogger?.warn.mock.calls)).not.toContain(reflectedCredential) mockListTools.mockClear() @@ -387,7 +421,9 @@ describe('McpService.discoverTools per-server caching', () => { }) ) }) - expect(mockCacheAdapter.set).not.toHaveBeenCalledWith( + expect(mockCacheAdapter.setIfCurrentMutation).not.toHaveBeenCalledWith( + expect.anything(), + expect.anything(), `workspace:${WORKSPACE_ID}:server:mcp-a:failure`, [], expect.any(Number) @@ -472,6 +508,114 @@ describe('McpService.discoverTools per-server caching', () => { ) }) + it('persists an allowlisted message when an upstream error reflects a custom credential', async () => { + const reflectedCredential = 'opaque-custom-header-value' + mockGetWorkspaceServersRows.mockResolvedValue([ + dbRow('mcp-a', 'A', { + statusConfig: { consecutiveFailures: 0, lastSuccessfulDiscovery: null }, + }), + ]) + mockListTools.mockRejectedValueOnce( + new Error(`Provider rejected X-Custom-Credential: ${reflectedCredential}`) + ) + + await expect(mcpService.discoverServerTools(USER_ID, 'mcp-a', WORKSPACE_ID)).rejects.toThrow( + reflectedCredential + ) + + expect(mockUpdateSet).toHaveBeenCalledWith( + expect.objectContaining({ + connectionStatus: 'disconnected', + lastError: 'Connection failed', + toolCount: 0, + }) + ) + expect(JSON.stringify(mockUpdateSet.mock.calls)).not.toContain(reflectedCredential) + }) + + it('does not return or cache tools discovered from a stale server configuration', async () => { + mockGetWorkspaceServersRows.mockResolvedValue([dbRow('mcp-a', 'A')]) + mockListTools.mockResolvedValueOnce([tool('stale-tool', 'mcp-a')]) + mockUpdateSet.mockReturnValueOnce({ + where: vi.fn().mockReturnValue({ returning: vi.fn().mockResolvedValue([]) }), + }) + + const tools = await mcpService.discoverTools(USER_ID, WORKSPACE_ID, true) + + expect(tools).toEqual([]) + expect(mockCacheAdapter.setIfCurrentMutation).not.toHaveBeenCalledWith( + expect.anything(), + expect.anything(), + `workspace:${WORKSPACE_ID}:server:mcp-a`, + expect.anything(), + expect.any(Number) + ) + }) + + it('waits for bulk discovery status publication before returning', async () => { + mockGetWorkspaceServersRows.mockResolvedValue([dbRow('mcp-a', 'A')]) + mockListTools.mockResolvedValueOnce([tool('a1', 'mcp-a')]) + + let releaseStatus: (() => void) | undefined + const pendingStatus = new Promise((resolve) => { + releaseStatus = resolve + }) + mockUpdateSet.mockReturnValueOnce({ + where: vi.fn().mockReturnValue({ + returning: vi.fn().mockReturnValue(pendingStatus.then(() => [{ id: 'mcp-a' }])), + }), + }) + + let settled = false + const discovery = mcpService.discoverTools(USER_ID, WORKSPACE_ID, true).finally(() => { + settled = true + }) + + await vi.waitFor(() => expect(mockListTools).toHaveBeenCalledTimes(1)) + await Promise.resolve() + expect(settled).toBe(false) + + releaseStatus?.() + await expect(discovery).resolves.toEqual([tool('a1', 'mcp-a')]) + }) + + it('keeps a newer successful cache entry when an older failure finishes later', async () => { + mockGetWorkspaceServersRows.mockResolvedValue([dbRow('mcp-a', 'A')]) + + let rejectOlder: ((error: Error) => void) | undefined + const olderList = new Promise((_resolve, reject) => { + rejectOlder = reject + }) + mockListTools.mockReturnValueOnce(olderList).mockResolvedValueOnce([tool('new-tool', 'mcp-a')]) + + let releaseOlderFailureCache: (() => void) | undefined + const olderFailureCacheGate = new Promise((resolve) => { + releaseOlderFailureCache = resolve + }) + const defaultSet = mockCacheAdapter.setIfCurrentMutation.getMockImplementation() + mockCacheAdapter.setIfCurrentMutation.mockImplementationOnce( + async (scopeKey: string, mutationId: number, key: string, tools: unknown[], ttl: number) => { + if (key.endsWith(':failure')) await olderFailureCacheGate + return defaultSet?.(scopeKey, mutationId, key, tools, ttl) ?? false + } + ) + + const older = mcpService.discoverServerTools(USER_ID, 'mcp-a', WORKSPACE_ID, false) + await vi.waitFor(() => expect(mockListTools).toHaveBeenCalledTimes(1)) + rejectOlder?.(new Error('Older request failed')) + + const newer = mcpService.discoverServerTools(USER_ID, 'mcp-a', WORKSPACE_ID, true) + await expect(newer).resolves.toEqual([tool('new-tool', 'mcp-a')]) + + releaseOlderFailureCache?.() + await expect(older).rejects.toThrow('Older request failed') + + expect(cacheStore.get(`workspace:${WORKSPACE_ID}:server:mcp-a`)?.tools).toEqual([ + tool('new-tool', 'mcp-a'), + ]) + expect(cacheStore.has(`workspace:${WORKSPACE_ID}:server:mcp-a:failure`)).toBe(false) + }) + it('retries a transient tools/list timeout and succeeds on the second attempt', async () => { mockGetWorkspaceServersRows.mockResolvedValue([dbRow('mcp-a', 'A')]) mockListTools @@ -507,7 +651,9 @@ describe('McpService.discoverTools per-server caching', () => { }) ) expect(JSON.stringify(mockUpdateSet.mock.calls)).not.toContain(reflectedCredential) - expect(JSON.stringify(mockCacheAdapter.set.mock.calls)).not.toContain(reflectedCredential) + expect(JSON.stringify(mockCacheAdapter.setIfCurrentMutation.mock.calls)).not.toContain( + reflectedCredential + ) expect(JSON.stringify(mockLogger?.warn.mock.calls)).not.toContain(reflectedCredential) mockListTools.mockClear() @@ -531,7 +677,9 @@ describe('McpService.discoverTools per-server caching', () => { lastError: null, }) ) - expect(mockCacheAdapter.set).not.toHaveBeenCalledWith( + expect(mockCacheAdapter.setIfCurrentMutation).not.toHaveBeenCalledWith( + expect.anything(), + expect.anything(), `workspace:${WORKSPACE_ID}:server:mcp-a:failure`, [], expect.any(Number) diff --git a/apps/sim/lib/mcp/service.ts b/apps/sim/lib/mcp/service.ts index 960007bf868..9416b36cb91 100644 --- a/apps/sim/lib/mcp/service.ts +++ b/apps/sim/lib/mcp/service.ts @@ -4,7 +4,7 @@ import { ErrorCode, McpError } from '@modelcontextprotocol/sdk/types.js' import { db } from '@sim/db' import { mcpServers } from '@sim/db/schema' import { createLogger } from '@sim/logger' -import { getErrorMessage } from '@sim/utils/errors' +import { describeError, getErrorMessage } from '@sim/utils/errors' import { sleep } from '@sim/utils/helpers' import { backoffWithJitter } from '@sim/utils/retry' import { and, eq, isNull, lte, or } from 'drizzle-orm' @@ -61,16 +61,38 @@ type DiscoveryOutcome = tools: McpTool[] resolvedConfig: McpServerConfig resolvedIP: string | null + mutation: CacheMutation | null } - | { kind: 'oauth-pending' } + | { kind: 'oauth-pending'; config: McpServerConfig; mutation: CacheMutation | null } | { kind: 'unhealthy' } // originalError preserves the type so markServerUnhealthy's instanceof // exemption survives the getErrorMessage call. - | { kind: 'error'; message: string; originalError: unknown } + | { + kind: 'error' + message: string + originalError: unknown + config: McpServerConfig + mutation: CacheMutation | null + } + +interface CacheMutation { + scopeKey: string + id: number +} type ServerStatusUpdate = - | { outcome: 'connected'; toolCount: number } - | { outcome: 'failed'; error: string; discoveryStartedAt?: Date } + | { + outcome: 'connected' + toolCount: number + configUpdatedAt: string + discoveryStartedAt: Date + } + | { + outcome: 'failed' + error: string + configUpdatedAt: string + discoveryStartedAt: Date + } function isOauthAuthorizationError(error: unknown, authType: McpServerConfig['authType']): boolean { return ( @@ -90,7 +112,34 @@ function getDiscoveryFailureMessage( if (isTimeoutError(error)) { return 'The MCP server took too long to respond and timed out' } - return getErrorMessage(error, fallback) + if (error instanceof StreamableHTTPError) { + if (error.code === 401 || error.code === 403) return 'Authentication failed' + if (error.code === 429) return 'The MCP server is rate limited. Try again shortly.' + if (typeof error.code === 'number' && error.code >= 500) { + return 'The MCP server is temporarily unavailable' + } + } + const message = getErrorMessage(error, '').toLowerCase() + if ( + message.includes('econnrefused') || + message.includes('econnreset') || + message.includes('socket hang up') || + message.includes('fetch failed') || + message.includes('network') + ) { + return 'Unable to reach the MCP server' + } + return fallback === 'Unknown error' ? 'Connection failed' : fallback +} + +function getSafeErrorDiagnostics(error: unknown) { + const described = describeError(error) + return { + name: described.name, + code: described.code, + errno: described.errno, + syscall: described.syscall, + } } function isTimeoutError(error: unknown): boolean { @@ -139,19 +188,11 @@ class McpService { if (mcpConnectionManager) { this.unsubscribeConnectionManager = mcpConnectionManager.subscribe((event) => { - this.cacheAdapter - .delete(serverCacheKey(event.workspaceId, event.serverId)) - .catch((err) => - logger.warn(`Failed to invalidate cache for ${event.serverName} on listChanged:`, err) - ) - this.cacheAdapter - .delete(failureCacheKey(event.workspaceId, event.serverId)) - .catch((err) => - logger.warn( - `Failed to invalidate failure cache for ${event.serverName} on listChanged:`, - err - ) - ) + this.invalidateServerCache(event.workspaceId, event.serverId).catch((error) => { + logger.warn(`Failed to invalidate cache for ${event.serverName} on listChanged`, { + error: getSafeErrorDiagnostics(error), + }) + }) }) } } @@ -375,9 +416,20 @@ class McpService { ): Promise { try { const now = new Date() + const configUpdatedAt = new Date(update.configUpdatedAt) + const publicationConditions = and( + eq(mcpServers.id, serverId), + eq(mcpServers.workspaceId, workspaceId), + isNull(mcpServers.deletedAt), + eq(mcpServers.updatedAt, configUpdatedAt), + or( + isNull(mcpServers.lastToolsRefresh), + lte(mcpServers.lastToolsRefresh, update.discoveryStartedAt) + ) + ) if (update.outcome === 'connected') { - await db + const updatedServers = await db .update(mcpServers) .set({ connectionStatus: 'connected', @@ -389,10 +441,10 @@ class McpService { consecutiveFailures: 0, lastSuccessfulDiscovery: now.toISOString(), }, - updatedAt: now, }) - .where(eq(mcpServers.id, serverId)) - return true + .where(publicationConditions) + .returning({ id: mcpServers.id }) + return updatedServers.length > 0 } const [currentServer] = await db @@ -424,25 +476,14 @@ class McpService { .set({ connectionStatus: isErrorState ? 'error' : 'disconnected', lastError: update.error || 'Unknown error', + toolCount: 0, + lastToolsRefresh: now, statusConfig: { consecutiveFailures: newFailures, lastSuccessfulDiscovery: currentConfig.lastSuccessfulDiscovery, }, - updatedAt: now, }) - .where( - and( - eq(mcpServers.id, serverId), - eq(mcpServers.workspaceId, workspaceId), - isNull(mcpServers.deletedAt), - update.discoveryStartedAt - ? or( - isNull(mcpServers.lastConnected), - lte(mcpServers.lastConnected, update.discoveryStartedAt) - ) - : undefined - ) - ) + .where(publicationConditions) .returning({ id: mcpServers.id }) if (isErrorState && updatedServers.length > 0) { @@ -463,46 +504,53 @@ class McpService { workspaceId: string, serverId: string, error: unknown, - authType: McpServerConfig['authType'] - ): Promise { + authType: McpServerConfig['authType'], + mutation: CacheMutation | null + ): Promise { if (isOauthAuthorizationError(error, authType)) { - return + return true } + if (!mutation) return true try { - await this.cacheAdapter.set( + return await this.cacheAdapter.setIfCurrentMutation( + mutation.scopeKey, + mutation.id, failureCacheKey(workspaceId, serverId), FAILURE_CACHE_SENTINEL, MCP_CLIENT_CONSTANTS.FAILURE_CACHE_TTL_MS ) } catch (err) { logger.warn(`Failed to write failure cache for server ${serverId}:`, err) + return true } } private async markServerOauthPending( serverId: string, workspaceId: string, - discoveryStartedAt?: Date + configUpdatedAt: string, + discoveryStartedAt: Date ): Promise { try { + const now = new Date() const updatedServers = await db .update(mcpServers) .set({ connectionStatus: 'disconnected', lastError: null, - updatedAt: new Date(), + toolCount: 0, + lastToolsRefresh: now, }) .where( and( eq(mcpServers.id, serverId), eq(mcpServers.workspaceId, workspaceId), isNull(mcpServers.deletedAt), - discoveryStartedAt - ? or( - isNull(mcpServers.lastConnected), - lte(mcpServers.lastConnected, discoveryStartedAt) - ) - : undefined + eq(mcpServers.updatedAt, new Date(configUpdatedAt)), + or( + isNull(mcpServers.lastToolsRefresh), + lte(mcpServers.lastToolsRefresh, discoveryStartedAt) + ) ) ) .returning({ id: mcpServers.id }) @@ -522,14 +570,165 @@ class McpService { } } - private async clearServerFailure(workspaceId: string, serverId: string): Promise { + private async clearServerFailure( + workspaceId: string, + serverId: string, + mutation: CacheMutation | null + ): Promise { + if (!mutation) return true try { - await this.cacheAdapter.delete(failureCacheKey(workspaceId, serverId)) + return await this.cacheAdapter.deleteIfCurrentMutation( + mutation.scopeKey, + mutation.id, + failureCacheKey(workspaceId, serverId) + ) } catch (err) { logger.warn(`Failed to clear failure cache for server ${serverId}:`, err) + return true + } + } + + private async beginServerCacheMutation( + workspaceId: string, + serverId: string + ): Promise { + const scopeKey = serverCacheKey(workspaceId, serverId) + try { + return { scopeKey, id: await this.cacheAdapter.beginMutation(scopeKey) } + } catch (error) { + logger.warn(`Failed to order cache mutation for server ${serverId}`, { + error: getSafeErrorDiagnostics(error), + }) + return null + } + } + + private async invalidateServerCache(workspaceId: string, serverId: string): Promise { + const mutation = await this.beginServerCacheMutation(workspaceId, serverId) + if (!mutation) return + await Promise.all([ + this.cacheAdapter.deleteIfCurrentMutation( + mutation.scopeKey, + mutation.id, + serverCacheKey(workspaceId, serverId) + ), + this.cacheAdapter.deleteIfCurrentMutation( + mutation.scopeKey, + mutation.id, + failureCacheKey(workspaceId, serverId) + ), + ]) + } + + private async setServerToolsCache( + workspaceId: string, + serverId: string, + mutation: CacheMutation | null, + tools: McpTool[] + ): Promise { + if (!mutation) return true + try { + return await this.cacheAdapter.setIfCurrentMutation( + mutation.scopeKey, + mutation.id, + serverCacheKey(workspaceId, serverId), + tools, + this.cacheTimeout + ) + } catch (error) { + logger.warn(`Failed to write tool cache for server ${serverId}`, { + error: getSafeErrorDiagnostics(error), + }) + return true + } + } + + private async deleteServerToolsCache( + workspaceId: string, + serverId: string, + mutation: CacheMutation | null + ): Promise { + if (!mutation) return true + try { + return await this.cacheAdapter.deleteIfCurrentMutation( + mutation.scopeKey, + mutation.id, + serverCacheKey(workspaceId, serverId) + ) + } catch (error) { + logger.warn(`Failed to delete tool cache for server ${serverId}`, { + error: getSafeErrorDiagnostics(error), + }) + return true } } + private async publishSuccessfulDiscovery( + workspaceId: string, + config: McpServerConfig, + mutation: CacheMutation | null, + tools: McpTool[], + discoveryStartedAt: Date + ): Promise { + const statusApplied = await this.updateServerStatus(config.id, workspaceId, { + outcome: 'connected', + toolCount: tools.length, + configUpdatedAt: config.updatedAt!, + discoveryStartedAt, + }) + if (!statusApplied) return false + + const [toolsApplied, failureCleared] = await Promise.all([ + this.setServerToolsCache(workspaceId, config.id, mutation, tools), + this.clearServerFailure(workspaceId, config.id, mutation), + ]) + return toolsApplied && failureCleared + } + + private async publishFailedDiscovery( + workspaceId: string, + config: McpServerConfig, + mutation: CacheMutation | null, + error: unknown, + message: string, + discoveryStartedAt: Date + ): Promise { + const statusApplied = await this.updateServerStatus(config.id, workspaceId, { + outcome: 'failed', + error: message, + configUpdatedAt: config.updatedAt!, + discoveryStartedAt, + }) + if (!statusApplied) return false + + const [toolsDeleted, failureApplied] = await Promise.all([ + this.deleteServerToolsCache(workspaceId, config.id, mutation), + this.markServerUnhealthy(workspaceId, config.id, error, config.authType, mutation), + ]) + return toolsDeleted && failureApplied + } + + private async publishOauthPending( + workspaceId: string, + config: McpServerConfig, + mutation: CacheMutation | null, + discoveryStartedAt: Date + ): Promise { + const statusApplied = await this.markServerOauthPending( + config.id, + workspaceId, + config.updatedAt!, + discoveryStartedAt + ) + if (!statusApplied) return false + + const [toolsDeleted, failureCleared] = await Promise.all([ + this.deleteServerToolsCache(workspaceId, config.id, mutation), + this.clearServerFailure(workspaceId, config.id, mutation), + ]) + return toolsDeleted && failureCleared + } + async discoverTools( userId: string, workspaceId: string, @@ -570,6 +769,8 @@ class McpService { } } + const mutation = await this.beginServerCacheMutation(workspaceId, config.id) + try { const { config: resolvedConfig, resolvedIP } = await this.resolveConfigEnvVars( config, @@ -582,112 +783,101 @@ class McpService { logger.debug( `[${requestId}] Discovered ${tools.length} tools from server ${config.name}` ) - return { kind: 'fetched', tools, resolvedConfig, resolvedIP } + return { kind: 'fetched', tools, resolvedConfig, resolvedIP, mutation } } finally { await client.disconnect() } } catch (error) { if (isOauthAuthorizationError(error, config.authType)) { - return { kind: 'oauth-pending' } + return { kind: 'oauth-pending', config, mutation } } return { kind: 'error', message: getDiscoveryFailureMessage(error, config.authType, 'Unknown error'), originalError: error, + config, + mutation, } } }) ) - const allTools: McpTool[] = [] - const cacheWrites: Promise[] = [] - const deferredSideEffects: Promise[] = [] - const liveConnections: Array<{ - resolvedConfig: McpServerConfig - resolvedIP: string | null - }> = [] - let cachedCount = 0 - let fetchedCount = 0 - let failedCount = 0 - - outcomes.forEach((outcome, index) => { - const server = servers[index] - if (outcome.kind === 'cached') { - cachedCount++ - allTools.push(...outcome.tools) - return - } - if (outcome.kind === 'fetched') { - fetchedCount++ - allTools.push(...outcome.tools) - deferredSideEffects.push( - this.updateServerStatus(server.id, workspaceId, { - outcome: 'connected', - toolCount: outcome.tools.length, - }) - ) - cacheWrites.push( - this.cacheAdapter - .set(serverCacheKey(workspaceId, server.id), outcome.tools, this.cacheTimeout) - .catch((err) => - logger.warn(`[${requestId}] Cache write failed for ${server.name}:`, err) + const publications = await Promise.all( + outcomes.map(async (outcome, index) => { + const server = servers[index] + if (outcome.kind === 'cached') { + return { + tools: outcome.tools, + cached: 1, + fetched: 0, + failed: 0, + liveConnection: null, + } + } + if (outcome.kind === 'fetched') { + const published = await this.publishSuccessfulDiscovery( + workspaceId, + outcome.resolvedConfig, + outcome.mutation, + outcome.tools, + discoveryStartedAt + ) + if (!published) { + logger.info( + `[${requestId}] Ignoring superseded discovery result for server ${server.id}` ) - ) - deferredSideEffects.push(this.clearServerFailure(workspaceId, server.id)) - liveConnections.push({ - resolvedConfig: outcome.resolvedConfig, - resolvedIP: outcome.resolvedIP, - }) - return - } - if (outcome.kind === 'oauth-pending') { - // Mark disconnected so the UI surfaces the re-auth button. - logger.info(`[${requestId}] Skipping server ${server.name}: OAuth authorization pending`) - deferredSideEffects.push( - this.markServerOauthPending(server.id, workspaceId, discoveryStartedAt).then( - () => undefined + } + return { + tools: published ? outcome.tools : [], + cached: 0, + fetched: 1, + failed: published ? 0 : 1, + liveConnection: published + ? { + resolvedConfig: outcome.resolvedConfig, + resolvedIP: outcome.resolvedIP, + } + : null, + } + } + if (outcome.kind === 'oauth-pending') { + logger.info( + `[${requestId}] Skipping server ${server.name}: OAuth authorization pending` ) - ) - return - } - if (outcome.kind === 'unhealthy') { - // Status was persisted on the original failure; nothing to re-write. - failedCount++ - return - } - failedCount++ - logger.warn( - `[${requestId}] Failed to discover tools from server ${server.name}: ${outcome.message}` - ) - deferredSideEffects.push( - this.updateServerStatus(server.id, workspaceId, { - outcome: 'failed', + await this.publishOauthPending( + workspaceId, + outcome.config, + outcome.mutation, + discoveryStartedAt + ) + return { tools: [], cached: 0, fetched: 0, failed: 0, liveConnection: null } + } + if (outcome.kind === 'unhealthy') { + return { tools: [], cached: 0, fetched: 0, failed: 1, liveConnection: null } + } + + logger.warn(`[${requestId}] Failed to discover tools from server ${server.name}`, { error: outcome.message, - discoveryStartedAt, - }).then(async (statusApplied) => { - if (!statusApplied) return - await Promise.allSettled([ - this.markServerUnhealthy( - workspaceId, - server.id, - outcome.originalError, - server.authType - ), - this.cacheAdapter - .delete(serverCacheKey(workspaceId, server.id)) - .catch((err) => - logger.warn(`[${requestId}] Cache delete failed for ${server.name}:`, err) - ), - ]) }) - ) - }) + await this.publishFailedDiscovery( + workspaceId, + outcome.config, + outcome.mutation, + outcome.originalError, + outcome.message, + discoveryStartedAt + ) + return { tools: [], cached: 0, fetched: 0, failed: 1, liveConnection: null } + }) + ) - // Await cache writes so a follow-up discoverTools sees consistent state. - await Promise.allSettled(cacheWrites) - // Each deferred side-effect self-logs failures, so we just mark the - // promises as handled to avoid unhandled-rejection warnings. - for (const p of deferredSideEffects) p.catch(() => {}) + const allTools = publications.flatMap((publication) => publication.tools) + const cachedCount = publications.reduce((sum, publication) => sum + publication.cached, 0) + const fetchedCount = publications.reduce((sum, publication) => sum + publication.fetched, 0) + const failedCount = publications.reduce((sum, publication) => sum + publication.failed, 0) + const liveConnections = publications.flatMap((publication) => + publication.liveConnection ? [publication.liveConnection] : [] + ) if (mcpConnectionManager) { for (const conn of liveConnections) { @@ -770,18 +960,19 @@ class McpService { } } + const mutation = await this.beginServerCacheMutation(workspaceId, serverId) + for (let attempt = 0; attempt < maxRetries; attempt++) { - let authType: McpServerConfig['authType'] + let config: McpServerConfig | null = null try { logger.info( `[${requestId}] Discovering tools from server ${serverId} for user ${userId}${attempt > 0 ? ` (attempt ${attempt + 1})` : ''}` ) - const config = await this.getServerConfig(serverId, workspaceId) + config = await this.getServerConfig(serverId, workspaceId) if (!config) { throw new Error(`Server ${serverId} not found or not accessible`) } - authType = config.authType const { config: resolvedConfig, resolvedIP } = await this.resolveConfigEnvVars( config, @@ -793,18 +984,19 @@ class McpService { try { const tools = await client.listTools() logger.info(`[${requestId}] Discovered ${tools.length} tools from server ${config.name}`) - await Promise.allSettled([ - this.cacheAdapter - .set(serverCacheKey(workspaceId, serverId), tools, this.cacheTimeout) - .catch((err) => - logger.warn(`[${requestId}] Cache write failed for ${config.name}:`, err) - ), - this.clearServerFailure(workspaceId, serverId), - this.updateServerStatus(serverId, workspaceId, { - outcome: 'connected', - toolCount: tools.length, - }), - ]) + const published = await this.publishSuccessfulDiscovery( + workspaceId, + resolvedConfig, + mutation, + tools, + discoveryStartedAt + ) + if (!published) { + logger.info( + `[${requestId}] Ignoring superseded discovery result for server ${serverId}` + ) + return [] + } return tools } finally { await client.disconnect() @@ -812,29 +1004,25 @@ class McpService { } catch (error) { if (isRetryableDiscoveryError(error) && attempt < maxRetries - 1) { logger.warn( - `[${requestId}] Transient error discovering tools from server ${serverId}, retrying (attempt ${attempt + 1}):`, - error + `[${requestId}] Transient error discovering tools from server ${serverId}, retrying (attempt ${attempt + 1})`, + { error: getSafeErrorDiagnostics(error) } ) await sleep(backoffWithJitter(attempt + 1, null, { baseMs: 250, maxMs: 2000 })) continue } - // Drop positive cache so a follow-up doesn't return stale tools. - const statusApplied = isOauthAuthorizationError(error, authType) - ? await this.markServerOauthPending(serverId, workspaceId, discoveryStartedAt) - : await this.updateServerStatus(serverId, workspaceId, { - outcome: 'failed', - error: getDiscoveryFailureMessage(error, authType, 'Connection failed'), - discoveryStartedAt, - }) - if (statusApplied) { - await Promise.allSettled([ - this.cacheAdapter - .delete(serverCacheKey(workspaceId, serverId)) - .catch((err) => - logger.warn(`[${requestId}] Cache delete failed for ${serverId}:`, err) - ), - this.markServerUnhealthy(workspaceId, serverId, error, authType), - ]) + if (config) { + if (isOauthAuthorizationError(error, config.authType)) { + await this.publishOauthPending(workspaceId, config, mutation, discoveryStartedAt) + } else { + await this.publishFailedDiscovery( + workspaceId, + config, + mutation, + error, + getDiscoveryFailureMessage(error, config.authType, 'Connection failed'), + discoveryStartedAt + ) + } } throw error } @@ -917,12 +1105,7 @@ class McpService { .select({ id: mcpServers.id }) .from(mcpServers) .where(eq(mcpServers.workspaceId, workspaceId)) - await Promise.allSettled( - rows.flatMap((r) => [ - this.cacheAdapter.delete(serverCacheKey(workspaceId, r.id)), - this.cacheAdapter.delete(failureCacheKey(workspaceId, r.id)), - ]) - ) + await Promise.allSettled(rows.map((row) => this.invalidateServerCache(workspaceId, row.id))) logger.debug(`Cleared MCP tool cache for workspace ${workspaceId} (${rows.length} servers)`) } else { await this.cacheAdapter.clear() diff --git a/apps/sim/lib/mcp/storage/adapter.ts b/apps/sim/lib/mcp/storage/adapter.ts index 9332bb371b5..6449154660c 100644 --- a/apps/sim/lib/mcp/storage/adapter.ts +++ b/apps/sim/lib/mcp/storage/adapter.ts @@ -9,6 +9,20 @@ export interface McpCacheStorageAdapter { get(key: string): Promise set(key: string, tools: McpTool[], ttlMs: number): Promise delete(key: string): Promise + /** + * Starts an ordered mutation for one server. Conditional writes using an + * older mutation id must be ignored so a slow discovery cannot overwrite a + * newer result. + */ + beginMutation(scopeKey: string): Promise + setIfCurrentMutation( + scopeKey: string, + mutationId: number, + key: string, + tools: McpTool[], + ttlMs: number + ): Promise + deleteIfCurrentMutation(scopeKey: string, mutationId: number, key: string): Promise clear(): Promise dispose(): void } diff --git a/apps/sim/lib/mcp/storage/memory-cache.test.ts b/apps/sim/lib/mcp/storage/memory-cache.test.ts index c97944288fd..d48ebc1a7cf 100644 --- a/apps/sim/lib/mcp/storage/memory-cache.test.ts +++ b/apps/sim/lib/mcp/storage/memory-cache.test.ts @@ -165,6 +165,46 @@ describe('MemoryMcpCache', () => { }) }) + describe('ordered mutations', () => { + it('prevents an older discovery from overwriting a newer cache result', async () => { + const older = await cache.beginMutation('server-1') + const newer = await cache.beginMutation('server-1') + + expect( + await cache.setIfCurrentMutation( + 'server-1', + newer, + 'server-1:tools', + [createTool('new-tool')], + 60000 + ) + ).toBe(true) + expect( + await cache.setIfCurrentMutation('server-1', older, 'server-1:failure', [], 60000) + ).toBe(false) + + expect((await cache.get('server-1:tools'))?.tools).toEqual([createTool('new-tool')]) + expect(await cache.get('server-1:failure')).toBeNull() + }) + + it('invalidates in-flight mutations when the cache is cleared', async () => { + const mutation = await cache.beginMutation('server-1') + + await cache.clear() + + expect( + await cache.setIfCurrentMutation( + 'server-1', + mutation, + 'server-1:tools', + [createTool('stale-tool')], + 60000 + ) + ).toBe(false) + expect(await cache.get('server-1:tools')).toBeNull() + }) + }) + describe('clear', () => { it('removes all entries from cache', async () => { const tools = [createTool('tool-1')] diff --git a/apps/sim/lib/mcp/storage/memory-cache.ts b/apps/sim/lib/mcp/storage/memory-cache.ts index b9d54194827..cb82ef9f1c9 100644 --- a/apps/sim/lib/mcp/storage/memory-cache.ts +++ b/apps/sim/lib/mcp/storage/memory-cache.ts @@ -7,6 +7,8 @@ const logger = createLogger('McpMemoryCache') export class MemoryMcpCache implements McpCacheStorageAdapter { private cache = new Map() + private mutationVersions = new Map() + private nextMutationId = 0 private readonly maxCacheSize = MCP_CONSTANTS.MAX_CACHE_SIZE private cleanupInterval: NodeJS.Timeout | null = null @@ -88,8 +90,39 @@ export class MemoryMcpCache implements McpCacheStorageAdapter { this.cache.delete(key) } + async beginMutation(scopeKey: string): Promise { + const mutationId = ++this.nextMutationId + this.mutationVersions.set(scopeKey, mutationId) + return mutationId + } + + async setIfCurrentMutation( + scopeKey: string, + mutationId: number, + key: string, + tools: McpTool[], + ttlMs: number + ): Promise { + if (this.mutationVersions.get(scopeKey) !== mutationId) return false + await this.set(key, tools, ttlMs) + return true + } + + async deleteIfCurrentMutation( + scopeKey: string, + mutationId: number, + key: string + ): Promise { + if (this.mutationVersions.get(scopeKey) !== mutationId) return false + await this.delete(key) + return true + } + async clear(): Promise { this.cache.clear() + for (const scopeKey of this.mutationVersions.keys()) { + this.mutationVersions.set(scopeKey, ++this.nextMutationId) + } } dispose(): void { @@ -98,6 +131,7 @@ export class MemoryMcpCache implements McpCacheStorageAdapter { this.cleanupInterval = null } this.cache.clear() + this.mutationVersions.clear() logger.info('Memory cache disposed') } } diff --git a/apps/sim/lib/mcp/storage/redis-cache.test.ts b/apps/sim/lib/mcp/storage/redis-cache.test.ts new file mode 100644 index 00000000000..b266122791b --- /dev/null +++ b/apps/sim/lib/mcp/storage/redis-cache.test.ts @@ -0,0 +1,92 @@ +/** + * @vitest-environment node + */ +import type Redis from 'ioredis' +import { beforeEach, describe, expect, it, vi } from 'vitest' +import { RedisMcpCache } from '@/lib/mcp/storage/redis-cache' +import type { McpTool } from '@/lib/mcp/types' + +const tool: McpTool = { + name: 'new-tool', + description: 'New tool', + inputSchema: { type: 'object' }, + serverId: 'server-1', + serverName: 'Server 1', +} + +describe('RedisMcpCache ordered mutations', () => { + const multi = { + incr: vi.fn(), + pexpire: vi.fn(), + exec: vi.fn(), + } + const redis = { + multi: vi.fn(() => multi), + eval: vi.fn(), + } + const cache = new RedisMcpCache(redis as unknown as Redis) + + beforeEach(() => { + vi.clearAllMocks() + multi.incr.mockReturnValue(multi) + multi.pexpire.mockReturnValue(multi) + multi.exec.mockResolvedValue([ + [null, 7], + [null, 1], + ]) + }) + + it('allocates a shared per-server mutation id with an expiry', async () => { + await expect(cache.beginMutation('workspace:w:server:s')).resolves.toBe(7) + + expect(multi.incr).toHaveBeenCalledWith('mcp:tools-mutation:workspace:w:server:s') + expect(multi.pexpire).toHaveBeenCalledWith( + 'mcp:tools-mutation:workspace:w:server:s', + 24 * 60 * 60 * 1000 + ) + }) + + it('reports whether a tool cache write still owns the current mutation', async () => { + redis.eval.mockResolvedValueOnce(1).mockResolvedValueOnce(0) + + await expect( + cache.setIfCurrentMutation('workspace:w:server:s', 7, 'workspace:w:server:s', [tool], 60_000) + ).resolves.toBe(true) + await expect( + cache.setIfCurrentMutation( + 'workspace:w:server:s', + 6, + 'workspace:w:server:s:failure', + [], + 60_000 + ) + ).resolves.toBe(false) + + expect(redis.eval).toHaveBeenNthCalledWith( + 1, + expect.stringContaining("redis.call('SET'"), + 2, + 'mcp:tools-mutation:workspace:w:server:s', + 'mcp:tools:workspace:w:server:s', + '7', + expect.stringContaining('new-tool'), + '60000' + ) + }) + + it('guards cache deletion with the same mutation id', async () => { + redis.eval.mockResolvedValueOnce(0) + + await expect( + cache.deleteIfCurrentMutation('workspace:w:server:s', 6, 'workspace:w:server:s:failure') + ).resolves.toBe(false) + + expect(redis.eval).toHaveBeenCalledWith( + expect.stringContaining("redis.call('DEL'"), + 2, + 'mcp:tools-mutation:workspace:w:server:s', + 'mcp:tools:workspace:w:server:s:failure', + '6' + ) + }) +}) diff --git a/apps/sim/lib/mcp/storage/redis-cache.ts b/apps/sim/lib/mcp/storage/redis-cache.ts index f04b9fea119..d989bbeb59d 100644 --- a/apps/sim/lib/mcp/storage/redis-cache.ts +++ b/apps/sim/lib/mcp/storage/redis-cache.ts @@ -6,6 +6,24 @@ import type { McpCacheEntry, McpCacheStorageAdapter } from './adapter' const logger = createLogger('McpRedisCache') const REDIS_KEY_PREFIX = 'mcp:tools:' +const MUTATION_KEY_PREFIX = 'mcp:tools-mutation:' +const MUTATION_TTL_MS = 24 * 60 * 60 * 1000 + +const SET_IF_CURRENT_MUTATION = ` +if redis.call('GET', KEYS[1]) ~= ARGV[1] then + return 0 +end +redis.call('SET', KEYS[2], ARGV[2], 'PX', ARGV[3]) +return 1 +` + +const DELETE_IF_CURRENT_MUTATION = ` +if redis.call('GET', KEYS[1]) ~= ARGV[1] then + return 0 +end +redis.call('DEL', KEYS[2]) +return 1 +` export class RedisMcpCache implements McpCacheStorageAdapter { constructor(private redis: Redis) {} @@ -14,6 +32,10 @@ export class RedisMcpCache implements McpCacheStorageAdapter { return `${REDIS_KEY_PREFIX}${key}` } + private getMutationKey(scopeKey: string): string { + return `${MUTATION_KEY_PREFIX}${scopeKey}` + } + async get(key: string): Promise { try { const redisKey = this.getKey(key) @@ -61,6 +83,72 @@ export class RedisMcpCache implements McpCacheStorageAdapter { } } + async beginMutation(scopeKey: string): Promise { + try { + const mutationKey = this.getMutationKey(scopeKey) + const transaction = this.redis.multi() + transaction.incr(mutationKey) + transaction.pexpire(mutationKey, MUTATION_TTL_MS) + const results = await transaction.exec() + const mutationId = results?.[0]?.[1] + if (typeof mutationId !== 'number') { + throw new Error('Redis did not return an MCP cache mutation id') + } + return mutationId + } catch (error) { + logger.error('Redis cache mutation start error:', error) + throw error + } + } + + async setIfCurrentMutation( + scopeKey: string, + mutationId: number, + key: string, + tools: McpTool[], + ttlMs: number + ): Promise { + try { + const entry: McpCacheEntry = { + tools, + expiry: Date.now() + ttlMs, + } + const result = await this.redis.eval( + SET_IF_CURRENT_MUTATION, + 2, + this.getMutationKey(scopeKey), + this.getKey(key), + String(mutationId), + JSON.stringify(entry), + String(ttlMs) + ) + return result === 1 + } catch (error) { + logger.error('Redis conditional cache set error:', error) + throw error + } + } + + async deleteIfCurrentMutation( + scopeKey: string, + mutationId: number, + key: string + ): Promise { + try { + const result = await this.redis.eval( + DELETE_IF_CURRENT_MUTATION, + 2, + this.getMutationKey(scopeKey), + this.getKey(key), + String(mutationId) + ) + return result === 1 + } catch (error) { + logger.error('Redis conditional cache delete error:', error) + throw error + } + } + async clear(): Promise { try { let cursor = '0' @@ -82,6 +170,26 @@ export class RedisMcpCache implements McpCacheStorageAdapter { } } while (cursor !== '0') + cursor = '0' + do { + const [nextCursor, keys] = await this.redis.scan( + cursor, + 'MATCH', + `${MUTATION_KEY_PREFIX}*`, + 'COUNT', + 100 + ) + cursor = nextCursor + if (keys.length > 0) { + const transaction = this.redis.multi() + for (const key of keys) { + transaction.incr(key) + transaction.pexpire(key, MUTATION_TTL_MS) + } + await transaction.exec() + } + } while (cursor !== '0') + logger.debug(`Cleared ${deletedCount} MCP cache entries from Redis`) } catch (error) { logger.error('Redis cache clear error:', error) From ea7a66b67d78e1bac1c86eb4a2f216e81316ced6 Mon Sep 17 00:00:00 2001 From: Justin Blumencranz <96924014+j15z@users.noreply.github.com> Date: Fri, 17 Jul 2026 18:58:29 -0700 Subject: [PATCH 02/16] Address PR review feedback (#5754) - preserve typed static-header authentication failures\n- order discovery publication by start time\n- atomically publish tools and failure cache state --- apps/sim/lib/mcp/client.test.ts | 3 +- apps/sim/lib/mcp/client.ts | 4 + apps/sim/lib/mcp/service.test.ts | 167 ++++++++++++--- apps/sim/lib/mcp/service.ts | 191 +++++++----------- apps/sim/lib/mcp/storage/adapter.ts | 13 ++ apps/sim/lib/mcp/storage/index.ts | 2 +- apps/sim/lib/mcp/storage/memory-cache.test.ts | 21 ++ apps/sim/lib/mcp/storage/memory-cache.ts | 21 +- apps/sim/lib/mcp/storage/redis-cache.test.ts | 29 +++ apps/sim/lib/mcp/storage/redis-cache.ts | 49 ++++- 10 files changed, 358 insertions(+), 142 deletions(-) diff --git a/apps/sim/lib/mcp/client.test.ts b/apps/sim/lib/mcp/client.test.ts index d8dbd68e8d3..198a67e5c4c 100644 --- a/apps/sim/lib/mcp/client.test.ts +++ b/apps/sim/lib/mcp/client.test.ts @@ -277,12 +277,13 @@ describe('McpClient notification handler', () => { securityPolicy: { requireConsent: false, auditLevel: 'basic' }, }) - await expect(client.connect()).rejects.toThrow('Static token rejected') + await expect(client.connect()).rejects.toBeInstanceOf(UnauthorizedError) expect(mockLogger.error).toHaveBeenCalledWith( expect.stringContaining('Failed to connect'), expect.objectContaining({ outcome: 'unauthorized' }) ) + expect(client.getStatus().lastError).toBe('Authentication failed') }) it('logs tools/list failures without echoed credentials or session identifiers', async () => { diff --git a/apps/sim/lib/mcp/client.ts b/apps/sim/lib/mcp/client.ts index 5473d755a6c..595d1bdb743 100644 --- a/apps/sim/lib/mcp/client.ts +++ b/apps/sim/lib/mcp/client.ts @@ -207,6 +207,10 @@ export class McpClient { this.connectionStatus.lastError = undefined throw error } + if (error instanceof UnauthorizedError) { + this.connectionStatus.lastError = 'Authentication failed' + throw error + } this.connectionStatus.lastError = errorMessage throw new McpConnectionError(errorMessage, this.config.name) } diff --git a/apps/sim/lib/mcp/service.test.ts b/apps/sim/lib/mcp/service.test.ts index bed170d18f8..55544b029f9 100644 --- a/apps/sim/lib/mcp/service.test.ts +++ b/apps/sim/lib/mcp/service.test.ts @@ -70,6 +70,24 @@ const { cacheStore.delete(key) return true }), + applyMutationIfCurrent: vi.fn( + async ( + scopeKey: string, + mutationId: number, + setEntry: { key: string; tools: unknown[]; ttlMs: number } | null, + deleteKeys: string[] + ) => { + if (cacheMutations.get(scopeKey) !== mutationId) return false + if (setEntry) { + cacheStore.set(setEntry.key, { + tools: setEntry.tools, + expiry: Date.now() + setEntry.ttlMs, + }) + } + for (const key of deleteKeys) cacheStore.delete(key) + return true + } + ), clear: vi.fn(async () => { cacheStore.clear() for (const scopeKey of cacheMutations.keys()) { @@ -371,7 +389,7 @@ describe('McpService.discoverTools per-server caching', () => { statusConfig: { consecutiveFailures: 0, lastSuccessfulDiscovery: null }, }), ]) - mockListTools.mockRejectedValueOnce( + mockConnect.mockRejectedValueOnce( new UnauthorizedError(`Rejected Authorization: ${reflectedCredential}`) ) @@ -386,19 +404,23 @@ describe('McpService.discoverTools per-server caching', () => { statusConfig: { consecutiveFailures: 1, lastSuccessfulDiscovery: null }, }) ) - expect(mockCacheAdapter.setIfCurrentMutation).toHaveBeenCalledWith( + expect(mockCacheAdapter.applyMutationIfCurrent).toHaveBeenCalledWith( `workspace:${WORKSPACE_ID}:server:mcp-a`, expect.any(Number), - `workspace:${WORKSPACE_ID}:server:mcp-a:failure`, - [], - expect.any(Number) + { + key: `workspace:${WORKSPACE_ID}:server:mcp-a:failure`, + tools: [], + ttlMs: expect.any(Number), + }, + [`workspace:${WORKSPACE_ID}:server:mcp-a`] ) }) expect(JSON.stringify(mockUpdateSet.mock.calls)).not.toContain(reflectedCredential) - expect(JSON.stringify(mockCacheAdapter.setIfCurrentMutation.mock.calls)).not.toContain( + expect(JSON.stringify(mockCacheAdapter.applyMutationIfCurrent.mock.calls)).not.toContain( reflectedCredential ) expect(JSON.stringify(mockLogger?.warn.mock.calls)).not.toContain(reflectedCredential) + expect(mockListTools).not.toHaveBeenCalled() mockListTools.mockClear() const second = await mcpService.discoverTools(USER_ID, WORKSPACE_ID) @@ -421,12 +443,11 @@ describe('McpService.discoverTools per-server caching', () => { }) ) }) - expect(mockCacheAdapter.setIfCurrentMutation).not.toHaveBeenCalledWith( + expect(mockCacheAdapter.applyMutationIfCurrent).not.toHaveBeenCalledWith( expect.anything(), expect.anything(), - `workspace:${WORKSPACE_ID}:server:mcp-a:failure`, - [], - expect.any(Number) + expect.objectContaining({ key: `workspace:${WORKSPACE_ID}:server:mcp-a:failure` }), + expect.anything() ) mockResolveEnvVars.mockClear() @@ -543,13 +564,107 @@ describe('McpService.discoverTools per-server caching', () => { const tools = await mcpService.discoverTools(USER_ID, WORKSPACE_ID, true) expect(tools).toEqual([]) - expect(mockCacheAdapter.setIfCurrentMutation).not.toHaveBeenCalledWith( - expect.anything(), + expect(mockCacheAdapter.applyMutationIfCurrent).toHaveBeenCalledWith( expect.anything(), - `workspace:${WORKSPACE_ID}:server:mcp-a`, expect.anything(), - expect.any(Number) + { + key: `workspace:${WORKSPACE_ID}:server:mcp-a`, + tools: [tool('stale-tool', 'mcp-a')], + ttlMs: expect.any(Number), + }, + [`workspace:${WORKSPACE_ID}:server:mcp-a:failure`] ) + expect(cacheStore.has(`workspace:${WORKSPACE_ID}:server:mcp-a`)).toBe(false) + }) + + it('orders status publication by discovery start when an older run completes first', async () => { + vi.useFakeTimers({ toFake: ['Date'] }) + try { + mockGetWorkspaceServersRows.mockResolvedValue([dbRow('mcp-a', 'A')]) + mockCacheAdapter.beginMutation + .mockRejectedValueOnce(new Error('cache ordering unavailable')) + .mockRejectedValueOnce(new Error('cache ordering unavailable')) + + let resolveOlder: ((tools: ReturnType[]) => void) | undefined + let resolveNewer: ((tools: ReturnType[]) => void) | undefined + mockListTools + .mockReturnValueOnce( + new Promise((resolve) => { + resolveOlder = resolve + }) + ) + .mockReturnValueOnce( + new Promise((resolve) => { + resolveNewer = resolve + }) + ) + + const olderStartedAt = new Date('2026-02-01T00:00:00.000Z') + vi.setSystemTime(olderStartedAt) + const older = mcpService.discoverServerTools(USER_ID, 'mcp-a', WORKSPACE_ID, false) + await vi.waitFor(() => expect(mockListTools).toHaveBeenCalledTimes(1)) + + const newerStartedAt = new Date('2026-02-01T00:00:01.000Z') + vi.setSystemTime(newerStartedAt) + const newer = mcpService.discoverServerTools(USER_ID, 'mcp-a', WORKSPACE_ID, true) + await vi.waitFor(() => expect(mockListTools).toHaveBeenCalledTimes(2)) + + vi.setSystemTime(new Date('2026-02-01T00:00:02.000Z')) + resolveOlder?.([tool('old-tool', 'mcp-a')]) + await expect(older).resolves.toEqual([tool('old-tool', 'mcp-a')]) + + vi.setSystemTime(new Date('2026-02-01T00:00:03.000Z')) + resolveNewer?.([tool('new-tool', 'mcp-a')]) + await expect(newer).resolves.toEqual([tool('new-tool', 'mcp-a')]) + + const publishedRefreshTimes = mockUpdateSet.mock.calls + .map(([update]) => update) + .filter((update) => update.connectionStatus === 'connected') + .map((update) => update.lastToolsRefresh) + expect(publishedRefreshTimes).toEqual([olderStartedAt, newerStartedAt]) + } finally { + vi.useRealTimers() + } + }) + + it('does not write status after its cache mutation is superseded', async () => { + mockGetWorkspaceServersRows.mockResolvedValue([dbRow('mcp-a', 'A')]) + + let resolveList: ((tools: ReturnType[]) => void) | undefined + mockListTools.mockReturnValueOnce( + new Promise((resolve) => { + resolveList = resolve + }) + ) + + let releaseOlderCacheMutation: (() => void) | undefined + const olderCacheMutationGate = new Promise((resolve) => { + releaseOlderCacheMutation = resolve + }) + const defaultApply = mockCacheAdapter.applyMutationIfCurrent.getMockImplementation() + mockCacheAdapter.applyMutationIfCurrent.mockImplementationOnce( + async ( + scopeKey: string, + mutationId: number, + setEntry: { key: string; tools: unknown[]; ttlMs: number } | null, + deleteKeys: string[] + ) => { + await olderCacheMutationGate + return defaultApply?.(scopeKey, mutationId, setEntry, deleteKeys) ?? false + } + ) + + const discovery = mcpService.discoverServerTools(USER_ID, 'mcp-a', WORKSPACE_ID, false) + await vi.waitFor(() => expect(mockListTools).toHaveBeenCalledTimes(1)) + resolveList?.([tool('superseded-tool', 'mcp-a')]) + await vi.waitFor(() => expect(mockCacheAdapter.applyMutationIfCurrent).toHaveBeenCalledTimes(1)) + + await mockCacheAdapter.beginMutation(`workspace:${WORKSPACE_ID}:server:mcp-a`) + releaseOlderCacheMutation?.() + + await expect(discovery).resolves.toEqual([]) + expect(mockUpdateSet).not.toHaveBeenCalled() + expect(cacheStore.has(`workspace:${WORKSPACE_ID}:server:mcp-a`)).toBe(false) }) it('waits for bulk discovery status publication before returning', async () => { @@ -592,11 +707,16 @@ describe('McpService.discoverTools per-server caching', () => { const olderFailureCacheGate = new Promise((resolve) => { releaseOlderFailureCache = resolve }) - const defaultSet = mockCacheAdapter.setIfCurrentMutation.getMockImplementation() - mockCacheAdapter.setIfCurrentMutation.mockImplementationOnce( - async (scopeKey: string, mutationId: number, key: string, tools: unknown[], ttl: number) => { - if (key.endsWith(':failure')) await olderFailureCacheGate - return defaultSet?.(scopeKey, mutationId, key, tools, ttl) ?? false + const defaultApply = mockCacheAdapter.applyMutationIfCurrent.getMockImplementation() + mockCacheAdapter.applyMutationIfCurrent.mockImplementationOnce( + async ( + scopeKey: string, + mutationId: number, + setEntry: { key: string; tools: unknown[]; ttlMs: number } | null, + deleteKeys: string[] + ) => { + await olderFailureCacheGate + return defaultApply?.(scopeKey, mutationId, setEntry, deleteKeys) ?? false } ) @@ -651,7 +771,7 @@ describe('McpService.discoverTools per-server caching', () => { }) ) expect(JSON.stringify(mockUpdateSet.mock.calls)).not.toContain(reflectedCredential) - expect(JSON.stringify(mockCacheAdapter.setIfCurrentMutation.mock.calls)).not.toContain( + expect(JSON.stringify(mockCacheAdapter.applyMutationIfCurrent.mock.calls)).not.toContain( reflectedCredential ) expect(JSON.stringify(mockLogger?.warn.mock.calls)).not.toContain(reflectedCredential) @@ -677,12 +797,11 @@ describe('McpService.discoverTools per-server caching', () => { lastError: null, }) ) - expect(mockCacheAdapter.setIfCurrentMutation).not.toHaveBeenCalledWith( + expect(mockCacheAdapter.applyMutationIfCurrent).not.toHaveBeenCalledWith( expect.anything(), expect.anything(), - `workspace:${WORKSPACE_ID}:server:mcp-a:failure`, - [], - expect.any(Number) + expect.objectContaining({ key: `workspace:${WORKSPACE_ID}:server:mcp-a:failure` }), + expect.anything() ) mockResolveEnvVars.mockClear() diff --git a/apps/sim/lib/mcp/service.ts b/apps/sim/lib/mcp/service.ts index 9416b36cb91..b706980a7b2 100644 --- a/apps/sim/lib/mcp/service.ts +++ b/apps/sim/lib/mcp/service.ts @@ -27,6 +27,7 @@ import { resolveMcpConfigEnvVars } from '@/lib/mcp/resolve-config' import { createMcpCacheAdapter, getMcpCacheType, + type McpCacheMutationSet, type McpCacheStorageAdapter, } from '@/lib/mcp/storage' import { @@ -65,8 +66,8 @@ type DiscoveryOutcome = } | { kind: 'oauth-pending'; config: McpServerConfig; mutation: CacheMutation | null } | { kind: 'unhealthy' } - // originalError preserves the type so markServerUnhealthy's instanceof - // exemption survives the getErrorMessage call. + // originalError preserves the type so the OAuth exemption survives the + // getErrorMessage call. | { kind: 'error' message: string @@ -436,7 +437,10 @@ class McpService { lastConnected: now, lastError: null, toolCount: update.toolCount, - lastToolsRefresh: now, + // This column is also the publication ordering token. Persist the + // discovery start (rather than finish) so a newer-started run can + // still publish after an older run completes while it is in flight. + lastToolsRefresh: update.discoveryStartedAt, statusConfig: { consecutiveFailures: 0, lastSuccessfulDiscovery: now.toISOString(), @@ -477,7 +481,7 @@ class McpService { connectionStatus: isErrorState ? 'error' : 'disconnected', lastError: update.error || 'Unknown error', toolCount: 0, - lastToolsRefresh: now, + lastToolsRefresh: update.discoveryStartedAt, statusConfig: { consecutiveFailures: newFailures, lastSuccessfulDiscovery: currentConfig.lastSuccessfulDiscovery, @@ -496,31 +500,26 @@ class McpService { } } - /** - * Negative-cache a discovery failure. OAuth-required errors are exempt so - * reconnects retry immediately. - */ - private async markServerUnhealthy( + private async applyServerCacheMutation( workspaceId: string, serverId: string, - error: unknown, - authType: McpServerConfig['authType'], - mutation: CacheMutation | null + mutation: CacheMutation | null, + setEntry: McpCacheMutationSet | null, + deleteKeys: string[] ): Promise { - if (isOauthAuthorizationError(error, authType)) { - return true - } if (!mutation) return true try { - return await this.cacheAdapter.setIfCurrentMutation( + return await this.cacheAdapter.applyMutationIfCurrent( mutation.scopeKey, mutation.id, - failureCacheKey(workspaceId, serverId), - FAILURE_CACHE_SENTINEL, - MCP_CLIENT_CONSTANTS.FAILURE_CACHE_TTL_MS + setEntry, + deleteKeys ) - } catch (err) { - logger.warn(`Failed to write failure cache for server ${serverId}:`, err) + } catch (error) { + logger.warn(`Failed to atomically update cache for server ${serverId}`, { + workspaceId, + error: getSafeErrorDiagnostics(error), + }) return true } } @@ -532,14 +531,13 @@ class McpService { discoveryStartedAt: Date ): Promise { try { - const now = new Date() const updatedServers = await db .update(mcpServers) .set({ connectionStatus: 'disconnected', lastError: null, toolCount: 0, - lastToolsRefresh: now, + lastToolsRefresh: discoveryStartedAt, }) .where( and( @@ -570,24 +568,6 @@ class McpService { } } - private async clearServerFailure( - workspaceId: string, - serverId: string, - mutation: CacheMutation | null - ): Promise { - if (!mutation) return true - try { - return await this.cacheAdapter.deleteIfCurrentMutation( - mutation.scopeKey, - mutation.id, - failureCacheKey(workspaceId, serverId) - ) - } catch (err) { - logger.warn(`Failed to clear failure cache for server ${serverId}:`, err) - return true - } - } - private async beginServerCacheMutation( workspaceId: string, serverId: string @@ -606,63 +586,12 @@ class McpService { private async invalidateServerCache(workspaceId: string, serverId: string): Promise { const mutation = await this.beginServerCacheMutation(workspaceId, serverId) if (!mutation) return - await Promise.all([ - this.cacheAdapter.deleteIfCurrentMutation( - mutation.scopeKey, - mutation.id, - serverCacheKey(workspaceId, serverId) - ), - this.cacheAdapter.deleteIfCurrentMutation( - mutation.scopeKey, - mutation.id, - failureCacheKey(workspaceId, serverId) - ), + await this.applyServerCacheMutation(workspaceId, serverId, mutation, null, [ + serverCacheKey(workspaceId, serverId), + failureCacheKey(workspaceId, serverId), ]) } - private async setServerToolsCache( - workspaceId: string, - serverId: string, - mutation: CacheMutation | null, - tools: McpTool[] - ): Promise { - if (!mutation) return true - try { - return await this.cacheAdapter.setIfCurrentMutation( - mutation.scopeKey, - mutation.id, - serverCacheKey(workspaceId, serverId), - tools, - this.cacheTimeout - ) - } catch (error) { - logger.warn(`Failed to write tool cache for server ${serverId}`, { - error: getSafeErrorDiagnostics(error), - }) - return true - } - } - - private async deleteServerToolsCache( - workspaceId: string, - serverId: string, - mutation: CacheMutation | null - ): Promise { - if (!mutation) return true - try { - return await this.cacheAdapter.deleteIfCurrentMutation( - mutation.scopeKey, - mutation.id, - serverCacheKey(workspaceId, serverId) - ) - } catch (error) { - logger.warn(`Failed to delete tool cache for server ${serverId}`, { - error: getSafeErrorDiagnostics(error), - }) - return true - } - } - private async publishSuccessfulDiscovery( workspaceId: string, config: McpServerConfig, @@ -670,19 +599,35 @@ class McpService { tools: McpTool[], discoveryStartedAt: Date ): Promise { + const cacheApplied = await this.applyServerCacheMutation( + workspaceId, + config.id, + mutation, + { + key: serverCacheKey(workspaceId, config.id), + tools, + ttlMs: this.cacheTimeout, + }, + [failureCacheKey(workspaceId, config.id)] + ) + if (!cacheApplied) return false + const statusApplied = await this.updateServerStatus(config.id, workspaceId, { outcome: 'connected', toolCount: tools.length, configUpdatedAt: config.updatedAt!, discoveryStartedAt, }) - if (!statusApplied) return false - - const [toolsApplied, failureCleared] = await Promise.all([ - this.setServerToolsCache(workspaceId, config.id, mutation, tools), - this.clearServerFailure(workspaceId, config.id, mutation), + if (statusApplied) return true + + // A config change or newer discovery won the database CAS after the cache + // mutation. Remove this result only if its mutation is still current; a + // newer cache publisher must never be disturbed. + await this.applyServerCacheMutation(workspaceId, config.id, mutation, null, [ + serverCacheKey(workspaceId, config.id), + failureCacheKey(workspaceId, config.id), ]) - return toolsApplied && failureCleared + return false } private async publishFailedDiscovery( @@ -693,19 +638,35 @@ class McpService { message: string, discoveryStartedAt: Date ): Promise { + const cacheApplied = await this.applyServerCacheMutation( + workspaceId, + config.id, + mutation, + isOauthAuthorizationError(error, config.authType) + ? null + : { + key: failureCacheKey(workspaceId, config.id), + tools: FAILURE_CACHE_SENTINEL, + ttlMs: MCP_CLIENT_CONSTANTS.FAILURE_CACHE_TTL_MS, + }, + [serverCacheKey(workspaceId, config.id)] + ) + if (!cacheApplied) return false + const statusApplied = await this.updateServerStatus(config.id, workspaceId, { outcome: 'failed', error: message, configUpdatedAt: config.updatedAt!, discoveryStartedAt, }) - if (!statusApplied) return false + if (statusApplied) return true - const [toolsDeleted, failureApplied] = await Promise.all([ - this.deleteServerToolsCache(workspaceId, config.id, mutation), - this.markServerUnhealthy(workspaceId, config.id, error, config.authType, mutation), + // Do not leave a negative-cache entry for a failure that lost the + // database publication CAS. + await this.applyServerCacheMutation(workspaceId, config.id, mutation, null, [ + failureCacheKey(workspaceId, config.id), ]) - return toolsDeleted && failureApplied + return false } private async publishOauthPending( @@ -714,19 +675,21 @@ class McpService { mutation: CacheMutation | null, discoveryStartedAt: Date ): Promise { - const statusApplied = await this.markServerOauthPending( + const cacheApplied = await this.applyServerCacheMutation( + workspaceId, + config.id, + mutation, + null, + [serverCacheKey(workspaceId, config.id), failureCacheKey(workspaceId, config.id)] + ) + if (!cacheApplied) return false + + return this.markServerOauthPending( config.id, workspaceId, config.updatedAt!, discoveryStartedAt ) - if (!statusApplied) return false - - const [toolsDeleted, failureCleared] = await Promise.all([ - this.deleteServerToolsCache(workspaceId, config.id, mutation), - this.clearServerFailure(workspaceId, config.id, mutation), - ]) - return toolsDeleted && failureCleared } async discoverTools( diff --git a/apps/sim/lib/mcp/storage/adapter.ts b/apps/sim/lib/mcp/storage/adapter.ts index 6449154660c..ee56f8f8018 100644 --- a/apps/sim/lib/mcp/storage/adapter.ts +++ b/apps/sim/lib/mcp/storage/adapter.ts @@ -5,6 +5,12 @@ export interface McpCacheEntry { expiry: number // Unix timestamp ms } +export interface McpCacheMutationSet { + key: string + tools: McpTool[] + ttlMs: number +} + export interface McpCacheStorageAdapter { get(key: string): Promise set(key: string, tools: McpTool[], ttlMs: number): Promise @@ -23,6 +29,13 @@ export interface McpCacheStorageAdapter { ttlMs: number ): Promise deleteIfCurrentMutation(scopeKey: string, mutationId: number, key: string): Promise + /** Atomically applies one server's complete cache state if this mutation still owns it. */ + applyMutationIfCurrent( + scopeKey: string, + mutationId: number, + setEntry: McpCacheMutationSet | null, + deleteKeys: string[] + ): Promise clear(): Promise dispose(): void } diff --git a/apps/sim/lib/mcp/storage/index.ts b/apps/sim/lib/mcp/storage/index.ts index 0990173c82d..f361d5d2548 100644 --- a/apps/sim/lib/mcp/storage/index.ts +++ b/apps/sim/lib/mcp/storage/index.ts @@ -1,2 +1,2 @@ -export type { McpCacheStorageAdapter } from './adapter' +export type { McpCacheMutationSet, McpCacheStorageAdapter } from './adapter' export { createMcpCacheAdapter, getMcpCacheType } from './factory' diff --git a/apps/sim/lib/mcp/storage/memory-cache.test.ts b/apps/sim/lib/mcp/storage/memory-cache.test.ts index d48ebc1a7cf..86158c29d03 100644 --- a/apps/sim/lib/mcp/storage/memory-cache.test.ts +++ b/apps/sim/lib/mcp/storage/memory-cache.test.ts @@ -187,6 +187,27 @@ describe('MemoryMcpCache', () => { expect(await cache.get('server-1:failure')).toBeNull() }) + it('atomically replaces tools and failure state for one mutation', async () => { + await cache.set('server-1:failure', [], 60000) + const mutation = await cache.beginMutation('server-1') + + expect( + await cache.applyMutationIfCurrent( + 'server-1', + mutation, + { + key: 'server-1:tools', + tools: [createTool('new-tool')], + ttlMs: 60000, + }, + ['server-1:failure'] + ) + ).toBe(true) + + expect((await cache.get('server-1:tools'))?.tools).toEqual([createTool('new-tool')]) + expect(await cache.get('server-1:failure')).toBeNull() + }) + it('invalidates in-flight mutations when the cache is cleared', async () => { const mutation = await cache.beginMutation('server-1') diff --git a/apps/sim/lib/mcp/storage/memory-cache.ts b/apps/sim/lib/mcp/storage/memory-cache.ts index cb82ef9f1c9..a8b60bfe4a6 100644 --- a/apps/sim/lib/mcp/storage/memory-cache.ts +++ b/apps/sim/lib/mcp/storage/memory-cache.ts @@ -1,7 +1,7 @@ import { createLogger } from '@sim/logger' import type { McpTool } from '@/lib/mcp/types' import { MCP_CONSTANTS } from '@/lib/mcp/utils' -import type { McpCacheEntry, McpCacheStorageAdapter } from './adapter' +import type { McpCacheEntry, McpCacheMutationSet, McpCacheStorageAdapter } from './adapter' const logger = createLogger('McpMemoryCache') @@ -118,6 +118,25 @@ export class MemoryMcpCache implements McpCacheStorageAdapter { return true } + async applyMutationIfCurrent( + scopeKey: string, + mutationId: number, + setEntry: McpCacheMutationSet | null, + deleteKeys: string[] + ): Promise { + if (this.mutationVersions.get(scopeKey) !== mutationId) return false + + if (setEntry) { + this.cache.set(setEntry.key, { + tools: setEntry.tools, + expiry: Date.now() + setEntry.ttlMs, + }) + } + for (const key of deleteKeys) this.cache.delete(key) + this.evictIfNeeded() + return true + } + async clear(): Promise { this.cache.clear() for (const scopeKey of this.mutationVersions.keys()) { diff --git a/apps/sim/lib/mcp/storage/redis-cache.test.ts b/apps/sim/lib/mcp/storage/redis-cache.test.ts index b266122791b..1fbf245e241 100644 --- a/apps/sim/lib/mcp/storage/redis-cache.test.ts +++ b/apps/sim/lib/mcp/storage/redis-cache.test.ts @@ -89,4 +89,33 @@ describe('RedisMcpCache ordered mutations', () => { '6' ) }) + + it('atomically replaces the complete cache state for the current mutation', async () => { + redis.eval.mockResolvedValueOnce(1) + + await expect( + cache.applyMutationIfCurrent( + 'workspace:w:server:s', + 7, + { + key: 'workspace:w:server:s', + tools: [tool], + ttlMs: 60_000, + }, + ['workspace:w:server:s:failure'] + ) + ).resolves.toBe(true) + + expect(redis.eval).toHaveBeenCalledWith( + expect.stringMatching(/redis\.call\('SET'.*redis\.call\('DEL'/s), + 3, + 'mcp:tools-mutation:workspace:w:server:s', + 'mcp:tools:workspace:w:server:s', + 'mcp:tools:workspace:w:server:s:failure', + '7', + '1', + expect.stringContaining('new-tool'), + '60000' + ) + }) }) diff --git a/apps/sim/lib/mcp/storage/redis-cache.ts b/apps/sim/lib/mcp/storage/redis-cache.ts index d989bbeb59d..0c5ddba4a88 100644 --- a/apps/sim/lib/mcp/storage/redis-cache.ts +++ b/apps/sim/lib/mcp/storage/redis-cache.ts @@ -1,7 +1,7 @@ import { createLogger } from '@sim/logger' import type Redis from 'ioredis' import type { McpTool } from '@/lib/mcp/types' -import type { McpCacheEntry, McpCacheStorageAdapter } from './adapter' +import type { McpCacheEntry, McpCacheMutationSet, McpCacheStorageAdapter } from './adapter' const logger = createLogger('McpRedisCache') @@ -25,6 +25,19 @@ redis.call('DEL', KEYS[2]) return 1 ` +const APPLY_MUTATION_IF_CURRENT = ` +if redis.call('GET', KEYS[1]) ~= ARGV[1] then + return 0 +end +if ARGV[2] == '1' then + redis.call('SET', KEYS[2], ARGV[3], 'PX', ARGV[4]) +end +for index = 3, #KEYS do + redis.call('DEL', KEYS[index]) +end +return 1 +` + export class RedisMcpCache implements McpCacheStorageAdapter { constructor(private redis: Redis) {} @@ -149,6 +162,40 @@ export class RedisMcpCache implements McpCacheStorageAdapter { } } + async applyMutationIfCurrent( + scopeKey: string, + mutationId: number, + setEntry: McpCacheMutationSet | null, + deleteKeys: string[] + ): Promise { + try { + const entry = setEntry + ? JSON.stringify({ + tools: setEntry.tools, + expiry: Date.now() + setEntry.ttlMs, + } satisfies McpCacheEntry) + : '' + const keys = [ + this.getMutationKey(scopeKey), + setEntry ? this.getKey(setEntry.key) : this.getMutationKey(scopeKey), + ...deleteKeys.map((key) => this.getKey(key)), + ] + const result = await this.redis.eval( + APPLY_MUTATION_IF_CURRENT, + keys.length, + ...keys, + String(mutationId), + setEntry ? '1' : '0', + entry, + String(setEntry?.ttlMs ?? 0) + ) + return result === 1 + } catch (error) { + logger.error('Redis atomic cache mutation error:', error) + throw error + } + } + async clear(): Promise { try { let cursor = '0' From 2563ab7aa3a0dc8db65dcc9fb2618d4301f019a0 Mon Sep 17 00:00:00 2001 From: Justin Blumencranz <96924014+j15z@users.noreply.github.com> Date: Fri, 17 Jul 2026 19:03:54 -0700 Subject: [PATCH 03/16] Fix MCP discovery timestamp CAS precision (#5754) Match configuration generations within the JavaScript millisecond window so PostgreSQL microseconds do not suppress valid publication. --- apps/sim/lib/mcp/service.test.ts | 19 ++++++++++++++++++- apps/sim/lib/mcp/service.ts | 22 ++++++++++++++++++---- 2 files changed, 36 insertions(+), 5 deletions(-) diff --git a/apps/sim/lib/mcp/service.test.ts b/apps/sim/lib/mcp/service.test.ts index 55544b029f9..8c927a33579 100644 --- a/apps/sim/lib/mcp/service.test.ts +++ b/apps/sim/lib/mcp/service.test.ts @@ -180,7 +180,7 @@ vi.mock('@/lib/mcp/storage', () => ({ getMcpCacheType: () => 'memory', })) -import { mcpService } from '@/lib/mcp/service' +import { getTimestampMillisecondBounds, mcpService } from '@/lib/mcp/service' import { McpOauthAuthorizationRequiredError } from '@/lib/mcp/types' const mockLogger = vi.mocked(loggerMock.createLogger).mock.results.at(-1)?.value @@ -218,6 +218,23 @@ function tool(name: string, serverId: string) { } } +describe('getTimestampMillisecondBounds', () => { + it('includes PostgreSQL sub-millisecond precision but excludes the next millisecond', () => { + const { startInclusive, endExclusive } = getTimestampMillisecondBounds( + '2026-01-01T00:00:00.123Z' + ) + const startMicroseconds = startInclusive.getTime() * 1_000 + const endMicroseconds = endExclusive.getTime() * 1_000 + const isWithinBounds = (candidateMicroseconds: number) => + candidateMicroseconds >= startMicroseconds && candidateMicroseconds < endMicroseconds + + // PostgreSQL can retain any of these extra microseconds even though the + // JavaScript Date used as the generation token is truncated to .123. + expect(isWithinBounds(startMicroseconds + 999)).toBe(true) + expect(isWithinBounds(endMicroseconds)).toBe(false) + }) +}) + describe('McpService.discoverTools per-server caching', () => { beforeEach(async () => { vi.clearAllMocks() diff --git a/apps/sim/lib/mcp/service.ts b/apps/sim/lib/mcp/service.ts index b706980a7b2..fa9e5401908 100644 --- a/apps/sim/lib/mcp/service.ts +++ b/apps/sim/lib/mcp/service.ts @@ -7,7 +7,7 @@ import { createLogger } from '@sim/logger' import { describeError, getErrorMessage } from '@sim/utils/errors' import { sleep } from '@sim/utils/helpers' import { backoffWithJitter } from '@sim/utils/retry' -import { and, eq, isNull, lte, or } from 'drizzle-orm' +import { and, eq, gte, isNull, lt, lte, or } from 'drizzle-orm' import { isTest } from '@/lib/core/config/env-flags' import { generateRequestId } from '@/lib/core/utils/request' import { McpClient } from '@/lib/mcp/client' @@ -53,6 +53,17 @@ function failureCacheKey(workspaceId: string, serverId: string): string { return `workspace:${workspaceId}:server:${serverId}:failure` } +export function getTimestampMillisecondBounds(timestamp: string): { + startInclusive: Date + endExclusive: Date +} { + const startInclusive = new Date(timestamp) + return { + startInclusive, + endExclusive: new Date(startInclusive.getTime() + 1), + } +} + const FAILURE_CACHE_SENTINEL: McpTool[] = [] type DiscoveryOutcome = @@ -417,12 +428,13 @@ class McpService { ): Promise { try { const now = new Date() - const configUpdatedAt = new Date(update.configUpdatedAt) + const configUpdatedAt = getTimestampMillisecondBounds(update.configUpdatedAt) const publicationConditions = and( eq(mcpServers.id, serverId), eq(mcpServers.workspaceId, workspaceId), isNull(mcpServers.deletedAt), - eq(mcpServers.updatedAt, configUpdatedAt), + gte(mcpServers.updatedAt, configUpdatedAt.startInclusive), + lt(mcpServers.updatedAt, configUpdatedAt.endExclusive), or( isNull(mcpServers.lastToolsRefresh), lte(mcpServers.lastToolsRefresh, update.discoveryStartedAt) @@ -531,6 +543,7 @@ class McpService { discoveryStartedAt: Date ): Promise { try { + const configUpdatedAtBounds = getTimestampMillisecondBounds(configUpdatedAt) const updatedServers = await db .update(mcpServers) .set({ @@ -544,7 +557,8 @@ class McpService { eq(mcpServers.id, serverId), eq(mcpServers.workspaceId, workspaceId), isNull(mcpServers.deletedAt), - eq(mcpServers.updatedAt, new Date(configUpdatedAt)), + gte(mcpServers.updatedAt, configUpdatedAtBounds.startInclusive), + lt(mcpServers.updatedAt, configUpdatedAtBounds.endExclusive), or( isNull(mcpServers.lastToolsRefresh), lte(mcpServers.lastToolsRefresh, discoveryStartedAt) From cf88f81214d6d5d8d0d18e272563fd69d7bbcde1 Mon Sep 17 00:00:00 2001 From: Justin Blumencranz <96924014+j15z@users.noreply.github.com> Date: Fri, 17 Jul 2026 19:11:25 -0700 Subject: [PATCH 04/16] Harden MCP cache fallback diagnostics (#5754) - fall back to best-effort cache writes when mutation ordering is unavailable\n- share bounded redacted MCP error diagnostics across client and service --- apps/sim/lib/mcp/client.ts | 20 ++----- apps/sim/lib/mcp/error-diagnostics.test.ts | 39 ++++++++++++++ apps/sim/lib/mcp/error-diagnostics.ts | 25 +++++++++ apps/sim/lib/mcp/service.test.ts | 61 ++++++++++++++++++++++ apps/sim/lib/mcp/service.ts | 60 +++++++++++++++------ 5 files changed, 173 insertions(+), 32 deletions(-) create mode 100644 apps/sim/lib/mcp/error-diagnostics.test.ts create mode 100644 apps/sim/lib/mcp/error-diagnostics.ts diff --git a/apps/sim/lib/mcp/client.ts b/apps/sim/lib/mcp/client.ts index 595d1bdb743..b3e7c815b64 100644 --- a/apps/sim/lib/mcp/client.ts +++ b/apps/sim/lib/mcp/client.ts @@ -9,10 +9,10 @@ import { ToolListChangedNotificationSchema, } from '@modelcontextprotocol/sdk/types.js' import { createLogger } from '@sim/logger' -import { describeError, getErrorMessage } from '@sim/utils/errors' +import { getErrorMessage } from '@sim/utils/errors' import { getMaxExecutionTimeout } from '@/lib/core/execution-limits' import { createPinnedFetch } from '@/lib/core/security/input-validation.server' -import { sanitizeForLogging } from '@/lib/core/security/redaction' +import { getMcpSafeErrorDiagnostics } from '@/lib/mcp/error-diagnostics' import { McpOauthRedirectRequired } from '@/lib/mcp/oauth' import { type McpClientOptions, @@ -60,16 +60,6 @@ function classifyConnectionOutcome( return 'error' } -function getSafeErrorDiagnostics(error: unknown) { - const describedError = describeError(error) - return { - name: sanitizeForLogging(describedError.name, 100), - code: describedError.code ? sanitizeForLogging(describedError.code, 100) : undefined, - errno: describedError.errno ? sanitizeForLogging(describedError.errno, 100) : undefined, - syscall: describedError.syscall ? sanitizeForLogging(describedError.syscall, 100) : undefined, - } -} - interface McpClientConnectOptions { isCancelled?: () => boolean } @@ -127,7 +117,7 @@ export class McpClient { serverId: this.config.id, phase: 'transport', sessionIdPresent: Boolean(this.transport.sessionId), - error: getSafeErrorDiagnostics(error), + error: getMcpSafeErrorDiagnostics(error), }) } } @@ -200,7 +190,7 @@ export class McpClient { logger.error(`Failed to connect to MCP server ${this.config.name}`, { ...diagnostics, durationMs: Date.now() - startedAt, - error: getSafeErrorDiagnostics(error), + error: getMcpSafeErrorDiagnostics(error), outcome, }) if (outcome === 'authorization_required') { @@ -287,7 +277,7 @@ export class McpClient { idleTimeoutMs, maxTotalTimeoutMs, sessionIdPresent: Boolean(this.transport.sessionId), - error: getSafeErrorDiagnostics(error), + error: getMcpSafeErrorDiagnostics(error), }) throw error } diff --git a/apps/sim/lib/mcp/error-diagnostics.test.ts b/apps/sim/lib/mcp/error-diagnostics.test.ts new file mode 100644 index 00000000000..2badd603c05 --- /dev/null +++ b/apps/sim/lib/mcp/error-diagnostics.test.ts @@ -0,0 +1,39 @@ +/** + * @vitest-environment node + */ + +import { describe, expect, it } from 'vitest' +import { getMcpSafeErrorDiagnostics } from './error-diagnostics' + +describe('getMcpSafeErrorDiagnostics', () => { + it('redacts and truncates structural fields without emitting message text', () => { + const secret = 'sk-abcdefghijklmnopqrstuvwxyz1234567890' + const error = Object.assign(new Error(`message contains ${secret}`), { + name: `Bearer ${secret}`, + code: `code-${'c'.repeat(200)}`, + errno: `api_key: "${secret}"`, + syscall: `Bearer ${secret}`, + sessionId: secret, + }) + + const diagnostics = getMcpSafeErrorDiagnostics(error) + + expect(diagnostics).toEqual({ + name: expect.any(String), + code: expect.any(String), + errno: expect.any(String), + syscall: expect.any(String), + }) + expect(diagnostics).not.toHaveProperty('message') + expect(diagnostics).not.toHaveProperty('causeChain') + expect(diagnostics).not.toHaveProperty('sessionId') + for (const value of Object.values(diagnostics)) { + expect(value).not.toContain(secret) + expect(value?.length).toBeLessThanOrEqual(100) + } + expect(diagnostics.name).toContain('[REDACTED]') + expect(diagnostics.errno).toContain('[REDACTED]') + expect(diagnostics.syscall).toContain('[REDACTED]') + expect(diagnostics.code).toHaveLength(100) + }) +}) diff --git a/apps/sim/lib/mcp/error-diagnostics.ts b/apps/sim/lib/mcp/error-diagnostics.ts new file mode 100644 index 00000000000..ce3b6bac7b4 --- /dev/null +++ b/apps/sim/lib/mcp/error-diagnostics.ts @@ -0,0 +1,25 @@ +import { describeError } from '@sim/utils/errors' +import { sanitizeForLogging } from '@/lib/core/security/redaction' + +const MAX_DIAGNOSTIC_FIELD_LENGTH = 100 + +export interface McpSafeErrorDiagnostics { + name: string + code: string | undefined + errno: string | undefined + syscall: string | undefined +} + +/** Returns bounded structural error fields without messages, causes, or session identifiers. */ +export function getMcpSafeErrorDiagnostics(error: unknown): McpSafeErrorDiagnostics { + const described = describeError(error) + const sanitizeOptional = (value: string | undefined) => + value === undefined ? undefined : sanitizeForLogging(value, MAX_DIAGNOSTIC_FIELD_LENGTH) + + return { + name: sanitizeForLogging(described.name, MAX_DIAGNOSTIC_FIELD_LENGTH), + code: sanitizeOptional(described.code), + errno: sanitizeOptional(described.errno), + syscall: sanitizeOptional(described.syscall), + } +} diff --git a/apps/sim/lib/mcp/service.test.ts b/apps/sim/lib/mcp/service.test.ts index 8c927a33579..50359692136 100644 --- a/apps/sim/lib/mcp/service.test.ts +++ b/apps/sim/lib/mcp/service.test.ts @@ -505,6 +505,67 @@ describe('McpService.discoverTools per-server caching', () => { expect(mockListTools).not.toHaveBeenCalled() }) + it('falls back to unordered cache writes when mutation ordering is unavailable', async () => { + const serverKey = `workspace:${WORKSPACE_ID}:server:mcp-a` + const failureKey = `${serverKey}:failure` + mockGetWorkspaceServersRows.mockResolvedValue([dbRow('mcp-a', 'A')]) + cacheStore.set(failureKey, { tools: [], expiry: Date.now() + 60_000 }) + mockCacheAdapter.beginMutation.mockRejectedValueOnce(new Error('cache ordering unavailable')) + mockListTools.mockResolvedValueOnce([tool('a1', 'mcp-a')]) + + const tools = await mcpService.discoverServerTools(USER_ID, 'mcp-a', WORKSPACE_ID, true) + + expect(tools).toEqual([tool('a1', 'mcp-a')]) + expect(cacheStore.get(serverKey)?.tools).toEqual([tool('a1', 'mcp-a')]) + expect(cacheStore.has(failureKey)).toBe(false) + expect(mockCacheAdapter.set).toHaveBeenCalledWith( + serverKey, + [tool('a1', 'mcp-a')], + expect.any(Number) + ) + expect(mockCacheAdapter.delete).toHaveBeenCalledWith(failureKey) + expect(mockUpdateSet).toHaveBeenCalledWith( + expect.objectContaining({ connectionStatus: 'connected', toolCount: 1 }) + ) + }) + + it('publishes database status when both ordered and fallback cache writes fail', async () => { + const reflectedCredential = 'opaque-cache-provider-message' + mockGetWorkspaceServersRows.mockResolvedValue([dbRow('mcp-a', 'A')]) + mockCacheAdapter.beginMutation.mockRejectedValueOnce(new Error(reflectedCredential)) + mockCacheAdapter.set.mockRejectedValueOnce(new Error(reflectedCredential)) + mockCacheAdapter.delete.mockRejectedValueOnce(new Error(reflectedCredential)) + mockListTools.mockResolvedValueOnce([tool('a1', 'mcp-a')]) + + await expect( + mcpService.discoverServerTools(USER_ID, 'mcp-a', WORKSPACE_ID, true) + ).resolves.toEqual([tool('a1', 'mcp-a')]) + + expect(mockUpdateSet).toHaveBeenCalledWith( + expect.objectContaining({ connectionStatus: 'connected', toolCount: 1 }) + ) + expect(JSON.stringify(mockLogger?.warn.mock.calls)).not.toContain(reflectedCredential) + }) + + it('falls back to unordered deletes when cache invalidation cannot be ordered', async () => { + const serverKey = `workspace:${WORKSPACE_ID}:server:mcp-a` + const failureKey = `${serverKey}:failure` + mockGetWorkspaceServersRows.mockResolvedValue([dbRow('mcp-a', 'A')]) + cacheStore.set(serverKey, { + tools: [tool('stale-tool', 'mcp-a')], + expiry: Date.now() + 60_000, + }) + cacheStore.set(failureKey, { tools: [], expiry: Date.now() + 60_000 }) + mockCacheAdapter.beginMutation.mockRejectedValueOnce(new Error('cache ordering unavailable')) + + await mcpService.clearCache(WORKSPACE_ID) + + expect(cacheStore.has(serverKey)).toBe(false) + expect(cacheStore.has(failureKey)).toBe(false) + expect(mockCacheAdapter.delete).toHaveBeenCalledWith(serverKey) + expect(mockCacheAdapter.delete).toHaveBeenCalledWith(failureKey) + }) + it('does not negative-cache OAuth-required errors', async () => { mockGetWorkspaceServersRows.mockResolvedValue([dbRow('mcp-a', 'A')]) mockListTools.mockRejectedValueOnce(new McpOauthAuthorizationRequiredError('mcp-a', 'A')) diff --git a/apps/sim/lib/mcp/service.ts b/apps/sim/lib/mcp/service.ts index fa9e5401908..8e6c94573bd 100644 --- a/apps/sim/lib/mcp/service.ts +++ b/apps/sim/lib/mcp/service.ts @@ -4,7 +4,7 @@ import { ErrorCode, McpError } from '@modelcontextprotocol/sdk/types.js' import { db } from '@sim/db' import { mcpServers } from '@sim/db/schema' import { createLogger } from '@sim/logger' -import { describeError, getErrorMessage } from '@sim/utils/errors' +import { getErrorMessage } from '@sim/utils/errors' import { sleep } from '@sim/utils/helpers' import { backoffWithJitter } from '@sim/utils/retry' import { and, eq, gte, isNull, lt, lte, or } from 'drizzle-orm' @@ -17,6 +17,7 @@ import { validateMcpDomain, validateMcpServerSsrf, } from '@/lib/mcp/domain-check' +import { getMcpSafeErrorDiagnostics } from '@/lib/mcp/error-diagnostics' import { getOrCreateOauthRow, loadPreregisteredClient, @@ -144,16 +145,6 @@ function getDiscoveryFailureMessage( return fallback === 'Unknown error' ? 'Connection failed' : fallback } -function getSafeErrorDiagnostics(error: unknown) { - const described = describeError(error) - return { - name: described.name, - code: described.code, - errno: described.errno, - syscall: described.syscall, - } -} - function isTimeoutError(error: unknown): boolean { if (error instanceof McpError && error.code === ErrorCode.RequestTimeout) { return true @@ -202,7 +193,7 @@ class McpService { this.unsubscribeConnectionManager = mcpConnectionManager.subscribe((event) => { this.invalidateServerCache(event.workspaceId, event.serverId).catch((error) => { logger.warn(`Failed to invalidate cache for ${event.serverName} on listChanged`, { - error: getSafeErrorDiagnostics(error), + error: getMcpSafeErrorDiagnostics(error), }) }) }) @@ -519,7 +510,43 @@ class McpService { setEntry: McpCacheMutationSet | null, deleteKeys: string[] ): Promise { - if (!mutation) return true + if (!mutation) { + const operations = [ + ...(setEntry + ? [ + { + kind: 'set', + run: () => this.cacheAdapter.set(setEntry.key, setEntry.tools, setEntry.ttlMs), + }, + ] + : []), + ...deleteKeys.map((key) => ({ + kind: 'delete', + run: () => this.cacheAdapter.delete(key), + })), + ] + const results = await Promise.allSettled( + operations.map(({ run }) => Promise.resolve().then(run)) + ) + const failedOperations = results.flatMap((result, index) => + result.status === 'rejected' + ? [ + { + kind: operations[index].kind, + error: getMcpSafeErrorDiagnostics(result.reason), + }, + ] + : [] + ) + if (failedOperations.length > 0) { + logger.warn(`Failed to update cache fallback for server ${serverId}`, { + workspaceId, + failedOperations, + }) + } + // Cache availability must not block the authoritative database status. + return true + } try { return await this.cacheAdapter.applyMutationIfCurrent( mutation.scopeKey, @@ -530,7 +557,7 @@ class McpService { } catch (error) { logger.warn(`Failed to atomically update cache for server ${serverId}`, { workspaceId, - error: getSafeErrorDiagnostics(error), + error: getMcpSafeErrorDiagnostics(error), }) return true } @@ -591,7 +618,7 @@ class McpService { return { scopeKey, id: await this.cacheAdapter.beginMutation(scopeKey) } } catch (error) { logger.warn(`Failed to order cache mutation for server ${serverId}`, { - error: getSafeErrorDiagnostics(error), + error: getMcpSafeErrorDiagnostics(error), }) return null } @@ -599,7 +626,6 @@ class McpService { private async invalidateServerCache(workspaceId: string, serverId: string): Promise { const mutation = await this.beginServerCacheMutation(workspaceId, serverId) - if (!mutation) return await this.applyServerCacheMutation(workspaceId, serverId, mutation, null, [ serverCacheKey(workspaceId, serverId), failureCacheKey(workspaceId, serverId), @@ -982,7 +1008,7 @@ class McpService { if (isRetryableDiscoveryError(error) && attempt < maxRetries - 1) { logger.warn( `[${requestId}] Transient error discovering tools from server ${serverId}, retrying (attempt ${attempt + 1})`, - { error: getSafeErrorDiagnostics(error) } + { error: getMcpSafeErrorDiagnostics(error) } ) await sleep(backoffWithJitter(attempt + 1, null, { baseMs: 250, maxMs: 2000 })) continue From 273faffbfa9bbedf375b12e53499eb035f358170 Mon Sep 17 00:00:00 2001 From: Justin Blumencranz <96924014+j15z@users.noreply.github.com> Date: Fri, 17 Jul 2026 19:33:09 -0700 Subject: [PATCH 05/16] Fail MCP discovery closed without cache ownership (#5754) Retry cache mutation ownership before publishing discovery state, avoid unordered publication that older writers can supersede, and keep explicit invalidation best effort. --- apps/sim/lib/mcp/service.test.ts | 126 ++++++++++++++++++++++++++----- apps/sim/lib/mcp/service.ts | 84 ++++++++++----------- 2 files changed, 148 insertions(+), 62 deletions(-) diff --git a/apps/sim/lib/mcp/service.test.ts b/apps/sim/lib/mcp/service.test.ts index 50359692136..a5d30ef9518 100644 --- a/apps/sim/lib/mcp/service.test.ts +++ b/apps/sim/lib/mcp/service.test.ts @@ -505,7 +505,7 @@ describe('McpService.discoverTools per-server caching', () => { expect(mockListTools).not.toHaveBeenCalled() }) - it('falls back to unordered cache writes when mutation ordering is unavailable', async () => { + it('retries mutation ownership before publishing discovery state', async () => { const serverKey = `workspace:${WORKSPACE_ID}:server:mcp-a` const failureKey = `${serverKey}:failure` mockGetWorkspaceServersRows.mockResolvedValue([dbRow('mcp-a', 'A')]) @@ -518,36 +518,123 @@ describe('McpService.discoverTools per-server caching', () => { expect(tools).toEqual([tool('a1', 'mcp-a')]) expect(cacheStore.get(serverKey)?.tools).toEqual([tool('a1', 'mcp-a')]) expect(cacheStore.has(failureKey)).toBe(false) - expect(mockCacheAdapter.set).toHaveBeenCalledWith( + expect(mockCacheAdapter.beginMutation).toHaveBeenCalledTimes(2) + expect(mockCacheAdapter.applyMutationIfCurrent).toHaveBeenCalledWith( serverKey, - [tool('a1', 'mcp-a')], - expect.any(Number) + expect.any(Number), + { key: serverKey, tools: [tool('a1', 'mcp-a')], ttlMs: expect.any(Number) }, + [failureKey] ) - expect(mockCacheAdapter.delete).toHaveBeenCalledWith(failureKey) + expect(mockCacheAdapter.set).not.toHaveBeenCalled() + expect(mockCacheAdapter.delete).not.toHaveBeenCalled() expect(mockUpdateSet).toHaveBeenCalledWith( expect.objectContaining({ connectionStatus: 'connected', toolCount: 1 }) ) }) - it('publishes database status when both ordered and fallback cache writes fail', async () => { + it('keeps an older ordered publisher from superseding a retry-acquired mutation', async () => { + const serverKey = `workspace:${WORKSPACE_ID}:server:mcp-a` + mockGetWorkspaceServersRows.mockResolvedValue([dbRow('mcp-a', 'A')]) + + let resolveOlder: ((tools: ReturnType[]) => void) | undefined + mockListTools + .mockReturnValueOnce( + new Promise((resolve) => { + resolveOlder = resolve + }) + ) + .mockResolvedValueOnce([tool('new-tool', 'mcp-a')]) + + const older = mcpService.discoverServerTools(USER_ID, 'mcp-a', WORKSPACE_ID, false) + await vi.waitFor(() => expect(mockListTools).toHaveBeenCalledTimes(1)) + + mockCacheAdapter.beginMutation.mockRejectedValueOnce(new Error('transient ordering failure')) + const newer = mcpService.discoverServerTools(USER_ID, 'mcp-a', WORKSPACE_ID, true) + await expect(newer).resolves.toEqual([tool('new-tool', 'mcp-a')]) + + resolveOlder?.([tool('old-tool', 'mcp-a')]) + await expect(older).resolves.toEqual([]) + + expect(mockCacheAdapter.beginMutation).toHaveBeenCalledTimes(3) + expect(cacheStore.get(serverKey)?.tools).toEqual([tool('new-tool', 'mcp-a')]) + expect(mockUpdateSet).toHaveBeenCalledTimes(1) + }) + + it('fails a newer discovery closed while an older mutation remains the owner', async () => { + const serverKey = `workspace:${WORKSPACE_ID}:server:mcp-a` + mockGetWorkspaceServersRows.mockResolvedValue([dbRow('mcp-a', 'A')]) + + let resolveOlder: ((tools: ReturnType[]) => void) | undefined + mockListTools + .mockReturnValueOnce( + new Promise((resolve) => { + resolveOlder = resolve + }) + ) + .mockResolvedValueOnce([tool('unowned-new-tool', 'mcp-a')]) + + const older = mcpService.discoverServerTools(USER_ID, 'mcp-a', WORKSPACE_ID, false) + await vi.waitFor(() => expect(mockListTools).toHaveBeenCalledTimes(1)) + + mockCacheAdapter.beginMutation + .mockRejectedValueOnce(new Error('ordering unavailable')) + .mockRejectedValueOnce(new Error('ordering unavailable')) + const newer = mcpService.discoverServerTools(USER_ID, 'mcp-a', WORKSPACE_ID, true) + await expect(newer).resolves.toEqual([]) + + resolveOlder?.([tool('owned-old-tool', 'mcp-a')]) + await expect(older).resolves.toEqual([tool('owned-old-tool', 'mcp-a')]) + + expect(mockCacheAdapter.beginMutation).toHaveBeenCalledTimes(3) + expect(cacheStore.get(serverKey)?.tools).toEqual([tool('owned-old-tool', 'mcp-a')]) + expect(mockCacheAdapter.set).not.toHaveBeenCalled() + expect(mockCacheAdapter.delete).not.toHaveBeenCalled() + expect(mockUpdateSet).toHaveBeenCalledTimes(1) + }) + + it('fails discovery publication closed when mutation ownership stays unavailable', async () => { const reflectedCredential = 'opaque-cache-provider-message' mockGetWorkspaceServersRows.mockResolvedValue([dbRow('mcp-a', 'A')]) - mockCacheAdapter.beginMutation.mockRejectedValueOnce(new Error(reflectedCredential)) - mockCacheAdapter.set.mockRejectedValueOnce(new Error(reflectedCredential)) - mockCacheAdapter.delete.mockRejectedValueOnce(new Error(reflectedCredential)) + mockCacheAdapter.beginMutation + .mockRejectedValueOnce(new Error(reflectedCredential)) + .mockRejectedValueOnce(new Error(reflectedCredential)) mockListTools.mockResolvedValueOnce([tool('a1', 'mcp-a')]) await expect( mcpService.discoverServerTools(USER_ID, 'mcp-a', WORKSPACE_ID, true) - ).resolves.toEqual([tool('a1', 'mcp-a')]) + ).resolves.toEqual([]) - expect(mockUpdateSet).toHaveBeenCalledWith( - expect.objectContaining({ connectionStatus: 'connected', toolCount: 1 }) + expect(mockCacheAdapter.applyMutationIfCurrent).not.toHaveBeenCalled() + expect(mockCacheAdapter.set).not.toHaveBeenCalled() + expect(mockCacheAdapter.delete).not.toHaveBeenCalled() + expect(mockUpdateSet).not.toHaveBeenCalled() + expect(JSON.stringify(mockLogger?.warn.mock.calls)).not.toContain(reflectedCredential) + }) + + it('fails discovery publication closed when the atomic cache transition fails', async () => { + const reflectedCredential = 'opaque-atomic-cache-provider-message' + mockGetWorkspaceServersRows.mockResolvedValue([dbRow('mcp-a', 'A')]) + mockCacheAdapter.applyMutationIfCurrent.mockRejectedValueOnce(new Error(reflectedCredential)) + mockListTools.mockResolvedValueOnce([tool('a1', 'mcp-a')]) + + await expect( + mcpService.discoverServerTools(USER_ID, 'mcp-a', WORKSPACE_ID, true) + ).resolves.toEqual([]) + + expect(mockCacheAdapter.beginMutation).toHaveBeenCalledTimes(1) + expect(mockCacheAdapter.applyMutationIfCurrent).toHaveBeenCalledTimes(1) + expect(mockUpdateSet).not.toHaveBeenCalled() + expect(mockLogger?.warn).toHaveBeenCalledWith( + 'Failed to atomically update cache for server mcp-a', + expect.objectContaining({ + workspaceId: WORKSPACE_ID, + error: expect.objectContaining({ name: 'Error' }), + }) ) expect(JSON.stringify(mockLogger?.warn.mock.calls)).not.toContain(reflectedCredential) }) - it('falls back to unordered deletes when cache invalidation cannot be ordered', async () => { + it('best-effort deletes both cache keys when invalidation cannot be ordered', async () => { const serverKey = `workspace:${WORKSPACE_ID}:server:mcp-a` const failureKey = `${serverKey}:failure` mockGetWorkspaceServersRows.mockResolvedValue([dbRow('mcp-a', 'A')]) @@ -556,7 +643,9 @@ describe('McpService.discoverTools per-server caching', () => { expiry: Date.now() + 60_000, }) cacheStore.set(failureKey, { tools: [], expiry: Date.now() + 60_000 }) - mockCacheAdapter.beginMutation.mockRejectedValueOnce(new Error('cache ordering unavailable')) + mockCacheAdapter.beginMutation + .mockRejectedValueOnce(new Error('cache ordering unavailable')) + .mockRejectedValueOnce(new Error('cache ordering unavailable')) await mcpService.clearCache(WORKSPACE_ID) @@ -655,13 +744,10 @@ describe('McpService.discoverTools per-server caching', () => { expect(cacheStore.has(`workspace:${WORKSPACE_ID}:server:mcp-a`)).toBe(false) }) - it('orders status publication by discovery start when an older run completes first', async () => { + it('supersedes an older discovery before it can publish status', async () => { vi.useFakeTimers({ toFake: ['Date'] }) try { mockGetWorkspaceServersRows.mockResolvedValue([dbRow('mcp-a', 'A')]) - mockCacheAdapter.beginMutation - .mockRejectedValueOnce(new Error('cache ordering unavailable')) - .mockRejectedValueOnce(new Error('cache ordering unavailable')) let resolveOlder: ((tools: ReturnType[]) => void) | undefined let resolveNewer: ((tools: ReturnType[]) => void) | undefined @@ -689,7 +775,7 @@ describe('McpService.discoverTools per-server caching', () => { vi.setSystemTime(new Date('2026-02-01T00:00:02.000Z')) resolveOlder?.([tool('old-tool', 'mcp-a')]) - await expect(older).resolves.toEqual([tool('old-tool', 'mcp-a')]) + await expect(older).resolves.toEqual([]) vi.setSystemTime(new Date('2026-02-01T00:00:03.000Z')) resolveNewer?.([tool('new-tool', 'mcp-a')]) @@ -699,7 +785,7 @@ describe('McpService.discoverTools per-server caching', () => { .map(([update]) => update) .filter((update) => update.connectionStatus === 'connected') .map((update) => update.lastToolsRefresh) - expect(publishedRefreshTimes).toEqual([olderStartedAt, newerStartedAt]) + expect(publishedRefreshTimes).toEqual([newerStartedAt]) } finally { vi.useRealTimers() } diff --git a/apps/sim/lib/mcp/service.ts b/apps/sim/lib/mcp/service.ts index 8e6c94573bd..060f58e80bd 100644 --- a/apps/sim/lib/mcp/service.ts +++ b/apps/sim/lib/mcp/service.ts @@ -66,6 +66,7 @@ export function getTimestampMillisecondBounds(timestamp: string): { } const FAILURE_CACHE_SENTINEL: McpTool[] = [] +const CACHE_MUTATION_BEGIN_ATTEMPTS = 2 type DiscoveryOutcome = | { kind: 'cached'; tools: McpTool[] } @@ -511,41 +512,11 @@ class McpService { deleteKeys: string[] ): Promise { if (!mutation) { - const operations = [ - ...(setEntry - ? [ - { - kind: 'set', - run: () => this.cacheAdapter.set(setEntry.key, setEntry.tools, setEntry.ttlMs), - }, - ] - : []), - ...deleteKeys.map((key) => ({ - kind: 'delete', - run: () => this.cacheAdapter.delete(key), - })), - ] - const results = await Promise.allSettled( - operations.map(({ run }) => Promise.resolve().then(run)) - ) - const failedOperations = results.flatMap((result, index) => - result.status === 'rejected' - ? [ - { - kind: operations[index].kind, - error: getMcpSafeErrorDiagnostics(result.reason), - }, - ] - : [] - ) - if (failedOperations.length > 0) { - logger.warn(`Failed to update cache fallback for server ${serverId}`, { - workspaceId, - failedOperations, - }) - } - // Cache availability must not block the authoritative database status. - return true + // An unordered fallback cannot safely publish discovery state. An older + // ordered mutation could overwrite it and then lose the database CAS, + // while an unguarded delete could erase a newer publisher's result. + // Fail closed so cache and database publication remain one ordered unit. + return false } try { return await this.cacheAdapter.applyMutationIfCurrent( @@ -559,7 +530,7 @@ class McpService { workspaceId, error: getMcpSafeErrorDiagnostics(error), }) - return true + return false } } @@ -614,18 +585,47 @@ class McpService { serverId: string ): Promise { const scopeKey = serverCacheKey(workspaceId, serverId) - try { - return { scopeKey, id: await this.cacheAdapter.beginMutation(scopeKey) } - } catch (error) { - logger.warn(`Failed to order cache mutation for server ${serverId}`, { - error: getMcpSafeErrorDiagnostics(error), + let lastError: unknown + for (let attempt = 0; attempt < CACHE_MUTATION_BEGIN_ATTEMPTS; attempt++) { + try { + return { scopeKey, id: await this.cacheAdapter.beginMutation(scopeKey) } + } catch (error) { + lastError = error + } + } + logger.warn(`Failed to order cache mutation for server ${serverId}`, { + attempts: CACHE_MUTATION_BEGIN_ATTEMPTS, + error: getMcpSafeErrorDiagnostics(lastError), + }) + return null + } + + private async bestEffortInvalidateServerCache( + workspaceId: string, + serverId: string + ): Promise { + const keys = [serverCacheKey(workspaceId, serverId), failureCacheKey(workspaceId, serverId)] + const results = await Promise.allSettled(keys.map((key) => this.cacheAdapter.delete(key))) + const failures = results.flatMap((result) => + result.status === 'rejected' ? [getMcpSafeErrorDiagnostics(result.reason)] : [] + ) + if (failures.length > 0) { + logger.warn(`Failed best-effort cache invalidation for server ${serverId}`, { + workspaceId, + failures, }) - return null } } private async invalidateServerCache(workspaceId: string, serverId: string): Promise { const mutation = await this.beginServerCacheMutation(workspaceId, serverId) + if (!mutation) { + // During a total cache-backend outage there is no cross-process token we + // can advance, so this can only be best effort. Deleting both keys still + // prevents stale reads whenever the backend accepts ordinary commands. + await this.bestEffortInvalidateServerCache(workspaceId, serverId) + return + } await this.applyServerCacheMutation(workspaceId, serverId, mutation, null, [ serverCacheKey(workspaceId, serverId), failureCacheKey(workspaceId, serverId), From 9f5d55414708ac07b940811a13ed50f83f6af536 Mon Sep 17 00:00:00 2001 From: Justin Blumencranz <96924014+j15z@users.noreply.github.com> Date: Fri, 17 Jul 2026 19:41:45 -0700 Subject: [PATCH 06/16] Unify MCP cache and database publication order (#5754) Use timestamp-based mutation tokens for both cache ownership and database CAS ordering, and invalidate Redis mutation owners before deleting entries during a full clear. --- apps/sim/lib/mcp/service.test.ts | 70 ++++++++++++++++--- apps/sim/lib/mcp/service.ts | 67 +++++++----------- apps/sim/lib/mcp/storage/adapter.ts | 7 +- apps/sim/lib/mcp/storage/memory-cache.test.ts | 4 ++ apps/sim/lib/mcp/storage/memory-cache.ts | 9 ++- apps/sim/lib/mcp/storage/redis-cache.test.ts | 40 +++++++++-- apps/sim/lib/mcp/storage/redis-cache.ts | 48 ++++++++----- 7 files changed, 167 insertions(+), 78 deletions(-) diff --git a/apps/sim/lib/mcp/service.test.ts b/apps/sim/lib/mcp/service.test.ts index a5d30ef9518..7e525d23a16 100644 --- a/apps/sim/lib/mcp/service.test.ts +++ b/apps/sim/lib/mcp/service.test.ts @@ -48,7 +48,8 @@ const { cacheStore.delete(key) }), beginMutation: vi.fn(async (scopeKey: string) => { - const mutationId = ++nextMutationId + const mutationId = Math.max(nextMutationId + 1, Date.now()) + nextMutationId = mutationId cacheMutations.set(scopeKey, mutationId) return mutationId }), @@ -89,10 +90,12 @@ const { } ), clear: vi.fn(async () => { - cacheStore.clear() for (const scopeKey of cacheMutations.keys()) { - cacheMutations.set(scopeKey, ++nextMutationId) + const mutationId = Math.max(nextMutationId + 1, Date.now()) + nextMutationId = mutationId + cacheMutations.set(scopeKey, mutationId) } + cacheStore.clear() }), dispose: () => {}, } @@ -560,6 +563,56 @@ describe('McpService.discoverTools per-server caching', () => { expect(mockUpdateSet).toHaveBeenCalledTimes(1) }) + it('uses the cache mutation token to order database publication after a begin retry', async () => { + vi.useFakeTimers({ toFake: ['Date'] }) + try { + mockGetWorkspaceServersRows.mockResolvedValue([dbRow('mcp-a', 'A')]) + + let rejectOlderBegin: ((error: Error) => void) | undefined + mockCacheAdapter.beginMutation.mockImplementationOnce( + () => + new Promise((_resolve, reject) => { + rejectOlderBegin = reject + }) + ) + mockListTools + .mockRejectedValueOnce(new Error('Later-started discovery failed')) + .mockResolvedValueOnce([tool('retry-winner', 'mcp-a')]) + + vi.setSystemTime(new Date('2030-02-01T00:00:00.000Z')) + const older = mcpService.discoverServerTools(USER_ID, 'mcp-a', WORKSPACE_ID, false) + await vi.waitFor(() => expect(mockCacheAdapter.beginMutation).toHaveBeenCalledTimes(1)) + + const laterMutationTime = new Date('2030-02-01T00:00:01.000Z') + vi.setSystemTime(laterMutationTime) + const later = mcpService.discoverServerTools(USER_ID, 'mcp-a', WORKSPACE_ID, true) + await expect(later).rejects.toThrow('Later-started discovery failed') + + const retriedMutationTime = new Date('2030-02-01T00:00:02.000Z') + vi.setSystemTime(retriedMutationTime) + rejectOlderBegin?.(new Error('Transient mutation start failure')) + await expect(older).resolves.toEqual([tool('retry-winner', 'mcp-a')]) + + const publications = mockUpdateSet.mock.calls + .map(([update]) => update) + .filter((update) => update.lastToolsRefresh) + expect(publications.map((update) => update.connectionStatus)).toEqual([ + 'disconnected', + 'connected', + ]) + expect(publications.map((update) => update.lastToolsRefresh)).toEqual([ + laterMutationTime, + retriedMutationTime, + ]) + expect(cacheStore.get(`workspace:${WORKSPACE_ID}:server:mcp-a`)?.tools).toEqual([ + tool('retry-winner', 'mcp-a'), + ]) + expect(cacheStore.has(`workspace:${WORKSPACE_ID}:server:mcp-a:failure`)).toBe(false) + } finally { + vi.useRealTimers() + } + }) + it('fails a newer discovery closed while an older mutation remains the owner', async () => { const serverKey = `workspace:${WORKSPACE_ID}:server:mcp-a` mockGetWorkspaceServersRows.mockResolvedValue([dbRow('mcp-a', 'A')]) @@ -763,21 +816,21 @@ describe('McpService.discoverTools per-server caching', () => { }) ) - const olderStartedAt = new Date('2026-02-01T00:00:00.000Z') + const olderStartedAt = new Date('2030-02-01T00:00:00.000Z') vi.setSystemTime(olderStartedAt) const older = mcpService.discoverServerTools(USER_ID, 'mcp-a', WORKSPACE_ID, false) await vi.waitFor(() => expect(mockListTools).toHaveBeenCalledTimes(1)) - const newerStartedAt = new Date('2026-02-01T00:00:01.000Z') + const newerStartedAt = new Date('2030-02-01T00:00:01.000Z') vi.setSystemTime(newerStartedAt) const newer = mcpService.discoverServerTools(USER_ID, 'mcp-a', WORKSPACE_ID, true) await vi.waitFor(() => expect(mockListTools).toHaveBeenCalledTimes(2)) - vi.setSystemTime(new Date('2026-02-01T00:00:02.000Z')) + vi.setSystemTime(new Date('2030-02-01T00:00:02.000Z')) resolveOlder?.([tool('old-tool', 'mcp-a')]) await expect(older).resolves.toEqual([]) - vi.setSystemTime(new Date('2026-02-01T00:00:03.000Z')) + vi.setSystemTime(new Date('2030-02-01T00:00:03.000Z')) resolveNewer?.([tool('new-tool', 'mcp-a')]) await expect(newer).resolves.toEqual([tool('new-tool', 'mcp-a')]) @@ -785,7 +838,8 @@ describe('McpService.discoverTools per-server caching', () => { .map(([update]) => update) .filter((update) => update.connectionStatus === 'connected') .map((update) => update.lastToolsRefresh) - expect(publishedRefreshTimes).toEqual([newerStartedAt]) + expect(publishedRefreshTimes).toHaveLength(1) + expect(publishedRefreshTimes[0].getTime()).toBeGreaterThanOrEqual(newerStartedAt.getTime()) } finally { vi.useRealTimers() } diff --git a/apps/sim/lib/mcp/service.ts b/apps/sim/lib/mcp/service.ts index 060f58e80bd..29e33d443be 100644 --- a/apps/sim/lib/mcp/service.ts +++ b/apps/sim/lib/mcp/service.ts @@ -99,13 +99,13 @@ type ServerStatusUpdate = outcome: 'connected' toolCount: number configUpdatedAt: string - discoveryStartedAt: Date + publicationOrder: Date } | { outcome: 'failed' error: string configUpdatedAt: string - discoveryStartedAt: Date + publicationOrder: Date } function isOauthAuthorizationError(error: unknown, authType: McpServerConfig['authType']): boolean { @@ -429,7 +429,7 @@ class McpService { lt(mcpServers.updatedAt, configUpdatedAt.endExclusive), or( isNull(mcpServers.lastToolsRefresh), - lte(mcpServers.lastToolsRefresh, update.discoveryStartedAt) + lte(mcpServers.lastToolsRefresh, update.publicationOrder) ) ) @@ -441,10 +441,10 @@ class McpService { lastConnected: now, lastError: null, toolCount: update.toolCount, - // This column is also the publication ordering token. Persist the - // discovery start (rather than finish) so a newer-started run can - // still publish after an older run completes while it is in flight. - lastToolsRefresh: update.discoveryStartedAt, + // The cache mutation id is a monotonic millisecond timestamp. Use + // that same token here so cache and database publication cannot + // choose different winners during begin-mutation retries. + lastToolsRefresh: update.publicationOrder, statusConfig: { consecutiveFailures: 0, lastSuccessfulDiscovery: now.toISOString(), @@ -485,7 +485,7 @@ class McpService { connectionStatus: isErrorState ? 'error' : 'disconnected', lastError: update.error || 'Unknown error', toolCount: 0, - lastToolsRefresh: update.discoveryStartedAt, + lastToolsRefresh: update.publicationOrder, statusConfig: { consecutiveFailures: newFailures, lastSuccessfulDiscovery: currentConfig.lastSuccessfulDiscovery, @@ -538,7 +538,7 @@ class McpService { serverId: string, workspaceId: string, configUpdatedAt: string, - discoveryStartedAt: Date + publicationOrder: Date ): Promise { try { const configUpdatedAtBounds = getTimestampMillisecondBounds(configUpdatedAt) @@ -548,7 +548,7 @@ class McpService { connectionStatus: 'disconnected', lastError: null, toolCount: 0, - lastToolsRefresh: discoveryStartedAt, + lastToolsRefresh: publicationOrder, }) .where( and( @@ -559,7 +559,7 @@ class McpService { lt(mcpServers.updatedAt, configUpdatedAtBounds.endExclusive), or( isNull(mcpServers.lastToolsRefresh), - lte(mcpServers.lastToolsRefresh, discoveryStartedAt) + lte(mcpServers.lastToolsRefresh, publicationOrder) ) ) ) @@ -636,8 +636,7 @@ class McpService { workspaceId: string, config: McpServerConfig, mutation: CacheMutation | null, - tools: McpTool[], - discoveryStartedAt: Date + tools: McpTool[] ): Promise { const cacheApplied = await this.applyServerCacheMutation( workspaceId, @@ -650,13 +649,13 @@ class McpService { }, [failureCacheKey(workspaceId, config.id)] ) - if (!cacheApplied) return false + if (!cacheApplied || !mutation) return false const statusApplied = await this.updateServerStatus(config.id, workspaceId, { outcome: 'connected', toolCount: tools.length, configUpdatedAt: config.updatedAt!, - discoveryStartedAt, + publicationOrder: new Date(mutation.id), }) if (statusApplied) return true @@ -675,8 +674,7 @@ class McpService { config: McpServerConfig, mutation: CacheMutation | null, error: unknown, - message: string, - discoveryStartedAt: Date + message: string ): Promise { const cacheApplied = await this.applyServerCacheMutation( workspaceId, @@ -691,13 +689,13 @@ class McpService { }, [serverCacheKey(workspaceId, config.id)] ) - if (!cacheApplied) return false + if (!cacheApplied || !mutation) return false const statusApplied = await this.updateServerStatus(config.id, workspaceId, { outcome: 'failed', error: message, configUpdatedAt: config.updatedAt!, - discoveryStartedAt, + publicationOrder: new Date(mutation.id), }) if (statusApplied) return true @@ -712,8 +710,7 @@ class McpService { private async publishOauthPending( workspaceId: string, config: McpServerConfig, - mutation: CacheMutation | null, - discoveryStartedAt: Date + mutation: CacheMutation | null ): Promise { const cacheApplied = await this.applyServerCacheMutation( workspaceId, @@ -722,13 +719,13 @@ class McpService { null, [serverCacheKey(workspaceId, config.id), failureCacheKey(workspaceId, config.id)] ) - if (!cacheApplied) return false + if (!cacheApplied || !mutation) return false return this.markServerOauthPending( config.id, workspaceId, config.updatedAt!, - discoveryStartedAt + new Date(mutation.id) ) } @@ -738,8 +735,6 @@ class McpService { forceRefresh = false ): Promise { const requestId = generateRequestId() - const discoveryStartedAt = new Date() - try { logger.info(`[${requestId}] Discovering MCP tools for workspace ${workspaceId}`) @@ -822,8 +817,7 @@ class McpService { workspaceId, outcome.resolvedConfig, outcome.mutation, - outcome.tools, - discoveryStartedAt + outcome.tools ) if (!published) { logger.info( @@ -847,12 +841,7 @@ class McpService { logger.info( `[${requestId}] Skipping server ${server.name}: OAuth authorization pending` ) - await this.publishOauthPending( - workspaceId, - outcome.config, - outcome.mutation, - discoveryStartedAt - ) + await this.publishOauthPending(workspaceId, outcome.config, outcome.mutation) return { tools: [], cached: 0, fetched: 0, failed: 0, liveConnection: null } } if (outcome.kind === 'unhealthy') { @@ -867,8 +856,7 @@ class McpService { outcome.config, outcome.mutation, outcome.originalError, - outcome.message, - discoveryStartedAt + outcome.message ) return { tools: [], cached: 0, fetched: 0, failed: 1, liveConnection: null } }) @@ -941,7 +929,6 @@ class McpService { forceRefresh: boolean ): Promise { const requestId = generateRequestId() - const discoveryStartedAt = new Date() const maxRetries = 2 if (!forceRefresh) { @@ -991,8 +978,7 @@ class McpService { workspaceId, resolvedConfig, mutation, - tools, - discoveryStartedAt + tools ) if (!published) { logger.info( @@ -1015,15 +1001,14 @@ class McpService { } if (config) { if (isOauthAuthorizationError(error, config.authType)) { - await this.publishOauthPending(workspaceId, config, mutation, discoveryStartedAt) + await this.publishOauthPending(workspaceId, config, mutation) } else { await this.publishFailedDiscovery( workspaceId, config, mutation, error, - getDiscoveryFailureMessage(error, config.authType, 'Connection failed'), - discoveryStartedAt + getDiscoveryFailureMessage(error, config.authType, 'Connection failed') ) } } diff --git a/apps/sim/lib/mcp/storage/adapter.ts b/apps/sim/lib/mcp/storage/adapter.ts index ee56f8f8018..a05b0ae0c74 100644 --- a/apps/sim/lib/mcp/storage/adapter.ts +++ b/apps/sim/lib/mcp/storage/adapter.ts @@ -16,9 +16,10 @@ export interface McpCacheStorageAdapter { set(key: string, tools: McpTool[], ttlMs: number): Promise delete(key: string): Promise /** - * Starts an ordered mutation for one server. Conditional writes using an - * older mutation id must be ignored so a slow discovery cannot overwrite a - * newer result. + * Starts an ordered mutation for one server and returns a monotonic Unix + * timestamp in milliseconds. Conditional writes using an older mutation id + * must be ignored so a slow discovery cannot overwrite a newer result. The + * same value orders database publication for an end-to-end consistent state. */ beginMutation(scopeKey: string): Promise setIfCurrentMutation( diff --git a/apps/sim/lib/mcp/storage/memory-cache.test.ts b/apps/sim/lib/mcp/storage/memory-cache.test.ts index 86158c29d03..7f879cbb51a 100644 --- a/apps/sim/lib/mcp/storage/memory-cache.test.ts +++ b/apps/sim/lib/mcp/storage/memory-cache.test.ts @@ -167,9 +167,13 @@ describe('MemoryMcpCache', () => { describe('ordered mutations', () => { it('prevents an older discovery from overwriting a newer cache result', async () => { + const beforeMutation = Date.now() const older = await cache.beginMutation('server-1') const newer = await cache.beginMutation('server-1') + expect(older).toBeGreaterThanOrEqual(beforeMutation) + expect(newer).toBeGreaterThan(older) + expect( await cache.setIfCurrentMutation( 'server-1', diff --git a/apps/sim/lib/mcp/storage/memory-cache.ts b/apps/sim/lib/mcp/storage/memory-cache.ts index a8b60bfe4a6..f2a42a444a1 100644 --- a/apps/sim/lib/mcp/storage/memory-cache.ts +++ b/apps/sim/lib/mcp/storage/memory-cache.ts @@ -91,7 +91,8 @@ export class MemoryMcpCache implements McpCacheStorageAdapter { } async beginMutation(scopeKey: string): Promise { - const mutationId = ++this.nextMutationId + const mutationId = Math.max(this.nextMutationId + 1, Date.now()) + this.nextMutationId = mutationId this.mutationVersions.set(scopeKey, mutationId) return mutationId } @@ -138,10 +139,12 @@ export class MemoryMcpCache implements McpCacheStorageAdapter { } async clear(): Promise { - this.cache.clear() for (const scopeKey of this.mutationVersions.keys()) { - this.mutationVersions.set(scopeKey, ++this.nextMutationId) + const mutationId = Math.max(this.nextMutationId + 1, Date.now()) + this.nextMutationId = mutationId + this.mutationVersions.set(scopeKey, mutationId) } + this.cache.clear() } dispose(): void { diff --git a/apps/sim/lib/mcp/storage/redis-cache.test.ts b/apps/sim/lib/mcp/storage/redis-cache.test.ts index 1fbf245e241..738ee60b35a 100644 --- a/apps/sim/lib/mcp/storage/redis-cache.test.ts +++ b/apps/sim/lib/mcp/storage/redis-cache.test.ts @@ -23,11 +23,16 @@ describe('RedisMcpCache ordered mutations', () => { const redis = { multi: vi.fn(() => multi), eval: vi.fn(), + scan: vi.fn(), + del: vi.fn(), } const cache = new RedisMcpCache(redis as unknown as Redis) beforeEach(() => { vi.clearAllMocks() + redis.eval.mockReset() + redis.scan.mockReset() + redis.del.mockReset() multi.incr.mockReturnValue(multi) multi.pexpire.mockReturnValue(multi) multi.exec.mockResolvedValue([ @@ -36,13 +41,17 @@ describe('RedisMcpCache ordered mutations', () => { ]) }) - it('allocates a shared per-server mutation id with an expiry', async () => { - await expect(cache.beginMutation('workspace:w:server:s')).resolves.toBe(7) + it('allocates a timestamp-based per-server mutation id with an expiry', async () => { + const mutationId = 1_900_000_000_000 + redis.eval.mockResolvedValueOnce(mutationId) - expect(multi.incr).toHaveBeenCalledWith('mcp:tools-mutation:workspace:w:server:s') - expect(multi.pexpire).toHaveBeenCalledWith( + await expect(cache.beginMutation('workspace:w:server:s')).resolves.toBe(mutationId) + + expect(redis.eval).toHaveBeenCalledWith( + expect.stringMatching(/redis\.call\('TIME'\).*math\.max/s), + 1, 'mcp:tools-mutation:workspace:w:server:s', - 24 * 60 * 60 * 1000 + String(24 * 60 * 60 * 1000) ) }) @@ -118,4 +127,25 @@ describe('RedisMcpCache ordered mutations', () => { '60000' ) }) + + it('invalidates mutation owners before deleting entries during a full clear', async () => { + redis.scan + .mockResolvedValueOnce(['0', ['mcp:tools-mutation:workspace:w:server:s']]) + .mockResolvedValueOnce([ + '0', + ['mcp:tools:workspace:w:server:s', 'mcp:tools:workspace:w:server:s:failure'], + ]) + redis.del.mockResolvedValueOnce(2) + + await cache.clear() + + expect(multi.incr).toHaveBeenCalledWith('mcp:tools-mutation:workspace:w:server:s') + expect(redis.del).toHaveBeenCalledWith( + 'mcp:tools:workspace:w:server:s', + 'mcp:tools:workspace:w:server:s:failure' + ) + expect(multi.exec.mock.invocationCallOrder[0]).toBeLessThan( + redis.del.mock.invocationCallOrder[0] + ) + }) }) diff --git a/apps/sim/lib/mcp/storage/redis-cache.ts b/apps/sim/lib/mcp/storage/redis-cache.ts index 0c5ddba4a88..4976dc7cfcf 100644 --- a/apps/sim/lib/mcp/storage/redis-cache.ts +++ b/apps/sim/lib/mcp/storage/redis-cache.ts @@ -9,6 +9,15 @@ const REDIS_KEY_PREFIX = 'mcp:tools:' const MUTATION_KEY_PREFIX = 'mcp:tools-mutation:' const MUTATION_TTL_MS = 24 * 60 * 60 * 1000 +const BEGIN_MUTATION = ` +local current = tonumber(redis.call('GET', KEYS[1]) or '0') +local redisTime = redis.call('TIME') +local now = tonumber(redisTime[1]) * 1000 + math.floor(tonumber(redisTime[2]) / 1000) +local mutationId = math.max(current + 1, now) +redis.call('SET', KEYS[1], tostring(mutationId), 'PX', ARGV[1]) +return mutationId +` + const SET_IF_CURRENT_MUTATION = ` if redis.call('GET', KEYS[1]) ~= ARGV[1] then return 0 @@ -99,11 +108,12 @@ export class RedisMcpCache implements McpCacheStorageAdapter { async beginMutation(scopeKey: string): Promise { try { const mutationKey = this.getMutationKey(scopeKey) - const transaction = this.redis.multi() - transaction.incr(mutationKey) - transaction.pexpire(mutationKey, MUTATION_TTL_MS) - const results = await transaction.exec() - const mutationId = results?.[0]?.[1] + const mutationId = await this.redis.eval( + BEGIN_MUTATION, + 1, + mutationKey, + String(MUTATION_TTL_MS) + ) if (typeof mutationId !== 'number') { throw new Error('Redis did not return an MCP cache mutation id') } @@ -199,41 +209,43 @@ export class RedisMcpCache implements McpCacheStorageAdapter { async clear(): Promise { try { let cursor = '0' - let deletedCount = 0 - + // Invalidate existing mutation owners before deleting their cache + // entries. An old writer either commits before this point and is then + // deleted, or observes the advanced id and cannot commit afterward. do { const [nextCursor, keys] = await this.redis.scan( cursor, 'MATCH', - `${REDIS_KEY_PREFIX}*`, + `${MUTATION_KEY_PREFIX}*`, 'COUNT', 100 ) cursor = nextCursor - if (keys.length > 0) { - await this.redis.del(...keys) - deletedCount += keys.length + const transaction = this.redis.multi() + for (const key of keys) { + transaction.incr(key) + transaction.pexpire(key, MUTATION_TTL_MS) + } + await transaction.exec() } } while (cursor !== '0') cursor = '0' + let deletedCount = 0 do { const [nextCursor, keys] = await this.redis.scan( cursor, 'MATCH', - `${MUTATION_KEY_PREFIX}*`, + `${REDIS_KEY_PREFIX}*`, 'COUNT', 100 ) cursor = nextCursor + if (keys.length > 0) { - const transaction = this.redis.multi() - for (const key of keys) { - transaction.incr(key) - transaction.pexpire(key, MUTATION_TTL_MS) - } - await transaction.exec() + await this.redis.del(...keys) + deletedCount += keys.length } } while (cursor !== '0') From 405e85818312dc379d353546132f2e76d27889c4 Mon Sep 17 00:00:00 2001 From: Justin Blumencranz <96924014+j15z@users.noreply.github.com> Date: Fri, 17 Jul 2026 19:52:09 -0700 Subject: [PATCH 07/16] Keep MCP failure counts and live results consistent (#5754) Recompute consecutive failures after status CAS conflicts, reload winning cached tools for superseded discoveries, and return live tools without unordered publication when cache ordering is unavailable. --- apps/sim/lib/mcp/service.test.ts | 71 +++++++++++-- apps/sim/lib/mcp/service.ts | 170 +++++++++++++++++++------------ 2 files changed, 167 insertions(+), 74 deletions(-) diff --git a/apps/sim/lib/mcp/service.test.ts b/apps/sim/lib/mcp/service.test.ts index 7e525d23a16..3b9fe090802 100644 --- a/apps/sim/lib/mcp/service.test.ts +++ b/apps/sim/lib/mcp/service.test.ts @@ -556,7 +556,7 @@ describe('McpService.discoverTools per-server caching', () => { await expect(newer).resolves.toEqual([tool('new-tool', 'mcp-a')]) resolveOlder?.([tool('old-tool', 'mcp-a')]) - await expect(older).resolves.toEqual([]) + await expect(older).resolves.toEqual([tool('new-tool', 'mcp-a')]) expect(mockCacheAdapter.beginMutation).toHaveBeenCalledTimes(3) expect(cacheStore.get(serverKey)?.tools).toEqual([tool('new-tool', 'mcp-a')]) @@ -613,7 +613,7 @@ describe('McpService.discoverTools per-server caching', () => { } }) - it('fails a newer discovery closed while an older mutation remains the owner', async () => { + it('returns live tools without publishing when cache ownership is unavailable', async () => { const serverKey = `workspace:${WORKSPACE_ID}:server:mcp-a` mockGetWorkspaceServersRows.mockResolvedValue([dbRow('mcp-a', 'A')]) @@ -633,7 +633,7 @@ describe('McpService.discoverTools per-server caching', () => { .mockRejectedValueOnce(new Error('ordering unavailable')) .mockRejectedValueOnce(new Error('ordering unavailable')) const newer = mcpService.discoverServerTools(USER_ID, 'mcp-a', WORKSPACE_ID, true) - await expect(newer).resolves.toEqual([]) + await expect(newer).resolves.toEqual([tool('unowned-new-tool', 'mcp-a')]) resolveOlder?.([tool('owned-old-tool', 'mcp-a')]) await expect(older).resolves.toEqual([tool('owned-old-tool', 'mcp-a')]) @@ -645,7 +645,7 @@ describe('McpService.discoverTools per-server caching', () => { expect(mockUpdateSet).toHaveBeenCalledTimes(1) }) - it('fails discovery publication closed when mutation ownership stays unavailable', async () => { + it('returns live tools but skips publication when mutation ownership stays unavailable', async () => { const reflectedCredential = 'opaque-cache-provider-message' mockGetWorkspaceServersRows.mockResolvedValue([dbRow('mcp-a', 'A')]) mockCacheAdapter.beginMutation @@ -655,7 +655,7 @@ describe('McpService.discoverTools per-server caching', () => { await expect( mcpService.discoverServerTools(USER_ID, 'mcp-a', WORKSPACE_ID, true) - ).resolves.toEqual([]) + ).resolves.toEqual([tool('a1', 'mcp-a')]) expect(mockCacheAdapter.applyMutationIfCurrent).not.toHaveBeenCalled() expect(mockCacheAdapter.set).not.toHaveBeenCalled() @@ -664,7 +664,22 @@ describe('McpService.discoverTools per-server caching', () => { expect(JSON.stringify(mockLogger?.warn.mock.calls)).not.toContain(reflectedCredential) }) - it('fails discovery publication closed when the atomic cache transition fails', async () => { + it('returns bulk live tools without publication when mutation ownership is unavailable', async () => { + mockGetWorkspaceServersRows.mockResolvedValue([dbRow('mcp-a', 'A')]) + mockCacheAdapter.beginMutation + .mockRejectedValueOnce(new Error('cache ordering unavailable')) + .mockRejectedValueOnce(new Error('cache ordering unavailable')) + mockListTools.mockResolvedValueOnce([tool('a1', 'mcp-a')]) + + await expect(mcpService.discoverTools(USER_ID, WORKSPACE_ID, true)).resolves.toEqual([ + tool('a1', 'mcp-a'), + ]) + + expect(mockCacheAdapter.applyMutationIfCurrent).not.toHaveBeenCalled() + expect(mockUpdateSet).not.toHaveBeenCalled() + }) + + it('returns live tools but skips publication when the atomic cache transition fails', async () => { const reflectedCredential = 'opaque-atomic-cache-provider-message' mockGetWorkspaceServersRows.mockResolvedValue([dbRow('mcp-a', 'A')]) mockCacheAdapter.applyMutationIfCurrent.mockRejectedValueOnce(new Error(reflectedCredential)) @@ -672,7 +687,7 @@ describe('McpService.discoverTools per-server caching', () => { await expect( mcpService.discoverServerTools(USER_ID, 'mcp-a', WORKSPACE_ID, true) - ).resolves.toEqual([]) + ).resolves.toEqual([tool('a1', 'mcp-a')]) expect(mockCacheAdapter.beginMutation).toHaveBeenCalledTimes(1) expect(mockCacheAdapter.applyMutationIfCurrent).toHaveBeenCalledTimes(1) @@ -1049,6 +1064,45 @@ describe('McpService.discoverTools per-server caching', () => { ) }) + it('recomputes a failure count after a concurrent status update wins the CAS', async () => { + const beforeSuccess = dbRow('mcp-a', 'A', { + statusConfig: { consecutiveFailures: 2, lastSuccessfulDiscovery: null }, + }) + const afterSuccess = dbRow('mcp-a', 'A', { + statusConfig: { + consecutiveFailures: 0, + lastSuccessfulDiscovery: '2030-02-01T00:00:00.000Z', + }, + }) + mockGetWorkspaceServersRows + .mockResolvedValueOnce([beforeSuccess]) + .mockResolvedValueOnce([beforeSuccess]) + .mockResolvedValueOnce([afterSuccess]) + mockUpdateReturning.mockResolvedValueOnce([]).mockResolvedValueOnce([{ id: 'mcp-a' }]) + mockListTools.mockRejectedValueOnce(new Error('Connection refused')) + + await expect(mcpService.discoverServerTools(USER_ID, 'mcp-a', WORKSPACE_ID)).rejects.toThrow( + 'Connection refused' + ) + + const failureUpdates = mockUpdateSet.mock.calls + .map(([update]) => update) + .filter((update) => update.lastError === 'Connection failed') + expect(failureUpdates).toEqual([ + expect.objectContaining({ + connectionStatus: 'error', + statusConfig: { consecutiveFailures: 3, lastSuccessfulDiscovery: null }, + }), + expect.objectContaining({ + connectionStatus: 'disconnected', + statusConfig: { + consecutiveFailures: 1, + lastSuccessfulDiscovery: '2030-02-01T00:00:00.000Z', + }, + }), + ]) + }) + it('persists OAuth-required discovery as disconnected without a failure error', async () => { mockGetWorkspaceServersRows.mockResolvedValue([dbRow('mcp-a', 'A')]) mockListTools.mockRejectedValueOnce(new McpOauthAuthorizationRequiredError('mcp-a', 'A')) @@ -1068,12 +1122,13 @@ describe('McpService.discoverTools per-server caching', () => { it('does not negative-cache a failure older than a successful discovery', async () => { mockGetWorkspaceServersRows.mockResolvedValue([dbRow('mcp-a', 'A')]) mockListTools.mockRejectedValueOnce(new Error('Older request failed')) - mockUpdateReturning.mockResolvedValueOnce([]) + mockUpdateReturning.mockResolvedValue([]) await expect(mcpService.discoverServerTools(USER_ID, 'mcp-a', WORKSPACE_ID)).rejects.toThrow( 'Older request failed' ) + mockUpdateReturning.mockResolvedValue([{ id: 'server-1' }]) mockListTools.mockResolvedValueOnce([tool('a1', 'mcp-a')]) const tools = await mcpService.discoverServerTools(USER_ID, 'mcp-a', WORKSPACE_ID) diff --git a/apps/sim/lib/mcp/service.ts b/apps/sim/lib/mcp/service.ts index 29e33d443be..0b5c63d2e37 100644 --- a/apps/sim/lib/mcp/service.ts +++ b/apps/sim/lib/mcp/service.ts @@ -67,6 +67,10 @@ export function getTimestampMillisecondBounds(timestamp: string): { const FAILURE_CACHE_SENTINEL: McpTool[] = [] const CACHE_MUTATION_BEGIN_ATTEMPTS = 2 +const STATUS_UPDATE_CAS_ATTEMPTS = 3 + +type CacheMutationResult = 'applied' | 'superseded' | 'unavailable' +type SuccessfulPublicationResult = 'published' | Exclude type DiscoveryOutcome = | { kind: 'cached'; tools: McpTool[] } @@ -455,49 +459,58 @@ class McpService { return updatedServers.length > 0 } - const [currentServer] = await db - .select({ statusConfig: mcpServers.statusConfig }) - .from(mcpServers) - .where( - and( - eq(mcpServers.id, serverId), - eq(mcpServers.workspaceId, workspaceId), - isNull(mcpServers.deletedAt) + for (let attempt = 0; attempt < STATUS_UPDATE_CAS_ATTEMPTS; attempt++) { + const [currentServer] = await db + .select({ statusConfig: mcpServers.statusConfig }) + .from(mcpServers) + .where( + and( + eq(mcpServers.id, serverId), + eq(mcpServers.workspaceId, workspaceId), + isNull(mcpServers.deletedAt) + ) ) - ) - .limit(1) - - const storedConfig = currentServer?.statusConfig as Partial | null - const currentConfig: McpServerStatusConfig = { - consecutiveFailures: - typeof storedConfig?.consecutiveFailures === 'number' - ? storedConfig.consecutiveFailures - : 0, - lastSuccessfulDiscovery: storedConfig?.lastSuccessfulDiscovery ?? null, - } - - const newFailures = currentConfig.consecutiveFailures + 1 - const isErrorState = newFailures >= MCP_CONSTANTS.MAX_CONSECUTIVE_FAILURES + .limit(1) + + if (!currentServer) return false + const storedConfig = currentServer.statusConfig as Partial | null + const currentConfig: McpServerStatusConfig = { + consecutiveFailures: + typeof storedConfig?.consecutiveFailures === 'number' + ? storedConfig.consecutiveFailures + : 0, + lastSuccessfulDiscovery: storedConfig?.lastSuccessfulDiscovery ?? null, + } + const newFailures = currentConfig.consecutiveFailures + 1 + const isErrorState = newFailures >= MCP_CONSTANTS.MAX_CONSECUTIVE_FAILURES + const statusConfigMatches = currentServer.statusConfig + ? eq(mcpServers.statusConfig, currentServer.statusConfig) + : isNull(mcpServers.statusConfig) - const updatedServers = await db - .update(mcpServers) - .set({ - connectionStatus: isErrorState ? 'error' : 'disconnected', - lastError: update.error || 'Unknown error', - toolCount: 0, - lastToolsRefresh: update.publicationOrder, - statusConfig: { - consecutiveFailures: newFailures, - lastSuccessfulDiscovery: currentConfig.lastSuccessfulDiscovery, - }, - }) - .where(publicationConditions) - .returning({ id: mcpServers.id }) + const updatedServers = await db + .update(mcpServers) + .set({ + connectionStatus: isErrorState ? 'error' : 'disconnected', + lastError: update.error || 'Unknown error', + toolCount: 0, + lastToolsRefresh: update.publicationOrder, + statusConfig: { + consecutiveFailures: newFailures, + lastSuccessfulDiscovery: currentConfig.lastSuccessfulDiscovery, + }, + }) + .where(and(publicationConditions, statusConfigMatches)) + .returning({ id: mcpServers.id }) - if (isErrorState && updatedServers.length > 0) { - logger.warn(`Server ${serverId} marked as error after ${newFailures} consecutive failures`) + if (updatedServers.length === 0) continue + if (isErrorState) { + logger.warn( + `Server ${serverId} marked as error after ${newFailures} consecutive failures` + ) + } + return true } - return updatedServers.length > 0 + return false } catch (err) { logger.error(`Failed to update server status for ${serverId}:`, err) return false @@ -510,27 +523,28 @@ class McpService { mutation: CacheMutation | null, setEntry: McpCacheMutationSet | null, deleteKeys: string[] - ): Promise { + ): Promise { if (!mutation) { // An unordered fallback cannot safely publish discovery state. An older // ordered mutation could overwrite it and then lose the database CAS, // while an unguarded delete could erase a newer publisher's result. // Fail closed so cache and database publication remain one ordered unit. - return false + return 'unavailable' } try { - return await this.cacheAdapter.applyMutationIfCurrent( + const applied = await this.cacheAdapter.applyMutationIfCurrent( mutation.scopeKey, mutation.id, setEntry, deleteKeys ) + return applied ? 'applied' : 'superseded' } catch (error) { logger.warn(`Failed to atomically update cache for server ${serverId}`, { workspaceId, error: getMcpSafeErrorDiagnostics(error), }) - return false + return 'unavailable' } } @@ -632,12 +646,27 @@ class McpService { ]) } + private async getCurrentCachedTools( + workspaceId: string, + serverId: string + ): Promise { + try { + return (await this.cacheAdapter.get(serverCacheKey(workspaceId, serverId)))?.tools ?? null + } catch (error) { + logger.warn(`Failed to read current cache winner for server ${serverId}`, { + workspaceId, + error: getMcpSafeErrorDiagnostics(error), + }) + return null + } + } + private async publishSuccessfulDiscovery( workspaceId: string, config: McpServerConfig, mutation: CacheMutation | null, tools: McpTool[] - ): Promise { + ): Promise { const cacheApplied = await this.applyServerCacheMutation( workspaceId, config.id, @@ -649,7 +678,8 @@ class McpService { }, [failureCacheKey(workspaceId, config.id)] ) - if (!cacheApplied || !mutation) return false + if (cacheApplied !== 'applied') return cacheApplied + if (!mutation) return 'unavailable' const statusApplied = await this.updateServerStatus(config.id, workspaceId, { outcome: 'connected', @@ -657,7 +687,7 @@ class McpService { configUpdatedAt: config.updatedAt!, publicationOrder: new Date(mutation.id), }) - if (statusApplied) return true + if (statusApplied) return 'published' // A config change or newer discovery won the database CAS after the cache // mutation. Remove this result only if its mutation is still current; a @@ -666,7 +696,7 @@ class McpService { serverCacheKey(workspaceId, config.id), failureCacheKey(workspaceId, config.id), ]) - return false + return 'superseded' } private async publishFailedDiscovery( @@ -689,7 +719,7 @@ class McpService { }, [serverCacheKey(workspaceId, config.id)] ) - if (!cacheApplied || !mutation) return false + if (cacheApplied !== 'applied' || !mutation) return false const statusApplied = await this.updateServerStatus(config.id, workspaceId, { outcome: 'failed', @@ -719,7 +749,7 @@ class McpService { null, [serverCacheKey(workspaceId, config.id), failureCacheKey(workspaceId, config.id)] ) - if (!cacheApplied || !mutation) return false + if (cacheApplied !== 'applied' || !mutation) return false return this.markServerOauthPending( config.id, @@ -813,28 +843,34 @@ class McpService { } } if (outcome.kind === 'fetched') { - const published = await this.publishSuccessfulDiscovery( + const publication = await this.publishSuccessfulDiscovery( workspaceId, outcome.resolvedConfig, outcome.mutation, outcome.tools ) - if (!published) { + if (publication !== 'published') { logger.info( - `[${requestId}] Ignoring superseded discovery result for server ${server.id}` + `[${requestId}] Discovery state was not published for server ${server.id}`, + { reason: publication } ) } + const responseTools = + publication === 'published' || publication === 'unavailable' + ? outcome.tools + : ((await this.getCurrentCachedTools(workspaceId, server.id)) ?? []) return { - tools: published ? outcome.tools : [], + tools: responseTools, cached: 0, fetched: 1, - failed: published ? 0 : 1, - liveConnection: published - ? { - resolvedConfig: outcome.resolvedConfig, - resolvedIP: outcome.resolvedIP, - } - : null, + failed: publication === 'superseded' && responseTools.length === 0 ? 1 : 0, + liveConnection: + publication === 'published' + ? { + resolvedConfig: outcome.resolvedConfig, + resolvedIP: outcome.resolvedIP, + } + : null, } } if (outcome.kind === 'oauth-pending') { @@ -974,17 +1010,19 @@ class McpService { try { const tools = await client.listTools() logger.info(`[${requestId}] Discovered ${tools.length} tools from server ${config.name}`) - const published = await this.publishSuccessfulDiscovery( + const publication = await this.publishSuccessfulDiscovery( workspaceId, resolvedConfig, mutation, tools ) - if (!published) { - logger.info( - `[${requestId}] Ignoring superseded discovery result for server ${serverId}` - ) - return [] + if (publication !== 'published') { + logger.info(`[${requestId}] Discovery state was not published for server ${serverId}`, { + reason: publication, + }) + if (publication === 'superseded') { + return (await this.getCurrentCachedTools(workspaceId, serverId)) ?? [] + } } return tools } finally { From 29bb919f10007b24dab597faba80bfdbbbc3e546 Mon Sep 17 00:00:00 2001 From: Justin Blumencranz <96924014+j15z@users.noreply.github.com> Date: Fri, 17 Jul 2026 20:02:23 -0700 Subject: [PATCH 08/16] 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. --- .../mcp/servers/[id]/refresh/route.test.ts | 55 +++++++++++++++---- .../app/api/mcp/servers/[id]/refresh/route.ts | 23 ++++++-- apps/sim/lib/mcp/service.test.ts | 14 +++-- apps/sim/lib/mcp/service.ts | 36 ++++++++++-- 4 files changed, 104 insertions(+), 24 deletions(-) diff --git a/apps/sim/app/api/mcp/servers/[id]/refresh/route.test.ts b/apps/sim/app/api/mcp/servers/[id]/refresh/route.test.ts index 72ec98d0087..f8b88850680 100644 --- a/apps/sim/app/api/mcp/servers/[id]/refresh/route.test.ts +++ b/apps/sim/app/api/mcp/servers/[id]/refresh/route.test.ts @@ -43,7 +43,7 @@ vi.mock('@/lib/mcp/middleware', () => ({ vi.mock('@/lib/mcp/service', () => ({ mcpService: { clearCache: mockClearCache, - discoverServerTools: mockDiscoverServerTools, + discoverServerToolsWithMetadata: mockDiscoverServerTools, }, })) @@ -57,6 +57,7 @@ const initialServer = { connectionStatus: 'connected', lastError: null, lastConnected: new Date('2026-01-01T00:00:00.000Z'), + lastToolsRefresh: new Date('2026-01-01T00:00:00.000Z'), toolCount: 4, statusConfig: { consecutiveFailures: 0, lastSuccessfulDiscovery: null }, } @@ -139,6 +140,7 @@ describe('MCP server refresh route', () => { const newerSuccessfulServer = { ...initialServer, lastConnected: new Date(Date.now() + 60_000), + lastToolsRefresh: new Date(Date.now() + 60_000), toolCount: 7, } mockSelect.mockReturnValueOnce(selectRows([newerSuccessfulServer])) @@ -161,15 +163,18 @@ describe('MCP server refresh route', () => { }) it('does not 500 when workflow sync fails after a successful discovery', async () => { - mockDiscoverServerTools.mockResolvedValueOnce([ - { - name: 'search', - description: 'Search tool', - inputSchema: {}, - serverId: 'server-1', - serverName: 'OAuth Server', - }, - ]) + mockDiscoverServerTools.mockResolvedValueOnce({ + tools: [ + { + name: 'search', + description: 'Search tool', + inputSchema: {}, + serverId: 'server-1', + serverName: 'OAuth Server', + }, + ], + state: 'published', + }) // The route's server lookup consumes the first select (beforeEach). The sync's // workflow select is left unmocked, so it throws — exercising the guard that // keeps a secondary sync failure from turning a successful refresh into a 500. @@ -190,4 +195,34 @@ describe('MCP server refresh route', () => { }) ) }) + + it('reports live tools when cache degradation prevents status publication', async () => { + mockDiscoverServerTools.mockResolvedValueOnce({ + tools: [ + { + name: 'search', + description: 'Search tool', + inputSchema: {}, + serverId: 'server-1', + serverName: 'OAuth Server', + }, + ], + state: 'unavailable', + }) + mockSelect.mockReturnValueOnce(selectRows([persistedServer])) + + const request = new Request('http://localhost/api/mcp/servers/server-1/refresh', { + method: 'POST', + }) as NextRequest + const response = await POST(request, { params: Promise.resolve({ id: 'server-1' }) }) + const body = await response.json() + + expect(body.data).toEqual( + expect.objectContaining({ + status: 'connected', + error: null, + toolCount: 1, + }) + ) + }) }) diff --git a/apps/sim/app/api/mcp/servers/[id]/refresh/route.ts b/apps/sim/app/api/mcp/servers/[id]/refresh/route.ts index 87ca976ed9a..d7024bbe1c3 100644 --- a/apps/sim/app/api/mcp/servers/[id]/refresh/route.ts +++ b/apps/sim/app/api/mcp/servers/[id]/refresh/route.ts @@ -9,7 +9,7 @@ import { mcpServerIdParamsSchema } from '@/lib/api/contracts/mcp' import { validationErrorResponse } from '@/lib/api/server' import { withRouteHandler } from '@/lib/core/utils/with-route-handler' import { withMcpAuth } from '@/lib/mcp/middleware' -import { mcpService } from '@/lib/mcp/service' +import { type McpServerDiscoveryState, mcpService } from '@/lib/mcp/service' import type { McpTool, McpToolSchema } from '@/lib/mcp/types' import { categorizeError, @@ -188,16 +188,18 @@ export const POST = withRouteHandler( let syncResult: SyncResult = { updatedCount: 0, updatedWorkflowIds: [] } let discoveredTools: McpTool[] = [] + let discoveryState: McpServerDiscoveryState | null = null let discoveryError: string | null = null - const discoveryStartedAt = new Date() try { - discoveredTools = await mcpService.discoverServerTools( + const discovery = await mcpService.discoverServerToolsWithMetadata( userId, serverId, workspaceId, true ) + discoveredTools = discovery.tools + discoveryState = discovery.state logger.info( `[${requestId}] Discovered ${discoveredTools.length} tools from server ${serverId}` ) @@ -214,6 +216,7 @@ export const POST = withRouteHandler( url: mcpServers.url, connectionStatus: mcpServers.connectionStatus, lastConnected: mcpServers.lastConnected, + lastToolsRefresh: mcpServers.lastToolsRefresh, lastError: mcpServers.lastError, toolCount: mcpServers.toolCount, }) @@ -253,10 +256,20 @@ export const POST = withRouteHandler( let lastError = refreshedServer ? refreshedServer.lastError : discoveryError let toolCount = refreshedServer?.toolCount ?? discoveredTools.length + if ( + discoveryError === null && + (discoveryState === 'unavailable' || discoveryState === 'winner-cache') + ) { + connectionStatus = 'connected' + lastError = null + toolCount = discoveredTools.length + } + if (discoveryError !== null && connectionStatus === 'connected') { const newerSuccessWonRace = - refreshedServer?.lastConnected != null && - refreshedServer.lastConnected > discoveryStartedAt + refreshedServer?.lastToolsRefresh != null && + (server.lastToolsRefresh == null || + refreshedServer.lastToolsRefresh > server.lastToolsRefresh) if (!newerSuccessWonRace) { connectionStatus = 'disconnected' diff --git a/apps/sim/lib/mcp/service.test.ts b/apps/sim/lib/mcp/service.test.ts index 3b9fe090802..d5adc661824 100644 --- a/apps/sim/lib/mcp/service.test.ts +++ b/apps/sim/lib/mcp/service.test.ts @@ -548,7 +548,7 @@ describe('McpService.discoverTools per-server caching', () => { ) .mockResolvedValueOnce([tool('new-tool', 'mcp-a')]) - const older = mcpService.discoverServerTools(USER_ID, 'mcp-a', WORKSPACE_ID, false) + const older = mcpService.discoverServerToolsWithMetadata(USER_ID, 'mcp-a', WORKSPACE_ID, false) await vi.waitFor(() => expect(mockListTools).toHaveBeenCalledTimes(1)) mockCacheAdapter.beginMutation.mockRejectedValueOnce(new Error('transient ordering failure')) @@ -556,7 +556,10 @@ describe('McpService.discoverTools per-server caching', () => { await expect(newer).resolves.toEqual([tool('new-tool', 'mcp-a')]) resolveOlder?.([tool('old-tool', 'mcp-a')]) - await expect(older).resolves.toEqual([tool('new-tool', 'mcp-a')]) + await expect(older).resolves.toEqual({ + tools: [tool('new-tool', 'mcp-a')], + state: 'winner-cache', + }) expect(mockCacheAdapter.beginMutation).toHaveBeenCalledTimes(3) expect(cacheStore.get(serverKey)?.tools).toEqual([tool('new-tool', 'mcp-a')]) @@ -632,8 +635,11 @@ describe('McpService.discoverTools per-server caching', () => { mockCacheAdapter.beginMutation .mockRejectedValueOnce(new Error('ordering unavailable')) .mockRejectedValueOnce(new Error('ordering unavailable')) - const newer = mcpService.discoverServerTools(USER_ID, 'mcp-a', WORKSPACE_ID, true) - await expect(newer).resolves.toEqual([tool('unowned-new-tool', 'mcp-a')]) + const newer = mcpService.discoverServerToolsWithMetadata(USER_ID, 'mcp-a', WORKSPACE_ID, true) + await expect(newer).resolves.toEqual({ + tools: [tool('unowned-new-tool', 'mcp-a')], + state: 'unavailable', + }) resolveOlder?.([tool('owned-old-tool', 'mcp-a')]) await expect(older).resolves.toEqual([tool('owned-old-tool', 'mcp-a')]) diff --git a/apps/sim/lib/mcp/service.ts b/apps/sim/lib/mcp/service.ts index 0b5c63d2e37..509ed6877c8 100644 --- a/apps/sim/lib/mcp/service.ts +++ b/apps/sim/lib/mcp/service.ts @@ -72,6 +72,18 @@ const STATUS_UPDATE_CAS_ATTEMPTS = 3 type CacheMutationResult = 'applied' | 'superseded' | 'unavailable' type SuccessfulPublicationResult = 'published' | Exclude +export type McpServerDiscoveryState = + | 'cached' + | 'published' + | 'winner-cache' + | 'superseded' + | 'unavailable' + +export interface McpServerDiscoveryResult { + tools: McpTool[] + state: McpServerDiscoveryState +} + type DiscoveryOutcome = | { kind: 'cached'; tools: McpTool[] } | { @@ -188,7 +200,7 @@ class McpService { private readonly cacheTimeout = MCP_CONSTANTS.CACHE_TIMEOUT private unsubscribeConnectionManager?: () => void // Keyed on (workspaceId, serverId, userId) — OAuth-scoped tokens vary per user. - private inflightServerDiscovery = new Map>() + private inflightServerDiscovery = new Map>() constructor() { this.cacheAdapter = createMcpCacheAdapter() @@ -942,6 +954,16 @@ class McpService { workspaceId: string, forceRefresh = false ): Promise { + return (await this.discoverServerToolsWithMetadata(userId, serverId, workspaceId, forceRefresh)) + .tools + } + + async discoverServerToolsWithMetadata( + userId: string, + serverId: string, + workspaceId: string, + forceRefresh = false + ): Promise { const inflightKey = `${workspaceId}:${serverId}:${userId}:${forceRefresh ? 'force' : 'cache'}` const existing = this.inflightServerDiscovery.get(inflightKey) if (existing) return existing @@ -963,7 +985,7 @@ class McpService { serverId: string, workspaceId: string, forceRefresh: boolean - ): Promise { + ): Promise { const requestId = generateRequestId() const maxRetries = 2 @@ -972,7 +994,7 @@ class McpService { const cached = await this.cacheAdapter.get(serverCacheKey(workspaceId, serverId)) if (cached) { logger.debug(`[${requestId}] Cache hit for server ${serverId}`) - return cached.tools + return { tools: cached.tools, state: 'cached' } } } catch (error) { logger.warn(`[${requestId}] Cache read failed for server ${serverId}:`, error) @@ -1021,10 +1043,14 @@ class McpService { reason: publication, }) if (publication === 'superseded') { - return (await this.getCurrentCachedTools(workspaceId, serverId)) ?? [] + const winner = await this.getCurrentCachedTools(workspaceId, serverId) + return winner + ? { tools: winner, state: 'winner-cache' } + : { tools: [], state: 'superseded' } } + return { tools, state: 'unavailable' } } - return tools + return { tools, state: 'published' } } finally { await client.disconnect() } From eb9b603911d6d33a7d09bf2b619192b3b58a898f Mon Sep 17 00:00:00 2001 From: Justin Blumencranz <96924014+j15z@users.noreply.github.com> Date: Fri, 17 Jul 2026 20:10:02 -0700 Subject: [PATCH 09/16] Preserve MCP tools across metadata races (#5754) Probe mutation ownership after database CAS misses, keep valid live tools for metadata-only edits, and invalidate ownership for transport and auth-type changes. --- .../lib/mcp/orchestration/server-lifecycle.ts | 2 + apps/sim/lib/mcp/service.test.ts | 38 ++++++++++++++++++- apps/sim/lib/mcp/service.ts | 20 ++++++---- 3 files changed, 50 insertions(+), 10 deletions(-) diff --git a/apps/sim/lib/mcp/orchestration/server-lifecycle.ts b/apps/sim/lib/mcp/orchestration/server-lifecycle.ts index d8d2d73115a..623384b1aa7 100644 --- a/apps/sim/lib/mcp/orchestration/server-lifecycle.ts +++ b/apps/sim/lib/mcp/orchestration/server-lifecycle.ts @@ -384,6 +384,8 @@ export async function performUpdateMcpServer( const shouldClearCache = urlChanged || credsChanged || + params.transport !== undefined || + params.authType !== undefined || params.enabled !== undefined || params.headers !== undefined || params.timeout !== undefined || diff --git a/apps/sim/lib/mcp/service.test.ts b/apps/sim/lib/mcp/service.test.ts index d5adc661824..1a2ae759940 100644 --- a/apps/sim/lib/mcp/service.test.ts +++ b/apps/sim/lib/mcp/service.test.ts @@ -798,8 +798,18 @@ describe('McpService.discoverTools per-server caching', () => { it('does not return or cache tools discovered from a stale server configuration', async () => { mockGetWorkspaceServersRows.mockResolvedValue([dbRow('mcp-a', 'A')]) mockListTools.mockResolvedValueOnce([tool('stale-tool', 'mcp-a')]) + const serverKey = `workspace:${WORKSPACE_ID}:server:mcp-a` mockUpdateSet.mockReturnValueOnce({ - where: vi.fn().mockReturnValue({ returning: vi.fn().mockResolvedValue([]) }), + where: vi.fn().mockReturnValue({ + returning: vi.fn().mockImplementation(async () => { + // Connection-changing edits invalidate both cache entries and advance + // mutation ownership after updating the database row. + await mockCacheAdapter.beginMutation(serverKey) + cacheStore.delete(serverKey) + cacheStore.delete(`${serverKey}:failure`) + return [] + }), + }), }) const tools = await mcpService.discoverTools(USER_ID, WORKSPACE_ID, true) @@ -815,7 +825,31 @@ describe('McpService.discoverTools per-server caching', () => { }, [`workspace:${WORKSPACE_ID}:server:mcp-a:failure`] ) - expect(cacheStore.has(`workspace:${WORKSPACE_ID}:server:mcp-a`)).toBe(false) + expect(cacheStore.has(serverKey)).toBe(false) + }) + + it('keeps valid live tools when a metadata-only edit wins the database CAS', async () => { + const serverKey = `workspace:${WORKSPACE_ID}:server:mcp-a` + mockGetWorkspaceServersRows.mockResolvedValue([dbRow('mcp-a', 'A')]) + mockListTools.mockResolvedValueOnce([tool('still-valid', 'mcp-a')]) + mockUpdateSet.mockReturnValueOnce({ + where: vi.fn().mockReturnValue({ returning: vi.fn().mockResolvedValue([]) }), + }) + + await expect( + mcpService.discoverServerToolsWithMetadata(USER_ID, 'mcp-a', WORKSPACE_ID, true) + ).resolves.toEqual({ + tools: [tool('still-valid', 'mcp-a')], + state: 'unavailable', + }) + + expect(cacheStore.get(serverKey)?.tools).toEqual([tool('still-valid', 'mcp-a')]) + expect(mockCacheAdapter.applyMutationIfCurrent).toHaveBeenLastCalledWith( + serverKey, + expect.any(Number), + null, + [] + ) }) it('supersedes an older discovery before it can publish status', async () => { diff --git a/apps/sim/lib/mcp/service.ts b/apps/sim/lib/mcp/service.ts index 509ed6877c8..7e30cf0b07a 100644 --- a/apps/sim/lib/mcp/service.ts +++ b/apps/sim/lib/mcp/service.ts @@ -701,14 +701,18 @@ class McpService { }) if (statusApplied) return 'published' - // A config change or newer discovery won the database CAS after the cache - // mutation. Remove this result only if its mutation is still current; a - // newer cache publisher must never be disturbed. - await this.applyServerCacheMutation(workspaceId, config.id, mutation, null, [ - serverCacheKey(workspaceId, config.id), - failureCacheKey(workspaceId, config.id), - ]) - return 'superseded' + // A connection-config edit advances mutation ownership, while metadata-only + // edits only bump updatedAt. Probe ownership without changing cache state: + // superseded results must reload the winner, but metadata races can keep + // and return these valid live tools without publishing stale DB status. + const ownership = await this.applyServerCacheMutation( + workspaceId, + config.id, + mutation, + null, + [] + ) + return ownership === 'superseded' ? 'superseded' : 'unavailable' } private async publishFailedDiscovery( From 3df5fc24df704ff754fc9de4fa29ef383b07253a Mon Sep 17 00:00:00 2001 From: Justin Blumencranz <96924014+j15z@users.noreply.github.com> Date: Fri, 17 Jul 2026 20:16:39 -0700 Subject: [PATCH 10/16] Verify MCP config revisions after status races (#5754) Hash connection-affecting server fields and recheck the fresh revision after database CAS misses so metadata edits retain valid tools while URL, credential, and transport changes fail closed. --- apps/sim/lib/mcp/service.test.ts | 25 ++++++------ apps/sim/lib/mcp/service.ts | 69 ++++++++++++++++++++++++++++++-- apps/sim/lib/mcp/types.ts | 2 + 3 files changed, 80 insertions(+), 16 deletions(-) diff --git a/apps/sim/lib/mcp/service.test.ts b/apps/sim/lib/mcp/service.test.ts index 1a2ae759940..aa34b0e4180 100644 --- a/apps/sim/lib/mcp/service.test.ts +++ b/apps/sim/lib/mcp/service.test.ts @@ -796,20 +796,16 @@ describe('McpService.discoverTools per-server caching', () => { }) it('does not return or cache tools discovered from a stale server configuration', async () => { - mockGetWorkspaceServersRows.mockResolvedValue([dbRow('mcp-a', 'A')]) + mockGetWorkspaceServersRows.mockResolvedValueOnce([dbRow('mcp-a', 'A')]).mockResolvedValueOnce([ + dbRow('mcp-a', 'A', { + url: 'https://changed-config.example.com/mcp', + updatedAt: new Date('2026-01-01T00:00:01Z'), + }), + ]) mockListTools.mockResolvedValueOnce([tool('stale-tool', 'mcp-a')]) const serverKey = `workspace:${WORKSPACE_ID}:server:mcp-a` mockUpdateSet.mockReturnValueOnce({ - where: vi.fn().mockReturnValue({ - returning: vi.fn().mockImplementation(async () => { - // Connection-changing edits invalidate both cache entries and advance - // mutation ownership after updating the database row. - await mockCacheAdapter.beginMutation(serverKey) - cacheStore.delete(serverKey) - cacheStore.delete(`${serverKey}:failure`) - return [] - }), - }), + where: vi.fn().mockReturnValue({ returning: vi.fn().mockResolvedValue([]) }), }) const tools = await mcpService.discoverTools(USER_ID, WORKSPACE_ID, true) @@ -830,7 +826,12 @@ describe('McpService.discoverTools per-server caching', () => { it('keeps valid live tools when a metadata-only edit wins the database CAS', async () => { const serverKey = `workspace:${WORKSPACE_ID}:server:mcp-a` - mockGetWorkspaceServersRows.mockResolvedValue([dbRow('mcp-a', 'A')]) + mockGetWorkspaceServersRows.mockResolvedValueOnce([dbRow('mcp-a', 'A')]).mockResolvedValueOnce([ + dbRow('mcp-a', 'Renamed A', { + description: 'Updated display-only description', + updatedAt: new Date('2026-01-01T00:00:01Z'), + }), + ]) mockListTools.mockResolvedValueOnce([tool('still-valid', 'mcp-a')]) mockUpdateSet.mockReturnValueOnce({ where: vi.fn().mockReturnValue({ returning: vi.fn().mockResolvedValue([]) }), diff --git a/apps/sim/lib/mcp/service.ts b/apps/sim/lib/mcp/service.ts index 7e30cf0b07a..27675747e06 100644 --- a/apps/sim/lib/mcp/service.ts +++ b/apps/sim/lib/mcp/service.ts @@ -1,3 +1,4 @@ +import { createHash } from 'node:crypto' import { UnauthorizedError } from '@modelcontextprotocol/sdk/client/auth.js' import { StreamableHTTPError } from '@modelcontextprotocol/sdk/client/streamableHttp.js' import { ErrorCode, McpError } from '@modelcontextprotocol/sdk/types.js' @@ -90,6 +91,7 @@ type DiscoveryOutcome = kind: 'fetched' tools: McpTool[] resolvedConfig: McpServerConfig + sourceConfig: McpServerConfig resolvedIP: string | null mutation: CacheMutation | null } @@ -110,6 +112,42 @@ interface CacheMutation { id: number } +interface DiscoveryRevisionInput { + transport: string | null + url: string | null + authType: string | null + oauthClientId: string | null + oauthClientSecret: string | null + headers: unknown + timeout: number | null + retries: number | null + enabled: boolean +} + +function getDiscoveryRevision(config: DiscoveryRevisionInput): string { + const headers = + config.headers && typeof config.headers === 'object' && !Array.isArray(config.headers) + ? Object.entries(config.headers as Record).sort(([left], [right]) => + left.localeCompare(right) + ) + : [] + return createHash('sha256') + .update( + JSON.stringify({ + transport: config.transport, + url: config.url, + authType: config.authType, + oauthClientId: config.oauthClientId, + oauthClientSecret: config.oauthClientSecret, + headers, + timeout: config.timeout, + retries: config.retries, + enabled: config.enabled, + }) + ) + .digest('hex') +} + type ServerStatusUpdate = | { outcome: 'connected' @@ -275,6 +313,7 @@ class McpService { enabled: server.enabled, createdAt: server.createdAt.toISOString(), updatedAt: server.updatedAt.toISOString(), + discoveryRevision: getDiscoveryRevision(server), } } @@ -305,6 +344,7 @@ class McpService { enabled: server.enabled, createdAt: server.createdAt.toISOString(), updatedAt: server.updatedAt.toISOString(), + discoveryRevision: getDiscoveryRevision(server), })) .filter((config) => isMcpDomainAllowed(config.url)) } @@ -712,7 +752,21 @@ class McpService { null, [] ) - return ownership === 'superseded' ? 'superseded' : 'unavailable' + if (ownership === 'superseded') return 'superseded' + + const currentConfig = await this.getServerConfig(config.id, workspaceId) + if ( + !currentConfig || + !config.discoveryRevision || + currentConfig.discoveryRevision !== config.discoveryRevision + ) { + await this.applyServerCacheMutation(workspaceId, config.id, mutation, null, [ + serverCacheKey(workspaceId, config.id), + failureCacheKey(workspaceId, config.id), + ]) + return 'superseded' + } + return 'unavailable' } private async publishFailedDiscovery( @@ -827,7 +881,14 @@ class McpService { logger.debug( `[${requestId}] Discovered ${tools.length} tools from server ${config.name}` ) - return { kind: 'fetched', tools, resolvedConfig, resolvedIP, mutation } + return { + kind: 'fetched', + tools, + resolvedConfig, + sourceConfig: config, + resolvedIP, + mutation, + } } finally { await client.disconnect() } @@ -861,7 +922,7 @@ class McpService { if (outcome.kind === 'fetched') { const publication = await this.publishSuccessfulDiscovery( workspaceId, - outcome.resolvedConfig, + outcome.sourceConfig, outcome.mutation, outcome.tools ) @@ -1038,7 +1099,7 @@ class McpService { logger.info(`[${requestId}] Discovered ${tools.length} tools from server ${config.name}`) const publication = await this.publishSuccessfulDiscovery( workspaceId, - resolvedConfig, + config, mutation, tools ) diff --git a/apps/sim/lib/mcp/types.ts b/apps/sim/lib/mcp/types.ts index 575e86ea2f8..dd8d4dc51d6 100644 --- a/apps/sim/lib/mcp/types.ts +++ b/apps/sim/lib/mcp/types.ts @@ -30,6 +30,8 @@ export interface McpServerConfig { statusConfig?: McpServerStatusConfig createdAt?: string updatedAt?: string + /** Internal hash of fields that affect discovery; excludes display-only metadata. */ + discoveryRevision?: string } export interface McpVersionInfo { From 0fe18650d6f5a9549e640c5460236721cccb8a11 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Sat, 18 Jul 2026 00:59:10 -0700 Subject: [PATCH 11/16] improvement(mcp): negotiate HTTP/2 for MCP transport MCP servers are commonly behind HTTP/2 fronts (CDNs, cloud LBs), but the SSRF-pinned undici Agent is HTTP/1.1-only unless it opts into h2 via ALPN. Add an allowH2 option to createPinnedFetch (default false, so LLM-provider callers are unchanged) and centralize the MCP h2 decision in one createPinnedMcpFetch helper routed through the transport, the OAuth probe, and the SSRF-guarded discovery/revocation fetch. Pinning is unaffected: the pinned lookup forces every connection to the resolved IP regardless of the negotiated protocol. --- .../lib/core/security/input-validation.server.ts | 16 ++++++++++++++-- .../core/security/pinned-fetch.server.test.ts | 12 ++++++++++++ apps/sim/lib/mcp/client.ts | 4 ++-- apps/sim/lib/mcp/oauth/probe.test.ts | 4 +--- apps/sim/lib/mcp/oauth/probe.ts | 5 ++--- apps/sim/lib/mcp/pinned-fetch.test.ts | 4 ++-- apps/sim/lib/mcp/pinned-fetch.ts | 14 +++++++++++++- 7 files changed, 46 insertions(+), 13 deletions(-) diff --git a/apps/sim/lib/core/security/input-validation.server.ts b/apps/sim/lib/core/security/input-validation.server.ts index 7f81039622c..e3074a2accd 100644 --- a/apps/sim/lib/core/security/input-validation.server.ts +++ b/apps/sim/lib/core/security/input-validation.server.ts @@ -433,9 +433,21 @@ export function createPinnedLookup(resolvedIP: string): LookupFunction { * * The `Agent` is captured for the lifetime of the returned function, so repeated * calls (e.g. a provider tool loop) reuse its keep-alive connections. + * + * `allowH2` opts the pinned Agent into HTTP/2 (ALPN-negotiated, h1.1 fallback). + * It defaults to `false` to leave existing consumers unchanged. Enabling it does + * not weaken pinning: the pinned `connect.lookup` forces every connection on the + * Agent to `resolvedIP` regardless of authority, so h2 connection coalescing can + * never reach an address other than the validated one. */ -export function createPinnedFetch(resolvedIP: string): typeof fetch { - const dispatcher = new Agent({ connect: { lookup: createPinnedLookup(resolvedIP) } }) +export function createPinnedFetch( + resolvedIP: string, + options?: { allowH2?: boolean } +): typeof fetch { + const dispatcher = new Agent({ + allowH2: options?.allowH2 ?? false, + connect: { lookup: createPinnedLookup(resolvedIP) }, + }) const pinned = async (input: RequestInfo | URL, init?: RequestInit): Promise => { // double-cast-allowed: DOM RequestInfo/URL and undici fetch input types differ but are structurally compatible at runtime (Node's global fetch IS undici) diff --git a/apps/sim/lib/core/security/pinned-fetch.server.test.ts b/apps/sim/lib/core/security/pinned-fetch.server.test.ts index e07d0d412dd..0ecfa22cb9e 100644 --- a/apps/sim/lib/core/security/pinned-fetch.server.test.ts +++ b/apps/sim/lib/core/security/pinned-fetch.server.test.ts @@ -55,6 +55,18 @@ describe('createPinnedFetch', () => { expect(resolved).toEqual({ address: '203.0.113.10', family: 4 }) }) + it('defaults allowH2 to false so existing consumers are unchanged', () => { + createPinnedFetch('203.0.113.10') + const opts = capturedAgentOptions[0] as { allowH2?: boolean } + expect(opts.allowH2).toBe(false) + }) + + it('opts the Agent into HTTP/2 when allowH2 is requested', () => { + createPinnedFetch('203.0.113.10', { allowH2: true }) + const opts = capturedAgentOptions[0] as { allowH2?: boolean } + expect(opts.allowH2).toBe(true) + }) + it('uses IPv6 family when the validated IP is IPv6', async () => { createPinnedFetch('2606:4700:4700::1111') const { connect } = capturedAgentOptions[0] as { connect: { lookup: PinnedLookup } } diff --git a/apps/sim/lib/mcp/client.ts b/apps/sim/lib/mcp/client.ts index b3e7c815b64..bc3d32c6dde 100644 --- a/apps/sim/lib/mcp/client.ts +++ b/apps/sim/lib/mcp/client.ts @@ -11,9 +11,9 @@ import { import { createLogger } from '@sim/logger' import { getErrorMessage } from '@sim/utils/errors' import { getMaxExecutionTimeout } from '@/lib/core/execution-limits' -import { createPinnedFetch } from '@/lib/core/security/input-validation.server' import { getMcpSafeErrorDiagnostics } from '@/lib/mcp/error-diagnostics' import { McpOauthRedirectRequired } from '@/lib/mcp/oauth' +import { createPinnedMcpFetch } from '@/lib/mcp/pinned-fetch' import { type McpClientOptions, McpConnectionError, @@ -98,7 +98,7 @@ export class McpClient { this.transport = new StreamableHTTPClientTransport(new URL(this.config.url), { authProvider: useOauth ? this.authProvider : undefined, requestInit: { headers: this.config.headers }, - ...(resolvedIP ? { fetch: createPinnedFetch(resolvedIP) } : {}), + ...(resolvedIP ? { fetch: createPinnedMcpFetch(resolvedIP) } : {}), }) this.client = new Client( diff --git a/apps/sim/lib/mcp/oauth/probe.test.ts b/apps/sim/lib/mcp/oauth/probe.test.ts index d691f1178c7..ff5d96525a7 100644 --- a/apps/sim/lib/mcp/oauth/probe.test.ts +++ b/apps/sim/lib/mcp/oauth/probe.test.ts @@ -15,10 +15,8 @@ const { mockCreatePinnedFetch, mockCreateSsrfGuardedMcpFetch, mockPinnedFetch, m } }) -vi.mock('@/lib/core/security/input-validation.server', () => ({ - createPinnedFetch: mockCreatePinnedFetch, -})) vi.mock('@/lib/mcp/pinned-fetch', () => ({ + createPinnedMcpFetch: mockCreatePinnedFetch, createSsrfGuardedMcpFetch: mockCreateSsrfGuardedMcpFetch, })) diff --git a/apps/sim/lib/mcp/oauth/probe.ts b/apps/sim/lib/mcp/oauth/probe.ts index 4343f27870a..431480ae0f6 100644 --- a/apps/sim/lib/mcp/oauth/probe.ts +++ b/apps/sim/lib/mcp/oauth/probe.ts @@ -1,9 +1,8 @@ import { extractWWWAuthenticateParams } from '@modelcontextprotocol/sdk/client/auth.js' import type { FetchLike } from '@modelcontextprotocol/sdk/shared/transport.js' import { createLogger } from '@sim/logger' -import { createPinnedFetch } from '@/lib/core/security/input-validation.server' import { isLoopbackHostname } from '@/lib/core/utils/urls' -import { createSsrfGuardedMcpFetch } from '@/lib/mcp/pinned-fetch' +import { createPinnedMcpFetch, createSsrfGuardedMcpFetch } from '@/lib/mcp/pinned-fetch' import type { McpAuthType } from '@/lib/mcp/types' const logger = createLogger('McpOauthProbe') @@ -34,7 +33,7 @@ export async function detectMcpAuthType( } const probeFetch: FetchLike = resolvedIP - ? createPinnedFetch(resolvedIP) + ? createPinnedMcpFetch(resolvedIP) : createSsrfGuardedMcpFetch() const controller = new AbortController() diff --git a/apps/sim/lib/mcp/pinned-fetch.test.ts b/apps/sim/lib/mcp/pinned-fetch.test.ts index 64354ee708b..3dc7701d6a7 100644 --- a/apps/sim/lib/mcp/pinned-fetch.test.ts +++ b/apps/sim/lib/mcp/pinned-fetch.test.ts @@ -31,7 +31,7 @@ describe('createSsrfGuardedMcpFetch', () => { await fetchLike('https://attacker.example/revoke', { method: 'POST' }) expect(mockValidateMcpServerSsrf).toHaveBeenCalledWith('https://attacker.example/revoke') - expect(mockCreatePinnedFetch).toHaveBeenCalledWith('203.0.113.10') + expect(mockCreatePinnedFetch).toHaveBeenCalledWith('203.0.113.10', { allowH2: true }) expect(sentinelFetch).toHaveBeenCalledWith('https://attacker.example/revoke', { method: 'POST', }) @@ -54,7 +54,7 @@ describe('createSsrfGuardedMcpFetch', () => { await fetchLike(new URL('https://attacker.example/discover')) expect(mockValidateMcpServerSsrf).toHaveBeenCalledWith('https://attacker.example/discover') - expect(mockCreatePinnedFetch).toHaveBeenCalledWith('203.0.113.10') + expect(mockCreatePinnedFetch).toHaveBeenCalledWith('203.0.113.10', { allowH2: true }) }) it('falls back to global fetch when validation returns no IP', async () => { diff --git a/apps/sim/lib/mcp/pinned-fetch.ts b/apps/sim/lib/mcp/pinned-fetch.ts index 3184a0da7a9..4188d348cac 100644 --- a/apps/sim/lib/mcp/pinned-fetch.ts +++ b/apps/sim/lib/mcp/pinned-fetch.ts @@ -2,6 +2,18 @@ import type { FetchLike } from '@modelcontextprotocol/sdk/shared/transport.js' import { createPinnedFetch } from '@/lib/core/security/input-validation.server' import { validateMcpServerSsrf } from '@/lib/mcp/domain-check' +/** + * Pinned fetch for all MCP traffic. MCP servers are commonly deployed behind + * HTTP/2 fronts (CDNs, cloud LBs), and undici's Agent is h1.1-only unless opted + * into h2 via ALPN — so every MCP connection enables it. This is the single + * source of the "MCP implies h2" decision; the base `createPinnedFetch` keeps + * its h1.1 default for non-MCP consumers. Pinning is unaffected: the pinned + * lookup forces the socket to `resolvedIP` regardless of negotiated protocol. + */ +export function createPinnedMcpFetch(resolvedIP: string): typeof fetch { + return createPinnedFetch(resolvedIP, { allowH2: true }) +} + /** * Builds a `FetchLike` that validates every outbound request URL against the * MCP SSRF policy before issuing it, then pins the connection to the resolved @@ -25,7 +37,7 @@ export function createSsrfGuardedMcpFetch(): FetchLike { return (async (url, init) => { const target = typeof url === 'string' ? url : url.href const resolvedIP = await validateMcpServerSsrf(target) - const pinnedFetch: FetchLike = resolvedIP ? createPinnedFetch(resolvedIP) : globalThis.fetch + const pinnedFetch: FetchLike = resolvedIP ? createPinnedMcpFetch(resolvedIP) : globalThis.fetch return pinnedFetch(url, init) }) satisfies FetchLike } From 2d8118bf3b5b8b4d581cc547286698de68f43a08 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Sat, 18 Jul 2026 00:59:22 -0700 Subject: [PATCH 12/16] chore(mcp): remove unused cache CAS methods and simplify discovery helpers - Delete the unused setIfCurrentMutation/deleteIfCurrentMutation cache methods (interface + memory + redis adapters) and their two Redis Lua scripts; production only uses applyMutationIfCurrent. Tests rewritten onto applyMutationIfCurrent (ordering coverage preserved) or dropped where they only exercised the removed Lua. - Drop the vestigial fallback param from getDiscoveryFailureMessage (all callers coerced to 'Connection failed'). - Route isDynamicClientRegistrationUnsupported through getErrorMessage to match the module's error-inspection convention. --- apps/sim/app/api/mcp/oauth/start/route.ts | 9 ++- apps/sim/lib/mcp/service.test.ts | 18 ------ apps/sim/lib/mcp/service.ts | 14 ++-- apps/sim/lib/mcp/storage/adapter.ts | 8 --- apps/sim/lib/mcp/storage/memory-cache.test.ts | 21 +++--- apps/sim/lib/mcp/storage/memory-cache.ts | 22 ------- apps/sim/lib/mcp/storage/redis-cache.test.ts | 44 ------------- apps/sim/lib/mcp/storage/redis-cache.ts | 64 ------------------- 8 files changed, 21 insertions(+), 179 deletions(-) diff --git a/apps/sim/app/api/mcp/oauth/start/route.ts b/apps/sim/app/api/mcp/oauth/start/route.ts index d176fb6d673..4354257a1e0 100644 --- a/apps/sim/app/api/mcp/oauth/start/route.ts +++ b/apps/sim/app/api/mcp/oauth/start/route.ts @@ -2,7 +2,7 @@ import { OAuthError, ServerError } from '@modelcontextprotocol/sdk/server/auth/e import { db } from '@sim/db' import { mcpServers } from '@sim/db/schema' import { createLogger } from '@sim/logger' -import { toError } from '@sim/utils/errors' +import { getErrorMessage, toError } from '@sim/utils/errors' import { and, eq, isNull } from 'drizzle-orm' import type { NextRequest } from 'next/server' import { NextResponse } from 'next/server' @@ -29,10 +29,9 @@ const DCR_UNSUPPORTED_MESSAGE = "This server doesn't support OAuth client registration. Configure a token instead." function isDynamicClientRegistrationUnsupported(error: unknown): boolean { - return ( - error instanceof Error && - error.message.toLowerCase().includes('does not support dynamic client registration') - ) + return getErrorMessage(error, '') + .toLowerCase() + .includes('does not support dynamic client registration') } export function surfaceOauthError(error: unknown): string { diff --git a/apps/sim/lib/mcp/service.test.ts b/apps/sim/lib/mcp/service.test.ts index aa34b0e4180..a671fa918e4 100644 --- a/apps/sim/lib/mcp/service.test.ts +++ b/apps/sim/lib/mcp/service.test.ts @@ -53,24 +53,6 @@ const { cacheMutations.set(scopeKey, mutationId) return mutationId }), - setIfCurrentMutation: vi.fn( - async ( - scopeKey: string, - mutationId: number, - key: string, - tools: unknown[], - ttlMs: number - ) => { - if (cacheMutations.get(scopeKey) !== mutationId) return false - cacheStore.set(key, { tools, expiry: Date.now() + ttlMs }) - return true - } - ), - deleteIfCurrentMutation: vi.fn(async (scopeKey: string, mutationId: number, key: string) => { - if (cacheMutations.get(scopeKey) !== mutationId) return false - cacheStore.delete(key) - return true - }), applyMutationIfCurrent: vi.fn( async ( scopeKey: string, diff --git a/apps/sim/lib/mcp/service.ts b/apps/sim/lib/mcp/service.ts index 27675747e06..88de16fe1b0 100644 --- a/apps/sim/lib/mcp/service.ts +++ b/apps/sim/lib/mcp/service.ts @@ -169,11 +169,7 @@ function isOauthAuthorizationError(error: unknown, authType: McpServerConfig['au ) } -function getDiscoveryFailureMessage( - error: unknown, - authType: McpServerConfig['authType'], - fallback: string -): string { +function getDiscoveryFailureMessage(error: unknown, authType: McpServerConfig['authType']): string { if (authType !== 'oauth' && error instanceof UnauthorizedError) { return 'Authentication failed' } @@ -197,7 +193,7 @@ function getDiscoveryFailureMessage( ) { return 'Unable to reach the MCP server' } - return fallback === 'Unknown error' ? 'Connection failed' : fallback + return 'Connection failed' } function isTimeoutError(error: unknown): boolean { @@ -898,7 +894,7 @@ class McpService { } return { kind: 'error', - message: getDiscoveryFailureMessage(error, config.authType, 'Unknown error'), + message: getDiscoveryFailureMessage(error, config.authType), originalError: error, config, mutation, @@ -1137,7 +1133,7 @@ class McpService { config, mutation, error, - getDiscoveryFailureMessage(error, config.authType, 'Connection failed') + getDiscoveryFailureMessage(error, config.authType) ) } } @@ -1200,7 +1196,7 @@ class McpService { status: 'error', toolCount: 0, lastSeen: undefined, - error: getDiscoveryFailureMessage(error, config.authType, 'Connection failed'), + error: getDiscoveryFailureMessage(error, config.authType), }) } } diff --git a/apps/sim/lib/mcp/storage/adapter.ts b/apps/sim/lib/mcp/storage/adapter.ts index a05b0ae0c74..87b3c2fe644 100644 --- a/apps/sim/lib/mcp/storage/adapter.ts +++ b/apps/sim/lib/mcp/storage/adapter.ts @@ -22,14 +22,6 @@ export interface McpCacheStorageAdapter { * same value orders database publication for an end-to-end consistent state. */ beginMutation(scopeKey: string): Promise - setIfCurrentMutation( - scopeKey: string, - mutationId: number, - key: string, - tools: McpTool[], - ttlMs: number - ): Promise - deleteIfCurrentMutation(scopeKey: string, mutationId: number, key: string): Promise /** Atomically applies one server's complete cache state if this mutation still owns it. */ applyMutationIfCurrent( scopeKey: string, diff --git a/apps/sim/lib/mcp/storage/memory-cache.test.ts b/apps/sim/lib/mcp/storage/memory-cache.test.ts index 7f879cbb51a..94e5c80e28d 100644 --- a/apps/sim/lib/mcp/storage/memory-cache.test.ts +++ b/apps/sim/lib/mcp/storage/memory-cache.test.ts @@ -175,16 +175,20 @@ describe('MemoryMcpCache', () => { expect(newer).toBeGreaterThan(older) expect( - await cache.setIfCurrentMutation( + await cache.applyMutationIfCurrent( 'server-1', newer, - 'server-1:tools', - [createTool('new-tool')], - 60000 + { key: 'server-1:tools', tools: [createTool('new-tool')], ttlMs: 60000 }, + [] ) ).toBe(true) expect( - await cache.setIfCurrentMutation('server-1', older, 'server-1:failure', [], 60000) + await cache.applyMutationIfCurrent( + 'server-1', + older, + { key: 'server-1:failure', tools: [], ttlMs: 60000 }, + [] + ) ).toBe(false) expect((await cache.get('server-1:tools'))?.tools).toEqual([createTool('new-tool')]) @@ -218,12 +222,11 @@ describe('MemoryMcpCache', () => { await cache.clear() expect( - await cache.setIfCurrentMutation( + await cache.applyMutationIfCurrent( 'server-1', mutation, - 'server-1:tools', - [createTool('stale-tool')], - 60000 + { key: 'server-1:tools', tools: [createTool('stale-tool')], ttlMs: 60000 }, + [] ) ).toBe(false) expect(await cache.get('server-1:tools')).toBeNull() diff --git a/apps/sim/lib/mcp/storage/memory-cache.ts b/apps/sim/lib/mcp/storage/memory-cache.ts index f2a42a444a1..ac2c56176d9 100644 --- a/apps/sim/lib/mcp/storage/memory-cache.ts +++ b/apps/sim/lib/mcp/storage/memory-cache.ts @@ -97,28 +97,6 @@ export class MemoryMcpCache implements McpCacheStorageAdapter { return mutationId } - async setIfCurrentMutation( - scopeKey: string, - mutationId: number, - key: string, - tools: McpTool[], - ttlMs: number - ): Promise { - if (this.mutationVersions.get(scopeKey) !== mutationId) return false - await this.set(key, tools, ttlMs) - return true - } - - async deleteIfCurrentMutation( - scopeKey: string, - mutationId: number, - key: string - ): Promise { - if (this.mutationVersions.get(scopeKey) !== mutationId) return false - await this.delete(key) - return true - } - async applyMutationIfCurrent( scopeKey: string, mutationId: number, diff --git a/apps/sim/lib/mcp/storage/redis-cache.test.ts b/apps/sim/lib/mcp/storage/redis-cache.test.ts index 738ee60b35a..af158a3a43c 100644 --- a/apps/sim/lib/mcp/storage/redis-cache.test.ts +++ b/apps/sim/lib/mcp/storage/redis-cache.test.ts @@ -55,50 +55,6 @@ describe('RedisMcpCache ordered mutations', () => { ) }) - it('reports whether a tool cache write still owns the current mutation', async () => { - redis.eval.mockResolvedValueOnce(1).mockResolvedValueOnce(0) - - await expect( - cache.setIfCurrentMutation('workspace:w:server:s', 7, 'workspace:w:server:s', [tool], 60_000) - ).resolves.toBe(true) - await expect( - cache.setIfCurrentMutation( - 'workspace:w:server:s', - 6, - 'workspace:w:server:s:failure', - [], - 60_000 - ) - ).resolves.toBe(false) - - expect(redis.eval).toHaveBeenNthCalledWith( - 1, - expect.stringContaining("redis.call('SET'"), - 2, - 'mcp:tools-mutation:workspace:w:server:s', - 'mcp:tools:workspace:w:server:s', - '7', - expect.stringContaining('new-tool'), - '60000' - ) - }) - - it('guards cache deletion with the same mutation id', async () => { - redis.eval.mockResolvedValueOnce(0) - - await expect( - cache.deleteIfCurrentMutation('workspace:w:server:s', 6, 'workspace:w:server:s:failure') - ).resolves.toBe(false) - - expect(redis.eval).toHaveBeenCalledWith( - expect.stringContaining("redis.call('DEL'"), - 2, - 'mcp:tools-mutation:workspace:w:server:s', - 'mcp:tools:workspace:w:server:s:failure', - '6' - ) - }) - it('atomically replaces the complete cache state for the current mutation', async () => { redis.eval.mockResolvedValueOnce(1) diff --git a/apps/sim/lib/mcp/storage/redis-cache.ts b/apps/sim/lib/mcp/storage/redis-cache.ts index 4976dc7cfcf..04d5b95f493 100644 --- a/apps/sim/lib/mcp/storage/redis-cache.ts +++ b/apps/sim/lib/mcp/storage/redis-cache.ts @@ -18,22 +18,6 @@ redis.call('SET', KEYS[1], tostring(mutationId), 'PX', ARGV[1]) return mutationId ` -const SET_IF_CURRENT_MUTATION = ` -if redis.call('GET', KEYS[1]) ~= ARGV[1] then - return 0 -end -redis.call('SET', KEYS[2], ARGV[2], 'PX', ARGV[3]) -return 1 -` - -const DELETE_IF_CURRENT_MUTATION = ` -if redis.call('GET', KEYS[1]) ~= ARGV[1] then - return 0 -end -redis.call('DEL', KEYS[2]) -return 1 -` - const APPLY_MUTATION_IF_CURRENT = ` if redis.call('GET', KEYS[1]) ~= ARGV[1] then return 0 @@ -124,54 +108,6 @@ export class RedisMcpCache implements McpCacheStorageAdapter { } } - async setIfCurrentMutation( - scopeKey: string, - mutationId: number, - key: string, - tools: McpTool[], - ttlMs: number - ): Promise { - try { - const entry: McpCacheEntry = { - tools, - expiry: Date.now() + ttlMs, - } - const result = await this.redis.eval( - SET_IF_CURRENT_MUTATION, - 2, - this.getMutationKey(scopeKey), - this.getKey(key), - String(mutationId), - JSON.stringify(entry), - String(ttlMs) - ) - return result === 1 - } catch (error) { - logger.error('Redis conditional cache set error:', error) - throw error - } - } - - async deleteIfCurrentMutation( - scopeKey: string, - mutationId: number, - key: string - ): Promise { - try { - const result = await this.redis.eval( - DELETE_IF_CURRENT_MUTATION, - 2, - this.getMutationKey(scopeKey), - this.getKey(key), - String(mutationId) - ) - return result === 1 - } catch (error) { - logger.error('Redis conditional cache delete error:', error) - throw error - } - } - async applyMutationIfCurrent( scopeKey: string, mutationId: number, From 7909bb0f60c65cef6a4c8fadf18cbcb8715b7a6d Mon Sep 17 00:00:00 2001 From: Justin Blumencranz <96924014+j15z@users.noreply.github.com> Date: Sat, 18 Jul 2026 01:17:37 -0700 Subject: [PATCH 13/16] Fix MCP retry publication ordering (#5754) Reacquire cache mutation ownership for each discovery retry and cover cache invalidation during retry backoff. Note: the full app suite has pre-existing failures in unrelated workflow, webhook, chat, and UI tests; the focused MCP suite passes 38/38. --- apps/sim/lib/mcp/service.test.ts | 52 ++++++++++++++++++++++++++++++++ apps/sim/lib/mcp/service.ts | 3 +- 2 files changed, 53 insertions(+), 2 deletions(-) diff --git a/apps/sim/lib/mcp/service.test.ts b/apps/sim/lib/mcp/service.test.ts index a671fa918e4..7f6eeffe0b5 100644 --- a/apps/sim/lib/mcp/service.test.ts +++ b/apps/sim/lib/mcp/service.test.ts @@ -1004,6 +1004,58 @@ describe('McpService.discoverTools per-server caching', () => { expect(mockListTools).toHaveBeenCalledTimes(2) }) + it('reacquires mutation ownership after cache invalidation during retry backoff', async () => { + vi.useFakeTimers({ toFake: ['setTimeout', 'clearTimeout'] }) + try { + const serverKey = `workspace:${WORKSPACE_ID}:server:mcp-a` + mockGetWorkspaceServersRows.mockResolvedValue([dbRow('mcp-a', 'A')]) + + let rejectFirstAttempt: ((error: Error) => void) | undefined + let markFirstAttemptStarted: (() => void) | undefined + const firstAttemptStarted = new Promise((resolve) => { + markFirstAttemptStarted = resolve + }) + mockListTools + .mockImplementationOnce( + () => + new Promise((_resolve, reject) => { + rejectFirstAttempt = reject + markFirstAttemptStarted?.() + }) + ) + .mockResolvedValueOnce([tool('retry-winner', 'mcp-a')]) + + const discovery = mcpService.discoverServerToolsWithMetadata( + USER_ID, + 'mcp-a', + WORKSPACE_ID, + true + ) + await firstAttemptStarted + + rejectFirstAttempt?.(new Error('Request timed out')) + await vi.advanceTimersByTimeAsync(0) + expect(mockListTools).toHaveBeenCalledTimes(1) + + await mcpService.clearCache(WORKSPACE_ID) + expect(mockCacheAdapter.beginMutation).toHaveBeenCalledTimes(2) + + await vi.runAllTimersAsync() + await expect(discovery).resolves.toEqual({ + tools: [tool('retry-winner', 'mcp-a')], + state: 'published', + }) + + expect(mockCacheAdapter.beginMutation).toHaveBeenCalledTimes(3) + expect(cacheStore.get(serverKey)?.tools).toEqual([tool('retry-winner', 'mcp-a')]) + expect(mockUpdateSet).toHaveBeenCalledWith( + expect.objectContaining({ connectionStatus: 'connected', toolCount: 1 }) + ) + } finally { + vi.useRealTimers() + } + }) + it('persists and negative-caches per-server UnauthorizedError for headers auth', async () => { const reflectedCredential = 'Bearer static-secret-for-server-discovery' mockGetWorkspaceServersRows.mockResolvedValue([ diff --git a/apps/sim/lib/mcp/service.ts b/apps/sim/lib/mcp/service.ts index 88de16fe1b0..04cdde28a14 100644 --- a/apps/sim/lib/mcp/service.ts +++ b/apps/sim/lib/mcp/service.ts @@ -1069,9 +1069,8 @@ class McpService { } } - const mutation = await this.beginServerCacheMutation(workspaceId, serverId) - for (let attempt = 0; attempt < maxRetries; attempt++) { + const mutation = await this.beginServerCacheMutation(workspaceId, serverId) let config: McpServerConfig | null = null try { logger.info( From 9743f93aca85c1927b741c12632262e17699fff3 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Sat, 18 Jul 2026 01:12:59 -0700 Subject: [PATCH 14/16] 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. --- apps/sim/lib/mcp/service.test.ts | 17 +++++++++++++++++ apps/sim/lib/mcp/service.ts | 4 ++++ 2 files changed, 21 insertions(+) diff --git a/apps/sim/lib/mcp/service.test.ts b/apps/sim/lib/mcp/service.test.ts index 7f6eeffe0b5..369ff69fe26 100644 --- a/apps/sim/lib/mcp/service.test.ts +++ b/apps/sim/lib/mcp/service.test.ts @@ -517,6 +517,23 @@ describe('McpService.discoverTools per-server caching', () => { ) }) + it('acquires a fresh mutation on each discovery retry so a retried result still publishes', async () => { + const serverKey = `workspace:${WORKSPACE_ID}:server:mcp-a` + mockGetWorkspaceServersRows.mockResolvedValue([dbRow('mcp-a', 'A')]) + mockCacheAdapter.beginMutation.mockClear() + mockListTools + .mockRejectedValueOnce(new Error('Request timed out')) + .mockResolvedValueOnce([tool('a1', 'mcp-a')]) + + const tools = await mcpService.discoverServerTools(USER_ID, 'mcp-a', WORKSPACE_ID, true) + + expect(tools).toEqual([tool('a1', 'mcp-a')]) + expect(cacheStore.get(serverKey)?.tools).toEqual([tool('a1', 'mcp-a')]) + // One begin per attempt: the retry publishes under a current ownership id + // instead of a stale pre-loop id that a concurrent clearCache could supersede. + expect(mockCacheAdapter.beginMutation).toHaveBeenCalledTimes(2) + }) + it('keeps an older ordered publisher from superseding a retry-acquired mutation', async () => { const serverKey = `workspace:${WORKSPACE_ID}:server:mcp-a` mockGetWorkspaceServersRows.mockResolvedValue([dbRow('mcp-a', 'A')]) diff --git a/apps/sim/lib/mcp/service.ts b/apps/sim/lib/mcp/service.ts index 04cdde28a14..f7cc3fe6dc8 100644 --- a/apps/sim/lib/mcp/service.ts +++ b/apps/sim/lib/mcp/service.ts @@ -1072,6 +1072,10 @@ class McpService { for (let attempt = 0; attempt < maxRetries; attempt++) { const mutation = await this.beginServerCacheMutation(workspaceId, serverId) let config: McpServerConfig | null = null + // Begin a fresh mutation per attempt. A retry that succeeds after a + // concurrent clearCache must publish under a current ownership id — a + // stale pre-loop id would lose the CAS and drop otherwise-valid tools. + const mutation = await this.beginServerCacheMutation(workspaceId, serverId) try { logger.info( `[${requestId}] Discovering tools from server ${serverId} for user ${userId}${attempt > 0 ? ` (attempt ${attempt + 1})` : ''}` From 253c47190424cd425728374fcecdd6b1bb7cb8b6 Mon Sep 17 00:00:00 2001 From: Justin Blumencranz <96924014+j15z@users.noreply.github.com> Date: Sat, 18 Jul 2026 01:34:17 -0700 Subject: [PATCH 15/16] Close MCP invalidation publication races (#5754) Publish cache invalidation tokens into database ordering and fail refresh closed when discovery is superseded. Focused MCP suites pass 48/48 and API validation passes. Note: the full app suite retains the same 41 pre-existing failures in unrelated workflow, webhook, chat, and UI tests. --- .../mcp/servers/[id]/refresh/route.test.ts | 42 ++++- .../app/api/mcp/servers/[id]/refresh/route.ts | 8 +- apps/sim/lib/mcp/service.test.ts | 177 ++++++++++++++++++ apps/sim/lib/mcp/service.ts | 54 +++++- 4 files changed, 267 insertions(+), 14 deletions(-) diff --git a/apps/sim/app/api/mcp/servers/[id]/refresh/route.test.ts b/apps/sim/app/api/mcp/servers/[id]/refresh/route.test.ts index f8b88850680..88d3fc1301f 100644 --- a/apps/sim/app/api/mcp/servers/[id]/refresh/route.test.ts +++ b/apps/sim/app/api/mcp/servers/[id]/refresh/route.test.ts @@ -4,17 +4,19 @@ import type { NextRequest } from 'next/server' import { beforeEach, describe, expect, it, vi } from 'vitest' -const { mockClearCache, mockDiscoverServerTools, mockSelect, mockUpdateSet } = vi.hoisted(() => ({ - mockClearCache: vi.fn(), - mockDiscoverServerTools: vi.fn(), - mockSelect: vi.fn(), - mockUpdateSet: vi.fn(), -})) +const { mockClearCache, mockDiscoverServerTools, mockSelect, mockUpdate, mockUpdateSet } = + vi.hoisted(() => ({ + mockClearCache: vi.fn(), + mockDiscoverServerTools: vi.fn(), + mockSelect: vi.fn(), + mockUpdate: vi.fn(), + mockUpdateSet: vi.fn(), + })) vi.mock('@sim/db', () => ({ db: { select: mockSelect, - update: vi.fn().mockReturnValue({ set: mockUpdateSet }), + update: mockUpdate.mockReturnValue({ set: mockUpdateSet }), }, })) @@ -225,4 +227,30 @@ describe('MCP server refresh route', () => { }) ) }) + + it('fails closed without syncing workflows when discovery is superseded', async () => { + mockDiscoverServerTools.mockResolvedValueOnce({ + tools: [], + state: 'superseded', + }) + mockSelect.mockReturnValueOnce(selectRows([initialServer])) + + const request = new Request('http://localhost/api/mcp/servers/server-1/refresh', { + method: 'POST', + }) as NextRequest + const response = await POST(request, { params: Promise.resolve({ id: 'server-1' }) }) + const body = await response.json() + + expect(body.data).toEqual( + expect.objectContaining({ + status: 'disconnected', + error: 'Tool discovery was superseded by a newer refresh. Please retry.', + toolCount: 0, + workflowsUpdated: 0, + updatedWorkflowIds: [], + }) + ) + expect(mockSelect).toHaveBeenCalledTimes(2) + expect(mockUpdate).not.toHaveBeenCalled() + }) }) diff --git a/apps/sim/app/api/mcp/servers/[id]/refresh/route.ts b/apps/sim/app/api/mcp/servers/[id]/refresh/route.ts index d7024bbe1c3..95d5c1fe02d 100644 --- a/apps/sim/app/api/mcp/servers/[id]/refresh/route.ts +++ b/apps/sim/app/api/mcp/servers/[id]/refresh/route.ts @@ -230,7 +230,7 @@ export const POST = withRouteHandler( ) .limit(1) - if (discoveryError === null) { + if (discoveryError === null && discoveryState !== 'superseded') { try { syncResult = await syncToolSchemasToWorkflows( workspaceId, @@ -256,7 +256,11 @@ export const POST = withRouteHandler( let lastError = refreshedServer ? refreshedServer.lastError : discoveryError let toolCount = refreshedServer?.toolCount ?? discoveredTools.length - if ( + if (discoveryState === 'superseded') { + connectionStatus = 'disconnected' + lastError = 'Tool discovery was superseded by a newer refresh. Please retry.' + toolCount = 0 + } else if ( discoveryError === null && (discoveryState === 'unavailable' || discoveryState === 'winner-cache') ) { diff --git a/apps/sim/lib/mcp/service.test.ts b/apps/sim/lib/mcp/service.test.ts index 369ff69fe26..1359fffd569 100644 --- a/apps/sim/lib/mcp/service.test.ts +++ b/apps/sim/lib/mcp/service.test.ts @@ -329,6 +329,15 @@ describe('McpService.discoverTools per-server caching', () => { await mcpService.clearCache(WORKSPACE_ID) + const [discoveryUpdate, invalidationUpdate] = mockUpdateSet.mock.calls.map(([update]) => update) + expect(invalidationUpdate).toEqual({ + toolCount: 0, + lastToolsRefresh: expect.any(Date), + }) + expect(invalidationUpdate.lastToolsRefresh.getTime()).toBeGreaterThan( + discoveryUpdate.lastToolsRefresh.getTime() + ) + mockListTools.mockClear() mockListTools.mockResolvedValueOnce([tool('a1', 'mcp-a')]) await mcpService.discoverTools(USER_ID, WORKSPACE_ID) @@ -967,6 +976,174 @@ describe('McpService.discoverTools per-server caching', () => { await expect(discovery).resolves.toEqual([tool('a1', 'mcp-a')]) }) + it('publishes a newer invalidation barrier while successful status publication is pending', async () => { + vi.useFakeTimers({ toFake: ['Date'] }) + try { + const serverKey = `workspace:${WORKSPACE_ID}:server:mcp-a` + const discoveryStartedAt = new Date('2100-02-01T00:00:00.000Z') + const invalidatedAt = new Date('2100-02-01T00:00:01.000Z') + mockGetWorkspaceServersRows.mockResolvedValue([dbRow('mcp-a', 'A')]) + mockListTools.mockResolvedValueOnce([tool('stale-tool', 'mcp-a')]) + + let releaseDiscoveryStatus: ((rows: { id: string }[]) => void) | undefined + mockUpdateReturning.mockReturnValueOnce( + new Promise((resolve) => { + releaseDiscoveryStatus = resolve + }) + ) + + vi.setSystemTime(discoveryStartedAt) + const discovery = mcpService.discoverServerToolsWithMetadata( + USER_ID, + 'mcp-a', + WORKSPACE_ID, + true + ) + await vi.waitFor(() => + expect(mockUpdateSet).toHaveBeenCalledWith( + expect.objectContaining({ + connectionStatus: 'connected', + lastToolsRefresh: discoveryStartedAt, + }) + ) + ) + expect(cacheStore.get(serverKey)?.tools).toEqual([tool('stale-tool', 'mcp-a')]) + + vi.setSystemTime(invalidatedAt) + await mcpService.clearCache(WORKSPACE_ID) + + const invalidationUpdate = mockUpdateSet.mock.calls + .map(([update]) => update) + .find( + (update) => + update.toolCount === 0 && update.lastToolsRefresh?.getTime() === invalidatedAt.getTime() + ) + expect(invalidationUpdate).toEqual({ + toolCount: 0, + lastToolsRefresh: invalidatedAt, + }) + expect(cacheStore.has(serverKey)).toBe(false) + + // The real lastToolsRefresh predicate rejects this older publication + // after the invalidation barrier wins the database race. + releaseDiscoveryStatus?.([]) + await expect(discovery).resolves.toEqual({ tools: [], state: 'superseded' }) + } finally { + vi.useRealTimers() + } + }) + + it('publishes a newer invalidation barrier while failed status publication is pending', async () => { + vi.useFakeTimers({ toFake: ['Date'] }) + try { + const serverKey = `workspace:${WORKSPACE_ID}:server:mcp-a` + const failureKey = `${serverKey}:failure` + const discoveryStartedAt = new Date('2100-03-01T00:00:00.000Z') + const invalidatedAt = new Date('2100-03-01T00:00:01.000Z') + mockGetWorkspaceServersRows.mockResolvedValue([ + dbRow('mcp-a', 'A', { + statusConfig: { consecutiveFailures: 0, lastSuccessfulDiscovery: null }, + }), + ]) + mockListTools.mockRejectedValueOnce(new Error('Permanent discovery failure')) + + let releaseDiscoveryStatus: ((rows: { id: string }[]) => void) | undefined + mockUpdateReturning + .mockReturnValueOnce( + new Promise((resolve) => { + releaseDiscoveryStatus = resolve + }) + ) + .mockResolvedValueOnce([{ id: 'mcp-a' }]) + .mockResolvedValueOnce([]) + .mockResolvedValueOnce([]) + + vi.setSystemTime(discoveryStartedAt) + const discovery = mcpService.discoverServerTools(USER_ID, 'mcp-a', WORKSPACE_ID, true) + await vi.waitFor(() => + expect(mockUpdateSet).toHaveBeenCalledWith( + expect.objectContaining({ + connectionStatus: 'disconnected', + lastError: 'Connection failed', + lastToolsRefresh: discoveryStartedAt, + }) + ) + ) + expect(cacheStore.has(failureKey)).toBe(true) + + vi.setSystemTime(invalidatedAt) + await mcpService.clearCache(WORKSPACE_ID) + + expect(mockUpdateSet).toHaveBeenCalledWith({ + toolCount: 0, + lastToolsRefresh: invalidatedAt, + }) + expect(cacheStore.has(serverKey)).toBe(false) + expect(cacheStore.has(failureKey)).toBe(false) + + releaseDiscoveryStatus?.([]) + await expect(discovery).rejects.toThrow('Permanent discovery failure') + expect( + mockUpdateSet.mock.calls + .map(([update]) => update) + .filter((update) => update.lastError === 'Connection failed') + .every((update) => update.lastToolsRefresh?.getTime() === discoveryStartedAt.getTime()) + ).toBe(true) + } finally { + vi.useRealTimers() + } + }) + + it('publishes a newer invalidation barrier while OAuth status publication is pending', async () => { + vi.useFakeTimers({ toFake: ['Date'] }) + try { + const serverKey = `workspace:${WORKSPACE_ID}:server:mcp-a` + const discoveryStartedAt = new Date('2100-04-01T00:00:00.000Z') + const invalidatedAt = new Date('2100-04-01T00:00:01.000Z') + mockGetWorkspaceServersRows.mockResolvedValue([dbRow('mcp-a', 'A')]) + mockListTools.mockRejectedValueOnce(new McpOauthAuthorizationRequiredError('mcp-a', 'A')) + + let releaseDiscoveryStatus: ((rows: { id: string }[]) => void) | undefined + mockUpdateReturning.mockReturnValueOnce( + new Promise((resolve) => { + releaseDiscoveryStatus = resolve + }) + ) + + vi.setSystemTime(discoveryStartedAt) + const discovery = mcpService.discoverServerTools(USER_ID, 'mcp-a', WORKSPACE_ID, true) + await vi.waitFor(() => + expect(mockUpdateSet).toHaveBeenCalledWith( + expect.objectContaining({ + connectionStatus: 'disconnected', + lastError: null, + lastToolsRefresh: discoveryStartedAt, + }) + ) + ) + + vi.setSystemTime(invalidatedAt) + await mcpService.clearCache(WORKSPACE_ID) + + const invalidationUpdate = mockUpdateSet.mock.calls + .map(([update]) => update) + .find( + (update) => + update.toolCount === 0 && update.lastToolsRefresh?.getTime() === invalidatedAt.getTime() + ) + expect(invalidationUpdate).toEqual({ + toolCount: 0, + lastToolsRefresh: invalidatedAt, + }) + expect(cacheStore.has(serverKey)).toBe(false) + + releaseDiscoveryStatus?.([]) + await expect(discovery).rejects.toThrow('OAuth authorization required') + } finally { + vi.useRealTimers() + } + }) + it('keeps a newer successful cache entry when an older failure finishes later', async () => { mockGetWorkspaceServersRows.mockResolvedValue([dbRow('mcp-a', 'A')]) diff --git a/apps/sim/lib/mcp/service.ts b/apps/sim/lib/mcp/service.ts index f7cc3fe6dc8..0e2cb2b28d4 100644 --- a/apps/sim/lib/mcp/service.ts +++ b/apps/sim/lib/mcp/service.ts @@ -633,6 +633,45 @@ class McpService { } } + /** + * Publish an invalidation's cache-order token without changing connection + * health. A list_changed notification comes from a live connection, while + * configuration lifecycle code persists its own intended connection state. + */ + private async markServerCacheInvalidated( + serverId: string, + workspaceId: string, + publicationOrder: Date + ): Promise { + try { + const updatedServers = await db + .update(mcpServers) + .set({ + toolCount: 0, + lastToolsRefresh: publicationOrder, + }) + .where( + and( + eq(mcpServers.id, serverId), + eq(mcpServers.workspaceId, workspaceId), + isNull(mcpServers.deletedAt), + or( + isNull(mcpServers.lastToolsRefresh), + lte(mcpServers.lastToolsRefresh, publicationOrder) + ) + ) + ) + .returning({ id: mcpServers.id }) + return updatedServers.length > 0 + } catch (error) { + logger.warn(`Failed to publish cache invalidation for server ${serverId}`, { + workspaceId, + error: getMcpSafeErrorDiagnostics(error), + }) + return false + } + } + private async isServerUnhealthy(workspaceId: string, serverId: string): Promise { try { const entry = await this.cacheAdapter.get(failureCacheKey(workspaceId, serverId)) @@ -688,10 +727,16 @@ class McpService { await this.bestEffortInvalidateServerCache(workspaceId, serverId) return } - await this.applyServerCacheMutation(workspaceId, serverId, mutation, null, [ - serverCacheKey(workspaceId, serverId), - failureCacheKey(workspaceId, serverId), - ]) + const cacheApplied = await this.applyServerCacheMutation( + workspaceId, + serverId, + mutation, + null, + [serverCacheKey(workspaceId, serverId), failureCacheKey(workspaceId, serverId)] + ) + if (cacheApplied !== 'applied') return + + await this.markServerCacheInvalidated(serverId, workspaceId, new Date(mutation.id)) } private async getCurrentCachedTools( @@ -1070,7 +1115,6 @@ class McpService { } for (let attempt = 0; attempt < maxRetries; attempt++) { - const mutation = await this.beginServerCacheMutation(workspaceId, serverId) let config: McpServerConfig | null = null // Begin a fresh mutation per attempt. A retry that succeeds after a // concurrent clearCache must publish under a current ownership id — a From 9043b7ee911bcfaf0a37e2c94bf714559b8faefe Mon Sep 17 00:00:00 2001 From: Justin Blumencranz <96924014+j15z@users.noreply.github.com> Date: Sat, 18 Jul 2026 02:34:36 -0700 Subject: [PATCH 16/16] fix(mcp): verify racing refresh success --- .../mcp/servers/[id]/refresh/route.test.ts | 26 +++++++++++++++++++ .../app/api/mcp/servers/[id]/refresh/route.ts | 4 ++- 2 files changed, 29 insertions(+), 1 deletion(-) diff --git a/apps/sim/app/api/mcp/servers/[id]/refresh/route.test.ts b/apps/sim/app/api/mcp/servers/[id]/refresh/route.test.ts index 88d3fc1301f..95d7cb0c7ec 100644 --- a/apps/sim/app/api/mcp/servers/[id]/refresh/route.test.ts +++ b/apps/sim/app/api/mcp/servers/[id]/refresh/route.test.ts @@ -164,6 +164,32 @@ describe('MCP server refresh route', () => { expect(mockClearCache).not.toHaveBeenCalled() }) + it('reports the discovery failure when only cache invalidation advanced', async () => { + mockDiscoverServerTools.mockRejectedValueOnce(new Error('Connection failed')) + const cacheInvalidatedServer = { + ...initialServer, + lastToolsRefresh: new Date(initialServer.lastToolsRefresh.getTime() + 60_000), + toolCount: 0, + } + mockSelect.mockReturnValueOnce(selectRows([cacheInvalidatedServer])) + + const request = new Request('http://localhost/api/mcp/servers/server-1/refresh', { + method: 'POST', + }) as NextRequest + const response = await POST(request, { params: Promise.resolve({ id: 'server-1' }) }) + const body = await response.json() + + expect(body.data).toEqual( + expect.objectContaining({ + status: 'disconnected', + error: 'Internal server error', + toolCount: 0, + workflowsUpdated: 0, + }) + ) + expect(mockClearCache).not.toHaveBeenCalled() + }) + it('does not 500 when workflow sync fails after a successful discovery', async () => { mockDiscoverServerTools.mockResolvedValueOnce({ tools: [ diff --git a/apps/sim/app/api/mcp/servers/[id]/refresh/route.ts b/apps/sim/app/api/mcp/servers/[id]/refresh/route.ts index 95d5c1fe02d..9ab15c8f149 100644 --- a/apps/sim/app/api/mcp/servers/[id]/refresh/route.ts +++ b/apps/sim/app/api/mcp/servers/[id]/refresh/route.ts @@ -273,7 +273,9 @@ export const POST = withRouteHandler( const newerSuccessWonRace = refreshedServer?.lastToolsRefresh != null && (server.lastToolsRefresh == null || - refreshedServer.lastToolsRefresh > server.lastToolsRefresh) + refreshedServer.lastToolsRefresh > server.lastToolsRefresh) && + refreshedServer.lastConnected != null && + (server.lastConnected == null || refreshedServer.lastConnected > server.lastConnected) if (!newerSuccessWonRace) { connectionStatus = 'disconnected'