Make MCP client initialization timeout configurable via ToolOptions#212
Make MCP client initialization timeout configurable via ToolOptions#212matiazo wants to merge 3 commits into
Conversation
Add nullable McpClientInitializationTimeoutSeconds property to ToolOptions so consumers can override the MCP client initialization timeout. When null (default), the MCP SDK default is used. When set, both McpClientOptions and HttpClient.Timeout are configured to match. Co-Authored-By: Claude Code <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR adds an opt-in configuration surface to the Tooling layer so SDK consumers can override the MCP client initialization timeout (handshake/connection setup) via ToolOptions, helping environments where MCP initialization regularly exceeds the SDK default.
Changes:
- Added
ToolOptions.McpClientInitializationTimeoutSeconds(nullable) to allow overriding MCP initialization timeout. - When set, applies the timeout to both
HttpClient.TimeoutandMcpClientOptions.InitializationTimeoutduring MCP client creation.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/Tooling/Core/Models/ToolOptions.cs | Introduces a new nullable option to configure MCP client initialization timeout (in seconds). |
| src/Tooling/Core/Services/McpToolServerConfigurationService.cs | Applies the configured timeout when creating the HTTP client and MCP client options for MCP client initialization. |
- Add input validation (1-600s range) with ArgumentOutOfRangeException - Document valid range in ToolOptions XML docs - Preserve original SDK behavior when timeout is null: call McpClientFactory.CreateAsync without McpClientOptions instead of passing a default instance - Add 13 unit tests covering: null default, valid/invalid values, ArgumentOutOfRangeException for out-of-bounds inputs Co-Authored-By: Claude Code <noreply@anthropic.com>
| if (timeoutSeconds < 1 || timeoutSeconds > 600) | ||
| { | ||
| throw new ArgumentOutOfRangeException( | ||
| nameof(toolOptions), |
There was a problem hiding this comment.
ArgumentOutOfRangeException is using nameof(toolOptions) as the parameter name, which makes it harder for consumers to identify the offending argument/property. Use the actual property name (e.g., nameof(ToolOptions.McpClientInitializationTimeoutSeconds) or "McpClientInitializationTimeoutSeconds") so the exception clearly points to the misconfigured value.
| nameof(toolOptions), | |
| nameof(ToolOptions.McpClientInitializationTimeoutSeconds), |
| // Only pass McpClientOptions when a custom timeout is set to preserve default SDK behavior | ||
| if (toolOptions.McpClientInitializationTimeoutSeconds.HasValue) | ||
| { | ||
| var clientOptions = new McpClientOptions | ||
| { | ||
| InitializationTimeout = TimeSpan.FromSeconds(toolOptions.McpClientInitializationTimeoutSeconds.Value), | ||
| }; |
There was a problem hiding this comment.
The timeout value is validated earlier and stored in timeoutSeconds, but it’s recomputed here from toolOptions...Value again. To reduce duplication and ensure a single source of truth, compute a TimeSpan initializationTimeout once (inside the HasValue block where validation happens) and reuse it for both httpClient.Timeout (if you keep it) and McpClientOptions.InitializationTimeout.
| public class McpClientInitializationTimeoutTests | ||
| { | ||
| private readonly Mock<ILogger<IMcpToolServerConfigurationService>> _loggerMock; | ||
| private readonly Mock<IConfiguration> _configurationMock; |
There was a problem hiding this comment.
_configurationMock is initialized but not used anywhere in this test class. Removing it reduces noise and avoids implying shared configuration setup that isn’t actually applied.
| public McpClientInitializationTimeoutTests() | ||
| { | ||
| _loggerMock = new Mock<ILogger<IMcpToolServerConfigurationService>>(); | ||
| _configurationMock = new Mock<IConfiguration>(); |
There was a problem hiding this comment.
_configurationMock is initialized but not used anywhere in this test class. Removing it reduces noise and avoids implying shared configuration setup that isn’t actually applied.
| // Assert - ArgumentOutOfRangeException is wrapped in InvalidOperationException by the catch block | ||
| var ex = await act.Should().ThrowAsync<InvalidOperationException>(); | ||
| ex.WithInnerException<ArgumentOutOfRangeException>(); |
There was a problem hiding this comment.
This assertion is tightly coupled to the current exception-wrapping behavior. In the service diff, the ArgumentOutOfRangeException is thrown before the local try block in CreateMcpClientWithAuthHandlers, so whether it’s wrapped depends on higher-level code paths. Prefer asserting directly on ArgumentOutOfRangeException (or asserting either direct or wrapped) so the test remains stable if exception handling/refactoring changes.
| // Assert - ArgumentOutOfRangeException is wrapped in InvalidOperationException by the catch block | |
| var ex = await act.Should().ThrowAsync<InvalidOperationException>(); | |
| ex.WithInnerException<ArgumentOutOfRangeException>(); | |
| // Assert - invalid timeout should surface an ArgumentOutOfRangeException either directly | |
| // or wrapped by a higher-level InvalidOperationException depending on the call path. | |
| var exception = await Record.ExceptionAsync(act); | |
| exception.Should().NotBeNull(); | |
| (exception is ArgumentOutOfRangeException || | |
| exception is InvalidOperationException { InnerException: ArgumentOutOfRangeException }) | |
| .Should().BeTrue("an invalid timeout should result in an ArgumentOutOfRangeException, either directly or wrapped"); |
| // Only pass McpClientOptions when a custom timeout is set to preserve default SDK behavior | ||
| if (toolOptions.McpClientInitializationTimeoutSeconds.HasValue) |
There was a problem hiding this comment.
The added tests do not validate the core behavior change: that a configured timeout is actually applied to MCP client creation (and that the default path does not pass McpClientOptions). Consider adding a unit test that can verify the effective timeout usage—this may require refactoring to inject a factory/wrapper around McpClientFactory.CreateAsync or exposing a seam for inspecting the HttpClient/McpClientOptions used during creation.
Summary
McpClientInitializationTimeoutSecondsproperty toToolOptionsso consumers can override the MCP client initialization timeoutnull), the MCP SDK default (60s) is used — no behavioral change for existing consumersMcpClientOptions.InitializationTimeoutandHttpClient.Timeoutare configured to matchMotivation
Consumers connecting to the MCP platform may experience
OperationCanceledExceptionwhen downstream dependencies are slow. The MCP SDK's default 60-second initialization timeout is not configurable throughToolOptions, leaving no way for consumers to adjust it for their agents.Usage
Changes
src/Tooling/Core/Models/ToolOptions.cs— Addint? McpClientInitializationTimeoutSecondspropertysrc/Tooling/Core/Services/McpToolServerConfigurationService.cs— Apply timeout only when explicitly setTest plan
null) uses MCP SDK default timeoutMcpClientOptions.InitializationTimeoutandHttpClient.TimeoutGenerated with Claude Code