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] 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 0000000000..4c38162014 --- /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 0000000000..cbc72f058d --- /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 6b732caf78..a8518d430e 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;