diff --git a/NEWS.md b/NEWS.md index 1405af4f6..1090e6f25 100644 --- a/NEWS.md +++ b/NEWS.md @@ -18,6 +18,11 @@ border-radius: 128px; - Redesigned the Admin Tagging Status table to be more informative. - Fixes + - Tag writes rename each comic before writing it, and the database follows + the file immediately. Bookmarks and read progress now survive a rename or + a CBR conversion even when a library scan lands in the middle of one. + - Editing a comic's tags twice in a row no longer fails the second edit with + a "no such file" error when renaming is on. - 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. diff --git a/codex/librarian/scribe/importer/init.py b/codex/librarian/scribe/importer/init.py index 1133708ad..6cdf11065 100644 --- a/codex/librarian/scribe/importer/init.py +++ b/codex/librarian/scribe/importer/init.py @@ -50,10 +50,6 @@ from codex.librarian.scribe.importer.tasks import ImportTask from codex.librarian.scribe.search.status import SearchIndexCleanStatus from codex.librarian.scribe.status import UpdateCollectionTimestampsStatus -from codex.librarian.scribe.tagwrite_moves import ( - get_pending_tag_write_paths, - release_tag_write_moves, -) from codex.librarian.worker import WorkerStatusBase from codex.models import Library from codex.settings import LOGLEVEL @@ -162,41 +158,6 @@ def timed_step(self, name: str, method: Callable[[], Any]) -> Any: self.phase_times[name] = self.phase_times.get(name, 0.0) + elapsed return result - def _defer_pending_tag_write_moves(self) -> None: - """ - Leave paths a tag-write batch is still moving to that batch. - - A scan that lands during a long tag write reports the conversion - it is watching as an unrelated delete plus create, and its task - outranks the tag writer's end-of-batch move by enqueue time. - Dropping those paths here makes the scan a no-op for them, so the - move still finds its source row — and its bookmarks — in place. - - A task that carries a registered move reconciles it, so it both - releases that guard and is exempt from it — the tag writer's own - task keeps the re-read it asked for. The exemption is computed - from the task rather than from the release so a move that later - turns out to be unappliable can't cost the task its own paths. - Runs before the write wait and the status init so neither counts - a deferred path. - """ - release_tag_write_moves(self.task.files_moved) - own = frozenset(self.task.files_moved) | frozenset( - self.task.files_moved.values() - ) - pending = get_pending_tag_write_paths() - own - if not pending: - return - deferred = pending & ( - self.task.files_deleted | self.task.files_created | self.task.files_modified - ) - if not deferred: - return - self.task.files_deleted -= pending - self.task.files_created -= pending - self.task.files_modified -= pending - self.log.info(f"Deferred {len(deferred)} path(s) to an in-flight tag write.") - def _wait_for_filesystem_ops_to_finish(self) -> bool: """Watcher sends events before filesystem events finish, so wait for them.""" started_checking = time() @@ -423,7 +384,6 @@ def _init_librarian_status(self, path) -> None: def init_apply(self) -> None: """Initialize the library and status flags.""" self.start_time = now() - self._defer_pending_tag_write_moves() self.library.start_update() if self._wait_for_filesystem_ops_to_finish(): # The import runs anyway: abandoning the task would drop these diff --git a/codex/librarian/scribe/tag_writer.py b/codex/librarian/scribe/tag_writer.py index 1605a2d6d..58f577561 100644 --- a/codex/librarian/scribe/tag_writer.py +++ b/codex/librarian/scribe/tag_writer.py @@ -7,7 +7,6 @@ from pathlib import Path from typing import TYPE_CHECKING, cast -from comicbox.box import Comicbox from comicbox.config import get_config from comicbox.events import ( BatchFinished, @@ -16,17 +15,28 @@ FileParsed, FileShortCircuited, ) -from comicbox.formats import MetadataFormats from comicbox.write import BulkWriteItem, bulk_write +from django.core.cache import cache +from django.utils.timezone import now -from codex.librarian.notifier.tasks import TAG_WRITE_ERRORS_CHANGED_TASK +from codex.librarian.notifier.tasks import ( + LIBRARY_CHANGED_TASK, + TAG_WRITE_ERRORS_CHANGED_TASK, +) +from codex.librarian.scribe.importer.importer import ComicImporter from codex.librarian.scribe.importer.tasks import ImportTask from codex.librarian.scribe.status import TagWriteStatus from codex.librarian.scribe.tagwrite_errors import add_tag_write_error -from codex.librarian.scribe.tagwrite_moves import register_tag_write_move +from codex.librarian.scribe.tagwrite_rename import ( + RenamePlan, + build_predict_config, + plan_rename, + predict_name, + will_convert, +) +from codex.librarian.scribe.timestamp_update import TimestampUpdater from codex.librarian.worker import WorkerStatusAbortableBase from codex.models.comic import Comic -from codex.settings import COMICBOX_RENAME_CONFIG if TYPE_CHECKING: from comicbox.events import Event @@ -149,257 +159,367 @@ def _collect_written_paths( @staticmethod def _resolve_comics( task: BulkTagWriteTask, - ) -> tuple[dict[int, Path], dict[int, int], dict[int, bool]]: + ) -> tuple[dict[int, Path], dict[int, int]]: """ Resolve writable comics to path / library maps. Never returns comics in read-only libraries, even if a task somehow carries their pks (the API funnel already drops them; this is a - backstop). Returns (path-by-pk, library-id-by-pk, watcher-by-library). + backstop). Returns (path-by-pk, library-id-by-pk). + + Whether a library is watched no longer changes anything here: every + move is applied inline, and the re-read is requested either way + because a watched library's own re-read is stat-only unless the + import-metadata flag is on. """ comics = ( Comic.objects.filter(pk__in=task.comic_pks) .exclude(library__read_only=True) - .select_related("library") - .only("pk", "path", "library__events") + .only("pk", "path", "library") ) comic_paths: dict[int, Path] = {} lib_of: dict[int, int] = {} - library_events: dict[int, bool] = {} for comic in comics: comic_paths[comic.pk] = Path(comic.path) lib_of[comic.pk] = comic.library_id # pyright: ignore[reportAttributeAccessIssue] - library_events[comic.library_id] = comic.library.events # pyright: ignore[reportAttributeAccessIssue] - return comic_paths, lib_of, library_events + return comic_paths, lib_of def write_tags(self, task: BulkTagWriteTask) -> None: - """Execute bulk tag write, optional rename, and DB sync.""" + """ + Rename each archive to its final name, then write tags there. + + Renaming first, and applying each move to the database before this + method returns, is what keeps a comic's bookmarks through the churn. + Every path a write moves through used to be reconciled *later*, by + an ``ImportTask`` queued behind whatever else the scribe was doing — + so a scan landing in between saw an unexplained delete plus create, + deleted the row by its now-dead path, and cascaded the bookmarks + away. Doing the move here, on the scribe's own thread, means no scan + can be processed while the database disagrees with the disk: a + stale delete finds no row and a stale create converges onto the row + already sitting at that path. + + A conversion (CBR/CBT/CB7 repacked as a CBZ) still moves the file + after the write, so it is synced the same way as soon as the batch + finishes. Only the metadata re-read is left to a queued task, which + is safe because it names a path the database already holds. + """ if not task.comic_pks: self.log.debug("Tag write called with no comic pks.") return - comic_paths, lib_of, library_events = self._resolve_comics(task) - items = self._build_items(task, comic_paths) - if not items and not task.rename: + comic_paths, lib_of = self._resolve_comics(task) + if not self._build_items(task, comic_paths) and not task.rename: self.log.debug("Tag write: no patches to apply.") return - 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_paths = self._collect_written_paths(items, path_to_pk, base_config) - - 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. 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) - - self._sync_db( - task, comic_paths, written_paths, renamed_paths, lib_of, library_events + renamed_paths = self._rename_first(task, comic_paths, lib_of) + current_paths = {**comic_paths, **renamed_paths} + + written_paths = self._write(task, current_paths) + converted_paths = self._sync_conversions( + task, current_paths, written_paths, lib_of + ) + # A kept original leaves the row where it is and the new CBZ is + # simply a new file, so its scheme name is applied after the write. + post_renamed = self._rename_kept_conversions( + task, current_paths, written_paths, converted_paths ) - num_written = len(written_paths) - num_renamed = len(renamed_paths) - self.log.info( - f"Tag write complete: {num_written} written, {num_renamed} renamed." + + self._enqueue_rereads( + current_paths, written_paths, converted_paths, post_renamed, lib_of + ) + num_renamed = len(renamed_paths) + len(post_renamed) + reason = ( + f"Tag write complete: {len(written_paths)} written, {num_renamed} renamed." ) + self.log.info(reason) - def _rename_one(self, old_path: Path) -> Path | None: + def _write( + self, task: BulkTagWriteTask, current_paths: dict[int, Path] + ) -> dict[int, Path]: + """Write tags at each comic's post-rename path.""" + items = self._build_items(task, current_paths) + if not items: + return {} + path_to_pk = {path: pk for pk, path in current_paths.items()} + base_config = self._build_base_config(task) + return self._collect_written_paths(items, path_to_pk, base_config) + + def _plan_renames( + self, task: BulkTagWriteTask, comic_paths: dict[int, Path] + ) -> list[RenamePlan]: """ - Rename one archive to the comicbox (comicfn2dict) filename scheme. - - 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``). - - 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. + Build every comic's rename plan, dropping the ones that can't run. + + A plan is dropped when two comics in the batch predict the same + name, when either the rename target or the destination a later + conversion needs is already taken on disk or in the database, or + when no name could be built at all. Both ends matter: comicbox + refuses to convert onto an existing file, and that refusal would + land *after* the rename and its database move had already happened. """ - 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 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("."): + config = build_predict_config(task.delete_keys, task.mode) + plans: list[RenamePlan] = [] + claimed: dict[Path, int] = {} + for pk, old_path in comic_paths.items(): + if not task.delete_original and will_convert(old_path): + # Renamed after the write instead; the row stays put. + continue + try: + plan = plan_rename(pk, old_path, self._patch_for(task, pk), config) + except Exception as exc: + self._report_error(old_path, f"rename failed: {exc}") + continue + if plan is None: 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( + continue + if plan.target == old_path and plan.final_path == old_path: + continue + if reason := self._claim_conflict(plan, claimed, comic_paths): + self._report_error(old_path, reason) + continue + claimed[plan.target] = pk + claimed[plan.final_path] = pk + plans.append(plan) + return plans + + @staticmethod + def _patch_for(task: BulkTagWriteTask, pk: int) -> dict | None: + """Return the patch this comic will be written with, if any.""" + if task.per_comic_patches and pk in task.per_comic_patches: + return task.per_comic_patches[pk] + return task.patch or None + + @staticmethod + def _destination_conflict( + plan: RenamePlan, destination: Path, claimed: dict[Path, int] + ) -> str: + """Return why one destination is unavailable, or "".""" + other = claimed.get(destination) + if other is not None and other != plan.pk: + return f"another comic in this batch renames to {destination}" + if destination == plan.old_path: + return "" + if destination.exists() and not ( + # A case-only rename on a case-insensitive filesystem finds + # itself at the destination; that is the file we are moving. + plan.old_path.exists() and destination.samefile(plan.old_path) + ): + return f"rename target already exists: {destination}" + if Comic.objects.filter(path=str(destination)).exclude(pk=plan.pk).exists(): + return f"another comic already holds {destination}" + return "" + + @classmethod + def _claim_conflict( + cls, plan: RenamePlan, claimed: dict[Path, int], comic_paths: dict[int, Path] + ) -> str: + """Return why this plan's destinations are unavailable, or "".""" + for destination in (plan.target, plan.final_path): + if reason := cls._destination_conflict(plan, destination, claimed): + return reason + # A path another comic in this batch is renaming *away* from is + # only free once that rename runs, which it may not. + for other_pk, other_path in comic_paths.items(): + if other_pk != plan.pk and other_path in (plan.target, plan.final_path): + return f"{plan.target} is another comic's current path" + return "" + + def _rename_first( self, - candidates: set[int], + task: BulkTagWriteTask, + comic_paths: dict[int, Path], + lib_of: dict[int, int], + ) -> dict[int, Path]: + """Rename archives to their scheme names and move the rows with them.""" + if not task.rename: + return {} + plans = self._plan_renames(task, comic_paths) + renamed: dict[int, Path] = {} + moves: defaultdict[int, dict[int, tuple[str, str]]] = defaultdict(dict) + for plan in plans: + if plan.target == plan.old_path: + continue + try: + plan.old_path.rename(plan.target) + except OSError as exc: + self._report_error(plan.old_path, f"rename failed: {exc}") + continue + renamed[plan.pk] = plan.target + moves[lib_of[plan.pk]][plan.pk] = ( + str(plan.old_path), + str(plan.target), + ) + for library_id, library_moves in moves.items(): + for pk in self._apply_moves_inline(library_id, library_moves): + # The database refused the move, so put the file back + # rather than leave the row pointing at a path that no + # longer exists. + self._revert_rename(pk, library_moves[pk]) + renamed.pop(pk, None) + return renamed + + def _revert_rename(self, pk: int, move: tuple[str, str]) -> None: + """Undo a disk rename whose database move did not take.""" + src, dest = move + old_path = Path(src) + new_path = Path(dest) + try: + if new_path.exists() and not old_path.exists(): + new_path.rename(old_path) + except OSError as exc: + self.log.warning(f"Could not undo rename of {dest}: {exc}") + self._report_error(new_path, f"rename reverted; database move failed (pk {pk})") + + def _rename_kept_conversions( + self, + task: BulkTagWriteTask, current_paths: dict[int, Path], + written_paths: dict[int, Path], + converted_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 = current_paths[pk] + """Give the new CBZ of a kept-original conversion its scheme name.""" + if not task.rename or task.delete_original: + return {} + config = build_predict_config(task.delete_keys, task.mode) + post_renamed: dict[int, Path] = {} + for pk, written_path in written_paths.items(): + if pk in converted_paths or written_path == current_paths[pk]: + continue try: - new_path = self._rename_one(old_path) + name = predict_name(written_path, None, config) + if not name: + continue + target = written_path.parent / name + if target == written_path or target.exists(): + continue + written_path.rename(target) except Exception as exc: - self.log.warning(f"Rename error for {old_path}: {exc}") - add_tag_write_error(str(old_path), f"rename failed: {exc}") - had_errors = True + self._report_error(written_path, f"rename failed: {exc}") continue - if new_path is None or new_path == old_path: - continue - renamed_paths[pk] = new_path - if had_errors: - self.librarian_queue.put(TAG_WRITE_ERRORS_CHANGED_TASK) - 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]: + post_renamed[pk] = target + return post_renamed + + def _report_error(self, path: Path, reason: str) -> None: + """Surface a per-file failure to admins (badge + Tagging panel).""" + self.log.warning(f"Tag write: {reason} for {path}") + add_tag_write_error(str(path), reason) + self.librarian_queue.put(TAG_WRITE_ERRORS_CHANGED_TASK) + + def _apply_moves_inline( + self, library_id: int, moves: dict[int, tuple[str, str]] + ) -> set[int]: """ - 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. + Apply path moves to the database now, and report which didn't take. + + Runs the importer's own move phase rather than a hand-rolled path + update: it drops moves onto occupied destinations, keeps the stored + stat a rename doesn't change, and re-parents the row. Constructing + the importer costs one query. Its ``apply``/``finish`` are never + called — ``finish`` would end this batch's live progress status. + + Failures are read back from the database rather than inferred from + the move phase's count, which reports nothing about *which* comic + it dropped and can come back zero after the rows were already + updated. """ - 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 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 - - @staticmethod - def _guard_move_paths(src: str, written_path: Path | None, move_to: str) -> None: + if not moves: + return set() + start_time = now() + import_task = ImportTask( + library_id=library_id, + files_moved=dict(moves.values()), + ) + importer = ComicImporter( + import_task, + self.log, + self.librarian_queue, + self.db_write_lock, + self.abort_event, + ) + try: + importer.bulk_comics_moved() + except Exception: + self.log.exception(f"Applying tag write moves in library {library_id}") + landed = dict(Comic.objects.filter(pk__in=moves).values_list("pk", "path")) + failed = {pk for pk, (_, dest) in moves.items() if landed.get(pk) != dest} + TimestampUpdater( + self.log, self.librarian_queue, self.db_write_lock + ).update_library_collections(importer.library, start_time, {}) + # The browser's page-mtime cache would otherwise serve the old + # paths for its TTL. + cache.clear() + self.librarian_queue.put(LIBRARY_CHANGED_TASK) + return failed + + def _sync_conversions( + self, + task: BulkTagWriteTask, + current_paths: dict[int, Path], + written_paths: dict[int, Path], + lib_of: dict[int, int], + ) -> dict[int, Path]: """ - Hold every path this move passes through until the importer applies it. - - A scan that lands mid-batch reports the same conversion as an - unrelated delete plus create and, being enqueued first, reaches - the importer first. Registering the DB's now-dead source, the - interim archive the write produced, and the final destination - makes that scan a no-op for them, so the move below still finds - its source row — and its bookmarks — in place. See - ``codex.librarian.scribe.tagwrite_moves``. + Move rows onto the CBZ a conversion produced, and report which moved. + + A repacked archive is a new file at a new path, and the original is + gone under ``delete_original`` — neither scanner can pair that into + a move, so codex has to state it. With the original kept, the row + stays on it and the CBZ is a separate new file. """ - waypoints = (str(written_path),) if written_path else () - register_tag_write_move(src, move_to, waypoints) + if not task.delete_original: + return {} + moves: defaultdict[int, dict[int, tuple[str, str]]] = defaultdict(dict) + converted: dict[int, Path] = {} + for pk, written_path in written_paths.items(): + source = current_paths[pk] + if written_path == source: + continue + moves[lib_of[pk]][pk] = (str(source), str(written_path)) + converted[pk] = written_path + for library_id, library_moves in moves.items(): + for pk in self._apply_moves_inline(library_id, library_moves): + self.log.warning( + f"Tag write: conversion move failed for {library_moves[pk][1]}" + ) + converted.pop(pk, None) + return converted - def _sync_db( + def _enqueue_rereads( self, - task: BulkTagWriteTask, - comic_paths: dict[int, Path], + current_paths: dict[int, Path], written_paths: dict[int, Path], - renamed_paths: dict[int, Path], + converted_paths: dict[int, Path], + post_renamed: dict[int, Path], lib_of: dict[int, int], - library_events: dict[int, bool], ) -> None: """ - 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. A batch - long enough to force a mid-batch watcher flush (or a poll that lands - during it) would otherwise get that scan's delete in first, so every - path a move passes through is registered in ``tagwrite_moves`` and - the importer holds it for this task. 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): 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 its paths are held against a mid-batch scan exactly as a - conversion's are. - - In-place write (no conversion, no rename): watched libraries re-read - via the watcher's modify event; unwatched ones are told here. + Ask the importer to re-read the metadata codex just wrote. + + Every move is already applied, so these tasks only name paths the + database holds: a scan that beats them to the queue reconciles + against rows that are already correct. Watched libraries would + eventually re-read from the watcher's own modify event, but only + with the import-metadata flag on, so the re-read is requested + either way. """ - 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(): + for pk, written_path in written_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: - src = str(db_path) - moved[library_id][src] = move_to - self._guard_move_paths(src, written_paths.get(pk), 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(): + if pk in converted_paths: + modified[library_id].add(str(converted_paths[pk])) + elif written_path != current_paths[pk]: + # A kept original's new CBZ is a file the database has + # never seen. + created[library_id].add(str(post_renamed.get(pk, written_path))) + else: + modified[library_id].add(str(written_path)) + # A rename with no write changed no metadata, so nothing to re-read. + for library_id in modified.keys() | created.keys(): import_task = ImportTask( library_id=library_id, - files_moved=moved.get(library_id, {}), files_modified=frozenset(modified.get(library_id, ())), files_created=frozenset(created.get(library_id, ())), force_import_metadata=True, diff --git a/codex/librarian/scribe/tagwrite_moves.py b/codex/librarian/scribe/tagwrite_moves.py deleted file mode 100644 index 6315a52e8..000000000 --- a/codex/librarian/scribe/tagwrite_moves.py +++ /dev/null @@ -1,105 +0,0 @@ -""" -Paths a tag-write batch has moved but has not yet reconciled in the database. - -Writing tags can move a comic's archive out from under its database row. -Comicbox repacks an unwritable CBR as a CBZ — a new inode at a new path — -and the rename pass then gives the result its scheme name. ``TagWriter`` -records those moves as one targeted ``ImportTask`` enqueued when the whole -batch finishes, but the batch can run for minutes and both scanners report -the same filesystem churn in the meantime. - -A watcher batch force-yielded mid-write (60s of continuous activity, see -``codex.librarian.fs.watcher.watcher``) or a poll that lands during the -write carries a conversion as an unrelated delete plus create: the new -archive is a new inode, so neither scanner can pair it into a move. That -task is enqueued *before* the tag writer's, and ``ScribeThread``'s -``PriorityQueue`` breaks ties between equal-priority import tasks by -enqueue time, so it runs first — deleting the comic row by its now-dead -path, cascading its bookmarks away, and leaving the tag writer's move with -no source row to find. - -This registry lets the importer recognize those paths as codex's own -in-flight work and leave them to it. ``TagWriter`` registers every path a -move it is about to enqueue passes through; ``init_apply`` drops -registered paths from a task's created/modified/deleted sets and releases -a move's whole group when a task actually carries that move — which the -tag writer's own task does, in the phase that runs before its own reads -and deletes. - -The store is process-local on purpose. Only ``ScribeThread`` writes and -reads it, and its lifetime should match the librarian queue's: a librarian -restart loses the pending ``ImportTask`` along with the queue, so a guard -that outlived it would strand those paths instead. The TTL is a backstop -for a batch whose move task never arrives at all; expiry simply restores -the unguarded behavior, which the next scan reconciles. -""" - -from __future__ import annotations - -from threading import Lock -from time import monotonic -from typing import TYPE_CHECKING - -if TYPE_CHECKING: - from collections.abc import Iterable, Mapping - -# Long enough to outlast a queue backed up behind a large import, short -# enough that a leaked entry heals the same day. -_TTL = 60 * 60 * 6 - -_LOCK = Lock() -# Guarded path -> (move source, move destination, expiry). Absolute paths -# are unique across libraries — the admin serializer rejects a library -# path that nests inside another — so no library scoping is needed here. -_PENDING: dict[str, tuple[str, str, float]] = {} - - -def _prune(now: float) -> None: - """Drop expired entries. Call with the lock held.""" - expired = [path for path, entry in _PENDING.items() if entry[-1] <= now] - for path in expired: - del _PENDING[path] - - -def register_tag_write_move( - src_path: str, dest_path: str, waypoints: Iterable[str] = () -) -> None: - """Guard every path one pending tag-write move passes through.""" - now = monotonic() - expiry = now + _TTL - with _LOCK: - _prune(now) - for path in (src_path, dest_path, *waypoints): - _PENDING[path] = (src_path, dest_path, expiry) - - -def get_pending_tag_write_paths() -> frozenset[str]: - """Return every path guarded by an unreconciled tag-write move.""" - now = monotonic() - with _LOCK: - _prune(now) - return frozenset(_PENDING) - - -def release_tag_write_moves(moves: Mapping[str, str]) -> None: - """ - Release the guarded group of every registered move a task carries. - - Matches on the whole move, not just its source: a scanner that infers - some *other* destination for a guarded source has not reconciled this - move and must not lift its guard. - """ - if not moves: - return - with _LOCK: - released = [ - path for path, (src, dest, _) in _PENDING.items() if moves.get(src) == dest - ] - for path in released: - del _PENDING[path] - - -def clear_tag_write_moves() -> None: - """Drop every guard. For test isolation; the process owns the lifetime.""" - with _LOCK: - _PENDING.clear() diff --git a/codex/librarian/scribe/tagwrite_rename.py b/codex/librarian/scribe/tagwrite_rename.py new file mode 100644 index 000000000..3d1c37350 --- /dev/null +++ b/codex/librarian/scribe/tagwrite_rename.py @@ -0,0 +1,97 @@ +""" +Predict where a tag write will leave each archive. + +Renaming happens *before* the write, so the destination has to be known +in advance rather than read back off the finished file. Comicbox does the +predicting — the name it renders is exactly what ``rename_file`` would +use — but it has to be handed the same settings and pending patch the +write will apply, or the rename lands somewhere the write never agreed +to. + +The admin preflight preview derives its names through here too, so the +dialog cannot promise a name the rename won't produce. +""" + +from __future__ import annotations + +from dataclasses import dataclass, replace +from typing import TYPE_CHECKING + +from comicbox.box import Comicbox +from comicbox.config.settings import WriteMode + +from codex.settings import COMICBOX_CONFIG + +if TYPE_CHECKING: + from collections.abc import Collection, Mapping + from pathlib import Path + +#: Archives comicbox writes in place. Every other format is repacked as a +#: CBZ at a new path, which is what makes a conversion a *move* rather +#: than a modification. +_WRITE_IN_PLACE_SUFFIXES = frozenset({".cbz", ".pdf"}) +_CBZ_SUFFIX = ".cbz" + + +@dataclass(frozen=True, slots=True) +class RenamePlan: + """Where one comic's archive is headed.""" + + pk: int + old_path: Path + #: The pre-write rename destination. Keeps the archive's current + #: suffix: a CBR is still a CBR until the write repacks it. + target: Path + #: Where the file ends up once the write's conversion (if any) is + #: done. Only differs from ``target`` for a converting archive. + final_path: Path + + +def build_predict_config(delete_keys: Collection[str] | None, mode: str): + """ + Return the config a write with these settings would parse under. + + Mirrors comicbox's own ``_build_write_settings``: the write mode + picks the merger that decides whether a patch value replaces or + extends what the archive already holds, and cleared fields must + vanish from the predicted name exactly as the write will clear them. + """ + write = replace(COMICBOX_CONFIG.write, mode=WriteMode(mode)) + keys = frozenset( + key.removeprefix("comicbox.") for key in (delete_keys or ()) if key + ) + if not keys: + return replace(COMICBOX_CONFIG, write=write) + general = replace( + COMICBOX_CONFIG.general, + delete_keys=COMICBOX_CONFIG.general.delete_keys | keys, + ) + return replace(COMICBOX_CONFIG, write=write, general=general) + + +def predict_name(path: Path, patch: Mapping | None, config) -> str: + """ + Return the scheme name a write carrying ``patch`` would rename to. + + Empty when no usable name could be built. Opens the archive. + """ + metadata = {"comicbox": dict(patch)} if patch else None + with Comicbox(path, config=config, metadata=metadata) as car: + return car.predict_filename() + + +def will_convert(path: Path) -> bool: + """Whether writing this archive repacks it as a CBZ at a new path.""" + return path.suffix.lower() not in _WRITE_IN_PLACE_SUFFIXES + + +def plan_rename( + pk: int, old_path: Path, patch: Mapping | None, config +) -> RenamePlan | None: + """Build one comic's rename plan, or None when there is no name.""" + name = predict_name(old_path, patch, config) + if not name: + return None + target = old_path.parent / name + final_path = target.with_suffix(_CBZ_SUFFIX) if will_convert(target) else target + return RenamePlan(pk=pk, old_path=old_path, target=target, final_path=final_path) diff --git a/codex/settings/__init__.py b/codex/settings/__init__.py index c095c41f6..3b81a068e 100644 --- a/codex/settings/__init__.py +++ b/codex/settings/__init__.py @@ -1188,22 +1188,3 @@ 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 1c585eb3f..a75228b51 100644 --- a/codex/views/admin/tagwrite.py +++ b/codex/views/admin/tagwrite.py @@ -2,23 +2,20 @@ import json from collections.abc import Sequence -from dataclasses import replace from pathlib import Path from types import MappingProxyType from typing import override -from comicbox.box import Comicbox -from comicbox.formats import MetadataFormats from rest_framework.permissions import BasePermission, IsAdminUser from rest_framework.response import Response from rest_framework.status import HTTP_202_ACCEPTED from codex.librarian.mp_queue import LIBRARIAN_QUEUE +from codex.librarian.scribe.tagwrite_rename import build_predict_config, plan_rename from codex.librarian.scribe.tasks import BulkTagWriteTask from codex.models.admin import ComicboxTaggingDefaults from codex.models.comic import Comic from codex.serializers.admin.tagging import TagWriteRequestSerializer -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,50 +101,35 @@ class AdminTagWritePreflightView(FilteredComicPksView): """Check how many comics need conversion before writing.""" @staticmethod - def _preview_one(old_path: Path, patch: dict | None, config) -> str: + def _preview_one(pk: int, old_path: Path, patch: dict | None, config) -> str: """ - Return the comicbox-scheme name the given patch produces for one comic. + Return the name this comic ends up with, or "" if it can't be built. - 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 - 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. + Derived through the same planner the rename itself uses, so the + dialog cannot promise a name the write won't produce. Shows the + *final* name, which for an archive the write will repack is the + converted CBZ rather than the interim one. Opens the archive (I/O). """ - metadata = dict(patch) if patch else {} - if ext := old_path.suffix.lstrip("."): - metadata["ext"] = ext try: - with Comicbox( - old_path, config=config, metadata={"comicbox": metadata} - ) as car: - target = car.to_string(MetadataFormats.FILENAME) or "" + plan = plan_rename(pk, old_path, patch, config) except Exception: return "" - return "" if target.startswith(".") else target + return plan.final_path.name if plan else "" def _filename_previews( self, comic_pks: frozenset[int], patch_str: str, delete_keys: tuple[str, ...] = (), + mode: str = "additive", ) -> list[dict[str, str]]: """Preview the rename (old → new) for each selected comic, capped.""" patch = json.loads(patch_str or "null") # Pending cleared fields must vanish from the previewed name exactly - # as the real write (BulkWriteItem.delete_keys) will clear them. - config = COMICBOX_RENAME_CONFIG - if delete_keys: - config = replace( - config, - general=replace( - config.general, - delete_keys=config.general.delete_keys | frozenset(delete_keys), - ), - ) + # as the real write (BulkWriteItem.delete_keys) will clear them, and + # the write mode decides whether a patch value replaces or extends + # what the archive already holds. + config = build_predict_config(delete_keys, mode) comics = ( Comic.objects.filter(pk__in=comic_pks) .only("pk", "path") @@ -159,7 +141,7 @@ def _filename_previews( previews.append( { "old": old_path.name, - "new": self._preview_one(old_path, patch, config), + "new": self._preview_one(comic.pk, old_path, patch, config), } ) return previews @@ -199,6 +181,7 @@ def post(self, request): comic_pks, data.get("patch") or "", tuple(data.get("delete_keys") or ()), + data["mode"], ), "skipped": self.skipped_read_only, } diff --git a/tests/importer/test_tag_write_move_guard.py b/tests/importer/test_tag_write_move_guard.py deleted file mode 100644 index d72ecbd82..000000000 --- a/tests/importer/test_tag_write_move_guard.py +++ /dev/null @@ -1,228 +0,0 @@ -""" -A scan that lands mid tag-write must not reconcile the paths it is moving. - -Writing tags to a CBR converts it to a CBZ at a new inode, so neither the -watcher nor the poller can pair the change into a move. A watcher batch -force-yielded during a long write (or a poll landing in it) therefore -carries the conversion as an unrelated delete plus create, and it reaches -the importer *before* the tag writer's end-of-batch move task. Deleting -the row by its now-dead path would cascade the comic's bookmarks away and -leave the move with no source. ``_defer_pending_tag_write_moves`` holds -those paths for the task that owns them. -""" - -import shutil -from pathlib import Path -from threading import Event, Lock -from typing import override - -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.librarian.scribe.tagwrite_moves import ( - clear_tag_write_moves, - get_pending_tag_write_paths, - register_tag_write_move, -) -from codex.models import ( - Comic, - Folder, - Imprint, - Library, - Publisher, - Series, - Volume, -) -from tests.importer.test_basic import ( - COMIC_PATH, - LIBRARY_PATH, - BaseTestImporter, -) - -# The DB path, the interim archive the conversion wrote, and the final -# name the rename pass gave it. -_CBR_PATH = str(LIBRARY_PATH / "converted.cbr") -_CBZ_PATH = str(LIBRARY_PATH / "converted.cbz") -_RENAMED_PATH = str(LIBRARY_PATH / "Renamed #001.cbz") -_UNRELATED_PATH = str(LIBRARY_PATH / "unrelated.cbz") -_UNRELATED_PATH_ALT = str(LIBRARY_PATH / "unrelated-alt.cbz") - - -class TestImporterTagWriteMoveGuard(BaseTestImporter): - """Paths an in-flight tag write owns are deferred to its own task.""" - - @override - def setUp(self) -> None: - super().setUp() - clear_tag_write_moves() - 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="Guard Pub") - imp = Imprint.objects.create(name="Guard Imprint", publisher=pub) - ser = Series.objects.create(name="Guard 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 - - @override - def tearDown(self) -> None: - clear_tag_write_moves() - super().tearDown() - - def _create_comic(self, path: str) -> Comic: - """Create a comic with its file present, as presave stats disk.""" - shutil.copy(COMIC_PATH, path) - self.issue_number += 1 - return Comic.objects.create( - library=self.library, - path=path, - parent_folder=self.folder, - issue_number=self.issue_number, - name=Path(path).stem, - size=1, - page_count=1, - **self.tags, - ) - - def _importer(self, **task_kwargs) -> ComicImporter: - task = ImportTask(library_id=self.library.pk, **task_kwargs) - return ComicImporter(task, logger, LIBRARIAN_QUEUE, Lock(), Event()) - - @staticmethod - def _register_conversion() -> None: - """Register the move a tag write is about to enqueue.""" - register_tag_write_move(_CBR_PATH, _RENAMED_PATH, (_CBZ_PATH,)) - - def test_delete_of_a_pending_move_source_is_deferred(self) -> None: - """The row the pending move needs survives the scan's delete.""" - comic = self._create_comic(_CBR_PATH) - self._register_conversion() - importer = self._importer(files_deleted=frozenset({_CBR_PATH})) - - importer._defer_pending_tag_write_moves() # noqa: SLF001 - importer.delete() - - assert not importer.task.files_deleted - assert importer.counts.comics_deleted == 0 - assert Comic.objects.filter(pk=comic.pk).exists() - - def test_create_of_a_pending_move_path_is_deferred(self) -> None: - """The interim and final archives are not imported as new comics.""" - self._create_comic(_CBR_PATH) - self._register_conversion() - importer = self._importer( - files_created=frozenset({_CBZ_PATH, _RENAMED_PATH}), - ) - - importer._defer_pending_tag_write_moves() # noqa: SLF001 - - assert not importer.task.files_created - - 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})) - - importer._defer_pending_tag_write_moves() # noqa: SLF001 - importer.delete() - - assert importer.counts.comics_deleted == 1 - assert not Comic.objects.filter(pk=comic.pk).exists() - - def test_carrying_the_move_releases_its_own_guard(self) -> None: - """The tag writer's own task keeps the move and the re-read it asked for.""" - self._create_comic(_CBR_PATH) - self._register_conversion() - importer = self._importer( - files_moved={_CBR_PATH: _RENAMED_PATH}, - files_modified=frozenset({_RENAMED_PATH}), - ) - - importer._defer_pending_tag_write_moves() # noqa: SLF001 - - # Its own move and re-read survive... - assert importer.task.files_moved == {_CBR_PATH: _RENAMED_PATH} - assert importer.task.files_modified == frozenset({_RENAMED_PATH}) - # ...and the guard is gone, so a later scan reconciles normally. - later = self._importer(files_deleted=frozenset({_CBZ_PATH})) - later._defer_pending_tag_write_moves() # noqa: SLF001 - assert later.task.files_deleted == frozenset({_CBZ_PATH}) - - def test_a_different_destination_does_not_release_the_guard(self) -> None: - """Only the registered move reconciles it; a scanner's guess must not.""" - comic = self._create_comic(_CBR_PATH) - self._register_conversion() - # A scanner inferred some other destination for the same source. - scan = self._importer( - files_moved={_CBR_PATH: _UNRELATED_PATH}, - files_deleted=frozenset({_CBZ_PATH}), - ) - - scan._defer_pending_tag_write_moves() # noqa: SLF001 - - # The guard held, so the interim archive was not reaped... - assert not scan.task.files_deleted - # ...and the real move still finds its source row. - move = self._importer(files_moved={_CBR_PATH: _RENAMED_PATH}) - move._defer_pending_tag_write_moves() # noqa: SLF001 - assert not get_pending_tag_write_paths() - assert Comic.objects.filter(pk=comic.pk).exists() - - def test_a_tasks_own_move_paths_are_never_deferred(self) -> None: - """A task's own move survives even when its guard is still registered.""" - self._create_comic(_CBR_PATH) - # Registered against a different destination, so the release - # below does not fire and the guard stays live. - register_tag_write_move(_CBR_PATH, _UNRELATED_PATH, (_CBZ_PATH,)) - importer = self._importer( - files_moved={_CBR_PATH: _UNRELATED_PATH_ALT}, - files_modified=frozenset({_UNRELATED_PATH_ALT}), - ) - - importer._defer_pending_tag_write_moves() # noqa: SLF001 - - assert importer.task.files_moved == {_CBR_PATH: _UNRELATED_PATH_ALT} - assert importer.task.files_modified == frozenset({_UNRELATED_PATH_ALT}) - - def test_scan_delete_then_move_keeps_the_original_row(self) -> None: - """End to end: the scan is a no-op and the move lands on the same row.""" - comic = self._create_comic(_CBR_PATH) - # The conversion happened on disk: the cbr is gone, the renamed - # cbz is in its place. - shutil.copy(COMIC_PATH, _RENAMED_PATH) - Path(_CBR_PATH).unlink() - self._register_conversion() - - # The scan that force-flushed mid-write runs first. - scan = self._importer( - files_deleted=frozenset({_CBR_PATH}), - files_created=frozenset({_CBZ_PATH}), - ) - scan._defer_pending_tag_write_moves() # noqa: SLF001 - scan.delete() - - # Then the tag writer's move task. - move = self._importer(files_moved={_CBR_PATH: _RENAMED_PATH}) - move._defer_pending_tag_write_moves() # noqa: SLF001 - move.move_and_modify_dirs() - - assert move.counts.comics_moved == 1 - comic.refresh_from_db() - assert comic.path == _RENAMED_PATH diff --git a/tests/test_tag_writer_rename.py b/tests/test_tag_writer_rename.py index 2fdafdc19..aa21b4d36 100644 --- a/tests/test_tag_writer_rename.py +++ b/tests/test_tag_writer_rename.py @@ -1,12 +1,12 @@ """ -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, 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, the paths a recorded move -holds against a scan that lands mid-batch, and the skip-and-report -collision guard. +Tests for ``TagWriter``'s rename-first flow and its inline database sync. + +Renaming happens before the write, and every move it causes is applied to +the database before ``write_tags`` returns. That ordering is the whole +point: no scan can be processed while the database disagrees with the +disk, so a comic keeps its bookmarks through a rename, a conversion, or +both. What remains queued is only the metadata re-read, which names a path +the database already holds. """ from __future__ import annotations @@ -16,6 +16,7 @@ import zipfile from io import BytesIO from pathlib import Path +from threading import Event, Lock from typing import Any, Final, Self, override from unittest.mock import patch @@ -27,26 +28,24 @@ from codex.librarian.scribe.importer.tasks import ImportTask from codex.librarian.scribe.tag_writer import TagWriter from codex.librarian.scribe.tagwrite_errors import get_tag_write_errors -from codex.librarian.scribe.tagwrite_moves import ( - clear_tag_write_moves, - get_pending_tag_write_paths, -) +from codex.librarian.scribe.tagwrite_rename import build_predict_config, plan_rename from codex.librarian.scribe.tasks import BulkTagWriteTask from codex.models import ( Comic, + Folder, Imprint, Library, Publisher, 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" +_COMICBOX_TARGET: Final = "codex.librarian.scribe.tagwrite_rename.Comicbox" _TARGET_NAME: Final = "Renamed #001.cbz" _EXAMPLE_CBZ: Final = Path(__file__).parent / "files" / "comicbox-2-example.cbz" +#: One comic renames, the other keeps its own path. +_DISTINCT_PATHS: Final = 2 def _double(stub: object) -> Any: @@ -66,16 +65,15 @@ def put(self, item) -> None: class _FakeComicbox: """ - Stand-in for ``comicbox.box.Comicbox`` used by the rename pass. + Stand-in for ``comicbox.box.Comicbox`` used by rename prediction. - ``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``. + ``predict_filename`` returns a fixed scheme name, keeping the source + archive's suffix as the real one does. Codex performs the rename, so + the fake never touches the filesystem — the collision checks, + ``samefile``, and the database sync all run for real. """ - target: str = _TARGET_NAME + stem: str = Path(_TARGET_NAME).stem def __init__(self, path, **_kwargs) -> None: self._path = Path(path) @@ -86,36 +84,45 @@ def __enter__(self) -> Self: def __exit__(self, *_exc: object) -> bool: return False - 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 predict_filename(self) -> str: + return f"{self.stem}{self._path.suffix}" -def _make_comic(*, events: bool, name: str = "c.cbz", read_only: bool = False) -> Comic: +def _make_library(*, events: bool, read_only: bool = False) -> Library: _TMP_DIR.mkdir(exist_ok=True, parents=True) - library = Library.objects.create( + return Library.objects.create( path=str(_TMP_DIR), events=events, read_only=read_only ) - 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( + + +def _make_comic( + *, + events: bool, + name: str = "c.cbz", + read_only: bool = False, + library: Library | None = None, + issue_number: int = 1, +) -> Comic: + library = library or _make_library(events=events, read_only=read_only) + publisher, _ = Publisher.objects.get_or_create(name="P") + imprint, _ = Imprint.objects.get_or_create(name="I", publisher=publisher) + series, _ = Series.objects.get_or_create( + name="S", publisher=publisher, imprint=imprint + ) + volume, _ = Volume.objects.get_or_create( name="1", publisher=publisher, imprint=imprint, series=series ) + folder, _ = Folder.objects.get_or_create( + library=library, path=str(_TMP_DIR), defaults={"name": _TMP_DIR.name} + ) path = _TMP_DIR / name path.write_text("comic") - return Comic.objects.create( + comic = Comic.objects.create( library=library, path=path, - issue_number=1, - name="c", + parent_folder=folder, + issue_number=issue_number, + name=path.stem, publisher=publisher, imprint=imprint, series=series, @@ -123,6 +130,8 @@ def _make_comic(*, events: bool, name: str = "c.cbz", read_only: bool = False) - size=1, file_type="CBZ", ) + comic.folders.add(folder) + return comic def _make_writer(queue: _FakeQueue) -> TagWriter: @@ -130,387 +139,348 @@ def _make_writer(queue: _FakeQueue) -> TagWriter: writer = TagWriter.__new__(TagWriter) writer.log = _double(logger) writer.librarian_queue = _double(queue) + writer.db_write_lock = _double(Lock()) + writer.abort_event = _double(Event()) return writer -class TagWriterRenameTests(TestCase): - """The rename pass renames archives and syncs the DB, watcher-aware.""" +def _imports(queue: _FakeQueue) -> list[ImportTask]: + return [i for i in queue.items if isinstance(i, ImportTask)] + + +class TagWriterRenameFirstTests(TestCase): + """The rename lands on disk and in the database before the write.""" @override def setUp(self) -> None: caches["default"].clear() caches["tagging"].clear() - clear_tag_write_moves() @override def tearDown(self) -> None: - clear_tag_write_moves() shutil.rmtree(_TMP_DIR, ignore_errors=True) - def test_rename_only_unwatched_enqueues_move(self) -> None: - """Rename-only in an unwatched library renames + enqueues a move task.""" + @staticmethod + def _rename_only(comic: Comic) -> BulkTagWriteTask: + return BulkTagWriteTask(comic_pks=frozenset({comic.pk}), rename=True) + + def test_rename_moves_the_row_before_returning(self) -> None: + """The database holds the new path by the time write_tags returns.""" 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): - writer.write_tags(task) + _make_writer(queue).write_tags(self._rename_only(comic)) new_path = old_path.parent / _TARGET_NAME assert new_path.exists() assert not old_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(new_path)} - # Rename-only: metadata unchanged, so no re-read is requested. - assert imports[0].files_modified == frozenset() - assert imports[0].force_import_metadata is True - - def test_read_only_library_is_never_renamed(self) -> None: - """A read-only comic is not renamed even if a task carries its pk.""" - comic = _make_comic(events=False, read_only=True) + comic.refresh_from_db() + assert comic.path == str(new_path) + # No move task is queued: there is nothing left for the importer to + # reconcile, which is what makes a racing scan harmless. + assert all(not i.files_moved for i in _imports(queue)) + + def test_a_watched_library_is_synced_the_same_way(self) -> None: + """Watcher pairing is unreliable, so codex never depends on it.""" + comic = _make_comic(events=True) 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) + _make_writer(queue).write_tags(self._rename_only(comic)) - # File untouched and nothing enqueued: the read-only exclusion drops it - # before the rename pass ever runs. - assert old_path.exists() - assert not (old_path.parent / _TARGET_NAME).exists() - assert not queue.items + comic.refresh_from_db() + assert comic.path == str(old_path.parent / _TARGET_NAME) - 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) + def test_rename_only_asks_for_no_reread(self) -> None: + """A rename changes no metadata, so nothing needs re-reading.""" + comic = _make_comic(events=False) queue = _FakeQueue() - writer = _make_writer(queue) - task = BulkTagWriteTask(comic_pks=frozenset({comic.pk}), rename=True) with patch(_COMICBOX_TARGET, _FakeComicbox): - writer.write_tags(task) + _make_writer(queue).write_tags(self._rename_only(comic)) - 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() - # Both ends are held until that move is applied, so a scan landing - # first can't reconcile them out from under it. - assert get_pending_tag_write_paths() == frozenset( - {str(old_path), str(new_path)} - ) + assert not _imports(queue) + + def test_the_row_keeps_its_identity(self) -> None: + """The row is moved, not replaced, so its bookmarks survive.""" + comic = _make_comic(events=False) + pk = comic.pk + queue = _FakeQueue() + + with patch(_COMICBOX_TARGET, _FakeComicbox): + _make_writer(queue).write_tags(self._rename_only(comic)) + + assert Comic.objects.filter(pk=pk).exists() - def test_tag_write_and_rename_watched_enqueues_move_and_reread(self) -> None: + def test_a_second_write_resolves_the_new_path(self) -> None: """ - A watched write + rename records the move and re-reads the new path. + A follow-up edit sees the renamed path, not the pre-rename one. - 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. + The database used to be synced by a queued task that a second + tag-write outranked, so the second write opened a path that no + longer existed and dropped the user's edit. """ - comic = _make_comic(events=True) - old_path = Path(comic.path) + comic = _make_comic(events=False) 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) + with patch(_COMICBOX_TARGET, _FakeComicbox): + writer = _make_writer(queue) + writer.write_tags(self._rename_only(comic)) + resolved, _ = writer._resolve_comics( # noqa: SLF001 + self._rename_only(comic) + ) - 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)}) - assert get_pending_tag_write_paths() == frozenset( - {str(old_path), str(new_path)} - ) + new_path = _TMP_DIR / _TARGET_NAME + assert resolved[comic.pk] == new_path + assert resolved[comic.pk].exists() - 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) + def test_read_only_library_is_never_renamed(self) -> None: + """A read-only comic is untouched even if a task carries its pk.""" + comic = _make_comic(events=False, read_only=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) + with patch(_COMICBOX_TARGET, _FakeComicbox): + _make_writer(queue).write_tags(self._rename_only(comic)) - # 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)] - assert not get_pending_tag_write_paths() + assert old_path.exists() + assert not (old_path.parent / _TARGET_NAME).exists() + assert not queue.items - def test_collision_skips_and_reports(self) -> None: - """A target collision skips the rename and records a tag-write error.""" - comic = _make_comic(events=False) + def test_no_change_when_the_name_already_matches(self) -> None: + """Nothing happens when the scheme name is the current name.""" + comic = _make_comic(events=False, name=_TARGET_NAME) old_path = Path(comic.path) - # Pre-create a *different* file at the target name. - (old_path.parent / _TARGET_NAME).write_text("other") queue = _FakeQueue() - writer = _make_writer(queue) - task = BulkTagWriteTask(comic_pks=frozenset({comic.pk}), rename=True) with patch(_COMICBOX_TARGET, _FakeComicbox): - writer.write_tags(task) + _make_writer(queue).write_tags(self._rename_only(comic)) - # Original untouched, no move enqueued, error surfaced. assert old_path.exists() - assert not [i for i in queue.items if isinstance(i, ImportTask)] - assert TAG_WRITE_ERRORS_CHANGED_TASK in queue.items - errors = get_tag_write_errors() - assert errors - assert errors[0]["path"] == str(old_path) + assert not _imports(queue) - 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) +class TagWriterRenameCollisionTests(TestCase): + """Destinations that aren't free are skipped and reported.""" - assert old_path.exists() - assert not (old_path.parent / ".cbz").exists() - assert not [i for i in queue.items if isinstance(i, ImportTask)] + @override + def setUp(self) -> None: + caches["default"].clear() + caches["tagging"].clear() - 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) + @override + def tearDown(self) -> None: + shutil.rmtree(_TMP_DIR, ignore_errors=True) + + def test_existing_file_at_the_target_skips_the_rename(self) -> None: + """An unrelated file already at the name is never clobbered.""" + comic = _make_comic(events=False) old_path = Path(comic.path) + (old_path.parent / _TARGET_NAME).write_text("other") queue = _FakeQueue() - writer = _make_writer(queue) - task = BulkTagWriteTask(comic_pks=frozenset({comic.pk}), rename=True) with patch(_COMICBOX_TARGET, _FakeComicbox): - writer.write_tags(task) + _make_writer(queue).write_tags( + BulkTagWriteTask(comic_pks=frozenset({comic.pk}), rename=True) + ) assert old_path.exists() - assert not [i for i in queue.items if isinstance(i, ImportTask)] + assert (old_path.parent / _TARGET_NAME).read_text() == "other" + comic.refresh_from_db() + assert comic.path == str(old_path) + assert TAG_WRITE_ERRORS_CHANGED_TASK in queue.items + assert get_tag_write_errors() + + def test_two_comics_predicting_one_name_keep_the_first(self) -> None: + """A batch-internal collision costs one rename, not the archive.""" + library = _make_library(events=False) + first = _make_comic(events=False, name="a.cbz", library=library) + second = _make_comic( + events=False, name="b.cbz", library=library, issue_number=2 + ) + queue = _FakeQueue() - def test_tag_write_and_rename_unwatched_rereads_metadata(self) -> None: - """A tag write + rename re-reads metadata for the new path (unwatched).""" - comic = _make_comic(events=False) + with patch(_COMICBOX_TARGET, _FakeComicbox): + _make_writer(queue).write_tags( + BulkTagWriteTask( + comic_pks=frozenset({first.pk, second.pk}), rename=True + ) + ) + + renamed = _TMP_DIR / _TARGET_NAME + assert renamed.exists() + # Exactly one of them moved; the other kept its file and its row. + paths = { + Comic.objects.get(pk=first.pk).path, + Comic.objects.get(pk=second.pk).path, + } + assert str(renamed) in paths + assert len(paths) == _DISTINCT_PATHS + assert get_tag_write_errors() + + def test_a_target_another_comic_holds_is_skipped(self) -> None: + """A name the database already assigns elsewhere is refused.""" + library = _make_library(events=False) + comic = _make_comic(events=False, name="a.cbz", library=library) + _make_comic(events=False, name=_TARGET_NAME, library=library, issue_number=2) 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) + with patch(_COMICBOX_TARGET, _FakeComicbox): + _make_writer(queue).write_tags( + BulkTagWriteTask(comic_pks=frozenset({comic.pk}), rename=True) + ) - 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)} - # Tags were written, so the new path is re-read. - assert imports[0].files_modified == frozenset({str(new_path)}) + comic.refresh_from_db() + assert comic.path == str(old_path) + assert old_path.exists() 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. - """ + """A conversion moves the file after the write, and is synced then.""" @override def setUp(self) -> None: caches["default"].clear() caches["tagging"].clear() - clear_tag_write_moves() @override def tearDown(self) -> None: - clear_tag_write_moves() 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) + def _convert(source: Path) -> Path: + """Stand in for comicbox repacking an unwritable archive.""" + new_path = source.with_suffix(".cbz") + source.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.""" + def test_conversion_moves_the_row_to_the_cbz(self) -> None: + """Rename first, then the conversion move, both applied inline.""" 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) + renamed = old_path.parent / f"{Path(_TARGET_NAME).stem}.cbr" + converted = renamed.with_suffix(".cbz") + + def fake_write(_self, _task, current_paths): + # The write repacks whatever the rename left behind. + assert current_paths[comic.pk] == renamed + return {comic.pk: self._convert(current_paths[comic.pk])} with ( patch(_COMICBOX_TARGET, _FakeComicbox), - patch.object( - TagWriter, - "_collect_written_paths", - return_value={comic.pk: cbz_path}, - ), + patch.object(TagWriter, "_write", fake_write), ): - writer.write_tags(task) - - renamed_path = old_path.parent / _TARGET_NAME - assert renamed_path.exists() - assert not cbz_path.exists() + writer.write_tags( + BulkTagWriteTask( + comic_pks=frozenset({comic.pk}), + patch={"series": {"name": "S"}}, + delete_original=True, + rename=True, + ) + ) + + assert converted.exists() assert not old_path.exists() - imports = [i for i in queue.items if isinstance(i, ImportTask)] + comic.refresh_from_db() + assert comic.path == str(converted) + # Only the re-read is left queued, and it names the row's own path. + imports = _imports(queue) 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)}) - # Every path the move passes through is held against a scan that - # lands before the move task does. - assert get_pending_tag_write_paths() == frozenset( - {str(old_path), str(cbz_path), str(renamed_path)} - ) + assert not imports[0].files_moved + assert imports[0].files_modified == frozenset({str(converted)}) - def test_converted_write_without_rename_enqueues_move(self) -> None: - """Conversion alone moves the DB row onto the new cbz.""" + def test_keeping_the_original_leaves_the_row_alone(self) -> None: + """The row stays on the original; the CBZ is reported as new.""" 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) + def fake_write(_self, _task, current_paths): + source = current_paths[comic.pk] + new_path = source.with_suffix(".cbz") + new_path.write_text("converted") + return {comic.pk: new_path} - imports = [i for i in queue.items if isinstance(i, ImportTask)] + with ( + patch(_COMICBOX_TARGET, _FakeComicbox), + patch.object(TagWriter, "_write", fake_write), + ): + writer.write_tags( + BulkTagWriteTask( + comic_pks=frozenset({comic.pk}), + patch={"series": {"name": "S"}}, + delete_original=False, + rename=True, + ) + ) + + # The original is untouched, so its row never moved. + assert old_path.exists() + comic.refresh_from_db() + assert comic.path == str(old_path) + # The new CBZ got the scheme name and is imported as a new comic. + renamed_cbz = old_path.parent / _TARGET_NAME + assert renamed_cbz.exists() + imports = _imports(queue) assert len(imports) == 1 - assert imports[0].files_moved == {str(old_path): str(cbz_path)} - assert imports[0].files_modified == frozenset({str(cbz_path)}) + assert imports[0].files_created == frozenset({str(renamed_cbz)}) + assert not imports[0].files_moved + + +class TagWriterInPlaceWriteTests(TestCase): + """A write that moves nothing still gets its metadata re-read.""" + + @override + def setUp(self) -> None: + caches["default"].clear() + caches["tagging"].clear() + + @override + def tearDown(self) -> None: + shutil.rmtree(_TMP_DIR, ignore_errors=True) - 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") + def _write_in_place(self, comic: Comic, queue: _FakeQueue) -> None: old_path = Path(comic.path) - cbz_path = self._convert(old_path) + with patch.object(TagWriter, "_write", lambda *_args: {comic.pk: old_path}): + _make_writer(queue).write_tags( + BulkTagWriteTask( + comic_pks=frozenset({comic.pk}), patch={"series": {"name": "S"}} + ) + ) + + def test_unwatched_in_place_write_is_reread(self) -> None: + """An unwatched library has no scanner to notice the write.""" + comic = _make_comic(events=False) 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) + self._write_in_place(comic, queue) - renamed_path = old_path.parent / _TARGET_NAME - assert renamed_path.exists() - imports = [i for i in queue.items if isinstance(i, ImportTask)] + imports = _imports(queue) assert len(imports) == 1 - assert imports[0].files_moved == {str(old_path): str(renamed_path)} + assert imports[0].files_modified == frozenset({str(comic.path)}) + assert imports[0].force_import_metadata is True - 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) + def test_watched_in_place_write_is_reread_too(self) -> None: + """The watcher's own re-read is stat-only when the flag is off.""" + comic = _make_comic(events=True) 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() - # Nothing moved, so nothing needs holding back from a scan. - assert not get_pending_tag_write_paths() + self._write_in_place(comic, queue) + imports = _imports(queue) + assert len(imports) == 1 + assert imports[0].files_modified == frozenset({str(comic.path)}) -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. - """ +class TagWriterRenamePlanTests(TestCase): + """The plan names both the interim and the final path.""" @override def setUp(self) -> None: @@ -530,41 +500,87 @@ def _make_cbt(path: Path) -> None: 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.""" + def test_a_converting_archive_plans_both_paths(self) -> None: + """The interim rename keeps .cbt; the write's output is the .cbz.""" 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 + plan = plan_rename(1, old_path, None, build_predict_config((), "additive")) - assert new_path is not None - assert new_path.suffix == ".cbt" - assert new_path.exists() - assert not old_path.exists() + assert plan is not None + assert plan.target.suffix == ".cbt" + assert plan.final_path.suffix == ".cbz" + assert plan.final_path.stem == plan.target.stem - def test_cbz_archive_keeps_its_extension(self) -> None: - """The common case still renders .cbz.""" + def test_a_cbz_plans_one_path(self) -> None: + """Nothing to convert, so the rename target is the final path.""" 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 + plan = plan_rename(1, old_path, None, build_predict_config((), "additive")) - assert new_path is not None - assert new_path.suffix == ".cbz" - assert new_path.exists() + assert plan is not None + assert plan.target == plan.final_path + assert plan.target.suffix == ".cbz" - 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 +class TagWriterRenameEdgeCaseTests(TestCase): + """The awkward cases: same file, different name; and a refused move.""" + + @override + def setUp(self) -> None: + caches["default"].clear() + caches["tagging"].clear() + + @override + def tearDown(self) -> None: + shutil.rmtree(_TMP_DIR, ignore_errors=True) + + def test_case_only_rename_is_allowed(self) -> None: + """ + Renaming only the case of a name is a real rename, not a collision. + + On a case-insensitive filesystem the destination already "exists" — + as the very file being renamed — so the collision check has to ask + whether it is the same file, not merely whether something is there. + """ + comic = _make_comic(events=False, name=_TARGET_NAME.lower()) + queue = _FakeQueue() + + with patch(_COMICBOX_TARGET, _FakeComicbox): + _make_writer(queue).write_tags( + BulkTagWriteTask(comic_pks=frozenset({comic.pk}), rename=True) + ) + + comic.refresh_from_db() + assert comic.path == str(_TMP_DIR / _TARGET_NAME) + assert not get_tag_write_errors() + + def test_a_refused_database_move_puts_the_file_back(self) -> None: + """ + Disk and database never disagree, even when the move is refused. - assert new_path is not None - assert preview == new_path.name + The move phase drops a move it can't apply, which would otherwise + leave the row pointing at a path that no longer exists — the exact + state a later scan turns into a delete. + """ + comic = _make_comic(events=False) + old_path = Path(comic.path) + queue = _FakeQueue() + + with ( + patch(_COMICBOX_TARGET, _FakeComicbox), + patch( + "codex.librarian.scribe.tag_writer.ComicImporter.bulk_comics_moved", + return_value=0, + ), + ): + _make_writer(queue).write_tags( + BulkTagWriteTask(comic_pks=frozenset({comic.pk}), rename=True) + ) + + assert old_path.exists() + assert not (_TMP_DIR / _TARGET_NAME).exists() + comic.refresh_from_db() + assert comic.path == str(old_path) + assert get_tag_write_errors() diff --git a/tests/test_tagging_rename_wiring.py b/tests/test_tagging_rename_wiring.py index 8c5e1de30..0718ccaeb 100644 --- a/tests/test_tagging_rename_wiring.py +++ b/tests/test_tagging_rename_wiring.py @@ -40,12 +40,17 @@ _START_URL: Final = "/api/v4/admin/tag-sessions/start" _QUEUE_TARGET: Final = "codex.views.admin.tagwrite.LIBRARIAN_QUEUE" _START_QUEUE_TARGET: Final = "codex.views.admin.onlinetag.LIBRARIAN_QUEUE" -_VIEW_COMICBOX_TARGET: Final = "codex.views.admin.tagwrite.Comicbox" +_VIEW_COMICBOX_TARGET: Final = "codex.librarian.scribe.tagwrite_rename.Comicbox" _PREVIEW_NAME: Final = "Series v01 #001.cbz" class _PreviewComicbox: - """Minimal Comicbox stand-in for the preflight filename preview.""" + """ + Minimal Comicbox stand-in for the preflight filename preview. + + The preview derives its name through the same planner the rename uses, + so the seam is comicbox's own prediction. + """ def __init__(self, path, **_kwargs) -> None: self._path = Path(path) @@ -56,7 +61,7 @@ def __enter__(self) -> Self: def __exit__(self, *_exc: object) -> bool: return False - def to_string(self, _fmt) -> str: + def predict_filename(self) -> str: return _PREVIEW_NAME