From 6233860d66e1ad23de7f9d57591eb11e7583070f Mon Sep 17 00:00:00 2001 From: Broque Thomas <26755000+Nezreka@users.noreply.github.com> Date: Tue, 12 May 2026 16:47:55 -0700 Subject: [PATCH] Fix Copy Debug Info music_source + surface missing services MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - music_source / spotify_connected / spotify_rate_limited were reading a non-existent 'spotify' key on _status_cache and silently falling through to the missing-value default (always 'unknown' / False). Routed through the canonical accessors get_primary_source + get_spotify_status now. - added hydrabase_connected, youtube_available, hifi_instance_count, and always_available_metadata_sources so the debug dump reflects the full service surface - removed a local re-import of get_spotify_status that was making python 3.12 treat the name as function-scoped, breaking the new lambda above it (NameError on free variable) — module-level import already exists 11 endpoint-level tests pin music_source / spotify_* / hydrabase_* / youtube_available / always_available_metadata_sources / hifi_instance_count and the defensive fall-through paths when each lookup raises. --- core/debug_info.py | 49 +++++++-- tests/test_debug_info_services.py | 167 ++++++++++++++++++++++++++++++ webui/static/helper.js | 1 + 3 files changed, 210 insertions(+), 7 deletions(-) create mode 100644 tests/test_debug_info_services.py diff --git a/core/debug_info.py b/core/debug_info.py index 88a5ece9..852c3463 100644 --- a/core/debug_info.py +++ b/core/debug_info.py @@ -14,7 +14,12 @@ from pathlib import Path from flask import jsonify, request from config.settings import config_manager -from core.metadata.registry import get_spotify_client +from core.metadata.registry import ( + get_spotify_client, + get_primary_source, + is_hydrabase_enabled, +) +from core.metadata.status import get_spotify_status logger = logging.getLogger(__name__) @@ -183,21 +188,47 @@ def get_debug_info(): info['paths']['music_videos_path'] = music_videos_path info['paths']['music_videos_path_exists'] = os.path.isdir(music_videos_path) - # Services from status cache - spotify_cache = _status_cache.get('spotify', {}) + # Services. `_status_cache` only carries 'media_server' and 'soulseek' + # (no 'spotify' key) so anything we used to read from `spotify_cache` + # silently defaulted to the missing-value fallback — that's the + # "music_source: unknown" bug. Spotify status now comes from the + # canonical `get_spotify_status` accessor; primary metadata source + # comes from `get_primary_source` (which already accounts for the + # auth-fallback chain — Spotify drops back to Deezer when not + # authenticated). media_server_cache = _status_cache.get('media_server', {}) soulseek_cache = _status_cache.get('soulseek', {}) + spotify_status = _safe_check(lambda: get_spotify_status(spotify_client=spotify_client), default={}) + if not isinstance(spotify_status, dict): + spotify_status = {} info['services'] = { - 'music_source': spotify_cache.get('source', 'unknown'), - 'spotify_connected': spotify_cache.get('connected', False), - 'spotify_rate_limited': spotify_cache.get('rate_limited', False), + 'music_source': _safe_check(get_primary_source, default='unknown') or 'unknown', + 'spotify_connected': bool(spotify_status.get('connected', False)), + 'spotify_rate_limited': bool(spotify_status.get('rate_limited', False)), 'media_server_type': media_server_cache.get('type', 'none'), 'media_server_connected': media_server_cache.get('connected', False), 'soulseek_connected': soulseek_cache.get('connected', False), 'download_source': config_manager.get('download_source.mode', 'hybrid'), 'tidal_connected': _safe_check(lambda: bool(tidal_client and tidal_client.is_authenticated())), 'qobuz_connected': _safe_check(lambda: bool(qobuz_enrichment_worker and qobuz_enrichment_worker.client and qobuz_enrichment_worker.client.is_authenticated())), + 'hydrabase_connected': _safe_check(is_hydrabase_enabled), + # YouTube is URL-based via yt-dlp — no auth, always reachable as + # long as the binary is installed. Surfaced so the debug dump + # documents that YouTube is one of the available download sources + # rather than implying it doesn't exist. + 'youtube_available': True, } + # HiFi instance count — separate from connection status because each + # instance is its own independent endpoint with its own auth state. + info['services']['hifi_instance_count'] = _safe_check( + lambda: len(get_database().get_hifi_instances()), default=0 + ) + # Always-available public metadata sources (no auth, no per-user + # connection state). Listed so the debug dump reflects the full + # metadata surface SoulSync queries from, not just the auth-gated ones. + info['services']['always_available_metadata_sources'] = [ + 'deezer', 'itunes', 'musicbrainz', + ] # Enrichment workers workers = {} @@ -300,7 +331,11 @@ def get_debug_info(): # API rate monitor — current calls/min, 24h totals, peaks, rate limit events try: from core.api_call_tracker import api_call_tracker - from core.metadata.status import get_spotify_status + # `get_spotify_status` is already imported at module level. A + # local re-import here would make Python treat the name as a + # function-scoped local for the WHOLE body, breaking the lambda + # at the top of get_debug_info that closes over the module-level + # binding (Python 3.12 NameError on free variables). rates = api_call_tracker.get_all_rates() info['api_rates'] = rates # Rich 24h debug summary with peaks, totals, per-endpoint breakdown, events diff --git a/tests/test_debug_info_services.py b/tests/test_debug_info_services.py new file mode 100644 index 00000000..aaf69651 --- /dev/null +++ b/tests/test_debug_info_services.py @@ -0,0 +1,167 @@ +"""Pin the `info['services']` block returned by /api/debug-info. + +Pre-fix the `music_source` field always rendered as "unknown" because +the code read `_status_cache.get('spotify', {})` — but the cache only +ever holds 'media_server' and 'soulseek' keys, so the fallback always +fired. Same problem (silently) for `spotify_connected` and +`spotify_rate_limited`. Hydrabase was missing entirely. + +Fix routes those reads through the canonical accessors: +- `music_source` → `core.metadata.registry.get_primary_source` (which + already accounts for the auth-fallback chain — Spotify → Deezer when + unauthenticated) +- `spotify_connected` / `spotify_rate_limited` → + `core.metadata.status.get_spotify_status` +- `hydrabase_connected` → `core.metadata.registry.is_hydrabase_enabled` +- `youtube_available` → constant True (URL-based, no auth) +- `hifi_instance_count` → `db.get_hifi_instances` +- `always_available_metadata_sources` → static list of public-API + sources (Deezer / iTunes / MusicBrainz) +""" + +from __future__ import annotations + +from contextlib import contextmanager +from unittest.mock import patch + +import pytest + + +@pytest.fixture +def app_test_client(): + import web_server + web_server.app.config['TESTING'] = True + with web_server.app.test_client() as client: + yield client + + +@contextmanager +def _patched_endpoint( + primary_source='spotify', + spotify_status=None, + hydrabase_enabled=False, + primary_source_raises=False, + spotify_status_raises=False, + hydrabase_raises=False, +): + """Patch the three module-level lookups inside `core.debug_info` + and yield. Each can either return a fixed value or raise — the + `*_raises` flags select which.""" + if spotify_status is None: + spotify_status = {'connected': True, 'rate_limited': False} + + def _boom(*_a, **_k): + raise RuntimeError('forced failure for test') + + primary_patch = patch( + 'core.debug_info.get_primary_source', + side_effect=_boom if primary_source_raises else None, + return_value=None if primary_source_raises else primary_source, + ) + spotify_patch = patch( + 'core.debug_info.get_spotify_status', + side_effect=_boom if spotify_status_raises else None, + return_value=None if spotify_status_raises else spotify_status, + ) + hydrabase_patch = patch( + 'core.debug_info.is_hydrabase_enabled', + side_effect=_boom if hydrabase_raises else None, + return_value=None if hydrabase_raises else hydrabase_enabled, + ) + with primary_patch, spotify_patch, hydrabase_patch: + yield + + +def _services(client): + resp = client.get('/api/debug-info') + assert resp.status_code == 200 + return resp.get_json()['services'] + + +def test_music_source_uses_primary_source_not_status_cache(app_test_client): + """The bug: music_source always read 'unknown' because it pulled + from a non-existent 'spotify' key in `_status_cache`. Fix routes + it through `get_primary_source` which is the actual authority.""" + with _patched_endpoint(primary_source='tidal'): + services = _services(app_test_client) + assert services['music_source'] == 'tidal' + + +def test_music_source_falls_back_to_unknown_when_lookup_raises(app_test_client): + """Defensive: if `get_primary_source` itself blows up, the field + still renders as 'unknown' rather than crashing the whole endpoint.""" + with _patched_endpoint(primary_source_raises=True): + services = _services(app_test_client) + assert services['music_source'] == 'unknown' + + +def test_spotify_connected_uses_get_spotify_status(app_test_client): + """`spotify_connected` was reading `_status_cache.get('spotify', {})`, + which never had the key. Routed through `get_spotify_status` now.""" + with _patched_endpoint(spotify_status={'connected': True, 'rate_limited': False}): + services = _services(app_test_client) + assert services['spotify_connected'] is True + + +def test_spotify_rate_limited_uses_get_spotify_status(app_test_client): + with _patched_endpoint(spotify_status={'connected': True, 'rate_limited': True}): + services = _services(app_test_client) + assert services['spotify_rate_limited'] is True + + +def test_spotify_status_lookup_failure_does_not_break_endpoint(app_test_client): + """`get_spotify_status` raises → both spotify_* fields default to + False rather than 500'ing the whole debug dump.""" + with _patched_endpoint(spotify_status_raises=True): + services = _services(app_test_client) + assert services['spotify_connected'] is False + assert services['spotify_rate_limited'] is False + + +def test_hydrabase_connected_present(app_test_client): + """Hydrabase status was never surfaced in debug info even though + it's an active metadata source. Now reported.""" + with _patched_endpoint(hydrabase_enabled=True): + services = _services(app_test_client) + assert services['hydrabase_connected'] is True + + +def test_hydrabase_disconnected_when_disabled(app_test_client): + with _patched_endpoint(hydrabase_enabled=False): + services = _services(app_test_client) + assert services['hydrabase_connected'] is False + + +def test_hydrabase_lookup_failure_defaults_false(app_test_client): + with _patched_endpoint(hydrabase_raises=True): + services = _services(app_test_client) + assert services['hydrabase_connected'] is False + + +def test_youtube_available_always_true(app_test_client): + """YouTube is URL-based via yt-dlp, no auth, always available. + Surfaced so the dump documents it as a download source.""" + with _patched_endpoint(): + services = _services(app_test_client) + assert services['youtube_available'] is True + + +def test_always_available_metadata_sources_listed(app_test_client): + """Public-API metadata sources (no auth, no per-user state) listed + so the debug dump reflects the full metadata surface.""" + with _patched_endpoint(): + services = _services(app_test_client) + available = services['always_available_metadata_sources'] + assert 'deezer' in available + assert 'itunes' in available + assert 'musicbrainz' in available + + +def test_hifi_instance_count_present(app_test_client): + """HiFi instance count exposed because each instance is a separate + endpoint with its own auth state — single connected/disconnected + bool wouldn't capture the actual config.""" + with _patched_endpoint(): + services = _services(app_test_client) + assert 'hifi_instance_count' in services + assert isinstance(services['hifi_instance_count'], int) diff --git a/webui/static/helper.js b/webui/static/helper.js index 3b648e5b..f913187b 100644 --- a/webui/static/helper.js +++ b/webui/static/helper.js @@ -3416,6 +3416,7 @@ const WHATS_NEW = { '2.5.1': [ // --- post-release patch work on the 2.5.1 line — entries hidden by _getLatestWhatsNewVersion until the build version bumps --- { date: 'Unreleased — 2.5.1 patch work' }, + { title: 'Help & Docs: Copy Debug Info Now Reports The Right Music Source + Lists All Services', desc: 'the music_source field always rendered as "unknown" because the code read `_status_cache.get(\'spotify\', {})` — but the cache only has \'media_server\' and \'soulseek\' keys, so the lookup always fell through. same silent miss for spotify_connected and spotify_rate_limited. fix routes those reads through the canonical accessors: `get_primary_source()` for music source (which already accounts for the spotify→deezer auth fallback), `get_spotify_status()` for connection + rate-limit state. also added hydrabase_connected (was missing entirely), youtube_available (always true — yt-dlp + url-based, no auth), hifi_instance_count (separate from connection because each instance is its own endpoint with its own auth), and an always_available_metadata_sources list (deezer / itunes / musicbrainz — public apis, no auth) so the dump reflects the full metadata surface. while in there: removed a local `from core.metadata.status import get_spotify_status` re-import that was making python 3.12 treat the name as a function-scoped local, breaking the new lambda above it (NameError on free variable). 11 new tests at the endpoint boundary pin music_source, spotify_*, hydrabase_*, youtube_available, always_available_metadata_sources, hifi_instance_count, and the defensive paths when each lookup raises.', page: 'tools' }, { title: 'Download Discography: Skips Tracks Already In Your Library', desc: 'discord report (skowl): clicking download discography on the same artist twice re-queued every track instead of skipping the half already on disk. trace: the endpoint added each track via `add_to_wishlist`, which dedups against the wishlist itself but never checks the library — once a downloaded track leaves the wishlist the next click re-inserts it. fix: same library-ownership check the discography backfill repair job already runs (`db.check_track_exists` at confidence ≥ 0.7). format-agnostic — name + artist + album, no extension comparison — so blasphemy mode (flac → mp3 with original deleted) doesn\'t false-miss. exception during the check returns "not owned" so a transient db hiccup doesn\'t silently nuke the discography fetch (a redundant wishlist add is cheap, a missed track isn\'t). per-album response carries a new `tracks_skipped_owned` counter alongside the artist / content / wishlist skips. 10 new tests at the helper boundary.', page: 'discover' }, { title: 'Download Discography: No More Cross-Artist Tracks Or Unwanted Remixes', desc: 'issue #559: download discography pulled in tracks from compilations / appears-on albums where the artist was only featured on one or two tracks — every other track on those albums got added too. also ignored your watchlist "include remixes / live / acoustic / instrumental" settings, so one-off discography downloads kept stuffing your wishlist with remix ladders. fix: per-track filter at the endpoint. drops tracks where the requested artist isn\'t named in the track\'s artists list (keeps features, drops unrelated compilation entries). honors `watchlist.global_include_*` settings the same way the discography backfill repair job already does. per-album response carries new skip counts so the ui can show how much got filtered. 21 new tests at the helper boundary.', page: 'discover' }, { title: 'Album Completeness: "Could Not Determine Album Folder" Error Now Tells You What To Fix', desc: 'github issue #558 (gabistek, navidrome on docker / arch host): clicking auto-fill or fix selected on the album completeness findings page returned a flat "could not determine album folder from existing tracks" error with no diagnostic. trace: the path resolver in `core/library/path_resolver.py` probes transfer + download + `library.music_paths` config + plex api library locations to map db-recorded paths to actual files on disk. for plex users the api auto-discovers the mount paths (per #476). navidrome\'s subsonic api doesn\'t expose filesystem paths at all (only folder names via `getMusicFolders`), and navidrome\'s native rest api on top of that doesn\'t expose them either — there is no api signal we can probe. so for navidrome users in docker, if the path navidrome reports (`/music/artist/album/track.flac`) doesn\'t exist as-is in the soulsync container view AND the user hasn\'t manually configured settings → library → music paths, the resolver returns none and the fix workflow bailed silently. fix: lifted the resolver into a diagnostic-aware variant (`resolve_library_file_path_with_diagnostic` returning a `(resolved, ResolveAttempt)` tuple) that records what was tried — raw-path-existed, base-dirs-probed, whether config_manager / plex_client were wired up. repair_worker uses the diagnostic to render a multi-part error: names the active media server, shows one sample db-recorded path the album\'s tracks have, lists every base directory the resolver actually probed, and points at settings → library → music paths as the actionable fix. user can now read the error and know exactly what to mount or configure. no auto-probing of common docker conventions — too speculative, could resolve to wrong dirs on the suffix-walk if conventional paths happen to contain a partial collision. backwards compatible: legacy `resolve_library_file_path` kept as a thin wrapper that drops the attempt, every existing call site unchanged. 12 new tests pin: tuple shape, raw-path short-circuit attempt fields, base-dirs listed even on walk failure, had-flags reflect caller inputs, error renders active server name + sample path + base dirs, distinguishes empty-base-dirs vs tried-and-failed cases, settings hint always present, defensive against none attempt + missing sample + missing config_manager.', page: 'tools' },