From a2edad715fcf6d34ac79f509d6e48f9258369ba3 Mon Sep 17 00:00:00 2001 From: atlas Date: Sun, 2 Aug 2026 15:45:27 +0200 Subject: [PATCH] jobq: make the builder the only way nodes enter a graph MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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. --- hive-jobq/src/builder.rs | 20 ++++++++++---------- hive-jobq/src/lib.rs | 30 ++++++++++++++++++++++++++++-- hive-jobq/src/scheduler.rs | 17 ++++++++++++++++- 3 files changed, 54 insertions(+), 13 deletions(-) diff --git a/hive-jobq/src/builder.rs b/hive-jobq/src/builder.rs index ed894cae..fa1e9807 100644 --- a/hive-jobq/src/builder.rs +++ b/hive-jobq/src/builder.rs @@ -29,7 +29,7 @@ use std::cell::RefCell; use std::collections::HashMap; -use crate::{Dep, DepWhen, GraphError, NodeId, TerminalState}; +use crate::{Dep, DepWhen, NodeId, TerminalState}; /// An opaque identity for a node **within the job being built**. /// @@ -108,10 +108,6 @@ pub enum BuildError { /// The target of the unsatisfiable edge. dep: NodeGuid, }, - /// The graph rejected an otherwise well-formed node — an out-of-group edge, - /// an unsatisfiable [`DepWhen`], and so on. - #[error(transparent)] - Graph(#[from] GraphError), } /// Reject a job whose own declarations don't hold up — **before anything is @@ -310,14 +306,15 @@ impl JobBuilder { /// /// # Errors /// - /// [`BuildError::ForwardEdge`] / [`BuildError::ForwardParent`] if a node - /// references one declared after it, or [`BuildError::Graph`] if the graph - /// rejects a node (see [`crate::Graph::insert`]). + /// A [`BuildError`] if the job's own declarations don't hold up — see + /// [`check_job_shape`], which decides every one of them **before** the + /// first insert. The sink itself is infallible: by the time it runs, the + /// job is known-good, so no node can be rejected half-way through. pub(crate) fn insert_with( self, root_parent: Option, wanted: &[NodeGuid], - mut insert: impl FnMut(N, Vec>, Option) -> Result, + mut insert: impl FnMut(N, Vec>, Option) -> NodeId, ) -> Result, BuildError> { let pending = self.nodes.into_inner(); check_job_shape(&pending, wanted, root_parent)?; @@ -337,7 +334,10 @@ impl JobBuilder { .into_iter() .map(|(name, count)| Dep::Resource { name, count }), ); - let id = insert(node.payload, deps, parent)?; + // Infallible by construction: `check_job_shape` above decided every + // rejection the graph could raise, so there is nothing left here to + // abandon a half-inserted job on. + let id = insert(node.payload, deps, parent); ids.insert(node.guid, id); } Ok(wanted.iter().map(|g| ids[g]).collect()) diff --git a/hive-jobq/src/lib.rs b/hive-jobq/src/lib.rs index e6f61e54..838c0c29 100644 --- a/hive-jobq/src/lib.rs +++ b/hive-jobq/src/lib.rs @@ -419,11 +419,18 @@ impl Graph { /// graph that minted it, so a dangling reference is rejected here rather than /// surfacing as a broken edge later. /// + /// **Crate-private on purpose.** Nodes enter a graph only through + /// [`crate::builder::JobBuilder`] (via + /// [`crate::scheduler::Scheduler::insert_job`]) or + /// [`crate::scheduler::Scheduler::append`]. A second public way in would be + /// a second place validation has to agree with the builder's, and the two + /// would drift. + /// /// # Errors /// Returns [`GraphError::UnknownDep`] / [`GraphError::UnknownParent`] for a /// dangling dependency or parent id, or [`GraphError::DepOutsideParent`] if a /// `Dep::Node` edge points outside the node's own parent group. - pub fn insert( + pub(crate) fn insert( &mut self, payload: N, deps: Vec>, @@ -444,6 +451,25 @@ impl Graph { } } } + Ok(self.insert_unchecked(payload, deps, parent)) + } + + /// Insert without re-validating — **only** for a node the builder has + /// already proved well-formed. + /// + /// [`crate::builder::check_job_shape`] decides every rejection + /// [`Graph::insert`] could raise, before the first node lands. Re-checking + /// here would not add safety: the insert loop mutates as it goes, so a + /// rejection at node `i` would leave `0..i` in the graph — a loud error + /// *after* the corruption rather than instead of it. Making the sink + /// infallible is what turns the builder's atomicity from an accident of the + /// pre-pass being exhaustive into a property of the types. + pub(crate) fn insert_unchecked( + &mut self, + payload: N, + deps: Vec>, + parent: Option, + ) -> NodeId { let id = self.mint_id(); self.nodes.push(Node { id, @@ -455,7 +481,7 @@ impl Graph { finished_at: None, error: None, }); - Ok(id) + id } /// Borrow a node by id. diff --git a/hive-jobq/src/scheduler.rs b/hive-jobq/src/scheduler.rs index 2727f0fa..12f061b4 100644 --- a/hive-jobq/src/scheduler.rs +++ b/hive-jobq/src/scheduler.rs @@ -100,6 +100,21 @@ impl Scheduler { self.graph.insert(payload, deps, parent) } + /// [`Scheduler::append`] for a node the builder has already validated — + /// infallible, so the insert loop cannot abandon a half-built job. + /// + /// Not public: the only caller is [`Scheduler::insert_job`], feeding nodes + /// that [`crate::builder::check_job_shape`] has already proved + /// well-formed. See [`Graph::insert_unchecked`]. + pub(crate) fn append_unchecked( + &mut self, + payload: N, + deps: Vec>, + parent: Option, + ) -> NodeId { + self.graph.insert_unchecked(payload, deps, parent) + } + /// Insert a whole job under `root_parent`, returning the id each handle's /// node was minted as. /// @@ -131,7 +146,7 @@ impl Scheduler { let job = JobBuilder::new(); let wanted = declare(&job); job.insert_with(root_parent, &wanted, |payload, deps, parent| { - self.append(payload, deps, parent) + self.append_unchecked(payload, deps, parent) }) }