fix(petitlyrics): give the lane an error policy and surface a revoked application id - #749
Merged
Merged
Conversation
The orchestrator referenced ZERO petitlyrics sentinels. Every failure on that lane - a rejected clientAppId (401), a refused request shape (403), and provider throttling (429) alike - fell through to the "transport / unexpected error" branch that deliberately leaves the breaker untouched. The lane could not trip on any condition, and a clean miss never reset the ramp either, since petitlyrics.ErrNotFound is not in musixmatch.IsBenignMiss's vocabulary. So the second provider lane was silently unmanaged: no breaker trip, no pacer ratchet, and none of the diagnostic warnings the musixmatch lane gets for the same conditions. The sentinels are classified in their own block rather than folded into the musixmatch one, because the two providers do not share a throttle model. Musixmatch distinguishes a never-succeeded 401 (a bad token) from a post-success 401 (an egress throttle) and ratchets the adaptive pacer only in the latter case. petitlyrics has no token: its 401 means the hardcoded clientAppId was rejected, which is never a throttle and must never ratchet the pacer. Only an explicit 429 does. 403 trips without ratcheting, and stays distinct from 429: per internal/petitlyrics/errors.go a 403 is a refused request SHAPE, which is the #495 User-Agent denylist, where treating it as throttling sent the investigation after a phantom rate limit. ErrNotFound and ErrUnsupportedTier are both healthy round trips and reset the ramp without setting EverSucceeded. An undecodable tier means the response arrived and parsed fine and only its payload tier is unsupported, which says nothing about lane health. Five tests, one per sentinel, each failing at its assertion before the fix. Part of #607
…rror A revoked clientAppId does NOT produce a 401. The API keeps answering: HTTP 200, well-formed XML, zero songs. That maps to ErrNotFound, which is byte-identical to a genuine miss, so a total lane outage would present as "the provider suddenly has none of my tracks" indefinitely, with the lane looking healthy and returning honest misses the whole time. ErrProviderUnavailable escalates after 20 consecutive zero-result lookups. The count is CONSECUTIVE, never cumulative: any response carrying at least one song proves the id is still accepted and clears it, whatever the client then makes of the payload. A cumulative counter would eventually escalate on any long-lived client no matter how healthy the provider. 20 is sized against this lane's measured coverage. As a fallback it only sees tracks the primary already missed, hitting on roughly 1 in 4, so 8 consecutive misses happens about 10% of the time while 20 in a row is under 0.4%. At the 30s pacing floor that is about 10 minutes to detection. The sentinel WRAPS ErrNotFound so existing callers that bucket a miss as benign keep working unchanged, mirroring how musixmatch.ErrTokenRenewalRequired also satisfies errors.Is(_, ErrUnauthorized). That wrapping makes case ORDER load-bearing in both classifiers: tested after the benign-miss case, an outage would reset the ramp and record a stable miss against every track it touched. Both call sites carry a comment saying so, and a test pins it. Per the maintainer's ruling the sentinel TRIPS the breaker: continuing to fire paced requests at a provider answering nothing is waste against a shared egress, and the backoff re-probes so a transient cause still recovers. It does NOT ratchet the pacer, since a dead credential is not a throttle and the slowdown would persist after restoration. A control test proves the pacer does ratchet on an explicit 429, so that assertion cannot pass vacuously. Also fixes the same gap in ClassifyOutcome, which the worker branches on to decide how an item is released (worker.go:1187). It enumerated only musixmatch sentinels, so every petitlyrics error classified as OutcomeTransport and the two provider lanes disagreed about what a miss even is. ErrForbidden stays deliberately out of the auth class: a 403 is a refused request shape that no waiting or rotation fixes, and bucketing it there would repeat the #495 misdiagnosis. Closes #607
From the pre-push hostile review. ClassifyOutcome's doc claimed "classification is per-provider today (only Musixmatch lanes exist)", which the same commit that added the petitlyrics sentinels falsified. Left as-is, a future reader would conclude petitlyrics errors are unclassified, which is the exact wrong belief. It now also records WHY both providers must be enumerated: OutcomeTransport outranks a benign miss in precedence, so an unenumerated provider's routine miss wins the cross-lane ranking on an ordinary double-miss and the worker records a queue failure against the row. That is not hypothetical - it is what petitlyrics did before this branch. The ErrProviderUnavailable threshold comment stated a correct per-window probability but framed it as the run-level false-positive rate. Some run of 20 is expected about every 1,250 lookups, roughly half a day of sustained fallback traffic, so a large scan should expect to trip it occasionally. The comment now says so, and records that the counter keeps climbing past the threshold, so a long dry spell ramps the breaker toward its 30-minute cap. Declined from the same review: wrapping the bare ErrProviderUnavailable return with per-request context. The condition is global to the client rather than per-track, so the triggering track is noise, and resolve.go already logs the cause with lane context. Part of #607
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughPetitLyrics now detects sustained zero-result responses, exposes a provider-unavailable sentinel, and maps PetitLyrics errors through orchestrator breaker, pacing, and outcome classification rules. ChangesPetitLyrics outage handling
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant PetitLyricsAPI
participant Resolve
participant LaneBreaker
Client->>PetitLyricsAPI: Request lyrics
PetitLyricsAPI-->>Client: Empty successful response
Client->>Client: Count consecutive zero-result responses
Client-->>Resolve: Return ErrNotFound or ErrProviderUnavailable
Resolve->>LaneBreaker: Apply error classification
LaneBreaker-->>Resolve: Trip breaker or update pacing
Possibly related issues
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
📋 Issue PlannerBuilt with CodeRabbit's Coding Plans for faster development and fewer bugs. View plan used: ✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
github-actions
Bot
added
enhancement
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Pull request overview
This PR strengthens the Petit Lyrics provider lane’s health/error handling so that sustained “HTTP 200 but zero songs” responses (revoked clientAppId behavior) become diagnosable, can trip the circuit breaker, and are classified correctly in orchestrator outcome ranking (preventing ordinary misses from being treated as transport failures).
Changes:
- Add a new Petit Lyrics sentinel (
ErrProviderUnavailable) triggered after a threshold of consecutive zero-result responses, with hit-driven reset and one-time transition logging. - Implement Petit Lyrics–specific error policy in
providerClassifierto trip/reset the lane breaker appropriately and ratchet pacing only on explicit throttling. - Enumerate Petit Lyrics sentinels in
orchestrator.ClassifyOutcomeand add regression tests to prevent recurrence of the “unrecognized provider miss ranks as transport” failure mode.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| internal/petitlyrics/unavailable_test.go | Adds tests covering below-threshold behavior, threshold crossing, and reset-on-success for zero-result escalation. |
| internal/petitlyrics/errors.go | Introduces ErrProviderUnavailable and documents/justifies the consecutive-zero threshold. |
| internal/petitlyrics/client.go | Tracks consecutive zero-result responses in the client and surfaces ErrProviderUnavailable after threshold; resets on any non-zero response. |
| internal/orchestrator/resolve.go | Adds Petit Lyrics-specific breaker/pacer policy in providerClassifier (401/403/429/unavailable/benign miss handling). |
| internal/orchestrator/petitlyrics_lane_test.go | Adds regression tests pinning Petit Lyrics lane breaker/pacer behavior and ClassifyOutcome mapping. |
| internal/orchestrator/errors.go | Enumerates Petit Lyrics sentinels in ClassifyOutcome to avoid benign misses being mis-ranked as transport. |
Suppressed comments (1)
internal/petitlyrics/client.go:129
- recordNonZeroResult logs while holding the client mutex. Even though this is only on recovery, it still introduces avoidable lock contention. Capture whether a log is needed (and the count) under the lock, then unlock before emitting the log.
if c.zeroReported {
slog.Info("petitlyrics: provider returned results again; the sustained zero-result run has ended",
"after", c.consecutiveZero)
c.zeroReported = false
}
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/petitlyrics/unavailable_test.go`:
- Around line 79-102: Synchronize the shared hit flag in the newTestClient
handler test: replace the unsynchronized boolean access with a mutex or atomic
value, and use the same protection for reads in the request handler and writes
before and after the reset lookup. Preserve the existing response-switching
behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4689c126-0ce2-4615-9c98-532f03a29c7f
📒 Files selected for processing (6)
internal/orchestrator/errors.gointernal/orchestrator/petitlyrics_lane_test.gointernal/orchestrator/resolve.gointernal/petitlyrics/client.gointernal/petitlyrics/errors.gointernal/petitlyrics/unavailable_test.go
Two review findings from PR #749. Copilot: recordZeroResult and recordNonZeroResult both called slog while holding c.mu. That mutex also paces every outbound request, and an slog handler can block on I/O and take locks of its own, so logging under it serializes concurrent lookups -- worst at exactly the moment an outage transition fires. This also broke an established local pattern: pace() already releases the lock before its slog.Debug. Copilot surfaced one site inline and SUPPRESSED the second in a collapsed section of its review body. Both are fixed here; a fix to only the visible one would have resolved the thread while leaving half the defect in place. Moving the emission out means the "already reported?" latch has to be claimed INSIDE the lock, or every goroutine crossing the threshold together emits its own warning and the one-line outage signal becomes a flood. TestZeroResultLatchIsExactlyOnceUnderConcurrency pins that: 40 goroutines, no lost increments, exactly one winner. CodeRabbit: the httptest handler goroutine read the `hit` flag while the test wrote it. -race did not flag it, because the writes land between sequential FindLyrics calls and the handler has finished in practice -- but nothing guarantees that ordering, so the flag is now an atomic.Bool and the safety is explicit rather than timing-dependent. Part of #607
sydlexius
commented
Aug 11, 2026
Owner
Author
|
@coderabbitai resolve |
✅ Action performedComments resolved and changes approved. |
sydlexius
added a commit
that referenced
this pull request
Aug 12, 2026
…ust count CodeRabbit on PR #750, and the second independent reviewer to land on it - the pre-push hostile review flagged the same gap on #749 and deferred it as a minor. Two reviewers finding it separately is a decent signal it is worth taking. The guard existed to stop the test passing for the wrong reason: a round trip that dropped everything would also show zero WordTimings. But it compared only the NUMBER of cues, so a round trip that preserved the count while corrupting text or timestamps satisfied it, and the test would report the WordTimings constraint intact while the persisted subtitle data had silently changed. Mutation-verified: corrupting cue text in decodeSong while preserving the count reddens at the new assertion, naming both changed lines. The old count-only guard passed that mutation.
sydlexius
added a commit
that referenced
this pull request
Aug 12, 2026
…gression tests (#750) * test(worker): pin that the song cache round trip drops word timings models.Song.WordTimings is json:"-" and the worker caches songs as JSON, so a cache hit returns a song classified as merely line-synced. That is a design constraint on the Enhanced-LRC writer (#480), which would otherwise emit word-synced output only on a fresh fetch. Verified to have teeth: with the json:"-" tag removed the test fails at the assertion (got 4, want 0), not at a build error. Part of #480 * docs(petitlyrics): retract the availableLyricsType capability-set claim Two comments asserted availableLyricsType is not a capability set, concluded from a two-track probe. Measured over 107 hits it predicts the returned tier with no exceptions, and the design doc marks the original claim RETRACTED. Payload classification remains correct and stays; only the stated reason changes, so the next reader does not inherit a refuted premise. * test(petitlyrics): add a capture transport for probe runs Tees each response body to an index-named file and restores it so the client can still read it. Test-only, so nothing ships in the binary. Filenames carry a sample index, never a track title. Part of #602 * test(petitlyrics): add the aggregate-only survey report generator The report is the only probe output permitted on a shared surface, so it is safe by construction: sampleObservation carries no title, artist, album, or lyric field, and the renderer counts the free-form copyright field rather than printing it. Verified to have teeth: dumping copyright values instead of counting them fails the canary assertion. Part of #615 * test(petitlyrics): add the env-gated live survey probe One paced sweep answering three questions off the same responses: whether isOfficial discriminates usefully (#615, #600), word-sync coverage on this library rather than on curated playlists (#480), and whether a tier-2 corpus can be collected (#602). Reuses Client.request rather than reimplementing the POST, so the measurement describes canticle and not a copy that could drift on a detail like the User-Agent (#495). Aborts on 401/403/429 rather than continuing: a credential or throttle failure mid-sweep makes later lookups resemble misses, and the coverage number would read as poor coverage rather than a dead credential. apiSong now decodes isOfficial and copyright, for measurement only; nothing in the fetch path consumes them. Part of #615 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018M4dKboj8B4eWY27LiTh9L * test(petitlyrics): make the probe's under-repo guard a real containment check The guard was a substring match on "canticle/internal" and "canticle/docs", which accepted the repo ROOT itself and any otherwise-named directory under it (plprobe/, scratch/). That is precisely where raw captures of a private library must not land: one `git add -A` from a public surface. It also false-positived on an unrelated path that merely contained those substrings. Replaced with resolve-then-relate: filepath.Abs plus EvalSymlinks, then reject unless filepath.Rel(repoRoot, dir) escapes with "..". The repo root is located by walking up to go.mod, so the check holds in a clone at any path and no directory name is hardcoded. A Rel error means different volumes, which is itself proof the path is outside. Also: the abort caveat is now prepended to the rendered report rather than only logged, so it travels to report.txt, the artifact intended for an issue and the one labeled safe to share. It stays aggregate-only, recording the sample index and the sentinel error class, never the wrapped error. Documents that a one-word line counts as non-distinct in distinctStartRatio (arithmetic unchanged), and breaks a run-on in the readTrackList comment. Six offline tests cover the new helpers, including the repo-root, plprobe/, and scratch/ cases the old guard let through; verified by mutation. Part of #615 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018M4dKboj8B4eWY27LiTh9L * test(lyrics): generate an A2 sample for player-support verification Writes a known-good Enhanced-LRC file with synthesized content so Emby and Symphonium support can be judged directly. That verdict gates #480: a non-supporting player renders the word markers as literal text, which is worse than clean line-sync. The renderer is throwaway scaffolding for this sample only; the real writer is #480's job and belongs on the full timing-guard and provenance path. Part of #480 * fix(petitlyrics): make the probe's availableLyricsType and cue-count stats real Four review findings on the survey probe, all measurement correctness or comment accuracy. No runtime behavior changes. The report printed "availableLyricsType agreement: 0/0" unconditionally: the field was never assigned, and could not be, since apiSong did not decode it. A reader would take 0/0 as "absent on every sample", the opposite of the established 107-hit finding. apiSong now decodes availableLyricsType and surveySample stamps it, so the line reports a real agreement rate. Decoded for MEASUREMENT ONLY, alongside isOfficial and copyright: neither selection nor classification consults it, and the payload bytes remain the authoritative discriminator. CueCount was set by surveySample but had no accumulator and was never printed. It is now aggregated into a min/median/max distribution, matching the distinct-start-ratio block's form. A count carries no library metadata, and it sizes the future writer work. The warning that isOfficial values are printed VERBATIM (unlike copyright, which is only counted) now sits on the field itself rather than 135 lines away in test prose. Finally, a third occurrence of the retracted "availableLyricsType is not a capability set" claim, missed by 576c7ba, is corrected on TestClassifyPayload. The test's conclusion stands, only the stated reason was refuted, and the corrected comment does not argue for trusting the field. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018M4dKboj8B4eWY27LiTh9L * test(lrcnormalize): prove A2 word markers survive the round trip Issue #480 names this as an acceptance criterion, and it holds independently of any media player: canticle reads its own sidecars back off disk through ParseBody on the revalidate path (#442), so a writer that emits word markers which this normalizer then mangles on read is a defect whatever a player does with the file. Two tests, matching the two directions the criterion names. ParseBody must yield one cue per LINE, not one per word marker, with the markup carried as literal cue text. Expand must not split an A2 cue: a split shifts every later word-timing index, which is what the petitlyrics client's length guard at client.go:326 exists to detect. The mechanism is tsRe being anchored and requiring square brackets, so an angle-bracket marker cannot match on either count. Verified to have teeth: widening tsRe to accept angle brackets reddens both at their assertions, shattering each 4-word line into 4 cues whose text starts mid-line. Not a build error. The A2 body is byte-identical to the sample generator's output so the two cannot drift. All content is synthesized placeholder words. Part of #480 * fix(petitlyrics): abort the probe on a sustained zero-result run The merge with main was textually clean but semantically stale. #607's ErrProviderUnavailable sentinel arrived via #749; the probe predated it and never learned about it, so surveySample had no case for it. That sentinel WRAPS ErrNotFound by construction, so errors.Is(err, ErrNotFound) matched it and the probe classified a credential outage as an ordinary miss and kept sweeping. The resulting coverage number would read as "the provider has little for this library" when the truth is "the credential died at sample N" - the exact contamination the abort semantics exist to prevent, in the one place whose job is to notice it. Third instance of this wrapper-ordering trap on this work, after providerClassifier and ClassifyOutcome. A test now pins it: mutating the case back below the miss case reddens at the assertion, not at a build error. Found by the pre-push hostile review. * docs(petitlyrics): say why the agreement denominator is smaller than the tier total From the pre-push hostile review. A sample that classifies a tier but then fails to decode counts toward the tier distribution (deliberately - the question is what the provider OFFERS, not what we managed to decode) yet never reaches the availableLyricsType accumulators. So the two denominators in the report legitimately differ, and a reader cross-referencing them had no way to know why. The render line now says so inline, since report.txt is the artifact that travels to an issue and its reader has no access to the code. Declined from the same review: the transport-error banner gap when a run's last two samples fail transport without reaching the three-in-a-row cap. The reviewer called it a judgment call rather than a defect, and the errCounts table already shows the failures, so a truncated tail is visible without a banner. The spec's claim that copyright raw values are recorded was also corrected (the code only counts them, which is the safer behavior); that file is gitignored so it does not appear in this commit. * fix(petitlyrics): close a fail-open in the probe's repo-containment guard CodeRabbit review on PR #750. The Major finding is real and I verified the mechanism empirically rather than by reading. resolveSymlinks returned the path unchanged whenever EvalSymlinks failed, and EvalSymlinks fails when ANY component is missing - which is the normal first run, before the operator has created PLPROBE_DIR. With a symlinked component in the path (on macOS /tmp is a symlink to /private/tmp) the resolved repo root and the unresolved probe dir landed in different namespaces, so filepath.Rel returned a ".."-prefixed result and the guard read a directory INSIDE the working tree as outside it. Measured: Rel("/tmp/plprobe-check/real", "/tmp/plprobe-check/link/not-created-yet") returns "../link/not-created-yet". That is a fail-open in the one guard whose whole job is keeping raw captures of a private library out of the repo. It now resolves the nearest existing ancestor and re-attaches the missing tail, so both sides stay in the same namespace whether or not the leaf exists. A test pins it, mutation-verified: restoring the old behavior reddens at the assertion with the accepted in-repo path printed. The same asymmetry made a findRepoRoot assertion vacuous (an unresolved `bare` compared against a resolved result can never match on a symlinked TMPDIR); it now resolves both sides. Also from the review: - The privacy canary fed its canary through Copyright, which render() only COUNTS, while setting IsOfficial - the one field printed verbatim - to a benign "1". So it asserted over the path that cannot leak and left the path that can unexercised. IsOfficial now passes through boundedToken: a short enumerated token prints, anything longer becomes a length-only placeholder. BOUNDED rather than bucketed on purpose - the reviewer suggested bucketing, but printing the real values is exactly how the sweep found the inverted isOfficial correlation that redirected #480 and #615. - The abort test pinned only one direction of the case order. A surveySample that aborted on EVERY miss would have broken at i == 0 and satisfied every assertion. It now asserts each sub-threshold lookup is a plain miss and that the abort lands exactly at the threshold. - A comment claimed the A2 body was byte-identical to the sample generator's output. It is not (that generator also writes [al], [by], a third cue and trailing spaces), and the claim would have sent a reader trying to sync them. Corrected to describe it as a minimized body sharing the cue-line shape. Declined the suggestion to share one fixture: it would couple a permanent parser test to throwaway scaffolding already marked for deletion. * test(worker): assert cue CONTENT survives the cache round trip, not just count CodeRabbit on PR #750, and the second independent reviewer to land on it - the pre-push hostile review flagged the same gap on #749 and deferred it as a minor. Two reviewers finding it separately is a decent signal it is worth taking. The guard existed to stop the test passing for the wrong reason: a round trip that dropped everything would also show zero WordTimings. But it compared only the NUMBER of cues, so a round trip that preserved the count while corrupting text or timestamps satisfied it, and the test would report the WordTimings constraint intact while the persisted subtitle data had silently changed. Mutation-verified: corrupting cue text in decodeSong while preserving the count reddens at the new assertion, naming both changed lines. The old count-only guard passed that mutation. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
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
providerClassifierreferenced only the musixmatch sentinels, so a rejected application id (401), a refused request shape (403), and provider throttling (429) all fell through to the branch that deliberately leaves the breaker untouched. The lane could not trip on any condition, and a clean miss never reset its throttle ramp either.ErrProviderUnavailablefor the failure petitlyrics: distinguish a revoked clientAppId from ordinary misses #607 actually describes: a revokedclientAppIddoes not produce a 401. The API keeps answering HTTP 200 with well-formed XML and zero songs, which maps toErrNotFoundand is indistinguishable from a genuine miss. Escalates after 20 consecutive zero-result lookups; any response carrying a song clears the count.ClassifyOutcomehad zero petitlyrics references, so a routine miss classified asOutcomeTransport(precedence 3), which outranks a benign miss (precedence 1). On an ordinary double-miss the worker therefore recorded a queue failure against the row:attempts++, geometric backoff, marching toward retirement. Ordinary misses were burning retry budget.No user-visible behavior change to lyric output. The change is to lane health, breaker behavior, and how the queue releases an item.
Linked issue
Closes #607
Part of #748 (the class fix -- an enumeration test so a third provider cannot reproduce this silently -- is tracked there, not here).
Design decisions worth reviewing
ErrProviderUnavailablewrapsErrNotFound, so existing callers that bucket a miss as benign keep working unchanged. That makes case order load-bearing in bothproviderClassifierandClassifyOutcome: tested after the benign-miss case, a credential outage would reset the ramp and record a stable miss against every track it touched. Both sites carry a comment saying so, and a test pins it.ErrForbiddenstays out of the auth class. A 403 is a refused request shape that no waiting or rotation fixes; bucketing it with throttling would repeat the petitlyrics lane fails 100% with HTTP 403 at the search stage (7/7, zero successes) #495 misdiagnosis, where a User-Agent denylist rejection read as a phantom rate limit.Pre-flight checklist
make gate), re-run after the review fixes.d630bba..templor CSS source changed.Test plan
make gatepasses locally, re-run atd630bba.circuit.Breaker.Allow()andClassifyOutcomedirectly rather than through/metricsor the dashboard, neither of which this touches.attemptswere inflated by the misclassification, and whether it relates to done rows settle through musixmatch with no outcome_type; 3,163 and still occurring #655 -- same subsystem, deliberately not asserted either way.Summary by CodeRabbit