diff --git a/hive-c0re/src/job_queue/exec.rs b/hive-c0re/src/job_queue/exec.rs index fa303189..8c01b7d5 100644 --- a/hive-c0re/src/job_queue/exec.rs +++ b/hive-c0re/src/job_queue/exec.rs @@ -13,7 +13,6 @@ use anyhow::{Context as _, Result}; use hive_jobq::{NodeId, TerminalState}; use super::model::NodeKind; -use super::resource::Resource; use crate::coordinator::Coordinator; use crate::power::{ReconcileAction, reconcile_action}; @@ -80,13 +79,7 @@ pub(super) async fn run_node( }), NodeKind::Reconcile { .. } => run_reconcile(coord, agent).await.map(|sub| { if let Some(kind) = sub { - // `Start` / `Stop` declare the lease they run under. This node - // is their parent and holds it, so the declaration is a - // re-entrant borrow — no second unit, no deadlock. It exists so - // the requirement belongs to the node rather than to the fact - // that a `Reconcile` happens to fan it out. - let lease = Resource::Agent(kind.agent().to_owned()); - let _ = job.node(kind).needs(lease); + super::templates::fanned_out_mechanical(&job, kind); } }), NodeKind::Start { .. } => run_start(coord, agent).await, diff --git a/hive-c0re/src/job_queue/templates.rs b/hive-c0re/src/job_queue/templates.rs index c0016c54..dcba5239 100644 --- a/hive-c0re/src/job_queue/templates.rs +++ b/hive-c0re/src/job_queue/templates.rs @@ -80,6 +80,23 @@ fn resolve_approval_tails(b: &Job, approval_id: i64, root: Handle<'_>) { } } +/// Declare the mechanical node a [`NodeKind::Reconcile`] planner fans out +/// (`Start` / `Stop`) onto the builder it was handed while running. +/// +/// `Start` / `Stop` declare the agent lease they run under. Their `Reconcile` +/// parent is holding it already, so the declaration is a **re-entrant borrow** +/// — no second unit, no deadlock. It exists so the requirement belongs to the +/// node rather than to the fact that a `Reconcile` happens to fan it out. +/// +/// Lives here rather than inline in `exec.rs` for the same reason every other +/// declaration does: this is the one construction site that was hiding in an +/// executor, which meant the only test of it had to re-declare the same two +/// calls itself and would have kept passing if the executor changed. +pub(crate) fn fanned_out_mechanical(b: &Job, kind: NodeKind) { + let lease = Resource::Agent(kind.agent().to_owned()); + let _ = b.node(kind).needs(lease); +} + /// Knobs for [`rebuild_nodes`]. A struct rather than two positional `bool`s so /// a call site cannot silently swap them. #[derive(Debug, Clone, Copy)] diff --git a/hive-c0re/src/job_queue/tests.rs b/hive-c0re/src/job_queue/tests.rs index 9602c352..497eb145 100644 --- a/hive-c0re/src/job_queue/tests.rs +++ b/hive-c0re/src/job_queue/tests.rs @@ -807,71 +807,6 @@ fn offline_agents_skip_mechanical_nodes_but_keep_reconcile() { ); } -#[test] -fn a_fanned_out_start_declares_the_lease_and_re_enters_its_reconciles_grant() { - // `Start` / `Stop` / `PostSwap` were lease-exempt *as kinds*, which was only - // safe because every construction site fans them out from inside a - // lease-holding ancestor. Now they declare the lease themselves. - // - // The contract says that costs nothing — a descendant re-enters the - // ancestor's grant instead of taking a fresh unit. That is exactly the sort - // of claim that is true until a node is used from a second site, so it is - // pinned here rather than argued: the fanned-out `Start` must (a) actually - // carry the declaration, (b) still run under its parent's grant, and - // (c) not have consumed a second unit of a cap-1 lease. - let q = JobQueue::new(4); - let id = submit( - &q, - templates::reconcile_only("agent-a", Source::Manual, "converge".to_owned()), - ); - // A competing DAG on the same agent, to prove the lease is genuinely held - // (and held *once*) across the fan-out. - let rival = submit( - &q, - templates::reconcile_only("agent-a", Source::Manual, "rival".to_owned()), - ); - - let reconcile = claim_one(&q); - assert_eq!(reconcile.dag_id, id); - assert_eq!(reconcile.kind.as_str(), "reconcile"); - - // What `run_reconcile` does on observing a down container with wanted=Up: - // declare into the builder it was handed, then hand it back with the - // completion. Same two calls the scheduler makes, in the same order. - let grown = q.new_job(); - let kind = NodeKind::Start { - agent: "agent-a".to_owned(), - }; - let lease = Resource::Agent(kind.agent().to_owned()); - let _ = grown.node(kind).needs(lease); - q.complete_node_growing(reconcile.node_id, Ok(()), grown); - - // (a) + (b): the child runs, under the parent that parked in `Finishing`. - let start = claim_one(&q); - assert_eq!(start.kind.as_str(), "start"); - assert_eq!( - declared_resources(&q, start.node_id), - vec![Resource::Agent("agent-a".to_owned())], - "a fanned-out Start declares the lease it runs under" - ); - - // (c): one unit, not two. `claim_one` above already asserted the rival did - // not come back in the same pass; make the reason explicit. - assert!( - q.claim_ready().is_empty(), - "the rival DAG's Reconcile must still be blocked — the appended Start \ - borrowed the grant rather than acquiring a second unit" - ); - - q.complete_node(start.node_id, Ok(())); - // Subtree terminal → the grant releases and the rival finally runs. - let rival_reconcile = claim_one(&q); - assert_eq!(rival_reconcile.dag_id, rival); - q.complete_node(rival_reconcile.node_id, Ok(())); - assert_eq!(state_of(&q, id), State::Done); - assert_eq!(state_of(&q, rival), State::Done); -} - #[test] fn boot_sweep_nodes_declare_their_own_resources() { // Regression, and the reason it needs its own test: `workers::auto_update` @@ -1212,6 +1147,49 @@ fn rebuild_reconcile_waits_for_the_whole_build_subtree() { // `cancelled_power_op_runs_no_compensating_node`, which checks the DAG has no // pending nodes left at all. +/// `Start` / `Stop` were lease-exempt *as kinds*, which was only safe because +/// every construction site fans them out from inside a lease-holding ancestor. +/// They declare the lease themselves now, and this pins that they do. +/// +/// Was `a_fanned_out_start_declares_the_lease_and_re_enters_its_reconciles_ +/// grant`, which submitted a `Reconcile`, claimed it, and then **re-declared +/// the fan-out inline** — *"same two calls the scheduler makes"*. That is a +/// copy of production in a test: had `exec.rs` stopped declaring the lease, it +/// would have kept passing. The declaration now lives in `templates:: +/// fanned_out_mechanical`, so this calls the real thing. +/// +/// The other half of the old test — that a descendant *re-enters* its +/// ancestor's grant rather than taking a second unit of a cap-1 lease — is +/// `hive_jobq`'s, and is tested there by +/// `child_borrows_ancestor_grant_released_when_subtree_done` and +/// `nested_borrowers_never_deadlock`. +#[test] +fn a_fanned_out_mechanical_node_declares_its_agent_lease() { + let q = JobQueue::new(4); + let id = submit( + &q, + DagSpec { + source: Source::Manual, + reason: "fan-out".to_owned(), + declare: Box::new(|b: &Job| { + templates::fanned_out_mechanical( + b, + NodeKind::Start { + agent: "agent-a".to_owned(), + }, + ); + }), + }, + ); + assert_eq!(declared_shape(&q, id), vec![row("start", None, &[])]); + assert_eq!( + declared_resources(&q, node_of(&q, id, "start")), + vec![Resource::Agent("agent-a".to_owned())], + "the fanned-out node carries the lease itself, rather than relying on \ + whoever happened to fan it out" + ); +} + // ---- cancel ---- #[test]