From 31a1853a45152e1114349603292fc2b7ce1daef8 Mon Sep 17 00:00:00 2001 From: atlas Date: Tue, 4 Aug 2026 12:51:45 +0200 Subject: [PATCH 1/6] 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", From a0790e4e491990373222e6efeb5dd7b7393c973e Mon Sep 17 00:00:00 2001 From: atlas Date: Tue, 4 Aug 2026 12:55:52 +0200 Subject: [PATCH 2/6] refactor(#3034): rename PostSwap to RebuildBookkeeping It reads as a swap thing but is post-*rebuild* bookkeeping: rev marker, forge and matrix sync, manager kick, container rescan, meta-inputs snapshot. Wire label follows (`post_swap` -> `rebuild_bookkeeping`); the graph view renders whatever label it is sent, so nothing keys on the old string. Also corrects a doc the previous commit falsified: this node's comment still said it declares the agent lease as a re-entrant borrow, after that declaration moved to the brace. Clippy and the whole suite pass over a stale doc comment, so it took reading the file to find. Verified by grepping the new name for places it has no business being, which caught the sed rewriting a *historical* test name in a `// Replaces ...` comment - reverted, since prose about the past must keep its old spelling. --- hive-c0re/src/actions.rs | 2 +- hive-c0re/src/job_queue/exec.rs | 10 +++--- hive-c0re/src/job_queue/model.rs | 22 +++++++------ hive-c0re/src/job_queue/templates.rs | 14 ++++----- hive-c0re/src/job_queue/tests.rs | 46 +++++++++++++++++++--------- 5 files changed, 56 insertions(+), 38 deletions(-) diff --git a/hive-c0re/src/actions.rs b/hive-c0re/src/actions.rs index 83f5bd07..7333f387 100644 --- a/hive-c0re/src/actions.rs +++ b/hive-c0re/src/actions.rs @@ -867,7 +867,7 @@ async fn prepare_applied_target( /// [`run_deploy_tail`] must not roll `main` back. Ordering that ahead of the tag /// plant is what makes the tail's `deployed/` cross-check a second line of /// defence rather than the only one. No agent kick — the rebuild's own -/// `PostSwap` already did it. +/// `RebuildBookkeeping` already did it. /// /// # Errors /// diff --git a/hive-c0re/src/job_queue/exec.rs b/hive-c0re/src/job_queue/exec.rs index 34929f28..6a0bb513 100644 --- a/hive-c0re/src/job_queue/exec.rs +++ b/hive-c0re/src/job_queue/exec.rs @@ -63,7 +63,7 @@ pub(super) async fn run_node( NodeKind::MetaSync { relock, .. } => run_meta_sync(coord, agent, *relock).await, NodeKind::Prebuild { .. } => run_prebuild(agent, id).await, NodeKind::Swap { .. } => run_swap(coord, agent, id).await, - NodeKind::PostSwap { .. } => run_post_swap(coord, agent).await, + NodeKind::RebuildBookkeeping { .. } => run_rebuild_bookkeeping(coord, agent).await, NodeKind::Provision { .. } => run_provision(coord, agent).await, NodeKind::Create { .. } => run_create(agent).await, NodeKind::MetaLock { @@ -263,9 +263,9 @@ async fn run_swap(coord: &Arc, name: &str, id: NodeId) -> Result<() let paths = Coordinator::agent_paths(name, agent_dir); let result = crate::lifecycle::swap_update(name, &hive, &paths, Some(id.get())).await; // On success the Ok-only bookkeeping tail (rev marker, forge/matrix - // sync, kick, rescan, snapshot) runs in the sibling `PostSwap` node, - // which deps `AfterOk(Swap)`. On failure `PostSwap` is cancel-cascaded - // and the tail `Reconcile` (`AfterAny(PostSwap)`) handles recovery; here + // sync, kick, rescan, snapshot) runs in the sibling `RebuildBookkeeping` node, + // which deps `AfterOk(Swap)`. On failure `RebuildBookkeeping` is cancel-cascaded + // and the tail `Reconcile` (`AfterAny(RebuildBookkeeping)`) handles recovery; here // we only refresh the observed state so dashboards reflect the failed // swap immediately. The `Rebuilt { ok: false }` manager event is emitted by // the DAG's `EmitRebuilt` tail (any node may be the one that failed). @@ -280,7 +280,7 @@ async fn run_swap(coord: &Arc, name: &str, id: NodeId) -> Result<() /// means the profile swap succeeded. Store/forge/matrix work only — no nix /// build (build-slot-exempt); the agent lease taken at `Swap` is still held /// (the whole chain up to `Reconcile` is one agent's subgraph). -async fn run_post_swap(coord: &Arc, name: &str) -> Result<()> { +async fn run_rebuild_bookkeeping(coord: &Arc, name: &str) -> Result<()> { if let Some(rev) = crate::auto_update::current_flake_rev(&coord.hyperhive_flake) && let Err(e) = std::fs::write(crate::paths::applied_rev_marker(name), rev) { diff --git a/hive-c0re/src/job_queue/model.rs b/hive-c0re/src/job_queue/model.rs index 0518829a..03f86c4b 100644 --- a/hive-c0re/src/job_queue/model.rs +++ b/hive-c0re/src/job_queue/model.rs @@ -53,20 +53,22 @@ pub enum NodeKind { /// `nixos-container update` profile-swap (requires the container /// stopped). Re-applies nspawn flags + resource limits first — /// rebuild is the reconcile verb. The post-rebuild bookkeeping tail - /// lives in the sibling `PostSwap` node. + /// lives in the sibling `RebuildBookkeeping` node. Swap { agent: String }, /// The post-`Swap` bookkeeping tail as a first-class node: rev marker, /// forge + matrix sync, manager kick, container rescan, meta-inputs /// snapshot. Split out of `Swap` for dashboard visibility + retry /// granularity. Deps `AfterOk(Swap)`, so it runs only when the profile - /// swap succeeded; the tail `Reconcile` deps `AfterAny(PostSwap)`, so on + /// swap succeeded; the tail `Reconcile` deps `AfterAny(RebuildBookkeeping)`, so on /// swap failure this node is cancel-cascaded (a terminal state) and - /// recovery still runs. Store/forge/matrix work only — no nix build, so - /// build-slot-exempt. It *does* declare the agent lease: an ancestor in the - /// stop chain already holds it, so this is a re-entrant borrow rather than a - /// second unit — declaring it keeps the requirement true of this node rather - /// than of the one DAG shape it happens to be used in. - PostSwap { agent: String }, + /// recovery still runs. Store/forge/matrix work only — no nix build. + /// + /// Declares **no resources**: it is a coordinated child of + /// [`NodeKind::AgentWindow`], which holds the agent lease (and the build + /// slot) for the whole rebuild subtree. See `templates.rs`'s module doc for + /// why the brace is the one place a resource is declared on behalf of + /// others. + RebuildBookkeeping { agent: String }, /// First-spawn pre-create provisioning: proposed/applied repos, /// state subvolume, and meta registration (`sync_agents`). Runs /// ahead of `Create` so the `nixos-container create --flake @@ -360,7 +362,7 @@ impl NodeKind { NodeKind::MetaSync { .. } => "meta_sync", NodeKind::Prebuild { .. } => "prebuild", NodeKind::Swap { .. } => "swap", - NodeKind::PostSwap { .. } => "post_swap", + NodeKind::RebuildBookkeeping { .. } => "rebuild_bookkeeping", NodeKind::Provision { .. } => "provision", NodeKind::Create { .. } => "create", NodeKind::MetaLock { .. } => "meta_lock", @@ -396,7 +398,7 @@ impl NodeKind { NodeKind::MetaSync { agent, .. } | NodeKind::Prebuild { agent } | NodeKind::Swap { agent } - | NodeKind::PostSwap { agent } + | NodeKind::RebuildBookkeeping { agent } | NodeKind::Provision { agent } | NodeKind::Create { agent } | NodeKind::Reconcile { agent } diff --git a/hive-c0re/src/job_queue/templates.rs b/hive-c0re/src/job_queue/templates.rs index f1311595..f9353d15 100644 --- a/hive-c0re/src/job_queue/templates.rs +++ b/hive-c0re/src/job_queue/templates.rs @@ -22,7 +22,7 @@ //! shape says "this subtree is coordinated, one holder speaks for it". //! //! ```text -//! rebuild(a): MetaSync(a) → AgentWindow(a){ Prebuild(a) → StopForUpdate(a) → Swap(a) →(ok) PostSwap(a) } →(any) Reconcile(a) +//! rebuild(a): MetaSync(a) → AgentWindow(a){ Prebuild(a) → StopForUpdate(a) → Swap(a) →(ok) RebuildBookkeeping(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» @@ -149,7 +149,7 @@ pub(crate) struct RebuildRoots<'a> { pub meta_sync: 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`). + /// `StopForUpdate` → `Swap` → `RebuildBookkeeping`). pub agent_window: Handle<'a>, /// The recovery/convergence tail root. pub reconcile: Handle<'a>, @@ -186,7 +186,7 @@ impl<'a> RebuildRoots<'a> { /// `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` (child of `StopForUpdate`), then `RebuildBookkeeping` (`AfterOk` its sibling /// `Swap`): the Ok-only bookkeeping tail (rev marker, forge/matrix sync, /// kick, rescan). /// - `Reconcile` (**last, root**): `AfterAny` `AgentWindow`, which rolls up @@ -260,8 +260,8 @@ fn rebuild_subtree<'a>( let swap = builder .node(NodeKind::Swap { agent: a() }) .part_of(stop_for_update); - let _post_swap = builder - .node(NodeKind::PostSwap { agent: a() }) + let _rebuild_bookkeeping = builder + .node(NodeKind::RebuildBookkeeping { agent: a() }) .part_of(stop_for_update) .after_ok(swap); @@ -317,7 +317,7 @@ pub(crate) fn graceful_rebuild_nodes<'a>( /// `FinalizeDeploy` waits on **two** roots, which together reproduce the gate /// the old fused node had around its inline `rebuild_no_meta` call: /// - `AfterOk` `Prebuild` — a parent's state is its roll-up, so this is `Done` -/// only once `StopForUpdate` → `Swap` → `PostSwap` all are (a failed *or* +/// only once `StopForUpdate` → `Swap` → `RebuildBookkeeping` all are (a failed *or* /// cancelled child rolls the parent up `Failed`). That's the old /// `build_result`. /// - `AfterOk` `Reconcile` — the old call passed `deferred_start = false` on @@ -350,7 +350,7 @@ pub(crate) fn deploy_rebuild_nodes(builder: &JobBuilder, agent: &str, approval_i /// /// Closed by an [`NodeKind::EmitRebuilt`] tail edged onto all three group-roots /// (`MetaSync`, `Prebuild`, `Reconcile`) — `Prebuild`'s roll-up carries the -/// whole `StopForUpdate`→`Swap`→`PostSwap` subtree, so those three cover every +/// whole `StopForUpdate`→`Swap`→`RebuildBookkeeping` subtree, so those three cover every /// node. Edging `Reconcile` alone would not do: it is `AfterAny` `Prebuild`, so /// it reaches `Done` even after a failed swap and the tail would report success. pub fn rebuild(builder: &JobBuilder, agent: &str, relock: bool) { diff --git a/hive-c0re/src/job_queue/tests.rs b/hive-c0re/src/job_queue/tests.rs index 4a52ef26..4b6bdd63 100644 --- a/hive-c0re/src/job_queue/tests.rs +++ b/hive-c0re/src/job_queue/tests.rs @@ -418,7 +418,11 @@ fn rebuild_chain_is_declared_serial() { &[("prebuild", "done")] ), row("swap", Some("stop_for_update"), &[]), - row("post_swap", Some("stop_for_update"), &[("swap", "done")]), + row( + "rebuild_bookkeeping", + Some("stop_for_update"), + &[("swap", "done")] + ), row( "reconcile", None, @@ -500,7 +504,11 @@ fn graceful_rebuild_chain_drains_before_stopping() { &[("prebuild", "done"), ("drain", "done")] ), row("swap", Some("stop_for_update"), &[]), - row("post_swap", Some("stop_for_update"), &[("swap", "done")]), + row( + "rebuild_bookkeeping", + Some("stop_for_update"), + &[("swap", "done")] + ), row( "reconcile", None, @@ -534,7 +542,7 @@ fn non_graceful_rebuild_has_no_signal_or_drain() { "prebuild", "stop_for_update", "swap", - "post_swap", + "rebuild_bookkeeping", "reconcile" ], "exactly seven nodes, and none of them is signal or drain" @@ -713,7 +721,11 @@ fn multi_agent_start_one_dag_folds_per_agent_stale_rebuild() { &[("prebuild", "done")] ), row("swap", Some("stop_for_update"), &[]), - row("post_swap", Some("stop_for_update"), &[("swap", "done")]), + row( + "rebuild_bookkeeping", + Some("stop_for_update"), + &[("swap", "done")] + ), row( "reconcile", None, @@ -869,21 +881,21 @@ fn rebuild_chain_nodes_suppress_crash_watch() { // individually legible, and the arrangement was the only reason this module // needed to claim and complete nodes. -/// The swap-success path: `Swap` ok → the `AfterOk` `PostSwap` (bookkeeping +/// The swap-success path: `Swap` ok → the `AfterOk` `RebuildBookkeeping` (bookkeeping /// tail) runs, and only then does `Reconcile` fire — serialized behind -/// `PostSwap` (not racing it) because `Reconcile` deps `AfterAny(PostSwap)`. +/// `RebuildBookkeeping` (not racing it) because `Reconcile` deps `AfterAny(RebuildBookkeeping)`. #[test] fn rebuild_reconcile_waits_for_the_whole_build_subtree() { // Replaces `swap_ok_runs_post_swap_before_reconcile` and // `swap_failure_still_runs_reconcile`, which walked the same DAG with the // swap succeeding in one and failing in the other. // - // The interesting claim was "Reconcile must wait for PostSwap, not race + // The interesting claim was "Reconcile must wait for RebuildBookkeeping, not race // it" — and it does *not* come from an edge between them. `reconcile` deps - // `AfterAny(agent_window)`, while `post_swap` sits inside the brace's - // subtree (post_swap → stop_for_update → agent_window). A parent is not + // `AfterAny(agent_window)`, while `rebuild_bookkeeping` sits inside the brace's + // subtree (rebuild_bookkeeping → 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 + // rebuild_bookkeeping is outstanding. **The ordering is the parent chain, not a // dependency.** // // Both facts are asserted in `rebuild_chain_is_declared_serial`; this test @@ -907,7 +919,7 @@ fn rebuild_reconcile_waits_for_the_whole_build_subtree() { .unwrap_or_else(|| panic!("{kind} node")) .parent }; - assert_eq!(parent_of("post_swap"), Some("stop_for_update")); + assert_eq!(parent_of("rebuild_bookkeeping"), Some("stop_for_update")); assert_eq!(parent_of("stop_for_update"), Some("agent_window")); assert_eq!( shape @@ -917,13 +929,13 @@ fn rebuild_reconcile_waits_for_the_whole_build_subtree() { .after, 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" + subtree — including rebuild_bookkeeping — and runs on failure too" ); } // `swap_failure_still_runs_reconcile` lived here. // -// It asserted that a failed swap leaves `post_swap` `Skipped` and `swap` +// It asserted that a failed swap leaves `rebuild_bookkeeping` `Skipped` and `swap` // `Failed`, and that reconcile still runs. All three are hive_jobq's cascade // (`failed_after_ok_dep_cancels_dependents_but_after_any_still_runs`), and the // "says so on the wire" half turned out to be nothing: `snapshot` fills @@ -1300,7 +1312,11 @@ fn deploy_apply_grows_rebuild_subgraph_and_finalizes_after_it() { &[("prebuild", "done")] ), row("swap", Some("stop_for_update"), &[]), - row("post_swap", Some("stop_for_update"), &[("swap", "done")]), + row( + "rebuild_bookkeeping", + Some("stop_for_update"), + &[("swap", "done")] + ), row( "reconcile", None, @@ -1539,7 +1555,7 @@ fn perm_change_shape_prefixes_rebuild_chain() { "prebuild", "stop_for_update", "swap", - "post_swap", + "rebuild_bookkeeping", "reconcile", // the ok / !ok tail pair "emit_rebuilt", From 7d26d6017fb6ed5d850c5e9a63da6ad8ea7d5d77 Mon Sep 17 00:00:00 2001 From: atlas Date: Tue, 4 Aug 2026 13:00:07 +0200 Subject: [PATCH 3/6] docs(#3034): move the brace rationale out of templates.rs into coordinator.md The pre-push comment-block lint rejected two 40-line doc blocks, correctly: the module doc and `rebuild_subtree`'s now carry the trigger and a pointer, and the reasoning lives in a new `#### Braces` section. That move surfaced a third doc the resource change had falsified. The scheduler's lease-acquirer list still named `StopForUpdate` / `Swap` / `Signal` / `Drain`, all of which are now exempt. The list now separates container-affecting nodes from braces, and says why the rebuild subtree's members are exempt for a different reason than `MetaSync` / `Prebuild`: they do touch the container, but their brace holds the lease above them. --- docs/coordinator.md | 61 ++++++++++++++++++----- hive-c0re/src/job_queue/templates.rs | 72 ++++++++++------------------ 2 files changed, 76 insertions(+), 57 deletions(-) diff --git a/docs/coordinator.md b/docs/coordinator.md index e06d77fb..4cd74ac4 100644 --- a/docs/coordinator.md +++ b/docs/coordinator.md @@ -190,17 +190,56 @@ resources are free. Resources: held by nix-heavy nodes for the node's duration. 2. **Per-agent lifecycle lease** — keyed on the **node's** agent (agent is per-node; a DAG can span agents) and globally exclusive per agent across - all DAGs: acquired at a container-affecting node (`SetWanted`, - `StopForUpdate`, `Swap`, `Signal`, `Drain`, `Reconcile`, `WriteDropin`, - `Create`, `DeployWindow`), held by the owning DAG until it's terminal, - so two DAGs never interleave container ops on the same agent. A DAG - touching several agents holds one lease per agent. (`SetWanted` is a store - write, not a container op, but takes the lease anyway so a power-op DAG's - intent write + reconcile is atomic — two racing ops can't clobber intent - before either reconciles.) **Lease-exempt**: `MetaSync`, `Prebuild`, - `MetaLock`, `WritePermFile`, `Reparent` — - they touch the store / meta, not the running container, which is exactly - why a stop can land while another DAG's prebuild is still building. + all DAGs: acquired either at a container-affecting node (`SetWanted`, + `Reconcile`, `WriteDropin`, `Create`) or at a **brace** (`AgentWindow`, + `DeployWindow`) on behalf of a whole coordinated subtree; held by the owning + DAG until it's terminal, so two DAGs never interleave container ops on the + same agent. A DAG touching several agents holds one lease per agent. + (`SetWanted` is a store write, not a container op, but takes the lease anyway + so a power-op DAG's intent write + reconcile is atomic — two racing ops can't + clobber intent before either reconciles.) **Lease-exempt**: `MetaSync`, + `Prebuild`, `MetaLock`, `WritePermFile`, `Reparent` — they touch the store / + meta, not the running container, which is exactly why a stop can land while + another DAG's prebuild is still building. Also exempt, for a different + reason, are the rebuild subtree's own members (`StopForUpdate`, `Swap`, + `Signal`, `Drain`, `RebuildBookkeeping`): they genuinely do touch the + container, but their `AgentWindow` brace holds the lease above them — see + _Braces_ below. + +#### Braces + +Templates otherwise declare a resource on **every** node that needs it, even +when a parent already holds it, so the requirement belongs to the node rather +than to one DAG shape it happens to appear in. A **brace** is the one sanctioned +exception: a pure-resource-holder root that declares on behalf of a subtree +coordinated with itself, whose members then declare nothing. + +It is forced rather than stylistic. Declaring a resource means *"I need this +exclusively"*, and the agent lease is single-unit — so **two siblings that both +declared it could never run concurrently.** For a subtree whose whole point is +concurrency (`Prebuild` beside the `Signal` → `Drain` quiesce window), declaring +the requirement truthfully on every node and running those nodes in parallel are +mutually exclusive. One holder above them speaks for the subtree. + +This is the opposite of the failure the declare-your-own rule exists to prevent, +not a relapse into it: there the requirement was *implicit*, inferred from a +node's kind and true only by accident of placement. Here it is explicit, on one +node, with the omission below it documented on the brace itself. + +Two consequences worth knowing: + +- **Flattening a chain under a brace is safe.** The stop chain used to nest + `Signal` over `Drain` over `StopForUpdate` specifically so the lease stayed + continuous — as independent siblings each would acquire it separately and + leave a gap another DAG could claim the agent in, mid-bounce. A brace supplies + that continuity directly, so the nesting is no longer load-bearing. +- **Observability is unaffected.** `running_transients` keys off a node's + *payload* agent, not off a declared lease edge, so every child still lights its + own dashboard pill and still reports its own `takes_container_down` to the + crash watcher. A brace itself reports `false`: it parents the stopping nodes + but does not stop anything, and claiming otherwise would widen crash + suppression across the build and tail, where a vanished container is still a + real crash. Among simultaneously-ready nodes competing for a resource, DAG-submit order wins (FIFO) so bulk operations drain predictably. The scheduler also owns the diff --git a/hive-c0re/src/job_queue/templates.rs b/hive-c0re/src/job_queue/templates.rs index f9353d15..12b1c5d4 100644 --- a/hive-c0re/src/job_queue/templates.rs +++ b/hive-c0re/src/job_queue/templates.rs @@ -1,25 +1,15 @@ -//! DAG shape builders — every operation as a template over the shared -//! node primitives. Pure (no I/O); each node carries its own `agent` (there is -//! no DAG-level agent) and **declares its own resources** with `.needs(…)` -//! right where it is constructed, rather than having them derived from its -//! kind. Deriving made the requirement a property of the *kind*, so a kind that -//! happened to run under an ancestor already holding the resource could get -//! away with declaring nothing. +//! DAG shape builders — every operation as a template over the shared node +//! primitives. Pure (no I/O); each node carries its own `agent` (there is no +//! DAG-level agent) and **declares its own resources** with `.needs(…)` right +//! where it is constructed, rather than derived from its kind. Deriving made the +//! requirement a property of the *kind*, so a kind running under an ancestor +//! that already held 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". +//! ([`NodeKind::AgentWindow`], [`NodeKind::DeployWindow`]) declaring for a +//! coordinated subtree whose members then declare nothing. Forced by the lease +//! being single-unit: two siblings that both declared it could never run +//! concurrently. Why this is not a relapse: `docs/coordinator.md`. //! //! ```text //! rebuild(a): MetaSync(a) → AgentWindow(a){ Prebuild(a) → StopForUpdate(a) → Swap(a) →(ok) RebuildBookkeeping(a) } →(any) Reconcile(a) @@ -37,7 +27,7 @@ //! The hive-wide **power ops** (`stop` / `start` / `restart`) are NOT here: //! their per-agent shape depends on live running state (an async //! `lifecycle::is_running` read), so `submit.rs` assembles them out of the -//! primitives this module exports ([`rebuild_nodes`]) over `JobBuilder::node`. +//! primitives this module exports ([`rebuild_nodes`]). use hive_jobq::TerminalState; @@ -174,34 +164,24 @@ impl<'a> RebuildRoots<'a> { /// 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. +/// - `Prebuild` and the **quiesce chain** (`Signal` → `Drain`, graceful only) +/// are siblings under the brace and run **concurrently** — they contend for +/// different resources, so nesting the stop under the build (as this template +/// did) only hid the graceful-stop timeout behind the nix build. The container +/// is still up throughout the quiesce. /// - `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 `RebuildBookkeeping` (`AfterOk` its sibling -/// `Swap`): the Ok-only bookkeeping tail (rev marker, forge/matrix sync, -/// kick, rescan). +/// `Drain`. Waiting on the build is deliberate — running the drain early is +/// the win, taking the container *down* early would be pure downtime. +/// - `Swap` (child of `StopForUpdate`), then `RebuildBookkeeping` (`AfterOk` its +/// sibling `Swap`): the Ok-only bookkeeping tail. /// - `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. +/// terminal only once its whole subtree has settled — so it 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). Fresh lease; +/// the tiny gap is harmless, `Reconcile` converges 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. +/// Why flattening the stop chain is safe, and why the lease-continuity argument +/// that used to justify nesting it is satisfied by the brace: `docs/coordinator.md`. fn rebuild_subtree<'a>( builder: &'a JobBuilder, agent: &str, From 50808007a6c6859c26de55a1d7a02ba74288066f Mon Sep 17 00:00:00 2001 From: atlas Date: Tue, 4 Aug 2026 13:07:14 +0200 Subject: [PATCH 4/6] docs(#3034): sweep the remaining stale rebuild-shape references argus caught `approvals.md` still describing the old serial chain under the old node name. Grepping the name across *all* tracked files rather than just `*.rs` turned up three more, all in `coordinator.md`: the node-inventory rows for `Swap` and the bookkeeping tail, and the rebuild shape diagram. Three of the four were in the file I had edited in the previous commit to add the brace section, which is the point worth recording: I grepped the *concept* I had changed (`lease`) and the *symbol* I had renamed, but scoped the rename grep to Rust. Neither pass could see an old node name sitting in prose. Also adds the missing `AgentWindow` row to the node inventory. --- docs/approvals.md | 5 +++-- docs/coordinator.md | 7 ++++--- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/docs/approvals.md b/docs/approvals.md index 998cbab8..c1c3f244 100644 --- a/docs/approvals.md +++ b/docs/approvals.md @@ -319,8 +319,9 @@ running and a restart resumes at node granularity: `applied//main` (which the deploy already fast-forwarded to the reviewed PR head). 2. The rebuild subgraph `DeployApply` grows into the DAG builds and - swaps the container (`Prebuild → StopForUpdate → Swap → PostSwap`, - plus `Reconcile`). Nix evaluates against the staged lock. + swaps the container (`AgentWindow` bracing `Prebuild → StopForUpdate + → Swap → RebuildBookkeeping`, plus `Reconcile`). Nix evaluates + against the staged lock. 3. On success — `FinalizeDeploy` drops the rollback ref, plants `deployed/`, then `meta::finalize_deploy(name, sha, "deployed/ ")` stages `flake.lock` and commits with diff --git a/docs/coordinator.md b/docs/coordinator.md index 4cd74ac4..115d8ec2 100644 --- a/docs/coordinator.md +++ b/docs/coordinator.md @@ -40,7 +40,7 @@ Nix-heavy — hold one of the `buildSlots` permits for the node's duration: | Node | Wraps | | ---------------- | ---------------------------------------------------------------------------------------------------------------------------------------------- | | `Prebuild` | `lifecycle::prebuild_toplevel` — build the toplevel out-of-band while the container keeps serving (its meta preamble is the upstream `MetaSync` node) | -| `Swap` | drop-in rewrite + `nixos-container update` profile-swap (requires the container stopped); the post-swap bookkeeping tail lives in the sibling `PostSwap` node | +| `Swap` | drop-in rewrite + `nixos-container update` profile-swap (requires the container stopped); the post-swap bookkeeping tail lives in the sibling `RebuildBookkeeping` node | | `Create` | first-spawn provisioning + `nixos-container create` (atomic build+create) | | `MetaLock` | meta flake lock bump (`lock_update` / boot-sweep `lock_update_hyperhive`, commit fused — see below); fans out child `Rebuild` DAGs on completion | | `DeployWindow` | resource-holding root of the merge-config-PR deploy subtree — declares the build slot, the lease and the meta window, then completes immediately so its children run under them (see _Approvals_ below) | @@ -55,7 +55,8 @@ Cheap — no build slot: | `MetaSync` | the rebuild's meta preamble — rebuild-dir prep, idempotent meta `sync_agents`, optional per-agent relock. Holds the `MetaWindow` resource (below); deliberately its own node so the window never covers `Prebuild`'s multi-minute build | | `Reconcile` | idempotent power converge: read `wanted` (below) + observed state; start if `Up` & down (cold-start fallback included), stop if `Offline` & up, else noop | | `StopForUpdate` | mechanical `nixos-container stop` for the profile swap; never touches `wanted`; noop if already stopped | -| `PostSwap` | the swap's Ok-only bookkeeping tail — rev marker, forge/matrix sync, manager kick, rescan, meta-inputs snapshot; `AfterOk(Swap)` so it runs only on a successful swap (the `Rebuilt` manager event is emitted by the DAG's `EmitRebuilt` tail node, not here) | +| `RebuildBookkeeping` | the swap's Ok-only bookkeeping tail — rev marker, forge/matrix sync, manager kick, rescan, meta-inputs snapshot; `AfterOk(Swap)` so it runs only on a successful swap (the `Rebuilt` manager event is emitted by the DAG's `EmitRebuilt` tail node, not here) | +| `AgentWindow` | pure resource holder — the brace for one agent's rebuild. Declares the build slot + agent lease atomically and holds both for its whole subtree, so `Prebuild` and the `Signal`→`Drain` quiesce window run concurrently instead of one nested under the other. Performs no work; see _Braces_ | | `Signal` | set the graceful fence + kick, so the harness runs one stop-checkpoint turn | | `Drain` | await the harness clearing the fence, bounded by the 3-min graceful-stop timeout; resolves ok either way | | `WriteDropin` | `set_nspawn_flags` + `set_resource_limits` + daemon-reload | @@ -118,7 +119,7 @@ sweep. `start` folds the per-agent stale-rev upgrade in (a *down + stale* agent's subgraph is a rebuild-then-start). ```text -rebuild(a): MetaSync(a) → Prebuild(a) → StopForUpdate(a) → Swap(a) →(after-ok) PostSwap(a) →(after-any) Reconcile(a) +rebuild(a): MetaSync(a) → AgentWindow(a){ Prebuild(a) ∥ [Signal(a)→Drain(a) if graceful]; both →(after-ok) StopForUpdate(a) → Swap(a) →(after-ok) RebuildBookkeeping(a) } →(after-any) Reconcile(a) stop(a..): online a: SetWanted(a,Off) → [Signal→Drain→ if graceful] Reconcile(a) offline a: SetWanted(a,Off) → Reconcile(a) (N subgraphs, 1 DAG) restart(a..): online a: [Signal→Drain→ if graceful] StopForUpdate(a) → Reconcile(a) (no SetWanted) From c2eafa754816c8e52640b21629e1477fc68500e7 Mon Sep 17 00:00:00 2001 From: atlas Date: Tue, 4 Aug 2026 15:04:26 +0200 Subject: [PATCH 5/6] refactor(#3034): one quiesce builder, shared by the rebuild and the stop chain MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `Signal` -> `Drain` pair was built in three places, in three different shapes: siblings under `SetWanted` in `stop_chain`, `Drain` nested under a lease-holding `Signal` in `restart_chain`, and — as of this branch — a hybrid in `rebuild_subtree` that was brace-held like the first and nested like the second. `templates::quiesce(builder, agent, brace)` is now the one definition, returning the `Drain` handle a caller edges its stop onto. Both nodes hang off the brace as dep-ordered siblings and declare nothing, borrowing the lease it already holds. That also fixes an inconsistency this branch introduced: the PR argued that a brace makes nesting unnecessary and used it to flatten `StopForUpdate` off `Signal`, then left `Drain` nested under `Signal` two lines away. Nesting is only load-bearing where `Signal` is itself the lease holder. `stop_chain`'s pair loses its own `Agent` declaration as a result — `SetWanted` holds the lease for the subtree, so those were redundant re-entrant borrows. `restart_chain` is left alone and says why in place: it has no brace, so `Signal` holds the lease and the nesting under it is what keeps the grant continuous. Giving it one would unify all three sites at the cost of an extra no-op node on every graceful restart, which an operator would see — not something to change as a side effect of a rebuild-shape PR. --- hive-c0re/src/job_queue/submit.rs | 21 +++++++++------- hive-c0re/src/job_queue/templates.rs | 36 +++++++++++++++++++++------- hive-c0re/src/job_queue/tests.rs | 24 ++++++++++++------- 3 files changed, 56 insertions(+), 25 deletions(-) diff --git a/hive-c0re/src/job_queue/submit.rs b/hive-c0re/src/job_queue/submit.rs index 95a36260..3ec3e11b 100644 --- a/hive-c0re/src/job_queue/submit.rs +++ b/hive-c0re/src/job_queue/submit.rs @@ -81,15 +81,10 @@ fn stop_chain(builder: &JobBuilder, agent: &str, graceful: bool, running: bool) // Declaration order is dependency order: the quiesce steps come first so // the `Reconcile` that waits on them can name them. if graceful && running { - let signal = builder - .node(NodeKind::Signal { agent: a() }) - .needs(Resource::Agent(a())) - .part_of(wanted); - let drain = builder - .node(NodeKind::Drain { agent: a() }) - .needs(Resource::Agent(a())) - .part_of(wanted) - .after_ok(signal); + // `SetWanted` is the brace here, so the quiesce pair borrows its grant + // rather than declaring the lease itself — same shape the rebuild + // template uses, one definition. + let drain = super::templates::quiesce(builder, agent, wanted); let _ = builder .node(NodeKind::Reconcile { agent: a() }) .needs(Resource::Agent(a())) @@ -155,6 +150,14 @@ fn restart_chain(builder: &JobBuilder, agent: &str, graceful: bool, running: boo // `Reconcile` gates on the last mechanical step. For a non-graceful bounce // that step *is* the root, and the parent gate already orders it — a child // must NOT dep on its own parent (dep-scope), so it takes no sibling edge. + // + // ⚠️ This is the one quiesce site that does NOT use `templates::quiesce`. + // The helper needs a brace holding the lease above the pair; here `Signal` + // *is* the holder, and the nesting under it is what keeps the grant + // continuous across the bounce. Giving this chain its own brace would + // unify all three sites — at the cost of one extra no-op node on every + // graceful restart, which an operator would see. Deliberately not done as + // a side effect of a rebuild-shape change. if graceful { let signal = builder .node(NodeKind::Signal { agent: a() }) diff --git a/hive-c0re/src/job_queue/templates.rs b/hive-c0re/src/job_queue/templates.rs index 12b1c5d4..03c043ff 100644 --- a/hive-c0re/src/job_queue/templates.rs +++ b/hive-c0re/src/job_queue/templates.rs @@ -128,6 +128,33 @@ pub(crate) fn fanned_out_mechanical(builder: &JobBuilder, kind: NodeKind) { let _ = builder.node(kind).needs(lease); } +/// The graceful quiesce pair — `Signal` then `Drain`: ask the agent to run its +/// stop-checkpoint turn, then wait for it to go quiet. Returns the `Drain` +/// handle, which is what a caller edges its stop onto. +/// +/// The container is still **up** throughout; this only reaches a point where +/// stopping is safe. The actual stop is the caller's next node. +/// +/// `brace` must already hold [`Resource::Agent`] for its whole subtree. The two +/// nodes therefore declare **nothing** and borrow that grant — which is what +/// lets them run *beside* the brace's other children (a nix build, typically) +/// instead of serialising against them. Declaring the lease here would make +/// them mutually exclusive with any sibling that also declared it, since the +/// lease is single-unit; see this module's header for the general rule. +/// +/// They are **siblings under the brace, dep-ordered**, not nested. Nesting +/// `Drain` under `Signal` is only necessary where `Signal` is itself the lease +/// holder and the nesting is what keeps the grant continuous — with a brace +/// above, that reason is gone. +pub(crate) fn quiesce<'a>(builder: &'a JobBuilder, agent: &str, brace: Handle<'a>) -> Handle<'a> { + let a = || agent.to_owned(); + let signal = builder.node(NodeKind::Signal { agent: a() }).part_of(brace); + builder + .node(NodeKind::Drain { agent: a() }) + .part_of(brace) + .after_ok(signal) +} + /// The group-roots a [`rebuild_nodes`] subgraph exposes to its caller: what a /// tail node edges onto, and what a follow-up node waits for. /// @@ -217,14 +244,7 @@ fn rebuild_subtree<'a>( .node(NodeKind::Prebuild { agent: a() }) .part_of(agent_window); - let drain = graceful.then(|| { - let signal = builder - .node(NodeKind::Signal { agent: a() }) - .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). - builder.node(NodeKind::Drain { agent: a() }).part_of(signal) - }); + let drain = graceful.then(|| quiesce(builder, agent, agent_window)); // The container goes down here, not earlier: `AfterOk` the build so a // failed build never stops a healthy container, and `AfterOk` the drain so diff --git a/hive-c0re/src/job_queue/tests.rs b/hive-c0re/src/job_queue/tests.rs index 4b6bdd63..66f16e9b 100644 --- a/hive-c0re/src/job_queue/tests.rs +++ b/hive-c0re/src/job_queue/tests.rs @@ -494,7 +494,9 @@ fn graceful_rebuild_chain_drains_before_stopping() { // 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"), &[]), + // Siblings under the brace, dep-ordered — not nested. Nesting is + // only needed where `Signal` itself holds the lease. + row("drain", Some("agent_window"), &[("signal", "done")]), // 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. @@ -1490,18 +1492,24 @@ fn graceful_stop_shape_signal_drain_reconcile() { row("reconcile", Some("set_wanted"), &[("drain", "done")]), ] ); - // Neither half of the graceful window takes a build slot. That is what lets - // a whole-hive graceful stop overlap every agent's drain even at - // `buildSlots = 1` while a rebuild hogs the slot — the cost ceiling is one - // `GRACEFUL_STOP_TIMEOUT` in total, not one per agent. - let agent = Resource::Agent("agent-a".to_owned()); + // The quiesce pair declares **nothing**: `set_wanted` is the brace and holds + // the lease for the whole subtree, so both borrow that grant. Same shape, + // same helper (`templates::quiesce`) as the rebuild's window. + // + // ⚠️ In particular neither takes a build slot — which is what lets a + // whole-hive graceful *stop* overlap every agent's drain even at + // `buildSlots = 1`: the ceiling is one `GRACEFUL_STOP_TIMEOUT` in total, + // not one per agent. That holds here because nothing in a stop chain is + // slot-needing. It does **not** hold for the boot *sweep*, whose rebuilds + // do hold the slot across their drains — see the note on the sweep in + // `exec.rs`. assert_eq!( [ declared_resources(&q, node_of(&q, id, "signal")), declared_resources(&q, node_of(&q, id, "drain")), ], - [vec![agent.clone()], vec![agent]], - "signal and drain hold the lease but never a build slot" + [vec![], vec![]], + "the quiesce pair borrows the brace's lease and declares nothing" ); } From 0a14055a33530820ec526dce14ad70856be972a8 Mon Sep 17 00:00:00 2001 From: atlas Date: Tue, 4 Aug 2026 16:38:30 +0200 Subject: [PATCH 6/6] docs(#3034): give the brace one home instead of five MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Measured after mara's "2/3 of this is docs, most of it duplicated": 243 of 380 added .rs lines were comments. The brace rationale was written out in full in `model.rs`, the `templates.rs` module header, `quiesce`, `rebuild_subtree` and `docs/coordinator.md` — five copies of one argument, which is why four docs needed correcting earlier in this branch. Correcting every copy preserves the thing that made them go stale. `docs/coordinator.md` (_Braces_) is now the single home. The rest state what a node *is* and point there. Also drops the per-operation DAG diagram from the `templates.rs` header, which the same doc already carries, and cuts `rebuild_subtree`'s node-by-node walkthrough down to the three choices a reader would otherwise undo — the code below it is the source of truth for the shape. Comments -59 lines, no behaviour change, 317 tests unchanged. --- hive-c0re/src/job_queue/exec.rs | 7 +-- hive-c0re/src/job_queue/model.rs | 49 +++++------------ hive-c0re/src/job_queue/templates.rs | 80 +++++++++------------------- hive-c0re/src/job_queue/tests.rs | 25 +++------ 4 files changed, 45 insertions(+), 116 deletions(-) diff --git a/hive-c0re/src/job_queue/exec.rs b/hive-c0re/src/job_queue/exec.rs index 6a0bb513..fe0c4074 100644 --- a/hive-c0re/src/job_queue/exec.rs +++ b/hive-c0re/src/job_queue/exec.rs @@ -130,11 +130,8 @@ pub(super) async fn run_node( // 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. - // - `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. + // - `DeployWindow` / `AgentWindow`: pure resource holders (braces) — + // what they declare stays held until their subtree settles. NodeKind::Dag { .. } | NodeKind::DeployWindow { .. } | NodeKind::AgentWindow { .. } => { Ok(()) } diff --git a/hive-c0re/src/job_queue/model.rs b/hive-c0re/src/job_queue/model.rs index 03f86c4b..f1f7936d 100644 --- a/hive-c0re/src/job_queue/model.rs +++ b/hive-c0re/src/job_queue/model.rs @@ -159,36 +159,14 @@ 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. + /// Group root of a rebuild subtree — the **brace**: declares the agent + /// lease and the build slot, holds both for the whole subtree, and performs + /// no work of its own. Same pure-resource-holder shape as + /// [`NodeKind::DeployWindow`], scoped to one agent. /// - /// 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. + /// Its children declare no resources and borrow these grants, which is what + /// lets `Prebuild` run beside the `Signal` → `Drain` window. Why braces + /// exist and what they cost: `docs/coordinator.md`, _Braces_. 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 @@ -460,13 +438,10 @@ 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. + // - `AgentWindow` likewise, though it *parents* nodes that answer + // `true`. This is per-node, not per-subtree, and each of those + // children reports for itself — so `true` here would only widen + // suppression over the build and 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 03c043ff..536093fc 100644 --- a/hive-c0re/src/job_queue/templates.rs +++ b/hive-c0re/src/job_queue/templates.rs @@ -7,18 +7,9 @@ //! //! **The one sanctioned exception is a brace** — a pure-resource-holder root //! ([`NodeKind::AgentWindow`], [`NodeKind::DeployWindow`]) declaring for a -//! coordinated subtree whose members then declare nothing. Forced by the lease -//! being single-unit: two siblings that both declared it could never run -//! concurrently. Why this is not a relapse: `docs/coordinator.md`. -//! -//! ```text -//! rebuild(a): MetaSync(a) → AgentWindow(a){ Prebuild(a) → StopForUpdate(a) → Swap(a) →(ok) RebuildBookkeeping(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» -//! reparent(moves): Reparent(moves) [no rebuild — topology.json is read live] -//! ``` +//! coordinated subtree whose members then declare nothing. See +//! `docs/coordinator.md`, _Braces_ — which also carries the per-operation DAG +//! shapes, so they are not restated here. //! //! Nodes are **named, not counted** — a template holds the handle //! [`JobBuilder::node`] hands back, so an edge says which node it waits on. Why that @@ -130,22 +121,13 @@ pub(crate) fn fanned_out_mechanical(builder: &JobBuilder, kind: NodeKind) { /// The graceful quiesce pair — `Signal` then `Drain`: ask the agent to run its /// stop-checkpoint turn, then wait for it to go quiet. Returns the `Drain` -/// handle, which is what a caller edges its stop onto. +/// handle, which is what a caller edges its stop onto. The container stays +/// **up** throughout; the actual stop is the caller's next node. /// -/// The container is still **up** throughout; this only reaches a point where -/// stopping is safe. The actual stop is the caller's next node. -/// -/// `brace` must already hold [`Resource::Agent`] for its whole subtree. The two -/// nodes therefore declare **nothing** and borrow that grant — which is what -/// lets them run *beside* the brace's other children (a nix build, typically) -/// instead of serialising against them. Declaring the lease here would make -/// them mutually exclusive with any sibling that also declared it, since the -/// lease is single-unit; see this module's header for the general rule. -/// -/// They are **siblings under the brace, dep-ordered**, not nested. Nesting -/// `Drain` under `Signal` is only necessary where `Signal` is itself the lease -/// holder and the nesting is what keeps the grant continuous — with a brace -/// above, that reason is gone. +/// `brace` must already hold [`Resource::Agent`] for its subtree; the pair +/// declares nothing and borrows it (see the module header). They are +/// dep-ordered **siblings**, not nested — nesting is only load-bearing where +/// `Signal` itself holds the lease, as in `submit.rs`'s `restart_chain`. pub(crate) fn quiesce<'a>(builder: &'a JobBuilder, agent: &str, brace: Handle<'a>) -> Handle<'a> { let a = || agent.to_owned(); let signal = builder.node(NodeKind::Signal { agent: a() }).part_of(brace); @@ -179,36 +161,22 @@ impl<'a> RebuildRoots<'a> { } } -/// 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 the brace's -/// parent: a resource is held across the holder's whole subtree, so parenting -/// 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** (`Signal` → `Drain`, graceful only) -/// are siblings under the brace and run **concurrently** — they contend for -/// different resources, so nesting the stop under the build (as this template -/// did) only hid the graceful-stop timeout behind the nix build. The container -/// is still up throughout the quiesce. -/// - `StopForUpdate` (child of the brace): `AfterOk` **both** `Prebuild` and -/// `Drain`. Waiting on the build is deliberate — running the drain early is -/// the win, taking the container *down* early would be pure downtime. -/// - `Swap` (child of `StopForUpdate`), then `RebuildBookkeeping` (`AfterOk` its -/// sibling `Swap`): the Ok-only bookkeeping tail. -/// - `Reconcile` (**last, root**): `AfterAny` `AgentWindow`, which rolls up -/// terminal only once its whole subtree has settled — so it 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). Fresh lease; -/// the tiny gap is harmless, `Reconcile` converges idempotently. +/// The rebuild node subtree — three group roots (`MetaSync`, the `AgentWindow` +/// brace, `Reconcile`). `after`, when given, is the node it chains behind. The +/// shape itself is in `docs/coordinator.md`; the code below is the source of +/// truth for it, so only the three choices a reader would otherwise undo are +/// called out here: /// -/// Why flattening the stop chain is safe, and why the lease-continuity argument -/// that used to justify nesting it is satisfied by the brace: `docs/coordinator.md`. +/// - **`MetaSync` is a sibling root, not the brace's parent** — it owns the +/// *global* `MetaWindow`, and a resource is held across the holder's whole +/// subtree, so parenting the rebuild under it would serialise every agent's +/// nix build behind one hive-wide window. +/// - **`StopForUpdate` waits on `Prebuild` as well as `Drain`** — running the +/// drain early is the win; taking the container *down* early is pure downtime. +/// - **`Reconcile` is a top-level root, not a child** — so it survives the +/// cancel-cascade of a failed brace and still converges the container +/// (recovery-start invariant). It takes a fresh lease; the gap is harmless +/// because it is idempotent. fn rebuild_subtree<'a>( builder: &'a JobBuilder, agent: &str, diff --git a/hive-c0re/src/job_queue/tests.rs b/hive-c0re/src/job_queue/tests.rs index 66f16e9b..4bc199a9 100644 --- a/hive-c0re/src/job_queue/tests.rs +++ b/hive-c0re/src/job_queue/tests.rs @@ -558,16 +558,10 @@ 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`). // - // ⚠️ **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. + // ⚠️ **The rename is a reversal, not tidying.** The old name asserted "the + // slot follows the nix work and the lease follows the container" — each node + // declaring what it personally needed. The brace inverts that for a + // coordinated subtree; see `docs/coordinator.md`, _Braces_. // // (`hive_jobq` owns slot *fairness*, pinned there by // `a_contended_resource_goes_to_the_oldest_waiter`. What is c0re's is @@ -903,14 +897,9 @@ fn rebuild_reconcile_waits_for_the_whole_build_subtree() { // 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. + // and still lose the guarantee. That warning earned itself: `AgentWindow` + // **did** flatten this chain, and the guarantee survives only because the + // roll-up point moved with it. let q = JobQueue::new(1); let id = submit(&q, "r", |builder| rebuild(builder, "agent-a")); let shape = declared_shape(&q, id);