fix: copy through a temporary file so a cut-short copy is not kept

Copies of already-MP3 sources were written straight to their destination while
encodes went via a temporary file and a rename. A copy interrupted by a full
disk, a killed container or an I/O error therefore left a truncated MP3 in the
mirror -- and because shutil.copy2 reproduces the source's mtime along with its
bytes, staleness detection would read that fragment as up to date and never
replace it. The damage is silent and permanent until someone plays the track.

Give copy the same temporary-file-and-rename path encode already uses, so the
destination either has the whole file or has nothing.

README.md

@@ -51,9 +51,25 @@ 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
+Both encodes and copies are written to a temporary file and renamed into place,
-interrupted run cannot leave a truncated MP3 that the next run mistakes for
+so an interrupted run cannot leave a truncated MP3 that the next run mistakes
-finished work. A lock file in the mirror root stops two passes overlapping.
+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
+
+Everything written into the mirror is made group-readable, and its directories
+group-traversable, so the mirror can be read back by whatever serves it. Neither
+writer does that unaided: the temporary file an encode renames into place is
+created `0600` regardless of the umask, and a straight copy of an existing MP3
+inherits the mode of a source file in a library this tool does not own. Only the
+group bits are touched; whether the mirror is world-readable stays with the
+umask, as does the ownership.
+
+Mirror files written before this existed are topped up on the next pass. Their
+mtimes are correct, so nothing else would revisit them — and they are not
+re-encoded, only chmod'ed.
 
 ## Usage
 

music_mirror.py

@@ -76,6 +76,15 @@ MIRROR_SUFFIX = ".mp3"
 # Filesystems disagree about mtime precision; SMB in particular rounds.
 MTIME_TOLERANCE_SECONDS = 2
 
+# The mirror exists to be read back by something else -- an SMB share, another
+# account on the box -- so everything written into it has to be group-readable.
+# Neither writer manages that unaided: tempfile.mkstemp forces 0600 whatever the
+# umask, and shutil.copy2 carries the source file's mode across from a library
+# that may be tighter still. Directories need the execute bit too, or the group
+# cannot enter them to reach the readable files inside.
+GROUP_READ = 0o040
+GROUP_ENTER = 0o050
+
 
 @dataclass
 class Result:
@@ -123,6 +132,13 @@ def is_current(source, mirror):
     return abs(source.stat().st_mtime - mirror.stat().st_mtime) <= MTIME_TOLERANCE_SECONDS
 
 
+def make_group_readable(path):
+    """Add the group-read bit to a mirror file, leaving the rest of the mode alone."""
+    mode = path.stat().st_mode
+    if not mode & GROUP_READ:
+        path.chmod(mode | GROUP_READ)
+
+
 def find_cover(directory):
     """Return an external cover image for a directory, if one is present."""
     for name in COVER_NAMES:
@@ -223,6 +239,9 @@ def encode(source, mirror, quality_args, dry_run):
             lines = completed.stderr.strip().splitlines()
             return Result("failed", source, lines[-1] if lines else "ffmpeg failed")
         os.utime(temporary, (stat.st_atime, stat.st_mtime))
+        # Before the rename, so the file is never visible in the mirror without
+        # the bit.
+        make_group_readable(temporary)
         os.replace(temporary, mirror)
     except Exception as error:  # noqa: BLE001 - reported per file, run continues
         return Result("failed", source, str(error))
@@ -234,16 +253,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(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)
@@ -252,6 +286,14 @@ def copy(source, mirror, dry_run):
 def process(source, mirror, quality_args, dry_run):
     """Bring one source file's mirror entry up to date."""
     if is_current(source, mirror):
+        # A mirror written before this bit was set has a correct mtime, so
+        # nothing else in the pass would ever revisit it. Top it up here
+        # instead: one stat per file, and no chmod at all once it is right.
+        if not dry_run:
+            try:
+                make_group_readable(mirror)
+            except OSError as error:
+                return Result("failed", mirror, str(error))
         return Result("skipped", mirror)
     if source.suffix.lower() in COPY_EXTENSIONS:
         return copy(source, mirror, dry_run)
@@ -432,6 +474,13 @@ def main(argv=None):
     logging.basicConfig(format="%(asctime)s %(levelname)s %(message)s", level=logging.INFO)
     args = build_parser().parse_args(argv)
 
+    # Directories are created with 0o777 masked by the umask, so clear the group
+    # bits from it once here rather than chmod'ing every directory the walk
+    # creates. Files cannot be handled this way -- mkstemp and copy2 both set a
+    # mode outright -- so they get an explicit chmod instead.
+    inherited = os.umask(0o077)
+    os.umask(inherited & ~GROUP_ENTER)
+
     if not args.source or not args.mirror:
         logger.error("both --source and --mirror are required")
         return 2

tests/conftest.py

@@ -19,6 +19,14 @@ def require_ffmpeg():
             pytest.skip(f"{tool} is not on PATH", allow_module_level=True)
 
 
+@pytest.fixture
+def tight_umask():
+    """Run a test under a umask that would otherwise make the mirror private."""
+    previous = os.umask(0o077)
+    yield
+    os.umask(previous)
+
+
 @pytest.fixture
 def make_flac():
     """Return a factory writing a short tagged FLAC file."""

tests/test_music_mirror.py

@@ -1,7 +1,9 @@
 import os
 import shutil
+import stat
 import subprocess
 import time
+from pathlib import Path
 
 import pytest
 
@@ -153,6 +155,105 @@ 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"
+    mirror = tmp_path / "dst"
+    make_flac(source / "a.flac")
+
+    run(source, mirror)
+
+    assert (mirror / "a.mp3").stat().st_mode & stat.S_IRGRP
+
+
+def test_copied_file_is_group_readable(tmp_path, make_flac, tight_umask):
+    """copy2 carries the source's mode across, and the source may be private."""
+    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()
+    (source / "b.mp3").chmod(0o600)
+
+    run(source, mirror)
+
+    assert (mirror / "b.mp3").stat().st_mode & stat.S_IRGRP
+
+
+def test_mirror_directories_are_group_traversable(tmp_path, make_flac, tight_umask):
+    """A readable file is unreachable if the group cannot enter its directory."""
+    source = tmp_path / "src"
+    mirror = tmp_path / "dst"
+    make_flac(source / "Artist" / "Album" / "a.flac")
+
+    run(source, mirror)
+
+    for directory in (mirror, mirror / "Artist", mirror / "Artist" / "Album"):
+        mode = directory.stat().st_mode
+        assert mode & stat.S_IRGRP, directory
+        assert mode & stat.S_IXGRP, directory
+
+
+def test_private_mirror_file_is_repaired_without_re_encoding(tmp_path, make_flac):
+    """A mirror written by an older version has a correct mtime, so nothing
+    else in the pass would revisit it."""
+    source = tmp_path / "src"
+    mirror = tmp_path / "dst"
+    make_flac(source / "a.flac")
+
+    run(source, mirror)
+    output = mirror / "a.mp3"
+    output.chmod(output.stat().st_mode & ~stat.S_IRGRP)
+    before = output.stat().st_mtime_ns
+
+    run(source, mirror)
+
+    assert output.stat().st_mode & stat.S_IRGRP
+    assert output.stat().st_mtime_ns == before
+
+
+def test_dry_run_does_not_change_permissions(tmp_path, make_flac):
+    source = tmp_path / "src"
+    mirror = tmp_path / "dst"
+    make_flac(source / "a.flac")
+
+    run(source, mirror)
+    output = mirror / "a.mp3"
+    output.chmod(output.stat().st_mode & ~stat.S_IRGRP)
+
+    run(source, mirror, "--dry-run")
+
+    assert not output.stat().st_mode & stat.S_IRGRP
+
+
 def test_format_upgrade_replaces_rather_than_duplicating(tmp_path, make_flac):
     """Lidarr replacing an MP3 with a FLAC must not leave two mirror files."""
     source = tmp_path / "src"