diff --git a/Cargo.lock b/Cargo.lock index ca64ec5b0..1d27c383e 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1505,7 +1505,7 @@ dependencies = [ "fabro-mcp", "fabro-model", "fabro-oauth", - "fabro-proctitle", + "fabro-proc", "fabro-retro", "fabro-sandbox", "fabro-server", @@ -1522,7 +1522,6 @@ dependencies = [ "indicatif", "insta", "jsonwebtoken", - "libc", "object_store", "open", "paste", @@ -1758,7 +1757,7 @@ dependencies = [ ] [[package]] -name = "fabro-proctitle" +name = "fabro-proc" version = "0.176.2" dependencies = [ "cc", @@ -1796,11 +1795,11 @@ dependencies = [ "daytona-sdk", "fabro-config", "fabro-github", + "fabro-proc", "fabro-types", "futures", "git2", "glob", - "libc", "rand 0.8.5", "serde", "serde_json", diff --git a/lib/crates/fabro-cli/Cargo.toml b/lib/crates/fabro-cli/Cargo.toml index 1c34bb415..502302503 100644 --- a/lib/crates/fabro-cli/Cargo.toml +++ b/lib/crates/fabro-cli/Cargo.toml @@ -29,7 +29,7 @@ fabro-devcontainer = { path = "../fabro-devcontainer" } fabro-hooks = { path = "../fabro-hooks" } fabro-interview = { path = "../fabro-interview" } fabro-mcp = { path = "../fabro-mcp" } -fabro-proctitle = { path = "../fabro-proctitle" } +fabro-proc = { path = "../fabro-proc" } fabro-retro = { path = "../fabro-retro" } fabro-sandbox = { path = "../fabro-sandbox", features = ["daytona"] } fabro-checkpoint = { path = "../fabro-checkpoint" } @@ -82,9 +82,6 @@ shlex = "1" walkdir.workspace = true object_store.workspace = true -[target.'cfg(unix)'.dependencies] -libc = "0.2" - [target.'cfg(target_os = "macos")'.dependencies] core-foundation = { version = "0.9", optional = true } diff --git a/lib/crates/fabro-cli/src/commands/run/attach.rs b/lib/crates/fabro-cli/src/commands/run/attach.rs index c3466ff46..3c5ba883b 100644 --- a/lib/crates/fabro-cli/src/commands/run/attach.rs +++ b/lib/crates/fabro-cli/src/commands/run/attach.rs @@ -715,28 +715,14 @@ async fn determine_exit_code_with_store(run_store: &dyn RunStore, run_dir: &Path } } -#[allow(unsafe_code)] fn kill_engine(run_dir: &Path) { - if let Some(pid) = read_launcher_pid(run_dir).map(|pid| i32::try_from(pid).unwrap()) { - #[cfg(unix)] - unsafe { - libc::kill(pid, libc::SIGTERM); - } - let _ = pid; + if let Some(pid) = read_launcher_pid(run_dir) { + fabro_proc::sigterm(pid); } } -#[allow(unsafe_code)] fn process_alive(pid: u32) -> bool { - #[cfg(unix)] - { - unsafe { libc::kill(i32::try_from(pid).unwrap(), 0) == 0 } - } - #[cfg(not(unix))] - { - let _ = pid; - true - } + fabro_proc::process_alive(pid) } #[cfg(test)] @@ -887,7 +873,6 @@ mod tests { } #[test] - #[allow(unsafe_code)] fn engine_child_guard_defuse_keeps_alive() { let child = std::process::Command::new("sleep") .arg("60") @@ -908,9 +893,7 @@ mod tests { // Clean up #[cfg(unix)] - unsafe { - libc::kill(i32::try_from(pid).unwrap(), libc::SIGKILL); - } + fabro_proc::sigkill(pid); } #[test] diff --git a/lib/crates/fabro-cli/src/commands/run/detached.rs b/lib/crates/fabro-cli/src/commands/run/detached.rs index 56ea45633..9edf60c7c 100644 --- a/lib/crates/fabro-cli/src/commands/run/detached.rs +++ b/lib/crates/fabro-cli/src/commands/run/detached.rs @@ -14,7 +14,7 @@ use fabro_workflow::records::{RunRecord, RunRecordExt}; use crate::shared; use crate::store; pub(crate) async fn execute(run_dir: PathBuf, launcher_path: PathBuf, resume: bool) -> Result<()> { - let _ = fabro_proctitle::init(); + let _ = fabro_proc::title_init(); let _launcher_guard = scopeguard::guard(launcher_path.clone(), |path| { super::launcher::remove_launcher_record(&path); @@ -24,9 +24,9 @@ pub(crate) async fn execute(run_dir: PathBuf, launcher_path: PathBuf, resume: bo let on_node: fabro_workflow::OnNodeCallback = Some({ let run_id = run_record.run_id.to_string(); let short_id = super::short_run_id(&run_id).to_string(); - fabro_proctitle::set(&format!("fabro: {short_id}")); + fabro_proc::title_set(&format!("fabro: {short_id}")); Arc::new(move |node_id: &str| { - fabro_proctitle::set(&format!("fabro: {short_id} {node_id}")); + fabro_proc::title_set(&format!("fabro: {short_id} {node_id}")); }) as Arc }); let store = store::build_store(&run_record.settings.storage_dir())?; diff --git a/lib/crates/fabro-cli/src/commands/run/launcher.rs b/lib/crates/fabro-cli/src/commands/run/launcher.rs index 87e4ee8a4..67dee0f5a 100644 --- a/lib/crates/fabro-cli/src/commands/run/launcher.rs +++ b/lib/crates/fabro-cli/src/commands/run/launcher.rs @@ -62,21 +62,7 @@ pub(crate) fn active_launcher_record_for_run(run_dir: &Path) -> Option bool { - process_alive(record.pid) && launcher_process_matches(record) -} - -#[cfg(unix)] -#[allow(unsafe_code)] -fn process_alive(pid: u32) -> bool { - let Ok(pid) = i32::try_from(pid) else { - return false; - }; - unsafe { libc::kill(pid, 0) == 0 } -} - -#[cfg(not(unix))] -fn process_alive(_pid: u32) -> bool { - true + fabro_proc::process_alive(record.pid) && launcher_process_matches(record) } #[cfg(unix)] diff --git a/lib/crates/fabro-cli/src/commands/run/resume.rs b/lib/crates/fabro-cli/src/commands/run/resume.rs index 5bf104fdc..22826d810 100644 --- a/lib/crates/fabro-cli/src/commands/run/resume.rs +++ b/lib/crates/fabro-cli/src/commands/run/resume.rs @@ -65,20 +65,7 @@ pub(crate) async fn resume_command( fn launcher_pid_alive(run_dir: &std::path::Path) -> bool { super::launcher::active_launcher_record_for_run(run_dir) - .is_some_and(|record| process_alive(record.pid)) -} - -#[allow(unsafe_code)] -fn process_alive(pid: u32) -> bool { - #[cfg(unix)] - { - unsafe { libc::kill(i32::try_from(pid).unwrap(), 0) == 0 } - } - #[cfg(not(unix))] - { - let _ = pid; - true - } + .is_some_and(|record| fabro_proc::process_alive(record.pid)) } #[cfg(test)] diff --git a/lib/crates/fabro-cli/src/commands/run/start.rs b/lib/crates/fabro-cli/src/commands/run/start.rs index c454b0909..c838255f3 100644 --- a/lib/crates/fabro-cli/src/commands/run/start.rs +++ b/lib/crates/fabro-cli/src/commands/run/start.rs @@ -15,7 +15,6 @@ use super::launcher::{ /// /// The engine process reads `run.json` from the run directory and executes the /// workflow. Returns the child process handle (use `.id()` for the PID). -#[allow(unsafe_code)] pub(crate) fn start_run(run_dir: &Path, resume: bool) -> Result { if !resume { ensure_startable_run(run_dir)?; @@ -50,15 +49,7 @@ pub(crate) fn start_run(run_dir: &Path, resume: bool) -> Result std::io::Result { - unsafe { - cmd.pre_exec(|| { - libc::prctl(libc::PR_SET_PDEATHSIG, libc::SIGTERM); - Ok(()) - }); - } + fabro_proc::pre_exec_pdeathsig(cmd); cmd.spawn() } diff --git a/lib/crates/fabro-proctitle/Cargo.toml b/lib/crates/fabro-proc/Cargo.toml similarity index 61% rename from lib/crates/fabro-proctitle/Cargo.toml rename to lib/crates/fabro-proc/Cargo.toml index eb02ef89f..1d0adeff2 100644 --- a/lib/crates/fabro-proctitle/Cargo.toml +++ b/lib/crates/fabro-proc/Cargo.toml @@ -1,10 +1,10 @@ [package] -name = "fabro-proctitle" +name = "fabro-proc" edition.workspace = true version.workspace = true publish = false license.workspace = true -description = "In-place process title updates for Fabro detached engines" +description = "Safe wrappers for process management primitives (signals, pre-exec hooks, title rewriting)" [lints] workspace = true diff --git a/lib/crates/fabro-proctitle/build.rs b/lib/crates/fabro-proc/build.rs similarity index 100% rename from lib/crates/fabro-proctitle/build.rs rename to lib/crates/fabro-proc/build.rs diff --git a/lib/crates/fabro-proctitle/c/capture_argv.c b/lib/crates/fabro-proc/c/capture_argv.c similarity index 100% rename from lib/crates/fabro-proctitle/c/capture_argv.c rename to lib/crates/fabro-proc/c/capture_argv.c diff --git a/lib/crates/fabro-proc/src/lib.rs b/lib/crates/fabro-proc/src/lib.rs new file mode 100644 index 000000000..5872cd2e5 --- /dev/null +++ b/lib/crates/fabro-proc/src/lib.rs @@ -0,0 +1,19 @@ +#![allow(unsafe_code)] + +#[cfg(unix)] +mod pre_exec; +mod signal; +mod title; + +pub use title::{init as title_init, set as title_set}; + +pub use signal::process_alive; +#[cfg(unix)] +pub use signal::{sigkill, sigterm, sigterm_process_group}; + +#[cfg(target_os = "linux")] +pub use pre_exec::pre_exec_pdeathsig; +#[cfg(unix)] +pub use pre_exec::pre_exec_setpgid; +#[cfg(unix)] +pub use pre_exec::pre_exec_setsid; diff --git a/lib/crates/fabro-proc/src/pre_exec.rs b/lib/crates/fabro-proc/src/pre_exec.rs new file mode 100644 index 000000000..14f7657aa --- /dev/null +++ b/lib/crates/fabro-proc/src/pre_exec.rs @@ -0,0 +1,38 @@ +use std::os::unix::process::CommandExt; + +/// Register a `pre_exec` hook that calls `setsid()` to detach the child +/// into its own session. +pub fn pre_exec_setsid(cmd: &mut impl CommandExt) { + // SAFETY: setsid() is async-signal-safe per POSIX. + unsafe { + cmd.pre_exec(|| { + libc::setsid(); + Ok(()) + }); + } +} + +/// Register a `pre_exec` hook that calls `setpgid(0, 0)` to place the +/// child in its own process group. +pub fn pre_exec_setpgid(cmd: &mut impl CommandExt) { + // SAFETY: setpgid() is async-signal-safe per POSIX. + unsafe { + cmd.pre_exec(|| { + libc::setpgid(0, 0); + Ok(()) + }); + } +} + +/// Register a `pre_exec` hook that calls `prctl(PR_SET_PDEATHSIG, SIGTERM)` +/// so the child receives SIGTERM when its parent dies. +#[cfg(target_os = "linux")] +pub fn pre_exec_pdeathsig(cmd: &mut impl CommandExt) { + // SAFETY: prctl(PR_SET_PDEATHSIG, ...) is async-signal-safe. + unsafe { + cmd.pre_exec(|| { + libc::prctl(libc::PR_SET_PDEATHSIG, libc::SIGTERM); + Ok(()) + }); + } +} diff --git a/lib/crates/fabro-proc/src/signal.rs b/lib/crates/fabro-proc/src/signal.rs new file mode 100644 index 000000000..dffff68c5 --- /dev/null +++ b/lib/crates/fabro-proc/src/signal.rs @@ -0,0 +1,52 @@ +/// Check whether a process with the given PID is alive. +/// +/// On Unix, sends signal 0 via `kill(2)`. Returns `false` if the pid does not +/// fit in `i32`. On non-Unix platforms, conservatively returns `true`. +pub fn process_alive(pid: u32) -> bool { + #[cfg(unix)] + { + let Ok(pid) = i32::try_from(pid) else { + return false; + }; + // SAFETY: kill(pid, 0) is a read-only probe; it does not deliver a signal. + unsafe { libc::kill(pid, 0) == 0 } + } + #[cfg(not(unix))] + { + let _ = pid; + true + } +} + +/// Send SIGTERM to a single process. +#[cfg(unix)] +pub fn sigterm(pid: u32) { + if let Ok(pid) = i32::try_from(pid) { + // SAFETY: kill with a valid pid and SIGTERM is safe. + unsafe { + libc::kill(pid, libc::SIGTERM); + } + } +} + +/// Send SIGKILL to a single process. +#[cfg(unix)] +pub fn sigkill(pid: u32) { + if let Ok(pid) = i32::try_from(pid) { + // SAFETY: kill with a valid pid and SIGKILL is safe. + unsafe { + libc::kill(pid, libc::SIGKILL); + } + } +} + +/// Send SIGTERM to an entire process group. +#[cfg(unix)] +pub fn sigterm_process_group(pid: u32) { + if let Ok(pid) = i32::try_from(pid) { + // SAFETY: kill with -pid signals the process group. + unsafe { + libc::kill(-pid, libc::SIGTERM); + } + } +} diff --git a/lib/crates/fabro-proctitle/src/lib.rs b/lib/crates/fabro-proc/src/title.rs similarity index 99% rename from lib/crates/fabro-proctitle/src/lib.rs rename to lib/crates/fabro-proc/src/title.rs index f244a9e1f..4732b14f9 100644 --- a/lib/crates/fabro-proctitle/src/lib.rs +++ b/lib/crates/fabro-proc/src/title.rs @@ -1,5 +1,3 @@ -#![allow(unsafe_code)] - use std::sync::{Mutex, OnceLock}; struct Buffer { diff --git a/lib/crates/fabro-sandbox/Cargo.toml b/lib/crates/fabro-sandbox/Cargo.toml index 1457ebcec..9f52fc568 100644 --- a/lib/crates/fabro-sandbox/Cargo.toml +++ b/lib/crates/fabro-sandbox/Cargo.toml @@ -28,6 +28,7 @@ serde.workspace = true serde_json.workspace = true tracing.workspace = true base64.workspace = true +fabro-proc = { path = "../fabro-proc" } shlex = "1" # local @@ -50,9 +51,6 @@ daytona-sdk = { workspace = true, optional = true } daytona-api-client = { workspace = true, optional = true } git2 = { workspace = true, optional = true } -[target.'cfg(unix)'.dependencies] -libc = { version = "0.2" } - [dev-dependencies] tokio = { workspace = true, features = ["test-util", "macros"] } tempfile = "3" diff --git a/lib/crates/fabro-sandbox/src/local.rs b/lib/crates/fabro-sandbox/src/local.rs index 376f2f347..504cd2e16 100644 --- a/lib/crates/fabro-sandbox/src/local.rs +++ b/lib/crates/fabro-sandbox/src/local.rs @@ -197,15 +197,7 @@ impl Sandbox for LocalSandbox { .stderr(std::process::Stdio::piped()); #[cfg(unix)] - // SAFETY: setpgid(0, 0) is safe to call in a pre_exec hook — it places - // the child into its own process group so we can signal the whole group. - #[allow(unsafe_code)] - unsafe { - cmd.pre_exec(|| { - libc::setpgid(0, 0); - Ok(()) - }); - } + fabro_proc::pre_exec_setpgid(cmd.as_std_mut()); let mut child = cmd .spawn() @@ -481,15 +473,10 @@ impl Sandbox for LocalSandbox { } /// Send SIGTERM to the process group, wait 2s for graceful shutdown, then SIGKILL. -#[allow(unsafe_code)] async fn sigterm_then_kill(child: &mut Child) { #[cfg(unix)] if let Some(pid) = child.id() { - // SAFETY: kill with a negative pid signals the entire process group. - // The pid is valid because we just obtained it from child.id(). - unsafe { - libc::kill(-i32::try_from(pid).unwrap(), libc::SIGTERM); - } + fabro_proc::sigterm_process_group(pid); if time::timeout(std::time::Duration::from_secs(2), child.wait()) .await .is_err()