Fix flaky TransientAndSidecarNodeCanCoexist test by waiting for task host process exit

[huulinhnguyen-dev]

Fixes #

Context

NodeProviderOutOfProcTaskHost.ShutdownConnectedNodes() signals shutdown complete when it receives the NodeShutdown packet from the task host process. However, NodeShutdown is sent by the process before it fully exits — the process still needs to disconnect the endpoint, close wait handles, and return from Run(). This creates a race window between Build() returning and the process actually dying, causing TransientAndSidecarNodeCanCoexist to intermittently fail with WaitForExit(3000) returning false.

Changes Made

• NodeProviderOutOfProcTaskHost.ShutdownConnectedNodes(): after _noNodesActiveEvent.WaitOne(), added a parallel wait for all task host processes to fully exit (30s timeout per process) before returning.
• Added using System.Threading.Tasks to support Task.WaitAll.

Testing

• Existing test TransientAndSidecarNodeCanCoexist covers this scenario.
• No new tests added — the fix eliminates the race condition that caused the flakiness rather than adjusting timeouts.

Notes

[JanProvaznik]

it's not acceptable to wait here. teardown should be fast

[Copilot]

Pull request overview

Fixes flakiness in TransientAndSidecarNodeCanCoexist by making NodeProviderOutOfProcTaskHost.ShutdownConnectedNodes() wait for task host processes to fully terminate after receiving NodeShutdown, closing the race between build completion and OS process exit.

Changes:

• Added an additional wait after _noNodesActiveEvent.WaitOne() to wait for task host processes to exit.
• Introduced parallel waiting via Task.WaitAll(...) and added using System.Threading.Tasks.

[Copilot]

This waits for all task host processes to exit, including sidecar/reused task hosts. In TaskHostTask.cs, task hosts can be launched with HandshakeOptions.NodeReuse when _useSidecarTaskHost is true (TaskHostTask.cs:356-363), meaning those processes are expected to stay alive across builds. In that case WaitForExit(30_000) will time out and adds up to ~30s of shutdown latency per build. Consider limiting the post-NodeShutdown process-exit wait to contexts that are expected to terminate (e.g., those whose TaskHostNodeKey handshake options do not include NodeReuse, or those that were told NodeBuildComplete(reuse:false)).

Suggested change

	// Task host nodes always terminate after a build (MSBUILDREUSETASKHOSTNODES is off by default).
	// NodeShutdown only means the process has begun its exit sequence — it hasn't fully exited yet.
	// Wait for actual OS-level process termination so callers can rely on the processes being dead.
	// Use parallel wait so multiple nodes don't add latency serially.
	Task.WaitAll(contextsToShutDown.Select(c => Task.Run(() => c.Process.WaitForExit(30_000))).ToArray());
	// When node reuse is disabled, NodeShutdown means the process has begun its exit sequence
	// but has not necessarily terminated yet. Wait for actual OS-level process termination so
	// callers can rely on the processes being dead. If reuse is enabled, nodes may reset for
	// reuse instead of exiting, so waiting for process termination can add unnecessary timeout
	// latency.
	if (!enableReuse)
	{
	// Use parallel wait so multiple nodes don't add latency serially.
	Task.WaitAll(contextsToShutDown.Select(c => Task.Run(() => c.Process.WaitForExit(30_000))).ToArray());
	}

[Copilot]

Process.WaitForExit(int) returns false on timeout, but the result is ignored here. That means this method can still return with task host processes alive, contradicting the comment that callers can rely on processes being dead. Please check the return value and either (a) log + forcefully terminate the process (similar to NodeProviderOutOfProcBase.NodeContext.WaitForExitAsync which kills after the exit timeout), or (b) throw/propagate a failure so callers/tests don’t assume termination when it didn’t happen.

Suggested change

	Task.WaitAll(contextsToShutDown.Select(c => Task.Run(() => c.Process.WaitForExit(30_000))).ToArray());
	Task.WaitAll(contextsToShutDown.Select(c => Task.Run(() =>
	{
	Process process = c.Process;
	if (process.WaitForExit(30_000))
	{
	return;
	}
	try
	{
	if (!process.HasExited)
	{
	process.Kill();
	}
	}
	catch (InvalidOperationException)
	{
	// The process exited after the timeout but before Kill could be issued.
	return;
	}
	if (!process.WaitForExit(30_000))
	{
	throw new TimeoutException($"Timed out waiting for task host process {process.Id} to exit after it was terminated.");
	}
	})).ToArray());

[Copilot]

Using Task.Run(() => Process.WaitForExit(...)) blocks ThreadPool threads for up to 30s per task host. Since NodeProviderOutOfProcBase already has a non-blocking NodeContext.WaitForExitAsync(...) implementation (with polling + kill-on-timeout), consider reusing that instead of wrapping a blocking wait in Task.Run.

Suggested change

	// Use parallel wait so multiple nodes don't add latency serially.
	Task.WaitAll(contextsToShutDown.Select(c => Task.Run(() => c.Process.WaitForExit(30_000))).ToArray());
	// Use the existing non-blocking wait implementation so multiple nodes can be observed in parallel
	// without consuming a ThreadPool thread per process.
	Task.WaitAll(contextsToShutDown.Select(c => c.WaitForExitAsync(30_000)).ToArray());

[github-actions]

Error Message Quality — ISSUE

SEVERITY: MODERATE
FILE: src/Build/BackEnd/Components/Communications/NodeProviderOutOfProcTaskHost.cs
LINES: 225
SCENARIO: A task host node loads a user task whose finalizer deadlocks (or hangs on I/O). WaitForExit(30_000) returns false after 30 seconds. The lambda completes normally, Task.WaitAll succeeds, and the caller proceeds believing all processes are dead — the exact guarantee the comment on line 223 promises — while the zombie process still holds file locks.
FINDING: A 30-second timeout that silently swallows failure produces zero diagnostic output. Users debugging "file is locked after build" or "cannot delete output directory" have no signal that a task host failed to exit. The code's own contract ("callers can rely on the processes being dead") is silently violated.
RECOMMENDATION: Log a warning when WaitForExit returns false, e.g.:

Task.WaitAll(contextsToShutDown.Select(c => Task.Run(() =>
{
    if (!c.Process.WaitForExit(30_000))
    {
        CommunicationsUtilities.Trace("Task host node (PID {0}) did not exit within 30 s timeout.", c.Process.Id);
    }
})).ToArray());

At minimum a CommunicationsUtilities.Trace call gives developers a breadcrumb; an ILoggingService message at MessageImportance.Low would surface it in diagnostic-verbosity logs for users.

Note

🔒 Integrity filter blocked 2 items

The following items were blocked because they don't meet the GitHub integrity level.

• #13585 pull_request_read: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".
• #13585 pull_request_read: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".

To allow these resources, lower min-integrity in your GitHub frontmatter:

tools:
  github:
    min-integrity: approved  # merged | approved | unapproved | none

Generated by Expert Code Review (on open) for issue #13585 · ● 42.7M

[github-actions]

Documentation Accuracy issue: The comment "Task host nodes always terminate after a build" is inaccurate. When MSBUILDREUSETASKHOSTNODES=1 and enableReuse is true:

1. The base ShutdownConnectedNodes sends NodeBuildComplete(prepareForReuse: true) to reusable nodes.
2. OutOfProcTaskHostNode.HandleNodeBuildComplete (line 510) sets BuildCompleteReuse because PrepareForReuse && ReuseTaskHostNodes is true.
3. OutOfProcTaskHost.cs:118-120 loops back (restart = true) — the process stays alive.
4. The NodeShutdown packet is still sent (so _noNodesActiveEvent fires), but the process doesn't exit.
5. WaitForExit(30_000) on those nodes blocks for the full 30 seconds, then the comment "callers can rely on the processes being dead" is also false.

This is a real configuration: set MSBUILDREUSETASKHOSTNODES=1 with node reuse enabled, and every ShutdownConnectedNodes call will stall for 30 seconds waiting on alive-and-reused task host processes.

Consider either:

• Making the comment accurate: "Task host nodes terminate after a build unless MSBUILDREUSETASKHOSTNODES=1" and skipping WaitForExit for nodes that were told to reuse, or
• Filtering contextsToShutDown to only wait on nodes that were actually told to terminate (mirroring the base class logic that only waits for !reuseThisNode).

[JanProvaznik]

changing production behavior is not warranted for test fix, the test should test the right thing