hive-priv: stream the toplevel build live, and build it for create too

hive-priv/src/main.rs

@@ -623,19 +623,22 @@ async fn exec(
 }
 
 /// Shared body for `CreateContainer` / `UpdateContainer`: validate the
-/// name, build the `nixos-container <verb> …` argv, and run it (streaming
+/// name, build the toplevel ourselves, and run
-/// line events to `writer` when `stream` is set).
+/// `nixos-container <verb> … --system-path <built>` (streaming line
+/// events to `writer` when `stream` is set).
 ///
-/// `update` builds the toplevel itself first and passes `--system-path`
+/// **Both verbs build explicitly now — not just `update`.** The first cut
-/// instead of `--flake` — see [`nix_build_toplevel`] for why. `create`
+/// of this fix only rewrote `update`, reasoning that `create` was already
-/// keeps the plain `--flake` form: `nixos-container` takes an exclusive
+/// safe: it wraps its whole action in an exclusive `flock` before calling
-/// `flock` for the whole `create` action, and once `update` no longer
+/// `nixos-container`'s own `buildFlake()`, and once `update` stopped
-/// calls `buildFlake()` (below), concurrent `create`s are the only
+/// writing to `buildFlake()`'s shared `.tmp` out-link, concurrent
-/// remaining writers of the shared `.tmp` out-link — so they're mutually
+/// `create`s were the only remaining writers — mutually excluded by that
-/// excluded. ⚠️ That holds only while nothing else calls `buildFlake()`
+/// lock. True, but it leaves `create`'s safety resting on an internal
-/// unlocked: pre-this-PR, `create`'s own lock bought it nothing against a
+/// implementation detail of a script we don't own (its current locking
-/// concurrent *unlocked* `update` clobbering the same `.tmp` — the lock
+/// behavior, which could change upstream without notice) instead of on
-/// alone was never what made `create` safe, this fix is.
+/// 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,
@@ -643,22 +646,20 @@ async fn container_flake_action(
     writer: &mut OwnedWriteHalf,
 ) -> Result<(String, String)> {
     validate_container_name(name)?;
-    if verb == "update" {
+    // The build is the multi-minute phase of this operation — give it the
-        let toplevel = nix_build_toplevel(name).await?;
+    // same live-line treatment `container_run_streaming` gives
-        let args = [
+    // `nixos-container` itself when the caller asked for it. Without
-            verb,
+    // this, moving the build out of the streamed `nixos-container` call
-            &container_system_name(name),
+    // (which is the whole point of this fix) would silently regress every
-            "--system-path",
+    // UI that shows build progress: nothing until the longest phase
-            &toplevel,
+    // finishes, then everything at once.
-        ];
+    let toplevel = nix_build_toplevel(name, stream.then_some(&mut *writer)).await?;
-        return if stream {
+    let args = [
-            container_run_streaming(&args, writer).await
+        verb,
-        } else {
+        &container_system_name(name),
-            container_run(&args).await
+        "--system-path",
-        };
+        &toplevel,
-    }
+    ];
-    let flake_ref = agent_flake_ref(name);
-    let args = [verb, &container_system_name(name), "--flake", &flake_ref];
     if stream {
         container_run_streaming(&args, writer).await
     } else {
@@ -677,31 +678,57 @@ fn toplevel_attr(name: &str) -> String {
 /// Build `nixosConfigurations.<name>.config.system.build.toplevel` and
 /// return the resulting store path.
 ///
-/// **Why hive-priv builds this itself instead of letting `nixos-container
+/// **Why hive-priv builds this itself instead of letting `nixos-container`
-/// update` do it**: `nixos-container`'s own `buildFlake()` (invoked
+/// do it**: `nixos-container`'s own `buildFlake()` (invoked whenever
-/// whenever `--system-path` isn't passed) builds to a *hardcoded relative
+/// `--system-path` isn't passed) builds to a *hardcoded relative path* —
-/// path* — `$systemPath` is only ever assigned from the CLI flag or from
+/// `$systemPath` is only ever assigned from the CLI flag or from its own
-/// its own build result, so with no flag it stays `undef` and
+/// build result, so with no flag it stays `undef` and `"$systemPath.tmp"`
-/// `"$systemPath.tmp"` interpolates to the bare string `.tmp` in
+/// interpolates to the bare string `.tmp` in whatever the caller's cwd
-/// whatever the caller's cwd is. `buildFlake()` itself takes no lock at
+/// is. `buildFlake()` itself takes no lock at all: `create` wraps its
-/// all — `create` wraps its *whole action* in an exclusive `flock` before
+/// *whole action* in an exclusive `flock` before calling it, but `update`
-/// calling `buildFlake()`, but `update` calls it with no lock, so that
+/// used to call it with no lock at all, so that `create`-side lock never
-/// `create`-side lock never protected against a concurrent `update`
+/// protected against a concurrent `update` clobbering the same `.tmp`.
-/// clobbering the same `.tmp`. hive-priv never sets a per-call cwd, so two
+/// hive-priv never sets a per-call cwd, so with
-/// concurrent `update`s (this hive runs
+/// `services.hyperhive.c0re.buildSlots` > 1, two concurrent calls (any mix
-/// `services.hyperhive.c0re.buildSlots` > 1) share that one `.tmp`: one's
+/// of `create`/`update`) could share that one `.tmp`: one's
-/// `readlink(".tmp")` can resolve to the *other's* build output, handing
+/// `readlink(".tmp")` resolving to the *other's* build output, handing an
-/// an agent's container the wrong agent's closure — the "agent container
+/// 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` means `buildFlake()` — and its shared `.tmp` — never
+/// `--system-path` for *every* call means `buildFlake()` never runs at
-/// runs at all. `--no-link` avoids a competing out-link race of our own;
+/// all, for either verb — no shared `.tmp` left to race on, no locking
-/// we only need the store path, not a GC root (the store path is safe
+/// invariant of a script we don't own to keep track of. `--no-link`
-/// from collection for as long as it takes `nixos-container update` to
+/// avoids a competing out-link race of our own; we only need the store
-/// register it against the container's own profile, same window every
+/// path, not a GC root (the store path is safe from collection for as
-/// other consumer of a `--print-out-paths` result already relies on).
+/// long as it takes `nixos-container` to register it against the
-async fn nix_build_toplevel(name: &str) -> Result<String> {
+/// 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
+/// `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",
@@ -711,24 +738,85 @@ async fn nix_build_toplevel(name: &str) -> Result<String> {
         "--print-out-paths",
         &attr,
     ];
-    let out = Command::new("nix")
+    let mut child = Command::new("nix")
         .args(args)
-        .output()
+        .stdout(std::process::Stdio::piped())
-        .await
+        .stderr(std::process::Stdio::piped())
+        .spawn()
         .with_context(|| format!("invoke nix build {attr}"))?;
-    let stdout = String::from_utf8_lossy(&out.stdout).into_owned();
+
-    let stderr = String::from_utf8_lossy(&out.stderr).into_owned();
+    let stdout = child.stdout.take().expect("stdout piped");
-    for line in stderr.lines() {
+    let stderr = child.stderr.take().expect("stderr piped");
-        tracing::warn!(target: "nix-build-toplevel", "{line}");
+    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::warn!(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;
+                    }
+                }
+            }
+        }
     }
-    if !out.status.success() {
+
+    // ⚠️ 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 ({}): {}",
+            "nix build {attr} failed ({status}): {}",
-            out.status,
+            stderr_buf.lines().last().unwrap_or("").trim()
-            stderr.trim()
         );
     }
-    let path = stdout.trim();
+    let path = stdout_buf.trim();
     if path.is_empty() {
         bail!("nix build {attr} produced no output path");
     }
@@ -2486,11 +2574,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).