Merge remote-tracking branch 'origin/dev' into fix/reorganize-via-post-process-pipeline
# Conflicts: # webui/static/helper.js
This commit is contained in:
Broque Thomas
commit
98c85f928e
11 changed files with 737 additions and 13 deletions
|
|
@ -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:
|
||||
|
|
|
|||
|
|
@ -18,11 +18,34 @@ 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")
|
||||
|
||||
|
||||
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."""
|
||||
|
||||
|
|
@ -454,6 +477,16 @@ 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. 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,
|
||||
count_discogs_real_tracks(data.get('tracklist')),
|
||||
)
|
||||
|
||||
conn.commit()
|
||||
|
||||
except Exception as e:
|
||||
|
|
|
|||
|
|
@ -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}")
|
||||
|
|
|
|||
|
|
@ -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):
|
||||
|
|
|
|||
|
|
@ -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}")
|
||||
|
|
|
|||
|
|
@ -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
|
||||
)
|
||||
|
|
|
|||
|
|
@ -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()]
|
||||
|
|
@ -4715,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
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
129
tests/test_discogs_track_count.py
Normal file
129
tests/test_discogs_track_count.py
Normal file
|
|
@ -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
|
||||
116
tests/test_worker_utils_album_track_count.py
Normal file
116
tests/test_worker_utils_album_track_count.py
Normal file
|
|
@ -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)
|
||||
|
|
@ -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: 'Library Reorganize: Reroute Through the Download Pipeline', desc: 'Reported by winecountrygames — using "Reorganize All" on a 3-disc Aerosmith deluxe collapsed it to a flat 1-disc layout, and on other albums it left half the tracks in their original location with no error or count of what was skipped. Root cause: the reorganize endpoint reinvented several wheels (its own template engine, its own disc-number resolution from file tags, its own sidecar sweep, its own collision detection) and each had drifted from the canonical post-processing path used by downloads. The reorganize-only logic read disc_number from file tags and silently defaulted to 1 on any failure, so a single tag-less file collapsed the whole album to single-disc. Tracks whose file paths didn\'t resolve on disk were silently skipped. Rewrote it to follow the import page\'s pattern: copy each file to a per-album staging folder under your download path, look up the canonical tracklist from your configured metadata source (Deezer / Spotify / iTunes / Discogs / Hydrabase) using the album\'s stored source IDs, then route each file through the same `_post_process_matched_download` function fresh downloads use — same template, same tagging, same multi-disc subfolder logic, same sidecar handling, same AcoustID verification. Albums with no stored source ID are reported back and skipped entirely (degrading silently to file tags is what caused the original bug). Tracks not in the source\'s catalog version (bonus tracks on a deluxe edition) are reported as skipped and left in place rather than force-fed wrong context. Files that don\'t resolve on disk are surfaced with the offending DB path so the UI can show them. The 230-line inline reorganize logic in web_server.py was extracted into core/library_reorganize.py — net -195 lines from the monolith, +13 unit tests for the new orchestrator. Frontend behavior change: the per-call template parameter in the reorganize modal is now ignored — reorganize uses your configured download template, matching the pipeline downloads use', page: 'library', unreleased: true },
|
||||
{ title: 'Spotify: Longer Post-Ban Cooldown (30 min)', desc: 'A user reported their Spotify rate-limit ban expired after 4 hours, the system ran its 5-minute post-ban cooldown, and then 32 seconds after the cooldown ended a single get_artist_albums call from a background worker was hit with another 4-hour ban. Diagnosis: Spotify\'s server-side memory of the previous offense outlasted our 5-minute cooldown, so the very first call after cooldown got slapped immediately. The cooldown exists specifically to prevent the "ban expires → we probe → re-ban" cycle, but the value was too short. Bumped from 5 minutes to 30 minutes — same mechanism, just enough room for Spotify to actually forget. A more principled follow-up (adaptive cooldown that scales with the previous ban size, plus making the first post-cooldown call a single light probe rather than allowing background workers through) is documented as a future PR if reports persist after this bump', page: 'dashboard', unreleased: true },
|
||||
{ title: 'Tidal: Reject Silent Quality Downgrades', desc: 'Netti93 reported that with Tidal set to "HiRes only" and quality fallback disabled, tracks were still downloading successfully — as m4a 320kbps files. Root cause: Tidal\'s API silently serves whatever tier your account + the track + your region permits. Ask for HI_RES_LOSSLESS on a track that\'s only in LOW_320K and Tidal returns the AAC stream without raising. The downloader wrote the m4a to disk, the filesize cleared the 100KB stub threshold, and the download reported success. The worker-level fallback chain (hires → lossless → high → low) also never got a chance to advance, because every tier "succeeded" at the first one that returned anything. Fix: after getting the stream, compare stream.audio_quality against what we requested using a rank-based tier comparison (LOW < HIGH < LOSSLESS < HI_RES < HI_RES_LOSSLESS). Same tier or better = accept (so occasional Tidal upgrades don\'t get thrown away). Lower tier = treat this tier as failed, which lets the fallback chain advance when fallback is enabled or fails the whole download honestly when the user has "HiRes only, no fallback" configured. Unrecognized audioQuality values (a new Tidal tier we haven\'t mapped yet) are rejected conservatively so the final diagnostic log can name the unknown value. Older tidalapi builds without the audio_quality attribute fall through to the pre-existing codec / file-size guards so nothing regresses', page: 'downloads', unreleased: true },
|
||||
|
|
|
|||
Loading…
Reference in a new issue