Skip to content

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

Open
GeiserX wants to merge 1 commit into
UsefulSoftwareCo:mainfrom
GeiserX:fix/mcp-pool-idle-sweep
Open

Sweep every expired MCP connection, not just the one being asked for#1577
GeiserX wants to merge 1 commit into
UsefulSoftwareCo:mainfrom
GeiserX:fix/mcp-pool-idle-sweep

Conversation

@GeiserX

@GeiserX GeiserX commented Aug 13, 2026

Copy link
Copy Markdown

TL;DR

The pool's five-minute idle window was only checked against the entry being requested. An identity that was never dialled 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.

acquire now sweeps every expired entry, not just idle.get(key). Still lazy in the sense the pool intends: activity drives it, no timer, no background fiber. The map holds at most one entry per identity, so the scan is trivial.

Reuse is untouched — an entry inside the window is left alone, and a repeat call for the same identity still gets the parked session.


Why I think this is a bug rather than a preference

The interface doc-comment says "lazy five-minute idle eviction", and I want to be straight that I read that as deliberate before deciding it was worth changing.

My reading is that "lazy" describes the mechanism — no timer — and "five-minute idle eviction" describes the bound. The mechanism was intact; the bound was not. A connection idle for five hours was still open, still authenticated, and would have stayed that way indefinitely, because the only code that could have retired it ran solely when someone asked for that exact key again.

That is precisely the case where nobody asks again: a user tries an integration once and moves on. So the connections most likely to be retained forever are the ones least likely to be reused.

The change keeps the property the comment was protecting — no background work — while making the number in it true. If you intended the narrower behaviour, I am happy to close this; the reasoning is above so you can judge it directly rather than take my word.

What is held

A pooled McpConnection is a live authenticated transport. For a remote server the client is built with the rendered Authorization: Bearer <token> or API-key header, and/or an authProvider whose tokens() returns the access token verbatim. So the retained object is not just an idle socket — it is a credential and a session someone can still use.

Two related observations, offered as context rather than as part of this PR:

  • Nothing outside the pool can evict an entry, so removing a connection, removing an integration, or rotating a credential all leave a live session authenticated with the old value. An eviction hook would close that, but it is a larger change and yours to shape.
  • The pool is emptied only by close(). On the hosted path makeScopedExecutor builds plugins per request, and I could not find a finalizer that closes the resulting executor — so those pools appear to be dropped without closing rather than living to shutdown. I may well have missed the place it happens; worth a glance.

Tests

Three, driven by a fake connector rather than a real MCP server, because what is under test is exactly when close() is called and a fake makes that directly observable instead of inferred from session counts.

Mutation-checked, each mutation verified to have landed, with an unmutated control before and after:

mutation result
remove the sweep from acquire (i.e. revert this PR) killed
sweep ignores the TTL and closes everything killed (2 tests — including the reuse guard)
sweep drops entries but never closes them killed

The second mutation is why the "still inside the window" and "still reuses the same key" tests exist: a sweep that quietly closed everything would satisfy the headline test on its own while destroying pooling.

Package: 129 passed / 16 files. tsgo --noEmit, oxlint --deny-warnings and oxfmt --check clean.

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.
@GeiserX

GeiserX commented Aug 13, 2026

Copy link
Copy Markdown
Author

Context for this one: #1585 explains why this PR and twelve others exist — they came out of a single pass over credential handling, asking for each credential where it ends up, how long it stays, and who can read it once it's there.

This PR stands alone and doesn't depend on any of the others.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant