From 96a06799343ee50301f227009d3e61d7957ac361 Mon Sep 17 00:00:00 2001 From: atlas Date: Sun, 2 Aug 2026 19:50:47 +0200 Subject: [PATCH] job_queue: drop the erased-recipe test infra ErasedRecipe and erase() existed for one test, which put three power-op cases in a single array. Three recipe closures have distinct types and cannot share an array element type, so all three were boxed. The array was the reason, not the specs. The per-case assertion is now a helper taking the already-submitted DAG id, so each case submits its own spec at its own concrete type. Nothing is erased and nothing is boxed; production never needed either. The final assertion is also stronger than the one it replaces. claim_ready().is_empty() asks what is runnable at this instant, which a node that is alive but blocked on a dependency passes -- exactly what a leftover compensating node would look like. It now asserts the DAG has no Pending nodes at all. It was also a mutating call inside an assertion: claim_ready settles the graph. --- hive-c0re/src/job_queue/tests.rs | 137 ++++++++++++++----------------- 1 file changed, 61 insertions(+), 76 deletions(-) diff --git a/hive-c0re/src/job_queue/tests.rs b/hive-c0re/src/job_queue/tests.rs index 4aeb5f22..cf40a5a4 100644 --- a/hive-c0re/src/job_queue/tests.rs +++ b/hive-c0re/src/job_queue/tests.rs @@ -13,26 +13,6 @@ fn submit(q: &JobQueue, spec: DagSpec) -> u64 { q.submit(spec).expect("valid spec") } -/// A spec recipe with its concrete closure type erased. **Test-only** — the -/// module used to export this alias for the executor's growth path too, which -/// is exactly what a node declaring onto its own builder removed: nothing in -/// production stores a recipe to replay later, so nothing needs to box one. -type ErasedRecipe = Box; - -/// Erase a spec's recipe so specs of *different* shapes can share one type — -/// e.g. a table of `(name, spec)` cases. -/// -/// Production never needs this: each submit path builds one spec and hands it -/// straight to `submit`, so the concrete closure type is known end to end. A -/// test table is the one case where several shapes must be one type. -fn erase(spec: DagSpec) -> DagSpec { - DagSpec { - source: spec.source, - reason: spec.reason, - declare: Box::new(spec.declare), - } -} - fn ident(s: &str) -> hive_types::Ident { hive_types::Ident::parse(s).expect("valid test ident") } @@ -237,6 +217,20 @@ fn declared_shape(q: &JobQueue, dag: u64) -> Vec { .collect() } +/// Kinds of every node under `dag` still `Pending` — the nodes that could yet +/// run. Stronger than asking the scheduler what is *ready right now*: a node +/// blocked on a dep is not ready but is very much still alive. +fn pending_kinds(q: &JobQueue, dag: u64) -> Vec<&'static str> { + let sched = q.sched().lock().expect("job_queue mutex poisoned"); + let graph = sched.graph(); + let root = graph.resolve_id(dag).expect("dag id is a real node id"); + graph + .nodes() + .filter(|n| n.id != root && graph.root_of(n.id) == Some(root) && n.state == State::Pending) + .map(|n| n.payload.as_str()) + .collect() +} + /// Shorthand for one expected row, so the tables below read as a shape. fn row( kind: &'static str, @@ -1439,66 +1433,57 @@ fn cancel_refuses_running_dag() { /// deliberately-stopped as far as reconcile and crash-watch are concerned. #[test] fn cancelled_power_op_runs_no_compensating_node() { + /// Submit-cancel-assert for one power op. Taking the already-submitted DAG + /// id is what removes the need to put three differently-typed recipes in + /// one array: each caller submits its own spec, so no closure type has to + /// be erased to a boxed one. + fn assert_cancels_clean(q: &JobQueue, id: u64, writes_intent: bool, case: &str) { + // Read the intent head off the submitted DAG rather than out of the + // spec: a declared job holds its own nodes and inserts them. + let has_intent = declared_shape(q, id).iter().any(|d| d.kind == "set_wanted"); + assert_eq!(has_intent, writes_intent, "{case}: intent head"); + assert!(q.cancel(id), "{case}: cancelled while queued"); + assert_eq!(state_of(q, id), State::Cancelled); + // Nothing is left that *could* run. Asserting on the pending set rather + // than on "what is ready this instant" also covers a node that is alive + // but blocked — which is exactly what a leftover compensating node + // would look like. + assert_eq!( + pending_kinds(q, id), + Vec::<&str>::new(), + "{case}: a power op emits no tail node, so a cancelled one leaves nothing" + ); + } + for graceful in [false, true] { for running in [false, true] { let targets = vec![("agent-a".to_owned(), running)]; - // Erased to one boxed recipe type: three different recipe types - // have to sit in one array. - let cases = [ - ( - "restart", - false, - erase(submit::restart_spec( - &targets, - graceful, - Source::Manual, - "bounce".to_owned(), - )), + let case = format!("graceful={graceful} running={running}"); + + let q = JobQueue::new(1); + let id = submit( + &q, + submit::restart_spec(&targets, graceful, Source::Manual, "bounce".to_owned()), + ); + assert_cancels_clean(&q, id, false, &format!("restart {case}")); + + let q = JobQueue::new(1); + let id = submit( + &q, + submit::stop_spec(&targets, graceful, Source::Manual, "stop".to_owned()), + ); + assert_cancels_clean(&q, id, true, &format!("stop {case}")); + + let q = JobQueue::new(1); + let id = submit( + &q, + submit::start_spec( + &[("agent-a".to_owned(), running, false)], + Source::Manual, + "start".to_owned(), ), - ( - "stop", - true, - erase(submit::stop_spec( - &targets, - graceful, - Source::Manual, - "stop".to_owned(), - )), - ), - ( - "start", - true, - erase(submit::start_spec( - &[("agent-a".to_owned(), running, false)], - Source::Manual, - "start".to_owned(), - )), - ), - ]; - for (name, writes_intent, spec) in cases { - let q = JobQueue::new(1); - let id = submit(&q, spec); - // Read the intent head off the submitted DAG rather than out of - // the spec: a declared job holds its own nodes and inserts them. - assert_eq!( - q.snapshot() - .iter() - .find(|d| d.id == id) - .expect("submitted dag") - .nodes - .iter() - .any(|n| n.kind == "set_wanted"), - writes_intent, - "{name} intent head (graceful={graceful}, running={running})" - ); - assert!(q.cancel(id), "cancelled while queued"); - assert_eq!(state_of(&q, id), State::Cancelled); - assert!( - q.claim_ready().is_empty(), - "cancelled {name} (graceful={graceful}, running={running}) must \ - leave nothing to run — a power op emits no tail node" - ); - } + ); + assert_cancels_clean(&q, id, true, &format!("start {case}")); } } }