docs: VF-01 confirmed (worker race on rebuild) + VF-10 triaged; hold further code
VF-01 CONFIRMED and it is the most serious find of the day. Initialize does StrateLayout.Empty() and Passages.Empty()/Add() -- freeing and reallocating both -- with no lock, barrier or drain anywhere in that file, while worker threads read the same arrays through AnyPassageNearBox (:460), EvaluateModifierSDF and FindSlotIndexForChunkZ, reached from GetDensityAt/ClassifyTile. RegenerateAllChunks bumps the epoch AFTER the mutation, so previous-epoch workers are live during it; the epoch rejects a finished result, it cannot make a read of a freed allocation safe. Same class already fixed once here: DiffLayer.ChunkMods got ModsLock after a carve-vs-stream AV. The dangerous call site is OnObjectModifiedInEditor, which fires automatically on a strate asset edit while streaming -- routine here. VF-10 confirmed real (74 fields, per near-surface sample) but Sol missed the conclusion: the copy is INHERITED from GetDensityWithParams, so both paths pay it and it does NOT explain the op-stack regression. The op stack improved it by memoising across eleven detail ops. Future optimisation, not the answer. Deliberately NOT fixing VF-01 now. Four code changes are stacked unbuilt and three have "nothing should move" as their acceptance signal; a fifth change to streaming lifecycle would make an odd build result unattributable. The three fix options (drain / snapshot / RWLock) trade a possible crash for an editor hitch or hot-path lock traffic -- a product decision, not an agent's. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
@@ -11,7 +11,9 @@
|
|||||||
> | **VF-05** (radius envelope in `BuildChunkCache`) | ✅ **CONFIRMED and FIXED** — `CODEX-TASK-006`. Real, and the worst of three instances of this class: it is `TunnelNetwork`, it is in code **both** density paths share, and it breaks window invariance (`ARCHITECTURE §8.4`). Genuinely good find. |
|
> | **VF-05** (radius envelope in `BuildChunkCache`) | ✅ **CONFIRMED and FIXED** — `CODEX-TASK-006`. Real, and the worst of three instances of this class: it is `TunnelNetwork`, it is in code **both** density paths share, and it breaks window invariance (`ARCHITECTURE §8.4`). Genuinely good find. |
|
||||||
> | **VF-02** (3-second shutdown timeout) | ✅ **premise confirmed** — `VoxelWorld.cpp:327` literally reads *"Timeout after 3 seconds to avoid hanging the editor."* Note `CLAUDE.md` states the invariant more strongly than the code implements it ("EndPlay blocks on `ActiveTaskCount → 0`" — it blocks *with a deadline*). Worth deciding deliberately. |
|
> | **VF-02** (3-second shutdown timeout) | ✅ **premise confirmed** — `VoxelWorld.cpp:327` literally reads *"Timeout after 3 seconds to avoid hanging the editor."* Note `CLAUDE.md` states the invariant more strongly than the code implements it ("EndPlay blocks on `ActiveTaskCount → 0`" — it blocks *with a deadline*). Worth deciding deliberately. |
|
||||||
> | **VF-03** (TLS caches omit the owning world) | ⚠️ **substance plausible, EVIDENCE OVERSTATED.** It claims *"the test fixture explicitly documents observed cross-world contamination."* It does not. `VoxelForgeTestFixture.h` documents that the `thread_local` caches exist and flags an unrelated `TSoftObjectPtr` risk. The underlying point (caches keyed on chunk/seed/layout but not on which generator owns them) may still hold — but it needs checking on its own merits, not on this citation. |
|
> | **VF-03** (TLS caches omit the owning world) | ⚠️ **substance plausible, EVIDENCE OVERSTATED.** It claims *"the test fixture explicitly documents observed cross-world contamination."* It does not. `VoxelForgeTestFixture.h` documents that the `thread_local` caches exist and flags an unrelated `TSoftObjectPtr` risk. The underlying point (caches keyed on chunk/seed/layout but not on which generator owns them) may still hold — but it needs checking on its own merits, not on this citation. |
|
||||||
> | VF-01, VF-04, VF-06 … VF-10 | **NOT independently verified.** Read them as leads. |
|
> | **VF-01** (live rebuild races streaming workers) | ✅ **CONFIRMED — the most serious finding here.** `UVoxelStrateManager::Initialize` does `StrateLayout.Empty()` (:~36) **and** `Passages.Empty()` (:171) then `Passages.Add()`, i.e. it frees and reallocates both arrays. There is **no lock, no barrier, no drain** anywhere in that file. Worker-side readers of the same arrays: `AnyPassageNearBox` (:460, range-for over `Passages`), `EvaluateModifierSDF` (indexes `Passages[...]`), `FindSlotIndexForChunkZ` (iterates `StrateLayout`) — all reached from `GetDensityAt`/`ClassifyTile` on mesher workers. And `RegenerateAllChunks()` (which bumps the epoch) runs **after** `Initialize`, so previous-epoch workers are still live during the mutation. **This is the same class already fixed once in this codebase** — `DiffLayer.ChunkMods` got `ModsLock` after a carve-vs-stream access violation. Four call sites, incl. `OnObjectModifiedInEditor` (:309), which fires automatically when a strate asset is edited while the world streams. **NOT fixed — see the note below.** |
|
||||||
|
> | **VF-10** (~74-field per-voxel params copy) | ✅ **confirmed real, but Sol missed the conclusion that matters.** The 74 fields are real and the copy is per near-surface sample. **However it is INHERITED from the original path — `GetDensityWithParams` does the same copy — so both paths pay it equally and it does NOT explain the op-stack perf regression.** The op stack actually *improved* it (memoised so eleven detail ops don't each repeat it), and the site says so in its own comment. Genuine future optimisation for both paths; **not** the answer to "why is the op path slower". |
|
||||||
|
> | VF-04, VF-06, VF-07, VF-08, VF-09 | **NOT independently verified.** Read them as leads. |
|
||||||
>
|
>
|
||||||
> **Do not treat an unverified row as actionable.** The lesson this project keeps paying for is that a
|
> **Do not treat an unverified row as actionable.** The lesson this project keeps paying for is that a
|
||||||
> confident chain resting on an unchecked premise reverses about half the time — and VF-03 is an
|
> confident chain resting on an unchecked premise reverses about half the time — and VF-03 is an
|
||||||
|
|||||||
@@ -3835,3 +3835,67 @@ proceeds when it expires while tasks still hold a raw `this`. **`CLAUDE.md` stat
|
|||||||
strongly than the code implements it** ("EndPlay blocks on `ActiveTaskCount → 0`" — it blocks *with a
|
strongly than the code implements it** ("EndPlay blocks on `ActiveTaskCount → 0`" — it blocks *with a
|
||||||
deadline*). That is a design trade someone made on purpose (never hang the editor) and it should be
|
deadline*). That is a design trade someone made on purpose (never hang the editor) and it should be
|
||||||
re-affirmed or changed deliberately, not patched by an agent while he is away.
|
re-affirmed or changed deliberately, not patched by an agent while he is away.
|
||||||
|
|
||||||
|
## 2026-08-16 (j) — VF-01 CONFIRMED (crash class). And the deliberate decision to STOP writing code.
|
||||||
|
|
||||||
|
Jahni: *"with those new finds, anything you feel you should act on right now?"* Two verifications,
|
||||||
|
and then a judgement call that the answer is **no more code today**.
|
||||||
|
|
||||||
|
### ⛔ VF-01 is real, and it is the most serious thing found today
|
||||||
|
|
||||||
|
`UVoxelStrateManager::Initialize` does `StrateLayout.Empty()` **and** `Passages.Empty()` followed by
|
||||||
|
`Passages.Add()` — it frees and reallocates both arrays. **There is no lock, no barrier and no worker
|
||||||
|
drain anywhere in that file.** Meanwhile these are read *on mesher workers*:
|
||||||
|
|
||||||
|
- `AnyPassageNearBox` (:460) — range-for over `Passages`
|
||||||
|
- `EvaluateModifierSDF` — indexes `Passages[...]`
|
||||||
|
- `FindSlotIndexForChunkZ` — iterates `StrateLayout`
|
||||||
|
|
||||||
|
all reached from `GetDensityAt` / `ClassifyTile`. And `RegenerateAllChunks()` — which bumps the epoch
|
||||||
|
— runs **after** `Initialize`, so previous-epoch workers are still live *during* the mutation. The
|
||||||
|
epoch rejects a finished result; it cannot make a concurrent read of a freed allocation safe.
|
||||||
|
|
||||||
|
**This exact class has already bitten this project once and been fixed once:** `DiffLayer.ChunkMods`
|
||||||
|
is read on mesher workers and written on the game thread, and all access now holds `ModsLock` after a
|
||||||
|
carve-vs-stream access violation. `StrateLayout` / `Passages` are the same shape with no such guard.
|
||||||
|
|
||||||
|
Four `Initialize` call sites, and the dangerous one is **`OnObjectModifiedInEditor` (:309)** — it
|
||||||
|
fires *automatically* when a strate asset is edited while the world is streaming. That is a routine
|
||||||
|
action here, not an exotic one.
|
||||||
|
|
||||||
|
⚠️ **If Jahni has seen unexplained editor crashes while tweaking strate assets, this is a prime
|
||||||
|
suspect.** Worth asking before assuming it has never fired.
|
||||||
|
|
||||||
|
### VF-10 confirmed real — but Sol missed the conclusion that matters
|
||||||
|
|
||||||
|
The ~74-field `FStrateGenerationParams` copy per near-surface sample is real (74 fields counted).
|
||||||
|
**But it is inherited from the original path** — `GetDensityWithParams` does the same copy — so both
|
||||||
|
paths pay it equally and **it does not explain the op-stack perf regression.** The op stack actually
|
||||||
|
*improved* it, memoising so eleven detail ops don't each repeat it, and the site's own comment
|
||||||
|
already records it as a known perf item. A genuine future optimisation for both paths; not an answer
|
||||||
|
to "why is the op path slower". Sol reported the cost correctly and drew no comparison.
|
||||||
|
|
||||||
|
### ⛔ THE DECISION: stop adding code. This is the action, not an absence of one.
|
||||||
|
|
||||||
|
Four code changes are stacked unbuilt, and **three of them have "nothing should move" as their
|
||||||
|
acceptance signal**: `eaa44bf` and `cab8e8f` must be exact no-ops at correctly ordered params, and
|
||||||
|
`7dbdf51` must move exactly two numbers in one known direction. That is a delicate attribution setup
|
||||||
|
and it is only readable while it stays clean.
|
||||||
|
|
||||||
|
Adding a fifth change — to **streaming lifecycle**, the most timing-sensitive code in the plugin —
|
||||||
|
would make an unexpected build result unattributable. **The value of a clean, readable build now
|
||||||
|
exceeds the value of one more fix.**
|
||||||
|
|
||||||
|
VF-01's fix is also genuinely Jahni's call, not an agent's:
|
||||||
|
|
||||||
|
- **Minimal option** — stop submissions, drain to `ActiveTaskCount == 0`, then `Initialize`, then bump
|
||||||
|
the epoch, then resume. Reuses the drain `EndPlay` already has. Cost: an editor hitch on every
|
||||||
|
asset edit, proportional to the in-flight tile queue.
|
||||||
|
- **Sol's option** — an immutable generation snapshot (seed + layout + passages + resolved defs), one
|
||||||
|
atomic publish, each task holding a strong ref. Correct and hitch-free; a real refactor.
|
||||||
|
- **Narrow option** — an `FRWLock` over just `StrateLayout`/`Passages`, exactly the `ModsLock`
|
||||||
|
precedent. Smallest diff, adds a per-voxel read lock on the hot path — which is precisely the kind
|
||||||
|
of cost this project insists on measuring before accepting.
|
||||||
|
|
||||||
|
Trading a possible crash for a guaranteed hitch, or for hot-path lock traffic, is a product decision.
|
||||||
|
It waits for him.
|
||||||
|
|||||||
Reference in New Issue
Block a user