diff --git a/core/downloads/orphan_history.py b/core/downloads/orphan_history.py index 1d5effae..838f07f1 100644 --- a/core/downloads/orphan_history.py +++ b/core/downloads/orphan_history.py @@ -25,6 +25,7 @@ def find_orphan_history_ids( resolve: Callable[[dict], Any], *, min_for_safety: int = 5, + deletable: Callable[[dict], bool] | None = None, ) -> dict: """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 missing and there are at least ``min_for_safety`` of them — the mount-down 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 = [] checked = 0 + missing = 0 for row in rows: if not str((row.get('file_path') or '')).strip(): continue checked += 1 if resolve(row) is None: - orphan_ids.append(row.get('id')) - suspicious = checked >= min_for_safety and len(orphan_ids) == checked + missing += 1 + 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} diff --git a/database/music_database.py b/database/music_database.py index 818146fa..c928b5cc 100644 --- a/database/music_database.py +++ b/database/music_database.py @@ -13871,10 +13871,12 @@ class MusicDatabase: 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. + ("01 - Intro.flac") must NOT heal a different song. 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. When + 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 confirmed status, never downgrades and never deletes. Returns the number @@ -13882,18 +13884,28 @@ class MusicDatabase: track) are left untouched. """ healed = 0 + conn = None try: conn = self._get_connection() 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): 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} by_path = {} # exact path -> status (unambiguous; no title guard) by_base = {} # basename -> list of (norm_title, status) @@ -13904,26 +13916,31 @@ class MusicDatabase: for fp, st, ttitle in cursor.fetchall(): if not fp: 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): by_path[fp] = st - base = os.path.basename(fp) if base: by_base.setdefault(base, []).append((_norm(ttitle), st)) updates = [] - cursor.execute( - "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(): + for rid, fp, rtitle in stuck_rows: target = by_path.get(fp) if not target: want = _norm(rtitle) + candidates = by_base.get(os.path.basename(fp or ''), ()) 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: + for ttitle, st in candidates: + if want and ttitle: + # Both titled: must agree. + 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 if rank.get(st, 0) >= best: best = rank.get(st, 0) @@ -13940,6 +13957,9 @@ class MusicDatabase: logger.info("Reconciled %d unverified history rows from tracks truth", healed) except Exception as e: logger.error("Error reconciling unverified history: %s", e) + finally: + if conn: + conn.close() return healed def get_library_history_stats(self): diff --git a/tests/test_orphan_history.py b/tests/test_orphan_history.py index 7018ed0c..166ca5b7 100644 --- a/tests/test_orphan_history.py +++ b/tests/test_orphan_history.py @@ -47,3 +47,41 @@ def test_some_present_is_never_suspicious(): out = find_orphan_history_ids(rows, _resolve) assert out['suspicious'] is False # at least one file exists -> library is up 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} diff --git a/tests/test_unverified_history_reconcile.py b/tests/test_unverified_history_reconcile.py index b23ef324..a87f0917 100644 --- a/tests/test_unverified_history_reconcile.py +++ b/tests/test_unverified_history_reconcile.py @@ -172,3 +172,20 @@ def test_reconcile_basename_heals_when_history_title_missing(): healed = db.reconcile_unverified_history_from_tracks() assert healed == 1 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" diff --git a/web_server.py b/web_server.py index 93d0c4c3..d3b19db7 100644 --- a/web_server.py +++ b/web_server.py @@ -8236,8 +8236,14 @@ def clean_orphan_verification_items(): try: from core.downloads.orphan_history import find_orphan_history_ids 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 [] - 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']: return jsonify({ "success": False,