From 11df4a1bf558858b3b503b117a3f1b62b1ed08d8 Mon Sep 17 00:00:00 2001 From: atlas Date: Fri, 17 Jul 2026 12:09:51 +0200 Subject: [PATCH] feat(#2500): validate NodeId references on insert and deserialize MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Per mara's direction — validate ids as they enter the graph so internal iteration can trust every id the graph holds; the generational route for removal comes later. Adds GraphError; insert() now rejects a dangling Dep::Node / parent id (it is fallible); validate() checks all internal id references resolve and that next_id is past the largest existing id; deserialization runs validate() via #[serde(try_from = "GraphData")], so a loaded graph can never carry a dangling reference. 5 new tests; serde_json added as a dev-dependency for the round-trip cases. --- Cargo.lock | 1 + hive-jobq/Cargo.toml | 3 + hive-jobq/src/lib.rs | 224 ++++++++++++++++++++++++++++++++++++++++--- 3 files changed, 214 insertions(+), 14 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 470daab4..abf86db0 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1667,6 +1667,7 @@ name = "hive-jobq" version = "0.1.0" dependencies = [ "serde", + "serde_json", "thiserror 2.0.18", ] diff --git a/hive-jobq/Cargo.toml b/hive-jobq/Cargo.toml index a20512f9..71e644a4 100644 --- a/hive-jobq/Cargo.toml +++ b/hive-jobq/Cargo.toml @@ -9,3 +9,6 @@ workspace = true [dependencies] serde = { workspace = true } thiserror = { workspace = true } + +[dev-dependencies] +serde_json = { workspace = true } diff --git a/hive-jobq/src/lib.rs b/hive-jobq/src/lib.rs index 4420362b..6af94cde 100644 --- a/hive-jobq/src/lib.rs +++ b/hive-jobq/src/lib.rs @@ -153,17 +153,65 @@ pub struct Node { pub state: State, } +/// An error from inserting into or loading a [`Graph`] with a dangling id. +/// +/// A [`NodeId`] is only meaningful against the graph that minted it, so both +/// entry points — [`Graph::insert`] and deserialization — reject references to +/// nodes the graph does not contain. That is what lets internal iteration trust +/// every id the graph holds. +#[derive(Debug, Clone, PartialEq, Eq, thiserror::Error)] +pub enum GraphError { + /// A node's dependency named an id not present in the graph. + #[error("dependency references unknown node {0:?}")] + UnknownDep(NodeId), + /// A node's parent named an id not present in the graph. + #[error("parent references unknown node {0:?}")] + UnknownParent(NodeId), + /// A loaded graph's `next_id` counter is not past the largest existing id, + /// so the next minted id would collide with one already in the graph. + #[error("next_id {next_id} must exceed the largest existing node id {max_id}")] + NextIdTooSmall { + /// The persisted counter value. + next_id: u64, + /// The largest id already present. + max_id: u64, + }, +} + /// The single persistent graph of all nodes. /// /// New jobs are inserted as node groups; the scheduler (added in a follow-up) /// walks this graph filling open slots. Completed groups are retained (no /// pruning in v1). #[derive(Debug, serde::Serialize, serde::Deserialize)] +#[serde(try_from = "GraphData")] pub struct Graph { nodes: Vec>, next_id: u64, } +// Deserialization target: the raw fields, turned into a `Graph` by the `TryFrom` +// below — which runs [`Graph::validate`], so a loaded graph can never carry a +// dangling id reference (Serialize does not validate; Deserialize always does). +#[derive(serde::Deserialize)] +struct GraphData { + nodes: Vec>, + next_id: u64, +} + +impl TryFrom> for Graph { + type Error = GraphError; + + fn try_from(data: GraphData) -> Result { + let graph = Graph { + nodes: data.nodes, + next_id: data.next_id, + }; + graph.validate()?; + Ok(graph) + } +} + // A `derive(Default)` would wrongly require `N: Default` (an empty graph holds // no payload); an empty `Vec>` needs no such bound, so impl it directly. impl Default for Graph { @@ -191,7 +239,33 @@ impl Graph { /// Insert a node with the given payload, deps, and parent group, returning /// its freshly-minted id. The node starts [`State::Pending`]. - pub fn insert(&mut self, payload: N, deps: Vec, parent: Option) -> NodeId { + /// + /// Every [`Dep::Node`] id and the `parent` id (if any) must already resolve + /// to a node in the graph — an id is only meaningful against the graph that + /// minted it, so a dangling reference is rejected here rather than surfacing + /// as a broken edge later. + /// + /// # Errors + /// Returns [`GraphError::UnknownParent`] / [`GraphError::UnknownDep`] if the + /// parent or a dependency references a node not in the graph. + pub fn insert( + &mut self, + payload: N, + deps: Vec, + parent: Option, + ) -> Result { + if let Some(parent_id) = parent + && self.node(parent_id).is_none() + { + return Err(GraphError::UnknownParent(parent_id)); + } + for dep in &deps { + if let Dep::Node { id, .. } = dep + && self.node(*id).is_none() + { + return Err(GraphError::UnknownDep(*id)); + } + } let id = self.mint_id(); self.nodes.push(Node { id, @@ -200,7 +274,7 @@ impl Graph { deps, state: State::Pending, }); - id + Ok(id) } /// Borrow a node by id. @@ -227,6 +301,40 @@ impl Graph { }; node.state.is_terminal() && self.children(id).all(|child| self.group_terminal(child.id)) } + + /// Check that every id the graph holds resolves: each node's `parent` and + /// every [`Dep::Node`] id names a node present in the graph, and `next_id` + /// is past the largest existing id. Deserialization runs this, so a loaded + /// graph is internally consistent and internal iteration can trust its ids. + /// + /// # Errors + /// Returns [`GraphError`] on a dangling parent / dependency reference, or a + /// `next_id` that would remint an id already in the graph. + pub fn validate(&self) -> Result<(), GraphError> { + for node in &self.nodes { + if let Some(parent_id) = node.parent + && self.node(parent_id).is_none() + { + return Err(GraphError::UnknownParent(parent_id)); + } + for dep in &node.deps { + if let Dep::Node { id, .. } = dep + && self.node(*id).is_none() + { + return Err(GraphError::UnknownDep(*id)); + } + } + } + if let Some(max_id) = self.nodes.iter().map(|n| n.id.0).max() + && self.next_id <= max_id + { + return Err(GraphError::NextIdTooSmall { + next_id: self.next_id, + max_id, + }); + } + Ok(()) + } } #[cfg(test)] @@ -236,15 +344,17 @@ mod tests { #[test] fn insert_mints_stable_monotonic_ids() { let mut g: Graph<&str> = Graph::new(); - let a = g.insert("sweep", vec![], None); - let b = g.insert( - "update", - vec![Dep::Node { - id: a, - when: DepWhen::AfterOk, - }], - Some(a), - ); + let a = g.insert("sweep", vec![], None).unwrap(); + let b = g + .insert( + "update", + vec![Dep::Node { + id: a, + when: DepWhen::AfterOk, + }], + Some(a), + ) + .unwrap(); assert_eq!(a, NodeId(0)); assert_eq!(b, NodeId(1)); // Membership is the parent edge, not the id. @@ -260,8 +370,8 @@ mod tests { #[test] fn group_terminal_requires_the_group_node_and_all_children_terminal() { let mut g: Graph<&str> = Graph::new(); - let group = g.insert("group", vec![], None); - let child = g.insert("child", vec![], Some(group)); + let group = g.insert("group", vec![], None).unwrap(); + let child = g.insert("child", vec![], Some(group)).unwrap(); // Both pending → not terminal. assert!(!g.group_terminal(group)); // Child done, but the group node itself is still pending → NOT terminal: @@ -278,7 +388,7 @@ mod tests { // A running node with no children yet may still append some, so it must // not read as terminal just because its child set is currently empty. let mut g: Graph<&str> = Graph::new(); - let group = g.insert("group", vec![], None); + let group = g.insert("group", vec![], None).unwrap(); set_state(&mut g, group, State::Running); assert!(!g.group_terminal(group)); // Once it finishes (having grown no children), it is terminal. @@ -309,4 +419,90 @@ mod tests { assert!(DepWhen::AfterAny.satisfied_by(State::Cancelled)); assert!(!DepWhen::AfterAny.satisfied_by(State::Pending)); } + + #[test] + fn insert_rejects_unknown_parent() { + let mut g: Graph<&str> = Graph::new(); + let bogus = NodeId(7); + assert_eq!( + g.insert("x", vec![], Some(bogus)).unwrap_err(), + GraphError::UnknownParent(bogus) + ); + } + + #[test] + fn insert_rejects_unknown_dep() { + let mut g: Graph<&str> = Graph::new(); + let bogus = NodeId(42); + let deps = vec![Dep::Node { + id: bogus, + when: DepWhen::AfterOk, + }]; + assert_eq!( + g.insert("x", deps, None).unwrap_err(), + GraphError::UnknownDep(bogus) + ); + } + + #[test] + fn valid_graph_round_trips_through_serde() { + let mut g: Graph = Graph::new(); + let a = g.insert("a".to_owned(), vec![], None).unwrap(); + g.insert( + "b".to_owned(), + vec![Dep::Node { + id: a, + when: DepWhen::AfterAny, + }], + Some(a), + ) + .unwrap(); + let json = serde_json::to_string(&g).unwrap(); + let back: Graph = serde_json::from_str(&json).unwrap(); + assert!(back.validate().is_ok()); + assert_eq!(back.node(a).unwrap().payload, "a"); + } + + #[test] + fn deserialize_rejects_a_dangling_dependency() { + // Build a graph whose only node depends on a non-existent id, serialize + // it (Serialize does not validate), and confirm deserialize rejects it. + let bad = Graph:: { + nodes: vec![Node { + id: NodeId(0), + parent: None, + payload: "x".to_owned(), + deps: vec![Dep::Node { + id: NodeId(99), + when: DepWhen::AfterOk, + }], + state: State::Pending, + }], + next_id: 1, + }; + let json = serde_json::to_string(&bad).unwrap(); + let err = serde_json::from_str::>(&json).unwrap_err(); + assert!(err.to_string().contains("unknown node")); + } + + #[test] + fn validate_rejects_next_id_that_would_remint() { + let bad = Graph::<&str> { + nodes: vec![Node { + id: NodeId(5), + parent: None, + payload: "x", + deps: vec![], + state: State::Pending, + }], + next_id: 3, + }; + assert_eq!( + bad.validate().unwrap_err(), + GraphError::NextIdTooSmall { + next_id: 3, + max_id: 5, + } + ); + } }