hive-priv: filter/trim before counting output-path lines, fix log level

hive-priv/README.md

@@ -30,3 +30,63 @@ 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 non-empty, trimmed 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 whole-buffer `.trim()` would silently hand a
+multi-line string to `--system-path` the day that ever changes, and a
+bare untrimmed/unfiltered `.lines()` turns a lone `"\n"` into a bogus
+empty-string "path" — a wrong line count, or a blank/whitespace one,
+`bail!`s instead).

hive-priv/src/main.rs

@@ -20,7 +20,7 @@
 use std::os::fd::{AsRawFd as _, FromRawFd as _, OwnedFd, RawFd};
 use std::path::{Path, PathBuf};
 
-use anyhow::{Context as _, Result, bail};
+use anyhow::{Context as _, Result, anyhow, bail};
 use hive_priv_sock::{
     AGENT_PREFIX, AGENT_RUNTIME_ROOT, AGENT_STATE_ROOT, AgentTmpfilesEntry, BindMount,
     CredentialMount, InfraAction, InfraContainer, JournalQuery, META_DIR, MIGRATE_STAGING_ROOT,
@@ -623,8 +623,22 @@ async fn exec(
 }
 
 /// Shared body for `CreateContainer` / `UpdateContainer`: validate the
-/// name, build the `nixos-container <verb> … --flake <ref>` argv, and
+/// name, build the toplevel ourselves, and run
-/// run it (streaming line events to `writer` when `stream` is set).
+/// `nixos-container <verb> … --system-path <built>` (streaming line
+/// events to `writer` when `stream` is set).
+///
+/// **Both verbs build explicitly now — not just `update`.** The first cut
+/// of this fix only rewrote `update`, reasoning that `create` was already
+/// safe: it wraps its whole action in an exclusive `flock` before calling
+/// `nixos-container`'s own `buildFlake()`, and once `update` stopped
+/// writing to `buildFlake()`'s shared `.tmp` out-link, concurrent
+/// `create`s were the only remaining writers — mutually excluded by that
+/// lock. True, but it leaves `create`'s safety resting on an internal
+/// implementation detail of a script we don't own (its current locking
+/// behavior, which could change upstream without notice) instead of on
+/// something we control. Building here for both verbs removes `buildFlake()`
+/// from the picture entirely — there's no shared `.tmp` left to race on,
+/// so there's nothing left to reason about staying in sync with.
 async fn container_flake_action(
     verb: &str,
     name: &str,
@@ -632,8 +646,20 @@ async fn container_flake_action(
     writer: &mut OwnedWriteHalf,
 ) -> Result<(String, String)> {
     validate_container_name(name)?;
-    let flake_ref = agent_flake_ref(name);
+    // The build is the multi-minute phase of this operation — give it the
-    let args = [verb, &container_system_name(name), "--flake", &flake_ref];
+    // same live-line treatment `container_run_streaming` gives
+    // `nixos-container` itself when the caller asked for it. Without
+    // this, moving the build out of the streamed `nixos-container` call
+    // (which is the whole point of this fix) would silently regress every
+    // UI that shows build progress: nothing until the longest phase
+    // finishes, then everything at once.
+    let toplevel = nix_build_toplevel(name, stream.then_some(&mut *writer)).await?;
+    let args = [
+        verb,
+        &container_system_name(name),
+        "--system-path",
+        &toplevel,
+    ];
     if stream {
         container_run_streaming(&args, writer).await
     } else {
@@ -641,6 +667,149 @@ async fn container_flake_action(
     }
 }
 
+/// The explicit `nixosConfigurations.<name>.config.system.build.toplevel`
+/// flake attr path — same construction `hive-c0re`'s own
+/// `lifecycle::prebuild_toplevel` uses, kept here as a pure function so
+/// the exact string shape is unit-tested without needing to run `nix`.
+fn toplevel_attr(name: &str) -> String {
+    format!("{META_DIR}#nixosConfigurations.{name}.config.system.build.toplevel")
+}
+
+/// Build `nixosConfigurations.<name>.config.system.build.toplevel` and
+/// 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
+/// `PrivEvent::Line` forwarding.
+async fn nix_build_toplevel(name: &str, mut writer: Option<&mut OwnedWriteHalf>) -> Result<String> {
+    use tokio::io::AsyncBufReadExt as _;
+
+    let attr = toplevel_attr(name);
+    let args = [
+        "--extra-experimental-features",
+        "nix-command flakes",
+        "build",
+        "--no-link",
+        "--print-out-paths",
+        &attr,
+    ];
+    let mut child = Command::new("nix")
+        .args(args)
+        .stdout(std::process::Stdio::piped())
+        .stderr(std::process::Stdio::piped())
+        .spawn()
+        .with_context(|| format!("invoke nix build {attr}"))?;
+
+    let stdout = child.stdout.take().expect("stdout piped");
+    let stderr = child.stderr.take().expect("stderr piped");
+    let mut stdout_lines = BufReader::new(stdout).lines();
+    let mut stderr_lines = BufReader::new(stderr).lines();
+
+    let mut stdout_buf = String::new();
+    let mut stderr_buf = String::new();
+
+    // ⚠️ Both pipes are drained concurrently even though only one is
+    // streamed: reading stderr alone would let stdout fill its pipe
+    // buffer and deadlock the child on a build with enough stdout output
+    // to fill it.
+    //
+    // ⚠️ Each stream's EOF is tracked separately rather than breaking on
+    // the first `None`: `next_line()` on a closed stream returns
+    // `Ok(None)` immediately and forever, so a loop that keeps polling a
+    // finished stream spins hot until the other one ends too.
+    let mut stdout_done = false;
+    let mut stderr_done = false;
+    while !(stdout_done && stderr_done) {
+        tokio::select! {
+            line = stdout_lines.next_line(), if !stdout_done => {
+                match line {
+                    // Captured, never streamed — this is the store path.
+                    Ok(Some(l)) => {
+                        stdout_buf.push_str(&l);
+                        stdout_buf.push('\n');
+                    }
+                    Ok(None) => stdout_done = true,
+                    Err(e) => {
+                        tracing::warn!(error = %e, "nix build stdout read error");
+                        stdout_done = true;
+                    }
+                }
+            }
+            line = stderr_lines.next_line(), if !stderr_done => {
+                match line {
+                    // Streamed as it arrives — the progress the dashboard
+                    // and `journalctl -f` were missing.
+                    Ok(Some(l)) => {
+                        tracing::info!(target: "nix-build-toplevel", "{l}");
+                        if let Some(w) = writer.as_deref_mut() {
+                            write_line_event(w, PrivStream::Stderr, &l).await;
+                        }
+                        if !stderr_buf.is_empty() {
+                            stderr_buf.push('\n');
+                        }
+                        stderr_buf.push_str(&l);
+                    }
+                    Ok(None) => stderr_done = true,
+                    Err(e) => {
+                        tracing::warn!(error = %e, "nix build stderr read error");
+                        stderr_done = true;
+                    }
+                }
+            }
+        }
+    }
+
+    // ⚠️ Success is decided by the exit status, not by "we parsed a
+    // path" — a build can print to stdout and still fail.
+    let status = child
+        .wait()
+        .await
+        .with_context(|| format!("wait nix build {attr}"))?;
+    if !status.success() {
+        bail!(
+            "nix build {attr} failed ({status}): {}",
+            stderr_buf.lines().last().unwrap_or("").trim()
+        );
+    }
+    single_output_path(&stdout_buf)
+        .map(str::to_owned)
+        .map_err(|count| {
+            anyhow!("nix build {attr} produced {count} output path(s), expected exactly 1: {stdout_buf:?}")
+        })
+}
+
+/// Parse `nix build --print-out-paths`' stdout down to the single output
+/// path this function's caller expects. `--print-out-paths` prints one
+/// line *per output*, not one line total (`nix build --no-link
+/// --print-out-paths nixpkgs#openssl` prints two: `…-bin`, `…-man`);
+/// `config.system.build.toplevel` is single-output today, so this is one
+/// line in practice — but a bare whole-buffer `.trim()` would silently
+/// hand a multi-line string on to `--system-path` the day that ever
+/// changes, the same corrupted-argument failure this function exists to
+/// avoid. Trims and drops empty lines *before* counting, so a lone
+/// `"\n"` (or trailing whitespace on the real line) can't be mistaken
+/// for a present-but-blank path — see the unit tests below for the exact
+/// table this closes. `Err` carries the surviving line count, for the
+/// caller's error message.
+fn single_output_path(stdout: &str) -> Result<&str, usize> {
+    let lines: Vec<&str> = stdout
+        .lines()
+        .map(str::trim)
+        .filter(|l| !l.is_empty())
+        .collect();
+    match lines[..] {
+        [path] => Ok(path),
+        _ => Err(lines.len()),
+    }
+}
+
 /// `WriteNspawnFlags` — validate the container + every bind path + every
 /// credential entry, then write the container's nspawn flag overrides.
 fn handle_write_nspawn_flags(
@@ -2392,11 +2561,6 @@ fn validate_name_chars(name: &str) -> Result<()> {
     Ok(())
 }
 
-/// Derive the meta-flake ref for an agent by name.
-fn agent_flake_ref(name: &str) -> String {
-    format!("{META_DIR}#{name}")
-}
-
 /// Validate a bind-mount path: must be absolute, non-empty, and contain
 /// no newlines, null bytes, or double-quotes (which would break the
 /// `EXTRA_NSPAWN_FLAGS="..."` conf line format).
@@ -2677,7 +2841,8 @@ mod tests {
     use super::{
         BindMount, OwnedFd, PAUSED_MARKER_FILE, PrivRequest, check_fd_agreement,
         clear_runner_credentials, contains_secret_shaped_run, git_overlay_flags,
-        limits_dropin_body, redact_secret_line, remove_marker_in, write_state_file_nofollow,
+        limits_dropin_body, redact_secret_line, remove_marker_in, single_output_path,
+        toplevel_attr, write_state_file_nofollow,
     };
     use std::path::PathBuf;
     use std::sync::atomic::{AtomicU32, Ordering};
@@ -2690,6 +2855,44 @@ mod tests {
         }
     }
 
+    /// Pins the exact attr path we hand to `nix build` — it has to match
+    /// `hive-c0re`'s own `lifecycle::prebuild_toplevel` construction, since
+    /// that step's whole point is warming the store for this later build.
+    /// A drifted attr path defeats the cache-warming silently — no error,
+    /// just a slower `update`.
+    #[test]
+    fn toplevel_attr_matches_prebuild_toplevels_construction() {
+        assert_eq!(
+            toplevel_attr("atlas"),
+            "/var/lib/hyperhive/meta#nixosConfigurations.atlas.config.system.build.toplevel"
+        );
+    }
+
+    /// `--print-out-paths`' exact per-shape table (measured against real
+    /// `rustc` semantics, not just reasoned about) — a lone `"\n"` and a
+    /// trailing-space path are the two shapes a bare `.lines().collect()`
+    /// gets wrong, both catchable only by trimming + dropping empties
+    /// before counting rather than after.
+    #[test]
+    fn single_output_path_rejects_blank_and_trims_whitespace() {
+        assert_eq!(single_output_path(""), Err(0));
+        assert_eq!(single_output_path("\n"), Err(0));
+        assert_eq!(single_output_path("\n\n"), Err(0));
+        assert_eq!(single_output_path("/nix/store/abc\n"), Ok("/nix/store/abc"));
+        assert_eq!(
+            single_output_path("/nix/store/abc \n"),
+            Ok("/nix/store/abc")
+        );
+        assert_eq!(
+            single_output_path("/nix/store/abc\r\n"),
+            Ok("/nix/store/abc")
+        );
+        assert_eq!(
+            single_output_path("/nix/store/abc\n/nix/store/def\n"),
+            Err(2)
+        );
+    }
+
     /// Every bound git repo gets its `.git` overlaid — the knowledge tree
     /// and *each* config mount, an agent's own plus every child's.
     ///