* Add an explicit one-filesystem policy to backups
scantree() gains an opt-in one_filesystem flag: when a subdirectory's
device differs from the walk root's (a mount point), its contents are
excluded from the walk. The mount-point directory itself is still
yielded, so it appears as an empty directory in the backup rather than
vanishing silently - matching rsync's --one-file-system behavior.
Wired into rsync()'s two source-tree walks so backups never silently
pull in content from a bind mount or other filesystem mounted under
the source tree.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* Fix one-filesystem policy: symlinked root and external rsync parity
- scantree() computed the walk root's device with os.lstat(), but
scandir() follows the root path if it's a symlink. A symlinked root
would compute the symlink's own device instead of the target
directory's, making every real subdirectory look cross-device and
get its contents wrongly excluded. Use os.stat() for the root only.
- rsync_ext() (the external-rsync backend) didn't pass
--one-file-system, so backups using external_rsync=True still
crossed mount points despite the pure-Python path now refusing to.
Addresses GitHub Copilot review feedback on PR #18.
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>
* 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>
* Preserve symlinks and old-snapshot immutability
Two real bugs found while implementing this item, both in fs.py:
- copy_direntry() checked entry.is_dir() (default follow_symlinks=True)
before entry.is_symlink(). A symlink pointing at a directory was
misclassified as a directory and an empty real directory was created
in its place, silently destroying the symlink and losing its target
entirely. Fixed by checking is_symlink() first, and is_dir() with
follow_symlinks=False.
- rsync()'s permission/ownership-sync branch called os.chmod/os.chown
on dst_entry.path unconditionally. Two problems: (1) for a symlink
whose target already matched, this had no follow_symlinks=False
guard and could rewrite an external symlink target's permissions;
(2) for a regular file, that path may still be hardlinked to the
same file in the previous, completed snapshot (hardlink_dir() links
unchanged files in at the start of every backup) - mutating it in
place silently corrupted every older snapshot sharing the inode.
Fixed by skipping symlinks entirely (nothing to sync once their
target matches) and adding _break_shared_hardlink(), a copy-on-write
step that gives a file its own inode before any in-place metadata
change.
Also fixed the dir-mtime-restore loop's is_dir() call (same
default-follow_symlinks issue, lower severity since its own operations
were already lstat-based/no-follow).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* Address Copilot review: preserve ownership/timestamps when breaking hardlinks, fix test fd leak
- _break_shared_hardlink() now explicitly restores the original file's
uid/gid and atime/mtime on the new private copy before it replaces
the shared one. copy_file() only carries over mode at create time, so
without this a caller that only meant to update permissions would
silently also reset ownership/mtime as a side effect of the
copy-on-write break itself. Also cleans up the temp file on failure
and uses a less collision-prone temp name.
- Fixed a file descriptor leak in the new hardlink-immutability test
(bare open(...).read() without a context manager)
- Strengthened the same test to assert mtime survives the break
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>
- Fix follow_symlinks parameter being ignored in is_dir(), is_file()
- Change from realpath() to abspath() to preserve symlinks
- Add separate caches for stat() and lstat() results
- Remove incorrect follow_symlinks param from is_symlink()
- Add comprehensive docstring explaining purpose and design
When follow_symlinks=False, methods now correctly return False for
symlinks instead of following them. Previously all symlinks were
resolved, breaking symlink-aware backup operations.
Fixes#8
Initialize file descriptors to None before opening to prevent
UnboundLocalError in the finally block if file opening fails.
Changes:
- Initialize fin = None and fout = None at function start
- Check if descriptors are not None before closing
- Remove UnboundLocalError from exception handling (no longer needed)
This improves code quality by eliminating the need to catch an error
that indicates a logic bug. The existing tests verify correct behavior.
Fixes#6
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.
