fix: stabilize self-hosted Docker restarts for api/deriver#483
fix: stabilize self-hosted Docker restarts for api/deriver#483DevNewbie1826 wants to merge 1 commit intoplastic-labs:mainfrom
Conversation
WalkthroughThis change updates the Docker Compose configuration to inline the database provisioning command in the API service entrypoint, removing separate volume mounts for the project directory and venv, eliminating the named venv volume declaration, and adding a healthcheck to the deriver service. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested reviewers
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)
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.
🧹 Nitpick comments (3)
docker-compose.yml.example (3)
27-27: Consider using explicit Python path for consistency.The entrypoint uses
/app/.venv/bin/pythonexplicitly, but the healthcheck uses barepython. While this should work due to theENV PATHset in the Dockerfile, using the explicit path would be more consistent and defensive.Suggested fix
- test: ["CMD-SHELL", "python -c 'import src.deriver' || exit 1"] + test: ["CMD-SHELL", "/app/.venv/bin/python -c 'import src.deriver'"]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docker-compose.yml.example` at line 27, Update the healthcheck command to use the same explicit Python interpreter path as the container entrypoint instead of bare "python": replace the test command that runs "python -c 'import src.deriver' || exit 1" so it invokes "/app/.venv/bin/python -c 'import src.deriver' || exit 1" (reference the existing healthcheck test entry and the entrypoint's /app/.venv/bin/python usage to locate where to change).
7-7: The-lflag is unnecessary.The login shell flag (
-l) causes the shell to source profile files (e.g.,/etc/profile,~/.profile), which is unnecessary overhead here. The Dockerfile already setsENV PATH="/app/.venv/bin:$PATH", so the PATH is available without needing a login shell.Suggested simplification
- entrypoint: ["sh", "-lc", "/app/.venv/bin/python scripts/provision_db.py && exec /app/.venv/bin/fastapi run --host 0.0.0.0 src/main.py"] + entrypoint: ["sh", "-c", "/app/.venv/bin/python scripts/provision_db.py && exec /app/.venv/bin/fastapi run --host 0.0.0.0 src/main.py"]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docker-compose.yml.example` at line 7, Remove the unnecessary login-shell flag from the Docker entrypoint: change the entrypoint array that currently uses ["sh", "-lc", "/app/.venv/bin/python scripts/provision_db.py && exec /app/.venv/bin/fastapi run --host 0.0.0.0 src/main.py"] to use only the "-c" flag (i.e., ["sh", "-c", ...]) so the shell does not source login profile files; keep the existing commands (provision_db.py and exec fastapi run) unchanged.
26-31: Healthcheck only verifies import capability, not process liveness.The current healthcheck (
import src.deriver) only confirms the module can be imported—it doesn't verify the actual deriver process is running and responsive. If the running process hangs or crashes while the Python environment remains intact, this check would still pass.Consider a healthcheck that verifies the process is actually operational. For example, if the deriver exposes any metrics endpoint or has a status mechanism, use that. Alternatively, check if the specific process is running:
Option: Check process is running
healthcheck: - test: ["CMD-SHELL", "python -c 'import src.deriver' || exit 1"] + test: ["CMD-SHELL", "pgrep -f 'python -m src.deriver' || exit 1"] interval: 30s timeout: 10s start_period: 5s retries: 3Minor: The
|| exit 1is redundant sinceCMD-SHELLalready uses the command's exit code.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docker-compose.yml.example` around lines 26 - 31, The healthcheck currently only runs "import src.deriver" which verifies importability but not the running deriver process; replace the test under healthcheck to probe the actual service (e.g., curl the deriver's health/metrics HTTP endpoint or run a lightweight process check like pgrep for the deriver process) and remove the redundant "|| exit 1" from the CMD-SHELL invocation; update the healthcheck block (symbols: healthcheck, test, CMD-SHELL, import src.deriver) to use the chosen liveliness check and appropriate interval/timeout values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@docker-compose.yml.example`:
- Line 27: Update the healthcheck command to use the same explicit Python
interpreter path as the container entrypoint instead of bare "python": replace
the test command that runs "python -c 'import src.deriver' || exit 1" so it
invokes "/app/.venv/bin/python -c 'import src.deriver' || exit 1" (reference the
existing healthcheck test entry and the entrypoint's /app/.venv/bin/python usage
to locate where to change).
- Line 7: Remove the unnecessary login-shell flag from the Docker entrypoint:
change the entrypoint array that currently uses ["sh", "-lc",
"/app/.venv/bin/python scripts/provision_db.py && exec /app/.venv/bin/fastapi
run --host 0.0.0.0 src/main.py"] to use only the "-c" flag (i.e., ["sh", "-c",
...]) so the shell does not source login profile files; keep the existing
commands (provision_db.py and exec fastapi run) unchanged.
- Around line 26-31: The healthcheck currently only runs "import src.deriver"
which verifies importability but not the running deriver process; replace the
test under healthcheck to probe the actual service (e.g., curl the deriver's
health/metrics HTTP endpoint or run a lightweight process check like pgrep for
the deriver process) and remove the redundant "|| exit 1" from the CMD-SHELL
invocation; update the healthcheck block (symbols: healthcheck, test, CMD-SHELL,
import src.deriver) to use the chosen liveliness check and appropriate
interval/timeout values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bf638cdd-af8d-46cf-ba24-96d9a78a25dc
📒 Files selected for processing (1)
docker-compose.yml.example
|
Thanks @DevNewbie1826! The deriver healthcheck is a nice touch — if you'd like to contribute that as a small focused PR after #510 merges, we'd welcome it. Closing as covered. |
2 participants
Summary
.:/appandvenv:/app/.venv) from the Docker compose example forapiandderiver/openapi.jsonWhy
The previous compose example could fail after
docker compose restart api deriverwith errors like:ModuleNotFoundError: No module named 'sqlalchemy'The root cause was the dev-oriented runtime mount structure overlaying
/appand/app/.venv, which made dependency resolution unstable across restarts.Verification
I verified all of the following locally:
docker compose up -d --build api deriverdocker compose restart api deriverapibecomes healthy andhttp://localhost:8000/openapi.jsonrespondsderiverbecomes healthy with the new healthcheckhermes honcho statusconnects successfully after restartSummary by CodeRabbit