fix: entrypoint.sh to not fail on first docker up, improved handling of migrations, signals and errors. #546
Conversation
…al handling, database stability in startup and migration Signed-off-by: soekja <soekja@users.noreply.github.com>
WalkthroughThe Changes
Sequence Diagram(s)sequenceDiagram
participant S as entrypoint.sh
participant DB as Database
participant APP as Application
S->>DB: Execute migration command
alt User already exists error
DB-->>S: Return "user exists" error
S->>S: Print warning, continue processing
else Other errors
DB-->>S: Return error
S->>S: Exit with error message
end
S->>S: Sleep (2 seconds)
S->>APP: Launch Gunicorn with exec (3 workers)
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
entrypoint.sh (2)
21-22: Startup Delay to Ensure Database ReadinessThe addition of a fixed sleep delay helps avoid race conditions by giving the database extra time to start up.
Suggestion: For enhanced robustness, consider replacing the fixed delay with a loop that continuously checks for database availability (e.g., using tools like
ncorwait-for-it), especially if startup timing can vary.
32-34: Production Environment Startup UsingexecFor environments other than development, the script appropriately starts Gunicorn using
execwith production logging settings. This change supports proper signal forwarding and process management.Note: The worker count has been reduced to 1. Please verify that this meets your anticipated production load or that horizontal scaling is adequately configured.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
entrypoint.sh(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: lint (ubuntu-latest, 3.11.x)
- GitHub Check: lint (ubuntu-latest, 3.10.x)
- GitHub Check: Build Cognee Backend Docker App Image
- GitHub Check: docker-compose-test
🔇 Additional comments (3)
entrypoint.sh (3)
3-3: Immediate Exit on Error withset -eAdding
set -eensures the script exits as soon as any command fails, which is a best practice for robust shell scripting.
7-17: Enhanced Alembic Migration Error HandlingThe migration block now captures the output from the Alembic upgrade and conditionally handles known error messages (e.g., a default user already exists). This design prevents the script from exiting on expected non-critical errors while ensuring unexpected failures are caught and reported.
Note: Since the error-checking depends on specific substrings, ensure these strings remain consistent with any future changes in the migration tool's output.
25-31: Dev Mode: Conditional Startup with Proper Signal HandlingThis section correctly distinguishes between debug and non-debug modes in a development environment. Using
execto start the Python process (with or without debugpy) ensures that the Gunicorn process directly receives signals, which is essential for Docker container management.
| # Run Alembic migrations with proper error handling | ||
| echo "Running database migrations..." | ||
| MIGRATION_OUTPUT=$(poetry run alembic upgrade head 2>&1) || { | ||
| if [[ $MIGRATION_OUTPUT == *"UserAlreadyExists"* ]] || [[ $MIGRATION_OUTPUT == *"User default_user@example.com already exists"* ]]; then |
There was a problem hiding this comment.
Can you put a comment describing the reasoning behind this check please? So it doesn't confuse us in the future.
There was a problem hiding this comment.
Yes I will make the updates to have the comments.
entrypoint.sh
Outdated
| if [ "$ENVIRONMENT" = "dev" ]; then | ||
| if [ "$DEBUG" = true ]; then | ||
| echo "Waiting for the debugger to attach..." | ||
| exec python -m debugpy --wait-for-client --listen 0.0.0.0:5678 -m gunicorn -w 1 -k uvicorn.workers.UvicornWorker -t 30000 --bind=0.0.0.0:8000 --log-level debug --reload cognee.api.client:app |
There was a problem hiding this comment.
Why do we decrease the number of workers to 1?
There was a problem hiding this comment.
You're right it should be set to 3 workers like the previous script.
…lreadyExists check Signed-off-by: soekja <soekja@users.noreply.github.com>
|
Updated workers count and comments for UserAlreadyExists check as requested @borisarzentar |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
entrypoint.sh (2)
3-3: Use ofset -efor Immediate Exit on Errors
Introducingset -eis a good practice that ensures the script exits immediately if any command fails. As a further improvement, you might consider addingset -o pipefailto catch errors in pipelines as well.
29-30: Fixed Startup Delay for Database Readiness
Adding a 2-second sleep is a pragmatic way to allow the database time to become ready. In the long run, you might consider implementing a loop with a timeout that actively checks database connectivity, which would offer more robustness in varying environments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
entrypoint.sh(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (23)
- GitHub Check: run_notebook_test / test
- GitHub Check: Test on macos-15
- GitHub Check: run_eval_framework_test / test
- GitHub Check: test
- GitHub Check: run_notebook_test / test
- GitHub Check: run_simple_example_test / test
- GitHub Check: run_dynamic_steps_example_test / test
- GitHub Check: run_notebook_test / test
- GitHub Check: Test on macos-15
- GitHub Check: run_notebook_test / test
- GitHub Check: Test on macos-13
- GitHub Check: Test on macos-13
- GitHub Check: Test on macos-13
- GitHub Check: run_multimedia_example_test / test
- GitHub Check: run_networkx_metrics_test / test
- GitHub Check: test
- GitHub Check: Test on ubuntu-22.04
- GitHub Check: windows-latest
- GitHub Check: Test on ubuntu-22.04
- GitHub Check: Test on ubuntu-22.04
- GitHub Check: test
- GitHub Check: docker-compose-test
- GitHub Check: Build Cognee Backend Docker App Image
🔇 Additional comments (3)
entrypoint.sh (3)
7-16: Detailed Documentation for Migration Error Handling
The added comments explaining the rationale behind ignoring non-critical "UserAlreadyExists" errors are very clear. This aligns well with previous feedback requesting additional context.
16-25: Robust Handling of Alembic Migration Errors
Capturing the migration output and conditionally handling known non-critical errors (like a duplicate user) ensures that unexpected migration failures are properly surfaced. The logic is sound and helps prevent false negatives during startup.
32-42: Enhanced Gunicorn Startup and Signal Handling
Switching to usingexecfor launching Gunicorn improves signal handling by replacing the shell process. The conditional logic distinguishes between development (with optional debug mode and live reloading) and production environments effectively. Ensure that production deployments do not inadvertently include development-only flags like--reload.
Description
In it's current form, the entrypoint.sh script will run but fail with exit code 3 on the first docker compose up. Technically, running docker compose up a second time will not throw the same error and the application works fine. The new changes will improve the first time user experience and improve on some other aspects.
Summary of Changes:
1- entrypoint.sh to not fail with exit code 3 on first docker up.
2- Improved error and signal handling with set -e.
3- Improved database migration, verification and error handling. Avoids schema version mismatch and ensures db schema is always in sync with application code.
4- Added exec before Gunicorn commands to ensure proper signal handling.
DCO Affirmation
I affirm that all code in every commit of this pull request conforms to the terms of the Topoteretes Developer Certificate of Origin
Summary by CodeRabbit