From 009ef065959bf2bfd6403d6b38fc70485315cf5f Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Mon, 20 Jul 2026 13:55:05 -0700 Subject: [PATCH] improvement(mcp): bound pool create-invalidation to in-flight creates; document DCR match --- apps/sim/app/api/mcp/oauth/start/route.ts | 7 ++++ apps/sim/lib/mcp/connection-pool.ts | 48 ++++++++++++++++------- 2 files changed, 40 insertions(+), 15 deletions(-) diff --git a/apps/sim/app/api/mcp/oauth/start/route.ts b/apps/sim/app/api/mcp/oauth/start/route.ts index 882befca8d7..5bc08af493f 100644 --- a/apps/sim/app/api/mcp/oauth/start/route.ts +++ b/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() diff --git a/apps/sim/lib/mcp/connection-pool.ts b/apps/sim/lib/mcp/connection-pool.ts index e367fb7fd68..47e162668ca 100644 --- a/apps/sim/lib/mcp/connection-pool.ts +++ b/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 +} + export class McpConnectionPool { private entries = new Map() - private pending = new Map>() - /** Per-server counter bumped by `evictServer`; an in-flight create built against an older value is retired on completion. */ - private serverGenerations = new Map() + private pending = new Map() private idleCheckTimer: ReturnType | null = null private disposed = false @@ -91,7 +101,7 @@ export class McpConnectionPool { private async resolveEntry(params: AcquireParams): Promise { 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 { @@ -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 { - // 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())) } }