From fd613edad021658bb410110bff1d4346f05764d5 Mon Sep 17 00:00:00 2001 From: Maksim Shulga Date: Mon, 3 Aug 2026 22:47:43 +0000 Subject: [PATCH] Correct retention before enabling automatic deletion (#8) * Correct retention before enabling automatic deletion - Compute each retention tier's kept set independently from its own name range and union them, instead of carrying a single 'previous backup' comparison across tier boundaries - that carry-over was the actual cause of bug #32 (a coarser tier could evict a backup a finer tier, or the always-keep-latest rule, had already decided to retain) - Group weekly retention by (ISO year, ISO week) instead of ISO week number alone, fixing bug #33 (same week number in different years, or late-December dates that belong to next year's ISO week 1, were treated as duplicates/collisions) - Validate the computed retention plan (latest snapshot present, no phantom entries) before deleting anything - Fix cli.py to run initiate_backup before cleanup_old_backups, and skip cleanup entirely if the backup raised - retention must only run once a new snapshot is durable and complete, and a failed backup must never trigger deletion of history - Quarantine (rename) a deletion candidate before recursively removing it, reusing the already-hardened stale-staging-dir cleanup to finish the job if the process crashes mid-delete - Remove both retention xfail markers (bugs #32, #33) now that the tests they guarded pass; update pre-existing tests whose expectations encoded the old bug (deleting the latest snapshot when it collided with the next-newest one in a coarser bucket) Co-Authored-By: Claude Sonnet 5 * Address Copilot review: replace asserts with runtime checks, don't swallow quarantine-delete failures - Retention-plan validation now raises BackupFailedError instead of using assert, which python -O would silently strip ahead of a destructive operation - _quarantine_and_delete no longer passes ignore_errors=True to rmtree; a removal failure is now logged with the quarantine path so operators can find and retry it, instead of silently leaving a hidden directory consuming space Co-Authored-By: Claude Sonnet 5 --------- Co-authored-by: Maxim Shulga Co-authored-by: Claude Sonnet 5 --- IMPROVEMENTS.md | 12 ++-- curateipsum/backup.py | 155 +++++++++++++++++++++++++++++------------- curateipsum/cli.py | 9 ++- tests/test_backups.py | 70 +++++++++++++++---- tests/test_cli.py | 38 ++++++++++- 5 files changed, 209 insertions(+), 75 deletions(-) diff --git a/IMPROVEMENTS.md b/IMPROVEMENTS.md index cc4ecc6..de80766 100644 --- a/IMPROVEMENTS.md +++ b/IMPROVEMENTS.md @@ -89,13 +89,13 @@ Acceptance criteria: ### Correct retention before enabling automatic deletion -- [ ] Compute retained snapshots independently for each policy tier and keep +- [x] Compute retained snapshots independently for each policy tier and keep the union of those sets. -- [ ] Compare ISO year and ISO week together. -- [ ] Generate and validate a retention plan before deleting anything. -- [ ] Run retention only after a new snapshot is durable and complete. -- [ ] Quarantine deletion candidates before recursive removal. -- [ ] Convert the existing retention `xfail` cases into passing tests. +- [x] Compare ISO year and ISO week together. +- [x] Generate and validate a retention plan before deleting anything. +- [x] Run retention only after a new snapshot is durable and complete. +- [x] Quarantine deletion candidates before recursive removal. +- [x] Convert the existing retention `xfail` cases into passing tests. Acceptance criteria: diff --git a/curateipsum/backup.py b/curateipsum/backup.py index 9147606..52acf5a 100644 --- a/curateipsum/backup.py +++ b/curateipsum/backup.py @@ -548,62 +548,119 @@ def cleanup_old_backups(backups_dir: str, .strftime(BACKUP_ENT_FMT) ) + keep = _compute_retention_plan(all_backups, thresholds) + # Validate the plan before deleting anything: the most recent snapshot + # must always survive, and the plan can never invent a backup that + # doesn't exist. Plain `assert` would be stripped under `python -O`, + # silently disabling this check ahead of a destructive operation, so + # this is an explicit, always-on check instead. + if all_backups[0] not in keep: + raise BackupFailedError( + "Refusing to run retention: computed plan drops the latest " + "snapshot %s" % all_backups[0].name + ) + if not keep <= set(all_backups): + raise BackupFailedError( + "Refusing to run retention: computed plan keeps a backup " + "that doesn't exist" + ) + backups_dir_abs = _canonical(backups_dir) repo_id = _ensure_repo_id(backups_dir) - prev_backup = all_backups[0] - to_remove = {b: False for b in all_backups} - - for backup in all_backups[1:]: - # skip all backups made after threshold - if backup.name > thresholds["all"]: - prev_backup = backup - continue - - # leave only one backup per day for backups made after threshold - if backup.name > thresholds["daily"]: - if (_date_from_backup(prev_backup).date() - == _date_from_backup(backup).date()): - to_remove[prev_backup] = True - prev_backup = backup - continue - - # leave only one backup per week for backups made after threshold - if backup.name > thresholds["weekly"]: - if (_date_from_backup(prev_backup).isocalendar()[1] - == _date_from_backup(backup).isocalendar()[1]): - to_remove[prev_backup] = True - prev_backup = backup - continue - - # leave only one backup per month for backups made after threshold - if backup.name > thresholds["monthly"]: - if (_date_from_backup(prev_backup).date().replace(day=1) - == _date_from_backup(backup).date().replace(day=1)): - to_remove[prev_backup] = True - prev_backup = backup - continue - - # leave only one backup per year for backups made after threshold - if backup.name > thresholds["yearly"]: - if (_date_from_backup(prev_backup).date().replace(month=1, day=1) - == _date_from_backup(backup).date().replace(month=1, day=1)): - to_remove[prev_backup] = True - prev_backup = backup - continue - - to_remove[backup] = True - - for backup, do_delete in to_remove.items(): - if not do_delete: + for backup in all_backups: + if backup in keep: continue if dry_run: _lg.info("Would remove old backup %s", backup.name) continue - if not _verify_safe_to_delete(backup, backups_dir_abs, repo_id): - continue - _lg.info("Removing old backup %s", backup.name) - shutil.rmtree(backup.path) + _quarantine_and_delete(backup, backups_dir_abs, repo_id) + + +def _compute_retention_plan(all_backups: List[os.DirEntry], + thresholds: Dict[str, str]) -> set: + """ + Compute the set of backups to retain. Each tier's retained set is + computed independently from its own name range and unioned with the + rest, so a coarser tier (e.g. monthly) can never override a decision + a finer tier (e.g. weekly) already made about a backup that isn't + even in the coarser tier's range - unlike carrying a single "previous + backup compared against" value across tier boundaries, which is what + let a coarser tier silently evict a backup a finer tier had decided + to keep. + """ + def _oldest_per_group(members, key_func) -> set: + groups: Dict[object, os.DirEntry] = {} + for member in members: + key = key_func(member) + if key not in groups or member.name < groups[key].name: + groups[key] = member + return set(groups.values()) + + def _band(lower: str, upper: str) -> List[os.DirEntry]: + return [b for b in all_backups if lower < b.name <= upper] + + keep = {all_backups[0]} # the most recent snapshot is always retained + + keep |= {b for b in all_backups if b.name > thresholds["all"]} + + keep |= _oldest_per_group( + _band(thresholds["daily"], thresholds["all"]), + lambda b: _date_from_backup(b).date(), + ) + + keep |= _oldest_per_group( + _band(thresholds["weekly"], thresholds["daily"]), + # (ISO year, ISO week): ISO week alone collides across years, and + # a year spanning ISO week 53 puts late-December dates in the + # next ISO year - isocalendar() already accounts for both. + lambda b: _date_from_backup(b).isocalendar()[:2], + ) + + keep |= _oldest_per_group( + _band(thresholds["monthly"], thresholds["weekly"]), + lambda b: (_date_from_backup(b).year, _date_from_backup(b).month), + ) + + keep |= _oldest_per_group( + _band(thresholds["yearly"], thresholds["monthly"]), + lambda b: _date_from_backup(b).year, + ) + + return keep + + +def _quarantine_and_delete(entry: os.DirEntry, backups_dir_abs: str, + repo_id: str): + """ + Quarantine a retention deletion candidate before recursively removing + it: rename it out of its snapshot name first. If the process crashes + between the rename and the removal, the quarantined directory keeps + the staging-dir prefix, so the existing stale-staging-dir cleanup - + with the same boundary/mount/symlink safety checks - finishes + removing it on the next run, rather than leaving a half-deleted + directory sitting under its original snapshot name. + """ + if not _verify_safe_to_delete(entry, backups_dir_abs, repo_id): + return + quarantine_path = os.path.join( + backups_dir_abs, "%strash-%s" % (STAGING_PREFIX, entry.name) + ) + try: + os.rename(entry.path, quarantine_path) + except OSError as err: + _lg.error("Failed to quarantine %s before deletion: %s", + entry.path, err) + return + _lg.info("Removing old backup %s", entry.name) + try: + shutil.rmtree(quarantine_path) + except OSError as err: + _lg.error( + "Failed to remove quarantined backup %s: %s. It remains on " + "disk under %s and will be retried on the next cleanup or " + "backup startup.", entry.name, err, quarantine_path + ) def process_backed_entry(backup_dir: str, diff --git a/curateipsum/cli.py b/curateipsum/cli.py index 5877505..6eb20a4 100755 --- a/curateipsum/cli.py +++ b/curateipsum/cli.py @@ -105,8 +105,11 @@ def main(): exit_code = 0 try: - backup.cleanup_old_backups(backups_dir=backups_dir_abs, - dry_run=args.dry_run) + # Retention deletes old snapshots, so it must only run once the + # new snapshot is durable and complete - never before, and never + # if the backup below raised. Running it first (or unconditionally + # after a failure) would let a failed backup destroy history + # that a successful one would have covered. backup.initiate_backup( sources=args.sources, backups_dir=backups_dir_abs, @@ -114,6 +117,8 @@ def main(): external_rsync=args.external_rsync, external_hardlink=args.external_hardlink, ) + backup.cleanup_old_backups(backups_dir=backups_dir_abs, + dry_run=args.dry_run) except backup.BackupFailedError as err: _lg.error("Backup failed: %s", err) exit_code = 1 diff --git a/tests/test_backups.py b/tests/test_backups.py index 61f7bc0..ab854d5 100644 --- a/tests/test_backups.py +++ b/tests/test_backups.py @@ -105,7 +105,7 @@ class TestBackupCleanup: """Test threshold for keeping weekly backups""" mock_datetime.now.return_value = datetime(2021, 11, 11) backups = [ - add_backup("20211111_0300"), # remove, not first weekly (Thu) + add_backup("20211111_0300"), # keep, latest snapshot always kept add_backup("20211110_0300"), # remove, not first weekly (Wed) add_backup("20211108_0100"), # keep, first weekly 2021-11-08 (Mon) add_backup("20211107_2300"), # remove, not first weekly (Sun) @@ -122,8 +122,8 @@ class TestBackupCleanup: add_backup("20211002_0100"), # remove, older than 5 weeks ] - expected_backups = [backups[2], backups[4], backups[6], - backups[9], backups[12]] + expected_backups = [backups[0], backups[2], backups[4], + backups[6], backups[9], backups[12]] run_cleanup(keep_weekly=5) check_backups(expected_backups) @@ -133,12 +133,12 @@ class TestBackupCleanup: """Test threshold for keeping weekly backups""" mock_datetime.now.return_value = datetime(2021, 11, 11) backups = [ - add_backup("20211111_0300"), # remove, not first weekly (Thu) + add_backup("20211111_0300"), # keep, latest snapshot always kept add_backup("20211110_0300"), # keep, first weekly (Wed) add_backup("20211107_0100"), # remove, not first weekly (Sun) add_backup("20211102_0100"), # keep, first weekly (Tue) ] - expected_backups = [backups[1], backups[3]] + expected_backups = [backups[0], backups[1], backups[3]] run_cleanup(keep_weekly=5) check_backups(expected_backups) @@ -169,7 +169,7 @@ class TestBackupCleanup: """Test threshold for keeping yearly backups""" mock_datetime.now.return_value = datetime(2021, 11, 11) backups = [ - add_backup("20211103_0300"), # remove, not first yearly in 2021 + add_backup("20211103_0300"), # keep, latest snapshot always kept add_backup("20210810_0000"), # remove, not first yearly in 2021 add_backup("20210716_0100"), # keep, first yearly in 2021 add_backup("20201216_0100"), # remove, not first yearly in 2020 @@ -181,7 +181,8 @@ class TestBackupCleanup: add_backup("20171116_0100"), # remove, older than 3 years add_backup("20171115_0100"), # remove, older than 3 years ] - expected_backups = [backups[2], backups[4], backups[6], backups[8]] + expected_backups = [backups[0], backups[2], backups[4], backups[6], + backups[8]] run_cleanup(keep_yearly=3) check_backups(expected_backups) @@ -200,8 +201,6 @@ class TestBackupCleanup: run_cleanup(keep_all=2, dry_run=True) check_backups(backups) - @pytest.mark.xfail(reason="Bug #32: coarser retention tiers override finer " - "tiers, violating retention guarantees", strict=True) @mock.patch(f"{bk.__name__}.datetime", wraps=datetime) def test_combined_retention_policies(self, mock_datetime, add_backup, run_cleanup, check_backups): @@ -333,13 +332,12 @@ class TestBackupCleanup: add_backup("20201228_1200"), # Mon, ISO 2020-W53 ] - # Keep one week: should keep oldest backup from ISO week 53 - expected = [backups[5]] # 20201228 + # Keep one week: should keep oldest backup from ISO week 53, plus + # the latest snapshot overall (always kept) + expected = [backups[0], backups[5]] # 20210103, 20201228 run_cleanup(keep_weekly=1) check_backups(expected) - @pytest.mark.xfail(reason="Bug #33: Weekly retention bug: same ISO week " - "number across years treated as duplicates", strict=True) @mock.patch(f"{bk.__name__}.datetime", wraps=datetime) def test_iso_week_number_across_years(self, mock_datetime, add_backup, run_cleanup, check_backups): @@ -383,8 +381,9 @@ class TestBackupCleanup: add_backup("20250112_1200"), # Sun, ISO 2025-W02 ] - # Keep weekly: oldest from week 1 (Dec 30) and week 2 (Jan 8) - expected = [backups[0], backups[4]] + # Keep weekly: oldest from week 1 (Dec 30) and week 2 (Jan 8), plus + # the latest snapshot overall (always kept) + expected = [backups[0], backups[4], backups[5]] run_cleanup(keep_weekly=10) check_backups(expected) @@ -931,3 +930,44 @@ class TestDeletionBoundarySafety: run_cleanup(keep_all=0) assert not os.path.exists(old.path) + + +class TestRetentionPlan: + """Tests for the independent-per-tier retention plan computation.""" + + def test_latest_snapshot_always_retained_even_if_grouped_with_next( + self, backup_dir, add_backup, run_cleanup + ): + """ + Regression for the bug where the latest snapshot could be + evicted if it shared a weekly/monthly/yearly bucket with the + next-newest one, because retention carried a single "previous + backup" comparison across tier boundaries. + """ + latest = add_backup("20211111_0300") + add_backup("20211110_0300") # same ISO week as latest + add_backup("20211004_0100") + + run_cleanup(keep_weekly=5) + + assert os.path.isdir(latest.path) + + def test_quarantined_dir_recovered_by_stale_staging_cleanup( + self, backup_dir, add_backup + ): + """ + If a crash happens between quarantine (rename) and the recursive + delete, the quarantined directory must still get cleaned up on + the next startup via cleanup_stale_staging_dirs. + """ + add_backup("20220101_0000") + old = add_backup("20200101_0000") + + quarantine_path = os.path.join( + str(backup_dir), f"{bk.STAGING_PREFIX}trash-{old.name}" + ) + os.rename(old.path, quarantine_path) + + bk.cleanup_stale_staging_dirs(str(backup_dir)) + + assert not os.path.exists(quarantine_path) diff --git a/tests/test_cli.py b/tests/test_cli.py index ab026c5..6cf9587 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -466,12 +466,44 @@ class TestLockHandling: assert result == 1 m_release.assert_called_once_with(str(backups_dir)) + def test_skips_cleanup_when_backup_fails(self, tmp_path): + """ + A failed backup must prevent every retention deletion: if + initiate_backup raises, cleanup_old_backups must never run. + """ + backups_dir = tmp_path / "backups" + backups_dir.mkdir() + source = tmp_path / "src" + source.mkdir() + + from curateipsum import backup + + with mock.patch('sys.argv', ['cura-te-ipsum', '-b', + str(backups_dir), str(source)]): + with mock.patch('curateipsum.backup.set_backups_lock', + return_value=True): + with mock.patch( + 'curateipsum.backup.cleanup_old_backups') \ + as m_cleanup: + with mock.patch( + 'curateipsum.backup.initiate_backup', + side_effect=backup.BackupFailedError("boom")): + with mock.patch( + 'curateipsum.backup.release_backups_lock'): + cli.main() + + m_cleanup.assert_not_called() + class TestBackupExecution: """Test backup execution flow.""" - def test_calls_cleanup_before_initiate(self, tmp_path): - """Should call cleanup_old_backups before initiate_backup.""" + def test_calls_initiate_before_cleanup(self, tmp_path): + """ + Should call initiate_backup before cleanup_old_backups: retention + deletes old snapshots, so it must only run once the new snapshot + is durable, never before it. + """ backups_dir = tmp_path / "backups" backups_dir.mkdir() source = tmp_path / "src" @@ -497,7 +529,7 @@ class TestBackupExecution: 'curateipsum.backup.release_backups_lock'): cli.main() - assert call_order == ['cleanup', 'initiate'] + assert call_order == ['initiate', 'cleanup'] def test_passes_correct_arguments_to_initiate_backup(self, tmp_path): """Should pass all required arguments to initiate_backup."""