Consolidate unsafe process code into fabro-proc crate

Rename fabro-proctitle to fabro-proc and add safe wrappers for all
process management primitives (signals, pre-exec hooks). This contains
all unsafe proc code behind a safe API so downstream crates no longer
need #[allow(unsafe_code)] or direct libc dependencies.

New modules: signal (process_alive, sigterm, sigkill, sigterm_process_group),
pre_exec (pre_exec_setsid, pre_exec_setpgid, pre_exec_pdeathsig),
title (existing proctitle code). Eliminates three duplicate process_alive
definitions and removes libc as a direct dep of fabro-cli and fabro-sandbox.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This commit is contained in:
Bryan Helmkamp 2026-04-01 17:49:29 -04:00
parent 6c0eb0efb9
commit 2178cf16dd
No known key found for this signature in database
17 changed files with 129 additions and 102 deletions

7
Cargo.lock generated
View file

@ -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",

View file

@ -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 }

View file

@ -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]

View file

@ -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<dyn Fn(&str) + Send + Sync>
});
let store = store::build_store(&run_record.settings.storage_dir())?;

View file

@ -62,21 +62,7 @@ pub(crate) fn active_launcher_record_for_run(run_dir: &Path) -> Option<LauncherR
}
pub(crate) fn launcher_record_is_running(record: &LauncherRecord) -> 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)]

View file

@ -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)]

View file

@ -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<std::process::Child> {
if !resume {
ensure_startable_run(run_dir)?;
@ -50,15 +49,7 @@ pub(crate) fn start_run(run_dir: &Path, resume: bool) -> Result<std::process::Ch
.stdin(std::process::Stdio::null());
#[cfg(unix)]
{
use std::os::unix::process::CommandExt;
unsafe {
cmd.pre_exec(|| {
libc::setsid();
Ok(())
});
}
}
fabro_proc::pre_exec_setsid(&mut cmd);
let mut child = cmd.spawn()?;

View file

@ -1,6 +1,3 @@
#![allow(unsafe_code)]
use std::os::unix::process::CommandExt;
use std::process::{Child, Command};
use tracing::{debug, warn};
@ -24,12 +21,7 @@ impl LinuxSleepInhibitor {
/// Spawn a command with `PR_SET_PDEATHSIG` so the child is automatically
/// killed if the parent process dies (prevents orphan `sleep infinity`).
fn spawn_with_pdeathsig(cmd: &mut Command) -> std::io::Result<Child> {
unsafe {
cmd.pre_exec(|| {
libc::prctl(libc::PR_SET_PDEATHSIG, libc::SIGTERM);
Ok(())
});
}
fabro_proc::pre_exec_pdeathsig(cmd);
cmd.spawn()
}

View file

@ -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

View file

@ -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;

View file

@ -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(())
});
}
}

View file

@ -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);
}
}
}

View file

@ -1,5 +1,3 @@
#![allow(unsafe_code)]
use std::sync::{Mutex, OnceLock};
struct Buffer {

View file

@ -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"

View file

@ -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()