danvics/soulsync
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions 1

#889: FIX data loss — re-identify to the same release no longer deletes the file

Browse source 
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.
This commit is contained in:
BoulderBadgeDad 2026-06-18 17:13:46 -07:00
parent 1ccc7b5e15
commit 8e218303be
3 changed files with 93 additions and 26 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

30
core/auto_import_worker.py
View file

@ -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 ──

63
core/imports/rematch_hints.py
View file

@ -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:

26
tests/imports/test_rematch_hints_seam.py
View file

@ -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).