Refuse to stop a daemon whose PID file names this process (AI-1645)

src/Capacitor.Cli/Commands/DaemonCommands.cs

@@ -357,12 +357,60 @@ static int StopByName(string name) {
                 return 0;
             }
 
+            // Refuse to kill ourselves. IsOurDaemon has already said this PID is a live process
+            // whose start token matches, so every identity check upstream has PASSED — the PID is
+            // simply not a daemon. A real daemon is never the process running `daemon stop`, so
+            // reaching here means the PID file is describing something it should not, and killing
+            // its tree would take down this process and everything above it.
+            //
+            // Without this, Process.Kill(entireProcessTree: true) throws
+            // "Cannot be used to terminate a process tree containing the calling process" — an
+            // opaque InvalidOperationException that says nothing about WHICH pid file caused it.
+            // That is exactly how this surfaced: a random, always-different CI test failing
+            // with an identical stack, because the pid file named the test runner itself.
+            if (entry.Pid == Environment.ProcessId) {
+                Console.Error.WriteLine(
+                    $"Daemon '{name}' resolves to the current process (PID {entry.Pid}); refusing to stop it. "
+                  + $"Its PID file at {DaemonLockPaths.PidPath(name)} does not describe a daemon.");
+
+                return 1;
+            }
+
             var process = Process.GetProcessById(entry.Pid);
-            process.Kill(entireProcessTree: true);
-            // Wait for it to actually exit so the kernel releases its flock before
-            // we try to reclaim it for cleanup below.
-            try { process.WaitForExit(5000); } catch { /* best-effort */ }
-            Console.Out.WriteLine($"Daemon '{name}' stopped (PID {entry.Pid}).");
+            var killed  = false;
+
+            try {
+                process.Kill(entireProcessTree: true);
+                killed = true;
+            } catch (InvalidOperationException) when (process.HasExited) {
+                // A BENIGN race, not corruption: the daemon exited on its own between
+                // GetProcessById and Kill. .NET reports that as InvalidOperationException too, so
+                // without this arm an ordinary well-timed stop would be accused of having a
+                // malformed PID file. The outcome the caller asked for has happened — the daemon is
+                // gone — so report it like the ArgumentException path below and fall through to the
+                // marker cleanup.
+                //
+                // No test: reproducing it needs the process to exit inside that window, which cannot
+                // be scheduled deterministically. Distinguished by HasExited rather than by message.
+                Console.Out.WriteLine($"Daemon '{name}' was not running.");
+            } catch (InvalidOperationException ex) {
+                // What remains is the safety rejection: .NET refuses to kill a process tree that
+                // contains the caller. The self-PID guard above handles the case we can name in
+                // advance; this covers the ANCESTOR case, which cannot be detected portably. Report
+                // which daemon and which PID rather than letting an unattributed exception escape.
+                Console.Error.WriteLine(
+                    $"Daemon '{name}' (PID {entry.Pid}) could not be stopped: {ex.Message} "
+                  + $"Its PID file at {DaemonLockPaths.PidPath(name)} appears not to describe a daemon.");
+
+                return 1;
+            }
+
+            if (killed) {
+                // Wait for it to actually exit so the kernel releases its flock before
+                // we try to reclaim it for cleanup below.
+                try { process.WaitForExit(5000); } catch { /* best-effort */ }
+                Console.Out.WriteLine($"Daemon '{name}' stopped (PID {entry.Pid}).");
+            }
         } catch (ArgumentException) {
             Console.Out.WriteLine($"Daemon '{name}' was not running.");
         }

test/Capacitor.Cli.Tests.Unit/Daemon/DaemonStopSelfPidTests.cs

@@ -0,0 +1,73 @@
+using Capacitor.Cli.Commands;
+using Capacitor.Cli.Core;
+
+namespace Capacitor.Cli.Tests.Unit.Daemon;
+
+/// <summary>
+/// <c>daemon stop</c> must refuse to kill a PID that is this very process.
+///
+/// <para>This is a REGRESSION test for a real kcap-cli CI failure, and it reproduces it exactly
+/// rather than approximating it. A live <see cref="DaemonLock"/> writes the acquiring process's own
+/// pid and start token, so after acquiring one here, the daemons directory contains a pid file that
+/// names the test runner. Every identity check in <c>StopByName</c> then legitimately passes — the
+/// process exists, and its start token matches — and the old code proceeded to
+/// <c>Process.Kill(entireProcessTree: true)</c> on itself, which .NET refuses with
+/// <c>InvalidOperationException: Cannot be used to terminate a process tree containing the calling
+/// process</c>.</para>
+///
+/// <para>The point worth keeping: the pid was never stale or recycled, so no amount of extra
+/// validation could have caught it. A daemon simply is never the process running <c>daemon stop</c>,
+/// which is why identity is the wrong question and self-reference is the right one.</para>
+///
+/// <para>In CI this arrived as a random <c>UninstallCommandTests</c> failure — uninstall runs
+/// <c>daemon stop --yes</c>, which enumerates the daemons directory, and that directory is chosen by
+/// a process-global static that the daemon tests redirect. That parallelism hole is fixed separately
+/// by the constraint key on <c>UninstallCommandTests</c>; this test covers the production behaviour
+/// on its own, so the guard survives even if the test-isolation fix is later refactored away.</para>
+///
+/// <para><b>Mutation evidence, stated precisely.</b> Removing BOTH the self-pid guard and the
+/// <c>InvalidOperationException</c> catch fails this test with the exact CI error
+/// ("Cannot be used to terminate a process tree containing the calling process"). Removing either
+/// one ALONE does not — the other still returns a clean exit code. That is deliberate: they are two
+/// layers over the same hazard, the second covering the ancestor case that cannot be detected
+/// portably in advance. So this test pins the PROPERTY (a self-referencing pid file never escapes as
+/// an unhandled exception) rather than either individual guard, and it is worth knowing that is what
+/// it does — a future reader deleting one layer will not be told by this test.</para>
+/// </summary>
+[NotInParallel(nameof(DaemonLockPaths) + ".OverrideDirectoryForTesting")]
+public class DaemonStopSelfPidTests {
+    [Test]
+    public async Task Stop_refuses_to_kill_a_pid_file_naming_the_current_process() {
+        var dir = Path.Combine(Path.GetTempPath(), "kcap-stop-self-tests", Guid.NewGuid().ToString("N"));
+        Directory.CreateDirectory(dir);
+        DaemonLockPaths.OverrideDirectoryForTesting(dir);
+
+        try {
+            // Write the pid file directly rather than acquiring a real DaemonLock. The first
+            // version of this test DID acquire one — and was vacuous: a held lock leaves the pid
+            // file unreadable, so StopByName returns 1 down its "PID file unreadable" branch and
+            // never reaches the kill at all. It passed with the guard removed, which is the only
+            // reason that was caught. What matters is reaching the kill, not how the file got there.
+            //
+            // The token must be the REAL one for this pid: that is what makes IsOurDaemon return
+            // true, which is the whole point. A fabricated token would be rejected earlier and the
+            // test would again pass for the wrong reason.
+            var token = ProcessStartToken.ForPid(Environment.ProcessId);
+
+            await File.WriteAllTextAsync(
+                DaemonLockPaths.PidPath("self"), $"{Environment.ProcessId}\n{token}\n");
+
+            // Before the fix this threw InvalidOperationException out of Process.Kill. The exit code
+            // matters less than the fact that it RETURNS: an unhandled exception here takes down
+            // whichever unrelated test happens to be running — which is exactly how it showed up in
+            // CI, as a random UninstallCommandTests failure.
+            var exit = await DaemonCommands.HandleAsync(["daemon", "stop", "--name", "self", "--yes"]);
+
+            await Assert.That(exit).IsEqualTo(1);
+        } finally {
+            DaemonLockPaths.OverrideDirectoryForTesting(null);
+
+            try { Directory.Delete(dir, recursive: true); } catch { /* best effort */ }
+        }
+    }
+}

test/Capacitor.Cli.Tests.Unit/UninstallCommandTests.cs

@@ -12,7 +12,20 @@ namespace Capacitor.Cli.Tests.Unit;
 // ConfigDirEnvVar: uninstall reads KCAP_CONFIG_DIR fresh on every call,
 // so we mutate it per-test to point at the test's temp dir without disturbing
 // the assembly-wide value pinned by RepoPathStoreGlobalSetup.
-[NotInParallel(["HomeEnvVarMutation", "ConfigDirEnvVar", "CwdMutation"])]
+//
+// DaemonLockPaths.OverrideDirectoryForTesting: uninstall runs `daemon stop --yes`,
+// which enumerates the daemons directory and kills the PIDs it finds. That directory
+// comes from a process-global static, so a concurrent daemon test's override sends
+// uninstall at that test's temp dir — where a real DaemonLock has written the test
+// runner's own PID. Uninstall then kills the tree containing itself. The
+// KCAP_CONFIG_DIR isolation above cannot help; that directory ignores it by design.
+// See DaemonStopSelfPidTests for the full mechanism.
+[NotInParallel([
+    "HomeEnvVarMutation",
+    "ConfigDirEnvVar",
+    "CwdMutation",
+    nameof(DaemonLockPaths) + ".OverrideDirectoryForTesting"
+])]
 public class UninstallCommandTests {
     [Test]
     public async Task User_level_uninstall_removes_kcap_entries_and_preserves_user_data() {