Skip to content

fix(db): report SetFilePath load errors instead of a legacy write (#2819) - #2824

Merged
vavallee merged 1 commit into
mainfrom
fix/2819-setfilepath-errors
Sep 27, 2026
Merged

vavallee merged 1 commit into
mainfrom
fix/2819-setfilepath-errors

Conversation

@vavallee

Copy link
Copy Markdown
Owner

Closes #2819

Summary

BookRepo.SetFilePath fell back to a bare UPDATE books SET file_path, status on any GetByID failure. That dropped the load error, marked the book imported with no book_files row and no refreshBookStatus, and returned nil for a missing book (0 rows matched).

History: the fallback arrived with dual format support (0e0589c, #23) as a guard around the old single column write. book_files (3322c58, #343) made it obsolete. For a missing row it was a no op that claimed success; for an unreadable row it wrote state the rest of the system no longer reads from. There is no genuine case left, so it's removed:

  • a GetByID error is returned, wrapped as load book N: ...
  • a missing book returns book N not found

The one production caller, handleNewWantedBook, already logs a returned error as authors: record existing file path with book_id, so these now show up in the log instead of vanishing. Its behaviour otherwise is unchanged (it still returns fileFound=true and skips the auto search).

Overlap with #2794: that PR edits this same fallback (adds updated_at, switches to r.exec) and adds TestBookRepoUpdateHydratedMetadataLegacyUpdatedAtShapes/*/SetFilePathFallback, which expects SetFilePath to succeed on a corrupt row. Whichever lands second needs to drop that hunk and that subcase; with this change there is no fallback write for hydration to observe.

How it was verified

New internal/db/books_setfilepath_test.go uses the technique from the issue (calibre_id='corrupt', plus a missing id). On origin/main:

=== RUN   TestSetFilePathReportsLoadErrors/unreadable_book
    books_setfilepath_test.go:43: SetFilePath returned nil although the book could not be loaded
    books_setfilepath_test.go:51: row was written behind book_files: status="imported" file_path="/books/unreadable.epub", want "wanted" and empty
=== RUN   TestSetFilePathReportsLoadErrors/missing_book
    books_setfilepath_test.go:59: SetFilePath returned nil for a book that does not exist
--- FAIL: TestSetFilePathReportsLoadErrors (0.09s)

Passes after the fix. Mutation checks (both mutants compile):

  • if err != nil changed to if false && err != nil: unreadable_book fails, the error becomes book 1 not found and no longer carries the scan error
  • return fmt.Errorf("book %d not found", id) changed to return nil: missing_book fails

go build ./... && go vet ./... clean; go test ./cmd/... ./internal/... exit 0.

🤖 Generated with Claude Code

https://claude.ai/code/session_016fJcCVbNnKsmj2MAAMwWj9

)

SetFilePath fell back to a bare UPDATE of books.file_path and status on
any GetByID failure. That discarded the load error, marked the book
imported without a book_files row or refreshBookStatus, and returned nil
for a missing book because the UPDATE matched no rows.

The fallback came with dual format support (#23) as a guard around the
old single column write. Once book_files became the source of truth
(#343) it no longer had a case to serve: a missing row made it a no op
and an unreadable row got a write the rest of the system does not read.
Remove it; return the wrapped load error, and a not found error for a
missing book. The only production caller, handleNewWantedBook, already
logs a returned error with the book id.

Reported by magrhino.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016fJcCVbNnKsmj2MAAMwWj9
Signed-off-by: vavallee <vavallee@protonmail.com>

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fix is correct and the test coverage is solid.

Logic
GetByID returns (nil, nil) for a missing row (books.go:461-462) and (nil, err) for a scan failure — the two separate guards now handle both correctly. The old collapsed check fell back to a write that was a no-op for missing rows (reported success on 0 rows matched) and silently bypassed book_files + refreshBookStatus on scan errors. Both cases are now surfaced.

Tests
internal/db/books_setfilepath_test.go covers both failure modes, verifies the wrapped error carries the original scan error, and additionally asserts no dirty state was written to books.status / file_path. The precondition check on GetByID at line 40-43 is a good guard — the test fails loud if the corrupt-row behaviour ever changes.

One flag: conflict with #2794
The PR body calls this out, but worth repeating: #2794 touches the same fallback block (adds updated_at, switches to r.exec) and adds a subcase SetFilePathFallback under TestBookRepoUpdateHydratedMetadataLegacyUpdatedAtShapes that expects the fallback to succeed on a corrupt row. If #2794 lands after this one, that subcase will fail and the hunk needs to be dropped. No blocker — just needs a rebase on whichever merges second.

Otherwise clean.

— 🤖 Bindery triage bot (automated). Reply to correct me; a human will see it.

@vavallee
vavallee merged commit 64095b0 into main Sep 27, 2026
42 checks passed
@vavallee
vavallee deleted the fix/2819-setfilepath-errors branch September 27, 2026 19:32
@codecov

codecov Bot commented Sep 27, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

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.

fix(db): SetFilePath swallows GetByID errors and bypasses book_files in its fallback

1 participant