* 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>
* 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>
Adds 7 new tests in TestErrorRecovery class covering:
- Hardlink failure cleanup (incomplete backup removal)
- Rsync failure cleanup
- Detection of incomplete backups without marker files
- Lock handling after backup failures
- Permission error handling during cleanup
Tests verify that failed backups are properly cleaned up and don't interfere
with subsequent backup operations or lock management.
Tests --external-rsync and --external-hardlink flags with conditional
execution based on tool availability. Verifies external tools correctly
sync files and preserve hardlinks for unchanged files.
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.
