Code Review

This pull request introduces a more robust graceful shutdown strategy, escalating from SIGTERM to SIGKILL, which is a great improvement for process management. The implementation looks solid, and the accompanying documentation updates across various files are clear and consistent. I've made one suggestion in src/procs.rs to refactor the new termination logic for better readability and maintainability by extracting the polling logic into a helper function and using constants for timing values. Overall, this is a well-executed feature enhancement.

Pull request overview

This PR implements a progressive shutdown strategy for daemon processes, adding automatic SIGKILL escalation after two SIGTERM attempts. The implementation addresses issue #100 by ensuring stubborn processes that don't respond to SIGTERM are eventually force-terminated after ~5 seconds.

Changes:

• Implements SIGTERM → SIGTERM → SIGKILL escalation strategy in src/procs.rs with intelligent timing (10ms checks initially, then 50ms intervals)
• Removes redundant process termination verification from src/supervisor/lifecycle.rs since the new kill strategy is more robust
• Updates all documentation to reflect the new shutdown behavior
• Fixes unrelated macOS test failures by canonicalizing paths to resolve symlinks

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

AI-generated review (Claude Code)

Overall

Solid approach — progressive shutdown is a real need for a daemon supervisor. The docs and help text updates are thorough. A few concerns below, roughly ordered by severity.

Issues

Zombie process detection

The polling loop uses self.lock_system().process(sysinfo_pid).is_none() to detect exit. On Linux, zombie processes still have /proc/[pid] entries, so sysinfo returns Some(process) for them. If the tokio Child monitoring task doesn't reap quickly enough, a process that exited cleanly on the first SIGTERM will appear "still running" and get unnecessarily escalated through all phases to SIGKILL.

Consider also checking for zombie status:

if self.lock_system().process(sysinfo_pid)
    .map(|p| matches!(p.status(), sysinfo::ProcessStatus::Zombie))
    .unwrap_or(true)
{
    return true;
}

No process.wait() for SIGTERM exits

The old code always called process.wait() to reap the child. The new code only calls wait() in the SIGKILL path. For processes that exit from SIGTERM (detected via polling), there's no explicit reap — this relies on the supervisor's monitoring task. Probably fine in practice but it's a subtle behavioral change.

Second SIGTERM adds delay without clear benefit

SIGTERM → SIGTERM → SIGKILL is unusual. If a process ignores the first SIGTERM, it will almost certainly ignore the second. The standard pattern is SIGTERM → wait → SIGKILL. The second SIGTERM adds 2s of delay before the inevitable SIGKILL with no practical benefit for most cases. Simpler approach: SIGTERM → wait 3-5s → SIGKILL.

Lifecycle verification removal

The old post-kill verification loop in lifecycle.rs is removed with a comment that kill_async now guarantees termination. This is correct if the zombie concern above is addressed. Otherwise kill_async could return true for a zombie that still holds resources (ports, files), and the daemon state gets set to Stopped prematurely.

Nits

• The architecture doc additions (Port/Command readiness types, Stopping status) and the test canonicalize fixes seem unrelated to the SIGKILL feature — could be separate commits/PRs.
• The fast 10ms initial polling is a nice touch for the common case.