jobq: test the growth invariants where they are enforced

complete_growing had no test at all -- its only reference was the
internal call from claim_next -- so the rule moved into it in the
previous commit was enforced but unproven.

  - a_completing_node_grows_the_work_it_declared: the declared work
    lands under the emitter, and the emitter parks in Finishing rather
    than going terminal. That ordering is the point of growing as part
    of the completion.
  - a_failed_node_grows_nothing: the rule that moved out of the host.

Both were mutation-checked rather than trusted green: with the
Outcome::Failed guard deleted, a_failed_node_grows_nothing fails on the
appended node while its companion still passes, so the test bites and
the drop is specific to failure rather than blanket.

The departed-parent guard beside it stays untested and says so. Nothing
removes a node from the graph yet (eviction stops retaining a DAG; its
nodes linger) and NodeId cannot be fabricated by construction, so a test
would have to fake the precondition it checks. The comment names the
bounded prune as the point at which it becomes testable.
This commit is contained in:
atlas 2026-08-02 19:20:08 +02:00 committed by mara
commit 335ad5e0ee

View file

@ -387,12 +387,18 @@ impl<N, R: Clone + Eq + Hash> Scheduler<N, R> {
outcome: Outcome,
grown: JobBuilder<N, R>,
) -> 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);