fix(watcher): don't delete a library that is only unmounted - #827
Merged
Conversation
A dropped network share, an ejected volume, or a docker bind mount that didn't come up presents as an empty or missing directory rather than an error, so every comic in the library looks deleted at once. Acting on that removes every row and cascades away every bookmark and reading position in the library — for files that are perfectly fine and will be back as soon as the mount is. The poller has refused to scan a library in that state for a long time, by three separate checks. The watcher had none: it already holds the events, so it deleted. The delete phase's existence check can't help either, because while the mount is gone the files genuinely are unreachable. The watcher now consults the same checks before acting on any task that carries deletes. Adds and modifies are left alone; they can't destroy anything. Those checks move to ``codex.librarian.fs.mounted`` so both scanners share one definition of what a vanished library looks like, rather than one of them growing a defense the other never hears about. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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.
The last of the rename/move audit. It is not the change the plan called for — I implemented that one, found it unsafe, and reverted it. Details below, because the reasoning matters more than the diff.
What shipped
A dropped network share, an ejected volume, or a docker bind mount that didn't come up presents as an empty or missing directory rather than an error, so every comic in the library looks deleted at once. Acting on that removes every row and cascades away every bookmark and reading position in the library — for files that are perfectly fine and will be back as soon as the mount is.
The poller has refused to scan a library in that state for a long time, by three separate checks. The watcher had none — it already holds the events, so it deleted. PR #825's existence check can't help here either: while the mount is gone the files genuinely are unreachable.
The watcher now consults the same checks before acting on any task carrying deletes. Adds and modifies are untouched — they can't destroy anything. The checks move to
codex.librarian.fs.mountedso both scanners share one definition of a vanished library instead of one of them growing a defense the other never hears about; the poller's three inline blocks collapse into one call.What I reverted, and why
The plan's PR E was to relax the poller's move-pairing so an external write-then-rename between two polls is recognized (audit H4 — the last confirmed high). I implemented it: same-directory + newer-mtime waives the size check, reasoning that one directory's entries share a filesystem so the cross-mount inode collision the size check defends against can't arise between those two paths.
Then I applied the same waiver to the watcher for the split-batch case (M4), and it failed an existing test —
test_reused_inode_with_different_size_is_rejected, which encodes exactly the scenario the waiver can't distinguish: a bulk conversion frees a comic's inode, an unrelated new archive in the same directory is handed it, and the pair is refused on size. My waiver paired them, because an unrelated new file always has a newer mtime.The poller has the same hazard. Its equivalent test passes only by the accident of equal mtimes in the fixture; its stated intent ("two unrelated files sharing an inode but with different sizes don't pair") is defeated the same way. And on ext4 — the common Docker deployment — inode allocation prefers the parent directory's block group, so same-directory reuse after a delete is likely, not exotic.
So the trade is:
The codebase already made this call for the watcher and wrote a test to hold the line. I don't think it should be quietly reversed for the poller, and stat alone offers nothing that separates the two cases: both are "same inode, same directory, different size, newer mtime".
H4 therefore stays open, and is worth recording as a known limitation rather than a bug in flight. The real fix is the audit's other suggestion — a deletion tombstone plus inode-based row adoption, so a comic re-imported at a new path can re-adopt the identity of one recently deleted with its inode. That verifies identity instead of guessing it, and would close H4, M4, M6 and the cross-library move at once. It is a feature, not a patch, so I left it out of this PR rather than smuggling it in.
Notes for review
_process_changes— so there is now a test that drives_process_changesend-to-end and does fail when the call is removed.make lint,make ty, full pytest and vitest all pass.make fixalso dropped a# noqa: PLR0911the poller no longer needs.