Merge branch 'main' into copilot/fix-flaky-diagnostic-test

Author: ericstj

.github/copilot-instructions.md

@@ -85,22 +85,39 @@ The SDK consists of three main packages:
 - Test servers in `tests/ModelContextProtocol.Test*Server/` for integration scenarios
 - Filter manual tests with `[Trait("Execution", "Manual")]` - these require external dependencies
 
-### Test Infrastructure and Helpers
-- **LoggedTest**: Base class for tests that need logging output captured to xUnit test output
-  - Provides `ILoggerFactory` and `ITestOutputHelper` for test logging
-  - Use when debugging or when tests need to verify log output
-- **TestServerTransport**: In-memory transport for testing client-server interactions without network I/O
-- **MockLoggerProvider**: For capturing and asserting on log messages
-- **XunitLoggerProvider**: Routes `ILogger` output to xUnit's `ITestOutputHelper`
-- **KestrelInMemoryTransport** (AspNetCore.Tests): In-memory Kestrel connection for HTTP transport testing without network stack
-
-### Test Best Practices
-- Inherit from `LoggedTest` for tests needing logging infrastructure
-- Use `TestServerTransport` for in-memory client-server testing
-- Mock external dependencies (filesystem, HTTP clients) rather than calling real services
-- Use `CancellationTokenSource` with timeouts to prevent hanging tests
-- Dispose resources properly (servers, clients, transports) using `IDisposable` or `await using`
-- Run tests with: `dotnet test --filter '(Execution!=Manual)'`
+### Test Base Classes
+- **`LoggedTest`**: Base class that wires up `ILoggerFactory` with `XunitLoggerProvider` (test output) and `MockLoggerProvider` (log assertions). Inherit from this for any test needing logging.
+- **`ClientServerTestBase`**: Sets up in-memory client/server pair via `Pipe`. Override `ConfigureServices` to register tools/prompts/resources, then call `CreateMcpClientForServer()`. Handles async disposal automatically.
+- **`KestrelInMemoryTest`** (AspNetCore tests): Hosts ASP.NET Core with in-memory transport — no ports needed.
+- **`TestServerTransport`**: In-memory mock transport for testing client logic without a real server.
+
+### Transport Selection in Tests
+- **Never use `WithStdioServerTransport()` in unit tests.** It reads from the test host's stdin, which cannot be closed, permanently leaking a thread pool thread per test.
+- For DI-only tests: `WithStreamServerTransport(Stream.Null, Stream.Null)`
+- For client/server interaction: inherit `ClientServerTestBase`
+- For client-only logic: use `TestServerTransport`
+- For HTTP/SSE: inherit `KestrelInMemoryTest`
+- For process lifecycle tests: `StdioClientTransport` (only when testing actual process behavior)
+
+### Resource Management
+- **Always `await using` the `ServiceProvider`** when MCP server services are registered — `McpServerImpl` only implements `IAsyncDisposable`. Synchronous `using` throws at runtime.
+- **Always dispose clients and servers** — use `await using var client = ...`
+- **Use `TestContext.Current.CancellationToken`** for async MCP calls so xUnit can cancel on timeout.
+
+### Timeouts
+- **Always use `TestConstants.DefaultTimeout`** (60s) instead of hardcoded values. CI machines are slower than dev workstations.
+- For HTTP polling operations use `TestConstants.HttpClientPollingTimeout` (2s).
+
+### Synchronization
+- **Never use `Task.Delay` for synchronization.** Use `TaskCompletionSource`, `SemaphoreSlim`, or `Channel` so tests don't depend on timing.
+
+### Background Logging
+- `ITestOutputHelper.WriteLine` throws after the test method returns. Background threads (process event handlers, async continuations) can outlive the test, causing unhandled exceptions that crash the test host.
+- Route logging through `LoggedTest.LoggerFactory` — `XunitLoggerProvider` already catches post-test exceptions.
+- If calling `ITestOutputHelper` directly from an event handler, wrap in try/catch for `InvalidOperationException`.
+
+### Parallelism
+- Tests run in parallel by default. Apply `[Collection(nameof(DisableParallelization))]` to test classes that touch global state (e.g., `ActivitySource` listeners).
 
 ## Build and Development
 

.github/workflows/codeql.yml

@@ -47,7 +47,7 @@ jobs:
         # your codebase is analyzed, see https://docs.github.com/en/code-security/code-scanning/creating-an-advanced-setup-for-code-scanning/codeql-code-scanning-for-compiled-languages
     steps:
     - name: Checkout repository
-      uses: actions/checkout@v4
+      uses: actions/checkout@v6
 
     # Add any setup steps before running the `github/codeql-action/init` action.
     # This includes steps like installing compilers or runtimes (`actions/setup-node`

.github/workflows/release.yml

@@ -32,6 +32,8 @@ on:
 
 jobs:
   build-all-configs:
+    # Don't run scheduled/release triggers on forks; allow manual workflow_dispatch
+    if: ${{ github.repository == 'modelcontextprotocol/csharp-sdk' || github.event_name == 'workflow_dispatch' }}
     strategy:
       matrix:
         os: [ubuntu-latest, windows-latest, macos-latest]

CONTRIBUTING.md

@@ -65,6 +65,70 @@ dotnet test tests/ModelContextProtocol.Tests/
 
 Tools like Visual Studio, JetBrains Rider, and VS Code also provide integrated test runners that can be used to run and debug individual tests.
 
+### Writing Tests
+
+The test projects include shared infrastructure in `tests/Common/Utils/` that most tests build on. Familiarize yourself with these helpers before writing new tests.
+
+#### Test base classes
+
+- **`LoggedTest`** — Base class that wires up `ILoggerFactory` with both `XunitLoggerProvider` (routes to test output) and `MockLoggerProvider` (captures logs for assertions). Inherit from this for any test that needs logging.
+- **`ClientServerTestBase`** — Sets up an in-memory client/server pair connected via `Pipe` with proper async disposal. Override `ConfigureServices` to register tools, prompts, and resources, then call `CreateMcpClientForServer()` to get a connected client.
+- **`KestrelInMemoryTest`** (ASP.NET Core tests) — Hosts an ASP.NET Core server with in-memory transport so HTTP/SSE tests run without allocating ports.
+
+#### Choosing a transport
+
+| Scenario | Transport | Why |
+|---|---|---|
+| Unit tests that only need DI | `WithStreamServerTransport(Stream.Null, Stream.Null)` | No threads blocked, no process spawned |
+| Client/server interaction tests | `ClientServerTestBase` (uses `Pipe`) | Full bidirectional MCP, in-process |
+| Client-only logic | `TestServerTransport` | In-memory mock that auto-responds to standard MCP requests |
+| HTTP/SSE integration | `KestrelInMemoryTest` | Real HTTP stack, no network |
+| External process tests | `StdioClientTransport` | Only when testing actual process lifecycle |
+
+> **Do not** use `WithStdioServerTransport()` in unit tests. The stdio server transport reads from the test host process's standard input, which the test does not own and cannot close. This means the transport's background read loop can never terminate, permanently leaking a thread pool thread per test. Use `WithStreamServerTransport(Stream.Null, Stream.Null)` for tests that only need the DI container.
+
+#### Resource management
+
+- **Always `await using` the `ServiceProvider`** when MCP server services are registered — `McpServerImpl` only implements `IAsyncDisposable`, not `IDisposable`. A synchronous `using` will throw at runtime, and skipping disposal leaks transports and background threads.
+- **Use `TestContext.Current.CancellationToken`** when calling async MCP methods so that xUnit can cancel the test on timeout rather than hanging.
+- **Dispose clients and servers** explicitly. Prefer `await using var client = ...` over relying on finalizers. `ClientServerTestBase` handles this if you inherit from it.
+
+#### Timeouts
+
+Use `TestConstants.DefaultTimeout` (60 seconds) rather than hardcoded values. CI machines are often slower than developer workstations, and short timeouts cause flaky failures.
+
+```csharp
+// Good
+using var cts = CancellationTokenSource.CreateLinkedTokenSource(TestContext.Current.CancellationToken);
+cts.CancelAfter(TestConstants.DefaultTimeout);
+await client.CallToolAsync("my-tool", args, cts.Token);
+
+// Bad — too short for CI
+cts.CancelAfter(TimeSpan.FromSeconds(5));
+```
+
+#### Synchronization
+
+Avoid `Task.Delay` for synchronization. Use explicit signaling primitives (`TaskCompletionSource`, `SemaphoreSlim`, `Channel`) so tests don't depend on timing. If a producer/consumer test writes events for a streaming reader, use a `TaskCompletionSource` to confirm the reader is active before writing.
+
+#### Background logging
+
+`ITestOutputHelper.WriteLine` throws after the test method returns. Background threads (process event handlers, async continuations) can outlive the test. This manifests as unhandled exceptions that crash the test host. Two mitigations:
+
+1. **`XunitLoggerProvider`** already catches these exceptions. Route logging through `LoggedTest.LoggerFactory` rather than calling `ITestOutputHelper` directly from callbacks.
+2. **If you must call `ITestOutputHelper` from an event handler**, wrap it in a try/catch:
+   ```csharp
+   process.ErrorDataReceived += (s, e) =>
+   {
+       try { testOutputHelper.WriteLine(e.Data); }
+       catch (InvalidOperationException) { }
+   };
+   ```
+
+#### Parallelism
+
+Tests run in parallel by default. If a test class touches global state (e.g., `ActivitySource` listeners in diagnostics tests), apply `[Collection(nameof(DisableParallelization))]` to run it sequentially.
+
 ### Building the Documentation
 
 This project uses [DocFX](https://dotnet.github.io/docfx/) to generate its conceptual and reference documentation.

docs/concepts/index.md

@@ -17,7 +17,7 @@ Install the SDK and build your first MCP client and server.
 | [Ping](ping/ping.md) | Learn how to verify connection health using the ping mechanism. |
 | [Progress tracking](progress/progress.md) | Learn how to track progress for long-running operations through notification messages. |
 | [Cancellation](cancellation/cancellation.md) | Learn how to cancel in-flight MCP requests using cancellation tokens and notifications. |
-| [Pagination](pagination/pagination.md) | Learn how to use cursor-based pagination when listing tools, prompts, and resources. |
+| [Tasks](tasks/tasks.md) | Learn how to use task-based execution for long-running operations that can be polled for status and results. |
 
 ### Client Features
 
@@ -36,6 +36,7 @@ Install the SDK and build your first MCP client and server.
 | [Prompts](prompts/prompts.md) | Learn how to implement and consume reusable prompt templates with rich content types. |
 | [Completions](completions/completions.md) | Learn how to implement argument auto-completion for prompts and resource templates. |
 | [Logging](logging/logging.md) | Learn how to implement logging in MCP servers and how clients can consume log messages. |
+| [Pagination](pagination/pagination.md) | Learn how to use cursor-based pagination when listing tools, prompts, and resources. |
 | [Stateless and Stateful](stateless/stateless.md) | Learn when to use stateless vs. stateful mode for HTTP servers and how to configure sessions. |
 | [HTTP Context](httpcontext/httpcontext.md) | Learn how to access the underlying `HttpContext` for a request. |
 | [MCP Server Handler Filters](filters.md) | Learn how to add filters to the handler pipeline. Filters let you wrap the original handler with additional functionality. |

docs/concepts/toc.yml

@@ -17,8 +17,6 @@ items:
     uid: progress
   - name: Cancellation
     uid: cancellation
-  - name: Pagination
-    uid: pagination
   - name: Tasks
     uid: tasks
 - name: Client Features
@@ -41,6 +39,8 @@ items:
     uid: completions
   - name: Logging
     uid: logging
+  - name: Pagination
+    uid: pagination
   - name: HTTP Context
     uid: httpcontext
   - name: Filters

src/ModelContextProtocol.Core/Client/StdioClientSessionTransport.cs

@@ -10,15 +10,17 @@ internal sealed class StdioClientSessionTransport : StreamClientSessionTransport
     private readonly StdioClientTransportOptions _options;
     private readonly Process _process;
     private readonly Queue<string> _stderrRollingLog;
+    private readonly DataReceivedEventHandler _errorHandler;
     private int _cleanedUp = 0;
     private readonly int? _processId;
 
-    public StdioClientSessionTransport(StdioClientTransportOptions options, Process process, string endpointName, Queue<string> stderrRollingLog, ILoggerFactory? loggerFactory) :
+    public StdioClientSessionTransport(StdioClientTransportOptions options, Process process, string endpointName, Queue<string> stderrRollingLog, DataReceivedEventHandler errorHandler, ILoggerFactory? loggerFactory) :
         base(process.StandardInput.BaseStream, process.StandardOutput.BaseStream, encoding: null, endpointName, loggerFactory)
     {
         _options = options;
         _process = process;
         _stderrRollingLog = stderrRollingLog;
+        _errorHandler = errorHandler;
         try { _processId = process.Id; } catch { }
     }
 
@@ -33,7 +35,7 @@ public override async Task SendMessageAsync(JsonRpcMessage message, Cancellation
         {
             // 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)
+            if (await GetUnexpectedExitExceptionAsync().ConfigureAwait(false) is Exception processExitException)
             {
                 throw processExitException;
             }
@@ -45,15 +47,32 @@ public override async Task SendMessageAsync(JsonRpcMessage message, Cancellation
     /// <inheritdoc/>
     protected override async ValueTask CleanupAsync(Exception? error = null, CancellationToken cancellationToken = default)
     {
-        // Only clean up once.
+        // Only run the full stdio cleanup once (handler detach, process kill, etc.).
+        // If another call is already handling cleanup, cancel the shutdown token
+        // to unblock it (e.g. if it's stuck in WaitForExitAsync) and let it
+        // call SetDisconnected with full StdioClientCompletionDetails.
         if (Interlocked.Exchange(ref _cleanedUp, 1) != 0)
         {
+            CancelShutdown();
             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);
+        error ??= await GetUnexpectedExitExceptionAsync().ConfigureAwait(false);
+
+        // Ensure all pending ErrorDataReceived events are drained before detaching
+        // the handler. GetUnexpectedExitExceptionAsync does this when HasExited is
+        // true, but there is a narrow window on Linux where the process has closed
+        // stdout (causing EOF in ReadMessagesAsync) yet hasn't been fully reaped,
+        // so HasExited returns false and the drain is skipped. An unconditional
+        // wait here covers that gap. When the drain already happened above, the
+        // call returns immediately.
+        await WaitForProcessExitAsync().ConfigureAwait(false);
+
+        // Detach the stderr handler so no further ErrorDataReceived events
+        // are dispatched during or after process disposal.
+        _process.ErrorDataReceived -= _errorHandler;
 
         // Terminate the server process (or confirm it already exited), then build
         // and publish strongly-typed completion details while the process handle
@@ -78,26 +97,16 @@ protected override async ValueTask CleanupAsync(Exception? error = null, Cancell
         await base.CleanupAsync(error, cancellationToken).ConfigureAwait(false);
     }
 
-    private async ValueTask<Exception?> GetUnexpectedExitExceptionAsync(CancellationToken cancellationToken)
+    private async ValueTask<Exception?> GetUnexpectedExitExceptionAsync()
     {
         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.
-            // WaitForExitAsync only waits for exit; it does not guarantee that all
-            // ErrorDataReceived events have been dispatched. The synchronous WaitForExit()
-            // (no arguments) does ensure that, so call it after WaitForExitAsync completes.
-#if NET
-            await _process.WaitForExitAsync(cancellationToken).ConfigureAwait(false);
-#endif
-            _process.WaitForExit();
-        }
-        catch { }
+
+        await WaitForProcessExitAsync().ConfigureAwait(false);
 
         string errorMessage = "MCP server process exited unexpectedly";
 
@@ -122,6 +131,35 @@ protected override async ValueTask CleanupAsync(Exception? error = null, Cancell
         return new IOException(errorMessage);
     }
 
+    /// <summary>
+    /// Waits for the process to exit within <see cref="StdioClientTransportOptions.ShutdownTimeout"/>
+    /// and flushes pending <see cref="Process.ErrorDataReceived"/> events.
+    /// </summary>
+    /// <remarks>
+    /// On .NET, <c>Process.WaitForExitAsync</c> also waits for asynchronous output readers
+    /// to complete, ensuring all events have been dispatched. On .NET Framework,
+    /// <see cref="Process.WaitForExit(int)"/> does not guarantee this; the parameterless
+    /// <see cref="Process.WaitForExit()"/> overload is needed to flush the event queue.
+    /// This method is idempotent—calling it after the process has already been waited on
+    /// returns immediately.
+    /// </remarks>
+    private async ValueTask WaitForProcessExitAsync()
+    {
+        try
+        {
+#if NET
+            using var timeoutCts = new CancellationTokenSource(_options.ShutdownTimeout);
+            await _process.WaitForExitAsync(timeoutCts.Token).ConfigureAwait(false);
+#else
+            if (_process.WaitForExit((int)_options.ShutdownTimeout.TotalMilliseconds))
+            {
+                _process.WaitForExit();
+            }
+#endif
+        }
+        catch { }
+    }
+
     private StdioClientCompletionDetails BuildCompletionDetails(Exception? error)
     {
         StdioClientCompletionDetails details = new()