diff --git a/src/datastore/README.md b/src/datastore/README.md index 29e4a3fcda..dbadc605a6 100644 --- a/src/datastore/README.md +++ b/src/datastore/README.md @@ -18,6 +18,16 @@ This system is a reliable datastore system designed with promises and asyncronio ## Executive overiew This datastore prevents data loss by being explicit about what we're writing to, and only modifying the data that exists there instead of modifying the whole structure. +## Working on this package + +Read this before changing how player data is saved, and before adding any cleanup, teardown, or +flush to that path. It records design intent that cannot live in the code, because the shape is +driven by Roblox shutdown and cross-server session-lock behavior that is not visible from reading it: + +- [`docs/shutdown-and-session-locks.md`](docs/shutdown-and-session-locks.md) — why every save routes + through one function, what `PromiseAllSaves` has to wait on, and why flushing stores on manager + teardown was tried twice and removed. + ## Comparison to other solutions * Not specifically locked to players diff --git a/src/datastore/docs/shutdown-and-session-locks.md b/src/datastore/docs/shutdown-and-session-locks.md new file mode 100644 index 0000000000..6e1a12014a --- /dev/null +++ b/src/datastore/docs/shutdown-and-session-locks.md @@ -0,0 +1,183 @@ +# Shutdown and session locks + +Why [PlayerDataStoreManager] saves the way it does. Written for whoever changes this package next. +Consumers do not need any of it — using the manager correctly requires nothing from this file. + +Read it before adding any cleanup, teardown, or flush to the save path. Two attempts at exactly that +regressed player data in production, in opposite directions, and both looked obviously correct. + +## The engine behavior this rests on + +Two facts about Roblox, neither of them discoverable from this package's code: + +**A closing server fires `PlayerRemoving` for every player still in it**, and holds the shutdown open +for those handlers the same way it does for `BindToClose`. A restart is not a case where players +"never leave" — they all leave, then the server closes. So `PlayerRemoving` is the save path during a +shutdown, not a peacetime-only path that something else has to stand in for. + +**A session lock can only be released by the server holding it.** Nothing gives one server the +authority to unlock another's key. If a server dies still holding a lock, the next server that loads +that key sees a lock that still looks live and takes the graceful route: it messages the holder's +JobId asking it to close, and that JobId is gone, so nothing answers. + +The cost of that is worth knowing precisely, because the obvious knob is the wrong one. Each attempt +dies on the **hardcoded 5s timeout in `DataStoreMessageHelper.PromiseCloseSessionGraceful`**, not on +`SetSessionMessagingCloseDelaySeconds` — that delay sits in the *fulfilled* branch of +`_promiseGetAsyncNoCache` and is never reached when the holder is dead. Six attempts of that is 30s, +and `PromiseRetryUtils` adds five jittered inter-attempt waits totalling ~49s, so a player waits +roughly **80 seconds** before the lock is finally stolen. Turning `SetSessionMessagingCloseDelaySeconds` +down does not shorten it; the lever is that hardcoded 5. + +## The shape + +Everything converges on one function: + +``` +Players.PlayerRemoving ─┐ +SessionStolen ──────────┤ +SessionCloseRequested ──┼──► _removePlayerDataStore(userId) +PromiseSessionLockingFailed ─┤ removing callbacks +RemovePlayerDataStore ──┘ └─► SaveAndCloseSession() -- writes data AND releases the lock + └─► Destroy() -- only after that write settles + +BindToCloseService ────────► PromiseAllSaves() + removes anything left, then WAITS +``` + +`_removePlayerDataStore` is idempotent and order-independent: it returns early if the store is already +gone from `_datastores` or already `_removing`. So on a shutdown, whichever entry point reaches a given +player first performs the complete sequence and the others no-op. There is deliberately **no path that +destroys a store before its save-and-close has been attempted and settled** — note "attempted": the +`Destroy()` sits in a `Finally`, so a save that rejects still tears the store down. + +That `Finally` has a second consequence worth knowing. `Promise.Finally` is `Then(f, f)` and `f` +returns nothing, so the derived promise *fulfills* even when the save rejected. `removalPromise` +therefore never rejects, and `PromiseAllSaves` resolving means every removal **settled**, not that +every write succeeded. It is the right shape for a shutdown — one player's failed write must not +abandon everyone else's — but do not read a resolved close as proof of a successful save. + +`PromiseAllSaves` is a *waiter*, not a saver. `BindToCloseService` yields on it, which is the only +thing keeping the server alive long enough for the writes to land. + +## What `PromiseAllSaves` has to wait on, and why it isn't obvious + +It waits on the in-flight removal chains in `_removingPromises`, not only on `_pendingSaves`. This is +the part that actually loses live-server data, so it is worth following exactly. + +`_pendingSaves` is fed by a `datastore.Saving:Connect` that lives on the per-user maid at +`self._maid._savingConns[userId]`. The last thing `_removePlayerDataStore` does is +`self._maid._savingConns[userId] = nil`, which cleans that maid and **disconnects `Saving`**. And +`DataStore` fires `Saving` at the very *end* of `_doDataSync` — after `PromiseViewUpToDate()` and after +every saving callback has resolved. + +So the two only line up when the whole removal chain runs synchronously. The moment anything in it +yields — an async removing callback, an async saving callback, or a session-locked load still in flight +because the player left seconds after joining — `SaveAndCloseSession` reaches `Saving` *after* the +connection is already gone, and that save can **never** enter `_pendingSaves` at all. It is not a race +that usually goes the right way; it is a permanent miss. + +The old `PromiseAllSaves` then had literally nothing to wait on: `PromiseUtils.all({})` returns an +already-fulfilled promise, `BindToCloseService` stops yielding, and Roblox kills the server mid-write — +leaving the session locked, with nobody able to release it, and the next server paying the ~80 seconds. +Any consumer with an async removing or saving callback hits this, which is most of them. + +Waiting on `_removingPromises` fixes that, because the entry is inserted synchronously and cleared in an +identity-guarded `Finally`, so a fast leave/rejoin cannot clobber the entry the close is waiting on. + +It does not close the hole completely, and the remaining sliver is worth knowing rather than +rediscovering. `_removePlayerDataStore` latches `_removing[userId] = true` *before* it invokes the +removing callbacks, and only inserts into `_removingPromises` *after* that loop returns. A callback that +yields inside the loop — or throws out of it — leaves a window where the removal is latched but untracked, +and a close landing in that window early-returns on the `_removing` guard and again finds nothing to wait +on. A throwing callback makes it permanent: `_removing` stays true, so that store can never be removed. + +This is pre-existing, not introduced by the change that added the wait, and the window is far smaller +than what it replaced: previously the miss lasted the whole removal for *any* non-synchronous chain, +where now it lasts only while a consumer callback is on the stack inside one loop. +`RemovalCallbacks.spec.lua` characterizes both callback behaviours, though neither test drives a close +across the window, so the sliver itself is uncovered. + +Closing it means publishing the tracking entry before any consumer code runs — a pending promise +inserted at latch time and resolved to the real chain afterwards. That is a change to +`_removePlayerDataStore` itself, so it wants its own review, and note it only fixes the yielding case: +after a *throw* the placeholder is never resolved, so the close would hang on it instead of resolving +early. Arguably the better failure, but it needs the callback loop isolated to actually be closed. + +## Rejected: flushing the stores on manager teardown + +`_flushAndDestroyAll` existed for twelve days (added 2026-07-17, removed 2026-07-29) and was removed. It +ran from the manager's Maid and, for every store still in `_datastores`, saved and then destroyed it +synchronously. + +It was added for a real reason — a `DataStore` starts a `task.spawn` auto-save loop once loaded and +only cancels it on `Destroy()`, so a manager torn down without destroying its stores leaks those loops +(which matters in the shared test place, where a leaked loop fires inside a later package's window). + +It was wrong anyway, because **destroying the manager is not a shutdown.** No path in `ServiceBag`, +`BindToCloseService`, or any game in this repo destroys it on close; only a hot reload, a Studio stop, or +a test does. But wherever something did, the teardown got there before `PlayerRemoving`, cleared +`_datastores`, and destroyed the stores — so the removal that would have saved and closed the session +found nothing and returned early. Both flavors failed: + +- With `Save()`, the data was flushed but the lock was never released, handing the next server the + ~80-second ladder. +- With `SaveAndCloseSession()`, the lock was released, but the store was destroyed synchronously + against its own in-flight write, and the session was closed while players were still in the server + and still writing. Everything still holding the store wrote into a destroyed object: + `attempt to call missing method 'GetSubStore'`, and consumers' own "datastore already cleaned up" + guards firing on a loop for the rest of the shutdown window. + +The lesson is narrow and worth stating plainly: **the leak was a test-harness problem and belonged in +the test harness.** `DataStoreTestUtils` now shuts a manager down the way Roblox does before tearing it +down (`promiseSimulatedShutdown`), which removes the stores through the real path and cancels their +loops as a side effect. Production code did not need a second save path, and could not safely have one. + +## Deliberate: a removing callback that never resolves blocks its removal forever + +There is no timeout on the removing callbacks, and that is the intended behavior, not an oversight. A +callback that never settles holds its removal open, so the save never happens and the lock stays held, +and Roblox tears the thread down at the shutdown cap. A timeout here would convert that into a silent +partial save on a cadence nobody chose — better to let the consumer's broken callback take the blame. + +Be honest about the diagnostic, though: nothing here names the callback that hung. By the time the +removal is stuck, the callback has already *returned* (it handed back a promise that never settles), so +its frame is gone; the thread Roblox kills is `BindToCloseService`'s `:Yield()`. If chasing one of these +gets painful, the missing piece is a `task.delay` warn naming the still-pending userIds — not a timeout. + +`PlayerDataStoreManager.RemovalCallbacks.spec.lua` characterizes it under "failure modes" so the +behavior is pinned rather than accidental. + +Since `PromiseAllSaves` now awaits the removal chains, this is also a new source of shutdown latency, +and it is bounded and benign: `PromiseUtils.all` never short-circuits, so one stuck member delays the +close but cannot cancel or skip the others, and every other player's removal was already dispatched and +completes concurrently. Roblox's ~30s cap ends it either way. The likely trigger in practice is not a +hung callback but a contended session-locked load — a player who joined seconds before the shutdown can +hold the close for as long as the acquire ladder runs. + +## Testing this + +One consequence of dropping the flush: the `DataStore` instances are not owned by any maid (only +`_removePlayerDataStore`'s `Finally` destroys them), so **`manager:Destroy()` now destroys no stores.** +A harness that tears down by destroying a ServiceBag therefore leaves every loaded store alive with its +auto-save loop running — and a `PlayerMock` never fires the real `Players.PlayerRemoving`, so nothing +else removes them either. Any spec built that way has to shut down explicitly before it tears down. + +Resist the urge to fix that by re-adding a destroy on the manager's maid. A destroy-only teardown is +worse than the leak: it would leave `_datastores` populated with destroyed stores, and clearing +`_datastores` too would silently drop the save instead. + +Not via a later `PlayerRemoving`, though — `Maid.DoCleaning` disconnects every `RBXScriptConnection` in a +dedicated first pass before it runs any function task, so the manager's `PlayerRemoving` handler is +already gone by then. The reachable case is a removal *already in flight*: a removing callback that +yielded resumes to find the store destroyed under it, and `SaveAndCloseSession()` hits a nil metatable. +The leak is a harness problem; keep the fix in the harness. + +`manager:Destroy()` is not a shutdown and specs must not use it as one — several did, which is how the +teardown flush looked well covered while being wrong. Drive +`DataStoreTestUtils.promiseSimulatedShutdown(manager, userIds)` instead: it fires the removals, then +returns the promise `BindToCloseService` would yield on. + +The assertion that matters is not "the data was saved" but "the data was saved *by the time the close +resolved*" — that is the difference between a server that shuts down cleanly and one that dies holding +a lock. `PlayerDataStoreManager.spec.lua`'s "does not resolve the close while a PlayerRemoving save is +still in flight" pins it, using an async removing callback to open the window. diff --git a/src/datastore/src/Server/DataStore.GracefulClose.spec.lua b/src/datastore/src/Server/DataStore.GracefulClose.spec.lua index e29fd387c9..e9f7c55acb 100644 --- a/src/datastore/src/Server/DataStore.GracefulClose.spec.lua +++ b/src/datastore/src/Server/DataStore.GracefulClose.spec.lua @@ -22,12 +22,11 @@ local describe = Jest.Globals.describe local expect = Jest.Globals.expect local it = Jest.Globals.it -describe("clean session end releases the lock (server-shutdown teardown path)", function() - it("manager teardown closes the session so the next server loads immediately", function() +describe("clean session end releases the lock (server-shutdown path)", function() + it("a closing server releases the session so the next server loads immediately", function() local mock = DataStoreMock.new() - -- Server A: the manager still owns the store at teardown (nothing removed the player - -- first), which is exactly what a ServiceBag destroy after a clean session looks like. + -- Server A: a player in the server with a live session, about to be shut down. local maidA = Maid.new() local serviceBagA = DataStoreTestUtils.newServiceBag(maidA, MessagingServiceMock.new()) local managerA = maidA:Add(PlayerDataStoreManager.new(serviceBagA, mock :: any, function(userId) @@ -47,14 +46,17 @@ describe("clean session end releases the lock (server-shutdown teardown path)", return end - -- Clean shutdown: the whole session tears down without the player ever "removing". + -- Clean shutdown: Roblox fires PlayerRemoving for the player still in the server, and holds the + -- close open until that removal has flushed. + if not PromiseTestUtils.awaitSettled(DataStoreTestUtils.promiseSimulatedShutdown(managerA, { 1 }), 10) then + expect("A's shutdown never flushed").toEqual("A's shutdown flushed") + maidA:DoCleaning() + return + end maidA:DoCleaning() - -- The teardown flush must write the graceful close: data saved, lock released. - expect(PromiseTestUtils.awaitValue(function() - local raw = mock:GetRaw("user_1") - return raw ~= nil and raw.lock == nil - end, 5)).toEqual(true) + -- The close must have written the graceful release: data saved, lock gone. + expect(mock:GetRaw("user_1").lock).toEqual(nil) expect(mock:GetRaw("user_1").coins).toEqual(42) -- Server B: an immediate clean takeover -- bounded well under the 5s graceful-close diff --git a/src/datastore/src/Server/DataStoreTestUtils.lua b/src/datastore/src/Server/DataStoreTestUtils.lua index 749e849cad..1fb7417ca1 100644 --- a/src/datastore/src/Server/DataStoreTestUtils.lua +++ b/src/datastore/src/Server/DataStoreTestUtils.lua @@ -102,14 +102,83 @@ function DataStoreTestUtils.setup() } end +--[=[ + Simulates what a real Roblox server does when it shuts down, which is the only accurate model for + the save path: Roblox fires PlayerRemoving for every player still in the server, giving those + handlers the same "hold the shutdown open for me" treatment BindToClose gets. So the removals are + what save and close each session; the close callback's job is only to wait for them to flush. + + Destroying the manager is NOT a shutdown and never has been -- nothing in a live server destroys it. + + @param manager PlayerDataStoreManager + @param userIds { PlayerUserId }? -- players still in the server when it began closing + @return Promise -- what BindToCloseService yields on, so the server cannot die until it settles +]=] +function DataStoreTestUtils.promiseSimulatedShutdown(manager, userIds) + for _, userId in userIds or {} do + manager:RemovePlayerDataStore(userId) + end + + return manager:PromiseAllSaves() +end + +--[=[ + Shuts down the manager a [PlayerDataStoreService] owns, the way Roblox would, and waits for it. + + Call this from the `destroy()` of any spec that injects a datastore into the service and tears down + by destroying its ServiceBag. `manager:Destroy()` destroys no stores -- they are only ever destroyed + by a removal -- and a [PlayerMock] never fires the real `Players.PlayerRemoving`, so without this + every store the spec loaded outlives it with its `task.spawn` auto-save loop running. In the shared + test place that loop later fires inside another package's window and fails it. + + `userIds` is only needed to model "PlayerRemoving landed first" for an ordering assertion. For + cleanup, omit it: the close removes every store the manager still owns. + + @param playerDataStoreService PlayerDataStoreService + @param userIds { PlayerUserId }? + @param timeout number? -- defaults to 5 + @return boolean -- false only if a shutdown was started and did not settle in time +]=] +function DataStoreTestUtils.awaitServiceShutdown(playerDataStoreService, userIds, timeout) + local managerPromise = playerDataStoreService:PromiseManager() + + -- Nothing to shut down yet, so return rather than sit out the timeout. The manager is only + -- constructed inside the last link of PromiseManager's chain, so a promise still pending here means + -- that link never ran: no manager exists, so _createDataStore was never called and no store exists to + -- leak. That holds whatever the promise is waiting on -- a ServiceBag that was never started, or a + -- real datastore still resolving. Specs that start the bag per-test would otherwise pay the full + -- timeout on every teardown. + if managerPromise:IsPending() then + return true + end + + return PromiseTestUtils.awaitSettled( + managerPromise:Then(function(manager) + -- Promise runs handlers without a pcall, so calling a destroyed manager would throw straight + -- out of the spec's teardown rather than fail it. + if not manager.Destroy then + return nil + end + + return DataStoreTestUtils.promiseSimulatedShutdown(manager, userIds) + end), + timeout or 5 + ) +end + --[=[ Builds the controller the [PlayerDataStoreManager] specs share: a session-locked manager wired to - a fresh [DataStoreMock] (keyed `user_`), all owned by a Maid. `destroy()` tears down the - manager (and the loaded stores whose auto-save loops it owns) and the service bag. + a fresh [DataStoreMock] (keyed `user_`), all owned by a Maid. + + `destroy()` shuts the server down the way Roblox would (see + [DataStoreTestUtils.promiseSimulatedShutdown]) and then tears the objects down. The shutdown is not + optional bookkeeping: a store the spec loaded keeps its auto-save loop running until something + removes it, and in the shared test place that loop outlives the spec and fires inside a later + package's window. Fields: `manager`, `mock`, `serviceBag`. Helpers: `storeAndAwaitLock()` -> boolean -- stores a value on user 1's store and waits for the - session-locked load to write the lock envelope. + session-locked load to write the lock envelope. `promiseShutdown(userIds?)` -> Promise. @return { manager: PlayerDataStoreManager, mock: DataStoreMock, ... } ]=] @@ -123,6 +192,7 @@ function DataStoreTestUtils.setupDataStoreManager() return "user_" .. tostring(userId) end, true)) + -- Returns exactly one value: specs call this straight through expect(), which rejects a second arg. local function storeAndAwaitLock() local dataStore = manager:GetDataStore(1) dataStore:Store("coins", 5) @@ -133,12 +203,18 @@ function DataStoreTestUtils.setupDataStoreManager() end, 10) end + local function promiseShutdown(userIds) + return DataStoreTestUtils.promiseSimulatedShutdown(manager, userIds) + end + return { manager = manager, mock = mock, serviceBag = serviceBag, storeAndAwaitLock = storeAndAwaitLock, + promiseShutdown = promiseShutdown, destroy = function() + PromiseTestUtils.awaitSettled(promiseShutdown(), 5) maid:DoCleaning() end, } diff --git a/src/datastore/src/Server/PlayerDataStoreManager.RemovalCallbacks.spec.lua b/src/datastore/src/Server/PlayerDataStoreManager.RemovalCallbacks.spec.lua index 35ee45dacf..c410da21b6 100644 --- a/src/datastore/src/Server/PlayerDataStoreManager.RemovalCallbacks.spec.lua +++ b/src/datastore/src/Server/PlayerDataStoreManager.RemovalCallbacks.spec.lua @@ -57,6 +57,7 @@ describe("PlayerDataStoreManager removal matrix (misbehaving removing callbacks) error("removing callback boom") end) + local dataStore = controller.manager:GetDataStore(1) expect(controller.storeAndAwaitLock()).toEqual(true) expect(function() @@ -64,6 +65,10 @@ describe("PlayerDataStoreManager removal matrix (misbehaving removing callbacks) end).toThrow("removing callback boom") expect(controller.mock:GetRaw("user_1").lock ~= nil).toEqual(true) + -- The throw latched _removing before the store left _datastores, so no later removal can + -- reach it and nothing else destroys it. Kill its auto-save loop by hand, or it outlives this + -- spec and fires inside a later package's window. + dataStore:Destroy() controller:destroy() end) diff --git a/src/datastore/src/Server/PlayerDataStoreManager.lua b/src/datastore/src/Server/PlayerDataStoreManager.lua index f452215b5f..18eaf98f8b 100644 --- a/src/datastore/src/Server/PlayerDataStoreManager.lua +++ b/src/datastore/src/Server/PlayerDataStoreManager.lua @@ -130,12 +130,6 @@ function PlayerDataStoreManager.new( self:_removePlayerDataStore(player.UserId) end)) - -- On teardown (e.g. a hot-reloaded ServiceBag, or unit tests) flush and destroy any datastores we - -- still own. See _flushAndDestroyAll. - self._maid:GiveTask(function() - self:_flushAndDestroyAll() - end) - if skipBindingToClose ~= true then -- Route through BindToCloseService so the callback is unregistered on :Destroy() -- (unlike a raw game:BindToClose, which can never be unbound and would leak on hot reload). @@ -152,33 +146,6 @@ function PlayerDataStoreManager.new( return self end ---[=[ - Flushes and tears down every datastore we still own. Runs on manager teardown (a hot-reloaded - ServiceBag, or a unit test). SaveAndCloseSession() is a best-effort synchronous write: the - underlying UpdateAsync request is dispatched before Destroy() cancels the promise, so a live - server usually honors it, but it is not guaranteed. A store whose load failed rejects, so the - rejection is swallowed. Stores handed off gracefully via _removePlayerDataStore have already been - pulled out of _datastores, so this only covers the ones nothing else cleaned up. -]=] -function PlayerDataStoreManager._flushAndDestroyAll(self: PlayerDataStoreManager): () - for userId, datastore in self._datastores do - -- Cast past the DataStore intersection type: the solver otherwise blows up ("code too complex") - -- resolving :SaveAndCloseSession()/:Destroy() through it. - local store = datastore :: any - -- A failed load makes the save reject unconditionally; skip it so teardown does not - -- manufacture a guaranteed rejection. - if not store:DidLoadFail() then - -- Close the session, don't just save: this teardown ends the session, so it must also - -- release the session lock. A lock left held here reads as a live foreign session to the - -- next server that loads the key, which then grinds through the whole graceful-close/steal - -- retry ladder against a holder that no longer exists before it can load. - store:SaveAndCloseSession() - end - store:Destroy() - self._datastores[userId] = nil - end -end - --[=[ For if you want to disable saving in studio for faster close time! ]=] @@ -315,13 +282,36 @@ end --[=[ Removes all player data stores, and returns a promise that resolves when all pending saves are saved. + + On a closing server Roblox fires PlayerRemoving for every player, so a removal is usually already + in flight by the time this runs. Those removals do the real save-and-close themselves; this waits + for them rather than starting anything of its own. + @return Promise ]=] function PlayerDataStoreManager.PromiseAllSaves(self: PlayerDataStoreManager): Promise.Promise<()> for userId, _ in self._datastores do self:_removePlayerDataStore(userId) end - return self._maid:GivePromise(PromiseUtils.all(self._pendingSaves:GetAll())) + + local promises: { Promise.Promise } = {} + + -- Wait on the removals still in flight, not just on _pendingSaves. A removal only reaches its write + -- after the removing callbacks and then DataStore's own saving callbacks resolve, and Saving fires at + -- the very end of that sync -- so _pendingSaves can be empty while a PlayerRemoving triggered moments + -- earlier is still working toward its UpdateAsync. Resolving on that empty set lets BindToClose + -- return and the server die mid-write, leaving the session locked with nobody able to release it: a + -- lock belongs to the server that holds it, so the next server can only recover by grinding the + -- graceful-close handshake against a dead JobId and then stealing it. + for _, removalPromise in self._removingPromises do + table.insert(promises, removalPromise :: any) + end + + for _, savePromise in self._pendingSaves:GetAll() do + table.insert(promises, savePromise :: any) + end + + return self._maid:GivePromise(PromiseUtils.all(promises)) end function PlayerDataStoreManager._createDataStore( diff --git a/src/datastore/src/Server/PlayerDataStoreManager.spec.lua b/src/datastore/src/Server/PlayerDataStoreManager.spec.lua index 2b1e148748..a52522b6a9 100644 --- a/src/datastore/src/Server/PlayerDataStoreManager.spec.lua +++ b/src/datastore/src/Server/PlayerDataStoreManager.spec.lua @@ -8,6 +8,7 @@ local DataStoreTestUtils = require("DataStoreTestUtils") local Jest = require("Jest") local PlayerMock = require("PlayerMock") local PromiseTestUtils = require("PromiseTestUtils") +local PromiseUtils = require("PromiseUtils") local describe = Jest.Globals.describe local expect = Jest.Globals.expect @@ -189,94 +190,115 @@ describe("PlayerDataStoreManager.PromiseAllSaves", function() end) end) -describe("PlayerDataStoreManager teardown", function() - it("destroys the datastores it still owns when the manager is destroyed", function() +-- Models a closing server the way Roblox actually behaves: PlayerRemoving fires for everyone still in +-- the server and does the save-and-close, and the close is held open until those removals flush. +describe("PlayerDataStoreManager server shutdown", function() + it("saves the staged data and releases the lock for the leaving player", function() local controller = DataStoreTestUtils.setupDataStoreManager() - local dataStore = controller.manager:GetDataStore(1) - if not expectSettled(dataStore:PromiseLoadSuccessful(), 10) then + if not controller.storeAndAwaitLock() then + expect("lock was never acquired").toEqual("lock was acquired") controller:destroy() return end - controller.manager:Destroy() + if not expectSettled(controller.promiseShutdown({ 1 }), 10) then + controller:destroy() + return + end - expect(getmetatable(dataStore)).toBeNil() + local raw = controller.mock:GetRaw("user_1") + expect(raw.coins).toEqual(5) + expect(raw.lock).toEqual(nil) controller:destroy() end) - it("flushes staged data synchronously to the underlying store when destroyed", function() + it("releases the lock for every player still in the server", function() local controller = DataStoreTestUtils.setupDataStoreManager() - local dataStore = controller.manager:GetDataStore(1) - if not expectSettled(dataStore:PromiseLoadSuccessful(), 10) then + controller.manager:GetDataStore(1):Store("coins", 1) + controller.manager:GetDataStore(2):Store("coins", 2) + + local locked = PromiseTestUtils.awaitValue(function() + local rawOne = controller.mock:GetRaw("user_1") + local rawTwo = controller.mock:GetRaw("user_2") + return rawOne ~= nil and rawOne.lock ~= nil and rawTwo ~= nil and rawTwo.lock ~= nil + end, 10) + if not locked then + expect("both locks were never acquired").toEqual("both locks were acquired") controller:destroy() return end - dataStore:Store("coins", 5) - - controller.manager:Destroy() + if not expectSettled(controller.promiseShutdown({ 1, 2 }), 10) then + controller:destroy() + return + end - local raw = controller.mock:GetRaw("user_1") - expect(raw).never.toBeNil() - expect(raw.coins).toEqual(5) + expect(controller.mock:GetRaw("user_1").lock).toEqual(nil) + expect(controller.mock:GetRaw("user_2").lock).toEqual(nil) + expect(controller.mock:GetRaw("user_1").coins).toEqual(1) + expect(controller.mock:GetRaw("user_2").coins).toEqual(2) controller:destroy() end) - it("releases the session lock when destroyed, not just the staged data", function() + it("destroys each store once its removal has flushed", function() local controller = DataStoreTestUtils.setupDataStoreManager() - if not controller.storeAndAwaitLock() then - expect("lock was never acquired").toEqual("lock was acquired") + local dataStore = controller.manager:GetDataStore(1) + if not expectSettled(dataStore:PromiseLoadSuccessful(), 10) then controller:destroy() return end - controller.manager:Destroy() + if not expectSettled(controller.promiseShutdown({ 1 }), 10) then + controller:destroy() + return + end - expect(PromiseTestUtils.awaitValue(function() - local raw = controller.mock:GetRaw("user_1") - return raw ~= nil and raw.lock == nil - end, 5)).toEqual(true) - expect(controller.mock:GetRaw("user_1").coins).toEqual(5) + expect(getmetatable(dataStore)).toBeNil() controller:destroy() end) - it("releases the lock for every store it still owns", function() + it("does not resolve the close while a PlayerRemoving save is still in flight", function() local controller = DataStoreTestUtils.setupDataStoreManager() - controller.manager:GetDataStore(1):Store("coins", 1) - controller.manager:GetDataStore(2):Store("coins", 2) + -- An async removing callback pushes the write past the moment the close is requested, which is + -- exactly the window where waiting on pending saves alone finds nothing to wait for. + controller.manager:AddRemovingCallback(function() + return PromiseUtils.delayed(0.5) + end) - local locked = PromiseTestUtils.awaitValue(function() - local rawOne = controller.mock:GetRaw("user_1") - local rawTwo = controller.mock:GetRaw("user_2") - return rawOne ~= nil and rawOne.lock ~= nil and rawTwo ~= nil and rawTwo.lock ~= nil - end, 10) - if not locked then - expect("both locks were never acquired").toEqual("both locks were acquired") + if not controller.storeAndAwaitLock() then + expect("lock was never acquired").toEqual("lock was acquired") controller:destroy() return end - controller.manager:Destroy() + -- PlayerRemoving lands first, then the server begins closing. + controller.manager:RemovePlayerDataStore(1) - expect(PromiseTestUtils.awaitValue(function() - local rawOne = controller.mock:GetRaw("user_1") - local rawTwo = controller.mock:GetRaw("user_2") - return rawOne ~= nil and rawOne.lock == nil and rawTwo ~= nil and rawTwo.lock == nil - end, 5)).toEqual(true) - expect(controller.mock:GetRaw("user_1").coins).toEqual(1) - expect(controller.mock:GetRaw("user_2").coins).toEqual(2) + local closePromise = controller.manager:PromiseAllSaves() + expect(closePromise:IsPending()).toEqual(true) + + if not expectSettled(closePromise, 10) then + controller:destroy() + return + end + + -- The close resolving has to mean the write landed. If it can resolve first, the real server + -- dies here with the session still locked and no other server able to release it. + local raw = controller.mock:GetRaw("user_1") + expect(raw.coins).toEqual(5) + expect(raw.lock).toEqual(nil) controller:destroy() end) - it("tears down cleanly when a store's load failed, leaking no rejection and writing no lock", function() + it("closes cleanly when a store's load failed, leaking no rejection and writing no lock", function() local controller = DataStoreTestUtils.setupDataStoreManager() controller.mock:FailAllRequests() @@ -290,7 +312,10 @@ describe("PlayerDataStoreManager teardown", function() end expect((loaded:Wait())).toEqual(false) - controller.manager:Destroy() + if not expectSettled(controller.promiseShutdown({ 1 }), 10) then + controller:destroy() + return + end expect(controller.mock:GetRaw("user_1")).toBeNil() diff --git a/src/datastore/src/Server/PlayerDataStoreService.spec.lua b/src/datastore/src/Server/PlayerDataStoreService.spec.lua index f570c8d53b..b001184b7b 100644 --- a/src/datastore/src/Server/PlayerDataStoreService.spec.lua +++ b/src/datastore/src/Server/PlayerDataStoreService.spec.lua @@ -5,8 +5,10 @@ local require = require(script.Parent.loader).load(script) local DataStoreMock = require("DataStoreMock") +local DataStoreTestUtils = require("DataStoreTestUtils") local Jest = require("Jest") local Maid = require("Maid") +local Promise = require("Promise") local PromiseTestUtils = require("PromiseTestUtils") local ServiceBag = require("ServiceBag") @@ -26,10 +28,26 @@ local function setup(mock) serviceBag:Start() end + -- Drives the real close path: the manager the service owns, shut down the way Roblox does. Without a + -- mock the bag was never started, so there is no manager to reach and nothing to shut down. + local function promiseShutdown(userIds) + if not mock then + return Promise.resolved() + end + + return service:PromiseManager():Then(function(manager) + return DataStoreTestUtils.promiseSimulatedShutdown(manager, userIds) + end) + end + return { service = service, mock = mock, + promiseShutdown = promiseShutdown, destroy = function() + -- Otherwise a store the spec loaded outlives it with its auto-save loop running, and fires + -- inside a later package's window in the shared test place. + PromiseTestUtils.awaitSettled(promiseShutdown(), 5) maid:DoCleaning() end, } @@ -157,8 +175,10 @@ describe("PlayerDataStoreService failure handling", function() end) end) -describe("PlayerDataStoreService teardown", function() - it("destroys the datastore its manager owns when the service is destroyed", function() +-- The service registers manager:PromiseAllSaves() as its BindToClose callback, so a closing server is +-- PlayerRemoving doing the save-and-close with that callback held open until it flushes. +describe("PlayerDataStoreService server shutdown", function() + it("saves the staged data and destroys the store when the server closes", function() local controller = setup(DataStoreMock.new()) local promise = controller.service:PromiseDataStore(1) @@ -175,34 +195,19 @@ describe("PlayerDataStoreService teardown", function() return end - controller:destroy() - - expect(getmetatable(dataStore)).toBeNil() - end) - - it("flushes staged data synchronously to the underlying store when destroyed", function() - local controller = setup(DataStoreMock.new()) - - local promise = controller.service:PromiseDataStore(1) - if not PromiseTestUtils.awaitSettled(promise, 10) then - expect("hung").toEqual("settled") - controller:destroy() - return - end - local _ok, dataStore = promise:Yield() + dataStore:Store("coins", 7) - if not PromiseTestUtils.awaitSettled(dataStore:PromiseLoadSuccessful(), 10) then - expect("load hung").toEqual("load settled") + if not PromiseTestUtils.awaitSettled(controller.promiseShutdown({ 1 }), 10) then + expect("shutdown never flushed").toEqual("shutdown flushed") controller:destroy() return end - dataStore:Store("coins", 7) - - controller:destroy() - local raw = controller.mock:GetRaw("1") expect(raw).never.toBeNil() expect(raw.coins).toEqual(7) + expect(getmetatable(dataStore)).toBeNil() + + controller:destroy() end) end) diff --git a/src/saveslot/src/Server/Binders/HasSaveSlots.Code.spec.lua b/src/saveslot/src/Server/Binders/HasSaveSlots.Code.spec.lua index 1b7cc17fbc..1a657b0ec5 100644 --- a/src/saveslot/src/Server/Binders/HasSaveSlots.Code.spec.lua +++ b/src/saveslot/src/Server/Binders/HasSaveSlots.Code.spec.lua @@ -8,6 +8,7 @@ local require = require(script.Parent.loader).load(script) local DataStoreMock = require("DataStoreMock") +local DataStoreTestUtils = require("DataStoreTestUtils") local Jest = require("Jest") local PlayerDataStoreService = require("PlayerDataStoreService") local PlayerMock = require("PlayerMock") @@ -48,6 +49,10 @@ local function setup() hasSaveSlots.MaxSlotCount.Value = 5 local function destroy() + -- The store the spec loaded is only destroyed by a removal, and a PlayerMock never fires the + -- real Players.PlayerRemoving, so shut down the way Roblox does or its auto-save loop outlives + -- this spec and fires inside a later package's window. + DataStoreTestUtils.awaitServiceShutdown(playerDataStoreService) fakePlayer:Destroy() serviceBag:Destroy() end diff --git a/src/saveslot/src/Server/Binders/HasSaveSlots.ExportImport.spec.lua b/src/saveslot/src/Server/Binders/HasSaveSlots.ExportImport.spec.lua index da211aa9db..9bc4d10db9 100644 --- a/src/saveslot/src/Server/Binders/HasSaveSlots.ExportImport.spec.lua +++ b/src/saveslot/src/Server/Binders/HasSaveSlots.ExportImport.spec.lua @@ -9,6 +9,7 @@ local require = require(script.Parent.loader).load(script) local DataStoreMock = require("DataStoreMock") +local DataStoreTestUtils = require("DataStoreTestUtils") local Jest = require("Jest") local PlayerDataStoreService = require("PlayerDataStoreService") local PlayerMock = require("PlayerMock") @@ -44,6 +45,10 @@ local function setup() hasSaveSlots.MaxSlotCount.Value = 5 local function destroy() + -- The store the spec loaded is only destroyed by a removal, and a PlayerMock never fires the + -- real Players.PlayerRemoving, so shut down the way Roblox does or its auto-save loop outlives + -- this spec and fires inside a later package's window. + DataStoreTestUtils.awaitServiceShutdown(playerDataStoreService) fakePlayer:Destroy() serviceBag:Destroy() end diff --git a/src/saveslot/src/Server/Binders/HasSaveSlots.PreSelect.spec.lua b/src/saveslot/src/Server/Binders/HasSaveSlots.PreSelect.spec.lua index e4b9d4abb7..c628c0f1a7 100644 --- a/src/saveslot/src/Server/Binders/HasSaveSlots.PreSelect.spec.lua +++ b/src/saveslot/src/Server/Binders/HasSaveSlots.PreSelect.spec.lua @@ -8,6 +8,7 @@ local require = require(script.Parent.loader).load(script) local DataStoreMock = require("DataStoreMock") +local DataStoreTestUtils = require("DataStoreTestUtils") local Jest = require("Jest") local PlayerDataStoreService = require("PlayerDataStoreService") local PlayerMock = require("PlayerMock") @@ -73,6 +74,10 @@ local function setup() activeContext = nil end + -- The store the spec loaded is only destroyed by a removal, and a PlayerMock never fires the + -- real Players.PlayerRemoving, so shut down the way Roblox does or its auto-save loop outlives + -- this spec and fires inside a later package's window. + DataStoreTestUtils.awaitServiceShutdown(playerDataStoreService) fakePlayer:Destroy() serviceBag:Destroy() end diff --git a/src/saveslot/src/Server/Binders/HasSaveSlots.SharedStore.spec.lua b/src/saveslot/src/Server/Binders/HasSaveSlots.SharedStore.spec.lua index 0a0fb840ca..94c6d57bba 100644 --- a/src/saveslot/src/Server/Binders/HasSaveSlots.SharedStore.spec.lua +++ b/src/saveslot/src/Server/Binders/HasSaveSlots.SharedStore.spec.lua @@ -8,6 +8,7 @@ local require = require(script.Parent.loader).load(script) local DataStoreMock = require("DataStoreMock") +local DataStoreTestUtils = require("DataStoreTestUtils") local Jest = require("Jest") local PlayerDataStoreService = require("PlayerDataStoreService") local PlayerMock = require("PlayerMock") @@ -47,6 +48,10 @@ local function setup() hasSaveSlots.MaxSlotCount.Value = 5 local function destroy() + -- The store the spec loaded is only destroyed by a removal, and a PlayerMock never fires the + -- real Players.PlayerRemoving, so shut down the way Roblox does or its auto-save loop outlives + -- this spec and fires inside a later package's window. + DataStoreTestUtils.awaitServiceShutdown(playerDataStoreService) fakePlayer:Destroy() serviceBag:Destroy() end diff --git a/src/saveslot/src/Server/Binders/HasSaveSlots.TransferableEphemeral.spec.lua b/src/saveslot/src/Server/Binders/HasSaveSlots.TransferableEphemeral.spec.lua index 26c1dcfcd6..e120ed570a 100644 --- a/src/saveslot/src/Server/Binders/HasSaveSlots.TransferableEphemeral.spec.lua +++ b/src/saveslot/src/Server/Binders/HasSaveSlots.TransferableEphemeral.spec.lua @@ -9,6 +9,7 @@ local require = require(script.Parent.loader).load(script) local DataStoreMock = require("DataStoreMock") +local DataStoreTestUtils = require("DataStoreTestUtils") local Jest = require("Jest") local PlayerDataStoreService = require("PlayerDataStoreService") local PlayerMock = require("PlayerMock") @@ -49,6 +50,10 @@ local function setup() hasSaveSlots.MaxSlotCount.Value = 5 local function destroy() + -- The store the spec loaded is only destroyed by a removal, and a PlayerMock never fires the + -- real Players.PlayerRemoving, so shut down the way Roblox does or its auto-save loop outlives + -- this spec and fires inside a later package's window. + DataStoreTestUtils.awaitServiceShutdown(playerDataStoreService) fakePlayer:Destroy() serviceBag:Destroy() end diff --git a/src/saveslot/src/Server/Binders/HasSaveSlots.lua b/src/saveslot/src/Server/Binders/HasSaveSlots.lua index 503808aad9..85cd639547 100644 --- a/src/saveslot/src/Server/Binders/HasSaveSlots.lua +++ b/src/saveslot/src/Server/Binders/HasSaveSlots.lua @@ -418,7 +418,13 @@ function HasSaveSlots.PromiseImportSlotFromSharedDataStore( self: HasSaveSlots, key: string ): Promise.Promise - return self._sharedSaveSlotDataStoreService:PromiseRead(key):Then(function(export) + -- Maid-owned like the other shared-store reads: this one is not even gated behind _loadPromise, so + -- nothing upstream rejects it when the player leaves mid-read. + return self._maid:GivePromise(self._sharedSaveSlotDataStoreService:PromiseRead(key)):Then(function(export) + if not self.Destroy then + return (Promise :: any).rejected() -- Destroyed. Empty, so it stays out of the logs + end + if export == nil then return (Promise :: any).rejected(`No save slot stored under \{{key}\}`) end @@ -440,7 +446,15 @@ function HasSaveSlots.PromiseSelectTransferableEphemeralSlot( key: string ): Promise.Promise return (self._loadPromise :: any):Then(function() - return self._sharedSaveSlotDataStoreService:PromiseRead(key):Then(function(export) + -- Maid-owned for the same reason as _promiseLoadSlots: this read runs on the join path (a teleport + -- arriving with an ephemeral key) and outlives a leave, and its continuation builds a slot. + return self._maid:GivePromise(self._sharedSaveSlotDataStoreService:PromiseRead(key)):Then(function(export) + if not self.Destroy then + -- Destroyed. Rejecting keeps the Promise contract honest -- a nil fulfilment reads as + -- "imported, no slot" to a caller like SaveSlotCmdrService. Empty, so it stays out of the logs. + return (Promise :: any).rejected() + end + if export == nil then return (Promise :: any).rejected(`No save slot stored under \{{key}\}`) end @@ -1102,61 +1116,86 @@ function HasSaveSlots.PromiseIncomingSlotId(self: HasSaveSlots): Promise.Promise end) end +-- Maid-owning every hop is what makes this safe, for the same reason as the selection chain in +-- SaveSlotService: a session-locked or retrying read settles long after the player left, and Promise has no +-- cancellation, so owning only the outermost promise detaches nothing upstream. Calling into this destroyed +-- (metatable-stripped) object -- or into its destroyed stores -- throws, and because a datastore load +-- resolves inside its UpdateAsync transform, that throw surfaces as a "Transform function error" and takes +-- the transform's write down with it. The liveness checks cover a settle that lands mid-teardown, before +-- this promise's own maid task was reached. See SaveSlotLateSettle.spec. function HasSaveSlots._promiseLoadSlots(self: HasSaveSlots): Promise.Promise<{}> - return self._playerDataStoreService:PromiseDataStore(self._obj):Then(function(dataStore) + return self._maid:GivePromise(self._playerDataStoreService:PromiseDataStore(self._obj)):Then(function(dataStore) + if not self.Destroy then + return nil -- Destroyed + end + self._dataStore = dataStore self._systemStore = dataStore:GetSubStore(SaveSlotConstants.SYSTEM_STORE_KEY) self._metadataStore = self._systemStore:GetSubStore(SaveSlotConstants.METADATA_STORE_KEY) - return self._metadataStore:LoadAll({}):Then(function(metadata) + return self._maid:GivePromise(self._metadataStore:LoadAll({})):Then(function(metadata) + if not self.Destroy then + return nil -- Destroyed + end + for slotId, data in metadata do self:_buildSlot(slotId, data) end - return self._systemStore:Load("activeSlotId"):Then(function(activeId: SaveSlotData.SlotId?) - self._lastActiveSlotId = activeId - self.LastActiveSlotId.Value = activeId - - -- The persisted active-slot pointer and the replicated "Continue" target only ever track real - -- slots. This replaces StoreOnValueChange so an ephemeral selection is invisible to both: - -- entering one leaves them pinned to the real slot, and the ephemeral slot is torn down the - -- moment it stops being active. We track the id we are leaving to know which of those to do. - local previousActiveSlotId: SaveSlotData.SlotId? = activeId - - self._maid:GiveTask(self.ActiveSlotId.Changed:Connect(function() - local active = self.ActiveSlotId.Value - - -- Replicate the active transferable-ephemeral slot's shared-store key (nil otherwise) so a - -- client-initiated teleport can carry it forward (SaveSlotServiceClient's provider reads this). - self.ActiveTransferableEphemeralKey.Value = if active - then self._transferableEphemeralKeys[active] - else nil - - local leftSlotId = previousActiveSlotId - -- Read before the retire below, while the outgoing slot is still in the map. - local leavingEphemeral = self:_isEphemeral(leftSlotId) - previousActiveSlotId = active - - -- Persist the pointer + remember the Continue target only for real-slot transitions - -- (real -> real, real -> nil deselect, nil -> real). Skip both ephemeral cases: entering an - -- ephemeral slot must stay invisible to persistence, and leaving one back to no slot must - -- leave the real pointer pinned where it was. - local enteringEphemeral = self:_isEphemeral(active) - local leavingEphemeralToMenu = leavingEphemeral and active == nil - if not (enteringEphemeral or leavingEphemeralToMenu) then - self._systemStore:Store("activeSlotId", active) - if active ~= nil then - self._lastActiveSlotId = active - self.LastActiveSlotId.Value = active - end + return self._maid + :GivePromise(self._systemStore:Load("activeSlotId")) + :Then(function(activeId: SaveSlotData.SlotId?) + if not self.Destroy then + return nil -- Destroyed end - -- An ephemeral slot exists only while it is the active slot; retire the one we just left. - if leavingEphemeral and leftSlotId ~= active then - self:_destroyEphemeralSlot(leftSlotId :: SaveSlotData.SlotId) - end - end)) - end) + self._lastActiveSlotId = activeId + self.LastActiveSlotId.Value = activeId + + -- The persisted active-slot pointer and the replicated "Continue" target only ever track real + -- slots. This replaces StoreOnValueChange so an ephemeral selection is invisible to both: + -- entering one leaves them pinned to the real slot, and the ephemeral slot is torn down the + -- moment it stops being active. We track the id we are leaving to know which of those to do. + local previousActiveSlotId: SaveSlotData.SlotId? = activeId + + self._maid:GiveTask(self.ActiveSlotId.Changed:Connect(function() + local active = self.ActiveSlotId.Value + + -- Replicate the active transferable-ephemeral slot's shared-store key (nil otherwise) so a + -- client-initiated teleport can carry it forward (SaveSlotServiceClient's provider reads this). + self.ActiveTransferableEphemeralKey.Value = if active + then self._transferableEphemeralKeys[active] + else nil + + local leftSlotId = previousActiveSlotId + -- Read before the retire below, while the outgoing slot is still in the map. + local leavingEphemeral = self:_isEphemeral(leftSlotId) + previousActiveSlotId = active + + -- Persist the pointer + remember the Continue target only for real-slot transitions + -- (real -> real, real -> nil deselect, nil -> real). Skip both ephemeral cases: entering an + -- ephemeral slot must stay invisible to persistence, and leaving one back to no slot must + -- leave the real pointer pinned where it was. + local enteringEphemeral = self:_isEphemeral(active) + local leavingEphemeralToMenu = leavingEphemeral and active == nil + if not (enteringEphemeral or leavingEphemeralToMenu) then + self._systemStore:Store("activeSlotId", active) + if active ~= nil then + self._lastActiveSlotId = active + self.LastActiveSlotId.Value = active + end + end + + -- An ephemeral slot exists only while it is the active slot; retire the one we just left. + if leavingEphemeral and leftSlotId ~= active then + self:_destroyEphemeralSlot(leftSlotId :: SaveSlotData.SlotId) + end + end)) + + -- Matches the liveness-guard returns above, which make this callback's inferred return + -- type nil; falling off the end instead returns no values at all. + return nil + end) end) end) end diff --git a/src/saveslot/src/Server/Binders/HasSaveSlots.spec.lua b/src/saveslot/src/Server/Binders/HasSaveSlots.spec.lua index bd683850e4..67f52decde 100644 --- a/src/saveslot/src/Server/Binders/HasSaveSlots.spec.lua +++ b/src/saveslot/src/Server/Binders/HasSaveSlots.spec.lua @@ -10,6 +10,7 @@ local require = require(script.Parent.loader).load(script) local DataStoreMock = require("DataStoreMock") +local DataStoreTestUtils = require("DataStoreTestUtils") local Jest = require("Jest") local Maid = require("Maid") local Observable = require("Observable") @@ -50,6 +51,10 @@ local function setup(mock: DataStoreMock.DataStoreMock?) hasSaveSlots.MaxSlotCount.Value = 5 local function destroy() + -- The store the spec loaded is only destroyed by a removal, and a PlayerMock never fires the + -- real Players.PlayerRemoving, so shut down the way Roblox does or its auto-save loop outlives + -- this spec and fires inside a later package's window. + DataStoreTestUtils.awaitServiceShutdown(playerDataStoreService) fakePlayer:Destroy() serviceBag:Destroy() end diff --git a/src/saveslot/src/Server/SaveSlotLateSettle.spec.lua b/src/saveslot/src/Server/SaveSlotLateSettle.spec.lua index 5b9713cf04..cf6223ee0f 100644 --- a/src/saveslot/src/Server/SaveSlotLateSettle.spec.lua +++ b/src/saveslot/src/Server/SaveSlotLateSettle.spec.lua @@ -1,6 +1,6 @@ --!strict --[[ - The per-player selection chain (slots loaded -> teleport read -> default slot) runs against + The per-player load and selection chain (slots loaded -> teleport read -> default slot) runs against datastore reads that can settle long after the player left — session-lock and datastore retries outlive a leave. A continuation that then calls into the destroyed (metatable-stripped) HasSaveSlots binder throws "attempt to call missing method ..." as a @@ -14,10 +14,12 @@ local require = require(script.Parent.loader).load(script) local Workspace = game:GetService("Workspace") local DataStoreMock = require("DataStoreMock") +local DataStoreTestUtils = require("DataStoreTestUtils") local Jest = require("Jest") local Maid = require("Maid") local PlayerMock = require("PlayerMock") local PromiseTestUtils = require("PromiseTestUtils") +local SaveSlotConstants = require("SaveSlotConstants") local ServiceBag = require("ServiceBag") local afterEach = Jest.Globals.afterEach @@ -26,6 +28,7 @@ local expect = Jest.Globals.expect local it = Jest.Globals.it local USER_ID = 636363 +local EXISTING_SLOT_ID = "e6f0c1a2-late-settle" -- Every datastore read yields this long, so the spec can land the unbind inside a chosen -- read's in-flight window. @@ -65,6 +68,7 @@ local function setup() controller = { hasSaveSlotsBinder = hasSaveSlotsBinder, fakePlayer = fakePlayer, + mock = mock, destroy = function(self: any) if destroyed then return @@ -73,6 +77,10 @@ local function setup() if activeController == self then activeController = nil end + -- The store the spec loaded is only destroyed by a removal, and a PlayerMock never fires the + -- real Players.PlayerRemoving, so shut down the way Roblox does or its auto-save loop outlives + -- this spec and fires inside a later package's window. + DataStoreTestUtils.awaitServiceShutdown(playerDataStoreService) serviceBag:Destroy() maid:DoCleaning() -- Any straggler continuation blows up here, inside the test that owns it, rather @@ -85,6 +93,22 @@ local function setup() return controller end +-- A returning player's persisted slot, written straight into the mock the way a previous session left it. +local function seedExistingSlot(controller: any) + controller.mock:SetRaw(tostring(USER_ID), { + [SaveSlotConstants.SYSTEM_STORE_KEY] = { + [SaveSlotConstants.METADATA_STORE_KEY] = { + [EXISTING_SLOT_ID] = { + SlotIndex = SaveSlotConstants.DEFAULT_SLOT_INDEX, + SlotName = "Slot 1", + CreatedTime = 1, + }, + }, + activeSlotId = EXISTING_SLOT_ID, + }, + }) +end + describe("SaveSlotService selection chain vs a player who leaves mid-load", function() it("consumes a slots-load still pending when the binder dies", function() local controller = setup() @@ -101,6 +125,49 @@ describe("SaveSlotService selection chain vs a player who leaves mid-load", func controller:destroy() end) + -- Pins that seedExistingSlot really lands where the load reads it. Without this, drift in the raw + -- layout would quietly reduce the late-settle test below to the empty-metadata case it exists to + -- replace -- green, and no longer reaching _buildSlot at all. + it("loads the seeded slot when the player stays", function() + local controller = setup() + seedExistingSlot(controller) + + local hasSaveSlots = + assert(controller.hasSaveSlotsBinder:Bind(controller.fakePlayer), "Failed to bind HasSaveSlots") + expect(PromiseTestUtils.awaitSettled(hasSaveSlots:PromiseSlotsLoaded(), 10)).toEqual(true) + + local hasSlotStatus, hasSlot = PromiseTestUtils.awaitOutcome(hasSaveSlots:PromiseHasSlot(EXISTING_SLOT_ID), 10) + expect(hasSlotStatus).toEqual("resolved") + expect(hasSlot).toEqual(true) + + -- Waiting on the selection itself, rather than PromiseLastActiveSlotId (which can answer from + -- _lastActiveSlotId while the selection is still in flight), also quiesces the chain before teardown. + expect(PromiseTestUtils.awaitValue(function() + return hasSaveSlots.ActiveSlotId.Value == EXISTING_SLOT_ID + end, 10)).toEqual(true) + + controller:destroy() + end) + + it("consumes a returning player's slots-load that settles after the binder died", function() + local controller = setup() + + -- A returning player, so the load settles with metadata to build slots from. The empty-metadata + -- case above never reaches _buildSlot, which is how a live server kept throwing there + -- ("attempt to call missing method '_buildSlot'") while that test stayed green. + seedExistingSlot(controller) + + local hasSaveSlots = + assert(controller.hasSaveSlotsBinder:Bind(controller.fakePlayer), "Failed to bind HasSaveSlots") + local slotsLoaded = hasSaveSlots:PromiseSlotsLoaded() + + task.wait() + expect(PromiseTestUtils.awaitSettled(slotsLoaded, 0)).toEqual(false) + controller.hasSaveSlotsBinder:Unbind(controller.fakePlayer) + + controller:destroy() + end) + it("consumes a default-slot read that settles after the binder died", function() local controller = setup() diff --git a/src/saveslot/src/Server/SaveSlotLoadFlow.spec.lua b/src/saveslot/src/Server/SaveSlotLoadFlow.spec.lua index 84b78e2022..983882f9ab 100644 --- a/src/saveslot/src/Server/SaveSlotLoadFlow.spec.lua +++ b/src/saveslot/src/Server/SaveSlotLoadFlow.spec.lua @@ -10,6 +10,7 @@ local require = require(script.Parent.loader).load(script) local DataStoreMock = require("DataStoreMock") +local DataStoreTestUtils = require("DataStoreTestUtils") local Jest = require("Jest") local Maid = require("Maid") local PromiseTestUtils = require("PromiseTestUtils") @@ -34,6 +35,10 @@ local function setup(mock) playerDataStoreService = playerDataStoreService, mock = mock, destroy = function() + -- The store the spec loaded is only destroyed by a removal, and a PlayerMock never fires the + -- real Players.PlayerRemoving, so shut down the way Roblox does or its auto-save loop outlives + -- this spec and fires inside a later package's window. + DataStoreTestUtils.awaitServiceShutdown(playerDataStoreService) maid:DoCleaning() end, } diff --git a/src/saveslot/src/Server/SaveSlotOverflow.spec.lua b/src/saveslot/src/Server/SaveSlotOverflow.spec.lua index 096760d307..0c3deee965 100644 --- a/src/saveslot/src/Server/SaveSlotOverflow.spec.lua +++ b/src/saveslot/src/Server/SaveSlotOverflow.spec.lua @@ -12,6 +12,7 @@ local require = require(script.Parent.loader).load(script) local DataStoreMock = require("DataStoreMock") +local DataStoreTestUtils = require("DataStoreTestUtils") local Jest = require("Jest") local Maid = require("Maid") local PromiseTestUtils = require("PromiseTestUtils") @@ -36,6 +37,10 @@ local function setup(mock) playerDataStoreService = playerDataStoreService, mock = mock, destroy = function() + -- The store the spec loaded is only destroyed by a removal, and a PlayerMock never fires the + -- real Players.PlayerRemoving, so shut down the way Roblox does or its auto-save loop outlives + -- this spec and fires inside a later package's window. + DataStoreTestUtils.awaitServiceShutdown(playerDataStoreService) maid:DoCleaning() end, } diff --git a/src/saveslot/src/Server/SaveSlotService.spec.lua b/src/saveslot/src/Server/SaveSlotService.spec.lua index 065bf04f47..e76af17fa8 100644 --- a/src/saveslot/src/Server/SaveSlotService.spec.lua +++ b/src/saveslot/src/Server/SaveSlotService.spec.lua @@ -10,6 +10,7 @@ local require = require(script.Parent.loader).load(script) local DataStoreMock = require("DataStoreMock") +local DataStoreTestUtils = require("DataStoreTestUtils") local Jest = require("Jest") local Maid = require("Maid") local PlayerDataStoreService = require("PlayerDataStoreService") @@ -51,6 +52,10 @@ local function setup() return value end, destroy = function(_self) + -- The store the spec loaded is only destroyed by a removal, and a PlayerMock never fires the + -- real Players.PlayerRemoving, so shut down the way Roblox does or its auto-save loop outlives + -- this spec and fires inside a later package's window. + DataStoreTestUtils.awaitServiceShutdown(playerDataStoreService) maid:DoCleaning() end, }