From 657d1b506191c999fb366c0cfaaf2904d3ae4baa Mon Sep 17 00:00:00 2001 From: atlas Date: Mon, 27 Jul 2026 19:24:26 +0200 Subject: [PATCH] feat(#2788): carry Skipped to the wire as its own state MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A node ruled out by its own dependency edges settles `Skipped` host-side, but the wire folded it into `Cancelled` and `dag_view` filtered it out entirely, so a client never saw which branch a run didn't take. Post-#2785 that is not a rare shape: every approval DAG has two not-taken tails and every rebuild has one, on the happy path as much as on failure. `State` gains `Skipped`, and it counts as terminal — the wait loops in hivectl's progress display and the daemon's dag-settled check decide "finished" with `all(is_terminal)`, so omitting it would hang them on essentially every DAG. `dag_view` now emits skipped nodes but no longer lets them keep a DAG alive. Serialization and completion were the same expression: a DAG left the snapshot because its nodes had all been filtered away. Keeping skipped nodes on the wire under that rule would pin every finished deploy in the queue view forever, so the completion test is now its own flag. `rollupState` in the dashboard gains the matching arm. It has no `done` case — `done` is inferred by falling off the wire — so its trailing `return 'queued'` catches anything it doesn't recognise, and a green deploy would have read as permanently queued the moment the backend started emitting the new state. The Rust and JS roll-ups have silently disagreed before; they are edited together here and say so. --- frontend/packages/dashboard/src/builds.js | 4 ++ hive-c0re/src/job_queue/mod.rs | 31 +++++++------- hive-c0re/src/job_queue/tests.rs | 52 +++++++++++++++++------ hive-host-sock/src/jobs.rs | 34 ++++++++++++--- hivectl/src/dag_progress.rs | 3 ++ 5 files changed, 88 insertions(+), 36 deletions(-) diff --git a/frontend/packages/dashboard/src/builds.js b/frontend/packages/dashboard/src/builds.js index a20d1270..59eed2f7 100644 --- a/frontend/packages/dashboard/src/builds.js +++ b/frontend/packages/dashboard/src/builds.js @@ -34,6 +34,10 @@ function rollupState(nodes) { if (ns.some((n) => n.state === 'failed')) return 'failed'; if (ns.some((n) => n.state === 'cancelled')) return 'cancelled'; if (ns.some((n) => n.state === 'running')) return 'running'; + // A skipped node is a branch the run ruled out, which is expected on a + // healthy DAG — it must not make the roll-up read as still-pending. This + // mirrors `DagView::rollup_state` in hive-host-sock; edit the two together. + if (ns.every((n) => n.state === 'skipped' || n.state === 'done')) return 'done'; return 'queued'; } diff --git a/hive-c0re/src/job_queue/mod.rs b/hive-c0re/src/job_queue/mod.rs index f03ffe45..5f26e253 100644 --- a/hive-c0re/src/job_queue/mod.rs +++ b/hive-c0re/src/job_queue/mod.rs @@ -135,19 +135,14 @@ impl Default for JobQueue { /// Map a crate node state onto the wire state (`Pending` ↔ `Queued`; /// `Finishing` — own logic done, sub-nodes still running — reads as `Running`). -/// -/// `Skipped` has no wire counterpart and folds into `Cancelled`: to a reader -/// both mean "this never ran". The distinction is a *scheduling* one — it -/// decides whether a parent's roll-up counts the node — and the wire carries no -/// roll-up input, only display state. In practice a client never sees it either: -/// `dag_view` drops skipped nodes from the snapshot along with `Done` ones. fn to_wire_state(state: JobState) -> State { match state { JobState::Pending => State::Queued, JobState::Running | JobState::Finishing => State::Running, JobState::Done => State::Done, JobState::Failed => State::Failed, - JobState::Cancelled | JobState::Skipped => State::Cancelled, + JobState::Cancelled => State::Cancelled, + JobState::Skipped => State::Skipped, } } @@ -599,11 +594,17 @@ impl QueueInner { /// off each `hive_jobq::Node`; the client derives the DAG label, roll-up /// state, and DAG timestamps from the node set. Non-derivable per-node /// payload (`approval_id`, meta `inputs`) rides the owning node. Returns - /// `None` when every work node is `Done` — a fully-completed DAG drops - /// out of the snapshot entirely (a `Failed` one lingers until aged out). + /// `None` when every work node is `Done` or `Skipped` — a fully-settled + /// DAG drops out of the snapshot entirely (a `Failed` one lingers until + /// aged out). fn dag_view(&self, container: NodeId) -> Option { let meta = self.dag_meta(container)?; let mut nodes = Vec::new(); + // Whether anything in this DAG still has an outcome worth showing. + // Kept separate from `nodes` being non-empty: skipped nodes ride the + // wire so the dashboard can mark the branches that weren't taken, but + // they must not by themselves hold a finished DAG in the snapshot. + let mut any_unsettled = false; // DAG-level timestamps are taken over *all* subtree nodes (including the // `Done` ones excluded from the wire) — the client can't derive them // from a `Done`-filtered node set, so the host computes them here. @@ -619,13 +620,13 @@ impl QueueInner { if let Some(f) = node.finished_at { finished.push(f); } - // `Done` nodes drop off the wire (a finished step isn't interesting), - // and so do `Skipped` ones: a branch that was never taken is noise on - // the dashboard, and surfacing it would also drag the client-side - // roll-up toward `Cancelled` for a run that went fine. - if matches!(node.state, JobState::Done | JobState::Skipped) { + // `Done` nodes drop off the wire — a finished step isn't + // interesting. `Skipped` ones stay: which branch a run *didn't* + // take is the readable half of an outcome-branched DAG. + if matches!(node.state, JobState::Done) { continue; } + any_unsettled |= !matches!(node.state, JobState::Skipped); let deps: Vec = node .deps .iter() @@ -671,7 +672,7 @@ impl QueueInner { parent, }); } - if nodes.is_empty() { + if !any_unsettled { return None; } let is_terminal = self.dag_is_terminal(container); diff --git a/hive-c0re/src/job_queue/tests.rs b/hive-c0re/src/job_queue/tests.rs index 35753a8e..d94cdf58 100644 --- a/hive-c0re/src/job_queue/tests.rs +++ b/hive-c0re/src/job_queue/tests.rs @@ -83,8 +83,8 @@ fn settle_rebuild_tail(q: &JobQueue, dag_id: u64, agent: &str, expect_ok: bool) } fn state_of(q: &JobQueue, dag_id: u64) -> State { - // A fully-`Done` DAG drops out of the snapshot (its nodes are all - // excluded) — absence is the completion signal, so map it to `Done`. + // A DAG whose nodes have all settled `Done` or `Skipped` drops out of the + // snapshot — absence is the completion signal, so map it to `Done`. // Otherwise derive the roll-up from the node set, exactly as every wire // consumer does. q.snapshot() @@ -310,6 +310,27 @@ fn non_graceful_rebuild_has_no_signal_or_drain() { ); } +/// A cleanly-finished DAG leaves the snapshot even though its not-taken +/// failure branch is still in the graph as `Skipped`. Skipped nodes ride the +/// wire so the dashboard can mark them, which makes "the node list is empty" +/// and "nothing here is still worth showing" two different questions — only +/// the second one may drop the DAG. Conflating them pins every completed +/// deploy in the queue view forever. +#[test] +fn settled_dag_leaves_the_snapshot_despite_its_skipped_branch() { + let q = JobQueue::new(1); + let id = submit(&q, rebuild("agent-a", "r")); + for _ in 0..6 { + let c = claim_one(&q); + q.complete_node(id, c.node_id, Ok(())); + } + settle_rebuild_tail(&q, id, "agent-a", true); + assert!( + q.snapshot().iter().all(|d| d.id != id), + "a fully settled DAG drops out of the snapshot" + ); +} + // ---- build slots ---- #[test] @@ -858,16 +879,14 @@ fn failed_node_cancels_downstream_but_afterany_reconcile_runs() { }; assert_eq!(by_kind("prebuild"), State::Failed); // `StopForUpdate` / `Swap` / `PostSwap` were *ruled out* by the failed - // `Prebuild` — `Skipped`, and skipped nodes are filtered off the wire along - // with `Done` ones. The failure itself is still visible (the `prebuild` row - // above, and the roll-up), which is the part an operator acts on. - // Restoring that detail wants a real `Skipped` wire state the client renders - // as "not run" — surfacing them as `Cancelled` instead would make a - // *successful* DAG with a not-taken branch read as cancelled. + // `Prebuild`. They ride the wire as `Skipped` so an operator can see which + // steps the run never reached, without them reading as failures of their + // own — the roll-up ignores `Skipped` entirely. for ruled_out in ["stop_for_update", "swap", "post_swap"] { - assert!( - dag.nodes.iter().all(|n| n.kind != ruled_out), - "{ruled_out} was ruled out, so it is off the wire" + assert_eq!( + by_kind(ruled_out), + State::Skipped, + "{ruled_out} was ruled out, so it is on the wire as skipped" ); } // The AfterAny reconcile ran (claimed + completed Ok above) → it's `Done`, @@ -908,9 +927,14 @@ fn swap_failure_still_runs_reconcile() { q.complete_node(id, reconcile.node_id, Ok(())); let all_dags = q.snapshot(); let dag = all_dags.iter().find(|d| d.id == id).expect("dag"); - assert!( - dag.nodes.iter().all(|n| n.kind != "post_swap"), - "PostSwap is ruled out by the failed Swap (`Skipped`, so off the wire)" + assert_eq!( + dag.nodes + .iter() + .find(|n| n.kind == "post_swap") + .expect("post_swap node") + .state, + State::Skipped, + "PostSwap is ruled out by the failed Swap, and says so on the wire" ); assert_eq!( dag.nodes diff --git a/hive-host-sock/src/jobs.rs b/hive-host-sock/src/jobs.rs index 5fa5d24b..876eda1a 100644 --- a/hive-host-sock/src/jobs.rs +++ b/hive-host-sock/src/jobs.rs @@ -49,12 +49,25 @@ pub enum State { Done, Failed, Cancelled, + /// The node's own dependency edges ruled it out: an outcome branch that + /// wasn't taken. Distinct from `Cancelled`, which is work actively + /// dropped. A skipped node is an expected part of a healthy run — every + /// approval DAG has two not-taken tails and every rebuild has one — so + /// consumers must not read it as a failure or cancellation signal. + Skipped, } impl State { + /// Whether the node will never change state again. `Skipped` counts: + /// a branch that was ruled out is as final as one that ran, and the + /// wait loops (`hivectl`'s progress display, the daemon's + /// dag-settled check) hang forever if it doesn't. #[must_use] pub fn is_terminal(self) -> bool { - matches!(self, State::Done | State::Failed | State::Cancelled) + matches!( + self, + State::Done | State::Failed | State::Cancelled | State::Skipped + ) } } @@ -160,9 +173,12 @@ pub struct DagView { /// `started_at`. `None` while the DAG is still live. #[serde(default, skip_serializing_if = "Option::is_none")] pub finished_at: Option>, - /// Nodes of this DAG with `Done` ones excluded. A DAG whose nodes are - /// all `Done` is omitted from the snapshot entirely; a `Failed` DAG - /// lingers until aged out by the history cap. + /// Nodes of this DAG with `Done` ones excluded. `Skipped` nodes are + /// carried so the dashboard can show which branches weren't taken, but + /// they don't keep a DAG alive: one whose nodes are all `Done` or + /// `Skipped` is omitted from the snapshot entirely, and its absence is + /// what signals completion. A `Failed` DAG lingers until aged out by the + /// history cap. pub nodes: Vec, } @@ -182,9 +198,13 @@ impl DagView { /// the states barely co-occur: a cancel *cascade* originates at a `Failed` /// node, which returns early above. /// + /// `Skipped` contributes nothing: a not-taken branch is an expected part of + /// a healthy run, so counting it would make every successful DAG roll up + /// non-`Done`. + /// /// This ordering matches `frontend/packages/dashboard/src/builds.js`'s - /// `rollupState`, which has always ranked cancelled second — the two had - /// silently disagreed, and this is the side that was wrong. + /// `rollupState`. The two implementations must be edited together — they + /// have silently disagreed before. #[must_use] pub fn rollup_state(&self) -> State { let mut any_running = false; @@ -196,7 +216,7 @@ impl DagView { State::Running => any_running = true, State::Queued => any_queued = true, State::Cancelled => any_cancelled = true, - State::Done => {} + State::Done | State::Skipped => {} } } if any_cancelled { diff --git a/hivectl/src/dag_progress.rs b/hivectl/src/dag_progress.rs index ec32b6d8..6f8e7d2d 100644 --- a/hivectl/src/dag_progress.rs +++ b/hivectl/src/dag_progress.rs @@ -291,6 +291,9 @@ fn state_glyph(state: hive_host_sock::jobs::State) -> &'static str { hive_host_sock::jobs::State::Done => "✔", hive_host_sock::jobs::State::Failed => "✖", hive_host_sock::jobs::State::Cancelled => "⊘", + // Distinct from cancelled: nothing went wrong, this branch just + // wasn't the one the run took. + hive_host_sock::jobs::State::Skipped => "·", } }