Skip to content

Sidecar taskhost #12071

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 25 commits into from
Jul 8, 2025
Merged

Sidecar taskhost #12071

merged 25 commits into from
Jul 8, 2025

Conversation

SimaTian
Copy link
Member

@SimaTian SimaTian commented Jun 25, 2025

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 Copilot AI review requested due to automatic review settings June 25, 2025 15:41
Copy link
Contributor

@Copilot 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

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

@AR-May AR-May self-requested a review June 27, 2025 15:54
Copy link
Contributor

@kasperk81 kasperk81 left a comment

Choose a reason for hiding this comment

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

few cleanups

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>
Copy link
Member

@AR-May AR-May left a comment

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.

@YuliiaKovalova
Copy link
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.

Copy link
Member

@YuliiaKovalova YuliiaKovalova left a comment

Choose a reason for hiding this comment

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

Good job!

Copy link
Member

@AR-May AR-May left a comment

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!

@SimaTian SimaTian merged commit 0e91e45 into main Jul 8, 2025
9 checks passed
@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
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
4 participants