fix: preserve client request ordering - #1035
Open
ecschoye wants to merge 2 commits into
Open
Conversation
There was a problem hiding this comment.
Pull request overview
This PR ensures state-changing Spotify client requests (player commands, library/playlist mutations, and playback refreshes that can conflict with them) execute deterministically in receive order, while allowing independent read-only requests to remain concurrent. It also prevents stale playback refresh responses from overwriting newer local/player state via a refresh-generation mechanism, with deterministic async tests to validate behavior.
Changes:
- Add a serialized execution lane for “ordered”
ClientRequests while keeping other requests concurrent (client/handlers.rs,client/request.rs). - Introduce a playback refresh generation counter to discard stale refresh responses and invalidate refreshes on player-changing operations (
state/player.rs,client/mod.rs). - Add focused unit tests validating ordered vs concurrent dispatch and refresh supersession (
client/handlers.rs,client/request.rs,state/player.rs).
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| spotify_player/src/state/player.rs | Adds refresh-generation tracking APIs and unit tests to identify/discard stale playback refreshes. |
| spotify_player/src/client/request.rs | Classifies requests as ordered vs concurrent via requires_ordered_execution() and tests the classification. |
| spotify_player/src/client/mod.rs | Invalidates pending refreshes around player mutations and discards stale playback refresh results during retrieval. |
| spotify_player/src/client/handlers.rs | Implements the ordered/concurrent dispatch split and adds deterministic async tests for execution ordering. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+45
to
+62
| while let Ok(request) = request_sub.recv_async().await { | ||
| if requires_ordered_execution(&request) { | ||
| if ordered_pub.send(request).is_err() { | ||
| break; | ||
| } | ||
| } else { | ||
| let handler = handler.clone(); | ||
| concurrent_tasks.spawn(async move { | ||
| handler(request).await; | ||
| }); | ||
| } | ||
|
|
||
| while let Some(result) = concurrent_tasks.try_join_next() { | ||
| if let Err(err) = result { | ||
| tracing::error!("Concurrent client request task failed: {err:#}"); | ||
| } | ||
| } | ||
| } |
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
Why
The client handler previously spawned every received request as an independent task. Although the channel preserved receive order, completion order was uncontrolled. Rapid player commands could read the same buffered playback snapshot, and playlist or library mutations could reach Spotify out of order. Delayed playback refreshes could also overwrite newer local state.
The ordered lane now preserves receive order for conflicting requests without making slow catalog and library fetches block one another. Playback refresh generations ensure that only the newest in-flight response can update player state.
Validation
cargo fmt --all -- --checkcargo test --no-default-features --features rodio-backend,media-control,image,notify,fzfcargo clippy --no-default-features --features rodio-backend,media-control,image,notify,fzf -- -D warningscargo clippy --no-default-features -- -D warningsgit diff --checkCloses #1027