Fix stderr event loss due to missing WaitForExit in DisposeProcess#1280
Fix stderr event loss due to missing WaitForExit in DisposeProcess#1280stephentoub merged 6 commits intomainfrom
Conversation
…events When GetUnexpectedExitExceptionAsync sees HasExited as false and returns null, DisposeProcess is called next. By that time the process may have exited, causing it to skip KillTree and call Dispose() without first calling WaitForExit() to flush ErrorDataReceived callbacks. Add a WaitForExit() call for already-exited processes to ensure all redirected output events are dispatched before disposal. Co-authored-by: stephentoub <[email protected]>
|
@copilot this test is still failing in CI. |
After line 222 recomputes processRunning as `processRunning && !HasExited(process)`, the else branch already implies HasExited was true (when processRunning was originally true). The redundant check is unnecessary. Co-authored-by: stephentoub <[email protected]>
The previous fix only called WaitForExit() in the else branch (process already exited). But when the process is still running, KillTree is called which uses either Kill+WaitForExit(int) or returns early if the process already exited - neither of which guarantees ErrorDataReceived event dispatch. Move WaitForExit() (no args) to be unconditional after the if/else block so it always runs before Dispose(). Co-authored-by: stephentoub <[email protected]>
The previous fix only covered the Moved the |
…es out If KillTree's kill succeeds but WaitForExit(int) times out, the process is still alive. Calling WaitForExit() (no args) unconditionally would then block indefinitely. Guard with HasExited to only flush stderr events when we know the process has actually exited. Co-authored-by: stephentoub <[email protected]>
|
@copilot there are a bunch of tests failing in CI now. |
…lTree The previous fix called WaitForExit() unconditionally or guarded only by HasExited, which could hang on Windows when KillTree left child processes holding output handles. Now WaitForExit() is only called when the process exited before DisposeProcess was called (!processRunning), meaning KillTree was not invoked. In that case, no child processes hold handles and WaitForExit() completes quickly. Co-authored-by: stephentoub <[email protected]>
The Windows test host crashes were caused by Reverted |
|
@copilot CI is still failing |
The CI failures are pre-existing and unrelated to this change. On the Windows Release job, all 1447 ModelContextProtocol.Tests passed on every TFM (net8.0, net9.0, net10.0, net472), including the originally flaky The only file changed in this PR is |
stephentoub
left a comment
stephentoub
left a comment
There was a problem hiding this comment.
Merging to try to unblock very unstable ci
StdioClientTransportTests.CreateAsync_ValidProcessInvalidServer_StdErrCallbackInvokedfails on net8.0 withcount == 0StdioClientTransport.DisposeProcesswhereprocess.Dispose()could be called without callingWaitForExit()(no args), causingErrorDataReceivedevents to be lostWaitForExit()(no args) beforeDispose()only when the process exited on its own (not afterKillTree), to ensure stderr events are flushed without risking a hang from child processes keeping output handles open✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.