[JanProvaznik]

Related issue(s): Follow-up to #14647.

Context

Two overlapping plain -mt builds still contend for the resident server. The second currently falls back into its client, losing the Server GC selected when launching an MT server. #14647 isolated explicitly transient builds but did not change this resident-busy path.

Changes Made

When resident admission returns ServerBusy for a reusable MT request, make one attempt to launch and connect to a private transient server using the existing per-invocation identity and shutdown machinery.

Keep the original command line and node-reuse setting. Skip the extra attempt when cancellation is already requested. If transient acquisition fails, retain the existing in-process fallback and report the transient attempt's failure reason. A build already executed by a server is never replayed.

Add deterministic coverage for busy and launch-contended residents, Server GC, transient exit and binlog lifecycle events, failed-build result propagation, failed-launch fallback, non-MT behavior, and pre-cancelled requests. Update the server documentation.

Fix the warm-server debugger test's admission race exposed in Linux CI: receiving the warmup result does not mean the resident has released its busy mutex. Wait for that mutex to disappear before submitting the debugger request, register the resident for cleanup before waiting, and report the client exit type if the request unexpectedly finishes. The original debugger assertions remain intact.

Compatibility

Scoped to experimental, opt-in MT behavior; no additional ChangeWave. Non-MT routing, explicitly disabled server use, and -mt -nr:false remain unchanged. No new user-facing diagnostics, public APIs, or wire-format changes. The final in-process fallback remains available and does not guarantee Server GC. The Linux follow-up changes test synchronization only, not production admission behavior.

Testing

• CLI/test builds succeeded for net11.0 and net472.
• Original focused Windows group: all 12 cases passed (ten retry/fallback cases plus concurrent-transient and resident-survival regressions). Post-Linux-fix Windows group: all 12 cases passed (ten retry/fallback cases plus warm/cold debugger cases).
• Linux, pinned SDK 11.0.100-rc.1.26420.103: 20 single-CPU, coverage-enabled runs passed all 280 selected cases, including 20 warm and 20 cold debugger cases, 40 in-process debugger opt-out cases, and 200 retry/fallback cases. No skipped cases.
• Controlled Linux handoff experiment: temporarily delayed the server after sending its result but before releasing its busy mutex. The original warm debugger test failed with ServerBusy; with the same delay, the synchronized test passed. Removed the diagnostic delay, rebuilt, and warm/cold cases passed again. No diagnostic delay is part of this PR. The unmodified baseline did not fail naturally in 20 runs; the controlled experiment demonstrates the race mechanism rather than claiming a natural reproduction of the CI run.
• Repeated the overlapping plain -mt experiment with node reuse left at its default. GCSettings.IsServerGC was read inside the build process, not a TaskHost, with no GC environment overrides.

Binaries	First build	Concurrent second build
Daily SDK 11.0.100-rc.2.26459.117	Resident, Server GC True	Client/in-process, Server GC False
Local bootstrap with this change	Resident PID 384996, Server GC True	Private server PID 390232, Server GC True

The second changed build's diagnostic log records a server started for this build only; its process has a unique /serverinstanceid and exits afterward.

CI build 1591186's Linux Core failure was ServerOnlyDebuggingWaitsBeforeEvaluation(warmServer: True); the cold case and all ten new retry/fallback cases passed. The follow-up above addresses its result-before-idle race. Fresh CI is pending after the fix.

Earlier local caveat: MultiThreadedServerProcessUsesServerGC, which launches from the unit-test output layout, failed in isolation with an uncontended UnableToConnect. It does not enter the new busy-retry branch. The bootstrap-based coverage and live probe pass; that separate layout failure is not changed here. No full local suite was run.

Dependencies and Follow-up

Uses the already-merged #14647 transient-server infrastructure. Full TaskHost ownership and handling a resident originally started with Workstation GC remain outside this change.

[Copilot]

🔵 Needs a closer look

Unresolved moderate findings remain in telemetry, framework coverage, and test synchronization.

Pull request overview

This PR retries busy reusable MT builds through a private transient server while preserving fallback behavior.

Changes:

• Adds transient-server retry routing and lifecycle handling.
• Expands retry, fallback, GC, failure, cancellation, and debugger tests.
• Updates server documentation.

File summaries

File	Summary	Final finding
src/MSBuild/MSBuildClientApp.cs	Implements transient retry routing.	Moderate (1 vote): Preserve the initial telemetry admission state instead of overwriting hot with cold.
src/MSBuild.UnitTests/MSBuildServer_Tests.cs	Synchronizes warm-server debugger coverage.	Moderate (1 vote): Use OutOfProcServerNode.GetBusyServerMutexName(handshake) rather than duplicating the mutex-name formula.
src/MSBuild.UnitTests/MSBuildClientApp_Tests.cs	Adds retry and fallback coverage.	Moderate (1 vote): Provide equivalent net472 coverage without relying on Environment.ProcessId.
documentation/MSBuild-Server.md	Documents server lifecycle behavior.	Nit (1 vote): Document the additional ShortLived=true contention case.

Review details

Suppressed comments (4)

documentation/MSBuild-Server.md:27

• This introduces another producer of ShortLived=true: a reusable /mt request that falls back to a private server. The public MSBuildServerLifecycleEventArgs.ShortLived contract still documents the flag as only applying to /mt with node reuse off, so update that API documentation (and the corresponding server-node summary) with the contention case.

The client determines the requested mode and node-reuse setting from the response-file-aware command-line parse. It sets the `ShutdownAfterBuild` flag on the `ServerNodeBuildCommand` packet for a short-lived server, whether requested by `/mt -nodeReuse:false` or used after resident-server contention. The in-process fallback does not acquire Server GC merely because `/mt` was requested.

src/MSBuild.UnitTests/MSBuildClientApp_Tests.cs:59

• Environment.ProcessId is not available on the net472 target, so the #if !NETFRAMEWORK guard excludes this entire new suite from full-framework testing even though the production retry path is built there (src/Directory.Build.props:37-39). That leaves the busy/launch-contended behavior untested on the framework-specific server/client implementation; use a framework-compatible process-id API and run equivalent coverage for net472 instead of excluding the file.

        serverPid.ShouldNotBe(Environment.ProcessId, "The busy resident must not force the MT build into its client.");

src/MSBuild.UnitTests/MSBuildServer_Tests.cs:148

• Use OutOfProcServerNode.GetBusyServerMutexName(handshake) here instead of duplicating the production mutex-name formula. If the production name changes later, this wait will observe the wrong object and the test can pass while submitting the debugger request during the resident's busy interval.

                string busyMutexName = $@"Global\msbuild-server-busy-{handshake.ComputeHash()}";

src/MSBuild/MSBuildClientApp.cs:82

• MSBuildClient.Execute updates KnownTelemetry.PartialBuildTelemetry.InitialMSBuildServerState on every attempt. The first resident admission records hot, but this retry uses a private hash and overwrites it with cold, so successful busy-resident builds are reported as cold and the admission condition is lost from telemetry. Preserve the first admission state (or record the transient state separately) when retrying.

                exitResult = transientClient.Execute(cancellationToken);

• Files reviewed: 4/4 changed files
• Comments generated: 0
• Review effort level: Lite

---

💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.

[ViktorHofer]

Expert code review — origin/main...4d85f8d (2 commits, 4 files, +241/−4)

Verdict: request changes. No Critical issues. The core idea is sound and the plumbing is genuinely well done — but two items should be resolved before merge.

The _serverInstanceId mechanism is an elegant reuse of existing machinery: one field flows through GetHandshake() into the pipe name, all three mutex names, and the connection-timeout override, which is why the transient server is simultaneously private, unreachable by other clients, and self-limiting if abandoned.

Invariant confirmed: the retry cannot double-submit an already-dispatched build. A repo-wide search for = MSBuildClientExitType.ServerBusy returns exactly two sites — MSBuildClient.cs:254 (after ServerWasBusy()) and :536 (in TryLaunchServer) — and both precede TrySendBuildCommand() at :275. The new doc line "do not retry a build already submitted to a server" is accurate.

---

High

H1 — Unbounded transient-server stampede: no throttle, no kill switch

src/MSBuild/MSBuildClientApp.cs:74-83

Every /mt client that observes a busy resident now unconditionally spawns a full MSBuild server process with DOTNET_gcServer=1. There is no admission control on the path — and per MSBuildClient.cs:84-88 this is deliberate ("lets concurrent transient builds run their own servers rather than contending on one set of pipe and mutex names").

Cold-start fan-out:

T=0  16 concurrent `dotnet build -mt` start in one user session (CI agent / script / VS).
T=1  Client #1 wins the launch mutex, spawns the resident.
T=2  Clients #2..#16 hit TryLaunchServer -> mutex not new -> ServerBusy   (MSBuildClient.cs:536)
T=3  All 15 take the new retry -> 15 x Process.Start of a Server-GC MSBuild server.
     Before this change: 15 in-process builds, 0 new processes.

Steady state is worse, not better: the busy mutex is held for the entire duration of build #1, whereas the launch mutex is held only across Process.Start. So the ServerWasBusy() path (MSBuildClient.cs:251-256) is the dominant trigger and it stays open for minutes.

Server GC reserves per-core heap structures, so on a 32–64 core CI box this is a machine-wide memory multiplier precisely where memory is scarcest. Secondary aggravator: because the build's -nodeReuse:true is deliberately preserved, each transient server's worker/TaskHost nodes are not torn down when the transient server exits. The transient server is short-lived; its node footprint is not.

There is also no escape hatch. Convention here is that new process-spawning behavior gets a MSBUILDDISABLE* env var. A user hitting memory pressure on a build agent currently has no way to turn this off short of dropping /mt.

Suggested, in rough order of value for effort:

1. Add MSBUILDDISABLETRANSIENTSERVER=1 and document it in MSBuild-Server.md.
2. Prefer waiting over spawning. ServerIsBusyWithWaitAndRetry(int) already exists (MSBuildClient.cs:348-361) but is currently only used by the shutdown path. A brief poll (250–500 ms) of the resident's busy mutex before deciding to spawn would convert most short-build contention back into resident reuse — cheaper and faster than a new Server-GC process, and it shrinks the stampede to genuinely-long-build cases.
3. Longer term, a machine-wide cap: a Global\msbuild-transient-server-{residentHash} named semaphore sized around max(1, ProcessorCount / 8), falling straight through to in-process when full.

H2 — Diagnostic misattribution: "resident busy" gets reported as "server crashed"

src/MSBuild/MSBuildClientApp.cs:74-109, enshrined by the test at MSBuildClientApp_Tests.cs:109-113

The retry overwrites exitResult, destroying the first attempt's outcome. Three user-visible consequences:

(a) New stderr output where MSBuild was previously silent. Line 106 deliberately suppresses the notice for ServerBusy ("the well-understood 'another client is racing us' case"). After the retry, a busy resident is reclassified as UnableToConnect / LaunchError, so line 108 now fires:

T=0  resident busy  -> exitResult = ServerBusy     (previously: silent, in-proc fallback)
T=1  transient spawn/connect fails
T=2  exitResult = LaunchError                      -> line 106 predicate now TRUE
T=3  Console.Error.WriteLine("MSBuildServerUnavailable ...")   <- NEW stderr line

(b) The binlog reason code is wrong. GetServerFallbackReason (:139-157) maps LaunchError -> ServerNotUsedReasonCodeServerCrashed. So MSBuildServerLifecycleEventArgs.ReasonCode reports server-crashed and the localized text says "the server crashed on launch (exit code N)" — when the root cause was "the resident was busy." That sends someone hunting a crash that never happened. The new test asserts this as correct (ReasonCode.ShouldBe(ServerNotUsedReasonCodeServerCrashed) for the MT case), which locks the defect in.

(c) Telemetry is corrupted on the success path — this is the one I'd care most about, since it defeats the feature's own success metric. When the retry succeeds, ServerFallbackReason is never set, so the busy resident is invisible. Worse, the second Execute unconditionally re-runs MSBuildClient.cs:232-236:

KnownTelemetry.PartialBuildTelemetry.InitialMSBuildServerState = serverIsAlreadyRunning ? "hot" : "cold";

The transient client's handshake is salted with a fresh GUID, so ServerIsRunning() is always false → InitialMSBuildServerState is overwritten from "hot" to "cold". Contended builds will be reported as cold starts, hiding exactly the contention signal this change exists to address.

Roughly a 10-line fix:

MSBuildClientExitResult residentResult = exitResult;   // preserve
if (residentResult.MSBuildClientExitType == ServerBusy && multiThreaded && ...)
{
    string? stateBeforeRetry = KnownTelemetry.PartialBuildTelemetry?.InitialMSBuildServerState;
    exitResult = new MSBuildClient(..., shutdownServerAfterBuild: true).Execute(cancellationToken);
    if (KnownTelemetry.PartialBuildTelemetry is { } t)
    {
        t.InitialMSBuildServerState = stateBeforeRetry;                  // resident was hot
        t.ServerFallbackReason ??= "ResidentBusy_TransientRetried";
    }
}

On the fallback path, derive the reason from residentResult (still ServerBusy) rather than the retry's result — ideally with a distinct code such as server-busy-transient-failed — and update the test assertion to match.

---

Medium

M1 — Up to ~5 s of dead time added before the in-process fallback

MSBuildClientApp.cs:81-82. Worst case, where the transient also fails:

Step	Cost
Attempt 1: ServerIsRunning() + ServerWasBusy() → ServerBusy	~µs (mutex open)
Attempt 2: TryLaunchServer() → Process.Start	tens of ms + host startup
Attempt 2: TryConnectToServer(NewServerConnectionTimeoutMilliseconds)	up to 5,000 ms (MSBuildClient.cs:37, used at :259)
Fallback: full in-process MSBuildApp.Execute	unchanged

A user whose transient server fails to come up waits ~5 s of pure dead time before their build even starts, on a build that previously started immediately. Plus a stray server lingering up to 10 s before self-terminating (MSBuildClient.cs:612-614). The transient attempt connects to a process it just launched with a private identity, so a shorter budget than 5 s seems safe.

M2 — The whole new test file is excluded from .NET Framework, where the production code ships

MSBuildClientApp_Tests.cs:4 (#if !NETFRAMEWORK) / :196. The csproj targets $(RuntimeOutputTargetFrameworks) = net472;net10.0 on Windows, and the retry block at MSBuildClientApp.cs:74-83 is not #if-guarded — nor is anything on the path to it (IsMultiThreadedEnabled, ShouldUseMSBuildServer, the MSBuildClientApp.Execute call at XMake.cs:345). So a new process-spawning path ships in net472 MSBuild.exe with zero test coverage there.

Also inconsistent with both siblings: MSBuildServer_Tests.cs has no file-level guard (only individual tests use #if NET), and MSBuildMultithreaded_Tests.cs has none at all.

Either drop the file-level guard and guard only the specific tests that genuinely can't run on Framework, or #if-guard the production block too so code and tests agree.

M3 — Mutex name formats duplicated into tests; production lacks a helper for the launch mutex

MSBuildClientApp_Tests.cs:44, 88, 150-151; MSBuildServer_Tests.cs:148; production MSBuildClient.cs:526

Name	Production source	Helper?
Global\msbuild-server-running-{hash}	OutOfProcServerNode.cs:207	✅ GetRunningServerMutexName
Global\msbuild-server-busy-{hash}	OutOfProcServerNode.cs:210	✅ GetBusyServerMutexName
Global\msbuild-server-launch-{hash}	MSBuildClient.cs:526	❌ inline literal in production

This is a silent-false-pass hazard rather than a cosmetic one: renaming any of these compiles cleanly and turns the tests into no-ops — they'd create a mutex nobody looks at, the resident would never appear busy, the retry would never trigger, and every assertion would still pass because the build succeeds either way.

Suggest adding GetServerLaunchMutexName(ServerNodeHandshake) next to the other two, calling it from TryLaunchServer, and having the tests use the production helpers (ServerNodeHandshake_Tests.cs:95-96 already does this).

M4 — ChangeWave not needed, but an escape hatch is

A ChangeWave is not required: /mt is fully opt-in today (Traits.cs:46,52, or an explicit -mt), and changing behavior inside an opt-in preview feature is legitimately exempt.

An escape hatch still is, for two reasons: the new stderr output and changed reason codes in H2 are observable by /mt users today; and the stated trajectory is to default /mt on, at which point H1 becomes default behavior on every CI agent with no way to disable it and no ChangeWave to opt out of. Cheaper to add now.

---

Low / nits

• L1 — MSBuildClient is sealed but not IDisposable, yet its ctor allocates a NamedPipeClientStream + packet pump (:198-216). On the ServerBusy path ReadPacketsLoop — the only place that disposes the pump — never runs. Pre-existing, but this change now abandons two clients instead of one before running a full build in the same process.
• L2 — !cancellationToken.IsCancellationRequested is checked once, then TryConnectToServer takes no token, so Ctrl-C isn't observed for up to 5 s. Orphaning itself is bounded (transient servers self-exit within 10 s via the forced MSBUILDNODECONNECTIONTIMEOUT at MSBuildClient.cs:610-614), so this is responsiveness only. Re-checking the token right after TryLaunchServer would shrink the window.
• L3 — int.Parse / bool.Parse on file content with no CultureInfo.InvariantCulture and no probe.Length guard (:59-60, 64, 107). If the probe project fails before WriteLinesToFile, this surfaces as a bare FormatException with no indication of what the build did.
• L4 — Fixed 10 s timeouts (:71, 190; MSBuildServer_Tests.cs:153) bracket a real server launch, a full build, and teardown, in tests that deliberately create contention. Thin on a loaded CI agent; note xunit.runner.json sets longRunningTestSeconds: 60.
• L5 — if (Environment.ProcessorCount > 1) (:62-65) silently skips the Server GC assertion — the entire motivation for the change — and still reports green. Worth an Assert.Skip or at least an _output.WriteLine.
• N1 — Terminology drift: the same concept is transient (code), private/short-lived (doc), and overflow (test message at :72). "Transient" matches the existing constant.
• N2 — The !shutdownServerAfterBuild guard is unreachable today: such a client already has a private identity, so it can never observe ServerBusy. Correct defensive coding, but worth one comment line so the next reader doesn't try to cover it with a test.
• N3 — env.WithTransientProcess(serverPid) is registered after assertions that could throw (:59-61). Low real risk since the transient self-terminates, and if line 60 fires then serverPid == Environment.ProcessId anyway — but registering first is free.
• N4 — MSBuild-Server.md has a duplicate list number adjacent to the edit (2. Initiate handshake / 2. Issue build command). Pre-existing, but you're editing that list.
• N5 — If H1 ships as-is, the doc should state that the private server is not throttled and that N contended /mt builds produce N server processes.

---

Things I checked that turned out fine

• Test parallelism / global mutex collisions — not an issue. src/Shared/UnitTests/xunit.runner.json sets maxParallelThreads: 1 and parallelizeTestCollections: false and is linked into this csproj. Cross-process collision is prevented by the fresh MSBUILDNODEHANDSHAKESALT GUID, which Handshake's ctor reads live from the environment (Handshake.cs:93) rather than the cached Traits static — so the mutex names really are per-test-unique. Nice.
• ResetInstance_ForUnitTestsOnly leaking static state — correctly reverted; TestEnvironment.Cleanup() calls it unconditionally (TestEnvironment.cs:123) and the test holds the env in a using.
• Orphaned transient servers — bounded to 10 s, as above.
• Warm-server PID leak from the new if (!warmServer) — no leak; the PID is registered earlier in the new warm-server block (MSBuildServer_Tests.cs:140) from the same pidField value. The guard just avoids a duplicate registration.
• Doc claim about node-reuse semantics — accurate. _shutdownServerAfterBuild feeds only the ServerNodeBuildCommand flag (MSBuildClient.cs:668); commandLineArgs pass through unchanged, so the build keeps its -nodeReuse. Server side, OutOfProcServerNode.cs:510 sets BuildComplete and :487 publishes CurrentBuildShutsDownServerNode, which XMake.cs:2151-2158 turns into ShortLived = true.
• The MSBuildServer_Tests warm-server change is a correct fix for a genuine race, and testing real contention against real processes rather than mocking it away is the right call.

---

Summary

Before merge: H2 (preserve the resident's outcome; stop reporting server-crashed; restore InitialMSBuildServerState; fix the test assertion), H1 (at minimum an env-var kill switch + document the unthrottled behavior), M2 (align the test guard with the production guard).

Strongly recommended: the pre-spawn resident poll via the existing ServerIsBusyWithWaitAndRetry (H1.2 / M1) — it's the cheapest credible mitigation and it's already sitting in the file — plus M3.

Follow-ups, non-blocking: L1–L5, N1–N5, and a tracked issue for a real throttle regardless of which mitigation lands now.

_{🤖 Generated with GitHub Copilot CLI}

[JanProvaznik]

with the expert code review I am starting to doubt if this is actually desirable

1. try to connect
2. fallback-to-spawn and connect
   has overhead, and we achieve getting ServerGC which is plausibly not great to have multiple times on the machines

[JanProvaznik]

so M1 sounds worst,
H1 I disagree
H2 valid

[rainersigwald]

I don't think I'm too worried about the 5s worst-case wait where the transient server crashes, but I'm not going to push too hard for this either.

Agreed that H1 is bogus and H2 is valid.