From 99a763dace6684a8157fb70949052624faef1399 Mon Sep 17 00:00:00 2001 From: Broque Thomas <26755000+Nezreka@users.noreply.github.com> Date: Tue, 28 Apr 2026 20:30:12 -0700 Subject: [PATCH] fix: drop redundant library-cleanup pass from wishlist download flows MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both the auto and manual wishlist download paths called `remove_tracks_already_in_library` before submitting the batch — a serial DB lookup per track per artist (~1s/track on a 24-track wishlist). The batches set `force_download_all=True` which is explicitly documented as "skip the expensive library check" — the pre-flight cleanup was contradicting that flag. Removed the cleanup call from both flows. Kept `remove_wishlist_duplicates` (fast SQL DELETE) and the standalone `/api/wishlist/cleanup` endpoint that exposes the library scan as explicit user-triggered maintenance. Safety check on the trade-off: - post-processing at `core/imports/pipeline.py:576-624` already handles re-downloads defensively: existing file with metadata → skip overwrite + delete source duplicate, no library corruption. - Master worker's analysis loop normally removes wishlist entries for found tracks via `_check_and_remove_track_from_wishlist_by_metadata`, so stale wishlist entries should be rare in practice. - Worst case for the rare orphan: one redundant download attempt that the post-processing layer no-ops on. Bandwidth waste, not data damage. Tests updated: - `..._does_not_run_library_cleanup` (renamed from `_skips_enhance_tracks_during_cleanup`) asserts no DB track-existence checks happen and no wishlist removals fire — both `enhance` and "owned" tracks reach the master worker. - `..._marks_batch_complete_when_wishlist_genuinely_empty` (renamed from `..._after_cleanup`) covers the path where the wishlist starts empty. Full suite: 1232 passing. Ruff clean. --- core/wishlist/processing.py | 51 +++++++++----------------- tests/wishlist/test_manual_download.py | 43 ++++++++++++---------- 2 files changed, 41 insertions(+), 53 deletions(-) diff --git a/core/wishlist/processing.py b/core/wishlist/processing.py index a54ebe45..e774da4d 100644 --- a/core/wishlist/processing.py +++ b/core/wishlist/processing.py @@ -383,19 +383,12 @@ def _prepare_and_run_manual_wishlist_batch( if duplicates_removed > 0: logger.warning(f"[Manual-Wishlist] Removed {duplicates_removed} duplicate tracks") - logger.info("[Manual-Wishlist] Checking wishlist against library for already-owned tracks...") - cleanup_removed = remove_tracks_already_in_library( - wishlist_service, - SimpleNamespace(get_all_profiles=lambda: [{"id": manual_profile_id}]), - db, - runtime.active_server, - logger=logger, - skip_track_fn=lambda track: track.get('source_type') == 'enhance', - log_prefix="[Manual-Wishlist]", - ) - - if cleanup_removed > 0: - logger.info(f"[Manual-Wishlist] Cleaned up {cleanup_removed} already-owned tracks from wishlist") + # NOTE: We deliberately do NOT call remove_tracks_already_in_library here. + # Wishlist tracks are already known-missing (force_download_all=True is set on + # the batch). The library check duplicates the work the master worker would + # skip, and on large wishlists costs ~1s per track in serial DB lookups. + # The standalone /api/wishlist/cleanup endpoint still runs that pass when + # users explicitly ask for maintenance. raw_wishlist_tracks = wishlist_service.get_wishlist_tracks_for_download(profile_id=manual_profile_id) if not raw_wishlist_tracks: @@ -554,27 +547,19 @@ def process_wishlist_automatically(runtime: WishlistAutoProcessingRuntime, autom if duplicates_removed > 0: logger.warning(f"[Auto-Wishlist] Removed {duplicates_removed} duplicate tracks from profile {profile['id']}") - # CLEANUP: Remove tracks from wishlist that already exist in library - # This prevents wasting bandwidth on tracks we already have - logger.debug("[Auto-Wishlist] Checking wishlist against library for already-owned tracks...") - active_server = runtime.get_active_server() - cleanup_removed = remove_tracks_already_in_library( - wishlist_service, - database, - music_database, - active_server, - logger=logger, - ) + # NOTE: We deliberately do NOT call remove_tracks_already_in_library here. + # The batch sets force_download_all=True (see comment a few lines below), + # so wishlist tracks are treated as known-missing and the master worker + # skips per-track library lookups. Doing the same expensive scan here + # before submitting the batch defeats that optimization and adds + # ~1s per track in serial DB queries. The standalone + # /api/wishlist/cleanup endpoint still exposes that pass for users + # who want explicit maintenance. + runtime.update_automation_progress(automation_id, progress=25, phase='Preparing wishlist', + log_line='Skipped library scan — wishlist tracks treated as known-missing', + log_type='info') - if cleanup_removed > 0: - logger.info(f"[Auto-Wishlist] Cleaned up {cleanup_removed} already-owned tracks from wishlist") - runtime.update_automation_progress(automation_id, progress=25, phase='Cleaned up duplicates', - log_line=f'Removed {cleanup_removed} already-owned tracks', log_type='success') - else: - runtime.update_automation_progress(automation_id, progress=25, phase='Cleanup done', - log_line='No duplicates or already-owned tracks found', log_type='skip') - - # Get wishlist tracks for processing (after cleanup) - combine all profiles + # Get wishlist tracks for processing - combine all profiles raw_wishlist_tracks = [] for profile in all_profiles: raw_wishlist_tracks.extend(wishlist_service.get_wishlist_tracks_for_download(profile_id=profile['id'])) diff --git a/tests/wishlist/test_manual_download.py b/tests/wishlist/test_manual_download.py index 5a676f4e..b7a6b35d 100644 --- a/tests/wishlist/test_manual_download.py +++ b/tests/wishlist/test_manual_download.py @@ -184,8 +184,16 @@ def test_start_manual_wishlist_download_batch_filters_track_ids_and_starts_batch assert any("Filtered to 1 specific tracks by ID" in msg for msg in logger.info_messages) -def test_start_manual_wishlist_download_batch_skips_enhance_tracks_during_cleanup(): - runtime, service, _db, executor, logger, activity_calls, batch_map, master_calls = _build_runtime( +def test_start_manual_wishlist_download_batch_does_not_run_library_cleanup(): + """Manual flow does NOT scan the library for already-owned tracks. + + The batch sets force_download_all=True so owned tracks get downloaded + anyway. Running remove_tracks_already_in_library here would just add a + serial DB query per track (~30s on a 24-track wishlist) and contradict + force_download_all. The standalone /api/wishlist/cleanup endpoint + still exposes that pass for users who want explicit maintenance. + """ + runtime, service, db, executor, _logger, activity_calls, batch_map, master_calls = _build_runtime( tracks=[ { "id": "enhance-1", @@ -209,30 +217,25 @@ def test_start_manual_wishlist_download_batch_skips_enhance_tracks_during_cleanu assert status == 200 assert payload["success"] is True - # Run the bg job — cleanup happens here, not in the request handler. _run_submitted_bg_job(executor) - assert service.removed_ids == {"owned-1"} + # Owned-track removal pass does NOT run — wishlist still has the owned track. + assert service.removed_ids == set() + # The library check is skipped entirely — no per-track DB lookups. + assert db.track_checks == [] + + # All tracks are submitted to the master worker — including the "owned" one. assert len(master_calls) == 1 master_args, _ = master_calls[0] - assert [track["id"] for track in master_args[2]] == ["enhance-1"] - assert batch_map[payload["batch_id"]]["analysis_total"] == 1 - assert activity_calls == [("", "Wishlist Download Started", "1 tracks", "Now")] - assert any("Cleaned up 1 already-owned tracks" in msg for msg in logger.info_messages) + assert [track["id"] for track in master_args[2]] == ["enhance-1", "owned-1"] + assert batch_map[payload["batch_id"]]["analysis_total"] == 2 + assert activity_calls == [("", "Wishlist Download Started", "2 tracks", "Now")] -def test_bg_job_marks_batch_complete_when_wishlist_empty_after_cleanup(): - """If cleanup empties the wishlist, the bg job marks the batch complete (not error 400).""" +def test_bg_job_marks_batch_complete_when_wishlist_genuinely_empty(): + """If the wishlist is empty before the manual click, the bg job marks the batch complete.""" runtime, _service, _db, executor, _logger, _activity, batch_map, master_calls = _build_runtime( - tracks=[ - { - "id": "owned-1", - "name": "Owned Song", - "artists": [{"name": "Artist B"}], - "album": {"name": "Owned Album", "album_type": "album"}, - }, - ], - owned_matches={("Owned Song", "Artist B")}, + tracks=[], ) payload, status = processing.start_manual_wishlist_download_batch(runtime) @@ -240,7 +243,7 @@ def test_bg_job_marks_batch_complete_when_wishlist_empty_after_cleanup(): _run_submitted_bg_job(executor) - # Cleanup removed the only track. Master worker never called. Batch marked complete. + # No tracks → master worker never called, batch marked complete with explanatory error. assert master_calls == [] assert batch_map[payload["batch_id"]]["phase"] == "complete" assert batch_map[payload["batch_id"]]["error"] == "No tracks in wishlist"