fix(maintenance): recheck the kept copy right before each unlink

Re-review 2 of #313:

- n1: the cross-mount guard also matches when one path within its source
  ends with the other on a whole folder name (sources listed at different
  depths, e.g. 2024/x.png and x.png on a FUSE or noserverino mount).
- n2: after a file's records move and right before its unlink, the held
  kept copy must still be the file at its path (fstat of the hold against
  os.stat) and the cross-mount guard must still hold; otherwise the file
  is kept, its records move back to it, and it counts as not removed.

TESTING.md

@@ -2612,12 +2612,18 @@ copy of themselves (`test_a_folder_listed_twice_never_loses_its_only_copy`,
 `test_a_hardlink_is_never_a_copy_to_remove`). One file reached through two mounts is
 protected, never guessed at: a removal on another `st_dev` than its kept copy, with the
 same size and mtimes within 2 s, is kept as "may be the same file reached through another
-mount or share" when its `st_ino` is equal or its path within its source is the same
+mount or share" when its `st_ino` is equal or its path within its source is the same, or
+one ends with the other on a whole folder name (sources listed at different depths:
+`test_a_deeper_source_on_another_mount_is_protected`)
 (NFS and SMB, a loopback mount, a FUSE pool: `TestAnotherMount`). Some true copies on
 another disk are therefore never offered. A volume that reports no file id protects
 every pair ("Can't confirm this is a separate file on this drive"). The job checks both
-again right before each unlink and holds the kept copy open meanwhile, so on Windows
-deleting the kept copy itself through another path fails instead. The server holds the groups (content
+before each file's records move, holds the kept copy open, and right before the unlink
+checks again that the held kept copy is still the file at its path (`fstat` of the held
+handle against `os.stat` of the path) and that the guard still holds; otherwise the file
+is kept, its records move back to it, and it counts as not removed
+(`test_a_kept_copy_deleted_during_the_move_keeps_the_file`). On Windows the hold also
+makes deleting the kept copy itself through another path fail. The server holds the groups (content
 hash, root-relative paths, source index, size, mtime and inode) for the last three
 finds; `GET /prompt_manager/maintenance/duplicates/{job_id}?offset&limit` pages
 them ten at a time (`not-found` 404, `job-running` or `no-groups` 409). The preview

py/api/duplicate_files.py

@@ -56,6 +56,8 @@ from .duplicate_review import (  # noqa: F401 (CHANGED, OUTSIDE_SOURCE: public)
     GONE,
     KEEPER,
     KEEPER_CHANGED,
+    KEEPER_GONE,
+    KEEPER_WENT,
     KEPT_COPY,
     LOCKED_FILE,
     MAYBE_SAME_FILE,
@@ -78,9 +80,10 @@ from .duplicate_review import (  # noqa: F401 (CHANGED, OUTSIDE_SOURCE: public)
     _select,
     file_problem,
     human_bytes,
+    still_kept,
 )
-from .duplicate_spellings import Spellings
-from .images import _favorite_relative_key, _favorite_root_key, _output_image_entry
+from .duplicate_spellings import Spellings, copy_move
+from .images import _favorite_root_key, _output_image_entry
 from .job_routes import busy_response
 from .jobs import (
     DUPLICATE_CLEANUP_GROUP_NAMES,
@@ -684,14 +687,20 @@ class DuplicateFilesMixin:
         except OSError:
             return "kept", KEEPER, None
         with hold:
-            return self._move_then_unlink(db, target, keepers[group_id], spellings)
+            return self._move_then_unlink(
+                db, target, keepers[group_id], spellings, hold
+            )
 
-    def _move_then_unlink(self, db, target, kept_stat, spellings):
+    def _move_then_unlink(self, db, target, kept_stat, spellings, hold):
         try:
-            counts = db.move_copy_records(self._copy_move(target, kept_stat, spellings))
+            counts = db.move_copy_records(copy_move(target, kept_stat, spellings))
         except Exception:
             self.logger.exception(f"Could not move the records of {target['rel']}")
             return "error", RECORDS_FAILED, None
+        if not still_kept(target, hold):
+            # The kept copy went away or changed while the records moved:
+            # keep this file and give its records back to it.
+            return "kept", KEEPER_GONE, self._move_back(db, target, spellings)
         try:
             _unlink(target["abs"])
         except FileNotFoundError:
@@ -713,48 +722,26 @@ class DuplicateFilesMixin:
         _remove_thumbnail(Path(target["root"]), Path(target["rel"]), self.logger)
         return "removed", None, counts
 
-    @staticmethod
-    def _copy_move(target, kept_stat, spellings):
-        """What ``move_copy_records`` moves for one removal.
-
-        A record found under a folder spelling moves to the kept copy under
-        the same spelling when that folder holds it too, else to its real
-        path. Its favorite moves under every listed root holding the file.
-        """
-        removed = spellings.of(target["abs"])
-        kept = spellings.of(target["keeper_abs"])
-        targets = {
-            spelled: kept.get(folder, target["keeper_abs"])
-            for folder, spelled in removed.items()
+    def _move_back(self, db, target, spellings):
+        """Give the kept copy's records to the file that stays (blocking)."""
+        swapped = {
+            **target,
+            "abs": target["keeper_abs"],
+            "root": target["keeper_root"],
+            "rel": target["keeper_rel"],
+            "locator": target["keeper_locator"],
+            "keeper_abs": target["abs"],
+            "keeper_root": target["root"],
+            "keeper_rel": target["rel"],
+            "keeper_locator": target["locator"],
         }
-        kept_favorites = spellings.favorites(target["keeper_abs"])
-        favorites = []
-        for root, relative in spellings.favorites(target["abs"]).items():
-            kept_root, kept_rel = (
-                (root, kept_favorites[root])
-                if root in kept_favorites
-                else (target["keeper_root"], target["keeper_rel"])
+        try:
+            return db.move_copy_records(
+                copy_move(swapped, os.stat(target["abs"]), spellings)
             )
-            favorites.append(
-                (
-                    _favorite_root_key(root),
-                    _favorite_relative_key(relative),
-                    _favorite_root_key(kept_root),
-                    _favorite_relative_key(kept_rel),
-                )
-            )
-        return {
-            "removed": list(removed.values()),
-            "kept": list(kept.values()),
-            "targets": targets,
-            "kept_path": target["keeper_abs"],
-            "favorites": favorites,
-            "removed_locator": target["locator"],
-            "kept_locator": target["keeper_locator"],
-            "sha256": target["hash"],
-            "kept_size": kept_stat.st_size,
-            "kept_mtime_ns": kept_stat.st_mtime_ns,
-        }
+        except Exception:
+            self.logger.exception(f"Could not give the records back to {target['rel']}")
+            return None
 
 
 def _bound_problem(record, group, keeper=None):

py/api/duplicate_review.py

@@ -73,7 +73,13 @@ MAYBE_SAME_FILE = (
     "May be the same file reached through another mount or share; it was kept."
 )
 NO_FILE_ID_ROW = "Can't confirm this is a separate file on this drive; it was kept."
+KEEPER_GONE = "keeper-gone"
+KEEPER_WENT = (
+    "The kept copy changed or went away during the removal; this file was kept "
+    "and its links stay with it."
+)
 ROW_DETAILS = {
+    KEEPER_GONE: None,  # filled below
     MAYBE_SAME: MAYBE_SAME_FILE,
     NO_FILE_ID: NO_FILE_ID_ROW,
     SAME: SAME_FILE,
@@ -108,6 +114,29 @@ class SourcesChanged(Exception):
     """A source a review or removal names is no longer listed at its index."""
 
 
+ROW_DETAILS[KEEPER_GONE] = KEEPER_WENT
+
+
+def still_kept(target, hold):
+    """Right before an unlink: the held kept copy is still the file at its
+    path, and the file is still not the kept copy itself (``keeper_problem``).
+    """
+    try:
+        if not os.path.samestat(os.fstat(hold.fileno()), os.stat(target["keeper_abs"])):
+            return False
+        info = os.lstat(target["abs"])
+        problem = keeper_problem(
+            target["abs"],
+            target["root"],
+            info,
+            target["keeper_abs"],
+            target["keeper_root"],
+        )
+        return problem is None
+    except OSError:
+        return False
+
+
 def _plural(count, one, many=None):
     return f"{count:,} {one if count == 1 else (many or one + 's')}"
 
@@ -211,9 +240,11 @@ def keeper_problem(path, root, info, keeper, keeper_root=None):
       two spellings of one mount);
     - ``MAYBE_SAME``: another device, the same size, mtimes within
       ``MOUNT_MTIME_SLACK_NS``, and the same ``st_ino`` (NFS and SMB report
-      the server's file id) or the same path within their sources (one share
-      mounted twice, a FUSE pool over a listed disk). Protect, never guess:
-      a true copy on another disk at the same relative path is kept too.
+      the server's file id) or the same path within their sources, or one
+      ending with the other on a folder name (one share mounted twice, a
+      FUSE pool over a listed disk, sources listed at different depths).
+      Protect, never guess: a true copy on another disk at the same relative
+      path is kept too.
 
     Raises OSError when the kept copy cannot be read (the caller's GONE or
     UNREADABLE).
@@ -229,13 +260,24 @@ def keeper_problem(path, root, info, keeper, keeper_root=None):
         return None
     if info.st_ino == kept.st_ino:
         return MAYBE_SAME
-    if keeper_root is not None and _relative_key(path, root) == _relative_key(
-        keeper, keeper_root
+    if keeper_root is not None and _same_tail(
+        _relative_key(path, root), _relative_key(keeper, keeper_root)
     ):
         return MAYBE_SAME
     return None
 
 
+def _same_tail(one, other):
+    """Whether one relative path is the other, or ends with it on a folder name.
+
+    Two mounts of one tree may be listed at different depths (``2024/x.png``
+    in one source, ``x.png`` in the other); ``ax.png`` never matches ``x.png``.
+    """
+    if one is None or other is None:
+        return False
+    return one == other or one.endswith("/" + other) or other.endswith("/" + one)
+
+
 def _relative_key(path, root):
     try:
         relative = os.path.relpath(str(path), str(root))

py/api/duplicate_spellings.py

@@ -18,6 +18,8 @@ Blocking: each lookup stats the file's folders once (cached).
 import os
 from pathlib import Path
 
+from .images import _favorite_relative_key, _favorite_root_key
+
 
 def _identity(path):
     try:
@@ -86,3 +88,46 @@ class Spellings:
             if inside is not None:
                 held[root] = Path(os.path.relpath(path, inside)).as_posix()
         return held
+
+
+def copy_move(target, kept_stat, spellings):
+    """What ``move_copy_records`` moves for one removal.
+
+    A record found under a folder spelling moves to the kept copy under
+    the same spelling when that folder holds it too, else to its real
+    path. Its favorite moves under every listed root holding the file.
+    """
+    removed = spellings.of(target["abs"])
+    kept = spellings.of(target["keeper_abs"])
+    targets = {
+        spelled: kept.get(folder, target["keeper_abs"])
+        for folder, spelled in removed.items()
+    }
+    kept_favorites = spellings.favorites(target["keeper_abs"])
+    favorites = []
+    for root, relative in spellings.favorites(target["abs"]).items():
+        kept_root, kept_rel = (
+            (root, kept_favorites[root])
+            if root in kept_favorites
+            else (target["keeper_root"], target["keeper_rel"])
+        )
+        favorites.append(
+            (
+                _favorite_root_key(root),
+                _favorite_relative_key(relative),
+                _favorite_root_key(kept_root),
+                _favorite_relative_key(kept_rel),
+            )
+        )
+    return {
+        "removed": list(removed.values()),
+        "kept": list(kept.values()),
+        "targets": targets,
+        "kept_path": target["keeper_abs"],
+        "favorites": favorites,
+        "removed_locator": target["locator"],
+        "kept_locator": target["keeper_locator"],
+        "sha256": target["hash"],
+        "kept_size": kept_stat.st_size,
+        "kept_mtime_ns": kept_stat.st_mtime_ns,
+    }

tests/test_api_duplicate_safety.py

@@ -474,3 +474,72 @@ class TestLockedRowSaysWhereLinksAre(SafetyTestCase):
         self.assertIn(
             "its links now name the kept copy", cleanup.outcomes["errors"][0]["detail"]
         )
+
+
+class TestRound3(SafetyTestCase):
+    """Re-review 2: n1 (sources at different depths) and n2 (the kept copy
+    must still be the held file right before the unlink)."""
+
+    MAYBE = TestAnotherMount.MAYBE
+
+    def setUp(self):
+        super().setUp()
+        self.real_lstat, self.real_stat = os.lstat, os.stat
+
+    mount = TestAnotherMount.mount
+    review_with = TestAnotherMount.review_with
+
+    async def test_a_deeper_source_on_another_mount_is_protected(self):
+        # The reviewer's probe: ComfyUI/output -> /mnt/pool/output and
+        # /mnt/disk1/output/2024 as an extra root, FUSE-style inodes.
+        _keeper, other, patches = self.mount("x.png", "2024/x.png", ino="other")
+        _job, _group, plan = await self.review_with(patches, "2024/x.png")
+        self.assertEqual(plan["protected"]["reasons"], [self.MAYBE])
+        self.assertTrue(other.is_file())
+
+    async def test_a_shallower_source_on_another_mount_is_protected(self):
+        _keeper, _other, patches = self.mount("2024/x.png", "x.png", ino="other")
+        _job, _group, plan = await self.review_with(patches, "x.png")
+        self.assertEqual(plan["protected"]["reasons"], [self.MAYBE])
+
+    async def test_only_whole_folder_names_match(self):
+        _keeper, _other, patches = self.mount("photo-x.png", "x.png", ino="other")
+        _job, _group, plan = await self.review_with(patches, "x.png")
+        self.assertEqual(plan["scope"]["count"], 1)
+
+    async def removal_while_the_keeper_goes(self, change):
+        self.write("a.png", b"same", mtime=1)
+        self.write("b.png", b"same", mtime=2)
+        prompt = self.save("On b")
+        self.api.db.link_image_to_prompt(prompt, str(self.output_dir / "b.png"))
+        job, group = await self.found_group()
+        plan = await self.review(job.id, [self.keep_of(group, "a.png")])
+        real = self.api.db.move_copy_records
+
+        def move(*args, **kwargs):
+            counts = real(*args, **kwargs)
+            if args[0]["kept_path"].endswith("a.png"):
+                change(self.output_dir / "a.png")
+            return counts
+
+        with patch.object(self.api.db, "move_copy_records", side_effect=move):
+            cleanup = await self.apply_plan(plan)
+        self.assertTrue((self.output_dir / "b.png").is_file())
+        self.assertEqual(cleanup.counters["removed"], 0)
+        rows = {row["path"]: row["detail"] for row in cleanup.outcomes["kept"]}
+        self.assertEqual(rows["b.png"], duplicate_files.KEEPER_WENT)
+        self.assertEqual(self.linked_files(), [])
+        [image] = self.api.db.get_prompt_images(prompt)
+        self.assertEqual(os.path.basename(image["image_path"]), "b.png")
+        self.assertIn("1 not removed", cleanup.status_line)
+
+    async def test_a_kept_copy_deleted_during_the_move_keeps_the_file(self):
+        await self.removal_while_the_keeper_goes(lambda path: path.unlink())
+
+    async def test_a_kept_copy_replaced_during_the_move_keeps_the_file(self):
+        def replace(path):
+            spare = path.with_name("spare.tmp")
+            spare.write_bytes(b"other bytes")
+            os.replace(spare, path)
+
+        await self.removal_while_the_keeper_goes(replace)