Hold the game read lock across a binding's check and its reads (LockedHandle)

[ResurrectedTrader]

Follow-up to #56: fixes the check-then-use races in the JS bindings.

Problem

Bindings tested a handle in one lock window and read it in another:

auto* data = Unwrap(args.This());
if (!*data) return;                        // resolve #1, under its own lock
args.GetReturnValue().Set(data->GetStat(n)); // resolve #2, under another lock

If the game thread freed the unit between the two windows, the script got 0 / "" instead of undefined. This was never memory-unsafe, because every read re-resolves, but the answer was inconsistent.

Design

• LockedHandle<T> (api/core/LockedHandle.h) holds a game::GameReadLock for its lifetime and exposes the handle through -> / *.
  • Its own operator bool tests the wrapper pointer. *data's bool is still "does the game object exist".
  • It is movable: the destination takes its own re-entrant lock before the source releases, so the lock is never dropped. It cannot be copied or move-assigned.
• ClassBase::Unwrap returns a LockedHandle for the game handle natives and a raw pointer for everything else.
  • The game handle natives are game::Unit, Room, Level, Party, Control and StashTab, named by the GameHandle concept.
  • Everything else keeps its raw pointer and its call sites are unchanged.
  • There is no unlocked escape hatch for a game handle.
• The accessors' own Resolved<T> locks nest inside the guard as recursive re-entries, which are free.

So most of the diff is auto* data = Unwrap(...) becoming const auto data = Unwrap(...), with the check and the reads unchanged below it.

Rules while a guard is alive

Documented in docs/game_thread_safety.md under "Bindings - Unwrap returns a LockedHandle".

• Arguments first, then the guard. A binding converts its untyped arguments before its Unwrap, then takes one guard and checks and reads under it (getStat, checkCollision, shop, overhead, moveNPC, getRoom). Arguments already gated by IsString / IsNumber / IsUint32 run no script and stay where they were.
• Never run script under it. No callbacks, ExecuteEvents or Function::Call. The Unit iterator reads Pos() in a scoped block and calls the array iterator after it.
• Never wait under it, on either backend. Anything that can wait on the game thread runs on a copy of the handle after the guard's scope closes:
  • Unit: move, interact / TakeWaypoint, useMenu, shop, description, setSkill's bind loop (it pumps script events)
  • Control: click, setText, the text getter and setter, getText()
  • room.reveal, StashTab.click / depositGold / withdrawGold
  • Globals: clickMap(unit), clickItem(item) (both shapes), clickParty, moveNPC, revealLevel
• Build large results after it. Arrays and lists of handles (getStat(-1/-2), collision grids, items, rooms, exits, presets) are copied out in a scoped block and built after it. Numbers and single strings are built directly under the guard.

One documented exception to "one game state per guard": the first resolve of a room or level that is not built yet runs level init on the game thread, which hands the lock back meanwhile. This is safe because a guard holds identity handles, not raw pointers.

Other notable call-site changes

• JSControl::MenuOnly returns a LockedHandle<Control>, so the Menu-state test runs under the same lock.
• getDistance: the unit branch of getObjectPoint uses the guard in place of Hold the game read lock from resolve to last dereference (Resolved<T>) #56's ad-hoc GameReadLock, and the player branches go through a getPlayerPoint that tests and reads under one lock.
• These hold one explicit GameReadLock across a Unit::Player() / InteractingNPC() test and the reads after it: global getMercHP, getArea() (default area), revealLevel (the reveal itself runs after the lock), unit.getRepairCost.

JS-visible behaviour is unchanged apart from the race fix and one ordering change: an untyped argument's valueOf / toString now runs before the unit validity check in getStat, checkCollision, shop, overhead, moveNPC and getRoom(x, y). moveNPC returns straight away if converting its coordinates threw.

Trade-offs

• type and gid read cached identity fields and never needed a lock, and neither do StashTab.kind / index. Every game-handle Unwrap now takes one anyway: an uncontended recursive acquire.

Tests

tests/runtime/api/LockedHandleTest.cpp needs no V8:

• static_asserts that the six game handles satisfy GameHandle and that PresetUnitInfo, ExitInfo and int32_t do not
• copy / move traits
• the lock is held for the guard's lifetime and released after it
• an empty guard still holds the lock
• a move keeps recursion depth 1
• re-entrant nesting with an accessor's lock
• a writer on another thread is blocked until the guard is released

Verification

• build.ps1 Release (Win32), build.ps1 Release -Platform x64 and build.ps1 Debug all succeed.
• build.ps1 test: 169/169 test cases pass.
• build.ps1 check-format is clean.
• scripts/lint.ps1 -NoCache: 126/126 files pass.
• An independent review checked the diff against both backends (lod114d and the D2R branch). It looked for waits under a guard, script run under a guard, JS-visible changes, and leftover over-engineering. It found no waits or script under a guard. Its cleanups are applied: a redundant flag in interact, a contradiction in the docs, and a stray comment.
• Not live-tested in game.

🤖 Generated with Claude Code

[ResurrectedTrader]

This looks dubious

[ResurrectedTrader]

Simplified: the mask is converted first, then one unwrap of both units and one check under it. The same "arguments first, then one guard" rule now applies across the PR (getStat, shop, overhead, moveNPC), and the test-twice guidance is gone from the docs.