diff --git a/backend/AGENTS.md b/backend/AGENTS.md index 2da88cc..007ee1d 100644 --- a/backend/AGENTS.md +++ b/backend/AGENTS.md @@ -128,9 +128,18 @@ The policy differs by how deliberate the import is: | `create_many_from_existing_files` (consume watcher) | Skips, and **moves the file to `CHITAI_DUPLICATE_PATH//`** — nothing is deleted, and it cannot stay put because `watchfiles` only reports additions. That path must stay outside `consume_path` or the watcher re-imports it and tries to read the directory name as a library slug. | `allow_duplicates=true` overrides all of it, on every endpoint. Keep that working — the -hash is not proof of identity, so a false positive has to be recoverable. +hash is not proof of identity, so a wrong verdict has to be recoverable, and a scripted +import needs a way through. The **web UI deliberately does not offer it**: storing the +same bytes twice splits reading progress and shelf membership across two records that +can never converge, which is nothing anyone wants on purpose. -Two things to preserve when touching this code: +Three things to preserve when touching this code: + +- **A match only counts while the file is on disk.** `find_duplicate_files` stats each + candidate, and `add_files` writes a missing file back into the row that already + describes it (`_restore_file`) instead of adding a second row beside it. The hash + lives in the database and the file does not, so without this a file deleted behind + the app's back would go on refusing its own replacement. - **Screening runs before anything is written.** `fingerprint_upload` reads the spooled upload and rewinds it; the resulting fingerprints are handed to `_save_book_files`, diff --git a/backend/src/chitai/services/book.py b/backend/src/chitai/services/book.py index a37bba5..56a29e1 100644 --- a/backend/src/chitai/services/book.py +++ b/backend/src/chitai/services/book.py @@ -71,6 +71,9 @@ FileT = TypeVar("FileT", UploadFile, Path) # size makes a false match require both. Fingerprint = tuple[str, int] +# How much of a file is held in memory at a time while it is written to disk. +CHUNK_SIZE = 262144 # 256 KiB + @dataclass(frozen=True) class DuplicateFile: @@ -240,6 +243,11 @@ class BookService(SQLAlchemyAsyncRepositoryService[Book]): """ Look up which of the given fingerprints are already stored. + A row only counts while its bytes are still on disk. The hash lives in the + database and the file does not, so a file removed behind the app's back would + otherwise go on refusing its own replacement — telling the reader the library + holds something it cannot open. + Args: fingerprints: The `(hash, size)` pairs to look for. library: The library being imported into. The search is scoped to it @@ -262,6 +270,7 @@ class BookService(SQLAlchemyAsyncRepositoryService[Book]): Book.id, Book.title, Book.library_id, + Book.path, ) .join(Book, FileMetadata.book_id == Book.id) .where(tuple_(FileMetadata.hash, FileMetadata.size).in_(wanted)) @@ -276,17 +285,20 @@ class BookService(SQLAlchemyAsyncRepositoryService[Book]): rows = await self.repository.session.execute(statement) matches: dict[Fingerprint, DuplicateFile] = {} - for file_hash, size, path, book_id, title, library_id in rows: - matches.setdefault( - (file_hash, size), - DuplicateFile( - filename=Path(path).name, - hash=file_hash, - size=size, - library_id=library_id, - book_id=book_id, - book_title=title, - ), + for file_hash, size, path, book_id, title, library_id, book_path in rows: + if (file_hash, size) in matches: + continue + + if not book_path or not await aios.path.isfile(Path(book_path) / path): + continue + + matches[(file_hash, size)] = DuplicateFile( + filename=Path(path).name, + hash=file_hash, + size=size, + library_id=library_id, + book_id=book_id, + book_title=title, ) return matches @@ -810,10 +822,22 @@ class BookService(SQLAlchemyAsyncRepositoryService[Book]): fingerprints = await self._fingerprint_uploads(files) if not allow_duplicates: - # A file this book already carries is not a conflict — adding it again - # asks for a state that already holds, so there is simply nothing to do. - own = {(file.hash, file.size) for file in book.files} - files = [file for file in files if fingerprints[file.filename] not in own] + # A file this book already carries is not a conflict — adding it again asks + # for a state that already holds, so there is nothing to do. Unless its + # bytes have gone missing, in which case that state does not hold: the + # upload fills the existing row back in rather than earning the book a + # second row for the same file. + own = {(file.hash, file.size): file for file in book.files} + remaining = [] + + for file in files: + stored = own.get(fingerprints[file.filename]) + if stored is None: + remaining.append(file) + else: + await self._restore_file(book, stored, file) + + files = remaining known = await self.find_duplicate_files( (fingerprints[file.filename] for file in files), library @@ -834,6 +858,32 @@ class BookService(SQLAlchemyAsyncRepositoryService[Book]): book.files.extend(new_files) await self.update_book(book.id, {"files": [file for file in book.files]}, library) + @staticmethod + async def _restore_file( + book: Book, stored: FileMetadata, upload: UploadFile + ) -> None: + """ + Put one of a book's own files back on disk, if it is no longer there. + + Args: + book: The book the file belongs to. + stored: The row saying where the file should be. + upload: The uploaded copy of it. + """ + if not book.path: + return + + path = Path(book.path) / stored.path + if await aios.path.isfile(path): + return + + path.parent.mkdir(parents=True, exist_ok=True) + await upload.seek(0) + + async with aiofiles.open(path, "wb") as dest: + while chunk := await upload.read(CHUNK_SIZE): + await dest.write(chunk) + async def remove_files( self, book_id: int, file_ids: list[int], delete_files: bool, library: Library ) -> None: @@ -1090,7 +1140,6 @@ class BookService(SQLAlchemyAsyncRepositoryService[Book]): data["path"] = str(parent) file_metadata = [] - CHUNK_SIZE = 262144 # 256 KiB for file in data.pop("files", []): if TYPE_CHECKING: file: UploadFile diff --git a/backend/tests/unit/test_services/test_book_service.py b/backend/tests/unit/test_services/test_book_service.py index 8086d82..0052713 100644 --- a/backend/tests/unit/test_services/test_book_service.py +++ b/backend/tests/unit/test_services/test_book_service.py @@ -595,3 +595,48 @@ class TestBookPathCollisions: assert len(book.files) == 2 for file in book.files: assert (Path(book.path) / file.path).is_file() + + +@pytest.mark.asyncio +class TestMissingFiles: + """A row whose bytes are gone must not stand in for the file itself.""" + + async def test_a_missing_file_is_not_a_duplicate( + self, books_service: BookService, test_library: m.Library + ) -> None: + """Otherwise the library refuses to take back a file it can no longer open.""" + created = await books_service.create_many_from_files( + BooksCreateFromFiles(files=[upload(EPUB)]), test_library + ) + book = await books_service.get(created.books[0].id) + (Path(book.path) / book.files[0].path).unlink() + + result = await books_service.create_many_from_files( + BooksCreateFromFiles(files=[upload(EPUB)]), test_library + ) + + assert len(result.books) == 1 + assert result.duplicates == [] + + restored = await books_service.get(result.books[0].id) + assert (Path(restored.path) / restored.files[0].path).is_file() + + async def test_re_adding_a_missing_file_puts_it_back( + self, books_service: BookService, test_library: m.Library + ) -> None: + """The book already has a row for it, so the bytes go back where it says.""" + created = await books_service.create_many_from_files( + BooksCreateFromFiles(files=[upload(EPUB)]), test_library + ) + book_id = created.books[0].id + + book = await books_service.get(book_id) + path = Path(book.path) / book.files[0].path + path.unlink() + + await books_service.add_files(book_id, [upload(EPUB)], test_library) + + book = await books_service.get(book_id) + assert len(book.files) == 1 + assert path.is_file() + assert path.read_bytes() == EPUB.read_bytes()