diff --git a/core/discovery/sync.py b/core/discovery/sync.py index 3c602fb9..9d92b938 100644 --- a/core/discovery/sync.py +++ b/core/discovery/sync.py @@ -39,8 +39,7 @@ class SyncDeps: """Bundle of cross-cutting deps the sync worker needs.""" config_manager: Any sync_service: Any - plex_client: Any - jellyfin_client: Any + media_server_engine: Any automation_engine: Any run_async: Callable[..., Any] record_sync_history_start: Callable @@ -227,8 +226,9 @@ def run_sync_task(playlist_id, playlist_name, tracks_json, automation_id=None, p # Check sync service components logger.info(f" spotify_client: {sync_service.spotify_client is not None}") - logger.info(f" deps.plex_client: {sync_service.plex_client is not None}") - logger.info(f" deps.jellyfin_client: {sync_service.jellyfin_client is not None}") + _ms_engine = getattr(sync_service, '_engine', None) + logger.info(f" plex_client: {(_ms_engine.client('plex') if _ms_engine else None) is not None}") + logger.info(f" jellyfin_client: {(_ms_engine.client('jellyfin') if _ms_engine else None) is not None}") # Check media server connection before starting from config.settings import config_manager @@ -404,11 +404,12 @@ def run_sync_task(playlist_id, playlist_name, tracks_json, automation_id=None, p try: active_server = deps.config_manager.get_active_media_server() logger.info(f"[PLAYLIST IMAGE] active_server={active_server}") - if active_server == 'plex' and deps.plex_client: - ok = deps.plex_client.set_playlist_image(playlist_name, playlist_image_url) + _engine = deps.media_server_engine + if active_server == 'plex' and _engine and _engine.client('plex'): + ok = _engine.client('plex').set_playlist_image(playlist_name, playlist_image_url) logger.info(f"[PLAYLIST IMAGE] Plex upload result: {ok}") - elif active_server in ('jellyfin', 'emby') and deps.jellyfin_client: - ok = deps.jellyfin_client.set_playlist_image(playlist_name, playlist_image_url) + elif active_server in ('jellyfin', 'emby') and _engine and _engine.client('jellyfin'): + ok = _engine.client('jellyfin').set_playlist_image(playlist_name, playlist_image_url) logger.info(f"[PLAYLIST IMAGE] Jellyfin upload result: {ok}") # Navidrome doesn't support custom playlist images except Exception as img_err: diff --git a/core/jellyfin_client.py b/core/jellyfin_client.py index 28adba99..2439e1cf 100644 --- a/core/jellyfin_client.py +++ b/core/jellyfin_client.py @@ -1,33 +1,19 @@ import requests import time from typing import List, Optional, Dict, Any -from dataclasses import dataclass from datetime import datetime import json from utils.logging_config import get_logger from config.settings import config_manager +# Shared dataclasses live in the neutral media_server package — every +# server client used to define a near-identical XTrackInfo / +# XPlaylistInfo. Lifted to one canonical type so consumers (matching +# engine, sync service) get a single import. +from core.media_server.types import TrackInfo, PlaylistInfo + logger = get_logger("jellyfin_client") -@dataclass -class JellyfinTrackInfo: - id: str - title: str - artist: str - album: str - duration: int - track_number: Optional[int] = None - year: Optional[int] = None - rating: Optional[float] = None - -@dataclass -class JellyfinPlaylistInfo: - id: str - title: str - description: Optional[str] - duration: int - leaf_count: int - tracks: List[JellyfinTrackInfo] class JellyfinArtist: """Wrapper class to mimic Plex artist object interface""" @@ -146,7 +132,10 @@ class JellyfinTrack: return self._client.get_album_by_id(self._album_id) return None -class JellyfinClient: +from core.media_server.contract import MediaServerClient + + +class JellyfinClient(MediaServerClient): def __init__(self): self.base_url: Optional[str] = None self.api_key: Optional[str] = None @@ -1201,7 +1190,7 @@ class JellyfinClient: return stats - def get_all_playlists(self) -> List[JellyfinPlaylistInfo]: + def get_all_playlists(self) -> List[PlaylistInfo]: """Get all playlists from Jellyfin server""" if not self.ensure_connection(): return [] @@ -1218,7 +1207,7 @@ class JellyfinClient: playlists = [] for item in response.get('Items', []): - playlist_info = JellyfinPlaylistInfo( + playlist_info = PlaylistInfo( id=item.get('Id', ''), title=item.get('Name', 'Unknown Playlist'), description=item.get('Overview'), @@ -1235,7 +1224,7 @@ class JellyfinClient: logger.error(f"Error getting playlists from Jellyfin: {e}") return [] - def get_playlist_by_name(self, name: str) -> Optional[JellyfinPlaylistInfo]: + def get_playlist_by_name(self, name: str) -> Optional[PlaylistInfo]: """Get a specific playlist by name""" playlists = self.get_all_playlists() for playlist in playlists: diff --git a/core/listening_stats_worker.py b/core/listening_stats_worker.py index 72bdaf16..ec70e1fd 100644 --- a/core/listening_stats_worker.py +++ b/core/listening_stats_worker.py @@ -19,13 +19,17 @@ logger = get_logger("listening_stats_worker") class ListeningStatsWorker: """Background worker that polls media servers for play data.""" - def __init__(self, database, config_manager, plex_client=None, - jellyfin_client=None, navidrome_client=None): + def __init__(self, database, config_manager, media_server_engine=None): + """Initialize the worker. + + ``media_server_engine`` owns the per-server clients (Plex / + Jellyfin / Navidrome). The worker resolves the active server's + client through ``self._engine.client(name)`` instead of holding + per-server kwargs. + """ self.db = database self.config_manager = config_manager - self.plex_client = plex_client - self.jellyfin_client = jellyfin_client - self.navidrome_client = navidrome_client + self._engine = media_server_engine # Worker state self.running = False @@ -145,13 +149,11 @@ class ListeningStatsWorker: logger.info(f"Polling {active_server} for listening data...") self.current_item = f"Polling {active_server}..." - client = None - if active_server == 'plex' and self.plex_client: - client = self.plex_client - elif active_server == 'jellyfin' and self.jellyfin_client: - client = self.jellyfin_client - elif active_server == 'navidrome' and self.navidrome_client: - client = self.navidrome_client + client = self._engine.client(active_server) if self._engine else None + # SoulSync standalone has no listening data; only the three + # streaming servers contribute. Mirror the legacy guard here. + if active_server not in ('plex', 'jellyfin', 'navidrome'): + client = None if not client: logger.warning(f"No client available for active server: {active_server}") diff --git a/core/matching_engine.py b/core/matching_engine.py index 2522d920..9abb2d06 100644 --- a/core/matching_engine.py +++ b/core/matching_engine.py @@ -7,7 +7,10 @@ from utils.logging_config import get_logger from config.settings import config_manager from core.spotify_client import Track as SpotifyTrack -from core.plex_client import PlexTrackInfo +from core.media_server.types import TrackInfo +# TrackResult / AlbumResult moved out of core.soulseek_client into the +# neutral download_plugins package (download PR's Gap 1 lift). Import +# from the new location. from core.download_plugins.types import TrackResult, AlbumResult @@ -16,7 +19,7 @@ logger = get_logger("matching_engine") @dataclass class MatchResult: spotify_track: SpotifyTrack - plex_track: Optional[PlexTrackInfo] + plex_track: Optional[TrackInfo] confidence: float match_type: str @@ -302,7 +305,7 @@ class MusicMatchingEngine: confidence = (title_score * 0.60) + (artist_score * 0.30) + (duration_score * 0.10) return confidence, "standard_match" - def calculate_match_confidence(self, spotify_track: SpotifyTrack, plex_track: PlexTrackInfo) -> Tuple[float, str]: + def calculate_match_confidence(self, spotify_track: SpotifyTrack, plex_track: TrackInfo) -> Tuple[float, str]: """Calculates a confidence score using a prioritized model, starting with a strict 'core' title check.""" return self.score_track_match( source_title=spotify_track.name, @@ -313,7 +316,7 @@ class MusicMatchingEngine: candidate_duration_ms=plex_track.duration if plex_track.duration else 0 ) - def find_best_match(self, spotify_track: SpotifyTrack, plex_tracks: List[PlexTrackInfo]) -> MatchResult: + def find_best_match(self, spotify_track: SpotifyTrack, plex_tracks: List[TrackInfo]) -> MatchResult: """Finds the best Plex track match from a list of candidates.""" best_match = None best_confidence = 0.0 diff --git a/core/media_server/__init__.py b/core/media_server/__init__.py new file mode 100644 index 00000000..0dc6269c --- /dev/null +++ b/core/media_server/__init__.py @@ -0,0 +1,39 @@ +"""Media server engine — central registry-backed access to the +per-server clients (Plex, Jellyfin, Navidrome, SoulSync standalone). + +Companion to the download engine refactor — same architectural +shape applied to the read-side of the library. Pre-refactor +web_server.py held four separate per-server globals +(``plex_client`` / ``jellyfin_client`` / ``navidrome_client`` / +``soulsync_library_client``) that every dispatch site reached +individually. This package replaces those globals with a single +engine that owns the client instances + a generic +``engine.client(name)`` accessor. + +The 18-or-so ``if active_server == 'plex' / 'jellyfin' / ...`` +chains in web_server.py that do server-specific work (Plex raw +playlist API vs Jellyfin / Navidrome client methods returning +different shapes) stay explicit at the call site per the "lift +what's truly shared" standard — but they reach the per-server +client through ``engine.client(name)`` rather than the legacy +globals. The four uniform-shape ``is_connected`` chains were the +only ones genuinely shared and are now ``engine.is_connected()``. + +See ``docs/media-server-engine-refactor-plan.md`` for the full +phased plan. + +Note: only ``MediaServerClient`` is re-exported here. The engine + +registry are NOT — importing the registry triggers eager imports +of every per-server client class, and those clients now inherit +``MediaServerClient`` (Cin-1), so re-exporting them here would +form a circular import the moment a client tried to resolve its +base class. Import them directly from their submodules: + from core.media_server.engine import MediaServerEngine + from core.media_server.registry import build_default_registry +""" + +from core.media_server.contract import MediaServerClient + +__all__ = [ + "MediaServerClient", +] diff --git a/core/media_server/contract.py b/core/media_server/contract.py new file mode 100644 index 00000000..d051482a --- /dev/null +++ b/core/media_server/contract.py @@ -0,0 +1,110 @@ +"""Canonical contract for media server clients. + +Narrow on purpose. Protocol body declares ONLY the methods every +registered client actually implements today — keeps the static +contract honest. Server-specific extras (Plex's +``set_music_library_by_name``, Jellyfin's user picker, Navidrome's +music folder filter, SoulSync's filesystem rescan) and methods that +most-but-not-all servers implement (``search_tracks`` on Plex / +Navidrome but not Jellyfin; ``get_recently_added_albums`` on +Jellyfin / Navidrome / SoulSync but not Plex) stay off the Protocol +and are reached through ``engine.client(name)`` directly. + +The contract is a Protocol (structural typing) rather than an ABC — +existing PlexClient / JellyfinClient / NavidromeClient / +SoulSyncClient grew the same shape independently because every +caller needed the same four calls. This file just makes that +implicit contract explicit + the conformance test pins it. +""" + +from __future__ import annotations + +from typing import Any, List, Protocol, runtime_checkable + + +@runtime_checkable +class MediaServerClient(Protocol): + """Structural contract every media server client must satisfy. + + ``runtime_checkable`` lets ``isinstance(client, MediaServerClient)`` + work, but it ONLY checks method names — not signatures. The + conformance test in ``tests/media_server/test_conformance.py`` + does the deeper class-level check via REQUIRED_METHODS. + """ + + # ------------------------------------------------------------------ + # Connection / lifecycle — required, every server implements + # ------------------------------------------------------------------ + + def is_connected(self) -> bool: + """Cheap probe — does the client have a live connection / + token / session right now? Used by the dashboard status + indicators + endpoint guards.""" + ... + + def ensure_connection(self) -> bool: + """Re-auth or reconnect if needed. May make a network call. + Returns True if connection is usable after the call.""" + ... + + # ------------------------------------------------------------------ + # Library reads — required, every server implements + # ------------------------------------------------------------------ + + def get_all_artists(self) -> List[Any]: + """Return every artist the server knows about. Each item is + a server-specific wrapper object (PlexArtist, JellyfinArtist, + NavidromeArtist, SoulSyncArtist) — caller treats them + opaquely.""" + ... + + def get_all_album_ids(self) -> set: + """Return the set of every album ID in the library. ID + format is server-native — caller doesn't introspect.""" + ... + + +# --------------------------------------------------------------------------- +# Required method set — pinned by the conformance test. Mirrors the +# Protocol body exactly so static + runtime contracts can't drift. +# --------------------------------------------------------------------------- + +REQUIRED_METHODS = { + 'is_connected', + 'ensure_connection', + 'get_all_artists', + 'get_all_album_ids', +} + +# Methods that exist on SOME servers but NOT all, listed here for +# discoverability. The conformance test does NOT enforce these. Callers +# that need one reach the per-server client directly via +# ``engine.client(name).`` rather than going through the engine, +# since the engine has no uniform safe-default that fits every method. +# +# Coverage today (audited 2026-05): +# search_tracks: Plex ✓, Navidrome ✓, Jellyfin ✗, SoulSync ✗ +# get_recently_added_albums: Jellyfin ✓, Navidrome ✓, SoulSync ✓, Plex ✗ (uses recentlyAdded() on music library) +# trigger_library_scan / is_library_scanning: Plex ✓, Jellyfin ✓, Navidrome ✓, SoulSync ✗ (filesystem walks in-process) +# get_library_stats: Plex ✓, Jellyfin ✓, Navidrome ✓, SoulSync ✗ +# create_playlist / update_playlist / get_all_playlists / etc: Plex ✓, Jellyfin ✓, Navidrome ✓, SoulSync ✗ +# update_artist_*, update_album_poster, update_track_metadata: Plex ✓, Jellyfin partial, Navidrome stubs, SoulSync ✗ +KNOWN_PER_SERVER_METHODS = ( + 'search_tracks', + 'get_recently_added_albums', + 'trigger_library_scan', + 'is_library_scanning', + 'get_library_stats', + 'create_playlist', + 'update_playlist', + 'copy_playlist', + 'get_all_playlists', + 'get_playlist_by_name', + 'get_play_history', + 'get_track_play_counts', + 'update_artist_genres', + 'update_artist_poster', + 'update_album_poster', + 'update_artist_biography', + 'update_track_metadata', +) diff --git a/core/media_server/engine.py b/core/media_server/engine.py new file mode 100644 index 00000000..d65a52ba --- /dev/null +++ b/core/media_server/engine.py @@ -0,0 +1,209 @@ +"""MediaServerEngine — central registry-backed access to media server clients. + +Honest scope: the engine OWNS the per-server client instances and +exposes a small set of generic accessors so callers don't need +per-server attribute reaches. Most actual cross-server dispatch in +web_server.py (playlist add / remove / replace, per-server metadata +sync, deep scan with server-specific cache strategies) is genuinely +different per server and stays explicit in the call site — the +engine just provides the canonical client lookup so those sites +reach via ``engine.client(name)`` instead of separate globals. + +Surface: +- ``client(name)`` / ``active_client()`` — name → client lookup +- ``active_server`` — config-driven active server name +- ``is_connected()`` — only cross-server dispatch with real callers + today (dashboard status indicators); kept as the canonical example +- ``configured_clients()`` — replaces the legacy per-server + ``if X and X.is_connected()`` chains in web_server.py +- ``reload_config(name=None)`` — generic dispatch instead of + per-client reload calls + +Per-method engine wrappers for ``get_all_artists`` / ``search_tracks`` +/ ``trigger_library_scan`` / etc. were on an earlier draft but had no +production callers — every consumer reaches the active client directly +through ``sync_service._get_active_media_client()`` or +``engine.client(name)`` and calls the per-server method itself. Cut +per the "no premature abstraction" standard. + +Engine is constructed once during web_server.py init and held as a +process-wide singleton via ``set_media_server_engine`` / +``get_media_server_engine``, mirroring the metadata + download +engine factory shape. +""" + +from __future__ import annotations + +from typing import Any, Dict, Optional + +from utils.logging_config import get_logger + +from core.media_server.contract import MediaServerClient +from core.media_server.registry import MediaServerRegistry, build_default_registry + +logger = get_logger("media_server.engine") + + +class MediaServerEngine: + """Registry-backed access to the per-server media clients. + + Owns the per-server client instances + a small set of generic + accessors (``client(name)`` / ``active_client()`` / + ``configured_clients()`` / ``reload_config(name)``) so call sites + don't reach for separate per-server globals. The one cross-server + dispatch wrapper kept on the engine — ``is_connected()`` — + backs the dashboard status indicators that have multiple call + sites; everything else dispatches per-server in the call site + itself, reaching the relevant client through ``engine.client(name)``. + """ + + def __init__( + self, + registry: Optional[MediaServerRegistry] = None, + active_server_resolver=None, + clients: Optional[Dict[str, Any]] = None, + ) -> None: + """Initialize the engine. + + Args: + registry: Plugin registry. Defaults to the four built-in + servers (Plex, Jellyfin, Navidrome, SoulSync). + active_server_resolver: Callable returning the current + active server name (e.g. ``'plex'``). Defaults to + ``config_manager.get_active_media_server``. Tests + inject a custom resolver to switch active server + without touching real config. + clients: Pre-built {name: client_instance} dict. When + provided, the engine wraps these instances directly + instead of asking the registry to construct fresh + ones. web_server.py uses this so the engine + shares the same client objects as the + pre-existing global variables (no double-init). + """ + self.registry = registry if registry is not None else build_default_registry() + + if clients is not None: + # Wrap pre-built instances (production case from web_server.py + # init). Skip registry.initialize() — we already have the + # instances, hand them off via the registry's public + # set_instance(name, client) method so internal storage stays + # encapsulated. + for name, client in clients.items(): + self.registry.set_instance(name, client) + # Mark any registered-but-not-supplied as failed init so + # active_client() returns None for them. + for name in self.registry.names(): + if self.registry.get(name) is None and name not in clients: + self.registry.set_instance(name, None) + else: + self.registry.initialize() + + if active_server_resolver is None: + from config.settings import config_manager + active_server_resolver = config_manager.get_active_media_server + self._resolve_active = active_server_resolver + + # ------------------------------------------------------------------ + # Client lookup — generic accessors that replace per-server + # attribute reaches in callers. + # ------------------------------------------------------------------ + + def client(self, name: str) -> Optional[MediaServerClient]: + """Return the client instance for the given server name, or + None if it's not registered / failed to initialize. Used by + callers that need a server-specific method beyond the + contract surface.""" + return self.registry.get(name) + + @property + def active_server(self) -> str: + """The currently-selected media server name.""" + return self._resolve_active() + + def active_client(self) -> Optional[MediaServerClient]: + """The client for the currently-active server.""" + return self.registry.get(self.active_server) + + def is_connected(self) -> bool: + """Active server's connection state. False if no active + client (registered but failed to initialize). The dashboard + status indicators + endpoint guards rely on this — the only + cross-server dispatch wrapper kept on the engine because it + actually has callers.""" + client = self.active_client() + if client is None: + return False + try: + return client.is_connected() + except Exception as exc: + logger.warning("%s is_connected raised: %s", self.active_server, exc) + return False + + def configured_clients(self) -> Dict[str, MediaServerClient]: + """Return ``{name: client}`` for every server that's both + registered AND reports ``is_connected() == True``. Replaces + the legacy per-server `if X and X.is_connected(): ...` + chains in web_server.py. + + ``is_connected`` is in REQUIRED_METHODS, so every client the + registry yields here implements it — no hasattr guard needed. + """ + result: Dict[str, MediaServerClient] = {} + for name, client in self.registry.all_clients(): + try: + if client.is_connected(): + result[name] = client + except Exception as exc: + logger.warning("%s is_connected raised in configured_clients: %s", name, exc) + return result + + def reload_config(self, name: Optional[str] = None) -> bool: + """Reload config on a single server (or every server when + ``name`` is None). Generic dispatch — caller passes the name + instead of reaching for ``plex_client.reload_config()`` + / ``jellyfin_client.reload_config()`` directly. Servers + without a ``reload_config`` method are silently skipped. + """ + names = [name] if name else list(self.registry.names()) + ok = True + for n in names: + client = self.client(n) + if client is None or not hasattr(client, 'reload_config'): + continue + try: + client.reload_config() + except Exception as exc: + logger.warning("%s reload_config failed: %s", n, exc) + ok = False + return ok + + +# --------------------------------------------------------------------------- +# Singleton accessor — mirrors the get_metadata_engine() / +# get_download_orchestrator() pattern so callers that don't need a +# custom registry use this instead of instantiating MediaServerEngine +# directly. web_server.py constructs the singleton at startup and +# installs it via ``set_media_server_engine`` so the factory + the +# global handle share state. +# --------------------------------------------------------------------------- + +_default_engine: Optional['MediaServerEngine'] = None + + +def get_media_server_engine() -> 'MediaServerEngine': + """Return (lazily creating) the process-wide MediaServerEngine + singleton. Mirrors the ``get_metadata_engine()`` / + ``get_download_orchestrator()`` shape.""" + global _default_engine + if _default_engine is None: + _default_engine = MediaServerEngine() + return _default_engine + + +def set_media_server_engine(engine: Optional['MediaServerEngine']) -> None: + """Set the process-wide singleton. Used by web_server.py at boot + to install the engine it constructs (with the pre-built per-client + instances) as the default for callers reaching via + ``get_media_server_engine()``.""" + global _default_engine + _default_engine = engine diff --git a/core/media_server/registry.py b/core/media_server/registry.py new file mode 100644 index 00000000..03fe20b3 --- /dev/null +++ b/core/media_server/registry.py @@ -0,0 +1,142 @@ +"""Media server plugin registry. + +Single source of truth for which servers exist, what their canonical +names are, and which client class implements each. Replaces the +historic web_server.py pattern of holding 4 separate client globals +that every dispatch site reached individually. + +Server-specific dispatch chains in web_server.py (playlist add / +remove / replace, per-server metadata sync, etc.) still hand-branch +on ``active_server == X`` because the work each server does at those +sites is genuinely different — but they reach the per-server CLIENT +through ``engine.client(name)`` (which goes through this registry) +instead of separate globals. + +Adding a new server (Subsonic / Emby) = one ``register`` call here + +the new client class. +""" + +from __future__ import annotations + +from dataclasses import dataclass, field +from typing import Callable, Dict, Iterator, List, Optional, Tuple + +from utils.logging_config import get_logger + +from core.media_server.contract import MediaServerClient + +# Eager imports for the same import-order reason the download plugin +# registry uses them (some integration tests inject mock modules into +# sys.modules at collection time; lazy import would bind to the mock). +from core.jellyfin_client import JellyfinClient +from core.navidrome_client import NavidromeClient +from core.plex_client import PlexClient +from core.soulsync_client import SoulSyncClient + +logger = get_logger("media_server.registry") + + +@dataclass(frozen=True) +class ServerSpec: + """Static descriptor for a media server. ``factory`` is the + zero-arg callable that builds the client (each server has its + own setup chain — Plex pulls token from config, Jellyfin reads + user_id, etc.).""" + + name: str + factory: Callable[[], MediaServerClient] + display_name: str + aliases: Tuple[str, ...] = field(default_factory=tuple) + + +class MediaServerRegistry: + """Holds the live client instances + name → instance lookup. + + Two-phase construction (mirrors the download plugin registry): + 1. Specs registered cheaply (just stores callable refs). + 2. ``initialize()`` calls each factory once. Failures captured + in ``init_failures`` so one broken server doesn't take down + the orchestrator. + """ + + def __init__(self) -> None: + self._specs: Dict[str, ServerSpec] = {} + self._instances: Dict[str, Optional[MediaServerClient]] = {} + self._init_failures: List[str] = [] + + def register(self, spec: ServerSpec) -> None: + if spec.name in self._specs: + raise ValueError(f"Server already registered: {spec.name}") + self._specs[spec.name] = spec + + def initialize(self) -> None: + for spec in self._specs.values(): + try: + instance = spec.factory() + self._instances[spec.name] = instance + except Exception as exc: + logger.error("%s media server client failed to initialize: %s", spec.display_name, exc) + self._init_failures.append(spec.display_name) + self._instances[spec.name] = None + + def set_instance(self, name: str, instance: Optional[MediaServerClient]) -> None: + """Stash a pre-built client instance into the registry without + running the spec's factory. Used by callers (web_server.py at + boot) that already constructed the per-server clients and want + the engine to wrap those exact instances rather than build new + ones. Replaces the old pattern of reaching into ``_instances`` + directly from the engine.""" + self._instances[name] = instance + + @property + def init_failures(self) -> List[str]: + return list(self._init_failures) + + def get(self, name: str) -> Optional[MediaServerClient]: + if not name: + return None + if name in self._instances: + return self._instances[name] + for spec in self._specs.values(): + if name in spec.aliases: + return self._instances.get(spec.name) + return None + + def get_spec(self, name: str) -> Optional[ServerSpec]: + if name in self._specs: + return self._specs[name] + for spec in self._specs.values(): + if name in spec.aliases: + return spec + return None + + def display_name(self, name: str) -> str: + spec = self.get_spec(name) + return spec.display_name if spec else name + + def names(self) -> List[str]: + return list(self._specs.keys()) + + def all_clients(self) -> Iterator[Tuple[str, MediaServerClient]]: + """Yield (name, client) for every successfully-initialized + server. Used by cross-server operations.""" + for name, instance in self._instances.items(): + if instance is not None: + yield name, instance + + +def build_default_registry() -> MediaServerRegistry: + """Construct the registry with SoulSync's four built-in media + servers. Called once during MediaServerEngine construction. + + Adding a server (e.g. Subsonic, Emby) = one ``register`` call + here + the new client class. No dispatch-site changes required. + """ + registry = MediaServerRegistry() + + registry.register(ServerSpec(name='plex', factory=PlexClient, display_name='Plex')) + registry.register(ServerSpec(name='jellyfin', factory=JellyfinClient, display_name='Jellyfin')) + registry.register(ServerSpec(name='navidrome', factory=NavidromeClient, display_name='Navidrome')) + registry.register(ServerSpec(name='soulsync', factory=SoulSyncClient, display_name='SoulSync Library')) + + return registry diff --git a/core/media_server/types.py b/core/media_server/types.py new file mode 100644 index 00000000..30fdbe09 --- /dev/null +++ b/core/media_server/types.py @@ -0,0 +1,132 @@ +"""Shared dataclasses for the media-server contract. + +Plex / Jellyfin / Navidrome clients all surfaced near-identical +``XTrackInfo`` and ``XPlaylistInfo`` shapes (id, title, artist, +album, duration, track_number, year, optional rating) because every +consumer needed the same shape downstream. The per-server class +names were a copy-paste artifact, not a real contract difference. + +This module owns the canonical types. Plex's existing classmethod +constructors (``TrackInfo.from_plex_track``, +``PlaylistInfo.from_plex_playlist``) live here. Jellyfin and +Navidrome currently construct ``TrackInfo`` inline at their call +sites — lifting those into matching ``from_jellyfin_dict`` / +``from_navidrome_dict`` classmethods is a clean followup but isn't +needed for the dataclass unification this module ships. + +Heavy server SDK types (``PlexTrack``) imported under TYPE_CHECKING +so this module stays import-light. +""" + +from __future__ import annotations + +from dataclasses import dataclass, field +from typing import TYPE_CHECKING, List, Optional + +if TYPE_CHECKING: + # plexapi types — only loaded when type-checking; runtime + # paths through from_plex_track accept whatever PlexClient + # passes through. + from plexapi.audio import Track as PlexTrack + from plexapi.playlist import Playlist as PlexPlaylist + + +@dataclass +class TrackInfo: + """Canonical track-shape returned by media server clients. + + Plex, Jellyfin, and Navidrome each defined their own near-identical + ``XTrackInfo`` dataclass (SoulSync standalone uses richer per-track + wrappers and doesn't surface this exact shape). Lifted to one + canonical type here so consumers (matching engine, sync service, + library scanners) get a single import. + """ + + id: str + title: str + artist: str + album: str + duration: int + track_number: Optional[int] = None + year: Optional[int] = None + rating: Optional[float] = None + + # ------------------------------------------------------------------ + # Per-server constructors — mirror Cin's metadata Album.from_X_dict + # pattern. Each one knows ONE server's wire shape. + # ------------------------------------------------------------------ + + @classmethod + def from_plex_track(cls, track: 'PlexTrack') -> 'TrackInfo': + """Build a TrackInfo from a plexapi PlexTrack. + + Defensive: tracks may be missing artist or album metadata in + Plex (especially fan-uploaded content); fall back to + "Unknown Artist" / "Unknown Album" instead of raising. + """ + # Imported lazily so this module stays import-light. plexapi + # is heavy and pulls in network + ssl deps just to define + # exception types. + from plexapi.exceptions import NotFound + + try: + artist_title = track.artist().title if track.artist() else "Unknown Artist" + except (NotFound, AttributeError): + artist_title = "Unknown Artist" + + try: + album_title = track.album().title if track.album() else "Unknown Album" + except (NotFound, AttributeError): + album_title = "Unknown Album" + + return cls( + id=str(track.ratingKey), + title=track.title, + artist=artist_title, + album=album_title, + duration=track.duration, + track_number=track.trackNumber, + year=track.year, + rating=track.userRating, + ) + + +@dataclass +class PlaylistInfo: + """Canonical playlist-shape returned by every media server client. + + Same lift rationale as ``TrackInfo`` — every server defined the + same five-field dataclass + a list of tracks. + """ + + id: str + title: str + description: Optional[str] + duration: int + leaf_count: int + tracks: List[TrackInfo] = field(default_factory=list) + + # ------------------------------------------------------------------ + # Per-server constructors + # ------------------------------------------------------------------ + + @classmethod + def from_plex_playlist(cls, playlist: 'PlexPlaylist') -> 'PlaylistInfo': + """Build a PlaylistInfo from a plexapi Playlist. Skips items + that aren't audio tracks (Plex playlists can mix media types + in theory, though music libraries shouldn't).""" + from plexapi.audio import Track as PlexTrack + + tracks: List[TrackInfo] = [] + for item in playlist.items(): + if isinstance(item, PlexTrack): + tracks.append(TrackInfo.from_plex_track(item)) + + return cls( + id=str(playlist.ratingKey), + title=playlist.title, + description=playlist.summary, + duration=playlist.duration, + leaf_count=playlist.leafCount, + tracks=tracks, + ) diff --git a/core/navidrome_client.py b/core/navidrome_client.py index 2cc010a7..5dd29136 100644 --- a/core/navidrome_client.py +++ b/core/navidrome_client.py @@ -2,33 +2,19 @@ import requests import hashlib import secrets from typing import List, Optional, Dict, Any -from dataclasses import dataclass from datetime import datetime import json from utils.logging_config import get_logger from config.settings import config_manager +# Shared dataclasses live in the neutral media_server package — every +# server client used to define a near-identical XTrackInfo / +# XPlaylistInfo. Lifted to one canonical type so consumers (matching +# engine, sync service) get a single import. +from core.media_server.types import TrackInfo, PlaylistInfo + logger = get_logger("navidrome_client") -@dataclass -class NavidromeTrackInfo: - id: str - title: str - artist: str - album: str - duration: int - track_number: Optional[int] = None - year: Optional[int] = None - rating: Optional[float] = None - -@dataclass -class NavidromePlaylistInfo: - id: str - title: str - description: Optional[str] - duration: int - leaf_count: int - tracks: List[NavidromeTrackInfo] class NavidromeArtist: """Wrapper class to mimic Plex artist object interface""" @@ -150,7 +136,10 @@ class NavidromeTrack: return self._client.get_album_by_id(self._album_id) return None -class NavidromeClient: +from core.media_server.contract import MediaServerClient + + +class NavidromeClient(MediaServerClient): def __init__(self): self.base_url: Optional[str] = None self.username: Optional[str] = None @@ -824,7 +813,7 @@ class NavidromeClient: return {} return {} - def get_all_playlists(self) -> List[NavidromePlaylistInfo]: + def get_all_playlists(self) -> List[PlaylistInfo]: """Get all playlists from Navidrome server""" if not self.ensure_connection(): return [] @@ -838,7 +827,7 @@ class NavidromeClient: playlists_data = response.get('playlists', {}).get('playlist', []) for playlist_data in playlists_data: - playlist_info = NavidromePlaylistInfo( + playlist_info = PlaylistInfo( id=playlist_data.get('id', ''), title=playlist_data.get('name', 'Unknown Playlist'), description=playlist_data.get('comment'), @@ -855,7 +844,7 @@ class NavidromeClient: logger.error(f"Error getting playlists from Navidrome: {e}") return [] - def get_playlist_by_name(self, name: str) -> Optional[NavidromePlaylistInfo]: + def get_playlist_by_name(self, name: str) -> Optional[PlaylistInfo]: """Get a specific playlist by name""" playlists = self.get_all_playlists() for playlist in playlists: @@ -976,7 +965,7 @@ class NavidromeClient: logger.error(f"Error getting tracks for playlist {playlist_id}: {e}") return [] - def get_playlists_by_name(self, name: str) -> List[NavidromePlaylistInfo]: + def get_playlists_by_name(self, name: str) -> List[PlaylistInfo]: """Get all playlists matching a specific name (case-insensitive)""" matches = [] playlists = self.get_all_playlists() @@ -1140,7 +1129,7 @@ class NavidromeClient: self._track_cache.clear() logger.info("Navidrome client cache cleared") - def search_tracks(self, title: str, artist: str, limit: int = 15) -> List[NavidromeTrackInfo]: + def search_tracks(self, title: str, artist: str, limit: int = 15) -> List[TrackInfo]: """Search for tracks using Navidrome search API""" if not self.ensure_connection(): logger.warning("Navidrome not connected. Cannot perform search.") @@ -1166,7 +1155,7 @@ class NavidromeClient: search_result = response.get('searchResult3', {}) for track_data in search_result.get('song', []): - track_info = NavidromeTrackInfo( + track_info = TrackInfo( id=track_data.get('id', ''), title=track_data.get('title', ''), artist=track_data.get('artist', ''), diff --git a/core/plex_client.py b/core/plex_client.py index 43bbf8a8..1cf771e1 100644 --- a/core/plex_client.py +++ b/core/plex_client.py @@ -4,7 +4,6 @@ from plexapi.audio import Track as PlexTrack, Album as PlexAlbum, Artist as Plex from plexapi.playlist import Playlist as PlexPlaylist from plexapi.exceptions import PlexApiException, NotFound from typing import List, Optional, Dict, Any -from dataclasses import dataclass import requests from datetime import datetime, timedelta import re @@ -12,69 +11,17 @@ from utils.logging_config import get_logger from config.settings import config_manager import threading +# Shared dataclasses live in the neutral media_server package now — +# every server client used to define a near-identical XTrackInfo / +# XPlaylistInfo. Lifted to one canonical type so consumers (matching +# engine, sync service, etc.) get a single import. +from core.media_server.types import TrackInfo, PlaylistInfo +from core.media_server.contract import MediaServerClient + logger = get_logger("plex_client") -@dataclass -class PlexTrackInfo: - id: str - title: str - artist: str - album: str - duration: int - track_number: Optional[int] = None - year: Optional[int] = None - rating: Optional[float] = None - - @classmethod - def from_plex_track(cls, track: PlexTrack) -> 'PlexTrackInfo': - # Gracefully handle tracks that might be missing artist or album metadata in Plex - try: - artist_title = track.artist().title if track.artist() else "Unknown Artist" - except (NotFound, AttributeError): - artist_title = "Unknown Artist" - - try: - album_title = track.album().title if track.album() else "Unknown Album" - except (NotFound, AttributeError): - album_title = "Unknown Album" - return cls( - id=str(track.ratingKey), - title=track.title, - artist=artist_title, - album=album_title, - duration=track.duration, - track_number=track.trackNumber, - year=track.year, - rating=track.userRating - ) - -@dataclass -class PlexPlaylistInfo: - id: str - title: str - description: Optional[str] - duration: int - leaf_count: int - tracks: List[PlexTrackInfo] - - @classmethod - def from_plex_playlist(cls, playlist: PlexPlaylist) -> 'PlexPlaylistInfo': - tracks = [] - for item in playlist.items(): - if isinstance(item, PlexTrack): - tracks.append(PlexTrackInfo.from_plex_track(item)) - - return cls( - id=str(playlist.ratingKey), - title=playlist.title, - description=playlist.summary, - duration=playlist.duration, - leaf_count=playlist.leafCount, - tracks=tracks - ) - -class PlexClient: +class PlexClient(MediaServerClient): def __init__(self): self.server: Optional[PlexServer] = None self.music_library: Optional[MusicSection] = None @@ -268,7 +215,7 @@ class PlexClient: """Check if both server is connected AND music library is selected.""" return self.server is not None and self.music_library is not None - def get_all_playlists(self) -> List[PlexPlaylistInfo]: + def get_all_playlists(self) -> List[PlaylistInfo]: if not self.ensure_connection(): logger.error("Not connected to Plex server") return [] @@ -278,7 +225,7 @@ class PlexClient: try: for playlist in self.server.playlists(): if getattr(playlist, 'playlistType', None) == 'audio': - playlist_info = PlexPlaylistInfo.from_plex_playlist(playlist) + playlist_info = PlaylistInfo.from_plex_playlist(playlist) playlists.append(playlist_info) logger.info(f"Retrieved {len(playlists)} audio playlists") @@ -288,14 +235,14 @@ class PlexClient: logger.error(f"Error fetching playlists: {e}") return [] - def get_playlist_by_name(self, name: str) -> Optional[PlexPlaylistInfo]: + def get_playlist_by_name(self, name: str) -> Optional[PlaylistInfo]: if not self.ensure_connection(): return None try: playlist = self.server.playlist(name) if getattr(playlist, 'playlistType', None) == 'audio': - return PlexPlaylistInfo.from_plex_playlist(playlist) + return PlaylistInfo.from_plex_playlist(playlist) return None except NotFound: @@ -311,14 +258,14 @@ class PlexClient: return False try: - # Handle both PlexTrackInfo objects and actual Plex track objects + # Handle both TrackInfo objects and actual Plex track objects plex_tracks = [] for track in tracks: if hasattr(track, 'ratingKey'): # This is already a Plex track object plex_tracks.append(track) elif hasattr(track, '_original_plex_track'): - # This is a PlexTrackInfo object with stored original track reference + # This is a TrackInfo object with stored original track reference original_track = track._original_plex_track if original_track is not None: plex_tracks.append(original_track) @@ -326,7 +273,7 @@ class PlexClient: else: logger.warning(f"Stored track reference is None for: {track.title} by {track.artist}") elif hasattr(track, 'title'): - # Fallback: This is a PlexTrackInfo object, need to find the actual track + # Fallback: This is a TrackInfo object, need to find the actual track plex_track = self._find_track(track.title, track.artist, track.album) if plex_track: plex_tracks.append(plex_track) @@ -448,7 +395,7 @@ class PlexClient: logger.error(f"Error copying playlist '{source_name}' to '{target_name}': {e}") return False - def update_playlist(self, playlist_name: str, tracks: List[PlexTrackInfo]) -> bool: + def update_playlist(self, playlist_name: str, tracks: List[TrackInfo]) -> bool: if not self.ensure_connection(): return False @@ -519,7 +466,7 @@ class PlexClient: logger.error(f"Error searching for track '{title}' by '{artist}': {e}") return None - def search_tracks(self, title: str, artist: str, limit: int = 15) -> List[PlexTrackInfo]: + def search_tracks(self, title: str, artist: str, limit: int = 15) -> List[TrackInfo]: """ Searches for tracks using an efficient, multi-stage "early exit" strategy. It stops and returns results as soon as candidates are found. @@ -554,7 +501,7 @@ class PlexClient: # --- Early Exit: If Stage 1 found results, stop here --- if candidate_tracks: logger.info(f"Found {len(candidate_tracks)} candidates in Stage 1. Exiting early.") - tracks = [PlexTrackInfo.from_plex_track(track) for track in candidate_tracks[:limit]] + tracks = [TrackInfo.from_plex_track(track) for track in candidate_tracks[:limit]] # Store references to original tracks for playlist creation for i, track_info in enumerate(tracks): if i < len(candidate_tracks): @@ -573,7 +520,7 @@ class PlexClient: # --- Early Exit: If Stage 2 found results, stop here --- if candidate_tracks: logger.info(f"Found {len(candidate_tracks)} candidates in Stage 2. Exiting early.") - tracks = [PlexTrackInfo.from_plex_track(track) for track in candidate_tracks[:limit]] + tracks = [TrackInfo.from_plex_track(track) for track in candidate_tracks[:limit]] # Store references to original tracks for playlist creation for i, track_info in enumerate(tracks): if i < len(candidate_tracks): @@ -587,7 +534,7 @@ class PlexClient: # Removed to prevent false positives where tracks with same title # but different artists are incorrectly matched - tracks = [PlexTrackInfo.from_plex_track(track) for track in candidate_tracks[:limit]] + tracks = [TrackInfo.from_plex_track(track) for track in candidate_tracks[:limit]] # Store references to original tracks for playlist creation for i, track_info in enumerate(tracks): diff --git a/core/soulsync_client.py b/core/soulsync_client.py index a7b3ff98..41e6bb7d 100644 --- a/core/soulsync_client.py +++ b/core/soulsync_client.py @@ -190,7 +190,10 @@ class SoulSyncArtist: return self._albums -class SoulSyncClient: +from core.media_server.contract import MediaServerClient + + +class SoulSyncClient(MediaServerClient): """Filesystem-based media server client for standalone SoulSync operation. Scans the Transfer folder recursively, reads audio file tags, and diff --git a/core/web_scan_manager.py b/core/web_scan_manager.py index 810490ce..813fc834 100644 --- a/core/web_scan_manager.py +++ b/core/web_scan_manager.py @@ -19,16 +19,21 @@ class WebScanManager: - Progress tracking and status reporting """ - def __init__(self, media_clients, delay_seconds: int = 60): + def __init__(self, media_server_engine, delay_seconds: int = 60): """ Initialize the web scan manager. Args: - media_clients: Dict containing plex_client, jellyfin_client, navidrome_client + media_server_engine: MediaServerEngine that owns the per-server + clients. Replaces the legacy ``media_clients`` dict — the + manager now resolves the active server's client through + ``self._engine.client(name)`` instead of a hand-keyed + dict that drifted out of sync with the engine's source + of truth. delay_seconds: Debounce delay in seconds (default 60s) """ self.delay = delay_seconds - self.media_clients = media_clients + self._engine = media_server_engine self._timer = None self._scan_in_progress = False self._downloads_during_scan = False @@ -44,29 +49,19 @@ class WebScanManager: logger.info(f"WebScanManager initialized with {delay_seconds}s debounce delay") def _get_active_media_client(self): - """Get the active media client based on config settings""" + """Get the active media client through the engine.""" try: from config.settings import config_manager active_server = config_manager.get_active_media_server() - server_client_map = { - 'jellyfin': 'jellyfin_client', - 'navidrome': 'navidrome_client', - 'plex': 'plex_client', - 'soulsync': 'soulsync_library_client', - } + if not self._engine: + logger.error("Web scan manager has no engine reference") + return None, None - # Try to get the configured active server - if active_server in server_client_map: - client_key = server_client_map[active_server] - client = self.media_clients.get(client_key) - if client and hasattr(client, 'is_connected') and client.is_connected(): - return client, active_server - else: - logger.warning(f"{active_server.title()} client not connected — scan skipped") - return None, None - - logger.error("No active media server configured for scanning") + client = self._engine.client(active_server) + if client and hasattr(client, 'is_connected') and client.is_connected(): + return client, active_server + logger.warning(f"{(active_server or 'unknown').title()} client not connected — scan skipped") return None, None except Exception as e: diff --git a/docs/media-server-engine-refactor-plan.md b/docs/media-server-engine-refactor-plan.md new file mode 100644 index 00000000..9f271f66 --- /dev/null +++ b/docs/media-server-engine-refactor-plan.md @@ -0,0 +1,159 @@ +# Media Server Engine Refactor Plan + +## Goal + +Same playbook as the download engine refactor, applied to media servers. Replace 33 `if active_server == 'plex' / 'jellyfin' / 'navidrome' / 'soulsync'` dispatch sites in web_server.py with a central `MediaServerEngine` that owns server selection + cross-server query dispatch. Each per-server client stays as-is for its protocol-specific work (Plex API SDK, Jellyfin REST, Navidrome OpenSubsonic, SoulSync filesystem walk). + +**The smell:** Adding a 5th server (recent SoulSync standalone showed how) requires hunting through 33 web_server.py dispatch sites + DatabaseUpdateWorker branching + UI sync-button visibility hacks. Plex assumptions had leaked into nominally generic code paths. + +## Architecture target + +``` +┌───────────┐ ┌──────────────────┐ +│ feature │ ─search/scan/sync─▶ ┌──│ MediaServerEngine │ ─▶ Plex (PlexAPI SDK) +│ │ ◀── results ────── │ │ ◆ active selection│ ─▶ Jellyfin (REST) +└───────────┘ │ │ ◆ dispatch │ ─▶ Navidrome (Subsonic) + │ │ ◆ shared shape │ ─▶ SoulSync (filesystem) + │ └──────────────────┘ + │ (no need for per-server thread pool — server APIs + │ are sync-call shaped, not stream-shaped like downloads) +``` + +## What clients keep (per-server, legitimately) + +- **Auth** — Plex token, Jellyfin API key + user ID + library ID, Navidrome user/pass + salt, SoulSync filesystem path +- **Wire protocol** — PlexAPI SDK objects vs Jellyfin REST `/Items` vs Navidrome XML/JSON Subsonic vs filesystem walk +- **ID schemes** — Plex ratingKey (int), Jellyfin GUID (hex), Navidrome string, SoulSync MD5 +- **Connection state** — each client owns its session / token / connection object +- **Cache strategy** — Jellyfin's aggressive pre-cache, Navidrome's folder filter, SoulSync's 5-min TTL +- **Wrapper objects** — `JellyfinTrack`, `NavidromeTrack`, etc. stay in client modules + +## What moves into the engine + +| Today (web_server.py duplicated) | Tomorrow (MediaServerEngine, single dispatch site) | +|---|---| +| `if active_server == 'plex': plex_client.X() elif 'jellyfin': jellyfin_client.X() ...` | `engine.X()` — engine reads `active_server` once, routes | +| Status check 4-way branch | `engine.is_connected()` | +| Library scan trigger 3-way | `engine.trigger_library_scan()` | +| Search dispatch in 3 sites | `engine.search_tracks(title, artist)` | +| Play history 4-way | `engine.get_play_history()` | +| Metadata writeback 4-way (genre / poster / bio) | `engine.update_artist_genres()` etc. — engine routes, plugin no-ops where unsupported | +| `DatabaseUpdateWorker.server_type` branching | `engine` injected, no per-server branching | + +## Plugin contract (ABC, narrower than the download contract) + +```python +class MediaServerClient(ABC): + # Connection + @abstractmethod + def is_connected(self) -> bool: ... + @abstractmethod + def ensure_connection(self) -> bool: ... + + # Library reads (required) + @abstractmethod + def get_all_artists(self): ... + @abstractmethod + def get_all_album_ids(self): ... + @abstractmethod + def search_tracks(self, title, artist, limit=15): ... + @abstractmethod + def get_recently_added_albums(self, max_results=400): ... + + # Library writes (required for the scan endpoints — may no-op for SoulSync) + @abstractmethod + def trigger_library_scan(self) -> bool: ... + @abstractmethod + def is_library_scanning(self) -> bool: ... + + # Playlist sync (optional — Navidrome stub returns True) + def create_playlist(self, name, tracks) -> bool: return True + def update_playlist(self, name, tracks) -> bool: return True + def copy_playlist(self, source, dest_name) -> bool: return True + + # Analytics (optional) + def get_play_history(self, limit=500) -> list: return [] + def get_track_play_counts(self) -> dict: return {} + + # Metadata writeback (optional — clients no-op where API doesn't support it) + def update_artist_genres(self, artist, genres) -> bool: return True + def update_artist_poster(self, artist, image_data) -> bool: return True + def update_album_poster(self, album, image_data) -> bool: return True + def update_artist_biography(self, artist) -> bool: return True +``` + +## Phased commit plan + +### Phase 0 — Foundation +- **0.1** Plugin contract (`core/media_server/contract.py`) + ABC +- **0.2** Registry + dispatch helpers (`core/media_server/registry.py`) +- **0.3** Conformance tests — every registered client satisfies the ABC + +### Phase A — Behavior pinning +- **A1** Pin Plex client surface (status check, search, scan trigger, metadata writeback shape) +- **A2** Pin Jellyfin client surface +- **A3** Pin Navidrome client surface +- **A4** Pin SoulSync standalone client surface + +### Phase B — Engine skeleton +- **B1** `MediaServerEngine` skeleton — holds clients, exposes `active_server` selection +- **B2** Engine dispatch methods (`is_connected`, `search_tracks`, `trigger_library_scan`, `is_library_scanning`, etc.) +- **B3** Engine-level conformance tests + +### Phase C — Migrate web_server.py dispatch sites +Each commit migrates a logical cluster (status, scan, playlist, metadata, history) so the diff per commit is small + reviewable. + +- **C1** Status / connection-check sites +- **C2** Library scan trigger + scanning-state polling +- **C3** Search dispatch (3 sites) +- **C4** Playlist sync / apply / suggest +- **C5** Play history + track play counts +- **C6** Metadata writeback (genres / posters / bio) + +### Phase D — DatabaseUpdateWorker +- **D1** Inject engine into `DatabaseUpdateWorker` +- **D2** Strip `self.server_type` branching (use `engine.get_active_client_type()` where shape genuinely differs) + +### Phase E — Cleanup + ship +- **E1** Drop unused imports / dead code +- **E2** WHATS_NEW + PR description + +**Total estimated:** 15-18 commits. ~600 LOC moved, ~200 LOC deleted. + +## Risk profile + +**Low:** +- Phase 0 (pure additive — new contract, no existing code touched) +- Phase A (tests only) +- Phase B (engine exists but nothing routes through it yet) + +**Medium:** +- Phase C (each commit changes 5-10 dispatch sites in web_server.py — cross-cutting) +- Phase D (DatabaseUpdateWorker is a hot path during library refresh) + +**Mitigation:** +- Phase A pinning catches per-client behavior drift +- Phase B engine tested in isolation before C wires it in +- Phase C commits are small + reversible (one cluster per commit) +- Suite green at every commit + +## What's NOT in this PR + +- Unifying track ID schemes (Plex int vs Jellyfin GUID vs Navidrome string vs SoulSync MD5) — separate data model refactor +- Merging Plex/Jellyfin/Navidrome wrapper objects (each tied to its API) +- Extracting common metadata writeback logic — too server-specific +- Adding new server types (Subsonic, MusicBrainz local) — out of scope; this PR makes them easier later + +## Coordination with other work + +- Independent of the download engine refactor (PR #494) — different subsystem +- Cin's metadata engine work also independent +- No collisions expected + +## Compatibility commitments + +- All existing config keys preserved +- All existing API endpoints preserved +- Plex/Jellyfin/Navidrome/SoulSync clients keep their public methods (engine wraps, doesn't replace) +- Active-server config (`server.active`) unchanged +- `DatabaseUpdateWorker.server_type` available for external readers (deprecated but not removed) diff --git a/services/sync_service.py b/services/sync_service.py index 71a3cd35..a9b645a8 100644 --- a/services/sync_service.py +++ b/services/sync_service.py @@ -4,9 +4,7 @@ from dataclasses import dataclass from datetime import datetime from utils.logging_config import get_logger from core.spotify_client import SpotifyClient, Playlist as SpotifyPlaylist, Track as SpotifyTrack -from core.plex_client import PlexClient, PlexTrackInfo -from core.jellyfin_client import JellyfinClient -from core.navidrome_client import NavidromeClient +from core.media_server.types import TrackInfo from core.download_orchestrator import DownloadOrchestrator from core.matching_engine import MusicMatchingEngine, MatchResult @@ -44,17 +42,35 @@ class SyncProgress: failed_tracks: int = 0 class PlaylistSyncService: - def __init__(self, spotify_client: SpotifyClient, plex_client: PlexClient, download_orchestrator: DownloadOrchestrator, jellyfin_client: JellyfinClient = None, navidrome_client = None): + def __init__(self, spotify_client: SpotifyClient, download_orchestrator: DownloadOrchestrator, media_server_engine=None): + """Initialize the sync service. + + ``media_server_engine`` is the central MediaServerEngine that owns + the per-server clients (Plex / Jellyfin / Navidrome / SoulSync). + Replaces the legacy per-server kwargs (plex_client / jellyfin_client + / navidrome_client) — all media-server access now goes through + ``self._engine.client(name)`` so swapping the active server doesn't + need a service rebuild. + + ``download_orchestrator`` (formerly ``soulseek_client``) is the + DownloadOrchestrator that owns the per-source download clients — + rename + retype landed via the download refactor PR. + """ self.spotify_client = spotify_client - self.plex_client = plex_client - self.jellyfin_client = jellyfin_client - self.navidrome_client = navidrome_client + self._engine = media_server_engine self.download_orchestrator = download_orchestrator self.progress_callbacks = {} # Playlist-specific progress callbacks self.syncing_playlists = set() # Track multiple syncing playlists self._cancelled = False self.matching_engine = MusicMatchingEngine() - + + def _media_client(self, name: str): + """Resolve a per-server client through the engine, or None when the + engine isn't wired (defensive — every production path passes one).""" + if self._engine is None: + return None + return self._engine.client(name) + def _get_active_media_client(self, profile_id=None): """Get the active media client based on config settings. @@ -68,26 +84,27 @@ class PlaylistSyncService: active_server = config_manager.get_active_media_server() if active_server == "jellyfin": - if not self.jellyfin_client: + client = self._media_client('jellyfin') + if not client: logger.error("Jellyfin client not provided to sync service") return None, "jellyfin" - # Apply per-profile Jellyfin library if set if profile_id: - self._apply_profile_library(profile_id, 'jellyfin', self.jellyfin_client) - return self.jellyfin_client, "jellyfin" + self._apply_profile_library(profile_id, 'jellyfin', client) + return client, "jellyfin" elif active_server == "navidrome": - if not self.navidrome_client: + client = self._media_client('navidrome') + if not client: logger.error("Navidrome client not provided to sync service") return None, "navidrome" - return self.navidrome_client, "navidrome" + return client, "navidrome" else: # Default to Plex - # Apply per-profile Plex library if set - if profile_id: - self._apply_profile_library(profile_id, 'plex', self.plex_client) - return self.plex_client, "plex" + client = self._media_client('plex') + if profile_id and client: + self._apply_profile_library(profile_id, 'plex', client) + return client, "plex" except Exception as e: logger.error(f"Error determining active media server: {e}") - return self.plex_client, "plex" # Fallback to Plex + return self._media_client('plex'), "plex" # Fallback to Plex def _apply_profile_library(self, profile_id, server_type, client): """Apply per-profile library selection to a media client if configured.""" @@ -441,7 +458,7 @@ class PlaylistSyncService: self.clear_progress_callback(playlist.name) self._cancelled = False - async def _find_track_in_media_server(self, spotify_track: SpotifyTrack) -> Tuple[Optional[PlexTrackInfo], float]: + async def _find_track_in_media_server(self, spotify_track: SpotifyTrack) -> Tuple[Optional[TrackInfo], float]: """Find a track using the same improved database matching as Download Missing Tracks modal""" try: # Check active media server connection diff --git a/tests/discovery/test_discovery_sync.py b/tests/discovery/test_discovery_sync.py index f8fa3119..8cd9eee0 100644 --- a/tests/discovery/test_discovery_sync.py +++ b/tests/discovery/test_discovery_sync.py @@ -35,6 +35,15 @@ class _FakeMediaClient: return self._connected +class _FakeMediaServerEngine: + """Stand-in for MediaServerEngine — only the bits SyncDeps needs.""" + def __init__(self, plex=None, jellyfin=None, navidrome=None): + self._clients = {'plex': plex, 'jellyfin': jellyfin, 'navidrome': navidrome} + + def client(self, name): + return self._clients.get(name) + + class _FakeSyncService: def __init__(self, *, media_client=None, server_type='plex', sync_result=None, raise_on_sync=None, @@ -44,8 +53,12 @@ class _FakeSyncService: self._sync_result = sync_result or _FakeSyncResult() self._raise_on_sync = raise_on_sync self.spotify_client = object() if spotify_client else None - self.plex_client = object() if plex_client else None - self.jellyfin_client = object() if jellyfin_client else None + # The sync_service exposes the engine so the discovery worker + # can introspect per-server clients via self._engine.client(name). + self._engine = _FakeMediaServerEngine( + plex=object() if plex_client else None, + jellyfin=object() if jellyfin_client else None, + ) self.progress_callback = None self.progress_playlist_name = None self.cleared_callbacks = [] @@ -130,8 +143,10 @@ def _build_deps( return ds.SyncDeps( config_manager=config or _FakeConfig(), sync_service=sync_service or _FakeSyncService(media_client=_FakeMediaClient()), - plex_client=plex or _FakePlex(), - jellyfin_client=jellyfin or _FakeJellyfin(), + media_server_engine=_FakeMediaServerEngine( + plex=plex or _FakePlex(), + jellyfin=jellyfin or _FakeJellyfin(), + ), automation_engine=automation or _FakeAutomationEngine(), run_async=run_async or _run_async_sync, record_sync_history_start=record_sync_history_start or (lambda **kw: None), diff --git a/tests/media_server/__init__.py b/tests/media_server/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/tests/media_server/test_conformance.py b/tests/media_server/test_conformance.py new file mode 100644 index 00000000..dcf789c6 --- /dev/null +++ b/tests/media_server/test_conformance.py @@ -0,0 +1,72 @@ +"""Pin the structural conformance of every media server client to +``MediaServerClient``. Mirrors the download plugin conformance test +shape — class-level checks (not instance-level) so importing the +test doesn't drag every server's heavy auth init into the test +collection phase. +""" + +from __future__ import annotations + +import pytest + +from core.media_server.contract import REQUIRED_METHODS + + +def _import_server_classes(): + """Import every server client class lazily inside tests so + auth-init-heavy modules (Plex, Jellyfin) aren't imported at test + collection.""" + from core.jellyfin_client import JellyfinClient + from core.navidrome_client import NavidromeClient + from core.plex_client import PlexClient + from core.soulsync_client import SoulSyncClient + + return { + 'plex': PlexClient, + 'jellyfin': JellyfinClient, + 'navidrome': NavidromeClient, + 'soulsync': SoulSyncClient, + } + + +def test_default_registry_registers_all_four_servers(): + """Smoke check that the foundation registry knows about every + server SoulSync historically dispatched to.""" + from core.media_server.registry import build_default_registry + + registry = build_default_registry() + expected = {'plex', 'jellyfin', 'navidrome', 'soulsync'} + assert set(registry.names()) == expected + + +@pytest.mark.parametrize('server_name', ['plex', 'jellyfin', 'navidrome', 'soulsync']) +def test_server_class_has_all_required_methods(server_name): + """Every registered server class exposes every required protocol + method by name. Diagnostic-friendly: tells you WHICH method is + missing when a new server is added without all the required + methods.""" + classes = _import_server_classes() + cls = classes[server_name] + + missing = [m for m in REQUIRED_METHODS if not hasattr(cls, m)] + assert not missing, ( + f"{server_name} ({cls.__name__}) missing required methods: {missing}" + ) + + +@pytest.mark.parametrize('server_name', ['plex', 'jellyfin', 'navidrome', 'soulsync']) +def test_server_class_explicitly_inherits_contract(server_name): + """Per Cin's standard from the download refactor: clients must + explicitly inherit ``MediaServerClient`` so the contract conformance + is obvious from reading the class declaration. Structural + typing alone (which would still pass `hasattr` checks) leaves + the contract invisible to anyone reading the code — drift in a + future client class wouldn't fail at the contract boundary.""" + from core.media_server.contract import MediaServerClient + + classes = _import_server_classes() + cls = classes[server_name] + assert issubclass(cls, MediaServerClient), ( + f"{cls.__name__} must explicitly inherit MediaServerClient — " + f"structural typing isn't enough" + ) diff --git a/tests/media_server/test_engine.py b/tests/media_server/test_engine.py new file mode 100644 index 00000000..262fbe1e --- /dev/null +++ b/tests/media_server/test_engine.py @@ -0,0 +1,302 @@ +"""Tests for MediaServerEngine cross-server dispatch.""" + +from __future__ import annotations + +from unittest.mock import MagicMock + +import pytest + +from core.media_server.engine import MediaServerEngine +from core.media_server.registry import MediaServerRegistry, ServerSpec + + +class _FakeClient: + """Stand-in client supporting all required + optional methods. + Tests selectively override per-test to assert dispatch behavior.""" + + def __init__(self, name='fake', connected=True): + self.name = name + self._connected = connected + self._artists = [] + self._album_ids = set() + self._search_results = [] + self._scan_triggered = False + + def is_connected(self): + return self._connected + + def ensure_connection(self): + return self._connected + + def get_all_artists(self): + return self._artists + + def get_all_album_ids(self): + return self._album_ids + + def search_tracks(self, title, artist, limit=15): + return self._search_results + + def trigger_library_scan(self): + self._scan_triggered = True + return True + + def is_library_scanning(self): + return False + + def get_library_stats(self): + return {'artists': 0, 'albums': 0, 'tracks': 0} + + def get_recently_added_albums(self, max_results=400): + return [] + + +@pytest.fixture +def make_engine(): + """Build an engine with mock clients. Test passes a dict of + name → client; engine wires them via a registry + custom + active-server resolver.""" + + def _make(clients_by_name, active='plex'): + registry = MediaServerRegistry() + for name, client in clients_by_name.items(): + registry.register(ServerSpec( + name=name, + factory=lambda c=client: c, + display_name=name.title(), + )) + return MediaServerEngine( + registry=registry, + active_server_resolver=lambda: active, + ) + + return _make + + +# --------------------------------------------------------------------------- +# Active-server resolution +# --------------------------------------------------------------------------- + + +def test_active_server_property_reflects_resolver(make_engine): + plex = _FakeClient('plex') + jelly = _FakeClient('jellyfin') + engine = make_engine({'plex': plex, 'jellyfin': jelly}, active='jellyfin') + assert engine.active_server == 'jellyfin' + assert engine.active_client() is jelly + + +def test_client_lookup_by_name(make_engine): + plex = _FakeClient('plex') + engine = make_engine({'plex': plex}, active='plex') + assert engine.client('plex') is plex + assert engine.client('made_up') is None + + +# --------------------------------------------------------------------------- +# Required-method dispatch +# --------------------------------------------------------------------------- + + +def test_is_connected_routes_to_active_client(make_engine): + plex = _FakeClient('plex', connected=True) + jelly = _FakeClient('jellyfin', connected=False) + engine = make_engine({'plex': plex, 'jellyfin': jelly}, active='jellyfin') + assert engine.is_connected() is False # follows jellyfin + engine = make_engine({'plex': plex, 'jellyfin': jelly}, active='plex') + assert engine.is_connected() is True # follows plex + + +def test_engine_is_connected_returns_false_when_active_client_failed_to_init(): + """When the active client failed to initialize (registry stored + None), the engine returns False instead of raising.""" + registry = MediaServerRegistry() + registry.register(ServerSpec( + name='broken', + factory=lambda: (_ for _ in ()).throw(RuntimeError("init failed")), + display_name='Broken', + )) + registry.initialize() # captures the exception + engine = MediaServerEngine(registry=registry, active_server_resolver=lambda: 'broken') + + assert engine.is_connected() is False + assert engine.active_client() is None + + +def test_is_connected_swallows_exception_from_client(make_engine): + """If the client's is_connected raises, engine returns False + instead of propagating — dashboard status indicators stay + responsive even if a server is misbehaving.""" + plex = _FakeClient('plex') + plex.is_connected = MagicMock(side_effect=RuntimeError("boom")) + engine = make_engine({'plex': plex}, active='plex') + + assert engine.is_connected() is False + + +# --------------------------------------------------------------------------- +# Generic accessors (Cin's standard from the download refactor) +# --------------------------------------------------------------------------- + + +def test_configured_clients_only_returns_connected_servers(make_engine): + """Replaces the legacy per-server `if X and X.is_connected(): ...` + chains in web_server.py. Single call returns the dict.""" + plex = _FakeClient('plex', connected=True) + jelly = _FakeClient('jellyfin', connected=False) + soulsync = _FakeClient('soulsync', connected=True) + engine = make_engine( + {'plex': plex, 'jellyfin': jelly, 'soulsync': soulsync}, + active='plex', + ) + result = engine.configured_clients() + assert set(result.keys()) == {'plex', 'soulsync'} + assert result['plex'] is plex + assert result['soulsync'] is soulsync + + +def test_configured_clients_skips_clients_whose_is_connected_raises(make_engine): + """Defensive: a single broken is_connected() must not crash the + iteration. Healthy clients still come back.""" + healthy = _FakeClient('plex', connected=True) + broken = _FakeClient('jellyfin') + broken.is_connected = MagicMock(side_effect=RuntimeError("boom")) + engine = make_engine({'plex': healthy, 'jellyfin': broken}, active='plex') + result = engine.configured_clients() + assert 'plex' in result + assert 'jellyfin' not in result + + +def test_reload_config_dispatches_to_named_server(make_engine): + """Generic dispatch — caller passes server name instead of + reaching for plex_client.reload_config() directly.""" + + class _ReloadablePlex(_FakeClient): + def __init__(self): + super().__init__('plex') + self.reload_called = False + + def reload_config(self): + self.reload_called = True + + plex = _ReloadablePlex() + soulsync = _FakeClient('soulsync') # No reload_config method + engine = make_engine({'plex': plex, 'soulsync': soulsync}, active='plex') + + assert engine.reload_config('plex') is True + assert plex.reload_called is True + + +def test_reload_config_skips_clients_without_method(make_engine): + """Servers that don't expose reload_config are skipped silently + (return True).""" + soulsync = _FakeClient('soulsync') + engine = make_engine({'soulsync': soulsync}, active='soulsync') + assert engine.reload_config('soulsync') is True + + +def test_reload_config_with_no_args_reloads_every_server(make_engine): + """When called with no name, hits every registered server that + exposes reload_config.""" + + class _ReloadableClient(_FakeClient): + def __init__(self, name): + super().__init__(name) + self.reload_called = False + + def reload_config(self): + self.reload_called = True + + plex = _ReloadableClient('plex') + jelly = _ReloadableClient('jellyfin') + engine = make_engine({'plex': plex, 'jellyfin': jelly}, active='plex') + + engine.reload_config() + assert plex.reload_called is True + assert jelly.reload_called is True + + +# --------------------------------------------------------------------------- +# Singleton factory (matches get_metadata_engine() / get_download_orchestrator()) +# --------------------------------------------------------------------------- + + +def test_get_media_server_engine_returns_set_singleton(make_engine): + """When set_media_server_engine has been called (web_server.py + does this at boot), get_media_server_engine returns the installed + instance instead of building a fresh one with the default registry.""" + from core.media_server.engine import ( + get_media_server_engine, + set_media_server_engine, + ) + + engine = make_engine({'plex': _FakeClient('plex')}, active='plex') + set_media_server_engine(engine) + try: + assert get_media_server_engine() is engine + finally: + set_media_server_engine(None) + + +# --------------------------------------------------------------------------- +# Empty-engine fallback (web_server.py boot resilience) +# --------------------------------------------------------------------------- + + +def test_engine_with_empty_clients_dict_is_safe_to_use(): + """web_server.py falls back to ``MediaServerEngine(clients={})`` if + full engine init raises — preserves the resilience the per-server + globals had pre-refactor (each one had its own try/except so engine + failure didn't take down dispatch sites). Pin the contract: empty + engine still answers safely on every accessor instead of raising.""" + registry = MediaServerRegistry() + registry.register(ServerSpec( + name='plex', factory=lambda: _FakeClient('plex'), display_name='Plex', + )) + engine = MediaServerEngine( + registry=registry, + clients={}, # No pre-built clients passed. + active_server_resolver=lambda: 'plex', + ) + + # client(name) returns None for every server — engine doesn't crash. + assert engine.client('plex') is None + assert engine.client('jellyfin') is None + # Active-client lookup + is_connected gracefully handle the empty + # case without raising. + assert engine.active_client() is None + assert engine.is_connected() is False + # configured_clients() returns empty dict cleanly. + assert engine.configured_clients() == {} + + +# --------------------------------------------------------------------------- +# Registry encapsulation (engine reaches via public set_instance, +# not the private _instances dict) +# --------------------------------------------------------------------------- + + +def test_engine_uses_registry_set_instance_not_private_attr(): + """Engine constructor with ``clients=`` must hand instances off + via the registry's public ``set_instance(name, client)`` method + rather than reaching into ``registry._instances`` directly. Pin + by giving the registry a ``set_instance`` spy and verifying the + engine calls it.""" + plex = _FakeClient('plex') + registry = MediaServerRegistry() + registry.register(ServerSpec( + name='plex', factory=lambda: _FakeClient('plex'), display_name='Plex', + )) + set_instance_calls = [] + original = registry.set_instance + def _spy(name, client): + set_instance_calls.append((name, client)) + original(name, client) + registry.set_instance = _spy + + MediaServerEngine( + registry=registry, + clients={'plex': plex}, + active_server_resolver=lambda: 'plex', + ) + assert ('plex', plex) in set_instance_calls diff --git a/tests/media_server/test_jellyfin_pinning.py b/tests/media_server/test_jellyfin_pinning.py new file mode 100644 index 00000000..d05f407f --- /dev/null +++ b/tests/media_server/test_jellyfin_pinning.py @@ -0,0 +1,104 @@ +"""Phase A pinning tests for JellyfinClient. + +Pin the OBSERVABLE BEHAVIOR the engine will dispatch through after +Phase B/C. Jellyfin's connection-readiness check is stricter than +Plex (requires base_url + api_key + user_id + music_library_id all +present), and get_all_artists has a side effect of pre-populating +caches. +""" + +from __future__ import annotations + +from unittest.mock import MagicMock, patch + +import pytest + +from core.jellyfin_client import JellyfinClient + + +@pytest.fixture +def jellyfin_client(): + """A bare JellyfinClient with no real connection.""" + client = JellyfinClient.__new__(JellyfinClient) + client.base_url = None + client.api_key = None + client.user_id = None + client.music_library_id = None + client._connection_attempted = False + client._is_connecting = False + # Initialize the various caches the real __init__ sets up + client._album_cache = {} + client._track_cache = {} + client._artist_cache = {} + client._cache_populated = False + client._metadata_only_mode = False + return client + + +# --------------------------------------------------------------------------- +# is_connected +# --------------------------------------------------------------------------- + + +def test_is_connected_returns_false_when_no_credentials(jellyfin_client): + """Pinning: any of base_url / api_key / user_id / music_library_id + being None → False. All four must be set.""" + with patch.object(jellyfin_client, 'ensure_connection', return_value=False): + assert jellyfin_client.is_connected() is False + + +def test_is_connected_returns_true_only_when_all_four_set(jellyfin_client): + """Pinning: requires base_url AND api_key AND user_id AND + music_library_id. Setting only some → still False.""" + jellyfin_client._connection_attempted = True + + # Only some — still False + jellyfin_client.base_url = 'http://j' + jellyfin_client.api_key = 'k' + jellyfin_client.user_id = 'u' + jellyfin_client.music_library_id = None + assert jellyfin_client.is_connected() is False + + # All four — True + jellyfin_client.music_library_id = 'lib' + assert jellyfin_client.is_connected() is True + + +# --------------------------------------------------------------------------- +# get_all_artists / get_all_album_ids +# --------------------------------------------------------------------------- + + +def test_get_all_artists_returns_empty_when_not_connected(jellyfin_client): + with patch.object(jellyfin_client, 'ensure_connection', return_value=False): + assert jellyfin_client.get_all_artists() == [] + + +def test_get_all_artists_returns_empty_when_no_music_library_id(jellyfin_client): + """Pinning: even with ensure_connection True, no music_library_id + selected → empty (don't crash).""" + jellyfin_client.music_library_id = None + with patch.object(jellyfin_client, 'ensure_connection', return_value=True): + assert jellyfin_client.get_all_artists() == [] + + +def test_get_all_album_ids_returns_set(jellyfin_client): + """Pinning: returns a set of string Jellyfin GUID ids. Same shape + semantic as Plex (set of strings) — engine extraction depends on + uniform set type.""" + jellyfin_client._connection_attempted = True + jellyfin_client.base_url = 'http://j' + jellyfin_client.api_key = 'k' + jellyfin_client.user_id = 'u' + jellyfin_client.music_library_id = 'lib' + + fake_response_pages = [ + {'Items': [{'Id': 'guid-1'}, {'Id': 'guid-2'}], 'TotalRecordCount': 2}, + ] + + with patch.object(jellyfin_client, 'ensure_connection', return_value=True), \ + patch.object(jellyfin_client, '_make_request', side_effect=fake_response_pages): + result = jellyfin_client.get_all_album_ids() + + assert isinstance(result, set) + assert result == {'guid-1', 'guid-2'} diff --git a/tests/media_server/test_navidrome_pinning.py b/tests/media_server/test_navidrome_pinning.py new file mode 100644 index 00000000..500d3953 --- /dev/null +++ b/tests/media_server/test_navidrome_pinning.py @@ -0,0 +1,79 @@ +"""Phase A pinning tests for NavidromeClient. + +Pin the OBSERVABLE BEHAVIOR the engine will dispatch through. Auth +shape uses base_url + username + password (no token like Plex). +Both get_all_artists + get_all_album_ids return empty when the +client isn't connected. +""" + +from __future__ import annotations + +from unittest.mock import MagicMock, patch + +import pytest + +from core.navidrome_client import NavidromeClient + + +@pytest.fixture +def nav_client(): + """A bare NavidromeClient with no real connection.""" + client = NavidromeClient.__new__(NavidromeClient) + client.base_url = None + client.username = None + client.password = None + client._connection_attempted = False + client._is_connecting = False + client._artist_cache = {} + client._album_cache = {} + client._track_cache = {} + client._folder_album_ids = None + client._progress_callback = None + client.music_folder_id = None + return client + + +def test_is_connected_returns_false_without_credentials(nav_client): + with patch.object(nav_client, 'ensure_connection', return_value=False): + assert nav_client.is_connected() is False + + +def test_is_connected_returns_true_when_all_three_set(nav_client): + """Pinning: requires base_url AND username AND password. + No token model (vs Plex). Salt is generated per-request.""" + nav_client._connection_attempted = True + nav_client.base_url = 'http://nav' + nav_client.username = 'u' + nav_client.password = 'p' + assert nav_client.is_connected() is True + + +def test_get_all_artists_returns_empty_when_not_connected(nav_client): + with patch.object(nav_client, 'ensure_connection', return_value=False): + assert nav_client.get_all_artists() == [] + + +def test_get_all_album_ids_returns_set(nav_client): + """Pinning: returns a set of string Navidrome ids. Same shape + semantic as Plex/Jellyfin (set of strings) — engine extraction + depends on uniform set type.""" + nav_client._connection_attempted = True + nav_client.base_url = 'http://nav' + nav_client.username = 'u' + nav_client.password = 'p' + + # Navidrome uses paginated getAlbumList2 — first page returns + # 2 albums, second returns empty (terminates loop). + # _make_request unwraps the subsonic-response envelope and + # returns the body. get_all_album_ids reads response.albumList2.album. + page_responses = [ + {'albumList2': {'album': [{'id': 'nav-1'}, {'id': 'nav-2'}]}}, + {'albumList2': {'album': []}}, + ] + + with patch.object(nav_client, 'ensure_connection', return_value=True), \ + patch.object(nav_client, '_make_request', side_effect=page_responses): + result = nav_client.get_all_album_ids() + + assert isinstance(result, set) + assert result == {'nav-1', 'nav-2'} diff --git a/tests/media_server/test_plex_pinning.py b/tests/media_server/test_plex_pinning.py new file mode 100644 index 00000000..7f314f73 --- /dev/null +++ b/tests/media_server/test_plex_pinning.py @@ -0,0 +1,113 @@ +"""Phase A pinning tests for PlexClient. + +Pin the OBSERVABLE BEHAVIOR the engine will dispatch through after +Phase B/C. The web_server.py dispatch sites + DatabaseUpdateWorker +read these methods generically — they must keep their current shape +through the engine refactor. + +Plex's surface is wider than the contract requires (additional +playlist / metadata writeback methods), but pinning ALL of them +turns into ~30 tests. Focused on the methods the dispatch sites +actually call: is_connected, is_fully_configured, ensure_connection, +get_all_artists, get_all_album_ids, search_tracks, trigger_library_scan, +is_library_scanning, get_library_stats. +""" + +from __future__ import annotations + +from unittest.mock import MagicMock, patch + +import pytest + +from core.plex_client import PlexClient + + +@pytest.fixture +def plex_client(): + """A bare PlexClient with no real connection. Tests that need + a connected state set client.server = MagicMock() directly.""" + client = PlexClient.__new__(PlexClient) + client.server = None + client.music_library = None + client._connection_attempted = False + client._is_connecting = False + client._last_connection_check = 0 + client._connection_check_interval = 30 + return client + + +# --------------------------------------------------------------------------- +# is_connected +# --------------------------------------------------------------------------- + + +def test_is_connected_returns_false_when_no_server(plex_client): + """Pinning: no server object → False. Dashboard status indicators + + endpoint guards depend on this.""" + plex_client.server = None + # Patch ensure_connection to avoid network call + with patch.object(plex_client, 'ensure_connection', return_value=False): + assert plex_client.is_connected() is False + + +def test_is_connected_returns_true_when_server_present(plex_client): + """Pinning: a non-None server object → True (even if music_library + is unset). is_fully_configured() is the stricter check.""" + plex_client.server = MagicMock() + plex_client._connection_attempted = True + assert plex_client.is_connected() is True + + +def test_is_fully_configured_requires_server_and_music_library(plex_client): + """Pinning: is_fully_configured == True only when BOTH server AND + music_library are set. Used by playlist-sync gating.""" + plex_client.server = MagicMock() + plex_client.music_library = None + assert plex_client.is_fully_configured() is False + plex_client.music_library = MagicMock() + assert plex_client.is_fully_configured() is True + + +# --------------------------------------------------------------------------- +# get_all_artists / get_all_album_ids +# --------------------------------------------------------------------------- + + +def test_get_all_artists_returns_empty_when_not_connected(plex_client): + """Pinning: ensure_connection returning False → empty list (not + exception). Caller iterates the result, never raising.""" + with patch.object(plex_client, 'ensure_connection', return_value=False): + assert plex_client.get_all_artists() == [] + + +def test_get_all_artists_iterates_searchartists_when_connected(plex_client): + """Pinning: when connected, get_all_artists calls + music_library.searchArtists() and returns the result list.""" + fake_artists = [MagicMock(title='A'), MagicMock(title='B')] + plex_client.server = MagicMock() + plex_client.music_library = MagicMock() + plex_client.music_library.searchArtists.return_value = fake_artists + + with patch.object(plex_client, 'ensure_connection', return_value=True): + result = plex_client.get_all_artists() + + assert result == fake_artists + + +def test_get_all_album_ids_returns_set_of_string_ratingkeys(plex_client): + """Pinning: returns a set (not a list) of string ratingKey values. + DatabaseUpdateWorker uses set membership for diff-detection. + NOTE: Plex stores ratingKey as int but the method coerces to str + so the set semantics match other servers (Jellyfin GUIDs, Navidrome + string ids). Engine refactor must preserve.""" + fake_albums = [MagicMock(ratingKey=1), MagicMock(ratingKey=2), + MagicMock(ratingKey=3)] + plex_client.server = MagicMock() + plex_client.music_library = MagicMock() + plex_client.music_library.albums.return_value = fake_albums + + with patch.object(plex_client, 'ensure_connection', return_value=True): + result = plex_client.get_all_album_ids() + + assert isinstance(result, set) + assert result == {'1', '2', '3'} diff --git a/tests/media_server/test_soulsync_pinning.py b/tests/media_server/test_soulsync_pinning.py new file mode 100644 index 00000000..d419c829 --- /dev/null +++ b/tests/media_server/test_soulsync_pinning.py @@ -0,0 +1,90 @@ +"""Phase A pinning tests for SoulSyncClient (standalone library mode). + +Pin the OBSERVABLE BEHAVIOR the engine will dispatch through. +SoulSync standalone is the structurally-different one — no auth / +no API / no library scan. is_connected just checks `transfer_path` +is a directory. Filesystem walk happens via _get_cached_scan(). +""" + +from __future__ import annotations + +import os +from unittest.mock import patch + +import pytest + +from core.soulsync_client import SoulSyncClient + + +@pytest.fixture +def ss_client(tmp_path): + """A bare SoulSyncClient pointed at a temp directory.""" + client = SoulSyncClient.__new__(SoulSyncClient) + client._transfer_path = str(tmp_path) + client._progress_callback = None + client._cache = None + client._cache_time = 0 + client._cache_ttl = 300 + client._last_scan_time = None + return client + + +# --------------------------------------------------------------------------- +# is_connected / ensure_connection +# --------------------------------------------------------------------------- + + +def test_is_connected_true_when_transfer_path_exists(ss_client, tmp_path): + """Pinning: filesystem-based connectivity. is_connected just + checks os.path.isdir(transfer_path).""" + assert ss_client.is_connected() is True # tmp_path exists + + +def test_is_connected_false_when_transfer_path_missing(ss_client): + """Pinning: missing transfer_path → False. The standalone case + where the user hasn't configured a folder yet.""" + ss_client._transfer_path = '/nonexistent/path/that/does/not/exist' + assert ss_client.is_connected() is False + + +def test_ensure_connection_reloads_config_then_checks_path(ss_client, tmp_path): + """Pinning: ensure_connection re-reads config (so the user + changing the transfer_path takes effect without a process + restart) and then checks the path. Returns True iff the path + is a directory.""" + with patch.object(ss_client, '_reload_config') as fake_reload: + result = ss_client.ensure_connection() + + fake_reload.assert_called_once() + assert result is True + + +# --------------------------------------------------------------------------- +# get_all_artists / get_all_album_ids +# --------------------------------------------------------------------------- + + +def test_get_all_artists_uses_cached_scan(ss_client): + """Pinning: get_all_artists delegates to _get_cached_scan which + enforces the 5-min TTL. Engine extraction must preserve.""" + fake_artists = [object(), object(), object()] + with patch.object(ss_client, '_get_cached_scan', return_value=fake_artists): + result = ss_client.get_all_artists() + assert result == fake_artists + + +def test_get_all_album_ids_returns_set(ss_client): + """Pinning: returns a set of MD5-hashed string ids. Same uniform + set semantics as Plex/Jellyfin/Navidrome.""" + fake_album = type('FakeAlbum', (), {'ratingKey': 'hash-1'})() + fake_album2 = type('FakeAlbum', (), {'ratingKey': 'hash-2'})() + # Real SoulSyncArtist exposes albums() method (not _albums attr). + fake_artist = type('FakeArtist', (), { + 'albums': lambda self: [fake_album, fake_album2], + })() + + with patch.object(ss_client, '_get_cached_scan', return_value=[fake_artist]): + result = ss_client.get_all_album_ids() + + assert isinstance(result, set) + assert result == {'hash-1', 'hash-2'} diff --git a/web_server.py b/web_server.py index 77c100f5..8355ea95 100644 --- a/web_server.py +++ b/web_server.py @@ -570,7 +570,7 @@ IS_SHUTTING_DOWN = False # Each client is initialized independently so one failure doesn't take down everything. # Previously, a single exception set ALL clients to None, breaking the entire app. logger.info("Initializing SoulSync services for Web UI...") -spotify_client = plex_client = jellyfin_client = navidrome_client = soulsync_library_client = download_orchestrator = tidal_client = matching_engine = sync_service = web_scan_manager = None +spotify_client = download_orchestrator = tidal_client = matching_engine = sync_service = web_scan_manager = media_server_engine = None try: spotify_client = get_spotify_client() @@ -578,30 +578,63 @@ try: except Exception as e: logger.error(f" Spotify client failed to initialize: {e}") -try: - plex_client = PlexClient() - logger.info(" Plex client initialized") -except Exception as e: - logger.error(f" Plex client failed to initialize: {e}") -try: - jellyfin_client = JellyfinClient() - logger.info(" Jellyfin client initialized") -except Exception as e: - logger.error(f" Jellyfin client failed to initialize: {e}") +def _safe_init_media_client(factory, name): + """Build a media-server client, capturing per-server init failures + so one broken server doesn't take the engine down with it. -try: - navidrome_client = NavidromeClient() - logger.info(" Navidrome client initialized") -except Exception as e: - logger.error(f" Navidrome client failed to initialize: {e}") + Logs the exception class so the boot log distinguishes config errors + (ConnectionError, AuthenticationError) from genuine import / dependency + failures — the broad ``Exception`` catch is intentional (any failure + on this code path means the user can't use that server, and we want + the other three to keep working) but the log makes the cause + diagnosable.""" + try: + instance = factory() + logger.info(f" {name} client initialized") + return instance + except Exception as exc: + logger.error( + f" {name} client failed to initialize: {type(exc).__name__}: {exc}", + exc_info=True, + ) + return None + +# Build the MediaServerEngine. The engine OWNS the per-server client +# instances — no separate web_server.py globals (Cin's standard from +# the download refactor: drop redundant access paths). All callers go +# through media_server_engine.client(''). try: + from core.media_server.engine import MediaServerEngine, set_media_server_engine from core.soulsync_client import SoulSyncClient - soulsync_library_client = SoulSyncClient() - logger.info(" SoulSync library client initialized") + media_server_engine = MediaServerEngine(clients={ + 'plex': _safe_init_media_client(PlexClient, "Plex"), + 'jellyfin': _safe_init_media_client(JellyfinClient, "Jellyfin"), + 'navidrome': _safe_init_media_client(NavidromeClient, "Navidrome"), + 'soulsync': _safe_init_media_client(SoulSyncClient, "SoulSync library"), + }) + # Install as process-wide singleton so callers reaching via + # get_media_server_engine() see the same instance web_server.py + # constructs at boot. Matches the metadata + download engine + # patterns. + set_media_server_engine(media_server_engine) + logger.info(" Media server engine initialized") except Exception as e: - logger.error(f" SoulSync library client failed to initialize: {e}") + logger.error(f" Media server engine failed to initialize: {e}", exc_info=True) + # Fallback: empty engine so downstream `engine.client('plex')` + # returns None instead of AttributeError'ing on a None engine + # global. Pre-refactor each per-server client global had its own + # try/except so engine failure didn't take down dispatch sites; + # this preserves that resilience. If the fallback ALSO fails + # (e.g. the import itself broke), media_server_engine stays as + # the None initialized at the top of the module. + try: + from core.media_server.engine import MediaServerEngine, set_media_server_engine + media_server_engine = MediaServerEngine(clients={}) + set_media_server_engine(media_server_engine) + except Exception as fallback_exc: + logger.error(f" Empty-engine fallback also failed: {fallback_exc}", exc_info=True) try: download_orchestrator = DownloadOrchestrator() @@ -626,7 +659,7 @@ except Exception as e: logger.error(f" Matching engine failed to initialize: {e}") try: - sync_service = PlaylistSyncService(spotify_client, plex_client, download_orchestrator, jellyfin_client, navidrome_client) + sync_service = PlaylistSyncService(spotify_client, download_orchestrator, media_server_engine=media_server_engine) logger.info(" Playlist sync service initialized") except Exception as e: logger.error(f" Playlist sync service failed to initialize: {e}") @@ -645,13 +678,7 @@ if download_orchestrator and hasattr(download_orchestrator, 'registry'): # Initialize web scan manager for automatic post-download scanning try: - media_clients = { - 'plex_client': plex_client, - 'jellyfin_client': jellyfin_client, - 'navidrome_client': navidrome_client, - 'soulsync_library_client': soulsync_library_client, - } - web_scan_manager = WebScanManager(media_clients, delay_seconds=60) + web_scan_manager = WebScanManager(media_server_engine, delay_seconds=60) logger.info(" Web scan manager initialized") except Exception as e: logger.error(f" Web scan manager failed to initialize: {e}") @@ -3461,23 +3488,14 @@ def get_status(): metadata_status = get_metadata_status_snapshot(spotify_client=spotify_client) - # Test media server - use EXISTING instances (they have internal caching) - # Media server clients already cache connection checks internally + # Test media server — engine reads active_server config + dispatches + # to the right client (with internal connection caching). Engine + # returns False safely if the active client is None / not registered. if current_time - _status_cache_timestamps['media_server'] > STATUS_CACHE_TTL: media_server_start = time.time() - media_server_status = False - if active_server == "plex" and plex_client: - # Use existing instance - has 30s internal connection cache - media_server_status = plex_client.is_connected() - elif active_server == "jellyfin" and jellyfin_client: - # Use existing instance - has internal connection caching - media_server_status = jellyfin_client.is_connected() - elif active_server == "navidrome" and navidrome_client: - # Use existing instance - media_server_status = navidrome_client.is_connected() - elif active_server == "soulsync": - # Standalone mode — always connected if Transfer folder exists - media_server_status = soulsync_library_client.is_connected() if soulsync_library_client else False + media_server_status = ( + media_server_engine.is_connected() if media_server_engine else False + ) media_server_response_time = (time.time() - media_server_start) * 1000 _status_cache['media_server'] = { 'connected': media_server_status, @@ -4142,12 +4160,12 @@ def handle_settings(): # Reload service clients with new settings (guard against None from partial init) if spotify_client: spotify_client.reload_config() - if plex_client: - plex_client.server = None - if jellyfin_client: - jellyfin_client.reload_config() - if navidrome_client: - navidrome_client.reload_config() + if media_server_engine.client('plex'): + media_server_engine.client('plex').server = None + if media_server_engine.client('jellyfin'): + media_server_engine.client('jellyfin').reload_config() + if media_server_engine.client('navidrome'): + media_server_engine.client('navidrome').reload_config() # Reload orchestrator settings (download source mode, hybrid_primary, etc.) if download_orchestrator: download_orchestrator.reload_settings() @@ -5111,8 +5129,8 @@ def clear_plex_library_preference(): from database.music_database import MusicDatabase db = MusicDatabase() db.set_preference('plex_music_library', '') - if plex_client: - plex_client.music_library = None + if media_server_engine.client('plex'): + media_server_engine.client('plex').music_library = None return jsonify({"success": True, "message": "Plex library preference cleared."}) except Exception as e: logger.error(f"Error clearing Plex library preference: {e}") @@ -5123,7 +5141,7 @@ def clear_plex_library_preference(): def get_plex_music_libraries(): """Get list of all available music libraries from Plex""" try: - libraries = plex_client.get_available_music_libraries() + libraries = media_server_engine.client('plex').get_available_music_libraries() # Get currently selected library from database.music_database import MusicDatabase @@ -5132,8 +5150,8 @@ def get_plex_music_libraries(): # Get the currently active library name current_library = None - if plex_client.music_library: - current_library = plex_client.music_library.title + if media_server_engine.client('plex').music_library: + current_library = media_server_engine.client('plex').music_library.title return jsonify({ "success": True, @@ -5155,7 +5173,7 @@ def select_plex_music_library(): if not library_name: return jsonify({"success": False, "error": "No library name provided"}), 400 - success = plex_client.set_music_library_by_name(library_name) + success = media_server_engine.client('plex').set_music_library_by_name(library_name) if success: add_activity_item("", "Library Selected", f"Plex music library set to: {library_name}", "Now") @@ -5171,7 +5189,7 @@ def select_plex_music_library(): def get_jellyfin_users(): """Get list of Jellyfin users that have music libraries""" try: - users = jellyfin_client.get_available_users() + users = media_server_engine.client('jellyfin').get_available_users() # Get currently selected user from database.music_database import MusicDatabase @@ -5180,9 +5198,9 @@ def get_jellyfin_users(): # Determine the current user name from user_id current_user = None - if jellyfin_client.user_id: + if media_server_engine.client('jellyfin').user_id: for u in users: - if u['id'] == jellyfin_client.user_id: + if u['id'] == media_server_engine.client('jellyfin').user_id: current_user = u['name'] break @@ -5206,7 +5224,7 @@ def select_jellyfin_user(): if not username: return jsonify({"success": False, "error": "No username provided"}), 400 - success = jellyfin_client.set_user_by_name(username) + success = media_server_engine.client('jellyfin').set_user_by_name(username) if success: add_activity_item("", "User Selected", f"Jellyfin user set to: {username}", "Now") @@ -5222,7 +5240,7 @@ def select_jellyfin_user(): def get_jellyfin_music_libraries(): """Get list of all available music libraries from Jellyfin""" try: - libraries = jellyfin_client.get_available_music_libraries() + libraries = media_server_engine.client('jellyfin').get_available_music_libraries() # Get currently selected library from database.music_database import MusicDatabase @@ -5231,10 +5249,10 @@ def get_jellyfin_music_libraries(): # Get the currently active library name (match Plex behavior) current_library = None - if jellyfin_client.music_library_id: + if media_server_engine.client('jellyfin').music_library_id: # Look up library name from ID for lib in libraries: - if lib['key'] == jellyfin_client.music_library_id: + if lib['key'] == media_server_engine.client('jellyfin').music_library_id: current_library = lib['title'] break @@ -5258,7 +5276,7 @@ def select_jellyfin_music_library(): if not library_name: return jsonify({"success": False, "error": "No library name provided"}), 400 - success = jellyfin_client.set_music_library_by_name(library_name) + success = media_server_engine.client('jellyfin').set_music_library_by_name(library_name) if success: add_activity_item("", "Library Selected", f"Jellyfin music library set to: {library_name}", "Now") @@ -5274,19 +5292,19 @@ def select_jellyfin_music_library(): def get_navidrome_music_folders(): """Get list of available music folders from Navidrome""" try: - if not navidrome_client: + if not media_server_engine.client('navidrome'): return jsonify({"success": False, "error": "Navidrome client not configured"}), 400 - folders = navidrome_client.get_music_folders() + folders = media_server_engine.client('navidrome').get_music_folders() from database.music_database import MusicDatabase db = MusicDatabase() selected_folder = db.get_preference('navidrome_music_folder') current_folder = None - if navidrome_client.music_folder_id: + if media_server_engine.client('navidrome').music_folder_id: for f in folders: - if f['key'] == navidrome_client.music_folder_id: + if f['key'] == media_server_engine.client('navidrome').music_folder_id: current_folder = f['title'] break @@ -5307,7 +5325,7 @@ def select_navidrome_music_folder(): data = request.get_json() folder_name = data.get('folder_name', '') - success = navidrome_client.set_music_folder_by_name(folder_name) + success = media_server_engine.client('navidrome').set_music_folder_by_name(folder_name) if success: if folder_name: @@ -6868,7 +6886,7 @@ def enhanced_search_library_check(): data = request.get_json() or {} result = _search_library_check.check_library_presence( database=get_database(), - plex_client=plex_client, + plex_client=media_server_engine.client('plex') if media_server_engine else None, config_manager=config_manager, profile_id=get_current_profile_id(), albums=data.get('albums', []), @@ -8035,9 +8053,9 @@ def test_automation_workflow(): results['media_clients'] = {'active_server': active_server} for client_name, client in [ - ('plex', plex_client), - ('jellyfin', jellyfin_client), - ('navidrome', navidrome_client) + ('plex', media_server_engine.client('plex')), + ('jellyfin', media_server_engine.client('jellyfin')), + ('navidrome', media_server_engine.client('navidrome')) ]: try: is_connected = client.is_connected() if client else False @@ -9295,15 +9313,9 @@ def get_artist_enhanced_detail(artist_id): if album.get('thumb_url'): album['thumb_url'] = fix_artist_image_url(album['thumb_url']) - # Include server type for sync option + # Include server type for sync option — engine routes to active client. active_server = config_manager.get_active_media_server() - server_connected = False - if active_server == 'plex': - server_connected = plex_client.is_connected() - elif active_server == 'jellyfin': - server_connected = jellyfin_client.is_connected() - elif active_server == 'navidrome': - server_connected = navidrome_client.is_connected() + server_connected = media_server_engine.is_connected() if media_server_engine else False result['server_type'] = active_server if server_connected else None return jsonify(result) @@ -9551,13 +9563,7 @@ def get_track_tag_preview(track_id): # Include server type so frontend can offer server sync option active_server = config_manager.get_active_media_server() - server_connected = False - if active_server == 'plex': - server_connected = plex_client.is_connected() - elif active_server == 'jellyfin': - server_connected = jellyfin_client.is_connected() - elif active_server == 'navidrome': - server_connected = navidrome_client.is_connected() + server_connected = media_server_engine.is_connected() if media_server_engine else False return jsonify({ "success": True, @@ -9658,15 +9664,9 @@ def get_batch_tag_preview(): results.append(entry) - # Server type info + # Server type info — engine routes to active client. active_server = config_manager.get_active_media_server() - server_connected = False - if active_server == 'plex': - server_connected = plex_client.is_connected() - elif active_server == 'jellyfin': - server_connected = jellyfin_client.is_connected() - elif active_server == 'navidrome': - server_connected = navidrome_client.is_connected() + server_connected = media_server_engine.is_connected() if media_server_engine else False return jsonify({ "success": True, @@ -10817,7 +10817,7 @@ def _sync_tracks_to_server(track_rows, server_type): if track_data.get('year'): metadata['year'] = track_data['year'] if metadata: - success = plex_client.update_track_metadata(str(track_data['id']), metadata) + success = media_server_engine.client('plex').update_track_metadata(str(track_data['id']), metadata) if success: result['synced'] += 1 else: @@ -10832,7 +10832,7 @@ def _sync_tracks_to_server(track_rows, server_type): elif server_type == 'jellyfin': # Jellyfin: just trigger a library scan once after all file writes try: - success = jellyfin_client.trigger_library_scan() + success = media_server_engine.client('jellyfin').trigger_library_scan() if success: result['synced'] = len(track_rows) else: @@ -10860,8 +10860,8 @@ def _resolve_library_file_path(file_path): # Also check the media server's music library path (handles Docker↔host path mismatch) library_dirs = set() try: - if plex_client and plex_client.server and plex_client.music_library: - for loc in plex_client.music_library.locations: + if media_server_engine.client('plex') and media_server_engine.client('plex').server and media_server_engine.client('plex').music_library: + for loc in media_server_engine.client('plex').music_library.locations: library_dirs.add(loc) except Exception: pass @@ -11533,9 +11533,9 @@ def redownload_search_metadata(track_id): if thumb_url and not thumb_url.startswith('http'): _ab = '' _at = '' - if plex_client and plex_client.server: - _ab = getattr(plex_client.server, '_baseurl', '') or '' - _at = getattr(plex_client.server, '_token', '') or '' + if media_server_engine.client('plex') and media_server_engine.client('plex').server: + _ab = getattr(media_server_engine.client('plex').server, '_baseurl', '') or '' + _at = getattr(media_server_engine.client('plex').server, '_token', '') or '' if not _ab: _pc = config_manager.get_plex_config() _ab = (_pc.get('base_url', '') or '').rstrip('/') @@ -11826,12 +11826,12 @@ def sync_artist_library(artist_id): if server_source: media_client = None - if server_source == 'plex' and plex_client and plex_client.server: - media_client = plex_client - elif server_source == 'jellyfin' and jellyfin_client: - media_client = jellyfin_client - elif server_source == 'navidrome' and navidrome_client: - media_client = navidrome_client + if server_source == 'plex' and media_server_engine.client('plex') and media_server_engine.client('plex').server: + media_client = media_server_engine.client('plex') + elif server_source == 'jellyfin' and media_server_engine.client('jellyfin'): + media_client = media_server_engine.client('jellyfin') + elif server_source == 'navidrome' and media_server_engine.client('navidrome'): + media_client = media_server_engine.client('navidrome') if media_client: try: @@ -15114,11 +15114,11 @@ def _run_db_update_task(full_refresh, server_type): media_client = None if server_type == "plex": - media_client = plex_client + media_client = media_server_engine.client('plex') elif server_type == "jellyfin": - media_client = jellyfin_client + media_client = media_server_engine.client('jellyfin') elif server_type == "navidrome": - media_client = navidrome_client + media_client = media_server_engine.client('navidrome') if not media_client: _db_update_error_callback(f"Media client for '{server_type}' not available.") @@ -15160,11 +15160,11 @@ def _run_deep_scan_task(server_type): media_client = None if server_type == "plex": - media_client = plex_client + media_client = media_server_engine.client('plex') elif server_type == "jellyfin": - media_client = jellyfin_client + media_client = media_server_engine.client('jellyfin') elif server_type == "navidrome": - media_client = navidrome_client + media_client = media_server_engine.client('navidrome') elif server_type == "soulsync": # SoulSync standalone deep scan: find untracked files → move to Staging, # remove stale DB records where files no longer exist on disk @@ -18037,10 +18037,10 @@ def get_server_playlists(): return jsonify({"success": False, "error": "No media server configured"}), 400 playlists_data = [] - if active_server == 'plex' and plex_client and plex_client.is_connected(): + if active_server == 'plex' and media_server_engine.client('plex') and media_server_engine.client('plex').is_connected(): # Use raw Plex API to get playlist metadata without fetching all tracks try: - raw_playlists = plex_client.server.playlists() + raw_playlists = media_server_engine.client('plex').server.playlists() logger.info(f"[ServerPlaylists] Plex returned {len(raw_playlists)} total playlists") for playlist in raw_playlists: if getattr(playlist, 'playlistType', None) == 'audio': @@ -18053,22 +18053,22 @@ def get_server_playlists(): except Exception as e: logger.error(f"[ServerPlaylists] Error fetching Plex playlists: {e}", exc_info=True) return jsonify({"success": False, "error": f"Plex error: {str(e)}"}), 500 - elif active_server == 'jellyfin' and jellyfin_client and jellyfin_client.is_connected(): - for pl in jellyfin_client.get_all_playlists(): + elif active_server == 'jellyfin' and media_server_engine.client('jellyfin') and media_server_engine.client('jellyfin').is_connected(): + for pl in media_server_engine.client('jellyfin').get_all_playlists(): playlists_data.append({ 'id': pl.id, 'name': pl.title, 'track_count': pl.leaf_count, }) - elif active_server == 'navidrome' and navidrome_client and navidrome_client.is_connected(): - for pl in navidrome_client.get_all_playlists(): + elif active_server == 'navidrome' and media_server_engine.client('navidrome') and media_server_engine.client('navidrome').is_connected(): + for pl in media_server_engine.client('navidrome').get_all_playlists(): playlists_data.append({ 'id': pl.id, 'name': pl.title, 'track_count': pl.leaf_count, }) else: - logger.warning(f"[ServerPlaylists] Server '{active_server}' not connected. plex_client={plex_client is not None}, jellyfin_client={jellyfin_client is not None}, navidrome_client={navidrome_client is not None}") + logger.warning(f"[ServerPlaylists] Server '{active_server}' not connected. plex_client={media_server_engine.client('plex') is not None}, jellyfin_client={media_server_engine.client('jellyfin') is not None}, navidrome_client={media_server_engine.client('navidrome') is not None}") return jsonify({"success": False, "error": f"{active_server} not connected"}), 400 return jsonify({"success": True, "server_type": active_server, "playlists": playlists_data}) @@ -18086,24 +18086,30 @@ def get_server_playlist_tracks(playlist_id): # Get tracks from server server_tracks = [] - if active_server == 'plex' and plex_client: + if active_server == 'plex' and media_server_engine.client('plex'): try: # Try by ID first, fall back to name lookup (ID changes when playlist is recreated) raw_playlist = None try: - raw_playlist = plex_client.server.fetchItem(int(playlist_id)) + raw_playlist = media_server_engine.client('plex').server.fetchItem(int(playlist_id)) except Exception: pass if not raw_playlist and playlist_name: try: - raw_playlist = plex_client.server.playlist(playlist_name) + raw_playlist = media_server_engine.client('plex').server.playlist(playlist_name) except Exception: pass + if not raw_playlist: + logger.warning( + f"[ServerPlaylistTracks] Plex playlist not found by " + f"id={playlist_id} or name='{playlist_name}' — " + f"compare view will show every source track as Find & Add" + ) if raw_playlist: if not playlist_name: playlist_name = raw_playlist.title - plex_base = getattr(plex_client.server, '_baseurl', '') or '' - plex_token = getattr(plex_client.server, '_token', '') or '' + plex_base = getattr(media_server_engine.client('plex').server, '_baseurl', '') or '' + plex_token = getattr(media_server_engine.client('plex').server, '_token', '') or '' if not plex_base: # Fallback: get from config _pc = config_manager.get_plex_config() @@ -18128,9 +18134,9 @@ def get_server_playlist_tracks(playlist_id): }) except Exception as e: logger.error(f"[ServerPlaylistTracks] Plex error: {e}", exc_info=True) - elif active_server == 'jellyfin' and jellyfin_client: - tracks = jellyfin_client.get_playlist_tracks(playlist_id) - jf_base = jellyfin_client.base_url or '' + elif active_server == 'jellyfin' and media_server_engine.client('jellyfin'): + tracks = media_server_engine.client('jellyfin').get_playlist_tracks(playlist_id) + jf_base = media_server_engine.client('jellyfin').base_url or '' for t in (tracks or []): raw = t._data if hasattr(t, '_data') else {} artists = raw.get('Artists', []) @@ -18145,8 +18151,8 @@ def get_server_playlist_tracks(playlist_id): 'duration': t.duration, 'thumb': thumb, }) - elif active_server == 'navidrome' and navidrome_client: - tracks = navidrome_client.get_playlist_tracks(playlist_id) + elif active_server == 'navidrome' and media_server_engine.client('navidrome'): + tracks = media_server_engine.client('navidrome').get_playlist_tracks(playlist_id) for t in (tracks or []): raw = t._data if hasattr(t, '_data') else {} # Navidrome cover art via Subsonic API @@ -18171,9 +18177,9 @@ def get_server_playlist_tracks(playlist_id): # Build server art URL prefix for resolving relative thumb paths _art_prefix = '' _art_suffix = '' - if active_server == 'plex' and plex_client and plex_client.server: - _ab = getattr(plex_client.server, '_baseurl', '') or '' - _at = getattr(plex_client.server, '_token', '') or '' + if active_server == 'plex' and media_server_engine.client('plex') and media_server_engine.client('plex').server: + _ab = getattr(media_server_engine.client('plex').server, '_baseurl', '') or '' + _at = getattr(media_server_engine.client('plex').server, '_token', '') or '' if not _ab: _pc = config_manager.get_plex_config() _ab = (_pc.get('base_url', '') or '').rstrip('/') @@ -18356,11 +18362,22 @@ def server_playlist_replace_track(playlist_id): active_server = config_manager.get_active_media_server() - if active_server == 'plex' and plex_client: - # Use raw Plex API - fetch playlist directly + if active_server == 'plex' and media_server_engine.client('plex'): + # ID-first, name-fallback (Plex deletes + recreates on edit + # so the cached rating key can be stale). + plex_server = media_server_engine.client('plex').server + raw_playlist = None try: - raw_playlist = plex_client.server.playlist(playlist_name) + raw_playlist = plex_server.fetchItem(int(playlist_id)) except Exception: + pass + if not raw_playlist and playlist_name: + try: + raw_playlist = plex_server.playlist(playlist_name) + except Exception: + pass + if not raw_playlist: + logger.warning(f"[ServerPlaylist] replace-track: playlist not found by id={playlist_id} or name='{playlist_name}'") return jsonify({"success": False, "error": "Playlist not found on server"}), 404 # Build new track list with replacement @@ -18368,7 +18385,7 @@ def server_playlist_replace_track(playlist_id): replaced = False for item in raw_playlist.items(): if str(item.ratingKey) == str(old_track_id) and not replaced: - new_item = plex_client.server.fetchItem(int(new_track_id)) + new_item = media_server_engine.client('plex').server.fetchItem(int(new_track_id)) if new_item: new_tracks.append(new_item) replaced = True @@ -18381,13 +18398,13 @@ def server_playlist_replace_track(playlist_id): # Delete old and recreate directly (avoid update_playlist's backup logic) raw_playlist.delete() from plexapi.playlist import Playlist - new_pl = Playlist.create(plex_client.server, playlist_name, items=new_tracks) + new_pl = Playlist.create(media_server_engine.client('plex').server, playlist_name, items=new_tracks) return jsonify({"success": True, "message": "Track replaced", "new_playlist_id": str(new_pl.ratingKey)}) else: return jsonify({"success": False, "error": "Old track not found in playlist"}), 404 - elif active_server == 'jellyfin' and jellyfin_client: - current_tracks = jellyfin_client.get_playlist_tracks(playlist_id) + elif active_server == 'jellyfin' and media_server_engine.client('jellyfin'): + current_tracks = media_server_engine.client('jellyfin').get_playlist_tracks(playlist_id) new_track_ids = [] replaced = False for t in (current_tracks or []): @@ -18400,12 +18417,12 @@ def server_playlist_replace_track(playlist_id): if replaced: new_track_objs = [type('T', (), {'ratingKey': tid, 'title': ''})() for tid in new_track_ids] - jellyfin_client.update_playlist(playlist_name, new_track_objs) + media_server_engine.client('jellyfin').update_playlist(playlist_name, new_track_objs) return jsonify({"success": True, "message": "Track replaced"}) return jsonify({"success": False, "error": "Old track not found"}), 404 - elif active_server == 'navidrome' and navidrome_client: - current_tracks = navidrome_client.get_playlist_tracks(playlist_id) + elif active_server == 'navidrome' and media_server_engine.client('navidrome'): + current_tracks = media_server_engine.client('navidrome').get_playlist_tracks(playlist_id) new_track_ids = [] replaced = False for t in (current_tracks or []): @@ -18418,7 +18435,7 @@ def server_playlist_replace_track(playlist_id): if replaced: new_track_objs = [type('T', (), {'ratingKey': tid, 'title': ''})() for tid in new_track_ids] - navidrome_client.create_playlist(playlist_name, new_track_objs, playlist_id=playlist_id) + media_server_engine.client('navidrome').create_playlist(playlist_name, new_track_objs, playlist_id=playlist_id) return jsonify({"success": True, "message": "Track replaced"}) return jsonify({"success": False, "error": "Old track not found"}), 404 @@ -18444,12 +18461,26 @@ def server_playlist_add_track(playlist_id): active_server = config_manager.get_active_media_server() - if active_server == 'plex' and plex_client: + if active_server == 'plex' and media_server_engine.client('plex'): + # ID-first, name-fallback — Plex deletes + recreates playlists + # on edit so the rating key the frontend cached can be stale. + # The GET tracks endpoint uses the same lookup chain. + plex_server = media_server_engine.client('plex').server + raw_playlist = None try: - raw_playlist = plex_client.server.playlist(playlist_name) + raw_playlist = plex_server.fetchItem(int(playlist_id)) except Exception: + pass + if not raw_playlist and playlist_name: + try: + raw_playlist = plex_server.playlist(playlist_name) + except Exception: + pass + if not raw_playlist: + logger.warning(f"[ServerPlaylist] add-track: playlist not found by id={playlist_id} or name='{playlist_name}'") return jsonify({"success": False, "error": "Playlist not found"}), 404 - new_item = plex_client.server.fetchItem(int(track_id)) + + new_item = plex_server.fetchItem(int(track_id)) if not new_item: return jsonify({"success": False, "error": "Track not found on server"}), 404 @@ -18476,22 +18507,22 @@ def server_playlist_add_track(playlist_id): logger.info(f"[ServerPlaylist] Added track to playlist, playlist ID: {new_id}") return jsonify({"success": True, "message": "Track added", "new_playlist_id": new_id}) - elif active_server == 'jellyfin' and jellyfin_client: - current_tracks = jellyfin_client.get_playlist_tracks(playlist_id) or [] + elif active_server == 'jellyfin' and media_server_engine.client('jellyfin'): + current_tracks = media_server_engine.client('jellyfin').get_playlist_tracks(playlist_id) or [] track_ids = [str(t.ratingKey) for t in current_tracks] pos = max(0, min(int(position), len(track_ids))) if position is not None else len(track_ids) track_ids.insert(pos, track_id) new_track_objs = [type('T', (), {'ratingKey': tid, 'title': ''})() for tid in track_ids] - jellyfin_client.update_playlist(playlist_name, new_track_objs) + media_server_engine.client('jellyfin').update_playlist(playlist_name, new_track_objs) return jsonify({"success": True, "message": "Track added"}) - elif active_server == 'navidrome' and navidrome_client: - current_tracks = navidrome_client.get_playlist_tracks(playlist_id) or [] + elif active_server == 'navidrome' and media_server_engine.client('navidrome'): + current_tracks = media_server_engine.client('navidrome').get_playlist_tracks(playlist_id) or [] track_ids = [str(t.ratingKey) for t in current_tracks] pos = max(0, min(int(position), len(track_ids))) if position is not None else len(track_ids) track_ids.insert(pos, track_id) new_track_objs = [type('T', (), {'ratingKey': tid, 'title': ''})() for tid in track_ids] - navidrome_client.create_playlist(playlist_name, new_track_objs, playlist_id=playlist_id) + media_server_engine.client('navidrome').create_playlist(playlist_name, new_track_objs, playlist_id=playlist_id) return jsonify({"success": True, "message": "Track added"}) return jsonify({"success": False, "error": f"Unsupported server: {active_server}"}), 400 @@ -18515,10 +18546,22 @@ def server_playlist_remove_track(playlist_id): active_server = config_manager.get_active_media_server() - if active_server == 'plex' and plex_client: + if active_server == 'plex' and media_server_engine.client('plex'): + # ID-first, name-fallback (Plex deletes + recreates on edit + # so the cached rating key can be stale). + plex_server = media_server_engine.client('plex').server + raw_playlist = None try: - raw_playlist = plex_client.server.playlist(playlist_name) + raw_playlist = plex_server.fetchItem(int(playlist_id)) except Exception: + pass + if not raw_playlist and playlist_name: + try: + raw_playlist = plex_server.playlist(playlist_name) + except Exception: + pass + if not raw_playlist: + logger.warning(f"[ServerPlaylist] remove-track: playlist not found by id={playlist_id} or name='{playlist_name}'") return jsonify({"success": False, "error": "Playlist not found"}), 404 # Rebuild without the target track @@ -18529,26 +18572,26 @@ def server_playlist_remove_track(playlist_id): raw_playlist.delete() if new_items: from plexapi.playlist import Playlist - new_pl = Playlist.create(plex_client.server, playlist_name, items=new_items) + new_pl = Playlist.create(media_server_engine.client('plex').server, playlist_name, items=new_items) return jsonify({"success": True, "message": "Track removed", "new_playlist_id": str(new_pl.ratingKey)}) return jsonify({"success": True, "message": "Track removed (playlist now empty)"}) - elif active_server == 'jellyfin' and jellyfin_client: - current_tracks = jellyfin_client.get_playlist_tracks(playlist_id) or [] + elif active_server == 'jellyfin' and media_server_engine.client('jellyfin'): + current_tracks = media_server_engine.client('jellyfin').get_playlist_tracks(playlist_id) or [] new_ids = [str(t.ratingKey) for t in current_tracks if str(t.ratingKey) != str(remove_track_id)] if len(new_ids) == len(current_tracks): return jsonify({"success": False, "error": "Track not found in playlist"}), 404 new_track_objs = [type('T', (), {'ratingKey': tid, 'title': ''})() for tid in new_ids] - jellyfin_client.update_playlist(playlist_name, new_track_objs) + media_server_engine.client('jellyfin').update_playlist(playlist_name, new_track_objs) return jsonify({"success": True, "message": "Track removed"}) - elif active_server == 'navidrome' and navidrome_client: - current_tracks = navidrome_client.get_playlist_tracks(playlist_id) or [] + elif active_server == 'navidrome' and media_server_engine.client('navidrome'): + current_tracks = media_server_engine.client('navidrome').get_playlist_tracks(playlist_id) or [] new_ids = [str(t.ratingKey) for t in current_tracks if str(t.ratingKey) != str(remove_track_id)] if len(new_ids) == len(current_tracks): return jsonify({"success": False, "error": "Track not found in playlist"}), 404 new_track_objs = [type('T', (), {'ratingKey': tid, 'title': ''})() for tid in new_ids] - navidrome_client.create_playlist(playlist_name, new_track_objs, playlist_id=playlist_id) + media_server_engine.client('navidrome').create_playlist(playlist_name, new_track_objs, playlist_id=playlist_id) return jsonify({"success": True, "message": "Track removed"}) return jsonify({"success": False, "error": f"Unsupported server: {active_server}"}), 400 @@ -18572,9 +18615,9 @@ def library_search_tracks(): # Build thumb URL resolver for this server _art_prefix = '' _art_suffix = '' - if active_server == 'plex' and plex_client and plex_client.server: - _ab = getattr(plex_client.server, '_baseurl', '') or '' - _at = getattr(plex_client.server, '_token', '') or '' + if active_server == 'plex' and media_server_engine.client('plex') and media_server_engine.client('plex').server: + _ab = getattr(media_server_engine.client('plex').server, '_baseurl', '') or '' + _at = getattr(media_server_engine.client('plex').server, '_token', '') or '' if not _ab: _pc = config_manager.get_plex_config() _ab = (_pc.get('base_url', '') or '').rstrip('/') @@ -23251,8 +23294,7 @@ def _build_sync_deps(): return _discovery_sync.SyncDeps( config_manager=config_manager, sync_service=sync_service, - plex_client=plex_client, - jellyfin_client=jellyfin_client, + media_server_engine=media_server_engine, automation_engine=automation_engine, run_async=run_async, record_sync_history_start=_record_sync_history_start, @@ -23384,12 +23426,12 @@ def test_database_access(): # Test media clients logger.info(" Media clients status:") - logger.info(f" plex_client: {plex_client is not None}") - if plex_client: - logger.info(f" plex_client.is_connected(): {plex_client.is_connected()}") - logger.info(f" jellyfin_client: {jellyfin_client is not None}") - if jellyfin_client: - logger.info(f" jellyfin_client.is_connected(): {jellyfin_client.is_connected()}") + logger.info(f" media_server_engine.client('plex'): {media_server_engine.client('plex') is not None}") + if media_server_engine.client('plex'): + logger.info(f" media_server_engine.client('plex').is_connected(): {media_server_engine.client('plex').is_connected()}") + logger.info(f" media_server_engine.client('jellyfin'): {media_server_engine.client('jellyfin') is not None}") + if media_server_engine.client('jellyfin'): + logger.info(f" media_server_engine.client('jellyfin').is_connected(): {media_server_engine.client('jellyfin').is_connected()}") return jsonify({ "success": True, @@ -23398,8 +23440,8 @@ def test_database_access(): "database_initialized": db is not None, "database_stats": stats, "active_server": active_server, - "plex_connected": plex_client.is_connected() if plex_client else False, - "jellyfin_connected": jellyfin_client.is_connected() if jellyfin_client else False, + "plex_connected": media_server_engine.client('plex').is_connected() if media_server_engine.client('plex') else False, + "jellyfin_connected": media_server_engine.client('jellyfin').is_connected() if media_server_engine.client('jellyfin') else False, } }) @@ -29118,17 +29160,17 @@ def start_metadata_update(): # Get appropriate media client - Support all three servers if active_server == "jellyfin": - media_client = jellyfin_client + media_client = media_server_engine.client('jellyfin') if not media_client: add_activity_item("", "Metadata Update", "Jellyfin client not available", "Now") return jsonify({"success": False, "error": "Jellyfin client not available"}), 400 elif active_server == "navidrome": - media_client = navidrome_client + media_client = media_server_engine.client('navidrome') if not media_client: add_activity_item("", "Metadata Update", "Navidrome client not available", "Now") return jsonify({"success": False, "error": "Navidrome client not available"}), 400 else: # plex - media_client = plex_client + media_client = media_server_engine.client('plex') if not media_client: add_activity_item("", "Metadata Update", "Plex client not available", "Now") return jsonify({"success": False, "error": "Plex client not available"}), 400 @@ -32833,9 +32875,7 @@ try: listening_stats_worker = ListeningStatsWorker( database=listening_stats_db, config_manager=config_manager, - plex_client=plex_client, - jellyfin_client=jellyfin_client, - navidrome_client=navidrome_client, + media_server_engine=media_server_engine, ) listening_stats_worker.start() logger.info("Listening stats worker initialized and started") diff --git a/webui/static/helper.js b/webui/static/helper.js index ccc46139..607ba6c8 100644 --- a/webui/static/helper.js +++ b/webui/static/helper.js @@ -3432,6 +3432,8 @@ const WHATS_NEW = { '2.4.2': [ // --- post-2.4.1 dev work — entries hidden by _getLatestWhatsNewVersion until the build version bumps --- { date: 'Unreleased — 2.4.2 dev cycle' }, + { title: 'Internal: Media Server Engine Cin/JohnBaumb Pass', desc: 'internal — applied the same architectural cleanups the download engine PR went through to the media server engine PR before review. (1) every server client (Plex / Jellyfin / Navidrome / SoulSync) now explicitly inherits `MediaServerClient` instead of relying on structural typing — drift in any class fails at the conformance test boundary. (2) generic accessors on the engine: `configured_clients()` (replaces per-server `if X and X.is_connected(): clients[name] = X` chains in web_server.py) and `reload_config(name=None)` (generic dispatch instead of per-client reload calls). (3) singleton factory: `get_media_server_engine()` / `set_media_server_engine()` matching the metadata + download engine shape. web_server.py boots via `set_media_server_engine(...)` so factory + global handle share state. (4) ~70 direct `plex_client.X` / `jellyfin_client.X` / `navidrome_client.X` / `soulsync_library_client.X` attribute reaches in web_server.py migrated to `media_server_engine.client(\'\').X`. ~60 standalone refs (truthy checks, media_client assignments, source-name tuples) also routed through the engine. (5) the per-server `plex_client` / `jellyfin_client` / `navidrome_client` / `soulsync_library_client` globals in web_server.py are gone entirely — engine owns the client instances now, every caller reaches via `media_server_engine.client(\'\')`. four multi-client consumers (`PlaylistSyncService`, `ListeningStatsWorker`, `WebScanManager`, discovery `SyncDeps`) refactored to take the engine instead of separate per-server kwargs. (6) `TrackInfo` and `PlaylistInfo` lifted out of `core/plex_client.py` / `jellyfin_client.py` / `navidrome_client.py` (each was defining a near-identical copy) into the neutral `core/media_server/types.py` module — same lift Cin caught on the download `TrackResult`/`AlbumResult`/`DownloadStatus` situation. consumers (matching engine, sync service) get one import. zero behavior change.' }, + { title: 'Internal: Media Server Engine Foundation', desc: 'internal — companion to the download engine refactor. introduces a media-server engine + plugin contract on top of the four server clients (plex / jellyfin / navidrome / soulsync standalone). pre-refactor web_server.py held four separate per-server client globals that every dispatch site reached individually. new `core/media_server/` package provides `MediaServerEngine` that owns the per-server clients + a small set of generic accessors (`client(name)`, `active_client()`, `configured_clients()`, `reload_config(name)`) so call sites use one canonical lookup pattern. plugin contract requires the four methods every server actually implements (is_connected, ensure_connection, get_all_artists, get_all_album_ids) — methods that exist on most-but-not-all servers (search_tracks, trigger_library_scan, get_library_stats, etc.) are listed in `KNOWN_PER_SERVER_METHODS` for discoverability and reached directly via `engine.client(name).` since there\'s no uniform safe-default that fits every method. honest scope: the four uniform-shape `is_connected` dispatches were lifted into `engine.is_connected()` (the one cross-server wrapper kept on the engine); the ~18 server-specific chains that do genuinely different per-server work (playlist track replace, metadata sync, scan strategies) stay explicit at the call site per the "lift what\'s truly shared" standard, but reach the per-server client through the engine. 42 tests pin: per-server observable behavior (4 server pinning files, 20 tests), engine surface + accessor contracts (15 tests), structural conformance + explicit-inheritance (9 tests). zero behavior change for users.' }, { title: 'Drop SoundCloud Preview Snippets at Search Time', desc: 'soundcloud serves a ~30s preview clip for tracks that are gated behind go+ / login (very common for major-label uploads — official content basically doesn\'t exist on soundcloud, so what shows up is bootlegs, fan uploads, type beats, and these 30s previews). yt-dlp accepts the preview as the download payload, the post-download integrity check catches the duration mismatch and quarantines the file, but the user just sees "all candidates failed" with no explanation. previews also showed up in the candidate-review modal where clicking one bypassed validation and downloaded the same broken file. now `filter_soundcloud_previews` drops these candidates at every entry point — auto-search scoring, modal-cache fallback, AND the not-found raw-results path — so previews never reach the matcher OR the user. drops candidates < 35s or below half the expected duration, gated on expected being non-trivially long (>60s) so genuine short tracks still pass. also fixed a silent regression in the hybrid-fallback path where the per-source attribute removal left `getattr(orch, \'youtube\', None)` returning None for every source — fallback never fired. now resolves through the orchestrator\'s `client(name)` accessor.', page: 'downloads' }, { title: 'Internal: Move Shared Download Dataclasses + Singleton Boot Path', desc: 'internal — two architectural cleanups on top of the download engine refactor. (1) `TrackResult`, `AlbumResult`, `DownloadStatus`, `SearchResult` lived in `core/soulseek_client.py` for historical reasons (they grew up there as the soulseek-only types and got exported when other download sources were added). every plugin imported these from the soulseek module just to satisfy the contract — coupling 8 clients to a sibling source for type imports only. moved them to `core/download_plugins/types.py` (the neutral plugin package) and updated all 14 import sites across deezer/hifi/lidarr/qobuz/soundcloud/tidal/youtube clients + the engine + matching engine + redownload + tests. clean break, no backward-compat re-export. (2) `web_server.py` now boots the orchestrator via `set_download_orchestrator(DownloadOrchestrator())` so the singleton factory + boot path share state — `get_download_orchestrator()` returns the same instance the global handle points at instead of lazily building a separate one. matches cin\'s `get_metadata_engine()` pattern.' }, { title: 'Internal: Rename `soulseek_client` Global → `download_orchestrator`', desc: 'internal — followup cleanup. the global handle in web_server.py was named `soulseek_client` for historical reasons (the orchestrator was originally just the soulseek client and grew downstream sources around it), but the type has long been `DownloadOrchestrator` not `SoulseekClient`. renamed the global + every parameter/attribute that carried the legacy name across web_server.py, api/, core/downloads/*, core/search/*, core/streaming/*, services/sync_service.py, and the test fixtures (`MasterDeps.soulseek_client` → `download_orchestrator`, `init(soulseek_client_obj)` → `init(download_orchestrator_obj)`, etc). module path `core.soulseek_client` and class `SoulseekClient` (the actual soulseek-only client) are unchanged — only the orchestrator handle renamed. ~250 references touched, suite green.' },