Fix the two flaky tests taking down build-and-package - #751
Merged
Conversation
Ten consecutive Build and Package failures came from two independent flaky tests, not one. RunDotnetProcessAsync_Cancellation_KillsProcessTree timed out waiting for its PID file. Start-Process failure is a NON-terminating error, so the root script carried on to Set-Content with $p still null, published a 0-byte file, and left the reader polling something that would never parse -- then reported the file "was not written" about a file that had in fact been written, empty. The child's stderr was redirected and discarded, so the real reason never reached CI. #732 fixed a different failure mode in this helper (a sharing violation); this timeout signature predates that fix and still reproduces on commits containing it. AnalyzeDumpAsync_RealManagedDump_RunsClrMdManagedEnumeration dumped a PowerShell child after a blind 1.5s sleep. Under load the child was still in native startup -- CI dumps show only hostfxr/hostpolicy loading coreclr -- so ClrMD found no runtime and the "CLR Version" assertion failed. Nothing had ever fixed this one. Fixes: - $ErrorActionPreference = 'Stop' plus an explicit null guard, so a failed child start aborts before Set-Content instead of publishing an empty file - WaitForPidFileAsync watches the launcher task and fails fast with the root's exit code and stderr rather than waiting out the clock - the timeout message now distinguishes never-created, unreadable, and created-but-unparsable - the managed child allocates, collects, and only then signals readiness, so the dump is taken with the runtime up AND its heap walkable Signalling readiness alone was not enough: a token emitted from the first line of managed code made the dump test fail or go inconclusive on a machine where the old sleep passed 6/6, which is why the child warms its heap before signalling. The script builder is shared with its regression test so the script proven not to publish an empty file is the one the tree-kill test runs. Verified by removing the fix: the guard fails with the original script and passes with the hardened one. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: c8e8ead7-3a65-4148-b196-a4c217fde912
Copilot started reviewing on behalf of
Alexandre Zollinger Chohfi (azchohfi)
August 14, 2026 00:14
View session
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| if (child.HasExited) | ||
| // Drain stderr so a chatty host can never fill the pipe and wedge the child. | ||
| var draining = child; | ||
| _ = Task.Run(() => { try { draining.StandardError.ReadToEnd(); } catch { /* ignore */ } }); |
Contributor
There was a problem hiding this comment.
Pull request overview
Hardens two flaky process-based tests in the CLI test suite.
Changes:
- Adds deterministic managed-runtime readiness signaling before dump capture.
- Improves PID-file failure handling and diagnostics.
- Adds regression coverage for launcher failures and malformed PID files.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
TestProcessDump.cs |
Warms the managed heap and waits for readiness before dumping. |
DotNetServiceTests.cs |
Hardens tree-kill setup, PID polling, diagnostics, and tests. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+2061
to
+2069
| var sw = Stopwatch.StartNew(); | ||
| var ex = await Assert.ThrowsAsync<InvalidOperationException>( | ||
| async () => await WaitForPidFileAsync(pidFile, TimeSpan.FromSeconds(30), TestContext.CancellationToken, rootTask)); | ||
| sw.Stop(); | ||
|
|
||
| StringAssert.Contains(ex.Message, "exited with code 1"); | ||
| StringAssert.Contains(ex.Message, "cannot find the file", "The root's stderr is the whole point of the diagnostic."); | ||
| Assert.IsLessThan(TimeSpan.FromSeconds(5), sw.Elapsed, | ||
| "A dead root must fail fast rather than wait out the full timeout."); |
| var state = (everExisted, lastRead) switch | ||
| { | ||
| (false, _) => "it was never created", | ||
| (true, null) => "it was created but could never be read (the writer held it exclusively for the whole wait)", |
Contributor
Build Metrics ReportBinary Sizes
Test Results✅ 4576 passed, 5 skipped out of 4581 tests in 605.3s (+4 tests, -12.1s vs. baseline) Test Coverage✅ 89.1% line coverage, 82.4% branch coverage · ✅ no change vs. baseline CLI Startup Time50ms median (x64, Try This BuildInstalls the MSIX for your architecture, replacing any previously installed build. Needs the GitHub CLI — the command offers to install it and sign you in if it is missing. & ([scriptblock]::Create((irm https://raw.githubusercontent.com/microsoft/winappCli/main/scripts/winapp-pr.ps1))) 751Switching between builds often?Put the tool on your PATH once: & ([scriptblock]::Create((irm https://raw.githubusercontent.com/microsoft/winappCli/main/scripts/winapp-pr.ps1))) -AddToPathThen this build is just: winapp-pr 751Run Updated 2026-08-14 00:35:58 UTC · commit |
Zach Teutsch (zateutsch)
approved these changes
Aug 14, 2026
Zach Teutsch (zateutsch)
left a comment
Zach Teutsch (zateutsch)
left a comment
Contributor
There was a problem hiding this comment.
PR review skill was all clean, looks good to me.
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.
Problem
The last ten
Build and Packagefailures came from two independent flaky tests, not one. On Aug 13 the workflow failed 13 of 33 runs (~39%).DotNetServiceTests.RunDotnetProcessAsync_Cancellation_KillsProcessTreeTimeoutException: descendant PID file was not written within 00:00:15CrashDumpServiceWorkflowTests.AnalyzeDumpAsync_RealManagedDump_RunsClrMdManagedEnumerationStringAssert.Contains— analysis log lacksCLR Version1. Tree-kill — #732 fixed a real bug, just not this one
#732 and #737 fixed a sharing-violation
IOExceptioninWaitForPidFileAsync, and that signature is gone. The current failure is a different mode. It already appeared on Aug 12 at 17:39, before #737 merged, and 4 of the 6 failures are on commits that contain the fix — including one onmain. Issue #729 is still open.Root cause, reproduced directly:
Start-Processfailure is a non-terminating error, so the script ran on toSet-Contentwith$pstill null and published a 0-byte file:int.TryParsesilently skipped it and the reader polled the full 15s. The message "was not written" was actively misleading — the file was written, just empty. The child's stderr was redirected and discarded, so the real error never reached CI. For scale, that script publishes in 373 ms on an unloaded machine, against a 15 s budget.2. Managed dump — never fixed at all
TestProcessDump.TryCreateManagedDumpdumped a PowerShell child after a blindThread.Sleep(1500). Under load the child was still in native startup — the CI dumps show onlyhostfxr/hostpolicyloading coreclr — so ClrMD found no runtime. No commit had ever touched this.Fixes
Tree-kill
$ErrorActionPreference = 'Stop'plus an explicit null guard, so a failed child start aborts beforeSet-Contentrather than publishing an empty fileWaitForPidFileAsyncnow watches the launcher task and fails fast with the root's exit code and stderr instead of waiting out the clockManaged dump
On the second fix, specifically
Signalling readiness alone was not enough, and this is worth naming because the obvious fix is wrong. A token emitted from the first line of managed code proves the CLR started but not that ClrMD can walk it — that version made the dump test fail or go inconclusive on a machine where the old sleep passed 6/6. Warming the heap before signalling is what actually fixes it:
Thread.Sleep(1500))Verification
TreeKillScript_WhenTheChildCannotStart_PublishesNoPidFileAtAllfails with the original script (Assert.IsFalse failed ... File.Exists(pidFile)) and passes with the hardened one.winapp initE2E tests, confirmed pre-existing by re-running them with these changes stashed — they fail onFailed to get version for Microsoft.WindowsAppSDK, the documented corp-machine nuget.org restriction.Local load testing could not reproduce CI's 4-core I/O-bound conditions, so the load column above is weaker evidence than the idle column. The structural argument stands on its own: the old code bounded CLR startup with a fixed 1.5 s guess, and CI captured dumps proving that guess was exceeded.
Closes #729