Skip to content

Commit 9b2c9ab

Browse files
committed
update to new killsignal approach
1 parent 066ca05 commit 9b2c9ab

File tree

2 files changed

+31
-49
lines changed

2 files changed

+31
-49
lines changed

Cargo.lock

Lines changed: 12 additions & 12 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

crates/pixi_cli/src/run.rs

Lines changed: 19 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ use std::{
66
};
77

88
use clap::Parser;
9-
use deno_task_shell::{KillSignal, ProcessSignaler};
9+
use deno_task_shell::KillSignal;
1010
use dialoguer::theme::ColorfulTheme;
1111
use fancy_display::FancyDisplay;
1212
use indicatif::ProgressDrawTarget;
@@ -401,9 +401,7 @@ async fn execute_task(
401401
};
402402
let cwd = task.working_directory()?;
403403

404-
// Use execute_with_signaler to get access to child process PIDs for
405-
// proper signal forwarding based on process groups.
406-
let (process_signaler, execute_future) = deno_task_shell::execute_with_signaler(
404+
let execute_future = deno_task_shell::execute(
407405
script,
408406
command_env.clone(),
409407
cwd,
@@ -412,8 +410,7 @@ async fn execute_task(
412410
);
413411

414412
// Execute the process and forward signals.
415-
let status_code =
416-
run_future_forwarding_signals(kill_signal, process_signaler, execute_future).await;
413+
let status_code = run_future_forwarding_signals(kill_signal, execute_future).await;
417414
if status_code != 0 {
418415
return Err(TaskExecutionError::NonZeroExitCode(status_code));
419416
}
@@ -481,7 +478,6 @@ fn reset_cursor() {
481478
/// Signal listeners and ctrl+c listening will be setup.
482479
pub async fn run_future_forwarding_signals<TOutput>(
483480
#[cfg_attr(windows, allow(unused_variables))] kill_signal: KillSignal,
484-
#[cfg_attr(windows, allow(unused_variables))] process_signaler: ProcessSignaler,
485481
future: impl std::future::Future<Output = TOutput>,
486482
) -> TOutput {
487483
fn spawn_future_with_cancellation(
@@ -506,10 +502,7 @@ pub async fn run_future_forwarding_signals<TOutput>(
506502
spawn_future_with_cancellation(listen_ctrl_c_windows(), token.clone());
507503

508504
#[cfg(unix)]
509-
spawn_future_with_cancellation(
510-
listen_and_forward_all_signals(kill_signal, process_signaler),
511-
token,
512-
);
505+
spawn_future_with_cancellation(listen_and_forward_all_signals(kill_signal), token);
513506

514507
future.await
515508
})
@@ -527,20 +520,18 @@ async fn listen_ctrl_c_windows() {
527520

528521
/// Listens to all incoming signals and forwards them to the child process.
529522
///
530-
/// For SIGINT in interactive mode when the child is in the same process group,
531-
/// we add a small delay before forwarding. This gives the terminal driver time
532-
/// to deliver SIGINT directly to the child (in the case of CTRL+C), while still
533-
/// ensuring that signals sent via `kill -INT` are forwarded.
523+
/// Signal forwarding follows UV's approach:
524+
/// - In interactive mode (stdin is TTY), we pass our PGID with the signal.
525+
/// If the child is in the same PGID, deno_task_shell skips forwarding because
526+
/// the terminal driver already delivered the signal to the entire process group.
527+
/// - In non-interactive mode, we always forward signals since there's no terminal
528+
/// driver involved and `kill -INT <pid>` is the expected way to send signals.
534529
///
535-
/// This approach mimics UV's signal handling:
536-
/// https://github.com/astral-sh/uv/blob/9d17dfa3537312b928f94479f632891f918c4760/crates/uv/src/child.rs#L156C21-L168C77
530+
/// Trade-off: `kill -INT <pixi>` won't work in interactive mode when the child
531+
/// is in the same PGID. Use CTRL+C instead, or `kill -TERM` which always forwards.
537532
#[cfg(unix)]
538-
async fn listen_and_forward_all_signals(
539-
kill_signal: KillSignal,
540-
process_signaler: ProcessSignaler,
541-
) {
533+
async fn listen_and_forward_all_signals(kill_signal: KillSignal) {
542534
use std::io::IsTerminal;
543-
use std::time::Duration;
544535

545536
use futures::FutureExt;
546537
use pixi_core::signals::SIGNALS;
@@ -557,29 +548,20 @@ async fn listen_and_forward_all_signals(
557548
}
558549

559550
let kill_signal = kill_signal.clone();
560-
let process_signaler = process_signaler.clone();
561551
futures.push(
562552
async move {
563553
let Ok(mut stream) = tokio::signal::unix::signal(signo.into()) else {
564554
return;
565555
};
566556
let signal_kind = signo.into();
567557
while let Some(()) = stream.recv().await {
568-
// For SIGINT in interactive mode when child is in same process
569-
// group, add a delay to let the terminal deliver the signal first.
570-
// This avoids double-delivery for CTRL+C while ensuring `kill -INT`
571-
// still works (since the child won't have received it from terminal).
572-
if signo == libc::SIGINT && is_interactive {
573-
if let Some(child_pid) = process_signaler.current_pid() {
574-
let child_pgid = unsafe { libc::getpgid(child_pid as i32) };
575-
if child_pgid == our_pgid {
576-
// Child is in same process group. Add a small delay
577-
// to let the terminal deliver the signal first.
578-
tokio::time::sleep(Duration::from_millis(100)).await;
579-
}
580-
}
558+
// In interactive mode, include our PGID so deno_task_shell
559+
// can handle same-PGID scenarios (CTRL+C vs kill -INT)
560+
if is_interactive {
561+
kill_signal.send_from_pgid(signal_kind, our_pgid);
562+
} else {
563+
kill_signal.send(signal_kind);
581564
}
582-
kill_signal.send(signal_kind);
583565
}
584566
}
585567
.boxed_local(),

0 commit comments

Comments
 (0)