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.
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).
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.
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.
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.
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.
`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.
