Review the collector: the raw line no longer waits on the database - #58
Merged
Merged
Conversation
lift_status/ and scripts/ predate review on this repository and are the only code whose mistakes a rebuild cannot undo, so they were reviewed whole, once. Seven of ten findings are fixed here; notes/collector-review.md has all ten, including the one that was not a real path and the two left open as decisions. - Write the raw line before opening SQLite. A database left corrupt by a power cut stopped every poll from reaching the log, with no alert; it is now exit 7 with a banner saying the response was kept. - Route an OSError from the append to the storage alert: the empty probe file in check_writable passes on a full SD card. - Finish a line a power cut left without a newline before appending, so the fragment no longer takes the next good record with it. - Treat a truncated or corrupt gzip body (EOFError, zlib.error) as a transient error instead of letting it escape unlogged. - Clear the alert dedup marker on a clean run, so the same fault coming back within a day alerts again. - Raise the poll unit's TimeoutStartSec above the client's own worst case, and bound the backup unit and its ssh so a stalled push cannot hang it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AnrcmsrTHYCgnVGyqBqji4
…same sort_keys=True exists so two collectors' logs can be merged with sort -u, which is also how git's conflict on a shared day file would be resolved. But sort -u orders lines by their first key, body, and replay followed line order, so a merged file replayed out of time order: closures, miss counts and listing stretches would all have come out wrong. iter_raw_lines now sorts each file by fetched_at_utc, stably. The old line-order rule guarded against clock jumps, but only partly, since a pre-NTP stamp already lands in the wrong day's file. All 2,224 real lines are already in time order within their files, and rebuild then stats gives identical output before and after. The NTP wait stays as it is, by the owner's call, and the note says so. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AnrcmsrTHYCgnVGyqBqji4
- A fetch that failed keeps its own alert when the database is broken too. The database banner said the response was kept when there was no response, and hid a rejected key behind exit 7. - The database banner says what to do for a lock, a full card and corruption, instead of telling every error to move the database aside. - The alert marker clears after four consecutive clean runs, not the first. Clearing on one clean poll made a flapping API alert on every failure, which is what the repeat window exists to stop. - backup-to-git.sh traps TERM: dash skips the EXIT trap on a signal it does not trap, so the new 15-minute cap ended a stalled push silently. - Store.write_raw is gone, so the only way to write the raw line is the one that does not open the database first; the newline check reads through the append handle. - The replay-order note states the power-cut case it gives up, and the CLAUDE.md row is a pointer again. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AnrcmsrTHYCgnVGyqBqji4
baz8080
added a commit
that referenced
this pull request
Sep 24, 2026
Chapter 17 closes chapter 16's two bugs, neither the way its issue proposed, and corrects chapter 16's Kishoge diagnosis. Re-measuring for it found August's national figure moved from 76% to 75% after August ended: Tullamore's planned works came back on 16 September and the pooled grace took its August A to a D on the 19th. Narrated as a finding, not decided. Chapter 18 is the collector's first whole-file review, and corrects two sentences of chapter 01: the raw line waited on the database, and a sort -u merge reorders, so replay now sorts by fetch time. The closing becomes 19a and 19b, forward pointers go into 01, 10 and 16, and every current figure is re-measured at 24 September. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This was referenced Sep 24, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
lift_status/andscripts/were written before PRs here went through a review agent, and have barely changed since (parse.py,client.pyand__main__.pynot at all). They are also the only code whose mistakes a rebuild cannot undo: a response that never reached the raw log is gone. So they got one whole-file review, and the rest of the repository did not:lift_access/and most oflift_site/have been reviewed diff by diff since 2026-08-26, and the real-corpus and golden tests pin what they publish.The review made ten findings. Eight are fixed here, one turned out not to be a real path, and one is left for now. All ten are in
notes/collector-review.md. A second review, of this PR itself, is addressed in the last commit.Fixed
Store()opened SQLite before the raw line was written, so a database broken by a power cut stopped every poll from reaching the log, with a traceback and no alert.append_rawnow writes the line before the database is opened, and it is the only way to write one. A database failure is exit 7, and the banner says what to do for a lock, a full card or corruption. A fetch that failed keeps its own alert even when the database is broken too.check_writabletouches an empty file, which passes on a full disk. AnOSErrorfrom the append now goes to the storage banner.EOFErrorandzlib.errorare neither anOSErrornor anHTTPException. They are now aTransientError, so they get retried and logged.TimeoutStartSec=60was below the client's own worst case, so it is now 300.ConnectTimeoutandServerAliveInterval, and the unit has a 15-minute cap. The script traps TERM, so hitting the cap still alerts.sort -umerge replayed out of order.sort -uorders lines bybody, but replay followed line order, so two collectors' merged logs would have rebuilt the wrong history. Replay now sorts each file byfetched_at_utc, stably. The cost, written into the note: after a power cut, a catch-up poll stamped up to an hour early is applied before runs it followed. On the real data this moves nothing.Left for now
time-sync.targetdoes not wait for NTP unlesssystemd-time-wait-sync.serviceis enabled. Enabling it risks polls that silently never run if NTP is unreachable, so it is left as it is, by the owner's call.Deploying
The unit files and the backup script only take effect on the Pi after
scripts/install-native.shand adaemon-reload.Verification
ruff checkandscripts/no-em-dash.share cleanLIFT_STATUS_DATA_DIRset. The 12 skips were already there: no survey directory, and running as rootrebuildthenstatsagainstlifts-datagives identical output before and after, on every commitgit fetchthat hangs and its process group sent TERM, sends its alert with exit 143🤖 Generated with Claude Code
https://claude.ai/code/session_01AnrcmsrTHYCgnVGyqBqji4