From 42aec6720cb869366e8940732cabd9845d81fdba Mon Sep 17 00:00:00 2001 From: cucadmuh <49571317+cucadmuh@users.noreply.github.com> Date: Tue, 2 Jun 2026 10:58:39 +0300 Subject: [PATCH] fix(cover): stop per-song over-fetch + log failed cover downloads (#944) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(cover): stop per-song cover over-fetch (album/mf-* explosion) album_has_distinct_disc_covers returned true as soon as two tracks on the same disc had different cover ids. On Navidrome every song has its own mf- coverArt, so almost every album was flagged "distinct disc covers" and backfill warmed one cover per track (~520k for ~170k tracks), filling album/ with mf-* dirs instead of ~one cover per album. Treat a release as having distinct disc covers only when each disc has a single consistent cover that differs across discs (genuine box set); per-song ids now collapse to one cover per album. Mirror the same fix in the TS albumHasDistinctDiscCovers used by on-demand warming. Adds regression tests on both sides. * docs(changelog): record per-song cover over-fetch fix (PR #944) Give the album/mf-* over-fetch fix its own [1.47.0] Fixed entry and a settingsCredits line under PR #944. * feat(cover): log failed cover downloads with album/artist name A non-200 (or network-failed) getCoverArt download was swallowed silently. Now the failure is logged with the resolved album/artist name and the server error, so a server refusing covers under backfill load (5xx/429/timeouts) is visible. - cover_resolve: describe_cover_entity() resolves a human label from the local index (album "Name" — Artist / artist "Name"), best-effort with id fallback. - cover_cache: log_cover_fetch_failure() in ensure_inner logs on the download error path; threads optional library_server_id through CoverCacheEnsureArgs so the name lookup happens only on failure. Backfill logs at normal level, on-demand misses at debug level. --- CHANGELOG.md | 8 + .../psysonic-library/src/cover_resolve.rs | 158 ++++++++++++++++-- src-tauri/src/cover_cache/backfill_worker.rs | 1 + src-tauri/src/cover_cache/mod.rs | 42 ++++- src/config/settingsCredits.ts | 1 + src/cover/resolveEntry.test.ts | 21 +++ src/cover/resolveEntry.ts | 30 +++- 7 files changed, 244 insertions(+), 17 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8c3f20b8..5e8a7fef 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -662,6 +662,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 * **Settings → offline & cache** caused periodic CPU spikes: the section re-walked each server's full cover directory every 15s. The per-server walk is now memoized with a short TTL and reused by the stats/progress commands; the menu recomputes on entry and via progress/cache-cleared events, with a 5-minute safety poll instead of a tight 15s loop. * Transient download failures (network / 5xx / 429) retry up to 3× with exponential backoff; permanent 4xx settle without re-scanning. +### Cover art — per-song over-fetch on Navidrome (album/mf-* explosion) + +**By [@cucadmuh](https://github.com/cucadmuh), PR [#944](https://github.com/Psychotoxical/psysonic/pull/944)** + +* The per-disc cover detection treated each track's own `mf-` coverArt as "distinct disc art", so backfill warmed one cover per track (e.g. ~520k cached elements for ~170k tracks) and filled the `album/` bucket with `mf-*` directories. +* It now treats a release as multi-disc only when each disc has a single consistent cover that differs across discs (a genuine box set); per-song ids collapse to one cover per album (≈ albums + artists). Fixed on both the Rust backfill path and the on-demand TS `albumHasDistinctDiscCovers`. +* Failed cover downloads are now logged with the album/artist name and the server error (e.g. `fetch failed for album "X" — Artist (coverArtId=…): cover HTTP 503`). Backfill failures log at the normal level; incidental on-demand misses stay at the debug level. + ## [1.46.0] - 2026-05-18 > **🙏 Special thanks to [@zz5zz](https://github.com/zz5zz)** for his tireless quirk-spotting and bug reports on the [Psysonic Discord](https://discord.gg/AMnDRErm4u) — several of the polish fixes in this release landed directly off the back of his messages. diff --git a/src-tauri/crates/psysonic-library/src/cover_resolve.rs b/src-tauri/crates/psysonic-library/src/cover_resolve.rs index 02b61fd0..2a2d07c2 100644 --- a/src-tauri/crates/psysonic-library/src/cover_resolve.rs +++ b/src-tauri/crates/psysonic-library/src/cover_resolve.rs @@ -54,7 +54,15 @@ pub fn album_has_distinct_disc_covers( row.get::<_, Option>(3)?, )) })?; - let mut art_by_disc: std::collections::HashMap = std::collections::HashMap::new(); + // Genuine per-disc artwork = a multi-disc release where each disc has ONE + // consistent cover and those covers differ between discs (e.g. a box set). + // It must NOT be tripped by per-song cover ids: Navidrome (and other + // OpenSubsonic servers) give every track its own `mf-` coverArt, so a + // disc whose tracks carry many different ids is per-song art, not per-disc + // art — treating it as distinct explodes the backfill into one cover per + // track instead of one per album. + let mut art_by_disc: std::collections::HashMap> = + std::collections::HashMap::new(); for row in rows { let (track_id, disc_number, cover_art_id, row_album_id) = row?; let disc = disc_number.unwrap_or(1); @@ -63,19 +71,22 @@ pub fn album_has_distinct_disc_covers( .filter(|s| !s.is_empty()) .unwrap_or(album_id); let fetch = song_fetch_cover_art_id(cover_art_id.as_deref(), &track_id, al); - if let Some(prev) = art_by_disc.get(&disc) { - if prev != &fetch { - return Ok(true); - } - } else { - art_by_disc.insert(disc, fetch); - } + art_by_disc.entry(disc).or_default().insert(fetch); } if art_by_disc.len() <= 1 { return Ok(false); } - let unique: std::collections::HashSet<_> = art_by_disc.values().collect(); - Ok(unique.len() > 1) + let mut disc_covers: std::collections::HashSet = std::collections::HashSet::new(); + for covers in art_by_disc.values() { + // Tracks within a disc disagree → per-song ids, not a shared disc cover. + if covers.len() != 1 { + return Ok(false); + } + if let Some(cover) = covers.iter().next() { + disc_covers.insert(cover.clone()); + } + } + Ok(disc_covers.len() > 1) }) } @@ -192,6 +203,72 @@ pub fn cover_backfill_items_for_album( Ok(out) } +/// Human-readable label for a cover target, for failure logs: +/// `album "Name" — Artist` / `artist "Name"`. Best-effort: returns `None` when +/// the entity is not in the local index (e.g. a stale per-disc `mf-*` id), so +/// the caller can fall back to the raw id. +pub fn describe_cover_entity( + store: &LibraryStore, + library_server_id: &str, + cache_kind: &str, + cache_entity_id: &str, +) -> Option { + let id = cache_entity_id.trim(); + if id.is_empty() { + return None; + } + store + .with_read_conn(|conn| { + let label = if cache_kind == "artist" { + let name = conn + .query_row( + "SELECT name FROM artist WHERE server_id = ?1 AND id = ?2", + rusqlite::params![library_server_id, id], + |row| row.get::<_, String>(0), + ) + .optional()? + .or(conn + .query_row( + "SELECT artist FROM track + WHERE server_id = ?1 AND artist_id = ?2 AND deleted = 0 + AND NULLIF(TRIM(artist), '') IS NOT NULL + LIMIT 1", + rusqlite::params![library_server_id, id], + |row| row.get::<_, String>(0), + ) + .optional()?); + name.map(|n| format!("artist \"{n}\"")) + } else { + let pair = conn + .query_row( + "SELECT name, artist FROM album WHERE server_id = ?1 AND id = ?2", + rusqlite::params![library_server_id, id], + |row| Ok((row.get::<_, String>(0)?, row.get::<_, Option>(1)?)), + ) + .optional()? + .or(conn + .query_row( + "SELECT album, artist FROM track + WHERE server_id = ?1 AND album_id = ?2 AND deleted = 0 + AND NULLIF(TRIM(album), '') IS NOT NULL + LIMIT 1", + rusqlite::params![library_server_id, id], + |row| Ok((row.get::<_, String>(0)?, row.get::<_, Option>(1)?)), + ) + .optional()?); + pair.map(|(name, artist)| { + match artist.as_deref().map(str::trim).filter(|s| !s.is_empty()) { + Some(a) => format!("album \"{name}\" — {a}"), + None => format!("album \"{name}\""), + } + }) + }; + Ok(label) + }) + .ok() + .flatten() +} + pub fn resolve_artist_cover_entry( _store: &LibraryStore, _library_server_id: &str, @@ -329,4 +406,65 @@ mod tests { let e = resolve_track_cover_entry(&store, "srv", "tr2").unwrap().unwrap(); assert_eq!(e.cache_entity_id, "mf-b"); } + + // Navidrome gives every song its own `mf-` coverArt. Many tracks on a + // single disc must NOT count as distinct disc covers, or backfill would warm + // one cover per track instead of one per album. + #[test] + fn per_song_ids_within_one_disc_are_not_distinct() { + let store = LibraryStore::open_in_memory(); + seed_album(&store, "srv", "al-nav", None); + seed_track(&store, "srv", "tr1", "al-nav", 1, Some("mf-1")); + seed_track(&store, "srv", "tr2", "al-nav", 1, Some("mf-2")); + seed_track(&store, "srv", "tr3", "al-nav", 1, Some("mf-3")); + assert!(!album_has_distinct_disc_covers(&store, "srv", "al-nav").unwrap()); + let items = cover_backfill_items_for_album(&store, "srv", "al-nav").unwrap(); + let ids: Vec<_> = items.iter().map(|i| i.cache_entity_id.as_str()).collect(); + assert_eq!(ids, vec!["al-nav"]); + } + + #[test] + fn describe_entity_labels_album_and_artist() { + let store = LibraryStore::open_in_memory(); + store + .with_conn_mut("seed_describe", |conn| { + conn.execute( + "INSERT INTO album (server_id, id, name, artist, synced_at, raw_json) + VALUES ('srv', 'al-1', 'Discovery', 'Daft Punk', 1, '{}')", + [], + )?; + conn.execute( + "INSERT INTO artist (server_id, id, name, synced_at, raw_json) + VALUES ('srv', 'ar-1', 'Daft Punk', 1, '{}')", + [], + )?; + Ok(()) + }) + .unwrap(); + assert_eq!( + describe_cover_entity(&store, "srv", "album", "al-1").as_deref(), + Some("album \"Discovery\" — Daft Punk"), + ); + assert_eq!( + describe_cover_entity(&store, "srv", "artist", "ar-1").as_deref(), + Some("artist \"Daft Punk\""), + ); + assert_eq!(describe_cover_entity(&store, "srv", "album", "al-missing"), None); + } + + // Multi-disc, but each disc still exposes per-song ids (not a shared disc + // cover) → per-song art, so backfill collapses to the single album cover. + #[test] + fn per_song_ids_across_discs_are_not_distinct() { + let store = LibraryStore::open_in_memory(); + seed_album(&store, "srv", "al-nav2", None); + seed_track(&store, "srv", "tr1", "al-nav2", 1, Some("mf-1")); + seed_track(&store, "srv", "tr2", "al-nav2", 1, Some("mf-2")); + seed_track(&store, "srv", "tr3", "al-nav2", 2, Some("mf-3")); + seed_track(&store, "srv", "tr4", "al-nav2", 2, Some("mf-4")); + assert!(!album_has_distinct_disc_covers(&store, "srv", "al-nav2").unwrap()); + let items = cover_backfill_items_for_album(&store, "srv", "al-nav2").unwrap(); + let ids: Vec<_> = items.iter().map(|i| i.cache_entity_id.as_str()).collect(); + assert_eq!(ids, vec!["al-nav2"]); + } } diff --git a/src-tauri/src/cover_cache/backfill_worker.rs b/src-tauri/src/cover_cache/backfill_worker.rs index 4ed3e9a5..74488573 100644 --- a/src-tauri/src/cover_cache/backfill_worker.rs +++ b/src-tauri/src/cover_cache/backfill_worker.rs @@ -271,6 +271,7 @@ async fn ensure_one( username: session.username, password: session.password, library_bulk: true, + library_server_id: Some(session.library_server_id), }; let _ = CoverCacheState::ensure_inner(&st, &app, &args, Some(http_sem)).await; } diff --git a/src-tauri/src/cover_cache/mod.rs b/src-tauri/src/cover_cache/mod.rs index 34328ffa..5a3eab78 100644 --- a/src-tauri/src/cover_cache/mod.rs +++ b/src-tauri/src/cover_cache/mod.rs @@ -107,6 +107,10 @@ pub struct CoverCacheEnsureArgs { /// Library backfill: all derived tiers, no `cover:tier-ready` floods to the webview. #[serde(default)] pub library_bulk: bool, + /// Library server id (DB key) — set by backfill so a failed fetch can be logged + /// with the album/artist name. On-demand UI ensures leave it `None`. + #[serde(default)] + pub library_server_id: Option, } fn cover_dir_for_args(root: &Path, args: &CoverCacheEnsureArgs) -> PathBuf { @@ -266,7 +270,8 @@ impl CoverCacheState { } else { match download_cover_payload(&dir, &client, &http_sem, args).await { Ok(bytes) => CoverSource::Bytes(bytes), - Err(_) => { + Err(err) => { + log_cover_fetch_failure(app, args, &err); let _ = std::fs::create_dir_all(&dir); let _ = std::fs::write(dir.join(COVER_FETCH_FAIL_MARKER), b"1"); return Ok(CoverCacheEnsureResult { @@ -359,6 +364,41 @@ impl CoverCacheState { } } +/// Log a non-200 / failed cover download with the album/artist name when known. +/// Backfill fetches (`library_bulk`) log at the normal level — the user wants to +/// see which covers a busy server refused; incidental on-demand UI misses stay at +/// the debug level so they don't spam the normal log. +fn log_cover_fetch_failure(app: &AppHandle, args: &CoverCacheEnsureArgs, err: &str) { + let label = args + .library_server_id + .as_deref() + .filter(|s| !s.is_empty()) + .and_then(|lib_id| { + app.try_state::().and_then(|rt| { + psysonic_library::cover_resolve::describe_cover_entity( + &rt.store, + lib_id, + &args.cache_kind, + &args.cache_entity_id, + ) + }) + }) + .unwrap_or_else(|| format!("{} {}", args.cache_kind, args.cache_entity_id)); + if args.library_bulk { + crate::app_eprintln!( + "[cover-backfill] fetch failed for {label} (coverArtId={}, tier={}): {err}", + args.cover_art_id, + args.tier + ); + } else { + crate::app_deprintln!( + "[cover] fetch failed for {label} (coverArtId={}, tier={}): {err}", + args.cover_art_id, + args.tier + ); + } +} + fn emit_tier_ready(app: &AppHandle, args: &CoverCacheEnsureArgs, tier: u32, path: &Path) { let Ok(meta) = std::fs::metadata(path) else { return; diff --git a/src/config/settingsCredits.ts b/src/config/settingsCredits.ts index 45dfe7b0..72f95501 100644 --- a/src/config/settingsCredits.ts +++ b/src/config/settingsCredits.ts @@ -145,6 +145,7 @@ const CONTRIBUTOR_ENTRIES = [ 'Live Search: scoped browse on Artists, Albums, New Releases, Tracks, and Composers — header badge, ghost restore, album title FTS, session stash (PR #938)', 'Performance: idle Rust CPU — backfill coordinator park, probe overlay stability, throttled idle polls, lazy cover prefetch restore (PR #939)', 'Cover backfill: disk-free idle gate, snapshot-diff worklist, live-tunable parallelism, transient-error retries, and memoized offline & cache stats to stop idle CPU spin (PR #943)', + 'Cover art: fix per-song cover over-fetch on Navidrome — only genuine per-disc artwork expands, collapsing ~520k per-track fetches to one cover per album (PR #944)', ], }, { diff --git a/src/cover/resolveEntry.test.ts b/src/cover/resolveEntry.test.ts index d96424cc..67b274e1 100644 --- a/src/cover/resolveEntry.test.ts +++ b/src/cover/resolveEntry.test.ts @@ -58,4 +58,25 @@ describe('albumHasDistinctDiscCovers', () => { ]), ).toBe(true); }); + + it('false for per-song ids within a single disc (Navidrome)', () => { + expect( + albumHasDistinctDiscCovers([ + { id: 't1', albumId: 'al-1', coverArt: 'mf-1', discNumber: 1 }, + { id: 't2', albumId: 'al-1', coverArt: 'mf-2', discNumber: 1 }, + { id: 't3', albumId: 'al-1', coverArt: 'mf-3', discNumber: 1 }, + ]), + ).toBe(false); + }); + + it('false for per-song ids across discs (no shared disc cover)', () => { + expect( + albumHasDistinctDiscCovers([ + { id: 't1', albumId: 'al-1', coverArt: 'mf-1', discNumber: 1 }, + { id: 't2', albumId: 'al-1', coverArt: 'mf-2', discNumber: 1 }, + { id: 't3', albumId: 'al-1', coverArt: 'mf-3', discNumber: 2 }, + { id: 't4', albumId: 'al-1', coverArt: 'mf-4', discNumber: 2 }, + ]), + ).toBe(false); + }); }); diff --git a/src/cover/resolveEntry.ts b/src/cover/resolveEntry.ts index 43b22238..6a50d69c 100644 --- a/src/cover/resolveEntry.ts +++ b/src/cover/resolveEntry.ts @@ -33,21 +33,39 @@ export function resolveSongFetchCoverArtId(song: CoverArtResolvableSong): string return undefined; } -/** True when 2+ discs use different cover art ids. */ +/** + * True only for genuine per-disc artwork: a multi-disc release where each disc + * has ONE consistent cover and those covers differ between discs (e.g. a box + * set). It must NOT be tripped by per-song cover ids — Navidrome (and other + * OpenSubsonic servers) give every track its own `mf-` coverArt, so a disc + * whose tracks carry many different ids is per-song art, not per-disc art, and + * treating it as distinct would warm one cover per track instead of per album. + * + * Mirrors `album_has_distinct_disc_covers` in `psysonic-library/cover_resolve.rs`. + */ export function albumHasDistinctDiscCovers( songs: ReadonlyArray>, ): boolean { - const artByDisc = new Map(); + const artByDisc = new Map>(); for (const song of songs) { const disc = song.discNumber ?? 1; const artId = resolveSongFetchCoverArtId(song); if (!artId) continue; - const prev = artByDisc.get(disc); - if (prev !== undefined && prev !== artId) return true; - artByDisc.set(disc, artId); + let set = artByDisc.get(disc); + if (!set) { + set = new Set(); + artByDisc.set(disc, set); + } + set.add(artId); } if (artByDisc.size <= 1) return false; - return new Set(artByDisc.values()).size > 1; + const discCovers = new Set(); + for (const covers of artByDisc.values()) { + // Tracks within a disc disagree → per-song ids, not a shared disc cover. + if (covers.size !== 1) return false; + for (const cover of covers) discCovers.add(cover); + } + return discCovers.size > 1; } /** Album entity — one cache slot per album unless `distinctDiscCovers`. */