fix(supervisor): require a start time to identify an orphaned process

[jdx]

The hole

Orphan cleanup verifies that a live PID really belongs to the daemon recorded against it before adopting or killing it. Identity is the kernel start time — but when a record had no start time, process_identity_matches fell back to comparing the process name:

match recorded_start_time {
    Some(recorded) => current_start_time == Some(recorded),
    None => matches!(
        (recorded_title, current_title),
        (Some(recorded), Some(current)) if recorded == current
    ),
}

A name is not an identity. It is exactly what a second copy of the same program shares, so a recycled PID running node, nginx, or postgres matches a legacy record for a different daemon of the same program — and the supervisor then adopts it, or kills it under orphan_policy = "kill". That is the failure mode the identity check exists to prevent, left open for precisely the records that predate the check.

The fallback was kept in #632 to avoid breaking the ≤2.17.0 upgrade path. It buys very little: the window is one supervisor restart after upgrading, and every record written since then has a start time.

The change

Both start times are required. A missing one on either side means unverifiable, which is treated differently from mismatched:

• mismatched — a stranger holds the PID; reset the record (or kill, per policy)
• unverifiable — retain the running state and log a warning; never act on the process

So a legacy record now stops being a licence to kill: it keeps its running state until the daemon is next started under a version that records identity properly.

Tests

orphan_identity_requires_both_start_times and orphan_identity_rejects_records_without_a_start_time replace the three title-fallback cases. 521 unit tests and 26/26 supervisor.bats pass locally.

---

This PR was generated by Claude Code.

---

Note

High Risk
Changes process-group kill/stop and orphan adopt/kill decisions on PID reuse; mistakes could leave daemons unstoppable or, before this fix, signal the wrong tree.

Overview
Orphan identity no longer falls back to comparing process names when start_time is missing. process_identity_matches only succeeds when both recorded and live kernel start times are present and equal; legacy or unreadable identity is treated as unverifiable (keep running, log a warning) rather than mistaken for a mismatch (reset state). Startup orphan cleanup and interval reconciliation share that behavior.

User-initiated signalling adds signalling_pid_is_authorized, which is weaker than orphan matching: stop and resource-violation kills are skipped only when start times contradict each other (proven PID reuse). Missing start time still allows stop so old records do not become unstoppable. On contradiction, stop clears the daemon as stopped without killpg; the resource watcher finalizes the record instead of killing a stranger’s process group.

Tests cover the new identity rules and a bats case where pitchfork stop must not tear down an unrelated process group that recycled the recorded PID.

^{Reviewed by Cursor Bugbot for commit acb18ee. Bugbot is set up for automated code reviews on this repo. Configure here.}

Summary by CodeRabbit

• Bug Fixes

  • Improved process identity checks when managing daemon processes.
  • Prevented stop and resource-violation actions from signalling unrelated processes after PID reuse.
  • Preserved running-state information when process identity cannot be verified.
  • Updated daemon status handling when a recorded process is no longer valid.

• Tests

  • Added coverage confirming recycled process IDs are not signalled during daemon shutdown.

[coderabbitai]

Warning

Review limit reached

@jdx, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 3 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Central YAML (base), Organization UI (inherited)

Review profile: CHILL

Plan: Pro Plus

Run ID: c87a3505-c005-4ef1-ad16-66867df2144f

📥 Commits

Reviewing files that changed from the base of the PR and between 9453ac8 and acb18ee.

📒 Files selected for processing (2)

• src/supervisor/adopt.rs
• src/supervisor/watchers.rs

📝 Walkthrough

Walkthrough

Daemon identity checks now rely on kernel process start times. Stop and resource-violation paths refuse signalling when recorded and current identities conflict, while reconciliation preserves running state when identity is unavailable. Tests cover helper semantics and recycled-PID stopping.

Changes

PID identity safety

Layer / File(s)	Summary
Start-time identity reconciliation src/supervisor/mod.rs, src/supervisor/adopt.rs	Process matching now requires equal recorded and current start times, removes title-based fallback matching, and preserves running state when either start time is unavailable.
Signalling authorization guards src/supervisor/mod.rs, src/supervisor/lifecycle.rs, src/supervisor/watchers.rs	A crate-visible authorization helper permits signalling when identity is unknown but rejects contradictory start times; stop paths mark mismatched daemons stopped without signalling.
Recycled PID integration validation test/supervisor.bats	Adds coverage confirming a recycled PID does not cause an unrelated process group to receive stop signals.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant Supervisor
  participant PROCS
  participant DaemonState
  Client->>Supervisor: Request daemon stop
  Supervisor->>DaemonState: Read recorded start_time
  Supervisor->>PROCS: Read current PID start_time
  Supervisor->>Supervisor: Evaluate signalling authorization
  alt Start times contradict
    Supervisor->>DaemonState: Set pid=None and status=Stopped
    Supervisor-->>Client: DaemonWasNotRunning
  else PID authorized
    Supervisor->>PROCS: Signal process group
  end

Loading

Possibly related PRs

• jdx/pitchfork#632: Introduced related start-time identity checks before killing orphaned daemons.
• jdx/pitchfork#633: Updated supervisor reconciliation and PID identity handling.
• jdx/pitchfork#635: Added related process-ownership safeguards for supervisor lifecycle operations.

Suggested reviewers: gaojunran

Poem

I’m a rabbit guarding the PID burrow,
No recycled hare gets signals to borrow.
Start times align, the stop may proceed,
Mismatched processes stay safe indeed.
The daemon rests, and I munch clover.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately captures the main change: orphan-process identity now depends on start time rather than process name.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[greptile-apps]

Greptile Summary

Tightens orphan identity matching and adds PID start-time checks to process-group signalling paths.

• Requires matching persisted and current kernel start times before orphan adoption or cleanup.
• Retains running state when process identity cannot be verified.
• Adds signalling checks to explicit stop and resource-limit enforcement.
• Adds integration coverage for stopping a record whose PID was provably recycled.

Confidence Score: 3/5

This PR is not yet safe to merge because unverifiable retained daemon records can still signal an unrelated process group after PID reuse.

The new signalling helper rejects only positively unequal start times; missing identity remains authorized even though orphan reconciliation deliberately preserves those records as running, leaving explicit stop and resource-limit kill paths able to act on an unrelated recycled PID.

Files Needing Attention: src/supervisor/mod.rs, src/supervisor/lifecycle.rs, src/supervisor/watchers.rs, src/supervisor/adopt.rs

Important Files Changed

Filename	Overview
src/supervisor/adopt.rs	Removes process-title identity fallback and preserves unverifiable running records, which still feed the outstanding signalling path.
src/supervisor/lifecycle.rs	Prevents stop from signalling positively mismatched PIDs, but still signals when identity is unavailable.
src/supervisor/mod.rs	Introduces strict orphan matching alongside a weaker signalling authorization rule that leaves the prior unsafe legacy-record case reachable.
src/supervisor/watchers.rs	Adds the same permissive identity authorization before resource-limit process-group kills.
test/supervisor.bats	Covers unequal known start times but not the retained legacy-record case where the recorded start time is absent.

_{Reviews (3): Last reviewed commit: "fix(supervisor): finalize a daemon whose..." | Re-trigger Greptile}

[jdx]

Unverified PID remains killable — a later manual stop or autostop signals that process group without verifying its start time.

Confirmed and fixed in 9453ac8. stop() checked only PROCS.is_running(pid) and then killed the whole process group, so this was real — and it predates this PR, though this PR did widen the set of records that reach it (a legacy record whose PID had been recycled by a differently named process used to be reset; it is now retained).

What changed. signalling_pid_is_authorized refuses to signal when the recorded and live start times positively disagree; stop() then reports the daemon as not running and clears the record, which is accurate — that PID is somebody else's process. The resource-violation kill in watchers.rs gets the same guard, where a recycled PID would otherwise mean measuring one process tree and killing another.

Why it's weaker than process_identity_matches. Unknown identity on either side still signals as before. Being strict there would make any record written before start times existed permanently unstoppable — the user has named the daemon and asked for it to stop, so the check exists to catch a provably wrong target, not to demand proof of a right one.

What I didn't use. kill_process_group_if_start_time_matches_async already exists and is race-free (pidfd-pinned), but it deliberately refuses on non-Linux Unix, where there's no durable process handle. Wiring it into stop() would make daemons unstoppable on macOS. The check-then-kill guard has a TOCTOU window instead; that's a real limitation, and still strictly better than no check on every platform.

The four other kill_process_group_async sites in lifecycle.rs are inside the readiness loop for a child this supervisor just spawned and still holds, so their PID can't have been recycled.

Test. stop does not signal a process that recycled the daemon's PID. First version of it was vacuous — a background job in a bats script joins bats' own process group, so killpg hit ESRCH no matter what the daemon code did. Rebuilt on setsid so the bystander leads a real group with two children; it now fails (bystander killed) with the guard removed and passes with it. supervisor + basic + autostop suites: 65/65.

---

This comment was generated by Claude Code.

[cursor]

Cursor Bugbot has reviewed your changes and found 1 potential issue.

^{❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.}

^{Reviewed by Cursor Bugbot for commit 9453ac8. Configure here.}

[jdx]

Resource kill skips state update — the record can stay running and resource checks keep treating a recycled PID as that daemon.

Correct, fixed in the latest commit. The refusal branch returned without touching state, so on the next tick the watcher would read the stranger's memory/CPU as the daemon's, refuse to kill again, and repeat indefinitely.

It now finalizes through finalize_if_pid rather than writing state directly — that revalidates both the monitor registry and the record's PID inside the same state-lock critical section, so a successor that claimed the daemon between the snapshot and the write isn't clobbered. The status is Errored(-1), matching what orphan reconciliation records for a PID discovered to be recycled, which keeps the daemon eligible for retry. finalize_if_pid went from private to pub(super) to be callable from watchers.rs.

On the second location you flagged: stop() already clears pid and marks the daemon Stopped in its refusal branch, and Stopped is right there — the user asked for the daemon to stop, so retries should not fire.

523 unit tests; supervisor + interval suites 31/31.

---

This comment was generated by Claude Code.