fix(datastore): stop manager teardown from destroying stores before PlayerRemoving saves them - #780
Merged
Merged
Conversation
…layerRemoving saves them A closing Roblox server fires PlayerRemoving for every player still in it and holds the shutdown open for those handlers, so PlayerRemoving is the save path during a restart. The teardown flush on the manager's maid got there first: it cleared _datastores and destroyed each store, so the removal that would have saved the data and released the session lock found nothing and returned early. Both flavors of that flush lost data. Saving without closing left the lock held, and because a lock can only be released by the server holding it, the next server to load the key spent ~76 seconds asking a dead JobId to close gracefully before it could steal it. Saving and closing released the lock but destroyed the store synchronously against its own in-flight write, while players were still in the server and still writing, so consumers wrote into a destroyed object for the rest of the shutdown window. Removes the teardown hook so every save routes through _removePlayerDataStore as it did before, and adds the ordering guarantee the close path was missing: PromiseAllSaves now waits on the removals already in flight, not only on a _pendingSaves snapshot. DataStore fires Saving at the very end of its sync, so a removal triggered moments earlier can still be working toward its UpdateAsync while _pendingSaves is empty -- BindToClose would then return and the server die mid-write, still locked. Specs modelled a shutdown as manager:Destroy(), which is not one and never has been, which is how the flush looked well covered while being wrong. They now drive DataStoreTestUtils.promiseSimulatedShutdown, and a new test pins that the close cannot resolve while a PlayerRemoving save is still in flight. Adds src/datastore/docs/shutdown-and-session-locks.md recording the engine behavior this rests on and why the flush was rejected. Claude-Session: https://claude.ai/code/session_01NLMCUpaUds4ZuB9o4bkgXX
Test Results
6 packages tested, 6 passed, 0 failed in 1m11s · View logs Deploy Resultsℹ️ No changed packages with deploy targets were discovered for this PR. · View logs |
…dder math Review of #780 caught four factual errors. The load-bearing one: the doc built its case on a teardown that races PlayerRemoving during a restart, but no path in ServiceBag, BindToCloseService, or any game here destroys the manager on close, so that story is unsubstantiated. The real live-server data-loss path is stronger and now documented in its place. The Saving connection lives on the per-user maid that _removePlayerDataStore clears synchronously, and DataStore fires Saving at the end of its sync -- so the moment anything in a removal yields, that save can never enter _pendingSaves at all. The old PromiseAllSaves therefore had nothing to wait on, guaranteed, rather than losing a race. Also corrects: the ~80s ladder comes from a hardcoded 5s timeout in DataStoreMessageHelper, not from SetSessionMessagingCloseDelaySeconds (which sits in a branch never reached when the holder is dead), so tuning that setter does not shorten it; "no path destroys a store without saving" overstated it, since the Destroy sits in a Finally that also runs on rejection, which additionally means a resolved close proves every removal settled and not that every write succeeded; and nothing actually emits a stack trace naming a hung removing callback, since the callback has already returned by then. Records that manager:Destroy() now destroys no stores, and why a destroy-only teardown would be worse than the leak it would fix. Claude-Session: https://claude.ai/code/session_01NLMCUpaUds4ZuB9o4bkgXX
…s down Deleting the manager teardown flush means manager:Destroy() destroys no stores -- they are only ever destroyed by a removal -- and a PlayerMock never fires the real Players.PlayerRemoving, so every store these specs loaded outlived them 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, which is silent until a run crosses the autosave interval. Adds DataStoreTestUtils.awaitServiceShutdown and calls it from each harness that injects a datastore into PlayerDataStoreService, so the stores are removed through the real close path. Keeping this in the harness rather than re-adding a destroy on the manager is deliberate: a destroy-only teardown would leave _datastores holding destroyed stores, so a PlayerRemoving arriving afterwards would call SaveAndCloseSession() on a nil metatable. Claude-Session: https://claude.ai/code/session_01NLMCUpaUds4ZuB9o4bkgXX
… left HasSaveSlots._promiseLoadSlots chained its datastore reads raw, with only the outermost promise maid-owned. A session-locked or retrying load outlives a leave, so when one finally settled the continuation called into the destroyed (metatable-stripped) binder: "HasSaveSlots:1113: attempt to call missing method '_buildSlot' of table". A load resolves inside its own UpdateAsync transform, so production reported it as a Transform function error and lost that transform's write with it -- the lock it was acquiring included. Same defect f8716a3 fixed in SaveSlotService's selection chain, one layer down in the binder's own load, so it takes the same shape: every hop maid-owned, every continuation re-checking the object is alive before touching it or its stores. Promise has no cancellation, so maid-owning the outer promise alone detaches nothing upstream. SaveSlotLateSettle.spec covered the empty-metadata case only, where the load settles with nothing to build and never reaches _buildSlot -- green while a live server threw. Seeds a returning player's slot metadata so the crashing line runs: without these guards that spec reproduces the production stack at the same line and fails the run. Claude-Session: https://claude.ai/code/session_01HouVsNhp1j387CUHa4zCMq
…ded slot Review follow-ups on the load guard. PromiseSelectTransferableEphemeralSlot ran the shared-store read raw and then built a slot from it, the same shape and the same _buildSlot as the load. It is on the join path -- SaveSlotService starts it on every bind -- so a player leaving during that GetAsync lands the continuation on the destroyed binder. The late-settle spec asserted only that the load was still pending, so drift in the seeded raw layout would have quietly reduced it to the empty-metadata case it was written to replace: still green, no longer reaching _buildSlot. Pins the seed with a stay-put test that reads the slot back through PromiseHasSlot and PromiseLastActiveSlotId. Also says plainly in the comment that maid ownership is the fix and the liveness checks only cover a continuation queued before the maid died -- with every hop owned, they are belt-and-braces, and a reader should not mistake them for the mechanism. Claude-Session: https://claude.ai/code/session_01HouVsNhp1j387CUHa4zCMq
Owner
Author
Subagent review of the saveslot half — outcomeVerified sound and left as-is: the Acted on in 2b76762:
Knowingly not in scope here, and worth a follow-up rather than widening a shutdown PR: the same raw-hop shape survives in
|
…was never started awaitServiceShutdown waited out its full timeout whenever PromiseManager was still pending, which is the normal state for a spec that starts its ServiceBag per test rather than in setup. SaveSlotService's suite starts the bag in 11 of 29 tests, so the other 18 each paid 5 seconds of teardown with nothing to clean -- about 90 seconds of dead wall-clock, and half of each test's timeout. A pending manager means the link that constructs it never ran, so no store exists to leak, whatever the promise is waiting on. Return instead of waiting, and skip a manager that is already destroyed, since Promise runs handlers without a pcall and would otherwise throw out of a spec's teardown. Also destroys the store in the throwing-removing-callback spec by hand. That throw latches _removing before the store leaves _datastores, so no later removal can reach it and nothing else destroys it -- the one leak the harness cannot close on its own. Corrects two doc claims found in review: the removal is tracked only after the removing callbacks are invoked, not before anything can yield, so a callback that yields or throws in that loop leaves a real window where a close finds nothing to wait on; and a destroy-only teardown breaks via a removal already in flight rather than a later PlayerRemoving, which Maid.DoCleaning's connection-first pass has already disconnected. Claude-Session: https://claude.ai/code/session_01NLMCUpaUds4ZuB9o4bkgXX
…il when destroyed Second review round. PromiseImportSlotFromSharedDataStore is the same raw shared-store read one function above the one just guarded, and it is not gated behind _loadPromise at all, so nothing upstream rejects it when the player leaves mid-read -- its continuation would call PromiseImportSlot on the destroyed binder. Both shared-store paths are declared Promise<SlotId>, so the destroyed branch now rejects instead of fulfilling nil, which read as "imported, no slot" to a caller like SaveSlotCmdrService. The rejection carries no values: Promise only reports a rejection that carries information, and neither caller has a Catch, so a message here would surface as an uncaught-exception traceback and fail the cloud gate. The stay-put test now waits on ActiveSlotId itself rather than PromiseLastActiveSlotId, which can answer from _lastActiveSlotId while the selection is still in flight -- pinning the selection directly and quiescing the chain before teardown. Comment corrected: continuations queued on a maid-owned promise before the maid dies receive its rejection, so what the liveness checks actually cover is a settle landing mid-teardown. Claude-Session: https://claude.ai/code/session_01HouVsNhp1j387CUHa4zCMq
Owner
Author
Second review round — SHIP verdict, four fixes applied (14d551b)A re-review of the saveslot half returned ship, with one gap I had wrongly waved off as out of scope:
Independently verified by the reviewer, worth recording: the positive control is genuine — Still out of scope, deliberately:
|
…o the type check passes The liveness guards added to _promiseLoadSlots make the innermost continuation's inferred return type nil, but the success path fell off the end and returned no values at all, so luau-lsp rejected it: "Not all codepaths in this function return 'nil'". npm run lint:luau was failing on the branch, which also blocks pushes through the pre-push hook. Returns nil explicitly on that path. No behavior change -- the callback's value was already unused. Claude-Session: https://claude.ai/code/session_01NLMCUpaUds4ZuB9o4bkgXX
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.
Player data was being lost on server restarts because destroying the manager tore each datastore down before PlayerRemoving could save it, and Roblox fires PlayerRemoving for everyone still in a closing server. Depending on which version of that teardown flush ran, a restart either left the session lock held (costing the next server about 76 seconds before it could steal it) or destroyed the store while players were still writing to it.
Player saves now go through the one path they went through before, and the close path waits for any removal still in flight rather than a snapshot that can be empty while a save is mid-write.
The specs had been modelling a shutdown as destroying the manager, which is not what a shutdown is, so they now simulate PlayerRemoving followed by the close the way a real server does.
Also: the crash that lock cost surfaced (saveslot)
A production server hit
HasSaveSlots:1113: attempt to call missing method '_buildSlot' of tablewhile retrying a load against exactly the held lock described above.BaseObject.Destroystrips the metatable, so that message is a method call on a destroyed object: the player left during the retry ladder, the binder was destroyed, and when the lock finally cleared the load's continuation called into it. Because a load resolves inside its ownUpdateAsynctransform, Roblox reported it as aTransform function errorand discarded that transform's write.HasSaveSlots._promiseLoadSlotswas the one load path f8716a3 missed when it fixed this same defect inSaveSlotService's selection chain, so it now takes the same shape: every hop maid-owned, every continuation re-checking the object is alive.SaveSlotLateSettle.spechad only covered a fresh player, whose load settles with no metadata and never reaches_buildSlot; it now seeds a returning player's slots, which reproduces the production stack at the same line without the guards.Verified on
src/saveslotvia Open Cloud: 189/189 with the guards, and the same suite exits 1 on that stray error without them.https://claude.ai/code/session_01NLMCUpaUds4ZuB9o4bkgXX
https://claude.ai/code/session_01HouVsNhp1j387CUHa4zCMq