refactor(fbuild-core): migrate to running-process-core 3.4 (crates.io) (#254)

Author: zackees

Cargo.lock

@@ -71,12 +71,8 @@ tracing-test = "0.2"
 
 # Process containment: all subprocess spawns the daemon performs (compilers,
 # esptool, qemu, simavr, node, npm, …) and any grandchildren they fork must
-# die with the daemon. Pinned to a pre-release rev that exposes the
-# `Containment` enum / `spawn_with_containment` API. The published
-# `running-process-core` 3.4.x dropped those in favor of
-# `ContainedProcessGroup` — fbuild will need a containment-layer
-# refactor before it can move to crates.io. See FastLED/fbuild#32.
-running-process-core = { git = "https://github.com/zackees/running-process", rev = "ff9d7972504f7a0dcee0f410274daf3b02e4fcc2", package = "running-process-core" }
+# die with the daemon. See FastLED/fbuild#32.
+running-process-core = "3.4"
 
 [profile.dev]
 debug = 0

Cargo.toml

@@ -56,10 +56,7 @@
 use std::process::{Child, Command};
 use std::sync::OnceLock;
 
-#[cfg(unix)]
-use running_process_core::ContainedProcessGroup;
-#[cfg(windows)]
-use running_process_core::{ContainedChild, ContainedProcessGroup, Containment};
+use running_process_core::{ContainedProcessGroup, ORIGINATOR_ENV_VAR};
 
 /// Global process-wide containment group. Initialised once by the
 /// daemon; remains `None` in non-daemon contexts (CLI binary, tests).
@@ -101,35 +98,48 @@ pub fn is_initialised() -> bool {
 /// Falls back to uncontained `Command::spawn` when no global group has
 /// been initialised (non-daemon binaries).
 ///
-/// **Windows**: delegates to `ContainedProcessGroup::spawn` which
-/// assigns the child to the Job Object — safe and stateless.
+/// **Windows**: spawn directly, then assign the child handle to a Job
+/// Object with `JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE`
+/// ([`windows_job::assign`]) — the same containment mechanism the
+/// pre-publication `running-process-core` rev used internally,
+/// reimplemented locally since the published 3.4 API no longer exposes
+/// `spawn_with_containment(_, Containment::Contained)`.
 ///
 /// **Unix**: installs a per-child `pre_exec` hook that creates a new
 /// process group (`setpgid(0, 0)`) and, on Linux, requests
-/// `PR_SET_PDEATHSIG(SIGKILL)`. This sidesteps
-/// [`ContainedProcessGroup::spawn_with_containment`]'s shared-pgid
-/// behaviour, which fails with EPERM on the second spawn after the
-/// first child (the pgid leader) has exited — the root cause of
+/// `PR_SET_PDEATHSIG(SIGKILL)`. We deliberately do not call
+/// `ContainedProcessGroup::spawn` here because the per-child pgid
+/// approach sidesteps an EPERM race that hit the second spawn after the
+/// first child (the pgid leader) had exited — the root cause of
 /// FastLED/fbuild#129. The Linux kernel's `PR_SET_PDEATHSIG` still
 /// enforces the "child dies with daemon" contract; macOS relies on
 /// process-group-leader death alone.
 pub fn spawn_contained(command: &mut Command) -> std::io::Result<Child> {
     #[cfg(windows)]
     {
-        match GLOBAL_GROUP.get() {
-            Some(group) => {
-                let ContainedChild { child, .. } =
-                    group.spawn_with_containment(command, Containment::Contained)?;
-                Ok(child)
-            }
-            None => command.spawn(),
+        let Some(group) = GLOBAL_GROUP.get() else {
+            return command.spawn();
+        };
+        inject_originator_env(command, group);
+        let mut child = command.spawn()?;
+        use std::os::windows::io::AsRawHandle;
+        if let Err(e) = windows_job::assign(child.as_raw_handle()) {
+            // The atomic spawn+assign that `ContainedProcessGroup::spawn_with_containment`
+            // used to provide is gone in 3.4. If assign fails after spawn
+            // succeeds, kill the orphan so the caller can't leak an
+            // uncontained child by accident.
+            let _ = child.kill();
+            let _ = child.wait();
+            return Err(e);
         }
+        Ok(child)
     }
     #[cfg(unix)]
     {
-        if GLOBAL_GROUP.get().is_none() {
+        let Some(group) = GLOBAL_GROUP.get() else {
             return command.spawn();
-        }
+        };
+        inject_originator_env(command, group);
         unix_install_pre_exec(command);
         command.spawn()
     }
@@ -141,28 +151,39 @@ pub fn spawn_contained(command: &mut Command) -> std::io::Result<Child> {
 pub fn spawn_detached(command: &mut Command) -> std::io::Result<Child> {
     #[cfg(windows)]
     {
-        match GLOBAL_GROUP.get() {
-            Some(group) => {
-                let ContainedChild { child, .. } =
-                    group.spawn_with_containment(command, Containment::Detached)?;
-                Ok(child)
-            }
-            None => command.spawn(),
+        // Detached: no Job Object assignment so the child survives
+        // when the daemon's job handle closes. We still inject the
+        // originator env var for cross-process correlation.
+        if let Some(group) = GLOBAL_GROUP.get() {
+            inject_originator_env(command, group);
         }
+        command.spawn()
     }
     #[cfg(unix)]
     {
         // Detached: create a new session so the child survives the
         // daemon thread that spawned it. Matches the upstream behaviour
         // but without joining any shared pgid.
-        if GLOBAL_GROUP.get().is_none() {
+        let Some(group) = GLOBAL_GROUP.get() else {
             return command.spawn();
-        }
+        };
+        inject_originator_env(command, group);
         unix_install_detached_pre_exec(command);
         command.spawn()
     }
 }
 
+/// Mirror of `ContainedProcessGroup::inject_originator_env`: stamp
+/// `RUNNING_PROCESS_ORIGINATOR=TOOL:PID` onto the command's env. We do
+/// this manually because the published 3.4 API only exposes it via
+/// `ContainedProcessGroup::spawn` (which returns its own
+/// `SpawnedChild`, not a `std::process::Child` — see #32).
+fn inject_originator_env(command: &mut Command, group: &ContainedProcessGroup) {
+    if let Some(value) = group.originator_value() {
+        command.env(ORIGINATOR_ENV_VAR, value);
+    }
+}
+
 /// Install a `pre_exec` hook that puts the child in a fresh process
 /// group (Unix) and, on Linux, asks the kernel to SIGKILL the child
 /// when the spawning thread exits.
@@ -240,8 +261,11 @@ pub mod tokio_spawn {
     pub fn spawn_contained(
         command: &mut tokio::process::Command,
     ) -> std::io::Result<tokio::process::Child> {
-        if !super::is_initialised() {
+        let Some(group) = super::GLOBAL_GROUP.get() else {
             return command.spawn();
+        };
+        if let Some(value) = group.originator_value() {
+            command.env(super::ORIGINATOR_ENV_VAR, value);
         }
         configure(command);
         let child = command.spawn()?;

crates/fbuild-core/src/containment.rs

@@ -12,15 +12,18 @@
 //!   * strip MSYS/MSYS2 env vars that would otherwise poison native
 //!     Windows toolchain binaries.
 //!
-//! Containment is honoured via `ProcessConfig::containment = Some(...)`
-//! when the daemon has installed the global containment group. CLI
-//! binaries and unit tests run uncontained just as before.
+//! On Windows, containment is applied post-spawn by
+//! [`crate::containment::windows_job::assign`] when the daemon has
+//! installed the global containment group; CLI binaries and unit tests
+//! run uncontained just as before. (Earlier code used a `containment:`
+//! field on `ProcessConfig` from a pre-release `running-process-core`;
+//! the published 3.4 API does not expose that field — see #32.)
 
 use std::path::Path;
 use std::time::Duration;
 
 use running_process_core::{
-    CommandSpec, Containment, NativeProcess, ProcessConfig, ProcessError, StderrMode, StdinMode,
+    CommandSpec, NativeProcess, ProcessConfig, ProcessError, StderrMode, StdinMode,
 };
 
 use crate::{FbuildError, Result};
@@ -153,11 +156,6 @@ fn build_config(
         create_process_group: false,
         stdin_mode,
         nice: None,
-        containment: if crate::containment::is_initialised() {
-            Some(Containment::Contained)
-        } else {
-            None
-        },
     })
 }