fix(metadata): keep edits made during ABS merges, catalogue sync and author refresh (#2926)

[vavallee]

Summary

These are the follow-ups #3008 left. Four more paths fetched from a metadata provider and then wrote the whole book or author row from a snapshot taken before the call, so an edit saved during the call was silently undone. Each one now writes guarded on updated_at, and each has its own answer for what happens when the guard loses.

Refs #2926 (closed by #3008).

Implementation notes

Path	Before	After	When the guard loses
abs/import_upserts.go mergeUpstreamBook (also from matchHardcoverSeries)	full BookRepo.Update after the ISBN/ASIN/title lookup or the Hardcover series fetch	merge planned on a copy, written with BookRepo.UpdateIfUnchanged	retries once: the upstream record is already in hand, so it is merged again onto a fresh read. A second loss drops the merge for this import with an INFO log and a run message, and the next import retries. Either way book ends up holding the stored row.
abs/import_upserts.go enrichAuthor → new mergeUpstreamAuthor	full AuthorRepo.Update after the upstream author search and GetAuthor	merge planned on a copy, written with AuthorRepo.UpdateIfUnchanged	same policy as the book merge: one retry on a fresh read (the ABS name match is checked again, since an edit may have renamed the author), then dropped for this import with an INFO log and a run message.
api/authors.go runCatalogueSync, seenTitles branch (calibre stub upgrade, dual format merge, same format ratings)	full Update of the row read by ListByAuthorIncludingExcluded at sync start	refreshTitleMatch re-reads the row right before the change, then writeTitleMatch writes with UpdateIfUnchanged guarded on that read	the change is skipped (INFO) and the row is reloaded. The work's ids and series link are not recorded either, so the next sync still matches it by title and retries the change instead of resolving it through the id branch, which never relinks a stub or widens the format. A row deleted or excluded during the sync is left alone.
scheduler.refreshMetadata	full AuthorRepo.Update of the author listed at the start of the run	each author is re-read just before its GetAuthor, then written with the new AuthorRepo.UpdateIfUnchanged	skipped with an INFO log, and the job runs again on schedule. Because of the re-read, an edit saved earlier in a long run does not cost that author its refresh.

• Bookkeeping in the ABS merges. The book merge records conflict rows and, on a relink, keeps the old ABS id as a book identifier. The author merge records conflict rows, a variant alias for a rename, and the upstream id for an author whose identity is not replaceable. All of these used to be written before the row. applyConflictField is replaced by planConflictField, which applies the value but does not write the conflict. Both merges collect their bookkeeping and record it only after UpdateIfUnchanged returns true (or when the merge needs no write at all), and the author matcher only learns a row that was written. So a dropped merge leaves nothing behind for a change that never landed, and a retried merge records the values it actually wrote. The one exception is the ABS id a book relink replaces: it is written before the row, so a crash in between cannot leave a row no ABS key lookup finds (ABS importer matches by title, so retitling an item in ABS + re-import mints a duplicate book record — including 'wanted' twins of owned books #1691). If the write then loses, that identifier still names the row's current id.
• Deleted rows. A book or author deleted during the upstream lookup is skipped with an INFO log and a run message instead of failing the item.
• What the ABS merge protects. Fields the merge does not take from upstream (monitoring, narrator, format, profiles) keep the user's edit. Fields it does take (book description, cover, original title, release date, language, ratings; author name, description, image, disambiguation, sort name) are merged again on the retry, so a cover saved during the lookup can still be replaced under the usual conflict rules. That is not a regression, and the wiki and changelog say so.
• AuthorRepo.UpdateIfUnchanged mirrors the book method: the same full row write plus AND CAST(updated_at AS TEXT)=?. When the guard loses it writes nothing, not even the author_identifiers upsert, and it does not stamp the caller's struct. Update is now a thin wrapper over the shared helper, with its SQL unchanged. models.Author gains UpdatedAtRaw, read via CAST(updated_at AS TEXT) appended to authorSelectCols/authorSelectColsA. The cast is needed because the driver reformats a DATETIME it can parse, so the plain column cannot reproduce the stored text. A test pins this on a legacy 2026-09-01 10:00:00 row.
• Author updated_at audit. I checked every UPDATE authors in non test Go code. The full row update, UpgradeSyntheticDNB and the four alias merge setters all bump updated_at. Three statements do not: the sort_key/name_sort_key backfill in db.go (runs at migration time), last_discovery_at, and catalogue_populated_at. None of them is a user edit, and the full row update writes none of those columns, so no data is lost in either direction. All handler and importer author writes go through Update.
• No transaction is held across HTTP. Each guarded write runs after the provider call returns, as a single statement (for authors, one short transaction together with the identifier upsert).

Checklist

• Commits signed off with git commit -s
• Tests added or updated
• Doc-update gate cleared (docs/User-Guide-Wiki.md, paragraph after the fix(metadata): guard audnex enrichment writes against concurrent edits (#2926) #3008 note)
• Added a changelog fragment under changelog.d/ (concurrent-overwrite-followups.md)

Test plan

Fail before: I ran each new test with only the test files applied on top of origin/main source.

Test	On main
abs TestMergeUpstreamBookRetriesOnTopOfConcurrentEdit	FAIL: monitored=true narrator="" (edit overwritten)
abs TestMergeUpstreamBookSkipsWhenTheBookKeepsChanging	FAIL: caller row not refreshed, and the relink plus its bookkeeping reported for a write that did not land
abs TestMergeUpstreamAuthorRetriesOnTopOfConcurrentEdit	FAIL: concurrent unmonitor overwritten (run against the first commit's author merge)
abs TestMergeUpstreamAuthorSkipsWhenTheAuthorKeepsChanging	FAIL: caller row not refreshed, relink reported for a write that did not land
scheduler TestRefreshMetadata_PreservesEditDuringProviderCall	FAIL: monitored=true image="https://upstream.test/a.jpg"
api TestRefreshAuthorBooks_TitleMatchKeepsEditMadeDuringSync	FAIL: monitored=true image=""
api TestRefreshAuthorBooks_LostTitleMatchIsRetriedNextSync	FAIL against the previous branch head: ids recorded after the lost write, and the second sync leaves foreignID="calibre:book:77"
abs TestMerge*SkipsWhen*IsDeletedDuringLookup	FAIL against the previous branch head: sql: no rows in result set

In each test the fake provider commits a user edit through the ordinary Update while it handles the request. That request is the ISBN lookup for the ABS book merge, GetAuthor for the ABS author merge, GetAuthor for the scheduler, and the MinPages edition prefetch for the catalogue sync. The ABS double loss tests use a test only RAISE(IGNORE) trigger on the relink write so that every attempt loses. TestMergeUpstreamBookPersistsWithoutConcurrentEdit and TestMergeUpstreamAuthorPersistsWithoutConcurrentEdit are the controls. TestAuthorRepoUpdateIfUnchanged covers the repo method: a legacy timestamp shape, no identifier written when the guard loses, and invalid snapshots rejected.

• gofmt -l internal cmd (clean)
• go vet ./...
• golangci-lint run ./... v2.11.4 (0 issues)
• go test -race -timeout 40m ./internal/db/ ./internal/abs/
• go test -timeout 40m ./internal/api/ ./internal/scheduler/ ./internal/models/
• New tests rerun after rebasing onto 8ecf7761
• Review fix commit: gofmt, vet, golangci-lint run ./... (0 issues), go test -timeout 40m ./internal/abs/ ./internal/db/ ./internal/scheduler/ ./internal/api/
• Author merge commit: gofmt, go vet ./internal/abs/, golangci-lint run ./... (0 issues), go test -timeout 40m ./internal/abs/ ./internal/db/ and the same with -race
• go test ./cmd/... ./internal/...: every package passed except internal/api, which hit the 30m timeout on a host with load average around 40. It passed on its own in the 40m run above.

🤖 Generated with Claude Code

https://claude.ai/code/session_016fJcCVbNnKsmj2MAAMwWj9

[codecov]

Codecov Report

❌ Patch coverage is 72.54098% with 67 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
internal/abs/import_upserts.go	75.83%	22 Missing and 14 partials ⚠️
internal/api/authors.go	48.38%	7 Missing and 9 partials ⚠️
internal/db/authors.go	85.36%	4 Missing and 2 partials ⚠️
internal/scheduler/scheduler.go	62.50%	3 Missing and 3 partials ⚠️
internal/abs/import_conflicts.go	57.14%	3 Missing ⚠️

📢 Thoughts on this report? Let us know!

[github-actions]

Clean. A few things I checked:

preserveRelinkedBookIdentifier runs before the guarded write (internal/abs/import_upserts.go, mergeUpstreamBook loop): intentional for the #1691 invariant — on a double-loss the leftover identifier is benign because it still names the current row id. That's documented and correct.

upsertIdentifierTx is gated on the guard (internal/db/authors.go, update): the early return false, nil on n == 0 is before the upsertIdentifierTx call, so a lost guard leaves no identifier row behind. Matches BookRepo.

Trigger SQL in tests (internal/abs/import_upserts_test.go:718, internal/api/authors_test.go:1317): string-interpolates mergeRaceUpstreamID / "OL-RETRY-W" into a CREATE TRIGGER. These are compile-time constants in test files, not user input — fine.

writeTitleMatch reload on a lost guard (internal/api/authors.go:1095): the reload is best-effort; a failure is warned and false is returned. The lostGuard flag then prevents the stale row from being used downstream, so the only consequence is the sync will see a stale snapshot in seenTitles for any subsequent title with the same key in the same run — which can't happen in practice (one entry per title).

All four paths have first-loss and second-loss tests with real in-memory DB state, plus controls. TestAuthorRepoUpdateIfUnchanged pins the legacy timestamp shape. No blocking findings.

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