Bug fix for repeated scrobbles when track might be paused; more tests for the MPD route

src/db.rs

@@ -205,11 +205,10 @@ fn init_schema(conn: &Connection) -> Result<()> {
     )?;
     // Add the source column to existing databases. SQLite returns an error if
     // the column already exists, which we silently ignore for idempotency.
-    conn.execute(
+    conn.execute("ALTER TABLE scrobbles ADD COLUMN source TEXT", [])
-        "ALTER TABLE scrobbles ADD COLUMN source TEXT",
+        .ok();
-        [],
+    conn.execute_batch(
-    ).ok();
+        "
-    conn.execute_batch("
         -- Cache table for album metadata (cover art, genres) from MusicBrainz.
         -- Populated by the 'enrich' command or automatically when generating
         -- HTML reports. Rows with cover_url = NULL are re-tried on next run.

src/enrich.rs

@@ -927,7 +927,12 @@ pub fn run_enrich(conn: &Connection, force: bool, quiet: bool, no_itunes: bool)
 }
 
 /// Inner enrichment loop shared by `run_enrich` and `run_enrich_targeted`.
-fn run_enrich_albums(conn: &Connection, albums: Vec<db::UncachedAlbum>, quiet: bool, no_itunes: bool) {
+fn run_enrich_albums(
+    conn: &Connection,
+    albums: Vec<db::UncachedAlbum>,
+    quiet: bool,
+    no_itunes: bool,
+) {
     if albums.is_empty() {
         if !quiet {
             eprintln!("All albums are already cached. Nothing to do.");
@@ -969,8 +974,7 @@ fn run_enrich_albums(conn: &Connection, albums: Vec<db::UncachedAlbum>, quiet: b
                 None
             } else {
                 eprintln!("  Trying iTunes...");
-                fetch_itunes_cover_url(&client, &album.artist, &album.album)
+                fetch_itunes_cover_url(&client, &album.artist, &album.album).and_then(|art_url| {
-                    .and_then(|art_url| {
                     let stem = itunes_cover_stem(&album.artist, &album.album);
                     let dest = covers.join(format!("{}.jpg", stem));
                     try_download(&client, &art_url, &dest)
@@ -1053,8 +1057,7 @@ fn run_enrich_albums(conn: &Connection, albums: Vec<db::UncachedAlbum>, quiet: b
                 );
                 for alt_mbid in &candidates[1..] {
                     thread::sleep(RATE_LIMIT_DELAY);
-                    let alt_url =
+                    let alt_url = format!("https://coverartarchive.org/release/{}/front", alt_mbid);
-                        format!("https://coverartarchive.org/release/{}/front", alt_mbid);
                     let dest = covers.join(format!("{}.jpg", primary_mbid));
                     if let Some(path) = try_download(&client, &alt_url, &dest) {
                         eprintln!("  Cover downloaded (from alternate release {}).", alt_mbid);

src/main.rs

@@ -703,7 +703,17 @@ fn main() {
                 host: mpd_host,
                 port: mpd_port,
             };
-            run_report(&period, json, html, output.as_deref(), limit, atl, no_enrich, &mpd_cfg, &path);
+            run_report(
+                &period,
+                json,
+                html,
+                output.as_deref(),
+                limit,
+                atl,
+                no_enrich,
+                &mpd_cfg,
+                &path,
+            );
         }
         Commands::Enrich {
             online,
@@ -717,8 +727,12 @@ fn main() {
         } => {
             if no_mpd_covers && !online {
                 eprintln!("Nothing to do. Pass --online and/or omit --no-mpd-covers.");
-                eprintln!("  (default)      Extract embedded covers from music files via MPD (offline)");
+                eprintln!(
-                eprintln!("  --online       Fetch metadata and covers from MusicBrainz / iTunes / CAA");
+                    "  (default)      Extract embedded covers from music files via MPD (offline)"
+                );
+                eprintln!(
+                    "  --online       Fetch metadata and covers from MusicBrainz / iTunes / CAA"
+                );
                 std::process::exit(0);
             }
 

src/mpd.rs

@@ -909,10 +909,7 @@ fn try_extract_cover(
     let cover_path = dest.to_string_lossy().to_string();
     let locked = db_conn.lock().unwrap();
     if db::set_local_cover(&locked, &state.artist, &state.album, &cover_path).is_ok() {
-        eprintln!(
+        eprintln!("[mpd] Cover cached for {} — {}", state.artist, state.album);
-            "[mpd] Cover cached for {} — {}",
-            state.artist, state.album
-        );
     }
 }
 
@@ -1103,6 +1100,26 @@ fn run_mpd_cover_enrich_albums(
 #[cfg(test)]
 mod tests {
     use super::*;
+    use std::cell::RefCell;
+    use std::rc::Rc;
+
+    fn state(
+        file: &str,
+        artist: &str,
+        album: &str,
+        title: &str,
+        duration_secs: u64,
+        state: &str,
+    ) -> MpdPlayerState {
+        MpdPlayerState {
+            file: file.to_string(),
+            artist: artist.to_string(),
+            album: album.to_string(),
+            title: title.to_string(),
+            duration_secs: Some(duration_secs),
+            state: state.to_string(),
+        }
+    }
 
     // -----------------------------------------------------------------------
     // album_cover_stem — filename generation
@@ -1198,6 +1215,123 @@ mod tests {
         assert_eq!(parse_mpd_state("unknown"), PlayerStatus::Stopped);
     }
 
+    // -----------------------------------------------------------------------
+    // dispatch_events
+    // -----------------------------------------------------------------------
+
+    #[test]
+    fn test_dispatch_events_pause_resume_same_file_keeps_metadata_marker() {
+        let scrobbled: Rc<RefCell<Vec<db::NewScrobble>>> = Rc::new(RefCell::new(Vec::new()));
+        let sink = scrobbled.clone();
+        let mut tracker = watcher::ScrobbleTracker::new(
+            move |s: db::NewScrobble| {
+                sink.borrow_mut().push(s);
+            },
+            "MPD".to_string(),
+        );
+
+        let mut last_metadata_file: Option<String> = None;
+
+        let mut prev = MpdPlayerState::default();
+        let play_a = state(
+            "a.flac",
+            "Deftones",
+            "White Pony",
+            "Digital Bath",
+            291,
+            "play",
+        );
+        dispatch_events(&prev, &play_a, &mut tracker, &mut last_metadata_file);
+        assert_eq!(last_metadata_file.as_deref(), Some("a.flac"));
+
+        let pause_a = state(
+            "a.flac",
+            "Deftones",
+            "White Pony",
+            "Digital Bath",
+            291,
+            "pause",
+        );
+        dispatch_events(&play_a, &pause_a, &mut tracker, &mut last_metadata_file);
+        assert_eq!(last_metadata_file.as_deref(), Some("a.flac"));
+
+        prev = pause_a;
+        let resume_a = state(
+            "a.flac",
+            "Deftones",
+            "White Pony",
+            "Digital Bath",
+            291,
+            "play",
+        );
+        dispatch_events(&prev, &resume_a, &mut tracker, &mut last_metadata_file);
+        assert_eq!(last_metadata_file.as_deref(), Some("a.flac"));
+
+        // No track boundary occurred; therefore no scrobble should have been emitted.
+        assert!(scrobbled.borrow().is_empty());
+    }
+
+    #[test]
+    fn test_dispatch_events_stop_clears_metadata_marker() {
+        let scrobbled: Rc<RefCell<Vec<db::NewScrobble>>> = Rc::new(RefCell::new(Vec::new()));
+        let sink = scrobbled.clone();
+        let mut tracker = watcher::ScrobbleTracker::new(
+            move |s: db::NewScrobble| {
+                sink.borrow_mut().push(s);
+            },
+            "MPD".to_string(),
+        );
+
+        let mut last_metadata_file: Option<String> = None;
+        let prev = MpdPlayerState::default();
+        let play_a = state("a.flac", "A", "Alb", "Song A", 200, "play");
+        dispatch_events(&prev, &play_a, &mut tracker, &mut last_metadata_file);
+        assert_eq!(last_metadata_file.as_deref(), Some("a.flac"));
+
+        let stop_a = state("a.flac", "A", "Alb", "Song A", 200, "stop");
+        dispatch_events(&play_a, &stop_a, &mut tracker, &mut last_metadata_file);
+        assert_eq!(last_metadata_file, None);
+    }
+
+    #[test]
+    fn test_dispatch_events_replay_same_file_after_stop_reinitialises_track() {
+        let scrobbled: Rc<RefCell<Vec<db::NewScrobble>>> = Rc::new(RefCell::new(Vec::new()));
+        let sink = scrobbled.clone();
+        let mut tracker = watcher::ScrobbleTracker::new(
+            move |s: db::NewScrobble| {
+                sink.borrow_mut().push(s);
+            },
+            "MPD".to_string(),
+        );
+
+        let mut last_metadata_file: Option<String> = None;
+        let prev = MpdPlayerState::default();
+
+        // Use 0-second duration so threshold is 0 and each Stop scrobbles
+        // immediately if the tracker is tracking an active track.
+        let play_a = state("a.flac", "A", "Alb", "Song A", 0, "play");
+        dispatch_events(&prev, &play_a, &mut tracker, &mut last_metadata_file);
+
+        let stop_a = state("a.flac", "A", "Alb", "Song A", 0, "stop");
+        dispatch_events(&play_a, &stop_a, &mut tracker, &mut last_metadata_file);
+        assert_eq!(scrobbled.borrow().len(), 1);
+        assert_eq!(last_metadata_file, None);
+
+        // Replay the same file. Because Stop cleared last_metadata_file,
+        // dispatch_events must emit Metadata again and reinitialise tracking.
+        let replay_a = state("a.flac", "A", "Alb", "Song A", 0, "play");
+        dispatch_events(&stop_a, &replay_a, &mut tracker, &mut last_metadata_file);
+        let stop_again = state("a.flac", "A", "Alb", "Song A", 0, "stop");
+        dispatch_events(
+            &replay_a,
+            &stop_again,
+            &mut tracker,
+            &mut last_metadata_file,
+        );
+
+        assert_eq!(scrobbled.borrow().len(), 2);
+    }
+
     // -----------------------------------------------------------------------
     // MpdConfig
     // -----------------------------------------------------------------------

src/report.rs

@@ -601,12 +601,7 @@ pub fn print_terminal_report(data: &ReportData) {
                 ]
             })
             .collect();
-        print_box_table(
+        print_box_table(&["Source", "Scrobbles", "Time"], &rows, None, &[0]);
-            &["Source", "Scrobbles", "Time"],
-            &rows,
-            None,
-            &[0],
-        );
     }
 
     // --- Top Artists ---
@@ -927,14 +922,13 @@ pub fn render_html_report(conn: &Connection, limit: i64, all_time_limit: i64) ->
                 .map(|(i, s)| {
                     // Prefix the source name with a colour dot matching the
                     // palette slot assigned to it in the all-time ranking.
-                    let label = if let Some((bg, _)) =
+                    let label = if let Some((bg, _)) = source_colours(&s.source, &ordered_sources) {
-                        source_colours(&s.source, &ordered_sources)
-                    {
                         format!(
                             "<span style=\"display:inline-block;width:9px;height:9px;\
                              border-radius:50%;background:{};margin-right:5px;\
                              vertical-align:middle\"></span>{}",
-                            bg, html_escape(&s.source)
+                            bg,
+                            html_escape(&s.source)
                         )
                     } else {
                         html_escape(&s.source)
@@ -986,10 +980,7 @@ pub fn render_html_report(conn: &Connection, limit: i64, all_time_limit: i64) ->
                     .as_deref()
                     .and_then(|src| source_colours(src, &ordered_sources))
                     .map(|(bg, border)| {
-                        format!(
+                        format!(" style=\"background:{};border-color:{}\"", bg, border)
-                            " style=\"background:{};border-color:{}\"",
-                            bg, border
-                        )
                     })
                     .unwrap_or_default();
                 h.linef(format_args!("<article class=\"album\"{}>", card_style));
@@ -1080,11 +1071,7 @@ pub fn render_html_report(conn: &Connection, limit: i64, all_time_limit: i64) ->
                     html_escape(&a.album)
                 };
                 BarRow {
-                    cells: vec![
+                    cells: vec![(i + 1).to_string(), html_escape(&a.artist), album_cell],
-                        (i + 1).to_string(),
-                        html_escape(&a.artist),
-                        album_cell,
-                    ],
                     value: a.plays,
                     suffix: format_duration(a.listen_time_secs),
                     cover,
@@ -1373,10 +1360,7 @@ const SOURCE_PALETTE: &[(&str, &str)] = &[
 
 /// Look up the card background and border colours for a source name, given
 /// the ordered list of all-time sources (most played first).
-fn source_colours<'a>(
+fn source_colours<'a>(source: &str, ordered_sources: &[String]) -> Option<(&'a str, &'a str)> {
-    source: &str,
-    ordered_sources: &[String],
-) -> Option<(&'a str, &'a str)> {
     ordered_sources
         .iter()
         .position(|s| s == source)
@@ -1549,7 +1533,10 @@ fn write_bar_table(h: &mut HtmlWriter, title: &str, headers: &[&str], rows: &[Ba
     let play_count_idx = headers.len().saturating_sub(2);
     for (i, hdr) in headers.iter().enumerate() {
         if i == play_count_idx {
-            h.linef(format_args!("<th class=\"play-count\">{}</th>", html_escape(hdr)));
+            h.linef(format_args!(
+                "<th class=\"play-count\">{}</th>",
+                html_escape(hdr)
+            ));
         } else {
             h.linef(format_args!("<th>{}</th>", html_escape(hdr)));
         }

src/watcher.rs

@@ -128,6 +128,7 @@ impl CurrentTrack {
 pub struct ScrobbleTracker<F: FnMut(NewScrobble)> {
     current_track: Option<CurrentTrack>,
     is_playing: bool,
+    has_seen_status: bool,
     playing_since: Option<Instant>,
     accumulated_secs: f64,
     scrobble_fn: F,
@@ -140,6 +141,7 @@ impl<F: FnMut(NewScrobble)> ScrobbleTracker<F> {
         Self {
             current_track: None,
             is_playing: false,
+            has_seen_status: false,
             playing_since: None,
             accumulated_secs: 0.0,
             scrobble_fn,
@@ -151,9 +153,10 @@ impl<F: FnMut(NewScrobble)> ScrobbleTracker<F> {
     ///
     /// ## Event handling:
     ///
-    /// - **Metadata**: A new track started. Evaluate the previous track
+    /// - **Metadata**: A track boundary or metadata refresh. If metadata
-    ///   (scrobble if threshold met), then begin tracking the new one.
+    ///   points to a different track, evaluate the previous one (scrobble if
-    ///   We assume the new track starts in Playing state.
+    ///   threshold met), then begin tracking the new one. If metadata refers
+    ///   to the same track, update fields in place and keep timing state.
     ///
     /// - **Status(Playing)**: Resume accumulating play time. If already
     ///   playing, this is a no-op (avoids double-counting).
@@ -178,6 +181,20 @@ impl<F: FnMut(NewScrobble)> ScrobbleTracker<F> {
                 title,
                 duration_us,
             } => {
+                // Some players emit metadata updates for the same track (for
+                // example around pause/resume or delayed tag updates). Treat
+                // those as in-place updates, not as a track boundary.
+                if let Some(current) = &mut self.current_track
+                    && current.artist == artist
+                    && current.album == album
+                    && current.title == title
+                {
+                    if duration_us.is_some() {
+                        current.duration_us = duration_us;
+                    }
+                    return;
+                }
+
                 // Evaluate the previous track before switching to the new one.
                 self.evaluate_previous_track();
 
@@ -190,12 +207,23 @@ impl<F: FnMut(NewScrobble)> ScrobbleTracker<F> {
                 });
                 self.accumulated_secs = 0.0;
 
-                // A metadata event means the player is actively playing the new track.
+                // Do not blindly assume metadata implies Playing. If we've
+                // never seen status yet, fall back to assuming playback has
+                // started so startup still works when metadata arrives first.
+                if self.has_seen_status {
+                    self.playing_since = if self.is_playing {
+                        Some(Instant::now())
+                    } else {
+                        None
+                    };
+                } else {
                     self.is_playing = true;
                     self.playing_since = Some(Instant::now());
                 }
+            }
             Event::Status(status) => match status {
                 PlayerStatus::Playing => {
+                    self.has_seen_status = true;
                     // Only start the clock if we weren't already playing.
                     // This prevents double-counting if we receive redundant Playing events.
                     if !self.is_playing {
@@ -204,6 +232,7 @@ impl<F: FnMut(NewScrobble)> ScrobbleTracker<F> {
                     }
                 }
                 PlayerStatus::Paused => {
+                    self.has_seen_status = true;
                     // Flush the elapsed time from the current playing stretch
                     // into the accumulator, then stop the clock.
                     self.flush_playing_time();
@@ -211,6 +240,7 @@ impl<F: FnMut(NewScrobble)> ScrobbleTracker<F> {
                     self.playing_since = None;
                 }
                 PlayerStatus::Stopped => {
+                    self.has_seen_status = true;
                     // Treat Stop as a final decision: evaluate the current
                     // track now (scrobble if threshold met, discard if not)
                     // and clear all tracking state. This prevents a spurious
@@ -219,6 +249,7 @@ impl<F: FnMut(NewScrobble)> ScrobbleTracker<F> {
                     self.evaluate_previous_track();
                     self.accumulated_secs = 0.0;
                     self.is_playing = false;
+                    self.playing_since = None;
                 }
             },
             Event::Eof => {
@@ -380,20 +411,26 @@ pub fn create_db_tracker(
     conn: std::sync::Arc<std::sync::Mutex<Connection>>,
     source: String,
 ) -> ScrobbleTracker<impl FnMut(NewScrobble)> {
-    ScrobbleTracker::new(move |scrobble: NewScrobble| {
+    ScrobbleTracker::new(
+        move |scrobble: NewScrobble| {
             let conn = conn.lock().unwrap();
             match db::insert_scrobble(&conn, &scrobble) {
                 Ok(_) => {
                     eprintln!(
                         "[scrobbled] {}: {} - {} ({}s)",
-                    scrobble.source, scrobble.artist, scrobble.title, scrobble.played_duration_secs
+                        scrobble.source,
+                        scrobble.artist,
+                        scrobble.title,
+                        scrobble.played_duration_secs
                     );
                 }
                 Err(e) => {
                     eprintln!("[error] Failed to insert scrobble: {}", e);
                 }
             }
-    }, source)
+        },
+        source,
+    )
 }
 
 // ===========================================================================
@@ -410,6 +447,7 @@ pub fn create_db_tracker(
 pub struct TestableTracker {
     current_track: Option<CurrentTrack>,
     is_playing: bool,
+    has_seen_status: bool,
     /// Simulated timestamp (in seconds) when the current Playing stretch began.
     playing_since_secs: Option<f64>,
     /// Accumulated play time for the current track (in seconds).
@@ -428,6 +466,7 @@ impl TestableTracker {
         Self {
             current_track: None,
             is_playing: false,
+            has_seen_status: false,
             playing_since_secs: None,
             accumulated_secs: 0.0,
             scrobbled: Vec::new(),
@@ -484,6 +523,17 @@ impl TestableTracker {
                 title,
                 duration_us,
             } => {
+                if let Some(current) = &mut self.current_track
+                    && current.artist == artist
+                    && current.album == album
+                    && current.title == title
+                {
+                    if duration_us.is_some() {
+                        current.duration_us = duration_us;
+                    }
+                    return;
+                }
+
                 self.evaluate_previous_track();
                 self.current_track = Some(CurrentTrack {
                     artist,
@@ -492,25 +542,37 @@ impl TestableTracker {
                     duration_us,
                 });
                 self.accumulated_secs = 0.0;
+                if self.has_seen_status {
+                    self.playing_since_secs = if self.is_playing {
+                        Some(self.clock_secs)
+                    } else {
+                        None
+                    };
+                } else {
                     self.is_playing = true;
                     self.playing_since_secs = Some(self.clock_secs);
                 }
+            }
             Event::Status(status) => match status {
                 PlayerStatus::Playing => {
+                    self.has_seen_status = true;
                     if !self.is_playing {
                         self.is_playing = true;
                         self.playing_since_secs = Some(self.clock_secs);
                     }
                 }
                 PlayerStatus::Paused => {
+                    self.has_seen_status = true;
                     self.flush_playing_time();
                     self.is_playing = false;
                     self.playing_since_secs = None;
                 }
                 PlayerStatus::Stopped => {
+                    self.has_seen_status = true;
                     self.evaluate_previous_track();
                     self.accumulated_secs = 0.0;
                     self.is_playing = false;
+                    self.playing_since_secs = None;
                 }
             },
             Event::Eof => {
@@ -876,6 +938,82 @@ mod tests {
         assert_eq!(tracker.scrobbled[1].title, "Digital Bath");
     }
 
+    #[test]
+    fn test_duplicate_metadata_same_track_does_not_double_scrobble() {
+        // Some players emit duplicate metadata lines for the same track.
+        // They must not split a single listen into multiple scrobbles.
+        let mut tracker = TestableTracker::new();
+
+        tracker.handle_event(Event::Metadata {
+            artist: "††† (Crosses)".into(),
+            album: "††† (Crosses)".into(),
+            title: "This Is a Trick".into(),
+            duration_us: Some(186_000_000), // threshold = 93s
+        });
+        tracker.advance_time(50.0);
+
+        // Duplicate metadata for the same track should be ignored as a boundary.
+        tracker.handle_event(Event::Metadata {
+            artist: "††† (Crosses)".into(),
+            album: "††† (Crosses)".into(),
+            title: "This Is a Trick".into(),
+            duration_us: Some(186_000_000),
+        });
+        tracker.advance_time(50.0);
+
+        // Next track triggers evaluation.
+        tracker.handle_event(Event::Metadata {
+            artist: "Deftones".into(),
+            album: "White Pony".into(),
+            title: "Digital Bath".into(),
+            duration_us: Some(291_000_000),
+        });
+
+        assert_eq!(tracker.scrobbled.len(), 1);
+        assert_eq!(tracker.scrobbled[0].title, "This Is a Trick");
+        assert_eq!(tracker.scrobbled[0].played_duration_secs, 100);
+    }
+
+    #[test]
+    fn test_metadata_while_paused_does_not_start_clock() {
+        // If metadata arrives while paused, we should not treat it as playing.
+        let mut tracker = TestableTracker::new();
+
+        tracker.handle_event(Event::Metadata {
+            artist: "††† (Crosses)".into(),
+            album: "††† (Crosses)".into(),
+            title: "Telepathy".into(),
+            duration_us: Some(215_000_000), // threshold = 107.5s
+        });
+
+        tracker.advance_time(30.0);
+        tracker.handle_event(Event::Status(PlayerStatus::Paused));
+
+        // Metadata update while paused (same track, perhaps with better tags).
+        tracker.handle_event(Event::Metadata {
+            artist: "††† (Crosses)".into(),
+            album: "††† (Crosses)".into(),
+            title: "Telepathy".into(),
+            duration_us: Some(215_000_000),
+        });
+
+        // Long paused stretch should not count.
+        tracker.advance_time(500.0);
+
+        // Resume and play a bit more, still below threshold overall.
+        tracker.handle_event(Event::Status(PlayerStatus::Playing));
+        tracker.advance_time(30.0);
+
+        tracker.handle_event(Event::Metadata {
+            artist: "Deftones".into(),
+            album: "White Pony".into(),
+            title: "Digital Bath".into(),
+            duration_us: Some(291_000_000),
+        });
+
+        assert_eq!(tracker.scrobbled.len(), 0);
+    }
+
     #[test]
     fn test_stop_below_threshold_no_scrobble() {
         // Play a track for 3 seconds then stop — should NOT be scrobbled,