diff --git a/config/settings.py b/config/settings.py index 78b019fc..aa10bff5 100644 --- a/config/settings.py +++ b/config/settings.py @@ -605,7 +605,16 @@ class ConfigManager: "metadata_enhancement": { "enabled": True, "embed_album_art": True, - "post_process_order": ["musicbrainz", "deezer", "audiodb", "tidal", "qobuz", "lastfm", "genius"] + "post_process_order": ["musicbrainz", "deezer", "audiodb", "tidal", "qobuz", "lastfm", "genius"], + # Ordered preferred cover-art sources (empty = use the + # download's own art, i.e. today's behavior). Resolved + walked + # with fallback by core/metadata/art_sources.py. + "album_art_order": [], + # Minimum cover-art resolution (shortest side, px). A preferred + # source whose art is smaller is skipped so the next source is + # tried — stops a low-res Cover Art Archive upload from winning. + # 0 disables the size gate. + "min_art_size": 1000 }, "musicbrainz": { "embed_tags": True diff --git a/core/acoustid_client.py b/core/acoustid_client.py index b11c5960..a97f149a 100644 --- a/core/acoustid_client.py +++ b/core/acoustid_client.py @@ -282,8 +282,9 @@ class AcoustIDClient: def test_api_key(self) -> Tuple[bool, str]: """ - Validate the API key by fingerprinting a real audio file and looking it up. - Falls back to a direct API call if no audio files are available. + Validate the API key with a direct AcoustID lookup call. An invalid key + is reported as invalid (error code 4); any other error means the key was + accepted. Returns: Tuple of (success, message) @@ -294,24 +295,12 @@ class AcoustIDClient: import requests try: - # Try to find a real audio file to fingerprint for an end-to-end test - test_file = self._find_test_audio_file() - - if test_file and CHROMAPRINT_AVAILABLE: - logger.info(f"Testing API key with real audio file: {test_file}") - try: - result = self.fingerprint_and_lookup(test_file) - # If we get here without exception, the API key is valid - # (invalid keys raise or return error before results) - return True, "AcoustID API key is valid" - except Exception as e: - error_str = str(e).lower() - if 'invalid' in error_str and 'api' in error_str: - return False, "Invalid AcoustID API key - get one from https://acoustid.org/new-application" - # Fingerprint/lookup failed for non-key reasons, fall through to direct test - logger.warning(f"Real file test failed ({e}), trying direct API call") - - # Fallback: direct API call with minimal fingerprint + # Authoritative key check: a direct API lookup with a dummy + # fingerprint. AcoustID validates the client key first, so an + # invalid key returns error code 4 regardless of the fingerprint. + # (The previous real-file path trusted "no exception = valid", but + # fingerprint_and_lookup swallows the invalid-key error and returns + # None — so it reported broken keys as valid. #756-adjacent.) url = 'https://api.acoustid.org/v2/lookup' params = { 'client': self.api_key, @@ -326,7 +315,6 @@ class AcoustIDClient: if data.get('status') == 'error': error = data.get('error', {}) error_code = error.get('code', 0) - error_msg = error.get('message', 'Unknown error') # Error code 4 is specifically "invalid API key" if error_code == 4: @@ -346,33 +334,33 @@ class AcoustIDClient: logger.error(f"Error testing AcoustID API key: {e}") return False, f"Error: {str(e)}" - def fingerprint_and_lookup(self, audio_file: str) -> Optional[Dict[str, Any]]: - """ - Generate fingerprint and look up recording in AcoustID. + def lookup_with_status(self, audio_file: str) -> Dict[str, Any]: + """Fingerprint + AcoustID lookup returning a STRUCTURED result. - This is the main method - combines fingerprinting and lookup in one call. + Unlike fingerprint_and_lookup() (which collapses every outcome into + dict-or-None), this distinguishes a genuine no-match from an actual + error — an invalid API key, rate limit, missing chromaprint, or a + fingerprint failure. That distinction is what lets the UI show "AcoustID + Error" (something is broken — fix it) instead of a benign-looking + "Skipped" that silently hides a dead key. - Args: - audio_file: Path to the audio file - - Returns: - Dict with: - 'recordings': list of dicts with 'mbid', 'title', 'artist', 'score' - 'best_score': float (highest score across all results) - 'recording_mbids': list of unique MBIDs (for backward compat) - Or None on error. + Returns dict with: + 'status': 'ok' | 'no_match' | 'error' | 'no_backend' + | 'fingerprint_error' | 'unsupported' | 'unavailable' + | 'not_found' + 'recordings': list (meaningful only for 'ok') + 'best_score': float + 'recording_mbids': list + 'error': human-readable detail for any non-'ok' status + 'invalid_key': bool (True when the API specifically rejected the key) """ if not ACOUSTID_AVAILABLE: - logger.debug("Cannot lookup: pyacoustid not available") - return None - + return {'status': 'unavailable', 'recordings': [], 'error': 'pyacoustid library not installed'} if not self.api_key: - logger.debug("Cannot lookup: no API key") - return None - + return {'status': 'unavailable', 'recordings': [], 'error': 'No AcoustID API key configured'} if not os.path.isfile(audio_file): logger.warning(f"Cannot lookup: file not found: {audio_file}") - return None + return {'status': 'not_found', 'recordings': [], 'error': f'File not found: {audio_file}'} # Check channel count — chromaprint crashes (SIGABRT) on >2 channel files (e.g. 5.1 surround) try: @@ -382,7 +370,8 @@ class AcoustIDClient: channels = getattr(mf.info, 'channels', 2) if channels and channels > 2: logger.warning(f"Skipping AcoustID: file has {channels} channels (surround audio): {audio_file}") - return None + return {'status': 'unsupported', 'recordings': [], + 'error': f'{channels}-channel (surround) audio not supported by chromaprint'} except Exception as e: logger.debug(f"Could not check channel count, proceeding anyway: {e}") @@ -392,17 +381,12 @@ class AcoustIDClient: api_key_preview = f"{self.api_key[:8]}..." if self.api_key and len(self.api_key) > 8 else "NOT SET" logger.info(f"Fingerprinting and looking up: {audio_file} (API key: {api_key_preview})") - # Use match() which handles fingerprinting + lookup + parsing logger.debug("Running acoustid.match()...") recordings = [] seen_mbids = set() best_score = 0.0 - for result in acoustid.match( - self.api_key, - audio_file, - parse=True - ): + for result in acoustid.match(self.api_key, audio_file, parse=True): # match() with parse=True returns (score, recording_id, title, artist) if not isinstance(result, tuple) or len(result) < 2: logger.warning(f"Unexpected result format: {result}") @@ -420,45 +404,57 @@ class AcoustIDClient: if recording_id and recording_id not in seen_mbids: seen_mbids.add(recording_id) - recordings.append({ - 'mbid': recording_id, - 'title': title, - 'artist': artist, - 'score': score, - }) + recordings.append({'mbid': recording_id, 'title': title, 'artist': artist, 'score': score}) logger.debug(f"Found match: {title} by {artist} (MBID: {recording_id}, score: {score})") if not recordings: logger.info(f"No AcoustID matches found for: {audio_file}") - return None + return {'status': 'no_match', 'recordings': [], 'best_score': best_score, + 'recording_mbids': [], 'error': 'Track not found in AcoustID database'} logger.info(f"AcoustID found {len(recordings)} recording(s) (best score: {best_score:.2f})") - return { - 'recordings': recordings, - 'best_score': best_score, - 'recording_mbids': list(seen_mbids), - } + return {'status': 'ok', 'recordings': recordings, 'best_score': best_score, + 'recording_mbids': list(seen_mbids)} except acoustid.NoBackendError: logger.error("Chromaprint library not found and fpcalc not available") - return None + return {'status': 'no_backend', 'recordings': [], + 'error': 'Chromaprint/fpcalc not installed (install libchromaprint1)'} except acoustid.FingerprintGenerationError as e: logger.warning(f"Failed to fingerprint {audio_file}: {e}") - return None + return {'status': 'fingerprint_error', 'recordings': [], 'error': f'Could not fingerprint file: {e}'} except acoustid.WebServiceError as e: - # Log more details about the API error api_key_preview = f"{self.api_key[:8]}..." if self.api_key and len(self.api_key) > 8 else "???" logger.warning(f"AcoustID API error (key: {api_key_preview}): {e}") - # Check for common errors error_str = str(e).lower() - if 'invalid' in error_str or 'unknown' in error_str: - logger.error("API key appears to be invalid - check your AcoustID settings") + # Old pyacoustid reports an invalid key as the bare "status: error" + # (it drops the detail), so treat that as an invalid-key signal too. + invalid = ('invalid' in error_str or 'unknown' in error_str or 'status: error' in error_str) + if invalid: + logger.error("AcoustID API key appears to be invalid — check your AcoustID settings") elif 'rate' in error_str or 'limit' in error_str: - logger.warning("Rate limited by AcoustID - will retry later") - return None + logger.warning("Rate limited by AcoustID — will retry later") + return {'status': 'error', 'recordings': [], 'invalid_key': invalid, + 'error': f'AcoustID API error: {e}'} except Exception as e: logger.error(f"Unexpected error in AcoustID lookup: {e}", exc_info=True) - return None + return {'status': 'error', 'recordings': [], 'error': f'Unexpected error: {e}'} + + def fingerprint_and_lookup(self, audio_file: str) -> Optional[Dict[str, Any]]: + """Legacy dict-or-None lookup. Returns the recordings dict on a confirmed + match, else None. Kept for callers that only need "did we identify it" + (library scanner, auto-import). Callers that must report WHY a lookup + didn't match (verification badge, key test) should use + ``lookup_with_status`` so an error isn't mistaken for a no-match. + """ + res = self.lookup_with_status(audio_file) + if res.get('status') == 'ok': + return { + 'recordings': res['recordings'], + 'best_score': res.get('best_score', 0.0), + 'recording_mbids': res.get('recording_mbids', []), + } + return None def refresh_config(self): """Refresh cached config values (call after settings change).""" diff --git a/core/acoustid_verification.py b/core/acoustid_verification.py index d1b87918..24930ec0 100644 --- a/core/acoustid_verification.py +++ b/core/acoustid_verification.py @@ -50,8 +50,9 @@ class VerificationResult(Enum): """Possible outcomes of audio verification.""" PASS = "pass" # Title/artist match - file is correct FAIL = "fail" # Title/artist mismatch - wrong file downloaded - SKIP = "skip" # Could not verify (error or unavailable) - continue normally + SKIP = "skip" # Genuinely couldn't verify (no match in DB) - continue normally DISABLED = "disabled" # Verification not enabled + ERROR = "error" # Lookup errored (invalid key / rate limit / no backend) - continue, but flag it def _normalize(text: str) -> str: @@ -399,18 +400,33 @@ class AcoustIDVerification: logger.debug(f"AcoustID verification skipped: {reason}") return VerificationResult.SKIP, reason - # Step 2: Fingerprint and lookup in AcoustID + # Step 2: Fingerprint and lookup in AcoustID (structured so an + # actual error — invalid key / rate limit / no chromaprint — is + # reported distinctly from a genuine no-match, instead of both + # silently surfacing as "Skipped"). logger.info(f"Fingerprinting and looking up: {audio_file_path}") - acoustid_result = self.acoustid_client.fingerprint_and_lookup(audio_file_path) + lookup = self.acoustid_client.lookup_with_status(audio_file_path) or {} + status = lookup.get('status') + # Infer status by content when absent (a caller/stub that returned + # just recordings): recordings => matched, none => no match. + if status is None: + status = 'ok' if lookup.get('recordings') else 'no_match' - if not acoustid_result: - return VerificationResult.SKIP, "Track not found in AcoustID database" + if status in ('error', 'no_backend', 'fingerprint_error', 'unavailable'): + # Something is broken (not the track's fault) — never quarantine + # on this; surface it so the user can fix it. + return VerificationResult.ERROR, lookup.get('error', 'AcoustID lookup failed') + if status != 'ok': + # no_match / unsupported / not_found — genuinely could not verify. + return VerificationResult.SKIP, lookup.get('error', 'No match in AcoustID database') + + acoustid_result = lookup recordings = acoustid_result.get('recordings', []) best_score = acoustid_result.get('best_score', 0) if not recordings: - return VerificationResult.SKIP, "AcoustID returned no recordings" + return VerificationResult.SKIP, "No match in AcoustID database" logger.debug( f"AcoustID returned {len(recordings)} recording(s) " diff --git a/core/amazon_client.py b/core/amazon_client.py index 2414051e..5d61daf2 100644 --- a/core/amazon_client.py +++ b/core/amazon_client.py @@ -70,7 +70,16 @@ _meta_cache_lock = threading.Lock() class AmazonClientError(RuntimeError): - """Raised on unrecoverable T2Tunes API errors.""" + """Raised on unrecoverable T2Tunes API errors. + + Carries the HTTP ``status_code`` when the failure was an HTTP error, so + callers (the worker's outage detection) can tell a source outage (5xx) from + a per-item miss without parsing the message. + """ + + def __init__(self, *args, status_code=None): + super().__init__(*args) + self.status_code = status_code # --------------------------------------------------------------------------- @@ -703,7 +712,8 @@ class AmazonClient: ) continue raise AmazonClientError( - f"HTTP {exc.response.status_code} for {url} — body: {body!r}" + f"HTTP {exc.response.status_code} for {url} — body: {body!r}", + status_code=exc.response.status_code, ) from exc except requests.RequestException as exc: raise AmazonClientError(f"Request failed for {url}: {exc}") from exc diff --git a/core/amazon_outage.py b/core/amazon_outage.py new file mode 100644 index 00000000..b8205464 --- /dev/null +++ b/core/amazon_outage.py @@ -0,0 +1,61 @@ +"""Amazon enrichment outage detection + back-off — pure, importable, testable. + +The Amazon worker enriches via a public T2Tunes proxy instance. When that +instance is down (HTTP 5xx, "Amazon Music API is not initialized", or an +unreachable host), the worker must NOT treat every album as an individual +failure: doing so floods the logs with an error per item, churns network + DB +continuously, and permanently marks the whole library ``error`` (which the +retry tiers never re-attempt) for what is really a transient outage. + +Instead it recognizes "the whole source is down", leaves the item untouched so +it's retried once the instance recovers, and backs off hard. These two pure +helpers carry that logic so it can be unit-tested without the worker, the DB, +or the network. +""" + +from __future__ import annotations + +import re + +# HTTP statuses that mean "the source/proxy is unhealthy", not "no match". +_OUTAGE_STATUS = {500, 502, 503, 504} + +# Substrings (lower-cased) in an error message that indicate a source outage +# rather than a per-item miss: proxy not ready, gateway errors, the host being +# unreachable, or an error page returned instead of JSON. +_OUTAGE_PHRASES = ( + "not initialized", "not configured", "service unavailable", + "bad gateway", "gateway time", "request failed", "response not json", + "max retries", "connection", "timed out", "temporarily unavailable", +) + +# Back-off schedule while the source is down. +_NORMAL_DELAY = 2 # seconds between items when healthy +_OUTAGE_BASE = 30 # first back-off step +_OUTAGE_CAP = 1800 # 30 minutes max + + +def is_source_outage(exc: Exception) -> bool: + """True when ``exc`` indicates the Amazon source/proxy is down (transient, + whole-source), as opposed to a normal per-item error. + + Robust to how the error is surfaced: an explicit ``status_code`` attribute, + an ``HTTP `` prefix in the message, or an outage phrase (covers + connection failures and non-JSON error pages that carry no status code).""" + code = getattr(exc, "status_code", None) + if isinstance(code, int) and code in _OUTAGE_STATUS: + return True + msg = str(exc).lower() + m = re.search(r"http\s+(\d{3})", msg) + if m and int(m.group(1)) in _OUTAGE_STATUS: + return True + return any(p in msg for p in _OUTAGE_PHRASES) + + +def next_poll_delay_seconds(outage_streak: int) -> int: + """Seconds to wait before the next item. Normal cadence when healthy; + escalating back-off (30s, 60s, 120s, … capped at 30 min) the longer the + source has been down, so a dead instance can't flood logs/CPU/DB.""" + if outage_streak <= 0: + return _NORMAL_DELAY + return min(_OUTAGE_BASE * (2 ** min(outage_streak - 1, 6)), _OUTAGE_CAP) diff --git a/core/amazon_worker.py b/core/amazon_worker.py index 2ac47ee4..c4dbc0c7 100644 --- a/core/amazon_worker.py +++ b/core/amazon_worker.py @@ -9,6 +9,7 @@ from database.music_database import MusicDatabase from core.amazon_client import AmazonClient from core.worker_utils import interruptible_sleep, set_album_api_track_count from core.enrichment.manual_match_honoring import honor_stored_match +from core.amazon_outage import is_source_outage, next_poll_delay_seconds logger = get_logger("amazon_worker") @@ -39,6 +40,11 @@ class AmazonWorker: self.retry_days = 30 self.name_similarity_threshold = 0.80 + # Source-outage circuit breaker: counts consecutive whole-source + # failures (proxy down / "not initialized" / unreachable) so the loop + # backs off instead of grinding the whole library item-by-item. + self._outage_streak = 0 + logger.info("Amazon background worker initialized") def _ensure_amazon_schema(self, cursor) -> None: @@ -151,7 +157,9 @@ class AmazonWorker: continue self._process_item(item) - interruptible_sleep(self._stop_event, 2) + # Normal 2s cadence when healthy; escalating back-off (up to + # 30 min) while the source is in an outage streak. + interruptible_sleep(self._stop_event, next_poll_delay_seconds(self._outage_streak)) except Exception as e: logger.error(f"Error in worker loop: {e}") @@ -275,7 +283,32 @@ class AmazonWorker: elif item_type == 'track': self._process_track(item_id, item_name, item.get('artist', ''), item) + # The source answered (match or not_found) — clear any outage streak. + if self._outage_streak: + logger.info("Amazon source recovered after %d outage(s), resuming", + self._outage_streak) + self._outage_streak = 0 + except Exception as e: + if is_source_outage(e): + # The whole source is down (proxy 5xx / "not initialized" / + # unreachable). Do NOT mark the item 'error' — that would burn + # the entire library to a state the retry tiers never re-attempt + # for a transient outage. Leave it untouched so it's retried once + # the instance recovers, and let the loop back off. Log once per + # streak to avoid flooding. + self._outage_streak += 1 + if self._outage_streak == 1: + logger.warning("Amazon source unavailable — pausing enrichment " + "until it recovers: %s", e) + else: + logger.debug("Amazon source still unavailable (streak=%d): %s", + self._outage_streak, e) + return + # A non-outage error means the source actually answered (e.g. a + # 404/parse error on a real response), so the outage is over — + # clear the streak and handle this as a normal per-item error. + self._outage_streak = 0 logger.error(f"Error processing {item['type']} #{item['id']}: {e}") self.stats['errors'] += 1 try: diff --git a/core/discovery/sync.py b/core/discovery/sync.py index a77b5d3e..eb338edc 100644 --- a/core/discovery/sync.py +++ b/core/discovery/sync.py @@ -49,6 +49,90 @@ class SyncDeps: sync_lock: Any # threading.Lock +async def _database_only_find_track(spotify_track, candidate_pool=None): + """Database-only track matcher used when no media server is connected. + + Patched onto sync_service._find_track_in_media_server. Accepts + ``candidate_pool`` for interface parity with the real matcher (sync_service + calls it with candidate_pool=...); the DB path queries the library directly + via check_track_exists, so it doesn't need the per-artist candidate cache — + but it MUST accept the kwarg or sync raises "unexpected keyword argument + 'candidate_pool'". Module-level (not a nested closure) so it's importable + and unit-tested. + """ + logger.info(f"Database-only search for: '{spotify_track.name}' by {spotify_track.artists}") + try: + from database.music_database import MusicDatabase + from config.settings import config_manager + + db = MusicDatabase() + active_server = config_manager.get_active_media_server() + original_title = spotify_track.name + spotify_id = getattr(spotify_track, 'id', '') or '' + + # --- Sync match cache fast-path --- + if spotify_id: + try: + cached = db.read_sync_match_cache(spotify_id, active_server) + if cached: + db_track_check = db.get_track_by_id(cached['server_track_id']) + if db_track_check: + class DatabaseTrackCached: + def __init__(self, db_t): + self.ratingKey = db_t.id + self.title = db_t.title + self.id = db_t.id + logger.debug(f"Sync cache hit: '{original_title}' → server track {cached['server_track_id']}") + return DatabaseTrackCached(db_track_check), cached['confidence'] + logger.warning(f"Sync cache stale for '{original_title}' — track gone") + except Exception as e: + logger.debug("sync match cache fast-path failed: %s", e) + # --- End cache fast-path --- + + # Try each artist + for artist in spotify_track.artists: + if isinstance(artist, str): + artist_name = artist + elif isinstance(artist, dict) and 'name' in artist: + artist_name = artist['name'] + else: + artist_name = str(artist) + + db_track, confidence = db.check_track_exists( + original_title, artist_name, + confidence_threshold=0.80, + server_source=active_server + ) + + if db_track and confidence >= 0.80: + logger.info(f"Database match: '{db_track.title}' (confidence: {confidence:.2f})") + if spotify_id: + try: + from core.matching_engine import MusicMatchingEngine + me = MusicMatchingEngine() + db.save_sync_match_cache( + spotify_id, me.clean_title(original_title), me.clean_artist(artist_name), + active_server, db_track.id, db_track.title, confidence + ) + except Exception as e: + logger.debug("save sync match cache failed: %s", e) + + class DatabaseTrackMock: + def __init__(self, db_track): + self.ratingKey = db_track.id + self.title = db_track.title + self.id = db_track.id + + return DatabaseTrackMock(db_track), confidence + + logger.warning(f"No database match found for: '{original_title}'") + return None, 0.0 + + except Exception as e: + logger.error(f"Database search error: {e}") + return None, 0.0 + + def run_sync_task(playlist_id, playlist_name, tracks_json, automation_id=None, profile_id=1, playlist_image_url='', deps: SyncDeps = None, sync_mode: str = 'replace'): """The actual sync function that runs in the background thread.""" sync_states = deps.sync_states @@ -260,89 +344,9 @@ def run_sync_task(playlist_id, playlist_name, tracks_json, automation_id=None, p if media_client is None or not media_client.is_connected(): logger.info("Media client not connected - patching sync service for database-only matching") - # Store original method - original_find_track = sync_service._find_track_in_media_server - - # Create database-only replacement method - async def database_only_find_track(spotify_track): - logger.info(f"Database-only search for: '{spotify_track.name}' by {spotify_track.artists}") - try: - from database.music_database import MusicDatabase - from config.settings import config_manager - - db = MusicDatabase() - active_server = config_manager.get_active_media_server() - original_title = spotify_track.name - spotify_id = getattr(spotify_track, 'id', '') or '' - - # --- Sync match cache fast-path --- - if spotify_id: - try: - cached = db.read_sync_match_cache(spotify_id, active_server) - if cached: - db_track_check = db.get_track_by_id(cached['server_track_id']) - if db_track_check: - class DatabaseTrackCached: - def __init__(self, db_t): - self.ratingKey = db_t.id - self.title = db_t.title - self.id = db_t.id - logger.debug(f"Sync cache hit: '{original_title}' → server track {cached['server_track_id']}") - return DatabaseTrackCached(db_track_check), cached['confidence'] - logger.warning(f"Sync cache stale for '{original_title}' — track gone") - except Exception as e: - logger.debug("sync match cache fast-path failed: %s", e) - # --- End cache fast-path --- - - # Try each artist (same logic as original) - for artist in spotify_track.artists: - # Extract artist name from both string and dict formats - if isinstance(artist, str): - artist_name = artist - elif isinstance(artist, dict) and 'name' in artist: - artist_name = artist['name'] - else: - artist_name = str(artist) - - db_track, confidence = db.check_track_exists( - original_title, artist_name, - confidence_threshold=0.80, - server_source=active_server - ) - - if db_track and confidence >= 0.80: - logger.info(f"Database match: '{db_track.title}' (confidence: {confidence:.2f})") - - # Save to sync match cache - if spotify_id: - try: - from core.matching_engine import MusicMatchingEngine - me = MusicMatchingEngine() - db.save_sync_match_cache( - spotify_id, me.clean_title(original_title), me.clean_artist(artist_name), - active_server, db_track.id, db_track.title, confidence - ) - except Exception as e: - logger.debug("save sync match cache failed: %s", e) - - # Create mock track object for playlist creation - class DatabaseTrackMock: - def __init__(self, db_track): - self.ratingKey = db_track.id - self.title = db_track.title - self.id = db_track.id - - return DatabaseTrackMock(db_track), confidence - - logger.warning(f"No database match found for: '{original_title}'") - return None, 0.0 - - except Exception as e: - logger.error(f"Database search error: {e}") - return None, 0.0 - - # Patch the method - sync_service._find_track_in_media_server = database_only_find_track + # Patch the matcher to the module-level database-only implementation + # (importable + unit-tested; accepts candidate_pool for parity). + sync_service._find_track_in_media_server = _database_only_find_track logger.info("Patched sync service to use database-only matching") sync_start_time = time.time() diff --git a/core/download_plugins/album_bundle.py b/core/download_plugins/album_bundle.py index cf9030e4..17d9d6fa 100644 --- a/core/download_plugins/album_bundle.py +++ b/core/download_plugins/album_bundle.py @@ -21,8 +21,10 @@ folder scan. from __future__ import annotations +import re import shutil import time +import unicodedata import uuid from pathlib import Path from typing import Any, Callable, Iterable, Optional @@ -32,6 +34,13 @@ from utils.logging_config import get_logger logger = get_logger("download_plugins.album_bundle") +# Minimum album-title relevance a Prowlarr candidate must clear to be eligible +# for an album-bundle download (#730). Prowlarr returns broad fuzzy matches — a +# "Heroes" search also returns other Bowie albums — so without this gate the +# most-popular result wins regardless of whether it's the right album. Below +# this floor we refuse the bundle and let the caller fall back to per-track. +_ALBUM_TITLE_RELEVANCE_FLOOR = 0.6 + # Album-pick size floor / ceiling. Single-track torrents (~10 MB) # are rejected when bigger candidates exist; anything past 3 GB is @@ -92,10 +101,96 @@ def quality_score(title: str, quality_guess) -> int: return _QUALITY_SCORE.get(quality_guess(title) or '', 0) -def pick_best_album_release(candidates, quality_guess) -> Optional[object]: +def _normalize_release_text(text: str) -> str: + """Lowercase, fold accents (Björk -> bjork), strip punctuation to spaces. + + NFKD-decompose then drop combining marks so accented characters fold to + their base letter instead of fragmenting (the naive approach turned + 'Björk' into 'bj rk'). Collapses runs of whitespace. + """ + if not text: + return "" + decomposed = unicodedata.normalize("NFKD", text) + stripped = "".join(c for c in decomposed if not unicodedata.combining(c)) + lowered = stripped.lower() + # Punctuation -> space (so "heroes" matches "heroes:" / "heroes -"), + # then collapse whitespace. + cleaned = re.sub(r"[^a-z0-9]+", " ", lowered) + return re.sub(r"\s+", " ", cleaned).strip() + + +# Edition / format / qualifier words that appear in stored album names or +# release titles but say nothing about WHICH album it is. Stripped before +# scoring so "Currents" matches "Currents (Deluxe)" and "Heroes" matches +# "Heroes (2017 Remaster)" — the #730 fix must not reject the RIGHT album just +# because the DB name carries an edition suffix the torrent title lacks. +_ALBUM_NOISE_WORDS = frozenset({ + "deluxe", "edition", "remaster", "remastered", "remasters", "remix", + "expanded", "anniversary", "bonus", "version", "explicit", "clean", + "reissue", "special", "limited", "collectors", "collector", "the", + "ep", "lp", "album", "single", "disc", "cd", "vol", "volume", + "flac", "mp3", "aac", "ogg", "wav", "alac", "m4a", "320", "256", "192", + "web", "vinyl", "hi", "res", "hires", "24bit", "16bit", "original", + "soundtrack", "ost", +}) + + +def _significant_words(normalized: str) -> list: + """Words that actually identify an album: drop pure-digit tokens (years, + bitrates) and edition/format noise. Keeps at least the raw words if the + filter would empty it (e.g. an album literally named '1989' or 'Deluxe').""" + words = [w for w in normalized.split() + if w not in _ALBUM_NOISE_WORDS and not w.isdigit()] + return words or normalized.split() + + +def album_title_relevance(candidate_title: str, album_name: str) -> float: + """How well a release title matches the requested album, 0.0–1.0. + + Scores the fraction of the album's SIGNIFICANT words (edition/format/year + noise removed) that appear as whole words in the candidate title. + Word-boundary, not substring, so "Heroes" does NOT match "Superheroes" and + a different album sharing no significant words scores 0 — while "Currents" + still matches "Currents (Deluxe)" and "Heroes" matches the "2017 Remaster". + + Returns 1.0 when there's no album name to check (can't gate on nothing — + preserves old behavior for callers that don't pass a title). + """ + norm_album = _normalize_release_text(album_name) + if not norm_album: + return 1.0 + norm_title = _normalize_release_text(candidate_title) + if not norm_title: + return 0.0 + album_words = _significant_words(norm_album) + title_words = set(norm_title.split()) + if not album_words: + return 1.0 + matched = sum(1 for w in album_words if w in title_words) + coverage = matched / len(album_words) + # Full-phrase bonus (idea from contributor PR #731): when the album's core + # phrase appears intact in the title, we're highly confident it's the right + # release even if token-coverage is dragged down by a long multi-word name. + # MUST be word-boundary anchored, NOT a raw substring — a naive + # `phrase in norm_title` lets "heroes" match "superheroes" and reintroduces + # the exact wrong-album bug #730 fixes (PR #731's version has this flaw). + core_phrase = " ".join(album_words) + if core_phrase and re.search(rf"(?:^| ){re.escape(core_phrase)}(?: |$)", norm_title): + coverage = max(coverage, 0.9) + return coverage + + +def pick_best_album_release(candidates, quality_guess, + album_name: str = "") -> Optional[object]: """Pick the single best torrent / NZB for an album-bundle download. Heuristic, in priority order: + 0. Album-TITLE relevance gate (#730): drop candidates whose title doesn't + sufficiently match the requested album. Prowlarr returns broad fuzzy + matches, so without this the most-popular result wins even when it's a + different album. When ``album_name`` is given and NOTHING clears the + relevance floor, return None — the caller then falls back to per-track + rather than downloading a confident mismatch. 1. Reasonable album-ish size (40 MB – 3 GB) — drops single-track releases that snuck in and quarantines suspicious giants. 2. Higher seeders > lower (dead torrents = dead downloads). @@ -106,6 +201,24 @@ def pick_best_album_release(candidates, quality_guess) -> Optional[object]: """ if not candidates: return None + + # 0. Title-relevance gate. Only applied when we know the album name; with + # no name we can't judge relevance, so we don't gate (old behavior). + if album_name: + relevant = [ + c for c in candidates + if album_title_relevance(c.title or "", album_name) >= _ALBUM_TITLE_RELEVANCE_FLOOR + ] + if not relevant: + logger.warning( + "[Album Bundle] No candidate cleared the title-relevance floor " + "for '%s' (%d candidates rejected as wrong album) — refusing the " + "bundle so the caller falls back to per-track.", + album_name, len(candidates), + ) + return None + candidates = relevant + sized = [c for c in candidates if ALBUM_PICK_MIN_BYTES <= (c.size or 0) <= ALBUM_PICK_MAX_BYTES] pool = sized or list(candidates) diff --git a/core/download_plugins/torrent.py b/core/download_plugins/torrent.py index 17ef566b..dbebcf95 100644 --- a/core/download_plugins/torrent.py +++ b/core/download_plugins/torrent.py @@ -494,12 +494,25 @@ class TorrentDownloadPlugin(DownloadSourcePlugin): candidates = [r for r in search_results if r.protocol == 'torrent' and (r.magnet_uri or r.download_url)] if not candidates: + # Album isn't available on this source. Mark the failure as + # fallback-eligible so the dispatch returns to the per-track flow + # instead of hard-failing the batch — in hybrid mode that lets the + # next configured source take over. Without this flag a torrent-first + # hybrid would get stuck at "searching" forever when Prowlarr + # returns nothing, never trying the other sources. result['error'] = f'No torrent results found for "{query}"' + result['fallback'] = True return result - picked = pick_best_album_release(candidates, _guess_quality_from_title) + picked = pick_best_album_release( + candidates, _guess_quality_from_title, album_name=album_name, + ) if picked is None: - result['error'] = 'No suitable torrent candidate after filtering' + # No candidate matched the requested album (or none passed filtering). + # Fall back to the per-track flow rather than downloading a wrong + # album (#730) — per-track searches each track individually. + result['error'] = 'No torrent candidate matched the requested album' + result['fallback'] = True return result download_url = picked.magnet_uri or picked.download_url diff --git a/core/download_plugins/usenet.py b/core/download_plugins/usenet.py index a8b0c529..2bf14512 100644 --- a/core/download_plugins/usenet.py +++ b/core/download_plugins/usenet.py @@ -465,12 +465,22 @@ class UsenetDownloadPlugin(DownloadSourcePlugin): candidates = [r for r in search_results if r.protocol == 'usenet' and r.download_url] if not candidates: + # Album isn't available on this source — fall back to the per-track + # flow (next configured source in hybrid mode) rather than hard- + # failing the whole batch. Mirrors the torrent plugin + soulseek's + # default fallback contract. result['error'] = f'No usenet results found for "{query}"' + result['fallback'] = True return result - picked = pick_best_album_release(candidates, _guess_quality_from_title) + picked = pick_best_album_release( + candidates, _guess_quality_from_title, album_name=album_name, + ) if picked is None: - result['error'] = 'No suitable NZB candidate after filtering' + # No candidate matched the requested album (or none passed filtering). + # Fall back to per-track rather than grabbing a wrong album (#730). + result['error'] = 'No NZB candidate matched the requested album' + result['fallback'] = True return result logger.info("[Usenet album] Picked '%s' (size=%.1fMB grabs=%s indexer=%s)", diff --git a/core/downloads/album_bundle_dispatch.py b/core/downloads/album_bundle_dispatch.py index 634c0e95..ef6b104f 100644 --- a/core/downloads/album_bundle_dispatch.py +++ b/core/downloads/album_bundle_dispatch.py @@ -173,7 +173,22 @@ def try_dispatch( ) except Exception as exc: logger.exception("[Album Bundle] %s plugin raised: %s", mode, exc) - outcome = {'success': False, 'error': f'Plugin error: {exc}'} + # An OSError means an I/O step failed after the source already had the + # album — most importantly the staging dir not being writable (#760), + # but also any transient filesystem error. Treat it as fallback-eligible + # so we return to the per-track flow instead of hard-failing the whole + # batch (the #715 symptom: files download, then the batch fails). + # Programming errors (TypeError, KeyError, …) are NOT OSError and stay + # terminal, so genuine bugs still fail loudly. (requests' network + # exceptions also subclass OSError, but plugins normally catch those + # internally and return an outcome rather than raising; if one does + # surface here, falling back to per-track is still the safe choice.) + is_io_failure = isinstance(exc, OSError) + outcome = { + 'success': False, + 'error': f'Plugin error: {exc}', + 'fallback': is_io_failure, + } if not outcome.get('success'): err = outcome.get('error', 'Album bundle download failed') diff --git a/core/downloads/task_worker.py b/core/downloads/task_worker.py index 77cb803f..cc6751dc 100644 --- a/core/downloads/task_worker.py +++ b/core/downloads/task_worker.py @@ -34,11 +34,22 @@ logger = logging.getLogger(__name__) def _private_album_bundle_staging_miss_reason(batch_id: Optional[str], deps: Any) -> Optional[str]: """Return a user-facing miss reason when per-track search should stop. - Torrent / usenet / Soulseek album batches first download one private staged release, + Torrent / usenet album batches first download one private staged release, then each track claims the matching staged file. If that claim fails after the release is already staged, falling through to the normal per-track search only retries release-level sources N times and can keep re-adding - the same torrent. Treat the staged release as authoritative for this pass. + the same torrent/NZB. For those two sources we treat the staged release as + authoritative for this pass. + + Soulseek is deliberately NOT short-circuited. A Soulseek album bundle stages + whichever single folder scored best, and ``album_bundle_partial`` only + reflects whether the files found IN that folder downloaded — not whether the + folder actually contained every track the album needs. So a track the album + needs but that wasn't in the chosen folder would otherwise be marked + not_found with no fallback (#743). Unlike torrent/usenet, Soulseek per-track + search is a genuine per-file network search — it doesn't re-add a release — + so letting these misses fall through to the normal per-track flow (and, in + hybrid mode, onward to the next source) is correct and cheap. """ if not batch_id: return None @@ -60,7 +71,7 @@ def _private_album_bundle_staging_miss_reason(batch_id: Optional[str], deps: Any batch.get('album_bundle_private_staging') and batch.get('album_bundle_state') == 'staged' and not batch.get('album_bundle_partial') - and source in ('torrent', 'usenet', 'soulseek') + and source in ('torrent', 'usenet') and (mode == source or (mode == 'hybrid' and hybrid_first == source)) ): return f'Track was not found in the staged {source} album release' diff --git a/core/downloads/track_detail.py b/core/downloads/track_detail.py new file mode 100644 index 00000000..806de655 --- /dev/null +++ b/core/downloads/track_detail.py @@ -0,0 +1,106 @@ +"""Assemble a per-track detail view for the download-modal "track detail" modal. + +Merges a live download task with its ``library_history`` record (the same data +the Download History cards render) into one dict the frontend modal consumes. +Kept pure + importable so the merge + status classification are unit-tested +without Flask or the DB; the web endpoint is thin glue around build_track_detail. +""" + +from __future__ import annotations + +from typing import Any, Dict, Optional + +# error_message substrings that mean "quarantined" (file recoverable) rather +# than a plain failure. Mirrors the download-modal status renderer. +_QUARANTINE_KEYWORDS = ( + 'integrity check failed', + 'bit depth filter', + 'verification failed', + 'quarantin', +) + + +def classify_status_kind(status: str, error_message: str = '') -> str: + """Map a raw task status to a UI 'kind' that drives the modal layout: + completed / quarantined / failed / not_found / in_progress. + """ + s = (status or '').lower() + if s == 'completed': + return 'completed' + if s in ('failed', 'cancelled'): + em = (error_message or '').lower() + if any(k in em for k in _QUARANTINE_KEYWORDS): + return 'quarantined' + return 'failed' + if s == 'not_found': + return 'not_found' + return 'in_progress' + + +def _first_artist(track_info: Dict[str, Any]) -> str: + artists = track_info.get('artists') or [] + if isinstance(artists, list) and artists: + first = artists[0] + if isinstance(first, dict): + return (first.get('name') or '').strip() + return str(first).strip() + return (track_info.get('artist') or track_info.get('artist_name') or '').strip() + + +def _album_name(track_info: Dict[str, Any]) -> str: + album = track_info.get('album') + if isinstance(album, dict): + return (album.get('name') or '').strip() + return (album or '').strip() if isinstance(album, str) else '' + + +def build_track_detail(task: Dict[str, Any], history: Optional[Dict[str, Any]] = None) -> Dict[str, Any]: + """Merge a download task (+ optional library_history row) into one detail dict. + + The task supplies live status/source/reason/quarantine id; the history row + (when found) supplies the durable provenance — final file path, quality, + AcoustID verdict, source, and the expected-vs-downloaded comparison. + """ + ti = task.get('track_info') if isinstance(task.get('track_info'), dict) else {} + status = task.get('status', '') or '' + kind = classify_status_kind(status, task.get('error_message', '') or '') + + detail: Dict[str, Any] = { + 'task_id': task.get('task_id') or task.get('id') or '', + 'status': status, + 'status_kind': kind, + 'title': (ti.get('name') or '').strip(), + 'artist': _first_artist(ti), + 'album': _album_name(ti), + 'source': (task.get('username') or '').strip(), + 'reason': (task.get('error_message') or '').strip(), + 'quarantine_entry_id': task.get('quarantine_entry_id') or '', + 'file_path': (task.get('filename') or '').strip(), + 'quality': '', + 'acoustid_result': '', + 'thumb_url': '', + 'expected': {}, + 'downloaded': {}, + } + + if history: + detail['file_path'] = (history.get('file_path') or detail['file_path']) + detail['quality'] = history.get('quality') or '' + detail['acoustid_result'] = history.get('acoustid_result') or '' + detail['source'] = history.get('download_source') or detail['source'] + detail['thumb_url'] = history.get('thumb_url') or '' + detail['downloaded'] = { + 'title': history.get('title') or '', + 'artist': history.get('artist_name') or '', + 'album': history.get('album_name') or '', + } + detail['expected'] = { + 'title': history.get('source_track_title') or '', + 'artist': history.get('source_artist') or '', + } + # Fall back to history values when the task had none. + detail['title'] = detail['title'] or detail['downloaded']['title'] + detail['artist'] = detail['artist'] or detail['downloaded']['artist'] + detail['album'] = detail['album'] or detail['downloaded']['album'] + + return detail diff --git a/core/image_cache.py b/core/image_cache.py index 040c1126..e1087e34 100644 --- a/core/image_cache.py +++ b/core/image_cache.py @@ -172,11 +172,14 @@ class ImageCache: raise ImageCacheError(f"Upstream response is not an image: {mime_type}") declared_size = response.headers.get("Content-Length") + expected_bytes = None try: - if declared_size and int(declared_size) > self.max_download_bytes: - raise ImageCacheError("Image exceeds configured size limit") + if declared_size: + expected_bytes = int(declared_size) + if expected_bytes > self.max_download_bytes: + raise ImageCacheError("Image exceeds configured size limit") except ValueError: - pass + expected_bytes = None ext = mimetypes.guess_extension(mime_type) or ".img" if ext == ".jpe": @@ -205,6 +208,22 @@ class ImageCache: if total <= 0: raise ImageCacheError("Image response was empty") + # Truncation guard (#750): a dropped/short connection makes + # iter_content end early WITHOUT raising, so a partial image would + # otherwise be committed as status='ok' and cached permanently — + # rendering as a half-decoded cover (top strip, rest grey). If the + # server declared a Content-Length and we got fewer bytes, treat it + # as a failed download: discard the tmp file and don't cache it, so + # the next request retries fresh instead of serving a broken file. + if expected_bytes is not None and total < expected_bytes: + try: + tmp_path.unlink(missing_ok=True) + except Exception as cleanup_exc: + logger.debug("image_cache tmp cleanup failed: %s", cleanup_exc) + raise ImageCacheError( + f"Truncated image download: got {total} of {expected_bytes} bytes" + ) + os.replace(tmp_path, path) expires_at = now + self.ttl_seconds with self._db_lock: diff --git a/core/imports/file_integrity.py b/core/imports/file_integrity.py index a0ebb18a..615af097 100644 --- a/core/imports/file_integrity.py +++ b/core/imports/file_integrity.py @@ -183,11 +183,29 @@ def check_audio_integrity( checks["actual_length_s"] = actual_length_s if actual_length_s <= 0: + # Length 0 is NOT proof of corruption here: the file already passed the + # size gate, was identified as a real audio format, and has a valid + # info block. A genuinely empty/truncated/stub file fails one of those + # earlier checks instead. The real cause of a clean-but-zero-length + # parse is "length unknown" — fragmented / streamed FLAC carries + # total_samples=0 in its STREAMINFO even though every audio frame is + # present and the file plays fine. HiFi is the common trigger: it + # assembles FLAC from HLS segments and demuxes with `ffmpeg -c copy`, + # which preserves total_samples=0, so mutagen computes length 0 and the + # file was wrongly quarantined (#756). Treat it as unknown length: + # accept the file and skip the duration cross-check we can't perform + # without a length. mutagen never decoded/validated frame data anyway, + # so accepting here doesn't weaken real corruption detection. + logger.warning( + "[Integrity] %s parsed cleanly (%d bytes, format=%s) but reports " + "length 0 — treating as unknown length (likely streamed/fragmented " + "FLAC), not rejecting", + os.path.basename(file_path), size, type(audio).__name__, + ) return IntegrityResult( - ok=False, - reason="Mutagen reports zero-length audio — file has no playable " - "audio data", - checks={**checks, "mutagen_parse": "zero_length"}, + ok=True, + checks={**checks, "mutagen_parse": "zero_length_unknown", + "length_check": "skipped_unknown_length"}, ) # --- Check 3: duration agreement (optional) --- diff --git a/core/imports/pipeline.py b/core/imports/pipeline.py index 7015b927..fa196aa5 100644 --- a/core/imports/pipeline.py +++ b/core/imports/pipeline.py @@ -92,12 +92,22 @@ def _should_skip_quarantine_check(context: dict, check_name: str) -> bool: def _mark_task_quarantined(context: dict, quarantine_path: str | None) -> None: if not quarantine_path: return + entry_id = entry_id_from_quarantined_filename(quarantine_path) + # Always stash the entry id on the context. The verification wrapper + # (post_process_matched_download_with_verification) pops task_id/batch_id + # OUT of the context before running the inner pipeline, so when a quarantine + # happens inside that inner run context.get('task_id') is None here and the + # direct write below no-ops. The wrapper restores task_id afterward and + # applies this stashed id to the real task — without this, downloads that go + # through the wrapper (album-bundle / staging path) quarantined with no + # quarantine_entry_id, so the UI had no way to manage the file (#756-adjacent). + context['_quarantine_entry_id'] = entry_id task_id = context.get('task_id') if not task_id: return with tasks_lock: if task_id in download_tasks: - download_tasks[task_id]['quarantine_entry_id'] = entry_id_from_quarantined_filename(quarantine_path) + download_tasks[task_id]['quarantine_entry_id'] = entry_id def build_import_pipeline_runtime( @@ -948,6 +958,9 @@ def post_process_matched_download_with_verification(context_key, context, file_p if task_id in download_tasks: download_tasks[task_id]['status'] = 'failed' download_tasks[task_id]['error_message'] = f"AcoustID verification failed: {failure_msg}" + _eid = context.get('_quarantine_entry_id') + if _eid: + download_tasks[task_id]['quarantine_entry_id'] = _eid with matched_context_lock: if context_key in matched_downloads_context: del matched_downloads_context[context_key] @@ -1001,6 +1014,9 @@ def post_process_matched_download_with_verification(context_key, context, file_p download_tasks[task_id]['error_message'] = ( f"File integrity check failed: {failure_msg}" ) + _eid = context.get('_quarantine_entry_id') + if _eid: + download_tasks[task_id]['quarantine_entry_id'] = _eid with matched_context_lock: if context_key in matched_downloads_context: del matched_downloads_context[context_key] diff --git a/core/imports/quarantine.py b/core/imports/quarantine.py index 43ed5191..3df79f85 100644 --- a/core/imports/quarantine.py +++ b/core/imports/quarantine.py @@ -14,6 +14,7 @@ from __future__ import annotations import json import os import shutil +import time from pathlib import Path from typing import Any, Dict, List, Optional, Tuple @@ -289,6 +290,56 @@ def _restore_filename(quarantined_filename: str, sidecar_original: Optional[str] return base +def get_quarantine_entry_stream_info( + quarantine_dir: str, entry_id: str +) -> Optional[Tuple[str, str]]: + """Resolve a quarantined entry to ``(file_path, original_extension)`` for + in-app playback. + + The on-disk file carries a ``.quarantined`` suffix, so its own extension is + useless for picking an audio MIME type. Recover the real extension from the + sidecar's ``original_filename`` when present, else from the quarantine + filename convention. Returns None when the entry's file can't be found. + """ + file_path, sidecar_path = _resolve_entry_paths(quarantine_dir, entry_id) + if not file_path or not os.path.isfile(file_path): + return None + + sidecar_original: Optional[str] = None + if sidecar_path: + try: + with open(sidecar_path, encoding="utf-8") as f: + sidecar_original = json.load(f).get("original_filename") + except Exception as exc: + logger.debug("stream-info: sidecar read failed for %s: %s", entry_id, exc) + + original_name = _restore_filename(os.path.basename(file_path), sidecar_original) + extension = os.path.splitext(original_name)[1].lower() + return file_path, extension + + +def _move_with_retry(src: str, dst: str, attempts: int = 4, delay: float = 0.4) -> bool: + """Move a file, retrying briefly on transient OS locks. + + On Windows a still-open read handle (e.g. the in-app quarantine preview + player that just streamed the file) makes shutil.move raise + PermissionError / WinError 32 "file in use". The handle is released a beat + after playback stops, so a few short retries clear the common case without + failing the whole Approve/Recover. Returns True on success. + """ + last_exc: Optional[BaseException] = None + for i in range(attempts): + try: + shutil.move(src, dst) + return True + except OSError as exc: + last_exc = exc + if i < attempts - 1: + time.sleep(delay) + logger.error("move failed after %d attempts: %s -> %s: %s", attempts, src, dst, last_exc) + return False + + def approve_quarantine_entry( quarantine_dir: str, entry_id: str, @@ -334,10 +385,9 @@ def approve_quarantine_entry( restored_path = os.path.join(restore_dir, original_name) restored_path = _ensure_unique_path(restored_path) - try: - shutil.move(file_path, restored_path) - except OSError as exc: - logger.error("approve: failed to restore %s -> %s: %s", file_path, restored_path, exc) + if not _move_with_retry(file_path, restored_path): + logger.error("approve: failed to restore %s -> %s (file may still be in use)", + file_path, restored_path) return None try: @@ -377,10 +427,8 @@ def recover_to_staging( os.makedirs(staging_dir, exist_ok=True) target = _ensure_unique_path(os.path.join(staging_dir, restored_name)) - try: - shutil.move(file_path, target) - except OSError as exc: - logger.error("recover: failed to move %s -> %s: %s", file_path, target, exc) + if not _move_with_retry(file_path, target): + logger.error("recover: failed to move %s -> %s (file may still be in use)", file_path, target) return None if sidecar_path and os.path.isfile(sidecar_path): diff --git a/core/library/duplicate_keep.py b/core/library/duplicate_keep.py new file mode 100644 index 00000000..281bf1d7 --- /dev/null +++ b/core/library/duplicate_keep.py @@ -0,0 +1,57 @@ +"""Choosing which copy of a duplicate track to keep. + +The keeper is the highest-quality copy, and **format/lossless is the primary +criterion**: a lossless FLAC must beat a lossy MP3 regardless of the recorded +bitrate number — a FLAC whose bitrate the library scan never populated (a +common gap) still has to win over a 282 kbps MP3. Only *within* the same format +tier do bitrate, then duration, then track number break the tie. + +This was lifted out of the repair worker so the deletion path and the UI's +"Keep Best" recommendation rank identically (previously both ranked by bitrate +first, which kept the MP3 when the FLAC's bitrate was missing), and so the +ranking is unit-testable in isolation. +""" + +from __future__ import annotations + +import os +from typing import Dict, List, Optional, Tuple + +# Format quality rank by file extension (higher = better). Mirrors +# ``auto_import_worker._quality_rank`` so lossless always outranks lossy. +_FORMAT_RANK = { + ".flac": 10, ".wav": 9, ".aiff": 9, ".aif": 9, ".ape": 8, + ".m4a": 7, ".ogg": 6, ".opus": 6, ".mp3": 5, ".aac": 5, ".wma": 3, +} + + +def format_rank_for_path(file_path: Optional[str]) -> int: + """Quality rank for a file by extension (higher = better, unknown = 1).""" + if not file_path: + return 1 + ext = os.path.splitext(str(file_path))[1].lower() + return _FORMAT_RANK.get(ext, 1) + + +def duplicate_keep_sort_key(track: Dict) -> Tuple[int, int, float, int]: + """Sort key for picking the keeper — the higher tuple wins. + + Order of precedence: format/lossless tier, then bitrate, then duration, + then track number (a real number over a placeholder ``01``). Putting format + first is the whole point — it makes FLAC beat MP3 even when the FLAC's + bitrate is 0/missing in the DB. + """ + return ( + format_rank_for_path(track.get("file_path")), + track.get("bitrate") or 0, + track.get("duration") or 0, + track.get("track_number") or 0, + ) + + +def pick_duplicate_to_keep(tracks: List[Dict]) -> Optional[Dict]: + """Return the copy to keep from a duplicate group (highest sort key), or + ``None`` if the group is empty.""" + if not tracks: + return None + return max(tracks, key=duplicate_keep_sort_key) diff --git a/core/library/manual_library_match.py b/core/library/manual_library_match.py index 132ad729..ef14af9f 100644 --- a/core/library/manual_library_match.py +++ b/core/library/manual_library_match.py @@ -14,12 +14,27 @@ from utils.logging_config import get_logger logger = get_logger("library.manual_library_match") +def normalize_library_track_id(value: Any) -> Optional[str]: + """Normalize an incoming library_track_id to the opaque string id we store. + + Library track ids are ``str(ratingKey)`` — numeric strings for Plex, but + GUIDs/hashes for Jellyfin, Navidrome, and other Subsonic servers (see + ``tracks.id`` which is TEXT). They must NOT be coerced to int: doing so + rejected every non-numeric id with "Invalid library track id". We only + reject empty/None here; the caller validates existence against the DB. + """ + if value is None: + return None + text = str(value).strip() + return text or None + + def save_match( db, profile_id: int, source: str, source_track_id: str, - library_track_id: int, + library_track_id: str, **meta, ) -> bool: """Save (insert or replace) a manual match.""" diff --git a/core/metadata/art_lookup.py b/core/metadata/art_lookup.py new file mode 100644 index 00000000..458dc946 --- /dev/null +++ b/core/metadata/art_lookup.py @@ -0,0 +1,291 @@ +"""Per-source album-art lookups + availability for the cover-art picker. + +Bridges the pure resolver in ``art_sources.py`` to the real metadata clients. +Each supported source contributes two things: + +- **availability** — is this source usable for the current user right now? + Free sources (CAA, Deezer, iTunes, AudioDB) are always available; account + sources (Spotify) only when connected. This is what powers "not everybody + has access to every source": the UI offers only available sources and the + resolver skips the rest. +- **lookup** — ``(artist, album, metadata) -> cover_url | None``, calling an + EXISTING client method. Every lookup is individually guarded so any error or + miss degrades to ``None`` (the resolver then falls through to the next + source, finally to the download's own art) — a flaky source can never raise + into, or break, a download. + +Lookups are cached per album via ``build_art_lookup`` so resolving art for a +16-track album hits each source at most once. + +Client accessors are imported lazily inside each function to keep this module +import-light (the pure resolver + its tests never pull a network client). +""" + +from __future__ import annotations + +import re +from typing import Callable, Dict, List, Optional + +from utils.logging_config import get_logger + +from core.metadata.art_sources import ART_CAPABLE_SOURCES + +logger = get_logger("metadata.art_lookup") + +# Sources that need no account/config — always offered. +_FREE_SOURCES = ("caa", "deezer", "itunes", "audiodb") + + +# --------------------------------------------------------------------------- +# Availability +# --------------------------------------------------------------------------- + + +def _spotify_available() -> bool: + """Spotify art is only usable when the user is connected. Reuse the + canonical accessor, which returns a client only when authenticated.""" + try: + from core.metadata.registry import get_client_for_source + return get_client_for_source("spotify") is not None + except Exception: + return False + + +_AVAILABILITY: Dict[str, Callable[[], bool]] = { + "spotify": _spotify_available, +} + + +def is_art_source_available(source: str) -> bool: + """Is ``source`` usable right now? Free sources are always available; + account sources defer to their connection check. Unknown/unsupported + sources are never available.""" + name = (source or "").strip().lower() + if name not in ART_CAPABLE_SOURCES: + return False + if name in _FREE_SOURCES: + return True + check = _AVAILABILITY.get(name) + return bool(check()) if check else False + + +def available_art_sources() -> List[str]: + """The supported art sources currently usable for this user, in the + default priority order — for populating the settings UI.""" + return [s for s in ART_CAPABLE_SOURCES if is_art_source_available(s)] + + +# --------------------------------------------------------------------------- +# Album-match validation. Every client's search returns its top hit +# unvalidated (results[0]), so a source that lacks the album could hand back a +# DIFFERENT one — embedding wrong-album art, which is worse than the download's +# own cover. We therefore confirm the returned album matches the request before +# trusting its art; a mismatch returns None so the resolver falls through, +# preserving the "worst case = the cover you'd get today" guarantee. +# --------------------------------------------------------------------------- + +_STOPWORDS = {"the", "a", "an"} + + +def _norm(s) -> str: + return re.sub(r"[^a-z0-9]+", " ", str(s or "").lower()).strip() + + +def _significant_tokens(s) -> set: + return {t for t in _norm(s).split() if t not in _STOPWORDS} + + +def _result_album_artist(obj): + """Best-effort (album_name, artist_name) from a search result — handles + both raw dicts (Deezer/AudioDB) and dataclasses (iTunes/Spotify).""" + if isinstance(obj, dict): + name = obj.get("title") or obj.get("name") or obj.get("strAlbum") or "" + artist = obj.get("strArtist") or "" + a = obj.get("artist") + if not artist and isinstance(a, dict): + artist = a.get("name", "") + elif not artist and isinstance(a, str): + artist = a + return name, artist + name = getattr(obj, "name", None) or getattr(obj, "title", None) or "" + arts = getattr(obj, "artists", None) + if arts is None: + arts = getattr(obj, "artist", None) + if isinstance(arts, (list, tuple)): + artist = ", ".join(str(x) for x in arts if x) + else: + artist = str(arts) if arts else "" + return name, artist + + +def _album_matches(req_artist, req_album, got_artist, got_album) -> bool: + """True when the returned album plausibly IS the requested one. + + Both album and artist are compared by significant-token subset (stopwords + dropped), which tolerates leading articles, word order, "(Deluxe)"/ + "- Remastered" suffixes, punctuation, and "feat."/"&"/multi-artist ordering. + Lenient enough to never reject a genuine hit — a false reject just falls + back to today's art — yet strict enough to drop a different album (the + generic-title case, e.g. two "Greatest Hits", is caught by the artist gate).""" + ra, ga = _significant_tokens(req_album), _significant_tokens(got_album) + if not ra or not ga: + return False + if not (ra <= ga or ga <= ra): + return False + ta, tg = _significant_tokens(req_artist), _significant_tokens(got_artist) + if not ta: + return True # requested artist unknown -> album match suffices + if not tg: + return False # asked for an artist, none returned -> can't confirm + return ta <= tg or tg <= ta or bool(ta & tg) + + +# --------------------------------------------------------------------------- +# Per-source lookups — each returns a cover URL or None, never raises. +# Non-CAA sources validate the returned album before trusting its art. +# --------------------------------------------------------------------------- + + +def _caa_art(artist: str, album: str, metadata: dict) -> Optional[str]: + # Cover Art Archive is keyed by the MusicBrainz release id, so it's THE + # release's art by definition — no fuzzy match to validate. Resolves only + # once MusicBrainz enrichment has found the release. + mbid = (metadata or {}).get("musicbrainz_release_id") + if not mbid: + return None + return f"https://coverartarchive.org/release/{mbid}/front-1200" + + +def _deezer_art(artist: str, album: str, metadata: dict) -> Optional[str]: + from core.metadata.registry import get_deezer_client + client = get_deezer_client() + if not client: + return None + data = client.search_album(artist, album) + if not data: + return None + got_album, got_artist = _result_album_artist(data) + if not _album_matches(artist, album, got_artist, got_album): + return None + url = data.get("cover_xl") or data.get("cover_big") or data.get("cover_medium") + if not url: + return None + try: + from core.deezer_client import _upgrade_deezer_cover_url + return _upgrade_deezer_cover_url(url) + except Exception: + return url + + +def _itunes_art(artist: str, album: str, metadata: dict) -> Optional[str]: + from core.metadata.registry import get_itunes_client + client = get_itunes_client() + if not client: + return None + for alb in (client.search_albums(f"{artist} {album}") or []): + url = getattr(alb, "image_url", None) + if not url: + continue + got_album, got_artist = _result_album_artist(alb) + if _album_matches(artist, album, got_artist, got_album): + # iTunes serves any size via the WxH segment — request the max so + # iTunes contributes high-res art, not the 600px default. + return re.sub(r"/\d+x\d+bb\.", "/3000x3000bb.", url) + return None + + +def _audiodb_art(artist: str, album: str, metadata: dict) -> Optional[str]: + from core.audiodb_client import AudioDBClient + data = AudioDBClient().search_album(artist, album) + if not data: + return None + got_album, got_artist = _result_album_artist(data) + if not _album_matches(artist, album, got_artist, got_album): + return None + return data.get("strAlbumThumb") or None + + +def _spotify_art(artist: str, album: str, metadata: dict) -> Optional[str]: + from core.metadata.registry import get_client_for_source + client = get_client_for_source("spotify") + if not client: + return None + for alb in (client.search_albums(f"{artist} {album}") or []): + url = getattr(alb, "image_url", None) + if not url: + continue + got_album, got_artist = _result_album_artist(alb) + if _album_matches(artist, album, got_artist, got_album): + return url + return None + + +_LOOKUPS: Dict[str, Callable[[str, str, dict], Optional[str]]] = { + "caa": _caa_art, + "deezer": _deezer_art, + "itunes": _itunes_art, + "audiodb": _audiodb_art, + "spotify": _spotify_art, +} + + +def select_preferred_art_url( + artist: Optional[str], + album: Optional[str], + metadata: Optional[dict], + configured_order, + validate: Optional[Callable[[str, str], bool]] = None, +) -> Optional[str]: + """Pick a cover-art URL from the user's configured source order, or None. + + ``None`` means "feature off, or nothing in the list resolved" — the caller + then keeps its existing art (today's behavior), so the worst case is simply + the cover you'd get today. This is the single entry point the art pipeline + calls; it's a no-op (returns ``None`` immediately) unless ``album_art_order`` + is an explicit non-empty list, which keeps every existing install untouched. + + ``validate(source, url)`` is an optional gate forwarded to the resolver — the + art pipeline passes one that fetches the candidate and rejects images below a + minimum resolution, so a too-small cover (e.g. a low-res Cover Art Archive + upload) is skipped and the next source is tried instead of winning by + priority alone. + """ + if not isinstance(configured_order, (list, tuple)) or not configured_order: + return None + from core.metadata.art_sources import effective_art_order, resolve_cover_art + order = [s for s in effective_art_order(configured_order) if is_art_source_available(s)] + if not order: + return None + lookup = build_art_lookup(artist or "", album or "", metadata or {}) + url, _src = resolve_cover_art(order, lookup, validate=validate) + return url + + +def build_art_lookup( + artist: str, + album: str, + metadata: Optional[dict] = None, +) -> Callable[[str], Optional[str]]: + """Return a ``source_name -> cover_url | None`` callable for one album, + suitable to pass straight to ``art_sources.resolve_cover_art``. Results are + cached per source so re-resolving across an album's tracks costs at most one + lookup per source, and every lookup is guarded (errors → None).""" + meta = metadata or {} + cache: Dict[str, Optional[str]] = {} + + def lookup(source: str) -> Optional[str]: + name = (source or "").strip().lower() + if name in cache: + return cache[name] + fn = _LOOKUPS.get(name) + url: Optional[str] = None + if fn is not None: + try: + url = fn(artist, album, meta) + except Exception as exc: + logger.debug("[art] %s lookup failed: %s", name, exc) + url = None + cache[name] = url + return url + + return lookup diff --git a/core/metadata/art_sources.py b/core/metadata/art_sources.py new file mode 100644 index 00000000..98dfdd3e --- /dev/null +++ b/core/metadata/art_sources.py @@ -0,0 +1,106 @@ +"""Cover-art source selection. + +Picks album cover art from a user-ordered list of sources, falling back down +the list and finally to the download's own art when nothing in the list +resolves. This generalizes the legacy single ``prefer_caa_art`` toggle into an +ordered, mix-and-match preference (Sokhi's request) while preserving today's +behavior byte-for-byte when no order is configured. + +The module is deliberately pure and import-light: the ordering + fallback +contract is unit-testable without network, config, or a DB. The actual +per-source art lookups are injected as callables (a registry the caller +builds), so this module never imports a metadata client — that keeps the +selection logic fast to test and impossible to break with a client-side +network change. +""" + +from __future__ import annotations + +from typing import Callable, Optional, Sequence, Tuple + +# Sources we can reliably pull album cover art from, in a sensible default +# priority. This is the set the UI offers and that ``effective_art_order`` +# accepts — anything else in a saved order is filtered out. +# +# Genius (lyrics) and Last.fm (deprecated/unreliable images) are excluded — no +# dependable album covers. Tidal/Qobuz/HiFi are deferred: their album lookups +# return IDs that need cover-URL construction and they lack a clean core-side +# client accessor, so rather than ship extraction that silently yields nothing +# we add them once that's verified. The current set covers the universally +# available free sources plus Spotify (the common connected account source). +ART_CAPABLE_SOURCES: Tuple[str, ...] = ( + "caa", "deezer", "itunes", "spotify", "audiodb", +) + +# Minimum byte size for a fetched image to count as a real cover. Mirrors the +# existing Cover Art Archive guard in ``artwork.py`` (a 1x1 pixel, a +# placeholder, or a truncated download is a miss, not a hit). +MIN_VALID_ART_BYTES = 1000 + + +def effective_art_order( + order, + *, + prefer_caa_art: bool = False, +) -> list: + """Resolve the configured art-source order into a concrete priority list. + + Rules (in priority): + - A configured non-empty list wins. It's lower-cased, trimmed, filtered to + known art-capable sources, and de-duplicated (first occurrence kept). + - An empty / missing / all-invalid list preserves **legacy behavior**: + ``['caa']`` when ``prefer_caa_art`` is on (Cover Art Archive first, then + the download's own art), else ``[]`` (use the download's own art only). + + The empty-list case is what makes the feature non-breaking: an install that + has never touched the new setting resolves to exactly today's logic. + """ + if isinstance(order, (list, tuple)): + seen = set() + deduped = [] + for raw in order: + name = str(raw).strip().lower() + if not name or name not in ART_CAPABLE_SOURCES or name in seen: + continue + seen.add(name) + deduped.append(name) + if deduped: + return deduped + return ["caa"] if prefer_caa_art else [] + + +def resolve_cover_art( + order: Sequence[str], + lookup: Callable[[str], Optional[str]], + *, + validate: Optional[Callable[[str, str], bool]] = None, +) -> Tuple[Optional[str], Optional[str]]: + """Walk ``order`` and return ``(art_url, source_name)`` for the first source + whose ``lookup(source)`` yields a URL that passes ``validate`` (if given). + + Returns ``(None, None)`` when nothing in the list resolves — the caller then + falls back to the download's own art (today's default), so art is never + *worse* than before. + + Robustness contract: + - ``lookup`` is ``source_name -> url | None``. A source that returns a + falsy value is skipped. + - An exception raised by ``lookup`` or ``validate`` for one source is + swallowed and treated as a miss, so a single flaky source can never + abort the whole chain (or the download). + """ + for source in order: + try: + url = lookup(source) + except Exception: + url = None + if not url: + continue + if validate is not None: + try: + if not validate(source, url): + continue + except Exception: + continue + return url, source + return None, None diff --git a/core/metadata/artwork.py b/core/metadata/artwork.py index 0da28c58..9b8386af 100644 --- a/core/metadata/artwork.py +++ b/core/metadata/artwork.py @@ -8,7 +8,7 @@ import urllib.request from ipaddress import ip_address from urllib.parse import quote, urlparse -from core.imports.context import get_import_context_album +from core.imports.context import get_import_context_album, get_import_context_artist from core.metadata.common import ( get_config_manager, get_image_dimensions, @@ -330,6 +330,35 @@ def _fetch_art_bytes(art_url: str): return None, None +def _min_size_art_validator(min_px): + """Build a ``(validate, cache)`` pair for the preferred-art resolver. + + ``validate(source, url)`` fetches the candidate cover, caches its bytes (so + the winning source isn't fetched twice), and accepts it only when its + shortest side is at least ``min_px``. A too-small cover — e.g. a low-res + Cover Art Archive upload — is rejected so the resolver falls through to the + next source instead of letting it win on priority alone. Images whose + dimensions can't be read are accepted (don't over-reject; the fallback is + still today's art). ``min_px <= 0`` disables the size gate entirely. + """ + cache = {} + + def validate(_source, url): + res = _fetch_art_bytes(url) + cache[url] = res + data = res[0] if res else None + if not data: + return False + if not min_px or min_px <= 0: + return True + dims = get_image_dimensions(data) + if not dims: + return True + return min(dims[0] or 0, dims[1] or 0) >= min_px + + return validate, cache + + def embed_album_art_metadata(audio_file, metadata: dict): cfg = get_config_manager() symbols = get_mutagen_symbols() @@ -340,8 +369,31 @@ def embed_album_art_metadata(audio_file, metadata: dict): image_data = None mime_type = None + # User-preferred cover-art source. When album_art_order is a non-empty + # list it is the SOLE authority for preferred art (put 'caa' in it to use + # Cover Art Archive), and the legacy prefer_caa_art toggle below is + # skipped. With no list this is a no-op and behavior is exactly as before. + album_art_order = cfg.get("metadata_enhancement.album_art_order") + art_list_active = isinstance(album_art_order, (list, tuple)) and len(album_art_order) > 0 + try: + from core.metadata.art_lookup import select_preferred_art_url + _validate, _art_cache = _min_size_art_validator( + cfg.get("metadata_enhancement.min_art_size", 1000)) + preferred_url = select_preferred_art_url( + metadata.get("album_artist") or metadata.get("artist"), + metadata.get("album"), + metadata, + album_art_order, + validate=_validate, + ) + if preferred_url: + cached = _art_cache.get(preferred_url) + image_data, mime_type = cached if (cached and cached[0]) else _fetch_art_bytes(preferred_url) + except Exception as exc: + logger.debug("Preferred art-source selection failed: %s", exc) + release_mbid = metadata.get("musicbrainz_release_id") - if release_mbid and cfg.get("metadata_enhancement.prefer_caa_art", False): + if not image_data and not art_list_active and release_mbid and cfg.get("metadata_enhancement.prefer_caa_art", False): try: # 1200px CDN thumbnail, not the flaky bare /front original. caa_url = f"https://coverartarchive.org/release/{release_mbid}/front-1200" @@ -395,7 +447,12 @@ def download_cover_art(album_info: dict, target_dir: str, context: dict = None): cover_path = os.path.join(target_dir, "cover.jpg") album_info = album_info or {} release_mbid = album_info.get("musicbrainz_release_id") - prefer_caa = cfg.get("metadata_enhancement.prefer_caa_art", False) + # When a preferred-art priority list is configured it is the sole + # authority, so the legacy CAA toggle is neutralized for this whole + # function (it gates the existing-file upgrade logic too). + _art_order = cfg.get("metadata_enhancement.album_art_order") + _art_list_active = isinstance(_art_order, (list, tuple)) and len(_art_order) > 0 + prefer_caa = cfg.get("metadata_enhancement.prefer_caa_art", False) and not _art_list_active if os.path.exists(cover_path): if release_mbid and prefer_caa: @@ -412,7 +469,31 @@ def download_cover_art(album_info: dict, target_dir: str, context: dict = None): is_upgrade = False image_data = None - if release_mbid and prefer_caa: + + # User-preferred cover-art source (no-op unless album_art_order is set). + # cover.jpg only supports the artist+album sources here (no MBID in + # album_info), which matches today's CAA-only special-casing. + try: + from core.metadata.art_lookup import select_preferred_art_url + artist_ctx = get_import_context_artist(context) if context else {} + _validate, _art_cache = _min_size_art_validator( + cfg.get("metadata_enhancement.min_art_size", 1000)) + preferred_url = select_preferred_art_url( + (artist_ctx or {}).get("name"), + album_info.get("album_name"), + album_info, + cfg.get("metadata_enhancement.album_art_order"), + validate=_validate, + ) + if preferred_url: + cached = _art_cache.get(preferred_url) + pref_data = cached[0] if (cached and cached[0]) else _fetch_art_bytes(preferred_url)[0] + if pref_data and len(pref_data) > 1000: + image_data = pref_data + except Exception as exc: + logger.debug("Preferred art-source selection failed: %s", exc) + + if not image_data and release_mbid and prefer_caa: try: # 1200px CDN thumbnail, not the flaky bare /front original. caa_url = f"https://coverartarchive.org/release/{release_mbid}/front-1200" diff --git a/core/metadata/cache.py b/core/metadata/cache.py index b49daae5..1d296da8 100644 --- a/core/metadata/cache.py +++ b/core/metadata/cache.py @@ -14,6 +14,26 @@ from typing import Optional, Dict, List, Tuple logger = logging.getLogger(__name__) +# Hard ceiling on cached entity rows. TTL-only eviction has no upper bound, so +# heavy discovery/enrichment within the TTL window let this table reach ~1.8M +# rows / 7.6 GB (and helped bloat the main DB toward a corruption incident). +# Beyond this cap we evict least-recently-ACCESSED rows (LRU) — the data to do +# it (last_accessed_at) is already stored. Tunable via config; 0 disables. +_DEFAULT_MAX_CACHE_ENTITIES = 250_000 + + +def entities_to_evict_for_capacity(total_rows: int, max_rows: int) -> int: + """Pure decision: how many LRU rows to drop to bring the cache to ``max_rows``. + + Separated from SQL so it's unit-testable (mirrors core.db_integrity. + prune_backups). ``max_rows <= 0`` means "no cap" -> never evict. Never + returns negative. + """ + if max_rows <= 0: + return 0 + return max(0, total_rows - max_rows) + + # Singleton _cache_instance = None _cache_lock = threading.Lock() @@ -619,6 +639,43 @@ class MetadataCache: return self._run_maintenance_write("Cache eviction", _operation) + def evict_over_capacity(self, max_rows: int = _DEFAULT_MAX_CACHE_ENTITIES) -> int: + """Enforce a hard row ceiling: if the entity cache exceeds ``max_rows``, + delete the least-recently-ACCESSED rows down to the cap (LRU). This is + the bound TTL-only eviction lacked. Returns the count evicted. + + Runs AFTER evict_expired in the maintenance job, so TTL-expired and junk + rows are already gone — this only trims a still-oversized healthy cache. + """ + def _operation(conn): + cursor = conn.cursor() + total = cursor.execute( + "SELECT COUNT(*) FROM metadata_cache_entities" + ).fetchone()[0] + to_evict = entities_to_evict_for_capacity(total, max_rows) + if to_evict <= 0: + return 0 + # Delete the oldest-accessed rows. NULL last_accessed_at sorts first + # (evicted earliest) — those were never touched since insert. + cursor.execute(""" + DELETE FROM metadata_cache_entities + WHERE id IN ( + SELECT id FROM metadata_cache_entities + ORDER BY last_accessed_at ASC + LIMIT ? + ) + """, (to_evict,)) + evicted = cursor.rowcount + conn.commit() + if evicted > 0: + logger.info( + "Cache over capacity (%d > %d) — evicted %d least-recently-used entities", + total, max_rows, evicted, + ) + return evicted + + return self._run_maintenance_write("Cache capacity eviction", _operation) + def clean_junk_entities(self) -> int: """Delete cached entities with empty/placeholder names.""" def _operation(conn): diff --git a/core/musicbrainz_client.py b/core/musicbrainz_client.py index a802373d..f8c4a178 100644 --- a/core/musicbrainz_client.py +++ b/core/musicbrainz_client.py @@ -7,6 +7,19 @@ from utils.logging_config import get_logger logger = get_logger("musicbrainz_client") +# Lucene query-syntax characters that must be backslash-escaped when a +# user-supplied value is interpolated into a query (e.g. artist names like +# "Sunn O)))", "Anthony Green (Saosin)", "Therapy?", "!!!"). Without this an +# unbalanced paren or a stray ?/* either breaks the field group (returning +# unrelated results) or yields zero hits. +_LUCENE_SPECIAL = set('+-&|!(){}[]^"~*?:\\/') + + +def _escape_lucene(text: str) -> str: + """Backslash-escape Lucene special characters in a user-supplied term.""" + return ''.join('\\' + ch if ch in _LUCENE_SPECIAL else ch for ch in text) + + # Global rate limiting variables _last_api_call_time = 0 _api_call_lock = threading.Lock() @@ -158,14 +171,14 @@ class MusicBrainzClient: safe_artist = artist_name.replace('\\', '\\\\').replace('"', '\\"') query += f' AND artist:"{safe_artist}"' else: - # Bare query — MB tokenizes against title + artist credit + - # alias + sortname indexes together with diacritic folding. - # Recovers cases like "Bjork" → "Björk" that strict phrase - # queries miss. - parts = [album_name] - if artist_name: - parts.append(artist_name) - query = ' '.join(p for p in parts if p) + # Loose title terms (no phrase quotes → diacritic folding and + # alias/sortname recall, e.g. "Bjork" → "Björk"), but + # FIELD-SCOPE the artist so it constrains rather than floating + # as a free fuzzy term that lets unrelated releases whose titles + # echo the artist name rank first (same root cause as #754). + query = album_name + if artist_name and artist_name.strip(): + query += f' AND artist:({_escape_lucene(artist_name.strip())})' params = { 'query': query, @@ -223,18 +236,24 @@ class MusicBrainzClient: safe_artist = artist_name.replace('\\', '\\\\').replace('"', '\\"') query += f' AND artist:"{safe_artist}"' else: - # Bare query — see search_release for rationale. - parts = [track_name] - if artist_name: - parts.append(artist_name) - query = ' '.join(p for p in parts if p) + # Loose track terms (no phrase quotes → tolerant of bracketed + # suffixes and diacritics, hitting MB's alias/sortname indexes), + # but FIELD-SCOPE the artist so it actually constrains results. + # A bare "track artist" blob let the artist float as a free + # fuzzy term, so covers/karaoke whose TITLES contain the artist + # name outranked the real recording (#754: "Sweet Child O Mine" + # / "Guns N Roses" returned only covers). Scoping still folds + # diacritics — artist:(Bjork) matches "Björk". + query = track_name + if artist_name and artist_name.strip(): + query += f' AND artist:({_escape_lucene(artist_name.strip())})' params = { 'query': query, 'fmt': 'json', 'limit': limit } - + response = self.session.get( f"{self.BASE_URL}/recording", params=params, diff --git a/core/playback/__init__.py b/core/playback/__init__.py new file mode 100644 index 00000000..a199965a --- /dev/null +++ b/core/playback/__init__.py @@ -0,0 +1 @@ +"""Playback helpers (web-player play logging).""" diff --git a/core/playback/play_log.py b/core/playback/play_log.py new file mode 100644 index 00000000..8b2387a1 --- /dev/null +++ b/core/playback/play_log.py @@ -0,0 +1,65 @@ +"""Build a listening-history event from a SoulSync web-player play. + +Pure, DB-agnostic. ``listening_history`` is otherwise populated only from the +media server (Plex/Jellyfin/Navidrome) by ``listening_stats_worker``; this lets +the WEB PLAYER record its own plays too, so "recently played" and the Phase-2 +smart-radio recency signal reflect what was actually played in SoulSync. + +Kept as a pure function so it's unit-testable without a DB or Flask: it +normalizes the player's track payload into the exact event shape +``MusicDatabase.insert_listening_events`` expects. +""" + +from __future__ import annotations + +from typing import Any, Dict, Optional + +# Marks rows that came from the SoulSync web player (vs a media server), so they +# can be distinguished in queries / dedup if ever needed. +WEB_PLAYER_SOURCE = "soulsync_web" + + +def build_play_event(track: Dict[str, Any], played_at: str, + duration_ms: int = 0) -> Optional[Dict[str, Any]]: + """Normalize a player track payload into a listening_history event. + + ``played_at`` MUST be supplied by the caller (an ISO timestamp string) — + this module never reads the clock, so it stays pure/testable. Returns None + when there's nothing worth logging (no title), so callers can skip cleanly. + + The event shape matches insert_listening_events(): + track_id, title, artist, album, played_at, duration_ms, server_source, + db_track_id. + """ + if not isinstance(track, dict): + return None + title = (track.get("title") or "").strip() + if not title: + return None + + # db_track_id is the local tracks.id when it's a real library track (a + # plain integer id). Streamed/search results may carry a composite id — + # keep it only when it's a clean int so the FK-ish join stays valid. + raw_id = track.get("id") + db_track_id = int(raw_id) if _is_int_like(raw_id) else None + + return { + "track_id": str(raw_id) if raw_id is not None else None, + "title": title, + "artist": (track.get("artist") or "").strip(), + "album": (track.get("album") or "").strip(), + "played_at": played_at, + "duration_ms": int(duration_ms) if _is_int_like(duration_ms) else 0, + "server_source": WEB_PLAYER_SOURCE, + "db_track_id": db_track_id, + } + + +def _is_int_like(v: Any) -> bool: + if isinstance(v, bool): + return False + if isinstance(v, int): + return True + if isinstance(v, str): + return v.isdigit() + return False diff --git a/core/radio/__init__.py b/core/radio/__init__.py new file mode 100644 index 00000000..e43912f6 --- /dev/null +++ b/core/radio/__init__.py @@ -0,0 +1,28 @@ +"""Radio / auto-play recommendation logic. + +Pure, DB-agnostic helpers that decide *what* radio should play. The SQL +execution stays in ``database.music_database.get_radio_tracks``; this package +owns the decisions (tag parsing, tier caps, dedup/collection, LIKE-condition +building) so they're unit-testable without a live DB — the seam Phase 2's +smarter ranking will plug into. +""" + +from core.radio.selection import ( + RadioCollector, + build_like_conditions, + merge_tags, + parse_tags, + rank_candidates, + same_artist_cap, + score_candidate, +) + +__all__ = [ + "RadioCollector", + "build_like_conditions", + "merge_tags", + "parse_tags", + "rank_candidates", + "same_artist_cap", + "score_candidate", +] diff --git a/core/radio/selection.py b/core/radio/selection.py new file mode 100644 index 00000000..93637165 --- /dev/null +++ b/core/radio/selection.py @@ -0,0 +1,222 @@ +"""Pure radio-selection decisions, lifted out of the DB layer. + +``database.music_database.get_radio_tracks`` used to inline all of this between +``cursor.execute`` calls, so the algorithm couldn't be tested without a live DB +(which also happens to throw in the dev sandbox). These helpers carry the same +behavior as before — they're a faithful extraction, not a rewrite — but as +plain functions they're unit-testable and give Phase 2 (smart ranking) a clean +place to evolve the logic. + +Nothing here touches sqlite; callers pass already-fetched rows (as dicts) and +get back decisions. +""" + +from __future__ import annotations + +import hashlib +import json +import math +from typing import Any, Dict, Iterable, List, Optional, Sequence, Tuple + + +def parse_tags(raw_val: Any) -> List[str]: + """Parse a genre/mood/style field into a list of tags. + + The field may be a JSON array (canonical) or a legacy comma-separated + string. Mirrors the inline ``_parse_tags`` the DB method used. + """ + if not raw_val: + return [] + try: + parsed = json.loads(raw_val) + return parsed if isinstance(parsed, list) else [str(parsed)] + except (json.JSONDecodeError, ValueError, TypeError): + return [t.strip() for t in str(raw_val).split(",") if t.strip()] + + +def same_artist_cap(limit: int) -> int: + """How many same-artist tracks tier 1 may contribute. + + Capped so radio doesn't become an all-one-artist playlist: 30% of the + limit, floored at 5 (matches the original ``max(5, limit * 3 // 10)``). + """ + return max(5, limit * 3 // 10) + + +def merge_tags(*tag_groups: Iterable[str]) -> List[str]: + """Concatenate tag lists, dedupe, preserve first-seen order. + + Mirrors ``list(dict.fromkeys(a + b))`` used for genre/mood/style merges. + """ + merged: List[str] = [] + for group in tag_groups: + for tag in group: + merged.append(tag) + return list(dict.fromkeys(merged)) + + +def build_like_conditions( + tags: Sequence[str], columns: Sequence[str] +) -> Tuple[str, List[str]]: + """Build an OR-of-LIKEs SQL fragment + params for matching ``tags`` + against each of ``columns``. + + Returns ``(sql_fragment, params)`` where the fragment is + ``"col1 LIKE ? OR col1 LIKE ? OR col2 LIKE ? ..."`` (one LIKE per + column per tag) and params are the ``%tag%`` wildcards in matching + order. Returns ``("", [])`` when there are no tags or no columns, so + callers can skip the tier cleanly. + + This reproduces the original per-tier condition building, which paired + every tag against album-level and artist-level columns. + """ + if not tags or not columns: + return "", [] + conditions: List[str] = [] + params: List[str] = [] + # Group by column (all tags for column A, then all tags for column B) to + # match the original ordering: it emitted every ``al. LIKE ?`` then + # every ``ar. LIKE ?``, with params being ``[%tag%...] * 2``. + for col in columns: + for tag in tags: + conditions.append(f"{col} LIKE ?") + params.append(f"%{tag}%") + return " OR ".join(conditions), params + + +class RadioCollector: + """Accumulates radio candidates across tiers with dedup + cap logic. + + Replaces the inline ``collected`` list + ``seen_ids`` set + ``_collect`` + closure the DB method used. Construct with the overall ``limit`` and the + set of IDs to exclude up front (seed track + caller-supplied), then feed + each tier's fetched rows through :meth:`collect`. + """ + + def __init__(self, limit: int, exclude_ids: Optional[Iterable[Any]] = None): + self.limit = limit + self._collected: List[Dict[str, Any]] = [] + # seen_ids seeds with the exclude set so excluded tracks never collect + # AND so the placeholders/values used in WHERE ... NOT IN stay in sync. + self._seen: set[str] = {str(e) for e in (exclude_ids or [])} + + @property + def tracks(self) -> List[Dict[str, Any]]: + return self._collected + + @property + def filled(self) -> bool: + """True once we've reached the overall limit.""" + return len(self._collected) >= self.limit + + def exclude_placeholders(self) -> str: + """SQL ``?,?,...`` placeholder string sized to the current seen set.""" + return ",".join("?" * len(self._seen)) + + def exclude_values(self) -> List[str]: + """Param values for the placeholders above (current seen set).""" + return list(self._seen) + + def remaining(self) -> int: + """How many more tracks are needed to hit the limit.""" + return max(0, self.limit - len(self._collected)) + + def collect( + self, + rows: Iterable[Dict[str, Any]], + cap: Optional[int] = None, + *, + rank: bool = False, + ) -> bool: + """Append ``rows`` (dict-like) to the result, skipping already-seen IDs. + + ``cap`` bounds how many THIS call may add (on top of what's already + collected); ``None`` means bounded only by the overall limit. Returns + True once the overall limit is reached. Mirrors the original + ``_collect`` closure exactly. + + When ``rank`` is True the (still-unseen) rows are scored and sorted by + :func:`rank_candidates` before being appended — so a tier hands the DB + a generous random pool and this picks the best of it (Phase 2 smart + radio). When False the rows are taken in the order given (the original + behavior, preserved for any caller that wants it). + """ + target = min(self.limit, len(self._collected) + cap) if cap else self.limit + fresh = [dict(r) for r in rows if str(r["id"]) not in self._seen] + if rank: + fresh = rank_candidates(fresh) + for r in fresh: + rid = str(r["id"]) + if rid in self._seen: + continue + self._seen.add(rid) + self._collected.append(r) + if len(self._collected) >= target: + return True + return self.filled + + +# ── Phase 2: smart ranking ───────────────────────────────────────────────── +# +# The old radio was pure ``ORDER BY RANDOM()``. We now fetch a generous random +# POOL per tier (so the SQL stays cheap and varied) and rank it here by a +# weighted score. All the intelligence is in these pure functions so it's +# unit-testable and tunable without touching SQL. + +# Weights are deliberately modest so popularity guides but doesn't dominate — +# radio should still surface lesser-played tracks, just less often. +_W_PLAY_COUNT = 1.0 # local play_count (log-damped) +_W_LASTFM = 0.5 # lastfm_playcount (log-damped) — global popularity hint +_W_RECENCY_PENALTY = 2.0 # subtracted for very-recently-played (avoid repeats) +_W_JITTER = 1.0 # deterministic per-track noise so runs vary a little + + +def _log_damp(value: Any) -> float: + """log1p of a non-negative count, 0 for missing/invalid. Damps so a track + with 10000 plays doesn't bury one with 50 — popularity is a nudge.""" + try: + v = float(value) + except (TypeError, ValueError): + return 0.0 + if v <= 0: + return 0.0 + return math.log1p(v) + + +def _stable_jitter(track_id: Any) -> float: + """Deterministic [0,1) pseudo-random per track id. + + Math.random / Date are unavailable / nondeterministic-unfriendly here and + we want runs to be reproducible in tests, so derive jitter from a hash of + the id. Two different tracks get different jitter; the same track is stable + within a ranking pass. + """ + h = hashlib.sha1(str(track_id).encode("utf-8")).hexdigest() + return int(h[:8], 16) / 0xFFFFFFFF + + +def score_candidate(row: Dict[str, Any]) -> float: + """Weighted desirability score for a radio candidate row. + + Signals (all optional — absent columns score 0, so this is safe on a DB + that hasn't recorded listening data yet): + + play_count local plays (log-damped) + + lastfm_playcount global popularity hint (log-damped) + - recently_played caller-flagged repeat-risk → penalty + + jitter stable per-track noise for run-to-run variety + """ + score = 0.0 + score += _W_PLAY_COUNT * _log_damp(row.get("play_count")) + score += _W_LASTFM * _log_damp(row.get("lastfm_playcount")) + if row.get("_recently_played"): + score -= _W_RECENCY_PENALTY + score += _W_JITTER * _stable_jitter(row.get("id")) + return score + + +def rank_candidates(rows: Sequence[Dict[str, Any]]) -> List[Dict[str, Any]]: + """Sort candidate rows best-first by :func:`score_candidate`. + + Stable sort; ties keep input order. Does not mutate the input rows. + """ + return sorted(rows, key=score_candidate, reverse=True) diff --git a/core/repair_jobs/cache_evictor.py b/core/repair_jobs/cache_evictor.py index e2559fc9..6fa22c17 100644 --- a/core/repair_jobs/cache_evictor.py +++ b/core/repair_jobs/cache_evictor.py @@ -89,4 +89,21 @@ class CacheEvictorJob(RepairJob): logger.error("MB null cleanup failed: %s", e, exc_info=True) result.errors += 1 + if context.check_stop(): + return result + + # Phase 5: Hard capacity cap (LRU). Runs LAST so TTL/junk/orphan/null + # rows are already gone; this only trims a still-oversized HEALTHY cache + # down to the row ceiling — the bound TTL-only eviction never had (the + # cache previously reached ~1.8M rows / 7.6 GB). + try: + over = cache.evict_over_capacity() + result.auto_fixed += over + result.scanned += over + if over > 0: + logger.info("Phase 5 — capacity cap: evicted %d LRU entries", over) + except Exception as e: + logger.error("Capacity eviction failed: %s", e, exc_info=True) + result.errors += 1 + return result diff --git a/core/repair_worker.py b/core/repair_worker.py index 9af2ff30..87ba8abb 100644 --- a/core/repair_worker.py +++ b/core/repair_worker.py @@ -1364,8 +1364,11 @@ class RepairWorker: return {'success': False, 'error': f'Selected track ID {keep_id} not found in duplicates'} best_id = keep_id else: - # Auto-pick: highest bitrate, then longest duration, then highest track number (correct > 01) - best = max(tracks, key=lambda t: (t.get('bitrate', 0) or 0, t.get('duration', 0) or 0, t.get('track_number', 0) or 0)) + # Auto-pick the keeper: lossless format first (so a FLAC beats an + # MP3 even when the FLAC's bitrate is missing in the DB), then + # bitrate, duration, and track number as tie-breakers. + from core.library.duplicate_keep import pick_duplicate_to_keep + best = pick_duplicate_to_keep(tracks) best_id = best.get('track_id') or best.get('id') if not best_id: diff --git a/core/streaming/state.py b/core/streaming/state.py new file mode 100644 index 00000000..a35c7531 --- /dev/null +++ b/core/streaming/state.py @@ -0,0 +1,140 @@ +"""Stream playback state — testable store, foundation for multi-listener. + +Today ``web_server.py`` keeps ONE module-global ``stream_state`` dict + one +``stream_lock`` (``web_server.py:747``). That means the whole server has a +single "currently playing" — every browser tab/device is a remote for the same +playback, and two listeners collide. Fixing that (the player-revamp Phase 3 +goal) requires per-session state, but the global is woven through ~22 call +sites and isn't unit-testable where it lives. + +This module lifts the state into a small, tested abstraction WITHOUT yet +changing behavior: + +* ``StreamSession`` — one playback's state. Behaves like the old dict + (``s["status"]``, ``s.get(...)``, ``s.update({...})``) so existing call sites + work unchanged, but each carries its OWN lock so distinct sessions never + block or clobber each other. +* ``StreamStateStore`` — a registry of named sessions. ``DEFAULT_SESSION`` is + the single shared session that reproduces today's exact behavior; wiring the + web server through it is a no-op refactor. When Phase 3 adds a per-request + session id (browser/device), the store already supports it — that step is the + only remaining (browser-side, unprovable-here) piece. + +Pure Python, no Flask/DB. Fully unit-testable. +""" + +from __future__ import annotations + +import threading +from typing import Any, Dict, Iterator, List, Optional + +DEFAULT_SESSION = "default" + + +def _fresh_state() -> Dict[str, Any]: + """The stopped/empty baseline — matches web_server.py's original literal.""" + return { + "status": "stopped", # stopped | loading | queued | ready | error + "progress": 0, + "track_info": None, + "file_path": None, + "error_message": None, + } + + +class StreamSession: + """One playback session's state, with its own lock. + + Dict-compatible for the operations the existing call sites use + (``__getitem__``, ``__setitem__``, ``get``, ``update``) so lifting the + global is a drop-in. ``lock`` is exposed so callers that did + ``with stream_lock:`` keep that exact guard — now per-session. + """ + + def __init__(self, initial: Optional[Dict[str, Any]] = None): + self._state: Dict[str, Any] = _fresh_state() + if initial: + self._state.update(initial) + self.lock = threading.RLock() + + # -- dict-compatible surface (matches old stream_state usage) -- + def __getitem__(self, key: str) -> Any: + return self._state[key] + + def __setitem__(self, key: str, value: Any) -> None: + self._state[key] = value + + def __contains__(self, key: str) -> bool: + return key in self._state + + def get(self, key: str, default: Any = None) -> Any: + return self._state.get(key, default) + + def update(self, values: Dict[str, Any]) -> None: + self._state.update(values) + + def snapshot(self) -> Dict[str, Any]: + """A shallow copy — for emitting to clients without leaking the live + dict (the old code read individual keys under the lock; a snapshot is + the safe equivalent for the whole thing).""" + return dict(self._state) + + def reset(self) -> None: + """Return to the stopped/empty baseline (used by stop).""" + self._state = _fresh_state() + + def replace(self, new_state: Dict[str, Any]) -> None: + """Wholesale replace the backing dict (mirrors the old + ``_set_stream_state`` global reassignment).""" + self._state = dict(new_state) + + +class StreamStateStore: + """Registry of named :class:`StreamSession` objects. + + ``get()`` lazily creates a session on first reference, so a brand-new + session id just works. The default session reproduces the single-global + behavior the app has today. + """ + + def __init__(self): + self._sessions: Dict[str, StreamSession] = {} + self._registry_lock = threading.RLock() + + def get(self, session_id: str = DEFAULT_SESSION) -> StreamSession: + with self._registry_lock: + session = self._sessions.get(session_id) + if session is None: + session = StreamSession() + self._sessions[session_id] = session + return session + + def has(self, session_id: str) -> bool: + with self._registry_lock: + return session_id in self._sessions + + def drop(self, session_id: str) -> bool: + """Remove a session (e.g. on disconnect). Returns True if one existed. + The default session is never dropped — it's the always-present shared + playback.""" + if session_id == DEFAULT_SESSION: + return False + with self._registry_lock: + return self._sessions.pop(session_id, None) is not None + + def session_ids(self) -> List[str]: + with self._registry_lock: + return list(self._sessions.keys()) + + def active_ids(self) -> List[str]: + """Session ids whose status is not 'stopped' — i.e. currently doing + something. The signal multi-listener UI / cleanup will key off of.""" + with self._registry_lock: + return [ + sid for sid, s in self._sessions.items() + if s.get("status") != "stopped" + ] + + def __iter__(self) -> Iterator[StreamSession]: + with self._registry_lock: + return iter(list(self._sessions.values())) diff --git a/database/music_database.py b/database/music_database.py index 1394719f..85beef86 100644 --- a/database/music_database.py +++ b/database/music_database.py @@ -3654,6 +3654,36 @@ class MusicDatabase: if conn: conn.close() + def record_web_player_play(self, event): + """Record a single SoulSync web-player play: insert the listening_history + row AND bump tracks.play_count / last_played for the smart-radio recency + signal. ``event`` is the dict from core.playback.play_log.build_play_event. + + Returns True if the history row was newly inserted. + """ + if not event: + return False + inserted = self.insert_listening_events([event]) + db_id = event.get('db_track_id') + if db_id is not None: + conn = None + try: + conn = self._get_connection() + cursor = conn.cursor() + cursor.execute(""" + UPDATE tracks + SET play_count = COALESCE(play_count, 0) + 1, + last_played = ? + WHERE id = ? + """, (event.get('played_at'), db_id)) + conn.commit() + except Exception as e: + logger.error(f"Error bumping play_count for track {db_id}: {e}") + finally: + if conn: + conn.close() + return inserted > 0 + def update_track_play_counts(self, counts): """Update play_count and last_played on the tracks table. @@ -4237,9 +4267,17 @@ class MusicDatabase: def _add_metadata_cache_tables(self, cursor): """Create metadata_cache_entities and metadata_cache_searches tables for universal API response caching""" try: + # Skip only when the marker is set AND the tables actually exist. + # A marker-only guard is fragile: if the `metadata` table survives a + # corruption-recovery but the (large) cache tables don't, the stale + # marker would permanently short-circuit creation and the metadata + # cache would silently never work again (nothing lands in the + # browser). Verifying the table presence makes this self-heal. + cursor.execute("SELECT name FROM sqlite_master WHERE type='table' AND name='metadata_cache_entities'") + tables_present = cursor.fetchone() is not None cursor.execute("SELECT value FROM metadata WHERE key = 'metadata_cache_v1' LIMIT 1") - if cursor.fetchone(): - return # Already migrated + if cursor.fetchone() and tables_present: + return # Already migrated and tables present logger.info("Creating metadata cache tables...") @@ -4410,7 +4448,7 @@ class MusicDatabase: logger.error(f"Error creating manual_library_track_matches table: {e}") def save_manual_library_match(self, profile_id: int, source: str, source_track_id: str, - library_track_id: int, **meta) -> bool: + library_track_id: str, **meta) -> bool: """Insert or replace a manual match. meta keys: source_title, source_artist, source_album, source_context_json, server_source.""" try: @@ -12796,24 +12834,53 @@ class MusicDatabase: seed = dict(seed) artist_name = seed['artist_name'] - # Build the set of IDs to exclude (seed + caller-supplied) - excluded = {str(track_id)} + # Selection decisions (dedup, caps, tag parsing, condition + # building) live in core.radio.selection so they're unit- + # testable without a live DB. The cursor work stays here. + from core.radio.selection import ( + RadioCollector, + build_like_conditions, + merge_tags, + parse_tags, + same_artist_cap, + ) + + # Seed + caller-supplied IDs to exclude (seeds the collector's + # seen-set so excluded tracks never collect and the NOT IN + # placeholders/values stay in sync). + exclude_seed = [str(track_id)] if exclude_ids: - excluded.update(str(eid) for eid in exclude_ids) + exclude_seed.extend(str(eid) for eid in exclude_ids) + collector = RadioCollector(limit, exclude_ids=exclude_seed) - collected: list[dict] = [] - seen_ids: set[str] = set(excluded) + # Phase 2 smart radio: each tier pulls a generous RANDOM pool, + # then core.radio.selection ranks it (play_count + lastfm + # popularity, recency penalty, stable jitter) and the collector + # keeps the best. Pool factor keeps SQL cheap while giving the + # ranker real choice; bumped, then floored so small tiers still + # over-fetch a little. + _POOL_FACTOR = 4 - def _exclude_placeholders(): - return ','.join('?' * len(seen_ids)) + def _pool(n): + return max(n * _POOL_FACTOR, n + 10) - def _exclude_values(): - return list(seen_ids) + # Ranking signals (play_count / lastfm_playcount) are added by a + # migration, but probe for them so radio still works on a DB that + # predates it — the ranker treats missing columns as score 0, so + # we simply omit them from the SELECT when absent rather than + # crashing on "no such column". + cursor.execute("PRAGMA table_info(tracks)") + _track_cols = {row[1] for row in cursor.fetchall()} + _rank_cols = "".join( + f"t.{c}, " for c in ("play_count", "lastfm_playcount") + if c in _track_cols + ) - _track_select = """ + _track_select = f""" SELECT t.id, t.title, t.track_number, t.duration, t.file_path, t.bitrate, t.album_id, t.artist_id, + {_rank_cols} al.title AS album, COALESCE(al.thumb_url, ar.thumb_url) AS image_url, ar.name AS artist @@ -12824,98 +12891,72 @@ class MusicDatabase: # Only return tracks that have actual files on disk _file_filter = "t.file_path IS NOT NULL AND t.file_path != ''" - def _collect(rows, cap=None): - """Append rows to collected. Stop at cap or limit.""" - target = min(limit, (len(collected) + cap)) if cap else limit - for row in rows: - r = dict(row) - rid = str(r['id']) - if rid not in seen_ids: - seen_ids.add(rid) - collected.append(r) - if len(collected) >= target: - return True - return len(collected) >= limit - - def _parse_tags(raw_val): - """Parse a JSON array or comma-separated string into a list.""" - if not raw_val: - return [] - try: - parsed = json.loads(raw_val) - return parsed if isinstance(parsed, list) else [str(parsed)] - except (json.JSONDecodeError, ValueError): - return [t.strip() for t in raw_val.split(',') if t.strip()] - # --- 1. Same artist, different albums (capped at 30% of limit) --- - same_artist_cap = max(5, limit * 3 // 10) + artist_cap = same_artist_cap(limit) cursor.execute(f""" {_track_select} - WHERE {_file_filter} AND ar.name = ? AND t.album_id != ? AND t.id NOT IN ({_exclude_placeholders()}) + WHERE {_file_filter} AND ar.name = ? AND t.album_id != ? AND t.id NOT IN ({collector.exclude_placeholders()}) ORDER BY RANDOM() LIMIT ? - """, [artist_name, seed['album_id']] + _exclude_values() + [same_artist_cap]) - _collect(cursor.fetchall(), cap=same_artist_cap) + """, [artist_name, seed['album_id']] + collector.exclude_values() + [_pool(artist_cap)]) + collector.collect(cursor.fetchall(), cap=artist_cap, rank=True) - if len(collected) >= limit: - return {'success': True, 'tracks': collected} + if collector.filled: + return {'success': True, 'tracks': collector.tracks} # --- 2. Same genre (album genres + artist genres, other artists) --- - genre_list = _parse_tags(seed.get('album_genres')) - artist_genre_list = _parse_tags(seed.get('artist_genres')) - all_genres = list(dict.fromkeys(genre_list + artist_genre_list)) # dedupe, preserve order - - if all_genres: - genre_conditions = ' OR '.join( - ['al.genres LIKE ?' for _ in all_genres] + - ['ar.genres LIKE ?' for _ in all_genres] - ) - genre_params = [f'%{g}%' for g in all_genres] * 2 + all_genres = merge_tags( + parse_tags(seed.get('album_genres')), + parse_tags(seed.get('artist_genres')), + ) + genre_conditions, genre_params = build_like_conditions( + all_genres, ('al.genres', 'ar.genres') + ) + if genre_conditions: cursor.execute(f""" {_track_select} WHERE {_file_filter} AND ({genre_conditions}) AND ar.name != ? - AND t.id NOT IN ({_exclude_placeholders()}) + AND t.id NOT IN ({collector.exclude_placeholders()}) ORDER BY RANDOM() LIMIT ? - """, genre_params + [artist_name] + _exclude_values() + [limit - len(collected)]) - if _collect(cursor.fetchall()): - return {'success': True, 'tracks': collected} + """, genre_params + [artist_name] + collector.exclude_values() + [_pool(collector.remaining())]) + if collector.collect(cursor.fetchall(), rank=True): + return {'success': True, 'tracks': collector.tracks} # --- 3. Same mood / style (album + artist level) --- for field_name in ('mood', 'style'): - album_tags = _parse_tags(seed.get(f'album_{field_name}')) - artist_tags = _parse_tags(seed.get(f'artist_{field_name}')) - all_tags = list(dict.fromkeys(album_tags + artist_tags)) - - if all_tags: - tag_conditions = ' OR '.join( - [f'al.{field_name} LIKE ?' for _ in all_tags] + - [f'ar.{field_name} LIKE ?' for _ in all_tags] - ) - tag_params = [f'%{t}%' for t in all_tags] * 2 + all_tags = merge_tags( + parse_tags(seed.get(f'album_{field_name}')), + parse_tags(seed.get(f'artist_{field_name}')), + ) + tag_conditions, tag_params = build_like_conditions( + all_tags, (f'al.{field_name}', f'ar.{field_name}') + ) + if tag_conditions: cursor.execute(f""" {_track_select} WHERE {_file_filter} AND ({tag_conditions}) AND ar.name != ? - AND t.id NOT IN ({_exclude_placeholders()}) + AND t.id NOT IN ({collector.exclude_placeholders()}) ORDER BY RANDOM() LIMIT ? - """, tag_params + [artist_name] + _exclude_values() + [limit - len(collected)]) - if _collect(cursor.fetchall()): - return {'success': True, 'tracks': collected} + """, tag_params + [artist_name] + collector.exclude_values() + [_pool(collector.remaining())]) + if collector.collect(cursor.fetchall(), rank=True): + return {'success': True, 'tracks': collector.tracks} - # --- 4. Random library tracks --- - if len(collected) < limit: + # --- 4. Random library tracks (ranked: popular-but-unheard + # beats pure noise even in the last-resort tier) --- + if not collector.filled: cursor.execute(f""" {_track_select} - WHERE {_file_filter} AND t.id NOT IN ({_exclude_placeholders()}) + WHERE {_file_filter} AND t.id NOT IN ({collector.exclude_placeholders()}) ORDER BY RANDOM() LIMIT ? - """, _exclude_values() + [limit - len(collected)]) - _collect(cursor.fetchall()) + """, collector.exclude_values() + [_pool(collector.remaining())]) + collector.collect(cursor.fetchall(), rank=True) - return {'success': True, 'tracks': collected} + return {'success': True, 'tracks': collector.tracks} except Exception as e: logger.error(f"Error getting radio tracks for track {track_id}: {e}") diff --git a/entrypoint.sh b/entrypoint.sh index 28bae10a..13986c2c 100644 --- a/entrypoint.sh +++ b/entrypoint.sh @@ -81,15 +81,15 @@ chown soulsync:soulsync /app/config/settings.py 2>/dev/null || true # Pre-mid-2026 the chown line had `|| true` but mkdir didn't — combined # with `set -e`, a permission-denied mkdir crashed the container into a # restart loop. Both lines are now best-effort. -mkdir -p /app/config /app/data /app/logs /app/downloads /app/Transfer /app/Staging /app/Stream 2>/dev/null || true -chown soulsync:soulsync /app/config /app/data /app/logs /app/downloads /app/Transfer /app/Staging /app/Stream 2>/dev/null || true +mkdir -p /app/config /app/data /app/logs /app/downloads /app/Transfer /app/Staging /app/Stream /app/storage /app/MusicVideos /app/scripts 2>/dev/null || true +chown soulsync:soulsync /app/config /app/data /app/logs /app/downloads /app/Transfer /app/Staging /app/Stream /app/storage /app/MusicVideos /app/scripts 2>/dev/null || true # Writability audit — surface a loud warning if any bind-mounted dir # isn't writable by the soulsync user. The restart-loop fix above makes # the container start regardless, but a non-writable Staging / downloads # / Transfer will fail silently inside the app (auto-import quarantine, # download writes). Better to log now than to debug missing files later. -for dir in /app/config /app/data /app/logs /app/downloads /app/Transfer /app/Staging /app/Stream /app/MusicVideos /app/scripts; do +for dir in /app/config /app/data /app/logs /app/downloads /app/Transfer /app/Staging /app/Stream /app/storage /app/MusicVideos /app/scripts; do if [ -d "$dir" ] && ! gosu soulsync test -w "$dir" 2>/dev/null; then echo "⚠️ WARNING: $dir is not writable by soulsync (uid $(id -u soulsync))." echo " Host bind-mount perms likely mismatch the PUID/PGID env vars." diff --git a/revamp_plan.md b/revamp_plan.md new file mode 100644 index 00000000..39d339a8 --- /dev/null +++ b/revamp_plan.md @@ -0,0 +1,43 @@ +# Stream / Player / Radio Revamp — Plan + +Goal: bring the audio stream + media-player + radio system to Spotify/Apple-level polish and feature set. Target stack: **plain JS** (`webui/static/media-player.js`), not the React migration. Intended architecture direction: **multi-listener** (final call deferred to Phase 3; Phases 0–2 stay compatible either way). + +Rule for every phase: kettui standard — importable/testable logic, seam-level + differential tests, break nothing, ship one reviewable phase at a time. + +--- + +## Phase 0 — Make it provable (foundation, no user-visible change) + +- [x] **0a. Extract radio selection logic into testable `core/radio/`.** DONE (commit cbc001e2). `core/radio/selection.py` owns parse_tags/merge_tags/same_artist_cap/build_like_conditions/RadioCollector; DB method delegates. 29 tests, refactor-equivalence proven (behavioral tests pass against old AND new). +- [ ] **0b. Centralize frontend player state.** ~10 scattered `np*` globals in `media-player.js` → one `PlayerState` object. Seam for every later frontend phase. No behavior change. + +## Phase 1 — Polish / feel (frontend) + +- [x] Persistent queue across refresh (localStorage) — commit a8985b31 +- [x] Drag-to-reorder queue; duration + art per queue item — ffbe669c + 3461d923 +- [x] Seek tooltip (hover timestamp) — 112ecbb2 +- [x] Crossfade via dual-`