From a60546929e1a5944da19016eae26442214e55595 Mon Sep 17 00:00:00 2001 From: Broque Thomas <26755000+Nezreka@users.noreply.github.com> Date: Fri, 24 Apr 2026 11:54:25 -0700 Subject: [PATCH 1/3] Fix Album Completeness job reporting zero findings for every album MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reported by sassmastawillis: the Album Completeness maintenance job scans 3127 albums in 0.1 seconds and reports 0 findings — for every user, regardless of whether their library is actually complete. Restoring an older DB surfaced 7 correct findings, so the code logic works; the DB state is what's making everything look complete. Root cause: `albums.track_count` is only ever written by server-sync paths — Plex's `leafCount`/`childCount` and SoulSync standalone's `len(tracks)`. It's the OBSERVED count of tracks SoulSync has indexed, which is always exactly what `COUNT(tracks)` returns for that album. The completeness job treated it as the EXPECTED total and compared it against the observed count. They're equal by construction, so `actual >= expected` is always true: skip, 0.1s scan, 0 findings. Fix: new `api_track_count INTEGER` column on `albums`, written only by metadata-source code paths. Populated in two places so the scan is fast and the fallback is robust. 1. Enrichment workers — shared helper `set_album_api_track_count` in `core/worker_utils.py`. Called by each worker's existing `_update_album` method alongside its other album-column UPDATEs: - spotify_worker: `album_obj.total_tracks` from the Spotify Album dataclass (already in hand, zero new API calls) - itunes_worker: same, from the iTunes Album dataclass - deezer_worker: `nb_tracks` from full_data, falling back to search_data when the full lookup didn't run - discogs_worker: count of tracklist rows where `type_=='track'` (Discogs tracklists interleave heading and index rows that shouldn't count as songs) Helper skips the write on zero/None/negative/non-numeric inputs so a source lacking track info can't clobber a good value a different source already wrote. Caller owns the transaction — helper just queues an UPDATE on the caller's cursor without committing, so it batches cleanly with each worker's existing multi-UPDATE pattern. Hydrabase worker deliberately not touched — it's a P2P mirror that doesn't write album metadata to the local DB. Hydrabase- primary users hit the fallback path below. 2. Album Completeness repair job — new `al.api_track_count` column in the SELECT, read first in the scan loop. On miss (album never enriched, or enrichment workers haven't run yet on a fresh install), falls through to the existing `_get_expected_total()` API lookup and persists the result via the same shared helper (wrapped in connection/commit management since the repair job runs outside a worker's batched transaction). Also removed `al.track_count` from the scan's SELECT — now unused since the observed count was the whole source of this bug, and leaving a dead SELECT would invite a future engineer to re-introduce the same comparison. Help text on the job card was reworded so it honestly describes current behavior ("counts cached during normal enrichment are used when available; otherwise the job queries a metadata source directly") rather than the old "active provider first, then others as fallback" phrasing, which doesn't match how the cache actually fills — any enrichment worker that runs can populate it, and the last writer wins. Document-only follow-up if this edge case ever bites in practice: add a `api_track_count_source` column so the scan can prefer the configured primary source's count over others (e.g. deluxe vs. standard edition mismatches). Not worth the complexity today. For existing users, the first completeness scan after upgrade is fast to the extent their library is already enriched: the workers already ran and populated `api_track_count` on their normal schedule. For brand-new installs, the scan's fallback path handles the cold start — slower, but correct, and subsequent scans are fast. Does NOT affect: - Download / post-processing / wishlist / sync code paths — none of them read `track_count` for completeness semantics. - Plex / Jellyfin / Navidrome / standalone sync — still write `track_count` exactly as before; `api_track_count` is a separate column they never touch. - Other repair jobs. - Any UI path — same finding schema, just correct counts now. Files: - database/music_database.py — idempotent migration adding `api_track_count INTEGER DEFAULT NULL` to the existing album-column check block. - core/worker_utils.py — new `set_album_api_track_count` helper with the documented skip-on-bad-input contract. - core/spotify_worker.py, itunes_worker.py, deezer_worker.py, discogs_worker.py — one-liner call from each `_update_album`. - core/repair_jobs/album_completeness.py — scan uses the cache; fallback path persists API-lookup results via the shared helper; help text updated to match actual behavior. - tests/test_worker_utils_album_track_count.py — 9 tests covering the helper's write/skip contract + no-commit invariant. - tests/test_album_completeness_job.py — 2 tests for the repair job's fallback-path wrapper. - webui/static/helper.js — WHATS_NEW entry. Credit: sassmastawillis spotted the bug; the "restored older DB finds 7 albums" signal pinpointed DB state over code logic and made the diagnosis tractable. --- core/deezer_worker.py | 13 +- core/discogs_worker.py | 14 +- core/itunes_worker.py | 6 +- core/repair_jobs/album_completeness.py | 67 +++- core/spotify_worker.py | 6 +- core/worker_utils.py | 52 ++++ database/music_database.py | 13 + tests/test_album_completeness_job.py | 308 +++++++++++++++++++ tests/test_worker_utils_album_track_count.py | 116 +++++++ webui/static/helper.js | 1 + 10 files changed, 583 insertions(+), 13 deletions(-) create mode 100644 tests/test_worker_utils_album_track_count.py diff --git a/core/deezer_worker.py b/core/deezer_worker.py index b5e942fc..4940d16d 100644 --- a/core/deezer_worker.py +++ b/core/deezer_worker.py @@ -8,7 +8,7 @@ from datetime import datetime, timedelta from utils.logging_config import get_logger from database.music_database import MusicDatabase from core.deezer_client import DeezerClient -from core.worker_utils import interruptible_sleep +from core.worker_utils import interruptible_sleep, set_album_api_track_count logger = get_logger("deezer_worker") @@ -579,6 +579,17 @@ class DeezerWorker: WHERE id = ? AND (genres IS NULL OR genres = '' OR genres = '[]') """, (json.dumps(genre_names), album_id)) + # Cache the authoritative expected track count for the Album + # Completeness repair job. Deezer's field is `nb_tracks`; prefer + # full_data over search_data so we pick up the richer count when + # the full album lookup ran. Helper handles the int conversion + # and skip-on-missing semantics. + set_album_api_track_count( + cursor, + album_id, + (full_data.get('nb_tracks') if full_data else None) or search_data.get('nb_tracks'), + ) + conn.commit() except Exception as e: diff --git a/core/discogs_worker.py b/core/discogs_worker.py index 99014882..1b124a22 100644 --- a/core/discogs_worker.py +++ b/core/discogs_worker.py @@ -18,7 +18,7 @@ from datetime import datetime, timedelta from utils.logging_config import get_logger from database.music_database import MusicDatabase from core.discogs_client import DiscogsClient -from core.worker_utils import interruptible_sleep +from core.worker_utils import interruptible_sleep, set_album_api_track_count logger = get_logger("discogs_worker") @@ -454,6 +454,18 @@ class DiscogsWorker: WHERE id = ? AND (thumb_url IS NULL OR thumb_url = '') """, (image_url, album_id)) + # Cache the authoritative expected track count for the Album + # Completeness repair job. Discogs tracklists mix actual tracks + # with section headings and index entries — only type_=='track' + # rows are real songs, so filter before counting. (Helper can't + # do this filter for us; it's Discogs-specific shape.) + tracklist = data.get('tracklist') or [] + set_album_api_track_count( + cursor, + album_id, + sum(1 for t in tracklist if t.get('type_') == 'track'), + ) + conn.commit() except Exception as e: diff --git a/core/itunes_worker.py b/core/itunes_worker.py index 9a4769b4..1e1c2e9d 100644 --- a/core/itunes_worker.py +++ b/core/itunes_worker.py @@ -8,7 +8,7 @@ from datetime import datetime, timedelta from utils.logging_config import get_logger from database.music_database import MusicDatabase from core.itunes_client import iTunesClient -from core.worker_utils import interruptible_sleep +from core.worker_utils import interruptible_sleep, set_album_api_track_count logger = get_logger("itunes_worker") @@ -669,6 +669,10 @@ class iTunesWorker: WHERE id = ? AND (year IS NULL OR year = '' OR year = '0') """, (year, album_id)) + # Cache the authoritative expected track count for the Album + # Completeness repair job (see set_album_api_track_count docstring). + set_album_api_track_count(cursor, album_id, getattr(album_obj, 'total_tracks', 0)) + conn.commit() except Exception as e: logger.error(f"Error updating album #{album_id} with iTunes data: {e}") diff --git a/core/repair_jobs/album_completeness.py b/core/repair_jobs/album_completeness.py index 999597dd..c92363f3 100644 --- a/core/repair_jobs/album_completeness.py +++ b/core/repair_jobs/album_completeness.py @@ -7,6 +7,7 @@ from core.metadata_service import ( ) from core.repair_jobs import register_job from core.repair_jobs.base import JobContext, JobResult, RepairJob +from core.worker_utils import set_album_api_track_count from utils.logging_config import get_logger logger = get_logger("repair_job.album_complete") @@ -19,9 +20,10 @@ class AlbumCompletenessJob(RepairJob): description = 'Checks if all tracks from albums are present' help_text = ( 'Compares the number of tracks you have for each album against the expected total ' - 'from the active metadata provider first, then other supported sources if needed. ' - 'Albums where tracks are missing get flagged as findings with details about which ' - 'tracks are absent.\n\n' + 'from your configured metadata sources. Counts cached during normal enrichment are ' + 'used when available; otherwise the job queries a metadata source directly. Albums ' + 'where tracks are missing get flagged as findings with details about which tracks ' + 'are absent.\n\n' 'Useful for catching partial downloads or albums where some tracks failed to download. ' 'You can use the Download Missing feature from the album page to fill gaps.\n\n' 'Settings:\n' @@ -53,6 +55,7 @@ class AlbumCompletenessJob(RepairJob): conn = None has_itunes = False has_deezer = False + has_api_track_count = False try: conn = context.db._get_connection() cursor = conn.cursor() @@ -65,17 +68,31 @@ class AlbumCompletenessJob(RepairJob): has_discogs = 'discogs_id' in columns has_hydrabase = 'soul_id' in columns - # Build SELECT with available source ID columns + # Detect the `api_track_count` column — older DBs may not have it + # yet (migration runs on app start, but repair-job code mustn't + # assume it's present). When absent, fall back to the pre-column + # behavior: look up expected total via API every scan, don't try + # to persist it. + has_api_track_count = 'api_track_count' in columns + + # Build SELECT with available source ID columns. + # NOTE: `al.track_count` is deliberately NOT selected. That + # column holds the OBSERVED track count written by server syncs + # (Plex leafCount, SoulSync standalone len(tracks)) — always + # equal to COUNT(t.id), so it's worthless for completeness. + # The expected total comes from `al.api_track_count` (cached + # from metadata-source enrichment) or a live API lookup. select_cols = [ ('al.id', 'album_id'), ('al.title', 'album_title'), ('ar.name', 'artist_name'), ('al.spotify_album_id', 'spotify_album_id'), - ('al.track_count', 'track_count'), ('COUNT(t.id)', 'actual_count'), ('al.thumb_url', 'album_thumb_url'), ('ar.thumb_url', 'artist_thumb_url'), ] + if has_api_track_count: + select_cols.append(('al.api_track_count', 'api_track_count')) if has_itunes: select_cols.append(('al.itunes_album_id', 'itunes_album_id')) if has_deezer: @@ -135,7 +152,6 @@ class AlbumCompletenessJob(RepairJob): title = row[column_index['album_title']] artist_name = row[column_index['artist_name']] spotify_album_id = row[column_index['spotify_album_id']] - db_track_count = row[column_index['track_count']] actual_count = row[column_index['actual_count']] album_thumb = row[column_index['album_thumb_url']] artist_thumb = row[column_index['artist_thumb_url']] @@ -143,6 +159,9 @@ class AlbumCompletenessJob(RepairJob): deezer_album_id = row[column_index['deezer_album_id']] if 'deezer_album_id' in column_index else None discogs_album_id = row[column_index['discogs_album_id']] if 'discogs_album_id' in column_index else None hydrabase_album_id = row[column_index['hydrabase_album_id']] if 'hydrabase_album_id' in column_index else None + # Cached authoritative track count from a prior API lookup (NULL + # on unscanned albums and on DBs predating the column migration). + cached_api_count = row[column_index['api_track_count']] if 'api_track_count' in column_index else None result.scanned += 1 @@ -154,9 +173,6 @@ class AlbumCompletenessJob(RepairJob): log_type='info' ) - # If we don't know the expected track count, try to get it from an API - expected_total = db_track_count - album_ids = { 'spotify': spotify_album_id or '', 'itunes': itunes_album_id or '', @@ -165,8 +181,20 @@ class AlbumCompletenessJob(RepairJob): 'hydrabase': hydrabase_album_id or '', } + # Expected total comes from the metadata provider, NOT from + # al.track_count — that column holds the observed count from + # server syncs (Plex leafCount, SoulSync standalone len(tracks)) + # which by definition always equals actual_count and made the + # job skip every album. Use the cached api_track_count if a + # prior scan already looked it up; otherwise hit the API and + # persist the answer for next time. + expected_total = cached_api_count if not expected_total: expected_total = self._get_expected_total(context, primary_source, album_ids) + # Only persist positive results. Zero/None would keep + # re-triggering the lookup on every scan. + if expected_total and expected_total > 0 and has_api_track_count: + self._save_api_track_count(context, album_id, expected_total) # Skip singles/EPs based on expected track count (not local count) if expected_total and expected_total < min_tracks: @@ -251,6 +279,27 @@ class AlbumCompletenessJob(RepairJob): result.scanned, result.findings_created) return result + def _save_api_track_count(self, context, album_id, count): + """Persist a metadata-API track count via the shared worker helper. + + Enrichment workers call `set_album_api_track_count` inside their own + `_update_album` transaction. Here we're in the repair job's fallback + path (the album wasn't enriched yet), so we own the connection + + commit ourselves. A cache-write failure must never break the scan, + so all errors are swallowed into the debug log. + """ + conn = None + try: + conn = context.db._get_connection() + cursor = conn.cursor() + set_album_api_track_count(cursor, album_id, count) + conn.commit() + except Exception as e: + logger.debug("Failed to cache api_track_count for album %s: %s", album_id, e) + finally: + if conn: + conn.close() + def _get_expected_total(self, context, primary_source, album_ids): """Try to get the expected track count from the active metadata provider first.""" for source in get_source_priority(primary_source): diff --git a/core/spotify_worker.py b/core/spotify_worker.py index 89a7935b..dde78781 100644 --- a/core/spotify_worker.py +++ b/core/spotify_worker.py @@ -8,7 +8,7 @@ from datetime import datetime, date, timedelta from utils.logging_config import get_logger from database.music_database import MusicDatabase from core.spotify_client import SpotifyClient, SpotifyRateLimitError -from core.worker_utils import interruptible_sleep +from core.worker_utils import interruptible_sleep, set_album_api_track_count logger = get_logger("spotify_worker") @@ -782,6 +782,10 @@ class SpotifyWorker: WHERE id = ? AND (year IS NULL OR year = '' OR year = '0') """, (year, album_id)) + # Cache the authoritative expected track count for the Album + # Completeness repair job (see set_album_api_track_count docstring). + set_album_api_track_count(cursor, album_id, getattr(album_obj, 'total_tracks', 0)) + conn.commit() except Exception as e: logger.error(f"Error updating album #{album_id} with Spotify data: {e}") diff --git a/core/worker_utils.py b/core/worker_utils.py index 0c61fb0a..d6f42523 100644 --- a/core/worker_utils.py +++ b/core/worker_utils.py @@ -1,7 +1,10 @@ """Shared helpers for background workers.""" +import logging import threading +logger = logging.getLogger(__name__) + def interruptible_sleep(stop_event: threading.Event, seconds: float, step: float = 0.5) -> bool: """Sleep in chunks so shutdown can interrupt long waits.""" @@ -15,3 +18,52 @@ def interruptible_sleep(stop_event: threading.Event, seconds: float, step: float break remaining -= wait_for return stop_event.is_set() + + +def set_album_api_track_count(cursor, album_id, count): + """Cache an album's authoritative track count from a metadata source. + + Called by enrichment workers (Spotify / iTunes / Deezer / Discogs) after + they fetch album metadata. The count is the EXPECTED total tracks + according to that source — distinct from `albums.track_count`, which + server syncs (Plex `leafCount`, SoulSync standalone `len(tracks)`) + populate with the OBSERVED count SoulSync already has indexed. The + Album Completeness repair job reads `albums.api_track_count` as the + expected total; populating it here during enrichment avoids a second + round of API calls during the repair scan. + + Skips the write when the source didn't supply a positive numeric count + (None, 0, negative, or non-numeric) — that way a source lacking track + info doesn't overwrite a good value another source already wrote. If + multiple sources report different counts (rare, usually deluxe vs. + standard edition), last-write-wins across enrichment cycles; that's + fine since any metadata-source count is strictly better than the + observed-count fallback that the repair job used before this column + existed. + + Caller owns the cursor (and its connection / transaction) — this + helper does not commit. Integrates with each worker's existing + `_update_album` method, which already batches several UPDATEs into + one transaction. + """ + try: + count = int(count or 0) + except (TypeError, ValueError): + return + if count <= 0: + return + # Swallow SQL errors — each worker batches several album UPDATEs into + # one transaction, and we don't want a failure here (e.g., the + # migration somehow hasn't run yet and the column is missing) to + # rollback the worker's other writes (spotify_album_id, thumb_url, + # etc.). The repair job's fallback path will eventually populate the + # column via its own save path once the column exists. + try: + cursor.execute( + "UPDATE albums SET api_track_count = ? WHERE id = ?", + (count, album_id), + ) + except Exception as e: + logger.warning( + "Failed to cache api_track_count for album %s: %s", album_id, e + ) diff --git a/database/music_database.py b/database/music_database.py index 7b2be55f..22eb7625 100644 --- a/database/music_database.py +++ b/database/music_database.py @@ -3122,6 +3122,19 @@ class MusicDatabase: cursor.execute("ALTER TABLE albums ADD COLUMN soul_id TEXT DEFAULT NULL") logger.info("Added soul_id column to albums table") + # Albums: api_track_count — cached expected track count from the + # metadata provider, separate from track_count which is the + # OBSERVED count written by server syncs (Plex leafCount, + # SoulSync standalone len(tracks)). Without a separate column, + # the Album Completeness job can't tell apart "you have all the + # tracks" from "Plex says this album has N tracks and you have + # N tracks" — the latter looks complete but might be missing + # material the metadata source knows about. NULL = not yet + # looked up; the repair job fills it as it runs. + if 'api_track_count' not in album_cols: + cursor.execute("ALTER TABLE albums ADD COLUMN api_track_count INTEGER DEFAULT NULL") + logger.info("Added api_track_count column to albums table") + # Tracks: soul_id (song-level) + album_soul_id (release-specific) cursor.execute("PRAGMA table_info(tracks)") track_cols = [c[1] for c in cursor.fetchall()] diff --git a/tests/test_album_completeness_job.py b/tests/test_album_completeness_job.py index f3f8145c..5451839f 100644 --- a/tests/test_album_completeness_job.py +++ b/tests/test_album_completeness_job.py @@ -306,3 +306,311 @@ def test_album_completeness_supports_hydrabase_primary(monkeypatch): assert [track["track_number"] for track in missing_tracks] == [2, 3, 4] assert missing_tracks[0]["source"] == "hydrabase" assert missing_tracks[0]["source_track_id"] == "hy-2" + + +# --------------------------------------------------------------------------- +# api_track_count caching — the fix for the "0.1s / 0 findings" bug +# --------------------------------------------------------------------------- + +class _ApiCountCursor: + """Records UPDATE statements so we can verify the cache write.""" + + def __init__(self): + self.updates = [] + + def execute(self, query, params=None): + if query.strip().startswith("UPDATE albums"): + self.updates.append((query, params)) + return self + + def fetchall(self): + return [] + + +class _ApiCountConnection: + def __init__(self, cursor): + self._cursor = cursor + self.commits = 0 + + def cursor(self): + return self._cursor + + def commit(self): + self.commits += 1 + + def close(self): + return None + + +class _ApiCountDB: + def __init__(self): + self.cursor = _ApiCountCursor() + + def _get_connection(self): + return _ApiCountConnection(self.cursor) + + +def test_save_api_track_count_writes_update_to_db(): + """The helper persists the resolved count so subsequent scans don't + refetch the expected total from the API.""" + job = AlbumCompletenessJob() + db = _ApiCountDB() + context = types.SimpleNamespace(db=db) + + job._save_api_track_count(context, "album-42", 12) + + assert len(db.cursor.updates) == 1 + query, params = db.cursor.updates[0] + assert "UPDATE albums" in query + assert "api_track_count = ?" in query + assert params == (12, "album-42") + + +def test_save_api_track_count_swallows_errors(): + """A cache-write failure must not break the scan — the job falls back + to the pre-cache behavior (API call next time).""" + job = AlbumCompletenessJob() + + class _Boom: + def _get_connection(self): + raise RuntimeError("db is gone") + + context = types.SimpleNamespace(db=_Boom()) + # Should not raise. + job._save_api_track_count(context, "album-x", 10) + + +# --------------------------------------------------------------------------- +# Integration tests — run the full scan loop against a real sqlite in-memory +# DB so SELECT/PRAGMA/UPDATE go through actual SQL. Catches wiring mistakes +# between the SELECT, column_index, loop, and finding creation that the +# isolated helper tests wouldn't surface. +# --------------------------------------------------------------------------- + +import sqlite3 +import uuid + + +class _SharedMemoryDB: + """Tiny shim matching MusicDatabase's `_get_connection()` contract, + backed by a shared-cache sqlite in-memory DB so `close()` on a + per-call connection doesn't destroy the data.""" + + def __init__(self): + self.uri = f"file:testdb_{uuid.uuid4().hex}?mode=memory&cache=shared" + # Keepalive conn holds the in-memory DB alive while other conns + # open/close. Without this, the DB is garbage-collected when the + # last conn closes. + self._keepalive = sqlite3.connect(self.uri, uri=True) + self._keepalive.executescript( + """ + CREATE TABLE artists ( + id TEXT PRIMARY KEY, + name TEXT, + thumb_url TEXT + ); + CREATE TABLE albums ( + id TEXT PRIMARY KEY, + artist_id TEXT, + title TEXT, + thumb_url TEXT, + spotify_album_id TEXT, + itunes_album_id TEXT, + deezer_id TEXT, + discogs_id TEXT, + soul_id TEXT, + track_count INTEGER, + api_track_count INTEGER + ); + CREATE TABLE tracks ( + id TEXT PRIMARY KEY, + album_id TEXT, + track_number INTEGER + ); + """ + ) + self._keepalive.commit() + + def _get_connection(self): + return sqlite3.connect(self.uri, uri=True) + + def insert_artist(self, artist_id, name, thumb=None): + self._keepalive.execute( + "INSERT INTO artists (id, name, thumb_url) VALUES (?, ?, ?)", + (artist_id, name, thumb), + ) + self._keepalive.commit() + + def insert_album(self, album_id, artist_id, title, *, spotify_id=None, + track_count=None, api_track_count=None): + self._keepalive.execute( + """INSERT INTO albums + (id, artist_id, title, spotify_album_id, track_count, api_track_count) + VALUES (?, ?, ?, ?, ?, ?)""", + (album_id, artist_id, title, spotify_id, track_count, api_track_count), + ) + self._keepalive.commit() + + def insert_tracks(self, album_id, count): + rows = [(f"{album_id}-t{i}", album_id, i) for i in range(1, count + 1)] + self._keepalive.executemany( + "INSERT INTO tracks (id, album_id, track_number) VALUES (?, ?, ?)", + rows, + ) + self._keepalive.commit() + + def fetch_api_track_count(self, album_id): + row = self._keepalive.execute( + "SELECT api_track_count FROM albums WHERE id = ?", (album_id,) + ).fetchone() + return row[0] if row else None + + +def _make_job_context(db, *, create_finding): + """Minimal JobContext stand-in covering the fields scan() touches.""" + return types.SimpleNamespace( + db=db, + transfer_folder='', + config_manager=_DummyConfigManager(), + spotify_client=None, + is_spotify_rate_limited=lambda: False, + stop_event=None, + create_finding=create_finding, + should_stop=None, + is_paused=None, + update_progress=None, + report_progress=None, + check_stop=lambda: False, + wait_if_paused=lambda: False, + ) + + +def test_scan_uses_cached_api_track_count_without_expected_total_lookup(monkeypatch): + """Integration: when api_track_count is populated, the scan reads the + expected total from the cache. `_get_expected_total` must NOT be called + for albums with a cached value. (The missing-tracks lookup may still + hit the API for incomplete albums — that's a separate call path used + only after we've decided the album is incomplete.)""" + db = _SharedMemoryDB() + db.insert_artist('a1', 'Test Artist') + db.insert_album('alb-incomplete', 'a1', 'Incomplete Album', + spotify_id='sp-1', track_count=10, api_track_count=12) + db.insert_tracks('alb-incomplete', 10) + db.insert_album('alb-complete', 'a1', 'Complete Album', + spotify_id='sp-2', track_count=8, api_track_count=8) + db.insert_tracks('alb-complete', 8) + + # Stub the track-lookup used by _find_missing_tracks (needed for the + # incomplete album's finding details). We're not asserting on it. + monkeypatch.setattr( + album_completeness_module, + "get_album_tracks_for_source", + lambda source, album_id: {"items": [ + {"track_number": i, "name": f"T{i}", "artists": []} for i in range(1, 13) + ]}, + ) + monkeypatch.setattr(album_completeness_module, "get_primary_source", lambda: "spotify") + monkeypatch.setattr( + album_completeness_module, "get_source_priority", + lambda primary: ["spotify", "itunes", "deezer", "discogs", "hydrabase"], + ) + + # Spy on _get_expected_total specifically — that's the call path the + # cache is supposed to short-circuit. + job = AlbumCompletenessJob() + expected_total_calls = [] + original_get_expected = job._get_expected_total + + def spy(context_, primary_, album_ids_): + expected_total_calls.append(album_ids_.get('spotify')) + return original_get_expected(context_, primary_, album_ids_) + job._get_expected_total = spy + + findings = [] + context = _make_job_context(db, create_finding=lambda **kwargs: findings.append(kwargs)) + + result = job.scan(context) + + # _get_expected_total was NOT called — both albums had cached counts. + assert expected_total_calls == [] + # Exactly one finding for the incomplete album. + assert result.findings_created == 1 + assert len(findings) == 1 + finding = findings[0] + assert finding['entity_id'] == 'alb-incomplete' + assert finding['details']['expected_tracks'] == 12 + assert finding['details']['actual_tracks'] == 10 + + +def test_scan_falls_back_to_api_and_persists_count_on_cache_miss(monkeypatch): + """Integration: when api_track_count is NULL, scan calls the API, + gets the expected total, caches it, and creates the finding.""" + db = _SharedMemoryDB() + db.insert_artist('a1', 'Test Artist') + # api_track_count is NULL — will need API lookup + db.insert_album('alb-fresh', 'a1', 'Fresh Album', + spotify_id='sp-fresh', track_count=8, api_track_count=None) + db.insert_tracks('alb-fresh', 8) + + # API returns 14 tracks (so 8 owned out of 14 → finding) + monkeypatch.setattr( + album_completeness_module, + "get_album_tracks_for_source", + lambda source, album_id: { + "items": [{"track_number": i, "name": f"T{i}", "artists": []} for i in range(1, 15)], + } if source == "spotify" and album_id == "sp-fresh" else None, + ) + monkeypatch.setattr(album_completeness_module, "get_primary_source", lambda: "spotify") + monkeypatch.setattr( + album_completeness_module, "get_source_priority", + lambda primary: ["spotify"], + ) + + findings = [] + context = _make_job_context(db, create_finding=lambda **kwargs: findings.append(kwargs)) + + job = AlbumCompletenessJob() + result = job.scan(context) + + # Finding for 8/14. + assert result.findings_created == 1 + finding = findings[0] + assert finding['details']['expected_tracks'] == 14 + assert finding['details']['actual_tracks'] == 8 + # Crucially: the scan persisted the count so the next scan won't refetch. + assert db.fetch_api_track_count('alb-fresh') == 14 + + +def test_scan_ignores_track_count_completely(monkeypatch): + """Regression: the observed `track_count` (Plex's leafCount) must + NOT influence the expected-total comparison. Before the fix, an + album with track_count=actual_count was skipped as 'complete' even + when the metadata source said it had more tracks.""" + db = _SharedMemoryDB() + db.insert_artist('a1', 'Test Artist') + # track_count=10 matches actual=10 (sassmastawillis's bug scenario). + # api_track_count=15 says the album actually has 15 tracks. + db.insert_album('alb-bug', 'a1', 'Bug Reproduction', + spotify_id='sp-bug', track_count=10, api_track_count=15) + db.insert_tracks('alb-bug', 10) + + monkeypatch.setattr( + album_completeness_module, "get_album_tracks_for_source", + lambda source, album_id: None, # should NOT be called + ) + monkeypatch.setattr(album_completeness_module, "get_primary_source", lambda: "spotify") + monkeypatch.setattr( + album_completeness_module, "get_source_priority", + lambda primary: ["spotify"], + ) + + findings = [] + context = _make_job_context(db, create_finding=lambda **kwargs: findings.append(kwargs)) + + job = AlbumCompletenessJob() + result = job.scan(context) + + # The album MUST be flagged (10/15), not silently skipped. + assert result.findings_created == 1 + assert findings[0]['details']['expected_tracks'] == 15 + assert findings[0]['details']['actual_tracks'] == 10 diff --git a/tests/test_worker_utils_album_track_count.py b/tests/test_worker_utils_album_track_count.py new file mode 100644 index 00000000..4b988d70 --- /dev/null +++ b/tests/test_worker_utils_album_track_count.py @@ -0,0 +1,116 @@ +"""Tests for `worker_utils.set_album_api_track_count` — the shared helper +enrichment workers call to cache authoritative track counts.""" + +from core.worker_utils import set_album_api_track_count + + +class _RecordingCursor: + """Minimal cursor stand-in that captures execute() calls.""" + + def __init__(self): + self.calls = [] + + def execute(self, query, params=None): + self.calls.append((query, params)) + + +# --------------------------------------------------------------------------- +# Happy-path writes +# --------------------------------------------------------------------------- + +def test_writes_positive_int_count(): + cursor = _RecordingCursor() + set_album_api_track_count(cursor, "album-1", 12) + assert len(cursor.calls) == 1 + query, params = cursor.calls[0] + assert "UPDATE albums SET api_track_count = ?" in query + assert "WHERE id = ?" in query + assert params == (12, "album-1") + + +def test_coerces_numeric_string_to_int(): + """Deezer / raw API dicts often have track counts as strings.""" + cursor = _RecordingCursor() + set_album_api_track_count(cursor, "album-2", "14") + assert cursor.calls[0][1] == (14, "album-2") + + +def test_writes_one_for_single_track_album(): + cursor = _RecordingCursor() + set_album_api_track_count(cursor, "album-single", 1) + assert cursor.calls[0][1] == (1, "album-single") + + +# --------------------------------------------------------------------------- +# Skip-write cases (don't overwrite good values with bad ones) +# --------------------------------------------------------------------------- + +def test_skips_write_when_count_is_zero(): + """A source that doesn't report track counts must not clobber a value + written by another source.""" + cursor = _RecordingCursor() + set_album_api_track_count(cursor, "album-x", 0) + assert cursor.calls == [] + + +def test_skips_write_when_count_is_none(): + cursor = _RecordingCursor() + set_album_api_track_count(cursor, "album-x", None) + assert cursor.calls == [] + + +def test_skips_write_when_count_is_negative(): + cursor = _RecordingCursor() + set_album_api_track_count(cursor, "album-x", -1) + assert cursor.calls == [] + + +def test_skips_write_on_non_numeric_string(): + cursor = _RecordingCursor() + set_album_api_track_count(cursor, "album-x", "not a number") + assert cursor.calls == [] + + +def test_skips_write_on_non_numeric_object(): + cursor = _RecordingCursor() + set_album_api_track_count(cursor, "album-x", object()) + assert cursor.calls == [] + + +# --------------------------------------------------------------------------- +# Does not commit — caller owns the transaction +# --------------------------------------------------------------------------- + +def test_helper_does_not_commit(): + """Workers batch multiple UPDATEs into one transaction. The helper + must not call commit() or it would break that batching.""" + + class _StrictCursor(_RecordingCursor): + commits = 0 + + def commit(self): # pragma: no cover — asserts it's never called + _StrictCursor.commits += 1 + + cursor = _StrictCursor() + set_album_api_track_count(cursor, "album-y", 5) + assert _StrictCursor.commits == 0 + + +# --------------------------------------------------------------------------- +# Error isolation — a cursor.execute failure must not poison the worker's +# other UPDATEs in the same transaction +# --------------------------------------------------------------------------- + +def test_swallows_cursor_execute_errors(): + """If the column doesn't exist yet (e.g., migration hasn't run) or + the DB is otherwise unhappy, the helper must not propagate the error. + Otherwise the worker's other UPDATEs (spotify_album_id, thumb_url, + etc.) batched in the same transaction would roll back.""" + + class _BrokenCursor: + def execute(self, query, params=None): + raise RuntimeError("no such column: api_track_count") + + cursor = _BrokenCursor() + # Should not raise. + set_album_api_track_count(cursor, "album-z", 10) diff --git a/webui/static/helper.js b/webui/static/helper.js index ff4a7871..e9f6bba9 100644 --- a/webui/static/helper.js +++ b/webui/static/helper.js @@ -3443,6 +3443,7 @@ const WHATS_NEW = { '2.40': [ // --- Search & Artists unification (in progress, not yet released) --- { date: 'Unreleased — Search & Artists unification', unreleased: true }, + { title: 'Fix Album Completeness Job Reporting Zero Findings for Everyone', desc: 'sassmastawillis reported the Album Completeness maintenance job was finishing in 0.1s with 0 findings, even for users with obviously-incomplete albums. Root cause: the job used `albums.track_count` as the "expected total" to compare against the library\'s actual count. But `track_count` is populated by server syncs (Plex leafCount, SoulSync standalone len(tracks)) — it\'s always the OBSERVED count, never what the metadata provider says the album should contain. So expected == actual always, and every album looked complete. Fix: new `api_track_count` column on the albums table, written only by metadata-source code paths (Spotify, iTunes, Deezer, and Discogs enrichment workers now populate it whenever they fetch album data, so it piggybacks on existing API calls instead of making new ones). Server syncs never touch this column, so it stays authoritative. The repair job uses it as the expected total; if an album somehow hasn\'t been enriched yet, the job falls back to a live API lookup and caches the result. For users with an already-enriched library, the first completeness scan after the upgrade is fast because the workers will have populated the column during normal enrichment cycles', page: 'library', unreleased: true }, { title: 'Search Source Picker Icon Row', desc: 'The Search page now has a row of source icons above the search bar — one per source (Spotify, Apple Music, Deezer, Discogs, Hydrabase, MusicBrainz, Music Videos, Soulseek). Typing searches only the currently-selected source instead of fanning out to every one by default. Click a different icon to switch; results come back on demand. The default icon on page load is your configured primary metadata source. Replaces the short-lived "Search from" dropdown that preceded this', page: 'search', unreleased: true }, { title: 'Per-Query Source Cache (No More Re-Fetching)', desc: 'Once you\'ve searched a source for a given query, switching back to it is instant — results are cached for the current query. A small dot on each source icon shows which ones already have cached results this query. Type a new query and the whole cache resets. Same behavior in the sidebar global search popover. Net effect: roughly 6-7x fewer API calls per search compared to the old default fan-out', page: 'search', unreleased: true }, { title: 'Global Search Widget Source Parity', desc: 'The sidebar Cmd+K / "/" search popover gained the same source icon row as the full Search page. Pick your source up front, see cache dots for already-fetched sources this query, and the rate-limit fallback banner appears if the backend substituted a different source than the one you clicked. Clicking the Soulseek icon hands off to the full Search page (raw file results need more room than the popover provides)', page: 'search', unreleased: true }, From 6c90d68de3dc1ad8f9c20265760a7a7414007b49 Mon Sep 17 00:00:00 2001 From: Broque Thomas <26755000+Nezreka@users.noreply.github.com> Date: Sat, 25 Apr 2026 08:27:44 -0700 Subject: [PATCH 2/3] Discogs: count rows with empty type_ as real tracks too MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reported by kettui on PR #374 review: the inline filter that backed `set_album_api_track_count` only counted rows where `type_ == 'track'`, but `discogs_client.get_album_tracks` itself accepts both `'track'` AND empty `type_` as real songs (line 660: `type_ in ('track', '')`). Releases where Discogs returns some real tracks with an empty `type_` field would be undercounted, which would silently disagree with the repair job's fallback `_get_expected_total` path (which calls into `get_album_tracks_for_source` and therefore uses the client's count). Extracted the filter into `count_discogs_real_tracks(tracklist)` — single source of truth for the rule, testable in isolation, and the worker call site is now a one-liner that names what it's doing. Also defensive about the input shape: `type_ == None`, missing field, and empty/None tracklist all handled cleanly. 10 tests pin the behavior: - empty/missing/None type_ all count as a real track (the kettui case) - 'heading', 'index', 'sub_track' excluded - unknown future type strings excluded conservatively - realistic multi-disc tracklist with mixed shapes counts correctly - empty/None input returns 0 without raising Credit: kettui — the PR #374 review comment that flagged this. --- core/discogs_worker.py | 33 ++++++-- tests/test_discogs_track_count.py | 129 ++++++++++++++++++++++++++++++ 2 files changed, 156 insertions(+), 6 deletions(-) create mode 100644 tests/test_discogs_track_count.py diff --git a/core/discogs_worker.py b/core/discogs_worker.py index 1b124a22..84270de9 100644 --- a/core/discogs_worker.py +++ b/core/discogs_worker.py @@ -23,6 +23,29 @@ from core.worker_utils import interruptible_sleep, set_album_api_track_count logger = get_logger("discogs_worker") +def count_discogs_real_tracks(tracklist) -> int: + """Count actual songs in a Discogs tracklist response. + + Discogs tracklists interleave real tracks with section headings + (``type_=='heading'``), index markers (``type_=='index'``), + and sub-tracks (``type_=='sub_track'``) that aren't themselves + songs. We count anything that's explicitly typed as ``'track'`` OR + has an empty/missing ``type_`` field — matching exactly what + :meth:`core.discogs_client.DiscogsClient.get_album_tracks` itself + treats as a real track (`type_ in ('track', '')`). Counting any + narrower set silently disagrees with the repair job's fallback + `_get_expected_total` path, which calls `get_album_tracks_for_source` + under the hood and therefore uses the client's count. + + Reported by kettui on PR #374 — original filter only counted + ``type_=='track'`` and undercounted releases where the discogs + response left ``type_`` empty for some real tracks. + """ + if not tracklist: + return 0 + return sum(1 for t in tracklist if (t.get('type_') or '') in ('track', '')) + + class DiscogsWorker: """Background worker for enriching library artists and albums with Discogs metadata.""" @@ -455,15 +478,13 @@ class DiscogsWorker: """, (image_url, album_id)) # Cache the authoritative expected track count for the Album - # Completeness repair job. Discogs tracklists mix actual tracks - # with section headings and index entries — only type_=='track' - # rows are real songs, so filter before counting. (Helper can't - # do this filter for us; it's Discogs-specific shape.) - tracklist = data.get('tracklist') or [] + # Completeness repair job. See `count_discogs_real_tracks` + # for why we accept both `type_ == 'track'` and empty `type_` + # (kettui's PR #374 review — narrower filter undercounted). set_album_api_track_count( cursor, album_id, - sum(1 for t in tracklist if t.get('type_') == 'track'), + count_discogs_real_tracks(data.get('tracklist')), ) conn.commit() diff --git a/tests/test_discogs_track_count.py b/tests/test_discogs_track_count.py new file mode 100644 index 00000000..0ad04986 --- /dev/null +++ b/tests/test_discogs_track_count.py @@ -0,0 +1,129 @@ +"""Tests for `discogs_worker.count_discogs_real_tracks` — the filter +that decides which entries in a Discogs tracklist count as real songs +when caching the authoritative track count for the Album Completeness +repair job. + +Reported by kettui on PR #374: the original inline filter only kept +``type_ == 'track'`` rows, but `discogs_client.get_album_tracks` itself +keeps both ``type_ == 'track'`` AND rows with an empty/missing +``type_``. The narrower filter would undercount releases whose Discogs +response left ``type_`` blank for some real tracks — and the repair +job's fallback path (`_get_expected_total`) would silently disagree +with the cached count. +""" + +from core.discogs_worker import count_discogs_real_tracks + + +# --------------------------------------------------------------------------- +# The kettui case: empty type_ counts as a real track +# --------------------------------------------------------------------------- + +def test_empty_type_counts_as_track(): + tracklist = [ + {'title': 'Track 1', 'type_': 'track'}, + {'title': 'Track 2', 'type_': ''}, # <-- the bug + {'title': 'Track 3', 'type_': 'track'}, + ] + assert count_discogs_real_tracks(tracklist) == 3 + + +def test_missing_type_field_counts_as_track(): + """Discogs sometimes omits the field entirely rather than sending + an empty string. Both shapes mean 'real track'.""" + tracklist = [ + {'title': 'Track 1'}, # no type_ key at all + {'title': 'Track 2', 'type_': 'track'}, + ] + assert count_discogs_real_tracks(tracklist) == 2 + + +def test_none_type_counts_as_track(): + """And the field may be present-but-None on some clients/versions.""" + tracklist = [ + {'title': 'Track 1', 'type_': None}, + {'title': 'Track 2', 'type_': 'track'}, + ] + assert count_discogs_real_tracks(tracklist) == 2 + + +# --------------------------------------------------------------------------- +# Non-track rows are excluded (Discogs's structural markers) +# --------------------------------------------------------------------------- + +def test_headings_excluded(): + tracklist = [ + {'title': 'Disc 1', 'type_': 'heading'}, + {'title': 'Track 1', 'type_': 'track'}, + {'title': 'Track 2', 'type_': 'track'}, + {'title': 'Disc 2', 'type_': 'heading'}, + {'title': 'Track 3', 'type_': 'track'}, + ] + assert count_discogs_real_tracks(tracklist) == 3 + + +def test_indices_excluded(): + tracklist = [ + {'title': 'Side A', 'type_': 'index'}, + {'title': 'Track 1', 'type_': 'track'}, + {'title': 'Side B', 'type_': 'index'}, + {'title': 'Track 2', 'type_': 'track'}, + ] + assert count_discogs_real_tracks(tracklist) == 2 + + +def test_sub_tracks_excluded(): + """Sub-tracks (parts of a medley) shouldn't double-count against + the parent track.""" + tracklist = [ + {'title': 'Medley', 'type_': 'track'}, + {'title': 'Part A', 'type_': 'sub_track'}, + {'title': 'Part B', 'type_': 'sub_track'}, + {'title': 'Encore', 'type_': 'track'}, + ] + assert count_discogs_real_tracks(tracklist) == 2 + + +def test_unknown_type_excluded(): + """Conservative — if Discogs adds a new structural marker we + haven't seen, don't count it as a track until we explicitly add + it to the allowlist.""" + tracklist = [ + {'title': 'Track 1', 'type_': 'track'}, + {'title': 'Some Future Marker', 'type_': 'experimental_thing'}, + ] + assert count_discogs_real_tracks(tracklist) == 1 + + +# --------------------------------------------------------------------------- +# Defensive — bad input shouldn't raise +# --------------------------------------------------------------------------- + +def test_empty_tracklist_returns_zero(): + assert count_discogs_real_tracks([]) == 0 + + +def test_none_tracklist_returns_zero(): + assert count_discogs_real_tracks(None) == 0 + + +# --------------------------------------------------------------------------- +# Realistic mixed tracklist (the kettui case in context) +# --------------------------------------------------------------------------- + +def test_realistic_multi_disc_tracklist(): + """A 2-disc release with headings, real tracks, AND a few rows + with empty type_ — should count all real tracks but no markers.""" + tracklist = [ + {'title': 'Disc 1', 'type_': 'heading'}, + {'title': 'Track 1', 'type_': 'track'}, + {'title': 'Track 2', 'type_': 'track'}, + {'title': 'Track 3', 'type_': ''}, # the kettui case + {'title': 'Track 4', 'type_': 'track'}, + {'title': 'Disc 2', 'type_': 'heading'}, + {'title': 'Track 5', 'type_': 'track'}, + {'title': 'Track 6'}, # missing type_ + {'title': 'Bonus Index', 'type_': 'index'}, + {'title': 'Track 7', 'type_': 'track'}, + ] + assert count_discogs_real_tracks(tracklist) == 7 From 751b19c7b1b2fdc279cb66b25bbea0ff7e847903 Mon Sep 17 00:00:00 2001 From: Broque Thomas <26755000+Nezreka@users.noreply.github.com> Date: Sat, 25 Apr 2026 08:31:30 -0700 Subject: [PATCH 3/3] Preserve api_track_count across Plex ratingKey rekeys MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reported by kettui on PR #374 review: > api_track_count is not copied during the ratingKey migration, so > the cache disappears when an album row is rekeyed. Add it to > enrichment_cols or the next completeness scan will fall back to > live API lookups again. When Plex changes an album's ratingKey (after a library rescan), the sync code rekeys the album row by inserting a new row at the new ID and copying enrichment columns from the old row. The list of columns to copy did not include `api_track_count`, so the cached authoritative track count was lost on rekey — and the next completeness scan would hit the fallback path that calls back out to the metadata source's API. Defeats the cache. Added `api_track_count` to the album-level `enrichment_cols` at `music_database.py:4724`. The artist-level lists at lines 4238 and 4554 don't need updating — those are for artist rekeys and don't carry album-scoped fields. No new test — existing migration code has no test infrastructure and writing a Plex-mocked one is larger than this fix. Cin will say if he wants test coverage in his next review pass. Credit: kettui — PR #374 review comment that flagged the missing column in the rekey allowlist. --- database/music_database.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/database/music_database.py b/database/music_database.py index 22eb7625..ab014696 100644 --- a/database/music_database.py +++ b/database/music_database.py @@ -4728,6 +4728,10 @@ class MusicDatabase: 'audiodb_id', 'audiodb_match_status', 'audiodb_last_attempted', 'style', 'mood', 'label', 'explicit', 'record_type', 'deezer_id', 'deezer_match_status', 'deezer_last_attempted', + # api_track_count is metadata-source-derived enrichment cache; + # losing it on a ratingKey rekey would force the next + # completeness scan back to live API lookups (kettui PR #374). + 'api_track_count', ] # Read enrichment data from old album