[Backup] Log backup rotation failures instead of silently swallowing them#305
Conversation
…them Signed-off-by: Sai Asish Y <say.apm35@gmail.com>
git-miku
Bot
added
the
needs-triage
git-miku
Bot
added
size/S
|
Tip 👋 Hey @SAY-5 — Miku's on it. Here are the most useful commands for this PR:
Note 🎓 This repo is part of ECSOC26. Run Note ⭐ Star this repo to unlock all commands. Say 📖 All commands🤖 AI-powered — 14 commands
🔧 Issue & PR management — 18 commands
🎉 Community & utility — 9 commands
|
|
| Filename | Overview |
|---|---|
| termstory/backup.py | Upgrades rotation-failure logging from logger.warning (no traceback) to logger.exception (ERROR level + full traceback) and updates the message to include backup_dir; no logic changes to the rotation or backup paths. |
| tests/test_backup.py | Adds test_backup_survives_rotation_failure which correctly seeds 11 backups, patches os.remove to raise PermissionError, and asserts both the backup path and log emission; uses record.message instead of the reliable caplog.messages API for the log assertion. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[backup_db called] --> B[Copy DB via sqlite3.backup API]
B --> C{Copy succeeded?}
C -- No --> D[Exception propagates to caller]
C -- Yes --> E[Glob existing backups, sort by name]
E --> F{Count > 10?}
F -- No --> G[Return backup_path]
F -- Yes --> H[os.remove oldest backup]
H --> I{OSError?}
I -- No --> F
I -- Yes --> J[logger.exception with backup_dir and full traceback]
J --> G
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[backup_db called] --> B[Copy DB via sqlite3.backup API]
B --> C{Copy succeeded?}
C -- No --> D[Exception propagates to caller]
C -- Yes --> E[Glob existing backups, sort by name]
E --> F{Count > 10?}
F -- No --> G[Return backup_path]
F -- Yes --> H[os.remove oldest backup]
H --> I{OSError?}
I -- No --> F
I -- Yes --> J[logger.exception with backup_dir and full traceback]
J --> G
Reviews (3): Last reviewed commit: "Merge remote-tracking branch 'upstream/m..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
/rewrite |
|
miku ✅ PR description rewritten! Check the PR body above. PreviewSummaryThis PR fixes a silent failure in ChangesCore
Tests
Fixes
Breaking ChangesNone TestingRun the backup test suite to verify the fix: python3 -m pytest tests/test_backup.py -vThe new test
NotesThe rotation logic maintains a 10-snapshot policy by deleting the oldest backups when the count exceeds 10 . This change ensures that filesystem errors during cleanup (such as disk-full conditions or permission issues) are now visible in logs while preserving the non-crashing contract that the backup operation itself always succeeds and returns a valid path. |
|
Can you go to the issue and do /claim, please? |
|
I'll triage it by the end of the day. |
bitflicker64
left a comment
bitflicker64
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Approve with minor suggestions — production fix is correct and merge-safe; strengthen the test (and PR body) so logging can’t regress silently.
Intent
Fixes #296: rotation failures were swallowed with except Exception: pass and no logging. This PR narrows the catch to OSError, logs with logger.exception, and keeps the non-crashing backup contract.
Impact
| Area | Behavior |
|---|---|
| Happy path | Unchanged |
| FS errors during rotation | Still non-fatal; now logged at ERROR with traceback |
Non-OSError bugs in the rotation block |
Previously swallowed; now propagate (intentional) |
| Return value | Still returns backup_path when the SQLite backup succeeded |
What’s correct
- Tight scope: one production block + one focused regression test
- Matches the issue’s suggested fix almost verbatim
- Module-level
logger = logging.getLogger(__name__)is standard and testable - Test correctly forces past the rotation threshold before monkeypatching
os.remove - CI green across 3.9–3.12
Warnings
- Test never asserts logging — The core change is “log the failure.” The new test only checks that backup still returns a path. A silent revert to
passwould still pass CI. - PR description overclaims — Body says the test validates that errors are “caught and logged,” but logging is not asserted. Please align the description with what the test actually checks (or add the assertion below).
Suggestions
See inline comments. Summary:
- Add a
caplogassertion forlogger.exception - Optionally assert backup count stays
> 10after the failing rotation - Nice-to-have: DRY
MockDatetime; magic10→ named constant is fine as a follow-up (as the issue already notes)
Not blocking
- Keep
logger.exception(not barelogger.error) - Keep
OSError(do not re-widen toException) - Do not make rotation failures raise — callers rely on the non-crashing contract
Review posted from local review session
|
|
||
| # Rotation now fails, but the backup itself must still succeed and return a path. | ||
| backup_path = backup_db() | ||
| assert os.path.isfile(backup_path) |
There was a problem hiding this comment.
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):.)
| raise PermissionError("cannot delete backup") | ||
|
|
||
| monkeypatch.setattr("termstory.backup.os.remove", boom) | ||
|
|
There was a problem hiding this comment.
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 deletingNice-to-have, not blocking.
| os.remove(oldest) | ||
| except Exception: | ||
| pass # Rotation failure should not crash the backup process | ||
| except OSError: |
There was a problem hiding this comment.
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.
|
/triage |
git-miku
Bot
added
triage/triaged
|
miku ✅ Marked as triaged ( |
Signed-off-by: Sai Asish Y <say.apm35@gmail.com>
|
Good catch, added the caplog assertion so the test now fails if the rotation error stops being logged. Also aligned the PR description with what the test checks. Left the count-remains check out to keep the test focused, and kept OSError / logger.exception as-is. All three backup tests pass locally. |
|
:miku /check-star |
|
miku @SAY-5 still hasn't starred this repo. Star it first, then run |
bitflicker64
added
the
do-not-merge/needs-star
|
:miku /check-star |
|
:miku /check-star |
|
u can star it till the end of ecsoc thing |
bitflicker64
left a comment
bitflicker64
left a comment
There was a problem hiding this comment.
Resolve conflicts, please.
…ion-logging Signed-off-by: Sai Asish Y <say.apm35@gmail.com> # Conflicts: # termstory/backup.py
|
👋 Thanks for the PR, @SAY-5! To get this PR reviewed, please ⭐ star this repo — it's free and helps others discover it. Once starred, run :miku /check-star on this PR and I'll start helping out. A |
|
Rebased on main and resolved the conflict in backup.py, keeping logger.exception as you asked. The rotation test already asserts the failure is logged (the caplog check for "Failed to rotate old backups"), so a silent revert to pass would fail it. All backup tests pass locally. |
|
:miku /review |
|
miku This is a clean fix that addresses the silent failure issue properly. The change from The new test One small nit: the PR description mentions adding NotesThe backup rotation logic maintains a 10-snapshot policy by deleting the oldest backups when the count exceeds 10 . This change ensures filesystem errors during cleanup are now visible in logs while preserving the non-crashing contract that the backup operation itself always succeeds and returns a valid path. |
2 participants
Summary
This PR fixes a silent failure in
backup_db()'s rotation block whereexcept Exception: passswallowed all errors without logging, potentially causing unbounded backup accumulation when disk-full, permission errors, or filesystem issues occurred. The change narrows the exception handler toOSError(which coversPermissionErrorandFileNotFoundError) and addslogger.exception()to surface failures while preserving the guarantee that rotation errors never crash the backup process.Changes
Core
import loggingand module-levellogger = logging.getLogger(__name__)for error loggingexcept Exception:to specificexcept OSError:to catch filesystem-related errors while allowing other exceptions to propagatepasswithlogger.exception("Failed to rotate old backups in %s", backup_dir)to log rotation failures with full tracebackTests
test_backup_survives_rotation_failure()to verify that backup operations succeed even when rotation fails due toPermissionErrorMockDatetimeto create 11 backups (exceeding the 10-backup rotation threshold) and mocksos.removeto raisePermissionError, then asserts the backup still returns a valid file pathcaplogand asserts a "Failed to rotate old backups" ERROR record is emitted, so a regression back toexcept OSError: passwould fail the testFixes
Breaking Changes
None
Testing
Run the backup test suite to verify the fix:
The new test
test_backup_survives_rotation_failurespecifically validates that:caplog) without crashing the backup operationNotes
The rotation logic maintains a 10-snapshot policy by deleting the oldest backups when the count exceeds 10 . This change ensures that filesystem errors during cleanup (such as disk-full conditions or permission issues) are now visible in logs while preserving the non-crashing contract that the backup operation itself always succeeds and returns a valid path.