fix: drop redundant library-cleanup pass from wishlist download flows
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.
This commit is contained in:
Broque Thomas
parent
6a25dcd49e
commit
99a763dace
2 changed files with 41 additions and 53 deletions
|
|
@ -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']))
|
||||
|
|
|
|||
|
|
@ -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"
|
||||
|
|
|
|||
Loading…
Reference in a new issue