From bfd8a61d19c73716662ee24b483f70e118367793 Mon Sep 17 00:00:00 2001 From: damocles Date: Fri, 28 Aug 2026 17:35:26 +0200 Subject: [PATCH 1/6] hive-priv: build the container toplevel ourselves instead of nixos-container's buildFlake --- hive-priv/src/main.rs | 106 ++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 103 insertions(+), 3 deletions(-) diff --git a/hive-priv/src/main.rs b/hive-priv/src/main.rs index 53a4bceb..a6ac9bb7 100644 --- a/hive-priv/src/main.rs +++ b/hive-priv/src/main.rs @@ -623,8 +623,14 @@ async fn exec( } /// Shared body for `CreateContainer` / `UpdateContainer`: validate the -/// name, build the `nixos-container … --flake ` argv, and -/// run it (streaming line events to `writer` when `stream` is set). +/// name, build the `nixos-container …` argv, and run it (streaming +/// line events to `writer` when `stream` is set). +/// +/// `update` builds the toplevel itself first and passes `--system-path` +/// instead of `--flake` — see [`nix_build_toplevel`] for why. `create` +/// keeps the plain `--flake` form: `nixos-container`'s own `buildFlake()` +/// takes an internal `flock` for `create` (not for `update`), so the +/// hazard this works around doesn't apply there. async fn container_flake_action( verb: &str, name: &str, @@ -632,6 +638,20 @@ async fn container_flake_action( writer: &mut OwnedWriteHalf, ) -> Result<(String, String)> { validate_container_name(name)?; + if verb == "update" { + let toplevel = nix_build_toplevel(name).await?; + let args = [ + verb, + &container_system_name(name), + "--system-path", + &toplevel, + ]; + return if stream { + container_run_streaming(&args, writer).await + } else { + container_run(&args).await + }; + } let flake_ref = agent_flake_ref(name); let args = [verb, &container_system_name(name), "--flake", &flake_ref]; if stream { @@ -641,6 +661,72 @@ async fn container_flake_action( } } +/// The explicit `nixosConfigurations..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..config.system.build.toplevel` and +/// return the resulting store path. +/// +/// **Why hive-priv builds this itself instead of letting `nixos-container +/// update` 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. Worse, the `create` action's `flock` +/// around this isn't taken for `update` at all. hive-priv never sets a +/// per-call cwd, so two concurrent `update`s (this hive runs +/// `services.hyperhive.c0re.buildSlots` > 1) share that one `.tmp`: one's +/// `readlink(".tmp")` can resolve 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` means `buildFlake()` — and its shared `.tmp` — never +/// runs at all. `--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 update` to +/// register it against the container's own profile, same window every +/// other consumer of a `--print-out-paths` result already relies on). +async fn nix_build_toplevel(name: &str) -> Result { + let attr = toplevel_attr(name); + let args = [ + "--extra-experimental-features", + "nix-command flakes", + "build", + "--no-link", + "--print-out-paths", + &attr, + ]; + let out = Command::new("nix") + .args(args) + .output() + .await + .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(); + for line in stderr.lines() { + tracing::warn!(target: "nix-build-toplevel", "{line}"); + } + if !out.status.success() { + bail!( + "nix build {attr} failed ({}): {}", + out.status, + stderr.trim() + ); + } + let path = stdout.trim(); + if path.is_empty() { + bail!("nix build {attr} produced no output path"); + } + Ok(path.to_owned()) +} + /// `WriteNspawnFlags` — validate the container + every bind path + every /// credential entry, then write the container's nspawn flag overrides. fn handle_write_nspawn_flags( @@ -2677,7 +2763,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, toplevel_attr, + write_state_file_nofollow, }; use std::path::PathBuf; use std::sync::atomic::{AtomicU32, Ordering}; @@ -2690,6 +2777,19 @@ 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" + ); + } + /// Every bound git repo gets its `.git` overlaid — the knowledge tree /// and *each* config mount, an agent's own plus every child's. /// From c24ae9d7141157ed4cfb5c662288f1062e56c0ef Mon Sep 17 00:00:00 2001 From: damocles Date: Fri, 28 Aug 2026 17:39:13 +0200 Subject: [PATCH 2/6] hive-priv: fix doc comment attribution of nixos-container's flock --- hive-priv/src/main.rs | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/hive-priv/src/main.rs b/hive-priv/src/main.rs index a6ac9bb7..96430c29 100644 --- a/hive-priv/src/main.rs +++ b/hive-priv/src/main.rs @@ -628,9 +628,14 @@ async fn exec( /// /// `update` builds the toplevel itself first and passes `--system-path` /// instead of `--flake` — see [`nix_build_toplevel`] for why. `create` -/// keeps the plain `--flake` form: `nixos-container`'s own `buildFlake()` -/// takes an internal `flock` for `create` (not for `update`), so the -/// hazard this works around doesn't apply there. +/// keeps the plain `--flake` form: `nixos-container` takes an exclusive +/// `flock` for the whole `create` action, and once `update` no longer +/// calls `buildFlake()` (below), concurrent `create`s are the only +/// remaining writers of the shared `.tmp` out-link — so they're mutually +/// excluded. ⚠️ That holds only while nothing else calls `buildFlake()` +/// unlocked: pre-this-PR, `create`'s own lock bought it nothing against a +/// concurrent *unlocked* `update` clobbering the same `.tmp` — the lock +/// alone was never what made `create` safe, this fix is. async fn container_flake_action( verb: &str, name: &str, @@ -678,9 +683,12 @@ fn toplevel_attr(name: &str) -> String { /// 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. Worse, the `create` action's `flock` -/// around this isn't taken for `update` at all. hive-priv never sets a -/// per-call cwd, so two concurrent `update`s (this hive runs +/// whatever the caller's cwd is. `buildFlake()` itself takes no lock at +/// all — `create` wraps its *whole action* in an exclusive `flock` before +/// calling `buildFlake()`, but `update` calls it with no lock, so that +/// `create`-side lock never protected against a concurrent `update` +/// clobbering the same `.tmp`. hive-priv never sets a per-call cwd, so two +/// concurrent `update`s (this hive runs /// `services.hyperhive.c0re.buildSlots` > 1) share that one `.tmp`: one's /// `readlink(".tmp")` can resolve to the *other's* build output, handing /// an agent's container the wrong agent's closure — the "agent container From c3e3753dd056446a8cfcdf728bc1255cbf3926f5 Mon Sep 17 00:00:00 2001 From: damocles Date: Fri, 28 Aug 2026 17:51:32 +0200 Subject: [PATCH 3/6] hive-priv: stream the toplevel build live, and build it for create too --- hive-priv/src/main.rs | 217 +++++++++++++++++++++++++++++------------- 1 file changed, 150 insertions(+), 67 deletions(-) diff --git a/hive-priv/src/main.rs b/hive-priv/src/main.rs index 96430c29..0c5b9315 100644 --- a/hive-priv/src/main.rs +++ b/hive-priv/src/main.rs @@ -623,19 +623,22 @@ async fn exec( } /// Shared body for `CreateContainer` / `UpdateContainer`: validate the -/// name, build the `nixos-container …` argv, and run it (streaming -/// line events to `writer` when `stream` is set). +/// name, build the toplevel ourselves, and run +/// `nixos-container … --system-path ` (streaming line +/// events to `writer` when `stream` is set). /// -/// `update` builds the toplevel itself first and passes `--system-path` -/// instead of `--flake` — see [`nix_build_toplevel`] for why. `create` -/// keeps the plain `--flake` form: `nixos-container` takes an exclusive -/// `flock` for the whole `create` action, and once `update` no longer -/// calls `buildFlake()` (below), concurrent `create`s are the only -/// remaining writers of the shared `.tmp` out-link — so they're mutually -/// excluded. ⚠️ That holds only while nothing else calls `buildFlake()` -/// unlocked: pre-this-PR, `create`'s own lock bought it nothing against a -/// concurrent *unlocked* `update` clobbering the same `.tmp` — the lock -/// alone was never what made `create` safe, this fix is. +/// **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, @@ -643,22 +646,20 @@ async fn container_flake_action( writer: &mut OwnedWriteHalf, ) -> Result<(String, String)> { validate_container_name(name)?; - if verb == "update" { - let toplevel = nix_build_toplevel(name).await?; - let args = [ - verb, - &container_system_name(name), - "--system-path", - &toplevel, - ]; - return if stream { - container_run_streaming(&args, writer).await - } else { - container_run(&args).await - }; - } - let flake_ref = agent_flake_ref(name); - let args = [verb, &container_system_name(name), "--flake", &flake_ref]; + // The build is the multi-minute phase of this operation — give it the + // 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 { @@ -677,31 +678,57 @@ 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 -/// update` 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 `buildFlake()`, but `update` calls it with no lock, so that -/// `create`-side lock never protected against a concurrent `update` -/// clobbering the same `.tmp`. hive-priv never sets a per-call cwd, so two -/// concurrent `update`s (this hive runs -/// `services.hyperhive.c0re.buildSlots` > 1) share that one `.tmp`: one's -/// `readlink(".tmp")` can resolve to the *other's* build output, handing -/// an agent's container the wrong agent's closure — the "agent container +/// **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` means `buildFlake()` — and its shared `.tmp` — never -/// runs at all. `--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 update` to -/// register it against the container's own profile, same window every -/// other consumer of a `--print-out-paths` result already relies on). -async fn nix_build_toplevel(name: &str) -> Result { +/// `--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 +/// `PrivEvent::Line` forwarding. +async fn nix_build_toplevel(name: &str, mut writer: Option<&mut OwnedWriteHalf>) -> Result { + 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 { "--print-out-paths", &attr, ]; - let out = Command::new("nix") + let mut child = Command::new("nix") .args(args) - .output() - .await + .stdout(std::process::Stdio::piped()) + .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(); - for line in stderr.lines() { - tracing::warn!(target: "nix-build-toplevel", "{line}"); + + 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::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 ({}): {}", - out.status, - stderr.trim() + "nix build {attr} failed ({status}): {}", + stderr_buf.lines().last().unwrap_or("").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). From 240783dd49d3c0502effbed6512a0fb5f8ecd723 Mon Sep 17 00:00:00 2001 From: damocles Date: Fri, 28 Aug 2026 18:07:24 +0200 Subject: [PATCH 4/6] hive-priv: require exactly one nix build output path, don't just trim --- hive-priv/src/main.rs | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/hive-priv/src/main.rs b/hive-priv/src/main.rs index 0c5b9315..4f1dc185 100644 --- a/hive-priv/src/main.rs +++ b/hive-priv/src/main.rs @@ -816,10 +816,22 @@ async fn nix_build_toplevel(name: &str, mut writer: Option<&mut OwnedWriteHalf>) stderr_buf.lines().last().unwrap_or("").trim() ); } - let path = stdout_buf.trim(); - if path.is_empty() { - bail!("nix build {attr} produced no output path"); - } + // `--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 + // `.trim()` would silently hand a multi-line string on to + // `--system-path` the day that ever changes, which is the same + // corrupted-argument failure this function exists to avoid. Require + // exactly one line and error otherwise, so a future multi-output + // attr fails loudly here instead of downstream in `nixos-container`. + let lines: Vec<&str> = stdout_buf.lines().collect(); + let [path] = lines[..] else { + bail!( + "nix build {attr} produced {} output path(s), expected exactly 1: {stdout_buf:?}", + lines.len() + ); + }; Ok(path.to_owned()) } From 40db6c898776c5ef5c2a92653fb6678c06d4811a Mon Sep 17 00:00:00 2001 From: damocles Date: Fri, 28 Aug 2026 18:24:21 +0200 Subject: [PATCH 5/6] 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 From b35063cf467987980d19e1125659129ddda7aa6a Mon Sep 17 00:00:00 2001 From: damocles Date: Fri, 28 Aug 2026 18:37:08 +0200 Subject: [PATCH 6/6] hive-priv: filter/trim before counting output-path lines, fix log level --- hive-priv/README.md | 13 ++++--- hive-priv/src/main.rs | 80 +++++++++++++++++++++++++++++++------------ 2 files changed, 67 insertions(+), 26 deletions(-) diff --git a/hive-priv/README.md b/hive-priv/README.md index 21e39350..d23b44c3 100644 --- a/hive-priv/README.md +++ b/hive-priv/README.md @@ -82,8 +82,11 @@ 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). +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). diff --git a/hive-priv/src/main.rs b/hive-priv/src/main.rs index 98a28a1c..5038d839 100644 --- a/hive-priv/src/main.rs +++ b/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, @@ -747,7 +747,7 @@ async fn nix_build_toplevel(name: &str, mut writer: Option<&mut OwnedWriteHalf>) // Streamed as it arrives — the progress the dashboard // and `journalctl -f` were missing. Ok(Some(l)) => { - tracing::warn!(target: "nix-build-toplevel", "{l}"); + tracing::info!(target: "nix-build-toplevel", "{l}"); if let Some(w) = writer.as_deref_mut() { write_line_event(w, PrivStream::Stderr, &l).await; } @@ -778,23 +778,36 @@ async fn nix_build_toplevel(name: &str, mut writer: Option<&mut OwnedWriteHalf>) stderr_buf.lines().last().unwrap_or("").trim() ); } - // `--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 - // `.trim()` would silently hand a multi-line string on to - // `--system-path` the day that ever changes, which is the same - // corrupted-argument failure this function exists to avoid. Require - // exactly one line and error otherwise, so a future multi-output - // attr fails loudly here instead of downstream in `nixos-container`. - let lines: Vec<&str> = stdout_buf.lines().collect(); - let [path] = lines[..] else { - bail!( - "nix build {attr} produced {} output path(s), expected exactly 1: {stdout_buf:?}", - lines.len() - ); - }; - Ok(path.to_owned()) + 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 @@ -2828,8 +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, toplevel_attr, - 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}; @@ -2855,6 +2868,31 @@ mod tests { ); } + /// `--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. ///