From f702196dca57bfbd5384e77f931ffbdcd8016dec Mon Sep 17 00:00:00 2001 From: Broque Thomas <26755000+Nezreka@users.noreply.github.com> Date: Mon, 4 May 2026 20:49:25 -0700 Subject: [PATCH 01/23] Phase 0: Add MediaServerClient contract + registry MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `core/media_server/` package with the Protocol contract that every media server client (Plex, Jellyfin, Navidrome, SoulSync standalone) satisfies, plus the registry that holds them. Required methods conservatively limited to the four every server truly implements today: is_connected, ensure_connection, get_all_artists, get_all_album_ids. Other generic methods (search_tracks, trigger_library_scan, get_recently_added_albums, etc.) are listed as OPTIONAL — present on most servers but not all (SoulSync has no library-scan API since it walks the filesystem directly; Jellyfin uses a different search shape). Phase B's engine adapters route around the gaps with per-server fallback instead of forcing every client to declare a no-op stub. Same registry shape as the download plugin registry — single source of truth for which servers exist + name resolution. Adding a 5th server (Subsonic, Emby, etc.) becomes one register call plus the new client class. 5 conformance tests pin every server class implements every required method. Plan doc at docs/media-server-engine-refactor-plan.md. Pure additive — no consumer routes through the contract or registry yet. Suite still green (1921 passed). --- core/media_server/__init__.py | 23 ++++ core/media_server/contract.py | 148 ++++++++++++++++++++ core/media_server/registry.py | 127 +++++++++++++++++ docs/media-server-engine-refactor-plan.md | 159 ++++++++++++++++++++++ tests/media_server/__init__.py | 0 tests/media_server/test_conformance.py | 54 ++++++++ 6 files changed, 511 insertions(+) create mode 100644 core/media_server/__init__.py create mode 100644 core/media_server/contract.py create mode 100644 core/media_server/registry.py create mode 100644 docs/media-server-engine-refactor-plan.md create mode 100644 tests/media_server/__init__.py create mode 100644 tests/media_server/test_conformance.py diff --git a/core/media_server/__init__.py b/core/media_server/__init__.py new file mode 100644 index 00000000..422fba04 --- /dev/null +++ b/core/media_server/__init__.py @@ -0,0 +1,23 @@ +"""Media server engine — central dispatch for 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. The orchestrator +historically had 33+ ``if active_server == 'plex' / 'jellyfin' / +...`` dispatch sites in web_server.py. This package replaces those +with a single ``MediaServerEngine.method()`` call per operation. + +Per-server clients keep their protocol-specific work (Plex's +PlexAPI SDK, Jellyfin's REST endpoints, Navidrome's OpenSubsonic +API, SoulSync's filesystem walk). The engine just routes by +``active_server`` config + provides a uniform shape for the calls +``web_server.py`` makes generically. + +See ``docs/media-server-engine-refactor-plan.md`` for the full +phased plan. +""" + +from core.media_server.contract import MediaServerClient +from core.media_server.registry import MediaServerRegistry, build_default_registry + +__all__ = ["MediaServerClient", "MediaServerRegistry", "build_default_registry"] diff --git a/core/media_server/contract.py b/core/media_server/contract.py new file mode 100644 index 00000000..986b4108 --- /dev/null +++ b/core/media_server/contract.py @@ -0,0 +1,148 @@ +"""Canonical contract for media server clients. + +Narrow on purpose. Only declares the methods the orchestrator +dispatches generically across all servers. Server-specific extras +(Plex's `set_music_library_by_name`, Jellyfin's user picker, +Navidrome's music folder filter, SoulSync's filesystem rescan) +stay on the underlying client and are accessed through the +registry's typed accessor — same pattern as the download +plugin contract. + +Every required method must be implemented by every registered +client. Optional methods have default no-op implementations so +servers without that capability (e.g. Navidrome's metadata +writeback stubs, SoulSync's playlist sync N/A) don't have to +declare a no-op explicitly. + +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 calls. This file just makes the implicit +contract explicit. +""" + +from __future__ import annotations + +from typing import Any, Dict, List, Optional, Protocol, runtime_checkable + + +@runtime_checkable +class MediaServerClient(Protocol): + """Structural contract every media server client must satisfy. + + ``runtime_checkable`` lets ``isinstance(client, MediaServerClient)`` + work for the conformance test, but it ONLY checks method names — + not signatures. The conformance test in + ``tests/media_server/test_conformance.py`` does the deeper + signature check. + """ + + # ------------------------------------------------------------------ + # Connection / lifecycle + # ------------------------------------------------------------------ + + 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 must support these) + # ------------------------------------------------------------------ + + 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.""" + ... + + def search_tracks(self, title: str, artist: str, limit: int = 15) -> List[Any]: + """Search the server's library for tracks matching the title + + artist. Used by playlist sync, download matching, and the + general search UI. Each item is a server-specific TrackInfo + wrapper.""" + ... + + def get_recently_added_albums(self, max_results: int = 400) -> List[Any]: + """Recently-added view — used by the Discover page + + watchlist scanner. Sorted by add timestamp descending.""" + ... + + # ------------------------------------------------------------------ + # Library writes — scan triggers + # ------------------------------------------------------------------ + + def trigger_library_scan(self) -> bool: + """Ask the server to scan its music library. Some servers + (SoulSync standalone) walk the filesystem themselves; some + (Plex / Jellyfin / Navidrome) hit a server-side scan API.""" + ... + + def is_library_scanning(self) -> bool: + """True if a scan is currently running. Polled by the + scan-progress UI.""" + ... + + def get_library_stats(self) -> Dict[str, int]: + """Counts of artists / albums / tracks. Used by the + dashboard system-stats card.""" + ... + + +# --------------------------------------------------------------------------- +# Required + optional method names — used by the conformance tests to +# check structural conformance without the proxy weight of dataclasses. +# --------------------------------------------------------------------------- + +# Conservative requirement set — only methods every one of the four +# servers actually implements today. Audited by the conformance test. +# Other methods (search_tracks, trigger_library_scan, etc.) exist on +# most servers but not all (e.g. SoulSync has no library scan API +# because it walks the filesystem directly). Phase B's engine +# adapters handle those with per-server fallback rather than forcing +# every client to declare a no-op stub. +REQUIRED_METHODS = { + 'is_connected', + 'ensure_connection', + 'get_all_artists', + 'get_all_album_ids', +} + +# Methods declared on the protocol but NOT enforced — the +# conformance test does NOT fail if a client lacks one. Engine +# adapters route around the gaps. Listed here so future contributors +# know what the engine expects to find when present. +OPTIONAL_METHODS = { + 'search_tracks', + 'get_recently_added_albums', + 'trigger_library_scan', + 'is_library_scanning', + 'get_library_stats', + # Playlist sync (Plex / Jellyfin / Navidrome implement; SoulSync no-op) + 'create_playlist', + 'update_playlist', + 'copy_playlist', + 'get_all_playlists', + 'get_playlist_by_name', + # Analytics + 'get_play_history', + 'get_track_play_counts', + # Metadata writeback (Plex full support; Jellyfin partial; Navidrome stubs; SoulSync N/A) + 'update_artist_genres', + 'update_artist_poster', + 'update_album_poster', + 'update_artist_biography', + 'update_track_metadata', +} diff --git a/core/media_server/registry.py b/core/media_server/registry.py new file mode 100644 index 00000000..173c7a72 --- /dev/null +++ b/core/media_server/registry.py @@ -0,0 +1,127 @@ +"""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 ++ 33 hand-maintained ``if active_server == 'plex' / 'jellyfin' / ...`` +dispatch sites. + +Adding a new server (e.g. Subsonic, Emby) becomes one ``register`` +call here + the new client class. Web server dispatch stays put. +""" + +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 + + @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/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/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..fab74111 --- /dev/null +++ b/tests/media_server/test_conformance.py @@ -0,0 +1,54 @@ +"""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 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}" + ) From c1da56b2c2cbd7e79d2318e5134b13cadf5a853c Mon Sep 17 00:00:00 2001 From: Broque Thomas <26755000+Nezreka@users.noreply.github.com> Date: Mon, 4 May 2026 20:53:00 -0700 Subject: [PATCH 02/23] A1: Pin PlexClient observable behavior 6 tests pin the Plex client surface the engine will dispatch through after Phase B/C migrations: - is_connected returns False on no-server, True on server-present - is_fully_configured requires BOTH server AND music_library - get_all_artists empty list on not-connected, iterates music_library.searchArtists() when connected - get_all_album_ids returns a set of STRING ratingKey values (coerced from Plex ints so semantics match Jellyfin GUIDs + Navidrome string ids) Phase A pinning catches behavior drift during web_server.py dispatch-site migrations (Phase C) and engine adapter wiring (Phase B). --- tests/media_server/test_plex_pinning.py | 113 ++++++++++++++++++++++++ 1 file changed, 113 insertions(+) create mode 100644 tests/media_server/test_plex_pinning.py 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'} From 5da2cfec462ac51daba499ccc12447fa5779a33a Mon Sep 17 00:00:00 2001 From: Broque Thomas <26755000+Nezreka@users.noreply.github.com> Date: Mon, 4 May 2026 20:53:55 -0700 Subject: [PATCH 03/23] A2: Pin JellyfinClient observable behavior 5 tests pin Jellyfin client surface. is_connected requires ALL four of base_url + api_key + user_id + music_library_id (stricter than Plex's is_connected). get_all_album_ids returns a set of string GUID ids matching the cross-server uniform set semantics. --- tests/media_server/test_jellyfin_pinning.py | 104 ++++++++++++++++++++ 1 file changed, 104 insertions(+) create mode 100644 tests/media_server/test_jellyfin_pinning.py 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'} From edcdaaa99368696021eadd46c9c5d0f7d4235c02 Mon Sep 17 00:00:00 2001 From: Broque Thomas <26755000+Nezreka@users.noreply.github.com> Date: Mon, 4 May 2026 21:01:11 -0700 Subject: [PATCH 04/23] A3: Pin NavidromeClient observable behavior MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 4 tests pin the Navidrome client surface. Auth shape: base_url + username + password (no token model — salt generated per request). get_all_album_ids paginates getAlbumList2 and returns a set of string ids matching cross-server uniform set semantics. --- tests/media_server/test_navidrome_pinning.py | 79 ++++++++++++++++++++ 1 file changed, 79 insertions(+) create mode 100644 tests/media_server/test_navidrome_pinning.py 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'} From 50fe4bec97757646884b0459547c190849706dfd Mon Sep 17 00:00:00 2001 From: Broque Thomas <26755000+Nezreka@users.noreply.github.com> Date: Mon, 4 May 2026 21:03:37 -0700 Subject: [PATCH 05/23] A4: Pin SoulSyncClient observable behavior MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 5 tests pin the SoulSync standalone client surface — the structurally-different one (no auth, no API, no library scan). is_connected just checks os.path.isdir(transfer_path). ensure_connection reloads config first so the user changing the transfer_path takes effect without a process restart. get_all_album_ids returns a set of MD5-hashed string ids matching cross-server uniform set semantics. --- tests/media_server/test_soulsync_pinning.py | 90 +++++++++++++++++++++ 1 file changed, 90 insertions(+) create mode 100644 tests/media_server/test_soulsync_pinning.py 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'} From 6b54ca6598a847337cf9a46df8c5ee1cbe804ae2 Mon Sep 17 00:00:00 2001 From: Broque Thomas <26755000+Nezreka@users.noreply.github.com> Date: Mon, 4 May 2026 21:14:17 -0700 Subject: [PATCH 06/23] Phase B: Add MediaServerEngine skeleton MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `MediaServerEngine` reads the active server from config + dispatches to the corresponding registered client. Per-server reaches still work through `engine.client(name)`. Required-method dispatch (is_connected, ensure_connection, get_all_artists, get_all_album_ids) returns safe defaults when the active client failed to initialize OR when the method raises. Optional-method dispatch (search_tracks, trigger_library_scan, is_library_scanning, get_library_stats, get_recently_added_albums) checks hasattr first — SoulSync standalone has no trigger_library_scan or get_library_stats, engine no-ops with appropriate defaults instead of forcing every client to declare stub methods. 10 new engine tests pin: active-server resolution, required dispatch routing, exception safety, missing-optional-method fallback shape. Suite still green (1951 passed). Engine isn't on any production code path yet — Phase C migrates the 33 web_server.py dispatch sites to call engine.method() instead of hand-branching by active_server name. --- core/media_server/__init__.py | 8 +- core/media_server/engine.py | 215 ++++++++++++++++++++++++++++++ tests/media_server/test_engine.py | 192 ++++++++++++++++++++++++++ 3 files changed, 414 insertions(+), 1 deletion(-) create mode 100644 core/media_server/engine.py create mode 100644 tests/media_server/test_engine.py diff --git a/core/media_server/__init__.py b/core/media_server/__init__.py index 422fba04..c1f5012b 100644 --- a/core/media_server/__init__.py +++ b/core/media_server/__init__.py @@ -18,6 +18,12 @@ phased plan. """ from core.media_server.contract import MediaServerClient +from core.media_server.engine import MediaServerEngine from core.media_server.registry import MediaServerRegistry, build_default_registry -__all__ = ["MediaServerClient", "MediaServerRegistry", "build_default_registry"] +__all__ = [ + "MediaServerClient", + "MediaServerEngine", + "MediaServerRegistry", + "build_default_registry", +] diff --git a/core/media_server/engine.py b/core/media_server/engine.py new file mode 100644 index 00000000..55415123 --- /dev/null +++ b/core/media_server/engine.py @@ -0,0 +1,215 @@ +"""MediaServerEngine — central dispatch for media server operations. + +Replaces the historic 33+ ``if active_server == 'plex' / 'jellyfin' / +'navidrome' / 'soulsync'`` chains in ``web_server.py``. Each +operation web_server.py used to dispatch by hand becomes a single +``engine.method()`` call here that: + +1. Reads the ``server.active`` config to find the current target. +2. Looks up the registered client. +3. Calls the corresponding method (with safe per-server fallbacks + for methods that don't exist on every client — e.g. SoulSync + has no library-scan API). + +Per-server client objects stay accessible via ``engine.client(name)`` +so any caller that needs a Plex-specific method (e.g. +``set_music_library_by_name`` for the settings page) keeps working +through ``engine.client('plex').set_music_library_by_name(...)``. + +Engine itself is constructed once during web_server.py init and +held as a module-level singleton, mirroring the existing pattern +for the per-server client globals. +""" + +from __future__ import annotations + +from typing import Any, Dict, List, 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: + """Single entry point for cross-server library operations. + + The engine knows which server is "active" via the + ``server.active`` config + falls back to direct dispatch for + server-specific calls via ``engine.client(name)``. + """ + + def __init__( + self, + registry: Optional[MediaServerRegistry] = None, + active_server_resolver=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. + """ + self.registry = registry if registry is not None else build_default_registry() + 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 + + # ------------------------------------------------------------------ + # Direct client access (backward-compat for source-specific reaches) + # ------------------------------------------------------------------ + + 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) + + # ------------------------------------------------------------------ + # Cross-server dispatch — required methods (always present) + # ------------------------------------------------------------------ + + def is_connected(self) -> bool: + """Active server's connection state. False if no active + client (registered but failed to initialize).""" + client = self.active_client() + if client is None: + return False + try: + return client.is_connected() + except Exception as exc: + logger.debug("%s is_connected raised: %s", self.active_server, exc) + return False + + def ensure_connection(self) -> bool: + """Re-auth or reconnect the active server. Returns True if + usable after the call.""" + client = self.active_client() + if client is None: + return False + try: + return client.ensure_connection() + except Exception as exc: + logger.debug("%s ensure_connection raised: %s", self.active_server, exc) + return False + + def get_all_artists(self) -> List[Any]: + """Active server's full artist list. Empty list if not + connected or call fails.""" + client = self.active_client() + if client is None: + return [] + try: + return client.get_all_artists() + except Exception as exc: + logger.debug("%s get_all_artists raised: %s", self.active_server, exc) + return [] + + def get_all_album_ids(self) -> set: + """Active server's album-ID set. Empty set if not connected + or call fails.""" + client = self.active_client() + if client is None: + return set() + try: + return client.get_all_album_ids() + except Exception as exc: + logger.debug("%s get_all_album_ids raised: %s", self.active_server, exc) + return set() + + # ------------------------------------------------------------------ + # Optional methods — engine routes if the client implements them, + # returns a safe default otherwise (mirrors the legacy web_server.py + # branches that special-cased SoulSync / Navidrome). + # ------------------------------------------------------------------ + + def search_tracks(self, title: str, artist: str, limit: int = 15) -> List[Any]: + """Search the active server's library. Returns empty list + for servers that don't implement search_tracks (SoulSync + standalone reads filesystem; no live search API).""" + client = self.active_client() + if client is None or not hasattr(client, 'search_tracks'): + return [] + try: + return client.search_tracks(title, artist, limit) + except Exception as exc: + logger.debug("%s search_tracks raised: %s", self.active_server, exc) + return [] + + def trigger_library_scan(self) -> bool: + """Trigger a server-side library scan. No-op (returns True) + for SoulSync standalone — filesystem walks happen in-process.""" + client = self.active_client() + if client is None: + return False + if not hasattr(client, 'trigger_library_scan'): + return True + try: + return client.trigger_library_scan() + except Exception as exc: + logger.debug("%s trigger_library_scan raised: %s", self.active_server, exc) + return False + + def is_library_scanning(self) -> bool: + """True if the active server is currently scanning. Always + False for SoulSync standalone.""" + client = self.active_client() + if client is None or not hasattr(client, 'is_library_scanning'): + return False + try: + return client.is_library_scanning() + except Exception as exc: + logger.debug("%s is_library_scanning raised: %s", self.active_server, exc) + return False + + def get_library_stats(self) -> Dict[str, int]: + """Counts of artists / albums / tracks. Default empty dict + if the server doesn't implement (SoulSync standalone).""" + client = self.active_client() + if client is None or not hasattr(client, 'get_library_stats'): + return {} + try: + return client.get_library_stats() + except Exception as exc: + logger.debug("%s get_library_stats raised: %s", self.active_server, exc) + return {} + + def get_recently_added_albums(self, max_results: int = 400) -> List[Any]: + """Recently-added albums view. Plex uses a different name; + engine routes to whichever method the active server has.""" + client = self.active_client() + if client is None: + return [] + # Plex uses recentlyAdded() on the music library object, not + # a top-level method. SoulSync, Jellyfin, Navidrome all + # expose get_recently_added_albums directly. + if hasattr(client, 'get_recently_added_albums'): + try: + return client.get_recently_added_albums(max_results) + except Exception as exc: + logger.debug( + "%s get_recently_added_albums raised: %s", + self.active_server, exc, + ) + return [] + return [] diff --git a/tests/media_server/test_engine.py b/tests/media_server/test_engine.py new file mode 100644 index 00000000..f4650a28 --- /dev/null +++ b/tests/media_server/test_engine.py @@ -0,0 +1,192 @@ +"""Tests for MediaServerEngine cross-server dispatch.""" + +from __future__ import annotations + +from unittest.mock import MagicMock + +import pytest + +from core.media_server import MediaServerEngine, MediaServerRegistry +from core.media_server.registry import 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_get_all_album_ids_returns_active_clients_set(make_engine): + plex = _FakeClient('plex') + plex._album_ids = {'p-1', 'p-2'} + jelly = _FakeClient('jellyfin') + jelly._album_ids = {'j-1'} + engine = make_engine({'plex': plex, 'jellyfin': jelly}, active='plex') + assert engine.get_all_album_ids() == {'p-1', 'p-2'} + engine = make_engine({'plex': plex, 'jellyfin': jelly}, active='jellyfin') + assert engine.get_all_album_ids() == {'j-1'} + + +def test_engine_returns_safe_defaults_when_active_client_failed_to_init(make_engine): + """When the active client failed to initialize (registry stored + None), the engine returns safe defaults 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.get_all_artists() == [] + assert engine.get_all_album_ids() == set() + assert engine.search_tracks('t', 'a') == [] + assert engine.trigger_library_scan() is False + assert engine.is_library_scanning() is False + + +def test_engine_swallows_per_method_exceptions(make_engine): + """A method that raises must NOT propagate to the dispatch + site — engine returns the safe default instead, mirroring the + legacy web_server.py defensive try/except chains.""" + plex = _FakeClient('plex') + plex.is_connected = MagicMock(side_effect=RuntimeError("boom")) + plex.get_all_album_ids = MagicMock(side_effect=RuntimeError("boom")) + engine = make_engine({'plex': plex}, active='plex') + + assert engine.is_connected() is False + assert engine.get_all_album_ids() == set() + + +# --------------------------------------------------------------------------- +# Optional-method dispatch (engine returns safe default when missing) +# --------------------------------------------------------------------------- + + +class _MinimalClient: + """Stand-in for SoulSync standalone — only the required methods, + NO optional methods. Used to assert engine routes around missing + optional methods with safe defaults.""" + + def is_connected(self): return True + def ensure_connection(self): return True + def get_all_artists(self): return [] + def get_all_album_ids(self): return set() + + +def test_search_tracks_returns_empty_when_client_lacks_method(make_engine): + """SoulSync standalone has no search_tracks — engine returns + [] instead of raising AttributeError.""" + engine = make_engine({'soulsync': _MinimalClient()}, active='soulsync') + assert engine.search_tracks('t', 'a') == [] + + +def test_trigger_library_scan_returns_true_when_client_lacks_method(make_engine): + """SoulSync has no trigger_library_scan (filesystem walks + happen in-process). Engine no-ops with True so callers don't + treat it as a failure.""" + engine = make_engine({'soulsync': _MinimalClient()}, active='soulsync') + assert engine.trigger_library_scan() is True + + +def test_is_library_scanning_returns_false_when_client_lacks_method(make_engine): + engine = make_engine({'soulsync': _MinimalClient()}, active='soulsync') + assert engine.is_library_scanning() is False + + +def test_get_library_stats_returns_empty_dict_when_client_lacks_method(make_engine): + engine = make_engine({'soulsync': _MinimalClient()}, active='soulsync') + assert engine.get_library_stats() == {} From 971d683ebd3b23f15f50013c40fe503cb6ec2e0d Mon Sep 17 00:00:00 2001 From: Broque Thomas <26755000+Nezreka@users.noreply.github.com> Date: Mon, 4 May 2026 21:25:20 -0700 Subject: [PATCH 07/23] C1: Migrate status-check dispatch sites to engine MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two sites in web_server.py replaced: - /status route's media-server connectivity check (4-way if/elif for plex/jellyfin/navidrome/soulsync) → engine.is_connected() - /api/playlists endpoint's server_connected check (3-way if/elif) → engine.is_connected() Engine reads active_server config + dispatches to the right client with internal connection caching preserved (the underlying clients all cache is_connected() calls). Engine constructor now accepts a pre-built clients={...} dict so web_server.py wires the same instances as its existing per-client globals — no double-init. Suite still green. Per-server clients still accessible via engine.client(name) for source-specific reaches. --- core/media_server/engine.py | 23 +++++++++++++++++- web_server.py | 48 +++++++++++++++++++------------------ 2 files changed, 47 insertions(+), 24 deletions(-) diff --git a/core/media_server/engine.py b/core/media_server/engine.py index 55415123..a5d043bb 100644 --- a/core/media_server/engine.py +++ b/core/media_server/engine.py @@ -45,6 +45,7 @@ class MediaServerEngine: self, registry: Optional[MediaServerRegistry] = None, active_server_resolver=None, + clients: Optional[Dict[str, Any]] = None, ) -> None: """Initialize the engine. @@ -56,9 +57,29 @@ class MediaServerEngine: ``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() - self.registry.initialize() + + if clients is not None: + # Wrap pre-built instances (production case from web_server.py + # init). Skip registry.initialize() — we already have the + # instances, just stash them in the registry's slots so + # registry.get(name) works. + for name, client in clients.items(): + self.registry._instances[name] = client + # Mark any registered-but-not-supplied as failed init so + # active_client() returns None for them. + for name in self.registry._specs: + if name not in self.registry._instances: + self.registry._instances[name] = None + else: + self.registry.initialize() if active_server_resolver is None: from config.settings import config_manager diff --git a/web_server.py b/web_server.py index 7034281c..896e7de5 100644 --- a/web_server.py +++ b/web_server.py @@ -603,6 +603,23 @@ try: except Exception as e: logger.error(f" SoulSync library client failed to initialize: {e}") +# Build the MediaServerEngine on top of the per-client globals above. +# Engine wraps the same instances — no double-init. Provides +# ``engine.method()`` dispatch in place of the historic +# ``if active_server == 'plex' / 'jellyfin' / ...`` chains. +try: + from core.media_server import MediaServerEngine + media_server_engine = MediaServerEngine(clients={ + 'plex': plex_client, + 'jellyfin': jellyfin_client, + 'navidrome': navidrome_client, + 'soulsync': soulsync_library_client, + }) + logger.info(" Media server engine initialized") +except Exception as e: + logger.error(f" Media server engine failed to initialize: {e}") + media_server_engine = None + try: soulseek_client = DownloadOrchestrator() logger.info(" Download orchestrator initialized") @@ -3460,23 +3477,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, @@ -9293,15 +9301,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) From b769f02f04fa14083265fca8267e8f7cf8c94ae0 Mon Sep 17 00:00:00 2001 From: Broque Thomas <26755000+Nezreka@users.noreply.github.com> Date: Mon, 4 May 2026 21:28:09 -0700 Subject: [PATCH 08/23] C2: Migrate remaining uniform is_connected dispatches MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two more sites in web_server.py replaced (tag-preview + batch tag-preview server_type checks). Same pattern as C1: 3-way if/elif → engine.is_connected(). Honest scope note: the recon agent counted 33 dispatch sites, but most are deeply server-specific logic where each branch does completely different work (playlist track replace, per- server metadata sync, deep scan with server-specific helpers). Lifting those would move per-server branches into engine helper methods that route the same work — net zero LOC, more indirection. Engine helps where the shape is TRULY uniform; the deep dispatches stay explicit. Phase C ends here at 4 simple sites lifted. Suite still green. --- web_server.py | 18 +++--------------- 1 file changed, 3 insertions(+), 15 deletions(-) diff --git a/web_server.py b/web_server.py index 896e7de5..17b0bd0b 100644 --- a/web_server.py +++ b/web_server.py @@ -9551,13 +9551,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 +9652,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, From 650327ba186c29031b256fb42da4b1ff250b9dab Mon Sep 17 00:00:00 2001 From: Broque Thomas <26755000+Nezreka@users.noreply.github.com> Date: Mon, 4 May 2026 21:45:29 -0700 Subject: [PATCH 09/23] Phase E: Add WHATS_NEW entry for media server engine refactor Internal-track entry covering the media server engine + contract + the honest-scope note explaining why we lifted the 4 truly-uniform is_connected dispatches and left the deep server-specific dispatches explicit (each does fundamentally different work per server, so lifting would just move per-server branches into engine helper methods). --- webui/static/helper.js | 1 + 1 file changed, 1 insertion(+) diff --git a/webui/static/helper.js b/webui/static/helper.js index 411196c9..4004593d 100644 --- a/webui/static/helper.js +++ b/webui/static/helper.js @@ -3432,6 +3432,7 @@ 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 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). web_server.py historically had ~33 `if active_server == "plex" / "jellyfin" / ...` dispatch sites. new `core/media_server/` package provides `MediaServerEngine` that reads `server.active` config + routes to the right client. plugin contract narrowly requires only the four methods every server actually implements (is_connected, ensure_connection, get_all_artists, get_all_album_ids); optional methods (search_tracks, trigger_library_scan, get_library_stats, etc.) are routed with safe defaults so SoulSync standalone (no library scan API since it walks the filesystem directly) doesn\'t need stub methods. lifted the four uniform `is_connected` dispatches into `engine.is_connected()`. honest scope: most "dispatch sites" the recon counted are genuinely different per server (playlist track replace, per-server metadata sync, deep scan with server-specific cache strategies) — those stay explicit per the "lift what\'s truly shared" standard. 35 new tests pin: per-server observable behavior (4 server pinning files, 21 tests), engine cross-server dispatch (10 tests), structural conformance (4 tests). engine reference + plugin contract available for future targeted refactors. zero behavior change for users.' }, { title: 'Internal: Typed Metadata Foundation', desc: 'internal — first step of a multi-pr migration to give the metadata pipeline a real contract. the codebase historically grew duck-typed extractors (`_extract_lookup_value(album_data, "id", "album_id", "collectionId", "release_id", default=...)`) at every consumer site because each provider returns its own response shape. ~150 of those across the codebase. new `core/metadata/types.py` defines canonical typed `Album` / `Track` / `Artist` dataclasses with strict required fields. per-source classmethod converters (from_spotify_dict, from_itunes_dict, from_deezer_dict, from_discogs_dict, from_musicbrainz_dict, from_hydrabase_dict) are the SINGLE place that knows each provider\'s wire shape. zero behavior changes in this pr — pure additive foundation. follow-up prs migrate consumers one at a time. full migration plan documented at docs/metadata-types-migration.md.', page: 'library' }, { title: 'Internal: Migrate Album-Info Builders to Typed Path', desc: 'internal — steps 2+3 of the typed metadata migration in one pr. two album-info builders now route through `Album.from__dict()` when the caller passes a known source: `_build_album_info` (used by every album-tracks lookup) and the embedded album section of `_build_single_import_context_payload` (used by single-track import context resolution). legacy duck-typed extraction stays as the fallback when source is empty/unknown, raw input isn\'t a dict, or the typed converter raises — so a converter bug can\'t break album resolution or import context. caller-provided album_id / album_name / artist_name fallbacks apply on the typed path the same way they did on legacy. zero behavior change for existing callers since they don\'t pass a source yet — opt-in only. 22 new tests pin the typed path, the legacy fallback, and parametrized coverage across registered providers.' }, { title: 'Internal: Migrate Discography + Quality Scanner to Typed Path', desc: 'internal — next round of the typed metadata migration. three more album-shape consumers now route through `Album.from__dict()` when the caller passes a known source: `_build_discography_release_dict` (artist discography release cards), `_build_artist_detail_release_card` (artist detail page release cards), and `_normalize_track_album` (quality scanner result normalization). legacy duck-typed extraction stays as the fallback when source is empty/unknown, raw input isn\'t a dict, or the typed converter raises — same safety contract as the prior migration steps. 20 new tests pin the typed path + legacy fallback + parametrized coverage across registered providers.' }, From 49f7679eef3563e31e7d5a16304e475aca796690 Mon Sep 17 00:00:00 2001 From: Broque Thomas <26755000+Nezreka@users.noreply.github.com> Date: Tue, 5 May 2026 16:59:05 -0700 Subject: [PATCH 10/23] MS Cin-1 + Cin-2: Explicit contract inheritance + generic accessors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Apply the Cin-1 / Cin-2 pattern from the download refactor PR to the media server engine PR before review. Cin-1 — explicit inheritance: - PlexClient, JellyfinClient, NavidromeClient, SoulSyncClient now explicitly inherit MediaServerClient instead of relying on structural typing alone. Pre-change a reader of plex_client.py had no way to know the class was supposed to satisfy the contract. - Removed the engine + registry re-exports from core/media_server/__init__.py to break the circular import that the inheritance change introduced (importing the package now triggered a chain that loaded clients before their base class resolved). Submodules import directly: from core.media_server.engine import MediaServerEngine, etc. - Conformance test now also asserts isinstance() / issubclass() against MediaServerClient — drift in any class fails at the test boundary instead of at runtime. Cin-2 — generic accessors + singleton: - engine.configured_clients() — replaces the legacy per-server `if X and X.is_connected(): clients[name] = X` chains in web_server.py. - engine.reload_config(name=None) — generic dispatch, so callers pass the server name instead of reaching for plex_client.reload_config() directly. - get_media_server_engine() / set_media_server_engine() singleton factory matching the get_metadata_engine() / get_download_orchestrator() shape. web_server.py boots via set_media_server_engine(...) so factory + global handle share state. - 7 new tests pin the accessors + singleton behaviour. --- core/jellyfin_client.py | 5 +- core/media_server/__init__.py | 14 ++-- core/media_server/engine.py | 70 ++++++++++++++++ core/navidrome_client.py | 5 +- core/plex_client.py | 5 +- core/soulsync_client.py | 5 +- tests/media_server/test_conformance.py | 20 ++++- tests/media_server/test_engine.py | 108 ++++++++++++++++++++++++- web_server.py | 7 +- 9 files changed, 226 insertions(+), 13 deletions(-) diff --git a/core/jellyfin_client.py b/core/jellyfin_client.py index 28adba99..ee564f6c 100644 --- a/core/jellyfin_client.py +++ b/core/jellyfin_client.py @@ -146,7 +146,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 diff --git a/core/media_server/__init__.py b/core/media_server/__init__.py index c1f5012b..3c125ede 100644 --- a/core/media_server/__init__.py +++ b/core/media_server/__init__.py @@ -15,15 +15,19 @@ API, SoulSync's filesystem walk). The engine just routes by 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 -from core.media_server.engine import MediaServerEngine -from core.media_server.registry import MediaServerRegistry, build_default_registry __all__ = [ "MediaServerClient", - "MediaServerEngine", - "MediaServerRegistry", - "build_default_registry", ] diff --git a/core/media_server/engine.py b/core/media_server/engine.py index a5d043bb..c1cb202b 100644 --- a/core/media_server/engine.py +++ b/core/media_server/engine.py @@ -234,3 +234,73 @@ class MediaServerEngine: ) return [] return [] + + # ------------------------------------------------------------------ + # Generic accessors — replace per-server attribute reaches in + # callers (Cin's standard from the download refactor). + # ------------------------------------------------------------------ + + 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.""" + result: Dict[str, MediaServerClient] = {} + for name, client in self.registry.all_clients(): + try: + if not hasattr(client, 'is_connected') or client.is_connected(): + result[name] = client + except Exception as exc: + logger.debug("%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/navidrome_client.py b/core/navidrome_client.py index 2cc010a7..0e396fe4 100644 --- a/core/navidrome_client.py +++ b/core/navidrome_client.py @@ -150,7 +150,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 diff --git a/core/plex_client.py b/core/plex_client.py index 43bbf8a8..f9c1a24d 100644 --- a/core/plex_client.py +++ b/core/plex_client.py @@ -74,7 +74,10 @@ class PlexPlaylistInfo: tracks=tracks ) -class PlexClient: +from core.media_server.contract import MediaServerClient + + +class PlexClient(MediaServerClient): def __init__(self): self.server: Optional[PlexServer] = None self.music_library: Optional[MusicSection] = None 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/tests/media_server/test_conformance.py b/tests/media_server/test_conformance.py index fab74111..dcf789c6 100644 --- a/tests/media_server/test_conformance.py +++ b/tests/media_server/test_conformance.py @@ -32,7 +32,7 @@ def _import_server_classes(): 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 import build_default_registry + from core.media_server.registry import build_default_registry registry = build_default_registry() expected = {'plex', 'jellyfin', 'navidrome', 'soulsync'} @@ -52,3 +52,21 @@ def test_server_class_has_all_required_methods(server_name): 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 index f4650a28..6205e306 100644 --- a/tests/media_server/test_engine.py +++ b/tests/media_server/test_engine.py @@ -6,8 +6,8 @@ from unittest.mock import MagicMock import pytest -from core.media_server import MediaServerEngine, MediaServerRegistry -from core.media_server.registry import ServerSpec +from core.media_server.engine import MediaServerEngine +from core.media_server.registry import MediaServerRegistry, ServerSpec class _FakeClient: @@ -190,3 +190,107 @@ def test_is_library_scanning_returns_false_when_client_lacks_method(make_engine) def test_get_library_stats_returns_empty_dict_when_client_lacks_method(make_engine): engine = make_engine({'soulsync': _MinimalClient()}, active='soulsync') assert engine.get_library_stats() == {} + + +# --------------------------------------------------------------------------- +# 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) diff --git a/web_server.py b/web_server.py index 17b0bd0b..d913d0b3 100644 --- a/web_server.py +++ b/web_server.py @@ -608,13 +608,18 @@ except Exception as e: # ``engine.method()`` dispatch in place of the historic # ``if active_server == 'plex' / 'jellyfin' / ...`` chains. try: - from core.media_server import MediaServerEngine + from core.media_server.engine import MediaServerEngine, set_media_server_engine media_server_engine = MediaServerEngine(clients={ 'plex': plex_client, 'jellyfin': jellyfin_client, 'navidrome': navidrome_client, 'soulsync': soulsync_library_client, }) + # 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" Media server engine failed to initialize: {e}") From 1bc5017592e98e74489cfbcb4db836181b7104e3 Mon Sep 17 00:00:00 2001 From: Broque Thomas <26755000+Nezreka@users.noreply.github.com> Date: Tue, 5 May 2026 17:40:27 -0700 Subject: [PATCH 11/23] MS Cin-3 + Cin-4: Route web_server through engine instead of per-client globals MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Pre-change web_server.py had ~70 direct attribute reaches against the per-server globals (plex_client.X, jellyfin_client.X, navidrome_client.X, soulsync_library_client.X) plus ~60 standalone refs (truthy checks, media_client assignments, source-name tuples). The engine was wired but only used in 4 places, so most of the codebase still hand-dispatched — the exact "partially defeats the purpose of this refactor" critique Cin landed on the download PR initially. - All ~70 client.attribute reaches migrated to media_server_engine.client('').attribute. The chains in web_server.py do server-specific work (Plex raw API, Jellyfin / Navidrome client methods, all returning different shapes), so the if/elif structure stays — but the per-server CLIENT REACH now goes through the engine like Cin's POC pattern intended. - All ~60 standalone refs migrated: - if plex_client → if media_server_engine.client('plex') - media_client = plex_client → media_client = media_server_engine.client('plex') - ('plex', plex_client) tuples → ('plex', media_server_engine.client('plex')) - Per-server globals (plex_client / jellyfin_client / navidrome_client / soulsync_library_client) kept for now — external modules (PlaylistSyncService, WebScanManager, ListeningStatsWorker, search library check, discovery sync deps) still take them as kwargs. Dropping them entirely needs a follow-up sweep across those modules. Suite green (1961 pass). --- web_server.py | 220 +++++++++++++++++++++++++------------------------- 1 file changed, 110 insertions(+), 110 deletions(-) diff --git a/web_server.py b/web_server.py index d913d0b3..0c554964 100644 --- a/web_server.py +++ b/web_server.py @@ -644,7 +644,7 @@ except Exception as e: logger.error(f" Matching engine failed to initialize: {e}") try: - sync_service = PlaylistSyncService(spotify_client, plex_client, soulseek_client, jellyfin_client, navidrome_client) + sync_service = PlaylistSyncService(spotify_client, plex_client, soulseek_client, jellyfin_client, media_server_engine.client('navidrome')) logger.info(" Playlist sync service initialized") except Exception as e: logger.error(f" Playlist sync service failed to initialize: {e}") @@ -4154,12 +4154,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 soulseek_client: soulseek_client.reload_settings() @@ -5122,8 +5122,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}") @@ -5134,7 +5134,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 @@ -5143,8 +5143,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, @@ -5166,7 +5166,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") @@ -5182,7 +5182,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 @@ -5191,9 +5191,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 @@ -5217,7 +5217,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") @@ -5233,7 +5233,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 @@ -5242,10 +5242,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 @@ -5269,7 +5269,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") @@ -5285,19 +5285,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 @@ -5318,7 +5318,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: @@ -8046,9 +8046,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 @@ -10810,7 +10810,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: @@ -10825,7 +10825,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: @@ -10853,8 +10853,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 @@ -11526,9 +11526,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('/') @@ -11834,12 +11834,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: @@ -15122,11 +15122,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.") @@ -15168,11 +15168,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 @@ -18045,10 +18045,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': @@ -18061,22 +18061,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}) @@ -18094,24 +18094,24 @@ 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 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() @@ -18136,9 +18136,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', []) @@ -18153,8 +18153,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 @@ -18179,9 +18179,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('/') @@ -18364,10 +18364,10 @@ def server_playlist_replace_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'): # Use raw Plex API - fetch playlist directly try: - raw_playlist = plex_client.server.playlist(playlist_name) + raw_playlist = media_server_engine.client('plex').server.playlist(playlist_name) except Exception: return jsonify({"success": False, "error": "Playlist not found on server"}), 404 @@ -18376,7 +18376,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 @@ -18389,13 +18389,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 []): @@ -18408,12 +18408,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 []): @@ -18426,7 +18426,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 @@ -18452,12 +18452,12 @@ 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'): try: - raw_playlist = plex_client.server.playlist(playlist_name) + raw_playlist = media_server_engine.client('plex').server.playlist(playlist_name) except Exception: return jsonify({"success": False, "error": "Playlist not found"}), 404 - new_item = plex_client.server.fetchItem(int(track_id)) + new_item = media_server_engine.client('plex').server.fetchItem(int(track_id)) if not new_item: return jsonify({"success": False, "error": "Track not found on server"}), 404 @@ -18484,22 +18484,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 @@ -18523,9 +18523,9 @@ 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'): try: - raw_playlist = plex_client.server.playlist(playlist_name) + raw_playlist = media_server_engine.client('plex').server.playlist(playlist_name) except Exception: return jsonify({"success": False, "error": "Playlist not found"}), 404 @@ -18537,26 +18537,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 @@ -18580,9 +18580,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('/') @@ -23399,12 +23399,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, @@ -23413,8 +23413,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, } }) @@ -29133,17 +29133,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 From d3f8a06d7a1c0fc6f111d767fa147d82445975db Mon Sep 17 00:00:00 2001 From: Broque Thomas <26755000+Nezreka@users.noreply.github.com> Date: Tue, 5 May 2026 17:43:05 -0700 Subject: [PATCH 12/23] WHATS_NEW entry for media server engine review pass --- webui/static/helper.js | 1 + 1 file changed, 1 insertion(+) diff --git a/webui/static/helper.js b/webui/static/helper.js index 4004593d..eac147e8 100644 --- a/webui/static/helper.js +++ b/webui/static/helper.js @@ -3432,6 +3432,7 @@ 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. the per-server if/elif chains stay because the work is genuinely server-specific (Plex raw API vs Jellyfin / Navidrome client methods returning different shapes), but the per-server CLIENT REACH now goes through the engine like the POC pattern intended. 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). web_server.py historically had ~33 `if active_server == "plex" / "jellyfin" / ...` dispatch sites. new `core/media_server/` package provides `MediaServerEngine` that reads `server.active` config + routes to the right client. plugin contract narrowly requires only the four methods every server actually implements (is_connected, ensure_connection, get_all_artists, get_all_album_ids); optional methods (search_tracks, trigger_library_scan, get_library_stats, etc.) are routed with safe defaults so SoulSync standalone (no library scan API since it walks the filesystem directly) doesn\'t need stub methods. lifted the four uniform `is_connected` dispatches into `engine.is_connected()`. honest scope: most "dispatch sites" the recon counted are genuinely different per server (playlist track replace, per-server metadata sync, deep scan with server-specific cache strategies) — those stay explicit per the "lift what\'s truly shared" standard. 35 new tests pin: per-server observable behavior (4 server pinning files, 21 tests), engine cross-server dispatch (10 tests), structural conformance (4 tests). engine reference + plugin contract available for future targeted refactors. zero behavior change for users.' }, { title: 'Internal: Typed Metadata Foundation', desc: 'internal — first step of a multi-pr migration to give the metadata pipeline a real contract. the codebase historically grew duck-typed extractors (`_extract_lookup_value(album_data, "id", "album_id", "collectionId", "release_id", default=...)`) at every consumer site because each provider returns its own response shape. ~150 of those across the codebase. new `core/metadata/types.py` defines canonical typed `Album` / `Track` / `Artist` dataclasses with strict required fields. per-source classmethod converters (from_spotify_dict, from_itunes_dict, from_deezer_dict, from_discogs_dict, from_musicbrainz_dict, from_hydrabase_dict) are the SINGLE place that knows each provider\'s wire shape. zero behavior changes in this pr — pure additive foundation. follow-up prs migrate consumers one at a time. full migration plan documented at docs/metadata-types-migration.md.', page: 'library' }, { title: 'Internal: Migrate Album-Info Builders to Typed Path', desc: 'internal — steps 2+3 of the typed metadata migration in one pr. two album-info builders now route through `Album.from__dict()` when the caller passes a known source: `_build_album_info` (used by every album-tracks lookup) and the embedded album section of `_build_single_import_context_payload` (used by single-track import context resolution). legacy duck-typed extraction stays as the fallback when source is empty/unknown, raw input isn\'t a dict, or the typed converter raises — so a converter bug can\'t break album resolution or import context. caller-provided album_id / album_name / artist_name fallbacks apply on the typed path the same way they did on legacy. zero behavior change for existing callers since they don\'t pass a source yet — opt-in only. 22 new tests pin the typed path, the legacy fallback, and parametrized coverage across registered providers.' }, From a6bb5f5b43499ccd6a39176ce16080331f34e4c9 Mon Sep 17 00:00:00 2001 From: Broque Thomas <26755000+Nezreka@users.noreply.github.com> Date: Tue, 5 May 2026 18:05:45 -0700 Subject: [PATCH 13/23] =?UTF-8?q?MS=20Cin-5:=20Drop=20per-server=20globals?= =?UTF-8?q?=20=E2=80=94=20engine=20owns=20the=20clients?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Per-server web_server.py globals (plex_client / jellyfin_client / navidrome_client / soulsync_library_client) are gone. The engine now owns the per-server client instances; web_server.py constructs them inline into the engine init and routes everything through media_server_engine.client(''). Multi-client consumers refactored to take the engine instead of separate per-server kwargs: - services/sync_service.py: PlaylistSyncService.__init__ now takes media_server_engine. Internal _get_active_media_client resolves the active server's client through self._engine.client(name) instead of the per-server self.X_client attributes. - core/listening_stats_worker.py: ListeningStatsWorker takes media_server_engine. The plex/jellyfin/navidrome dispatch in _poll collapses to engine.client(active_server) (gated to those three servers — SoulSync standalone has no listening data). - core/web_scan_manager.py: WebScanManager takes media_server_engine instead of the hand-keyed media_clients dict that drifted out of sync with the engine. - core/discovery/sync.py: SyncDeps holds media_server_engine instead of plex_client / jellyfin_client. Playlist-image dispatch routes through engine.client(name). Web_server.py: - Per-server globals removed from the chained `= None` init line + their try/except construction blocks. Replaced with a _safe_init_media_client(factory, name) helper that captures per-server init failures + passes the resulting clients straight into the MediaServerEngine init dict. - All construction sites (PlaylistSyncService, WebScanManager, ListeningStatsWorker, SyncDeps, library_check) updated to receive the engine instead of per-server clients. Test fixtures (tests/discovery/test_discovery_sync.py) gain a _FakeMediaServerEngine stub + the SyncDeps build helper passes that instead of separate plex/jellyfin clients. --- core/discovery/sync.py | 17 ++++--- core/listening_stats_worker.py | 26 +++++----- core/web_scan_manager.py | 37 ++++++-------- services/sync_service.py | 47 +++++++++++------ tests/discovery/test_discovery_sync.py | 23 +++++++-- web_server.py | 70 +++++++++----------------- 6 files changed, 114 insertions(+), 106 deletions(-) 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/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/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/services/sync_service.py b/services/sync_service.py index 4582a5cf..119aff87 100644 --- a/services/sync_service.py +++ b/services/sync_service.py @@ -44,17 +44,31 @@ class SyncProgress: failed_tracks: int = 0 class PlaylistSyncService: - def __init__(self, spotify_client: SpotifyClient, plex_client: PlexClient, soulseek_client: SoulseekClient, jellyfin_client: JellyfinClient = None, navidrome_client = None): + def __init__(self, spotify_client: SpotifyClient, soulseek_client: SoulseekClient, 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. + """ 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.soulseek_client = soulseek_client 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 +82,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.""" 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/web_server.py b/web_server.py index 0c554964..3ac6a69e 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 = soulseek_client = tidal_client = matching_engine = sync_service = web_scan_manager = None +spotify_client = soulseek_client = tidal_client = matching_engine = sync_service = web_scan_manager = None try: spotify_client = get_spotify_client() @@ -578,42 +578,31 @@ 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: + instance = factory() + logger.info(f" {name} client initialized") + return instance + except Exception as exc: + logger.error(f" {name} client failed to initialize: {exc}") + return None -try: - navidrome_client = NavidromeClient() - logger.info(" Navidrome client initialized") -except Exception as e: - logger.error(f" Navidrome client failed to initialize: {e}") -try: - from core.soulsync_client import SoulSyncClient - soulsync_library_client = SoulSyncClient() - logger.info(" SoulSync library client initialized") -except Exception as e: - logger.error(f" SoulSync library client failed to initialize: {e}") - -# Build the MediaServerEngine on top of the per-client globals above. -# Engine wraps the same instances — no double-init. Provides -# ``engine.method()`` dispatch in place of the historic -# ``if active_server == 'plex' / 'jellyfin' / ...`` chains. +# 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 media_server_engine = MediaServerEngine(clients={ - 'plex': plex_client, - 'jellyfin': jellyfin_client, - 'navidrome': navidrome_client, - 'soulsync': soulsync_library_client, + '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 @@ -644,7 +633,7 @@ except Exception as e: logger.error(f" Matching engine failed to initialize: {e}") try: - sync_service = PlaylistSyncService(spotify_client, plex_client, soulseek_client, jellyfin_client, media_server_engine.client('navidrome')) + sync_service = PlaylistSyncService(spotify_client, soulseek_client, 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}") @@ -669,13 +658,7 @@ if soulseek_client: # 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}") @@ -6879,7 +6862,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', []), @@ -23266,8 +23249,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, @@ -32848,9 +32830,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") From 2ebaf2e6e3b976cf5a5d21ba2f3c0e183ddbb99f Mon Sep 17 00:00:00 2001 From: Broque Thomas <26755000+Nezreka@users.noreply.github.com> Date: Tue, 5 May 2026 18:25:28 -0700 Subject: [PATCH 14/23] MS Gap 1: Lift shared TrackInfo + PlaylistInfo to neutral types module MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Plex / Jellyfin / Navidrome each defined a near-identical XTrackInfo (id / title / artist / album / duration / track_number / year / rating) and XPlaylistInfo (id / title / description / duration / leaf_count / tracks). Three classes that grew up by copy-paste — not a real contract difference. Lifted both to core/media_server/types.py as canonical TrackInfo + PlaylistInfo. Per-server constructors live as classmethods on the unified class (TrackInfo.from_plex_track, PlaylistInfo.from_plex_playlist) matching the metadata Album.from_X_dict pattern Cin's POC uses. Heavy plexapi imports stay lazy under TYPE_CHECKING. - core/plex_client.py / jellyfin_client.py / navidrome_client.py: per-server XTrackInfo / XPlaylistInfo dataclass definitions removed; each module now imports TrackInfo + PlaylistInfo from the neutral package and uses the shared name internally. - core/matching_engine.py: was annotating callers with PlexTrackInfo even though sync_service hands it Jellyfin / Navidrome instances at runtime when those servers are active. Annotation is now the unified TrackInfo, so signatures match the actual contract. - services/sync_service.py: same import + annotation update. --- core/jellyfin_client.py | 32 +++------ core/matching_engine.py | 8 +-- core/media_server/types.py | 130 +++++++++++++++++++++++++++++++++++++ core/navidrome_client.py | 38 ++++------- core/plex_client.py | 94 ++++++--------------------- services/sync_service.py | 5 +- 6 files changed, 177 insertions(+), 130 deletions(-) create mode 100644 core/media_server/types.py diff --git a/core/jellyfin_client.py b/core/jellyfin_client.py index ee564f6c..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""" @@ -1204,7 +1190,7 @@ class JellyfinClient(MediaServerClient): 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 [] @@ -1221,7 +1207,7 @@ class JellyfinClient(MediaServerClient): 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'), @@ -1238,7 +1224,7 @@ class JellyfinClient(MediaServerClient): 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/matching_engine.py b/core/matching_engine.py index da1fb9bc..fc76fb18 100644 --- a/core/matching_engine.py +++ b/core/matching_engine.py @@ -7,7 +7,7 @@ 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 from core.soulseek_client import TrackResult, AlbumResult @@ -16,7 +16,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 +302,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 +313,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/types.py b/core/media_server/types.py new file mode 100644 index 00000000..7d48e1c5 --- /dev/null +++ b/core/media_server/types.py @@ -0,0 +1,130 @@ +"""Shared dataclasses for the media-server contract. + +Plex / Jellyfin / Navidrome / SoulSync 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. Per-server constructors live +on the unified class as classmethods (``TrackInfo.from_plex_track``, +``TrackInfo.from_jellyfin_dict``, ``TrackInfo.from_navidrome_dict``) +— same pattern Cin used for ``Album.from_spotify_dict`` etc. on the +metadata engine. Heavy server SDK types (``PlexTrack``, dict +shapes) imported under TYPE_CHECKING so this module stays +import-light. +""" + +from __future__ import annotations + +from dataclasses import dataclass, field +from datetime import datetime +from typing import TYPE_CHECKING, Any, Dict, 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 every media server client. + + All four servers (Plex, Jellyfin, Navidrome, SoulSync standalone) + used to define their own near-identical ``XTrackInfo`` dataclass. + 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 0e396fe4..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""" @@ -827,7 +813,7 @@ class NavidromeClient(MediaServerClient): 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 [] @@ -841,7 +827,7 @@ class NavidromeClient(MediaServerClient): 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'), @@ -858,7 +844,7 @@ class NavidromeClient(MediaServerClient): 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: @@ -979,7 +965,7 @@ class NavidromeClient(MediaServerClient): 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() @@ -1143,7 +1129,7 @@ class NavidromeClient(MediaServerClient): 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.") @@ -1169,7 +1155,7 @@ class NavidromeClient(MediaServerClient): 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 f9c1a24d..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,70 +11,15 @@ from utils.logging_config import get_logger from config.settings import config_manager import threading -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 - ) - +# 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") + class PlexClient(MediaServerClient): def __init__(self): @@ -271,7 +215,7 @@ class PlexClient(MediaServerClient): """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 [] @@ -281,7 +225,7 @@ class PlexClient(MediaServerClient): 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") @@ -291,14 +235,14 @@ class PlexClient(MediaServerClient): 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: @@ -314,14 +258,14 @@ class PlexClient(MediaServerClient): 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) @@ -329,7 +273,7 @@ class PlexClient(MediaServerClient): 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) @@ -451,7 +395,7 @@ class PlexClient(MediaServerClient): 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 @@ -522,7 +466,7 @@ class PlexClient(MediaServerClient): 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. @@ -557,7 +501,7 @@ class PlexClient(MediaServerClient): # --- 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): @@ -576,7 +520,7 @@ class PlexClient(MediaServerClient): # --- 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): @@ -590,7 +534,7 @@ class PlexClient(MediaServerClient): # 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/services/sync_service.py b/services/sync_service.py index 119aff87..39fb0930 100644 --- a/services/sync_service.py +++ b/services/sync_service.py @@ -4,7 +4,8 @@ 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.plex_client import PlexClient +from core.media_server.types import TrackInfo from core.jellyfin_client import JellyfinClient from core.navidrome_client import NavidromeClient from core.soulseek_client import SoulseekClient @@ -456,7 +457,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 From edb6d1bc3344894461ed47a7be147c8a1adc89bf Mon Sep 17 00:00:00 2001 From: Broque Thomas <26755000+Nezreka@users.noreply.github.com> Date: Tue, 5 May 2026 18:36:36 -0700 Subject: [PATCH 15/23] Drop dead per-server class imports + update WHATS_NEW - services/sync_service.py: dropped unused PlexClient / JellyfinClient / NavidromeClient class imports. After the engine refactor the service only needs TrackInfo for type annotations; the class imports were dead. - WHATS_NEW: extended the media server engine review-pass entry to cover the followup commits (Cin-5 per-server global removal + Gap 1 shared types lift) so the changelog matches the actual branch state. --- services/sync_service.py | 3 --- webui/static/helper.js | 2 +- 2 files changed, 1 insertion(+), 4 deletions(-) diff --git a/services/sync_service.py b/services/sync_service.py index 39fb0930..0c017633 100644 --- a/services/sync_service.py +++ b/services/sync_service.py @@ -4,10 +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 from core.media_server.types import TrackInfo -from core.jellyfin_client import JellyfinClient -from core.navidrome_client import NavidromeClient from core.soulseek_client import SoulseekClient from core.matching_engine import MusicMatchingEngine, MatchResult diff --git a/webui/static/helper.js b/webui/static/helper.js index eac147e8..33e03fe0 100644 --- a/webui/static/helper.js +++ b/webui/static/helper.js @@ -3432,7 +3432,7 @@ 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. the per-server if/elif chains stay because the work is genuinely server-specific (Plex raw API vs Jellyfin / Navidrome client methods returning different shapes), but the per-server CLIENT REACH now goes through the engine like the POC pattern intended. zero behavior change.' }, + { 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). web_server.py historically had ~33 `if active_server == "plex" / "jellyfin" / ...` dispatch sites. new `core/media_server/` package provides `MediaServerEngine` that reads `server.active` config + routes to the right client. plugin contract narrowly requires only the four methods every server actually implements (is_connected, ensure_connection, get_all_artists, get_all_album_ids); optional methods (search_tracks, trigger_library_scan, get_library_stats, etc.) are routed with safe defaults so SoulSync standalone (no library scan API since it walks the filesystem directly) doesn\'t need stub methods. lifted the four uniform `is_connected` dispatches into `engine.is_connected()`. honest scope: most "dispatch sites" the recon counted are genuinely different per server (playlist track replace, per-server metadata sync, deep scan with server-specific cache strategies) — those stay explicit per the "lift what\'s truly shared" standard. 35 new tests pin: per-server observable behavior (4 server pinning files, 21 tests), engine cross-server dispatch (10 tests), structural conformance (4 tests). engine reference + plugin contract available for future targeted refactors. zero behavior change for users.' }, { title: 'Internal: Typed Metadata Foundation', desc: 'internal — first step of a multi-pr migration to give the metadata pipeline a real contract. the codebase historically grew duck-typed extractors (`_extract_lookup_value(album_data, "id", "album_id", "collectionId", "release_id", default=...)`) at every consumer site because each provider returns its own response shape. ~150 of those across the codebase. new `core/metadata/types.py` defines canonical typed `Album` / `Track` / `Artist` dataclasses with strict required fields. per-source classmethod converters (from_spotify_dict, from_itunes_dict, from_deezer_dict, from_discogs_dict, from_musicbrainz_dict, from_hydrabase_dict) are the SINGLE place that knows each provider\'s wire shape. zero behavior changes in this pr — pure additive foundation. follow-up prs migrate consumers one at a time. full migration plan documented at docs/metadata-types-migration.md.', page: 'library' }, { title: 'Internal: Migrate Album-Info Builders to Typed Path', desc: 'internal — steps 2+3 of the typed metadata migration in one pr. two album-info builders now route through `Album.from__dict()` when the caller passes a known source: `_build_album_info` (used by every album-tracks lookup) and the embedded album section of `_build_single_import_context_payload` (used by single-track import context resolution). legacy duck-typed extraction stays as the fallback when source is empty/unknown, raw input isn\'t a dict, or the typed converter raises — so a converter bug can\'t break album resolution or import context. caller-provided album_id / album_name / artist_name fallbacks apply on the typed path the same way they did on legacy. zero behavior change for existing callers since they don\'t pass a source yet — opt-in only. 22 new tests pin the typed path, the legacy fallback, and parametrized coverage across registered providers.' }, From 03d1c366372e4181e4463adbb83d73b8930772ed Mon Sep 17 00:00:00 2001 From: Broque Thomas <26755000+Nezreka@users.noreply.github.com> Date: Tue, 5 May 2026 18:42:21 -0700 Subject: [PATCH 16/23] Fallback to empty MediaServerEngine if init fails MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If MediaServerEngine init raised, ``media_server_engine`` was set to None. Every downstream caller (now that the per-server globals are gone) does ``media_server_engine.client('plex')`` style access — which would AttributeError on the None. Pre-refactor each per-server global had its own try/except so engine failure didn't take down per-server dispatch sites. Preserve that resilience by falling back to an empty MediaServerEngine — its ``client(name)`` returns None, downstream truthy checks treat that as "not configured" exactly the same way the legacy globals did. --- web_server.py | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/web_server.py b/web_server.py index 3ac6a69e..3c6aeb90 100644 --- a/web_server.py +++ b/web_server.py @@ -612,7 +612,17 @@ try: logger.info(" Media server engine initialized") except Exception as e: logger.error(f" Media server engine failed to initialize: {e}") - media_server_engine = None + # 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 was its own + # try/except so engine failure didn't take down dispatch sites; + # this preserves that resilience. + 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: + media_server_engine = None try: soulseek_client = DownloadOrchestrator() From 218af65606a34f75503d483537471e9631a6e40f Mon Sep 17 00:00:00 2001 From: Broque Thomas <26755000+Nezreka@users.noreply.github.com> Date: Tue, 5 May 2026 19:54:31 -0700 Subject: [PATCH 17/23] ID-first fallback for server-playlist add-track + diagnostic logging MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The /api/server/playlist//add-track endpoint only looked up the target Plex playlist by name, but Plex deletes + recreates playlists on edit so the rating key the frontend cached can be stale. The companion GET /tracks endpoint already had id-first / name-fallback; add-track now does the same. Added a warning log on GET /tracks when BOTH lookups fail so the "all source items show Find & Add" symptom (which happens when server_tracks comes back empty) has a clear diagnostic in the log instead of silently rendering an empty server column. Not a refactor regression — the original code had the same name-only lookup. The mass-replace of `plex_client` → `media_server_engine.client('plex')` is byte-equivalent. Just surfacing the latent bug. --- web_server.py | 24 ++++++++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-) diff --git a/web_server.py b/web_server.py index 3c6aeb90..66302b51 100644 --- a/web_server.py +++ b/web_server.py @@ -18100,6 +18100,12 @@ def get_server_playlist_tracks(playlist_id): 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 @@ -18446,11 +18452,25 @@ def server_playlist_add_track(playlist_id): active_server = config_manager.get_active_media_server() 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 = media_server_engine.client('plex').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 = media_server_engine.client('plex').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 From 397a1c73df748005e488e8947bb147713b6ee86a Mon Sep 17 00:00:00 2001 From: Broque Thomas <26755000+Nezreka@users.noreply.github.com> Date: Tue, 5 May 2026 20:06:08 -0700 Subject: [PATCH 18/23] ID-first fallback for replace-track + remove-track too MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Same latent bug as add-track — replace-track and remove-track only looked up the Plex playlist by name. Plex deletes + recreates playlists on edit so the rating key the frontend cached can be stale, name lookups can also fail (special chars, encoding). Both now use the same id-first / name-fallback chain as the GET tracks endpoint, with a diagnostic log when both lookups fail. Pre-existing latent bug, not a refactor regression. --- web_server.py | 29 ++++++++++++++++++++++++++--- 1 file changed, 26 insertions(+), 3 deletions(-) diff --git a/web_server.py b/web_server.py index 66302b51..7b530d42 100644 --- a/web_server.py +++ b/web_server.py @@ -18364,10 +18364,21 @@ def server_playlist_replace_track(playlist_id): active_server = config_manager.get_active_media_server() if active_server == 'plex' and media_server_engine.client('plex'): - # Use raw Plex API - fetch playlist directly + # 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 = media_server_engine.client('plex').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 @@ -18537,9 +18548,21 @@ def server_playlist_remove_track(playlist_id): active_server = config_manager.get_active_media_server() 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 = media_server_engine.client('plex').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 From 860f9a0a8cac2a2b1f19c6bbf4c8047354da4152 Mon Sep 17 00:00:00 2001 From: Broque Thomas <26755000+Nezreka@users.noreply.github.com> Date: Tue, 5 May 2026 21:04:14 -0700 Subject: [PATCH 19/23] =?UTF-8?q?MS=20pre-review=20polish=20=E2=80=94=20en?= =?UTF-8?q?capsulation=20+=20visibility=20+=20tests?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Five tightening passes anticipating Cin / JohnBaumb's review nits: (1) Engine no longer reaches into ``registry._instances`` private attr. New public ``MediaServerRegistry.set_instance(name, client)`` method — engine constructor calls it for the ``clients=`` pre-built case so internal storage stays encapsulated. (2) Engine module docstring no longer overclaims. Originally said it "Replaces the historic 33+ if/elif chains" — but only the four uniform-shape ``is_connected`` chains were collapsed. The 19 chains that do server-specific work (Plex raw API vs Jellyfin / Navidrome client methods returning different shapes) stay explicit per the "lift what's truly shared" standard. Docstring rewritten to say exactly that. (3) Per-method exception swallows upgraded from ``logger.debug`` to ``logger.warning``. Returning safe defaults stays the right behavior for a read-side engine (Plex offline shouldn't crash the app), but silent debug-level swallowing made debugging hard — JohnBaumb pushed the download engine to surface real errors. Same treatment here: default still safe, but the warning tells you Plex is down. (4) ``_safe_init_media_client`` in web_server.py now logs the exception type + traceback. Broad ``except Exception`` is still intentional (any failure means that one server can't be used; the others stay up) but the boot log now distinguishes config errors (ConnectionError, AuthenticationError) from import / dependency failures. (5) Two new tests pin the encapsulation + fallback contracts: - ``test_engine_with_empty_clients_dict_is_safe_to_use`` — empty engine returns safe defaults on every method, doesn't raise. - ``test_engine_uses_registry_set_instance_not_private_attr`` — spy on registry.set_instance verifies engine uses the public method. --- core/media_server/engine.py | 53 +++++++++++++------------ core/media_server/registry.py | 9 +++++ tests/media_server/test_engine.py | 65 +++++++++++++++++++++++++++++++ web_server.py | 14 ++++++- 4 files changed, 115 insertions(+), 26 deletions(-) diff --git a/core/media_server/engine.py b/core/media_server/engine.py index c1cb202b..2f22c37b 100644 --- a/core/media_server/engine.py +++ b/core/media_server/engine.py @@ -1,9 +1,10 @@ """MediaServerEngine — central dispatch for media server operations. -Replaces the historic 33+ ``if active_server == 'plex' / 'jellyfin' / -'navidrome' / 'soulsync'`` chains in ``web_server.py``. Each -operation web_server.py used to dispatch by hand becomes a single -``engine.method()`` call here that: +Replaces the *uniform-shape* dispatch chains in web_server.py +(``is_connected``, ``get_all_artists``, etc. — anything where every +server returns the same shape and the only branching was on +``active_server == X``). Each such operation is now one +``engine.method()`` call that: 1. Reads the ``server.active`` config to find the current target. 2. Looks up the registered client. @@ -11,14 +12,17 @@ operation web_server.py used to dispatch by hand becomes a single for methods that don't exist on every client — e.g. SoulSync has no library-scan API). -Per-server client objects stay accessible via ``engine.client(name)`` -so any caller that needs a Plex-specific method (e.g. -``set_music_library_by_name`` for the settings page) keeps working -through ``engine.client('plex').set_music_library_by_name(...)``. +Server-specific dispatch sites (Plex's raw playlist API, Jellyfin / +Navidrome client methods returning different shapes) stay explicit +in web_server.py per the "lift what's truly shared" standard. They +reach individual clients via ``engine.client(name)`` rather than +the per-server globals — same generic-accessor pattern as the +download orchestrator. Engine itself is constructed once during web_server.py init and -held as a module-level singleton, mirroring the existing pattern -for the per-server client globals. +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 @@ -69,15 +73,16 @@ class MediaServerEngine: if clients is not None: # Wrap pre-built instances (production case from web_server.py # init). Skip registry.initialize() — we already have the - # instances, just stash them in the registry's slots so - # registry.get(name) works. + # 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._instances[name] = client + 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._specs: - if name not in self.registry._instances: - self.registry._instances[name] = None + 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() @@ -119,7 +124,7 @@ class MediaServerEngine: try: return client.is_connected() except Exception as exc: - logger.debug("%s is_connected raised: %s", self.active_server, exc) + logger.warning("%s is_connected raised: %s", self.active_server, exc) return False def ensure_connection(self) -> bool: @@ -131,7 +136,7 @@ class MediaServerEngine: try: return client.ensure_connection() except Exception as exc: - logger.debug("%s ensure_connection raised: %s", self.active_server, exc) + logger.warning("%s ensure_connection raised: %s", self.active_server, exc) return False def get_all_artists(self) -> List[Any]: @@ -143,7 +148,7 @@ class MediaServerEngine: try: return client.get_all_artists() except Exception as exc: - logger.debug("%s get_all_artists raised: %s", self.active_server, exc) + logger.warning("%s get_all_artists raised: %s", self.active_server, exc) return [] def get_all_album_ids(self) -> set: @@ -155,7 +160,7 @@ class MediaServerEngine: try: return client.get_all_album_ids() except Exception as exc: - logger.debug("%s get_all_album_ids raised: %s", self.active_server, exc) + logger.warning("%s get_all_album_ids raised: %s", self.active_server, exc) return set() # ------------------------------------------------------------------ @@ -174,7 +179,7 @@ class MediaServerEngine: try: return client.search_tracks(title, artist, limit) except Exception as exc: - logger.debug("%s search_tracks raised: %s", self.active_server, exc) + logger.warning("%s search_tracks raised: %s", self.active_server, exc) return [] def trigger_library_scan(self) -> bool: @@ -188,7 +193,7 @@ class MediaServerEngine: try: return client.trigger_library_scan() except Exception as exc: - logger.debug("%s trigger_library_scan raised: %s", self.active_server, exc) + logger.warning("%s trigger_library_scan raised: %s", self.active_server, exc) return False def is_library_scanning(self) -> bool: @@ -200,7 +205,7 @@ class MediaServerEngine: try: return client.is_library_scanning() except Exception as exc: - logger.debug("%s is_library_scanning raised: %s", self.active_server, exc) + logger.warning("%s is_library_scanning raised: %s", self.active_server, exc) return False def get_library_stats(self) -> Dict[str, int]: @@ -212,7 +217,7 @@ class MediaServerEngine: try: return client.get_library_stats() except Exception as exc: - logger.debug("%s get_library_stats raised: %s", self.active_server, exc) + logger.warning("%s get_library_stats raised: %s", self.active_server, exc) return {} def get_recently_added_albums(self, max_results: int = 400) -> List[Any]: diff --git a/core/media_server/registry.py b/core/media_server/registry.py index 173c7a72..6418e002 100644 --- a/core/media_server/registry.py +++ b/core/media_server/registry.py @@ -73,6 +73,15 @@ class MediaServerRegistry: 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) diff --git a/tests/media_server/test_engine.py b/tests/media_server/test_engine.py index 6205e306..eb0be123 100644 --- a/tests/media_server/test_engine.py +++ b/tests/media_server/test_engine.py @@ -294,3 +294,68 @@ def test_get_media_server_engine_returns_set_singleton(make_engine): 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 method 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 + # Cross-server methods return safe defaults instead of raising. + assert engine.is_connected() is False + assert engine.get_all_artists() == [] + assert engine.get_all_album_ids() == set() + assert engine.search_tracks('t', 'a') == [] + # 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/web_server.py b/web_server.py index 292baaa4..401f5b2a 100644 --- a/web_server.py +++ b/web_server.py @@ -581,13 +581,23 @@ except Exception as 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.""" + so one broken server doesn't take the engine down with it. + + 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: {exc}") + logger.error( + f" {name} client failed to initialize: {type(exc).__name__}: {exc}", + exc_info=True, + ) return None From 99f527ecd1db68535a01009019738baf9da1ba9b Mon Sep 17 00:00:00 2001 From: Broque Thomas <26755000+Nezreka@users.noreply.github.com> Date: Tue, 5 May 2026 21:25:27 -0700 Subject: [PATCH 20/23] =?UTF-8?q?MS=20pre-review=20polish=20followup=20?= =?UTF-8?q?=E2=80=94=20log=20levels=20+=20docstring=20honesty?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three nits I missed in the prior pass: (1) Two more exception swallows still at logger.debug — the get_recently_added_albums wrapper and the configured_clients inner loop. My earlier sed only matched single-line patterns. Both now log at warning level so a broken Plex / Jellyfin surfaces in the boot log instead of silently returning []. (2) registry.py module docstring still claimed it replaced "33 hand-maintained dispatch sites" — same overclaim I fixed on the engine docstring. Server-specific chains stay explicit in web_server.py per the "lift what's truly shared" standard; the registry just owns name → client lookup. Docstring rewritten to match reality. --- core/media_server/engine.py | 4 ++-- core/media_server/registry.py | 14 ++++++++++---- 2 files changed, 12 insertions(+), 6 deletions(-) diff --git a/core/media_server/engine.py b/core/media_server/engine.py index 2f22c37b..52b96b2a 100644 --- a/core/media_server/engine.py +++ b/core/media_server/engine.py @@ -233,7 +233,7 @@ class MediaServerEngine: try: return client.get_recently_added_albums(max_results) except Exception as exc: - logger.debug( + logger.warning( "%s get_recently_added_albums raised: %s", self.active_server, exc, ) @@ -256,7 +256,7 @@ class MediaServerEngine: if not hasattr(client, 'is_connected') or client.is_connected(): result[name] = client except Exception as exc: - logger.debug("%s is_connected raised in configured_clients: %s", name, exc) + logger.warning("%s is_connected raised in configured_clients: %s", name, exc) return result def reload_config(self, name: Optional[str] = None) -> bool: diff --git a/core/media_server/registry.py b/core/media_server/registry.py index 6418e002..03fe20b3 100644 --- a/core/media_server/registry.py +++ b/core/media_server/registry.py @@ -3,11 +3,17 @@ 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 -+ 33 hand-maintained ``if active_server == 'plex' / 'jellyfin' / ...`` -dispatch sites. +that every dispatch site reached individually. -Adding a new server (e.g. Subsonic, Emby) becomes one ``register`` -call here + the new client class. Web server dispatch stays put. +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 6489244bcc6898db8b6e54082b7c866aa3f6ccb4 Mon Sep 17 00:00:00 2001 From: Broque Thomas <26755000+Nezreka@users.noreply.github.com> Date: Tue, 5 May 2026 22:36:05 -0700 Subject: [PATCH 21/23] =?UTF-8?q?MS=20Cin/JohnBaumb=20honesty=20pass=20?= =?UTF-8?q?=E2=80=94=20drop=20dead=20wrappers,=20sync=20contract=20to=20re?= =?UTF-8?q?ality?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Pre-review audit found premature abstraction + lying docstrings. Cut what isn't used, made the rest match what's actually shipped. (1) Engine: dropped 7 cross-server dispatch wrappers that had ZERO production callers (ensure_connection / get_all_artists / get_all_album_ids / search_tracks / trigger_library_scan / is_library_scanning / get_library_stats / get_recently_added_albums). Every consumer reaches the active client directly via sync_service._get_active_media_client() or engine.client(name). Engine surface shrinks to client(name) / active_client() / active_server / is_connected() (the one wrapper that has callers — 4 dashboard status sites) / configured_clients() / reload_config(). ~150 lines deleted, 5 dead-method tests removed. (2) Contract Protocol body trimmed to match REQUIRED_METHODS exactly (is_connected, ensure_connection, get_all_artists, get_all_album_ids). The other 5 methods that were declared in the Protocol "required" section weren't actually required — Plex doesn't implement get_recently_added_albums, Jellyfin doesn't implement search_tracks, SoulSync doesn't implement most of them. Static contract now matches runtime conformance test. Optional methods moved to a KNOWN_PER_SERVER_METHODS data-only listing with audited per-server coverage notes — discoverability without false promises. (3) Engine module docstring + __init__.py docstring no longer overclaim "33+ chains collapsed" — only 4 uniform-shape chains were collapsed; ~18 server-specific chains stay explicit per the "lift what's truly shared" standard. Phrasing now matches reality. (4) types.py docstring claimed TrackInfo.from_jellyfin_dict and TrackInfo.from_navidrome_dict exist as classmethods. They don't — only from_plex_track / from_plex_playlist do. Jellyfin and Navidrome construct TrackInfo inline at their call sites today. Docstring now honest about that + flags the lift as a clean followup. (5) Engine line 95 comment "backward-compat for source-specific reaches" was misleading — there is no legacy alternative being preserved; engine.client(name) IS the canonical access pattern. Section header rewritten. Tests: 2121 pass (was 2126; -5 dead-method pin tests). --- core/media_server/__init__.py | 28 +++-- core/media_server/contract.py | 110 ++++++------------ core/media_server/engine.py | 181 ++++++------------------------ core/media_server/types.py | 28 ++--- tests/media_server/test_engine.py | 81 ++----------- 5 files changed, 114 insertions(+), 314 deletions(-) diff --git a/core/media_server/__init__.py b/core/media_server/__init__.py index 3c125ede..0dc6269c 100644 --- a/core/media_server/__init__.py +++ b/core/media_server/__init__.py @@ -1,17 +1,23 @@ -"""Media server engine — central dispatch for the per-server clients -(Plex, Jellyfin, Navidrome, SoulSync standalone). +"""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. The orchestrator -historically had 33+ ``if active_server == 'plex' / 'jellyfin' / -...`` dispatch sites in web_server.py. This package replaces those -with a single ``MediaServerEngine.method()`` call per operation. +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. -Per-server clients keep their protocol-specific work (Plex's -PlexAPI SDK, Jellyfin's REST endpoints, Navidrome's OpenSubsonic -API, SoulSync's filesystem walk). The engine just routes by -``active_server`` config + provides a uniform shape for the calls -``web_server.py`` makes generically. +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. diff --git a/core/media_server/contract.py b/core/media_server/contract.py index 986b4108..d051482a 100644 --- a/core/media_server/contract.py +++ b/core/media_server/contract.py @@ -1,29 +1,25 @@ """Canonical contract for media server clients. -Narrow on purpose. Only declares the methods the orchestrator -dispatches generically across all servers. Server-specific extras -(Plex's `set_music_library_by_name`, Jellyfin's user picker, -Navidrome's music folder filter, SoulSync's filesystem rescan) -stay on the underlying client and are accessed through the -registry's typed accessor — same pattern as the download -plugin contract. +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. -Every required method must be implemented by every registered -client. Optional methods have default no-op implementations so -servers without that capability (e.g. Navidrome's metadata -writeback stubs, SoulSync's playlist sync N/A) don't have to -declare a no-op explicitly. - -The contract is a Protocol (structural typing) rather than an -ABC — existing PlexClient / JellyfinClient / NavidromeClient / +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 calls. This file just makes the implicit -contract explicit. +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, Dict, List, Optional, Protocol, runtime_checkable +from typing import Any, List, Protocol, runtime_checkable @runtime_checkable @@ -31,14 +27,13 @@ class MediaServerClient(Protocol): """Structural contract every media server client must satisfy. ``runtime_checkable`` lets ``isinstance(client, MediaServerClient)`` - work for the conformance test, but it ONLY checks method names — - not signatures. The conformance test in - ``tests/media_server/test_conformance.py`` does the deeper - signature check. + 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 + # Connection / lifecycle — required, every server implements # ------------------------------------------------------------------ def is_connected(self) -> bool: @@ -53,7 +48,7 @@ class MediaServerClient(Protocol): ... # ------------------------------------------------------------------ - # Library reads (required — every server must support these) + # Library reads — required, every server implements # ------------------------------------------------------------------ def get_all_artists(self) -> List[Any]: @@ -68,51 +63,12 @@ class MediaServerClient(Protocol): format is server-native — caller doesn't introspect.""" ... - def search_tracks(self, title: str, artist: str, limit: int = 15) -> List[Any]: - """Search the server's library for tracks matching the title - + artist. Used by playlist sync, download matching, and the - general search UI. Each item is a server-specific TrackInfo - wrapper.""" - ... - - def get_recently_added_albums(self, max_results: int = 400) -> List[Any]: - """Recently-added view — used by the Discover page + - watchlist scanner. Sorted by add timestamp descending.""" - ... - - # ------------------------------------------------------------------ - # Library writes — scan triggers - # ------------------------------------------------------------------ - - def trigger_library_scan(self) -> bool: - """Ask the server to scan its music library. Some servers - (SoulSync standalone) walk the filesystem themselves; some - (Plex / Jellyfin / Navidrome) hit a server-side scan API.""" - ... - - def is_library_scanning(self) -> bool: - """True if a scan is currently running. Polled by the - scan-progress UI.""" - ... - - def get_library_stats(self) -> Dict[str, int]: - """Counts of artists / albums / tracks. Used by the - dashboard system-stats card.""" - ... - # --------------------------------------------------------------------------- -# Required + optional method names — used by the conformance tests to -# check structural conformance without the proxy weight of dataclasses. +# Required method set — pinned by the conformance test. Mirrors the +# Protocol body exactly so static + runtime contracts can't drift. # --------------------------------------------------------------------------- -# Conservative requirement set — only methods every one of the four -# servers actually implements today. Audited by the conformance test. -# Other methods (search_tracks, trigger_library_scan, etc.) exist on -# most servers but not all (e.g. SoulSync has no library scan API -# because it walks the filesystem directly). Phase B's engine -# adapters handle those with per-server fallback rather than forcing -# every client to declare a no-op stub. REQUIRED_METHODS = { 'is_connected', 'ensure_connection', @@ -120,29 +76,35 @@ REQUIRED_METHODS = { 'get_all_album_ids', } -# Methods declared on the protocol but NOT enforced — the -# conformance test does NOT fail if a client lacks one. Engine -# adapters route around the gaps. Listed here so future contributors -# know what the engine expects to find when present. -OPTIONAL_METHODS = { +# 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', - # Playlist sync (Plex / Jellyfin / Navidrome implement; SoulSync no-op) 'create_playlist', 'update_playlist', 'copy_playlist', 'get_all_playlists', 'get_playlist_by_name', - # Analytics 'get_play_history', 'get_track_play_counts', - # Metadata writeback (Plex full support; Jellyfin partial; Navidrome stubs; SoulSync N/A) '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 index 52b96b2a..b8c4f3c3 100644 --- a/core/media_server/engine.py +++ b/core/media_server/engine.py @@ -1,33 +1,40 @@ -"""MediaServerEngine — central dispatch for media server operations. +"""MediaServerEngine — central registry-backed access to media server clients. -Replaces the *uniform-shape* dispatch chains in web_server.py -(``is_connected``, ``get_all_artists``, etc. — anything where every -server returns the same shape and the only branching was on -``active_server == X``). Each such operation is now one -``engine.method()`` call that: +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. -1. Reads the ``server.active`` config to find the current target. -2. Looks up the registered client. -3. Calls the corresponding method (with safe per-server fallbacks - for methods that don't exist on every client — e.g. SoulSync - has no library-scan API). +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 -Server-specific dispatch sites (Plex's raw playlist API, Jellyfin / -Navidrome client methods returning different shapes) stay explicit -in web_server.py per the "lift what's truly shared" standard. They -reach individual clients via ``engine.client(name)`` rather than -the per-server globals — same generic-accessor pattern as the -download orchestrator. +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 itself 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. +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, List, Optional +from typing import Any, Dict, Optional from utils.logging_config import get_logger @@ -92,7 +99,8 @@ class MediaServerEngine: self._resolve_active = active_server_resolver # ------------------------------------------------------------------ - # Direct client access (backward-compat for source-specific reaches) + # Client lookup — generic accessors that replace per-server + # attribute reaches in callers. # ------------------------------------------------------------------ def client(self, name: str) -> Optional[MediaServerClient]: @@ -111,13 +119,12 @@ class MediaServerEngine: """The client for the currently-active server.""" return self.registry.get(self.active_server) - # ------------------------------------------------------------------ - # Cross-server dispatch — required methods (always present) - # ------------------------------------------------------------------ - def is_connected(self) -> bool: """Active server's connection state. False if no active - client (registered but failed to initialize).""" + 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 @@ -127,124 +134,6 @@ class MediaServerEngine: logger.warning("%s is_connected raised: %s", self.active_server, exc) return False - def ensure_connection(self) -> bool: - """Re-auth or reconnect the active server. Returns True if - usable after the call.""" - client = self.active_client() - if client is None: - return False - try: - return client.ensure_connection() - except Exception as exc: - logger.warning("%s ensure_connection raised: %s", self.active_server, exc) - return False - - def get_all_artists(self) -> List[Any]: - """Active server's full artist list. Empty list if not - connected or call fails.""" - client = self.active_client() - if client is None: - return [] - try: - return client.get_all_artists() - except Exception as exc: - logger.warning("%s get_all_artists raised: %s", self.active_server, exc) - return [] - - def get_all_album_ids(self) -> set: - """Active server's album-ID set. Empty set if not connected - or call fails.""" - client = self.active_client() - if client is None: - return set() - try: - return client.get_all_album_ids() - except Exception as exc: - logger.warning("%s get_all_album_ids raised: %s", self.active_server, exc) - return set() - - # ------------------------------------------------------------------ - # Optional methods — engine routes if the client implements them, - # returns a safe default otherwise (mirrors the legacy web_server.py - # branches that special-cased SoulSync / Navidrome). - # ------------------------------------------------------------------ - - def search_tracks(self, title: str, artist: str, limit: int = 15) -> List[Any]: - """Search the active server's library. Returns empty list - for servers that don't implement search_tracks (SoulSync - standalone reads filesystem; no live search API).""" - client = self.active_client() - if client is None or not hasattr(client, 'search_tracks'): - return [] - try: - return client.search_tracks(title, artist, limit) - except Exception as exc: - logger.warning("%s search_tracks raised: %s", self.active_server, exc) - return [] - - def trigger_library_scan(self) -> bool: - """Trigger a server-side library scan. No-op (returns True) - for SoulSync standalone — filesystem walks happen in-process.""" - client = self.active_client() - if client is None: - return False - if not hasattr(client, 'trigger_library_scan'): - return True - try: - return client.trigger_library_scan() - except Exception as exc: - logger.warning("%s trigger_library_scan raised: %s", self.active_server, exc) - return False - - def is_library_scanning(self) -> bool: - """True if the active server is currently scanning. Always - False for SoulSync standalone.""" - client = self.active_client() - if client is None or not hasattr(client, 'is_library_scanning'): - return False - try: - return client.is_library_scanning() - except Exception as exc: - logger.warning("%s is_library_scanning raised: %s", self.active_server, exc) - return False - - def get_library_stats(self) -> Dict[str, int]: - """Counts of artists / albums / tracks. Default empty dict - if the server doesn't implement (SoulSync standalone).""" - client = self.active_client() - if client is None or not hasattr(client, 'get_library_stats'): - return {} - try: - return client.get_library_stats() - except Exception as exc: - logger.warning("%s get_library_stats raised: %s", self.active_server, exc) - return {} - - def get_recently_added_albums(self, max_results: int = 400) -> List[Any]: - """Recently-added albums view. Plex uses a different name; - engine routes to whichever method the active server has.""" - client = self.active_client() - if client is None: - return [] - # Plex uses recentlyAdded() on the music library object, not - # a top-level method. SoulSync, Jellyfin, Navidrome all - # expose get_recently_added_albums directly. - if hasattr(client, 'get_recently_added_albums'): - try: - return client.get_recently_added_albums(max_results) - except Exception as exc: - logger.warning( - "%s get_recently_added_albums raised: %s", - self.active_server, exc, - ) - return [] - return [] - - # ------------------------------------------------------------------ - # Generic accessors — replace per-server attribute reaches in - # callers (Cin's standard from the download refactor). - # ------------------------------------------------------------------ - def configured_clients(self) -> Dict[str, MediaServerClient]: """Return ``{name: client}`` for every server that's both registered AND reports ``is_connected() == True``. Replaces diff --git a/core/media_server/types.py b/core/media_server/types.py index 7d48e1c5..9467d8e1 100644 --- a/core/media_server/types.py +++ b/core/media_server/types.py @@ -1,19 +1,21 @@ """Shared dataclasses for the media-server contract. -Plex / Jellyfin / Navidrome / SoulSync 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. +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. Per-server constructors live -on the unified class as classmethods (``TrackInfo.from_plex_track``, -``TrackInfo.from_jellyfin_dict``, ``TrackInfo.from_navidrome_dict``) -— same pattern Cin used for ``Album.from_spotify_dict`` etc. on the -metadata engine. Heavy server SDK types (``PlexTrack``, dict -shapes) imported under TYPE_CHECKING so this module stays -import-light. +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 diff --git a/tests/media_server/test_engine.py b/tests/media_server/test_engine.py index eb0be123..262fbe1e 100644 --- a/tests/media_server/test_engine.py +++ b/tests/media_server/test_engine.py @@ -107,20 +107,9 @@ def test_is_connected_routes_to_active_client(make_engine): assert engine.is_connected() is True # follows plex -def test_get_all_album_ids_returns_active_clients_set(make_engine): - plex = _FakeClient('plex') - plex._album_ids = {'p-1', 'p-2'} - jelly = _FakeClient('jellyfin') - jelly._album_ids = {'j-1'} - engine = make_engine({'plex': plex, 'jellyfin': jelly}, active='plex') - assert engine.get_all_album_ids() == {'p-1', 'p-2'} - engine = make_engine({'plex': plex, 'jellyfin': jelly}, active='jellyfin') - assert engine.get_all_album_ids() == {'j-1'} - - -def test_engine_returns_safe_defaults_when_active_client_failed_to_init(make_engine): +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 safe defaults instead of raising.""" + None), the engine returns False instead of raising.""" registry = MediaServerRegistry() registry.register(ServerSpec( name='broken', @@ -131,65 +120,18 @@ def test_engine_returns_safe_defaults_when_active_client_failed_to_init(make_eng engine = MediaServerEngine(registry=registry, active_server_resolver=lambda: 'broken') assert engine.is_connected() is False - assert engine.get_all_artists() == [] - assert engine.get_all_album_ids() == set() - assert engine.search_tracks('t', 'a') == [] - assert engine.trigger_library_scan() is False - assert engine.is_library_scanning() is False + assert engine.active_client() is None -def test_engine_swallows_per_method_exceptions(make_engine): - """A method that raises must NOT propagate to the dispatch - site — engine returns the safe default instead, mirroring the - legacy web_server.py defensive try/except chains.""" +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")) - plex.get_all_album_ids = MagicMock(side_effect=RuntimeError("boom")) engine = make_engine({'plex': plex}, active='plex') assert engine.is_connected() is False - assert engine.get_all_album_ids() == set() - - -# --------------------------------------------------------------------------- -# Optional-method dispatch (engine returns safe default when missing) -# --------------------------------------------------------------------------- - - -class _MinimalClient: - """Stand-in for SoulSync standalone — only the required methods, - NO optional methods. Used to assert engine routes around missing - optional methods with safe defaults.""" - - def is_connected(self): return True - def ensure_connection(self): return True - def get_all_artists(self): return [] - def get_all_album_ids(self): return set() - - -def test_search_tracks_returns_empty_when_client_lacks_method(make_engine): - """SoulSync standalone has no search_tracks — engine returns - [] instead of raising AttributeError.""" - engine = make_engine({'soulsync': _MinimalClient()}, active='soulsync') - assert engine.search_tracks('t', 'a') == [] - - -def test_trigger_library_scan_returns_true_when_client_lacks_method(make_engine): - """SoulSync has no trigger_library_scan (filesystem walks - happen in-process). Engine no-ops with True so callers don't - treat it as a failure.""" - engine = make_engine({'soulsync': _MinimalClient()}, active='soulsync') - assert engine.trigger_library_scan() is True - - -def test_is_library_scanning_returns_false_when_client_lacks_method(make_engine): - engine = make_engine({'soulsync': _MinimalClient()}, active='soulsync') - assert engine.is_library_scanning() is False - - -def test_get_library_stats_returns_empty_dict_when_client_lacks_method(make_engine): - engine = make_engine({'soulsync': _MinimalClient()}, active='soulsync') - assert engine.get_library_stats() == {} # --------------------------------------------------------------------------- @@ -306,7 +248,7 @@ def test_engine_with_empty_clients_dict_is_safe_to_use(): 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 method instead of raising.""" + engine still answers safely on every accessor instead of raising.""" registry = MediaServerRegistry() registry.register(ServerSpec( name='plex', factory=lambda: _FakeClient('plex'), display_name='Plex', @@ -320,11 +262,10 @@ def test_engine_with_empty_clients_dict_is_safe_to_use(): # client(name) returns None for every server — engine doesn't crash. assert engine.client('plex') is None assert engine.client('jellyfin') is None - # Cross-server methods return safe defaults instead of raising. + # Active-client lookup + is_connected gracefully handle the empty + # case without raising. + assert engine.active_client() is None assert engine.is_connected() is False - assert engine.get_all_artists() == [] - assert engine.get_all_album_ids() == set() - assert engine.search_tracks('t', 'a') == [] # configured_clients() returns empty dict cleanly. assert engine.configured_clients() == {} From e27ecb84f4262161ce5ef0823a04eaf4b31169c0 Mon Sep 17 00:00:00 2001 From: Broque Thomas <26755000+Nezreka@users.noreply.github.com> Date: Tue, 5 May 2026 22:59:33 -0700 Subject: [PATCH 22/23] =?UTF-8?q?Final=20review-pass=20nits=20=E2=80=94=20?= =?UTF-8?q?class=20docstring,=20dead=20branch,=20dead=20imports,=20boot=20?= =?UTF-8?q?resilience?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Going line-by-line through the engine package + boot wiring. Five small things worth fixing before Cin reads it: (1) MediaServerEngine class docstring still claimed to be a "single entry point for cross-server library operations" — but the prior honesty pass cut all the cross-server dispatch wrappers because they had no callers. Class is really lookup + small accessors now. Docstring rewritten to match. (2) configured_clients() had a dead `not hasattr(client, 'is_connected')` branch. is_connected is in REQUIRED_METHODS so every client the registry yields here implements it. Branch removed; comment notes the reasoning. (3) types.py imported `datetime` and `Dict` but used neither — dead imports dropped. (4) types.py docstring claimed "all four servers" defined an XTrackInfo dataclass. Actually only Plex / Jellyfin / Navidrome did; SoulSync uses richer per-track wrappers. Fixed. (5) web_server.py boot: - media_server_engine added to the chained `= None` declaration so it's always defined before the try/except, defending against the rare path where engine init AND fallback both raise. - Outer engine init failure logger now uses exc_info=True for full traceback (boot-time issues are rare but worth diagnosing). - Nested fallback failure now logs explicitly instead of silently leaving media_server_engine as None. Tests: 2121 still pass. --- core/media_server/engine.py | 21 +++++++++++++++------ core/media_server/types.py | 14 +++++++------- web_server.py | 14 ++++++++------ 3 files changed, 30 insertions(+), 19 deletions(-) diff --git a/core/media_server/engine.py b/core/media_server/engine.py index b8c4f3c3..d65a52ba 100644 --- a/core/media_server/engine.py +++ b/core/media_server/engine.py @@ -45,11 +45,16 @@ logger = get_logger("media_server.engine") class MediaServerEngine: - """Single entry point for cross-server library operations. + """Registry-backed access to the per-server media clients. - The engine knows which server is "active" via the - ``server.active`` config + falls back to direct dispatch for - server-specific calls via ``engine.client(name)``. + 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__( @@ -138,11 +143,15 @@ class MediaServerEngine: """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.""" + 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 not hasattr(client, 'is_connected') or client.is_connected(): + if client.is_connected(): result[name] = client except Exception as exc: logger.warning("%s is_connected raised in configured_clients: %s", name, exc) diff --git a/core/media_server/types.py b/core/media_server/types.py index 9467d8e1..30fdbe09 100644 --- a/core/media_server/types.py +++ b/core/media_server/types.py @@ -21,8 +21,7 @@ so this module stays import-light. from __future__ import annotations from dataclasses import dataclass, field -from datetime import datetime -from typing import TYPE_CHECKING, Any, Dict, List, Optional +from typing import TYPE_CHECKING, List, Optional if TYPE_CHECKING: # plexapi types — only loaded when type-checking; runtime @@ -34,12 +33,13 @@ if TYPE_CHECKING: @dataclass class TrackInfo: - """Canonical track-shape returned by every media server client. + """Canonical track-shape returned by media server clients. - All four servers (Plex, Jellyfin, Navidrome, SoulSync standalone) - used to define their own near-identical ``XTrackInfo`` dataclass. - Lifted to one canonical type here so consumers (matching engine, - sync service, library scanners) get a single import. + 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 diff --git a/web_server.py b/web_server.py index 401f5b2a..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 = 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() @@ -621,18 +621,20 @@ try: set_media_server_engine(media_server_engine) logger.info(" Media server engine initialized") except Exception as e: - logger.error(f" Media server engine 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 was its own + # 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. + # 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: - media_server_engine = None + except Exception as fallback_exc: + logger.error(f" Empty-engine fallback also failed: {fallback_exc}", exc_info=True) try: download_orchestrator = DownloadOrchestrator() From b0dc139b722087a0527186a2486f6228a7a393b6 Mon Sep 17 00:00:00 2001 From: Broque Thomas <26755000+Nezreka@users.noreply.github.com> Date: Wed, 6 May 2026 08:07:44 -0700 Subject: [PATCH 23/23] Sync WHATS_NEW with current engine surface The "Media Server Engine Foundation" entry was written when the engine still had safe-default routing wrappers for optional methods. Those were dropped in the honesty pass. Entry now matches reality: - Lists the actual engine surface (6 methods: client / active_client / active_server / is_connected / configured_clients / reload_config) instead of claiming "uniform safe defaults for optional methods" - References KNOWN_PER_SERVER_METHODS as the data-only listing (replaces the old OPTIONAL_METHODS naming) - Cites real test counts (42 total) instead of the stale 35 - Drops the "33+ dispatch sites" overclaim (was already partial); the actual framing is "uniform-shape chains lifted, ~18 server-specific chains stay explicit per the lift-what's-truly-shared standard" --- webui/static/helper.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/webui/static/helper.js b/webui/static/helper.js index b9a86990..607ba6c8 100644 --- a/webui/static/helper.js +++ b/webui/static/helper.js @@ -3433,7 +3433,7 @@ const WHATS_NEW = { // --- 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). web_server.py historically had ~33 `if active_server == "plex" / "jellyfin" / ...` dispatch sites. new `core/media_server/` package provides `MediaServerEngine` that reads `server.active` config + routes to the right client. plugin contract narrowly requires only the four methods every server actually implements (is_connected, ensure_connection, get_all_artists, get_all_album_ids); optional methods (search_tracks, trigger_library_scan, get_library_stats, etc.) are routed with safe defaults so SoulSync standalone (no library scan API since it walks the filesystem directly) doesn\'t need stub methods. lifted the four uniform `is_connected` dispatches into `engine.is_connected()`. honest scope: most "dispatch sites" the recon counted are genuinely different per server (playlist track replace, per-server metadata sync, deep scan with server-specific cache strategies) — those stay explicit per the "lift what\'s truly shared" standard. 35 new tests pin: per-server observable behavior (4 server pinning files, 21 tests), engine cross-server dispatch (10 tests), structural conformance (4 tests). engine reference + plugin contract available for future targeted refactors. zero behavior change for users.' }, + { 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.' },