From 8ec85d39c55769c564298782a51c6ec25dd6ba9f Mon Sep 17 00:00:00 2001 From: salmon Date: Wed, 12 Aug 2026 02:56:18 +0200 Subject: fix a bug where adding track using `now` when shuffle is on does not work as expected --- src/daemon.rs | 14 +++++----- src/queue.rs | 87 +++++++++++++++++++++++++++++++++++++++++++--------------- tests/queue.rs | 77 +++++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 149 insertions(+), 29 deletions(-) diff --git a/src/daemon.rs b/src/daemon.rs index 21f1a3b..bdee362 100644 --- a/src/daemon.rs +++ b/src/daemon.rs @@ -1075,7 +1075,7 @@ fn process_command( let new_pos = q.position() + 1; q.set_position(new_pos); } - if let Some(track) = q.track_at(q.position()) { + if let Some(track) = q.current() { audio.play(track.path.clone()); spawn_send_now_playing(scrobbler, track); if let Ok(mut s) = scrobble_state.lock() { @@ -1181,12 +1181,12 @@ fn process_command( match position { "now" => { spawn_try_scrobble_current(scrobbler, scrobble_state); - q.push_track(track.clone()); + q.insert_after_current(PathBuf::from(path_str.clone())); if !was_empty { let new_pos = q.position() + 1; q.set_position(new_pos); } - if let Some(track) = q.track_at(q.position()) { + if let Some(track) = q.current() { audio.play(track.path.clone()); spawn_send_now_playing(scrobbler, track); if let Ok(mut s) = scrobble_state.lock() { @@ -1199,7 +1199,7 @@ fn process_command( format!("Playing now: {}\n", path_str) } "next" => { - q.push_track(track); + q.insert_after_current(PathBuf::from(path_str.clone())); format!("Queued next: {}\n", path_str) } _ => { @@ -1237,7 +1237,7 @@ fn process_command( let new_pos = q.position() + 1; q.set_position(new_pos); } - if let Some(track) = q.track_at(q.position()) { + if let Some(track) = q.current() { audio.play(track.path.clone()); spawn_send_now_playing(scrobbler, track); if let Ok(mut s) = scrobble_state.lock() { @@ -1291,7 +1291,7 @@ fn process_command( let new_pos = q.position() + 1; q.set_position(new_pos); } - if let Some(track) = q.track_at(q.position()) { + if let Some(track) = q.current() { audio.play(track.path.clone()); spawn_send_now_playing(scrobbler, track); if let Ok(mut s) = scrobble_state.lock() { @@ -1389,7 +1389,7 @@ fn process_command( let new_pos = q.position() + 1; q.set_position(new_pos); } - if let Some(track) = q.track_at(q.position()) { + if let Some(track) = q.current() { audio.play(track.path.clone()); spawn_send_now_playing(scrobbler, track); if let Ok(mut s) = scrobble_state.lock() { diff --git a/src/queue.rs b/src/queue.rs index c3f8dbe..da79441 100644 --- a/src/queue.rs +++ b/src/queue.rs @@ -5,7 +5,9 @@ * https://codeberg.org/salmonik/RSPlayer */ +use rand::rngs::StdRng; use rand::seq::SliceRandom; +use rand::SeedableRng; use std::path::{Path, PathBuf}; #[derive(Debug, Clone, PartialEq)] @@ -202,6 +204,7 @@ pub struct Queue { position: usize, shuffle: ShuffleMode, shuffled_indices: Vec, + rng: StdRng, } impl Default for Queue { @@ -217,6 +220,20 @@ impl Queue { position: 0, shuffle: ShuffleMode::None, shuffled_indices: Vec::new(), + rng: rand::make_rng(), + } + } + + /// Construct a queue with a fixed RNG seed, making shuffle order fully + /// reproducible. Used by tests and handy for debugging a specific order. + #[allow(dead_code)] // used only by integration tests (bin crate does not link the lib) + pub fn with_seed(seed: u64) -> Self { + Queue { + tracks: Vec::new(), + position: 0, + shuffle: ShuffleMode::None, + shuffled_indices: Vec::new(), + rng: StdRng::seed_from_u64(seed), } } @@ -225,7 +242,6 @@ impl Queue { self.update_shuffle_indices(); } - #[allow(dead_code)] pub fn push_track(&mut self, track: Track) { self.tracks.push(track); self.update_shuffle_indices(); @@ -234,11 +250,27 @@ impl Queue { pub fn insert_after_current(&mut self, path: PathBuf) { if self.tracks.is_empty() { self.tracks.push(Track::from_path(path)); - } else { - let insert_idx = self.position + 1; - self.tracks.insert(insert_idx, Track::from_path(path)); + self.update_shuffle_indices(); + return; } - self.update_shuffle_indices(); + let insert_idx = self.position + 1; + self.tracks.insert(insert_idx, Track::from_path(path)); + if self.shuffle == ShuffleMode::None { + self.update_shuffle_indices(); + return; + } + // Under shuffle, splice the new entry into the play order right after + // the current slot instead of re-shuffling the tail: the inserted + // track deterministically occupies slot `position + 1`, so "play + // now/next" is meaningful and `current()` agrees with what the daemon + // starts playing. The remaining play order is preserved (indices that + // shifted right in `tracks` are bumped), not re-randomized. + for v in self.shuffled_indices.iter_mut() { + if *v >= insert_idx { + *v += 1; + } + } + self.shuffled_indices.insert(self.position + 1, insert_idx); } pub fn track_at(&self, idx: usize) -> Option<&Track> { @@ -309,10 +341,9 @@ impl Queue { self.shuffled_indices = (0..len).collect(); match self.shuffle { ShuffleMode::All => { - let mut rng = rand::rng(); let pos = self.position.min(len.saturating_sub(1)); if pos + 1 < len { - self.shuffled_indices[pos + 1..].shuffle(&mut rng); + self.shuffled_indices[pos + 1..].shuffle(&mut self.rng); } } ShuffleMode::Album => { @@ -330,10 +361,14 @@ impl Queue { self.shuffled_indices = (0..len).collect(); if len > 1 { - let mut rng = rand::rng(); let pos = self.position.min(len.saturating_sub(1)); if pos + 1 < len { - self.shuffle_album_range(pos + 1..len, &mut rng); + Self::shuffle_album_range( + &self.tracks, + &mut self.shuffled_indices, + pos + 1..len, + &mut self.rng, + ); } } } @@ -352,7 +387,6 @@ impl Queue { } let range_start = pos + 1; - let mut rng = rand::rng(); // Build album groups within the range, shuffling tracks within each group let mut groups: Vec> = Vec::new(); @@ -368,7 +402,7 @@ impl Queue { } else if album != current_album { // Shuffle tracks within this album group before adding it if current_group.len() > 1 { - current_group.shuffle(&mut rng); + current_group.shuffle(&mut self.rng); } groups.push(std::mem::take(&mut current_group)); current_group.push(i); @@ -379,13 +413,13 @@ impl Queue { } if !current_group.is_empty() { if current_group.len() > 1 { - current_group.shuffle(&mut rng); + current_group.shuffle(&mut self.rng); } groups.push(current_group); } // Shuffle the album group order - groups.shuffle(&mut rng); + groups.shuffle(&mut self.rng); let mut new_indices: Vec = self.shuffled_indices[..=pos].to_vec(); for group in &groups { @@ -394,7 +428,12 @@ impl Queue { self.shuffled_indices = new_indices; } - fn shuffle_album_range(&mut self, range: std::ops::Range, rng: &mut impl rand::Rng) { + fn shuffle_album_range( + tracks: &[Track], + indices: &mut [usize], + range: std::ops::Range, + rng: &mut impl rand::Rng, + ) { if range.start >= range.end { return; } @@ -404,7 +443,7 @@ impl Queue { let mut range_start: Option = None; for i in range.clone() { - let album = self.tracks.get(i).and_then(|t| t.album.as_deref()); + let album = tracks.get(i).and_then(|t| t.album.as_deref()); match (current_album, range_start) { (None, None) => { current_album = album; @@ -424,7 +463,7 @@ impl Queue { for (start, end) in album_ranges { if start + 1 < end { - self.shuffled_indices[start..end].shuffle(rng); + indices[start..end].shuffle(rng); } } } @@ -498,13 +537,17 @@ impl Queue { if self.position != 0 || self.tracks.len() <= 1 { return; } - let mut rng = rand::rng(); match self.shuffle { ShuffleMode::All => { - self.shuffled_indices.shuffle(&mut rng); + self.shuffled_indices.shuffle(&mut self.rng); } ShuffleMode::Album => { - self.shuffle_album_range(0..self.tracks.len(), &mut rng); + Self::shuffle_album_range( + &self.tracks, + &mut self.shuffled_indices, + 0..self.tracks.len(), + &mut self.rng, + ); } ShuffleMode::Albums => { let mut groups: Vec> = Vec::new(); @@ -519,7 +562,7 @@ impl Queue { current_album = album; } else if album != current_album { if current_group.len() > 1 { - current_group.shuffle(&mut rng); + current_group.shuffle(&mut self.rng); } groups.push(std::mem::take(&mut current_group)); current_group.push(i); @@ -530,12 +573,12 @@ impl Queue { } if !current_group.is_empty() { if current_group.len() > 1 { - current_group.shuffle(&mut rng); + current_group.shuffle(&mut self.rng); } groups.push(current_group); } - groups.shuffle(&mut rng); + groups.shuffle(&mut self.rng); self.shuffled_indices = groups.concat(); } diff --git a/tests/queue.rs b/tests/queue.rs index d338f2a..67cbabf 100644 --- a/tests/queue.rs +++ b/tests/queue.rs @@ -567,6 +567,83 @@ fn test_shuffle_off_keeps_unplayed_duplicate_entries() { assert_eq!(q.position(), 1); } +#[test] +fn test_shuffle_play_now_current_is_inserted_track() { + // BUG: under shuffle, `insert_after_current` re-shuffles the tail + // (`update_shuffle_indices`), so the inserted track's play-order slot is a + // random draw instead of `position + 1`. The daemon's "now" flow (insert → + // `set_position(pos+1)` → play `track_at(position)`, src/daemon.rs) then + // starts the right file while `current()`/`next()`/the queue display + // follow a *different* track, and auto-advance can replay or skip the + // inserted track entirely. + // + // Contract: after the "now" sequence, `current()` must be the inserted + // track. The seed sweep keeps the test deterministic — for a fixed seed the + // outcome never varies between runs, and pre-fix a given seed satisfies the + // invariant with probability 1/(tail size) = 1/3, so the chance all 40 + // seeds accidentally pass is ~3e-19. The fix makes the invariant hold + // structurally (insert into the play order at `position+1`), independent + // of the seed. + let inserted = PathBuf::from("/music/new.mp3"); + for seed in 1..=40u64 { + let mut q = Queue::with_seed(seed); + add_track(&mut q, "/music/a.mp3"); + add_track(&mut q, "/music/b.mp3"); + add_track(&mut q, "/music/c.mp3"); + q.set_shuffle(ShuffleMode::All); + + // `rsp play --now new.mp3` with a non-empty queue: insert after the + // current track and move the cursor onto it. + q.insert_after_current(inserted.clone()); + let new_pos = q.position() + 1; + q.set_position(new_pos); + + assert_eq!(q.position(), 1); + let cur = q.current().expect("queue is not empty"); + assert_eq!( + cur.path, inserted, + "seed {}: current() after play-now is {:?}, expected {:?}; shuffled_indices={:?}", + seed, + cur.path, + inserted, + q.shuffled_indices() + ); + } +} + +// ---------- Seedable RNG ---------- +// The queue uses a seeded PRNG (rand::rngs::StdRng) instead of the OS-seeded +// thread RNG, so shuffle order is reproducible via Queue::with_seed. Shuffle +// is not a security domain — a PRNG suffices, and deterministic tests need a +// fixed seed. + +#[test] +fn test_same_seed_produces_same_shuffle_order() { + let mut q1 = Queue::with_seed(0x5EED); + let mut q2 = Queue::with_seed(0x5EED); + for i in 0..10 { + add_track(&mut q1, &format!("/music/t{}.mp3", i)); + add_track(&mut q2, &format!("/music/t{}.mp3", i)); + } + q1.set_shuffle(ShuffleMode::All); + q2.set_shuffle(ShuffleMode::All); + assert_eq!(q1.shuffled_indices(), q2.shuffled_indices()); +} + +#[test] +fn test_different_seed_produces_different_shuffle_order() { + let mut q1 = Queue::with_seed(1); + let mut q2 = Queue::with_seed(2); + for i in 0..20 { + add_track(&mut q1, &format!("/music/t{}.mp3", i)); + add_track(&mut q2, &format!("/music/t{}.mp3", i)); + } + q1.set_shuffle(ShuffleMode::All); + q2.set_shuffle(ShuffleMode::All); + // 20! permutations — identical order for two seeds is practically impossible. + assert_ne!(q1.shuffled_indices(), q2.shuffled_indices()); +} + // ---------- Track struct ---------- #[test] -- cgit v1.2.3