From 18bd8dd2c741982b60dff19c08eba79cc0b7add7 Mon Sep 17 00:00:00 2001 From: atlas Date: Thu, 24 Sep 2026 11:00:51 +0200 Subject: [PATCH] hive-c0re: fail a nixos-container destroy that leaves the container in place lifecycle::destroy logged a failed `nixos-container destroy` and returned Ok, so the DestroyContainer node went green, the agent was unregistered, and with purge the after_ok PurgeState deleted its state while the container config and root still existed. Propagate the error unless the container list, read after the failure, no longer names the container. An unreadable list fails too. --- hive-c0re/src/job_queue/exec.rs | 4 ++- hive-c0re/src/lifecycle/mod.rs | 33 +++++++++++++++++++-- hive-c0re/src/lifecycle/tests.rs | 51 ++++++++++++++++++++++++++++++++ 3 files changed, 85 insertions(+), 3 deletions(-) diff --git a/hive-c0re/src/job_queue/exec.rs b/hive-c0re/src/job_queue/exec.rs index d7f82ce0..4d3da7d0 100644 --- a/hive-c0re/src/job_queue/exec.rs +++ b/hive-c0re/src/job_queue/exec.rs @@ -267,7 +267,9 @@ async fn sync_meta_after_lifecycle(coord: &Coordinator) -> Result<()> { /// /// The only fallible step is the destroy itself: once the container is gone the /// un-registration cannot meaningfully fail, and returning early would strand -/// the roster claiming an agent that no longer exists. +/// the roster claiming an agent that no longer exists. A destroy that leaves the +/// container in place fails this node before un-registration, and its failure +/// is what keeps the `after_ok` purge and bookkeeping from running. async fn run_destroy_container(coord: &Arc, agent: &str) -> Result<()> { crate::lifecycle::destroy(agent).await?; coord.unregister_agent(agent); diff --git a/hive-c0re/src/lifecycle/mod.rs b/hive-c0re/src/lifecycle/mod.rs index 4a83734d..998397a6 100644 --- a/hive-c0re/src/lifecycle/mod.rs +++ b/hive-c0re/src/lifecycle/mod.rs @@ -721,13 +721,18 @@ pub async fn is_running(name: &str) -> bool { /// Fully tear down a sub-agent's container: stop + remove via `nixos-container /// destroy`, then clean our own systemd drop-in. Leaves it to the caller to /// wipe `/var/lib/hyperhive/...` state and the per-agent runtime dir. +/// +/// Fails when the container survives the destroy. The destroy DAG purges the +/// agent's state only after this returns `Ok`, so a swallowed failure here +/// deletes the state of a container that still exists. pub async fn destroy(name: &str) -> Result<()> { validate(name)?; let container = container_name(name); // nixos-container destroy handles stop + removal of /var/lib/nixos-containers/ - // and /etc/nixos-containers/.conf. Tolerate "no such container". + // and /etc/nixos-containers/.conf. When it errors, the container list + // decides: a container that is already gone is still a successful destroy. if let Err(e) = priv_run("destroy", name).await { - tracing::warn!(error = ?e, "nixos-container destroy returned an error; continuing cleanup"); + confirm_gone_after_failed_destroy(e, &container, list().await)?; } // Remove the systemd resource-limits drop-in via hive-priv. if let Err(e) = crate::priv_client::remove_service_dropin(&container).await { @@ -736,6 +741,30 @@ pub async fn destroy(name: &str) -> Result<()> { Ok(()) } +/// Settle a failed `nixos-container destroy` against the container list taken +/// after it. `Ok` only when the list is readable and no longer names the +/// container; a list that cannot be read proves nothing, so it fails too. +fn confirm_gone_after_failed_destroy( + destroy_err: anyhow::Error, + container: &str, + listed: Result>, +) -> Result<()> { + match listed { + Ok(names) if !names.iter().any(|n| n == container) => { + tracing::warn!( + error = ?destroy_err, + %container, + "nixos-container destroy returned an error, but the container is gone" + ); + Ok(()) + } + Ok(_) => Err(destroy_err.context(format!("container {container} still exists"))), + Err(list_err) => Err(destroy_err.context(format!( + "cannot confirm whether container {container} still exists: {list_err:#}" + ))), + } +} + /// 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 diff --git a/hive-c0re/src/lifecycle/tests.rs b/hive-c0re/src/lifecycle/tests.rs index c48720b8..ce50093c 100644 --- a/hive-c0re/src/lifecycle/tests.rs +++ b/hive-c0re/src/lifecycle/tests.rs @@ -326,3 +326,54 @@ fn passwd_parse_rejects_unusable_ids() { Some((1000, 994)) ); } + +/// A failed `nixos-container destroy` whose container is still listed fails, +/// and keeps hive-priv's error in the chain. The destroy DAG gates the state +/// purge on this call, so this is the case that must not read as success. +#[test] +fn failed_destroy_with_the_container_still_listed_fails() { + let err = confirm_gone_after_failed_destroy( + anyhow::anyhow!("nixos-container destroy h-doomed failed (exit status: 1)"), + "h-doomed", + Ok(vec!["h-other".to_owned(), "h-doomed".to_owned()]), + ) + .expect_err("a surviving container must fail the destroy"); + let chain = format!("{err:#}"); + assert!(chain.contains("h-doomed still exists"), "{chain}"); + assert!(chain.contains("exit status: 1"), "{chain}"); +} + +/// A container missing from the list is gone, whatever the destroy exit code +/// said, so the destroy succeeds. +#[test] +fn failed_destroy_with_the_container_gone_succeeds() { + confirm_gone_after_failed_destroy( + anyhow::anyhow!("nixos-container destroy h-doomed failed"), + "h-doomed", + Ok(vec!["h-other".to_owned()]), + ) + .expect("an absent container is a completed destroy"); + // Control: the same list with the container in it fails, so the pass + // above comes from the absence, not from the helper accepting anything. + assert!( + confirm_gone_after_failed_destroy( + anyhow::anyhow!("nixos-container destroy h-doomed failed"), + "h-doomed", + Ok(vec!["h-other".to_owned(), "h-doomed".to_owned()]), + ) + .is_err() + ); +} + +/// An unreadable list proves nothing about the container, so the destroy +/// fails rather than guessing it is gone. +#[test] +fn failed_destroy_with_an_unreadable_list_fails() { + let err = confirm_gone_after_failed_destroy( + anyhow::anyhow!("nixos-container destroy h-doomed failed"), + "h-doomed", + Err(anyhow::anyhow!("connect to hive-priv socket")), + ) + .expect_err("an unreadable list must not pass for an absent container"); + assert!(format!("{err:#}").contains("cannot confirm"), "{err:#}"); +}