Skip to content

Conversation

Copy link
Contributor

Copilot AI commented Nov 20, 2025

Fix flaky ConnectAndPing_Stdio test on .NET Framework 4.7.2

This PR fixes a race condition in StdioClientTransport that causes test flakiness on .NET Framework 4.7.2.

Problem

The ConnectAndPing_Stdio(clientId: "everything") test was failing intermittently on .NET Framework 4.7.2 with a 60-second timeout during client initialization:

System.TimeoutException : Initialization timed out
---- System.OperationCanceledException : The operation was canceled.

The test passed consistently on .NET 8.0, 9.0, and 10.0, but failed only on .NET Framework 4.7.2.

Root Cause

On .NET Framework, StdioClientTransport.ConnectAsync temporarily modifies the global Console.InputEncoding to ensure UTF-8 encoding for stdin. This is necessary because .NET Framework's ProcessStartInfo doesn't have a StandardInputEncoding property.

When multiple tests run in parallel (which they do by default in xUnit), they create a race condition:

Time  | Test A                          | Test B
------|--------------------------------|--------------------------------
T1    | Save encoding (Default)        |
T2    | Set to UTF-8                   |
T3    |                                | Save encoding (now UTF-8!)
T4    |                                | Set to UTF-8
T5    | Start process (UTF-8 ✓)        |
T6    |                                | Start process (UTF-8 ✓)
T7    | Restore to Default ✓           |
T8    |                                | Restore to UTF-8 ✗

At T8, Test B restores Console.InputEncoding to UTF-8 instead of the original Default encoding. This leaves the console in an incorrect state. Subsequent process starts may get the wrong encoding, causing the spawned npx process to receive corrupted input, leading to communication failures and timeout errors.

Solution

Added a static lock object (s_consoleEncodingLock) that serializes access to the Console.InputEncoding modification on .NET Framework. This ensures atomic execution of:

  1. Read the original encoding
  2. Set it to UTF-8
  3. Start the process
  4. Restore the original encoding

The lock is only compiled for .NET Framework builds (using #if !NET) since modern .NET versions have ProcessStartInfo.StandardInputEncoding and don't need this workaround.

Changes

  • Added: s_consoleEncodingLock static field (lines 28-32) - only on .NET Framework
  • Wrapped: Console encoding modification in a lock statement (lines 170-182) - only on .NET Framework
  • Added: Comment explaining the synchronization requirement inside the #else block (lines 168-169)

Testing

  • Diagnosed root cause - race condition in global Console.InputEncoding modification
  • Implemented fix - added thread synchronization for .NET Framework
  • Build passes - no compilation errors or warnings
  • Tests pass on .NET 8.0 - all tests pass including ConnectAndPing_Stdio
  • CodeQL security scan passes - no vulnerabilities detected
  • Addressed review feedback - moved IMPORTANT comment into else block
  • CI validation on .NET Framework 4.7.2 - pending

Impact

This is a minimal, surgical fix that:

  • ✅ Only affects .NET Framework builds (zero impact on modern .NET)
  • ✅ Only adds synchronization to a single critical section (minimal performance impact)
  • ✅ Maintains full backward compatibility
  • ✅ Fixes the flaky test without changing test logic
  • ✅ No new dependencies introduced
  • ✅ No breaking changes
Original prompt

A test failed in CI. Please diagnose the flakiness and submit a PR to fix it.

failed ModelContextProtocol.Tests.ClientIntegrationTests.ConnectAndPing_Stdio(clientId: "everything") (1m 00s 092ms)
System.TimeoutException : Initialization timed out
---- System.OperationCanceledException : The operation was canceled.
from D:\a\csharp-sdk\csharp-sdk\artifacts\bin\ModelContextProtocol.Tests\Debug\net472\ModelContextProtocol.Tests.exe (net48|x64)
Xunit.MicrosoftTestingPlatform.XunitException: System.TimeoutException : Initialization timed out
---- System.OperationCanceledException : The operation was canceled.
at ModelContextProtocol.Client.McpClientImpl.d__27.MoveNext() in //src/ModelContextProtocol.Core/Client/McpClientImpl.cs:197
--- End of stack trace from previous location where exception was thrown ---
at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
at ModelContextProtocol.Client.McpClientImpl.d__27.MoveNext() in /
/src/ModelContextProtocol.Core/Client/McpClientImpl.cs:204
--- End of stack trace from previous location where exception was thrown ---
at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
at System.Runtime.CompilerServices.ConfiguredTaskAwaitable.ConfiguredTaskAwaiter.GetResult()
at ModelContextProtocol.Client.McpClient.d__6.MoveNext() in //src/ModelContextProtocol.Core/Client/McpClient.Methods.cs:42
--- End of stack trace from previous location where exception was thrown ---
at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
at ModelContextProtocol.Client.McpClient.d__6.MoveNext() in /
/src/ModelContextProtocol.Core/Client/McpClient.Methods.cs:47
--- End of stack trace from previous location where exception was thrown ---
at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
at System.Runtime.CompilerServices.TaskAwaiter1.GetResult() at ModelContextProtocol.Tests.ClientIntegrationTests.<ConnectAndPing_Stdio>d__6.MoveNext() in /_/tests/ModelContextProtocol.Tests/ClientIntegrationTests.cs:36 --- End of stack trace from previous location where exception was thrown --- at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw() at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task) --- End of stack trace from previous location where exception was thrown --- at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw() at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task) ----- Inner Stack Trace ----- at System.Threading.CancellationToken.ThrowOperationCanceledException() at System.Threading.CancellationToken.ThrowIfCancellationRequested() at System.Threading.Tasks.TaskExtensions.<WaitAsync>d__3.MoveNext() in /_/src/Common/Polyfills/System/Threading/Tasks/TaskExtensions.cs:46 --- End of stack trace from previous location where exception was thrown --- at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw() at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task) at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task) at System.Runtime.CompilerServices.ConfiguredTaskAwaitable.ConfiguredTaskAwaiter.GetResult() at System.Threading.Tasks.TaskExtensions.<WaitAsync>d__21.MoveNext() in //src/Common/Polyfills/System/Threading/Tasks/TaskExtensions.cs:20
--- End of stack trace from previous location where exception was thrown ---
at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
at System.Runtime.CompilerServices.ConfiguredTaskAwaitable1.ConfiguredTaskAwaiter.GetResult() at ModelContextProtocol.McpSessionHandler.<SendRequestAsync>d__33.MoveNext() in /_/src/ModelContextProtocol.Core/McpSessionHandler.cs:449 --- End of stack trace from previous location where exception was thrown --- at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw() at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task) at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task) at System.Runtime.CompilerServices.ConfiguredTaskAwaitable1.ConfiguredTaskAwaiter.GetResult()
at ModelContextProtocol.McpSession.d__9`2.MoveNext() in /
/src/ModelContextProtocol.Core/McpSession.Methods.cs:71
--- End of stack trace from previous location where exception was thrown ---
at System.Runtime.ExceptionServices.E...


💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.

Add thread synchronization around Console.InputEncoding modification to prevent race conditions when multiple transports are created concurrently on .NET Framework.

Co-authored-by: stephentoub <2642209+stephentoub@users.noreply.github.com>
Copilot AI changed the title [WIP] Diagnose and fix flakiness in ConnectAndPing_Stdio test Fix race condition in StdioClientTransport on .NET Framework Nov 20, 2025
Copilot AI requested a review from stephentoub November 20, 2025 14:46
Copilot finished work on behalf of stephentoub November 20, 2025 14:46
@stephentoub stephentoub marked this pull request as ready for review November 20, 2025 14:49
Co-authored-by: stephentoub <2642209+stephentoub@users.noreply.github.com>
Copilot finished work on behalf of stephentoub November 20, 2025 14:58
Copilot AI requested a review from stephentoub November 20, 2025 14:58
@stephentoub stephentoub enabled auto-merge (squash) November 21, 2025 20:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants