Skip to content

Surface metadata source failures instead of returning an empty result - #9

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

johnnyxmas wants to merge 1 commit into
janholtzhausen:mainfrom
johnnyxmas:fix/surface-metadata-search-errors

Conversation

@johnnyxmas

Copy link
Copy Markdown

Summary

BookInfoProxy.Search wraps the search proxy call in a blanket catch (Exception) that logs a warning and returns an empty list:

try
{
    result = _goodreadsSearchProxy.Search(query);
}
catch (Exception e)
{
    _logger.Warn(e, "Error searching for {0}", query);
    return new List<Book>();
}

So any failure to reach the metadata source renders as "no results found", which is indistinguishable from a genuine empty result. A metadata outage looks to the user like the author or book simply does not exist.

This is currently very visible, because the origin rate limits aggressively (see #8). Searching dune returns 25 results while Louis Armstrong, plain Armstrong and tolkien all return 0 — not because those have no books, but because the origin answered 429:

[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]

Why rethrowing is the right fix

Two things indicate the exception was always meant to propagate:

  1. GoodreadsException derives from NzbDroneClientException and is constructed with HttpStatusCode.ServiceUnavailable, so it is already designed to surface as a 503 with a message rather than be swallowed.

  2. CandidateService already catches GoodreadsException to degrade gracefully during import identification:

catch (GoodreadsException e)
{
    _logger.Info(e, "Skipping author/title search due to Goodreads Error");
    remoteBooks = new List<Book>();
}

Because the blanket catch in BookInfoProxy runs first, that handler can never observe the exception — it is effectively dead code today.

Change

Rethrow GoodreadsException, keeping the existing empty-list fallback for other unexpected errors.

  • Import identification is unaffected — CandidateService still catches it and falls back to an empty candidate list, which is what it already intended to do.
  • Interactive search now reports the failure instead of silently showing nothing, surfacing as 503 "Unable to communicate with metadata source".

Considered and rejected: retrying with back-off

BookInfoProxy has a WaitUntilRetry helper used on other paths, but it calls Thread.Sleep on the Retry-After value. The origin currently returns retry-after: 132, so retrying a user-initiated search would block the request for over two minutes. Failing fast with an accurate message seemed clearly preferable for the interactive path; happy to revisit if you would rather have a bounded retry.

Test plan

  • Verified GoodreadsException resolves in BookInfoProxy (using NzbDrone.Core.MetadataSource.Goodreads already present) and is the same type CandidateService catches
  • Confirmed it derives from NzbDroneClientException with ServiceUnavailable, so it maps to a 503 rather than an unhandled 500
  • Built on linux-x64 and deployed; rate-limited searches now report an error instead of an empty list

BookInfoProxy.Search wrapped the call to the search proxy in a blanket
catch (Exception) that logged a warning and returned an empty list, so
any failure to reach the metadata source rendered as "no results found".
That is indistinguishable from a genuine empty result, which makes a
metadata outage look like the author or book simply does not exist.

GoodreadsException already derives from NzbDroneClientException with
HttpStatusCode.ServiceUnavailable, i.e. it is built to be surfaced, and
CandidateService already catches GoodreadsException explicitly to
degrade gracefully during import identification. The blanket catch here
swallowed the exception before that handler could ever observe it,
leaving it effectively dead code.

Rethrow GoodreadsException so callers see it, and keep the existing
empty-list fallback for other unexpected errors. Import identification
behaviour is unchanged because CandidateService still handles it;
interactive search now reports the failure rather than silently showing
nothing.
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