Skip to content

Key the alert window on the kind of fault, and act on the review of the review - #59

Merged
baz8080 merged 1 commit into
mainfrom
claude/busy-volta-pbvvy5
Sep 24, 2026
Merged

baz8080 merged 1 commit into
mainfrom
claude/busy-volta-pbvvy5

Conversation

@baz8080

@baz8080 baz8080 commented Sep 24, 2026

Copy link
Copy Markdown
Owner

The last commit of #58, the one that acted on its review, was merged without a review of its own. Reviewing it found ten more issues, three of them caused by the alert dedup marker. The marker held one digest of the whole banner, including the raw error text. This PR fixes those. Its own changes were then reviewed twice before pushing: the first pass changed the approach (below), and the second found nothing.

Changes

  • Each kind of fault has its own 24h window. Before, a broken database and a flapping API took turns: the database banner and the unreachable banner alternated, and each change of banner was delivered again.
  • The kind is the exit code, plus a detail where a difference matters. Before, the same fault with a different raw error ('timed out', then 'connection refused') was never suppressed. Nor was lift check, which words the error with str() where the poll uses repr(). A rejected key's kind includes its masked key, so a second key that is also rejected still alerts. Keying on the banner's title was tried first and rejected in review for exactly that reason: it would have suppressed the alert for a newly captured key that was also refused.
  • fail() restarts the clean-run count on every failure. Before, a fault whose alert failed to deliver counted as a clean run.
  • The database banner finds a lock's holder with fuser. It used to blame lift rebuild, but a rebuild holds the poll lock, so a poll never sees its database lock.
  • backup-to-git.sh:
    • Exit 143 always alerts and names the likely cause: the timeout, or a stop or shutdown mid-backup.
    • on_exit ignores TERM, and the alert's curl inherits that, so the TERM systemd sends to every process in the unit cannot kill it.
    • INT now exits 130.
  • Tidy-ups: one marker reader instead of three copies, and _run classifies the fetch before opening the database instead of again inside the error handler.

notes/collector-review.md has a new section with all ten findings and the accepted trade-offs:

  • Schema root and schema drift share one window.
  • A failing lift check suppresses the poll's alert of the same kind.
  • A marker in the old format reads as empty, so the first failure after deploying can send one extra alert.

The one finding not acted on restates the replay-order trade-off already settled in #58.

Verification

  • ruff check and scripts/no-em-dash.sh are clean.

  • Full suite OK with LIFT_STATUS_DATA_DIR set (the 12 skips were already there). Collector suites pass on Python 3.11.

  • New tests cover:

    • the same kind of fault with a different raw error;
    • a second rejected key;
    • two faults at once;
    • an undelivered failure breaking the clean stretch;
    • the old marker format;
    • a failure between clean runs at the poll level.

    Those that exercise changed behaviour fail on main.

  • backup-to-git.sh, with its process group sent TERM, delivers the TERM alert in both cases:

    • while a git fetch hangs;
    • while an earlier alert's curl is still running.

Deploying

This changes the Pi collector, so re-run scripts/install-native.sh on the Pi after merging.

🤖 Generated with Claude Code

https://claude.ai/code/session_01AnrcmsrTHYCgnVGyqBqji4


Generated by Claude Code

…he review

The commit that acted on #58's review shipped without a review of its
own. One found ten more, three of them rooted in the dedup marker: it
held one digest of the whole banner, raw error included.

- Each kind of fault has its own window, so a broken database and a
  flapping API no longer take turns to alert.
- The kind is the exit code plus a detail where a difference is news, a
  rejected key's masked key, not the banner text: the same fault with a
  different raw error, or `lift check`'s str() wording, is one alert,
  and a second rejected key is still pushed. Keying on the banner title
  was tried first and rejected in review because it sat on a newly
  captured key that was also refused.
- fail() restarts the clean-run count on every failure, including one
  whose alert was not delivered.
- The database banner finds a lock's holder with fuser rather than
  blaming lift rebuild, which holds the poll lock and so never locks a
  poll out.
- backup-to-git.sh: 143 always alerts and names the timeout or a stop;
  on_exit ignores TERM so its curl survives the cgroup-wide TERM; INT is
  130.
- One marker reader, and the fetch is classified once in _run.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AnrcmsrTHYCgnVGyqBqji4
@baz8080
baz8080 merged commit 9cf049a into main Sep 24, 2026
3 checks passed
@baz8080
baz8080 deleted the claude/busy-volta-pbvvy5 branch September 24, 2026 11:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants