From 100671da998befde0fc5ffc944a8ddf720d9f38f Mon Sep 17 00:00:00 2001 From: Emma Thorpe Date: Mon, 24 Aug 2026 11:26:13 +0100 Subject: [PATCH 1/4] fix: make everything written into the mirror group-readable The mirror is written by one account and read by another -- an SMB share, or whatever else serves it -- but nothing here produced a group-readable file. Encodes go through `tempfile.mkstemp`, which creates 0600 regardless of the umask and keeps that mode through the rename into place, so every encoded track landed unreadable. Copies of existing MP3s inherit the mode of a source file in a library this tool does not own, which may be no better. Add the group-read bit explicitly: to the temporary file before it is renamed, so a mirror file is never visible without it, and to a copy once it has landed. Directories are handled by clearing the group bits from the process umask rather than chmod'ing each one, since a file the group cannot reach is no more useful than one it cannot read. Only the group bits are touched; the world bits and ownership stay with the umask as before. Mirror files written before this are repaired on the next pass. Their mtimes are correct, so no other part of the pass would revisit them, and topping up the mode costs a stat rather than a re-encode. --- music_mirror.py | 37 +++++++++++++++++++ tests/conftest.py | 8 ++++ tests/test_music_mirror.py | 76 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 121 insertions(+) diff --git a/music_mirror.py b/music_mirror.py index 4756609..41ea537 100644 --- a/music_mirror.py +++ b/music_mirror.py @@ -77,6 +77,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: @@ -124,6 +133,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) + + @functools.lru_cache(maxsize=4096) def find_cover(directory): """Return an external cover image for a directory, if one is present. @@ -234,6 +250,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)) @@ -253,6 +272,9 @@ def copy(source, mirror, dry_run): mirror.parent.mkdir(parents=True, exist_ok=True) try: shutil.copy2(source, mirror) + # copy2 brings the source's mode with it, and the source library is not + # ours to have permissions opinions about. + make_group_readable(mirror) except OSError as error: return Result("failed", source, str(error)) @@ -263,6 +285,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) @@ -463,6 +493,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 diff --git a/tests/conftest.py b/tests/conftest.py index 7854130..abcac25 100644 --- a/tests/conftest.py +++ b/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.""" diff --git a/tests/test_music_mirror.py b/tests/test_music_mirror.py index 825dfae..0964c97 100644 --- a/tests/test_music_mirror.py +++ b/tests/test_music_mirror.py @@ -1,5 +1,6 @@ import os import shutil +import stat import subprocess import time @@ -153,6 +154,81 @@ 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_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" From 6e48d94b32653ba5aa0d8458c79dc0c1cc5b7314 Mon Sep 17 00:00:00 2001 From: Emma Thorpe Date: Mon, 24 Aug 2026 11:28:18 +0100 Subject: [PATCH 2/4] docs: describe how the mirror handles permissions Explain why the group bits are set explicitly rather than left to the umask, what is deliberately not touched, and that an existing mirror is repaired in place rather than re-encoded. --- README.md | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/README.md b/README.md index f74904a..92d05a1 100644 --- a/README.md +++ b/README.md @@ -55,6 +55,20 @@ 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. +### 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 ```sh From a1382185a79f96bb9225cc08916f577c62361581 Mon Sep 17 00:00:00 2001 From: Emma Thorpe Date: Mon, 24 Aug 2026 11:33:23 +0100 Subject: [PATCH 3/4] 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 | 8 +++++--- music_mirror.py | 18 +++++++++++++++--- tests/test_music_mirror.py | 25 +++++++++++++++++++++++++ 3 files changed, 45 insertions(+), 6 deletions(-) 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" From e9852e6c866c0b0fe13aeca872796bd206b776d5 Mon Sep 17 00:00:00 2001 From: Emma Thorpe Date: Mon, 24 Aug 2026 13:21:07 +0100 Subject: [PATCH 4/4] fix: keep the mirror readable under a umask that masks the owner's read bit The umask handling added for group access cleared only the group bits and left owner and other to the environment. A container whose umask carries 0400 then produces mirror directories of mode 0300: writable and enterable, unreadable to the very run that created them, and unreadable to anything serving the share. Clear the owner read and execute bits from the umask as well. The `other` bits stay where the environment puts them, because whether the mirror is world-readable is a real policy question; being able to read a directory the process itself just created is not. Files were never exposed to this: mkstemp sets 0600 outright and copy2 takes the source file's mode, both ignoring the umask. --- README.md | 11 ++++++++--- music_mirror.py | 19 +++++++++++++------ tests/conftest.py | 15 +++++++++++++++ tests/test_music_mirror.py | 21 +++++++++++++++++++++ 4 files changed, 57 insertions(+), 9 deletions(-) diff --git a/README.md b/README.md index dc23bf1..aaae683 100644 --- a/README.md +++ b/README.md @@ -63,9 +63,14 @@ 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. +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. 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 diff --git a/music_mirror.py b/music_mirror.py index cf8a6b6..7b4c263 100644 --- a/music_mirror.py +++ b/music_mirror.py @@ -84,7 +84,12 @@ 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 -GROUP_ENTER = 0o050 + +# 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 @dataclass @@ -505,12 +510,14 @@ 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. + # 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. inherited = os.umask(0o077) - os.umask(inherited & ~GROUP_ENTER) + os.umask(inherited & ~DIRECTORY_ACCESS) if not args.source or not args.mirror: logger.error("both --source and --mirror are required") diff --git a/tests/conftest.py b/tests/conftest.py index abcac25..753d029 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -27,6 +27,21 @@ 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.""" diff --git a/tests/test_music_mirror.py b/tests/test_music_mirror.py index 238302e..b609985 100644 --- a/tests/test_music_mirror.py +++ b/tests/test_music_mirror.py @@ -222,6 +222,27 @@ 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."""