Merge pull request 'ci: build the image once instead of twice' (#5) from ci/one-build-not-two into main

Reviewed-on: #5

.gitea/workflows/build-and-publish.yaml

@@ -45,7 +45,8 @@ jobs:
 
       # The suite runs inside the image, against the ffmpeg that ships, rather
       # than against whatever the runner happens to provide. A failing test
-      # fails the build. Layers are shared with the push build below.
+      # fails the build. The runtime stage below is built from the same daemon
+      # afterwards, so its layers are already in cache.
       - name: Run the test suite inside the image
         run: docker build --target test -t music-mirror:test .
 
@@ -124,9 +125,6 @@ jobs:
           echo "release=${release}" >> "$GITHUB_OUTPUT"
           echo "Computed bump=${bump}, release=${release}, base=${base}"
 
-      - name: Set up Buildx
-        uses: docker/setup-buildx-action@d7f5e7f509e45cec5c76c4d5afdd7de93d0b3df5 # v4
-
       - name: Log in to the Gitea container registry
         if: github.event_name != 'pull_request'
         uses: docker/login-action@650006c6eb7dba73a995cc03b0b2d7f5ca915bee # v4
@@ -135,21 +133,34 @@ jobs:
           username: ${{ github.repository_owner }}
           password: ${{ secrets.PACKAGES_TOKEN }}
 
-      - name: Build and push
+      # Plain `docker build` rather than buildx. buildx boots its own buildkit
-        uses: docker/build-push-action@f9f3042f7e2789586610d6e8b85c8f03e5195baf # v7
+      # in a container with a cache of its own, so it shared nothing with the
-        with:
+      # test build above and rebuilt the image from the base up -- installing
-          context: .
+      # ffmpeg and the package a second time, for nothing. It earns that cost
-          # Without this the last stage in the Dockerfile -- the test stage --
+      # when building for several platforms; this only ever targets the amd64
-          # would be what gets published.
+      # NAS, so it does not.
-          target: runtime
+      #
-          # The NAS is the only host this runs on. Building arm64 as well would
+      # `--target runtime` is a strict prefix of the test stage, so every layer
-          # mean emulating it under QEMU for no consumer.
+      # is already in the daemon's cache and this resolves in seconds.
-          platforms: linux/amd64
+      - name: Build the runtime image
-          push: ${{ github.event_name != 'pull_request' }}
+        run: |
-          tags: ${{ steps.version.outputs.tags }}
+          set -euo pipefail
-          labels: |
+          tags=()
-            org.opencontainers.image.source=${{ github.server_url }}/${{ github.repository }}
+          while IFS= read -r tag; do
-            org.opencontainers.image.revision=${{ github.sha }}
+            [ -n "$tag" ] && tags+=(-t "$tag")
+          done <<< "${{ steps.version.outputs.tags }}"
+          docker build --target runtime \
+            --label "org.opencontainers.image.source=${GITHUB_SERVER_URL}/${GITHUB_REPOSITORY}" \
+            --label "org.opencontainers.image.revision=${GITHUB_SHA}" \
+            "${tags[@]}" .
+
+      - name: Push
+        if: github.event_name != 'pull_request'
+        run: |
+          set -euo pipefail
+          while IFS= read -r tag; do
+            [ -n "$tag" ] && docker push "$tag"
+          done <<< "${{ steps.version.outputs.tags }}"
 
       # Record the release: write the computed version into pyproject.toml, then
       # commit and tag it, so the packaging metadata always matches the release

README.md

@@ -51,9 +51,30 @@ 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.
+
+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
+re-encoded, only chmod'ed.
 
 ## Usage
 

music_mirror.py

@@ -77,6 +77,20 @@ 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
+
+# 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
 class Result:
@@ -124,6 +138,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 +255,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))
@@ -245,16 +269,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)
@@ -263,6 +302,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 +510,15 @@ 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.
+    inherited = os.umask(0o077)
+    os.umask(inherited & ~DIRECTORY_ACCESS)
+
     if not args.source or not args.mirror:
         logger.error("both --source and --mirror are required")
         return 2

pyproject.toml

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

tests/conftest.py

@@ -19,6 +19,29 @@ 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 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

@@ -1,7 +1,9 @@
 import os
 import shutil
+import stat
 import subprocess
 import time
+from pathlib import Path
 
 import pytest
 
@@ -153,6 +155,126 @@ 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_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."""
+    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"