Hold the game read lock from resolve to last dereference (Resolved<T>)

[ResurrectedTrader]

What races

Handle accessors called ResolvePtr(), which took a GameReadLock only for the resolve itself. The caller then dereferenced the returned raw game pointer with no lock held (review item G18). The game thread holds the write lock for its whole frame, so between the resolve and the read it could free or relink the unit / room / control being read.

Design: Resolved<T>

Resolved<T> (contract, src/contract/game/HandleCache.h) is a move-only RAII value that:

• takes a GameReadLock before the resolve,
• resolves through the existing ResolvePtr(), so every resolve still goes through the per-frame HandleCache. Caching and its invalidation are unchanged,
• holds the lock for its whole lifetime, so the resolve and every dereference happen under one lock,
• has one way to reach the pointer: -> / * for the struct, and an implicit operator T*() const& to pass it on. That conversion also serves the null test (if (!u)). It is deleted on a temporary (operator T*() const&& = delete), so T* p = Resolve<T>(); does not compile and a pointer can't outlive its lock. There is no Get() and no separate operator bool.

Each handle (Unit, Room, Level, Party, Control) gets a private Resolve<T>(). T is the backend's own struct type, so the contract never names a backend type and the backend stops hand-casting (AsUnit(ResolvePtr())):

uint32_t Unit::Mode() const {
    const auto u = Resolve<D2UnitStrc>();
    return u ? u->dwAnimMode : 0U;
}

All lod114d accessors read through it. That is 116 methods: 73 on Unit, 16 on Room, 2 on Level, 10 on Party, 15 on Control, plus the Unit/Room operator==.

ResolvePtr() no longer locks. It asserts in debug builds that the calling thread already holds the game lock (IsGameLockHeld(): a read lock, or the game thread's write lock). Validity tests (operator bool) and static factories that read game memory directly hold an explicit GameReadLock.

Unlocked raw-pointer reads fixed

Handle accessors:

• Level::Name called ResolvePtr() with no lock.
• Unit::GetSkillLevel dereferenced its resolved pointer with no lock.

Free functions in GameHelpers.cpp:

• ClickItem read the item it looked up in the hash table before taking the lock. The lookup and every read now happen under one lock and are copied out before the dispatch waits on the game thread.
• ResolveBeltSize walked the player's inventory unlocked.
• IsScrollingText walked Storm's live window-handler lists unlocked.
• CheckUnitCollision looked up two units and ran the collision test unlocked.
• ClickPartyMember handed a roster pointer to the game after its lock scope had ended. The lock now spans the dispatch.

Deliberately left as they are:

• GetGameState dereferences the player chain without a lock. It is hot and called from many non-script threads, and adding a lock there changes blocking behaviour everywhere, so it needs its own change.
• The Bnet/session getters and SwapWeapon's expansion check read the long-lived Bnet session struct.
• Say copies into static chat buffers.
• The PlugY page helpers only run under their callers' locks.
• Menu.cpp goes through handles or runs inside GameThread::Execute.
• SubmitItem, RevealLevel and the ClickMapAt / click dispatches dereference only inside GameThread::Execute, on the game thread.

One lock, tightly scoped

• Bridge::Lock() is gone. It was just static GameReadLock Lock() { return {}; }, a second name for the same lock.
• Every lock site was narrowed. The lock is now taken right before the first game read, after argument checks, WaitForGameReady and V8 argument extraction. It is released after the last read, so V8 allocation and script-facing work happen outside it. Reads that must agree with each other (a walk, or a level and its rooms) stay under one lock.
  • Level::Get (review comment), FindNextInventoryItem, Unit::ItemCost / TakeWaypoint / Overhead / GetSkillName, and AcceptTrade / TradeOK / ClickBodyLocation / ClickContainerSlot / GetDialogLines in the backend.
  • getCollision(level, x, y), getRoom, getPresetUnit, getPresetUnits and getControls now copy the game data out under one lock and build the JS values after it.
  • 22 binding locks that only wrapped a single self-locking (or waiting) backend call are removed. They are in JSRoom, JSStashTab, JSUnit, and in getParty, getStashTabs, getDialogLines, clickItem, getDistance, acceptTrade, tradeOk and revealLevel. Several of them held the lock across V8 allocation, which the docs had flagged as a latent stall. The getDistance one also covered V8 object-property reads.

Review fixes

• getDistance resolves a JS Unit's validity and position under one lock. Only that branch is locked: plain {x, y} objects are still read outside the lock, because their property getters run script.
• Resolved<T>'s operator* is deleted on temporaries, like the T* conversion, and the resolver constructor is constrained so a copy attempt hits the deleted copy constructor. Both are covered by static_asserts.
• IsTradeAccepted, GetRecentTradeId and IsTradeBlocked take the read lock.
• The doc example now matches the real getControls loop.

Stale-pointer check (release epoch)

A wait that hands the lock back invalidates every pointer resolved before it. That covers GameThread::Execute, PollUntil, and anything else built on GameReadLockReleaser.

• GameReadLock::ReleaseEpoch() is a thread-local counter. GameReadLockReleaser bumps it when it releases a held read lock, and GameWriteLockReleaser bumps it when it yields the write lock.
• Resolved<T> snapshots the counter when it is created and, in debug builds, asserts it is unchanged on ->, * and the T* conversion.
• Release builds compile the check out.

Methods that wait

Method	Wait	Handling
Unit::Move	ClickMapAt (Execute) + sleep	Resolved in an inner scope that copies out the click point, then waits
Unit::Interact	ClickMapAt(unit) (Execute)	inner scope for the packet paths; ClickMapAt re-resolves by id on the game thread
Unit::EquipItem	GameThread::Execute	inner scope copies out bodyLoc, and the checks run in the same order as before
Control::Click	sleeps between PostMessages	inner scope copies out the click coordinates
Control::SetText	GameThread::Execute	the task re-resolves with self.Resolve<D2WinControlStrc>() on the game thread
Room::Reveal	GameThread::Execute	inner-scope precheck, then the task re-resolves on the game thread
ClickItem	GameThread::Execute	lookup and reads in one locked scope, copied out before the dispatch

The epoch assert did not surface any other stale use.

No JS-visible behaviour changes.

Verification

• build.ps1 format / check-format: clean
• build.ps1 Release (Win32) and build.ps1 Release -Platform x64: 0 errors
• build.ps1 Debug (Win32, asserts on): 0 errors. The Debug js_tests.exe passes too.
• build.ps1 test: 164/164 (157 existing + 7 new). The new cases cover Resolved<T> holding, releasing, nesting and moving the lock; static_asserts that the rvalue conversion is deleted; the release epoch across both releasers; and IsGameLockHeld().
• python scripts/gen_enum_names.py --check: up to date
• scripts/lint.ps1 -Jobs 8 -NoCache: 125 passed, 0 failed

Not validated in a live game.

🤖 Generated with Claude Code

[ResurrectedTrader]

Could move below the zero check.

[ResurrectedTrader]

Done - moved below the check. Narrowed the other lock sites the same way: lock taken after argument checks and released before V8 work in FindNextInventoryItem, Unit::ItemCost/TakeWaypoint/Overhead/GetSkillName, AcceptTrade/TradeOK/ClickBodyLocation/ClickContainerSlot/GetDialogLines, and getCollision(level,x,y)/getRoom/getPresetUnit/getPresetUnits/getControls (data copied out under one lock, JS values built after); and dropped 22 binding locks that only wrapped a single self-locking or waiting call (JSRoom, JSStashTab, JSUnit, getParty, getStashTabs, getDialogLines, clickItem, getDistance, acceptTrade, tradeOk, revealLevel). Consistent walks still hold one lock throughout.