From 5409f8160be60838d1d5200243baafcfb4d621b0 Mon Sep 17 00:00:00 2001 From: atlas Date: Mon, 28 Sep 2026 23:22:10 +0200 Subject: [PATCH] hive-c0re: bound prebuild_toplevel's nix build with a timeout prebuild_toplevel awaited nix build with a bare child.wait().await, so a wedged nix-daemon (unreachable remote builder, stuck build slot) hung the rebuild job forever with no way for the job queue to recover short of restarting hive-c0re. Mirrors #4741's hive-priv fix: the child now leads its own process group, and after PREBUILD_TIMEOUT (1h, four times CI's observed cold-cache flake check) the whole group is SIGKILLed and the call fails with a named timeout error instead of hanging. Refs #4723, #4741 --- hive-c0re/src/lifecycle/mod.rs | 54 ++++++++++++++++++++++++++-- hive-c0re/src/lifecycle/tests.rs | 62 ++++++++++++++++++++++++++++++++ 2 files changed, 113 insertions(+), 3 deletions(-) diff --git a/hive-c0re/src/lifecycle/mod.rs b/hive-c0re/src/lifecycle/mod.rs index 7203e9a5..6d3e449c 100644 --- a/hive-c0re/src/lifecycle/mod.rs +++ b/hive-c0re/src/lifecycle/mod.rs @@ -694,6 +694,39 @@ fn confirm_gone_after_failed_destroy( } } +/// Wall-clock bound on [`prebuild_toplevel`]'s `nix build`. 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. A stalled download is bounded by nix's own +/// `stalled-download-timeout`; this covers the rest, such as a wedged +/// nix-daemon holding the rebuild's build slot forever. +const PREBUILD_TIMEOUT: std::time::Duration = std::time::Duration::from_hours(1); + +/// Wait for `child` until `limit`. `None` means it was still running, and its +/// whole process group was `SIGKILL`ed and the child reaped, so a grandchild +/// holding the daemon connection dies with it. The child must have been +/// spawned with `process_group(0)`. +async fn wait_bounded( + child: &mut tokio::process::Child, + limit: std::time::Duration, +) -> std::io::Result> { + // Group leader, so its pid is the pgid. Read before waiting: a reaped + // child has no id. + let pgid = child.id().and_then(|pid| i32::try_from(pid).ok()); + if let Ok(status) = tokio::time::timeout(limit, child.wait()).await { + return status.map(Some); + } + if let Some(pgid) = pgid { + // SAFETY: plain `kill(2)`; `-pgid` targets the child's own process + // group, which `process_group(0)` made it lead. + unsafe { + libc::kill(-pgid, libc::SIGKILL); + } + } + child.wait().await?; + Ok(None) +} + /// Pre-build `system.build.toplevel` against `meta#` so the /// subsequent `nixos-container update` finds the result cached and /// skips straight to the profile-swap. Store-warming only — container @@ -750,6 +783,7 @@ pub async fn prebuild_toplevel(name: &str, flake_ref: &str, node_id: Option .args(&args) .stdout(std::process::Stdio::piped()) .stderr(std::process::Stdio::piped()) + .process_group(0) .spawn() .with_context(|| format!("spawn {cmdline}"))?; @@ -780,10 +814,24 @@ pub async fn prebuild_toplevel(name: &str, flake_ref: &str, node_id: Option } }); - let status = child - .wait() + let Some(status) = wait_bounded(&mut child, PREBUILD_TIMEOUT) .await - .with_context(|| format!("wait {cmdline}"))?; + .with_context(|| format!("wait {cmdline}"))? + else { + // A process that left the group can still hold the pipes open. + pump_stdout.abort(); + pump_stderr.abort(); + if let (Some(h), Some(id)) = (&logs, log_id) { + h.finish(id, crate::build_logs::BuildStatus::Fail); + } + let secs = PREBUILD_TIMEOUT.as_secs(); + match log_id { + Some(id) => { + bail!("prebuild {cmdline} timed out after {secs}s; killed it; see build log #{id}") + } + None => bail!("prebuild {cmdline} timed out after {secs}s; killed it"), + } + }; let _ = pump_stdout.await; let _ = pump_stderr.await; diff --git a/hive-c0re/src/lifecycle/tests.rs b/hive-c0re/src/lifecycle/tests.rs index 7114c330..8bea8469 100644 --- a/hive-c0re/src/lifecycle/tests.rs +++ b/hive-c0re/src/lifecycle/tests.rs @@ -348,3 +348,65 @@ fn agent_names_propagates_an_unreadable_list() { .expect_err("an unreadable list must not read as zero agents"); assert!(format!("{err:#}").contains("hive-priv"), "{err:#}"); } + +fn is_dead(pid: &str) -> bool { + // A killed process that is not our child stays a zombie until its own + // parent reaps it, so a zombie counts as dead. + match std::fs::read_to_string(format!("/proc/{pid}/stat")) { + Err(_) => true, + Ok(stat) => stat + .rsplit_once(") ") + .is_some_and(|(_, rest)| rest.starts_with('Z')), + } +} + +/// A child still running at the limit comes back `None`, and the kill reaches +/// its whole process group: the backgrounded grandchild dies too, not just the +/// shell that started it. +#[tokio::test] +async fn a_prebuild_past_its_limit_is_killed_with_its_group() { + use std::time::{Duration, Instant}; + let dir = tempfile::tempdir().expect("tempdir"); + let pidfile = dir.path().join("grandchild"); + let mut child = Command::new("sh") + .arg("-c") + .arg(format!("sleep 600 & echo $! > {}; wait", pidfile.display())) + .process_group(0) + .spawn() + .expect("spawn sh"); + + let started = Instant::now(); + let status = wait_bounded(&mut child, Duration::from_secs(2)) + .await + .expect("wait_bounded"); + assert!(status.is_none(), "must time out, got {status:?}"); + assert!( + started.elapsed() < Duration::from_mins(1), + "the limit did not bound the wait" + ); + + let pid = std::fs::read_to_string(&pidfile).expect("grandchild pid"); + let pid = pid.trim(); + let deadline = Instant::now() + Duration::from_secs(5); + while !is_dead(pid) && Instant::now() < deadline { + tokio::time::sleep(Duration::from_millis(50)).await; + } + assert!(is_dead(pid), "grandchild {pid} outlived the timeout"); +} + +/// The control: a child that exits inside its limit comes back with its +/// status. +#[tokio::test] +async fn a_prebuild_inside_its_limit_returns_its_status() { + let mut child = Command::new("sh") + .arg("-c") + .arg("exit 3") + .process_group(0) + .spawn() + .expect("spawn sh"); + let status = wait_bounded(&mut child, std::time::Duration::from_mins(1)) + .await + .expect("wait_bounded") + .expect("a fast child must not time out"); + assert_eq!(status.code(), Some(3)); +}