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