Fix correctness and robustness issues found in a full code review

Correctness:

- MPD command reads no longer abandon a response half-way. The socket's
  500 ms read timeout exists so the idle loop can poll the shutdown flag,
  but it applied to every read, so a slow reply left unread bytes that the
  next command parsed as its own. Add MpdConn::read_line/read_exact, which
  retry across that timeout up to a 15 s deadline, and a desynced flag that
  forces a reconnect when a read is genuinely abandoned. read_response now
  returns Option so a truncated reply is a failure rather than an empty
  result -- a timed-out currentsong used to look like "MPD is stopped".
- resolve_cover returned a covers/<name> path even when the file was
  missing, so a deleted image produced a broken <img> instead of the
  placeholder. Gate the return value on the same existence check as the
  copy list.
- enrich --force no longer runs DELETE FROM album_cache. That discarded
  pin-album entries, MPD-extracted cover paths, and any genre the re-fetch
  then failed to find. Select every album via all_scrobbled_albums and
  overwrite rows in place instead.
- Album grouping, cover fallback and source attribution disagreed: the
  dominant_source subquery and artist_cover matched on album name alone, so
  two records sharing a title cross-contaminated. Extract one
  album_group_key_expr and use it everywhere.
- Escape quotes and backslashes in MusicBrainz Lucene queries; a title
  containing a quote closed the term early and matched nothing.
- Size the HTML cover grid from the period's own limit, so all-time no
  longer shows fewer covers than the table beneath it.
- Fix a panic in the terminal report: top_genres ranks broad single-word
  genres last, so first() is not the busiest row, and a later genre with
  more plays overflowed the bar width. Scale to the true maximum and clamp
  the bar arithmetic.
- Bound readpicture transfers by size and chunk count so a bogus size:
  header cannot drive an unbounded allocation or an endless loop.

Robustness:

- Kill the playerctl children before joining their reader threads. The
  threads block until stdout closes, which only happened because an
  interactive Ctrl+C signals the whole process group; under a supervisor
  that signals one PID, shutdown hung and left orphans.
- Reject --limit and --all-time-limit below 1. SQLite reads a negative
  LIMIT as unlimited, silently dumping the whole library.
- Set a 5 s busy_timeout, so running report against a live watch does not
  fail immediately with "database is locked".
- Replace panics in default_db_path, covers_dir and the enrich queries with
  reported errors, and check PRAGMA table_info for the source column
  migration instead of discarding every ALTER TABLE error.

Cleanups:

- Collapse eight reset_*_timestamps functions into reset_retry_timestamps
  with a RetryScope enum.
- Share one FNV-1a helper (enrich::cover_stem) between the MPD and iTunes
  filename generators.
- Use params_from_iter to remove six duplicated query_map branches.
- Extract source_dot_cell and reuse db::split_genre_list in the report.
- Skip the genre lookup when only covers were requested, saving a request
  and a rate-limit sleep per album.
- Keep the cached genre in pin-album when MusicBrainz returns nothing and a
  --cover-url was supplied.

README.md

@@ -360,6 +360,11 @@ Use `--force` to re-fetch all albums from scratch (online pipeline only):
 scrbblr enrich --pipeline online --fetch all --force
 ```
 
+Note: `--force` revisits every scrobbled album and overwrites each cache row in
+place; it does not empty the cache first. An album whose re-fetch turns up
+nothing keeps the data it already had, so covers pinned with `pin-album` and
+covers extracted from MPD survive a forced run.
+
 Genre normalisation notes:
 
 - Genre labels are passed through from MusicBrainz with light cleanup only.

src/db.rs

@@ -27,10 +27,16 @@
 //! duplicating it across every query function.
 
 use chrono::{Datelike, NaiveDateTime};
-use rusqlite::{Connection, Result, params};
+use rusqlite::{Connection, Result, params, params_from_iter};
 use serde::Serialize;
 use std::collections::HashMap;
 
+/// How long to wait for a competing writer before returning SQLITE_BUSY.
+///
+/// `report` is routinely run while `watch` holds the database open, so without
+/// this a read that lands during a scrobble insert fails immediately.
+const BUSY_TIMEOUT_MS: u32 = 5_000;
+
 // ---------------------------------------------------------------------------
 // Data structures
 // ---------------------------------------------------------------------------
@@ -143,6 +149,10 @@ pub struct TopSource {
 /// all CREATE statements use `IF NOT EXISTS`.
 pub fn open_db(path: &str) -> Result<Connection> {
     let conn = Connection::open(path)?;
+    // `watch` and `report` are frequently run against the same file at the
+    // same time. Without a busy timeout, any overlap surfaces as an immediate
+    // "database is locked" error instead of a short wait.
+    conn.busy_timeout(std::time::Duration::from_millis(BUSY_TIMEOUT_MS as u64))?;
     init_schema(&conn)?;
     Ok(conn)
 }
@@ -203,10 +213,13 @@ fn init_schema(conn: &Connection) -> Result<()> {
         CREATE INDEX IF NOT EXISTS idx_scrobbled_at ON scrobbles(scrobbled_at);
         CREATE INDEX IF NOT EXISTS idx_artist ON scrobbles(artist);",
     )?;
-    // Add the source column to existing databases. SQLite returns an error if
+    // Add the `source` column to databases created before it existed. Check
-    // the column already exists, which we silently ignore for idempotency.
+    // the current columns first rather than running the ALTER and discarding
-    conn.execute("ALTER TABLE scrobbles ADD COLUMN source TEXT", [])
+    // every error — that would also hide genuine failures (disk full, the
-        .ok();
+    // table being locked, a corrupt file).
+    if !column_exists(conn, "scrobbles", "source")? {
+        conn.execute("ALTER TABLE scrobbles ADD COLUMN source TEXT", [])?;
+    }
     conn.execute_batch(
         "
         -- Cache table for album metadata (cover art, genres) from MusicBrainz.
@@ -226,6 +239,21 @@ fn init_schema(conn: &Connection) -> Result<()> {
     Ok(())
 }
 
+/// Return true if `table` already has a column named `column`.
+///
+/// Used to make additive migrations idempotent without swallowing real errors.
+fn column_exists(conn: &Connection, table: &str, column: &str) -> Result<bool> {
+    let mut stmt = conn.prepare(&format!("PRAGMA table_info({})", table))?;
+    let mut rows = stmt.query([])?;
+    while let Some(row) = rows.next()? {
+        let name: String = row.get(1)?;
+        if name == column {
+            return Ok(true);
+        }
+    }
+    Ok(false)
+}
+
 // ---------------------------------------------------------------------------
 // Insert
 // ---------------------------------------------------------------------------
@@ -346,8 +374,11 @@ fn where_clause(period: &str) -> (String, Vec<String>) {
 
 /// Get aggregate overview statistics for the given period.
 ///
-/// Unique tracks are counted by concatenating artist + null byte + title,
+/// Unique tracks are counted by concatenating artist + a `\0` separator +
-/// so that "Artist A - Song X" and "Artist B - Song X" are counted separately.
+/// title, so that "Artist A - Song X" and "Artist B - Song X" are counted
+/// separately. The separator is the literal two-character sequence backslash-
+/// zero rather than a NUL byte, which SQLite string literals cannot express;
+/// either way it will not occur inside a real artist or track name.
 pub fn overview(conn: &Connection, period: &str) -> Result<Overview> {
     let (whr, p) = where_clause(period);
     let sql = format!(
@@ -362,28 +393,15 @@ pub fn overview(conn: &Connection, period: &str) -> Result<Overview> {
     );
 
     let mut stmt = conn.prepare(&sql)?;
-    let row = if p.is_empty() {
+    stmt.query_row(params_from_iter(p.iter()), |row| {
-        stmt.query_row([], |row| {
+        Ok(Overview {
-            Ok(Overview {
+            total_scrobbles: row.get(0)?,
-                total_scrobbles: row.get(0)?,
+            total_listen_time_secs: row.get(1)?,
-                total_listen_time_secs: row.get(1)?,
+            unique_artists: row.get(2)?,
-                unique_artists: row.get(2)?,
+            unique_albums: row.get(3)?,
-                unique_albums: row.get(3)?,
+            unique_tracks: row.get(4)?,
-                unique_tracks: row.get(4)?,
-            })
         })
-    } else {
+    })
-        stmt.query_row(params![p[0], p[1]], |row| {
-            Ok(Overview {
-                total_scrobbles: row.get(0)?,
-                total_listen_time_secs: row.get(1)?,
-                unique_artists: row.get(2)?,
-                unique_albums: row.get(3)?,
-                unique_tracks: row.get(4)?,
-            })
-        })
-    }?;
-    Ok(row)
 }
 
 /// Get the top N artists by play count for the given period,
@@ -403,18 +421,14 @@ pub fn top_artists(conn: &Connection, period: &str, limit: i64) -> Result<Vec<To
     );
 
     let mut stmt = conn.prepare(&sql)?;
-    let mapper = |row: &rusqlite::Row| {
+    stmt.query_map(params_from_iter(p.iter()), |row| {
         Ok(TopArtist {
             artist: row.get(0)?,
             plays: row.get(1)?,
             listen_time_secs: row.get(2)?,
         })
-    };
+    })?
-    if p.is_empty() {
+    .collect()
-        stmt.query_map([], mapper)?.collect()
-    } else {
-        stmt.query_map(params![p[0], p[1]], mapper)?.collect()
-    }
 }
 
 /// Get the top N albums by play count for the given period,
@@ -433,19 +447,6 @@ pub fn top_artists(conn: &Connection, period: &str, limit: i64) -> Result<Vec<To
 /// names for stable output ordering.
 pub fn top_albums(conn: &Connection, period: &str, limit: i64) -> Result<Vec<TopAlbum>> {
     let (whr, p) = where_clause(period);
-    // Group by (album, MBID) rather than (artist, album) so that scrobbles
-    // tagged with different artist names for the same physical release are
-    // counted together.
-    //
-    // For each row the group key resolves as:
-    //   1. The MBID from the direct (artist, album) album_cache entry, or
-    //   2. The MBID from any album_cache entry sharing the same album name
-    //      (catches variants tagged with a performer vs. composer), or
-    //   3. The composite "artist::album" string (no MBID found — treated as
-    //      a unique album, same as before).
-    //
-    // The displayed artist is similarly resolved: the enriched (cached)
-    // artist takes priority so that album_cache_meta() can find the cover.
     // The outer query uses `whr` which starts with " WHERE ...".
     // The dominant_source subquery already has its own WHERE, so we need an
     // AND-form of the same filter to avoid double WHERE.
@@ -455,6 +456,13 @@ pub fn top_albums(conn: &Connection, period: &str, limit: i64) -> Result<Vec<Top
         " AND s2.scrobbled_at BETWEEN ?1 AND ?2".to_string()
     };
 
+    let group_key = album_group_key_expr("s", "c");
+    // The dominant_source subquery must aggregate over exactly the rows that
+    // the outer GROUP BY collapses into this row. Matching on album name alone
+    // would pull in unrelated albums that happen to share a title, so the
+    // subquery re-derives the same grouping key and compares it.
+    let subquery_group_key = album_group_key_expr("s2", "c2");
+
     let sql = format!(
         "SELECT
              CASE WHEN c.artist IS NOT NULL THEN s.artist
@@ -472,27 +480,22 @@ pub fn top_albums(conn: &Connection, period: &str, limit: i64) -> Result<Vec<Top
              -- of this album in the queried period. Used to colour-code cards.
              (SELECT s2.source
               FROM scrobbles s2
+              LEFT JOIN album_cache c2 ON c2.artist = s2.artist AND c2.album = s2.album
               WHERE s2.album = s.album
-                AND s2.source IS NOT NULL{}
+                AND s2.source IS NOT NULL
+                AND {subquery_group_key} = {group_key}{subquery_period_filter}
               GROUP BY s2.source
               ORDER BY COUNT(*) DESC
               LIMIT 1) AS dominant_source
          FROM scrobbles s
-         LEFT JOIN album_cache c ON c.artist = s.artist AND c.album = s.album{}
+         LEFT JOIN album_cache c ON c.artist = s.artist AND c.album = s.album{whr}
-         GROUP BY s.album, COALESCE(
+         GROUP BY s.album, {group_key}
-             c.musicbrainz_id,
-             (SELECT ac.musicbrainz_id FROM album_cache ac
-              WHERE ac.album = s.album AND ac.musicbrainz_id IS NOT NULL
-              ORDER BY ac.musicbrainz_id LIMIT 1),
-             s.artist || '::' || s.album
-         )
          ORDER BY plays DESC, listen_time DESC, artist ASC, album ASC
-         LIMIT {}",
+         LIMIT {limit}"
-        subquery_period_filter, whr, limit
     );
 
     let mut stmt = conn.prepare(&sql)?;
-    let mapper = |row: &rusqlite::Row| {
+    stmt.query_map(params_from_iter(p.iter()), |row| {
         Ok(TopAlbum {
             artist: row.get(0)?,
             album: row.get(1)?,
@@ -500,12 +503,42 @@ pub fn top_albums(conn: &Connection, period: &str, limit: i64) -> Result<Vec<Top
             listen_time_secs: row.get(3)?,
             dominant_source: row.get(4)?,
         })
-    };
+    })?
-    if p.is_empty() {
+    .collect()
-        stmt.query_map([], mapper)?.collect()
+}
-    } else {
+
-        stmt.query_map(params![p[0], p[1]], mapper)?.collect()
+/// SQL expression resolving the grouping key for one `scrobbles` row.
-    }
+///
+/// Scrobbles that resolve to the same key are treated as the same physical
+/// album. `scrobble_alias` is the alias of the `scrobbles` row; `cache_alias`
+/// is the alias of an `album_cache` row LEFT JOINed on
+/// `(artist, album)` — the caller must provide that join.
+///
+/// The key resolves as:
+///   1. The MBID from the direct (artist, album) album_cache entry, or
+///   2. The MBID from any album_cache entry sharing the same album name
+///      (catches variants tagged with a performer vs. composer), or
+///   3. The composite "artist::album" string (no MBID found — treated as a
+///      unique album).
+///
+/// Note: step 2 keys on album name alone, so two genuinely different albums
+/// that share a title (a self-titled debut, "Greatest Hits") will merge if
+/// exactly one of them has been enriched. Distinguishing them would require
+/// per-scrobble release IDs, which neither MPRIS nor MPD provides.
+///
+/// Every caller that needs to reason about these groups must use this same
+/// expression, so that grouping, cover lookup and source attribution agree.
+fn album_group_key_expr(scrobble_alias: &str, cache_alias: &str) -> String {
+    format!(
+        "COALESCE(
+             {cache_alias}.musicbrainz_id,
+             (SELECT ac.musicbrainz_id FROM album_cache ac
+              WHERE ac.album = {scrobble_alias}.album
+                AND ac.musicbrainz_id IS NOT NULL
+              ORDER BY ac.musicbrainz_id LIMIT 1),
+             {scrobble_alias}.artist || '::' || {scrobble_alias}.album
+         )"
+    )
 }
 
 /// Get the top N tracks by play count for the given period,
@@ -546,7 +579,7 @@ pub fn top_tracks(conn: &Connection, period: &str, limit: i64) -> Result<Vec<Top
     );
 
     let mut stmt = conn.prepare(&sql)?;
-    let mapper = |row: &rusqlite::Row| {
+    stmt.query_map(params_from_iter(p.iter()), |row| {
         Ok(TopTrack {
             artist: row.get(0)?,
             album: row.get(1)?,
@@ -555,12 +588,8 @@ pub fn top_tracks(conn: &Connection, period: &str, limit: i64) -> Result<Vec<Top
             listen_time_secs: row.get(4)?,
             dominant_source: row.get(5)?,
         })
-    };
+    })?
-    if p.is_empty() {
+    .collect()
-        stmt.query_map([], mapper)?.collect()
-    } else {
-        stmt.query_map(params![p[0], p[1]], mapper)?.collect()
-    }
 }
 
 /// Get the top N genres for the given period.
@@ -668,18 +697,14 @@ pub fn top_sources(conn: &Connection, period: &str) -> Result<Vec<TopSource>> {
     );
 
     let mut stmt = conn.prepare(&sql)?;
-    let mapper = |row: &rusqlite::Row| {
+    stmt.query_map(params_from_iter(p.iter()), |row| {
         Ok(TopSource {
             source: row.get(0)?,
             scrobbles: row.get(1)?,
             listen_time_secs: row.get(2)?,
         })
-    };
+    })?
-    if p.is_empty() {
+    .collect()
-        stmt.query_map([], mapper)?.collect()
-    } else {
-        stmt.query_map(params![p[0], p[1]], mapper)?.collect()
-    }
 }
 
 /// Get the most recent scrobbles for the given period, ordered by
@@ -695,7 +720,7 @@ pub fn recent_scrobbles(conn: &Connection, period: &str, limit: i64) -> Result<V
     );
 
     let mut stmt = conn.prepare(&sql)?;
-    let mapper = |row: &rusqlite::Row| {
+    stmt.query_map(params_from_iter(p.iter()), |row| {
         Ok(Scrobble {
             id: row.get(0)?,
             artist: row.get(1)?,
@@ -705,12 +730,8 @@ pub fn recent_scrobbles(conn: &Connection, period: &str, limit: i64) -> Result<V
             played_duration_secs: row.get(5)?,
             scrobbled_at: row.get(6)?,
         })
-    };
+    })?
-    if p.is_empty() {
+    .collect()
-        stmt.query_map([], mapper)?.collect()
-    } else {
-        stmt.query_map(params![p[0], p[1]], mapper)?.collect()
-    }
 }
 
 // ---------------------------------------------------------------------------
@@ -725,6 +746,83 @@ pub struct UncachedAlbum {
     pub album: String,
 }
 
+/// Which incomplete `album_cache` rows a cooldown reset should target.
+///
+/// Selected by `enrich --retry <mode>`. Resetting `fetched_at` to NULL makes
+/// [`uncached_albums`] pick the row up again on the next enrichment run,
+/// bypassing the usual 7-day cooldown.
+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
+pub enum RetryScope {
+    /// Rows with no `cover_url`.
+    Covers,
+    /// Rows with no `genre`.
+    Genres,
+    /// Rows missing either `cover_url` or `genre`.
+    All,
+    /// Rows with no `genre` for albums that were scrobbled from MPD.
+    MpdGenres,
+}
+
+impl RetryScope {
+    /// The `WHERE` fragment selecting the rows this scope targets.
+    fn predicate(self) -> &'static str {
+        match self {
+            Self::Covers => "cover_url IS NULL",
+            Self::Genres => "genre IS NULL",
+            Self::All => "(cover_url IS NULL OR genre IS NULL)",
+            Self::MpdGenres => {
+                "genre IS NULL
+                   AND EXISTS (
+                     SELECT 1 FROM scrobbles s
+                     WHERE s.artist = album_cache.artist
+                       AND s.album = album_cache.album
+                       AND s.source = 'MPD'
+                   )"
+            }
+        }
+    }
+
+    /// The CLI spelling of this scope, for progress messages.
+    pub fn as_str(self) -> &'static str {
+        match self {
+            Self::Covers => "covers",
+            Self::Genres => "genres",
+            Self::All => "all",
+            Self::MpdGenres => "mpd-genres",
+        }
+    }
+}
+
+/// Reset `fetched_at` to NULL for the `album_cache` rows matching `scope`,
+/// returning how many rows were updated.
+///
+/// When `artist` is `Some`, only that artist's rows are touched; matching is
+/// case-insensitive (`LOWER(artist) = LOWER(?1)`) so callers can pass user
+/// input without worrying about exact casing.
+pub fn reset_retry_timestamps(
+    conn: &Connection,
+    scope: RetryScope,
+    artist: Option<&str>,
+) -> Result<usize> {
+    let sql = format!(
+        "UPDATE album_cache
+         SET fetched_at = NULL
+         WHERE {}{}",
+        scope.predicate(),
+        if artist.is_some() {
+            "\n           AND LOWER(artist) = LOWER(?1)"
+        } else {
+            ""
+        }
+    );
+
+    let count = match artist {
+        Some(name) => conn.execute(&sql, params![name])?,
+        None => conn.execute(&sql, [])?,
+    };
+    Ok(count)
+}
+
 /// Find all unique (artist, album) pairs in `scrobbles` that either:
 /// - don't have a corresponding row in `album_cache` at all, or
 /// - have a cached row but with `cover_url` still NULL, or
@@ -733,133 +831,6 @@ pub struct UncachedAlbum {
 /// This ensures albums are re-tried if a previous run failed to find cover art
 /// or genre metadata, but only after a cooldown to avoid hitting MusicBrainz
 /// on every single report generation.
-/// Reset the `fetched_at` timestamp to NULL for all `album_cache` rows that
-/// have no `cover_url`. This makes [`uncached_albums`] pick them up again on
-/// the next enrichment run, bypassing the 7-day cooldown.
-///
-/// Used by `enrich --pipeline online --retry covers` to re-attempt cover
-/// downloads
-/// without forcing a full re-fetch of all albums.
-pub fn reset_missing_cover_timestamps(conn: &Connection) -> Result<usize> {
-    let count = conn.execute(
-        "UPDATE album_cache SET fetched_at = NULL WHERE cover_url IS NULL",
-        [],
-    )?;
-    Ok(count)
-}
-
-/// Reset `fetched_at` to NULL for cover-missing albums for one artist only.
-///
-/// Matching is case-insensitive (`LOWER(artist) = LOWER(?1)`), so callers can
-/// pass user input without worrying about exact casing.
-pub fn reset_missing_cover_timestamps_for_artist(conn: &Connection, artist: &str) -> Result<usize> {
-    let count = conn.execute(
-        "UPDATE album_cache
-         SET fetched_at = NULL
-         WHERE cover_url IS NULL
-           AND LOWER(artist) = LOWER(?1)",
-        params![artist],
-    )?;
-    Ok(count)
-}
-
-/// Reset the `fetched_at` timestamp to NULL for all rows that have no genre.
-///
-/// Used when the user explicitly wants to retry genre lookups immediately,
-/// bypassing the default 7-day cooldown.
-pub fn reset_missing_genre_timestamps(conn: &Connection) -> Result<usize> {
-    let count = conn.execute(
-        "UPDATE album_cache
-         SET fetched_at = NULL
-         WHERE genre IS NULL",
-        [],
-    )?;
-    Ok(count)
-}
-
-/// Reset `fetched_at` to NULL for genre-missing albums for one artist only.
-///
-/// Matching is case-insensitive (`LOWER(artist) = LOWER(?1)`).
-pub fn reset_missing_genre_timestamps_for_artist(conn: &Connection, artist: &str) -> Result<usize> {
-    let count = conn.execute(
-        "UPDATE album_cache
-         SET fetched_at = NULL
-         WHERE genre IS NULL
-           AND LOWER(artist) = LOWER(?1)",
-        params![artist],
-    )?;
-    Ok(count)
-}
-
-/// Reset `fetched_at` to NULL for rows missing either cover or genre.
-pub fn reset_incomplete_timestamps(conn: &Connection) -> Result<usize> {
-    let count = conn.execute(
-        "UPDATE album_cache
-         SET fetched_at = NULL
-         WHERE cover_url IS NULL OR genre IS NULL",
-        [],
-    )?;
-    Ok(count)
-}
-
-/// Reset `fetched_at` to NULL for incomplete rows for one artist only.
-///
-/// A row is considered incomplete when either `cover_url` or `genre` is NULL.
-/// Matching is case-insensitive (`LOWER(artist) = LOWER(?1)`).
-pub fn reset_incomplete_timestamps_for_artist(conn: &Connection, artist: &str) -> Result<usize> {
-    let count = conn.execute(
-        "UPDATE album_cache
-         SET fetched_at = NULL
-         WHERE (cover_url IS NULL OR genre IS NULL)
-           AND LOWER(artist) = LOWER(?1)",
-        params![artist],
-    )?;
-    Ok(count)
-}
-
-/// Reset `fetched_at` to NULL for MPD-sourced albums that have no genre yet.
-///
-/// This allows online enrichment to re-try genre lookups immediately for
-/// MPD albums, bypassing the 7-day cooldown used by [`uncached_albums`].
-pub fn reset_missing_genre_timestamps_for_mpd(conn: &Connection) -> Result<usize> {
-    let count = conn.execute(
-        "UPDATE album_cache
-         SET fetched_at = NULL
-         WHERE genre IS NULL
-           AND EXISTS (
-             SELECT 1 FROM scrobbles s
-             WHERE s.artist = album_cache.artist
-               AND s.album = album_cache.album
-               AND s.source = 'MPD'
-           )",
-        [],
-    )?;
-    Ok(count)
-}
-
-/// Reset `fetched_at` to NULL for one artist's MPD-sourced genre-missing albums.
-///
-/// Matching is case-insensitive (`LOWER(artist) = LOWER(?1)`).
-pub fn reset_missing_genre_timestamps_for_mpd_artist(
-    conn: &Connection,
-    artist: &str,
-) -> Result<usize> {
-    let count = conn.execute(
-        "UPDATE album_cache
-         SET fetched_at = NULL
-         WHERE genre IS NULL
-           AND LOWER(artist) = LOWER(?1)
-           AND EXISTS (
-             SELECT 1 FROM scrobbles s
-             WHERE s.artist = album_cache.artist
-               AND s.album = album_cache.album
-               AND s.source = 'MPD'
-           )",
-        params![artist],
-    )?;
-    Ok(count)
-}
-
 pub fn uncached_albums(conn: &Connection) -> Result<Vec<UncachedAlbum>> {
     // Retry incomplete cache entries at most once per 7 days.
     let retry_before = (chrono::Local::now().naive_local() - chrono::Duration::days(7))
@@ -889,6 +860,27 @@ pub fn uncached_albums(conn: &Connection) -> Result<Vec<UncachedAlbum>> {
     rows.collect()
 }
 
+/// Return every distinct (artist, album) pair that has scrobbles, whether or
+/// not it is already cached. Empty album names are excluded.
+///
+/// Used by `enrich --force`, which re-fetches everything rather than only the
+/// rows that are missing data.
+pub fn all_scrobbled_albums(conn: &Connection) -> Result<Vec<UncachedAlbum>> {
+    let mut stmt = conn.prepare(
+        "SELECT DISTINCT s.artist, s.album
+         FROM scrobbles s
+         WHERE s.album != ''
+         ORDER BY s.artist, s.album",
+    )?;
+    let rows = stmt.query_map([], |row| {
+        Ok(UncachedAlbum {
+            artist: row.get(0)?,
+            album: row.get(1)?,
+        })
+    })?;
+    rows.collect()
+}
+
 /// Return all distinct albums with scrobbles for a specific artist.
 ///
 /// Matching is case-insensitive. Empty album names are excluded.
@@ -933,8 +925,10 @@ pub fn looks_like_url(label: &str) -> bool {
 /// Split a comma-separated genre string into cleaned labels.
 ///
 /// Domain-like garbage (see [`looks_like_url`]) is dropped here so that any
-/// such values already cached in the database never reach the report.
+/// such values already cached in the database never reach the report. This is
-fn split_genre_list(csv: &str) -> Vec<String> {
+/// the single place genre strings are parsed — the report renderer uses it too,
+/// so both paths agree on what counts as a genre.
+pub fn split_genre_list(csv: &str) -> Vec<String> {
     csv.split(',')
         .map(str::trim)
         .filter(|s| !s.is_empty())
@@ -1168,18 +1162,31 @@ pub fn album_cache_meta(
 /// the cover that was pinned under a different artist tag for the same album.
 pub fn artist_cover(conn: &Connection, artist: &str, period: &str) -> Option<String> {
     // The cover subquery first tries the direct (artist, album) cache entry;
-    // if that has no cover it falls back to any cache entry for the same
+    // if that has no cover it falls back to a cache entry that resolves to the
-    // album name that does have one (same MBID-merging logic as top_albums).
+    // same album group. Matching on album name alone would hand this artist the
-    let cover_expr = "COALESCE(
+    // cover of an unrelated album that merely shares a title, so the fallback
-                       c.cover_url,
+    // compares the same grouping key `top_albums` uses.
-                       (SELECT ac.cover_url FROM album_cache ac
+    let group_key = album_group_key_expr("s", "c");
-                        WHERE ac.album = s.album AND ac.cover_url IS NOT NULL
+    let cover_expr = format!(
-                        ORDER BY ac.artist LIMIT 1)
+        "COALESCE(
-                     )";
+             c.cover_url,
+             (SELECT ac.cover_url FROM album_cache ac
+              WHERE ac.album = s.album
+                AND ac.cover_url IS NOT NULL
+                AND COALESCE(ac.musicbrainz_id, ac.artist || '::' || ac.album) = {group_key}
+              ORDER BY ac.artist LIMIT 1)
+         )"
+    );
 
-    let time_filter = match period_range(period) {
+    // Bind the period rather than interpolating it: the values come from
-        Some((from, to)) => format!("AND s.scrobbled_at BETWEEN '{}' AND '{}'", from, to),
+    // `period_range` and so are well-formed, but a bound parameter cannot
-        None => String::new(),
+    // become part of the statement no matter what it contains.
+    let (time_filter, bounds) = match period_range(period) {
+        Some((from, to)) => (
+            "AND s.scrobbled_at BETWEEN ?2 AND ?3".to_string(),
+            vec![from, to],
+        ),
+        None => (String::new(), vec![]),
     };
 
     let sql = format!(
@@ -1196,8 +1203,11 @@ pub fn artist_cover(conn: &Connection, artist: &str, period: &str) -> Option<Str
          LIMIT 1"
     );
 
+    let mut params: Vec<&str> = vec![artist];
+    params.extend(bounds.iter().map(|s| s.as_str()));
+
     let mut stmt = conn.prepare(&sql).ok()?;
-    let mut rows = stmt.query(params![artist]).ok()?;
+    let mut rows = stmt.query(params_from_iter(params.iter())).ok()?;
     rows.next().ok()?.and_then(|row| row.get(0).ok())
 }
 
@@ -1616,6 +1626,228 @@ mod tests {
         assert_eq!(albums[0].artist, "Choir");
     }
 
+    #[test]
+    fn test_top_albums_dominant_source_ignores_same_titled_other_album() {
+        // Two unrelated artists with an identically-titled album. Neither is
+        // enriched, so they form separate groups — and each group's source dot
+        // must reflect only its own scrobbles. Matching on album name alone
+        // would let the busier artist's source win for both rows.
+        let conn = open_memory_db().unwrap();
+
+        insert_scrobble(
+            &conn,
+            &NewScrobble {
+                artist: "Artist A".into(),
+                album: "Greatest Hits".into(),
+                title: "A1".into(),
+                track_duration_secs: Some(200),
+                played_duration_secs: 200,
+                scrobbled_at: today_at("10:00:00"),
+                source: "MPD".into(),
+            },
+        )
+        .unwrap();
+
+        // Artist B has more scrobbles, from a different source.
+        for (i, hms) in ["11:00:00", "11:05:00", "11:10:00"].iter().enumerate() {
+            insert_scrobble(
+                &conn,
+                &NewScrobble {
+                    artist: "Artist B".into(),
+                    album: "Greatest Hits".into(),
+                    title: format!("B{}", i),
+                    track_duration_secs: Some(200),
+                    played_duration_secs: 200,
+                    scrobbled_at: today_at(hms),
+                    source: "Qobuz".into(),
+                },
+            )
+            .unwrap();
+        }
+
+        let albums = top_albums(&conn, "all", 10).unwrap();
+        assert_eq!(
+            albums.len(),
+            2,
+            "unenriched same-title albums stay separate"
+        );
+
+        let a = albums.iter().find(|x| x.artist == "Artist A").unwrap();
+        let b = albums.iter().find(|x| x.artist == "Artist B").unwrap();
+        assert_eq!(a.plays, 1);
+        assert_eq!(b.plays, 3);
+        assert_eq!(
+            a.dominant_source.as_deref(),
+            Some("MPD"),
+            "Artist A's source must not be taken from Artist B's rows"
+        );
+        assert_eq!(b.dominant_source.as_deref(), Some("Qobuz"));
+    }
+
+    #[test]
+    fn test_artist_cover_does_not_borrow_same_title_album() {
+        // "Artist B" has a cover for their own "Greatest Hits". "Artist A"'s
+        // identically-titled album is a different record and must not inherit
+        // it just because the names collide.
+        let conn = open_memory_db().unwrap();
+
+        for artist in ["Artist A", "Artist B"] {
+            insert_scrobble(
+                &conn,
+                &NewScrobble {
+                    artist: artist.into(),
+                    album: "Greatest Hits".into(),
+                    title: "Song".into(),
+                    track_duration_secs: Some(200),
+                    played_duration_secs: 200,
+                    scrobbled_at: today_at("10:00:00"),
+                    source: "test".into(),
+                },
+            )
+            .unwrap();
+        }
+
+        upsert_album_cache(
+            &conn,
+            &AlbumCacheEntry {
+                artist: "Artist B".into(),
+                album: "Greatest Hits".into(),
+                musicbrainz_id: None,
+                cover_url: Some("covers/b.jpg".into()),
+                genre: None,
+                fetched_at: today_at("12:00:00"),
+            },
+        )
+        .unwrap();
+
+        assert_eq!(
+            artist_cover(&conn, "Artist B", "all").as_deref(),
+            Some("covers/b.jpg")
+        );
+        assert_eq!(
+            artist_cover(&conn, "Artist A", "all"),
+            None,
+            "an unrelated album with the same title must not supply the cover"
+        );
+    }
+
+    #[test]
+    fn test_artist_cover_still_shares_within_one_release() {
+        // The flip side of the previous test: tracks from one classical
+        // release tagged with different performers must still share the cover
+        // pinned under whichever artist was enriched.
+        let conn = open_memory_db().unwrap();
+
+        let album = "Vivaldi: Gloria";
+        for artist in ["Choir", "Soloist"] {
+            insert_scrobble(
+                &conn,
+                &NewScrobble {
+                    artist: artist.into(),
+                    album: album.into(),
+                    title: "Movement".into(),
+                    track_duration_secs: Some(300),
+                    played_duration_secs: 300,
+                    scrobbled_at: today_at("10:00:00"),
+                    source: "test".into(),
+                },
+            )
+            .unwrap();
+        }
+
+        // Only "Choir" is enriched, and it carries an MBID.
+        upsert_album_cache(
+            &conn,
+            &AlbumCacheEntry {
+                artist: "Choir".into(),
+                album: album.into(),
+                musicbrainz_id: Some("mbid-vivaldi".into()),
+                cover_url: Some("covers/vivaldi.jpg".into()),
+                genre: Some("classical".into()),
+                fetched_at: today_at("12:00:00"),
+            },
+        )
+        .unwrap();
+
+        assert_eq!(
+            artist_cover(&conn, "Soloist", "all").as_deref(),
+            Some("covers/vivaldi.jpg"),
+            "a performer on the same release should inherit the pinned cover"
+        );
+    }
+
+    #[test]
+    fn test_all_scrobbled_albums_includes_fully_cached_rows() {
+        // `--force` re-fetches everything, so this must return albums that
+        // `uncached_albums` deliberately filters out.
+        let conn = open_memory_db().unwrap();
+        seed_db(&conn);
+
+        upsert_album_cache(
+            &conn,
+            &AlbumCacheEntry {
+                artist: "Deftones".into(),
+                album: "White Pony".into(),
+                musicbrainz_id: Some("mbid".into()),
+                cover_url: Some("covers/wp.jpg".into()),
+                genre: Some("alternative metal".into()),
+                fetched_at: today_at("12:00:00"),
+            },
+        )
+        .unwrap();
+
+        let uncached: Vec<String> = uncached_albums(&conn)
+            .unwrap()
+            .into_iter()
+            .map(|a| a.album)
+            .collect();
+        assert!(
+            !uncached.contains(&"White Pony".to_string()),
+            "a complete row is not 'uncached'"
+        );
+
+        let all: Vec<String> = all_scrobbled_albums(&conn)
+            .unwrap()
+            .into_iter()
+            .map(|a| a.album)
+            .collect();
+        assert!(all.contains(&"White Pony".to_string()));
+        assert!(all.contains(&"††† (Crosses)".to_string()));
+    }
+
+    #[test]
+    fn test_reset_retry_timestamps_all_artists() {
+        let conn = open_memory_db().unwrap();
+
+        for (artist, album) in [("Pink Floyd", "The Wall"), ("Deftones", "White Pony")] {
+            upsert_album_cache(
+                &conn,
+                &AlbumCacheEntry {
+                    artist: artist.into(),
+                    album: album.into(),
+                    musicbrainz_id: None,
+                    cover_url: None,
+                    genre: None,
+                    fetched_at: today_at("12:00:00"),
+                },
+            )
+            .unwrap();
+        }
+
+        // No artist filter: every cover-missing row is reset.
+        let updated = reset_retry_timestamps(&conn, RetryScope::Covers, None).unwrap();
+        assert_eq!(updated, 2);
+
+        let remaining: i64 = conn
+            .query_row(
+                "SELECT COUNT(*) FROM album_cache WHERE fetched_at IS NOT NULL",
+                [],
+                |r| r.get(0),
+            )
+            .unwrap();
+        assert_eq!(remaining, 0);
+    }
+
     #[test]
     fn test_top_tracks_tie_breaks_by_listen_time() {
         let conn = open_memory_db().unwrap();
@@ -2199,7 +2431,8 @@ mod tests {
         )
         .unwrap();
 
-        let updated = reset_missing_cover_timestamps_for_artist(&conn, "pink floyd").unwrap();
+        let updated =
+            reset_retry_timestamps(&conn, RetryScope::Covers, Some("pink floyd")).unwrap();
         assert_eq!(updated, 1);
 
         let pink_fetched: Option<String> = conn
@@ -2251,7 +2484,8 @@ mod tests {
         )
         .unwrap();
 
-        let updated = reset_missing_genre_timestamps_for_artist(&conn, "pink floyd").unwrap();
+        let updated =
+            reset_retry_timestamps(&conn, RetryScope::Genres, Some("pink floyd")).unwrap();
         assert_eq!(updated, 1);
 
         let pink_fetched: Option<String> = conn
@@ -2303,7 +2537,7 @@ mod tests {
         )
         .unwrap();
 
-        let updated = reset_incomplete_timestamps_for_artist(&conn, "pink floyd").unwrap();
+        let updated = reset_retry_timestamps(&conn, RetryScope::All, Some("pink floyd")).unwrap();
         assert_eq!(updated, 1);
 
         let pink_fetched: Option<String> = conn
@@ -2409,7 +2643,7 @@ mod tests {
         )
         .unwrap();
 
-        let updated = reset_missing_genre_timestamps_for_mpd(&conn).unwrap();
+        let updated = reset_retry_timestamps(&conn, RetryScope::MpdGenres, None).unwrap();
         assert_eq!(updated, 1);
 
         let deftones_fetched: Option<String> = conn
@@ -2499,7 +2733,8 @@ mod tests {
         )
         .unwrap();
 
-        let updated = reset_missing_genre_timestamps_for_mpd_artist(&conn, "pink floyd").unwrap();
+        let updated =
+            reset_retry_timestamps(&conn, RetryScope::MpdGenres, Some("pink floyd")).unwrap();
         assert_eq!(updated, 1);
 
         let pink_fetched: Option<String> = conn

src/enrich.rs

@@ -57,6 +57,7 @@ use image::codecs::jpeg::JpegEncoder;
 use reqwest::blocking::Client;
 use rusqlite::Connection;
 use serde::Deserialize;
+use std::fmt::Write as _;
 use std::fs;
 use std::io::Write;
 use std::path::{Path, PathBuf};
@@ -245,14 +246,23 @@ struct MbTag {
 ///
 /// Default: `~/.local/share/scrbblr/covers/`
 /// (respects `$XDG_DATA_HOME`)
-pub fn covers_dir() -> PathBuf {
+///
-    let data_dir = std::env::var("XDG_DATA_HOME").unwrap_or_else(|_| {
+/// Returns an error rather than panicking when neither `$XDG_DATA_HOME` nor
-        let home = std::env::var("HOME").expect("HOME not set");
+/// `$HOME` is set, or the directory cannot be created — cover caching is an
-        format!("{}/.local/share", home)
+/// optional convenience and should not take down a `watch` session that is
-    });
+/// otherwise scrobbling fine.
+pub fn covers_dir() -> Result<PathBuf, String> {
+    let data_dir = match std::env::var("XDG_DATA_HOME") {
+        Ok(d) if !d.is_empty() => d,
+        _ => {
+            let home = std::env::var("HOME")
+                .map_err(|_| "neither XDG_DATA_HOME nor HOME is set".to_string())?;
+            format!("{}/.local/share", home)
+        }
+    };
     let dir = PathBuf::from(format!("{}/scrbblr/covers", data_dir));
-    fs::create_dir_all(&dir).expect("Failed to create covers directory");
+    fs::create_dir_all(&dir).map_err(|e| format!("could not create {}: {}", dir.display(), e))?;
-    dir
+    Ok(dir)
 }
 
 // ---------------------------------------------------------------------------
@@ -288,7 +298,8 @@ fn search_releases(client: &Client, artist: &str, album: &str) -> Vec<String> {
         }
         let query = format!(
             "artist:\"{}\" AND release:\"{}\"",
-            artist_variants[0], variant
+            lucene_escape(&artist_variants[0]),
+            lucene_escape(variant)
         );
         let results = run_mb_search(client, &query);
         if !results.is_empty() {
@@ -307,7 +318,11 @@ fn search_releases(client: &Client, artist: &str, album: &str) -> Vec<String> {
                 eprintln!("  Retrying with alternate album name: \"{}\"...", variant);
             }
             thread::sleep(RATE_LIMIT_DELAY);
-            let query = format!("artist:\"{}\" AND release:\"{}\"", alt_artist, variant);
+            let query = format!(
+                "artist:\"{}\" AND release:\"{}\"",
+                lucene_escape(alt_artist),
+                lucene_escape(variant)
+            );
             let results = run_mb_search_with_min_score(client, &query, 60);
             if !results.is_empty() {
                 return results;
@@ -330,7 +345,8 @@ fn search_releases(client: &Client, artist: &str, album: &str) -> Vec<String> {
         client,
         &format!(
             "artist:\"{}\" AND recording:\"{}\"",
-            artist_variants[0], recording_query
+            lucene_escape(&artist_variants[0]),
+            lucene_escape(&recording_query)
         ),
         60,
     );
@@ -347,7 +363,8 @@ fn search_releases(client: &Client, artist: &str, album: &str) -> Vec<String> {
             client,
             &format!(
                 "artist:\"{}\" AND recording:\"{}\"",
-                artist_variants[0], shortest
+                lucene_escape(&artist_variants[0]),
+                lucene_escape(shortest)
             ),
             50,
         );
@@ -372,7 +389,7 @@ fn search_releases(client: &Client, artist: &str, album: &str) -> Vec<String> {
             eprintln!("  Retrying with title-only search (no artist constraint)...");
         }
         thread::sleep(RATE_LIMIT_DELAY);
-        let query = format!("release:\"{}\"", variant);
+        let query = format!("release:\"{}\"", lucene_escape(variant));
         let results = run_mb_search_with_min_score(client, &query, 85);
         if !results.is_empty() {
             return results;
@@ -480,6 +497,24 @@ fn normalise_spaces(s: &str) -> String {
     s.split_whitespace().collect::<Vec<_>>().join(" ")
 }
 
+/// Escape a value for use inside a quoted Lucene term in a MusicBrainz query.
+///
+/// Searches are built as `artist:"X" AND release:"Y"`. Without escaping, a
+/// title containing a double quote closes the term early and the rest is
+/// parsed as query syntax, so MusicBrainz rejects the query or silently
+/// matches nothing — precisely on the titles most likely to need the retry
+/// ladder. Inside a quoted phrase Lucene only treats `\` and `"` as special.
+fn lucene_escape(s: &str) -> String {
+    let mut out = String::with_capacity(s.len() + 4);
+    for c in s.chars() {
+        if c == '\\' || c == '"' {
+            out.push('\\');
+        }
+        out.push(c);
+    }
+    out
+}
+
 /// Trim common punctuation/noise from both title ends.
 fn trim_title_edges(s: &str) -> String {
     s.trim_matches(|c: char| {
@@ -647,6 +682,27 @@ fn fetch_release_details(client: &Client, mbid: &str) -> (Option<String>, Option
     (genre, rgid)
 }
 
+/// Fetch just the release-group ID for a release.
+///
+/// Used when the caller wants covers but not genres: the release-group is the
+/// Cover Art Archive's "any edition" fallback key, so it is still needed, but
+/// requesting it alone skips the follow-up genre lookup that
+/// [`fetch_release_details`] may perform.
+fn fetch_release_group_id(client: &Client, mbid: &str) -> Option<String> {
+    let url = format!(
+        "https://musicbrainz.org/ws/2/release/{}?inc=release-groups&fmt=json",
+        mbid
+    );
+
+    let response = client.get(&url).send().ok()?;
+    if !response.status().is_success() {
+        return None;
+    }
+
+    let details: MbReleaseDetails = response.json().ok()?;
+    details.release_group.map(|rg| rg.id)
+}
+
 /// Fetch release-group genres/tags and return a genre string if available.
 fn fetch_release_group_genre(client: &Client, release_group_id: &str) -> Option<String> {
     let url = format!(
@@ -836,12 +892,20 @@ pub fn resize_cover_bytes(bytes: &[u8]) -> Option<Vec<u8>> {
 // iTunes Search API — cover art fallback
 // ---------------------------------------------------------------------------
 
-/// Generate a stable filename stem for a cover sourced from iTunes.
+/// Generate a stable filename stem for a locally-cached cover.
 ///
-/// Uses the same FNV-1a 64-bit hash approach as the MPD cover extractor,
+/// Uses a 64-bit FNV-1a hash of `"artist\talbum"`, formatted as a 16-char hex
-/// keyed on `"artist\talbum"`, so the filename is deterministic across runs.
+/// string behind `prefix`. This gives a compact, deterministic filename that:
-/// Prefixed with `"itunes_"` to distinguish from MBID-based filenames.
+///
-fn itunes_cover_stem(artist: &str, album: &str) -> String {
+/// - Is stable across runs (same artist+album always maps to the same name).
+/// - Is visually distinct from MBID-based filenames (MBIDs contain hyphens).
+/// - Has negligible collision probability across a typical music library.
+///
+/// `prefix` names the source the cover came from, so the origin of a cached
+/// file is obvious on disk and MPD-local covers can be found again for
+/// revalidation. Example: `cover_stem("mpd", ..)` -> `mpd_a3f7c2b91e045d68`.
+pub fn cover_stem(prefix: &str, artist: &str, album: &str) -> String {
+    // FNV-1a 64-bit hash constants.
     const FNV_OFFSET: u64 = 14695981039346656037;
     const FNV_PRIME: u64 = 1099511628211;
 
@@ -851,7 +915,7 @@ fn itunes_cover_stem(artist: &str, album: &str) -> String {
         hash ^= byte as u64;
         hash = hash.wrapping_mul(FNV_PRIME);
     }
-    format!("itunes_{:016x}", hash)
+    format!("{}_{:016x}", prefix, hash)
 }
 
 /// Build a relaxed normalised key for fuzzy text matching.
@@ -987,14 +1051,17 @@ fn try_download(client: &Client, url: &str, dest: &Path) -> Option<String> {
 /// Simple URL encoding for query parameters.
 /// Encodes characters that are not unreserved per RFC 3986.
 fn urlencoded(s: &str) -> String {
-    let mut result = String::with_capacity(s.len() * 2);
+    let mut result = String::with_capacity(s.len() * 3);
-    for c in s.chars() {
+    // Encode straight from the UTF-8 bytes: a multi-byte character is encoded
-        match c {
+    // one byte at a time anyway, and going through `char` would allocate a
-            'A'..='Z' | 'a'..='z' | '0'..='9' | '-' | '_' | '.' | '~' => result.push(c),
+    // fresh `String` per character.
+    for &byte in s.as_bytes() {
+        match byte {
+            b'A'..=b'Z' | b'a'..=b'z' | b'0'..=b'9' | b'-' | b'_' | b'.' | b'~' => {
+                result.push(byte as char)
+            }
             _ => {
-                for byte in c.to_string().as_bytes() {
+                let _ = write!(result, "%{:02X}", byte);
-                    result.push_str(&format!("%{:02X}", byte));
-                }
             }
         }
     }
@@ -1076,20 +1143,34 @@ pub fn run_enrich_targeted_with_options(
 /// - `force` — if true, re-fetch all albums even if already cached
 /// - `quiet` — if true, suppress the "nothing to do" hint (used when called
 ///   from `report --html` where the user didn't explicitly ask for enrichment)
-/// Run online enrichment with explicit fetch/source options.
+/// - `options` — which metadata to fetch and which cover source to prefer
 pub fn run_enrich_with_options(
     conn: &Connection,
     force: bool,
     quiet: bool,
     options: OnlineEnrichOptions,
 ) {
-    if force {
+    // Force mode selects every scrobbled album instead of clearing the cache
-        conn.execute("DELETE FROM album_cache", [])
+    // first. `DELETE FROM album_cache` would also discard albums pinned by
-            .expect("Failed to clear album_cache");
+    // hand with `pin-album` (which exist precisely because automatic search
-        eprintln!("Cleared album cache (force mode).");
+    // cannot find them) and the paths of covers extracted from MPD, orphaning
+    // those files on disk — and anything the re-fetch then failed to find
+    // would be lost for good. Each row is instead overwritten in place by
+    // `upsert_album_cache`, so a failed lookup leaves the old value standing.
+    let albums = if force {
         eprintln!("Force mode enabled: refreshing existing cover files when re-fetching.");
-    }
+        db::all_scrobbled_albums(conn)
-    let albums = db::uncached_albums(conn).expect("Failed to query uncached albums");
+    } else {
+        db::uncached_albums(conn)
+    };
+
+    let albums = match albums {
+        Ok(v) => v,
+        Err(e) => {
+            eprintln!("[error] Failed to query albums to enrich: {}", e);
+            return;
+        }
+    };
     run_enrich_albums(conn, albums, quiet, force, options);
 }
 
@@ -1110,7 +1191,13 @@ fn run_enrich_albums(
     }
 
     let client = build_client();
-    let covers = covers_dir();
+    let covers = match covers_dir() {
+        Ok(d) => d,
+        Err(e) => {
+            eprintln!("[error] Cover cache unavailable: {}", e);
+            return;
+        }
+    };
     let fetch_covers = options.fetch_mode.wants_covers();
     let fetch_genres = options.fetch_mode.wants_genres();
     let use_itunes = fetch_covers && matches!(options.cover_source, CoverSourceMode::ItunesThenCaa);
@@ -1155,7 +1242,7 @@ fn run_enrich_albums(
             } else {
                 eprintln!("  Trying iTunes...");
                 fetch_itunes_cover_url(&client, &album.artist, &album.album).and_then(|art_url| {
-                    let stem = itunes_cover_stem(&album.artist, &album.album);
+                    let stem = cover_stem("itunes", &album.artist, &album.album);
                     let dest = covers.join(format!("{}.jpg", stem));
                     try_download(&client, &art_url, &dest)
                 })
@@ -1188,10 +1275,17 @@ fn run_enrich_albums(
         let primary_mbid = &candidates[0];
         eprintln!("  Found MBID: {}", primary_mbid);
 
-        // Step 2: Fetch genres/tags and release-group ID from the primary release.
+        // Step 2: Fetch genres/tags and release-group ID from the primary
-        let (fetched_genre, release_group_id) = if fetch_genres || fetch_covers {
+        // release. Skip the genre half of the work when only covers were asked
+        // for: the release-group ID is still needed as a CAA fallback key, but
+        // chasing genres would cost an extra request and rate-limit sleep per
+        // album for a value that is then thrown away.
+        let (fetched_genre, release_group_id) = if fetch_genres {
             thread::sleep(RATE_LIMIT_DELAY);
             fetch_release_details(&client, primary_mbid)
+        } else if fetch_covers {
+            thread::sleep(RATE_LIMIT_DELAY);
+            (None, fetch_release_group_id(&client, primary_mbid))
         } else {
             (None, None)
         };
@@ -1206,11 +1300,7 @@ fn run_enrich_albums(
         // Step 3: Try iTunes first — it's fast, has no rate limit, and has
         // good commercial coverage. Reuse the MBID-based filename so the file
         // is still associated with the correct release regardless of source.
-        let mut cover = if !fetch_covers {
+        let mut cover = if fetch_covers && use_itunes {
-            None
-        } else if !use_itunes {
-            None
-        } else {
             let itunes_cover = fetch_itunes_cover_url(&client, &album.artist, &album.album)
                 .and_then(|art_url| {
                     let dest = covers.join(format!("{}.jpg", primary_mbid));
@@ -1220,6 +1310,8 @@ fn run_enrich_albums(
                 eprintln!("  Cover downloaded (from iTunes).");
             }
             itunes_cover
+        } else {
+            None
         };
 
         // Step 4: If iTunes had no cover, fall back to the Cover Art Archive.
@@ -1322,7 +1414,13 @@ pub fn enrich_by_mbid(
     }
 
     let client = build_client();
-    let covers = covers_dir();
+    let covers = match covers_dir() {
+        Ok(d) => d,
+        Err(e) => {
+            eprintln!("[error] Cover cache unavailable: {}", e);
+            return;
+        }
+    };
 
     eprintln!("Fetching release details for MBID {}...", mbid);
     let (genre, release_group_id) = fetch_release_details(&client, mbid);
@@ -1378,6 +1476,17 @@ pub fn enrich_by_mbid(
         result
     };
 
+    // `upsert_album_cache` overwrites `genre` unconditionally. When the caller
+    // supplied `--cover-url` and MusicBrainz returned nothing, writing None
+    // would wipe a perfectly good genre that a previous run had found, so keep
+    // whatever is already cached in that case.
+    let genre = genre.or_else(|| {
+        db::album_cache_meta(conn, artist, album)
+            .ok()
+            .flatten()
+            .and_then(|m| m.genre)
+    });
+
     let entry = AlbumCacheEntry {
         artist: artist.to_string(),
         album: album.to_string(),
@@ -1429,6 +1538,37 @@ mod tests {
         assert_eq!(urlencoded("A-Z_a-z.0~9"), "A-Z_a-z.0~9");
     }
 
+    #[test]
+    fn test_lucene_escape_quotes_and_backslashes() {
+        assert_eq!(lucene_escape("Purple Rain"), "Purple Rain");
+        // A quote inside a title would otherwise close the quoted term early
+        // and turn the remainder into query syntax.
+        assert_eq!(lucene_escape("Say \"Hello\""), "Say \\\"Hello\\\"");
+        assert_eq!(lucene_escape("AC\\DC"), "AC\\\\DC");
+        // Colons are meaningful outside a quoted phrase but not inside one,
+        // so they are left alone.
+        assert_eq!(lucene_escape("Vivaldi: Gloria"), "Vivaldi: Gloria");
+    }
+
+    #[test]
+    fn test_cover_stem_prefix_and_stability() {
+        let a = cover_stem("mpd", "Bonobo", "Black Sands");
+        assert_eq!(a, cover_stem("mpd", "Bonobo", "Black Sands"));
+        assert!(a.starts_with("mpd_"), "stem = {}", a);
+        assert_eq!(a.len(), "mpd_".len() + 16);
+
+        // The prefix distinguishes sources without changing the hash.
+        let b = cover_stem("itunes", "Bonobo", "Black Sands");
+        assert_eq!(
+            a.trim_start_matches("mpd_"),
+            b.trim_start_matches("itunes_")
+        );
+
+        // Different albums must not collide.
+        assert_ne!(a, cover_stem("mpd", "Bonobo", "The North Borders"));
+        assert_ne!(a, cover_stem("mpd", "Deftones", "Black Sands"));
+    }
+
     #[test]
     fn test_strip_bracketed_segments() {
         assert_eq!(strip_bracketed_segments("If (Killing Eve)"), "If");

src/main.rs

@@ -103,6 +103,20 @@ enum EnrichRetry {
     MpdGenres,
 }
 
+impl EnrichRetry {
+    /// The database-level scope this retry mode selects, or `None` when the
+    /// normal cooldown should be left alone.
+    fn scope(self) -> Option<db::RetryScope> {
+        match self {
+            Self::None => None,
+            Self::Covers => Some(db::RetryScope::Covers),
+            Self::Genres => Some(db::RetryScope::Genres),
+            Self::All => Some(db::RetryScope::All),
+            Self::MpdGenres => Some(db::RetryScope::MpdGenres),
+        }
+    }
+}
+
 #[derive(Clone, Copy, Debug, Eq, PartialEq, ValueEnum)]
 #[value(rename_all = "kebab-case")]
 enum CoverSource {
@@ -318,13 +332,28 @@ enum Commands {
 /// Falls back to: ~/.local/share/scrbblr/scrobbles.db
 ///
 /// Creates the parent directory if it doesn't exist.
+/// Exits with a diagnostic instead of panicking when the location cannot be
+/// determined or created — an unset `$HOME` is a misconfiguration to report,
+/// not a bug to dump a backtrace for. Pass `--db-path` to bypass this.
 fn default_db_path() -> String {
-    let data_dir = std::env::var("XDG_DATA_HOME").unwrap_or_else(|_| {
+    let data_dir = match std::env::var("XDG_DATA_HOME") {
-        let home = std::env::var("HOME").expect("HOME not set");
+        Ok(d) if !d.is_empty() => d,
-        format!("{}/.local/share", home)
+        _ => match std::env::var("HOME") {
-    });
+            Ok(home) => format!("{}/.local/share", home),
+            Err(_) => {
+                eprintln!(
+                    "Cannot determine the database location: neither XDG_DATA_HOME \
+                     nor HOME is set. Pass --db-path explicitly."
+                );
+                std::process::exit(1);
+            }
+        },
+    };
     let dir = format!("{}/scrbblr", data_dir);
-    std::fs::create_dir_all(&dir).expect("Failed to create data directory");
+    if let Err(e) = std::fs::create_dir_all(&dir) {
+        eprintln!("Failed to create data directory {}: {}", dir, e);
+        std::process::exit(1);
+    }
     format!("{}/scrobbles.db", dir)
 }
 
@@ -383,13 +412,21 @@ fn run_watch(player: &str, use_mpris: bool, mpd_config: Option<mpd::MpdConfig>,
     let (tx, rx) = mpsc::channel::<watcher::Event>();
 
     // Track how many reader threads have sent Eof so we know when both
-    // playerctl processes have ended. Set to 2 when MPRIS is disabled so the
+    // playerctl processes have ended. Set to 0 when MPRIS is disabled so the
-    // main loop exits immediately on Ctrl+C (no processes to wait for).
+    // first Eof (from the Ctrl+C handler) ends the loop immediately — there
+    // are no reader threads to wait for in that case.
     let expected_eofs: usize = if use_mpris { 2 } else { 0 };
 
     let mut meta_handle = None;
     let mut status_handle = None;
 
+    // Live playerctl children. They are killed explicitly on shutdown: their
+    // reader threads block in `read_line` until stdout closes, so joining them
+    // without terminating the child first would hang. An interactive Ctrl+C
+    // happens to signal the whole process group, but a supervisor that signals
+    // only our PID (systemd, a wrapper script) would not.
+    let mut children: Vec<std::process::Child> = Vec::new();
+
     if use_mpris {
         // --- Spawn playerctl metadata follower ---
         // Outputs one tab-separated line per track change:
@@ -416,15 +453,27 @@ fn run_watch(player: &str, use_mpris: bool, mpd_config: Option<mpd::MpdConfig>,
             .spawn();
 
         match (metadata_proc, status_proc) {
-            (Ok(meta_proc), Ok(stat_proc)) => {
+            (Ok(mut meta_proc), Ok(mut stat_proc)) => {
+                // Take the pipes out of the children so the reader threads own
+                // the streams while we retain the handles needed to kill them.
+                let meta_stdout = meta_proc
+                    .stdout
+                    .take()
+                    .expect("No stdout for metadata process");
+                let stat_stdout = stat_proc
+                    .stdout
+                    .take()
+                    .expect("No stdout for status process");
+                children.push(meta_proc);
+                children.push(stat_proc);
+
                 // --- Metadata reader thread ---
                 // Reads lines from the metadata process stdout, parses them into
                 // `Event::Metadata` values, and sends them through the channel.
                 // Sends `Event::Eof` when the process exits (stdout closes).
                 let tx_meta = tx.clone();
                 meta_handle = Some(thread::spawn(move || {
-                    let stdout = meta_proc.stdout.expect("No stdout for metadata process");
+                    let reader = std::io::BufReader::new(meta_stdout);
-                    let reader = std::io::BufReader::new(stdout);
                     for line in reader.lines() {
                         match line {
                             Ok(l) => {
@@ -444,8 +493,7 @@ fn run_watch(player: &str, use_mpris: bool, mpd_config: Option<mpd::MpdConfig>,
                 // Same pattern as the metadata reader, but parses status lines.
                 let tx_status = tx.clone();
                 status_handle = Some(thread::spawn(move || {
-                    let stdout = stat_proc.stdout.expect("No stdout for status process");
+                    let reader = std::io::BufReader::new(stat_stdout);
-                    let reader = std::io::BufReader::new(stdout);
                     for line in reader.lines() {
                         match line {
                             Ok(l) => {
@@ -461,12 +509,19 @@ fn run_watch(player: &str, use_mpris: bool, mpd_config: Option<mpd::MpdConfig>,
                     let _ = tx_status.send(watcher::Event::Eof);
                 }));
             }
-            (meta_result, _) => {
+            (meta_result, status_result) => {
                 // At least one spawn failed — MPRIS watcher cannot run.
-                // Print the error and send synthetic Eofs so the main loop
+                // Print the error, reap whichever process did start so it is
+                // not left orphaned, and send synthetic Eofs so the main loop
                 // below exits cleanly; the MPD watcher (if any) continues.
-                if let Err(e) = meta_result {
+                for result in [meta_result, status_result] {
-                    eprintln!("[warn] Failed to spawn playerctl: {}", e);
+                    match result {
+                        Ok(mut child) => {
+                            let _ = child.kill();
+                            let _ = child.wait();
+                        }
+                        Err(e) => eprintln!("[warn] Failed to spawn playerctl: {}", e),
+                    }
                 }
                 eprintln!("[warn] MPRIS watcher will be inactive.");
                 let _ = tx.send(watcher::Event::Eof);
@@ -524,26 +579,53 @@ fn run_watch(player: &str, use_mpris: bool, mpd_config: Option<mpd::MpdConfig>,
         tracker.handle_event(watcher::Event::Eof);
     }
 
+    // Terminate the playerctl children. `playerctl --follow` never exits on
+    // its own, so its stdout would stay open and the reader threads below
+    // would block forever. Do this before any join.
+    for mut child in children {
+        let _ = child.kill();
+        let _ = child.wait();
+    }
+
     // Wait for the MPD watcher thread to finish its graceful shutdown.
     // It will notice `running == false` on the next idle timeout (≤ 500 ms).
     if let Some(handle) = mpd_handle {
         let _ = handle.join();
     }
 
-    eprintln!("Goodbye.");
+    // Wait for MPRIS reader threads to finish. Their pipes are now closed.
-
-    // Wait for MPRIS reader threads to finish.
     if let Some(h) = meta_handle {
         let _ = h.join();
     }
     if let Some(h) = status_handle {
         let _ = h.join();
     }
+
+    eprintln!("Goodbye.");
 }
 
-/// Round a value to the nearest multiple of 5.
+/// Round a non-negative value to the nearest multiple of 5.
+///
+/// Only ever called with a validated positive `--limit`, so integer division
+/// truncating toward zero is not a concern here.
 fn round_to_5(n: i64) -> i64 {
-    ((n + 2) / 5) * 5
+    ((n.max(0) + 2) / 5) * 5
+}
+
+/// Everything [`run_report`] needs, bundled so the call stays readable.
+struct ReportOptions<'a> {
+    period: &'a str,
+    json: bool,
+    html: bool,
+    /// Output directory for `--html`. `None` prints to stdout.
+    output: Option<&'a str>,
+    /// Top-N size for the per-period sections.
+    limit: i64,
+    /// Top-N size for the all-time sections.
+    all_time_limit: i64,
+    no_enrich: bool,
+    mpd_cfg: &'a mpd::MpdConfig,
+    db_path: &'a str,
 }
 
 // ---------------------------------------------------------------------------
@@ -560,17 +642,26 @@ fn round_to_5(n: i64) -> i64 {
 /// - **HTML** (`--html`): generates a multi-period report (Today / Week / Month /
 ///   All Time) with bar charts and album cover art.
 ///   With `--output <dir>`, writes `index.html` + `covers/` to a directory.
-fn run_report(
+fn run_report(opts: &ReportOptions<'_>) {
-    period: &str,
+    let ReportOptions {
-    json: bool,
+        period,
-    html: bool,
+        json,
-    output: Option<&str>,
+        html,
-    limit: i64,
+        output,
-    all_time_limit: i64,
+        limit,
-    no_enrich: bool,
+        all_time_limit,
-    mpd_cfg: &mpd::MpdConfig,
+        no_enrich,
-    db_path: &str,
+        mpd_cfg,
-) {
+        db_path,
+    } = *opts;
+
+    // Validate output-format flags before doing any work, so an obvious
+    // mistake fails immediately rather than after opening the database.
+    if json && html {
+        eprintln!("Please choose one output format: either --json or --html.");
+        std::process::exit(1);
+    }
+
     let conn = match db::open_db(db_path) {
         Ok(c) => c,
         Err(e) => {
@@ -590,11 +681,6 @@ fn run_report(
         std::process::exit(1);
     }
 
-    if json && html {
-        eprintln!("Please choose one output format: either --json or --html.");
-        std::process::exit(1);
-    }
-
     // Cover enrichment is only meaningful for HTML reports — terminal and JSON
     // output never displays cover art, so there is nothing to gain from
     // extracting or downloading covers for those modes.
@@ -755,24 +841,40 @@ fn main() {
             mpd_port,
             db_path,
         } => {
+            // Both limits are interpolated directly into SQL `LIMIT` clauses.
+            // They are plain integers so there is no injection risk, but SQLite
+            // treats a negative LIMIT as "no limit", which would silently dump
+            // the entire library instead of the requested top-N.
+            if limit < 1 {
+                eprintln!("--limit must be at least 1 (got {}).", limit);
+                std::process::exit(1);
+            }
+            if let Some(atl) = all_time_limit
+                && atl < 1
+            {
+                eprintln!("--all-time-limit must be at least 1 (got {}).", atl);
+                std::process::exit(1);
+            }
+
             let path = db_path.unwrap_or_else(default_db_path);
-            let atl =
+            let atl = all_time_limit
-                all_time_limit.unwrap_or_else(|| round_to_5((limit as f64 * 2.5).round() as i64));
+                .unwrap_or_else(|| round_to_5((limit as f64 * 2.5).round() as i64))
+                .max(1);
             let mpd_cfg = mpd::MpdConfig {
                 host: mpd_host,
                 port: mpd_port,
             };
-            run_report(
+            run_report(&ReportOptions {
-                &period,
+                period: &period,
                 json,
                 html,
-                output.as_deref(),
+                output: output.as_deref(),
                 limit,
-                atl,
+                all_time_limit: atl,
                 no_enrich,
-                &mpd_cfg,
+                mpd_cfg: &mpd_cfg,
-                &path,
+                db_path: &path,
-            );
+            });
         }
         Commands::Enrich {
             pipeline,
@@ -863,53 +965,12 @@ fn main() {
             if run_online {
                 // Optional retry selector: reset cooldown timestamps so the
                 // online stage revisits selected missing fields immediately.
-                if !matches!(retry, EnrichRetry::None) {
+                if let Some(scope) = retry.scope() {
-                    let reset = match retry {
+                    match db::reset_retry_timestamps(&conn, scope, artist.as_deref()) {
-                        EnrichRetry::None => Ok(0),
-                        EnrichRetry::Covers => {
-                            if let Some(ref artist_name) = artist {
-                                db::reset_missing_cover_timestamps_for_artist(&conn, artist_name)
-                            } else {
-                                db::reset_missing_cover_timestamps(&conn)
-                            }
-                        }
-                        EnrichRetry::Genres => {
-                            if let Some(ref artist_name) = artist {
-                                db::reset_missing_genre_timestamps_for_artist(&conn, artist_name)
-                            } else {
-                                db::reset_missing_genre_timestamps(&conn)
-                            }
-                        }
-                        EnrichRetry::All => {
-                            if let Some(ref artist_name) = artist {
-                                db::reset_incomplete_timestamps_for_artist(&conn, artist_name)
-                            } else {
-                                db::reset_incomplete_timestamps(&conn)
-                            }
-                        }
-                        EnrichRetry::MpdGenres => {
-                            if let Some(ref artist_name) = artist {
-                                db::reset_missing_genre_timestamps_for_mpd_artist(
-                                    &conn,
-                                    artist_name,
-                                )
-                            } else {
-                                db::reset_missing_genre_timestamps_for_mpd(&conn)
-                            }
-                        }
-                    };
-
-                    match reset {
                         Ok(n) => eprintln!(
                             "Reset cooldown for {} album(s) via --retry {}.",
                             n,
-                            match retry {
+                            scope.as_str()
-                                EnrichRetry::None => "none",
-                                EnrichRetry::Covers => "covers",
-                                EnrichRetry::Genres => "genres",
-                                EnrichRetry::All => "all",
-                                EnrichRetry::MpdGenres => "mpd-genres",
-                            }
                         ),
                         Err(e) => eprintln!("[warn] Failed to reset retry timestamps: {}", e),
                     }

src/mpd.rs

@@ -76,7 +76,7 @@ use std::os::unix::net::UnixStream;
 use std::path::Path;
 use std::sync::atomic::{AtomicBool, Ordering};
 use std::sync::{Arc, Mutex};
-use std::time::Duration;
+use std::time::{Duration, Instant};
 
 use rusqlite::Connection;
 
@@ -129,6 +129,27 @@ impl MpdConfig {
 // Low-level connection
 // ---------------------------------------------------------------------------
 
+/// How long to keep waiting for a command's response before giving up.
+///
+/// The socket's own read timeout is deliberately short (see [`READ_TIMEOUT`])
+/// so the idle loop can poll the shutdown flag. Every other exchange must wait
+/// for the real reply instead: abandoning a response half-read leaves unread
+/// bytes in the socket that the next command would parse as its own reply.
+const RESPONSE_TIMEOUT: Duration = Duration::from_secs(15);
+
+/// Socket read timeout. Short enough that the idle loop notices shutdown
+/// promptly; [`MpdConn`] transparently retries across it for command replies.
+const READ_TIMEOUT: Duration = Duration::from_millis(500);
+
+/// Return true when an I/O error is just the socket's read timeout firing,
+/// rather than a genuine failure.
+fn is_timeout(e: &io::Error) -> bool {
+    matches!(
+        e.kind(),
+        io::ErrorKind::WouldBlock | io::ErrorKind::TimedOut | io::ErrorKind::Interrupted
+    )
+}
+
 /// An open, authenticated connection to MPD.
 ///
 /// Holds a buffered reader for receiving response lines (and binary chunks)
@@ -136,12 +157,96 @@ impl MpdConfig {
 /// the same underlying socket.
 ///
 /// The read half has a short timeout (500 ms) so that the idle loop can
-/// periodically check the shutdown flag without blocking indefinitely.
+/// periodically check the shutdown flag without blocking indefinitely. All
+/// command/response reads go through [`MpdConn::read_line`] and
+/// [`MpdConn::read_exact`], which retry across that timeout so a slow reply is
+/// never mistaken for a finished one.
 struct MpdConn {
     /// Buffered reader wrapping the socket's read half.
     reader: BufReader<Box<dyn Read + Send>>,
     /// Write handle for sending commands.
     writer: Box<dyn Write + Send>,
+    /// Set once we abandon a read part-way through a response. The stream can
+    /// no longer be trusted to be positioned at a message boundary, so the
+    /// connection must be torn down and re-established rather than reused.
+    desynced: bool,
+}
+
+impl MpdConn {
+    /// Read one `\n`-terminated line, retrying while the socket's short read
+    /// timeout fires. Returns `Ok(None)` at EOF.
+    ///
+    /// Partial data accumulated before a timeout is retained and the read
+    /// resumes, so a reply split across timeouts is reassembled correctly.
+    /// Giving up marks the connection desynced.
+    fn read_line(&mut self) -> io::Result<Option<String>> {
+        let deadline = Instant::now() + RESPONSE_TIMEOUT;
+        let mut buf: Vec<u8> = Vec::new();
+        loop {
+            match self.reader.read_until(b'\n', &mut buf) {
+                Ok(_) => {
+                    if buf.last() == Some(&b'\n') {
+                        return Ok(Some(String::from_utf8_lossy(&buf).into_owned()));
+                    }
+                    // No delimiter and no error means EOF.
+                    if buf.is_empty() {
+                        return Ok(None);
+                    }
+                    return Ok(Some(String::from_utf8_lossy(&buf).into_owned()));
+                }
+                Err(ref e) if is_timeout(e) => {
+                    if Instant::now() >= deadline {
+                        self.desynced = true;
+                        return Err(io::Error::new(
+                            io::ErrorKind::TimedOut,
+                            "timed out waiting for MPD response line",
+                        ));
+                    }
+                }
+                Err(e) => {
+                    self.desynced = true;
+                    return Err(e);
+                }
+            }
+        }
+    }
+
+    /// Fill `buf` completely, retrying while the socket's short read timeout
+    /// fires.
+    ///
+    /// `Read::read_exact` cannot be used here: on error it does not report how
+    /// many bytes it consumed, so a timeout mid-chunk would silently swallow
+    /// part of the binary payload. This loop tracks progress itself.
+    fn read_exact(&mut self, buf: &mut [u8]) -> io::Result<()> {
+        let deadline = Instant::now() + RESPONSE_TIMEOUT;
+        let mut filled = 0;
+        while filled < buf.len() {
+            match self.reader.read(&mut buf[filled..]) {
+                Ok(0) => {
+                    self.desynced = true;
+                    return Err(io::Error::new(
+                        io::ErrorKind::UnexpectedEof,
+                        "MPD closed the connection mid-chunk",
+                    ));
+                }
+                Ok(n) => filled += n,
+                Err(ref e) if is_timeout(e) => {
+                    if Instant::now() >= deadline {
+                        self.desynced = true;
+                        return Err(io::Error::new(
+                            io::ErrorKind::TimedOut,
+                            "timed out reading MPD binary chunk",
+                        ));
+                    }
+                }
+                Err(e) => {
+                    self.desynced = true;
+                    return Err(e);
+                }
+            }
+        }
+        Ok(())
+    }
 }
 
 /// Open a TCP connection to MPD, set a read timeout, and consume the
@@ -154,7 +259,7 @@ fn connect_tcp(config: &MpdConfig) -> Result<MpdConn, String> {
     // A short read timeout lets the idle loop notice shutdown requests without
     // blocking for potentially minutes waiting for the next player event.
     stream
-        .set_read_timeout(Some(Duration::from_millis(500)))
+        .set_read_timeout(Some(READ_TIMEOUT))
         .map_err(|e| format!("Failed to set MPD read timeout: {}", e))?;
 
     // Clone the stream so we have independent read and write handles.
@@ -165,6 +270,7 @@ fn connect_tcp(config: &MpdConfig) -> Result<MpdConn, String> {
     let mut conn = MpdConn {
         reader: BufReader::new(Box::new(stream)),
         writer: Box::new(write_stream),
+        desynced: false,
     };
 
     // Consume the "OK MPD x.y.z" greeting before any command can be sent.
@@ -179,7 +285,7 @@ fn connect_unix(config: &MpdConfig) -> Result<MpdConn, String> {
         .map_err(|e| format!("Could not connect to MPD socket {}: {}", config.host, e))?;
 
     stream
-        .set_read_timeout(Some(Duration::from_millis(500)))
+        .set_read_timeout(Some(READ_TIMEOUT))
         .map_err(|e| format!("Failed to set MPD read timeout: {}", e))?;
 
     let write_stream = stream
@@ -189,6 +295,7 @@ fn connect_unix(config: &MpdConfig) -> Result<MpdConn, String> {
     let mut conn = MpdConn {
         reader: BufReader::new(Box::new(stream)),
         writer: Box::new(write_stream),
+        desynced: false,
     };
 
     consume_welcome(&mut conn)?;
@@ -214,10 +321,10 @@ fn connect(config: &MpdConfig) -> Result<MpdConn, String> {
 /// Returns `Err` if the connection is closed immediately or the greeting
 /// doesn't start with `"OK MPD"`, which would indicate a wrong host/port.
 fn consume_welcome(conn: &mut MpdConn) -> Result<(), String> {
-    let mut line = String::new();
+    let line = conn
-    conn.reader
+        .read_line()
-        .read_line(&mut line)
+        .map_err(|e| format!("Failed to read MPD welcome: {}", e))?
-        .map_err(|e| format!("Failed to read MPD welcome: {}", e))?;
+        .ok_or_else(|| "MPD closed the connection before sending a greeting".to_string())?;
 
     if !line.starts_with("OK MPD") {
         return Err(format!("Unexpected MPD greeting: {:?}", line));
@@ -239,26 +346,37 @@ fn send_command(conn: &mut MpdConn, cmd: &str) -> io::Result<()> {
 /// Read a `key: value` response from MPD until `OK\n` is seen.
 ///
 /// Returns a `HashMap<key, value>` of all lines parsed before `OK`. An `ACK`
-/// line is treated as an error and returns an empty map (the caller ignores
+/// line is treated as an error and returns what was parsed so far (usually
-/// the result for most queries).
+/// nothing) — the `ACK` itself terminates the response, so the stream stays in
+/// sync.
+///
+/// Returns `None` when the response could not be read to completion (EOF or a
+/// read failure). In that case the connection is left desynced and the caller
+/// must reconnect rather than issue another command: the unread remainder of
+/// this reply would otherwise be parsed as the next command's response.
 ///
 /// Lines not matching `"key: value"` are silently skipped — MPD sometimes
 /// includes continuation data that doesn't follow this pattern.
-fn read_response(conn: &mut MpdConn) -> HashMap<String, String> {
+fn read_response(conn: &mut MpdConn) -> Option<HashMap<String, String>> {
     let mut map = HashMap::new();
     loop {
-        let mut line = String::new();
+        let raw = match conn.read_line() {
-        match conn.reader.read_line(&mut line) {
+            Ok(Some(l)) => l,
-            Ok(0) => break, // EOF — connection closed
+            // EOF part-way through a response is a lost connection, not an
-            Ok(_) => {}
+            // empty result.
-            Err(_) => break,
+            Ok(None) => {
-        }
+                conn.desynced = true;
-        let line = line.trim_end_matches('\n').trim_end_matches('\r');
+                return None;
+            }
+            Err(_) => return None,
+        };
+        let line = raw.trim_end_matches('\n').trim_end_matches('\r');
         if line == "OK" {
             break;
         }
         if line.starts_with("ACK") {
-            // Protocol error — log it and return what we have (usually nothing).
+            // Protocol-level error. The ACK line ends the response, so the
+            // stream remains usable.
             break;
         }
         // Split on the first ": " only, in case a value contains ": ".
@@ -268,7 +386,7 @@ fn read_response(conn: &mut MpdConn) -> HashMap<String, String> {
             map.insert(key, val);
         }
     }
-    map
+    Some(map)
 }
 
 /// Read the `readpicture` response header lines until `binary: N` is seen.
@@ -279,9 +397,8 @@ fn read_response(conn: &mut MpdConn) -> HashMap<String, String> {
 fn read_picture_header(conn: &mut MpdConn) -> Option<(u64, u64)> {
     let mut total_size: u64 = 0;
     loop {
-        let mut line = String::new();
+        let raw = conn.read_line().ok()??;
-        conn.reader.read_line(&mut line).ok()?;
+        let line = raw.trim_end_matches('\n').trim_end_matches('\r');
-        let line = line.trim_end_matches('\n').trim_end_matches('\r');
 
         if line == "OK" {
             // Reached OK without a binary field — unexpected, treat as no data.
@@ -303,12 +420,13 @@ fn read_picture_header(conn: &mut MpdConn) -> Option<(u64, u64)> {
 /// Read exactly `n` raw bytes from MPD's response stream.
 ///
 /// Used after parsing the `binary: N` header to extract the image chunk.
-/// `BufReader::read_exact` correctly drains buffered data first and then
+/// [`MpdConn::read_exact`] drains buffered data first and then reads the
-/// reads the remainder from the socket, so mixing text line reads and binary
+/// remainder from the socket, so mixing text line reads and binary chunk reads
-/// chunk reads on the same `BufReader` is safe.
+/// on the same `BufReader` is safe, and it retries across the socket's short
+/// read timeout so a chunk that arrives slowly is not abandoned half-read.
 fn read_exact_bytes(conn: &mut MpdConn, n: u64) -> Option<Vec<u8>> {
     let mut buf = vec![0u8; n as usize];
-    conn.reader.read_exact(&mut buf).ok()?;
+    conn.read_exact(&mut buf).ok()?;
     Some(buf)
 }
 
@@ -322,10 +440,9 @@ fn read_exact_bytes(conn: &mut MpdConn, n: u64) -> Option<Vec<u8>> {
 /// EOF, protocol errors, or I/O errors.
 fn read_chunk_terminator(conn: &mut MpdConn) -> Option<()> {
     loop {
-        let mut line = String::new();
+        let raw = conn.read_line().ok()??;
-        conn.reader.read_line(&mut line).ok()?;
 
-        let line = line.trim_end_matches('\n').trim_end_matches('\r');
+        let line = raw.trim_end_matches('\n').trim_end_matches('\r');
         if line.is_empty() {
             // Expected: this is the newline that follows the raw binary bytes.
             continue;
@@ -334,10 +451,14 @@ fn read_chunk_terminator(conn: &mut MpdConn) -> Option<()> {
             return Some(());
         }
         if line.starts_with("ACK") {
+            // An ACK terminates the response, so the stream is still aligned.
             return None;
         }
 
-        // Any other non-empty line here means the stream got out of sync.
+        // Any other non-empty line here means the stream got out of sync:
+        // we cannot tell how much of the binary payload we mis-parsed, so the
+        // connection must not be reused.
+        conn.desynced = true;
         return None;
     }
 }
@@ -375,7 +496,7 @@ struct MpdStatus {
 /// Returns `None` if MPD is stopped (no current song) or if the query fails.
 fn query_currentsong(conn: &mut MpdConn) -> Option<MpdSong> {
     send_command(conn, "currentsong").ok()?;
-    let map = read_response(conn);
+    let map = read_response(conn)?;
 
     // If MPD is stopped, currentsong returns an empty response.
     if map.is_empty() {
@@ -408,9 +529,13 @@ fn query_status(conn: &mut MpdConn) -> MpdStatus {
     if send_command(conn, "status").is_err() {
         return MpdStatus::default();
     }
-    let map = read_response(conn);
+    match read_response(conn) {
-    MpdStatus {
+        Some(map) => MpdStatus {
-        state: map.get("state").cloned().unwrap_or_default(),
+            state: map.get("state").cloned().unwrap_or_default(),
+        },
+        // Unreadable response: the connection is already marked desynced and
+        // the event loop will tear it down. Report an empty state meanwhile.
+        None => MpdStatus::default(),
     }
 }
 
@@ -436,11 +561,17 @@ fn search_song_for_album(conn: &mut MpdConn, artist: &str, album: &str) -> Optio
         escape_mpd_string(artist),
         escape_mpd_string(album)
     );
-    if send_command(conn, &cmd).is_ok() {
+    if send_command(conn, &cmd).is_ok()
-        let map = read_response(conn);
+        && let Some(map) = read_response(conn)
-        if let Some(file) = map.get("file") {
+        && let Some(file) = map.get("file")
-            return Some(file.clone());
+    {
-        }
+        return Some(file.clone());
+    }
+
+    // A failed read leaves the stream mid-response; issuing the fallback
+    // search on the same connection would misparse it.
+    if conn.desynced {
+        return None;
     }
 
     // Fall back to the Artist tag (handles single-artist albums and tracks
@@ -450,11 +581,11 @@ fn search_song_for_album(conn: &mut MpdConn, artist: &str, album: &str) -> Optio
         escape_mpd_string(artist),
         escape_mpd_string(album)
     );
-    if send_command(conn, &cmd).is_ok() {
+    if send_command(conn, &cmd).is_ok()
-        let map = read_response(conn);
+        && let Some(map) = read_response(conn)
-        if let Some(file) = map.get("file") {
+        && let Some(file) = map.get("file")
-            return Some(file.clone());
+    {
-        }
+        return Some(file.clone());
     }
 
     None
@@ -480,12 +611,17 @@ fn escape_mpd_string(s: &str) -> String {
 ///
 /// Returns `None` if the file has no embedded picture, if the file is not
 /// found in MPD's database, or if a protocol error occurs.
+///
+/// The transfer is bounded on both size and iteration count. MPD is normally
+/// trustworthy, but the loop is driven entirely by numbers MPD supplies, so a
+/// buggy or hostile server could otherwise keep it allocating forever — see
+/// [`MAX_PICTURE_BYTES`] and [`MAX_PICTURE_CHUNKS`].
 fn read_picture(conn: &mut MpdConn, file_uri: &str) -> Option<Vec<u8>> {
     let mut all_bytes: Vec<u8> = Vec::new();
     let mut offset: u64 = 0;
     let mut expected_total: Option<u64> = None;
 
-    loop {
+    for _ in 0..MAX_PICTURE_CHUNKS {
         // Request the next chunk starting at `offset`.
         let cmd = format!("readpicture \"{}\" {}", escape_mpd_string(file_uri), offset);
         send_command(conn, &cmd).ok()?;
@@ -493,6 +629,11 @@ fn read_picture(conn: &mut MpdConn, file_uri: &str) -> Option<Vec<u8>> {
         // Parse the `size`, `type`, and `binary` header fields.
         let (total_size, chunk_size) = read_picture_header(conn)?;
 
+        if total_size > MAX_PICTURE_BYTES || chunk_size > MAX_PICTURE_BYTES {
+            eprintln!("  [warn] Embedded cover art is implausibly large; skipping.");
+            return None;
+        }
+
         if total_size > 0 {
             match expected_total {
                 None => expected_total = Some(total_size),
@@ -505,7 +646,7 @@ fn read_picture(conn: &mut MpdConn, file_uri: &str) -> Option<Vec<u8>> {
         if chunk_size == 0 {
             // If we have already accumulated bytes, we're done; otherwise there
             // was no picture at all.
-            break;
+            return finish_picture(all_bytes, expected_total);
         }
 
         // Read the raw binary chunk from the stream.
@@ -517,13 +658,41 @@ fn read_picture(conn: &mut MpdConn, file_uri: &str) -> Option<Vec<u8>> {
 
         offset += chunk_size;
 
-        // Stop when we've read the complete image.
+        if all_bytes.len() as u64 > MAX_PICTURE_BYTES {
+            eprintln!("  [warn] Embedded cover art exceeded the size limit; skipping.");
+            return None;
+        }
+
+        // Stop when we've read the complete image. A server that never
+        // declares a total size falls through to the chunk-count guard rather
+        // than looping forever.
         if total_size > 0 && offset >= total_size {
-            break;
+            return finish_picture(all_bytes, expected_total);
         }
     }
 
-    // If MPD declared a total byte size, ensure we received all of it.
+    // Ran out of iterations without MPD ever signalling completion.
+    None
+}
+
+/// Largest embedded picture we are willing to buffer, in bytes.
+///
+/// Cover art is a JPEG or PNG of a few megabytes at most; the cap only exists
+/// so a bogus `size:`/`binary:` header cannot drive an unbounded allocation.
+const MAX_PICTURE_BYTES: u64 = 64 * 1024 * 1024;
+
+/// Upper bound on `readpicture` round-trips for a single image.
+///
+/// MPD's default chunk size is 8 KiB, so this allows well over
+/// [`MAX_PICTURE_BYTES`] worth of chunks while still terminating if a server
+/// reports no total size and keeps returning data.
+const MAX_PICTURE_CHUNKS: usize = 16_384;
+
+/// Validate a completed picture transfer and hand back the bytes.
+///
+/// Returns `None` when MPD declared a total size we did not fully receive, or
+/// when the file simply had no embedded art.
+fn finish_picture(all_bytes: Vec<u8>, expected_total: Option<u64>) -> Option<Vec<u8>> {
     if let Some(total) = expected_total
         && all_bytes.len() as u64 != total
     {
@@ -543,27 +712,12 @@ fn read_picture(conn: &mut MpdConn, file_uri: &str) -> Option<Vec<u8>> {
 
 /// Generate a stable filename stem for a locally-extracted MPD cover.
 ///
-/// Uses a 64-bit FNV-1a hash of `"artist\talbum"`, formatted as a 16-char
+/// Thin wrapper over [`enrich::cover_stem`] that pins the `mpd_` prefix, which
-/// hex string prefixed with `"mpd_"`. This gives a compact, deterministic
+/// `repair-mpd-covers` relies on to find these files again.
-/// filename that:
-///
-/// - Is stable across runs (same artist+album always maps to the same name).
-/// - Is visually distinct from MBID-based filenames (MBIDs contain hyphens).
-/// - Has negligible collision probability across a typical music library.
 ///
 /// Example: `mpd_a3f7c2b91e045d68.jpg`
 fn album_cover_stem(artist: &str, album: &str) -> String {
-    // FNV-1a 64-bit hash constants.
+    enrich::cover_stem("mpd", artist, album)
-    const FNV_OFFSET: u64 = 14695981039346656037;
-    const FNV_PRIME: u64 = 1099511628211;
-
-    let input = format!("{}\t{}", artist, album);
-    let mut hash = FNV_OFFSET;
-    for byte in input.bytes() {
-        hash ^= byte as u64;
-        hash = hash.wrapping_mul(FNV_PRIME);
-    }
-    format!("mpd_{:016x}", hash)
 }
 
 // ---------------------------------------------------------------------------
@@ -748,6 +902,15 @@ fn run_event_loop(
         // Query MPD's current state now that we know something changed.
         let song = query_currentsong(mpd_conn);
         let status = query_status(mpd_conn);
+
+        // If either query failed part-way, the stream is no longer positioned
+        // at a message boundary. Reconnect instead of issuing more commands
+        // against a connection whose replies we can no longer line up.
+        if mpd_conn.desynced {
+            eprintln!("[mpd] Lost sync with MPD protocol stream.");
+            return false;
+        }
+
         let new = MpdPlayerState::from_queries(song, status);
 
         // Determine what changed and dispatch the appropriate events.
@@ -757,6 +920,11 @@ fn run_event_loop(
         // cover for the new album if we don't already have one cached.
         if new.file != prev.file && !new.file.is_empty() && !new.album.is_empty() {
             try_extract_cover(mpd_conn, db_conn, &new);
+            if mpd_conn.desynced {
+                eprintln!("[mpd] Lost sync while reading embedded cover art.");
+                *prev = new;
+                return false;
+            }
         }
 
         *prev = new;
@@ -785,12 +953,24 @@ enum IdleResult {
 /// of whether a `changed: player` line appeared. In both cases the caller
 /// re-queries MPD state, so the distinction doesn't matter.
 fn wait_for_idle_response(conn: &mut MpdConn, running: &Arc<AtomicBool>) -> IdleResult {
+    // This is the one place that reads the socket directly rather than going
+    // through `MpdConn::read_line`: here a timeout is the *point*, since it is
+    // what lets us poll the shutdown flag while MPD is silent.
+    //
+    // `line` lives outside the loop so that a line split across two timeouts
+    // is reassembled rather than truncated. `read_until` appends what it got
+    // before erroring and leaves it in the buffer, so resuming is correct.
+    let mut line: Vec<u8> = Vec::new();
     loop {
-        let mut line = String::new();
+        match conn.reader.read_until(b'\n', &mut line) {
-        match conn.reader.read_line(&mut line) {
             Ok(0) => return IdleResult::ConnectionLost, // EOF
             Ok(_) => {
-                let trimmed = line.trim();
+                if line.last() != Some(&b'\n') {
+                    // Delimiter missing without an error means EOF mid-line.
+                    return IdleResult::ConnectionLost;
+                }
+                let text = String::from_utf8_lossy(&line);
+                let trimmed = text.trim();
                 if trimmed == "OK" {
                     // End of idle response — re-query state regardless of whether
                     // "changed: player" appeared. With `idle player` the line
@@ -800,10 +980,9 @@ fn wait_for_idle_response(conn: &mut MpdConn, running: &Arc<AtomicBool>) -> Idle
                 }
                 // "changed: player" and any other key/value lines are consumed
                 // and discarded here — we query currentsong/status ourselves.
+                line.clear();
             }
-            Err(ref e)
+            Err(ref e) if is_timeout(e) => {
-                if e.kind() == io::ErrorKind::WouldBlock || e.kind() == io::ErrorKind::TimedOut =>
-            {
                 // Read timeout — check if we should shut down, then keep waiting.
                 if !running.load(Ordering::SeqCst) {
                     return IdleResult::Shutdown;
@@ -938,8 +1117,13 @@ fn try_extract_cover(
         None => return,
     };
 
-    // Save the processed image to the covers cache directory.
+    // Save the processed image to the covers cache directory. Caching a cover
-    let covers = enrich::covers_dir();
+    // is opportunistic, so an unusable cache directory just means no cover —
+    // it must not interrupt the watch loop.
+    let covers = match enrich::covers_dir() {
+        Ok(d) => d,
+        Err(_) => return,
+    };
     let stem = album_cover_stem(&state.artist, &state.album);
     let dest = covers.join(format!("{}.jpg", stem));
     if std::fs::write(&dest, &processed).is_err() {
@@ -1069,6 +1253,17 @@ pub fn run_mpd_cover_revalidate(config: &MpdConfig, conn: &Connection, artist: O
         return;
     }
 
+    // Resolve the covers directory once, up front: it is the boundary the
+    // per-album path check below is measured against, so failing to resolve it
+    // must stop the run rather than weaken the check.
+    let covers_root = match enrich::covers_dir() {
+        Ok(d) => d,
+        Err(e) => {
+            eprintln!("[error] Cover cache unavailable: {}", e);
+            return;
+        }
+    };
+
     let mut mpd_conn = match connect(config) {
         Ok(c) => c,
         Err(e) => {
@@ -1092,6 +1287,12 @@ pub fn run_mpd_cover_revalidate(config: &MpdConfig, conn: &Connection, artist: O
             album.album
         );
 
+        if mpd_conn.desynced {
+            eprintln!("  [warn] Lost sync with MPD; aborting revalidation.");
+            skipped += albums.len() - i;
+            break;
+        }
+
         let file_uri = match search_song_for_album(&mut mpd_conn, &album.artist, &album.album) {
             Some(f) => f,
             None => {
@@ -1124,7 +1325,7 @@ pub fn run_mpd_cover_revalidate(config: &MpdConfig, conn: &Connection, artist: O
         // Sanity-check: only write to paths inside the scrbblr covers
         // directory. Rejects any DB row whose cover_url was tampered with to
         // point outside our own data directory.
-        if !dest.starts_with(enrich::covers_dir()) {
+        if !dest.starts_with(&covers_root) {
             eprintln!(
                 "  [warn] cover_url '{}' is outside the covers directory; skipping.",
                 album.cover_url
@@ -1198,7 +1399,13 @@ fn run_mpd_cover_enrich_albums(
         }
     };
 
-    let covers = enrich::covers_dir();
+    let covers = match enrich::covers_dir() {
+        Ok(d) => d,
+        Err(e) => {
+            eprintln!("[error] Cover cache unavailable: {}", e);
+            return;
+        }
+    };
     eprintln!(
         "Extracting covers from MPD for {} album(s)...",
         albums.len()
@@ -1216,6 +1423,12 @@ fn run_mpd_cover_enrich_albums(
             album.album
         );
 
+        if mpd_conn.desynced {
+            eprintln!("  [warn] Lost sync with MPD; aborting cover extraction.");
+            skipped += albums.len() - i;
+            break;
+        }
+
         // Step 1: Ask MPD for any file from this album so we have a path to
         // hand to readpicture.
         let file_uri = match search_song_for_album(&mut mpd_conn, &album.artist, &album.album) {
@@ -1307,6 +1520,7 @@ mod tests {
         MpdConn {
             reader: BufReader::new(Box::new(Cursor::new(input.to_vec()))),
             writer: Box::new(NullWriter),
+            desynced: false,
         }
     }
 
@@ -1448,6 +1662,62 @@ mod tests {
         assert!(read_picture(&mut conn, "music/test.flac").is_none());
     }
 
+    #[test]
+    fn test_read_picture_terminates_when_total_size_never_declared() {
+        // A server that keeps returning chunks but never declares a total size
+        // must not loop forever. Two chunks then EOF: the header read fails and
+        // the transfer is abandoned rather than spinning.
+        let mut conn = fake_conn(b"size: 0\nbinary: 5\nhello\nOK\nsize: 0\nbinary: 5\nworld\nOK\n");
+        assert!(read_picture(&mut conn, "music/test.flac").is_none());
+    }
+
+    #[test]
+    fn test_read_picture_rejects_implausible_size() {
+        // A bogus `size:` header must not drive a multi-gigabyte allocation.
+        let mut conn = fake_conn(b"size: 999999999999\nbinary: 4\ndata\nOK\n");
+        assert!(read_picture(&mut conn, "music/test.flac").is_none());
+    }
+
+    #[test]
+    fn test_read_chunk_terminator_flags_desync_on_unexpected_line() {
+        // A line that is neither blank nor OK means we mis-parsed the binary
+        // payload boundary. The connection must be marked unusable so the
+        // caller reconnects rather than reading the rest as a fresh response.
+        let mut conn = fake_conn(b"size: 99\n");
+        assert!(read_chunk_terminator(&mut conn).is_none());
+        assert!(
+            conn.desynced,
+            "unexpected line must mark the stream desynced"
+        );
+    }
+
+    #[test]
+    fn test_read_chunk_terminator_ack_keeps_stream_usable() {
+        // An ACK terminates the response cleanly, so the connection is fine.
+        let mut conn = fake_conn(b"ACK [50@0] {readpicture} No file exists\n");
+        assert!(read_chunk_terminator(&mut conn).is_none());
+        assert!(!conn.desynced, "an ACK is a normal response terminator");
+    }
+
+    #[test]
+    fn test_read_response_reports_eof_as_failure() {
+        // A response that ends without its terminating OK is a lost
+        // connection, not an empty result set. Returning an empty map here
+        // would make `currentsong` look like "MPD is stopped".
+        let mut conn = fake_conn(b"file: a.flac\nTitle: Song\n");
+        assert!(read_response(&mut conn).is_none());
+        assert!(conn.desynced);
+    }
+
+    #[test]
+    fn test_read_response_parses_complete_reply() {
+        let mut conn = fake_conn(b"file: a.flac\nTitle: Song\nOK\n");
+        let map = read_response(&mut conn).expect("complete response");
+        assert_eq!(map.get("file").map(String::as_str), Some("a.flac"));
+        assert_eq!(map.get("Title").map(String::as_str), Some("Song"));
+        assert!(!conn.desynced);
+    }
+
     // -----------------------------------------------------------------------
     // dispatch_events
     // -----------------------------------------------------------------------

src/report.rs

@@ -370,10 +370,14 @@ fn print_box_table(
                 let shown = truncate_cell(cell, widths[i]);
                 print!(" {:<width$} ", shown, width = widths[i]);
                 if bar_max.is_some() && i == ncols - 1 {
-                    // Render bar chart in the extra column.
+                    // Render bar chart in the extra column. `filled` is
+                    // clamped rather than trusted: a caller that passes a
+                    // `bar_max` below the largest row would otherwise make
+                    // `bar_width - filled` underflow and panic.
                     let val: i64 = cell.parse().unwrap_or(0);
                     let max = bar_max.unwrap_or(1).max(1);
-                    let filled = ((val as f64 / max as f64) * bar_width as f64).round() as usize;
+                    let ratio = (val as f64 / max as f64).clamp(0.0, 1.0);
+                    let filled = ((ratio * bar_width as f64).round() as usize).min(bar_width);
                     let empty = bar_width - filled;
                     print!("│ {}{} ", "█".repeat(filled), "░".repeat(empty));
                 }
@@ -627,7 +631,7 @@ pub fn print_terminal_report(data: &ReportData) {
     // --- Top Artists ---
     if !data.top_artists.is_empty() {
         println!("\n  Top Artists");
-        let max_plays = data.top_artists.first().map(|a| a.plays).unwrap_or(1);
+        let max_plays = data.top_artists.iter().map(|a| a.plays).max().unwrap_or(1);
         let rows: Vec<Vec<String>> = data
             .top_artists
             .iter()
@@ -652,7 +656,7 @@ pub fn print_terminal_report(data: &ReportData) {
     // --- Top Albums ---
     if !data.top_albums.is_empty() {
         println!("\n  Top Albums");
-        let max_plays = data.top_albums.first().map(|a| a.plays).unwrap_or(1);
+        let max_plays = data.top_albums.iter().map(|a| a.plays).max().unwrap_or(1);
         let rows: Vec<Vec<String>> = data
             .top_albums
             .iter()
@@ -678,7 +682,9 @@ pub fn print_terminal_report(data: &ReportData) {
     // --- Top Genres ---
     if !data.top_genres.is_empty() {
         println!("\n  Top Genres");
-        let max_plays = data.top_genres.first().map(|g| g.plays).unwrap_or(1);
+        // Not `first()`: `top_genres` ranks multi-word genres above broad
+        // single-word ones, so the top row is not necessarily the busiest.
+        let max_plays = data.top_genres.iter().map(|g| g.plays).max().unwrap_or(1);
         let rows: Vec<Vec<String>> = data
             .top_genres
             .iter()
@@ -703,7 +709,7 @@ pub fn print_terminal_report(data: &ReportData) {
     // --- Top Tracks ---
     if !data.top_tracks.is_empty() {
         println!("\n  Top Tracks");
-        let max_plays = data.top_tracks.first().map(|t| t.plays).unwrap_or(1);
+        let max_plays = data.top_tracks.iter().map(|t| t.plays).max().unwrap_or(1);
         let rows: Vec<Vec<String>> = data
             .top_tracks
             .iter()
@@ -976,7 +982,14 @@ pub fn render_html_report(conn: &Connection, limit: i64, all_time_limit: i64) ->
         // We intentionally round the cover count up to a full desktop row so
         // the grid doesn't end with a partially filled row when the user picks
         // a limit like 20 and desktop layout has 6 columns.
-        let cover_limit = album_cover_grid_limit(limit);
+        //
+        // Use this period's own limit, not the base one: all-time sections
+        // render a longer top-albums table, and a grid sized from `limit`
+        // would show fewer covers than the table directly beneath it. It also
+        // has to match what `albums_needed_for_report` pre-enriched, which is
+        // likewise computed per period.
+        let period_limit = if is_all_time { all_time_limit } else { limit };
+        let cover_limit = album_cover_grid_limit(period_limit);
         let cover_albums = db::top_albums(conn, &data.period.name, cover_limit)
             .unwrap_or_else(|_| data.top_albums.clone());
         if !cover_albums.is_empty() {
@@ -1073,22 +1086,7 @@ pub fn render_html_report(conn: &Connection, limit: i64, all_time_limit: i64) ->
                     .flatten()
                     .and_then(|m| m.cover_url);
                 let cover = resolve_cover(cover_url, &mut cover_files);
-                let source_cell = if let Some(src) = a.dominant_source.as_deref() {
+                let source_cell = source_dot_cell(a.dominant_source.as_deref(), &ordered_sources);
-                    if let Some((bg, _)) = source_colours(src, &ordered_sources) {
-                        format!(
-                            "<span class=\"source-dot\" style=\"background:{}\" title=\"{}\"></span>",
-                            bg,
-                            html_attr_escape(src)
-                        )
-                    } else {
-                        format!(
-                            "<span class=\"source-dot source-dot-fallback\" title=\"{}\"></span>",
-                            html_attr_escape(src)
-                        )
-                    }
-                } else {
-                    "-".to_string()
-                };
                 BarRow {
                     cells: vec![
                         (i + 1).to_string(),
@@ -1170,22 +1168,7 @@ pub fn render_html_report(conn: &Connection, limit: i64, all_time_limit: i64) ->
                     .flatten()
                     .and_then(|m| m.cover_url);
                 let cover = resolve_cover(cover_url, &mut cover_files);
-                let source_cell = if let Some(src) = t.dominant_source.as_deref() {
+                let source_cell = source_dot_cell(t.dominant_source.as_deref(), &ordered_sources);
-                    if let Some((bg, _)) = source_colours(src, &ordered_sources) {
-                        format!(
-                            "<span class=\"source-dot\" style=\"background:{}\" title=\"{}\"></span>",
-                            bg,
-                            html_attr_escape(src)
-                        )
-                    } else {
-                        format!(
-                            "<span class=\"source-dot source-dot-fallback\" title=\"{}\"></span>",
-                            html_attr_escape(src)
-                        )
-                    }
-                } else {
-                    "-".to_string()
-                };
                 BarRow {
                     cells: vec![
                         (i + 1).to_string(),
@@ -1409,6 +1392,29 @@ const SOURCE_PALETTE: &[(&str, &str)] = &[
     ("rgba(220,130,130,0.22)", "rgba(220,130,130,0.60)"), // red
 ];
 
+/// Render the "Src" table cell for a row: a coloured dot carrying the source
+/// name as its tooltip, or `-` when the row predates the `source` column.
+///
+/// Sources beyond the palette fall back to a neutral dot so the column never
+/// silently loses the tooltip. The returned string is HTML and must be placed
+/// in a `BarRow` with `raw_cells: true`.
+fn source_dot_cell(source: Option<&str>, ordered_sources: &[String]) -> String {
+    let Some(src) = source else {
+        return "-".to_string();
+    };
+    match source_colours(src, ordered_sources) {
+        Some((bg, _)) => format!(
+            "<span class=\"source-dot\" style=\"background:{}\" title=\"{}\"></span>",
+            bg,
+            html_attr_escape(src)
+        ),
+        None => format!(
+            "<span class=\"source-dot source-dot-fallback\" title=\"{}\"></span>",
+            html_attr_escape(src)
+        ),
+    }
+}
+
 /// 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>(source: &str, ordered_sources: &[String]) -> Option<(&'a str, &'a str)> {
@@ -1517,6 +1523,11 @@ impl HtmlWriter {
 /// for use in HTML (`covers/<filename>`), and track the source file for
 /// copying into the output directory. Returns `None` if the path is missing
 /// or the file doesn't exist on disk.
+///
+/// The existence check gates the returned path as well as the copy list: a
+/// cache row whose image has since been deleted must fall through to the
+/// "No cover" placeholder rather than emit an `<img>` pointing at a file that
+/// was never copied into the output directory.
 fn resolve_cover(
     abs_path: Option<String>,
     cover_files: &mut Vec<std::path::PathBuf>,
@@ -1524,9 +1535,10 @@ fn resolve_cover(
     let src = abs_path?;
     let src_path = std::path::Path::new(&src);
     let filename = src_path.file_name()?;
-    if src_path.exists() {
+    if !src_path.exists() {
-        cover_files.push(src_path.to_path_buf());
+        return None;
     }
+    cover_files.push(src_path.to_path_buf());
     Some(format!("covers/{}", filename.to_string_lossy()))
 }
 
@@ -1723,13 +1735,9 @@ fn html_attr_escape(s: &str) -> String {
 /// applying the same deprioritisation rule as `top_genres`: multi-word genres
 /// rank before single-word broad descriptors ("rock", "electronic", etc.).
 fn top_genre_labels(genre_csv: &str, max_labels: usize) -> Vec<String> {
-    let mut labels: Vec<String> = genre_csv
+    // Reuse the parser the genre ranking uses, so a label rejected there (a
-        .split(',')
+    // review-site domain, say) can never reappear on an album card.
-        .map(str::trim)
+    let mut labels = db::split_genre_list(genre_csv);
-        .filter(|s| !s.is_empty())
-        .filter(|s| !db::looks_like_url(s))
-        .map(|s| s.to_string())
-        .collect();
 
     // Stable sort so that multi-word genres always precede single-word ones,
     // preserving the original MusicBrainz order within each group.
@@ -1798,6 +1806,57 @@ mod tests {
         assert_eq!(truncate_cell("abcdef", 3), "abc");
     }
 
+    #[test]
+    fn test_print_box_table_survives_row_exceeding_bar_max() {
+        // `top_genres` ranks deprioritised single-word genres last, so the
+        // first row is not necessarily the busiest. Passing an under-stated
+        // bar_max used to underflow `bar_width - filled` and panic mid-report.
+        let rows = vec![
+            vec![
+                "1".to_string(),
+                "alternative metal".to_string(),
+                "1".to_string(),
+            ],
+            vec!["2".to_string(), "pop".to_string(), "9".to_string()],
+        ];
+        // bar_max deliberately smaller than the largest row value.
+        print_box_table(&["#", "Genre", "Plays"], &rows, Some(1), &[1]);
+    }
+
+    #[test]
+    fn test_resolve_cover_skips_missing_file() {
+        let mut files: Vec<std::path::PathBuf> = Vec::new();
+
+        // No cover recorded at all.
+        assert_eq!(resolve_cover(None, &mut files), None);
+        assert!(files.is_empty());
+
+        // A cache row pointing at a file that is no longer on disk must not
+        // produce an <img> — nothing would be copied next to the HTML, so the
+        // page would show a broken image instead of the placeholder.
+        let missing = "/nonexistent/scrbblr/covers/gone.jpg".to_string();
+        assert_eq!(resolve_cover(Some(missing), &mut files), None);
+        assert!(
+            files.is_empty(),
+            "a missing file must not be queued to copy"
+        );
+    }
+
+    #[test]
+    fn test_resolve_cover_accepts_existing_file() {
+        let dir = std::env::temp_dir().join("scrbblr-test-covers");
+        std::fs::create_dir_all(&dir).unwrap();
+        let path = dir.join("present.jpg");
+        std::fs::write(&path, b"not really a jpeg").unwrap();
+
+        let mut files: Vec<std::path::PathBuf> = Vec::new();
+        let rel = resolve_cover(Some(path.to_string_lossy().to_string()), &mut files);
+        assert_eq!(rel.as_deref(), Some("covers/present.jpg"));
+        assert_eq!(files, vec![path.clone()]);
+
+        let _ = std::fs::remove_file(&path);
+    }
+
     #[test]
     fn test_album_cover_grid_limit_rounds_to_full_rows() {
         assert_eq!(album_cover_grid_limit(20), 24);
@@ -1808,9 +1867,10 @@ mod tests {
 
     #[test]
     fn test_top_genre_labels_caps_to_three() {
-        // "alternative rock" and "industrial" are multi-word → rank first.
+        // "alternative rock" is the only multi-word genre → it ranks first.
-        // "electronic" and "darkwave" are single-word → deprioritised.
+        // "electronic", "industrial" and "darkwave" are single-word and so are
-        // With limit 3 we get both multi-word genres + one single-word.
+        // deprioritised, keeping their original order among themselves.
+        // With limit 3 we get the multi-word genre + the first two of those.
         let labels = top_genre_labels("alternative rock, electronic, industrial, darkwave", 3);
         assert_eq!(
             labels,