Skip to content

Fix what a retroactive review found in alerting, the scripts and the units - #54

Merged
baz8080 merged 18 commits into
mainfrom
claude/eager-sagan-wn68lr
Sep 24, 2026
Merged

baz8080 merged 18 commits into
mainfrom
claude/eager-sagan-wn68lr

Conversation

@baz8080

@baz8080 baz8080 commented Sep 24, 2026 •

Copy link
Copy Markdown
Owner

Slice 4 of the retroactive review, following #48, #51, #52 and #53. It covers a whole-file review of esb_outages/alert.py, esb_outages/__main__.py, the three shell scripts and the four systemd units.

The history is 18 commits, one per fix. The follow-ups from several review rounds of this PR are folded into the fix they belong to. Every commit passes lint and the full test suite on its own.

The common thread: several failures on the Pi ended silently, or with only the dead-man's monitor noticing hours later with no cause.

Collector

  1. Alerts are delivered best effort and never print their URLs.
    • A webhook URL missing its scheme used to crash the run it was reporting on.
    • A failed delivery's warning quoted the URL, or its path, which is the secret. It now names only the kind of error.
  2. A run that fails partway now alerts.
    • The pre-run probe passes on a full disk.
    • An OSError, or a SQLite error whose code means disk or permissions, is the storage alert (exit 6).
    • Anything else is a new crash alert (exit 1). It names esb rebuild, and says that a rebuild failing the same way, or the next run crashing again, means the code needs a fix.
    • Neither pings.
    • Recorded in notes/alerting.md, and the README's exit table gains exit 1.
  3. A negative or non-numeric poll delay is refused, with one parser shared by the flag and the environment.
  4. An overlapping run that removed the .write-test probe no longer raises a false storage alarm.
  5. Only a held lock reads as held. Any other flock error used to be a silent skip. A poll also waits up to two minutes for the lock, counted inside its budget, and the messages name every possible holder.
  6. Tests for alert paths nothing held: which runs ping, that the ping is a GET, ntfy versus JSON bodies, and every outcome of test-alert.
  7. Stale wording is fixed, including "hourly" and the hardcoded chown path.
  8. Error text is capped: a response body at 200 characters, an alert at 1,900 (Discord rejects anything over 2,000).
  9. The run budget is 22 minutes (was 24) and the backstop 26 (was 25).
    • After the budget come the last fetch, the webhook and the heartbeat, each able to time out on two addresses and then the read.
    • The timer's jitter plus the backstop still ends before the next trigger.
    • Recorded in notes/storms.md, and TestTheBackstop holds it.
  10. esb rebuild and esb compact work beside a malformed esb.db. Neither opens it first, and Store.open() keeps its connection only once the file has opened.

Scripts and units

  1. Every backup failure is announced, through an EXIT trap.
  2. The backup's git errors go into variables, not fixed file names in /tmp, and the unit gets PrivateTmp.
  3. The backup takes the poll lock for add, commit and merge.
    • It fetches first, outside the lock.
    • Git under the lock closes the lock's file descriptor, so a detached auto-gc can't keep holding it.
    • A push rejected after a long wait gets one more fetch, commit and merge.
    • It waits up to 30 minutes, which is longer than a poll's backstop plus its stop timeout.
  4. A stalled backup times out and says so. The unit covers two lock waits and four network steps, ssh has connect and keepalive timeouts, and a TERM trap reports the timeout.
  5. The esb wrapper passes secrets to sudo in the environment, not on the command line, which sudo logs.
  6. One interpreter, /usr/bin/python3, is used by the installer's gate, the wrapper and the unit.
  7. An empty host key scan stops the install, and the unit pins the key (StrictHostKeyChecking=yes).
  8. The backup setup docs are fixed: git runs as the service user, the deploy key path and mode are documented, and the installer names the backup timer.

Tried and dropped

Building a rebuild into a side file and renaming it in. It was about 60 lines, and four review rounds kept finding holes in it, all to guard esb.db, which the repo treats as disposable. A rebuild that fails now leaves a partial esb.db, and the next successful one replaces it; the raw logs are untouched either way. Recorded in notes/alerting.md.

Not taken

  • The backup unit's timeout bounds dead connections, not a slow live push. It is a backstop, and the TERM trap announces a push it stops.
  • A SIGTERM during the two-minute lock wait ends the process by default. Nothing has been written by then, and a stop is the operator's intent.
  • ntfy detection by substring: it fits the one webhook in use.
  • Reporting only the partial banner when a run is also drifted: the next run reports the drift.
  • An env-file URL containing &: no configured URL has one.
  • argparse's usage-error exit code, 2, is the same number as EXIT_AUTH: only interactive runs can hit it.

Deploying

These changes reach the Pi only when sudo sh scripts/install-native.sh is re-run there. The backup unit now pins GitHub's host key, and the installer re-seeds an empty known_hosts file and fails loudly if the scan fails.

Checks

  • ruff check: clean, on every commit.
  • unittest discover: passes on every commit. The final commit, with ESB_DATA_DIR set, runs 400 tests OK.
  • tests/test_scripts.py runs the backup against throwaway git repos, using hooks to stand in for auto-gc and a racing host, and runs the wrapper against stand-in sudo and id.
  • dash -n passes on all three scripts.

🤖 Generated with Claude Code

https://claude.ai/code/session_01UqWuH6frirpbQDKnF5rEdT

A webhook URL missing its scheme (ntfy.sh/topic) made urllib.request.Request
raise before _deliver's try, so alert.fail raised out of the run it was
reporting on: exit 1 instead of the real code, no heartbeat, and a traceback
naming the secret topic. The request is now built inside the guard.

A failed delivery's warning printed the exception's text, and urllib and
http.client quote the URL, or just its path, which is the secret (an ntfy
topic, a ping id). The warning now names only the kind of error: the HTTP
status, the socket error, or the exception class.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UqWuH6frirpbQDKnF5rEdT
The storage probe creates an empty file, which a full disk still allows, so
ENOSPC arrived from the first real write and escaped the run, as did any error
with no handler: exit 1, a traceback, no webhook and no heartbeat, leaving the
dead-man's monitor to notice two hours later with no cause.

run_poll now catches both. An OSError, or a SQLite error whose code is a full
disk, an I/O error, a file it cannot open or a read-only database, is the
storage alert (exit 6). Anything else is a new crash alert (exit 1) with the
traceback in the journal. It names esb rebuild for every crash, because
choosing from the exception's type was wrong both ways, and says a rebuild
that fails the same way, or a next run that crashes again, means the code
needs a fix. Neither pings. Recorded in notes/alerting.md.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UqWuH6frirpbQDKnF5rEdT
time.sleep raises on a negative pause, so ESB_POLL_DELAY_MS=-1 wrote the list
and one detail and then died, every run. poll.milliseconds now parses both the
flag, which refuses a bad value, and the environment variable, which falls
back to the default with a warning.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UqWuH6frirpbQDKnF5rEdT
Two runs share the .write-test name: one touches, the other touches, the first
unlinks, and the second's unlink raised FileNotFoundError, which read as an
unwritable directory: exit 6, a webhook, and no heartbeat.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UqWuH6frirpbQDKnF5rEdT
poll_lock caught every OSError from flock, so a filesystem that cannot lock
(ENOLCK) made every run skip silently. Only BlockingIOError now means held, and
any other error reaches the storage alert.

A poll waits up to two minutes for the lock, counted inside its budget, so a
backup committing for a few seconds no longer costs it the whole slot. The
skip line and the refusal from rebuild and compact name every holder: a poll,
the backup, or esb rebuild or compact.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UqWuH6frirpbQDKnF5rEdT
A partial run pings; a key rejected mid-run and an unwritable directory do
not; the ping is a GET; ntfy gets the banner as the body with a Title, and any
other webhook gets JSON; and every outcome of test-alert. No code change.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UqWuH6frirpbQDKnF5rEdT
The unreachable banner promised "the next hourly run" and two comments said
hourly, from before the timer moved to every 30 minutes. The storage banner's
chown line hardcoded /var/lib/esb-outages beside df and ls lines that use the
directory actually in use.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UqWuH6frirpbQDKnF5rEdT
AuthError and TransientError embedded the whole response body, so a 5xx HTML
page went into the partial-loss banner ten times over and into the run log's
error_summary. Discord rejects a message over 2,000 characters, so that alert
would never arrive. The body is now cut to 200 characters, and the webhook
message to 1,900 with a note that the journal has the rest.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UqWuH6frirpbQDKnF5rEdT
… ping

The 24-minute budget is checked only between fetches, and after it come the
slowest fetch, the webhook a partial or drifted run sends, and the heartbeat.
Each request can time out connecting to an IPv4 and an IPv6 address and then
on the read: 200 seconds in all, past the 25-minute TimeoutStartSec. In a storm
with a degraded API systemd would fail the unit and kill the heartbeat
mid-send.

The budget is now 22 minutes and the backstop 26, which with the timer's three
minutes of jitter still ends before the next trigger. TestTheBackstop
recomputes both from the constants and the unit files.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UqWuH6frirpbQDKnF5rEdT
esb rebuild opened the store before rebuilding, and opening a malformed esb.db
raises before rebuild() can delete it, so the command the crash alert names
failed on exactly the database it exists to replace. compact opened it too,
though it only touches raw/. Neither opens it now, and Store.open() keeps its
connection only once the file has opened.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UqWuH6frirpbQDKnF5rEdT
Under set -e a failed git add, commit or rev-parse exited before any notify: a
.git/index.lock left by a power cut failed every later backup with no alert,
and the heartbeat does not cover the backup, so the site's stale banner was
the only sign. An EXIT trap now alerts on any failure nothing else reported.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UqWuH6frirpbQDKnF5rEdT
/tmp/esb-backup-*.err were fixed names in a shared /tmp: a leftover file the
esb user could not write made the redirection fail, so git never ran and every
slot alerted with an empty body. The output is captured instead, the merge's
stdout with it (git prints CONFLICT lines there), and the unit gets its own
/tmp as well.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UqWuH6frirpbQDKnF5rEdT
The backup ran git add and git merge on the live data directory, and its slots
overlap storm runs, so a line the poll was still writing could be committed
and published torn, and a merge could refuse over a file mid-write with a
misleading conflict alert.

It now fetches outside the lock, then adds, commits and merges under it, and
pushes after. Every git run under the lock closes the descriptor, or a
detached auto-gc would hold the lock and every poll meanwhile would skip. A
push rejected after a long wait, by another host pushing meanwhile, gets one
more fetch, commit and merge. The wait is 30 minutes, above a poll's backstop
plus the 90-second stop timeout the poll unit now states.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UqWuH6frirpbQDKnF5rEdT
The backup unit is a oneshot with no TimeoutStartSec, which means no timeout,
and ssh had no ConnectTimeout or keepalive, so a stalled fetch or push held
the unit and absorbed every later slot with no alert. The unit now allows two
lock waits and four network steps, ssh gives up on a dead connection, and the
script turns the SIGTERM into an exit so the failure trap announces it.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UqWuH6frirpbQDKnF5rEdT
The esb wrapper put ESB_ALERT_WEBHOOK, ESB_HEARTBEAT_URL and ESB_API_KEY on
sudo's command line as env VAR=value, and sudo logs the whole command to the
auth log, which the adm group reads; ps showed them too. They are now exported
and passed with --preserve-env. ESB_POLL_DELAY_MS rides along, which the
wrapper had not forwarded at all.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UqWuH6frirpbQDKnF5rEdT
The installer checked whichever python3 sudo's PATH found first, and that puts
/usr/local/bin ahead of /usr/bin, while the unit runs /usr/bin/python3. A Pi
with a newer build in /usr/local and an older system Python passed the install
and then failed every timer run at import, before alert could load. The
installer and the wrapper now name /usr/bin/python3 as the unit does.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UqWuH6frirpbQDKnF5rEdT
If ssh-keyscan failed at install (no network), || true left an empty
/etc/esb-outages-known_hosts, and one missing ssh-keyscan left none. ssh cannot
write to that root-owned file, so with accept-new every push logged a warning
and took whatever key it was shown. The install now stops unless the scan
returned keys, and the unit uses StrictHostKeyChecking=yes.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UqWuH6frirpbQDKnF5rEdT
The backup script sent readers to a README walkthrough that an earlier trim
removed, and its setup lines ran git init as root, which git then refuses as
"dubious ownership" and the script reports as "no 'origin' remote". Nothing
documented the deploy key's path or mode, and the installer's next steps never
mentioned the backup timer, though its push is the only way the site updates.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UqWuH6frirpbQDKnF5rEdT
@baz8080
baz8080 force-pushed the claude/eager-sagan-wn68lr branch from 477c746 to 0fbc9cb Compare September 24, 2026 15:06
@baz8080
baz8080 merged commit 4b11f84 into main Sep 24, 2026
3 checks passed
@baz8080
baz8080 deleted the claude/eager-sagan-wn68lr branch September 24, 2026 15:09
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