Compare commits

...
Author SHA1 Message Date
atlas
5c4a637941 job_queue: delete the cancelled-power-op intent revert
The revert hook is dead by construction, so it can only ever be wrong.

DAG state `Cancelled` has exactly one producer: `JobQueue::cancel`, which
refuses unless every work node is still `Pending`. A cancel *cascade*
(some node failed, downstream cancelled) rolls up `Failed` instead —
`dag_rollup` short-circuits on any failed subtree node. So on a DAG that
reaches `Cancelled`, no node ever executed: the `SetWanted` head provably
never ran and `wanted` still reads whatever the operator last set it to.

There is therefore nothing to revert, and `revert_intent` did not revert
anything — it wrote `Wanted::from_running(observed)`, i.e. the agent's
*observed* state, over an intent the DAG never touched. Harmless when
observed already matched, silent corruption otherwise: cancel a queued
start for an agent that is down but `wanted = Up` (crashed, or caught
mid-bounce) and the intent flips to `Offline`, leaving it
deliberately-stopped as far as reconcile and crash-watch are concerned.

The hook made sense when `set_wanted` was a pre-submit side effect
written before the DAG ran; moving it into the DAG as a node left the
hook vestigial.

Drop `HookKind::RevertIntent`, `revert_intent`, and the power-op arm of
`terminal_hook` — start / stop / graceful-stop now settle with no
terminal hook, same as restart always did. The test asserts the general
statement across restart/stop/start x graceful x running: stop and start
carry a `SetWanted` head, and cancelling them still fires no hook.
2026-07-26 16:30:26 +02:00
atlas
7589f4c06c job_queue: stop reverting power intent on a cancelled restart
`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
2026-07-26 16:30:26 +02:00
4 changed files with 73 additions and 32 deletions

View file

@ -112,7 +112,6 @@ pub(crate) async fn run_terminal_hook(coord: &Arc<Coordinator>, terminal: &super
crate::actions::resolve_approval_dag(coord, terminal).await;
}
Some(super::HookKind::EmitRebuilt) => emit_rebuilt(coord, terminal),
Some(super::HookKind::RevertIntent) => revert_intent(coord, terminal).await,
None => {}
}
}
@ -141,25 +140,6 @@ fn emit_rebuilt(coord: &Arc<Coordinator>, terminal: &super::TerminalDag) {
}
}
/// Power-op hook: on a *cancelled* DAG, revert each targeted agent's `wanted`
/// 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.
async fn revert_intent(coord: &Arc<Coordinator>, terminal: &super::TerminalDag) {
if terminal.state != State::Cancelled {
return;
}
for agent in &terminal.agents {
let running = crate::lifecycle::is_running(agent).await;
if let Err(e) = coord
.power
.set(agent, crate::power::Wanted::from_running(running))
{
tracing::warn!(%agent, error = ?e, "agent_power: cancel revert failed");
}
}
}
/// Write the agent's durable power intent — the DAG-node form of the old
/// pre-submit `set_wanted` side effect. Store-only (no container touch), so
/// build-slot-exempt; but it takes the agent's lifecycle lease (see

View file

@ -169,12 +169,18 @@ 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.
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).
/// for a DAG with no terminal side effect (power-op, meta-update, boot, bare
/// reconcile).
///
/// A cancelled DAG deliberately gets **no** compensating hook. [`JobQueue::cancel`]
/// refuses unless every work node is still `Pending`, and a cancel *cascade*
/// rolls up `Failed` (see `dag_rollup`), never `Cancelled` — so on a
/// `Cancelled` DAG no node ever executed and there is nothing to undo. A power
/// op's `SetWanted` head provably never ran, so its intent is still whatever
/// the operator last set it to.
#[must_use]
pub fn terminal_hook(template: Template, approval_id: Option<i64>) -> Option<HookKind> {
if approval_id.is_some() {
@ -182,11 +188,6 @@ pub fn terminal_hook(template: Template, approval_id: Option<i64>) -> Option<Hoo
}
match template {
Template::Rebuild | Template::PermChange => Some(HookKind::EmitRebuilt),
Template::Start
| Template::Stop
| Template::GracefulStop
| Template::Restart
| Template::GracefulRestart => Some(HookKind::RevertIntent),
_ => None,
}
}

View file

@ -10,10 +10,10 @@
//! held, so it appears when the agent's owner node starts and disappears when
//! its subgraph settles — one pill per agent a DAG touches.
//!
//! Per-DAG terminal work (approval resolution, `Rebuilt`, cancelled-power-op
//! intent revert) is not drained here: it runs as the DAG's focused terminal
//! node (`ResolveApproval` / `EmitRebuilt` / `RevertIntent`), dispatched through
//! `exec::run_node` like any other node once the DAG settles.
//! Per-DAG terminal work (approval resolution, `Rebuilt`) is not drained here:
//! it runs as the DAG's focused terminal node (`ResolveApproval` /
//! `EmitRebuilt`), dispatched through `exec::run_node` like any other node once
//! the DAG settles.
//!
//! In-DAG growth (a `MetaLock` growing rebuild subgraphs, a `Reconcile` fanning
//! its `Start`/`Stop`) flows through `NodeOutput.append_subgraph`, applied

View file

@ -863,6 +863,66 @@ fn cancel_refuses_running_dag() {
assert_eq!(state_of(&q, id), State::Running);
}
/// A cancelled power op must fire **no** compensating hook — not even one that
/// carries a `SetWanted` head.
///
/// `cancel` refuses unless every work node is still `Pending`
/// (`cancel_refuses_running_dag`) and a cancel *cascade* rolls up `Failed`
/// rather than `Cancelled`, so a `Cancelled` DAG provably never executed a
/// node: its `SetWanted` never ran and the agent's intent still reads whatever
/// the operator last set. A "revert" instead writes the agent's *observed*
/// state, which for a down-but-`wanted = Up` agent (crashed, or caught
/// mid-bounce) flips the intent to `Offline` and leaves it
/// deliberately-stopped as far as reconcile and crash-watch are concerned.
#[test]
fn cancelled_power_op_fires_no_hook() {
for graceful in [false, true] {
for running in [false, true] {
let targets = vec![("agent-a".to_owned(), running)];
let cases = [
(
"restart",
false,
submit::restart_spec(&targets, graceful, Source::Manual, "bounce".to_owned()),
),
(
"stop",
true,
submit::stop_spec(&targets, graceful, Source::Manual, "stop".to_owned()),
),
(
"start",
true,
submit::start_spec(
&[("agent-a".to_owned(), running, false)],
Source::Manual,
"start".to_owned(),
),
),
];
for (name, writes_intent, spec) in cases {
assert_eq!(
spec.nodes
.iter()
.any(|n| matches!(n.kind, NodeKind::SetWanted { .. })),
writes_intent,
"{name} intent head (graceful={graceful}, running={running})"
);
let q = JobQueue::new(1);
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 {name} (graceful={graceful}, running={running}) must \
fire no hook no node of it ever ran"
);
}
}
}
}
// ---- terminal reporting + lease release ----
#[test]