Final review-pass nits — class docstring, dead branch, dead imports, boot resilience

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.
pull/497/head
Broque Thomas 3 weeks ago
parent 6489244bcc
commit e27ecb84f4

@ -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)

@ -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

@ -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()

Loading…
Cancel
Save