fix: use signal handler in shim (#4313)

### Description
I believe this fixes #3711

#4196 added graceful shutting down when calling `go-turbo`, this does
the same but for when we need to invoke the correct Rust binary in
`shim.rs`. I extracted the logic added in #4196 to a function that can
be used throughout the codebase.

### Testing Instructions

Currently only doing manual via the reproduction provided in #3711.
Edit: I originally wanted to add integration test to cover these cases,
this has proven to be a challenge. Will still look at trying to
orchestrate a test for this, but considering how much traction the
related issues have, I don't want to block on getting an integration
test.

Author: chris-olszewski

Cargo.lock

@@ -0,0 +1,31 @@
+Setup
+  $ . ${TESTDIR}/../setup.sh
+  $ . ${TESTDIR}/setup.sh $(pwd)
+
+Run script with INT handler and verify that INT gets passed to script
+
+Start turbo in the background
+  $ ${TURBO} trap &
+Save the PID of turbo
+  $ TURBO_PID=$!
+We send INT to turbo, but with a delay to give us time to bring turbo back to
+the foreground.
+  $ sh -c "sleep 1 && kill -2 ${TURBO_PID}" &
+Bring turbo back to the foreground
+  $ fg 1
+  ${TURBO} trap
+  \xe2\x80\xa2 Packages in scope: test (esc)
+  \xe2\x80\xa2 Running trap in 1 packages (esc)
+  \xe2\x80\xa2 Remote caching disabled (esc)
+  test:trap: cache miss, executing d25759ee0a8e12ae
+  test:trap: 
+  test:trap: > trap
+  test:trap: > trap 'echo trap hit; sleep 1; echo trap finish' INT; sleep 5 && echo 'script finish'
+  test:trap: 
+  test:trap: trap hit
+  test:trap: trap finish
+  test:trap: npm ERR! Lifecycle script `trap` failed with error: 
+  test:trap: npm ERR! Error: command failed 
+  test:trap: npm ERR!   in workspace: test 
+  test:trap: npm ERR!   at location: .*ctrlc.t/apps/test  (re)
+  [1]

cli/integration_tests/signal_handling/ctrlc.t

@@ -0,0 +1,8 @@
+#!/bin/bash
+# Enable jobcontrol as we need it for fg to work
+set -m
+
+SCRIPT_DIR=$(dirname ${BASH_SOURCE[0]})
+TARGET_DIR=$1
+cp -a ${SCRIPT_DIR}/test_repo/. ${TARGET_DIR}/
+${SCRIPT_DIR}/../setup_git.sh ${TARGET_DIR}

cli/integration_tests/signal_handling/setup.sh

@@ -0,0 +1,6 @@
+{
+  "name": "test",
+  "scripts": {
+    "trap": "trap 'echo trap hit; sleep 1; echo trap finish' INT; sleep 5 && echo 'script finish'"
+  }
+}

cli/integration_tests/signal_handling/test_repo/apps/test/package.json

@@ -0,0 +1,7 @@
+{
+  "name": "signal-test",
+  "workspaces": [
+    "apps/*"
+  ],
+  "packageManager": "npm@8.0.0"
+}

cli/integration_tests/signal_handling/test_repo/package.json

@@ -0,0 +1,6 @@
+{
+  "$schema": "https://turbo.build/schema.json",
+  "pipeline": {
+    "trap": {}
+  }
+}

cli/integration_tests/signal_handling/test_repo/turbo.json

@@ -31,6 +31,7 @@ clap_complete = { workspace = true }
 command-group = { version = "2.1.0", features = ["with-tokio"] }
 config = "0.13"
 console = { workspace = true }
+ctrlc = { version = "3.2.5", features = ["termination"] }
 dialoguer = { workspace = true, features = ["fuzzy-select"] }
 dirs-next = "2.0.0"
 dunce = { workspace = true }
@@ -42,6 +43,7 @@ hostname = "0.3.1"
 humantime = "2.1.0"
 indicatif = { workspace = true }
 lazy_static = { workspace = true }
+libc = "0.2.140"
 log = { workspace = true }
 notify = { version = "5.1.0", default-features = false, features = [
   "macos_kqueue",
@@ -55,6 +57,7 @@ serde = { workspace = true, features = ["derive"] }
 serde_json = { workspace = true }
 serde_yaml = { workspace = true }
 sha2 = "0.10.6"
+shared_child = "1.0.0"
 sysinfo = "0.27.7"
 thiserror = "1.0.38"
 tiny-gradient = { workspace = true }

cli/internal/ffi/libgit2.a

@@ -0,0 +1,29 @@
+use std::{process::Command, sync::Arc};
+
+use anyhow::Result;
+use shared_child::SharedChild;
+
+/// Spawns a child in a way where SIGINT is correctly forwarded to the child
+pub fn spawn_child(mut command: Command) -> Result<Arc<SharedChild>> {
+    let shared_child = Arc::new(SharedChild::spawn(&mut command)?);
+    let handler_shared_child = shared_child.clone();
+
+    ctrlc::set_handler(move || {
+        // on windows, we can't send signals so just kill
+        // we are quiting anyways so just ignore
+        #[cfg(target_os = "windows")]
+        handler_shared_child.kill().ok();
+
+        // on unix, we should send a SIGTERM to the child
+        // so that go can gracefully shut down process groups
+        // SAFETY: we could pull in the nix crate to handle this
+        // 'safely' but nix::sys::signal::kill just calls libc::kill
+        #[cfg(not(target_os = "windows"))]
+        unsafe {
+            libc::kill(handler_shared_child.id() as i32, libc::SIGTERM);
+        }
+    })
+    .expect("handler set");
+
+    Ok(shared_child)
+}

crates/turborepo-lib/Cargo.toml

@@ -1,5 +1,6 @@
 #![feature(assert_matches)]
 
+mod child;
 mod cli;
 mod client;
 mod commands;
@@ -11,6 +12,7 @@ mod shim;
 mod ui;
 
 use anyhow::Result;
+pub use child::spawn_child;
 use log::error;
 
 pub use crate::cli::Args;

crates/turborepo-lib/src/child.rs

@@ -21,7 +21,7 @@ use serde::{Deserialize, Serialize};
 use tiny_gradient::{GradientStr, RGB};
 use turbo_updater::check_for_updates;
 
-use crate::{cli, get_version, package_manager::Globs, PackageManager, Payload};
+use crate::{cli, get_version, package_manager::Globs, spawn_child, PackageManager, Payload};
 
 // all arguments that result in a stdout that much be directly parsable and
 // should not be paired with additional output (from the update notifier for
@@ -649,18 +649,21 @@ impl RepoState {
 
         // We spawn a process that executes the local turbo
         // that we've found in node_modules/.bin/turbo.
-        let mut command = process::Command::new(local_turbo_path)
+        let mut command = process::Command::new(local_turbo_path);
+        command
             .args(&raw_args)
             // rather than passing an argument that local turbo might not understand, set
             // an environment variable that can be optionally used
             .env(cli::INVOCATION_DIR_ENV_VAR, &shim_args.invocation_dir)
             .current_dir(cwd)
             .stdout(Stdio::inherit())
-            .stderr(Stdio::inherit())
-            .spawn()
-            .expect("Failed to execute turbo.");
+            .stderr(Stdio::inherit());
 
-        Ok(command.wait()?.code().unwrap_or(2))
+        let child = spawn_child(command)?;
+
+        let exit_code = child.wait()?.code().unwrap_or(2);
+
+        Ok(exit_code)
     }
 }
 

crates/turborepo-lib/src/lib.rs

@@ -27,14 +27,11 @@ anyhow = { workspace = true, features = ["backtrace"] }
 clap = { workspace = true, features = ["derive"] }
 clap_complete = { workspace = true }
 command-group = { version = "2.0.1", features = ["with-tokio"] }
-ctrlc = { version = "3.2.5", features = ["termination"] }
 dunce = { workspace = true }
-libc = "0.2.140"
 log = { workspace = true }
 serde = { workspace = true, features = ["derive"] }
 serde_json = { workspace = true }
 serde_yaml = { workspace = true }
-shared_child = "1.0.0"
 tiny-gradient = { workspace = true }
 tokio-util = { version = "0.7.7", features = ["io"] }
 turborepo-lib = { workspace = true }

crates/turborepo-lib/src/shim.rs

@@ -7,7 +7,7 @@ use std::{
 use anyhow::Result;
 use dunce::canonicalize as fs_canonicalize;
 use log::{debug, error, trace};
-use turborepo_lib::{Args, Payload};
+use turborepo_lib::{spawn_child, Args, Payload};
 
 fn run_go_binary(args: Args) -> Result<i32> {
     // canonicalize the binary path to ensure we can find go-turbo
@@ -49,28 +49,8 @@ fn run_go_binary(args: Args) -> Result<i32> {
         .stdout(Stdio::inherit())
         .stderr(Stdio::inherit());
 
-    let shared_child = shared_child::SharedChild::spawn(&mut command).unwrap();
-    let child_arc = std::sync::Arc::new(shared_child);
-
-    let child_arc_clone = child_arc.clone();
-    ctrlc::set_handler(move || {
-        // on windows, we can't send signals so just kill
-        // we are quiting anyways so just ignore
-        #[cfg(target_os = "windows")]
-        child_arc_clone.kill().ok();
-
-        // on unix, we should send a SIGTERM to the child
-        // so that go can gracefully shut down process groups
-        // SAFETY: we could pull in the nix crate to handle this
-        // 'safely' but nix::sys::signal::kill just calls libc::kill
-        #[cfg(not(target_os = "windows"))]
-        unsafe {
-            libc::kill(child_arc_clone.id() as i32, libc::SIGTERM);
-        }
-    })
-    .expect("handler set");
-
-    let exit_code = child_arc.wait()?.code().unwrap_or(2);
+    let child = spawn_child(command)?;
+    let exit_code = child.wait()?.code().unwrap_or(2);
 
     Ok(exit_code)
 }