fix(tinker): avoid SQLite pool exhaustion under rollout load - #2097
Open
hershg wants to merge 6 commits into
Open
fix(tinker): avoid SQLite pool exhaustion under rollout load#2097hershg wants to merge 6 commits into
hershg wants to merge 6 commits into
Conversation
Signed-off-by: Hersh Godse <hersh@trajectory.ai>
hershg
marked this pull request as ready for review
August 27, 2026 23:18
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces an ExternalFutureStore to keep forwarded sample futures off the database hot path by managing them in memory and persisting them asynchronously in batches. It also implements caching for sampling models and sampler checkpoint validations, and introduces database write serialization locks (particularly for SQLite) to reduce database contention. I have no feedback to provide as there are no review comments to assess.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
Reviewed by Cursor Bugbot for commit 2a507ce. Configure here.
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.

Summary
Why
Tau creates 512 concurrent samples alongside training futures and heartbeats. Stock SkyRL repeatedly validates the same sampler checkpoint and creates/completes each future through a SQLite pool of 5+10 connections. Under the real mix, the pool exhausts and accepted training stalls.
This is a source-level runtime fix. It does not enlarge the pool or add model-, accelerator-, or workload-specific behavior.
Comparison
All arms were pinned to their exact heads and exercised with the same four-wave gate: 512 model-path samples, 512 forward/backward futures, and 32 heartbeats per wave.
27ae52bfQueuePool size 5 overflow 1092c3628d2760b58720db38cf#2059's negative in-memory IDs are useful, but its terminal results are memory-only, its external path returns JSON rather than protobuf, and its forwarding tasks are not drained at shutdown. #2061 removes more SQLite traffic, but the current queue/Unix-socket design adds substantially more code and fails admission, reconnect, cache-invalidation, and shutdown contracts. This PR is the smallest arm that passes the representative workload and preserves durable semantics.
Lifecycle guarantees
Evidence
git diff --check 59d4daed...HEADpassedExact live consumer proof: deployment XID 1048275 cold-deployed image
sha256:efb03cc344ea373f514a78859f4c0b9a91d3f58b187ab2c201ed8699e499ce11from SkyRL source27d94205. That source contains this PR's first four commits unchanged and the final lifecycle commit rebased as747b5bd1; both lifecycle commits have stable patch ID9668ff4d.Training XID 1048276 then completed the full real workload on that exact image:
The same mechanism previously completed ten optimizer steps in XIDs 1048178 and 1048230, using precursor source
abc014a5. XID 1048276 closes the exact-current-patch integration gap. Current head20db38cfadditionally regression-tests that a terminal-persistence failure is still propagated while the background engine is always stopped.Tests
uv run --isolated --frozen --extra dev --extra tinker --extra ray --extra jax \ pytest -q tests/tinker/test_external_future_store.py uv run --no-sync pytest -q tests/tinker -m 'not integration' pre-commit run --all-files --config .pre-commit-config.yamlNote
Medium Risk
Changes async request lifecycle, SQLite write ordering, and shutdown for the hot rollout path; failures surface as persistence errors to waiters rather than silent loss of durable futures.
Overview
Under high concurrent rollout load (many samples plus training futures and heartbeats), the Tinker API was exhausting the small SQLite connection pool because every forwarded sample created and completed futures and repeatedly validated the same sampler checkpoint on the DB hot path.
This PR adds
ExternalFutureStorefor non-colocated SkyRL-Train inference forwarding: samples get in-memory futures with negative IDs,retrieve_futurewaits on the store first, and terminal rows are batched into SQLite only after inference completes—waiters are not released until persistence succeeds.SkyRLTrainInferenceForwardingClientcompletes through the store instead of per-requestFutureDBupdates.Remaining SQLite writes on SQLite are serialized via
db_write_lock(heartbeats,forward_backwardcreation); Postgres keeps parallel writes.asamplecaches sampling-session model resolution and validates sampler checkpoints once per(model_id, checkpoint_id); deletion shares the validation lock and clears that cache.Shutdown now drains tracked forwarding tasks before closing the inference client and future store, then stops the background engine even if external close fails.
Reviewed by Cursor Bugbot for commit 20db38c. Bugbot is set up for automated code reviews on this repo. Configure here.