From 6a75d656fa44763edf8caa04937f954bfb4a116f Mon Sep 17 00:00:00 2001 From: Broque Thomas <26755000+Nezreka@users.noreply.github.com> Date: Mon, 4 May 2026 22:35:45 -0700 Subject: [PATCH] Cin-2: Generic accessors on orchestrator + singleton factory MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Cin's review feedback: external callers reach per-source clients via attribute access (orch.hifi.reload_instances()) — needs generic accessors so the registry IS the single source of truth. Adds: - orch.client(name) — public accessor for a per-source client. Resolves canonical names (deezer) AND legacy aliases (deezer_dl). - orch.configured_clients() — returns {name: client} for every initialized AND is_configured() == True source. Replaces the 6+ if/hasattr/is_configured chain Cin called out: if hasattr(orch, 'soulseek') and orch.soulseek and \ orch.soulseek.is_configured(): ... - orch.reload_instances(source=None) — generic dispatch for source-specific reload calls. Replaces orch.hifi.reload_instances() with orch.reload_instances('hifi'). - get_download_orchestrator() / set_download_orchestrator() singleton factory matching Cin's get_metadata_engine pattern in PR #498. web_server.py can install the orchestrator it builds at boot so future callers grab via the factory instead of importing the legacy `soulseek_client` global. Phase Cin-3/Cin-4 will replace existing call sites; this commit just provides the surface so those migrations are mechanical. Suite still green (335 download tests + 6 new generic-accessor tests). --- core/download_orchestrator.py | 89 ++++++++++++++- tests/downloads/test_download_orchestrator.py | 105 ++++++++++++++++++ 2 files changed, 190 insertions(+), 4 deletions(-) diff --git a/core/download_orchestrator.py b/core/download_orchestrator.py index a8b5d6d0..d8aeba78 100644 --- a/core/download_orchestrator.py +++ b/core/download_orchestrator.py @@ -133,12 +133,62 @@ class DownloadOrchestrator: logger.info(f"Download Orchestrator settings reloaded - Mode: {self.mode}") - def _client(self, name): - """Get a client by name, returning None if not initialized. - Resolves both canonical names (``deezer``) and legacy aliases - (``deezer_dl``) via the registry.""" + def client(self, name): + """Generic accessor for a download source client by name. + + Cin's review feedback: external callers should reach into + per-source clients via this method (``orch.client('hifi')``) + instead of attribute access (``orch.hifi``). Resolves both + canonical names (``deezer``) and legacy aliases (``deezer_dl``) + via the registry. Returns None if the source isn't registered + or failed to initialize. + """ return self.registry.get(name) + # Internal alias kept for legacy callers inside this file. + _client = client + + def configured_clients(self) -> dict: + """Return ``{source_name: client}`` for every download source + that's both initialized AND reports is_configured() == True. + + Replaces the legacy per-source iteration pattern Cin called + out — `if hasattr(orch, 'soulseek') and orch.soulseek and + orch.soulseek.is_configured(): download_clients['soulseek'] + = orch.soulseek` repeated for each source. + """ + result = {} + for name, client in self.registry.all_plugins(): + try: + if not hasattr(client, 'is_configured') or client.is_configured(): + result[name] = client + except Exception as exc: + logger.debug("%s is_configured raised: %s", name, exc) + return result + + def reload_instances(self, source: str = None) -> bool: + """Reload a source's instance config (e.g. HiFi instance list, + Qobuz session restore). Generic dispatch — caller passes the + source name instead of reaching for ``orch.hifi.reload_instances()``. + + When ``source`` is None, reloads every source that has a + ``reload_instances`` method. + """ + sources = [source] if source else list(self.registry.names()) + ok = True + for name in sources: + client = self.client(name) + if client is None: + continue + if not hasattr(client, 'reload_instances'): + continue + try: + client.reload_instances() + except Exception as exc: + logger.warning("%s reload_instances failed: %s", name, exc) + ok = False + return ok + def is_configured(self) -> bool: """ Check if orchestrator is configured and ready to use. @@ -470,3 +520,34 @@ class DownloadOrchestrator: except Exception: ok = False return ok + + +# --------------------------------------------------------------------------- +# Singleton accessor — mirrors Cin's metadata engine pattern +# (``get_metadata_engine()``). Callers that don't need a custom +# registry use this instead of instantiating DownloadOrchestrator +# directly. Currently web_server.py constructs the singleton at +# startup and exposes it via the legacy ``soulseek_client`` global; +# this factory exists for new callers + future migration of that +# global to a more honestly-named ``download_orchestrator``. +# --------------------------------------------------------------------------- + +_default_orchestrator: Optional['DownloadOrchestrator'] = None + + +def get_download_orchestrator() -> 'DownloadOrchestrator': + """Return (lazily creating) the process-wide DownloadOrchestrator + singleton. Mirrors the ``get_metadata_engine()`` pattern Cin used + for the metadata engine refactor.""" + global _default_orchestrator + if _default_orchestrator is None: + _default_orchestrator = DownloadOrchestrator() + return _default_orchestrator + + +def set_download_orchestrator(orchestrator: 'DownloadOrchestrator') -> None: + """Set the process-wide singleton. Used by web_server.py at boot + to install the orchestrator it constructs as the default for + callers that grab via ``get_download_orchestrator()``.""" + global _default_orchestrator + _default_orchestrator = orchestrator diff --git a/tests/downloads/test_download_orchestrator.py b/tests/downloads/test_download_orchestrator.py index f0467c06..79c25f21 100644 --- a/tests/downloads/test_download_orchestrator.py +++ b/tests/downloads/test_download_orchestrator.py @@ -100,3 +100,108 @@ def test_clear_all_completed_downloads_propagates_configured_failures(): assert result is False assert orch.soulseek.clear_calls == 1 + + +# --------------------------------------------------------------------------- +# Cin-2 generic accessors +# --------------------------------------------------------------------------- + + +def test_client_returns_registered_client_by_name(): + """Cin's review feedback: orch.client('hifi') is the canonical + way to reach a per-source client, replacing orch.hifi attribute + access.""" + soulseek = _FakeClient() + youtube = _FakeClient() + orch = _build_orchestrator(soulseek=soulseek, youtube=youtube) + + assert orch.client('soulseek') is soulseek + assert orch.client('youtube') is youtube + assert orch.client('made_up') is None + + +def test_configured_clients_excludes_unconfigured_sources(): + """Replaces the legacy iteration pattern: 6+ if/hasattr/is_configured + checks per source. Single call returns dict of configured clients.""" + configured = _FakeClient(configured=True) + unconfigured = _FakeClient(configured=False) + orch = _build_orchestrator( + soulseek=configured, + youtube=unconfigured, + ) + result = orch.configured_clients() + assert 'soulseek' in result + assert 'youtube' not in result + assert result['soulseek'] is configured + + +def test_reload_instances_dispatches_to_named_source(): + """Generic dispatch — caller passes source name instead of + reaching for orch.hifi.reload_instances() directly.""" + + class _ReloadableClient(_FakeClient): + def __init__(self): + super().__init__(configured=True) + self.reload_called = False + + def reload_instances(self): + self.reload_called = True + + hifi = _ReloadableClient() + soulseek = _FakeClient() # No reload_instances method + orch = _build_orchestrator(soulseek=soulseek, hifi=hifi) + + assert orch.reload_instances('hifi') is True + assert hifi.reload_called is True + + +def test_reload_instances_skips_clients_without_method(): + """Sources that don't expose reload_instances are skipped, not + treated as failures.""" + soulseek = _FakeClient() # No reload_instances method + orch = _build_orchestrator(soulseek=soulseek) + # Calling on a source without the method = silent no-op + assert orch.reload_instances('soulseek') is True + + +def test_reload_instances_with_no_args_reloads_every_source(): + """When called with no source argument, hits every registered + source that exposes reload_instances.""" + + class _ReloadableClient(_FakeClient): + def __init__(self): + super().__init__() + self.reload_called = False + + def reload_instances(self): + self.reload_called = True + + a = _ReloadableClient() + b = _ReloadableClient() + orch = _build_orchestrator(soulseek=a, hifi=b) + + orch.reload_instances() + assert a.reload_called is True + assert b.reload_called is True + + +# --------------------------------------------------------------------------- +# Singleton factory (matches Cin's get_metadata_engine pattern) +# --------------------------------------------------------------------------- + + +def test_get_download_orchestrator_returns_set_singleton(): + """When set_download_orchestrator has been called (web_server.py + does this at boot), get_download_orchestrator returns the + installed instance instead of building a fresh one.""" + from core.download_orchestrator import ( + get_download_orchestrator, + set_download_orchestrator, + ) + + orch = _build_orchestrator(soulseek=_FakeClient()) + set_download_orchestrator(orch) + try: + assert get_download_orchestrator() is orch + finally: + set_download_orchestrator(None)