Skip to content

Fix the two flaky tests taking down build-and-package #751

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Nikola Metulev (nmetulev) merged 1 commit into from
Aug 14, 2026
Merged

Fix the two flaky tests taking down build-and-package #751

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
179 changes: 166 additions & 13 deletions src/winapp-CLI/WinApp.Cli.Tests/DotNetServiceTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1704,13 +1704,7 @@ public async Task RunDotnetProcessAsync_Cancellation_KillsProcessTree()
// with -PassThru, writes ping's PID to a temp file, then blocks on Wait-Process. The test polls
// that file — no WMI or Win32 process enumeration required.
var pidFile = Path.Join(Path.GetTempPath(), $"winapp_treekill_{Guid.NewGuid():N}.pid");
// Escape single quotes for the single-quoted PowerShell string literal so a temp path
// containing an apostrophe (e.g. a profile like O'Brien) still produces a valid script.
var pidFileLiteral = pidFile.Replace("'", "''");
var script =
"$p = Start-Process -FilePath ping.exe -ArgumentList '-n','60','127.0.0.1' -PassThru -WindowStyle Hidden; " +
$"Set-Content -LiteralPath '{pidFileLiteral}' -Value $p.Id; " +
"Wait-Process -Id $p.Id";
var script = BuildTreeKillScript(pidFile, "ping.exe");

var startInfo = new ProcessStartInfo
{
Expand Down Expand Up @@ -1744,8 +1738,11 @@ public async Task RunDotnetProcessAsync_Cancellation_KillsProcessTree()
rootPid = rootPidValue;
Assert.IsFalse(Process.GetProcessById(rootPidValue).HasExited, "The root process should be running before cancellation.");

// Capture the ping descendant via the PID file the root script writes.
var pingPidValue = await WaitForPidFileAsync(pidFile, TimeSpan.FromSeconds(15), TestContext.CancellationToken);
// Capture the ping descendant via the PID file the root script writes. The run task is passed
// in so a root that dies before publishing fails here with its own stderr rather than after a
// silent wait. The budget is generous because it costs nothing on success (a healthy spawn
// publishes in well under a second) and only bounds a genuinely wedged agent.
var pingPidValue = await WaitForPidFileAsync(pidFile, TimeSpan.FromSeconds(30), TestContext.CancellationToken, runTask);
pingPid = pingPidValue;
Assert.IsFalse(Process.GetProcessById(pingPidValue).HasExited, "The ping descendant should be running before cancellation.");

Expand Down Expand Up @@ -1795,6 +1792,38 @@ public async Task RunDotnetProcessAsync_Cancellation_KillsProcessTree()
}
}

/// <summary>
/// Builds the root script for the tree-kill test: start <paramref name="childExe"/>, publish its PID
/// to <paramref name="pidFile"/>, then block until it exits.
/// </summary>
/// <remarks>
/// <c>$ErrorActionPreference = 'Stop'</c> is what keeps the published PID file honest, and it is the
/// fix for a CI flake rather than boilerplate. A <c>Start-Process</c> failure is a NON-terminating
/// error by default, so the script used to carry straight on to <c>Set-Content</c> with <c>$p</c>
/// still null, publish a 0-byte file, and leave the reader polling a file that would never parse
/// until its whole budget expired — reporting that the file "was not written" about a file that had
/// in fact been written, just empty. Stopping on the error means the root exits instead, with the
/// real reason on stderr for <see cref="WaitForPidFileAsync"/> to surface.
/// <para>
/// Shared with the regression test so the script proven not to publish an empty file is the very
/// script the tree-kill test runs, and the two cannot drift apart.
/// </para>
/// </remarks>
private static string BuildTreeKillScript(string pidFile, string childExe)
{
// Escape single quotes for the single-quoted PowerShell string literals so a temp path
// containing an apostrophe (e.g. a profile like O'Brien) still produces a valid script.
var pidFileLiteral = pidFile.Replace("'", "''");
var childExeLiteral = childExe.Replace("'", "''");

return
"$ErrorActionPreference = 'Stop'; " +
$"$p = Start-Process -FilePath '{childExeLiteral}' -ArgumentList '-n','60','127.0.0.1' -PassThru -WindowStyle Hidden; " +
"if ($null -eq $p) { throw 'Start-Process returned no process object' }; " +
$"Set-Content -LiteralPath '{pidFileLiteral}' -Value $p.Id; " +
"Wait-Process -Id $p.Id";
}

/// <summary>Best-effort force-kill of a process (and its tree) by id, ignoring already-exited processes.</summary>
private static void KillProcessTreeIfRunning(int? pid)
{
Expand Down Expand Up @@ -1875,17 +1904,31 @@ private static async Task<bool> WaitForProcessExitAsync(int pid, CancellationTok
/// </para>
/// An empty or partially written file is covered by the existing <see cref="int.TryParse(string, out int)"/>
/// guard, which simply keeps polling until the value parses.
/// <para>
/// <paramref name="rootTask"/> is the optional launcher task for the process that publishes the file.
/// The PID can only ever arrive from that process, so once it has exited the wait is already lost and
/// running out the clock just delays the failure and throws away the reason. Awaiting the completed
/// task yields its exit code and captured stderr, turning a blind "not written" timeout into the
/// actual error the script reported.
/// </para>
/// </remarks>
private static async Task<int> WaitForPidFileAsync(string pidFile, TimeSpan timeout, CancellationToken cancellationToken)
private static async Task<int> WaitForPidFileAsync(
string pidFile,
TimeSpan timeout,
CancellationToken cancellationToken,
Task<(int ExitCode, string Output, string Error)>? rootTask = null)
{
var deadline = DateTime.UtcNow + timeout;
var everExisted = false;
string? lastRead = null;
while (DateTime.UtcNow < deadline)
{
string? text = null;
try
{
if (File.Exists(pidFile))
{
everExisted = true;
text = (await File.ReadAllTextAsync(pidFile, cancellationToken)).Trim();
}
}
Expand All @@ -1894,17 +1937,41 @@ private static async Task<int> WaitForPidFileAsync(string pidFile, TimeSpan time
// Still being written (or replaced) by the root script — fall through and retry.
}

if (text is not null && int.TryParse(text, out var pid))
if (text is not null)
{
return pid;
lastRead = text;
if (int.TryParse(text, out var pid))
{
return pid;
}
}

if (rootTask is { IsCompleted: true })
{
// Awaiting a faulted task rethrows the original exception, which is at least as
// informative as anything this method could compose.
var (exitCode, output, error) = await rootTask;
throw new InvalidOperationException(
$"The root process exited with code {exitCode} before publishing a descendant PID to " +
$"'{pidFile}'. stderr: {DescribeStream(error)} | stdout: {DescribeStream(output)}");
}

await Task.Delay(20, cancellationToken);
}

throw new TimeoutException($"The descendant PID file '{pidFile}' was not written within {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)",
_ => $"it was created but never contained a parsable PID (last read: '{lastRead}')",
};
throw new TimeoutException($"The descendant PID file '{pidFile}' was not usable within {timeout}: {state}.");
}

/// <summary>Renders a captured stdio stream for a failure message, collapsing it onto one line.</summary>
private static string DescribeStream(string? stream)
=> string.IsNullOrWhiteSpace(stream) ? "(empty)" : stream.Trim().ReplaceLineEndings(" ");

#endregion

#region PID file read (flake regression)
Expand Down Expand Up @@ -1981,6 +2048,92 @@ await Assert.ThrowsAsync<TimeoutException>(
Assert.IsTrue(File.Exists(pidFile), "The file existed throughout; the timeout came from the parse guard, not a missing file.");
}

[TestMethod]
public async Task WaitForPidFile_RootExitedWithoutPublishing_FailsFastWithTheRootsError()
{
// The PID can only ever come from the root process, so once it has exited the wait is already
// lost. The old helper had no idea the root was gone and kept polling to the end of its budget,
// then reported "was not written" -- true, but silent about why. This is the diagnosis half of
// the CI flake fix: the failure must name the root's exit code and stderr.
var pidFile = Path.Join(_tempDirectory.FullName, $"rootdied_{Guid.NewGuid():N}.pid");
var rootTask = Task.FromResult((ExitCode: 1, Output: "some stdout", Error: "Start-Process : cannot find the file"));

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.");
Comment on lines +2061 to +2069
}

[TestMethod]
public async Task WaitForPidFile_NeverWritten_TimeoutSaysSo()
{
// The timeout message has to distinguish "never created" from "created but unusable", because
// the two point at completely different causes and the old wording ("was not written") was
// actively misleading for the second one.
var pidFile = Path.Join(_tempDirectory.FullName, $"missing_{Guid.NewGuid():N}.pid");

var ex = await Assert.ThrowsAsync<TimeoutException>(
async () => await WaitForPidFileAsync(pidFile, TimeSpan.FromMilliseconds(200), TestContext.CancellationToken));

StringAssert.Contains(ex.Message, "never created");
}

[TestMethod]
public async Task WaitForPidFile_EmptyFile_TimeoutReportsItWasCreatedButUnparsable()
{
// The exact shape of the CI flake: Set-Content published a 0-byte file, so the file WAS written
// and the old "was not written within 00:00:15" message sent anyone reading it down the wrong path.
var pidFile = Path.Join(_tempDirectory.FullName, $"emptymsg_{Guid.NewGuid():N}.pid");
await File.WriteAllTextAsync(pidFile, string.Empty, TestContext.CancellationToken);

var ex = await Assert.ThrowsAsync<TimeoutException>(
async () => await WaitForPidFileAsync(pidFile, TimeSpan.FromMilliseconds(200), TestContext.CancellationToken));

StringAssert.Contains(ex.Message, "never contained a parsable PID");
}

[TestMethod]
public async Task TreeKillScript_WhenTheChildCannotStart_PublishesNoPidFileAtAll()
{
// Root-cause regression guard. Start-Process failure is a NON-terminating error, so without
// $ErrorActionPreference = 'Stop' the script ran on to Set-Content with a null $p and left a
// 0-byte PID file behind -- which the reader then polled until it timed out. Measured against
// the unhardened script, this produced a zero-length file every time.
//
// Driving the real script through a real PowerShell is the point: the bug lived in PowerShell's
// error semantics, so nothing short of running it can prove the behaviour.
var pidFile = Path.Join(_tempDirectory.FullName, $"nostart_{Guid.NewGuid():N}.pid");
var script = BuildTreeKillScript(pidFile, "winapp_no_such_binary_" + Guid.NewGuid().ToString("N") + ".exe");

var startInfo = new ProcessStartInfo
{
FileName = "powershell.exe",
RedirectStandardOutput = true,
RedirectStandardError = true,
UseShellExecute = false,
CreateNoWindow = true,
};
startInfo.ArgumentList.Add("-NoProfile");
startInfo.ArgumentList.Add("-NonInteractive");
startInfo.ArgumentList.Add("-Command");
startInfo.ArgumentList.Add(script);

using var process = Process.Start(startInfo)!;
var stderr = await process.StandardError.ReadToEndAsync(TestContext.CancellationToken);
await process.WaitForExitAsync(TestContext.CancellationToken);

Assert.IsFalse(
File.Exists(pidFile),
$"A failed child start must abort the script before Set-Content, leaving no PID file to mislead the reader. stderr: {stderr}");
Assert.AreNotEqual(0, process.ExitCode, "The script must report the failure through its exit code.");
StringAssert.Contains(stderr, "Start-Process", "The reason has to reach stderr, which is what the reader surfaces.");
}

#endregion

#region RunDotnetCommandAsync cancellation (process-tree kill)
Expand Down
74 changes: 50 additions & 24 deletions src/winapp-CLI/WinApp.Cli.Tests/TestProcessDump.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,15 @@ internal static class TestProcessDump
/// can enumerate the CLR. Returns the dump path or <c>null</c> (with a reason) when not possible.
/// The caller owns deleting the returned file.
/// </summary>
/// <remarks>
/// The child proves it is ready and the dump is taken only once that proof arrives, because a host
/// that has been <em>started</em> is not yet a host with a walkable CLR in it. A fixed 1.5s sleep
/// stood in for that proof and did not hold on a loaded agent: CI captured dumps whose only stack
/// was <c>hostfxr</c>/<c>hostpolicy</c> still loading coreclr, so ClrMD had nothing to enumerate,
/// the analysis reported "No CLR runtime found in dump (native-only crash)", and the managed
/// assertions failed intermittently. See <see cref="ManagedChildCandidates"/> for what the child
/// does before signalling and why merely reaching managed code is not enough on its own.
/// </remarks>
public static string? TryCreateManagedDump(out string? error)
{
error = null;
Expand All @@ -104,28 +113,27 @@ internal static class TestProcessDump
CreateNoWindow = true,
RedirectStandardInput = true,
RedirectStandardOutput = true,
RedirectStandardError = true,
});

if (child == null)
{
continue;
}

// Give the runtime time to fully spin up its heap before dumping.
Thread.Sleep(1500);
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 */ } });

if (!WaitForToken(child, ManagedReadyToken, TimeSpan.FromSeconds(30)))
{
error = $"managed child did not emit readiness token '{ManagedReadyToken}' (host {fileName})";
KillQuietly(child);
child = null;
continue;
}

var service = new CrashDumpService(
new TestConsole(),
NullLogger<CrashDumpService>.Instance,
new FakeXamlTriageService());

var dumpPath = service.WriteMiniDump(
(uint)child.Id, savedContext: null, savedThreadId: 0,
savedExceptionCode: 0, savedExceptionAddress: 0);
var dumpPath = DumpRunningChild(child);
if (dumpPath != null)
{
return dumpPath;
Expand All @@ -137,18 +145,9 @@ internal static class TestProcessDump
}
finally
{
try
{
if (child != null && !child.HasExited)
{
child.Kill(entireProcessTree: true);
}

child?.Dispose();
}
catch
if (child != null)
{
// best effort
KillQuietly(child);
}
}
}
Expand All @@ -157,10 +156,37 @@ internal static class TestProcessDump
return null;
}

/// <summary>Readiness token the benign managed child emits once its CLR is up and its heap is warm.</summary>
private const string ManagedReadyToken = "MANAGED_READY";

private static IEnumerable<(string FileName, string Args)> ManagedChildCandidates()
{
yield return ("pwsh.exe", "-NoLogo -NoProfile -Command \"Start-Sleep -Seconds 120\"");
yield return ("powershell.exe", "-NoLogo -NoProfile -Command \"Start-Sleep -Seconds 120\"");
// The child allocates, forces a blocking collection, and only then reports readiness, so the
// token means "the runtime is up AND its heap is populated and settled" rather than merely "a
// process exists". Both halves matter and neither is decoration:
//
// - Signalling from managed code is what rules out dumping a host still in NATIVE startup.
// That was the CI failure: dumps whose only stack was hostfxr/hostpolicy loading coreclr, so
// ClrMD found no runtime and the analysis reported a native-only crash.
// - Allocating and collecting first is what rules out the opposite mistake. A token emitted by
// the first line of managed code proves too little -- ClrMD needs walkable GC structures, and
// dumping that early trades the old race for a new one (measured: signalling immediately made
// this test fail or go inconclusive on a machine where the previous code passed).
//
// [Console]::Out with an explicit Flush, not Write-Object: the token must reach the reader as
// soon as it is written, which only an unbuffered write to the real stdout handle guarantees
// once the stream is redirected. Single quotes throughout so the script survives being embedded
// in the double-quoted -Command argument.
const string script =
"$sink = 1..2000 | ForEach-Object { [pscustomobject]@{ I = $_; S = 'warm' + $_ } }; " +
"[GC]::Collect(); " +
"[GC]::WaitForPendingFinalizers(); " +
$"[Console]::Out.WriteLine('{ManagedReadyToken}'); " +
"[Console]::Out.Flush(); " +
"Start-Sleep -Seconds 120";

yield return ("pwsh.exe", $"-NoLogo -NoProfile -Command \"{script}\"");
yield return ("powershell.exe", $"-NoLogo -NoProfile -Command \"{script}\"");
}

/// <summary>
Expand Down
Loading