diff --git a/core/imports/quarantine.py b/core/imports/quarantine.py index 0e9af594..7dd43be6 100644 --- a/core/imports/quarantine.py +++ b/core/imports/quarantine.py @@ -153,6 +153,83 @@ def get_quarantined_source_keys(quarantine_dir: str) -> set: return keys +def quarantine_group_key( + expected_artist: Any, expected_track: Any, context: Any = None +) -> Optional[str]: + """Grouping key for "the same intended download target". + + #876: when several sources are downloaded for one wishlist/queue + track they each fail verification and land in quarantine as separate + entries. They are *alternatives for the same song*, so they should + group together — and once the user accepts one, the rest are + redundant failed attempts at a song they now own. + + The key identifies the *intended* target — what SoulSync was trying to + fetch — NOT the downloaded file's own tags. That matters: the file's + metadata is frequently *wrong* (that's why it failed acoustid / + integrity), whereas the target is fixed and identical across every + alternative for one song (they're all Soulseek uploads of the *same* + source track), so grouping by it is an exact relationship, not a fuzzy + metadata guess. + + Prefers a stable target-track id from the sidecar `context.track_info` + when present — isrc, then source id, then uri — since those are exact + and constant across siblings. Falls back to the normalized + artist|track name only for legacy/thin sidecars that carry no context. + Keys are kind-prefixed so an id-based key never collides with a + name-based one. + + Returns ``None`` when nothing identifies the target (no usable id and + both name fields empty). Callers treat a ``None`` key as "its own + singleton group" — ungroupable entries must never collapse together. + """ + ti = {} + if isinstance(context, dict): + maybe_ti = context.get("track_info") + if isinstance(maybe_ti, dict): + ti = maybe_ti + isrc = str(ti.get("isrc") or "").strip().lower() + if isrc: + return f"isrc:{isrc}" + tid = str(ti.get("id") or "").strip() + if tid: + return f"id:{tid}" + uri = str(ti.get("uri") or "").strip() + if uri: + return f"uri:{uri}" + artist = " ".join(str(expected_artist or "").split()).lower() + track = " ".join(str(expected_track or "").split()).lower() + if not artist and not track: + return None + return f"nm:{artist}|{track}" + + +def find_quarantine_siblings(quarantine_dir: str, entry_id: str) -> List[str]: + """Other entry ids that share ``entry_id``'s intended-target group key. + + Returns the ids of every *other* quarantine entry whose + `expected_artist`/`expected_track` normalize to the same key as + ``entry_id`` (see :func:`quarantine_group_key`). Excludes ``entry_id`` + itself. Returns ``[]`` when the entry is missing, has an ungroupable + (``None``) key, or has no siblings. Never raises. + """ + if not entry_id: + return [] + entries = list_quarantine_entries(quarantine_dir) + target_key = None + for e in entries: + if e.get("id") == entry_id: + target_key = e.get("group_key") + break + if target_key is None: + return [] + return [ + e["id"] + for e in entries + if e.get("id") != entry_id and e.get("group_key") == target_key + ] + + def list_quarantine_entries(quarantine_dir: str) -> List[Dict[str, Any]]: """Enumerate quarantined files paired with their sidecars. @@ -213,6 +290,11 @@ def list_quarantine_entries(quarantine_dir: str) -> List[Dict[str, Any]]: "reason": sidecar.get("quarantine_reason", "Unknown reason"), "expected_track": sidecar.get("expected_track", ""), "expected_artist": sidecar.get("expected_artist", ""), + "group_key": quarantine_group_key( + sidecar.get("expected_artist", ""), + sidecar.get("expected_track", ""), + ctx, + ), "timestamp": sidecar.get("timestamp", ""), "size_bytes": size_bytes, "has_full_context": isinstance(sidecar.get("context"), dict), diff --git a/tests/imports/test_quarantine_management.py b/tests/imports/test_quarantine_management.py index 3615ca6a..7234c497 100644 --- a/tests/imports/test_quarantine_management.py +++ b/tests/imports/test_quarantine_management.py @@ -5,9 +5,11 @@ from core.imports.quarantine import ( approve_quarantine_entry, delete_quarantine_entry, entry_id_from_quarantined_filename, + find_quarantine_siblings, get_quarantine_entry_stream_info, get_quarantined_source_keys, list_quarantine_entries, + quarantine_group_key, recover_to_staging, serialize_quarantine_context, ) @@ -71,18 +73,20 @@ def test_serialize_round_trips_through_json(): # list_quarantine_entries # ────────────────────────────────────────────────────────────────────── -def _write_entry(quarantine_dir, entry_id, original_name, *, with_context=False, trigger="integrity", reason="boom", file_bytes=b"X" * 100): +def _write_entry(quarantine_dir, entry_id, original_name, *, with_context=False, trigger="integrity", reason="boom", file_bytes=b"X" * 100, expected_track="Track", expected_artist="Artist", context=None): qfile = quarantine_dir / f"{entry_id}_{original_name}.quarantined" qfile.write_bytes(file_bytes) sidecar = { "original_filename": original_name, "quarantine_reason": reason, - "expected_track": "Track", - "expected_artist": "Artist", + "expected_track": expected_track, + "expected_artist": expected_artist, "timestamp": "2026-05-14T12:00:00", "trigger": trigger, } - if with_context: + if context is not None: + sidecar["context"] = context + elif with_context: sidecar["context"] = {"track_info": {"name": "Track"}, "context_key": entry_id} sidecar_path = quarantine_dir / f"{entry_id}_{os.path.splitext(original_name)[0]}.json" sidecar_path.write_text(json.dumps(sidecar)) @@ -454,3 +458,95 @@ def test_move_with_retry_returns_false_on_missing_source(tmp_path): # attempts=1 keeps the test fast (no retry sleeps) assert _move_with_retry(str(tmp_path / "nope.flac"), str(tmp_path / "dst.flac"), attempts=1, delay=0) is False + + +# ────────────────────────────────────────────────────────────────────── +# #876: grouping alternatives for one song — quarantine_group_key / +# find_quarantine_siblings, and the group_key field on list entries. +# ────────────────────────────────────────────────────────────────────── + +def test_group_key_prefers_isrc_over_everything(): + ctx = {"track_info": {"isrc": "USRC12345678", "id": "spid", "uri": "spotify:track:x"}} + assert quarantine_group_key("Artist", "Track", ctx) == "isrc:usrc12345678" + + +def test_group_key_falls_back_to_source_id_then_uri(): + assert quarantine_group_key("A", "T", {"track_info": {"id": "abc123"}}) == "id:abc123" + assert quarantine_group_key("A", "T", {"track_info": {"uri": "spotify:track:z"}}) == "uri:spotify:track:z" + + +def test_group_key_falls_back_to_normalized_name_without_context(): + # Trivial case/whitespace differences still collapse to one key. + k1 = quarantine_group_key("Kendrick Lamar", "DNA.") + k2 = quarantine_group_key("kendrick lamar", "dna.") + assert k1 == k2 == "nm:kendrick lamar|dna." + + +def test_group_key_none_when_nothing_identifies_target(): + assert quarantine_group_key("", "", {}) is None + assert quarantine_group_key("", "", None) is None + + +def test_list_entries_carry_group_key(tmp_path): + _write_entry(tmp_path, "20260514_120000", "a.flac", + context={"track_info": {"isrc": "USABC1234567"}}) + entries = list_quarantine_entries(str(tmp_path)) + assert entries[0]["group_key"] == "isrc:usabc1234567" + + +def test_find_siblings_returns_same_target_attempts(tmp_path): + # Two failed source attempts at the SAME target track (same isrc) + an + # unrelated entry. Siblings of #2 = {#1}, never the unrelated one. + same = {"track_info": {"isrc": "USAAA0000001"}} + other = {"track_info": {"isrc": "USZZZ9999999"}} + q1, _ = _write_entry(tmp_path, "20260514_120000", "src1.flac", context=same) + q2, _ = _write_entry(tmp_path, "20260514_120001", "src2.flac", context=same) + _write_entry(tmp_path, "20260514_120002", "diff.flac", context=other) + + id1 = entry_id_from_quarantined_filename(q1.name) + id2 = entry_id_from_quarantined_filename(q2.name) + assert find_quarantine_siblings(str(tmp_path), id2) == [id1] + + +def test_find_siblings_groups_by_intended_target_not_file_tags(tmp_path): + # Same intended target (isrc) even though the bad files differ — that's + # the whole point: the file metadata is wrong, the target is constant. + same = {"track_info": {"isrc": "USAAA0000001", "name": "Whatever"}} + q1, _ = _write_entry(tmp_path, "20260514_120000", "garbage_wrong_song.flac", context=same) + q2, _ = _write_entry(tmp_path, "20260514_120001", "another_bad_rip.flac", context=same) + id1 = entry_id_from_quarantined_filename(q1.name) + id2 = entry_id_from_quarantined_filename(q2.name) + assert find_quarantine_siblings(str(tmp_path), id1) == [id2] + + +def test_find_siblings_empty_for_ungroupable_entry(tmp_path): + # No id and blank expected fields -> None key -> never grouped. + q1, _ = _write_entry(tmp_path, "20260514_120000", "orphan.flac", + expected_track="", expected_artist="") + id1 = entry_id_from_quarantined_filename(q1.name) + assert find_quarantine_siblings(str(tmp_path), id1) == [] + + +def test_find_siblings_empty_for_missing_entry(tmp_path): + _write_entry(tmp_path, "20260514_120000", "a.flac") + assert find_quarantine_siblings(str(tmp_path), "does_not_exist") == [] + + +def test_siblings_must_be_captured_before_accepted_entry_leaves_quarantine(tmp_path): + # Regression for the approve-endpoint ordering: approving RESTORES (moves) + # the accepted entry out of quarantine, after which an id-based sibling + # lookup for that id can't resolve its group_key and returns []. The + # endpoint therefore captures siblings BEFORE approving. This pins that + # invariant: lookup-before == sibling found, lookup-after == empty. + same = {"track_info": {"isrc": "USAAA0000001"}} + q1, _ = _write_entry(tmp_path, "20260514_120000", "a.flac", context=same) + q2, _ = _write_entry(tmp_path, "20260514_120001", "b.flac", context=same) + id1 = entry_id_from_quarantined_filename(q1.name) + id2 = entry_id_from_quarantined_filename(q2.name) + + captured = find_quarantine_siblings(str(tmp_path), id1) # while id1 present + assert captured == [id2] + + delete_quarantine_entry(str(tmp_path), id1) # simulate approve restoring it + + assert find_quarantine_siblings(str(tmp_path), id1) == [] # too late now diff --git a/web_server.py b/web_server.py index e53a2711..7cfd9387 100644 --- a/web_server.py +++ b/web_server.py @@ -7556,6 +7556,18 @@ def approve_quarantine_item(entry_id): docker_resolve_path(config_manager.get('soulseek.download_path', './downloads')), 'Transfer', ) + _req = request.get_json(silent=True) or {} + # #876: capture the sibling alternatives BEFORE approving — the approve + # restores (moves) this entry's file out of quarantine, after which its + # own group_key can no longer be looked up by id. Read-only here; the + # actual deletion happens only after the re-import is safely kicked off. + _sibling_ids = [] + if _req.get('remove_siblings'): + from core.imports.quarantine import find_quarantine_siblings + try: + _sibling_ids = find_quarantine_siblings(_get_quarantine_dir(), entry_id) + except Exception as sib_exc: + logger.warning(f"[Quarantine] Sibling lookup for {entry_id} failed: {sib_exc}") result = approve_quarantine_entry(_get_quarantine_dir(), entry_id, restore_dir) if result is None: return jsonify({ @@ -7574,7 +7586,6 @@ def approve_quarantine_item(entry_id): # context lost task_id/batch_id (the wrapper pops them before quarantine), # so we re-supply them here. Manager-tab approvals (no task_id) keep the # original inner-pipeline path. - _req = request.get_json(silent=True) or {} _task_id = (_req.get('task_id') or '').strip() or None _batch_id = None if _task_id: @@ -7594,7 +7605,28 @@ def approve_quarantine_item(entry_id): _reprocess = lambda: _post_process_matched_download(context_key, context, restored_path) threading.Thread(target=_reprocess, daemon=True).start() logger.info(f"[Quarantine] Approved {entry_id} (original_trigger={trigger}, bypass=all, task={_task_id}) → re-running pipeline") - return jsonify({"success": True, "trigger_bypassed": "all", "original_trigger": trigger}) + # #876: once one alternative for a song is accepted, the other + # quarantined attempts at the SAME intended target are redundant + # failed downloads of a track the user now owns. Delete the siblings + # captured above (scoped to the quarantine manager via `remove_siblings` + # — the download-modal chooser passes no flag and is unaffected). + removed_siblings = [] + if _sibling_ids: + from core.imports.quarantine import delete_quarantine_entry + try: + for sib_id in _sibling_ids: + if delete_quarantine_entry(_get_quarantine_dir(), sib_id): + removed_siblings.append(sib_id) + if removed_siblings: + logger.info(f"[Quarantine] Auto-removed {len(removed_siblings)} sibling alternative(s) of {entry_id}: {removed_siblings}") + except Exception as sib_exc: + logger.warning(f"[Quarantine] Sibling cleanup for {entry_id} failed: {sib_exc}") + return jsonify({ + "success": True, + "trigger_bypassed": "all", + "original_trigger": trigger, + "removed_siblings": removed_siblings, + }) except Exception as e: logger.error(f"[Quarantine] Error approving {entry_id}: {e}") return jsonify({"success": False, "error": str(e)}), 500 diff --git a/webui/static/style.css b/webui/static/style.css index 608ee724..6c344644 100644 --- a/webui/static/style.css +++ b/webui/static/style.css @@ -10530,6 +10530,63 @@ body.helper-mode-active #dashboard-activity-feed:hover { gap: 4px; } +/* #876: quarantine "alternatives for one song" group — a collapsible parent + row wrapping the standard entry rows for each failed source attempt. */ +.lh-quarantine-group { + border-left: 3px solid rgba(var(--accent-rgb), 0.35); + border-radius: 8px; + margin-bottom: 4px; + background: rgba(var(--accent-rgb), 0.03); +} + +.lh-quarantine-group-header { + display: flex; + align-items: center; + gap: 12px; + padding: 10px 12px; + cursor: pointer; + border-radius: 8px; + transition: background 0.15s ease; +} + +.lh-quarantine-group-header:hover { + background: rgba(var(--accent-rgb), 0.06); +} + +.lh-quarantine-group-text { + flex: 1; + min-width: 0; +} + +.lh-quarantine-group-count { + flex-shrink: 0; + font-size: 12px; + color: rgba(var(--accent-rgb), 0.95); + border: 1px solid rgba(var(--accent-rgb), 0.4); + border-radius: 999px; + padding: 2px 10px; + white-space: nowrap; +} + +.lh-quarantine-group-header .lh-expand-btn { + transition: transform 0.18s ease; +} + +.lh-quarantine-group:not(.lh-qgroup-collapsed) .lh-quarantine-group-header .lh-expand-btn { + transform: rotate(180deg); +} + +.lh-quarantine-group-members { + padding: 0 8px 8px 16px; + display: flex; + flex-direction: column; + gap: 4px; +} + +.lh-quarantine-group.lh-qgroup-collapsed .lh-quarantine-group-members { + display: none; +} + .library-history-entry-row1 { display: flex; align-items: flex-start; diff --git a/webui/static/wishlist-tools.js b/webui/static/wishlist-tools.js index 8f95c421..c0ad23c1 100644 --- a/webui/static/wishlist-tools.js +++ b/webui/static/wishlist-tools.js @@ -3300,13 +3300,66 @@ async function loadQuarantineList() { list.innerHTML = '
🛡️

No quarantined files. Nice and clean.
'; return; } - list.innerHTML = entries.map(renderQuarantineEntry).join(''); + list.innerHTML = _groupQuarantineEntries(entries).map(renderQuarantineGroupOrEntry).join(''); } catch (err) { console.error('Error loading quarantine entries:', err); list.innerHTML = '
Error loading quarantine
'; } } +// #876: bucket entries that are alternatives for the SAME intended target +// (same backend group_key — derived from expected_artist/expected_track, the +// track SoulSync was trying to fetch, not the bad file's own tags). Entries +// with a null group_key (legacy/orphan, ungroupable) each stand alone. +// Preserves the newest-first order the backend already sorted by. +function _groupQuarantineEntries(entries) { + const groups = []; + const byKey = new Map(); + for (const entry of entries) { + const key = entry.group_key; + if (!key) { groups.push({ key: null, members: [entry] }); continue; } + let g = byKey.get(key); + if (!g) { g = { key, members: [] }; byKey.set(key, g); groups.push(g); } + g.members.push(entry); + } + return groups; +} + +function renderQuarantineGroupOrEntry(group) { + if (group.members.length === 1) return renderQuarantineEntry(group.members[0]); + return renderQuarantineGroup(group); +} + +// A collapsible parent row for multiple alternatives of one song. Header shows +// the shared track/artist + an alternatives count; members reuse the standard +// entry markup so per-row Approve/Recover/Delete keep working unchanged. +function renderQuarantineGroup(group) { + const first = group.members[0] || {}; + const title = first.expected_track || first.original_filename || 'Unknown'; + const artist = first.expected_artist || ''; + const n = group.members.length; + const sub = artist ? escapeHtml(artist) : ''; + // Reuse the album art the sidecar context carried, when any member has it. + const thumb = (group.members.find(m => m.thumb_url) || {}).thumb_url || ''; + const thumbHtml = thumb + ? `` + : '
🛡️
'; + return `
+
+ ${thumbHtml} +
+
${escapeHtml(title)}
+
${sub}
+
+ ${n} alternatives + ▾ +
+
+ ${group.members.map(renderQuarantineEntry).join('')} +
+
`; +} + function renderQuarantineEntry(entry) { const triggerLabels = { integrity: 'Duration / Integrity', acoustid: 'AcoustID Mismatch', bit_depth: 'Bit Depth Filter', unknown: 'Unknown' }; const triggerColors = { integrity: '#facc15', acoustid: '#ef5350', bit_depth: '#fb923c', unknown: '#888' }; @@ -3388,12 +3441,20 @@ async function approveQuarantineEntry(entryId) { }); if (!ok) return; try { - const r = await fetch(`/api/quarantine/${encodeURIComponent(entryId)}/approve`, { method: 'POST' }); + const r = await fetch(`/api/quarantine/${encodeURIComponent(entryId)}/approve`, { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + // #876: clear the other quarantined alternatives for this same song + // once one is accepted — they're redundant failed attempts now. + body: JSON.stringify({ remove_siblings: true }), + }); const data = await r.json(); if (!data.success) { showToast(`Approve failed: ${data.error}`, 'error'); } else { - showToast(`Approved — skipped ${data.trigger_bypassed} check, re-running pipeline.`, 'success'); + const removed = (data.removed_siblings || []).length; + const extra = removed ? ` — removed ${removed} other option${removed === 1 ? '' : 's'}.` : ''; + showToast(`Approved — skipped ${data.trigger_bypassed} check, re-running pipeline.${extra}`, 'success'); } } catch (err) { showToast(`Approve failed: ${err.message}`, 'error');