diff --git a/core/metadata/common.py b/core/metadata/common.py index ebf6ee5d..407de668 100644 --- a/core/metadata/common.py +++ b/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): diff --git a/core/tag_writer.py b/core/tag_writer.py index 707454ac..e0f0ea88 100644 --- a/core/tag_writer.py +++ b/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} diff --git a/tests/test_atomic_audio_save.py b/tests/test_atomic_audio_save.py new file mode 100644 index 00000000..396dafb8 --- /dev/null +++ b/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()