From 8e218303be0c7aad7fd13c3733c0f8a8c23a9d1e Mon Sep 17 00:00:00 2001 From: BoulderBadgeDad Date: Thu, 18 Jun 2026 17:13:46 -0700 Subject: [PATCH] =?UTF-8?q?#889:=20FIX=20data=20loss=20=E2=80=94=20re-iden?= =?UTF-8?q?tify=20to=20the=20same=20release=20no=20longer=20deletes=20the?= =?UTF-8?q?=20file?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Picking the release a track is ALREADY in deleted the file: the re-import lands at the same path, record_soulsync_library_entry skips the insert (row exists), so no new row is created — then delete_replaced_track removed that very row AND unlinked the file (the freshly-imported one). This was the 'assumption' I documented but never enforced. Bulletproof guard: the pipeline writes its landing path into context['_final_processed_path']; _process_matches captures it onto the candidate, and _finalize_rematch_hint passes those new_paths to delete_replaced_track. If the old file canonically equals where the import landed, it's a NO-OP — the row and file are kept (that file IS the re-imported track). Canonical compare folds symlinks/case/sep. So same-release re-identify is now a harmless re-tag-in-place; only a genuine re-home (different path) deletes the old. 114 auto-import + rematch tests green. --- core/auto_import_worker.py | 30 ++++++++--- core/imports/rematch_hints.py | 63 ++++++++++++++++-------- tests/imports/test_rematch_hints_seam.py | 26 ++++++++++ 3 files changed, 93 insertions(+), 26 deletions(-) diff --git a/core/auto_import_worker.py b/core/auto_import_worker.py index 38c7ad8a..15776fa4 100644 --- a/core/auto_import_worker.py +++ b/core/auto_import_worker.py @@ -719,9 +719,11 @@ class AutoImportWorker: self._bump_stat('auto_processed') # Re-identify (#889): only NOW that the new home exists do we # consume the hint and (if replace was chosen) delete the old - # row + file — so a failed import never loses the original. + # row + file — so a failed import never loses the original. Pass + # the landing paths so we never delete a file the re-import landed + # at the SAME place (picking the release it's already in). if rematch_hint is not None: - self._finalize_rematch_hint(rematch_hint) + self._finalize_rematch_hint(rematch_hint, getattr(candidate, '_reid_final_paths', None)) else: self._bump_stat('failed') @@ -1053,10 +1055,12 @@ class AutoImportWorker: logger.debug("[Auto-Import] rematch-hint lookup skipped: %s", e) return None, None - def _finalize_rematch_hint(self, hint) -> None: + def _finalize_rematch_hint(self, hint, new_paths=None) -> None: """Post-success: delete the replaced library row + file (if the user chose - replace) and consume the hint so it's single-use. Best-effort — a cleanup - failure is logged, never raised, since the re-import already succeeded.""" + replace) and consume the hint so it's single-use. ``new_paths`` are where the + re-import landed — passed through so the same-home guard never deletes a file + the import wrote at the old location. Best-effort — a cleanup failure is + logged, never raised, since the re-import already succeeded.""" try: from core.imports.rematch_hints import consume_hint, delete_replaced_track @@ -1072,7 +1076,8 @@ class AutoImportWorker: conn = self.database._get_connection() try: cursor = conn.cursor() - removed = delete_replaced_track(cursor, hint.replace_track_id, resolve_fn=_resolve_old) + removed = delete_replaced_track(cursor, hint.replace_track_id, + resolve_fn=_resolve_old, new_paths=new_paths) consume_hint(cursor, hint.id) conn.commit() finally: @@ -1682,6 +1687,7 @@ class AutoImportWorker: processed = 0 errors = [] + reid_final_paths = [] # #889: where the pipeline landed each file (same-home guard) all_matches = list(match_result.get('matches', [])) # Album total duration — sum of every matched track's duration. @@ -1862,6 +1868,11 @@ class AutoImportWorker: self._process_callback(context_key, context, file_path) processed += 1 + # Capture where the pipeline actually landed the file (#889 same-home + # guard) — the pipeline writes it back into the mutable context. + _landed = context.get('_final_processed_path') + if _landed: + reid_final_paths.append(_landed) logger.info(f"[Auto-Import] Processed: {track_number}. {track_name}") except Exception as e: @@ -1885,6 +1896,13 @@ class AutoImportWorker: except Exception as e: logger.debug("automation emit failed: %s", e) + # Stash landing paths on the candidate so _finalize_rematch_hint can avoid + # deleting a file the re-import landed at the SAME place (#889). + try: + candidate._reid_final_paths = reid_final_paths + except Exception as e: + logger.debug("could not stash reid final paths: %s", e) + return processed > 0 # ── Database ── diff --git a/core/imports/rematch_hints.py b/core/imports/rematch_hints.py index 52dd75c7..8e5d8e4c 100644 --- a/core/imports/rematch_hints.py +++ b/core/imports/rematch_hints.py @@ -222,27 +222,41 @@ def build_identification_from_hint(hint: RematchHint) -> dict: } +def _canonical(path: Optional[str]) -> str: + """Canonical form of a path for same-file comparison (symlinks + case + sep).""" + if not path: + return "" + try: + return os.path.normcase(os.path.realpath(path)) + except OSError: + return os.path.normcase(os.path.normpath(path)) + + def delete_replaced_track( cursor: Any, replace_track_id: Any, *, unlink=os.remove, resolve_fn: Optional[Callable[[str], Optional[str]]] = None, + new_paths: Optional[list] = 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 - original is never lost on failure. Safe by construction: the file is unlinked - only if it still exists and **no other track row references it** (guards against - 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. + original is never lost on failure. Safe by construction: - ``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).""" + * **Same-home guard (CRITICAL):** if the re-import landed at the SAME file as the + old one (``new_paths`` — the paths the import actually wrote), this is a no-op: + we DON'T delete the row or the file, because that file IS the re-imported track. + This is what stops "re-identify to the release it's already in" from deleting + the file (the import reuses the same row, so deleting it would orphan the file). + * the file is unlinked only if it still exists and **no other track row references + it** (guards against 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. ``resolve_fn`` maps the STORED DB path to the file's actual + on-disk location (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,)) @@ -250,25 +264,34 @@ def delete_replaced_track( if row is None: return None old_path = (row["file_path"] if not isinstance(row, (tuple, list)) else row[0]) or "" - - cursor.execute("DELETE FROM tracks WHERE id = ?", (replace_track_id,)) - if not old_path: + cursor.execute("DELETE FROM tracks WHERE id = ?", (replace_track_id,)) return None - # 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. + + # Resolve the old stored path to its real on-disk location up front. 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 + + # Same-home guard: if the re-import wrote to this very file, do NOTHING — the row + # is the re-imported track's row and the file is its file. Deleting either would + # be data loss (the "picked the same release" bug). + if new_paths: + landed = {_canonical(p) for p in new_paths if p} + if _canonical(real_path) in landed or _canonical(old_path) in landed: + return None + + cursor.execute("DELETE FROM tracks WHERE id = ?", (replace_track_id,)) + # 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 try: - if os.path.exists(real_path): + if os.path.exists(real_path): # real_path resolved above unlink(real_path) return real_path except OSError: diff --git a/tests/imports/test_rematch_hints_seam.py b/tests/imports/test_rematch_hints_seam.py index c1457755..5917cbdc 100644 --- a/tests/imports/test_rematch_hints_seam.py +++ b/tests/imports/test_rematch_hints_seam.py @@ -112,6 +112,32 @@ 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_same_home_is_noop(conn): + # THE data-loss bug: re-identify to the release it's already in → the import + # reuses the same file/row, so deleting it would orphan the file. Guard: no-op. + cur = conn.cursor() + cur.execute("INSERT INTO tracks (id, file_path) VALUES (7, '/lib/Album1/05 - Song.flac')") + removed = [] + out = delete_replaced_track(cur, 7, unlink=lambda p: removed.append(p), + new_paths=['/lib/Album1/05 - Song.flac']) + assert out is None and removed == [] # NOTHING unlinked + cur.execute("SELECT 1 FROM tracks WHERE id = 7") + assert cur.fetchone() is not None # row PRESERVED (it's the re-imported track) + + +def test_delete_replaced_track_different_home_still_deletes(conn): + # Genuinely re-homed (new path differs) → old row + file removed as intended. + cur = conn.cursor() + cur.execute("INSERT INTO tracks (id, file_path) VALUES (7, '/lib/EP1/05 - Song.flac')") + removed = [] + out = delete_replaced_track(cur, 7, unlink=lambda p: removed.append(p), + new_paths=['/lib/Album1/05 - Song.flac']) + assert out == '/lib/EP1/05 - Song.flac' + assert removed == ['/lib/EP1/05 - Song.flac'] + cur.execute("SELECT 1 FROM tracks WHERE id = 7") + assert cur.fetchone() is None + + 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).