Repository navigation
fix(metadata): shared retry loop and provider wide refusal hold, Hardcover GraphQL rate limits (#2369, #2075, #2791) - #3100
Merged
Merged
Conversation
…iders (#2369, #2075) Only OpenLibrary retried a 429. Google Books, DNB, NB, Audible and audnex made one bare request, so a refusal was a hard "metadata lookup failed" and, for all but NB, carried no providererr mark for discovery to back off on (#2369). OpenLibrary's loop moves into internal/metadata/providerhttp and all six HTTP providers use it: retry 429/502/503/504 and transport errors up to 3 times, honour Retry-After (capped at 30s) or jittered backoff, drain error bodies, and mark what is left with ErrRateLimited or ErrUnavailable. Google Books keeps flattening transport errors so its key never reaches a log (#1144); OpenLibrary keeps its error chain. Each client also gets a Gate. A refusal used to back off only the request that was refused, so the other catalogue fetches and edition samples a bulk import had in flight kept hitting a provider that had asked for quiet, which is how #2075's 429s escalated into timeouts and refused connections. Now a refusal holds every request through that client, and a request whose hold would outlast its deadline fails at once as rate limited instead of sleeping into a timeout. Hardcover keeps its own adaptive pacer. Signed-off-by: vavallee <vavallee@protonmail.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016fJcCVbNnKsmj2MAAMwWj9
query called pacer().succeed() before looking at the body, so a 200 carrying a GraphQL errors array counted as a healthy request. A rate limit delivered that way never penalised the throttle and advanced its decay counter, unwinding pacing earlier refusals had set up. The envelope is now classified first. An errors array whose message or extensions code reads as a rate limit or quota refusal penalises the pacer with Hardcover's own hint and is retried like a 429, marked ErrRateLimited. Any other GraphQL error is returned without touching the pacer. Only a clean answer counts as a success. Signed-off-by: vavallee <vavallee@protonmail.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016fJcCVbNnKsmj2MAAMwWj9
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
… daily quota (#2369) Review follow ups on the shared provider request loop: - A hold now remembers what set it. A request turned away by a hold a 502, 503 or 504 set reports ErrUnavailable; only a 429 hold reports ErrRateLimited. - Request.Final lets a provider stop the loop on an answer waiting will not fix. Google Books uses it for a 403 or 429 whose reason is dailyLimitExceeded or quotaExceeded, returned at once and marked as a rate limit; a per minute rateLimitExceeded is still retried. - A transport failure whose backoff would outlast the caller's deadline returns the error at once instead of sleeping into the timeout. - An unparseably large Retry-After is clamped to the cap, not ignored. - SetBaseDelay lets provider test suites shorten the backoff; nb, dnb, googlebooks, audible and audnex use it (nb went from 26s to 0.1s). - The DNB num= fallback test pins the retry count and the fallback query. Signed-off-by: vavallee <vavallee@protonmail.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016fJcCVbNnKsmj2MAAMwWj9
…t breaker (#2075) The shared provider loop fails a held request at once rather than sleeping into the lookup's deadline. The import outage breaker only exempted hardcover.ErrRateLimited, so after a single OpenLibrary 429 every following row was turned away within a millisecond, three of them tripped the breaker, and CSV, Readarr and Goodreads imports failed every remaining row. Before the shared loop that took about 24s of refusals. The breaker now leaves any rate limit out of its streak, whichever provider refused, and the importers wait out a primary's hold before asking again (the hold's own end time, or 2s for a refusal that names none, at most 3 times per lookup) instead of failing the row. A hold an outage set is still waited out once its end is known, then counts toward the breaker as before. Also gives the Add Book ISBN and ASIN lookups one 20 second deadline: they ask the providers in turn and each now retries, so two dead providers could outlast the server's 120s write timeout. Signed-off-by: vavallee <vavallee@protonmail.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016fJcCVbNnKsmj2MAAMwWj9
Waiting out a primary's hold made a refusal that never ends unbounded: each row waited 3 times, then failed as rate limited, which the outage breaker ignores, so a 1000 row import against a refusing provider ran for hours and sent thousands of requests. The Readarr and Goodreads imports run detached and could not be stopped. A lookup still refused after its waits now counts toward the outage streak, so three such lookups in a row trip the breaker and the rest of the rows fail at once with "primary metadata provider X is rate limiting requests, try the import again later". A run also has a 10 minute budget for waiting out holds; once spent, a refused lookup is not waited out and counts straight away. isRateLimited moves into outage.go, which keeps the errors and hardcover imports in use, so the file still builds when #2699, which adds an errors.As case to the same switch, lands alongside this. Signed-off-by: vavallee <vavallee@protonmail.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016fJcCVbNnKsmj2MAAMwWj9
vavallee
marked this pull request as ready for review
October 8, 2026 03:44
vavallee
enabled auto-merge (squash)
October 8, 2026 03:44
15 of 17 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #2369
Closes #2791
Refs #2075
#2369: one request loop for every HTTP provider. Only OpenLibrary retried a 429. Google Books, DNB, NB, Audible and audnex made one bare request, so a refusal surfaced as "metadata lookup failed" and the caller moved on. OpenLibrary's loop now lives in
internal/metadata/providerhttpand all six use it: 429/502/503/504 and transport errors retried up to 3 times,Retry-Afterhonoured (capped at 30s, an unparseably large value clamps to the cap) or jittered backoff, error bodies drained, and what outlives the retries markedprovidererr.ErrRateLimited/ErrUnavailableso discovery backs off. Per provider differences stay at the call site: Google Books still flattens transport errors so its key never reaches a log (#1144), OpenLibrary keeps its error chain, 404 handling unchanged. A Google Books 403 or 429 whose reason isdailyLimitExceededorquotaExceededis not retried (it resets tomorrow) and comes back marked as a rate limit; a per minuterateLimitExceededis still retried.#2075: a refusal holds the whole provider, not one request. This is the remaining "OpenLibrary side" of the report. #2101 gave each request its own backoff, but a bulk import has catalogue fetches and edition samples in flight together, and while one waited out
Retry-Afterthe others kept sending. Each client now owns aGate: a refusal shuts it for every request through that client, and a request whose hold would outlast its deadline fails at once instead of sleeping into a timeout. The hold remembers its cause, so a request turned away after a 429 reportsErrRateLimitedand one turned away after a 5xx reportsErrUnavailable. A transport failure whose backoff would outlast the caller's deadline also returns at once. Same design as Hardcover's shared pacer, which stays as is.Importers wait out holds. Because a held request now fails fast, the import outage breaker (which only exempted
hardcover.ErrRateLimited) would trip within a millisecond of a single OpenLibrary 429 and fail every remaining row.migrate/outage.gonow exempts any rate limit, and the CSV, Readarr and Goodreads importers wait out a primary's hold (its end time, or 2s when none is named, at most 3 times per lookup) and ask again instead of failing the row. A hold set by an outage is waited out too, then counts toward the breaker as before. Waiting is bounded: a lookup still refused after its waits counts toward the outage streak, so a primary that keeps refusing trips the breaker after 3 such lookups and the remaining rows fail at once with "is rate limiting requests, try the import again later", and a run spends at most 10 minutes waiting out holds in total (the Readarr and Goodreads imports run detached and cannot be cancelled).Add Book lookups have one deadline. The ISBN and ASIN lookups ask providers in turn and each now retries, so two dead providers could outlast the 120s server write timeout. Both run under one 20s deadline, which the provider retries and holds respect.
#2791: Hardcover 200 with GraphQL errors.
querycalledsucceed()before reading the envelope. Now the envelope is classified first: an errors array whose message orextensions.codereads as a rate limit or quota refusal penalises the pacer with Hardcover's hint and is retried like a 429 (markedErrRateLimited); any other GraphQL error returns without touching the pacer; only a clean answer counts as a success.#2699 compatibility. No changes to the daily quota code paths.
hardcover/client.gomerges cleanly with the PR head.migrate/outage.gokeeps itserrorsandhardcoverimports in use, and a trial merge (not committed) of the #2699 head onto this branch merges cleanly, builds (go build ./...), vets and passesinternal/migratetests.DailyQuotaErrordoes not matchErrRateLimited, so the importers do not wait on a daily hold; #2699's case trips the breaker first.Follow up
Fail before
New tests run against the pre fix sources (test files only added):
Review fixes, against this branch before them:
TestDoHoldCauseClassification,TestDoFinalIsNotRetriedandTestDoTransportBackoffRespectsDeadlinecover the new loop behaviour inproviderhttp. Two existing tests changed: OpenLibrary's retry tests now name the shared constants, and the DNB num= fallback test routes by query and pins the retry count and fallback query. Provider test suites shorten the backoff throughproviderhttp.SetBaseDelay(nb 26s to 0.1s).Checklist
git commit -sdocs/ARCHITECTURE.md(metadata row),docs/User-Guide-Wiki.md(discovery under a struggling provider)changelog.d/;CHANGELOG.mduntouchedTest plan
gofmt -l,go vet ./...golangci-lint runoninternal/metadata/...,internal/migrate/...,internal/api(v2.11.4): 0 issuesgo test -race ./internal/metadata/providerhttp/go test -p 2 -timeout 40moninternal/metadata/...,internal/migrate/...,internal/abs/...,internal/scheduler/...,internal/api/...🤖 Generated with Claude Code
https://claude.ai/code/session_016fJcCVbNnKsmj2MAAMwWj9