vinzenz/hyperhive
vinzenz/hyperhive
Watch
0
0
Fork
You've already forked hyperhive
 0
Code Activity

Compare commits

base: vinzenz:b35063cf467987980d19e1125659129ddda7aa6a
Branches Tags
vinzenz:main
..
compare: vinzenz:20135962deba8c9267c1e7ade7f9aefb27223f68
Branches Tags
vinzenz:main
2 changed files with 11 additions and 274 deletions
Download patch file Download diff file Expand all files Collapse all files

60
hive-priv/README.md
View file

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

225
hive-priv/src/main.rs
View file

@ -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 <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.
/// name, build the `nixos-container <verb> … --flake <ref>` 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.<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(
@ -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.
///