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

[realtonyyoung]

Fixes AI-1645.

The filed diagnosis was wrong, and correcting it changes the fix

AI-1645 attributed the random UninstallCommandTests failures to a stale PID file or a recycled PID, and proposed validating the PID before killing.

That validation already exists — IsOurDaemon, since AI-839 (#147) — and it could not have helped here. The PID was neither stale nor recycled. It was the test runner's own live PID, carrying a start token that genuinely matched, so every identity check correctly passed. No amount of extra validation catches a PID that is legitimately valid.

What actually happens

Verified by reading the code, not inferred from the stack:

1. DaemonLockPaths selects its directory through a process-global static (OverrideDirectoryForTesting).
2. DaemonLockTests redirects it and acquires a real DaemonLock, which writes the acquiring process's own PID — its own assertion at line 185 proves this.
3. UninstallCommandTests is in a different NotInParallel group, so it can run concurrently — and it sees the same global override.
4. Uninstall runs daemon stop --yes, which enumerates that redirected directory, finds a live PID with a matching token, and calls Process.Kill(entireProcessTree: true) — on the tree containing itself.

Hence: a random uninstall test, an identical stack every time, not reproducible on demand.

This also explains why the existing per-test KCAP_CONFIG_DIR isolation never helped — the daemons directory deliberately ignores KCAP_CONFIG_DIR, which its own doc comment says explicitly.

Two changes

1. Test isolation — the actual flake fix. UninstallCommandTests joins the DaemonLockPaths.OverrideDirectoryForTesting constraint group. The rule worth remembering: a reader of a globally-selected directory has to exclude everyone who can reselect it. I swept all 12 mutator classes and every reader; uninstall was the only reader missing the key (DaemonRestartCommandTests only calls a pure parser).

2. Production guard. Refuse a PID equal to this process, and catch the InvalidOperationException for the ancestor case, which cannot be detected portably in advance. A daemon is never the process running a stop command, so reaching there means the PID file describes something it should not. Both paths now name the daemon and its PID file rather than surfacing an unattributed exception.

The production hazard is narrower than AI-1645 claimed — in real use the CLI and daemon are separate processes, so self-reference should not arise. I kept the guard anyway because it converts an opaque crash into a diagnosable message, which is what issue suggestion (2) asked for.

Mutation evidence, stated precisely

Removing both guards fails the new test with the exact CI error:

InvalidOperationException: 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 defence-in-depth, but it means the test pins the property (a self-referencing PID file never escapes as an unhandled exception), not each guard individually. That limitation is written into the test, so nobody mistakes it for per-guard coverage.

The first version of this test was vacuous, and mutation is the only reason I know that. It acquired a real DaemonLock — which leaves the PID file unreadable, so stop returned 1 down its "PID file unreadable" branch without ever reaching the kill. It passed with the guard removed. Rewritten to write the PID file directly with a real ProcessStartToken.ForPid token, so it reaches the code under test.

Verification

DaemonStopSelfPidTests 1/1 · DaemonLockTests 17/17.

UninstallCommandTests is 11/16 failing locally on macOS — and identically on origin/main, confirmed in a detached baseline worktree. Pre-existing and unrelated; CI (ubuntu/windows) is the gate for that class.

🤖 Generated with Claude Code

[linear-code]

AI-1645

[qodo-code-review]

PR Summary by Qodo

Prevent daemon stop from killing the current process via self-referencing PID files

🐞 Bug fix 🧪 Tests 🕐 20-40 Minutes

AI Description

• Refuse to stop a daemon when its PID file resolves to the current process.
• Improve error reporting when Process.Kill(tree) fails (e.g., ancestor PID case).
• Fix CI flake by serializing uninstall tests against daemon-directory override mutations.

Diagram

graph TD
  C["DaemonLockTests (override dir)"] --> B["DaemonLockPaths.OverrideDirectoryForTesting"] --> D[("Temp daemons dir")] --> E["DaemonCommands.StopByName"] --> F["Process.Kill(tree)"]
  A["UninstallCommandTests"] --> E --> G["Self/ancestor guard + message"]
  G -. "blocks/handles" .-> F

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Remove process-global daemon-dir override (inject directory resolver)

• ➕ Eliminates cross-test contamination risk entirely
• ➕ Makes production/test behavior easier to reason about (no hidden global state)
• ➖ Larger refactor across CLI/daemon/tests
• ➖ Higher risk of unintended behavior changes in path resolution

2. Scope the override with an IDisposable/RAII helper (push/pop stack)

• ➕ Reduces chance of leaked overrides in tests
• ➕ Makes override lifetime explicit and easier to audit
• ➖ Still global state; still requires strict serialization
• ➖ Requires careful nesting semantics and auditing existing callers

Recommendation: The PR’s approach is a good minimal-risk fix: it closes the immediate test parallelism hole by sharing the existing NotInParallel constraint key, and adds a targeted production guard that turns an opaque InvalidOperationException into a diagnosable error without changing normal daemon-stop behavior. A deeper refactor to eliminate the global override could be considered later if this pattern causes more concurrency hazards.

Files changed (3) +129 / -2

Bug fix (1) +32 / -1

DaemonCommands.csGuard 'daemon stop' against self/ancestor PID kills with clearer diagnostics +32/-1

Guard 'daemon stop' against self/ancestor PID kills with clearer diagnostics

• Adds a hard check rejecting PID files that resolve to the current process. Wraps 'Process.Kill(entireProcessTree: true)' to catch 'InvalidOperationException' (e.g., ancestor-process tree) and report the daemon name and PID file path instead of crashing.

src/Capacitor.Cli/Commands/DaemonCommands.cs

Tests (2) +97 / -1

DaemonStopSelfPidTests.csRegression test for self-referencing daemon PID file during stop +76/-0

Regression test for self-referencing daemon PID file during stop

• Introduces a unit regression test that writes a PID file naming the test runner with a real start token, then asserts 'daemon stop' returns cleanly (exit code 1) rather than throwing. Uses the DaemonLockPaths override serialization key to avoid cross-test interference.

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

UninstallCommandTests.csSerialize uninstall tests with daemon-directory override to prevent CI flake +21/-1

Serialize uninstall tests with daemon-directory override to prevent CI flake

• Expands the NotInParallel group list to include the DaemonLockPaths.OverrideDirectoryForTesting constraint key. Prevents uninstall’s 'daemon stop --yes' from enumerating a redirected daemons directory while other tests are mutating it.

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

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📜 Skill insights (0)

Context used

⚠️ Tickets: not configured — ticket URL found in PR but could not be fetched — check ticket provider credentials

1. Exited daemon treated failure ✓ Resolved 🐞 Bug ≡ Correctness

Description

StopByName now catches InvalidOperationException from Process.Kill and immediately returns 1,
but InvalidOperationException can also occur if the target daemon exits between GetProcessById
and Kill. This makes kcap daemon stop (and therefore kcap uninstall) spuriously fail and also
skips TryCleanupMarkersUnderLock, leaving stale markers behind.

Code

src/Capacitor.Cli/Commands/DaemonCommands.cs[R380-383]

+            try {
+                process.Kill(entireProcessTree: true);
+            } catch (InvalidOperationException ex) {
+                // The self-PID guard above covers the case we can name. This covers the rest of the

Evidence

The new catch returns 1 on any InvalidOperationException and exits before the unconditional
cleanup at the end of StopByName. UninstallCommand marks uninstall as failed if `daemon stop
--yes` returns non-zero, so this new failure code can incorrectly block uninstall cleanup even when
the daemon is already gone (a race ArgumentException already treats as success).

src/Capacitor.Cli/Commands/DaemonCommands.cs[348-409]
src/Capacitor.Cli/Commands/UninstallCommand.cs[83-112]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`StopByName` catches any `InvalidOperationException` thrown by `process.Kill(entireProcessTree: true)` and returns exit code `1` immediately. However, `InvalidOperationException` can also happen when the target process exits in the small race window between `Process.GetProcessById(entry.Pid)` and `Kill`. In that situation the daemon is already stopped and `daemon stop` should behave idempotently (similar to the existing `ArgumentException` path) and still run marker cleanup.

### Issue Context
`UninstallCommand` treats any non-zero exit from `DaemonCommands.HandleAsync(["daemon","stop","--yes"])` as a failure and will skip config-directory deletion, so a benign stop race can incorrectly make uninstall fail.

### Fix Focus Areas
- src/Capacitor.Cli/Commands/DaemonCommands.cs[378-408]

### What to change
- In the `catch (InvalidOperationException ex)` block, distinguish the benign case (process already exited) from the intentional self/ancestor process-tree protection:
 - If `process.HasExited` (and/or a `process.Refresh()` then `HasExited`) is true, treat it as “already stopped”: print the same style message as the `ArgumentException` path (or a concise equivalent), then continue to marker cleanup and return `0`.
 - Otherwise, keep returning `1` with the current diagnostic message.
- Ensure marker cleanup (`TryCleanupMarkersUnderLock(name)`) still runs in the benign-exited case (i.e., avoid the early return that skips it).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. AI-1645 in StopByName comment ✓ Resolved 📘 Rule violation ⚙ Maintainability

Description

Source comments and XML doc comments in both production and test code reference the Linear issue id
AI-1645, violating the requirement to avoid Linear identifiers in source comments. This leaks
non-portable/internal tracking IDs into the codebase and test suite.

Code

src/Capacitor.Cli/Commands/DaemonCommands.cs[R366-369]

+            // 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.
+            // (AI-1645: that is exactly how this surfaced, as a random kcap-cli CI failure.)

Evidence

PR Compliance ID 7 forbids including Linear issue IDs in source comments (e.g., AI-###). The cited
locations show direct uses of AI-1645: a comment in DaemonCommands.cs includes (AI-1645: ...),
the XML doc summary in DaemonStopSelfPidTests.cs begins with AI-1645 — ..., and a comment in
UninstallCommandTests contains DaemonLockPaths.OverrideDirectoryForTesting (AI-1645): ..., each
demonstrating the prohibited pattern in comments/documentation.

CLAUDE.md: Do Not Reference Linear Issue Numbers in Source Comments
src/Capacitor.Cli/Commands/DaemonCommands.cs[366-370]
test/Capacitor.Cli.Tests.Unit/Daemon/DaemonStopSelfPidTests.cs[6-8]
test/Capacitor.Cli.Tests.Unit/UninstallCommandTests.cs[16-17]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Comments and XML documentation include a Linear issue id (`AI-1645`), which is disallowed in source comments.

## Issue Context
Per policy (PR Compliance ID 7), source comments must not reference Linear IDs. If an issue reference is essential, replace it with a GitHub issue number; otherwise remove the reference entirely.

## Fix Focus Areas
- src/Capacitor.Cli/Commands/DaemonCommands.cs[366-369]
- test/Capacitor.Cli.Tests.Unit/Daemon/DaemonStopSelfPidTests.cs[6-8]
- test/Capacitor.Cli.Tests.Unit/UninstallCommandTests.cs[16-16]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. Overly verbose Uninstall comments ✓ Resolved 📘 Rule violation ⚙ Maintainability

Description

Several newly added comments/XML documentation blocks are disproportionately long and include
detailed execution/history narratives (test parallelism constraints, CI/mutation background, and
operational context), which reduces scanability and makes the files harder to read. This goes beyond
the minimal context needed and conflicts with the guideline to keep comments concise and prefer
self-explanatory code.

Code

test/Capacitor.Cli.Tests.Unit/UninstallCommandTests.cs[R16-19]

+// DaemonLockPaths.OverrideDirectoryForTesting (AI-1645): uninstall runs
+// `daemon stop --yes`, which ENUMERATES every name in the daemons directory and
+// kills the PID it finds. That directory is selected by a process-global static,
+// so while any daemon test holds the override, uninstall reads THAT test's temp

Evidence

PR Compliance ID 8 requires minimal/concise comments that add only necessary context. The cited
areas introduce multi-line blocks (roughly 10–15+ lines, including multi-paragraph XML
documentation) that describe detailed steps, background/CI behavior, mutation evidence, and
error-text/context rather than a short statement of test intent or a brief rationale (e.g., why a
shared NotInParallel key is used); since the code already conveys much of the behavior, the extra
narrative lowers signal-to-noise and violates the “minimal comments” requirement.

CLAUDE.md: Keep Comments Minimal; Prefer Self-Explanatory Code
test/Capacitor.Cli.Tests.Unit/UninstallCommandTests.cs[16-29]
test/Capacitor.Cli.Tests.Unit/Daemon/DaemonStopSelfPidTests.cs[6-36]
src/Capacitor.Cli/Commands/DaemonCommands.cs[360-369]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Multiple new comment/documentation blocks are excessively verbose and include detailed narrative (parallelism constraints history, CI/mutation-testing background, and operational context). Shorten these to the minimum necessary rationale/intent, relying on the code/tests to express behavior and keeping deeper incident history outside source comments.

## Issue Context
PR Compliance ID 8 calls for minimal comments. Keep only the essential explanation needed to justify the shared `NotInParallel` key and the intent of the tests/guards; avoid multi-paragraph historical background, CI behavior descriptions, mutation-testing narrative, and lengthy restatement of what the code already makes clear.

## Fix Focus Areas
- test/Capacitor.Cli.Tests.Unit/UninstallCommandTests.cs[16-29]
- test/Capacitor.Cli.Tests.Unit/Daemon/DaemonStopSelfPidTests.cs[6-36]
- src/Capacitor.Cli/Commands/DaemonCommands.cs[360-369]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

_{To customize comments, go to the Qodo configuration screen, or learn more in the docs.}

[realtonyyoung]

/agentic_review

[qodo-code-review]

Code review by qodo was updated up to the latest commit 74b5a7c