Skip to content

GetBook, GetAuthor and GetEditions hand every caller the cached object, and two callers mutate it #2809

Closed

Description

@vavallee

Pre-flight

Current behaviour

Aggregator.GetBook (internal/metadata/aggregator.go:793-818) stores the provider's *models.Book in the shared 24 hour ttlCache and returns that same pointer to every caller that hits the key. GetAuthor (:770-791), GetEditions (:821-836) and GetEditionsFromProvider (:841-864) do the same with *models.Author and []models.Edition.

Two callers mutate what they are handed:

  • add_book_core.go:381-400 sets AuthorID, OwnerUserID, Monitored, Status and MediaType on the result, then passes it to BookRepo.Create, which writes ID, CreatedAt and UpdatedAt back onto the struct (internal/db/books.go:666-668).
  • abs/import_upserts.go:866-883 sets ForeignID, AuthorID, OwnerUserID, Monitored, Status, AnyEditionOK and runs applyABSFormatFields on it.

So after either path, the cache entry for that work carries one user's author id, owner, monitoring choice, media type and database row id until it expires.

The codebase already knows this is wrong. enrichmentSnapshot's own comment (aggregator_enrichment.go) says aliasing a *models.Book into the cache "would let those mutations leak into the next cache hit", and the author works cache clones on both store and read (cloneBooks, aggregator_author_works.go:170, :243, :309, :366). GetBook, GetAuthor and the two editions lookups are the paths that never got that treatment.

Why this is filed as latent rather than as a user-facing bug

I could not construct a sequence where it changes what a user sees today. The obvious ones are blocked by foreign_id being unique across the instance: a second Create for the same work fails on the constraint rather than inheriting the stale row id, and the request builder (requests.go:587-603) and resolveAuthorForBook (authors.go:3201) read only title, foreign id, author and editions, none of which the mutating callers change. It is a trap for the next caller that reads ID, OwnerUserID or MediaType off a GetBook result, not a defect with a reproduction.

Expected behaviour

Nothing reachable through a cache entry is reachable through a returned value. Either clone on store and on read, as cloneBooks does, or return a copy and document that the result is the caller's to mutate.

Suggested fix

Clone at the aggregator boundary for all four lookups. models.Book holds slices (Genres, Editions, SeriesRefs) and a *models.Author, so a struct copy alone re-creates the #2783 bug one level down; cloneBooks already handles that for books. A regression test: call GetBook twice for one foreign id, mutate the first result's OwnerUserID and Genres[0], assert the second result is unchanged.

Activity

  1. vavallee commented on Sep 27, 2026

    @vavallee
    OwnerAuthor

    PR #2615 fixes this as a side effect. Its cachedRequest clones on store, on hit and for singleflight followers, and wraps GetBook, GetAuthor and both editions lookups. It's waiting on a rebase behind #2699.

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

    bugSomething isn't working

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions