Skip to content

GetBookByISBNWithOutcome hands every caller the cached book and keys it without provider scope #2869

Closed

Description

@magrhino

Impact

Latent, the same class of bug as #2809, on a lookup #2809 does not list. Aggregator.GetBookByISBNWithOutcome (internal/metadata/aggregator.go) stores the enriched *models.Book in the shared 24-hour ttlCache under isbn:<normalized isbn> (cacheISBNBook, a.cache.set(key, book)). A cache hit then returns that same pointer (return cached.(*models.Book), ...).

  • Shared object. Any caller that mutates the returned book, or anything reachable from it (Genres, Editions, SeriesRefs, Author), rewrites the cache entry for every later ISBN lookup of that edition. The callers today, internal/api/search.go (ISBN search) and internal/abs/import_upserts.go (lookupUpstreamBook → mergeUpstreamBook), appear to read the result and write only their own row, so I could not construct a user-visible sequence. It is a trap for the next caller that edits the result, as GetBook, GetAuthor and GetEditions hand every caller the cached object, and two callers mutate it #2809 describes for GetBook.
  • No provider/credential scope. The key has no provider-configuration scope. After a Hardcover token is added, changed or removed, or the primary provider changes, the ISBN answer built under the old configuration is served for up to 24 hours.

Why this is outside #2615

#2615 (metadata request caching) moves GetBook, GetAuthor, GetEditions and GetEditionsFromProvider onto cloned, provider-scoped cache entries, which fixes #2809. It does not touch the ISBN path, and folding it in would widen that PR beyond the lookups #2809 names.

Suggested fix

Once #2615 lands, give the ISBN lookup the same treatment: clone on store and on hit (cloneBook), and include the bound provider scope in the key (bindCacheProviders), so a configuration change selects a fresh entry. Regression test: look up one ISBN twice, mutate the first result's Genres[0] and OwnerUserID, and assert the second result is unchanged.

The series and canonical/audnex caches (aggregator_series.go, aggregator_enrichment.go) are also unscoped and may share pointers. Their callers have not been traced for mutation, so they are mentioned here only as candidates to check alongside this fix.

Activity

  1. magrhino commented on Oct 4, 2026

    @magrhino
    ContributorAuthor

    Two more places with the same pattern, found while reviewing the #2615 follow-up that bounds retries of failing enrichers:

    • GetCanonicalBookByASIN (internal/metadata/aggregator_enrichment.go): stores its result under asin-canonical:<asin> with no provider scope, and a hit returns the cached *models.Book pointer itself, so a caller that modifies the book changes the cache entry.
    • Both this lookup and the ISBN path (cacheISBNBook, if store { a.cache.set(key, book) }) run enrichBook and then cache the book for 24h whether or not an enricher failed. An incomplete enrichment is therefore pinned for a day.

    #2615 handles this for GetBook: an incomplete enrichment snapshot is kept for five minutes only, so a failing enricher is retried once per window. These two lookups should get the same treatment along with the clone/scope fix above: clone on store and on hit, scope the key, and keep a result built during an enricher failure in the short cache rather than the 24h one.

  2. added
    bugSomething isn't working
    area:metadataMetadata providers and matching
    on Oct 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    area:metadataMetadata providers and matchingbugSomething isn't workingpriority:lowCosmetic or edge case

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions