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] =?UTF-8?q?Final=20review-pass=20nits=20=E2=80=94=20class?= =?UTF-8?q?=20docstring,=20dead=20branch,=20dead=20imports,=20boot=20resil?= =?UTF-8?q?ience?= 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 | 23 ++++++++++++++++------- core/media_server/types.py | 14 +++++++------- web_server.py | 14 ++++++++------ 3 files changed, 31 insertions(+), 20 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. - - 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)``. + """Registry-backed access to the per-server media clients. + + Owns the per-server client instances + a small set of generic + accessors (``client(name)`` / ``active_client()`` / + ``configured_clients()`` / ``reload_config(name)``) so call sites + don't reach for separate per-server globals. The one cross-server + dispatch wrapper kept on the engine — ``is_connected()`` — + backs the dashboard status indicators that have multiple call + sites; everything else dispatches per-server in the call site + itself, reaching the relevant client through ``engine.client(name)``. """ def __init__( @@ -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()