From 1367108e027431d52398ee3cdc8e826cbde786ae Mon Sep 17 00:00:00 2001 From: BoulderBadgeDad Date: Thu, 18 Jun 2026 16:46:07 -0700 Subject: [PATCH] #889: fix replace-delete (resolve path) + re-identify now inherits album year/track# MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two real bugs surfaced in testing: 1. Original file not deleted on replace: delete_replaced_track checked os.path.exists on the RAW stored DB path (a Docker/media-server view), so it removed the row but orphaned the file. Now takes a resolve_fn (wired to resolve_library_file_path in the worker) and unlinks the RESOLVED real path. 2. No year / wrong album context: build_identification_from_hint set is_single=True, routing re-identify through _match_tracks' singles fast-path — which never fetches the chosen album, so the re-import got a bare stub (no release_date, total_tracks=1). Added force_album_match so the matcher FETCHES the chosen release even for a lone staged file → the track inherits the real album's year, in-album track number, and art. Holds for single-type releases too (they have a year as well). Normal single-import behavior unchanged (force_album_match absent → same path). 112 auto-import + rematch tests green. --- core/auto_import_worker.py | 19 +++++++++-- core/imports/rematch_hints.py | 40 +++++++++++++++++++----- tests/imports/test_rematch_hints_seam.py | 23 ++++++++++++++ 3 files changed, 71 insertions(+), 11 deletions(-) diff --git a/core/auto_import_worker.py b/core/auto_import_worker.py index c3f85603..38c7ad8a 100644 --- a/core/auto_import_worker.py +++ b/core/auto_import_worker.py @@ -1059,10 +1059,20 @@ class AutoImportWorker: failure is logged, never raised, since the re-import already succeeded.""" try: from core.imports.rematch_hints import consume_hint, delete_replaced_track + + def _resolve_old(stored): + # The old row's path is a STORED path (Docker/media-server view) — map + # it to a file this process can actually unlink, same as everywhere else. + try: + from core.library.path_resolver import resolve_library_file_path + return resolve_library_file_path(stored, config_manager=getattr(self, '_config_manager', None)) + except Exception: + return None + conn = self.database._get_connection() try: cursor = conn.cursor() - removed = delete_replaced_track(cursor, hint.replace_track_id) + removed = delete_replaced_track(cursor, hint.replace_track_id, resolve_fn=_resolve_old) consume_hint(cursor, hint.id) conn.commit() finally: @@ -1500,8 +1510,11 @@ class AutoImportWorker: def _match_tracks(self, candidate: FolderCandidate, identification: Dict) -> Optional[Dict]: """Match staging files to the identified album's tracklist.""" - # Singles: no album tracklist to match against — the file IS the match - if candidate.is_single or identification.get('is_single'): + # Singles: no album tracklist to match against — the file IS the match. + # force_album_match (set by a re-identify hint) overrides this: even a lone + # staged file is matched INTO the chosen album, so it inherits the album's + # year / track number / art instead of the bare singles stub (#889). + if not identification.get('force_album_match') and (candidate.is_single or identification.get('is_single')): conf = identification.get('identification_confidence', 0.7) track_data = { 'name': identification.get('track_name', identification.get('album_name', '')), diff --git a/core/imports/rematch_hints.py b/core/imports/rematch_hints.py index 2a75e66a..52dd75c7 100644 --- a/core/imports/rematch_hints.py +++ b/core/imports/rematch_hints.py @@ -28,7 +28,7 @@ from __future__ import annotations import os from dataclasses import dataclass -from typing import Any, Optional +from typing import Any, Callable, Optional # Columns in INSERT/SELECT order — single source of truth so the dataclass, the # write, and the read can't drift apart. @@ -212,12 +212,23 @@ def build_identification_from_hint(hint: RematchHint) -> dict: "source": hint.source, "method": "rematch_hint", "identification_confidence": 1.0, - "is_single": True, + # is_single reflects the CHOSEN release, but force_album_match makes the + # matcher FETCH that release (even for a lone staged file) instead of taking + # the singles fast-path — so the re-imported track gets the real album + # metadata: year, the correct in-album track number, and the album art. + "is_single": (str(hint.album_type or "").lower() == "single"), + "force_album_match": True, "album_type": hint.album_type, } -def delete_replaced_track(cursor: Any, replace_track_id: Any, *, unlink=os.remove) -> Optional[str]: +def delete_replaced_track( + cursor: Any, + replace_track_id: Any, + *, + unlink=os.remove, + resolve_fn: Optional[Callable[[str], Optional[str]]] = None, +) -> Optional[str]: """Remove the OLD library row a re-identify replaces, and its file. Called only AFTER the re-import has landed the track at its new home, so the @@ -226,7 +237,12 @@ def delete_replaced_track(cursor: Any, replace_track_id: Any, *, unlink=os.remov yanking a file a different row legitimately points to). Returns the path it removed, or ``None`` if there was nothing to do. ``unlink`` is injectable for tests. Assumes the new home differs from the old — the Re-identify modal never - offers the track's current release as a target, so this holds.""" + offers the track's current release as a target, so this holds. + + ``resolve_fn`` maps the STORED DB path to the file's actual on-disk location + before the exists/unlink check — essential because the stored path may be a + Docker/media-server view this process can't read literally (without it we'd + delete the row but orphan the file).""" if not replace_track_id: return None cursor.execute("SELECT file_path FROM tracks WHERE id = ?", (replace_track_id,)) @@ -239,14 +255,22 @@ def delete_replaced_track(cursor: Any, replace_track_id: Any, *, unlink=os.remov if not old_path: return None - # Only unlink if no surviving row still points at this file. + # Only unlink if no surviving row still points at this file (rows store the + # stored path, so compare against the stored path, not the resolved one). cursor.execute("SELECT 1 FROM tracks WHERE file_path = ? LIMIT 1", (old_path,)) if cursor.fetchone() is not None: return None + # Resolve to the real on-disk path before touching the filesystem. + real_path = old_path + if resolve_fn is not None: + try: + real_path = resolve_fn(old_path) or old_path + except Exception: + real_path = old_path try: - if os.path.exists(old_path): - unlink(old_path) - return old_path + if os.path.exists(real_path): + unlink(real_path) + return real_path except OSError: pass return None diff --git a/tests/imports/test_rematch_hints_seam.py b/tests/imports/test_rematch_hints_seam.py index 58d479fd..c1457755 100644 --- a/tests/imports/test_rematch_hints_seam.py +++ b/tests/imports/test_rematch_hints_seam.py @@ -69,7 +69,18 @@ def test_build_identification_maps_hint_fields(): assert ident["track_number"] == 5 assert ident["method"] == "rematch_hint" assert ident["identification_confidence"] == 1.0 + # album_type 'album' → not a single, and force_album_match makes the matcher + # fetch the real album (year/track#/art) instead of the singles stub. + assert ident["is_single"] is False + assert ident["force_album_match"] is True + + +def test_build_identification_single_release_still_forces_album_fetch(): + # Even a chosen SINGLE release is fetched (it has a year too); is_single flags + # the type, force_album_match drives the album path regardless. + ident = build_identification_from_hint(_hint(album_type="single")) assert ident["is_single"] is True + assert ident["force_album_match"] is True # ── pure: safe replacement ──────────────────────────────────────────────────── @@ -101,6 +112,18 @@ def test_delete_replaced_track_noops_on_missing_id(conn): assert delete_replaced_track(cur, 999) is None # no such row +def test_delete_replaced_track_resolves_path_before_unlink(conn): + # The stored path is a server/Docker view this process can't read literally; + # resolve_fn maps it to the real file so we unlink the RIGHT path (not orphan it). + cur = conn.cursor() + cur.execute("INSERT INTO tracks (id, file_path) VALUES (7, '/mnt/serverview/Song.flac')") + removed = [] + out = delete_replaced_track(cur, 7, unlink=lambda p: removed.append(p), + resolve_fn=lambda stored: '/real/local/Song.flac') + assert out == '/real/local/Song.flac' + assert removed == ['/real/local/Song.flac'] # unlinked the RESOLVED path + + # patch os.path.exists so the unlink branch is reachable without real files @pytest.fixture(autouse=True) def _exists(monkeypatch):