From 2c8f115b609d7ebb542cedba8dbb3fc626adfb1b Mon Sep 17 00:00:00 2001 From: can1357 Date: Fri, 8 May 2026 11:23:58 +0200 Subject: [PATCH] fix(brush-core-vendored): fixed pipeline stage session detach behavior during launch - Added pipeline membership tracking to command execution contexts and propagated it into simple command metadata. - Passed the pipeline flag into external command session setup so pipeline stages no longer trigger session detachment. - Updated related comments and session-action tests to cover first-stage and pipeline-stage non-terminal stdin behavior. --- crates/brush-core-vendored/src/commands.rs | 22 +++++++++++-------- crates/brush-core-vendored/src/interp.rs | 12 ++++++++++- crates/pi-natives/src/shell.rs | 25 +++++++++++----------- 3 files changed, 37 insertions(+), 22 deletions(-) diff --git a/crates/brush-core-vendored/src/commands.rs b/crates/brush-core-vendored/src/commands.rs index 23b043b9d..ac97d0011 100644 --- a/crates/brush-core-vendored/src/commands.rs +++ b/crates/brush-core-vendored/src/commands.rs @@ -315,6 +315,8 @@ pub struct SimpleCommand<'a, SE: extensions::ShellExtensions> { /// The process group ID to use for externally executed commands. This may be /// `None`, in which case the default behavior will be used. pub process_group_id: Option, + /// Whether this command is part of a multi-command pipeline. + pub in_pipeline: bool, /// Optional override for the `argv[0]` value presented to an externally /// spawned process. When `None`, `command_name` is used. @@ -350,6 +352,7 @@ impl<'a, SE: extensions::ShellExtensions> SimpleCommand<'a, SE> { use_functions: true, path_dirs: None, process_group_id: None, + in_pipeline: false, argv0: None, post_execute: None, } @@ -551,6 +554,7 @@ impl<'a, SE: extensions::ShellExtensions> SimpleCommand<'a, SE> { let result = execute_external_command( cmd_context, resolved_path.as_ref(), + self.in_pipeline, self.process_group_id, self.argv0.as_deref(), &self.args[1..], @@ -570,6 +574,7 @@ impl<'a, SE: extensions::ShellExtensions> SimpleCommand<'a, SE> { pub(crate) fn execute_external_command( context: ExecutionContext<'_, impl extensions::ShellExtensions>, executable_path: &str, + in_pipeline: bool, process_group_id: Option, argv0_override: Option<&str>, args: &[CommandArg], @@ -594,8 +599,7 @@ pub(crate) fn execute_external_command( // Figure out if we should be setting up a new process group. let new_pg = matches!(context.params.process_group_policy, ProcessGroupPolicy::NewProcessGroup); - let session_action = - child_session_action(new_pg, child_stdin_is_terminal, process_group_id.is_some()); + let session_action = child_session_action(new_pg, child_stdin_is_terminal, in_pipeline); // Compose the std::process::Command that encapsulates what we want to launch. // argv[0] defaults to context.command_name (the user-facing name of the @@ -960,11 +964,12 @@ 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 to call when joining an established pipeline -/// group (`!new_pg && in_pipeline_group`): `setsid()` would either fail with -/// EPERM or move the child into a fresh session, breaking the pipeline's shared -/// process group and its job-control signal propagation. Pipeline stages -/// therefore keep their pre-fix behavior (no detach). +/// `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). /// /// Foregrounding remains gated on `new_pg && child_stdin_is_terminal`. pub fn child_session_action( @@ -980,8 +985,7 @@ pub fn child_session_action( return ChildSessionAction::None; } - let joining_pipeline_group = !new_pg && in_pipeline_group; - if joining_pipeline_group { + if in_pipeline_group { return ChildSessionAction::None; } diff --git a/crates/brush-core-vendored/src/interp.rs b/crates/brush-core-vendored/src/interp.rs index bfb64b668..802c9e450 100644 --- a/crates/brush-core-vendored/src/interp.rs +++ b/crates/brush-core-vendored/src/interp.rs @@ -29,6 +29,8 @@ struct PipelineExecutionContext<'a, SE: extensions::ShellExtensions> { shell: commands::ShellForCommand<'a, SE>, /// Process group ID for spawned processes. process_group_id: Option, + /// Whether this command is part of a multi-command pipeline. + in_pipeline: bool, } /// Information about an expanded external command launch. @@ -698,11 +700,13 @@ async fn spawn_pipeline_processes( parent: shell, }, process_group_id, + in_pipeline: pipeline_len > 1, } } else { PipelineExecutionContext { shell: commands::ShellForCommand::ParentShell(shell), process_group_id, + in_pipeline: pipeline_len > 1, } }; @@ -965,6 +969,7 @@ impl Execute for ast::CoprocessCommand { let pipeline_context = PipelineExecutionContext { shell: commands::ShellForCommand::ParentShell(&mut child_shell), process_group_id: None, + in_pipeline: false, }; let spawn_result = body .execute_in_pipeline(pipeline_context, child_params) @@ -1489,7 +1494,11 @@ impl ExecuteInPipeline for ast::SimpleComma }; let context = - PipelineExecutionContext { shell, process_group_id: context.process_group_id }; + PipelineExecutionContext { + shell, + process_group_id: context.process_group_id, + in_pipeline: context.in_pipeline, + }; match execute_command(context, params, cmd_name, assignments, args).await { Ok(result) => Ok(result), @@ -1574,6 +1583,7 @@ async fn execute_command( // Construct the command struct. let mut cmd = commands::SimpleCommand::new(context.shell, params, cmd_name, args); cmd.process_group_id = context.process_group_id; + cmd.in_pipeline = context.in_pipeline; // Arrange to pop off that ephemeral environment scope. cmd.post_execute = Some(|shell| shell.env_mut().pop_scope(EnvironmentScope::Command)); diff --git a/crates/pi-natives/src/shell.rs b/crates/pi-natives/src/shell.rs index 1f96bd57b..ba216013e 100644 --- a/crates/pi-natives/src/shell.rs +++ b/crates/pi-natives/src/shell.rs @@ -1371,18 +1371,18 @@ mod tests { #[test] fn interactive_with_terminal_stdin_takes_foreground() { assert_eq!(child_session_action(true, true, false), ChildSessionAction::TakeForeground,); - // `in_pipeline_group` is meaningless when `new_pg` is true; result MUST - // not depend on it. + // Terminal foregrounding wins even when this is the first stage of a + // pipeline; no detach is attempted. assert_eq!(child_session_action(true, true, true), ChildSessionAction::TakeForeground,); } - /// Interactive brush leading its own pgroup but with non-terminal stdin - /// (e.g. redirected): detach so SIGTTIN/SIGTTOU on the inherited tty - /// cannot stop the parent. + /// 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. #[test] - fn interactive_with_non_terminal_stdin_detaches() { + fn non_terminal_stdin_leading_new_pgroup_detaches_unless_pipeline() { assert_eq!(child_session_action(true, false, false), ChildSessionAction::DetachSession,); - assert_eq!(child_session_action(true, false, true), ChildSessionAction::DetachSession,); + assert_eq!(child_session_action(true, false, true), ChildSessionAction::None,); } /// Non-interactive brush, terminal stdin, no pipeline: nothing to do. @@ -1408,11 +1408,12 @@ mod tests { } /// **Pipeline carve-out.** Non-interactive brush, non-terminal stdin - /// (pipe), joining an established pipeline pgroup: MUST NOT detach. - /// `setsid()` would either fail with EPERM or move the child into a new - /// session, breaking the pipeline's shared process group and its - /// job-control signal propagation. This is the regression Codex flagged - /// in PR #895. + /// (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. #[test] fn pipeline_stage_does_not_detach() { assert_eq!(child_session_action(false, false, true), ChildSessionAction::None,);