diff --git a/core/deezer_download_client.py b/core/deezer_download_client.py index 1d7dccc7..d3a13d26 100644 --- a/core/deezer_download_client.py +++ b/core/deezer_download_client.py @@ -613,8 +613,11 @@ class DeezerDownloadClient(DownloadSourcePlugin): logger.error("Deezer not authenticated — cannot download") return None if self._engine is None: - logger.error("Deezer client has no engine reference — cannot dispatch download") - return None + # Raise rather than return None so the orchestrator's + # download_with_fallback surfaces a real warning + tries + # the next source. Returning None silently dropped the + # download with no user feedback (per JohnBaumb). + raise RuntimeError("Deezer client has no engine reference — cannot dispatch download") # Parse filename: "track_id||display_name" parts = filename.split('||', 1) diff --git a/core/download_engine/engine.py b/core/download_engine/engine.py index a1391111..8aaa07e8 100644 --- a/core/download_engine/engine.py +++ b/core/download_engine/engine.py @@ -277,15 +277,21 @@ class DownloadEngine: # # All methods are async to match the per-plugin contract. - async def get_all_downloads(self): + async def get_all_downloads(self, exclude: Tuple[str, ...] = ()): """Aggregated view across every registered plugin's active downloads. Per-plugin exceptions are swallowed (one source failing shouldn't take down cross-source aggregation) but logged at debug level — same defensive shape the legacy - orchestrator had.""" + orchestrator had. + + ``exclude`` skips named sources entirely. The download monitor + passes ``('soulseek',)`` so it doesn't double-fetch slskd + transfers (it already pulled them via the slskd transfers + endpoint earlier in the same loop). + """ all_downloads = [] for source_name, plugin in self._plugins.items(): - if plugin is None: + if plugin is None or source_name in exclude: continue try: all_downloads.extend(await plugin.get_all_downloads()) diff --git a/core/downloads/monitor.py b/core/downloads/monitor.py index d08843a4..ff939352 100644 --- a/core/downloads/monitor.py +++ b/core/downloads/monitor.py @@ -273,7 +273,13 @@ class WebUIDownloadMonitor: all_downloads = [] if download_orchestrator and hasattr(download_orchestrator, 'engine'): try: - all_downloads = run_async(download_orchestrator.engine.get_all_downloads()) + # Exclude soulseek — slskd transfers were already + # pulled via the transfers/downloads endpoint above. + # Without the exclude both fetch paths run, doubling + # the per-tick slskd API hit. + all_downloads = run_async( + download_orchestrator.engine.get_all_downloads(exclude=('soulseek',)) + ) except Exception: pass for download in all_downloads: diff --git a/core/hifi_client.py b/core/hifi_client.py index cf383b52..8fa51cdd 100644 --- a/core/hifi_client.py +++ b/core/hifi_client.py @@ -579,8 +579,11 @@ class HiFiClient(DownloadSourcePlugin): logger.error(f"Invalid filename format: {filename}") return None if self._engine is None: - logger.error("HiFi client has no engine reference — cannot dispatch download") - return None + # Raise rather than return None so the orchestrator's + # download_with_fallback surfaces a real warning + tries + # the next source. Returning None silently dropped the + # download with no user feedback (per JohnBaumb). + raise RuntimeError("HiFi client has no engine reference — cannot dispatch download") track_id_str, display_name = filename.split('||', 1) try: diff --git a/core/qobuz_client.py b/core/qobuz_client.py index 48ceeb08..745bf27d 100644 --- a/core/qobuz_client.py +++ b/core/qobuz_client.py @@ -899,8 +899,11 @@ class QobuzClient(DownloadSourcePlugin): logger.error(f"Invalid filename format: {filename}") return None if self._engine is None: - logger.error("Qobuz client has no engine reference — cannot dispatch download") - return None + # Raise rather than return None so the orchestrator's + # download_with_fallback surfaces a real warning + tries + # the next source. Returning None silently dropped the + # download with no user feedback (per JohnBaumb). + raise RuntimeError("Qobuz client has no engine reference — cannot dispatch download") track_id_str, display_name = filename.split('||', 1) try: diff --git a/core/soundcloud_client.py b/core/soundcloud_client.py index b942a292..530d6f72 100644 --- a/core/soundcloud_client.py +++ b/core/soundcloud_client.py @@ -335,8 +335,11 @@ class SoundcloudClient(DownloadSourcePlugin): logger.error(f"Missing SoundCloud track id or url in: {filename}") return None if self._engine is None: - logger.error("SoundCloud client has no engine reference — cannot dispatch download") - return None + # Raise rather than return None so the orchestrator's + # download_with_fallback surfaces a real warning + tries + # the next source. Returning None silently dropped the + # download with no user feedback (per JohnBaumb). + raise RuntimeError("SoundCloud client has no engine reference — cannot dispatch download") logger.info(f"Starting SoundCloud download: {display_name}") diff --git a/core/tidal_download_client.py b/core/tidal_download_client.py index aab53290..45bdd4b9 100644 --- a/core/tidal_download_client.py +++ b/core/tidal_download_client.py @@ -619,8 +619,11 @@ class TidalDownloadClient(DownloadSourcePlugin): logger.error(f"Invalid filename format: {filename}") return None if self._engine is None: - logger.error("Tidal client has no engine reference — cannot dispatch download") - return None + # Raise rather than return None so the orchestrator's + # download_with_fallback surfaces a real warning + tries + # the next source. Returning None silently dropped the + # download with no user feedback (per JohnBaumb). + raise RuntimeError("Tidal client has no engine reference — cannot dispatch download") track_id_str, display_name = filename.split('||', 1) try: diff --git a/core/youtube_client.py b/core/youtube_client.py index 08993bab..b40f76cb 100644 --- a/core/youtube_client.py +++ b/core/youtube_client.py @@ -890,8 +890,11 @@ class YouTubeClient(DownloadSourcePlugin): logger.error(f"Invalid filename format: {filename}") return None if self._engine is None: - logger.error("YouTube client has no engine reference — cannot dispatch download") - return None + # Raise rather than return None so the orchestrator's + # download_with_fallback surfaces a real warning + tries + # the next source. Returning None silently dropped the + # download with no user feedback (per JohnBaumb). + raise RuntimeError("YouTube client has no engine reference — cannot dispatch download") video_id, title = filename.split('||', 1) youtube_url = f"https://www.youtube.com/watch?v={video_id}" diff --git a/tests/downloads/test_deezer_pinning.py b/tests/downloads/test_deezer_pinning.py index aa885907..f3f2cab2 100644 --- a/tests/downloads/test_deezer_pinning.py +++ b/tests/downloads/test_deezer_pinning.py @@ -48,12 +48,18 @@ def test_download_returns_none_when_not_authenticated(deezer_client_with_engine) assert result is None -def test_download_returns_none_when_engine_not_wired(): +def test_download_raises_when_engine_not_wired(): + """Defensive: client without engine reference must raise so the + orchestrator's download_with_fallback surfaces the error and + moves on to the next source. Returning None silently would drop + the download with no user feedback (per JohnBaumb).""" + import pytest client = DeezerDownloadClient.__new__(DeezerDownloadClient) client._engine = None + # Bypass auth gate so we exercise the engine check. client._authenticated = True - result = _run_async(client.download('deezer_dl', '12345||x', 0)) - assert result is None + with pytest.raises(RuntimeError, match="engine reference"): + _run_async(client.download('deezer', 'v||t', 0)) def test_download_track_id_stays_as_string(deezer_client_with_engine): diff --git a/tests/downloads/test_download_engine.py b/tests/downloads/test_download_engine.py index e4e9cff0..d46a0ee2 100644 --- a/tests/downloads/test_download_engine.py +++ b/tests/downloads/test_download_engine.py @@ -320,6 +320,24 @@ def test_engine_get_all_downloads_aggregates_across_plugins(): assert {r.id for r in result} == {'yt-1', 'td-1', 'td-2'} +def test_engine_get_all_downloads_skips_excluded_sources(): + """Per JohnBaumb: monitor pulls slskd transfers via the + transfers/downloads endpoint earlier in its loop, so engine + aggregation must skip soulseek to avoid double-fetching.""" + engine = DownloadEngine() + sl_plugin = _FakePlugin('soulseek', downloads=[_FakeStatus('sl-1', 'soulseek')]) + yt_plugin = _FakePlugin('youtube', downloads=[_FakeStatus('yt-1', 'youtube')]) + td_plugin = _FakePlugin('tidal', downloads=[_FakeStatus('td-1', 'tidal')]) + engine.register_plugin('soulseek', sl_plugin) + engine.register_plugin('youtube', yt_plugin) + engine.register_plugin('tidal', td_plugin) + + result = _run_async(engine.get_all_downloads(exclude=('soulseek',))) + ids = {r.id for r in result} + assert ids == {'yt-1', 'td-1'} + assert 'sl-1' not in ids + + def test_engine_get_all_downloads_swallows_per_plugin_exceptions(): """One plugin throwing must NOT take down the whole list — same defensive behavior as the legacy orchestrator (matched by diff --git a/tests/downloads/test_hifi_pinning.py b/tests/downloads/test_hifi_pinning.py index 8111423f..24ca15f1 100644 --- a/tests/downloads/test_hifi_pinning.py +++ b/tests/downloads/test_hifi_pinning.py @@ -44,11 +44,16 @@ def test_download_returns_none_for_non_integer_track_id(hifi_client_with_engine) assert result is None -def test_download_returns_none_when_engine_not_wired(): +def test_download_raises_when_engine_not_wired(): + """Defensive: client without engine reference must raise so the + orchestrator's download_with_fallback surfaces the error and + moves on to the next source. Returning None silently would drop + the download with no user feedback (per JohnBaumb).""" + import pytest client = HiFiClient.__new__(HiFiClient) client._engine = None - result = _run_async(client.download('hifi', '12345||x', 0)) - assert result is None + with pytest.raises(RuntimeError, match="engine reference"): + _run_async(client.download('hifi', 'v||t', 0)) def test_download_returns_uuid_for_valid_filename(hifi_client_with_engine): diff --git a/tests/downloads/test_qobuz_pinning.py b/tests/downloads/test_qobuz_pinning.py index 9db40975..ab55d8ed 100644 --- a/tests/downloads/test_qobuz_pinning.py +++ b/tests/downloads/test_qobuz_pinning.py @@ -47,11 +47,16 @@ def test_download_returns_none_for_non_integer_track_id(qobuz_client_with_engine assert result is None -def test_download_returns_none_when_engine_not_wired(): +def test_download_raises_when_engine_not_wired(): + """Defensive: client without engine reference must raise so the + orchestrator's download_with_fallback surfaces the error and + moves on to the next source. Returning None silently would drop + the download with no user feedback (per JohnBaumb).""" + import pytest client = QobuzClient.__new__(QobuzClient) client._engine = None - result = _run_async(client.download('qobuz', '12345||x', 0)) - assert result is None + with pytest.raises(RuntimeError, match="engine reference"): + _run_async(client.download('qobuz', 'v||t', 0)) def test_download_returns_uuid_for_valid_filename(qobuz_client_with_engine): diff --git a/tests/downloads/test_soundcloud_pinning.py b/tests/downloads/test_soundcloud_pinning.py index 2a6e7127..4a5bc893 100644 --- a/tests/downloads/test_soundcloud_pinning.py +++ b/tests/downloads/test_soundcloud_pinning.py @@ -50,13 +50,16 @@ def test_download_returns_none_for_empty_track_id_or_url(sc_client_with_engine): assert _run_async(client.download('soundcloud', 'track123||', 0)) is None -def test_download_returns_none_when_engine_not_wired(): +def test_download_raises_when_engine_not_wired(): + """Defensive: client without engine reference must raise so the + orchestrator's download_with_fallback surfaces the error and + moves on to the next source. Returning None silently would drop + the download with no user feedback (per JohnBaumb).""" + import pytest client = SoundcloudClient.__new__(SoundcloudClient) client._engine = None - result = _run_async(client.download( - 'soundcloud', 'sc-1||https://soundcloud.com/x/y||T', 0, - )) - assert result is None + with pytest.raises(RuntimeError, match="engine reference"): + _run_async(client.download('soundcloud', 'v||t', 0)) def test_download_accepts_three_part_filename_with_display(sc_client_with_engine): diff --git a/tests/downloads/test_tidal_pinning.py b/tests/downloads/test_tidal_pinning.py index 405ee21b..74327f37 100644 --- a/tests/downloads/test_tidal_pinning.py +++ b/tests/downloads/test_tidal_pinning.py @@ -78,11 +78,16 @@ def test_download_returns_none_for_non_integer_track_id(tidal_client_with_engine assert result is None -def test_download_returns_none_when_engine_not_wired(): +def test_download_raises_when_engine_not_wired(): + """Defensive: client without engine reference must raise so the + orchestrator's download_with_fallback surfaces the error and + moves on to the next source. Returning None silently would drop + the download with no user feedback (per JohnBaumb).""" + import pytest client = TidalDownloadClient.__new__(TidalDownloadClient) client._engine = None - result = _run_async(client.download('tidal', '12345||x', 0)) - assert result is None + with pytest.raises(RuntimeError, match="engine reference"): + _run_async(client.download('tidal', 'v||t', 0)) def test_download_returns_uuid_for_valid_filename(tidal_client_with_engine): diff --git a/tests/downloads/test_youtube_pinning.py b/tests/downloads/test_youtube_pinning.py index 59cfb71b..4637af97 100644 --- a/tests/downloads/test_youtube_pinning.py +++ b/tests/downloads/test_youtube_pinning.py @@ -77,14 +77,16 @@ def test_download_returns_none_for_invalid_filename_format(yt_client_with_engine assert result is None -def test_download_returns_none_when_engine_not_wired(): - """Defensive: client without engine reference can't dispatch. - In production this never happens (orchestrator wires engine - immediately) but the soft-fail keeps tests + dev paths safe.""" +def test_download_raises_when_engine_not_wired(): + """Defensive: client without engine reference must raise so the + orchestrator's download_with_fallback surfaces the error and + moves on to the next source. Returning None silently would drop + the download with no user feedback (per JohnBaumb).""" + import pytest client = YouTubeClient.__new__(YouTubeClient) client._engine = None - result = _run_async(client.download('youtube', 'v||t', 0)) - assert result is None + with pytest.raises(RuntimeError, match="engine reference"): + _run_async(client.download('youtube', 'v||t', 0)) def test_download_returns_uuid_download_id_for_valid_filename(yt_client_with_engine): diff --git a/web_server.py b/web_server.py index 74a332e7..77c100f5 100644 --- a/web_server.py +++ b/web_server.py @@ -2474,32 +2474,35 @@ def get_cached_transfer_data(): key = _make_context_key(transfer.get('username'), transfer.get('filename', '')) live_transfers_lookup[key] = transfer - # Also add non-Soulseek downloads (avoid redundant slskd call through orchestrator). - # Every streaming source must appear here — task progress for in-flight - # downloads comes from this lookup. Missing a source = task.progress - # stays at 0 even when the underlying client knows the real percent. + # Also add non-Soulseek downloads. Soulseek is excluded + # because slskd's transfers endpoint was already pulled + # above — without the exclude both fetch paths run. + # Every streaming source must appear here — task progress + # for in-flight downloads comes from this lookup. Missing a + # source = task.progress stays at 0 even when the + # underlying client knows the real percent. try: all_downloads = [] - # Generic dispatch — engine returns active downloads - # across every plugin in one call, no per-source iteration. if download_orchestrator and hasattr(download_orchestrator, 'engine'): try: - all_downloads = run_async(download_orchestrator.engine.get_all_downloads()) + all_downloads = run_async( + download_orchestrator.engine.get_all_downloads(exclude=('soulseek',)) + ) except Exception: pass for download in all_downloads: - key = _make_context_key(download.username, download.filename) - # Convert DownloadStatus to transfer dict format - live_transfers_lookup[key] = { - 'id': download.id, - 'filename': download.filename, - 'username': download.username, - 'state': download.state, - 'percentComplete': download.progress, - 'size': download.size, - 'bytesTransferred': download.transferred, - 'averageSpeed': download.speed, - } + key = _make_context_key(download.username, download.filename) + # Convert DownloadStatus to transfer dict format + live_transfers_lookup[key] = { + 'id': download.id, + 'filename': download.filename, + 'username': download.username, + 'state': download.state, + 'percentComplete': download.progress, + 'size': download.size, + 'bytesTransferred': download.transferred, + 'averageSpeed': download.speed, + } except Exception as e: logger.error(f"Could not fetch streaming source downloads: {e}")