Fix Copy Debug Info music_source + surface missing services
- 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.
This commit is contained in:
Broque Thomas
parent
7ee01b80b7
commit
6233860d66
3 changed files with 210 additions and 7 deletions
|
|
@ -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
|
||||
|
|
|
|||
167
tests/test_debug_info_services.py
Normal file
167
tests/test_debug_info_services.py
Normal file
|
|
@ -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)
|
||||
|
|
@ -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' },
|
||||
|
|
|
|||
Loading…
Reference in a new issue