Improve cross-platform node discovery for reuse with NodeMode filtering#13256
Improve cross-platform node discovery for reuse with NodeMode filtering#13256
Conversation
src/Build/BackEnd/Components/Communications/NodeProviderOutOfProcBase.cs
Outdated
Show resolved
Hide resolved
src/Build/BackEnd/Components/Communications/NodeProviderOutOfProcBase.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
This pull request implements cross-platform MSBuild node discovery improvements to enable better node reuse. The key change is extending node discovery beyond just Process.GetProcessesByName() to also inspect dotnet processes hosting MSBuild.dll and filter candidates by NodeMode type, ensuring worker nodes only reuse worker nodes and task host nodes only reuse task host nodes.
Changes:
- Added cross-platform
ProcessExtensions.GetCommandLine()to retrieve process command lines via WMI (Windows/.NET Framework), native Windows APIs (Windows/.NET Core+), /proc filesystem (Linux), and sysctl (macOS) - Enhanced
GetPossibleRunningNodes()to discover both msbuild.exe and dotnet processes, filter by MSBuild.dll presence, and validate/filter by NodeMode using regex extraction - Added NodeMode helper methods
ToCommandLineArgument()andTryParse()for consistent command line formatting and parsing
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Shared/ProcessExtensions.cs | Implements cross-platform command line retrieval with platform-specific implementations for Windows (WMI + native APIs), Linux (/proc), and macOS (sysctl) |
| src/Utilities/Microsoft.Build.Utilities.csproj | Adds System.Management reference for .NET Framework to support WMI-based command line retrieval |
| src/Build/Microsoft.Build.csproj | Adds System.Management reference for .NET Framework in Build project |
| src/Build/BackEnd/Components/Communications/NodeProviderOutOfProcBase.cs | Enhances node discovery to search both msbuild.exe and dotnet processes, extracts NodeMode via regex, and filters candidates by matching NodeMode values |
| src/Framework.UnitTests/NodeMode_Tests.cs | Adds comprehensive unit tests for NodeMode command line formatting and parsing (integers, enum names, invalid values) |
| src/Utilities.UnitTests/ProcessExtensions_Tests.cs | Adds tests for GetCommandLine() including null process, running process, exited process, and Unix-specific scenarios |
src/Build/BackEnd/Components/Communications/NodeProviderOutOfProcBase.cs
Outdated
Show resolved
Hide resolved
src/Build/BackEnd/Components/Communications/NodeProviderOutOfProcBase.cs
Outdated
Show resolved
Hide resolved
src/Build/BackEnd/Components/Communications/NodeProviderOutOfProcBase.cs
Show resolved
Hide resolved
src/Build/BackEnd/Components/Communications/NodeProviderOutOfProcBase.cs
Outdated
Show resolved
Hide resolved
src/Build/BackEnd/Components/Communications/NodeProviderOutOfProcBase.cs
Outdated
Show resolved
Hide resolved
src/Build/BackEnd/Components/Communications/NodeProviderOutOfProcBase.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: baronfel <573979+baronfel@users.noreply.github.com>
Head branch was pushed to by a user without write access
I can see that WMI COM interop was recently added for Windows command line retrieval (commit e29eb81). WMI can be unreliable in test environments due to COM initialization issues, security contexts, and concurrency. Could you provide more details about the test failures? Specifically:
The tests pass consistently on Linux, so this is Windows-specific. I suspect the WMI COM approach may need additional error handling or we might need to consider an alternative approach. |
…n usage Co-authored-by: baronfel <573979+baronfel@users.noreply.github.com>
src/Build/BackEnd/Components/Communications/NodeProviderOutOfProcBase.cs
Show resolved
Hide resolved
src/Build/BackEnd/Components/Communications/NodeProviderOutOfProcBase.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: baronfel <573979+baronfel@users.noreply.github.com>
Summary
Improves MSBuild node discovery to enable better node reuse by filtering candidate processes by NodeMode type. This ensures worker nodes only reuse worker nodes and task host nodes only reuse task host nodes, preventing incompatible node reuse.
Customer Impact
Positive Impact:
Opt-out Mechanism:
MSBUILDDISABLEFEATURESFROMVERSION=18.6if issues ariseRegression?
This is new functionality for improved node discovery. The feature is opt-in via change wave and includes defensive error handling:
Testing
Unit Tests:
ProcessExtensions.TryGetCommandLine()tests covering Windows, Linux, and macOSKillTreetest validates process tree termination functionalityCode Quality:
ValueSpanon .NET to avoid string allocation when parsing NodeMode from command linesArrayPool<byte>.Sharedfor buffer management (no native allocations)ArrayPool<char>.Sharedfor UTF-8 decoding (avoids intermediate string allocations)MemoryMarshal.Read<int>for reading argcMacOSOnlyFactAttributefromMicrosoft.DotNet.XUnitExtensionsImplementation Details:
Process command line retrieval (
ProcessExtensions.cs)/proc/{pid}/cmdlinewith UTF-8 encoding, fixed TOCTOU issue(CTL_KERN, KERN_PROCARGS2, pid), uses ArrayPool for efficient memory management, properly handles padding nulls between executable path and argumentsEnhanced node discovery (
NodeProviderOutOfProcBase.cs)expectedProcessName(maintains framework vs core separation)expectedProcessNameis dotnet, filters by MSBuild.dll presence in command line/nodemode:<value>parameter usingNodeModeHelper.ExtractFromCommandLine()(consistent withDebugUtils)expectedNodeModeis providedConstants.DotnetProcessNamefor cross-platform dotnet detectionCommunicationsUtilities.Trace()at every decision point for debuggingNodeMode helpers (
NodeMode.cs)ToCommandLineArgument(): Format as/nodemode:{value}TryParse(): Parse integer or enum name (case-insensitive), supports bothstringandReadOnlySpan<char>overloadsExtractFromCommandLine(): Extract NodeMode from command line using regex (with source generator for .NET 10+, static compiled regex for .NET Framework)Risk
Risk Mitigation:
ValueSpanon .NET to avoid string allocation during parsing, ArrayPool for buffer managementOriginal prompt
This pull request was created from Copilot chat.
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.