feat(#2500): validate NodeId references on insert and deserialize

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<N>")], 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

@@ -1667,6 +1667,7 @@ name = "hive-jobq"
 version = "0.1.0"
 dependencies = [
  "serde",
+ "serde_json",
  "thiserror 2.0.18",
 ]
 

hive-jobq/Cargo.toml

@@ -9,3 +9,6 @@ workspace = true
 [dependencies]
 serde = { workspace = true }
 thiserror = { workspace = true }
+
+[dev-dependencies]
+serde_json = { workspace = true }

hive-jobq/src/lib.rs

@@ -153,17 +153,65 @@ pub struct Node<N> {
     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<N>")]
 pub struct Graph<N> {
     nodes: Vec<Node<N>>,
     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<N> {
+    nodes: Vec<Node<N>>,
+    next_id: u64,
+}
+
+impl<N> TryFrom<GraphData<N>> for Graph<N> {
+    type Error = GraphError;
+
+    fn try_from(data: GraphData<N>) -> Result<Self, Self::Error> {
+        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<Node<N>>` needs no such bound, so impl it directly.
 impl<N> Default for Graph<N> {
@@ -191,7 +239,33 @@ impl<N> Graph<N> {
 
     /// 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<Dep>, parent: Option<NodeId>) -> 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<Dep>,
+        parent: Option<NodeId>,
+    ) -> Result<NodeId, GraphError> {
+        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<N> Graph<N> {
             deps,
             state: State::Pending,
         });
-        id
+        Ok(id)
     }
 
     /// Borrow a node by id.
@@ -227,6 +301,40 @@ impl<N> Graph<N> {
         };
         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 a = g.insert("sweep", vec![], None).unwrap();
-        let b = g.insert(
+        let b = g
-            "update",
+            .insert(
-            vec![Dep::Node {
+                "update",
-                id: a,
+                vec![Dep::Node {
-                when: DepWhen::AfterOk,
+                    id: a,
-            }],
+                    when: DepWhen::AfterOk,
-            Some(a),
+                }],
-        );
+                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 group = g.insert("group", vec![], None).unwrap();
-        let child = g.insert("child", vec![], Some(group));
+        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<String> = 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<String> = 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::<String> {
+            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::<Graph<String>>(&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,
+            }
+        );
+    }
 }