fix(#2756): a cancelled DAG must not read back as Queued
Review catch from argus on #2770: with the terminal hook gone, cancel spares the DAG's tail node so it can report the cancellation — which leaves that node `Pending` until the scheduler's next pass. `post_rebuild_queue_cancel` emits its snapshot synchronously, and `rollup_state` ranked `Queued` above `Cancelled`, so the operator who just cancelled a DAG saw it go back to **Queued**: "the cancel didn't take". Asserted in `cancel_clears_queued_dag`, which failed before this. Fixing it surfaced an older disagreement. `builds.js::rollupState` has always been `failed > cancelled > running > queued`; the Rust was `failed > running > queued > cancelled`. The two had silently diverged under a doc-comment claiming they agree. Harmless until now only because cancelled nodes never coexisted with live ones — a spared tail running over cancelled work would have rendered `Running` host-side and `Cancelled` in the dashboard. The JS was the correct side, so the Rust moves to match it exactly: `Failed > Cancelled > Running > Queued > Done`. No frontend change.
This commit is contained in:
parent
e8e6998ac5
commit
36918e0432
2 changed files with 21 additions and 5 deletions
|
|
@ -934,6 +934,11 @@ fn cancel_clears_queued_dag() {
|
|||
let q = JobQueue::new(1);
|
||||
let id = submit(&q, rebuild("agent-a", "r"));
|
||||
assert!(q.cancel(id), "fully-queued dag cancels");
|
||||
// The operator sees `Cancelled` the moment the cancel returns — the spared
|
||||
// tail is still `Pending`, and a DAG must not read `Queued` back to the
|
||||
// operator who just cancelled it (the dashboard renders this roll-up from
|
||||
// the snapshot `post_rebuild_queue_cancel` emits synchronously).
|
||||
assert_eq!(state_of(&q, id), State::Cancelled, "no stale Queued gap");
|
||||
// Every work node is `Cancelled`, but the tail is spared so it can still
|
||||
// report the cancellation — so it is the one thing left to claim.
|
||||
let tail = claim_one(&q);
|
||||
|
|
|
|||
|
|
@ -170,10 +170,21 @@ impl DagView {
|
|||
/// Roll-up state derived from the node set — the shared derivation every
|
||||
/// Rust consumer (hivectl, the wait loops, tests) uses so the dashboard's
|
||||
/// JS render and the host agree: `Failed` if any node failed, else
|
||||
/// `Running` if any running, else `Queued` if any queued, else
|
||||
/// `Cancelled` if any cancelled, else `Done`. `Done` nodes are excluded
|
||||
/// **`Cancelled` if any cancelled**, else `Running` if any running, else
|
||||
/// `Queued` if any queued, else `Done`. `Done` nodes are excluded
|
||||
/// from the wire, so a DAG that is *entirely* done isn't sent at all —
|
||||
/// its absence from the snapshot is what signals completion.
|
||||
///
|
||||
/// `Cancelled` outranks both `Running` and `Queued` because a cancelled DAG
|
||||
/// still has its weak-edged tail node to run (it reports the cancellation),
|
||||
/// so `Queued`-then-`Running` would flicker back at the operator who just
|
||||
/// cancelled it and read as "the cancel didn't take". Outside that window
|
||||
/// the states barely co-occur: a cancel *cascade* originates at a `Failed`
|
||||
/// node, which returns early above.
|
||||
///
|
||||
/// 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.
|
||||
#[must_use]
|
||||
pub fn rollup_state(&self) -> State {
|
||||
let mut any_running = false;
|
||||
|
|
@ -188,12 +199,12 @@ impl DagView {
|
|||
State::Done => {}
|
||||
}
|
||||
}
|
||||
if any_running {
|
||||
if any_cancelled {
|
||||
State::Cancelled
|
||||
} else if any_running {
|
||||
State::Running
|
||||
} else if any_queued {
|
||||
State::Queued
|
||||
} else if any_cancelled {
|
||||
State::Cancelled
|
||||
} else {
|
||||
State::Done
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in a new issue