From 33d83618dc52cd74014ba749df7050afa382001f Mon Sep 17 00:00:00 2001 From: Artur Meski Date: Sat, 25 Jul 2026 23:19:21 +0100 Subject: [PATCH] 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/ path even when the file was missing, so a deleted image produced a broken 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 | 5 + src/db.rs | 701 +++++++++++++++++++++++++++++++++----------------- src/enrich.rs | 224 +++++++++++++--- src/main.rs | 241 ++++++++++------- src/mpd.rs | 426 ++++++++++++++++++++++++------ src/report.rs | 162 ++++++++---- 6 files changed, 1265 insertions(+), 494 deletions(-) diff --git a/README.md b/README.md index 7daabc1..96a79c1 100644 --- a/README.md +++ b/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. diff --git a/src/db.rs b/src/db.rs index 1ad66de..46f61bf 100644 --- a/src/db.rs +++ b/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 { 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 - // the column already exists, which we silently ignore for idempotency. - conn.execute("ALTER TABLE scrobbles ADD COLUMN source TEXT", []) - .ok(); + // Add the `source` column to databases created before it existed. Check + // the current columns first rather than running the ALTER and discarding + // every error — that would also hide genuine failures (disk full, the + // 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 { + 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) { /// Get aggregate overview statistics for the given period. /// -/// Unique tracks are counted by concatenating artist + null byte + title, -/// so that "Artist A - Song X" and "Artist B - Song X" are counted separately. +/// Unique tracks are counted by concatenating artist + a `\0` separator + +/// 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 { let (whr, p) = where_clause(period); let sql = format!( @@ -362,28 +393,15 @@ pub fn overview(conn: &Connection, period: &str) -> Result { ); let mut stmt = conn.prepare(&sql)?; - let row = if p.is_empty() { - stmt.query_row([], |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)?, - }) + stmt.query_row(params_from_iter(p.iter()), |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)?, }) - } 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 Result Result> { 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 Result Result 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 Result Result> { ); 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() { - stmt.query_map([], mapper)?.collect() - } else { - stmt.query_map(params![p[0], p[1]], mapper)?.collect() - } + })? + .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 Result`. 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 { + 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 { - 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 { - 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 { - 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 { - 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 { - 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 { - 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 { - 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 { - 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> { // 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> { 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> { + 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. -fn split_genre_list(csv: &str) -> Vec { +/// such values already cached in the database never reach the report. This is +/// 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 { 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 { // 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 - // album name that does have one (same MBID-merging logic as top_albums). - let cover_expr = "COALESCE( - c.cover_url, - (SELECT ac.cover_url FROM album_cache ac - WHERE ac.album = s.album AND ac.cover_url IS NOT NULL - ORDER BY ac.artist LIMIT 1) - )"; + // if that has no cover it falls back to a cache entry that resolves to the + // same album group. Matching on album name alone would hand this artist the + // cover of an unrelated album that merely shares a title, so the fallback + // compares the same grouping key `top_albums` uses. + let group_key = album_group_key_expr("s", "c"); + let cover_expr = format!( + "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) { - Some((from, to)) => format!("AND s.scrobbled_at BETWEEN '{}' AND '{}'", from, to), - None => String::new(), + // Bind the period rather than interpolating it: the values come from + // `period_range` and so are well-formed, but a bound parameter cannot + // 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 = 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 = 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 = 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 = 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 = 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 = 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 = 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 = conn diff --git a/src/enrich.rs b/src/enrich.rs index a3c3021..ae60da2 100644 --- a/src/enrich.rs +++ b/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(|_| { - let home = std::env::var("HOME").expect("HOME not set"); - format!("{}/.local/share", home) - }); +/// +/// Returns an error rather than panicking when neither `$XDG_DATA_HOME` nor +/// `$HOME` is set, or the directory cannot be created — cover caching is an +/// optional convenience and should not take down a `watch` session that is +/// otherwise scrobbling fine. +pub fn covers_dir() -> Result { + 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"); - dir + fs::create_dir_all(&dir).map_err(|e| format!("could not create {}: {}", dir.display(), e))?; + Ok(dir) } // --------------------------------------------------------------------------- @@ -288,7 +298,8 @@ fn search_releases(client: &Client, artist: &str, album: &str) -> Vec { } 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 { 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 { 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 { 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 { 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::>().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, 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 { + 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 { let url = format!( @@ -836,12 +892,20 @@ pub fn resize_cover_bytes(bytes: &[u8]) -> Option> { // 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, -/// keyed on `"artist\talbum"`, so the filename is deterministic across runs. -/// Prefixed with `"itunes_"` to distinguish from MBID-based filenames. -fn itunes_cover_stem(artist: &str, album: &str) -> String { +/// Uses a 64-bit FNV-1a hash of `"artist\talbum"`, formatted as a 16-char hex +/// string behind `prefix`. This gives a compact, deterministic 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. +/// +/// `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 { /// 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); - for c in s.chars() { - match c { - 'A'..='Z' | 'a'..='z' | '0'..='9' | '-' | '_' | '.' | '~' => result.push(c), + let mut result = String::with_capacity(s.len() * 3); + // Encode straight from the UTF-8 bytes: a multi-byte character is encoded + // one byte at a time anyway, and going through `char` would allocate a + // 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() { - result.push_str(&format!("%{:02X}", byte)); - } + let _ = write!(result, "%{: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 { - conn.execute("DELETE FROM album_cache", []) - .expect("Failed to clear album_cache"); - eprintln!("Cleared album cache (force mode)."); + // Force mode selects every scrobbled album instead of clearing the cache + // first. `DELETE FROM album_cache` would also discard albums pinned by + // hand with `pin-album` (which exist precisely because automatic search + // 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."); - } - let albums = db::uncached_albums(conn).expect("Failed to query uncached albums"); + db::all_scrobbled_albums(conn) + } 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. - let (fetched_genre, release_group_id) = if fetch_genres || fetch_covers { + // Step 2: Fetch genres/tags and release-group ID from the primary + // 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 { - None - } else if !use_itunes { - None - } else { + let mut cover = if fetch_covers && use_itunes { 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"); diff --git a/src/main.rs b/src/main.rs index e1d6bc5..ebf66c1 100644 --- a/src/main.rs +++ b/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 { + 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 home = std::env::var("HOME").expect("HOME not set"); - format!("{}/.local/share", home) - }); + let data_dir = match std::env::var("XDG_DATA_HOME") { + Ok(d) if !d.is_empty() => d, + _ => 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, let (tx, rx) = mpsc::channel::(); // 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 - // main loop exits immediately on Ctrl+C (no processes to wait for). + // playerctl processes have ended. Set to 0 when MPRIS is disabled so the + // 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 = 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, .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(stdout); + let reader = std::io::BufReader::new(meta_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, // 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(stdout); + let reader = std::io::BufReader::new(stat_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, 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 { - eprintln!("[warn] Failed to spawn playerctl: {}", e); + for result in [meta_result, status_result] { + 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, 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. + // Wait for MPRIS reader threads to finish. Their pipes are now closed. 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 `, writes `index.html` + `covers/` to a directory. -fn run_report( - period: &str, - json: bool, - html: bool, - output: Option<&str>, - limit: i64, - all_time_limit: i64, - no_enrich: bool, - mpd_cfg: &mpd::MpdConfig, - db_path: &str, -) { +fn run_report(opts: &ReportOptions<'_>) { + let ReportOptions { + period, + json, + html, + output, + limit, + all_time_limit, + no_enrich, + 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 = - all_time_limit.unwrap_or_else(|| round_to_5((limit as f64 * 2.5).round() as i64)); + let atl = all_time_limit + .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( - &period, + run_report(&ReportOptions { + period: &period, json, html, - output.as_deref(), + output: output.as_deref(), limit, - atl, + all_time_limit: atl, no_enrich, - &mpd_cfg, - &path, - ); + mpd_cfg: &mpd_cfg, + 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) { - let reset = match retry { - 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 { + if let Some(scope) = retry.scope() { + match db::reset_retry_timestamps(&conn, scope, artist.as_deref()) { Ok(n) => eprintln!( "Reset cooldown for {} album(s) via --retry {}.", n, - match retry { - EnrichRetry::None => "none", - EnrichRetry::Covers => "covers", - EnrichRetry::Genres => "genres", - EnrichRetry::All => "all", - EnrichRetry::MpdGenres => "mpd-genres", - } + scope.as_str() ), Err(e) => eprintln!("[warn] Failed to reset retry timestamps: {}", e), } diff --git a/src/mpd.rs b/src/mpd.rs index 42e6ffc..8fa2c9f 100644 --- a/src/mpd.rs +++ b/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>, /// Write handle for sending commands. writer: Box, + /// 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> { + let deadline = Instant::now() + RESPONSE_TIMEOUT; + let mut buf: Vec = 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 { // 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 { 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 { .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 { 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 { /// 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(); - conn.reader - .read_line(&mut line) - .map_err(|e| format!("Failed to read MPD welcome: {}", e))?; + let line = conn + .read_line() + .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` of all lines parsed before `OK`. An `ACK` -/// line is treated as an error and returns an empty map (the caller ignores -/// the result for most queries). +/// line is treated as an error and returns what was parsed so far (usually +/// 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 { +fn read_response(conn: &mut MpdConn) -> Option> { let mut map = HashMap::new(); loop { - let mut line = String::new(); - match conn.reader.read_line(&mut line) { - Ok(0) => break, // EOF — connection closed - Ok(_) => {} - Err(_) => break, - } - let line = line.trim_end_matches('\n').trim_end_matches('\r'); + let raw = match conn.read_line() { + Ok(Some(l)) => l, + // EOF part-way through a response is a lost connection, not an + // empty result. + Ok(None) => { + conn.desynced = true; + 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 { 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 { fn read_picture_header(conn: &mut MpdConn) -> Option<(u64, u64)> { let mut total_size: u64 = 0; loop { - let mut line = String::new(); - conn.reader.read_line(&mut line).ok()?; - let line = line.trim_end_matches('\n').trim_end_matches('\r'); + let raw = conn.read_line().ok()??; + let line = raw.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 -/// reads the remainder from the socket, so mixing text line reads and binary -/// chunk reads on the same `BufReader` is safe. +/// [`MpdConn::read_exact`] drains buffered data first and then reads the +/// remainder from the socket, so mixing text line reads and binary chunk reads +/// 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> { 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> { /// EOF, protocol errors, or I/O errors. fn read_chunk_terminator(conn: &mut MpdConn) -> Option<()> { loop { - let mut line = String::new(); - conn.reader.read_line(&mut line).ok()?; + let raw = conn.read_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 { 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); - MpdStatus { - state: map.get("state").cloned().unwrap_or_default(), + match read_response(conn) { + Some(map) => MpdStatus { + 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() { - let map = read_response(conn); - if let Some(file) = map.get("file") { - return Some(file.clone()); - } + if send_command(conn, &cmd).is_ok() + && let Some(map) = read_response(conn) + && let Some(file) = map.get("file") + { + 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() { - let map = read_response(conn); - if let Some(file) = map.get("file") { - return Some(file.clone()); - } + if send_command(conn, &cmd).is_ok() + && let Some(map) = read_response(conn) + && let Some(file) = map.get("file") + { + 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> { let mut all_bytes: Vec = Vec::new(); let mut offset: u64 = 0; let mut expected_total: Option = 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> { // 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> { 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> { 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, expected_total: Option) -> Option> { 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> { /// 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 -/// hex string prefixed with `"mpd_"`. This gives a compact, deterministic -/// 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. +/// Thin wrapper over [`enrich::cover_stem`] that pins the `mpd_` prefix, which +/// `repair-mpd-covers` relies on to find these files again. /// /// Example: `mpd_a3f7c2b91e045d68.jpg` fn album_cover_stem(artist: &str, album: &str) -> String { - // FNV-1a 64-bit hash constants. - 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) + enrich::cover_stem("mpd", artist, album) } // --------------------------------------------------------------------------- @@ -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) -> 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 = Vec::new(); loop { - let mut line = String::new(); - match conn.reader.read_line(&mut line) { + match conn.reader.read_until(b'\n', &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) -> Idle } // "changed: player" and any other key/value lines are consumed // and discarded here — we query currentsong/status ourselves. + line.clear(); } - Err(ref e) - if e.kind() == io::ErrorKind::WouldBlock || e.kind() == io::ErrorKind::TimedOut => - { + Err(ref e) if is_timeout(e) => { // 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. - let covers = enrich::covers_dir(); + // Save the processed image to the covers cache directory. Caching a cover + // 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 // ----------------------------------------------------------------------- diff --git a/src/report.rs b/src/report.rs index fd24582..3a991f1 100644 --- a/src/report.rs +++ b/src/report.rs @@ -370,10 +370,14 @@ fn print_box_table( let shown = truncate_cell(cell, widths[i]); print!(" {:> = 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> = 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> = 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> = 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() { - if let Some((bg, _)) = source_colours(src, &ordered_sources) { - format!( - "", - bg, - html_attr_escape(src) - ) - } else { - format!( - "", - html_attr_escape(src) - ) - } - } else { - "-".to_string() - }; + let source_cell = source_dot_cell(a.dominant_source.as_deref(), &ordered_sources); 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() { - if let Some((bg, _)) = source_colours(src, &ordered_sources) { - format!( - "", - bg, - html_attr_escape(src) - ) - } else { - format!( - "", - html_attr_escape(src) - ) - } - } else { - "-".to_string() - }; + let source_cell = source_dot_cell(t.dominant_source.as_deref(), &ordered_sources); 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!( + "", + bg, + html_attr_escape(src) + ), + None => format!( + "", + 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/`), 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 `` pointing at a file that +/// was never copied into the output directory. fn resolve_cover( abs_path: Option, cover_files: &mut Vec, @@ -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() { - cover_files.push(src_path.to_path_buf()); + if !src_path.exists() { + 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 { - let mut labels: Vec = genre_csv - .split(',') - .map(str::trim) - .filter(|s| !s.is_empty()) - .filter(|s| !db::looks_like_url(s)) - .map(|s| s.to_string()) - .collect(); + // Reuse the parser the genre ranking uses, so a label rejected there (a + // review-site domain, say) can never reappear on an album card. + let mut labels = db::split_genre_list(genre_csv); // 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 = 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 — 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 = 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. - // "electronic" and "darkwave" are single-word → deprioritised. - // With limit 3 we get both multi-word genres + one single-word. + // "alternative rock" is the only multi-word genre → it ranks first. + // "electronic", "industrial" and "darkwave" are single-word and so are + // 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,