improvement(mcp): bound pool create-invalidation to in-flight creates; document DCR match (#5777)

apps/sim/app/api/mcp/oauth/start/route.ts

@@ -28,6 +28,13 @@ const MAX_SURFACED_ERROR_LENGTH = 250
 const DCR_UNSUPPORTED_MESSAGE =
   "This server doesn't support automatic OAuth client registration. Add a pre-registered OAuth client ID and secret, or configure a token instead."
 
+/**
+ * The MCP SDK throws a plain `Error` (no typed class or code) when an auth server lacks a
+ * `registration_endpoint`, so this string-match is the only available signal. Pinned to
+ * `@modelcontextprotocol/sdk` v1.29.0 `registerClient` (client/auth.js): "Incompatible auth
+ * server: does not support dynamic client registration". If a version bump rephrases this, the
+ * 422 branch silently stops firing (users get a generic 500) — update the substring here.
+ */
 function isDynamicClientRegistrationUnsupported(error: unknown): boolean {
   return getErrorMessage(error, '')
     .toLowerCase()

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

@@ -64,11 +64,21 @@ interface PoolEntry {
   closing: boolean
 }
 
+/**
+ * An in-flight `createEntry`. `evictServer` sets `invalidated` on the records for its
+ * server so a connect racing a config edit/delete is one-shot instead of pooled stale.
+ * Lives only until the create settles (cleared in `finally`), so it's self-bounded.
+ */
+interface PendingCreate {
+  serverId: string
+  /** `flag.invalidated` is set by `evictServer` racing this create → it one-shots instead of pooling. */
+  flag: { invalidated: boolean }
+  promise: Promise<PoolEntry>
+}
+
 export class McpConnectionPool {
   private entries = new Map<string, PoolEntry>()
-  private pending = new Map<string, Promise<PoolEntry>>()
-  /** Per-server counter bumped by `evictServer`; an in-flight create built against an older value is retired on completion. */
-  private serverGenerations = new Map<string, number>()
+  private pending = new Map<string, PendingCreate>()
   private idleCheckTimer: ReturnType<typeof setInterval> | null = null
   private disposed = false
 
@@ -91,7 +101,7 @@ export class McpConnectionPool {
   private async resolveEntry(params: AcquireParams): Promise<PoolEntry> {
     while (true) {
       const pending = this.pending.get(params.key)
-      if (pending) return pending
+      if (pending) return pending.promise
 
       const current = this.entries.get(params.key)
       if (!current) return this.createEntry(params)
@@ -136,10 +146,12 @@ export class McpConnectionPool {
     // Re-check: the `await` in `resolveEntry` yields, so two acquires can both
     // reach here; the first registers the pending create, the rest join.
     const inFlight = this.pending.get(params.key)
-    if (inFlight) return inFlight
+    if (inFlight) return inFlight.promise
 
+    // Declared before the create closure so `evictServer` can invalidate a create
+    // that's racing a config edit/delete (the closure reads it after its `await`).
+    const flag = { invalidated: false }
     const creation = (async () => {
-      const generation = this.serverGenerations.get(params.serverId) ?? 0
       const client = await params.create()
       const now = Date.now()
       const entry: PoolEntry = {
@@ -157,7 +169,7 @@ export class McpConnectionPool {
       // 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) {
+      if (this.disposed || flag.invalidated) {
         entry.retired = true
         return entry
       }
@@ -171,10 +183,15 @@ export class McpConnectionPool {
       return entry
     })()
 
-    this.pending.set(params.key, creation)
-    return creation.finally(() => {
-      if (this.pending.get(params.key) === creation) this.pending.delete(params.key)
-    })
+    const record: PendingCreate = {
+      serverId: params.serverId,
+      flag,
+      promise: creation.finally(() => {
+        if (this.pending.get(params.key) === record) this.pending.delete(params.key)
+      }),
+    }
+    this.pending.set(params.key, record)
+    return record.promise
   }
 
   private async release(entry: PoolEntry, poison: boolean): Promise<void> {
@@ -215,9 +232,11 @@ export class McpConnectionPool {
 
   /** Retire every connection for a server (all users) — config changed or deleted. */
   async evictServer(serverId: string, reason: string): Promise<void> {
-    // Bump first so an in-flight create for this server retires on completion
-    // instead of pooling a connection built against the now-stale config.
-    this.serverGenerations.set(serverId, (this.serverGenerations.get(serverId) ?? 0) + 1)
+    // Flag in-flight creates first so one racing this eviction is one-shot on
+    // completion instead of pooling a connection built against the now-stale config.
+    for (const record of this.pending.values()) {
+      if (record.serverId === serverId) record.flag.invalidated = true
+    }
     for (const entry of this.entries.values()) {
       if (entry.serverId === serverId) this.retire(entry, reason)
     }
@@ -262,7 +281,6 @@ export class McpConnectionPool {
     for (const entry of entries) entry.retired = true
     this.entries.clear()
     this.pending.clear()
-    this.serverGenerations.clear()
     void Promise.allSettled(entries.map((entry) => entry.client.disconnect()))
   }
 }