diff --git a/hive-jobq/src/builder.rs b/hive-jobq/src/builder.rs index e0a55abe..191beef2 100644 --- a/hive-jobq/src/builder.rs +++ b/hive-jobq/src/builder.rs @@ -34,8 +34,33 @@ use crate::{Dep, DepWhen, Graph, GraphError, NodeId, TerminalState}; /// that exists. Deliberately not a [`NodeId`]: those are minted by the graph at /// insert time and are meaningful system-wide, while this is a builder-local /// label that stops existing once the job is inserted. +/// +/// The `job` half is what makes it safe to compare. A [`NodeRef`] converts into +/// a bare `NodeGuid`, dropping the borrow that ties it to its builder — so a +/// handle *can* be carried to a different job (captured by an inner closure, +/// say). Were the identity a plain per-builder counter, the two jobs' `0`s would +/// be equal and that handle would silently resolve to an unrelated node. With a +/// per-builder random half it simply isn't found, and the insert fails by name. #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] -pub struct NodeGuid(u64); +pub struct NodeGuid { + /// Identifies the builder that issued this handle. Random per builder, so + /// two jobs' handles never compare equal. + job: u64, + /// Position within that builder — the cheap half, since `job` already + /// separates one builder's handles from another's. + seq: u32, +} + +/// A fresh identity for one builder. +/// +/// `RandomState` is seeded from the process's random state and advances per +/// instance, so each builder gets a distinct, unguessable value — enough to +/// make a foreign handle a miss rather than a collision. No `rand` dependency +/// for what is a collision-avoidance job, not a cryptographic one. +fn fresh_job_id() -> u64 { + use std::hash::BuildHasher as _; + std::collections::hash_map::RandomState::new().hash_one(0_u8) +} /// Why a job could not be inserted. #[derive(Debug, Clone, PartialEq, Eq, thiserror::Error)] @@ -89,9 +114,10 @@ struct Pending { #[derive(Debug)] pub struct JobBuilder { nodes: RefCell>>, - /// Source of handles. Monotonic and builder-local; the value is - /// deliberately meaningless outside this builder. - next_guid: Cell, + /// Identifies this builder in every handle it issues. + job: u64, + /// Position within this builder — the `seq` half of a [`NodeGuid`]. + next_seq: Cell, } // Hand-written rather than derived: `#[derive(Default)]` would demand @@ -100,15 +126,22 @@ impl Default for JobBuilder { fn default() -> Self { Self { nodes: RefCell::new(Vec::new()), - next_guid: Cell::new(0), + job: fresh_job_id(), + next_seq: Cell::new(0), } } } impl JobBuilder { /// A fresh, empty builder. - #[must_use] - pub fn new() -> Self { + /// + /// **Crate-private, and that is the API.** A builder is only ever handed to + /// a closure by an insertion entry point ([`Graph::insert_job`], + /// [`crate::scheduler::Scheduler::insert_job`]), which inserts the declared + /// nodes and returns the ids. Nothing job-shaped is constructible or + /// carryable outside this crate — otherwise it is a spec factory again, + /// just with a builder's name on it. + pub(crate) fn new() -> Self { Self::default() } @@ -124,8 +157,11 @@ impl JobBuilder { /// The returned handle is where those are declared; it is [`Copy`], so it /// can be named as a dependency as many times as needed. pub fn node(&self, payload: N) -> NodeRef<'_, N, R> { - let guid = NodeGuid(self.next_guid.get()); - self.next_guid.set(guid.0 + 1); + let guid = NodeGuid { + job: self.job, + seq: self.next_seq.get(), + }; + self.next_seq.set(guid.seq + 1); self.nodes.borrow_mut().push(Pending { guid, payload, @@ -157,7 +193,7 @@ impl JobBuilder { /// [`BuildError::ForwardEdge`] / [`BuildError::ForwardParent`] if a node /// references one declared after it, or [`BuildError::Graph`] if the graph /// rejects a node (see [`Graph::insert`]). - pub fn insert_into( + pub(crate) fn insert_into( self, graph: &mut Graph, root_parent: Option, @@ -182,7 +218,7 @@ impl JobBuilder { /// need all-or-nothing should insert into a scratch graph, or treat a /// failure as fatal — every variant is a programming error in the job's own /// shape, not a runtime condition to recover from. - pub fn insert_with( + pub(crate) fn insert_with( self, root_parent: Option, mut insert: impl FnMut(N, Vec>, Option) -> Result, @@ -353,7 +389,7 @@ impl NodeRef<'_, N, R> { #[cfg(test)] mod tests { - use super::{BuildError, JobBuilder, NodeGuid}; + use super::BuildError; use crate::{Dep, DepWhen, Graph, NodeId}; /// A graph whose payload is a name and whose resources are strings. @@ -370,12 +406,15 @@ mod tests { #[test] fn edges_resolve_to_minted_ids() { let mut g = graph(); - let b = JobBuilder::new(); - let first = b.node("a"); - let second = b.node("b").after_ok(first); - let (first, second) = (first.guid(), second.guid()); - - let ids = b.insert_into(&mut g, None).expect("insert"); + let mut named = None; + let ids = g + .insert_job(None, |b| { + let first = b.node("a"); + let second = b.node("b").after_ok(first); + named = Some((first.guid(), second.guid())); + }) + .expect("insert"); + let (first, second) = named.expect("declared"); assert_eq!( deps_of(&g, ids[&second]), vec![Dep::Node { @@ -389,12 +428,15 @@ mod tests { #[test] fn parent_resolves_to_a_minted_id() { let mut g = graph(); - let b = JobBuilder::new(); - let root = b.node("a"); - let child = b.node("b").part_of(root).guid(); - let root = root.guid(); - - let ids = b.insert_into(&mut g, None).expect("insert"); + let mut named = None; + let ids = g + .insert_job(None, |b| { + let root = b.node("a"); + let child = b.node("b").part_of(root); + named = Some((root.guid(), child.guid())); + }) + .expect("insert"); + let (root, child) = named.expect("declared"); assert_eq!(g.node(ids[&root]).expect("root").parent, None); assert_eq!(g.node(ids[&child]).expect("child").parent, Some(ids[&root])); } @@ -404,13 +446,16 @@ mod tests { #[test] fn one_handle_can_be_depended_on_twice() { let mut g = graph(); - let b = JobBuilder::new(); - let shared = b.node("a"); - let ok = b.node("b").after_ok(shared).guid(); - let any = b.node("c").after_any(shared).guid(); - let shared = shared.guid(); - - let ids = b.insert_into(&mut g, None).expect("insert"); + let mut named = None; + let ids = g + .insert_job(None, |b| { + let shared = b.node("a"); + let ok = b.node("b").after_ok(shared); + let any = b.node("c").after_any(shared); + named = Some((shared.guid(), ok.guid(), any.guid())); + }) + .expect("insert"); + let (shared, ok, any) = named.expect("declared"); assert_eq!( deps_of(&g, ids[&ok]), vec![Dep::Node { @@ -431,11 +476,18 @@ mod tests { #[test] fn resources_become_resource_deps() { let mut g = graph(); - let b = JobBuilder::new(); - let only = b.node("a").needs("agent/atlas").needs_units("build", 2); - let only = only.guid(); - - let ids = b.insert_into(&mut g, None).expect("insert"); + let mut named = None; + let ids = g + .insert_job(None, |b| { + named = Some( + b.node("a") + .needs("agent/atlas") + .needs_units("build", 2) + .guid(), + ); + }) + .expect("insert"); + let only = named.expect("declared"); assert_eq!( deps_of(&g, ids[&only]), vec![ @@ -458,17 +510,21 @@ mod tests { #[test] fn a_forward_edge_is_rejected_by_name() { let mut g = graph(); - let b = JobBuilder::new(); - let first = b.node("a"); - let second = b.node("b"); - let _ = first.after_any(second); - - let err = b.insert_into(&mut g, None).expect_err("forward edge"); + let mut named = None; + let err = g + .insert_job(None, |b| { + let first = b.node("a"); + let second = b.node("b"); + let _ = first.after_any(second); + named = Some((first.guid(), second.guid())); + }) + .expect_err("forward edge"); + let (first, second) = named.expect("declared"); assert_eq!( err, BuildError::ForwardEdge { - node: NodeGuid(0), - dep: NodeGuid(1) + node: first, + dep: second } ); } @@ -476,33 +532,66 @@ mod tests { #[test] fn a_forward_parent_is_rejected_by_name() { let mut g = graph(); - let b = JobBuilder::new(); - let child = b.node("a"); - let parent = b.node("b"); - let _ = child.part_of(parent); - - let err = b.insert_into(&mut g, None).expect_err("forward parent"); + let mut named = None; + let err = g + .insert_job(None, |b| { + let child = b.node("a"); + let parent = b.node("b"); + let _ = child.part_of(parent); + named = Some((child.guid(), parent.guid())); + }) + .expect_err("forward parent"); + let (child, parent) = named.expect("declared"); assert_eq!( err, BuildError::ForwardParent { - node: NodeGuid(0), - parent: NodeGuid(1) + node: child, + parent } ); } + /// A handle belongs to the job that issued it. Carrying one into a second + /// job — an inner closure capturing an outer handle, say — must not + /// silently address whichever node happens to sit at the same position. + /// + /// This is what the random half of a [`NodeGuid`] buys: with a plain + /// per-builder counter both jobs' first handles would be equal, and the + /// edge below would resolve, wrongly, to the second job's own node. + #[test] + fn a_handle_from_another_job_is_not_silently_resolved() { + let mut g = graph(); + let mut foreign = None; + g.insert_job(None, |b| { + foreign = Some(b.node("first job").guid()); + }) + .expect("first job inserts"); + let foreign = foreign.expect("declared"); + + let err = g + .insert_job(None, |b| { + let _ = b.node("second job").after_ok(foreign); + }) + .expect_err("foreign handle"); + assert!( + matches!(err, BuildError::ForwardEdge { dep, .. } if dep == foreign), + "{err:?}" + ); + } + /// The graph's own validation still applies — the builder does not /// pre-empt it. #[test] fn graph_rejection_surfaces_as_is() { let mut g = graph(); - let b = JobBuilder::new(); - let root = b.node("root"); - // A child may not depend on its own parent: the parent gate already - // orders them, and the edge would deadlock. - let _ = b.node("child").part_of(root).after_ok(root); - - let err = b.insert_into(&mut g, None).expect_err("out-of-group dep"); + let err = g + .insert_job(None, |b| { + let root = b.node("root"); + // A child may not depend on its own parent: the parent gate + // already orders them, and the edge would deadlock. + let _ = b.node("child").part_of(root).after_ok(root); + }) + .expect_err("out-of-group dep"); assert!(matches!(err, BuildError::Graph(_)), "{err:?}"); } @@ -514,12 +603,15 @@ mod tests { let mut g = graph(); let container = g.insert("container", Vec::new(), None).expect("container"); - let b = JobBuilder::new(); - let root = b.node("root"); - let child = b.node("child").part_of(root).guid(); - let root = root.guid(); - - let ids = b.insert_into(&mut g, Some(container)).expect("insert"); + let mut named = None; + let ids = g + .insert_job(Some(container), |b| { + let root = b.node("root"); + let child = b.node("child").part_of(root); + named = Some((root.guid(), child.guid())); + }) + .expect("insert"); + let (root, child) = named.expect("declared"); assert_eq!(g.node(ids[&root]).expect("root").parent, Some(container)); assert_eq!(g.node(ids[&child]).expect("child").parent, Some(ids[&root])); } @@ -527,9 +619,7 @@ mod tests { #[test] fn an_empty_builder_inserts_nothing() { let mut g = graph(); - let ids = JobBuilder::<&str, &str>::new() - .insert_into(&mut g, None) - .expect("insert"); + let ids = g.insert_job(None, |_| {}).expect("insert"); assert!(ids.is_empty()); assert_eq!(g.nodes().count(), 0); } diff --git a/hive-jobq/src/lib.rs b/hive-jobq/src/lib.rs index e6f61e54..6f27bfb0 100644 --- a/hive-jobq/src/lib.rs +++ b/hive-jobq/src/lib.rs @@ -458,6 +458,28 @@ impl Graph { Ok(id) } + /// Insert a whole job under `root_parent`, returning the id each handle's + /// node was minted as. + /// + /// `declare` receives a fresh [`JobBuilder`] and names the job's nodes on + /// it; the builder never leaves this call, so a job cannot be built in one + /// place and inserted in another. The graph-only counterpart of + /// [`crate::scheduler::Scheduler::insert_job`] — prefer that one when a + /// scheduler owns the graph, since it also records what it started. + /// + /// # Errors + /// Propagates [`BuildError`] — a forward reference in the job's own + /// declarations, a handle from a different job, or a graph rejection. + pub fn insert_job( + &mut self, + root_parent: Option, + declare: impl FnOnce(&JobBuilder), + ) -> Result, BuildError> { + let job = JobBuilder::new(); + declare(&job); + job.insert_into(self, root_parent) + } + /// Borrow a node by id. #[must_use] pub fn node(&self, id: NodeId) -> Option<&Node> { diff --git a/hive-jobq/src/scheduler.rs b/hive-jobq/src/scheduler.rs index dbc10fb9..dd92d07a 100644 --- a/hive-jobq/src/scheduler.rs +++ b/hive-jobq/src/scheduler.rs @@ -100,23 +100,31 @@ impl Scheduler { self.graph.insert(payload, deps, parent) } - /// Insert a whole job — every node a [`JobBuilder`] declared — under - /// `root_parent`, returning the id each handle's node was minted as. + /// Insert a whole job under `root_parent`, returning the id each handle's + /// node was minted as. /// - /// The scheduler-side counterpart of [`JobBuilder::insert_into`]: same + /// `declare` receives a fresh [`JobBuilder`] and names the job's nodes on + /// it; the builder never leaves this call. That is the whole insertion + /// API — a caller cannot construct a builder, hold one, or insert one + /// itself, so there is no way to end up with a job-shaped value being + /// passed around as a spec. + /// + /// The scheduler-side counterpart of [`Graph::insert_job`]: same /// resolution, but each node goes through [`Scheduler::append`], so a - /// caller building a job never has to reach past the scheduler at the graph - /// underneath. Call [`Scheduler::settle`] afterwards to start whatever - /// became runnable. + /// caller never has to reach past the scheduler at the graph underneath. + /// Call [`Scheduler::settle`] afterwards to start whatever became + /// runnable. /// /// # Errors /// Propagates [`BuildError`] — a forward reference in the job's own /// declarations, or a graph rejection. pub fn insert_job( &mut self, - job: JobBuilder, root_parent: Option, + declare: impl FnOnce(&JobBuilder), ) -> Result, BuildError> { + let job = JobBuilder::new(); + declare(&job); job.insert_with(root_parent, |payload, deps, parent| { self.append(payload, deps, parent) })