fix(docker): eliminate slow recursive chown in image build and container start#1981
Conversation
|
Warning Review limit reached
Next review available in: 2 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR resolves issue ChangesDocker Build and Startup Performance
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Review SummaryVerdict: minor-fixes-needed Nice change — the Blocking issuesNone. Suggested fixes
Minor / nice-to-have
Compliments
Reviewed via maintainer-review-pr workflow (Pi/Minimax). Aspects run: code-review, error-handling, test-coverage (no-op — no source diff), comment-quality, docs-impact. |
- fix_ownership now captures stderr from find/chown and emits it before exit 1, so a read-only or permission-denied volume is diagnosable instead of only the canned error line (still fails fast) - reword comments that referenced removed code (old chown -Rh semantics, 'same way we do /.archon') to state the invariant directly - trim the Dockerfile layer comment and entrypoint rationale to one-liners; keep the coleam00#1970 issue ref, drop timing snapshots - note why the find predicate uses -o, not -a - split a double-em-dash sentence in the docker deployment doc
603135c to
84f197a
Compare
Summary
Describe this PR in 2-5 bullets:
RUN chown -R appuser:appuser /appat the end of the Dockerfile production stage rewrites (and duplicates, under overlay2) every file in/appincludingnode_modules, causing 50+ minute build stalls on slow disks (chown for appuser:appuser takes too long to finish #1970). Additionally,docker-entrypoint.shblanket-chown -Rhs the entire/.archonand/home/appuservolumes on every container start, which scales with workspace size ("bottleneck for updates").COPY --chown=appuser:appuserand runsbun installasappuser, eliminating the recursive-chown layer. The entrypoint now usesfind … -exec chown -h …to touch only files with wrong ownership instead of every inode. Thebun installcache goes to/tmpand is removed in the same layer, keeping it out of both the image and the persisted home volume./app,/.archon, and/home/appuserstill ends upappuser:appuser. Entrypoint error handling (read-only volumes, incompatible mounts) preserves the sameERROR … exit 1semantics. Stages 1–2 (deps, web-build) anddocker-compose.ymlare untouched. The gosu git-config setup and safe.directory find loop remain unchanged.UX Journey
Before
After
Architecture Diagram
Before
After
Connection inventory (list every module-to-module edge, mark changes):
Label Snapshot
risk: lowsize: Sconfigconfig:dockerChange Metadata
bugmultiLinked Issue
Validation Evidence (required)
Commands and result summary:
bash -n docker-entrypoint.sh→ exit 0 ✅grepconfirms nochown -R appuser:appuser /appremains in Dockerfile ✅grepconfirms exactly oneUSER appuser(line 127) and oneUSER root(line 176) in production stage ✅grepconfirms all COPY lines between USER boundaries carry--chown=appuser:appuser✅fix_ownershipsimulation: root-owned file + dangling symlink → both correctly chowned, symlink not dereferenced ✅findpass finds 0 wrong-owned files ✅find -exec chownwithout permission → exit 1 (triggers ERROR + exit 1) ✅docker version→ not available in dev environmentbun run validatefull pipeline:check:pi-vendor-mapfails with pre-existing "stale vs installed pi-ai SDK" error (unrelated to Docker/entrypoint/docs files).check:bundled,check:bundled-skill, andcheck:bundled-schemaall pass. Type-check has pre-existing Copilot provider type errors. Lint OOMs in the dev environment. None of these are related to this PR's file changes.Security Impact (required)
No)No)No)No)Yes, describe risk and mitigation: N/ACompatibility / Migration
Yes)No)No)Human Verification (required)
What was personally validated beyond CI:
chown -R /app, correct USER ordering, all COPYs annotatedbash -n) passesfix_ownershiplogic simulated with root-owned files, symlinks, and idempotent second pass — all correctchown -htargets the symlink itself (no dereference), matching old-Rhbehaviordocker build/docker compose up— no Docker runtime available in dev environment. Reporter's repro machine can confirm the 50+ min → fast build transition.Side Effects / Blast Radius (required)
/app,/.archon, or/home/appuserwould indicate ownership regression — the entrypoint ERROR message will surface these at container start.Rollback Plan (required)
git revert <merge-commit>+ rebuild/app,/.archon, or/home/appuser.Risks and Mitigations
List real risks in this PR (or write
None).COPY --chown=<name>COPY --chownhas been supported since Docker 17.09 (2017). No BuildKit-only syntax is used. The userappuserexists before the first--chownuse (created at line 117).BUN_INSTALL_CACHE_DIR=/tmp/bun-install-cachecould cause install failures in edge casesBUN_INSTALL_CACHE_DIRis a documented Bun environment variable. The install is--frozen-lockfile, so any failure is deterministic (wrong hash), not corruption. If the cache dir is the issue, dropping only the env var (cache then lands in/home/appuser/.bun) is a simple fallback.USERdirective does not reliably exportHOMEin some Docker versionsHOME=/home/appuseris set explicitly in thebun installRUN command.Summary by CodeRabbit
Bug Fixes
Documentation