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:#}"); +}