Re-key recovery-stalled alarm to the family sum of all reason series

[tomerweller]

Closes #3824

Summary

Check 12b (the recovery-stalled counter-streak) watched a single reason
series — henyey_recovery_stalled_tick_total{reason="forcing_catchup_behind"} —
of the node's recovery-reason 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 (forcing_catchup_not_behind,
near_tip_peer_scp_recovery) and the tick reported ok (delta=0).

This re-keys the delta/streak/burst trigger onto the sum of every reason
series (extraction = "form2-sum-all"), so the trigger observes whichever
branch actually moves and automatically covers any future reason. The
post-restart absolute guard (#3197/#3198) is scoped to a single calibrated label
via a new post_restart_absolute_label = "forcing_catchup_behind" field — the
summed family legitimately reaches ~113 during warmup and would otherwise
false-fire the absolute threshold on every restart. A per-reason breakdown is
attached on breach/firing ([by reason: <label>+<delta>, …]) so a summed fire
still names the moving labels, on the status line and in filing details.

Baseline migration uses the #3222 lever — the snapshot file is renamed
(counter_streak_snapshot → recovery_family_streak_snapshot) so the old
baseline is absent at the new path and the first post-migration tick re-collects
cleanly (no spurious post-restart fire). baseline_version bumped 3→4; cooldown
and filing identity generalized to the family.

Plan reference

Converged Plan comment

Test plan

• python3 -m pytest scripts/lib/ scripts/ci/ — 161 passed (incl. new test_eval_alarms_recovery_family.py)
• bash scripts/test-monitor-skill-snippets.sh — all TAP cases pass
• scripts/ci/check-alarm-versions.py old→new — semantic changes covered by the baseline_version bump
• catalog --validate-only passes; .claude/.agents catalog byte-identical
• No Rust files changed → cargo fmt/clippy N/A

Regression test (bug-fix)

• Tests: scripts/lib/test_eval_alarms_recovery_family.py + catalog TAP assertion (Test 41c) in scripts/test-monitor-skill-snippets.sh
• Pre-fix: committed as 63304f7 — verified FAILED on origin/main (eval_counter_streak has no prev param; post_restart_absolute_label unhandled so the family sum false-fires the absolute guard; validate_catalog does not reject a non-string label; no reason breakdown; catalog still extraction=form2 + single-label selector)
• Post-fix: verified PASSES after b43a243

Deviations from plan

• The plan described "all 8 reason series". The node (crates/app/src/metrics.rs) now exports 10 reasons, two of which (near_tip_gap1_suppressed, near_tip_park_inflated) are deliberately non-alarming suppression counters. form2-sum-all sums them too. This is safe and intended: streak-3 / burst-10 gating absorbs the transient single-tick blips those counters represent, while a node genuinely parked at gap==1 (steadily climbing suppressed counter, flat ledger) is exactly the "worth manual investigation" condition the prior docs flagged — now surfaced on the recovery_stalled: line. metrics: henyey_recovery_stalled_tick_total{reason="forcing_catchup_behind"} — sustained breach #3728's false-positive class does not return. Documented in the Check 12b metrics: henyey_recovery_stalled_tick_total{reason="forcing_catchup_behind"} — sustained breach #3728 note.
• Applied the same load-bearing doc edits to the .agents/ Codex mirrors (snapshot filename, family-union table row/description, scoped post-restart guard, cooldown/filing identity), since both runtimes share the one catalog whose snapshot_file changed — leaving the mirror stale would make the Codex runtime write the wrong snapshot path.
• Added /metrics/recovery_family_streak_snapshot to .gitignore (and its stray-name test list) alongside the retained old entry.
• Out of scope per plan (not included): a new henyey_recovery_tx_set_stuck_seconds alarm; a per-label independent-threshold model (superseded by the union approach).

🤖 Generated with Claude Code

[tomerweller]

🔍 Reviewer: Correctness

Verdict: APPROVE

Summary: Re-keys the recovery-stalled counter-streak from a single reason label to the family sum (form2-sum-all), scopes the post-restart absolute guard to a calibrated label, and adds per-reason attribution. Cycle 1 — complete review; the bug-fix has a real regression test that provably failed pre-fix and passes post-fix.

Full review

Bug-fix test gate (kind:bug): PASS.

• scripts/lib/test_eval_alarms_recovery_family.py (8 tests) + catalog TAP Test 41c reproduce the blind spot: the pre-fix commit 63304f7 is documented FAILED on origin/main (eval_counter_streak has no prev param, post_restart_absolute_label unhandled, validate_catalog accepts a non-string label, no reason breakdown). I ran the suite on the PR head merged with current origin/main: 138 passed (pytest scripts/lib/), plus scripts/ci/ 23 passed. Catalog --validate-only → valid: true, alarm_count: 37.

New public surface — all covered:

• post_restart_label_value — covered by test_post_restart_absolute_uses_label_not_sum (label 40<50, sum 120≥50 ⇒ collecting_baseline) and test_post_restart_absolute_label_fires_on_label_value (label 63≥50 ⇒ fire, snapshot stores the sum 113).
• compute_reason_breakdown — test_reason_breakdown_names_moving_labels (moved labels named, flat label omitted); returns [] when prev is None (no-prev tick safe).
• format_reason_breakdown / render suffix — test_render_breakdown_appended_to_line.
• validate_catalog non-string-label rejection — test_validate_catalog_rejects_non_string_post_restart_label.

Logic consistency: the streak baseline snapshots the family sum (cur_val) while maybe_post_restart_fire receives the scoped-label value via post_restart_label_value — verified consistent in both reset branches (empty-snapshot first tick and cur_val < prev_counter). form2-sum-all is a pre-existing extract_value handler (line 201), so labels = [] + sum extraction is correct; main() now threads prev=prev into eval_counter_streak. Error handling is sound: missing scoped series ⇒ 0.0 ⇒ no fire; missing family ⇒ documented skip condition.

Non-blocking (advisory, already documented in code comments): the per-reason breakdown deltas are computed against prev.prom while the headline delta= is snapshot-based; if prev.prom is stale/absent the sum of per-reason deltas can differ from the aggregate delta shown. This is explicitly scoped as advisory attribution and returns [] when prev is unavailable, so it never misreports the trigger — acceptable as-is.

No new-class concerns. APPROVE.

[tomerweller]

🔍 Reviewer: Risk

Verdict: APPROVE

Summary: Monitoring-only change (Check 12b alarm re-key + eval-alarms.py machinery). No observable/interop surface is touched, so this is out of parity scope. Baseline migration, versioning, and dual-catalog identity are all handled correctly. Cycle 1 — complete review.

Full review

Parity scope: None. Changed files are .claude/.agents monitor skills, shared/metric-alarms.toml, scripts/lib/eval-alarms.py, scripts/ci/check-alarm-versions.py, tests, .gitignore. No ledger/bucket hashes, tx result/meta XDR, SCP/overlay wire bytes, history format, or HTTP/RPC/CLI contracts. Per docs/PARITY.md this is internal monitoring — explicitly in the free-to-deviate tier.

Migration / operational risk — handled:

• Snapshot file renamed counter_streak_snapshot → recovery_family_streak_snapshot (the monitor-tick: counter_streak_snapshot path-doubling + check-12b format drift #3222 lever): old baseline absent at new path ⇒ first post-deploy tick re-collects cleanly, no spurious post-restart fire. Old file orphaned harmlessly — confirmed no other catalog alarm consumes counter_streak_snapshot (only a comment reference remains). test_snapshot_file_rename_rebaselines_without_post_restart_fire locks this in.
• baseline_version 3→4 and semantic_change_date bumped; check-alarm-versions.py allowlist extended with post_restart_absolute_label so the new field is a recognized semantic change, not an unversioned drift. scripts/ci/ self-tests green.
• .gitignore retains the old entry and adds the new snapshot path.

Catalog integrity: .claude and .agents copies are byte-identical (diff clean) and both validate (valid: true, 37 alarms). The .agents Codex mirror doc edits (snapshot filename, family-union row, scoped guard, cooldown/filing identity) are necessary since both runtimes share the one catalog whose snapshot_file changed.

Behavioral risk — summing suppression counters (#3728 regression class): the family sum now includes the deliberately-non-alarming near_tip_gap1_suppressed / near_tip_park_inflated series. Mitigated: streak-3 / burst-10 gating absorbs transient single-tick blips (a momentary gap==1 self-heals before a 3-tick streak and is far below a +10 burst), while a genuinely parked node with a steadily-climbing suppressed counter and flat ledger is exactly the condition worth surfacing. Documented in the Check 12b #3728 note. False-positive class does not return.

No security or data-format exposure. APPROVE.

[tomerweller]

✅ Merged

Commit: 854e23b

Follow-up issues filed for unaddressed inline review comments: none (no unresolved inline threads)