fix(datastore): stop a stale lock locking a player out permanently

CONTEXT.md

@@ -0,0 +1,152 @@
+# CONTEXT.md — TEMPORARY, DELETE BEFORE MERGE
+
+**This file is a session-handoff scratchpad. It must be removed before this PR merges.**
+It exists only so a fresh Claude (Opus 4.8) chat on another machine can pick up where the last
+one left off. It is not documentation. `git rm CONTEXT.md` before merge.
+
+---
+
+## What this PR is
+
+Repo: `Quenty/NevermoreEngine` (this checkout is `D:\Source\Nevermore`).
+Fork: `buildthomas/NevermoreEngine`. Branch: `users/buildthomas/fix-datastore-teardown-during-request`.
+PR: **#784** — "fix(datastore): stop a stale lock locking a player out permanently".
+
+Two commits (branch order):
+- `f9ec95526d` fix(datastore): stop a save reporting theft over a lock the load would take
+- `700fe11024` fix(datastore): survive a teardown that lands mid-request
+
+The consuming game is `egg-hunt-2026` (sibling repo, `D:\Source\egg-hunt-2026`). It is built on the
+Nevermore + Raven package ecosystems. `@quenty/datastore` is consumed as an npm package; production
+runs whatever version the deploy's lockfile resolved.
+
+## The production incident this fixes
+
+Symptom: certain players kicked 1–2s after joining, on **every** join, message
+**"DataStore session stolen by another active session. Please message developers."** Their
+`PlayerData` key (datastore `PlayerData`, scope `SaveData_10` in prod, `SaveData_9` otherwise; key =
+`tostring(userId)`) held a root-level `lock` field naming a session from a server that died hours-to-
+days earlier. **Manually deleting only the `lock` field permanently fixes that player.** That single
+fact is what localized the bug to the lock machinery.
+
+Two composing defects:
+
+1. **The kick (root cause) — lock-age asymmetry.** `DataStoreLockHelper.AcquireLock` (load path)
+   treats a foreign lock older than `GetAutoSaveTimeSeconds() * UNLOCK_BY_DEFAULT_TIME_MULTIPLIER`
+   (300 * 2.1 = 630s) as a crashed server's and steals it. `DataStoreLockHelper.ToUnlockedProfile`
+   (save path) never consulted `LastUpdateTime` at all — ANY foreign `ActiveSession`, any age, →
+   `isValid=false` → `SessionStolen` fires (`DataStore.lua`, the `_doDataSync` transform) →
+   `PlayerDataStoreManager` kicks. So a foreign lock that survives a load is stealable on load and
+   fatal on save. The kick tears the session down before the lock is rewritten/released, so it
+   survives to the next join. Waiting cannot help — the save path has no notion of age.
+
+   **Fix:** both halves now call a shared `DataStoreLockHelper._isLockStale(parsedLockData)`. Save
+   validates a stale foreign lock instead of reporting theft; the subsequent save rewrites the lock
+   as ours, so an affected key self-heals. Fresh foreign locks and locks with no `LastUpdateTime`
+   still report theft (tests cover both).
+
+2. **The persistence — teardown mid-request.** `DataStore` destroyed while an `UpdateAsync` is in
+   flight raised out of its own transform:
+   `Transform function error ...DataStore:696: attempt to call missing method 'AcquireLock' of table`.
+   `Promise.spawn` (`src/promise/src/Shared/Promise.lua:78-84`) does `task.spawn` without retaining
+   the thread, so cancelling the maid-held promise does NOT stop the call — Roblox invokes the
+   transform anyway. By then `BaseObject.Destroy` (`src/baseobject/src/Shared/BaseObject.lua:39-42`)
+   has run `setmetatable(obj, nil)` on the store AND its helper, so method dispatch on either raises
+   and the raise aborts the write Roblox was about to commit. On the load path that kills the
+   steal-write that would have replaced the stale lock; on the save path it silently drops staged
+   data.
+
+   **Fix (IMPORTANT — earlier version was wrong):** the guard must NOT be a method on the store,
+   because post-Destroy `self:anything()` is the same crash. First attempt used
+   `self:_getSessionLockingHelper()` — a method call — which just renamed the crash. Current code
+   reads `self._sessionLockingEnabledHelper` as a RAW FIELD (safe on a metatable-less table) and
+   treats `getmetatable(helper) == nil` as proof of teardown, cancelling via the transforms'
+   existing `return nil` path. Both transforms (`_doDataSync` and `_promiseGetAsyncNoCache`) patched
+   this way. Non-session-locked stores keep old behavior via the existing `promise:IsRejected()`
+   check.
+
+## Files changed on the branch (`git diff origin/main...HEAD`)
+
+- `src/datastore/src/Server/DataStoreLockHelper.lua` — `_isLockStale`; `ToUnlockedProfile` stale
+  branch; `AcquireLock` uses the shared predicate.
+- `src/datastore/src/Server/DataStore.lua` — raw-field teardown guard in both UpdateAsync
+  transforms; new `IsLoadPending()`.
+- `src/datastore/src/Server/PlayerDataStoreManager.lua` — traceback `warn` when a store is removed
+  with its first load still in flight (gated on that window).
+- `src/datastore/src/Server/Mocks/DataStoreMock.lua` — `pcall`s the transform, records
+  `_lastTransformError`, exposes `GetLastTransformError()` so a spec can tell an aborted write from a
+  cancelled one.
+- `src/datastore/src/Server/DataStore.SessionLock.spec.lua` — save-side stale/fresh/no-timestamp
+  cases.
+- `src/datastore/src/Server/DataStore.TeardownDuringRequest.spec.lua` (new) — destroy mid-load,
+  mid-save, and no-lock-left-behind.
+
+## How the analysis was verified
+
+Two subagents (one re-derived the root cause from `origin/main`, one adversarially audited the
+diff). The audit CAUGHT the method-vs-field bug in the teardown guard described above; it has been
+fixed and the branch force-pushed. The staleness commit was found sound and regression-free (all
+existing theft specs use fresh `os.time()` locks, so none regress). Join-time saver that trips the
+kick within seconds was traced to egg-hunt's reconcilers: `EggHuntCodeAccessService`
+(`:281`/`:303`), `EggHuntRefundHoldService` (`:128`), and chapter-receipt re-delivery — all
+self-re-arming, which explains "consistently the same players."
+
+## Open issues / still to do
+
+1. **CI has never run on this PR.** `gh pr checks 784` → no checks; local `lint:luau`/test runner is
+   broken on the origin machine (`rojo --version` panics on this checkout's aftman spec,
+   `quenty/rojo@7.7.0-rc.1-quenty.4`, "missing field 'source'"). stylua + selene are clean; specs
+   were hand-traced against the mock's blocking semantics only. **The suite must get a real CI run
+   before merge** — the teardown spec especially.
+
+2. **UNRESOLVED root-cause gap (the important one).** A race cannot explain a *100% consistent*
+   per-player lockout, and we could not prove why the load's steal-write fails on every join for the
+   affected accounts. What IS certain: the stored lock is foreign at save time, which is only
+   possible if the load's write didn't land. The failure is INVISIBLE by design: the load resolves
+   from INSIDE its transform (before commit), and any later failure lands in a `:Catch` guarded by
+   `if loadPromise:IsPending()` — already false — and is discarded with no warn/log/reject.
+   Proposed minimal follow-up (NOT yet in the PR): add an `else` that `warn`s the discarded error,
+   no behavior change, so the next affected join finally names the failure (throttle? size? backend?
+   aborted transform?). Consider adding this to the PR or as a sibling.
+
+3. **Behavioral follow-ups flagged in the PR body, not fixed here:**
+   - Load resolving before its write commits (the blindness above) — fixing means resolving after
+     commit settles; a real behavior change, wanted Quenty's opinion first.
+   - A theft-dropped save `return nil`s, which is a SUCCESSFUL no-op UpdateAsync — so
+     `Save()`/`SaveAndCloseSession()` RESOLVE while data was dropped. Receipt processors
+     (`PromiseGrantChapter`) and shutdown flushes believe writes landed that didn't. Plausibly how
+     the lost chapter purchases happened (see support tools below).
+
+4. **Design decision awaiting reviewer:** the fix loosens the save-side theft rule (symmetry). The
+   alternative is keep the save strict and make the load guarantee it never leaves a foreign lock
+   behind. Called out in the PR body; commits split cleanly if Quenty prefers.
+
+5. **`min_account_age_gate`** (egg-hunt `src/scripts/Server/AccountAgeGate.lua`) was investigated as
+   a suspect and ruled out (affected players are old accounts). Mentioned only so it isn't re-chased.
+
+## Support tools built (in egg-hunt, `tools/support/`, UNTRACKED — not committed anywhere)
+
+Open Cloud Standard DataStore API, stdlib-only Python 3.8, plan/apply pattern, backups,
+`matchVersion`, userIds/attributes round-trip, read-back verify. Env:
+`ROBLOX_OPEN_CLOUD_KEY`, `ROBLOX_UNIVERSE_ID` (universe id, NOT place id).
+- `strip_session_lock.py <userid> [--apply]` — removes only `lock`. This is the live remediation;
+  fixes affected players. Refuses locks younger than 630s without `--force`.
+- `restore_chapter_access.py <userid> --from <json> [--apply]` — merges `ChapterAccess` purchase
+  records; never overwrites existing without `--overwrite-conflicts`; validates hard.
+- `overwrite_player_data.py <userid> --from <json> [--apply]` — full-key replace, for reproducing a
+  broken account's state on a test account. `--strip-lock` for a control. userIds default to the
+  TARGET, not the source.
+
+Repro note: a planted stale lock alone does NOT reproduce on a clean join (the load steals it). To
+reproduce the kick deterministically, get in-game first, THEN plant a foreign lock and wait for a
+save.
+
+## Conventions (IMPORTANT)
+
+- **No AI/Claude attribution** in commits or PRs — no trailers, no footers, plain technical prose.
+  This is a standing user preference; do not add them.
+- Package is `--!strict`, stylua-formatted (`stylua.toml`), selene-linted. Repo mandates LF via
+  `.gitattributes`; on Windows some files check out CRLF — normalize before stylua or every line
+  reads as a diff.
+- Run tests with `nevermore test --cloud` from repo root (local mode reports false passes). Specs
+  live next to code as `*.spec.lua`; tear down `ServiceBag`/objects via a `setup()`/`destroy()` Maid.

src/datastore/src/Server/DataStore.SessionLock.spec.lua

@@ -166,6 +166,37 @@ describe("DataStoreLockHelper.ToUnlockedProfile (save-side thief detection)", fu
 		controller:destroy()
 	end)
 
+	-- The save side has to reach the same verdict as AcquireLock on the same lock. Where it did
+	-- not, a stale foreign lock that outlived a load made the next save report theft, kicking the
+	-- player and dropping the write, forever -- the lock was never rewritten, so every later
+	-- session repeated it.
+	it("validates a profile whose foreign lock has gone stale, as AcquireLock would", function()
+		local controller = DataStoreTestUtils.setup()
+		local helper = controller.newLockHelper()
+		-- Older than GetAutoSaveTimeSeconds() * 2.1 (300 * 2.1 = 630s).
+		local result = helper:ToUnlockedProfile(lockedBy(foreignSession(), os.time() - 700, { coins = 5 }))
+		expect(result.isValid).toEqual(true)
+		expect(result.unlockedProfile.coins).toEqual(5)
+		expect(result.unlockedProfile.lock).toEqual(nil)
+		controller:destroy()
+	end)
+
+	it("still invalidates a foreign lock that is only slightly old", function()
+		local controller = DataStoreTestUtils.setup()
+		local helper = controller.newLockHelper()
+		local result = helper:ToUnlockedProfile(lockedBy(foreignSession(), os.time() - 100, { coins = 5 }))
+		expect(result.isValid).toEqual(false)
+		controller:destroy()
+	end)
+
+	it("still invalidates a foreign lock with no LastUpdateTime (cannot judge staleness)", function()
+		local controller = DataStoreTestUtils.setup()
+		local helper = controller.newLockHelper()
+		local result = helper:ToUnlockedProfile(lockedBy(foreignSession(), nil, { coins = 5 }))
+		expect(result.isValid).toEqual(false)
+		controller:destroy()
+	end)
+
 	it("validates a profile that has no lock", function()
 		local controller = DataStoreTestUtils.setup()
 		local helper = controller.newLockHelper()

src/datastore/src/Server/DataStore.TeardownDuringRequest.spec.lua

@@ -0,0 +1,93 @@
+--!nonstrict
+--[[
+	Teardown that lands while a datastore request is already in flight.
+
+	[Promise.spawn] runs the request on a thread it does not retain, so the promise a teardown
+	cancels is not the call -- Roblox invokes the transform regardless, after [BaseObject.Destroy]
+	has stripped the session-locking helper's metatable. Reaching through the field at that point
+	raises "attempt to call missing method" from inside the transform, which aborts the write
+	Roblox was about to commit. The transform has to notice the store is gone and cancel instead.
+
+	@class DataStore.TeardownDuringRequest.spec.lua
+]]
+local require = require(script.Parent.loader).load(script)
+
+local DataStoreTestUtils = require("DataStoreTestUtils")
+local Jest = require("Jest")
+local PromiseTestUtils = require("PromiseTestUtils")
+
+local describe = Jest.Globals.describe
+local expect = Jest.Globals.expect
+local it = Jest.Globals.it
+
+-- Waits out the drain by watching for the failure itself: with the guard in place nothing is ever
+-- recorded and this spends its whole budget, which is also how long the request needs to land.
+local function drain(controller)
+	PromiseTestUtils.awaitValue(function()
+		return controller.mock:GetLastTransformError() ~= nil
+	end, 2)
+end
+
+describe("teardown during an in-flight load", function()
+	it("cancels the write instead of raising out of the transform", function()
+		local controller = DataStoreTestUtils.setup()
+		controller.mock:BlockRequests()
+
+		local dataStore = controller.newSessionLockedStore()
+		local promise = dataStore:PromiseLoadSuccessful()
+		expect(PromiseTestUtils.awaitSettled(promise, 1)).toEqual(false)
+
+		dataStore:Destroy()
+		controller.mock:UnblockRequests()
+		drain(controller)
+
+		expect(controller.mock:GetLastTransformError()).toEqual(nil)
+		expect(controller.mock:GetRaw("player_1")).toEqual(nil)
+
+		controller:destroy()
+	end)
+
+	it("leaves no lock behind for the next session to contend with", function()
+		local controller = DataStoreTestUtils.setup()
+		controller.mock:BlockRequests()
+
+		local dataStore = controller.newSessionLockedStore()
+		dataStore:PromiseLoadSuccessful()
+		expect(PromiseTestUtils.awaitSettled(dataStore:PromiseLoadSuccessful(), 1)).toEqual(false)
+
+		dataStore:Destroy()
+		controller.mock:UnblockRequests()
+		drain(controller)
+
+		local raw = controller.mock:GetRaw("player_1")
+		expect(raw == nil or raw.lock == nil).toEqual(true)
+
+		controller:destroy()
+	end)
+end)
+
+describe("teardown during an in-flight save", function()
+	it("cancels the write instead of raising out of the transform", function()
+		local controller = DataStoreTestUtils.setup()
+
+		local dataStore = controller.newSessionLockedStore()
+		if not controller.awaitOwn(dataStore) then
+			expect("load never settled").toEqual("load settled")
+			controller:destroy()
+			return
+		end
+
+		controller.mock:BlockRequests()
+		dataStore:Store("coins", 5)
+		local savePromise = dataStore:Save()
+		expect(PromiseTestUtils.awaitSettled(savePromise, 1)).toEqual(false)
+
+		dataStore:Destroy()
+		controller.mock:UnblockRequests()
+		drain(controller)
+
+		expect(controller.mock:GetLastTransformError()).toEqual(nil)
+
+		controller:destroy()
+	end)
+end)

src/datastore/src/Server/DataStore.lua

@@ -204,6 +204,16 @@ function DataStore.SetSessionLockingEnabled(self: DataStore, sessionLockingEnabl
 	end
 end
 
+--[=[
+	Whether the first load is still outstanding. A store destroyed in this window is destroyed
+	under an active request.
+
+	@return boolean
+]=]
+function DataStore.IsLoadPending(self: DataStore): boolean
+	return self._firstLoadPromise ~= nil and self._firstLoadPromise:IsPending()
+end
+
 --[=[
 	Sets session messaging enabled.
 
@@ -587,8 +597,20 @@ function DataStore._doDataSync(
 		promise:Resolve(
 			maid:GivePromise(
 				DataStorePromises.updateAsync(self._robloxDataStore, self._key, function(original, datastoreKeyInfo)
-					if self._sessionLockingEnabledHelper then
-						local unlocked = self._sessionLockingEnabledHelper:ToUnlockedProfile(original)
+					-- A raw field read plus getmetatable, never a method call. This transform can
+					-- run after Destroy: [Promise.spawn] does not retain the request thread, so the
+					-- promise a teardown cancels is not the call -- and by then [BaseObject.Destroy]
+					-- has stripped the metatable of this store AND of its helpers, so any method
+					-- dispatch on either (including on self) raises out of the transform and aborts
+					-- the write Roblox was about to commit. A helper whose metatable is gone is
+					-- proof of teardown: cancel the write instead.
+					local lockHelper = self._sessionLockingEnabledHelper
+					if lockHelper ~= nil and getmetatable(lockHelper :: any) == nil then
+						return nil
+					end
+
+					if lockHelper then
+						local unlocked = lockHelper:ToUnlockedProfile(original)
 						if unlocked.isValid then
 							original = unlocked.unlockedProfile
 						else
@@ -636,8 +658,8 @@ function DataStore._doDataSync(
 						metadata = datastoreKeyInfo:GetMetadata()
 					end
 
-					if self._sessionLockingEnabledHelper then
-						result = self._sessionLockingEnabledHelper:ToLockedProfile(result, doCloseSession)
+					if lockHelper then
+						result = lockHelper:ToLockedProfile(result, doCloseSession)
 					end
 
 					return result, userIdList, metadata
@@ -693,7 +715,17 @@ function DataStore._promiseGetAsyncNoCache(self: DataStore): Promise.Promise<()>
 								)
 							end
 
-							local lockResult = self._sessionLockingEnabledHelper:AcquireLock(data, canStealLock)
+							-- A raw field read plus getmetatable, never a method call on self -- see
+							-- the teardown guard in _doDataSync for why. A stripped helper means this
+							-- store was destroyed while the request was in flight; every promise this
+							-- load would settle was already rejected by the teardown, so cancel the
+							-- write.
+							local lockHelper = self._sessionLockingEnabledHelper
+							if lockHelper == nil or getmetatable(lockHelper :: any) == nil then
+								return nil
+							end
+
+							local lockResult = lockHelper:AcquireLock(data, canStealLock)
 							if not lockResult.isValid then
 								if self._sessionMessagingEnabledHelper and tryMessagingServiceSessionClose then
 									-- Gracefully kick to avoid losing memory