diff --git a/hive-c0re/src/job_queue/mod.rs b/hive-c0re/src/job_queue/mod.rs index 6427a0a1..5f26e253 100644 --- a/hive-c0re/src/job_queue/mod.rs +++ b/hive-c0re/src/job_queue/mod.rs @@ -480,7 +480,7 @@ impl JobQueue { inner .containers() .into_iter() - .filter(|&c| inner.sched.graph().is_settled(c) == Some(false)) + .filter(|&c| !inner.dag_is_terminal(c)) .count() } } @@ -505,17 +505,25 @@ impl QueueInner { }) } - /// The DAG container a node belongs to. A container is exactly a group root, - /// so this is the graph's own parent-chain walk. + /// The DAG container a node belongs to — walk its parent chain to the root + /// (`parent == None`), which is the container. Returns `id` itself for a + /// container node. fn dag_of(&self, id: NodeId) -> Option { - self.sched.graph().root_of(id) + let mut cur = id; + loop { + match self.sched.graph().node(cur)?.parent { + Some(p) => cur = p, + None => return Some(cur), + } + } } /// The DAG's work nodes — its `container`'s subtree, excluding the container. fn subtree(&self, container: NodeId) -> Vec { self.sched .graph() - .descendants(container) + .nodes() + .filter(|n| n.id != container && self.dag_of(n.id) == Some(container)) .map(|n| n.id) .collect() } @@ -545,6 +553,15 @@ impl QueueInner { }) } + /// True when the DAG has settled — its container has rolled up terminal + /// (equivalent to every work node being terminal). + fn dag_is_terminal(&self, container: NodeId) -> bool { + self.sched + .graph() + .node(container) + .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 @@ -557,12 +574,18 @@ impl QueueInner { }) } - /// First failed work node's error, for the dashboard's DAG-level error line. + /// 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 { - self.sched - .graph() - .first_error(container) - .map(ToOwned::to_owned) + for id in self.subtree(container) { + if let Some(n) = self.sched.graph().node(id) + && n.state == JobState::Failed + && let Some(e) = n.error.clone() + { + return Some(e); + } + } + None } /// Project a DAG into its wire [`DagView`]: a near-raw view of the @@ -652,7 +675,7 @@ impl QueueInner { if !any_unsettled { return None; } - let is_terminal = self.sched.graph().is_settled(container) == Some(true); + let is_terminal = self.dag_is_terminal(container); Some(DagView { id: container.get(), source: meta.source, @@ -695,7 +718,7 @@ impl QueueInner { let mut live: Vec = Vec::new(); let mut terminal: Vec<(NodeId, i64)> = Vec::new(); for c in self.containers() { - if self.sched.graph().is_settled(c) == Some(true) { + if self.dag_is_terminal(c) { terminal.push((c, self.dag_finished_at(c))); } else { live.push(c); diff --git a/hive-jobq/src/lib.rs b/hive-jobq/src/lib.rs index 3c374514..60e5b6cc 100644 --- a/hive-jobq/src/lib.rs +++ b/hive-jobq/src/lib.rs @@ -462,60 +462,6 @@ impl Graph { self.nodes.iter() } - /// Top of `id`'s [`Node::parent`] chain — the group root whose subtree `id` - /// lives in. Returns `id` itself when `id` is already a root, and `None` - /// only when `id` isn't in the graph. - #[must_use] - pub fn root_of(&self, id: NodeId) -> Option { - let mut cur = id; - loop { - match self.node(cur)?.parent { - Some(p) => cur = p, - None => return Some(cur), - } - } - } - - /// Every node in `id`'s subtree, excluding `id` itself, in insertion order. - pub fn descendants(&self, id: NodeId) -> impl Iterator> { - self.nodes - .iter() - .filter(move |n| self.is_descendant(n.id, id)) - } - - /// Every group root — the nodes with no parent. - pub fn roots(&self) -> impl Iterator> { - self.nodes.iter().filter(|n| n.parent.is_none()) - } - - /// Whether `id` has settled, or `None` when there is no such node. A group - /// root's state is its subtree's roll-up, so for a root this answers "is - /// everything under it finished" — which is why callers don't scan the - /// subtree themselves. - /// - /// `None` rather than `false` for an unknown id: "this node is not finished" - /// and "there is no such node" are different answers, and a caller that - /// conflates them keeps polling an id that will never settle. - #[must_use] - pub fn is_settled(&self, id: NodeId) -> Option { - self.node(id).map(|n| n.state.is_terminal()) - } - - /// Why `id`'s subtree failed: the error of the first `Failed` descendant - /// that carries one, in insertion order. - /// - /// Skipping the ones without an error is the point, not an optimisation. A - /// node that rolled up `Failed` from a child holds no error of its own, and - /// such a node can sort before the child that actually broke — stopping at - /// the first `Failed` node would report `None` while the real reason sits - /// further down the subtree. - #[must_use] - pub fn first_error(&self, id: NodeId) -> Option<&str> { - self.descendants(id) - .filter(|n| matches!(n.state, State::Failed)) - .find_map(|n| n.error.as_deref()) - } - /// Whether `ancestor` lies on `node`'s [`Node::parent`] chain (i.e. `node` is /// in `ancestor`'s subtree). `node` is not its own ancestor. fn is_descendant(&self, node: NodeId, ancestor: NodeId) -> bool { @@ -648,82 +594,6 @@ mod tests { )); } - /// Borrow a node mutably so a test can force its lifecycle state. Looks the - /// node up by id rather than indexing, so it doesn't quietly depend on ids - /// and positions coinciding. - fn node_mut<'g>( - g: &'g mut Graph<&'static str, String>, - id: NodeId, - ) -> &'g mut Node<&'static str, String> { - g.nodes - .iter_mut() - .find(|n| n.id == id) - .expect("node in graph") - } - - /// `root_of` walks to the top of the parent chain; `descendants` is its - /// inverse and excludes the node itself. - #[test] - fn root_of_and_descendants_span_the_subtree() { - let mut g: Graph<&str, String> = Graph::new(); - let root = g.insert("root", vec![], None).unwrap(); - let mid = g.insert("mid", vec![], Some(root)).unwrap(); - let leaf = g.insert("leaf", vec![], Some(mid)).unwrap(); - let other = g.insert("other-root", vec![], None).unwrap(); - - assert_eq!(g.root_of(leaf), Some(root), "walks the whole chain"); - assert_eq!(g.root_of(root), Some(root), "a root is its own root"); - assert_eq!(g.root_of(NodeId(99)), None, "unknown id"); - - let mut under_root: Vec = g.descendants(root).map(|n| n.id).collect(); - under_root.sort_unstable(); - assert_eq!(under_root, vec![mid, leaf], "excludes the node itself"); - assert_eq!(g.descendants(other).count(), 0); - - let roots: Vec = g.roots().map(|n| n.id).collect(); - assert_eq!(roots, vec![root, other]); - } - - /// The reason `first_error` looks for the first failed descendant **that - /// carries an error** rather than simply the first failed one: a node that - /// rolled its `Failed` up from a child holds no error of its own, and it - /// sorts *before* that child. Stopping at the first `Failed` node would - /// report `None` and lose the real reason. - #[test] - fn first_error_skips_a_rolled_up_failure_carrying_no_error() { - let mut g: Graph<&str, String> = Graph::new(); - let container = g.insert("dag", vec![], None).unwrap(); - let rolled_up = g.insert("prebuild", vec![], Some(container)).unwrap(); - let broke = g.insert("swap", vec![], Some(rolled_up)).unwrap(); - - node_mut(&mut g, rolled_up).state = State::Failed; - let broken = node_mut(&mut g, broke); - broken.state = State::Failed; - broken.error = Some("nix build exploded".to_owned()); - - assert_eq!(g.first_error(container), Some("nix build exploded")); - } - - #[test] - fn is_settled_tracks_node_state() { - let mut g: Graph<&str, String> = Graph::new(); - let n = g.insert("n", vec![], None).unwrap(); - assert_eq!(g.is_settled(n), Some(false), "Pending is not settled"); - node_mut(&mut g, n).state = State::Finishing; - assert_eq!( - g.is_settled(n), - Some(false), - "Finishing still has children running" - ); - node_mut(&mut g, n).state = State::Done; - assert_eq!(g.is_settled(n), Some(true)); - assert_eq!( - g.is_settled(NodeId(99)), - None, - "an unknown id is not the same answer as `not settled`" - ); - } - #[test] fn state_terminality() { assert!(State::Done.is_terminal());