refactor(#2815): derive the transient pill from the running node
The dashboard pill was declared once per DAG at submit time, so a rebuild reported `rebuilding` for its entire life — through the prebuild, the stop, the swap, the tail and the reconcile. It named the intent of the request, not what was happening. It is now read off the nodes actually running. A node lights a pill when it is `Running` and declares the agent's own resource. Declaring is the test, not targeting: `Prebuild` and `MetaSync` name an agent but are lease-exempt on purpose (the container keeps serving), so they must not light one. It is also not the lease *owner* — `resource_state()` answers "who holds the slot", which is a different question from "what is running", and a descendant that borrows an ancestor's grant never appears in that map. `TransientKind` is gone entirely rather than being re-derived. The label is the node's own wire tag (`NodeKind::as_str`) — the same vocabulary `NodeView.kind` already ships, so a pill and a DAG node name an operation identically and there is no second taxonomy to keep in step. Work with no node behind it (destroy, migration) supplies its own literal. `DagSpec::transient`, `Claim::transient`, `DagMeta::transient` and `NodeKind::Dag`'s `transient` field all go with it. ## the safety half, which is deliberately not the display half `crash_watch::is_deliberate_stop` used to match a `TransientKind` to decide whether a vanished container was intentional or a crash. That made a pill's display vocabulary decide an alerting question, so renaming or adding a label would silently move the alerting boundary. `TransientState` now carries two independent fields: `label` (rendered, nothing branches on it) and `deliberate_stop` (read only by the crash watcher). The producer sets the second, because the producer is the only thing that knows — it is not recoverable from the first. For queue work that value is `NodeKind::takes_container_down()`, and it is emphatically not "holds a lease": `Create` and `Start` hold the agent's lease exactly like `Stop` does, and a container dying *while starting* is a real crash that must keep reporting as one. The default is `false` on purpose — a wrong `false` costs a spurious crash event, a wrong `true` swallows a real crash silently. ## known cost, accepted on the issue A restart no longer reads `restarting`. No `NodeKind` is unique to a restart — `restart_chain` reuses `Signal` / `StopForUpdate` / `Drain` / `Reconcile` — because "restart" is a property of the DAG's shape, not of any node. A restart now reads `signal` / `stop_for_update`, then the agent returns. `Start` / `Stop` / `PostSwap` run inside a lease-holding ancestor and re-declare nothing, so they light no pill and the agent reads idle for those windows. Closing that is the resources-where-constructed work (#2818), not this change. Checked with clippy (`--all-targets -D warnings`), `cargo test -p hive-c0re -p hive-jobq` (321 + 40 passed) and `nix fmt`.
This commit is contained in:
atlas
parent
0aac20d863
commit
d3d73b5ffb
14 changed files with 295 additions and 261 deletions
|
|
@ -133,7 +133,7 @@ pub struct Coordinator {
|
|||
/// agents whose tombstone is still inside the grace window. Crash
|
||||
/// watcher consults both this and the active map before declaring
|
||||
/// a stop deliberate.
|
||||
recent_transient: Mutex<HashMap<String, (TransientKind, std::time::Instant)>>,
|
||||
recent_transient: Mutex<HashMap<String, (bool, std::time::Instant)>>,
|
||||
/// Timestamps of recent unexpected container crashes, keyed by agent.
|
||||
/// Fed by `crash_watch` each time it classifies a stop as a crash (so
|
||||
/// a crash-looping container — which `Restart=on-failure` flips back
|
||||
|
|
@ -357,9 +357,30 @@ pub struct AgentPaths {
|
|||
|
||||
/// Per-agent in-progress state that the dashboard surfaces between approve
|
||||
/// click and container ready.
|
||||
///
|
||||
/// The two fields answer genuinely different questions and are set
|
||||
/// independently on purpose. There used to be a single `TransientKind` enum
|
||||
/// serving both, which meant a display concern and a safety decision shared one
|
||||
/// vocabulary and moved together.
|
||||
#[derive(Debug, Clone)]
|
||||
pub struct TransientState {
|
||||
pub kind: TransientKind,
|
||||
/// What the dashboard pill renders. For queue-driven work this is the
|
||||
/// running node's own wire tag ([`crate::job_queue::NodeKind::as_str`]) —
|
||||
/// the same vocabulary the DAG view ships, so a pill and a node name an
|
||||
/// operation identically. Work with no node behind it (destroy, migration)
|
||||
/// supplies its own.
|
||||
///
|
||||
/// Display only. Nothing branches on it — match on a string and this
|
||||
/// becomes a taxonomy again, silently.
|
||||
pub label: String,
|
||||
/// Whether the container going down is **expected**, i.e. this operation
|
||||
/// takes it down on purpose. Read by the crash watcher to tell a
|
||||
/// deliberate stop from a crash, so a wrong value here either raises a
|
||||
/// false alarm or swallows a real one.
|
||||
///
|
||||
/// Set by whoever creates the transient, which is the only place that
|
||||
/// actually knows — it is not recoverable from `label`.
|
||||
pub deliberate_stop: bool,
|
||||
pub since: std::time::Instant,
|
||||
}
|
||||
|
||||
|
|
@ -408,38 +429,6 @@ impl Drop for MetaUpdateGuard {
|
|||
}
|
||||
}
|
||||
|
||||
#[derive(Debug, Clone, Copy, PartialEq, Eq, serde::Serialize)]
|
||||
#[serde(rename_all = "snake_case")]
|
||||
pub enum TransientKind {
|
||||
/// `lifecycle::spawn` is running (nixos-container create + update + start).
|
||||
Spawning,
|
||||
/// `lifecycle::start` is running.
|
||||
Starting,
|
||||
/// `lifecycle::kill` is running.
|
||||
Stopping,
|
||||
/// A restart (`lifecycle::kill` then `lifecycle::start`) is running.
|
||||
Restarting,
|
||||
/// `lifecycle::rebuild` is running (nixos-container update).
|
||||
Rebuilding,
|
||||
/// `actions::destroy` is running.
|
||||
Destroying,
|
||||
}
|
||||
|
||||
impl TransientKind {
|
||||
/// Wire/UI label. Matches the strings the dashboard already
|
||||
/// renders in the transient spinner.
|
||||
pub fn as_str(self) -> &'static str {
|
||||
match self {
|
||||
TransientKind::Spawning => "spawning",
|
||||
TransientKind::Starting => "starting",
|
||||
TransientKind::Stopping => "stopping",
|
||||
TransientKind::Restarting => "restarting",
|
||||
TransientKind::Rebuilding => "rebuilding",
|
||||
TransientKind::Destroying => "destroying",
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/// Field-named payload for [`Coordinator::emit_approval_resolved`].
|
||||
/// Mirrors the `ApprovalResolved` dashboard-event fields. `agent`
|
||||
/// borrows from the caller; `approval_kind` / `status` are
|
||||
|
|
@ -1094,11 +1083,12 @@ impl Coordinator {
|
|||
/// is cancelled (HTTP request aborted, runtime shutdown mid-rebuild,
|
||||
/// panic). A bare set with no guaranteed clear would leak the transient
|
||||
/// and leave the dashboard stuck in "rebuilding…" forever.
|
||||
fn set_transient(&self, name: &str, kind: TransientKind) {
|
||||
fn set_transient(&self, name: &str, label: String, deliberate_stop: bool) {
|
||||
self.transient.lock().unwrap().insert(
|
||||
name.to_owned(),
|
||||
TransientState {
|
||||
kind,
|
||||
label: label.clone(),
|
||||
deliberate_stop,
|
||||
since: std::time::Instant::now(),
|
||||
},
|
||||
);
|
||||
|
|
@ -1113,7 +1103,7 @@ impl Coordinator {
|
|||
self.emit_dashboard_event(DashboardEvent::TransientSet {
|
||||
seq: self.next_seq(),
|
||||
name: name.to_owned(),
|
||||
transient_kind: kind.as_str(),
|
||||
transient_kind: label,
|
||||
since_unix,
|
||||
});
|
||||
}
|
||||
|
|
@ -1130,10 +1120,10 @@ impl Coordinator {
|
|||
// spurious ContainerCrash on every operator stop/restart.
|
||||
// Old entries get reaped lazily on read so the map doesn't
|
||||
// grow unbounded.
|
||||
self.recent_transient
|
||||
.lock()
|
||||
.unwrap()
|
||||
.insert(name.to_owned(), (state.kind, std::time::Instant::now()));
|
||||
self.recent_transient.lock().unwrap().insert(
|
||||
name.to_owned(),
|
||||
(state.deliberate_stop, std::time::Instant::now()),
|
||||
);
|
||||
self.emit_dashboard_event(DashboardEvent::TransientCleared {
|
||||
seq: self.next_seq(),
|
||||
name: name.to_owned(),
|
||||
|
|
@ -1205,20 +1195,19 @@ impl Coordinator {
|
|||
result
|
||||
}
|
||||
|
||||
/// Set of agents whose transient was cleared within the last
|
||||
/// `grace` seconds — i.e. agents the operator just acted on,
|
||||
/// whose stop the crash watcher should NOT classify as a crash.
|
||||
/// Lazily reaps entries older than `grace` so the map stays
|
||||
/// bounded by the active agent count.
|
||||
pub fn recent_transient_within(
|
||||
&self,
|
||||
grace: std::time::Duration,
|
||||
) -> HashMap<String, TransientKind> {
|
||||
/// Per-agent `deliberate_stop` for transients cleared within the last
|
||||
/// `grace` seconds — i.e. agents the operator just acted on, whose stop the
|
||||
/// crash watcher should NOT classify as a crash. Lazily reaps entries older
|
||||
/// than `grace` so the map stays bounded by the active agent count.
|
||||
///
|
||||
/// Carries only the safety bit, not the display label: nothing downstream
|
||||
/// should be able to re-derive a stop/crash decision from a pill's wording.
|
||||
pub fn recent_transient_within(&self, grace: std::time::Duration) -> HashMap<String, bool> {
|
||||
let now = std::time::Instant::now();
|
||||
let mut map = self.recent_transient.lock().unwrap();
|
||||
map.retain(|_, (_, ts)| now.duration_since(*ts) <= grace);
|
||||
map.iter()
|
||||
.map(|(k, (kind, _))| (k.clone(), *kind))
|
||||
.map(|(k, (deliberate, _))| (k.clone(), *deliberate))
|
||||
.collect()
|
||||
}
|
||||
|
||||
|
|
@ -1254,8 +1243,18 @@ impl Coordinator {
|
|||
/// cancelled or panic between set and clear (HTTP handlers, spawned
|
||||
/// tasks). The guard's `Drop` runs even on task cancellation, so
|
||||
/// the dashboard's spinner can't get pinned forever.
|
||||
pub fn transient_guard(self: &Arc<Self>, name: &str, kind: TransientKind) -> TransientGuard {
|
||||
self.set_transient(name, kind);
|
||||
///
|
||||
/// `label` is what the pill renders; `deliberate_stop` says whether this
|
||||
/// operation takes the container down on purpose, and is what the crash
|
||||
/// watcher reads. Only the caller knows the second one — it is not
|
||||
/// recoverable from the first.
|
||||
pub fn transient_guard(
|
||||
self: &Arc<Self>,
|
||||
name: &str,
|
||||
label: impl Into<String>,
|
||||
deliberate_stop: bool,
|
||||
) -> TransientGuard {
|
||||
self.set_transient(name, label.into(), deliberate_stop);
|
||||
TransientGuard {
|
||||
coord: self.clone(),
|
||||
name: name.to_owned(),
|
||||
|
|
|
|||
Loading…
Reference in a new issue