Nothing in the gate read doc-comments: clippy doesn't check intra-doc
links, cargo test doesn't, and no check built docs. So a [`Foo`] pointing
at a renamed, moved or deleted item rendered as plain text and had no
discoverer but a human happening to read the comment.
That matters here more than in most repos, because the convention is to
put a thing's authoritative description in one doc-comment and point at
it from everywhere else -- the design leans on the pointers being real,
and a dangling link is worse than no link since it names something and
sends the reader looking.
Adds `docs-rustdoc` to nix/checks.nix: craneLib.cargoDoc over
--workspace --no-deps --document-private-items, denying six rustdoc
lints. Listed explicitly rather than -D warnings so a new lint appearing
upstream cannot red the build on a class nobody has triaged.
--document-private-items is load-bearing rather than thoroughness for
its own sake: most of this workspace's doc-comments live on private
items and //! module headers, so without it rustdoc checks a small
fraction of the links and the gate sits green while the rot continues.
Then fixes every error it reports, 40 to 0 across nine crates. The
classes differ and so do the fixes:
- public item, wrong scope -> qualify. Node and Node::parent are both
public; the link failed only because scheduler.rs does not import
Node. Six sites become [`crate::Node::parent`].
- private item -> downgrade to backticks. Nothing was made public to
satisfy a lint; changing API surface to appease a doc check would be
the tail wagging the dog.
- genuinely dead -> [`JobBuilder::insert_into`] names a method that does
not exist. Insertion is Scheduler::insert_job.
- prose that looks like markup -> argv[0] parsed as a link, and
<args>/<hex>/<name> parsed as HTML tags.
Note for future fixes: pub(crate) resolves in an intra-doc link, a plain
private fn in a binary crate does not (wait_for_nodes resolved,
connect_hint did not, same crate, same shape).
The check does not ride the clippy/test artifact cache. It takes
cargoArtifacts, but rustdoc needs its own flavour of dependency
metadata, which cargo build does not produce, so a --no-deps docs build
still compiles dependencies it never documents. Measured at 6m47s cold;
that reasoning is recorded in the check's own comment so the next reader
does not re-derive it.
Verified by running the check's exact command against the pre-cleanup
tree first: 40 errors, build failed. A gate that cannot fail is not
evidence, and building it before the cleanup makes that proof free.
Six more, in the crate's standalone README: the summary, the model section,
NodeId, the Graph entry and the Node serde note. Same wrong claim as the
module doc, in the sibling file the first sweep didn't look at.
A crate's README and its //! module doc are the same document in two files,
so a fix that touches one and not the other is the default outcome rather
than an unlucky miss.
The crate described itself as a persistent scheduler and NodeId promised
stability across restarts. Neither is true: hive-c0re constructs an empty
Graph on every boot and re-derives desired state with its reconcile sweep,
and nothing in the workspace writes or loads a graph. hive-c0re's own
job_queue module doc has said "runtime-only (no persistence)" all along —
only the extracted library's prose drifted.
Seven claims corrected across the module doc, NodeId, the id-counter error
and the Graph type, plus the repo map. The module doc now states the fact
positively rather than just dropping the word: serde exists so the graph can
be projected onto a wire and so a store could be added later, ids and
timestamps are stable within a run.
NodeId spells out the consequence, since that is the part that could mislead
someone: an id stored outside the process is a historical record, not a
handle that will resolve after a restart.
`Node` carried two of the three lifecycle timestamps; the third lived on
hive-c0re's `NodeKind::Dag` container payload, a core-specific wrapper the
graph knows nothing about. Give it its real home so the container's copy
becomes redundant rather than load-bearing.
Not an `Option` like its neighbours: starting and finishing are events that
may never happen, but a node that exists was created. Modelling it as
optional would encode a state the graph cannot be in.
`hive-jobq-wire::GraphNode` gains the field in the same commit — it already
carries the other two, and without this one the value cannot reach a viewer
when the container's copy is deleted.
Three things, all from review:
Accepted outcomes were built from a hand-listed [Done, Failed, Cancelled,
Skipped] array. Exhaustive today, silently short the day someone adds a
variant — the new outcome would vanish from every edge that accepts it.
BitFlags::ALL asks the type instead.
TerminalState carried rename_all = "snake_case" while its sibling State did
not, so one enum shipped "done" and the other "Done". A rename is a second
spelling of a name that then has to be kept in agreement by hand; both now
serialise their variant names verbatim. Nothing else reads TerminalState off
a wire, so no consumer moves. GraphDep's tag values likewise.
The endpoint documented its body as serde_json::Value, which tells a spec
reader nothing. hive-jobq-wire now derives ToSchema. State and TerminalState
are foreign types here and utoipa stays out of the scheduler crate, so the
schema points at local mirror enums. A mirror that drifts is worse than none:
the conversions are exhaustive (a new upstream variant fails the build) and a
test asserts each documented name equals the serialised one, since an
exhaustive match still compiles when only the spellings diverge.
`submit` used to complete the container node by hand, right after
inserting it, so it would park in `Finishing` and its children unblock.
That was the last caller of `Scheduler::complete` outside the crate, and
the justification was that the container "never needs claiming or
executing".
It does, though, in the sense that matters: it is a node with no logic of
its own, and the scheduler already knows what to do with one. It declares
no resources, so it is claimable the moment it is inserted; `run_node`'s
`Dag` arm already returns `Ok(())`, exactly as it does for `DeployWindow`,
which is the same shape and was never special-cased. Deleting the inline
completion costs one claim round-trip and removes the only reason the
crate had to expose completion at all.
`complete` is `pub(crate)` now. Completion is reachable only from inside
the future `claim_next` hands back, so a node cannot be finished without
the claim it answers, and cannot be claimed without the future that
finishes it. That was the point of the seam.
The last two claim-driven tests were both arranging node states to observe
something that never needed a run:
`settled_dag_leaves_the_snapshot_despite_its_skipped_branch` completed all
seven nodes of a rebuild to assert the DAG left the snapshot. That is one
predicate over a list of states. `shown_on_wire` is it, split out of
`dag_view`, and the cases can now be named rather than arranged — including
the empty set, the one input where "any" and "all" disagree. It takes
states rather than projected nodes so the caller skips projecting what it
is about to discard; a `NodeView` costs a `build_logs` lookup.
`failed_node_cancels_downstream_but_afterany_reconcile_runs` asserted three
unrelated things from one arranged failure: the cascade (hive_jobq's, and
already tested there), the wire filter (now `shown_on_wire`), and the
roll-up. `DagView::rollup_state` lives in hive-host-sock, which had no
tests at all — it does now, next to the invariant, covering the ordering
its own doc comment says has silently disagreed with the frontend before.
With nothing left claiming, `Claimed` / `ClaimReady` / `CompleteNode` /
`claim_one` / `settle_rebuild_tail` are deleted. Claim/complete sites in
`job_queue/tests.rs`: 109 -> 0.
jobq narrows to match: `settle` is gone (it was a `claim_one` loop
returning a Vec, and its only callers were tests — it lives in the test
module now), `claim_one` is private, and `complete_growing` is
`pub(crate)`. `claim_next` is the whole run-loop surface.
`complete` stays `pub` for one caller, noted at the definition: `submit`
completes a group root with no logic of its own so it parks in `Finishing`
and its children unblock. That is a statement about the node, not an event
to report, and it wants to be expressible at insert time.
It handed out exactly the builder JobBuilder::new is pub(crate) to
withhold, which was agreed against more than once. I had left it in
place with a docstring naming it as the hole and folded the fix into an
open question. It was not an open question.
It only survived because two hive-c0re tests minted a builder by hand to
simulate a MetaLock growing its cascade. The grown thing is a template,
so the fix is the same as for the deploy graft and the reconcile
fan-out: call it.
exec.rs's MetaLock arm looped rebuild_nodes inline -- the second and last
construction site declaring nodes inside an executor. It is now
templates::grown_rebuilds, so a test can assert what a sweep declares by
calling the same function production calls.
grown_subgraph_roots_on_emitter_and_rebases_local_deps and
meta_update_grows_cascade_in_dag differed only in RebuildOpts; they are
one test over the declared shape, checking a root chain per agent, a
build each, and a drain each because a boot sweep is graceful.
With those gone, CompleteNode::new_job, CompleteNode::complete_node_growing
and drain_meta_syncs had no callers either. hive-jobq's own two growth
tests use JobBuilder::new() directly -- in-crate, so the wall holds.
grep for new_job across the workspace now returns nothing.
fifo_fairness_for_the_slot lived in hive-c0re and submitted three
rebuilds, driving one to completion to watch the freed build slot go to
the earlier waiter. The guarantee it was checking is this crate's:
claim_one scans nodes in insertion order and takes the first satisfiable
one. Nothing here tested it -- the property hive-jobq provides was
asserted only downstream, through a host's templates.
a_contended_resource_goes_to_the_oldest_waiter tests it directly.
Mutation-checked: reversing the scan order fails it.
What is hive-c0re's is which nodes contend for the slot at all, and that
is a declaration, so its half is now a declared_resources table with
nothing running. The measured shape corrected an assumption on the way:
MetaSync takes the meta window only, and the agent lease starts at
StopForUpdate -- the first node that touches the container -- not at the
head of the chain. Prebuild deliberately holds no lease, which is what
lets it overlap another DAG on the same agent while the container is
still up.
"Uniform hold across the chain" needs no test of its own: a resource is
held for the acquirer's whole subtree, and the parent nesting is already
asserted in rebuild_chain_is_declared_serial.
complete_growing had no test at all -- its only reference was the
internal call from claim_next -- so the rule moved into it in the
previous commit was enforced but unproven.
- a_completing_node_grows_the_work_it_declared: the declared work
lands under the emitter, and the emitter parks in Finishing rather
than going terminal. That ordering is the point of growing as part
of the completion.
- a_failed_node_grows_nothing: the rule that moved out of the host.
Both were mutation-checked rather than trusted green: with the
Outcome::Failed guard deleted, a_failed_node_grows_nothing fails on the
appended node while its companion still passes, so the test bites and
the drop is specific to failure rather than blanket.
The departed-parent guard beside it stays untested and says so. Nothing
removes a node from the graph yet (eviction stops retaining a DAG; its
nodes linger) and NodeId cannot be fabricated by construction, so a test
would have to fake the precondition it checks. The comment names the
bounded prune as the point at which it becomes testable.
Two review findings from the previous round, re-checked against the
actual tree rather than against my notes.
`Scheduler::complete` was still a public wrapper whose entire body was
`self.finish(id, outcome)`. Its docstring argued the split was not a
redirect because both completion forms shared `finish` -- but sharing a
private helper is not a reason for two public names. `finish`'s body now
lives in `complete`, and `complete_growing` calls it. Same sharing, one
name, no redirect.
Growth on a failed node is now dropped by `complete_growing` instead of
by the host loop. Failure cancel-cascades to every pending child of the
completing node, and grown work is inserted as its children, so anything
appended here is Skipped by the next statement -- the insert is not
wrong, it is provably pointless. That is a consequence of this crate's
cascade rule, so this crate should be the one enforcing it; a host that
has to remember it can forget it. Behaviour is unchanged: hive-c0re
already dropped growth before calling, and now no longer has to.
`Scheduler::new_job` is left alone but documented for what it is: the
hole in `JobBuilder::new`'s pub(crate) wall, with no non-test caller
since claim_next mints a builder per running node. Closing it is a venue
question rather than a rename, so it stays for now.
The caller supplies how to run a node and spawns what it gets back; it
never touches claiming or completion. The returned future runs the node
*and completes it*, so "forgot to finish the node" stops being something
a caller can do — completion is inside the thing they spawn.
The `Option` is answered synchronously, before anything is awaited, so the
run loop learns whether there was work without waiting on the node it just
started. That is what lets it choose between claiming again immediately
and backing off; an id alone cannot express that choice.
Locking: taken twice, briefly, and never held across the await — once to
claim, once inside the future to complete. A guard alive across an await
point would make the future non-`Send` and unspawnable, which is also why
the node itself runs unlocked for however long it takes. `Arc` +
`std::sync::Mutex` keep this runtime-agnostic: no tokio in this crate.
`run` receives an owned payload rather than a borrow for the same reason a
`&Job` could not be threaded through the executors: a reference parameter
is live for the whole future, borrowing the graph across the await and
poisoning `Send`.
The output carries the insert result instead of swallowing it. This crate
has no logger by design, so a malformed grown job is reported to the
caller, who can log it. The node completes either way — its own work
already happened.
One-at-a-time claiming is what lets a caller choose between claiming again
immediately and backing off — a batch return cannot express that choice,
and the choice is the point: the run loop wants to know there was work
before it decides whether to wait.
`settle()` keeps its exact meaning as `while let Some(id) = claim_one()`.
A node started by an earlier iteration is `Running`, not terminal, so it
cannot satisfy another node's dependency in the same sweep; it only
consumes resources. The crate's ~45 existing `settle()` assertions — which
cover resource borrowing, cap-1 serialisation, roll-up and cancellation —
are what verify that equivalence, so it is checked rather than argued.
`None` means "nothing runnable right now", which is deliberately a
different statement from "nothing pending": a node can be pending and
unrunnable because its resources are held elsewhere.
Cost stated rather than left to be found: each `claim_one` rescans the
pending set, so `settle` is O(n^2) in nodes claimed where the single-pass
version was O(n). The graph is bounded by history retention.
A node no longer hands back a recipe for the scheduler to replay later. It
declares straight onto a builder it was given, and that builder is inserted
as part of completing the node.
Deleted: `pub type Declare`, `struct NodeOutput` (+ its hand-written `Debug`),
`JobQueue::append_subgraph`. Nothing added to `Dag` / `DagView`.
jobq gains `Scheduler::new_job()` (the only way to obtain a `JobBuilder`) and
`complete_growing(id, outcome, grown)`, which inserts under `id` and *then*
completes it, so a DAG cannot roll terminal while grown work is still pending.
`complete()` and `complete_growing()` share a private `finish()` rather than
one redirecting through the other. The DAG-gone guard lives beside the graph
now, where it cannot be skipped, instead of being a caller-side lookup.
The growth executors return data (`run_meta_lock -> (Vec<String>, RebuildOpts)`,
`run_reconcile -> Option<NodeKind>`) rather than taking the builder: a `&Job`
parameter is live for the whole function body, and `&RefCell<T>` is never
`Send`, so an async fn taking one cannot be spawned. `run_node` threads the
builder by value and hands it back.
A node can now declare work and then fail, which was previously inexpressible.
`grown` is dropped in that case — failure cancel-cascades downstream, so
inserting it would only add nodes to immediately cancel — and the log line
carries `grown_nodes` so the drop is visible.
The builder's module doc claimed a builder "cannot be constructed, held
or inserted from outside this crate". Two of those three were false:
`new()` is `pub(crate)`, but a hand-written `impl Default for
JobBuilder` is a trait impl on a `pub` type, so it is public regardless
— `JobBuilder::default()` compiled downstream.
Nothing was unsound (`insert_with` stayed `pub(crate)`, so an
outside-built builder could not reach a graph), but the sentence claimed
more than the visibility enforced, which is the bug this crate's docs
have hit before.
Delete the impl; `new()` constructs directly. The doc now says only what
is enforced, and records why there is no `Default` — so the next person
reaching for one finds the reason instead of adding it back.
`Scheduler::append_unchecked` was a one-line pass-through to
`Graph::insert_unchecked` with a single caller. `insert_job` is already a
method on `Scheduler`, so it can borrow `self.graph` and call the graph
directly — the wrapper bought nothing but a name.
Also fixes `insert_job`'s doc, which still claimed every node goes
through `Scheduler::append`; it goes straight to the graph's unchecked
insert, and the reason belongs in that doc rather than on a wrapper.
`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.
mara on !2909: "shouldnt node_by_id be part of jobq?" — yes. Resolving a
raw value to a `NodeId` is the exact inverse of `NodeId::get`, which
already lives in hive-jobq, and it is only a search because the graph
owns the counter that makes ids unfabricable. Both halves of that
round-trip belong on the same side of the crate boundary.
Placing it in `QueueInner` also put it in a layer slated for removal, so
the c0re-side helper would have had to move later anyway — and it was
private there, leaving any other caller needing the same resolution to
write the same `nodes().find_map(…)` by hand.
`Graph::resolve_id` replaces it, with a unit test covering the
round-trip and the rejection of a value that was never an id.
While re-reading the diff for that question: the doc comment added in
the previous commit landed *between* `container`'s doc comment and its
signature, silently reattaching "The container node of `dag_id`" to the
new helper and leaving `container` undocumented. Restored.
No behaviour change and no wire change — same search, same call site.
Checked with clippy (`--all-targets -D warnings`), `cargo test -p
hive-jobq -p hive-c0re` (41 + 322 passed) and `nix fmt`.
`hive_host_sock::jobs::State` was a hand-maintained copy of
`hive_jobq::State` — five variants spelled the same in both, kept in sync
by whoever remembered. Adding `Skipped` last week meant adding it twice.
The wire crate now re-exports the scheduler's enum and `to_wire_state` is
gone.
Two states that were hidden now reach clients. `to_wire_state` renamed
`Pending` to `Queued` and folded `Finishing` into `Running`, so the
dashboard could not distinguish a node waiting on its dependencies from
one whose own work is done while its sub-nodes still run. Both are now
visible, and consumers say which they mean.
Every consumer had to move with it, and only the Rust ones said so: the
exhaustive matches in `hivectl` and `DagView::rollup_state` failed to
compile, while the dashboard's fourteen string comparisons would have
gone quietly wrong — a `finishing` node no longer counting as running,
a `pending` node no longer as queued.
The frontend also builds CSS class names out of the state string
(`rqe-` + state, `rqe-node-` + state) and keys its glyph map on it, all
lowercase. Those go through a `stateSlug` helper now; comparisons use the
wire spelling, presentation lowercases. Without that split every queue
entry and node chip would have silently lost its styling.
Dropping the `State as JobState` alias in hive-c0re falls out of this:
the alias only existed to tell two `State` types apart, and there is one
now.
The roll-up treated a cancelled child the same as a failed one, so a DAG
the operator cancelled before it started reported `Failed` — it claimed to
have failed at something when nothing under it ever ran.
`Failed` still outranks `Cancelled`: a group where one step broke and the
rest were dropped in response is a failure, and that is the fact worth
surfacing. Only a group with no failed child at all reports the cancel.
This also settles a disagreement. `DagView::rollup_state` on the wire has
always ranked failed over cancelled over the rest; the graph's own roll-up
had no `Cancelled` outcome to rank, so the two described the same DAG
differently depending on which one you asked.
`JobQueue::cancel` decided whether a DAG could be cancelled by reading node
run-state, walked the subtree, judged per node whether that node had asked
to observe cancellation, and re-ran the container's roll-up. Every one of
those is a fact the scheduler owns; core was reaching across the boundary
to compute them.
`Scheduler::cancel_node` now takes the whole subtree: cancelling a node
cancels the work under it, since a group is abandoned by abandoning its
root. The existing method generalises rather than gaining a sibling — it
had one production caller, which this replaces.
The gate runs over the work *under* the node, not the node itself: a group
root's state is its subtree's roll-up rather than a step that ran, so a
container is `Finishing` and never `Pending`, and gating on it would refuse
every cancel. A node with no children is its own work, which keeps the
previous single-node behaviour.
`observes_cancellation` moves in with it — it reads a node's declared edges
and knows nothing about what the payload means.
Core keeps the one genuinely domain-specific step, resolving a wire
`dag_id` to its container node, and is three lines otherwise.
Returning `false` for an unknown id gave the same answer as a node that is
merely still running, so a caller polling a stale id would wait forever for
a state that can never arrive. `Option<bool>` makes the two cases separate,
matching `node()`'s convention that `None` means the id isn't in the graph.
hive-c0re hand-rolled four traversals over a graph it doesn't own, because
`Graph` exposed only `node()` and `nodes()`. They are generic — nothing in
them knows what a hyperhive DAG is — so they move to `hive-jobq` and core
delegates.
`Graph` gains `root_of`, `descendants`, `roots`, `is_settled` and
`first_error`; they reuse the private `is_descendant` the crate already had
for its dep-scope rule. `subtree` gets faster on the way: core walked every
node's whole parent chain to the root for every node in the graph, where
`is_descendant` stops as soon as it sees the ancestor.
`first_error` deliberately looks for the first `Failed` descendant that
*carries* an error rather than the first `Failed` one. A node that rolled
its failure up from a child holds no error of its own and sorts before that
child, so the simpler version reports `None` for the common case and the
dashboard loses the reason. The distinction has its own test.
`dag_is_terminal` is deleted rather than moved: it was already a plain
`state.is_terminal()` read, and its three call sites now ask the graph.
`cargo add -p hive-jobq` wrote the version into the crate's own manifest
instead of `[workspace.dependencies]`, which is how every other shared
dep here is declared and the thing that stops two members drifting onto
different versions of the same crate.
Review follow-ups on #2785.
`DepWhen` wraps `BitFlags<TerminalState>` instead of a hand-rolled `u8`,
so the bit manipulation belongs to the library and `TerminalState` gains
its flag value from `#[bitflags]` rather than a `bit()` match anyone
could get wrong. `of`/`accepts`/`is_empty` become one-liners over it.
Serialization is written out by hand rather than derived: clippy's
`unsafe_derive_deserialize` fires on deriving over a type with unsafe
internals, and the honest fix is to say what the wire form is. It is now
the list of accepted outcomes — `["done","failed"]` — which reads better
than a bitmask and survives the bits being renumbered.
Also drops the `pub use` re-export of `DepWhen` / `TerminalState` from
hive-c0re's `model`. It existed so that `use super::model::…` kept
compiling, which is a shim for one consumer's convenience; the sites
import from `hive_jobq` directly now.
And removes comments narrating what the code used to be. Git holds that.
Splits what was one `Cancelled` outcome into two, because they were two
different facts wearing one name:
- `Skipped` — the node's own edges ruled it out. Expected; the failure
branch of a run that succeeded is `Skipped`. A parent's roll-up
**ignores** it.
- `Cancelled` — the work was dropped before it could start. Still
not-success for the roll-up, as before.
Without that split, branching on outcome defeats itself: exactly one
branch is always ruled out, `any_child_failed` counted it, and every DAG
containing a branch would have rolled up failed no matter how the run
went. Caught in review before it was written, not after.
`AFTER_ANY` becomes `{Done, Failed, Skipped}` — "anything except the work
being dropped". That is what it always meant; it only swept in
cancellation because cancellation wasn't distinguishable from
elimination. Audited every user rather than assuming, which is how the
one regression in my own proposal surfaced: `{Done, Failed}` would have
refused to run rebuild's recovery `Reconcile` after a failed `MetaSync`
(that eliminates `Prebuild`, so the tail's dep is `Skipped`, not
`Failed`) and left the container down.
With that, the templates stop computing outcomes and let the graph pick:
- `ResolveApproval { approval_id, outcome }` — one tail per outcome, each
edged to accept only its own, so exactly one is ever runnable.
- `EmitRebuilt { agent, ok }` — a pair. `ok` is not derived, it is which
of the two the graph let run.
Edges are conjunctive, so "any of these roots failed" is not directly
sayable. The composition: the success branch is `AFTER_OK` on every root
(so it is itself eliminated the moment one doesn't succeed), and the
failure branch keys off *that* elimination. The failure branch also
waits on every root — without it, a failed `Prebuild` eliminates the
success branch immediately and the failure would be announced while the
recovery `Reconcile` was still running. The tests caught that one.
Deletes, all of them #2770's host-side debt:
- `Claim.deps`, `DepOutcome`, `Claim::deps_state`, `Claim::deps_error`
and the dep-snapshotting loop in `claim_ready`. Executors read their
own variant now; nothing inspects anything.
- `NodeKind::is_tail()` and the `cancel` exemption built on it. Sparing
is derived from the edges: `cancel` keeps a node iff one of its edges
accepts `Cancelled`. An approval tail names it and survives to resolve
the row; `Reconcile` doesn't and is cancelled with the rest. My earlier
claim that this couldn't dissolve was only true while `AFTER_ANY`
accepted cancellation.
`resolve_approval_dag` / `deploy_terminal_tag` now take `TerminalState`
rather than the wire `State`, so both matches are exhaustive instead of
ending in a catch-all.
Skipped nodes are filtered off the wire alongside `Done` ones. That costs
some dashboard detail on a failed rebuild — which steps were skipped —
and the tests say so with a pointer to the follow-up. Surfacing them as
`Cancelled` instead would be worse: the client roll-up ranks `Cancelled`
above `Running`, so a successful DAG with a not-taken branch would read
as cancelled.
`DepWhen` was two named cases, so every new combination wanted a new
variant. It is now a set over the terminal outcomes: a `u8` bitset
newtype, no dependency, with `AFTER_OK` / `AFTER_ANY` kept as the two
constants the templates actually use. "Run regardless" is all outcomes,
"anything that isn't a failure" is `{Done, Cancelled}`, a compensation
branch is `{Failed}` — closed under combination, so it never needs
another variant.
`TerminalState` is its own type rather than a subset of `State`, so an
edge cannot name `Pending` / `Running` / `Finishing`. Those are
meaningless in a dependency and are better unrepresentable than
validated against. The empty set is the one thing that can't be typed
away — nothing satisfies it, so `validate` rejects it next to the cycle
check.
Two consequences worth calling out:
- `cascade_cancel` collapses to one rule: a pending node is doomed once
any edge it names can no longer be satisfied. The hardcoded `AfterOk`
special case is gone, and a weak-edged node survives its dependency's
cancellation because of its own edge rather than by exemption.
- The cascade now runs on **any** terminal outcome, `Done` included.
With sets, success rules dependents out just as failure does — a
`{Failed}` branch is unsatisfiable the moment its dependency succeeds,
and leaving it `Pending` would wedge the subtree non-terminal forever.
That is a hang, not a wrong answer, so it is the load-bearing half of
this commit.
Edges are conjunctive, so "any of these N failed" is not directly
sayable. The composition that works is in the tests: the success branch
depends `AFTER_OK` on every root, so it is itself cancelled the moment
one of them doesn't succeed, and the failure branch hangs off *that*
with `{Cancelled}`. Exactly one of the two runs.
Also deletes hive-c0re's duplicate `DepWhen` enum and the
`to_crate_when` translation beside it. The copy bought nothing and had
to be widened in lockstep with the crate's edge model — it is the
in-between layer #2772 exists to remove, and it is what broke the build
when the crate's spelling changed.
All 34 jobq tests pass, including the four new ones covering both
directions of a failure-only branch, weak-edge survival of a cancelled
dependency, and the aggregator composition.
Adds crate READMEs (matching the hive-claude precedent) and wires
readme = "README.md" into each Cargo.toml [package] for hive-jobq,
hive-host-sock, and hive-priv-sock — the crates squarely in the infra
lane. Each README leads with purpose + when-to-use and points at the
crate-root //! docs for depth rather than duplicating them.
First increment of the per-crate-README effort; the shape here is the
proposed template for the remaining crates (see issue discussion).
Node gains started_at/finished_at (chrono DateTime<Utc>, serialized
RFC 3339 on the wire per hive_sh4re::wire_time) plus error (String).
Graph::set_state self-stamps started_at on the first Running transition
and finished_at on the first terminal one, via an internal now_utc()
clock (keeps settle/complete signatures stable). Outcome::Failed(String)
carries the failure reason, set on the terminal transition.
hive-c0re complete_node builds Outcome::Failed(msg); its node_rt
side-table stays i64 for now (double-write) until #2637 reads the Node.
Toward #2637: the jobq graph becomes the source of truth for per-node
lifecycle so the queue can be sent to the client as-is.
Rework the crate's scheduling model onto an explicit parent (grouping)
axis, separate from the dep (ordering) axis.
- Node gains a structural `parent: Option<NodeId>`, set by the caller
independent of its `Dep::Node` edges. Grouping is not ordering. A
`Dep::Node` edge must stay inside the depender's own parent group
(validated) — never crossing to another group or onto the parent.
- Resource holding walks the parent tree: acquire fresh when no ancestor
holds it (the acquirer owns it, held for its whole subtree); borrow an
ancestor's grant (one branch at a time; nodes inside are covered); take
an extra unit when the grant is lent to a sibling branch, else wait. A
grant releases only once the owner and its whole subtree are terminal.
- Completion rolls up the parent tree: a node's sub-nodes run after its
own logic, and it is not terminal until they finish — it parks in
`State::Finishing`, rolling up to Done (every child Done) or Failed
(any child Failed/Cancelled). A child is gated on its parent reaching
Finishing; a downstream dep on a node therefore waits for that node's
dynamically-appended children with no explicit edge. A failed node
cancels its pending sub-nodes.
Deletes the SharedResources/ResourceGuard layer (guard.rs) and the
add_dep graph-growth hook (no longer needed). The scheduler stays
single-threaded, owning the ResourceTable directly. Early release of a
grant once no subtree node still needs it is a deferred optimization
(unsafe under dynamically-appended subnodes, #2611).
Base for the hive-c0re job_queue port (#2605), split out so that PR can
rebase onto it.
When every resource dep of a node re-enters an ancestor's lock, `owned_reqs`
is empty; the old code still called `acquire(vec![])` and stored a no-op
empty guard in `owned`. Gate the acquire + guard insertion on
`!owned_reqs.is_empty()` — one fewer `borrow_mut` + `HashMap` entry per
fully-re-entrant node in the settle loop. `node_owns` already treats a
missing `owned` entry as non-owning, so behaviour is unchanged.
Make the resource lock unmisusable from outside the crate: the public
surface is now purely declarative (build a Graph with Dep::Resource edges,
configure capacities, run the Scheduler), and the scheduler owns every
acquire/release — a consumer never holds a guard, so it cannot hold the
lock wrong.
- `guard` module + `ResourceTable::try_acquire_all`/`release_all` +
`Graph::set_state` are now `pub(crate)`.
- Remove the dead borrowed-guard layer (`ResourceGuard::borrowed`,
`Acq::Borrowed`, `is_owning`): the scheduler tracks re-entrancy via its
own single borrow slot per (holder, resource) and never constructs a
borrowed guard, so re-entrancy lives in exactly one place. `Acq`
collapses into the owning `ResourceGuard` struct.
- `#[must_use]` on `Scheduler::settle` — ignoring its ids silently drops
runnable work.
- `SharedResources::with` (test-only table observability) is `#[cfg(test)]`.
- Drop the moot borrowed-guard tests; retained owning tests are black-box,
and the redundant `set_state` test helper is gone.
Replace the concrete ResourceName(String) with a type parameter
R: Clone + Eq + Hash threaded end-to-end (Dep<R>, Node<N,R>, Graph<N,R>,
ResourceTable<R>, ResourceGuard<R>/SharedResources<R>, Scheduler<N,R>).
The crate no longer hard-codes the resource identity; the consumer picks
the concrete type (a String, or an enum like BuildSlot/Agent(name)) at
the port. Tests use String as the concrete R. Pure type-parameter
thread-through, no logic change. 25 tests green, clippy pedantic clean.
Named counting-semaphore resources — the Dep::Resource side of the v2
model. A ResourceTable tracks per-name capacity + held counts; unconfigured
names default to capacity 1 (created lazily). try_acquire_all grants every
requested unit or none, leaving the table untouched on failure — so a node
never holds one resource while waiting for another, which is what makes the
scheduler deadlock-free without cycle detection. Duplicate names in a
request are summed; over-capacity requests can never acquire. release_all
saturates rather than underflowing. Runtime scheduler state, not persisted:
held counts are rederived from running nodes on restart. Guard objects
(RAII release, recursive re-entrancy) wrap this in a follow-up.
Per mara's direction — validate ids as they enter the graph so internal
iteration can trust every id the graph holds; the generational route for
removal comes later. Adds GraphError; insert() now rejects a dangling
Dep::Node / parent id (it is fallible); validate() checks all internal id
references resolve and that next_id is past the largest existing id;
deserialization runs validate() via #[serde(try_from = "GraphData<N>")], so
a loaded graph can never carry a dangling reference. 5 new tests; serde_json
added as a dev-dependency for the round-trip cases.
argus flagged that `NodeId(pub u64)` contradicted the "opaque" doc — a pub
inner field lets callers fabricate `NodeId(42)`. Make the field `pub(crate)`
so an id can only originate from the graph's monotonic counter or serde
deserialization, never a caller. Tests construct ids in-crate (unaffected);
the derived Serialize/Deserialize round-trips fine. Doc keeps "opaque" — now
accurate — with a line explaining the enforcement.
group_terminal checked only that every child was terminal, never the group
node's own state — so an empty group whose node is still Running returned
true (empty .all()), making a running node that has yet to append its
subgraph look already-finished. Now it requires the group node itself
terminal AND every child recursively terminal. Deciding when to settle a
group node to terminal once its children are done stays a scheduler concern;
this answers the dependents' question — is the whole group, node included,
finished. Adds group-node-pending-with-child-done + empty-running-group tests.
First step of extracting the job-DAG queue into a domain-agnostic
`hive-jobq` library, per the operator's v2 design: one persistent
graph, named-counter resources, recursive node groups, opaque stable
node ids, guard-object locks, a slot-filling scheduler.
This commit lands only the data model, so the shape can be reviewed
before the machinery is built on it:
- NodeId: opaque, stable, monotonic; group membership is a parent
edge, not encoded in the id (the 1/1/2 hierarchy is a derived UI
label).
- ResourceName, Dep (Node | Resource{name,count}), State.
- Node<N>: caller-defined payload N so the library stays
container-agnostic.
- Graph<N>: insert (mints stable ids), node lookup, children,
recursive group-terminal check. Retains completed groups (no
pruning in v1).
The resource-acquisition machinery (atomic all-or-nothing acquire),
the recursive-lock guards, and the scheduler loop are follow-ups.
Tests cover id minting, group terminality, and state terminality;
clippy + rustdoc clean.
