Sweep every expired MCP connection, not just the one being asked for

.changeset/mcp-pool-idle-sweep.md

@@ -0,0 +1,13 @@
+---
+"executor": patch
+---
+
+**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.
+
+`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.

packages/plugins/mcp/src/sdk/connection-pool-sweep.test.ts

@@ -0,0 +1,144 @@
+// ---------------------------------------------------------------------------
+// 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 { 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";
+
+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,
+  );
+
+/** 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<void>(() => {}),
+      }) 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();
+    }),
+  );
+
+  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();
+    }),
+  );
+});

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<void> =>
-  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");
@@ -64,12 +74,47 @@ 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<string, IdleConnection>();
 
+  /** 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.
+   *
+   *  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[] = [];
+    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, { concurrency: "unbounded", 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;