Runs take fifteen to eighteen minutes, and the log shows why: the image is
built twice, in full.
The test stage is built by the runner's docker daemon. The runtime stage was
then built by docker/build-push-action, which runs under a buildx builder that
setup-buildx-action creates in its own container with its own cache. The two
share nothing, so the second build spent seventy-six seconds booting buildkit
and then installed ffmpeg and the package all over again -- around a hundred
and ten seconds for the apk and another hundred for pip, neither of which
produced anything the first build had not already made. The comment above the
test step claimed those layers were shared, which is what made this look
reasonable.
buildx earns that overhead when producing several architectures. This produces
linux/amd64 only, by an explicit decision recorded in the workflow, so it earns
nothing here. Use plain docker build against the same daemon that ran the
tests, and push with docker push. The runtime stage is a strict prefix of the
test stage, so every layer is a cache hit: measured at 1.3 seconds locally.
Identical to the change made in music-curator, whose workflow this one was
copied from.
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.
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.
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.
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.
libmp3lame is single-threaded, so concurrency is one ffmpeg process per file
and the pool width is the whole of it. The width came from os.cpu_count(),
which reports the host's cores and ignores a container's cpus: allowance -- on
a 12-thread host limited to 4 CPUs that is threefold oversubscription, which
costs context switches and NAS responsiveness for no throughput.
The default now reads the cgroup v2 quota, falling back to process CPU
affinity and then to the host count. Each pass logs the number it chose.
Also stops probing every file for embedded cover art. The probe only changes
the command when a cover file sits beside the track, so ask only then; with no
cover file, -map 0:v:0? already carries embedded art if there is any. Worth
roughly 30 ms per track against about 4 s of encoding, so this is tidiness
rather than a speed-up. The per-directory cover lookup is cached alongside.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The login step referenced PACKAGES_SECRET while the secret configured on this
repository -- and on its sibling -- is PACKAGES_TOKEN. The expression resolved
to an empty string and docker/login-action failed with "Password required",
so the first release published nothing and created no tag.
The push trigger only fires on image-affecting paths, which a workflow-only
change is not, so merging this fix alone would not produce a release.
workflow_dispatch can now cut one, restricted to main so a manual run on
another branch cannot tag a commit that is not on the default branch.
Verified by running the version step across every event and ref combination:
push and workflow_dispatch on main release, everything else does not.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Emma Thorpe