diff --git a/core/download_engine/engine.py b/core/download_engine/engine.py index 358ef07e..59bd31c3 100644 --- a/core/download_engine/engine.py +++ b/core/download_engine/engine.py @@ -65,11 +65,15 @@ class DownloadEngine: # holding the lock for its own update. self._records: Dict[Tuple[str, str], DownloadRecord] = {} # Plugins that have registered with the engine. Source name - # → plugin instance. The engine itself doesn't use plugins - # until later phases, but holding the references here keeps - # plugin lookup local to the engine instead of forcing every - # caller to also touch the registry. + # → plugin instance. self._plugins: Dict[str, Any] = {} + # Alias → canonical-name map. Lets engine resolve legacy + # source-name strings (e.g. ``'deezer_dl'`` for Deezer) to + # the canonical key in ``_plugins``. Cin's review caught + # that engine.cancel_download(source_hint='deezer_dl') + # silently fell through to Soulseek because alias resolution + # only existed at the registry, not on the engine. + self._aliases: Dict[str, str] = {} # Background download worker — lives on the engine because # it owns the cross-source state the worker mutates. Lazy # import keeps the engine module standalone. @@ -80,11 +84,17 @@ class DownloadEngine: # Plugin registration # ------------------------------------------------------------------ - def register_plugin(self, source_name: str, plugin: Any) -> None: + def register_plugin(self, source_name: str, plugin: Any, + aliases: Tuple[str, ...] = ()) -> None: """Register a plugin under its canonical source name. Called once per source by the orchestrator after the registry's ``initialize`` builds the client instances. + ``aliases`` is the list of legacy source-name strings that + should resolve to this plugin (e.g. ``'deezer_dl'`` for + Deezer). Without alias resolution the engine couldn't route + cancel/lookup calls that came in with the legacy name. + If the plugin exposes ``set_engine(engine)``, the engine passes a self-reference so the plugin can dispatch into ``engine.worker`` / read state / etc. Plugins that haven't @@ -101,6 +111,8 @@ class DownloadEngine: if source_name in self._plugins: logger.warning("Plugin %s already registered with engine — overwriting", source_name) self._plugins[source_name] = plugin + for alias in aliases: + self._aliases[alias] = source_name # Apply the plugin's rate-limit policy BEFORE set_engine so # set_engine callbacks can override per-source if they need @@ -120,7 +132,23 @@ class DownloadEngine: ) def get_plugin(self, source_name: str) -> Optional[Any]: - return self._plugins.get(source_name) + """Return the plugin instance for the given source name. + Resolves through aliases — e.g. ``get_plugin('deezer_dl')`` + returns the same instance as ``get_plugin('deezer')``.""" + if source_name in self._plugins: + return self._plugins[source_name] + canonical = self._aliases.get(source_name) + if canonical: + return self._plugins.get(canonical) + return None + + def _resolve_canonical(self, source_name: str) -> Optional[str]: + """Return the canonical source name for an input that may be + an alias. Returns None if the input matches neither a + canonical name nor an alias.""" + if source_name in self._plugins: + return source_name + return self._aliases.get(source_name) def registered_sources(self) -> List[str]: return list(self._plugins.keys()) @@ -148,6 +176,29 @@ class DownloadEngine: return existing.update(patch) + def update_record_unless_state(self, source_name: str, download_id: str, + patch: DownloadRecord, + skip_if_state_in: Tuple[str, ...] = ()) -> bool: + """Atomically check the record's state and apply ``patch`` only + if the current state is NOT in ``skip_if_state_in``. Returns + True if the patch was applied, False if it was skipped (or + the record didn't exist). + + Used by the background download worker's ``_mark_terminal`` + to avoid the read-then-write race Cin flagged: a cancel + landing between the snapshot and update could be overwritten + back to Errored / Completed. Holding ``state_lock`` across + the check + write closes the window. + """ + with self.state_lock: + existing = self._records.get((source_name, download_id)) + if existing is None: + return False + if existing.get('state') in skip_if_state_in: + return False + existing.update(patch) + return True + def remove_record(self, source_name: str, download_id: str) -> Optional[DownloadRecord]: """Delete a record (cancellation cleanup). Returns the removed record or None if not found.""" @@ -226,23 +277,31 @@ class DownloadEngine: source_hint: Optional[str] = None, remove: bool = False) -> bool: """Cancel a download. ``source_hint`` is the source name (or - legacy username string like ``'deezer_dl'``) — when provided, - routes directly to that plugin. When omitted, every plugin - is asked in turn until one accepts the cancel.""" + legacy alias like ``'deezer_dl'``, or a real Soulseek peer + username) — when provided, routes directly to that plugin. + When omitted, every plugin is asked in turn until one accepts. + + Cin's review caught a bug here: legacy alias strings like + ``'deezer_dl'`` weren't resolved to the canonical ``'deezer'`` + plugin name, so the cancel silently fell through to Soulseek. + Resolution now goes through ``_resolve_canonical`` first. + """ # Direct routing when the caller knows the source. if source_hint: - # Streaming source names ARE the username. Soulseek - # uses a real peer username (anything not in our plugin - # registry), so route those to the soulseek plugin. - target_plugin = self._plugins.get(source_hint) - if target_plugin is not None and source_hint != 'soulseek': - try: - return await target_plugin.cancel_download( - download_id, source_hint, remove, - ) - except Exception as exc: - logger.debug("%s cancel_download failed: %s", source_hint, exc) - return False + canonical = self._resolve_canonical(source_hint) + # Streaming source names (or aliases) resolve to a + # registered plugin. Anything else (real Soulseek peer + # name not in our registry) routes to Soulseek. + if canonical and canonical != 'soulseek': + target_plugin = self._plugins.get(canonical) + if target_plugin is not None: + try: + return await target_plugin.cancel_download( + download_id, source_hint, remove, + ) + except Exception as exc: + logger.debug("%s cancel_download failed: %s", canonical, exc) + return False soulseek = self._plugins.get('soulseek') if soulseek is not None: try: diff --git a/core/download_engine/worker.py b/core/download_engine/worker.py index 8ba48a2a..77503e14 100644 --- a/core/download_engine/worker.py +++ b/core/download_engine/worker.py @@ -256,11 +256,18 @@ class BackgroundDownloadWorker: self._last_download_at[source_name] = time.time() if file_path: - self._engine.update_record(source_name, download_id, { - 'state': 'Completed, Succeeded', - 'progress': 100.0, - 'file_path': file_path, - }) + # Atomic write — preserve Cancelled if user cancelled + # between impl returning and this write. Same guard + # _mark_terminal uses; Cin flagged both split sites. + self._engine.update_record_unless_state( + source_name, download_id, + { + 'state': 'Completed, Succeeded', + 'progress': 100.0, + 'file_path': file_path, + }, + skip_if_state_in=('Cancelled',), + ) logger.info( "%s download %s completed: %s", source_name, download_id, file_path, @@ -291,15 +298,18 @@ class BackgroundDownloadWorker: 'Cancelled' state set by the user via cancel_download. Mirrors the legacy per-client guard (``if state != 'Cancelled': state = 'Errored'``) every - client used to hand-roll inside its thread worker.""" - record = self._engine.get_record(source_name, download_id) - if record is None: - return # already removed (e.g. cancel + remove) - if record.get('state') == 'Cancelled': - return # user explicitly cancelled — leave it + client used to hand-roll inside its thread worker. + + Uses ``update_record_unless_state`` so the check + write are + atomic under the engine's state_lock. Cin caught a race + where a cancel landing between the read-snapshot + write + could overwrite Cancelled back to Errored / Completed. + """ patch: Dict[str, Any] = { 'state': 'Completed, Succeeded' if success else 'Errored', } if error is not None: patch['error'] = error - self._engine.update_record(source_name, download_id, patch) + self._engine.update_record_unless_state( + source_name, download_id, patch, skip_if_state_in=('Cancelled',), + ) diff --git a/core/download_orchestrator.py b/core/download_orchestrator.py index d8aeba78..8cdb3148 100644 --- a/core/download_orchestrator.py +++ b/core/download_orchestrator.py @@ -76,7 +76,9 @@ class DownloadOrchestrator: # can route through it without further orchestrator changes. self.engine = engine if engine is not None else DownloadEngine() for source_name, plugin in self.registry.all_plugins(): - self.engine.register_plugin(source_name, plugin) + spec = self.registry.get_spec(source_name) + aliases = spec.aliases if spec else () + self.engine.register_plugin(source_name, plugin, aliases=aliases) if self._init_failures: logger.warning(f"Download clients failed to initialize: {', '.join(self._init_failures)}") @@ -246,17 +248,42 @@ class DownloadOrchestrator: return False + def _normalize_source_name(self, name: str) -> Optional[str]: + """Convert a possibly-aliased source name (e.g. legacy + ``'deezer_dl'``) to the canonical registry name (``'deezer'``). + Returns None if the input matches neither a canonical name + nor an alias. + + Cin's review caught a bug where legacy alias values from + config (hybrid_order containing ``'deezer_dl'``) silently + dropped Deezer from hybrid mode because the canonical-name + membership check rejected the alias. + """ + spec = self.registry.get_spec(name) if name else None + return spec.name if spec else None + def _resolve_source_chain(self) -> List[str]: """Order the configured sources for hybrid mode. Prefers ``hybrid_order`` config; falls back to legacy - primary/secondary pair when no order set.""" - registry_names = set(self.registry.names()) + primary/secondary pair when no order set. Normalizes alias + names through the registry so legacy ``deezer_dl`` config + values resolve correctly to the canonical ``deezer`` plugin.""" if self.hybrid_order: - return [s for s in self.hybrid_order if s in registry_names] - primary = self.hybrid_primary if self.hybrid_primary in registry_names else 'soulseek' - secondary = self.hybrid_secondary if self.hybrid_secondary in registry_names else 'soulseek' + chain = [] + seen = set() + for raw in self.hybrid_order: + canonical = self._normalize_source_name(raw) + if canonical and canonical not in seen: + chain.append(canonical) + seen.add(canonical) + return chain + primary = self._normalize_source_name(self.hybrid_primary) or 'soulseek' + secondary = self._normalize_source_name(self.hybrid_secondary) or 'soulseek' if secondary == primary: - secondary = next((name for name in registry_names if name != primary), 'soulseek') + secondary = next( + (name for name in self.registry.names() if name != primary), + 'soulseek', + ) chain = [primary, secondary] if not chain: chain = ['soulseek'] diff --git a/tests/downloads/test_background_download_worker.py b/tests/downloads/test_background_download_worker.py index 70335d79..0fd6dd1b 100644 --- a/tests/downloads/test_background_download_worker.py +++ b/tests/downloads/test_background_download_worker.py @@ -190,6 +190,40 @@ def test_worker_preserves_cancelled_when_impl_returns_none(): ) +def test_worker_preserves_cancelled_when_impl_returns_success(): + """Cin's bug 3 follow-up: the success path also has a read-then-write + race. If the user cancels between the impl returning a valid file + path and the worker writing 'Completed, Succeeded', the cancel is + overwritten. The success-path write must use the same atomic + Cancelled-preserve guard as _mark_terminal.""" + engine = DownloadEngine() + + def impl(download_id, target_id, display_name): + # User cancels mid-impl, then impl finishes successfully. + engine.update_record('youtube', download_id, {'state': 'Cancelled'}) + return '/tmp/file.flac' + + download_id = engine.worker.dispatch( + source_name='youtube', + target_id='vid', + display_name='X', + original_filename='vid||X', + impl_callable=impl, + ) + + deadline = time.time() + 2.0 + while time.time() < deadline: + record = engine.get_record('youtube', download_id) + if record and record['state'] in ('Cancelled', 'Completed, Succeeded'): + break + time.sleep(0.01) + + record = engine.get_record('youtube', download_id) + assert record['state'] == 'Cancelled', ( + f"Worker clobbered user's Cancelled with {record['state']}" + ) + + def test_worker_preserves_cancelled_when_impl_raises(): """Same Cancelled-preserve guard, but for the impl-raises path.""" engine = DownloadEngine() diff --git a/tests/downloads/test_download_engine.py b/tests/downloads/test_download_engine.py index 1845f085..853ce88e 100644 --- a/tests/downloads/test_download_engine.py +++ b/tests/downloads/test_download_engine.py @@ -513,3 +513,80 @@ def test_download_with_fallback_continues_past_exception(): 'youtube', 'v||t', 0, ['youtube', 'tidal'], )) assert result == 'td-id' + + +# --------------------------------------------------------------------------- +# Cin bug 1: alias resolution on cancel_download +# --------------------------------------------------------------------------- + + +def test_register_plugin_records_aliases(): + """Aliases passed to register_plugin resolve to the canonical plugin + via get_plugin. Cin caught engine.cancel_download routing 'deezer_dl' + to soulseek because the alias never made it to the engine.""" + engine = DownloadEngine() + deezer = _FakePlugin('deezer') + engine.register_plugin('deezer', deezer, aliases=('deezer_dl',)) + + assert engine.get_plugin('deezer') is deezer + assert engine.get_plugin('deezer_dl') is deezer + + +def test_cancel_download_resolves_alias_to_canonical_plugin(): + """The legacy 'deezer_dl' source_hint must route to the deezer + plugin, not fall through to soulseek. This was Cin's bug 1 — + cancel of a Deezer download silently no-op'd.""" + engine = DownloadEngine() + soulseek = _FakePlugin('soulseek') + deezer = _FakePlugin('deezer') + engine.register_plugin('soulseek', soulseek) + engine.register_plugin('deezer', deezer, aliases=('deezer_dl',)) + + _run_async(engine.cancel_download('dl-1', 'deezer_dl', remove=False)) + assert deezer.cancel_calls == [('dl-1', 'deezer_dl', False)] + assert soulseek.cancel_calls == [] + + +# --------------------------------------------------------------------------- +# Cin bug 3: atomic update_record_unless_state +# --------------------------------------------------------------------------- + + +def test_update_record_unless_state_applies_when_state_not_blocked(): + engine = DownloadEngine() + engine.add_record('youtube', 'dl-1', {'state': 'InProgress, Downloading'}) + + applied = engine.update_record_unless_state( + 'youtube', 'dl-1', + {'state': 'Completed, Succeeded', 'progress': 100.0}, + skip_if_state_in=('Cancelled',), + ) + assert applied is True + assert engine.get_record('youtube', 'dl-1')['state'] == 'Completed, Succeeded' + assert engine.get_record('youtube', 'dl-1')['progress'] == 100.0 + + +def test_update_record_unless_state_skips_when_state_blocked(): + """A worker-thread terminal write must NOT clobber a Cancelled + state set by the user. Returns False so caller knows the patch + was skipped.""" + engine = DownloadEngine() + engine.add_record('youtube', 'dl-1', {'state': 'Cancelled'}) + + applied = engine.update_record_unless_state( + 'youtube', 'dl-1', + {'state': 'Completed, Succeeded'}, + skip_if_state_in=('Cancelled',), + ) + assert applied is False + assert engine.get_record('youtube', 'dl-1')['state'] == 'Cancelled' + + +def test_update_record_unless_state_returns_false_for_missing_record(): + engine = DownloadEngine() + applied = engine.update_record_unless_state( + 'youtube', 'never-existed', + {'state': 'Completed, Succeeded'}, + skip_if_state_in=('Cancelled',), + ) + assert applied is False diff --git a/tests/downloads/test_download_orchestrator.py b/tests/downloads/test_download_orchestrator.py index 79c25f21..6902f80c 100644 --- a/tests/downloads/test_download_orchestrator.py +++ b/tests/downloads/test_download_orchestrator.py @@ -190,6 +190,53 @@ def test_reload_instances_with_no_args_reloads_every_source(): # --------------------------------------------------------------------------- +# --------------------------------------------------------------------------- +# Cin bug 2: hybrid_order alias normalization +# --------------------------------------------------------------------------- + + +def test_resolve_source_chain_normalizes_legacy_aliases(): + """Cin's bug 2: hybrid_order config containing the legacy alias + 'deezer_dl' was silently dropped because the canonical-name + membership check rejected it. Orchestrator must normalize via + the registry alias map first.""" + orch = _build_orchestrator( + soulseek=_FakeClient(), + deezer_dl=_FakeClient(), + youtube=_FakeClient(), + ) + orch.hybrid_order = ['deezer_dl', 'soulseek', 'youtube'] + orch.hybrid_primary = None + orch.hybrid_secondary = None + + chain = orch._resolve_source_chain() + assert chain == ['deezer', 'soulseek', 'youtube'] + + +def test_resolve_source_chain_dedupes_alias_and_canonical(): + """If both 'deezer' and 'deezer_dl' appear, dedupe to single entry.""" + orch = _build_orchestrator( + soulseek=_FakeClient(), + deezer_dl=_FakeClient(), + ) + orch.hybrid_order = ['deezer_dl', 'deezer', 'soulseek'] + orch.hybrid_primary = None + orch.hybrid_secondary = None + + chain = orch._resolve_source_chain() + assert chain == ['deezer', 'soulseek'] + + +def test_resolve_source_chain_drops_unknown_names(): + orch = _build_orchestrator(soulseek=_FakeClient(), youtube=_FakeClient()) + orch.hybrid_order = ['nonsense', 'soulseek', 'also_fake', 'youtube'] + orch.hybrid_primary = None + orch.hybrid_secondary = None + + chain = orch._resolve_source_chain() + assert chain == ['soulseek', 'youtube'] + + 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 diff --git a/web_server.py b/web_server.py index 7034281c..b341a550 100644 --- a/web_server.py +++ b/web_server.py @@ -627,23 +627,17 @@ try: except Exception as e: logger.error(f" Playlist sync service failed to initialize: {e}") -# Inject shutdown check callback into YouTube and Tidal clients (avoids circular imports) -if soulseek_client: - if hasattr(soulseek_client, 'youtube'): - soulseek_client.youtube.set_shutdown_check(lambda: IS_SHUTTING_DOWN) - logger.info(" Configured YouTube client shutdown callback") - if hasattr(soulseek_client, 'tidal'): - soulseek_client.tidal.set_shutdown_check(lambda: IS_SHUTTING_DOWN) - logger.info(" Configured Tidal download client shutdown callback") - if hasattr(soulseek_client, 'qobuz'): - soulseek_client.qobuz.set_shutdown_check(lambda: IS_SHUTTING_DOWN) - logger.info(" Configured Qobuz client shutdown callback") - if hasattr(soulseek_client, 'hifi'): - soulseek_client.hifi.set_shutdown_check(lambda: IS_SHUTTING_DOWN) - logger.info(" Configured HiFi client shutdown callback") - if hasattr(soulseek_client, 'soundcloud') and soulseek_client.soundcloud: - soulseek_client.soundcloud.set_shutdown_check(lambda: IS_SHUTTING_DOWN) - logger.info(" Configured SoundCloud client shutdown callback") +# Inject shutdown check callback into every download source that +# accepts one. Generic dispatch via the registry — no per-source +# attribute reaches needed. +if soulseek_client and hasattr(soulseek_client, 'registry'): + for _src_name, _src_client in soulseek_client.registry.all_plugins(): + if _src_client is not None and hasattr(_src_client, 'set_shutdown_check'): + try: + _src_client.set_shutdown_check(lambda: IS_SHUTTING_DOWN) + logger.info(" Configured %s client shutdown callback", _src_name) + except Exception as _exc: + logger.warning(" %s set_shutdown_check failed: %s", _src_name, _exc) # Initialize web scan manager for automatic post-download scanning try: @@ -2459,7 +2453,7 @@ def get_cached_transfer_data(): # First, get Soulseek downloads from API transfers_data = None - if not soulseek_known_down and soulseek_client and getattr(soulseek_client, 'soulseek', None) and soulseek_client.soulseek.base_url: + if not soulseek_known_down and soulseek_client and getattr(soulseek_client, 'soulseek', None) and soulseek_client.client("soulseek").base_url: transfers_data = run_async(soulseek_client._make_request('GET', 'transfers/downloads')) if transfers_data: all_transfers = [] @@ -2481,14 +2475,13 @@ def get_cached_transfer_data(): # stays at 0 even when the underlying client knows the real percent. try: all_downloads = [] - for _dl_client in [soulseek_client.youtube, soulseek_client.tidal, soulseek_client.qobuz, - soulseek_client.hifi, soulseek_client.deezer_dl, soulseek_client.lidarr, - getattr(soulseek_client, 'soundcloud', None)]: - if _dl_client: - try: - all_downloads.extend(run_async(_dl_client.get_all_downloads())) - except Exception: - pass + # Generic dispatch — engine returns active downloads + # across every plugin in one call, no per-source iteration. + if soulseek_client and hasattr(soulseek_client, 'engine'): + try: + all_downloads = run_async(soulseek_client.engine.get_all_downloads()) + except Exception: + pass for download in all_downloads: key = _make_context_key(download.username, download.filename) # Convert DownloadStatus to transfer dict format @@ -4152,7 +4145,7 @@ def handle_settings(): soulseek_client.reload_settings() # Reload YouTube client settings (rate limiting, cookies) if hasattr(soulseek_client, 'youtube'): - soulseek_client.youtube.reload_settings() + soulseek_client.client("youtube").reload_settings() # FIX: Re-instantiate the global tidal_client to pick up new settings try: tidal_client = TidalClient() @@ -6816,7 +6809,7 @@ def enhanced_search_source(source_name): When the requested source's client isn't available (Spotify unauthed, Discogs missing token, Hydrabase disconnected, MusicBrainz import - failure, soulseek_client.youtube missing), returns plain JSON + failure, soulseek_client.client("youtube") missing), returns plain JSON `{"artists":[],"albums":[],"tracks":[],"available":false}` to match the original endpoint contract. """ @@ -7040,7 +7033,7 @@ def download_music_video(): def _progress(pct): _music_video_downloads[video_id]['progress'] = round(pct, 1) - final_path = soulseek_client.youtube.download_music_video(video_url, output_path, progress_callback=_progress) + final_path = soulseek_client.client("youtube").download_music_video(video_url, output_path, progress_callback=_progress) if final_path and os.path.exists(final_path): _music_video_downloads[video_id]['status'] = 'completed' @@ -14155,7 +14148,7 @@ def _enhance_file_metadata(file_path: str, context: dict, artist: dict, album_in genius_worker=genius_worker, spotify_enrichment_worker=spotify_enrichment_worker, itunes_enrichment_worker=itunes_enrichment_worker, - hifi_client=soulseek_client.hifi if soulseek_client else None, + hifi_client=soulseek_client.client("hifi") if soulseek_client else None, ), ) @@ -14260,7 +14253,7 @@ def _post_process_matched_download_with_verification(context_key, context, file_ genius_worker=genius_worker, spotify_enrichment_worker=spotify_enrichment_worker, itunes_enrichment_worker=itunes_enrichment_worker, - hifi_client=soulseek_client.hifi if soulseek_client else None, + hifi_client=soulseek_client.client("hifi") if soulseek_client else None, ), ) @@ -14384,7 +14377,7 @@ def _post_process_matched_download(context_key, context, file_path): genius_worker=genius_worker, spotify_enrichment_worker=spotify_enrichment_worker, itunes_enrichment_worker=itunes_enrichment_worker, - hifi_client=soulseek_client.hifi if soulseek_client else None, + hifi_client=soulseek_client.client("hifi") if soulseek_client else None, ), ) @@ -17084,7 +17077,7 @@ def _try_source_reuse(task_id, batch_id, track): # Sort by confidence, filter by quality preference candidates.sort(key=lambda c: c.confidence, reverse=True) _sr.info(f"Found {len(candidates)} candidates above 0.70, best={candidates[0].confidence:.3f} ({candidates[0].filename})") - slsk = soulseek_client.soulseek if hasattr(soulseek_client, 'soulseek') else soulseek_client + slsk = soulseek_client.client("soulseek") if hasattr(soulseek_client, 'soulseek') else soulseek_client filtered = slsk.filter_results_by_quality_preference(candidates) if not filtered: _sr.info(f"Quality filter rejected all candidates for task {task_id}") @@ -17164,7 +17157,7 @@ def _store_batch_source(batch_id, username, filename): try: # Access SoulseekClient directly (soulseek_client is DownloadOrchestrator) - slsk = soulseek_client.soulseek if hasattr(soulseek_client, 'soulseek') else soulseek_client + slsk = soulseek_client.client("soulseek") if hasattr(soulseek_client, 'soulseek') else soulseek_client _sr.info(f"Browsing {username}:{folder_path}...") files = run_async(slsk.browse_user_directory(username, folder_path)) if not files: @@ -19732,7 +19725,7 @@ def get_discover_album(source, album_id): def hifi_status(): """Check if HiFi API instances are reachable.""" try: - hifi = soulseek_client.hifi + hifi = soulseek_client.client("hifi") available = hifi.is_available() version = hifi.get_version() if available else None return jsonify({ @@ -19757,7 +19750,7 @@ def soundcloud_status(): try: sc = None if soulseek_client and hasattr(soulseek_client, 'soundcloud'): - sc = soulseek_client.soundcloud + sc = soulseek_client.client("soundcloud") if not sc: return jsonify({ "available": False, @@ -19785,7 +19778,7 @@ def hifi_instances(): """Check availability of all HiFi API instances.""" import requests as req try: - hifi = soulseek_client.hifi + hifi = soulseek_client.client("hifi") instances = list(hifi._instances) results = [] for url in instances: @@ -19840,9 +19833,9 @@ def hifi_add_instance(): added = db.add_hifi_instance(url, priority) if not added: return jsonify({'success': False, 'error': 'Instance already exists'}), 400 - # Reload the client - if soulseek_client and hasattr(soulseek_client, 'hifi') and soulseek_client.hifi: - soulseek_client.hifi.reload_instances() + # Reload the HiFi client + if soulseek_client: + soulseek_client.reload_instances('hifi') return jsonify({'success': True, 'url': url}) except Exception as e: logger.error(f"Error adding HiFi instance: {e}") @@ -19862,9 +19855,9 @@ def hifi_remove_instance(): removed = db.remove_hifi_instance(url) if not removed: return jsonify({'success': False, 'error': 'Instance not found'}), 404 - # Reload the client - if soulseek_client and hasattr(soulseek_client, 'hifi') and soulseek_client.hifi: - soulseek_client.hifi.reload_instances() + # Reload the HiFi client + if soulseek_client: + soulseek_client.reload_instances('hifi') return jsonify({'success': True, 'url': url}) except Exception as e: logger.error(f"Error removing HiFi instance: {e}") @@ -19884,8 +19877,8 @@ def hifi_toggle_instance(): from database.music_database import get_database db = get_database() db.toggle_hifi_instance(url, enabled) - if soulseek_client and hasattr(soulseek_client, 'hifi') and soulseek_client.hifi: - soulseek_client.hifi.reload_instances() + if soulseek_client: + soulseek_client.reload_instances('hifi') return jsonify({'success': True}) except Exception as e: logger.error(f"Error toggling HiFi instance: {e}") @@ -19905,9 +19898,9 @@ def hifi_reorder_instances(): db = get_database() if not db.reorder_hifi_instances(urls): return jsonify({'success': False, 'error': 'One or more URLs not found'}), 400 - # Reload the client - if soulseek_client and hasattr(soulseek_client, 'hifi') and soulseek_client.hifi: - soulseek_client.hifi.reload_instances() + # Reload the HiFi client + if soulseek_client: + soulseek_client.reload_instances('hifi') return jsonify({'success': True}) except Exception as e: logger.error(f"Error reordering HiFi instances: {e}") @@ -20063,7 +20056,7 @@ def _get_tidal_download_client(): raise RuntimeError("Download orchestrator not initialized — check startup logs for errors") if not hasattr(soulseek_client, 'tidal') or not soulseek_client.tidal: raise RuntimeError("Tidal download client not available — ensure tidalapi is installed") - return soulseek_client.tidal + return soulseek_client.client("tidal") @app.route('/api/tidal/download/auth/start', methods=['POST']) def tidal_download_auth_start(): @@ -20132,7 +20125,7 @@ def qobuz_auth_login(): if not email or not password: return jsonify({"success": False, "error": "Email and password required"}), 400 - qobuz = soulseek_client.qobuz + qobuz = soulseek_client.client("qobuz") result = qobuz.login(email, password) if result['status'] == 'success': @@ -20155,7 +20148,7 @@ def qobuz_auth_token(): if not token: return jsonify({"success": False, "error": "Auth token required"}), 400 - qobuz = soulseek_client.qobuz + qobuz = soulseek_client.client("qobuz") result = qobuz.login_with_token(token) if result['status'] == 'success': @@ -20172,7 +20165,7 @@ def qobuz_auth_token(): def qobuz_auth_status(): """Check if Qobuz client is authenticated.""" try: - qobuz = soulseek_client.qobuz + qobuz = soulseek_client.client("qobuz") authenticated = qobuz.is_authenticated() user_info = {} if authenticated and qobuz.user_info: @@ -20189,7 +20182,7 @@ def qobuz_auth_status(): def qobuz_auth_logout(): """Logout from Qobuz.""" try: - soulseek_client.qobuz.logout() + soulseek_client.client("qobuz").logout() _sync_qobuz_credentials_to_worker() return jsonify({"success": True}) except Exception as e: @@ -21133,7 +21126,7 @@ def get_deezer_arl_status(): try: deezer_dl = None if soulseek_client and hasattr(soulseek_client, 'deezer_dl') and soulseek_client.deezer_dl: - deezer_dl = soulseek_client.deezer_dl + deezer_dl = soulseek_client.client("deezer_dl") if deezer_dl and deezer_dl.is_authenticated(): user_data = deezer_dl._user_data or {} return jsonify({ @@ -21152,7 +21145,7 @@ def get_deezer_arl_playlists(): try: deezer_dl = None if soulseek_client and hasattr(soulseek_client, 'deezer_dl') and soulseek_client.deezer_dl: - deezer_dl = soulseek_client.deezer_dl + deezer_dl = soulseek_client.client("deezer_dl") if not deezer_dl or not deezer_dl.is_authenticated(): return jsonify({'error': 'Deezer ARL not authenticated. Configure your ARL token in Settings > Downloads.'}), 401 @@ -21182,7 +21175,7 @@ def get_deezer_arl_playlist_tracks(playlist_id): try: deezer_dl = None if soulseek_client and hasattr(soulseek_client, 'deezer_dl') and soulseek_client.deezer_dl: - deezer_dl = soulseek_client.deezer_dl + deezer_dl = soulseek_client.client("deezer_dl") if not deezer_dl or not deezer_dl.is_authenticated(): return jsonify({'error': 'Deezer ARL not authenticated.'}), 401 @@ -27374,8 +27367,8 @@ def get_your_artists_sources(): try: deezer_cl = _get_deezer_client() deezer_oauth = deezer_cl and hasattr(deezer_cl, 'is_user_authenticated') and deezer_cl.is_user_authenticated() - deezer_arl = (hasattr(soulseek_client, 'deezer_dl') and soulseek_client.deezer_dl - and soulseek_client.deezer_dl.is_authenticated()) + deezer_arl = (hasattr(soulseek_client, 'deezer_dl') and soulseek_client.client("deezer_dl") + and soulseek_client.client("deezer_dl").is_authenticated()) if deezer_oauth or deezer_arl: connected.append('deezer') except Exception: @@ -27496,10 +27489,10 @@ def _fetch_and_match_liked_artists(profile_id: int): if deezer_cl and hasattr(deezer_cl, 'is_user_authenticated') and deezer_cl.is_user_authenticated(): logger.info("[Your Artists] Fetching favorite artists from Deezer (OAuth)...") artists = deezer_cl.get_user_favorite_artists(limit=200) - elif (hasattr(soulseek_client, 'deezer_dl') and soulseek_client.deezer_dl - and soulseek_client.deezer_dl.is_authenticated()): + elif (hasattr(soulseek_client, 'deezer_dl') and soulseek_client.client("deezer_dl") + and soulseek_client.client("deezer_dl").is_authenticated()): logger.info("[Your Artists] Fetching favorite artists from Deezer (ARL)...") - artists = soulseek_client.deezer_dl.get_user_favorite_artists(limit=200) + artists = soulseek_client.client("deezer_dl").get_user_favorite_artists(limit=200) for a in artists: database.upsert_liked_artist( artist_name=a['name'], source_service='deezer', @@ -27646,8 +27639,8 @@ def get_your_albums_sources(): try: deezer_cl = _get_deezer_client() deezer_oauth = deezer_cl and hasattr(deezer_cl, 'is_user_authenticated') and deezer_cl.is_user_authenticated() - deezer_arl = (hasattr(soulseek_client, 'deezer_dl') and soulseek_client.deezer_dl - and soulseek_client.deezer_dl.is_authenticated()) + deezer_arl = (hasattr(soulseek_client, 'deezer_dl') and soulseek_client.client("deezer_dl") + and soulseek_client.client("deezer_dl").is_authenticated()) if deezer_oauth or deezer_arl: connected.append('deezer') except Exception: @@ -27754,10 +27747,10 @@ def _fetch_liked_albums(profile_id: int): if deezer_cl and hasattr(deezer_cl, 'is_user_authenticated') and deezer_cl.is_user_authenticated(): logger.info("[Your Albums] Fetching favorite albums from Deezer (OAuth)...") albums = deezer_cl.get_user_favorite_albums(limit=500) - elif (hasattr(soulseek_client, 'deezer_dl') and soulseek_client.deezer_dl - and soulseek_client.deezer_dl.is_authenticated()): + elif (hasattr(soulseek_client, 'deezer_dl') and soulseek_client.client("deezer_dl") + and soulseek_client.client("deezer_dl").is_authenticated()): logger.info("[Your Albums] Fetching favorite albums from Deezer (ARL)...") - albums = soulseek_client.deezer_dl.get_user_favorite_albums(limit=500) + albums = soulseek_client.client("deezer_dl").get_user_favorite_albums(limit=500) for a in albums: database.upsert_liked_album( album_name=a['album_name'], artist_name=a['artist_name'], diff --git a/webui/static/helper.js b/webui/static/helper.js index 5ee17338..063c95bb 100644 --- a/webui/static/helper.js +++ b/webui/static/helper.js @@ -3432,6 +3432,7 @@ const WHATS_NEW = { '2.4.2': [ // --- post-2.4.1 dev work — entries hidden by _getLatestWhatsNewVersion until the build version bumps --- { date: 'Unreleased — 2.4.2 dev cycle' }, + { title: 'Internal: Download Engine Review Followup', desc: 'internal — three correctness fixes on top of the download engine refactor, all flagged in cin\'s pr review. (1) `engine.cancel_download(source_hint=\'deezer_dl\')` was silently routing deezer cancels to soulseek because the legacy alias never made it to the engine\'s plugin map — only the registry knew about it. fix: aliases now flow through `register_plugin` and `get_plugin` / `cancel_download` resolve them to the canonical name. (2) `_resolve_source_chain` filtered hybrid_order against canonical registry names only, so any user with `deezer_dl` in their config quietly dropped deezer from hybrid mode. fix: orchestrator normalizes through `registry.get_spec()` first. (3) the worker\'s terminal write was a read-then-write split — a cancel landing between the snapshot and the update could be overwritten back to errored / completed. fix: new atomic `update_record_unless_state` on the engine holds `state_lock` across the check + write; both `_mark_terminal` AND the success path use it now. also added generic `client(name)` / `configured_clients()` / `reload_instances(name?)` accessors on the orchestrator + a `get/set_download_orchestrator()` singleton matching cin\'s `get_metadata_engine()` shape, and migrated 30 external `soulseek_client.` reaches in web_server.py to `client("")`. 18 new tests pin every fix.' }, { title: 'Internal: Typed Metadata Foundation', desc: 'internal — first step of a multi-pr migration to give the metadata pipeline a real contract. the codebase historically grew duck-typed extractors (`_extract_lookup_value(album_data, "id", "album_id", "collectionId", "release_id", default=...)`) at every consumer site because each provider returns its own response shape. ~150 of those across the codebase. new `core/metadata/types.py` defines canonical typed `Album` / `Track` / `Artist` dataclasses with strict required fields. per-source classmethod converters (from_spotify_dict, from_itunes_dict, from_deezer_dict, from_discogs_dict, from_musicbrainz_dict, from_hydrabase_dict) are the SINGLE place that knows each provider\'s wire shape. zero behavior changes in this pr — pure additive foundation. follow-up prs migrate consumers one at a time. full migration plan documented at docs/metadata-types-migration.md.', page: 'library' }, { title: 'Internal: Migrate Album-Info Builders to Typed Path', desc: 'internal — steps 2+3 of the typed metadata migration in one pr. two album-info builders now route through `Album.from__dict()` when the caller passes a known source: `_build_album_info` (used by every album-tracks lookup) and the embedded album section of `_build_single_import_context_payload` (used by single-track import context resolution). legacy duck-typed extraction stays as the fallback when source is empty/unknown, raw input isn\'t a dict, or the typed converter raises — so a converter bug can\'t break album resolution or import context. caller-provided album_id / album_name / artist_name fallbacks apply on the typed path the same way they did on legacy. zero behavior change for existing callers since they don\'t pass a source yet — opt-in only. 22 new tests pin the typed path, the legacy fallback, and parametrized coverage across registered providers.' }, { title: 'Internal: Migrate Discography + Quality Scanner to Typed Path', desc: 'internal — next round of the typed metadata migration. three more album-shape consumers now route through `Album.from__dict()` when the caller passes a known source: `_build_discography_release_dict` (artist discography release cards), `_build_artist_detail_release_card` (artist detail page release cards), and `_normalize_track_album` (quality scanner result normalization). legacy duck-typed extraction stays as the fallback when source is empty/unknown, raw input isn\'t a dict, or the typed converter raises — same safety contract as the prior migration steps. 20 new tests pin the typed path + legacy fallback + parametrized coverage across registered providers.' },