Skip to content

Commit 7ff9d2f

Browse files
ericstjCopilot
andcommitted
Fix stderr drain cancelled prematurely by CancelShutdown race
GetUnexpectedExitExceptionAsync used a linked CancellationTokenSource that included the caller's token (_shutdownCts.Token). When ReadMessagesAsync and DisposeAsync race to call CleanupAsync, the loser calls CancelShutdown() which cancels _shutdownCts. This prematurely aborted WaitForExitAsync in the winner's cleanup path, preventing stderr pipe buffers from being fully drained. The ErrorDataReceived callback would never fire for lines still in the pipe, causing CreateAsync_ValidProcessInvalidServer_StdErrCallbackInvoked to fail intermittently. Fix: use a standalone timeout CTS instead of linking to the caller's token. The process is already dead at this point—we only need a timeout to bound the pipe drain, not external cancellation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent 0665aaf commit 7ff9d2f

File tree

1 file changed

+7
-8
lines changed

1 file changed

+7
-8
lines changed

src/ModelContextProtocol.Core/Client/StdioClientSessionTransport.cs

Lines changed: 7 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,7 @@ public override async Task SendMessageAsync(JsonRpcMessage message, Cancellation
3535
{
3636
// We failed to send due to an I/O error. If the server process has exited, which is then very likely the cause
3737
// for the I/O error, we should throw an exception for that instead.
38-
if (await GetUnexpectedExitExceptionAsync(cancellationToken).ConfigureAwait(false) is Exception processExitException)
38+
if (await GetUnexpectedExitExceptionAsync().ConfigureAwait(false) is Exception processExitException)
3939
{
4040
throw processExitException;
4141
}
@@ -59,7 +59,7 @@ protected override async ValueTask CleanupAsync(Exception? error = null, Cancell
5959

6060
// We've not yet forcefully terminated the server. If it's already shut down, something went wrong,
6161
// so create an exception with details about that.
62-
error ??= await GetUnexpectedExitExceptionAsync(cancellationToken).ConfigureAwait(false);
62+
error ??= await GetUnexpectedExitExceptionAsync().ConfigureAwait(false);
6363

6464
// Detach the stderr handler so no further ErrorDataReceived events
6565
// are dispatched during or after process disposal.
@@ -88,7 +88,7 @@ protected override async ValueTask CleanupAsync(Exception? error = null, Cancell
8888
await base.CleanupAsync(error, cancellationToken).ConfigureAwait(false);
8989
}
9090

91-
private async ValueTask<Exception?> GetUnexpectedExitExceptionAsync(CancellationToken cancellationToken)
91+
private async ValueTask<Exception?> GetUnexpectedExitExceptionAsync()
9292
{
9393
if (!StdioClientTransport.HasExited(_process))
9494
{
@@ -100,12 +100,11 @@ protected override async ValueTask CleanupAsync(Exception? error = null, Cancell
100100
{
101101
// The process has exited, but we still need to ensure stderr has been flushed.
102102
// Use a bounded wait: the process is already dead, we're just draining pipe
103-
// buffers. If the caller's token is never canceled (e.g. _shutdownCts hasn't
104-
// been canceled yet), an unbounded wait here can hang indefinitely when the
105-
// threadpool is slow to deliver the stderr EOF callback.
103+
// buffers. Don't link to the caller's token—a concurrent CancelShutdown() call
104+
// would cancel the drain prematurely and lose stderr lines that haven't been
105+
// delivered via ErrorDataReceived yet.
106106
#if NET
107-
using var timeoutCts = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken);
108-
timeoutCts.CancelAfter(_options.ShutdownTimeout);
107+
using var timeoutCts = new CancellationTokenSource(_options.ShutdownTimeout);
109108
await _process.WaitForExitAsync(timeoutCts.Token).ConfigureAwait(false);
110109
#else
111110
_process.WaitForExit((int)_options.ShutdownTimeout.TotalMilliseconds);

0 commit comments

Comments
 (0)