Skip to content

[Backup] Log backup rotation failures instead of silently swallowing them #305

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
bitflicker64 merged 3 commits into from
Jul 20, 2026
Merged

[Backup] Log backup rotation failures instead of silently swallowing them #305

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
83129f1
[Backup] Log backup rotation failures instead of silently swallowing …
SAY-5 Jul 18, 2026
4a3c9dd
test: assert rotation failure is logged in backup test
SAY-5 Jul 18, 2026
f42094e
Merge remote-tracking branch 'upstream/main' into bugfix/backup-rotat…
SAY-5 Jul 20, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions termstory/backup.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import logging
import os
import shutil

Check failure on line 3 in termstory/backup.py

View workflow job for this annotation

GitHub Actions / Lint

ruff (F401) 

termstory/backup.py:3:8: F401 `shutil` imported but unused
  help: Remove unused import: `shutil`
import sqlite3
import glob
from datetime import datetime
Expand Down Expand Up @@ -46,8 +46,9 @@
oldest = backups.pop(0)
if os.path.exists(oldest):
os.remove(oldest)
except OSError as e:
logger.warning("Failed to rotate old backups: %s", e)
except OSError:

@bitflicker64 bitflicker64 Jul 18, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. OSError correctly covers PermissionError / FileNotFoundError / common FS failures, and logger.exception surfaces a traceback without failing the backup.

One intentional behavior change to be aware of: non-OSError exceptions in this block (e.g. unexpected bugs) now propagate instead of being swallowed. That’s the right tradeoff — just noting it so reviewers don’t treat this as a pure observability-only diff.

# 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

Expand Down
44 changes: 44 additions & 0 deletions tests/test_backup.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import logging
import os
import pytest
from termstory.backup import backup_db, restore_db
Expand Down Expand Up @@ -84,3 +85,46 @@ 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, caplog):
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)

@bitflicker64 bitflicker64 Jul 18, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optional stronger assertion: After the failing rotation, also check that cleanup did not complete, so the test isn’t only checking that a new backup file exists:

from termstory.backup import _get_backup_dir
import glob

remaining = glob.glob(os.path.join(_get_backup_dir(), "termstory_backup_*.db"))
assert len(remaining) > 10  # rotation did not finish deleting

Nice-to-have, not blocking.

# Rotation now fails, but the backup itself must still succeed and return a path.
with caplog.at_level(logging.ERROR, logger="termstory.backup"):
backup_path = backup_db()

assert os.path.isfile(backup_path)

@bitflicker64 bitflicker64 Jul 18, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: Assert that the failure is actually logged. Right now this only proves the backup still returns a path — a regression back to except OSError: pass would still pass.

Also, the PR body currently claims this test verifies errors are “caught and logged”; that claim is only true if we lock logging in here.

import logging

# ... setup / MockDatetime / 11 seed backups / monkeypatch os.remove ...

with caplog.at_level(logging.ERROR, logger="termstory.backup"):
    backup_path = backup_db()

assert os.path.isfile(backup_path)
assert any(
    "Failed to rotate old backups" in r.message
    for r in caplog.records
)

(Add caplog to the test fixture list: def test_backup_survives_rotation_failure(tmp_path, monkeypatch, caplog):.)


# 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
)
Loading