[tinker] Size the API database pool for rollout bursts - #2123
Conversation
There was a problem hiding this comment.
Code Review
This pull request updates the database engine creation in skyrl/tinker/api.py to configure connection pool parameters (pool_size, max_overflow, and pool_timeout). The reviewer noted that applying these parameters directly can cause a TypeError with in-memory SQLite databases and lead to database lock contention for file-based SQLite. They recommended conditionally applying these parameters only when the database backend is not SQLite.
| app.state.db_engine = create_async_engine( | ||
| db_url, echo=False, pool_size=50, max_overflow=100, pool_timeout=120 | ||
| ) |
There was a problem hiding this comment.
Passing pool_size, max_overflow, and pool_timeout directly to create_async_engine introduces two major issues:
- TypeError with In-Memory SQLite: When using an in-memory SQLite database (common in test suites), SQLAlchemy defaults to using
StaticPool.StaticPooldoes not accept these pool configuration arguments, which will cause aTypeErrorand crash the application on startup. - Database Lock Contention: For file-based SQLite (the default database configuration), a pool size of 50 with 100 overflow (up to 150 connections) is highly discouraged. SQLite only supports a single concurrent writer, and having so many concurrent connections will lead to severe database lock contention (
database is locked/SQLITE_BUSYerrors), even with WAL mode enabled.
To resolve this, we should conditionally apply these pool parameters only when the database backend is not SQLite (e.g., PostgreSQL).
| app.state.db_engine = create_async_engine( | |
| db_url, echo=False, pool_size=50, max_overflow=100, pool_timeout=120 | |
| ) | |
| engine_kwargs = {"echo": False} | |
| if not db_url.startswith("sqlite"): | |
| engine_kwargs.update( | |
| pool_size=50, | |
| max_overflow=100, | |
| pool_timeout=120, | |
| ) | |
| app.state.db_engine = create_async_engine(db_url, **engine_kwargs) |
bash\nuv run --no-sync ruff check skyrl/tinker/api.py\n\n\nRuff passed. The focused DB suite could not collect in the shared environment because itstinker.prototest dependency is absent.Note
Low Risk
Single startup configuration change with no query or auth logic; main operational risk is higher peak DB connection use under burst load.
Overview
Sizes the Tinker API’s async SQLAlchemy pool so short-lived rollout traffic (many concurrent
retrieve_futurewaiters and per-requestAsyncSessioncheckouts) is less likely to exhaust connections or fail while waiting for a slot.On startup in
lifespan,create_async_enginenow usespool_size=50,max_overflow=100, andpool_timeout=120seconds instead of SQLAlchemy’s smaller defaults.Reviewed by Cursor Bugbot for commit cd03551. Bugbot is set up for automated code reviews on this repo. Configure here.