From 4487a9d2dc5bbae47f9242237ec52b66d5d42e66 Mon Sep 17 00:00:00 2001 From: BoulderBadgeDad Date: Sat, 27 Jun 2026 22:26:55 -0700 Subject: [PATCH] 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. --- core/downloads/orphan_history.py | 14 +++++- database/music_database.py | 58 +++++++++++++++------- tests/test_orphan_history.py | 38 ++++++++++++++ tests/test_unverified_history_reconcile.py | 17 +++++++ web_server.py | 8 ++- 5 files changed, 113 insertions(+), 22 deletions(-) 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,