fix: engine too many connections - #231
Conversation
Fix create_engine() connection leaks per request/call
Use lru cached engine
rafizamankhan
left a comment
rafizamankhan
left a comment
There was a problem hiding this comment.
@bhtabor Good catch. Caching a shared get_engine() stops per-request create_engine() leaks that can exhaust Postgres connections, and avoiding str(url) as a cache key is right (str(URL) masks the password as ***).
Follow-ups I added: pool_pre_ping (detect/replace dead connections on checkout), explicit pool caps, lifespan/tear_down_db cache clear, and a few core tests so the shared engine stays bounded and shuts down clean.
I dropped lru_cache. cache_clear() forgets entries but can’t dispose() the engines those entries held. Engines own connection pools; treating them like pure memoized values leaves pools open after “clear.”
Kept the URL-keyed dict for the same one-engine-per-URL reuse, with explicit dispose() on shutdown/teardown so it’s defensible for production and tests.
Cheers!
- Route rate_limit.py through the shared get_engine() cache instead of its own module-level singleton, so no second unmanaged pool survives clear_engine_cache() or goes stale when the connection URL changes. - Guard the engine cache with a threading.Lock: sync dependencies run in FastAPI's threadpool, so concurrent first requests could race in get_engine() and orphan an undisposed engine, and clear_engine_cache() could hit a dict-mutated-during-iteration error. - Remove the clear_engine_cache() side effect from tear_down_db(); test fixtures that relied on it now call it explicitly in their teardown. - Make engine-cache tests isolation-safe via an engine_cache fixture so cleanup runs even when an assertion fails mid-test. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
246f969
into
Promptly-Technologies-LLC:main
* chore: release v1.1.8 [skip ci] Automatically generated by python-semantic-release * fix: engine too many connections (#231) * fix: engine too many connections Fix create_engine() connection leaks per request/call * refactor: use lru cached engine Use lru cached engine * add pool_pre_ping and dispose() * fix: address review feedback on engine cache - Route rate_limit.py through the shared get_engine() cache instead of its own module-level singleton, so no second unmanaged pool survives clear_engine_cache() or goes stale when the connection URL changes. - Guard the engine cache with a threading.Lock: sync dependencies run in FastAPI's threadpool, so concurrent first requests could race in get_engine() and orphan an undisposed engine, and clear_engine_cache() could hit a dict-mutated-during-iteration error. - Remove the clear_engine_cache() side effect from tear_down_db(); test fixtures that relied on it now call it explicitly in their teardown. - Make engine-cache tests isolation-safe via an engine_cache fixture so cleanup runs even when an assertion fails mid-test. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Rafiuzzaman Khan <rafiuzzamank@gmail.com> Co-authored-by: chriscarrollsmith <chriscarrollsmith@gmail.com> Co-authored-by: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: github-actions <actions@users.noreply.github.com> Co-authored-by: Biruk Haileye Tabor <biruk.haileye@gmail.com> Co-authored-by: Rafiuzzaman Khan <rafiuzzamank@gmail.com> Co-authored-by: Claude Fable 5 <noreply@anthropic.com> Co-authored-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: Christopher Carroll Smith <chriscarrollsmith@users.noreply.github.com>
Excellent! Makes perfect sense. I was a bit reluctant to add |
3 participants
Fix create_engine() connection leaks per request/call