fix: stop the example harness waiting out MSBuild node reuse - #35
Merged
Conversation
Cli.Run redirects the child's stdout and stderr, and finishes with an unbounded WaitForExit(). That overload waits for the redirected streams to reach EOF, not for the process to exit. MSBuild worker nodes inherit those handles from the build they are spawned by, and with node reuse left on they outlive it by the reuse idle timeout, so the pipes stay open and the harness sits there for the full fifteen minutes. Measured on Build_SyncsStyleConfigFiles_WhenMultiTargetingInParallel: node reuse on build 10.9s WaitForExit() 900.0s test 15m 14s node reuse off build 18.3s WaitForExit() 0.0s test 23s ExampleWorkspace.Build shuts the build server down immediately before building, which is what makes this reproducible rather than occasional: it guarantees fresh nodes are spawned instead of existing ones connected to, and only a freshly spawned node inherits our handles. That is also why it looks like a hang rather than slow work - the CPU is flat, the build finished long ago. Full suite: 45.5 minutes to 9.8 minutes, 53 tests passing. Node reuse is an inner-loop optimisation with nothing to offer a harness that builds each project once in a throwaway workspace, so it is disabled for every process the harness starts rather than per call site. Refs #34
|
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.



Fixes the hang half of #34.
What was happening
Cli.Runredirects the child's stdout and stderr and finishes with an unboundedWaitForExit(). That overload waits for the redirected streams to reach EOF, not for the process to exit. MSBuild worker nodes inherit those handles from the build that spawns them, and with node reuse left on they outlive it by the reuse idle timeout — so the pipes stay open and the harness sits there for the full fifteen minutes.Measured on
Build_SyncsStyleConfigFiles_WhenMultiTargetingInParallel, same machine, only node reuse changed:WaitForExit()900.0 s is not a coincidence — it is exactly MSBuild's fifteen-minute node-reuse idle timeout.
ExampleWorkspace.Buildrunsdotnet build-server shutdownimmediately before building, and that is what makes this reproduce every time rather than occasionally: it guarantees the build must spawn fresh nodes instead of connecting to existing ones, and only a freshly spawned node inherits this process's handles. It is also why it reads as a hang rather than as slow work — the CPU is flat and the build finished long ago.That last detail is why my first attempt to reproduce it in isolation failed: I ran the serial build first, which left nodes running for the
-mrun to reuse, and the stall vanished.The fix
One line:
MSBUILDDISABLENODEREUSE=1in the environmentCli.Rungives every process it starts. Node reuse is an inner-loop optimisation with nothing to offer a harness that builds each project once in a throwaway workspace, so it is disabled centrally rather than per call site.Result
Full suite 45.5 minutes → 9.8 minutes, 53 tests passing. The parallel-sync test went 39 m 11 s → 25 s.
CLAUDE.mdclaimed that test costs ~15 minutes and used that to justify extracting the pure logic; the number is now wrong, so it is corrected in place.Tests
CliProcessLifetimeTests.Build_ReturnsWhenTheBuildExits_RatherThanWaitingOutMsbuildNodeReusebuilds the multi-targeted demo under-mand asserts the call returns inside five minutes — far above the ~25 s it needs and far below the fifteen-minute stall, so it separates the two without being sensitive to machine speed.Watched it fail first:
and pass after:
[24 s].What this does not claim
#34 also reports
Test host process crashed. That did not reproduce in any of the four runs today, including a full pre-fix run that took 45 minutes and completed cleanly, so I cannot claim it is fixed. What this removes is the most plausible contributor: a run that no longer spends most of its time blocked, and no longer accumulates idle worker nodes holding pipes open. I would leave #34 open until a few CI-length runs have gone by without one.The issue's third acceptance point is already satisfied, and worth recording: CI is not masking anything today.
Example Integration Testson ubuntu-latest reportsTest Run Successful. Total tests: 52, Passed: 52, and an aborted run exits non-zero with nocontinue-on-error, so it would fail the step loudly.The stall was not local, though - CI was paying it too.
Example Integration Testson this PR ran in 3 m 7 s, against a main baseline of 1050-1101 s (~17.5 minutes) across the last four runs. Node reuse was costing every CI run about fourteen minutes as well; it simply never crashed there. Only the crash is Windows-local.