diff --git a/hive-c0re/src/job_queue/mod.rs b/hive-c0re/src/job_queue/mod.rs index e5d2ebee..f4ed5cc0 100644 --- a/hive-c0re/src/job_queue/mod.rs +++ b/hive-c0re/src/job_queue/mod.rs @@ -252,22 +252,6 @@ impl JobQueue { self.lock().graph().root_of(node).map(NodeId::get) } - /// A builder for a node to declare more work into while it runs. - /// - /// Handed to [`exec::run_node`] and returned to the crate's completion. - /// Only `hive_jobq` can construct one, which is why this goes through the - /// scheduler rather than `Job::default()`. - /// - /// The completion wrappers that used to live beside this (`complete_node`, - /// `complete_node_growing`) are **gone**: a node is completed inside the - /// future [`hive_jobq::scheduler::Scheduler::claim_next`] hands back, so - /// this layer has nothing left to wrap. The tests keep their own extension - /// trait for driving completions by hand. - #[must_use] - pub fn new_job(&self) -> Job { - self.lock().new_job() - } - /// Cancel a DAG that hasn't started yet: every work node is still `Pending`, /// so each is cancelled. `false` once any work node is running or terminal — /// an in-flight nix build isn't interruptible. diff --git a/hive-c0re/src/job_queue/scheduler.rs b/hive-c0re/src/job_queue/scheduler.rs index 33982ae2..897f41ad 100644 --- a/hive-c0re/src/job_queue/scheduler.rs +++ b/hive-c0re/src/job_queue/scheduler.rs @@ -101,17 +101,14 @@ pub async fn run_worker(coord: Arc) { "job_queue: node failed" ), } - let outcome = super::outcome_of(result.map_err(|e| format!("{e:#}"))); - // Growth is dropped on failure: a node that declared - // follow-up work and *then* failed does not want it run — - // failure cancel-cascades, so inserting it would only add - // nodes to immediately cancel. - let grown = if matches!(outcome, hive_jobq::scheduler::Outcome::Failed(_)) { - coord.job_queue.new_job() - } else { - grown - }; - (grown, outcome) + // Growth on a failed node is dropped by `complete_growing`, + // not here: failure cancel-cascades inside jobq, so that + // rule is the crate's to enforce and this loop does not get + // to forget it. + ( + grown, + super::outcome_of(result.map_err(|e| format!("{e:#}"))), + ) } }) }; diff --git a/hive-c0re/src/job_queue/tests.rs b/hive-c0re/src/job_queue/tests.rs index 17e231fe..1cfbdc35 100644 --- a/hive-c0re/src/job_queue/tests.rs +++ b/hive-c0re/src/job_queue/tests.rs @@ -122,9 +122,23 @@ impl ClaimReady for JobQueue { trait CompleteNode { fn complete_node(&self, node_id: NodeId, result: Result<(), String>); fn complete_node_growing(&self, node_id: NodeId, result: Result<(), String>, grown: Job); + fn new_job(&self) -> Job; } impl CompleteNode for JobQueue { + /// Mint a builder to declare growth into. + /// + /// Test-only for the same reason as the rest of this trait: production + /// never mints one, because `claim_next` hands each running node its + /// builder and takes it back. That leaves `Scheduler::new_job` with no + /// non-test caller either — see the note on that fn. + fn new_job(&self) -> Job { + self.sched() + .lock() + .expect("job_queue mutex poisoned") + .new_job() + } + fn complete_node(&self, node_id: NodeId, result: Result<(), String>) { self.sched() .lock() diff --git a/hive-jobq/src/scheduler.rs b/hive-jobq/src/scheduler.rs index 8b1e0413..5fde9947 100644 --- a/hive-jobq/src/scheduler.rs +++ b/hive-jobq/src/scheduler.rs @@ -324,7 +324,18 @@ impl Scheduler { /// propagates up the parent chain. Call [`Scheduler::settle`] again afterwards /// to start newly-unblocked work. pub fn complete(&mut self, id: NodeId, outcome: Outcome) { - self.finish(id, outcome); + match outcome { + Outcome::Failed(error) => { + // Record the reason before the terminal transition so it's set + // by the time `set_state` stamps `finished_at`. + self.graph.set_error(id, error); + self.graph.set_state(id, State::Failed); + self.cascade_cancel(id); + } + Outcome::Done => self.settle_terminal(id), + } + self.roll_up_ancestors(id); + self.release_ready(); } /// A fresh builder for a **running** node to declare more work into. @@ -335,9 +346,13 @@ impl Scheduler { /// [`NodeId`]s only at insert), so it can be filled in freely and handed /// back to [`Scheduler::complete_growing`], which inserts it under the lock. /// - /// This is the only way to get one — [`JobBuilder::new`] is `pub(crate)` and - /// there is no `Default` impl — so a caller can declare work but never - /// insert it itself. + /// ⚠️ **This has no non-test caller left, and it is the hole in + /// [`JobBuilder::new`]'s `pub(crate)` wall** — it hands out exactly the + /// builder that fn withholds. [`Scheduler::claim_next`] mints one per + /// running node itself, so production never asks. Kept only so the host's + /// graph-growth tests can still declare work by hand; the fix is a venue + /// question (move those tests here vs. a closure-form completion), not a + /// rename. #[must_use] pub fn new_job(&self) -> JobBuilder { JobBuilder::new() @@ -353,6 +368,13 @@ impl Scheduler { /// outright, which is the overwhelmingly common case (most nodes grow no /// work at all). /// + /// **A failed node grows nothing**, whatever it declared. Failure + /// cancel-cascades to every pending child of `id`, so work inserted here + /// would be `Skipped` by the very next statement — the insert is not wrong, + /// it is provably pointless. This lives here rather than in the caller + /// because it is a consequence of *this crate's* cascade rule; a host that + /// had to remember it could forget it. + /// /// # Errors /// [`BuildError`] if `grown` is malformed — **and the node is still /// completed**. Its own work already happened; refusing to complete it @@ -371,7 +393,10 @@ impl Scheduler { // 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. - let grew = if grown.is_empty() || self.graph.node(id).is_none() { + let grew = if grown.is_empty() + || matches!(outcome, Outcome::Failed(_)) + || self.graph.node(id).is_none() + { Ok(()) } else { let graph = &mut self.graph; @@ -381,29 +406,10 @@ impl Scheduler { }) .map(|_ids| ()) }; - self.finish(id, outcome); + self.complete(id, outcome); grew } - /// The completion half, shared by [`Scheduler::complete`] and - /// [`Scheduler::complete_growing`] so neither is a redirect through the - /// other: the growing form must insert *before* this runs, and the plain - /// form must not pay for an empty job. - fn finish(&mut self, id: NodeId, outcome: Outcome) { - match outcome { - Outcome::Failed(error) => { - // Record the reason before the terminal transition so it's set - // by the time `set_state` stamps `finished_at`. - self.graph.set_error(id, error); - self.graph.set_state(id, State::Failed); - self.cascade_cancel(id); - } - Outcome::Done => self.settle_terminal(id), - } - self.roll_up_ancestors(id); - self.release_ready(); - } - /// Whether every direct child of `id` is terminal. fn all_children_terminal(&self, id: NodeId) -> bool { self.graph