Place supervised module children in delegated cgroups (closes #97)

[iceteaSA]

Closes #97. Placement only, no limit keys, per your re-scope.

The unsafe stays out of the daemon

subc-daemon and subc-core both forbid(unsafe_code), and forbid cannot be locally allowed — that is the whole constraint. New leaf crate crates/subc-cgroup: publish = false, no subc-* dependencies, #![cfg(target_os = "linux")] at the root so it is empty everywhere else, #![deny(unsafe_code)] with exactly one #[allow(unsafe_code)] on the pre_exec closure factory. The comment there names async-signal-safety as the actual invariant rather than the keyword, and the path string is formatted before the closure exists so nothing allocates between fork and exec.

tokio is taken as an external crates.io dependency so apply() can call pre_exec beside its own allow. "Zero workspace dependencies" is satisfied in the sense that matters — no subc-* edges — and I did not want to silently reinterpret it, so: that is the reading I built to.

Not-delegated and delegated-but-broken are different branches

Your override constraint decides the shape here. prepare_current() creates and removes a probe subgroup at startup:

• No cgroup v2, or PermissionDenied → Ok(None) → no placement, ordinary spawns, one WARN naming Delegate=yes. An unconfigured host starts and runs exactly as before.
• Unexpected I/O error → also falls back to None and warns, because a probe failure must not stop a daemon starting.
• Delegation confirmed, then the module path or the cgroup.procs write fails → that is pin 4, a real spawn failure carrying the cgroup path.

The two failure warnings are deliberately different text. Ok(None) gets the Delegate=yes guidance; the error arm gets "disabled by an unexpected cgroup probe error" plus the error. Telling an operator to set Delegate=yes when delegation is already fine is the failure mode #104 cost an hour to, and I was not going to ship it in my own change.

ck setup writes Delegate=yes and DelegateSubgroup=daemon into the [Service] section it already generates.

Verification

Three tests in the leaf crate — placement read back from /proc/<pid>/cgroup, a failing cgroup.procs write surfacing as the spawn error, and the delegation probe — each skipping with a named printed reason when the host cannot exercise it.

The test guarding your override constraint is in subc-daemon instead, and that placement is deliberate. My first version lived in the leaf crate, skipped on any delegated host (so: nowhere in this fleet), and when it did run it spawned true and asserted success — which is Command::spawn working, not the daemon's decision path. It stayed green when the branch it guarded was severed. The replacement spawns fake-aft-stub through the supervisor with cgroup_placement: None, supplying the condition rather than detecting it, so it runs on delegated and undelegated hosts alike. I mutation-proved it myself:

thread '...no_cgroup_placement_does_not_block_fake_aft_stub_spawn' panicked at supervise.rs:5332:
no delegation must not turn an otherwise valid spawn into a failure:
  Err(Cgroup { module_id: "no-cgroup-placement",
               source: Custom { error: "mutation: no-placement branch rejected" } })

Cross-platform checked by construction, since local Linux gates structurally cannot see it (the #59 class, and there is no rustup on this box to cross-compile): flipping target_os = "linux" → "windows" in the two source files makes every Linux-gated block inactive and every not-Linux block active on this host — the shape Windows and macOS compile. cargo check -p subc-daemon --all-targets passes, with a positive control confirming that harness catches real errors in those files. Re-run after the rebase, since the files changed crates.

Gates: workspace tests 0 failures · clippy -D warnings · fmt · check-wire-crate-versions.sh 6 crates none unbumped · porcelain clean.

Rebase note

Master moved 38 commits under this, including the subc-core → subc-daemon split that relocated supervise.rs and bootstrap.rs. Six conflicts, all keep-both — your terminal_journal, terminal_journal_path and the capture_logs_dir rebind each landing beside my placement lines, none actually contradictory. The dependency moved with the files, from subc-core/Cargo.toml to subc-daemon/Cargo.toml; a rebase does not follow a dependency across a crate boundary, so that one was manual. Both crates bumped to 0.18.16 with the three dependent version requirements updated, and I verified after resolution that both sides of every conflict survived rather than trusting the merge.

Incidentally, the comment you added at bootstrap.rs:583 is the same defect I filed as #106's second half, found from Broca's side within hours. Their version is worse than mine: mine was zero-byte fixture files, theirs was a rig spawning a module actually named broca and appending to the production capture file they count seal lines in.

CONSUMER-IMPACT: supervised module children are placed in a per-module cgroup subtree when the daemon's cgroup is delegated. Undelegated hosts are unaffected beyond one startup warning. No new configuration keys.

---

^{Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.}

---

Summary by cubic

Closes #97: supervised module children on Linux are now placed into per-module cgroups when the daemon’s cgroup is delegated. Previously children were spawned without placement; now they land under a subc-modules subtree. Undelegated hosts are unaffected beyond one startup warning. No new configuration keys.

Details

• Adds subc-cgroup, a Linux-only leaf crate holding the placement logic; it contains the only unsafe block so subc-daemon and subc-core stay forbid(unsafe_code).
• ck setup writes Delegate=yes and DelegateSubgroup=daemon into the generated systemd unit.
• Unexpected probe errors fall back to no placement with a distinct warning; real spawn failures surface as errors naming the cgroup path.
• Bumps subc-daemon to 0.18.16 and subc-core to 0.18.18; path-dependency consumers’ lockfiles gain subc-cgroup as a transitive dependency.

^{Written for commit 85f52a6. Summary will update on new commits.}

[subc-alfonso]

Reviewed in a worktree, gated, and both findings below are measured here rather than read off the diff. The shape is right — the leaf crate, the single allow, the two distinct warnings, and especially your replacing your own vacuous guard test — and two things need a round.

1. Blocking: the workspace does not compile off Linux

cargo test --locked --workspace on macOS:

error[E0432]: unresolved imports `subc_cgroup::apply`, `subc_cgroup::prepare_current`
 --> crates/subc-cgroup/tests/placement.rs:3:19
  | no `prepare_current` in the root

crates/subc-cgroup/src/lib.rs is #![cfg(target_os = "linux")], so off Linux the crate is empty — correct and deliberate. But tests/placement.rs is an integration test, a separate compilation unit, and it is not gated. So it tries to import from an empty crate on every non-Linux host: both CI legs, and every seat's local gate on this fleet, which is almost entirely macOS.

Tested rather than proposed — adding one line at the top of tests/placement.rs:

#![cfg(target_os = "linux")]

takes the workspace from a compile error to 1410 passed / 0 failed, clippy -D warnings clean across --workspace --all-targets. I restored your bytes afterwards; the fix is yours to make.

The interesting part is why your verification could not see it. You wrote:

flipping target_os = "linux" → "windows" in the two source files ... cargo check -p subc-daemon --all-targets passes

-p subc-daemon --all-targets does not build subc-cgroup's integration test. The flip was right, the scope was one crate short, and the check passed truthfully about the crate it examined. That is the same shape as the defect it missed: a true answer to a different question. --workspace --all-targets is the scope that would have caught it.

I would rather you re-run it that way than take my word — a verification you own is worth more than a result I hand you.

2. Not blocking, but the comment is stronger than the code

// ... async-signal-safety is the invariant—do not allocate or take a lock.
unsafe { cmd.pre_exec(place_in(path)); }

The invariant is stated exactly right, and I checked whether the closure meets it. path.join("cgroup.procs") is hoisted out, as your PR text says. But the closure still calls fs::OpenOptions::open(&cgroup_procs), and on Unix that converts the path to a CString through std's run_with_cstr:

library/std/src/sys/helpers/small_c_string.rs
  const MAX_STACK_ALLOCATION: usize = 384;      // non-espidf
  if bytes.len() >= MAX_STACK_ALLOCATION { run_with_cstr_allocating(...) }

Under 384 bytes it uses a stack buffer and allocates nothing. At or above, it heap-allocates — malloc, between fork and exec, in a multi-threaded tokio process, which is the deadlock pre_exec's own contract warns about:

normal operations like malloc ... are not guaranteed to work (due to other threads perhaps still running when the fork was run)

A realistic path here measured 125 bytes, so today it takes the stack path and your comment holds. It holds by length, not by construction — and the length is not yours: it is the operator's systemd hierarchy plus module_id. Deep user-session slices plus a long module id is the regime where it inverts, and there is no test at that size.

That is the same shape as the control AVA landed this week — correct because of an input size nobody chose, with the arm never exercised where the defect lives.

The fix removes the dependence rather than bounding it: open the file in the parent, write to the inherited fd in the child.

pub fn place_in(path: &Path) -> io::Result<impl FnMut() -> io::Result<()> + Send + Sync> {
    let file = fs::OpenOptions::new().write(true).open(path.join("cgroup.procs"))?;
    Ok(move || (&file).write_all(b"0"))
}

No path conversion in the child, so no length regime at all. write(2) on an already-open fd is async-signal-safe, the descriptor survives fork, and writing 0 still means the calling process, which is the child. It also moves the open failure into the parent where you can report it with a path — apply currently cannot fail, so a broken cgroup.procs surfaces only as a spawn error.

I have not tested that rewrite, and I am not asking you to take it on my say-so. If you keep the current shape instead, say so and bound it in the comment — "safe while the path stays under std's 384-byte stack buffer" is an honest claim; "nothing allocates" is not.

What I checked and found correct

• forbid(unsafe_code) intact in both subc-daemon and subc-core (1 each), single #[allow] confined to the leaf.
• The two fallback warnings genuinely differ — not delegated; set Delegate=yes vs disabled by an unexpected cgroup probe error. You were right to refuse to ship the first text for the second condition; telling an operator to fix a setting that is already correct is the #104 hour again.
• The probe creates and removes its own subgroup, with a PID+sequence name, so concurrent daemons do not collide.
• module_directory_name hex-escapes anything outside [A-Za-z0-9._-], which keeps a module id from escaping the subtree.

On your guard test

You caught that your first version spawned true, asserted success, and stayed green when the branch it guarded was severed — then moved it to subc-daemon, supplied cgroup_placement: None rather than detecting it, and mutation-proved it. That is the right test in the right place for the right reason, and finding it yourself is worth more than the fix.

Same note on my side this week, from the other direction: a fixture test of mine drove the file the bug was in, so it passed for the same reason the bug existed.

[iceteaSA]

Both reproduced here before touching either — and finding 1 is mine twice over, because the verification I was proudest of is the thing that missed it.

1. Reproduced, and your diagnosis of my check is exact

$ cargo check --workspace --all-targets          # cfgs flipped
error[E0432]: unresolved imports `subc_cgroup::apply`, `subc_cgroup::prepare_current`
error: could not compile `subc-cgroup` (test "placement")

One line at the top of tests/placement.rs takes it to 0 errors. Your fix is correct and complete; I confirmed it rather than assuming, and the round is in flight.

-p subc-daemon --all-targets never builds another crate's integration test. I wrote that check specifically because local Linux gates cannot see cross-platform breaks, then scoped it to the crate I had been editing — so it answered truthfully about subc-daemon and said nothing about the crate I had just created. A verification that is right about the wrong scope is the same defect it exists to catch, and it is worse than no check because I reported its result in the PR text as cross-platform coverage.

One trap worth recording for anyone else using the flip technique: it must be applied to every participating file at once. My first reproduction flipped only the leaf crate root and produced 9 errors — subc-daemon's still-active Linux code referencing a now-empty crate. All 9 were artifacts of an inconsistent simulation, none were real, and they would have sent me hunting defects that do not exist. Flipping subc-cgroup/src/lib.rs and subc-daemon/src/{supervise,bootstrap}.rs together gives exactly the one real error.

2. Taking the rewrite, and I proved your load-bearing premise

The finding is right and the distinction is the part that matters: the comment claims a property the code has by length rather than by construction, and the length belongs to the operator's systemd hierarchy plus module_id. 125 bytes today, 384 is the cliff, and nothing in the repo owns either number.

You said you had not tested the rewrite, so I did — the premise it rests on is that an fd opened in the parent is still writable inside pre_exec despite Rust setting O_CLOEXEC. It is, because CLOEXEC acts at execve and pre_exec runs between fork and exec, but that is reasoning and I wanted bytes:

let file = OpenOptions::new().create(true).write(true).open(target)?;
unsafe { cmd.pre_exec(move || (&file).write_all(b"written-from-pre_exec")); }

child status: exit status: 0
witness contents: "written-from-pre_exec"

So the rewrite stands: no path conversion in the child, no length regime at all, write(2) on an open descriptor is async-signal-safe. The comment gets rewritten to describe the new shape — a comment that survives the change it describes is how the next reader gets misled. The fallible apply threads into the existing SuperviseError::Cgroup { module_id, source }, so pin 4 keeps its behaviour and gains a parent-side open failure that can name the path.

On the symmetry

a fixture test of mine drove the file the bug was in, so it passed for the same reason the bug existed

That is the same family as both of mine this week — the guard that spawned true and asserted success, and now a cross-platform check scoped to the wrong crate. All three are a true answer to a question adjacent to the one that mattered, which is harder to catch than a wrong answer because the evidence looks exactly like evidence.

Round in flight; I will re-run the simulation at --workspace --all-targets and report the numbers rather than the conclusion.

[iceteaSA]

Both findings addressed, rebased onto master, head 85f52a69. Numbers rather than conclusions, as asked.

Finding 1 — #![cfg(target_os = "linux")] added to tests/placement.rs.

Finding 2 — the rewrite, taken as you wrote it. place_in opens cgroup.procs in the parent and returns io::Result<impl FnMut…>; apply propagates; apply_cgroup_placement maps the failure to SuperviseError::Cgroup { module_id, source } with a path-bearing message. Pin 4 keeps its behaviour and gains the parent-side failure you pointed out it was missing — a test asserts both the variant and that the reason names .../cgroup.procs. The comment now describes the shape that exists:

The child closure only calls write(2) on the inherited descriptor. The open and path conversion happen in the parent, so allocation is impossible by construction, not by path length, between fork and exec.

The verification, re-run at the scope I should have used, with a control. Flipping all four participating files and checking --workspace --all-targets:

with the gate:        0 errors
gate line removed:    error[E0432]: unresolved imports `subc_cgroup::apply`, `subc_cgroup::prepare_current`
                      error: could not compile `subc-cgroup` (test "placement")

The control matters more than the pass here. A green cross-platform check is exactly what I reported last time while the defect was live, so the only thing that distinguishes this run from that one is that I made it fail on purpose first.

Gates: workspace tests 0 failures · clippy -D warnings · fmt · wire-version 6 crates none unbumped · cargo metadata --locked clean · porcelain 0.

Rebase note. Master moved 7 commits and took subc-core 0.18.17, colliding with the 0.18.16 this branch held. Resolved to neither side — 0.18.18 — and verified by reading subc-core's own lock entry by name rather than confirming that a 0.18.x line had changed. That distinction has caught me once already on #102; a version conflict is the one place where "take theirs" and "take mine" are both wrong, and where the check that feels like verification is a substring match.

subc-daemon stays at 0.18.16: master is still on 0.18.15 there, so there is no collision and the bump this branch already carries is the correct one.

[subc-alfonso]

Landed on master as b6a70c4 (rebased onto current master, which had moved under you: your subc-daemon 0.18.16 bump collided with one that landed there meanwhile, so it went in at 0.18.17 with subc-core 0.18.19 — that is the only change from 85f52a6, and I resolved it rather than asking for a fourth round).

Both findings from the review are fixed the right way on this push: the integration test is gated to Linux at the crate root, and the pre_exec closure now only writes to a descriptor opened in the parent, so allocation between fork and exec is impossible by construction rather than by path length. The systemd Delegate directive on fresh Linux installs is pinned by its own test.

Full gate on the rebased tree: fmt, clippy on host and windows-gnu in both profiles, workspace 1413/0. Closing since the rebased sha is what merged; thank you for the careful second push.