diff --git a/src/Infrastructure/BotSharp.Core/MCP/BotSharpMCPExtensions.cs b/src/Infrastructure/BotSharp.Core/MCP/BotSharpMCPExtensions.cs index 8eeee7b35..8da28baae 100644 --- a/src/Infrastructure/BotSharp.Core/MCP/BotSharpMCPExtensions.cs +++ b/src/Infrastructure/BotSharp.Core/MCP/BotSharpMCPExtensions.cs @@ -22,6 +22,11 @@ public static IServiceCollection AddBotSharpMCP(this IServiceCollection services if (settings != null && settings.Enabled && !settings.McpServerConfigs.IsNullOrEmpty()) { + // McpClientManager opens every connection over a client from this factory, so that + // connections to one server share a pooled handler instead of each building its own. + // Idempotent, and a host that already called it is unaffected. + services.AddHttpClient(); + services.AddScoped(); services.AddScoped(); } diff --git a/src/Infrastructure/BotSharp.Core/MCP/Hooks/MCPToolAgentHook.cs b/src/Infrastructure/BotSharp.Core/MCP/Hooks/MCPToolAgentHook.cs index ede3dd111..66313e683 100644 --- a/src/Infrastructure/BotSharp.Core/MCP/Hooks/MCPToolAgentHook.cs +++ b/src/Infrastructure/BotSharp.Core/MCP/Hooks/MCPToolAgentHook.cs @@ -50,7 +50,7 @@ private async Task> GetMcpContent(Agent agent) var mcps = agent.McpTools?.Where(x => !x.Disabled) ?? []; foreach (var item in mcps) { - var mcpClient = await mcpClientManager.GetMcpClientAsync(item.ServerId); + await using var mcpClient = await mcpClientManager.GetMcpClientAsync(item.ServerId); if (mcpClient == null) continue; var tools = await mcpClient.ListToolsAsync(); diff --git a/src/Infrastructure/BotSharp.Core/MCP/Managers/McpClientManager.cs b/src/Infrastructure/BotSharp.Core/MCP/Managers/McpClientManager.cs index 8e21d74ef..e4249f3d3 100644 --- a/src/Infrastructure/BotSharp.Core/MCP/Managers/McpClientManager.cs +++ b/src/Infrastructure/BotSharp.Core/MCP/Managers/McpClientManager.cs @@ -1,9 +1,38 @@ using BotSharp.Core.MCP.Settings; using ModelContextProtocol.Client; +using System.Net.Http; namespace BotSharp.Core.MCP.Managers; -public class McpClientManager : IDisposable +/// +/// Opens MCP clients. Each call returns a client of its own, which the caller owns and must +/// dispose; what is shared between callers is the HTTP connection underneath it. +/// +/// +/// +/// WHY NOTHING ABOVE THE SOCKET IS SHARED. An MCP 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 that session. Handing one session to +/// two callers would show one of them the other's tasks, and no per-request header can undo that +/// because it is server-side state rather than an authorization question. Since +/// lets a host open a connection as the signed-in user, +/// sharing a session would also mean sharing an identity. So sessions are never shared. +/// +/// +/// WHAT IS SHARED. The HttpClient comes from IHttpClientFactory, named per server, so every +/// connection to one server reuses a pooled HttpMessageHandler -- the same TCP and TLS the +/// factory would give any other caller. That layer carries no identity: the credential lives in +/// the transport's headers, and CreateClient hands back a fresh HttpClient each time, so headers +/// set for one caller are never seen by another. This is what makes a per-call session cheap: +/// the handshake runs over an already-warm connection. +/// +/// +/// Building the transport with its own HttpClient, as this did before, gave every MCP 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. +/// +/// +public class McpClientManager { private readonly IServiceProvider _services; private readonly ILogger _logger; @@ -16,12 +45,17 @@ public McpClientManager( _logger = logger; } + /// + /// Opens a client for . The caller owns it and must dispose it + /// -- an undisposed client leaves its session open on the server until the server times it out. + /// Answers null rather than throwing when the server is unknown, disabled or unreachable. + /// public async Task GetMcpClientAsync(string serverId) { try { var settings = _services.GetRequiredService(); - var config = settings.McpServerConfigs.Where(x => x.Id == serverId).FirstOrDefault(); + var config = settings.McpServerConfigs?.FirstOrDefault(x => x.Id == serverId); if (config == null || !config.Enabled) { return null; @@ -30,7 +64,7 @@ public McpClientManager( IClientTransport? transport = null; if (config.HttpConfig != null) { - transport = new HttpClientTransport(new HttpClientTransportOptions + transport = CreateHttpTransport(config, new HttpClientTransportOptions { Name = config.Name, Endpoint = new Uri(config.HttpConfig.EndPoint), @@ -40,7 +74,7 @@ public McpClientManager( } else if (config.SseConfig != null) { - transport = new HttpClientTransport(new HttpClientTransportOptions + transport = CreateHttpTransport(config, new HttpClientTransportOptions { Name = config.Name, Endpoint = new Uri(config.SseConfig.EndPoint), @@ -74,13 +108,40 @@ public McpClientManager( } } + /// + /// A transport over an HttpClient from the factory, named for this server so its handler -- + /// and therefore its connection pool -- is reused by every later connection to the same + /// server. The instance itself is fresh per call, which is what keeps one caller's headers + /// out of another's request. + /// + private HttpClientTransport CreateHttpTransport(McpServerConfigModel config, HttpClientTransportOptions options) + { + var factory = _services.GetRequiredService(); + var http = factory.CreateClient(HttpClientName(config.Id)); + + // Timeout is left at the factory default (100s) deliberately: no configured tool is + // expected to run that long. Note this is a cap the SDK's own HttpClient may not have + // had, so it arrived with this change -- a server whose transport keeps a GET open for + // the session (SSE, or streamable HTTP with a standalone listening stream) would be cut + // off at 100s no matter how quick its tools are. The symptom is a tool call failing with + // a canceled request; the fix is Timeout.InfiniteTimeSpan here. + + return new HttpClientTransport(options, http, loggerFactory: null, ownsHttpClient: true); + } + + /// + /// One handler pool per server, so a slow or unhealthy server cannot occupy the connections + /// of the others. + /// + private static string HttpClientName(string serverId) => $"mcp:{serverId}"; + /// /// The headers to open a connection with: the ones from configuration, unless the host has /// registered an that wants to adjust them. /// /// /// No provider is registered by default, and a provider is free to answer with what it was - /// given, so a host without one — or with one that does not recognise this server — gets the + /// given, so a host without one -- or with one that does not recognise this server -- gets the /// configured headers back untouched. /// private Dictionary? ResolveHeaders(string serverId, Dictionary? configured) @@ -88,9 +149,4 @@ public McpClientManager( var provider = _services.GetService(); return provider == null ? configured : provider.GetHeaders(serverId, configured); } - - public void Dispose() - { - - } } diff --git a/src/Infrastructure/BotSharp.Core/MCP/Services/McpService.cs b/src/Infrastructure/BotSharp.Core/MCP/Services/McpService.cs index 7dd20daf6..5b7d1c33c 100644 --- a/src/Infrastructure/BotSharp.Core/MCP/Services/McpService.cs +++ b/src/Infrastructure/BotSharp.Core/MCP/Services/McpService.cs @@ -28,7 +28,7 @@ public async Task> GetServerConfigsAsync() foreach (var config in configs) { - var client = await clientManager.GetMcpClientAsync(config.Id); + await using var client = await clientManager.GetMcpClientAsync(config.Id); if (client == null) continue; var tools = await client.ListToolsAsync(); diff --git a/src/Infrastructure/BotSharp.Core/Routing/Executor/MCPToolExecutor.cs b/src/Infrastructure/BotSharp.Core/Routing/Executor/MCPToolExecutor.cs index 08be74599..06d345ff1 100644 --- a/src/Infrastructure/BotSharp.Core/Routing/Executor/MCPToolExecutor.cs +++ b/src/Infrastructure/BotSharp.Core/Routing/Executor/MCPToolExecutor.cs @@ -27,7 +27,10 @@ public async Task ExecuteAsync(RoleDialogModel message) Dictionary argDict = JsonToDictionary(message.FunctionArgs); var clientManager = _services.GetRequiredService(); - var client = await clientManager.GetMcpClientAsync(_mcpServerId); + + // The client is a session of its own, so this call owns it. Disposing closes the + // session on the server; the connection underneath it stays in the factory's pool. + await using var client = await clientManager.GetMcpClientAsync(_mcpServerId); if (client == null) {