--- title: DB session/engine lifecycle hardening status: draft epic: https://github.com/ccatobs/ops-db-api/issues/158 repos: - ccatobs/ops-db-api --- # PRD 0001: DB session/engine lifecycle hardening > Source of truth for this epic. Tracker: [epic issue](https://github.com/ccatobs/ops-db-api/issues/158). > The epic links here; this file is not embedded in the issue body. ## Problem PR #157 reworked how the API creates and shares database sessions and engines. It fixed two real bugs — a per-startup async-engine leak (the main async session factory was rebuilt on every call) and silent main-site write loss (critical operations on the institute site built transaction steps but never persisted them). A two-reviewer analysis (senior architect + code reviewer) confirmed the PR is a net improvement and mergeable, but surfaced a set of session-lifecycle and connection-pool issues that should be fixed as a follow-up once #157 is on `develop`: - **Connection budget.** Each async engine is sized `pool_size=10 + max_overflow=20 = 30`. On the main site the local and main database URLs resolve to the same PostgreSQL server, so two engines open up to 60 connections to one server — plus the sync `NullPool` churn — against a shared `max_connections = 100` that data-transfer, workflow-manager, and the scheduler also draw on. Under load this can exhaust the server. - **Silent success on the main site.** The new direct-execution decorator path logs an error but still returns the endpoint's success response when the transaction executor or builder is missing — reintroducing the silent write-loss class the PR set out to remove. - **Never-disposed engines.** Engines live as lazy module globals that are never `dispose()`-d. This is the root of event-loop-binding fragility (a cached async engine reused under a second loop raises "Event loop is closed" / "Future attached to a different loop" in tests, `--reload`, or app re-instantiation) and a latent fork hazard if `--workers` is ever added. - **Read/DB divergence window.** On the main site the Redis read buffer is written before the DB commit; a failed commit leaves a phantom record readable until its TTL, and the background processor that would clear it is now gated off on the main site. - **Root cause behind NullPool.** `get_db` is a sync session held for an entire request while async endpoints await long work, so a bounded QueuePool exhausts. `NullPool` is an acceptable band-aid at telescope scale but treats the symptom. ## Solution A sequenced set of vertical slices, each one branch/one PR, all gated on #157 being merged to `develop`: - **Right-size the connection budget** — collapse the main site to a single shared async engine when the local and main URLs are equal, reduce pools to `pool_size=5 / max_overflow=10`, and expose the sizes through Dynaconf so ops can tune per environment. - **Fail loud on the main site** — the direct-execution decorator raises HTTP 500 (as the secondary-site path already does) when it cannot execute, instead of returning a success body. - **Move engine lifecycle into the FastAPI lifespan** — create engines on startup (honoring the sizing and single-engine sharing above), keep them on `app.state` via `async_sessionmaker`, and dispose them on shutdown. This retires the never-disposed globals, the event-loop-binding fragility, and the fork hazard in one move. - **Make the main-site write and read buffer consistent** — commit before the read-buffer write (or invalidate on failure) and report an accurate response status for a direct write. - **Hygiene** — correct two misleading docstrings, move the background-processor verify/log inside the secondary-only branch, document the event-loop-thread invariant on the async getters, normalize id-keying in `get_records_by_ids`, and make dev-token seeding reuse the same cached sync engine. - **Test the paths that startup currently skips** — the suite never enters the app as a context manager, so the lifespan, the pool singletons, the main-site decorator branch, and the secondary-only gating are unexercised. - **Design review for the NullPool root cause** — a human-gated (HITL) slice to decide whether to make the auth/validation reads async (or tightly scope the sync session) and revert `get_db` to a small, fail-fast QueuePool. ## Implementation decisions - **Single async engine per distinct database URL.** On the main site `get_database_url()` and `get_main_database_url()` are equal; the code must detect this and share one engine. Secondary sites keep distinct local + main engines. - **Pool sizing via Dynaconf.** `_ASYNC_POOL_SIZE` / `_ASYNC_POOL_MAX_OVERFLOW` become settings (prefix `CCAT_OPS_DB_API_`) defaulting to `5` / `10`. The worst-case per-process connection count is documented next to the engine factory. PgBouncer (transaction pooling, with asyncpg `statement_cache_size=0`) is noted as a strategic option but out of scope for this epic. - **Engines owned by the lifespan.** `create_async_engine` / `async_sessionmaker` run in the lifespan startup; `app.state` holds the engines and session factories; `dispose()` runs in shutdown. Request dependencies resolve the session factory from `app.state` rather than module globals. `expire_on_commit=False` is preserved. - **Decorator error contract.** The main-site branch raises `HTTPException(500)` on a missing executor or an undiscoverable transaction builder, matching the secondary path. On success it commits before touching the read buffer and returns a status that reflects a direct (not buffered) write. - **No per-request DDL concern.** ops-db's `create_schema` is guarded by `_schema_initialized`, so `create_all` runs at most once per engine per process; `NullPool` does not reintroduce per-request DDL. No work item addresses this. ## Testing decisions Good tests here exercise externally observable startup/lifecycle behavior, not implementation details. The current suite yields a `TestClient` without entering it as a context manager, so the lifespan never runs. Tests should: - drive startup/shutdown (enter the app as a context manager) so the lifespan, engine creation, and disposal actually execute; - assert the main-site decorator executes steps via the executor on success and raises HTTP 500 on a missing executor/builder; - assert `start_background_processing` is not called when `is_secondary_site` is False, and is called on a secondary site; - assert engines are disposed on shutdown; - cover `get_records_by_ids` id-key normalization. Prior art: the existing mocked-session fixtures in `tests/conftest.py` and the `tests/test_smart_query_manager.py` suite that already covers the new batched read helpers. ## Out of scope - The full async-auth migration / dropping `NullPool` for a fail-fast QueuePool is **not** implemented here — it is captured as a human-gated (HITL) design slice because it touches `get_db`, used across ~262 endpoint signatures in 43 files. Implementation follows a separate approved design note/ADR. - PgBouncer / connection-proxy deployment. - Any change to ops-db's engine caching or `create_schema` behavior. - The non-session portions of PR #157 (auth roles, new endpoints, scheduler logic).