fix(world): drain workers before mutating strate layout/passages (VF-01)

Initialize does StrateLayout.Empty() and Passages.Empty()/Add() -- freeing and
reallocating both -- with no guard, while mesher workers read them through
AnyPassageNearBox, EvaluateModifierSDF and FindSlotIndexForChunkZ. The epoch is
bumped AFTER, so previous-epoch workers are live during the mutation; the epoch
rejects a finished result, it cannot make a read of a freed allocation safe.
Same class as the DiffLayer.ChunkMods carve-vs-stream AV that ModsLock fixed.

Chose the drain over the two alternatives:
  - FRWLock on the arrays: correct, exact in-repo precedent, but a read lock on
    the per-voxel hot path (~43k/tile) would contaminate the perf measurement
    CODEX-TASK-001/002 are queued to take. Worst possible timing.
  - Immutable generation snapshot (Sol's proposal): right long-term, refactors
    the whole StrateManager API surface. A design conversation, not an agent's
    unprompted call.
  - Drain: zero hot-path cost, reuses the machinery EndPlay already proved, and
    its stall lands only on human-initiated editor actions, never during play.

FScopedGenerationPause raises a new bGenerationPaused (deliberately NOT
bShuttingDown, which means teardown) and drains both reader populations: chunk
tasks via ActiveTaskCount and decoration tasks via a new
WaitForDecorationTasks, whose refactor also removed NotifyShutdown's duplicate
spin loop. On timeout it does NOT mutate -- logs an Error and leaves the world
consistent. A timeout that proceeds is what the bug already does.

RebuildStrates, OnObjectModifiedInEditor and ChangeSeed cannot reach Initialize
without the pause. BeginPlay untouched (no tasks yet). EndPlay untouched --
VF-02's 3s timeout is a separate deliberate decision.

Verified: four files, no density/mesher file, so terrain cannot move.
ShouldAbortWork replaced three existing checks, adding none. No worker path
waits on the game thread, so the drain is bounded.

Not built -- Jahni builds.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
2026-08-16 17:51:19 +02:00
parent bc0bf83c64
commit 49a9959aed
6 changed files with 349 additions and 50 deletions
+150
View File
@@ -0,0 +1,150 @@
# Codex task 007 — VF-01: never mutate layout/passages while workers read them
**Owner:** Codex (Model Luna, xHigh) · **Orchestrator:** Claude · **Branch:** `experimental`
**Status:** specified, not started
**Kind:** ⚠️ **crash class (use-after-free).** Highest-severity item found on 2026-08-16.
**Origin:** Sol-High audit VF-01, **independently confirmed by reading** before this spec was written.
---
## The defect
`UVoxelStrateManager::Initialize` does `StrateLayout.Empty()` and `Passages.Empty()` + `Passages.Add()`
— it **frees and reallocates** both arrays. There is **no lock, no barrier, no drain** in that file.
Meanwhile those same arrays are read **on mesher worker threads**:
| reader | access |
|---|---|
| `AnyPassageNearBox` (`VoxelStrateManager.cpp:460`) | range-`for` over `Passages` |
| `EvaluateModifierSDF` | indexes `Passages[...]` |
| `FindSlotIndexForChunkZ` | iterates `StrateLayout` |
all reached from `GetDensityAt` / `ClassifyTile` inside chunk tasks.
`RegenerateAllChunks()` bumps the epoch **after** `Initialize`, so previous-epoch workers are live
*during* the mutation. **The epoch rejects a finished result; it cannot make a read of a freed
allocation safe.**
**Precedent in this very codebase:** `DiffLayer.ChunkMods` is read on mesher workers and written on
the game thread, and all access now holds `ModsLock` — added after a real carve-vs-stream access
violation. `StrateLayout` / `Passages` are the same shape with no guard.
Four `Initialize` call sites:
| line | function | dangerous? |
|---|---|---|
| 145 | `RebuildStrates` | **yes** |
| 309 | `OnObjectModifiedInEditor` | **yes — fires automatically on a strate asset edit while streaming** |
| 417 | `BeginPlay` | **no** — no tasks exist yet. **Leave it alone.** |
| 2091 | `ChangeSeed` | **yes** (also writes `Generator`'s `Seed` / `OriginSpineRadius`) |
## Why THIS fix and not the other two
Rejected deliberately — do not "improve" the design into either of these:
- **An `FRWLock` around the two arrays** (the `ModsLock` shape) would put a **read lock on the
per-voxel hot path** — `EvaluateModifierSDF` and `FindSlotIndexForChunkZ` run ~43k times per tile.
There is an open, unmeasured perf regression under active investigation (`CODEX-TASK-001/002`);
adding hot-path lock traffic now would **contaminate the very measurement those tasks exist to
take.** Correct, but the worst possible timing.
- **An immutable generation snapshot** (Sol's suggestion) is the right long-term architecture and a
real refactor of `UVoxelStrateManager`'s whole API surface. Too large to improvise, and it belongs
in a design conversation.
**The drain has zero hot-path cost**, reuses machinery already proven in `EndPlay`, and its only
cost — a brief stall — lands exclusively on **human-initiated editor actions** (asset edit, rebuild,
seed change). It never occurs during play.
## What to build
### 1. A pause flag distinct from shutdown
Add to `AVoxelWorld`: `std::atomic<bool> bGenerationPaused{false};`
⚠️ **Do NOT reuse `bShuttingDown` for this.** It would work mechanically, but it means "we are tearing
down" and a future reader would be misled about lifetime. Introduce a small helper used at the
existing gate points:
```cpp
FORCEINLINE bool ShouldAbortWork() const
{
return bShuttingDown.load(std::memory_order_relaxed)
|| bGenerationPaused.load(std::memory_order_relaxed);
}
```
Route the **existing** checks through it — the submission gate (`VoxelWorld.cpp:638`) and the
in-task checks (`:1467`, `:1474`). **Do not add new check points**; do not change what those sites do
when the check is true.
### 2. An RAII scoped pause, modelled on `EndPlay`'s drain
`EndPlay` (`:323334`) already implements this exact pattern: raise the gate, then spin until
`ActiveTaskCount` reaches 0. Mirror it.
```
FScopedGenerationPause guard(this);
if (!guard.Acquired()) { /* log error, DO NOT mutate, return */ }
```
- **Ctor:** set `bGenerationPaused = true`, then wait for **both** `AVoxelWorld::ActiveTaskCount == 0`
**and** the decoration tasks to finish. Decoration tasks are counted by the file-static
`GActiveDecoTasks` in `VoxelContentManager.cpp` and already drained by `NotifyShutdown` (`:6580`) —
add a small public drain/wait accessor on `UVoxelContentManager` rather than exposing the counter.
- **Dtor:** always clear `bGenerationPaused`, including on the failure path.
### 3. ⚠️ FAIL SAFE — this is the most important line in the spec
If the deadline expires with tasks still running: **DO NOT MUTATE.** Log an error naming the
function, clear the flag, and return, leaving the world in its previous consistent state. The user
can retry the edit.
**Mutating anyway is what the bug already does.** A timeout that proceeds is not a fix. The three
dangerous call sites must each be structured so the `Initialize` call is *unreachable* unless the
pause was acquired.
Use a generous deadline (≥ 5 s) and log at `Error` when it expires — a silent skip would look like
the edit simply didn't apply.
### 4. Wrap the three call sites
`RebuildStrates`, `OnObjectModifiedInEditor`, `ChangeSeed`. The pause must cover **all** the mutation,
including `ChangeSeed`'s writes to the generator's `Seed` / `OriginSpineRadius`, and it must be
released **before** `RegenerateAllChunks()` so regeneration can submit work. **`BeginPlay` is not
wrapped.**
## ⚠️ Invariants
1. **No density, mesher, or geometry code may change.** This must not move one bit of generated
terrain. If your diff touches `VoxelGenerator.cpp`, `VoxelDensityOpStack.cpp`,
`VoxelCaveMorphology.cpp` or `VoxelMarchingCubesMesher.cpp`, stop — wrong site.
2. **No deadlock.** The pause is taken on the **game thread**. Verify by reading that chunk tasks
never block on the game thread (they read the generator and `Enqueue` to an MPSC queue, which is
non-blocking) — so a drain is bounded. **State in your report that you checked this**, and if you
find any worker path that waits on the game thread, STOP and report it instead of proceeding.
3. **`ProcessQueue` stays `EQueueMode::Mpsc`.** Do not touch it.
4. **Do not change `EndPlay`.** Its 3-second timeout is a separate, deliberate decision
(audit VF-02) and is Jahni's call, not part of this task.
5. **Carry the `Epoch`** through anything you touch; do not reorder the existing epoch bump relative
to `RegenerateAllChunks`.
6. Comments are French + English; match the surrounding file.
## Acceptance
- `stat`/gameplay unchanged; **generated terrain bit-identical** (the equivalence tests and every
box-verdict number must be untouched — this change cannot reach them).
- Editing a strate asset while the world streams: brief stall, then the edit applies. **No crash.**
- The failure path is reachable and honest: if the drain times out, an `Error` log names the function
and the world keeps its previous state.
- `git diff --stat` should list `VoxelWorld.cpp`, `VoxelWorld.h`, and `VoxelContentManager.{h,cpp}`
for the drain accessor. Nothing else.
## Notes for the reviewer (Claude)
- Confirm the three dangerous sites cannot reach `Initialize` when the pause was not acquired, and
that `BeginPlay` is untouched.
- Confirm the dtor clears the flag on **every** path including early return.
- Confirm `ShouldAbortWork` replaced the existing checks rather than adding new ones, and that
`bShuttingDown`'s own semantics are unchanged.
- Confirm the deco drain is included — chunk tasks alone are not the whole reader set.