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 => "·", } }