diff --git a/NEWS.md b/NEWS.md index 78c8731e2..1405af4f6 100644 --- a/NEWS.md +++ b/NEWS.md @@ -18,6 +18,13 @@ border-radius: 128px; - Redesigned the Admin Tagging Status table to be more informative. - Fixes + - Comics are never deleted from the database while their files are still on + disk, so a misread filesystem event can no longer take a comic's bookmarks + and read progress with it. + - A comic replaced in place, by a tool that removes and rewrites the file, + is re-read instead of deleted and re-added as a new comic. + - Deleting a folder refreshes the series and publishers it emptied, which + kept listing comics that were gone. - Renaming follows a comic to its new path. Tagging a CBR converts it to CBZ without the rename failing, and a watched library no longer mistakes an unrelated new file for a renamed comic. diff --git a/codex/librarian/fs/import_task.py b/codex/librarian/fs/import_task.py index 40f4b621c..830d988f3 100644 --- a/codex/librarian/fs/import_task.py +++ b/codex/librarian/fs/import_task.py @@ -48,8 +48,28 @@ def _remove_paths(kwargs: dict[str, Any], deleted_key: str, moved_key: str) -> N del kwargs[moved_key][src_path] +def _replaced_paths(kwargs: dict[str, Any], added_key: str, deleted_key: str) -> set: + """ + Take paths reported both deleted and added; they were replaced in place. + + An external tool that swaps a file by ``rm`` + ``mv``, or a watcher + backend that reports an atomic replace as a delete plus an add, leaves + both events in one batch. The recreated file carries a new inode, so + move detection can never pair them — and letting the delete win would + destroy the row, and its bookmarks, while a file sits at that very + path. It is the same path with new content: a modification. + """ + replaced = kwargs[added_key] & kwargs[deleted_key] + kwargs[added_key] -= replaced + kwargs[deleted_key] -= replaced + return replaced + + def _deduplicate(kwargs: dict[str, Any]) -> None: """Prune conflicting events on the same paths.""" + replaced_files = _replaced_paths(kwargs, "files_added", "files_deleted") + replaced_covers = _replaced_paths(kwargs, "covers_added", "covers_deleted") + # deleted wins over moved-from-this-source _remove_paths(kwargs, "dirs_deleted", "dirs_moved") _remove_paths(kwargs, "files_deleted", "files_moved") @@ -81,6 +101,11 @@ def _deduplicate(kwargs: dict[str, Any]) -> None: kwargs["covers_modified"] -= kwargs["covers_deleted"] kwargs["covers_modified"] -= kwargs["covers_added"] + # Added last: a replaced path is neither created nor deleted, and the + # subtractions above would have stripped it back out. + kwargs["files_modified"] |= replaced_files + kwargs["covers_modified"] |= replaced_covers + def build_import_task( library_id: int, diff --git a/codex/librarian/scribe/importer/delete/__init__.py b/codex/librarian/scribe/importer/delete/__init__.py index 74eeb9330..fcd7e2334 100644 --- a/codex/librarian/scribe/importer/delete/__init__.py +++ b/codex/librarian/scribe/importer/delete/__init__.py @@ -11,12 +11,20 @@ def delete(self) -> None: """Delete files and folders.""" if self.abort_event.is_set(): return - self.counts.folders_deleted += self.bulk_folders_deleted() + folders_deleted, comics_cascaded, folder_collections = ( + self.bulk_folders_deleted() + ) + self.counts.folders_deleted += folders_deleted + # Comics under a deleted folder die by cascade, not by path, so they + # never reach ``bulk_comics_deleted`` to be counted there. + self.counts.comics_deleted += comics_cascaded if self.abort_event.is_set(): return - self.counts.comics_deleted, deleted_comic_collections = ( - self.bulk_comics_deleted() - ) + comics_deleted, deleted_comic_collections = self.bulk_comics_deleted() + self.counts.comics_deleted += comics_deleted + for model, pks in folder_collections.items(): + if pks: + deleted_comic_collections.setdefault(model, set()).update(pks) if self.abort_event.is_set(): return self.counts.covers_deleted = self.bulk_covers_deleted() diff --git a/codex/librarian/scribe/importer/delete/comics.py b/codex/librarian/scribe/importer/delete/comics.py index 68bd3512a..1087794a6 100644 --- a/codex/librarian/scribe/importer/delete/comics.py +++ b/codex/librarian/scribe/importer/delete/comics.py @@ -2,6 +2,7 @@ from codex.librarian.scribe.importer.const import ALL_COMIC_COLLECTION_FIELD_NAMES from codex.librarian.scribe.importer.delete.covers import DeletedCoversImporter +from codex.librarian.scribe.importer.delete.existence import confirm_deleted from codex.librarian.scribe.importer.statii.delete import ImporterRemoveComicsStatus from codex.models import Comic, Folder, StoryArc from codex.settings import ( @@ -9,6 +10,12 @@ IMPORTER_LINK_FK_BATCH_SIZE, ) +# A delete this large, and this much of the library, reads like a vanished +# mount rather than a user tidying up. The floor keeps small libraries from +# tripping it whenever a couple of comics are removed. +_MASS_DELETE_FLOOR = 50 +_MASS_DELETE_FRACTION = 0.5 + class DeletedComicsImporter(DeletedCoversImporter): """Delete comics methods.""" @@ -54,6 +61,26 @@ def _populate_deleted_comic_collections( ): cls._populate_deleted_comic_collection(deleted_comic_collections, comic) + def _warn_on_mass_delete(self, num_deleted: int) -> None: + """ + Flag a delete large enough to look like the library vanished. + + An unmounted volume or dropped network share makes every path read + as missing, which the existence backstop cannot tell from a real + mass deletion. Nothing is blocked here — this only leaves a + breadcrumb in the log for a user asking where their comics went. + """ + if num_deleted < _MASS_DELETE_FLOOR: + return + total = Comic.objects.filter(library=self.library).count() + if total and num_deleted >= total * _MASS_DELETE_FRACTION: + reason = ( + f"Deleting {num_deleted} of {total} comics in" + f" {self.library.path}. If that library lives on a network" + f" share or removable volume, check that it is still mounted." + ) + self.log.warning(reason) + def bulk_comics_deleted(self, **kwargs) -> tuple[int, dict]: """Bulk delete comics found missing from the filesystem.""" count = 0 @@ -64,8 +91,11 @@ def bulk_comics_deleted(self, **kwargs) -> tuple[int, dict]: return count, deleted_comic_collections self.status_controller.start(status) # Batch path__in to stay under SQLite's variable limit. - paths = tuple(self.task.files_deleted) + paths = confirm_deleted(self.task.files_deleted, self.log, "comics") self.task.files_deleted = frozenset() + if not paths: + return count, deleted_comic_collections + self._warn_on_mass_delete(len(paths)) delete_comic_pks: set[int] = set() for start in range(0, len(paths), IMPORTER_LINK_FK_BATCH_SIZE): if self.abort_event.is_set(): diff --git a/codex/librarian/scribe/importer/delete/covers.py b/codex/librarian/scribe/importer/delete/covers.py index 61c3f4739..1e590efad 100644 --- a/codex/librarian/scribe/importer/delete/covers.py +++ b/codex/librarian/scribe/importer/delete/covers.py @@ -1,6 +1,7 @@ """Clean up covers from the db.""" from codex.librarian.covers.tasks import CoverRemoveTask +from codex.librarian.scribe.importer.delete.existence import confirm_deleted from codex.librarian.scribe.importer.search import SearchIndexImporter from codex.librarian.scribe.importer.statii.delete import ImporterRemoveCoversStatus from codex.models.paths import CustomCover @@ -23,10 +24,11 @@ def bulk_covers_deleted(self, **kwargs) -> int: if not self.task.covers_deleted: return 0 self.status_controller.start(status) - covers = CustomCover.objects.filter( - library=self.library, path__in=self.task.covers_deleted - ) + paths = confirm_deleted(self.task.covers_deleted, self.log, "covers") self.task.covers_deleted = frozenset() + if not paths: + return 0 + covers = CustomCover.objects.filter(library=self.library, path__in=paths) delete_cover_pks = frozenset(covers.values_list("pk", flat=True)) count, _ = covers.delete() diff --git a/codex/librarian/scribe/importer/delete/existence.py b/codex/librarian/scribe/importer/delete/existence.py new file mode 100644 index 000000000..898416f0a --- /dev/null +++ b/codex/librarian/scribe/importer/delete/existence.py @@ -0,0 +1,49 @@ +""" +Confirm a scanner's deletes against the filesystem before acting on them. + +Deleting a comic row cascades its bookmarks and read progress away, and +nothing restores them — the next scan re-imports the file as a fresh, +unread comic. So a delete is only safe when the file is really gone. + +Both scanners infer deletes rather than observing them, and every inference +they make has failure modes: a watcher batch that reports a delete whose +paired add lands in the *next* batch, a directory expansion that overmatched, +an inode pair the compatibility checks refused. In each case the path is +still on disk, and the delete is wrong. + +Probing the path is cheap next to what it protects, and a path that is +genuinely gone answers immediately. A row skipped here is not stranded: it +still points at a real file, so the next scan reconciles it normally — a +stale row costs a re-read, a wrongly deleted one costs the user's place in +the book. + +This cannot save a library whose whole mount disappeared, where every path +reads as missing. ``DeletedComicsImporter`` logs that case instead. +""" + +from collections.abc import Collection +from pathlib import Path + + +def split_extant(paths: Collection[str]) -> tuple[tuple[str, ...], tuple[str, ...]]: + """Partition paths into (gone from disk, still on disk).""" + gone: list[str] = [] + extant: list[str] = [] + for path in paths: + if Path(path).exists(): + extant.append(path) + else: + gone.append(path) + return tuple(gone), tuple(extant) + + +def confirm_deleted(paths: Collection[str], log, kind: str) -> tuple[str, ...]: + """Return only the paths that are really gone, reporting any that aren't.""" + gone, extant = split_extant(paths) + if extant: + reason = ( + f"Not deleting {len(extant)} {kind} a scan reported missing that" + f" are still on disk. The next scan will reconcile them." + ) + log.warning(reason) + return gone diff --git a/codex/librarian/scribe/importer/delete/folders.py b/codex/librarian/scribe/importer/delete/folders.py index fd4307d3d..5245eda7b 100644 --- a/codex/librarian/scribe/importer/delete/folders.py +++ b/codex/librarian/scribe/importer/delete/folders.py @@ -1,6 +1,7 @@ """Delete database folders methods.""" from codex.librarian.scribe.importer.delete.comics import DeletedComicsImporter +from codex.librarian.scribe.importer.delete.existence import confirm_deleted from codex.librarian.scribe.importer.statii.delete import ImporterRemoveFoldersStatus from codex.models.collections import Folder from codex.models.comic import Comic @@ -9,26 +10,39 @@ class DeletedFoldersImporter(DeletedComicsImporter): """Delete database folders methods.""" - def bulk_folders_deleted(self, **kwargs) -> int: - """Bulk delete folders.""" + def bulk_folders_deleted(self, **kwargs) -> tuple[int, int, dict]: + """ + Bulk delete folders. Return (folders, cascaded comics, collections). + + Comics under a deleted folder die by the ``parent_folder`` cascade + rather than through ``bulk_comics_deleted``, so their collections + are gathered here too. Without them a series or publisher emptied by + a folder delete is never re-stamped, and browsers viewing it keep + listing comics that are gone until some unrelated import moves the + timestamp. + """ status = ImporterRemoveFoldersStatus(0, len(self.task.dirs_deleted)) + deleted_comic_collections = self._init_deleted_comic_collections() try: if not self.task.dirs_deleted: - return 0 + return 0, 0, deleted_comic_collections self.status_controller.start(status) - folders = Folder.objects.filter( - library=self.library, path__in=self.task.dirs_deleted - ) + paths = confirm_deleted(self.task.dirs_deleted, self.log, "folders") self.task.dirs_deleted = frozenset() - delete_comic_pks = frozenset( - Comic.objects.filter(library=self.library, folders__in=folders) - .distinct() - .values_list("pk", flat=True) + if not paths: + return 0, 0, deleted_comic_collections + folders = Folder.objects.filter(library=self.library, path__in=paths) + folder_count = folders.count() + delete_comic_qs = Comic.objects.filter( + library=self.library, folders__in=folders + ).distinct() + self._populate_deleted_comic_collections( + delete_comic_qs, deleted_comic_collections ) + delete_comic_pks = frozenset(delete_comic_qs.values_list("pk", flat=True)) folders.delete() - count = len(delete_comic_pks) self.remove_covers(delete_comic_pks, custom=False) finally: self.status_controller.finish(status) - return count + return folder_count, len(delete_comic_pks), deleted_comic_collections diff --git a/tests/importer/test_delete_backstop.py b/tests/importer/test_delete_backstop.py new file mode 100644 index 000000000..e6e3c78c0 --- /dev/null +++ b/tests/importer/test_delete_backstop.py @@ -0,0 +1,187 @@ +""" +Deletes are confirmed against the filesystem before they cascade. + +Both scanners *infer* deletes, and every inference has failure modes that +name a path still sitting on disk — a split watch batch, an overmatched +directory expansion, a refused inode pair. Acting on one destroys the +comic's bookmarks and read progress permanently, so the delete phase +checks the disk first and leaves anything still there for the next scan. +""" + +import shutil +from pathlib import Path +from threading import Event, Lock +from typing import override +from unittest.mock import MagicMock + +from loguru import logger + +from codex.librarian.mp_queue import LIBRARIAN_QUEUE +from codex.librarian.scribe.importer.importer import ComicImporter +from codex.librarian.scribe.importer.tasks import ImportTask +from codex.models import ( + Comic, + Folder, + Imprint, + Library, + Publisher, + Series, + Volume, +) +from tests.importer.test_basic import ( + COMIC_PATH, + LIBRARY_PATH, + BaseTestImporter, +) + +_GONE = str(LIBRARY_PATH / "gone.cbz") +_EXTANT = str(LIBRARY_PATH / "still-here.cbz") +_SUBDIR = LIBRARY_PATH / "subdir" + + +class _DeleteTestBase(BaseTestImporter): + """A library with a folder row and comic-creation helpers.""" + + @override + def setUp(self) -> None: + super().setUp() + self.library = Library.objects.get(pk=self.task.library_id) + self.folder = Folder.objects.create( + library=self.library, path=str(LIBRARY_PATH), name=LIBRARY_PATH.name + ) + pub = Publisher.objects.create(name="Delete Pub") + imp = Imprint.objects.create(name="Delete Imprint", publisher=pub) + ser = Series.objects.create(name="Delete Series", imprint=imp, publisher=pub) + self.tags = { + "publisher": pub, + "imprint": imp, + "series": ser, + "volume": Volume.objects.create( + name="1", series=ser, imprint=imp, publisher=pub + ), + } + self.issue_number = 0 + + def _create_comic(self, path: str, folder: Folder | None = None) -> Comic: + """Create a comic with its file present, as presave stats disk.""" + Path(path).parent.mkdir(parents=True, exist_ok=True) + shutil.copy(COMIC_PATH, path) + self.issue_number += 1 + comic = Comic.objects.create( + library=self.library, + path=path, + parent_folder=folder or self.folder, + issue_number=self.issue_number, + name=Path(path).stem, + size=1, + page_count=1, + **self.tags, + ) + comic.folders.add(folder or self.folder) + return comic + + def _make_subdir_folder(self) -> Folder: + """Track a subdirectory, present on disk as its row requires.""" + _SUBDIR.mkdir(parents=True, exist_ok=True) + return Folder.objects.create( + library=self.library, path=str(_SUBDIR), name=_SUBDIR.name + ) + + def _delete(self, **task_kwargs) -> ComicImporter: + """Run the delete phase with a fresh importer, as a scan would.""" + task = ImportTask(library_id=self.library.pk, **task_kwargs) + importer = ComicImporter(task, logger, LIBRARIAN_QUEUE, Lock(), Event()) + importer.delete() + return importer + + +class TestDeleteExistenceBackstop(_DeleteTestBase): + """A path still on disk is never deleted from the database.""" + + def test_comic_still_on_disk_is_not_deleted(self) -> None: + """The whole point: a wrongly reported delete keeps its row.""" + comic = self._create_comic(_EXTANT) + + importer = self._delete(files_deleted=frozenset({_EXTANT})) + + assert Comic.objects.filter(pk=comic.pk).exists() + assert importer.counts.comics_deleted == 0 + + def test_comic_missing_from_disk_is_deleted(self) -> None: + """A real delete still deletes.""" + comic = self._create_comic(_GONE) + Path(_GONE).unlink() + + importer = self._delete(files_deleted=frozenset({_GONE})) + + assert not Comic.objects.filter(pk=comic.pk).exists() + assert importer.counts.comics_deleted == 1 + + def test_only_the_extant_path_is_spared(self) -> None: + """A mixed batch deletes what is gone and keeps what isn't.""" + gone = self._create_comic(_GONE) + extant = self._create_comic(_EXTANT) + Path(_GONE).unlink() + + self._delete(files_deleted=frozenset({_GONE, _EXTANT})) + + assert not Comic.objects.filter(pk=gone.pk).exists() + assert Comic.objects.filter(pk=extant.pk).exists() + + def test_folder_still_on_disk_is_not_deleted(self) -> None: + """A folder delete that would cascade comics is checked too.""" + subdir_folder = self._make_subdir_folder() + comic = self._create_comic(str(_SUBDIR / "c.cbz"), folder=subdir_folder) + + self._delete(dirs_deleted=frozenset({str(_SUBDIR)})) + + assert Folder.objects.filter(pk=subdir_folder.pk).exists() + assert Comic.objects.filter(pk=comic.pk).exists() + + def test_deleted_folder_cascade_is_counted_and_restamped(self) -> None: + """Comics dying by folder cascade are counted, and re-stamp their series.""" + subdir_folder = self._make_subdir_folder() + comic = self._create_comic(str(_SUBDIR / "c.cbz"), folder=subdir_folder) + series_pk = comic.series.pk + stamped_before = Series.objects.get(pk=series_pk).updated_at + shutil.rmtree(_SUBDIR) + + importer = self._delete(dirs_deleted=frozenset({str(_SUBDIR)})) + + assert not Comic.objects.filter(pk=comic.pk).exists() + # Counted as one folder and one comic, not one folder-shaped comic. + assert importer.counts.folders_deleted == 1 + assert importer.counts.comics_deleted == 1 + # The emptied series must be re-stamped or browsers keep listing it. + assert Series.objects.get(pk=series_pk).updated_at > stamped_before + + +class TestMassDeleteWarning(_DeleteTestBase): + """A delete big enough to look like a vanished mount is flagged.""" + + def _importer(self) -> tuple[ComicImporter, MagicMock]: + """Build an importer whose log is captured for assertions.""" + task = ImportTask(library_id=self.library.pk) + importer = ComicImporter(task, logger, LIBRARIAN_QUEUE, Lock(), Event()) + mock_log = MagicMock() + importer.log = mock_log + return importer, mock_log + + def test_small_delete_is_quiet(self) -> None: + """Ordinary tidying up must not cry wolf.""" + self._create_comic(_EXTANT) + importer, mock_log = self._importer() + + importer._warn_on_mass_delete(1) # noqa: SLF001 + + mock_log.warning.assert_not_called() + + def test_large_delete_warns(self) -> None: + """Losing most of a library logs where to look.""" + self._create_comic(_EXTANT) + importer, mock_log = self._importer() + + importer._warn_on_mass_delete(500) # noqa: SLF001 + + mock_log.warning.assert_called_once() + assert "mounted" in mock_log.warning.call_args[0][0] diff --git a/tests/importer/test_tag_write_move_guard.py b/tests/importer/test_tag_write_move_guard.py index 766b297f7..d72ecbd82 100644 --- a/tests/importer/test_tag_write_move_guard.py +++ b/tests/importer/test_tag_write_move_guard.py @@ -133,6 +133,10 @@ def test_create_of_a_pending_move_path_is_deferred(self) -> None: def test_unregistered_paths_are_untouched(self) -> None: """A guard for one comic never defers another comic's delete.""" comic = self._create_comic(_UNRELATED_PATH) + # Really remove it: the delete phase spares rows whose file is still + # on disk, so a fixture that leaves the file behind would pass for + # the wrong reason. + Path(_UNRELATED_PATH).unlink() self._register_conversion() importer = self._importer(files_deleted=frozenset({_UNRELATED_PATH})) diff --git a/tests/test_import_task_build.py b/tests/test_import_task_build.py index bf536643e..af540109f 100644 --- a/tests/test_import_task_build.py +++ b/tests/test_import_task_build.py @@ -89,3 +89,55 @@ def test_no_work_returns_none(): ) assert task is None + + +def _added(path: str) -> FSEvent: + return FSEvent(src_path=path, change=FSChange.added) + + +def _deleted(path: str) -> FSEvent: + return FSEvent(src_path=path, change=FSChange.deleted) + + +def test_delete_plus_add_of_one_path_is_a_modify(): + """A file replaced in place must be re-read, not deleted.""" + task = _build(_deleted(_OLD), _added(_OLD)) + + assert task + # Letting the delete win would destroy the row — and its bookmarks — + # while a file sits at that path. + assert task.files_deleted == set() + assert task.files_created == set() + assert task.files_modified == {_OLD} + + +def test_replaced_path_survives_alongside_a_real_delete(): + """One path's replacement doesn't rescue another path's real delete.""" + task = _build(_deleted(_OLD), _added(_OLD), _deleted(_OTHER)) + + assert task + assert task.files_modified == {_OLD} + assert task.files_deleted == {_OTHER} + + +def test_replaced_path_with_a_modify_event_is_not_duplicated(): + """A batch that also reports the write leaves one modify, not two.""" + task = _build(_deleted(_OLD), _added(_OLD), _modified(_OLD)) + + assert task + assert task.files_modified == {_OLD} + assert task.files_deleted == set() + + +def test_replaced_cover_is_a_modify(): + """Custom covers replaced in place are re-read too.""" + cover = "/comics/Series/cover.jpg" + task = _build( + FSEvent(src_path=cover, change=FSChange.deleted, is_cover=True), + FSEvent(src_path=cover, change=FSChange.added, is_cover=True), + ) + + assert task + assert task.covers_deleted == set() + assert task.covers_created == set() + assert task.covers_modified == {cover}