fix(db): use NullPool for SQLite to eliminate silent connection pooling (#347) - #391
Open
santhiprakash wants to merge 3 commits into
Open
fix(db): use NullPool for SQLite to eliminate silent connection pooling (#347)#391santhiprakash wants to merge 3 commits into
santhiprakash wants to merge 3 commits into
Conversation
…ng (OpenHands#347) - Problem: _create_sqlite_engine() claimed "No pooling for SQLite" but passed pool_pre_ping=True without specifying a poolclass, so SQLAlchemy applied its default AsyncAdaptedQueuePool (5 connections, 10 overflow, 30s timeout). Under concurrent fan-out this caused HTTP 500s when the pool exhausted. - Fix: pass poolclass=NullPool to disable SQLAlchemy-level pooling entirely, matching the documented intent. Remove pool_pre_ping (no-op with NullPool). Update the docstring to explain why. - Verification: uv run pytest tests/ -q (1457 passed, 0 failed). Pre-commit clean (ruff format, ruff lint, pycodestyle, pyright).
Author
|
Cross-linking #382 since it touches the same Trade-off summary for whoever reviews first (defaults verified against the pinned SQLAlchemy 2.0.49):
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
_create_sqlite_engine()claimed "No pooling for SQLite - it handles this internally" but passedpool_pre_ping=Truewithout specifying apoolclass. SQLAlchemy therefore applied its defaultAsyncAdaptedQueuePool(5 connections, 10 overflow, 30 s timeout). Under concurrent fan-out (e.g. the Automation view fetching runs for 21 automations), this pool silently exhausted and produced HTTP 500s on 6 of 21 concurrent requests (#347).The
db_pool_size,db_max_overflow, anddb_pool_timeoutconfig values were also silently ignored by the SQLite path.Root cause
create_async_engine()without an explicitpoolclassuses the defaultAsyncAdaptedQueuePoolregardless of the comment claiming otherwise. The misleadingpool_pre_ping=Truereinforced the wrong mental model.Fix
poolclass=NullPooltocreate_async_engine()in_create_sqlite_engine(), which disables SQLAlchemy-level pooling entirely — each request opens and closes its own connection, matching the documented intent.pool_pre_ping=True(no-op withNullPool).NullPoolchoice.test_uses_null_pool) asserting the engine's pool class isNullPool.The N+1 fan-out problem (separate batch-read endpoint) is a separate fix tracked in the same issue; this PR addresses only the pool configuration bug.
Verification
Notes / Risks
NullPoolis the standard SQLAlchemy recommendation for SQLite in multi-threaded/async contexts (connections are lightweight and SQLite handles its own internal locking).AsyncAdaptedQueuePoolwith the configured pool settings.HUMAN: This PR was authored with AI assistance.