Preserve SafetyHook concurrency finding as a committed upstream repro

A multithreaded test that tight-looped InlineHook enable()/disable() while
other threads called the hooked function flaked ~1/10. Isolation proved this is
a SafetyHook limitation, not our code: with the hook created once (no install
race, no trampoline UAF), tight-loop toggling AVs ~1/3 of runs in Debug
(0xC0000005, faulting RIP in the target body), while a no-toggle control is
clean at ~60M calls. enable()/disable() re-patch the prologue in place under a
VEH page-trap that only relocates a thread parked ON the prologue; a thread in
the function body faults on the briefly-non-exec page and relies on instruction
retry, which under rapid toggling races a half-rewritten prologue.

Rather than silently drop the flaky test, preserve the finding:
- tools/sh_concurrency_repro/: minimal, committed, non-CI reproducer
  (coop_sh_concurrency_repro; --callonly is the control). Surfaces 5/16 AVs.
- docs/safetyhook-concurrency.md: upstream-ready write-up (mechanism + fix
  directions + why it does not affect us).
- README lessons-learned + memory updated; tests/CMakeLists cross-references it.

Our code stays in SafetyHook's safe envelope (install/remove reconciled from a
single tick-bounded worker thread, never a tight loop), so the mock_game_test
storm is reliably green; the persistent-trampoline contract is covered
deterministically by hook_install_test + detour_gate_test. Removes the temp
_sh_probe wiring.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

docs/safetyhook-concurrency.md

@@ -0,0 +1,105 @@
+# SafetyHook: `InlineHook::enable()/disable()` are unsafe under rapid concurrent toggling
+
+**Component:** SafetyHook (vendored as a submodule at `third_party/safetyhook`)
+**Version:** `v0.7.0-1-g2f28386` (commit `2f283866189c5c728384ae8b9e7f58c268ae036c`)
+**Platform observed:** Windows x64, MSVC, Release & Debug
+**Severity:** crash (access violation) in the *caller* of a hooked function
+**Reproducer:** `tools/sh_concurrency_repro/` (built target `coop_sh_concurrency_repro`)
+
+This is a write-up of a limitation we hit in CoopAllTheThings, kept so it can be filed upstream
+and so we don't silently rediscover it. Our shipping code does **not** depend on the unsafe pattern
+(see "Why this does not affect CoopAllTheThings" below); this documents the underlying behaviour.
+
+## Summary
+
+`InlineHook::enable()` and `InlineHook::disable()` rewrite the target function's prologue bytes in
+place, guarding concurrently-executing threads with a VEH-based "trap" (`trap_threads` in
+`src/os.windows.cpp`). That trap reliably rescues a thread **parked on the prologue bytes being
+patched**, but it does *not* safely handle a thread executing in the function **body** that faults
+because the whole code page was made non-executable during the patch — when `enable()`/`disable()`
+are called in a **tight loop** concurrent with calls to the hooked function. Under that load the
+re-patch (a non-atomic byte copy) races a faulting caller's instruction retry, and the process
+crashes with an access violation whose faulting RIP is inside the target's body (not on the prologue).
+
+## Reproducer
+
+`tools/sh_concurrency_repro/main.cpp`. The hook is created **once** and never recreated/destroyed, so
+no install race and no trampoline use-after-free are involved — the only variable is concurrent
+`enable()`/`disable()` vs. calls.
+
+- **default mode:** one thread tight-loops `disable()`/`enable()`; two threads hammer `target_fn`.
+- **`--callonly` (control):** the two caller threads hammer; **no** toggling.
+
+Run it repeatedly:
+
+```powershell
+1..16 | % { & .\coop_sh_concurrency_repro.exe; "exit=$LASTEXITCODE" }
+```
+
+**Observed** (this repo, ~215k toggle cycles per 3s run): default mode faults with `0xC0000005`
+frequently — **5 of 16** runs in a Debug build, ~1 in 8 in earlier Release runs; `--callonly` is
+**clean every run** (~60M calls). Under a debugger the faulting instruction pointer is inside the
+target function body (e.g. `target_fn+0x4`), i.e. a *caller* thread, not the toggling thread. Note the
+toggle mode completes far fewer caller calls (~1M vs ~60M) because the VEH fault storm and the
+`trap_threads` mutex throttle the callers — itself evidence of the thundering-herd fault path.
+
+## Mechanism (why it faults)
+
+In `enable()` / `disable()` the byte rewrite runs inside `trap_threads(from, to, len, run_fn)`
+(`src/os.windows.cpp`):
+
+1. registers a trap `{from, to, len}` in a global map and installs a VEH (`trap_handler`) once;
+2. `VirtualProtect`s **both the `from` and `to` pages to `PAGE_READWRITE`** — removing *execute*
+   permission from the entire page the function lives on — for the duration of the rewrite;
+3. runs `run_fn` (a plain, non-atomic `emit_jmp` on enable / `std::copy` of the original bytes on
+   disable);
+4. restores the page protections.
+
+While the page is non-executable, any thread executing **anywhere on it** faults. `trap_handler`:
+
+- if the faulting RIP is exactly within `[from, from+len)` (a thread parked on the prologue),
+  relocates it to the matching offset in `to` via `fix_ip` — **only** an exact `RIP == from+i` match;
+- otherwise, if the fault is merely elsewhere on the trapped page, returns
+  `EXCEPTION_CONTINUE_EXECUTION` to **retry** the instruction once protections are restored.
+
+This is correct for a *single, occasional* enable/disable. It breaks under tight-loop toggling:
+
+- A caller executing the function **body** (past the stolen prologue bytes) is never relocated by
+  `fix_ip` (its RIP is not `from+i`); it depends entirely on the retry path, which assumes the bytes
+  at its RIP are **stable** once executable again.
+- But under a tight loop one thread is almost continuously inside `trap_threads` flipping page
+  protection and rewriting the prologue (enable writes the jump, disable restores originals), while
+  caller threads continuously fault on the non-executable page and re-enter the VEH. The opposing
+  traps `{target→trampoline}` and `{trampoline→target}` are keyed differently and **never removed**
+  from the map (`add_trap` only `insert_or_assign`s), so they accumulate and can ping-pong a retrying
+  thread between them.
+- A caller that retries at an instruction boundary while the prologue is **half-rewritten** by a
+  concurrent toggle decodes a torn instruction → unrecoverable AV in the body.
+
+In short: the re-patch is not atomic with respect to a concurrently *retrying* caller, and the VEH
+relocation only rescues threads parked on the prologue, not threads mid-body caught by the
+whole-page non-executable window during a re-patch that another toggle is simultaneously mutating.
+
+## Suggested directions for an upstream fix
+
+- Remove a trap from the map once `trap_threads` finishes, so opposing traps can't accumulate and
+  ping-pong.
+- Quiesce in-flight callers for the duration of the rewrite (e.g. suspend other threads, or stage the
+  patch so no thread can observe a half-written prologue), rather than relying solely on retry.
+- Document explicitly that `enable()`/`disable()` are not safe to call in a tight loop concurrent with
+  calls to the hooked function, and recommend an atomic enable/disable that does not re-patch bytes
+  (e.g. a gate the detour checks) for callers that toggle frequently.
+
+## Why this does not affect CoopAllTheThings
+
+Our hooks are reconciled from a **single worker thread, tick-bounded** (`hook/src/dllmain.cpp`): a
+subsystem's `install_*`/`remove_*` (≡ `enable()`/`disable()`) runs at most once per reconcile tick,
+never in a tight loop, and never from two threads at once for the same hook. The end-to-end
+hook/unhook storm (`tests/mock_game_test.cpp`) toggles every subsystem while the game presents at
+thousands of frames/s and is reliably crash-free — that is SafetyHook's designed, safe case (a single
+enable/disable concurrent with calls; the VEH relocates in-flight callers). Only the synthetic
+tight-loop in the reproducer, which our architecture cannot produce, hits the window above.
+
+Our contract (persistent trampoline reuse, drain coordination) is covered deterministically by
+`tests/hook_install_test.cpp` and `tests/detour_gate_test.cpp`; the integrated concurrent behaviour at
+realistic cadence is covered by `tests/mock_game_test.cpp`.