terminal: Actually close process groups when the terminal is closed by feitreim · Pull Request #61467 · zed-industries/zed

feitreim · 2026-07-22T15:48:46Z

Objective

Currently, processes that ignore SIGHUP/SIGTERM will not be killed when the terminal is closed or Zed is closed.

Solution

We need to follow up on the processes to ensure that they have been properly killed BEFORE zed closes fully. We send SIGTERM then 100ms later send SIGKILL, this gives programs some time to exit gracefully before being SIGKILL'd.

Testing

I tested it by both closing the terminal pane and closing zed completely, both worked. There is a new test added as well. The best way to test it is with the reproduction in this comment:
#47412 (comment)

I only tested this on MacOS 27. It might be worth testing on linux, but also mac/linux behavior here should be quite similar. Windows behavior as well should be unchanged but could be worth testing.

Self-Review Checklist:

I've reviewed my own diff for quality, security, and reliability
Unsafe blocks (if any) have justifying comments

Okay there is an unsafe block in this PR, but its just being moved from one location to another, and it didn't originally have a comment, I assume because its just a libc call so its pretty clear.

The content adheres to Zed's UI standards (UX/UI and icon guidelines)
Tests cover the new/changed behavior
Performance impact has been considered and is acceptable

Showcase

Video showcasing the new behavior:

After.Behavior.mp4

Release Notes:

Terminal: Prevents processes from being left alive when the terminal is closed.

…processes don't leak Closing a terminal tab or quitting Zed previously only delivered SIGHUP (via PTY teardown), SIGTERM to the shell's process group, and a delayed SIGKILL to the shell process alone. Under job control the foreground job runs in its own process group, so a process ignoring SIGHUP/SIGTERM (e.g. claude, docker, some node CLIs) was orphaned. On app quit even the delayed SIGKILL never fired, because it ran on a detached background task and the app exited before its timer expired. Now the foreground process group is captured (via tcgetpgrp) before the PTY shuts down, both the shell's and the foreground job's process groups receive SIGTERM, and after a 100ms grace period both are SIGKILLed. A per-terminal on_app_quit observer performs the same escalation on quit, with its future keeping the app alive through the grace period. Fixes zed-industries#47412

zed-industries-bot · 2026-07-22T15:49:10Z

	Messages
📖	This PR includes links to the following GitHub Issues: #47412 If this PR aims to close an issue, please include a `Closes #ISSUE` line at the top of the PR body.

Generated by 🚫 dangerJS against b587ef6

ChristopherBiscardi

thanks! I was able to confirm the repro and the fix using #47412 (comment)

no more zombies :D

SomeoneToIgnore · 2026-08-07T12:13:48Z

@ChristopherBiscardi , do you think we should revert this based on #62286 ?

Ideally, some fast-forward fix could be made too, but do not have a good context for that myself.

feitreim · 2026-08-07T12:43:21Z

@SomeoneToIgnore looking into this right now. will try to get a fix out ASAP.

…ed-industries#61467) # Objective Closes zed-industries#47412 Currently, processes that ignore SIGHUP/SIGTERM will not be killed when the terminal is closed or Zed is closed. ## Solution We need to follow up on the processes to ensure that they have been properly killed BEFORE zed closes fully. We send SIGTERM then 100ms later send SIGKILL, this gives programs some time to exit gracefully before being SIGKILL'd. ## Testing I tested it by both closing the terminal pane and closing zed completely, both worked. There is a new test added as well. The best way to test it is with the reproduction in this comment: zed-industries#47412 (comment) I only tested this on MacOS 27. It might be worth testing on linux, but also mac/linux behavior here should be quite similar. Windows behavior as well should be unchanged but could be worth testing. ## Self-Review Checklist: - [x] I've reviewed my own diff for quality, security, and reliability - [x] Unsafe blocks (if any) have justifying comments Okay there is an unsafe block in this PR, but its just being moved from one location to another, and it didn't originally have a comment, I assume because its just a libc call so its pretty clear. - [x] The content adheres to Zed's UI standards ([UX/UI](https://github.com/zed-industries/zed/blob/main/CONTRIBUTING.md#uiux-checklist) and [icon](https://github.com/zed-industries/zed/blob/main/crates/icons/README.md) guidelines) - [x] Tests cover the new/changed behavior - [x] Performance impact has been considered and is acceptable ## Showcase Video showcasing the new behavior: https://github.com/user-attachments/assets/b2b8cb0c-0648-41b4-b940-91c466aeaab3 --- Release Notes: - Terminal: Prevents processes from being left alive when the terminal is closed.

…closed (zed-industries#61467)" (zed-industries#62399) This reverts commit 6297c88. --- fixes zed-industries#62286 fixes zed-industries#62095 zed-industries#61467 fixed its intended bug, but at the same time introduced an issue where running tasks that would cause new tasks to be terminated immediately. zed-industries#62322 tried to fix that forward, but was unsuccessful. In the mean-time I am going to revert the original PR. We can try to re-land the original bugfix in a future PR. Release Notes: - N/A

…closed (#61467)" (#62399) (cherry-pick to preview) (#62412) Cherry-pick of #62399 to preview ---- This reverts commit 6297c88. --- fixes #62286 fixes #62095 #61467 fixed its intended bug, but at the same time introduced an issue where running tasks that would cause new tasks to be terminated immediately. #62322 tried to fix that forward, but was unsuccessful. In the mean-time I am going to revert the original PR. We can try to re-land the original bugfix in a future PR. Release Notes: - N/A Co-authored-by: Chris Biscardi <chris@christopherbiscardi.com>

…closed (#61467)" (#62399) (cherry-pick to stable) (#62413) Cherry-pick of #62399 to stable ---- This reverts commit 6297c88. --- fixes #62286 fixes #62095 #61467 fixed its intended bug, but at the same time introduced an issue where running tasks that would cause new tasks to be terminated immediately. #62322 tried to fix that forward, but was unsuccessful. In the mean-time I am going to revert the original PR. We can try to re-land the original bugfix in a future PR. Release Notes: - N/A Co-authored-by: Chris Biscardi <chris@christopherbiscardi.com>

…closed (zed-industries#61467)" (zed-industries#62399) This reverts commit 6297c88. --- fixes zed-industries#62286 fixes zed-industries#62095 zed-industries#61467 fixed its intended bug, but at the same time introduced an issue where running tasks that would cause new tasks to be terminated immediately. zed-industries#62322 tried to fix that forward, but was unsuccessful. In the mean-time I am going to revert the original PR. We can try to re-land the original bugfix in a future PR. Release Notes: - N/A

…n tasks Re-lands zed-industries#61467 (reverted in zed-industries#62399) with the races that caused zed-industries#62095 and zed-industries#62286 designed out. Why the original landing regressed rerun tasks: ProcessIdGetter kept a raw fd number for the PTY master, but the event loop owns that fd and closes it as soon as the child exits - long before the completed task's Terminal entity is dropped. Rerunning a task spawns the replacement terminal first, whose PTY recycles the freed fd number; when the old entity was then dropped, tcgetpgrp on the recycled number read the new terminal's foreground process group and SIGTERM/SIGKILLed it. Why the tcgetsid guard (zed-industries#62322) was not enough: it validated a descriptor Zed does not own with non-atomic syscalls (the fd can be closed or reused between the check and the use), and when the guard rejected, cleanup still fell back to killpg on the child pid captured arbitrarily long ago, signalling a possibly recycled id unconditionally. The re-land replaces both heuristics with two deterministic mechanisms: * ProcessIdGetter now owns a dup of the PTY master (Arc<OwnedFd>), so the descriptor always refers to this terminal's PTY and fd-number recycling cannot occur by construction. Once the session dies, tcgetpgrp on our own master returns 0, so a completed terminal yields no foreground candidate at all. * Process-group ids are validated against the terminal's session before any killpg: the spawned child called setsid, so its pid doubles as the session id, and a group is only signalled while getsid(pgid) still reports that session. A completed terminal therefore captures nothing (its drop is signal-free), and a recycled pid lives in a different session and is rejected. Validation happens once at capture time so the SIGKILL escalation still reaches groups whose leader the SIGTERM already killed. kill_child_process and kill_current_process's stale-fallback path (kill task) get the same session-leader guard. Fixes zed-industries#47412

Three fixes to `script/find-release-commit`: - Under `set -euo pipefail`, the `match=$(git log ... | grep ...)` assignment aborted the whole script the first time a PR number wasn't found on main, so it exited 1 without printing anything -- not even the merge-base fallback it was supposed to reach. Guard the assignment with `|| true`, which also covers a SIGPIPE from `head` closing the pipe. - A subject can carry more than one PR number, and which one lives on main differs by case: for `foo (zed-industries#61199) (cherry-pick to preview) (zed-industries#62869)` only zed-industries#61199 is on main, while for `Revert "foo (zed-industries#61467)" (zed-industries#62399)` it's zed-industries#62399 -- zed-industries#61467 is the far older commit being undone. Taking the first match traced reverts to the wrong commit; try the numbers right-to-left instead. - When the fallback does kick in, list the commits that couldn't be traced (capped at `UNTRACED_LIMIT`, default 20) so the warning explains itself. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

feitreim added 3 commits July 21, 2026 14:11

review improvements

6e70c9e

recieve response signals just like original impl

b587ef6

cla-bot Bot added the cla-signed The user has signed the Contributor License Agreement label Jul 22, 2026

zed-community-bot Bot added the guild Pull requests by someone in Zed Guild. NOTE: the label application is automated via github actions label Jul 22, 2026

maxdeviant changed the title ~~Terminal: actually close process groups when the terminal is closed~~ terminal: Actually close process groups when the terminal is closed Jul 22, 2026

ChristopherBiscardi added the area:integrations/terminal Feedback for terminal integration, shell commands, etc label Jul 23, 2026

ChristopherBiscardi approved these changes Jul 23, 2026

View reviewed changes

ChristopherBiscardi added this pull request to the merge queue Jul 23, 2026

ChristopherBiscardi self-assigned this Jul 23, 2026

Merged via the queue into zed-industries:main with commit 6297c88 Jul 23, 2026
64 checks passed

feitreim deleted the bugfix-terminal-processes branch July 23, 2026 14:01

github-actions Bot mentioned this pull request Aug 6, 2026

🎨 Zed GPUI Release Tracker gpui-archipelago/gpui-release-notes#3

Open

lingyaochu mentioned this pull request Aug 7, 2026

Task won't re-run #62286

Closed

This was referenced Aug 7, 2026

Tasks terminate with SIGKILL when re-run in an existing terminal #62095

Closed

terminal: Avoid killing rerun task process groups #62322

Closed

marccampbell mentioned this pull request Aug 7, 2026

train/engineering-review: human — thanks! I was able to confirm the repro and the fix using https://github… adversarylabs/engineering-review-adversary#54

Closed

10 tasks

ChristopherBiscardi mentioned this pull request Aug 10, 2026

Revert "terminal: Actually close process groups when the terminal is closed (#61467)" #62399

Merged

feitreim mentioned this pull request Aug 13, 2026

terminal: Close process groups on shutdown, only when intended #62595

Open

5 tasks

Navigation Menu

Uh oh!

Uh oh!

terminal: Actually close process groups when the terminal is closed - #61467

terminal: Actually close process groups when the terminal is closed#61467
ChristopherBiscardi merged 3 commits into
zed-industries:mainfrom
feitreim:bugfix-terminal-processes

feitreim commented Jul 22, 2026 •

edited

Loading

Uh oh!

zed-industries-bot commented Jul 22, 2026

Uh oh!

ChristopherBiscardi left a comment

Uh oh!

Uh oh!

SomeoneToIgnore commented Aug 7, 2026

Uh oh!

feitreim commented Aug 7, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Uh oh!

Conversation

feitreim commented Jul 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Objective

Solution

Testing

Self-Review Checklist:

Showcase

Uh oh!

zed-industries-bot commented Jul 22, 2026

Uh oh!

ChristopherBiscardi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

SomeoneToIgnore commented Aug 7, 2026

Uh oh!

feitreim commented Aug 7, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

feitreim commented Jul 22, 2026 •

edited

Loading