Skip to content

fix(metadata): guard audnex enrichment writes against concurrent edits (#2926) - #3008

Merged
vavallee merged 2 commits into
mainfrom
fix/2926-audnex-concurrent-edits
Oct 5, 2026
Merged

vavallee merged 2 commits into
mainfrom
fix/2926-audnex-concurrent-edits

Conversation

@vavallee

@vavallee vavallee commented Oct 5, 2026 •

Copy link
Copy Markdown
Owner

Summary

Three paths made an Audnex call and then wrote the whole book row from the snapshot taken before it, so an edit saved during the call (unmonitor, new cover, narrator fix) was silently undone. They now use the same updated_at guard #2794 added for hydration. Rebind had the same shape around its provider fetch and is fixed too.

Closes #2926.

Implementation notes

Path Before After When the guard loses
abs/importer.go enrichAudiobookFromASIN (background) full Update UpdateHydratedMetadata(book, snapshot.UpdatedAtRaw) enrichment dropped, row reloaded so the rest of the import (series, editions, owned state, rollback snapshot) works from the edit; DEBUG log (the next ABS import enriches again)
hardcoverlistsyncer enrichAudiobook (background) full Update UpdateHydratedMetadata enrichment dropped and row reloaded; INFO log, because list sync skips tracked books later and never retries it. On the create path the reloaded row is what the queued search sees; the widen path queued its copy before enriching
api/books.go EnrichAudiobook (manual) full Update UpdateHydratedMetadata, or UpdateIfUnchanged when the ASIN map rewrote the book's identity retries once from a fresh read (ownership and audiobook/ASIN checks re-run); Audnex answers are cached in the aggregator, so the retry normally costs no second request. A second loss returns 409 "This book changed while it was being enriched. Try again."
api/books.go Rebind (manual) full Update after GetBookFromProvider UpdateIfUnchanged 409 "This book changed while it was being rebound. Try again." No retry, since the provider fetch is not cached. Both new 409s carry reason: "changed"; RebindModal previously turned any 409 without force_required into the duplicate message ("This metadata record conflicts with an existing book"), so it now branches on reason and shows bookRebind.changed (en.json only). The duplicate foreign ID 409 is unchanged.
  • BookRepo.UpdateIfUnchanged is the guarded sibling of Update: the same full-row write with AND CAST(updated_at AS TEXT)=?, comparing stored text like UpdateHydratedMetadata so legacy timestamp shapes still match. On a lost guard it writes nothing and doesn't reload b. Update is now a thin wrapper over the shared helper. Its SQL and behaviour are unchanged.
  • The manual enrich path only writes the whole row when the ASIN map actually changed the identity (tryMapAudiobookMetadataByASIN now returns whether it did). Otherwise only the enrichment columns are written.
  • Same caveat as fix(metadata): preserve concurrent edits during hydration (#2758) #2794: a writer that does not bump updated_at is invisible to the guard (fix(db): SetFilePath fallback changes status without bumping updated_at #2793).

Follow-ups (not in this PR)

Other fetch-then-full-Update paths found by grepping every BookRepo.Update call site. Each one needs its own decision about what a lost guard means, so I left them out:

  • abs/import_upserts.go mergeUpstreamBook (line ~573). It snapshots after applyBookFields, then runs the upstream lookup (lookupUpstreamBook, ISBN/search) and the merge. It is also reached from matchHardcoverSeries (import_hardcover_series.go:158) after SearchSeries/GetSeriesCatalog. The merge records conflicts and identifiers, so a lost guard needs a decision about those.
  • api/authors.go runCatalogueSync, the seenTitles dedup branch (lines ~2529/2557/2568). Rows come from ListByAuthorIncludingExcluded at the start of the sync and are written after cover enrichment, edition fetches and earlier iterations' hydration, so the window is the whole catalogue loop.
  • Not books: scheduler.refreshMetadata does meta.GetAuthor then a full authors.Update, which is the same bug on the authors table.

Checked and not affected (no network between the read and the write): BookHandler.Update, the bulk setters, applyGenresToBooks, add book core, queue removal, ABS conflict choice, adoptDirectInsertMatch, upsertManualBook, ABS and Calibre applyBookFields, both rollbacks, refreshBookStatus, and the list syncer's widen Update.

Checklist

  • Commits signed off with git commit -s
  • Tests added or updated
  • Enrich button checked: it shows the server's error text for any failure, so its 409 already reads sensibly; a vitest now pins that
  • Doc-update gate cleared (docs/User-Guide-Wiki.md paragraph next to the fix(metadata): preserve concurrent edits during hydration (#2758) #2794 hydration note)
  • Added a changelog fragment under changelog.d/ (2926-audnex-concurrent-edits.md)

Test plan

Fail before (each test run against origin/main code, with only the new tests applied):

Test On main
abs TestEnrichAudiobookFromASINPreservesConcurrentEdit FAIL: monitored=true narrator="Audnex Narrator" image="https://audnex.test/cover.jpg"
hardcoverlistsyncer TestSync_EnrichmentPreservesConcurrentBookEdit FAIL: same overwrite
api TestEnrichAudiobook_RetriesAfterConcurrentEdit FAIL: monitored=true image="https://audnex.test/cover.jpg"
api TestEnrichAudiobook_MappedRetriesAfterConcurrentEdit FAIL: concurrent unmonitor overwritten
api TestEnrichAudiobook_ConflictWhenEditsKeepLanding FAIL: 200 instead of 409
api TestRebind_ConflictWhenBookChangesDuringFetch FAIL: 200 instead of 409

The fake Audnex (AudnexBookClient stub, or the list syncer's enricher interface) commits a user edit through BookRepo.Update while it handles the request. Controls (...PersistsWithoutConcurrentEdit, TestEnrichAudiobook_PersistsAudnexFields) confirm the guarded write still lands when nothing races it. TestBookRepoUpdateIfUnchanged covers the new repo method on a legacy CURRENT_TIMESTAMP shaped row.

  • gofmt -l internal/ cmd/ (clean)
  • go vet ./...
  • golangci-lint run ./... v2.11.4 (0 issues)
  • go test -race on ./internal/db/, ./internal/abs/, ./internal/hardcoverlistsyncer/ (ok), and ./internal/api/ -run 'TestEnrichAudiobook|TestRebind|TestBook' (ok). The whole api package under -race hit a 45m timeout on this machine with no failures or races before it; left to CI's sharded race job.
  • go test -count=1 ./cmd/... ./internal/... (all ok)
  • After rebasing onto 2eeba271 (main had touched internal/db/books.go): go build ./..., and go test on db, abs, hardcoverlistsyncer, importer and the api enrich/rebind/book tests (all ok)
  • cd web && npm run lint (0 errors; 7 warnings, all in untouched files) && npm run typecheck && npm run build
  • npx vitest run src/components/RebindModal.test.tsx src/pages/BookDetailPage.test.tsx (127 passed, including the new changed-book rebind case and the enrich 409 case)
  • After the UI commit: gofmt, go vet and go test on internal/api and internal/hardcoverlistsyncer, golangci-lint on both (0 issues)

🤖 Generated with Claude Code

https://claude.ai/code/session_016fJcCVbNnKsmj2MAAMwWj9

vavallee and others added 2 commits October 5, 2026 00:44
#2926)

Three paths made an Audnex call and then wrote the whole book row from
the snapshot taken before it, so an edit saved during the call was lost:
the ABS importer, the Hardcover list syncer and the manual enrich
endpoint. They now write through the same updated_at guard hydration
uses since #2794.

The background paths use UpdateHydratedMetadata, which writes only the
enrichment columns and, on a lost guard, reloads the row so the rest of
the import or sync works from the user's edit. The manual endpoint
retries once from a fresh read (the Audnex answer is cached, so this
normally costs no second request) and returns 409 if the row changes
again. When the endpoint also maps the book onto the canonical work for
its ASIN it needs the whole row, so BookRepo gains UpdateIfUnchanged,
the guarded sibling of Update.

Rebind had the same shape around its provider fetch and now uses
UpdateIfUnchanged too, answering 409 on a lost guard.

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>
…cts (#2926)

The rebind modal turned every 409 without force_required into "This
metadata record conflicts with an existing book", so a user whose book
was edited during the rebind was told the target id was taken. Both new
409s (rebind and enrich) now carry reason "changed", and the modal shows
its own string for it. The enrich button already showed the server's
message; a test now pins that.

List sync logs a dropped enrichment at INFO, since it is not retried on
later syncs, and the comment no longer claims the reloaded row reaches
the widen path's queued search.

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>
@vavallee
vavallee marked this pull request as ready for review October 5, 2026 04:33

@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.

Reviewed the guard mechanics end-to-end. No blocking defects.

Correctness

  • UpdateHydratedMetadata guard miss → ReloadHydratedBook reloads b in-place (consistent with #2794). Both background callers (enrichAudiobookFromASIN, enrichAudiobook) correctly use b after the call. internal/abs/importer.go:49 comment is accurate.
  • UpdateIfUnchanged intentionally does not reload b on miss. Both callers (Rebind, EnrichAudiobook) return immediately on !updated without touching b again, so there's no stale-pointer hazard. Worth a note in the godoc for future callers, but not a bug now.
  • The retry loop in EnrichAudiobook (internal/api/books.go:218) is a for attempt := 0; ; attempt++ with all exits as explicit returns. Max two iterations, correct. Slightly unusual idiom but not wrong.
  • Ownership re-checked on the fresh read at internal/api/books.go:266 — correct.
  • The tryMapAudiobookMetadataByASIN → UpdateIfUnchanged vs UpdateHydratedMetadata branch is correct: the bool return distinguishes "identity rewritten" (needs full-row write) from "enrichment only" (partial-column write).

Security

No new surface. The 409 reason: "changed" field is safe to expose. The tenancy check on the reloaded book (auth.CheckOwnership) mirrors the pattern used throughout internal/api/.

Tests

Controls (PersistsWithoutConcurrentEdit) plus regression cases for each path. TestBookRepoUpdateIfUnchanged covers the legacy timestamp shape. TestEnrichAudiobook_ConflictWhenEditsKeepLanding verifies the second miss is a 409, not a 200.

Nit

internal/db/books.go:670 godoc for UpdateIfUnchanged could say explicitly that b is not reloaded on a missed guard, to match the equivalent note UpdateHydratedMetadata implies through ReloadHydratedBook. Low stakes given the tests, but a future caller scanning the signature will miss that distinction.

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

@vavallee
vavallee merged commit 87ebea6 into main Oct 5, 2026
39 of 40 checks passed
@vavallee
vavallee deleted the fix/2926-audnex-concurrent-edits branch October 5, 2026 04:58
vavallee added a commit that referenced this pull request Oct 6, 2026
…author refresh (#2926) (#3038)

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

Three more paths fetched from a provider and then wrote a whole row back
from a snapshot taken before the call, silently undoing an edit saved in
between. #3008 guarded the Audnex and rebind paths; these are the
follow-ups it listed.

- ABS mergeUpstreamBook (also reached from matchHardcoverSeries): the
  merge is planned in memory and written with BookRepo.UpdateIfUnchanged.
  On a lost guard the upstream record already in hand is merged once more
  onto a fresh read; a second loss drops the merge for this import. The
  conflict rows and the relinked row's old identifier are recorded only
  after the row is written, so they always describe what was stored.
- Catalogue sync, title match branch: the row is re-read just before the
  change instead of trusting the start of sync snapshot, then written
  guarded on that read. A row deleted or excluded meanwhile is left alone.
- Scheduled author refresh: new AuthorRepo.UpdateIfUnchanged (same stored
  text updated_at guard). Each author is re-read before its lookup and the
  write is skipped, logged at INFO, when it loses; the job runs again on
  schedule. Authors now carry UpdatedAtRaw from CAST(updated_at AS TEXT).

Every UPDATE authors that a user edit or the refresh can race bumps
updated_at; the three that do not (sort key backfill, last_discovery_at,
catalogue_populated_at) touch columns the full row write does not.

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>

* fix(abs): guard the ABS author merge against concurrent edits (#2926)

enrichAuthor looked the author up upstream and then wrote the whole row
back with authors.Update, so an edit saved during the lookup (unmonitoring,
a new profile) was undone. It now follows mergeUpstreamBook's rules:

- the merge is planned on a copy and written with
  AuthorRepo.UpdateIfUnchanged on the row as read before the lookup;
- a lost guard merges the record already in hand once more onto a fresh
  read, re-checking that the ABS name still matches the author; a second
  loss drops the merge for this import and the next import retries it;
- the variant alias for a rename, the identifier for a non-replaceable
  identity and the conflict rows are recorded only after the write lands,
  and the matcher only learns a row that was written.

applyConflictField had no callers left, so planConflictField replaces it.

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>

* fix(metadata): address review of the concurrent edit guards (#2926)

- Catalogue sync: when a title match write loses its guard, skip
  recording the work's ids and its series link as well. Recorded ids
  sent the next sync down the id branch, which never relinks a calibre
  stub or widens the format, so the lost change was never retried.
- ABS book merge: keep the ABS id in the identity table before the
  relink write again, so a crash between the two cannot leave a row no
  ABS key lookup finds (#1691). A lost guard leaves an identifier that
  still names the row's current id.
- ABS book and author merges: a row deleted during the upstream lookup
  is skipped with an INFO log and a run message instead of failing the
  item with "sql: no rows".
- Move refreshTitleMatch and writeTitleMatch above runCatalogueSync's
  doc comment so godoc attaches it to the right function.
- Wiki and changelog: say which fields the ABS merge protects. Fields it
  takes from upstream (cover, description and the like) still follow the
  metadata conflict rules on the retry.

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>

---------

Signed-off-by: vavallee <vavallee@protonmail.com>
Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
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.

Three Audnex enrichment paths still overwrite concurrent book edits

1 participant