ENG-3666 Parallelize warm_async_pool connections with asyncio.gather#8097
Conversation
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
/code-review |
There was a problem hiding this comment.
Code Review — Parallelize async pool warm-up (#8097)
The change is well-motivated and the approach is sound: replacing the sequential connection loop with asyncio.gather(..., return_exceptions=True) correctly avoids paying N × RTT on startup. Partial-failure handling, improved logging, and the parallel close in finally are all nice additions. The test suite is thorough and covers the key cases.
One real issue
conn.close() gather in finally is missing return_exceptions=True — if any close call raises, the exception propagates out of the finally block, where it will either silence an in-flight exception or produce an unhandled error. The fix is one word: return_exceptions=True on line 192, consistent with how the open-gather is written.
Minor / nit
- The outer
except Exceptionis now effectively dead code. Withreturn_exceptions=True, individual coroutine failures are captured as return values and never raised, so there's no realistic path that reaches the catch. It could be removed or replaced with a clearer comment. import asyncio(new) andfrom asyncio import Lock(existing) coexist — minor inconsistency worth consolidating.import pytestin the test file is unused.
Overall this is a clean, focused improvement. The single actionable fix is the missing return_exceptions=True on the close-gather.
🔬 Codegraph: connected (48630 nodes)
💡 Write /code-review in a comment to re-run this review.
| # Release all connections back to the pool | ||
| for conn in connections: | ||
| await conn.close() | ||
| await asyncio.gather(*(conn.close() for conn in connections)) |
There was a problem hiding this comment.
src/fides/api/db/ctl_session.py:192
The asyncio.gather in the finally block is missing return_exceptions=True. If any conn.close() raises, the gather will propagate that exception, which in a finally block will silently replace any exception already in flight (or cause an unhandled error in the normal case). Since closing a connection is best-effort cleanup, this should mirror the approach used for opening:
await asyncio.gather(*(conn.close() for conn in connections), return_exceptions=True)There was a problem hiding this comment.
I don't think closing a connection should fail ? if it does, a loud exception might be better
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8097 +/- ##
==========================================
+ Coverage 84.98% 85.01% +0.03%
==========================================
Files 633 633
Lines 41737 41736 -1
Branches 4886 4886
==========================================
+ Hits 35471 35483 +12
+ Misses 5157 5145 -12
+ Partials 1109 1108 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Ticket ENG-3666
Description Of Changes
Parallelizes the
warm_async_poolfunction to open all database connections concurrently usingasyncio.gatherinstead of sequentially. This reduces pool warm-up time fromN * RTTto ~1 RTT, which significantly improves startup time for deployments with large pool sizes and/or high-latency database connections.Partial connection failures are handled gracefully via
return_exceptions=True— successful connections are still tracked and cleaned up properly.Code Changes
src/fides/api/db/ctl_session.py: Replace sequentialforloop withasyncio.gatherfor concurrent connection opening; also parallelizeclose()calls in thefinallyblocktests/lib/test_ctl_session.py: New test file with unit tests (mocked engine) and an integration test (real async engine against Postgres)Steps to Confirm
tests/lib/test_ctl_session.py— all 4 tests should passN/N connectionson startup with a readonly pool configuredPre-Merge Checklist
CHANGELOG.mdupdated