[Tinker] Keep SQLite off the forwarded-future hot path

[j316chuck]

Summary

• Keep active EXTERNAL sample futures in memory and batch their terminal SQLite persistence instead of contending on SQLite during admission and completion.
• Serialize SQLite mutation paths for training futures and session heartbeats; cache immutable sampler-checkpoint validation.
• Preserve failure propagation: a terminal-persistence failure reaches waiters and stops the engine safely.

Testing

• uv run --extra dev --extra tinker pytest tests/tinker/test_external_future_store.py tests/tinker/test_future_waiting.py tests/tinker/skyrl_train/test_async_sample_routing.py -q — 18 passed, 4 skipped
• uv run --extra dev pre-commit run --files tests/tinker/test_external_future_store.py — passed

Follow-up

The deterministic test-only contention repro is stacked at j316chuck#17.

---

Note

Medium Risk
Changes async sample routing, future persistence timing, and SQLite write serialization for training/heartbeat paths; incorrect locking or shutdown ordering could cause lost results, 404s, or stuck shutdown, but behavior is covered by new unit and integration tests.

Overview
For non-colocated SkyRL-Train inference forwarding, forwarded asample requests no longer insert or update FutureDB on the admission/completion path. A new ExternalFutureStore holds active EXTERNAL futures in memory (negative **request_id**s), retrieve_future waits on them directly, and a background worker batch-persists terminal rows under the shared db_write_lock.

SQLite contention is reduced elsewhere: db_write_lock serializes mutating paths on SQLite only (heartbeats, forward_backward commits); sampling-session → model lookups are cached; sampler checkpoint validation runs once per checkpoint with delete/validation locking and cache invalidation on delete.

Shutdown is ordered: tracked forwarding tasks finish before closing the inference client and flushing/closing the store; persistence failures propagate to waiters via RuntimeError. SkyRLTrainInferenceForwardingClient completes futures through the store when present instead of writing FutureDB immediately.

^{Reviewed by Cursor Bugbot for commit 6cae365. Bugbot is set up for automated code reviews on this repo. Configure here.}

[gemini-code-assist]

Code Review

This pull request introduces an ExternalFutureStore to keep forwarded sample futures off the database hot path, along with corresponding integrations in the API and forwarding client, and database write locks to serialize SQLite writes. The review feedback suggests defensively using .get(request_id) in ExternalFutureStore.complete to prevent potential KeyError crashes if an entry is missing.

[gemini-code-assist]

To prevent potential KeyError crashes in the background forwarding task (for example, if a task is cancelled or completed after the store has been cleared or restarted), it is safer to use .get(request_id) defensively and handle the case where the entry is missing.

Suggested change

	async def complete(self, request_id: int, result_data: BaseModel, status: RequestStatus) -> None:
	entry = self._entries[request_id]
	async def complete(self, request_id: int, result_data: BaseModel, status: RequestStatus) -> None:
	entry = self._entries.get(request_id)
	if entry is None:
	logger.warning("Attempted to complete non-existent or already removed external future %s", request_id)
	return

[cursor]

Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.

^{Reviewed by Cursor Bugbot for commit 6cae365. Configure here.}

[cursor]

Lock order can fail persistence

Medium Severity

_persist acquires db_write_lock before checking out a connection, while forward_backward and session_heartbeat check out a get_session connection first and then take that same lock. Under concurrent training writes the pool can fill with lock waiters, so persist blocks on a connection it cannot obtain. The worker then treats the checkout timeout as a terminal persistence failure, which surfaces to waiters and fails shutdown.

Additional Locations (2)

• skyrl/tinker/api.py#L975-L982
• skyrl/tinker/api.py#L1177-L1187

 

^{Reviewed by Cursor Bugbot for commit 6cae365. Configure here.}

[cursor]

Negative IDs can yield zero

Medium Severity

Persisting explicit negative request_id values into FutureDB before any positive autoincrement row exists makes SQLite's next implicit INTEGER PRIMARY KEY max(rowid)+1, which is 0. create_future then hits assert future_db.request_id and fails the first training admission.

Additional Locations (1)

• skyrl/tinker/api.py#L448-L450

 

^{Reviewed by Cursor Bugbot for commit 6cae365. Configure here.}