diff --git a/src/Capacitor.Cli.Daemon/Services/ProcessTree.cs b/src/Capacitor.Cli.Daemon/Services/ProcessTree.cs index 015cfba618..659aa3a724 100644 --- a/src/Capacitor.Cli.Daemon/Services/ProcessTree.cs +++ b/src/Capacitor.Cli.Daemon/Services/ProcessTree.cs @@ -50,13 +50,46 @@ public static void Kill(int pid) { if (ProcessIdentity.Capture(pid) is { } identity) KillSubtree(pid, identity); } + /// Kills only while still carries . Use + /// when the caller captured the identity itself — otherwise the bare would + /// take a FRESH identity, and a pid recycled between the caller's capture and here would let it + /// signal the replacement process. A mismatch is a no-op, not a failure. + public static void Kill(int pid, string identity) { + if (pid <= 0) return; + + if (OperatingSystem.IsWindows()) { + try { + // Open FIRST, then derive the token from the SAME handle we are about to kill — + // checking a pid-based token before opening would let a recycled pid slip through. + using var process = Process.GetProcessById(pid); + // Force the Process to retain its long-term SafeHandle: StartTime and Kill each + // otherwise open their OWN short-term handle, letting the pid be reused between + // them. Reading Handle pins one handle for both. + _ = process.Handle; + if ($"tk:{process.StartTime.ToUniversalTime().Ticks}" != identity) return; + Kill(process); + } catch (ArgumentException) { /* already gone */ } + return; + } + + if (ProcessIdentity.Matches(pid, identity)) KillSubtree(pid, identity); + } + static void KillSubtree(int pid, string identity) { var seen = new HashSet(); for (var rescan = 0; rescan <= MaxRescans; rescan++) { var fresh = false; - foreach (var child in Children(pid)) { + // The listing is trusted only when bracketed: the root must be the captured incarnation just + // before it and not proven a different one just after. A root that exited in between leaves + // its listed children to be killed, not orphaned; a pid proven reused leaves them alone. What + // remains is a reuse, plus the replacement spawning and exiting, inside one listing. + if (!ProcessIdentity.Matches(pid, identity)) break; + var children = Children(pid).ToList(); + if (ProcessIdentity.MatchesTri(pid, identity) == false) return; + + foreach (var child in children) { if (!seen.Add(child)) continue; // handled on an earlier pass; a zombie stays listed until its parent dies fresh = true; diff --git a/test/Capacitor.Cli.Daemon.Tests.Unit/DaemonRunnerVersionProbeTests.cs b/test/Capacitor.Cli.Daemon.Tests.Unit/DaemonRunnerVersionProbeTests.cs index 04e3cfbb68..8c5c8ae8ae 100644 --- a/test/Capacitor.Cli.Daemon.Tests.Unit/DaemonRunnerVersionProbeTests.cs +++ b/test/Capacitor.Cli.Daemon.Tests.Unit/DaemonRunnerVersionProbeTests.cs @@ -99,18 +99,74 @@ public async Task A_descendant_holding_the_pipe_open_is_bounded_and_still_reads_ using var tmp = new TempDir(); // Prints the version, then leaves a child holding the inherited stdout open after the CLI // itself exits — the shape that made ProbeCliVersionOnce block on a drain that never saw EOF. + // The stub records the descendant's pid so the test can reap it: once reparented it is no + // longer the test's child to wait on, and left alone it outlives the test host into teardown. The marker + // in its command line is what proves a pid is still that descendant when the test reaps it. + // (`; :` keeps the shell from exec-ing sleep, which would drop the marker.) + var pidFile = tmp.PathTo("faketool.pid"); + var marker = $"kcap-probe-descendant-{Guid.NewGuid():N}"; var cli = tmp.CreateExecutable( - "faketool", "#!/bin/sh\necho 'faketool 9.9.9'\nsleep 20 &\nexit 0\n"); + "faketool", $"#!/bin/sh\necho 'faketool 9.9.9'\nsh -c 'sleep 20; :' {marker} &\necho $! > {ShellQuote(pidFile)}\nexit 0\n"); var sw = System.Diagnostics.Stopwatch.StartNew(); - var version = await Task.Run(() => DaemonRunner.ProbeCliVersionForLaunch(cli)); - sw.Stop(); - - // Bounded: a return well under the descendant's 20s lifetime proves the drain was not blocked - // to an EOF that never comes. - await Assert.That(sw.Elapsed).IsLessThan(TimeSpan.FromSeconds(12)); - // Recovered: the version printed ahead of the block survives, so registration advertises it - // rather than an unknown version that would reject a launch. - await Assert.That(version).IsEqualTo("9.9.9"); + string? version = null; + try { + version = await Task.Run(() => DaemonRunner.ProbeCliVersionForLaunch(cli)); + sw.Stop(); + + // Bounded: a return well under the descendant's 20s lifetime proves the drain was not + // blocked to an EOF that never comes. + await Assert.That(sw.Elapsed).IsLessThan(TimeSpan.FromSeconds(12)); + // Recovered: the version printed ahead of the block survives, so registration advertises + // it rather than an unknown version that would reject a launch. + await Assert.That(version).IsEqualTo("9.9.9"); + } finally { + // In a finally so a failing probe or assertion still reaps — otherwise the same leaked + // descendant turns a useful assertion failure into an exit-139 teardown failure. + await ReapDescendantAsync(pidFile, marker); + } + } + + /// Kills the reparented descendant so it cannot linger past this test into the host's + /// teardown. A pid is killed only when its captured incarnation still carries this test's marker: + /// a descendant that already exited may have had its pid reused, and that process is left alone. + static async Task ReapDescendantAsync(string pidFile, string marker) { + int pid = 0; + for (var i = 0; i < 50; i++) { + if (File.Exists(pidFile) && int.TryParse((await File.ReadAllTextAsync(pidFile)).Trim(), out pid)) break; + await Task.Delay(20); + } + if (pid <= 0) return; + + // Capture throws both when the pid is gone AND when a live process's identity is unreadable, + // so a bare catch is not proof of absence. + string identity; + try { identity = Capacitor.Tests.Helpers.PidIdentity.Capture(pid); } + catch { + try { + using var process = System.Diagnostics.Process.GetProcessById(pid); + if (process.HasExited) return; + } catch (ArgumentException) { return; } // genuinely gone + throw; // present but identity unreadable — do not assume it is gone + } + + // The marker is read after the capture and the identity checked again after the read, so the + // command line that carried the marker belongs to the captured incarnation. + if (!(await CommandLineAsync(pid)).Contains(marker, StringComparison.Ordinal)) return; + if (Capacitor.Tests.Helpers.PidIdentity.IsGone(pid, identity)) return; + + Capacitor.Cli.Daemon.Services.ProcessTree.Kill(pid, identity); + await Capacitor.Tests.Helpers.PidIdentity.WaitUntilGoneAsync(pid, identity, TimeSpan.FromSeconds(5)); + } + + static string ShellQuote(string value) => "'" + value.Replace("'", "'\\''", StringComparison.Ordinal) + "'"; + + static async Task CommandLineAsync(int pid) { + using var ps = System.Diagnostics.Process.Start(new System.Diagnostics.ProcessStartInfo("ps", ["-o", "args=", "-p", pid.ToString(System.Globalization.CultureInfo.InvariantCulture)]) { + RedirectStandardOutput = true, UseShellExecute = false, + })!; + var output = await ps.StandardOutput.ReadToEndAsync(); + await ps.WaitForExitAsync(); + return output; } }