diff --git a/src/shell/commands/executable.rs b/src/shell/commands/executable.rs index 2d5a75d..dfea6cf 100644 --- a/src/shell/commands/executable.rs +++ b/src/shell/commands/executable.rs @@ -59,32 +59,49 @@ impl ShellCommand for ExecutableCommand { context.state.track_child_process(&child); + // Track the spawned child process in the kill signal + let kill_signal = context.state.kill_signal().clone(); + if let Some(pid) = child.id() { + kill_signal.set_child_process(pid); + } + // avoid deadlock since this is holding onto the pipes drop(sub_command); + // Get the child's PGID for signal forwarding decisions + #[cfg(unix)] + let child_pgid = kill_signal.current_child_pgid(); + loop { tokio::select! { result = child.wait() => match result { - Ok(status) => return ExecuteResult::Continue( - status.code().unwrap_or(1), - Vec::new(), - Vec::new(), - ), + Ok(status) => { + kill_signal.clear_child_process(); + return ExecuteResult::Continue( + status.code().unwrap_or(1), + Vec::new(), + Vec::new(), + ); + } Err(err) => { + kill_signal.clear_child_process(); let _ = stderr.write_line(&format!("{}", err)); return ExecuteResult::from_exit_code(1); } }, - signal = context.state.kill_signal().wait_any() => { + signal = kill_signal.wait_any() => { if let Some(_id) = child.id() { #[cfg(unix)] - kill(_id as i32, signal); + if signal.should_forward_to(child_pgid) { + kill(_id as i32, signal.kind); + } - if cfg!(not(unix)) && signal.causes_abort() { + if cfg!(not(unix)) && signal.kind.causes_abort() { let _ = child.start_kill(); let status = child.wait().await.ok(); + kill_signal.clear_child_process(); return ExecuteResult::from_exit_code( - status.and_then(|s| s.code()).unwrap_or(signal.aborted_code()), + status.and_then(|s| s.code()).unwrap_or(signal.kind.aborted_code()), ); } } diff --git a/src/shell/commands/mod.rs b/src/shell/commands/mod.rs index 0a2d7e2..0f58bc9 100644 --- a/src/shell/commands/mod.rs +++ b/src/shell/commands/mod.rs @@ -131,7 +131,7 @@ macro_rules! execute_with_cancellation { result }, signal = $kill_signal.wait_aborted() => { - ExecuteResult::from_exit_code(signal.aborted_code()) + ExecuteResult::from_exit_code(signal.kind.aborted_code()) } } }; diff --git a/src/shell/mod.rs b/src/shell/mod.rs index 7f30d6b..f0a8d64 100644 --- a/src/shell/mod.rs +++ b/src/shell/mod.rs @@ -15,6 +15,7 @@ pub use types::ShellPipeReader; pub use types::ShellPipeWriter; pub use types::ShellState; pub use types::SignalKind; +pub use types::SignalMessage; pub use types::pipe; pub use which::CommandPathResolutionError; diff --git a/src/shell/types.rs b/src/shell/types.rs index 3ff5ede..d6f66ff 100644 --- a/src/shell/types.rs +++ b/src/shell/types.rs @@ -498,6 +498,17 @@ pub fn pipe() -> (ShellPipeReader, ShellPipeWriter) { ) } +/// Information about the current child process being tracked. +#[derive(Debug, Clone, Copy, Default)] +struct ChildProcessInfo { + /// The PID of the current foreground child process. + pid: Option, + /// The PGID of the current foreground child process (Unix only). + /// Cached at spawn time to avoid repeated syscalls. + #[cfg(unix)] + pgid: Option, +} + #[derive(Debug)] struct KillSignalInner { // WARNING: This should struct should not be made Sync. @@ -507,24 +518,26 @@ struct KillSignalInner { // then awaited after. If an abort happened between that then // it could be missed. aborted_code: RefCell>, - sender: broadcast::Sender, + sender: broadcast::Sender, children: RefCell>>, + /// Information about the current child process. + child_process: Cell, } impl KillSignalInner { - pub fn send(&self, signal_kind: SignalKind) { - if signal_kind.causes_abort() { + pub fn send(&self, signal: SignalMessage) { + if signal.kind.causes_abort() { let mut stored_aborted_code = self.aborted_code.borrow_mut(); if stored_aborted_code.is_none() { - *stored_aborted_code = Some(signal_kind.aborted_code()); + *stored_aborted_code = Some(signal.kind.aborted_code()); } } - _ = self.sender.send(signal_kind); + _ = self.sender.send(signal); // notify children self.children.borrow_mut().retain(|weak_child| { if let Some(child) = weak_child.upgrade() { - child.send(signal_kind); + child.send(signal); true } else { false // clean-up dropped children @@ -544,6 +557,7 @@ impl Default for KillSignal { aborted_code: RefCell::new(None), sender, children: Default::default(), + child_process: Cell::new(ChildProcessInfo::default()), })) } } @@ -562,6 +576,7 @@ impl KillSignal { aborted_code: RefCell::new(self.aborted_code()), sender, children: RefCell::new(Vec::new()), + child_process: Cell::new(ChildProcessInfo::default()), }); // Add the child to the parent's list of children @@ -584,29 +599,81 @@ impl KillSignal { } } - /// Send a signal to commands being run. + /// Send a signal to commands being run (programmatic, always forwarded). pub fn send(&self, signal: SignalKind) { - self.0.send(signal) + self.0.send(SignalMessage::new(signal)) + } + + /// Send a signal that originated from a specific process group. + /// + /// Use this when forwarding signals that came from the terminal (e.g., Ctrl+C). + /// Children in the same process group as `origin_pgid` will not receive + /// a duplicate signal, since the terminal already sent it to them directly. + #[cfg(unix)] + pub fn send_from_pgid(&self, signal: SignalKind, origin_pgid: i32) { + self.0.send(SignalMessage::from_pgid(signal, origin_pgid)) } /// Waits for only signals deemed to abort a command. - pub async fn wait_aborted(&self) -> SignalKind { + pub async fn wait_aborted(&self) -> SignalMessage { let mut receiver = self.0.sender.subscribe(); loop { // unwrap is ok because we're holding a sender in `self` let signal = receiver.recv().await.unwrap(); - if signal.causes_abort() { + if signal.kind.causes_abort() { return signal; } } } /// Waits for any signal to be received. - pub async fn wait_any(&self) -> SignalKind { + pub async fn wait_any(&self) -> SignalMessage { let mut receiver = self.0.sender.subscribe(); // unwrap is ok because we're holding a sender in `self` receiver.recv().await.unwrap() } + + /// Returns the PID of the current foreground child process, if any. + /// + /// This is useful for signal forwarding scenarios where you need to check + /// if the child process is in the same process group as the parent. + pub fn current_child_pid(&self) -> Option { + self.0.child_process.get().pid + } + + /// Returns the PGID of the current foreground child process, if any. + /// + /// The PGID is cached at spawn time to avoid repeated syscalls. + /// This is useful for determining whether to forward signals: + /// if the child is in the same process group, the terminal's signal + /// will already reach it directly. + #[cfg(unix)] + pub fn current_child_pgid(&self) -> Option { + self.0.child_process.get().pgid + } + + /// Called internally when a child process is spawned. + /// + /// On Unix, this also caches the child's PGID. + pub(crate) fn set_child_process(&self, pid: u32) { + #[cfg(unix)] + let pgid = { + // Cache the PGID at spawn time + let pgid = unsafe { nix::libc::getpgid(pid as i32) }; + if pgid > 0 { Some(pgid) } else { None } + }; + + self.0.child_process.set(ChildProcessInfo { + pid: Some(pid), + #[cfg(unix)] + pgid, + }); + } + + /// Called internally when a child process exits. + pub(crate) fn clear_child_process(&self) { + self.0.child_process.set(ChildProcessInfo::default()); + } } /// Guard that on drop will send a signal on the associated `KillSignal`. @@ -643,6 +710,62 @@ pub enum SignalKind { Other(i32), } +/// A signal message that includes origin information to prevent double-signaling. +/// +/// When a signal originates from the terminal (e.g., Ctrl+C), it's sent to all +/// processes in the foreground process group. If the shell also forwards the +/// signal programmatically, children would receive it twice. The `origin_pgid` +/// field allows recipients to skip forwarding if the child is in the same +/// process group as the signal origin. +#[derive(Debug, Clone, Copy)] +pub struct SignalMessage { + pub kind: SignalKind, + /// The process group ID where this signal originated. + /// If Some, children in the same PGID already received this signal + /// from the terminal and should not be signaled again. + /// If None, the signal is programmatic and should always be forwarded. + #[cfg(unix)] + pub origin_pgid: Option, +} + +impl SignalMessage { + /// Create a new signal message for programmatic signals (always forwarded). + pub fn new(kind: SignalKind) -> Self { + Self { + kind, + #[cfg(unix)] + origin_pgid: None, + } + } + + /// Create a new signal message originating from a specific process group. + /// Children in the same PGID will not receive a duplicate signal. + #[cfg(unix)] + pub fn from_pgid(kind: SignalKind, pgid: i32) -> Self { + Self { + kind, + origin_pgid: Some(pgid), + } + } + + /// Check if a signal should be forwarded to a child process. + /// Returns true if the signal should be forwarded, false if the child + /// already received it from the terminal. + #[cfg(unix)] + pub fn should_forward_to(&self, child_pgid: Option) -> bool { + match (self.origin_pgid, child_pgid) { + (Some(origin), Some(child)) => origin != child, + _ => true, // Forward if we can't determine (programmatic or unknown PGID) + } + } + + /// On non-Unix platforms, always forward signals. + #[cfg(not(unix))] + pub fn should_forward_to(&self, _child_pgid: Option) -> bool { + true + } +} + impl SignalKind { pub fn causes_abort(&self) -> bool { match self { @@ -730,7 +853,7 @@ mod test { // Wait for the signal in the main task let signal = kill_signal.wait_any().await; - assert_eq!(signal, SignalKind::SIGTERM); + assert_eq!(signal.kind, SignalKind::SIGTERM); } #[tokio::test] @@ -753,7 +876,7 @@ mod test { ); for signal in [signals.0, signals.1, signals.2].into_iter() { - assert_eq!(signal, SignalKind::SIGKILL); + assert_eq!(signal.kind, SignalKind::SIGKILL); } assert_eq!(child_signal.aborted_code(), Some(128 + 9)); assert_eq!(sibling_signal.aborted_code(), Some(128 + 9)); @@ -789,7 +912,7 @@ mod test { // Wait for the aborting signal in the main task let signal = kill_signal.wait_aborted().await; - assert_eq!(signal, SignalKind::SIGABRT); + assert_eq!(signal.kind, SignalKind::SIGABRT); assert!(kill_signal.aborted_code().is_some()); } @@ -811,7 +934,7 @@ mod test { // Wait for the signal in the child let signal = child_signal.wait_aborted().await; - assert_eq!(signal, SignalKind::SIGQUIT); + assert_eq!(signal.kind, SignalKind::SIGQUIT); assert_eq!(parent_signal.aborted_code(), Some(128 + 3)); assert_eq!(child_signal.aborted_code(), Some(128 + 3)); } @@ -836,7 +959,7 @@ mod test { // Verify no panic occurred and the parent still functions let signal = parent_signal.wait_any().await; - assert_eq!(signal, SignalKind::SIGTERM); + assert_eq!(signal.kind, SignalKind::SIGTERM); } #[tokio::test] @@ -860,4 +983,83 @@ mod test { Some(SignalKind::SIGTERM.aborted_code()) ); } + + #[test] + fn test_kill_signal_child_process_tracking() { + let kill_signal = KillSignal::default(); + + // Initially no child process + assert_eq!(kill_signal.current_child_pid(), None); + #[cfg(unix)] + assert_eq!(kill_signal.current_child_pgid(), None); + + // Set a child process + kill_signal.set_child_process(1234); + assert_eq!(kill_signal.current_child_pid(), Some(1234)); + // PGID is retrieved via syscall, so it might be None for a fake PID + + // Clear the child process + kill_signal.clear_child_process(); + assert_eq!(kill_signal.current_child_pid(), None); + #[cfg(unix)] + assert_eq!(kill_signal.current_child_pgid(), None); + } + + #[test] + fn test_child_signal_has_separate_child_process_tracking() { + let parent_signal = KillSignal::default(); + let child_signal = parent_signal.child_signal(); + + // Set child process on parent + parent_signal.set_child_process(1234); + assert_eq!(parent_signal.current_child_pid(), Some(1234)); + // Child signal should not see parent's child process + assert_eq!(child_signal.current_child_pid(), None); + + // Set child process on child signal + child_signal.set_child_process(5678); + assert_eq!(child_signal.current_child_pid(), Some(5678)); + // Parent should still see its own child process + assert_eq!(parent_signal.current_child_pid(), Some(1234)); + } + + #[cfg(unix)] + #[test] + fn test_signal_message_should_forward_to() { + use super::SignalMessage; + + // Programmatic signal (no origin_pgid) should always forward + let programmatic = SignalMessage::new(SignalKind::SIGINT); + assert!(programmatic.should_forward_to(Some(1000))); + assert!(programmatic.should_forward_to(Some(2000))); + assert!(programmatic.should_forward_to(None)); + + // Signal from PGID 1000 should NOT forward to child in same PGID + let from_tty = SignalMessage::from_pgid(SignalKind::SIGINT, 1000); + assert!(!from_tty.should_forward_to(Some(1000))); // Same PGID - don't forward + assert!(from_tty.should_forward_to(Some(2000))); // Different PGID - forward + assert!(from_tty.should_forward_to(None)); // Unknown PGID - forward (safe default) + } + + #[cfg(unix)] + #[tokio::test] + async fn test_send_from_pgid() { + let kill_signal = KillSignal::default(); + + // Spawn a task to send a signal from a specific PGID + let signal_sender = kill_signal.clone(); + deno_unsync::spawn(async move { + signal_sender.send_from_pgid(SignalKind::SIGINT, 12345); + }); + + // Wait for the signal + let signal = kill_signal.wait_any().await; + assert_eq!(signal.kind, SignalKind::SIGINT); + assert_eq!(signal.origin_pgid, Some(12345)); + + // Same PGID should not forward + assert!(!signal.should_forward_to(Some(12345))); + // Different PGID should forward + assert!(signal.should_forward_to(Some(99999))); + } }