PRD 0001: DB session/engine lifecycle hardening#

Source of truth for this epic. Tracker: epic issue. 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).