feat: queue failed scrobbles offline and resubmit with original timestamps#445
Closed
firatege wants to merge 4 commits into
Closed
feat: queue failed scrobbles offline and resubmit with original timestamps#445firatege wants to merge 4 commits into
firatege wants to merge 4 commits into
Conversation
…tamps Scrobbles that failed (no network, server down) were logged and lost, so offline listening never reached Last.fm / Libre.fm / ListenBrainz. Both protocols support backdated submissions, so nothing forces that. New scrobble::queue module: a scrobble that fails with a transient error (no response or 5xx) is persisted to scrobble_queue.json with its original listen timestamp and the services it is still owed to. The queue is drained on startup and whenever a scrobble succeeds again (which signals connectivity is back). Permanent errors (4xx, e.g. revoked credentials) are never queued since retrying cannot succeed. The queue is capped at 500 entries, oldest dropped first, and a corrupt or missing file simply loads as an empty queue. While wiring this into the player controller, the local-playback path also gained the Libre.fm now-playing/scrobble submission that Kopuz-org#378 only added to the server-stream path. Covered by unit tests (cap/oldest-drop, save/load roundtrip, corrupt file, cross-service merge, credential-less drain) and verified end to end by blocking the scrobble hosts, playing a track (entry queued), unblocking, and playing another (queue drained, resubmitted with the original timestamp). Closes Kopuz-org#335
Review feedback: a truncated write could corrupt the queue file, which load() silently treats as empty, dropping the whole backlog — save now writes to a temp file and renames. And drain only persisted at the end, so an interrupted run would re-send already delivered scrobbles — it now checkpoints after every entry.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds an offline scrobble queue ( ChangesOffline Scrobble Buffering and Retry
Sequence Diagram(s)sequenceDiagram
participant PlayerController as use_player_controller
participant Service as Last.fm / Libre.fm / MusicBrainz
participant Queue as scrobble::queue
participant Disk as scrobble_queue.json
Note over PlayerController: On scrobble event
PlayerController->>PlayerController: compute listened_at, scrobble_ok=false
PlayerController->>Service: submit *_at scrobble payload
alt HTTP success
Service-->>PlayerController: Ok
PlayerController->>PlayerController: scrobble_ok = true
PlayerController->>Queue: drain(path, credentials)
Queue->>Disk: load queued items
Queue->>Service: resubmit each pending scrobble
Service-->>Queue: Sent / Transient / Permanent
Queue->>Disk: checkpoint save after each entry
else Transient error (network / 5xx)
Service-->>PlayerController: error
PlayerController->>Queue: enqueue(service, artist, title, album, listened_at)
Queue->>Disk: merge + save queue
end
Note over PlayerController: On startup (non-wasm)
PlayerController->>Queue: drain(path, credentials) via use_future
Queue->>Disk: load queued items
Queue->>Service: resubmit all pending scrobbles
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@crates/hooks/src/use_player_controller.rs`:
- Around line 1702-1707: The code at line 1702 creates a listening event with
MusicBrainz metadata in the map parameter passed to make_listen_at, but the
queuing logic around line 1729 only stores artist, title, album, and timestamp
without preserving the additional_info map containing release/recording/track
MBIDs. When offline retries reconstruct ListenBrainz payloads from the queue,
the MusicBrainz IDs are lost. Modify the queued item structure to also persist
the additional_info map (containing the MBIDs), and ensure that when draining
the queue and reconstructing payloads, this metadata is passed back to
make_listen_at or equivalent payload construction logic.
- Around line 1001-1008: The `listened_at` timestamp capture in the scrobble
handling code is occurring after the `threshold_secs` sleep, which means it
records the scrobble submission time rather than the original listen start time.
Move the `listened_at` calculation block (containing
std::time::SystemTime::now(), duration_since(std::time::UNIX_EPOCH), as_secs(),
and unwrap_or(0)) to immediately after the duration check and before any
threshold sleep operations, then reuse this captured value after the threshold
wait. Apply the same fix to the local-track scrobble task in the similar
timestamp capture block around lines 1599-1606 to ensure consistency across both
submission paths.
In `@crates/scrobble/src/queue.rs`:
- Around line 167-225: The drain function holds QUEUE_LOCK across the entire
operation including the await on submit_one network calls, which blocks the
enqueue function for the full drain duration. Refactor to narrow the lock scope:
acquire the lock only to load the queue with ScrobbleQueue::load, release it
before processing the loop over queue.items where submit_one is awaited, then
re-acquire the lock only when performing checkpoint saves with queue.save. This
allows network submissions to run concurrently with new enqueue operations and
prevents blocking scrobble persistence during drain.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 33b757ae-ae86-4f1c-a1cf-8f90214e57d3
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
crates/hooks/src/use_player_controller.rscrates/scrobble/Cargo.tomlcrates/scrobble/src/lastfm.rscrates/scrobble/src/lib.rscrates/scrobble/src/librefm.rscrates/scrobble/src/musicbrainz.rscrates/scrobble/src/queue.rs
Comment on lines
+1001
to
+1008
| // Offline queue bookkeeping (issue #335): scrobbles | ||
| // that fail with a transient error are queued with | ||
| // this listen timestamp and resubmitted later; one | ||
| // success means we're online, so drain the backlog. | ||
| let listened_at = std::time::SystemTime::now() | ||
| .duration_since(std::time::UNIX_EPOCH) | ||
| .map(|d| d.as_secs() as i64) | ||
| .unwrap_or(0); |
Contributor
There was a problem hiding this comment.
Capture listened_at before the threshold wait.
Line 1005 and Line 1603 run after the now-playing calls and threshold_secs sleep, so direct submissions and queued retries are timestamped at the scrobble attempt time rather than the original listen start. Move this capture to immediately after the duration check and reuse it after the threshold.
Proposed adjustment
// track must be longer than 30 seconds
if duration_secs < 30 {
return;
}
+
+ let listened_at = std::time::SystemTime::now()
+ .duration_since(std::time::UNIX_EPOCH)
+ .map(|d| d.as_secs() as i64)
+ .unwrap_or(0);
{
let source = scrobble_source.peek().clone();
if let Err(e) =
source.scrobble_now_playing(&scrobble_id).await
@@
- let listened_at = std::time::SystemTime::now()
- .duration_since(std::time::UNIX_EPOCH)
- .map(|d| d.as_secs() as i64)
- .unwrap_or(0);
let queue_path = scrobble::queue::default_queue_path();Apply the same move in the local-track scrobble task before its now-playing/threshold work.
Also applies to: 1599-1606
🤖 Prompt for 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.
In `@crates/hooks/src/use_player_controller.rs` around lines 1001 - 1008, The
`listened_at` timestamp capture in the scrobble handling code is occurring after
the `threshold_secs` sleep, which means it records the scrobble submission time
rather than the original listen start time. Move the `listened_at` calculation
block (containing std::time::SystemTime::now(),
duration_since(std::time::UNIX_EPOCH), as_secs(), and unwrap_or(0)) to
immediately after the duration check and before any threshold sleep operations,
then reuse this captured value after the threshold wait. Apply the same fix to
the local-track scrobble task in the similar timestamp capture block around
lines 1599-1606 to ensure consistency across both submission paths.
Comment on lines
+1702
to
+1707
| let listen = scrobble::musicbrainz::make_listen_at( | ||
| &scrobble_track.artist, | ||
| &scrobble_track.title, | ||
| Some(&scrobble_track.album), | ||
| Some(map.clone()), | ||
| listened_at, |
Contributor
There was a problem hiding this comment.
Preserve MusicBrainz IDs in queued ListenBrainz retries.
Line 1706 sends Some(map.clone()) for the online ListenBrainz submission, but Line 1729 queues only artist/title/album/timestamp; the queue drain reconstructs ListenBrainz payloads with no additional_info. Offline retries will lose the release/recording/track MBIDs populated above, so please persist and replay that metadata for queued ListenBrainz items.
Also applies to: 1729-1736
🤖 Prompt for 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.
In `@crates/hooks/src/use_player_controller.rs` around lines 1702 - 1707, The code
at line 1702 creates a listening event with MusicBrainz metadata in the map
parameter passed to make_listen_at, but the queuing logic around line 1729 only
stores artist, title, album, and timestamp without preserving the
additional_info map containing release/recording/track MBIDs. When offline
retries reconstruct ListenBrainz payloads from the queue, the MusicBrainz IDs
are lost. Modify the queued item structure to also persist the additional_info
map (containing the MBIDs), and ensure that when draining the queue and
reconstructing payloads, this metadata is passed back to make_listen_at or
equivalent payload construction logic.
…e-acquire only for checkpoint writes
Contributor
|
still need to resolve rabbit's comments tho |
Member
|
sup @firatege can you fix the conflicts |
Contributor
Author
|
Yes, I can do that, but it will take some time I don't have access to a computer right now. |
Member
|
im adding new stuff to scrobbling stuff so you may close this when you access your pc and add offline scrobbling to new build |
Member
|
ehhh im gonna take over this im bored .d |
17 tasks
Member
|
i took over this, can be closed |
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
3 participants
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
Closes #335. Scrobbles that failed (no network, server hiccup) were logged and lost, so offline listening never reached Last.fm / Libre.fm / ListenBrainz. Both protocols support backdated submissions (Last.fm
track.scrobbletakes atimestamp, ListenBrainz takeslistened_at), so this adds an offline queue that resubmits them with their original listen time.How it works
New
scrobble::queuemodule:scrobble_queue.jsonnext toconfig.json, with its original timestamp and the services it's still owed to (one listen failing on two services stays one entry with two pending services).Also in this PR
The local-playback path was missing the Libre.fm now-playing/scrobble submission entirely (#378 only added it to the server-stream path), so local plays never scrobbled to Libre.fm. It's wired up now, and the queue covers it.
Tests
Unit tests in
scrobble::queue: queue cap and oldest-first dropping, save/load roundtrip, corrupt/missing file loads empty, cross-service merge of the same listen, credential-less drain releases entries.Verified end to end
Queue file emptied afterwards and the resubmitted listen kept its original timestamp.
Summary by CodeRabbit