fix(mcp): decouple pool validity from updatedAt, widen dead-connection detection, guard ping race

Author: waleedlatif1

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

@@ -41,13 +41,17 @@ function makeFakeClient(init: { connected?: boolean; pingRejects?: boolean } = {
 }
 
 /** Acquire + immediately release (models a completed borrow). */
-async function borrow(pool: McpConnectionPool, params: AcquireParams, poison = false): Promise<void> {
+async function borrow(
+  pool: McpConnectionPool,
+  params: AcquireParams,
+  poison = false
+): Promise<void> {
   const lease = await pool.acquire(params)
   await lease.release(poison)
 }
 
-function params(key: string, create: () => Promise<McpClient>, configUpdatedAt?: string): AcquireParams {
-  return { key, serverId: key.split(':')[0], configUpdatedAt, create }
+function params(key: string, create: () => Promise<McpClient>): AcquireParams {
+  return { key, serverId: key.split(':')[0], create }
 }
 
 describe('McpConnectionPool', () => {
@@ -108,23 +112,6 @@ describe('McpConnectionPool', () => {
     expect(createB).toHaveBeenCalledTimes(1)
   })
 
-  it('rebuilds and disconnects the old connection when the config changes', async () => {
-    const oldClient = makeFakeClient()
-    const newClient = makeFakeClient()
-    const create = vi
-      .fn<() => Promise<McpClient>>()
-      .mockResolvedValueOnce(oldClient)
-      .mockResolvedValueOnce(newClient)
-
-    await borrow(pool, params('s1:w1:u1', create, '2026-01-01T00:00:00.000Z'))
-    const lease = await pool.acquire(params('s1:w1:u1', create, '2026-01-02T00:00:00.000Z'))
-
-    expect(create).toHaveBeenCalledTimes(2)
-    expect(oldClient.disconnect).toHaveBeenCalledTimes(1)
-    expect(lease.client).toBe(newClient)
-    await lease.release()
-  })
-
   it('rebuilds after the max connection age', async () => {
     const oldClient = makeFakeClient()
     const newClient = makeFakeClient()
@@ -139,6 +126,7 @@ describe('McpConnectionPool', () => {
 
     expect(create).toHaveBeenCalledTimes(2)
     expect(oldClient.disconnect).toHaveBeenCalledTimes(1)
+    expect(lease.client).toBe(newClient)
     await lease.release()
   })
 
@@ -234,7 +222,10 @@ describe('McpConnectionPool', () => {
 
   it('idle-evicts a connection once no borrower holds it', async () => {
     const client = makeFakeClient()
-    await borrow(pool, params('s1:w1:u1', async () => client))
+    await borrow(
+      pool,
+      params('s1:w1:u1', async () => client)
+    )
 
     await vi.advanceTimersByTimeAsync(6 * 60 * 1000)
     expect(client.disconnect).toHaveBeenCalledTimes(1)
@@ -248,13 +239,14 @@ describe('McpConnectionPool', () => {
       .mockResolvedValueOnce(oldClient)
       .mockResolvedValueOnce(newClient)
 
-    // Force a rebuild via config change; oldClient is now retired, newClient pooled.
-    await borrow(pool, params('s1:w1:u1', create, '2026-01-01T00:00:00.000Z'))
-    await borrow(pool, params('s1:w1:u1', create, '2026-01-02T00:00:00.000Z'))
+    // oldClient's transport closes → retired; the next acquire pools newClient.
+    await borrow(pool, params('s1:w1:u1', create))
+    oldClient.__fireClose()
+    await borrow(pool, params('s1:w1:u1', create))
 
-    // The old client's late close must NOT drop the replacement.
+    // A late duplicate close from the old client must NOT drop the replacement.
     oldClient.__fireClose()
-    await borrow(pool, params('s1:w1:u1', create, '2026-01-02T00:00:00.000Z'))
+    await borrow(pool, params('s1:w1:u1', create))
 
     // Still 2 creates — the replacement survived the stale close.
     expect(create).toHaveBeenCalledTimes(2)

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

@@ -15,6 +15,11 @@
  * so a stale `onClose` can't disconnect a replacement stored under the same key.
  * Pools are per-process. Callers must release every acquired lease and must not
  * disconnect the client themselves.
+ *
+ * Config-change invalidation is the caller's job via `evictServer` (wired to the
+ * service's `clearCache`), not a per-acquire timestamp check — `updatedAt` is also
+ * bumped by status telemetry, so it can't distinguish a config edit. Cross-process,
+ * a config edit is bounded by max age (and self-heals when a stale connection errors).
  */
 
 import { createLogger } from '@sim/logger'
@@ -36,8 +41,6 @@ export interface AcquireParams {
   key: string
   /** Server id, for bulk `evictServer` on config change/delete. */
   serverId: string
-  /** Server config `updatedAt`; a newer value than the pooled entry rebuilds it. */
-  configUpdatedAt?: string
   /** Builds and connects a fresh client; invoked once per miss (single-flight). */
   create: () => Promise<McpClient>
 }
@@ -53,21 +56,13 @@ interface PoolEntry {
   serverId: string
   client: McpClient
   createdAt: number
-  configUpdatedAtMs: number
   lastActivityAt: number
   lastLivenessCheckAt: number
   borrowers: number
   retired: boolean
   closing: boolean
 }
 
-/** ISO → epoch ms; `0` (unknown) is always ≤ a real stamp, so it never rebuilds. */
-function toEpochMs(iso: string | undefined): number {
-  if (!iso) return 0
-  const ms = Date.parse(iso)
-  return Number.isNaN(ms) ? 0 : ms
-}
-
 export class McpConnectionPool {
   private entries = new Map<string, PoolEntry>()
   private pending = new Map<string, Promise<PoolEntry>>()
@@ -89,30 +84,22 @@ export class McpConnectionPool {
   private async resolveEntry(params: AcquireParams): Promise<PoolEntry> {
     const pending = this.pending.get(params.key)
     if (pending) {
-      // A joiner may hold a newer config than the create used; rebuild if so.
       const entry = await pending
-      if (!entry.retired && toEpochMs(params.configUpdatedAt) <= entry.configUpdatedAtMs) {
-        return entry
-      }
-      this.retire(entry)
+      if (!entry.retired) return entry
       return this.resolveEntry(params)
     }
 
     const current = this.entries.get(params.key)
     if (current) {
-      const reusable = await this.tryReuse(params, current)
+      const reusable = await this.tryReuse(current)
       if (reusable) return reusable
     }
     return this.createEntry(params)
   }
 
-  /** Return `entry` if config-fresh, in-age, connected, and live; else retire it and return null. */
-  private async tryReuse(params: AcquireParams, entry: PoolEntry): Promise<PoolEntry | null> {
+  /** Return `entry` if in-age, connected, and live; else retire it and return null. */
+  private async tryReuse(entry: PoolEntry): Promise<PoolEntry | null> {
     const now = Date.now()
-    if (toEpochMs(params.configUpdatedAt) > entry.configUpdatedAtMs) {
-      this.retire(entry)
-      return null
-    }
     if (now - entry.createdAt > MAX_CONNECTION_AGE_MS) {
       this.retire(entry)
       return null
@@ -126,6 +113,9 @@ export class McpConnectionPool {
         .ping(LIVENESS_PING_TIMEOUT_MS)
         .then(() => true)
         .catch(() => false)
+      // A concurrent poison/evict/age eviction may have retired this during the
+      // ping await; don't hand out a connection that's already closing.
+      if (entry.retired) return null
       if (!alive) {
         this.retire(entry)
         return null
@@ -149,7 +139,6 @@ export class McpConnectionPool {
         serverId: params.serverId,
         client,
         createdAt: now,
-        configUpdatedAtMs: toEpochMs(params.configUpdatedAt),
         lastActivityAt: now,
         lastLivenessCheckAt: now,
         borrowers: 0,

apps/sim/lib/mcp/service.ts

@@ -101,12 +101,29 @@ function isTimeoutError(error: unknown): boolean {
   return getErrorMessage(error, '').toLowerCase().includes('timed out')
 }
 
-/** A pooled connection is dead and must be retired — a stale session or a closed transport, not a benign tool/consent error. */
+/**
+ * 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.
+ */
 function isDeadConnectionError(error: unknown): boolean {
   if (error instanceof StreamableHTTPError) {
     return error.code === 404 || error.code === 400
   }
-  return error instanceof McpError && error.code === ErrorCode.ConnectionClosed
+  if (error instanceof McpError && error.code === ErrorCode.ConnectionClosed) {
+    return true
+  }
+  if (isTimeoutError(error)) {
+    return true
+  }
+  const message = getErrorMessage(error, '').toLowerCase()
+  return (
+    message.includes('econnreset') ||
+    message.includes('econnrefused') ||
+    message.includes('epipe') ||
+    message.includes('socket hang up')
+  )
 }
 
 /** Transient failures a read-only `tools/list` may safely retry (idempotent, unlike `tools/call`); excludes OAuth and terminal 4xx. */
@@ -327,7 +344,7 @@ class McpService {
    * Otherwise connect one-shot and always disconnect.
    */
   private async withServerClient<T>(
-    opts: { key: string; serverId: string; configUpdatedAt?: string; allowPool: boolean },
+    opts: { key: string; serverId: string; allowPool: boolean },
     create: () => Promise<McpClient>,
     fn: (client: McpClient) => Promise<T>
   ): Promise<T> {
@@ -336,7 +353,6 @@ class McpService {
       const lease = await pool.acquire({
         key: opts.key,
         serverId: opts.serverId,
-        configUpdatedAt: opts.configUpdatedAt,
         create,
       })
       let poison = false
@@ -400,7 +416,6 @@ class McpService {
           {
             key: this.poolKey(serverId, workspaceId, userId),
             serverId,
-            configUpdatedAt: config.updatedAt,
             allowPool: !hasExtraHeaders,
           },
           create,
@@ -644,7 +659,6 @@ class McpService {
               {
                 key: this.poolKey(config.id, workspaceId, userId),
                 serverId: config.id,
-                configUpdatedAt: config.updatedAt,
                 allowPool: true,
               },
               () => this.createClient(resolvedConfig, resolvedIP, userId),
@@ -863,7 +877,6 @@ class McpService {
           {
             key: this.poolKey(serverId, workspaceId, userId),
             serverId,
-            configUpdatedAt: config.updatedAt,
             allowPool: true,
           },
           create,
@@ -940,7 +953,6 @@ class McpService {
             {
               key: this.poolKey(config.id, workspaceId, userId),
               serverId: config.id,
-              configUpdatedAt: config.updatedAt,
               allowPool: true,
             },
             create,