refactor(#2908): cancel a node, not a DAG
`JobQueue::cancel(dag_id)` resolved the id to a `NodeKind::Dag` container
and cancelled that. But the container lookup was the only DAG-specific
part — everything that makes cancel work already lives in the scheduler:
`cancel_node` marks the node `Cancelled` and cascades to its pending
descendants, sparing any node whose edge accepts `Cancelled` (which is
what keeps a dropped approval DAG from dangling its row).
So `cancel` now takes any node id. A group root cancels the whole group,
which is what the dashboard's button does today and why nothing about
its behaviour changes: a DAG id *is* its root node's id. An interior
node cancels just that branch — a capability the DAG-scoped version
could not express, covered by the new test (a hive-wide restart drops
one agent's subgraph while the other keeps running).
`QueueInner::node_by_id` replaces `container()` here: same search, same
cost, without asserting the node is a DAG container. `container()` stays
for `first_error` and the append-subgraph guard, which are genuinely
DAG-scoped.
No wire change. The route is `POST /api/rebuild-queue/{id}/cancel` with
a `u64` path param — same type, same route, and the client keeps sending
the same number. Only the param's documented meaning moves from "DAG id"
to "node id".
Checked with clippy (`--all-targets -D warnings`), `cargo test -p
hive-c0re` (322 passed) and `nix fmt`. No option surface touched, so no
nix-eval gate.
This commit is contained in:
parent
4835ca8c91
commit
eb557ee3c1
3 changed files with 60 additions and 6 deletions
|
|
@ -153,8 +153,15 @@ pub(super) async fn post_schedule_fire_now(
|
|||
}
|
||||
}
|
||||
|
||||
/// `POST /api/rebuild-queue/{id}/cancel` — drop a still-fully-queued
|
||||
/// DAG from the job queue. Refuses `Running` / terminal DAGs: an
|
||||
/// `POST /api/rebuild-queue/{id}/cancel` — drop still-queued work from the job
|
||||
/// queue.
|
||||
///
|
||||
/// `id` is a **node** id. A DAG's root cancels the whole group (the scheduler
|
||||
/// cascades to pending descendants), which is what the dashboard's cancel
|
||||
/// button sends today — a DAG id *is* its root node's id. An interior node
|
||||
/// cancels just that branch.
|
||||
///
|
||||
/// Refuses `Running` / terminal nodes: an
|
||||
/// in-flight node owns the agent's nix store + nixos-container update
|
||||
/// lock and can't be safely interrupted from the queue side. Always
|
||||
/// returns 200; the body is `{"cancelled": true}` on a successful
|
||||
|
|
@ -164,7 +171,7 @@ pub(super) async fn post_schedule_fire_now(
|
|||
#[utoipa::path(
|
||||
post,
|
||||
path = "/api/rebuild-queue/{id}/cancel",
|
||||
params(("id" = u64, Path, description = "job-queue DAG id")),
|
||||
params(("id" = u64, Path, description = "job-queue node id (a DAG's root cancels the group)")),
|
||||
responses((status = 200, description = "whether the DAG was cancelled", body = serde_json::Value)),
|
||||
tag = "schedules"
|
||||
)]
|
||||
|
|
|
|||
|
|
@ -345,12 +345,16 @@ impl JobQueue {
|
|||
/// say) is cancelled along with everything else — there is nothing to converge
|
||||
/// when no node ever ran. Only a node that named `Cancelled` survives, and it
|
||||
/// survives because it asked to.
|
||||
pub fn cancel(&self, dag_id: u64) -> bool {
|
||||
/// `id` names **any node**, not specifically a DAG. Cancelling a group root
|
||||
/// drops that whole group (the cascade is the scheduler's), which is what
|
||||
/// the dashboard's whole-DAG cancel does; cancelling an interior node drops
|
||||
/// just that branch. Nothing here knows about DAGs.
|
||||
pub fn cancel(&self, id: u64) -> bool {
|
||||
let mut inner = self.lock();
|
||||
let Some(container) = inner.container(dag_id) else {
|
||||
let Some(node) = inner.node_by_id(id) else {
|
||||
return false;
|
||||
};
|
||||
if !inner.sched.cancel_node(container) {
|
||||
if !inner.sched.cancel_node(node) {
|
||||
return false;
|
||||
}
|
||||
drop(inner);
|
||||
|
|
@ -460,6 +464,17 @@ impl QueueInner {
|
|||
|
||||
/// The container node of `dag_id` — the `NodeKind::Dag` root whose id equals
|
||||
/// `dag_id`. `NodeId` is un-fabricable from a raw `u64`, so this is a search.
|
||||
/// Resolve a raw wire `u64` to a graph [`NodeId`], whatever kind of node it
|
||||
/// names. `NodeId` is un-fabricable from a `u64`, so this is a search —
|
||||
/// same cost as [`QueueInner::container`], without asserting the node is a
|
||||
/// DAG container.
|
||||
fn node_by_id(&self, id: u64) -> Option<NodeId> {
|
||||
self.sched
|
||||
.graph()
|
||||
.nodes()
|
||||
.find_map(|n| (n.id.get() == id).then_some(n.id))
|
||||
}
|
||||
|
||||
fn container(&self, dag_id: u64) -> Option<NodeId> {
|
||||
self.sched.graph().nodes().find_map(|n| {
|
||||
(n.parent.is_none()
|
||||
|
|
|
|||
|
|
@ -1010,6 +1010,38 @@ fn cancel_clears_queued_dag() {
|
|||
assert_eq!(state_of(&q, id), State::Cancelled);
|
||||
}
|
||||
|
||||
/// `cancel` takes a **node** id, not a DAG id — so an interior node can be
|
||||
/// dropped without touching the rest of the group.
|
||||
///
|
||||
/// This is the capability the DAG-scoped version couldn't express, and the
|
||||
/// reason it reads naturally: a DAG id *is* its root node's id, so the
|
||||
/// whole-group cancel every other test does is just this called on a root.
|
||||
/// Here a hive-wide restart drops **one agent's** subgraph and the other agent
|
||||
/// still runs.
|
||||
#[test]
|
||||
fn cancel_drops_one_agents_branch_leaving_the_rest() {
|
||||
let q = JobQueue::new(2);
|
||||
let id = submit(&q, restart_online(&["agent-a", "agent-b"], false, "r"));
|
||||
// Per-agent subgraphs are independent roots; find agent-a's.
|
||||
let snap = q.snapshot();
|
||||
let dag = snap.iter().find(|d| d.id == id).expect("dag in snapshot");
|
||||
let a_root = dag
|
||||
.nodes
|
||||
.iter()
|
||||
.find(|n| n.agent == "agent-a" && n.parent.is_none())
|
||||
.expect("agent-a has a group root");
|
||||
|
||||
assert!(q.cancel(a_root.id), "an interior/group root cancels alone");
|
||||
|
||||
// agent-b's work is untouched and still claimable; agent-a's is not.
|
||||
let claims = q.claim_ready();
|
||||
assert!(
|
||||
!claims.is_empty() && claims.iter().all(|c| c.agent == "agent-b"),
|
||||
"only agent-b remains runnable, got {:?}",
|
||||
claims.iter().map(|c| c.agent.as_str()).collect::<Vec<_>>()
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn cancel_refuses_running_dag() {
|
||||
let q = JobQueue::new(1);
|
||||
|
|
|
|||
Loading…
Reference in a new issue