fix(#3034): run prebuild beside the graceful-stop window, braced by AgentWindow
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.
This commit is contained in:
parent
289db00321
commit
31a1853a45
4 changed files with 295 additions and 140 deletions
|
|
@ -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.
|
||||
}
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in a new issue