[tinker] Raise SQLite busy timeout under rollout load - #2124
Conversation
There was a problem hiding this comment.
Code Review
This pull request increases the SQLite busy_timeout from 30 seconds to 300 seconds in skyrl/tinker/db_models.py. Feedback suggests that a 5-minute timeout is excessively high and risks thread pool starvation under heavy write contention. It is recommended to use a more reasonable timeout, such as 60 seconds, combined with application-level retries.
| cursor = dbapi_connection.cursor() | ||
| cursor.execute("PRAGMA journal_mode=WAL") | ||
| cursor.execute("PRAGMA busy_timeout=30000") | ||
| cursor.execute("PRAGMA busy_timeout=300000") |
There was a problem hiding this comment.
Increasing the SQLite busy_timeout to 300 seconds (5 minutes) is extremely high and poses a significant risk of thread pool starvation. Since this application uses aiosqlite (which runs blocking SQLite operations in a thread pool), blocking a database thread for up to 5 minutes under heavy write contention can quickly exhaust the thread pool. This will freeze all other database operations (including reads) and potentially cause health checks or other asynchronous tasks to time out.
Consider using a more reasonable timeout (e.g., 60 seconds) combined with application-level retries with exponential backoff, or optimizing transaction boundaries to keep write locks as short as possible.
| cursor.execute("PRAGMA busy_timeout=300000") | |
| cursor.execute("PRAGMA busy_timeout=60000") |
bash\nuv run --no-sync ruff check skyrl/tinker/db_models.py\n\n\nRuff passed. The focused DB suite could not collect in the shared environment because itstinker.prototest dependency is absent.