Fix flaky tree-kill test by retrying the PID file read - #732
Merged
Nikola Metulev (nmetulev) merged 4 commits intoAug 12, 2026
Conversation
RunDotnetProcessAsync_Cancellation_KillsProcessTree fails intermittently on CI:
System.IO.IOException: The process cannot access the file
'...\winapp_treekill_<guid>.pid' because it is being used by another process.
at System.IO.File.InternalReadAllTextAsync(...)
at DotNetServiceTests.WaitForPidFileAsync(...) DotNetServiceTests.cs:1864
The helper polls for the PID file that the spawned PowerShell root writes with
Set-Content. File.Exists turns true the moment Set-Content creates the file, but
PowerShell still holds the write handle for a short window after that, so a read
landing inside the window fails with a sharing violation. The exception escaped
the polling loop and failed the test.
Reproduced directly: hold a handle open with FileShare.None and File.Exists
returns true while File.ReadAllText throws that exact IOException.
A transient IO failure here just means "not written yet", which is the condition
the loop already exists to wait out, so it is caught and retried. An empty or
partially written file was already covered by the existing int.TryParse guard.
The target test passed 3/3 locally after the change. WaitForPidFileAsync has a
single caller, so nothing else is affected.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: a209763e-7185-4986-bf0b-98e52c06b4be
Copilot started reviewing on behalf of
Alexandre Zollinger Chohfi (azchohfi)
August 11, 2026 22:31
View session
Contributor
There was a problem hiding this comment.
Pull request overview
Retries transient PID-file read failures to stabilize the process-tree cancellation test.
Changes:
- Retries
IOExceptionwhile PowerShell finishes writing the PID file. - Documents the race condition and existing parse retry behavior.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Contributor
Build Metrics ReportBinary Sizes
Test Results✅ 4554 passed, 5 skipped out of 4559 tests in 556.2s (+3 tests, -25.0s vs. baseline) Test Coverage✅ 89.1% line coverage, 82.4% branch coverage · ✅ no change vs. baseline CLI Startup Time46ms 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))) 732Switching 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 732Run Updated 2026-08-12 00:07:06 UTC · commit |
Alexandre Zollinger Chohfi (azchohfi)
marked this pull request as draft
August 11, 2026 22:50
Two follow-ups from review. Only IOException is caught now. UnauthorizedAccessException is a permissions or path failure, not the transient sharing violation, and swallowing it turned an actionable error into a misleading 15 second "was not written within" timeout. The race is now reproduced deterministically instead of by timing luck. The original test only failed when a poll landed inside PowerShell's write window -- a sub-millisecond target against a 20ms poll -- which is why it survived 400 consecutive local runs while still failing on loaded CI agents. Holding the file the way Set-Content does fails the old implementation in 174ms with the exact CI exception, and passes with the retry. Measurements behind the approach, against a real Set-Content writer: reader using File.ReadAllText 10845 sharing violations reader using ReadWrite|Delete share 10845 sharing violations writer publishing by atomic rename 503 sharing violations Set-Content holds the destination exclusively, so no FileShare mode on the reader can open it during the window, and publishing by rename only narrows the window rather than closing it. Retrying is therefore the fix, not a workaround. Also covers the two neighbouring cases: a file that never appears must still time out, and an empty file must be polled past by the parse guard rather than treated as an error. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: a209763e-7185-4986-bf0b-98e52c06b4be
Alexandre Zollinger Chohfi (azchohfi)
marked this pull request as ready for review
August 11, 2026 23:48
Nikola Metulev (nmetulev)
approved these changes
Aug 12, 2026
Nikola Metulev (nmetulev)
deleted the
azchohfi-fix-treekill-pidfile-race
branch
August 12, 2026 00:49
Alexandre Zollinger Chohfi (azchohfi)
added a commit
that referenced
this pull request
Aug 12, 2026
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
Nikola Metulev (nmetulev)
pushed a commit
that referenced
this pull request
Aug 12, 2026
## Problem `WaitForPidFile_EmptyThenPopulated_WaitsForAParsableValue` — added by me in #732 and now on `main` — failed on CI: ``` System.TimeoutException: The descendant PID file '...empty_....pid' was not written within 00:00:15. ``` Both tests #732 added did the same thing: populate (or release) the file from a `Task.Run` continuation, then `await` the 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: ```csharp using var exclusive = new FileStream(pidFile, FileMode.Open, FileAccess.Write, FileShare.None); await Assert.ThrowsAsync<TimeoutException>( async () => await WaitForPidFileAsync(pidFile, TimeSpan.FromMilliseconds(300), ct)); ``` The old code threw `IOException` on the very first poll; the retry swallows it and runs out the clock. `TimeoutException` *is* 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 - 3/3 pass, both rewritten tests now finishing in well under a second instead of 15. - Regression coverage intact: with the `catch (IOException)` removed, `WaitForPidFile_WriterHoldsFileExclusively_RetriesInsteadOfThrowing` still fails in **72 ms**. - No production code touched — `DotNetService` is unchanged; this is tests only. This unblocks `build-and-package`, which is currently red on `main` and on #724. --------- Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: azchohfi <527713+azchohfi@users.noreply.github.com> Copilot-Session: a209763e-7185-4986-bf0b-98e52c06b4be
Nikola Metulev (nmetulev)
pushed a commit
that referenced
this pull request
Aug 14, 2026
## 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
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
RunDotnetProcessAsync_Cancellation_KillsProcessTreefails intermittently on CI — it just took downbuild-and-packageon #730, which is a docs/logging-only PR:Root cause
WaitForPidFileAsyncpolls for the PID file the spawned PowerShell root writes withSet-Content:File.Existsturns true the instantSet-Contentcreates the file, but PowerShell still holds the write handle for a short window afterwards. A read landing inside that window fails with a sharing violation, and the exception escaped the polling loop instead of being treated as "not ready yet".Reproduced directly rather than inferred:
That is exactly the CI error, and it explains why it's load-dependent: the window is tiny, so it only loses the race on a busy agent.
Fix
Catch the transient IO failure and keep polling. A sharing violation here means "not written yet" — precisely the condition the loop already exists to wait out.
An empty or partially written file needed no new handling: the existing
int.TryParseguard already keeps polling until the value parses.Validation
WaitForPidFileAsynchas exactly one caller, so the blast radius is that one test.check-docskeys offsrc/winapp-CLI/**/Commands/, and this is test-only.