diff --git a/core/downloads/history_match.py b/core/downloads/history_match.py new file mode 100644 index 00000000..dadea5e1 --- /dev/null +++ b/core/downloads/history_match.py @@ -0,0 +1,62 @@ +"""Match a file back to its download-history row when its path has drifted (#934). + +``library_history.file_path`` is frozen at import time, but the file moves afterward +(media-server import, library reorganize) and ``tracks.file_path`` — what the AcoustID +scanner reads — no longer equals it. Matching on the exact path alone then fails twice: +the verification status never reaches the history row (verified tracks read "unverified"), +and a fresh ``acoustid_scan`` row gets inserted every run (thousands of duplicates). + +This module picks the canonical history row by exact path first, then by FILENAME guarded +by a title check — so a shared filename ("01 - Intro.flac") can never heal the wrong song. +Pure (no DB) so the matching rules are unit-testable; the caller does the SQL. +""" + +from __future__ import annotations + +import os +from typing import Iterable, Optional, Sequence, Tuple + + +def _norm_title(value) -> str: + """Alphanumeric-only lowercase form, so "Song (Remaster)" vs "song remaster" + style drift between the download tag and the media-server tag still agrees.""" + return ''.join(ch for ch in str(value or '').lower() if ch.isalnum()) + + +def like_filename_filter(basename: str) -> str: + """A ``LIKE ... ESCAPE '\\'`` pattern that coarsely matches rows whose path ends + in ``basename``. Escapes the LIKE metacharacters (``%`` ``_`` ``\\``) — filenames + routinely contain underscores. Callers MUST still confirm with an exact basename + compare (``pick_history_row`` does), since ``'%name'`` also matches ``'xname'``.""" + esc = basename.replace('\\', '\\\\').replace('%', '\\%').replace('_', '\\_') + return '%' + esc + + +def pick_history_row(candidates: Sequence[Tuple], *, current_paths: Iterable[str], + basename: str, title: str) -> Optional[int]: + """Return the id of the history row to update for this file, or None. + + ``candidates``: ``(id, file_path, title, download_source)`` rows the DB pre-filtered + (exact path or filename LIKE). A row matches when its path equals the current path OR + its filename matches AND its title agrees — the title guard prevents a shared filename + ("01 - Intro.flac") from healing a different song's row. Among matches a REAL download + row is preferred over a synthetic ``acoustid_scan`` row, so the scanner heals the + genuine record and the caller can delete the synthetic duplicate. None when nothing + matches safely (caller then inserts a fresh row — the "file SoulSync never downloaded" + intent).""" + paths = {p for p in current_paths if p} + want = _norm_title(title) + matches: list = [] # (id, is_exact, is_real) + for cid, cpath, ctitle, csource in candidates: + is_real = csource != 'acoustid_scan' + if cpath and cpath in paths: + matches.append((cid, True, is_real)) + elif (basename and cpath and os.path.basename(cpath) == basename + and (not want or not _norm_title(ctitle) or _norm_title(ctitle) == want)): + matches.append((cid, False, is_real)) + if not matches: + return None + # Prefer a REAL download row over a synthetic acoustid_scan row; within that, prefer an + # exact-path match over a filename match. Stable, so ties keep DB order (first/oldest id). + matches.sort(key=lambda m: (m[2], m[1]), reverse=True) + return matches[0][0] diff --git a/core/repair_jobs/acoustid_scanner.py b/core/repair_jobs/acoustid_scanner.py index 16ceabda..b52ef6aa 100644 --- a/core/repair_jobs/acoustid_scanner.py +++ b/core/repair_jobs/acoustid_scanner.py @@ -389,14 +389,43 @@ class AcoustIDScannerJob(RepairJob): cur.execute( "UPDATE tracks SET verification_status = ? WHERE id = ?", (status, track_id)) - matched = 0 + exp = expected or {} + # Find the canonical history row for this file. The stored path is frozen at + # import time while the file has since moved (media-server import / reorganize), + # so an exact-path match alone misses it — then the status never lands and a + # duplicate row gets inserted every scan (#934). Match exact path first, then + # filename guarded by title, and HEAL the row's path so future scans match cleanly. + from core.downloads.history_match import pick_history_row, like_filename_filter + current = fpath or db_path + basename = os.path.basename(current) if current else '' + clauses, params = [], [] for p in {p for p in (fpath, db_path) if p}: + clauses.append("file_path = ?") + params.append(p) + if basename: + clauses.append("file_path LIKE ? ESCAPE '\\'") + params.append(like_filename_filter(basename)) + row_id = None + if clauses: cur.execute( - "UPDATE library_history SET verification_status = ? WHERE file_path = ?", - (status, p)) - matched += max(getattr(cur, 'rowcount', 0) or 0, 0) - if status == 'unverified' and matched == 0: - exp = expected or {} + "SELECT id, file_path, title, download_source FROM library_history WHERE " + + " OR ".join(clauses), + params) + row_id = pick_history_row( + cur.fetchall(), + current_paths=(fpath, db_path), + basename=basename, title=exp.get('title') or '') + if row_id is not None: + cur.execute( + "UPDATE library_history SET verification_status = ?, file_path = ? WHERE id = ?", + (status, current, row_id)) + # Drop synthetic scan-created duplicates for this exact file (the #934 + # leftovers). Exact path → collision-free; never touches a real download row. + cur.execute( + "DELETE FROM library_history WHERE id != ? AND download_source = 'acoustid_scan' " + "AND file_path = ?", + (row_id, current)) + elif status == 'unverified': cur.execute( """INSERT INTO library_history (event_type, title, artist_name, album_name, file_path, diff --git a/tests/test_acoustid_history_heal.py b/tests/test_acoustid_history_heal.py new file mode 100644 index 00000000..02108e10 --- /dev/null +++ b/tests/test_acoustid_history_heal.py @@ -0,0 +1,95 @@ +"""#934: the AcoustID scanner heals the download-history row when the file moved, +instead of leaving it stuck 'unverified' and inserting duplicate scan rows. + +Seeds the exact bug shape (a real download row at the OLD import path + a synthetic +'acoustid_scan' duplicate at the NEW library path) and drives the real _persist_status, +asserting it collapses to one correct, verified row. +""" + +from __future__ import annotations + +import types + +import pytest + +from core.repair_jobs.acoustid_scanner import AcoustIDScannerJob +from database.music_database import MusicDatabase + + +@pytest.fixture() +def db(tmp_path): + return MusicDatabase(str(tmp_path / 'm.db')) + + +def _scanner(): + # _persist_status uses only `context`, never instance state — bypass __init__. + return AcoustIDScannerJob.__new__(AcoustIDScannerJob) + + +def _rows(db): + with db._get_connection() as conn: + return [dict(r) for r in conn.execute( + "SELECT file_path, download_source, verification_status FROM library_history")] + + +OLD = '/downloads/transfer/Artist/01 - Song.flac' # frozen import path in history +NEW = '/music/Artist/Album/01 - Song.flac' # where the file lives now (tracks path) + + +def test_verify_heals_drifted_row_and_drops_synthetic_dup(db): + # the bug's leftover state: a stuck real row + a synthetic scan dup for the same song. + db.add_library_history_entry('download', 'Song', file_path=OLD, + download_source='soulseek', verification_status='unverified') + db.add_library_history_entry('download', 'Song', file_path=NEW, + download_source='acoustid_scan', verification_status='unverified') + + _scanner()._persist_status( + types.SimpleNamespace(db=db), track_id='t1', fpath=NEW, db_path=NEW, + status='verified', write_tag=False, expected={'title': 'Song'}) + + rows = _rows(db) + assert len(rows) == 1 # synthetic dup deleted, no new insert + assert rows[0]['download_source'] == 'soulseek' # the REAL row survived + assert rows[0]['verification_status'] == 'verified' + assert rows[0]['file_path'] == NEW # path healed to current location + + +def test_idempotent_no_growth_on_rescan(db): + db.add_library_history_entry('download', 'Song', file_path=OLD, + download_source='soulseek', verification_status='unverified') + ctx = types.SimpleNamespace(db=db) + for _ in range(3): + _scanner()._persist_status(ctx, track_id='t1', fpath=NEW, db_path=NEW, + status='verified', write_tag=False, expected={'title': 'Song'}) + rows = _rows(db) + assert len(rows) == 1 and rows[0]['verification_status'] == 'verified' + + +def test_unknown_file_inserts_one_row_then_dedups(db): + # a file SoulSync never downloaded → first scan inserts one review-queue row... + ctx = types.SimpleNamespace(db=db) + _scanner()._persist_status(ctx, track_id='t1', fpath=NEW, db_path=NEW, + status='unverified', write_tag=False, + expected={'title': 'Song', 'artist': 'Artist'}) + assert len(_rows(db)) == 1 + # ...and a rescan matches it (no duplicate). + _scanner()._persist_status(ctx, track_id='t1', fpath=NEW, db_path=NEW, + status='unverified', write_tag=False, + expected={'title': 'Song', 'artist': 'Artist'}) + rows = _rows(db) + assert len(rows) == 1 and rows[0]['download_source'] == 'acoustid_scan' + + +def test_does_not_heal_wrong_song_with_same_filename(db): + # different song, same filename, different title → must stay untouched (no false heal). + db.add_library_history_entry('download', 'A Different Song', file_path='/other/01 - Song.flac', + download_source='soulseek', verification_status='verified') + _scanner()._persist_status( + types.SimpleNamespace(db=db), track_id='t1', fpath=NEW, db_path=NEW, + status='unverified', write_tag=False, expected={'title': 'Song'}) + rows = _rows(db) + # the unrelated row is untouched, and the unknown file got its own new row. + paths = {r['file_path'] for r in rows} + assert '/other/01 - Song.flac' in paths and NEW in paths + other = next(r for r in rows if r['file_path'] == '/other/01 - Song.flac') + assert other['verification_status'] == 'verified' # not corrupted diff --git a/tests/test_acoustid_scanner.py b/tests/test_acoustid_scanner.py index 1c83cac6..7057c968 100644 --- a/tests/test_acoustid_scanner.py +++ b/tests/test_acoustid_scanner.py @@ -4,8 +4,9 @@ from core.repair_jobs.acoustid_scanner import AcoustIDScannerJob class _FakeCursor: - def __init__(self, rows): + def __init__(self, rows, lib_rows=None): self._rows = rows + self._lib_rows = lib_rows or [] self.executed = [] def execute(self, query, params=None): @@ -13,6 +14,10 @@ class _FakeCursor: return self def fetchall(self): + # The #934 history-match SELECT gets its own (id, file_path, title, source) + # rows; the tracks scan query gets the track rows. + if self.executed and 'FROM library_history' in self.executed[-1][0]: + return self._lib_rows return self._rows def fetchone(self): @@ -20,8 +25,8 @@ class _FakeCursor: class _FakeConnection: - def __init__(self, rows): - self._cursor = _FakeCursor(rows) + def __init__(self, rows, lib_rows=None): + self._cursor = _FakeCursor(rows, lib_rows) def cursor(self): return self._cursor @@ -107,12 +112,13 @@ def test_scan_handles_mixed_track_id_types(monkeypatch): # and finds the primary artist at 100%, suppressing the false flag. -def _make_finding_capturing_context(track_row, captured): +def _make_finding_capturing_context(track_row, captured, lib_rows=None): """Context that captures any create_finding calls into the `captured` list. Tests assert against this list to verify whether the scanner created a finding (false positive) or correctly - skipped (multi-value match resolved).""" - conn = _FakeConnection([track_row]) + skipped (multi-value match resolved). ``lib_rows`` seeds the + library_history match SELECT (#934).""" + conn = _FakeConnection([track_row], lib_rows) config_manager = SimpleNamespace( get=lambda key, default=None: default, set=lambda *args, **kwargs: None, @@ -887,9 +893,10 @@ def test_human_verified_files_are_never_scanned(monkeypatch): # --------------------------------------------------------------------------- -def _run_persistence_scan(monkeypatch, *, file_status, aid_artist, expected_artist): +def _run_persistence_scan(monkeypatch, *, file_status, aid_artist, expected_artist, lib_rows=None): """Drive one _scan_file call and return (status_updates, tag_writes) where - status_updates is the list of (query, params) UPDATEs the scanner ran.""" + status_updates is the list of (query, params) UPDATEs the scanner ran. + ``lib_rows`` seeds the library_history match SELECT (#934).""" import core.repair_jobs.acoustid_scanner as scanner_mod monkeypatch.setattr(scanner_mod, "_resolve_expected_artist_aliases", lambda name: [], raising=False) @@ -905,7 +912,7 @@ def _run_persistence_scan(monkeypatch, *, file_status, aid_artist, expected_arti context = _make_finding_capturing_context( track_row=("9", "Call Your Name", expected_artist, "/music/cyn.flac", 1, "Album", None, None), - captured=captured) + captured=captured, lib_rows=lib_rows) fake = SimpleNamespace(fingerprint_and_lookup=lambda f: { 'best_score': 0.97, 'recordings': [{'title': 'Call Your Name', 'artist': aid_artist}]}) @@ -920,29 +927,32 @@ def _run_persistence_scan(monkeypatch, *, file_status, aid_artist, expected_arti def test_scan_pass_backfills_verified_status(monkeypatch): - # Untagged file + clean fingerprint PASS → the scan backfills 'verified' - # into the tag, the tracks row AND library_history (review-queue feed). + # Clean fingerprint PASS → the scan backfills 'verified' into the tag, the tracks + # row AND the file's library_history row. The history row's path drifted since + # download (file moved), so the scan heals it by id (#934) rather than missing it. updates, tag_writes, captured = _run_persistence_scan( monkeypatch, file_status=None, - aid_artist='Sawano Hiroyuki', expected_artist='Sawano Hiroyuki') + aid_artist='Sawano Hiroyuki', expected_artist='Sawano Hiroyuki', + lib_rows=[(1, '/downloads/old/cyn.flac', 'Call Your Name', 'soulseek')]) assert captured == [] assert tag_writes == [('/music/cyn.flac', 'verified')] assert any('tracks' in q and p == ('verified', '9') for q, p in updates) - assert any('library_history' in q and p == ('verified', '/music/cyn.flac') + # healed by id, status set + path refreshed to the file's current location. + assert any('library_history' in q and p == ('verified', '/music/cyn.flac', 1) for q, p in updates) def test_scan_skip_marks_untagged_file_unverified(monkeypatch): # Title matches but the artist is ambiguous (cover/collab band?) → SKIP. - # An untagged file gets 'unverified' so it surfaces in the Downloads-page - # review queue instead of silently passing or being deleted. + # An untagged file SoulSync never downloaded (no history row) gets a fresh + # 'unverified' row INSERTed so it surfaces in the Downloads-page review queue. updates, tag_writes, captured = _run_persistence_scan( monkeypatch, file_status=None, - aid_artist='Mantilla', expected_artist='Metallica') + aid_artist='Mantilla', expected_artist='Metallica') # no lib_rows → no existing row assert captured == [] assert tag_writes == [('/music/cyn.flac', 'unverified')] assert any('tracks' in q and p == ('unverified', '9') for q, p in updates) - assert any('library_history' in q and p == ('unverified', '/music/cyn.flac') + assert any('INSERT INTO library_history' in q and p[-1] == 'unverified' for q, p in updates) diff --git a/tests/test_history_match.py b/tests/test_history_match.py new file mode 100644 index 00000000..bfb05e8d --- /dev/null +++ b/tests/test_history_match.py @@ -0,0 +1,63 @@ +"""Pure matcher that re-links a moved file to its download-history row (#934).""" + +from core.downloads.history_match import pick_history_row, like_filename_filter + + +# (id, file_path, title, download_source) +def _row(i, path, title='', source='soulseek'): + return (i, path, title, source) + + +def test_exact_current_path_wins(): + cands = [_row(1, '/old/song.flac', 'Song'), _row(2, '/lib/song.flac', 'Song')] + assert pick_history_row(cands, current_paths=('/lib/song.flac', None), + basename='song.flac', title='Song') == 2 + + +def test_falls_back_to_filename_when_path_drifted(): + # history has the OLD import path; scanner only knows the NEW library path. + cands = [_row(7, '/downloads/transfer/Artist/01 - Song.flac', 'Song')] + assert pick_history_row(cands, current_paths=('/music/Artist/Album/01 - Song.flac', None), + basename='01 - Song.flac', title='Song') == 7 + + +def test_title_guard_blocks_shared_filename_collision(): + # two different songs both named "01 - Intro.flac" — must NOT heal the wrong one. + cands = [_row(1, '/a/01 - Intro.flac', 'Album A Intro'), + _row(2, '/b/01 - Intro.flac', 'Album B Intro')] + got = pick_history_row(cands, current_paths=('/c/01 - Intro.flac', None), + basename='01 - Intro.flac', title='Album B Intro') + assert got == 2 + + +def test_title_drift_still_matches_after_normalization(): + cands = [_row(5, '/old/track.flac', 'Song (Remastered)')] + assert pick_history_row(cands, current_paths=('/new/track.flac', None), + basename='track.flac', title='song remastered') == 5 + + +def test_prefers_real_download_row_over_synthetic_scan_row(): + # the #934 collapse: a real download row (drifted path) + a synthetic scan dup at the + # exact current path. The REAL row must win, so the synthetic one can be deleted. + cands = [_row(10, '/old/song.flac', 'Song', source='soulseek'), + _row(11, '/lib/song.flac', 'Song', source='acoustid_scan')] + assert pick_history_row(cands, current_paths=('/lib/song.flac', None), + basename='song.flac', title='Song') == 10 + + +def test_no_basename_match_returns_none(): + cands = [_row(1, '/x/other.flac', 'Other')] + assert pick_history_row(cands, current_paths=('/x/wanted.flac', None), + basename='wanted.flac', title='Wanted') is None + + +def test_filename_only_substring_is_not_a_match(): + # '/x/mysong.flac' must NOT satisfy basename 'song.flac' + cands = [_row(1, '/x/mysong.flac', 'My Song')] + assert pick_history_row(cands, current_paths=('/x/song.flac', None), + basename='song.flac', title='Song') is None + + +def test_like_filter_escapes_metacharacters(): + # underscores/percents in filenames must not become LIKE wildcards + assert like_filename_filter('a_b%c.flac') == r'%a\_b\%c.flac'