soulsync/tests/test_status_engine_fallback.py
Broque Thomas e20994e1c7 Manual picks: stream results, don't auto-retry, fix stuck-at-0%
Three follow-on fixes to the manual-search candidates modal once people
started actually using it:

1. NDJSON streaming. Manual search waited for every source to return
   before showing anything. Now streams one event per source as each
   completes — header line, source_results per source, done terminator.
   Frontend appends rows incrementally via response.body.getReader().

2. Manual picks no longer auto-retry on failure. New _user_manual_pick
   flag set on the task in /download-candidate. Both monitor retry
   paths (not-in-live-transfers stuck + Errored state) bail on the
   flag. Surfaces the failure to the user instead of silently picking
   a different candidate via fresh search.

3. Non-Soulseek manual picks (youtube/tidal/qobuz/hifi/deezer/
   soundcloud/lidarr) no longer stuck at "downloading 0%" forever. The
   live_transfers IF branch now marks manual-pick tasks failed
   directly when the engine reports Errored, instead of deferring to
   the monitor (which bails on manual picks). Engine fallback in else
   branch covers the rare race where the orchestrator's pre-populated
   transfer lookup is missing the entry.

Plus a deadlock fix discovered along the way: the new failure path
synchronously called on_download_completed while holding tasks_lock,
which itself re-acquires the same Lock — non-reentrant
threading.Lock self-deadlocked the polling thread. While wedged, every
other endpoint that needed the lock (including /candidates → other
failed rows couldn't open modals) hung waiting. Moved completion
callbacks onto a daemon thread so the lock releases first.

Plus failed/not_found/cancelled rows are now ALWAYS clickable (not
just when the auto-search cached candidates) — the modal carries the
manual search bar, which is the user's recourse for empty results.

Plus manual download worker now runs on a dedicated thread instead of
competing with the batch's 3-worker missing_download_executor pool —
saturated batches no longer queue manual picks indefinitely.

All scoped to manual picks via the _user_manual_pick flag — auto
attempt flow byte-identical to before. Engine fallback gated on the
flag too so auto attempts in the else branch keep the original
do-nothing behavior (safety valve handles the stuck-forever case).

Also dropped _handle_failed_download from web_server.py — defined
but had no callers (dead code).

17 new unit tests pin the gate behavior:
- engine fallback: Errored/Cancelled/Succeeded/InProgress transitions,
  manual-pick gate, terminal-state skip, soulseek skip, missing
  download_id skip, engine returning None, orchestrator exception
- monitor: manual-pick skips not-in-live-transfers retry + Errored
  retry
- IF-branch end-to-end: Errored marks failed, "Completed, Errored"
  hits failure branch, auto attempts defer to monitor

Manual-search endpoint tests rewritten for NDJSON: 11 cases (validation,
single-source dispatch, parallel "all" dispatch, one-event-per-source
streaming shape, unconfigured-source skip + reject, header metadata,
per-source exception isolation).

Full suite 2259 passed, 1 skipped.
2026-05-08 15:12:58 -07:00

469 lines
17 KiB
Python

"""Pin the engine-state fallback that drives non-Soulseek (streaming)
download status forward.
Soulseek downloads land in slskd's ``live_transfers_lookup``, so their
status updates flow through the existing slskd-state branch. Streaming
sources (YouTube, Tidal, Qobuz, HiFi, Deezer, SoundCloud, Lidarr) never
appear there — without these tests' code path, a manually-picked
SoundCloud download stays at "downloading 0%" forever, even after the
engine logs an Errored terminal state.
These tests exercise ``_apply_engine_state_fallback`` directly with a
fake ``download_orchestrator`` so we don't have to spin up the real
engine. The real fix relies on the per-source plugin storing the
terminal state via ``_mark_terminal`` (state='Errored' / 'Completed,
Succeeded'), which our fake mirrors.
"""
from __future__ import annotations
from dataclasses import dataclass
from typing import Optional
from unittest.mock import MagicMock
import pytest
from core.downloads import status as status_mod
@dataclass
class _FakeDownloadStatus:
"""Mirror the DownloadStatus shape that engine plugins return — only
the fields the fallback reads."""
id: str
state: str
progress: float = 0
error_message: Optional[str] = None
def _make_deps(record_for_id: dict, on_completed=None, submit_pp=None):
"""StatusDeps with a fake orchestrator that returns whatever
DownloadStatus the test provides for a given download_id."""
fake_orch = MagicMock()
async def _fake_get_status(download_id):
return record_for_id.get(download_id)
fake_orch.get_download_status = _fake_get_status
def _sync_run_async(coro):
import asyncio
loop = asyncio.new_event_loop()
try:
return loop.run_until_complete(coro)
finally:
loop.close()
return status_mod.StatusDeps(
config_manager=MagicMock(),
docker_resolve_path=lambda p: p,
find_completed_file=lambda *args, **kwargs: (None, None),
make_context_key=lambda u, f: f"{u}::{f}",
submit_post_processing=submit_pp or (lambda task_id, batch_id: None),
get_cached_transfer_data=lambda: {},
download_orchestrator=fake_orch,
run_async=_sync_run_async,
on_download_completed=on_completed,
)
def _task(*, status='downloading', username='soundcloud', filename='1234||https://sc/x',
download_id='dl-1', manual_pick=True):
"""Default to manual_pick=True because the engine fallback is
deliberately scoped to manual picks — auto attempts go through the
live_transfers branch + monitor retry path. Tests opt out of the
flag explicitly when exercising the auto-attempt skip."""
return {
'status': status,
'username': username,
'filename': filename,
'download_id': download_id,
'_user_manual_pick': manual_pick,
'track_info': {'name': 'Test Track'},
}
# ---------------------------------------------------------------------------
# Failure / cancel / success transitions
# ---------------------------------------------------------------------------
def test_errored_state_marks_task_failed():
import time as _t
task = _task()
task_status = {'status': 'downloading', 'progress': 0}
completed_calls = []
deps = _make_deps(
{'dl-1': _FakeDownloadStatus(id='dl-1', state='Errored', error_message='HTTP 404')},
on_completed=lambda batch_id, task_id, success: completed_calls.append((batch_id, task_id, success)),
)
status_mod._apply_engine_state_fallback('t1', task, task_status, 'b1', deps)
assert task['status'] == 'failed'
assert task['error_message'] == 'HTTP 404'
assert task_status['status'] == 'failed'
# on_download_completed is deferred to a daemon thread to avoid the
# tasks_lock self-deadlock — give it a beat to fire.
for _ in range(50):
if completed_calls:
break
_t.sleep(0.01)
assert completed_calls == [('b1', 't1', False)]
def test_compound_completed_errored_hits_failure_branch_first():
"""``"Completed, Errored"`` must be treated as failure, not success.
Order of state-substring checks matters."""
task = _task()
task_status = {'status': 'downloading', 'progress': 0}
deps = _make_deps(
{'dl-1': _FakeDownloadStatus(id='dl-1', state='Completed, Errored')},
on_completed=lambda *args: None,
)
status_mod._apply_engine_state_fallback('t1', task, task_status, 'b1', deps)
assert task['status'] == 'failed'
def test_cancelled_state_marks_task_cancelled():
import time as _t
task = _task()
task_status = {'status': 'downloading', 'progress': 0}
completed_calls = []
deps = _make_deps(
{'dl-1': _FakeDownloadStatus(id='dl-1', state='Cancelled')},
on_completed=lambda *args: completed_calls.append(args),
)
status_mod._apply_engine_state_fallback('t1', task, task_status, 'b1', deps)
assert task['status'] == 'cancelled'
assert task_status['status'] == 'cancelled'
for _ in range(50):
if completed_calls:
break
_t.sleep(0.01)
assert len(completed_calls) == 1
def test_succeeded_state_submits_post_processing():
task = _task()
task_status = {'status': 'downloading', 'progress': 0}
pp_calls = []
deps = _make_deps(
{'dl-1': _FakeDownloadStatus(id='dl-1', state='Completed, Succeeded', progress=100)},
submit_pp=lambda task_id, batch_id: pp_calls.append((task_id, batch_id)),
)
status_mod._apply_engine_state_fallback('t1', task, task_status, 'b1', deps)
assert task['status'] == 'post_processing'
assert task_status['status'] == 'post_processing'
assert pp_calls == [('t1', 'b1')]
def test_inprogress_reflects_progress_without_changing_status():
task = _task()
task_status = {'status': 'downloading', 'progress': 0}
deps = _make_deps(
{'dl-1': _FakeDownloadStatus(id='dl-1', state='InProgress, Downloading', progress=42.5)},
)
status_mod._apply_engine_state_fallback('t1', task, task_status, 'b1', deps)
assert task['status'] == 'downloading'
assert task_status['status'] == 'downloading'
assert task_status['progress'] == 42.5
# ---------------------------------------------------------------------------
# Gates — bail without mutating state
# ---------------------------------------------------------------------------
def test_skips_when_orchestrator_missing():
task = _task()
task_status = {'status': 'downloading', 'progress': 0}
deps = status_mod.StatusDeps(
config_manager=MagicMock(),
docker_resolve_path=lambda p: p,
find_completed_file=lambda *a, **k: (None, None),
make_context_key=lambda u, f: f"{u}::{f}",
submit_post_processing=lambda *a: None,
get_cached_transfer_data=lambda: {},
download_orchestrator=None,
)
status_mod._apply_engine_state_fallback('t1', task, task_status, 'b1', deps)
assert task['status'] == 'downloading' # unchanged
def test_skips_terminal_states():
"""Already-failed / completed / cancelled tasks must not be touched —
they may have been marked by another path (e.g. the live_transfers
branch on a slskd Errored state for a Soulseek manual pick)."""
for terminal in ('completed', 'failed', 'cancelled', 'not_found', 'post_processing'):
task = _task(status=terminal)
task_status = {'status': terminal, 'progress': 0}
deps = _make_deps({'dl-1': _FakeDownloadStatus(id='dl-1', state='Errored')})
status_mod._apply_engine_state_fallback('t1', task, task_status, 'b1', deps)
assert task['status'] == terminal
def test_skips_soulseek_username():
"""Soulseek goes through live_transfers_lookup — never the engine
fallback. Otherwise we'd double-process its terminal state."""
task = _task(username='peer-username-xyz') # not in _STREAMING_SOURCE_NAMES
task_status = {'status': 'downloading', 'progress': 0}
deps = _make_deps({'dl-1': _FakeDownloadStatus(id='dl-1', state='Errored')})
status_mod._apply_engine_state_fallback('t1', task, task_status, 'b1', deps)
assert task['status'] == 'downloading'
def test_skips_when_download_id_missing():
task = _task()
task.pop('download_id')
task_status = {'status': 'downloading', 'progress': 0}
deps = _make_deps({})
status_mod._apply_engine_state_fallback('t1', task, task_status, 'b1', deps)
assert task['status'] == 'downloading'
def test_engine_returning_none_leaves_task_alone():
"""Engine doesn't know about this download_id (worker hasn't registered
yet, or the record was cleaned). The fallback must not falsely mark
the task failed in this case — the safety valve covers stuck-forever."""
task = _task()
task_status = {'status': 'downloading', 'progress': 0}
deps = _make_deps({'dl-1': None})
status_mod._apply_engine_state_fallback('t1', task, task_status, 'b1', deps)
assert task['status'] == 'downloading'
def test_skips_auto_attempts_without_manual_pick_flag():
"""Auto attempts (no _user_manual_pick flag) must NOT hit the engine
fallback even if they end up in the else branch. The monitor's retry
path owns auto-attempt failure handling — short-circuiting it here
would skip the fallback-to-next-candidate behavior."""
task = _task(manual_pick=False)
task_status = {'status': 'downloading', 'progress': 0}
deps = _make_deps({'dl-1': _FakeDownloadStatus(id='dl-1', state='Errored')})
status_mod._apply_engine_state_fallback('t1', task, task_status, 'b1', deps)
# Untouched — auto retry path will handle it.
assert task['status'] == 'downloading'
# ---------------------------------------------------------------------------
# Live-transfers IF branch — manual-pick failure path
# ---------------------------------------------------------------------------
#
# Streaming-source records are pre-populated into ``live_transfers_lookup``
# via ``download_orchestrator.engine.get_all_downloads(exclude=('soulseek',))``,
# so a manually-picked SoundCloud / YouTube / Tidal / etc. download whose
# engine record reports ``state='Errored'`` arrives via the IF branch
# (lookup_key IS in live_transfers_lookup), NOT the engine-fallback else
# branch. Without the manual-pick guard inside that elif, the live-
# transfers branch would defer to the monitor — which itself bails on
# manual picks — and the task would sit at "downloading 0%" forever.
#
# These tests exercise ``build_batch_status_data`` end-to-end so the
# guard is pinned by behavior rather than by the unit-level fallback
# tests above.
def _seed_runtime(batch_id, task_id, *, manual_pick: bool):
import time as _t
from core.runtime_state import download_batches, download_tasks, tasks_lock
with tasks_lock:
download_tasks[task_id] = {
'status': 'downloading',
'username': 'soundcloud',
'filename': '1234||https://sc/x||Display Name',
'download_id': 'dl-1',
'_user_manual_pick': manual_pick,
'track_info': {'name': 'Test Track'},
'track_index': 0,
# Recent so the safety-valve "stuck-too-long" branch doesn't fire.
'status_change_time': _t.time(),
'cached_candidates': [],
}
download_batches[batch_id] = {
'phase': 'downloading',
'queue': [task_id],
'analysis_results': [],
'active_count': 1,
'max_concurrent': 3,
}
def _clear_runtime():
from core.runtime_state import download_batches, download_tasks, tasks_lock
with tasks_lock:
download_tasks.clear()
download_batches.clear()
@pytest.fixture
def runtime():
"""Seeded download_batches + download_tasks; cleared after each test."""
_clear_runtime()
yield
_clear_runtime()
def _build_batch_deps(completed_calls):
"""StatusDeps with a long timeout so the safety valve doesn't fire +
a captured ``on_download_completed`` we can assert against."""
fake_config = MagicMock()
fake_config.get = lambda key, default=None: 99999 if key == 'soulseek.download_timeout' else default
return status_mod.StatusDeps(
config_manager=fake_config,
docker_resolve_path=lambda p: p,
find_completed_file=lambda *a, **k: (None, None),
make_context_key=lambda u, f: f"{u}::{f}",
submit_post_processing=lambda *a: None,
get_cached_transfer_data=lambda: {},
download_orchestrator=None, # IF branch doesn't need engine
run_async=None,
on_download_completed=lambda batch_id, task_id, success: completed_calls.append(
(batch_id, task_id, success)
),
)
def test_if_branch_manual_pick_marks_failed_on_errored(runtime):
"""Manual-pick task whose live_transfers entry reports Errored —
must transition to 'failed' synchronously, not defer to the monitor."""
import time as _t
from core.runtime_state import download_batches
batch_id = 'b1'
task_id = 't1'
_seed_runtime(batch_id, task_id, manual_pick=True)
completed_calls = []
deps = _build_batch_deps(completed_calls)
live_transfers_lookup = {
'soundcloud::1234||https://sc/x||Display Name': {
'state': 'Errored',
'percentComplete': 0,
'errorMessage': 'HTTP 404 Not Found',
}
}
response = status_mod.build_batch_status_data(
batch_id, download_batches[batch_id], live_transfers_lookup, deps,
)
task_status = response['tasks'][0]
assert task_status['status'] == 'failed'
assert 'HTTP 404' in (task_status.get('error_message') or '')
from core.runtime_state import download_tasks
assert download_tasks[task_id]['status'] == 'failed'
# on_download_completed is deferred to a daemon thread — wait briefly.
for _ in range(50):
if completed_calls:
break
_t.sleep(0.01)
assert completed_calls == [(batch_id, task_id, False)]
def test_if_branch_compound_completed_errored_hits_manual_pick_failure(runtime):
"""``"Completed, Errored"`` must trigger the failure branch, not the
success branch. Slskd / engine pluginscan emit compound states when
a download technically completes but the file is corrupt / partial."""
from core.runtime_state import download_batches
batch_id = 'b1'
task_id = 't1'
_seed_runtime(batch_id, task_id, manual_pick=True)
deps = _build_batch_deps([])
live_transfers_lookup = {
'soundcloud::1234||https://sc/x||Display Name': {
'state': 'Completed, Errored',
'percentComplete': 100,
}
}
response = status_mod.build_batch_status_data(
batch_id, download_batches[batch_id], live_transfers_lookup, deps,
)
assert response['tasks'][0]['status'] == 'failed'
def test_if_branch_auto_attempt_defers_to_monitor(runtime):
"""Auto attempts (no manual-pick flag) keep the original "let monitor
handle retry" behavior — task stays in its current pre-error status
so the monitor's retry path can detect the Errored live_info on its
next tick. This is the byte-identical pre-fix behavior; the guard is
additive."""
from core.runtime_state import download_batches, download_tasks
batch_id = 'b1'
task_id = 't1'
_seed_runtime(batch_id, task_id, manual_pick=False)
deps = _build_batch_deps([])
live_transfers_lookup = {
'soundcloud::1234||https://sc/x||Display Name': {
'state': 'Errored',
'percentComplete': 0,
}
}
status_mod.build_batch_status_data(
batch_id, download_batches[batch_id], live_transfers_lookup, deps,
)
# Auto retry path keeps the task in 'downloading' so the monitor can
# observe the Errored state on its own poll. NOT marked failed here.
assert download_tasks[task_id]['status'] == 'downloading'
def test_orchestrator_exception_swallowed():
"""If get_download_status raises, the fallback logs + bails — it must
not propagate and crash the whole status response."""
task = _task()
task_status = {'status': 'downloading', 'progress': 0}
fake_orch = MagicMock()
async def _boom(_):
raise RuntimeError("network blip")
fake_orch.get_download_status = _boom
deps = status_mod.StatusDeps(
config_manager=MagicMock(),
docker_resolve_path=lambda p: p,
find_completed_file=lambda *a, **k: (None, None),
make_context_key=lambda u, f: f"{u}::{f}",
submit_post_processing=lambda *a: None,
get_cached_transfer_data=lambda: {},
download_orchestrator=fake_orch,
run_async=lambda coro: __import__('asyncio').new_event_loop().run_until_complete(coro),
)
status_mod._apply_engine_state_fallback('t1', task, task_status, 'b1', deps)
assert task['status'] == 'downloading'