From f01049fffa3e1c120cc68ce04ef318452979036e Mon Sep 17 00:00:00 2001 From: AJ Slater Date: Mon, 24 Aug 2026 02:44:41 -0700 Subject: [PATCH 1/3] update deps --- bun.lock | 2 +- pyproject.toml | 2 +- uv.lock | 8 ++++---- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/bun.lock b/bun.lock index 5526d00f7..deb53527b 100644 --- a/bun.lock +++ b/bun.lock @@ -558,7 +558,7 @@ "js-types": ["js-types@4.0.0", "", {}, "sha512-/c+n06zvqFQGxdz1BbElF7S3nEghjNchLN1TjQnk2j10HYDaUc57rcvl6BbnziTx8NQmrg0JOs/iwRpvcYaxjQ=="], - "jsdoc-type-pratt-parser": ["jsdoc-type-pratt-parser@9.1.2", "", { "dependencies": { "@types/estree": "^1.0.9" } }, "sha512-9EXymowgk1mb9RY1VxuwKc+AhaxfBk2CV0dWxgGM+l5RURTtiUoAx7MlKwcsiVcEXK5HEPa7FeH/tsRpqjEPRg=="], + "jsdoc-type-pratt-parser": ["jsdoc-type-pratt-parser@9.2.0", "", { "dependencies": { "@types/estree": "^1.0.9" } }, "sha512-9TsacmMHRpB1pcKGuXZCJxV6MzYD6h0filpYuYnoO1WzUVin5dg8TFrwbfZSR3R6wlNN+Q81iwcIR2gzpCWmdg=="], "jsesc": ["jsesc@3.1.0", "", { "bin": { "jsesc": "bin/jsesc" } }, "sha512-/sM3dO2FOzXjKQhJuo0Q173wf2KOo8t4I8vHy6lF9poUp7bKT0/NHE8fPX23PwfhnykfqnC2xRxOnVw5XuGIaA=="], diff --git a/pyproject.toml b/pyproject.toml index 0db655847..14ed0128c 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -15,7 +15,7 @@ dependencies = [ "adrf~=0.1.12", "bidict~=0.23", "channels~=4.2", - "comicbox[pdf]~=4.8.4", + "comicbox[pdf]~=4.8.5", "cryptography>=48.0.0", "dateparser~=1.2", "django-allauth[socialaccount]~=65.13", diff --git a/uv.lock b/uv.lock index c80e80a91..cf03c475f 100644 --- a/uv.lock +++ b/uv.lock @@ -723,7 +723,7 @@ requires-dist = [ { name = "adrf", specifier = "~=0.1.12" }, { name = "bidict", specifier = "~=0.23" }, { name = "channels", specifier = "~=4.2" }, - { name = "comicbox", extras = ["pdf"], specifier = "~=4.8.4" }, + { name = "comicbox", extras = ["pdf"], specifier = "~=4.8.5" }, { name = "cryptography", specifier = ">=48.0.0" }, { name = "dateparser", specifier = "~=1.2" }, { name = "django", specifier = "~=6.0" }, @@ -809,7 +809,7 @@ wheels = [ [[package]] name = "comicbox" -version = "4.8.4" +version = "4.8.5" source = { registry = "https://pypi.org/simple" } dependencies = [ { name = "bidict" }, @@ -846,9 +846,9 @@ dependencies = [ { name = "xmltodict" }, { name = "zipremove" }, ] -sdist = { url = "https://files.pythonhosted.org/packages/14/29/11c3a0c600f8df52c4946fc36fa2b9848c5a934396cda19469f7287869cb/comicbox-4.8.4.tar.gz", hash = "sha256:e49cc36048603f7a7f5d01434a7c532e85aa54ad6f10fea800df86082da7794a", size = 1039981, upload-time = "2026-08-23T22:28:53.416Z" } +sdist = { url = "https://files.pythonhosted.org/packages/52/ad/c25e8a3d798dcaa20589912c9099c2456ec8a78f7808d5d616bde3fc1875/comicbox-4.8.5.tar.gz", hash = "sha256:d133951754a0d4f54a4b766b82768148f97e8de369e20da8892151017aa2f044", size = 1040273, upload-time = "2026-08-24T09:41:38.777Z" } wheels = [ - { url = "https://files.pythonhosted.org/packages/63/dd/eafd083aaa06de89c42a698030c57cd6ca0d554135b004a184a028da784c/comicbox-4.8.4-py3-none-any.whl", hash = "sha256:a15aba7b6908582cb13789c70e99c966ac075c5bd286138115dc0af405530da7", size = 346505, upload-time = "2026-08-23T22:28:51.914Z" }, + { url = "https://files.pythonhosted.org/packages/66/a1/5b8c70ad25d2bf47a35e8be42c4a959a409219794dce2f8ce3243d902385/comicbox-4.8.5-py3-none-any.whl", hash = "sha256:0a0baf8b0f0a5e0695aeaf31142ef6fb8ef9b9721c601f1d9fb180d4b976b51f", size = 346812, upload-time = "2026-08-24T09:41:37.199Z" }, ] [package.optional-dependencies] From 7c15ff22133958167c6f453b9f23a72f8e1d9950 Mon Sep 17 00:00:00 2001 From: AJ Slater Date: Mon, 24 Aug 2026 09:00:58 -0700 Subject: [PATCH 2/3] fix(tagging): follow CBR->CBZ conversion through rename and DB sync MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Writing tags to a CBR converts it to CBZ (comicbox repacks unwritable archives) and, with delete_original, removes the .cbr. The rename pass then opened the dead .cbr path and failed with 'does not exist', and the comic's row stayed pointed at the removed file. The converted CBZ is a new inode, so neither the watcher's nor the poller's inode move detection could ever pair old to new: the row was deleted and recreated as a fresh comic, losing bookmarks. Consume comicbox 4.8.5's WriteResult.final_path so the writer knows where each archive ended up. The rename pass now chases the written file to its post-conversion path, and a new conversion-aware DB sync replaces the split rename/unwatched-reimport enqueues: a delete_original conversion is recorded as a targeted move — for watched libraries too, since the watcher cannot pair it; its later add/delete events reconcile against the already-moved row. Conversions that keep the original leave the row alone and report the new CBZ as a created file. Pure renames and in-place writes keep their existing watcher-aware behavior. Co-Authored-By: Claude Fable 5 --- NEWS.md | 9 ++ codex/librarian/scribe/tag_writer.py | 209 ++++++++++++++++----------- tests/test_tag_writer_rename.py | 145 ++++++++++++++++++- 3 files changed, 279 insertions(+), 84 deletions(-) diff --git a/NEWS.md b/NEWS.md index 055d55a44..8adf5f04d 100644 --- a/NEWS.md +++ b/NEWS.md @@ -6,6 +6,15 @@ width: 128px; border-radius: 128px; " /> +## v2.2.11 + +- Fixes + - Tagging a CBR (or other non-CBZ) archive converts it to CBZ; the rename + pass and the database now follow the file to its converted path. The + rename step no longer fails with "does not exist", and the comic's + database row — bookmarks included — moves onto the new CBZ instead of + being dropped and re-created as a new comic on the next scan. + ## v2.2.10 - Fixes diff --git a/codex/librarian/scribe/tag_writer.py b/codex/librarian/scribe/tag_writer.py index f3541ed85..14ab89477 100644 --- a/codex/librarian/scribe/tag_writer.py +++ b/codex/librarian/scribe/tag_writer.py @@ -92,27 +92,6 @@ def _build_items( ) return items - def _reimport_unwatched(self, comic_paths: dict[int, Path]) -> None: - """Re-import comics in libraries without filesystem event watching.""" - if not comic_paths: - return - comics = Comic.objects.filter(pk__in=comic_paths.keys()).only( - "pk", "path", "library_id", "library__events" - ) - library_path_map: defaultdict[int, set[str]] = defaultdict(set) - for comic in comics: - if not comic.library.events: - library_path_map[comic.library_id].add(comic.path) # pyright: ignore[reportAttributeAccessIssue] - - for library_id, paths in library_path_map.items(): - import_task = ImportTask( - library_id=library_id, - files_modified=frozenset(paths), - force_import_metadata=True, - check_metadata_mtime=False, - ) - self.librarian_queue.put(import_task) - @staticmethod def _build_base_config(task: BulkTagWriteTask): """ @@ -129,14 +108,21 @@ def _build_base_config(task: BulkTagWriteTask): cfg = get_config() return replace(cfg, general=replace(cfg.general, delete_orig=True)) - def _collect_written_pks( + def _collect_written_paths( self, items: list[BulkWriteItem], path_to_pk: dict[Path, int], base_config, - ) -> set[int]: - """Run bulk_write and return pks that were successfully written.""" - written_pks: set[int] = set() + ) -> dict[int, Path]: + """ + Run bulk_write; map successfully written pks to their on-disk paths. + + The mapped path is the *final* one comicbox reports: writing an + unwritable archive (CBR/CBT/CB7) repacks it as a CBZ at a new path, + and every later step — rename, DB sync — must chase the file there, + not the submitted path the DB still holds. + """ + written_paths: dict[int, Path] = {} had_errors = False for result in bulk_write( items, @@ -153,11 +139,11 @@ def _collect_written_pks( continue pk = path_to_pk.get(result.path) if pk is not None: - written_pks.add(pk) + written_paths[pk] = result.final_path or result.path if had_errors: # Surface the failures to admins (red badge + Tagging-tab panel). self.librarian_queue.put(TAG_WRITE_ERRORS_CHANGED_TASK) - return written_pks + return written_paths @staticmethod def _resolve_comics( @@ -186,7 +172,7 @@ def _resolve_comics( return comic_paths, lib_of, library_events def write_tags(self, task: BulkTagWriteTask) -> None: - """Execute bulk tag write, optional rename, and re-import.""" + """Execute bulk tag write, optional rename, and DB sync.""" if not task.comic_pks: self.log.debug("Tag write called with no comic pks.") return @@ -197,33 +183,29 @@ def write_tags(self, task: BulkTagWriteTask) -> None: self.log.debug("Tag write: no patches to apply.") return - written_pks: set[int] = set() + written_paths: dict[int, Path] = {} if items: path_to_pk = {path: pk for pk, path in comic_paths.items()} base_config = self._build_base_config(task) - written_pks = self._collect_written_pks(items, path_to_pk, base_config) + written_paths = self._collect_written_paths(items, path_to_pk, base_config) - renamed_pks: set[int] = set() + renamed_paths: dict[int, Path] = {} if task.rename: # Rename-only (no patch) renames every resolved comic from its - # existing on-archive metadata; with a patch, only the written ones. - candidates = written_pks if items else set(comic_paths) - renamed_pks = self._rename_comics( - candidates, - comic_paths, - lib_of, - library_events, - tags_written=bool(items), - ) + # existing on-archive metadata; with a patch, only the written + # ones. Renames chase the written file to its post-conversion + # path, not the possibly-stale DB path. + candidates = set(written_paths) if items else set(comic_paths) + current_paths = {**comic_paths, **written_paths} + renamed_paths = self._rename_comics(candidates, current_paths) - # Non-renamed written comics keep the existing unwatched re-import path; - # renamed comics are synced inside _rename_comics (their old path is gone). - non_renamed = { - pk: comic_paths[pk] for pk in written_pks if pk not in renamed_pks - } - self._reimport_unwatched(non_renamed) + self._sync_db( + task, comic_paths, written_paths, renamed_paths, lib_of, library_events + ) + num_written = len(written_paths) + num_renamed = len(renamed_paths) self.log.info( - f"Tag write complete: {len(written_pks)} written, {len(renamed_pks)} renamed." + f"Tag write complete: {num_written} written, {num_renamed} renamed." ) def _rename_one(self, old_path: Path) -> Path | None: @@ -256,19 +238,13 @@ def _rename_one(self, old_path: Path) -> Path | None: def _rename_comics( self, candidates: set[int], - comic_paths: dict[int, Path], - lib_of: dict[int, int], - library_events: dict[int, bool], - *, - tags_written: bool, - ) -> set[int]: - """Rename candidate comics and sync the DB. Return the renamed pks.""" - # library_id -> {old_path_str: new_path_str} - moved: defaultdict[int, dict[str, str]] = defaultdict(dict) - renamed_pks: set[int] = set() + current_paths: dict[int, Path], + ) -> dict[int, Path]: + """Rename candidate comics on disk. Return new paths by pk.""" + renamed_paths: dict[int, Path] = {} had_errors = False for pk in candidates: - old_path = comic_paths[pk] + old_path = current_paths[pk] try: new_path = self._rename_one(old_path) except Exception as exc: @@ -278,42 +254,109 @@ def _rename_comics( continue if new_path is None or new_path == old_path: continue - moved[lib_of[pk]][str(old_path)] = str(new_path) - renamed_pks.add(pk) + renamed_paths[pk] = new_path if had_errors: self.librarian_queue.put(TAG_WRITE_ERRORS_CHANGED_TASK) - self._enqueue_rename_imports(moved, library_events, tags_written=tags_written) - return renamed_pks + return renamed_paths + + @staticmethod + def _sync_ops_for_comic( + db_path: Path, + written_path: Path | None, + renamed_path: Path | None, + *, + watched: bool, + delete_original: bool, + ) -> tuple[str | None, str | None, str | None]: + """ + Classify one comic's on-disk outcome into DB sync operations. + + Returns (moved_dest, modified_path, created_path); each is None when + that operation isn't needed. See ``_sync_db`` for the rationale + behind each case. + """ + if written_path is None and renamed_path is None: + return None, None, None + converted = written_path is not None and written_path != db_path + end_path = str(renamed_path or written_path or db_path) + if converted and not delete_original: + # The DB comic is the untouched original; the CBZ is a new file. + return None, None, None if watched else end_path + if converted: + # New inode: nothing downstream can pair this move; record it + # for watched libraries too. + return end_path, end_path, None + if watched: + return None, None, None + if renamed_path is not None: + modify = end_path if written_path is not None else None + return end_path, modify, None + return None, end_path, None - def _enqueue_rename_imports( + def _sync_db( self, - moved: dict[int, dict[str, str]], + task: BulkTagWriteTask, + comic_paths: dict[int, Path], + written_paths: dict[int, Path], + renamed_paths: dict[int, Path], + lib_of: dict[int, int], library_events: dict[int, bool], - *, - tags_written: bool, ) -> None: """ - Sync the DB for renamed comics, watcher-aware. - - Watched libraries: enqueue nothing — the watcher's inode move-detection - emits the ``files_moved`` import, and ``build_import_task`` remaps the - tag-write's modify event from the pre-rename path onto the move - destination, so the new tags re-read from the same batch. A - self-enqueued move would only duplicate it. - Unwatched libraries: enqueue one targeted move import that updates the - path and, when tags were written, re-reads the new file's metadata - (``move_and_modify_dirs`` runs before the per-comic ``read`` phase). + Sync the DB to the on-disk outcome of the write + rename, watcher-aware. + + Three on-disk outcomes need a DB move or re-read: + + Conversion (CBR/CBT/CB7 repacked as CBZ during the write, original + deleted): the new archive is a *new inode*, which neither the watcher + nor the poller can pair into a move — left alone, the row would be + deleted and recreated, losing bookmarks. Codex must record the move + itself, for watched libraries too; the watcher's later add/delete + events reconcile as no-ops against the already-moved row. Best-effort: + a write batch long enough to force a mid-batch watcher flush can land + the watcher's delete first, degrading to the old delete+recreate — + never worse. When the + original is kept (``delete_original`` off), the DB comic is untouched + and the converted CBZ is simply a new file: watched libraries see its + create event, unwatched ones are told here. + + Pure rename (same path reported back): a watched library's watcher + pairs the move itself and ``build_import_task`` remaps the tag-write's + modify event onto the destination, so codex enqueues nothing — a + self-enqueued move would only duplicate it. (PDF writes replace the + file at the same path with a new inode, defeating that pairing — a + pre-existing gap this sync does not yet cover.) Unwatched libraries + get a targeted move (``move_and_modify_dirs`` runs before the + per-comic ``read`` phase). + + In-place write (no conversion, no rename): watched libraries re-read + via the watcher's modify event; unwatched ones are told here. """ - for library_id, lib_moved in moved.items(): - if library_events.get(library_id): - continue - files_modified = ( - frozenset(lib_moved.values()) if tags_written else frozenset() + moved: defaultdict[int, dict[str, str]] = defaultdict(dict) + modified: defaultdict[int, set[str]] = defaultdict(set) + created: defaultdict[int, set[str]] = defaultdict(set) + for pk, db_path in comic_paths.items(): + library_id = lib_of[pk] + move_to, modify, create = self._sync_ops_for_comic( + db_path, + written_paths.get(pk), + renamed_paths.get(pk), + watched=library_events.get(library_id, False), + delete_original=task.delete_original, ) + if move_to: + moved[library_id][str(db_path)] = move_to + if modify: + modified[library_id].add(modify) + if create: + created[library_id].add(create) + + for library_id in moved.keys() | modified.keys() | created.keys(): import_task = ImportTask( library_id=library_id, - files_moved=dict(lib_moved), - files_modified=files_modified, + files_moved=moved.get(library_id, {}), + files_modified=frozenset(modified.get(library_id, ())), + files_created=frozenset(created.get(library_id, ())), force_import_metadata=True, check_metadata_mtime=False, ) diff --git a/tests/test_tag_writer_rename.py b/tests/test_tag_writer_rename.py index a38bff244..ecd6a2909 100644 --- a/tests/test_tag_writer_rename.py +++ b/tests/test_tag_writer_rename.py @@ -234,7 +234,11 @@ def test_tag_write_and_rename_unwatched_rereads_metadata(self) -> None: with ( patch(_COMICBOX_TARGET, _FakeComicbox), - patch.object(TagWriter, "_collect_written_pks", return_value={comic.pk}), + patch.object( + TagWriter, + "_collect_written_paths", + return_value={comic.pk: old_path}, + ), ): writer.write_tags(task) @@ -244,3 +248,142 @@ def test_tag_write_and_rename_unwatched_rereads_metadata(self) -> None: assert imports[0].files_moved == {str(old_path): str(new_path)} # Tags were written, so the new path is re-read. assert imports[0].files_modified == frozenset({str(new_path)}) + + +class TagWriterConversionTests(TestCase): + """ + A tag write that converts the archive (CBR -> CBZ) syncs the DB. + + Comicbox repacks unwritable archives as CBZ during a write and reports + the new file as the result's ``final_path``. The converted archive is a + new inode, which neither the watcher nor the poller can pair into a + move, so codex must record the move itself — for watched libraries too + — and the rename pass must chase the file to its converted path. + """ + + @override + def setUp(self) -> None: + caches["default"].clear() + caches["tagging"].clear() + + @override + def tearDown(self) -> None: + shutil.rmtree(_TMP_DIR, ignore_errors=True) + + @staticmethod + def _convert(old_path: Path) -> Path: + """Simulate comicbox's CBR->CBZ conversion with delete_orig.""" + new_path = old_path.with_suffix(".cbz") + old_path.rename(new_path) + return new_path + + def _write_task(self, comic: Comic) -> BulkTagWriteTask: + return BulkTagWriteTask( + comic_pks=frozenset({comic.pk}), + patch={"series": {"name": "S"}}, + delete_original=True, + rename=True, + ) + + def test_converted_write_renames_the_cbz_and_enqueues_move(self) -> None: + """Rename follows the conversion; one move from the DB path lands.""" + comic = _make_comic(events=False, name="c.cbr") + old_path = Path(comic.path) + cbz_path = self._convert(old_path) + queue = _FakeQueue() + writer = _make_writer(queue) + task = self._write_task(comic) + + with ( + patch(_COMICBOX_TARGET, _FakeComicbox), + patch.object( + TagWriter, + "_collect_written_paths", + return_value={comic.pk: cbz_path}, + ), + ): + writer.write_tags(task) + + renamed_path = old_path.parent / _TARGET_NAME + assert renamed_path.exists() + assert not cbz_path.exists() + assert not old_path.exists() + imports = [i for i in queue.items if isinstance(i, ImportTask)] + assert len(imports) == 1 + # The move source is the DB's path (the dead .cbr), not the interim cbz. + assert imports[0].files_moved == {str(old_path): str(renamed_path)} + assert imports[0].files_modified == frozenset({str(renamed_path)}) + + def test_converted_write_without_rename_enqueues_move(self) -> None: + """Conversion alone moves the DB row onto the new cbz.""" + comic = _make_comic(events=False, name="c.cbr") + old_path = Path(comic.path) + cbz_path = self._convert(old_path) + queue = _FakeQueue() + writer = _make_writer(queue) + task = self._write_task(comic) + task.rename = False + + with patch.object( + TagWriter, + "_collect_written_paths", + return_value={comic.pk: cbz_path}, + ): + writer.write_tags(task) + + imports = [i for i in queue.items if isinstance(i, ImportTask)] + assert len(imports) == 1 + assert imports[0].files_moved == {str(old_path): str(cbz_path)} + assert imports[0].files_modified == frozenset({str(cbz_path)}) + + def test_converted_write_watched_still_enqueues_move(self) -> None: + """Watched libraries can't inode-pair a conversion; codex enqueues it.""" + comic = _make_comic(events=True, name="c.cbr") + old_path = Path(comic.path) + cbz_path = self._convert(old_path) + queue = _FakeQueue() + writer = _make_writer(queue) + task = self._write_task(comic) + + with ( + patch(_COMICBOX_TARGET, _FakeComicbox), + patch.object( + TagWriter, + "_collect_written_paths", + return_value={comic.pk: cbz_path}, + ), + ): + writer.write_tags(task) + + renamed_path = old_path.parent / _TARGET_NAME + assert renamed_path.exists() + imports = [i for i in queue.items if isinstance(i, ImportTask)] + assert len(imports) == 1 + assert imports[0].files_moved == {str(old_path): str(renamed_path)} + + def test_converted_write_keeping_original_creates_not_moves(self) -> None: + """Without delete_original the DB comic is untouched; cbz is new.""" + comic = _make_comic(events=False, name="c.cbr") + old_path = Path(comic.path) + # Original kept: the cbz appears alongside it. + cbz_path = old_path.with_suffix(".cbz") + shutil.copyfile(old_path, cbz_path) + queue = _FakeQueue() + writer = _make_writer(queue) + task = self._write_task(comic) + task.delete_original = False + task.rename = False + + with patch.object( + TagWriter, + "_collect_written_paths", + return_value={comic.pk: cbz_path}, + ): + writer.write_tags(task) + + assert old_path.exists() + imports = [i for i in queue.items if isinstance(i, ImportTask)] + assert len(imports) == 1 + assert not imports[0].files_moved + assert imports[0].files_created == frozenset({str(cbz_path)}) + assert imports[0].files_modified == frozenset() From 97f5ecc03ccaeb33308813f9e8781dd92bbb532c Mon Sep 17 00:00:00 2001 From: AJ Slater Date: Mon, 24 Aug 2026 11:05:23 -0700 Subject: [PATCH 3/3] fix(tagging): record watched-library renames instead of inferring them MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A rename in a watched library enqueued nothing, on the premise that the watcher's inode pairing would detect the move on its own. That premise does not hold for PDFs. pdffile's save() writes a temp file and replace()s it over the original, so an "in-place" PDF tag write leaves a new inode at the same path. The following rename then reaches the watcher as an unpairable delete+add — detect_moves compares the row's stale stored inode against the renamed file's fresh one — and the Comic row is deleted and recreated, losing bookmarks and read state. Even a same-inode archive goes unpaired when its delete and add land in different watcher batches. Enqueue the targeted move for watched libraries too. Codex performed the rename; it should state the move rather than leave the watcher to re-derive it from inodes. Scoping this to PDFs by file_type would freeze a snapshot of comicbox's per-format write strategy from another repo: its CBZ path patches the zip in place today, but is non-atomic, and an obvious future move to tmp+replace would silently reintroduce this bug. Duplicating a move the watcher does pair is safe: whichever copy lands second is dropped by _remove_file_move_collisions for an occupied destination, or matches no source row in _bulk_comics_move_prepare. As with a conversion, a watcher delete that lands first degrades to the old delete+recreate — never worse. Co-Authored-By: Claude Fable 5 --- NEWS.md | 2 + codex/librarian/scribe/tag_writer.py | 29 +++++++---- tests/test_tag_writer_rename.py | 78 +++++++++++++++++++++++++--- 3 files changed, 91 insertions(+), 18 deletions(-) diff --git a/NEWS.md b/NEWS.md index 8adf5f04d..66a197def 100644 --- a/NEWS.md +++ b/NEWS.md @@ -14,6 +14,8 @@ border-radius: 128px; rename step no longer fails with "does not exist", and the comic's database row — bookmarks included — moves onto the new CBZ instead of being dropped and re-created as a new comic on the next scan. + - Renaming comics in a watched library keeps their bookmarks and read + progress. PDFs lost them every time, other formats occasionally. ## v2.2.10 diff --git a/codex/librarian/scribe/tag_writer.py b/codex/librarian/scribe/tag_writer.py index 14ab89477..442161e80 100644 --- a/codex/librarian/scribe/tag_writer.py +++ b/codex/librarian/scribe/tag_writer.py @@ -286,11 +286,13 @@ def _sync_ops_for_comic( # New inode: nothing downstream can pair this move; record it # for watched libraries too. return end_path, end_path, None - if watched: - return None, None, None if renamed_path is not None: + # Codex performed this rename, so it states the move rather + # than leaving the watcher to re-infer it; watched too. modify = end_path if written_path is not None else None return end_path, modify, None + if watched: + return None, None, None return None, end_path, None def _sync_db( @@ -320,14 +322,21 @@ def _sync_db( and the converted CBZ is simply a new file: watched libraries see its create event, unwatched ones are told here. - Pure rename (same path reported back): a watched library's watcher - pairs the move itself and ``build_import_task`` remaps the tag-write's - modify event onto the destination, so codex enqueues nothing — a - self-enqueued move would only duplicate it. (PDF writes replace the - file at the same path with a new inode, defeating that pairing — a - pre-existing gap this sync does not yet cover.) Unwatched libraries - get a targeted move (``move_and_modify_dirs`` runs before the - per-comic ``read`` phase). + Pure rename (same path reported back): codex records the move + itself, for watched libraries too. A watcher can only recognize a + rename by pairing its delete and add on a matching inode, and that + pairing is not dependable. An in-place PDF tag write saves to a + temp file and ``replace()``s it over the original, so the file + carries a *new* inode that the row's stored one can never match; + even a same-inode archive goes unpaired when the delete and add + land in different watcher batches. An unpaired rename deletes the + row and recreates it, losing bookmarks and read state. Duplicating + a move the watcher does pair costs nothing: whichever copy lands + second is dropped by ``_remove_file_move_collisions`` for an + occupied destination, or matches no source row in + ``_bulk_comics_move_prepare``. The move is targeted, so + ``move_and_modify_dirs`` runs before the per-comic ``read`` phase, + and the same mid-batch-flush caveat as a conversion applies. In-place write (no conversion, no rename): watched libraries re-read via the watcher's modify event; unwatched ones are told here. diff --git a/tests/test_tag_writer_rename.py b/tests/test_tag_writer_rename.py index ecd6a2909..5760605d1 100644 --- a/tests/test_tag_writer_rename.py +++ b/tests/test_tag_writer_rename.py @@ -2,9 +2,10 @@ Tests for ``TagWriter`` comicbox-scheme file renaming. Covers the rename pass and its watcher-aware DB sync: rename-only (no tag -patch) and tag-write-plus-rename, the unwatched (codex enqueues a targeted -move ``ImportTask``) vs watched (the watcher owns it, codex enqueues nothing) -branch, and the skip-and-report collision guard. +patch) and tag-write-plus-rename, both enqueueing a targeted move +``ImportTask`` whether or not the library is watched, the in-place write a +watched library is left to notice for itself, and the skip-and-report +collision guard. """ from __future__ import annotations @@ -169,8 +170,8 @@ def test_read_only_library_is_never_renamed(self) -> None: assert not (old_path.parent / _TARGET_NAME).exists() assert not queue.items - def test_rename_only_watched_enqueues_nothing(self) -> None: - """A watched library's rename is left to the watcher; codex stays quiet.""" + def test_rename_only_watched_enqueues_move(self) -> None: + """A watched library's rename is recorded by codex, not left to the watcher.""" comic = _make_comic(events=True) old_path = Path(comic.path) queue = _FakeQueue() @@ -180,9 +181,70 @@ def test_rename_only_watched_enqueues_nothing(self) -> None: with patch(_COMICBOX_TARGET, _FakeComicbox): writer.write_tags(task) - # File still renamed on disk, but no ImportTask: the watcher's - # move-detection will pick it up. - assert (old_path.parent / _TARGET_NAME).exists() + new_path = old_path.parent / _TARGET_NAME + assert new_path.exists() + # The watcher's inode pairing is best-effort, so codex states the + # move it performed. A move the watcher also pairs is deduplicated + # downstream by the occupied-destination guard. + imports = [i for i in queue.items if isinstance(i, ImportTask)] + assert len(imports) == 1 + assert imports[0].files_moved == {str(old_path): str(new_path)} + # Rename-only: metadata unchanged, so no re-read is requested. + assert imports[0].files_modified == frozenset() + + def test_tag_write_and_rename_watched_enqueues_move_and_reread(self) -> None: + """ + A watched write + rename records the move and re-reads the new path. + + This is the PDF case: pdffile's save() writes a temp file and + ``replace()``s it over the original, so the renamed file carries a + new inode and the watcher can never pair it to the row's stored one. + """ + comic = _make_comic(events=True) + old_path = Path(comic.path) + queue = _FakeQueue() + writer = _make_writer(queue) + task = BulkTagWriteTask( + comic_pks=frozenset({comic.pk}), + patch={"series": {"name": "S"}}, + rename=True, + ) + + with ( + patch(_COMICBOX_TARGET, _FakeComicbox), + patch.object( + TagWriter, + "_collect_written_paths", + return_value={comic.pk: old_path}, + ), + ): + writer.write_tags(task) + + new_path = old_path.parent / _TARGET_NAME + imports = [i for i in queue.items if isinstance(i, ImportTask)] + assert len(imports) == 1 + assert imports[0].files_moved == {str(old_path): str(new_path)} + assert imports[0].files_modified == frozenset({str(new_path)}) + + def test_write_only_watched_enqueues_nothing(self) -> None: + """An in-place write with no rename is still left to the watcher.""" + comic = _make_comic(events=True) + old_path = Path(comic.path) + queue = _FakeQueue() + writer = _make_writer(queue) + task = BulkTagWriteTask( + comic_pks=frozenset({comic.pk}), patch={"series": {"name": "S"}} + ) + + with patch.object( + TagWriter, + "_collect_written_paths", + return_value={comic.pk: old_path}, + ): + writer.write_tags(task) + + # The path never changed, so there is no move to state; the + # watcher's modify event carries the re-read. assert not [i for i in queue.items if isinstance(i, ImportTask)] def test_collision_skips_and_reports(self) -> None: