Add /health/ready dependency check; fix oversized SECRET_KEY placeholder#3
Add /health/ready dependency check; fix oversized SECRET_KEY placeholder#3sarangs1621 wants to merge 1 commit into
Conversation
/health stays a static liveness probe; /health/ready now checks Postgres (SELECT 1) and Redis (PING) and returns 503 with per-dependency status if either is unreachable, so platforms that gate routing/restarts on a health endpoint can detect a broken DB/Redis connection. The .env.example SECRET_KEY placeholder was 33 chars, which happens to pass the >=32-char startup validation -- a copy-paste deploy that forgot to regenerate it would silently run with a public, known JWT secret instead of failing fast. Shortened it to a value that fails validation until replaced. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR adds a health readiness endpoint ( ChangesHealth Readiness Endpoint and Configuration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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 docstrings
🧪 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.
Actionable comments posted: 1
🤖 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 `@app/main.py`:
- Around line 83-92: The health checks for AsyncSessionLocal (database) and
get_redis_client().ping() (redis) can hang; wrap each awaited operation in
asyncio.wait_for with a short timeout (e.g., 2–5s) and handle
asyncio.TimeoutError to mark checks["database"] or checks["redis"] = "error";
keep existing exception handlers for SQLAlchemyError and RedisError but ensure
timeouts are caught separately to avoid blocking readiness probes.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 86bb6f4f-1fc9-44ee-84c6-72d3336650f6
📒 Files selected for processing (3)
.env.exampleapp/main.pytests/test_security_headers.py
| try: | ||
| async with AsyncSessionLocal() as session: | ||
| await session.execute(text("SELECT 1")) | ||
| except SQLAlchemyError: | ||
| checks["database"] = "error" | ||
|
|
||
| try: | ||
| await get_redis_client().ping() | ||
| except RedisError: | ||
| checks["redis"] = "error" |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Add timeouts to dependency health checks.
Neither the database nor Redis connectivity check has an explicit timeout. If either dependency hangs (network partition, slow query, etc.) rather than immediately refusing the connection, the readiness probe will hang indefinitely. Orchestration systems (Kubernetes, Docker Swarm, etc.) expect health checks to complete within a configured timeout window; a hanging probe can prevent proper failover, scaling, and traffic routing.
Consider wrapping each check in asyncio.wait_for() with a reasonable timeout (e.g., 2-5 seconds).
⏱️ Proposed fix to add timeouts
+import asyncio
+
from fastapi import FastAPI, Request, status
from fastapi.middleware.cors import CORSMiddleware
...
`@app.get`("/health/ready", tags=["health"])
async def readiness_check() -> JSONResponse:
checks = {"database": "ok", "redis": "ok"}
try:
- async with AsyncSessionLocal() as session:
- await session.execute(text("SELECT 1"))
+ async with asyncio.timeout(5.0):
+ async with AsyncSessionLocal() as session:
+ await session.execute(text("SELECT 1"))
- except SQLAlchemyError:
+ except (SQLAlchemyError, TimeoutError):
checks["database"] = "error"
try:
- await get_redis_client().ping()
- except RedisError:
+ async with asyncio.timeout(5.0):
+ await get_redis_client().ping()
+ except (RedisError, TimeoutError):
checks["redis"] = "error"🤖 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 `@app/main.py` around lines 83 - 92, The health checks for AsyncSessionLocal
(database) and get_redis_client().ping() (redis) can hang; wrap each awaited
operation in asyncio.wait_for with a short timeout (e.g., 2–5s) and handle
asyncio.TimeoutError to mark checks["database"] or checks["redis"] = "error";
keep existing exception handlers for SQLAlchemyError and RedisError but ensure
timeouts are caught separately to avoid blocking readiness probes.
1 participant
Summary
GET /health/ready, which checks Postgres (SELECT 1) and Redis (PING) and returns503with per-dependency status if either is unreachable./healthremains a static liveness probe (existing behavior unchanged)..env.example'sSECRET_KEYplaceholder: the old value (change-me-to-a-long-random-secret) is 33 characters, which happens to pass the>=32-char startup validation inapp/core/config.py-- a copy-paste deploy that forgets to regenerate it would silently run with a public, known JWT secret instead of failing at startup. New placeholder (change-me) is intentionally short so the app refuses to start until a real secret is generated.Test plan
tests/test_security_headers.py:/health/readyreturns 200 +{"status":"ok","database":"ok","redis":"ok"}when healthy, and 503 with the relevant dependency marked"error"when Postgres or Redis is unreachable (simulated via monkeypatched broken clients).ruff checkandmypyclean on changed files.Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests