fix(docker): the published image crash-loops on a fresh volume; smoke-test that it runs - #9
Merged
Merged
Conversation
…-test that it runs
The image we published an hour ago cannot start. It builds perfectly, then:
PermissionError: [Errno 13] Permission denied: '/app/cache/_roformer-models'
[entrypoint] Server exited and auto-update disabled. Exiting.
...and `restart: unless-stopped` turns that into a crash-loop.
Cause. /app/cache does not exist in the image (no cache/ in the repo, and it is
.dockerignore'd), so `chown -R appuser /app` never touches it. Docker initializes a fresh
named volume from whatever sits at the mountpoint in the image - ownership included - and
with nothing there it creates the directory as root:root. The container runs as appuser
(uid 10001) and dies on its first write. Verified, not inferred: the volume's contents are
owned by 0:0, and `id` in the image reports uid=10001.
This breaks the DOCUMENTED path exactly as hard: `docker compose up -d` from the README
does the same thing. Credit to @mbanks850, whose #5 ("Fix build and permission errors in
Dockerfile") hit this - I misread it as build-only and owe them a correction.
Fix: create /app/cache and chown it BEFORE the VOLUME instruction, so a fresh volume
inherits uid 10001.
A green build certified a broken image, so also: a SMOKE TEST in CI. It starts the
container the way a user does and requires it to serve /health. Two things about it are
deliberate:
* It mounts a FRESH NAMED VOLUME. That is the load-bearing part. Without it the
container writes to the image's own filesystem, the permission bug never fires, and
the test passes on the very bug it exists to catch.
* It does not stop at /health. A 200 there is necessary but not sufficient - the server
only touches the cache once a job arrives, so it can answer /health and still be
broken. The test also writes a probe file into /app/cache AS THE RUNTIME USER, which
is the precise thing that failed.
It fails fast if the container leaves `running`, rather than burning two minutes polling
something a restart policy is bouncing in a loop.
README: pulling the fixed image is NOT enough on its own. Docker sets a volume's ownership
only when it first creates it, so a volume made by the broken image stays root-owned
forever and will keep crash-looping on a perfectly good image. Users must
`docker volume rm feedback-demucs-cache`. Without that note, a fixed image still generates
"still broken" reports.
Signed-off-by: topkoa <topkoa@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe image prepares ChangesContainer runtime validation
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant DockerBuildWorkflow
participant BuiltContainer
participant HealthEndpoint
participant CacheVolume
DockerBuildWorkflow->>BuiltContainer: start image with cache volume
DockerBuildWorkflow->>HealthEndpoint: poll /health
HealthEndpoint-->>DockerBuildWorkflow: readiness response
DockerBuildWorkflow->>CacheVolume: write and delete probe file
DockerBuildWorkflow->>BuiltContainer: clean up container
DockerBuildWorkflow->>CacheVolume: remove test volume
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
.github/workflows/docker-build.yml (2)
97-101: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winConsider passing the tag via env var instead of direct template expansion.
zizmor flags
"${{ steps.tag.outputs.scan }}"insiderun:as a template-injection pattern. Risk here is low (value derives from repo name + commit SHA), but routing it through an env var avoids the anti-pattern generally.🔒 Proposed fix
- name: Smoke test — the container must start and serve /health run: | set -euo pipefail docker volume rm -f smoke-cache >/dev/null 2>&1 || true docker run -d --name smoke \ -e SKIP_WARMUP=true \ -v smoke-cache:/app/cache \ -p 7865:7865 \ - "${{ steps.tag.outputs.scan }}" + "$SCAN_TAG" + env: + SCAN_TAG: ${{ steps.tag.outputs.scan }}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/docker-build.yml around lines 97 - 101, Update the docker run step to pass steps.tag.outputs.scan through an environment variable, then reference that variable in the command instead of directly expanding the GitHub Actions expression in run:. Preserve the existing container options and image tag value.Source: Linters/SAST tools
122-141: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCache-write-probe failure exits without cleanup.
Lines 135-136
exit 1on probe failure, skipping thedocker rm/docker volume rmat 139-140 (unlike the earlier/healthfailure path, which cleans up before exiting). Harmless on ephemeral GH-hosted runners, but the duplicated cleanup logic across two exit paths is easy to consolidate.♻️ Proposed fix using a trap
- name: Smoke test — the container must start and serve /health run: | set -euo pipefail docker volume rm -f smoke-cache >/dev/null 2>&1 || true + cleanup() { + docker rm -f smoke >/dev/null 2>&1 || true + docker volume rm -f smoke-cache >/dev/null 2>&1 || true + } + trap cleanup EXIT docker run -d --name smoke \ ... if [ -z "$ok" ]; then echo "::error::/health never answered. The image builds but does not run." - docker rm -f smoke >/dev/null 2>&1 || true - docker volume rm -f smoke-cache >/dev/null 2>&1 || true exit 1 fi ... docker exec smoke sh -c 'touch /app/cache/.write-probe && rm /app/cache/.write-probe' \ || { echo "::error::/app/cache is not writable by the runtime user"; exit 1; } echo "cache dir is writable by the runtime user" - - docker rm -f smoke >/dev/null 2>&1 || true - docker volume rm -f smoke-cache >/dev/null 2>&1 || true🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/docker-build.yml around lines 122 - 141, Consolidate smoke-container cleanup in the workflow’s smoke-test section so every failure path, including the cache write probe, removes both the smoke container and smoke-cache volume before exiting. Use a shell trap or equivalent centralized cleanup, remove the probe’s direct exit path cleanup duplication, and preserve the existing failure messages and status.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@README.md`:
- Around line 203-205: Update the fenced code block near the PermissionError
example in README.md to include the text language tag, changing the opening
fence to ```text while preserving the error content unchanged.
---
Nitpick comments:
In @.github/workflows/docker-build.yml:
- Around line 97-101: Update the docker run step to pass steps.tag.outputs.scan
through an environment variable, then reference that variable in the command
instead of directly expanding the GitHub Actions expression in run:. Preserve
the existing container options and image tag value.
- Around line 122-141: Consolidate smoke-container cleanup in the workflow’s
smoke-test section so every failure path, including the cache write probe,
removes both the smoke container and smoke-cache volume before exiting. Use a
shell trap or equivalent centralized cleanup, remove the probe’s direct exit
path cleanup duplication, and preserve the existing failure messages and status.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8f4ca020-b6bf-492f-ae75-99040159f6e7
📒 Files selected for processing (3)
.github/workflows/docker-build.ymlDockerfileREADME.md
The PermissionError sample in the volume-permissions warning had no language on its fence. Signed-off-by: topkoa <topkoa@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The image we published today cannot start
It builds perfectly. Then, on first run:
…and
restart: unless-stoppedturns that into a crash-loop. This breaks the documented path too —docker compose up -dstraight from the README does exactly this.Found by actually running it (
docker runthe published image against a fresh named volume), not by reading it.Cause
/app/cachedoesn't exist in the image — there's nocache/in the repo and it's.dockerignored. Sochown -R appuser /appnever touches it.Docker initializes a fresh named volume from whatever sits at that mountpoint in the image, ownership included. With nothing there, it creates the directory as
root:root. The container runs asappuser(uid 10001) and dies on its first write.Verified, not inferred:
Credit to @mbanks850 — PR #5 is titled "Fix build and permission errors in Dockerfile". They hit this. I read that PR as build-only and told them it was worth keeping just for the rename; that was wrong, and I've said so on their PR.
The fix
Create
/app/cacheandchownit before theVOLUMEinstruction, so a fresh volume inherits uid 10001.The real fix: a smoke test
A green build certified an image that cannot run. A build proves the layers assembled; it says nothing about whether the thing starts. So CI now starts the container the way a user does and requires it to serve
/health.Two things about the test are deliberate:
/health. A 200 there is necessary but not sufficient — the server only touches the cache once a job arrives, so it can answer/healthand still be broken. The test also writes a probe file into/app/cacheas the runtime user, which is the precise thing that failed.It also fails fast if the container leaves
running, instead of burning two minutes polling something a restart policy is bouncing in a loop.This PR's own CI run is the proof. If the smoke test passes here, the image genuinely runs.
Docker sets a volume's ownership only when it first creates it. A volume made by the broken image stays root-owned forever and will keep crash-looping on a perfectly good image. Anyone who ran the image today must:
Documented in the README, because without it a fixed image still generates "still broken" reports. The volume only holds cached weights — deleting it costs a re-download and nothing else.
The feedBack stem-splitter plugin detects this exact case (root-owned volume + crash-loop) and tells the user the same thing, since they can't run
docker logsfrom inside the app.Summary by CodeRabbit
Bug Fixes
/health, and can write to the cache directory.Documentation