fix(mcp): retire pooled connections on auth failure and server delete

Author: waleedlatif1

apps/sim/lib/mcp/orchestration/server-lifecycle.test.ts

@@ -14,11 +14,13 @@ import {
 } from '@sim/testing'
 import { beforeEach, describe, expect, it, vi } from 'vitest'
 
-const { mockClearCache, mockOauthCredsChanged, mockRevokeOauthTokens } = vi.hoisted(() => ({
-  mockClearCache: vi.fn(),
-  mockOauthCredsChanged: vi.fn(),
-  mockRevokeOauthTokens: vi.fn(),
-}))
+const { mockClearCache, mockOauthCredsChanged, mockRevokeOauthTokens, mockEvictServer } =
+  vi.hoisted(() => ({
+    mockClearCache: vi.fn(),
+    mockOauthCredsChanged: vi.fn(),
+    mockRevokeOauthTokens: vi.fn(),
+    mockEvictServer: vi.fn(),
+  }))
 
 vi.mock('@sim/audit', () => auditMock)
 vi.mock('@sim/db', () => ({
@@ -47,10 +49,16 @@ vi.mock('@/lib/mcp/oauth', () => ({
 vi.mock('@/lib/mcp/service', () => ({
   mcpService: { clearCache: mockClearCache },
 }))
+vi.mock('@/lib/mcp/connection-pool', () => ({
+  mcpConnectionPool: { evictServer: mockEvictServer },
+}))
 vi.mock('@/lib/mcp/utils', () => ({ generateMcpServerId: vi.fn() }))
 vi.mock('@/lib/posthog/server', () => posthogServerMock)
 
-import { performUpdateMcpServer } from '@/lib/mcp/orchestration/server-lifecycle'
+import {
+  performDeleteMcpServer,
+  performUpdateMcpServer,
+} from '@/lib/mcp/orchestration/server-lifecycle'
 
 describe('MCP server lifecycle orchestration', () => {
   beforeEach(() => {
@@ -140,4 +148,19 @@ describe('MCP server lifecycle orchestration', () => {
     // ...and revoke the now-orphaned OAuth tokens rather than leaving them stored and valid.
     expect(mockRevokeOauthTokens).toHaveBeenCalledWith('server-1')
   })
+
+  it('evicts the deleted server from the connection pool (row is already gone from clearCache)', async () => {
+    dbChainMockFns.returning.mockResolvedValueOnce([
+      { id: 'server-1', workspaceId: 'workspace-1', name: 'Example', transport: 'streamable-http' },
+    ])
+
+    const result = await performDeleteMcpServer({
+      workspaceId: 'workspace-1',
+      userId: 'user-1',
+      serverId: 'server-1',
+    })
+
+    expect(result.success).toBe(true)
+    expect(mockEvictServer).toHaveBeenCalledWith('server-1', expect.any(String))
+  })
 })

apps/sim/lib/mcp/orchestration/server-lifecycle.ts

@@ -6,6 +6,7 @@ import { generateId } from '@sim/utils/id'
 import { and, eq, isNull } from 'drizzle-orm'
 import type { NextRequest } from 'next/server'
 import { encryptSecret } from '@/lib/core/security/encryption'
+import { mcpConnectionPool } from '@/lib/mcp/connection-pool'
 import {
   McpDnsResolutionError,
   McpDomainNotAllowedError,
@@ -438,6 +439,9 @@ export async function performDeleteMcpServer(
 
     if (!server) return { success: false, error: 'Server not found', errorCode: 'not_found' }
 
+    // The row is already gone, so clearCache's row-enumeration can't reach it —
+    // evict the deleted server's pooled connections explicitly.
+    await mcpConnectionPool?.evictServer(params.serverId, 'server deleted')
     await mcpService.clearCache(params.workspaceId)
     const source =
       params.source === 'settings' || params.source === 'tool_input' ? params.source : undefined

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

@@ -7,6 +7,7 @@
  * dead-connection error poisons the lease. Pool internals are unit-tested in
  * `connection-pool.test.ts`.
  */
+import { UnauthorizedError } from '@modelcontextprotocol/sdk/client/auth.js'
 import { StreamableHTTPError } from '@modelcontextprotocol/sdk/client/streamableHttp.js'
 import { loggerMock } from '@sim/testing'
 import { beforeEach, describe, expect, it, vi } from 'vitest'
@@ -169,4 +170,14 @@ describe('McpService connection reuse wiring', () => {
 
     expect(mockRelease).toHaveBeenCalledWith(false)
   })
+
+  it('poisons the lease on an auth failure so a rotated credential is re-resolved', async () => {
+    mockCallTool.mockRejectedValue(new UnauthorizedError('token rejected'))
+
+    await expect(
+      mcpService.executeTool(USER_ID, 'server-1', { name: 'do', arguments: {} }, WORKSPACE_ID)
+    ).rejects.toThrow()
+
+    expect(mockRelease).toHaveBeenCalledWith(true)
+  })
 })

apps/sim/lib/mcp/service.ts

@@ -103,13 +103,17 @@ function isTimeoutError(error: unknown): boolean {
 
 /**
  * A pooled connection is dead and must be retired so the caller's retry rebuilds
- * fresh: a stale session (400/404), a closed transport, a timeout (no response —
- * possibly wedged), or a reset socket. Benign tool/consent errors and healthy
- * upstream responses (429/5xx) keep the connection warm.
+ * fresh: a stale session (400/404), an auth failure (401 — a rotated/revoked
+ * credential; the rebuild re-resolves it), a closed transport, a timeout (no
+ * response — possibly wedged), or a reset socket. Benign tool/consent errors and
+ * healthy upstream responses (429/5xx) keep the connection warm.
  */
 function isDeadConnectionError(error: unknown): boolean {
+  if (error instanceof UnauthorizedError) {
+    return true
+  }
   if (error instanceof StreamableHTTPError) {
-    return error.code === 404 || error.code === 400
+    return error.code === 404 || error.code === 400 || error.code === 401
   }
   if (error instanceof McpError && error.code === ErrorCode.ConnectionClosed) {
     return true