From 7f54b9a637d04fc2727a70a2b1116388eb220245 Mon Sep 17 00:00:00 2001 From: PeterPhuTran Date: Thu, 9 Jul 2026 23:28:01 -0700 Subject: [PATCH] swaglog: delete oldest (not newest) logs on rotation after restart 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/FrogPilot#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 --- common/swaglog.py | 3 ++- common/tests/test_swaglog.py | 37 ++++++++++++++++++++++++++++++++++++ 2 files changed, 39 insertions(+), 1 deletion(-) create mode 100644 common/tests/test_swaglog.py diff --git a/common/swaglog.py b/common/swaglog.py index d009f00e76177d..79ebd8601c7683 100644 --- a/common/swaglog.py +++ b/common/swaglog.py @@ -45,7 +45,8 @@ def get_existing_logfiles(self): fp = os.path.join(base_dir, fn) if fp.startswith(self.base_filename) and os.path.isfile(fp): log_files.append(fp) - return sorted(log_files) + # newest first, matching _open()'s insert(0, ...) so doRollover()'s pop() deletes the oldest + return sorted(log_files, reverse=True) def shouldRollover(self, record): size_exceeded = self.max_bytes > 0 and self.stream.tell() >= self.max_bytes diff --git a/common/tests/test_swaglog.py b/common/tests/test_swaglog.py new file mode 100644 index 00000000000000..08915e3e38cf07 --- /dev/null +++ b/common/tests/test_swaglog.py @@ -0,0 +1,37 @@ +from openpilot.common.swaglog import SwaglogRotatingFileHandler + + +def seed(base: str, indexes) -> None: + for i in indexes: + with open(f"{base}.{i:010}", "w") as f: + f.write("x") + + +def existing(tmp_path) -> list[str]: + return sorted(f.name for f in tmp_path.iterdir()) + + +class TestSwaglogRotation: + def test_rollover_deletes_oldest_first(self, tmp_path): + base = str(tmp_path / "swaglog") + seed(base, range(8)) # swaglog.0000000000 .. swaglog.0000000007, exceeds backup_count + + # restart over pre-existing files: __init__ rolls over, opening .8 and pruning to backup_count + handler = SwaglogRotatingFileHandler(base, backup_count=4) + try: + assert existing(tmp_path) == [f"swaglog.{i:010}" for i in (5, 6, 7, 8)] + + # subsequent rollovers keep deleting oldest-first + handler.doRollover() + assert existing(tmp_path) == [f"swaglog.{i:010}" for i in (6, 7, 8, 9)] + handler.doRollover() + assert existing(tmp_path) == [f"swaglog.{i:010}" for i in (7, 8, 9, 10)] + finally: + handler.close() + + # a second restart over the survivors keeps the same invariant + handler = SwaglogRotatingFileHandler(base, backup_count=4) + try: + assert existing(tmp_path) == [f"swaglog.{i:010}" for i in (8, 9, 10, 11)] + finally: + handler.close()