Adopt HTTPStatusError across all API clients; fix fatal-error classification

[joestump]

What / Why

Part of epic #318. internal/services/resilience.go defined a typed HTTPStatusError that no client constructed, so error classification ran entirely on string matching — and the fatal patterns ("status 401") did not match Navidrome's real format ("navidrome API returned status: 401", with a colon). Revoked Navidrome credentials were classified retriable, retried forever on capped backoff, and never triggered the fatal toast or the ADR-0026 sync-failure email.

Governing artifacts: ADR-0020 (exponential backoff and circuit breaker), ADR-0026 (sync-failure email notifications), SPEC error-handling REQ-ERR-003.

Changes

• New internal/resilience package containing HTTPStatusError + NewHTTPStatusError. This is required because internal/services imports internal/providers, so providers/enrichers cannot import internal/services without an import cycle. The package location matches the spec's implementation note ("new struct in internal/services/ or internal/resilience/"). internal/services keeps HTTPStatusError as a type alias and re-exports the constructor, so all existing call sites and tests are unchanged.
• Typed adoption in every client: all non-2xx responses in the Navidrome provider (login, internal API, all Subsonic call sites), Spotify provider, Last.fm provider, the Spotify, Last.fm, MusicBrainz (+ Cover Art Archive), Fanart.tv, Navidrome, and Lidarr enrichers, and the shared image downloader (internal/enrichers/images.go) are wrapped in resilience.NewHTTPStatusError. Error() delegates to the inner error, so log output and message-based behavior are byte-for-byte unchanged.
• ClassifyError prefers the typed path; string matching is now explicitly a fallback and extended (via a (status|error):? *(401|403) regex) to match the formats clients actually emit, including Navidrome's colon variant and Lidarr's api error: 401 - ... format. In the fallback, fatal patterns are checked before retriable substrings so a fatal status embedded in a message wins even when the appended response body contains a retriable-looking word (e.g. status 401: ...timeout...).
• Explicit status-code decisions in classifyHTTPStatus (this branch went live once clients started constructing the typed error, so each 4xx is now deliberate):
  • 408 → retriable. It is a timeout; REQ-ERR-002 requires timeouts to be retriable.
  • 404 → retriable (deliberate). Reverse proxies (e.g. Traefik) return transient 404s while a backend's route is dropped during a container redeploy; a fatal classification would permanently stop sync with a misleading "reconnect credentials" email. A genuinely wrong base URL keeps retrying at the 30-minute backoff cap instead — invalid configuration is REQ-ERR-003's concern at config-validation time, not per-request. The string-fallback regex excludes 404 for the same reason, keeping typed and fallback paths consistent. Both pinned with tests.
  • 401/403 → fatal (REQ-ERR-003); remaining 4xx (400, 405, 422, ...) → fatal — the request itself is wrong and will not succeed on retry; 5xx → retriable.
• REQ-ERR-003: JSON/XML decode errors (json.SyntaxError, json.UnmarshalTypeError, xml.SyntaxError) classify fatal (unparseable response body = API contract change) instead of falling through to default-retriable. Truncated-body I/O errors (io.EOF/ErrUnexpectedEOF) deliberately remain retriable — those indicate transient network truncation, not a contract change.
• Backoff-scenario math — decision: fixed the code. CalculateBackoff now uses 2^(consecutiveFailures-1), so with RecordFailure's increment-before-calculate the first retry waits ~30s, then ~60s, ~120s — matching SPEC error-handling Scenarios 1-2 and ADR-0020's consequences section ("30s, 60s, 120s, ... up to 30 minutes"). The previous code produced 60/120/240s. No spec amendment needed for the scenarios; see deferred notes below on REQ-BACK-001 wording.

Test evidence

• make test (full suite) passes; go test ./internal/services/... -race -count=1 passes.
• gofmt -l . clean; go vet ./internal/... clean.
• Acceptance grep: grep -rn "NewHTTPStatusError" internal/providers internal/enrichers shows adoption in every client (25 call sites including the shared image downloader).
• Key tests:
  • TestSyncer_RevokedNavidromeCredentials_FatalStopsRetryAndNotifies — end-to-end: a 401 formatted exactly as the Navidrome client emits it classifies ErrorClassFatal, the provider is not called again on the next sync (backoff stops), and NotifyIfNeeded fires exactly once.
  • TestSyncer_LegacyNavidromeErrorString_FallbackClassifiesFatal — same guarantees through the string fallback for unwrapped errors.
  • TestDBNotifier_SendsOnRevokedNavidromeCredentials — the real DBNotifier sends the ADR-0026 email for both the typed error and the legacy string format.
  • TestClassifyError_RealWorldClientFormats — fatal for every real client format (Navidrome colon, Last.fm, Lidarr, Fanart.tv); 5xx/404 variants of the same formats stay retriable.
  • TestClassifyError_Transient404_Typed, TestClassifyError_RequestTimeout408_Typed, TestClassifyError_FatalPatternWinsOverRetriableSubstring — pin the review-driven decisions.
  • TestClassifyError_DecodeErrorsFatal, TestCalculateBackoff_SpecScenarioProgression, TestBackoffManager_FirstFailureDelayMatchesSpec.

Deferred Design Doc Updates

• SPEC error-handling REQ-BACK-001 states delay = min(baseDelay * 2^consecutiveFailures, maxDelay), which literally contradicts the spec's own Scenarios 1-2 (first failure → ~30s implies 2^(n-1) when n is the post-increment failure count). The code follows the scenarios and ADR-0020. Suggest amending REQ-BACK-001 to 2^(consecutiveFailures-1) or clarifying that consecutiveFailures in the formula means failures before the current one.
• SPEC error-handling REQ-BACK-001 vs REQ-BACK-002 self-contradiction on jitter-after-clamp: the formula applies jitterFactor after the min(..., maxDelay) clamp, so at the cap the actual delay ranges up to 37.5 minutes, while REQ-BACK-002 says the delay "MUST NOT exceed 30 minutes regardless". The code (and pre-existing tests) apply jitter after the clamp per the REQ-BACK-001 formula. Suggest amending REQ-BACK-002 to "MUST NOT exceed 30 minutes before jitter" or clamping after jitter in a follow-up.
• Consider documenting the 404-retriable and 408-retriable decisions in SPEC error-handling REQ-ERR-002 (neither status is currently enumerated).

(Spec files are protected paths — not edited here.)

Closes #325

Refs: #318, ADR-0020, ADR-0026, SPEC error-handling

🤖 Posted on behalf of @joestump by Claude.

[joestump]

Adversarial review — PR #354

Verdict: CHANGES NEEDED — the headline fix (revoked Navidrome credentials → fatal → notification, backoff stops) is correct, well-tested end-to-end, and I reproduced the green suite (make test, go test ./internal/services/... ./internal/notifications/... -race -count=1, go vet, gofmt -l all clean in a detached checkout of f3b93c4). One real defect needs fixing before merge; everything else is minor.

Blocking

1. classifyHTTPStatus default 4xx → fatal is now live code, and it misclassifies 408 (and arguably 404) — internal/services/resilience.go:117-133.
Before this PR nothing constructed HTTPStatusError, so this switch was dead. Now every non-2xx from every client hits it, which activates two previously-unreachable behaviors:

• HTTP 408 (Request Timeout) → fatal. A timeout is transient by definition; SPEC error-handling REQ-ERR-002 lists "network timeout" as MUST-retriable. A single 408 from any provider now sets the fatal flag, stops sync until user action, and fires the ADR-0026 email. Fix: add http.StatusRequestTimeout to the retriable case.
• HTTP 404 → fatal, a behavior change for Navidrome specifically. REQ-ERR-003 does not list 404 as fatal (only 401/403, invalid config, unparseable body, OAuth refresh). Before this PR, Navidrome's "navidrome API returned status: 404" matched no fatal pattern → retriable; now it is fatal via both the typed path and the new regex. Failure scenario: Navidrome behind Traefik/label-based discovery during a container redeploy — the route disappears and the proxy returns 404 for ~seconds; sync is now permanently blocked (until reconnect or process restart) and the user gets a misleading "reconnect your credentials" email. Counterpoint: a well-formed-but-wrong base URL also manifests as 404, which supports fatal (misconfiguration detection, and "status 404" was already a fatal string pattern for Spotify-format messages pre-PR). Either stance is defensible — but make it an explicit, tested decision: either map 404 → retriable in classifyHTTPStatus (and drop 404 from fatalStatusPattern), or keep it fatal with a comment justifying it against the transient-proxy-404 scenario and a test pinning typed 404 behavior (current tests only cover the string path at resilience_test.go:114).

Non-blocking

2. internal/enrichers/images.go:59 is the sole remaining unwrapped status site, and its message now matches the fatal regex. "failed to download image, status: %s" with resp.Status = 404 Not Found lowercases to "...status: 404 not found" → fatalStatusPattern match → fatal. Today this is harmless — I traced every ClassifyError caller (sync.go:298, sync.go:401, notifier.go:71, templates.go:38) and enricher/image errors never reach classification; metadata.go (e.g. :710) only logs Warn. But #331/#333 will consolidate retry paths, and a missing image URL classifying provider-fatal would be a nasty latent surprise. Wrap it in resilience.NewHTTPStatusError now (or make the message not match), so its classification is deliberate rather than regex-accidental.

3. Fallback ordering: retriable substrings are checked before the fatal regex — resilience.go:104-108. An unwrapped message like "last.fm api returned status 401: ...timeout..." (last.fm/lidarr formats append response bodies) contains "timeout"/"eof" → classifies retriable despite the 401. Typed wrapping shields all real clients now, and the ordering is pre-existing, but since the regex was added for exactly these formats, consider checking fatalStatusPattern before the retriable substring scan in the fallback.

4. Regex looseness — resilience.go:169. (?:status|error):? *(?:401|403|404)\b matches with zero separator ("error401") and generic prose ("error 404" inside a track/playlist title embedded in an unwrapped message). Low practical risk given the typed path dominates; optionally require a separator or anchor on the known shapes (returned status, failed with status, api error:). Checked for false negatives: no "status code %d" style messages exist in the repo; last.fm api error: %d uses Last.fm API codes (≤ ~30), so no collision with 401/403/404.

5. internal/services/lidarr_submitter.go:660 still builds unwrapped "lidarr api error: %d - %s". It lives in the separate ComputeBackoff subsystem (out of #325's scope), but its message matches the new fatal regex — wrap it during the #331 consolidation.

6. Decode-errors-fatal: spec-sanctioned, carve-out verified. REQ-ERR-003 explicitly makes unparseable bodies fatal, so classifying json.SyntaxError/UnmarshalTypeError/xml.SyntaxError fatal is correct. I verified the io.ErrUnexpectedEOF carve-out actually holds: clients either io.ReadAll first (truncation surfaces as an I/O error before unmarshal) or use json.Decoder (truncation → io.ErrUnexpectedEOF), both retriable. Residual accepted risk worth knowing: a proxy serving an HTML error page with HTTP 200 → json.SyntaxError → fatal + email even though transient. That is what the spec asks for; no change requested.

7. For the deferred REQ-BACK-001 spec amendment: note the spec also self-contradicts on the cap — jitter is applied after the 30m clamp (in both the spec formula and the code), so worst case is 37.5m, violating REQ-BACK-002's "MUST NOT exceed 30 minutes". Pre-existing; fold into the same amendment.

Verified good

• Import-cycle claim holds: internal/resilience imports nothing; type HTTPStatusError = resilience.HTTPStatusError is a true alias, so *services.HTTPStatusError keeps type identity — all existing call sites/tests compile unchanged.
• errors.As survives %w chains (covered by TestClassifyError_NavidromeRevokedCredentials_Typed).
• All 24 wrap sites are 1:1 in-place; control flow unchanged (checked the lidarr 500-retry continue/return lastErr loop, last.fm <500 early return, Spotify enricher 429 paths; body draining/closing untouched).
• 429/500/502/503 typed → retriable; classification matrix matches REQ-ERR-002/003 except the 404/408 item above.
• Backoff 30/60/120s matches Scenarios 1-2 and ADR-0020; the only other backoff consumer (lidarr_submitter.ComputeBackoff) is independent and untouched; no stale tests assert the old 60/120/240 bands.
• No file overlap with in-flight Story #324: Consolidate configuration into Viper Config struct; make Lidarr optional #352 (touches sync.go, which this PR doesn't) or Replace IsEncrypted heuristic with explicit enc:v1: ciphertext marker (fixes auth-row bricking) #353.
• API shape for Consolidate rate limiting and retries into shared resilience helpers #331/Harden API clients: Lucene escaping, URL escaping, decode fallback, download timeouts #333: fine. Suggestion for Consolidate rate limiting and retries into shared resilience helpers #331 — consider migrating ClassifyError/BackoffManager into internal/resilience too, so enrichers can classify without importing services; the alias pattern makes that migration cheap.

🤖 Posted on behalf of @joestump by Claude.

[joestump]

Review feedback addressed in 868e067 (independent review: #354 (comment)).

BLOCKING

1a. 408 → retriable — classifyHTTPStatus now lists http.StatusRequestTimeout explicitly in the retriable set (it is a timeout; REQ-ERR-002). Pinned by TestClassifyError_RequestTimeout408_Typed and the retriable-statuses table test.

1b. 404 → retriable (deliberate decision) — went with the reviewer's suggested safer call. Rationale (documented in code and PR body): Traefik-style reverse proxies return transient 404s while a backend's route is dropped during container redeploys; fatal would permanently stop sync with a misleading "reconnect credentials" email. A genuinely wrong base URL degrades to retrying at the 30-minute cap, and malformed-config detection belongs to config validation (REQ-ERR-003), not per-request classification. To keep typed and fallback paths consistent, 404 was also removed from the fatal string regex (the one pre-existing plain_status_404 expectation was updated accordingly). Pinned by TestClassifyError_Transient404_Typed plus string-format cases in TestClassifyError_RealWorldClientFormats.

RECOMMENDED

2. internal/enrichers/images.go wrapped — the last non-2xx site now constructs resilience.NewHTTPStatusError, defusing the latent "status: 404 Not Found" fallback-regex trap ahead of Consolidate rate limiting and retries into shared resilience helpers #331/Harden API clients: Lucene escaping, URL escaping, decode fallback, download timeouts #333.

3. Fallback ordering fixed — fatal patterns are now checked before retriable substrings, so status 401: ...timeout... classifies fatal. Pinned by TestClassifyError_FatalPatternWinsOverRetriableSubstring.

FYI items: the REQ-BACK-001 jitter-after-clamp self-contradiction is now listed in the PR body's Deferred Design Doc Updates section, alongside a note suggesting REQ-ERR-002 enumerate the 404/408 decisions.

Verification: make test passes, go test ./internal/services/... -race -count=1 passes, gofmt -l . clean.

🤖 Posted on behalf of @joestump by Claude.

[joestump]

Delta re-review — commit 868e067

Verdict: LGTM — all four findings addressed correctly; independently re-verified on a detached checkout of 868e067 (make test zero failures, go test ./internal/services/... ./internal/notifications/... ./internal/enrichers/... -race -count=1 green, gofmt -l / go vet clean).

• 408 → retriable: typed case added at resilience.go:122 with TestClassifyError_RequestTimeout408_Typed; the string path also lands retriable (regex never matched 408). ✓
• 404 → retriable on both paths: typed case at resilience.go:128 with rationale comment, 404 dropped from fatalStatusPattern, plain_status_404 expectation flipped, and both TestClassifyError_Transient404_Typed and string-path cases pin it. I checked this doesn't reintroduce the Adopt HTTPStatusError across all API clients; fix fatal-error classification #325 bug for any real fatal case: no client in this codebase signals credential failure via 404 (Navidrome/Subsonic auth = 401 or in-body code with HTTP 200; Spotify/Lidarr = 401; Last.fm = 403/in-body codes), and all observed 401/403 formats remain fatal — TestClassifyError_RealWorldClientFormats, both typed tests, and both end-to-end syncer tests still pass. Worst case for a wrong base URL is retry at the 30-minute cap with logging, which the new comment sensibly assigns to config-validation. ✓
• images.go:59 wrapped: now resilience.NewHTTPStatusError; combined with 404→retriable, a missing image URL is typed-retriable, and enrichment callers remain log-only — no notification blast radius. ✓
• Fatal-before-retriable reorder: sanity-checked the trade. Real network timeouts arrive typed (net.Error/*net.OpError) and are classified retriable before the string fallback runs (resilience.go:75-88), so the reorder only affects bare-string errors. The only both-match shapes are "status 401/403" + a retriable word in an appended response body — overwhelmingly real auth failures; Go's genuine timeout messages ("i/o timeout", "context deadline exceeded") contain no fatal substrings. No realistic new false-fatal; TestClassifyError_FatalPatternWinsOverRetriableSubstring pins it. ✓

No new issues introduced by the delta. The remaining items from my first review were explicitly non-blocking (lidarr_submitter wrap → #331; REQ-BACK-001/002 spec amendment → deferred doc change). Good to merge.

🤖 Posted on behalf of @joestump by Claude.