From 0a14055a33530820ec526dce14ad70856be972a8 Mon Sep 17 00:00:00 2001 From: atlas Date: Tue, 4 Aug 2026 16:38:30 +0200 Subject: [PATCH] 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);