diff --git a/hive-jobq/src/scheduler.rs b/hive-jobq/src/scheduler.rs index 5fde9947..d86a9c21 100644 --- a/hive-jobq/src/scheduler.rs +++ b/hive-jobq/src/scheduler.rs @@ -387,12 +387,18 @@ impl Scheduler { outcome: Outcome, grown: JobBuilder, ) -> Result<(), BuildError> { - // A node that is no longer in the graph grows nothing. The DAG it - // belonged to can be cancelled or evicted while it runs, and the insert - // below is *unchecked* — rooting on a departed parent would plant a - // dangling `parent` edge rather than being rejected. The host used to - // carry this guard itself, as a lookup before a separate append call; - // it belongs here, where the graph is and where it cannot be skipped. + // A node that is no longer in the graph grows nothing. The insert below + // is *unchecked* — rooting on a departed parent would plant a dangling + // `parent` edge rather than being rejected. The host used to carry this + // guard itself, as a lookup before a separate append call; it belongs + // here, where the graph is and where it cannot be skipped. + // + // ⚠️ Deliberately untested, and untestable today: nothing removes a node + // from the graph yet (eviction only stops *retaining* a DAG; its nodes + // linger), and `NodeId` cannot be fabricated, so a test would have to + // fake the very condition it checks. This guard is defensive against the + // bounded prune that does not exist yet — when that lands, it needs a + // test, and this comment is the reminder. let grew = if grown.is_empty() || matches!(outcome, Outcome::Failed(_)) || self.graph.node(id).is_none() @@ -704,6 +710,63 @@ mod tests { s.resources.available(&res(name)) } + /// Children of `id`, by payload, in insertion order. + fn children_of(s: &Scheduler<&'static str, String>, id: NodeId) -> Vec<&'static str> { + s.graph() + .nodes() + .filter(|n| n.parent == Some(id)) + .map(|n| n.payload) + .collect() + } + + /// A node completing `Done` gets the work it declared while running, + /// inserted **under itself** — so the DAG cannot roll terminal with the + /// appended work still pending. + #[test] + fn a_completing_node_grows_the_work_it_declared() { + let mut s = scheduler_with_slots(1); + let n = s.append("emitter", vec![], None).expect("insert"); + assert_eq!(s.settle(), vec![n]); + + let grown = s.new_job(); + grown.node("child-a"); + grown.node("child-b"); + s.complete_growing(n, Outcome::Done, grown) + .expect("well-formed growth"); + + assert_eq!(children_of(&s, n), vec!["child-a", "child-b"]); + // The emitter parks in `Finishing` rather than going terminal: its own + // appended work is still pending under it. That ordering is the whole + // point of growing *as part of* the completion. + assert_eq!(s.graph().node(n).unwrap().state, State::Finishing); + } + + /// A **failed** node grows nothing, whatever it declared. + /// + /// The companion to the test above, and the reason this rule lives in the + /// crate rather than in a caller: failure cancel-cascades to every pending + /// child of the completing node, so anything inserted here would be + /// `Skipped` by the very next statement. Enforcing it host-side means every + /// host has to remember it; enforcing it here means none can forget. + #[test] + fn a_failed_node_grows_nothing() { + let mut s = scheduler_with_slots(1); + let n = s.append("emitter", vec![], None).expect("insert"); + assert_eq!(s.settle(), vec![n]); + + let grown = s.new_job(); + grown.node("never-runs"); + s.complete_growing(n, Outcome::Failed("boom".to_owned()), grown) + .expect("growth is dropped, not rejected"); + + assert!( + children_of(&s, n).is_empty(), + "a failed node must not append work, got {:?}", + children_of(&s, n) + ); + assert_eq!(s.graph().node(n).unwrap().state, State::Failed); + } + #[test] fn leaf_owner_goes_done_directly_and_releases() { let mut s = scheduler_with_slots(1);