Fix MSBuild Server client dropping build result under WaitAny race (#14172)

[JanProvaznik]

Summary

Addresses #14172: on Linux with MSBuild Server (/mt), a build that succeeds intermittently makes the entry process exit 1 with the final Build succeeded. summary dropped (no error printed).

Root cause

The MSBuild Server client's packet loop waits on WaitAny([cancellation, PacketPumpCompleted, PacketReceivedEvent]):

• PacketPumpCompleted is a sticky ManualResetEvent at a lower WaitAny index than PacketReceivedEvent.
• The packet pump enqueues the final ServerNodeBuildResult (setting PacketReceivedEvent) and then, a few instructions later, sets PacketPumpCompleted.
• If the main loop reaches WaitAny when both are already signaled, it returns the lower index and runs HandlePacketPumpCompleted, which set _buildFinished = true without draining the queue — dropping the queued ServerNodeBuildResult and any trailing console writes (e.g. the Build succeeded. summary).
• With no result processed, MSBuildAppExitTypeString stays null, so MSBuildClientApp.Execute returns MSBuildApp.ExitType.MSBuildClientFailure → the process exits 1 on a successful build.

This regressed in #7852, which changed the index‑1 wait handle from an error‑only event to the always‑set PacketPumpCompleted without making the handler drain the received-packets queue. Before that change, the low‑priority handle only fired on genuine errors, so a successful result was always drained via the PacketReceivedEvent branch.

Fix

Drain the received-packets queue before honoring PacketPumpCompleted:

• Extracted the loop into ProcessPacketsUntilBuildFinished and added a shared DrainPacketQueue helper used by both the PacketReceivedEvent and PacketPumpCompleted branches.
• A delivered result is honored even if the pump also recorded an exception (DrainPacketQueue, then if (!_buildFinished) HandlePacketPumpCompleted(...)), and queued console output is flushed on all completion paths.
• Exception propagation for a genuine mid-build disconnect (no result delivered) is preserved.

Testing

• Deterministic reproduction (both Windows and Linux): with an env‑gated stall that arranges the main loop to reach WaitAny after both events are set (the descheduling that CPU saturation causes naturally), the pre‑fix binary reproduces the exact symptom — exit 1, Build succeeded. dropped, no error, output truncated, with the trace confirming the ServerNodeBuildResult was still queued. With the fix, the same scenario exits 0 with the summary shown, and normal builds are unaffected. (Scaffolding not included in this PR.)
• Regression unit test (MSBuildClient_Tests.ProcessPackets_WhenPumpCompletedRacesQueuedResult_DoesNotDropBuildResult): drives the real loop with the result + summary queued and both events pre‑signaled; fails on the pre‑fix loop, passes on the fixed one.

Notes on reproduction confidence

The field symptom only reproduces naturally under heavy CPU load on ~32‑core Linux (~2.3%); it did not reproduce end‑to‑end on a 16‑vCPU box. The mechanism is confirmed by the deterministic forced‑race and by ruling out the two competing hypotheses in code (the server writes the result before closing the pipe — Join() before Dispose() with drain‑on‑terminate — so it is always delivered; and the build's exit type is frozen before the result is sent, so late node/TaskHost teardown cannot corrupt it). A failing‑build client MSBUILDDEBUGCOMM trace would make the attribution airtight; this change removes a real latent defect matching the symptom with no behavioral regression.

[JanProvaznik]

Expert code review — 24-dimension analysis

Reviewed with the repo's expert-reviewer methodology (.github/agents/expert-reviewer.agent.md). Verdict: approve. The production change is a correct, minimal, root-cause fix with no regression risk and no convention violations — no Critical/High/Medium findings.

Why it's correct

Draining the received-packets queue in both WaitAny branches makes the outcome independent of WaitAny index ordering. Because PacketPumpCompleted is a sticky ManualResetEvent that the pump sets strictly after every PacketReceived enqueue (MSBuildClientPacketPump.cs — enqueue+Set() then, after loop exit, PacketPumpCompleted.Set()), at least one full DrainPacketQueue is guaranteed to happen-after the last enqueue. So the final ServerNodeBuildResult can never be stranded, whichever handle WaitAny returns. This is a structural fix, not a retry/paper-over.

Threading is single-producer (pump thread enqueues + sets events) / single-consumer (ProcessPacketsUntilBuildFinished dequeues); _buildFinished is written and read only on the consumer thread, so there is no cross-thread visibility question. Every interleaving was walked — the both-signaled race, cancel + complete + result, and a genuine mid-build disconnect (exception, no result) — none drops, double-processes, hangs, or spins. The mid-build disconnect path still surfaces the exception as MSBuildClientExitType.Unexpected (behavior preserved), and trailing console writes now flush before that throw (a strict improvement).

Empirical verification

Checked out the branch, ran a full build.cmd, and ran the regression test from the built net10.0 exe → passes. Confirmed via code reading that pre-fix the PacketPumpCompleted branch set _buildFinished without draining, leaving MSBuildAppExitTypeString null (→ non-zero process exit), which the test's MSBuildAppExitTypeString.ShouldBe("Success") assertion catches.

Follow-up commit

Pushed b8dce54 strengthening the regression test: it now redirects Console.Out and asserts the trailing Build succeeded. summary is actually forwarded — the real user-visible symptom of #14172 — and this also keeps the seeded console write out of the test-run stdout.

Per-dimension results

#	Dimension	Verdict	Notes
1	Backwards Compatibility	✅	Restores intended behavior (success → exit 0). No new warnings/errors; WarnAsError-safe. Only intentional delta is the #14172 fix.
2	ChangeWave Discipline	✅ N/A	Correctly omitted — MSBuild Server is opt-in (/mt); gating a wrong-exit-code fix behind an opt-out would perpetuate the bug.
3	Performance & Allocation	✅	Client packet loop runs once per build (not a hot path). Helper extraction is allocation-neutral.
4	Test Coverage	✅	Deterministic regression test drives the exact both-signaled race; fails pre-fix, passes post-fix. Strengthened in b8dce54 to assert the dropped summary.
5	Error Message Quality	✅ N/A	No user-facing diagnostics changed.
6	Logging & Diagnostics	✅	Trace calls preserved; restores the dropped summary; trailing console writes now flush on all completion paths.
7	String Comparison	✅ N/A	No string comparisons introduced.
8	API Surface Discipline	✅	New members private except ProcessSeededPacketsForTests (internal, via InternalsVisibleTo). No public API / PublicAPI.txt change.
9	Target Authoring	✅ N/A	No targets/props.
10	Design Before Implementation	✅	Index-order-independent drain — structural root-cause fix matching the documented mechanism.
11	Cross-Platform Correctness	✅	Pure managed WaitHandle/ConcurrentQueue logic; platform-agnostic though the symptom is Linux-observed.
12	Code Simplification	✅	Extraction + shared DrainPacketQueue removes duplication; clearer control flow.
13	Concurrency & Thread Safety	✅	Single-producer/single-consumer; sticky-event guarantees a drain happens-after the last enqueue; all interleavings safe.
14	Naming Precision	✅	ProcessPacketsUntilBuildFinished/DrainPacketQueue are clear; test follows MethodUnderTest_Scenario_ExpectedResult.
15	SDK Integration	✅ N/A	No SDK boundary.
16	Idiomatic C#	✅	Collection expressions, consistent nullable annotations, using declarations.
17	File I/O & Path Handling	✅ N/A	No path/file I/O (test uses in-memory MemoryStream/StringWriter).
18	Documentation Accuracy	✅	XML docs on all new members; comments cite #14172 and explain the why.
19	Build Infrastructure	✅ N/A	No dependency/version/eng changes.
20	Scope & PR Discipline	✅	Two files, tightly scoped to the fix + its regression test; no drive-by changes.
21	Evaluation Model Integrity	✅ N/A	Not evaluation code.
22	Correctness & Edge Cases	✅	Race, cancel+complete+result, and disconnect paths all correct; result honored over a concurrent PacketPumpException (mutually exclusive in practice; result is authoritative).
23	Dependency Management	✅ N/A	No dependency changes.
24	Security Awareness	✅ N/A	Local named-pipe IPC unchanged; no untrusted-input handling changes.

Optional nits (non-blocking)

• result.MSBuildClientExitType.ShouldBe(Success) is not load-bearing — Success is the enum's default (0), so it passes even on the buggy code. The MSBuildAppExitTypeString and (new) console-content assertions are the real guards. Fine to keep as intent documentation.
• TestEnvironment env is unused in the new test (the seeded-packet path touches no env/mutex state), but it's harmless and consistent with the sibling test.

Generated with the GitHub Copilot CLI expert-reviewer methodology.

[Copilot]

Pull request overview

Fixes an MSBuild Server (/mt) client-side race where WaitHandle.WaitAny can observe PacketPumpCompleted before draining a queued ServerNodeBuildResult, causing a successful build to exit 1 with the final success summary dropped.

Changes:

• Refactors the packet-processing loop into ProcessPacketsUntilBuildFinished and adds a shared DrainPacketQueue helper so queued packets are drained before honoring PacketPumpCompleted.
• Ensures a delivered ServerNodeBuildResult is processed even if the pump also completed (and would otherwise end the loop early).
• Adds a regression unit test that deterministically seeds the pump/events to reproduce the race and validates the result + console write are not dropped.

Reviewed changes

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

File	Description
src/Build/BackEnd/Client/MSBuildClient.cs	Drains the received-packets queue before treating the packet pump as completed; adds helper + test hook to prevent dropping the final build result under the WaitAny ordering race.
src/Build.UnitTests/BackEnd/MSBuildClient_Tests.cs	Adds deterministic regression coverage to ensure the queued build result and success summary are not lost when PacketPumpCompleted and PacketReceivedEvent are both signaled.