Feature/mcp client header provider - #1414
Merged
yileicn merged 7 commits intoSep 3, 2026
Merged
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? |
A model routinely asks for several independent tools at once. Both providers kept FirstOrDefault of that set and dropped the rest, so the calls it did not get an answer for came back on the next turn -- the same lookups, run again, one per round trip. RoleDialogModel.ToolCalls now carries the whole set, in the order the model produced it. The single FunctionName, FunctionArgs and ToolCallId fields beside it are the first entry, computed from the same ordered list they were before, so a caller that can only run one call -- the routing engine, every agent on it -- sees exactly what it saw. From deliberately does not copy ToolCalls: it describes one model reply, and a message derived from that reply is not it. The streaming path had a second problem behind the first. Argument fragments arrive chunked and were concatenated into a single string across all calls, which is correct while there is one call and produces one malformed blob as soon as there are two. They are now accumulated per call, keyed by the tool call id that is present when a call opens, since the SDK update carries no index. The first call therefore has valid arguments where it used to have garbage. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
An agent that writes a sentence before calling a tool is saying something worth keeping -- it is the reasoning behind the call -- but it is not a message to the user, and rendering it would read as a half answer followed by a real one. MessageTypeName.Internal marks such a message. It is stored, read back into the model context like any other, and skipped when the dialog endpoint renders a conversation. Nothing in either repository produced this type before, so every message already in storage renders exactly as it did. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Listing tools costs a session of its own -- a handshake, a notification and a stream, three or four HTTP round trips -- and every agent load paid it again for a list that changes when a server is redeployed, not between two messages of one conversation. Measured against a remote server it was 0.3 to 0.5 seconds of every turn, before the model had been asked anything. The listing is now reused for McpSettings.ToolListCacheSeconds, sixty by default: short enough that a tool added upstream shows up while someone is still testing it, long enough that no conversation pays for the listing twice. Zero restores the old behaviour. The entry is keyed by the headers the connection would carry rather than by the server alone. IMcpClientHeaderProvider lets a host open the connection as the signed-in user, so a server that shows one caller a different set of tools than another must never be served one caller from the other one's entry. The headers are fingerprinted, so no credential ends up in a cache key. Two things are deliberately not cached. A failed or empty listing is not, because a server that is briefly unreachable would otherwise leave every agent that depends on it answering from its prompt alone -- with no tools and no error -- for the length of the window. And callers get their own FunctionDef instances over the shared parameter schemas, so an agent that rewrites a description on the way to the model cannot rewrite it for every other agent on the same server. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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
3 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.