diff --git a/hive-c0re/src/job_queue/mod.rs b/hive-c0re/src/job_queue/mod.rs index aa4be634..9300b526 100644 --- a/hive-c0re/src/job_queue/mod.rs +++ b/hive-c0re/src/job_queue/mod.rs @@ -345,13 +345,14 @@ impl JobQueue { /// 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. + /// /// `id` names **any node**, not specifically a DAG. Cancelling a group root /// drops that whole group (the cascade is the scheduler's), which is what /// the dashboard's whole-DAG cancel does; cancelling an interior node drops /// just that branch. Nothing here knows about DAGs. pub fn cancel(&self, id: u64) -> bool { let mut inner = self.lock(); - let Some(node) = inner.node_by_id(id) else { + let Some(node) = inner.sched.graph().resolve_id(id) else { return false; }; if !inner.sched.cancel_node(node) { @@ -464,17 +465,6 @@ impl QueueInner { /// The container node of `dag_id` — the `NodeKind::Dag` root whose id equals /// `dag_id`. `NodeId` is un-fabricable from a raw `u64`, so this is a search. - /// Resolve a raw wire `u64` to a graph [`NodeId`], whatever kind of node it - /// names. `NodeId` is un-fabricable from a `u64`, so this is a search — - /// same cost as [`QueueInner::container`], without asserting the node is a - /// DAG container. - fn node_by_id(&self, id: u64) -> Option { - self.sched - .graph() - .nodes() - .find_map(|n| (n.id.get() == id).then_some(n.id)) - } - fn container(&self, dag_id: u64) -> Option { self.sched.graph().nodes().find_map(|n| { (n.parent.is_none() diff --git a/hive-jobq/src/lib.rs b/hive-jobq/src/lib.rs index 4646428f..d89a1763 100644 --- a/hive-jobq/src/lib.rs +++ b/hive-jobq/src/lib.rs @@ -461,6 +461,21 @@ impl Graph { self.nodes.iter().find(|n| n.id == id) } + /// Resolve a raw value back to the opaque [`NodeId`] it names — the inverse + /// of [`NodeId::get`], and the only way to perform that direction. A caller + /// holding a value that crossed a wire cannot fabricate an id from it (that + /// impossibility is the point of the type), so it has to be matched against + /// the graph, which is what makes this a search rather than a cast. + /// + /// `None` when no node carries that value, which covers both a value that + /// was never an id and one whose node has since been reaped. + #[must_use] + pub fn resolve_id(&self, raw: u64) -> Option { + self.nodes + .iter() + .find_map(|n| (n.id.0 == raw).then_some(n.id)) + } + /// Every node in the graph, in insertion order. The scheduler iterates /// this to find runnable pending nodes. pub fn nodes(&self) -> impl Iterator> { @@ -729,6 +744,20 @@ mod tests { ); } + #[test] + fn resolve_id_inverts_get_and_rejects_a_value_that_was_never_an_id() { + let mut g: Graph<&str, String> = Graph::new(); + let a = g.insert("a", vec![], None).unwrap(); + let b = g.insert("b", vec![], None).unwrap(); + // Round-trips every id the graph handed out: this is the only way back + // from a raw value, since NodeId can't be constructed from one. + assert_eq!(g.resolve_id(a.get()), Some(a)); + assert_eq!(g.resolve_id(b.get()), Some(b)); + // A value that was never an id resolves to nothing, so a caller can't + // reach a node by guessing a number off the wire. + assert_eq!(g.resolve_id(u64::MAX), None); + } + #[test] fn state_terminality() { assert!(State::Done.is_terminal());