fix: return 503 service_unavailable on database outage - #366
Conversation
register_exception_handlers only registered handlers for RequestValidationError, StarletteHTTPException, and a catch-all Exception that returns 500/internal_error. A database outage raises sqlalchemy.exc.OperationalError, which fell through to that catch-all and was indistinguishable from a genuine endpoint bug, not the kind of signal most retry policies back off on. /readyz already treats the identical condition as not_ready and returns 503 (ReadinessResult.http_status), so the two surfaces disagreed about what a DB outage means. Add a scoped handler for OperationalError that returns 503/service_unavailable with a Retry-After header, matching the readiness endpoint's classification. Fixes smaramwbc#354 Signed-off-by: Amir Fathi <amirfathi.me@gmail.com>
Empirically, the asyncpg dialect never raises OperationalError for the issue's actual reproduction: connect-time failures (DB down, DNS) escape as raw OSError subclasses and a mid-query disconnect surfaces as a bare DBAPIError, so the new exception handler was unreachable in production. Wrap both at the engine level (do_connect + handle_error events) into OperationalError; auth/config errors and SQL bugs keep their types. Adds an engine-level regression test and an end-to-end test that hits the real get_session path against a dead port (no dependency override).
|
Thanks — good catch on the What testing showed (SQLAlchemy 2.0.52 / asyncpg 0.31, the shipped stack): the asyncpg dialect never raises
so the new handler was unreachable in production, and the tests passed because they inject The follow-up commit normalizes those two outage shapes into Verified: full unit suite (964) + integration suite (275) green, ruff clean. Docs companion for the new |
|
Appreciate you tracking that down and pushing the engine level fix, the asyncpg exception mapping wasn't something I tested against a live driver. Glad it's in good shape now. |
POST /v1/resolutions upserts by (subject_id, session_id, tenant_id), and every write after the first returned 500 while persisting the value. The only upsert-by-logical-key on the consumer surface told callers their write had failed, after committing it. `create_resolution` serialises the row it gets back after committing. `session.commit()` expires every instance in the session, so reading a column off it is lazy IO, which raises MissingGreenlet on the async engine and falls into the catch-all handler as 500/internal_error. It only bit the UPDATE path. `upsert_resolution` returns the instance this request just constructed on INSERT, whose attributes are still populated in Python; on UPDATE it returns the row it loaded from the database, which the commit expired. The fix is the pattern POST /v1/episodes already uses for the same reason: commit, then refresh. resolutions.py was the one router that commits and then reads ORM attributes without refreshing — subjects.py reads only plain ints afterwards, and health.py returns 200 on repeated calls. Not a regression from #295/#356: `from_row` reads the same nine fields the inline mapping read, also after the commit. b99596b~1 behaves identically (200, 500, 500). The defect dates to d4fef67. Not covered by #366 either: that maps OperationalError to 503, and MissingGreenlet is an InvalidRequestError. Three regression tests, all failing on main: repeated writes to one key all return 200; the update response carries the new value rather than a stale first-write body; and the upsert still collapses to one row.
2 participants
Root cause
register_exception_handlers(server/core/errors.py) registers handlers forRequestValidationError,StarletteHTTPException, and a catch-allExceptionthat returns
500/internal_error. A database outage raisessqlalchemy.exc.OperationalError, which falls into that catch-all and comesback indistinguishable from a genuine endpoint bug, not the kind of signal
most retry policies back off on.
/readyzalready treats the identicalcondition as
not_readyand returns 503 (ReadinessResult.http_status), sothe two surfaces disagreed about what a DB outage means.
Fix
Added a scoped handler for
OperationalErrorthat returns503/service_unavailablewith aRetry-Afterheader, ahead of thecatch-all in registration order so it takes the more specific match.
Verification
tests/test_db_outage_error_handling.py) fails onmain(500/internal_error) and passes on this branch(
503/service_unavailablewithRetry-After), confirmed both ways in aclean
python:3.11-slimcontainer. A second test pins that unrelatedexceptions still return 500.
pytest tests/test_*.py) green: 960 passed, 5 skipped,against a real Postgres 16 container with migrations applied, matching CI.
ruff check server/ tests/clean.tests/integration/(unrelatedto this file) and behavior under the real
docker-publishimage.Fixes #354