datastore: fix session-locked load hang + comprehensive session-lock tests#736
Merged
Conversation
Test Results
39 tested, 39 passed, 0 failed in 1m22s · View logs Deploy Results
1 deployed, 1 passed, 0 failed in 2.5s · View logs |
Quenty
commented
Jul 17, 2026
Quenty
commented
Jul 17, 2026
Quenty
commented
Jul 17, 2026
Quenty
commented
Jul 17, 2026
Quenty
commented
Jul 17, 2026
Quenty
commented
Jul 17, 2026
Quenty
commented
Jul 17, 2026
Quenty
commented
Jul 17, 2026
Quenty
commented
Jul 17, 2026
Quenty
commented
Jul 17, 2026
Quenty
commented
Jul 17, 2026
Quenty
commented
Jul 17, 2026
Quenty
commented
Jul 17, 2026
Quenty
commented
Jul 17, 2026
Quenty
commented
Jul 17, 2026
Quenty
force-pushed
the
users/quenty/datastore-hardening
branch
from
July 17, 2026 18:10
7dd64a7 to
1fc183c
Compare
PromiseRetryUtils.retry gains an optional shouldRetry predicate. 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. Also adds PromiseUtils.race (settles with the first input promise to settle, resolving or rejecting) and a first-class PromiseTestUtils (awaitSettled/awaitValue/awaitOutcome) that awaits by racing a timeout rather than polling with wait(), so a hung promise fails a test instead of freezing the runner. Adds a test place for the promise package with specs covering retry and the new helpers.
DataStoreMock is a strict, 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. Adds a matching in-memory MessagingServiceMock and a SetRobloxMessagingService seam on PlaceMessagingService, so cross-server session messaging can be exercised in-process without touching the real MessagingService.
…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. 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. DataStoreStage now xpcall-isolates saving callbacks: a throwing callback fails the save cleanly with its stack trace preserved, instead of leaking an uncaught error. The fire-and-forget load/save side-effect branches and cancelled-load rejections are also consumed, so destroying a store mid-load no longer surfaces an uncaught exception.
…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. Cross-server messaging runs against an injected MessagingServiceMock rather than the real service. Saveslot: adds a test place 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). Waits go through the shared PromiseTestUtils helpers, throwing assertions check the error message, and load timings are tuned via the new setters so the suite stays fast.
Quenty
force-pushed
the
users/quenty/datastore-hardening
branch
from
July 17, 2026 19:32
1fc183c to
25f48bb
Compare
…ing CLI injection The specs required the running place to have been stamped by the CLI deploy-metadata pass, which only runs on the cloud test/deploy path. The standard Jest CI job runs specs in a plain Roblox VM with no injection, so deployed was false and every spec failed. Stamp a synthetic instance the way the CLI does and assert the reader returns reasonable, well-shaped values (hex commit/version, test target, IDs that round-trip from strings back to numbers), plus the un-stamped Studio case. A separate running-place section still asserts the real deploy stamp, but guards on deployed so it only checks it on the cloud path and stays green in the injection-free CI job.
…o background tasks can't leak across packages The CI test job runs every package's specs in one shared Roblox place, sequentially. A DataStore starts a task.spawn auto-save loop once it loads and only cancels it on :Destroy(), so a spec that leaves a loaded store undestroyed keeps that loop running; when it later fires Save/Sync it throws in whatever package's window happens to be active. That flakily marked @quenty/secrets FAILED even though its own suite passed — the leak came from the datastore specs. DataStoreHookErrors was the one leaking on the passing path: newManager() built a session-locked, loaded PlayerDataStoreManager and the tests only destroyed the serviceBag, never the manager (a standalone object the bag doesn't own), so its store's auto-save loop survived every run. Route every spec's teardown through a file-level Maid cleaned in afterEach, so no store, manager, helper, or serviceBag can outlive a test even when an assertion throws or a promise hangs. Standalone objects use maid:Add (DoCleaning skips an object the test already destroyed, since BaseObject.Destroy nils the metatable); where destroy order matters the maid runs a closure that destroys the child before the serviceBag it borrows PlaceMessagingService from. No assertions, timings, or mocks changed.
…troy() controller pattern Replaces the file-level Maid + afterEach cleanup with the codebase-standard setup()/destroy() controller pattern used by the rogue-properties specs and Hoarcekat stories. Each spec has a local setup() that creates a Maid, maid:Add()s every object it builds (the ServiceBag inline, plus stores, managers, and helpers via small factories), and returns a controller table of named fields/factories with destroy = function() maid:DoCleaning() end. Every test does local controller = setup(), uses the controller, and calls controller:destroy() at the end and before each hung-promise early return. This keeps the same leak fix (nothing a test builds outlives it, so no background task leaks into a later package's window in the shared test place) while matching the standard object-ownership idiom. The local Maid also makes the mid-test :Destroy() tests safe for free — DoCleaning skips an object whose metatable was already nil'd, so there is no double-destroy. No assertions, timeouts, mock behavior, or test descriptions changed. Testing docs updated to document the pattern.
… teardown PlayerDataStoreManager and GameDataStoreService tracked their DataStores outside their maid, so destroying the manager/service (a hot-reloaded ServiceBag, or a unit test) left the stores alive: their auto-save loops kept running and, once the async load resolved, wrote through a now-orphaned scheduler. On Destroy these now fire a best-effort synchronous Save() (the UpdateAsync request is dispatched before Destroy cancels the promise, so a live server usually honors it, though it is not guaranteed) and then tear the store down. PrivateServerDataStoreService already destroyed its store but never flushed first; it now does too. Adds teardown coverage (destroys the inner store, and flushes staged data synchronously) across all four services/managers. Claude-Session: https://claude.ai/code/session_01JgaGXK6J76zChNKPGmmW5b
…ery HasSaveSlots test binds The HasSaveSlots binder spec re-spied PlayerDataStoreManager._toPlayerUserIdOrError in each test's setup() and restored it with jest.restoreAllMocks() in afterEach. In Jest-Lua, jest.spyOn is a no-op the second time it targets a method after a restore: it hands back the stale, already-restored spy without re-patching the object, so only the first test's spy took effect. Every later test ran the real method against the fake Folder, threw "Bad playerOrUserId", and binder:Bind() returned nil -- failing all downstream assertions and emitting uncaught-error noise that fails the package. Monkeypatch the class field directly and restore it in afterEach instead, which re-patches deterministically each setup. All 20 saveslot tests pass. Claude-Session: https://claude.ai/code/session_01JgaGXK6J76zChNKPGmmW5b
Owner
Author
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.
Fixes a session-locked DataStore load that hung forever when the underlying UpdateAsync failed (e.g. a 509 Personal-RCC block): the rejection was swallowed, so the load never settled and everything downstream — save-slot loading, player load — stalled with no error surfaced. Op failures now reject the load with the preserved error and fail fast instead of grinding the ~49s acquire backoff, while genuine lock contention still retries and the stale-lock steal still works. DataStoreStage also isolates a throwing saving callback so it fails the save cleanly with its stack trace, and destroying a store mid-load no longer surfaces an uncaught rejection.
Supporting this, PromiseRetryUtils.retry gains an optional shouldRetry predicate, PromiseUtils gains race, and a shared PromiseTestUtils provides bounded, non-polling awaits for tests. A first-class in-memory DataStoreMock (with failure injection and a block/unblock gate) plus SetRobloxDataStore seams, and a matching MessagingServiceMock plus a SetRobloxMessagingService seam, let the session-locking and cross-server messaging flows be characterized end-to-end without touching real services. Comprehensive datastore, session-lock, and save-slot suites are added, along with a test place for the promise and save-slot packages.