Terminal: Close process groups on shutdown, only when intended - #62595
Open
feitreim wants to merge 4 commits into
Open
Terminal: Close process groups on shutdown, only when intended#62595feitreim wants to merge 4 commits into
feitreim wants to merge 4 commits into
Conversation
…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
… test Each drop of a completed terminal frees its PTY fd number for immediate reuse by the next spawn, so a single spawn/drop cycle can miss a stale-fd or stale-pid race by timing luck. Loop the rerun cycle from zed-industries#62095 ten times within one test, asserting on every iteration that the replacement's task runs to completion untouched.
cla-bot
Bot
added
the
cla-signed
zed-community-bot
Bot
added
the
guild
dinocosta
added
the
area:integrations/terminal
feitreim
changed the title
feitreim
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Objective
Re-fixes: #47412
Fixes #62286
Fixes #62095
Fixes the bug introduced in #61467
Solution
#47412 was an issue where some processes were not being dropped after zed closed, I fixed that issue in #61467, but then introduced the issues in #62286 and #62095, tldr: I added something to send additional SIGKILL/SIGTERM signals after the terminal was closed, but when running tasks, these signals were firing on the wrong processes, due to race conditions regarding FD reuse.
The big design change this time around is moving
ProcessIdGetterto having anOwnedFD, this categorically prevents the FD race that was observed in previous iterations. Checks for which process to kill are run through theOwnedFD, so there cannot be a race where the raw FD we are holding has been reassigned to a new process group. Where the original process group is created, the session id is set, when we go to tear down the process group, we ensure that the current process group has the correct, matching session id, this prevents the admittedly much less likely pid reuse race.Testing
Manually tested the process exiting changes, and manually tested the task spawning (many times in a row this time, some overlapped).
Additionally this adds a test for #47412, as well as a simple test for #62286/#62095, and a test for those that creates tasks 10x in a row. There is also a new test for the PID reuse case, this one is deterministic.
Self-Review Checklist:
Release Notes: