* Make dry-run genuinely non-mutating
fs.rsync() now takes a real dry_run code path: every mutating call
(copy, delete, chmod, chown, utime) is skipped, and a missing dst_dir
is never created - the same comparison logic still runs so the yielded
actions match what a real run would do.
backup.initiate_backup() diffs each source directly against the latest
completed snapshot on dry-run, instead of building and discarding a
throwaway staging directory with real hardlinks. cleanup_old_backups()
no longer creates the repo-id file as a side effect of a dry-run.
cli.py skips lock acquisition entirely on dry-run (set_backups_lock()
writes PID/timestamp metadata, itself a mutation) and exposes the
retention policy via --keep-all/--keep-daily/--keep-weekly/
--keep-monthly/--keep-yearly.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* Address Copilot review: guard restore lock release, reject negative retention values
- Guard restore --dry-run's finally block the same way backup's is
guarded, for symmetry (release_backups_lock() was already a safe
no-op when unheld, but this makes the dry-run contract explicit
rather than relying on that implementation detail).
- Reject negative --keep-* values via a dedicated argparse type: a
negative threshold shifts retention comparisons into the future and
can make cleanup treat every existing backup as deletable.
- Add a restore --dry-run lock-skip regression test, mirroring the
existing backup one.
- Fix _snapshot_all_stats() in the new dry-run non-mutation test to
also record the backups_dir root's own stat, not just its children.
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>
* Implement restore command
Add a restore subcommand alongside the existing backup command:
- curateipsum/restore.py: restore_snapshot() and verify_restored(),
reusing fs.scantree/copy_direntry/rm_direntry so metadata handling
(mode, ownership, timestamps, symlinks) matches backup exactly.
- backup.find_backup(): resolve a snapshot by name or latest.
- cli.py: argparse subcommands (backup/restore), with backup implied
when no subcommand is given so existing invocations keep working.
Restore supports dry-run (creates nothing), partial restore by
snapshot-relative path, an overwrite policy (never/always), a
destination-inside-snapshot boundary check, and manifest-checksum
verification of restored content. It takes the same backups_lock a
backup does, since retention could otherwise delete the snapshot
being restored out from under it.
Closes P1 'Provide a safe restoration workflow' in IMPROVEMENTS.md.
* Address Copilot review: symlink-escape boundary check, docstring fix
- restore_snapshot() and verify_restored() now canonicalize each
destination path and refuse to operate on it if it resolves outside
dest_dir - a symlink planted under dest_dir (by another process, or
left over from a prior run) could otherwise redirect a write, delete,
or read outside the intended destination.
- Fixed restore_snapshot()'s docstring: the skipped-paths list reports
what would be skipped on dry-run too, it isn't always empty.
* Address Copilot suppressed findings: stream plan_restore, restore-specific lock message
- plan_restore() is now a generator instead of building a full list in
memory, matching fs.scantree()'s streaming style everywhere else in
the codebase.
- restore CLI logs a restore-specific message on lock-acquire failure,
since set_backups_lock()'s own log line talks about a 'previous
backup', which read wrong for a restore.
* Fix PATH docs: snapshot-relative means source-basename-prefixed
README and CLI help examples for restore's PATH argument omitted the
source directory's basename, which backup.initiate_backup() always
prefixes onto snapshot-relative paths - the given examples would have
matched nothing in an actual snapshot.
---------
Co-authored-by: Maxim Shulga <maxim.shulga@alludo.com>
* 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>
* Validate topology and destructive path boundaries
- Canonicalize sources/backups_dir and reject overlap in either direction
- Reject duplicate sources and destination basename collisions
- Reject non-normalized relative paths in nest_hardlink
- Give each backup repository a persistent identity, recorded in every
snapshot's completion manifest
- Verify repo identity, directory type (no symlinks), containment, and
device before any recursive snapshot/staging-dir deletion, refusing to
cross mount points
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* Address Copilot review: root-path check, symlink/TOCTOU-safe writes, legacy marker adoption
- _is_same_or_within: drop the redundant, root-mishandling startswith
check; commonpath() alone is already component-aware
- _ensure_repo_id/_write_manifest: write via temp-file + atomic rename
(O_NOFOLLOW, O_EXCL on the temp file) instead of open+write in place,
so writes can never go through a pre-existing symlink and are never
left half-written
- _verify_safe_to_delete: distinguish a legacy empty marker (pre-dates
the manifest format, can only be a pre-migration artifact) from a
non-empty-but-invalid one (corrupt or forged); adopt the former into
the repository instead of refusing to delete it forever
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>
* Replace PID-file process killing with a real fcntl.flock() lock
set_backups_lock()/release_backups_lock() now use an exclusive
fcntl.flock() on the lock file's inode instead of trusting the PID
written inside it. The kernel releases the lock automatically on
normal exit, on any signal, and on a crash - no PID liveness check or
staleness heuristic is needed, and there is nothing left in this code
path that sends a signal to another process.
- The open file descriptor is held for the full operation in a
module-level _held_locks map keyed by the backups directory, and
release_backups_lock() only ever releases a descriptor this process
itself acquired, never one it infers from the lock file's content.
- PID and start time are still written into the lock file, but purely
as diagnostics for whoever reads it (e.g. in a warning log line);
they play no role in acquisition or release decisions anymore.
- --force no longer sends SIGTERM/SIGKILL to the previous holder. It
now blocks on the same flock() until that process releases the lock
on its own (exit, signal, or crash all count).
- release_backups_lock() intentionally stops unlinking the lock file:
since flock() locks an inode rather than a path, unlinking while
another process is blocked opening the same path would let that
process (or a third one) acquire an independent lock on a fresh
inode at the same path, defeating mutual exclusion.
Rewrites TestBackupLock to match: a real second file descriptor with
its own flock() stands in for "another process" in the fast unit
tests, and a new subprocess-based test proves end-to-end that only one
process can hold the lock and that killing the holder with SIGKILL
frees it for the next acquirer without our code ever calling os.kill.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* Address Copilot review feedback on flock-based lock
- Make the fcntl import optional (try/except ImportError -> None), so
importing curateipsum.backup no longer crashes on Windows before
cli.py's own "Not supported platform" check gets a chance to run.
- Guard set_backups_lock() against being called twice for a directory
this process already holds: it now fails fast instead of silently
overwriting the tracked fd in _held_locks, which would leak the
original descriptor and leave the lock held even after
release_backups_lock() closes only the newest one.
- Wrap the post-acquire metadata write (ftruncate/write/fsync) in a
try/except: a failure there now releases the flock and closes the fd
before returning False, instead of leaving the lock held with
no caller aware of it.
- Cap _read_lock_metadata()'s read and strip newlines before logging,
so a stale or forged lock file can't inject bogus log lines or force
an unbounded read.
Adds a regression test for the double-acquire guard.
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>
* 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>
