From c2eafa754816c8e52640b21629e1477fc68500e7 Mon Sep 17 00:00:00 2001 From: atlas Date: Tue, 4 Aug 2026 15:04:26 +0200 Subject: [PATCH] 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" ); }