fix(mcp): use evicted-mid-create connections one-shot and decouple pool eviction from cache clear

Author: waleedlatif1

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

@@ -252,7 +252,7 @@ describe('McpConnectionPool', () => {
     expect(create).toHaveBeenCalledTimes(2)
   })
 
-  it('does not pool a connection whose create finished after the server was evicted', async () => {
+  it('uses an evicted-mid-create connection one-shot and does not pool it', async () => {
     let resolveCreate: ((client: McpClient) => void) | undefined
     const created = new Promise<McpClient>((resolve) => {
       resolveCreate = resolve
@@ -270,11 +270,18 @@ describe('McpConnectionPool', () => {
     resolveCreate?.(staleClient)
     const lease = await acquireP
 
-    // The stale-config connection is disconnected, not pooled; a fresh one is built.
+    // The in-flight request still uses the connection it built (as connect-per-op
+    // would), but it isn't pooled and is disconnected on release.
+    expect(lease.client).toBe(staleClient)
+    expect(staleClient.disconnect).not.toHaveBeenCalled()
+    await lease.release()
     expect(staleClient.disconnect).toHaveBeenCalledTimes(1)
-    expect(lease.client).toBe(freshClient)
+
+    // The next acquire builds fresh — the stale connection was never pooled.
+    const next = await pool.acquire(params('s1:w1:u1', create))
+    expect(next.client).toBe(freshClient)
     expect(create).toHaveBeenCalledTimes(2)
-    await lease.release()
+    await next.release()
   })
 
   it('bypasses the pool once disposed (connects without caching)', async () => {

apps/sim/lib/mcp/connection-pool.ts

@@ -85,21 +85,14 @@ export class McpConnectionPool {
 
   private async resolveEntry(params: AcquireParams): Promise<PoolEntry> {
     const pending = this.pending.get(params.key)
-    if (pending) {
-      const entry = await pending
-      if (!entry.retired) return entry
-      return this.resolveEntry(params)
-    }
+    if (pending) return pending
 
     const current = this.entries.get(params.key)
     if (current) {
       const reusable = await this.tryReuse(current)
       if (reusable) return reusable
     }
-    const created = await this.createEntry(params)
-    // Evicted (config edit/delete) mid-create → don't borrow the stale connection.
-    if (created.retired && !this.disposed) return this.resolveEntry(params)
-    return created
+    return this.createEntry(params)
   }
 
   /** Return `entry` if in-age, connected, and live; else retire it and return null. */
@@ -152,10 +145,11 @@ export class McpConnectionPool {
         closing: false,
       }
       // Disposed, or the server was evicted (config edit/delete) while connecting:
-      // don't pool a connection built against the old config.
+      // don't pool a connection built against the now-stale config. The current
+      // borrower still uses it once (as connect-per-op would for an in-flight
+      // request); it's disconnected on release, and the next acquire builds fresh.
       if (this.disposed || (this.serverGenerations.get(params.serverId) ?? 0) !== generation) {
         entry.retired = true
-        void client.disconnect().catch(() => {})
         return entry
       }
       this.evictLruIfFull()

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

@@ -14,12 +14,12 @@ import {
 } from '@sim/testing'
 import { beforeEach, describe, expect, it, vi } from 'vitest'
 
-const { mockClearCache, mockOauthCredsChanged, mockRevokeOauthTokens, mockEvictServer } =
+const { mockClearCache, mockOauthCredsChanged, mockRevokeOauthTokens, mockEvictServerConnections } =
   vi.hoisted(() => ({
     mockClearCache: vi.fn(),
     mockOauthCredsChanged: vi.fn(),
     mockRevokeOauthTokens: vi.fn(),
-    mockEvictServer: vi.fn(),
+    mockEvictServerConnections: vi.fn(),
   }))
 
 vi.mock('@sim/audit', () => auditMock)
@@ -47,10 +47,10 @@ vi.mock('@/lib/mcp/oauth', () => ({
   revokeMcpOauthTokens: mockRevokeOauthTokens,
 }))
 vi.mock('@/lib/mcp/service', () => ({
-  mcpService: { clearCache: mockClearCache },
-}))
-vi.mock('@/lib/mcp/connection-pool', () => ({
-  mcpConnectionPool: { evictServer: mockEvictServer },
+  mcpService: {
+    clearCache: mockClearCache,
+    evictServerConnections: mockEvictServerConnections,
+  },
 }))
 vi.mock('@/lib/mcp/utils', () => ({ generateMcpServerId: vi.fn() }))
 vi.mock('@/lib/posthog/server', () => posthogServerMock)
@@ -161,6 +161,6 @@ describe('MCP server lifecycle orchestration', () => {
     })
 
     expect(result.success).toBe(true)
-    expect(mockEvictServer).toHaveBeenCalledWith('server-1', expect.any(String))
+    expect(mockEvictServerConnections).toHaveBeenCalledWith('server-1', expect.any(String))
   })
 })

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

@@ -6,7 +6,6 @@ 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,
@@ -208,6 +207,7 @@ export async function performCreateMcpServer(
       })
 
       await mcpService.clearCache(params.workspaceId)
+      await mcpService.evictServerConnections(serverId, 'config changed')
       return { success: true, serverId, updated: true, authType: resolvedAuthType }
     }
 
@@ -397,7 +397,10 @@ export async function performUpdateMcpServer(
       params.timeout !== undefined ||
       params.retries !== undefined
 
-    if (shouldClearCache) await mcpService.clearCache(params.workspaceId)
+    if (shouldClearCache) {
+      await mcpService.clearCache(params.workspaceId)
+      await mcpService.evictServerConnections(params.serverId, 'config changed')
+    }
 
     recordAudit({
       workspaceId: params.workspaceId,
@@ -439,10 +442,8 @@ 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)
+    await mcpService.evictServerConnections(params.serverId, 'server deleted')
     const source =
       params.source === 'settings' || params.source === 'tool_input' ? params.source : undefined
 

apps/sim/lib/mcp/service.ts

@@ -1008,6 +1008,11 @@ class McpService {
     }
   }
 
+  /**
+   * Invalidate the MCP tool cache. This does NOT evict pooled connections —
+   * pool eviction is tied to config changes (see `evictServerConnections`), so a
+   * refresh or a single-server edit doesn't tear down unrelated warm connections.
+   */
   async clearCache(workspaceId?: string): Promise<void> {
     try {
       if (workspaceId) {
@@ -1022,7 +1027,6 @@ class McpService {
           rows.flatMap((r) => [
             this.cacheAdapter.delete(serverCacheKey(workspaceId, r.id)),
             this.cacheAdapter.delete(failureCacheKey(workspaceId, r.id)),
-            mcpConnectionPool?.evictServer(r.id, 'cache cleared'),
           ])
         )
         logger.debug(`Cleared MCP tool cache for workspace ${workspaceId} (${rows.length} servers)`)
@@ -1034,6 +1038,11 @@ class McpService {
       logger.warn('Failed to clear cache:', error)
     }
   }
+
+  /** Evict a single server's warm pooled connections (all users) — call on config change/delete. */
+  async evictServerConnections(serverId: string, reason: string): Promise<void> {
+    await mcpConnectionPool?.evictServer(serverId, reason)
+  }
 }
 
 export const mcpService = new McpService()