#876: group quarantine alternatives by target track-id + auto-clear siblings on approve

Multiple failed source attempts at one song each land in quarantine as
separate entries. Group them by the *intended* target (sidecar context
track_info isrc -> id -> uri, falling back to normalized artist|title for
legacy thin sidecars) — an exact relationship across siblings, since the
bad files' own tags differ but the target track is constant.

- core: quarantine_group_key() + find_quarantine_siblings() seams; list
  entries now carry group_key.
- approve endpoint: remove_siblings flag auto-deletes the other attempts
  once one is accepted (captured BEFORE approve restores the file out of
  quarantine, or the id lookup would resolve nothing). Scoped to the
  quarantine manager; download-modal chooser + version-mismatch fallback
  pass no flag and are unaffected.
- UI: multi-member groups render as a collapsible parent row (album art +
  'N alternatives'); singletons unchanged. Toast reports removed count.
- 11 tests incl. ordering regression for capture-before-approve.
This commit is contained in:
BoulderBadgeDad 2026-06-15 22:12:06 -07:00
parent 93af95d865
commit 46be97b195
5 changed files with 337 additions and 9 deletions

View file

@ -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),

View file

@ -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

View file

@ -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

View file

@ -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;

View file

@ -3300,13 +3300,66 @@ async function loadQuarantineList() {
list.innerHTML = '<div class="library-history-empty">🛡️<br><br>No quarantined files. Nice and clean.</div>';
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 = '<div class="library-history-empty">Error loading quarantine</div>';
}
}
// #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
? `<img class="library-history-thumb" src="${escapeHtml(thumb)}" alt="" onerror="this.style.display='none'">`
: '<div class="library-history-thumb-placeholder">🛡️</div>';
return `<div class="lh-quarantine-group lh-qgroup-collapsed">
<div class="lh-quarantine-group-header" onclick="this.parentElement.classList.toggle('lh-qgroup-collapsed')">
${thumbHtml}
<div class="lh-quarantine-group-text">
<div class="library-history-entry-title">${escapeHtml(title)}</div>
<div class="library-history-entry-meta">${sub}</div>
</div>
<span class="lh-quarantine-group-count">${n} alternatives</span>
<span class="lh-expand-btn">&#x25BE;</span>
</div>
<div class="lh-quarantine-group-members">
${group.members.map(renderQuarantineEntry).join('')}
</div>
</div>`;
}
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');