refactor(fbuild-core): migrate to running-process-core 3.4 (crates.io)#254
Conversation
Drop the git-pinned ff9d7972 rev of `running-process-core` (an abandoned feature branch) and adopt the published 3.4 release from crates.io. See #32. The pre-publication API exposed: - a `Containment` enum (`Contained` / `Detached`), - a `ProcessConfig::containment` field, and - `ContainedProcessGroup::spawn_with_containment(command, mode)` returning `ContainedChild { child, .. }`. The published 3.4 API replaced these with a `ContainedProcessGroup::spawn` method returning a custom `SpawnedChild` (and `spawn_daemon` for detached spawns). `SpawnedChild` is not convertible to `std::process::Child`, so switching wholesale would break fbuild's `spawn_contained` / `spawn_detached` return type and ripple into every caller. Changes: - `subprocess.rs`: drop the `Containment` import and the `containment:` field from `ProcessConfig` literal. The published `ProcessConfig` no longer has that field; Windows containment is now applied post-spawn by `containment::windows_job::assign` instead. - `containment.rs`: replace the Windows `spawn_with_containment(_, Contained)` call with `command.spawn()` followed by `windows_job::assign(child.as_raw_handle())` — the same `JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE` behaviour, reusing the existing `windows_job::assign` helper already used by the tokio spawn path. - Windows `spawn_detached` drops the Job Object assignment entirely (that's what "detached" means) but still injects the originator env var so cross-process correlation continues to work. - Add `inject_originator_env` helper that mirrors `ContainedProcessGroup::inject_originator_env` so children of the std-path spawns still inherit `RUNNING_PROCESS_ORIGINATOR=TOOL:PID`. Tests: all 534+ workspace tests pass, including the issue #129 EPERM regression test that verifies two sequential `spawn_contained` calls both succeed after the first child exits. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Your plan currently allows 1 review/hour. Refill in 54 minutes and 2 seconds. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more review capacity refills, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than trial, open-source, and free plans. In all cases, review capacity refills continuously over time. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PR migrates ChangesProcess Containment Refactor
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/fbuild-core/src/containment.rs (1)
253-263:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMissing originator env injection in
tokio_spawn::spawn_contained.The
std::processpath (spawn_containedat line 117) injectsRUNNING_PROCESS_ORIGINATORviainject_originator_env, but the tokio path does not. Tokio-spawned children (e.g., emulator processes pershared.rs:192) will lack the originator correlation env var, breaking cross-process tracing after crashes.Add env injection before spawning:
Proposed fix
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()?; post_spawn(&child)?; Ok(child) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/fbuild-core/src/containment.rs` around lines 253 - 263, tokio_spawn's spawn_contained does not inject RUNNING_PROCESS_ORIGINATOR like the std::process path, so call inject_originator_env(command) inside spawn_contained (e.g., after configure(command) and before let child = command.spawn()?) and propagate its Result with ? so any error bubbles up; keep post_spawn(&child)? as-is to match existing flow and ensure originator env is set for tokio-spawned children (references: function spawn_contained, configure, inject_originator_env, post_spawn).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/fbuild-core/src/containment.rs`:
- Around line 120-127: If windows_job::assign(child.as_raw_handle()) fails after
command.spawn() succeeds, the child is orphaned; update the spawn-and-assign
sequence in the containment logic so that on assign error you terminate the
spawned child (e.g., call child.kill() and wait/cleanup) before returning the
error. Specifically, in the block that uses GLOBAL_GROUP.get(), after
inject_originator_env(command, group) and let child = command.spawn()?, attempt
windows_job::assign(child.as_raw_handle()) and if that returns Err, ensure you
first stop the child process (handle kill()/wait or equivalent on the Child),
ignore any secondary errors appropriately, then propagate the original assign
error to the caller. Ensure no handle leak remains and that errors from
kill/wait are logged/handled but do not mask the job-assignment error.
---
Outside diff comments:
In `@crates/fbuild-core/src/containment.rs`:
- Around line 253-263: tokio_spawn's spawn_contained does not inject
RUNNING_PROCESS_ORIGINATOR like the std::process path, so call
inject_originator_env(command) inside spawn_contained (e.g., after
configure(command) and before let child = command.spawn()?) and propagate its
Result with ? so any error bubbles up; keep post_spawn(&child)? as-is to match
existing flow and ensure originator env is set for tokio-spawned children
(references: function spawn_contained, configure, inject_originator_env,
post_spawn).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 27b01cc6-35d1-4e04-8415-fc20d49320f0
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
Cargo.tomlcrates/fbuild-core/src/containment.rscrates/fbuild-core/src/subprocess.rs
Two issues flagged by CR: 1. **Orphan on Job Object assignment failure** (Major): the upstream `spawn_with_containment` was atomic — if assignment failed the child was never visible. My split spawn+assign in `spawn_contained` (Windows) leaked the child on assign failure. Now kill+wait the child before returning the error, preserving the containment guarantee. 2. **Missing originator env in tokio path** (Major): the std-path `spawn_contained` injects `RUNNING_PROCESS_ORIGINATOR` via the new `inject_originator_env` helper, but `tokio_spawn::spawn_contained` didn't, so tokio-spawned children (emulator processes via `shared.rs`) lacked the correlation env var. Mirror the std-path injection in the tokio variant. Tests: containment regression suite still passes (including the issue #129 EPERM test). `cargo clippy --workspace -- -D warnings` clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@coderabbitai both addressed in 2c93e37:
Containment regression tests still pass; |
|
Tip For best results, initiate chat on the files or code changes.
Great work keeping clippy clean and the containment regression suite green. 🎉 (∩^ω^)⊃━☆ﾟ.*･｡ﾟ If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
1 participant
Summary
Closes the loop on the running-process-core upgrade deferred from #253. Drops the git-pinned
ff9d7972rev (an abandoned feature branch with a pre-publication API) and adopts the published 3.4 release from crates.io. See #32.Why
The pre-publication API exposed
Containmentenum,ProcessConfig::containmentfield, andContainedProcessGroup::spawn_with_containment(...). The published 3.4 API replaced these withContainedProcessGroup::spawn(...)returning a customSpawnedChild(not convertible tostd::process::Child), so a clean swap isn't possible without changing fbuild'sspawn_contained/spawn_detachedreturn type and rippling into every caller.What changes
subprocess.rs— drop theContainmentimport and thecontainment:field from theProcessConfigliteral (the published struct no longer has it).containment.rsWindows path — replacegroup.spawn_with_containment(command, Containment::Contained)?withcommand.spawn()?followed bywindows_job::assign(child.as_raw_handle())?. SameJOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSEsemantics, reusing the existingwindows_job::assignhelper already used by the tokio spawn path.containment.rsWindowsspawn_detached— drops Job Object assignment entirely (that's what "detached" means) but still injects the originator env var.inject_originator_envmirrorsContainedProcessGroup::inject_originator_envso children still inheritRUNNING_PROCESS_ORIGINATOR=TOOL:PIDfor cross-process correlation.pre_exechook (per-child pgid +PR_SET_PDEATHSIG) and never touched the upstream API.Test plan
uv run soldr cargo check --workspace --all-targets— cleanuv run soldr cargo clippy --workspace --all-targets -- -D warnings— cleanuv run soldr cargo test --workspace— 534+ tests pass, including:containment::tests::sequential_contained_spawns_do_not_fail_with_eperm(the issue fbuild-2.1.18 broken: wheel script missing exec bit + 'Operation not permitted' on every build #129 EPERM regression test)subprocess::tests::run_echo+run_captures_stderr(subprocess.rs change)🤖 Generated with Claude Code
Summary by CodeRabbit