mirror of
https://github.com/l0ng-ai/tty7.git
synced 2026-09-22 16:02:24 +00:00
fix(pane): let kill reach what runs in a pane
`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.
This commit is contained in:
@@ -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
|
/// 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
|
/// the crate's own constructor so an upstream rewording fails there rather
|
||||||
/// than here.
|
/// 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<libc::sigset_t>);
|
||||||
|
|
||||||
|
#[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<i32> {
|
fn reported_exit_code(status: &portable_pty::ExitStatus) -> Option<i32> {
|
||||||
match status.to_string().starts_with("Terminated by") {
|
match status.to_string().starts_with("Terminated by") {
|
||||||
true => None,
|
true => None,
|
||||||
@@ -1320,6 +1360,25 @@ impl DaemonPane {
|
|||||||
let pair = native_pty_system().openpty(pty_size)?;
|
let pair = native_pty_system().openpty(pty_size)?;
|
||||||
let spawn = build_spawn_config(id, cwd, shell, workspace.as_deref())?;
|
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 child = pair.slave.spawn_command(spawn.cmd)?;
|
||||||
let shell_pid = child.process_id();
|
let shell_pid = child.process_id();
|
||||||
let child = Arc::new(Mutex::new(child));
|
let child = Arc::new(Mutex::new(child));
|
||||||
@@ -4561,6 +4620,47 @@ mod tests {
|
|||||||
assert!(rx.try_recv().is_err());
|
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]
|
#[test]
|
||||||
fn signalled_status_is_not_reported_as_a_plain_exit() {
|
fn signalled_status_is_not_reported_as_a_plain_exit() {
|
||||||
use portable_pty::ExitStatus;
|
use portable_pty::ExitStatus;
|
||||||
|
|||||||
Reference in New Issue
Block a user