From 69a22a299e91d8224f5971598959f55347b538ab Mon Sep 17 00:00:00 2001 From: "Eric St. John" Date: Thu, 19 Mar 2026 10:46:53 -0700 Subject: [PATCH 1/3] Fix process crash when testing Stderr --- .../Client/StdioClientTransport.cs | 25 +++++++++++++------ .../Transport/StdioClientTransportTests.cs | 11 ++++++++ 2 files changed, 29 insertions(+), 7 deletions(-) diff --git a/src/ModelContextProtocol.Core/Client/StdioClientTransport.cs b/src/ModelContextProtocol.Core/Client/StdioClientTransport.cs index 24a47613b..5ae5011b4 100644 --- a/src/ModelContextProtocol.Core/Client/StdioClientTransport.cs +++ b/src/ModelContextProtocol.Core/Client/StdioClientTransport.cs @@ -151,7 +151,17 @@ public async Task ConnectAsync(CancellationToken cancellationToken = stderrRollingLog.Enqueue(data); } - _options.StandardErrorLines?.Invoke(data); + try + { + _options.StandardErrorLines?.Invoke(data); + } + catch (Exception ex) + { + // Prevent exceptions in the user callback from propagating + // to the background thread that dispatches ErrorDataReceived, + // which would crash the process. + LogStderrCallbackFailed(logger, endpointName, ex); + } LogReadStderr(logger, endpointName, data); } @@ -230,12 +240,10 @@ internal static void DisposeProcess( // Ensure all redirected stderr/stdout events have been dispatched // before disposing. Only the no-arg WaitForExit() guarantees this; - // WaitForExit(int) (as used by KillTree) does not. - // This should not hang: either the process already exited on its own - // (no child processes holding handles), or KillTree killed the entire - // process tree. If it does take too long, the test infrastructure's - // own timeout will catch it. - if (!processRunning && HasExited(process)) + // WaitForExit(int) (as used by KillTree) does not. We must call + // this whether the process exited on its own or was killed above, + // otherwise ErrorDataReceived handlers may fire after Dispose(). + if (HasExited(process)) { process.WaitForExit(); } @@ -299,6 +307,9 @@ private static string EscapeArgumentString(string argument) => [LoggerMessage(Level = LogLevel.Information, Message = "{EndpointName} received stderr log: '{Data}'.")] private static partial void LogReadStderr(ILogger logger, string endpointName, string data); + [LoggerMessage(Level = LogLevel.Warning, Message = "{EndpointName} StandardErrorLines callback failed.")] + private static partial void LogStderrCallbackFailed(ILogger logger, string endpointName, Exception exception); + [LoggerMessage(Level = LogLevel.Information, Message = "{EndpointName} started server process with PID {ProcessId}.")] private static partial void LogTransportProcessStarted(ILogger logger, string endpointName, int processId); diff --git a/tests/ModelContextProtocol.Tests/Transport/StdioClientTransportTests.cs b/tests/ModelContextProtocol.Tests/Transport/StdioClientTransportTests.cs index d84ea9377..1a999fd14 100644 --- a/tests/ModelContextProtocol.Tests/Transport/StdioClientTransportTests.cs +++ b/tests/ModelContextProtocol.Tests/Transport/StdioClientTransportTests.cs @@ -59,6 +59,17 @@ public async Task CreateAsync_ValidProcessInvalidServer_StdErrCallbackInvoked() Assert.Contains(id, sb.ToString()); } + [Fact(Skip = "Platform not supported by this test.", SkipUnless = nameof(IsStdErrCallbackSupported))] + public async Task CreateAsync_StdErrCallbackThrows_DoesNotCrashProcess() + { + StdioClientTransport transport = RuntimeInformation.IsOSPlatform(OSPlatform.Windows) ? + new(new() { Command = "cmd", Arguments = ["/c", "echo fail >&2 & exit /b 1"], StandardErrorLines = _ => throw new InvalidOperationException("boom") }, LoggerFactory) : + new(new() { Command = "sh", Arguments = ["-c", "echo fail >&2; exit 1"], StandardErrorLines = _ => throw new InvalidOperationException("boom") }, LoggerFactory); + + // Should throw IOException for the failed server, not crash the host process. + await Assert.ThrowsAnyAsync(() => McpClient.CreateAsync(transport, loggerFactory: LoggerFactory, cancellationToken: TestContext.Current.CancellationToken)); + } + [Theory] [InlineData(null)] [InlineData("argument with spaces")] From ee120f6f810f4e9077e489c8dbea8f610d3a297f Mon Sep 17 00:00:00 2001 From: "Eric St. John" Date: Thu, 19 Mar 2026 19:28:14 -0700 Subject: [PATCH 2/3] Clarify comment. --- .../Client/StdioClientTransport.cs | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/ModelContextProtocol.Core/Client/StdioClientTransport.cs b/src/ModelContextProtocol.Core/Client/StdioClientTransport.cs index 5ae5011b4..d7919934c 100644 --- a/src/ModelContextProtocol.Core/Client/StdioClientTransport.cs +++ b/src/ModelContextProtocol.Core/Client/StdioClientTransport.cs @@ -238,11 +238,12 @@ internal static void DisposeProcess( process.KillTree(shutdownTimeout); } - // Ensure all redirected stderr/stdout events have been dispatched - // before disposing. Only the no-arg WaitForExit() guarantees this; - // WaitForExit(int) (as used by KillTree) does not. We must call - // this whether the process exited on its own or was killed above, - // otherwise ErrorDataReceived handlers may fire after Dispose(). + // When a process has exited either on it's own or via KillTree, we must + // call WaitForExit() to ensure all redirected stderr/stdout events have + // been dispatched, otherwise ErrorDataReceived handlers may fire after + // Dispose(). + // The HasExited check is here to avoid waiting forever on a process that + // failed to be killed. if (HasExited(process)) { process.WaitForExit(); From 2539d5ca6bd598681e58855a2af72e380b757940 Mon Sep 17 00:00:00 2001 From: "Eric St. John" Date: Fri, 20 Mar 2026 18:01:00 -0700 Subject: [PATCH 3/3] Fix additional crashes identified in dumps. --- .../ClientConformanceTests.cs | 10 +- .../ServerConformanceTests.cs | 10 +- .../Program.cs | 145 ++++++++++-------- 3 files changed, 94 insertions(+), 71 deletions(-) diff --git a/tests/ModelContextProtocol.AspNetCore.Tests/ClientConformanceTests.cs b/tests/ModelContextProtocol.AspNetCore.Tests/ClientConformanceTests.cs index 1418574a7..29c5302f5 100644 --- a/tests/ModelContextProtocol.AspNetCore.Tests/ClientConformanceTests.cs +++ b/tests/ModelContextProtocol.AspNetCore.Tests/ClientConformanceTests.cs @@ -82,11 +82,13 @@ public async Task RunConformanceTest(string scenario) var process = new Process { StartInfo = startInfo }; + // Protect callbacks with try/catch to prevent ITestOutputHelper from + // throwing on a background thread if events arrive after the test completes. process.OutputDataReceived += (sender, e) => { if (e.Data != null) { - _output.WriteLine(e.Data); + try { _output.WriteLine(e.Data); } catch { } outputBuilder.AppendLine(e.Data); } }; @@ -95,7 +97,7 @@ public async Task RunConformanceTest(string scenario) { if (e.Data != null) { - _output.WriteLine(e.Data); + try { _output.WriteLine(e.Data); } catch { } errorBuilder.AppendLine(e.Data); } }; @@ -119,6 +121,10 @@ public async Task RunConformanceTest(string scenario) ); } + // Ensure all redirected stdout/stderr events have been dispatched before + // the test completes and ITestOutputHelper becomes invalid. + process.WaitForExit(); + var output = outputBuilder.ToString(); var error = errorBuilder.ToString(); var success = process.ExitCode == 0 || HasOnlyWarnings(output, error); diff --git a/tests/ModelContextProtocol.AspNetCore.Tests/ServerConformanceTests.cs b/tests/ModelContextProtocol.AspNetCore.Tests/ServerConformanceTests.cs index d2501456c..e6607b803 100644 --- a/tests/ModelContextProtocol.AspNetCore.Tests/ServerConformanceTests.cs +++ b/tests/ModelContextProtocol.AspNetCore.Tests/ServerConformanceTests.cs @@ -136,11 +136,13 @@ public async Task RunPendingConformanceTest_ServerSsePolling() var process = new Process { StartInfo = startInfo }; + // Protect callbacks with try/catch to prevent ITestOutputHelper from + // throwing on a background thread if events arrive after the test completes. process.OutputDataReceived += (sender, e) => { if (e.Data != null) { - output.WriteLine(e.Data); + try { output.WriteLine(e.Data); } catch { } outputBuilder.AppendLine(e.Data); } }; @@ -149,7 +151,7 @@ public async Task RunPendingConformanceTest_ServerSsePolling() { if (e.Data != null) { - output.WriteLine(e.Data); + try { output.WriteLine(e.Data); } catch { } errorBuilder.AppendLine(e.Data); } }; @@ -173,6 +175,10 @@ public async Task RunPendingConformanceTest_ServerSsePolling() ); } + // Ensure all redirected stdout/stderr events have been dispatched before + // the test completes and ITestOutputHelper becomes invalid. + process.WaitForExit(); + return ( Success: process.ExitCode == 0, Output: outputBuilder.ToString(), diff --git a/tests/ModelContextProtocol.ConformanceClient/Program.cs b/tests/ModelContextProtocol.ConformanceClient/Program.cs index 40dc424cb..b5e048dd0 100644 --- a/tests/ModelContextProtocol.ConformanceClient/Program.cs +++ b/tests/ModelContextProtocol.ConformanceClient/Program.cs @@ -105,85 +105,96 @@ OAuth = oauthOptions, }, loggerFactory: consoleLoggerFactory); -await using var mcpClient = await McpClient.CreateAsync(clientTransport, options, loggerFactory: consoleLoggerFactory); +try +{ + await using var mcpClient = await McpClient.CreateAsync(clientTransport, options, loggerFactory: consoleLoggerFactory); -bool success = true; + bool success = true; -switch (scenario) -{ - case "tools_call": + switch (scenario) { - var tools = await mcpClient.ListToolsAsync(); - Console.WriteLine($"Available tools: {string.Join(", ", tools.Select(t => t.Name))}"); + case "tools_call": + { + var tools = await mcpClient.ListToolsAsync(); + Console.WriteLine($"Available tools: {string.Join(", ", tools.Select(t => t.Name))}"); - // Call the "add_numbers" tool - var toolName = "add_numbers"; - Console.WriteLine($"Calling tool: {toolName}"); - var result = await mcpClient.CallToolAsync(toolName: toolName, arguments: new Dictionary + // Call the "add_numbers" tool + var toolName = "add_numbers"; + Console.WriteLine($"Calling tool: {toolName}"); + var result = await mcpClient.CallToolAsync(toolName: toolName, arguments: new Dictionary + { + { "a", 5 }, + { "b", 10 } + }); + success &= !(result.IsError == true); + break; + } + case "elicitation-sep1034-client-defaults": { - { "a", 5 }, - { "b", 10 } - }); - success &= !(result.IsError == true); - break; - } - case "elicitation-sep1034-client-defaults": - { - var tools = await mcpClient.ListToolsAsync(); - Console.WriteLine($"Available tools: {string.Join(", ", tools.Select(t => t.Name))}"); - var toolName = "test_client_elicitation_defaults"; - Console.WriteLine($"Calling tool: {toolName}"); - var result = await mcpClient.CallToolAsync(toolName: toolName, arguments: new Dictionary()); - success &= !(result.IsError == true); - break; - } - case "sse-retry": - { - var tools = await mcpClient.ListToolsAsync(); - Console.WriteLine($"Available tools: {string.Join(", ", tools.Select(t => t.Name))}"); - var toolName = "test_reconnection"; - Console.WriteLine($"Calling tool: {toolName}"); - var result = await mcpClient.CallToolAsync(toolName: toolName, arguments: new Dictionary()); - success &= !(result.IsError == true); - break; - } - case "auth/scope-step-up": - { - // Just testing that we can authenticate and list tools - var tools = await mcpClient.ListToolsAsync(); - Console.WriteLine($"Available tools: {string.Join(", ", tools.Select(t => t.Name))}"); - - // Call the "test_tool" tool - var toolName = "test-tool"; - Console.WriteLine($"Calling tool: {toolName}"); - var result = await mcpClient.CallToolAsync(toolName: toolName, arguments: new Dictionary + var tools = await mcpClient.ListToolsAsync(); + Console.WriteLine($"Available tools: {string.Join(", ", tools.Select(t => t.Name))}"); + var toolName = "test_client_elicitation_defaults"; + Console.WriteLine($"Calling tool: {toolName}"); + var result = await mcpClient.CallToolAsync(toolName: toolName, arguments: new Dictionary()); + success &= !(result.IsError == true); + break; + } + case "sse-retry": { - { "foo", "bar" }, - }); - success &= !(result.IsError == true); - break; - } - case "auth/scope-retry-limit": - { - // Try to list tools - this triggers the auth flow that always fails with 403. - // The test validates the client doesn't retry indefinitely. - try + var tools = await mcpClient.ListToolsAsync(); + Console.WriteLine($"Available tools: {string.Join(", ", tools.Select(t => t.Name))}"); + var toolName = "test_reconnection"; + Console.WriteLine($"Calling tool: {toolName}"); + var result = await mcpClient.CallToolAsync(toolName: toolName, arguments: new Dictionary()); + success &= !(result.IsError == true); + break; + } + case "auth/scope-step-up": { - await mcpClient.ListToolsAsync(); + // Just testing that we can authenticate and list tools + var tools = await mcpClient.ListToolsAsync(); + Console.WriteLine($"Available tools: {string.Join(", ", tools.Select(t => t.Name))}"); + + // Call the "test_tool" tool + var toolName = "test-tool"; + Console.WriteLine($"Calling tool: {toolName}"); + var result = await mcpClient.CallToolAsync(toolName: toolName, arguments: new Dictionary + { + { "foo", "bar" }, + }); + success &= !(result.IsError == true); + break; } - catch (Exception ex) + case "auth/scope-retry-limit": { - Console.WriteLine($"Expected auth failure: {ex.Message}"); + // Try to list tools - this triggers the auth flow that always fails with 403. + // The test validates the client doesn't retry indefinitely. + try + { + await mcpClient.ListToolsAsync(); + } + catch (Exception ex) + { + Console.WriteLine($"Expected auth failure: {ex.Message}"); + } + break; } - break; + default: + // No extra processing for other scenarios + break; } - default: - // No extra processing for other scenarios - break; -} -// Exit code 0 on success, 1 on failure -return success ? 0 : 1; + // Exit code 0 on success, 1 on failure + return success ? 0 : 1; +} +catch (Exception ex) +{ + // Report the error to stderr and exit with a non-zero code rather than + // crashing the process with an unhandled exception. An unhandled exception + // generates a crash dump which can abort the parent test host. + Console.Error.WriteLine($"Conformance client failed: {ex}"); + return 1; +} // Copied from ProtectedMcpClient sample // Simulate a user opening the browser and logging in