Handle special filesystem entries explicitly (#11)
* 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>
This commit is contained in:
5 files changed
+233
-15
No files matched your search
+32
-7
@@ -159,12 +159,14 @@ def _ensure_repo_id(backups_dir: str) -> str:
|
||||
|
||||
|
||||
def _write_manifest(marker_path: str, backups_dir: str, snapshot_name: str,
|
||||
checksums: Optional[Dict[str, str]] = None):
|
||||
checksums: Optional[Dict[str, str]] = None,
|
||||
excluded: Optional[Dict[str, str]] = None):
|
||||
"""Write and fsync the completion manifest for a finished snapshot."""
|
||||
manifest = json.dumps({
|
||||
"repo_id": _ensure_repo_id(backups_dir),
|
||||
"snapshot": snapshot_name,
|
||||
"checksums": checksums or {},
|
||||
"excluded": excluded or {},
|
||||
})
|
||||
_write_small_file_no_follow(marker_path, manifest)
|
||||
|
||||
@@ -719,7 +721,8 @@ def process_backed_entry(backup_dir: str,
|
||||
- if DirEntry was not deleted, hardlink it to DELTA_DIR.
|
||||
"""
|
||||
_lg.debug("%s %s %s", action, entry_relpath, msg)
|
||||
if action not in (fs.Actions.ERROR, fs.Actions.DELETE):
|
||||
if action not in (fs.Actions.ERROR, fs.Actions.DELETE,
|
||||
fs.Actions.EXCLUDE):
|
||||
fs.nest_hardlink(src_dir=backup_dir, src_relpath=entry_relpath,
|
||||
dst_dir=os.path.join(backup_dir, DELTA_DIR))
|
||||
|
||||
@@ -802,6 +805,11 @@ def initiate_backup(sources,
|
||||
# keyed by its path relative to the snapshot root. Recorded in the
|
||||
# manifest so a completed snapshot's integrity can be verified later.
|
||||
checksums: Dict[str, str] = {}
|
||||
# Entries this run deliberately never backed up (sockets, FIFOs,
|
||||
# devices, ...), keyed by path relative to the snapshot root, valued
|
||||
# with the excluded kind. Recorded in the manifest so an exclusion is
|
||||
# a visible, auditable result rather than a silent gap.
|
||||
excluded: Dict[str, str] = {}
|
||||
try:
|
||||
for src in sources:
|
||||
src_abs = os.path.abspath(src)
|
||||
@@ -817,6 +825,8 @@ def initiate_backup(sources,
|
||||
"Failed to copy %s: %s" % (entry_relpath, msg)
|
||||
)
|
||||
full_relpath = os.path.join(src_name, entry_relpath)
|
||||
if action == fs.Actions.EXCLUDE:
|
||||
excluded[full_relpath] = msg
|
||||
if action in (fs.Actions.CREATE, fs.Actions.REWRITE):
|
||||
if msg:
|
||||
# fs.rsync() already hashed the file while
|
||||
@@ -832,15 +842,18 @@ def initiate_backup(sources,
|
||||
and not os.path.islink(full_path)):
|
||||
checksums[full_relpath] = _sha256_file(full_path)
|
||||
# TODO maybe should be run if first backup too?
|
||||
if latest_backup is not None:
|
||||
if latest_backup is not None and action != fs.Actions.EXCLUDE:
|
||||
process_backed_entry(
|
||||
backup_dir=staging.path,
|
||||
entry_relpath=full_relpath,
|
||||
action=action,
|
||||
msg=msg,
|
||||
)
|
||||
# raise flag if something was changed since last backup
|
||||
backup_changed = True
|
||||
# raise flag if something was changed since last backup.
|
||||
# A recurring exclusion (e.g. a FIFO that's always there)
|
||||
# is not itself a change worth keeping a new snapshot for.
|
||||
if action != fs.Actions.EXCLUDE:
|
||||
backup_changed = True
|
||||
except fs.BackupCreationError as err:
|
||||
_lg.error("Error during backup creation: %s", err)
|
||||
_lg.error("Failed to create backup %s, removing staging directory",
|
||||
@@ -858,7 +871,18 @@ def initiate_backup(sources,
|
||||
_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
|
||||
# do not create backup if no change from previous one - but a change
|
||||
# in *which* entries are excluded is itself a change worth keeping,
|
||||
# or the new exclusions would never make it into a written manifest.
|
||||
if latest_backup is not None and not backup_changed:
|
||||
prev_manifest = _read_manifest(_get_backup_marker(latest_backup).path)
|
||||
prev_excluded = (
|
||||
prev_manifest.get("excluded", {})
|
||||
if isinstance(prev_manifest, dict) else {}
|
||||
)
|
||||
if excluded != prev_excluded:
|
||||
backup_changed = True
|
||||
|
||||
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)
|
||||
@@ -869,7 +893,8 @@ def initiate_backup(sources,
|
||||
# then atomically rename the staging dir into its final snapshot name
|
||||
marker_name = "%s_%s" % (BACKUP_MARKER, snapshot_name)
|
||||
_write_manifest(os.path.join(staging.path, marker_name),
|
||||
backups_dir, snapshot_name, checksums=checksums)
|
||||
backups_dir, snapshot_name, checksums=checksums,
|
||||
excluded=excluded)
|
||||
_fsync_dir(staging.path)
|
||||
|
||||
final_path = os.path.join(backups_dir, snapshot_name)
|
||||
|
||||
Reference in new issue
Block a user