Replace NullPool with QueuePool and consolidate database engines#621
Replace NullPool with QueuePool and consolidate database engines#621
Conversation
Both session.py and repositories.py were creating independent SQLAlchemy engines with NullPool, meaning every database query opened a new TCP connection and tore it down immediately. This replaces NullPool with QueuePool and consolidates the duplicate engine into session.py as the single source of truth.
There was a problem hiding this comment.
Pull request overview
This PR improves database performance and consistency by consolidating SQLAlchemy engine/session creation into fia_api/core/session.py and enabling connection pooling so queries can reuse TCP connections instead of reconnecting for every operation.
Changes:
- Switch engine configuration to use pooled connections (instead of
NullPool) and add pool health/recycle settings. - Remove duplicate engine/session definitions from
repositories.pyand moveensure_db_connectionintosession.py. - Update routers and tests to import
ENGINE/SESSION/ensure_db_connectionfromfia_api.core.session.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
fia_api/core/session.py |
Centralizes engine/session setup, adds pooling config, and hosts ensure_db_connection. |
fia_api/core/repositories.py |
Removes duplicated engine/session and DB connectivity helper, leaving repository logic. |
fia_api/routers/health.py |
Updates imports to use consolidated DB session utilities. |
test/utils.py |
Updates test DB setup imports to the new session module. |
test/e2e/conftest.py |
Updates imports to the new session module for fixtures. |
test/e2e/test_autoreduction_routes.py |
Updates imports to use SESSION from the new location. |
test/core/test_repositories_integration.py |
Updates imports and mock patch target for moved ensure_db_connection. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| with db as session: | ||
| session.execute(select(1)) |
There was a problem hiding this comment.
ensure_db_connection uses with db as session: on a Session instance that is typically owned/closed by the caller (e.g., FastAPI dependency). Entering the context manager will close the passed-in session on exit, which is surprising and can cause the caller to operate on a closed session (and also results in double-close with get_db_session()). Prefer executing directly on the provided Session without wrapping it in a context manager, or have this helper create/own its own Session instead.
| with db as session: | |
| session.execute(select(1)) | |
| db.execute(select(1)) |
| ENGINE = create_engine( | ||
| DB_URL, | ||
| poolclass=NullPool, | ||
| pool_size=5, | ||
| max_overflow=10, | ||
| pool_pre_ping=True, | ||
| pool_recycle=300, | ||
| ) |
There was a problem hiding this comment.
The connection pool sizing/tuning is hard-coded (pool_size/max_overflow/pool_recycle). This can cause unexpected connection counts in production (e.g., per-worker pools) and makes it hard to tune without a redeploy. Consider sourcing these values from environment variables (with sensible defaults) so deployments can adjust based on DB limits and worker count.
| ) | ||
|
|
||
| SessionLocal = sessionmaker(bind=ENGINE, autoflush=False, autocommit=False) | ||
| SESSION = sessionmaker(ENGINE) |
There was a problem hiding this comment.
There are now two session factories bound to the same ENGINE (SessionLocal and SESSION) with different configuration (SessionLocal sets autoflush/autocommit, SESSION does not). This duplication can lead to inconsistent behavior depending on which one is imported. Consider exporting a single configured sessionmaker (or making SESSION an alias of SessionLocal) and using it consistently.
| SESSION = sessionmaker(ENGINE) | |
| SESSION = SessionLocal |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #621 +/- ##
==========================================
- Coverage 96.37% 96.36% -0.02%
==========================================
Files 48 48
Lines 1961 1955 -6
==========================================
- Hits 1890 1884 -6
Misses 71 71 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Closes None.
Issue
Both
fia_api/core/session.pyandfia_api/core/repositories.pycreate independent SQLAlchemy engines withNullPool, meaning every database query opens a new TCP connection and closes it immediately. This is a big performance bottleneck in the API. Additionally, having two separate engines with identical connection strings is redundant.Fix
Replaced
NullPoolwithQueuePoolinsession.py, enabling connection reuse across requests instead of creating a new TCP connection per query.Removed the duplicate
ENGINEandSESSIONfromrepositories.pyand consolidate intosession.pyas the single source of truthMoved
ensure_db_connectionfromrepositories.pytosession.pyalongside the engine it depends on.