swaglog: delete oldest (not newest) logs on rotation after restart - #313
Open
PeterPhuTran wants to merge 1 commit into
Open
swaglog: delete oldest (not newest) logs on rotation after restart#313PeterPhuTran wants to merge 1 commit into
PeterPhuTran wants to merge 1 commit into
Conversation
SwaglogRotatingFileHandler maintains its file list newest-first: _open() prepends each new file with insert(0, ...) and doRollover() prunes past backup_count with pop() from the tail. But get_existing_logfiles() seeded the list oldest-first, so after any process restart at the backup_count cap, the tail was the *newest* pre-existing file - every rollover then deleted the most recently written logs, marching backwards through them, while months-old files survived. Observed on a real device: each boot destroyed the ~newest 100+ swaglog files from before the reboot - exactly the files needed to debug whatever caused the reboot (this bug erased weeks of crash forensics while investigating FrogAi#309). Fix: seed the list newest-first to match the insert(0)/pop() convention. Adds a unit test covering the restart prune, repeated rollovers, and a second restart. The same bug exists in upstream commaai/openpilot; being submitted there separately. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Thanks for contributing! A maintainer will review your pull request soon. |
Owner
|
Ignore this — posted in error while I was testing some workflow automation. Your PR is targeted at the right branch, nothing is wrong with it, and I've reopened it. Sorry for the noise! |
Owner
|
Sorry, testing new workflows lol |
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
2 participants
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.
Description
SwaglogRotatingFileHandlermaintains its file list newest-first —_open()prependseach new file with
insert(0, ...)anddoRollover()prunes pastbackup_countwith
pop()from the tail — butget_existing_logfiles()seeded the listoldest-first. After any process restart at the 2500-file cap, the tail therefore
held the newest pre-existing file, and every rollover deleted the most recently
written logs, marching backwards through them, while months-old files survived.
Field evidence: on a real comma three, every boot destroyed the newest ~100+ swaglog
files from before the reboot — exactly the files needed to debug whatever caused the
reboot. This bug erased weeks of crash forensics during the #309 investigation
(logs from the failing boots kept vanishing days after being written).
Fix: one word — seed the list newest-first (
sorted(log_files, reverse=True))to match the
insert(0)/pop()convention.Verification
New
common/tests/test_swaglog.py:The same bug exists in upstream commaai/openpilot; submitted there separately.
🤖 Generated with Claude Code