From b118b12520589c631d69e9bf4110f913d6971d44 Mon Sep 17 00:00:00 2001 From: damocles Date: Sun, 2 Aug 2026 19:03:22 +0200 Subject: [PATCH] hive-priv/hive-c0re: drop stop's SIGKILL escalation, surface a crit dashboard warning instead --- hive-c0re/src/lifecycle/mod.rs | 52 +++++++++++++++++++++++++++++++++- hive-priv/src/main.rs | 46 ++++++++++++++---------------- 2 files changed, 73 insertions(+), 25 deletions(-) diff --git a/hive-c0re/src/lifecycle/mod.rs b/hive-c0re/src/lifecycle/mod.rs index 7ff2b32f..6f816f02 100644 --- a/hive-c0re/src/lifecycle/mod.rs +++ b/hive-c0re/src/lifecycle/mod.rs @@ -330,9 +330,59 @@ pub async fn agents_for_meta_listing() -> Result> { agents_for_meta(None).await } +/// Per-agent "stop failed" banner guards, keyed by agent name. Cleared the +/// next time that agent's container stops cleanly — a `Mutex>` +/// rather than a bare `set_boot_warning` because this condition (unlike a +/// one-shot boot step) genuinely resolves later, once an operator has +/// intervened, and the banner should clear with it instead of surviving +/// until the next hive-c0re restart. +fn stop_failed_guards() +-> &'static std::sync::Mutex> { + static REG: std::sync::OnceLock< + std::sync::Mutex>, + > = std::sync::OnceLock::new(); + REG.get_or_init(|| std::sync::Mutex::new(std::collections::HashMap::new())) +} + +/// Leak a per-agent warning `kind`. Agent names are a small, bounded set — +/// leaking one short string per distinct agent that ever hits a stop +/// failure is cheap, same reasoning as `forge::static_kind`. +fn static_stop_kind(name: &str) -> &'static str { + Box::leak(format!("agent_stop_failed_{name}").into_boxed_str()) +} + +/// Stop `name`'s container. See `hive-priv`'s `stop_and_release` for why +/// a failure here means the container is likely wedged rather than just +/// slow — SIGKILL doesn't recover that case in practice, so this +/// surfaces it on the dashboard instead of silently retrying. pub async fn kill(name: &str) -> Result<()> { validate(name)?; - priv_run("stop", name).await + let result = priv_run("stop", name).await; + // Lock only for the synchronous map update — never held across the + // `.await` above. + let mut guards = stop_failed_guards() + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner); + match result { + Ok(()) => { + // A clean stop clears any earlier "this looked wedged" banner + // for the same agent. + guards.remove(name); + Ok(()) + } + Err(e) => { + tracing::error!(%name, error = %e, "container stop failed"); + guards.insert( + name.to_owned(), + crate::warnings::set_warning( + static_stop_kind(name), + "crit", + format!("{name}: stop failed — {e}"), + ), + ); + Err(e) + } + } } /// Start a container. Success is defined as the container's unit reaching diff --git a/hive-priv/src/main.rs b/hive-priv/src/main.rs index b979b185..b2091554 100644 --- a/hive-priv/src/main.rs +++ b/hive-priv/src/main.rs @@ -1927,12 +1927,19 @@ async fn wait_name_released(machine: &str) -> bool { /// there is no cleaning it up after the fact. The only place to catch it is /// here, in the stop. /// -/// So: ask for the stop, wait for the name, and escalate to SIGKILL if it is -/// still held. If even that doesn't free it, fail loudly — a caller that is -/// told the stop worked will go on to start the container and hit the -/// confusing registration error instead of this one. +/// So: ask for the stop, wait for the name, and fail loudly if it is still +/// held — a caller that is told the stop worked will go on to start the +/// container and hit the confusing registration error instead of this one. /// -/// The verify-then-escalate lives in the helper rather than at a call site so +/// This used to escalate to `machinectl kill --signal=SIGKILL` here. Dropped +/// per real incident data: the case this guards is a container genuinely +/// wedged (e.g. a root-login process survived the stop), and SIGKILL +/// doesn't recover that in practice — only a host-level reboot has. +/// Pretending a kill attempt handled it hides a condition that needs a human +/// to look at the host, so this now just reports the failure instead of +/// quietly (and ineffectually) trying to force it. +/// +/// The verify-then-fail lives in the helper rather than at a call site so /// that every stop gets it: dashboard, reconcile, destroy, cold-start /// fallback. The start path already distrusts its own exit code the same way; /// this is the missing half of that pair. @@ -1945,28 +1952,19 @@ async fn stop_and_release(machine: &str) -> Result<(String, String)> { return stop; } - tracing::warn!( + tracing::error!( %machine, - "stop finished but machined still holds the registration; escalating to SIGKILL" - ); - let kill = machinectl_run(&["kill", machine, "--signal=SIGKILL"]).await; - if wait_name_released(machine).await { - if let Err(e) = &kill { - // The name is free, which is what was asked for. machinectl - // complaining on the way there is worth a line, not a failure. - tracing::warn!(%machine, error = %e, "SIGKILL reported an error but the name was released"); - } - return Ok(kill.unwrap_or_default()); - } - - let detail = kill.err().map_or_else( - || "SIGKILL was delivered".to_owned(), - |e| format!("SIGKILL also failed: {e}"), + "stop finished but machined still holds the registration — the \ + container is likely wedged (a process outside its init tree \ + survived the stop); this needs a host-level look, not another \ + stop attempt" ); bail!( - "stop {machine}: machined still holds the machine name after {}s ({detail}). \ - Starting this container again will fail to register.", - NAME_RELEASE_TIMEOUT.as_secs() * 2 + "stop {machine}: machined still holds the machine name after {}s. \ + This container is likely wedged and starting it again will fail to \ + register — needs host-level intervention (a reboot has been the \ + only reliable fix in practice).", + NAME_RELEASE_TIMEOUT.as_secs() ) }