jobq: complete is the completion, and failure grows nothing
Two review findings from the previous round, re-checked against the actual tree rather than against my notes. `Scheduler::complete` was still a public wrapper whose entire body was `self.finish(id, outcome)`. Its docstring argued the split was not a redirect because both completion forms shared `finish` -- but sharing a private helper is not a reason for two public names. `finish`'s body now lives in `complete`, and `complete_growing` calls it. Same sharing, one name, no redirect. Growth on a failed node is now dropped by `complete_growing` instead of by the host loop. Failure cancel-cascades to every pending child of the completing node, and grown work is inserted as its children, so anything appended here is Skipped by the next statement -- the insert is not wrong, it is provably pointless. That is a consequence of this crate's cascade rule, so this crate should be the one enforcing it; a host that has to remember it can forget it. Behaviour is unchanged: hive-c0re already dropped growth before calling, and now no longer has to. `Scheduler::new_job` is left alone but documented for what it is: the hole in `JobBuilder::new`'s pub(crate) wall, with no non-test caller since claim_next mints a builder per running node. Closing it is a venue question rather than a rename, so it stays for now.
This commit is contained in:
parent
ab53f6710d
commit
8459cc66bd
4 changed files with 53 additions and 52 deletions
|
|
@ -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.
|
||||
|
|
|
|||
|
|
@ -101,17 +101,14 @@ pub async fn run_worker(coord: Arc<Coordinator>) {
|
|||
"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:#}"))),
|
||||
)
|
||||
}
|
||||
})
|
||||
};
|
||||
|
|
|
|||
|
|
@ -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()
|
||||
|
|
|
|||
Loading…
Reference in a new issue