unverified-history reconcile + orphan-clean: hardening follow-up to #938
four fixes from the review (and a self-correction): 1) close the connection. reconcile_unverified_history_from_tracks opened a connection with no finally/close. runs once per boot so GC reclaimed it, but now it's consistent + robust. 2) scope the tracks scan to the review queue. it built lookup dicts from EVERY verified/ human_verified track (~350k on a large library) on every boot while anything is unverified (the normal state). now it loads the stuck rows first and skips verified tracks whose path AND basename can't match any queued row, so dicts stay proportional to the queue, not the library. behaviour identical (all 13 PR reconcile tests still pass). 3) close the title-less basename collision. a title-less history row fell back to filename-only matching with no ambiguity check, so a generic name like "01 - Intro.flac" could heal a DIFFERENT song to verified. now a title-less basename heal only fires when that basename is unique among verified tracks; unique-basename rows still heal (recall preserved). 4) "Clean orphaned" protects force_imported rows (deliberate user decision, keep for human approval) without weakening the mount-down safety gate. CRUCIAL self-correction: filtering them out BEFORE the orphan check (my first cut) shrank the checked count below the threshold and would have let a few unverified orphans be deleted during a mount outage. instead, find_orphan_history_ids now takes a deletable predicate: protected rows still count toward checked / all-missing (gate stays strong) but never enter the orphan_ids delete set. 3 new regression tests (title-less collision; deletable protects from delete; protected rows still count toward the gate). 936 verification/acoustid/history/downloads tests green. builds on nick2000713's #938.
This commit is contained in:
parent
d84fba14ba
commit
4487a9d2dc
5 changed files with 113 additions and 22 deletions
|
|
@ -25,6 +25,7 @@ def find_orphan_history_ids(
|
||||||
resolve: Callable[[dict], Any],
|
resolve: Callable[[dict], Any],
|
||||||
*,
|
*,
|
||||||
min_for_safety: int = 5,
|
min_for_safety: int = 5,
|
||||||
|
deletable: Callable[[dict], bool] | None = None,
|
||||||
) -> dict:
|
) -> dict:
|
||||||
"""Return ``{'orphan_ids', 'checked', 'suspicious'}``.
|
"""Return ``{'orphan_ids', 'checked', 'suspicious'}``.
|
||||||
|
|
||||||
|
|
@ -32,14 +33,23 @@ def find_orphan_history_ids(
|
||||||
find no file for it. ``suspicious`` is True when every checked row is
|
find no file for it. ``suspicious`` is True when every checked row is
|
||||||
missing and there are at least ``min_for_safety`` of them — the mount-down
|
missing and there are at least ``min_for_safety`` of them — the mount-down
|
||||||
signature; the caller should refuse to delete in that case.
|
signature; the caller should refuse to delete in that case.
|
||||||
|
|
||||||
|
``deletable`` (optional) protects rows from removal WITHOUT weakening the
|
||||||
|
safety gate: a protected row still counts toward ``checked`` and the
|
||||||
|
all-missing signal (so e.g. a few unverified orphans can't be swept during a
|
||||||
|
mount outage just because protected rows were filtered out first), but it
|
||||||
|
never appears in ``orphan_ids``. Default: every missing row is deletable.
|
||||||
"""
|
"""
|
||||||
orphan_ids = []
|
orphan_ids = []
|
||||||
checked = 0
|
checked = 0
|
||||||
|
missing = 0
|
||||||
for row in rows:
|
for row in rows:
|
||||||
if not str((row.get('file_path') or '')).strip():
|
if not str((row.get('file_path') or '')).strip():
|
||||||
continue
|
continue
|
||||||
checked += 1
|
checked += 1
|
||||||
if resolve(row) is None:
|
if resolve(row) is None:
|
||||||
orphan_ids.append(row.get('id'))
|
missing += 1
|
||||||
suspicious = checked >= min_for_safety and len(orphan_ids) == checked
|
if deletable is None or deletable(row):
|
||||||
|
orphan_ids.append(row.get('id'))
|
||||||
|
suspicious = checked >= min_for_safety and missing == checked
|
||||||
return {'orphan_ids': orphan_ids, 'checked': checked, 'suspicious': suspicious}
|
return {'orphan_ids': orphan_ids, 'checked': checked, 'suspicious': suspicious}
|
||||||
|
|
|
||||||
|
|
@ -13871,10 +13871,12 @@ class MusicDatabase:
|
||||||
Unverified (issue #934).
|
Unverified (issue #934).
|
||||||
|
|
||||||
A basename match is title-guarded: a shared track-number filename
|
A basename match is title-guarded: a shared track-number filename
|
||||||
("01 - Intro.flac") must NOT heal a different song, so when both the
|
("01 - Intro.flac") must NOT heal a different song. When both the history
|
||||||
history row and the candidate track carry a title they have to agree
|
row and the candidate track carry a title they have to agree
|
||||||
(alphanumeric-lowercase) — the same guard the AcoustID matcher uses. An
|
(alphanumeric-lowercase) — the same guard the AcoustID matcher uses. When
|
||||||
exact-path match is unambiguous and needs no guard.
|
a title is missing on either side we can't tell which file the basename
|
||||||
|
refers to, so we only heal if that basename is unambiguous (a single
|
||||||
|
verified candidate). An exact-path match needs no guard.
|
||||||
|
|
||||||
Upgrade-only and non-destructive: it only lifts 'unverified' rows to the
|
Upgrade-only and non-destructive: it only lifts 'unverified' rows to the
|
||||||
confirmed status, never downgrades and never deletes. Returns the number
|
confirmed status, never downgrades and never deletes. Returns the number
|
||||||
|
|
@ -13882,18 +13884,28 @@ class MusicDatabase:
|
||||||
track) are left untouched.
|
track) are left untouched.
|
||||||
"""
|
"""
|
||||||
healed = 0
|
healed = 0
|
||||||
|
conn = None
|
||||||
try:
|
try:
|
||||||
conn = self._get_connection()
|
conn = self._get_connection()
|
||||||
cursor = conn.cursor()
|
cursor = conn.cursor()
|
||||||
# Cheap early-out: skip the tracks scan entirely when nothing is stuck.
|
|
||||||
cursor.execute(
|
|
||||||
"SELECT 1 FROM library_history WHERE verification_status = 'unverified' LIMIT 1")
|
|
||||||
if cursor.fetchone() is None:
|
|
||||||
return 0
|
|
||||||
|
|
||||||
def _norm(value):
|
def _norm(value):
|
||||||
return ''.join(ch for ch in str(value or '').lower() if ch.isalnum())
|
return ''.join(ch for ch in str(value or '').lower() if ch.isalnum())
|
||||||
|
|
||||||
|
# Load the stuck rows first. Cheap early-out when nothing is stuck —
|
||||||
|
# and their paths/basenames scope the tracks scan below, so the
|
||||||
|
# lookup dicts stay proportional to the (small) review queue instead
|
||||||
|
# of the whole library.
|
||||||
|
cursor.execute(
|
||||||
|
"SELECT id, file_path, title FROM library_history "
|
||||||
|
"WHERE verification_status = 'unverified' "
|
||||||
|
"AND file_path IS NOT NULL AND file_path != ''")
|
||||||
|
stuck_rows = cursor.fetchall()
|
||||||
|
if not stuck_rows:
|
||||||
|
return 0
|
||||||
|
needed_paths = {fp for _, fp, _ in stuck_rows if fp}
|
||||||
|
needed_bases = {os.path.basename(fp) for _, fp, _ in stuck_rows if fp}
|
||||||
|
|
||||||
rank = {'verified': 1, 'human_verified': 2}
|
rank = {'verified': 1, 'human_verified': 2}
|
||||||
by_path = {} # exact path -> status (unambiguous; no title guard)
|
by_path = {} # exact path -> status (unambiguous; no title guard)
|
||||||
by_base = {} # basename -> list of (norm_title, status)
|
by_base = {} # basename -> list of (norm_title, status)
|
||||||
|
|
@ -13904,26 +13916,31 @@ class MusicDatabase:
|
||||||
for fp, st, ttitle in cursor.fetchall():
|
for fp, st, ttitle in cursor.fetchall():
|
||||||
if not fp:
|
if not fp:
|
||||||
continue
|
continue
|
||||||
|
base = os.path.basename(fp)
|
||||||
|
# Skip verified tracks that can't possibly match a queued row.
|
||||||
|
if fp not in needed_paths and base not in needed_bases:
|
||||||
|
continue
|
||||||
if rank.get(st, 0) >= rank.get(by_path.get(fp), 0):
|
if rank.get(st, 0) >= rank.get(by_path.get(fp), 0):
|
||||||
by_path[fp] = st
|
by_path[fp] = st
|
||||||
base = os.path.basename(fp)
|
|
||||||
if base:
|
if base:
|
||||||
by_base.setdefault(base, []).append((_norm(ttitle), st))
|
by_base.setdefault(base, []).append((_norm(ttitle), st))
|
||||||
|
|
||||||
updates = []
|
updates = []
|
||||||
cursor.execute(
|
for rid, fp, rtitle in stuck_rows:
|
||||||
"SELECT id, file_path, title FROM library_history "
|
|
||||||
"WHERE verification_status = 'unverified' "
|
|
||||||
"AND file_path IS NOT NULL AND file_path != ''")
|
|
||||||
for rid, fp, rtitle in cursor.fetchall():
|
|
||||||
target = by_path.get(fp)
|
target = by_path.get(fp)
|
||||||
if not target:
|
if not target:
|
||||||
want = _norm(rtitle)
|
want = _norm(rtitle)
|
||||||
|
candidates = by_base.get(os.path.basename(fp or ''), ())
|
||||||
best = 0
|
best = 0
|
||||||
for ttitle, st in by_base.get(os.path.basename(fp or ''), ()):
|
for ttitle, st in candidates:
|
||||||
# Title guard: require agreement only when BOTH titles are
|
if want and ttitle:
|
||||||
# present (legacy rows may lack one — fall back to filename).
|
# Both titled: must agree.
|
||||||
if want and ttitle and want != ttitle:
|
if want != ttitle:
|
||||||
|
continue
|
||||||
|
elif len(candidates) > 1:
|
||||||
|
# Title missing on a side AND the basename collides
|
||||||
|
# across verified files — can't tell which one this
|
||||||
|
# row is, so don't risk healing the wrong song.
|
||||||
continue
|
continue
|
||||||
if rank.get(st, 0) >= best:
|
if rank.get(st, 0) >= best:
|
||||||
best = rank.get(st, 0)
|
best = rank.get(st, 0)
|
||||||
|
|
@ -13940,6 +13957,9 @@ class MusicDatabase:
|
||||||
logger.info("Reconciled %d unverified history rows from tracks truth", healed)
|
logger.info("Reconciled %d unverified history rows from tracks truth", healed)
|
||||||
except Exception as e:
|
except Exception as e:
|
||||||
logger.error("Error reconciling unverified history: %s", e)
|
logger.error("Error reconciling unverified history: %s", e)
|
||||||
|
finally:
|
||||||
|
if conn:
|
||||||
|
conn.close()
|
||||||
return healed
|
return healed
|
||||||
|
|
||||||
def get_library_history_stats(self):
|
def get_library_history_stats(self):
|
||||||
|
|
|
||||||
|
|
@ -47,3 +47,41 @@ def test_some_present_is_never_suspicious():
|
||||||
out = find_orphan_history_ids(rows, _resolve)
|
out = find_orphan_history_ids(rows, _resolve)
|
||||||
assert out['suspicious'] is False # at least one file exists -> library is up
|
assert out['suspicious'] is False # at least one file exists -> library is up
|
||||||
assert 99 not in out['orphan_ids']
|
assert 99 not in out['orphan_ids']
|
||||||
|
|
||||||
|
|
||||||
|
def _status_rows(*specs):
|
||||||
|
# specs: (id, file_path, exists?, verification_status)
|
||||||
|
return [{'id': i, 'file_path': p, '_exists': e, 'verification_status': s}
|
||||||
|
for i, p, e, s in specs]
|
||||||
|
|
||||||
|
|
||||||
|
def _deletable_unverified(row):
|
||||||
|
return row.get('verification_status') == 'unverified'
|
||||||
|
|
||||||
|
|
||||||
|
def test_deletable_protects_rows_from_orphan_ids():
|
||||||
|
"""force_imported rows must never be swept, even when their file is gone."""
|
||||||
|
rows = _status_rows(
|
||||||
|
(1, '/a.flac', False, 'unverified'),
|
||||||
|
(2, '/b.flac', False, 'force_imported'), # gone, but protected
|
||||||
|
)
|
||||||
|
out = find_orphan_history_ids(rows, _resolve, deletable=_deletable_unverified)
|
||||||
|
assert out['orphan_ids'] == [1] # only the unverified orphan
|
||||||
|
assert 2 not in out['orphan_ids']
|
||||||
|
|
||||||
|
|
||||||
|
def test_protected_rows_still_count_toward_mount_down_gate():
|
||||||
|
"""Regression guard (#938 follow-up): protecting rows must NOT shrink the
|
||||||
|
safety gate. A few unverified + many force_imported, ALL unreachable (mount
|
||||||
|
outage) must still read 'suspicious' so nothing is deleted — even though only
|
||||||
|
the unverified ones would otherwise be deletable."""
|
||||||
|
rows = _status_rows(
|
||||||
|
(1, '/a.flac', False, 'unverified'),
|
||||||
|
(2, '/b.flac', False, 'unverified'),
|
||||||
|
*[(i, f'/x{i}.flac', False, 'force_imported') for i in range(3, 8)],
|
||||||
|
)
|
||||||
|
out = find_orphan_history_ids(rows, _resolve, deletable=_deletable_unverified)
|
||||||
|
assert out['checked'] == 7 # all rows counted
|
||||||
|
assert out['suspicious'] is True # gate fires on all-missing → refuse
|
||||||
|
# (caller refuses on suspicious, so orphan_ids is moot — but it's only the unverified)
|
||||||
|
assert set(out['orphan_ids']) == {1, 2}
|
||||||
|
|
|
||||||
|
|
@ -172,3 +172,20 @@ def test_reconcile_basename_heals_when_history_title_missing():
|
||||||
healed = db.reconcile_unverified_history_from_tracks()
|
healed = db.reconcile_unverified_history_from_tracks()
|
||||||
assert healed == 1
|
assert healed == 1
|
||||||
assert db._status_of(1) == "verified"
|
assert db._status_of(1) == "verified"
|
||||||
|
|
||||||
|
|
||||||
|
def test_reconcile_titleless_row_does_not_heal_on_basename_collision():
|
||||||
|
"""Follow-up hardening (#938 review): a title-less history row must NOT heal
|
||||||
|
via filename when that basename collides across MORE THAN ONE verified track —
|
||||||
|
we can't tell which song it is, so healing would risk marking a genuinely
|
||||||
|
unverified import 'verified'. Unique-basename title-less heal still works
|
||||||
|
(see test_reconcile_basename_heals_when_history_title_missing)."""
|
||||||
|
db = _InMemoryDB()
|
||||||
|
# Two DIFFERENT verified songs that happen to share the generic filename.
|
||||||
|
db._add_track(1, "/lib/AlbumA/01 - Intro.flac", "verified", title="Intro A")
|
||||||
|
db._add_track(2, "/lib/AlbumB/01 - Intro.flac", "verified", title="Intro B")
|
||||||
|
# A stale, title-less history row with that same basename.
|
||||||
|
db._add_history("/transfer/X/01 - Intro.flac", "unverified", title="")
|
||||||
|
healed = db.reconcile_unverified_history_from_tracks()
|
||||||
|
assert healed == 0
|
||||||
|
assert db._status_of(1) == "unverified"
|
||||||
|
|
|
||||||
|
|
@ -8236,8 +8236,14 @@ def clean_orphan_verification_items():
|
||||||
try:
|
try:
|
||||||
from core.downloads.orphan_history import find_orphan_history_ids
|
from core.downloads.orphan_history import find_orphan_history_ids
|
||||||
db = get_database()
|
db = get_database()
|
||||||
|
# get_library_history_unverified() returns unverified + force_imported rows.
|
||||||
|
# Check ALL of them so the mount-down gate sees the true count, but only
|
||||||
|
# DELETE 'unverified' orphans — 'force_imported' is a deliberate user
|
||||||
|
# decision (accepted a version mismatch) and stays for human approval.
|
||||||
rows = db.get_library_history_unverified() or []
|
rows = db.get_library_history_unverified() or []
|
||||||
result = find_orphan_history_ids(rows, _resolve_history_audio_path)
|
result = find_orphan_history_ids(
|
||||||
|
rows, _resolve_history_audio_path,
|
||||||
|
deletable=lambda r: r.get('verification_status') == 'unverified')
|
||||||
if result['suspicious']:
|
if result['suspicious']:
|
||||||
return jsonify({
|
return jsonify({
|
||||||
"success": False,
|
"success": False,
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue