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

jobq: one insertion entry point, and make it atomic

Browse source 
Three findings from the operator's review, all correct.

1. Two insert_job's. Graph::insert_job had no caller outside hive-jobq's
   own tests -- production only ever went through Scheduler::insert_job.
   It existed because the graph-level one got written first. Deleted; the
   tests moved onto a Scheduler, which is where insertion belongs anyway.

2. insert_job was not atomic, and the previous commit made that worse: a
   forward edge or forward parent surfaced mid-loop, leaving the nodes
   before it in the graph, and resolve_wanted ran after every insert, so
   an unknown handle failed once the whole job was already committed.
   The module documented this under "Partial insertion" instead of fixing
   it -- prose describing a hole is not a design.

   All three are decidable from what the builder holds, so
   check_declaration_order now runs before the first insert and the loop
   indexes ids directly. A malformed job leaves the graph untouched.
   What remains mid-insert is the graph's own rejection (out-of-group
   dep, empty DepWhen); closing that needs a dry-run validate on Graph,
   which is a separate change.

3. DagSpec no longer boxes its recipe: it is generic over the closure,
   which travels from the template that built it straight into submit.
   The box bought type inference, and paying for it costs annotations --
   `|b: &Job|` at each declaration site (the field needs an HRTB, and an
   unannotated closure binds one lifetime) and `+ use<>` on each
   returning signature (or the opaque type captures the caller's borrows).
   Erasure is still needed where several recipe shapes share one type:
   the boxed Declare stays for the executor's append_subgraph, and a test
   table uses an erase() helper.
This commit is contained in:
atlas 2026-08-02 14:39:37 +02:00 • committed by mara
commit f035b63b9a
8 changed files with 211 additions and 176 deletions
Download patch file Download diff file Expand all files Collapse all files

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

@ -6,15 +6,15 @@
//! get wrong.
//!
//! **An insertion API, not a spec factory.** 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 the graph minted. It cannot be constructed, held
//! closure by the single insertion entry point
//! ([`crate::scheduler::Scheduler::insert_job`]), which inserts the declared
//! nodes and returns the ids the job asked for. It cannot be constructed, held
//! or inserted from outside this crate, and there is no intermediate
//! node-description type to keep in sync with [`Graph::insert`]'s signature —
//! node-description type to keep in sync with [`crate::Graph::insert`]'s signature —
//! so a job has no representation that can be passed around instead of being
//! inserted.
//!
//! **Payload-agnostic.** Generic over the same `N` and `R` as [`Graph`]: the
//! **Payload-agnostic.** Generic over the same `N` and `R` as [`crate::Graph`]: the
//! builder knows nothing about what a node *does*, only how nodes relate.
//!
//! # Declaration order
@ -29,7 +29,7 @@
use std::cell::RefCell;
use std::collections::HashMap;
use crate::{Dep, DepWhen, Graph, GraphError, NodeId, TerminalState};
use crate::{Dep, DepWhen, GraphError, NodeId, TerminalState};
/// An opaque identity for a node **within the job being built**.
///
@ -88,24 +88,48 @@ pub enum BuildError {
Graph(#[from] GraphError),
}
/// Look each handle a job asked for up in what the insert actually minted,
/// preserving the order it asked in — the last step of both insertion entry
/// points.
/// Reject a job whose own declarations don't hold up — **before anything is
/// inserted**, so these three failures cannot leave a partial job behind.
///
/// Each is decidable from what the builder already holds: a node may only name
/// handles declared before it, and a job may only ask for handles it declared.
/// Running this first is what lets the insert loop index `ids` directly instead
/// of discovering a bad reference halfway through mutating the graph.
///
/// # Errors
/// [`BuildError::UnknownNode`] for a handle this job never issued.
pub(crate) fn resolve_wanted(
/// [`BuildError::ForwardEdge`] / [`BuildError::ForwardParent`] for a reference
/// to a later node, [`BuildError::UnknownNode`] for a requested handle this job
/// never declared.
fn check_declaration_order<N, R>(
pending: &[Pending<N, R>],
wanted: &[NodeGuid],
ids: &HashMap<NodeGuid, NodeId>,
) -> Result<Vec<NodeId>, BuildError> {
wanted
.iter()
.map(|g| {
ids.get(g)
.copied()
.ok_or(BuildError::UnknownNode { node: *g })
})
.collect()
) -> Result<(), BuildError> {
let mut declared: std::collections::HashSet<NodeGuid> = std::collections::HashSet::new();
for node in pending {
for (dep, _) in &node.deps {
if !declared.contains(dep) {
return Err(BuildError::ForwardEdge {
node: node.guid,
dep: *dep,
});
}
}
if let Some(parent) = node.parent
&& !declared.contains(&parent)
{
return Err(BuildError::ForwardParent {
node: node.guid,
parent,
});
}
declared.insert(node.guid);
}
for guid in wanted {
if !declared.contains(guid) {
return Err(BuildError::UnknownNode { node: *guid });
}
}
Ok(())
}
/// One node as the builder holds it: edges and parent still name *handles*, so
@ -143,8 +167,8 @@ impl<N, R> JobBuilder<N, R> {
/// A fresh, empty builder.
///
/// **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
/// a closure by the single insertion entry point
/// ([`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.
@ -195,64 +219,35 @@ 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(crate) fn insert_into(
self,
graph: &mut Graph<N, R>,
root_parent: Option<NodeId>,
) -> Result<HashMap<NodeGuid, NodeId>, BuildError> {
self.insert_with(root_parent, |payload, deps, parent| {
graph.insert(payload, deps, parent)
})
}
/// [`JobBuilder::insert_into`] against an arbitrary sink — the same
/// resolution, for a caller that inserts through something wrapping the
/// graph (e.g. [`crate::scheduler::Scheduler::insert_job`], which has
/// bookkeeping of its own to do per node).
///
/// # Errors
///
/// As [`JobBuilder::insert_into`].
///
/// # Partial insertion
///
/// An error leaves the nodes inserted *before* it in the sink. Callers that
/// 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.
/// rejects a node (see [`crate::Graph::insert`]).
pub(crate) fn insert_with(
self,
root_parent: Option<NodeId>,
wanted: &[NodeGuid],
mut insert: impl FnMut(N, Vec<Dep<R>>, Option<NodeId>) -> Result<NodeId, GraphError>,
) -> Result<HashMap<NodeGuid, NodeId>, BuildError> {
) -> Result<Vec<NodeId>, BuildError> {
let pending = self.nodes.into_inner();
check_declaration_order(&pending, wanted)?;
let mut ids: HashMap<NodeGuid, NodeId> = HashMap::new();
for pending in self.nodes.into_inner() {
let parent = match pending.parent {
for node in pending {
let parent = match node.parent {
None => root_parent,
Some(p) => Some(*ids.get(&p).ok_or(BuildError::ForwardParent {
node: pending.guid,
parent: p,
})?),
Some(p) => Some(ids[&p]),
};
let mut deps: Vec<Dep<R>> = Vec::with_capacity(pending.deps.len());
for (on, when) in pending.deps {
let id = *ids.get(&on).ok_or(BuildError::ForwardEdge {
node: pending.guid,
dep: on,
})?;
deps.push(Dep::Node { id, when });
let mut deps: Vec<Dep<R>> = Vec::with_capacity(node.deps.len());
for (on, when) in node.deps {
deps.push(Dep::Node { id: ids[&on], when });
}
deps.extend(
pending
.resources
node.resources
.into_iter()
.map(|(name, count)| Dep::Resource { name, count }),
);
let id = insert(pending.payload, deps, parent)?;
ids.insert(pending.guid, id);
let id = insert(node.payload, deps, parent)?;
ids.insert(node.guid, id);
}
Ok(ids)
Ok(wanted.iter().map(|g| ids[g]).collect())
}
/// Apply `f` to the node named by `guid`.
@ -393,22 +388,25 @@ impl<N, R> NodeRef<'_, N, R> {
#[cfg(test)]
mod tests {
use super::BuildError;
use crate::resources::ResourceTable;
use crate::scheduler::Scheduler;
use crate::{Dep, DepWhen, Graph, NodeId};
/// A graph whose payload is a name and whose resources are strings.
fn graph() -> Graph<&'static str, &'static str> {
Graph::new()
/// A scheduler over a graph whose payload is a name and whose resources are
/// strings — the only way in, since insertion is a scheduler operation.
fn sched() -> Scheduler<&'static str, &'static str> {
Scheduler::new(Graph::new(), ResourceTable::new())
}
fn deps_of(g: &Graph<&'static str, &'static str>, id: NodeId) -> Vec<Dep<&'static str>> {
g.node(id).expect("node present").deps.clone()
fn deps_of(g: &Scheduler<&'static str, &'static str>, id: NodeId) -> Vec<Dep<&'static str>> {
g.graph().node(id).expect("node present").deps.clone()
}
/// The point of the handle layer: an edge declared against a *handle* comes
/// out addressing the id that node was actually minted as.
#[test]
fn edges_resolve_to_minted_ids() {
let mut g = graph();
let mut g = sched();
let ids = g
.insert_job(None, |b| {
let first = b.node("a");
@ -431,7 +429,7 @@ mod tests {
#[test]
fn parent_resolves_to_a_minted_id() {
let mut g = graph();
let mut g = sched();
let ids = g
.insert_job(None, |b| {
let root = b.node("a");
@ -442,15 +440,15 @@ mod tests {
let [root, child] = ids[..] else {
panic!("two ids back")
};
assert_eq!(g.node(root).expect("root").parent, None);
assert_eq!(g.node(child).expect("child").parent, Some(root));
assert_eq!(g.graph().node(root).expect("root").parent, None);
assert_eq!(g.graph().node(child).expect("child").parent, Some(root));
}
/// A handle is `Copy`, so naming the same node as a dependency twice must
/// not consume it — the fan-out every composite job needs.
#[test]
fn one_handle_can_be_depended_on_twice() {
let mut g = graph();
let mut g = sched();
let ids = g
.insert_job(None, |b| {
let shared = b.node("a");
@ -481,7 +479,7 @@ mod tests {
/// Resource deps ride along with the node deps, in one insert.
#[test]
fn resources_become_resource_deps() {
let mut g = graph();
let mut g = sched();
let ids = g
.insert_job(None, |b| {
vec![
@ -516,7 +514,7 @@ mod tests {
/// of quietly reordering.
#[test]
fn a_forward_edge_is_rejected_by_name() {
let mut g = graph();
let mut g = sched();
let mut named = None;
let err = g
.insert_job(None, |b| {
@ -541,7 +539,7 @@ mod tests {
#[test]
fn a_forward_parent_is_rejected_by_name() {
let mut g = graph();
let mut g = sched();
let mut named = None;
let err = g
.insert_job(None, |b| {
@ -571,7 +569,7 @@ mod tests {
/// 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 g = sched();
let mut foreign = None;
g.insert_job(None, |b| {
foreign = Some(b.node("first job").guid());
@ -596,7 +594,7 @@ mod tests {
/// pre-empt it.
#[test]
fn graph_rejection_surfaces_as_is() {
let mut g = graph();
let mut g = sched();
let err = g
.insert_job(None, |b| {
let root = b.node("root");
@ -614,8 +612,8 @@ mod tests {
/// template be written without knowing the container it will live under.
#[test]
fn root_parent_adopts_only_the_jobs_own_roots() {
let mut g = graph();
let container = g.insert("container", Vec::new(), None).expect("container");
let mut g = sched();
let container = g.append("container", Vec::new(), None).expect("container");
let ids = g
.insert_job(Some(container), |b| {
@ -627,8 +625,8 @@ mod tests {
let [root, child] = ids[..] else {
panic!("two ids back")
};
assert_eq!(g.node(root).expect("root").parent, Some(container));
assert_eq!(g.node(child).expect("child").parent, Some(root));
assert_eq!(g.graph().node(root).expect("root").parent, Some(container));
assert_eq!(g.graph().node(child).expect("child").parent, Some(root));
}
/// A handle that names no node in *this* job is refused rather than
@ -636,7 +634,7 @@ mod tests {
/// short vector would misalign every id after it.
#[test]
fn asking_for_a_foreign_handle_is_an_error() {
let mut g = graph();
let mut g = sched();
let mut foreign = None;
g.insert_job(None, |b| {
foreign = Some(b.node("first job").guid());
@ -656,9 +654,9 @@ mod tests {
#[test]
fn an_empty_builder_inserts_nothing() {
let mut g = graph();
let mut g = sched();
let ids = g.insert_job(None, |_| Vec::new()).expect("insert");
assert!(ids.is_empty());
assert_eq!(g.nodes().count(), 0);
assert_eq!(g.graph().nodes().count(), 0);
}
}

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

@ -458,35 +458,6 @@ impl<N, R> Graph<N, R> {
Ok(id)
}
/// Insert a whole job under `root_parent`, returning the ids of the nodes
/// `declare` **asked for**, in the order it named them.
///
/// `declare` receives a fresh [`JobBuilder`], names the job's nodes on it,
/// and returns the handles whose ids it wants back. 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.
///
/// Asking is how a caller addresses a node it created: the alternative — a
/// map of everything inserted, or a positional vector — either hands back a
/// lookup nobody performs or reintroduces the counting this API exists to
/// remove.
///
/// # 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>) -> Vec<NodeGuid>,
) -> Result<Vec<NodeId>, BuildError> {
let job = JobBuilder::new();
let wanted = declare(&job);
let ids = job.insert_into(self, root_parent)?;
crate::builder::resolve_wanted(&wanted, &ids)
}
/// Borrow a node by id.
#[must_use]
pub fn node(&self, id: NodeId) -> Option<&Node<N, R>> {

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

@ -110,15 +110,19 @@ impl<N, R: Clone + Eq + Hash> Scheduler<N, R> {
/// 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 never has to reach past the scheduler at the graph underneath.
/// Call [`Scheduler::settle`] afterwards to start whatever became
/// runnable.
/// The one insertion entry point: every node goes through
/// [`Scheduler::append`], so a caller never has to reach past the scheduler
/// at the graph underneath. Call [`Scheduler::settle`] afterwards to start
/// whatever became runnable.
///
/// **Atomic in the job's own shape.** A forward edge, a forward parent, or
/// a request for a handle this job never declared is rejected *before* the
/// first node is inserted, so a malformed job leaves the graph untouched
/// rather than half-built.
///
/// # Errors
/// Propagates [`BuildError`] — a forward reference in the job's own
/// declarations, or a graph rejection.
/// declarations, a handle from a different job, or a graph rejection.
pub fn insert_job(
&mut self,
root_parent: Option<NodeId>,
@ -126,10 +130,9 @@ impl<N, R: Clone + Eq + Hash> Scheduler<N, R> {
) -> Result<Vec<NodeId>, BuildError> {
let job = JobBuilder::new();
let wanted = declare(&job);
let ids = job.insert_with(root_parent, |payload, deps, parent| {
job.insert_with(root_parent, &wanted, |payload, deps, parent| {
self.append(payload, deps, parent)
})?;
crate::builder::resolve_wanted(&wanted, &ids)
})
}
/// Claim every currently-runnable pending node and start it: node-deps