Trust user manual picks past AcoustID verification (#701)
When a task failed AcoustID verification and got quarantined, opening the candidates modal and manually picking a different file would just re-quarantine it. The manual-pick path through `_attempt_download_with_candidates` ran full post-processing with no quarantine bypass — so if the alternate file disagreed with AcoustID's stored metadata too (common for live versions, remasters, regional title differences, fingerprint coverage gaps) the file landed right back in quarantine. User got stuck in the loop. The Approve button on quarantined rows already handles the "I want this exact file" case via `_skip_quarantine_check='all'`. The candidates modal handles the "I want a different file" case — same user intent, opposite direction, but the bypass plumbing didn't carry through. `/api/downloads/task/<id>/download-candidate` already sets `task['_user_manual_pick'] = True`. `attempt_download_with_candidates` now reads that flag under tasks_lock alongside `used_sources` and, when set, injects `_skip_quarantine_check='acoustid'` plus `_user_manual_pick=True` into the stored `matched_downloads_context` entry. The acoustid-only scope is deliberate: integrity + bit-depth gates still run because those check the new file's actual condition (corruption, sample rate) rather than its identity — only the metadata-mismatch gate is the user-override case. Auto-search picks (the normal task-worker path) leave the flag unset and continue to run full AcoustID verification, preserving the existing safety net for non-user-initiated downloads. Tests: - positive: manual-pick task → stored context has `_skip_quarantine_check='acoustid'` and `_user_manual_pick=True` - negative: auto-search task → stored context has neither key, AcoustID still runs as before Full suite 3976 pass.
This commit is contained in:
parent
85ba93f16f
commit
b5755d6307
3 changed files with 62 additions and 0 deletions
|
|
@ -84,6 +84,11 @@ def attempt_download_with_candidates(task_id, candidates, track, batch_id=None,
|
|||
if not task:
|
||||
return False
|
||||
used_sources = task.get('used_sources', set())
|
||||
# User-initiated manual picks (candidates modal) bypass quarantine
|
||||
# gates downstream. The user already accepted the risk by choosing
|
||||
# the file; we trust their selection over AcoustID disagreement so
|
||||
# repeated manual picks don't loop back into quarantine.
|
||||
user_manual_pick = bool(task.get('_user_manual_pick', False))
|
||||
|
||||
# Try each candidate until one succeeds (like GUI's fallback logic)
|
||||
for candidate_index, candidate in enumerate(candidates):
|
||||
|
|
@ -331,6 +336,20 @@ def attempt_download_with_candidates(task_id, candidates, track, batch_id=None,
|
|||
"track_info": track_info, # Add track_info for playlist folder mode
|
||||
"_download_username": username, # Source username for AcoustID skip logic
|
||||
}
|
||||
if user_manual_pick:
|
||||
# The user explicitly picked this candidate via the
|
||||
# candidates modal — trust their metadata judgement
|
||||
# over AcoustID disagreement so manual picks don't
|
||||
# loop back into quarantine. Integrity + bit-depth
|
||||
# gates still run because those check the new file's
|
||||
# actual condition, not its identity.
|
||||
matched_downloads_context[context_key]['_skip_quarantine_check'] = 'acoustid'
|
||||
matched_downloads_context[context_key]['_user_manual_pick'] = True
|
||||
logger.info(
|
||||
"[Context] User manual pick — bypassing AcoustID for "
|
||||
"task=%s username=%s filename=%s",
|
||||
task_id, username, os.path.basename(filename),
|
||||
)
|
||||
|
||||
logger.info(f"[Context] Set is_album_download: {is_album_context} (has clean data: {has_clean_spotify_data})")
|
||||
logger.debug(f"[Debug] Context creation - track_info: {track_info is not None}, playlist_folder_mode: {track_info.get('_playlist_folder_mode', False) if track_info else False}")
|
||||
|
|
|
|||
|
|
@ -419,6 +419,48 @@ def test_candidates_with_equal_confidence_both_tried():
|
|||
assert deps.download_orchestrator.download_calls[0][1] == "a.flac"
|
||||
|
||||
|
||||
def test_user_manual_pick_injects_acoustid_bypass_into_post_process_context():
|
||||
"""Issue #701: when the user picks a specific candidate via the
|
||||
candidates modal, the download_selected_candidate endpoint sets
|
||||
`_user_manual_pick=True` on the task. The candidates helper must
|
||||
propagate that into the stored post-process context as
|
||||
`_skip_quarantine_check='acoustid'`; without it the manual pick
|
||||
loops straight back into quarantine whenever AcoustID disagrees
|
||||
with the user's selection."""
|
||||
deps = _build_deps()
|
||||
_seed_task("t_manual_pick")
|
||||
download_tasks["t_manual_pick"]["_user_manual_pick"] = True
|
||||
|
||||
candidates = [_Candidate(filename="picked.flac", confidence=0.99)]
|
||||
track = _Track()
|
||||
|
||||
result = dc.attempt_download_with_candidates("t_manual_pick", candidates, track, batch_id="b1", deps=deps)
|
||||
|
||||
assert result is True
|
||||
ctx = matched_downloads_context["user1::picked.flac"]
|
||||
assert ctx["_skip_quarantine_check"] == "acoustid"
|
||||
assert ctx["_user_manual_pick"] is True
|
||||
|
||||
|
||||
def test_auto_search_pick_does_not_inject_acoustid_bypass():
|
||||
"""The bypass is ONLY for user-initiated manual picks. Auto-search
|
||||
candidate picks (which run during the normal download flow) must
|
||||
still get AcoustID verification — they're the canonical guard
|
||||
against the wrong-file leak that quarantine exists to catch."""
|
||||
deps = _build_deps()
|
||||
_seed_task("t_auto_pick") # No _user_manual_pick flag set
|
||||
|
||||
candidates = [_Candidate(filename="auto.flac", confidence=0.99)]
|
||||
track = _Track()
|
||||
|
||||
result = dc.attempt_download_with_candidates("t_auto_pick", candidates, track, batch_id="b1", deps=deps)
|
||||
|
||||
assert result is True
|
||||
ctx = matched_downloads_context["user1::auto.flac"]
|
||||
assert "_skip_quarantine_check" not in ctx
|
||||
assert "_user_manual_pick" not in ctx
|
||||
|
||||
|
||||
def test_equal_confidence_candidates_prefer_better_peer_quality():
|
||||
"""Equal-confidence Soulseek candidates use peer quality as the tiebreaker."""
|
||||
deps = _build_deps()
|
||||
|
|
|
|||
|
|
@ -3415,6 +3415,7 @@ function closeHelperSearch() {
|
|||
const WHATS_NEW = {
|
||||
'2.6.2': [
|
||||
{ date: 'May 24, 2026 — 2.6.2 release' },
|
||||
{ title: 'Fix: songs stuck in quarantine loop when picking a different candidate', desc: 'when a track failed AcoustID verification and got quarantined, opening the candidates modal and manually picking a different file would just re-quarantine it — the manual pick path ran full AcoustID verification with no bypass, so if the alternate file disagreed with AcoustID\'s stored metadata too (common for live versions, remasters, regional title differences) the file landed right back in quarantine. user got stuck in the loop. manual picks via the candidates modal now skip AcoustID for that one post-process pass (matching what the Approve button already does for restored quarantine files). integrity and bit-depth gates still run because those check the new file\'s actual condition, not its identity. closes #701.' },
|
||||
{ title: 'Fix: whole-album downloads ending up "failed" with empty queues', desc: 'the Soulseek album-bundle path routes whole-album downloads through a private staging dir, then per-track workers claim the staged files. when slskd files arrived without ID3 tags, the staging cache fell back to filename stems like "Artist - Album - 03 - Title" — too noisy to clear the title-similarity threshold against the clean Spotify title, so every track went not_found and the batch ended failed even with all files on disk. staging match now pulls the trailing-title segment when a bare track-number is present between " - " delimiters, so slskd filename patterns match cleanly. closes #700.' },
|
||||
{ title: 'Fix: album tracks getting requeued as singles by the wishlist', desc: 'downstream of the album-bundle fix above. failed album-batch tracks were going to the wishlist with `source_type=\'playlist\'` hardcoded, and a couple of fallback paths were stamping `album_type=\'single\'` on the stored album dict. on requeue the path builder saw single → routed to the Singles tree even though the track belonged to an album (running Reorganize would patch it because the DB still knew). album batches now carry `source_type=\'album\'`, source-context preserves `album_context` / `artist_context`, and the non-dict-album + slskd-reconstruction fallbacks default to `album` instead of lying with `single`. closes #698.' },
|
||||
{ title: 'Fix: Redownload Album button on the enhanced artist view was dead', desc: 'the Redownload button on the enhanced artist-page album row was throwing a silent ReferenceError on click — no popup, no toast, no log line, button just did nothing. the underlying function was lost when script.js got split into 17 domain modules and nothing caught it for a while. restored. closes #699.' },
|
||||
|
|
|
|||
Loading…
Reference in a new issue