Use pool_pre_ping in database engine creation #426
Conversation
Trying to solve an issue similar to llamastack/llama-stack#3208
WalkthroughEnables SQLAlchemy connection pool pre-ping by adding pool_pre_ping: True to create_engine_kwargs within initialize_database(), affecting both SQLite and PostgreSQL engine creation. No other logic or public interfaces were changed. Changes
Sequence Diagram(s)sequenceDiagram
participant App as initialize_database()
participant SA as SQLAlchemy Engine
participant Pool as Connection Pool
participant DB as Database
App->>SA: create_engine(..., pool_pre_ping=True)
note right of SA: Engine configured to pre-ping
App->>Pool: acquire connection
Pool->>DB: SELECT 1 (pre-ping)
alt ping ok
DB-->>Pool: ok
Pool-->>App: live connection
else ping fails
DB-->>Pool: failure
Pool->>DB: reconnect and retry
Pool-->>App: fresh connection or raise
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~6 minutes Poem
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/app/database.py (2)
110-113: Optional: limit pre_ping to networked DBs to avoid needless round-trips on SQLitePre-ping adds a round-trip per checkout. It’s typically unnecessary for local SQLite and can be reserved for Postgres (or other networked DBs). If you want to keep it simple, your current approach is fine; otherwise, consider this tweak.
Apply:
@@ - create_engine_kwargs = { - "echo": echo, - "pool_pre_ping": True, - } + create_engine_kwargs = { + "echo": echo, + } @@ - engine = _create_postgres_engine(postgres_config, **create_engine_kwargs) + engine = _create_postgres_engine( + postgres_config, + pool_pre_ping=True, + **create_engine_kwargs, + )Also applies to: 121-123
61-65: Optional: build the Postgres URL via SQLAlchemy’s URL.create to avoid escaping bugsManually composing the DSN risks breakage if usernames/passwords include reserved characters (:, @, /, %). Using URL.create handles proper quoting and keeps things robust.
Apply:
- postgres_url = ( - f"postgresql://{config.user}:{config.password}@" - f"{config.host}:{config.port}/{config.db}" - f"?sslmode={config.ssl_mode}&gssencmode={config.gss_encmode}" - ) + postgres_url = URL.create( + "postgresql", + username=config.user, + password=config.password, + host=config.host, + port=config.port, + database=config.db, + query={"sslmode": config.ssl_mode, "gssencmode": config.gss_encmode}, + )Additionally add this import near your other SQLAlchemy imports:
from sqlalchemy.engine import URL
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/app/database.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-pr
- GitHub Check: e2e_tests
🔇 Additional comments (2)
src/app/database.py (2)
112-112: Pre-ping enabled on the pool — LGTMEnabling pool_pre_ping is a solid, low-risk way to mitigate stale/disconnected connection errors (e.g., with PG/pgBouncer or idle timeout). This should improve resilience without altering call sites.
101-126: SQLAlchemy version pin and create_engine usage verified
- pyproject.toml pins
sqlalchemy>=2.0.42, which fully supportspool_pre_ping.- No other
create_engine()call sites were found outsidesrc/app/database.py.All engines correctly go through
initialize_database().
We want to try out llamastack/llama-stack#3208 and lightspeed-core/lightspeed-stack#426 before they merge
3 participants
Description
Trying to solve an issue similar to llamastack/llama-stack#3208
Type of change
Related Tickets & Documents
llamastack/llama-stack#3208
Checklist before requesting a review
Testing
We're still testing in the process of testing it manually, we're not sure it helps solve our issue, but it also doesn't seem like it can cause much harm
Summary by CodeRabbit