refactor(#2756): declare the terminal hook instead of inferring it
`Template` was a DAG-level enum that three different things read back out: `terminal_hook()` mapped it to a side effect, the retention pass bucketed history by it, and a tracing field printed it. None of those needed a *label* — they needed the two facts the label happened to encode. So the enum was a lossy stand-in for intent, and every new DAG shape had to pick the variant whose inferred behaviour matched, whether or not the name fit (`reparent` rode `MetaUpdate` for exactly this reason, with a 10-line comment apologising for it). Replace the inference with a declaration: `DagSpec.hook: Option<HookKind>`. Only the builder assembling a DAG knows why it did so, so only the builder can say what should happen when it settles. `run_terminal_hook` becomes a field read, and `reparent`'s apology becomes `hook: None`. Hook assignment is byte-identical to the old precedence rule (`approval_id.is_some()` wins, then `Rebuild | PermChange`), checked site by site; `meta_update` is the only builder with a variable approval id and so the only remaining conditional. Retention loses the per-template bucket with the enum that keyed it. The dashboard renders one recent-builds list, so one flat newest-first cap (`MAX_HISTORY_DAGS`) bounds it. `HISTORY_GRACE_SECS` goes too — it existed to stop a burst of same-template DAGs evicting each other inside one poll interval, which is not a failure mode a flat cap has. That takes `snapshot_capped()` and the `snapshot_no_grace()` test hook with it. The queue is runtime-only (empty graph on boot), so the serde changes carry no migration risk.
This commit is contained in:
parent
a8728ac532
commit
ca7146e4f0
8 changed files with 113 additions and 241 deletions
|
|
@ -300,13 +300,7 @@ fn lease_serializes_two_lifecycle_dags_for_same_agent() {
|
|||
let restart = submit(&q, restart_online(&["agent-a"], false, "restart"));
|
||||
let stop = submit(
|
||||
&q,
|
||||
templates::reconcile_only(
|
||||
Template::Stop,
|
||||
"agent-a",
|
||||
Source::Manual,
|
||||
"stop".to_owned(),
|
||||
None,
|
||||
),
|
||||
templates::reconcile_only("agent-a", Source::Manual, "stop".to_owned(), None),
|
||||
);
|
||||
// Restart's first node (StopForUpdate) takes the lease; stop's
|
||||
// Reconcile must wait even though slots are free.
|
||||
|
|
@ -337,13 +331,7 @@ fn lease_exempt_prebuild_overlaps_other_dag_on_same_agent() {
|
|||
submit(&q, rebuild("agent-a", "rebuild"));
|
||||
let stop = submit(
|
||||
&q,
|
||||
templates::reconcile_only(
|
||||
Template::Stop,
|
||||
"agent-a",
|
||||
Source::Manual,
|
||||
"stop".to_owned(),
|
||||
None,
|
||||
),
|
||||
templates::reconcile_only("agent-a", Source::Manual, "stop".to_owned(), None),
|
||||
);
|
||||
// Both DAGs' heads are lease-independent of each other: the rebuild's
|
||||
// MetaSync (meta window) and the stop's Reconcile (agent lease).
|
||||
|
|
@ -603,7 +591,7 @@ fn append_subgraph_roots_on_emitter_and_rebases_local_deps() {
|
|||
// the emitter and its LOCAL 0-based deps are rebased onto the DAG.
|
||||
let q = JobQueue::new(4);
|
||||
let spec = DagSpec {
|
||||
template: Template::Boot,
|
||||
hook: None,
|
||||
source: Source::AutoUpdate,
|
||||
reason: "sweep".to_owned(),
|
||||
approval_id: None,
|
||||
|
|
@ -831,13 +819,7 @@ fn failed_reconcile_marks_dag_failed() {
|
|||
let q = JobQueue::new(1);
|
||||
let id = submit(
|
||||
&q,
|
||||
templates::reconcile_only(
|
||||
Template::Start,
|
||||
"agent-a",
|
||||
Source::Manual,
|
||||
"start".to_owned(),
|
||||
None,
|
||||
),
|
||||
templates::reconcile_only("agent-a", Source::Manual, "start".to_owned(), None),
|
||||
);
|
||||
let c = claim_one(&q);
|
||||
q.complete_node(id, c.node_id, Err("start failed".to_owned()));
|
||||
|
|
@ -917,8 +899,7 @@ fn cancelled_power_op_fires_no_hook() {
|
|||
let summary = q.cancel(id).expect("cancelled while queued");
|
||||
assert_eq!(summary.state, State::Cancelled);
|
||||
assert_eq!(
|
||||
terminal_hook(summary.template, summary.approval_id),
|
||||
None,
|
||||
summary.hook, None,
|
||||
"cancelled {name} (graceful={graceful}, running={running}) must \
|
||||
fire no hook — no node of it ever ran"
|
||||
);
|
||||
|
|
@ -951,13 +932,7 @@ fn dag_settles_terminal_and_releases_lease_after_work() {
|
|||
// immediately.
|
||||
let next = submit(
|
||||
&q,
|
||||
templates::reconcile_only(
|
||||
Template::Stop,
|
||||
"agent-a",
|
||||
Source::Manual,
|
||||
"stop".to_owned(),
|
||||
None,
|
||||
),
|
||||
templates::reconcile_only("agent-a", Source::Manual, "stop".to_owned(), None),
|
||||
);
|
||||
let c = claim_one(&q);
|
||||
assert_eq!(c.dag_id, next);
|
||||
|
|
@ -1220,14 +1195,20 @@ fn set_build_log_id_links_running_node() {
|
|||
);
|
||||
}
|
||||
|
||||
/// History retention is a **flat** newest-first cap over all terminal DAGs
|
||||
/// (`MAX_HISTORY_DAGS`), not a per-template bucket behind a grace window.
|
||||
/// The dashboard renders one recent-builds list, so one number bounds it —
|
||||
/// and with no bucketing there's nothing for a burst of same-shaped DAGs to
|
||||
/// evict early, which is what the grace window used to paper over.
|
||||
#[test]
|
||||
fn history_evicts_old_terminals_per_template() {
|
||||
fn history_evicts_oldest_terminals_past_flat_cap() {
|
||||
const OVERFLOW: usize = 8;
|
||||
let q = JobQueue::new(1);
|
||||
for i in 0..8 {
|
||||
let mut ids = Vec::new();
|
||||
for i in 0..(MAX_HISTORY_DAGS + OVERFLOW) {
|
||||
let id = submit(
|
||||
&q,
|
||||
templates::reconcile_only(
|
||||
Template::Start,
|
||||
&format!("agent-{i}"),
|
||||
Source::Manual,
|
||||
"start".to_owned(),
|
||||
|
|
@ -1241,17 +1222,19 @@ fn history_evicts_old_terminals_per_template() {
|
|||
// node rolls the container up terminal (its inline hook fires off the
|
||||
// returned summary — no terminal-hook node).
|
||||
q.complete_node(id, c.node_id, Err("boom".to_owned()));
|
||||
ids.push(id);
|
||||
}
|
||||
let kept: std::collections::HashSet<u64> = q.snapshot().iter().map(|d| d.id).collect();
|
||||
assert_eq!(kept.len(), MAX_HISTORY_DAGS, "flat history cap");
|
||||
// Newest-first: the oldest `OVERFLOW` fall off, everything after survives.
|
||||
// These DAGs settle within the same wall-clock second, so this also pins
|
||||
// the `NodeId`-descending tiebreak that orders them when `finished_at` ties.
|
||||
for old in &ids[..OVERFLOW] {
|
||||
assert!(!kept.contains(old), "oldest terminal {old} evicted");
|
||||
}
|
||||
for recent in &ids[OVERFLOW..] {
|
||||
assert!(kept.contains(recent), "recent terminal {recent} retained");
|
||||
}
|
||||
// Fresh terminals are inside the grace window: nothing evicts yet,
|
||||
// so a ~1s QueueDag poller can still observe every terminal state
|
||||
// (a broad stop/start settles many same-template DAGs at once).
|
||||
assert_eq!(
|
||||
q.snapshot().len(),
|
||||
8,
|
||||
"grace window protects fresh terminals"
|
||||
);
|
||||
// Past the grace window the per-template cap applies.
|
||||
assert_eq!(q.snapshot_no_grace().len(), 5, "per-template history cap");
|
||||
assert_eq!(q.live_count(), 0);
|
||||
}
|
||||
|
||||
|
|
|
|||
Loading…
Reference in a new issue