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

@@ -63,14 +63,9 @@ 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.
-
-Directories are handled by clearing the owner and group read/execute bits from
-the process umask, once, at startup. Owner as well as group, because a umask
-carrying `0400` produces directories of mode `0300` — writable and enterable,
-unreadable to the very run that created them. The `other` bits are left where
-the umask puts them: whether the mirror is world-readable is a genuine policy
-question, and so is its ownership.
+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
@@ -99,20 +94,6 @@ music-mirror --source /music --mirror /music-mp3 --subdir "Artist/Album"
 `--subdir` never prunes: a partial pass cannot tell an orphan from a file
 outside its own scope.
 
-### Concurrency
-
-LAME is single-threaded — ffmpeg reports `Threading capabilities: none` for
-`libmp3lame` — so throughput comes entirely from running several encoders at
-once, one process per file. `--jobs` defaults to the CPUs the process may
-actually use, which inside a container means the `cpus:` allowance rather than
-the host's core count. Each pass logs the number it settled on.
-
-As a rough guide, a Zen 3 core encodes about 40–60× realtime at V0 depending on
-clock, so six cores clear roughly 250 hours of audio per hour of wall clock.
-The first full pass is the expensive one; after that only new and changed files
-are touched. Lower `MUSIC_MIRROR_JOBS` if you would rather the NAS stayed
-responsive than finished sooner.
-
 Requires `ffmpeg` and `ffprobe` on `PATH`. The container image provides both.
 
 ## Running it on TrueNAS Scale

music_mirror.py

@@ -16,7 +16,6 @@ makes runs idempotent without a database to keep in step.
 import argparse
 import concurrent.futures
 import fcntl
-import functools
 import logging
 import os
 import re
@@ -84,12 +83,7 @@ MTIME_TOLERANCE_SECONDS = 2
 # 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
-
-# Cleared from the umask so directories this run creates can be listed and
-# entered. Owner as well as group: a umask carrying 0400 -- which is unusual but
-# not ours to assume away -- otherwise produces a mirror tree that not even the
-# process that built it can read back.
-DIRECTORY_ACCESS = 0o550
+GROUP_ENTER = 0o050
 
 
 @dataclass
@@ -145,13 +139,8 @@ def make_group_readable(path):
         path.chmod(mode | GROUP_READ)
 
 
-@functools.lru_cache(maxsize=4096)
 def find_cover(directory):
-    """Return an external cover image for a directory, if one is present.
-
-    Cached because an album's tracks all ask the same question, and the answer
-    costs one stat per candidate name.
-    """
+    """Return an external cover image for a directory, if one is present."""
     for name in COVER_NAMES:
         candidate = directory / name
         if candidate.is_file():
@@ -228,12 +217,7 @@ def encode(source, mirror, quality_args, dry_run):
         return Result("encoded", mirror)
 
     mirror.parent.mkdir(parents=True, exist_ok=True)
-    # Probing costs an ffprobe process per file, so only ask when the answer
-    # can change the command. With no cover file beside the track, `-map
-    # 0:v:0?` carries embedded art if there is any and shrugs if there is not.
-    cover = find_cover(source.parent)
-    if cover is not None and has_embedded_picture(source):
-        cover = None
+    cover = None if has_embedded_picture(source) else find_cover(source.parent)
 
     # Read the source's mtime before encoding, not after. If the file is still
     # being written -- a Lidarr import landing mid-pass -- stamping the mirror
@@ -389,7 +373,6 @@ def run_once(scan_root, source_root, mirror_root, quality_args, jobs, dry_run, d
     computed against; they differ only for a partial pass over one directory.
     """
     started = time.monotonic()
-    logger.info("pass starting with %d concurrent encoders", jobs)
     counts = {"encoded": 0, "copied": 0, "skipped": 0, "failed": 0}
     failures = []
 
@@ -435,25 +418,6 @@ def acquire_lock(mirror_root):
     return handle
 
 
-def default_jobs():
-    """Return the number of CPUs this process may actually use.
-
-    os.cpu_count() reports the host's total, which in a container with a `cpus:`
-    limit means starting several times more encoders than there is CPU to run
-    them. libmp3lame is single-threaded, so one process per available CPU is the
-    whole of the concurrency story.
-    """
-    try:
-        quota, period = Path("/sys/fs/cgroup/cpu.max").read_text().split()
-        if quota != "max":
-            return max(1, round(int(quota) / int(period)))
-    except (OSError, ValueError):
-        pass
-    if hasattr(os, "process_cpu_count"):  # 3.13+, respects CPU affinity
-        return os.process_cpu_count() or 4
-    return os.cpu_count() or 4
-
-
 def build_parser():
     """Return the argument parser. Every option also reads an env var, so the
     container can be configured without a command line."""
@@ -479,8 +443,8 @@ def build_parser():
     parser.add_argument(
         "--jobs",
         type=int,
-        default=int(os.getenv("MUSIC_MIRROR_JOBS", "0")) or default_jobs(),
-        help="concurrent encodes (env MUSIC_MIRROR_JOBS; default: available CPUs)",
+        default=int(os.getenv("MUSIC_MIRROR_JOBS", "0")) or (os.cpu_count() or 4),
+        help="concurrent encodes (env MUSIC_MIRROR_JOBS; default: CPU count)",
     )
     parser.add_argument(
         "--interval",
@@ -510,14 +474,12 @@ 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 bits
-    # that matter from it once here rather than chmod'ing every directory the
-    # walk creates. The `other` bits are left alone, since whether the mirror is
-    # world-readable is a real policy question; owner and group access is not.
-    # Files cannot be handled this way -- mkstemp and copy2 both set a mode
-    # outright, ignoring the umask -- so they get an explicit chmod instead.
+    # 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 & ~DIRECTORY_ACCESS)
+    os.umask(inherited & ~GROUP_ENTER)
 
     if not args.source or not args.mirror:
         logger.error("both --source and --mirror are required")

pyproject.toml

@@ -4,7 +4,7 @@ build-backend = "setuptools.build_meta"
 
 [project]
 name = "music-mirror"
-version = "0.1.2"
+version = "0.1.0"
 description = "Maintain a lossy MP3 mirror of a lossless music library"
 readme = "README.md"
 requires-python = ">=3.11"

tests/conftest.py

@@ -27,21 +27,6 @@ def tight_umask():
     os.umask(previous)
 
 
-@pytest.fixture
-def owner_hostile_umask():
-    """Return a callable applying a umask that masks off the owner's read bit.
-
-    Unusual, but it is what produces a mirror tree of mode 0300 -- writable and
-    enterable, unreadable to the very process that built it. Applied on demand
-    rather than for the whole test, because the source library is built by
-    something else entirely and the same umask would make the test's own
-    fixtures unreadable before the run under test even started.
-    """
-    previous = os.umask(0o022)
-    yield lambda: os.umask(0o477)
-    os.umask(previous)
-
-
 @pytest.fixture
 def make_flac():
     """Return a factory writing a short tagged FLAC file."""

tests/test_music_mirror.py

@@ -222,27 +222,6 @@ def test_mirror_directories_are_group_traversable(tmp_path, make_flac, tight_uma
         assert mode & stat.S_IXGRP, directory
 
 
-def test_mirror_directories_survive_an_owner_hostile_umask(
-    tmp_path, make_flac, owner_hostile_umask
-):
-    """A umask carrying 0400 otherwise builds a tree the run cannot read back."""
-    source = tmp_path / "src"
-    mirror = tmp_path / "dst"
-    make_flac(source / "Artist" / "Album" / "a.flac")
-
-    # Applied only now: the library already exists, and the umask under test is
-    # the one the container starts this run with.
-    owner_hostile_umask()
-    run(source, mirror)
-
-    for directory in (mirror, mirror / "Artist", mirror / "Artist" / "Album"):
-        mode = directory.stat().st_mode
-        assert mode & stat.S_IRUSR, directory
-        assert mode & stat.S_IXUSR, directory
-        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."""