Harden BuildHost interactions

[jasonmalinowski]

We've had some reports of BuildHosts floating around. This generally tightens up some interactions there. Commit-at-a-time recommended.

Microsoft Reviewers: Open in CodeFlow

[azure-pipelines]

Azure Pipelines:
Successfully started running 2 pipeline(s).
There may be pipelines that require an authorized user to comment /azp run to run.

[jasonmalinowski]

This shouldn't be needed since we're now going to unceremoniously kill the process, but it seems easier to add this than argue why it's not here, and maybe this also helps if for some reason the main process is unable to kill the process, or something else were to throw.

[jasonmalinowski]

Adding this condition to ensure that if the ConnectAsync() threw, that we aren't going to null ref and still try shutting things down cleanly.

[Copilot]

Pull request overview

This PR hardens MSBuild BuildHost lifecycle handling by improving cleanup paths and tightening initialization/disposal behavior so disconnected or failed-to-initialize BuildHosts don’t linger.

Changes:

• Dispose the RPC pipe stream when shutting down RpcClient.
• Ensure BuildHost processes are disposed on initialization failures and are given a short grace period to exit before being force-killed.
• Move BuildHost Disconnected subscription to the point where the process is tracked in the manager.

Show a summary per file

File	Description
src/Workspaces/MSBuild/Core/Rpc/RpcClient.cs	Disposes the underlying PipeStream during shutdown to proactively tear down the RPC transport.
src/Workspaces/MSBuild/Core/MSBuild/BuildHostProcessManager.cs	Improves BuildHost initialization failure cleanup and disposal/kill behavior; adjusts Disconnected event subscription timing.

Copilot's findings

Suppressed comments (1)

src/Workspaces/MSBuild/Core/MSBuild/BuildHostProcessManager.cs:516

• DisposeAsync returns immediately when the process has already exited, which skips shutting down the RpcClient. With RpcClient.Shutdown now disposing the underlying PipeStream, this early return can leave the pipe handle undisposed in the common "process died" path.

                // If the process is already exited, then we simply have nothing left to do
                if (_process.HasExited)
                    return;

                if (_buildHost is not null)

• Files reviewed: 2/2 changed files
• Comments generated: 1

[dibarbet]

@jasonmalinowski do we have anything on the buildhost side to kill itself if its parent goes away? Wonder if that is one potential cause

[jasonmalinowski]

@dibarbet We do not have that today; the expectation was since we're listening to the pipe, if the app process goes away the pipe should be closed. As discussed, maybe we should be adding that, but I'd want some evidence we need that first.

[Copilot]

Copilot's findings

Suppressed comments (3)

src/Workspaces/MSBuild/Core/MSBuild/BuildHostProcessManager.cs:135

• The connection-failure exception message still reads process.ExitCode unconditionally. Process.ExitCode throws InvalidOperationException if the process hasn't exited yet (which can still be the case even after DisposeAsync() attempts to kill it), potentially hiding the real connection failure. Also the message should use its (possessive) rather than it's. Consider capturing an exit-code string guarded by HasExited (or a try/catch) and updating the wording.

                await buildHostProcess.DisposeAsync().ConfigureAwait(false);

                throw new Exception($"The build host was started but we were unable to connect to it's pipe. The process exited with {process.ExitCode}. Process output:{Environment.NewLine}{buildHostProcess.GetBuildHostProcessOutput()}", innerException: e);
            }

src/Workspaces/MSBuild/Core/MSBuild/BuildHostProcessManager.cs:507

• Doc comment typo/grammar: "not expected to throw expectations" should be "not expected to throw exceptions" (and the first sentence currently over-promises absolute termination). Please fix the wording to match the intent.

        /// <summary>
        /// Shuts down the BuildHost process, ensuring the process is killed by the time this returns. This method is not expected to throw
        /// expectations so callers don't have to deal with exceptions coming from broken BuildHosts.
        /// </summary>

src/Workspaces/MSBuild/Core/MSBuild/BuildHostProcessManager.cs:530

• DisposeAsync() returns immediately when _process.HasExited is true. That skips _rpcClient.Shutdown(), which now disposes the pipe stream, so an unexpected process exit can leave the pipe handle undisposed in this process. Suggest removing the early return and instead only skip the remote BuildHost.ShutdownAsync call when the process has already exited, but still shut down the RPC client for local cleanup.

                // If the process is already exited, then we simply have nothing left to do
                if (_process.HasExited)
                    return;

                if (_buildHost is not null)
                {
                    _logger?.LogTrace("Sending a Shutdown request to the BuildHost.");
                    await BuildHost.ShutdownAsync(CancellationToken.None).ConfigureAwait(false);
                }

                if (_rpcClient is not null)
                {
                    _rpcClient.Shutdown();
                    _logger?.LogTrace($"{nameof(RpcClient)} has been shut down.");
                }

• Files reviewed: 2/2 changed files
• Comments generated: 0 new