Import: atomic tag saves so an interrupted/OOM save can't destroy the file (#819)

CubeComming: manual imports of large hi-res Qobuz FLACs came out as empty
shells — no audio, no tags, no length/bitrate. Root cause: mutagen's in-place
save() rewrites the file, and it's NOT atomic; if the process is interrupted or
OOM-killed mid-write (he also reported the memory-growth issue #802), the file
is left truncated — audio and tags gone.

save_audio_file (the chokepoint both the enrichment tag-write AND wipe_source_
tags route through) now saves atomically: copy the original to a temp in the
same dir, write the modified tags into the copy, verify it's still valid audio
(duration > 0), then os.replace() it in. The original is untouched until that
atomic swap, so a crash can only orphan the temp — never destroy the user's
file. Falls back to the plain in-place save (byte-identical to before) when the
atomic path can't run, so nothing is ever left worse off. tag_writer's
write_tags_to_file routes through the same helper.

Verified the atomic path works with REAL mutagen on a real FLAC (audio length
preserved 1.0s→1.0s, tag written, temp cleaned). Tests: replace-on-success,
original-survives-save-failure, corrupt-temp-rejected, no-filename-plain-save,
+ a real-FLAC round-trip (skips without ffmpeg). 2443 import/metadata/tag tests
pass.

core/metadata/common.py

@@ -3,6 +3,7 @@
 from __future__ import annotations
 
 import os
+import shutil
 import threading
 import weakref
 from types import SimpleNamespace
@@ -142,13 +143,63 @@ def is_vorbis_like(audio_file: Any, symbols: Any) -> bool:
     return bool(vorbis_classes) and isinstance(audio_file, vorbis_classes) or is_ogg_opus(audio_file)
 
 
-def save_audio_file(audio_file: Any, symbols: Any) -> None:
+def _raw_audio_save(audio_file: Any, symbols: Any, target: Any = None) -> None:
+    """The plain mutagen save with the format-specific kwargs. ``target`` None →
+    save in place (the exact call used before #819, byte-for-byte unchanged); a
+    path → save into that file (the atomic temp copy)."""
     if isinstance(audio_file.tags, symbols.ID3):
-        audio_file.save(v1=0, v2_version=4)
+        audio_file.save(v1=0, v2_version=4) if target is None else audio_file.save(target, v1=0, v2_version=4)
     elif isinstance(audio_file, symbols.FLAC):
-        audio_file.save(deleteid3=True)
+        audio_file.save(deleteid3=True) if target is None else audio_file.save(target, deleteid3=True)
     else:
-        audio_file.save()
+        audio_file.save() if target is None else audio_file.save(target)
+
+
+def save_audio_file(audio_file: Any, symbols: Any) -> None:
+    """Persist mutagen tag changes ATOMICALLY where possible (#819).
+
+    mutagen's in-place ``save()`` rewrites the file; if the process is
+    interrupted or OOM-killed mid-write, the file is left truncated — audio AND
+    tags gone (CubeComming's large hi-res FLACs imported to an empty shell).
+    Instead: copy the original to a temp in the same directory, write the
+    modified tags into that copy, verify it's still a valid audio file, then
+    ``os.replace`` it in atomically. The original is never touched until that
+    final swap, so a crash can only ever orphan the temp — never destroy the
+    user's file.
+
+    Falls back to the plain in-place save if the atomic path can't run (no
+    filename, copy fails, or a format mutagen can't save-to-path) so the file
+    is never left worse off than it is today.
+    """
+    path = getattr(audio_file, "filename", None)
+    try:
+        path = os.fspath(path) if path else None
+    except TypeError:
+        path = None
+    if not path or not os.path.isfile(path):
+        _raw_audio_save(audio_file, symbols)
+        return
+
+    tmp = f"{path}.sstmp"
+    try:
+        shutil.copy2(path, tmp)               # snapshot original (audio + tags)
+        _raw_audio_save(audio_file, symbols, target=tmp)  # write new tags into the copy
+        check = symbols.File(tmp)             # verify it's still real audio
+        if check is None or getattr(getattr(check, "info", None), "length", 0) <= 0:
+            raise ValueError("atomic save produced a file with no audio")
+        os.replace(tmp, path)                 # atomic swap — original safe until here
+    except Exception as atomic_err:
+        # Original untouched (only tmp was written). Clean up + fall back to the
+        # original in-place save so any format/edge the atomic path can't handle
+        # behaves exactly as before.
+        try:
+            if os.path.exists(tmp):
+                os.remove(tmp)
+        except OSError:
+            pass
+        logger.warning("[Atomic Save] atomic path failed (%s) — in-place fallback for %s",
+                       atomic_err, os.path.basename(path))
+        _raw_audio_save(audio_file, symbols)
 
 
 def get_image_dimensions(data: bytes):

core/tag_writer.py

@@ -390,13 +390,12 @@ def write_tags_to_file(file_path: str, db_data: Dict[str, Any],
             if art_ok:
                 written.append('cover_art')
 
-        # Save
-        if isinstance(audio.tags, ID3):
-            audio.save(v1=0, v2_version=4)
-        elif isinstance(audio, FLAC):
-            audio.save(deleteid3=True)
-        else:
-            audio.save()
+        # Save — atomically (#819): write into a temp copy + atomic replace so an
+        # interrupted/OOM-killed save can never truncate the user's file. Same
+        # format kwargs as before, just routed through the shared atomic helper.
+        from types import SimpleNamespace
+        from core.metadata.common import save_audio_file
+        save_audio_file(audio, SimpleNamespace(ID3=ID3, FLAC=FLAC, File=MutagenFile))
 
         return {'success': True, 'written_fields': written}
 

tests/test_atomic_audio_save.py

@@ -0,0 +1,137 @@
+"""Atomic tag saves: an interrupted/OOM-killed save must never destroy the
+user's file (#819 — CubeComming's hi-res FLACs imported to an empty shell).
+
+save_audio_file writes the modified tags into a temp COPY, verifies it's still
+valid audio, then os.replace()s it in — the original is untouched until that
+atomic swap.
+"""
+
+from __future__ import annotations
+
+import os
+import shutil
+import subprocess
+from types import SimpleNamespace
+
+import pytest
+
+from core.metadata.common import save_audio_file
+
+
+def _symbols(file_length):
+    """Symbols whose File() reports the given decoded length (None → invalid)."""
+    return SimpleNamespace(
+        ID3=type("ID3", (), {}), FLAC=type("FLAC", (), {}),
+        File=lambda p: (None if file_length is None
+                        else SimpleNamespace(info=SimpleNamespace(length=file_length))),
+    )
+
+
+def test_atomic_replace_on_success(tmp_path):
+    f = tmp_path / "song.flac"
+    f.write_bytes(b"ORIGINAL-AUDIO")
+    saved_to = []
+
+    class Audio:
+        filename = str(f)
+        tags = None
+
+        def save(self, target=None, **k):
+            saved_to.append(target)
+            # mimic mutagen writing modified tags into the temp copy
+            with open(target, "ab") as h:
+                h.write(b"+TAGS")
+
+    save_audio_file(Audio(), _symbols(180.0))
+
+    assert f.read_bytes() == b"ORIGINAL-AUDIO+TAGS"     # replaced with the tagged copy
+    assert saved_to == [str(f) + ".sstmp"]              # wrote the temp, NOT in place
+    assert not (tmp_path / "song.flac.sstmp").exists()  # temp cleaned up
+
+
+def test_original_survives_save_failure(tmp_path):
+    # The #819 scenario: the save blows up mid-write. The original must be intact.
+    f = tmp_path / "song.flac"
+    f.write_bytes(b"ORIGINAL")
+    inplace = []
+
+    class Audio:
+        filename = str(f)
+        tags = None
+
+        def save(self, target=None, **k):
+            if target is not None:
+                raise OSError("simulated interrupted/OOM save")
+            inplace.append(True)   # fallback in-place save (writes nothing here)
+
+    save_audio_file(Audio(), _symbols(180.0))
+
+    assert f.read_bytes() == b"ORIGINAL"               # never destroyed
+    assert not (tmp_path / "song.flac.sstmp").exists()  # temp removed
+    assert inplace == [True]                            # fell back to in-place
+
+
+def test_corrupt_temp_rejected(tmp_path):
+    # save-to-temp "succeeds" but produces a file with no audio → must NOT
+    # replace the original; fall back instead.
+    f = tmp_path / "song.flac"
+    f.write_bytes(b"ORIGINAL")
+    inplace = []
+
+    class Audio:
+        filename = str(f)
+        tags = None
+
+        def save(self, target=None, **k):
+            if target is None:
+                inplace.append(True)
+
+    save_audio_file(Audio(), _symbols(0))   # File().info.length == 0 → invalid
+
+    assert f.read_bytes() == b"ORIGINAL"
+    assert not (tmp_path / "song.flac.sstmp").exists()
+    assert inplace == [True]
+
+
+def test_no_filename_plain_save():
+    saves = []
+
+    class Audio:
+        filename = None
+        tags = None
+
+        def save(self, target=None, **k):
+            saves.append(target)
+
+    save_audio_file(Audio(), _symbols(180.0))
+    assert saves == [None]   # nothing to be atomic about → plain in-place
+
+
+# ── real mutagen round-trip (only if ffmpeg can make a FLAC) ──
+
+def _make_flac(path):
+    ff = shutil.which("ffmpeg")
+    if not ff:
+        return False
+    r = subprocess.run(
+        [ff, "-f", "lavfi", "-i", "sine=frequency=440:duration=1", "-y", str(path)],
+        capture_output=True)
+    return r.returncode == 0 and os.path.getsize(path) > 0
+
+
+def test_real_flac_atomic_save_preserves_audio(tmp_path):
+    from mutagen.flac import FLAC
+    f = tmp_path / "real.flac"
+    if not _make_flac(f):
+        pytest.skip("ffmpeg unavailable — cannot build a real FLAC")
+    orig_len = FLAC(str(f)).info.length
+
+    audio = FLAC(str(f))
+    audio["title"] = "Atomic Test"
+    save_audio_file(audio, SimpleNamespace(ID3=type("ID3", (), {}), FLAC=FLAC,
+                                           File=__import__("mutagen").File))
+
+    reread = FLAC(str(f))
+    assert reread.info.length == pytest.approx(orig_len, abs=0.05)  # audio intact
+    assert reread["title"] == ["Atomic Test"]                      # tag written
+    assert not (tmp_path / "real.flac.sstmp").exists()