From 27ea2ee73599451b72e7ec442593753d6de8fe14 Mon Sep 17 00:00:00 2001 From: dev Date: Sun, 28 Jun 2026 01:49:41 +0200 Subject: [PATCH] downloads(#934): title-guard the reconcile basename match (fix self-introduced false-heal) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Second-pass audit of the startup reconcile found a real correctness bug in it: the basename fallback had NO title guard, so a shared track-number filename ("01 - Intro.flac" in different albums) would heal the WRONG song — marking an actually-unverified file 'verified' and silently dropping it from the review queue. This is the exact collision the scan-time matcher (history_match.py) guards against; the reconcile now mirrors it. - basename match now requires the history row's title and the candidate track's title to agree (alphanumeric-lowercase), only when BOTH are present (legacy rows without a title still fall back to filename-only, like the matcher). - exact-path matches stay unguarded (same path = same file, unambiguous). - cheap early-out: skip the tracks scan entirely when no 'unverified' rows exist (keeps the every-boot cost ~nil on healthy libraries). 3 new tests (collision must-not-heal, titles-agree heals, missing-title falls back). 8 reconcile tests green. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01LWJk7EuM7YktQeNyqQwTZY --- database/music_database.py | 50 +++++++++++++++++----- tests/test_unverified_history_reconcile.py | 42 ++++++++++++++++-- 2 files changed, 77 insertions(+), 15 deletions(-) diff --git a/database/music_database.py b/database/music_database.py index 07a85687..818146fa 100644 --- a/database/music_database.py +++ b/database/music_database.py @@ -13870,6 +13870,12 @@ class MusicDatabase: exact-path-only heal left thousands of already-verified files showing as Unverified (issue #934). + A basename match is title-guarded: a shared track-number filename + ("01 - Intro.flac") must NOT heal a different song, so when both the + history row and the candidate track carry a title they have to agree + (alphanumeric-lowercase) — the same guard the AcoustID matcher uses. An + exact-path match is unambiguous and needs no guard. + Upgrade-only and non-destructive: it only lifts 'unverified' rows to the confirmed status, never downgrades and never deletes. Returns the number of rows healed. Genuinely-unverified rows and orphans (no matching @@ -13879,27 +13885,49 @@ class MusicDatabase: try: conn = self._get_connection() cursor = conn.cursor() - rank = {'verified': 1, 'human_verified': 2} - by_path = {} - by_base = {} + # Cheap early-out: skip the tracks scan entirely when nothing is stuck. cursor.execute( - "SELECT file_path, verification_status FROM tracks " + "SELECT 1 FROM library_history WHERE verification_status = 'unverified' LIMIT 1") + if cursor.fetchone() is None: + return 0 + + def _norm(value): + return ''.join(ch for ch in str(value or '').lower() if ch.isalnum()) + + rank = {'verified': 1, 'human_verified': 2} + by_path = {} # exact path -> status (unambiguous; no title guard) + by_base = {} # basename -> list of (norm_title, status) + cursor.execute( + "SELECT file_path, verification_status, title FROM tracks " "WHERE verification_status IN ('verified', 'human_verified') " "AND file_path IS NOT NULL AND file_path != ''") - for fp, st in cursor.fetchall(): + for fp, st, ttitle in cursor.fetchall(): if not fp: continue - by_path[fp] = st + if rank.get(st, 0) >= rank.get(by_path.get(fp), 0): + by_path[fp] = st base = os.path.basename(fp) - if base and rank.get(st, 0) >= rank.get(by_base.get(base), 0): - by_base[base] = st + if base: + by_base.setdefault(base, []).append((_norm(ttitle), st)) + updates = [] cursor.execute( - "SELECT id, file_path FROM library_history " + "SELECT id, file_path, title FROM library_history " "WHERE verification_status = 'unverified' " "AND file_path IS NOT NULL AND file_path != ''") - for rid, fp in cursor.fetchall(): - target = by_path.get(fp) or by_base.get(os.path.basename(fp or '')) + for rid, fp, rtitle in cursor.fetchall(): + target = by_path.get(fp) + if not target: + want = _norm(rtitle) + best = 0 + for ttitle, st in by_base.get(os.path.basename(fp or ''), ()): + # Title guard: require agreement only when BOTH titles are + # present (legacy rows may lack one — fall back to filename). + if want and ttitle and want != ttitle: + continue + if rank.get(st, 0) >= best: + best = rank.get(st, 0) + target = st if target: updates.append((target, rid)) for status, rid in updates: diff --git a/tests/test_unverified_history_reconcile.py b/tests/test_unverified_history_reconcile.py index e614b4d3..b23ef324 100644 --- a/tests/test_unverified_history_reconcile.py +++ b/tests/test_unverified_history_reconcile.py @@ -63,7 +63,7 @@ class _InMemoryDB(MusicDatabase): self._conn.row_factory = sqlite3.Row self._conn.execute( "CREATE TABLE tracks (id INTEGER PRIMARY KEY, file_path TEXT, " - "verification_status TEXT)" + "title TEXT, verification_status TEXT)" ) self._conn.execute( "CREATE TABLE library_history (" @@ -76,10 +76,10 @@ class _InMemoryDB(MusicDatabase): def _get_connection(self): return _NonClosingConn(self._conn) - def _add_track(self, tid, path, status): + def _add_track(self, tid, path, status, title="Song"): self._conn.execute( - "INSERT INTO tracks (id, file_path, verification_status) VALUES (?,?,?)", - (tid, path, status)) + "INSERT INTO tracks (id, file_path, title, verification_status) VALUES (?,?,?,?)", + (tid, path, title, status)) self._conn.commit() def _add_history(self, path, status, title="Song"): @@ -138,3 +138,37 @@ def test_reconcile_leaves_orphans_untouched(): healed = db.reconcile_unverified_history_from_tracks() assert healed == 0 assert db._status_of(1) == "unverified" + + +def test_reconcile_basename_collision_does_not_false_heal(): + """Two different songs share the track-number filename '01 - Intro.flac'. + Only one is a verified track; the OTHER's stale history row must NOT inherit + that verified status just because the filename collides (title guard).""" + db = _InMemoryDB() + db._add_track(1, "/lib/AlbumA/01 - Intro.flac", "verified", title="Intro A") + # A genuinely different, still-unverified song with the same filename. + db._add_history("/transfer/AlbumB/01 - Intro.flac", "unverified", title="Intro B") + healed = db.reconcile_unverified_history_from_tracks() + assert healed == 0 + assert db._status_of(1) == "unverified" + + +def test_reconcile_basename_heals_when_titles_agree(): + """Same filename, same song (path drifted) — titles agree, so it heals.""" + db = _InMemoryDB() + db._add_track(1, "/lib/AlbumA/01 - Intro.flac", "verified", title="Intro") + db._add_history("/transfer/old/01 - Intro.flac", "unverified", title="Intro") + healed = db.reconcile_unverified_history_from_tracks() + assert healed == 1 + assert db._status_of(1) == "verified" + + +def test_reconcile_basename_heals_when_history_title_missing(): + """Legacy history rows may have no title — fall back to filename-only match + (mirrors the scanner matcher's allowance) so they still heal.""" + db = _InMemoryDB() + db._add_track(1, "/lib/A/01 - Song.flac", "verified", title="Whatever") + db._add_history("/transfer/old/01 - Song.flac", "unverified", title="") + healed = db.reconcile_unverified_history_from_tracks() + assert healed == 1 + assert db._status_of(1) == "verified"