danvics/soulsync
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions 1

Surface engine-not-wired errors + exclude soulseek from monitor aggregation

Browse source 
Two findings from JohnBaumb on the engine refactor.

(1) Every download client returned None when self._engine was None,
just logging an error. The orchestrator's download_with_fallback
treated None as "source declined", so the user got no feedback —
download silently disappeared. Now each client raises a RuntimeError
on the engine-not-wired path. download_with_fallback already catches
plugin exceptions, logs a warning, and tries the next source — so
the visible behavior is "real error in logs + fallback to next
source" instead of "silent drop". Six clients touched (deezer, hifi,
qobuz, soundcloud, tidal, youtube). Pinning tests updated to expect
raise.

(2) Monitor's engine.get_all_downloads() walked every plugin
including soulseek, but the same monitor loop already pulled slskd
transfers via the transfers/downloads endpoint a few lines earlier —
soulseek's records were being fetched twice per tick. Same issue in
web_server.py's get_cached_transfer_data path. Added an exclude
parameter to engine.get_all_downloads(); both call sites now pass
('soulseek',). New test pins the exclude semantic.

Also fixed a stray 8-space over-indent on the for-loop body in
get_cached_transfer_data (cosmetic, JohnBaumb flagged the same
pattern in monitor.py earlier).
This commit is contained in:
Broque Thomas 2026-05-05 12:20:51 -07:00
parent 2c19d7d1f2
commit c4c922c40f
16 changed files with 135 additions and 58 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

7
core/deezer_download_client.py
View file

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

12
core/download_engine/engine.py
View file

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

8
core/downloads/monitor.py
View file

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

7
core/hifi_client.py
View file

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

7
core/qobuz_client.py
View file

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

7
core/soundcloud_client.py
View file

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

7
core/tidal_download_client.py
View file

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

7
core/youtube_client.py
View file

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

12
tests/downloads/test_deezer_pinning.py
View file

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

18
tests/downloads/test_download_engine.py
View file

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

11
tests/downloads/test_hifi_pinning.py
View file

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

11
tests/downloads/test_qobuz_pinning.py
View file

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

13
tests/downloads/test_soundcloud_pinning.py
View file

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

11
tests/downloads/test_tidal_pinning.py
View file

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

14
tests/downloads/test_youtube_pinning.py
View file

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

41
web_server.py
View file

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