fix(test): cover the atlas-tier seam the D-1 fix depends on; correct AP-150's citation
Both items come from the D-1 fix review (both lenses PASS, D-1 genuinely closed). No production behaviour changes. L1 — THE SEAM HAD NO COVERAGE. The D-1 fix's "empty by construction" claim rests on LandblockSpawnAdapter's atlas-tier filter (`if (entity.ServerGuid != 0) continue;`) skipping the live server projections DetachNearLayer deliberately RETAINS across a demote. The reviewer removed that filter and all 4,170 App tests passed — only two Core unit tests caught it, none through a demote. So the invariant the re-assert depends on could have been deleted silently, re-opening D-1 by another route: a non-empty re-assert whose mesh reference is never satisfied leaves IsRenderReady false, which is the portal hang again. NearToFarDemote_WithALiveServerEntity_StaysRenderReady now demotes a landblock that CARRIES a live server-spawned entity through the real GpuWorldState + LandblockSpawnAdapter + LandblockPresentationPipeline, and asserts the retained entity never enters the desired set. Sabotage-verified: with the filter removed, exactly one test fails — this one — and the other 25 pass, including all four D-1 regression tests. That is the finding restated as a measurement: the D-1 tests genuinely do not cover this seam, and now something does. AP-150 citation corrected: the row cited 0x004D7064 as the ECM_UI::SendNotice_DisplayStringInfo call site. That address is the PStringBase construction of the "In Portal Space - Please Wait..." literal (:219516); the actual call is 0x004D70A1 (-> 0x006925B0). Same class of slip the #280 commit had just corrected for #326 — worth noting that a row filed WITH a byte-level disassembly still mis-cited a neighbouring address. Also refactored the existing pipeline demote test to keep its doc comment attached to its own method (an earlier insertion had orphaned its [Fact]). App.Tests 4,170 -> 4,171 passed / 3 skipped, net +1 for the new test. No new skips; none of #302/#308/#321 surfaced. src/ is byte-unchanged (the sabotage was reverted and verified). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
parent
43cfdc4a40
commit
bcb66ccdf3
3 changed files with 552 additions and 1 deletions
474
docs/research/2026-08-06-280-d1-fix-review.md
Normal file
474
docs/research/2026-08-06-280-d1-fix-review.md
Normal file
|
|
@ -0,0 +1,474 @@
|
|||
# #280 D-1 fix review — commit `73cdb95c`
|
||||
|
||||
**Date:** 2026-08-06
|
||||
**Subject:** `fix(streaming): make a demoted landblock render-ready like a
|
||||
published one (#280 D-1)`, commit `73cdb95c`, branch
|
||||
`claude/acdream-physics-divergence-5aa784`.
|
||||
**Mode:** adversarial, read-only, both lenses (retail conformance +
|
||||
architecture). No subagents; everything below was executed inline.
|
||||
|
||||
## Verdicts
|
||||
|
||||
| Lens | Verdict |
|
||||
|---|---|
|
||||
| **Retail conformance** | **PASS** (one LOW documentation defect) |
|
||||
| **Architecture** | **PASS** (three LOW latent risks, three style notes) |
|
||||
|
||||
**Is D-1 genuinely closed?** Yes, for the transition it names, and the fix is
|
||||
at the state owner rather than at the predicate — the non-workaround shape.
|
||||
Verified end to end: after a Near→Far demote the landblock ends with exactly
|
||||
the spawn-adapter registration a `PublicationKind.Far` activation installs, on
|
||||
both retirement call sites, and the reveal gate reopens.
|
||||
|
||||
---
|
||||
|
||||
## 1. What was reproduced independently
|
||||
|
||||
Everything below was run in this worktree at `73cdb95c`, from a **genuinely
|
||||
clean rebuild** — all 42 `obj`/`bin` directories under `src/`, `tests/`,
|
||||
`tools/` deleted first, so no stale test DLL could carry a deleted symbol.
|
||||
|
||||
| Claim | Result |
|
||||
|---|---|
|
||||
| Release build, 0 errors | ✅ reproduced (21 warnings total: 18 xUnit analyzer + 3 CS8767 nullability, all pre-existing) |
|
||||
| Full suite `-m:1`, `ACDREAM_PAK_PATH` set | ✅ **11,192 passed / 4 skipped / 0 failed** |
|
||||
| App.Tests 4157 → 4170 | ✅ App.Tests = 4,170 / 3 skips |
|
||||
| +13 net reconciles to this commit | ✅ 3 readiness Facts + 1 integration Fact + 2 warmup Facts + 7 Theory rows = 13 |
|
||||
| No new skips | ✅ 4 skips, unchanged from the `fafc0b65` baseline figure |
|
||||
| Sabotage: no-op re-assert → the four new tests fail, others pass | ✅ **4 failed / 324 passed** in the Streaming filter; the four failures are exactly the four named tests |
|
||||
| The `?? true` tautology is really dead | ✅ verified independently (below) |
|
||||
| Blast radius: four types App-internal | ✅ Headless/Runtime/Core/Core.Net/UI.Abstractions reference them only in comments; Headless tests 89/89 green |
|
||||
| Retail binary pairs with our PDB | ✅ `check_exe_pdb.py` → `MATCH` (GUID `9e847e2f-…`, linker 2013-09-06) |
|
||||
|
||||
Per-assembly breakdown of the 11,192: App 4170/3, Bake 15, Cli 4, Content 124,
|
||||
Core.Net 764, Core 4263/1, Headless 89, Runtime 1217, UI.Abstractions 546.
|
||||
|
||||
**Tree restored.** Both sabotages (and a third, described in L1) were reverted;
|
||||
`git status --short` is empty and the solution rebuilds clean.
|
||||
|
||||
### 1.1 The sabotage, reproduced
|
||||
|
||||
Inserted `if (landblockId != 0u) return;` immediately after the
|
||||
`OnLandblockUnloaded` call in
|
||||
`src/AcDream.App/Streaming/GpuWorldState.cs:1602`, rebuilt, ran the Streaming
|
||||
filter:
|
||||
|
||||
```
|
||||
NearToFarDemote_LeavesTheLandblockRenderReadyThroughTheRealPipeline [FAIL]
|
||||
NearToFarDemote_LeavesTheLandblockRenderReadyUnderBudgetedRetirement [FAIL]
|
||||
TieredWindow_StaysResidentAfterAnOuterRingDemote [FAIL]
|
||||
OutdoorReveal_SurvivesAnOuterRingDemoteDuringTheHold [FAIL]
|
||||
Failed: 4, Passed: 324, Total: 328
|
||||
```
|
||||
|
||||
The commit's claim was "all four fail, 315 others pass"; the true sibling count
|
||||
under a `~Streaming` filter is 324. The substance — exactly these four, nothing
|
||||
else — holds.
|
||||
|
||||
### 1.2 The tautology-kill, verified rather than accepted
|
||||
|
||||
The commit claims the pre-existing `WorldRevealDerivedWindowIntegrationTests`
|
||||
fixtures were stubbed out by `IsRenderReady`'s
|
||||
`(_wbSpawnAdapter?.IsLandblockRenderReady(id) ?? true)`
|
||||
(`GpuWorldState.cs:180-181`). I tested that directly: with the **production fix
|
||||
still sabotaged**, I reverted only the new D-1 integration test's world
|
||||
construction back to `new GpuWorldState()` (no adapter).
|
||||
`OutdoorReveal_SurvivesAnOuterRingDemoteDuringTheHold` **passed**.
|
||||
|
||||
That is conclusive: the null adapter degenerated `IsRenderReady` to `IsLoaded`,
|
||||
the fixture change is load-bearing, and the new test would have been another
|
||||
green-covering-nothing test without it.
|
||||
|
||||
---
|
||||
|
||||
## 2. Architecture lens — the four attack surfaces
|
||||
|
||||
### 2.1 Can the re-assert fire when it must not? — No.
|
||||
|
||||
The guard is
|
||||
`_loaded.TryGetValue(canonical, …)` **and**
|
||||
`_tierByLandblock[canonical] == Far` (`GpuWorldState.cs:1604-1611`).
|
||||
Checked by symbol against every teardown path:
|
||||
|
||||
- **Full retirement.** `DetachLandblock` removes both maps —
|
||||
`_tierByLandblock.Remove(canonical)` at `GpuWorldState.cs:1298` and
|
||||
`RemoveLoadedLandblock` at `:1299` (→ `_loaded.Remove` at `:435`). It runs at
|
||||
`LandblockRetirementCoordinator.cs:680-682`, **before** the ticket is created
|
||||
at `:702`, so by the time `AdvanceTicket`/`AdvanceTicketOne` reach the
|
||||
`MeshReferences` stage both conditions are already false.
|
||||
- **Origin recenter.** `DetachAllForOriginRecenter` does `_loaded.Clear()`
|
||||
(`:1481`) and `_tierByLandblock.Clear()` (`:1485`) synchronously inside the
|
||||
call; `AdoptDetachedFull` only creates ledgers afterwards
|
||||
(`LandblockRetirementCoordinator.cs:393-474`).
|
||||
- **Landblock replacement at the same canonical id while a receipt is still
|
||||
draining.** Fenced. `StreamingController.IsPublicationBlockedByRetirement`
|
||||
(`:2094`) is installed as the completion queue's peek predicate (`:1707`), so
|
||||
no `Loaded`/`Promoted` completion is applied while any retirement is pending
|
||||
for that id — the one funnel every completion goes through. This is what
|
||||
makes the resurrection scenario unreachable, and it is also why the
|
||||
`retained` record the re-assert reads is guaranteed to be the post-demote
|
||||
record and not a newer generation.
|
||||
- **Near-layer retirement of a landblock that is not loaded** (pending-near or
|
||||
pending-render-ids only): `DetachNearLayer` returns a receipt without ever
|
||||
entering the `_loaded` branch (`:1854-1889`), so `_tierByLandblock` is
|
||||
untouched and the guard's `_loaded` lookup fails.
|
||||
- **Mixed-kind ordering.** NearLayer ticket pending, then a Full begins:
|
||||
`BeginCore`'s same-kind scan (`:647-668`) does not match, `DetachLandblock`
|
||||
clears both maps, and the stale NearLayer ticket's release finds nothing.
|
||||
Reverse order: `DetachNearLayer` returns `null` (`hadNearLayer` false), so no
|
||||
second ticket exists.
|
||||
|
||||
**Verdict:** the guard cannot resurrect a registration on a genuinely unloaded
|
||||
landblock. No scenario found.
|
||||
|
||||
### 2.2 Ordering, interleaving, double-fire, clobbering — clean.
|
||||
|
||||
- **"Converges before the re-assert" is structurally enforced, not asserted.**
|
||||
`LandblockSpawnAdapter.OnLandblockUnloaded` ends in `ThrowFailures`
|
||||
(`:180-182`), so any unreleased reference throws out of the method and the
|
||||
re-assert is unreachable. The retirement ticket retries; the second pass
|
||||
finds the registration already dropped (`:161-162` early return), does not
|
||||
throw, and the re-assert then fires. Convergent.
|
||||
- **No reentrancy.** For a demote the re-assert's `unique` set is empty, so
|
||||
`OnLandblockLoaded` makes **zero** adapter calls. The unload's
|
||||
`DecrementRefCount` path (`WbMeshAdapter.cs:405-409`) only forwards to
|
||||
`ObjectMeshManager` and never re-enters `GpuWorldState`.
|
||||
- **Cannot fire twice per ticket.** `RunOnce`/`RunOnceStep` mark the
|
||||
`MeshReferences` stage complete on success.
|
||||
- **Cannot clobber a live registration.** This is the important one, and it is
|
||||
a real safety property rather than an accident:
|
||||
`OnLandblockLoaded` calls `MarkAllUndesired` **only** inside
|
||||
`else if (!registration.WantsLoaded)` (`LandblockSpawnAdapter.cs:103-112`).
|
||||
Re-asserting an empty set over a still-live registration therefore marks
|
||||
nothing undesired, releases only already-undesired leftovers, and re-acquires
|
||||
desired-but-unheld ones. It is a convergence step, never a teardown.
|
||||
|
||||
### 2.3 Is the empty set genuinely empty? — Yes, verified both halves.
|
||||
|
||||
- `DetachNearLayer` (`GpuWorldState.cs:1866-1885`) puts only `ServerGuid != 0`
|
||||
entities into `retainedLive`; every `ServerGuid == 0` entity goes to
|
||||
`retiredEntities` and `RemoveFlatEntity`.
|
||||
- `OnLandblockLoaded`'s atlas-tier filter skips exactly `ServerGuid != 0`
|
||||
(`LandblockSpawnAdapter.cs:88`). Intersection is empty by construction.
|
||||
- **Live server entities lose nothing**, because they were never registered
|
||||
here — they belong to `EntitySpawnAdapter`, and nothing in
|
||||
`ReleaseLandblockMeshReferences` touches it.
|
||||
- **Prepared (EnvCell shell) ids also match**, which the commit asserts but does
|
||||
not show. Verified: a `PublicationKind.Far` transaction builds
|
||||
`farLandblock` with `Array.Empty<WorldEntity>()` and
|
||||
`new LandblockBuild(farLandblock, Origin)` — no `EnvCells`
|
||||
(`LandblockPresentationPipeline.cs:442-464`), so
|
||||
`renderIds = transaction.Build.EnvCells?.Shells…` is `null` at `:899-901`.
|
||||
The re-assert's `additionalReadinessIds: null` therefore reproduces the Far
|
||||
registration exactly, `Prepared` dictionary included.
|
||||
- The ordering claim added to `StreamingController.cs:271-279` also checks out:
|
||||
presentation (terrain upload) commits at `PresentationCommitted`
|
||||
(`LandblockPresentationPipeline.cs:874`) **before**
|
||||
`SpatialPresentationCommitted` runs `ActivateLandblockPresentation` →
|
||||
`OnLandblockLoaded` (`:882-925`).
|
||||
|
||||
### 2.4 All four call sites — correct, and the placement argument holds.
|
||||
|
||||
| Site | Path | Behaviour under the new meaning |
|
||||
|---|---|---|
|
||||
| `LandblockRetirementCoordinator.cs:740` (`AdvanceTicket`, legacy) | after detach commit | full → guard fails; near-layer → re-assert. Correct. |
|
||||
| `LandblockRetirementCoordinator.cs:801` (`AdvanceTicketOne`, budgeted) | after detach commit | same. This is production's path (`CreateBudgeted`) and has its own test. |
|
||||
| `GpuWorldState.cs:1549` (`RemoveLandblock`) | after `DetachLandblock` | both maps cleared → guard fails. Correct. |
|
||||
| `GpuWorldState.cs:1907` (`RemoveEntitiesFromLandblock`) | after `DetachNearLayer` | re-assert fires. Correct — it is the same demote semantics. |
|
||||
|
||||
The architecture review's "test-only compatibility edge" flag on
|
||||
`RemoveEntitiesFromLandblock` is accurate and harmless: `grep` shows zero
|
||||
production callers for it or for `GpuWorldState.RemoveLandblock` (the many
|
||||
`.RemoveLandblock(` hits are on `TerrainModernRenderer`, `EnvCellRenderer`,
|
||||
`CellVisibility`, `PhysicsEngine.ShadowObjects`, `CellGraph` — different
|
||||
types). Placing the fix at the state owner rather than the retirement stage is
|
||||
the right call: it covers all four with one invariant, and the two compat edges
|
||||
now agree with production instead of diverging from it.
|
||||
|
||||
### 2.5 R-2 (composite-warmup trigger) — a real restoration, gate not weakened.
|
||||
|
||||
Checked against `3aab05b0^` (pre-#280): `Prepare` used a single
|
||||
`radius = RequiredRenderRadius(cell)` = `OutdoorNeighborhoodRadius` = 1 for
|
||||
**both** the readiness test and `_prepareCompositeTextures`. Trigger scope ==
|
||||
domain scope. #280 widened only the gate. The new
|
||||
`_isRenderNeighborhoodReady(cell, near, near)` restores that identity.
|
||||
|
||||
- **The gate is untouched.** `Evaluate` still calls
|
||||
`_isRenderNeighborhoodReady(cell, required.NearRadius, required.FarRadius)`
|
||||
and still ANDs `_areCompositeTexturesReady()`
|
||||
(`WorldRevealReadinessBarrier.cs:184-194`).
|
||||
- **The trigger is not a rubber stamp.** With `farRadius == nearRadius`, every
|
||||
member of `IsRenderNeighborhoodResident`'s loop satisfies `isInnerRing`, so it
|
||||
demands `IsNearTier && IsRenderReady` for the whole near square — the strict
|
||||
pre-#280 predicate, not a relaxation.
|
||||
- **Starting earlier cannot latch a stale ready.**
|
||||
`WbDrawDispatcher.PrepareCompositeTextures` is built for repeated per-frame
|
||||
calls: `RequiresCompositeWarmupRebuild` / `ShouldBeginCompositeWarmupRescan`
|
||||
run *before* the `if (CompositeTexturesReady) return` early-out (`:168-189`),
|
||||
and a rescan resets `CompositeTexturesReady` (`:330-332`). Far-tier builds
|
||||
carry no entities, so far-ring publication adds nothing to the domain anyway.
|
||||
- Two tests cover both directions
|
||||
(`Prepare_StartsWarmupOnceTheNearSubWindowIsPublished`,
|
||||
`Prepare_StillWaitsWhenTheNearSubWindowIsIncomplete`), and the first also
|
||||
asserts `IsReady` is still false.
|
||||
|
||||
It **is** a behaviour change riding in a fix commit. It is disclosed in the
|
||||
message, scoped to warmup timing, tested, and provably a restoration rather
|
||||
than a new position. Acceptable.
|
||||
|
||||
### 2.6 R-1 (`ParseRadius` floor 1) — correct and inert.
|
||||
|
||||
`ACDREAM_PROBE_REVEAL_RADIUS=0` yielded `far = 0` for an outdoor destination
|
||||
(`ApplyRevealRadiusOverride`, `StreamingDiagnostics.cs:48-51`), which Runtime's
|
||||
`invalid-readiness-shape` invariant rejects on every acknowledgement. Rejecting
|
||||
it at the parser makes the probe fall back to the derivation. Diagnostic-only,
|
||||
7-case table test, no production behaviour touched.
|
||||
|
||||
### 2.7 Blast radius across both hosts — verified.
|
||||
|
||||
`GpuWorldState`, `LandblockSpawnAdapter`, `WorldRevealReadinessBarrier`,
|
||||
`StreamingDiagnostics` appear in `AcDream.Headless`, `AcDream.Runtime`,
|
||||
`AcDream.Core`, `AcDream.Core.Net`, `AcDream.UI.Abstractions` **only in
|
||||
comments**: `HeadlessSessionWorldProjection.cs:963`,
|
||||
`RuntimeRemotePlacementDriveController.cs:13` and `:356`,
|
||||
`RuntimeWorldTransitState.cs:576`, `WorldEntity.cs:14`. `AcDream.Headless.Tests`
|
||||
ran 89/89 green in my own clean run. C5b's "survey skipped the no-window host"
|
||||
failure mode is not repeated.
|
||||
|
||||
Worth recording: `tests/AcDream.Core.Tests/Streaming/GpuWorldStateTwoTierTests.cs`
|
||||
exercises `GpuWorldState` from the *Core* test project. That is a test-project
|
||||
reference, not a production dependency, and all 4,263 Core tests are green — but
|
||||
it means "App-internal" is true of production only.
|
||||
|
||||
---
|
||||
|
||||
## 3. Retail-conformance lens
|
||||
|
||||
All addresses below were decoded from
|
||||
`C:\Users\erikn\Downloads\acclient.exe`, confirmed `MATCH` against
|
||||
`refs/acclient.pdb`. **Where a comparison or constant is load-bearing I
|
||||
disassembled rather than trusting Binary Ninja**, per the C5b/#317 lesson: BN
|
||||
prints `bool p_2 = /* unimplemented {test ah, 0x41} */` at the exact site
|
||||
AP-150 rests on.
|
||||
|
||||
### 3.1 AP-150 clause 1 — "unconditional per tunnel rotation segment": CONFIRMED
|
||||
|
||||
`gmSmartBoxUI::UseTime` @`0x004D6E30`. Decoded from `0x004D6FC1`:
|
||||
|
||||
```
|
||||
004D6FC1 dc 86 40 06 00 00 fadd qword [esi+0x640] ; + teleportRotationStartTime
|
||||
004D6FC7 dc 5c 24 18 fcomp qword [esp+0x18] ; vs Timer::cur_time
|
||||
004D6FCB df e0 fnstsw ax
|
||||
004D6FCD f6 c4 41 test ah, 0x41 ; C0|C3
|
||||
004D6FD0 0f 8a de 00 00 00 jp 0x004D70B6
|
||||
```
|
||||
|
||||
`test ah,0x41` sets PF from the popcount of `ah & 0x41`: `0x00` (segment still
|
||||
running) and `0x41` (unordered) are even → **jump taken**, into the
|
||||
angle-interpolation block at `0x004D70B6`. `0x01` (expired) and `0x40` (exactly
|
||||
equal) are odd → **fall through**, into the re-randomise + emit block. So the
|
||||
notice sits on the segment-**expired** path, which is what AP-150 says.
|
||||
|
||||
Within that block the only conditional jumps are the temporary string's
|
||||
refcount-release guard:
|
||||
|
||||
```
|
||||
004D708C 75 0c jnz 0x004D709A
|
||||
004D7090 74 08 jz 0x004D709A
|
||||
...
|
||||
004D709A 8d 4c 24 30 lea ecx, [esp+0x30]
|
||||
004D709E 51 push ecx
|
||||
004D709F 6a 1a push 0x1A
|
||||
004D70A1 e8 0a b5 1b 00 call 0x006925B0 ; ECM_UI::SendNotice_DisplayStringInfo
|
||||
```
|
||||
|
||||
Both branch targets are `0x004D709A`, i.e. immediately *before* the call. There
|
||||
is no `blocking_for_cells` test, no elapsed-time test, no once-only latch. The
|
||||
emit is unconditional on every segment expiry while the state is one of
|
||||
`TAS_TUNNEL{,_FADE_IN,_CONTINUE,_FADE_OUT}`. **Claim confirmed.**
|
||||
|
||||
### 3.2 AP-150 clause 2 — `RandDouble(0.6, 1.8)` at `0x004D6FE6`: CONFIRMED
|
||||
|
||||
```
|
||||
004D6FE6 68 cc cc fc 3f push 0x3FFCCCCC ; hi(1.8)
|
||||
004D6FEB 68 cd cc cc cc push 0xCCCCCCCD ; lo(1.8)
|
||||
004D6FFA 68 33 33 e3 3f push 0x3FE33333 ; hi(0.6)
|
||||
004D6FFF 68 33 33 33 33 push 0x33333333 ; lo(0.6)
|
||||
004D7016 e8 b5 d3 1a 00 call 0x006843D0 ; RandDouble (symbols.json ✓)
|
||||
004D701B dd 9e 48 06 00 00 fstp qword [esi+0x648] ; teleportRotationDuration
|
||||
```
|
||||
|
||||
`0x3FFCCCCCCCCCCCCD` = 1.8, `0x3FE3333333333333` = 0.6. cdecl pushes reverse,
|
||||
so the last push (0.6) is arg 1 → **`RandDouble(0.6, 1.8)`**, exactly as
|
||||
claimed, and `0x004D6FE6` is the correct anchor. The sibling
|
||||
`teleportRotationEndAngle = RandDouble(0, 360)` also checks out
|
||||
(`push 0x40768000` @`0x004D702D` = hi(360.0), three `push 0` for lo/0.0).
|
||||
|
||||
acdream's `PortalTunnelPresentation.cs:62-63` are `RotationDurationMin = 0.6f`,
|
||||
`RotationDurationMax = 1.8f`, and `TickRotation` re-randomises with
|
||||
`NextDouble(0.0, 360.0)` — cadence is faithful. The arming is not:
|
||||
`RuntimeWorldTransitState.cs:66-67` `RetailWaitCueDelay = 5 s`, enforced at
|
||||
`:680` (`elapsed < RetailWaitCueDelay → return false`) with a once-only
|
||||
`WaitCueShown` latch, and `PortalTunnelPresentation.cs:380` re-emits only
|
||||
`if (_waitCueVisible)`. **"Only the arming is wrong" is exactly right.**
|
||||
|
||||
### 3.3 AP-150 clause 3 — the 5.0 at `0x007991B0` is unrelated: CONFIRMED
|
||||
|
||||
`CellManager::CheckPrefetchStatus` @`0x00455BE0`:
|
||||
|
||||
```
|
||||
00455BE0 dd 05 a8 69 83 00 fld qword [0x008369A8] ; Timer::cur_time
|
||||
00455BE9 dc 66 10 fsub qword [esi+0x10] ; - last_prefetch_check
|
||||
00455BEE dc 1d b0 91 79 00 fcomp qword [0x007991B0] ; vs 5.0
|
||||
00455BF4 df e0 fnstsw ax
|
||||
00455BF6 f6 c4 41 test ah, 0x41
|
||||
00455BF9 75 2a jnz -> return 0
|
||||
```
|
||||
|
||||
The double at `0x007991B0` is exactly `5.0`. It is a prefetch **retry throttle**
|
||||
— skip `PreFetchCells` unless 5 s have elapsed. The tunnel-cue code path
|
||||
`0x004D6FC0`–`0x004D70B2` contains no reference to `0x007991B0`. **#280's
|
||||
commit message mis-attribution is correctly retracted.**
|
||||
|
||||
### 3.4 AP-151 — the gate is stricter than retail's DAT-residency predicate: CONFIRMED
|
||||
|
||||
`LScape::PreFetchCells` @`0x00505660` walks the `mid_radius` square, applies the
|
||||
`>= 0x7f8` bounds test the `StreamingController` comment cites, and per member
|
||||
does only `DBObj::PreFetch` → `CACHE_OBJECT_IN_MEMORY`/`IN_FILE` → `DBObj::Get`
|
||||
non-null → `CLandBlock::PreFetchCells`. No geometry construction, no vertex
|
||||
buffers, no upload — all of that is lazy at draw. acdream's per-member predicate
|
||||
(worker DAT read, terrain mesh build, render-thread upload, spatial commit,
|
||||
collision admission, spawn-adapter activation, metered) is unambiguously
|
||||
heavier. **Claim confirmed**, and correctly recorded as the *opposite*
|
||||
asymmetry from AP-149.
|
||||
|
||||
### 3.5 Collateral citations, all spot-checked
|
||||
|
||||
| Citation | Result |
|
||||
|---|---|
|
||||
| `SmartBox::set_mid_radius` @`0x00453180` (#326 correction) | ✅ `symbols.json`; and `0x004531D0` is genuinely the mid-function re-arm — `CellManager::ChangePosition(cell_manager, &player->m_position, 1)` |
|
||||
| `SmartBox::SetRegion` @`0x004531F0` | ✅ and it assigns `mid_radius` from `Render::m_RenderPrefs.LandscapeDrawDistance` |
|
||||
| `Render_LandscapeDrawDistance_Values` @`0x007CA988` = {3,5,8,11,15,25} | ✅ byte-read from `.rdata` |
|
||||
| `LScape::SetMidRadius` @`0x00504C00`, `SmartBox::UseTime` @`0x00455410`, `CellManager::PreFetchCells` @`0x00455820`, `CEnvCell::PreFetchCells` @`0x0052D1E0`, `CLandBlock/CLandBlockInfo/CBldPortal::PreFetchCells` @`0x00530240`/`0x0052E7C0`/`0x0053BD00`, `gmSmartBoxUI::BeginTeleportAnimation` @`0x004D6300` | ✅ all match |
|
||||
| Wait-cue string VA `0x007BD6A8` | ✅ `push 0x7BD6A8` @`0x004D705B` |
|
||||
| Issue `#327` referenced by AP-151 | ✅ exists (`docs/ISSUES.md:1761`) |
|
||||
|
||||
---
|
||||
|
||||
## 4. Findings
|
||||
|
||||
### Defect — LOW (documentation)
|
||||
|
||||
**D1. AP-150 cites the wrong address for the notice call.**
|
||||
`docs/architecture/retail-divergence-register.md:179` reads
|
||||
"`ECM_UI::SendNotice_DisplayStringInfo` call @0x004D7064". `0x004D7064` is the
|
||||
`PStringBase<unsigned short>::PStringBase` constructor call. The
|
||||
`SendNotice_DisplayStringInfo` call is at **`0x004D70A1`**
|
||||
(`e8 0a b5 1b 00` → `0x006925B0`, which `symbols.json` confirms is
|
||||
`ECM_UI::SendNotice_DisplayStringInfo`). Every other address in the row is
|
||||
correct, and neither `docs/ISSUES.md` #329 nor the contract doc repeats the
|
||||
error. This is precisely the class of off-by-one-call-site citation error the
|
||||
same commit corrected for #326 — worth fixing so a future reader setting a cdb
|
||||
breakpoint from the register lands on the right instruction.
|
||||
|
||||
### Latent risks
|
||||
|
||||
**L1 — LOW. The "empty by construction" invariant is guarded, but not at the
|
||||
seam this fix newly depends on.**
|
||||
The new doc-comment on `ReleaseLandblockMeshReferences` states the re-assert is
|
||||
safe because "the retained entity list holds only live server projections,
|
||||
which the adapter's atlas-tier filter skips". I tested that dependency: I
|
||||
removed the filter (`if (entity.ServerGuid != 0) continue;`,
|
||||
`LandblockSpawnAdapter.cs:88`) and **all 4,170 App tests passed**. Only two
|
||||
tests anywhere caught it, both in `AcDream.Core.Tests`
|
||||
(`LandblockSpawnAdapterTests.OnLandblockLoaded_SkipsServerSpawnedEntities`,
|
||||
`PendingSpawnIntegrationTests.LiveEntity_ParkedBeforeLandblock_DrainsButIsNotRegisteredWithAdapter`),
|
||||
and both at the adapter's unit level rather than through a demote.
|
||||
No test demotes a landblock retaining a live server projection that carries
|
||||
`MeshRefs` — the discriminating case. The filter itself is protected, so this
|
||||
is not a defect; but if it were ever relaxed deliberately, a Far-tier
|
||||
landblock's `IsRenderReady` would start depending on live-entity mesh
|
||||
readiness, which is a D-1 variant (a live entity whose mesh never becomes ready
|
||||
makes the outer ring permanently un-ready) and nothing at the composition level
|
||||
would fail. One test with a live entity carrying a `MeshRef` through
|
||||
`BeginNearLayerRetirement` would close it.
|
||||
|
||||
**L2 — LOW. The fix is repair-after-self-inflicted-teardown, not
|
||||
release-the-near-layer-only.**
|
||||
`ReleaseLandblockMeshReferences` unregisters then re-registers. That is
|
||||
functionally sound today because (a) nothing observes `WantsLoaded` between the
|
||||
two synchronous calls, and (b) `RunOnce`/`RunOnceStep` make the whole method
|
||||
all-or-nothing so it can never be suspended between them. The stronger shape
|
||||
would be "release the Near layer's references and leave the registration
|
||||
standing", which would carry the invariant structurally. As written, a future
|
||||
budgeted split of this method into two metered steps would silently reopen
|
||||
D-1 — the ticket-stage granularity is the only thing preventing it, and that is
|
||||
not stated at the site.
|
||||
|
||||
**L3 — LOW. The restated P2 obligation is literally stronger than what the code
|
||||
guarantees.**
|
||||
`docs/research/2026-08-05-280-contract.md` now states: *"no transition may
|
||||
revoke `IsRenderReady` from a landblock that stays inside `FarRadius`"*. A
|
||||
Far→Near **promote** does revoke it transiently — new entity mesh references
|
||||
are registered before their upload completes — and that is correct behaviour,
|
||||
and is how the inner arm is meant to be satisfied. The obligation that actually
|
||||
holds is *"…may revoke it with no path to restoration"*. Since the first
|
||||
wording of P2 (discharged against residency instead of `IsRenderReady`) is
|
||||
exactly what let D-1 ship, the precision of the restatement matters more here
|
||||
than usual.
|
||||
|
||||
### Style
|
||||
|
||||
**S1.** `ReleaseLandblockMeshReferences` calls
|
||||
`_wbSpawnAdapter.OnLandblockUnloaded(landblockId)` with the **raw** id
|
||||
(`GpuWorldState.cs:1602`) but performs both guard lookups on `canonical`
|
||||
(`:1604-1611`). Production always passes canonical ids (both coordinator paths
|
||||
canonicalize at `Begin*`; both compat edges pass `retirement.LandblockId`), so
|
||||
this is inert today. But inside a nine-line method the asymmetry invites a
|
||||
future non-canonical caller into "unregister nothing, then register something",
|
||||
which the registration's idempotence would silently absorb. Canonicalize once
|
||||
at the top.
|
||||
|
||||
**S2.** `WorldRevealDerivedWindowIntegrationTests.RecordingMeshAdapter`
|
||||
implements `IsRenderDataReady(ulong) => true`, so the mesh-readiness arm of
|
||||
`IsLandblockRenderReady` is still stubbed in that file — the sibling
|
||||
`ReadinessMeshAdapter` in `StreamingControllerReadinessTests` uses honest set
|
||||
membership. The fixture change removed the load-bearing tautology (the
|
||||
`?? true`), which is the one that mattered; this residual is worth a comment so
|
||||
the next reader does not over-read "real spawn adapter" as "real readiness".
|
||||
|
||||
**S3.** `StreamingDiagnostics.ParseRadius` widened `private` → `internal` purely
|
||||
so the 7-case table test could call it. Reasonable, noted only because it is a
|
||||
test-driven visibility relaxation on a diagnostic owner.
|
||||
|
||||
---
|
||||
|
||||
## 5. Process-rule compliance
|
||||
|
||||
Checked against `CLAUDE.md`: no suppression flag, no grace period, no symptom
|
||||
guard, no retry loop, no `try/catch` swallow, no new skip, no weakened
|
||||
assertion. Skips stayed at 4. The rejected alternative
|
||||
(`|| (IsFarTier && IsLoaded)` at the gate) is correctly identified as the
|
||||
symptom-guard shape and correctly declined. The register rule was honoured —
|
||||
AP-150 and AP-151 were filed in the same commit as the finding, AD-2's false
|
||||
clause was corrected rather than left standing, AP-115 was scope-noted, and the
|
||||
user-visible presentation change (retail's unconditional emit) was filed as
|
||||
#329 rather than folded in. Known flakes #302/#308/#321 did not surface in my
|
||||
run and are not conflated with anything above.
|
||||
|
||||
## 6. What was checked and found clean (so the PASS is auditable)
|
||||
|
||||
Guard reachability during full retirement, removal, origin recenter, session
|
||||
reset, generation change and same-id replacement · retirement-ticket ordering
|
||||
relative to spatial detach · same-kind and mixed-kind ticket interleaving ·
|
||||
throwing-release retry semantics · adapter reentrancy into `GpuWorldState` ·
|
||||
double-fire · live-registration clobbering · atlas-tier emptiness · prepared
|
||||
(EnvCell) id parity between demote and Far arrival · terrain-upload-before-
|
||||
registration ordering · all four `ReleaseLandblockMeshReferences` call sites ·
|
||||
production-caller census for both compat edges · `Evaluate` unchanged by R-2 ·
|
||||
`IsRenderNeighborhoodResident(cell, near, near)` strictness · composite-warmup
|
||||
rescan/reopen semantics · R-1 probe floor · cross-assembly references from
|
||||
Headless/Runtime/Core/Core.Net/UI.Abstractions · clean-rebuild suite totals and
|
||||
their per-assembly decomposition · the +13 delta reconciliation · both
|
||||
sabotages · nine retail symbol addresses, four byte-level decodes, and two
|
||||
IEEE-754 constant pairs.
|
||||
Loading…
Add table
Add a link
Reference in a new issue