fix: corrected child-session behavior to DetachSession for non-term stdin

- Renamed non-terminal stdin pipeline tests and updated non-terminal expectations to DetachSession behavior.
- Updated child_session_action behavior so non-terminal, non-pipeline stdin now yields DetachSession instead of None.
- Updated execute_external_command to skip process_group for detached children and move take_foreground into its foreground arm.
- Added async regression coverage for detached pipeline stages and logged the fix in the natives CHANGELOG.
This commit is contained in:
can1357
2026-05-30 20:55:55 +02:00
parent 1c968a7bfc
commit 4ee35bec7a
4 changed files with 199 additions and 53 deletions
+49 -35
View File
@@ -617,37 +617,46 @@ pub(crate) fn execute_external_command(
// Set up process group/session state.
//
// A child we are about to `setsid()` (`DetachSession`) must NOT also be
// handed a `process_group(...)`. For a would-be new-group leader it would
// duplicate the group `setsid` already creates; for a pipeline stage joining
// an established group it is a cross-session `setpgid` that fails with EPERM
// now that the leader (and every prior stage) has moved into its own session.
// In both cases `setsid` alone gives the child its own session and process
// group. See `child_session_action` for the decision rationale.
let command_leads_session = new_pg
&& matches!(session_action, ChildSessionAction::TakeForeground)
&& context.shell.options().external_cmd_leads_session;
if new_pg {
match session_action {
ChildSessionAction::DetachSession => {
// `detach_session()` calls `setsid()`, which creates a fresh session
// and process group; requesting `process_group(0)` as well would
// conflict with that setup.
}
ChildSessionAction::TakeForeground if command_leads_session => {
// Don't set process_group(0) - setsid() in pre_exec will handle it.
cmd.lead_session();
}
ChildSessionAction::TakeForeground | ChildSessionAction::None => {
// Normal case: create new process group in current session.
match session_action {
ChildSessionAction::DetachSession => {
// setsid() creates the fresh session + process group; no process_group().
cmd.detach_session();
}
ChildSessionAction::TakeForeground if command_leads_session => {
// Don't set process_group(0) - setsid() in pre_exec will handle it.
cmd.lead_session();
}
ChildSessionAction::TakeForeground => {
// Foreground a child that is not leading its own session: create/join
// the process group in the current session, then grab the terminal.
if new_pg {
cmd.process_group(0);
} else if let Some(pgid) = process_group_id {
cmd.process_group(pgid);
}
cmd.take_foreground();
}
ChildSessionAction::None => {
// Normal case: create a new process group in the current session, or
// join an established one (later pipeline stages).
if new_pg {
cmd.process_group(0);
} else if let Some(pgid) = process_group_id {
cmd.process_group(pgid);
}
}
} else if let Some(pgid) = process_group_id {
// We need to join an established process group.
cmd.process_group(pgid);
}
// See `child_session_action` for the decision rationale and call-out about
// pipeline groups.
match session_action {
ChildSessionAction::DetachSession => cmd.detach_session(),
ChildSessionAction::TakeForeground if !command_leads_session => cmd.take_foreground(),
ChildSessionAction::TakeForeground | ChildSessionAction::None => {}
}
// When tracing is enabled, report.
@@ -964,18 +973,27 @@ pub enum ChildSessionAction {
/// child inherited the host's controlling tty, and any `/dev/tty` open or
/// `tcsetpgrp` call from the child could SIGTTIN/SIGTTOU and stop the host.
///
/// `detach_session()` is unsafe for any member of a multi-command pipeline:
/// for the first stage it puts the process-group leader in a different session,
/// causing later stages' `setpgid()` to fail with EPERM; for later stages it
/// either fails with EPERM or moves the child into a fresh session, breaking the
/// pipeline's shared process group and job-control signal propagation. Pipeline
/// stages therefore keep their pre-fix behavior (no detach).
/// A child whose stdin is **not** a terminal therefore always detaches, even
/// when it is a stage of a multi-command pipeline. An interactive program in a
/// pipeline (`zsh -i ... | awk`) would otherwise open `/dev/tty`, `tcsetpgrp`
/// itself to the foreground, and leave the host stopped on its next tty read.
/// `setsid()` puts each stage in its own session with no controlling tty, so it
/// cannot reach `/dev/tty` at all. The historical EPERM hazard — a later stage
/// `setpgid()`-joining a leader that already moved to a new session — is avoided
/// in `execute_external_command`, which skips `process_group(...)` entirely for
/// detached children; pipeline stages no longer share one process group, which
/// the embedded host does not rely on (it cancels via the descendant tree, and
/// pipes are session-independent).
///
/// `in_pipeline_group` is no longer consulted: a pipeline stage that legitimately
/// needs the shared tty group has terminal stdin and is handled by the
/// `child_stdin_is_terminal` arm before pipeline membership would ever matter.
///
/// Foregrounding remains gated on `new_pg && child_stdin_is_terminal`.
pub fn child_session_action(
new_pg: bool,
child_stdin_is_terminal: bool,
in_pipeline_group: bool,
_in_pipeline_group: bool,
) -> ChildSessionAction {
if new_pg && child_stdin_is_terminal {
return ChildSessionAction::TakeForeground;
@@ -985,9 +1003,5 @@ pub fn child_session_action(
return ChildSessionAction::None;
}
if in_pipeline_group {
return ChildSessionAction::None;
}
ChildSessionAction::DetachSession
}
+9 -4
View File
@@ -356,9 +356,11 @@ mod tests {
}
#[test]
fn non_terminal_stdin_leading_new_pgroup_detaches_unless_pipeline() {
fn non_terminal_stdin_detaches_regardless_of_pipeline() {
assert_eq!(child_session_action(true, false, false), ChildSessionAction::DetachSession);
assert_eq!(child_session_action(true, false, true), ChildSessionAction::None);
// A leading-new-pgroup stage of a pipeline still detaches: setsid keeps
// it off the host's controlling tty.
assert_eq!(child_session_action(true, false, true), ChildSessionAction::DetachSession);
}
#[test]
@@ -377,8 +379,11 @@ mod tests {
}
#[test]
fn pipeline_stage_does_not_detach() {
assert_eq!(child_session_action(false, false, true), ChildSessionAction::None);
fn pipeline_stage_with_non_terminal_stdin_detaches() {
// Regression: an interactive child inside a pipeline (`zsh -i | awk`)
// must not stay in the host session and seize its tty. Pre-fix this
// returned `None`, leaving the stage attached and able to SIGTTIN the host.
assert_eq!(child_session_action(false, false, true), ChildSessionAction::DetachSession);
}
}
+137 -14
View File
@@ -1634,13 +1634,16 @@ mod tests {
assert_eq!(child_session_action(true, true, true), ChildSessionAction::TakeForeground,);
}
/// Brush leading a new pgroup with non-terminal stdin detaches only when
/// it is not part of a multi-command pipeline. Pipeline leaders must stay
/// in the parent session so later stages can join their process group.
/// Brush leading a new pgroup with non-terminal stdin always detaches —
/// including the first stage of a pipeline. `setsid()` keeps the child
/// off the host's controlling tty; the spawn path skips
/// `process_group(...)` for detached children, so later stages no
/// longer try to `setpgid`-join a leader that has moved sessions (the
/// historical EPERM hazard).
#[test]
fn non_terminal_stdin_leading_new_pgroup_detaches_unless_pipeline() {
fn non_terminal_stdin_detaches_regardless_of_pipeline() {
assert_eq!(child_session_action(true, false, false), ChildSessionAction::DetachSession,);
assert_eq!(child_session_action(true, false, true), ChildSessionAction::None,);
assert_eq!(child_session_action(true, false, true), ChildSessionAction::DetachSession,);
}
/// Non-interactive brush, terminal stdin, no pipeline: nothing to do.
@@ -1665,16 +1668,16 @@ mod tests {
assert_eq!(child_session_action(false, false, false), ChildSessionAction::DetachSession,);
}
/// **Pipeline carve-out.** Non-interactive brush, non-terminal stdin
/// (pipe), and a multi-command pipeline: MUST NOT detach. For the first
/// external stage, `setsid()` puts the process-group leader into a
/// different session, so later stages fail to join its group with
/// EPERM. For later stages, `setsid()` would either fail with EPERM or
/// move the child into a new session, breaking the pipeline's shared
/// process group and job-control signal propagation.
/// **Pipeline tty-safety.** Non-interactive brush, non-terminal stdin
/// (pipe), and a multi-command pipeline: detach. An interactive child in
/// a pipeline (`zsh -i ... | awk`) would otherwise open `/dev/tty`,
/// `tcsetpgrp` itself to the foreground, and leave the host stopped on
/// its next tty read (`suspended (tty input)`). Each stage gets its own
/// session instead; the embedded host cancels via the descendant tree,
/// not a shared pgroup, and pipes are session-independent.
#[test]
fn pipeline_stage_does_not_detach() {
assert_eq!(child_session_action(false, false, true), ChildSessionAction::None,);
fn pipeline_stage_with_non_terminal_stdin_detaches() {
assert_eq!(child_session_action(false, false, true), ChildSessionAction::DetachSession,);
}
}
@@ -1796,6 +1799,126 @@ mod tests {
);
}
/// Regression for the `suspended (tty input)` bug: an **interactive child
/// inside a pipeline** (`zsh -i ... | awk`) used to stay in the host
/// session, open `/dev/tty`, `tcsetpgrp` itself to the foreground, and
/// leave the embedded host (OMP) stopped on its next tty read. The earlier
/// embedded-host fix carved pipelines out of `detach_session` because a
/// later stage that `setpgid`-joined a detached leader failed with EPERM.
///
/// This test boots a real embedded `BrushShell` and runs a two-stage
/// pipeline whose first stage prints its PID then sleeps (forwarded to us
/// by `cat`). It asserts two contracts at once:
/// 1. the first stage runs in its **own session** (`getsid == own pid`),
/// so it can never reach the host's controlling tty — guards the
/// decision; and
/// 2. the pipeline still exits **successfully**, proving the second stage
/// spawned without the cross-session `setpgid` EPERM — guards the
/// wiring that skips `process_group(...)` for detached children.
#[cfg(unix)]
#[tokio::test(flavor = "multi_thread")]
async fn embedded_pipeline_stage_runs_in_its_own_session() {
use std::io::Read as _;
// SAFETY: `getsid(0)` only queries the current process session; checked below.
let host_sid = unsafe { libc::getsid(0) };
assert!(host_sid > 0, "getsid(0) failed: {}", std::io::Error::last_os_error());
let config = ShellConfig { session_env: None, snapshot_path: None, minimizer: None };
let mut session = create_session(&config).await.expect("create_session");
let (mut reader, writer) = pipe_to_files("e2e-pipe").expect("pipe");
let stdout_file = OpenFile::from(writer.try_clone().expect("clone"));
let stderr_file = OpenFile::from(writer);
let mut params = session.shell.default_exec_params();
params.set_fd(OpenFiles::STDIN_FD, null_file().expect("null stdin"));
params.set_fd(OpenFiles::STDOUT_FD, stdout_file);
params.set_fd(OpenFiles::STDERR_FD, stderr_file);
let (pid_tx, pid_rx) = tokio::sync::oneshot::channel::<i32>();
let reader_handle = tokio::task::spawn_blocking(move || {
let mut buf = Vec::new();
let mut chunk = [0u8; 64];
let mut pid_tx = Some(pid_tx);
while let Ok(n) = reader.read(&mut chunk)
&& n > 0
{
buf.extend_from_slice(&chunk[..n]);
if pid_tx.is_some()
&& let Some(line_end) = buf.iter().position(|&byte| byte == b'\n')
&& let Ok(line) = std::str::from_utf8(&buf[..line_end])
&& let Ok(pid) = line.trim().parse::<i32>()
{
let _ = pid_tx
.take()
.expect("pid sender should be present")
.send(pid);
}
}
buf
});
let shell_handle = tokio::spawn(async move {
let source_info = SourceInfo::from("pi-natives:test");
// First stage prints its own PID and sleeps; `cat` forwards the PID
// line to our reader and exits on EOF. The first stage leads the
// pipeline's process group, the second (`cat`) is the join-or-detach
// stage that would EPERM without the wiring fix.
let exec = session
.shell
.run_string(
"/bin/sh -c 'printf \"%d\\n\" \"$$\"; sleep 1' | /bin/cat",
&source_info,
&params,
)
.await
.expect("run_string");
drop(params);
(session, exec)
});
let child_pid = time::timeout(Duration::from_secs(5), pid_rx)
.await
.expect("timed out waiting for first-stage PID")
.expect("reader closed pid channel without sending");
assert!(child_pid > 0, "got non-positive child pid: {child_pid}");
// SAFETY: `child_pid` is a live positive PID (still in `sleep`); the return
// value is checked.
let child_sid = unsafe { libc::getsid(child_pid) };
assert!(
child_sid > 0,
"getsid({child_pid}) failed: {} (child may have already exited)",
std::io::Error::last_os_error(),
);
let (_session, exec) = time::timeout(Duration::from_secs(5), shell_handle)
.await
.expect("shell timed out")
.expect("shell task panicked");
// Guards the wiring: the second stage spawned without a cross-session
// `setpgid` EPERM, so the whole pipeline succeeded.
assert!(
matches!(exec.exit_code, ExecutionExitCode::Success),
"pipeline did not succeed (second stage may have hit setpgid EPERM): {}",
exit_code(&exec),
);
let _ = time::timeout(Duration::from_secs(2), reader_handle).await;
// Guards the decision: a pipeline stage must not share the host session,
// or it could seize the controlling tty and SIGTTIN the host.
assert_ne!(
child_sid, host_sid,
"pipeline stage PID {child_pid} inherited host session {host_sid}; it could seize the \
controlling tty — the pipeline tty-suspend bug is back",
);
assert_eq!(
child_sid, child_pid,
"pipeline stage PID {child_pid} should be its own session leader after setsid",
);
}
#[tokio::test]
async fn abort_state_signals_cancel_token() {
let abort_state = ShellAbortState::default();
+4
View File
@@ -2,6 +2,10 @@
## [Unreleased]
### Fixed
- Fixed an interactive shell inside a **pipeline** (`zsh -i ... | awk`, `time zsh -i | cat`, etc.) suspending the embedded host with `suspended (tty input)`. The earlier embedded-host fix `setsid`-detached external children so they could not seize the host's controlling tty, but carved pipeline stages out because a later stage that `setpgid`-joined a detached leader failed with EPERM — leaving every pipeline stage in the host session, where an interactive child opened `/dev/tty`, `tcsetpgrp`'d itself to the foreground, and stopped the host (OMP) on its next tty read. `pi_shell` now detaches pipeline stages too: `child_session_action` returns `DetachSession` for any non-terminal-stdin child regardless of pipeline membership, and `execute_external_command` skips `process_group(...)` entirely for detached children so no cross-session `setpgid` is attempted. Pipeline stages no longer share one process group, which the embedded host does not rely on (cancellation walks the descendant tree and pipes are session-independent).
## [15.6.0] - 2026-05-30
### Changed