Recover the wifi instead of waiting for someone to notice, and make the daily video cheaper - #15
Merged
Merged
Conversation
last_capture_age() took the newest mtime with `find | sort -rn | head -1`.
head exits after one line, so sort was left writing into a closed pipe on every
run. Interactively that is invisible -- SIGPIPE kills sort silently -- but
systemd sets IgnoreSIGPIPE=yes by default, so under the service sort's write()
returns EPIPE instead and GNU sort reports it:
raspilapse-watchdog[13480]: sort: write failed: 'standard output': Broken pipe
raspilapse-watchdog[13480]: sort: write error
Two lines every five minutes, ~576 a day, into a journal capped at 200M that is
already at the cap. It was evicting exactly the history you go looking for when
something goes wrong: diagnosing this week's incident, journalctl --list-boots
only reached back two days of a nine-day run.
Taking the max in one awk pass closes the pipe question entirely, drops a full
sort of ~20,000 paths twice an hour, and truncates the fraction so the caller
no longer needs ${newest%.*}.
Verified by reproducing the systemd condition: with `trap "" PIPE` the old
pipeline prints both lines and the new one prints nothing. Value is unchanged
on the live image directory, and the service now logs nothing at all on the
healthy path.
Not vacuuming the journal, though the plan called for it: journald already
rotates at the cap, so vacuuming buys no lasting headroom -- it would only
delete this week's incident logs sooner. Cutting the write volume is the fix.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DmLb2SeiKE4hdQXptxhaja
SystemMonitor has read memory and disk every thirty seconds since it was written. store_capture bound cpu_temp and the three load averages and dropped the rest on the floor, and the docstring said so plainly -- "System monitoring data (cpu_temp, load)". That cost real time this week. A camera looked frozen after nine days, and ruling out a memory leak meant arguing from CPU temperature and load alone, because no memory series existed to look at. The answer turned out to be elsewhere, but nothing in the database could have told us that. Eight columns, migration v6: memory used and percent, disk free and percent, uptime, the capture process's own RSS, and whether there was a default route with the wifi signal alongside it. INTEGER wherever the fraction is noise -- SQLite varint-encodes small integers, so these cost roughly 20 MB over a 180-day window rather than the 33 MB eight REALs would, the same arithmetic migration 5 used to justify dropping indexes. Measured on a copy of this camera's real database: 549,288 rows migrated in 111 ms, every existing value intact and every new column NULL. Two new collectors, both pure /proc reads with no subprocess, since this runs in the capture loop and get_cpu_temperature already costs a fork. RSS from /proc/self/statm, verified equal to VmRSS. Network state from /proc/net/route and /proc/net/wireless: network_up means a default route exists on an interface the kernel calls up, explicitly not that the internet is reachable -- deciding that needs a socket, and the capture loop must never block on one. Docker's bridge route filters out on its destination rather than by name. store_capture's INSERT was a 36-name list, a hand-typed run of 36 question marks and a 36-element positional tuple, maintained separately. Adding eight columns by hand across three places is a misalignment waiting to happen, and a misalignment there writes plausible wrong data into the wrong column instead of raising. Names and values now come from one dict. The migration test fails on the unmigrated code, checked by removing the migration: "no such column: network_up". test_migration_5 stopped asserting a hardcoded 5 -- opening a legacy file runs every migration, so that number breaks on each bump for a reason unrelated to what the test is about. 1052 tests pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DmLb2SeiKE4hdQXptxhaja
This camera captured 2880 frames a day for nine days without missing one, and
still looked frozen from the outside. At 23:31 the access point dropped, both
BSSIDs timed out within 23 seconds, and NetworkManager read the timeout as a
wrong password:
wpa_supplicant: Authentication with 66:7f:f0:07:03:c0 timed out.
NetworkManager: Activation: (wifi) disconnected during association, asking for new key
NetworkManager: state change: activated -> need-auth (reason 'supplicant-disconnect')
NetworkManager: no secrets: No agents were available for this request.
NetworkManager: state change: need-auth -> failed (reason 'no-secrets')
That last line sets an autoconnect blocked reason on the profile rather than a
retry counter, so connection.autoconnect-retries=-1 -- already in force here --
does nothing, and no other setting in NetworkManager.conf helps either. It
clears on an agent supplying secrets, an explicit nmcli activation, a change to
the profile's secrets, or an NM restart. wlan0 sat inactive for eight and a
half hours until a human power-cycled the Pi. The uploads and the live image
were the only symptoms, which is exactly what a frozen camera looks like.
So: a sibling of check_service.sh on a two-minute timer. Two checks of grace so
an access point reboot rides out untouched, then nmcli dev connect (the step
that actually clears the block), the priority profile explicitly, a radio
cycle, an NM restart, and only then a reboot.
The reboot is the part worth being careful about, because getting it wrong
turns an outage into a reboot loop. Three gates, all of which must hold: thirty
minutes of continuous failure, our SSID seen on the air at least once during
it, and six hours since the last watchdog reboot -- a stamp shared with
check_service.sh so the two cannot ping-pong. If the SSID is not on the air the
access point is off or we are out of range, nothing local can fix that, and it
can never reach a reboot however long it lasts. The stamp is synced before
rebooting, or ext4's commit window loses the rate limit across the very reboot
it bounds.
Detection is a gateway ping, not DNS and not an internet host: an ISP outage is
not something reconnecting the wifi can fix. A default route on any interface
that is not the managed wifi returns healthy immediately, so ethernet is never
disturbed.
Two lockout traps closed deliberately. `nmcli radio wifi off` is persisted by
NM and survives reboots, so a run killed between the off and the on would
strand a headless camera permanently -- there is an EXIT trap, and because
traps do not run on SIGKILL, an unconditional re-enable at the top of every
run. And modprobe -r brcmfmac is deliberately absent, with a comment saying so,
because a failed firmware reload needs physical access to the camera.
Tested by stubbing nmcli/ip/ping/systemctl on PATH, which covers the ladder
without a radio. Every guard was verified by removing it and watching the test
that exists for it fail: drop the SSID gate and the missing-AP case reboots,
drop the interval floor and a 60-second-old reboot reboots again, drop the
radio re-enable and it stays disabled. Also exercised end to end on this
machine in --dry-run against the real nmcli: it reads both configured profiles
including the one with a space in its name, confirms the AP is on the air, and
walks the ladder to the reboot decision without writing a stamp.
Opt-in like the watchdog, since it runs as root and can reboot:
./scripts/install.sh --with-netwatch
1070 tests pass.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DmLb2SeiKE4hdQXptxhaja
check_network.sh can reboot the camera and so can check_service.sh, and a Pi that has wedged its wifi is quite likely to look stalled to the other one as well. Two independent escalation ladders with no shared limit means whichever notices first reboots, the other one's counter survives, and the machine can end up cycling. Both now read and write /var/lib/raspilapse/last_reboot and refuse to reboot within six hours of the other having done so. The stamp is written and synced before the reboot -- ext4's commit window will otherwise drop it across the very reboot it exists to bound, which silently removes the limit. A stamp that is missing or unparseable reads as "never", so corruption cannot wedge recovery in the other direction either. Also TimeoutStartSec=180 on the watchdog unit. It is Type=oneshot behind a timer, and a find over a stalled filesystem would leave it in 'activating' forever; a timer does not re-trigger a unit that never finished, so the watchdog would quietly stop watching at exactly the wrong moment. check_service.sh had no tests, which was survivable when its worst action was a restart and is not now. Nine, driven with a stub systemctl and real file mtimes rather than a mocked clock. The shared floor is verified by removing it and watching the test fail. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DmLb2SeiKE4hdQXptxhaja
preset fast -> veryfast, crf 25 -> 20, threads 2 -> 3. Native 4K is unchanged.
A preset does not set quality, crf does; a faster preset reaches the same crf
with a bigger file. At 3840x2160 on a Pi the encode is dominated by decode,
scale and deflicker rather than by bitrate, so the slower preset was buying
very little. Measured over 60 real frames from this camera:
fast crf 25 25.2 Mbit/s 75 s SSIM 0.985634 <- was
veryfast crf 20 43.8 Mbit/s 35 s SSIM 0.990225 <- is
Faster and closer to the source, which is not a trade-off so much as the old
settings being on the wrong side of the curve. crf 25 at 4K quantises away
exactly the fine texture 4K exists to carry -- the frame stays 3840x2160 while
foliage, water and distant edges flatten -- so the file was 4K in dimensions
carrying rather less than that in detail. Disk is what pays: 284 -> 493 MB/day,
which at the seven-day retention added in this branch is 3.4 GB against 82 GB
free. threads 3 rather than 4 leaves a core for the capture loop, and 4
measured no faster anyway because the job is decode-bound.
Also, three things that were latent rather than measured:
h264_v4l2m2m is refused above 1920x1080 instead of being handed to ffmpeg.
Nothing sets a resolution by default, so simply setting video.codec.name to
the hardware encoder turned the 05:00 job into a nightly "can't configure
encoder". CONFIG-REFERENCE.yml already warned about this in a comment; now the
code enforces it. Verified on this hardware that it fails at both 4K and 1440p
and succeeds at 1080p -- which is why the two existing hardware-codec tests
now pass a resolution: they were asserting a shape that could never run in
production.
A flock on data/daily.lock. systemd already stops the unit overlapping itself;
what it cannot see is someone running the module in a shell during the timer's
run, which is two ffmpegs and a starved capture loop. Contention returns 0, not
1 -- a duplicate invocation is not a failure, and a 'failed' unit is the
misleading signal this whole investigation started from.
CPUWeight=20 on the unit, because Nice alone does not hold back a
multi-threaded encode. MemoryHigh/MemoryMax are set too, with the honest
caveat in the unit: Raspberry Pi OS ships the memory cgroup controller
disabled, so on a stock Pi systemd accepts them and they do nothing. /proc/
cgroups has no memory row here and systemctl reports MemoryMax=infinity. They
need cgroup_enable=memory cgroup_memory=1 in cmdline.txt and a reboot to arm.
Left in regardless: correct, free when inert, live the moment it is enabled.
Guard and lock both verified by removing them and watching their tests fail.
1084 tests pass.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DmLb2SeiKE4hdQXptxhaja
config/config.yml is gitignored, so the previous commit's preset/crf/threads
change only applied to this camera. A fresh install fell back to the values in
timelapse.py -- ultrafast, crf 23, 2 threads -- which nothing measured and
which are a good deal softer than what this camera has actually been running.
Those fallbacks were chosen when 4K encoding was OOMing, so replacing them
needed a memory number rather than an assertion. Peak child RSS over 60 real
4K frames, each in its own process because ru_maxrss for RUSAGE_CHILDREN is a
high-water mark that never falls:
ultrafast crf 23 2 threads 401 MB (the old fallback)
fast crf 25 2 threads 1399 MB (what production actually ran)
veryfast crf 20 3 threads 1021 MB (the new default)
So this is not a relaxation of the OOM fix: the new default peaks 378 MB below
the configuration that has been running here for months, while being 2.1x
faster and closer to the source. MemoryHigh at 1800M has comfortable headroom
over it, and the unit's comment now carries the real numbers rather than a
rounded guess.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DmLb2SeiKE4hdQXptxhaja
cleanup_old_images.sh has deleted source JPEGs older than seven days for a long time. Nothing has ever deleted what those frames turn into, so the bounded input fed an unbounded output: /var/www/html/videos is 3.2 GB from twelve days and climbing. The cleanup timer already exists, is installed and runs nightly at 02:00, so this is a third ExecStart on that unit rather than any new plumbing. It is Python rather than an addition to the bash script for one reason: the safety rule needs the upload queue, and the queue is SQLite. The rule is that a file goes when it is older than video.retention_days and the queue does not hold a row for it in any state other than 'success'. Two halves of that are worth stating because they are the surprising ones. 'failed' counts as protected. Those rows have exhausted their retries and the retry timer will never touch them again, so treating them as terminal would be defensible -- but that video exists nowhere else, and quietly deleting the one copy of a day nobody managed to upload is a worse outcome than keeping it. Retained files are logged as retained so they read as something to deal with rather than as an invisible leak. Files the queue has never heard of are not protected. That is deliberate: it is how the ad-hoc partial-day renders accumulating in that directory (..._0500-1552.mp4 and friends) finally get cleaned up. Symlinks are never followed, an unreadable database deletes nothing at all, and emptied YYYY/MM directories go the same way cleanup_old_images.sh removes them. Dry-run against this camera's real directory: 23 files, 1305.9 MB, everything from 26-30 July and nothing from the 31st on, with zero non-success queue rows so nothing was at risk. Not executed -- the timer will do it, or `raspilapse-prune-videos` will. 15 tests, most of them about what is kept rather than what goes. 1099 pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DmLb2SeiKE4hdQXptxhaja
Both outputs are a vertical strip taken from every frame; only which strip and where it lands differs. They were built by two separate full passes over the day's images, so after ffmpeg had already read all 2880 4K frames, the job read them twice more. From the 7 August journal: keogram 314s, slitscan 271s. create_time_slices does one decode and feeds both canvases. Measured over 300 real frames from this camera: 60.7s to 27.6s, 2.20x. create_keogram and create_slitscan are now thin wrappers, so the module CLI and existing callers are untouched. Verified byte-identical, which is the only result that makes this safe to ship: both outputs sha256-match what the split implementation produced on the same 300 frames. Three behaviours had to survive intact. The two resize rules stay separate -- a keogram matches height and preserves aspect, a slitscan forces both dimensions -- rather than being unified into one. The slitscan advances its x position even when a frame fails to open, so a bad frame leaves a gap instead of sliding every later frame one strip left. And there is one progress readout now instead of two. The tests for that deserve a caveat rather than a claim. My first version compared fused output against create_keogram, which is now a wrapper over the same code -- so breaking the shared branch broke both sides identically and the comparison still passed. It proved nothing. Rewritten around what is actually observable: a keogram column is the *centre* of a frame, and the centre of an image is the centre of that image scaled, so applying the wrong resize rule to it changes pixels by resampling noise alone (44 vs 46 on a ramp). No output assertion can pin that branch down and the test file now says so instead of pretending otherwise. The slitscan's rule takes its strip at a position, so getting it wrong takes it from the wrong part of the scene -- that one is asserted concretely, and verified by making the slitscan adopt the keogram's rule and watching it fail. 1110 tests pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DmLb2SeiKE4hdQXptxhaja
The expensive part of this week was not fixing anything, it was working out that the camera had never stopped. A dead network and a dead Pi look identical from a desk: no upload, no live image, nothing responding. Capture writes to the local card and does not care whether the network exists, so the camera can be entirely healthy and entirely invisible at the same time, and the reflex -- power-cycle it -- both "fixes" the problem and destroys the evidence. TROUBLESHOOTING gains that section, leading with the two commands that answer the question (ask the card, not the dashboard), the actual NetworkManager log lines to match against, why no configuration setting clears 'no-secrets', and the --dry-run invocation for watching the recovery script decide without letting it act. Also documents video.retention_days under "Disk filling up", where the existing text explained image retention and database retention and had nothing to say about the videos, which were the unbounded ones. End-to-end check of the whole changed pipeline on a real hour of frames: 3840x2160 native, libx264, 46.9 Mbit/s, keogram and slitscan both produced from the single fused pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DmLb2SeiKE4hdQXptxhaja
Import ordering in the two function-local imports, and a Path import left behind when the retention tests moved to string paths. make lint passes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DmLb2SeiKE4hdQXptxhaja
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 20 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (13)
📝 WalkthroughWalkthroughThis PR adds capture telemetry persistence, shared keogram and slitscan generation, queue-aware video retention, staged network recovery, coordinated watchdog reboot limits, daily-run locking, and systemd resource controls. ChangesCapture telemetry persistence
Media generation and retention
Network and service watchdogs
Daily video operations
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant NetwatchTimer
participant check_network.sh
participant NetworkManager
participant systemd
NetwatchTimer->>check_network.sh: Run network check
check_network.sh->>NetworkManager: Inspect route and Wi-Fi state
check_network.sh->>NetworkManager: Apply staged recovery
check_network.sh->>systemd: Request reboot when gates pass
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (4)
tests/test_check_network.py (1)
189-202: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a corrupt reboot-stamp test for the network watchdog.
tests/test_check_service.pyhastest_a_corrupt_stamp_does_not_block_forever, but this file has no equivalent.check_network.shdoes not validate the stamp contents, so a corrupt stamp blocks every reboot permanently. See the comment onscripts/check_network.shLines 178-184.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_check_network.py` around lines 189 - 202, Add a test alongside the reboot-stamp cases in the network watchdog tests that writes a non-numeric value to RASPILAPSE_LAST_REBOOT, runs the check with conditions that would otherwise permit a reboot, and verifies the reboot command is invoked. Use the existing test helpers and assertions from test_a_recent_reboot_blocks_another and test_a_short_outage_does_not_reboot_even_at_a_high_count.raspilapse/video/timelapse.py (2)
228-228: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTwo inline codec lists remain.
Line 262 and line 584 still test
codec in ["h264_v4l2m2m", "h264_omx"]. The comment at lines 26-29 states the constant exists so the places that care cannot disagree. Replace those two literals withHARDWARE_CODECSto complete that.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@raspilapse/video/timelapse.py` at line 228, Replace the inline codec lists at the checks around lines 262 and 584 with the shared HARDWARE_CODECS constant, matching the existing check near line 228 and preserving the current membership-test behavior.
169-188: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd the minimal fix to the error message.
The message offers two remedies: change to
libx264, or scale the output down. It omits the third and most direct remedy for a hardware-encoder user: set an explicit resolution at or below 1920x1080. The-hwflag alone now always fails, becauseresolutionstaysNoneunless-hdis passed.Consider naming the explicit resolution option in the message, and stating the
-hdrequirement in the--hwhelp text.♻️ Proposed message change
msg = ( f"{codec} cannot encode above {max_w}x{max_h}; requested {requested}. " - f"Set video.codec.name to libx264, or scale the output down." + f"Set a resolution of {max_w}x{max_h} or lower (--hd), " + f"or set video.codec.name to libx264." )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@raspilapse/video/timelapse.py` around lines 169 - 188, Update the error message constructed in the hardware-codec validation block to explicitly recommend setting an output resolution at or below the supported maximum, including the relevant -hd option. Also update the --hw help text to state that -hd must be provided with an explicit supported resolution, while preserving the existing libx264 and scaling remedies.raspilapse/video/retention.py (1)
159-194: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider surfacing a skipped run.
prune_videosreturns an empty result in three different situations: retention disabled, the video directory is missing, and the upload queue could not be read.mainprints "Deleted 0 file(s)" for all three and returns 0. An operator cannot tell a clean no-op from a run that skipped its safety check.Consider returning a distinct status from
prune_videos, or at least logging at error level inmainwhen the queue read failed.Also move
import osto the module top with the other imports; the deferredimport argparseat line 161 already runs before it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@raspilapse/video/retention.py` around lines 159 - 194, Update prune_videos and main so a failed upload-queue read is distinguishable from retention-disabled or missing-directory no-ops, and have main report the queue-read failure at error level while preserving successful zero-deletion output. Move the os import to the module-level imports and remove the deferred import inside main.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/TROUBLESHOOTING.md`:
- Around line 303-306: Update the fenced code block containing the
NetworkManager log output to specify the text language, changing its opening
fence to use text while preserving the log contents.
- Around line 330-334: Update the documented dry-run command in the “Watch it
decide without letting it act” section to set RASPILAPSE_NETWORK_STATE through
sudo env, using the sudo-compatible invocation with env before the script
command.
In `@raspilapse/storage/database.py`:
- Around line 146-149: Update the _as_int conversion helper to also catch
OverflowError from rounding non-finite numeric values, returning None so
store_capture can persist NULL while preserving the existing handling of
TypeError and ValueError.
In `@raspilapse/video/daily.py`:
- Around line 156-162: Update the lock acquisition exception handling around
fcntl.flock in the daily-run entrypoint to return 0 only when OSError.errno is
EACCES or EAGAIN. For all other OSError values, report the error and return 1,
preserving the existing duplicate-run message and success behavior only for
genuine lock contention.
In `@raspilapse/video/keogram.py`:
- Around line 275-292: Before the conditional _save_slice calls in the keogram
generation flow, check whether processed is zero and report failure by returning
results with both requested output entries set to False (or the established
failure representation). Ensure no black canvases are saved or queued when all
frames fail, while preserving normal saving when processed is greater than zero.
- Around line 187-193: Validate target_height immediately after calculating it
and before the Image.new calls for keogram and slitscan. When target_height is
less than or equal to zero, append the appropriate error to results and return
results, preventing image creation and unhandled exceptions.
In `@raspilapse/video/retention.py`:
- Around line 84-104: Validate retention_days in the retention flow before
computing cutoff, rejecting negative values while preserving the existing no-op
behavior for zero or unset values. Apply this guard to both CLI-provided and
config-derived values, and return the existing result without deleting files
when the value is invalid; update the logic around the retention_days handling
and cutoff calculation.
- Around line 62-66: Update the protected-path normalization in the function
populating protected to use Path(path).resolve() instead of absolute(), and make
the corresponding comparison key at the upload-queue lookup use resolve() as
well. Keep the existing protected mapping and status message behavior unchanged.
In `@scripts/check_network.sh`:
- Around line 178-184: The network watchdog must validate the shared reboot
stamp and ensure its directory exists before writing. In
scripts/check_network.sh lines 178-184, update the stamp handling after reading
REBOOT_STAMP to reset non-numeric values to 0, and update do_reboot to create
the stamp directory before writing. In tests/test_check_network.py lines
189-202, add coverage for non-numeric RASPILAPSE_LAST_REBOOT content and assert
systemctl reboot still runs, matching
test_a_corrupt_stamp_does_not_block_forever in tests/test_check_service.py.
In `@tests/test_video_retention.py`:
- Around line 137-145: Update test_a_symlink_is_never_followed so the symlink
target outside also receives an old mtime before prune_videos runs, while
preserving the existing old symlink mtime setup; this ensures the test fails if
retention logic follows the link instead of honoring the symlink guard.
---
Nitpick comments:
In `@raspilapse/video/retention.py`:
- Around line 159-194: Update prune_videos and main so a failed upload-queue
read is distinguishable from retention-disabled or missing-directory no-ops, and
have main report the queue-read failure at error level while preserving
successful zero-deletion output. Move the os import to the module-level imports
and remove the deferred import inside main.
In `@raspilapse/video/timelapse.py`:
- Line 228: Replace the inline codec lists at the checks around lines 262 and
584 with the shared HARDWARE_CODECS constant, matching the existing check near
line 228 and preserving the current membership-test behavior.
- Around line 169-188: Update the error message constructed in the
hardware-codec validation block to explicitly recommend setting an output
resolution at or below the supported maximum, including the relevant -hd option.
Also update the --hw help text to state that -hd must be provided with an
explicit supported resolution, while preserving the existing libx264 and scaling
remedies.
In `@tests/test_check_network.py`:
- Around line 189-202: Add a test alongside the reboot-stamp cases in the
network watchdog tests that writes a non-numeric value to
RASPILAPSE_LAST_REBOOT, runs the check with conditions that would otherwise
permit a reboot, and verifies the reboot command is invoked. Use the existing
test helpers and assertions from test_a_recent_reboot_blocks_another and
test_a_short_outage_does_not_reboot_even_at_a_high_count.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ce312166-c1da-4015-80a8-19284acf8e97
📒 Files selected for processing (28)
CHANGELOG.mddocs/CONFIG-REFERENCE.ymldocs/TROUBLESHOOTING.mdpyproject.tomlraspilapse/cli/prune_videos.pyraspilapse/storage/database.pyraspilapse/system.pyraspilapse/video/daily.pyraspilapse/video/keogram.pyraspilapse/video/retention.pyraspilapse/video/timelapse.pyscripts/check_network.shscripts/check_service.shscripts/install.shsystemd/nm-raspilapse.confsystemd/raspilapse-cleanup.service.insystemd/raspilapse-daily-video.service.insystemd/raspilapse-netwatch.service.insystemd/raspilapse-netwatch.timer.insystemd/raspilapse-watchdog.service.intests/test_check_network.pytests/test_check_service.pytests/test_daily_timelapse.pytests/test_database.pytests/test_make_timelapse.pytests/test_system_monitor.pytests/test_time_slices.pytests/test_video_retention.py
… fail Ten findings, all verified against the code before changing anything. The two that mattered most were about my own tests. **Tests that proved nothing.** The symlink test asserted `outside.exists()` after pruning -- but unlink() on a symlink removes the link and leaves the target, so that assertion holds whether or not the guard exists. Confirmed by deleting the guard and watching it still pass. It now asserts on the link and on an empty deletion list. CodeRabbit spotted the stale-mtime half of this; aging the target alone did not fix it, which took a second revert-check to find. Likewise the all-frames-fail test: it wrote garbage files, which fail the first-frame dimension read and exercise the early-return path rather than the loop. Truncating real JPEGs to 70% gets a readable header and a failing decode, which is the case actually under test. **Real defects fixed.** retention.py compared upload-queue paths with absolute(), which only prepends the cwd -- it leaves '..' and symlinks in place. A queue path reaching the same file through a symlinked directory misses the lookup and the file is deleted while its upload is still pending, which is the one thing that function exists to prevent. resolve() on both sides, with a test. A negative retention_days puts the cutoff in the future, so every file matches and a mistyped `--retention-days -7` empties the directory. Rejected now. check_network.sh read the shared reboot stamp without validating it. Verified: "garbage" evaluates to 0 and is harmless, but "1abc" makes bash arithmetic fail outright, the gate returns 1, and reboots are blocked permanently. check_service.sh already guarded this; now both do, and do_reboot creates the stamp directory. create_time_slices saved and reported success on blank canvases when every frame failed to decode, feeding a black image into the upload queue. And crop percentages summing past 100 made Image.new raise outside the per-frame handler, killing the run with a traceback. Both guarded. _as_int(inf) raised OverflowError, which escaped into store_capture's handler and dropped the entire capture row -- losing a frame's exposure and brightness history over one bad telemetry reading. daily.py returned 0 for any OSError from flock. Contention is EACCES/EAGAIN; ENOLCK on a filesystem without lock support would have skipped the day's video while systemd reported success. **Smaller.** Two inline codec lists still bypassed HARDWARE_CODECS, whose own comment says it exists so they cannot disagree. The hardware-encoder error message named neither of the remedies that work, and --hw's help did not say it needs --hd. prune_videos now reports *why* it did nothing, so "upload queue unreadable" is distinguishable from "nothing to delete". MD040 on a fence, and the documented dry-run command uses `sudo env` -- the bare VAR=value form works here only because default Pi sudoers implies SETENV. Every new guard verified by removing it and watching its test fail. 1128 pass, lint clean, and the pipeline still renders end to end. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DmLb2SeiKE4hdQXptxhaja
Found by watching it run on the camera rather than in tests: rfkill writes a syslog line on every invocation, including when there was nothing to unblock. Verified directly -- an unblock against an already-unblocked radio still adds a journal entry. The step-0 safety net called it unconditionally on a two-minute timer, so it was about 720 lines a day into a journal already sitting at its 200 MB cap. That is the same spam this branch removed from check_service.sh, reintroduced three commits later by the thing meant to protect the logs. Now it only fires when `rfkill list wifi` reports something blocked, which also makes the case worth a log line when it does happen. The unconditional `nmcli radio wifi on` above it is unaffected: nmcli is silent when the radio is already enabled, and that check is the one that saves a camera whose recovery run was SIGKILLed mid-cycle. For scale: measured against the existing 5-minute watchdog, journald costs about 1.3 KB per unit activation line, so the netwatch timer itself accounts for roughly 2.7 MB/day, or 1.35% of the cap. Worth knowing before adding more timers. Verified by making it unconditional again and watching the test fail. 1130 pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DmLb2SeiKE4hdQXptxhaja
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
1 participant
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.
The Pi never froze
Investigated a camera that appeared to freeze after ~9 days. It didn't. Capture ran the entire time:
What actually happened
The access point dropped at 23:31 on 6 August. Both BSSIDs timed out within 23 seconds, and NetworkManager read the timeout as a wrong password:
failed (reason 'no-secrets')sets an autoconnect blocked reason on the profile, not a retry counter — soconnection.autoconnect-retries=-1, already in force, could not clear it, and noNetworkManager.confsetting can either. wlan0 satinactivefor 8h35m.The only symptoms were the upload and the live image going quiet, which is indistinguishable from a hang unless you look at the card. Rebooting "fixed" it and destroyed the evidence.
Changes
Network recovery (
scripts/check_network.sh, opt-in via--with-netwatch). Two checks of grace so an AP reboot rides out untouched, thennmcli dev connect(the step that clears the block), the priority profile, a radio cycle, an NM restart, and only then a reboot — gated on 30 min of continuous failure, the SSID having been seen on the air, and 6 h since the last watchdog reboot. An AP that is switched off can never trigger a reboot, however long it stays off. Tested with stubnmcli/ip/ping/systemctlon PATH; every guard verified by removing it and watching its test fail.Telemetry (schema v6).
SystemMonitorcollected memory and disk every 30 s andstore_capturethrew them away. Ruling out a leak meant arguing from temperature and load alone. Now stores memory, disk, uptime, process RSS and network state.system_uptime_smakes every reboot visible in the capture timeline;network_signal_dbmwould have predicted this. Migrating the real 549,288-row database took 111 ms.The daily encode is faster and closer to the source —
veryfast/crf 20/3 threads, still native 4K:A preset doesn't set quality, crf does; at 4K the encode is dominated by decode and filtering, so the slower preset was buying almost nothing. crf 25 at 4K quantises away the fine texture 4K exists to carry. Memory went down.
One decode pass for keogram + slitscan. Both are a vertical strip from every frame, built by two separate full passes over the day's 4K images (314 s + 271 s). Now 2.20× faster, with both outputs sha256-identical to what the split version produced.
Video retention (
video.retention_days: 7). Source frames were expired; the videos made from them never were — 3.2 GB from twelve days, unbounded. Runs as a third step of the existing nightly cleanup timer. Never deletes a file the upload queue holds in any state other thansuccess, includingfailed, because that video exists nowhere else.Two smaller ones. The capture watchdog's
find | sort | headleftsortwriting into a closed pipe — invisible interactively, but systemd setsIgnoreSIGPIPE=yes, so it printed two errors every 5 minutes into a journal already at its cap, evicting the history needed to diagnose this. Andcodec.name: h264_v4l2m2msilently turned the 05:00 job into a nightly failure, since the Pi's encoder stops at 1080p and nothing sets a resolution; now refused up front.Verification
1110 tests pass,
make lintclean. End-to-end run on a real hour of frames: 3840×2160 native, libx264, 46.9 Mbit/s, keogram and slitscan from the single fused pass.Two things I'd flag rather than bury:
MemoryHigh/MemoryMaxon the video unit do nothing on stock Raspberry Pi OS — the memory cgroup controller is disabled. They needcgroup_enable=memory cgroup_memory=1incmdline.txtand a reboot. Left in with that stated in the unit;CPUWeightworks today.🤖 Generated with Claude Code
Summary by CodeRabbit