Import: never wipe a clean/matched import's tags when enhancement fails (#804)
CubeComming #804: since 2.6.7, importing already-tagged files (Bruno Mars, Coldplay) blanked EVERY tag and filed them under "Unknown Artist". Root cause: both metadata-enhancement blocks in post_process_matched_download did `except Exception: wipe_source_tags(file_path)` — a full audio.tags.clear() + strip + clear_pictures. But enhancement throwing means NO new tags were written, so wiping just destroys the originals. A transient enhancement error on a well-tagged file = total metadata loss. (The reported "bitrate change" is a red herring: mutagen padding on re-save, not a re-encode — ReplayGain only reads via ffmpeg and tags via mutagen.) Fix: gate the failure-path wipe on should_wipe_tags_on_enhancement_failure() (new pure, tested policy) — only wipe UNMATCHED downloads (likely junk source tags); NEVER wipe a clean/matched import, preserve its existing tags + log. Unmatched-download behavior is unchanged, so the only thing that changes is the broken case. Tests: 3 pin the policy (clean→preserve, unmatched→strip, falsey→strip). 1211 import/pipeline/metadata tests pass.
This commit is contained in:
parent
8f7ff472b2
commit
d9dcf57f43
3 changed files with 60 additions and 2 deletions
|
|
@ -62,6 +62,7 @@ from core.runtime_state import (
|
|||
)
|
||||
from core.metadata.artwork import download_cover_art
|
||||
from core.metadata.common import wipe_source_tags
|
||||
from core.imports.tag_policy import should_wipe_tags_on_enhancement_failure
|
||||
from core.metadata.enrichment import enhance_file_metadata
|
||||
from core.imports.paths import (
|
||||
build_final_path_for_track,
|
||||
|
|
@ -590,7 +591,13 @@ def post_process_matched_download(context_key, context, file_path, runtime, meta
|
|||
except Exception as meta_err:
|
||||
import traceback
|
||||
pp_logger.info(f"[inner] Metadata enhancement FAILED for {context_key}: {meta_err}\n{traceback.format_exc()}")
|
||||
wipe_source_tags(file_path)
|
||||
if should_wipe_tags_on_enhancement_failure(has_clean_metadata):
|
||||
wipe_source_tags(file_path)
|
||||
else:
|
||||
logger.warning(
|
||||
"[Metadata] Enhancement failed but import has clean/matched metadata — "
|
||||
"preserving the file's existing tags (not wiping): %s",
|
||||
os.path.basename(file_path))
|
||||
|
||||
logger.info(f"Moving '{os.path.basename(file_path)}' to '{final_path}'")
|
||||
safe_move_file(file_path, final_path)
|
||||
|
|
@ -785,7 +792,13 @@ def post_process_matched_download(context_key, context, file_path, runtime, meta
|
|||
except Exception as meta_err:
|
||||
import traceback
|
||||
pp_logger.info(f"[inner] Metadata enhancement FAILED for {context_key}: {meta_err}\n{traceback.format_exc()}")
|
||||
wipe_source_tags(file_path)
|
||||
if should_wipe_tags_on_enhancement_failure(has_clean_metadata):
|
||||
wipe_source_tags(file_path)
|
||||
else:
|
||||
logger.warning(
|
||||
"[Metadata] Enhancement failed but import has clean/matched metadata — "
|
||||
"preserving the file's existing tags (not wiping): %s",
|
||||
os.path.basename(file_path))
|
||||
|
||||
_enhance_source_info = context.get('track_info', {}).get('source_info') or {}
|
||||
if isinstance(_enhance_source_info, str):
|
||||
|
|
|
|||
23
core/imports/tag_policy.py
Normal file
23
core/imports/tag_policy.py
Normal file
|
|
@ -0,0 +1,23 @@
|
|||
"""Tag-preservation policy for the import pipeline.
|
||||
|
||||
Tiny, pure, and deliberately its own seam: it encodes one rule that, when it
|
||||
was wrong, silently destroyed users' metadata (#804). Keeping it here with a
|
||||
regression test stops anyone from re-introducing the unconditional wipe.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
|
||||
def should_wipe_tags_on_enhancement_failure(has_clean_metadata: bool) -> bool:
|
||||
"""Whether to strip the file's tags after metadata enhancement raised.
|
||||
|
||||
Enhancement throwing means NO new tags were written, so wiping just
|
||||
destroys whatever the file already had. For a clean/matched import that's
|
||||
catastrophic — #804: already-tagged files (Bruno Mars, Coldplay) got
|
||||
blanked into an "Unknown Artist" folder by a transient enhancement error.
|
||||
|
||||
So: only wipe for UNMATCHED downloads (no clean/matched metadata), where
|
||||
the tags are likely source junk anyway. NEVER wipe a clean/matched import —
|
||||
preserve the user's existing tags.
|
||||
"""
|
||||
return not bool(has_clean_metadata)
|
||||
22
tests/test_import_tag_preservation.py
Normal file
22
tests/test_import_tag_preservation.py
Normal file
|
|
@ -0,0 +1,22 @@
|
|||
"""Regression: a metadata-enhancement failure must NOT wipe a clean/matched
|
||||
import's tags (#804 — already-tagged files were blanked into Unknown Artist).
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
from core.imports.tag_policy import should_wipe_tags_on_enhancement_failure
|
||||
|
||||
|
||||
def test_clean_matched_import_is_never_wiped_on_failure():
|
||||
# The #804 case: matched import (clean metadata) → preserve existing tags.
|
||||
assert should_wipe_tags_on_enhancement_failure(has_clean_metadata=True) is False
|
||||
|
||||
|
||||
def test_unmatched_download_still_strips_junk_on_failure():
|
||||
# Unchanged behavior for unmatched downloads (likely junk source tags).
|
||||
assert should_wipe_tags_on_enhancement_failure(has_clean_metadata=False) is True
|
||||
|
||||
|
||||
def test_falsey_values_treated_as_unmatched():
|
||||
assert should_wipe_tags_on_enhancement_failure(None) is True
|
||||
assert should_wipe_tags_on_enhancement_failure(0) is True
|
||||
Loading…
Reference in a new issue