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

remove the hive's local build of agent closures

Browse source 
A rebuild or create downloads the closure the swarm built and applies it; with none to deploy it fails and changes nothing. hive-priv takes a store path instead of an internal/meta commit and builds nothing.
This commit is contained in:
atlas 2026-10-29 22:17:07 +01:00
commit 4590b04cac
1 changed files with 632 additions and 1351 deletions
Download patch file Download diff file Expand all files Collapse all files

318
hive-priv/src/container.rs
View file

@ -3,12 +3,11 @@
//! and `ListContainers`. Every op takes a logical agent name and reaches only
//! the `h-`-prefixed container [`container_system_name`] derives from it.
//!
//! `CreateContainer` / `UpdateContainer` build the toplevel here
//! ([`nix_build_toplevel`]) and stream progress to the caller as
//! [`PrivEvent::Line`]s when asked to.
//! `CreateContainer` / `UpdateContainer` apply a closure already in the store
//! and stream progress to the caller as [`PrivEvent::Line`]s when asked to.
use anyhow::{Context as _, Result, anyhow, bail};
use hive_priv_sock::{PrivEvent, PrivStream, PrivStreamLine, meta_flake_url, valid_meta_rev};
use anyhow::{Context as _, Result, bail};
use hive_priv_sock::{PrivEvent, PrivStream, PrivStreamLine, valid_closure_path};
use tokio::io::{AsyncWriteExt, BufReader};
use tokio::net::unix::OwnedWriteHalf;
use tokio::process::Command;
@ -72,52 +71,43 @@ pub(crate) async fn destroy_container(name: &str) -> Result<(String, String)> {
container_run(&["destroy", &container_system_name(name)]).await
}
/// Shared body for `CreateContainer` / `UpdateContainer`: validate the
/// name, build the toplevel ourselves, and apply it (streaming line
/// events to `writer` when `stream` is set).
/// `toplevel` as a store path, or the error saying it is not one. The path
/// crosses the socket from hive-c0re, which is not root.
fn toplevel_to_apply(toplevel: &str) -> Result<&str> {
if !valid_closure_path(toplevel) {
bail!("{toplevel:?} is not a store path");
}
Ok(toplevel)
}
/// Shared body for `CreateContainer` / `UpdateContainer`: validate the name
/// and the closure, and apply it (streaming line events to `writer` when
/// `stream` is set). Nothing is built here: `toplevel` is a closure already in
/// the store, which `RealiseClosure` put there.
///
/// **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.
/// `create` passes `--system-path` so `nixos-container`'s own `buildFlake()`
/// never runs; it builds to a relative `.tmp` out-link that concurrent calls
/// share, which handed an agent another agent's closure.
///
/// **`update` no longer calls `nixos-container update` at all.** That
/// action's own version-compat probe runs unconditionally before
/// `--system-path` is ever honored, dying on every agent's update.
/// [`swap_container_profile`] replicates exactly what `update`'s own
/// action does *past* that probe (confirmed against its source): `nix-env
/// --set` the per-container profile, then `systemctl reload` if the
/// container is running. `create` is untouched — it isn't the failing
/// verb.
pub(crate) async fn container_flake_action(
/// `update` does not call `nixos-container update` at all: that action's
/// version-compat probe runs before `--system-path` is honored and dies on every
/// agent. [`swap_container_profile`] does what the action does past that probe:
/// `nix-env --set` the per-container profile, then `systemctl reload` if the
/// container is running.
pub(crate) async fn container_toplevel_action(
verb: &str,
name: &str,
meta_rev: &str,
toplevel: &str,
stream: bool,
writer: &mut OwnedWriteHalf,
) -> Result<(String, String)> {
validate_agent_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, meta_rev, stream.then_some(&mut *writer)).await?;
let toplevel = toplevel_to_apply(toplevel)?;
let system_name = container_system_name(name);
if verb == "update" {
return swap_container_profile(&system_name, &toplevel, stream, writer).await;
return swap_container_profile(&system_name, toplevel, stream, writer).await;
}
let args = [verb, &system_name, "--system-path", &toplevel];
let args = [verb, &system_name, "--system-path", toplevel];
if stream {
container_run_streaming(&args, writer).await
} else {
@ -126,7 +116,7 @@ pub(crate) async fn container_flake_action(
}
/// Apply a prebuilt toplevel to an existing container directly, without
/// going through `nixos-container update` (see `container_flake_action`'s
/// going through `nixos-container update` (see `container_toplevel_action`'s
/// doc comment for why). Mirrors `nixos-container.pl`'s own `update`
/// action verbatim, past its version probe: point the per-container `nix-
/// env` profile at the new toplevel, then reload the container unit *if*
@ -209,46 +199,10 @@ async fn log_and_forward(stdout: &str, stderr: &str, stream: bool, writer: &mut
}
}
/// The environment variable naming the forge's `http` origin, which
/// `internal/meta` is fetched from. Set by the unit; never taken from a
/// request.
pub(crate) const FORGE_URL_ENV: &str = "HIVE_PRIV_FORGE_URL";
/// The flake agent toplevels are built from: `internal/meta` at `rev`. The URL
/// is `forge_base`, hive-priv's own; only the rev comes from the request, and
/// it is checked here.
fn meta_flake(rev: &str, forge_base: Option<&str>) -> Result<String> {
if !valid_meta_rev(rev) {
bail!("meta_rev {rev:?} is not a 40-digit lowercase hex commit id");
}
let base = forge_base.filter(|b| !b.is_empty()).with_context(|| {
format!("{FORGE_URL_ENV} is unset, so there is no internal/meta to build from")
})?;
Ok(meta_flake_url(base, rev))
}
/// 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(flake: &str, name: &str) -> String {
format!("{flake}#nixosConfigurations.{name}.config.system.build.toplevel")
}
/// Wall-clock bound on [`nix_build_toplevel`]. Four times CI's observed
/// cold-cache `nix flake check` (~15 min, `.forgejo/workflows/ci.yml`), which
/// compiles this workspace's crates, the part of an agent toplevel no binary
/// cache holds; the rebuild path has usually warmed this exact attr already
/// (`hive-c0re`'s `prebuild_toplevel`). A stalled download is bounded by nix's
/// own `stalled-download-timeout`; this covers the rest, such as a wedged
/// nix-daemon holding the caller's build slot forever.
const NIX_BUILD_TIMEOUT: std::time::Duration = std::time::Duration::from_hours(1);
/// How a [`run_bounded`] child ended.
pub(crate) enum BoundedRun {
Exited {
status: std::process::ExitStatus,
stdout: String,
stderr: String,
},
/// Still running at the limit; its whole process group was killed.
@ -256,8 +210,7 @@ pub(crate) enum BoundedRun {
}
/// Run `cmd` to completion or until `limit`, draining both pipes: stdout is
/// captured silently, stderr is logged and, when `writer` is set, forwarded
/// live as [`PrivEvent::Line`]s.
/// discarded, stderr is kept and also logged as it arrives.
///
/// The child leads its own process group and a timeout `SIGKILL`s the whole
/// group, so a grandchild still holding the pipes (or the daemon connection)
@ -265,7 +218,6 @@ pub(crate) enum BoundedRun {
pub(crate) async fn run_bounded(
mut cmd: Command,
limit: std::time::Duration,
mut writer: Option<&mut OwnedWriteHalf>,
) -> std::io::Result<BoundedRun> {
use tokio::io::AsyncBufReadExt as _;
@ -282,13 +234,10 @@ pub(crate) async fn run_bounded(
let run = async {
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.
// ⚠️ Both pipes are drained concurrently: reading one alone would let
// the other fill its pipe buffer and deadlock the child.
//
// ⚠️ Each stream's EOF is tracked separately rather than breaking on
// the first `None`: `next_line()` on a closed stream returns
@ -300,27 +249,18 @@ pub(crate) async fn run_bounded(
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(Some(_)) => {}
Ok(None) => stdout_done = true,
Err(e) => {
tracing::warn!(error = %e, "nix build stdout read error");
tracing::warn!(error = %e, "child stdout read error");
stdout_done = true;
}
}
}
line = stderr_lines.next_line(), if !stderr_done => {
match line {
// Streamed as it arrives, so the dashboard and
// `journalctl -f` show build progress live.
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;
}
tracing::info!(target: "nix-store", "{l}");
if !stderr_buf.is_empty() {
stderr_buf.push('\n');
}
@ -328,7 +268,7 @@ pub(crate) async fn run_bounded(
}
Ok(None) => stderr_done = true,
Err(e) => {
tracing::warn!(error = %e, "nix build stderr read error");
tracing::warn!(error = %e, "child stderr read error");
stderr_done = true;
}
}
@ -338,7 +278,6 @@ pub(crate) async fn run_bounded(
let status = child.wait().await?;
Ok::<_, std::io::Error>(BoundedRun::Exited {
status,
stdout: stdout_buf,
stderr: stderr_buf,
})
};
@ -356,90 +295,6 @@ pub(crate) async fn run_bounded(
Ok(BoundedRun::TimedOut)
}
/// 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. Bounded by [`NIX_BUILD_TIMEOUT`].
async fn nix_build_toplevel(
name: &str,
meta_rev: &str,
writer: Option<&mut OwnedWriteHalf>,
) -> Result<String> {
let flake = meta_flake(meta_rev, std::env::var(FORGE_URL_ENV).ok().as_deref())?;
let attr = toplevel_attr(&flake, name);
let mut cmd = Command::new("nix");
cmd.args([
"--extra-experimental-features",
"nix-command flakes",
"build",
"--no-link",
"--print-out-paths",
&attr,
]);
let (status, stdout_buf, stderr_buf) = match run_bounded(cmd, NIX_BUILD_TIMEOUT, writer)
.await
.with_context(|| format!("run nix build {attr}"))?
{
BoundedRun::Exited {
status,
stdout,
stderr,
} => (status, stdout, stderr),
BoundedRun::TimedOut => bail!(
"nix build {attr} timed out after {}s; killed it",
NIX_BUILD_TIMEOUT.as_secs()
),
};
// ⚠️ Success is decided by the exit status, not by "we parsed a
// path" — a build can print to stdout and still fail.
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()),
}
}
/// Invoke `nixos-container` with the given args, log output to journald.
pub(crate) async fn container_run(args: &[&str]) -> Result<(String, String)> {
let out = Command::new("nixos-container")
@ -709,85 +564,37 @@ async fn container_run_streaming(
}
Ok((String::new(), String::new()))
}
#[cfg(test)]
mod tests {
use super::{
BoundedRun, FORGE_URL_ENV, meta_flake, run_bounded, single_output_path, toplevel_attr,
};
use super::{BoundedRun, run_bounded, toplevel_to_apply};
use crate::shared::test_support::scratch;
/// 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`. The flake is `internal/meta` at the rev, and its
/// URL is the unit's, never the request's.
#[test]
fn a_meta_rev_builds_from_internal_meta_at_the_units_forge() {
let rev = "c".repeat(40);
let forge = Some("http://forge.t.local");
assert_eq!(
toplevel_attr(&meta_flake(&rev, forge).unwrap(), "atlas"),
format!(
"git+http://forge.t.local/internal/meta.git?rev={rev}\
#nixosConfigurations.atlas.config.system.build.toplevel"
)
);
}
const TOPLEVEL: &str = "/nix/store/0123456789abcdfghijklmnpqrsvwxyz-nixos-system-atlas";
/// Nothing but a commit id gets into the ref, whatever the request says.
/// A store path passes through unchanged; anything else, whatever the
/// request says, never reaches `nix-env` or `nixos-container`.
#[test]
fn a_meta_rev_that_is_not_a_commit_id_is_refused() {
let forge = Some("http://forge.t.local");
fn only_a_store_path_is_applied() {
assert_eq!(toplevel_to_apply(TOPLEVEL).unwrap(), TOPLEVEL);
for bad in [
"main",
"",
"main",
"../../x",
&format!("{}#evil", "a".repeat(34)),
&format!("{}&ref=x", "a".repeat(34)),
"/etc/passwd",
"/nix/store/x",
&format!("{TOPLEVEL}/bin"),
&format!("{TOPLEVEL};id"),
&"c".repeat(40),
] {
let err = meta_flake(bad, forge).unwrap_err();
let err = toplevel_to_apply(bad).unwrap_err();
assert!(
format!("{err:#}").contains("not a 40-digit"),
format!("{err:#}").contains("not a store path"),
"{bad}: {err:#}"
);
}
}
#[test]
fn a_meta_rev_without_a_forge_in_the_unit_is_refused() {
let rev = "c".repeat(40);
for forge in [None, Some("")] {
let err = meta_flake(&rev, forge).unwrap_err();
assert!(format!("{err:#}").contains(FORGE_URL_ENV), "{err:#}");
}
}
/// `--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)
);
}
/// Whether `pid` is gone, a zombie (`Z`), or being reaped (`X`): none of
/// these runs again, and a process passes through them in that order,
/// so once this is true it stays true.
@ -817,7 +624,7 @@ mod tests {
/// reaches its whole process group: the backgrounded grandchild dies too,
/// not just the shell that started it.
#[tokio::test]
async fn a_build_past_its_limit_is_killed_with_its_group() {
async fn a_child_past_its_limit_is_killed_with_its_group() {
use std::time::{Duration, Instant};
let dir = scratch();
let pidfile = dir.join("grandchild");
@ -826,9 +633,7 @@ mod tests {
.arg(format!("sleep 600 & echo $! > {}; wait", pidfile.display()));
let started = Instant::now();
let run = run_bounded(cmd, Duration::from_secs(2), None)
.await
.unwrap();
let run = run_bounded(cmd, Duration::from_secs(2)).await.unwrap();
assert!(matches!(run, BoundedRun::TimedOut), "must time out");
assert!(
started.elapsed() < Duration::from_mins(1),
@ -846,23 +651,18 @@ mod tests {
}
/// The control: a child that exits inside its limit is `Exited`, with its
/// status and both streams kept apart.
/// status and its stderr.
#[tokio::test]
async fn a_build_inside_its_limit_returns_its_output() {
async fn a_child_inside_its_limit_returns_its_status_and_stderr() {
let mut cmd = tokio::process::Command::new("sh");
cmd.arg("-c")
.arg("echo progress >&2; echo /nix/store/abc-toplevel");
match run_bounded(cmd, std::time::Duration::from_mins(1), None)
match run_bounded(cmd, std::time::Duration::from_mins(1))
.await
.unwrap()
{
BoundedRun::Exited {
status,
stdout,
stderr,
} => {
BoundedRun::Exited { status, stderr } => {
assert!(status.success());
assert_eq!(stdout, "/nix/store/abc-toplevel\n");
assert_eq!(stderr, "progress");
}
BoundedRun::TimedOut => panic!("a fast child must not time out"),