Give each transient MSBuild Server its own identity - #14647
Draft
JanProvaznik wants to merge 12 commits into
Draft
Give each transient MSBuild Server its own identity#14647JanProvaznik wants to merge 12 commits into
JanProvaznik wants to merge 12 commits into
Conversation
Fixes dotnet#14313. A reusable sidecar TaskHost disconnected at the end of a build and went back to listening on its per-PID pipe, joining a machine-wide pool of reconnectable nodes. Nothing owned it afterwards. Shutdown could not reach it. BuildManager.ShutdownAllNodes only drove the worker node manager, and NodeProviderOutOfProcBase.ShutdownAllNodes discovers nodes by scanning for processes named MSBuildTaskHost.exe using the worker handshake, so a sidecar hosted by dotnet was invisible to it. `dotnet build-server shutdown` therefore left sidecars running with no owner and no way to be found again. Make a sidecar owned by the process that launched it, with no new wire protocol: the sidecar/short-lived distinction that already exists is the ownership signal. - The sidecar keeps its connection across builds. On a reusable NodeBuildComplete it disposes build-lifetime state, resets in place, and acknowledges, instead of disconnecting and relistening. - The provider retains the NodeContext and tracks active separately from idle, so a retained idle sidecar is not mistaken for one serving a build. - Shutdown walks those retained connections directly. No process enumeration, so a sidecar is reached regardless of the process name of the runtime hosting it. - Losing the owner connection terminates the sidecar rather than returning it to the pool, which covers owners that die abruptly. Ownership is not specific to the server. When the server is disabled the owner is the ordinary command-line MSBuild process, and the sidecar dies with it. The user-visible consequence is that a sidecar no longer survives the process that created it, so consecutive invocations without a server each start their own. Reuse within one owner's lifetime is unchanged. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8ecd77e2-5945-45e3-b885-befa4a23d37a
Three problems, found by reproducing CI on Linux via WSL and by reading the published CI test results. BuildManager.ShutdownAllNodes created the task host node manager just to shut it down, and NodeProviderOutOfProcTaskHost.ShutdownAllNodes then dereferenced ComponentHost.BuildParameters, which is null when no build has run through that BuildManager. CanShutdownServerProcess(byBuildManager: true) does exactly that. Only shut down the manager when a build created it, matching how the two node managers are already shut down in pairs, and guard BuildParameters the same way NodeProviderOutOfProc.ShutdownAllNodes does. SidecarDoesNotOutliveANonServerBuild launched MSBuild without the bootstrap environment, so the sidecar could not be launched at all and node creation waited out its timeout. Use ExecBootstrapedMSBuild, which applies that environment, as its documentation recommends for exactly this case. ServerOwnsReusableSidecarsUntilShutdown asserted that the sidecar process ID is identical across two builds on the same server. That holds on Linux but not on Windows, where the second build gets a new sidecar. Cross-build sidecar reuse is an optimization rather than the guarantee this change is about, so assert the guarantee instead: every sidecar the server created is gone once the server is shut down, including one from an earlier build that was not reused. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8ecd77e2-5945-45e3-b885-befa4a23d37a
CI disables node reuse by default (eng/common/build.ps1 sets -nodeReuse:false unless MSBUILD_NODEREUSE_ENABLED=1), and BuildParameters.EnableNodeReuse honours MSBUILDDISABLENODEREUSE. TaskHostTask only creates a sidecar when node reuse is on, so under CI both tests silently degraded into tests of short-lived TaskHosts, which exit on their own and never exercise ownership. That is also what the earlier CI failure was: the two builds reported different TaskHost process IDs because each build got a fresh short-lived TaskHost rather than reusing a sidecar. Reproduced locally by setting MSBUILDDISABLENODEREUSE=1, which turns a stable sidecar PID across two builds into two different PIDs. Clear MSBUILDDISABLENODEREUSE and pass -nodeReuse:true so a real sidecar is created, and restore the assertion that the server reuses the sidecar it already owns. Drive both builds and the shutdown through the bootstrap installation. The in-process MSBuildClient.ShutdownServer derives its handshake from the currently running MSBuild, which is a different installation than the bootstrap that served the builds, so it cannot find that server. Add RunnerUtilities.BootstrapDotnetHostPath so a test can run build-server shutdown against the same installation it built with. Verified on Windows and Linux: the sidecar is reused across both builds and both the server and the sidecar are gone after shutdown. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8ecd77e2-5945-45e3-b885-befa4a23d37a
CI reuses the server across the two builds but not the TaskHost: it reported the same server process and two different TaskHost processes, so the assertion that the sidecar is reused failed there while passing locally on Windows and Linux. Whether a reusable sidecar or a short-lived TaskHost is used depends on how node reuse resolves in the environment, and that is not the guarantee this change makes. Assert the guarantee instead: after build-server shutdown, neither the server nor any TaskHost process it used is still running. That holds whichever kind of TaskHost the build used, and still fails if an owned sidecar is left behind, which is the regression worth catching. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8ecd77e2-5945-45e3-b885-befa4a23d37a
An owned sidecar now replies in place instead of disconnecting, so the acknowledgement packet is the only thing that ends the wait for it. If a sidecar dies or wedges before acknowledging, an unbounded wait hangs the owner: at the end of a build in ShutdownConnectedNodes, and during shutdown in ShutdownAllNodes, where the set can also include nodes that were idle rather than serving a build and whose process may already be gone. Bound both waits with the same 30s timeout the node provider already uses for waiting on node exit. Shutdown proceeds regardless, and node contexts are torn down when the owner exits. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8ecd77e2-5945-45e3-b885-befa4a23d37a
The shutdown cascade routed through ShutdownAllNodes, which shuts down owned TaskHosts and then also scans the machine for unrelated pooled ones, connecting to every candidate process it finds. Running that scan while a server is exiting delays its shutdown, and the point of ownership is that owned nodes are reachable over connections already held, with no enumeration at all. That delay is the likely reason ServerShouldStartWhenBuildIsInteractive began failing on this branch in the Windows Full Release leg while passing on the handshake-only PR: the previous test's server was still shutting down and holding the running-server mutex, so the next build found no usable server and ran in process, leaving no server process for the test to inspect. Split the two: ShutdownOwnedNodes shuts down only what this process owns, over its existing connections, and ShutdownAllNodes keeps its previous meaning of owned nodes plus the machine-wide scan for the public BuildManager API. The server cascade now uses the former. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8ecd77e2-5945-45e3-b885-befa4a23d37a
BuildManager.ShutdownAllNodes drove the TaskHost node manager's ShutdownAllNodes, which shuts down owned TaskHosts and then runs the machine-wide scan in NodeProviderOutOfProcBase. That scan is shared with the worker provider and already searches both MSBuild worker processes and MSBuildTaskHost.exe, so the line above it had just performed exactly the same scan. Only the owned half is needed here. An owned sidecar cannot outlive its owner, so there is never a stale one on the machine for a later shutdown to discover, and legacy pooled TaskHosts are already covered by the worker provider's scan. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8ecd77e2-5945-45e3-b885-befa4a23d37a
The change carried machinery that measurement showed was not needed. An explicit shutdown cascade from the server, and a matching path through BuildManager.ShutdownAllNodes, existed to reap sidecars when their owner exits. They are redundant: the orphan in dotnet#14313 exists only because a sidecar disconnects and relistens with nobody attached, and once it stays connected its owner exiting breaks the pipe, which already terminates the child. Both sidecar tests pass with the cascade removed, so the ownership guarantee now rests on one mechanism that covers the server and non-server cases identically instead of two that overlap. Also drops naming invented for this change. A sidecar is exactly a TaskHost launched with node reuse, so "owned node" was the existing sidecar concept under another name; ownership is a property of a sidecar, not a second category. NodeContext decides once whether it is talking to a sidecar rather than re-deriving it from wire state, and MarkNodeActive becomes TryActivateNode, doing one job for one caller now that shutdown no longer marks nodes active in order to shut them down. Restores AcquireAndSetUpHost to its original shape, keeping only the one line that matters: a sidecar retained from an earlier build is idle and must be re-activated, because shutdown waits only for nodes marked active. Behaviour is unchanged and verified: three consecutive server builds reuse one sidecar (2744ms, then 240ms and 211ms), and build-server shutdown reaps it. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8ecd77e2-5945-45e3-b885-befa4a23d37a
A sidecar answered a reusable NodeBuildComplete by sending NodeShutdown, so that packet meant both 'I am exiting' and 'I have reset and am still here'. Every reader of it then had to work out which was meant, and NodeContext reads it in three places. That put a TaskHost-specific concept, plus three guards, into NodeProviderOutOfProcBase, which serves worker nodes as well. Send NodeReadyForNextBuild instead. NodeShutdown recovers its single meaning, so all three guards and both sidecar members are gone from the shared base. The packet is deliberately node-level rather than TaskHost-level, and sits in the core range instead of the TaskHost callback range: OutOfProcNode.HandleNodeBuildComplete has the same end-of-build shape, so if worker nodes later stay connected to their owner they should send this same packet rather than introduce a second one meaning the same thing. What the base class still needs is only whether a connection outlives build completion, because its write-side drain thread must keep running when more packets will follow. It now asks that as a mechanism-level question, which the TaskHost provider answers, rather than deciding for itself what a sidecar is. No protocol version bump: an old parent calls GetHandshakeOptions without a nodeReuse argument, so it never requests a sidecar and a new child paired with one never sends this packet. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8ecd77e2-5945-45e3-b885-befa4a23d37a
DisposeCacheObjects(Build) already clears the build-lifetime dictionary, and AppDomain-lifetime objects live in a static field that the new instance never touched. The reallocation was a no-op that read as if it discarded the whole cache. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8ecd77e2-5945-45e3-b885-befa4a23d37a
A `-mt -nodeReuse:false` build asks the server it uses to shut down after the build. Both the client and the server derived their pipe and mutex names from a handshake that did not distinguish the two classes of server, so that request reached the resident server and killed it. XMake.cs:491 calls this a "short-lived server: fresh process, torn down afterward", but the process was never fresh: MSBuildClient.GetHandshake and OutOfProcServerNode.Run both leave nodeReuse at its default, so resident and transient hashed identically. The omission used to be safe. Before -mt, node reuse being off disqualified the server entirely (XMake.cs:464), so node reuse was a precondition for a server existing rather than a dimension of its identity. -mt created a second class of server without giving it one. A transient server now carries a per-invocation instance id, generated by the client and passed to the server it launches. Resident servers pass no id and are byte-for-byte unchanged. The id feeds ComputeHash, and therefore the pipe and all three mutex names, but deliberately not RetrieveHandshakeComponents: the handshake still answers "are we compatible", the hash answers "which server". No wire-format change. Distinguishing the classes with the existing NodeReuse bit would have fixed the reported bug but given every transient build one shared identity, so concurrent transient builds would contend on the running mutex and silently fall back in-process. Per-invocation ids let N concurrent transient builds run N private servers. Also scopes the over-provisioning guard, which counted every nodemode-8 process machine-wide despite a comment claiming it was per handshake. It is unreachable today, but multiple concurrent servers would otherwise arm it and let a resident server terminate itself on seeing transients. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8ecd77e2-5945-45e3-b885-befa4a23d37a
TransientBuildDoesNotDisturbResidentSidecars asserted that the resident server's sidecar was still alive after a transient build. That failed on all three Windows CI legs while passing on Linux, macOS and locally (3/3, plus three targeted experiments that each falsified a plausible cause: the end-of-build machine-wide scan cannot reach a TaskHost because it probes with the worker handshake, a foreign -nodeReuse:false build does not reap an owned sidecar, and neither does a second tenant with a different handshake salt requesting its own sidecar). A sidecar lives exactly as long as its owner, so its survival is a property of TransientBuildDoesNotShutDownResidentServer and of dotnet#14584, not of this change. Asserting it here also depends on nothing else on the machine touching that process for the duration of a second build, which does not hold when the suite runs in parallel. The test now asserts the deterministic property this change owns: a transient build brings its own server and its own TaskHost rather than adopting ones owned by a server that outlives it. The renamed test still fails without the fix, because the transient build would otherwise share the resident server outright. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8ecd77e2-5945-45e3-b885-befa4a23d37a
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #14584 (sidecar TaskHost ownership); review that first. The diff below is this commit only.
The bug
dotnet build -mt -nodeReuse:falsekills a resident MSBuild Server.A
-mtbuild with node reuse off still needs a server (for Server GC), but must not leave one behind, soXMake.cs:491setsshutdownServerAfterBuildand the client tells its server to exit once the build finishes. The comment there calls this "a short-lived server: fresh process, torn down afterward".The process was never fresh.
MSBuildClient.GetHandshake()andOutOfProcServerNode.Runboth callGetHandshakeOptions(taskHost: false, TaskHostParameters.Empty, architectureFlagToSet: …), wherenodeReusedefaults tofalse, so resident and transient servers hashed to identical pipe and mutex names. The transient client therefore connected to the resident server and ordered that to shut down.Why the omission was originally safe, at
XMake.cs:464:Drop the
-mtclause — the recent addition — and it reads!nodeReuse: node reuse being off disqualified the server entirely. Node reuse was a precondition for a server existing, never a dimension of its identity.-mtintroduced a second class of server without giving it one.The fix
A transient server carries a per-invocation instance id, generated by the client and passed to the server it launches. Resident servers pass no id and their endpoint is byte-for-byte unchanged.
The id feeds
ComputeHash(), and therefore the pipe and all three mutex names, but deliberately notRetrieveHandshakeComponents(). The handshake still answers "are we compatible"; the hash answers "which server am I talking to". Keeping those separate means no wire-format change.Passing the id on the child's command line follows the existing pattern for launch-time node parameters (
/nodemode,/nodereuse,/low,/parentpacketversion).Why not just add the
NodeReusebit to the handshakeThat is the smaller change and it does fix the reported bug, but it gives all transient builds one shared identity — and every name they need comes from that one hash: the pipe,
msbuild-server-running-*,msbuild-server-busy-*andmsbuild-server-launch-*. Transient servers then cannot coexist:-mtasked for;OutOfProcServerNoderefuses to start with "MSBuild server is already running!";Per-invocation ids let N concurrent transient builds run N private servers.
Also fixed: the over-provisioning guard
HandleServerShutdownCommandself-terminates whenCountActiveNodesWithMode(NodeMode.OutOfProcServerNode) > 1. The comment claims "per handshake", but the implementation filters only on process name and/nodemode:, so it counts every server process machine-wide. It is unreachable today (it needs aNodeBuildComplete(PrepareForReuse: true), and the only sender sendsfalse), but allowing several servers at once would arm it and let a resident server terminate itself on seeing transients. The count now excludes transient servers, which never compete for the resident role.Behaviour changes
dotnet build-server shutdowncannot reach a transient server, by design — it is private to one build. That is only safe because a transient is self-limiting, so all three exits were checked: it exits after its build; if its client dies once connected the pipe breaks andOnLinkStatusChangedsetsConnectionFailed; and if its client dies before connecting,NodeEndpointOutOfProcBase.PacketPumpProcgives up afterNodeConnectionTimeout(default 15 minutes). An orphan is bounded, not permanent.Verification
End-to-end, on a bootstrapped build:
Concurrency, sampled while three
-mt -nodeReuse:falsebuilds overlap:Tests added:
TransientBuildDoesNotShutDownResidentServer— the regression.ConcurrentTransientBuildsEachGetTheirOwnServer— the invariant this design exists for and the one theNodeReuse-bit approach fails. It checks each build ran in a server distinct from its own client first, because contention shows up as an in-process fallback rather than as equal PIDs.TransientBuildDoesNotAdoptResidentSidecars— cross-check against Own sidecar TaskHosts so they cannot outlive their owner #14584: a transient build must bring its own TaskHost rather than adopt one owned by a server that outlives it.GetKey()(the wire handshake) is unaffected, and that pipe and both mutex names differ.All three E2E tests were confirmed to fail with only the id generation disabled and pass with it, so they genuinely cover the bug.
*Server*suite locally: 19 failures before this change and 19 after, with 3 more passing tests — no regressions. (Those 19 are a pre-existing local-harness limitation: MSBuild Server does not start underExecMSBuildon this machine, and they fail identically on cleanmain. The new tests use the bootstrap, which does start a server.)Not in this PR
Worker-node ownership, the remaining item in this series.
One test was narrowed after the first CI run
An earlier version asserted the resident server's sidecar was still alive after a transient build. It failed on all three Windows legs while passing on Linux, macOS and locally (3/3). Three targeted experiments each falsified a plausible cause: the end-of-build machine-wide scan cannot reach a TaskHost at all because it probes with the worker handshake; a foreign
-nodeReuse:falsebuild does not reap an owned sidecar; and neither does a second tenant with a different handshake salt requesting its own sidecar.A sidecar lives exactly as long as its owner, so its survival belongs to
TransientBuildDoesNotShutDownResidentServerand to #14584 rather than to this change — and asserting it here additionally requires that nothing else on the machine touches that process for the duration of a second build, which does not hold when the suite runs in parallel. The test now asserts the deterministic property this change owns: a transient build brings its own server and its own TaskHost. It still fails without the fix.