From 56202065d59ec879f1ebac649435d1840fb5ba12 Mon Sep 17 00:00:00 2001 From: atlas Date: Sat, 1 Aug 2026 17:30:05 +0200 Subject: [PATCH] refactor(#2815): the scheduler publishes pill edges, it doesn't own pills MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit mara on !2910: "transient guard as well - should be removable now?" — for the queue path, yes. `set_transient`'s own doc explained why the RAII guard existed: a cancelled future must not leak an imperatively-set transient and pin the dashboard on "rebuilding…" forever. That cannot happen to a derived set. `running_transients()` is recomputed from the graph every loop, so a node that stops running stops appearing — there is nothing to own and nothing to leak. So the scheduler no longer holds a guard per pill. It keeps the previous derived value and publishes the transitions, which is the one thing a derived read cannot express: the dashboard wants `TransientSet` / `TransientCleared` edges, and the crash watcher wants the *moment* a pill cleared, since its grace window is what stops an operator stop from reading as a crash. That also retires a hazard rather than restating it. The old code carried a warning that stale guards had to be dropped before new ones were created, because `TransientGuard::drop` clears by agent with no notion of which label it was clearing — so a same-agent label change could clear the pill it had just set. With no guards there is no ordering to get wrong; clears are emitted before sets so a relabel reads as clear-then-set rather than two overlapping pills. `set_transient` / `clear_transient` become `pub(crate)`. The guard stays for destroy and migration, which have no node behind them and where the cancellation concern is real. Checked with clippy (`--all-targets -D warnings`), `cargo test -p hive-c0re -p hive-jobq` (322 + 41 passed) and `nix fmt`. --- hive-c0re/src/coordinator.rs | 27 +++++++----- hive-c0re/src/job_queue/scheduler.rs | 66 +++++++++++++++++----------- 2 files changed, 57 insertions(+), 36 deletions(-) diff --git a/hive-c0re/src/coordinator.rs b/hive-c0re/src/coordinator.rs index 196b95ac..7325d92e 100644 --- a/hive-c0re/src/coordinator.rs +++ b/hive-c0re/src/coordinator.rs @@ -1077,13 +1077,17 @@ impl Coordinator { /// Mark an agent as in-progress (only one state per agent for now). /// - /// Private on purpose: the RAII [`TransientGuard`] (via - /// [`Coordinator::transient_guard`]) is the only door, so the paired - /// `clear_transient` always runs on drop even if the surrounding future - /// is cancelled (HTTP request aborted, runtime shutdown mid-rebuild, - /// panic). A bare set with no guaranteed clear would leak the transient - /// and leave the dashboard stuck in "rebuilding…" forever. - fn set_transient(&self, name: &str, label: String, deliberate_stop: bool) { + /// Two callers, for two different reasons: + /// - **Work with a queue node behind it** — the job-queue scheduler, which + /// publishes the edges of a *derived* set + /// ([`crate::job_queue::JobQueue::running_transients`]). It needs no guard: + /// nothing is owned, and a node that stops running stops appearing. + /// - **Work with no node** (destroy, migration) — via the RAII + /// [`TransientGuard`], so the paired `clear_transient` still runs if the + /// surrounding future is cancelled (HTTP request aborted, shutdown + /// mid-rebuild, panic). There a bare set really would leak the transient + /// and pin the dashboard on "rebuilding…" forever. + pub(crate) fn set_transient(&self, name: &str, label: String, deliberate_stop: bool) { self.transient.lock().unwrap().insert( name.to_owned(), TransientState { @@ -1108,10 +1112,11 @@ impl Coordinator { }); } - /// Clear an agent's transient state. Private: only reachable through - /// [`TransientGuard`]'s `Drop`, which guarantees it runs (see - /// [`Coordinator::set_transient`]). - fn clear_transient(&self, name: &str) { + /// Clear an agent's transient state. Reached either from + /// [`TransientGuard`]'s `Drop` (the no-node callers) or from the scheduler + /// when a derived pill stops being current — see + /// [`Coordinator::set_transient`]. + pub(crate) fn clear_transient(&self, name: &str) { let removed = self.transient.lock().unwrap().remove(name); if let Some(state) = removed { // Stamp the tombstone so the crash watcher can still see diff --git a/hive-c0re/src/job_queue/scheduler.rs b/hive-c0re/src/job_queue/scheduler.rs index 2df092b6..492aa55d 100644 --- a/hive-c0re/src/job_queue/scheduler.rs +++ b/hive-c0re/src/job_queue/scheduler.rs @@ -54,10 +54,12 @@ struct NodeDone { pub async fn run_worker(coord: Arc) { let mut shutdown = coord.shutdown_rx(); let (tx, mut rx) = tokio::sync::mpsc::unbounded_channel::(); - // Keyed by agent (its lease is cap-1, so one pill each); the label rides - // along so a change of label can be detected and the guard swapped. - let mut transients: HashMap = - HashMap::new(); + // Last derived pill set we published, keyed by agent (its lease is cap-1, + // so one pill each). Purely the previous value of a *derived* quantity — + // it exists to spot transitions, since the dashboard wants edges + // (`TransientSet` / `TransientCleared`) and the crash watcher wants the + // moment of the clear. Nothing owns a pill; nothing can leak one. + let mut transients: HashMap = HashMap::new(); loop { // Checked every iteration, not just in the `select!` below — a // continuous stream of ready claims never reaches the `select!`, so @@ -148,10 +150,23 @@ fn handle_completion(coord: &Arc, done: NodeDone) { coord.emit_rebuild_queue_snapshot(); } -/// Reconcile the transient-guard set against the live pill set: drop guards for -/// pills that are no longer current, create one for each newly-current -/// `(agent, label)`. Keyed by agent — an agent's lease is cap-1, so it has at -/// most one pill. +/// Publish the transitions between the previously-derived pill set and the +/// current one. `prev` is last loop's derived value, keyed by agent (an agent's +/// lease is cap-1, so at most one pill each). +/// +/// The pill set itself isn't owned or stored here — it is +/// [`super::JobQueue::running_transients`], recomputed from the graph. What this +/// publishes is the **edges**, which a derived read can't express on its own: +/// the dashboard wants `TransientSet` / `TransientCleared` events, and the crash +/// watcher wants the *moment* a pill cleared (its grace window is what keeps an +/// operator stop from reading as a crash). +/// +/// There used to be an RAII `TransientGuard` per pill here, and a hazard note +/// about dropping stale guards before creating new ones or a same-agent label +/// change would clear the pill it had just set. Both are gone: a guard exists so +/// a cancelled future can't *leak* an imperatively-set transient, and a derived +/// set has nothing to leak — a node that stops running simply stops appearing. +/// (Destroy and migration still take guards; they have no node behind them.) /// /// `deliberate_stop` rides along per node ([`NodeKind::takes_container_down`]) /// rather than being blanket-`true` for anything holding a lease: `Create` and @@ -159,24 +174,25 @@ fn handle_completion(coord: &Arc, done: NodeDone) { /// starting* is a real crash that must keep reporting as one. /// /// [`NodeKind::takes_container_down`]: super::NodeKind::takes_container_down -/// -/// ⚠️ **`retain` must run to completion before anything is created.** -/// `TransientGuard::drop` calls `clear_transient(agent)` — keyed by agent alone, -/// with no notion of *which* label it was clearing. So when a pill's label -/// changes for the same agent (which is now routine: the label follows the -/// running node as a DAG advances), creating the new guard first and dropping -/// the old second would clear the pill that was just set. Dropping first is what -/// makes the swap safe. -fn reconcile_transients( - coord: &Arc, - transients: &mut HashMap, -) { +fn reconcile_transients(coord: &Arc, prev: &mut HashMap) { let running = coord.job_queue.running_transients(); - transients.retain(|agent, (label, _)| running.iter().any(|(a, l, _)| a == agent && l == label)); + + // Cleared: in `prev`, gone (or relabelled) now. Emitted before the sets + // below so a same-agent label change reads as clear-then-set rather than + // two overlapping pills. + prev.retain(|agent, label| { + let still = running.iter().any(|(a, l, _)| a == agent && l == label); + if !still { + coord.clear_transient(agent); + } + still + }); + for (agent, label, takes_down) in running { - transients.entry(agent.clone()).or_insert_with(|| { - let guard = coord.transient_guard(&agent, label.clone(), takes_down); - (label, guard) - }); + if prev.get(&agent) == Some(&label) { + continue; + } + coord.set_transient(&agent, label.clone(), takes_down); + prev.insert(agent, label); } }