From 31a1853a45152e1114349603292fc2b7ce1daef8 Mon Sep 17 00:00:00 2001 From: atlas Date: Tue, 4 Aug 2026 12:51:45 +0200 Subject: [PATCH] fix(#3034): run prebuild beside the graceful-stop window, braced by AgentWindow MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The graceful path hung `Signal` under `Prebuild`, and a child only starts once its parent's own logic completes — so the drain window waited for the entire nix build before the agent was even asked to checkpoint. Up to the full GRACEFUL_STOP_TIMEOUT hidden behind the build, per agent, on every boot sweep. `Prebuild` needs the build slot and `Signal`/`Drain` need the agent lease, so there was never any contention to justify the nesting. Adds `NodeKind::AgentWindow`, a pure resource holder in the `DeployWindow` pattern. It declares the build slot and the agent lease atomically and holds both for its whole subtree; `Prebuild` and the quiesce chain hang off it as siblings and run concurrently. `StopForUpdate` is AfterOk *both*, so the container still goes down only once the build is ready and the agent has checkpointed — running the drain early is the win, stopping early would just be downtime. Two things this deliberately reverses, both documented in place: * The coordinated children now declare no resources. `templates.rs`'s module doc said each node must declare its own, precisely so one running under a holding ancestor could not get away with declaring nothing. That rule stands; the brace is named as its one exception, because declaring a resource means "I need this exclusively" and the lease is single-unit — two siblings that both declared it could never overlap, which is the whole point of the shape. * `rebuild_chain_declares_the_slot_where_the_nix_work_is` asserted the old principle in its name. Renamed to `..._declares_its_resources_on_the_brace` rather than left saying something the code no longer does. Hoisting the build slot is not new serialisation: a unit is held until the acquirer's subtree settles, and everything downstream already sat inside `Prebuild`, so the slot already spanned the entire rebuild. `graceful_rebuild_chain_drains_before_stopping` now asserts full rows instead of the kind list — the kind list is identical whether the chain runs beside the build or under it, so it could not see this bug. Verified by mutation: re-nesting `Signal` under `Prebuild` fails exactly that one test out of 317. --- hive-c0re/src/job_queue/exec.rs | 11 +- hive-c0re/src/job_queue/model.rs | 41 +++++ hive-c0re/src/job_queue/templates.rs | 161 +++++++++++-------- hive-c0re/src/job_queue/tests.rs | 222 ++++++++++++++++++--------- 4 files changed, 295 insertions(+), 140 deletions(-) diff --git a/hive-c0re/src/job_queue/exec.rs b/hive-c0re/src/job_queue/exec.rs index a7f30146..34929f28 100644 --- a/hive-c0re/src/job_queue/exec.rs +++ b/hive-c0re/src/job_queue/exec.rs @@ -126,13 +126,18 @@ pub(super) async fn run_node( Ok(()) } NodeKind::SetWanted { up, .. } => run_set_wanted(coord, agent, *up), - // The two nodes that carry no work of their own; completing either - // lets it reach `Finishing` so the nodes under it start. + // The nodes that carry no work of their own; completing one lets it + // reach `Finishing` so the nodes under it start. // - `Dag`: pure grouping container. The DAG's terminal side effect, if // any, is its own tail node in the graph. // - `DeployWindow`: pure resource holder — the meta window, agent lease // and build slot it declares stay held until its subtree settles. - NodeKind::Dag { .. } | NodeKind::DeployWindow { .. } => Ok(()), + // - `AgentWindow`: pure resource holder for one agent's rebuild — holds + // the lease so the build and the graceful-stop window can run as + // siblings rather than one nested under the other. + NodeKind::Dag { .. } | NodeKind::DeployWindow { .. } | NodeKind::AgentWindow { .. } => { + Ok(()) + } }; (builder, result) } diff --git a/hive-c0re/src/job_queue/model.rs b/hive-c0re/src/job_queue/model.rs index 769b86b6..0518829a 100644 --- a/hive-c0re/src/job_queue/model.rs +++ b/hive-c0re/src/job_queue/model.rs @@ -157,6 +157,37 @@ pub enum NodeKind { /// those phases does — the id is the node's own payload, not something a /// DAG-level catch-all hands down. DeployWindow { agent: String, approval_id: i64 }, + /// Group root of a rebuild subtree, and the node that **owns the agent + /// lease** for every phase below it. Performs no work of its own — same + /// pure-resource-holder shape as [`NodeKind::DeployWindow`], scoped to one + /// agent instead of a whole deploy. + /// + /// It exists so the lease is held *continuously* across the build, the + /// graceful-stop window and the swap. That is what lets `Prebuild` and the + /// `Signal` → `Drain` stop window run **concurrently**: they contend for + /// different resources (a build slot vs. the agent), and without a brace + /// the only way to order the stop after the build was to nest it under + /// `Prebuild` — which hid the whole graceful-stop timeout behind the nix + /// build, per agent, on every sweep. + /// + /// ⚠️ Its children deliberately **do not declare + /// [`Resource::Agent`](super::resource::Resource::Agent)**. Declaring a + /// resource means "I need this exclusively", and the lease is single-unit — + /// two siblings that both declared it could never run in parallel, which is + /// the entire point of the brace. Holding it on the parent and omitting it + /// on coordinated children is the opt-in "this subtree knows what it is + /// doing" shape. + /// + /// This costs nothing in observability: `running_transients` keys off the + /// node's **payload** agent, not off a declared lease edge, so every child + /// still lights its own pill and still reports its own + /// [`NodeKind::takes_container_down`] to the crash watcher. + /// + /// Sits *after* `MetaSync` rather than under it — a parent holds its + /// resources for its whole subtree, so nesting this inside `MetaSync` would + /// pin the **global** meta window across every agent's build and serialise + /// the sweep. + AgentWindow { agent: String }, /// Deploy phase 1 — **verify only, mutates nothing.** Drift-gate the /// approval's PR head, fetch it into the applied repo, and eval-verify the /// merge head. Any failure here aborts the deploy with the forge state @@ -343,6 +374,7 @@ impl NodeKind { NodeKind::WritePermFile { .. } => "write_perm_file", NodeKind::Reparent { .. } => "reparent", NodeKind::DeployWindow { .. } => "deploy_window", + NodeKind::AgentWindow { .. } => "agent_window", NodeKind::MergeVerify { .. } => "merge_verify", NodeKind::DeployApply { .. } => "deploy_apply", NodeKind::FinalizeDeploy { .. } => "finalize_deploy", @@ -376,6 +408,7 @@ impl NodeKind { | NodeKind::WriteDropin { agent } | NodeKind::WritePermFile { agent, .. } | NodeKind::DeployWindow { agent, .. } + | NodeKind::AgentWindow { agent } | NodeKind::MergeVerify { agent, .. } | NodeKind::DeployApply { agent, .. } | NodeKind::FinalizeDeploy { agent, .. } @@ -425,5 +458,13 @@ impl NodeKind { // - `Reconcile` is a planner; it fans out `Start` / `Stop`, which carry // their own answer. // - `DeployWindow` brackets a deploy without itself stopping anything. + // - `AgentWindow` likewise. It is the one that looks wrong: it *parents* + // `Signal` / `Drain` / `StopForUpdate` / `Swap`, which all answer + // `true`. But this is per-node, not per-subtree, and every one of + // those children is in `running_transients` on its own — so the + // suppression window is exactly the span where a child that really + // takes the container down is running, not the whole rebuild. Saying + // `true` here would widen it to cover the build and the tail, where a + // vanished container is still a real crash. } } diff --git a/hive-c0re/src/job_queue/templates.rs b/hive-c0re/src/job_queue/templates.rs index 89a9b206..f1311595 100644 --- a/hive-c0re/src/job_queue/templates.rs +++ b/hive-c0re/src/job_queue/templates.rs @@ -6,9 +6,24 @@ //! happened to run under an ancestor already holding the resource could get //! away with declaring nothing. //! +//! **The one sanctioned exception is a brace** — a pure-resource-holder root +//! ([`NodeKind::AgentWindow`], [`NodeKind::DeployWindow`]) that declares for a +//! subtree of nodes coordinated with each other, which then declare nothing. +//! This is the opposite of the failure above, not a relapse into it: there the +//! requirement was *implicit*, inferred from a kind and true only by accident of +//! placement; here it is declared explicitly on one node, and the omission below +//! it is deliberate and documented on the brace. +//! +//! It has to work this way, because declaring a resource means *"I need this +//! exclusively"* and the agent lease is single-unit: **two siblings that both +//! declared it could never run concurrently.** So for a subtree whose whole +//! point is concurrency, declaring the requirement truthfully on every node and +//! running those nodes in parallel are mutually exclusive. The brace is how the +//! shape says "this subtree is coordinated, one holder speaks for it". +//! //! ```text -//! rebuild(a): MetaSync(a) → Prebuild(a) → StopForUpdate(a) → Swap(a) →(ok) PostSwap(a) →(any) Reconcile(a) -//! rebuild(a) graceful: … Prebuild(a) → Signal(a) → Drain(a) → StopForUpdate(a) → … [boot sweep only] +//! rebuild(a): MetaSync(a) → AgentWindow(a){ Prebuild(a) → StopForUpdate(a) → Swap(a) →(ok) PostSwap(a) } →(any) Reconcile(a) +//! rebuild(a) graceful: … AgentWindow(a){ Prebuild(a) ∥ Signal(a) → Drain(a); both →(ok) StopForUpdate(a) → … } [boot sweep only] //! spawn(a): Provision(a) → Create(a) → WriteDropin(a) → Reconcile(a) [wanted=Up at approve] //! perm-change(a): WritePermFile(a) → «rebuild subgraph» //! meta-update(inp): MetaLock(inp) →«in-DAG rebuild subgraph per affected a» @@ -132,9 +147,10 @@ pub(crate) fn fanned_out_mechanical(builder: &JobBuilder, kind: NodeKind) { pub(crate) struct RebuildRoots<'a> { /// The meta-repo preamble. pub meta_sync: Handle<'a>, - /// The build root — its roll-up carries the whole - /// `StopForUpdate` → `Swap` → `PostSwap` subtree. - pub prebuild: Handle<'a>, + /// The brace holding the agent lease and the build slot — its roll-up + /// carries the whole mechanical subtree (`Prebuild`, the quiesce chain, + /// `StopForUpdate` → `Swap` → `PostSwap`). + pub agent_window: Handle<'a>, /// The recovery/convergence tail root. pub reconcile: Handle<'a>, } @@ -142,40 +158,50 @@ pub(crate) struct RebuildRoots<'a> { impl<'a> RebuildRoots<'a> { /// The three roots as a slice, for edging a tail onto all of them. fn all(self) -> [Handle<'a>; 3] { - [self.meta_sync, self.prebuild, self.reconcile] + [self.meta_sync, self.agent_window, self.reconcile] } } -/// The rebuild node subtree (nested, three group roots). `after`, when given, is -/// the node this subgraph chains behind. Structure: +/// The rebuild node subtree (three group roots). `after`, when given, is the +/// node this subgraph chains behind. Structure: /// - `MetaSync` (**root**): the meta-repo preamble (dir prep, agent sync, /// optional relock). Owns the global `MetaWindow` — and *only* for its own -/// short duration, which is why it is a sibling root rather than `Prebuild`'s +/// short duration, which is why it is a sibling root rather than the brace's /// parent: a resource is held across the holder's whole subtree, so parenting -/// the build under it would extend a hive-global window over every rebuild's -/// nix build. -/// - `Prebuild` (**root**): `AfterOk` `MetaSync`. Owns the build slot for the -/// whole mechanical subtree below it. Lease-exempt — the nix build overlaps -/// other DAGs on the same agent. -/// - the **stop root** (child of `Prebuild`): owns the agent lease and runs once -/// `Prebuild` reaches `Finishing` (parent gate). Non-graceful that is -/// `StopForUpdate` itself; graceful it is `Signal`, with `Drain` and then -/// `StopForUpdate` as its children so the lease stays continuous across the -/// whole stop — siblings would each take the lease separately and leave a gap -/// another DAG could claim the agent in, mid-bounce. -/// - `Swap` (child of `StopForUpdate`): borrows the agent lease from its -/// ancestors and the build slot from `Prebuild` — both continuous. -/// - `PostSwap` (child of `StopForUpdate`): the swap's Ok-only bookkeeping tail -/// (rev marker, forge/matrix sync, kick, rescan), `AfterOk` its sibling -/// `Swap`. -/// - `Reconcile` (**last, root**): `AfterAny` `Prebuild`, which rolls up -/// terminal only once its whole mechanical subtree (SFU→Swap→PostSwap) has -/// settled — so `Reconcile` runs after the swap regardless of outcome, and as -/// a top-level root it survives the cancel-cascade of a failed `Prebuild` -/// (recovery-start invariant, which also covers a failed `MetaSync`: that -/// cancel-cascades `Prebuild`, i.e. terminal, so the tail still runs). It -/// takes a fresh lease; the tiny gap is harmless — `Reconcile` converges to -/// the persisted `wanted` idempotently. +/// the rebuild under it would extend a hive-global window over every +/// rebuild's nix build. +/// - `AgentWindow` (**root**): `AfterOk` `MetaSync`. The brace — declares the +/// build slot *and* the agent lease, atomically, and holds both for the whole +/// subtree. Everything below it declares **nothing** and re-enters these +/// grants. +/// - `Prebuild` and the quiesce chain are **siblings under the brace, and run +/// concurrently.** That is the point of the brace: they contend for different +/// resources (slot vs. agent), so nesting the stop under the build — as this +/// template did — hid the entire graceful-stop timeout behind the nix build, +/// per agent, on every sweep. +/// - the **quiesce chain** (graceful only): `Signal` then `Drain` as its child. +/// Asks the agent to checkpoint and waits for it to go quiet; the container +/// is still *up* throughout. +/// - `StopForUpdate` (child of the brace): `AfterOk` **both** `Prebuild` and +/// `Drain`. Waiting on the build is deliberate — running the drain window +/// early is the win, taking the container *down* early would be pure +/// downtime. This is the node that actually stops the container. +/// - `Swap` (child of `StopForUpdate`), then `PostSwap` (`AfterOk` its sibling +/// `Swap`): the Ok-only bookkeeping tail (rev marker, forge/matrix sync, +/// kick, rescan). +/// - `Reconcile` (**last, root**): `AfterAny` `AgentWindow`, which rolls up +/// terminal only once its whole subtree has settled — so `Reconcile` runs +/// after the swap regardless of outcome, and as a top-level root it survives +/// the cancel-cascade of a failed brace (recovery-start invariant, which also +/// covers a failed `MetaSync`: that cancel-cascades the brace, i.e. terminal, +/// so the tail still runs). It takes a fresh lease; the tiny gap is harmless — +/// `Reconcile` converges to the persisted `wanted` idempotently. +/// +/// The lease-continuity argument that used to justify nesting the stop chain +/// (*"siblings would each take the lease separately and leave a gap another DAG +/// could claim the agent in, mid-bounce"*) is **satisfied by the brace instead** +/// — one holder above them all, so siblings share one continuous grant and +/// there is no gap to claim. That is what makes flattening them safe. fn rebuild_subtree<'a>( builder: &'a JobBuilder, agent: &str, @@ -191,59 +217,62 @@ fn rebuild_subtree<'a>( if let Some(after) = after { meta_sync = meta_sync.after_ok(after); } - let prebuild = builder - .node(NodeKind::Prebuild { agent: a() }) + // The brace. Both resources are declared here, on one node, on purpose — + // the queue acquires a node's resources atomically, so a single + // multi-resource root can never hold one and block on another. Hoisting the + // slot up costs nothing: a resource is held for the acquirer's whole + // subtree, and the build slot already spanned the entire rebuild when + // `Prebuild` was the one holding it. + let agent_window = builder + .node(NodeKind::AgentWindow { agent: a() }) .needs(Resource::BuildSlot) + .needs(Resource::Agent(a())) .after_ok(meta_sync); - // The stop root hangs off `Prebuild` and owns the agent lease for - // everything below it. `StopForUpdate` parents the swap pair either way. - let stop_for_update = if graceful { + // Siblings under the brace: the build and the quiesce chain run + // concurrently, borrowing the brace's grants rather than declaring their + // own. Declaring the lease on both would make them mutually exclusive — + // it is single-unit — which is exactly what this shape exists to avoid. + let prebuild = builder + .node(NodeKind::Prebuild { agent: a() }) + .part_of(agent_window); + + let drain = graceful.then(|| { let signal = builder .node(NodeKind::Signal { agent: a() }) - .needs(Resource::Agent(a())) - .part_of(prebuild); + .part_of(agent_window); // `Drain` is a *child* of `Signal`, so the parent gate already orders // it — a child must not dep on its own parent (dep-scope). - let drain = builder - .node(NodeKind::Drain { agent: a() }) - .needs(Resource::Agent(a())) - .part_of(signal); - builder - .node(NodeKind::StopForUpdate { agent: a() }) - .needs(Resource::Agent(a())) - .part_of(signal) - .after_ok(drain) - } else { - builder - .node(NodeKind::StopForUpdate { agent: a() }) - .needs(Resource::Agent(a())) - .part_of(prebuild) - }; + builder.node(NodeKind::Drain { agent: a() }).part_of(signal) + }); + + // The container goes down here, not earlier: `AfterOk` the build so a + // failed build never stops a healthy container, and `AfterOk` the drain so + // the agent has checkpointed. + let mut stop_for_update = builder + .node(NodeKind::StopForUpdate { agent: a() }) + .part_of(agent_window) + .after_ok(prebuild); + if let Some(drain) = drain { + stop_for_update = stop_for_update.after_ok(drain); + } let swap = builder .node(NodeKind::Swap { agent: a() }) - .needs(Resource::BuildSlot) - .needs(Resource::Agent(a())) .part_of(stop_for_update); - // `PostSwap` declares the lease it actually runs under. It is a child of - // `StopForUpdate`, which holds it, so this is a re-entrant borrow — no - // second unit, no deadlock. Declaring it is what stops the requirement - // being true only of this one DAG shape. let _post_swap = builder .node(NodeKind::PostSwap { agent: a() }) - .needs(Resource::Agent(a())) .part_of(stop_for_update) .after_ok(swap); let reconcile = builder .node(NodeKind::Reconcile { agent: a() }) .needs(Resource::Agent(a())) - .after_any(prebuild); + .after_any(agent_window); RebuildRoots { meta_sync, - prebuild, + agent_window, reconcile, } } @@ -309,7 +338,7 @@ pub(crate) fn deploy_rebuild_nodes(builder: &JobBuilder, agent: &str, approval_i approval_id, }) .needs(Resource::MetaWindow) - .after_ok(roots.prebuild) + .after_ok(roots.agent_window) .after_ok(roots.reconcile); } @@ -444,7 +473,7 @@ pub fn perm_change(builder: &JobBuilder, agent: &str, payload: PermPayload) { emit_rebuilt_tails( builder, agent, - &[write, roots.meta_sync, roots.prebuild, roots.reconcile], + &[write, roots.meta_sync, roots.agent_window, roots.reconcile], ); } diff --git a/hive-c0re/src/job_queue/tests.rs b/hive-c0re/src/job_queue/tests.rs index 10759af5..4a52ef26 100644 --- a/hive-c0re/src/job_queue/tests.rs +++ b/hive-c0re/src/job_queue/tests.rs @@ -390,23 +390,40 @@ fn rebuild_chain_is_declared_serial() { // observe an order that is fully declared the moment `submit` returns. // // ⚠️ The old name was also wrong about the mechanism, and reading it rather - // than the graph is how you'd stay wrong: **only half this chain is dep - // edges.** `stop_for_update` and `swap` declare no deps at all — they are - // ordered by *parent nesting* ("a node's sub-nodes run after its own - // logic"). Both axes are asserted below because a template can break either - // one independently. + // than the graph is how you'd stay wrong: **only part of this chain is dep + // edges.** `swap` declares no deps at all — it is ordered by *parent + // nesting* ("a node's sub-nodes run after its own logic"). Both axes are + // asserted below because a template can break either one independently. + // + // ⚠️ "Serial" is now about the *declared* order, not about concurrency: + // `prebuild` and the graceful quiesce chain are siblings under the brace and + // run **in parallel** (see `graceful_rebuild_chain_drains_before_stopping`). + // The non-graceful shape asserted here has no quiesce chain, so nothing here + // is concurrent — but the name would mislead about the graceful one. let q = JobQueue::new(1); let id = submit(&q, "r", |builder| rebuild(builder, "agent-a")); assert_eq!( declared_shape(&q, id), vec![ row("meta_sync", None, &[]), - row("prebuild", None, &[("meta_sync", "done")]), - // No dep: ordered by hanging under `prebuild`. - row("stop_for_update", Some("prebuild"), &[]), + // The brace: holds the lease + slot for everything nested below it. + row("agent_window", None, &[("meta_sync", "done")]), + // No dep: ordered by hanging under the brace. + row("prebuild", Some("agent_window"), &[]), + // A real dep, not nesting: the container must not go down until the + // build that replaces it has succeeded. + row( + "stop_for_update", + Some("agent_window"), + &[("prebuild", "done")] + ), row("swap", Some("stop_for_update"), &[]), row("post_swap", Some("stop_for_update"), &[("swap", "done")]), - row("reconcile", None, &[("prebuild", "done|failed|skipped")]), + row( + "reconcile", + None, + &[("agent_window", "done|failed|skipped")] + ), // The tail pair. The ok tail needs every root to succeed; the !ok // tail hangs off the ok tail's *elimination* (`skipped`), which is // what makes exactly one of them run. @@ -415,7 +432,7 @@ fn rebuild_chain_is_declared_serial() { None, &[ ("meta_sync", "done"), - ("prebuild", "done"), + ("agent_window", "done"), ("reconcile", "done"), ], ), @@ -425,7 +442,7 @@ fn rebuild_chain_is_declared_serial() { &[ ("emit_rebuilt", "skipped"), ("meta_sync", "done|failed|skipped"), - ("prebuild", "done|failed|skipped"), + ("agent_window", "done|failed|skipped"), ("reconcile", "done|failed|skipped"), ], ), @@ -433,12 +450,17 @@ fn rebuild_chain_is_declared_serial() { ); } -/// The boot sweep's graceful shape: the agent gets `Signal` → `Drain` to -/// finish its turn before `StopForUpdate` takes the container down. `Signal` -/// *parents* the rest of the stop rather than sitting beside it, so the agent -/// lease is held continuously across the whole bounce — as siblings, each of -/// `Signal` / `Drain` / `StopForUpdate` would acquire the lease separately and -/// leave a window for another DAG to claim the agent mid-stop. +/// The boot sweep's graceful shape: the agent gets `Signal` → `Drain` to finish +/// its turn before `StopForUpdate` takes the container down — **concurrently +/// with the nix build**, which is the whole reason the brace exists. The drain +/// window costs nothing it doesn't already cost; nesting it under `Prebuild` +/// used to hide the entire `GRACEFUL_STOP_TIMEOUT` behind the build. +/// +/// Lease continuity across the bounce is preserved by `AgentWindow` holding the +/// lease above all of them, which is what makes them safe as siblings — the +/// older shape had to nest `Signal` over the rest of the stop for exactly that +/// reason, since otherwise each would acquire the lease separately and leave a +/// window for another DAG to claim the agent mid-stop. #[test] fn graceful_rebuild_chain_drains_before_stopping() { let q = JobQueue::new(1); @@ -451,30 +473,47 @@ fn graceful_rebuild_chain_drains_before_stopping() { }, ) .expect("valid shape"); + // Asserted as full rows, not just kinds: the kind list is identical whether + // the quiesce chain runs beside the build or nested under it, so a + // kind-only assertion cannot see the bug this shape exists to fix. assert_eq!( - declared_shape(&q, id) - .iter() - .map(|d| d.kind) - .collect::>(), + declared_shape(&q, id), vec![ - "meta_sync", - "prebuild", - // The graceful window goes between the build and the stop: the - // agent gets its turn to finish before the container goes down. - "signal", - "drain", - "stop_for_update", - "swap", - "post_swap", - "reconcile", + row("meta_sync", None, &[]), + row("agent_window", None, &[("meta_sync", "done")]), + row("prebuild", Some("agent_window"), &[]), + // 🎯 **The fix, and the thing to guard.** `signal` hangs off the + // *brace*, not off `prebuild`, and declares no dep on it — so the + // drain window runs concurrently with the nix build instead of + // behind it. Both halves matter: re-parenting it under `prebuild` + // OR adding an `AfterOk(prebuild)` edge would each silently restore + // the original defect (up to GRACEFUL_STOP_TIMEOUT hidden behind + // every agent's build, on every boot sweep). + row("signal", Some("agent_window"), &[]), + row("drain", Some("signal"), &[]), + // The container still goes down only when *both* are ready: the + // build succeeded and the agent has checkpointed. Running the drain + // early is the win; stopping early would just be downtime. + row( + "stop_for_update", + Some("agent_window"), + &[("prebuild", "done"), ("drain", "done")] + ), + row("swap", Some("stop_for_update"), &[]), + row("post_swap", Some("stop_for_update"), &[("swap", "done")]), + row( + "reconcile", + None, + &[("agent_window", "done|failed|skipped")] + ), ], - "graceful inserts signal + drain ahead of the stop, and nothing else" + "graceful runs signal + drain beside the build, and stops only after both" ); } /// The non-graceful shape is the default everywhere except the boot sweep: /// a manual rebuild, a meta-update cascade child and a deploy must NOT spend a -/// drain window, so `StopForUpdate` still hangs straight off `Prebuild`. +/// drain window, so `StopForUpdate` waits only on `Prebuild`. #[test] fn non_graceful_rebuild_has_no_signal_or_drain() { // Read the shape off the queue rather than out of a node list: a declared @@ -491,32 +530,38 @@ fn non_graceful_rebuild_has_no_signal_or_drain() { .collect::>(), vec![ "meta_sync", + "agent_window", "prebuild", "stop_for_update", "swap", "post_swap", "reconcile" ], - "exactly six nodes, and neither of them is signal or drain" + "exactly seven nodes, and none of them is signal or drain" ); } // ---- build slots ---- #[test] -fn rebuild_chain_declares_the_slot_where_the_nix_work_is() { - // Was `fifo_fairness_for_the_slot`, which submitted three rebuilds and - // drove one to completion to watch the freed slot go to the earlier - // waiter. **That fairness guarantee is hive_jobq's**, and it had no test - // there at all — its claim primitive scans nodes in insertion order and - // takes the first satisfiable one, and nothing pinned that. It does now: - // `a_contended_resource_goes_to_the_oldest_waiter`. +fn rebuild_chain_declares_its_resources_on_the_brace() { + // Was `rebuild_chain_declares_the_slot_where_the_nix_work_is` (and before + // that `fifo_fairness_for_the_slot`). // - // What is c0re's is *which* nodes contend for the slot in the first place, - // and that is a declaration. "Uniform hold across the chain" then follows - // from the parent nesting asserted in `rebuild_chain_is_declared_serial`: - // a resource unit is held for the acquirer's whole subtree, so the slot - // `Prebuild` takes covers `StopForUpdate` → `Swap` → `PostSwap` beneath it. + // ⚠️ **This rename is a reversal, not tidying.** The old assertion read + // "the slot follows the nix work and the lease follows the container": each + // node declared the resource it personally needed, deliberately, so that the + // requirement belonged to the node rather than to one DAG shape. The brace + // inverts that for a coordinated subtree, and the reason is forced: + // declaring a resource means *"I need this exclusively"* and the lease is + // single-unit, so `Prebuild` and the quiesce chain — which exist to run + // **concurrently** — could never overlap if both declared it. One holder + // above them speaks for the subtree. `templates.rs`'s module doc carries the + // general rule and states this as its one sanctioned exception. + // + // (`hive_jobq` owns slot *fairness*, pinned there by + // `a_contended_resource_goes_to_the_oldest_waiter`. What is c0re's is + // *which* nodes contend in the first place — a declaration, asserted here.) let q = JobQueue::new(1); let id = submit(&q, "r", |builder| rebuild(builder, "agent-a")); let res = |kind: &str| declared_resources(&q, node_of(&q, id, kind)); @@ -525,6 +570,7 @@ fn rebuild_chain_declares_the_slot_where_the_nix_work_is() { assert_eq!( [ res("meta_sync"), + res("agent_window"), res("prebuild"), res("stop_for_update"), res("swap"), @@ -532,21 +578,28 @@ fn rebuild_chain_declares_the_slot_where_the_nix_work_is() { ], [ // The meta preamble takes the global window and *nothing else* — - // no slot (it does no nix work) and no lease. + // no slot (it does no nix work) and no lease. It stays a sibling + // root so that hive-global window is not held across any build. vec![Resource::MetaWindow], - // The nix build is the slot-needer, and takes **no lease**. That is - // what lets a prebuild overlap another DAG on the same agent: the - // container is still up and untouched while it builds. - vec![Resource::BuildSlot], - // The lease starts here — the first node that touches the - // container — and not one node earlier. - vec![agent()], - // Swap needs both. It re-enters the slot its `Prebuild` ancestor - // holds rather than acquiring a second unit. + // The brace takes both, atomically, and holds them for its whole + // subtree. Hoisting the slot here is not a widening: it already + // spanned the entire rebuild when `Prebuild` held it, because a unit + // is held until the acquirer's subtree settles and everything below + // was inside `Prebuild`. vec![Resource::BuildSlot, agent()], + // The coordinated children declare nothing and re-enter the brace's + // grants. An empty vec here is the *point* of the shape, not an + // omission — if any of these regains a declaration it will silently + // stop running in parallel with its siblings. + vec![], + vec![], + vec![], + // `Reconcile` is the exception that stays: a top-level root outside + // the brace, so it takes a genuinely fresh lease after the window + // has released. vec![agent()], ], - "the slot follows the nix work and the lease follows the container" + "the brace declares for the subtree; coordinated children declare nothing" ); } @@ -652,11 +705,20 @@ fn multi_agent_start_one_dag_folds_per_agent_stale_rebuild() { vec![ row("set_wanted", None, &[]), row("meta_sync", None, &[("set_wanted", "done")]), - row("prebuild", None, &[("meta_sync", "done")]), - row("stop_for_update", Some("prebuild"), &[]), + row("agent_window", None, &[("meta_sync", "done")]), + row("prebuild", Some("agent_window"), &[]), + row( + "stop_for_update", + Some("agent_window"), + &[("prebuild", "done")] + ), row("swap", Some("stop_for_update"), &[]), row("post_swap", Some("stop_for_update"), &[("swap", "done")]), - row("reconcile", None, &[("prebuild", "done|failed|skipped")]), + row( + "reconcile", + None, + &[("agent_window", "done|failed|skipped")] + ), ], "a stale agent gets the whole rebuild chain wedged between intent and \ convergence — same DAG, same head kind, more in the middle" @@ -818,15 +880,23 @@ fn rebuild_reconcile_waits_for_the_whole_build_subtree() { // // The interesting claim was "Reconcile must wait for PostSwap, not race // it" — and it does *not* come from an edge between them. `reconcile` deps - // `AfterAny(prebuild)`, while `post_swap` sits inside prebuild's subtree - // (post_swap → stop_for_update → prebuild). A parent is not terminal until - // its subtree is, so prebuild cannot satisfy that edge while post_swap is - // outstanding. **The ordering is the parent chain, not a dependency.** + // `AfterAny(agent_window)`, while `post_swap` sits inside the brace's + // subtree (post_swap → stop_for_update → agent_window). A parent is not + // terminal until its subtree is, so the brace cannot satisfy that edge while + // post_swap is outstanding. **The ordering is the parent chain, not a + // dependency.** // // Both facts are asserted in `rebuild_chain_is_declared_serial`; this test // states the derived property explicitly because the indirection is the // easy thing to break — someone flattening the chain would keep every edge // and still lose the guarantee. + // + // 🎯 That warning earned itself: introducing `AgentWindow` **did** flatten + // this chain (`stop_for_update` moved off `prebuild` and onto the brace), + // and the guarantee survives only because the roll-up point moved with it. + // The same roll-up rule is also what makes the obvious-looking alternative + // — `stop_for_update` `AfterOk` `prebuild` *with* `swap` still nested under + // `prebuild` — a **cycle**: the two would wait on each other. let q = JobQueue::new(1); let id = submit(&q, "r", |builder| rebuild(builder, "agent-a")); let shape = declared_shape(&q, id); @@ -838,15 +908,15 @@ fn rebuild_reconcile_waits_for_the_whole_build_subtree() { .parent }; assert_eq!(parent_of("post_swap"), Some("stop_for_update")); - assert_eq!(parent_of("stop_for_update"), Some("prebuild")); + assert_eq!(parent_of("stop_for_update"), Some("agent_window")); assert_eq!( shape .iter() .find(|d| d.kind == "reconcile") .expect("reconcile node") .after, - vec![("prebuild", "done|failed|skipped".to_owned())], - "reconcile gates on prebuild's roll-up, which covers the whole build \ + vec![("agent_window", "done|failed|skipped".to_owned())], + "reconcile gates on the brace's roll-up, which covers the whole \ subtree — including post_swap — and runs on failure too" ); } @@ -1222,11 +1292,20 @@ fn deploy_apply_grows_rebuild_subgraph_and_finalizes_after_it() { declared_shape(&q, id), vec![ row("meta_sync", None, &[]), - row("prebuild", None, &[("meta_sync", "done")]), - row("stop_for_update", Some("prebuild"), &[]), + row("agent_window", None, &[("meta_sync", "done")]), + row("prebuild", Some("agent_window"), &[]), + row( + "stop_for_update", + Some("agent_window"), + &[("prebuild", "done")] + ), row("swap", Some("stop_for_update"), &[]), row("post_swap", Some("stop_for_update"), &[("swap", "done")]), - row("reconcile", None, &[("prebuild", "done|failed|skipped")]), + row( + "reconcile", + None, + &[("agent_window", "done|failed|skipped")] + ), // The deploy tag is planted only after the rebuild came up clean: // `AfterOk` on **both** roots, so either one failing skips it. That // pair of edges is the whole "skips finalize on a failed graft" @@ -1234,7 +1313,7 @@ fn deploy_apply_grows_rebuild_subgraph_and_finalizes_after_it() { row( "finalize_deploy", None, - &[("prebuild", "done"), ("reconcile", "done")] + &[("agent_window", "done"), ("reconcile", "done")] ), ] ); @@ -1456,6 +1535,7 @@ fn perm_change_shape_prefixes_rebuild_chain() { vec![ "write_perm_file", "meta_sync", + "agent_window", "prebuild", "stop_for_update", "swap",