From bf951f469024dc25ad2c4a5e05ea13a25d84f5e8 Mon Sep 17 00:00:00 2001 From: AJ Slater Date: Tue, 25 Aug 2026 16:16:27 -0700 Subject: [PATCH 1/7] fix(tagging): rename archives with their own file extension MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ``ext`` is a metadata field, not the file's suffix, and codex's read config deletes it — so comicfn2dict fell back to its "cbz" default and every PDF/CBR/CBT/CB7 was renamed to a name claiming to be a zip. The admin preview showed the same wrong name. Codex now performs the rename itself. Comicbox's ``rename_file`` derives its own destination and cannot be handed a corrected target, so owning the move is what makes the suffix correctable. A rendered name that is nothing but an extension would create a hidden file, so it is treated as no name at all. Co-Authored-By: Claude Fable 5 --- codex/librarian/scribe/tag_writer.py | 41 +++++++++++--------- codex/views/admin/tagwrite.py | 12 ++++-- tests/test_tag_writer_rename.py | 56 ++++++++++++++++++++++------ 3 files changed, 78 insertions(+), 31 deletions(-) diff --git a/codex/librarian/scribe/tag_writer.py b/codex/librarian/scribe/tag_writer.py index 982e58d7e..7dfe25332 100644 --- a/codex/librarian/scribe/tag_writer.py +++ b/codex/librarian/scribe/tag_writer.py @@ -215,26 +215,33 @@ def _rename_one(self, old_path: Path) -> Path | None: Returns the new path, or None when the name is unchanged or no name could be built. Raises ``FileExistsError`` on a collision with a - *different* file so the caller reports it without clobbering anything - (comicbox's ``rename_file`` does a bare ``Path.rename``). + *different* file so the caller reports it without clobbering anything. + + Codex performs the rename itself rather than calling comicbox's + ``rename_file``, which derives its own destination and does a bare + ``Path.rename`` — there is no way to hand it a corrected target, and + the extension it renders is not the archive's own (see below). """ with Comicbox(old_path, config=COMICBOX_CONFIG) as car: - # to_string(FILENAME) is exactly what rename_file() derives the - # name from (schema.dumps(_to_dict(FILENAME))), so this pre-check - # targets the precise destination rename_file() will use. target = car.to_string(MetadataFormats.FILENAME) - if not target: - self.log.warning(f"Rename skipped; no filename built for {old_path}") - return None - new_path = old_path.parent / target - if new_path == old_path: - return None - if new_path.exists() and not new_path.samefile(old_path): - reason = f"rename target already exists: {new_path}" - raise FileExistsError(reason) - car.rename_file() - renamed = car.get_path() - return renamed or new_path + # A rendered name always ends in an extension, but not necessarily + # this file's: ``ext`` is a *metadata* field, and codex's read config + # deletes it, so comicfn2dict falls back to its "cbz" default and + # every PDF/CBR/CBT/CB7 would be renamed to a ".cbz" name it isn't. + # The archive on disk is the authority, so keep its real suffix. + # A name that is nothing but an extension (no metadata parsed at all) + # would make a hidden file, so treat it as no name at all. + if not target or target.startswith("."): + self.log.warning(f"Rename skipped; no filename built for {old_path}") + return None + new_path = (old_path.parent / target).with_suffix(old_path.suffix) + if new_path == old_path: + return None + if new_path.exists() and not new_path.samefile(old_path): + reason = f"rename target already exists: {new_path}" + raise FileExistsError(reason) + old_path.rename(new_path) + return new_path def _rename_comics( self, diff --git a/codex/views/admin/tagwrite.py b/codex/views/admin/tagwrite.py index 9c0aa85ad..132467b62 100644 --- a/codex/views/admin/tagwrite.py +++ b/codex/views/admin/tagwrite.py @@ -109,15 +109,21 @@ def _preview_one(old_path: Path, metadata: dict | None, config) -> str: Return the comicbox-scheme name the given patch produces for one comic. Overlays the pending (unsaved) patch onto the archive's metadata in - memory and serializes the FILENAME format — the same construction - ``rename_file`` uses — so the dialog can show the would-be name. Opens + memory and serializes the FILENAME format — the same construction the + rename pass uses — so the dialog can show the would-be name. Opens the archive (I/O). Returns "" when no name could be built. """ try: with Comicbox(old_path, config=config, metadata=metadata) as car: - return car.to_string(MetadataFormats.FILENAME) or "" + target = car.to_string(MetadataFormats.FILENAME) or "" except Exception: return "" + # Mirrors ``TagWriter._rename_one``: the rendered extension is + # comicfn2dict's "cbz" default rather than this archive's, so the + # preview must show the real suffix the rename will keep. + if not target or target.startswith("."): + return "" + return Path(target).with_suffix(old_path.suffix).name def _filename_previews( self, diff --git a/tests/test_tag_writer_rename.py b/tests/test_tag_writer_rename.py index 7a28b1b5a..8282fe927 100644 --- a/tests/test_tag_writer_rename.py +++ b/tests/test_tag_writer_rename.py @@ -62,9 +62,9 @@ class _FakeComicbox: """ Stand-in for ``comicbox.box.Comicbox`` used by the rename pass. - ``to_string(FILENAME)`` returns a fixed scheme name and ``rename_file`` - actually moves the file on disk (mirroring comicbox) so the real - collision check, ``samefile``, and DB sync all run against the filesystem. + ``to_string(FILENAME)`` returns a fixed scheme name; the move itself is + codex's, so the real collision check, ``samefile``, and DB sync all run + against the filesystem. """ target: str = _TARGET_NAME @@ -81,14 +81,6 @@ def __exit__(self, *_exc: object) -> bool: def to_string(self, _fmt) -> str: return self.target - def rename_file(self) -> None: - new_path = self._path.parent / self.target - self._path.rename(new_path) - self._path = new_path - - def get_path(self) -> Path: - return self._path - def _make_comic(*, events: bool, name: str = "c.cbz", read_only: bool = False) -> Comic: _TMP_DIR.mkdir(exist_ok=True, parents=True) @@ -284,6 +276,48 @@ def test_collision_skips_and_reports(self) -> None: assert errors assert errors[0]["path"] == str(old_path) + def test_rename_keeps_the_archives_own_extension(self) -> None: + """ + A non-CBZ archive keeps its real suffix. + + ``ext`` is a metadata field that codex's read config deletes, so the + rendered scheme name always ends in comicfn2dict's "cbz" default. The + file on disk is the authority: a PDF must not be renamed to a name + claiming it is a zip. + """ + comic = _make_comic(events=False, name="c.pdf") + old_path = Path(comic.path) + queue = _FakeQueue() + writer = _make_writer(queue) + task = BulkTagWriteTask(comic_pks=frozenset({comic.pk}), rename=True) + + with patch(_COMICBOX_TARGET, _FakeComicbox): + writer.write_tags(task) + + new_path = old_path.parent / f"{Path(_TARGET_NAME).stem}.pdf" + assert new_path.exists() + assert not (old_path.parent / _TARGET_NAME).exists() + imports = [i for i in queue.items if isinstance(i, ImportTask)] + assert imports[0].files_moved == {str(old_path): str(new_path)} + + def test_rename_skipped_when_only_an_extension_is_rendered(self) -> None: + """A name with no stem would make a hidden file, so skip the rename.""" + comic = _make_comic(events=False) + old_path = Path(comic.path) + queue = _FakeQueue() + writer = _make_writer(queue) + task = BulkTagWriteTask(comic_pks=frozenset({comic.pk}), rename=True) + + with ( + patch(_COMICBOX_TARGET, _FakeComicbox), + patch.object(_FakeComicbox, "target", ".cbz"), + ): + writer.write_tags(task) + + assert old_path.exists() + assert not (old_path.parent / ".cbz").exists() + assert not [i for i in queue.items if isinstance(i, ImportTask)] + def test_no_change_when_name_matches(self) -> None: """When the scheme name equals the current name, nothing happens.""" comic = _make_comic(events=False, name=_TARGET_NAME) From ea3d03e64ff58be08ddcb5da71a66bebb984ac69 Mon Sep 17 00:00:00 2001 From: AJ Slater Date: Tue, 25 Aug 2026 16:16:34 -0700 Subject: [PATCH 2/7] fix(watcher): match path prefixes on directory boundaries MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both the deleted-directory expansion and the library attributor compared bare string prefixes, so any two paths where one name merely began with the other were treated as parent and child. Deleting a watched folder therefore expanded into every sibling tree sharing its leading name — "Batman" collecting all of "Batman Beyond" — and those comics were deleted, with no paired add to rescue them and their bookmarks cascading away while the files were still on disk. The same bug filed a sibling library's events under whichever library happened to be a string prefix of it. Terminating each prefix with a separator restores the boundary. The library root itself still matches its own events. Co-Authored-By: Claude Fable 5 --- codex/librarian/fs/watcher/dirs.py | 20 ++-- codex/librarian/fs/watcher/events.py | 10 +- tests/test_watcher_path_prefixes.py | 147 +++++++++++++++++++++++++++ 3 files changed, 168 insertions(+), 9 deletions(-) create mode 100644 tests/test_watcher_path_prefixes.py diff --git a/codex/librarian/fs/watcher/dirs.py b/codex/librarian/fs/watcher/dirs.py index 936940366..72e5ff6ca 100644 --- a/codex/librarian/fs/watcher/dirs.py +++ b/codex/librarian/fs/watcher/dirs.py @@ -57,23 +57,27 @@ def expand_dir_deleted(dir_path: str, library_pk: int, batch: ChangeBatch) -> No ) ) + # Only true children. A bare prefix match also claims every sibling whose + # name merely starts with this one ("/c/Batman" would delete "/c/Batman + # Beyond" and all its comics), so terminate the prefix with a separator. + child_prefix = dir_path.rstrip(os.sep) + os.sep + # Child folders child_folder_paths = Folder.objects.filter( - library_id=library_pk, path__startswith=dir_path + library_id=library_pk, path__startswith=child_prefix ).values_list("path", flat=True) for path in child_folder_paths: - if path != dir_path: - batch.dir_deleted.append( - ( - library_pk, - FSEvent(src_path=path, change=FSChange.deleted, is_directory=True), - ) + batch.dir_deleted.append( + ( + library_pk, + FSEvent(src_path=path, change=FSChange.deleted, is_directory=True), ) + ) # Child comics and failed imports for model in (Comic, FailedImport): child_paths = model.objects.filter( - library_id=library_pk, path__startswith=dir_path + library_id=library_pk, path__startswith=child_prefix ).values_list("path", flat=True) for path in child_paths: batch.deleted.append( diff --git a/codex/librarian/fs/watcher/events.py b/codex/librarian/fs/watcher/events.py index 560303ea1..5b272774e 100644 --- a/codex/librarian/fs/watcher/events.py +++ b/codex/librarian/fs/watcher/events.py @@ -7,6 +7,7 @@ 3. Move detection -> match delete+add pairs by inode within a batch """ +import os from pathlib import Path from watchfiles import Change @@ -67,7 +68,14 @@ def _process_change( def _find_library(library_paths: dict[str, int], file_path: str) -> int | None: """Find which library a changed path belongs to.""" for lib_path, pk in library_paths.items(): - if file_path.startswith(lib_path): + # Terminate the root with a separator before matching. Libraries may + # legitimately be siblings sharing a name prefix ("/c/comics" and + # "/c/comics-kids" — the admin serializer only rejects nesting), and a + # bare prefix match would file the second library's events under the + # first, importing its comics at paths outside their own library. + if file_path == lib_path or file_path.startswith( + lib_path.rstrip(os.sep) + os.sep + ): return pk return None diff --git a/tests/test_watcher_path_prefixes.py b/tests/test_watcher_path_prefixes.py new file mode 100644 index 000000000..465676a43 --- /dev/null +++ b/tests/test_watcher_path_prefixes.py @@ -0,0 +1,147 @@ +""" +Watcher path matching must respect directory boundaries. + +Both the deleted-directory expansion and the library attributor matched a +bare string prefix, so any two paths where one name starts with the other +("Batman" / "Batman Beyond") were treated as parent and child. Expanding a +delete that way destroys a sibling tree's comics — and their bookmarks — +while the files are still on disk. +""" + +from __future__ import annotations + +import shutil +from pathlib import Path +from typing import Final, override + +from django.test import TestCase + +from codex.librarian.fs.events import FSChange +from codex.librarian.fs.watcher.data import ChangeBatch +from codex.librarian.fs.watcher.dirs import expand_dir_deleted +from codex.librarian.fs.watcher.events import _find_library +from codex.models import ( + Comic, + Folder, + Imprint, + Library, + Publisher, + Series, + Volume, +) + +_ROOT: Final = Path("/tmp/codex.tests.watcherprefix") # noqa: S108 +_MAIN_PK: Final = 1 +_KIDS_PK: Final = 2 + + +class FindLibraryPrefixTests(TestCase): + """Events belong to the library that actually contains them.""" + + def test_sibling_library_prefix_does_not_capture_events(self) -> None: + """A sibling root sharing a name prefix never claims the other's events.""" + library_paths = {"/comics": _MAIN_PK, "/comics-kids": _KIDS_PK} + + assert _find_library(library_paths, "/comics-kids/x.cbz") == _KIDS_PK + assert _find_library(library_paths, "/comics/x.cbz") == _MAIN_PK + + def test_library_root_itself_matches(self) -> None: + """An event on the root directory still belongs to that library.""" + assert _find_library({"/comics": _MAIN_PK}, "/comics") == _MAIN_PK + + def test_unrelated_path_matches_nothing(self) -> None: + """A path outside every library root is unattributed.""" + assert _find_library({"/comics": _MAIN_PK}, "/comics-kids") is None + assert _find_library({"/comics": _MAIN_PK}, "/elsewhere/x.cbz") is None + + +class ExpandDirDeletedTests(TestCase): + """Deleting a directory only expands to its own children.""" + + @override + def setUp(self) -> None: + _ROOT.mkdir(parents=True, exist_ok=True) + self.library = Library.objects.create(path=str(_ROOT)) # pyright: ignore[reportUninitializedInstanceVariable] + publisher = Publisher.objects.create(name="P") + imprint = Imprint.objects.create(name="I", publisher=publisher) + series = Series.objects.create(name="S", publisher=publisher, imprint=imprint) + volume = Volume.objects.create( + name="1", publisher=publisher, imprint=imprint, series=series + ) + self.fks = { # pyright: ignore[reportUninitializedInstanceVariable] + "publisher": publisher, + "imprint": imprint, + "series": series, + "volume": volume, + } + + @override + def tearDown(self) -> None: + shutil.rmtree(_ROOT, ignore_errors=True) + + def _make_folder(self, path: Path) -> Folder: + path.mkdir(parents=True, exist_ok=True) + return Folder.objects.create( + library=self.library, path=str(path), name=path.name + ) + + def _make_comic(self, path: Path) -> Comic: + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text("comic") + return Comic.objects.create( + library=self.library, + path=str(path), + issue_number=1, + name=path.stem, + size=1, + file_type="CBZ", + **self.fks, + ) + + def test_sibling_prefix_tree_is_untouched(self) -> None: + """Deleting /Batman must not expand into /Batman Beyond.""" + target = _ROOT / "Batman" + sibling = _ROOT / "Batman Beyond" + self._make_folder(target) + self._make_folder(sibling) + target_comic = self._make_comic(target / "a.cbz") + sibling_comic = self._make_comic(sibling / "b.cbz") + + batch = ChangeBatch() + expand_dir_deleted(str(target), self.library.pk, batch) + + deleted_files = {event.src_path for _, event in batch.deleted} + deleted_dirs = {event.src_path for _, event in batch.dir_deleted} + assert deleted_files == {target_comic.path} + assert sibling_comic.path not in deleted_files + assert deleted_dirs == {str(target)} + assert str(sibling) not in deleted_dirs + + def test_real_children_are_expanded(self) -> None: + """Nested folders and their comics are still collected.""" + target = _ROOT / "Batman" + child = target / "Year One" + self._make_folder(target) + self._make_folder(child) + comic = self._make_comic(child / "a.cbz") + + batch = ChangeBatch() + expand_dir_deleted(str(target), self.library.pk, batch) + + assert {event.src_path for _, event in batch.deleted} == {comic.path} + assert {event.src_path for _, event in batch.dir_deleted} == { + str(target), + str(child), + } + assert all(event.change == FSChange.deleted for _, event in batch.dir_deleted) + + def test_directory_is_listed_once(self) -> None: + """The deleted directory's own row must not be emitted twice.""" + target = _ROOT / "Batman" + self._make_folder(target) + + batch = ChangeBatch() + expand_dir_deleted(str(target), self.library.pk, batch) + + paths = [event.src_path for _, event in batch.dir_deleted] + assert paths == [str(target)] From aa165135f7326dd968ad303586977bf3647b756c Mon Sep 17 00:00:00 2001 From: AJ Slater Date: Tue, 25 Aug 2026 16:16:41 -0700 Subject: [PATCH 3/7] fix(librarian): keep the scribe priority queue totally ordered MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two ways a scribe task could raise TypeError inside the queue: ``SHUTDOWN_MSG`` was a bare int where every real item is a ``(priority, timestamp)`` tuple, so stopping the thread with any task still queued raised comparing int to tuple — aborting the daemon's shutdown loop before the remaining threads were ever told to stop. Equal priorities fell through to comparing the ScribeTask dataclasses, which define no ordering. Timestamps tie more readily than they look (they are truncated, and a clock can step backwards), and the loser was a task dropped in the routing thread. A monotonic counter now closes the tuple so two entries can never compare equal. Co-Authored-By: Claude Fable 5 --- codex/librarian/scribe/priority.py | 14 +++++++++-- codex/librarian/scribe/scribed.py | 8 +++++- tests/test_scribe_priority.py | 39 +++++++++++++++++++++++++++++- 3 files changed, 57 insertions(+), 4 deletions(-) diff --git a/codex/librarian/scribe/priority.py b/codex/librarian/scribe/priority.py index 1212ef2f9..e2725551f 100644 --- a/codex/librarian/scribe/priority.py +++ b/codex/librarian/scribe/priority.py @@ -1,6 +1,7 @@ """Priority for Scribe tasks in the PriorityQueue.""" from datetime import UTC, datetime +from itertools import count from codex.librarian.scribe.importer.tasks import ( ImportTask, @@ -77,9 +78,18 @@ JanitorDumpUserDataTask, ) +# Final element of every priority tuple. Tasks are pushed onto a heap as +# ``(priority, task)``; when two priorities compare equal the heap falls +# through to comparing the tasks themselves, and ScribeTask dataclasses +# define no ordering — a ``TypeError`` in the routing thread that loses the +# task. Timestamps tie more often than they look like they would (they are +# truncated, and a clock can step backwards), so carry a strictly monotonic +# counter that can never tie. ``next`` on an ``itertools.count`` is atomic. +_TIE_BREAKER = count() -def get_task_priority(task: ScribeTask) -> tuple[int, float]: + +def get_task_priority(task: ScribeTask) -> tuple[int, float, int]: """Get task priority by index.""" now = datetime.now(tz=UTC).timestamp() priority = _SCRIBE_TASK_PRIORITY.index(type(task)) - return priority, now + return priority, now, next(_TIE_BREAKER) diff --git a/codex/librarian/scribe/scribed.py b/codex/librarian/scribe/scribed.py index 7c898bed3..deb33f876 100644 --- a/codex/librarian/scribe/scribed.py +++ b/codex/librarian/scribe/scribed.py @@ -48,7 +48,13 @@ class ScribeThread(QueuedThread): """A worker to handle all bulk database updates.""" - SHUTDOWN_MSG = (0, QueuedThread.SHUTDOWN_MSG) + # Shaped like a real queue item — ``(get_task_priority(...), task)`` — + # because a PriorityQueue orders the shutdown message against whatever + # is already queued. A bare int here raises TypeError comparing int to + # tuple, so stopping the thread with a task pending would abort the + # daemon's shutdown loop. The negative index sorts it ahead of every + # task so a stop is honored promptly. + SHUTDOWN_MSG = ((-1, 0.0, -1), QueuedThread.SHUTDOWN_MSG) # Importer / janitor / search bursts are minutes-to-hours apart on # a typical install. Releasing the conn between bursts saves an # open file handle + ~50 KiB pinned for the entire idle gap; the diff --git a/tests/test_scribe_priority.py b/tests/test_scribe_priority.py index b267b439b..ffa1b108f 100644 --- a/tests/test_scribe_priority.py +++ b/tests/test_scribe_priority.py @@ -9,9 +9,14 @@ from __future__ import annotations +from queue import PriorityQueue +from unittest.mock import patch + +from codex.librarian.scribe.importer.tasks import ImportTask from codex.librarian.scribe.janitor.janitor import _JANITOR_METHOD_MAP, _NIGHTLY_TASKS from codex.librarian.scribe.janitor.tasks import JanitorFolderRelationsCheckTask from codex.librarian.scribe.priority import _SCRIBE_TASK_PRIORITY, get_task_priority +from codex.librarian.scribe.scribed import ScribeThread from codex.librarian.scribe.tasks import ScribeTask @@ -41,6 +46,38 @@ def test_nightly_scribe_tasks_are_priority_rankable() -> None: def test_get_task_priority_folder_relations_check() -> None: """The reported crash: ranking JanitorFolderRelationsCheckTask must not raise.""" - priority, now = get_task_priority(JanitorFolderRelationsCheckTask()) + priority, now, tie_breaker = get_task_priority(JanitorFolderRelationsCheckTask()) assert isinstance(priority, int) assert isinstance(now, float) + assert isinstance(tie_breaker, int) + + +def test_equal_priorities_never_compare_tasks() -> None: + """ + Two same-class tasks must be orderable even with an identical timestamp. + + Without a tie-breaker the heap falls through to comparing the tasks + themselves, and ScribeTask dataclasses define no ordering — the push + raises TypeError in the routing thread and the task is lost. + """ + frozen = 1724500000.0 + with patch("codex.librarian.scribe.priority.datetime") as mock_datetime: + mock_datetime.now.return_value.timestamp.return_value = frozen + first = get_task_priority(ImportTask(library_id=1)) + second = get_task_priority(ImportTask(library_id=1)) + + assert first[:2] == second[:2], "timestamps should be identical for this test" + queue = PriorityQueue() + queue.put((first, ImportTask(library_id=1))) + queue.put((second, ImportTask(library_id=1))) + assert queue.get()[0] == first + assert queue.get()[0] == second + + +def test_shutdown_msg_orders_against_a_pending_task() -> None: + """Stopping the thread with work queued must not raise TypeError.""" + queue = PriorityQueue() + queue.put((get_task_priority(ImportTask(library_id=1)), ImportTask(library_id=1))) + queue.put(ScribeThread.SHUTDOWN_MSG) + # Shutdown outranks queued work, and its shape still round-trips equality. + assert queue.get() == ScribeThread.SHUTDOWN_MSG From 78cc91b263fd41b7586c33d4b86f29db196afa31 Mon Sep 17 00:00:00 2001 From: AJ Slater Date: Tue, 25 Aug 2026 16:16:50 -0700 Subject: [PATCH 4/7] fix(tagging): dedupe a merged online tag scan by path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ``_merge_task`` tested each candidate path against ``path_to_pk``'s *values*, which are pks — a Path never equals an int, so the guard never excluded anything. Starting a second scan whose selection overlapped the running one (re-picking a folder to catch additions) queued every shared comic again: an inflated total, duplicate lookups against rate-limited sources, and a second write of the same file. Co-Authored-By: Claude Fable 5 --- codex/librarian/onlinetag/session_manager.py | 4 +- tests/test_onlinetag_merge_task.py | 82 ++++++++++++++++++++ 2 files changed, 85 insertions(+), 1 deletion(-) create mode 100644 tests/test_onlinetag_merge_task.py diff --git a/codex/librarian/onlinetag/session_manager.py b/codex/librarian/onlinetag/session_manager.py index ecaf2d8b7..838b303b8 100644 --- a/codex/librarian/onlinetag/session_manager.py +++ b/codex/librarian/onlinetag/session_manager.py @@ -354,7 +354,9 @@ def _merge_task(self, state: SessionState, task: Any) -> None: new_paths = {} for comic in comics: path = Path(comic.path) - if path not in state.path_to_pk.values(): + # ``path_to_pk`` is keyed by path; its *values* are pks, which a + # Path never equals, so testing them admitted every comic twice. + if path not in state.path_to_pk: new_paths[path] = comic.pk if not new_paths: return diff --git a/tests/test_onlinetag_merge_task.py b/tests/test_onlinetag_merge_task.py new file mode 100644 index 000000000..aead55a92 --- /dev/null +++ b/tests/test_onlinetag_merge_task.py @@ -0,0 +1,82 @@ +""" +Merging a second scan into a running online-tag session. + +A second ``BulkOnlineTagTask`` enqueued mid-scan is merged into the live +session. Comics the session already holds must be recognized, or a scan +whose selection overlaps (re-picking a folder to catch additions is the +natural gesture) queues them twice: inflated totals and duplicate lookups +against rate-limited sources. +""" + +from __future__ import annotations + +from pathlib import Path +from typing import Final + +from codex.librarian.onlinetag.session_state import SessionState +from codex.librarian.onlinetag.tasks import BulkOnlineTagTask +from codex.models import Comic +from tests.onlinetag_session_fakes import ( + FakeSession, + OnlineTagSessionTestCase, + double, + make_comic, +) + +#: The comic the session already holds, plus the one merged into it. +_HELD_PLUS_NEW: Final = 2 + + +class MergeTaskTests(OnlineTagSessionTestCase): + """_merge_task adds only comics the session isn't already tagging.""" + + def _state_holding(self, comic) -> SessionState: + """Build a session state that already carries ``comic``.""" + return SessionState( + session=double(FakeSession()), + path_to_pk={Path(comic.path): comic.pk}, + total_comics=1, + ) + + def test_already_queued_comic_is_not_merged_again(self) -> None: + """An overlapping selection must not re-queue a comic.""" + comic = make_comic() + state = self._state_holding(comic) + task = BulkOnlineTagTask(comic_pks=frozenset({comic.pk}), session_id="s") + + self.manager._merge_task(state, task) # noqa: SLF001 + + assert state.total_comics == 1 + assert state.pending_paths == [] + assert state.path_to_pk == {Path(comic.path): comic.pk} + + @staticmethod + def _sibling_of(comic: Comic) -> Comic: + """Create a second comic in the same library as ``comic``.""" + path = Path(comic.path).parent / "d.cbz" + path.touch() + return Comic.objects.create( + library=comic.library, + path=path, + issue_number=2, + name="d", + publisher=comic.publisher, + imprint=comic.imprint, + series=comic.series, + volume=comic.volume, + size=1, + file_type="CBZ", + ) + + def test_new_comic_is_merged(self) -> None: + """A comic the session doesn't hold is queued and counted.""" + held = make_comic() + state = self._state_holding(held) + new_comic = self._sibling_of(held) + task = BulkOnlineTagTask(comic_pks=frozenset({new_comic.pk}), session_id="s") + + self.manager._merge_task(state, task) # noqa: SLF001 + + assert state.total_comics == _HELD_PLUS_NEW + assert state.pending_paths == [Path(new_comic.path)] + assert state.path_to_pk[Path(new_comic.path)] == new_comic.pk From e952ff57685cb9d809d0da7ac9411e69472112f1 Mon Sep 17 00:00:00 2001 From: AJ Slater Date: Tue, 25 Aug 2026 16:16:50 -0700 Subject: [PATCH 5/7] fix(importer): say what the filesystem settle timeout actually does MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The warning told the admin to poll again once copying finished, implying the task had been abandoned, but only ``init_apply`` returned early — the import ran on regardless, and skipped starting its statuses on the way out. Keep importing (abandoning the task would drop the events entirely on a watched library that isn't also polled) and describe that instead. Co-Authored-By: Claude Fable 5 --- codex/librarian/scribe/importer/init.py | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/codex/librarian/scribe/importer/init.py b/codex/librarian/scribe/importer/init.py index 44495d70d..1133708ad 100644 --- a/codex/librarian/scribe/importer/init.py +++ b/codex/librarian/scribe/importer/init.py @@ -425,14 +425,16 @@ def init_apply(self) -> None: self.start_time = now() self._defer_pending_tag_write_moves() self.library.start_update() - too_long = self._wait_for_filesystem_ops_to_finish() - if too_long: + if self._wait_for_filesystem_ops_to_finish(): + # The import runs anyway: abandoning the task would drop these + # events entirely on a watched library that isn't also polled. + # Files still mid-copy fail to import and are retried by a later + # scan, so say that rather than implying nothing was imported. reason = ( "Import apply waited for the filesystem to stop changing too long. " - "Try polling again once files have finished copying" - f" in library: {self.library.path}" + "Importing anyway; files still copying may fail and be retried" + f" on a later scan in library: {self.library.path}" ) self.log.warning(reason) - return self._log_task() self._init_librarian_status(self.library.path) From 19117f4c7c3b502431fe42c4a6264c494c5e77f7 Mon Sep 17 00:00:00 2001 From: AJ Slater Date: Tue, 25 Aug 2026 16:16:50 -0700 Subject: [PATCH 6/7] docs(news): rename, watcher prefix and queue fixes in v2.2.11 Co-Authored-By: Claude Fable 5 --- NEWS.md | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/NEWS.md b/NEWS.md index 4825d746a..78c8731e2 100644 --- a/NEWS.md +++ b/NEWS.md @@ -24,6 +24,17 @@ border-radius: 128px; - Comics keep their bookmarks and read progress through renames, conversions and long tag write batches. PDFs lost them every time, other formats occasionally. + - Deleting or renaming a watched folder no longer deletes comics from + sibling folders whose names begin the same way, like "Batman" and "Batman + Beyond". + - Renaming keeps each archive's own file extension. PDFs and unconverted + CBRs were renamed to .cbz names. + - Libraries with paths like /comics and /comics-kids no longer claim each + other's file changes. + - A second online tagging scan no longer re-queues comics the running scan + already has. + - The librarian shuts down cleanly with work queued, and tasks queued in the + same instant no longer collide and lose one. ## v2.2.10 From 1842caf624c35c10609a72f38c3c33eb5a4f50bd Mon Sep 17 00:00:00 2001 From: AJ Slater Date: Tue, 25 Aug 2026 16:46:27 -0700 Subject: [PATCH 7/7] fix(tagging): let comicbox rename, with the extension stated MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Supersedes the previous commit's approach. Taking the rename away from comicbox fixed the name but duplicated comicbox's job, broke the invariant that codex's collision pre-check targets the exact path ``rename_file`` will use, and would have been dead weight the moment comicbox renders the extension itself. The real defect is the input, not the renamer: ``ext`` is a metadata field, and the read config deletes it, so comicfn2dict fell back to its "cbz" default. Neither half of the fix works alone — un-deleting the key leaves it unset, and stating it under the read config gets it deleted after the merge — so renaming uses a config that keeps ``ext`` and states the archive's real suffix as metadata. That outranks any extension a third-party tagger embedded in the archive too. The admin preview derives its name the same way, so it can no longer promise a name the rename won't produce. Covered against a real archive (a CBT repacked from the example CBZ), since whether the rendered extension is right now depends on what codex hands comicbox — something the test double cannot exercise. Co-Authored-By: Claude Fable 5 --- codex/librarian/scribe/tag_writer.py | 60 ++++++++------ codex/settings/__init__.py | 19 +++++ codex/views/admin/tagwrite.py | 31 ++++---- tests/test_tag_writer_rename.py | 114 ++++++++++++++++++++------- 4 files changed, 157 insertions(+), 67 deletions(-) diff --git a/codex/librarian/scribe/tag_writer.py b/codex/librarian/scribe/tag_writer.py index 7dfe25332..1605a2d6d 100644 --- a/codex/librarian/scribe/tag_writer.py +++ b/codex/librarian/scribe/tag_writer.py @@ -26,7 +26,7 @@ from codex.librarian.scribe.tagwrite_moves import register_tag_write_move from codex.librarian.worker import WorkerStatusAbortableBase from codex.models.comic import Comic -from codex.settings import COMICBOX_CONFIG +from codex.settings import COMICBOX_RENAME_CONFIG if TYPE_CHECKING: from comicbox.events import Event @@ -215,33 +215,41 @@ def _rename_one(self, old_path: Path) -> Path | None: Returns the new path, or None when the name is unchanged or no name could be built. Raises ``FileExistsError`` on a collision with a - *different* file so the caller reports it without clobbering anything. - - Codex performs the rename itself rather than calling comicbox's - ``rename_file``, which derives its own destination and does a bare - ``Path.rename`` — there is no way to hand it a corrected target, and - the extension it renders is not the archive's own (see below). + *different* file so the caller reports it without clobbering anything + (comicbox's ``rename_file`` does a bare ``Path.rename``). + + The rendered name ends in ``ext``, which is a *metadata* field rather + than the file's suffix, so it is stated here from the archive on disk + — the authority. Left to the merge it would be missing (the read + config deletes it) and comicfn2dict would fall back to its "cbz" + default, renaming every PDF or unconverted CBR to a name claiming to + be a zip; or it would be whatever a third-party tagger embedded in + the archive. Stating it as metadata outranks both. """ - with Comicbox(old_path, config=COMICBOX_CONFIG) as car: + ext = old_path.suffix.lstrip(".") + with Comicbox( + old_path, + config=COMICBOX_RENAME_CONFIG, + metadata={"comicbox": {"ext": ext}} if ext else None, + ) as car: + # to_string(FILENAME) is exactly what rename_file() derives the + # name from (schema.dumps(_to_dict(FILENAME))), so this pre-check + # targets the precise destination rename_file() will use. target = car.to_string(MetadataFormats.FILENAME) - # A rendered name always ends in an extension, but not necessarily - # this file's: ``ext`` is a *metadata* field, and codex's read config - # deletes it, so comicfn2dict falls back to its "cbz" default and - # every PDF/CBR/CBT/CB7 would be renamed to a ".cbz" name it isn't. - # The archive on disk is the authority, so keep its real suffix. - # A name that is nothing but an extension (no metadata parsed at all) - # would make a hidden file, so treat it as no name at all. - if not target or target.startswith("."): - self.log.warning(f"Rename skipped; no filename built for {old_path}") - return None - new_path = (old_path.parent / target).with_suffix(old_path.suffix) - if new_path == old_path: - return None - if new_path.exists() and not new_path.samefile(old_path): - reason = f"rename target already exists: {new_path}" - raise FileExistsError(reason) - old_path.rename(new_path) - return new_path + # A name that is nothing but an extension (nothing parsed at all) + # would make a hidden file, so treat it as no name. + if not target or target.startswith("."): + self.log.warning(f"Rename skipped; no filename built for {old_path}") + return None + new_path = old_path.parent / target + if new_path == old_path: + return None + if new_path.exists() and not new_path.samefile(old_path): + reason = f"rename target already exists: {new_path}" + raise FileExistsError(reason) + car.rename_file() + renamed = car.get_path() + return renamed or new_path def _rename_comics( self, diff --git a/codex/settings/__init__.py b/codex/settings/__init__.py index 3b81a068e..c095c41f6 100644 --- a/codex/settings/__init__.py +++ b/codex/settings/__init__.py @@ -1188,3 +1188,22 @@ def _get_middleware(features: FeatureFlags) -> tuple[str, ...]: } } ) + +# Renaming to the comicbox filename scheme needs one field the read config +# deletes: ``ext``, which the rendered name ends in. Deleted, comicfn2dict +# falls back to its "cbz" default and every PDF or unconverted CBR is +# renamed to a name claiming to be a zip. Callers must still *supply* the +# extension (the archive's real suffix) as metadata — un-deleting the key +# alone leaves it unset — but the delete runs after the merge, so it would +# strip a supplied value too. Read paths keep ``COMICBOX_CONFIG``: this +# only widens what the rename pass parses. +COMICBOX_RENAME_CONFIG: ComicboxSettings = get_config( + { + "comicbox": { + "general": { + "loglevel": LOGLEVEL, + "delete_keys": tuple(sorted(_COMICBOX_DELETE_KEYS - {"ext"})), + } + } + } +) diff --git a/codex/views/admin/tagwrite.py b/codex/views/admin/tagwrite.py index 132467b62..1c585eb3f 100644 --- a/codex/views/admin/tagwrite.py +++ b/codex/views/admin/tagwrite.py @@ -18,7 +18,7 @@ from codex.models.admin import ComicboxTaggingDefaults from codex.models.comic import Comic from codex.serializers.admin.tagging import TagWriteRequestSerializer -from codex.settings import COMICBOX_CONFIG +from codex.settings import COMICBOX_RENAME_CONFIG from codex.views.admin.auth import AdminAPIView from codex.views.admin.identifier_parse import parse_identifier_url from codex.views.browser.filters.filter import BrowserFilterView @@ -104,26 +104,30 @@ class AdminTagWritePreflightView(FilteredComicPksView): """Check how many comics need conversion before writing.""" @staticmethod - def _preview_one(old_path: Path, metadata: dict | None, config) -> str: + def _preview_one(old_path: Path, patch: dict | None, config) -> str: """ Return the comicbox-scheme name the given patch produces for one comic. Overlays the pending (unsaved) patch onto the archive's metadata in - memory and serializes the FILENAME format — the same construction the - rename pass uses — so the dialog can show the would-be name. Opens + memory and serializes the FILENAME format — the same construction + ``rename_file`` uses — so the dialog can show the would-be name. Opens the archive (I/O). Returns "" when no name could be built. + + States ``ext`` from the file's real suffix exactly as + ``TagWriter._rename_one`` does, so the preview cannot promise a name + the rename won't produce. """ + metadata = dict(patch) if patch else {} + if ext := old_path.suffix.lstrip("."): + metadata["ext"] = ext try: - with Comicbox(old_path, config=config, metadata=metadata) as car: + with Comicbox( + old_path, config=config, metadata={"comicbox": metadata} + ) as car: target = car.to_string(MetadataFormats.FILENAME) or "" except Exception: return "" - # Mirrors ``TagWriter._rename_one``: the rendered extension is - # comicfn2dict's "cbz" default rather than this archive's, so the - # preview must show the real suffix the rename will keep. - if not target or target.startswith("."): - return "" - return Path(target).with_suffix(old_path.suffix).name + return "" if target.startswith(".") else target def _filename_previews( self, @@ -133,10 +137,9 @@ def _filename_previews( ) -> list[dict[str, str]]: """Preview the rename (old → new) for each selected comic, capped.""" patch = json.loads(patch_str or "null") - metadata = {"comicbox": patch} if patch else None # Pending cleared fields must vanish from the previewed name exactly # as the real write (BulkWriteItem.delete_keys) will clear them. - config = COMICBOX_CONFIG + config = COMICBOX_RENAME_CONFIG if delete_keys: config = replace( config, @@ -156,7 +159,7 @@ def _filename_previews( previews.append( { "old": old_path.name, - "new": self._preview_one(old_path, metadata, config), + "new": self._preview_one(old_path, patch, config), } ) return previews diff --git a/tests/test_tag_writer_rename.py b/tests/test_tag_writer_rename.py index 8282fe927..2fdafdc19 100644 --- a/tests/test_tag_writer_rename.py +++ b/tests/test_tag_writer_rename.py @@ -12,6 +12,9 @@ from __future__ import annotations import shutil +import tarfile +import zipfile +from io import BytesIO from pathlib import Path from typing import Any, Final, Self, override from unittest.mock import patch @@ -37,10 +40,13 @@ Series, Volume, ) +from codex.settings import COMICBOX_RENAME_CONFIG +from codex.views.admin.tagwrite import AdminTagWritePreflightView _TMP_DIR: Final = Path("/tmp/codex.tests.tagrename") # noqa: S108 _COMICBOX_TARGET: Final = "codex.librarian.scribe.tag_writer.Comicbox" _TARGET_NAME: Final = "Renamed #001.cbz" +_EXAMPLE_CBZ: Final = Path(__file__).parent / "files" / "comicbox-2-example.cbz" def _double(stub: object) -> Any: @@ -62,9 +68,11 @@ class _FakeComicbox: """ Stand-in for ``comicbox.box.Comicbox`` used by the rename pass. - ``to_string(FILENAME)`` returns a fixed scheme name; the move itself is - codex's, so the real collision check, ``samefile``, and DB sync all run - against the filesystem. + ``to_string(FILENAME)`` returns a fixed scheme name and ``rename_file`` + actually moves the file on disk (mirroring comicbox) so the real + collision check, ``samefile``, and DB sync all run against the filesystem. + Whether the *rendered* name carries the right extension is comicbox's + job, covered against a real archive by ``TagWriterRenameExtensionTests``. """ target: str = _TARGET_NAME @@ -81,6 +89,14 @@ def __exit__(self, *_exc: object) -> bool: def to_string(self, _fmt) -> str: return self.target + def rename_file(self) -> None: + new_path = self._path.parent / self.target + self._path.rename(new_path) + self._path = new_path + + def get_path(self) -> Path: + return self._path + def _make_comic(*, events: bool, name: str = "c.cbz", read_only: bool = False) -> Comic: _TMP_DIR.mkdir(exist_ok=True, parents=True) @@ -276,30 +292,6 @@ def test_collision_skips_and_reports(self) -> None: assert errors assert errors[0]["path"] == str(old_path) - def test_rename_keeps_the_archives_own_extension(self) -> None: - """ - A non-CBZ archive keeps its real suffix. - - ``ext`` is a metadata field that codex's read config deletes, so the - rendered scheme name always ends in comicfn2dict's "cbz" default. The - file on disk is the authority: a PDF must not be renamed to a name - claiming it is a zip. - """ - comic = _make_comic(events=False, name="c.pdf") - old_path = Path(comic.path) - queue = _FakeQueue() - writer = _make_writer(queue) - task = BulkTagWriteTask(comic_pks=frozenset({comic.pk}), rename=True) - - with patch(_COMICBOX_TARGET, _FakeComicbox): - writer.write_tags(task) - - new_path = old_path.parent / f"{Path(_TARGET_NAME).stem}.pdf" - assert new_path.exists() - assert not (old_path.parent / _TARGET_NAME).exists() - imports = [i for i in queue.items if isinstance(i, ImportTask)] - assert imports[0].files_moved == {str(old_path): str(new_path)} - def test_rename_skipped_when_only_an_extension_is_rendered(self) -> None: """A name with no stem would make a hidden file, so skip the rename.""" comic = _make_comic(events=False) @@ -508,3 +500,71 @@ def test_converted_write_keeping_original_creates_not_moves(self) -> None: assert imports[0].files_modified == frozenset() # Nothing moved, so nothing needs holding back from a scan. assert not get_pending_tag_write_paths() + + +class TagWriterRenameExtensionTests(TestCase): + """ + A rendered name carries the archive's own extension. + + ``ext`` is a metadata field, not the file's suffix, so the name comicbox + renders is only right when codex hands it the real one. That depends on + the config and metadata codex passes, which a stand-in cannot exercise — + these run real comicbox against a real archive. + """ + + @override + def setUp(self) -> None: + _TMP_DIR.mkdir(exist_ok=True, parents=True) + + @override + def tearDown(self) -> None: + shutil.rmtree(_TMP_DIR, ignore_errors=True) + + @staticmethod + def _make_cbt(path: Path) -> None: + """Repack the example CBZ as a tarball: a real un-writable archive.""" + with zipfile.ZipFile(_EXAMPLE_CBZ) as zf, tarfile.open(path, "w") as tf: + for name in zf.namelist(): + data = zf.read(name) + info = tarfile.TarInfo(name) + info.size = len(data) + tf.addfile(info, BytesIO(data)) + + def test_non_cbz_archive_keeps_its_extension(self) -> None: + """A CBT must not be renamed to a name claiming to be a zip.""" + old_path = _TMP_DIR / "Rename Me v1999 #001 (1999).cbt" + self._make_cbt(old_path) + writer = _make_writer(_FakeQueue()) + + new_path = writer._rename_one(old_path) # noqa: SLF001 + + assert new_path is not None + assert new_path.suffix == ".cbt" + assert new_path.exists() + assert not old_path.exists() + + def test_cbz_archive_keeps_its_extension(self) -> None: + """The common case still renders .cbz.""" + old_path = _TMP_DIR / "Rename Me v1999 #002 (1999).cbz" + shutil.copy(_EXAMPLE_CBZ, old_path) + writer = _make_writer(_FakeQueue()) + + new_path = writer._rename_one(old_path) # noqa: SLF001 + + assert new_path is not None + assert new_path.suffix == ".cbz" + assert new_path.exists() + + def test_preview_matches_what_the_rename_produces(self) -> None: + """The admin preview must not promise a name the rename won't make.""" + old_path = _TMP_DIR / "Rename Me v1999 #003 (1999).cbt" + self._make_cbt(old_path) + writer = _make_writer(_FakeQueue()) + + preview = AdminTagWritePreflightView._preview_one( # noqa: SLF001 + old_path, None, COMICBOX_RENAME_CONFIG + ) + new_path = writer._rename_one(old_path) # noqa: SLF001 + + assert new_path is not None + assert preview == new_path.name