fix(quarantine): drop already-quarantined sources from candidate picker (#652)

When a file failed AcoustID verification and got quarantined, the next
auto-wishlist cycle would search for the same track, the deterministic
quality picker would re-select the same (uploader, filename) source,
re-download it, and re-quarantine it. Users woke up to hundreds of
duplicate .quarantined entries from a single bad upload — same source
URL repeatedly, byte-for-byte identical files.

Root cause: `SoulseekClient.filter_results_by_quality_preference` ranks
candidates by quality + bitrate density only. Quarantine history wasn't
consulted, so a high-bitrate FLAC upload with a wrong-track AcoustID
fingerprint kept winning the picker against every other candidate.

Fix shape:

- New helper `core/imports/quarantine.py::get_quarantined_source_keys`
  reads every quarantine sidecar's `context.original_search_result`
  and returns the set of `(username, filename)` tuples for O(1)
  membership checks. Sidecars missing the context field (legacy thin
  sidecars written pre-Feb 2026, or orphaned files) and corrupt JSON
  are skipped silently — defensive against transient FS / encoding
  issues.

- `SoulseekClient._drop_quarantined_sources` runs the membership
  filter against incoming TrackResults, drops matches, logs a single
  INFO line with the skip count. Called first inside
  `filter_results_by_quality_preference` so all four callers
  (search-and-download, master worker, validation, orchestrator)
  benefit transparently.

- Approving or deleting a quarantine entry removes its sidecar, so
  the dedup key disappears from the set on the next search — gives
  the user a way to opt back in to a previously-quarantined source
  without restarting the app.

7 helper tests cover: missing dir, empty dir, well-formed sidecars
collected as tuples, legacy sidecars skipped, empty source fields
skipped (so empty-string keys can't accidentally drop unrelated
results), corrupt JSON tolerated, duplicate quarantines collapse.

5 integration tests pin: clean candidates pass, known-bad candidates
drop, missing quarantine dir returns input unchanged, filesystem
errors swallowed (defensive), full `filter_results_by_quality_preference`
runs the dedup BEFORE the quality picker — so a high-quality
quarantined source can't win on bitrate.

692 existing download + import tests still green. Cosmetic surface
of the fix is invisible — same UX as today when no quarantine entries
exist; loop only kicks in once a sidecar has been written.

Out of scope: bulk-select / multi-delete UI for the quarantine tab —
S-Bryce mentioned this as a separate pain point in the issue, but
it's its own UX work, not a one-commit drive-by.
This commit is contained in:
Broque Thomas 2026-05-19 21:19:50 -07:00
parent 442cd5438e
commit 79ad4d885d
5 changed files with 399 additions and 0 deletions

View file

@ -93,6 +93,65 @@ def entry_id_from_quarantined_filename(quarantined_filename: str) -> str:
return _entry_id_from_filename(os.path.basename(quarantined_filename))
def get_quarantined_source_keys(quarantine_dir: str) -> set:
"""Return a set of ``(username, filename)`` tuples for every Soulseek
source that has been quarantined.
Used to gate the Soulseek candidate filter against re-picking the
exact same upload that already failed post-download verification.
Issue #652 — without this gate, the auto-wishlist processor's
candidate ranking is deterministic, so the same `(uploader, file)`
keeps winning the quality picker, downloading, quarantining, and
re-queueing in an infinite loop. Users wake up to hundreds of
duplicate `.quarantined` files for the same source URL.
The keys come from the sidecar JSON's
``context.original_search_result`` field which `move_to_quarantine`
persists from the originating SearchResult. Sidecars missing either
field (legacy thin sidecars written pre-Feb 2026, or orphaned
files) are skipped silently they can't gate anything anyway.
Returns an empty set when the directory doesn't exist or has no
parseable sidecars. Never raises; filesystem / JSON errors are
swallowed at debug level so a corrupt sidecar can't block the
download pipeline.
"""
keys: set = set()
if not quarantine_dir or not os.path.isdir(quarantine_dir):
return keys
try:
names = os.listdir(quarantine_dir)
except OSError as exc:
logger.debug("get_quarantined_source_keys: listdir failed: %s", exc)
return keys
for name in names:
if not name.endswith('.json'):
continue
sidecar_path = os.path.join(quarantine_dir, name)
try:
with open(sidecar_path, encoding='utf-8') as f:
sidecar = json.load(f)
except Exception as exc:
logger.debug("get_quarantined_source_keys: sidecar read failed for %s: %s", name, exc)
continue
if not isinstance(sidecar, dict):
continue
ctx = sidecar.get('context')
if not isinstance(ctx, dict):
continue
osr = ctx.get('original_search_result')
if not isinstance(osr, dict):
continue
username = osr.get('username') or ''
filename = osr.get('filename') or ''
if username and filename:
keys.add((str(username), str(filename)))
return keys
def list_quarantine_entries(quarantine_dir: str) -> List[Dict[str, Any]]:
"""Enumerate quarantined files paired with their sidecars.

View file

@ -1497,14 +1497,71 @@ class SoulseekClient(DownloadSourcePlugin):
'other': (0, 500),
}
def _drop_quarantined_sources(self, results: List[TrackResult]) -> List[TrackResult]:
"""Filter out candidates whose `(username, filename)` is on the
quarantine record. Issue #652.
Reads quarantine sidecars fresh each call so newly-quarantined
sources are honored immediately on the next search no client
state to invalidate. Filesystem cost is bounded (one listdir +
N small JSON reads) and dwarfed by the Soulseek search itself.
Returns the input list unchanged when the quarantine directory
is absent, empty, or unreadable i.e. defaults to today's
behaviour if anything goes wrong on the dedup path.
"""
try:
from core.imports.quarantine import get_quarantined_source_keys
download_path = config_manager.get('soulseek.download_path', './downloads')
quarantine_dir = os.path.join(download_path, 'ss_quarantine')
blocked = get_quarantined_source_keys(quarantine_dir)
except Exception as exc:
logger.debug("quarantine dedup: failed to load source keys, skipping filter: %s", exc)
return results
if not blocked:
return results
kept: List[TrackResult] = []
skipped = 0
for candidate in results:
key = (candidate.username or '', candidate.filename or '')
if key in blocked:
skipped += 1
continue
kept.append(candidate)
if skipped:
logger.info(
f"Quarantine dedup: dropped {skipped} candidate(s) matching previously-quarantined sources; "
f"{len(kept)} remain"
)
return kept
def filter_results_by_quality_preference(self, results: List[TrackResult]) -> List[TrackResult]:
"""
Filter candidates based on user's quality profile with bitrate density constraints.
Uses priority waterfall logic: tries highest priority quality first, falls back to lower priorities.
Returns candidates matching quality profile constraints, sorted by confidence and effective bitrate.
Issue #652: also drops candidates whose `(username, filename)`
matches a previously-quarantined download. Without this pre-filter
the auto-wishlist processor's ranking is deterministic — the same
`(uploader, file)` keeps winning the quality picker, downloading,
failing AcoustID, quarantining, and re-queueing in an infinite
loop. Users wake up to hundreds of duplicate `.quarantined` files
for the same source URL.
"""
from database.music_database import MusicDatabase
if not results:
return []
# Drop sources already quarantined — bypass the quality picker
# entirely so the same bad upload doesn't get re-selected on the
# next wishlist cycle. Filesystem read is bounded (~few hundred
# sidecars in practical use × <1ms each).
results = self._drop_quarantined_sources(results)
if not results:
return []

View file

@ -511,3 +511,172 @@ def test_non_connection_exception_still_logs_error(configured_client, caplog):
error_records = [r for r in caplog.records if r.levelno == logging.ERROR
and 'Error making API request' in r.message]
assert len(error_records) == 1, "Non-connection exceptions must still log ERROR"
# ---------------------------------------------------------------------------
# Issue #652 — quarantined-source dedup in the candidate filter
# ---------------------------------------------------------------------------
def _mk_track_result(username='peer', filename='song.flac', quality='flac',
bitrate=1411, size=10_000_000, duration=180_000):
"""Build a minimal TrackResult for the candidate filter tests."""
from core.download_plugins.types import TrackResult
return TrackResult(
username=username,
filename=filename,
size=size,
bitrate=bitrate,
duration=duration,
quality=quality,
free_upload_slots=1,
upload_speed=1_000_000,
queue_length=0,
)
def test_drop_quarantined_sources_keeps_clean_candidates(configured_client, tmp_path, monkeypatch):
"""When no candidate matches a quarantined `(username, filename)`,
every result passes through. Filter is a no-op for clean searches."""
quarantine_dir = tmp_path / 'ss_quarantine'
quarantine_dir.mkdir()
# Patch config_manager to point at our temp download path.
import core.soulseek_client as sc
monkeypatch.setattr(sc.config_manager, 'get',
lambda key, default=None: str(tmp_path) if key == 'soulseek.download_path' else default)
results = [
_mk_track_result(username='goodpeer1', filename='a.flac'),
_mk_track_result(username='goodpeer2', filename='b.flac'),
]
kept = configured_client._drop_quarantined_sources(results)
assert len(kept) == 2
assert {r.username for r in kept} == {'goodpeer1', 'goodpeer2'}
def test_drop_quarantined_sources_drops_known_bad(configured_client, tmp_path, monkeypatch):
"""Issue #652 core contract: a candidate whose `(username, filename)`
matches a quarantined entry is dropped before the quality picker
ranks it. Stops the loop where the same source kept winning the
quality picker and re-downloading itself."""
import json as _json
quarantine_dir = tmp_path / 'ss_quarantine'
quarantine_dir.mkdir()
# Write a sidecar matching the bad source.
sidecar = {
"original_filename": "bad.flac",
"quarantine_reason": "AcoustID mismatch",
"context": {
"original_search_result": {
"username": "badpeer", "filename": "albums/bad.flac",
},
},
}
(quarantine_dir / "20260518_120000.json").write_text(_json.dumps(sidecar))
import core.soulseek_client as sc
monkeypatch.setattr(sc.config_manager, 'get',
lambda key, default=None: str(tmp_path) if key == 'soulseek.download_path' else default)
results = [
_mk_track_result(username='badpeer', filename='albums/bad.flac'),
_mk_track_result(username='goodpeer', filename='albums/good.flac'),
]
kept = configured_client._drop_quarantined_sources(results)
assert len(kept) == 1
assert kept[0].username == 'goodpeer'
def test_drop_quarantined_sources_returns_input_when_quarantine_missing(configured_client, tmp_path, monkeypatch):
"""No quarantine directory yet (fresh install / never used) —
helper returns an empty set; filter returns the input unchanged.
Defaults to today's behaviour for users with no quarantine history."""
import core.soulseek_client as sc
monkeypatch.setattr(sc.config_manager, 'get',
lambda key, default=None: str(tmp_path) if key == 'soulseek.download_path' else default)
results = [_mk_track_result(username='peer', filename='song.flac')]
kept = configured_client._drop_quarantined_sources(results)
assert kept == results
def test_drop_quarantined_sources_swallows_filesystem_errors(configured_client, monkeypatch):
"""If something goes wrong loading the quarantine keys (permissions,
OS quirk, etc.), the filter must NOT break the download pipeline.
Returns input unchanged so legitimate downloads keep working
same defensive contract as the existing 401/connection handlers."""
import core.soulseek_client as sc
def _broken_get(key, default=None):
raise RuntimeError("config explosion")
monkeypatch.setattr(sc.config_manager, 'get', _broken_get)
results = [_mk_track_result(username='peer', filename='song.flac')]
kept = configured_client._drop_quarantined_sources(results)
assert kept == results
def test_filter_results_by_quality_runs_quarantine_dedup_first(configured_client, tmp_path, monkeypatch):
"""Integration pin: `filter_results_by_quality_preference` calls
the quarantine dedup BEFORE the quality picker. If a candidate is
on the quarantine record, it can't win the picker by virtue of
superior bitrate that's how the #652 loop manifested."""
import json as _json
quarantine_dir = tmp_path / 'ss_quarantine'
quarantine_dir.mkdir()
sidecar = {
"context": {
"original_search_result": {
"username": "badpeer", "filename": "high_bitrate_bad.flac",
},
},
}
(quarantine_dir / "20260518_120000.json").write_text(_json.dumps(sidecar))
import core.soulseek_client as sc
monkeypatch.setattr(sc.config_manager, 'get',
lambda key, default=None: str(tmp_path) if key == 'soulseek.download_path' else default)
# Mock the DB call inside filter_results_by_quality_preference so the
# test doesn't need a real DB. Quality profile permits FLAC.
class _FakeDB:
def get_quality_profile(self):
return {
'preset': 'flac',
'qualities': {
'flac': {'enabled': True, 'min_kbps': 800, 'max_kbps': 99999},
'mp3_320': {'enabled': False, 'min_kbps': 0, 'max_kbps': 0},
'mp3_256': {'enabled': False, 'min_kbps': 0, 'max_kbps': 0},
'mp3_192': {'enabled': False, 'min_kbps': 0, 'max_kbps': 0},
},
'priority': ['flac'],
}
import database.music_database as md
monkeypatch.setattr(md, 'MusicDatabase', lambda: _FakeDB())
results = [
# The "bad" source has the BEST quality on paper — pre-fix would win the picker.
_mk_track_result(username='badpeer', filename='high_bitrate_bad.flac',
quality='flac', bitrate=1411, size=20_000_000, duration=180_000),
_mk_track_result(username='goodpeer', filename='good.flac',
quality='flac', bitrate=1411, size=20_000_000, duration=180_000),
]
kept = configured_client.filter_results_by_quality_preference(results)
usernames = {r.username for r in kept}
assert 'badpeer' not in usernames, "Quarantined source must be filtered before the quality picker"
assert 'goodpeer' in usernames

View file

@ -5,6 +5,7 @@ from core.imports.quarantine import (
approve_quarantine_entry,
delete_quarantine_entry,
entry_id_from_quarantined_filename,
get_quarantined_source_keys,
list_quarantine_entries,
recover_to_staging,
serialize_quarantine_context,
@ -285,3 +286,115 @@ def test_recover_removes_sidecar_after_move(tmp_path):
recover_to_staging(str(quarantine), str(staging), "20260514_120000_song")
assert not sidecar.exists()
# ──────────────────────────────────────────────────────────────────────
# get_quarantined_source_keys — issue #652 dedup primitive
# ──────────────────────────────────────────────────────────────────────
def _write_quarantine_sidecar_with_source(quarantine_dir, entry_id, *,
username=None, filename=None):
"""Helper that writes a sidecar matching the shape `move_to_quarantine`
produces `context.original_search_result.{username, filename}` is
the path `get_quarantined_source_keys` pulls from."""
sidecar = {
"original_filename": "song.flac",
"quarantine_reason": "boom",
"timestamp": "2026-05-14T12:00:00",
"trigger": "acoustid",
}
if username is not None or filename is not None:
sidecar["context"] = {
"original_search_result": {
"username": username or "",
"filename": filename or "",
}
}
path = quarantine_dir / f"{entry_id}.json"
path.write_text(json.dumps(sidecar))
return path
def test_source_keys_empty_for_missing_dir(tmp_path):
"""Defensive: caller may pass a path that doesn't exist (config not
initialised, quarantine never used). Don't crash, just return an
empty set Soulseek filter then keeps every candidate."""
assert get_quarantined_source_keys(str(tmp_path / "nope")) == set()
def test_source_keys_empty_for_empty_dir(tmp_path):
"""Empty quarantine dir → empty set."""
assert get_quarantined_source_keys(str(tmp_path)) == set()
def test_source_keys_collects_username_filename_tuples(tmp_path):
"""Sidecars with `context.original_search_result.username` and
`.filename` round-trip into `(username, filename)` tuples that's
the exact shape the Soulseek candidate filter looks up against."""
_write_quarantine_sidecar_with_source(
tmp_path, "20260514_120000_a",
username="badpeer", filename="path/to/bad.flac",
)
_write_quarantine_sidecar_with_source(
tmp_path, "20260514_120100_b",
username="otherpeer", filename="other.mp3",
)
keys = get_quarantined_source_keys(str(tmp_path))
assert ("badpeer", "path/to/bad.flac") in keys
assert ("otherpeer", "other.mp3") in keys
assert len(keys) == 2
def test_source_keys_skip_legacy_sidecars_without_context(tmp_path):
"""Sidecars written pre-Feb 2026 don't have the `context` field —
can't gate against them since the originating source is unknown.
Must skip silently rather than crashing the dedup path."""
_write_quarantine_sidecar_with_source(tmp_path, "legacy_id") # no username/filename
assert get_quarantined_source_keys(str(tmp_path)) == set()
def test_source_keys_skip_sidecars_with_empty_source_fields(tmp_path):
"""Defensive: a sidecar with an empty string for username OR filename
can't gate anything meaningfully — dropping every result whose
username equals '' would catch unrelated downloads. Skip those
entries entirely."""
_write_quarantine_sidecar_with_source(tmp_path, "empty_user", username="", filename="x.flac")
_write_quarantine_sidecar_with_source(tmp_path, "empty_file", username="u", filename="")
assert get_quarantined_source_keys(str(tmp_path)) == set()
def test_source_keys_skip_corrupt_sidecars(tmp_path):
"""A corrupt JSON sidecar (truncated write, encoding glitch) must
not propagate up and break the dedup path. Filesystem read errors
are swallowed at debug level."""
bad = tmp_path / "corrupt.json"
bad.write_text("{not valid json")
_write_quarantine_sidecar_with_source(
tmp_path, "good", username="good_peer", filename="good.flac",
)
keys = get_quarantined_source_keys(str(tmp_path))
assert keys == {("good_peer", "good.flac")}
def test_source_keys_dedup_repeated_sources(tmp_path):
"""If the SAME `(username, filename)` was quarantined twice (which
is exactly the #652 bug — but until now wasn't being prevented),
the set collapses to one entry. The Soulseek filter still acts as
a single-membership check, so a single set entry is enough."""
_write_quarantine_sidecar_with_source(
tmp_path, "first", username="peer", filename="dupe.flac",
)
_write_quarantine_sidecar_with_source(
tmp_path, "second", username="peer", filename="dupe.flac",
)
keys = get_quarantined_source_keys(str(tmp_path))
assert keys == {("peer", "dupe.flac")}

View file

@ -3426,6 +3426,7 @@ const WHATS_NEW = {
{ title: 'Fix: Docker basic-search streaming silently failed under rootless Docker', desc: 'the streaming "Play" flow on the basic search page tried to create `/app/Stream` lazily at runtime, which fails silently when the container runs under rootless Docker / Podman (in-container root can\'t write to `/app`). pre-baked the directory at image build time, matching the same pattern that fixed `/app/Staging` earlier in the cycle. non-persistent — no volume needed.' },
{ title: 'Fix: slskd-unreachable log spam during non-Soulseek downloads', desc: 'when slskd was configured but not actually running (or unreachable on its configured port), the `/api/downloads/status` polling loop fanned out to every download plugin including Soulseek, producing one `ERROR - Cannot connect to host ... [Name or service not known]` log line per poll for the entire duration of any download — visible spam even when the user wasn\'t using Soulseek at all. Connection failures now emit one WARNING with actionable context (start slskd, or clear the slskd_url if you don\'t use Soulseek) and demote subsequent failures to debug. The flag resets on the next successful slskd response so a later outage warns again.' },
{ title: 'Fix: MusicBrainz "Other" release-groups now visible in discography', desc: 'MB tags music videos, one-off web releases, and broadcast singles with `primary-type=Other` — common pattern for Vocaloid producers, JP indie artists, and some Western indie acts. The release-group browse filter only requested `album|ep|single`, dropping every Other-typed group at the API layer. Combined with the inline type mapper defaulting unknown primary types to "album", this hid legitimate tracks from the artist detail page and left downloaded tracks orphaned (visible in track counts but not bound to any visible album / single card). Centralised the type-mapper into one shared helper (`core/metadata/release_type.py`) used by every provider\'s raw→Album projection, added `Other` and `Broadcast` handling that routes those release-groups into the Singles section, and added `other` to the MB API filter. For inabakumori, this surfaces 5 previously-invisible releases. 23 unit tests pin the mapper contract; existing 65 search-adapter tests still green.' },
{ title: 'Fix: quarantined files no longer re-downloaded on auto-wishlist cycles', desc: 'when a file failed AcoustID verification and got quarantined, the next auto-wishlist run would search for the same track again, and the candidate picker\'s deterministic quality ranking kept selecting the same `(uploader, filename)` source — re-downloading and re-quarantining the exact same file every cycle. Users woke up to hundreds of duplicate `.quarantined` entries from a single bad upload. The Soulseek candidate filter now reads quarantine sidecars and drops any candidate whose `(username, filename)` matches a previously-quarantined source before the quality picker ranks it. Filesystem read on every search (sub-ms in practice). Approving or deleting a quarantine entry removes its source from the dedup set automatically — the user can give the source a second chance by explicitly approving / deleting the quarantine record.' },
],
'2.5.5': [
{ date: 'May 17, 2026 — 2.5.5 release' },