From d7e6b45e9168109afd17b33d7118501f4451ba47 Mon Sep 17 00:00:00 2001 From: l0ng-ai <24760907+l0ng-ai@users.noreply.github.com> Date: Sun, 16 Aug 2026 04:29:10 +0800 Subject: [PATCH] fix(pane): let kill reach what runs in a pane MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `serve_sigterm` blocks SIGTERM on the daemon's main thread before any other starts, so every thread inherits the block and only its `sigwait` waiter ends the process. That is deliberate and the comment there explains it well. What it also does is reach places it was never meant to: `fork` copies the calling thread's mask into the child and `execve` keeps it. So every pane ran with SIGTERM blocked. `kill`, `pkill`, `timeout`, a supervisor, a CI cancellation — none of them could stop anything in a pane, and only `kill -9` would. Measured, not reasoned: tty7 run -- sh -c 'kill -TERM $$; echo STILL-ALIVE; exit 7' → STILL-ALIVE, exit 7 portable-pty does reset the child's SIGTERM disposition to `SIG_DFL`, but a blocked signal is never delivered for a disposition to apply, so that was never going to help. This is the unix half of the Windows line three above it, which resets the inherited "ignore Ctrl+C" state for the same reason: the daemon's own signal state is not the pane's. That one cost a pane every Ctrl+C (#451, #314); this one cost it every kill. The block is lifted for the spawn alone, because the mask a child forks with is the one it keeps, and the guard restores the whole saved mask rather than re-blocking, so a thread that never had it blocked is left as it was. The cost is a fork-wide window where a SIGTERM meant for the daemon could land on this thread and end it without the scrollback save; that is a millisecond against every pane being unstoppable. Verified against a running daemon, both halves: the command above is now terminated with nothing printed, and a SIGTERM to the daemon with a live pane still exits it and still leaves that pane's screen in `scrollback/`, which is what `serve_sigterm` exists to do. --- crates/tty7-core/src/daemon/pane.rs | 100 ++++++++++++++++++++++++++++ 1 file changed, 100 insertions(+) diff --git a/crates/tty7-core/src/daemon/pane.rs b/crates/tty7-core/src/daemon/pane.rs index 9040d660..c0c75beb 100644 --- a/crates/tty7-core/src/daemon/pane.rs +++ b/crates/tty7-core/src/daemon/pane.rs @@ -682,6 +682,46 @@ const EXIT_CODE_PROBE_WINDOW: Duration = Duration::from_millis(500); /// offers; `signalled_status_is_not_reported_as_a_plain_exit` pins it against /// the crate's own constructor so an upstream rewording fails there rather /// than here. +/// Lifts this thread's SIGTERM block for as long as it is held, and puts the +/// thread's own mask back when dropped. +/// +/// Only a spawn wants this: `fork` copies the calling thread's mask into the +/// child, and nothing else here does. Restores the whole saved mask rather +/// than re-blocking SIGTERM, so a thread that never had it blocked — a test, +/// or a build where `serve_sigterm` never ran — is left as it was found. +#[cfg(unix)] +struct SigtermUnblocked(Option); + +#[cfg(unix)] +impl SigtermUnblocked { + fn for_spawn() -> Self { + unsafe { + let mut just_sigterm: libc::sigset_t = std::mem::zeroed(); + libc::sigemptyset(&mut just_sigterm); + libc::sigaddset(&mut just_sigterm, libc::SIGTERM); + let mut previous: libc::sigset_t = std::mem::zeroed(); + match libc::pthread_sigmask(libc::SIG_UNBLOCK, &just_sigterm, &mut previous) { + 0 => Self(Some(previous)), + // Nothing was changed, so there is nothing to put back. The + // pane still starts; it just starts with the mask it would + // have had before this existed. + _ => Self(None), + } + } + } +} + +#[cfg(unix)] +impl Drop for SigtermUnblocked { + fn drop(&mut self) { + if let Some(previous) = self.0 { + unsafe { + libc::pthread_sigmask(libc::SIG_SETMASK, &previous, std::ptr::null_mut()); + } + } + } +} + fn reported_exit_code(status: &portable_pty::ExitStatus) -> Option { match status.to_string().starts_with("Terminated by") { true => None, @@ -1320,6 +1360,25 @@ impl DaemonPane { let pair = native_pty_system().openpty(pty_size)?; let spawn = build_spawn_config(id, cwd, shell, workspace.as_deref())?; + // The unix half of the same problem the Windows line above solves: the + // daemon's own signal state reaching a pane it has no business in. + // `serve_sigterm` blocks SIGTERM on every daemon thread so that only + // its `sigwait` waiter ends the process; `fork` hands the calling + // thread's mask to the child and `execve` keeps it. portable-pty does + // reset the child's SIGTERM *disposition* to `SIG_DFL`, but a blocked + // signal is never delivered for a disposition to apply — so every pane + // ran a shell that `kill`, `pkill` and `timeout` could not stop, and + // only `kill -9` would end. + // + // Lifted for the spawn alone: the mask the child forks with is the one + // it keeps, and this thread takes its own back immediately. The cost is + // a window of a fork's width where a SIGTERM aimed at the daemon could + // land on this thread and end it on the spot, losing the scrollback + // save that `serve_sigterm` exists to perform. That is a millisecond + // against every pane in the process being unstoppable. + #[cfg(unix)] + let _sigterm_unblocked = SigtermUnblocked::for_spawn(); + let child = pair.slave.spawn_command(spawn.cmd)?; let shell_pid = child.process_id(); let child = Arc::new(Mutex::new(child)); @@ -4561,6 +4620,47 @@ mod tests { assert!(rx.try_recv().is_err()); } + #[cfg(unix)] + fn sigterm_is_blocked_here() -> bool { + unsafe { + let mut current: libc::sigset_t = std::mem::zeroed(); + libc::pthread_sigmask(libc::SIG_BLOCK, std::ptr::null(), &mut current); + libc::sigismember(¤t, libc::SIGTERM) == 1 + } + } + + #[cfg(unix)] + #[test] + fn the_spawn_guard_lifts_sigterm_and_hands_the_mask_back() { + // Start where every daemon thread starts once `serve_sigterm` has run. + unsafe { + let mut set: libc::sigset_t = std::mem::zeroed(); + libc::sigemptyset(&mut set); + libc::sigaddset(&mut set, libc::SIGTERM); + libc::pthread_sigmask(libc::SIG_BLOCK, &set, std::ptr::null_mut()); + } + assert!( + sigterm_is_blocked_here(), + "the state the guard is written for" + ); + + { + let _guard = SigtermUnblocked::for_spawn(); + assert!( + !sigterm_is_blocked_here(), + "a `fork` here must hand the child an unblocked SIGTERM — this is \ + the whole fix: `execve` keeps the mask, so a pane that inherits \ + the block cannot be stopped by `kill`" + ); + } + + assert!( + sigterm_is_blocked_here(), + "and the daemon's own thread takes its mask back, so `sigwait` keeps \ + its claim on shutdown" + ); + } + #[test] fn signalled_status_is_not_reported_as_a_plain_exit() { use portable_pty::ExitStatus;