fix(desktop): kill all terminal child processes on close

[chasemcdo]

Summary

Fixes terminal child processes (Claude Code, npm dev servers, Next.js, etc.) not being killed when closing a terminal tab with Cmd+W. Previously, only the shell process was terminated while child processes continued running in the background.

Problem

When closing a terminal in the desktop app:

• Only the shell (zsh/bash) receives the termination signal
• Child processes like Claude Code, npm run dev, etc. survive and accumulate
• This causes resource leaks
• Next.js lock files aren't cleaned up, blocking new dev server starts

Reproduction

1. Open a terminal in the desktop app
2. Run claude to start Claude Code
3. Inside Claude Code, run echo $PPID to get the shell's PID (e.g., 12345)
4. Close the terminal with Cmd+W
5. Check if the process is still running: ps aux | grep 12345
6. Before this fix: The Claude Code process is still running
7. After this fix: The process is terminated

Root Cause

The original implementation used node-pty's ptyProcess.kill(signal) which only sends the signal to the shell's PID directly - not to any child processes.

A naive fix would be to use process.kill(-pid, signal) to kill by process group. However, this doesn't work because interactive shells with job control (zsh, bash) give foreground jobs their own process groups:

zsh (PID 100, PGID 100)  ← shell's process group
└─ claude (PID 101, PGID 101)  ← zsh gave claude its OWN process group!

So process.kill(-100, signal) only kills processes in PGID 100 (the shell), not PGID 101 (claude).

Solution

Use tree-kill to terminate the entire process tree by traversing parent-child relationships (PPID). This handles job control correctly because the parent-child link is preserved even when zsh assigns a new process group.

An alternative approach is TTY-based killing (pkill -t <tty>), which catches all processes sharing the controlling terminal. tree-kill is simpler (no shelling out to ps/pkill) and sufficient here—the edge cases where TTY wins (daemonized processes that keep the TTY) don't apply to interactive terminal commands.

Changes

• Added tree-kill dependency
• Updated handleKill() to use tree-kill with SIGTERM → SIGKILL escalation
• Updated handleDispose() to use tree-kill
• handleSignal() unchanged—Ctrl+C should only signal the foreground process, not kill the tree

Testing

1. Open a terminal, run claude (or npm run dev)
2. Close with Cmd+W
3. Verify process is gone: pgrep -f claude should not show the terminated instance

Resolves #893

---

Note

Ensures terminal child processes are reliably terminated when a tab is closed.

• Replace direct ptyProcess.kill() with tree-kill in handleKill() to terminate the full process tree; escalate to SIGKILL after 2s and force-exit if onExit never fires
• Update handleDispose() to tree-kill the process tree with SIGKILL on exit
• Keep handleSignal() behavior unchanged (signals like SIGINT only)
• Add tree-kill dependency and bump desktop app version to 0.0.59

^{Written by Cursor Bugbot for commit d386714. This will update automatically on new commits. Configure here.}

Summary by CodeRabbit

• Bug Fixes
  • Improved terminal process termination to reliably stop all related processes and ensure proper cleanup when closing terminal windows, with enhanced error handling and fallback mechanisms.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Caution

Review failed

The pull request is closed.

📝 Walkthrough

Walkthrough

Replaces direct process killing in the desktop PTY subprocess with the tree-kill library, adding a 2s escalation to SIGKILL and a forced-exit fallback; updates disposal to fire-and-forget tree-kill; adds the tree-kill dependency to the desktop app package.json. (≤50 words)

Changes

Cohort / File(s)	Summary
PTY subprocess termination apps/desktop/src/main/terminal-host/pty-subprocess.ts	Replaced direct ptyProcess.kill(...) / process.kill(...) with treeKill(pid, signal, callback). handleKill now invokes treeKill, starts a 2s escalation timer to call treeKill(pid, "SIGKILL", ...) if still alive, and includes a forced-exit synthesis if onExit never fires. handleDispose uses a fire-and-forget treeKill(pid, "SIGKILL", ...) and logs failures.
Dependency update apps/desktop/package.json	Added runtime dependency "tree-kill": "^1.2.2".

Sequence Diagram(s)

sequenceDiagram
    participant Caller
    participant PTYHost
    participant treeKillLib as tree-kill
    participant OS

    Caller->>PTYHost: request kill(pid, signal)
    PTYHost->>treeKillLib: treeKill(pid, signal, callback)
    treeKillLib->>OS: send signal to pid and its children
    treeKillLib-->>PTYHost: callback (error/ok)
    PTYHost->>PTYHost: start 2s escalation timer
    alt still alive after 2s
        PTYHost->>treeKillLib: treeKill(pid, "SIGKILL", callback)
        treeKillLib->>OS: send SIGKILL to pid tree
        treeKillLib-->>PTYHost: callback (error/ok)
    end
    PTYHost->>Caller: emit onExit or synthesize exit if needed

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 I thumped my paw and called tree-kill,

chased each child down every hill.
A two-second hop, then SIGKILL's decree,
no drifting Claudes shall trouble me.
Hooray — tidy terminals, carrot tea.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	The code changes fully address the issue #893 requirements by implementing process tree termination using tree-kill to eliminate orphaned child processes and resource leaks when closing terminals.
Out of Scope Changes check	✅ Passed	All changes are directly related to the linked issue: adding tree-kill dependency and updating terminal process termination logic in pty-subprocess.ts with no unrelated modifications.
Description check	✅ Passed	The PR description is comprehensive and well-structured, covering problem statement, root cause analysis, solution approach, changes made, testing steps, and issue resolution.
Title check	✅ Passed	The title accurately summarizes the main change: switching from TTY-based killing to tree-kill library to ensure all terminal child processes are terminated on close.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

• 📝 Generate docstrings

---

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 and usage tips.}

[coderabbitai]

Actionable comments posted: 1

🤖 Fix all issues with AI agents

In `@apps/desktop/src/main/terminal-host/pty-subprocess.ts`:
- Around line 371-399: The block in handleKill currently uses execSync with
interpolated signal and tty (and swallows errors); replace execSync calls with
execFileSync and pass the command and args array (e.g., ["pkill", `-SIG...`,
"-t", tty]) to avoid shell interpolation, validate the incoming signal (allowed
set like SIGHUP,SIGTERM,SIGKILL) and ensure tty is a safe value (reject "?" or
whitespace), and when catching errors: if pkill failed inspect the error.status
and if status === 1 treat as "no processes matched" (log at debug/info)
otherwise log an error with context (include pid, tty, signal and
error.message); do the same input validation and logging for the process.kill
fallback (keep existing behavior but log failures with context and do not
silently swallow exceptions). Ensure identical hardening/logging is applied in
the escalation timer and handleDispose code paths referenced by the diff.

[Kitenite]

I think this actually happens inconsistently which points to a race condition or we're not sending the proper kill signal. perhaps tree-kill would be a cleaner solution

[chasemcdo]

I think this actually happens inconsistently which points to a race condition or we're not sending the proper kill signal. perhaps tree-kill would be a cleaner solution

from my testing it seems to happen every time, but perhaps I am just very unlucky haha

[chasemcdo]

@Kitenite though you are probably right that we should use tree-kill. Just updated and for the issues I have reproduced this seems to be equivalently functional but definitely cleaner

[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.}

This PR is being reviewed by Cursor Bugbot

Details

You are on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle.

To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.

[cursor]

Async tree-kill skipped by immediate process.exit

Medium Severity

In handleDispose(), treeKill() is called asynchronously but process.exit(0) executes synchronously on the next line. Since tree-kill internally spawns child processes to discover and kill the process tree, calling process.exit(0) immediately terminates the Node.js event loop before tree-kill can complete its work. The comment "fire and forget since we're exiting" is misleading—async operations cannot complete if the process exits synchronously. Child processes won't be killed in this code path.