vinzenz/hyperhive
vinzenz/hyperhive
Watch
0
0
Fork
You've already forked hyperhive
 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

feat(jobq): hand the builder to a closure, and make a handle name its job

Browse source 
Two review findings from the operator, both about the builder being more
reachable than the design said.

**The builder must not leave the crate.** The module doc claimed "an
insertion API, not a spec factory — a builder is only ever handed to a
closure by the queue's insertion entry point", and then `new()` and
`insert_into` were public, so a caller could build one, carry it around
and insert it later. That is a spec factory with a builder's name on it.
`insert_job(root_parent, |b| …)` is now the whole API: the builder is
created inside the call, handed to the closure, and consumed there.
`new` / `insert_into` / `insert_with` are crate-private.

**A handle names the job that issued it.** `NodeGuid` was a per-builder
counter, so two jobs' first handles compared equal. `NodeRef` converts
into a bare `NodeGuid` — dropping the borrow that ties it to its
builder — so a handle carried into a second job (an inner closure
capturing an outer handle) would silently resolve to whatever that job's
first node happened to be. It is now `{ job, seq }` with a random `job`
half, so a foreign handle is a miss and the insert fails naming it. The
randomness comes from `RandomState`, which is collision-avoidance rather
than cryptography and needs no new dependency.

Tests moved onto the closure API rather than keeping their in-crate
access to the private constructor — a test that only passes because it
lives inside the crate is not testing the API a caller has. The two
forward-reference tests stopped asserting literal guid values (a random
half cannot be written down) and compare against the handles instead,
and a new test carries a handle between two jobs to pin the behaviour
that motivated the change.
This commit is contained in:
atlas 2026-08-02 13:29:44 +02:00 • committed by mara
commit f7548e4535
3 changed files with 195 additions and 75 deletions
Download patch file Download diff file Expand all files Collapse all files

226
hive-jobq/src/builder.rs
View file

@ -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<N, R> {
#[derive(Debug)]
pub struct JobBuilder<N, R> {
nodes: RefCell<Vec<Pending<N, R>>>,
/// Source of handles. Monotonic and builder-local; the value is
/// deliberately meaningless outside this builder.
next_guid: Cell<u64>,
/// Identifies this builder in every handle it issues.
job: u64,
/// Position within this builder — the `seq` half of a [`NodeGuid`].
next_seq: Cell<u32>,
}
// Hand-written rather than derived: `#[derive(Default)]` would demand
@ -100,15 +126,22 @@ impl<N, R> Default for JobBuilder<N, R> {
fn default() -> Self {
Self {
nodes: RefCell::new(Vec::new()),
next_guid: Cell::new(0),
job: fresh_job_id(),
next_seq: Cell::new(0),
}
}
}
impl<N, R> JobBuilder<N, R> {
/// 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<N, R> JobBuilder<N, R> {
/// 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<N, R> JobBuilder<N, R> {
/// [`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<N, R>,
root_parent: Option<NodeId>,
@ -182,7 +218,7 @@ impl<N, R> JobBuilder<N, R> {
/// 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<NodeId>,
mut insert: impl FnMut(N, Vec<Dep<R>>, Option<NodeId>) -> Result<NodeId, GraphError>,
@ -353,7 +389,7 @@ impl<N, R> 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);
}

22
hive-jobq/src/lib.rs
View file

@ -458,6 +458,28 @@ impl<N, R> Graph<N, R> {
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<NodeId>,
declare: impl FnOnce(&JobBuilder<N, R>),
) -> Result<std::collections::HashMap<NodeGuid, NodeId>, 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<N, R>> {

22
hive-jobq/src/scheduler.rs
View file

@ -100,23 +100,31 @@ impl<N, R: Clone + Eq + Hash> Scheduler<N, R> {
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<N, R>,
root_parent: Option<NodeId>,
declare: impl FnOnce(&JobBuilder<N, R>),
) -> Result<HashMap<NodeGuid, NodeId>, BuildError> {
let job = JobBuilder::new();
declare(&job);
job.insert_with(root_parent, |payload, deps, parent| {
self.append(payload, deps, parent)
})