[kevinatlee]

Summary

• add an explicit More → Reconcile catalogue… preview/apply flow for one author; normal metadata refresh remains non-destructive
• compare local rows with a fresh effective catalogue (primary provider plus configured supplements) and the current metadata profile, reporting candidates, protection counts, indeterminate rows, and per-book reasons
• let users deselect individual preview rows and submit only the chosen IDs; every candidate starts selected
• treat partial primary or supplemental results conservatively: absence is never deletion evidence after a provider failure, and unresolved language/edition evidence is kept
• recompute the preview on apply, intersect it with the requested IDs, and repeat every safeguard in one guarded database delete
• preserve imported/owned books and any row with filePath, ebookFilePath, audiobookFilePath, or a book_files record; no filesystem deletion is involved
• document the UI/API behavior and add a changelog fragment

The branch is rebased onto current main, including the merged Hardcover language fix from #2246. Reconciliation remains a separate explicit operation and does not make refresh destructive. Refs #2208; the separate series-fill behavior described there is intentionally outside this PR.

Suggested review order

1. internal/db/books.go and internal/api/catalogue_reconciliation.go — deletion boundary and apply-time safeguards
2. internal/metadata/aggregator_author_works.go and OpenLibrary completeness reporting — conservative catalogue evidence
3. internal/api/catalogue_reconciliation_test.go and internal/db/books_reconciliation_test.go — destructive/error-path coverage
4. web/src/components/CatalogueReconciliationModal.tsx — explicit preview and per-row selection
5. route wiring, API docs, user guide, and changelog fragment

Checklist

• Commits signed off with git commit -s — see Sign your work
• Tests added or updated
• docs/DEPLOYMENT.md updated if env vars, config, or upgrade path changed (not applicable: no deployment/config change)
• Added a changelog fragment under changelog.d/ (not an edit to CHANGELOG.md) — see changelog.d/README.md
• Wiki pages updated if user-facing behaviour changed

Test plan

• go test -timeout=30m ./cmd/... ./internal/...
• reconciliation API race tests, -count=10
• guarded DB deletion race tests, -count=10
• focused coverage: 100% statements for every function in catalogue_reconciliation.go and for DeleteMetadataOnlyWantedByIDs
• go vet ./...
• go build ./...
• golangci-lint run ./... (0 issues)
• pinned govulncheck ./... (no called vulnerabilities)
• frontend typecheck, lint, production build, and full test suite (67 files / 657 tests)
• real-binary HTTP smoke suite (go test -count=1 -timeout=60s ./tests/smoke/...)

The full backend run used TMPDIR=/private/tmp on macOS so the existing rTorrent deletion safety test sees a direct path instead of the /var → /private/var system symlink.

[codecov]

Codecov Report

❌ Patch coverage is 94.10188% with 22 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
internal/metadata/aggregator_author_works.go	64.86%	8 Missing and 5 partials ⚠️
internal/db/book_identifiers.go	70.00%	3 Missing and 3 partials ⚠️
internal/metadata/openlibrary/client.go	91.17%	1 Missing and 2 partials ⚠️

📢 Thoughts on this report? Let us know!

[vavallee]

Read the whole thing and ran it locally. This is careful work, and the deletion boundary in particular is built the way I'd want it built: ownership checked before anything else, apply recomputes the preview and intersects rather than trusting the client's ids, and every predicate repeated inside the SQL so an import that lands between preview and apply wins the race. The route sits next to DELETE /author/{id}, which is the right precedent for gating. Thanks for the suggested review order too, it made this much faster to go through.

Two things I want changed, then one limitation I think needs saying out loud rather than fixing.

1. An empty ForeignID is treated as a real identifier

buildCatalogueReconciliation keys both maps on the provider work's id:

acceptedIDs[work.ForeignID] = struct{}{}   // line 270
rejectedIDs[work.ForeignID] = reason       // line 278

and then looks a local book up by its own ForeignID plus its book_identifiers rows (lines 332 to 339). Nothing filters an empty string on either side, so "" becomes a shared key. You already guard for this exact case in the editions fan-out at line 242, so I think it's just an oversight rather than a decision.

I wrote a throwaway test against your branch to be sure. A provider work with no id, rejected on language, plus a local Wanted book with no id:

candidates=[{BookID:1 Title:A Locally Created Book Reason:language_not_allowed}]

The local book is offered for deletion under a reason it never earned. The mirror case also reproduces: an accepted empty-id work grants an unrelated empty-id local book a spurious Kept.

Reachability is genuinely narrow, and I want to be fair about that: books.foreign_id is TEXT NOT NULL UNIQUE, so at most one such row can exist in a database, and I could not find a creation path that writes one. So this is latent rather than something users will hit next week. I still want it closed, because it is a one line guard on a code path whose failure mode is deleting the wrong row, and because "no current path creates one" is a property of today's code rather than a property this function enforces.

2. ListBookIdentifiers runs once per local book

Line 332 issues a query per row inside the loop. On a 500 book author that's 500 round trips, and with SetMaxOpenConns(1) (#2147) they serialize behind every other reader. A single fetch keyed by book id before the loop would be a small change and keeps preview latency flat.

3. Provider call volume, and a limitation worth documenting

I measured this rather than guessing. A 300 work author with minPages set costs 300 GetEditions calls per preview at concurrency 4. Apply re-fetches the catalogue but reuses cached editions, so it is 2 catalogue fetches and 300 edition calls total rather than 600, which is better than I expected.

I'm not asking you to change it: it's the same RunBounded(..., authorAutoSearchConcurrency, ...) shape already used at internal/api/authors.go:330, so it is consistent with the codebase. Worth knowing that it lands on the providers we just had to add throttling to in #2277.

The one I do want said explicitly: searchAuthorWorks requests limit=200, and GetAuthorWorksSnapshot returns complete only when both halves are complete. So for any OpenLibrary author with more than 200 works, ProviderComplete is false permanently, the warning banner always shows, and reconciliation can never remove a not_in_current_catalogue row. That is the safe direction and I would not want it inverted. But it means the feature's headline capability quietly does not apply to exactly the prolific authors whose catalogues are worst, and a user staring at the banner has no way to know it will never clear. A sentence in the user guide saying so would be enough.

Not your problem

The red licenses check is infrastructure. go install github.com/google/go-licenses@v1.6.0 failed on sum.golang.org returning an HTTP/2 INTERNAL_ERROR while verifying a genproto go.mod, so the drift comparison never ran. Your branch touches no go.mod, go.sum, package.json or THIRD_PARTY_LICENSES.md. I'll re-run it.

Fix 1 and 2 and I'm happy to merge this.

[kevinatlee]

Addressed in b0d43b5 after rebasing onto current main:

• Blank or whitespace-only provider and local IDs are no longer indexed or matched during reconciliation. Added focused regressions for both the rejected-empty-ID false candidate and accepted-empty-ID false kept-match cases.
• Reconciliation now fetches all identifiers for the author in one query and indexes them by book ID before the local-book loop. Added a DB test covering grouping and author scoping; the existing cross-provider alias behavior remains covered.
• The user guide now states that OpenLibrary searchAuthorWorks uses limit=200, so authors over 200 works remain partial, the warning may remain, and not_in_current_catalogue rows stay protected.

The branch is rebased onto current upstream main. All CI checks are green, including lint, Go/frontend validation, smoke, licenses, patch coverage, and all six race shards.

[vavallee]

All three land. Checked locally rather than taking CI's word for it.

Empty and whitespace ids are now out of acceptedIDs, rejectedIDs and indeterminateIDs, and out of the per book ids slice. I reverted just those four guards on a scratch copy and both new tests fail there, so they are real regressions rather than tests that would have passed before the fix.

ListBookIdentifiersByAuthor is scope equivalent to the loop it replaces: ListByAuthorIncludingExcluded selects on WHERE author_id = ?, and the new query joins books b ON b.id = bi.book_id WHERE b.author_id = ?. Same book set, same columns, same per book ordering, one query instead of one per book. Moving the error context from the book id to the author id is the right call given there is no longer a per book call to attribute it to.

The 200 work ceiling being written down is what I wanted. That is the honest description of the behaviour until the OpenLibrary side paginates.

Branch builds and vets clean here, and go test ./internal/api/... ./internal/db/... is green.

Approving. The regression tests that actually fail without the fix are the part most PRs skip, so thanks for doing it properly.