diff --git a/Cargo.lock b/Cargo.lock index bc3f23d..e4206f0 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -792,6 +792,7 @@ dependencies = [ "minijinja", "mockall", "nanoid", + "nix", "normpath", "octocrab", "opentelemetry", @@ -824,6 +825,7 @@ dependencies = [ "tracing-subscriber", "url", "vergen", + "wait-timeout", "which", ] @@ -2181,6 +2183,18 @@ dependencies = [ "tempfile", ] +[[package]] +name = "nix" +version = "0.29.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "71e2746dc3a24dd78b3cfcb7be93368c6de9963d30f43a6a73998a9cf4b17b46" +dependencies = [ + "bitflags 2.9.1", + "cfg-if", + "cfg_aliases", + "libc", +] + [[package]] name = "normalize-line-endings" version = "0.3.0" diff --git a/Cargo.toml b/Cargo.toml index df17634..07bb8d4 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -80,8 +80,10 @@ which = { version = "8.0", features = ["regex"] } assert_cmd = "2.2.0" assert_fs = "1.1.3" escargot = "0.5.15" +nix = { version = "0.29", features = ["signal", "process"] } predicates = "3.1.4" tempfile = "3.27" +wait-timeout = "0.2" [build-dependencies] anyhow = "1.0.102" diff --git a/src/bin/scope-intercept.rs b/src/bin/scope-intercept.rs index db4663c..ad5140b 100644 --- a/src/bin/scope-intercept.rs +++ b/src/bin/scope-intercept.rs @@ -6,7 +6,9 @@ use human_panic::setup_panic; use std::env; use std::io::Cursor; use std::sync::Arc; +use std::sync::atomic::{AtomicBool, Ordering}; use tokio::io::BufReader; +use tokio::signal::unix::{SignalKind, signal}; use tracing::{Level, enabled, error, info, warn}; /// A wrapper CLI that can be used to capture output from a program, check if there are known errors @@ -76,6 +78,26 @@ async fn run_command(opts: Cli) -> anyhow::Result { let current_dir = std::env::current_dir()?; let path = env::var("PATH").unwrap_or_default(); + // SIGINT/SIGTERM/SIGHUP are delivered to every member of our foreground + // process group, including the child, so its own cleanup traps run. We + // just need to keep ourselves alive long enough for the child to finish + // shutting down — otherwise our captured stdout/stderr pipes close and + // the child dies with SIGPIPE before its trap can complete. + let interrupted = Arc::new(AtomicBool::new(false)); + for kind in [ + SignalKind::interrupt(), + SignalKind::terminate(), + SignalKind::hangup(), + ] { + let mut stream = signal(kind)?; + let interrupted = interrupted.clone(); + tokio::spawn(async move { + while stream.recv().await.is_some() { + interrupted.store(true, Ordering::SeqCst); + } + }); + } + let capture = OutputCapture::capture_output(CaptureOpts { working_dir: ¤t_dir, args: &command, @@ -93,6 +115,13 @@ async fn run_command(opts: Cli) -> anyhow::Result { return Ok(exit_code); } + // The user explicitly asked to quit — don't surprise them with + // known-error prompts, retry, or bug-report offers. Just propagate the + // child's exit code (typically 130 for SIGINT, 143 for SIGTERM). + if interrupted.load(Ordering::SeqCst) { + return Ok(exit_code); + } + error!(target: "user", "Command failed, checking for a known error"); let found_config = opts.config_options.load_config().await.unwrap_or_else(|e| { error!(target: "user", "Unable to load configs from disk: {:?}", e); diff --git a/tests/scope_intercept.rs b/tests/scope_intercept.rs index 75a74db..46c63f3 100644 --- a/tests/scope_intercept.rs +++ b/tests/scope_intercept.rs @@ -1,6 +1,12 @@ use assert_fs::fixture::{FileWriteStr, PathChild}; +use nix::sys::signal::{Signal, killpg}; +use nix::unistd::{Pid, setsid}; use predicates::boolean::PredicateBooleanExt; use predicates::prelude::predicate; +use std::os::unix::process::CommandExt; +use std::process::Command as StdCommand; +use std::time::{Duration, Instant}; +use wait_timeout::ChildExt; #[allow(dead_code)] mod common; @@ -158,3 +164,97 @@ fn test_intercept_no_known_errors_match() { helper.clean_work_dir(); } + +// When scope-intercept wraps a long-running interactive process (e.g. +// used as a shebang on a server start script), Ctrl+C must let the +// child run its own SIGINT trap and exit cleanly. Before this fix, +// scope-intercept terminated immediately on SIGINT, closing the +// captured stdout/stderr pipes and killing the child with SIGPIPE +// before its trap could complete. +#[test] +fn test_intercept_forwards_sigint_and_waits_for_child_cleanup() { + let helper = ScopeTestHelper::new( + "test_intercept_forwards_sigint_and_waits_for_child_cleanup", + "empty", + ); + + let work_dir = helper.work_dir.path().to_path_buf(); + let cleanup_marker = work_dir.join("cleanup.done"); + let ready_marker = work_dir.join("ready"); + + helper + .work_dir + .child("trap.sh") + .write_str( + "#!/bin/bash\n\ + trap 'echo trapped > cleanup.done; exit 130' INT\n\ + touch ready\n\ + # sleep in short increments so the trap fires promptly\n\ + for _ in $(seq 1 30); do sleep 1; done\n", + ) + .unwrap(); + + let intercept_bin = assert_cmd::cargo::cargo_bin("scope-intercept"); + let mut command = StdCommand::new(intercept_bin); + command + .current_dir(&work_dir) + .env("NO_COLOR", "1") + .args(["--", "bash", "trap.sh"]); + // Put scope-intercept in its own session/process group so killpg below + // hits both it and the wrapped bash, mirroring how a terminal delivers + // Ctrl+C to the foreground process group. + // + // SAFETY: pre_exec runs the closure between fork and exec, so it must + // call only async-signal-safe functions. setsid(2) is on the POSIX + // async-signal-safe list and touches no shared state in the parent. + unsafe { + command.pre_exec(|| setsid().map(|_| ()).map_err(std::io::Error::from)); + } + let mut child = command.spawn().expect("failed to spawn scope-intercept"); + + // Poll for the ready marker. There's no good cross-process "file + // appeared" primitive without bringing in inotify/kqueue, so we + // sleep between checks to avoid burning a core. + let deadline = Instant::now() + Duration::from_secs(5); + while !ready_marker.exists() { + if Instant::now() > deadline { + child.kill().ok(); + panic!( + "trap.sh never wrote ready marker; scope-intercept may not be running the script" + ); + } + std::thread::sleep(Duration::from_millis(50)); + } + + let pgid = Pid::from_raw(child.id() as i32); + killpg(pgid, Signal::SIGINT).expect("killpg(pgid, SIGINT) failed"); + + // If scope-intercept doesn't shut down within 10s, the bug is back: + // it's hanging instead of letting the child run its trap and + // propagating the exit. + let status = match child + .wait_timeout(Duration::from_secs(10)) + .expect("wait_timeout failed") + { + Some(status) => status, + None => { + child.kill().ok(); + panic!("scope-intercept did not exit within 10s after SIGINT"); + } + }; + + assert!( + cleanup_marker.exists(), + "child SIGINT trap did not run — cleanup.done was never created" + ); + let body = std::fs::read_to_string(&cleanup_marker).unwrap(); + assert_eq!(body.trim(), "trapped"); + + assert_eq!( + status.code(), + Some(130), + "scope-intercept should propagate the child's exit code (130 for SIGINT), got {status:?}" + ); + + helper.clean_work_dir(); +}