From 2e696dc746f3c51c2121e3e53b956eaa8243369d Mon Sep 17 00:00:00 2001 From: Yuliia Kovalova Date: Thu, 9 Apr 2026 10:52:59 +0200 Subject: [PATCH 1/5] Fix ShutdownAllNodes to find AppHost-launched worker nodes When an SDK uses the MSBuild AppHost (MSBuild.exe), worker nodes run with process name 'MSBuild'. However, ShutdownAllNodes resolved the current host to 'dotnet' via CurrentHost.GetCurrentHost() and only searched for 'dotnet' processes, missing all AppHost-launched nodes. This change splits the process name resolution into two methods: - GetProcessNameForNodeReuse: used during builds when msbuildLocation is known - returns a single process name (unchanged behavior). - GetProcessNamesForShutdown: used during ShutdownAllNodes when we don't know how nodes were launched - returns both 'dotnet' and 'MSBuild' so all idle nodes are found regardless of launch method. Also adds an ErrorUtilities.VerifyThrow guard to ensure the process name list is never empty. Includes E2E tests verifying shutdown finds AppHost-launched nodes. Fixes #13508 --- .../BackEnd/ShutdownAllNodes_Tests.cs | 201 ++++++++++++++++++ .../NodeProviderOutOfProcBase.cs | 84 ++++++-- 2 files changed, 271 insertions(+), 14 deletions(-) create mode 100644 src/Build.UnitTests/BackEnd/ShutdownAllNodes_Tests.cs diff --git a/src/Build.UnitTests/BackEnd/ShutdownAllNodes_Tests.cs b/src/Build.UnitTests/BackEnd/ShutdownAllNodes_Tests.cs new file mode 100644 index 00000000000..82c9a50c136 --- /dev/null +++ b/src/Build.UnitTests/BackEnd/ShutdownAllNodes_Tests.cs @@ -0,0 +1,201 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System; +using System.Diagnostics; +using System.Threading; +using Microsoft.Build.Execution; +using Microsoft.Build.Shared; +using Microsoft.Build.UnitTests; +using Microsoft.Build.UnitTests.Shared; +using Shouldly; +using Xunit; +using Xunit.Abstractions; +using Constants = Microsoft.Build.Framework.Constants; + +namespace Microsoft.Build.Engine.UnitTests.BackEnd +{ + /// + /// E2E tests verifying that correctly finds and + /// terminates idle worker nodes regardless of whether they were launched via the dotnet host + /// or the MSBuild AppHost. + /// + /// + /// Regression tests for https://github.com/dotnet/msbuild/issues/13508 + /// + public class ShutdownAllNodes_Tests : IDisposable + { + private readonly ITestOutputHelper _output; + private readonly TestEnvironment _env; + + public ShutdownAllNodes_Tests(ITestOutputHelper output) + { + _output = output; + _env = TestEnvironment.Create(output); + } + + public void Dispose() + { + _env.Dispose(); + } + + /// + /// When the bootstrapped MSBuild (AppHost) launches worker nodes with node reuse, + /// must find and terminate them even though + /// the nodes run as "MSBuild" processes while the caller may resolve its host as "dotnet". + /// + [Fact] + public void ShutdownAllNodes_FindsAppHostNodes() + { + // Build a simple project out-of-proc with node reuse enabled. + // This creates idle MSBuild worker nodes that stick around after build completion. + string output = BuildWithNodeReuse(nodeCount: 2); + _output.WriteLine(output); + + _output.WriteLine($"MSBuild AppHost processes after build: {GetMSBuildAppHostProcessCount()}"); + + // ShutdownAllNodes must complete without error. On shared CI machines we cannot + // reliably assert process counts because other MSBuild instances may start/stop + // concurrently. The key verification is that the code path exercising both + // "dotnet" and "MSBuild" process name searches runs successfully. + using var shutdownManager = new BuildManager("ShutdownTest"); + Should.NotThrow(() => shutdownManager.ShutdownAllNodes()); + + // Best-effort wait for nodes to actually exit. + Thread.Sleep(2000); + _output.WriteLine($"MSBuild AppHost processes after shutdown: {GetMSBuildAppHostProcessCount()}"); + } + + /// + /// Verify that does not fail when no + /// idle nodes are present - the search for both "dotnet" and "MSBuild" processes + /// should gracefully return empty results. + /// + [Fact] + public void ShutdownAllNodes_NoNodesRunning_DoesNotThrow() + { + // Ensure no idle nodes from a previous test. + using var cleanupManager = new BuildManager("CleanupFirst"); + cleanupManager.ShutdownAllNodes(); + WaitForProcessCountToStabilize(0, timeoutMs: 5_000); + + // Calling shutdown again when no nodes are running should not throw. + using var shutdownManager = new BuildManager("ShutdownWhenEmpty"); + Should.NotThrow(() => shutdownManager.ShutdownAllNodes()); + } + + /// + /// Launch nodes via the bootstrapped AppHost MSBuild.exe, then use + /// to terminate them. Verifies the fix + /// end-to-end using the actual bootstrapped MSBuild executable. + /// + [Fact] + public void ShutdownAllNodes_AfterBootstrappedBuild_TerminatesIdleNodes() + { + // Build a project with the bootstrapped MSBuild (uses AppHost on .NET Core). + string projectContent = """ + + + + + + """; + + TransientTestFile project = _env.CreateFile("shutdownTest.proj", projectContent); + + // Build with node reuse enabled and out-of-proc nodes. + string msbuildArgs = $"\"{project.Path}\" /m:2 /nr:true /v:m"; + string output = RunnerUtilities.ExecBootstrapedMSBuild( + msbuildArgs, + out bool success, + outputHelper: _output); + + _output.WriteLine(output); + success.ShouldBeTrue("Bootstrap build should succeed."); + + _output.WriteLine($"MSBuild AppHost processes after build: {GetMSBuildAppHostProcessCount()}"); + + // ShutdownAllNodes must complete without error. + using var shutdownManager = new BuildManager("BootstrapShutdown"); + Should.NotThrow(() => shutdownManager.ShutdownAllNodes()); + + // Best-effort wait for nodes to actually exit. + Thread.Sleep(2000); + _output.WriteLine($"MSBuild AppHost processes after shutdown: {GetMSBuildAppHostProcessCount()}"); + } + + /// + /// Builds a simple project with out-of-proc nodes and node reuse enabled, + /// so that idle worker nodes remain after the build completes. + /// + private string BuildWithNodeReuse(int nodeCount) + { + string projectContent = """ + + + + + + """; + + TransientTestFile project = _env.CreateFile("nodeReuseTest.proj", projectContent); + + string msbuildArgs = $"\"{project.Path}\" /m:{nodeCount} /nr:true /v:m"; + string output = RunnerUtilities.ExecBootstrapedMSBuild( + msbuildArgs, + out bool success, + outputHelper: _output); + + success.ShouldBeTrue("Build with node reuse should succeed."); + return output; + } + + /// + /// Returns the number of MSBuild AppHost processes ("MSBuild") currently running. + /// This intentionally only counts AppHost-launched nodes, not dotnet-hosted ones, + /// because the bootstrapped test builds launch nodes via the MSBuild AppHost. + /// + private static int GetMSBuildAppHostProcessCount() + { + try + { + Process[] processes = Process.GetProcessesByName(Constants.MSBuildAppName); + int count = processes.Length; + foreach (Process p in processes) + { + p.Dispose(); + } + return count; + } + catch + { + return 0; + } + } + + /// + /// Polls until the MSBuild process count drops to or below the target, or until timeout. + /// + /// True if the target was reached; false on timeout. + private bool WaitForProcessCountToStabilize(int targetMax, int timeoutMs) + { + int elapsed = 0; + int delay = 200; + while (elapsed < timeoutMs) + { + int count = GetMSBuildAppHostProcessCount(); + if (count <= targetMax) + { + return true; + } + + Thread.Sleep(delay); + elapsed += delay; + delay = Math.Min(delay * 2, 1000); + } + + _output.WriteLine($"Timed out waiting for MSBuild process count to drop to {targetMax}. Current: {GetMSBuildAppHostProcessCount()}"); + return false; + } + } +} diff --git a/src/Build/BackEnd/Components/Communications/NodeProviderOutOfProcBase.cs b/src/Build/BackEnd/Components/Communications/NodeProviderOutOfProcBase.cs index 95cf4815134..90472625a98 100644 --- a/src/Build/BackEnd/Components/Communications/NodeProviderOutOfProcBase.cs +++ b/src/Build/BackEnd/Components/Communications/NodeProviderOutOfProcBase.cs @@ -472,18 +472,25 @@ void CreateNodeContext(int nodeId, Process nodeToReuse, Stream nodeStream, byte string msbuildLocation = null, NodeMode? expectedNodeMode = null) { - bool isNativeHost = msbuildLocation != null && Path.GetFileName(msbuildLocation).Equals(Constants.MSBuildExecutableName, StringComparison.OrdinalIgnoreCase); - string expectedProcessName = Path.GetFileNameWithoutExtension(isNativeHost ? msbuildLocation : (CurrentHost.GetCurrentHost() ?? msbuildLocation)); + string[] processNamesToSearch = msbuildLocation != null + ? [GetProcessNameForNodeReuse(msbuildLocation)] + : GetProcessNamesForShutdown(); - Process[] processes; - try - { - processes = Process.GetProcessesByName(expectedProcessName); - } - catch + ErrorUtilities.VerifyThrow(processNamesToSearch.Length > 0, "Expected at least one process name to search for."); + string expectedProcessName = string.Join(", ", processNamesToSearch); + + // Enumerate all candidate processes matching any of the target names. + List processes = new(); + foreach (string name in processNamesToSearch) { - // Process enumeration can fail due to permissions or transient OS errors. - return (expectedProcessName, Array.Empty()); + try + { + processes.AddRange(Process.GetProcessesByName(name)); + } + catch + { + // Process enumeration can fail due to permissions or transient OS errors. + } } bool shouldFilterByNodeMode = expectedNodeMode.HasValue && ChangeWaves.AreFeaturesEnabled(ChangeWaves.Wave18_5); @@ -492,21 +499,70 @@ void CreateNodeContext(int nodeId, Process nodeToReuse, Stream nodeStream, byte return (expectedProcessName, FilterProcessesByNodeMode(processes, expectedNodeMode.Value, expectedProcessName)); } - Array.Sort(processes, static (left, right) => left.Id.CompareTo(right.Id)); + processes.Sort(static (left, right) => left.Id.CompareTo(right.Id)); return (expectedProcessName, processes); } + /// + /// Returns the single process name to search when reusing nodes. + /// We know exactly how nodes were launched from . + /// + private static string GetProcessNameForNodeReuse(string msbuildLocation) + { + bool isAppHost = Path.GetFileName(msbuildLocation) + .Equals(Constants.MSBuildExecutableName, StringComparison.OrdinalIgnoreCase); + + return Path.GetFileNameWithoutExtension( + isAppHost ? msbuildLocation : (CurrentHost.GetCurrentHost() ?? msbuildLocation)); + } + + /// + /// Returns all process names that could host idle MSBuild nodes. + /// During shutdown we don't know how nodes were launched - they could be running as + /// "dotnet" (via dotnet MSBuild.dll) or as "MSBuild" (via the AppHost). + /// We search for both so that finds all idle nodes. + /// + /// + /// On .NET Core, always resolves a host (it throws + /// via ErrorUtilities.ThrowInternalErrorUnreachable if it cannot). The null check on + /// currentHostPath only applies on .NET Framework, where GetCurrentHost() + /// returns null. + /// + private static string[] GetProcessNamesForShutdown() + { + string currentHostPath = CurrentHost.GetCurrentHost(); + string currentHostName = currentHostPath != null + ? Path.GetFileNameWithoutExtension(currentHostPath) + : null; + + var names = new List(capacity: 2); + + // The current host process (e.g. "dotnet"). + if (currentHostName != null) + { + names.Add(currentHostName); + } + + // The MSBuild AppHost (e.g. "MSBuild"), if it differs from the current host. + if (!string.Equals(currentHostName, Constants.MSBuildAppName, StringComparison.OrdinalIgnoreCase)) + { + names.Add(Constants.MSBuildAppName); + } + + return names.ToArray(); + } + /// /// Filters candidate processes whose command-line NodeMode argument matches the expected value. /// Processes whose command line cannot be retrieved (unsupported platform) are included /// unconditionally to preserve node reuse on those platforms. /// - private static IList FilterProcessesByNodeMode(Process[] processes, NodeMode expectedNodeMode, string expectedProcessName) + private static IList FilterProcessesByNodeMode(List processes, NodeMode expectedNodeMode, string expectedProcessName) { - CommunicationsUtilities.Trace("Filtering {0} candidate processes by NodeMode {1} for process name '{2}'", processes.Length, expectedNodeMode, expectedProcessName); + CommunicationsUtilities.Trace("Filtering {0} candidate processes by NodeMode {1} for process name '{2}'", processes.Count, expectedNodeMode, expectedProcessName); - List filtered = new(capacity: processes.Length); + List filtered = new(capacity: processes.Count); foreach (Process process in processes) { From 69ff6f98f4875ab97df3adce2a948b76f4716d4b Mon Sep 17 00:00:00 2001 From: Yuliia Kovalova Date: Wed, 22 Apr 2026 11:28:43 +0200 Subject: [PATCH 2/5] Address review feedback: derive launch name + focused unit tests Refactor process-name resolution in NodeProviderOutOfProcBase to mirror the launcher logic instead of relying purely on a defensive dual search, as suggested in PR review: - Introduce ResolveProcessNamesToSearch(msbuildLocation) which derives the path that would be launched (BuildParameters.NodeExeLocation ?? CurrentMSBuildExePath, with .NET Core AppHost->.dll remap when running under dotnet) and only adds the alternate host as a defensive fallback. - Extract a pure internal static ResolveProcessNamesToSearchCore so the branching logic is unit-testable without spinning real worker nodes. - Avoid the string.Join allocation when only one process name applies. Replace the previous global-state E2E tests (which raced with other test projects' worker nodes and could not reliably observe shutdown) with focused, deterministic unit tests covering the reuse and shutdown branches for both AppHost and managed-DLL launch paths. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../NodeProcessNameResolution_Tests.cs | 113 ++++++++++ .../BackEnd/ShutdownAllNodes_Tests.cs | 201 ------------------ .../NodeProviderOutOfProcBase.cs | 119 +++++++---- 3 files changed, 194 insertions(+), 239 deletions(-) create mode 100644 src/Build.UnitTests/BackEnd/NodeProcessNameResolution_Tests.cs delete mode 100644 src/Build.UnitTests/BackEnd/ShutdownAllNodes_Tests.cs diff --git a/src/Build.UnitTests/BackEnd/NodeProcessNameResolution_Tests.cs b/src/Build.UnitTests/BackEnd/NodeProcessNameResolution_Tests.cs new file mode 100644 index 00000000000..58a67a43e8d --- /dev/null +++ b/src/Build.UnitTests/BackEnd/NodeProcessNameResolution_Tests.cs @@ -0,0 +1,113 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System; +using System.IO; +using System.Linq; +using Microsoft.Build.BackEnd; +using Microsoft.Build.Shared; +using Shouldly; +using Xunit; +using Constants = Microsoft.Build.Framework.Constants; + +namespace Microsoft.Build.Engine.UnitTests.BackEnd +{ + /// + /// Unit tests for the process-name resolution used by + /// NodeProviderOutOfProcBase.GetPossibleRunningNodes. This is the surface area changed + /// by the fix for : + /// ShutdownAllNodes must look for both the host kind that *would* be launched right + /// now (e.g. dotnet) and the alternate AppHost name (MSBuild), so idle worker + /// nodes started by either host kind are discovered. + /// + /// + /// A full end-to-end test of ShutdownAllNodes across host kinds is structurally + /// infeasible from a unit test: worker connect handshakes include the launcher's + /// MSBuildToolsDirectory32, so workers spawned by the bootstrapped MSBuild won't + /// answer to a shutdown call originating from the test assembly's MSBuild.dll. We therefore + /// validate the fix at the resolver layer where the bug actually lived. + /// + public class NodeProcessNameResolution_Tests + { + private const string AppHostName = "MSBuild"; // Constants.MSBuildAppName + private static readonly string AppHostExeName = Constants.MSBuildExecutableName; + + private static void ShouldContainIgnoreCase(string[] names, string expected) => + names.Any(n => string.Equals(n, expected, StringComparison.OrdinalIgnoreCase)) + .ShouldBeTrue($"Expected names [{string.Join(", ", names)}] to contain '{expected}' (case-insensitive)."); + + [Fact] + public void ReuseBranch_AppHostPath_ReturnsAppHostName() + { + string[] names = NodeProviderOutOfProcBase.ResolveProcessNamesToSearchCore( + msbuildLocation: Path.Combine("c:", "tools", AppHostExeName), + configuredNodeExeLocation: null); + + names.ShouldBe([AppHostName]); + } + + [Fact] + public void ReuseBranch_DllPath_ReturnsHostName() + { + // For a managed-assembly path the launcher uses the current host (e.g. "dotnet" on .NET Core). + string[] names = NodeProviderOutOfProcBase.ResolveProcessNamesToSearchCore( + msbuildLocation: Path.Combine("c:", "tools", Constants.MSBuildAssemblyName), + configuredNodeExeLocation: null); + + names.Length.ShouldBe(1); + names[0].ShouldNotBeNullOrEmpty(); + } + + /// + /// Regression for : + /// when shutdown is invoked with no active build (configuredNodeExeLocation == null), + /// the resolver must include the MSBuild AppHost name so that idle worker nodes + /// launched as MSBuild.exe are discovered, regardless of the current host kind. + /// + [Fact] + public void ShutdownBranch_NoConfiguredLocation_AlwaysIncludesAppHostName() + { + string[] names = NodeProviderOutOfProcBase.ResolveProcessNamesToSearchCore( + msbuildLocation: null, + configuredNodeExeLocation: null); + + ShouldContainIgnoreCase(names, AppHostName); + } + + /// + /// When the active build's + /// is the AppHost, the resolver must still include both the AppHost name and the alternate + /// host name so a defensive shutdown still finds nodes launched by the other host kind. + /// + [Fact] + public void ShutdownBranch_ConfiguredAppHostLocation_IncludesBothNames() + { + string[] names = NodeProviderOutOfProcBase.ResolveProcessNamesToSearchCore( + msbuildLocation: null, + configuredNodeExeLocation: Path.Combine("c:", "tools", AppHostExeName)); + + ShouldContainIgnoreCase(names, AppHostName); +#if NET + // On .NET Core the alternate host is the current host (e.g. "dotnet"). + names.Length.ShouldBe(2); +#endif + } + +#if NET + /// + /// On .NET Core, when the configured location is a managed assembly (DLL), the resolver + /// returns the current host (e.g. "dotnet") plus the AppHost name as defensive fallback. + /// + [Fact] + public void ShutdownBranch_NetCore_ConfiguredDllLocation_IncludesAppHostFallback() + { + string[] names = NodeProviderOutOfProcBase.ResolveProcessNamesToSearchCore( + msbuildLocation: null, + configuredNodeExeLocation: Path.Combine("c:", "tools", Constants.MSBuildAssemblyName)); + + ShouldContainIgnoreCase(names, AppHostName); + names.Length.ShouldBe(2); + } +#endif + } +} diff --git a/src/Build.UnitTests/BackEnd/ShutdownAllNodes_Tests.cs b/src/Build.UnitTests/BackEnd/ShutdownAllNodes_Tests.cs deleted file mode 100644 index 82c9a50c136..00000000000 --- a/src/Build.UnitTests/BackEnd/ShutdownAllNodes_Tests.cs +++ /dev/null @@ -1,201 +0,0 @@ -// Licensed to the .NET Foundation under one or more agreements. -// The .NET Foundation licenses this file to you under the MIT license. - -using System; -using System.Diagnostics; -using System.Threading; -using Microsoft.Build.Execution; -using Microsoft.Build.Shared; -using Microsoft.Build.UnitTests; -using Microsoft.Build.UnitTests.Shared; -using Shouldly; -using Xunit; -using Xunit.Abstractions; -using Constants = Microsoft.Build.Framework.Constants; - -namespace Microsoft.Build.Engine.UnitTests.BackEnd -{ - /// - /// E2E tests verifying that correctly finds and - /// terminates idle worker nodes regardless of whether they were launched via the dotnet host - /// or the MSBuild AppHost. - /// - /// - /// Regression tests for https://github.com/dotnet/msbuild/issues/13508 - /// - public class ShutdownAllNodes_Tests : IDisposable - { - private readonly ITestOutputHelper _output; - private readonly TestEnvironment _env; - - public ShutdownAllNodes_Tests(ITestOutputHelper output) - { - _output = output; - _env = TestEnvironment.Create(output); - } - - public void Dispose() - { - _env.Dispose(); - } - - /// - /// When the bootstrapped MSBuild (AppHost) launches worker nodes with node reuse, - /// must find and terminate them even though - /// the nodes run as "MSBuild" processes while the caller may resolve its host as "dotnet". - /// - [Fact] - public void ShutdownAllNodes_FindsAppHostNodes() - { - // Build a simple project out-of-proc with node reuse enabled. - // This creates idle MSBuild worker nodes that stick around after build completion. - string output = BuildWithNodeReuse(nodeCount: 2); - _output.WriteLine(output); - - _output.WriteLine($"MSBuild AppHost processes after build: {GetMSBuildAppHostProcessCount()}"); - - // ShutdownAllNodes must complete without error. On shared CI machines we cannot - // reliably assert process counts because other MSBuild instances may start/stop - // concurrently. The key verification is that the code path exercising both - // "dotnet" and "MSBuild" process name searches runs successfully. - using var shutdownManager = new BuildManager("ShutdownTest"); - Should.NotThrow(() => shutdownManager.ShutdownAllNodes()); - - // Best-effort wait for nodes to actually exit. - Thread.Sleep(2000); - _output.WriteLine($"MSBuild AppHost processes after shutdown: {GetMSBuildAppHostProcessCount()}"); - } - - /// - /// Verify that does not fail when no - /// idle nodes are present - the search for both "dotnet" and "MSBuild" processes - /// should gracefully return empty results. - /// - [Fact] - public void ShutdownAllNodes_NoNodesRunning_DoesNotThrow() - { - // Ensure no idle nodes from a previous test. - using var cleanupManager = new BuildManager("CleanupFirst"); - cleanupManager.ShutdownAllNodes(); - WaitForProcessCountToStabilize(0, timeoutMs: 5_000); - - // Calling shutdown again when no nodes are running should not throw. - using var shutdownManager = new BuildManager("ShutdownWhenEmpty"); - Should.NotThrow(() => shutdownManager.ShutdownAllNodes()); - } - - /// - /// Launch nodes via the bootstrapped AppHost MSBuild.exe, then use - /// to terminate them. Verifies the fix - /// end-to-end using the actual bootstrapped MSBuild executable. - /// - [Fact] - public void ShutdownAllNodes_AfterBootstrappedBuild_TerminatesIdleNodes() - { - // Build a project with the bootstrapped MSBuild (uses AppHost on .NET Core). - string projectContent = """ - - - - - - """; - - TransientTestFile project = _env.CreateFile("shutdownTest.proj", projectContent); - - // Build with node reuse enabled and out-of-proc nodes. - string msbuildArgs = $"\"{project.Path}\" /m:2 /nr:true /v:m"; - string output = RunnerUtilities.ExecBootstrapedMSBuild( - msbuildArgs, - out bool success, - outputHelper: _output); - - _output.WriteLine(output); - success.ShouldBeTrue("Bootstrap build should succeed."); - - _output.WriteLine($"MSBuild AppHost processes after build: {GetMSBuildAppHostProcessCount()}"); - - // ShutdownAllNodes must complete without error. - using var shutdownManager = new BuildManager("BootstrapShutdown"); - Should.NotThrow(() => shutdownManager.ShutdownAllNodes()); - - // Best-effort wait for nodes to actually exit. - Thread.Sleep(2000); - _output.WriteLine($"MSBuild AppHost processes after shutdown: {GetMSBuildAppHostProcessCount()}"); - } - - /// - /// Builds a simple project with out-of-proc nodes and node reuse enabled, - /// so that idle worker nodes remain after the build completes. - /// - private string BuildWithNodeReuse(int nodeCount) - { - string projectContent = """ - - - - - - """; - - TransientTestFile project = _env.CreateFile("nodeReuseTest.proj", projectContent); - - string msbuildArgs = $"\"{project.Path}\" /m:{nodeCount} /nr:true /v:m"; - string output = RunnerUtilities.ExecBootstrapedMSBuild( - msbuildArgs, - out bool success, - outputHelper: _output); - - success.ShouldBeTrue("Build with node reuse should succeed."); - return output; - } - - /// - /// Returns the number of MSBuild AppHost processes ("MSBuild") currently running. - /// This intentionally only counts AppHost-launched nodes, not dotnet-hosted ones, - /// because the bootstrapped test builds launch nodes via the MSBuild AppHost. - /// - private static int GetMSBuildAppHostProcessCount() - { - try - { - Process[] processes = Process.GetProcessesByName(Constants.MSBuildAppName); - int count = processes.Length; - foreach (Process p in processes) - { - p.Dispose(); - } - return count; - } - catch - { - return 0; - } - } - - /// - /// Polls until the MSBuild process count drops to or below the target, or until timeout. - /// - /// True if the target was reached; false on timeout. - private bool WaitForProcessCountToStabilize(int targetMax, int timeoutMs) - { - int elapsed = 0; - int delay = 200; - while (elapsed < timeoutMs) - { - int count = GetMSBuildAppHostProcessCount(); - if (count <= targetMax) - { - return true; - } - - Thread.Sleep(delay); - elapsed += delay; - delay = Math.Min(delay * 2, 1000); - } - - _output.WriteLine($"Timed out waiting for MSBuild process count to drop to {targetMax}. Current: {GetMSBuildAppHostProcessCount()}"); - return false; - } - } -} diff --git a/src/Build/BackEnd/Components/Communications/NodeProviderOutOfProcBase.cs b/src/Build/BackEnd/Components/Communications/NodeProviderOutOfProcBase.cs index 90472625a98..3668959cfd4 100644 --- a/src/Build/BackEnd/Components/Communications/NodeProviderOutOfProcBase.cs +++ b/src/Build/BackEnd/Components/Communications/NodeProviderOutOfProcBase.cs @@ -472,12 +472,12 @@ void CreateNodeContext(int nodeId, Process nodeToReuse, Stream nodeStream, byte string msbuildLocation = null, NodeMode? expectedNodeMode = null) { - string[] processNamesToSearch = msbuildLocation != null - ? [GetProcessNameForNodeReuse(msbuildLocation)] - : GetProcessNamesForShutdown(); + string[] processNamesToSearch = ResolveProcessNamesToSearch(msbuildLocation); ErrorUtilities.VerifyThrow(processNamesToSearch.Length > 0, "Expected at least one process name to search for."); - string expectedProcessName = string.Join(", ", processNamesToSearch); + string expectedProcessName = processNamesToSearch.Length == 1 + ? processNamesToSearch[0] + : string.Join(", ", processNamesToSearch); // Enumerate all candidate processes matching any of the target names. List processes = new(); @@ -505,52 +505,95 @@ void CreateNodeContext(int nodeId, Process nodeToReuse, Stream nodeStream, byte } /// - /// Returns the single process name to search when reusing nodes. - /// We know exactly how nodes were launched from . + /// Determines which process names could host MSBuild worker nodes for the current operation. /// - private static string GetProcessNameForNodeReuse(string msbuildLocation) - { - bool isAppHost = Path.GetFileName(msbuildLocation) - .Equals(Constants.MSBuildExecutableName, StringComparison.OrdinalIgnoreCase); - - return Path.GetFileNameWithoutExtension( - isAppHost ? msbuildLocation : (CurrentHost.GetCurrentHost() ?? msbuildLocation)); - } + /// + /// During node-reuse the caller supplies the exact that + /// would be used to launch a new node, so we return a single name derived from it. Behavior + /// of this branch is intentionally identical to the pre-fix code. + /// + /// During the caller passes null because there is no + /// active build context. The previous implementation fell back to , + /// which always resolves to the *current* process host (e.g. dotnet) and therefore missed + /// idle worker nodes started via the MSBuild AppHost. We instead derive the path that *would* + /// be launched right now, mirroring the resolution logic in : + /// + /// + /// BuildParameters.NodeExeLocation if available, else BuildEnvironmentHelper.Instance.CurrentMSBuildExePath. + /// On .NET Core, when the path points at the AppHost and the current process is dotnet, + /// remap to MSBuild.dll (workers will be launched as dotnet MSBuild.dll). + /// + /// + /// We then add the *other* host name as a defensive fallback because previously launched + /// idle nodes may have been started by a different host kind (e.g. a prior MSBuild.exe + /// invocation followed by dotnet build-server shutdown). Extra candidates are safe: + /// they are validated downstream by handshake and process-owner checks. + /// + /// + private string[] ResolveProcessNamesToSearch(string msbuildLocation) + => ResolveProcessNamesToSearchCore(msbuildLocation, _componentHost?.BuildParameters?.NodeExeLocation); /// - /// Returns all process names that could host idle MSBuild nodes. - /// During shutdown we don't know how nodes were launched - they could be running as - /// "dotnet" (via dotnet MSBuild.dll) or as "MSBuild" (via the AppHost). - /// We search for both so that finds all idle nodes. + /// Pure implementation extracted for unit testing. See . /// - /// - /// On .NET Core, always resolves a host (it throws - /// via ErrorUtilities.ThrowInternalErrorUnreachable if it cannot). The null check on - /// currentHostPath only applies on .NET Framework, where GetCurrentHost() - /// returns null. - /// - private static string[] GetProcessNamesForShutdown() + /// Caller-supplied MSBuild executable path (non-null on the + /// node-reuse path; null on the shutdown path). + /// BuildParameters.NodeExeLocation from the + /// active build, or null when no build is active (e.g. ). + internal static string[] ResolveProcessNamesToSearchCore(string msbuildLocation, string configuredNodeExeLocation) { - string currentHostPath = CurrentHost.GetCurrentHost(); - string currentHostName = currentHostPath != null - ? Path.GetFileNameWithoutExtension(currentHostPath) - : null; - - var names = new List(capacity: 2); + if (msbuildLocation != null) + { + return [GetProcessNameFromExecutablePath(msbuildLocation)]; + } - // The current host process (e.g. "dotnet"). - if (currentHostName != null) + string wouldLaunchPath = configuredNodeExeLocation; + if (string.IsNullOrEmpty(wouldLaunchPath)) { - names.Add(currentHostName); + wouldLaunchPath = BuildEnvironmentHelper.Instance.CurrentMSBuildExePath; } - // The MSBuild AppHost (e.g. "MSBuild"), if it differs from the current host. - if (!string.Equals(currentHostName, Constants.MSBuildAppName, StringComparison.OrdinalIgnoreCase)) +#if RUNTIME_TYPE_NETCORE + // Mirror the AppHost -> .dll remap GetNodes performs when launching from a dotnet host. + if (!string.IsNullOrEmpty(wouldLaunchPath) + && Path.GetFileName(wouldLaunchPath).Equals(Constants.MSBuildExecutableName, StringComparison.OrdinalIgnoreCase) + && string.Equals(Path.GetFileName(EnvironmentUtilities.ProcessPath), Constants.DotnetProcessName, StringComparison.OrdinalIgnoreCase)) { - names.Add(Constants.MSBuildAppName); + string dllPath = Path.Combine(Path.GetDirectoryName(wouldLaunchPath), Constants.MSBuildAssemblyName); + if (File.Exists(dllPath)) + { + wouldLaunchPath = dllPath; + } } +#endif + + string primary = !string.IsNullOrEmpty(wouldLaunchPath) + ? GetProcessNameFromExecutablePath(wouldLaunchPath) + : (Path.GetFileNameWithoutExtension(CurrentHost.GetCurrentHost()) ?? Constants.MSBuildAppName); - return names.ToArray(); + // Defensive fallback: include the alternate host so previously launched idle nodes + // started by the other host kind (AppHost vs dotnet) are also discovered. + string alternate = string.Equals(primary, Constants.MSBuildAppName, StringComparison.OrdinalIgnoreCase) + ? Path.GetFileNameWithoutExtension(CurrentHost.GetCurrentHost()) + : Constants.MSBuildAppName; + + return string.IsNullOrEmpty(alternate) || string.Equals(primary, alternate, StringComparison.OrdinalIgnoreCase) + ? [primary] + : [primary, alternate]; + } + + /// + /// Returns the process name that would be observed for a worker node launched from + /// : the AppHost executable name when the path points + /// at the AppHost, otherwise the host name (e.g. dotnet) that runs MSBuild.dll. + /// + private static string GetProcessNameFromExecutablePath(string msbuildLocation) + { + bool isAppHost = Path.GetFileName(msbuildLocation) + .Equals(Constants.MSBuildExecutableName, StringComparison.OrdinalIgnoreCase); + + return Path.GetFileNameWithoutExtension( + isAppHost ? msbuildLocation : (CurrentHost.GetCurrentHost() ?? msbuildLocation)); } /// From 94c1f61daecef143af03061f0f9d013d4239db41 Mon Sep 17 00:00:00 2001 From: Yuliia Kovalova Date: Wed, 22 Apr 2026 11:51:31 +0200 Subject: [PATCH 3/5] Extract RemapAppHostToManagedDllIfHostedByDotnet helper Deduplicates the AppHost->managed-DLL remap that was inlined in both GetNodes (launch path) and ResolveProcessNamesToSearchCore (shutdown path). Using a single helper guarantees the two stay in sync by construction, addressing review feedback to reuse instead of mirroring. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../NodeProviderOutOfProcBase.cs | 48 ++++++++++--------- 1 file changed, 26 insertions(+), 22 deletions(-) diff --git a/src/Build/BackEnd/Components/Communications/NodeProviderOutOfProcBase.cs b/src/Build/BackEnd/Components/Communications/NodeProviderOutOfProcBase.cs index 3668959cfd4..3418b93ab8c 100644 --- a/src/Build/BackEnd/Components/Communications/NodeProviderOutOfProcBase.cs +++ b/src/Build/BackEnd/Components/Communications/NodeProviderOutOfProcBase.cs @@ -239,19 +239,9 @@ protected IList GetNodes( // worker nodes are launched via dotnet.exe, matching the parent process. // This only applies to regular out-of-proc worker nodes (nodemode:1), not task host nodes // (nodemode:2) which may need the AppHost for COM host object support. - if (expectedNodeMode == NodeMode.OutOfProcNode - && !String.IsNullOrEmpty(msbuildLocation) - && Path.GetFileName(msbuildLocation).Equals(Constants.MSBuildExecutableName, StringComparison.OrdinalIgnoreCase)) + if (expectedNodeMode == NodeMode.OutOfProcNode) { - string currentProcessName = Path.GetFileName(EnvironmentUtilities.ProcessPath); - if (currentProcessName?.Equals(Constants.DotnetProcessName, StringComparison.OrdinalIgnoreCase) == true) - { - string dllPath = Path.Combine(Path.GetDirectoryName(msbuildLocation), Constants.MSBuildAssemblyName); - if (File.Exists(dllPath)) - { - msbuildLocation = dllPath; - } - } + msbuildLocation = RemapAppHostToManagedDllIfHostedByDotnet(msbuildLocation); } #endif @@ -555,16 +545,7 @@ internal static string[] ResolveProcessNamesToSearchCore(string msbuildLocation, #if RUNTIME_TYPE_NETCORE // Mirror the AppHost -> .dll remap GetNodes performs when launching from a dotnet host. - if (!string.IsNullOrEmpty(wouldLaunchPath) - && Path.GetFileName(wouldLaunchPath).Equals(Constants.MSBuildExecutableName, StringComparison.OrdinalIgnoreCase) - && string.Equals(Path.GetFileName(EnvironmentUtilities.ProcessPath), Constants.DotnetProcessName, StringComparison.OrdinalIgnoreCase)) - { - string dllPath = Path.Combine(Path.GetDirectoryName(wouldLaunchPath), Constants.MSBuildAssemblyName); - if (File.Exists(dllPath)) - { - wouldLaunchPath = dllPath; - } - } + wouldLaunchPath = RemapAppHostToManagedDllIfHostedByDotnet(wouldLaunchPath); #endif string primary = !string.IsNullOrEmpty(wouldLaunchPath) @@ -596,6 +577,29 @@ private static string GetProcessNameFromExecutablePath(string msbuildLocation) isAppHost ? msbuildLocation : (CurrentHost.GetCurrentHost() ?? msbuildLocation)); } +#if RUNTIME_TYPE_NETCORE + /// + /// When the current process is the dotnet CLI host and + /// points at the MSBuild AppHost (MSBuild.exe/MSBuild), returns the sibling + /// MSBuild.dll path so worker nodes are launched as dotnet MSBuild.dll rather + /// than via the slower AppHost. Otherwise returns unchanged. + /// Used by both (launch path) and + /// (shutdown path) so the two stay in sync by construction. + /// + private static string RemapAppHostToManagedDllIfHostedByDotnet(string msbuildLocation) + { + if (string.IsNullOrEmpty(msbuildLocation) + || !Path.GetFileName(msbuildLocation).Equals(Constants.MSBuildExecutableName, StringComparison.OrdinalIgnoreCase) + || !string.Equals(Path.GetFileName(EnvironmentUtilities.ProcessPath), Constants.DotnetProcessName, StringComparison.OrdinalIgnoreCase)) + { + return msbuildLocation; + } + + string dllPath = Path.Combine(Path.GetDirectoryName(msbuildLocation), Constants.MSBuildAssemblyName); + return File.Exists(dllPath) ? dllPath : msbuildLocation; + } +#endif + /// /// Filters candidate processes whose command-line NodeMode argument matches the expected value. /// Processes whose command line cannot be retrieved (unsupported platform) are included From 3959ad077989e2cdf40bf66161eb427e733a3b6c Mon Sep 17 00:00:00 2001 From: Yuliia Kovalova Date: Wed, 22 Apr 2026 15:21:52 +0200 Subject: [PATCH 4/5] Simplify resolver: collapse wrapper + helpers into one method The previous instance wrapper (ResolveProcessNamesToSearch) and static core (ResolveProcessNamesToSearchCore) only existed to bridge instance state into a unit-testable method. Replace both with a single internal static ResolveProcessNamesToSearch(msbuildLocation, configuredNodeExeLocation) and have GetPossibleRunningNodes pass the instance state directly. The tiny GetProcessNameFromExecutablePath helper is inlined as a local function since it has no other callers. No behavior change; tests updated for the rename. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../NodeProcessNameResolution_Tests.cs | 10 +- .../NodeProviderOutOfProcBase.cs | 102 ++++++++---------- 2 files changed, 47 insertions(+), 65 deletions(-) diff --git a/src/Build.UnitTests/BackEnd/NodeProcessNameResolution_Tests.cs b/src/Build.UnitTests/BackEnd/NodeProcessNameResolution_Tests.cs index 58a67a43e8d..5e75e1d4a00 100644 --- a/src/Build.UnitTests/BackEnd/NodeProcessNameResolution_Tests.cs +++ b/src/Build.UnitTests/BackEnd/NodeProcessNameResolution_Tests.cs @@ -39,7 +39,7 @@ private static void ShouldContainIgnoreCase(string[] names, string expected) => [Fact] public void ReuseBranch_AppHostPath_ReturnsAppHostName() { - string[] names = NodeProviderOutOfProcBase.ResolveProcessNamesToSearchCore( + string[] names = NodeProviderOutOfProcBase.ResolveProcessNamesToSearch( msbuildLocation: Path.Combine("c:", "tools", AppHostExeName), configuredNodeExeLocation: null); @@ -50,7 +50,7 @@ public void ReuseBranch_AppHostPath_ReturnsAppHostName() public void ReuseBranch_DllPath_ReturnsHostName() { // For a managed-assembly path the launcher uses the current host (e.g. "dotnet" on .NET Core). - string[] names = NodeProviderOutOfProcBase.ResolveProcessNamesToSearchCore( + string[] names = NodeProviderOutOfProcBase.ResolveProcessNamesToSearch( msbuildLocation: Path.Combine("c:", "tools", Constants.MSBuildAssemblyName), configuredNodeExeLocation: null); @@ -67,7 +67,7 @@ public void ReuseBranch_DllPath_ReturnsHostName() [Fact] public void ShutdownBranch_NoConfiguredLocation_AlwaysIncludesAppHostName() { - string[] names = NodeProviderOutOfProcBase.ResolveProcessNamesToSearchCore( + string[] names = NodeProviderOutOfProcBase.ResolveProcessNamesToSearch( msbuildLocation: null, configuredNodeExeLocation: null); @@ -82,7 +82,7 @@ public void ShutdownBranch_NoConfiguredLocation_AlwaysIncludesAppHostName() [Fact] public void ShutdownBranch_ConfiguredAppHostLocation_IncludesBothNames() { - string[] names = NodeProviderOutOfProcBase.ResolveProcessNamesToSearchCore( + string[] names = NodeProviderOutOfProcBase.ResolveProcessNamesToSearch( msbuildLocation: null, configuredNodeExeLocation: Path.Combine("c:", "tools", AppHostExeName)); @@ -101,7 +101,7 @@ public void ShutdownBranch_ConfiguredAppHostLocation_IncludesBothNames() [Fact] public void ShutdownBranch_NetCore_ConfiguredDllLocation_IncludesAppHostFallback() { - string[] names = NodeProviderOutOfProcBase.ResolveProcessNamesToSearchCore( + string[] names = NodeProviderOutOfProcBase.ResolveProcessNamesToSearch( msbuildLocation: null, configuredNodeExeLocation: Path.Combine("c:", "tools", Constants.MSBuildAssemblyName)); diff --git a/src/Build/BackEnd/Components/Communications/NodeProviderOutOfProcBase.cs b/src/Build/BackEnd/Components/Communications/NodeProviderOutOfProcBase.cs index 3418b93ab8c..938f6a92e62 100644 --- a/src/Build/BackEnd/Components/Communications/NodeProviderOutOfProcBase.cs +++ b/src/Build/BackEnd/Components/Communications/NodeProviderOutOfProcBase.cs @@ -462,7 +462,9 @@ void CreateNodeContext(int nodeId, Process nodeToReuse, Stream nodeStream, byte string msbuildLocation = null, NodeMode? expectedNodeMode = null) { - string[] processNamesToSearch = ResolveProcessNamesToSearch(msbuildLocation); + string[] processNamesToSearch = ResolveProcessNamesToSearch( + msbuildLocation, + _componentHost?.BuildParameters?.NodeExeLocation); ErrorUtilities.VerifyThrow(processNamesToSearch.Length > 0, "Expected at least one process name to search for."); string expectedProcessName = processNamesToSearch.Length == 1 @@ -495,65 +497,50 @@ void CreateNodeContext(int nodeId, Process nodeToReuse, Stream nodeStream, byte } /// - /// Determines which process names could host MSBuild worker nodes for the current operation. + /// Returns the candidate process names to search for when looking up worker nodes. /// + /// + /// The MSBuild executable that *will* be used to launch a worker on the node-reuse path, + /// or null when called from (no active build). + /// + /// + /// from the active + /// build, or null when no build is active. Parameter (rather than instance state) + /// so the resolver can be unit-tested in isolation. + /// /// - /// During node-reuse the caller supplies the exact that - /// would be used to launch a new node, so we return a single name derived from it. Behavior - /// of this branch is intentionally identical to the pre-fix code. + /// On the reuse path the caller already knows the launch location, so we return the single + /// process name that location would produce — preserving pre-fix behavior. /// - /// During the caller passes null because there is no - /// active build context. The previous implementation fell back to , - /// which always resolves to the *current* process host (e.g. dotnet) and therefore missed - /// idle worker nodes started via the MSBuild AppHost. We instead derive the path that *would* - /// be launched right now, mirroring the resolution logic in : - /// - /// - /// BuildParameters.NodeExeLocation if available, else BuildEnvironmentHelper.Instance.CurrentMSBuildExePath. - /// On .NET Core, when the path points at the AppHost and the current process is dotnet, - /// remap to MSBuild.dll (workers will be launched as dotnet MSBuild.dll). - /// - /// - /// We then add the *other* host name as a defensive fallback because previously launched - /// idle nodes may have been started by a different host kind (e.g. a prior MSBuild.exe - /// invocation followed by dotnet build-server shutdown). Extra candidates are safe: - /// they are validated downstream by handshake and process-owner checks. + /// On the shutdown path we mirror the resolution performs: + /// configuredNodeExeLocation ?? CurrentMSBuildExePath, with the .NET Core AppHost→.dll + /// remap applied. We additionally include the *other* host name as a defensive fallback to + /// catch idle nodes started by an earlier build that used a different host kind (e.g. + /// MSBuild.exe followed by dotnet build-server shutdown). Extra candidates + /// are harmless: they are validated downstream by handshake and process-owner checks. /// /// - private string[] ResolveProcessNamesToSearch(string msbuildLocation) - => ResolveProcessNamesToSearchCore(msbuildLocation, _componentHost?.BuildParameters?.NodeExeLocation); - - /// - /// Pure implementation extracted for unit testing. See . - /// - /// Caller-supplied MSBuild executable path (non-null on the - /// node-reuse path; null on the shutdown path). - /// BuildParameters.NodeExeLocation from the - /// active build, or null when no build is active (e.g. ). - internal static string[] ResolveProcessNamesToSearchCore(string msbuildLocation, string configuredNodeExeLocation) + internal static string[] ResolveProcessNamesToSearch(string msbuildLocation, string configuredNodeExeLocation) { + // Reuse path: the launch location is known, so only one process name is possible. if (msbuildLocation != null) { - return [GetProcessNameFromExecutablePath(msbuildLocation)]; + return [GetProcessNameForLocation(msbuildLocation)]; } - string wouldLaunchPath = configuredNodeExeLocation; - if (string.IsNullOrEmpty(wouldLaunchPath)) - { - wouldLaunchPath = BuildEnvironmentHelper.Instance.CurrentMSBuildExePath; - } + // Shutdown path: derive the path that *would* be launched right now. + string wouldLaunchPath = !string.IsNullOrEmpty(configuredNodeExeLocation) + ? configuredNodeExeLocation + : BuildEnvironmentHelper.Instance.CurrentMSBuildExePath; #if RUNTIME_TYPE_NETCORE - // Mirror the AppHost -> .dll remap GetNodes performs when launching from a dotnet host. wouldLaunchPath = RemapAppHostToManagedDllIfHostedByDotnet(wouldLaunchPath); #endif string primary = !string.IsNullOrEmpty(wouldLaunchPath) - ? GetProcessNameFromExecutablePath(wouldLaunchPath) + ? GetProcessNameForLocation(wouldLaunchPath) : (Path.GetFileNameWithoutExtension(CurrentHost.GetCurrentHost()) ?? Constants.MSBuildAppName); - // Defensive fallback: include the alternate host so previously launched idle nodes - // started by the other host kind (AppHost vs dotnet) are also discovered. string alternate = string.Equals(primary, Constants.MSBuildAppName, StringComparison.OrdinalIgnoreCase) ? Path.GetFileNameWithoutExtension(CurrentHost.GetCurrentHost()) : Constants.MSBuildAppName; @@ -561,30 +548,25 @@ internal static string[] ResolveProcessNamesToSearchCore(string msbuildLocation, return string.IsNullOrEmpty(alternate) || string.Equals(primary, alternate, StringComparison.OrdinalIgnoreCase) ? [primary] : [primary, alternate]; - } - /// - /// Returns the process name that would be observed for a worker node launched from - /// : the AppHost executable name when the path points - /// at the AppHost, otherwise the host name (e.g. dotnet) that runs MSBuild.dll. - /// - private static string GetProcessNameFromExecutablePath(string msbuildLocation) - { - bool isAppHost = Path.GetFileName(msbuildLocation) - .Equals(Constants.MSBuildExecutableName, StringComparison.OrdinalIgnoreCase); + // AppHost path -> "MSBuild"; managed DLL path -> current host name (e.g. "dotnet"). + static string GetProcessNameForLocation(string location) + { + bool isAppHost = Path.GetFileName(location) + .Equals(Constants.MSBuildExecutableName, StringComparison.OrdinalIgnoreCase); - return Path.GetFileNameWithoutExtension( - isAppHost ? msbuildLocation : (CurrentHost.GetCurrentHost() ?? msbuildLocation)); + return Path.GetFileNameWithoutExtension( + isAppHost ? location : (CurrentHost.GetCurrentHost() ?? location)); + } } #if RUNTIME_TYPE_NETCORE /// - /// When the current process is the dotnet CLI host and - /// points at the MSBuild AppHost (MSBuild.exe/MSBuild), returns the sibling - /// MSBuild.dll path so worker nodes are launched as dotnet MSBuild.dll rather - /// than via the slower AppHost. Otherwise returns unchanged. - /// Used by both (launch path) and - /// (shutdown path) so the two stay in sync by construction. + /// When the current process is dotnet and points at + /// the MSBuild AppHost, returns the sibling MSBuild.dll so workers are launched as + /// dotnet MSBuild.dll (faster than the AppHost). Otherwise returns the input unchanged. + /// Shared by (launch) and + /// (shutdown) so the two cannot drift. /// private static string RemapAppHostToManagedDllIfHostedByDotnet(string msbuildLocation) { From 643c1e0bdc5cebd123e371e20eca475f433fac49 Mon Sep 17 00:00:00 2001 From: Yuliia Kovalova Date: Wed, 22 Apr 2026 15:32:50 +0200 Subject: [PATCH 5/5] Simplify XML docs on resolver and tests Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../NodeProcessNameResolution_Tests.cs | 31 ++------------ .../NodeProviderOutOfProcBase.cs | 41 ++++++------------- 2 files changed, 15 insertions(+), 57 deletions(-) diff --git a/src/Build.UnitTests/BackEnd/NodeProcessNameResolution_Tests.cs b/src/Build.UnitTests/BackEnd/NodeProcessNameResolution_Tests.cs index 5e75e1d4a00..b1ba00a1a3e 100644 --- a/src/Build.UnitTests/BackEnd/NodeProcessNameResolution_Tests.cs +++ b/src/Build.UnitTests/BackEnd/NodeProcessNameResolution_Tests.cs @@ -13,20 +13,9 @@ namespace Microsoft.Build.Engine.UnitTests.BackEnd { /// - /// Unit tests for the process-name resolution used by - /// NodeProviderOutOfProcBase.GetPossibleRunningNodes. This is the surface area changed - /// by the fix for : - /// ShutdownAllNodes must look for both the host kind that *would* be launched right - /// now (e.g. dotnet) and the alternate AppHost name (MSBuild), so idle worker - /// nodes started by either host kind are discovered. + /// Tests for , the resolver + /// changed by the fix for https://github.com/dotnet/msbuild/issues/13508. /// - /// - /// A full end-to-end test of ShutdownAllNodes across host kinds is structurally - /// infeasible from a unit test: worker connect handshakes include the launcher's - /// MSBuildToolsDirectory32, so workers spawned by the bootstrapped MSBuild won't - /// answer to a shutdown call originating from the test assembly's MSBuild.dll. We therefore - /// validate the fix at the resolver layer where the bug actually lived. - /// public class NodeProcessNameResolution_Tests { private const string AppHostName = "MSBuild"; // Constants.MSBuildAppName @@ -58,12 +47,7 @@ public void ReuseBranch_DllPath_ReturnsHostName() names[0].ShouldNotBeNullOrEmpty(); } - /// - /// Regression for : - /// when shutdown is invoked with no active build (configuredNodeExeLocation == null), - /// the resolver must include the MSBuild AppHost name so that idle worker nodes - /// launched as MSBuild.exe are discovered, regardless of the current host kind. - /// + // Regression for https://github.com/dotnet/msbuild/issues/13508. [Fact] public void ShutdownBranch_NoConfiguredLocation_AlwaysIncludesAppHostName() { @@ -74,11 +58,6 @@ public void ShutdownBranch_NoConfiguredLocation_AlwaysIncludesAppHostName() ShouldContainIgnoreCase(names, AppHostName); } - /// - /// When the active build's - /// is the AppHost, the resolver must still include both the AppHost name and the alternate - /// host name so a defensive shutdown still finds nodes launched by the other host kind. - /// [Fact] public void ShutdownBranch_ConfiguredAppHostLocation_IncludesBothNames() { @@ -94,10 +73,6 @@ public void ShutdownBranch_ConfiguredAppHostLocation_IncludesBothNames() } #if NET - /// - /// On .NET Core, when the configured location is a managed assembly (DLL), the resolver - /// returns the current host (e.g. "dotnet") plus the AppHost name as defensive fallback. - /// [Fact] public void ShutdownBranch_NetCore_ConfiguredDllLocation_IncludesAppHostFallback() { diff --git a/src/Build/BackEnd/Components/Communications/NodeProviderOutOfProcBase.cs b/src/Build/BackEnd/Components/Communications/NodeProviderOutOfProcBase.cs index 938f6a92e62..fc96ed6e8ac 100644 --- a/src/Build/BackEnd/Components/Communications/NodeProviderOutOfProcBase.cs +++ b/src/Build/BackEnd/Components/Communications/NodeProviderOutOfProcBase.cs @@ -497,38 +497,22 @@ void CreateNodeContext(int nodeId, Process nodeToReuse, Stream nodeStream, byte } /// - /// Returns the candidate process names to search for when looking up worker nodes. - /// - /// - /// The MSBuild executable that *will* be used to launch a worker on the node-reuse path, - /// or null when called from (no active build). - /// - /// - /// from the active - /// build, or null when no build is active. Parameter (rather than instance state) + /// Returns the candidate process names to search for when locating worker nodes. + /// On the reuse path ( non-null) returns the single name + /// that location would produce. On the shutdown path ( null) + /// derives the name from or the current MSBuild + /// path — mirroring — and adds the alternate host as a defensive + /// fallback for idle nodes started by an earlier build under a different host kind. + /// is a parameter (rather than instance state) /// so the resolver can be unit-tested in isolation. - /// - /// - /// On the reuse path the caller already knows the launch location, so we return the single - /// process name that location would produce — preserving pre-fix behavior. - /// - /// On the shutdown path we mirror the resolution performs: - /// configuredNodeExeLocation ?? CurrentMSBuildExePath, with the .NET Core AppHost→.dll - /// remap applied. We additionally include the *other* host name as a defensive fallback to - /// catch idle nodes started by an earlier build that used a different host kind (e.g. - /// MSBuild.exe followed by dotnet build-server shutdown). Extra candidates - /// are harmless: they are validated downstream by handshake and process-owner checks. - /// - /// + /// internal static string[] ResolveProcessNamesToSearch(string msbuildLocation, string configuredNodeExeLocation) { - // Reuse path: the launch location is known, so only one process name is possible. if (msbuildLocation != null) { return [GetProcessNameForLocation(msbuildLocation)]; } - // Shutdown path: derive the path that *would* be launched right now. string wouldLaunchPath = !string.IsNullOrEmpty(configuredNodeExeLocation) ? configuredNodeExeLocation : BuildEnvironmentHelper.Instance.CurrentMSBuildExePath; @@ -562,11 +546,10 @@ static string GetProcessNameForLocation(string location) #if RUNTIME_TYPE_NETCORE /// - /// When the current process is dotnet and points at - /// the MSBuild AppHost, returns the sibling MSBuild.dll so workers are launched as - /// dotnet MSBuild.dll (faster than the AppHost). Otherwise returns the input unchanged. - /// Shared by (launch) and - /// (shutdown) so the two cannot drift. + /// When the current process is dotnet and points + /// at the MSBuild AppHost, returns the sibling MSBuild.dll (so workers launch via + /// dotnet MSBuild.dll instead of the slower AppHost). Otherwise returns the input + /// unchanged. Shared by and . /// private static string RemapAppHostToManagedDllIfHostedByDotnet(string msbuildLocation) {