From e83cf199039341ea6f6782b3ff69b99bffda97c9 Mon Sep 17 00:00:00 2001 From: dev Date: Fri, 5 Jun 2026 02:24:00 +0200 Subject: [PATCH 01/14] Downloads: retry next-best candidate on AcoustID/integrity quarantine MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When a downloaded file is quarantined because AcoustID verification or the integrity/duration check fails, the task no longer dead-ends as failed — it re-runs the worker on the next-best candidate, skipping the quarantined source. Reuses the monitor's existing transfer-error retry machinery (used_sources + cached_candidates + worker re-dispatch), just triggered from the post-process verification wrapper's two quarantine branches instead of only on transfer errors. Universal across sources (Soulseek, HiFi, Tidal, etc.) since all batch/sync downloads funnel through post_process_matched_download_with_verification. - monitor.requeue_quarantined_task_for_retry(): marks bad source used, resets task to searching, resubmits worker. Guards: manual picks, cancelled tasks, missing source id, and a MAX_QUARANTINE_RETRIES=5 loop cap. - Opt-out via post_processing.retry_next_candidate_on_mismatch (default on). - Manual quarantine approve is unaffected (_skip_quarantine_check='all' bypasses the checks, so no quarantine flag, so no retry). Co-Authored-By: Claude Opus 4.8 --- core/downloads/monitor.py | 89 +++++++++++++++++ core/imports/pipeline.py | 50 ++++++++-- tests/imports/test_import_pipeline.py | 132 ++++++++++++++++++++++++++ 3 files changed, 265 insertions(+), 6 deletions(-) diff --git a/core/downloads/monitor.py b/core/downloads/monitor.py index 4bcb0bc3..c99028a3 100644 --- a/core/downloads/monitor.py +++ b/core/downloads/monitor.py @@ -37,11 +37,100 @@ missing_download_executor = None download_orchestrator = None _RELEASE_SOURCE_NAMES = frozenset(('torrent', 'usenet')) +# Hard ceiling on automatic next-candidate retries after a download was +# quarantined (AcoustID mismatch / integrity / duration). The natural +# terminator is used_sources exhaustion — once every candidate the worker can +# find has been tried, attempt_download_with_candidates returns False and the +# worker reports a clean failure. This cap is a safety net against a pathological +# quarantine→retry→quarantine loop (e.g. a source that keeps returning fresh +# wrong files). +MAX_QUARANTINE_RETRIES = 5 + def _download_id_key(download_id): return f"download_id::{download_id}" if download_id else None +def requeue_quarantined_task_for_retry(task_id, batch_id, trigger): + """Re-queue a task whose download was just quarantined so the worker tries + the NEXT best candidate instead of failing outright. + + Called from the post-processing verification wrapper when AcoustID + verification or the integrity/duration check quarantines a file. It mirrors + the monitor's transfer-error retry path: mark the bad source as used, clear + the stale download identity, reset the task to ``searching`` and resubmit + the download worker. Because ``used_sources`` is preserved across the + re-run, the worker skips the quarantined source and picks the next-best + candidate (see ``attempt_download_with_candidates``). + + Returns True if a retry was queued — the caller must then NOT mark the task + failed or notify batch completion, since the task is going around again. + Returns False when no retry is possible (retry engine unwired, manual pick, + cancelled, or retry budget exhausted); the caller falls through to its + existing failure handling. + """ + # Opt-out escape hatch — default on. Lets users restore the old + # quarantine-and-fail behaviour without a code change. + if not config_manager.get('post_processing.retry_next_candidate_on_mismatch', True): + return False + + # Retry engine not wired (e.g. manual-import path that never started a + # download worker). Nothing to re-run. + if missing_download_executor is None or _download_track_worker is None: + return False + + with tasks_lock: + task = download_tasks.get(task_id) + if not task: + return False + # The user explicitly picked this candidate via the candidates modal — + # honour their choice rather than silently swapping in another file. + # (Matches the monitor's transfer-retry guards.) + if task.get('_user_manual_pick'): + return False + if task.get('status') == 'cancelled': + return False + + username = task.get('username') + filename = task.get('filename') + # No source identity means this wasn't a worker-dispatched download we + # can retry — without the "{username}_{filename}" key we can't flag the + # bad source as used, so a re-run could re-pick the same file and loop. + # Bail and let the caller fail it normally. + if not username or not filename: + return False + + retry_count = task.get('quarantine_retry_count', 0) + if retry_count >= MAX_QUARANTINE_RETRIES: + logger.warning( + f"[Retry:{trigger}] Task {task_id} hit the quarantine-retry cap " + f"({MAX_QUARANTINE_RETRIES}) — giving up, marking failed" + ) + return False + + # Mark the quarantined source as used so the re-run won't pick it again. + # Uses the same "{username}_{filename}" key the worker dedups against. + used_sources = task.get('used_sources', set()) + used_sources.add(f"{username}_{filename}") + task['used_sources'] = used_sources + + task['quarantine_retry_count'] = retry_count + 1 + # Drop the stale download identity + the prior attempt's quarantine link. + task.pop('download_id', None) + task.pop('username', None) + task.pop('filename', None) + task.pop('quarantine_entry_id', None) + task['status'] = 'searching' + task['status_change_time'] = time.time() + + logger.info( + f"[Retry:{trigger}] Re-queuing task {task_id} for next-best candidate " + f"(attempt {retry_count + 1}/{MAX_QUARANTINE_RETRIES})" + ) + missing_download_executor.submit(_download_track_worker, task_id, batch_id) + return True + + def _is_release_task(task): ti = task.get('track_info') if isinstance(task.get('track_info'), dict) else {} username = task.get('username') or ti.get('username') diff --git a/core/imports/pipeline.py b/core/imports/pipeline.py index 067e4559..a2890519 100644 --- a/core/imports/pipeline.py +++ b/core/imports/pipeline.py @@ -109,6 +109,30 @@ def _mark_task_quarantined(context: dict, quarantine_path: str | None) -> None: download_tasks[task_id]['quarantine_entry_id'] = entry_id +def _requeue_quarantined_task_for_retry(task_id, batch_id, trigger) -> bool: + """Ask the download monitor to re-run this task on its next-best candidate. + + Thin lazy-import wrapper around + ``core.downloads.monitor.requeue_quarantined_task_for_retry``. Imported + lazily (and defensively) so the post-processing pipeline stays importable on + its own — the monitor's retry globals are wired by web_server at startup, and + manual-import callers that never started a download worker simply get False. + Returns True when a retry was queued (caller must not mark the task failed). + """ + if not task_id: + return False + try: + from core.downloads.monitor import requeue_quarantined_task_for_retry + except Exception as exc: # pragma: no cover - defensive + logger.debug(f"next-candidate retry unavailable ({trigger}): {exc}") + return False + try: + return requeue_quarantined_task_for_retry(task_id, batch_id, trigger) + except Exception as exc: # pragma: no cover - defensive + logger.error(f"next-candidate retry failed ({trigger}): {exc}") + return False + + def import_rejection_reason(context: dict) -> str | None: """Human-readable reason if post-processing terminally rejected the file (quarantine or race-guard), else ``None`` for a clean import. @@ -992,6 +1016,16 @@ def post_process_matched_download_with_verification(context_key, context, file_p if context.get('_acoustid_quarantined'): failure_msg = context.get('_acoustid_failure_msg', 'AcoustID verification failed') + # Before failing outright, try the next-best candidate. The wrong + # file was just quarantined; re-running the worker (with the bad + # source flagged used) picks the runner-up match instead. + with matched_context_lock: + matched_downloads_context.pop(context_key, None) + if _requeue_quarantined_task_for_retry(task_id, batch_id, 'acoustid'): + logger.info( + f"AcoustID mismatch for task {task_id} — retrying next-best candidate: {failure_msg}" + ) + return logger.info(f"File was quarantined by AcoustID verification (task={task_id}): {failure_msg}") with tasks_lock: if task_id in download_tasks: @@ -1000,9 +1034,6 @@ def post_process_matched_download_with_verification(context_key, context, file_p _eid = context.get('_quarantine_entry_id') if _eid: download_tasks[task_id]['quarantine_entry_id'] = _eid - with matched_context_lock: - if context_key in matched_downloads_context: - del matched_downloads_context[context_key] _notify_download_completed(batch_id, task_id, success=False) return @@ -1044,6 +1075,16 @@ def post_process_matched_download_with_verification(context_key, context, file_p # source files failed integrity and were quarantined. if context.get('_integrity_failure_msg'): failure_msg = context.get('_integrity_failure_msg', 'unknown') + # Integrity/duration mismatch (truncated transfer, wrong-length cut, + # etc). Same treatment as an AcoustID mismatch: quarantine the bad + # file and retry the next-best candidate before failing. + with matched_context_lock: + matched_downloads_context.pop(context_key, None) + if _requeue_quarantined_task_for_retry(task_id, batch_id, 'integrity'): + logger.info( + f"Integrity check failed for task {task_id} — retrying next-best candidate: {failure_msg}" + ) + return logger.error( f"Task {task_id} failed integrity check — marking failed: {failure_msg}" ) @@ -1056,9 +1097,6 @@ def post_process_matched_download_with_verification(context_key, context, file_p _eid = context.get('_quarantine_entry_id') if _eid: download_tasks[task_id]['quarantine_entry_id'] = _eid - with matched_context_lock: - if context_key in matched_downloads_context: - del matched_downloads_context[context_key] _notify_download_completed(batch_id, task_id, success=False) return diff --git a/tests/imports/test_import_pipeline.py b/tests/imports/test_import_pipeline.py index b283cb7e..6667e985 100644 --- a/tests/imports/test_import_pipeline.py +++ b/tests/imports/test_import_pipeline.py @@ -304,3 +304,135 @@ def test_verification_wrapper_applies_quarantine_entry_id_on_integrity_failure(m runtime_state.download_tasks.update(original) runtime_state.matched_downloads_context.clear() runtime_state.matched_downloads_context.update(original_ctx) + + +# --------------------------------------------------------------------------- +# Next-best-candidate retry on AcoustID / integrity quarantine. When a +# verification or integrity check quarantines the wrong/broken file, the wrapper +# asks the monitor to re-run the worker on the next candidate (skipping the bad +# source) instead of failing the task outright. +# --------------------------------------------------------------------------- + +def _wire_retry_engine(monkeypatch): + """Wire monitor's retry globals to capture the worker re-submission.""" + import core.downloads.monitor as monitor + + submitted = [] + + class _Exec: + def submit(self, fn, *args): + submitted.append(args) + + monkeypatch.setattr(monitor, "missing_download_executor", _Exec()) + monkeypatch.setattr(monitor, "_download_track_worker", lambda task_id, batch_id: None) + monkeypatch.setattr(monitor, "MAX_QUARANTINE_RETRIES", 5) + return submitted + + +def _run_wrapper_with_quarantine(monkeypatch, flag_setter, task_extra=None): + task_id, batch_id, context_key = "rtask", "rbatch", "rctx" + context = {"track_info": {}, "task_id": task_id, "batch_id": batch_id} + + monkeypatch.setattr(import_pipeline, "post_process_matched_download", flag_setter) + + original = dict(runtime_state.download_tasks) + original_ctx = dict(runtime_state.matched_downloads_context) + try: + runtime_state.download_tasks.clear() + task = { + "track_info": {}, "status": "downloading", + "username": "hifi", "filename": "123||A - B", "used_sources": set(), + } + if task_extra: + task.update(task_extra) + runtime_state.download_tasks[task_id] = task + runtime_state.matched_downloads_context.clear() + runtime_state.matched_downloads_context[context_key] = context + + completion = [] + runtime = types.SimpleNamespace( + automation_engine=None, + on_download_completed=lambda b, t, success: completion.append((b, t, success)), + web_scan_manager=None, + repair_worker=None, + ) + import_pipeline.post_process_matched_download_with_verification( + context_key, context, "/tmp/source.flac", task_id, batch_id, runtime, + ) + return dict(runtime_state.download_tasks[task_id]), completion, context_key + finally: + runtime_state.download_tasks.clear() + runtime_state.download_tasks.update(original) + runtime_state.matched_downloads_context.clear() + runtime_state.matched_downloads_context.update(original_ctx) + + +def test_acoustid_mismatch_requeues_next_candidate(monkeypatch): + submitted = _wire_retry_engine(monkeypatch) + + def _fake_inner(ck, ctx, fp, runtime, metadata_runtime=None): + ctx["_acoustid_quarantined"] = True + ctx["_acoustid_failure_msg"] = "wrong song" + + task, completion, context_key = _run_wrapper_with_quarantine(monkeypatch, _fake_inner) + + # Task goes back to searching for the next candidate — NOT failed. + assert task["status"] == "searching" + assert task["quarantine_retry_count"] == 1 + # The quarantined source is flagged so the re-run won't re-pick it. + assert "hifi_123||A - B" in task["used_sources"] + # Stale download identity cleared; worker re-submitted; no batch failure. + assert "download_id" not in task and "username" not in task + assert submitted == [("rtask", "rbatch")] + assert completion == [] + # Old context cleaned up (the re-run builds a fresh one for the new pick). + assert context_key not in runtime_state.matched_downloads_context + + +def test_integrity_mismatch_requeues_next_candidate(monkeypatch): + submitted = _wire_retry_engine(monkeypatch) + + def _fake_inner(ck, ctx, fp, runtime, metadata_runtime=None): + ctx["_integrity_failure_msg"] = "Duration mismatch: file is 231.0s, expected 271.0s" + + task, completion, _ = _run_wrapper_with_quarantine(monkeypatch, _fake_inner) + + assert task["status"] == "searching" + assert task["quarantine_retry_count"] == 1 + assert submitted == [("rtask", "rbatch")] + assert completion == [] + + +def test_manual_pick_does_not_requeue_on_mismatch(monkeypatch): + submitted = _wire_retry_engine(monkeypatch) + + def _fake_inner(ck, ctx, fp, runtime, metadata_runtime=None): + ctx["_integrity_failure_msg"] = "Duration mismatch" + + task, completion, _ = _run_wrapper_with_quarantine( + monkeypatch, _fake_inner, task_extra={"_user_manual_pick": True}, + ) + + # User explicitly chose this file — fail it, don't silently swap. + assert task["status"] == "failed" + assert submitted == [] + assert completion == [("rbatch", "rtask", False)] + + +def test_retry_budget_exhausted_fails_task(monkeypatch): + submitted = _wire_retry_engine(monkeypatch) + import core.downloads.monitor as monitor + monkeypatch.setattr(monitor, "MAX_QUARANTINE_RETRIES", 2) + + def _fake_inner(ck, ctx, fp, runtime, metadata_runtime=None): + ctx["_acoustid_quarantined"] = True + ctx["_acoustid_failure_msg"] = "wrong song" + + task, completion, _ = _run_wrapper_with_quarantine( + monkeypatch, _fake_inner, task_extra={"quarantine_retry_count": 2}, + ) + + # Cap reached — fall through to normal failure handling. + assert task["status"] == "failed" + assert submitted == [] + assert completion == [("rbatch", "rtask", False)] From f3d43f385e4ee55873376285031b3b9e69418255 Mon Sep 17 00:00:00 2001 From: dev Date: Fri, 5 Jun 2026 13:14:51 +0200 Subject: [PATCH 02/14] Downloads: per-source exhaustive retry budget on mismatch (opt-in) Adds an opt-in exhaustive mode to the quarantine-retry path. Default behaviour is unchanged: a single global cap (MAX_QUARANTINE_RETRIES=5). When post_processing.retry_exhaustive is on, each source gets its OWN retry budget sized as query_count x retries_per_query. Soulseek peers collapse to one 'soulseek' bucket; streaming plugins keep their name. The worker now records query_count on the task; the budget scales with the track's real query count. Loop protection is threefold: per-source cap, used_sources exhaustion (the natural terminator), and an absolute ceiling (MAX_TOTAL_QUARANTINE_RETRIES=100). New settings (config + WebUI): retry_next_candidate_on_mismatch (master), retry_exhaustive, retries_per_query (default 5). Tests: 6 new cases covering per-source budgeting, source separation, Soulseek-peer bucketing, query_count default, and the absolute ceiling. Co-Authored-By: Claude Opus 4.8 --- config/settings.py | 13 +++ core/downloads/monitor.py | 97 +++++++++++++++++-- core/downloads/task_worker.py | 5 + tests/imports/test_import_pipeline.py | 131 ++++++++++++++++++++++++++ webui/index.html | 19 ++++ webui/static/settings.js | 6 ++ 6 files changed, 262 insertions(+), 9 deletions(-) diff --git a/config/settings.py b/config/settings.py index 5ff13f6a..c1b0c114 100644 --- a/config/settings.py +++ b/config/settings.py @@ -494,6 +494,19 @@ class ConfigManager: "album_bundle_poll_interval_seconds": 2.0, "album_bundle_timeout_seconds": 6 * 60 * 60, # 6 hours }, + "post_processing": { + # When a download is quarantined (AcoustID mismatch, integrity / + # duration failure), retry the next-best candidate instead of + # failing outright. Default on. + "retry_next_candidate_on_mismatch": True, + # Opt-in exhaustive retry: budget retries PER SOURCE so every + # source (Soulseek, then HiFi/Tidal/…) gets its own attempts + # before the track gives up. Default off (single global cap). + "retry_exhaustive": False, + # Retries per search query per source in exhaustive mode. The + # per-source budget is query_count × this value. + "retries_per_query": 5, + }, "tidal_download": { "quality": "lossless", # Options: "low", "high", "lossless", "hires" "session": { diff --git a/core/downloads/monitor.py b/core/downloads/monitor.py index c99028a3..dcdfcd77 100644 --- a/core/downloads/monitor.py +++ b/core/downloads/monitor.py @@ -44,8 +44,37 @@ _RELEASE_SOURCE_NAMES = frozenset(('torrent', 'usenet')) # worker reports a clean failure. This cap is a safety net against a pathological # quarantine→retry→quarantine loop (e.g. a source that keeps returning fresh # wrong files). +# +# Default (non-exhaustive) mode uses this single global cap. The opt-in +# exhaustive mode (post_processing.retry_exhaustive) instead budgets retries +# PER SOURCE — see requeue_quarantined_task_for_retry. MAX_QUARANTINE_RETRIES = 5 +# Absolute runaway guard for exhaustive mode. Per-source budgets are already +# finite (query_count × retries_per_query, and Soulseek peers all collapse to +# one 'soulseek' bucket), but this ceiling caps the TOTAL retries across every +# source so a misbehaving source-resolution can never loop forever. +MAX_TOTAL_QUARANTINE_RETRIES = 100 + +# Streaming plugins report their source name as the download's "username" +# (see download_orchestrator._streaming_sources). Soulseek uses the peer name +# instead, so anything not in this set is bucketed under 'soulseek' for the +# per-source retry budget. +_STREAMING_SOURCE_NAMES = frozenset(( + 'youtube', 'tidal', 'qobuz', 'hifi', 'deezer_dl', 'lidarr', 'soundcloud', 'amazon', +)) + + +def _resolve_download_source(username): + """Map a download's username to its logical source for per-source budgeting. + + Streaming sources use the source name as username; Soulseek uses the peer + name, so every Soulseek peer collapses to a single 'soulseek' bucket. + """ + if username and username in _STREAMING_SOURCE_NAMES: + return username + return 'soulseek' + def _download_id_key(download_id): return f"download_id::{download_id}" if download_id else None @@ -100,13 +129,63 @@ def requeue_quarantined_task_for_retry(task_id, batch_id, trigger): if not username or not filename: return False - retry_count = task.get('quarantine_retry_count', 0) - if retry_count >= MAX_QUARANTINE_RETRIES: - logger.warning( - f"[Retry:{trigger}] Task {task_id} hit the quarantine-retry cap " - f"({MAX_QUARANTINE_RETRIES}) — giving up, marking failed" - ) - return False + total_count = task.get('quarantine_retry_count', 0) + + if config_manager.get('post_processing.retry_exhaustive', False): + # Exhaustive mode: a SEPARATE budget per source. The budget scales + # with the track's own query count (the worker generates a variable + # number of search queries per track) × the configured retries per + # query. Soulseek candidates are walked first (one per retry), then + # the worker's hybrid fallback moves to the next source — each source + # spending its own budget. The natural terminator (used_sources + # exhaustion → worker clean-fail) still ends most tracks well before + # any budget is reached; the budget is the per-source safety ceiling. + source = _resolve_download_source(username) + retries_per_query = config_manager.get('post_processing.retries_per_query', 5) + try: + retries_per_query = int(retries_per_query) + except (TypeError, ValueError): + retries_per_query = 5 + if retries_per_query < 1: + retries_per_query = 1 + + query_count = task.get('query_count') or 1 + if query_count < 1: + query_count = 1 + budget = query_count * retries_per_query + + counts = task.get('quarantine_retry_counts_by_source') + if not isinstance(counts, dict): + counts = {} + source_count = counts.get(source, 0) + + if source_count >= budget: + logger.warning( + f"[Retry:{trigger}] Task {task_id} exhausted its retry budget " + f"for source '{source}' ({source_count}/{budget}) — giving up, " + f"marking failed" + ) + return False + if total_count >= MAX_TOTAL_QUARANTINE_RETRIES: + logger.warning( + f"[Retry:{trigger}] Task {task_id} hit the absolute retry " + f"ceiling ({MAX_TOTAL_QUARANTINE_RETRIES}) — giving up, marking " + f"failed" + ) + return False + + counts[source] = source_count + 1 + task['quarantine_retry_counts_by_source'] = counts + attempt_desc = f"source '{source}' {source_count + 1}/{budget}" + else: + # Default mode: a single global cap, conservative and predictable. + if total_count >= MAX_QUARANTINE_RETRIES: + logger.warning( + f"[Retry:{trigger}] Task {task_id} hit the quarantine-retry cap " + f"({MAX_QUARANTINE_RETRIES}) — giving up, marking failed" + ) + return False + attempt_desc = f"{total_count + 1}/{MAX_QUARANTINE_RETRIES}" # Mark the quarantined source as used so the re-run won't pick it again. # Uses the same "{username}_{filename}" key the worker dedups against. @@ -114,7 +193,7 @@ def requeue_quarantined_task_for_retry(task_id, batch_id, trigger): used_sources.add(f"{username}_{filename}") task['used_sources'] = used_sources - task['quarantine_retry_count'] = retry_count + 1 + task['quarantine_retry_count'] = total_count + 1 # Drop the stale download identity + the prior attempt's quarantine link. task.pop('download_id', None) task.pop('username', None) @@ -125,7 +204,7 @@ def requeue_quarantined_task_for_retry(task_id, batch_id, trigger): logger.info( f"[Retry:{trigger}] Re-queuing task {task_id} for next-best candidate " - f"(attempt {retry_count + 1}/{MAX_QUARANTINE_RETRIES})" + f"(attempt {attempt_desc})" ) missing_download_executor.submit(_download_track_worker, task_id, batch_id) return True diff --git a/core/downloads/task_worker.py b/core/downloads/task_worker.py index cc6751dc..cf349381 100644 --- a/core/downloads/task_worker.py +++ b/core/downloads/task_worker.py @@ -277,6 +277,11 @@ def download_track_worker(task_id: str, batch_id: Optional[str], deps: TaskWorke seen.add(query.lower()) search_queries = unique_queries + # Expose the query count so the quarantine-retry budget (exhaustive mode) + # can size each source's budget as query_count × retries_per_query. + with tasks_lock: + if task_id in download_tasks: + download_tasks[task_id]['query_count'] = len(search_queries) logger.info(f"[Modal Worker] Generated {len(search_queries)} smart search queries for '{track.name}': {search_queries}") logger.info(f"[Modal Worker] About to start search loop for task {task_id} (track: '{track.name}')") diff --git a/tests/imports/test_import_pipeline.py b/tests/imports/test_import_pipeline.py index 6667e985..e6b6891a 100644 --- a/tests/imports/test_import_pipeline.py +++ b/tests/imports/test_import_pipeline.py @@ -329,6 +329,20 @@ def _wire_retry_engine(monkeypatch): return submitted +def _patch_config(monkeypatch, overrides): + """Override specific config keys for the monitor's config_manager reads.""" + import core.downloads.monitor as monitor + + real_get = monitor.config_manager.get + + def fake_get(key, default=None): + if key in overrides: + return overrides[key] + return real_get(key, default) + + monkeypatch.setattr(monitor.config_manager, "get", fake_get) + + def _run_wrapper_with_quarantine(monkeypatch, flag_setter, task_extra=None): task_id, batch_id, context_key = "rtask", "rbatch", "rctx" context = {"track_info": {}, "task_id": task_id, "batch_id": batch_id} @@ -436,3 +450,120 @@ def test_retry_budget_exhausted_fails_task(monkeypatch): assert task["status"] == "failed" assert submitted == [] assert completion == [("rbatch", "rtask", False)] + + +def _acoustid_quarantine(ck, ctx, fp, runtime, metadata_runtime=None): + ctx["_acoustid_quarantined"] = True + ctx["_acoustid_failure_msg"] = "wrong song" + + +def test_exhaustive_mode_uses_per_source_budget(monkeypatch): + submitted = _wire_retry_engine(monkeypatch) + _patch_config(monkeypatch, { + "post_processing.retry_exhaustive": True, + "post_processing.retries_per_query": 5, + }) + + # query_count=2 → budget for source 'hifi' = 2 * 5 = 10; first failure retries. + task, completion, _ = _run_wrapper_with_quarantine( + monkeypatch, _acoustid_quarantine, task_extra={"query_count": 2}, + ) + + assert task["status"] == "searching" + # Per-source budget tracked separately from the legacy global counter. + assert task["quarantine_retry_counts_by_source"] == {"hifi": 1} + assert task["quarantine_retry_count"] == 1 + assert "hifi_123||A - B" in task["used_sources"] + assert submitted == [("rtask", "rbatch")] + assert completion == [] + + +def test_exhaustive_source_budget_exhausted_fails(monkeypatch): + submitted = _wire_retry_engine(monkeypatch) + _patch_config(monkeypatch, { + "post_processing.retry_exhaustive": True, + "post_processing.retries_per_query": 5, + }) + + # hifi already at its full budget (query_count 2 * 5 = 10) → fail, no retry. + task, completion, _ = _run_wrapper_with_quarantine( + monkeypatch, _acoustid_quarantine, + task_extra={"query_count": 2, "quarantine_retry_counts_by_source": {"hifi": 10}}, + ) + + assert task["status"] == "failed" + assert submitted == [] + assert completion == [("rbatch", "rtask", False)] + + +def test_exhaustive_budget_is_separate_per_source(monkeypatch): + submitted = _wire_retry_engine(monkeypatch) + _patch_config(monkeypatch, { + "post_processing.retry_exhaustive": True, + "post_processing.retries_per_query": 5, + }) + + # soulseek is already maxed, but the failing download is on hifi — hifi has + # its own fresh budget, so the task still retries. + task, completion, _ = _run_wrapper_with_quarantine( + monkeypatch, _acoustid_quarantine, + task_extra={"query_count": 1, "quarantine_retry_counts_by_source": {"soulseek": 5}}, + ) + + assert task["status"] == "searching" + assert task["quarantine_retry_counts_by_source"] == {"soulseek": 5, "hifi": 1} + assert submitted == [("rtask", "rbatch")] + + +def test_exhaustive_soulseek_peer_resolves_to_soulseek(monkeypatch): + submitted = _wire_retry_engine(monkeypatch) + _patch_config(monkeypatch, { + "post_processing.retry_exhaustive": True, + "post_processing.retries_per_query": 5, + }) + + # A Soulseek peer name (not a streaming source) is bucketed under 'soulseek'. + task, completion, _ = _run_wrapper_with_quarantine( + monkeypatch, _acoustid_quarantine, + task_extra={"username": "DjPeer", "filename": "f.flac", "query_count": 1}, + ) + + assert task["status"] == "searching" + assert task["quarantine_retry_counts_by_source"] == {"soulseek": 1} + + +def test_exhaustive_budget_defaults_query_count_to_one(monkeypatch): + submitted = _wire_retry_engine(monkeypatch) + _patch_config(monkeypatch, { + "post_processing.retry_exhaustive": True, + "post_processing.retries_per_query": 1, + }) + + # No query_count on the task → budget defaults to 1 * 1 = 1; hifi already at 1. + task, completion, _ = _run_wrapper_with_quarantine( + monkeypatch, _acoustid_quarantine, + task_extra={"quarantine_retry_counts_by_source": {"hifi": 1}}, + ) + + assert task["status"] == "failed" + assert submitted == [] + + +def test_exhaustive_absolute_ceiling_guards_runaway(monkeypatch): + submitted = _wire_retry_engine(monkeypatch) + import core.downloads.monitor as monitor + monkeypatch.setattr(monitor, "MAX_TOTAL_QUARANTINE_RETRIES", 3) + _patch_config(monkeypatch, { + "post_processing.retry_exhaustive": True, + "post_processing.retries_per_query": 1000, # per-source budget effectively unbounded + }) + + # Per-source budget is huge, but the absolute total ceiling (3) still fires. + task, completion, _ = _run_wrapper_with_quarantine( + monkeypatch, _acoustid_quarantine, + task_extra={"query_count": 1, "quarantine_retry_count": 3, + "quarantine_retry_counts_by_source": {"hifi": 0}}, + ) + + assert task["status"] == "failed" + assert submitted == [] diff --git a/webui/index.html b/webui/index.html index a7a68330..cfed5f5f 100644 --- a/webui/index.html +++ b/webui/index.html @@ -5563,6 +5563,25 @@ Maximum drift between the file's actual length and the metadata source's expected length before the file is quarantined. 0 = auto (3s normal, 5s for tracks >10min). Raise this if tracks routinely quarantine for being a few seconds off (live recordings, alternate masterings, etc). Capped at 60s. +
+ + When a download is quarantined (AcoustID mismatch or duration/integrity failure), automatically try the next-best candidate instead of failing the track. Off = quarantine and fail immediately. +
+
+ + Give every source (Soulseek, then HiFi/Tidal/…) its own retry budget. Each source spends queries × retries-per-query attempts before the track moves on. Worst case across two sources can mean many downloads — use for hard-to-match tracks (e.g. CJK artist names). Requires the option above. +
+
+ + + In exhaustive mode, how many candidates to try per search query per source. The per-source budget is number of search queries × this value. Only used when Exhaustive retry is on. +
diff --git a/webui/static/settings.js b/webui/static/settings.js index 1cff59a0..ffa0e4cc 100644 --- a/webui/static/settings.js +++ b/webui/static/settings.js @@ -1168,6 +1168,9 @@ async function loadSettingsData() { document.getElementById('lrclib-enabled').checked = settings.metadata_enhancement?.lrclib_enabled !== false; document.getElementById('replaygain-enabled').checked = settings.post_processing?.replaygain_enabled === true; document.getElementById('duration-tolerance-seconds').value = settings.post_processing?.duration_tolerance_seconds ?? 0; + document.getElementById('retry-next-candidate').checked = settings.post_processing?.retry_next_candidate_on_mismatch !== false; + document.getElementById('retry-exhaustive').checked = settings.post_processing?.retry_exhaustive === true; + document.getElementById('retries-per-query').value = settings.post_processing?.retries_per_query ?? 5; // Load service master toggles document.getElementById('embed-spotify').checked = settings.spotify?.embed_tags !== false; document.getElementById('embed-itunes').checked = settings.itunes?.embed_tags !== false; @@ -2996,6 +2999,9 @@ async function saveSettings(quiet = false) { post_processing: { replaygain_enabled: document.getElementById('replaygain-enabled').checked, duration_tolerance_seconds: parseFloat(document.getElementById('duration-tolerance-seconds').value) || 0, + retry_next_candidate_on_mismatch: document.getElementById('retry-next-candidate').checked, + retry_exhaustive: document.getElementById('retry-exhaustive').checked, + retries_per_query: Math.max(1, parseInt(document.getElementById('retries-per-query').value, 10) || 5), }, library: { music_paths: collectMusicPaths(), From 5e0f86c5f5d50b72759830ee960f6a2dc17ebad8 Mon Sep 17 00:00:00 2001 From: dev Date: Fri, 5 Jun 2026 15:50:11 +0200 Subject: [PATCH 03/14] Downloads: exhausted source switches to next source instead of failing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In exhaustive retry mode, a source that spent its whole per-source budget (query_count × retries_per_query) gave up and failed the track outright — never trying the other configured sources. For tracks where Soulseek has a deep pool of wrong peers (e.g. an AcoustID title mismatch every copy shares), the budget tripped long before HiFi/Tidal/… were ever reached. Now, when a source's budget is spent, the monitor marks it exhausted on the task and re-queues so the worker excludes it from the next hybrid search, falling through to the next source in the chain. Each new source spends its own fresh budget. The task only fails once no fallback source remains (or the absolute total ceiling trips) — single-source mode still fails immediately, since there's nothing to fall back to. task_worker folds the exhausted-source set into both the orchestrator search exclusion and the hybrid-fallback source list. Co-Authored-By: Claude Opus 4.8 --- core/downloads/monitor.py | 75 ++++++++++++++++++++------ core/downloads/task_worker.py | 23 +++++++- tests/imports/test_import_pipeline.py | 76 +++++++++++++++++++++++++++ webui/index.html | 2 +- 4 files changed, 157 insertions(+), 19 deletions(-) diff --git a/core/downloads/monitor.py b/core/downloads/monitor.py index dcdfcd77..273d1d45 100644 --- a/core/downloads/monitor.py +++ b/core/downloads/monitor.py @@ -76,6 +76,25 @@ def _resolve_download_source(username): return 'soulseek' +def _remaining_fallback_sources(exhausted): + """Sources in the configured hybrid chain that haven't exhausted their + per-source budget yet. + + When a source spends its whole budget (exhaustive mode), the task switches + to the next source instead of failing — but only if there *is* another + source. Single-source mode has nothing to fall back to, so this returns + empty there (and when the orchestrator isn't wired). The returned list + drives both the give-up decision here and the worker's search-exclusion on + the next attempt (see task_worker: exhausted_download_sources). + """ + orch = download_orchestrator + if orch is None or getattr(orch, 'mode', None) != 'hybrid': + return [] + chain = getattr(orch, 'hybrid_order', None) or [] + blocked = {str(s).lower() for s in exhausted} + return [s for s in chain if str(s).lower() not in blocked] + + def _download_id_key(download_id): return f"download_id::{download_id}" if download_id else None @@ -160,23 +179,47 @@ def requeue_quarantined_task_for_retry(task_id, batch_id, trigger): source_count = counts.get(source, 0) if source_count >= budget: - logger.warning( - f"[Retry:{trigger}] Task {task_id} exhausted its retry budget " - f"for source '{source}' ({source_count}/{budget}) — giving up, " - f"marking failed" + # This source spent its whole budget. Rather than fail the + # track outright, mark the source exhausted and fall through to + # the next source in the hybrid chain (the worker excludes + # exhausted sources from its next search). Only give up once no + # fallback source remains — or the absolute ceiling trips. + exhausted = set(task.get('exhausted_download_sources') or ()) + exhausted.add(source) + remaining = _remaining_fallback_sources(exhausted) + if not remaining: + logger.warning( + f"[Retry:{trigger}] Task {task_id} exhausted its retry " + f"budget for source '{source}' ({source_count}/{budget}) " + f"and no fallback source remains — giving up, marking failed" + ) + return False + if total_count >= MAX_TOTAL_QUARANTINE_RETRIES: + logger.warning( + f"[Retry:{trigger}] Task {task_id} hit the absolute retry " + f"ceiling ({MAX_TOTAL_QUARANTINE_RETRIES}) — giving up, " + f"marking failed" + ) + return False + task['exhausted_download_sources'] = exhausted + # Don't push this source's counter past its budget — it's done. + # The next source starts spending its own fresh budget when its + # first candidate fails verification. + attempt_desc = ( + f"source '{source}' budget spent ({source_count}/{budget}) " + f"— switching sources (remaining: {', '.join(remaining)})" ) - return False - if total_count >= MAX_TOTAL_QUARANTINE_RETRIES: - logger.warning( - f"[Retry:{trigger}] Task {task_id} hit the absolute retry " - f"ceiling ({MAX_TOTAL_QUARANTINE_RETRIES}) — giving up, marking " - f"failed" - ) - return False - - counts[source] = source_count + 1 - task['quarantine_retry_counts_by_source'] = counts - attempt_desc = f"source '{source}' {source_count + 1}/{budget}" + else: + if total_count >= MAX_TOTAL_QUARANTINE_RETRIES: + logger.warning( + f"[Retry:{trigger}] Task {task_id} hit the absolute retry " + f"ceiling ({MAX_TOTAL_QUARANTINE_RETRIES}) — giving up, " + f"marking failed" + ) + return False + counts[source] = source_count + 1 + task['quarantine_retry_counts_by_source'] = counts + attempt_desc = f"source '{source}' {source_count + 1}/{budget}" else: # Default mode: a single global cap, conservative and predictable. if total_count >= MAX_QUARANTINE_RETRIES: diff --git a/core/downloads/task_worker.py b/core/downloads/task_worker.py index cf349381..092d589f 100644 --- a/core/downloads/task_worker.py +++ b/core/downloads/task_worker.py @@ -288,6 +288,16 @@ def download_track_worker(task_id: str, batch_id: Optional[str], deps: TaskWorke # 2. Sequential Query Search (matches GUI's start_search_worker_parallel logic) search_diagnostics = [] # Track what happened per query for detailed error messages all_raw_results = [] # Collect raw results across queries for candidate review modal + # Sources whose per-source quarantine-retry budget is spent (exhaustive + # mode). The monitor sets this when a source gives up; we exclude those + # sources from the hybrid search so the chain falls through to the next + # source instead of re-fetching the same exhausted one (e.g. Soulseek + # keeps returning fresh wrong peers — once its budget is gone, switch to + # HiFi/Tidal/…). See monitor.requeue_quarantined_task_for_retry. + with tasks_lock: + _exhausted_sources = [ + str(s) for s in (download_tasks.get(task_id, {}).get('exhausted_download_sources') or ()) + ] for query_index, query in enumerate(search_queries): # Cancellation check before each query with tasks_lock: @@ -324,9 +334,13 @@ def download_track_worker(task_id: str, batch_id: Optional[str], deps: TaskWorke _exclude_for_hybrid_album = ['torrent', 'usenet'] except Exception as _exc_filter_err: logger.debug("[Modal Worker] album-source-exclusion check failed: %s", _exc_filter_err) + # Fold in budget-exhausted sources (per-source quarantine retry). + _exclude_sources = list(_exhausted_sources) + if _exclude_for_hybrid_album: + _exclude_sources.extend(_exclude_for_hybrid_album) # Perform search with timeout tracks_result, _ = deps.run_async(deps.download_orchestrator.search( - query, timeout=30, exclude_sources=_exclude_for_hybrid_album, + query, timeout=30, exclude_sources=_exclude_sources or None, )) logger.debug(f"Search completed for task {task_id}, got {len(tracks_result) if tracks_result else 0} results") @@ -419,7 +433,12 @@ def download_track_worker(task_id: str, batch_id: Optional[str], deps: TaskWorke # (which was definitely tried). If the first was skipped (unconfigured), # the orchestrator would have tried the second — but trying it again is # harmless (streaming sources return fast). - remaining_sources = [s for s in hybrid_order[1:] if s in source_clients and source_clients[s]] + _exhausted_lower = {s.lower() for s in _exhausted_sources} + remaining_sources = [ + s for s in hybrid_order[1:] + if s in source_clients and source_clients[s] + and s.lower() not in _exhausted_lower + ] if remaining_sources: logger.warning(f"[Hybrid Fallback] Primary source had no valid matches. Trying fallback sources: {remaining_sources}") diff --git a/tests/imports/test_import_pipeline.py b/tests/imports/test_import_pipeline.py index e6b6891a..a3651254 100644 --- a/tests/imports/test_import_pipeline.py +++ b/tests/imports/test_import_pipeline.py @@ -567,3 +567,79 @@ def test_exhaustive_absolute_ceiling_guards_runaway(monkeypatch): assert task["status"] == "failed" assert submitted == [] + + +def _wire_orchestrator(monkeypatch, mode, hybrid_order): + """Wire monitor's download_orchestrator so per-source budget exhaustion can + decide whether another source remains to fall back to.""" + import core.downloads.monitor as monitor + orch = types.SimpleNamespace(mode=mode, hybrid_order=list(hybrid_order)) + monkeypatch.setattr(monitor, "download_orchestrator", orch) + return orch + + +def test_exhaustive_exhausted_source_switches_in_hybrid(monkeypatch): + submitted = _wire_retry_engine(monkeypatch) + _wire_orchestrator(monkeypatch, "hybrid", ["soulseek", "hifi"]) + _patch_config(monkeypatch, { + "post_processing.retry_exhaustive": True, + "post_processing.retries_per_query": 5, + }) + + # soulseek's budget (query_count 2 * 5 = 10) is spent. In hybrid mode the + # task switches to the next source instead of failing the whole track. + task, completion, _ = _run_wrapper_with_quarantine( + monkeypatch, _acoustid_quarantine, + task_extra={"username": "DjPeer", "query_count": 2, + "quarantine_retry_counts_by_source": {"soulseek": 10}}, + ) + + assert task["status"] == "searching" + # The spent source is flagged so the worker excludes it from the next search. + assert task["exhausted_download_sources"] == {"soulseek"} + # Its per-source counter is NOT pushed past budget — the source is simply done. + assert task["quarantine_retry_counts_by_source"]["soulseek"] == 10 + assert submitted == [("rtask", "rbatch")] + assert completion == [] + + +def test_exhaustive_all_sources_exhausted_fails_in_hybrid(monkeypatch): + submitted = _wire_retry_engine(monkeypatch) + _wire_orchestrator(monkeypatch, "hybrid", ["soulseek", "hifi"]) + _patch_config(monkeypatch, { + "post_processing.retry_exhaustive": True, + "post_processing.retries_per_query": 5, + }) + + # soulseek was exhausted on an earlier attempt; now hifi spends its last + # budget too — no fallback source remains, so the task finally fails. + task, completion, _ = _run_wrapper_with_quarantine( + monkeypatch, _acoustid_quarantine, + task_extra={"username": "hifi", "query_count": 2, + "exhausted_download_sources": {"soulseek"}, + "quarantine_retry_counts_by_source": {"hifi": 10}}, + ) + + assert task["status"] == "failed" + assert submitted == [] + assert completion == [("rbatch", "rtask", False)] + + +def test_exhaustive_single_source_exhausted_fails(monkeypatch): + submitted = _wire_retry_engine(monkeypatch) + # Single-source mode: nothing to fall back to once the budget is spent. + _wire_orchestrator(monkeypatch, "soulseek", []) + _patch_config(monkeypatch, { + "post_processing.retry_exhaustive": True, + "post_processing.retries_per_query": 5, + }) + + task, completion, _ = _run_wrapper_with_quarantine( + monkeypatch, _acoustid_quarantine, + task_extra={"username": "DjPeer", "query_count": 2, + "quarantine_retry_counts_by_source": {"soulseek": 10}}, + ) + + assert task["status"] == "failed" + assert submitted == [] + assert completion == [("rbatch", "rtask", False)] diff --git a/webui/index.html b/webui/index.html index cfed5f5f..9570cbda 100644 --- a/webui/index.html +++ b/webui/index.html @@ -5579,7 +5579,7 @@
- + In exhaustive mode, how many candidates to try per search query per source. The per-source budget is number of search queries × this value. Only used when Exhaustive retry is on.
From 07ca7eacfaab05a9564a68961530d89b5abf4490 Mon Sep 17 00:00:00 2001 From: dev Date: Fri, 5 Jun 2026 17:25:47 +0200 Subject: [PATCH 04/14] =?UTF-8?q?Downloads:=20cached-first=20quarantine=20?= =?UTF-8?q?retry=20=E2=80=94=20stop=20re-searching=20the=20same=20source?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Each AcoustID/integrity quarantine retry re-ran the FULL search (all queries, all sources) before picking the next-best candidate — so a track that failed verification a dozen times re-queried Soulseek a dozen times (~3 min/cycle in the field). The next-best pick was already sitting in cached_candidates. Now the monitor flags the re-queue as a quarantine retry; the worker walks the already-found candidates first (skipping used + budget-exhausted sources) and hands them straight to the download path — no search. A source is searched exactly once: once its candidates are cached, later quarantine retries exclude it (searched_sources) so the hybrid chain falls through to a not-yet-searched source instead of re-querying the spent one. Fresh downloads and the monitor's dead-connection/stuck retries clear searched_sources and search fresh, so the only re-search is for a genuinely new source or a dead peer. Co-Authored-By: Claude Opus 4.8 --- core/downloads/monitor.py | 6 + core/downloads/task_worker.py | 110 +++++++++++++++- tests/downloads/test_downloads_task_worker.py | 117 ++++++++++++++++++ tests/imports/test_import_pipeline.py | 14 +++ 4 files changed, 244 insertions(+), 3 deletions(-) diff --git a/core/downloads/monitor.py b/core/downloads/monitor.py index 273d1d45..472e521c 100644 --- a/core/downloads/monitor.py +++ b/core/downloads/monitor.py @@ -237,6 +237,12 @@ def requeue_quarantined_task_for_retry(task_id, batch_id, trigger): task['used_sources'] = used_sources task['quarantine_retry_count'] = total_count + 1 + # Flag the re-run as a quarantine retry so the worker walks the + # already-found candidates (cached-first) before re-searching — the + # connection was fine, the content was just wrong. Dead-connection / + # stuck retries (handled elsewhere in the monitor) deliberately do NOT + # set this, so they re-search fresh. + task['_quarantine_retry'] = True # Drop the stale download identity + the prior attempt's quarantine link. task.pop('download_id', None) task.pop('username', None) diff --git a/core/downloads/task_worker.py b/core/downloads/task_worker.py index 092d589f..00086c8c 100644 --- a/core/downloads/task_worker.py +++ b/core/downloads/task_worker.py @@ -31,6 +31,65 @@ from core.spotify_client import Track as SpotifyTrack logger = logging.getLogger(__name__) +def _resolve_worker_source(username): + """Logical source bucket for a candidate's username (Soulseek peers all + collapse to 'soulseek'; streaming sources keep their name). Mirrors the + monitor's resolver — imported lazily to avoid an import cycle.""" + try: + from core.downloads.monitor import _resolve_download_source + return _resolve_download_source(username) + except Exception: + return 'soulseek' + + +def _cand_user_file(candidate): + """Read (username, filename) from a candidate that may be a TrackResult + object or a plain dict (tests / cached raw rows).""" + if isinstance(candidate, dict): + return candidate.get('username'), candidate.get('filename') + return getattr(candidate, 'username', None), getattr(candidate, 'filename', None) + + +def _try_cached_candidates(task_id, batch_id, track, deps): + """Quarantine-retry fast path: attempt the already-found candidates before + re-searching anything. + + When a verified-bad file is re-queued, the connection was fine (the file + downloaded, it was just the wrong/broken content) — so the next-best pick is + almost always already sitting in ``cached_candidates``. Walk those (skipping + sources already tried or budget-exhausted) and hand them to the normal + download path. Returns True if a download was started; False to fall through + to a fresh search (which only happens for a not-yet-searched source). + """ + with tasks_lock: + task = download_tasks.get(task_id) + if not task: + return False + cached = list(task.get('cached_candidates') or []) + used = set(task.get('used_sources') or ()) + exhausted = {str(s).lower() for s in (task.get('exhausted_download_sources') or ())} + + remaining = [] + for c in cached: + uname, fname = _cand_user_file(c) + if not uname or not fname: + continue + if f"{uname}_{fname}" in used: + continue + if _resolve_worker_source(uname).lower() in exhausted: + continue + remaining.append(c) + + if not remaining: + return False + + logger.info( + f"[Modal Worker] Quarantine retry: trying {len(remaining)} cached " + f"candidate(s) before re-searching (task {task_id})" + ) + return deps.attempt_download_with_candidates(task_id, remaining, track, batch_id) + + def _private_album_bundle_staging_miss_reason(batch_id: Optional[str], deps: Any) -> Optional[str]: """Return a user-facing miss reason when per-track search should stop. @@ -206,6 +265,26 @@ def download_track_worker(task_id: str, batch_id: Optional[str], deps: TaskWorke download_tasks[task_id]['used_sources'] = set() # Else: keep existing used_sources to avoid retrying same failed hosts + # Cached-first quarantine retry. The monitor sets ``_quarantine_retry`` + # when a verified-bad file is re-queued; in that case we walk the + # already-found candidates before re-searching (the connection was fine, + # just the content was wrong). A NON-quarantine entry (fresh download, or + # the monitor's dead-connection/stuck retry) instead starts a new search + # generation: clear the searched-source memory so each source can be + # searched fresh again. + with tasks_lock: + _t = download_tasks.get(task_id, {}) + is_quarantine_retry = bool(_t.pop('_quarantine_retry', False)) + if not is_quarantine_retry: + _t.pop('searched_sources', None) + if is_quarantine_retry and _try_cached_candidates(task_id, batch_id, track, deps): + with tasks_lock: + used_filename = download_tasks.get(task_id, {}).get('filename') + used_username = download_tasks.get(task_id, {}).get('username') + if used_filename and used_username: + deps.store_batch_source(batch_id, used_username, used_filename) + return + # 1. Generate multiple search queries (like GUI's generate_smart_search_queries) artist_name = track.artists[0] if track.artists else None track_name = track.name @@ -294,10 +373,16 @@ def download_track_worker(task_id: str, batch_id: Optional[str], deps: TaskWorke # source instead of re-fetching the same exhausted one (e.g. Soulseek # keeps returning fresh wrong peers — once its budget is gone, switch to # HiFi/Tidal/…). See monitor.requeue_quarantined_task_for_retry. + # On a quarantine retry we also exclude sources we've ALREADY searched: + # their candidates are walked via the cached-first path, so re-searching + # them is the wasteful repeat the cached-first design removes. The chain + # then falls through to a not-yet-searched source. Fresh / dead-connection + # runs cleared searched_sources above, so they search everything again. with tasks_lock: - _exhausted_sources = [ - str(s) for s in (download_tasks.get(task_id, {}).get('exhausted_download_sources') or ()) - ] + _t = download_tasks.get(task_id, {}) + _exhausted_sources = [str(s) for s in (_t.get('exhausted_download_sources') or ())] + if is_quarantine_retry: + _exhausted_sources.extend(str(s) for s in (_t.get('searched_sources') or ())) for query_index, query in enumerate(search_queries): # Cancellation check before each query with tasks_lock: @@ -373,6 +458,17 @@ def download_track_worker(task_id: str, batch_id: Optional[str], deps: TaskWorke return # Store candidates for retry fallback (like GUI) download_tasks[task_id]['cached_candidates'] = candidates + # Remember which sources produced candidates so a later + # quarantine retry walks them via cache instead of + # re-searching them (cached-first design). + _searched = download_tasks[task_id].get('searched_sources') + if not isinstance(_searched, set): + _searched = set() + for _c in candidates: + _u, _ = _cand_user_file(_c) + if _u: + _searched.add(_resolve_worker_source(_u)) + download_tasks[task_id]['searched_sources'] = _searched # Try to download with these candidates success = deps.attempt_download_with_candidates(task_id, candidates, track, batch_id) @@ -457,6 +553,14 @@ def download_track_worker(task_id: str, batch_id: Optional[str], deps: TaskWorke fb_candidates = deps.get_valid_candidates(fb_results, track, fb_query) if fb_candidates: logger.warning(f"[Hybrid Fallback] {fallback_source} found {len(fb_candidates)} valid candidates!") + with tasks_lock: + if task_id in download_tasks: + download_tasks[task_id]['cached_candidates'] = fb_candidates + _searched = download_tasks[task_id].get('searched_sources') + if not isinstance(_searched, set): + _searched = set() + _searched.add(_resolve_worker_source(fallback_source)) + download_tasks[task_id]['searched_sources'] = _searched success = deps.attempt_download_with_candidates(task_id, fb_candidates, track, batch_id) if success: return diff --git a/tests/downloads/test_downloads_task_worker.py b/tests/downloads/test_downloads_task_worker.py index eea39ead..84d9a4e8 100644 --- a/tests/downloads/test_downloads_task_worker.py +++ b/tests/downloads/test_downloads_task_worker.py @@ -45,6 +45,7 @@ class _FakeClient: self._results = results if results is not None else [] self.mode = mode self.search_calls = [] + self.exclude_calls = [] # exclude_sources arg per search() call self._client_map = {} for k, v in (subclients or {}).items(): if k in self._CLIENT_NAMES: @@ -58,6 +59,7 @@ class _FakeClient: async def search(self, query, timeout=30, exclude_sources=None): self.search_calls.append((query, timeout)) + self.exclude_calls.append(exclude_sources) return (self._results, None) @@ -506,6 +508,121 @@ def test_hybrid_fallback_skipped_when_mode_not_hybrid(): assert yt.search_calls == [] +# --------------------------------------------------------------------------- +# Cached-first quarantine retry: walk already-found candidates before any +# re-search; never re-search a source whose candidates are spent (only switch +# to a not-yet-searched source). See task_worker cached-first phase. +# --------------------------------------------------------------------------- + +class _Cand: + def __init__(self, username, filename): + self.username = username + self.filename = filename + + +def test_quarantine_retry_tries_cached_candidates_without_searching(): + _seed_task( + _quarantine_retry=True, + cached_candidates=[_Cand('peerA', 'f1.flac'), _Cand('peerB', 'f2.flac')], + used_sources={'peerA_f1.flac'}, # peerA already tried + ) + attempted = [] + + def _attempt(task_id, candidates, track, batch_id): + attempted.append([getattr(c, 'filename', None) for c in candidates]) + return True + + sk = _FakeClient(results=['should-not-be-used']) + deps, _ = _build_deps( + soulseek=sk, + matching=_FakeMatchEngine(queries=['q1']), + attempt_download_with_candidates=_attempt, + ) + tw.download_track_worker('t1', 'b1', deps) + # No search performed — cached candidates used directly. + assert sk.search_calls == [] + # Only the unused candidate (peerB) was passed to attempt. + assert attempted == [['f2.flac']] + + +def test_quarantine_retry_skips_cached_from_exhausted_source(): + # hifi is budget-exhausted; its cached candidate must be skipped, soulseek's tried. + _seed_task( + _quarantine_retry=True, + cached_candidates=[_Cand('hifi', 'h.flac'), _Cand('peerB', 'f2.flac')], + used_sources=set(), + exhausted_download_sources={'hifi'}, + ) + attempted = [] + + def _attempt(task_id, candidates, track, batch_id): + attempted.append([getattr(c, 'username', None) for c in candidates]) + return True + + sk = _FakeClient(results=['x']) + deps, _ = _build_deps( + soulseek=sk, + matching=_FakeMatchEngine(queries=['q1']), + attempt_download_with_candidates=_attempt, + ) + tw.download_track_worker('t1', 'b1', deps) + assert sk.search_calls == [] + assert attempted == [['peerB']] # hifi candidate excluded + + +def test_quarantine_retry_searches_excluding_searched_source_when_cached_spent(): + # All cached used → Phase A finds nothing → search runs, but excludes the + # already-searched source so the chain falls through to a new source. + _seed_task( + _quarantine_retry=True, + cached_candidates=[_Cand('peerA', 'f1.flac')], + used_sources={'peerA_f1.flac'}, + searched_sources={'soulseek'}, + ) + sk = _FakeClient(results=[], mode='hybrid', + subclients={'hybrid_order': ['soulseek', 'hifi']}) + deps, _ = _build_deps( + soulseek=sk, + matching=_FakeMatchEngine(queries=['q1']), + ) + tw.download_track_worker('t1', 'b1', deps) + # Search ran (cached spent) and every call excluded the searched soulseek source. + assert sk.search_calls + assert all(ex and 'soulseek' in ex for ex in sk.exclude_calls) + + +def test_non_quarantine_run_ignores_cached_first_and_searches(): + # A fresh (non-quarantine) run must NOT use cached-first — it searches. + _seed_task( + cached_candidates=[_Cand('peerA', 'f1.flac')], + used_sources=set(), + ) + sk = _FakeClient(results=[]) + deps, _ = _build_deps( + soulseek=sk, + matching=_FakeMatchEngine(queries=['q1']), + attempt_download_with_candidates=lambda *a, **kw: False, + ) + tw.download_track_worker('t1', 'b1', deps) + assert sk.search_calls # searched, did not short-circuit on cache + + +def test_non_quarantine_run_resets_searched_sources(): + # A fresh / dead-connection retry starts a new search generation: the stale + # searched-source memory is cleared so sources can be searched again. + _seed_task( + cached_candidates=[], + searched_sources={'soulseek', 'hifi'}, + ) + sk = _FakeClient(results=[]) + deps, _ = _build_deps( + soulseek=sk, + matching=_FakeMatchEngine(queries=['q1']), + ) + tw.download_track_worker('t1', 'b1', deps) + assert download_tasks['t1'].get('searched_sources', set()) == set() + + # --------------------------------------------------------------------------- # Top-level exception path # --------------------------------------------------------------------------- diff --git a/tests/imports/test_import_pipeline.py b/tests/imports/test_import_pipeline.py index a3651254..e602ab81 100644 --- a/tests/imports/test_import_pipeline.py +++ b/tests/imports/test_import_pipeline.py @@ -403,6 +403,20 @@ def test_acoustid_mismatch_requeues_next_candidate(monkeypatch): assert context_key not in runtime_state.matched_downloads_context +def test_requeue_flags_quarantine_retry_for_cached_first(monkeypatch): + _wire_retry_engine(monkeypatch) + + def _fake_inner(ck, ctx, fp, runtime, metadata_runtime=None): + ctx["_acoustid_quarantined"] = True + ctx["_acoustid_failure_msg"] = "wrong song" + + task, _, _ = _run_wrapper_with_quarantine(monkeypatch, _fake_inner) + + # The re-run is flagged so the worker walks cached candidates before + # re-searching (cached-first), rather than re-running the full search. + assert task["_quarantine_retry"] is True + + def test_integrity_mismatch_requeues_next_candidate(monkeypatch): submitted = _wire_retry_engine(monkeypatch) From 70732ad80e602381aaa8333d7f5f11a2fea0f3d3 Mon Sep 17 00:00:00 2001 From: dev Date: Fri, 5 Jun 2026 19:12:36 +0200 Subject: [PATCH 05/14] =?UTF-8?q?Downloads:=20lazy=20multi-query=20quarant?= =?UTF-8?q?ine=20retry=20=E2=80=94=20exhaust=20all=20queries=20per=20sourc?= =?UTF-8?q?e?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The cached-first retry (8d98b755) abandoned a source after a single query: the first run returns as soon as ONE query starts a download, so cached_candidates held only that query's results. On a quarantine retry the whole source was then excluded from re-search (via searched_sources), so the later queries (e.g. "artist + album") never hit that source again — it jumped to the next source after one query instead of exhausting all queries per source. Track searched QUERIES (searched_queries) instead of whole sources. A quarantine retry now skips only the already-run queries (their candidates are walked via cached-first) and still searches the not-yet-run queries against the same source. Budget-exhausted sources (exhaustive mode) stay excluded, so the source switch still fires when a source is genuinely spent. Removes the now-dead searched_sources state (written but no longer read). Co-Authored-By: Claude Opus 4.8 --- core/downloads/task_worker.py | 64 ++++++---- tests/downloads/test_downloads_task_worker.py | 111 ++++++++++++++---- 2 files changed, 129 insertions(+), 46 deletions(-) diff --git a/core/downloads/task_worker.py b/core/downloads/task_worker.py index 00086c8c..3850dd2d 100644 --- a/core/downloads/task_worker.py +++ b/core/downloads/task_worker.py @@ -276,7 +276,7 @@ def download_track_worker(task_id: str, batch_id: Optional[str], deps: TaskWorke _t = download_tasks.get(task_id, {}) is_quarantine_retry = bool(_t.pop('_quarantine_retry', False)) if not is_quarantine_retry: - _t.pop('searched_sources', None) + _t.pop('searched_queries', None) if is_quarantine_retry and _try_cached_candidates(task_id, batch_id, track, deps): with tasks_lock: used_filename = download_tasks.get(task_id, {}).get('filename') @@ -373,16 +373,23 @@ def download_track_worker(task_id: str, batch_id: Optional[str], deps: TaskWorke # source instead of re-fetching the same exhausted one (e.g. Soulseek # keeps returning fresh wrong peers — once its budget is gone, switch to # HiFi/Tidal/…). See monitor.requeue_quarantined_task_for_retry. - # On a quarantine retry we also exclude sources we've ALREADY searched: - # their candidates are walked via the cached-first path, so re-searching - # them is the wasteful repeat the cached-first design removes. The chain - # then falls through to a not-yet-searched source. Fresh / dead-connection - # runs cleared searched_sources above, so they search everything again. + # + # On a quarantine retry we do NOT exclude a source just because it was + # searched once: the first run only ran ONE query before starting a + # download, so the later queries (e.g. "artist + album") have never hit + # that source yet and may surface the correct upload. Instead we remember + # which QUERIES already ran (``searched_queries``) and skip re-running + # only those — their candidates are walked via the cached-first path + # above. The not-yet-searched queries still search the same source, so + # every query is exhausted per source before the chain switches sources. + # Fresh / dead-connection runs cleared searched_queries above, so they + # search everything again. with tasks_lock: _t = download_tasks.get(task_id, {}) _exhausted_sources = [str(s) for s in (_t.get('exhausted_download_sources') or ())] - if is_quarantine_retry: - _exhausted_sources.extend(str(s) for s in (_t.get('searched_sources') or ())) + _searched_queries = ( + set(_t.get('searched_queries') or ()) if is_quarantine_retry else set() + ) for query_index, query in enumerate(search_queries): # Cancellation check before each query with tasks_lock: @@ -395,6 +402,17 @@ def download_track_worker(task_id: str, batch_id: Optional[str], deps: TaskWorke return download_tasks[task_id]['current_query_index'] = query_index + # Cached-first: a query already run last generation has its candidates + # sitting in cache (walked above) — re-searching it is the wasteful + # repeat the cached-first design removes. Skip it; the not-yet-run + # queries below still search this source. + if is_quarantine_retry and query in _searched_queries: + logger.debug( + f"[Modal Worker] Skipping already-searched query '{query}' " + f"(candidates served from cache) for task {task_id}" + ) + continue + logger.debug(f"[Modal Worker] Query {query_index + 1}/{len(search_queries)}: '{query}'") logger.debug(f"About to call soulseek search for task {task_id}") @@ -434,6 +452,16 @@ def download_track_worker(task_id: str, batch_id: Optional[str], deps: TaskWorke if task_id not in download_tasks: logger.info(f"[Modal Worker] Task {task_id} was deleted after search returned") return + # Remember this query ran so a later quarantine retry skips + # re-searching it (its candidates are walked via cached-first). + # Recorded regardless of result count: re-running a query is + # deterministic, so a query that returned nothing won't return + # anything new next time either. + _sq = download_tasks[task_id].get('searched_queries') + if not isinstance(_sq, set): + _sq = set() + _sq.add(query) + download_tasks[task_id]['searched_queries'] = _sq if download_tasks[task_id]['status'] == 'cancelled': logger.warning(f"[Modal Worker] Task {task_id} cancelled after search returned - ignoring results") # Don't call _on_download_completed for cancelled tasks as it can stop monitoring @@ -456,19 +484,10 @@ def download_track_worker(task_id: str, batch_id: Optional[str], deps: TaskWorke logger.warning(f"[Modal Worker] Task {task_id} cancelled before processing candidates") # Don't call _on_download_completed for cancelled tasks as it can stop monitoring return - # Store candidates for retry fallback (like GUI) + # Store candidates for retry fallback (like GUI). A + # later quarantine retry walks these via cached-first + # and skips re-searching this query (searched_queries). download_tasks[task_id]['cached_candidates'] = candidates - # Remember which sources produced candidates so a later - # quarantine retry walks them via cache instead of - # re-searching them (cached-first design). - _searched = download_tasks[task_id].get('searched_sources') - if not isinstance(_searched, set): - _searched = set() - for _c in candidates: - _u, _ = _cand_user_file(_c) - if _u: - _searched.add(_resolve_worker_source(_u)) - download_tasks[task_id]['searched_sources'] = _searched # Try to download with these candidates success = deps.attempt_download_with_candidates(task_id, candidates, track, batch_id) @@ -556,11 +575,6 @@ def download_track_worker(task_id: str, batch_id: Optional[str], deps: TaskWorke with tasks_lock: if task_id in download_tasks: download_tasks[task_id]['cached_candidates'] = fb_candidates - _searched = download_tasks[task_id].get('searched_sources') - if not isinstance(_searched, set): - _searched = set() - _searched.add(_resolve_worker_source(fallback_source)) - download_tasks[task_id]['searched_sources'] = _searched success = deps.attempt_download_with_candidates(task_id, fb_candidates, track, batch_id) if success: return diff --git a/tests/downloads/test_downloads_task_worker.py b/tests/downloads/test_downloads_task_worker.py index 84d9a4e8..39320940 100644 --- a/tests/downloads/test_downloads_task_worker.py +++ b/tests/downloads/test_downloads_task_worker.py @@ -570,14 +570,67 @@ def test_quarantine_retry_skips_cached_from_exhausted_source(): assert attempted == [['peerB']] # hifi candidate excluded -def test_quarantine_retry_searches_excluding_searched_source_when_cached_spent(): - # All cached used → Phase A finds nothing → search runs, but excludes the - # already-searched source so the chain falls through to a new source. +# A track_info that yields exactly the engine queries (no artist → no +# first-word legacy query; name equals a query → track-only query dedupes away), +# so the generated query set is deterministic for cached-first assertions. +_SOLO_TRACK = {'id': 'sp-1', 'name': 'Solo', 'artists': [], + 'album': 'A', 'duration_ms': 1000} + + +def test_quarantine_retry_searches_unsearched_query_without_excluding_source(): + # Cache spent + 'Solo' already searched, but 'q2' is NOT yet searched. The + # retry must search q2 against the SAME source (no source-level exclusion) so + # every query is exhausted per source before the chain switches sources + # (lazy multi-query retry). _seed_task( + track_info=dict(_SOLO_TRACK), _quarantine_retry=True, cached_candidates=[_Cand('peerA', 'f1.flac')], used_sources={'peerA_f1.flac'}, - searched_sources={'soulseek'}, + searched_queries={'Solo'}, + ) + sk = _FakeClient(results=[], mode='hybrid', + subclients={'hybrid_order': ['soulseek', 'hifi']}) + deps, _ = _build_deps( + soulseek=sk, + matching=_FakeMatchEngine(queries=['Solo', 'q2']), + ) + tw.download_track_worker('t1', 'b1', deps) + # Only q2 was searched — 'Solo' skipped because its candidates were cached. + assert [c[0] for c in sk.search_calls] == ['q2'] + # Soulseek NOT excluded: the not-yet-searched query still hits it. + assert all((not ex) or 'soulseek' not in ex for ex in sk.exclude_calls) + + +def test_quarantine_retry_skips_already_searched_query_no_research(): + # The only generated query is already searched and cache spent → it is NOT + # re-searched (its candidates live in cache). This is the wasteful repeat the + # cached-first design removes. + _seed_task( + track_info=dict(_SOLO_TRACK), + _quarantine_retry=True, + cached_candidates=[_Cand('peerA', 'f1.flac')], + used_sources={'peerA_f1.flac'}, + searched_queries={'Solo'}, + ) + sk = _FakeClient(results=[], mode='hybrid', + subclients={'hybrid_order': ['soulseek', 'hifi']}) + deps, _ = _build_deps( + soulseek=sk, + matching=_FakeMatchEngine(queries=['Solo']), + ) + tw.download_track_worker('t1', 'b1', deps) + assert sk.search_calls == [] # 'Solo' not re-searched + + +def test_quarantine_retry_still_excludes_budget_exhausted_source(): + # A source whose per-source budget is spent (exhaustive mode) stays excluded + # so the chain falls through to the next source. + _seed_task( + _quarantine_retry=True, + cached_candidates=[], + searched_queries=set(), + exhausted_download_sources={'soulseek'}, ) sk = _FakeClient(results=[], mode='hybrid', subclients={'hybrid_order': ['soulseek', 'hifi']}) @@ -586,11 +639,43 @@ def test_quarantine_retry_searches_excluding_searched_source_when_cached_spent() matching=_FakeMatchEngine(queries=['q1']), ) tw.download_track_worker('t1', 'b1', deps) - # Search ran (cached spent) and every call excluded the searched soulseek source. assert sk.search_calls assert all(ex and 'soulseek' in ex for ex in sk.exclude_calls) +def test_search_records_searched_queries(): + # Every query the worker actually runs is recorded so a later quarantine + # retry can skip re-searching it. + _seed_task(status='pending') + sk = _FakeClient(results=['r1']) + deps, _ = _build_deps( + soulseek=sk, + matching=_FakeMatchEngine(queries=['q1']), + get_valid_candidates=lambda r, t, q: [], # no candidates → loop completes + ) + tw.download_track_worker('t1', 'b1', deps) + assert 'q1' in download_tasks['t1'].get('searched_queries', set()) + + +def test_non_quarantine_run_resets_searched_queries(): + # A fresh / dead-connection retry starts a new search generation: the stale + # searched-query memory is cleared so every query can be searched again. + _seed_task( + track_info=dict(_SOLO_TRACK), + cached_candidates=[], + searched_queries={'stale-q'}, + ) + sk = _FakeClient(results=[]) + deps, _ = _build_deps( + soulseek=sk, + matching=_FakeMatchEngine(queries=['Solo']), + ) + tw.download_track_worker('t1', 'b1', deps) + sq = download_tasks['t1'].get('searched_queries', set()) + assert 'stale-q' not in sq # stale generation cleared + assert 'Solo' in sq # current generation recorded fresh + + def test_non_quarantine_run_ignores_cached_first_and_searches(): # A fresh (non-quarantine) run must NOT use cached-first — it searches. _seed_task( @@ -607,22 +692,6 @@ def test_non_quarantine_run_ignores_cached_first_and_searches(): assert sk.search_calls # searched, did not short-circuit on cache -def test_non_quarantine_run_resets_searched_sources(): - # A fresh / dead-connection retry starts a new search generation: the stale - # searched-source memory is cleared so sources can be searched again. - _seed_task( - cached_candidates=[], - searched_sources={'soulseek', 'hifi'}, - ) - sk = _FakeClient(results=[]) - deps, _ = _build_deps( - soulseek=sk, - matching=_FakeMatchEngine(queries=['q1']), - ) - tw.download_track_worker('t1', 'b1', deps) - assert download_tasks['t1'].get('searched_sources', set()) == set() - - # --------------------------------------------------------------------------- # Top-level exception path # --------------------------------------------------------------------------- From 6cb5d455f9d40dafe95302f519ad575b9c1eeb6a Mon Sep 17 00:00:00 2001 From: dev Date: Fri, 5 Jun 2026 21:34:37 +0200 Subject: [PATCH 06/14] MusicBrainz: alias trust-gate evaluates MB-score leader, not combined leader MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The cross-script alias bridge (#442/#586) silently returned [] for some artists ("Sawano Hiroyuki"). Root cause: the mb-only escape — built exactly for the case where local string similarity is ~0 (romaji↔kanji) but MB's own score is decisive — inspected scored[0], the COMBINED-score leader. When an unrelated same-script decoy outranks the real artist on combined score (decoy: sim 0.82 + mb_score 83 → combined 0.82, just under the 0.85 bar; real '澤野弘之': sim 0 + mb_score 100 → combined 0.30, sorted last), the gate saw the decoy's mb_score 83 (< 95), failed both paths, and cached an empty alias result. Verification then scored the kanji artist 0% against the romaji expected name and quarantined every correct file. Evaluate the MB-SCORE leader independently of combined ranking for the mb-only escape, and pull aliases from whichever entity actually passed (combined leader for the combined path, MB-score leader for the mb-only path). The unambiguity check now compares the top two raw MB scores. Same-script and single-result paths are unchanged (regression-guarded by the existing #442/#586 tests). Co-Authored-By: Claude Opus 4.8 --- core/musicbrainz_service.py | 49 +++++++++++++------ .../matching/test_artist_alias_lookup_586.py | 29 +++++++++++ 2 files changed, 63 insertions(+), 15 deletions(-) diff --git a/core/musicbrainz_service.py b/core/musicbrainz_service.py index d9085300..b7d47505 100644 --- a/core/musicbrainz_service.py +++ b/core/musicbrainz_service.py @@ -448,31 +448,50 @@ class MusicBrainzService: scored.sort(key=lambda x: -x[0]) best_score, best_mbid, best_mb_score = scored[0] + # The genuine cross-script match (romaji↔kanji, latin↔cyrillic) + # has near-zero LOCAL similarity, so its COMBINED score sinks + # below an unrelated same-script decoy — even though MB itself is + # certain. "Sawano Hiroyuki": a decoy entity led on combined + # (sim 0.82, mb_score 83, combined 0.82 — just under the 0.85 bar) + # while the real artist '澤野弘之' had mb_score 100 but combined + # 0.30, sorted last. So evaluate the MB-SCORE leader independently + # of the combined ranking for the mb-only escape, not scored[0]. + mb_leader = max(scored, key=lambda x: x[2]) # (combined, mbid, raw_mb) + mb_scores_desc = sorted((x[2] for x in scored), reverse=True) + mb_unambiguous = len(mb_scores_desc) < 2 or (mb_scores_desc[0] - mb_scores_desc[1]) >= 5 + # Trust gate. Two ways to pass: # 1. Combined score >= 0.85 (the historical strict bar that - # catches same-script matches) - # 2. MB's OWN score is very high (>= 95) AND the result is - # unambiguous (top result clearly leads). Bridges the - # cross-script case where local similarity is near zero - # ("Dmitry Yablonsky" vs "Дмитрий Яблонский" sim ~0) - # but MB's index found a high-confidence match. + # catches same-script matches) → trust the combined leader. + # 2. MB's OWN score is very high (>= 95) AND that MB-score leader + # is unambiguous → trust IT. Bridges the cross-script case + # where local similarity is near zero ("Dmitry Yablonsky" vs + # "Дмитрий Яблонский" sim ~0) but MB's index is confident. passes_combined = best_score >= 0.85 - passes_mb_only = best_mb_score >= 95 and ( - len(scored) < 2 or (scored[0][2] - scored[1][2]) >= 5 - ) + passes_mb_only = mb_leader[2] >= 95 and mb_unambiguous if not (passes_combined or passes_mb_only): logger.debug( "lookup_artist_aliases: best match for %r below trust " - "threshold (combined=%.2f, mb_score=%d)", - artist_name, best_score, best_mb_score, + "threshold (combined=%.2f, best_mb=%d, leader_mb=%d)", + artist_name, best_score, best_mb_score, mb_leader[2], ) self._save_to_cache('artist_aliases', artist_name, None, None, {'aliases': []}, 0) return [] + # Pick the entity to pull aliases from. Combined-strong matches use + # the combined leader; the mb-only escape uses the MB-score leader + # (which may differ from scored[0] in the cross-script case above). + if passes_combined: + chosen_mbid, chosen_conf = best_mbid, best_score + else: + chosen_mbid, chosen_conf = mb_leader[1], mb_leader[2] / 100.0 + # Ambiguity detection: when 2+ results both score high (within # 0.1 of the best combined), the search hit multiple distinct # artists with similar names. Pulling aliases for one could - # produce wrong matches. Skip + cache empty. + # produce wrong matches. Skip + cache empty. The unambiguous + # MB-score leader (passes_mb_only) is exempt — its decisiveness + # was already checked via mb_unambiguous. if len(scored) >= 2 and (scored[0][0] - scored[1][0]) < 0.1 and not passes_mb_only: logger.debug( "lookup_artist_aliases: ambiguous match for %r — top " @@ -482,10 +501,10 @@ class MusicBrainzService: self._save_to_cache('artist_aliases', artist_name, None, None, {'aliases': []}, 0) return [] - aliases = self.fetch_artist_aliases(best_mbid) + aliases = self.fetch_artist_aliases(chosen_mbid) self._save_to_cache( - 'artist_aliases', artist_name, None, best_mbid, - {'aliases': aliases}, int(best_score * 100), + 'artist_aliases', artist_name, None, chosen_mbid, + {'aliases': aliases}, int(chosen_conf * 100), ) return aliases diff --git a/tests/matching/test_artist_alias_lookup_586.py b/tests/matching/test_artist_alias_lookup_586.py index 97c9f03d..e67ca03b 100644 --- a/tests/matching/test_artist_alias_lookup_586.py +++ b/tests/matching/test_artist_alias_lookup_586.py @@ -191,6 +191,35 @@ def test_trust_gate_rejects_when_two_high_mb_scores_tie(service): assert aliases == [] +def test_trust_gate_uses_mb_score_leader_not_combined_leader(service): + # Production case "Sawano Hiroyuki": a same-script DECOY entity leads on + # COMBINED score (high local sim, mb_score 83) but sits just under the + # 0.85 combined bar, while the genuine cross-script artist has mb_score + # 100 and ~0 local sim → lowest combined, sorted last. The mb-only escape + # must evaluate the MB-SCORE leader, not scored[0] (the combined leader), + # otherwise it inspects mb_score 83 < 95 and wrongly returns []. + service._calculate_similarity = ( + lambda a, b: 0.82 if b == 'SawanoHiroyuki[nZk]' else 0.0 + ) + service.mb_client.search_artist.return_value = [ + {'id': 'mbid-decoy', 'name': 'SawanoHiroyuki[nZk]', 'score': 83}, + {'id': 'mbid-canonical', 'name': '澤野弘之', 'score': 100}, + ] + + def get_artist(mbid, **kwargs): + if mbid == 'mbid-canonical': + return {'name': '澤野弘之', 'aliases': [{'name': 'Hiroyuki Sawano'}]} + return {'name': 'SawanoHiroyuki[nZk]', 'aliases': []} + service.mb_client.get_artist.side_effect = get_artist + + aliases = service.lookup_artist_aliases('Sawano Hiroyuki') + # The canonical kanji name must come back (its alias set was fetched). + assert '澤野弘之' in aliases + # And we must have fetched the MB-score leader, not the decoy. + service.mb_client.get_artist.assert_called_once() + assert service.mb_client.get_artist.call_args.args[0] == 'mbid-canonical' + + def test_trust_gate_passes_combined_score_when_local_sim_strong(service): # Same-script case from #442 — local sim high. Should still pass # (no regression on the existing path). From 63036a41eefe46753be3a9b27766a8192fa1d3a4 Mon Sep 17 00:00:00 2001 From: dev Date: Fri, 5 Jun 2026 22:05:10 +0200 Subject: [PATCH 07/14] WebUI: Retry/Quality settings as collapsible Downloads tiles; restore sidebar hover in reduce-effects Settings reorg (Downloads page): - Move the retry controls (retry next-best candidate, exhaustive retry, retries-per-query) out of the Post-Processing tile into a new collapsible "Retry Logic" tile on the Downloads tab (data-stg=downloads), collapsed by default. Also decouples them from the post-processing master toggle, which previously hid them when post-processing was disabled. Config keys are unchanged (still post_processing.*); settings.js binds by element id so the DOM move needs no JS change. - Wrap the existing Quality Profile group in a matching collapsible tile, collapsed by default. Sidebar (reduce-effects): - The perf PR (#793) gated .nav-button hover/active-hover behind body:not(.reduce-effects), removing the highlight entirely in reduce-effects mode. Restore the highlight there using only cheap properties (flat background + border-color; the base already reserves a 1px transparent border so there's no layout shift) while keeping the expensive gradient / translateX transform / multi-layer box-shadow off. Co-Authored-By: Claude Opus 4.8 --- webui/index.html | 63 +++++++++++++++++++++++++++++------------- webui/static/style.css | 16 +++++++++++ 2 files changed, 60 insertions(+), 19 deletions(-) diff --git a/webui/index.html b/webui/index.html index 9570cbda..61a54d93 100644 --- a/webui/index.html +++ b/webui/index.html @@ -4966,6 +4966,14 @@ + + + + + + + + +
@@ -5563,25 +5607,6 @@ Maximum drift between the file's actual length and the metadata source's expected length before the file is quarantined. 0 = auto (3s normal, 5s for tracks >10min). Raise this if tracks routinely quarantine for being a few seconds off (live recordings, alternate masterings, etc). Capped at 60s.
-
- - When a download is quarantined (AcoustID mismatch or duration/integrity failure), automatically try the next-best candidate instead of failing the track. Off = quarantine and fail immediately. -
-
- - Give every source (Soulseek, then HiFi/Tidal/…) its own retry budget. Each source spends queries × retries-per-query attempts before the track moves on. Worst case across two sources can mean many downloads — use for hard-to-match tracks (e.g. CJK artist names). Requires the option above. -
-
- - - In exhaustive mode, how many candidates to try per search query per source. The per-source budget is number of search queries × this value. Only used when Exhaustive retry is on. -
diff --git a/webui/static/style.css b/webui/static/style.css index 4c153f12..fb496285 100644 --- a/webui/static/style.css +++ b/webui/static/style.css @@ -471,6 +471,22 @@ body:not(.reduce-effects) .nav-button.active:hover { inset 0 1px 0 rgba(255, 255, 255, 0.12); } +/* Reduce-effects mode keeps the sidebar hover/active highlight — the + feedback matters for navigation — but only the CHEAP parts: a flat + background + border-color change (the base already reserves a 1px + transparent border, so no layout shift). The expensive full-effects + bits (gradient, transform/translateX, multi-layer box-shadow) stay + off so hovering doesn't trigger compositing/repaint churn. */ +body.reduce-effects .nav-button:hover { + background: rgba(255, 255, 255, 0.05); + border-color: rgba(255, 255, 255, 0.08); +} + +body.reduce-effects .nav-button.active:hover { + background: rgba(var(--accent-rgb), 0.18); + border-color: rgba(var(--accent-rgb), 0.3); +} + .nav-icon { width: 40px; height: 40px; From 81291c198b6ec3728be48763d2b176113a57d16b Mon Sep 17 00:00:00 2001 From: dev Date: Fri, 5 Jun 2026 22:15:04 +0200 Subject: [PATCH 08/14] =?UTF-8?q?WebUI:=20fix=20empty=20Quality=20Profile?= =?UTF-8?q?=20tile=20=E2=80=94=20gate=20whole=20tile,=20not=20inner=20grou?= =?UTF-8?q?p?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Quality Profile tile expanded to an empty body: settings.js updateSourceVisibility toggled only the inner #quality-profile-section (Soulseek-only + downloads-tab gate), leaving the new collapsible tile's header/body visible with hidden contents. Wrap the tile in #quality-profile-tile and gate that wrapper as a unit instead, so the whole tile shows (Soulseek active) or hides (otherwise) — no empty shell. Co-Authored-By: Claude Opus 4.8 --- webui/index.html | 7 ++++++- webui/static/settings.js | 10 ++++++---- 2 files changed, 12 insertions(+), 5 deletions(-) diff --git a/webui/index.html b/webui/index.html index 61a54d93..59568c18 100644 --- a/webui/index.html +++ b/webui/index.html @@ -4967,7 +4967,11 @@ -