Conversation
📝 WalkthroughWalkthroughThe pull request refactors container user execution by moving user setup from the entrypoint script to the Dockerfile. The Dockerfile now explicitly declares Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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 |
|
![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 (1)
Dockerfile (1)
48-48: Removegosu— it's now unused and adds unnecessary attack surface.The entrypoint no longer calls
gosufor user-switching (theUSER appdirective in the Dockerfile handles that at the container level). Keeping it installed bloats the image and increases the supply-chain attack surface without benefit.♻️ Proposed fix — drop `gosu` from the apt-get install
-RUN apt-get update && apt-get install -y wget openssl ca-certificates gosu && rm -rf /var/lib/apt/lists/* && \ +RUN apt-get update && apt-get install -y wget openssl ca-certificates && rm -rf /var/lib/apt/lists/* && \ groupadd -r app && useradd -r -g app -m -d /home/app app🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Dockerfile` at line 48, Remove the unused package gosu from the Dockerfile's RUN apt-get install command: locate the RUN line that executes "apt-get update && apt-get install -y wget openssl ca-certificates gosu && rm -rf /var/lib/apt/lists/*" and edit it to drop "gosu" from the install list since user-switching is handled by the "USER app" directive; rebuild the image to confirm no references to gosu remain.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/docker-entrypoint.sh`:
- Around line 4-5: The current entrypoint uses sh -c 'npx prisma migrate deploy
&& exec npx tsx server.ts' which wraps the server in a subshell so signals
aren't forwarded; change the script to run the migration first and then replace
the shell with the server using exec at the outer level (i.e., run npx prisma
migrate deploy (check its exit code) and then exec npx tsx server.ts), so the
exec in scripts/docker-entrypoint.sh makes the Node process PID 1 and receives
Docker signals directly.
---
Nitpick comments:
In `@Dockerfile`:
- Line 48: Remove the unused package gosu from the Dockerfile's RUN apt-get
install command: locate the RUN line that executes "apt-get update && apt-get
install -y wget openssl ca-certificates gosu && rm -rf /var/lib/apt/lists/*" and
edit it to drop "gosu" from the install list since user-switching is handled by
the "USER app" directive; rebuild the image to confirm no references to gosu
remain.
| # Run migrations and start app | ||
| sh -c 'npx prisma migrate deploy && exec npx tsx server.ts' |
There was a problem hiding this comment.
exec inside sh -c doesn't forward signals — server won't shut down gracefully.
Running the container process wrapped in a shell script makes the shell script PID 1, and a shell script will not pass along any signals to its child processes. This means SIGTERM, the signal used for graceful shutdown, will be ignored by the actual server process.
With the current code, the process tree at runtime is:
- PID 1: outer
sh(running the entrypoint script) - Child:
sh -csubshell → (afterexec)npx tsx server.ts
Docker sends SIGTERM to PID 1 (the outer sh). The outer sh does not forward this signal to the Node.js server, so the server gets killed with SIGKILL after the stop timeout rather than shutting down gracefully.
If your entrypoint is a shell script, use exec to launch the final process so it becomes PID 1. The fix is to drop the sh -c wrapper entirely and let the outer shell exec into the server:
🔧 Proposed fix — proper `exec` at the outer shell level
-# Run migrations and start app
-sh -c 'npx prisma migrate deploy && exec npx tsx server.ts'
+# Run migrations and start app
+npx prisma migrate deploy
+exec npx tsx server.tsThis makes exec npx tsx server.ts replace the outer sh as PID 1, so the Node.js process receives Docker signals directly.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Run migrations and start app | |
| sh -c 'npx prisma migrate deploy && exec npx tsx server.ts' | |
| # Run migrations and start app | |
| npx prisma migrate deploy | |
| exec npx tsx server.ts |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/docker-entrypoint.sh` around lines 4 - 5, The current entrypoint uses
sh -c 'npx prisma migrate deploy && exec npx tsx server.ts' which wraps the
server in a subshell so signals aren't forwarded; change the script to run the
migration first and then replace the shell with the server using exec at the
outer level (i.e., run npx prisma migrate deploy (check its exit code) and then
exec npx tsx server.ts), so the exec in scripts/docker-entrypoint.sh makes the
Node process PID 1 and receives Docker signals directly.
1 participant
I am proposing those two changes to have a truly rootless docker image, as opposed to the current v7 one that starts the entrypoint as root and switches to the
appuser when starting the server.I changed the Dockerfile to end with
USER app, so that the entrypoint is started as theappuser straight away.The main consequence of this change is that the entrypoint can no longer change ownership of the
/app/databaseand/app/uploadsdirectories before starting the server: those volumes have to be set with appropriate ownership and permissions prior to starting the container. This is likely a matter of taste but I would rather have a rootless image than the convenience of a root entrypoint that does such things having side effects on the host.Anyway, this PR is only a proposal. Feel free to turn it down without justification if you feel like it's not worth it. I can always make my own derived image :)
Note: I could not run any image built off of the tip of the
v7branch but I could successfully test the same changes on thev7.3.6tag. Hopefully that would work on thev7branch once the other issues are resolved.Summary by CodeRabbit