From 633893965773b4b09d2292b061dfd30b3e194746 Mon Sep 17 00:00:00 2001 From: atlas Date: Thu, 13 Aug 2026 23:26:17 +0200 Subject: [PATCH] refactor(#2916): destroy submits a DAG instead of an imperative teardown Destroy was a straight-line async fn with no queue node behind it, so nothing in the graph could answer "is this container going down on purpose?". That gap is why an imperative crash-watch suppression guard existed: an RAII handle held for the operation's duration, a second way to say what every other lifecycle op already says through its node. Reuse the existing Stop node rather than teaching a new node to stop things: Stop -> DestroyContainer -> (PurgeState) -> DestroyBookkeeping Stop already declares takes_container_down honestly, so the suppression is now derived from the graph like every other op's. It also turns the precondition into an edge: DestroyContainer runs only under a completed Stop, so it operates on an already-stopped container and carries takes_container_down = false permanently. A container still alive at that point is a real bug and stays loud instead of being absorbed by a flag -- which matters because a wrong true silently swallows a crash while a wrong false only costs a spurious event. Removes suppress_crash_watch, CrashWatchSuppression, crash_suppressed, crash_watch_suppressed and NO_NODE_LABEL. The migration call sites went with the obsolete startup migrations, so destroy was the last caller and intent now has exactly one home. destroy() becomes a submit-and-return, matching every sibling endpoint (rebuild, kill, restart, start, pause, resume) -- it was the only lifecycle op that awaited its work. The container rescan moves into the bookkeeping tail, so ContainerRemoved now arrives after the 200 rather than before it. Also drops an orphaned doc-comment in coordinator.rs: two stacked blocks where only the second described crash_suppressed, the first documenting a field that no longer exists. Removing the field would have re-pointed it at recent_transient. --- hive-c0re/src/actions.rs | 103 +++----------------- hive-c0re/src/coordinator.rs | 88 ----------------- hive-c0re/src/dashboard/lifecycle_ops.rs | 16 ++-- hive-c0re/src/job_queue/exec.rs | 115 +++++++++++++++++++++++ hive-c0re/src/job_queue/model.rs | 42 +++++++++ hive-c0re/src/job_queue/templates.rs | 53 +++++++++++ hive-c0re/src/job_queue/tests.rs | 91 ++++++++++++++++++ hive-c0re/src/server.rs | 2 +- hive-c0re/src/workers/crash_watch.rs | 10 +- 9 files changed, 326 insertions(+), 194 deletions(-) diff --git a/hive-c0re/src/actions.rs b/hive-c0re/src/actions.rs index 085d2bf7..cdf84e88 100644 --- a/hive-c0re/src/actions.rs +++ b/hive-c0re/src/actions.rs @@ -899,99 +899,20 @@ pub async fn run_finalize_deploy(coord: &Arc, approval_id: i64) -> /// imperative infra that `auto_update::ensure_root_agent` recreates on the /// next hive-c0re startup if absent, so destroying it is transient rather /// than something to refuse at the API. -pub async fn destroy(coord: &Arc, name: &str, purge: bool) -> Result<()> { +/// +/// Submits the teardown DAG and returns — it does not wait for the container to +/// go away. Same contract as every other lifecycle op (`rebuild`, `kill`, +/// `restart`, `start`): the queue owns the work, the caller gets an +/// acknowledgement. Progress is visible as real nodes on the dashboard. +pub fn destroy(coord: &Arc, name: &str, purge: bool) { tracing::info!(%name, purge, "destroy"); - // Guard auto-clears on the success path's final scope exit and on - // every early-return / cancellation along the way. - // Destroy has no queue node behind it, so nothing in the graph says this - // container is going away on purpose — without this the crash watcher - // reports every destroy as a crash and the manager tries to recover it. - let guard = coord.suppress_crash_watch(name); - lifecycle::destroy(name).await?; - coord.unregister_agent(name); - let runtime = crate::paths::agent_runtime_dir(name); - if runtime.exists() { - let _ = std::fs::remove_dir_all(&runtime); + if let Err(e) = coord.job_queue.insert_job(|b| { + crate::job_queue::templates::destroy(b, name, purge); + Vec::new() + }) { + tracing::error!(agent = %name, error = ?e, "destroy: insert failed"); } - if purge { - // The state root may be a btrfs subvolume: a subvolume root - // can't be removed with rmdir/`remove_dir_all`, so delete it via - // hive-priv (root) first. No-op for plain-dir agents — the loop below - // then handles the plain-dir state root plus the applied dir. - if let Err(e) = crate::priv_client::delete_agent_subvolume(name).await { - tracing::warn!(error = ?e, %name, "purge: delete state subvolume failed"); - } - // A malformed name can't have a persistent state tree (the state dir - // is only ever created under a validated Ident), so its removal is a - // no-op — skip the state-dir sweep and just clear the applied dir. - let state_dir = hive_types::Ident::parse(name) - .ok() - .map(|id| crate::paths::agent_state_dir(&id)); - for dir in state_dir - .into_iter() - .chain([crate::paths::applied_dir(name)]) - { - if dir.exists() - && let Err(e) = std::fs::remove_dir_all(&dir) - { - tracing::warn!(error = ?e, dir = %dir.display(), "purge: remove failed"); - } - } - } - // Meta flake: drop the agent's input + nixosConfiguration so a - // future spawn under the same name re-seeds cleanly, and so the - // meta lock doesn't reference a vanished applied repo. Log + keep - // going on failure — destroy already succeeded at the - // nixos-container level, the meta repo is just bookkeeping. - if let Err(e) = sync_meta_after_lifecycle(coord).await { - tracing::warn!(error = ?e, %name, "meta sync after destroy failed"); - } - let _ = coord.approvals.fail_pending_for_agent( - name, - if purge { - "agent purged" - } else { - "agent destroyed" - }, - ); - // Drop the durable power intent — a future agent of the same name - // seeds fresh from its observed state. - if let Err(e) = coord.power.remove(name) { - tracing::warn!(%name, error = ?e, "agent_power: remove on destroy failed"); - } - drop(guard); - let _ = coord - .push_todo( - hive_sh4re::manager::MANAGER_AGENT, - "core", - Some(format!("destroyed:{name}")), - format!("agent '{name}' destroyed"), - None, - false, - ) - .await; - // Container row disappeared — rescan so the dashboard fires - // `ContainerRemoved` for the gone row, then emit the - // tombstones snapshot (gained one on destroy, lost one on - // purge — recompute either way). - coord.rescan_containers_and_emit().await; - crate::dashboard::emit_tombstones_snapshot(coord).await; - // Re-emit the schedules snapshot: the rescan above refreshed the live - // roster, so any schedule that still targets the just-destroyed agent - // now drops that ghost column live (no page reload needed). - coord.emit_schedules_snapshot(); - // Update tmpfiles.d to remove the destroyed agent's dirs from the - // boot-time pre-creation list. Best-effort: failure is logged only. - tokio::spawn(lifecycle::sync_tmpfiles()); - Ok(()) -} - -/// Rerender the meta flake from whatever containers still exist on -/// disk. Called after lifecycle ops that change the agent set (today: -/// destroy). Idempotent — a no-op when nothing changed. -async fn sync_meta_after_lifecycle(coord: &Coordinator) -> Result<()> { - let agents = lifecycle::agents_for_meta_listing().await?; - crate::meta::sync_agents(&coord.hive_env(), &agents).await + coord.emit_rebuild_queue_snapshot(); } pub fn deny(coord: &Coordinator, id: i64, note: Option<&str>) -> Result<()> { diff --git a/hive-c0re/src/coordinator.rs b/hive-c0re/src/coordinator.rs index 8c227a16..7be394d5 100644 --- a/hive-c0re/src/coordinator.rs +++ b/hive-c0re/src/coordinator.rs @@ -116,13 +116,6 @@ pub struct Coordinator { /// is never injected into containers. pub model_prices: crate::hive_stats::PriceTable, agents: Mutex>, - /// Agents whose lifecycle action (currently just spawn) is in flight. - /// Read by the dashboard to render a spinner; cleared when the action - /// resolves (success or failure). - /// Agents whose container is being taken down by work with **no queue node - /// behind it** (destroy, migration), so the crash watcher must not report - /// the disappearance as a crash. Not a pill — see [`CrashWatchSuppression`]. - crash_suppressed: Mutex>, /// Tombstone for transients that have JUST been cleared. The /// crash watcher polls every 10s and would race the /// drop-clears-immediately path of `TransientGuard`: an operator @@ -400,57 +393,6 @@ fn fold_tombstones_by_agent<'a>( out } -/// Tombstone label for work with **no queue node behind it** — the -/// out-of-band operations (destroy, migration) that hold a -/// [`CrashWatchGuard`] instead of appearing in the derived transient set. -/// -/// [`Coordinator::recent_transient`] is keyed by `(agent, label)` so concurrent -/// pills can't overwrite each other's `deliberate_stop`; a guard has no node and -/// therefore no node label, so it needs one of its own. Angle-bracketed to keep -/// it out of the `NodeKind::as_str` namespace — no node can ever render this. -const NO_NODE_LABEL: &str = ""; - -/// RAII handle returned by [`Coordinator::suppress_crash_watch`]. While held, -/// the crash watcher treats this container disappearing as **expected**. -/// -/// This is *not* a dashboard pill. Transients are derived from running queue -/// nodes and nothing stores them. But destroy and migration take a container -/// down without a node behind them, so nothing in the graph says the -/// disappearance was intended — and without that, `crash_watch` fires a -/// `ContainerCrash` for every destroy and every migrated agent, and the manager -/// tries to "recover" containers that were removed on purpose. -/// -/// It is held rather than stamped once because -/// [`crate::workers::crash_watch`]'s grace window is finite and these -/// operations are not: a long destroy would outlive a single tombstone. The -/// tombstone is stamped on drop, covering the poll that lands just after. -/// -/// Goes away entirely once destroy + migration are real queue nodes. -#[must_use = "suppression lasts as long as the guard; bind it for the operation's duration \ - (`let _guard = coord.suppress_crash_watch(...)`). An unbound call drops it \ - immediately and the very next poll can report a deliberate stop as a crash."] -pub struct CrashWatchSuppression { - coord: Arc, - name: String, -} - -impl Drop for CrashWatchSuppression { - fn drop(&mut self) { - self.coord - .crash_suppressed - .lock() - .unwrap() - .remove(&self.name); - // Tombstone the release so the next poll — which may land in the - // window between the container going away and this guard dropping — - // still reads the stop as deliberate. - self.coord.recent_transient.lock().unwrap().insert( - (self.name.clone(), NO_NODE_LABEL.to_owned()), - (true, std::time::Instant::now()), - ); - } -} - /// RAII guard for the `meta-update` in-progress flag, held for the /// duration of a `run_meta_update` background task. Created by /// `Coordinator::meta_update_guard`. Drop decrements the active-run @@ -586,7 +528,6 @@ impl Coordinator { agent_io_weight, model_prices, agents: Mutex::new(HashMap::new()), - crash_suppressed: Mutex::new(HashSet::new()), recent_transient: Mutex::new(HashMap::new()), recent_crashes: Mutex::new(HashMap::new()), graceful_stop_pending: Mutex::new(HashSet::new()), @@ -1300,35 +1241,6 @@ impl Coordinator { map.iter().map(|(k, v)| (k.clone(), v.len())).collect() } - /// Tell the crash watcher that `name`'s container is going down **on - /// purpose**, for the lifetime of the returned guard. See - /// [`CrashWatchSuppression`] for why this exists at all. - /// - /// Only for the operations with no queue node behind them. Anything the - /// job queue runs answers this from the node itself - /// ([`crate::job_queue::NodeKind::takes_container_down`]) and must not come - /// through here. - /// - /// The guard's `Drop` runs even on task cancellation, so an aborted HTTP - /// request or a panic mid-destroy can't leave a container permanently - /// exempt from crash reporting. - pub fn suppress_crash_watch(self: &Arc, name: &str) -> CrashWatchSuppression { - self.crash_suppressed - .lock() - .unwrap() - .insert(name.to_owned()); - CrashWatchSuppression { - coord: self.clone(), - name: name.to_owned(), - } - } - - /// Whether a no-node operation is currently taking this container down. - #[must_use] - pub fn crash_watch_suppressed(&self, name: &str) -> bool { - self.crash_suppressed.lock().unwrap().contains(name) - } - /// Every live transient, keyed by agent. /// /// **Derived on read, stored nowhere.** Straight off the running graph, so diff --git a/hive-c0re/src/dashboard/lifecycle_ops.rs b/hive-c0re/src/dashboard/lifecycle_ops.rs index e12723fa..855c2830 100644 --- a/hive-c0re/src/dashboard/lifecycle_ops.rs +++ b/hive-c0re/src/dashboard/lifecycle_ops.rs @@ -436,10 +436,9 @@ pub(super) struct DestroyForm { params(("name" = String, Path, description = "agent name")), request_body(content = DestroyForm, content_type = "application/x-www-form-urlencoded"), responses( - (status = 200, description = "destroyed", body = String), + (status = 200, description = "destroy queued", body = String), (status = 400, description = "bad agent name"), (status = 404, description = "no such agent"), - (status = 500, description = "destroy failed"), ), tag = "lifecycle_ops" )] @@ -453,11 +452,10 @@ pub(super) async fn post_destroy( } // Checkbox semantics: any non-empty value (axum sends "on") = purge. let purge = form.purge.as_deref().is_some_and(|v| !v.is_empty()); - // `actions::destroy` rescans the container list on success, so the - // `ContainerRemoved` event lands before we return 200. The matching - // form carries `data-no-refresh`. - match actions::destroy(&state.coord, &name, purge).await { - Ok(()) => (StatusCode::OK, "ok").into_response(), - Err(e) => error_response(&format!("destroy {name} failed: {e:#}")), - } + // Submit-and-return, like every other lifecycle endpoint here. The + // container rescan now runs in the DAG's bookkeeping tail, so + // `ContainerRemoved` arrives *after* this 200 rather than before it — the + // row disappears when the event lands, same as a rebuild's does. + actions::destroy(&state.coord, &name, purge); + (StatusCode::OK, "ok").into_response() } diff --git a/hive-c0re/src/job_queue/exec.rs b/hive-c0re/src/job_queue/exec.rs index d392bd78..9efcf81c 100644 --- a/hive-c0re/src/job_queue/exec.rs +++ b/hive-c0re/src/job_queue/exec.rs @@ -73,6 +73,15 @@ pub(super) async fn run_node( NodeKind::RebuildBookkeeping { .. } => run_rebuild_bookkeeping(coord, agent).await, NodeKind::Provision { .. } => run_provision(coord, agent).await, NodeKind::Create { .. } => run_create(agent).await, + NodeKind::DestroyContainer { .. } => run_destroy_container(coord, agent).await, + NodeKind::PurgeState { .. } => { + run_purge_state(agent).await; + Ok(()) + } + NodeKind::DestroyBookkeeping { purge, .. } => { + run_destroy_bookkeeping(coord, agent, *purge).await; + Ok(()) + } NodeKind::MetaLock { sweep, fanout, @@ -163,6 +172,112 @@ async fn run_resolve_approval( Ok(()) } +/// Rerender the meta flake from whatever containers still exist on disk. +/// Idempotent — a no-op when nothing changed. Lives here because the destroy +/// tail is its only caller; it moved with `destroy` when that became a DAG. +async fn sync_meta_after_lifecycle(coord: &Coordinator) -> Result<()> { + let agents = crate::lifecycle::agents_for_meta_listing().await?; + crate::meta::sync_agents(&coord.hive_env(), &agents).await +} + +/// `nixos-container destroy`, then drop the agent from the roster and clear +/// its ephemeral runtime dir (the mcp socket, which does not survive a restart +/// anyway). +/// +/// 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. +async fn run_destroy_container(coord: &Arc, agent: &str) -> Result<()> { + crate::lifecycle::destroy(agent).await?; + coord.unregister_agent(agent); + let runtime = crate::paths::agent_runtime_dir(agent); + if runtime.exists() { + let _ = std::fs::remove_dir_all(&runtime); + } + Ok(()) +} + +/// The `purge = true` half: wipe the agent's persistent trees. +/// +/// Every step is best-effort-with-a-warning rather than fatal, and that is +/// deliberate — the container is already destroyed by the time this runs, so +/// failing the node would leave the operator with a half-purged agent and a red +/// DAG, when what they can actually act on is the log line naming the path. +async fn run_purge_state(agent: &str) { + // The state root may be a btrfs subvolume: a subvolume root can't be + // removed with rmdir/`remove_dir_all`, so delete it via hive-priv (root) + // first. No-op for plain-dir agents — the loop below then handles the + // plain-dir state root plus the applied dir. + if let Err(e) = crate::priv_client::delete_agent_subvolume(agent).await { + tracing::warn!(error = ?e, %agent, "purge: delete state subvolume failed"); + } + // A malformed name can't have a persistent state tree (the state dir is + // only ever created under a validated Ident), so its removal is a no-op — + // skip the state-dir sweep and just clear the applied dir. + let state_dir = hive_types::Ident::parse(agent) + .ok() + .map(|id| crate::paths::agent_state_dir(&id)); + for dir in state_dir + .into_iter() + .chain([crate::paths::applied_dir(agent)]) + { + if dir.exists() + && let Err(e) = std::fs::remove_dir_all(&dir) + { + tracing::warn!(error = ?e, dir = %dir.display(), "purge: remove failed"); + } + } +} + +/// Post-destroy bookkeeping. Infallible by construction: every step is +/// warn-and-continue, because the destroy it follows has already succeeded and +/// none of this is undoable — a failed meta sync or power-store write is a +/// bookkeeping drift to log, not a reason to red a DAG whose container is +/// already gone. +async fn run_destroy_bookkeeping(coord: &Arc, agent: &str, purge: bool) { + // Meta flake: drop the agent's input + nixosConfiguration so a future spawn + // under the same name re-seeds cleanly, and so the meta lock doesn't + // reference a vanished applied repo. + if let Err(e) = sync_meta_after_lifecycle(coord).await { + tracing::warn!(error = ?e, %agent, "meta sync after destroy failed"); + } + let _ = coord.approvals.fail_pending_for_agent( + agent, + if purge { + "agent purged" + } else { + "agent destroyed" + }, + ); + // Drop the durable power intent — a future agent of the same name seeds + // fresh from its observed state. + if let Err(e) = coord.power.remove(agent) { + tracing::warn!(%agent, error = ?e, "agent_power: remove on destroy failed"); + } + let _ = coord + .push_todo( + hive_sh4re::manager::MANAGER_AGENT, + "core", + Some(format!("destroyed:{agent}")), + format!("agent '{agent}' destroyed"), + None, + false, + ) + .await; + // Container row disappeared — rescan so the dashboard fires + // `ContainerRemoved` for the gone row, then emit the tombstones snapshot + // (gained one on destroy, lost one on purge — recompute either way). + coord.rescan_containers_and_emit().await; + crate::dashboard::emit_tombstones_snapshot(coord).await; + // Re-emit the schedules snapshot: the rescan above refreshed the live + // roster, so any schedule that still targets the just-destroyed agent now + // drops that ghost column live (no page reload needed). + coord.emit_schedules_snapshot(); + // Update tmpfiles.d to remove the destroyed agent's dirs from the boot-time + // pre-creation list. Best-effort: failure is logged only. + tokio::spawn(crate::lifecycle::sync_tmpfiles()); +} + /// Emit this agent's rebuild-complete todo. `ok` is not computed — it is which /// of the tail pair the graph let run. The failure note comes from the DAG's /// first failing node, since the branch knows *that* it failed but not *why*. diff --git a/hive-c0re/src/job_queue/model.rs b/hive-c0re/src/job_queue/model.rs index 2f5e4573..be77e688 100644 --- a/hive-c0re/src/job_queue/model.rs +++ b/hive-c0re/src/job_queue/model.rs @@ -78,6 +78,36 @@ pub enum NodeKind { /// First-spawn `nixos-container create` proper. Assumes the /// upstream `Provision` node already registered the agent in meta. Create { agent: String }, + /// `nixos-container destroy` plus the un-registration that follows it: + /// drop the agent from the coordinator's roster and clear its ephemeral + /// runtime dir. + /// + /// **Deliberately not in [`NodeKind::takes_container_down`]**, and that + /// is the design rather than an oversight. This node runs *downstream of + /// a `Stop`*, which already carries the flag honestly, so by the time it + /// claims there is nothing left to take down. A container still live here + /// is a real bug and must page someone — a `true` would absorb exactly + /// that signal, and the flag's whole asymmetry (see that method) is that + /// a wrong `true` silently swallows a crash. + DestroyContainer { agent: String }, + /// The `purge = true` half of a destroy: delete the agent's state + /// subvolume (via hive-priv, since a subvolume root defeats + /// `remove_dir_all`) plus its state and applied dirs. Its own node + /// because it is conditional — a plain destroy never inserts it — and + /// because it is the irreversible step, so it earns a distinct row in + /// the graph rather than hiding inside a bookkeeping tail. + PurgeState { agent: String }, + /// The post-destroy bookkeeping tail: meta sync, fail the agent's pending + /// approvals, drop the durable power intent, notify the manager, rescan + /// containers, re-emit the tombstone + schedule snapshots, resync + /// tmpfiles. Split from [`NodeKind::DestroyContainer`] for the same + /// reason [`NodeKind::RebuildBookkeeping`] is split from `Swap`: + /// dashboard visibility and retry granularity for work that is pure + /// store/meta bookkeeping and touches no container. + /// + /// `purge` only selects the wording of the approval-failure reason and + /// the manager notification; the destructive work is `PurgeState`'s. + DestroyBookkeeping { agent: String, purge: bool }, /// Meta flake lock bump. `sweep = false`: `meta::lock_update` /// (commit fused, under `META_LOCK`) with this node's own `inputs`; /// `sweep = true`: `meta::lock_update_hyperhive`, *non-fatal* (a @@ -341,6 +371,9 @@ impl NodeKind { NodeKind::RebuildBookkeeping { .. } => "rebuild_bookkeeping", NodeKind::Provision { .. } => "provision", NodeKind::Create { .. } => "create", + NodeKind::DestroyContainer { .. } => "destroy_container", + NodeKind::PurgeState { .. } => "purge_state", + NodeKind::DestroyBookkeeping { .. } => "destroy_bookkeeping", NodeKind::MetaLock { .. } => "meta_lock", NodeKind::Reconcile { .. } => "reconcile", NodeKind::Start { .. } => "start", @@ -378,6 +411,9 @@ impl NodeKind { | NodeKind::RebuildBookkeeping { agent } | NodeKind::Provision { agent } | NodeKind::Create { agent } + | NodeKind::DestroyContainer { agent } + | NodeKind::PurgeState { agent } + | NodeKind::DestroyBookkeeping { agent, .. } | NodeKind::Reconcile { agent } | NodeKind::Start { agent } | NodeKind::Stop { agent } @@ -435,6 +471,12 @@ impl NodeKind { // - `Create` / `Start` / `SetWanted{up}` bring a container UP. A // container disappearing *while starting* is a genuine crash and has // to keep reporting as one. + // - `DestroyContainer` looks like the most obvious `true` on this list + // and is the one that must stay `false`. It is edged downstream of a + // `Stop`, so the container is already down when it claims; the stop + // that the operator asked for is accounted for by the node that + // performs it. A container found alive at destroy time is a genuine + // bug, and a `true` here would suppress the alert that says so. // - `Reconcile` is a planner; it fans out `Start` / `Stop`, which carry // their own answer. // - `DeployWindow` brackets a deploy without itself stopping anything. diff --git a/hive-c0re/src/job_queue/templates.rs b/hive-c0re/src/job_queue/templates.rs index ee67e388..94d7366a 100644 --- a/hive-c0re/src/job_queue/templates.rs +++ b/hive-c0re/src/job_queue/templates.rs @@ -474,6 +474,59 @@ pub fn spawn(builder: &JobBuilder, agent: &str, approval_id: i64) { resolve_approval_tails(builder, approval_id, provision); } +/// Teardown: `Stop` → `DestroyContainer` → (`PurgeState`) → `DestroyBookkeeping`. +/// +/// The chain is the point, not a decomposition for its own sake. Destroy used +/// to be a straight-line async fn with no queue node behind it, so nothing in +/// the graph could answer "is this container going down on purpose?" — which is +/// why an imperative crash-watch suppression guard existed at all. Reusing the +/// existing [`NodeKind::Stop`] answers it structurally: `Stop` already declares +/// `takes_container_down`, so the suppression is derived from the graph like +/// every other lifecycle op's. +/// +/// That also makes the precondition an edge rather than an assertion. +/// `DestroyContainer` runs only after `Stop` succeeded, so it operates on an +/// already-stopped container and carries `takes_container_down = false` +/// permanently — a container still alive at that point is a real bug and stays +/// loud instead of being absorbed by a flag. +/// +/// `Stop` is idempotent against an already-down container, so the common +/// "destroy something that isn't running" path costs nothing extra. +/// +/// `Stop` is the group root and holds the agent lease for the whole teardown; +/// the rest are `part_of` children that borrow it, so no other op can interleave +/// with a half-destroyed agent. `PurgeState` is inserted only when asked for — +/// the graph shows the irreversible step as its own row when it happens, and +/// omits it entirely when it doesn't. +pub fn destroy(builder: &JobBuilder, agent: &str, purge: bool) { + let a = || agent.to_owned(); + let stop = builder + .node(NodeKind::Stop { agent: a() }) + .needs(Resource::Agent(a())); + // `part_of` IS the ordering: a child runs once its parent reaches + // `Finishing`, and a node may not also declare a dep on its own parent + // (dep-scope validation rejects it — it would deadlock). So the + // "container is already stopped" precondition is the group edge itself, + // with no explicit `after_ok(stop)` to add. + let destroy = builder + .node(NodeKind::DestroyContainer { agent: a() }) + .part_of(stop); + // The bookkeeping tail hangs off the purge when there is one, so the + // irreversible delete lands before the meta sync that stops referencing it. + let last = if purge { + builder + .node(NodeKind::PurgeState { agent: a() }) + .part_of(stop) + .after_ok(destroy) + } else { + destroy + }; + let _tail = builder + .node(NodeKind::DestroyBookkeeping { agent: a(), purge }) + .part_of(stop) + .after_ok(last); +} + /// Perm change: commit the JSON file(s), then the rebuild subgraph so /// the updated `HIVE_TOOL_GROUPS` / `HIVE_CAPABILITIES` env var takes /// effect in the container. Group-roots are `WritePermFile` plus the rebuild diff --git a/hive-c0re/src/job_queue/tests.rs b/hive-c0re/src/job_queue/tests.rs index 4ac0a3f8..4e27357a 100644 --- a/hive-c0re/src/job_queue/tests.rs +++ b/hive-c0re/src/job_queue/tests.rs @@ -1714,6 +1714,97 @@ fn spawn_shape_provision_create_dropin_reconcile() { ); } +/// The destroy chain, and the reason it is a chain: `Stop` is reused so the +/// crash-watch answer comes from the node that actually stops the container. +/// +/// Asserting the *edges* is the point. `destroy_container` runs `after_ok` a +/// `stop`, which is what makes "the container is already down here" a +/// structural fact rather than a convention — see the companion test below for +/// why that matters. +#[test] +fn destroy_shape_stop_then_destroy_then_bookkeeping() { + let q = JobQueue::new(1); + insert(&q, |builder| { + templates::destroy(builder, "doomed", false); + }); + assert_eq!( + declared_shape(&q), + vec![ + row("stop", None, &[]), + // No explicit edge to `stop`: `part_of` already gates the child on + // its parent reaching `Finishing`, and declaring a dep on your own + // parent is rejected outright (it would deadlock). The precondition + // is the group membership. + row("destroy_container", Some("stop"), &[]), + row( + "destroy_bookkeeping", + Some("stop"), + &[("destroy_container", "done")] + ), + ] + ); +} + +/// `purge` inserts the irreversible delete as its own node, between the destroy +/// and the bookkeeping tail — so the meta sync that stops referencing the agent +/// runs *after* its trees are actually gone, and a purge is visibly distinct +/// from a plain destroy on the graph instead of being a hidden boolean. +#[test] +fn destroy_shape_purge_inserts_purge_state_before_the_tail() { + let q = JobQueue::new(1); + insert(&q, |builder| { + templates::destroy(builder, "doomed", true); + }); + assert_eq!( + declared_shape(&q), + vec![ + row("stop", None, &[]), + row("destroy_container", Some("stop"), &[]), + row( + "purge_state", + Some("stop"), + &[("destroy_container", "done")] + ), + row( + "destroy_bookkeeping", + Some("stop"), + &[("purge_state", "done")] + ), + ] + ); +} + +/// The counter-case to `rebuild_chain_nodes_suppress_crash_watch`, and the one +/// assertion in this file that exists to stop a *plausible* edit rather than a +/// wrong one. +/// +/// `destroy_container` is the most obvious candidate for `takes_container_down` +/// on the whole list and must stay `false`. It is edged downstream of a `Stop` +/// that already carries the flag, so the intentional stop is already accounted +/// for; a container still alive when this node claims is a genuine bug. Since a +/// wrong `true` **silently swallows a real crash** while a wrong `false` only +/// costs a spurious event, this is the asymmetry that has to be pinned. +#[test] +fn destroy_container_must_not_suppress_crash_watch() { + assert!( + !NodeKind::DestroyContainer { + agent: "a".to_owned() + } + .takes_container_down(), + "destroy_container runs after a Stop that already declared the \ + container is going down; claiming it again would suppress the alert \ + for a container found unexpectedly alive" + ); + // The upstream node is where the `true` lives — assert it here too, so the + // pair reads as one property and moving the flag breaks this test. + assert!( + NodeKind::Stop { + agent: "a".to_owned() + } + .takes_container_down() + ); +} + #[test] fn perm_change_shape_prefixes_rebuild_chain() { let q = JobQueue::new(1); diff --git a/hive-c0re/src/server.rs b/hive-c0re/src/server.rs index eedb54d7..753e67c3 100644 --- a/hive-c0re/src/server.rs +++ b/hive-c0re/src/server.rs @@ -144,7 +144,7 @@ async fn dispatch(req: &HostRequest, coord: Arc) -> HostResponse { handle_start(&coord, &agents, &infra).await? } HostRequest::Destroy { name, purge } => { - actions::destroy(&coord, name.as_str(), *purge).await?; + actions::destroy(&coord, name.as_str(), *purge); HostResponse::success() } HostRequest::Rebuild { name } => { diff --git a/hive-c0re/src/workers/crash_watch.rs b/hive-c0re/src/workers/crash_watch.rs index 6ee46982..cc15e6f3 100644 --- a/hive-c0re/src/workers/crash_watch.rs +++ b/hive-c0re/src/workers/crash_watch.rs @@ -88,17 +88,17 @@ fn emit_crash_transitions(coord: &Coordinator, prev: &HashSet, current: // guard between two crash-watch polls. let recent = coord.recent_transient_within(RECENT_TRANSIENT_GRACE); for stopped in prev.difference(current) { - // Two sources, because a container can go down on purpose either way: - // a running queue node that declared it takes the container down, or a - // no-node operation (destroy, migration) holding a suppression guard. + // One source: a running queue node that declared it takes the container + // down. There used to be a second — an imperative suppression guard for + // operations with no node behind them — and destroy becoming a DAG + // removed the last caller, so intent now has exactly one home. // `any`, not "the" pill: an agent can have several running nodes at // once (a lease-exempt build alongside a lease-holding stop), and it // only takes one of them expecting the container down for this to be // a deliberate stop rather than a crash. let active = transients .get(stopped) - .map(|sts| sts.iter().any(|st| st.takes_container_down)) - .or_else(|| coord.crash_watch_suppressed(stopped).then_some(true)); + .map(|sts| sts.iter().any(|st| st.takes_container_down)); let recently_cleared = recent.get(stopped).copied(); if is_deliberate_stop(active, recently_cleared) { continue;