From bbbc2e28c7181d6dd691b58325a490a7e21a608f Mon Sep 17 00:00:00 2001
From: damocles <damocles@hyperhive.local>
Date: Fri, 10 Jul 2026 12:49:44 +0200
Subject: [PATCH] fix(#2319): treat container start as success when the unit
 reaches active, not on the start exit code

---
 hive-c0re/src/lifecycle/mod.rs | 100 +++++++++++++++++++++++++--------
 1 file changed, 77 insertions(+), 23 deletions(-)

diff --git a/hive-c0re/src/lifecycle/mod.rs b/hive-c0re/src/lifecycle/mod.rs
index 207976b0..44f109f1 100644
--- a/hive-c0re/src/lifecycle/mod.rs
+++ b/hive-c0re/src/lifecycle/mod.rs
@@ -418,9 +418,31 @@ pub async fn kill(name: &str) -> Result<()> {
     priv_run("stop", name).await
 }
 
+/// Start a container. Success is defined as the container's unit reaching
+/// `active`, **not** the `nixos-container start` exit code — a start-job
+/// timeout on a slow boot (e.g. DHCP taking ~10s) exits non-zero while
+/// `container@<c>.service` keeps retrying and the container comes up seconds
+/// later. So a zero exit returns immediately (already active), and a non-zero
+/// exit polls the unit state for [`START_SETTLE_TIMEOUT`] before concluding
+/// the start actually failed. Every caller (dashboard restart, reconcile
+/// start, the cold-start fallback) gets this truth for free.
 pub async fn start(name: &str) -> Result<()> {
     validate(name)?;
-    priv_run("start", name).await
+    if priv_run("start", name).await.is_ok() {
+        return Ok(());
+    }
+    let container = container_name(name);
+    if wait_until_running(name, START_SETTLE_TIMEOUT).await {
+        tracing::info!(
+            container = %container,
+            "start exited non-zero but the container reached active on systemd's retry; treating as success"
+        );
+        return Ok(());
+    }
+    Err(anyhow::anyhow!(
+        "container {container} did not reach active within {}s of start",
+        START_SETTLE_TIMEOUT.as_secs()
+    ))
 }
 
 /// Opaque token produced by [`converge_start_preamble`].
@@ -472,41 +494,73 @@ pub async fn start_with_fallback(token: StartableAgent) -> Result<()> {
     start_with_fallback_inner(&token.name).await
 }
 
+/// How long to wait for a container's unit to reach `active` after a
+/// `nixos-container start` that returned a non-zero exit. A start-job timeout
+/// on a slow boot (e.g. DHCP taking ~10s) returns an error while
+/// `container@<c>.service` keeps retrying and the container comes up seconds
+/// later — so the exit code is NOT authoritative, the unit state is. 60s
+/// comfortably covers an observed slow boot (the reported incident settled
+/// ~16s after the "failure") without pinning a queue node on a genuine
+/// never-boots failure for too long.
+const START_SETTLE_TIMEOUT: std::time::Duration = std::time::Duration::from_mins(1);
+
+/// Poll cadence while waiting for a container to reach `active`.
+const START_SETTLE_POLL: std::time::Duration = std::time::Duration::from_secs(2);
+
+/// Poll [`is_running`] until the container's unit is active or `timeout`
+/// elapses; returns true as soon as it's active.
+async fn wait_until_running(name: &str, timeout: std::time::Duration) -> bool {
+    let deadline = tokio::time::Instant::now() + timeout;
+    loop {
+        if is_running(name).await {
+            return true;
+        }
+        if tokio::time::Instant::now() >= deadline {
+            return false;
+        }
+        tokio::time::sleep(START_SETTLE_POLL).await;
+    }
+}
+
 /// Internal implementation of the cold-start fallback. Used by
 /// [`start_with_fallback`] (public, token-gated) and by
 /// [`rebuild_no_meta`] where the preamble is already enforced structurally.
+///
+/// [`start`] already treats unit-active (not the exit code) as success and
+/// waits out a slow boot, so this only layers the activation-error recovery on
+/// top: if the container is still down after `start`, tear it down and try
+/// `start` once more. The teardown prefers a graceful `stop` and only
+/// escalates to SIGKILL when that `stop` itself fails.
 async fn start_with_fallback_inner(name: &str) -> Result<()> {
     validate(name)?;
-    if let Err(start_err) = priv_run("start", name).await {
-        let container = container_name(name);
+    if start(name).await.is_ok() {
+        return Ok(());
+    }
+    tracing::warn!(
+        %name,
+        "start did not bring the container up; hard-resetting (stop, kill only if that fails) and retrying once"
+    );
+    // Graceful `stop` first; only escalate to SIGKILL if the stop itself
+    // fails (e.g. a wedged container whose `machinectl poweroff` never
+    // completes). A clean stop is enough on its own — SIGKILL is the
+    // last-resort teardown for when graceful shutdown can't finish.
+    if let Err(stop_err) = priv_run("stop", name).await {
         tracing::warn!(
-            container = %container,
-            error = %start_err,
-            "start failed (possible activation error); retrying via stop + kill + start"
+            %name,
+            error = %stop_err,
+            "graceful stop failed; escalating to SIGKILL"
         );
-        priv_run("stop", name).await.unwrap_or_else(|e| {
-            tracing::warn!(
-                container = %container,
-                error = %e,
-                "stop before cold-start retry failed (ignored)"
-            );
-        });
         priv_run("kill", name).await.unwrap_or_else(|e| {
             tracing::warn!(
-                container = %container,
+                %name,
                 error = %e,
-                "kill before cold-start retry failed (ignored)"
+                "kill after failed stop also failed (ignored)"
             );
         });
-        priv_run("start", name).await.map_err(|e| {
-            anyhow::anyhow!(
-                "cold-start fallback also failed: {e:#} \
-                 (original start error: {start_err:#})"
-            )
-        })
-    } else {
-        Ok(())
     }
+    start(name)
+        .await
+        .with_context(|| format!("cold-start fallback also failed for {name}"))
 }
 
 /// Stop + start without regenerating any config. For "kick the container"