`Graph::insert` was public and validating, and the builder's insert loop
called it with `?`. That made job-level atomicity an accident: the loop
mutates as it goes, so a rejection at node `i` left `0..i` already in the
graph — the error fired loudly *after* the corruption, not instead of it.
It held only because `check_job_shape` happens to be exhaustive, with
nothing in the types saying so.
Make the guarantee structural instead. `Graph::insert` becomes
`pub(crate)`; the builder drains into a new infallible
`insert_unchecked` (via `Scheduler::append_unchecked`), so
`insert_with`'s sink returns a bare `NodeId` and a half-built job is no
longer expressible. Re-checking at the sink cannot add safety anyway — it
can only report after the mutation it was meant to prevent.
Drop `BuildError::Graph`: nothing in the builder path can produce a
`GraphError` any more. Clippy could not see this (an unreachable variant
of a `pub` enum is still constructible from outside the crate).
`Scheduler::append` stays public and validating — hive-c0re inserts a
DAG's container node through it. Folding that away means removing the
container/DagView indirection, which is out of scope here.
Follow-up to the operator's note that an unchecked insert is fine "as
long as the builder enforces all invariants". It didn't, so this makes
the claim true rather than assumed.
check_job_shape now decides everything Graph::insert can reject for a
builder-produced node:
- UnknownParent / UnknownDep were already impossible -- a handle only
exists if this job declared it, and the ids are minted during the
insert itself.
- DepOutsideParent was not. The grouping rule is now re-derived from the
job's own parent chains: a depender parented at `q` may only name a
proper descendant of `q` (never `q` itself, which would deadlock), and
a depender that declared no parent inherits root_parent -- so with a
container every job node qualifies, and without one the target must
also be top-level. Mirrors Graph::is_descendant, which starts at the
target's parent and so never treats a node as its own ancestor.
It also rejects an empty DepWhen, which the graph only catches when
validating a deserialized graph (Graph::validate, not insert). Such a
node inserts cleanly today and then never becomes runnable -- a silent
hang. Refusing it at declaration closes that on the way past.
Graph::insert stays the sink. The atomicity comes from the pre-pass
being complete, not from bypassing validation, and keeping the graph's
own checks means any future drift between the two copies of the
grouping rule surfaces as a loud BuildError::Graph instead of silently
corrupting the graph -- one branch per node for a backstop.
graph_rejection_surfaces_as_is asserted that the graph's rejection
surfaced through the builder. That case no longer reaches the graph, so
it now pins the stronger property: the error is DepOutsideGroup *and*
nothing was inserted. Same for the new empty-edge test.
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.
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.
Third time the operator asked for a guid and got a substitute: first an
i64 index, then a {random job id, per-builder counter} pair. The pair was
defensible in isolation -- a foreign handle misses rather than colliding,
with no new dependency -- but "an equivalent that avoids a dep" is a
counter-proposal, not an implementation.
It is also simpler as a guid, which was the question asked: NodeGuid(Uuid)
drops the `job` field, the `next_seq` counter, `fresh_job_id()` and the
`Cell` import, and halves the type's doc. One random draw per node rather
than one per builder -- noise next to what a node does when it runs.
uuid 1.24 was already in Cargo.lock as a transitive dependency, so this
adds an edge rather than a package.
Follows the jobq change: a builder can no longer be constructed or
inserted outside `hive_jobq`, so `DagSpec` cannot hold one. It carries a
`Declare` — `Box<dyn FnOnce(&Job) + Send>` — and the queue runs it
against a builder jobq owns, at the moment it inserts.
`NodeOutput.append_subgraph` becomes `Vec<Declare>` for the same reason,
and this is where the shape was always heading: that field's doc already
said an executor "cannot reach the queue, so it hands the declaration
back", while its type was a `Vec<Job>` the executor had built itself.
The rejected `build_nodes -> Vec<NodeSpec>` was the first version of that
escape hatch; a recipe is the last one, because there is no job-shaped
value to hand over at all.
Templates and the power-op assemblers move their owned data into the
closure and are otherwise unchanged — `rebuild_nodes`, `node` and the
tail helpers already took `&Job` and returned handles, so only each
template's outermost frame moved.
Two `Debug` impls are hand-written: a closure has nothing to show, and
its nodes do not exist until the queue runs it. `NodeOutput` reports how
many subgraphs were emitted, `DagSpec` its source and reason.
`append_subgraph`'s `is_empty()` early-return is gone — you cannot ask a
recipe whether it will declare anything without running it. It now
inserts and returns an empty id list if nothing was declared, which
takes the queue lock in a case that previously skipped it.
The two in-DAG-growth tests build `Declare`s now, so they exercise the
shape an executor actually produces rather than one only a test could
construct. 45 job-queue tests unchanged and passing.
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.
Every template built a `Vec<NodeSpec>` whose edges and parents were
positional indices into that vector, so a shape was expressed as
arithmetic: `base + 1`, `stop_root + 2`, `sfu + 1`, and a
`reconcile_index()` helper that read the emitted vector's length to find
out where its own last node had landed. `concat_subgraphs` existed
solely to rebase one per-agent subgraph's indices onto another's.
Templates now declare into a `hive_jobq::JobBuilder` and hold the
handles they get back, so an edge names the node it waits on. The
arithmetic is gone, and with it:
- `NodeSpec` and the job-queue's own index-based `Dep`.
- `insert_group`'s index resolution — it wraps `Scheduler::insert_job`.
- `concat_subgraphs` — per-agent chains share one builder and each keeps
its own root, so independence is structural rather than computed.
- `reconcile_index` and `dep_index`.
- `templates::validate` and its petgraph toposort. It rejected dangling
deps and cycles; both are now unrepresentable, since a handle only
exists for an already-declared node and every edge therefore points
backwards. (petgraph stays in the tree for `agent_config::topology`.)
`NodeOutput.append_subgraph` becomes `Vec<Job>`: an executor cannot
reach the queue, so it hands back declarations and the scheduler inserts
them under its own lock. That is what the in-DAG growth path always
wanted — a transferable declaration, not a vector of specs.
Resource declaration is unchanged in behaviour: the `templates::node`
helper applies `NodeKind::resource_deps()` at the construction site, so
every node still declares what its kind needs. Moving that declaration
to the call sites is #2818's job; this leaves it one place to delete.
Three tests went with the guard they covered — they hand-built malformed
specs out of indices, which is the representation that made those shapes
possible. Two more now read a DAG's shape off the queue rather than out
of a spec vector, which is where it is observable. The remaining 45
job-queue tests are unchanged and still pass: lease serialization,
roll-up, cancel-cascade, in-DAG growth and per-agent concurrency all
behave as before.
`JobBuilder::node(payload)` hands back a `NodeRef` handle obtainable no
other way, and edges are handle -> handle. `insert_into(graph,
root_parent)` consumes the builder, inserts in declaration order, and
returns the id each handle was minted as. There is no intermediate
node-description type: the builder inserts through `Graph::insert`
directly, so nothing has to stay in sync with that signature.
`root_parent` is the group's attachment point — a node that declared no
parent hangs there. That is what makes a job a self-contained sub-DAG: a
template is written without knowing which container node it will live
under, and the same builder serves a runtime-emitted subgraph hanging
off its emitting node.
Generic over the same `N`/`R` as `Graph`, so it belongs to the library
rather than to any one caller's node kind. `.needs(name)` /
`.needs_units(name, count)` declare resource deps at the construction
site, next to the node that needs them.
`Scheduler::insert_job` is the same over a scheduler, via the shared
`insert_with` sink, so a caller building a job never reaches past the
scheduler at the graph underneath.
Declaration order is enforced rather than papered over: a node
referencing one declared later is a named `BuildError::ForwardEdge` /
`ForwardParent`. Sorting for the caller would silently accept a shape
the graph cannot express, and would put ordering logic in a second
place.
Additive — `Graph` is untouched.
