Album bundle: let Soulseek staging-misses fall through to per-track/cross-source fallback (#743)
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.
This commit is contained in:
parent
163de6c146
commit
2824c25ec6
2 changed files with 57 additions and 13 deletions
|
|
@ -34,11 +34,22 @@ logger = logging.getLogger(__name__)
|
||||||
def _private_album_bundle_staging_miss_reason(batch_id: Optional[str], deps: Any) -> Optional[str]:
|
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.
|
"""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
|
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
|
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
|
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:
|
if not batch_id:
|
||||||
return None
|
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')
|
batch.get('album_bundle_private_staging')
|
||||||
and batch.get('album_bundle_state') == 'staged'
|
and batch.get('album_bundle_state') == 'staged'
|
||||||
and not batch.get('album_bundle_partial')
|
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))
|
and (mode == source or (mode == 'hybrid' and hybrid_first == source))
|
||||||
):
|
):
|
||||||
return f'Track was not found in the staged {source} album release'
|
return f'Track was not found in the staged {source} album release'
|
||||||
|
|
|
||||||
|
|
@ -223,7 +223,10 @@ def test_private_torrent_album_staging_miss_skips_per_track_search():
|
||||||
assert ('done', ('b1', 't1', False), {}) in rec.calls
|
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={
|
_seed_task(track_info={
|
||||||
'id': 'sp-1', 'name': 'Song', 'artists': ['Artist'],
|
'id': 'sp-1', 'name': 'Song', 'artists': ['Artist'],
|
||||||
'album': 'Album', 'duration_ms': 180000,
|
'album': 'Album', 'duration_ms': 180000,
|
||||||
|
|
@ -231,9 +234,9 @@ def test_private_soulseek_album_staging_miss_skips_per_track_search():
|
||||||
download_batches['b1'] = {
|
download_batches['b1'] = {
|
||||||
'album_bundle_private_staging': True,
|
'album_bundle_private_staging': True,
|
||||||
'album_bundle_state': 'staged',
|
'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()
|
rec = _Recorder()
|
||||||
deps, _ = _build_deps(
|
deps, _ = _build_deps(
|
||||||
soulseek=client,
|
soulseek=client,
|
||||||
|
|
@ -246,11 +249,43 @@ def test_private_soulseek_album_staging_miss_skips_per_track_search():
|
||||||
|
|
||||||
assert client.search_calls == []
|
assert client.search_calls == []
|
||||||
assert download_tasks['t1']['status'] == 'not_found'
|
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
|
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={
|
_seed_task(track_info={
|
||||||
'id': 'sp-1', 'name': 'Song', 'artists': ['Artist'],
|
'id': 'sp-1', 'name': 'Song', 'artists': ['Artist'],
|
||||||
'album': 'Album', 'duration_ms': 180000,
|
'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',
|
'album_bundle_source': 'soulseek',
|
||||||
}
|
}
|
||||||
client = _FakeClient(
|
client = _FakeClient(
|
||||||
results=['should-not-search'],
|
results=[],
|
||||||
mode='hybrid',
|
mode='hybrid',
|
||||||
subclients={'hybrid_order': ['soulseek', 'hifi']},
|
subclients={'hybrid_order': ['soulseek', 'hifi']},
|
||||||
)
|
)
|
||||||
rec = _Recorder()
|
|
||||||
deps, _ = _build_deps(
|
deps, _ = _build_deps(
|
||||||
soulseek=client,
|
soulseek=client,
|
||||||
matching=_FakeMatchEngine(queries=['Artist Song']),
|
matching=_FakeMatchEngine(queries=['Artist Song']),
|
||||||
try_staging_match=lambda *a, **kw: False,
|
try_staging_match=lambda *a, **kw: False,
|
||||||
on_download_completed=rec('done'),
|
|
||||||
)
|
)
|
||||||
|
|
||||||
tw.download_track_worker('t1', 'b1', deps)
|
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 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():
|
def test_partial_private_hybrid_first_soulseek_album_staging_miss_allows_per_track_search():
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue