From 40db6c898776c5ef5c2a92653fb6678c06d4811a Mon Sep 17 00:00:00 2001 From: damocles Date: Fri, 28 Aug 2026 18:24:21 +0200 Subject: [PATCH] hive-priv: move the toplevel-build doc comment's long prose into the README --- hive-priv/README.md | 57 +++++++++++++++++++++++++++++++++++++++++++ hive-priv/src/main.rs | 54 ++++++---------------------------------- 2 files changed, 65 insertions(+), 46 deletions(-) diff --git a/hive-priv/README.md b/hive-priv/README.md index 68da1905..21e39350 100644 --- a/hive-priv/README.md +++ b/hive-priv/README.md @@ -30,3 +30,60 @@ See `docs/boundary.md` and `docs/security.md` for the privilege boundary. The wire contract (`PrivRequest` / response types) lives in the separate `hive-priv-sock` crate so this root binary depends on just the protocol shapes, not the whole daemon-shared crate. + +## Implementation notes + +### Container toplevel builds (create/update) + +`container_flake_action` (in `src/main.rs`) builds +`nixosConfigurations..config.system.build.toplevel` itself +(`nix_build_toplevel`) and passes the resolved store path to +`nixos-container create`/`update` via `--system-path`, for both verbs. + +**Why not let `nixos-container` build it (the old `create`-only +behaviour, `update` used its own `--flake` path)**: `nixos-container`'s +own `buildFlake()` — invoked whenever `--system-path` isn't passed — +builds to a *hardcoded relative path*. `$systemPath` is only ever +assigned from the CLI flag or from `buildFlake()`'s own result, so with +no flag it stays `undef` and `"$systemPath.tmp"` interpolates to the +bare string `.tmp` in whatever the caller's cwd happens to be. +`buildFlake()` itself takes no lock at all: `create` wraps its *whole +action* in an exclusive `flock` before calling it, but `update` used to +call it with no lock whatsoever — so `create`'s lock never protected +against a concurrent `update` clobbering the same `.tmp`. hive-priv never +sets a per-call cwd, so with `services.hyperhive.c0re.buildSlots > 1`, +two concurrent calls (any mix of `create`/`update`) could share that one +`.tmp`: one's `readlink(".tmp")` resolving to the *other's* build +output, handing an agent's container the wrong agent's closure — the +"agent container gets closure of other agent" bug (hyperhive#3312). + +Building the toplevel here and passing the resolved store path via +`--system-path` for *every* call means `buildFlake()` never runs at all, +for either verb — no shared `.tmp` left to race on, no locking invariant +of a script we don't own to keep track of. `--no-link` avoids a +competing out-link race of our own; we only need the store path, not a +GC root (it's safe from collection for as long as it takes +`nixos-container` to register it against the container's own profile, +same window every other `--print-out-paths` consumer already relies on). + +**Forwards stderr live, captures stdout silently — deliberately not +symmetric.** This build is the multi-minute phase of a `create`/ +`update`, and it used to run *inside* `nixos-container`'s own `--flake` +invocation, which streams every line to the caller in real time. +Buffering it instead (`Command::output()`, as this function first +shipped) regressed that: nothing on the wire — dashboard or +`journalctl -f` alike — until the whole build finishes, then everything +at once. So both pipes are drained concurrently (needed to avoid +deadlocking if either pipe fills while the other is being read), but +only stderr — where nix's own progress goes — is forwarded live, same +shape as `container_run_streaming`. stdout is different: +`--print-out-paths` writes *only* the final store path there, once, at +the end — forwarding it the same way would risk interleaving a progress +line into the value this function hands back as `--system-path`, trading +a closure-mixup bug for a corrupted-argument one. So stdout lines are +accumulated silently and only consulted after the exit status is known +to be success — and even then, exactly one line is required (`nix build +--print-out-paths` prints one line *per output*, not one line total; +`config.system.build.toplevel` is single-output today, but a bare +`.trim()` would silently hand a multi-line string to `--system-path` the +day that ever changes, so a wrong line count `bail!`s instead). diff --git a/hive-priv/src/main.rs b/hive-priv/src/main.rs index 4f1dc185..98a28a1c 100644 --- a/hive-priv/src/main.rs +++ b/hive-priv/src/main.rs @@ -676,52 +676,14 @@ fn toplevel_attr(name: &str) -> String { } /// Build `nixosConfigurations..config.system.build.toplevel` and -/// return the resulting store path. -/// -/// **Why hive-priv builds this itself instead of letting `nixos-container` -/// do it**: `nixos-container`'s own `buildFlake()` (invoked whenever -/// `--system-path` isn't passed) builds to a *hardcoded relative path* — -/// `$systemPath` is only ever assigned from the CLI flag or from its own -/// build result, so with no flag it stays `undef` and `"$systemPath.tmp"` -/// interpolates to the bare string `.tmp` in whatever the caller's cwd -/// is. `buildFlake()` itself takes no lock at all: `create` wraps its -/// *whole action* in an exclusive `flock` before calling it, but `update` -/// used to call it with no lock at all, so that `create`-side lock never -/// protected against a concurrent `update` clobbering the same `.tmp`. -/// hive-priv never sets a per-call cwd, so with -/// `services.hyperhive.c0re.buildSlots` > 1, two concurrent calls (any mix -/// of `create`/`update`) could share that one `.tmp`: one's -/// `readlink(".tmp")` resolving to the *other's* build output, handing an -/// agent's container the wrong agent's closure — the "agent container -/// gets closure of other agent" mystery bug. -/// -/// Building the toplevel here and passing the resolved store path via -/// `--system-path` for *every* call means `buildFlake()` never runs at -/// all, for either verb — no shared `.tmp` left to race on, no locking -/// invariant of a script we don't own to keep track of. `--no-link` -/// avoids a competing out-link race of our own; we only need the store -/// path, not a GC root (the store path is safe from collection for as -/// long as it takes `nixos-container` to register it against the -/// container's own profile, same window every other consumer of a -/// `--print-out-paths` result already relies on). -/// -/// **Forwards stderr live, captures stdout silently — deliberately not -/// symmetric.** This build is the multi-minute phase of a `create`/ -/// `update`, and it used to run *inside* `nixos-container`'s own -/// `--flake` invocation, which streams every line to `writer` in real -/// time. Buffering it here (`Command::output()`, as this function first -/// shipped) regressed that: nothing on the wire — dashboard or -/// `journalctl -f` alike — until the whole build finishes, then -/// everything at once. So both pipes are drained concurrently (see the -/// in-body comments for why *both*, and why *concurrently*), but only -/// stderr — where nix's own progress goes — is forwarded to `writer` and -/// journald as it arrives, matching [`container_run_streaming`]'s shape. -/// stdout is different: `--print-out-paths` writes *only* the final store -/// path there, once, at the end — forwarding it the same way would risk -/// interleaving a progress line into the value this function hands back -/// as `--system-path`, trading a closure-mixup bug for a corrupted- -/// argument one. So stdout lines are accumulated silently and only -/// consulted after the exit status is known to be success. +/// return the resulting store path, so `create`/`update` can hand +/// `nixos-container` an explicit `--system-path` instead of letting its +/// own `buildFlake()` build to a racy shared out-link. See "Container +/// toplevel builds" in this crate's README for the full story — the +/// concurrency bug this closes (the "agent container gets closure of +/// other agent" mystery bug) and why stdout/stderr are drained +/// concurrently but handled asymmetrically (stderr streamed live, +/// stdout captured and required to be exactly one line). /// /// `writer` is `None` for the non-streaming call shape (`stream: false`); /// stderr still logs to journald either way, just without the