From 7589f4c06c3296ca433f840be1d1b79269cf552f Mon Sep 17 00:00:00 2001 From: atlas Date: Sun, 26 Jul 2026 16:05:13 +0200 Subject: [PATCH] job_queue: stop reverting power intent on a cancelled restart MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `terminal_hook` mapped `Restart` / `GracefulRestart` to `RevertIntent`, but a restart never writes `wanted` — `restart_chain` deliberately has no `SetWanted` head, so the tail `Reconcile` converges to the agent's existing intent and a deliberately-stopped agent isn't forced up by a hive-wide restart. `revert_intent` writes `Wanted::from_running(observed)` unconditionally on a cancelled DAG. So for an agent that is `wanted = Up` but currently down (crashed, or caught behind another queued op), submitting a restart and then cancelling it writes `wanted = Offline` — reverting an intent the DAG never touched, to a value nobody asked for. Reconcile and crash-watch both then read the agent as deliberately-stopped and leave it down. It's invisible for a running agent, since `from_running(true)` equals the intent already on file, which is why it went unnoticed. `cancel` only succeeds while every node is still `Pending`, so the reachable window is exactly "queued restart + observed != intent" — precisely when someone restarts and then thinks better of it. Drop both restart templates from the `RevertIntent` arm; they fall through to no terminal hook, which is correct for a DAG that writes no intent. Document the invariant on `HookKind::RevertIntent` and on `revert_intent` itself: the hook writes *observed* state, so dispatching it for a template with no `SetWanted` head doesn't restore an intent, it invents one. Test covers all four restart shapes (graceful x running), asserting both that the spec carries no `SetWanted` and that a cancelled restart dispatches no hook, with a contrast arm pinning stop's revert in place. Fixes hyperhive/hyperhive#2710 --- hive-c0re/src/job_queue/exec.rs | 5 ++++ hive-c0re/src/job_queue/mod.rs | 18 ++++++++----- hive-c0re/src/job_queue/tests.rs | 45 ++++++++++++++++++++++++++++++++ 3 files changed, 62 insertions(+), 6 deletions(-) diff --git a/hive-c0re/src/job_queue/exec.rs b/hive-c0re/src/job_queue/exec.rs index 2b8f55e3..28a0f668 100644 --- a/hive-c0re/src/job_queue/exec.rs +++ b/hive-c0re/src/job_queue/exec.rs @@ -145,6 +145,11 @@ fn emit_rebuilt(coord: &Arc, terminal: &super::TerminalDag) { /// intent to its observed state — the operator's cancel means "don't do it", so /// the intent snaps back instead of the flip executing as a surprise side effect /// of some later reconcile. Noop on any non-cancelled outcome. +/// +/// Only valid for templates carrying a `SetWanted` head (start / stop): the +/// revert writes *observed* state, so dispatching it for a template that never +/// wrote an intent doesn't restore anything — it invents one. See +/// [`super::terminal_hook`]. async fn revert_intent(coord: &Arc, terminal: &super::TerminalDag) { if terminal.state != State::Cancelled { return; diff --git a/hive-c0re/src/job_queue/mod.rs b/hive-c0re/src/job_queue/mod.rs index 877ca89a..17b8c7c0 100644 --- a/hive-c0re/src/job_queue/mod.rs +++ b/hive-c0re/src/job_queue/mod.rs @@ -169,12 +169,22 @@ pub enum HookKind { ResolveApproval, /// Rebuild / perm-change: emit one `Rebuilt` manager event per agent. EmitRebuilt, - /// Power-op: on a *cancelled* DAG, revert each agent's `wanted` intent. + /// Intent-writing power-op: on a *cancelled* DAG, revert each agent's + /// `wanted` intent. Only for templates that actually carry a `SetWanted` + /// head — reverting an intent a DAG never wrote invents one. RevertIntent, } /// The terminal hook a DAG needs, from its template + approval id — or `None` /// for a DAG with no terminal side effect (meta-update, boot, bare reconcile). +/// +/// Restart is deliberately **not** a `RevertIntent` template. `restart_chain` +/// writes no `SetWanted` (it bounces the container and lets the tail +/// `Reconcile` converge to the agent's existing intent), so there is nothing +/// for a cancel to revert — and `revert_intent` writes the *observed* state, +/// which for a down-but-`wanted = Up` agent (crashed, or mid-bounce) would +/// flip it to `Offline` and keep it down. Cancelling a restart must leave the +/// intent exactly as it was found. #[must_use] pub fn terminal_hook(template: Template, approval_id: Option) -> Option { if approval_id.is_some() { @@ -182,11 +192,7 @@ pub fn terminal_hook(template: Template, approval_id: Option) -> Option Some(HookKind::EmitRebuilt), - Template::Start - | Template::Stop - | Template::GracefulStop - | Template::Restart - | Template::GracefulRestart => Some(HookKind::RevertIntent), + Template::Start | Template::Stop | Template::GracefulStop => Some(HookKind::RevertIntent), _ => None, } } diff --git a/hive-c0re/src/job_queue/tests.rs b/hive-c0re/src/job_queue/tests.rs index 1327515a..f0de064e 100644 --- a/hive-c0re/src/job_queue/tests.rs +++ b/hive-c0re/src/job_queue/tests.rs @@ -863,6 +863,51 @@ fn cancel_refuses_running_dag() { assert_eq!(state_of(&q, id), State::Running); } +/// A cancelled restart must fire **no** intent revert. `restart_chain` writes +/// no `SetWanted`, so a cancel has nothing to roll back — and the revert hook +/// writes the agent's *observed* state, which for a down-but-`wanted = Up` +/// agent (crashed, or caught mid-bounce) would flip the intent to `Offline` +/// and leave it deliberately-stopped as far as reconcile and crash-watch are +/// concerned. Invisible for a running agent (observed == recorded), which is +/// why it went unnoticed; the window is exactly "observed ≠ intent", which is +/// when a restart is most likely to be issued and then cancelled. +#[test] +fn cancelled_restart_reverts_no_intent() { + for graceful in [false, true] { + for running in [false, true] { + let q = JobQueue::new(1); + let targets = vec![("agent-a".to_owned(), running)]; + let spec = + submit::restart_spec(&targets, graceful, Source::Manual, "bounce".to_owned()); + assert!( + !spec + .nodes + .iter() + .any(|n| matches!(n.kind, NodeKind::SetWanted { .. })), + "restart writes no intent (graceful={graceful}, running={running})" + ); + let id = submit(&q, spec); + let summary = q.cancel(id).expect("cancelled while queued"); + assert_eq!(summary.state, State::Cancelled); + assert_eq!( + terminal_hook(summary.template, summary.approval_id), + None, + "cancelled restart (graceful={graceful}, running={running}) must \ + not revert an intent it never wrote" + ); + } + } + // Contrast: stop *does* carry a `SetWanted` head, so its cancel still has a + // real intent flip to undo. + let q = JobQueue::new(1); + let id = submit(&q, stop_online(&["agent-a"], false, "stop")); + let summary = q.cancel(id).expect("cancelled while queued"); + assert_eq!( + terminal_hook(summary.template, summary.approval_id), + Some(HookKind::RevertIntent) + ); +} + // ---- terminal reporting + lease release ---- #[test]