diff --git a/core/wishlist/album_grouping.py b/core/wishlist/album_grouping.py index 148ea263..ceb98c43 100644 --- a/core/wishlist/album_grouping.py +++ b/core/wishlist/album_grouping.py @@ -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: diff --git a/core/wishlist/processing.py b/core/wishlist/processing.py index fb2b9ac4..2bc7fcb3 100644 --- a/core/wishlist/processing.py +++ b/core/wishlist/processing.py @@ -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 diff --git a/tests/wishlist/test_album_grouping.py b/tests/wishlist/test_album_grouping.py index 9fc3264a..bb458415 100644 --- a/tests/wishlist/test_album_grouping.py +++ b/tests/wishlist/test_album_grouping.py @@ -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' diff --git a/tests/wishlist/test_automation.py b/tests/wishlist/test_automation.py index 2d01a7ab..1082d23b 100644 --- a/tests/wishlist/test_automation.py +++ b/tests/wishlist/test_automation.py @@ -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 diff --git a/tests/wishlist/test_manual_download.py b/tests/wishlist/test_manual_download.py index 36437727..7e1386ad 100644 --- a/tests/wishlist/test_manual_download.py +++ b/tests/wishlist/test_manual_download.py @@ -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 = { diff --git a/tests/wishlist/test_processing.py b/tests/wishlist/test_processing.py index 3fcc2104..5cfa638c 100644 --- a/tests/wishlist/test_processing.py +++ b/tests/wishlist/test_processing.py @@ -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 = [] diff --git a/webui/static/helper.js b/webui/static/helper.js index 2b9aae06..1861f23e 100644 --- a/webui/static/helper.js +++ b/webui/static/helper.js @@ -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.' },