Skip to content

sidecar-taskhost-attempt-2#12145

Merged
SimaTian merged 11 commits intomainfrom
sidecar-taskhost-2
Aug 20, 2025
Merged

sidecar-taskhost-attempt-2#12145
SimaTian merged 11 commits intomainfrom
sidecar-taskhost-2

Conversation

@SimaTian
Copy link
Member

Fixes #12071

Second attempt at 12071.
First one failed due to VS insertion Perf DDRITs.

@ghost
Copy link

ghost commented Jul 11, 2025

Relevant to #11335

@ghost ghost assigned SimaTian Jul 14, 2025
@SimaTian SimaTian marked this pull request as ready for review August 4, 2025 08:43
Copilot AI review requested due to automatic review settings August 4, 2025 08:43
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

This PR implements a "sidecar TaskHost" feature to address issue #12071, providing an option to run long-lived TaskHost processes that persist after builds complete to reduce startup overhead and reuse in-memory caches.

  • Converts handshake operations from exception-based to result-based error handling
  • Adds HandshakeResult type and SidecarTaskHost handshake option to support error propagation
  • Implements logic to differentiate between transient TaskHostFactory and persistent sidecar TaskHost nodes

Reviewed Changes

Copilot reviewed 19 out of 19 changed files in this pull request and generated 8 comments.

Show a summary per file
File Description
CommunicationsUtilities.cs Adds HandshakeResult type and converts handshake methods to return results instead of throwing exceptions
OutOfProcRarClient.cs Updates connection logic to handle HandshakeResult return values
NodePipeServer.cs Converts handshake validation to use result-based error handling
NodePipeClient.cs Updates connection and handshake methods to return HandshakeResult
OutOfProcTaskHostNode.cs Adds nodeReuse parameter and sidecar TaskHost lifecycle management
TaskHostTask.cs Tracks whether TaskHostFactory was explicitly requested to control node reuse behavior
AssemblyTaskFactory.cs Passes TaskHostFactory flag to TaskHostTask for proper node lifecycle management
MSBuildClient.cs Updates connection logic to use HandshakeResult instead of exceptions

@SimaTian
Copy link
Member Author

SimaTian commented Aug 4, 2025

Once again ready for review, although I still need to dig through the code and tests to see what was causing some weird behavior in VS tests:

  • one of the tests monitors handled exception counts as a marker for weird behavior and potential perf issues.
  • technically, this change shouldn't change this count, however there were some additional exceptions being thrown in one of the tests.
  • with that in mind, I refactored the code in question to stop using exceptions as a way to pass information in our code
    • this solved the issue at hand so the tests now pass.
    • however we still don't know what the original cause of these being thrown was in the first place, hence the investigation.

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.

thank you for addressing the comments!

Copy link
Member

@JanProvaznik JanProvaznik left a comment

Choose a reason for hiding this comment

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

mostly style comments but the tests being skipped seems concerning; functionally it looks good

@SimaTian SimaTian merged commit e39ece4 into main Aug 20, 2025
9 checks passed
@SimaTian SimaTian deleted the sidecar-taskhost-2 branch August 20, 2025 08:48
@YuliiaKovalova YuliiaKovalova restored the sidecar-taskhost-2 branch August 25, 2025 15:56
@YuliiaKovalova YuliiaKovalova deleted the sidecar-taskhost-2 branch August 26, 2025 08:07
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