Skip to content

Sidecar taskhost - #12071

Merged
SimaTian merged 25 commits into
mainfrom
sidecar-taskhost
Jul 8, 2025
Merged

SimaTian merged 25 commits into
mainfrom
sidecar-taskhost

Conversation

@SimaTian

@SimaTian SimaTian commented Jun 25, 2025

Copy link
Copy Markdown
Contributor

Fixes #11335

Context

We already had TaskHost live alongside the build. It would get cleaned up when the build is done.
This is some additional plumbing to allow for TaskHost to live even after the build is done.
Now we have two cases:

  • when TaskHost is requested via TaskHostFactory, we assume that it is required for it to release the .dlls after the build is done and so we use the old build-lived TaskHost
  • when TaskHost is requested via MSBUILDFORCEALLTASKSOUTOFPROC (or any other way, something we need to keep in mind going forward), it will spawn a long-lived taskHost node that will live until it timeouts or is killed.

Changes Made

Added plumbing to allow for long lived TaskHost spawning.

Testing

TODO

Notes

  • before merging this one, we should merge Yuliias one first
    Support launching net taskhost - initial implementation #11393
  • however since it was confusing to parse out my commits when mixed with hers, I based on main in the end to make review easier.
    • secondary note, there might be some non-trivial conflict since one of the tests kept dying when based on her PR after merging. Currently it appears to be working.
  • Naming in Taskhost is confusing due to the way it references explicit factory using. If anyone has a good idea how to rename things to consolidate them, please let me know.

Copilot AI review requested due to automatic review settings June 25, 2025 15:41

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds plumbing to support long‐lived TaskHost processes by distinguishing between build‐lived and long‐lived task hosts. The changes refactor handshake serialization to use a structured HandshakeComponents type, update constants and environment retrieval for MSBuild paths, and adjust logging and process‐launch logic accordingly.

  • Introduce the _isNetRuntime flag in TaskHostConfiguration and update its constructor API.
  • Replace raw int arrays with a structured HandshakeComponents type and update related handshake loops.
  • Update constants, environment variable use, and process name references across the codebase for improved consistency.

Reviewed Changes

Copilot reviewed 20 out of 22 changed files in this pull request and generated no comments.

Show a summary per file
File Description
src/UnitTests.Shared/RunnerUtilities.cs Use Constants.MSBuildAssemblyName for MSBuild assembly lookup.
src/Shared/TaskHostConfiguration.cs Introduce _isNetRuntime field and update constructor comments and parameters.
src/Shared/NodePipeServer.cs Switch from a for loop to a foreach loop (with manual index) when processing handshake components.
src/Shared/NodePipeClient.cs Update handshake serialization to use component enumeration.
src/Shared/NodePipeBase.cs Change HandshakeComponents property type from int[] to a structured type.
src/Shared/NodeEndpointOutOfProcBase.cs Add IsHandshakePartValid and related handshake improvements.
src/Shared/CommunicationsUtilities.cs Refactor handshake construction and update constants handling.
src/Shared/BuildEnvironmentHelper.cs Add NET-specific directory properties for tools and assemblies.
src/MSBuild/MSBuild/Microsoft.Build.Core.xsd Update runtime attribute documentation and enumeration to include NET.
src/Build/Resources/Constants.cs Introduce new constants for dotnet process names and MSBuild executable/assembly names.
src/Build/Instance/TaskFactories/TaskHostTask.cs Update TaskHost spawning logic and error logging naming.
src/Build/Evaluation/IntrinsicFunctions.cs Improve dictionary initialization and task host existence check.
src/Build/BackEnd/** Various updates addressing node launching, handshake logic, and process name consistency.
.editorconfig Minor header/BOM change.
Comments suppressed due to low confidence (2)

src/Build/BackEnd/TaskExecutionHost/TaskExecutionHost.cs:340

  • The typo 'realTaskAssemblyLoaction' has been corrected to 'realTaskAssemblyLocation' for improved clarity.
            string realTaskAssemblyLocation = TaskInstance.GetType().Assembly.Location;

.editorconfig:1

  • [nitpick] Ensure that the file encoding (with BOM) is intentional and consistent with the project’s standards.
# editorconfig.org

Comment thread src/Shared/CommunicationsUtilities.cs Outdated
Comment thread src/Build/Instance/TaskFactories/AssemblyTaskFactory.cs Outdated
@AR-May
AR-May self-requested a review June 27, 2025 15:54

@kasperk81 kasperk81 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

few cleanups

Comment thread src/Build.UnitTests/BackEnd/TaskHostFactory_Tests.cs Outdated
Comment thread src/Build.UnitTests/BackEnd/TaskHostFactory_Tests.cs
Comment thread src/Build/Instance/ProjectInstance.cs Outdated
SimaTian and others added 3 commits June 30, 2025 10:11
Co-authored-by: kasperk81 <83082615+kasperk81@users.noreply.github.com>
…rocTaskHost.cs

Co-authored-by: kasperk81 <83082615+kasperk81@users.noreply.github.com>
Co-authored-by: kasperk81 <83082615+kasperk81@users.noreply.github.com>

@AR-May AR-May left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Overall, looks good to me, I have a number of small suggestions.
I think we need to add a bit more testing to cover the situation when two different task hosts nodes are called and used simultaneously: the sidecar and the explicitly requested one.

Comment thread src/Shared/CommunicationsUtilities.cs Outdated
Comment thread src/MSBuild/NodeEndpointOutOfProcTaskHost.cs Outdated
Comment thread src/Framework/Traits.cs Outdated
Comment thread src/Build/Instance/TaskRegistry.cs
Comment thread src/Build/Instance/TaskFactories/TaskHostTask.cs Outdated
Comment thread src/Build/Instance/TaskFactories/TaskHostTask.cs Outdated
Comment thread src/Shared/CommunicationsUtilities.cs Outdated
Comment thread src/Build.UnitTests/BackEnd/TaskHostFactory_Tests.cs Outdated
Comment thread src/Shared/CommunicationsUtilities.cs Outdated
Comment thread src/Framework/Traits.cs Outdated
Comment thread src/Shared/CommunicationsUtilities.cs Outdated
Comment thread src/Framework/Traits.cs Outdated
Comment thread src/Build/Instance/TaskFactories/TaskHostTask.cs
Comment thread src/Build.UnitTests/BackEnd/AssemblyTaskFactory_Tests.cs Outdated
Comment thread src/Build.UnitTests/BackEnd/TaskHostFactory_Tests.cs Outdated
Comment thread src/Shared/CommunicationsUtilities.cs
@YuliiaKovalova

Copy link
Copy Markdown
Member

I'm thinking if we can place value from
bool isTaskHostFactory = String.Equals(TaskFactoryAttributeName, TaskHostFactory, StringComparison.OrdinalIgnoreCase);

to existing TaskFactoryParameters dictionary.
Later it is propagated right to TaskHostTask.
We can read this value in TaskHostTask from dictionary and avoid modification of this chain of constructors.

Comment thread src/Build/Instance/TaskRegistry.cs Outdated
Comment thread src/Build/Instance/TaskRegistry.cs Outdated
Comment thread src/Build/Instance/TaskFactories/AssemblyTaskFactory.cs Outdated
Comment thread src/Build.UnitTests/BackEnd/AssemblyTaskFactory_Tests.cs Outdated
Comment thread src/Build.UnitTests/BackEnd/TaskHostFactory_Tests.cs Outdated
Comment thread src/Build/Instance/TaskFactories/AssemblyTaskFactory.cs Outdated
Comment thread src/Build/Instance/TaskRegistry.cs Outdated
Comment thread src/Build.UnitTests/BackEnd/AssemblyTaskFactory_Tests.cs Outdated
Comment thread src/MSBuild/OutOfProcTaskHostNode.cs Outdated

@YuliiaKovalova YuliiaKovalova left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Good job!

@AR-May AR-May left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good to me, thank you!

Comment thread src/Framework/Traits.cs
Comment thread src/MSBuild/OutOfProcTaskHostNode.cs Outdated
Comment thread src/Build.UnitTests/BackEnd/TaskHostFactory_Tests.cs Outdated
Comment thread src/Build.UnitTests/BackEnd/TaskHostFactory_Tests.cs Outdated
@SimaTian
SimaTian merged commit 0e91e45 into main Jul 8, 2025
@SimaTian
SimaTian deleted the sidecar-taskhost branch July 8, 2025 12:10
YuliiaKovalova added a commit that referenced this pull request Jul 9, 2025
YuliiaKovalova added a commit that referenced this pull request Jul 9, 2025
akoeplinger pushed a commit that referenced this pull request Jul 18, 2025
This reverts commit 0e91e45.

(cherry picked from commit 62481da)
SimaTian added a commit that referenced this pull request Aug 20, 2025
Fixes #12071 

Second attempt at 12071.
First one failed due to VS insertion Perf DDRITs.
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.

Add long-running "sidecar" TaskHosts

5 participants