From f9ec95526db028a64765b783d71d9b2a5a22382f Mon Sep 17 00:00:00 2001 From: unknown <40648899+buildthomas@users.noreply.github.com> Date: Mon, 3 Aug 2026 01:44:16 +0200 Subject: [PATCH 1/3] fix(datastore): stop a save reporting theft over a lock the load would take Lock age decided one question and not the other. AcquireLock treats a foreign lock older than GetAutoSaveTimeSeconds() * 2.1 as belonging to a crashed server and takes it. ToUnlockedProfile, guarding the save, only ever asked whether the lock is ours -- LastUpdateTime appears nowhere in it. So a foreign lock that outlives a load is stealable on load and fatal on save, at any age. The save fires SessionStolen, the manager kicks the player with "DataStore session stolen by another active session", and the write that was about to land is dropped. Nothing rewrites or releases the lock, so the next session repeats it exactly. Waiting does not help: the save path has no notion of age to wait out. Observed in production as players locked out indefinitely, recoverable only by deleting the lock from the key by hand. Both halves now ask _isLockStale, so they cannot disagree about the same lock. The rule loosened is a save by a session whose lock was taken while it was gone for longer than the protocol's own definition of dead. A fresh foreign lock still reports theft and a lock with no LastUpdateTime still does, both covered by tests. Worth a second opinion on: the alternative reading is that the save side should stay strict and the load side should guarantee it never leaves a foreign lock behind, which is the other commit on this branch. --- .../src/Server/DataStore.SessionLock.spec.lua | 31 ++++++++++++ .../src/Server/DataStoreLockHelper.lua | 49 +++++++++++++++---- 2 files changed, 71 insertions(+), 9 deletions(-) diff --git a/src/datastore/src/Server/DataStore.SessionLock.spec.lua b/src/datastore/src/Server/DataStore.SessionLock.spec.lua index df993eb8285..2fbfe5c0351 100644 --- a/src/datastore/src/Server/DataStore.SessionLock.spec.lua +++ b/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() diff --git a/src/datastore/src/Server/DataStoreLockHelper.lua b/src/datastore/src/Server/DataStoreLockHelper.lua index a3eda4c709e..bc35eaf9faf 100644 --- a/src/datastore/src/Server/DataStoreLockHelper.lua +++ b/src/datastore/src/Server/DataStoreLockHelper.lua @@ -102,6 +102,16 @@ function DataStoreLockHelper.ToUnlockedProfile(self: DataStoreLockHelper, origin isValid = true, unlockedProfile = self:ToRawUnlockedProfile(original), } + elseif self:_isLockStale(parsedLockData) then + -- Held by a session this protocol already considers dead. AcquireLock would take this + -- lock rather than wait for it, so reporting theft here would have the two halves + -- disagreeing about the same predicate -- and the cost of that disagreement is a + -- kicked player and the save that was about to land, over a session that died hours + -- ago and is never coming back to claim it. + return { + isValid = true, + unlockedProfile = self:ToRawUnlockedProfile(original), + } else -- Someone else owns the lock return { @@ -159,6 +169,35 @@ function DataStoreLockHelper.ToLockedProfile(self: DataStoreLockHelper, original end end +--[=[ + Whether a lock is old enough that the session holding it is assumed dead. + + A session refreshes its lock whenever it actually writes, so a lock that has gone several + auto-save intervals without moving most likely belongs to a server that crashed. (A live but + fully idle session can go quiet that long too, since a save with nothing staged skips the + write -- the load side has always accepted stealing such a lock, and that session's own next + real save then reads as theft and closes it out.) Both halves of the protocol ask this + predicate, and they have to agree: a lock the load will take is one the save must not report + as theft. + + @private + @param parsedLockData LockData? + @return boolean +]=] +function DataStoreLockHelper._isLockStale(self: DataStoreLockHelper, parsedLockData: LockData?): boolean + if parsedLockData == nil or parsedLockData.LastUpdateTime == nil then + return false + end + + local autoSaveSeconds = self._dataStore:GetAutoSaveTimeSeconds() + if not autoSaveSeconds then + return false + end + + local timeElapsed = os.time() - parsedLockData.LastUpdateTime + return timeElapsed > (autoSaveSeconds * UNLOCK_BY_DEFAULT_TIME_MULTIPLIER) +end + function DataStoreLockHelper._isInSession(self: DataStoreLockHelper, sessionData: LockedSessionData): boolean local ourData = self:_ourCurrentSessionData() @@ -266,15 +305,7 @@ function DataStoreLockHelper.AcquireLock(self: DataStoreLockHelper, data: any, c -- We're locked out, but there's conditions where it's ok to steal the lock - local lockStealingOk = canStealLock - if parsedLockData and parsedLockData.LastUpdateTime then - -- Assume the server crashed if it's been a while since the last update - local timeElapsed = os.time() - parsedLockData.LastUpdateTime - local autoSaveSeconds = self._dataStore:GetAutoSaveTimeSeconds() - if autoSaveSeconds and timeElapsed > (autoSaveSeconds * UNLOCK_BY_DEFAULT_TIME_MULTIPLIER) then - lockStealingOk = true - end - end + local lockStealingOk = canStealLock or self:_isLockStale(parsedLockData) if ALWAYS_STEAL_LOCKS_IN_STUDIO and RunService:IsStudio() then lockStealingOk = true From 700fe11024f7d69c02804956e3e9651ee3671c51 Mon Sep 17 00:00:00 2001 From: unknown <40648899+buildthomas@users.noreply.github.com> Date: Mon, 3 Aug 2026 01:44:17 +0200 Subject: [PATCH 2/3] fix(datastore): survive a teardown that lands mid-request A DataStore destroyed while an UpdateAsync is in flight raises out of its own transform: Transform function error ...datastore.Server.DataStore:696: attempt to call missing method 'AcquireLock' of table Promise.spawn hands the request to a thread it does not retain, so the promise a teardown cancels is not the call -- Roblox invokes the transform regardless. By then BaseObject.Destroy has run setmetatable(obj, nil) on the store AND on its session-locking helper (the maid strips the helper first, then the store itself), so both keep their identity and lose every method. The first method dispatch the transform performs raises, and the raise aborts the write Roblox was about to commit. The abort is worse than the log line suggests. On the save path (line 591, ToUnlockedProfile) a player's staged data is silently dropped. On the load path it kills the steal-write that would have replaced a stale foreign lock, so the lock survives for the next session to trip over -- invisibly, because the load already resolved from inside the transform and the late failure lands in a catch guarded by IsPending, which is already false. The guard therefore cannot be a method on the store: post-Destroy, calling self:anything() is the same class of crash. Both transforms instead read self._sessionLockingEnabledHelper as a raw field -- safe on a metatable-less table -- and treat a helper whose metatable is gone as proof of teardown, cancelling the write through the transforms' existing cancel path. A store without session locking keeps its old behavior: the save transform's IsRejected check cancels it. Also logs the caller when a store is removed with its first load still outstanding, which is the window this happens in. Gated on that window rather than logged per removal, which would be one traceback per player per shutdown. DataStoreMock records a throwing transform so a spec can tell an aborted write from a cancelled one; the pcall in DataStorePromises otherwise swallows it. --- .../DataStore.TeardownDuringRequest.spec.lua | 93 +++++++++++++++++++ src/datastore/src/Server/DataStore.lua | 42 ++++++++- .../src/Server/Mocks/DataStoreMock.lua | 23 ++++- .../src/Server/PlayerDataStoreManager.lua | 11 +++ 4 files changed, 163 insertions(+), 6 deletions(-) create mode 100644 src/datastore/src/Server/DataStore.TeardownDuringRequest.spec.lua diff --git a/src/datastore/src/Server/DataStore.TeardownDuringRequest.spec.lua b/src/datastore/src/Server/DataStore.TeardownDuringRequest.spec.lua new file mode 100644 index 00000000000..df17ff37634 --- /dev/null +++ b/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) diff --git a/src/datastore/src/Server/DataStore.lua b/src/datastore/src/Server/DataStore.lua index 16421ca25d3..d9ef7d8e35d 100644 --- a/src/datastore/src/Server/DataStore.lua +++ b/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 diff --git a/src/datastore/src/Server/Mocks/DataStoreMock.lua b/src/datastore/src/Server/Mocks/DataStoreMock.lua index f45be12afa4..d11629439c7 100644 --- a/src/datastore/src/Server/Mocks/DataStoreMock.lua +++ b/src/datastore/src/Server/Mocks/DataStoreMock.lua @@ -90,6 +90,7 @@ export type DataStoreMock = typeof(setmetatable( _errorInjector: ErrorInjector?, _blocked: boolean, _maxValueLength: number?, + _lastTransformError: string?, }, {} :: typeof({ __index = DataStoreMock }) )) @@ -129,6 +130,7 @@ function DataStoreMock.new(name: string?, scope: string?): DataStoreMock self._errorInjector = nil self._blocked = false self._maxValueLength = nil + self._lastTransformError = nil return self end @@ -251,6 +253,16 @@ function DataStoreMock.GetCallCount(self: DataStoreMock, method: string?): numbe return self._callCounts[method] or 0 end +--[=[ + The message from the most recent UpdateAsync transform that threw, or nil if none has. Lets a + spec assert that a transform bailed cleanly rather than raising. + + @return string? +]=] +function DataStoreMock.GetLastTransformError(self: DataStoreMock): string? + return self._lastTransformError +end + --[=[ Directly seeds a stored value without datastore semantics (no version bump, no failure injection). For test setup. @@ -463,7 +475,16 @@ function DataStoreMock.UpdateAsync( local current = deepCopy(self._store[key]) local keyInfo = self:_makeKeyInfo(key) - local newValue, userIds, metadata = transformFunction(current, keyInfo) + -- Recorded rather than only re-raised: a transform that throws is reported by Roblox as a + -- "Transform function error" and then swallowed by the pcall in [DataStorePromises], so a spec + -- has no other way to tell an aborted write from a cancelled one. Re-raised with level 0 to + -- keep the original message, the way the real UpdateAsync surfaces it. + local ok, newValue, userIds, metadata = pcall(transformFunction, current, keyInfo) + if not ok then + self._lastTransformError = tostring(newValue) + error(newValue, 0) + end + if newValue == nil then -- Update cancelled; nothing written return nil, keyInfo diff --git a/src/datastore/src/Server/PlayerDataStoreManager.lua b/src/datastore/src/Server/PlayerDataStoreManager.lua index 18eaf98f8b2..739be25ef9d 100644 --- a/src/datastore/src/Server/PlayerDataStoreManager.lua +++ b/src/datastore/src/Server/PlayerDataStoreManager.lua @@ -373,6 +373,17 @@ function PlayerDataStoreManager._removePlayerDataStore(self: PlayerDataStoreMana return end + -- Removing a store whose first load has not settled destroys it under an active request. The + -- store survives that now (see the teardown guards in DataStore._doDataSync and + -- DataStore._promiseGetAsyncNoCache), but the removal still discards a load nobody asked to + -- cancel, so name the caller that did it. Gated on the in-flight window rather than logged + -- per removal, which would be one traceback per player per shutdown. + if (datastore :: any):IsLoadPending() then + warn( + `[PlayerDataStoreManager] - Removing {userId} while its first load is still in flight.\n{debug.traceback()}` + ) + end + self._removing[userId] = true local removingPromises: { Promise.Promise } = {} From ba90d52d3263480c167c02ead01f2afda6040a9e Mon Sep 17 00:00:00 2001 From: unknown <40648899+buildthomas@users.noreply.github.com> Date: Fri, 28 Aug 2026 15:38:56 +0200 Subject: [PATCH 3/3] chore: add temporary CONTEXT.md session-handoff notes Scratchpad for continuing this work on another machine. Marked at the top as temporary; delete before merge. --- CONTEXT.md | 152 +++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 152 insertions(+) create mode 100644 CONTEXT.md diff --git a/CONTEXT.md b/CONTEXT.md new file mode 100644 index 00000000000..55a70ea3b23 --- /dev/null +++ b/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 [--apply]` — removes only `lock`. This is the live remediation; + fixes affected players. Refuses locks younger than 630s without `--force`. +- `restore_chapter_access.py --from [--apply]` — merges `ChapterAccess` purchase + records; never overwrites existing without `--overwrite-conflicts`; validates hard. +- `overwrite_player_data.py --from [--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.