Feature/mcp client header provider - #1414
Open
yuyixg wants to merge 3 commits into
Open
Conversation
GetMcpClientAsync built a fresh transport and a fresh McpClient on every call, and Dispose did nothing, so a turn that listed a server's tools and then called three of them opened four connections and closed none of them. Pooling a connection means reusing whatever headers IMcpClientHeaderProvider answered with, and that is an identity. The pool is therefore an instance field of this class, which is registered per DI scope -- one HTTP request, one crontab run, one queued message -- so everything sharing a pool is already the same caller, and one user's connection cannot be handed to another. That is structural rather than a rule someone has to remember. The pool key also folds in a SHA-256 of the headers a connection opens with, so the guarantee survives this class later being registered with a longer lifetime: two credentials land on two entries even inside one pool. It is a hash of secrets, so it is never logged, and a test pins that down along with the three identities OneBrainMcpHeaderProvider can answer with never sharing an entry. Headers are now resolved once and handed to both the key and the transport. Resolving separately for each let the two disagree, and the key is the thing keeping one caller's connection away from another. Entries hold Lazy<Task<McpClient?>> so concurrent callers wanting the same server open one connection between them rather than one each. A failed connection is removed rather than cached, and McpToolExecutor now drops the pooled client when a call fails: keeping a dead one fails every remaining call in the scope, while discarding a live one costs a single reconnect. McpClient only implements IAsyncDisposable, so the manager implements both disposal interfaces. Async scopes get DisposeAsync; scopes created with CreateScope tear down synchronously and get a bounded wait instead, because a wedged transport must not hang the unit of work that is trying to finish. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Pooling MCP clients, as the previous commit did, shares more than a socket. A client is a session: CreateAsync performs the initialize handshake, the server answers with a session id, and subscriptions and long-running tool tasks (ListTasksAsync, GetTaskResultAsync) live on it. Two callers on one session would see each other's tasks, and no per-request header can undo that, because it is server-side state rather than an authorization question. With IMcpClientHeaderProvider opening connections as the signed-in user, sharing a session would mean sharing an identity as well. So sessions are not shared at all now: every GetMcpClientAsync call opens its own and the caller owns it. The three call sites hold it in an await using, which closes the session on the server instead of leaving it to time out -- the leak the empty Dispose used to cause, and the reason the pool existed. What is shared instead is the layer that carries no identity. The HttpClient comes from IHttpClientFactory, named per server, so connections to one server reuse a pooled HttpMessageHandler. CreateClient hands back a fresh HttpClient each time, so one caller's headers are never seen by another. Building the transport with its own HttpClient, as this did before, gave every connection a private handler and therefore a private socket pool -- the usual way to exhaust sockets and to keep talking to an address DNS has already moved. AddBotSharpMCP now calls AddHttpClient so the factory it depends on is present. The call is idempotent, and a host that already registered one is unaffected. Timeout is left at the factory default. No configured tool is expected to run for 100 seconds, but that cap is one the SDK's own client may not have had, so a comment records the symptom and the one-line fix should a server keep a GET open for the length of its session. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
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
2 participants
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.
No description provided.