Skip to content

Fix process crash when testing Stderr #1449

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ericstj wants to merge 3 commits into
base: main
Choose a base branch
from
+126 −80
Open

Fix process crash when testing Stderr #1449

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
69a22a2
Fix process crash when testing Stderr
ericstj Mar 19, 2026
ee120f6
Clarify comment.
ericstj Mar 20, 2026
2539d5c
Fix additional crashes identified in dumps.
ericstj Mar 21, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 21 additions & 9 deletions src/ModelContextProtocol.Core/Client/StdioClientTransport.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,17 @@ public async Task<ITransport> 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);
}
Expand Down Expand Up @@ -228,14 +238,13 @@ 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.
// 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))
// 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))
Copy link
Contributor

@stephentoub stephentoub Mar 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand this change or the comment on it. If HasExited return false, how does this prevent ErrorDataReceived from firing after Dispose?

Copy link
Contributor Author

@ericstj ericstj Mar 20, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's what Copilot says:

The guard is there because WaitForExit() (no-arg) blocks indefinitely if the process hasn't exited.

Consider the scenarios at that point in the code:

Scenario HasExited What happens
Process exited on its own true Call WaitForExit() to flush events ✓
KillTree killed it true Call WaitForExit() to flush events ✓
KillTree called but process didn't die (edge case) false Skip — avoids hanging forever
Process never started (processStarted=false) throws → true Caught by the outer try

Without the guard, an unkillable process (or a race between kill and exit check) would cause WaitForExit() to hang the calling thread indefinitely. The tradeoff is that in that rare edge case, some ErrorDataReceived events may not be fully dispatched before Dispose() — but that's better than a deadlock.

HasExited returns true when the process has exited, but the no-arg WaitForExit() does additional work beyond waiting for exit — it drains the async stdout/stderr output readers.

Here are the docs that confirm that indeed you must call WaitForExit() to flush redirected events.
https://learn.microsoft.com/en-us/dotnet/api/system.diagnostics.process.waitforexit?view=net-10.0#system-diagnostics-process-waitforexit(system-int32)

Seems we could have a better name such an API - like Flush() or something.

Copy link
Contributor

@stephentoub stephentoub Mar 20, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not understand that response. My question wasn't about whether there needs to be a guard on the HasExited call... there already was a guard prior to this PR. This PR is just changing that guard from:

if (!processRunning && HasExited(process))

to

if (HasExited(process))

and the value of processRunning is not impacted at all based on whether KillTree was successful or not.

Moreover, the comment that was added suggests that "we must call [WaitForExit] [...] otherwise ErrorDataReceived handlers may fire after Dispose()" but there's nothing here preventing that race condition when we don't call WaitForExit because the process hasn't yet exited by the time we get here. So the logic is lost on me.

What am I missing?

Copy link
Contributor Author

@ericstj ericstj Mar 20, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did the comment update help? Your !processRunning check was causing this to not call WaitForExit() in the KillTree case, since we only call KillTree when processRunning is true. So in that KillTree case we wouldn't flush the handlers at all. KillTree, when successful should cause the process to synchronously exit and HasExited will be true. The process exiting of its own accord also causes the HasExited to be true. I don't think we need a guard on HasExited but I can't be sure why it was placed there before. It seems to contradict the comment.

We can't remove HasExited check since that would let us call WaitForExit even in the case that the process was hung, so we need to keep that check. There is of course the case that the process might exit on it's own after our HasExited check (if it just exceeds the shutown timeout) - that will still result in a race, but we accept uncommon race condition in favor of a hang. Maybe we should any event handlers instead?

This code is really difficult to understand because those names - HasExited means the native process actually exited, WaitForExit is called not to wait for that to happen but to wait to flush any pending redirected output events.

{
process.WaitForExit();
}
Expand Down Expand Up @@ -299,6 +308,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);

Expand Down
10 changes: 8 additions & 2 deletions tests/ModelContextProtocol.AspNetCore.Tests/ClientConformanceTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
};
Expand All @@ -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);
}
};
Expand All @@ -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);
Expand Down
10 changes: 8 additions & 2 deletions tests/ModelContextProtocol.AspNetCore.Tests/ServerConformanceTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
};
Expand All @@ -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);
}
};
Expand All @@ -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(),
Expand Down
145 changes: 78 additions & 67 deletions tests/ModelContextProtocol.ConformanceClient/Program.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<string, object?>
// Call the "add_numbers" tool
var toolName = "add_numbers";
Console.WriteLine($"Calling tool: {toolName}");
var result = await mcpClient.CallToolAsync(toolName: toolName, arguments: new Dictionary<string, object?>
{
{ "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<string, object?>());
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<string, object?>());
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<string, object?>
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<string, object?>());
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<string, object?>());
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<string, object?>
{
{ "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
Expand Down
11 changes: 11 additions & 0 deletions tests/ModelContextProtocol.Tests/Transport/StdioClientTransportTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<IOException>(() => McpClient.CreateAsync(transport, loggerFactory: LoggerFactory, cancellationToken: TestContext.Current.CancellationToken));
}

[Theory]
[InlineData(null)]
[InlineData("argument with spaces")]
Expand Down
Loading