jobq: a job asks for the ids it wants back

The operator's instruction on the issue was "the closure returns an array
of guids, and enqueue_job returns the node ids in that order". What was
here instead returned a HashMap of everything inserted, and no caller used
the keys: submit dropped the return, insert_group did into_values(), and
the scheduler ignored what append_subgraph handed back. The guid-keyed
lookup was dead weight, and into_values() made that Vec arbitrarily
ordered -- harmless only because nothing read it.

insert_job now takes FnOnce(&JobBuilder) -> Vec<NodeGuid> and returns the
matching ids positionally. A handle from another job is UnknownNode rather
than a silent omission: the return is positional, so a short vector would
misalign every id after it.

c0re's Declare stays FnOnce(&Job) and the wrapper names no handles in one
place, rather than ending seven templates in an empty vector -- a DAG is
addressed by its container node, which submit inserts itself. That frees
insert_group from needing every id, so the node_rt pre-seeding goes too:
NodeRuntime is one Option field and every reader already tolerated a
missing entry (entry().or_default(), get().and_then(), iter().find()).

The tests are the argument for the shape: capturing a handle through a
mutable binding to look it up in the map afterwards collapses into
returning it and destructuring the result.
This commit is contained in:
atlas 2026-08-02 14:03:37 +02:00 committed by mara
commit bf138ae79a
7 changed files with 147 additions and 76 deletions

View file

@ -57,7 +57,8 @@ hand-maintained per-file tree drifts out of sync with the code.
- **`hive-jobq/`** — persistent job-DAG scheduler, extracted from - **`hive-jobq/`** — persistent job-DAG scheduler, extracted from
hive-c0re's in-tree `job_queue` as a domain-agnostic library. One hive-c0re's in-tree `job_queue` as a domain-agnostic library. One
persistent graph for the whole system (not a DAG per job); enqueuing persistent graph for the whole system (not a DAG per job); enqueuing
inserts a self-contained sub-DAG and returns its node ids. Generic over inserts a self-contained sub-DAG and returns the ids of the nodes the job
asked for, in the order it named them. Generic over
the node payload `N` and the resource name `R`; resource deps are named the node payload `N` and the resource name `R`; resource deps are named
counting semaphores acquired all-or-nothing at node start. `hive-c0re`'s counting semaphores acquired all-or-nothing at node start. `hive-c0re`'s
remaining `job_queue/` module is the c0re-specific layer *over* this remaining `job_queue/` module is the c0re-specific layer *over* this

View file

@ -181,15 +181,18 @@ fn insert_group(
inner: &mut QueueInner, inner: &mut QueueInner,
declare: Declare, declare: Declare,
group_parent: Option<NodeId>, group_parent: Option<NodeId>,
) -> anyhow::Result<Vec<NodeId>> { ) -> anyhow::Result<()> {
let ids = inner inner
.sched .sched
.insert_job(group_parent, declare) .insert_job(group_parent, |b| {
declare(b);
// c0re names no handles: a DAG is addressed by its container node,
// which `submit` inserts itself, and nothing downstream looks an
// individual step up by id.
Vec::new()
})
.map_err(|e| anyhow::anyhow!("job_queue: graph insert failed: {e}"))?; .map_err(|e| anyhow::anyhow!("job_queue: graph insert failed: {e}"))?;
for &id in ids.values() { Ok(())
inner.node_rt.insert(id, NodeRuntime::default());
}
Ok(ids.into_values().collect())
} }
impl JobQueue { impl JobQueue {
@ -255,12 +258,11 @@ impl JobQueue {
/// gate — the children run once `dep_on` reaches `Finishing`. Because the /// gate — the children run once `dep_on` reaches `Finishing`. Because the
/// emitting node stays `Finishing` until this appended subtree is terminal and /// emitting node stays `Finishing` until this appended subtree is terminal and
/// the DAG's terminal node deps on the top root, roll-up keeps the DAG from /// the DAG's terminal node deps on the top root, roll-up keeps the DAG from
/// settling early with no explicit wiring. Returns the new node ids; empty if /// settling early with no explicit wiring. A no-op if the DAG is gone.
/// the DAG is gone or `nodes` is empty. pub fn append_subgraph(&self, dag_id: u64, declare: Declare, dep_on: NodeId) {
pub fn append_subgraph(&self, dag_id: u64, declare: Declare, dep_on: NodeId) -> Vec<NodeId> {
let mut inner = self.lock(); let mut inner = self.lock();
if inner.container(dag_id).is_none() { if inner.container(dag_id).is_none() {
return Vec::new(); return;
} }
// Insert the subgraph as a group rooted under the emitting node: the // Insert the subgraph as a group rooted under the emitting node: the
// subgraph's own root becomes a child of `dep_on`, its steps children of // subgraph's own root becomes a child of `dep_on`, its steps children of
@ -268,20 +270,16 @@ impl JobQueue {
// emitter stays `Finishing` until this appended subtree settles, and the // emitter stays `Finishing` until this appended subtree settles, and the
// container node rolls up terminal only once its whole subtree (incl. this // container node rolls up terminal only once its whole subtree (incl. this
// appended work) has settled, so the DAG hook waits for free. // appended work) has settled, so the DAG hook waits for free.
let ids = match insert_group(&mut inner, declare, Some(dep_on)) { if let Err(e) = insert_group(&mut inner, declare, Some(dep_on)) {
Ok(ids) => ids, tracing::error!(
Err(e) => { dag = dag_id,
tracing::error!( error = %e,
dag = dag_id, "job_queue: append_subgraph insert failed"
error = %e, );
"job_queue: append_subgraph insert failed" return;
); }
return Vec::new();
}
};
drop(inner); drop(inner);
self.notify.notify_one(); self.notify.notify_one();
ids
} }
/// Claim every currently-runnable node, acquiring its resources, and mark it /// Claim every currently-runnable node, acquiring its resources, and mark it

View file

@ -1252,12 +1252,11 @@ fn deploy_apply_grows_rebuild_subgraph_and_finalizes_after_it() {
// BEFORE the emitting node is completed. Completing first would settle the // BEFORE the emitting node is completed. Completing first would settle the
// apply node `Done` with nothing under it, opening the tail's `AfterAny` // apply node `Done` with nothing under it, opening the tail's `AfterAny`
// gate immediately and letting the deploy "finish" before it had built. // gate immediately and letting the deploy "finish" before it had built.
let grown = q.append_subgraph( q.append_subgraph(
id, id,
templates::deploy_rebuild_nodes("agent-a", 11), templates::deploy_rebuild_nodes("agent-a", 11),
apply.node_id, apply.node_id,
); );
assert!(!grown.is_empty(), "subgraph grafted onto the apply node");
q.complete_node(apply.node_id, Ok(())); q.complete_node(apply.node_id, Ok(()));
// The grafted chain runs in rebuild order. `claim_one` asserts exactly one // The grafted chain runs in rebuild order. `claim_one` asserts exactly one

View file

@ -16,8 +16,9 @@ kinds, wires deps, and supplies a runner; the scheduler decides what can start.
## Model ## Model
One **persistent graph** for the whole system, not a DAG per job. Enqueuing One **persistent graph** for the whole system, not a DAG per job. Enqueuing
inserts a self-contained sub-DAG and returns the new node ids; the scheduler inserts a self-contained sub-DAG and returns the ids of the nodes the job
runs a continuous loop, starting every node whose deps are satisfied: *asked* for, in the order it named them; the scheduler runs a continuous loop,
starting every node whose deps are satisfied:
- **Resource deps** are named counting semaphores over a caller-chosen type `R` - **Resource deps** are named counting semaphores over a caller-chosen type `R`
— e.g. `build-slot` (capacity N), `agent/<name>` (capacity 1), or any — e.g. `build-slot` (capacity N), `agent/<name>` (capacity 1), or any

View file

@ -74,12 +74,40 @@ pub enum BuildError {
/// The not-yet-declared parent. /// The not-yet-declared parent.
parent: NodeGuid, parent: NodeGuid,
}, },
/// A handle named in the closure's return value belongs to a different
/// job. Same cause as a foreign handle on an edge: a [`NodeGuid`] outlives
/// the borrow that tied it to its builder, so one can be carried here.
#[error("handle {node:?} names no node in this job")]
UnknownNode {
/// The handle that resolved to nothing.
node: NodeGuid,
},
/// The graph rejected an otherwise well-formed node — an out-of-group edge, /// The graph rejected an otherwise well-formed node — an out-of-group edge,
/// an unsatisfiable [`DepWhen`], and so on. /// an unsatisfiable [`DepWhen`], and so on.
#[error(transparent)] #[error(transparent)]
Graph(#[from] GraphError), 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.
///
/// # Errors
/// [`BuildError::UnknownNode`] for a handle this job never issued.
pub(crate) fn resolve_wanted(
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()
}
/// One node as the builder holds it: edges and parent still name *handles*, so /// One node as the builder holds it: edges and parent still name *handles*, so
/// nothing here depends on ids the graph has not minted yet. /// nothing here depends on ids the graph has not minted yet.
#[derive(Debug)] #[derive(Debug)]
@ -381,39 +409,41 @@ mod tests {
#[test] #[test]
fn edges_resolve_to_minted_ids() { fn edges_resolve_to_minted_ids() {
let mut g = graph(); let mut g = graph();
let mut named = None;
let ids = g let ids = g
.insert_job(None, |b| { .insert_job(None, |b| {
let first = b.node("a"); let first = b.node("a");
let second = b.node("b").after_ok(first); let second = b.node("b").after_ok(first);
named = Some((first.guid(), second.guid())); vec![first.guid(), second.guid()]
}) })
.expect("insert"); .expect("insert");
let (first, second) = named.expect("declared"); let [first, second] = ids[..] else {
panic!("two ids back, in the order asked for")
};
assert_eq!( assert_eq!(
deps_of(&g, ids[&second]), deps_of(&g, second),
vec![Dep::Node { vec![Dep::Node {
id: ids[&first], id: first,
when: DepWhen::AFTER_OK when: DepWhen::AFTER_OK
}] }]
); );
assert!(deps_of(&g, ids[&first]).is_empty()); assert!(deps_of(&g, first).is_empty());
} }
#[test] #[test]
fn parent_resolves_to_a_minted_id() { fn parent_resolves_to_a_minted_id() {
let mut g = graph(); let mut g = graph();
let mut named = None;
let ids = g let ids = g
.insert_job(None, |b| { .insert_job(None, |b| {
let root = b.node("a"); let root = b.node("a");
let child = b.node("b").part_of(root); let child = b.node("b").part_of(root);
named = Some((root.guid(), child.guid())); vec![root.guid(), child.guid()]
}) })
.expect("insert"); .expect("insert");
let (root, child) = named.expect("declared"); let [root, child] = ids[..] else {
assert_eq!(g.node(ids[&root]).expect("root").parent, None); panic!("two ids back")
assert_eq!(g.node(ids[&child]).expect("child").parent, Some(ids[&root])); };
assert_eq!(g.node(root).expect("root").parent, None);
assert_eq!(g.node(child).expect("child").parent, Some(root));
} }
/// A handle is `Copy`, so naming the same node as a dependency twice must /// A handle is `Copy`, so naming the same node as a dependency twice must
@ -421,27 +451,28 @@ mod tests {
#[test] #[test]
fn one_handle_can_be_depended_on_twice() { fn one_handle_can_be_depended_on_twice() {
let mut g = graph(); let mut g = graph();
let mut named = None;
let ids = g let ids = g
.insert_job(None, |b| { .insert_job(None, |b| {
let shared = b.node("a"); let shared = b.node("a");
let ok = b.node("b").after_ok(shared); let ok = b.node("b").after_ok(shared);
let any = b.node("c").after_any(shared); let any = b.node("c").after_any(shared);
named = Some((shared.guid(), ok.guid(), any.guid())); vec![shared.guid(), ok.guid(), any.guid()]
}) })
.expect("insert"); .expect("insert");
let (shared, ok, any) = named.expect("declared"); let [shared, ok, any] = ids[..] else {
panic!("three ids back")
};
assert_eq!( assert_eq!(
deps_of(&g, ids[&ok]), deps_of(&g, ok),
vec![Dep::Node { vec![Dep::Node {
id: ids[&shared], id: shared,
when: DepWhen::AFTER_OK when: DepWhen::AFTER_OK
}] }]
); );
assert_eq!( assert_eq!(
deps_of(&g, ids[&any]), deps_of(&g, any),
vec![Dep::Node { vec![Dep::Node {
id: ids[&shared], id: shared,
when: DepWhen::AFTER_ANY when: DepWhen::AFTER_ANY
}] }]
); );
@ -451,20 +482,21 @@ mod tests {
#[test] #[test]
fn resources_become_resource_deps() { fn resources_become_resource_deps() {
let mut g = graph(); let mut g = graph();
let mut named = None;
let ids = g let ids = g
.insert_job(None, |b| { .insert_job(None, |b| {
named = Some( vec![
b.node("a") b.node("a")
.needs("agent/atlas") .needs("agent/atlas")
.needs_units("build", 2) .needs_units("build", 2)
.guid(), .guid(),
); ]
}) })
.expect("insert"); .expect("insert");
let only = named.expect("declared"); let [only] = ids[..] else {
panic!("one id back")
};
assert_eq!( assert_eq!(
deps_of(&g, ids[&only]), deps_of(&g, only),
vec![ vec![
Dep::Resource { Dep::Resource {
name: "agent/atlas", name: "agent/atlas",
@ -492,6 +524,9 @@ mod tests {
let second = b.node("b"); let second = b.node("b");
let _ = first.after_any(second); let _ = first.after_any(second);
named = Some((first.guid(), second.guid())); named = Some((first.guid(), second.guid()));
// The insert fails, so nothing comes back to ask for — the
// handles under test travel out through `named` instead.
Vec::new()
}) })
.expect_err("forward edge"); .expect_err("forward edge");
let (first, second) = named.expect("declared"); let (first, second) = named.expect("declared");
@ -514,6 +549,7 @@ mod tests {
let parent = b.node("b"); let parent = b.node("b");
let _ = child.part_of(parent); let _ = child.part_of(parent);
named = Some((child.guid(), parent.guid())); named = Some((child.guid(), parent.guid()));
Vec::new()
}) })
.expect_err("forward parent"); .expect_err("forward parent");
let (child, parent) = named.expect("declared"); let (child, parent) = named.expect("declared");
@ -539,6 +575,7 @@ mod tests {
let mut foreign = None; let mut foreign = None;
g.insert_job(None, |b| { g.insert_job(None, |b| {
foreign = Some(b.node("first job").guid()); foreign = Some(b.node("first job").guid());
Vec::new()
}) })
.expect("first job inserts"); .expect("first job inserts");
let foreign = foreign.expect("declared"); let foreign = foreign.expect("declared");
@ -546,6 +583,7 @@ mod tests {
let err = g let err = g
.insert_job(None, |b| { .insert_job(None, |b| {
let _ = b.node("second job").after_ok(foreign); let _ = b.node("second job").after_ok(foreign);
Vec::new()
}) })
.expect_err("foreign handle"); .expect_err("foreign handle");
assert!( assert!(
@ -565,6 +603,7 @@ mod tests {
// A child may not depend on its own parent: the parent gate // A child may not depend on its own parent: the parent gate
// already orders them, and the edge would deadlock. // already orders them, and the edge would deadlock.
let _ = b.node("child").part_of(root).after_ok(root); let _ = b.node("child").part_of(root).after_ok(root);
Vec::new()
}) })
.expect_err("out-of-group dep"); .expect_err("out-of-group dep");
assert!(matches!(err, BuildError::Graph(_)), "{err:?}"); assert!(matches!(err, BuildError::Graph(_)), "{err:?}");
@ -578,23 +617,47 @@ mod tests {
let mut g = graph(); let mut g = graph();
let container = g.insert("container", Vec::new(), None).expect("container"); let container = g.insert("container", Vec::new(), None).expect("container");
let mut named = None;
let ids = g let ids = g
.insert_job(Some(container), |b| { .insert_job(Some(container), |b| {
let root = b.node("root"); let root = b.node("root");
let child = b.node("child").part_of(root); let child = b.node("child").part_of(root);
named = Some((root.guid(), child.guid())); vec![root.guid(), child.guid()]
}) })
.expect("insert"); .expect("insert");
let (root, child) = named.expect("declared"); let [root, child] = ids[..] else {
assert_eq!(g.node(ids[&root]).expect("root").parent, Some(container)); panic!("two ids back")
assert_eq!(g.node(ids[&child]).expect("child").parent, Some(ids[&root])); };
assert_eq!(g.node(root).expect("root").parent, Some(container));
assert_eq!(g.node(child).expect("child").parent, Some(root));
}
/// A handle that names no node in *this* job is refused rather than
/// silently dropped from the returned ids — the return is positional, so a
/// short vector would misalign every id after it.
#[test]
fn asking_for_a_foreign_handle_is_an_error() {
let mut g = graph();
let mut foreign = None;
g.insert_job(None, |b| {
foreign = Some(b.node("first job").guid());
Vec::new()
})
.expect("first job inserts");
let foreign = foreign.expect("declared");
let err = g
.insert_job(None, |b| {
let _ = b.node("second job");
vec![foreign]
})
.expect_err("foreign handle asked for");
assert_eq!(err, BuildError::UnknownNode { node: foreign });
} }
#[test] #[test]
fn an_empty_builder_inserts_nothing() { fn an_empty_builder_inserts_nothing() {
let mut g = graph(); let mut g = graph();
let ids = g.insert_job(None, |_| {}).expect("insert"); let ids = g.insert_job(None, |_| Vec::new()).expect("insert");
assert!(ids.is_empty()); assert!(ids.is_empty());
assert_eq!(g.nodes().count(), 0); assert_eq!(g.nodes().count(), 0);
} }

View file

@ -458,26 +458,33 @@ impl<N, R> Graph<N, R> {
Ok(id) Ok(id)
} }
/// Insert a whole job under `root_parent`, returning the id each handle's /// Insert a whole job under `root_parent`, returning the ids of the nodes
/// node was minted as. /// `declare` **asked for**, in the order it named them.
/// ///
/// `declare` receives a fresh [`JobBuilder`] and names the job's nodes on /// `declare` receives a fresh [`JobBuilder`], names the job's nodes on it,
/// it; the builder never leaves this call, so a job cannot be built in one /// and returns the handles whose ids it wants back. The builder never
/// place and inserted in another. The graph-only counterpart of /// 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 /// [`crate::scheduler::Scheduler::insert_job`] — prefer that one when a
/// scheduler owns the graph, since it also records what it started. /// 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 /// # Errors
/// Propagates [`BuildError`] — a forward reference in the job's own /// Propagates [`BuildError`] — a forward reference in the job's own
/// declarations, a handle from a different job, or a graph rejection. /// declarations, a handle from a different job, or a graph rejection.
pub fn insert_job( pub fn insert_job(
&mut self, &mut self,
root_parent: Option<NodeId>, root_parent: Option<NodeId>,
declare: impl FnOnce(&JobBuilder<N, R>), declare: impl FnOnce(&JobBuilder<N, R>) -> Vec<NodeGuid>,
) -> Result<std::collections::HashMap<NodeGuid, NodeId>, BuildError> { ) -> Result<Vec<NodeId>, BuildError> {
let job = JobBuilder::new(); let job = JobBuilder::new();
declare(&job); let wanted = declare(&job);
job.insert_into(self, root_parent) let ids = job.insert_into(self, root_parent)?;
crate::builder::resolve_wanted(&wanted, &ids)
} }
/// Borrow a node by id. /// Borrow a node by id.

View file

@ -103,11 +103,12 @@ impl<N, R: Clone + Eq + Hash> Scheduler<N, R> {
/// Insert a whole job under `root_parent`, returning the id each handle's /// Insert a whole job under `root_parent`, returning the id each handle's
/// node was minted as. /// node was minted as.
/// ///
/// `declare` receives a fresh [`JobBuilder`] and names the job's nodes on /// `declare` receives a fresh [`JobBuilder`], names the job's nodes on it,
/// it; the builder never leaves this call. That is the whole insertion /// and returns the handles whose ids it wants back — they come back in
/// API — a caller cannot construct a builder, hold one, or insert one /// that order. The builder never leaves this call. That is the whole
/// itself, so there is no way to end up with a job-shaped value being /// insertion API — a caller cannot construct a builder, hold one, or
/// passed around as a spec. /// 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 /// The scheduler-side counterpart of [`Graph::insert_job`]: same
/// resolution, but each node goes through [`Scheduler::append`], so a /// resolution, but each node goes through [`Scheduler::append`], so a
@ -121,13 +122,14 @@ impl<N, R: Clone + Eq + Hash> Scheduler<N, R> {
pub fn insert_job( pub fn insert_job(
&mut self, &mut self,
root_parent: Option<NodeId>, root_parent: Option<NodeId>,
declare: impl FnOnce(&JobBuilder<N, R>), declare: impl FnOnce(&JobBuilder<N, R>) -> Vec<NodeGuid>,
) -> Result<HashMap<NodeGuid, NodeId>, BuildError> { ) -> Result<Vec<NodeId>, BuildError> {
let job = JobBuilder::new(); let job = JobBuilder::new();
declare(&job); let wanted = declare(&job);
job.insert_with(root_parent, |payload, deps, parent| { let ids = job.insert_with(root_parent, |payload, deps, parent| {
self.append(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 /// Claim every currently-runnable pending node and start it: node-deps