jobq: make the builder the only way nodes enter a graph
`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.
This commit is contained in:
parent
236b13e68d
commit
a2edad715f
3 changed files with 54 additions and 13 deletions
|
|
@ -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<N, R> JobBuilder<N, R> {
|
|||
///
|
||||
/// # 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<NodeId>,
|
||||
wanted: &[NodeGuid],
|
||||
mut insert: impl FnMut(N, Vec<Dep<R>>, Option<NodeId>) -> Result<NodeId, GraphError>,
|
||||
mut insert: impl FnMut(N, Vec<Dep<R>>, Option<NodeId>) -> NodeId,
|
||||
) -> Result<Vec<NodeId>, BuildError> {
|
||||
let pending = self.nodes.into_inner();
|
||||
check_job_shape(&pending, wanted, root_parent)?;
|
||||
|
|
@ -337,7 +334,10 @@ impl<N, R> JobBuilder<N, R> {
|
|||
.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())
|
||||
|
|
|
|||
|
|
@ -419,11 +419,18 @@ impl<N, R> Graph<N, R> {
|
|||
/// 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<Dep<R>>,
|
||||
|
|
@ -444,6 +451,25 @@ impl<N, R> Graph<N, R> {
|
|||
}
|
||||
}
|
||||
}
|
||||
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<Dep<R>>,
|
||||
parent: Option<NodeId>,
|
||||
) -> NodeId {
|
||||
let id = self.mint_id();
|
||||
self.nodes.push(Node {
|
||||
id,
|
||||
|
|
@ -455,7 +481,7 @@ impl<N, R> Graph<N, R> {
|
|||
finished_at: None,
|
||||
error: None,
|
||||
});
|
||||
Ok(id)
|
||||
id
|
||||
}
|
||||
|
||||
/// Borrow a node by id.
|
||||
|
|
|
|||
|
|
@ -100,6 +100,21 @@ impl<N, R: Clone + Eq + Hash> Scheduler<N, R> {
|
|||
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<Dep<R>>,
|
||||
parent: Option<NodeId>,
|
||||
) -> 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<N, R: Clone + Eq + Hash> Scheduler<N, R> {
|
|||
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)
|
||||
})
|
||||
}
|
||||
|
||||
|
|
|
|||
Loading…
Reference in a new issue