Remove the thread-pool dependency from the PID-file tests - #737
Conversation
WaitForPidFile_EmptyThenPopulated_WaitsForAParsableValue failed on CI with a 15 second TimeoutException. Both tests added in #732 populated or released the file from a Task.Run continuation and then waited on the helper, so each depended on the thread pool scheduling that continuation promptly. On a loaded agent running four test workers that is not guaranteed -- reintroducing, in the tests meant to remove flakiness, exactly the kind of timing dependence they were fixing. Neither test needs concurrency to prove its point: - The sharing-violation test now asserts the exception TYPE with nothing ever releasing the handle. The old code threw IOException on the first poll; the retry swallows it and runs out the clock, so TimeoutException is the signal. Verified it still fails in 72ms when the retry is removed. - The empty-file test asserts that a permanently empty file times out, which is what the parse guard is for. The populated path is already covered by the tree-kill test end to end. Both now run in well under a second and cannot be perturbed by machine load. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: a209763e-7185-4986-bf0b-98e52c06b4be
There was a problem hiding this comment.
Pull request overview
Removes thread-pool scheduling dependencies from PID-file regression tests.
Changes:
- Replaces background writers with deterministic timeout assertions.
- Shortens test execution to sub-second timeouts.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Build Metrics ReportBinary Sizes
Test Results✅ 4555 passed, 5 skipped out of 4560 tests in 628.1s (+1 test, -85.4s vs. baseline) Test Coverage✅ 89.1% line coverage, 82.4% branch coverage · ✅ no change vs. baseline CLI Startup Time52ms 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))) 737Switching 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 737Run Updated 2026-08-12 04:56:48 UTC · commit |
|
Copilot review |
Reviewed. The diff is confined to the Two things I checked, and one I decided against: Comment accuracy. The remaining coverage gap, and why I left it. The three timeout tests prove The obvious fix is to start the wait while holding the handle, dispose it synchronously, then await the PID. I didn't, because that is the same shape as what this PR removes: after the dispose, the polling loop only resumes when the thread pool runs its That path is covered end-to-end by |
4ad81ef to
c4c9685
Compare
Review follow-up. Rewriting the exclusive-handle test to assert a timeout left both remaining tests asserting failure modes, so nothing at unit level proved a readable PID file is actually parsed -- only the tree-kill test did, and that exercises the whole launcher. The comment claiming otherwise was stale. Adds WaitForPidFile_PopulatedFile_ReturnsThePid: write a value, call the helper, assert the value. No concurrency, so it keeps the determinism the rest of this change is about, and corrects the comment to point at it. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: a209763e-7185-4986-bf0b-98e52c06b4be
Co-authored-by: azchohfi <527713+azchohfi@users.noreply.github.com>
c4c9685 to
e36b86e
Compare
## Problem The last ten `Build and Package` failures came from **two independent flaky tests**, not one. On Aug 13 the workflow failed 13 of 33 runs (~39%). | Test | Runs | Failure | |---|---|---| | `DotNetServiceTests.RunDotnetProcessAsync_Cancellation_KillsProcessTree` | 6/10 | `TimeoutException: descendant PID file was not written within 00:00:15` | | `CrashDumpServiceWorkflowTests.AnalyzeDumpAsync_RealManagedDump_RunsClrMdManagedEnumeration` | 5/10 | `StringAssert.Contains` — analysis log lacks `CLR Version` | ### 1. Tree-kill — #732 fixed a real bug, just not this one #732 and #737 fixed a **sharing-violation `IOException`** in `WaitForPidFileAsync`, 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 on `main`. Issue #729 is still open. Root cause, reproduced directly: `Start-Process` failure is a **non-terminating** error, so the script ran on to `Set-Content` with `$p` still null and published a **0-byte file**: ``` File created: YES; length=0 bytes; content=<>; int.TryParse=False ``` `int.TryParse` silently 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.TryCreateManagedDump` dumped a PowerShell child after a blind `Thread.Sleep(1500)`. Under load the child was still in **native** startup — the CI dumps show only `hostfxr`/`hostpolicy` loading 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 *before* `Set-Content` rather than publishing an empty file - `WaitForPidFileAsync` now watches the launcher task and fails fast with the root's exit code and stderr instead of waiting out the clock - the timeout message distinguishes never-created, unreadable, and created-but-unparsable **Managed dump** - the child allocates, forces a collection, and only then signals readiness, so the dump is taken with the runtime up **and** its heap walkable ## 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: | Version | Idle | Under 100% CPU load | |---|---|---| | Original (`Thread.Sleep(1500)`) | 6/6 pass | 6/6 pass | | Token only | **3/6 pass** | — | | Token after heap warm-up | 8/8 pass | 6/6 pass | ## Verification - Both regression guards were checked against the **original** code, not just the new code. `TreeKillScript_WhenTheChildCannotStart_PublishesNoPidFileAtAll` fails with the original script (`Assert.IsFalse failed ... File.Exists(pidFile)`) and passes with the hardened one. - The script builder is shared between the tree-kill test and its regression test, so the script proven not to publish an empty file is the one actually run — the two cannot drift. - Full suite: **4,572 passed / 4,581**. The 4 failures are `winapp init` E2E tests, confirmed pre-existing by re-running them with these changes stashed — they fail on `Failed 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 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: c8e8ead7-3a65-4148-b196-a4c217fde912
Problem
WaitForPidFile_EmptyThenPopulated_WaitsForAParsableValue— added by me in #732 and now onmain— failed on CI:Both tests #732 added did the same thing: populate (or release) the file from a
Task.Runcontinuation, thenawaitthe helper. That makes each one depend on the thread pool scheduling that continuation promptly. On a loaded agent running 4 test workers, it isn't.Which is the irony worth naming: these are the tests meant to remove flakiness from this file, and they reintroduced exactly the kind of timing dependence they were fixing. A 150 ms delay under a 15 s budget looks generous right up until the pool is saturated.
Fix
Neither test needs concurrency to prove its point.
Sharing violation — assert the exception type, with nothing ever releasing the handle:
The old code threw
IOExceptionon the very first poll; the retry swallows it and runs out the clock.TimeoutExceptionis the signal that the retry works — no second thread required.Empty file — assert that a permanently empty file times out, which is precisely what the parse guard is for. The populated path is already covered end-to-end by the tree-kill test.
Validation
catch (IOException)removed,WaitForPidFile_WriterHoldsFileExclusively_RetriesInsteadOfThrowingstill fails in 72 ms.DotNetServiceis unchanged; this is tests only.This unblocks
build-and-package, which is currently red onmainand on #724.