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

Make snapshot creation transactional (#5)

Browse Source 
* Make snapshot creation transactional

Build each snapshot in a private .incomplete-<name> staging directory
instead of mutating the final backup path directly. Any failure while
building the snapshot (hardlink failure, rsync error, or a single
Actions.ERROR entry) is now fatal: the staging directory is discarded
and BackupFailedError is raised, so a failed or interrupted backup can
never silently look like a success.

On success, the completion marker is written and the directory fsync'd
before an atomic os.rename() into the final snapshot name, so a crash
can never leave a partially-populated directory under a real snapshot
name.

Also:
- cleanup_stale_staging_dirs() removes abandoned staging directories
  left by a crashed run before a new backup starts.
- _reserve_snapshot_name() avoids same-second snapshot name collisions.
- cli.py releases the backup lock in finally and returns a nonzero
  exit code when BackupFailedError is raised.

Updates the error-recovery tests that previously asserted failures
were swallowed silently to assert BackupFailedError is raised instead,
matching the new fail-loud contract.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

* Address Copilot review feedback on transactional snapshots

- Remove unused uuid import.
- _reserve_snapshot_name() also checks for a collision with an existing
  .incomplete-<name> staging directory, not just the final snapshot
  name.
- _fsync_dir() opens with O_DIRECTORY where available.
- Fsync the completion marker file itself, and fsync the parent
  backups_dir after the rename, so the marker and the rename survive a
  crash/power loss.
- Re-raise an already-BackupFailedError unchanged (preserving its
  traceback) instead of wrapping it in a new instance; only
  fs.BackupCreationError gets wrapped.
- Wrap the initial os.mkdir(staging.path) failure in BackupFailedError
  so every failure path uses the documented exception.

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 21:30:39 +00:00
1 parent 48ebb7f9ae
commit a48eaf8117
5 files changed
+540 -74
Download Patch File Download Diff File Expand all files Collapse all files

No files matched your search

tests/test_cli.py
+27
View File
@@ -441,6 +441,33 @@ class TestLockHandling:
m_release.assert_called_once_with(str(backups_dir))
def test_releases_lock_and_exits_nonzero_after_failed_backup(
self, tmp_path):
"""Should still release lock and return nonzero exit code when
initiate_backup raises BackupFailedError."""
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'):
with mock.patch(
'curateipsum.backup.initiate_backup',
side_effect=backup.BackupFailedError("boom")):
with mock.patch(
'curateipsum.backup.release_backups_lock') \
as m_release:
result = cli.main()
assert result == 1
m_release.assert_called_once_with(str(backups_dir))
class TestBackupExecution:
"""Test backup execution flow."""
tests/test_integration.py
+110 -23
View File
@@ -581,18 +581,24 @@ class TestErrorRecovery:
monkeypatch.setattr(fs, "hardlink_dir", failing_hardlink_dir)
# Try to create second backup (should fail)
bk.initiate_backup(
sources=[str(source_dir)],
backups_dir=str(backups_dir),
dry_run=False
)
# Try to create second backup (should fail loudly, never silently)
with pytest.raises(bk.BackupFailedError):
bk.initiate_backup(
sources=[str(source_dir)],
backups_dir=str(backups_dir),
dry_run=False
)
# Only original backup should exist (failed backup cleaned up)
backups = [b for b in os.listdir(str(backups_dir))
if not b.startswith(".")]
assert len(backups) == 1
# No abandoned staging directory should remain either
staging_dirs = [d for d in os.listdir(str(backups_dir))
if d.startswith(bk.STAGING_PREFIX)]
assert staging_dirs == []
# Verify the remaining backup is the original one
backup_path = os.path.join(str(backups_dir), backups[0])
source_name = os.path.basename(str(source_dir))
@@ -632,18 +638,96 @@ class TestErrorRecovery:
monkeypatch.setattr(fs, "rsync", failing_rsync)
# Try to create second backup (should fail and clean up)
# Try to create second backup (should fail loudly and clean up)
with pytest.raises(bk.BackupFailedError):
bk.initiate_backup(
sources=[str(source_dir)],
backups_dir=str(backups_dir),
dry_run=False
)
# Only original backup should exist
backups = [b for b in os.listdir(str(backups_dir))
if not b.startswith(".")]
assert len(backups) == 1
def test_entry_level_error_action_is_fatal(
self, integration_dirs, monkeypatch
):
"""A single Actions.ERROR from rsync must fail the whole backup,
never be treated as a partial success."""
backups_dir, source_dir = integration_dirs
(source_dir / "file1.txt").write_text("content1")
bk.initiate_backup(
sources=[str(source_dir)],
backups_dir=str(backups_dir),
dry_run=False
)
# Only original backup should exist
time.sleep(1.1)
(source_dir / "file2.txt").write_text("content2")
from curateipsum import fs
def erroring_rsync(src, dst, dry_run=False):
yield "file2.txt", fs.Actions.ERROR, "simulated copy error"
monkeypatch.setattr(fs, "rsync", erroring_rsync)
with pytest.raises(bk.BackupFailedError):
bk.initiate_backup(
sources=[str(source_dir)],
backups_dir=str(backups_dir),
dry_run=False
)
backups = [b for b in os.listdir(str(backups_dir))
if not b.startswith(".")]
assert len(backups) == 1
def test_abandoned_staging_dir_is_cleaned_up_on_startup(
self, integration_dirs
):
"""Stale .incomplete-* staging dirs from a crashed run must be
quarantined/removed before a new backup starts, and must never be
mistaken for a real backup."""
backups_dir, source_dir = integration_dirs
(source_dir / "file1.txt").write_text("content1")
stale_staging = backups_dir / f"{bk.STAGING_PREFIX}deadbeef"
stale_staging.mkdir()
(stale_staging / "leftover.txt").write_text("leftover")
bk.initiate_backup(
sources=[str(source_dir)],
backups_dir=str(backups_dir),
dry_run=False
)
entries = os.listdir(str(backups_dir))
staging_dirs = [d for d in entries if d.startswith(bk.STAGING_PREFIX)]
assert staging_dirs == []
real_backups = [b for b in entries if not b.startswith(".")]
assert len(real_backups) == 1
def test_successful_backup_leaves_no_staging_dir(self, integration_dirs):
"""A completed backup must be renamed atomically into place, no
.incomplete-* directory should be left behind."""
backups_dir, source_dir = integration_dirs
(source_dir / "file1.txt").write_text("content1")
bk.initiate_backup(
sources=[str(source_dir)],
backups_dir=str(backups_dir),
dry_run=False
)
entries = os.listdir(str(backups_dir))
staging_dirs = [d for d in entries if d.startswith(bk.STAGING_PREFIX)]
assert staging_dirs == []
def test_incomplete_backup_without_marker(self, integration_dirs):
"""Test that backups without marker are not counted as valid"""
backups_dir, source_dir = integration_dirs
@@ -755,11 +839,12 @@ class TestErrorRecovery:
try:
# Backup should fail due to hardlink failure
bk.initiate_backup(
sources=[str(source_dir)],
backups_dir=str(backups_dir),
dry_run=False
)
with pytest.raises(bk.BackupFailedError):
bk.initiate_backup(
sources=[str(source_dir)],
backups_dir=str(backups_dir),
dry_run=False
)
finally:
# Lock should still exist (we manually acquired it)
assert os.path.exists(lock_path)
@@ -819,11 +904,12 @@ class TestErrorRecovery:
try:
# Backup should fail
bk.initiate_backup(
sources=[str(source_dir)],
backups_dir=str(backups_dir),
dry_run=False
)
with pytest.raises(bk.BackupFailedError):
bk.initiate_backup(
sources=[str(source_dir)],
backups_dir=str(backups_dir),
dry_run=False
)
finally:
# Lock still exists (we manually acquired it)
assert os.path.exists(lock_path)
@@ -869,11 +955,12 @@ class TestErrorRecovery:
monkeypatch.setattr(fs, "rsync", failing_rsync)
# Run backup (will fail and attempt cleanup)
bk.initiate_backup(
sources=[str(source_dir)],
backups_dir=str(backups_dir),
dry_run=False
)
with pytest.raises(bk.BackupFailedError):
bk.initiate_backup(
sources=[str(source_dir)],
backups_dir=str(backups_dir),
dry_run=False
)
# Verify cleanup was attempted (rmtree was called)
assert len(rmtree_called) > 0