Wishlist: only engage album-bundle when multiple tracks from same album (PR 1/4)

Real-world wishlist case the original c3b88e69 design missed: user with
26 missing tracks from 26 different albums. Each item used to promote
to its own album-bundle sub-batch (``min_tracks_per_album=1``), which
downloaded the ENTIRE album (5-42 files) to claim one track. Confirmed
in app.log:

- "Licensed To Ill" downloaded 3 times across cycles (3-4 files each)
- "The Understanding" 17 files for 1 wishlist track
- "Alright, Still" 42 files for 1 wishlist track
- ~85% wasted bandwidth, slskd hammered with 26 concurrent searches

PR 1 of a 4-PR fix series — see commit body footer for the other PRs.

Default ``min_tracks_per_album`` 1 → 2. Single-track wishlist items
fall to ``residual_tracks`` → classic per-track batch (already works,
already efficient). Album-bundle kept for the case it was designed
for: user has 2+ tracks missing from the same album.

Override via the new ``wishlist.album_bundle_min_tracks`` config key:
- 1 = previous behaviour (bundle every item)
- 2 = new default
- 3+ = stricter, for users who want bundle only on bigger gaps

Helper ``_resolve_album_bundle_threshold`` lives in
``core/wishlist/processing.py``. Defensive shape mirrors the existing
config-driven knobs (``get_poll_interval`` / ``get_transient_miss_threshold``):
non-numeric, non-positive, or config-manager-raise all fall back to
the safe default. Three test cases pin the fallback chain.

Both wishlist entry points wired through the same helper:
- ``process_wishlist_automatically`` (auto cycle, line 812)
- ``start_manual_wishlist_download_batch`` (manual run, line 539)

Tests:
- ``tests/wishlist/test_album_grouping.py`` — old ``test_default_threshold_promotes_solo_albums`` flipped to ``test_default_threshold_demotes_solo_albums`` with explanatory docstring naming the real-world cause. New ``test_default_threshold_promotes_multi_track_albums`` pins the 2+ promotion. New ``test_explicit_threshold_one_restores_solo_promotion`` pins that the kwarg still works for opt-back-in.
- ``tests/wishlist/test_processing.py`` — 3 new tests for ``_resolve_album_bundle_threshold``: default-when-config-missing, honors-config-override, falls-back-on-garbage.
- ``tests/wishlist/test_automation.py`` — ``test_wishlist_albums_cycle_splits_into_per_album_batches`` updated to use 2+ tracks per album (5 tracks across 2 albums instead of 3 across 2 with 1 solo). ``test_wishlist_albums_cycle_residual_for_orphan_tracks`` updated to include 2 tracks from Album One so it still promotes.
- ``tests/wishlist/test_manual_download.py`` — same shape update for the manual path test.
- ``tests/wishlist/test_album_grouping.py:test_multiple_albums_emit_separate_groups`` updated to reflect new default (alb1 with 2 tracks promotes, alb2 with 1 track goes residual).
- ``tests/wishlist/test_album_grouping.py:test_nested_track_data_payloads_normalized`` pinned with explicit ``min_tracks_per_album=1`` so the test stays focused on payload-shape parsing, not the threshold rule.

114 wishlist tests pass; 866 across wishlist + automation + downloads +
album_bundle + album_bundle_dispatch suites still green. Ruff clean.

Sibling PRs queued in TaskCreate:
- PR 2 — investigate post-process staging-match miss (the second-order
  bug that causes the same album to redownload every cycle when the
  staging step doesn't claim the requested track).
- PR 3 — fix sibling-completion gate that fires on first sibling
  instead of last (log evidence: run a4945c88 finalized 1/26 batches).
- PR 4 — UI distinguish Queued from Analyzing for batches waiting
  on the executor (23/26 batches sit at "Analyzing..." while really
  queued at max_workers=3).
This commit is contained in:
Broque Thomas 2026-05-27 13:42:04 -07:00
parent b0df627e18
commit dd32e3bbe1
7 changed files with 203 additions and 40 deletions

View file

@ -119,16 +119,26 @@ class WishlistGroupingResult:
def group_wishlist_tracks_by_album(
tracks: List[Dict[str, Any]],
*,
min_tracks_per_album: int = 1,
min_tracks_per_album: int = 2,
) -> WishlistGroupingResult:
"""Group wishlist tracks by their owning album.
``min_tracks_per_album`` controls the threshold for promoting an
album to its own sub-batch. Default ``1`` means even a single
missing track gets the album-bundle treatment (which is what the
user wants for releases where they only need one track from the
album). Set higher to require multiple missing tracks before
engaging the bundle search.
album to its own sub-batch. Default ``2`` means an album needs at
least two missing tracks before the album-bundle search engages
single-track items fall to ``residual_tracks`` and take the
classic per-track path. The 1-track case used to default to bundle
too, but real-world wishlists frequently look like "26 single
tracks from 26 different albums," and engaging bundle for each
one downloads ~85% of bandwidth as unwanted files, hammers slskd
with concurrent searches, and re-downloads the same album every
cycle when the staging-match step doesn't claim the requested
track. Bundle shines when several tracks from the same album are
missing that's the case worth the bandwidth premium.
Override via the ``wishlist.album_bundle_min_tracks`` config key
or by passing ``min_tracks_per_album=N`` explicitly (kept for
tests + power users who want different behaviour).
"""
result = WishlistGroupingResult()
if not tracks:

View file

@ -20,6 +20,33 @@ module_logger = get_logger("wishlist.processing")
logger = module_logger
# Album-bundle is wasteful for single-track wishlist items (downloads
# the entire album to claim one file), so the bundle path only engages
# when an album has ``N`` or more missing tracks in the wishlist. Default
# 2 catches the "this user is missing several tracks from one album"
# case while keeping single-track-per-album items on the cheaper
# per-track flow. Configurable via ``wishlist.album_bundle_min_tracks``
# for users who want different behaviour.
_DEFAULT_ALBUM_BUNDLE_MIN_TRACKS = 2
def _resolve_album_bundle_threshold() -> int:
"""Return the configured min-tracks-per-album threshold for the
wishlist album-bundle grouper. Falls back to the default when the
config key is missing or carries garbage same defensive shape as
the rest of the config-driven knobs in core/."""
try:
from config.settings import config_manager
raw = config_manager.get('wishlist.album_bundle_min_tracks',
_DEFAULT_ALBUM_BUNDLE_MIN_TRACKS)
value = int(raw)
if value >= 1:
return value
except Exception: # noqa: S110 — defensive config-read fallback; uses default below
pass
return _DEFAULT_ALBUM_BUNDLE_MIN_TRACKS
@dataclass
class WishlistAutoProcessingRuntime:
"""Dependencies needed to run automatic wishlist processing outside the controller."""
@ -533,7 +560,10 @@ def _prepare_and_run_manual_wishlist_batch(
# Tracks the grouper can't bucket fall through to a residual
# batch with the classic per-track flow.
from core.wishlist.album_grouping import group_wishlist_tracks_by_album
grouping = group_wishlist_tracks_by_album(wishlist_tracks)
grouping = group_wishlist_tracks_by_album(
wishlist_tracks,
min_tracks_per_album=_resolve_album_bundle_threshold(),
)
# Build the final payload list (batch_id, tracks, album_context,
# artist_context, is_album). The first payload re-uses the
@ -806,7 +836,10 @@ def process_wishlist_automatically(runtime: WishlistAutoProcessingRuntime, autom
_submitted_batches: list[str] = []
if current_cycle == 'albums':
from core.wishlist.album_grouping import group_wishlist_tracks_by_album
grouping = group_wishlist_tracks_by_album(wishlist_tracks)
grouping = group_wishlist_tracks_by_album(
wishlist_tracks,
min_tracks_per_album=_resolve_album_bundle_threshold(),
)
else:
grouping = None

View file

@ -55,20 +55,19 @@ def test_single_album_groups_all_tracks_together():
def test_multiple_albums_emit_separate_groups():
"""Two tracks in alb1 promotes that album to a group at the default
threshold of 2; alb2's one solo track falls to residual."""
tracks = [
_wt('Song A', 'Artist 1', 'alb1', 'Album 1'),
_wt('Song B', 'Artist 1', 'alb1', 'Album 1'),
_wt('Song C', 'Artist 2', 'alb2', 'Album 2'),
]
res = group_wishlist_tracks_by_album(tracks)
assert len(res.album_groups) == 2
keys = {g.album_key for g in res.album_groups}
assert keys == {'alb1', 'alb2'}
for g in res.album_groups:
if g.album_key == 'alb1':
assert len(g.tracks) == 2
else:
assert len(g.tracks) == 1
assert len(res.album_groups) == 1
assert res.album_groups[0].album_key == 'alb1'
assert len(res.album_groups[0].tracks) == 2
assert len(res.residual_tracks) == 1
assert res.residual_tracks[0]['track_name'] == 'Song C'
def test_missing_album_metadata_falls_through_to_residual():
@ -99,9 +98,9 @@ def test_missing_artist_demotes_to_residual():
def test_min_tracks_threshold_demotes_solos():
"""When ``min_tracks_per_album=2``, single-track albums fall to
residual so the user doesn't fire a bundle search for a 1-track
rip when per-track would do."""
"""When ``min_tracks_per_album=2`` (the default), single-track
albums fall to residual so the user doesn't fire a bundle search
for a 1-track rip when per-track would do."""
tracks = [
_wt('Solo Track', 'Artist 1', 'alb1', 'Album 1'),
_wt('Song A', 'Artist 2', 'alb2', 'Album 2'),
@ -114,12 +113,44 @@ def test_min_tracks_threshold_demotes_solos():
assert res.residual_tracks[0]['track_name'] == 'Solo Track'
def test_default_threshold_promotes_solo_albums():
"""Default ``min_tracks_per_album=1`` — even one missing track
triggers the album-bundle path. Matches the user's stated
preference (don't gate on track count)."""
def test_default_threshold_demotes_solo_albums():
"""Default ``min_tracks_per_album=2`` — a wishlist with one missing
track from one album falls to residual so the per-track flow
handles it instead of engaging album-bundle for a single file.
Real-world reason: typical wishlist looks like "26 single tracks
from 26 different albums" and bundling each one downloads ~85%
bandwidth as unwanted files, hammers slskd with concurrent
searches, and re-downloads the same album every cycle when the
staging-match step doesn't claim the requested track."""
tracks = [_wt('Solo', 'Artist 1', 'alb1', 'Album 1')]
res = group_wishlist_tracks_by_album(tracks)
assert res.album_groups == []
assert len(res.residual_tracks) == 1
assert res.residual_tracks[0]['track_name'] == 'Solo'
def test_default_threshold_promotes_multi_track_albums():
"""Default still promotes albums with 2+ missing tracks — the
album-bundle path is the right tool when several files from the
same release are missing (downloading 5 tracks to claim 2 still
beats five separate per-track searches against the same album)."""
tracks = [
_wt('Song A', 'Artist', 'alb1', 'Album'),
_wt('Song B', 'Artist', 'alb1', 'Album'),
]
res = group_wishlist_tracks_by_album(tracks)
assert len(res.album_groups) == 1
assert len(res.album_groups[0].tracks) == 2
assert res.residual_tracks == []
def test_explicit_threshold_one_restores_solo_promotion():
"""Power users / tests can opt back into the old "bundle even one
track" behaviour by passing ``min_tracks_per_album=1`` explicitly.
Default change is opt-out via the public kwarg, not a removed
capability."""
tracks = [_wt('Solo', 'Artist 1', 'alb1', 'Album 1')]
res = group_wishlist_tracks_by_album(tracks, min_tracks_per_album=1)
assert len(res.album_groups) == 1
assert res.residual_tracks == []
@ -154,6 +185,8 @@ def test_nested_track_data_payloads_normalized():
},
},
}]
res = group_wishlist_tracks_by_album(tracks)
# Use threshold=1 here — the test is about payload-shape parsing,
# not the threshold rule, so don't conflate the two.
res = group_wishlist_tracks_by_album(tracks, min_tracks_per_album=1)
assert len(res.album_groups) == 1
assert res.album_groups[0].album_key == 'a'

View file

@ -238,14 +238,17 @@ def test_process_wishlist_automatically_creates_batch_for_matching_tracks():
def test_wishlist_albums_cycle_splits_into_per_album_batches():
"""Multi-album wishlist run: each album emits its own sub-batch
"""Multi-album wishlist run: each album with at least the
threshold of missing tracks (default 2) emits its own sub-batch
with ``is_album_download=True`` + populated album/artist context.
Pinned so the album-bundle dispatch gate (which keys on those
fields) engages per album instead of falling through to per-track
on a single mixed batch."""
Single-track-per-album items fall to residual and take the
per-track path. Pinned so the album-bundle dispatch gate (which
keys on those fields) engages per album instead of falling through
to per-track on a single mixed batch."""
batch_map = {}
runtime, _service, _profiles_db, music_db, executor, _logger, _progress, _guards = _build_runtime(
tracks=[
# Album one: 2 missing tracks → promotes to album-bundle.
{
"name": "Song A1",
"artists": [{"name": "Artist 1"}],
@ -262,6 +265,7 @@ def test_wishlist_albums_cycle_splits_into_per_album_batches():
"artists": [{"name": "Artist 1"}],
},
},
# Album two: 3 missing tracks → also promotes.
{
"name": "Song B1",
"artists": [{"name": "Artist 2"}],
@ -270,9 +274,25 @@ def test_wishlist_albums_cycle_splits_into_per_album_batches():
"artists": [{"name": "Artist 2"}],
},
},
{
"name": "Song B2",
"artists": [{"name": "Artist 2"}],
"spotify_data": {
"album": {"id": "alb2", "name": "Album Two", "album_type": "album"},
"artists": [{"name": "Artist 2"}],
},
},
{
"name": "Song B3",
"artists": [{"name": "Artist 2"}],
"spotify_data": {
"album": {"id": "alb2", "name": "Album Two", "album_type": "album"},
"artists": [{"name": "Artist 2"}],
},
},
],
cycle_value="albums",
count=3,
count=5,
batch_map=batch_map,
)
@ -290,7 +310,7 @@ def test_wishlist_albums_cycle_splits_into_per_album_batches():
submitted_track_lists = [submitted_args[2] for _fn, submitted_args, _kw in executor.submissions]
track_counts = sorted(len(tracks) for tracks in submitted_track_lists)
assert track_counts == [1, 2]
assert track_counts == [2, 3]
# All sub-batches of one wishlist invocation share a single
# ``wishlist_run_id`` so the completion handler can gate the
@ -303,13 +323,16 @@ def test_wishlist_albums_cycle_splits_into_per_album_batches():
def test_wishlist_albums_cycle_residual_for_orphan_tracks():
"""Tracks without resolvable album metadata fall to the classic
per-track residual batch (no ``is_album_download`` flag), while
sibling tracks with valid album info still get their own
album-bundle sub-batch."""
sibling tracks with valid album info AND enough missing tracks to
clear the bundle threshold still get their own album-bundle
sub-batch. With the default threshold bumped to 2, the album side
needs at least two tracks from the same album to promote."""
batch_map = {}
runtime, _service, _profiles_db, music_db, executor, _logger, _progress, _guards = _build_runtime(
tracks=[
# Album side: two missing tracks from Album One → promotes.
{
"name": "Real Album Track",
"name": "Real Album Track 1",
"artists": [{"name": "Artist 1"}],
"spotify_data": {
"album": {"id": "alb1", "name": "Album One", "album_type": "album"},
@ -317,14 +340,22 @@ def test_wishlist_albums_cycle_residual_for_orphan_tracks():
},
},
{
# No album id, no album name — orphan
"name": "Real Album Track 2",
"artists": [{"name": "Artist 1"}],
"spotify_data": {
"album": {"id": "alb1", "name": "Album One", "album_type": "album"},
"artists": [{"name": "Artist 1"}],
},
},
# Orphan: no album id, no album name → residual.
{
"name": "Orphan",
"artists": [{"name": "X"}],
"spotify_data": {"album": {"album_type": "album"}, "artists": [{"name": "X"}]},
},
],
cycle_value="albums",
count=2,
count=3,
batch_map=batch_map,
)
@ -337,6 +368,7 @@ def test_wishlist_albums_cycle_residual_for_orphan_tracks():
assert len(album_batches) == 1
assert len(residual_batches) == 1
assert album_batches[0]["album_context"]["name"] == "Album One"
assert album_batches[0]["analysis_total"] == 2
assert residual_batches[0]["analysis_total"] == 1

View file

@ -234,15 +234,17 @@ def test_start_manual_wishlist_download_batch_does_not_run_library_cleanup():
def test_manual_wishlist_splits_into_per_album_sub_batches():
"""Manual wishlist run with multi-album content splits into one
sub-batch per album. Each sub-batch flips
``is_album_download=True`` + populates album/artist context so
the slskd / Prowlarr album-bundle dispatch engages.
sub-batch per album that has at least the threshold of missing
tracks (default 2). Each sub-batch flips ``is_album_download=True``
+ populates album/artist context so the slskd / Prowlarr
album-bundle dispatch engages.
Pinned to verify the manual path matches the auto path's
behavior the user's first real-world test hit the manual
flow, not the auto flow."""
runtime, _service, _db, executor, _logger, _activity, batch_map, master_calls = _build_runtime(
tracks=[
# Album one: 2 missing tracks → promotes to album-bundle.
{
"id": "trk-a1",
"spotify_track_id": "trk-a1",
@ -263,6 +265,7 @@ def test_manual_wishlist_splits_into_per_album_sub_batches():
"artists": [{"name": "Artist 1"}],
},
},
# Album two: 2 missing tracks → also promotes.
{
"id": "trk-b1",
"spotify_track_id": "trk-b1",
@ -273,6 +276,16 @@ def test_manual_wishlist_splits_into_per_album_sub_batches():
"artists": [{"name": "Artist 2"}],
},
},
{
"id": "trk-b2",
"spotify_track_id": "trk-b2",
"name": "Song B2",
"artists": [{"name": "Artist 2"}],
"spotify_data": {
"album": {"id": "alb2", "name": "Album Two", "album_type": "album"},
"artists": [{"name": "Artist 2"}],
},
},
]
)
@ -294,9 +307,9 @@ def test_manual_wishlist_splits_into_per_album_sub_batches():
assert second_args[0] in batch_map
assert batch_map[second_args[0]].get("is_album_download") is True
# Track counts across the two sub-batches sum to the wishlist total.
# Track counts across the two sub-batches: 2 each at threshold=2.
counts = sorted(len(args[2]) for args, _ in master_calls)
assert counts == [1, 2]
assert counts == [2, 2]
# Both sub-batches carry album context populated from spotify_data.
album_names = {

View file

@ -1,8 +1,49 @@
from contextlib import contextmanager
from unittest.mock import patch
from core.wishlist import processing
# ---------------------------------------------------------------------------
# _resolve_album_bundle_threshold — config-driven wishlist bundle threshold
# ---------------------------------------------------------------------------
def test_resolve_album_bundle_threshold_uses_default_when_config_missing():
"""Default 2 matches the bumped ``min_tracks_per_album`` floor in
``group_wishlist_tracks_by_album`` single-track wishlist items
take the per-track path, multi-track items take the bundle path."""
with patch('config.settings.config_manager') as cm:
cm.get.return_value = processing._DEFAULT_ALBUM_BUNDLE_MIN_TRACKS
assert processing._resolve_album_bundle_threshold() == 2
def test_resolve_album_bundle_threshold_honors_config_override():
"""Power users who want bundle for every wishlist track (old
behaviour) can set the key to 1. Users who want bundle only on
bigger gaps can set higher."""
with patch('config.settings.config_manager') as cm:
cm.get.return_value = 1
assert processing._resolve_album_bundle_threshold() == 1
cm.get.return_value = 5
assert processing._resolve_album_bundle_threshold() == 5
def test_resolve_album_bundle_threshold_falls_back_on_garbage():
"""Non-numeric / non-positive / config-manager-raise all fall back
to the safe default. Same defensive shape as get_poll_interval /
get_transient_miss_threshold."""
with patch('config.settings.config_manager') as cm:
cm.get.return_value = 'oops'
assert processing._resolve_album_bundle_threshold() == 2
cm.get.return_value = 0
assert processing._resolve_album_bundle_threshold() == 2
cm.get.return_value = -3
assert processing._resolve_album_bundle_threshold() == 2
cm.get.side_effect = RuntimeError('boom')
assert processing._resolve_album_bundle_threshold() == 2
class _FakeLogger:
def __init__(self):
self.errors = []

View file

@ -3415,6 +3415,7 @@ function closeHelperSearch() {
const WHATS_NEW = {
'2.6.3': [
{ unreleased: true },
{ title: 'Wishlist: only engage album-bundle when several tracks from the same album are missing', desc: 'auto-wishlist + manual-wishlist runs were promoting every single-track wishlist item to a per-album bundle search — so a wishlist of "26 single tracks from 26 different albums" downloaded full albums (5-42 files each, ~85% wasted bandwidth) and hammered slskd with concurrent searches. now the bundle path only engages when an album has 2 or more missing tracks in the wishlist; single-track items take the cheaper per-track path that already works fine. fewer slskd searches per cycle, less wasted bandwidth, no more downloading the same album three times in a row when staging-match misses a single track. configurable via `wishlist.album_bundle_min_tracks` if you want the old behaviour (set to 1) or stricter (set to 3+).' },
{ title: 'Auto-Sync: schedule playlists by weekday + time, not just by hour interval', desc: 'new Weekly Board tab on the Auto-Sync manager. drag a playlist onto a day column (Mon-Sun) to schedule it for that weekday at the default time, then click the card to open an editor for multi-day picks, custom time, and timezone. existing hourly schedules stay on the Hourly Board tab — one playlist gets exactly one schedule so swapping between weekly and hourly auto-replaces the old one. timezone defaults to whatever your browser reports (e.g. America/Los_Angeles); the editor accepts any IANA tz string. backend already had the plumbing from earlier 2.6.3 commits — this lights up the UI.', page: 'automations' },
{ title: 'Fix: usenet album downloads stuck on "downloading release" even after SAB finished', desc: 'when SAB moves a job from its queue into history (post-processing handoff — verify + unrar window), there\'s a brief gap where the job is in neither. SoulSync\'s poll treated one missing read as terminal failure and gave up silently, leaving the download modal frozen on "downloading release" forever. SAB queue states like `Pp` (post-processing) also weren\'t in the state map, hit the default-error fallback, and hung the poll the same way. now the poll tolerates ~10 seconds of transient misses, treats unmapped queue states as transient instead of silently looping, fires a terminal failed event on every failure path so the modal exits the downloading state, and queries SAB by nzo_id directly instead of paging the last-50-entries history (avoids losing the job on busy servers). same tolerance applies to torrent downloads — covers network blips that used to kill an otherwise healthy download.' },
{ title: 'Discogs: strip artist disambiguation suffixes everywhere', desc: 'Discogs marks duplicate-named artists with either `(N)` numeric suffixes ("Bullet (2)") or trailing asterisks ("John Smith*"). Only the `(N)` form was stripped, and only on the collection-import path — so artist + album search results in the UI surfaced raw "Foo*" or "Bar (2)" rows, and worse, import path inherited those characters into folder names on disk. centralized the cleanup into a single helper applied at every site a Discogs payload becomes a name string (artist search, album search, track lookups, get_artist_albums feature filtering). closes #634.' },