Repository navigation
Conversation
When a stdio server exits right after it starts, the read loop sees EOF and CleanupAsync closes the server's stdin while the client's first send may still be writing to it. On Unix, disposing the pipe while that write is creating its socket in SafePipeHandle.CreatePipeSocket hangs the write forever in SafeSocketHandle.CloseAsIs. Take the send lock, for at most ShutdownTimeout, before closing stdin. A send still holding it after that leaves stdin open and the server to the process-tree kill. A send that failed with an IOException checked for the server's exit before a server that had just exited might have been reaped, so it threw a bare "Failed to send message." instead of the exit code and stderr. Wait for the process to exit, for at most ShutdownTimeout, before building the exit exception.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #2005
Problem
When a stdio server exits right after it starts, the client's first send races the transport's own cleanup. On Unix, two things can go wrong. Both were reproduced on Linux; macOS uses the same Unix pipe code.
1. The send never returns
When the server exits, the read loop sees EOF, and
CleanupAsynccloses the server's stdin (#1838). If the first send is still inPipeStream.WriteAsyncat that moment, the write is creating the pipe's socket inSafePipeHandle.CreatePipeSocket. It finds the pipe disposed, so it disposes the new socket. But it still holds a reference on the pipe handle, andSafeSocketHandle.CloseAsIswaits for a release that only that handle'sReleaseHandleperforms. The thread waits on itself:That part looks like a runtime bug. The transport can avoid it anyway, by never closing stdin while a send is writing to it.
2. The send fails with a bare
IOException: Failed to send message.SendMessageAsynccatches theIOExceptionand callsGetUnexpectedExitExceptionAsync. That returnsnullunlessProcess.HasExitedis already true. A server that has just exited has closed its pipes but may not have been reaped yet. So the caller gets the bare send failure instead of "MCP server process exited unexpectedly", with the exit code and stderr.Repro
16 workers each connected over and over to a server that writes to stderr and exits at once (
sh -c 'echo … >&2; exit 3'). On 2.3.0, all 16 workers hung within 30,000 connects, in each of two runs. Those runs also had 152 and 1,844 connects that ended in the bare send failure. With this change on 2.3.0, 1,136,147 connects ran in 3 minutes, with no worker hung and no bare send failure.StdioClientSessionTransport.csis unchanged from v2.3.0 to v2.4.0, and onmain.Fix
CleanupAsyncnow takes the send lock, waiting at mostShutdownTimeout, before it closes stdin. So stdin is never disposed under a write. If a send still holds the lock after that, stdin is left open, and the server is left to the existing process-tree kill._sendLockinStreamClientSessionTransportbecomesprivate protectedfor this.IOException,SendMessageAsyncnow waits for the process to exit, at mostShutdownTimeout, before it builds the exit exception.Tests
Added to
StdioClientTransportTests. Both tests run on Unix only, with the existing stderr-callback guard, because both races are in the Unix pipe and process handling.SendMessageAsync_ServerExited_ThrowsWithExitDetails: the server closes its stdin, writes to stderr, and exits a second later. That holds open the window between the failed send and the observed exit. The send must throw the "exited unexpectedly" exception with the server's stderr, and the transport must complete with exit code 3. The test checks the exit code onStdioClientCompletionDetailsrather than in the message. The message can still lose the exit code when the transport's cleanup disposes theProcessfirst, and this PR doesn't change that. Onmain, the test fails every time with "Failed to send message.".ConnectSendAndDispose_ServerExitsImmediately_CompletesWithExitDetails: 16 workers each run 200 cycles of connect, send and dispose againstsh -c 'echo <id> >&2; exit 3'. All must complete withinTestConstants.DefaultTimeout, and no send may fail without the exit details. With the fix, it takes 0.4 to 0.6 s. Onmainthe outcome is probabilistic, because it depends on hitting the race. Locally it failed in 3 of 3 runs, with 5, 8 and 9 of the 16 workers hung. In a stack capture during one such run, every hung thread had the stack above. With 100 cycles per worker, it failed in 7 of 7 runs, with 1 to 8 workers hung.Validation, on Linux x64 with the .NET 10 SDK (only the .NET 10 runtime is installed, so the tests ran on net10.0):
dotnet buildanddotnet build -c Release: 0 warnings, 0 errors.StdioClientTransportTests, Debug and Release: 56 passed.dotnet test tests/ModelContextProtocol.Tests -f net10.0 --filter "(Execution!=Manual)": 3,676 passed, 0 failed, 5 skipped. The skipped tests need an external API key, or are Windows-only or skipped by attribute.Notes
ShutdownTimeout, for example because the server has stopped reading its stdin, stdin is not closed. Dispose then waits for the exit and kills the server as before, so in that case it can take up to one moreShutdownTimeoutthan it does today.ShutdownTimeout, not by the send'scancellationToken, the same as the existingWaitForProcessExitAsynccalls.