smashimo/oh-my-pi
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix(brush-core-vendored): fixed pipeline stage session detach behavior during launch

Browse Source 
- 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.
This commit is contained in:
can1357
2026-05-08 11:23:58 +02:00
parent afc2da34e0
commit 2c8f115b60
3 changed files with 37 additions and 22 deletions
Download Patch File Download Diff File Expand all files Collapse all files
crates/brush-core-vendored/src/commands.rs
+13 -9
View File
@@ -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<i32>,
/// 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<i32>,
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;
}
crates/brush-core-vendored/src/interp.rs
+11 -1
View File
@@ -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<i32>,
/// 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<SE: extensions::ShellExtensions> ExecuteInPipeline<SE> 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));
crates/pi-natives/src/shell.rs
+13 -12
View File
@@ -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,);