perf(metadata): cache provider searches and deduplicate requests

[magrhino]

Summary

Repeated searches and concurrent metadata cache misses currently send redundant provider requests, while routed and explicit Hardcover edition lookups can miss each other's cache entries. This change reuses successful provider responses and combines identical in-flight fetches while preserving account isolation, cancellation, complete editions, and retryable failures. Closes #2594. Closes #2809: cached books, authors and editions are cloned on store, on hit and for shared waiters, so callers no longer mutate the cache entry. Refs #2592.

• Add a bounded five-minute provider search cache (1,000 entries), with keys scoped to the provider, exact query and resolved configuration.
• Share identical in-flight metadata fetches, deep-copy returned results, and cancel upstream work when its last caller leaves. Hardcover queue admission respects the remaining callers' scheduling deadlines.
• Resolve providers before canonicalizing edition keys; freeze live Hardcover credentials across pagination and scope raw/derived metadata caches to their inputs and provider configuration.
• Work with main's manual-refresh cache bypass (bug: manual author metadata refresh can reuse 24-hour cached profile and catalogue #2601): a Refresh Metadata still skips the cached author profile and catalogue and writes back only intact answers, while nested edition, cover and ISBN lookups keep the caches. Enrichment snapshots still refuse locked fields (No Hardcover write path checks IsFieldLocked, so hydration and enrichment can overwrite fields the user edited by hand #2767) and clone genres (Enrichment snapshot caches an aliased Genres slice, so a later in-place mutation can poison it for every book of that work #2783).
• Recover a provider panic in a shared fetch as a generic error, so it cannot crash the process.
• Retain the existing 24-hour in-memory metadata cache and explicit uncached provider paths. Persisted edition reuse is deferred because local rows lack a complete-snapshot freshness marker.
• Document cache behavior and add focused coverage for bounds, expiry, concurrency, cancellation, errors, account changes, result ownership, edition identity and throttle admission.

Scope

• This PR has one reviewable objective: metadata caching and request deduplication.
• Follow-ups and intentionally deferred work: Daily quota protection #2593 owns daily accounting, allowance detection, account pacing, quota headroom, exhaustion/resume and durable deferred hydration; GetBookByISBNWithOutcome hands every caller the cached book and keys it without provider scope #2869 tracks the same shared-pointer and unscoped-key pattern in the ISBN lookup cache, which GetBook, GetAuthor and GetEditions hand every caller the cached object, and two callers mutate it #2809 does not cover. Per maintainer ordering, this lands after fix(hardcover): pause bulk work until daily quota resets #2699.

When integrating #2593, retain quota state in token-bound clients, keep cache hits and shared waiters outside attempt accounting, and verify quota headroom for mixed interactive/background callers. This PR does not implement quota accounting.

Checklist

• Commits signed off with git commit -s
• Tests added or updated
• README.md and docs/DEPLOYMENT.md updated; provider cache contract documented in CONTRIBUTING.md and godoc
• Added changelog.d/2594-metadata-request-caching.md
• template.env updated — N/A: no environment variables or configuration format changes
• CHANGELOG.md updated — N/A: repository policy uses fragments and assembles the changelog at release time
• Wiki pages updated — N/A: no series/UI workflow changes; cache behavior is documented in the deployment guide

Test plan

Rebased onto main at 84a0ab24. Go checks at fb7b4753 on macOS with GOTOOLCHAIN=go1.26.1:

• go vet ./...
• go test -race -count=3 ./internal/metadata ./internal/metadata/hardcover
• go test -race -count=5 -run TestDiscoverAuthorBooks_ConcurrentBulkRefreshCreatesOnceAnnouncesOnce ./internal/api
• New regression tests fail with their fix reverted (unconfigured enricher caching, panic recovery, sync summary clone)
• GOTOOLCHAIN=go1.26.6 go run golang.org/x/vuln/cmd/govulncheck@d1f380186385b4f64e00313f31743df8e4b89a77 ./...: zero affected vulnerabilities; one required-module advisory with no called vulnerable symbols
• git diff --check
• make test: all packages pass except 7 TestManualImportScan_* tests in internal/api, which fail identically on a clean 84a0ab24 checkout on macOS (/var vs /private/var); tracked in macOS: manual-import scan tests from #2480 fail and gosec G115 flags samedevice_unix.go #2868
• go run github.com/golangci/golangci-lint/v2/cmd/golangci-lint@v2.11.4 run --timeout=10m ./...: one macOS-only gosec G115 in internal/api/samedevice_unix.go, also present on a clean 84a0ab24; tracked in macOS: manual-import scan tests from #2480 fail and gosec G115 flags samedevice_unix.go #2868. Zero issues in files this PR touches
• tests/run-all.sh: N/A, backend validation uses the repository's Make targets
• Frontend build/tests: N/A, no frontend changes
• Docker or Compose validation: N/A, no container changes

The rebased patch passed independent high-risk review after the conflict resolution and follow-up fixes. Remote CI is not certified by these local results; see the PR comment for the full rebase notes.

[codecov]

Codecov Report

❌ Patch coverage is 93.83562% with 18 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
internal/metadata/request_cache.go	95.16%	3 Missing and 3 partials ⚠️
internal/metadata/aggregator_author_works.go	84.37%	3 Missing and 2 partials ⚠️
internal/metadata/cache_clones.go	94.11%	2 Missing and 2 partials ⚠️
internal/metadata/aggregator_enrichment.go	94.87%	1 Missing and 1 partial ⚠️
internal/metadata/aggregator.go	95.65%	1 Missing ⚠️

📢 Thoughts on this report? Let us know!

[vavallee]

Also scoped on #2594 this morning; pointing here so it's where you'll see it. This one wants to land after #2699, since it builds on the quota carrying client, and it needs a rebase rather than a merge: it's about seventy commits behind, and two files it touches moved underneath it (`aggregator_enrichment.go` with the field locks in #2767, and `GetBookByISBNWithOutcome`'s signature in `aggregator.go`). Please run the full suite against merged main after, since two individually green PRs on one file have broken main here before.

Something I found today that makes this more valuable than I'd credited: `cachedRequest` clones on store, on hit and for singleflight followers, which fixes #2809, where main's `GetBook`, `GetAuthor` and editions lookups hand every caller the cached object and two callers mutate it. Worth a `Closes #2809` once rebased. Heads up that your genres clone in `aggregator_enrichment.go` is line for line the same as #2787, which is approved, so whichever lands second gets a trivial conflict.

[magrhino]

Rebased onto main at 84a0ab24 (rebase, no merge) and force-pushed. The branch is three signed-off commits:

1. 2c23684f perf(metadata): cache searches and deduplicate requests (Caching and request deduplication #2594): the original commit, with conflicts resolved
2. 1c97cbeb fix(metadata): keep enrichment cached without a hardcover token and recover fetch panics (Caching and request deduplication #2594)
3. fb7b4753 fix(metadata): copy the thin cluster sample when cloning a cached author (Caching and request deduplication #2594)

The rebase needed more than conflict resolution. While this branch was behind, main changed the same functions it converts: #2601 (manual refresh skips the cache), #2767 (field locks in enrichment caching), #2783 (genres cloned on store and read), and the search fan-out rewrite that reports failed providers.

Conflict resolution (in 2c23684f)

• internal/metadata/aggregator.go
  • SearchAuthorsWithOutcome and SearchBooksWithOutcome keep main's version and route through the cached searchProviderAuthors / searchProviderBooks.
  • GetAuthor uses the scoped key. Under WithCacheBypass it calls the provider directly. It stores a clone on success and leaves the cached entry alone on error, which keeps the bug: manual author metadata refresh can reuse 24-hour cached profile and catalogue #2601 contract.
• internal/metadata/aggregator_author_works.go
  • GetAuthorWorks, GetAuthorWorksUnenriched and GetAuthorWorksForAuthor bind provider scope first, then consumeCacheBypass, so nested edition, cover and ISBN lookups keep their caches.
  • rawPrimaryAuthorWorks goes through cachedRequest. refreshPrimaryAuthorWorks uses the same scoped raw key through a new primaryAuthorWorksSource.
  • A refresh deletes the enriched authorworks:<scope>:<id> entry, so it still cannot shadow fresher raw works.
• internal/metadata/aggregator_enrichment.go
  • A snapshot is stored only when every enricher answered, the request wasn't cancelled, and neither description nor genres are locked (No Hardcover write path checks IsFieldLocked, so hydration and enrichment can overwrite fields the user edited by hand #2767).
  • applyEnrichmentSnapshot keeps the lock checks and slices.Clone of genres. The fix(metadata): clone the enrichment snapshot's genres on store and read (#2783) #2787 genres clone merged cleanly, as you predicted.
• internal/metadata/hardcover/throttle.go: imports both metadata (for SchedulingDeadline) and main's new providererr.

Full suite against merged main

As you suggested, I ran make test on the merged result. Two tests that landed after this branch's last CI run (#2662, #2663) failed. Both pass on plain main.

• TestCreateAuthorCore_DefaultParamsMatchHandler
  • The fixture's drain calls jobs.Group.Shutdown, which cancels the job context before the sync reaches the provider. It passed only because the stub ignores cancellation, while cachedRequest honours it up front.
  • The test now waits for the works lookup before draining.
• TestDiscoverAuthorBooks_ConcurrentBulkRefreshCreatesOnceAnnouncesOnce
  • It required two provider GetAuthor calls, but the concurrent refresh now shares discovery's in-flight lookup.
  • The stub is wrapped in an unsharedMetaProvider whose ResolveCacheProvider returns a fresh scope per request. Both syncs reach the provider again, and the original assertion (the refresh's call arrives while discovery is gated) is unchanged.

Fixes to the PR found in re-review (1c97cbeb, fb7b4753)

• Enrichment was never cached on a default install.
  • Hardcover is registered as an enricher with or without a token. Without one, SearchBooks returns ErrProviderNotConfigured, which marked every snapshot uncacheable and brought back the per-work search cost from ABS import stalls indefinitely on a large folder-backed item (no timeout, no error) and auto-resumes into the same stall #2578.
  • That error no longer blocks caching; real failures still do. The snapshot key includes the Hardcover token hash, so adding a token starts a fresh namespace.
• Provider panics. The shared fetch runs on its own goroutine, outside net/http's recovery. recoverFetch now turns a panic into a generic errFetchPanicked for every waiter. The log keeps the panic and stack with only the key's operation prefix: no query and no token hash.
• Author clone. AuthorSyncSummary.SkippedThinClusterSample was added on main after cloneAuthor was written, so a clone shared it with the cache entry. It is now copied, and a reflection test covers every slice on the summary.
• Docs. The caching section of docs/DEPLOYMENT.md now notes that a manual Refresh Metadata still skips the cached profile and catalogue.

Each new test fails with its fix reverted.

Validation at fb7b4753 (macOS, GOTOOLCHAIN=go1.26.1)

• Passed:
  • go vet ./...
  • go test -race on ./internal/metadata/...
  • -race -count=5 on the concurrency test
  • the pinned govulncheck (go1.26.6): 0 affected vulnerabilities
• make test: everything passes except 7 TestManualImportScan_* tests, which fail identically on a clean 84a0ab24 checkout on macOS (/var vs /private/var). Filed as macOS: manual-import scan tests from #2480 fail and gosec G115 flags samedevice_unix.go #2868.
• Pinned golangci-lint v2.11.4: clean apart from one macOS-only gosec G115 in samedevice_unix.go, also on clean 84a0ab24 and in macOS: manual-import scan tests from #2480 fail and gosec G115 flags samedevice_unix.go #2868.

Follow-ups

• GetBook, GetAuthor and GetEditions hand every caller the cached object, and two callers mutate it #2809: the body now says Closes #2809. GetBook, GetAuthor, GetEditions and GetEditionsFromProvider clone on store, on hit and for shared waiters.
• GetBookByISBNWithOutcome hands every caller the cached book and keys it without provider scope #2869: GetBookByISBNWithOutcome has the same shared-pointer and unscoped-key pattern. It is outside GetBook, GetAuthor and GetEditions hand every caller the cached object, and two callers mutate it #2809's list, so it is filed separately rather than widening this PR.
• fix(hardcover): pause bulk work until daily quota resets #2699 is still open. If it lands first I'll rebase again, per your ordering.

[vavallee]

Thanks for this, the isolation and dedup side held up to everything I threw at it: clones share nothing at any depth, keys carry the provider instance and token scope, waiters cancel independently, and a panic in a shared fetch is recovered. Two things before it merges.

Cache hits now re-run failing enrichers. GetBook caches the book before enrichment and calls enrichBook on every hit, and the enrichment snapshot is only stored when every enricher succeeds (aggregator_enrichment.go:336, :403). So with a revoked Hardcover token, Hardcover 429s or a Google Books outage, five GetBook calls send five requests to the failing enricher, where main sends one because it cached the enriched book for 24h. Healthy enrichers also get asked again after the 5 minute search TTL. Suggest storing the snapshot in the 5 minute requests.searches cache when it isn't cacheable, so it stays retryable but bounded. TestEnrichmentFailureRemainsRetryable would need a clock step.

CodeQL alert 750 (request_cache.go:200) is a false positive as you said, but passing op into recoverFetch as an explicit argument instead of recovering it with strings.Cut(key, ":") breaks the taint flow, so it costs nothing to clear.

Minor, take or leave: once #2699 lands the token scope in hardcover/cache.go:16 could reuse its HMAC fingerprint instead of a bare sha256.

[magrhino]

@vavallee thanks, both done as you suggested, in 38632444. I rebased onto d7ac8dcd and added a test commit, 6aff65cd.

Failing enrichers on cache hits. Did what you proposed: when a snapshot isn't cacheable because an enricher failed, enrichBook stores it in the 5-minute requests.searches cache, reached through a small shortCache() helper. Lookups check the 24h cache first, then that one.

• A failing enricher is now asked once per window rather than once per GetBook, and the result stays retryable.
• Locked books and cancelled requests still store nothing.
• TestEnrichmentFailureRemainsRetryable now steps the clock with synctest: five calls send one request; after the window the enricher is asked again; once it recovers, the complete snapshot holds for 24h.
• New tests cover the locked and cancelled cases and show that a complete 24h snapshot beats a short one under the same key.
• docs/DEPLOYMENT.md notes that these snapshots share the 1,000-entry cache with searches.

CodeQL 750. cachedRequest takes op as an explicit argument, and every caller passes a literal. recoverFetch logs that, not anything cut from the key, so the taint flow is gone.

HMAC fingerprint. Will switch hardcover/cache.go to it once #2699 lands.

Locally at 6aff65cd: make test, go vet, -race on internal/metadata, and pinned golangci-lint v2.11.4 all pass.

[vavallee]

Thanks, both look good. A failing enricher now gets asked once per 5 minute window, locked books and cancelled requests still store nothing, and a clean run still caches for 24h. I reverted the short cache write and your tests caught it, which is what I wanted. CodeQL 750 shows as fixed on the PR ref.

I also merged current main locally since #3038 added guarded writes in the scheduler refresh and ABS, and everything they read from the cache comes back as a clone, so no shared objects reach those writes. Build, vet, tests and race on metadata all pass. The two red checks are from a test merge older than #3036 and #2983, so I've merged main into the branch to rerun them. Only nit is a stray blank line at aggregator_enrichment.go:318, not worth a push on its own.

No need to wait on #2699 since nothing here depends on it, so this goes in first once CI is green. The HMAC fingerprint tweak can follow after #2699.