From 4435c0ac49429f749a2ebf537dc6f43efa4b1768 Mon Sep 17 00:00:00 2001 From: GeiserX <9169332+GeiserX@users.noreply.github.com> Date: Thu, 13 Aug 2026 03:43:31 +0200 Subject: [PATCH 1/3] Sweep every expired MCP connection, not just the one being asked for The idle window was consulted only against idle.get(key), so an identity that was never dialled again was never examined again: its session stayed open and authenticated for the pool's lifetime, holding the credential it was dialled with. The advertised five-minute bound applied only to connections that happened to be reused. acquire now closes every entry past the window. Still lazy in the sense the pool intends -- activity drives it, no timer, no background fiber -- and the map holds at most one entry per identity, so the scan is trivial. Reuse is unchanged. --- .changeset/mcp-pool-idle-sweep.md | 11 ++ .../mcp/src/sdk/connection-pool-sweep.test.ts | 103 ++++++++++++++++++ .../plugins/mcp/src/sdk/connection-pool.ts | 30 ++++- 3 files changed, 143 insertions(+), 1 deletion(-) create mode 100644 .changeset/mcp-pool-idle-sweep.md create mode 100644 packages/plugins/mcp/src/sdk/connection-pool-sweep.test.ts diff --git a/.changeset/mcp-pool-idle-sweep.md b/.changeset/mcp-pool-idle-sweep.md new file mode 100644 index 000000000..4c3816201 --- /dev/null +++ b/.changeset/mcp-pool-idle-sweep.md @@ -0,0 +1,11 @@ +--- +"executor": patch +--- + +**Idle MCP connections age out even when their identity is never dialled again** + +The pool's five-minute idle window was only consulted against the entry being requested, so an identity that was never asked for a second time was never examined a second time. Its session stayed open and authenticated for as long as the pool lived, holding the bearer token or API key it was dialled with. The advertised bound applied only to connections that happened to be reused. + +`acquire` now sweeps every entry past the window, closing each one, rather than just the entry matching the key. This stays lazy in the sense the pool intends — activity drives it, there is no timer and no background fiber — and the map holds at most one entry per identity, so the scan is trivial. + +Reuse is unchanged: an entry still inside the window is left alone, and a second call for the same identity still gets the parked session rather than a fresh dial. diff --git a/packages/plugins/mcp/src/sdk/connection-pool-sweep.test.ts b/packages/plugins/mcp/src/sdk/connection-pool-sweep.test.ts new file mode 100644 index 000000000..cbc72f058 --- /dev/null +++ b/packages/plugins/mcp/src/sdk/connection-pool-sweep.test.ts @@ -0,0 +1,103 @@ +// --------------------------------------------------------------------------- +// Idle eviction must reach every parked connection, not only the one being +// asked for. +// +// The TTL used to be consulted against `idle.get(key)` alone, so an identity +// that was never dialled again was never examined again — its session stayed +// open and authenticated indefinitely, holding the bearer it was dialled with. +// The advertised five-minute bound only applied to connections that happened to +// be reused. +// +// Driven with a fake connector rather than a real MCP server, because the thing +// under test is exactly WHEN `close()` is called, and a fake makes that directly +// observable instead of inferred from session counts. +// --------------------------------------------------------------------------- + +import { describe, expect, it } from "@effect/vitest"; +import { Effect } from "effect"; +// oxlint-disable-next-line executor/no-vitest-import -- boundary: system-time control comes from vitest itself +import { afterEach, vi } from "vitest"; + +import type { McpConnection, McpConnector } from "./connection"; +import { createMcpConnectionPool } from "./connection-pool"; + +const IDLE_TTL_MS = 5 * 60 * 1_000; + +afterEach(() => { + vi.useRealTimers(); +}); + +/** A connector whose connection records the moment it is closed. */ +const fakeConnector = (state: { closed: boolean }): McpConnector => + Effect.sync( + () => + ({ + client: {} as McpConnection["client"], + close: async () => { + state.closed = true; + }, + }) satisfies McpConnection, + ); + +describe("MCP connection pool idle sweep", () => { + it.effect("closes an expired connection parked under a DIFFERENT key", () => + Effect.gen(function* () { + vi.useFakeTimers(); + const pool = createMcpConnectionPool(); + const stale = { closed: false }; + const other = { closed: false }; + + // Park a connection under "stale" and never ask for that key again. + yield* pool.withConnection("stale", fakeConnector(stale), () => Effect.void); + expect(stale.closed).toBe(false); + + vi.advanceTimersByTime(IDLE_TTL_MS + 1_000); + + // Activity on an UNRELATED key is what must now reclaim it. + yield* pool.withConnection("other", fakeConnector(other), () => Effect.void); + + expect(stale.closed).toBe(true); + yield* pool.close(); + }), + ); + + it.effect("leaves a connection that is still inside the idle window alone", () => + Effect.gen(function* () { + // The other half: sweeping must not become "close everything on any + // activity", which would destroy pooling while still passing the test + // above. + vi.useFakeTimers(); + const pool = createMcpConnectionPool(); + const fresh = { closed: false }; + const other = { closed: false }; + + yield* pool.withConnection("fresh", fakeConnector(fresh), () => Effect.void); + vi.advanceTimersByTime(IDLE_TTL_MS - 1_000); + yield* pool.withConnection("other", fakeConnector(other), () => Effect.void); + + expect(fresh.closed).toBe(false); + yield* pool.close(); + }), + ); + + it.effect("still reuses a parked connection for the same key", () => + Effect.gen(function* () { + // Guards the pool's whole reason for existing: a sweep that quietly broke + // reuse would leave both tests above green. + vi.useFakeTimers(); + const pool = createMcpConnectionPool(); + const first = { closed: false }; + let dials = 0; + const counting: McpConnector = Effect.suspend(() => { + dials += 1; + return fakeConnector(first); + }); + + yield* pool.withConnection("same", counting, () => Effect.void); + yield* pool.withConnection("same", counting, () => Effect.void); + + expect(dials).toBe(1); + yield* pool.close(); + }), + ); +}); diff --git a/packages/plugins/mcp/src/sdk/connection-pool.ts b/packages/plugins/mcp/src/sdk/connection-pool.ts index 6b732caf7..a8518d430 100644 --- a/packages/plugins/mcp/src/sdk/connection-pool.ts +++ b/packages/plugins/mcp/src/sdk/connection-pool.ts @@ -60,12 +60,40 @@ export interface McpConnectionPool { } /** Creates an MCP connection pool with lazy five-minute idle eviction and one - * automatic fresh-dial retry for a reused session rejected with HTTP 404. */ + * automatic fresh-dial retry for a reused session rejected with HTTP 404. + * + * "Lazy" means activity-driven — there is no timer and no background fiber — but + * it applies to EVERY parked connection, not only the identity being asked for. + * A pooled session holds the credential it was dialled with, so an identity that + * is never requested again must still age out. */ export const createMcpConnectionPool = (): McpConnectionPool => { const idle = new Map(); + /** Close and drop every entry past the idle window, not just the one being + * asked for. + * + * The TTL used to be consulted only against `idle.get(key)`, so an identity + * that was never dialled again was never examined again: its session stayed + * open and authenticated indefinitely, holding the bearer it was dialled + * with. The advertised bound only held for connections that happened to be + * reused. + * + * Still lazy — activity drives it, there is no timer and no background fiber. + * The map holds at most one entry per identity, so scanning it is trivial. */ + const sweepExpired = Effect.suspend(() => { + const now = Date.now(); + const expired: McpConnection[] = []; + for (const [key, entry] of idle) { + if (now - entry.idleSince < IDLE_TTL_MS) continue; + idle.delete(key); + expired.push(entry.connection); + } + return Effect.forEach(expired, closeQuietly, { discard: true }); + }); + const acquire = (key: string, connector: McpConnector, forceFresh: boolean) => Effect.gen(function* () { + yield* sweepExpired; if (forceFresh) { const connection = yield* connector; return { connection, reused: false } satisfies ConnectionLease; From ed274cb6744e89184e395113b10ea6f0ed528bc8 Mon Sep 17 00:00:00 2001 From: GeiserX <9169332+GeiserX@users.noreply.github.com> Date: Sun, 16 Aug 2026 14:09:22 +0200 Subject: [PATCH 2/3] docs(mcp): state the changeset's idle bound as acquire-driven, not unconditional The headline is what lands in the published CHANGELOG, and it promised idle sessions age out with no qualifier. The sweep only runs inside acquire: a pool that sees no further activity holds a parked session until close(). Condition the headline on the pool's next acquire so the CHANGELOG does not promise a bound the code does not provide. --- .changeset/mcp-pool-idle-sweep.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/mcp-pool-idle-sweep.md b/.changeset/mcp-pool-idle-sweep.md index 4c3816201..00ddeccbe 100644 --- a/.changeset/mcp-pool-idle-sweep.md +++ b/.changeset/mcp-pool-idle-sweep.md @@ -2,7 +2,7 @@ "executor": patch --- -**Idle MCP connections age out even when their identity is never dialled again** +**Idle MCP connections age out on the pool's next acquire, even when their identity is never dialled again** The pool's five-minute idle window was only consulted against the entry being requested, so an identity that was never asked for a second time was never examined a second time. Its session stayed open and authenticated for as long as the pool lived, holding the bearer token or API key it was dialled with. The advertised bound applied only to connections that happened to be reused. From 9a5f35aa3a7d463385369757b22a8a091e821c11 Mon Sep 17 00:00:00 2001 From: Rhys Sullivan <39114868+RhysSullivan@users.noreply.github.com> Date: Thu, 27 Aug 2026 21:21:04 -0700 Subject: [PATCH 3/3] Bound and parallelise the MCP pool idle sweep so a hung close cannot stall an acquire --- .changeset/mcp-pool-idle-sweep.md | 2 + .../mcp/src/sdk/connection-pool-sweep.test.ts | 43 ++++++++++++++++++- .../plugins/mcp/src/sdk/connection-pool.ts | 25 +++++++++-- 3 files changed, 65 insertions(+), 5 deletions(-) diff --git a/.changeset/mcp-pool-idle-sweep.md b/.changeset/mcp-pool-idle-sweep.md index 00ddeccbe..56e194401 100644 --- a/.changeset/mcp-pool-idle-sweep.md +++ b/.changeset/mcp-pool-idle-sweep.md @@ -8,4 +8,6 @@ The pool's five-minute idle window was only consulted against the entry being re `acquire` now sweeps every entry past the window, closing each one, rather than just the entry matching the key. This stays lazy in the sense the pool intends — activity drives it, there is no timer and no background fiber — and the map holds at most one entry per identity, so the scan is trivial. +Because the sweep is paid for by whichever invocation acquires next, it cannot be allowed to stall that caller. The expired entries leave the pool synchronously, before any close is awaited, and the closes then run concurrently with each one bounded by a two-second timeout — so a server that accepts a close and goes quiet is abandoned rather than waited on, and cannot hold up an unrelated request or the connections queued behind it. + Reuse is unchanged: an entry still inside the window is left alone, and a second call for the same identity still gets the parked session rather than a fresh dial. diff --git a/packages/plugins/mcp/src/sdk/connection-pool-sweep.test.ts b/packages/plugins/mcp/src/sdk/connection-pool-sweep.test.ts index cbc72f058..0b83b0636 100644 --- a/packages/plugins/mcp/src/sdk/connection-pool-sweep.test.ts +++ b/packages/plugins/mcp/src/sdk/connection-pool-sweep.test.ts @@ -14,7 +14,8 @@ // --------------------------------------------------------------------------- import { describe, expect, it } from "@effect/vitest"; -import { Effect } from "effect"; +import { Duration, Effect, Fiber } from "effect"; +import { TestClock } from "effect/testing"; // oxlint-disable-next-line executor/no-vitest-import -- boundary: system-time control comes from vitest itself import { afterEach, vi } from "vitest"; @@ -39,6 +40,17 @@ const fakeConnector = (state: { closed: boolean }): McpConnector => }) satisfies McpConnection, ); +/** A connection whose `close()` is accepted and then never answered — the + * server that goes quiet mid-teardown. */ +const hangingConnector = (): McpConnector => + Effect.sync( + () => + ({ + client: {} as McpConnection["client"], + close: () => new Promise(() => {}), + }) satisfies McpConnection, + ); + describe("MCP connection pool idle sweep", () => { it.effect("closes an expired connection parked under a DIFFERENT key", () => Effect.gen(function* () { @@ -100,4 +112,33 @@ describe("MCP connection pool idle sweep", () => { yield* pool.close(); }), ); + + it.effect("a close that never answers does not strand the acquire that swept it", () => + Effect.gen(function* () { + // The sweep is paid for by whichever invocation happens to acquire next, + // so an unresponsive teardown is a live caller's latency. Only `Date` is + // faked here: the wait being asserted is an Effect sleep, which belongs to + // `it.effect`'s TestClock, and faking the platform timers underneath it + // would leave nothing to advance. + vi.useFakeTimers({ toFake: ["Date"] }); + const pool = createMcpConnectionPool(); + const other = { closed: false }; + + yield* pool.withConnection("hung", hangingConnector(), () => Effect.void); + vi.advanceTimersByTime(IDLE_TTL_MS + 1_000); + + const fiber = yield* Effect.forkChild( + pool.withConnection("other", fakeConnector(other), () => Effect.void), + ); + + // Past the close timeout, but nowhere near "forever": an unbounded close + // would leave this fiber suspended and the join below would never return. + yield* TestClock.adjust(Duration.seconds(5)); + yield* Fiber.join(fiber); + + // The unrelated connection was served, not collateral damage. + expect(other.closed).toBe(false); + yield* pool.close(); + }), + ); }); diff --git a/packages/plugins/mcp/src/sdk/connection-pool.ts b/packages/plugins/mcp/src/sdk/connection-pool.ts index 53f2c5023..c17bdbf5e 100644 --- a/packages/plugins/mcp/src/sdk/connection-pool.ts +++ b/packages/plugins/mcp/src/sdk/connection-pool.ts @@ -1,4 +1,4 @@ -import { Cause, Effect, Exit, Predicate } from "effect"; +import { Cause, Duration, Effect, Exit, Predicate } from "effect"; import type { McpConnection, McpConnector } from "./connection"; import type { McpInvocationError } from "./errors"; @@ -9,6 +9,16 @@ import type { McpInvocationError } from "./errors"; const IDLE_TTL_MS = 5 * 60 * 1_000; +/** How long a `close()` is waited on before the connection is abandoned. + * + * Eviction is driven by live traffic — the invocation that acquires a lease is + * the one that runs the sweep — so an unbounded close is that caller's problem: + * a server that accepts the close and then goes quiet would hold up a request + * that has nothing to do with the connection being reclaimed. Teardown is + * milliseconds' work when it works at all, so anything past this window is a + * socket that is not coming back. */ +const CLOSE_TIMEOUT = Duration.seconds(2); + type IdleConnection = { readonly connection: McpConnection; readonly idleSince: number; @@ -20,7 +30,7 @@ type ConnectionLease = { }; const closeQuietly = (connection: McpConnection): Effect.Effect => - Effect.tryPromise(() => connection.close()).pipe(Effect.ignore); + Effect.tryPromise(() => connection.close()).pipe(Effect.timeout(CLOSE_TIMEOUT), Effect.ignore); const isMcpInvocationError = (error: unknown): error is McpInvocationError => Predicate.isTagged(error, "McpInvocationError"); @@ -83,7 +93,14 @@ export const createMcpConnectionPool = (): McpConnectionPool => { * reused. * * Still lazy — activity drives it, there is no timer and no background fiber. - * The map holds at most one entry per identity, so scanning it is trivial. */ + * The map holds at most one entry per identity, so scanning it is trivial. + * + * The entries leave the map synchronously, before any close is awaited, so a + * slow teardown can never hand the same connection to a second caller. The + * closes themselves run concurrently and each is bounded by `CLOSE_TIMEOUT`, + * the same shape `close()` below uses: the sweep is work the acquiring + * invocation pays for, and one unresponsive server must not be able to stall + * it, let alone stall the connections queued behind it. */ const sweepExpired = Effect.suspend(() => { const now = Date.now(); const expired: McpConnection[] = []; @@ -92,7 +109,7 @@ export const createMcpConnectionPool = (): McpConnectionPool => { idle.delete(key); expired.push(entry.connection); } - return Effect.forEach(expired, closeQuietly, { discard: true }); + return Effect.forEach(expired, closeQuietly, { concurrency: "unbounded", discard: true }); }); const acquire = (key: string, connector: McpConnector, forceFresh: boolean) =>