microsoft
ExternalProcessService async and stream cleanup needs a second look
Our wait and kill logic at the end of this method looks like it will chop off output or error message and doesn't seem to address all the different timings of process end + errors lines processed + output lines processed.
https://github.com/microsoft/mcp/blob/main/core/Azure.Mcp.Core/src/Services/ProcessExecution/ExternalProcessService.cs#L72
Per the Process documentation:
The application that is processing the asynchronous output should call the WaitForExit() method to ensure that the output buffer has been flushed. Note that specifying a timeout by using the WaitForExit(Int32) overload does not ensure the output buffer has been flushed.
(From Process.ErrorDataReceived.)
As written, output and error data could still come in asynchronously after the ExecuteAsync method has returned. I think the only proper fix is to either always wait for the process to exit before leaving the method (including on exceptions) or catch and swallow ObjectDisposedExceptions in the ErrorDataReceived and OutputDataReceived event handlers.
I think it's pragmatic to wait a reasonable amount of time for process exit. I don't know that it's reasonable to bind our "do something with an external process" call to the life of that external process indefinitely. Sometimes, processes don't exit normally. Sometimes, processes don't exit when you kill them with process.Kill(). I think a resilient invoker will attempt happy path process exit, then forced process kill, then will eventually just throw an exception saying that the external process couldn't be stopped.
Our consuming tool should decide what to do with that. Should our local mcp server halt because we spawned an external azd process that we can't kill? Should our tool task run indefinitely waiting on a zombie process to stop?
I think the best approach is to, again, wait for normal exit using the timeout parameter, then fall back to forced kill + an exception, while also guarding against the late event handler calls with the ObjectDisposedException catch
I think the best approach is to, again, wait for normal exit using the timeout parameter, then fall back to forced kill + an exception, while also guarding against the late event handler calls with the ObjectDisposedException catch
That seems reasonable to me.
