fix: rename extensions, watcher path prefixes and scribe queue ordering - #824
Merged
Conversation
``ext`` is a metadata field, not the file's suffix, and codex's read config deletes it — so comicfn2dict fell back to its "cbz" default and every PDF/CBR/CBT/CB7 was renamed to a name claiming to be a zip. The admin preview showed the same wrong name. Codex now performs the rename itself. Comicbox's ``rename_file`` derives its own destination and cannot be handed a corrected target, so owning the move is what makes the suffix correctable. A rendered name that is nothing but an extension would create a hidden file, so it is treated as no name at all. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Both the deleted-directory expansion and the library attributor compared bare string prefixes, so any two paths where one name merely began with the other were treated as parent and child. Deleting a watched folder therefore expanded into every sibling tree sharing its leading name — "Batman" collecting all of "Batman Beyond" — and those comics were deleted, with no paired add to rescue them and their bookmarks cascading away while the files were still on disk. The same bug filed a sibling library's events under whichever library happened to be a string prefix of it. Terminating each prefix with a separator restores the boundary. The library root itself still matches its own events. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Two ways a scribe task could raise TypeError inside the queue: ``SHUTDOWN_MSG`` was a bare int where every real item is a ``(priority, timestamp)`` tuple, so stopping the thread with any task still queued raised comparing int to tuple — aborting the daemon's shutdown loop before the remaining threads were ever told to stop. Equal priorities fell through to comparing the ScribeTask dataclasses, which define no ordering. Timestamps tie more readily than they look (they are truncated, and a clock can step backwards), and the loser was a task dropped in the routing thread. A monotonic counter now closes the tuple so two entries can never compare equal. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
``_merge_task`` tested each candidate path against ``path_to_pk``'s *values*, which are pks — a Path never equals an int, so the guard never excluded anything. Starting a second scan whose selection overlapped the running one (re-picking a folder to catch additions) queued every shared comic again: an inflated total, duplicate lookups against rate-limited sources, and a second write of the same file. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The warning told the admin to poll again once copying finished, implying the task had been abandoned, but only ``init_apply`` returned early — the import ran on regardless, and skipped starting its statuses on the way out. Keep importing (abandoning the task would drop the events entirely on a watched library that isn't also polled) and describe that instead. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Supersedes the previous commit's approach. Taking the rename away from comicbox fixed the name but duplicated comicbox's job, broke the invariant that codex's collision pre-check targets the exact path ``rename_file`` will use, and would have been dead weight the moment comicbox renders the extension itself. The real defect is the input, not the renamer: ``ext`` is a metadata field, and the read config deletes it, so comicfn2dict fell back to its "cbz" default. Neither half of the fix works alone — un-deleting the key leaves it unset, and stating it under the read config gets it deleted after the merge — so renaming uses a config that keeps ``ext`` and states the archive's real suffix as metadata. That outranks any extension a third-party tagger embedded in the archive too. The admin preview derives its name the same way, so it can no longer promise a name the rename won't produce. Covered against a real archive (a CBT repacked from the example CBZ), since whether the rendered extension is right now depends on what codex hands comicbox — something the test double cannot exercise. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Quick fixes from an audit of the rename/move pipeline (the work behind #822 / #823 /
7c15ff221). Each is independent of the others; none of them change the rename architecture, which is a later change.The audit itself found the recent rename PRs sound — every codex-initiated rename event traces end-to-end to a preserved row and bookmarks. These are adjacent defects it turned up along the way.
Fixes
Rename gave every archive a
.cbzname.extis a comicbox metadata field rather than the file's suffix, and codex's read config deletes it, so comicfn2dict fell back to its"cbz"default: every PDF and unconverted CBR/CBT/CB7 was renamed to a name claiming to be a zip, and the admin preview showed the same wrong name. Renaming now uses a config that keepsextand states the archive's real suffix as metadata. Both halves are required — un-deleting the key leaves it unset, anddelete_keysis applied after the merge, so it would strip a stated value under the read config. comicbox still performs the rename; it just gets correct inputs. Stating the extension also outranks one a third-party tagger embedded in the archive.Deleting a watched folder deleted sibling folders' comics.
expand_dir_deletedmatched a bare path prefix, so deleting/c/Batmancollected everything under/c/Batman Beyondtoo. Those comics were deleted with no paired add to rescue them, cascading their bookmarks away while the files were still on disk. Prefixes now terminate at a separator.Sibling libraries stole each other's events.
_find_libraryhad the same bare-prefix bug, so with roots/comicsand/comics-kids(a pair the admin serializer permits — it only rejects nesting) the second library's events were filed under the first, importing comics at paths outside their own library.Two
TypeErrors in the scribe priority queue.SHUTDOWN_MSGwas a bare int where real items are(priority, timestamp)tuples, so stopping the thread with any task queued raised comparing int to tuple and aborted the daemon's shutdown loop before the remaining threads were told to stop. Separately, equal priorities fell through to comparingScribeTaskdataclasses, which define no ordering — a lost task in the routing thread. Timestamps tie more readily than they look (truncated, and clocks can step backwards), so a monotonic counter now closes the tuple.Online-tag merge never deduped.
_merge_tasktested candidate paths againstpath_to_pk's values, which are pks — aPathnever equals anint, so an overlapping second scan re-queued every shared comic: inflated totals and duplicate lookups against rate-limited sources.The settle-timeout warning was misleading. It told the admin to poll again once copying finished, implying the task was abandoned, while the import ran on regardless — and skipped starting its statuses on the way out.
Notes for review
FailedImportand are retried.COMICBOX_RENAME_CONFIGis separate fromCOMICBOX_CONFIGso read/import parsing is untouched — this only widens what the rename pass parses.TagWriterRenameExtensionTestsrepacks the example CBZ as a CBT and runs real comicbox — including one test asserting the admin preview equals what the rename produces. Every new test was verified to fail against the old code.make lint,make ty, full pytest and vitest all pass.