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

Author: j15z

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
     }

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)
+  })
+})

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),
+  }
+}

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'))

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<boolean> {
-    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,15 +618,14 @@ 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
     }
   }
 
   private async invalidateServerCache(workspaceId: string, serverId: string): Promise<void> {
     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