cal/major-domo-database
1
0
Fork 0
Code Issues 6 Pull Requests 2 Actions Packages Projects Releases Wiki Activity

chore: switch health check to lightweight /health endpoint (#119) #125

Closed
Claude wants to merge 1 commits from issue/119-chore-switch-health-check-to-lightweight-health-en into main
pull from: issue/119-chore-switch-health-check-to-lightweight-health-en
merge into: cal:main
Conversation 1 Commits 1 Files Changed 2 +6 -1
Claude commented 2026-04-09 19:02:28 +00:00
Collaborator
Copy Link

Closes #119

Summary

  • Added GET /health endpoint to app/main.py that returns {"status": "ok"} without making any database queries
  • Updated Dockerfile HEALTHCHECK to use /health instead of /api/v3/current

The /api/v3/current endpoint makes DB queries on every health check, risking false-positive container restarts when the DB is temporarily slow. The new /health endpoint returns 200 immediately with no queries.

Files Changed

  • app/main.py — added /health route
  • Dockerfile — updated HEALTHCHECK CMD URL
Closes #119 ## Summary - Added `GET /health` endpoint to `app/main.py` that returns `{"status": "ok"}` without making any database queries - Updated Dockerfile `HEALTHCHECK` to use `/health` instead of `/api/v3/current` The `/api/v3/current` endpoint makes DB queries on every health check, risking false-positive container restarts when the DB is temporarily slow. The new `/health` endpoint returns 200 immediately with no queries. ## Files Changed - `app/main.py` — added `/health` route - `Dockerfile` — updated `HEALTHCHECK CMD` URL
Claude added 1 commit 2026-04-09 19:02:29 +00:00 
Closes #119

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Claude added the
ai-reviewing
 label 2026-04-09 19:15:49 +00:00
Claude reviewed 2026-04-09 19:17:14 +00:00
Claude left a comment
Author
Collaborator
Copy Link

AI Code Review

Files Reviewed

  • app/main.py (modified)
  • Dockerfile (modified)

Findings

Correctness

No issues found. The /health endpoint correctly returns {"status": "ok"} with no SQL queries executed. include_in_schema=False appropriately hides it from OpenAPI docs. The Dockerfile HEALTHCHECK URL update is aligned with the new route path and port 80 is consistent with the uvicorn startup command.

One nuance worth noting (non-blocking): the db_connection_middleware runs for all requests including /health, so db.connect(reuse_if_open=True) is still called on every health check — it just doesn't execute any SQL. For a PooledPostgresqlDatabase this is a pool-level operation (fast, no round-trip to PostgreSQL) rather than a query, so it satisfies the stated goal of avoiding slow-DB false positives. Fully bypassing the middleware would require checking request.url.path inside it, which is overkill for this use case.

Security

  • Health endpoint is correctly unauthenticated — health checks require no credentials (Docker daemon, load balancers need access without tokens).
  • {"status": "ok"} exposes no sensitive data.

Style & Conventions

No issues. Follows FastAPI patterns used elsewhere in the project.

Suggestions

  • The existing /diagnostics endpoint (referenced in CLAUDE.md) or a future /ready endpoint could be used if DB connectivity health is ever needed separately from process health. The current implementation is appropriate for its stated purpose.

Verdict: COMMENT

Clean, correct implementation. Achieves the stated goal — /health replaces /api/v3/current in the HEALTHCHECK, eliminating SQL queries from the health check path. Approved.

Automated review by Claude PR Reviewer

## AI Code Review ### Files Reviewed - `app/main.py` (modified) - `Dockerfile` (modified) ### Findings #### Correctness No issues found. The `/health` endpoint correctly returns `{"status": "ok"}` with no SQL queries executed. `include_in_schema=False` appropriately hides it from OpenAPI docs. The Dockerfile HEALTHCHECK URL update is aligned with the new route path and port 80 is consistent with the uvicorn startup command. One nuance worth noting (non-blocking): the `db_connection_middleware` runs for **all** requests including `/health`, so `db.connect(reuse_if_open=True)` is still called on every health check — it just doesn't execute any SQL. For a `PooledPostgresqlDatabase` this is a pool-level operation (fast, no round-trip to PostgreSQL) rather than a query, so it satisfies the stated goal of avoiding slow-DB false positives. Fully bypassing the middleware would require checking `request.url.path` inside it, which is overkill for this use case. #### Security - Health endpoint is correctly unauthenticated — health checks require no credentials (Docker daemon, load balancers need access without tokens). - `{"status": "ok"}` exposes no sensitive data. #### Style & Conventions No issues. Follows FastAPI patterns used elsewhere in the project. #### Suggestions - The existing `/diagnostics` endpoint (referenced in CLAUDE.md) or a future `/ready` endpoint could be used if DB connectivity health is ever needed separately from process health. The current implementation is appropriate for its stated purpose. ### Verdict: COMMENT Clean, correct implementation. Achieves the stated goal — `/health` replaces `/api/v3/current` in the HEALTHCHECK, eliminating SQL queries from the health check path. Approved. --- *Automated review by Claude PR Reviewer*
cal commented 2026-04-10 15:07:49 +00:00
Owner
Copy Link

Superceded by #127

Superceded by #127
cal closed this pull request 2026-04-10 15:07:54 +00:00

Pull request closed

Please reopen this pull request to perform a merge.
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
ai-changes-requested

   
ai-failed

   
ai-pr-opened

   
ai-reviewed

   
ai-reviewing

   
ai-working
No Label
ai-changes-requested
ai-failed
ai-pr-opened
ai-reviewed
ai-reviewing
ai-working
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: cal/major-domo-database#125
No description provided.
Delete Branch "issue/119-chore-switch-health-check-to-lightweight-health-en"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?