diff --git a/hive-jobq/src/builder.rs b/hive-jobq/src/builder.rs index 2a6a07d4..ed894cae 100644 --- a/hive-jobq/src/builder.rs +++ b/hive-jobq/src/builder.rs @@ -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( +/// never declared, [`BuildError::DepOutsideGroup`] for an edge leaving the +/// depender's parent group, [`BuildError::UnsatisfiableDep`] for an empty edge. +fn check_job_shape( pending: &[Pending], wanted: &[NodeGuid], + root_parent: Option, ) -> Result<(), BuildError> { let mut declared: std::collections::HashSet = std::collections::HashSet::new(); + // Declared parent per node, for walking a job-internal parent chain. + let mut parent_of: HashMap> = 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( }); } 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( 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>, + node_parent: Option, + dep: NodeGuid, + root_parent: Option, +) -> 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 JobBuilder { mut insert: impl FnMut(N, Vec>, Option) -> Result, ) -> Result, BuildError> { let pending = self.nodes.into_inner(); - check_declaration_order(&pending, wanted)?; + check_job_shape(&pending, wanted, root_parent)?; let mut ids: HashMap = 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