Sweep every expired MCP connection, not just the one being asked for - #1577
Open
GeiserX wants to merge 1 commit into
Open
Sweep every expired MCP connection, not just the one being asked for#1577GeiserX wants to merge 1 commit into
GeiserX wants to merge 1 commit into
Conversation
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.
This was referenced Aug 13, 2026
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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.
acquirenow sweeps every expired entry, not justidle.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
McpConnectionis a live authenticated transport. For a remote server the client is built with the renderedAuthorization: Bearer <token>or API-key header, and/or anauthProviderwhosetokens()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:
close(). On the hosted pathmakeScopedExecutorbuilds 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:
acquire(i.e. revert this PR)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-warningsandoxfmt --checkclean.