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
This commit is contained in:
parent
3239526f98
commit
7589f4c06c
3 changed files with 62 additions and 6 deletions
|
|
@ -145,6 +145,11 @@ fn emit_rebuilt(coord: &Arc<Coordinator>, 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<Coordinator>, terminal: &super::TerminalDag) {
|
||||
if terminal.state != State::Cancelled {
|
||||
return;
|
||||
|
|
|
|||
|
|
@ -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<i64>) -> Option<HookKind> {
|
||||
if approval_id.is_some() {
|
||||
|
|
@ -182,11 +192,7 @@ 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),
|
||||
Template::Start | Template::Stop | Template::GracefulStop => Some(HookKind::RevertIntent),
|
||||
_ => None,
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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]
|
||||
|
|
|
|||
Loading…
Reference in a new issue