diff --git a/hive-priv/README.md b/hive-priv/README.md index d23b44c3..68da1905 100644 --- a/hive-priv/README.md +++ b/hive-priv/README.md @@ -30,63 +30,3 @@ 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 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 5038d839..53a4bceb 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, anyhow, bail}; +use anyhow::{Context as _, Result, 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,22 +623,8 @@ async fn exec( } /// Shared body for `CreateContainer` / `UpdateContainer`: validate the -/// name, build the toplevel ourselves, and run -/// `nixos-container … --system-path ` (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. +/// name, build the `nixos-container … --flake ` argv, and +/// run it (streaming line events to `writer` when `stream` is set). async fn container_flake_action( verb: &str, name: &str, @@ -646,20 +632,8 @@ async fn container_flake_action( writer: &mut OwnedWriteHalf, ) -> Result<(String, String)> { validate_container_name(name)?; - // 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, - ]; + 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 { @@ -667,149 +641,6 @@ 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, 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 { - 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( @@ -2561,6 +2392,11 @@ 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). @@ -2841,8 +2677,7 @@ 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, single_output_path, - toplevel_attr, write_state_file_nofollow, + limits_dropin_body, redact_secret_line, remove_marker_in, write_state_file_nofollow, }; use std::path::PathBuf; use std::sync::atomic::{AtomicU32, Ordering}; @@ -2855,44 +2690,6 @@ 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. ///