From 07078b76eff1f279f4f8f19643bec5a59d9a1bb5 Mon Sep 17 00:00:00 2001 From: atlas Date: Mon, 27 Jul 2026 16:48:14 +0200 Subject: [PATCH] feat(#2772): branch on outcome in the graph, not inside the node MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Splits what was one `Cancelled` outcome into two, because they were two different facts wearing one name: - `Skipped` — the node's own edges ruled it out. Expected; the failure branch of a run that succeeded is `Skipped`. A parent's roll-up **ignores** it. - `Cancelled` — the work was dropped before it could start. Still not-success for the roll-up, as before. Without that split, branching on outcome defeats itself: exactly one branch is always ruled out, `any_child_failed` counted it, and every DAG containing a branch would have rolled up failed no matter how the run went. Caught in review before it was written, not after. `AFTER_ANY` becomes `{Done, Failed, Skipped}` — "anything except the work being dropped". That is what it always meant; it only swept in cancellation because cancellation wasn't distinguishable from elimination. Audited every user rather than assuming, which is how the one regression in my own proposal surfaced: `{Done, Failed}` would have refused to run rebuild's recovery `Reconcile` after a failed `MetaSync` (that eliminates `Prebuild`, so the tail's dep is `Skipped`, not `Failed`) and left the container down. With that, the templates stop computing outcomes and let the graph pick: - `ResolveApproval { approval_id, outcome }` — one tail per outcome, each edged to accept only its own, so exactly one is ever runnable. - `EmitRebuilt { agent, ok }` — a pair. `ok` is not derived, it is which of the two the graph let run. Edges are conjunctive, so "any of these roots failed" is not directly sayable. The composition: the success branch is `AFTER_OK` on every root (so it is itself eliminated the moment one doesn't succeed), and the failure branch keys off *that* elimination. The failure branch also waits on every root — without it, a failed `Prebuild` eliminates the success branch immediately and the failure would be announced while the recovery `Reconcile` was still running. The tests caught that one. Deletes, all of them #2770's host-side debt: - `Claim.deps`, `DepOutcome`, `Claim::deps_state`, `Claim::deps_error` and the dep-snapshotting loop in `claim_ready`. Executors read their own variant now; nothing inspects anything. - `NodeKind::is_tail()` and the `cancel` exemption built on it. Sparing is derived from the edges: `cancel` keeps a node iff one of its edges accepts `Cancelled`. An approval tail names it and survives to resolve the row; `Reconcile` doesn't and is cancelled with the rest. My earlier claim that this couldn't dissolve was only true while `AFTER_ANY` accepted cancellation. `resolve_approval_dag` / `deploy_terminal_tag` now take `TerminalState` rather than the wire `State`, so both matches are exhaustive instead of ending in a catch-all. Skipped nodes are filtered off the wire alongside `Done` ones. That costs some dashboard detail on a failed rebuild — which steps were skipped — and the tests say so with a pointer to the follow-up. Surfacing them as `Cancelled` instead would be worse: the client roll-up ranks `Cancelled` above `Running`, so a successful DAG with a not-taken branch would read as cancelled. --- hive-c0re/src/actions.rs | 39 +++--- hive-c0re/src/job_queue/exec.rs | 80 +++++------- hive-c0re/src/job_queue/mod.rs | 131 ++++++------------- hive-c0re/src/job_queue/model.rs | 42 +++--- hive-c0re/src/job_queue/templates.rs | 150 ++++++++++++++++----- hive-c0re/src/job_queue/tests.rs | 187 +++++++++------------------ hive-jobq/src/lib.rs | 54 ++++++-- hive-jobq/src/scheduler.rs | 67 ++++++---- 8 files changed, 365 insertions(+), 385 deletions(-) diff --git a/hive-c0re/src/actions.rs b/hive-c0re/src/actions.rs index d00d9227..4dadeace 100644 --- a/hive-c0re/src/actions.rs +++ b/hive-c0re/src/actions.rs @@ -507,20 +507,18 @@ async fn run_approval_schedule_prompt( /// so the work's terminal state is the authoritative outcome and there's no /// in-node resolution to skip around. /// -/// `state` / `error` come from the tail node's own dependency roll-up -/// ([`Claim::deps_state`] / [`Claim::deps_error`]), so this runs on the success, -/// failure **and cancel** paths alike. +/// `outcome` is the one the calling node was built to report — a template emits +/// one tail per outcome, so success, failure and cancel each arrive here from +/// their own node rather than from one node branching. /// /// [`NodeKind::ResolveApproval`]: crate::job_queue::NodeKind::ResolveApproval -/// [`Claim::deps_state`]: crate::job_queue::Claim::deps_state -/// [`Claim::deps_error`]: crate::job_queue::Claim::deps_error pub(crate) async fn resolve_approval_dag( coord: &Arc, approval_id: i64, - state: crate::job_queue::State, + outcome: crate::job_queue::TerminalState, error: Option<&str>, ) { - use crate::job_queue::State; + use crate::job_queue::TerminalState; let approval = match coord.approvals.get(approval_id) { Ok(Some(a)) => a, Ok(None) => { @@ -532,10 +530,14 @@ pub(crate) async fn resolve_approval_dag( return; } }; - let result: Result<()> = match state { - State::Done => Ok(()), - State::Cancelled => Err(anyhow::anyhow!("cancelled before completion")), - _ => Err(anyhow::anyhow!("{}", error.unwrap_or("job dag failed"))), + let result: Result<()> = match outcome { + TerminalState::Done => Ok(()), + // `Skipped` never reaches here — no template emits a tail for it — but it + // reads the same to an operator either way: the work did not happen. + TerminalState::Cancelled | TerminalState::Skipped => { + Err(anyhow::anyhow!("cancelled before completion")) + } + TerminalState::Failed => Err(anyhow::anyhow!("{}", error.unwrap_or("job dag failed"))), }; let mut terminal_tag = None; match approval.kind { @@ -551,7 +553,7 @@ pub(crate) async fn resolve_approval_dag( } } ApprovalKind::MergeConfigPr => { - terminal_tag = deploy_terminal_tag(approval.agent.as_str(), approval_id, state).await; + terminal_tag = deploy_terminal_tag(approval.agent.as_str(), approval_id, outcome).await; // On a failed deploy, surface the failing build log back onto the // PR so the manager sees why it was rejected without leaving the // forge. Posted here rather than inside a node because this is the @@ -577,13 +579,14 @@ pub(crate) async fn resolve_approval_dag( async fn deploy_terminal_tag( agent: &str, approval_id: i64, - state: crate::job_queue::State, + outcome: crate::job_queue::TerminalState, ) -> Option { - use crate::job_queue::State; - let candidate = match state { - State::Done => format!("deployed/{approval_id}"), - State::Cancelled => return None, - _ => format!("failed/{approval_id}"), + use crate::job_queue::TerminalState; + let candidate = match outcome { + TerminalState::Done => format!("deployed/{approval_id}"), + // Nothing ran, so nothing was planted. + TerminalState::Cancelled | TerminalState::Skipped => return None, + TerminalState::Failed => format!("failed/{approval_id}"), }; lifecycle::git_rev_parse(&crate::paths::applied_dir(agent), &candidate) .await diff --git a/hive-c0re/src/job_queue/exec.rs b/hive-c0re/src/job_queue/exec.rs index 21ea0728..51de0607 100644 --- a/hive-c0re/src/job_queue/exec.rs +++ b/hive-c0re/src/job_queue/exec.rs @@ -11,7 +11,7 @@ use std::sync::Arc; use anyhow::{Context as _, Result}; use super::Claim; -use super::model::{NodeKind, NodeSpec, State}; +use super::model::{NodeKind, NodeSpec, TerminalState}; use crate::coordinator::Coordinator; use crate::power::{ReconcileAction, reconcile_action}; @@ -95,10 +95,11 @@ pub(super) async fn run_node(coord: &Arc, claim: &Claim) -> Result< NodeKind::DeployApply { .. } => run_deploy_apply(coord, claim).await, NodeKind::FinalizeDeploy { .. } => run_finalize_deploy(coord, claim).await, NodeKind::DeployTail { .. } => run_deploy_tail(coord, claim).await, - NodeKind::ResolveApproval { approval_id, .. } => { - run_resolve_approval(coord, claim, *approval_id).await - } - NodeKind::EmitRebuilt { .. } => Ok(run_emit_rebuilt(coord, claim)), + NodeKind::ResolveApproval { + approval_id, + outcome, + } => run_resolve_approval(coord, claim, *approval_id, *outcome).await, + NodeKind::EmitRebuilt { ok, .. } => Ok(run_emit_rebuilt(coord, claim, *ok)), NodeKind::SetWanted { up, .. } => run_set_wanted(coord, claim, *up), // Pure grouping container — no work; completing it lets it reach // `Finishing` so its child work nodes start. The DAG's terminal side @@ -107,58 +108,39 @@ pub(super) async fn run_node(coord: &Arc, claim: &Claim) -> Result< } } -/// Resolve the DAG's approval row from how the work it follows ended. The -/// outcome comes off this node's own dependency roll-up, not from re-reading -/// the world. Best-effort: a resolution failure is logged inside -/// [`crate::actions::resolve_approval_dag`], never surfaced as a node failure — -/// the work already happened, and failing the tail would only misreport it. +/// Resolve the DAG's approval row the way this node's own `outcome` says. +/// +/// Nothing is inspected: a template emits one of these per outcome, each edged to +/// accept only that one, so *which* node the scheduler let run already is the +/// answer. Best-effort — a resolution failure is logged inside +/// [`crate::actions::resolve_approval_dag`], never surfaced as a node failure, +/// since the work already happened and failing the tail would only misreport it. async fn run_resolve_approval( coord: &Arc, claim: &Claim, approval_id: i64, + outcome: TerminalState, ) -> Result { - let reason = failure_reason(coord, claim); - crate::actions::resolve_approval_dag(coord, approval_id, claim.deps_state(), reason.as_deref()) - .await; + let reason = (outcome == TerminalState::Failed) + .then(|| coord.job_queue.first_error(claim.dag_id)) + .flatten(); + crate::actions::resolve_approval_dag(coord, approval_id, outcome, reason.as_deref()).await; Ok(NodeOutput::default()) } -/// Why the work a tail node follows failed, as a human-readable string. -/// -/// Prefers the tail's own dependency error, but a dep that is a **group root** -/// rolled up `Failed` from a child carries no error of its own (the reason lives -/// on the leaf that actually failed) — and a grafted subgraph's nodes can't be -/// edged statically anyway. So fall back to the DAG's first failing node. Still -/// the queue's own graph, not the outside world. -fn failure_reason(coord: &Arc, claim: &Claim) -> Option { - claim - .deps_error() - .map(str::to_owned) - .or_else(|| coord.job_queue.first_error(claim.dag_id)) -} - -/// Emit this agent's `Rebuilt` manager event — `ok` when the work it follows is -/// `Done`, `!ok` with the failure note when it `Failed`, and nothing at all when -/// it `Cancelled` (nothing ran, so there is no rebuild to report). -fn run_emit_rebuilt(coord: &Arc, claim: &Claim) -> NodeOutput { - let agent = claim.agent.clone(); - match claim.deps_state() { - State::Done => coord.notify_manager(&hive_sh4re::HelperEvent::Rebuilt { - agent, - ok: true, - note: None, - sha: None, - tag: None, - }), - State::Failed => coord.notify_manager(&hive_sh4re::HelperEvent::Rebuilt { - agent, - ok: false, - note: failure_reason(coord, claim), - sha: None, - tag: None, - }), - _ => {} - } +/// Emit this agent's `Rebuilt` manager event. `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*. +fn run_emit_rebuilt(coord: &Arc, claim: &Claim, ok: bool) -> NodeOutput { + coord.notify_manager(&hive_sh4re::HelperEvent::Rebuilt { + agent: claim.agent.clone(), + ok, + note: (!ok) + .then(|| coord.job_queue.first_error(claim.dag_id)) + .flatten(), + sha: None, + tag: None, + }); NodeOutput::default() } diff --git a/hive-c0re/src/job_queue/mod.rs b/hive-c0re/src/job_queue/mod.rs index 680d8f74..9bb9335a 100644 --- a/hive-c0re/src/job_queue/mod.rs +++ b/hive-c0re/src/job_queue/mod.rs @@ -49,7 +49,7 @@ use hive_sh4re::wire_time::now_unix; use tokio::sync::Notify; use crate::coordinator::TransientKind; -pub use model::{DagSpec, DagView, NodeKind, NodeSpec, PermPayload, Source, State}; +pub use model::{DagSpec, DagView, NodeKind, NodeSpec, PermPayload, Source, State, TerminalState}; use resource::Resource; /// How many terminal DAGs (`Done` / `Failed` / `Cancelled`) the snapshot @@ -60,31 +60,6 @@ const MAX_HISTORY_DAGS: usize = 50; /// Cap on stored node error strings. const MAX_ERROR_LEN: usize = 2_000; -/// How one of a claimed node's dependencies finished, snapshotted at claim time. -/// -/// A node only starts once its edges are satisfied, so every dep named here is -/// already terminal: an `AfterOk` edge means [`State::Done`], an `AfterAny` edge -/// means any of `Done` / `Failed` / `Cancelled`. -/// -/// This is what lets a tail node be an ordinary node. A tail that must report -/// how the work below it went — "emit `Rebuilt { ok }`", "resolve this approval -/// with the failure note" — reads its deps' outcomes off its own claim, rather -/// than re-deriving them from the world after the fact. The alternative in this -/// codebase is `DeployTail`, which infers success by re-reading *git state*; that -/// works only because a deploy happens to write its result somewhere durable, and -/// it is not a pattern to copy. -/// -/// Carries no node id: a tail acts on *how* its dependencies ended, never on -/// which one it was, so an id here would be a field with no reader. -#[derive(Debug, Clone)] -pub struct DepOutcome { - /// Its terminal state, in wire terms. - pub state: State, - /// Its failure reason, when it failed with one of its own. A node that - /// rolled up `Failed` from a child, or was cancelled, carries no error. - pub error: Option, -} - /// A node claimed for execution — everything the executor needs, snapshotted at /// claim time. #[derive(Debug, Clone)] @@ -101,38 +76,6 @@ pub struct Claim { /// pill is currently shown is derived from live lease ownership /// ([`JobQueue::held_transients`]), not a per-claim edge. pub transient: Option, - /// How each node this one depends on finished. Empty for a head node. - /// See [`DepOutcome`] — this is how a tail node learns the outcome of the - /// work it follows without going back to the world to ask. - pub deps: Vec, -} - -impl Claim { - /// Roll this claim's dependencies up into one outcome — what a weak-edged - /// tail node acts on. `Done` only when every dep succeeded; `Failed` when any - /// failed; otherwise `Cancelled` (all terminal, none failed, so the work was - /// dropped before it ran). - /// - /// A head node has no deps and rolls up `Done` — vacuously true, and never - /// reached in practice since only tail kinds consult this. - pub fn deps_state(&self) -> State { - if self.deps.iter().all(|d| d.state == State::Done) { - State::Done - } else if self.deps.iter().any(|d| d.state == State::Failed) { - State::Failed - } else { - State::Cancelled - } - } - - /// The first failed dependency's error, for reporting *why* the work ended - /// badly. `None` when nothing failed. - pub fn deps_error(&self) -> Option<&str> { - self.deps - .iter() - .find(|d| d.state == State::Failed) - .and_then(|d| d.error.as_deref()) - } } /// Per-node runtime metadata the crate graph doesn't carry. Lifecycle @@ -192,13 +135,19 @@ impl Default for JobQueue { /// Map a crate node state onto the wire state (`Pending` ↔ `Queued`; /// `Finishing` — own logic done, sub-nodes still running — reads as `Running`). +/// +/// `Skipped` has no wire counterpart and folds into `Cancelled`: to a reader +/// both mean "this never ran". The distinction is a *scheduling* one — it +/// decides whether a parent's roll-up counts the node — and the wire carries no +/// roll-up input, only display state. In practice a client never sees it either: +/// `dag_view` drops skipped nodes from the snapshot along with `Done` ones. fn to_wire_state(state: JobState) -> State { match state { JobState::Pending => State::Queued, JobState::Running | JobState::Finishing => State::Running, JobState::Done => State::Done, JobState::Failed => State::Failed, - JobState::Cancelled => State::Cancelled, + JobState::Cancelled | JobState::Skipped => State::Cancelled, } } @@ -365,24 +314,6 @@ impl JobQueue { }; let kind = node.payload.clone(); let agent = node.payload.agent().to_owned(); - let dep_ids: Vec = node - .deps - .iter() - .filter_map(|d| match d { - Dep::Node { id, .. } => Some(*id), - Dep::Resource { .. } => None, - }) - .collect(); - let deps: Vec = dep_ids - .into_iter() - .filter_map(|dep| { - let n = inner.sched.graph().node(dep)?; - Some(DepOutcome { - state: to_wire_state(n.state), - error: n.error.clone(), - }) - }) - .collect(); let Some(container) = inner.dag_of(id) else { continue; }; @@ -397,7 +328,6 @@ impl JobQueue { approval_id: meta.approval_id, inputs: meta.inputs, transient: meta.transient, - deps, }); // `started_at` is stamped on the graph `Node` by the scheduler's // transition to `Running` — no host-side copy needed. @@ -430,17 +360,17 @@ impl JobQueue { /// so each is cancelled. `false` once any work node is running or terminal — /// an in-flight nix build isn't interruptible. /// - /// **Tail nodes are spared** ([`NodeKind::is_tail`]). They are weak-edged - /// (`AfterAny`), and a `Cancelled` dep satisfies a weak edge, so sparing one - /// leaves it *ready* rather than stranded: the scheduler claims it on the next - /// pass, its [`Claim::deps_state`] reads `Cancelled`, and it resolves the - /// approval as "cancelled before completion". That is what stops a queued - /// approval DAG the operator cancelled from dangling its approval forever — - /// the job the inline hook used to do from outside the graph. + /// **Nodes that explicitly observe cancellation are spared** — a node whose + /// edge names [`hive_jobq::TerminalState::Cancelled`] is asking to run when + /// the work it follows was dropped, which is exactly what an approval tail + /// needs: cancel the work, and the tail still fires to resolve the approval + /// row rather than leaving it dangling forever. /// - /// Cancelling the tail too would be the bug: `cancel_node` only cascades along - /// `AfterOk` edges and parent links, so nothing else would reach it, and the - /// approval row would simply never be touched. + /// Nothing is special-cased by node kind. `AFTER_ANY` deliberately does *not* + /// accept `Cancelled`, so an ordinary weak-edged step (rebuild's `Reconcile`, + /// say) is cancelled along with everything else — there is nothing to converge + /// when no node ever ran. Only a node that named `Cancelled` survives, and it + /// survives because it asked to. pub fn cancel(&self, dag_id: u64) -> bool { let mut inner = self.lock(); let Some(container) = inner.container(dag_id) else { @@ -458,12 +388,7 @@ impl JobQueue { return false; } for id in work { - if inner - .sched - .graph() - .node(id) - .is_some_and(|n| n.payload.is_tail()) - { + if inner.observes_cancellation(id) { continue; } inner.sched.cancel_node(id); @@ -642,6 +567,18 @@ impl QueueInner { .is_some_and(|n| n.state.is_terminal()) } + /// Whether `id` has an edge that accepts a **dropped** dependency — i.e. the + /// node exists to report on work that may never run. Used by + /// [`JobQueue::cancel`] to decide what to spare, so the decision comes from + /// the node's own declared edges rather than a hardcoded list of kinds. + fn observes_cancellation(&self, id: NodeId) -> bool { + self.sched.graph().node(id).is_some_and(|n| { + n.deps.iter().any(|d| { + matches!(d, Dep::Node { when, .. } if when.accepts(hive_jobq::TerminalState::Cancelled)) + }) + }) + } + /// First failed work node's error (read off the graph `Node`), for the /// dashboard's DAG-level error line. fn dag_first_error(&self, container: NodeId) -> Option { @@ -682,7 +619,11 @@ impl QueueInner { if let Some(f) = node.finished_at { finished.push(f); } - if node.state == JobState::Done { + // `Done` nodes drop off the wire (a finished step isn't interesting), + // and so do `Skipped` ones: a branch that was never taken is noise on + // the dashboard, and surfacing it would also drag the client-side + // roll-up toward `Cancelled` for a run that went fine. + if matches!(node.state, JobState::Done | JobState::Skipped) { continue; } let deps: Vec = node diff --git a/hive-c0re/src/job_queue/model.rs b/hive-c0re/src/job_queue/model.rs index df49007d..d6f75b3c 100644 --- a/hive-c0re/src/job_queue/model.rs +++ b/hive-c0re/src/job_queue/model.rs @@ -20,8 +20,8 @@ use crate::coordinator::TransientKind; /// When a dependency edge is satisfied — re-exported from [`hive_jobq`] rather /// than mirrored here. It used to be a duplicate enum with a `to_crate_when` /// translation beside it; the copy bought nothing and had to be widened in -/// lockstep every time the crate's edge model grew (#2772). -pub use hive_jobq::DepWhen; +/// lockstep every time the crate's edge model grew. +pub use hive_jobq::{DepWhen, TerminalState}; /// A dependency edge (intra-DAG only — cross-DAG ordering comes from /// the per-agent lease + dedup, never from edges between DAGs). @@ -233,9 +233,16 @@ pub enum NodeKind { /// carrying one here would be a second copy free to drift. Like /// [`NodeKind::MetaLock`] it reports `""` from [`NodeKind::agent`] and takes /// no lease — which is also what lets one close a multi-agent DAG. - /// - /// [`Claim::deps`]: super::Claim::deps - ResolveApproval { approval_id: i64 }, + ResolveApproval { + approval_id: i64, + /// Which outcome this node reports. A template emits **one per outcome**, + /// each edged to accept only that one, so exactly one is ever runnable + /// and the executor has nothing to decide — it resolves the row the way + /// its own variant says. The `Cancelled` one is also the node that + /// [`super::JobQueue::cancel`] spares, since its edge is the only one + /// that accepts a dropped dependency. + outcome: TerminalState, + }, /// Tail node of a rebuild / perm-change: emit this agent's `Rebuilt` manager /// event — `ok` when its deps are `Done`, `!ok` carrying the failure note when /// they `Failed`, and **nothing at all** when they `Cancelled` (a cancelled DAG @@ -243,8 +250,13 @@ pub enum NodeKind { /// /// One node **per agent**, unlike the DAG-wide hook it replaces: a multi-agent /// DAG now reports each agent's own outcome instead of painting every agent with - /// the whole DAG's roll-up. - EmitRebuilt { agent: String }, + /// the whole DAG's roll-up. And one per *outcome* — `ok` isn't computed here, + /// it's which of the pair the graph let run. + /// + /// There is deliberately no cancel variant: a DAG dropped before it started + /// has no rebuild to report, and neither tail's edge accepts `Cancelled`, so + /// both are cancelled with the rest and nothing is emitted. + EmitRebuilt { agent: String, ok: bool }, /// Write the agent's durable power intent (`wanted = Up` when `up`, else /// `Offline`) as a first-class DAG node, at the head of a power-op /// template so the downstream `Reconcile` reads it. Replaces the old @@ -333,7 +345,7 @@ impl NodeKind { | NodeKind::DeployApply { agent } | NodeKind::FinalizeDeploy { agent } | NodeKind::DeployTail { agent } - | NodeKind::EmitRebuilt { agent } + | NodeKind::EmitRebuilt { agent, .. } | NodeKind::SetWanted { agent, .. } => agent, NodeKind::MetaLock { .. } | NodeKind::Reparent { .. } @@ -342,20 +354,6 @@ impl NodeKind { } } - /// Whether this is a DAG's **tail** — a node that reports how the rest of the - /// DAG ended rather than doing work of its own. - /// - /// The one place this matters is [`super::JobQueue::cancel`], which spares - /// tails so they still run (and report `Cancelled`) on a cancelled DAG. Note - /// [`NodeKind::DeployTail`] is *not* one: despite the name it does real - /// compensating work, and on a cancelled DAG there is nothing to compensate. - pub fn is_tail(&self) -> bool { - matches!( - self, - NodeKind::ResolveApproval { .. } | NodeKind::EmitRebuilt { .. } - ) - } - /// Nix-heavy kinds hold one of the `buildSlots` semaphore permits /// for the node's duration. pub fn needs_build_slot(&self) -> bool { diff --git a/hive-c0re/src/job_queue/templates.rs b/hive-c0re/src/job_queue/templates.rs index aa787fb5..f893e9a5 100644 --- a/hive-c0re/src/job_queue/templates.rs +++ b/hive-c0re/src/job_queue/templates.rs @@ -30,7 +30,7 @@ use anyhow::{Result, bail}; -use super::model::{DagSpec, Dep, DepWhen, NodeKind, NodeSpec, PermPayload, Source}; +use super::model::{DagSpec, Dep, DepWhen, NodeKind, NodeSpec, PermPayload, Source, TerminalState}; use crate::coordinator::TransientKind; /// After-ok edge on the previous node — the common chain link. Shared with @@ -43,14 +43,28 @@ pub(crate) fn after_ok(on: u64) -> Vec { }] } -/// Weak edges onto every one of a DAG's other **group-roots** — how a tail node -/// (`ResolveApproval` / `EmitRebuilt`) sees the whole DAG's outcome. +/// `AfterOk` edges onto every one of a DAG's **group-roots** — the success +/// branch of a per-outcome tail pair, and the aggregator the failure branch +/// keys off. /// /// Group-roots are the right granularity, not "every node": a root's state *is* /// its subtree's roll-up, so edging the roots covers every descendant while -/// keeping the tail's dep list small and stable as subtrees grow. `AfterAny` -/// throughout, so the tail runs on success, failure and cancel alike and decides -/// from [`super::Claim::deps_state`]. +/// keeping the dep list small and stable as subtrees grow. Because every edge is +/// `AFTER_OK`, this node runs only if *all* of them succeeded — and is ruled out +/// ([`TerminalState::Skipped`]) the moment one doesn't, which is precisely the +/// signal [`on_elimination_of`] waits for. +pub(crate) fn after_ok_all(ons: &[u64]) -> Vec { + ons.iter() + .map(|&on| Dep { + on, + when: DepWhen::AFTER_OK, + }) + .collect() +} + +/// `AFTER_ANY` edges onto every group-root — "wait for all of these to finish, +/// however they went". Ordering only; it accepts any outcome except the DAG +/// being dropped. pub(crate) fn after_any_all(ons: &[u64]) -> Vec { ons.iter() .map(|&on| Dep { @@ -60,6 +74,89 @@ pub(crate) fn after_any_all(ons: &[u64]) -> Vec { .collect() } +/// A single edge satisfied only when `on` was **ruled out** by its own edges. +/// +/// Dependency edges are conjunctive, so "any one of these several nodes failed" +/// cannot be written directly. This is the composition that expresses it: point +/// the success branch at every root with [`after_ok_all`], then hang the failure +/// branch off *that* node's elimination. Exactly one of the pair ever runs. +/// +/// Note it accepts `Skipped` and **not** `Cancelled`: if the whole DAG was +/// dropped before it started, the success branch is marked `Cancelled` directly +/// and this branch is ruled out too — a job nobody ran reports nothing. +pub(crate) fn on_elimination_of(on: u64) -> Vec { + vec![Dep { + on, + when: DepWhen::of(&[TerminalState::Skipped]), + }] +} + +/// A single edge satisfied only by the listed outcomes of `on` — for the +/// one-tail-per-outcome shape an approval DAG uses. +pub(crate) fn on_outcome(on: u64, outcomes: &[TerminalState]) -> Vec { + vec![Dep { + on, + when: DepWhen::of(outcomes), + }] +} + +/// The `Rebuilt`-reporting tail pair for a rebuild-shaped DAG: the success node +/// gated on every group-root in `roots`, and the failure node gated on *its* +/// elimination. `base` is the spec index the pair starts at. +/// +/// Exactly one runs on a DAG that executed, and neither runs on one the operator +/// dropped — see [`on_elimination_of`]. +fn emit_rebuilt_tails(agent: &str, roots: &[u64], base: u64) -> Vec { + // The failure branch needs *both*: the ok branch being ruled out (that is the + // "something went wrong" signal) **and** every root actually finished. The + // second half is easy to forget and gets the ordering wrong without it — a + // failed `Prebuild` eliminates the ok branch immediately, while the recovery + // `Reconcile` is still bringing the container back up, so reporting straight + // off the elimination would announce the failure mid-recovery. + let mut on_fail = after_any_all(roots); + on_fail.extend(on_elimination_of(base)); + vec![ + node( + NodeKind::EmitRebuilt { + agent: agent.to_owned(), + ok: true, + }, + after_ok_all(roots), + ), + node( + NodeKind::EmitRebuilt { + agent: agent.to_owned(), + ok: false, + }, + on_fail, + ), + ] +} + +/// The approval-resolving tails for an approval-carrying DAG: one per outcome of +/// the DAG's single group-root `root`, each accepting only its own. +/// +/// The `Cancelled` node is what keeps a dropped approval DAG from dangling its +/// row forever — its edge is the only one [`super::JobQueue::cancel`] spares. +fn resolve_approval_tails(approval_id: i64, root: u64) -> Vec { + [ + TerminalState::Done, + TerminalState::Failed, + TerminalState::Cancelled, + ] + .into_iter() + .map(|outcome| { + node( + NodeKind::ResolveApproval { + approval_id, + outcome, + }, + on_outcome(root, &[outcome]), + ) + }) + .collect() +} + /// Build one **top-level (group-root)** node — `parent = None`. `kind` carries /// the agent it targets ([`NodeKind`] is the payload directly). Shared with /// `submit.rs`'s dynamic power-op builders. A root owns whatever resource it @@ -198,12 +295,7 @@ pub(crate) fn deploy_rebuild_nodes(agent: &str) -> Vec { /// it reaches `Done` even after a failed swap and the tail would report success. pub fn rebuild(agent: &str, source: Source, reason: String, relock: bool) -> DagSpec { let mut nodes = rebuild_nodes(agent, relock, 0); - nodes.push(node( - NodeKind::EmitRebuilt { - agent: agent.to_owned(), - }, - after_any_all(&[0, 1, 5]), - )); + nodes.extend(emit_rebuilt_tails(agent, &[0, 1, 5], 6)); DagSpec { source, reason, @@ -259,11 +351,10 @@ pub fn approval_deploy(agent: &str, approval_id: i64, reason: String) -> DagSpec when: DepWhen::AFTER_ANY, }], ), - node( - NodeKind::ResolveApproval { approval_id }, - after_any_all(&[0]), - ), - ], + ] + .into_iter() + .chain(resolve_approval_tails(approval_id, 0)) + .collect(), } } @@ -319,11 +410,10 @@ pub fn spawn(agent: &str, approval_id: i64, reason: String) -> DagSpec { child(0, NodeKind::Create { agent: a() }, Vec::new()), child(1, NodeKind::WriteDropin { agent: a() }, Vec::new()), child(1, NodeKind::Reconcile { agent: a() }, after_ok(2)), - node( - NodeKind::ResolveApproval { approval_id }, - after_any_all(&[0]), - ), ] + .into_iter() + .chain(resolve_approval_tails(approval_id, 0)) + .collect() }, } } @@ -342,12 +432,7 @@ pub fn perm_change(agent: &str, source: Source, reason: String, payload: PermPay Vec::new(), )]; nodes.extend(rebuild_nodes(agent, true, 1)); - nodes.push(node( - NodeKind::EmitRebuilt { - agent: agent.to_owned(), - }, - after_any_all(&[0, 1, 2, 6]), - )); + nodes.extend(emit_rebuilt_tails(agent, &[0, 1, 2, 6], 7)); DagSpec { source, reason, @@ -382,14 +467,11 @@ pub fn meta_update( Vec::new(), )]; // The bump itself has no side effect, so an operator-driven one ends at the - // `MetaLock`; an approval-driven one still has its row to resolve and gets a - // tail edged onto that single group-root — whose roll-up covers the rebuild - // subgraphs `MetaLock` grows into itself. + // `MetaLock`; an approval-driven one still has its row to resolve and gets the + // per-outcome tails edged onto that single group-root — whose roll-up covers + // the rebuild subgraphs `MetaLock` grows into itself. if let Some(approval_id) = approval_id { - nodes.push(node( - NodeKind::ResolveApproval { approval_id }, - after_any_all(&[0]), - )); + nodes.extend(resolve_approval_tails(approval_id, 0)); } DagSpec { source, diff --git a/hive-c0re/src/job_queue/tests.rs b/hive-c0re/src/job_queue/tests.rs index 6cf4ed1f..a7c45229 100644 --- a/hive-c0re/src/job_queue/tests.rs +++ b/hive-c0re/src/job_queue/tests.rs @@ -48,35 +48,34 @@ fn claim_one(q: &JobQueue) -> Claim { claims.pop().expect("one claim") } -/// Claim an approval DAG's `ResolveApproval` tail, assert which approval it -/// carries and what it will report to that row, then complete it. Replaces the -/// old `terminal_summary()` assertions: the outcome is no longer a struct handed -/// to a hook, it's what this node reads off its own deps. -fn settle_approval_tail(q: &JobQueue, dag_id: u64, approval_id: i64, expect: State) { +/// Claim an approval DAG's `ResolveApproval` tail and complete it, asserting it +/// is the one built for `expect`. +/// +/// A template emits one tail per outcome and the graph runs exactly one, so the +/// assertion is on *which node was claimed* — that alone says what the approval +/// row is about to be resolved as. Nothing computes it. +fn settle_approval_tail(q: &JobQueue, dag_id: u64, approval_id: i64, expect: TerminalState) { let tail = claim_one(q); assert!( - matches!(tail.kind, NodeKind::ResolveApproval { approval_id: got } if got == approval_id), - "expected the ResolveApproval tail for #{approval_id}, got {:?}", + matches!( + tail.kind, + NodeKind::ResolveApproval { approval_id: got, outcome } + if got == approval_id && outcome == expect + ), + "expected the {expect:?} ResolveApproval tail for #{approval_id}, got {:?}", tail.kind ); - assert_eq!( - tail.deps_state(), - expect, - "outcome the tail reports to approval #{approval_id}" - ); q.complete_node(dag_id, tail.node_id, Ok(())); } -/// The `EmitRebuilt` counterpart of [`settle_approval_tail`] — claim a rebuild / -/// perm-change DAG's tail, assert the `Rebuilt` event it will emit, complete it. -fn settle_rebuild_tail(q: &JobQueue, dag_id: u64, agent: &str, expect: State) { +/// The `EmitRebuilt` counterpart of [`settle_approval_tail`] — claim the tail the +/// graph let run and assert it's the `ok` one expected. +fn settle_rebuild_tail(q: &JobQueue, dag_id: u64, agent: &str, expect_ok: bool) { let tail = claim_one(q); - assert_eq!(tail.kind.as_str(), "emit_rebuilt"); - assert_eq!(tail.agent, agent, "`Rebuilt` is emitted per agent"); - assert_eq!( - tail.deps_state(), - expect, - "ok-ness of the emitted `Rebuilt`" + assert!( + matches!(&tail.kind, NodeKind::EmitRebuilt { agent: a, ok } if a == agent && *ok == expect_ok), + "expected the ok={expect_ok} EmitRebuilt tail for {agent}, got {:?}", + tail.kind ); q.complete_node(dag_id, tail.node_id, Ok(())); } @@ -229,7 +228,7 @@ fn rebuild_chain_claims_in_dep_order() { ); q.complete_node(id, c.node_id, Ok(())); } - settle_rebuild_tail(&q, id, "agent-a", State::Done); + settle_rebuild_tail(&q, id, "agent-a", true); assert_eq!(state_of(&q, id), State::Done); } @@ -736,73 +735,6 @@ fn meta_update_carries_rebuilding_transient_and_grows_cascade_in_dag() { ); } -// ---- dep outcomes on the claim ---- - -/// Every claimed node reports how each node it depends on finished, and those -/// deps are always already terminal — that is what a node's edges being -/// satisfied *means*. A tail node reads its `deps` instead of going back to the -/// world to find out how the work below it went. -#[test] -fn claim_carries_terminal_dep_outcomes() { - let q = JobQueue::new(1); - let id = submit(&q, rebuild("agent-a", "r")); - let mut saw_a_dep = false; - loop { - let mut claims = q.claim_ready(); - let Some(claim) = claims.pop() else { break }; - assert!(claims.is_empty(), "one build slot ⇒ one claim at a time"); - for dep in &claim.deps { - saw_a_dep = true; - assert!( - dep.state.is_terminal(), - "{} was claimed with a non-terminal dep ({:?}) — a node's edges \ - being satisfied is exactly the claim that its deps have finished", - claim.kind.as_str(), - dep.state - ); - assert_eq!( - dep.state, - State::Done, - "on the happy path every dep of {} finished Done", - claim.kind.as_str() - ); - assert_eq!(dep.error, None, "a Done dep carries no error"); - } - q.complete_node(id, claim.node_id, Ok(())); - } - assert!(saw_a_dep, "the rebuild DAG has at least one dependent node"); - assert_eq!(state_of(&q, id), State::Done); -} - -/// The failure direction, which is the whole point of carrying outcomes at all: -/// `Reconcile` hangs off `Prebuild` with `AfterAny`, so a failed prebuild -/// cancel-cascades `StopForUpdate`/`Swap`/`PostSwap` and `Reconcile` still runs -/// — and its claim hands it the failure, including the reason, rather than -/// leaving the executor to go and re-derive it from the world. -#[test] -fn claim_dep_outcome_reports_a_failed_dep_with_its_error() { - let q = JobQueue::new(1); - let id = submit(&q, rebuild("agent-a", "r")); - let meta_sync = claim_one(&q); - q.complete_node(id, meta_sync.node_id, Ok(())); - let prebuild = claim_one(&q); - assert_eq!(prebuild.kind.as_str(), "prebuild"); - q.complete_node(id, prebuild.node_id, Err("nix build exploded".to_owned())); - let reconcile = claim_one(&q); - assert_eq!(reconcile.kind.as_str(), "reconcile"); - assert_eq!( - reconcile - .deps - .iter() - .map(|d| (d.state, d.error.as_deref())) - .collect::>(), - vec![(State::Failed, Some("nix build exploded"))], - "reconcile's claim carries the failed prebuild and its reason" - ); - assert_eq!(reconcile.deps_state(), State::Failed); - assert_eq!(reconcile.deps_error(), Some("nix build exploded")); -} - // ---- failure: cancel-downstream + AfterAny ---- #[test] @@ -831,9 +763,19 @@ fn failed_node_cancels_downstream_but_afterany_reconcile_runs() { .state }; assert_eq!(by_kind("prebuild"), State::Failed); - assert_eq!(by_kind("stop_for_update"), State::Cancelled); - assert_eq!(by_kind("swap"), State::Cancelled); - assert_eq!(by_kind("post_swap"), State::Cancelled); + // `StopForUpdate` / `Swap` / `PostSwap` were *ruled out* by the failed + // `Prebuild` — `Skipped`, and skipped nodes are filtered off the wire along + // with `Done` ones. The failure itself is still visible (the `prebuild` row + // above, and the roll-up), which is the part an operator acts on. + // Restoring that detail wants a real `Skipped` wire state the client renders + // as "not run" — surfacing them as `Cancelled` instead would make a + // *successful* DAG with a not-taken branch read as cancelled. + for ruled_out in ["stop_for_update", "swap", "post_swap"] { + assert!( + dag.nodes.iter().all(|n| n.kind != ruled_out), + "{ruled_out} was ruled out, so it is off the wire" + ); + } // The AfterAny reconcile ran (claimed + completed Ok above) → it's `Done`, // and `Done` nodes are excluded from the wire, so it's absent here. assert!( @@ -872,14 +814,18 @@ fn swap_failure_still_runs_reconcile() { q.complete_node(id, reconcile.node_id, Ok(())); let all_dags = q.snapshot(); let dag = all_dags.iter().find(|d| d.id == id).expect("dag"); + assert!( + dag.nodes.iter().all(|n| n.kind != "post_swap"), + "PostSwap is ruled out by the failed Swap (`Skipped`, so off the wire)" + ); assert_eq!( dag.nodes .iter() - .find(|n| n.kind == "post_swap") - .expect("post_swap node") + .find(|n| n.kind == "swap") + .expect("swap node") .state, - State::Cancelled, - "PostSwap must cancel-cascade when Swap fails" + State::Failed, + "and the failure that ruled it out is still on the wire" ); assert_eq!(state_of(&q, id), State::Failed); } @@ -911,7 +857,7 @@ fn swap_ok_runs_post_swap_before_reconcile() { let reconcile = claim_one(&q); assert_eq!(reconcile.kind.as_str(), "reconcile"); q.complete_node(id, reconcile.node_id, Ok(())); - settle_rebuild_tail(&q, id, "agent-a", State::Done); + settle_rebuild_tail(&q, id, "agent-a", true); assert_eq!(state_of(&q, id), State::Done); } @@ -939,18 +885,14 @@ fn cancel_clears_queued_dag() { // operator who just cancelled it (the dashboard renders this roll-up from // the snapshot `post_rebuild_queue_cancel` emits synchronously). assert_eq!(state_of(&q, id), State::Cancelled, "no stale Queued gap"); - // Every work node is `Cancelled`, but the tail is spared so it can still - // report the cancellation — so it is the one thing left to claim. - let tail = claim_one(&q); - assert_eq!(tail.kind.as_str(), "emit_rebuilt"); - assert_eq!( - tail.deps_state(), - State::Cancelled, - "the spared tail sees its deps cancelled, so it emits no `Rebuilt`" + // Neither `EmitRebuilt` tail accepts a *dropped* dependency — the ok one is + // `AFTER_OK`, the failure one keys on elimination — so both are cancelled + // with the work and **nothing is emitted** for a rebuild that never ran. + assert!( + q.claim_ready().is_empty(), + "a dropped rebuild reports nothing" ); - q.complete_node(id, tail.node_id, Ok(())); assert_eq!(state_of(&q, id), State::Cancelled); - assert!(q.claim_ready().is_empty()); } #[test] @@ -1065,21 +1007,10 @@ fn cancelled_dag_still_runs_its_approval_tail() { templates::approval_deploy("agent-a", 7, "approval #7".to_owned()), ); assert!(q.cancel(id), "fully-queued dag cancels"); - let tail = claim_one(&q); - assert_eq!(tail.dag_id, id); - assert_eq!(tail.kind.as_str(), "resolve_approval"); - assert!( - matches!(tail.kind, NodeKind::ResolveApproval { approval_id: 7 }), - "the tail carries the approval to resolve, got {:?}", - tail.kind - ); - assert_eq!( - tail.deps_state(), - State::Cancelled, - "so the executor resolves the approval as cancelled, not as a failure" - ); - assert_eq!(tail.deps_error(), None, "a cancelled dep carries no error"); - q.complete_node(id, tail.node_id, Ok(())); + // The `Cancelled` tail is the only node whose edge accepts a dropped + // dependency, so it is the only one `cancel` spares — and claiming it *is* + // the assertion that the approval gets resolved as cancelled. + settle_approval_tail(&q, id, 7, TerminalState::Cancelled); assert_eq!(state_of(&q, id), State::Cancelled); // Unrelated later activity doesn't disturb the settled DAG. let other = submit(&q, rebuild("agent-b", "r")); @@ -1130,7 +1061,7 @@ fn deploy_dag_runs_phases_in_order_and_tails_a_failed_apply() { ); q.complete_node(id, tail.node_id, Ok(())); - settle_approval_tail(&q, id, 7, State::Failed); + settle_approval_tail(&q, id, 7, TerminalState::Failed); assert_eq!( state_of(&q, id), State::Failed, @@ -1203,7 +1134,7 @@ fn deploy_apply_grows_rebuild_subgraph_and_finalizes_after_it() { assert!(matches!(tail.kind, NodeKind::DeployTail { .. })); q.complete_node(id, tail.node_id, Ok(())); - settle_approval_tail(&q, id, 11, State::Done); + settle_approval_tail(&q, id, 11, TerminalState::Done); assert_eq!(state_of(&q, id), State::Done); } @@ -1254,7 +1185,7 @@ fn deploy_dag_skips_finalize_but_still_tails_a_failed_graft() { ); q.complete_node(id, tail.node_id, Ok(())); - settle_approval_tail(&q, id, 13, State::Failed); + settle_approval_tail(&q, id, 13, TerminalState::Failed); assert_eq!(state_of(&q, id), State::Failed); assert_eq!( q.first_error(id).as_deref(), @@ -1292,7 +1223,7 @@ fn deploy_dag_skips_apply_but_still_runs_tail_when_verify_fails() { ); q.complete_node(id, tail.node_id, Ok(())); - settle_approval_tail(&q, id, 9, State::Failed); + settle_approval_tail(&q, id, 9, TerminalState::Failed); assert_eq!(state_of(&q, id), State::Failed); } @@ -1432,7 +1363,7 @@ fn spawn_shape_provision_create_dropin_reconcile() { assert_eq!(c.approval_id, Some(7)); q.complete_node(id, c.node_id, Ok(())); } - settle_approval_tail(&q, id, 7, State::Done); + settle_approval_tail(&q, id, 7, TerminalState::Done); assert_eq!(state_of(&q, id), State::Done); } @@ -1464,7 +1395,7 @@ fn perm_change_shape_prefixes_rebuild_chain() { assert_eq!(c.kind.as_str(), expected); q.complete_node(id, c.node_id, Ok(())); } - settle_rebuild_tail(&q, id, "agent-a", State::Done); + settle_rebuild_tail(&q, id, "agent-a", true); assert_eq!(state_of(&q, id), State::Done); } diff --git a/hive-jobq/src/lib.rs b/hive-jobq/src/lib.rs index 625847ec..83a746da 100644 --- a/hive-jobq/src/lib.rs +++ b/hive-jobq/src/lib.rs @@ -66,8 +66,18 @@ pub enum TerminalState { Done, /// Own logic failed, or a sub-node did. Failed, - /// Never ran — an edge it depended on became unsatisfiable. + /// Never ran because the work was **dropped** before it could start — the + /// caller cancelled the whole group while it was still queued. Counts as + /// not-success when a parent rolls up. Cancelled, + /// Never ran because its own **edges ruled it out**: a dependency settled on + /// an outcome the edge doesn't accept. Expected, not a problem — the failure + /// branch of a run that succeeded is `Skipped`. + /// + /// A parent's roll-up **ignores** `Skipped` children entirely. Without that, + /// branching on outcome would be self-defeating: exactly one branch is always + /// ruled out, so every group containing one would roll up failed. + Skipped, } impl TerminalState { @@ -77,6 +87,7 @@ impl TerminalState { TerminalState::Done => 1, TerminalState::Failed => 1 << 1, TerminalState::Cancelled => 1 << 2, + TerminalState::Skipped => 1 << 3, } } } @@ -90,6 +101,7 @@ impl State { State::Done => Some(TerminalState::Done), State::Failed => Some(TerminalState::Failed), State::Cancelled => Some(TerminalState::Cancelled), + State::Skipped => Some(TerminalState::Skipped), State::Pending | State::Running | State::Finishing => None, } } @@ -119,11 +131,17 @@ impl DepWhen { /// cancelled down the chain — e.g. a failed `Prebuild` must not let /// `StopForUpdate` stop a healthy container. pub const AFTER_OK: Self = Self(TerminalState::Done.bit()); - /// The dependency need only be terminal — any outcome satisfies. For steps - /// that must converge regardless, e.g. `Reconcile` running even when the - /// preceding `Swap` failed, or a tail node that reports how the work ended. + /// Anything **except the work being dropped** — `Done`, `Failed` or + /// `Skipped`. For steps that must converge regardless of how the run went, + /// e.g. `Reconcile` bringing a container back up even when the preceding + /// `Swap` failed *or* was itself ruled out by a failed `MetaSync`. + /// + /// Deliberately excludes [`TerminalState::Cancelled`]: if the group never + /// started at all there is nothing to converge, and running the recovery + /// step anyway would act on work that provably never happened. A node that + /// must report a cancellation names `Cancelled` explicitly. pub const AFTER_ANY: Self = Self( - TerminalState::Done.bit() | TerminalState::Failed.bit() | TerminalState::Cancelled.bit(), + TerminalState::Done.bit() | TerminalState::Failed.bit() | TerminalState::Skipped.bit(), ); /// An edge satisfied by exactly the listed outcomes. @@ -204,18 +222,24 @@ pub enum State { Done, /// Completed unsuccessfully — own logic failed, or a sub-node did. Failed, - /// Never ran: an `AfterOk` dependency failed, so this node (and the rest of - /// its strong-dependent chain) is cancelled rather than run. + /// Never ran: the work was dropped while still queued. See + /// [`TerminalState::Cancelled`]. Cancelled, + /// Never ran: its own edges ruled it out. See [`TerminalState::Skipped`] — + /// notably, a parent's roll-up ignores these. + Skipped, } impl State { /// A node is *terminal* once it has finished — successfully, unsuccessfully, - /// or cancelled — which is when its resources are released and dependents - /// are re-evaluated. + /// dropped, or ruled out — which is when its resources are released and + /// dependents are re-evaluated. #[must_use] pub fn is_terminal(self) -> bool { - matches!(self, State::Done | State::Failed | State::Cancelled) + matches!( + self, + State::Done | State::Failed | State::Cancelled | State::Skipped + ) } } @@ -590,10 +614,16 @@ mod tests { assert!(!DepWhen::AFTER_OK.satisfied_by(State::Failed)); assert!(!DepWhen::AFTER_OK.satisfied_by(State::Cancelled)); assert!(!DepWhen::AFTER_OK.satisfied_by(State::Running)); - // AfterAny: any terminal state satisfies. + assert!(!DepWhen::AFTER_OK.satisfied_by(State::Skipped)); + // AfterAny: the dep reached a terminal state *some other way than being + // dropped* — success, failure, or ruled out by its own edges. assert!(DepWhen::AFTER_ANY.satisfied_by(State::Done)); assert!(DepWhen::AFTER_ANY.satisfied_by(State::Failed)); - assert!(DepWhen::AFTER_ANY.satisfied_by(State::Cancelled)); + assert!(DepWhen::AFTER_ANY.satisfied_by(State::Skipped)); + assert!( + !DepWhen::AFTER_ANY.satisfied_by(State::Cancelled), + "a dropped dep does not converge a weak dependent — nothing ever ran" + ); assert!(!DepWhen::AFTER_ANY.satisfied_by(State::Pending)); // Finishing satisfies neither — a dependent waits until the node rolls // up to a terminal state (all its sub-nodes done). diff --git a/hive-jobq/src/scheduler.rs b/hive-jobq/src/scheduler.rs index cb613fd0..f18437d5 100644 --- a/hive-jobq/src/scheduler.rs +++ b/hive-jobq/src/scheduler.rs @@ -235,6 +235,9 @@ impl Scheduler { /// Whether any direct child of `id` ended `Failed`/`Cancelled` — the roll-up /// failure condition for the parent. + /// `Skipped` children are **not** counted: being ruled out by an edge is the + /// expected fate of every branch not taken, so counting it would make any + /// group that branches on outcome roll up failed no matter how the run went. fn any_child_failed(&self, id: NodeId) -> bool { self.graph .nodes() @@ -321,9 +324,16 @@ impl Scheduler { .collect() } - /// Propagate cancellation out from a just-terminal `origin`: every - /// still-`Pending` node that can no longer run gets marked `Cancelled`, - /// transitively. Cancelled nodes were `Pending`, so they hold no resources. + /// Propagate elimination out from a just-terminal `origin`: every + /// still-`Pending` node that can no longer run gets marked + /// [`State::Skipped`], transitively. Skipped nodes were `Pending`, so they + /// hold no resources. + /// + /// `Skipped`, not `Cancelled`: these nodes were *ruled out by their edges*, + /// which is a normal outcome, not a dropped job. `Cancelled` is reserved for + /// work the caller abandoned before it started ([`Scheduler::cancel_node`]), + /// and the two are distinguished precisely so a parent's roll-up can ignore + /// the former while still treating the latter as not-success. /// /// One rule decides it: **a node is doomed once any edge it names can never /// be satisfied** — the dep settled on an outcome that edge does not accept. @@ -354,7 +364,7 @@ impl Scheduler { .map(|n| n.id) .collect(); for d in doomed { - self.graph.set_state(d, State::Cancelled); + self.graph.set_state(d, State::Skipped); stack.push(d); } } @@ -505,10 +515,10 @@ mod tests { assert_eq!(n.error.as_deref(), Some("boom")); assert!(n.finished_at.is_some()); - // The `AfterOk`-cancelled node: cascade-cancelled, so finished_at is set, + // The `AFTER_OK` dependent: ruled out by its edge, so finished_at is set, // but it never ran (no started_at) and carries no error of its own. let n = s.graph().node(downstream).unwrap(); - assert_eq!(n.state, State::Cancelled); + assert_eq!(n.state, State::Skipped); assert!(n.started_at.is_none()); assert!(n.finished_at.is_some()); assert_eq!(n.error, None); @@ -750,8 +760,8 @@ mod tests { .expect("weak"); assert_eq!(s.settle(), vec![root]); s.complete(root, Outcome::Failed(String::new())); - assert_eq!(s.graph().node(strong1).unwrap().state, State::Cancelled); - assert_eq!(s.graph().node(strong2).unwrap().state, State::Cancelled); + assert_eq!(s.graph().node(strong1).unwrap().state, State::Skipped); + assert_eq!(s.graph().node(strong2).unwrap().state, State::Skipped); assert_eq!(s.settle(), vec![weak]); } @@ -776,8 +786,9 @@ mod tests { s.complete(root, Outcome::Done); assert_eq!( s.graph().node(on_fail).unwrap().state, - State::Cancelled, - "a Failed-only branch is unsatisfiable once its dep succeeds" + State::Skipped, + "a Failed-only branch is unsatisfiable once its dep succeeds — and it is \ + `Skipped`, not `Cancelled`, so the parent roll-up ignores it" ); assert!(s.settle().is_empty(), "and nothing is left runnable"); } @@ -803,15 +814,15 @@ mod tests { .expect("on_fail"); assert_eq!(s.settle(), vec![root]); s.complete(root, Outcome::Failed("boom".to_owned())); - assert_eq!(s.graph().node(on_ok).unwrap().state, State::Cancelled); + assert_eq!(s.graph().node(on_ok).unwrap().state, State::Skipped); assert_eq!(s.settle(), vec![on_fail]); } - /// A weak edge accepts cancellation, so a tail survives the cancellation of - /// the work it reports on — the property hive-c0re's approval tails rely on, - /// held here by the edge itself rather than by any node-kind special case. + /// A weak edge accepts a dependency that was *ruled out*, so a tail still + /// runs when the work it reports on never happened — held by the edge itself + /// rather than by any node-kind special case. #[test] - fn cancelled_dep_still_satisfies_a_weak_edge() { + fn eliminated_dep_still_satisfies_a_weak_edge() { let mut s: Scheduler<&str, String> = Scheduler::new(Graph::new(), ResourceTable::new()); let root = s.append("root", vec![], None).expect("root"); let mid = s.append("mid", vec![after_ok(root)], None).expect("mid"); @@ -827,7 +838,7 @@ mod tests { .expect("tail"); assert_eq!(s.settle(), vec![root]); s.complete(root, Outcome::Failed("boom".to_owned())); - assert_eq!(s.graph().node(mid).unwrap().state, State::Cancelled); + assert_eq!(s.graph().node(mid).unwrap().state, State::Skipped); assert_eq!( s.settle(), vec![tail], @@ -838,11 +849,12 @@ mod tests { /// Edges are **conjunctive**, so "any of these several nodes failed" is not /// directly expressible — a `{Failed}` edge on each would mean *all* failed. /// The composition that does work: the success branch depends `AFTER_OK` on - /// every node (so it runs only if all succeeded, and is cancelled the moment - /// one doesn't), and the failure branch hangs off *it* with `{Cancelled}`. - /// The success branch becomes the aggregator, and exactly one of the two runs. + /// every node (so it runs only if all succeeded, and is ruled out the moment + /// one doesn't), and the failure branch hangs off *it* with `{Skipped}` — + /// "run when the success branch was ruled out". The success branch is the + /// aggregator, and exactly one of the two runs. #[test] - fn ok_branch_aggregates_and_failure_branch_hangs_off_its_cancellation() { + fn ok_branch_aggregates_and_failure_branch_hangs_off_its_elimination() { let build = || { let mut s: Scheduler<&str, String> = Scheduler::new(Graph::new(), ResourceTable::new()); let a = s.append("a", vec![], None).expect("a"); @@ -855,7 +867,7 @@ mod tests { "on_fail", vec![Dep::Node { id: on_ok, - when: DepWhen::of(&[TerminalState::Cancelled]), + when: DepWhen::of(&[TerminalState::Skipped]), }], None, ) @@ -870,16 +882,16 @@ mod tests { s.complete(b, Outcome::Done); assert_eq!(s.settle(), vec![on_ok]); s.complete(on_ok, Outcome::Done); - assert_eq!(s.graph().node(on_fail).unwrap().state, State::Cancelled); + assert_eq!(s.graph().node(on_fail).unwrap().state, State::Skipped); assert!(s.settle().is_empty()); - // One of them fails: the ok branch is cancelled, which is precisely the + // One of them fails: the ok branch is ruled out, which is precisely the // signal the failure branch waits on. let (mut s, a, b, on_ok, on_fail) = build(); assert_eq!(s.settle(), vec![a, b]); s.complete(a, Outcome::Failed("boom".to_owned())); s.complete(b, Outcome::Done); - assert_eq!(s.graph().node(on_ok).unwrap().state, State::Cancelled); + assert_eq!(s.graph().node(on_ok).unwrap().state, State::Skipped); assert_eq!(s.settle(), vec![on_fail]); } @@ -893,8 +905,8 @@ mod tests { let grandchild = s.append("gc", vec![], Some(child)).expect("gc"); assert_eq!(s.settle(), vec![root]); s.complete(root, Outcome::Failed(String::new())); - assert_eq!(s.graph().node(child).unwrap().state, State::Cancelled); - assert_eq!(s.graph().node(grandchild).unwrap().state, State::Cancelled); + assert_eq!(s.graph().node(child).unwrap().state, State::Skipped); + assert_eq!(s.graph().node(grandchild).unwrap().state, State::Skipped); } #[test] @@ -903,8 +915,9 @@ mod tests { let a = s.append("a", vec![], None).expect("a"); let b = s.append("b", vec![after_ok(a)], None).expect("b"); assert!(s.cancel_node(a)); + // `a` was dropped by the caller; `b` was merely ruled out by its edge. assert_eq!(s.graph().node(a).unwrap().state, State::Cancelled); - assert_eq!(s.graph().node(b).unwrap().state, State::Cancelled); + assert_eq!(s.graph().node(b).unwrap().state, State::Skipped); let c = s.append("c", vec![], None).expect("c"); assert_eq!(s.settle(), vec![c]); assert!(!s.cancel_node(c));