fix: wrap postgres health check SQL in sqlalchemy.text() - #805
Open
ApoorvThite wants to merge 7 commits into
Open
fix: wrap postgres health check SQL in sqlalchemy.text()#805ApoorvThite wants to merge 7 commits into
ApoorvThite wants to merge 7 commits into
Conversation
…robe
Add tests/integration/test_health_check.py: calls health_check() with a
working in-memory SQLite AsyncSession and asserts the postgres leg is
still reported unhealthy. This documents the bug — db.execute("SELECT 1")
passes a bare string, which SQLAlchemy 2.x rejects with ArgumentError,
and that exception is swallowed and misreported as the DB being down.
Also adds aiosqlite as a dev dependency so the test can run without a
live Postgres instance, and adds PLAN.md with the fix approach.
SQLAlchemy 2.x rejects bare strings for textual SQL, so
db.execute("SELECT 1") raised ArgumentError inside the health check's
Postgres probe. That exception was swallowed by the surrounding
try/except and misreported as the database being unhealthy, so /health
always failed the Postgres leg regardless of real DB state.
Wraps the query in sqlalchemy.text() so the probe executes correctly.
Fixes ascherj#154
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
ApoorvThite
marked this pull request as ready for review
August 4, 2026 22:50
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
GET /healthalways reported PostgreSQL as unhealthy, even when the database was fully reachable. The probe calleddb.execute("SELECT 1")with a bare Python string, but SQLAlchemy 2.x'sAsyncSession.execute()only accepts anExecutable(e.g.sqlalchemy.text(...)), so the call raisedArgumentErroron every invocation. That exception was swallowed by the surroundingtry/except, which then markeddependencies.postgres(and the overallstatus) as"unhealthy"regardless of the database's real state — a false negative that made the endpoint useless for monitoring. This PR wraps the query insqlalchemy.text("SELECT 1")so the probe actually executes and reports the database's true status.Issue
Closes #154
Changes
api/routes/health.py: importsqlalchemy.textand wrap the Postgres probe's query (db.execute(text("SELECT 1"))) so it's accepted by SQLAlchemy 2.x'sAsyncSession.execute().tests/integration/test_health_check.py: updated the existing reproduction test to assertdependencies.postgres == "healthy"against a live, working session (previously asserted the buggy"unhealthy"result). Added a second test asserting nopostgres_health_check_failedlog event fires, so a future regression back to a bare string is caught even if the string assertion is ever loosened.Testing
make test-unit)make test-integrationfortests/integration/test_health_check.py, run directly via.venv/bin/pytest tests/integration/test_health_check.py -m integration)make lint)make typecheck)Pre-existing failures (not introduced by this change)
This environment doesn't have the Docker/Postgres stack from
make setupavailable, so I verified the fix against an in-memory SQLiteAsyncSession(matching the existing reproduction test's approach) rather than real Postgres.I confirmed the following failures exist on
mainbefore my change and are unaffected by it (checked viagit stashdiffing before/after):make test-unit: 53 pre-existing failures, all unrelated tohealth.py— async-mock setup issues intest_review_service.py(AttributeError: 'coroutine' object has no attribute 'first'), and assertion/fixture issues intest_pii_scrubber.py,test_resume_parser.py,test_readme_parser.py,test_tech_detector.py,test_skill_extractor.py, and others. My change touches none of these modules; 375 tests pass either way.ruff check .: exactly 183 pre-existing errors both before and after my change (verified byte-for-byte identical count viagit stash).mypy api/routes/health.py: exactly 11 pre-existing errors both before and after my change, including one directly relevant to this issue:"Settings" has no attribute "redis_host"/"redis_port"— the Redis leg of this same health check references config fields that don't exist onSettings(core/config.pyonly definesredis_url), so it always raisesAttributeErrorand reportsunhealthy. This means/healthwill still return 503 overall after this PR merges, purely from the Redis leg — that's a separate, out-of-scope bug from Health check DB probe passes a raw SQL string, which fails under SQLAlchemy 2.x #154's title (which is specifically about the Postgres probe's SQL string). I'm planning to file it as a follow-up issue rather than fold it into this PR.Screenshots / Demo
N/A — backend-only change to a JSON health check endpoint. Verified via the integration tests above (both pass, and the second test's log-event assertion is a proxy for what a
curl /healthinspection would show: nopostgres_health_check_failedevent,dependencies.postgres: "healthy").Notes for Reviewers
db.execute("SELECT 1")→db.execute(text("SELECT 1"))); most of this PR's size is the updated/added tests.settings.redis_host/redis_portbug (noted above) be fixed in this PR, or is a separate follow-up issue the right call? I've leaned toward splitting it out since it's unrelated to Health check DB probe passes a raw SQL string, which fails under SQLAlchemy 2.x #154's title and the Postgres fix is independently correct and testable.PLAN.md.