From f6746f5953332a3a69950b92b4e231db98c09f56 Mon Sep 17 00:00:00 2001 From: vavallee Date: Sun, 11 Oct 2026 16:14:17 -0300 Subject: [PATCH 1/3] fix(hardcover): stop asking with a rejected API token (#3101) A 401 from Hardcover was classified for the error message but never recorded, so every row of a bulk import sent its own doomed request. A rejected token (401, or a 403 whose body is an authentication failure) now records a hold keyed by the token fingerprint, in its own private setting with the same HMAC key as the daily quota holds. Every call through any client sharing the store, primary, enricher, cover, ISBN and list paths included, is refused without a request. The hold ends when the token changes, after one hour (one probe request, then renewed or cleared), on a successful response, or when Test connection succeeds. Test connection always sends its request so a fixed token can be retested. The new metadata.TokenRejectedError is its own type, not DailyQuotaError, and every stop point (aggregator ISBN walk, import breaker, per row reason, discovery backoff) asks metadata.IsProviderHold instead of matching one type. System status reports hardcoverTokenRejected and Settings shows a notice. Signed-off-by: vavallee Co-Authored-By: Claude Sonnet 5.5 Claude-Session: https://claude.ai/code/session_016fJcCVbNnKsmj2MAAMwWj9 --- .../3101-rejected-hardcover-token-hold.md | 2 + cmd/bindery/discovery_wiring.go | 3 +- cmd/bindery/discovery_wiring_test.go | 2 + cmd/bindery/main.go | 8 +- docs/API.md | 2 +- docs/DEPLOYMENT.md | 23 ++ internal/abs/daily_quota_test.go | 24 ++ internal/api/rejected_token_test.go | 114 +++++++ internal/api/settings_handler.go | 5 + .../rejected_token_test.go | 45 +++ internal/metadata/aggregator.go | 3 +- internal/metadata/daily_quota.go | 47 +++ internal/metadata/daily_quota_test.go | 44 +++ internal/metadata/hardcover/client.go | 34 ++ .../hardcover/csv_rejected_token_test.go | 62 ++++ internal/metadata/hardcover/daily_quota.go | 164 ++++++++- internal/metadata/hardcover/export_test.go | 11 + .../metadata/hardcover/rejected_token_test.go | 320 ++++++++++++++++++ internal/migrate/authors.go | 5 +- internal/migrate/daily_quota_test.go | 34 ++ internal/migrate/outage.go | 3 +- internal/scheduler/daily_quota_test.go | 30 ++ web/src/api/system.ts | 2 + web/src/i18n/locales/en.json | 1 + web/src/pages/SettingsPage.test.tsx | 8 + web/src/pages/settings/ApiKeysTab.tsx | 5 + 26 files changed, 978 insertions(+), 23 deletions(-) create mode 100644 changelog.d/3101-rejected-hardcover-token-hold.md create mode 100644 internal/api/rejected_token_test.go create mode 100644 internal/hardcoverlistsyncer/rejected_token_test.go create mode 100644 internal/metadata/hardcover/csv_rejected_token_test.go create mode 100644 internal/metadata/hardcover/export_test.go create mode 100644 internal/metadata/hardcover/rejected_token_test.go diff --git a/changelog.d/3101-rejected-hardcover-token-hold.md b/changelog.d/3101-rejected-hardcover-token-hold.md new file mode 100644 index 000000000..1f787d16e --- /dev/null +++ b/changelog.d/3101-rejected-hardcover-token-hold.md @@ -0,0 +1,2 @@ +### Fixed +- **A rejected Hardcover token no longer costs a request per row** (#3101). When Hardcover refuses the saved API token (a 401, or a 403 that is an authentication failure), Bindery now stops asking with that token. A bulk import, discovery sweep, catalogue sync, list sync or scheduled refresh stops at the first rejection and says "Hardcover rejected the API token", instead of sending one doomed request for every row. Settings shows a notice, saving a new token lifts the pause immediately, **Test connection** still reaches Hardcover so you can retest after fixing the token, and one probe request an hour lets a restored token recover by itself. Server errors and queries Hardcover refuses for other reasons do not pause anything. diff --git a/cmd/bindery/discovery_wiring.go b/cmd/bindery/discovery_wiring.go index 8276fdd4c..9af2ecdd9 100644 --- a/cmd/bindery/discovery_wiring.go +++ b/cmd/bindery/discovery_wiring.go @@ -28,13 +28,12 @@ func newAuthorDiscoverer(discover authorDiscoverFunc, bulkRunning func() bool) s return scheduler.AuthorDiscovererFuncs{ Discover: func(ctx context.Context, author *models.Author) scheduler.DiscoveryOutcome { created, err := discover(ctx, author) - var daily *metadata.DailyQuotaError out := scheduler.DiscoveryOutcome{ Created: created, Err: err, // hardcover.ErrRateLimited is checked too: the bare sentinel does // not carry the shared mark, only its throttle errors do. - Backoff: errors.As(err, &daily) || errors.Is(err, metadata.ErrRateLimited) || errors.Is(err, hardcover.ErrRateLimited), + Backoff: metadata.IsProviderHold(err) || errors.Is(err, metadata.ErrRateLimited) || errors.Is(err, hardcover.ErrRateLimited), Busy: errors.Is(err, api.ErrAuthorSyncRunning), } out.Unavailable = !out.Backoff && !out.Busy && metadata.IsProviderUnavailable(err) diff --git a/cmd/bindery/discovery_wiring_test.go b/cmd/bindery/discovery_wiring_test.go index 161b1149a..abb51e7c3 100644 --- a/cmd/bindery/discovery_wiring_test.go +++ b/cmd/bindery/discovery_wiring_test.go @@ -28,6 +28,8 @@ func TestNewAuthorDiscoverer_MapsErrors(t *testing.T) { }{ {name: "success", created: 3}, {name: "daily quota", err: &metadata.DailyQuotaError{}, wantBackoff: true}, + {name: "rejected token", err: &metadata.TokenRejectedError{Provider: "Hardcover"}, wantBackoff: true}, + {name: "rejected token, wrapped", err: fmt.Errorf("author works: %w", &metadata.TokenRejectedError{Provider: "Hardcover"}), wantBackoff: true}, {name: "ordinary error", err: errors.New("provider 500")}, {name: "hardcover rate limit, wrapped", err: fmt.Errorf("author works: %w", hardcover.ErrRateLimited), wantBackoff: true}, {name: "openlibrary rate limit, wrapped", err: fmt.Errorf("author works: %w", openlibrary.ErrRateLimited), wantBackoff: true}, diff --git a/cmd/bindery/main.go b/cmd/bindery/main.go index ea93333c4..18b35d1af 100644 --- a/cmd/bindery/main.go +++ b/cmd/bindery/main.go @@ -956,8 +956,12 @@ func main() { cacheBytes, _ := imageProxyHandler.CacheSize() hardcoverState := api.HardcoverFeatureStateFor(r.Context(), settingsRepo, cfg.EnhancedHardcoverAPI) var pausedUntil string + var tokenRejected bool var exhausted *metadata.DailyQuotaError - if err := hcClient.CheckQuota(r.Context()); errors.As(err, &exhausted) { + var rejected *metadata.TokenRejectedError + if err := hcClient.CheckQuota(r.Context()); errors.As(err, &rejected) { + tokenRejected = true + } else if errors.As(err, &exhausted) { pausedUntil = exhausted.ResetAt.UTC().Format(time.RFC3339) } else if err != nil { slog.Warn("read Hardcover pause status", "error", err) @@ -975,6 +979,7 @@ func main() { EnhancedHardcoverAPI bool `json:"enhancedHardcoverApi"` HardcoverTokenConfigured bool `json:"hardcoverTokenConfigured"` HardcoverPausedUntil string `json:"hardcoverPausedUntil,omitempty"` + HardcoverTokenRejected bool `json:"hardcoverTokenRejected,omitempty"` EnhancedHardcoverDisabledReason string `json:"enhancedHardcoverDisabledReason,omitempty"` }{ Version: version, @@ -985,6 +990,7 @@ func main() { EnhancedHardcoverAPI: hardcoverState.EnhancedHardcoverAPI, HardcoverTokenConfigured: hardcoverState.HardcoverTokenConfigured, HardcoverPausedUntil: pausedUntil, + HardcoverTokenRejected: tokenRejected, EnhancedHardcoverDisabledReason: hardcoverState.EnhancedHardcoverDisabledReason, }) }) diff --git a/docs/API.md b/docs/API.md index c775f81f4..1d85558cb 100644 --- a/docs/API.md +++ b/docs/API.md @@ -755,7 +755,7 @@ POST /api/v1/backup snapshot the SQLite database ( GET /api/v1/backup list stored backups (admin) DELETE /api/v1/backup/{filename} delete one backup (admin) POST /api/v1/backup/{filename}/restore stage a backup for the next restart (admin, X-Confirm-Restore: true) -GET /api/v1/system/status version, commit, build date, newest published release, image cache size, Hardcover feature state +GET /api/v1/system/status version, commit, build date, newest published release, image cache size, Hardcover feature state; `hardcoverPausedUntil` while the daily quota holds, `hardcoverTokenRejected` while Hardcover has refused the saved token POST /api/v1/library/scan start a library scan in the background (202; {"queued": true} when one is already running) GET /api/v1/library/duplicate-candidates read-only duplicate title groups across every author, paginated (#2999) GET /api/v1/library/scan/status summary of the last library scan, paths included (admin) diff --git a/docs/DEPLOYMENT.md b/docs/DEPLOYMENT.md index b068090b1..712d92ec2 100644 --- a/docs/DEPLOYMENT.md +++ b/docs/DEPLOYMENT.md @@ -746,6 +746,29 @@ Scheduled jobs can run again on their next scheduled pass. This does not add automatic edition-hydration recovery or request accounting. Different tokens have separate holds, even if they belong to the same account. +### Hardcover rejected token pause + +When Hardcover answers a request with 401, or with a 403 whose body is an +authentication failure (`invalid_token`, `unauthorized`, `invalid_grant` or +"Unable to verify token"), Bindery stops sending requests with that token. A +bulk import, a discovery sweep, a catalogue sync, a list sync and the scheduled +refresh stop at the first rejection instead of asking once per row, and the +import result carries "Hardcover rejected the API token". Server errors (5xx), +a 403 that only describes one query (`insufficient_scope`, `access_denied`, +`invalid_request`) and GraphQL errors inside a 200 response do not start a pause. + +The pause is keyed to the token in the same way as the daily quota hold, so it +survives a restart and saving a different token in **Settings → API Keys** lifts +it at once. It also ends after one hour without a save: the next request is +sent as a probe and either clears the hold (Hardcover accepted the token) or +renews it. That is one request an hour for a token that stays bad. **Test +connection** always sends its request, so a token fixed on Hardcover's side can +be retested immediately, and a success lifts the pause. The pause applies to +every Hardcover call, including when Hardcover is only an enricher, a cover or +ISBN source or the list syncer. The hold is stored with the quota holds as a +private setting and is hidden from generic settings endpoints; a corrupt row is +logged and ignored. + ### Enhanced Hardcover series data deployment note **Schema:** enhanced series data uses migration `035`, which creates `series_hardcover_links` and backfills links for existing series whose foreign ID already points at Hardcover. Take a normal SQLite backup before upgrading, then let Bindery apply the migration on startup. diff --git a/internal/abs/daily_quota_test.go b/internal/abs/daily_quota_test.go index 3d3c28537..cc58e4c5b 100644 --- a/internal/abs/daily_quota_test.go +++ b/internal/abs/daily_quota_test.go @@ -13,11 +13,16 @@ type heldProvider struct { metadata.Provider checks int stopAfter int + // rejected makes the hold a rejected token instead of the daily quota. + rejected bool } func (p *heldProvider) CheckQuota(context.Context) error { p.checks++ if p.checks > p.stopAfter { + if p.rejected { + return &metadata.TokenRejectedError{Provider: "Hardcover"} + } return &metadata.DailyQuotaError{ResetAt: time.Date(2026, 9, 19, 1, 0, 0, 0, time.UTC)} } return nil @@ -46,3 +51,22 @@ func TestDailyQuotaStopsABSBeforeCheckpointingItem(t *testing.T) { }) } } + +func TestTokenRejectedStopsABSBeforeCheckpointingItem(t *testing.T) { + importer, _, _, _, _, _, _, _, _, _ := newABSImporterFixture(t) + importer.WithMetadata(metadata.NewAggregator(&heldProvider{stopAfter: 1, rejected: true})) + accepted := 0 + importer.enumerateFn = func(ctx context.Context, _ string, fn func(context.Context, NormalizedLibraryItem) error) (EnumerationStats, error) { + for range 3 { + if err := fn(ctx, sampleABSItem()); err != nil { + return EnumerationStats{}, err + } + accepted++ + } + return EnumerationStats{}, nil + } + _, err := importer.Run(context.Background(), ImportConfig{SourceID: DefaultSourceID, BaseURL: "https://abs.example.com", APIKey: "example-only", LibraryID: "lib-books", Enabled: true, DryRun: true}) + if err == nil || !strings.Contains(err.Error(), "Hardcover rejected the API token") || accepted != 0 { + t.Fatalf("accepted %d err %v", accepted, err) + } +} diff --git a/internal/api/rejected_token_test.go b/internal/api/rejected_token_test.go new file mode 100644 index 000000000..3340cfb03 --- /dev/null +++ b/internal/api/rejected_token_test.go @@ -0,0 +1,114 @@ +package api + +import ( + "context" + "crypto/hmac" + "crypto/sha256" + "encoding/hex" + "errors" + "fmt" + "net/http" + "net/http/httptest" + "strings" + "sync/atomic" + "testing" + "time" + + "github.com/vavallee/bindery/internal/db" + "github.com/vavallee/bindery/internal/httpsec" + "github.com/vavallee/bindery/internal/metadata" + "github.com/vavallee/bindery/internal/metadata/hardcover" +) + +// rejectedHoldRow stores a live rejected token hold for token under a known +// install secret, as a restart would find it. +func rejectedHoldRow(t *testing.T, settings *db.SettingsRepo, token string) { + t.Helper() + ctx := context.Background() + secret := strings.Repeat("ab", 32) + if err := settings.Set(ctx, "auth.hardcover_daily_hold_secret", secret); err != nil { + t.Fatal(err) + } + key, _ := hex.DecodeString(secret) + mac := hmac.New(sha256.New, key) + mac.Write([]byte(token)) + until := time.Now().UTC().Add(time.Hour).Format(time.RFC3339) + if err := settings.Set(ctx, "auth.hardcover_rejected_holds", fmt.Sprintf(`{"%x":%q}`, mac.Sum(nil), until)); err != nil { + t.Fatal(err) + } +} + +func TestRejectedTokenHoldDoesNotBlockTestConnection(t *testing.T) { + h, settings, ctx := settingsFixture(t) + token := "example-only" + if err := settings.Set(ctx, SettingHardcoverAPIToken, token); err != nil { + t.Fatal(err) + } + rejectedHoldRow(t, settings, token) + hold := hardcover.NewDailyQuota(settings) + h.WithDailyQuota(hold) + + // The same held token is refused for ordinary calls without a request. + var rejected *metadata.TokenRejectedError + if err := hardcover.New().WithDailyQuota(hold).WithToken(token).CheckQuota(ctx); !errors.As(err, &rejected) { + t.Fatalf("hold not live: %v", err) + } + // The test button must get past the hold. An outbound proxy that refuses + // every tunnel stands in for Hardcover and counts the attempts, so nothing + // leaves the machine. + t.Cleanup(func() { _, _ = httpsec.ConfigureOutboundProxy("", "", true) }) + var attempts atomic.Int32 + proxy := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + attempts.Add(1) + w.WriteHeader(http.StatusBadGateway) + })) + defer proxy.Close() + if _, err := httpsec.ConfigureOutboundProxy(proxy.URL, "", true); err != nil { + t.Fatal(err) + } + rec := httptest.NewRecorder() + h.TestHardcover(rec, httptest.NewRequest(http.MethodPost, "/api/v1/hardcover/test", nil)) + if body := rec.Body.String(); attempts.Load() == 0 || strings.Contains(body, "rejected the API token") { + t.Fatalf("test connection was blocked by the rejected hold (attempts %d): %s", attempts.Load(), body) + } +} + +func TestSavingNewTokenLiftsRejectedHold(t *testing.T) { + _, settings, ctx := settingsFixture(t) + rejectedHoldRow(t, settings, "old-token") + if err := settings.Set(ctx, SettingHardcoverAPIToken, "old-token"); err != nil { + t.Fatal(err) + } + // Wired as in cmd/bindery: the token is read per call from settings. + client := hardcover.New().WithDailyQuota(hardcover.NewDailyQuota(settings)).WithTokenSource(func(ctx context.Context) string { + return GetHardcoverAPIToken(ctx, settings) + }) + var rejected *metadata.TokenRejectedError + if err := client.CheckQuota(ctx); !errors.As(err, &rejected) { + t.Fatalf("hold not live: %v", err) + } + if err := settings.Set(ctx, SettingHardcoverAPIToken, "new-token"); err != nil { + t.Fatal(err) + } + if err := client.CheckQuota(ctx); err != nil { + t.Fatalf("new token still held: %v", err) + } +} + +func TestRejectedTokenHoldHiddenFromAdminSettings(t *testing.T) { + h, settings, ctx := settingsFixture(t) + key := "auth.hardcover_rejected_holds" + if err := settings.Set(ctx, key, `{"example-fingerprint":"2026-09-19T01:00:00Z"}`); err != nil { + t.Fatal(err) + } + rec := httptest.NewRecorder() + h.List(rec, adminReq(httptest.NewRequest(http.MethodGet, "/api/v1/setting", nil))) + if rec.Code != http.StatusOK || strings.Contains(rec.Body.String(), key) || strings.Contains(rec.Body.String(), "example-fingerprint") { + t.Fatalf("list: %d %s", rec.Code, rec.Body.String()) + } + rec = httptest.NewRecorder() + h.Get(rec, adminReq(withKey(httptest.NewRequest(http.MethodGet, "/api/v1/setting/"+key, nil), key))) + if rec.Code != http.StatusNotFound { + t.Fatalf("get: %d %s", rec.Code, rec.Body.String()) + } +} diff --git a/internal/api/settings_handler.go b/internal/api/settings_handler.go index aa9c5444b..9713d4931 100644 --- a/internal/api/settings_handler.go +++ b/internal/api/settings_handler.go @@ -314,6 +314,7 @@ func isSecretSetting(key string) bool { // 1. Explicit one-offs that don't match the patterns below. switch key { case "auth.hardcover_daily_holds", + "auth.hardcover_rejected_holds", SettingAuthAPIKey, SettingAuthSessionSecret, SettingAuthSessionSecretPrevious, @@ -679,6 +680,10 @@ func (h *SettingsHandler) TestHardcover(w http.ResponseWriter, r *http.Request) ctx, cancel := context.WithTimeout(r.Context(), 15*time.Second) defer cancel() + // A user retesting after fixing the token on Hardcover's side must reach + // Hardcover even while a rejected token hold is live (#3101). A daily quota + // hold still answers without a request. + ctx = hardcover.WithRejectedTokenProbe(ctx) client := hardcover.New().WithDailyQuota(h.dailyQuota).WithToken(token) series, err := client.SearchSeries(ctx, "Dune", 3) diff --git a/internal/hardcoverlistsyncer/rejected_token_test.go b/internal/hardcoverlistsyncer/rejected_token_test.go new file mode 100644 index 000000000..0b5eef6f0 --- /dev/null +++ b/internal/hardcoverlistsyncer/rejected_token_test.go @@ -0,0 +1,45 @@ +package hardcoverlistsyncer + +import ( + "context" + "crypto/hmac" + "crypto/sha256" + "encoding/hex" + "errors" + "fmt" + "strings" + "testing" + "time" + + "github.com/vavallee/bindery/internal/db" + "github.com/vavallee/bindery/internal/metadata" + "github.com/vavallee/bindery/internal/metadata/hardcover" +) + +func TestRejectedTokenListSyncStopsBeforeFetch(t *testing.T) { + database, err := db.OpenMemory() + if err != nil { + t.Fatal(err) + } + defer database.Close() + ctx := context.Background() + settings := db.NewSettingsRepo(database) + token := "example-only" + secret := strings.Repeat("ab", 32) + if err := settings.Set(ctx, "auth.hardcover_daily_hold_secret", secret); err != nil { + t.Fatal(err) + } + key, _ := hex.DecodeString(secret) + mac := hmac.New(sha256.New, key) + mac.Write([]byte(token)) + if err := settings.Set(ctx, "auth.hardcover_rejected_holds", fmt.Sprintf(`{"%x":%q}`, mac.Sum(nil), time.Now().Add(time.Hour).UTC().Format(time.RFC3339))); err != nil { + t.Fatal(err) + } + s := New(db.NewImportListRepo(database), db.NewAuthorRepo(database), db.NewBookRepo(database)).WithDailyQuota(hardcover.NewDailyQuota(settings)) + il := testImportList("Held", "hardcover", true) + il.APIKey = token + var rejected *metadata.TokenRejectedError + if err := s.syncList(ctx, il); !errors.As(err, &rejected) { + t.Fatalf("factory hold: %v", err) + } +} diff --git a/internal/metadata/aggregator.go b/internal/metadata/aggregator.go index c914b0776..df85c370b 100644 --- a/internal/metadata/aggregator.go +++ b/internal/metadata/aggregator.go @@ -920,8 +920,7 @@ func (a *Aggregator) GetBookByISBNWithOutcome(ctx context.Context, isbn string) if err != nil { // A held primary stops the lookup; a held enricher is just a failed // provider, so the primary's answer and later fallbacks still count. - var daily *DailyQuotaError - if idx == 0 && errors.As(err, &daily) { + if idx == 0 && IsProviderHold(err) { return nil, newSearchOutcome(primaryName, []string{normalizedProviderName(providerName(provider))}, answered, err), err } if errors.Is(err, ErrProviderNotConfigured) { diff --git a/internal/metadata/daily_quota.go b/internal/metadata/daily_quota.go index 5eb3a478f..b6acf3ce8 100644 --- a/internal/metadata/daily_quota.go +++ b/internal/metadata/daily_quota.go @@ -2,6 +2,7 @@ package metadata import ( "context" + "errors" "fmt" "time" ) @@ -14,6 +15,52 @@ func (e *DailyQuotaError) Error() string { return fmt.Sprintf("Hardcover daily quota exhausted; paused until %s; retry the job after this time", e.ResetAt.UTC().Format(time.RFC3339)) } +// TokenRejectedError means the provider answered an authentication failure for +// the configured API token. It is a long-lived pause like DailyQuotaError, but +// it ends when the token changes, not at a clock time, so it is its own type: +// a caller must not tell the user to wait for a reset that will never come. +// It never carries the token. Cause is the provider's own sanitised answer +// (for example "token rejected (HTTP 401: Invalid or expired token)") on the +// call that met the rejection, and nil for a hold served without a request. +type TokenRejectedError struct { + Provider string + Cause error +} + +func (e *TokenRejectedError) Error() string { + provider := e.Provider + if provider == "" { + provider = "provider" + } + msg := provider + " rejected the API token" + if e.Cause != nil { + msg = e.Cause.Error() + } + return msg + "; " + provider + " requests are paused until the token is changed in Settings" +} + +func (e *TokenRejectedError) Unwrap() error { return e.Cause } + +// ProviderHold returns err's DailyQuotaError or TokenRejectedError, or nil when +// err is neither. Every stop point for bulk work asks this instead of matching +// one type, so a new kind of hold cannot be wired into one consumer and missed +// by its sibling. +func ProviderHold(err error) error { + var daily *DailyQuotaError + if errors.As(err, &daily) { + return daily + } + var rejected *TokenRejectedError + if errors.As(err, &rejected) { + return rejected + } + return nil +} + +// IsProviderHold reports whether err is a long-lived provider pause that bulk +// work must stop on rather than retry for every row. +func IsProviderHold(err error) bool { return ProviderHold(err) != nil } + // CheckQuota lets bulk jobs stop while the primary provider is held. Enrichers // are not checked: bulk work does not need them, and their held calls already // fail fast without a request. No provider requests are made by this check. diff --git a/internal/metadata/daily_quota_test.go b/internal/metadata/daily_quota_test.go index 126a25c68..ca0bd26ce 100644 --- a/internal/metadata/daily_quota_test.go +++ b/internal/metadata/daily_quota_test.go @@ -77,3 +77,47 @@ func TestDailyQuotaISBNEnricherHoldKeepsOtherProviders(t *testing.T) { t.Fatalf("fallback answer: book %v err %v calls %d", book, err, fallback.getByISBNCalls) } } + +func TestTokenRejectedIsItsOwnHold(t *testing.T) { + rejected := &TokenRejectedError{Provider: "Hardcover", Cause: errors.New("token rejected (HTTP 401)")} + daily := &DailyQuotaError{ResetAt: time.Now().Add(time.Hour)} + for name, err := range map[string]error{ + "bare": rejected, + "wrapped": fmt.Errorf("lookup: %w", rejected), + "joined": errors.Join(rejected, errors.New("persist failed")), + } { + var asDaily *DailyQuotaError + if !IsProviderHold(err) || errors.As(err, &asDaily) || !errors.Is(ProviderHold(err), rejected) { + t.Errorf("%s: hold %v daily %v", name, IsProviderHold(err), errors.As(err, &asDaily)) + } + if errors.Is(err, ErrRateLimited) || IsProviderUnavailable(err) { + t.Errorf("%s: must not read as a throttle or an outage", name) + } + } + if !IsProviderHold(daily) || ProviderHold(errors.New("other")) != nil || IsProviderHold(nil) { + t.Fatal("ProviderHold misclassifies") + } + held := &TokenRejectedError{Provider: "Hardcover"} + if msg := held.Error(); !strings.Contains(msg, "Hardcover rejected the API token") || strings.Contains(msg, "reset") { + t.Fatal(msg) + } +} + +func TestTokenRejectedStopsPrimaryButNotEnricher(t *testing.T) { + ctx := context.Background() + rejected := &TokenRejectedError{Provider: "Hardcover"} + primary := &mockProvider{name: "hardcover", getByISBNErr: fmt.Errorf("lookup: %w", rejected)} + fallback := &mockProvider{name: "openlibrary"} + if _, _, err := NewAggregator(primary, fallback).GetBookByISBNWithOutcome(ctx, "9780441172719"); !errors.Is(err, rejected) || fallback.getByISBNCalls != 0 { + t.Fatalf("primary: err %v fallback calls %d", err, fallback.getByISBNCalls) + } + found := &models.Book{ForeignID: "OL1W", Title: "Dune", Description: strings.Repeat("d", 60)} + ol := &mockProvider{name: "openlibrary", getByISBN: found} + held := &mockProvider{name: "hardcover", getByISBNErr: fmt.Errorf("lookup: %w", rejected)} + if book, _, err := NewAggregator(ol, held).GetBookByISBNWithOutcome(ctx, "9780441172719"); err != nil || book == nil { + t.Fatalf("enricher hold stopped the lookup: %v %v", book, err) + } + if err := NewAggregator("aProvider{&mockProvider{name: "hardcover"}, rejected}).CheckQuota(ctx); !errors.Is(err, rejected) { + t.Fatalf("CheckQuota: %v", err) + } +} diff --git a/internal/metadata/hardcover/client.go b/internal/metadata/hardcover/client.go index 43ad22494..23b05a960 100644 --- a/internal/metadata/hardcover/client.go +++ b/internal/metadata/hardcover/client.go @@ -905,6 +905,18 @@ func (c *Client) query(ctx context.Context, q string, vars map[string]any, out i continue } + if rejected := tokenRejectionError(status, raw); rejected != nil { + // Every later call with this token would get the same answer, so + // record it and let every caller stop instead of asking per row. + return c.dailyQuota.rejectToken(ctx, c.token, rejected) + } + if status == http.StatusOK { + // Hardcover accepted the token (whatever the GraphQL layer says + // next), so a hold left by an earlier rejection is stale. + if err := c.dailyQuota.clearRejected(ctx, c.token); err != nil { + slog.Warn("clear Hardcover rejected token hold", "error", err) + } + } if status != http.StatusOK { statusErr := classifyHTTPError(status, raw) if status >= http.StatusInternalServerError { @@ -1078,6 +1090,28 @@ func classifyHTTPError(status int, raw []byte) error { } } +// tokenRejectionError returns the classified "token rejected" error when the +// response means Hardcover refused the credential itself, else nil. It is narrower than +// isTokenRejection on purpose: that classifier words an error for the user, this +// one starts a pause for every call, so it only takes the answers that no +// change to the query could fix. 401 counts. A 403 counts only when its body +// is an authentication failure (invalid_token, unauthorized, invalid_grant, or +// the "Unable to verify token" text), not insufficient_scope, access_denied or +// invalid_request, which can describe the one query. 5xx never counts. +func tokenRejectionError(status int, raw []byte) error { + envelope, parsed := parseHardcoverError(raw) + if !isTokenRejection(status, envelope, parsed) { + return nil + } + if status == http.StatusForbidden { + switch strings.ToLower(strings.TrimSpace(envelope.Error)) { + case "invalid_request", "access_denied", "insufficient_scope": + return nil + } + } + return classifyHTTPError(status, raw) +} + // parseHardcoverError decodes the error envelope, reporting whether the body // was a JSON object at all. A truncated or non-JSON body (an HTML error page // being the case that prompted #2128) reports false, and the caller then says diff --git a/internal/metadata/hardcover/csv_rejected_token_test.go b/internal/metadata/hardcover/csv_rejected_token_test.go new file mode 100644 index 000000000..4c2f5a249 --- /dev/null +++ b/internal/metadata/hardcover/csv_rejected_token_test.go @@ -0,0 +1,62 @@ +package hardcover_test + +import ( + "context" + "errors" + "io" + "net/http" + "strings" + "sync/atomic" + "testing" + + "github.com/vavallee/bindery/internal/db" + "github.com/vavallee/bindery/internal/metadata" + "github.com/vavallee/bindery/internal/metadata/hardcover" + "github.com/vavallee/bindery/internal/migrate" +) + +type countingTransport func(*http.Request) (*http.Response, error) + +func (f countingTransport) RoundTrip(r *http.Request) (*http.Response, error) { return f(r) } + +// TestCSVImportWithRejectedTokenAsksHardcoverOnce is the #3101 symptom: a +// bulk import with a rejected token used to send one doomed request per row. +func TestCSVImportWithRejectedTokenAsksHardcoverOnce(t *testing.T) { + database, err := db.OpenMemory() + if err != nil { + t.Fatal(err) + } + defer database.Close() + settings := db.NewSettingsRepo(database) + var requests atomic.Int32 + client := hardcover.New(). + WithDailyQuota(hardcover.NewDailyQuota(settings)). + WithToken("example-only-token"). + WithTransportForTest(countingTransport(func(*http.Request) (*http.Response, error) { + requests.Add(1) + return &http.Response{ + StatusCode: http.StatusUnauthorized, + Header: http.Header{"Content-Type": []string{"application/json"}}, + Body: io.NopCloser(strings.NewReader(`{"error":"invalid_token","error_description":"Invalid or expired token"}`)), + }, nil + })) + + var rows strings.Builder + rows.WriteString("name\n") + for _, name := range []string{"One", "Two", "Three", "Four", "Five", "Six", "Seven", "Eight", "Nine", "Ten"} { + rows.WriteString(name + " Author\n") + } + res, err := migrate.ImportCSVAuthors(context.Background(), strings.NewReader(rows.String()), + db.NewAuthorRepo(database), settings, metadata.NewAggregator(client), nil) + + var rejected *metadata.TokenRejectedError + if !errors.As(err, &rejected) { + t.Fatalf("import error = %v, want the rejected token to stop it", err) + } + if n := requests.Load(); n != 1 { + t.Fatalf("Hardcover saw %d requests for a 10 row import, want 1", n) + } + if res == nil || res.Added != 0 { + t.Fatalf("result = %+v", res) + } +} diff --git a/internal/metadata/hardcover/daily_quota.go b/internal/metadata/hardcover/daily_quota.go index 68b29f675..7c52e62fe 100644 --- a/internal/metadata/hardcover/daily_quota.go +++ b/internal/metadata/hardcover/daily_quota.go @@ -25,8 +25,33 @@ const ( // dailyHoldSecretSetting keys the token fingerprints. It is install-local // and separate from the session secret so rotating sessions keeps holds. dailyHoldSecretSetting = "auth.hardcover_daily_hold_secret" //nolint:gosec // setting key name, not a credential + // rejectedHoldSetting keys the rejected-token holds (#3101). Same + // fingerprints and the same secret as the daily holds, a separate row so a + // downgrade that only knows the daily row keeps reading it unchanged. + rejectedHoldSetting = "auth.hardcover_rejected_holds" + // rejectedHoldDuration is how long a rejected token stays refused without + // a request. It ends sooner when the token changes (a new token has a new + // key) or a probe succeeds. The cap exists so a 401 that was Hardcover's + // fault, or a token restored on Hardcover's side, heals without a restart + // or a Settings save, at the price of one request an hour. + rejectedHoldDuration = time.Hour ) +type rejectedProbeKey struct{} + +// WithRejectedTokenProbe marks ctx as a deliberate retest of a rejected token, +// which is how the Settings test button lets a user check a token again after +// fixing it on Hardcover's side. The request is sent even while a rejected +// hold is live, and a success clears the hold. A daily quota hold still applies. +func WithRejectedTokenProbe(ctx context.Context) context.Context { + return context.WithValue(ctx, rejectedProbeKey{}, true) +} + +func isRejectedTokenProbe(ctx context.Context) bool { + v, _ := ctx.Value(rejectedProbeKey{}).(bool) + return v +} + // DailyQuota remembers only server-reported exhaustion, not request usage. // All clients in a process share this store. Its lock never covers network I/O, // and after the first load it never covers a settings write either. @@ -36,9 +61,14 @@ type DailyQuota struct { settings *db.SettingsRepo secret []byte holds map[string]time.Time - // version counts hold changes; saved is the newest version on disk. - version uint64 - saved uint64 + // rejected holds tokens Hardcover refused, until the stored time (#3101). + rejected map[string]time.Time + // version counts hold changes; saved is the newest version on disk. The + // r-prefixed pair is the same for rejected. + version uint64 + saved uint64 + rversion uint64 + rsaved uint64 // writeMu orders settings writes so an older snapshot never lands last. writeMu sync.Mutex now func() time.Time @@ -73,11 +103,33 @@ func (q *DailyQuota) load(ctx context.Context) error { if holds == nil { holds = make(map[string]time.Time) } + rejected, err := q.loadRejected(ctx) + if err != nil { + return err + } q.secret = secret q.holds = holds + q.rejected = rejected return nil } +func (q *DailyQuota) loadRejected(ctx context.Context) (map[string]time.Time, error) { + rejected := make(map[string]time.Time) + row, err := q.settings.Get(ctx, rejectedHoldSetting) + if err != nil { + return nil, fmt.Errorf("read Hardcover rejected token hold: %w", err) + } + if row != nil { + if err := json.Unmarshal([]byte(row.Value), &rejected); err != nil || rejected == nil { + // Same reasoning as the daily row: failing closed would block every + // token for good and the API cannot write this row. + slog.Warn("Hardcover rejected token hold is corrupt; starting with no holds", "error", err) + rejected = make(map[string]time.Time) + } + } + return rejected, nil +} + // loadSecret reads the fingerprint key, creating it on first use and // replacing it when corrupt. func (q *DailyQuota) loadSecret(ctx context.Context) ([]byte, error) { @@ -146,8 +198,10 @@ func (q *DailyQuota) Check(ctx context.Context, token string) error { q.mu.Unlock() return err } - until := q.holds[q.tokenKey(token)] - pending := q.saved != q.version + key := q.tokenKey(token) + until := q.holds[key] + rejectedUntil := q.rejected[key] + pending := q.saved != q.version || q.rsaved != q.rversion now := q.now() q.mu.Unlock() @@ -155,6 +209,12 @@ func (q *DailyQuota) Check(ctx context.Context, token string) error { if until.After(now) { held = &metadata.DailyQuotaError{ResetAt: until} } + // A rejected token outranks a quota hold: waiting for a reset cannot help. + // After the hold lapses nothing is refused, so the next request is the + // probe; its 401 records a fresh hold before the next worker's second check. + if rejectedUntil.After(now) && !isRejectedTokenProbe(ctx) { + held = &metadata.TokenRejectedError{Provider: "Hardcover"} + } if !pending { return held } @@ -214,26 +274,106 @@ func (q *DailyQuota) observe(ctx context.Context, token string, headers http.Hea func (q *DailyQuota) persist(ctx context.Context) error { q.writeMu.Lock() defer q.writeMu.Unlock() + if err := q.persistSet(ctx, dailyHoldSetting, "daily hold", func() (map[string]time.Time, uint64, bool) { + return q.holds, q.version, q.saved != q.version + }, func(v uint64) { q.saved = v }); err != nil { + return err + } + return q.persistSet(ctx, rejectedHoldSetting, "rejected token hold", func() (map[string]time.Time, uint64, bool) { + return q.rejected, q.rversion, q.rsaved != q.rversion + }, func(v uint64) { q.rsaved = v }) +} + +// persistSet writes one hold map if it has unsaved changes. Callers hold +// writeMu; q.mu is held only to snapshot and to record the saved version. +func (q *DailyQuota) persistSet(ctx context.Context, setting, what string, snapshot func() (map[string]time.Time, uint64, bool), markSaved func(uint64)) error { q.mu.Lock() - if q.saved == q.version { + holds, version, dirty := snapshot() + if !dirty { q.mu.Unlock() return nil } - version := q.version - data, err := json.Marshal(q.holds) + data, err := json.Marshal(holds) q.mu.Unlock() if err != nil { - return fmt.Errorf("encode Hardcover daily hold: %w", err) + return fmt.Errorf("encode Hardcover %s: %w", what, err) } - if err := q.settings.Set(ctx, dailyHoldSetting, string(data)); err != nil { - return fmt.Errorf("persist Hardcover daily hold: %w", err) + if err := q.settings.Set(ctx, setting, string(data)); err != nil { + return fmt.Errorf("persist Hardcover %s: %w", what, err) } q.mu.Lock() - q.saved = version + markSaved(version) q.mu.Unlock() return nil } +// rejectToken records that Hardcover refused token and returns the error the +// current call should fail with. Nothing here talks to the network, and the +// lock is not held across the settings write. A persistence failure is joined +// to the rejection so the call still classifies as a hold; the write is retried +// by the next Check. +func (q *DailyQuota) rejectToken(ctx context.Context, token string, cause error) error { + rejection := &metadata.TokenRejectedError{Provider: "Hardcover", Cause: cause} + if q == nil || NormalizeAPIToken(token) == "" { + return rejection + } + saveCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), 5*time.Second) + defer cancel() + q.mu.Lock() + if err := q.load(saveCtx); err != nil { + q.mu.Unlock() + return errors.Join(rejection, err) + } + now := q.now() + key := q.tokenKey(token) + until := now.Add(rejectedHoldDuration) + if q.rejected[key].After(now) { + // Already held: concurrent workers that were admitted together each get + // their 401. Extend in memory, skip the write; a restart then probes a + // little sooner than it strictly had to. + q.rejected[key] = until + q.mu.Unlock() + return rejection + } + for k, hold := range q.rejected { + if !hold.After(now) { + delete(q.rejected, k) + } + } + q.rejected[key] = until + q.rversion++ + q.mu.Unlock() + slog.Warn("Hardcover rejected the API token; pausing Hardcover requests until the token is changed in Settings, with one retry an hour", "error", cause) + if err := q.persist(saveCtx); err != nil { + return errors.Join(rejection, err) + } + return rejection +} + +// clearRejected forgets a rejected hold after Hardcover accepted token. It is +// a map lookup in the common case of no hold. +func (q *DailyQuota) clearRejected(ctx context.Context, token string) error { + if q == nil || NormalizeAPIToken(token) == "" { + return nil + } + saveCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), 5*time.Second) + defer cancel() + q.mu.Lock() + if err := q.load(saveCtx); err != nil { + q.mu.Unlock() + return err + } + key := q.tokenKey(token) + if _, ok := q.rejected[key]; !ok { + q.mu.Unlock() + return nil + } + delete(q.rejected, key) + q.rversion++ + q.mu.Unlock() + return q.persist(saveCtx) +} + // dailyParameters ignores burst buckets and rejects malformed/duplicate values. func dailyParameters(value string) map[string]int64 { for _, entry := range strings.Split(value, ",") { diff --git a/internal/metadata/hardcover/export_test.go b/internal/metadata/hardcover/export_test.go new file mode 100644 index 000000000..87627d6fb --- /dev/null +++ b/internal/metadata/hardcover/export_test.go @@ -0,0 +1,11 @@ +package hardcover + +import "net/http" + +// WithTransportForTest lets the external test package point a real client at a +// counting transport without exporting the field in production code. +func (c *Client) WithTransportForTest(rt http.RoundTripper) *Client { + clone := *c + clone.http = &http.Client{Transport: rt} + return &clone +} diff --git a/internal/metadata/hardcover/rejected_token_test.go b/internal/metadata/hardcover/rejected_token_test.go new file mode 100644 index 000000000..7ffa68c22 --- /dev/null +++ b/internal/metadata/hardcover/rejected_token_test.go @@ -0,0 +1,320 @@ +package hardcover + +import ( + "context" + "errors" + "io" + "net/http" + "strings" + "sync" + "sync/atomic" + "testing" + "time" + + "github.com/vavallee/bindery/internal/db" + "github.com/vavallee/bindery/internal/metadata" +) + +const rejectedBody = `{"error":"invalid_token","error_description":"Invalid or expired token"}` + +// rejectedClient answers every request with status/body and counts them. +func rejectedClient(t *testing.T, q *DailyQuota, token string, status int, body string) (*Client, *atomic.Int32) { + t.Helper() + var calls atomic.Int32 + th, _ := newFakeThrottle() + c := New().WithDailyQuota(q).WithToken(token) + c.throttle = th + c.http = &http.Client{Transport: dailyTransport(func(*http.Request) (*http.Response, error) { + calls.Add(1) + return &http.Response{StatusCode: status, Header: make(http.Header), Body: io.NopCloser(strings.NewReader(body))}, nil + })} + return c, &calls +} + +func TestRejectedTokenIsAskedOnce(t *testing.T) { + q, _ := dailyFixture(t) + c, calls := rejectedClient(t, q, "example-only-token", 401, rejectedBody) + var out any + for i := range 20 { + err := c.query(context.Background(), "query", nil, &out) + var rejected *metadata.TokenRejectedError + var daily *metadata.DailyQuotaError + if !errors.As(err, &rejected) || errors.As(err, &daily) { + t.Fatalf("call %d: %v", i, err) + } + if strings.Contains(err.Error(), "example-only-token") { + t.Fatalf("error leaks the token: %v", err) + } + if i == 0 && !strings.Contains(err.Error(), "HTTP 401") { + t.Fatalf("first answer lost the upstream reason: %v", err) + } + if !strings.Contains(err.Error(), "Hardcover") || !strings.Contains(err.Error(), "changed in Settings") { + t.Fatalf("wording: %v", err) + } + if errors.Is(err, metadata.ErrRateLimited) || metadata.IsProviderUnavailable(err) { + t.Fatalf("a rejected token is neither a throttle nor an outage: %v", err) + } + } + if n := calls.Load(); n != 1 { + t.Fatalf("requests = %d, want 1", n) + } + if err := c.CheckQuota(context.Background()); !metadata.IsProviderHold(err) { + t.Fatalf("CheckQuota = %v", err) + } +} + +func TestRejectedTokenHoldSurvivesRestartAndKeysByToken(t *testing.T) { + ctx := context.Background() + q, settings := dailyFixture(t) + c, calls := rejectedClient(t, q, "bad-token", 401, rejectedBody) + var out any + _ = c.query(ctx, "query", nil, &out) + if row, err := settings.Get(ctx, rejectedHoldSetting); err != nil || row == nil || strings.Contains(row.Value, "bad-token") { + t.Fatalf("row = %+v err %v", row, err) + } + restarted := NewDailyQuota(settings) + c2, calls2 := rejectedClient(t, restarted, "Bearer bad-token", 401, rejectedBody) + for range 5 { + if err := c2.query(ctx, "query", nil, &out); !metadata.IsProviderHold(err) { + t.Fatal(err) + } + } + if calls.Load() != 1 || calls2.Load() != 0 { + t.Fatalf("requests before restart %d, after restart %d", calls.Load(), calls2.Load()) + } + // Saving a different token is the whole "resume when the token changes". + c3, calls3 := rejectedClient(t, restarted, "good-token", 200, `{"data":{}}`) + if err := c3.query(ctx, "query", nil, &out); err != nil || calls3.Load() != 1 { + t.Fatalf("new token: %v calls %d", err, calls3.Load()) + } + if err := restarted.Check(ctx, "bad-token"); !metadata.IsProviderHold(err) { + t.Fatalf("old token hold lost: %v", err) + } +} + +func TestRejectedTokenOnlyForAuthenticationFailures(t *testing.T) { + for _, tc := range []struct { + name string + status int + body string + hold bool + }{ + {"401", 401, ``, true}, + {"403 invalid_token", 403, rejectedBody, true}, + {"403 unable to verify", 403, `{"error":"Unable to verify token"}`, true}, + {"403 refused operator", 403, `{"error":"ilike and related operations are not permitted on this server."}`, false}, + {"403 insufficient scope", 403, `{"error":"insufficient_scope"}`, false}, + {"403 token bucket", 403, `{"error":"Token bucket exhausted, retry after 30s"}`, false}, + {"500", 500, `{"error":"boom"}`, false}, + {"500 mentions token", 500, `{"error":"invalid token cache"}`, false}, + {"401 from auth server outage as 503", 503, rejectedBody, false}, + {"graphql auth error in a 200", 200, `{"errors":[{"message":"Unable to verify token"}]}`, false}, + } { + t.Run(tc.name, func(t *testing.T) { + q, _ := dailyFixture(t) + c, calls := rejectedClient(t, q, "tok", tc.status, tc.body) + var out any + for range 3 { + _ = c.query(context.Background(), "query", nil, &out) + } + if err := c.CheckQuota(context.Background()); metadata.IsProviderHold(err) != tc.hold { + t.Fatalf("hold = %v, want %v (%v)", !tc.hold, tc.hold, err) + } + if tc.hold && calls.Load() != 1 { + t.Fatalf("requests = %d, want 1", calls.Load()) + } + }) + } +} + +func TestRejectedTokenProbeAndRecovery(t *testing.T) { + ctx := context.Background() + q, settings := dailyFixture(t) + now := time.Now().UTC() + q.now = func() time.Time { return now } + var status atomic.Int32 + status.Store(401) + var calls atomic.Int32 + th, _ := newFakeThrottle() + c := New().WithDailyQuota(q).WithToken("tok") + c.throttle = th + c.http = &http.Client{Transport: dailyTransport(func(*http.Request) (*http.Response, error) { + calls.Add(1) + body := rejectedBody + if status.Load() == 200 { + body = `{"data":{}}` + } + return &http.Response{StatusCode: int(status.Load()), Header: make(http.Header), Body: io.NopCloser(strings.NewReader(body))}, nil + })} + var out any + _ = c.query(ctx, "query", nil, &out) + _ = c.query(ctx, "query", nil, &out) + if calls.Load() != 1 { + t.Fatalf("calls = %d", calls.Load()) + } + // A deliberate retest reaches Hardcover while the hold is live, and a + // rejection renews the hold without adding a second request for others. + if err := c.query(WithRejectedTokenProbe(ctx), "query", nil, &out); !metadata.IsProviderHold(err) || calls.Load() != 2 { + t.Fatalf("probe: %v calls %d", err, calls.Load()) + } + if err := c.query(ctx, "query", nil, &out); !metadata.IsProviderHold(err) || calls.Load() != 2 { + t.Fatalf("after probe: %v calls %d", err, calls.Load()) + } + // Token fixed on Hardcover's side: the probe succeeds and lifts the hold. + status.Store(200) + if err := c.query(WithRejectedTokenProbe(ctx), "query", nil, &out); err != nil { + t.Fatal(err) + } + if err := c.query(ctx, "query", nil, &out); err != nil { + t.Fatalf("hold survived a successful probe: %v", err) + } + if err := NewDailyQuota(settings).Check(ctx, "tok"); err != nil { + t.Fatalf("cleared hold persisted as live: %v", err) + } + // Without any retest the hold lapses on its own after the cap. + status.Store(401) + calls.Store(0) + _ = c.query(ctx, "query", nil, &out) + now = now.Add(time.Hour - time.Minute) + _ = c.query(ctx, "query", nil, &out) + if calls.Load() != 1 { + t.Fatalf("early probe: calls %d", calls.Load()) + } + now = now.Add(2 * time.Minute) + _ = c.query(ctx, "query", nil, &out) + if calls.Load() != 2 { + t.Fatalf("no probe after the cap: calls %d", calls.Load()) + } +} + +func TestRejectedTokenAppliesToEveryClientCall(t *testing.T) { + q, _ := dailyFixture(t) + c, calls := rejectedClient(t, q, "tok", 401, rejectedBody) + ctx := context.Background() + if _, err := c.SearchAuthors(ctx, "Example"); !metadata.IsProviderHold(err) { + t.Fatal(err) + } + calls.Store(0) + checks := map[string]func() error{ + "SearchBooks": func() error { _, err := c.SearchBooks(ctx, "Example"); return err }, + "GetAuthor": func() error { _, err := c.GetAuthor(ctx, "hc:1"); return err }, + "GetAuthorWorks": func() error { _, err := c.GetAuthorWorks(ctx, "hc:1"); return err }, + "GetBook": func() error { _, err := c.GetBook(ctx, "hc:1"); return err }, + "GetEditions": func() error { _, err := c.GetEditions(ctx, "hc:1"); return err }, + "GetBookByISBN": func() error { _, err := c.GetBookByISBN(ctx, "9780441172719"); return err }, + "SearchSeries": func() error { _, err := c.SearchSeries(ctx, "Dune", 3); return err }, + "GetSeriesCatalog": func() error { _, err := c.GetSeriesCatalog(ctx, "1"); return err }, + "GetUserLists": func() error { _, err := c.GetUserLists(ctx); return err }, + "GetListBooks": func() error { _, err := c.GetListBooks(ctx, 1); return err }, + "GetUserWishlist": func() error { _, err := c.GetUserWishlist(ctx, 10); return err }, + } + for name, call := range checks { + if err := call(); !metadata.IsProviderHold(err) { + t.Errorf("%s: %v", name, err) + } + } + if calls.Load() != 0 { + t.Fatalf("held client sent %d requests", calls.Load()) + } +} + +func TestRejectedTokenAsPrimary(t *testing.T) { + ctx := context.Background() + q, _ := dailyFixture(t) + c, calls := rejectedClient(t, q, "tok", 401, rejectedBody) + _, _ = c.GetBookByISBN(ctx, "9780441172719") + calls.Store(0) + // Behind the aggregator as primary, nothing is sent. The enricher role + // is the same client methods, covered by TestRejectedTokenAppliesToEveryClientCall. + primary := metadata.NewAggregator(c) + if err := primary.CheckQuota(ctx); !metadata.IsProviderHold(err) { + t.Fatalf("primary CheckQuota = %v", err) + } + if _, _, err := primary.GetBookByISBNWithOutcome(ctx, "9780441172719"); !metadata.IsProviderHold(err) { + t.Fatalf("primary isbn: %v", err) + } + if calls.Load() != 0 { + t.Fatalf("held primary sent %d requests", calls.Load()) + } +} + +func TestRejectedTokenConcurrentWorkers(t *testing.T) { + ctx := context.Background() + q, _ := dailyFixture(t) + var calls atomic.Int32 + c := New().WithDailyQuota(q).WithToken("tok") + c.throttle = &throttle{now: time.Now, sleep: func(context.Context, time.Duration) error { return nil }} + gate := make(chan struct{}) + c.http = &http.Client{Transport: dailyTransport(func(*http.Request) (*http.Response, error) { + calls.Add(1) + <-gate // every worker is admitted before the first answer lands + return &http.Response{StatusCode: 401, Header: make(http.Header), Body: io.NopCloser(strings.NewReader(rejectedBody))}, nil + })} + var wg sync.WaitGroup + errs := make(chan error, 32) + for range 32 { + wg.Add(1) + go func() { + defer wg.Done() + var out any + errs <- c.query(ctx, "query", nil, &out) + }() + } + time.Sleep(100 * time.Millisecond) + close(gate) + wg.Wait() + close(errs) + for err := range errs { + if !metadata.IsProviderHold(err) { + t.Fatal(err) + } + } + var out any + before := calls.Load() + if err := c.query(ctx, "query", nil, &out); !metadata.IsProviderHold(err) || calls.Load() != before { + t.Fatalf("late caller: %v", err) + } + if before > 32 { + t.Fatalf("calls %d", before) + } +} + +func TestRejectedTokenCorruptRowAndFailedWrite(t *testing.T) { + ctx := context.Background() + database, err := db.OpenMemory() + if err != nil { + t.Fatal(err) + } + defer database.Close() + settings := db.NewSettingsRepo(database) + q := NewDailyQuota(settings) + if err := settings.Set(ctx, rejectedHoldSetting, `not json`); err != nil { + t.Fatal(err) + } + if err := q.Check(ctx, "tok"); err != nil { + t.Fatalf("corrupt row blocked admission: %v", err) + } + if _, err := database.Exec(`CREATE TRIGGER fail_rejected_write BEFORE INSERT ON settings BEGIN SELECT RAISE(FAIL, 'temporary write failure'); END`); err != nil { + t.Fatal(err) + } + if _, err := database.Exec(`DELETE FROM settings WHERE key = ?`, rejectedHoldSetting); err != nil { + t.Fatal(err) + } + err = q.rejectToken(ctx, "tok", errors.New("token rejected (HTTP 401)")) + var rejected *metadata.TokenRejectedError + if !errors.As(err, &rejected) || !strings.Contains(err.Error(), "temporary write failure") { + t.Fatalf("write failure hidden or hold lost: %v", err) + } + if err := q.Check(ctx, "other"); err == nil || !strings.Contains(err.Error(), "temporary write failure") { + t.Fatalf("pending write was not retried before admitting: %v", err) + } + if _, err := database.Exec(`DROP TRIGGER fail_rejected_write`); err != nil { + t.Fatal(err) + } + if err := q.Check(ctx, "other"); err != nil { + t.Fatal(err) + } + if err := NewDailyQuota(settings).Check(ctx, "tok"); !errors.As(err, &rejected) { + t.Fatalf("retried hold not durable: %v", err) + } +} diff --git a/internal/migrate/authors.go b/internal/migrate/authors.go index 373f15494..b0238d1af 100644 --- a/internal/migrate/authors.go +++ b/internal/migrate/authors.go @@ -42,9 +42,8 @@ func refusedBindReason(o metadata.SearchOutcome, foreignID string) string { // verdict on the row, which may well match once the primary is back, so it // says to retry rather than to fix anything. func primaryDownReason(o metadata.SearchOutcome) string { - var daily *metadata.DailyQuotaError - if errors.As(o.FirstErr, &daily) { - return daily.Error() + if hold := metadata.ProviderHold(o.FirstErr); hold != nil { + return hold.Error() } return fmt.Sprintf("primary metadata provider %s did not answer, run the import again once it responds", o.Primary) } diff --git a/internal/migrate/daily_quota_test.go b/internal/migrate/daily_quota_test.go index aeba167c8..3f9b388db 100644 --- a/internal/migrate/daily_quota_test.go +++ b/internal/migrate/daily_quota_test.go @@ -146,3 +146,37 @@ func TestDailyQuotaFailedCatalogueReadCountsOnce(t *testing.T) { t.Fatalf("errors=%d skipped=%d, want 1 and 0", res.Errors, res.Skipped) } } + +func TestTokenRejectedTripsImportBreakerAndSaysWhy(t *testing.T) { + rejected := &metadata.TokenRejectedError{Provider: "Hardcover"} + p := &primaryOutage{} + p.observe("goodreads", metadata.SearchOutcome{Primary: "hardcover", PrimaryFailed: true, FirstErr: fmt.Errorf("lookup: %w", rejected)}) + if !p.down() || !strings.Contains(p.reason(), "Hardcover rejected the API token") { + t.Fatalf("down %v reason %s", p.down(), p.reason()) + } + // A rejected enricher alone must not stop an import the primary can serve. + q := &primaryOutage{} + held := metadata.SearchOutcome{Primary: "openlibrary", Answered: []string{"openlibrary"}, FailedProviders: []string{"hardcover"}, FirstErr: fmt.Errorf("hardcover: %w", rejected)} + for range primaryOutageThreshold + 1 { + q.observe("csv", held) + } + if q.down() { + t.Fatal("rejected enricher tripped the breaker") + } +} + +func TestTokenRejectedStopsCSVAtFirstRow(t *testing.T) { + database := newTestDB(t) + p := &dailyProvider{} + calls := 0 + p.searchAuthorsFn = func(context.Context, string) ([]models.Author, error) { + calls++ + p.held = &metadata.TokenRejectedError{Provider: "Hardcover"} + return nil, p.held + } + _, err := ImportCSVAuthors(context.Background(), strings.NewReader("name\nFirst\nSecond\nThird\n"), db.NewAuthorRepo(database), db.NewSettingsRepo(database), metadata.NewAggregator(p), nil) + var rejected *metadata.TokenRejectedError + if !errors.As(err, &rejected) || calls != 1 { + t.Fatalf("calls %d err %v", calls, err) + } +} diff --git a/internal/migrate/outage.go b/internal/migrate/outage.go index f504d2242..aa37efa65 100644 --- a/internal/migrate/outage.go +++ b/internal/migrate/outage.go @@ -62,11 +62,10 @@ type primaryOutage struct { // provider order with the primary first, so FirstErr is the primary's error // whenever PrimaryFailed is set. func (p *primaryOutage) observe(source string, o metadata.SearchOutcome) { - var daily *metadata.DailyQuotaError switch { // Only the primary's hold stops the import. A held enricher's error can be // FirstErr while the primary answered, and that lookup is fine. - case o.PrimaryFailed && errors.As(o.FirstErr, &daily): + case o.PrimaryFailed && metadata.IsProviderHold(o.FirstErr): p.streak, p.last = primaryOutageThreshold, o case o.Primary != "" && slices.Contains(o.Answered, o.Primary): p.streak = 0 diff --git a/internal/scheduler/daily_quota_test.go b/internal/scheduler/daily_quota_test.go index 6c0bd364e..c7bcfa279 100644 --- a/internal/scheduler/daily_quota_test.go +++ b/internal/scheduler/daily_quota_test.go @@ -38,3 +38,33 @@ func TestDailyQuotaRefreshStopsBeforeProviderCalls(t *testing.T) { t.Fatalf("continued walking authors: %d checks", p.checks) } } + +type rejectedRefreshProvider struct { + metadata.Provider + checks int +} + +func (p *rejectedRefreshProvider) CheckQuota(context.Context) error { + p.checks++ + return &metadata.TokenRejectedError{Provider: "Hardcover"} +} + +func TestTokenRejectedRefreshStopsBeforeProviderCalls(t *testing.T) { + database, err := db.OpenMemory() + if err != nil { + t.Fatal(err) + } + defer database.Close() + authors := db.NewAuthorRepo(database) + for _, name := range []string{"First", "Second"} { + if err := authors.Create(context.Background(), &models.Author{Name: name, ForeignID: "hc:" + name, Monitored: true}); err != nil { + t.Fatal(err) + } + } + p := &rejectedRefreshProvider{} + s := &Scheduler{authors: authors, meta: metadata.NewAggregator(p)} + s.refreshMetadata() + if p.checks != 1 { + t.Fatalf("continued walking authors: %d checks", p.checks) + } +} diff --git a/web/src/api/system.ts b/web/src/api/system.ts index f4f748360..2fb75dce8 100644 --- a/web/src/api/system.ts +++ b/web/src/api/system.ts @@ -11,6 +11,8 @@ export interface SystemStatus { enhancedHardcoverApi: boolean hardcoverTokenConfigured: boolean hardcoverPausedUntil?: string + /** True while Hardcover refused the saved token and requests are paused until it changes. */ + hardcoverTokenRejected?: boolean enhancedHardcoverDisabledReason?: 'env_disabled' | 'missing_token' | 'admin_disabled' | string } diff --git a/web/src/i18n/locales/en.json b/web/src/i18n/locales/en.json index 4081a7054..feef7e030 100644 --- a/web/src/i18n/locales/en.json +++ b/web/src/i18n/locales/en.json @@ -1399,6 +1399,7 @@ "deleteBackupTitle": "Delete backup" }, "general": { + "hardcoverTokenRejected": "Hardcover rejected the API token, so Hardcover requests are paused. Save a working token below, or use Test connection once it is fixed on Hardcover.", "hardcoverPausedUntil": "Hardcover is paused until {{time}} because its daily quota was reached. Retry interrupted imports after this time.", "appearance": "Appearance", "theme": "Theme", diff --git a/web/src/pages/SettingsPage.test.tsx b/web/src/pages/SettingsPage.test.tsx index ee1315b41..c3fc80aa9 100644 --- a/web/src/pages/SettingsPage.test.tsx +++ b/web/src/pages/SettingsPage.test.tsx @@ -448,6 +448,14 @@ describe('SettingsPage', () => { else expect(screen.queryByText('settings.general.hardcoverPausedUntil')).not.toBeInTheDocument() }) + it.each([true, false])('shows the rejected Hardcover token notice only when set (%s)', async rejected => { + renderSettings({ status: { ...defaultStatus, hardcoverTokenRejected: rejected } }) + await openApiKeysTab() + await screen.findByText('Hardcover API Token') + if (rejected) expect(await screen.findByText('settings.general.hardcoverTokenRejected')).toBeInTheDocument() + else expect(screen.queryByText('settings.general.hardcoverTokenRejected')).not.toBeInTheDocument() + }) + it('adds a write-only Hardcover token field with API link', async () => { renderSettings() await openApiKeysTab() diff --git a/web/src/pages/settings/ApiKeysTab.tsx b/web/src/pages/settings/ApiKeysTab.tsx index 68dce10d7..467f3c77f 100644 --- a/web/src/pages/settings/ApiKeysTab.tsx +++ b/web/src/pages/settings/ApiKeysTab.tsx @@ -170,6 +170,11 @@ export default function ApiKeysTab() { {t('settings.general.hardcoverPausedUntil', { time: new Date(systemStatus.hardcoverPausedUntil).toLocaleString() })}

)} + {systemStatus?.hardcoverTokenRejected && ( +

+ {t('settings.general.hardcoverTokenRejected')} +

+ )}