hive-priv: move the toplevel-build doc comment's long prose into the README

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.<name>.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).

hive-priv/src/main.rs

@@ -676,52 +676,14 @@ fn toplevel_attr(name: &str) -> String {
 }
 
 /// Build `nixosConfigurations.<name>.config.system.build.toplevel` and
-/// return the resulting store path.
+/// return the resulting store path, so `create`/`update` can hand
-///
+/// `nixos-container` an explicit `--system-path` instead of letting its
-/// **Why hive-priv builds this itself instead of letting `nixos-container`
+/// own `buildFlake()` build to a racy shared out-link. See "Container
-/// do it**: `nixos-container`'s own `buildFlake()` (invoked whenever
+/// toplevel builds" in this crate's README for the full story — the
-/// `--system-path` isn't passed) builds to a *hardcoded relative path* —
+/// concurrency bug this closes (the "agent container gets closure of
-/// `$systemPath` is only ever assigned from the CLI flag or from its own
+/// other agent" mystery bug) and why stdout/stderr are drained
-/// build result, so with no flag it stays `undef` and `"$systemPath.tmp"`
+/// concurrently but handled asymmetrically (stderr streamed live,
-/// interpolates to the bare string `.tmp` in whatever the caller's cwd
+/// stdout captured and required to be exactly one line).
-/// 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.
 ///
 /// `writer` is `None` for the non-streaming call shape (`stream: false`);
 /// stderr still logs to journald either way, just without the