Improve exception diagnostics for stdio client (#376)

* Improve exception diagnostics for stdio client

Track the last several lines of stderr and include those in an exception created as part of CleanupAsync if the server process has already exited. This is based on an assumption that the server should never exit prior to CleanupAsync being called.

* Address feedback and fix test failure

Author: stephentoub

src/Common/Polyfills/System/Diagnostics/ProcessExtensions.cs

@@ -137,7 +137,7 @@ private async Task CloseAsync()
         }
         finally
         {
-            SetConnected(false);
+            SetDisconnected();
         }
     }
 
@@ -203,7 +203,7 @@ private async Task ReceiveMessagesAsync(CancellationToken cancellationToken)
         }
         finally
         {
-            SetConnected(false);
+            SetDisconnected();
         }
     }
 
@@ -251,7 +251,7 @@ private void HandleEndpointEvent(string data)
         _messageEndpoint = new Uri(_sseEndpoint, data);
 
         // Set connected state
-        SetConnected(true);
+        SetConnected();
         _connectionEstablished.TrySetResult(true);
     }
 

src/ModelContextProtocol/Protocol/Transport/SseClientSessionTransport.cs

@@ -9,54 +9,51 @@ internal sealed class StdioClientSessionTransport : StreamClientSessionTransport
 {
     private readonly StdioClientTransportOptions _options;
     private readonly Process _process;
+    private readonly Queue<string> _stderrRollingLog;
+    private int _cleanedUp = 0;
 
-    public StdioClientSessionTransport(StdioClientTransportOptions options, Process process, string endpointName, ILoggerFactory? loggerFactory)
+    public StdioClientSessionTransport(StdioClientTransportOptions options, Process process, string endpointName, Queue<string> stderrRollingLog, ILoggerFactory? loggerFactory)
         : base(process.StandardInput, process.StandardOutput, endpointName, loggerFactory)
     {
         _process = process;
         _options = options;
+        _stderrRollingLog = stderrRollingLog;
     }
 
     /// <inheritdoc/>
-    /// <remarks>
-    /// <para>
-    /// For stdio-based transports, this implementation first verifies that the underlying process 
-    /// is still running before attempting to send the message. If the process has exited or cannot
-    /// be accessed, a <see cref="InvalidOperationException"/> is thrown with details about the failure.
-    /// </para>
-    /// <para>
-    /// After verifying the process state, this method delegates to the base class implementation
-    /// to handle the actual message serialization and transmission to the process's standard input stream.
-    /// </para>
-    /// </remarks>
-    /// <exception cref="InvalidOperationException">
-    /// Thrown when the underlying process has exited or cannot be accessed.
-    /// </exception>
     public override async Task SendMessageAsync(JsonRpcMessage message, CancellationToken cancellationToken = default)
     {
-        Exception? processException = null;
-        bool hasExited = false;
         try
         {
-            hasExited = _process.HasExited;
+            await base.SendMessageAsync(message, cancellationToken);
         }
-        catch (Exception e)
+        catch (IOException)
         {
-            processException = e;
-            hasExited = true;
-        }
+            // We failed to send due to an I/O error. If the server process has exited, which is then very likely the cause
+            // for the I/O error, we should throw an exception for that instead.
+            if (await GetUnexpectedExitExceptionAsync(cancellationToken).ConfigureAwait(false) is Exception processExitException)
+            {
+                throw processExitException;
+            }
 
-        if (hasExited)
-        {
-            throw new InvalidOperationException("Transport is not connected", processException);
+            throw;
         }
-
-        await base.SendMessageAsync(message, cancellationToken).ConfigureAwait(false);
     }
 
     /// <inheritdoc/>
-    protected override ValueTask CleanupAsync(CancellationToken cancellationToken)
+    protected override async ValueTask CleanupAsync(Exception? error = null, CancellationToken cancellationToken = default)
     {
+        // Only clean up once.
+        if (Interlocked.Exchange(ref _cleanedUp, 1) != 0)
+        {
+            return;
+        }
+
+        // We've not yet forcefully terminated the server. If it's already shut down, something went wrong,
+        // so create an exception with details about that.
+        error ??= await GetUnexpectedExitExceptionAsync(cancellationToken).ConfigureAwait(false);
+
+        // Now terminate the server process.
         try
         {
             StdioClientTransport.DisposeProcess(_process, processRunning: true, _options.ShutdownTimeout, Name);
@@ -66,6 +63,49 @@ protected override ValueTask CleanupAsync(CancellationToken cancellationToken)
             LogTransportShutdownFailed(Name, ex);
         }
 
-        return base.CleanupAsync(cancellationToken);
+        // And handle cleanup in the base type.
+        await base.CleanupAsync(error, cancellationToken);
+    }
+
+    private async ValueTask<Exception?> GetUnexpectedExitExceptionAsync(CancellationToken cancellationToken)
+    {
+        if (!StdioClientTransport.HasExited(_process))
+        {
+            return null;
+        }
+
+        Debug.Assert(StdioClientTransport.HasExited(_process));
+        try
+        {
+            // The process has exited, but we still need to ensure stderr has been flushed.
+#if NET
+            await _process.WaitForExitAsync(cancellationToken).ConfigureAwait(false);
+#else
+            _process.WaitForExit();
+#endif
+        }
+        catch { }
+
+        string errorMessage = "MCP server process exited unexpectedly";
+
+        string? exitCode = null;
+        try
+        {
+            exitCode = $" (exit code: {(uint)_process.ExitCode})";
+        }
+        catch { }
+
+        lock (_stderrRollingLog)
+        {
+            if (_stderrRollingLog.Count > 0)
+            {
+                errorMessage =
+                    $"{errorMessage}{exitCode}{Environment.NewLine}" +
+                    $"Server's stderr tail:{Environment.NewLine}" +
+                    $"{string.Join(Environment.NewLine, _stderrRollingLog)}";
+            }
+        }
+
+        return new IOException(errorMessage);
     }
 }

src/ModelContextProtocol/Protocol/Transport/StdioClientSessionTransport.cs

@@ -127,8 +127,28 @@ public async Task<ITransport> ConnectAsync(CancellationToken cancellationToken =
 
             process = new() { StartInfo = startInfo };
 
-            // Set up error logging
-            process.ErrorDataReceived += (sender, args) => LogReadStderr(logger, endpointName, args.Data ?? "(no data)");
+            // Set up stderr handling. Log all stderr output, and keep the last
+            // few lines in a rolling log for use in exceptions.
+            const int MaxStderrLength = 10; // keep the last 10 lines of stderr
+            Queue<string> stderrRollingLog = new(MaxStderrLength);
+            process.ErrorDataReceived += (sender, args) =>
+            {
+                string? data = args.Data;
+                if (data is not null)
+                {
+                    lock (stderrRollingLog)
+                    {
+                        if (stderrRollingLog.Count >= MaxStderrLength)
+                        {
+                            stderrRollingLog.Dequeue();
+                        }
+
+                        stderrRollingLog.Enqueue(data);
+                    }
+
+                    LogReadStderr(logger, endpointName, data);
+                }
+            };
 
             // We need both stdin and stdout to use a no-BOM UTF-8 encoding. On .NET Core,
             // we can use ProcessStartInfo.StandardOutputEncoding/StandardInputEncoding, but
@@ -154,14 +174,14 @@ public async Task<ITransport> ConnectAsync(CancellationToken cancellationToken =
             if (!processStarted)
             {
                 LogTransportProcessStartFailed(logger, endpointName);
-                throw new InvalidOperationException("Failed to start MCP server process");
+                throw new IOException("Failed to start MCP server process.");
             }
 
             LogTransportProcessStarted(logger, endpointName, process.Id);
 
             process.BeginErrorReadLine();
 
-            return new StdioClientSessionTransport(_options, process, endpointName, _loggerFactory);
+            return new StdioClientSessionTransport(_options, process, endpointName, stderrRollingLog, _loggerFactory);
         }
         catch (Exception ex)
         {
@@ -176,7 +196,7 @@ public async Task<ITransport> ConnectAsync(CancellationToken cancellationToken =
                 LogTransportShutdownFailed(logger, endpointName, ex2);
             }
 
-            throw new InvalidOperationException("Failed to connect transport", ex);
+            throw new IOException("Failed to connect transport.", ex);
         }
     }
 
@@ -185,20 +205,9 @@ internal static void DisposeProcess(
     {
         if (process is not null)
         {
-            if (processRunning)
-            {
-                try
-                {
-                    processRunning = !process.HasExited;
-                }
-                catch
-                {
-                    processRunning = false;
-                }
-            }
-
             try
             {
+                processRunning = processRunning && !HasExited(process);
                 if (processRunning)
                 {
                     // Wait for the process to exit.
@@ -214,6 +223,19 @@ internal static void DisposeProcess(
         }
     }
 
+    /// <summary>Gets whether <paramref name="process"/> has exited.</summary>
+    internal static bool HasExited(Process process)
+    {
+        try
+        {
+            return process.HasExited;
+        }
+        catch
+        {
+            return true;
+        }
+    }
+
     [LoggerMessage(Level = LogLevel.Information, Message = "{EndpointName} connecting.")]
     private static partial void LogTransportConnecting(ILogger logger, string endpointName);
 

src/ModelContextProtocol/Protocol/Transport/StdioClientTransport.cs

@@ -53,17 +53,12 @@ public StreamClientSessionTransport(
         _readTask = readTask.Unwrap();
         readTask.Start();
 
-        SetConnected(true);
+        SetConnected();
     }
 
     /// <inheritdoc/>
     public override async Task SendMessageAsync(JsonRpcMessage message, CancellationToken cancellationToken = default)
     {
-        if (!IsConnected)
-        {
-            throw new InvalidOperationException("Transport is not connected");
-        }
-
         string id = "(no id)";
         if (message is JsonRpcMessageWithId messageWithId)
         {
@@ -82,31 +77,22 @@ public override async Task SendMessageAsync(JsonRpcMessage message, Cancellation
         catch (Exception ex)
         {
             LogTransportSendFailed(Name, id, ex);
-            throw new InvalidOperationException("Failed to send message", ex);
+            throw new IOException("Failed to send message.", ex);
         }
     }
 
     /// <inheritdoc/>
-    /// <summary>
-    /// Asynchronously releases all resources used by the stream client session transport.
-    /// </summary>
-    /// <returns>A task that represents the asynchronous dispose operation.</returns>
-    /// <remarks>
-    /// This method cancels ongoing operations and waits for the read task to complete
-    /// before marking the transport as disconnected. It calls <see cref="CleanupAsync"/> 
-    /// to perform the actual cleanup work.
-    /// After disposal, the transport can no longer be used to send or receive messages.
-    /// </remarks>
-    public override ValueTask DisposeAsync() =>
-        CleanupAsync(CancellationToken.None);
+    public override ValueTask DisposeAsync() => 
+        CleanupAsync(cancellationToken: CancellationToken.None);
 
     private async Task ReadMessagesAsync(CancellationToken cancellationToken)
     {
+        Exception? error = null;
         try
         {
             LogTransportEnteringReadMessagesLoop(Name);
 
-            while (!cancellationToken.IsCancellationRequested)
+            while (true)
             {
                 if (await _serverOutput.ReadLineAsync(cancellationToken).ConfigureAwait(false) is not string line)
                 {
@@ -130,12 +116,13 @@ private async Task ReadMessagesAsync(CancellationToken cancellationToken)
         }
         catch (Exception ex)
         {
+            error = ex;
             LogTransportReadMessagesFailed(Name, ex);
         }
         finally
         {
             _readTask = null;
-            await CleanupAsync(cancellationToken).ConfigureAwait(false);
+            await CleanupAsync(error, cancellationToken).ConfigureAwait(false);
         }
     }
 
@@ -166,7 +153,7 @@ private async Task ProcessMessageAsync(string line, CancellationToken cancellati
         }
     }
 
-    protected virtual async ValueTask CleanupAsync(CancellationToken cancellationToken)
+    protected virtual async ValueTask CleanupAsync(Exception? error = null, CancellationToken cancellationToken = default)
     {
         LogTransportShuttingDown(Name);
 
@@ -191,7 +178,7 @@ protected virtual async ValueTask CleanupAsync(CancellationToken cancellationTok
             }
         }
 
-        SetConnected(false);
+        SetDisconnected(error);
         LogTransportShutDown(Name);
     }
 }