From 2824c25ec64c501c40347135935a37966df0a7c3 Mon Sep 17 00:00:00 2001 From: BoulderBadgeDad Date: Sun, 31 May 2026 11:13:08 -0700 Subject: [PATCH] Album bundle: let Soulseek staging-misses fall through to per-track/cross-source fallback (#743) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A Soulseek album bundle stages whichever single folder scored best. If that folder doesn't contain every track the album needs, the missing tracks were marked not_found with no fallback — even in hybrid mode where later sources (Deezer, YouTube, etc.) could fill them. The staging-miss short-circuit fired for Soulseek because 'soulseek' was lumped into the torrent/usenet source set when album bundles were added, and album_bundle_partial only reflects whether the files found IN the folder downloaded, not whether the folder had every needed track. Drop 'soulseek' from the short-circuit (keep torrent/usenet). A track not claimed from the staged Soulseek folder now falls through to the normal per-track Soulseek search and, in hybrid mode, onward down the configured chain. Unlike torrent/usenet — where per-track search re-adds the same release — Soulseek per-track search is a genuine per-file network search, so this is correct and cheap. Realizes the original author's stated intent ('keep partial bundles from blocking per-track fallback') robustly, since the partial flag couldn't detect a folder that was simply missing tracks. Only affects tracks NOT claimed from staging — fully-staged albums claim every track via try_staging_match and never reach this gate, so working albums are unchanged. Likely also mitigates #755 (all-album-import failures now fall through to per-track instead of dying). Tests: rewrote the two Soulseek staged-miss tests to assert fall-through (single + hybrid-first); kept the torrent guard; added a usenet guard test. --- core/downloads/task_worker.py | 17 ++++-- tests/downloads/test_downloads_task_worker.py | 53 +++++++++++++++---- 2 files changed, 57 insertions(+), 13 deletions(-) diff --git a/core/downloads/task_worker.py b/core/downloads/task_worker.py index 77cb803f..cc6751dc 100644 --- a/core/downloads/task_worker.py +++ b/core/downloads/task_worker.py @@ -34,11 +34,22 @@ logger = logging.getLogger(__name__) def _private_album_bundle_staging_miss_reason(batch_id: Optional[str], deps: Any) -> Optional[str]: """Return a user-facing miss reason when per-track search should stop. - Torrent / usenet / Soulseek album batches first download one private staged release, + Torrent / usenet album batches first download one private staged release, then each track claims the matching staged file. If that claim fails after the release is already staged, falling through to the normal per-track search only retries release-level sources N times and can keep re-adding - the same torrent. Treat the staged release as authoritative for this pass. + the same torrent/NZB. For those two sources we treat the staged release as + authoritative for this pass. + + Soulseek is deliberately NOT short-circuited. A Soulseek album bundle stages + whichever single folder scored best, and ``album_bundle_partial`` only + reflects whether the files found IN that folder downloaded — not whether the + folder actually contained every track the album needs. So a track the album + needs but that wasn't in the chosen folder would otherwise be marked + not_found with no fallback (#743). Unlike torrent/usenet, Soulseek per-track + search is a genuine per-file network search — it doesn't re-add a release — + so letting these misses fall through to the normal per-track flow (and, in + hybrid mode, onward to the next source) is correct and cheap. """ if not batch_id: return None @@ -60,7 +71,7 @@ def _private_album_bundle_staging_miss_reason(batch_id: Optional[str], deps: Any batch.get('album_bundle_private_staging') and batch.get('album_bundle_state') == 'staged' and not batch.get('album_bundle_partial') - and source in ('torrent', 'usenet', 'soulseek') + and source in ('torrent', 'usenet') and (mode == source or (mode == 'hybrid' and hybrid_first == source)) ): return f'Track was not found in the staged {source} album release' diff --git a/tests/downloads/test_downloads_task_worker.py b/tests/downloads/test_downloads_task_worker.py index a222b311..eea39ead 100644 --- a/tests/downloads/test_downloads_task_worker.py +++ b/tests/downloads/test_downloads_task_worker.py @@ -223,7 +223,10 @@ def test_private_torrent_album_staging_miss_skips_per_track_search(): assert ('done', ('b1', 't1', False), {}) in rec.calls -def test_private_soulseek_album_staging_miss_skips_per_track_search(): +def test_private_usenet_album_staging_miss_skips_per_track_search(): + # Usenet keeps the short-circuit (the #743 change is Soulseek-only): + # per-track NZB search re-adds the same release, so the staged release + # stays authoritative. _seed_task(track_info={ 'id': 'sp-1', 'name': 'Song', 'artists': ['Artist'], 'album': 'Album', 'duration_ms': 180000, @@ -231,9 +234,9 @@ def test_private_soulseek_album_staging_miss_skips_per_track_search(): download_batches['b1'] = { 'album_bundle_private_staging': True, 'album_bundle_state': 'staged', - 'album_bundle_source': 'soulseek', + 'album_bundle_source': 'usenet', } - client = _FakeClient(results=['should-not-search'], mode='soulseek') + client = _FakeClient(results=['should-not-search'], mode='usenet') rec = _Recorder() deps, _ = _build_deps( soulseek=client, @@ -246,11 +249,43 @@ def test_private_soulseek_album_staging_miss_skips_per_track_search(): assert client.search_calls == [] assert download_tasks['t1']['status'] == 'not_found' - assert 'staged soulseek album release' in download_tasks['t1']['error_message'] + assert 'staged usenet album release' in download_tasks['t1']['error_message'] assert ('done', ('b1', 't1', False), {}) in rec.calls -def test_private_hybrid_first_soulseek_album_staging_miss_skips_per_track_search(): +def test_private_soulseek_album_staging_miss_falls_through_to_per_track_search(): + # #743: a track the album needs but that wasn't in the staged Soulseek + # folder must NOT be short-circuited to not_found — it falls through to the + # normal per-track Soulseek search (unlike torrent/usenet). Even with + # album_bundle_partial unset (folder fully downloaded, just incomplete), + # Soulseek now always falls through. + _seed_task(track_info={ + 'id': 'sp-1', 'name': 'Song', 'artists': ['Artist'], + 'album': 'Album', 'duration_ms': 180000, + }) + download_batches['b1'] = { + 'album_bundle_private_staging': True, + 'album_bundle_state': 'staged', + 'album_bundle_source': 'soulseek', + } + client = _FakeClient(results=[], mode='soulseek') + deps, _ = _build_deps( + soulseek=client, + matching=_FakeMatchEngine(queries=['Artist Song']), + try_staging_match=lambda *a, **kw: False, + ) + + tw.download_track_worker('t1', 'b1', deps) + + assert client.search_calls # per-track search actually ran + assert download_tasks['t1']['status'] == 'not_found' # nothing found, but via search + assert 'staged soulseek album release' not in download_tasks['t1']['error_message'] + + +def test_private_hybrid_first_soulseek_album_staging_miss_falls_through_to_per_track_search(): + # Same as above but Soulseek is FIRST in a hybrid chain. The miss must fall + # through to per-track search, which (in hybrid) can then reach later + # sources — exactly the cross-source fallback #743 asks for. _seed_task(track_info={ 'id': 'sp-1', 'name': 'Song', 'artists': ['Artist'], 'album': 'Album', 'duration_ms': 180000, @@ -261,23 +296,21 @@ def test_private_hybrid_first_soulseek_album_staging_miss_skips_per_track_search 'album_bundle_source': 'soulseek', } client = _FakeClient( - results=['should-not-search'], + results=[], mode='hybrid', subclients={'hybrid_order': ['soulseek', 'hifi']}, ) - rec = _Recorder() deps, _ = _build_deps( soulseek=client, matching=_FakeMatchEngine(queries=['Artist Song']), try_staging_match=lambda *a, **kw: False, - on_download_completed=rec('done'), ) tw.download_track_worker('t1', 'b1', deps) - assert client.search_calls == [] + assert client.search_calls # per-track search ran (not short-circuited) assert download_tasks['t1']['status'] == 'not_found' - assert 'staged soulseek album release' in download_tasks['t1']['error_message'] + assert 'staged soulseek album release' not in download_tasks['t1']['error_message'] def test_partial_private_hybrid_first_soulseek_album_staging_miss_allows_per_track_search():