Commit graph

4 commits

Author SHA1 Message Date
Broque Thomas
79ad4d885d 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.
2026-05-19 21:19:50 -07:00
Broque Thomas
f25433ea57 Harden quarantine approval flows 2026-05-18 08:37:11 -07:00
Broque Thomas
b42cafa150 AcoustID + quarantine modal: three bug fixes (closes #607, closes #608)
Issue #607 (AfonsoG6) -- two AcoustID problems:

1. Live recordings false-quarantining as "Version mismatch: expected
   '... (Live at Venue)' (live) but file is '...' (original)" because
   MusicBrainz often stores the recording entity with a bare title --
   the venue / live annotation lives on the release entity, not the
   recording. The audio fingerprint correctly identifies the live
   recording, but the title-text comparison flagged it as wrong.

   New pure helper `core/matching/version_mismatch.py:is_acceptable_version_mismatch`
   accepts the mismatch only when:
     - One-sided AND involves 'live': exactly one side is 'live' and
       the other is bare 'original'. Two-sided mismatches stay strict.
     - Fingerprint score >= 0.85 (stricter than the existing 0.80
       minimum -- escape valve only fires when AcoustID is more
       confident than its own threshold).
     - Bare title similarity >= 0.70.
     - Artist similarity >= 0.60.

   Other version markers (instrumental, remix, acoustic, demo, etc)
   stay strict -- those have distinct fingerprints AND MB always
   annotates them in the recording title. The existing
   test_acoustid_version_mismatch.py suite passes unchanged.

2. Audio-mismatch failure message reported "identified as '' by ''
   (artist=100%)" when AcoustID returned multiple recordings -- prior
   code mixed `recordings[0]`'s strings (which can be empty) with
   `best_rec`'s scores. Now uses `matched_title` / `matched_artist`
   consistently in both the high-confidence-skip path and the final
   fail message.

Issue #608 (AfonsoG6) -- quarantine modal:

3. Approve / Delete buttons silently no-op'd when the filename
   contained an apostrophe -- the unescaped quote broke the inline JS
   in the onclick handler. Now wraps the id via
   `escapeHtml(JSON.stringify(id))`, which round-trips quotes /
   backslashes / unicode / newlines safely through the HTML attribute
   to JS string boundary.

4. Bonus UX: quarantine entry expanded view now shows source uploader
   (username) and original soulseek filename when the sidecar carries
   that context -- helps trace which uploader the bad file came from.
   Backend exposes `source_username` + `source_filename` fields from
   `sidecar.context.original_search_result`. Degrades to '' on legacy
   thin sidecars.

Tests:
- 23 new boundary tests in tests/matching/test_version_mismatch.py
  pin every shape: equal versions trivial, one-sided live both
  directions, threshold floors (each just below default -> reject),
  two-sided strict, non-live one-sided strict (covers exact
  test_instrumental_returned_for_vocal_request_fails scenario),
  custom-threshold overrides.
- 4 existing test_acoustid_version_mismatch.py tests pass unchanged.
- 507 AcoustID / matching / imports tests pass.
2026-05-15 08:55:06 -07:00
Broque Thomas
f4cff78f13 Quarantine management — list, approve, delete, recover
Closes #584. Quarantined files used to sit in ss_quarantine/ with a
thin sidecar — no UI, no recovery, no way to see what got dropped.
This adds the management surface the user needs without going to the
filesystem.

UI: new "Quarantine" button on the downloads page header opens a
modal with every quarantined file (filename, expected track/artist,
reason, when, size). Three actions per row:

- Approve (one-click): restores the file, re-runs the post-process
  pipeline with ONLY the failing check skipped, lands in the library
  with full tags + lyrics + scan
- Recover (legacy fallback): moves to Staging for thin-sidecar
  entries that lack the embedded context Approve needs
- Delete: permanent removal of file + sidecar

Per-check bypass: context['_skip_quarantine_check'] = 'integrity' /
'acoustid' / 'bit_depth'. Skips ONLY the named check — other quality
gates stay live. No blanket bypass-all flag.

Sidecar expansion: move_to_quarantine now persists the full
json-serializable context via serialize_quarantine_context (drops
non-JSON-safe values, walks nested dicts/lists/sets, str-coerces
unknown objects) plus the trigger name. Existing thin sidecars are
detected and routed to Recover instead of Approve.

Pure helpers in core/imports/quarantine.py: list_quarantine_entries
/ delete_quarantine_entry / approve_quarantine_entry /
recover_to_staging / serialize_quarantine_context. 27 tests pin
every shape: orphan files / orphan sidecars / corrupt sidecars /
collision-safe filename restoration / full-context vs thin-sidecar
dispatch / json round-trip safety.

Four new endpoints in web_server.py — thin glue around the helpers:
GET /api/quarantine/list, DELETE /api/quarantine/<id>,
POST /api/quarantine/<id>/approve, POST /api/quarantine/<id>/recover.

Download modal status differentiates "🛡️ Quarantined" from
" Failed" so recoverable files are visible at a glance — checked
against the error_message text, no schema change needed.

Pipeline changes are three minimal per-check conditionals at the
existing quarantine sites in core/imports/pipeline.py. Each
move_to_quarantine call now passes its trigger name so the sidecar
records which check fired.

Full suite: 2992 passed.
2026-05-14 08:06:19 -07:00