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 <noreply@anthropic.com>

* 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 <noreply@anthropic.com>

---------

Co-authored-by: Maxim Shulga <maxim.shulga@alludo.com>
Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>

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.
+- [x] Compare ISO year and ISO week together.
-- [ ] Generate and validate a retention plan before deleting anything.
+- [x] Generate and validate a retention plan before deleting anything.
-- [ ] Run retention only after a new snapshot is durable and complete.
+- [x] Run retention only after a new snapshot is durable and complete.
-- [ ] Quarantine deletion candidates before recursive removal.
+- [x] Quarantine deletion candidates before recursive removal.
-- [ ] Convert the existing retention `xfail` cases into passing tests.
+- [x] Convert the existing retention `xfail` cases into passing tests.
 
 Acceptance criteria:
 

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]
+    for backup in all_backups:
-    to_remove = {b: False for b in all_backups}
+        if backup in keep:
-
-    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:
             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):
+        _quarantine_and_delete(backup, backups_dir_abs, repo_id)
-            continue
+
-        _lg.info("Removing old backup %s", backup.name)
+
-        shutil.rmtree(backup.path)
+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,

curateipsum/cli.py

@@ -105,8 +105,11 @@ def main():
 
     exit_code = 0
     try:
-        backup.cleanup_old_backups(backups_dir=backups_dir_abs,
+        # Retention deletes old snapshots, so it must only run once the
-                                   dry_run=args.dry_run)
+        # 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

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],
+        expected_backups = [backups[0], backups[2], backups[4],
-                            backups[9], backups[12]]
+                            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
+        # Keep one week: should keep oldest backup from ISO week 53, plus
-        expected = [backups[5]]  # 20201228
+        # 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)
+        # Keep weekly: oldest from week 1 (Dec 30) and week 2 (Jan 8), plus
-        expected = [backups[0], backups[4]]
+        # 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)

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):
+    def test_calls_initiate_before_cleanup(self, tmp_path):
-        """Should call cleanup_old_backups before initiate_backup."""
+        """
+        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."""