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.
This commit is contained in:
parent
7589f4c06c
commit
5c4a637941
4 changed files with 64 additions and 79 deletions
|
|
@ -112,7 +112,6 @@ pub(crate) async fn run_terminal_hook(coord: &Arc<Coordinator>, terminal: &super
|
||||||
crate::actions::resolve_approval_dag(coord, terminal).await;
|
crate::actions::resolve_approval_dag(coord, terminal).await;
|
||||||
}
|
}
|
||||||
Some(super::HookKind::EmitRebuilt) => emit_rebuilt(coord, terminal),
|
Some(super::HookKind::EmitRebuilt) => emit_rebuilt(coord, terminal),
|
||||||
Some(super::HookKind::RevertIntent) => revert_intent(coord, terminal).await,
|
|
||||||
None => {}
|
None => {}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
@ -141,30 +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.
|
|
||||||
///
|
|
||||||
/// 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<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
|
/// 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
|
/// pre-submit `set_wanted` side effect. Store-only (no container touch), so
|
||||||
/// build-slot-exempt; but it takes the agent's lifecycle lease (see
|
/// build-slot-exempt; but it takes the agent's lifecycle lease (see
|
||||||
|
|
|
||||||
|
|
@ -169,22 +169,18 @@ pub enum HookKind {
|
||||||
ResolveApproval,
|
ResolveApproval,
|
||||||
/// Rebuild / perm-change: emit one `Rebuilt` manager event per agent.
|
/// Rebuild / perm-change: emit one `Rebuilt` manager event per agent.
|
||||||
EmitRebuilt,
|
EmitRebuilt,
|
||||||
/// 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`
|
/// 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).
|
||||||
///
|
///
|
||||||
/// Restart is deliberately **not** a `RevertIntent` template. `restart_chain`
|
/// A cancelled DAG deliberately gets **no** compensating hook. [`JobQueue::cancel`]
|
||||||
/// writes no `SetWanted` (it bounces the container and lets the tail
|
/// refuses unless every work node is still `Pending`, and a cancel *cascade*
|
||||||
/// `Reconcile` converge to the agent's existing intent), so there is nothing
|
/// rolls up `Failed` (see `dag_rollup`), never `Cancelled` — so on a
|
||||||
/// for a cancel to revert — and `revert_intent` writes the *observed* state,
|
/// `Cancelled` DAG no node ever executed and there is nothing to undo. A power
|
||||||
/// which for a down-but-`wanted = Up` agent (crashed, or mid-bounce) would
|
/// op's `SetWanted` head provably never ran, so its intent is still whatever
|
||||||
/// flip it to `Offline` and keep it down. Cancelling a restart must leave the
|
/// the operator last set it to.
|
||||||
/// intent exactly as it was found.
|
|
||||||
#[must_use]
|
#[must_use]
|
||||||
pub fn terminal_hook(template: Template, approval_id: Option<i64>) -> Option<HookKind> {
|
pub fn terminal_hook(template: Template, approval_id: Option<i64>) -> Option<HookKind> {
|
||||||
if approval_id.is_some() {
|
if approval_id.is_some() {
|
||||||
|
|
@ -192,7 +188,6 @@ pub fn terminal_hook(template: Template, approval_id: Option<i64>) -> Option<Hoo
|
||||||
}
|
}
|
||||||
match template {
|
match template {
|
||||||
Template::Rebuild | Template::PermChange => Some(HookKind::EmitRebuilt),
|
Template::Rebuild | Template::PermChange => Some(HookKind::EmitRebuilt),
|
||||||
Template::Start | Template::Stop | Template::GracefulStop => Some(HookKind::RevertIntent),
|
|
||||||
_ => None,
|
_ => None,
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
|
||||||
|
|
@ -10,10 +10,10 @@
|
||||||
//! held, so it appears when the agent's owner node starts and disappears when
|
//! held, so it appears when the agent's owner node starts and disappears when
|
||||||
//! its subgraph settles — one pill per agent a DAG touches.
|
//! its subgraph settles — one pill per agent a DAG touches.
|
||||||
//!
|
//!
|
||||||
//! Per-DAG terminal work (approval resolution, `Rebuilt`, cancelled-power-op
|
//! Per-DAG terminal work (approval resolution, `Rebuilt`) is not drained here:
|
||||||
//! intent revert) is not drained here: it runs as the DAG's focused terminal
|
//! it runs as the DAG's focused terminal node (`ResolveApproval` /
|
||||||
//! node (`ResolveApproval` / `EmitRebuilt` / `RevertIntent`), dispatched through
|
//! `EmitRebuilt`), dispatched through `exec::run_node` like any other node once
|
||||||
//! `exec::run_node` like any other node once the DAG settles.
|
//! the DAG settles.
|
||||||
//!
|
//!
|
||||||
//! In-DAG growth (a `MetaLock` growing rebuild subgraphs, a `Reconcile` fanning
|
//! In-DAG growth (a `MetaLock` growing rebuild subgraphs, a `Reconcile` fanning
|
||||||
//! its `Start`/`Stop`) flows through `NodeOutput.append_subgraph`, applied
|
//! its `Start`/`Stop`) flows through `NodeOutput.append_subgraph`, applied
|
||||||
|
|
|
||||||
|
|
@ -863,49 +863,64 @@ fn cancel_refuses_running_dag() {
|
||||||
assert_eq!(state_of(&q, id), State::Running);
|
assert_eq!(state_of(&q, id), State::Running);
|
||||||
}
|
}
|
||||||
|
|
||||||
/// A cancelled restart must fire **no** intent revert. `restart_chain` writes
|
/// A cancelled power op must fire **no** compensating hook — not even one that
|
||||||
/// no `SetWanted`, so a cancel has nothing to roll back — and the revert hook
|
/// carries a `SetWanted` head.
|
||||||
/// 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`
|
/// `cancel` refuses unless every work node is still `Pending`
|
||||||
/// and leave it deliberately-stopped as far as reconcile and crash-watch are
|
/// (`cancel_refuses_running_dag`) and a cancel *cascade* rolls up `Failed`
|
||||||
/// concerned. Invisible for a running agent (observed == recorded), which is
|
/// rather than `Cancelled`, so a `Cancelled` DAG provably never executed a
|
||||||
/// why it went unnoticed; the window is exactly "observed ≠ intent", which is
|
/// node: its `SetWanted` never ran and the agent's intent still reads whatever
|
||||||
/// when a restart is most likely to be issued and then cancelled.
|
/// 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]
|
#[test]
|
||||||
fn cancelled_restart_reverts_no_intent() {
|
fn cancelled_power_op_fires_no_hook() {
|
||||||
for graceful in [false, true] {
|
for graceful in [false, true] {
|
||||||
for running in [false, true] {
|
for running in [false, true] {
|
||||||
let q = JobQueue::new(1);
|
|
||||||
let targets = vec![("agent-a".to_owned(), running)];
|
let targets = vec![("agent-a".to_owned(), running)];
|
||||||
let spec =
|
let cases = [
|
||||||
submit::restart_spec(&targets, graceful, Source::Manual, "bounce".to_owned());
|
(
|
||||||
assert!(
|
"restart",
|
||||||
!spec
|
false,
|
||||||
.nodes
|
submit::restart_spec(&targets, graceful, Source::Manual, "bounce".to_owned()),
|
||||||
.iter()
|
),
|
||||||
.any(|n| matches!(n.kind, NodeKind::SetWanted { .. })),
|
(
|
||||||
"restart writes no intent (graceful={graceful}, running={running})"
|
"stop",
|
||||||
);
|
true,
|
||||||
let id = submit(&q, spec);
|
submit::stop_spec(&targets, graceful, Source::Manual, "stop".to_owned()),
|
||||||
let summary = q.cancel(id).expect("cancelled while queued");
|
),
|
||||||
assert_eq!(summary.state, State::Cancelled);
|
(
|
||||||
assert_eq!(
|
"start",
|
||||||
terminal_hook(summary.template, summary.approval_id),
|
true,
|
||||||
None,
|
submit::start_spec(
|
||||||
"cancelled restart (graceful={graceful}, running={running}) must \
|
&[("agent-a".to_owned(), running, false)],
|
||||||
not revert an intent it never wrote"
|
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"
|
||||||
|
);
|
||||||
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
// 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 ----
|
// ---- terminal reporting + lease release ----
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue