From 3b0394dbc64948240e2ea744abd973888811e0d7 Mon Sep 17 00:00:00 2001 From: BoulderBadgeDad Date: Thu, 18 Jun 2026 08:42:04 -0700 Subject: [PATCH] Metadata: a mid-enrichment crash on an art-less file no longer leaves it UNTAGGED MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Sokhi: tracks occasionally land in Rockbox's 'untagged' bucket after a 'processing failed'. enhance_file_metadata saves the file with tags CLEARED up front (so stale tags never linger), then runs the failure-prone external steps (source-id embed, cover-art fetch). The core tags (album/artist/title/track from the matched context) are written to the in-memory object BEFORE those steps, but the on-disk file is still the cleared one until the final save. The #764 fix made the error handler restore ART — but gated the re-save on there being original art to restore. So a file with NO embedded art that hit a mid-enrichment crash threw away its in-memory core tags and was left on disk as the up-front clear saved it: untagged. Now the handler always persists the in-memory tags (restoring art when present), so a crash leaves a correctly-tagged file (album tag intact -> right bucket) instead of an empty one. Regression test drives the real enhance_file_metadata against an art-less FLAC. --- core/metadata/enrichment.py | 29 ++++--- tests/test_enrichment_tag_preservation.py | 100 ++++++++++++++++++++++ 2 files changed, 118 insertions(+), 11 deletions(-) create mode 100644 tests/test_enrichment_tag_preservation.py diff --git a/core/metadata/enrichment.py b/core/metadata/enrichment.py index fdd34042..e3642419 100644 --- a/core/metadata/enrichment.py +++ b/core/metadata/enrichment.py @@ -242,18 +242,25 @@ def enhance_file_metadata(file_path: str, context: dict, artist: dict, album_inf logger.warning("[Metadata Debug] Artist: %s", artist.get("name", "MISSING") if artist else "None") logger.warning("[Metadata Debug] Album info: %s", album_info.get("album_name", "MISSING") if album_info else "None") logger.error("[Metadata Debug] Traceback:\n%s", traceback.format_exc()) - # We cleared the file's art early; if the rewrite then crashed - # before re-embedding, the on-disk file (already saved cleared at - # the start) would be left art-less. Best-effort: put the original - # art back and persist it so a mid-enrichment crash never destroys - # the cover (#764). Guarded so a failure here can't mask the - # original error. + # The file was saved with tags CLEARED up front (so stale tags never + # linger), then the failure-prone enrichment ran. By the time most + # failures hit — the external source-id embed / cover-art fetch — the + # core tags (album/artist/title/track from the matched context) are + # already on the in-memory object but NOT yet on disk; the on-disk + # file is still the cleared one. Persist the in-memory tags now (and + # restore the original art too, #764) so a mid-enrichment crash leaves + # a correctly-tagged file instead of an UNTAGGED one (Sokhi: tracks + # landing in Rockbox's 'untagged' bucket after a 'processing failed'). + # + # Previously this save was gated on there being original art to + # restore, so an art-less file lost its tags entirely on any crash. + # Guarded so a failure here can't mask the original error. try: - if audio_file is not None and art_snapshot and restore_embedded_art( - audio_file, symbols, art_snapshot - ): + if audio_file is not None: + if art_snapshot: + restore_embedded_art(audio_file, symbols, art_snapshot) save_audio_file(audio_file, symbols) - logger.info("Restored original cover art after enrichment error.") + logger.info("Persisted core tags (and restored art) after enrichment error.") except Exception as restore_exc: - logger.debug("Art restore after error failed: %s", restore_exc) + logger.debug("Tag/art persist after error failed: %s", restore_exc) return False diff --git a/tests/test_enrichment_tag_preservation.py b/tests/test_enrichment_tag_preservation.py new file mode 100644 index 00000000..22127ee9 --- /dev/null +++ b/tests/test_enrichment_tag_preservation.py @@ -0,0 +1,100 @@ +"""Sokhi: tracks occasionally land 'untagged' after a processing failure. + +enhance_file_metadata clears the file's tags and saves it UP FRONT (so stale +tags never linger), then does the failure-prone enrichment (external source-id +embed, cover-art fetch) and saves again at the end. The core tags +(album/artist/title/track) come from the already-matched context and are written +to the in-memory object BEFORE those external steps — but the on-disk file is +still the cleared one until the final save. + +The #764 fix made the error handler restore ART, but it gated the re-save on +there being original art to restore. So a file with NO embedded art that hit a +mid-enrichment crash had its in-memory core tags thrown away and was left on disk +exactly as the up-front clear saved it: UNTAGGED. + +These tests run the REAL enhance_file_metadata against a REAL art-less FLAC and +assert the core tags survive a crash in the external step. +""" + +from __future__ import annotations + +import os +import tempfile +from unittest.mock import patch + +import pytest + +pytest.importorskip("mutagen") +from mutagen.flac import FLAC # noqa: E402 + +import core.metadata.enrichment as enrichment # noqa: E402 + + +class _Cfg: + def get(self, key, default=None): + return default + + +def _make_flac_no_art(path): + minimal = ( + b"fLaC" + + b"\x80\x00\x00\x22" + + b"\x00\x10\x00\x10" + + b"\x00\x00\x00\x00\x00\x00" + + b"\x0a\xc4\x42\xf0\x00\x00\x00\x00" + + b"\x00" * 16 + ) + with open(path, "wb") as f: + f.write(minimal) + FLAC(path).save() # valid FLAC, no tags, no pictures + + +@pytest.fixture +def flac_path(): + fd, path = tempfile.mkstemp(suffix=".flac") + os.close(fd) + _make_flac_no_art(path) + yield path + try: + os.remove(path) + except OSError: + pass + + +_CORE = {"title": "Yellow", "artist": "Coldplay", "album_artist": "Coldplay", + "album": "Parachutes", "track_number": 1, "total_tracks": 9, "disc_number": 1} + + +def _run(flac_path, *, metadata, embed_side_effect): + with patch.object(enrichment, "get_config_manager", return_value=_Cfg()), \ + patch.object(enrichment, "strip_all_non_audio_tags"), \ + patch.object(enrichment, "extract_source_metadata", return_value=metadata), \ + patch.object(enrichment, "embed_source_ids"), \ + patch.object(enrichment, "verify_metadata_written", return_value=True), \ + patch.object(enrichment, "embed_album_art_metadata", side_effect=embed_side_effect): + return enrichment.enhance_file_metadata( + flac_path, context={}, artist={"name": "Coldplay"}, album_info={}, + ) + + +def test_core_tags_survive_when_art_step_raises_on_artless_file(flac_path): + """The regression: art-less file + a crash in the external art step must NOT + leave the file untagged — the matched core tags must be on disk.""" + def boom(audio_file, metadata): + raise RuntimeError("art backend exploded") + + result = _run(flac_path, metadata=dict(_CORE), embed_side_effect=boom) + assert result is False # enrichment reported failure + f = FLAC(flac_path) + assert f.get("title") == ["Yellow"] # ...but core tags persisted + assert f.get("artist") == ["Coldplay"] + assert f.get("album") == ["Parachutes"] # the tag Rockbox buckets on + assert f.get("tracknumber") == ["1/9"] + + +def test_core_tags_written_on_happy_path_artless_file(flac_path): + result = _run(flac_path, metadata=dict(_CORE), embed_side_effect=lambda *a, **k: False) + assert result is True + f = FLAC(flac_path) + assert f.get("album") == ["Parachutes"] + assert f.get("artist") == ["Coldplay"]