fix(audio_key): retry key requests on denial and timeout - #1763
TheOneDenis wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Three moderate issues remain in audio-key cleanup and retry filtering.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This PR improves audio-key request reliability by retrying transient denials and timeouts, while cleaning up pending requests.
Changes:
- Adds up to three attempts with one-second backoff.
- Cleans pending entries after timeout or channel closure.
- Documents the fix in
CHANGELOG.md.
File summaries
| File | Summary | Review findings |
|---|---|---|
core/src/audio_key.rs |
Implements retry and cleanup logic. | Three moderate findings remain: synchronous send failures can leak entries (3 votes); dropped callers can leak entries without RAII cleanup (1 vote); retries should be limited to transient denials to avoid delaying normal missing-key cases (1 vote). |
CHANGELOG.md |
Adds the unreleased fix entry. | No findings. |
Review details
Suppressed comments (2)
core/src/audio_key.rs:98
- The cleanup below only runs after the
timeoutfuture completes. If the caller drops this async request while it is waiting onrx(for example, when a pending player/preload load is replaced), the receiver is dropped but this sender remains inpending; with no response, that entry is never removed. Use a drop guard/RAII cleanup for each registered sequence.
let (tx, rx) = oneshot::channel();
let seq = self.lock(move |inner| {
let seq = inner.sequence.get();
inner.pending.insert(seq, tx);
core/src/audio_key.rs:109
- This retries every
AudioKeyError::AesKey, but the existing player intentionally uses a missing key as the normal path for unencrypted files (playback/src/player.rs:1108-1115,audio/src/decrypt.rs:24-25). Those files now incur two 1-second sleeps and three requests before playback can continue. Preserve the server's reason/code and retry only transient denials so permanent no-key responses remain immediate.
Ok(Ok(Err(err))) => {
// The server denied the key for this request.
last_err = Some(err);
}
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| seq | ||
| }); | ||
|
|
||
| self.send_key_request(seq, track, file)?; |
There was a problem hiding this comment.
Good catch — addressed in 7b929fe: the pending entry is now removed before propagating the send error.
There was a problem hiding this comment.
Addressed in a519860: every attempt now registers its entry under a Drop guard, so the pending entry is removed however the request ends — including a caller dropping the future at an await point. The guard also replaces the manual cleanup on the send/timeout/channel paths.
On the retry filter: agreed — the server error code is now preserved in AudioKeyError::AesKey(u16) (marked breaking in the changelog) and only the transient code (0x0002, see #1649 reports of retries succeeding) is retried; permanent denials and no-key responses for unencrypted files return immediately.
9da2e46 to
7b929fe
Compare
|
Still got an error: |
Spotify intermittently denies audio key requests for some accounts even though the same request succeeds when retried shortly after (issue librespot-org#1649). Responses can also time out right after an access-point reconnect while the new connection is still settling. Retry the same key request up to 3 times with backoff, but only for the transient denial code (0x0002) and timeouts; permanent denials and no-key responses for unencrypted files return immediately so the normal playback path stays fast. To make this distinction possible the Spotify server error code is now carried in AudioKeyError::AesKey (breaking). The pending entry is guarded by a Drop guard, so it is removed when the request finishes or is dropped at any await point, not only on the paths that explicitly clean up.
7b929fe to
a519860
Compare
|
Thanks for testing @hexyliae! Your log confirms the retry path works ( …), but what you're hitting looks like the persistent variant of #1649 (error code That distinction is now encoded in a519860: only the transient code is retried (up to 3x with backoff), permanent denials return immediately as before, so your case would fail fast rather than burning 3 requests per track. Unfortunately no client-side change can lift a persistent server-side denial — that part needs Spotify, or works again as mysteriously as it broke (per reports in #1649). |
Summary
Retries audio key requests up to 3 times with backoff before failing the track, and cleans up timed-out pending entries so they no longer leak in the pending map.
Problem
Two failure modes are addressed:
error audio key 0 2in Audio key error #1649).Changes
core/src/audio_key.rs:request()retries the same request up to 3 times with a 1s backoff on both AesKey denial and timeout/channel-closure.Unavailable(denied) vsAborted(timeout) distinction for callers.[Unreleased]→Fixed.Testing
cargo buildandcargo clippyclean,cargo fmt --all -- --checkpasses.error audio key 0 2, while subsequent identical requests succeed).