Skip to content

Look for apphost when considering node reuse#13368

Merged
rainersigwald merged 2 commits intodotnet:mainfrom
rainersigwald:reap-msbuild-instead-of-dotnet
Mar 11, 2026
Merged

Look for apphost when considering node reuse#13368
rainersigwald merged 2 commits intodotnet:mainfrom
rainersigwald:reap-msbuild-instead-of-dotnet

Conversation

@rainersigwald
Copy link
Member

@rainersigwald rainersigwald commented Mar 11, 2026

There was a collision between #13220 (at end of build, look for other
running MSBuild processes and decide whether node reuse is likely to be
worthwhile) and #13175 (use an apphost on core). This broke the
overprovisioning detection, which was looking for dotnet instead
of the new MSBuild.

Always search for MSBuild instead, and remove the now-unnecesary
"try to filter dotnet.exe processes to those running MSBuild.dll
code too.

There was a collision between dotnet#13220 (at end of build, look for other
running MSBuild processes and decide whether node reuse is likely to be
worthwhile) and dotnet#13175 (use an apphost on core). This broke the
overprovisioning detection, which was looking for `dotnet[.exe]` instead
of the new `MSBuild[.exe]`.

Always search for `MSBuild[.exe]` instead, and remove the now-unnecesary
"try to filter `dotnet.exe` processes to those running `MSBuild.dll`
code too.
Copilot AI review requested due to automatic review settings March 11, 2026 17:19
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adjusts system-wide node detection to account for the MSBuild apphost so over-provisioning detection/node reuse decisions look for MSBuild processes instead of dotnet.

Changes:

  • Switch expected process name for node enumeration to MSBuild.
  • Remove dotnet-specific command-line filtering intended to detect MSBuild.dll under dotnet.
Comments suppressed due to low confidence (2)

src/Build/BackEnd/Components/Communications/NodeProviderOutOfProcBase.cs:644

  • There are unit tests for the over-provisioning logic (NodeProviderOutOfProc_Tests), but they override CountSystemWideActiveNodes, so this GetProcessesByName-based enumeration path isn't covered. Consider adding a test that would fail if an extension (".exe") is accidentally passed into GetProcessesByName on Windows (e.g., by asserting the computed expected process name is extensionless).
            var expectedProcessName = Constants.MSBuildExecutableName;

            Process[] processes;
            try
            {
                processes = Process.GetProcessesByName(expectedProcessName);
            }

src/Build/BackEnd/Components/Communications/NodeProviderOutOfProcBase.cs:644

  • Process.GetProcessesByName expects the process name without an extension. Constants.MSBuildExecutableName is MSBuild.exe on Windows, so this will likely return an empty set on Windows and break CountActiveNodesWithMode (server-node self-termination / over-provisioning detection). Use an extensionless name here (e.g., Path.GetFileNameWithoutExtension(Constants.MSBuildExecutableName) or Constants.MSBuildAppName).
            var expectedProcessName = Constants.MSBuildExecutableName;

            Process[] processes;
            try
            {
                processes = Process.GetProcessesByName(expectedProcessName);
            }

@rainersigwald rainersigwald self-assigned this Mar 11, 2026
@rainersigwald rainersigwald merged commit 8cee1da into dotnet:main Mar 11, 2026
10 checks passed
@rainersigwald rainersigwald deleted the reap-msbuild-instead-of-dotnet branch March 11, 2026 22:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants