From 8c5de704be630064b43cff60e06a3fee1ccd0d1e Mon Sep 17 00:00:00 2001 From: atlas Date: Mon, 27 Jul 2026 20:16:28 +0200 Subject: [PATCH] fix(#2772): is_settled distinguishes "not finished" from "no such node" Returning `false` for an unknown id gave the same answer as a node that is merely still running, so a caller polling a stale id would wait forever for a state that can never arrive. `Option` makes the two cases separate, matching `node()`'s convention that `None` means the id isn't in the graph. --- hive-c0re/src/job_queue/mod.rs | 6 +++--- hive-jobq/src/lib.rs | 32 ++++++++++++++++++++++---------- 2 files changed, 25 insertions(+), 13 deletions(-) diff --git a/hive-c0re/src/job_queue/mod.rs b/hive-c0re/src/job_queue/mod.rs index 08a5a148..6427a0a1 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)) + .filter(|&c| inner.sched.graph().is_settled(c) == Some(false)) .count() } } @@ -652,7 +652,7 @@ impl QueueInner { if !any_unsettled { return None; } - let is_terminal = self.sched.graph().is_settled(container); + let is_terminal = self.sched.graph().is_settled(container) == Some(true); Some(DagView { id: container.get(), source: meta.source, @@ -695,7 +695,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) { + if self.sched.graph().is_settled(c) == Some(true) { 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 68f3ac5b..3c374514 100644 --- a/hive-jobq/src/lib.rs +++ b/hive-jobq/src/lib.rs @@ -488,13 +488,17 @@ impl Graph { self.nodes.iter().filter(|n| n.parent.is_none()) } - /// Whether `id` has settled. 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. Unknown ids are not - /// settled. + /// 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) -> bool { - self.node(id).is_some_and(|n| n.state.is_terminal()) + 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 @@ -704,12 +708,20 @@ mod tests { fn is_settled_tracks_node_state() { let mut g: Graph<&str, String> = Graph::new(); let n = g.insert("n", vec![], None).unwrap(); - assert!(!g.is_settled(n), "Pending is not settled"); + assert_eq!(g.is_settled(n), Some(false), "Pending is not settled"); node_mut(&mut g, n).state = State::Finishing; - assert!(!g.is_settled(n), "Finishing still has children running"); + assert_eq!( + g.is_settled(n), + Some(false), + "Finishing still has children running" + ); node_mut(&mut g, n).state = State::Done; - assert!(g.is_settled(n)); - assert!(!g.is_settled(NodeId(99)), "unknown id is not settled"); + 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]