diff --git a/TESTING.md b/TESTING.md index a46a9d07..4be830a1 100644 --- a/TESTING.md +++ b/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 diff --git a/py/api/duplicate_files.py b/py/api/duplicate_files.py index 75f6330d..6edf265d 100644 --- a/py/api/duplicate_files.py +++ b/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): diff --git a/py/api/duplicate_review.py b/py/api/duplicate_review.py index d31ccc04..79f64ba0 100644 --- a/py/api/duplicate_review.py +++ b/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)) diff --git a/py/api/duplicate_spellings.py b/py/api/duplicate_spellings.py index 91455792..31e0f0b9 100644 --- a/py/api/duplicate_spellings.py +++ b/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, + } diff --git a/tests/test_api_duplicate_safety.py b/tests/test_api_duplicate_safety.py index 50fe08b8..c2302346 100644 --- a/tests/test_api_duplicate_safety.py +++ b/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)