* 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>
* Handle special filesystem entries explicitly
- Added fs.entry_kind(): classifies via lstat() mode bits alone
("file", "dir", "symlink", "fifo", "socket", "block_device",
"char_device", "unknown") - never opens the entry, so classifying a
FIFO can't block even without a writer.
- rsync() now classifies every source entry before touching it and
yields a new Actions.EXCLUDE for anything outside file/dir/symlink,
instead of falling through to copy_direntry()'s file branch, which
would either block forever opening a writerless FIFO for read, or
raise an OSError that initiate_backup() treats as fatal (aborting the
whole backup for one socket).
- initiate_backup() records exclusions in the completion manifest
(keyed by snapshot-relative path, valued with the excluded kind), and
no longer nest_hardlinks or counts a recurring exclusion as a change
worth keeping a new snapshot for.
- Chose the "recorded exclusion" policy the plan allows, rather than
fail-fast: a FIFO or socket living in a backed-up tree is not itself
a backup failure, and previously turned into one.
Note: rsync_ext() (external rsync) is unaffected by this change and
keeps whatever its own --archive flags do with special files; the P1
item on Python/external implementation parity covers reconciling that.
The pre-existing test_src_is_socket failure (AF_UNIX path length on
macOS tmp_path) is untouched here - it's separately tracked under P1
test fixes - but this PR adds working socket/FIFO/char-device coverage
via a short-path tempdir.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* Address Copilot review: fix stale exclude, exclusion-only diffs, test portability
- fs.rsync(): remove the existing dst_entry before yielding EXCLUDE for
a src entry with an unsupported kind, so an entry inherited via
hardlink from a prior snapshot doesn't linger in the new one.
- backup.initiate_backup(): compare this run's excluded set against
the previous snapshot's manifest; a change in exclusions alone now
counts as backup_changed, so the updated manifest gets written.
- tests: skip mkfifo/AF_UNIX-dependent tests on platforms without
support, and use TemporaryDirectory/context managers instead of
manual mkdtemp/unlink for more robust cleanup.
* Remove unused shutil import from test_rsync.py
Left over after switching the socket/fifo tests to
tempfile.TemporaryDirectory(), per Copilot's suppressed comment.
* Fix rm_direntry() no-op on special dst entries
rm_direntry() only handled file/symlink/dir - a FIFO, socket, or
device node at dst (e.g. inherited from an external-rsync snapshot)
silently survived removal, since is_file/is_dir/is_symlink all
return False for it. Any non-directory entry is unlinkable the same
way, so invert the branching: recurse for directories, unlink
everything else.
---------
Co-authored-by: Maxim Shulga <maxim.shulga@alludo.com>
Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
* Prevent stale, truncated, or changing-file copies
- copy_file() now loops until every byte is actually written (os.write
may write fewer bytes than given) and compares fstat() on the
source's own open descriptor before/after the read loop - this
reflects the underlying inode directly, catching a source file
truncated, grown, or overwritten in place while being read, and isn't
fooled by the source path being renamed away mid-copy. Raises
BackupCreationError (already a fatal, staging-discarding condition in
initiate_backup) on a mismatch.
- copy_file() returns a sha256 hex digest of the copied content;
copy_direntry()/update_direntry() propagate it; rsync() carries it
through the previously-unused msg field for CREATE/REWRITE actions.
- initiate_backup() collects those digests into a checksums dict keyed
by path relative to the snapshot root, recorded in the completion
manifest (already JSON since the topology-validation work).
- Added verify_snapshot(): re-hashes every file with a recorded
checksum and reports any mismatch, satisfying the release gate's
"completed snapshot passes manifest verification" requirement.
- Switched every mtime comparison and os.utime() restore (rsync's
file/directory sync, hardlink_dir's directory metadata, the
hardlink-breaking copy-on-write step) from float seconds to
nanoseconds, so a same-second modification can't be missed.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* Address Copilot review: hash checksums for external rsync too, fix log/error text
- initiate_backup() now hashes CREATE/REWRITE files itself when msg is
empty (fs.rsync_ext(), the external-rsync path, never yields a
digest). Without this, verify_snapshot() silently became a no-op for
every snapshot created with external_rsync=True.
- Fixed a pre-existing log message that said "different inodes" for
the branch whose actual condition is src/dst sharing an inode.
- verify_snapshot()'s error message now also mentions a corrupt
manifest, not just missing/pre-migration, matching what
_read_manifest() actually distinguishes.
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>
* 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>
This commit addresses 8 high-priority issues identified in code analysis.
Fixes#3Fixes#4Fixes#5Fixes#7Fixes#10Fixes#19Fixes#20Fixes#21
## Critical Bug Fixes
1. **Race condition in lock file creation (#3)**
- Changed to atomic file creation using os.O_CREAT | os.O_EXCL
- Prevents two processes from both acquiring the lock
- Location: curateipsum/backup.py:110-115
2. **Invalid lock file error handling (#4)**
- Added try/except for corrupted/empty lock files
- Gracefully removes corrupted locks and retries
- Location: curateipsum/backup.py:121-133
3. **SIGKILL vs SIGTERM issue (#5)**
- Now sends SIGTERM first for graceful shutdown
- Waits 5 seconds before escalating to SIGKILL
- Allows previous process to clean up resources
- Location: curateipsum/backup.py:146-156
4. **Wrong stat object for permissions (#7)**
- Fixed bug where dst_stat was used instead of src_stat
- Permissions are now correctly updated during rsync
- Location: curateipsum/fs.py:371
5. **os.chown() fails for non-root users (#10)**
- Wrapped all os.chown() calls in try/except blocks
- Logs debug message instead of crashing
- Allows backups to succeed for non-root users
- Locations: curateipsum/fs.py:217-221, 228-231, 383-387, 469-472
## Comprehensive Test Coverage
6. **Lock file tests (#19)**
- Added TestBackupLock class with 7 test cases
- Tests: creation, concurrent prevention, stale locks, corruption
- Location: tests/test_backups.py:228-330
7. **Filesystem operation tests (#20)**
- Added tests/test_fs_extended.py with 6 test classes
- Tests: copy_file, copy_direntry, rsync, hardlink_dir, scantree, rm_direntry
- 20+ test cases covering normal and edge cases
- Location: tests/test_fs_extended.py
8. **Integration tests (#21)**
- Added tests/test_integration.py with 2 test classes
- Tests full backup workflow end-to-end
- Tests: incremental backups, hardlinks, delta dirs, cleanup, recovery
- 14 test cases covering complete backup lifecycle
- Location: tests/test_integration.py
## Test Results
All 68 tests pass successfully:
- 11 original backup cleanup tests
- 7 new lock file tests
- 16 original fs tests
- 20 new fs extended tests
- 14 new integration tests
## Impact
These fixes address critical bugs that could cause:
- Data corruption from concurrent backups
- Incomplete cleanup from forced process termination
- Permission sync failures
- Tool unusability for non-root users
The comprehensive test coverage ensures these bugs are caught early
and provides confidence for future refactoring.
