snegov/cura-te-ipsum
1
0 Fork 0
Code Issues 20 Releases Wiki Activity

Correct retention before enabling automatic deletion (#8)

Browse Source 
* 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>
This commit is contained in:
snegovClaude Sonnet 5Maxim Shulga
 authored and GitHub committed 2026-08-03 22:47:43 +00:00
1 parent 72a8fe2b59
commit fd613edad0
5 files changed
+209 -75
Download Patch File Download Diff File Expand all files Collapse all files

No files matched your search

IMPROVEMENTS.md
+6 -6
View File
@@ -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:
curateipsum/backup.py
+106 -49
View File
@@ -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,
curateipsum/cli.py
+7 -2
View File
@@ -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
tests/test_backups.py
+55 -15
View File
@@ -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)
tests/test_cli.py
+35 -3
View File
@@ -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."""