Conversation
| func (q *Quota) observe(s *quotaState, h http.Header, code int) { | ||
| policy := dailyParameters(strings.Join(h.Values("RateLimit-Policy"), ",")) | ||
| if policy["q"] > 0 && policy["q"] <= 1_000_000_000 && policy["w"] == 86400 { | ||
| limit := int(policy["q"]) |
There was a problem hiding this comment.
I believe this is a false positive.
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
vavallee
left a comment
There was a problem hiding this comment.
Thanks for this, @magrhino, and for doing the homework on the premise. I checked it against the docs source (hardcover-docs, api/Getting-Started.mdx): the 5,000 and 50,000 daily limits, the RateLimit and RateLimit-Policy headers, and the 429 once the daily limit is spent are all there, and quota.go parses them correctly. The CodeQL alert on quota.go:451 is a false positive too, since the value is bounded.
I'd like to get protection for this in, but in a much smaller form. Some things turned up when I merged it onto current main and read it closely.
Blocking
- It no longer builds against main. #2641 changed
GetBookByISBNWithOutcometo return an outcome, soaggregator.go:921hasreturn nil, errwhere three values are needed. - Daily exhaustion doesn't stop imports.
QuotaDeferredErrormatchesErrRateLimited(quota.go:99), and since v1.36.2 the import outage breaker deliberately ignoresErrRateLimited, so short throttling doesn't fail a whole import. A spent daily quota lasts hours, so an import keeps going row by row. Exhaustion needs its own error, and imports should stop on it with the reset time. - Every Hardcover call in the process runs one at a time. The gate is held through the network request (
quota.go:246to391), across all users' tokens, and each call also costs about six settings reads and three writes. - A short per minute 429 turns into a long deferral. A Retry-After between 31 and 60 seconds sets
Hold, and hydration then stores a deferral marker instead of retrying. - Deferred edition hydration only resumes from manual refreshes.
retryDeferredEditionsruns fromfetchAuthorBookson an existing match, and all the callers that reach it are manual. Books added through the book, series or recommendation flows can stay half hydrated. - The interactive reserve is uneven. Only list sync,
refreshMetadataandfetchAuthorBooksare tagged background. ABS, CSV and Goodreads imports count as interactive and can use up the reserve, while a user adding an author gets a background catalogue sync that is skipped with only a log line.
Smaller things
- There's no UI, so the status is only visible at
GET /setting/hardcover/quota. - Migration 086 collides with #2633's 086.
- The legacy marker migration moves
auth.hardcover_deferred_editions.*keys that don't exist on main. They look like they're from an earlier draft of this PR, so it can go.
What I'd suggest
Nobody has reported hitting the daily limit yet. The closest is #2075, about bulk CSV load. So I'd rather start with the part that covers the real risk, a bulk job hammering an exhausted key:
- parse the documented
RateLimitheaders; - persist a hold until the daily reset when the remaining count reaches zero or the daily 429 arrives;
- give that its own error that stops imports and background jobs, leaving the existing short throttle handling alone;
- add one line in Settings saying Hardcover is paused until a given time.
That should be a few hundred lines. The shared accounting, the interactive reserve and deferred hydration can follow once someone actually runs into the limit, and they'll be easier to review on top of that base. On ordering, this should land before #2615, since the caching PR builds on the quota carrying client.
If you'd prefer to cut this PR down in place rather than open a new one, that's fine by me. Thanks again, the header work is the hard part and it's right.
ba52a65 to
064aead
Compare
|
@vavallee Thanks for the direction. I’ve opened #2699 with the smaller daily-reset hold and removed the accounting, reserve, and deferred-hydration pieces. For context, I encountered the original issue after a full ABS library preview followed by a real sync, but didn’t capture an error, so I can't state the exact cause just that my hardcover api was capped. The cache fixes may also address that original behavior. |
Summary
Hardcover daily exhaustion currently allows integrations to keep attempting work against an unavailable allowance. This change shares persistent quota accounting across Hardcover consumers, detects the actual plan allowance from documented API signals, and defers work until it is eligible again without losing provider identity or pending edition hydration.
Closes #2593. Refs #2592.
Scope
Accounting coordinates consumers within one Bindery process/database. Independent installations cannot share local counters. Resume uses existing scheduled/manual entry points; no immediate-resume job is introduced. Supported signals were verified against official API documentation; no authenticated live quota measurement was made.
Suggested review order
This is a 30-file change (+1,862/-45), including focused regression tests and documentation. Suggested order:
internal/metadata/hardcover/quota.goand its tests: accounting, discovery, admission, reset handling, and pacing.docs/Hardcover-Quota.mdand deployment/wiki updates.Checklist
git commit -s.changelog.d/.template.env— N/A: configuration is a runtime SQLite-backed setting, not an environment variable.CHANGELOG.md— N/A: repository policy reserves versioned changelog edits for releases; fragment supplied instead.Test plan
All listed completed commands passed (exit 0). Machine-specific cache paths are omitted below; focused tests, race tests, vet, and lint used Go 1.26.6, and lint used golangci-lint v2.11.4.
go test ./internal/metadata/hardcover ./internal/metadata ./internal/db ./internal/api ./internal/bookhydrate ./internal/hardcoverlistsyncer -run 'Test.*Quota|TestDeferredHardcover|TestMigration086|TestEditionPrefetchDeferred|TestThrottle|TestHydrateHardcover' -count=1— six packages passed.go test -race ./internal/metadata/hardcover ./internal/db ./internal/api ./internal/bookhydrate -run 'Test.*Quota|TestDeferredHardcover|TestMigration086|TestEditionPrefetchDeferred|TestThrottle|TestHydrateHardcover' -count=1— four packages passed.make test— full backend suite passed.make smoke— rebuilt frontend and binary; smoke suite passed. Vite reported a non-fatal large-chunk warning.go vet ./...— no diagnostics.golangci-lint run --timeout=5m— zero issues.git diff --checkand staged diff check — passed; changed Go files are gofmt-clean.tests/run-all.sh— not run; validation used the repository's make targets and focused commands above.The final concurrency/persistence fixes received focused independent review with no findings. A fresh full-feature review was not rerun after those fixes. Only deployment/wiki documentation links were added after the completed runtime validation.