From 63304f7024cece9f9897b73fbab6771efb02beab Mon Sep 17 00:00:00 2001 From: Tomer Weller Date: Tue, 25 Aug 2026 05:38:35 +0000 Subject: [PATCH 1/2] =?UTF-8?q?Regression=20tests=20for=20#3824=20?= =?UTF-8?q?=E2=80=94=20recovery-stalled=20family-union=20re-key=20(fail=20?= =?UTF-8?q?on=20main)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds scripts/lib/test_eval_alarms_recovery_family.py and a catalog TAP assertion (Test 41c) in test-monitor-skill-snippets.sh. On origin/main these fail: eval_counter_streak has no `prev` param, post_restart_absolute_label is unhandled (family sum false-fires the absolute guard), validate_catalog does not reject a non-string label, render_aggregate has no reason breakdown, and the catalog stanza is still extraction=form2 + single-label selector. Refs #3824 Co-authored-by: Claude Code --- .../lib/test_eval_alarms_recovery_family.py | 346 ++++++++++++++++++ scripts/test-monitor-skill-snippets.sh | 17 +- 2 files changed, 362 insertions(+), 1 deletion(-) create mode 100644 scripts/lib/test_eval_alarms_recovery_family.py diff --git a/scripts/lib/test_eval_alarms_recovery_family.py b/scripts/lib/test_eval_alarms_recovery_family.py new file mode 100644 index 00000000..fa263023 --- /dev/null +++ b/scripts/lib/test_eval_alarms_recovery_family.py @@ -0,0 +1,346 @@ +#!/usr/bin/env python3 +"""Regression tests for the recovery-stalled family-union re-key (issue #3824). + +Check 12b (`recovery-stalled` counter-streak) historically observed a single +`reason` series (`forcing_catchup_behind`) of the 8-label +`henyey_recovery_stalled_tick_total` family. During an at-tip stall the node +takes the `forcing_catchup_not_behind` branch by construction, so the one label +the alarm watched was exactly the branch that could not move — a real recovery +episode incremented two UNCOVERED labels and the tick reported `ok (delta=0)`. + +The fix re-keys the delta/streak/burst trigger onto the SUM of all `reason` +series (`extraction = "form2-sum-all"`), while scoping the post-restart absolute +guard to a single historically-calibrated label via `post_restart_absolute_label` +(so a summed warmup value of ~113 does not false-fire the absolute check tuned +for `forcing_catchup_behind` alone). A per-reason `reason_breakdown` is attached +on breach/firing so a summed fire still names the moving labels. +""" + +import tempfile +from pathlib import Path + +# eval-alarms.py uses a hyphen, so we need importlib +import importlib.util + +_spec = importlib.util.spec_from_file_location( + "eval_alarms", + Path(__file__).parent / "eval-alarms.py", +) +_mod = importlib.util.module_from_spec(_spec) +_spec.loader.exec_module(_mod) + +read_snapshot = _mod.read_snapshot +write_snapshot = _mod.write_snapshot +eval_counter_streak = _mod.eval_counter_streak +render_aggregate = _mod.render_aggregate +validate_catalog = _mod.validate_catalog + + +def _make_alarm(name="recovery-stalled", **kwargs): + alarm = {"name": name, "kind": "counter-streak"} + alarm.update(kwargs) + return alarm + + +def _family(behind, not_behind, peer_scp): + """Build a current-metrics dict for the recovery family with 3 reasons set.""" + return { + "henyey_recovery_stalled_tick_total": [ + ({"reason": "forcing_catchup_behind"}, float(behind)), + ({"reason": "forcing_catchup_not_behind"}, float(not_behind)), + ({"reason": "near_tip_peer_scp_recovery"}, float(peer_scp)), + ] + } + + +# ── post-restart absolute guard is scoped to a single label, not the sum ────── + +def test_post_restart_absolute_uses_label_not_sum(): + """On a baseline reset, the post-restart absolute guard must evaluate only + the `post_restart_absolute_label` series, NOT the family sum. + + forcing_catchup_behind == 40 (< 50 threshold) while the family sums to 120 + (> 50). The correct behaviour is `collecting_baseline` (no post-restart + fire), because the historically-calibrated absolute signal (#3197/#3198) is + the single label, not the aggregate. + + Fails on origin/main: `post_restart_absolute_label` is unhandled, so under a + `form2-sum-all` extraction the aggregate 120 >= 50 fires as post-restart. + """ + with tempfile.TemporaryDirectory() as d: + state_dir = Path(d) + snap_path = state_dir / "recovery_family_streak_snapshot" + # Existing baseline under a DIFFERENT pid → next eval is a baseline reset. + write_snapshot(snap_path, { + "version": "1", + "pid": "111", + "start_ticks": "100", + "counter_value": "0", + "breach_streak": "0", + }) + + alarm = _make_alarm( + metric="henyey_recovery_stalled_tick_total", + extraction="form2-sum-all", + delta_threshold=1, streak_threshold=3, burst_threshold=10, + post_restart_absolute_threshold=50, + post_restart_absolute_label="forcing_catchup_behind", + snapshot_file="recovery_family_streak_snapshot", + severity="WARN", + ) + # behind=40 (<50), family sum = 40 + 50 + 30 = 120 (>50). + current = _family(behind=40, not_behind=50, peer_scp=30) + + result = eval_counter_streak( + alarm, current, state_dir, "222", "200", prev=None, + ) + + assert result["state"] == "collecting_baseline", ( + "post-restart guard must use the single label (40 < 50), not the " + f"family sum (120 >= 50); got state={result['state']}" + ) + + +def test_post_restart_absolute_label_fires_on_label_value(): + """Complementary case: when the scoped label itself crosses the threshold, + the post-restart absolute fire still triggers (guard not disabled).""" + with tempfile.TemporaryDirectory() as d: + state_dir = Path(d) + snap_path = state_dir / "recovery_family_streak_snapshot" + write_snapshot(snap_path, { + "version": "1", + "pid": "111", + "start_ticks": "100", + "counter_value": "0", + "breach_streak": "0", + }) + + alarm = _make_alarm( + metric="henyey_recovery_stalled_tick_total", + extraction="form2-sum-all", + delta_threshold=1, streak_threshold=3, burst_threshold=10, + post_restart_absolute_threshold=50, + post_restart_absolute_label="forcing_catchup_behind", + snapshot_file="recovery_family_streak_snapshot", + severity="WARN", + ) + # behind=63 (>= 50) → post-restart fire on the scoped label. + current = _family(behind=63, not_behind=1, peer_scp=49) + + result = eval_counter_streak( + alarm, current, state_dir, "222", "200", prev=None, + ) + + assert result["state"] == "firing", ( + f"scoped label 63 >= 50 must post-restart fire, got {result['state']}" + ) + assert result.get("post_restart") is True + # The fresh baseline snapshot stores the SUM (the streak machine's unit). + snap = read_snapshot(snap_path) + assert snap["counter_value"] == "113", ( + f"baseline must snapshot the family sum 113, got {snap['counter_value']}" + ) + + +def test_snapshot_file_rename_rebaselines_without_post_restart_fire(): + """Baseline migration via snapshot_file rename (#3222 lever): when only the + OLD snapshot filename is present, the first post-migration tick re-collects + a fresh baseline (`collecting_baseline`) — NOT a post-restart fire — even + when the scoped label already exceeds the absolute threshold. + + The empty-snapshot first-tick branch does not invoke the post-restart path, + so renaming the file avoids the spurious post-restart fire a version bump + would cause on a long-running process. + """ + with tempfile.TemporaryDirectory() as d: + state_dir = Path(d) + # Only the OLD filename exists; the new path is absent. + old_path = state_dir / "counter_streak_snapshot" + write_snapshot(old_path, { + "version": "1", + "pid": "222", + "start_ticks": "200", + "counter_value": "500", + "breach_streak": "3", + }) + + alarm = _make_alarm( + metric="henyey_recovery_stalled_tick_total", + extraction="form2-sum-all", + delta_threshold=1, streak_threshold=3, burst_threshold=10, + post_restart_absolute_threshold=50, + post_restart_absolute_label="forcing_catchup_behind", + snapshot_file="recovery_family_streak_snapshot", + severity="WARN", + ) + # Same pid/start_ticks as the old snapshot; scoped label >= 50. + current = _family(behind=63, not_behind=1, peer_scp=50) + + result = eval_counter_streak( + alarm, current, state_dir, "222", "200", prev=None, + ) + + assert result["state"] == "collecting_baseline", ( + "rename must re-baseline cleanly (empty new-path snapshot), not " + f"post-restart fire; got {result['state']}" + ) + # New-path baseline written with the family sum. + snap = read_snapshot(state_dir / "recovery_family_streak_snapshot") + assert snap["counter_value"] == "114" + + +# ── the trigger fires on the family sum (the branch that actually moves) ─────── + +def test_at_tip_stall_fires_on_family_sum(): + """An at-tip stall increments `not_behind` + `near_tip_peer_scp_recovery` + while `forcing_catchup_behind` barely moves. Summing the family makes the + burst trigger observe the branch that moves: delta 161 >= burst 10 → fire. + """ + with tempfile.TemporaryDirectory() as d: + state_dir = Path(d) + snap_path = state_dir / "recovery_family_streak_snapshot" + # Baseline sum = 63 + 1 + 50 = 114. + write_snapshot(snap_path, { + "version": "1", + "pid": "222", + "start_ticks": "200", + "counter_value": "114", + "breach_streak": "0", + }) + + alarm = _make_alarm( + metric="henyey_recovery_stalled_tick_total", + extraction="form2-sum-all", + delta_threshold=1, streak_threshold=3, burst_threshold=10, + post_restart_absolute_threshold=50, + post_restart_absolute_label="forcing_catchup_behind", + snapshot_file="recovery_family_streak_snapshot", + severity="WARN", + ) + prev = _family(behind=63, not_behind=1, peer_scp=50) + # not_behind 1→81, peer_scp 50→130, behind 63→64 → sum 275, delta 161. + current = _family(behind=64, not_behind=81, peer_scp=130) + + result = eval_counter_streak( + alarm, current, state_dir, "222", "200", prev=prev, + ) + + assert result["state"] == "firing", ( + f"family sum delta 161 >= burst 10 must fire, got {result['state']}" + ) + assert result["value"] == 161, f"expected delta 161, got {result['value']}" + + +def test_reason_breakdown_names_moving_labels(): + """On a burst fire, the result carries `reason_breakdown` naming the moved + reasons with their deltas and omitting flat ones.""" + with tempfile.TemporaryDirectory() as d: + state_dir = Path(d) + snap_path = state_dir / "recovery_family_streak_snapshot" + write_snapshot(snap_path, { + "version": "1", + "pid": "222", + "start_ticks": "200", + "counter_value": "114", + "breach_streak": "0", + }) + + alarm = _make_alarm( + metric="henyey_recovery_stalled_tick_total", + extraction="form2-sum-all", + delta_threshold=1, streak_threshold=3, burst_threshold=10, + post_restart_absolute_threshold=50, + post_restart_absolute_label="forcing_catchup_behind", + snapshot_file="recovery_family_streak_snapshot", + severity="WARN", + ) + prev = _family(behind=63, not_behind=1, peer_scp=50) + current = _family(behind=63, not_behind=81, peer_scp=130) + + result = eval_counter_streak( + alarm, current, state_dir, "222", "200", prev=prev, + ) + + assert result["state"] == "firing" + breakdown = result.get("reason_breakdown") + assert breakdown, f"expected reason_breakdown, got {breakdown!r}" + moved = {b["reason"]: b["delta"] for b in breakdown} + assert moved.get("forcing_catchup_not_behind") == 80, moved + assert moved.get("near_tip_peer_scp_recovery") == 80, moved + # forcing_catchup_behind was flat (63→63) — omitted. + assert "forcing_catchup_behind" not in moved, moved + + +def test_render_breakdown_appended_to_line(): + """render_aggregate appends the per-reason breakdown to the recovery_stalled + line so a summed fire names the moving labels.""" + r = { + "contributes_to": "recovery_stalled", + "state": "firing", + "value": 160, + "post_restart": False, + "reason_breakdown": [ + {"reason": "forcing_catchup_not_behind", "delta": 80}, + {"reason": "near_tip_peer_scp_recovery", "delta": 80}, + ], + } + out = render_aggregate([r], watcher_mode=False) + line = out["recovery_stalled_line"] + assert "delta=160" in line and "(burst)" in line, line + assert "forcing_catchup_not_behind+80" in line, line + assert "near_tip_peer_scp_recovery+80" in line, line + + +# ── catalog validation ──────────────────────────────────────────────────────── + +def test_validate_catalog_rejects_non_string_post_restart_label(): + """A non-string post_restart_absolute_label is a schema error.""" + catalog = { + "schema_version": _mod.SCHEMA_VERSION, + "alarm": [{ + "name": "recovery-stalled", + "kind": "counter-streak", + "metric": "henyey_recovery_stalled_tick_total", + "severity": "WARN", + "delta_threshold": 1, + "streak_threshold": 3, + "burst_threshold": 10, + "post_restart_absolute_label": 123, # not a string + }], + } + errors = validate_catalog(catalog) + assert any("post_restart_absolute_label" in e for e in errors), ( + f"expected a post_restart_absolute_label type error, got {errors}" + ) + + +def test_validate_catalog_accepts_string_post_restart_label(): + catalog = { + "schema_version": _mod.SCHEMA_VERSION, + "alarm": [{ + "name": "recovery-stalled", + "kind": "counter-streak", + "metric": "henyey_recovery_stalled_tick_total", + "severity": "WARN", + "delta_threshold": 1, + "streak_threshold": 3, + "burst_threshold": 10, + "post_restart_absolute_label": "forcing_catchup_behind", + }], + } + errors = validate_catalog(catalog) + assert not any("post_restart_absolute_label" in e for e in errors), errors + + +if __name__ == "__main__": + import sys + fns = [v for k, v in sorted(globals().items()) if k.startswith("test_") and callable(v)] + failed = 0 + for fn in fns: + try: + fn() + print(f"ok - {fn.__name__}") + except Exception as e: # noqa: BLE001 + failed += 1 + print(f"not ok - {fn.__name__}: {e}") + sys.exit(1 if failed else 0) diff --git a/scripts/test-monitor-skill-snippets.sh b/scripts/test-monitor-skill-snippets.sh index 9f23dd18..d42efddf 100755 --- a/scripts/test-monitor-skill-snippets.sh +++ b/scripts/test-monitor-skill-snippets.sh @@ -55,7 +55,7 @@ cleanup() { trap cleanup EXIT # ── TAP state ──────────────────────────────────────────────────────────────── -TAP_PLAN=478 +TAP_PLAN=479 TAP_CURRENT=0 TAP_FAILURES=0 @@ -1604,6 +1604,7 @@ run_tests() { # Test 41: TOML catalog file exists, is parseable, and contains recovery-stalled alarm # Extract recovery-stalled alarm constants from metric-alarms.toml local streak_val burst_val delta_val post_restart_val snapshot_file mode_val metric_name metric_label + local extraction_val post_restart_label_val # Use Python to extract the recovery-stalled alarm entry from the TOML local toml_extract toml_extract=$(python3 - "$constants_file" <<'PYEOF' @@ -1620,6 +1621,8 @@ for a in data['alarm']: print('burst_val=' + str(a['burst_threshold'])) print('delta_val=' + str(a['delta_threshold'])) print('post_restart_val=' + str(a.get('post_restart_absolute_threshold', 0))) + print('extraction_val=' + str(a.get('extraction', ''))) + print('post_restart_label_val=' + str(a.get('post_restart_absolute_label', ''))) print('snapshot_file=' + a['snapshot_file']) print('metric_name=' + a['metric']) labels = a.get('labels', []) @@ -1646,6 +1649,18 @@ PYEOF tap_not_ok "metric-alarms: recovery-stalled post_restart_absolute_threshold == 50" \ "expected post_restart_absolute_threshold=50, got '$post_restart_val'" fi + # Test 41c: family-union re-key (#3824). The delta/streak/burst trigger must + # observe the SUM of all 8 `reason` series (extraction=form2-sum-all), and + # the post-restart absolute guard must be scoped to a single historically + # calibrated label (post_restart_absolute_label=forcing_catchup_behind) so a + # summed warmup value (~113) does not false-fire the absolute check. + if [[ "$extraction_val" == "form2-sum-all" \ + && "$post_restart_label_val" == "forcing_catchup_behind" ]]; then + tap_ok "metric-alarms: recovery-stalled family-union (form2-sum-all + post_restart_absolute_label)" + else + tap_not_ok "metric-alarms: recovery-stalled family-union (form2-sum-all + post_restart_absolute_label)" \ + "expected extraction=form2-sum-all and post_restart_absolute_label=forcing_catchup_behind, got extraction='$extraction_val' label='$post_restart_label_val'" + fi else tap_not_ok "metric-alarms: TOML exists and parseable" \ "Failed to parse recovery-stalled alarm from metric-alarms.toml" From b43a243e251802520c8f33472820f98b638550d0 Mon Sep 17 00:00:00 2001 From: Tomer Weller Date: Tue, 25 Aug 2026 05:56:29 +0000 Subject: [PATCH 2/2] Re-key recovery-stalled alarm to the family sum of all reason series MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Check 12b (recovery-stalled counter-streak) keyed only on henyey_recovery_stalled_tick_total{reason="forcing_catchup_behind"}. During an at-tip stall the node takes the forcing_catchup_not_behind branch by construction, so the one label the alarm watched was exactly the branch that could not move — a real recovery episode incremented two uncovered labels and the tick reported ok (delta=0). Re-key the delta/streak/burst trigger onto the SUM of every reason series (extraction = form2-sum-all). Scope the post-restart absolute guard to a single calibrated label via a new post_restart_absolute_label field, so the summed warmup value (~113) does not false-fire the #3197/#3198 absolute threshold tuned for forcing_catchup_behind alone. Attach a per-reason breakdown on breach/firing so a summed fire names the moving labels, on the status line and in filing details. Rename the snapshot file to force a clean re-baseline on deploy (empty new path → collecting_baseline, no spurious post-restart fire), bump baseline_version 3→4, and generalize the cooldown/filing identity to the family. Streak-3 / burst-10 gating still absorbs transient single-tick blips (including the deliberately-non-alarming near_tip_gap1_suppressed / near_tip_park_inflated suppression counters the node now also exports), so #3728's false-positive class does not return. Refs #3824 Co-authored-by: Claude Code --- .agents/skills/monitor-loop/SKILL.md | 6 +- .agents/skills/monitor-tick/SKILL.md | 59 ++++++---- .agents/skills/shared/metric-alarms.toml | 39 +++++-- .claude/skills/monitor-loop/SKILL.md | 6 +- .claude/skills/monitor-tick/SKILL.md | 100 +++++++++++------ .claude/skills/shared/metric-alarms.toml | 39 +++++-- .gitignore | 1 + scripts/ci/check-alarm-versions.py | 2 +- scripts/lib/eval-alarms.py | 131 +++++++++++++++++++++-- scripts/test-monitor-skill-snippets.sh | 45 ++++---- 10 files changed, 318 insertions(+), 110 deletions(-) diff --git a/.agents/skills/monitor-loop/SKILL.md b/.agents/skills/monitor-loop/SKILL.md index a4be555b..7766537a 100644 --- a/.agents/skills/monitor-loop/SKILL.md +++ b/.agents/skills/monitor-loop/SKILL.md @@ -245,11 +245,11 @@ table is the human reference. These counter-based checks use streak gating rather than immediate-fire thresholds because single-tick increments are often transient self-recovering events that don't warrant operator attention (see #2309). State is tracked independently of -the ratio checks in a separate snapshot (`metrics/counter_streak_snapshot`). +the ratio checks in a separate snapshot (`metrics/recovery_family_streak_snapshot`). | Metric | Delta threshold | Streak threshold | Burst threshold | Severity | Rationale | |--------|-----------------|------------------|-----------------|----------|-----------| -| `henyey_recovery_stalled_tick_total{reason="forcing_catchup_behind"}` | ≥ 1 | 3 ticks | ≥ 10 | WARN | Recovery forced catchup while behind consensus; single occurrences are transient self-recovering events (see #2309); large bursts indicate sustained stalling | +| `henyey_recovery_stalled_tick_total` (sum of all `reason` series, #3824) | ≥ 1 | 3 ticks | ≥ 10 | WARN | Recovery stalled — the delta/streak/burst trigger observes the SUM of every `reason` series (an at-tip stall moves `forcing_catchup_not_behind`/`near_tip_peer_scp_recovery`, not `forcing_catchup_behind`); single-tick blips are transient self-recovering events (see #2309/#3728) absorbed by streak-3 gating; large bursts indicate sustained stalling. Post-restart absolute guard stays scoped to `forcing_catchup_behind` via `post_restart_absolute_label` | **D. Ratio checks — fire on sustained ratio breach (3 consecutive ticks)** @@ -296,7 +296,7 @@ pending_breach_streak= ``` Invalidate on PID/start_ticks change, malformed snapshot, or counter reset (current < previous). -**Counter-streak snapshot** persisted at `~/data//metrics/counter_streak_snapshot` +**Counter-streak snapshot** persisted at `~/data//metrics/recovery_family_streak_snapshot` (format and invalidation rules defined in Check 12b of monitor-tick/SKILL.md; path canonicalized in [`shared/metric-alarms.toml`](../shared/metric-alarms.toml)). Separate from ratio snapshot — runs independently of ratio skip conditions (see diff --git a/.agents/skills/monitor-tick/SKILL.md b/.agents/skills/monitor-tick/SKILL.md index 4db59a53..2c6ce634 100644 --- a/.agents/skills/monitor-tick/SKILL.md +++ b/.agents/skills/monitor-tick/SKILL.md @@ -121,7 +121,7 @@ All files below live in `/home/tomer/data/$MONITOR_SESSION_ID/`: | `metrics/prev.prom` | previous Prometheus scrape | check 12 | | `metrics/scrape_identity` | process identity of the scrape now in prev.prom | check 12 | | `metrics/ratio_snapshot` | counter-ratio history (check 12) | check 12 | -| `metrics/counter_streak_snapshot` | counter-streak state (check 12b) | check 12b ([metric-alarms](../shared/metric-alarms.toml)) | +| `metrics/recovery_family_streak_snapshot` | counter-streak state (check 12b; #3824 family-union) | check 12b ([metric-alarms](../shared/metric-alarms.toml)) | | `metrics/anomaly_cooldown.json` | alert dedup state | check 9 | | `metrics/archive/` | Per-tick snapshot dirs (current.prom + prev.prom + metadata.env), rolling 500, atomic write | check 12 | | `logs/monitor.log` | node stdout/stderr (rotated on restart) | node process | @@ -1119,7 +1119,7 @@ against a node that is in real-time sync with age=2s). breach on the next tick before firing. - All other §GAUGES are unaffected — they are point-in-time readings from `current.prom` only. - - Do NOT skip ratio_snapshot or counter_streak_snapshot checks — they have + - Do NOT skip ratio_snapshot or recovery_family_streak_snapshot checks — they have their own independent PID/start_ticks invalidation logic and snapshot files. Independence is safe: each check reads PID/start_ticks from `/proc` and compares against its own snapshot. @@ -1357,12 +1357,25 @@ eval_result=$(python3 scripts/lib/eval-alarms.py \ > This section is authoritative for the state machine *logic*; inline literals > are cross-validated against the TOML by `scripts/test-monitor-skill-snippets.sh`. -This check tracks `henyey_recovery_stalled_tick_total{reason="forcing_catchup_behind"}` -using a streak-gated alert, independent of Check 12's ratio checks. It runs on -its own state machine because ratio checks are globally skipped during unsync +This check tracks the **sum of every `reason` series** of +`henyey_recovery_stalled_tick_total` (family-union, `extraction = "form2-sum-all"`, +#3824) using a streak-gated alert, independent of Check 12's ratio checks. It runs +on its own state machine because ratio checks are globally skipped during unsync states (ledger age > 30s, gap > 5, etc.), but the recovery-stalled counter fires precisely during recovery transitions when the node is briefly unsynced. +> **Why the family sum, not a single label (#3824):** Check 12b previously keyed +> only on `{reason="forcing_catchup_behind"}`. During an *at-tip* stall the node +> takes the `forcing_catchup_not_behind` branch by construction, so the one label +> the alarm watched was exactly the branch that could not move — a real recovery +> episode incremented two uncovered labels and the tick reported `ok (delta=0)`. +> Summing the whole family makes the trigger observe whichever branch moves and +> covers any future `reason`. On a fire a per-reason breakdown is appended +> (`[by reason: