From 9c715fea998d9122ba2681900a56634029b17433 Mon Sep 17 00:00:00 2001 From: Chris McClellan Date: Thu, 30 Apr 2026 06:24:03 -0400 Subject: [PATCH 1/2] fix: keep scope-intercept alive on SIGINT/SIGTERM/SIGHUP so child can clean up MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When scope-intercept wraps a long-running process (e.g. as the shebang on a server start script), Ctrl+C in the terminal delivers SIGINT to every member of the foreground process group — both scope-intercept and the wrapped command. Without a custom signal handler, scope-intercept follows the default and aborts immediately, closing the captured stdout/stderr pipes. The child then dies with SIGPIPE before its own SIGINT trap can finish, leaving services like overmind/nginx orphaned. Install Tokio Unix signal listeners for SIGINT, SIGTERM, and SIGHUP at the start of run_command. Their only job is to consume the default- terminate behavior so scope-intercept stays alive while the child runs its trap. The OS already delivered the signal to the child via the shared process group, so no manual forwarding is needed. When an interrupt has fired and the child exits non-zero, skip the known-error analysis, retry, and bug-report flows — the user asked to quit, not to be quizzed. OutputCapture and the scope doctor path are intentionally untouched; this is scoped to the intercept binary, which is the one wrapping interactive long-lived workloads. Includes a new regression test that spawns scope-intercept in its own process group, sends SIGINT via killpg (mimicking a terminal), and asserts the child's bash trap ran and the exit code is 130. Co-Authored-By: Claude Opus 4.7 --- Cargo.lock | 1 + Cargo.toml | 1 + src/bin/scope-intercept.rs | 29 ++++++++++ tests/scope_intercept.rs | 108 +++++++++++++++++++++++++++++++++++++ 4 files changed, 139 insertions(+) diff --git a/Cargo.lock b/Cargo.lock index bc3f23d..9b68d38 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -789,6 +789,7 @@ dependencies = [ "jsonschema", "jsonwebtoken", "lazy_static", + "libc", "minijinja", "mockall", "nanoid", diff --git a/Cargo.toml b/Cargo.toml index df17634..a3b8dd6 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -80,6 +80,7 @@ which = { version = "8.0", features = ["regex"] } assert_cmd = "2.2.0" assert_fs = "1.1.3" escargot = "0.5.15" +libc = "0.2" predicates = "3.1.4" tempfile = "3.27" 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..0ba0a22 100644 --- a/tests/scope_intercept.rs +++ b/tests/scope_intercept.rs @@ -1,6 +1,9 @@ use assert_fs::fixture::{FileWriteStr, PathChild}; 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}; #[allow(dead_code)] mod common; @@ -158,3 +161,108 @@ 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 process group so killpg(pid, SIGINT) + // hits both it and the wrapped bash, mirroring how a terminal delivers + // Ctrl+C to the foreground process group. + unsafe { + command.pre_exec(|| { + if libc::setsid() == -1 { + return Err(std::io::Error::last_os_error()); + } + Ok(()) + }); + } + let mut child = command.spawn().expect("failed to spawn scope-intercept"); + + // Wait for the bash trap to be installed. + 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 pid = child.id() as i32; + // SAFETY: killpg(2) with a valid pgid and SIGINT delivers to every + // member of the group. scope-intercept is the group leader (setsid + // above), so this hits both it and the wrapped bash. + let rc = unsafe { libc::killpg(pid, libc::SIGINT) }; + assert_eq!( + rc, + 0, + "killpg({pid}, SIGINT) failed: {}", + std::io::Error::last_os_error() + ); + + // Wait for scope-intercept to exit. If it doesn't shut down within 10s, + // the bug is back: scope-intercept is hanging instead of letting the + // child run its trap and propagating the exit. + let exit_deadline = Instant::now() + Duration::from_secs(10); + let status = loop { + match child.try_wait().expect("try_wait failed") { + Some(status) => break status, + None => { + if Instant::now() > exit_deadline { + child.kill().ok(); + panic!("scope-intercept did not exit within 10s after SIGINT"); + } + std::thread::sleep(Duration::from_millis(50)); + } + } + }; + + 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(); +} From 014749fdeafb9060d5f46142e552f0f0af802a88 Mon Sep 17 00:00:00 2001 From: Chris McClellan Date: Thu, 30 Apr 2026 09:42:08 -0400 Subject: [PATCH 2/2] test: address review feedback on scope-intercept SIGINT test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Per review on #309: - Replace raw libc::setsid / libc::killpg with the typed `nix` wrappers (`nix::unistd::setsid`, `nix::sys::signal::killpg` with `Pid` and `Signal`). Drops the `as i32` cast and the manual errno handling. - Add a SAFETY comment on the `unsafe { command.pre_exec(...) }` block spelling out the async-signal-safety contract pre_exec requires. - Replace the try_wait + sleep + deadline polling loop with `wait_timeout::ChildExt::wait_timeout`, which is the standard idiom for "wait for a child with a deadline". - Annotate the remaining poll loop (waiting for the bash trap to install a marker file) explaining why a sleep is needed there — no good cross-process file-appeared notification primitive without inotify/kqueue. Cargo.toml: drop `libc`, add `nix = "0.29"` (signal+process features) and `wait-timeout = "0.2"` as dev-deps. Test still passes; full suite remains green. Co-Authored-By: Claude Opus 4.7 --- Cargo.lock | 15 +++++++++- Cargo.toml | 3 +- tests/scope_intercept.rs | 60 +++++++++++++++++----------------------- 3 files changed, 42 insertions(+), 36 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 9b68d38..e4206f0 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -789,10 +789,10 @@ dependencies = [ "jsonschema", "jsonwebtoken", "lazy_static", - "libc", "minijinja", "mockall", "nanoid", + "nix", "normpath", "octocrab", "opentelemetry", @@ -825,6 +825,7 @@ dependencies = [ "tracing-subscriber", "url", "vergen", + "wait-timeout", "which", ] @@ -2182,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 a3b8dd6..07bb8d4 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -80,9 +80,10 @@ which = { version = "8.0", features = ["regex"] } assert_cmd = "2.2.0" assert_fs = "1.1.3" escargot = "0.5.15" -libc = "0.2" +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/tests/scope_intercept.rs b/tests/scope_intercept.rs index 0ba0a22..46c63f3 100644 --- a/tests/scope_intercept.rs +++ b/tests/scope_intercept.rs @@ -1,9 +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; @@ -197,20 +200,21 @@ fn test_intercept_forwards_sigint_and_waits_for_child_cleanup() { .current_dir(&work_dir) .env("NO_COLOR", "1") .args(["--", "bash", "trap.sh"]); - // Put scope-intercept in its own process group so killpg(pid, SIGINT) + // 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(|| { - if libc::setsid() == -1 { - return Err(std::io::Error::last_os_error()); - } - Ok(()) - }); + command.pre_exec(|| setsid().map(|_| ()).map_err(std::io::Error::from)); } let mut child = command.spawn().expect("failed to spawn scope-intercept"); - // Wait for the bash trap to be installed. + // 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 { @@ -222,32 +226,20 @@ fn test_intercept_forwards_sigint_and_waits_for_child_cleanup() { std::thread::sleep(Duration::from_millis(50)); } - let pid = child.id() as i32; - // SAFETY: killpg(2) with a valid pgid and SIGINT delivers to every - // member of the group. scope-intercept is the group leader (setsid - // above), so this hits both it and the wrapped bash. - let rc = unsafe { libc::killpg(pid, libc::SIGINT) }; - assert_eq!( - rc, - 0, - "killpg({pid}, SIGINT) failed: {}", - std::io::Error::last_os_error() - ); - - // Wait for scope-intercept to exit. If it doesn't shut down within 10s, - // the bug is back: scope-intercept is hanging instead of letting the - // child run its trap and propagating the exit. - let exit_deadline = Instant::now() + Duration::from_secs(10); - let status = loop { - match child.try_wait().expect("try_wait failed") { - Some(status) => break status, - None => { - if Instant::now() > exit_deadline { - child.kill().ok(); - panic!("scope-intercept did not exit within 10s after SIGINT"); - } - 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"); } };