Skip to content

Actually use the cached search response instead of always refetching - #7

Open
johnnyxmas wants to merge 1 commit into
janholtzhausen:mainfrom
johnnyxmas:fix/cache-metadata-search-results
Open

johnnyxmas wants to merge 1 commit into
janholtzhausen:mainfrom
johnnyxmas:fix/cache-metadata-search-results

Conversation

@johnnyxmas

Copy link
Copy Markdown

Summary

GoodreadsSearchProxy.Search passes useCache: false while also passing a 5 day TTL, which are contradictory:

var response = _cachedHttpClient.Get<List<SearchJsonResource>>(httpRequest, false, TimeSpan.FromDays(5));

CachedHttpResponseService.Get only reads the cache when useCache is true:

if (useCache && cached != null && cached.Expiry > DateTime.UtcNow)

It still writes the response to the cache afterwards, so today every search populates a cache entry that is then never read, and each repeated search goes back out to the metadata origin.

Why it matters

The metadata origin rate limits aggressively. In testing, only queries already sitting in the Cloudflare edge cache succeed; anything else hits the origin and is refused:

$ curl -sD - -o /dev/null "https://hardcover.bookinfo.pro/search?q=dune"
HTTP/2 200
age: 25845
cf-cache-status: HIT

$ curl -sD - -o /dev/null "https://hardcover.bookinfo.pro/search?q=tolkien"
HTTP/2 429
cf-cache-status: BYPASS
retry-after: 132

GoodreadsSearchProxy has no retry or back-off (unlike BookInfoProxy, which calls WaitUntilRetry in several places), so a 429 is thrown as a GoodreadsException and surfaces in the UI as an empty result set — which reads to a user as "this author has no books" rather than as an error:

[Warn] BookInfoProxy: Error searching for louis armstrong
NzbDrone.Core.MetadataSource.Goodreads.GoodreadsException: Search for 'louis armstrong' failed.
Unable to communicate with metadata source.
 ---> NzbDrone.Common.Http.TooManyRequestsException: HTTP request failed:
      [429:TooManyRequests] [GET] at [https://hardcover.bookinfo.pro/search?q=louis%20armstrong]

Honouring the cache for its intended 5 day lifetime removes a large share of those requests and makes repeated searches resolve instantly from the local DB. It does not fix a first-time search for an uncached term — that still depends on the origin — but it stops the client re-asking for things it already has.

Change

One character: false → true. The TTL already passed in is unchanged, so cached entries expire after 5 days as originally intended.

Test plan

  • Confirmed via CachedHttpResponseService.Get that useCache: false bypasses the read path while still writing the entry
  • Reproduced the 429-as-empty-results behaviour against the live origin, with trace logs as above
  • Built and deployed on Linux x64; searches that previously re-hit the origin now serve from cache

Possible follow-up

GoodreadsSearchProxy might also benefit from the TooManyRequests/WaitUntilRetry handling that BookInfoProxy already has, so a rate-limited search reports an error rather than an empty list. Happy to open that separately if you'd like it.

GoodreadsSearchProxy.Search passed useCache: false while also passing a
5 day TTL, which are contradictory. CachedHttpResponseService.Get only
reads the cache when useCache is true:

    if (useCache && cached != null && cached.Expiry > DateTime.UtcNow)

so every search was written to the cache and then never read from it,
and each repeated search hit the metadata origin again.

That matters because the metadata origin rate limits aggressively: an
uncached query frequently returns 429, which this path surfaces to the
user as an empty result set (there is no retry/back-off here, unlike
BookInfoProxy which calls WaitUntilRetry in several places). Reusing the
cached response for its intended 5 day lifetime removes a large share of
those requests and makes repeated searches resolve instantly.
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.

1 participant