From 83129f179a57d3d7f473675488ef0148e5a11302 Mon Sep 17 00:00:00 2001 From: Sai Asish Y Date: Sat, 18 Jul 2026 05:34:14 -0700 Subject: [PATCH 1/2] [Backup] Log backup rotation failures instead of silently swallowing them Signed-off-by: Sai Asish Y --- termstory/backup.py | 8 ++++++-- tests/test_backup.py | 34 ++++++++++++++++++++++++++++++++++ 2 files changed, 40 insertions(+), 2 deletions(-) diff --git a/termstory/backup.py b/termstory/backup.py index 7740d44..fba672a 100644 --- a/termstory/backup.py +++ b/termstory/backup.py @@ -2,9 +2,12 @@ import shutil import sqlite3 import glob +import logging from datetime import datetime from termstory.config import get_db_path +logger = logging.getLogger(__name__) + def _get_backup_dir() -> str: """Return the directory where backups are stored. Creates it if missing.""" @@ -43,8 +46,9 @@ def backup_db() -> str: oldest = backups.pop(0) if os.path.exists(oldest): os.remove(oldest) - except Exception: - pass # Rotation failure should not crash the backup process + except OSError: + # Rotation failure should not crash the backup process, but it must be visible. + logger.exception("Failed to rotate old backups in %s", backup_dir) return backup_path diff --git a/tests/test_backup.py b/tests/test_backup.py index e67a11c..d1364be 100644 --- a/tests/test_backup.py +++ b/tests/test_backup.py @@ -84,3 +84,37 @@ def now(cls): import glob remaining = glob.glob(os.path.join(backup_dir, "termstory_backup_*.db")) assert len(remaining) == 10 + + +def test_backup_survives_rotation_failure(tmp_path, monkeypatch): + db_file = tmp_path / "test_rotation_failure.db" + db_path = str(db_file) + monkeypatch.setenv("DB_PATH", db_path) + monkeypatch.setattr("termstory.config.get_db_path", lambda: db_path) + monkeypatch.setattr("termstory.backup.get_db_path", lambda: db_path) + + db = Database(db_path) + db.init_db() + + class MockDatetime: + counter = 0 + @classmethod + def now(cls): + cls.counter += 1 + from datetime import datetime as dt + return dt(2026, 6, 18, 19, 0, cls.counter) + + monkeypatch.setattr("termstory.backup.datetime", MockDatetime) + + # Fill past the rotation threshold so cleanup runs and hits the failing os.remove. + for _ in range(11): + backup_db() + + def boom(_path): + raise PermissionError("cannot delete backup") + + monkeypatch.setattr("termstory.backup.os.remove", boom) + + # Rotation now fails, but the backup itself must still succeed and return a path. + backup_path = backup_db() + assert os.path.isfile(backup_path) From 4a3c9dd3182ee53203b7d269c23c69ae3ec658fa Mon Sep 17 00:00:00 2001 From: Sai Asish Y Date: Sat, 18 Jul 2026 06:48:48 -0700 Subject: [PATCH 2/2] test: assert rotation failure is logged in backup test Signed-off-by: Sai Asish Y --- tests/test_backup.py | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/tests/test_backup.py b/tests/test_backup.py index d1364be..6bc18e9 100644 --- a/tests/test_backup.py +++ b/tests/test_backup.py @@ -1,3 +1,4 @@ +import logging import os import pytest from termstory.backup import backup_db, restore_db @@ -86,7 +87,7 @@ def now(cls): assert len(remaining) == 10 -def test_backup_survives_rotation_failure(tmp_path, monkeypatch): +def test_backup_survives_rotation_failure(tmp_path, monkeypatch, caplog): db_file = tmp_path / "test_rotation_failure.db" db_path = str(db_file) monkeypatch.setenv("DB_PATH", db_path) @@ -116,5 +117,14 @@ def boom(_path): monkeypatch.setattr("termstory.backup.os.remove", boom) # Rotation now fails, but the backup itself must still succeed and return a path. - backup_path = backup_db() + with caplog.at_level(logging.ERROR, logger="termstory.backup"): + backup_path = backup_db() + assert os.path.isfile(backup_path) + + # The rotation failure must be logged, not silently swallowed. A regression + # back to `except OSError: pass` would drop this record and fail here. + assert any( + "Failed to rotate old backups" in record.message + for record in caplog.records + )