Preserve symlinks and old-snapshot immutability (#9)

* 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>

IMPROVEMENTS.md

@@ -43,10 +43,10 @@ Acceptance criteria:
 
 ### Preserve symlinks and old-snapshot immutability
 
-- [ ] Classify entries with `lstat()` and process symlinks before directories.
-- [ ] Never follow symlinks while applying symlink metadata.
-- [ ] Break shared hardlinks before changing mode, ownership, flags, or times.
-- [ ] Define copy-on-write invariants for every mutation.
+- [x] Classify entries with `lstat()` and process symlinks before directories.
+- [x] Never follow symlinks while applying symlink metadata.
+- [x] Break shared hardlinks before changing mode, ownership, flags, or times.
+- [x] Define copy-on-write invariants for every mutation.
 
 Acceptance criteria:
 

curateipsum/fs.py

@@ -8,6 +8,7 @@ import logging
 import os
 import subprocess
 import sys
+import uuid
 from typing import Iterable, Tuple, Union
 
 _lg = logging.getLogger(__name__)
@@ -243,16 +244,59 @@ def copy_file(src, dst):
                 pass
 
 
+def _break_shared_hardlink(path: str):
+    """
+    Give `path` a private inode if it currently shares one with another
+    file (nlink > 1), so a subsequent in-place metadata change (chmod,
+    chown, utime, flags) can never mutate that other file. Files copied
+    into a staging directory via hardlink_dir() start out hardlinked to
+    the previous snapshot; this is the copy-on-write step that keeps
+    that previous, completed snapshot immutable.
+
+    The break itself must be metadata-neutral: copy_file() only carries
+    over the mode at file-creation time, not ownership or timestamps, so
+    the new private copy is explicitly restored to the original's full
+    stat before it replaces `path` - otherwise a caller that only meant
+    to update permissions would silently also reset ownership/mtime as a
+    side effect of this implementation detail.
+    """
+    st = os.lstat(path)
+    if st.st_nlink <= 1:
+        return
+    tmp_path = "%s.cow-%d-%s" % (path, os.getpid(), uuid.uuid4().hex[:8])
+    try:
+        copy_file(path, tmp_path)
+        try:
+            os.chown(tmp_path, st.st_uid, st.st_gid)
+        except PermissionError:
+            _lg.debug("Cannot preserve ownership while breaking hardlink "
+                      "(not root): %s", path)
+        os.chmod(tmp_path, st.st_mode)
+        os.utime(tmp_path, (st.st_atime, st.st_mtime))
+        os.replace(tmp_path, path)
+    except OSError:
+        try:
+            os.unlink(tmp_path)
+        except OSError:
+            pass
+        raise
+
+
 def copy_direntry(entry: Union[os.DirEntry, PseudoDirEntry], dst_path):
     """ Non-recursive DirEntry (file/dir/symlink) copy. """
     src_stat = entry.stat(follow_symlinks=False)
-    if entry.is_dir():
-        os.mkdir(dst_path)
-
-    elif entry.is_symlink():
+    # Classify with lstat semantics and check symlink-ness first: is_dir()
+    # (and is_file()) default to follow_symlinks=True, so a symlink
+    # pointing at a directory would otherwise be misclassified as a
+    # directory here and get an empty directory created in its place,
+    # silently losing the symlink and its target.
+    if entry.is_symlink():
         link_target = os.readlink(entry.path)
         os.symlink(link_target, dst_path)
 
+    elif entry.is_dir(follow_symlinks=False):
+        os.mkdir(dst_path)
+
     else:
         copy_file(entry.path, dst_path)
 
@@ -416,6 +460,26 @@ def rsync(src_dir,
                     yield rel_path, Actions.ERROR, str(exc)
                 continue
 
+        # Symlinks reaching here already have a matching target (handled
+        # above), and applying chmod/chown to a symlink path without
+        # follow_symlinks=False support would silently modify whatever
+        # the symlink points to - including a target outside the backup
+        # entirely. There's nothing meaningful left to sync for them.
+        if src_entry.is_symlink():
+            continue
+
+        # A regular file reaching here is unchanged in content, but may
+        # still be hardlinked to the same file in the previous, supposedly
+        # immutable snapshot (hardlink_dir() links unchanged files in at
+        # the start of every backup to save space). Break that link
+        # before mutating permissions/ownership in place, or the change
+        # would silently apply to every older snapshot sharing the inode.
+        if src_entry.is_file(follow_symlinks=False):
+            if src_stat.st_mode != dst_stat.st_mode or (
+                    src_stat.st_uid != dst_stat.st_uid
+                    or src_stat.st_gid != dst_stat.st_gid):
+                _break_shared_hardlink(dst_entry.path)
+
         # update permissions and ownership
         if src_stat.st_mode != dst_stat.st_mode:
             _lg.debug("Rsync, updating permissions: %s", rel_path)
@@ -443,7 +507,7 @@ def rsync(src_dir,
 
     # restore dir mtimes in dst, updated by updating files
     for src_entry in scantree(src_root_abs, dir_first=True):
-        if not src_entry.is_dir():
+        if not src_entry.is_dir(follow_symlinks=False):
             continue
         rel_path = src_entry.path[len(src_root_abs) + 1:]
         dst_path = os.path.join(dst_root_abs, rel_path)

tests/test_fs.py

@@ -285,6 +285,32 @@ class TestCopyDirEntry:
         dst_stat = os.lstat(dst_path)
         assert dst_stat.st_mode == src_stat.st_mode
 
+    def test_copy_symlink_to_directory_stays_a_symlink(self, tmp_path):
+        """
+        Regression: is_dir()/is_file() default to follow_symlinks=True,
+        so a symlink pointing at a directory must be classified via
+        is_symlink() first, or it gets misclassified as a directory and
+        an empty real directory is created in its place - silently
+        destroying the symlink and losing its target entirely.
+        """
+        target_dir = os.path.join(str(tmp_path), "target_dir")
+        src_link = os.path.join(str(tmp_path), "source_link")
+        dst_link = os.path.join(str(tmp_path), "dest_link")
+
+        os.mkdir(target_dir)
+        with open(os.path.join(target_dir, "canary"), "w") as f:
+            f.write("content")
+        os.symlink(target_dir, src_link)
+
+        with os.scandir(str(tmp_path)) as it:
+            for entry in it:
+                if entry.name == "source_link":
+                    fs.copy_direntry(entry, dst_link)
+                    break
+
+        assert os.path.islink(dst_link)
+        assert os.readlink(dst_link) == target_dir
+
     def test_copy_symlink_preserves_times_if_supported(self, tmp_path):
         """Test that symlink timestamps are preserved (OS-dependent)"""
         target_path = os.path.join(str(tmp_path), "target.txt")

tests/test_rsync.py

@@ -635,3 +635,77 @@ class TestRsyncExt:
             assert results[2][0] == "config.json"
             assert results[3][0] == "existing.txt"
             assert results[4][0] == "old/"
+
+
+class TestRsyncHardlinkImmutability:
+    """
+    Regression tests: rsync() is run against a staging directory that
+    was populated by hardlinking in the previous, completed snapshot
+    (curateipsum.backup.hardlink_dir). Any in-place metadata change made
+    while updating staging must never mutate that previous snapshot's
+    files via the shared inode.
+    """
+
+    def test_permission_change_does_not_mutate_shared_hardlink(
+            self, common_fs_dirs, tmp_path
+    ):
+        src_dir, dst_dir = common_fs_dirs
+        # Independent source file with new content/mtime but the SAME
+        # eventual permissions the old snapshot file already has - only
+        # the fallback "update permissions and ownership" branch should
+        # fire, not a full rewrite (which would already be safe via
+        # rm+recreate).
+        src_fpath = create_file(str(src_dir))
+        os.chmod(src_fpath, 0o600)
+
+        # Simulate staging (dst_dir) having been hardlinked in from the
+        # previous, completed snapshot: a separate directory tree outside
+        # dst_dir, sharing an inode with the staging file, with content
+        # and mtime matching src (so only permissions differ) but the old
+        # mode.
+        dst_fpath = os.path.join(str(dst_dir),
+                                 relpath(src_fpath, str(src_dir), str(dst_dir)))
+        prior_snapshot_fpath = str(tmp_path / "prior_snapshot")
+        with open(src_fpath) as src_f:
+            src_content = src_f.read()
+        with open(prior_snapshot_fpath, "w") as f:
+            f.write(src_content)
+        os.chmod(prior_snapshot_fpath, 0o644)
+        src_stat = os.lstat(src_fpath)
+        os.utime(prior_snapshot_fpath, (src_stat.st_atime, src_stat.st_mtime))
+        os.link(prior_snapshot_fpath, dst_fpath)
+        prior_mode_before = os.lstat(prior_snapshot_fpath).st_mode
+        dst_mtime_before = os.lstat(dst_fpath).st_mtime
+        assert os.lstat(dst_fpath).st_ino == os.lstat(
+            prior_snapshot_fpath).st_ino  # sanity: genuinely shared
+
+        all(fs.rsync(str(src_dir), str(dst_dir)))
+
+        assert os.lstat(dst_fpath).st_mode == os.lstat(src_fpath).st_mode
+        assert os.lstat(prior_snapshot_fpath).st_mode == prior_mode_before
+        # dst must now have its own inode, no longer shared
+        assert os.lstat(dst_fpath).st_ino != os.lstat(
+            prior_snapshot_fpath).st_ino
+        # breaking the hardlink must not, as a side effect, reset mtime:
+        # rsync only intended to change permissions here (content/mtime
+        # already matched), and uid/gid are identical for this test user
+        # so the chown branch never fires to "accidentally" fix ownership
+        assert os.lstat(dst_fpath).st_mtime == dst_mtime_before
+
+    def test_symlink_metadata_never_touches_external_target(
+            self, common_fs_dirs, tmp_path
+    ):
+        src_dir, dst_dir = common_fs_dirs
+        external_target = tmp_path / "external_target"
+        external_target.mkdir()
+        (external_target / "canary").write_text("do-not-touch")
+        external_mode_before = os.lstat(str(external_target)).st_mode
+
+        link_name = "a_symlink"
+        os.symlink(str(external_target), os.path.join(str(src_dir), link_name))
+        os.symlink(str(external_target), os.path.join(str(dst_dir), link_name))
+
+        all(fs.rsync(str(src_dir), str(dst_dir)))
+
+        assert os.lstat(str(external_target)).st_mode == external_mode_before
+        assert (external_target / "canary").read_text() == "do-not-touch"