datastore: fix session-locked load hang + comprehensive session-lock tests#736
Open
Quenty wants to merge 4 commits into
Open
datastore: fix session-locked load hang + comprehensive session-lock tests#736Quenty wants to merge 4 commits into
Quenty wants to merge 4 commits into
Conversation
…s.retry Callers can pass `shouldRetry` in RetryOptions. When it returns false after a failure, the retry stops immediately and rejects with that error, so failures that will not recover by retrying can fail fast instead of grinding the backoff. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WJ7WBBpACmLFdAoNhZ5drz
DataStoreMock is an in-memory GlobalDataStore stand-in: deep-copy round-tripping (so aliasing bugs surface like the real service), failure injection including the 509 Personal-RCC block, and a block/unblock gate for exercising in-flight requests. DataStorePromises.isDataStore accepts it anywhere a real datastore Instance is expected. PlayerDataStoreService, GameDataStoreService and PrivateServerDataStoreService gain SetRobloxDataStore(robloxDataStore) so a mock can be injected into a ServiceBag-driven flow for testing. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WJ7WBBpACmLFdAoNhZ5drz
…ocked load A session-locked load previously hung forever when the underlying UpdateAsync failed (e.g. a 509 Personal-RCC block): the rejection was swallowed inside whilePromise, so the load never settled and everything downstream (save-slot loading, player load) hung with no error surfaced. Now a :Catch on the UpdateAsync distinguishes an op failure (UpdateAsync rejects, its transform never runs) from lock contention (UpdateAsync resolves with a cancelled write). Op failures reject the load with the preserved error and, via the new PromiseRetryUtils shouldRetry predicate, fail fast rather than grinding the ~49s acquire backoff (Roblox already retries internally). Lock contention still retries and the stale-lock steal still works. DataStore also gains SetLoadRetryOptions and SetSessionMessagingCloseDelaySeconds to tune the acquire backoff and graceful-close propagation delay. Separately, DataStoreStage now xpcall-isolates saving callbacks: a throwing callback fails the save cleanly with its stack trace preserved, instead of leaking an uncaught error. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WJ7WBBpACmLFdAoNhZ5drz
…save-slot suites Datastore: unit + integration coverage for DataStoreMock, DataStore, DataStoreStage, DataStoreWriter, DataStoreSnapshotUtils, DataStorePromises, DataStoreLockHelper, DataStoreMessageHelper, PlayerDataStoreManager, and the three ServiceBag services. Session locking is characterized end to end with two DataStores over one mock: clean handoff, retry-resolved contention, stale-lock crash recovery, a concurrent load race, the MessagingService graceful-close handshake and production eviction, session-stolen (no duplication), and the read-before-store dupe that motivates locking. Also covers in-flight cancellation / re-entrance, hook-error isolation, and shared-load fan-out. Load timings are tuned via the new setters so the suite stays fast; jest testTimeout raised to 10s. Saveslot: adds a test place (jest.config, deploy target, bootstrap wiring, test deps) and integration tests for SaveSlotService config, the save-slot load flow, and the real HasSaveSlots binder (driven against a mock by binding a fake Folder player and intercepting the single UserId read). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WJ7WBBpACmLFdAoNhZ5drz
Test Results
38 tested, 35 passed, 3 failed in 1m36s · View logs Deploy Results
1 deployed, 1 passed, 0 failed in 3.7s · View logs |
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
1 participant
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.
Problem
A session-locked
DataStoreload hangs forever when the underlyingUpdateAsyncfails (e.g. a509"Data Store operations blocked while running on a Personal RCC"). The rejection is swallowed insidewhilePromiseinDataStore._promiseGetAsyncNoCache, so the load promise never settles. Everything downstream hangs with no error surfaced — most visibly, save-slot loading (HasSaveSlots:PromiseSlotsLoaded()) and player load never complete or report failure.Fix
The key insight: during a session-locked load, lock contention makes
UpdateAsyncresolve (the transform runs and cancels the write by returningnil), whereas an op failure makesUpdateAsyncreject (the transform never runs). So a single:Catchon theUpdateAsyncpromise cleanly distinguishes them:shouldRetrypredicate onPromiseRetryUtils.retry— fail fast instead of grinding the ~49s acquire backoff (Roblox already retries internally; an initial op failure won't recover).Production defaults are unchanged; only the previously-hanging path changes behavior.
Also included
DataStoreStagenowxpcall-isolates saving callbacks: a throwing callback fails the save cleanly with its stack trace preserved, instead of leaking an uncaught error.DataStore:SetLoadRetryOptions(...)and:SetSessionMessagingCloseDelaySeconds(...)(keep the suite fast; production defaults intact).DataStoreMock(in-memory GlobalDataStore stand-in with deep-copy round-tripping, failure injection incl. the 509, and a block/unblock gate) accepted viaDataStorePromises.isDataStore, plusSetRobloxDataStoreseams on the three ServiceBag services for mock-injected integration tests.Tests
Comprehensive new coverage (all green): datastore 267/267, saveslot 20/20.
Session locking is characterized end-to-end with two
DataStores over one mock: clean handoff, retry-resolved contention, stale-lock crash recovery, a concurrent-load race, the MessagingService graceful-close handshake and production eviction, session-stolen (no duplication), and the read-before-store dupe that motivates locking. Plus in-flight cancellation / re-entrance, a not-settling lock command, hook-error isolation, and shared-load fan-out. Save-slot side adds a test place and integration tests including the realHasSaveSlotsbinder (bound to a fake Folder player).Run with
nevermore test --cloudfrom each package.🤖 Generated with Claude Code
https://claude.ai/code/session_01WJ7WBBpACmLFdAoNhZ5drz