diff --git a/README.md b/README.md index 92d05a1..dc23bf1 100644 --- a/README.md +++ b/README.md @@ -51,9 +51,11 @@ The mtime is read _before_ encoding rather than after. A file still being written when the pass reaches it would otherwise be stamped with its final mtime while holding truncated audio, and never be revisited. -Encodes are written to a temporary file and renamed into place, so an -interrupted run cannot leave a truncated MP3 that the next run mistakes for -finished work. A lock file in the mirror root stops two passes overlapping. +Both encodes and copies are written to a temporary file and renamed into place, +so an interrupted run cannot leave a truncated MP3 that the next run mistakes +for finished work. Copies need it as much as encodes do: the mtime comes across +with the bytes, so a half-written copy would look current for ever. A lock file +in the mirror root stops two passes overlapping. ### Permissions diff --git a/music_mirror.py b/music_mirror.py index 41ea537..cf8a6b6 100644 --- a/music_mirror.py +++ b/music_mirror.py @@ -264,19 +264,31 @@ def encode(source, mirror, quality_args, dry_run): def copy(source, mirror, dry_run): - """Copy an already-MP3 source into the mirror.""" + """Copy an already-MP3 source into the mirror, atomically.""" if dry_run: logger.info("would copy %s", source) return Result("copied", mirror) mirror.parent.mkdir(parents=True, exist_ok=True) + + # Through a temporary file and a rename, for the same reason encodes go + # that way, and a sharper one: copy2 reproduces the source's mtime as well + # as its bytes, so a copy cut short by a full disk or a killed container + # would leave a truncated MP3 that every later pass reads as current. + handle, temporary = tempfile.mkstemp(dir=mirror.parent, suffix=".mp3.part") + os.close(handle) + temporary = Path(temporary) + try: - shutil.copy2(source, mirror) + shutil.copy2(source, temporary) # copy2 brings the source's mode with it, and the source library is not # ours to have permissions opinions about. - make_group_readable(mirror) + make_group_readable(temporary) + os.replace(temporary, mirror) except OSError as error: return Result("failed", source, str(error)) + finally: + temporary.unlink(missing_ok=True) logger.info("copied %s", source) return Result("copied", mirror) diff --git a/tests/test_music_mirror.py b/tests/test_music_mirror.py index 0964c97..238302e 100644 --- a/tests/test_music_mirror.py +++ b/tests/test_music_mirror.py @@ -3,6 +3,7 @@ import shutil import stat import subprocess import time +from pathlib import Path import pytest @@ -154,6 +155,30 @@ def test_existing_mp3_is_copied_not_re_encoded(tmp_path, make_flac): assert (mirror / "b.mp3").read_bytes() == (source / "b.mp3").read_bytes() +def test_interrupted_copy_leaves_nothing_behind(tmp_path, make_flac, monkeypatch): + """copy2 reproduces the source mtime, so a truncated copy left in the mirror + would be read as current by every later pass.""" + source = tmp_path / "src" + mirror = tmp_path / "dst" + flac = make_flac(source / "a.flac") + subprocess.run( + ["ffmpeg", "-loglevel", "error", "-y", "-i", str(flac), str(source / "b.mp3")], + check=True, + capture_output=True, + ) + flac.unlink() + + def truncated(src, destination, **kwargs): + Path(destination).write_bytes(Path(src).read_bytes()[:64]) + raise OSError("no space left on device") + + monkeypatch.setattr(music_mirror.shutil, "copy2", truncated) + + assert run(source, mirror) == 1 + assert not (mirror / "b.mp3").exists() + assert list(mirror.rglob("*.part")) == [] + + def test_encoded_file_is_group_readable(tmp_path, make_flac, tight_umask): """mkstemp creates 0600 whatever the umask, so the bit has to be added.""" source = tmp_path / "src"