From d9dcf57f43ce7e627539db89f89257bbd39ff59a Mon Sep 17 00:00:00 2001 From: BoulderBadgeDad Date: Sun, 7 Jun 2026 22:44:43 -0700 Subject: [PATCH] Import: never wipe a clean/matched import's tags when enhancement fails (#804) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- core/imports/pipeline.py | 17 +++++++++++++++-- core/imports/tag_policy.py | 23 +++++++++++++++++++++++ tests/test_import_tag_preservation.py | 22 ++++++++++++++++++++++ 3 files changed, 60 insertions(+), 2 deletions(-) create mode 100644 core/imports/tag_policy.py create mode 100644 tests/test_import_tag_preservation.py diff --git a/core/imports/pipeline.py b/core/imports/pipeline.py index fc5bdf24..369b4d66 100644 --- a/core/imports/pipeline.py +++ b/core/imports/pipeline.py @@ -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): diff --git a/core/imports/tag_policy.py b/core/imports/tag_policy.py new file mode 100644 index 00000000..3983b416 --- /dev/null +++ b/core/imports/tag_policy.py @@ -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) diff --git a/tests/test_import_tag_preservation.py b/tests/test_import_tag_preservation.py new file mode 100644 index 00000000..0ca0f308 --- /dev/null +++ b/tests/test_import_tag_preservation.py @@ -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