downloads(#934): title-guard the reconcile basename match (fix self-introduced false-heal)
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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LWJk7EuM7YktQeNyqQwTZY
This commit is contained in:
parent
027cf74d47
commit
27ea2ee735
2 changed files with 77 additions and 15 deletions
|
|
@ -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:
|
||||
|
|
|
|||
|
|
@ -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"
|
||||
|
|
|
|||
Loading…
Reference in a new issue