diff --git a/CODEX-TASK-007-generation-pause.md b/CODEX-TASK-007-generation-pause.md new file mode 100644 index 0000000..91b419e --- /dev/null +++ b/CODEX-TASK-007-generation-pause.md @@ -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 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` (`:323–334`) 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` (`:65–80`) — + 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. diff --git a/OPSTACK-PROGRESS.md b/OPSTACK-PROGRESS.md index 7a3127f..203451c 100644 --- a/OPSTACK-PROGRESS.md +++ b/OPSTACK-PROGRESS.md @@ -3899,3 +3899,63 @@ VF-01's fix is also genuinely Jahni's call, not an agent's: Trading a possible crash for a guaranteed hitch, or for hot-path lock traffic, is a product decision. It waits for him. + +## 2026-08-16 (k) — VF-01 FIXED: the drain, chosen over the lock and the snapshot + +Jahni: *"well, you select it, I trust you and Sol."* `CODEX-TASK-007`. + +**First, a position I updated rather than defended.** An hour earlier I argued for adding no more +code before the build, to keep the three "nothing should move" fixes attributable. That argument was +about the **geometry** numbers — and a streaming-lifecycle change cannot reach an equivalence test or +a box verdict, which are computed from params and hashes, not from *when* `Initialize` runs. The risk +I cited did not apply to this particular change. A lifecycle bug shows up as a hang or a hitch, which +is trivially attributable because nothing else in the stack touches lifecycle. + +### Why the drain, and not the other two + +| option | verdict | +|---|---| +| **`FRWLock` on the two arrays** (the `ModsLock` shape — smallest diff, exact in-repo precedent) | ⛔ **rejected on timing.** It puts a read lock on the **per-voxel hot path**: `EvaluateModifierSDF` and `FindSlotIndexForChunkZ` run ~43k times per tile. There is an open, unmeasured perf regression and two tasks queued to measure it — adding hot-path lock traffic now would **contaminate the exact measurement they exist to take.** Correct, worst possible timing. | +| **Immutable generation snapshot** (Sol's proposal) | ⛔ **right long-term, too large to improvise.** It refactors `UVoxelStrateManager`'s whole API surface. That is a design conversation, not an agent's unprompted call. | +| **Drain before mutating** | ✅ **chosen.** Zero hot-path cost. Reuses machinery already proven in `EndPlay`. Its only cost — a brief stall — lands exclusively on **human-initiated editor actions** (asset edit, rebuild, seed change) and never during play. | + +### What landed + +`FScopedGenerationPause` (RAII, in `VoxelWorld.cpp`) raises a new `bGenerationPaused` — **deliberately +not `bShuttingDown`**, which means "tearing down" and would have misled the next reader — then drains +**both** reader populations: `ActiveTaskCount` for chunk tasks and, via a new +`UVoxelContentManager::WaitForDecorationTasks`, the decoration tasks counted by `GActiveDecoTasks`. +Chunk tasks alone were never the whole reader set. + +⚠️ **The line that makes it a fix rather than a narrowing: on timeout it does NOT mutate.** It logs an +`Error` naming the function and returns, leaving the world in its previous consistent state. A +timeout that proceeds anyway is exactly what the bug already does. The three dangerous sites — +`RebuildStrates`, `OnObjectModifiedInEditor`, `ChangeSeed` — are structured so `Initialize` is +*unreachable* without the pause. `BeginPlay` (the fourth site) is untouched: no tasks exist yet. + +### Verified after the fact, not taken on report + +- Four files only; **no density or mesher file in the diff**, so generated terrain cannot move. +- `ShouldAbortWork()` **replaced** three existing `bShuttingDown` checks — 3 replacements, 0 new + check points. `bShuttingDown`'s own semantics are unchanged. +- `EndPlay` and `BeginPlay` untouched (VF-02's 3-second timeout is a separate deliberate decision and + stays Jahni's call). +- The deadline passed to `WaitForDecorationTasks` is **absolute**, documented as such in the header — + and the refactor **removed** the duplicate spin loop from `NotifyShutdown` rather than adding a + second one. +- Deadlock check, which the spec required Codex to perform and report: chunk and decoration tasks + read the generator and `Enqueue` to MPSC queues; **no worker path waits on the game thread**, so a + game-thread drain is bounded. Raising the pause also makes in-flight tasks bail early, so the drain + gets *faster*, not slower. + +### Compile-risk spots for the build + +1. `WaitForDecorationTasks(double)` declaration / definition / call-site agreement. +2. `FScopedGenerationPause`'s forward declaration and `friend class` access in `VoxelWorld.h`. +3. `ShouldAbortWork()` visibility from inside the `UE::Tasks::Launch` lambda. + +### What to check in the editor + +Edit a strate asset while the world is streaming. Expect a **brief stall, then the edit applies, and +no crash.** If the `Error` log about a timed-out pause ever appears, the drain deadline (5 s) is too +short for the in-flight queue and that is worth knowing rather than guessing. diff --git a/Source/VoxelForge/Private/VoxelContentManager.cpp b/Source/VoxelForge/Private/VoxelContentManager.cpp index feeef52..4070b59 100644 --- a/Source/VoxelForge/Private/VoxelContentManager.cpp +++ b/Source/VoxelForge/Private/VoxelContentManager.cpp @@ -68,17 +68,23 @@ void UVoxelContentManager::NotifyShutdown() // Wait for in-flight march tasks to finish (they check the flag and bail). Timeout to avoid hangs. const double Deadline = FPlatformTime::Seconds() + 3.0; - while (GActiveDecoTasks.load(std::memory_order_relaxed) > 0) - { - if (FPlatformTime::Seconds() > Deadline) break; - FPlatformProcess::Yield(); - } + WaitForDecorationTasks(Deadline); DrainDecoResults(); ResetGridBuildState(NearGrid); ResetGridBuildState(FarGrid); } +bool UVoxelContentManager::WaitForDecorationTasks(double Deadline) +{ + while (GActiveDecoTasks.load(std::memory_order_relaxed) > 0) + { + if (FPlatformTime::Seconds() > Deadline) return false; + FPlatformProcess::Yield(); + } + return true; +} + void UVoxelContentManager::DrainDecoResults() { FDecoCellResult Discard; diff --git a/Source/VoxelForge/Private/VoxelWorld.cpp b/Source/VoxelForge/Private/VoxelWorld.cpp index 5259c21..6bf34a9 100644 --- a/Source/VoxelForge/Private/VoxelWorld.cpp +++ b/Source/VoxelForge/Private/VoxelWorld.cpp @@ -88,6 +88,45 @@ static void BuildTileStreamSet(RealtimeMesh::FRealtimeMeshStreamSet& Streams, co } } +class FScopedGenerationPause +{ +public: + explicit FScopedGenerationPause(AVoxelWorld* InWorld) + : World(InWorld) + { + if (!World) return; + + World->bGenerationPaused.store(true, std::memory_order_release); + + // The game thread owns this gate; workers only read Generator/Mesher and enqueue results. + // La barrière est prise sur le thread de jeu ; les workers ne font qu'énumérer et Enqueue. + const double Deadline = FPlatformTime::Seconds() + 5.0; + while (World->ActiveTaskCount.load(std::memory_order_relaxed) > 0) + { + if (FPlatformTime::Seconds() > Deadline) return; + FPlatformProcess::Yield(); + } + + if (World->ContentManager && !World->ContentManager->WaitForDecorationTasks(Deadline)) return; + + bAcquired = true; + } + + ~FScopedGenerationPause() + { + if (World) + { + World->bGenerationPaused.store(false, std::memory_order_release); + } + } + + bool Acquired() const { return bAcquired; } + +private: + AVoxelWorld* World = nullptr; + bool bAcquired = false; +}; + //============================================================================= // LIVE EDIT — regenerate all chunks when params change in the Details panel //============================================================================= @@ -139,13 +178,22 @@ void AVoxelWorld::RegenerateAllChunks() void AVoxelWorld::RebuildStrates() { - if (StrateManager && Settings) { - // Re-applies layout + inter-strate gap + passage/spine settings from VoxelSettings. - StrateManager->Initialize(Settings, Settings->Seed); + FScopedGenerationPause Guard(this); + if (!Guard.Acquired()) + { + UE_LOG(LogTemp, Error, TEXT("[VoxelWorld] RebuildStrates: generation pause timed out; no mutation applied.")); + return; + } + + if (StrateManager && Settings) + { + // Re-applies layout + inter-strate gap + passage/spine settings from VoxelSettings. + StrateManager->Initialize(Settings, Settings->Seed); + } + if (AtmosphereManager) AtmosphereManager->Reset(); + if (ContentManager) ContentManager->ClearAll(); } - if (AtmosphereManager) AtmosphereManager->Reset(); - if (ContentManager) ContentManager->ClearAll(); // Reload all chunks against the rebuilt strate data. RegenerateAllChunks(); @@ -304,13 +352,22 @@ void AVoxelWorld::OnObjectModifiedInEditor(UObject* ModifiedObject) // Re-initialize the strate manager so it picks up the changed definition values, // then regenerate all chunks with the updated params. - if (StrateManager) { - StrateManager->Initialize(Settings, Settings->Seed); - } - if (Generator) - { - Generator->InitializeSettings(Settings); + FScopedGenerationPause Guard(this); + if (!Guard.Acquired()) + { + UE_LOG(LogTemp, Error, TEXT("[VoxelWorld] OnObjectModifiedInEditor: generation pause timed out; no mutation applied.")); + return; + } + + if (StrateManager) + { + StrateManager->Initialize(Settings, Settings->Seed); + } + if (Generator) + { + Generator->InitializeSettings(Settings); + } } RegenerateAllChunks(); @@ -635,7 +692,7 @@ bool AVoxelWorld::ApplyTileResult(FChunkResult& Result) // is refilled from the diff via MarkDirtyVoxelBox in RemeshDirtyChunks). void AVoxelWorld::SyncRemeshTile(const FVoxelTileKey& Tile) { - if (!Generator || !Mesher || bShuttingDown.load(std::memory_order_relaxed)) return; + if (!Generator || !Mesher || ShouldAbortWork()) return; const FIntVector OriginVoxels = Tile.OriginVoxels(); const int32 Cells = CHUNK_SIZE; // level 0 is always full-res (level 0 < FullResClipLevels) @@ -1464,14 +1521,14 @@ void AVoxelWorld::LoadTile(const FVoxelTileKey& Tile, bool bHighPriority) ~FTaskGuard() { Counter.fetch_sub(1, std::memory_order_relaxed); } } Guard{ActiveTaskCount}; - if (bShuttingDown.load(std::memory_order_relaxed)) return; + if (ShouldAbortWork()) return; FChunkResult Result; GenerateTileResult(Tile, OriginVoxels, Step, Cells, TaskEpoch, bWantCapture, BandVoxLo, BandVoxHi, BandChunkLo, BandChunkHi, bSheetTile, SheetChunkZ, HoleMinX, HoleMinY, HoleMaxX, HoleMaxY, Result); - if (!bShuttingDown.load(std::memory_order_relaxed)) + if (!ShouldAbortWork()) { ProcessQueue.Enqueue(MoveTemp(Result)); // move: don't copy the geometry payload } @@ -2072,42 +2129,51 @@ void AVoxelWorld::ChangeSeed(int32 NewSeed) const int32 OldSeed = Settings->Seed; const int32 OldSeason = Settings->CurrentSeason; - // 1. Update seed in Settings (the authoritative source) - Settings->Seed = NewSeed; - - // 2. Increment season counter - Settings->CurrentSeason++; - - // 3. Push new seed to Generator - if (Generator) { - Generator->InitializeSettings(Settings); - } + FScopedGenerationPause Guard(this); + if (!Guard.Acquired()) + { + UE_LOG(LogTemp, Error, TEXT("[VoxelWorld] ChangeSeed: generation pause timed out; no mutation applied.")); + return; + } - // 4. Rebuild strate layout with the new seed. - // Strate assignments and passages all change. - if (StrateManager) - { - StrateManager->Initialize(Settings, NewSeed); - } + // 1. Update seed in Settings (the authoritative source) + Settings->Seed = NewSeed; - // 5. Clear all player modifications — carvings from the old world are meaningless - if (DiffLayer) - { - DiffLayer->Clear(); - } + // 2. Increment season counter + Settings->CurrentSeason++; - // 5b. Update content placement seed so the new world scatters differently. - if (ContentManager) - { - ContentManager->SetSeed(NewSeed); - ContentManager->ClearAll(); - } + // 3. Push new seed to Generator + if (Generator) + { + Generator->InitializeSettings(Settings); + } - // 5c. Reset atmosphere — strate layout changed, re-apply on next Tick. - if (AtmosphereManager) - { - AtmosphereManager->Reset(); + // 4. Rebuild strate layout with the new seed. + // Strate assignments and passages all change. + if (StrateManager) + { + StrateManager->Initialize(Settings, NewSeed); + } + + // 5. Clear all player modifications — carvings from the old world are meaningless + if (DiffLayer) + { + DiffLayer->Clear(); + } + + // 5b. Update content placement seed so the new world scatters differently. + if (ContentManager) + { + ContentManager->SetSeed(NewSeed); + ContentManager->ClearAll(); + } + + // 5c. Reset atmosphere — strate layout changed, re-apply on next Tick. + if (AtmosphereManager) + { + AtmosphereManager->Reset(); + } } // 6. Unload all existing chunks and let Tick reload them with new generation diff --git a/Source/VoxelForge/Public/VoxelContentManager.h b/Source/VoxelForge/Public/VoxelContentManager.h index a5498fb..613c8c1 100644 --- a/Source/VoxelForge/Public/VoxelContentManager.h +++ b/Source/VoxelForge/Public/VoxelContentManager.h @@ -138,6 +138,10 @@ public: * UObject teardown (worker tasks read the Generator). */ void NotifyShutdown(); + /** Wait until in-flight decoration march tasks drain before a generation mutation. Deadline is absolute. + * Attend la fin des tâches de décoration avant une mutation de génération ; échéance absolue. */ + bool WaitForDecorationTasks(double Deadline); + //--- async-task plumbing (public so the worker lambda can reach them) ----- /** One placement decided off-thread; spawned on the game thread from FDecoCellResult::Entries. */ struct FDecoSpawn diff --git a/Source/VoxelForge/Public/VoxelWorld.h b/Source/VoxelForge/Public/VoxelWorld.h index 896da85..ea0147b 100644 --- a/Source/VoxelForge/Public/VoxelWorld.h +++ b/Source/VoxelForge/Public/VoxelWorld.h @@ -25,6 +25,7 @@ class UMaterialParameterCollection; class UVolumeTexture; class UMaterialInterface; class UMaterialInstanceDynamic; +class FScopedGenerationPause; namespace RealtimeMesh { struct FRealtimeMeshStreamSet; } // T1.f — worker-built geometry buffers /** @@ -373,6 +374,8 @@ public: UVolumeTexture* GetDensityVolumeTexture(int32 Level = 0) const; private: + friend class FScopedGenerationPause; + /** Get/create the shared MID wrapping a base terrain material (binds volume textures + shadow params). * Returns Base unchanged-wrapped, or nullptr if Base is null. */ UMaterialInstanceDynamic* GetOrCreateTerrainMID(UMaterialInterface* Base); @@ -677,9 +680,19 @@ public: // Set to true during EndPlay — async tasks check this before accessing UObjects std::atomic bShuttingDown{false}; + // Set during editor-driven generation mutations; distinct from teardown/shutdown semantics. + // Active pendant les mutations de génération lancées par l'éditeur, sans signifier la destruction. + std::atomic bGenerationPaused{false}; + // Number of async tasks currently running — EndPlay waits for this to reach 0 std::atomic ActiveTaskCount{0}; + FORCEINLINE bool ShouldAbortWork() const + { + return bShuttingDown.load(std::memory_order_relaxed) + || bGenerationPaused.load(std::memory_order_relaxed); + } + // Player's level-0 tile coord (= chunk coord). The desired set is rebuilt when this changes. FIntVector CurrentCenterChunk = FIntVector::ZeroValue;