From a48eaf8117a178e950d82fb7ebad2c2062ca109a Mon Sep 17 00:00:00 2001 From: Maksim Shulga Date: Mon, 3 Aug 2026 21:30:39 +0000 Subject: [PATCH] Make snapshot creation transactional (#5) * Make snapshot creation transactional Build each snapshot in a private .incomplete- 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 * Address Copilot review feedback on transactional snapshots - Remove unused uuid import. - _reserve_snapshot_name() also checks for a collision with an existing .incomplete- 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 --------- Co-authored-by: Maxim Shulga Co-authored-by: Claude Sonnet 5 --- IMPROVEMENTS.md | 242 ++++++++++++++++++++++++++++++++++++++ curateipsum/backup.py | 183 ++++++++++++++++++++++------ curateipsum/cli.py | 29 +++-- tests/test_cli.py | 27 +++++ tests/test_integration.py | 133 +++++++++++++++++---- 5 files changed, 540 insertions(+), 74 deletions(-) create mode 100644 IMPROVEMENTS.md diff --git a/IMPROVEMENTS.md b/IMPROVEMENTS.md new file mode 100644 index 0000000..002d09d --- /dev/null +++ b/IMPROVEMENTS.md @@ -0,0 +1,242 @@ +# Improvement Plan + +This project manipulates backup data, so correctness and recovery guarantees +come before packaging or profile presentation. The current implementation has +known cases where a failed or incomplete backup can be treated as successful +and where an older snapshot can be modified indirectly. + +Until the P0 items are complete, the README should describe the project as +experimental and should not claim that it is safe for production backups. + +## P0 - Data-safety requirements + +### Make snapshot creation transactional + +- [x] Build each snapshot in a unique `.incomplete-` staging directory. +- [x] Treat every copy error, parse error, unsupported entry, and nonzero + subprocess exit as fatal. +- [x] Return structured results or raise one documented backup exception. +- [x] Write and `fsync` a manifest only after every source succeeds. +- [x] Atomically rename the staging directory to its final snapshot name. +- [x] Release the backup lock in `finally` and return a nonzero CLI exit code. +- [x] Quarantine or clean abandoned staging directories safely on startup. +- [x] Prevent same-second snapshot-name collisions. + +Acceptance criteria: + +- Injected failures never leave a final snapshot or completion marker. +- The previous completed snapshot remains unchanged. +- A failed run exits nonzero, releases its lock, and can be retried. + +### Replace PID-file process killing with a real lock + +- [ ] Use `fcntl.flock()` and hold the descriptor for the full operation. +- [ ] Keep PID and process-start metadata for diagnostics only. +- [ ] Remove process killing from `--force` behavior. +- [ ] Verify lock ownership before releasing or replacing metadata. + +Acceptance criteria: + +- Multiprocess tests prove that only one backup or cleanup can run. +- Stale metadata and PID reuse cannot terminate an unrelated process. +- Kernel lock release works after normal exit, signals, and crashes. + +### Preserve symlinks and old-snapshot immutability + +- [ ] Classify entries with `lstat()` and process symlinks before directories. +- [ ] Never follow symlinks while applying symlink metadata. +- [ ] Break shared hardlinks before changing mode, ownership, flags, or times. +- [ ] Define copy-on-write invariants for every mutation. + +Acceptance criteria: + +- Relative, absolute, broken, file, and directory symlinks remain symlinks. +- External symlink targets are never modified. +- Content and metadata in every older snapshot remain unchanged. + +### Validate topology and destructive path boundaries + +- [ ] Canonicalize source and backup paths and reject every overlap. +- [ ] Reject duplicate sources and destination basename collisions, or require + explicit source labels. +- [ ] Require normalized relative paths for nested operations. +- [ ] Verify `commonpath` before every write, rename, or deletion. +- [ ] Give each backup repository an identity recorded in its manifest. +- [ ] Validate repository identity, snapshot type, marker, and device before + recursive deletion. +- [ ] Refuse to cross mount points during deletion. + +Acceptance criteria: + +- Tests reject output inside source, source inside output, aliases, absolute + paths, parent traversal, duplicate basenames, and forged markers. +- Bind mounts and nested mount points cannot cause external data deletion. + +### Prevent stale, truncated, or changing-file copies + +- [ ] Handle partial writes correctly or use a proven copy primitive. +- [ ] Use nanosecond timestamps and a documented integrity strategy. +- [ ] Add checksum verification, either always or through an explicit mode. +- [ ] Stat files before and after copying and retry or fail if they changed. +- [ ] Record integrity information in the completion manifest. + +Acceptance criteria: + +- Tests cover partial writes and files changed, replaced, truncated, or renamed + during copying. +- Same-size content changes with restored timestamps cannot be missed silently. +- A completed snapshot passes manifest verification. + +### Correct retention before enabling automatic deletion + +- [ ] 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. + +Acceptance criteria: + +- Property tests prove that the latest snapshot and every tier guarantee are + preserved. +- Backup failure prevents every retention deletion. +- Cross-year weekly retention behaves correctly. + +### Handle special filesystem entries explicitly + +- [ ] Distinguish regular files, directories, symlinks, sockets, FIFOs, block + devices, and character devices. +- [ ] Never open an unsupported entry as a regular file. +- [ ] Define a fail or recorded-exclusion policy for every unsupported type. +- [ ] Record exclusions in the snapshot manifest. + +Acceptance criteria: + +- A FIFO without a writer cannot block the backup. +- Tests cover sockets and platform-available device types. +- Unsupported entries produce a visible result and nonzero exit when required. + +### Add an immediate safety warning + +- [ ] Remove the current production-use claim from `README.md`. +- [ ] State that the software is experimental until the P0 release gate passes. +- [ ] Warn users not to modify hardlinked snapshots in place. +- [ ] Document current recovery and metadata limitations honestly. + +## P1 - Recovery and release readiness + +### Provide a safe restoration workflow + +- [ ] Implement a restore command or a complete tested restore procedure. +- [ ] Support dry-run, partial restore, overwrite policy, and destination + boundary checks. +- [ ] Preserve supported metadata and verify restored content against the + manifest. +- [ ] Test restoration from current and older snapshots. + +### Define filesystem compatibility + +- [ ] Document behavior for ACLs, xattrs, flags, sparse files, source hardlinks, + ownership, permissions, and timestamps. +- [ ] Add an explicit one-filesystem policy. +- [ ] Make Python and external implementations provide equivalent guarantees or + document every difference. +- [ ] Test supported behavior on Linux and macOS. + +### Make dry-run genuinely non-mutating + +- [ ] Separate operation planning from execution. +- [ ] Ensure dry-run creates no files, hardlinks, locks, metadata updates, or + temporary snapshots. +- [ ] Print proposed copy, exclusion, and deletion operations. +- [ ] Expose validated retention policy through CLI options or configuration. + +### Strengthen tests and CI + +- [ ] Run CI on pull requests as well as pushes. +- [ ] Add macOS CI because macOS is an advertised platform. +- [ ] Fix the Unix-socket path test that currently fails on a local macOS run. +- [ ] Replace nonexistent issue references in `xfail` markers with real tracked + work, then remove the markers as fixes land. +- [ ] Add subprocess-based concurrency, signal, interruption, and fault tests. +- [ ] Add filesystem-boundary and build/install smoke tests. +- [ ] Fix tests that change global state or assert against the wrong path. + +Acceptance criteria: + +- All known correctness defects have normal passing regression tests. +- Linux and macOS CI pass without unexplained skips or xfails. + +### Repair packaging and releases + +- [ ] Make source archives build without a `.git` directory. +- [ ] Test wheel and source distribution installation in clean environments. +- [ ] Commit a reproducible development dependency lock or define another + deterministic policy. +- [ ] Correct OS classifiers and license metadata in `pyproject.toml`. +- [ ] Decide whether PyPI publication is supported and document the decision. +- [ ] Publish GitHub Releases with checksums and release notes. + +Acceptance criteria: + +- Clean clone, source archive, sdist, and wheel builds all install successfully. +- Each installation can report its version and complete a smoke backup. + +### Correct operational documentation + +- [ ] Fix the timestamp format, cleanup order, and marker names in `README.md`. +- [ ] Replace the deleted `requirements-dev.txt` installation command. +- [ ] Correct the documented Python matrix to 3.8 through 3.14. +- [ ] Document failure semantics, restore steps, immutable snapshots, mount + behavior, and implementation differences. +- [ ] Generate or validate examples from integration tests. + +## P2 - Maintainability and public presentation + +### Separate safety-critical responsibilities + +- [ ] Separate planning, copying, locking, retention, and snapshot commit logic. +- [ ] Define typed operation plans and backend interfaces. +- [ ] Share safety checks between Python and external implementations. +- [ ] Add backend contract tests. + +### Establish scalability expectations + +- [ ] Avoid retaining every source directory entry in memory, or document the + supported scale explicitly. +- [ ] Benchmark runtime and peak memory on large and deeply nested trees. +- [ ] Publish the benchmark method and expected operating envelope. + +### Improve repository maintenance signals + +- [ ] Create real issues for every known defect and remove stale annotations. +- [ ] Protect the default branch and require pull-request CI. +- [ ] Add a changelog before the next release. +- [ ] Add concise security and contribution guidance. +- [ ] Align the package author name with the professional profile identity. +- [ ] Replace unclear CLI wording and validate that sources are directories. + +### Improve the public project page + +- [ ] Add a concise repository description and relevant topics. +- [ ] Add a small architecture diagram showing planning, copy, commit, and + retention phases. +- [ ] Add a safety-guarantee and limitations table. +- [ ] Add CI and release badges only after those signals are trustworthy. + +## Release gate + +Do not publish a stable release until all of the following are true: + +- [ ] Failed or interrupted backups cannot be marked complete. +- [ ] Previous snapshots are immutable in content and metadata. +- [ ] Locking cannot race or signal unrelated processes. +- [ ] Source, destination, mount, and deletion boundaries are enforced. +- [ ] Retention cannot delete required snapshots or run after backup failure. +- [ ] Unsupported files cannot block or disappear silently. +- [ ] A documented restore has been tested end to end. +- [ ] Linux and macOS CI cover failure and recovery paths. +- [ ] Installable release artifacts work without repository metadata. +- [ ] The README describes tested guarantees and remaining limitations. diff --git a/curateipsum/backup.py b/curateipsum/backup.py index 2097fac..46e930e 100644 --- a/curateipsum/backup.py +++ b/curateipsum/backup.py @@ -16,9 +16,20 @@ BACKUP_ENT_FMT = "%Y%m%d_%H%M%S" LOCK_FILE = ".backups_lock" DELTA_DIR = ".backup_delta" BACKUP_MARKER = ".backup_finished" +STAGING_PREFIX = ".incomplete-" _lg = logging.getLogger(__name__) +class BackupFailedError(Exception): + """Raised when a backup could not be completed successfully. + + Any failure while building a snapshot (copy error, parse error, + unsupported entry, nonzero subprocess exit) is fatal: the partially + built snapshot is discarded and this is the only way that failure is + reported to callers. + """ + + def _get_backup_marker( backup_entry: Union[os.DirEntry, fs.PseudoDirEntry] ) -> fs.PseudoDirEntry: @@ -182,6 +193,51 @@ def set_backup_marker(backup_entry: Union[os.DirEntry, fs.PseudoDirEntry]): open(backup_marker.path, "a").close() +_O_DIRECTORY = getattr(os, "O_DIRECTORY", 0) + + +def _fsync_dir(path: str): + """Fsync a directory so its content is durable before it is renamed.""" + dir_fd = os.open(path, os.O_RDONLY | _O_DIRECTORY) + try: + os.fsync(dir_fd) + finally: + os.close(dir_fd) + + +def _reserve_snapshot_name(backups_dir: str) -> str: + """ + Return a snapshot name that doesn't collide with an existing entry or + an in-progress staging directory. Guards against two backups started + within the same second. + """ + def _taken(candidate: str) -> bool: + return (os.path.lexists(os.path.join(backups_dir, candidate)) + or os.path.lexists(os.path.join( + backups_dir, f"{STAGING_PREFIX}{candidate}"))) + + name = datetime.now().strftime(BACKUP_ENT_FMT) + while _taken(name): + time.sleep(1) + name = datetime.now().strftime(BACKUP_ENT_FMT) + return name + + +def cleanup_stale_staging_dirs(backups_dir: str): + """ + Remove staging directories abandoned by a crashed or interrupted + backup run. Must be called before a new backup starts so a stale + staging directory can never be mistaken for a real snapshot. + """ + with os.scandir(backups_dir) as it: + entry: os.DirEntry + for entry in it: + if entry.name.startswith(STAGING_PREFIX) and entry.is_dir(): + _lg.warning("Removing abandoned staging directory: %s", + entry.name) + shutil.rmtree(entry.path, ignore_errors=True) + + def cleanup_old_backups(backups_dir: str, dry_run: bool = False, keep_all: int = 7, @@ -323,88 +379,137 @@ def initiate_backup(sources, backups_dir: str, dry_run: bool = False, external_rsync: bool = False, - external_hardlink: bool = False): + external_hardlink: bool = False + ) -> Optional[fs.PseudoDirEntry]: """ Main backup function. - Creates a new backup directory, copies data from the latest backup, - and then syncs data from sources. + Builds the new snapshot in a private ".incomplete-" staging + directory, copying data from the latest backup and then syncing data + from sources. Any failure is fatal: the staging directory is discarded + and BackupFailedError is raised, so a failed or interrupted backup can + never be mistaken for a successful one. On success, the staging + directory is fsync'd and atomically renamed into its final snapshot + name. :param sources: list of directories to backup (relative paths ok) :param backups_dir: directory where all backups are stored :param dry_run: if True, no actual changes will be made :param external_rsync: if True, use external rsync instead of python :param external_hardlink: if True, use external hardlink instead of python + :return: DirEntry-like object of the created snapshot, or None if no + snapshot was created (dry-run or no changes since last backup). + :raises BackupFailedError: if the backup could not be completed. """ + cleanup_stale_staging_dirs(backups_dir) - start_time_fmt = datetime.now().strftime(BACKUP_ENT_FMT) - cur_backup = fs.PseudoDirEntry(os.path.join(backups_dir, start_time_fmt)) - _lg.debug("Current backup dir: %s", cur_backup.path) + snapshot_name = _reserve_snapshot_name(backups_dir) + staging = fs.PseudoDirEntry( + os.path.join(backups_dir, f"{STAGING_PREFIX}{snapshot_name}") + ) + _lg.debug("Staging directory: %s", staging.path) latest_backup = _get_latest_backup(backups_dir) if latest_backup is None: - _lg.info("Creating empty directory for current backup: %s", - cur_backup.name) - os.mkdir(cur_backup.path) + _lg.info("Creating empty staging directory for backup: %s", + snapshot_name) + try: + os.mkdir(staging.path) + except OSError as err: + raise BackupFailedError( + "Failed to create staging directory %s: %s" + % (staging.path, err) + ) from err else: - _lg.info("Copying data from latest backup %s to current backup %s", - latest_backup.name, cur_backup.name) + _lg.info("Copying data from latest backup %s to staging for %s", + latest_backup.name, snapshot_name) hl_res = fs.hardlink_dir(src_dir=latest_backup.path, - dst_dir=cur_backup.path, + dst_dir=staging.path, use_external=external_hardlink) if not hl_res: _lg.error("Something went wrong during copying data from latest" - " backup, removing created %s", cur_backup.name) - shutil.rmtree(cur_backup.path, ignore_errors=True) - return + " backup, removing staging directory for %s", + snapshot_name) + shutil.rmtree(staging.path, ignore_errors=True) + raise BackupFailedError( + "Failed to copy data from latest backup %s" + % latest_backup.name + ) # remove backup markers from copied backup - for fname in os.listdir(cur_backup.path): + for fname in os.listdir(staging.path): if fname.startswith(BACKUP_MARKER): - os.remove(os.path.join(cur_backup.path, fname)) + os.remove(os.path.join(staging.path, fname)) # clean up delta dir from copied backup - shutil.rmtree(os.path.join(cur_backup.path, DELTA_DIR), + shutil.rmtree(os.path.join(staging.path, DELTA_DIR), ignore_errors=True) rsync_func = fs.rsync_ext if external_rsync else fs.rsync backup_changed = False - for src in sources: - src_abs = os.path.abspath(src) - src_name = os.path.basename(src_abs) - dst_abs = os.path.join(cur_backup.path, src_name) - _lg.info("Backing up directory %s to backup %s", - src_abs, cur_backup.name) - try: + try: + for src in sources: + src_abs = os.path.abspath(src) + src_name = os.path.basename(src_abs) + dst_abs = os.path.join(staging.path, src_name) + _lg.info("Backing up directory %s to staging for %s", + src_abs, snapshot_name) for entry_relpath, action, msg in rsync_func( src_abs, dst_abs, dry_run=dry_run ): + if action == fs.Actions.ERROR: + raise BackupFailedError( + "Failed to copy %s: %s" % (entry_relpath, msg) + ) # TODO maybe should be run if first backup too? if latest_backup is not None: process_backed_entry( - backup_dir=cur_backup.path, + backup_dir=staging.path, entry_relpath=os.path.join(src_name, entry_relpath), action=action, msg=msg, ) # raise flag if something was changed since last backup backup_changed = True - except fs.BackupCreationError as err: - _lg.error("Error during backup creation: %s", err) - _lg.error("Failed to create backup %s, removing", cur_backup.name) - shutil.rmtree(cur_backup.path, ignore_errors=True) + except fs.BackupCreationError as err: + _lg.error("Error during backup creation: %s", err) + _lg.error("Failed to create backup %s, removing staging directory", + snapshot_name) + shutil.rmtree(staging.path, ignore_errors=True) + raise BackupFailedError(str(err)) from err + except BackupFailedError: + _lg.error("Failed to create backup %s, removing staging directory", + snapshot_name) + shutil.rmtree(staging.path, ignore_errors=True) + raise # do not create backup on dry-run if dry_run: - _lg.info("Dry-run, removing created backup: %s", cur_backup.name) - shutil.rmtree(cur_backup.path, ignore_errors=True) + _lg.info("Dry-run, removing staging directory: %s", snapshot_name) + shutil.rmtree(staging.path, ignore_errors=True) + return None # do not create backup if no change from previous one - elif latest_backup is not None and not backup_changed: - _lg.info("Created backup %s is the same as previous one %s, removing", - cur_backup.name, latest_backup.name) - shutil.rmtree(cur_backup.path, ignore_errors=True) - else: - set_backup_marker(cur_backup) - _lg.info("Backup created: %s", cur_backup.name) + if latest_backup is not None and not backup_changed: + _lg.info("Staged backup is the same as previous one %s, removing", + latest_backup.name) + shutil.rmtree(staging.path, ignore_errors=True) + return None + + # write and fsync completion marker only after every source succeeded, + # then atomically rename the staging dir into its final snapshot name + marker_name = "%s_%s" % (BACKUP_MARKER, snapshot_name) + marker_fd = os.open(os.path.join(staging.path, marker_name), + os.O_CREAT | os.O_WRONLY, 0o644) + try: + os.fsync(marker_fd) + finally: + os.close(marker_fd) + _fsync_dir(staging.path) + + final_path = os.path.join(backups_dir, snapshot_name) + os.rename(staging.path, final_path) + _fsync_dir(backups_dir) + _lg.info("Backup created: %s", snapshot_name) + return fs.PseudoDirEntry(final_path) diff --git a/curateipsum/cli.py b/curateipsum/cli.py index d1090f2..fd4701a 100755 --- a/curateipsum/cli.py +++ b/curateipsum/cli.py @@ -96,23 +96,28 @@ def main(): if not backup.set_backups_lock(backups_dir_abs, args.force): return 1 - # TODO add cleaning up from non-finished backups - backup.cleanup_old_backups(backups_dir=backups_dir_abs, - dry_run=args.dry_run) - backup.initiate_backup( - sources=args.sources, - backups_dir=backups_dir_abs, - dry_run=args.dry_run, - external_rsync=args.external_rsync, - external_hardlink=args.external_hardlink, - ) - backup.release_backups_lock(backups_dir_abs) + exit_code = 0 + try: + backup.cleanup_old_backups(backups_dir=backups_dir_abs, + dry_run=args.dry_run) + backup.initiate_backup( + sources=args.sources, + backups_dir=backups_dir_abs, + dry_run=args.dry_run, + external_rsync=args.external_rsync, + external_hardlink=args.external_hardlink, + ) + except backup.BackupFailedError as err: + _lg.error("Backup failed: %s", err) + exit_code = 1 + finally: + backup.release_backups_lock(backups_dir_abs) end_time = time.time() spent_time = end_time - start_time _lg.info("Finished, time spent: %s", str(timedelta(seconds=spent_time))) - return 0 + return exit_code if __name__ == "__main__": diff --git a/tests/test_cli.py b/tests/test_cli.py index 0f48e45..db243fe 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -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.""" diff --git a/tests/test_integration.py b/tests/test_integration.py index 3231ffe..b7ee016 100644 --- a/tests/test_integration.py +++ b/tests/test_integration.py @@ -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