fix: use session-based SIGHUP for graceful action stop#710
Conversation
Interactive shells use job control, placing child commands in separate process groups. The previous approach sent SIGTERM to only the shell's process group via kill(-pid), which missed child process groups entirely. Additionally, interactive shells ignore SIGTERM by default, so the graceful stop always fell through to the 5-second SIGKILL escalation. Switch to `pkill -s <sid>` which targets every process in the session regardless of process group, reliably reaching the shell, its child commands, and all their descendants. The SIGKILL escalation also uses session-based kill to avoid leaving orphaned processes. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
macOS `pkill` does not support the `-s` (session) flag — it's Linux-only. The previous commit silently failed to send any signal, causing "Stopping…" to hang forever. Replace `pkill -s` with `libc::kill(-pgid, SIGHUP)`. Interactive shells handle SIGHUP by forwarding it to all managed jobs before exiting, which is POSIX-standard across bash/zsh/fish. The SIGKILL escalation also switches to direct `libc::kill(-pgid, SIGKILL)`, removing the platform-specific `pkill` dependency entirely. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
The previous SIGKILL escalation only targeted the shell's initial process group via kill(-pgid, SIGKILL). Interactive shells use job control, placing child commands in separate process groups. If the shell exits from SIGHUP but a child in a different process group survives (e.g. a backgrounded process that trapped SIGHUP), kill(-pgid, 0) returns ESRCH and SIGKILL is never sent — leaving orphans. Add platform-specific kill_session() that enumerates all PIDs in the session and sends SIGKILL to each: - macOS: sysctl(KERN_PROC_SESSION) to query processes by session ID - Linux: /proc/*/stat field 6 (session ID) enumeration Also check the SIGHUP return value — if it fails with ESRCH, the process group is already gone and the SIGKILL escalation thread is skipped. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
The kill_session function was only defined for macOS and Linux via target_os cfg gates, but called from a #[cfg(unix)] block. This would fail to compile on other unix targets like FreeBSD. Add a fallback implementation that uses kill(-sid, signal) for any other unix platform. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f976737990
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| let sid = pid as i32; | ||
| // SAFETY: kill(2) does not dereference pointers. A negative | ||
| // pid targets the process group. | ||
| let hup_result = unsafe { libc::kill(-sid, libc::SIGHUP) }; |
There was a problem hiding this comment.
For the local interactive path, foreground commands can be moved into their own process group by job control, so kill(-sid, SIGHUP) only reaches the shell's original process group and not the foreground job this change is trying to stop gracefully. In that scenario the child process receives no graceful signal at all and is only reached later by kill_session(sid, SIGKILL), so commands that need SIGHUP/TERM for cleanup are hard-killed after the grace period. Since the code already added session enumeration, the initial graceful signal needs to cover the session as well, not just the shell PGID.
Useful? React with 👍 / 👎.
1 participant
Summary
SIGTERMtoSIGHUPwhen stopping running actions. Interactive shells ignoreSIGTERMby default and place child commands in separate process groups via job control, sokill(-pgid, SIGTERM)can miss them.SIGHUPtriggers the shell's built-in cleanup: it forwardsSIGHUP+SIGCONTto every managed job, then exits.SIGKILLusing session-based kill instead of process-group kill. Since interactive shells use job control, child processes may live in different process groups. The newkill_session()function enumerates all processes in the session (viasysctlon macOS,/procon Linux) and signals each one individually.ESRCH).Test plan
crates-fmt,crates-lint,crates-test,differ-ci,staged-ciall pass on push🤖 Generated with Claude Code