vinzenz/hyperhive
vinzenz/hyperhive
Watch
0
0
Fork
You've already forked hyperhive
 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

jobq: reject every graph-rejectable shape before the first insert

Browse source 
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.
This commit is contained in:
atlas 2026-08-02 15:18:35 +02:00 • committed by mara
commit 236b13e68d
1 changed files with 133 additions and 13 deletions
Download patch file Download diff file Expand all files Collapse all files

146
hive-jobq/src/builder.rs
View file

@ -82,6 +82,32 @@ pub enum BuildError {
/// The handle that resolved to nothing.
node: NodeGuid,
},
/// An edge leaves the depender's own parent group — including an edge onto
/// its **own parent**, which would deadlock: a parent parks in
/// `Finishing` until its children settle, so a child waiting on it could
/// never run. The graph enforces the same rule
/// (`GraphError::DepOutsideParent`); this catches it before any insert.
#[error(
"node {node:?} depends on {dep:?}, which is outside its parent group — \
an edge must stay within the depender's own group"
)]
DepOutsideGroup {
/// The node carrying the edge.
node: NodeGuid,
/// The out-of-group target.
dep: NodeGuid,
},
/// An edge that no outcome can satisfy (an empty [`DepWhen`]), so the node
/// could never become runnable. Refused at declaration rather than
/// inserted — the graph only catches this when validating a *deserialized*
/// graph, so without this check it would insert and silently never run.
#[error("node {node:?} has an edge on {dep:?} that no outcome can satisfy")]
UnsatisfiableDep {
/// The node carrying the edge.
node: NodeGuid,
/// 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)]
@ -89,30 +115,60 @@ pub enum BuildError {
}
/// Reject a job whose own declarations don't hold up — **before anything is
/// inserted**, so these three failures cannot leave a partial job behind.
/// inserted**, so no failure can leave a partial job behind.
///
/// Each is decidable from what the builder already holds: a node may only name
/// handles declared before it, and a job may only ask for handles it declared.
/// Running this first is what lets the insert loop index `ids` directly instead
/// of discovering a bad reference halfway through mutating the graph.
/// This covers *every* rejection [`crate::Graph::insert`] can raise for a
/// builder-produced node, which is what makes the insert loop below infallible
/// in practice:
///
/// | graph rejection | why it cannot reach the graph |
/// |---|---|
/// | `UnknownParent` / `UnknownDep` | a handle only exists if this job declared it, and the ids are minted here |
/// | `DepOutsideParent` | the grouping rule is re-checked here against the job's own parent chains |
///
/// It also rejects an **empty [`DepWhen`]**, which the graph currently only
/// catches on the deserialize path (`Graph::validate`) — so a node that nothing
/// could ever satisfy is refused at declaration instead of being inserted and
/// silently never running.
///
/// `root_parent` matters for the grouping rule: a node that declared no parent
/// hangs there, so what counts as "inside the group" depends on whether the job
/// is being attached under a container or at the top level.
///
/// # Errors
/// [`BuildError::ForwardEdge`] / [`BuildError::ForwardParent`] for a reference
/// to a later node, [`BuildError::UnknownNode`] for a requested handle this job
/// never declared.
fn check_declaration_order<N, R>(
/// never declared, [`BuildError::DepOutsideGroup`] for an edge leaving the
/// depender's parent group, [`BuildError::UnsatisfiableDep`] for an empty edge.
fn check_job_shape<N, R>(
pending: &[Pending<N, R>],
wanted: &[NodeGuid],
root_parent: Option<NodeId>,
) -> Result<(), BuildError> {
let mut declared: std::collections::HashSet<NodeGuid> = std::collections::HashSet::new();
// Declared parent per node, for walking a job-internal parent chain.
let mut parent_of: HashMap<NodeGuid, Option<NodeGuid>> = HashMap::new();
for node in pending {
for (dep, _) in &node.deps {
for (dep, when) in &node.deps {
if !declared.contains(dep) {
return Err(BuildError::ForwardEdge {
node: node.guid,
dep: *dep,
});
}
if when.is_empty() {
return Err(BuildError::UnsatisfiableDep {
node: node.guid,
dep: *dep,
});
}
if !dep_in_group(&parent_of, node.parent, *dep, root_parent) {
return Err(BuildError::DepOutsideGroup {
node: node.guid,
dep: *dep,
});
}
}
if let Some(parent) = node.parent
&& !declared.contains(&parent)
@ -123,6 +179,7 @@ fn check_declaration_order<N, R>(
});
}
declared.insert(node.guid);
parent_of.insert(node.guid, node.parent);
}
for guid in wanted {
if !declared.contains(guid) {
@ -132,6 +189,42 @@ fn check_declaration_order<N, R>(
Ok(())
}
/// The graph's grouping rule (`Graph::dep_target_in_group`) decided against the
/// job's own declarations, before any node exists.
///
/// A depender whose declared parent is `Some(q)` may only name a **proper
/// descendant of `q`** — never `q` itself, which would deadlock (a parent parks
/// in `Finishing` until its children settle). A depender that declared no parent
/// hangs under `root_parent`:
/// - `root_parent = Some(_)` — every node in this job is somewhere under it, so
/// any job-internal target is in-group.
/// - `root_parent = None` — the depender is top-level, so the target must be
/// top-level too, i.e. it must also have declared no parent.
fn dep_in_group(
parent_of: &HashMap<NodeGuid, Option<NodeGuid>>,
node_parent: Option<NodeGuid>,
dep: NodeGuid,
root_parent: Option<NodeId>,
) -> bool {
match node_parent {
// Walk the target's declared chain looking for the depender's parent.
// Starts at the target's *parent*, so the target is never its own
// ancestor — matching `Graph::is_descendant`.
Some(group) => {
let mut cur = parent_of.get(&dep).copied().flatten();
while let Some(p) = cur {
if p == group {
return true;
}
cur = parent_of.get(&p).copied().flatten();
}
false
}
None if root_parent.is_some() => true,
None => parent_of.get(&dep).copied().flatten().is_none(),
}
}
/// 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.
#[derive(Debug)]
@ -227,7 +320,7 @@ impl<N, R> JobBuilder<N, R> {
mut insert: impl FnMut(N, Vec<Dep<R>>, Option<NodeId>) -> Result<NodeId, GraphError>,
) -> Result<Vec<NodeId>, BuildError> {
let pending = self.nodes.into_inner();
check_declaration_order(&pending, wanted)?;
check_job_shape(&pending, wanted, root_parent)?;
let mut ids: HashMap<NodeGuid, NodeId> = HashMap::new();
for node in pending {
@ -590,10 +683,15 @@ mod tests {
);
}
/// The graph's own validation still applies — the builder does not
/// pre-empt it.
/// The grouping rule is enforced **before the first insert**, so a job that
/// breaks it leaves the graph untouched rather than half-built.
///
/// This used to assert `BuildError::Graph(_)` — i.e. that the graph's own
/// rejection surfaced through the builder. It does not reach the graph any
/// more: the same rule is now decided from the job's own declarations, and
/// the stronger property (nothing was inserted) is what this pins.
#[test]
fn graph_rejection_surfaces_as_is() {
fn an_out_of_group_dep_is_refused_before_anything_is_inserted() {
let mut g = sched();
let err = g
.insert_job(None, |b| {
@ -604,7 +702,29 @@ mod tests {
Vec::new()
})
.expect_err("out-of-group dep");
assert!(matches!(err, BuildError::Graph(_)), "{err:?}");
assert!(matches!(err, BuildError::DepOutsideGroup { .. }), "{err:?}");
assert_eq!(g.graph().nodes().count(), 0, "nothing may have landed");
}
/// An edge no outcome can satisfy is refused at declaration. The graph only
/// catches this when validating a deserialized graph, so without the
/// pre-pass such a node would insert cleanly and then never become
/// runnable — a silent hang rather than an error.
#[test]
fn an_edge_no_outcome_can_satisfy_is_refused() {
let mut g = sched();
let err = g
.insert_job(None, |b| {
let first = b.node("a");
let _ = b.node("b").on_outcome(first, &[]);
Vec::new()
})
.expect_err("unsatisfiable edge");
assert!(
matches!(err, BuildError::UnsatisfiableDep { .. }),
"{err:?}"
);
assert_eq!(g.graph().nodes().count(), 0, "nothing may have landed");
}
/// A job's own roots hang under the group's attachment point, while a node