Skip to content

Revert "terminal: Actually close process groups when the terminal is closed (#61467)" (#62399) (cherry-pick to stable) #62413

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
zed-zippy merged 1 commit into from
Aug 10, 2026
Merged

Revert "terminal: Actually close process groups when the terminal is closed (#61467)" (#62399) (cherry-pick to stable) #62413

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
83 changes: 11 additions & 72 deletions crates/terminal/src/pty_info.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,71 +72,6 @@ pub(crate) struct ProcessInfo {
pub(crate) argv: Vec<String>,
}

/// Process (group) ids of a terminal's shell and foreground job, snapshotted
/// while the PTY master is still open: reading the foreground process group
/// requires `tcgetpgrp` on the PTY fd, which the event loop closes when the
/// terminal shuts down, so these ids must be captured before shutdown and
/// signalled afterwards.
#[derive(Clone, Copy)]
pub(crate) struct TerminalProcessIds {
#[cfg_attr(not(unix), allow(dead_code))]
foreground: Option<Pid>,
#[cfg_attr(not(unix), allow(dead_code))]
child: Pid,
}

#[cfg(unix)]
impl TerminalProcessIds {
/// The spawned child (the shell) leads its own process group, but under
/// job control a foreground job runs in a separate process group that
/// `killpg` on the shell's group never reaches, so both are signalled
/// (see #47412).
fn process_group_ids(self) -> impl Iterator<Item = i32> {
std::iter::once(self.child)
.chain(
self.foreground
.filter(|foreground| *foreground != self.child),
)
.map(|pid| pid.as_u32() as i32)
// `killpg(0, ...)` signals the caller's own process group, i.e.
// Zed itself, so never let a zero id (or a negative one from an
// implausibly large pid wrapping the cast) through.
.filter(|process_group_id| *process_group_id > 0)
}

/// Returns whether at least one process group was signalled successfully;
/// `killpg` failing with `ESRCH` (the group already exited) is expected and
/// reported as an unsuccessful signal.
fn signal_process_groups(&self, signal: i32) -> bool {
let mut signalled = false;
for process_group_id in self.process_group_ids() {
signalled |= unsafe { libc::killpg(process_group_id, signal) } == 0;
}
signalled
}

pub(crate) fn terminate(&self) -> bool {
self.signal_process_groups(libc::SIGTERM)
}

pub(crate) fn kill(&self) -> bool {
self.signal_process_groups(libc::SIGKILL)
}
}

#[cfg(not(unix))]
impl TerminalProcessIds {
pub(crate) fn terminate(&self) -> bool {
false
}

// Windows has no process groups to escalate on; killing the child relies
// on [`PtyProcessInfo::kill_child_process`] instead.
pub(crate) fn kill(&self) -> bool {
false
}
}

/// Fetches Zed-relevant Pseudo-Terminal (PTY) process information
pub(crate) struct PtyProcessInfo {
system: RwLock<System>,
Expand Down Expand Up @@ -183,13 +118,6 @@ impl PtyProcessInfo {
&self.pid_getter
}

pub(crate) fn capture_process_ids(&self) -> TerminalProcessIds {
TerminalProcessIds {
foreground: self.pid_getter.pid(),
child: self.pid_getter.fallback_pid(),
}
}

fn refresh(&self) -> Option<MappedRwLockReadGuard<'_, Process>> {
let pid = self.pid_getter.pid()?;
let fallback_pid = self.pid_getter.fallback_pid();
Expand Down Expand Up @@ -235,6 +163,17 @@ impl PtyProcessInfo {
self.get_child().is_some_and(|process| process.kill())
}

#[cfg(unix)]
pub(crate) fn terminate_child_process(&self) -> bool {
let pid = self.pid_getter.fallback_pid();
unsafe { libc::killpg(pid.as_u32() as i32, libc::SIGTERM) == 0 }
}

#[cfg(not(unix))]
pub(crate) fn terminate_child_process(&self) -> bool {
false
}

fn load(&self) -> Option<ProcessInfo> {
let process = self.refresh()?;
let cwd = process.cwd().map_or(PathBuf::new(), |p| p.to_owned());
Expand Down
164 changes: 9 additions & 155 deletions crates/terminal/src/terminal.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,6 @@ use std::{
borrow::Cow,
cmp::{self, min},
fmt::{self, Display, Formatter},
future::Future,
ops::{BitOr, BitOrAssign, Deref, Range as StdRange},
path::{Path, PathBuf},
process::ExitStatus,
Expand Down Expand Up @@ -75,33 +74,6 @@ use crate::alacritty::{
use crate::mappings::colors::to_vte_rgb;
use crate::mappings::keys::to_esc_str;

/// How long the shell and its foreground job get to exit gracefully after a
/// closed terminal sends SIGHUP/SIGTERM, before being SIGKILLed. Must stay
/// comfortably below [`gpui::SHUTDOWN_TIMEOUT`] so the escalation also
/// completes when the whole app is quitting.
const PROCESS_KILL_GRACE_PERIOD: Duration = Duration::from_millis(100);

/// Sends SIGTERM to the terminal's shell and foreground process groups, and
/// returns a future that SIGKILLs whatever survives [`PROCESS_KILL_GRACE_PERIOD`].
/// Closing the PTY only delivers SIGHUP, and a foreground job that ignores
/// SIGHUP/SIGTERM would otherwise be orphaned (#47412).
///
/// Must be called while the PTY master is still open (i.e. before
/// `pty_tx.shutdown()`): reading the foreground process group requires
/// `tcgetpgrp` on the PTY fd.
fn terminate_processes_with_grace_period(
info: Arc<PtyProcessInfo>,
executor: BackgroundExecutor,
) -> impl Future<Output = ()> {
let process_ids = info.capture_process_ids();
process_ids.terminate();
async move {
executor.timer(PROCESS_KILL_GRACE_PERIOD).await;
process_ids.kill();
info.kill_child_process();
}
}

/// Process-wide flag set by headless hosts (e.g. the eval CLI) that have no
/// controlling TTY. In such sandboxes PTY allocation and acquiring a
/// controlling terminal fail with `ENOTTY`, so when this is set terminals run
Expand Down Expand Up @@ -1341,32 +1313,6 @@ impl TerminalBuilder {
}

pub fn subscribe(mut self, cx: &Context<Terminal>) -> Terminal {
// `Terminal::drop` escalates to SIGKILL on a detached background task,
// which never gets to run when the whole app quits: the process exits
// as soon as the `on_app_quit` futures resolve. Perform the same
// escalation in a quit observer, whose future keeps the app alive for
// the grace period, so that processes ignoring SIGHUP/SIGTERM don't
// outlive Zed (#47412). The subscription can't be stored on `Terminal`
// (`Subscription` is not `Send`, and `TerminalBuilder` is built on a
// background thread), so its lifetime is tied to the entity's release
// instead.
let app_quit_subscription = cx.on_app_quit(|terminal, cx| {
let kill_processes = match &terminal.terminal_type {
TerminalType::Pty { info, .. } => Some(terminate_processes_with_grace_period(
info.clone(),
cx.background_executor().clone(),
)),
TerminalType::DisplayOnly => None,
};
async move {
if let Some(kill_processes) = kill_processes {
kill_processes.await;
}
}
});
cx.on_release(move |_, _| drop(app_quit_subscription))
.detach();

//Event loop
self.terminal.event_loop_task = cx.spawn(async move |terminal, cx| {
while let Some(event) = self.events_rx.next().await {
Expand Down Expand Up @@ -3180,10 +3126,16 @@ impl Drop for Terminal {
if let TerminalType::Pty { pty_tx, info } =
std::mem::replace(&mut self.terminal_type, TerminalType::DisplayOnly)
{
let kill_processes =
terminate_processes_with_grace_period(info, self.background_executor.clone());
pty_tx.shutdown();
self.background_executor.spawn(kill_processes).detach();
info.terminate_child_process();

let timer = self.background_executor.timer(Duration::from_millis(100));
self.background_executor
.spawn(async move {
timer.await;
info.kill_child_process();
})
.detach();
}
}
}
Expand Down Expand Up @@ -4928,104 +4880,6 @@ mod tests {
);
}

#[cfg(unix)]
fn parse_pid_marker(content: &str, prefix: &str, suffix: &str) -> i32 {
content
.split(prefix)
.nth(1)
.and_then(|rest| rest.split(suffix).next())
.and_then(|pid| pid.trim().parse().ok())
.unwrap_or_else(|| {
panic!("failed to parse pid between {prefix:?} and {suffix:?} from: {content}")
})
}

/// Regression test for <https://github.com/zed-industries/zed/issues/47412>:
/// closing a terminal must not orphan processes that ignore SIGHUP and
/// SIGTERM. The shell ignores both signals and the `sleep`s inherit the
/// ignored dispositions, so only the SIGKILL escalation can terminate them.
///
/// Two process groups are covered: the background `sleep` is spawned before
/// `set -m` and stays in the shell's own group, while job control places
/// the foreground job (an inner shell that `exec`s `sleep`) in a separate
/// group that killing the shell's group never reaches — it is only found
/// via the foreground-group capture (`tcgetpgrp`).
#[cfg(unix)]
#[gpui::test]
async fn test_dropping_terminal_kills_processes_ignoring_sighup_and_sigterm(
cx: &mut TestAppContext,
) {
cx.executor().allow_parking();

let (terminal, _completion_rx) = build_test_terminal_with_arguments(
cx,
"/bin/sh".to_string(),
vec![
"-c".to_string(),
"trap '' HUP TERM; sleep 300 & echo bg_marker_${!}_bgend; set -m; \
/bin/sh -c 'echo fg_marker_$$_fgend; exec sleep 300'"
.to_string(),
],
)
.await;

assert_content_eventually(&terminal, "_fgend", cx).await;
let content = terminal.update(cx, |term, _| term.get_content());
let background_sleep_pid = parse_pid_marker(&content, "bg_marker_", "_bgend");
let foreground_sleep_pid = parse_pid_marker(&content, "fg_marker_", "_fgend");

let shell_pid = terminal.update(cx, |terminal, _| match &terminal.terminal_type {
TerminalType::Pty { info, .. } => info.pid_getter().fallback_pid().as_u32() as i32,
TerminalType::DisplayOnly => panic!("expected a PTY-backed terminal"),
});

for pid in [background_sleep_pid, foreground_sleep_pid] {
assert_eq!(
unsafe { libc::kill(pid, 0) },
0,
"process {pid} should be running before the terminal is dropped"
);
}

// The foreground-group escalation is only exercised if `set -m`
// actually placed the foreground job in its own process group; assert
// the arrangement so this test fails loudly instead of silently
// degrading into a shell-group-only test.
let shell_pgid = unsafe { libc::getpgid(shell_pid) };
let foreground_pgid = unsafe { libc::getpgid(foreground_sleep_pid) };
assert!(shell_pgid > 0 && foreground_pgid > 0);
assert_ne!(
foreground_pgid, shell_pgid,
"job control should place the foreground sleep in its own process group"
);
assert_eq!(
unsafe { libc::getpgid(background_sleep_pid) },
shell_pgid,
"the background sleep should stay in the shell's process group"
);

drop(terminal);
// Flush effects so the released terminal entity is actually dropped.
cx.update(|_| {});

for _ in 0..300 {
let background_dead = unsafe { libc::kill(background_sleep_pid, 0) } != 0;
let foreground_dead = unsafe { libc::kill(foreground_sleep_pid, 0) } != 0;
if background_dead && foreground_dead {
return;
}
cx.background_executor
.timer(Duration::from_millis(10))
.await;
}
panic!(
"processes survived dropping the terminal: background sleep {background_sleep_pid} \
alive: {}, foreground sleep {foreground_sleep_pid} alive: {}",
unsafe { libc::kill(background_sleep_pid, 0) } == 0,
unsafe { libc::kill(foreground_sleep_pid, 0) } == 0,
);
}

/// Test that kill_active_task on a task that's not running is a no-op
#[gpui::test]
async fn test_kill_active_task_on_completed_task_is_noop(cx: &mut TestAppContext) {
Expand Down
Loading