diff --git a/hive-jobq/src/scheduler.rs b/hive-jobq/src/scheduler.rs index 462ff3d1..e61dd64d 100644 --- a/hive-jobq/src/scheduler.rs +++ b/hive-jobq/src/scheduler.rs @@ -233,15 +233,31 @@ impl Scheduler { .all(|n| n.parent != Some(id) || n.state.is_terminal()) } - /// 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() - .any(|n| n.parent == Some(id) && matches!(n.state, State::Failed | State::Cancelled)) + /// What `id` rolls up to once all its children are terminal: `Failed` if any + /// child failed, else `Cancelled` if any was cancelled, else `Done`. + /// + /// `Failed` outranks `Cancelled` because the failure is the actionable fact — + /// a group where one step broke and the rest were dropped in response is a + /// failure, not a cancellation. A group whose children were *all* dropped + /// never failed at anything, and says so. + /// + /// `Skipped` children are ignored: 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 non-`Done` however the run went. + fn rolled_up_state(&self, id: NodeId) -> State { + let mut any_cancelled = false; + for child in self.graph.nodes().filter(|n| n.parent == Some(id)) { + match child.state { + State::Failed => return State::Failed, + State::Cancelled => any_cancelled = true, + _ => {} + } + } + if any_cancelled { + State::Cancelled + } else { + State::Done + } } /// Transition a node whose own logic just *succeeded* to its resulting state: @@ -254,12 +270,10 @@ impl Scheduler { /// its dependency succeeds, and leaving it `Pending` would wedge the subtree /// non-terminal forever. fn settle_terminal(&mut self, id: NodeId) { - let state = if !self.all_children_terminal(id) { - State::Finishing - } else if self.any_child_failed(id) { - State::Failed + let state = if self.all_children_terminal(id) { + self.rolled_up_state(id) } else { - State::Done + State::Finishing }; self.graph.set_state(id, state); if state.is_terminal() { @@ -279,11 +293,7 @@ impl Scheduler { { break; } - let state = if self.any_child_failed(a) { - State::Failed - } else { - State::Done - }; + let state = self.rolled_up_state(a); self.graph.set_state(a, state); // Any terminal outcome can rule a dependent out — see `settle_terminal`. self.cascade_cancel(a); @@ -982,13 +992,10 @@ mod tests { assert!(s.cancel_node(root)); assert_eq!(s.graph().node(a).unwrap().state, State::Cancelled); assert_eq!(s.graph().node(b).unwrap().state, State::Cancelled); - // With every child terminal the root leaves `Finishing` and rolls up. - // It rolls up `Failed`, not `Cancelled`: the roll-up counts a cancelled - // child as a non-success, and it has no separate "the whole group was - // dropped" outcome. Pre-existing and unchanged here — a caller that - // wants to show a cancel as a cancel derives that from the node states, - // not from the root's. - assert_eq!(s.graph().node(root).unwrap().state, State::Failed); + // With every child terminal the root leaves `Finishing` and rolls up — + // as `Cancelled`, not `Failed`: nothing under it failed at anything, the + // work was dropped. + assert_eq!(s.graph().node(root).unwrap().state, State::Cancelled); } /// All-or-nothing: an in-flight node's work is not interruptible, and