Skip to content

fix(watcher): don't delete a library that is only unmounted - #827

Merged
ajslater merged 2 commits into
developfrom
fix/poller-write-then-rename
Aug 26, 2026
Merged

fix(watcher): don't delete a library that is only unmounted#827
ajslater merged 2 commits into
developfrom
fix/poller-write-then-rename

Conversation

@ajslater

Copy link
Copy Markdown
Owner

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.mounted so 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 testtest_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:

  • Without the waiver: an external write-then-rename loses bookmarks. Deterministic, but the loss is missing data — the comic is still there, just unread.
  • With it: an inode reuse re-paths one comic's row onto another comic's file. Rarer, but the result is wrong data — your reading position silently attached to a different book, and no scan ever corrects it.

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

  • The new tests were each checked against a deliberately broken implementation. Worth knowing: my first four covered the predicate and still passed with the guard unwired from _process_changes — so there is now a test that drives _process_changes end-to-end and does fail when the call is removed.
  • make lint, make ty, full pytest and vitest all pass. make fix also dropped a # noqa: PLR0911 the poller no longer needs.

ajslater and others added 2 commits August 25, 2026 20:29
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>
@ajslater
ajslater merged commit 86e83d4 into develop Aug 26, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant