# #280 architecture review — ownership, layering, blast radius, test quality - **Commit under review:** `3aab05b0` — *fix(streaming): derive the portal reveal window from the live streaming radii (#280)* - **Branch / HEAD:** `claude/acdream-physics-divergence-5aa784` @ `fafc0b65` - **Worktree:** `.claude/worktrees/peaceful-visvesvaraya-e0a196` - **Scope:** ownership, layering, blast radius, test quality. Retail fidelity is a separate reviewer's. - **Mode:** read-only. Production edits were made only as sabotage probes and reverted; the tree is clean at `fafc0b65` with no working-tree changes. ## Verdict: **FAIL** One confirmed defect, reproduced end-to-end through production code paths. The predicate D2 widened from a 3×3 always-Near neighbourhood to the full Far window now demands `IsRenderReady` from landblocks that the streaming system **permanently** takes out of render-readiness while leaving them loaded. There is no code path that restores them. Under two ordinary player flows — one of which is the exact mid-hold radius change this commit added `ReconcileDestinationReservationRadius` to support — the reveal gate becomes unsatisfiable and the client stays in "In Portal Space - Please Wait…" forever. Everything else checked out. Layering held, the no-window host is genuinely untouched, the allocation fix is real, and the tests are unusually well disciplined about not re-encoding the constant under test. The failure is a single missed state transition, not sloppy work — but it is the failure mode the change's own proof obligation P2 was written to exclude, discharged against the wrong predicate. --- ## Defects ### D-1 — A demoted-but-loaded landblock is permanently not `IsRenderReady`, so the widened gate can never open (**CONFIRMED, reproduced**) **Severity: defect. Blocks the fix.** `src/AcDream.App/Streaming/StreamingController.cs:270-278` — the new outer arm requires `_state.IsRenderReady(canonical)` of every in-bounds member out to `farRadius`. `IsRenderReady` is not a property of "loaded": - `src/AcDream.App/Streaming/GpuWorldState.cs:179-181` `IsRenderReady = _loaded.ContainsKey(id) && _wbSpawnAdapter.IsLandblockRenderReady(id)` - `src/AcDream.App/Rendering/Wb/LandblockSpawnAdapter.cs:135-139` returns **false** when the landblock has no registration, or a registration with `WantsLoaded == false`. A Near→Far **demote** destroys exactly that registration while keeping the landblock loaded: - `src/AcDream.App/Streaming/StreamingController.cs:555-559` `DemoteLandblock` → `LandblockPresentationPipeline.EnqueueNearLayerRetirement` - `src/AcDream.App/Streaming/LandblockRetirementCoordinator.cs:309-312` a `NearLayer` ticket still runs `CoreStages`, which includes `LandblockRetirementStage.MeshReferences` - `src/AcDream.App/Streaming/LandblockRetirementCoordinator.cs:738-740` → `GpuWorldState.ReleaseLandblockMeshReferences` - `src/AcDream.App/Streaming/GpuWorldState.cs:1559-1560` → `LandblockSpawnAdapter.OnLandblockUnloaded` → `WantsLoaded = false`, registration dropped - `src/AcDream.App/Streaming/GpuWorldState.cs:1800-1835` meanwhile `DetachNearLayer` **keeps** `_loaded[canonical]` and flips `_tierByLandblock[canonical] = Far` Nothing re-publishes an already-loaded landblock. `RecenterTo` (`StreamingRegion.cs:232-244`) emits `ToLoadFar` only for ids absent from `_tierResidence`, and a demoted block is still present as `Far`. Only a **promote** back into the Near ring (`StreamingRegion.cs:239-244` → `PublicationKind.PromoteExisting` → `ActivateLandblockPresentation` → `OnLandblockLoaded`) restores the registration. Members of the band `NearRadius+1 … FarRadius` are never promoted, so they stay dead for the rest of the window's life. **Evidence (temporary probes, run then removed):** | Probe | Result | |---|---| | `GpuWorldState.IsRenderReady` after `LandblockPresentationPipeline.BeginNearLayerRetirement` (the production demote entry point) | **false** | | `GpuWorldState.IsRenderReady` after `GpuWorldState.RemoveEntitiesFromLandblock` (same `DetachNearLayer` + `ReleaseLandblockMeshReferences` pair) | **false** | | Full Near 5×5 window; `controller.IsRenderNeighborhoodResident(dest, 1, 2)` before one outer-ring demote | **true** | | …the same call after that single demote | **false**, and never recovers | All three probes used a real `GpuWorldState` + real `LandblockSpawnAdapter` + real `LandblockPresentationPipeline`. **Reachable failure scenarios** 1. **Mid-hold quality change (the one this commit explicitly added code for).** `StreamingController.ReconfigureRadii` at `:484-488` emits `DemoteLandblock` for every currently-Near landblock that the new preset puts in the Far band. Presets are High `(4,12)` → Low `(2,5)` (`src/AcDream.UI.Abstractions/Settings/QualityPreset.cs:31-34`), so a High→Low switch demotes the Chebyshev 3–5 band, which is *inside* the new Far window. The gate then requires `IsRenderReady` from all of them. Result: the hold never ends, and `WorldRevealCoordinator.ReconcileDestinationReservationRadius` (`:521-543`) cheerfully re-opens a reservation on a square that can no longer converge. Note also that `NearRadius`/`FarRadius` only publish at transaction convergence (`StreamingController.cs:886-888`) while the demote mutations run earlier (`:860-876`), so for several frames the barrier measures the OLD wide window against blocks already demoted for the NEW narrow one. 2. **A reveal that does not recenter the origin.** `LocalPlayerTeleportController.cs:934` only calls `_streaming.BeginRecenter` when `transition.ChangesStreamingCenter`, i.e. when the destination landblock differs from the **world-origin** landblock. Origin recenter is the only thing that clears the window (`GpuWorldState.cs:1481` `_loaded.Clear()`). Walking ≳ `NearRadius + 2` landblocks from the last origin (`StreamingRegion.cs:263-270`) demotes blocks into the Far band; a subsequent teleport back to the origin landblock (lifestone recall → travel → lifestone recall) takes the no-recenter path and inherits them. Blocks that land back inside the Near ring are promoted and recover; blocks in `near+1 … far` do not. **Why the old gate was safe:** at `OutdoorNeighborhoodRadius = 1` the required set was the destination's own 3×3, which during a hold is pinned to the streaming centre (`StreamingFrameController.SelectObserver:194-200` freezes the observer at `_origin` while `PlayerState.PortalSpace`) and therefore always Near tier. Demotion inside the Near ring cannot happen. The defect is introduced by this commit, not exposed by it. **Why P2 missed it.** `docs/research/2026-08-05-280-contract.md:876-884` states P2 as "an outer-ring member cannot be **evicted** while it is inside `FarRadius`" and cites the `FarRadius + 2` unload threshold. That is true and irrelevant: the gate's atom is `IsRenderReady`, not residency. P2 was discharged against the wrong predicate. Every remaining sentence of the static proof (hysteresis, recenter, dungeon collapse) is correct. **Also note:** the P1 test's own comment (`tests/AcDream.App.Tests/Streaming/StreamingControllerReadinessTests.cs:567-570`) describes its subject as *"A Near-shaped completion that the streaming window has since demoted to Far"*. It is not — it is a fresh `PublishAsFar` of a landblock that was never loaded, which is the case that **does** hold. The comment names the one case that is false. A future reader will take it as coverage of the demote path. --- ## Latent risks ### R-1 — `ACDREAM_PROBE_REVEAL_RADIUS=0` is accepted by the parser and rejected by Runtime `src/AcDream.App/Streaming/StreamingDiagnostics.cs:54-55` accepts any `value >= 0`. `ApplyRevealRadiusOverride` then yields `StreamingRevealWindow(0, 0)`, so `RequiredWindow` returns far = 0 for an **outdoor** destination, and `src/AcDream.Runtime/World/RuntimeWorldTransitState.cs:585-597` fails `invalid-readiness-shape` on every acknowledgement (outdoor ⇒ `>= 1`). The A/B probe would hang the route it is meant to measure. The documented value is `1`; the parser should refuse `0` rather than let the two halves of the same commit disagree. One line. ### R-2 — the composite-warmup **trigger** moved onto the far window's critical path, undocumented `WorldRevealReadinessBarrier.Prepare:120-133` starts composite preparation only once `_isRenderNeighborhoodReady(dest, near, far)` is true. Before this commit that predicate was the destination's 3×3; it is now the entire 25×25. The commit message and D3 justify keeping the composite **radius** at `NearRadius` (correct — Far builds carry no entities), but say nothing about the trigger. Net effect: composite upload can no longer overlap far-ring streaming; the hold is longer than the streaming work alone requires by the whole warmup duration. This is a real serialisation the "expect longer holds" paragraph does not account for, and it is separable — warming composites at Near-ready would restore the overlap without weakening the gate. ### R-3 — two different windows describe "destination publication incomplete" `StreamingController.cs:719-724` computes the destination-lane preference with `(Math.Min(NearRadius, DestinationRadius), DestinationRadius)` — live `NearRadius` against the *reservation's* radius — while the gate uses `RequiredWindow`'s `(clamp(NearRadius, 0, FarRadius), FarRadius)`. Between a preset change and reservation reconciliation these name different squares, so the work-lane prioritisation can consider the destination complete on a square the gate still rejects. Cosmetic today; it becomes a real starvation question once R-1/D-1 are fixed and holds get long. ### R-4 — Runtime lost its only cross-host consistency check The `indoor ? 0 : 1` equality became `indoor ⇒ 0`, `outdoor ⇒ ≥ 1` (`RuntimeWorldTransitState.cs:585-597`). The layering argument is right — Runtime cannot learn the graphical host's streaming configuration, and plumbing App radii in would be exactly the C5b failure. The cost is that Runtime can no longer detect an App regression to a hardcoded radius: a future revert of D1 would emit `1` and pass. Accepted, but worth recording that the invariant is now shape-only and the only remaining guard on the value is the App-side tests. --- ## What was checked and is sound **Layering / ownership (attack 6) — held.** `WorldRevealCoordinator` and `WorldRevealReadinessBarrier` are `internal` to `AcDream.App`. `src/AcDream.Headless/AcDream.Headless.csproj` references only `AcDream.Runtime`. Both non-graphical producers (`HeadlessSessionWorldProjection.cs:979`, `RuntimeLiveEntitySessionController.cs:782`) keep emitting their own centre-ring token and are legal under the loosened shape by construction, and both were annotated in place explaining why they must **not** track the graphical radius. No App type crossed into Runtime. `AcDream.Headless.Tests` passes 89/89 including its dependency/loaded-assembly guards. C5b's lesson was applied, not repeated. **P1 (attack 2) — independently verified TRUE, by reading rather than by reading the test.** `PublishAsFar` (`LandblockPresentationPipeline.cs:432-522`) constructs a `LoadedLandblock` with `Array.Empty()` and `PhysicsDatBundle.Empty`; the spatial commit (`LandblockPresentationPipeline.cs:900-916`) routes `PublicationKind.Far` through `CommitLandblockSpatial`, which returns a publication with `RequiresActivation` defaulted true (`GpuWorldState.cs:24`), so `ActivateLandblockPresentation` (`:1009-1035`) calls `OnLandblockLoaded` with an empty entity list and empty `AdditionalRenderIds`. The registration is created with `WantsLoaded = true` and both reference dictionaries empty, so `IsLandblockRenderReady` returns true with no `IWbMeshAdapter` upload. The one early-out (`GpuWorldState.cs:913-923`, a stale Far completion over a live Near tier) returns `RequiresActivation: false`, but that case is already registered. **P1 holds. The fix shape does not collapse.** Its failure is D-1, a different transition. Same check for the collision arm, which is equally load-bearing and was not called out as a proof obligation: `LandblockPhysicsPublisher.AdvanceBeginOne` (`:390-435`) builds the terrain surface from the heightmap with no dat bundle and stages it, so a Far publication does register terrain collision; `AdvanceDemotion` (`:675-683`) calls `DemoteCollisionToTerrain`, which **retains** terrain. `IsNeighborhoodTerrainResident` therefore stays satisfiable across a demote. The render arm is the only one that breaks. **Reveal-hang sweep (attack 1) — all other configurations converge.** | Configuration | Result | |---|---| | Map edge (coords clamped 0/254) | Safe. Gate skips `> 254` (`StreamingController.cs:265-266`); `StreamingRegion` loads up to `0xFF` (`:80`). The loaded set is a strict superset of the required set, and `PhysicsEngine.cs:157` uses the identical bound. | | Dungeon → outdoor | Safe. `TryCommitOriginRecenterCore:1235-1251` clears `_collapsed` for a non-dungeon destination and nulls `_region` so the next tick bootstraps the full window. | | Indoor destination | Safe. `RequiredWindow` returns `(0,0)` before touching the live window (`WorldRevealReadinessBarrier.cs:206-215`), so no streaming state can gate it. | | Recenter in flight | Safe. `Tick` is blocked while `_originRecenterRetirement` is open, radii requests defer, and `_loaded.Clear()` (`GpuWorldState.cs:1481`) guarantees every member of the rebuilt window gets a fresh publication and registration. | | Destination/observer landblock offset | Safe **during a hold**: `StreamingFrameController.SelectObserver:194-200` pins the observer to `_origin` while `PlayerState.PortalSpace`, so the gate's inner ring and the streaming Near ring are the same square. Worth noting the new inner arm has **zero** margin here where the old one had `NearRadius − 1`; the pin is now load-bearing. | | `FarRadius == 0` on an outdoor destination | Not reachable from presets (min far = 5); reachable only via R-1. | **D6 allocation fix — real, and the sabotage number is honest.** `PhysicsEngine.cs:49-52,146-149`. Single production call site (`SessionPlayerComposition.cs:384`), main thread, leaf method — the instance-owned scratch is safe, and it is cleared at entry so no stale state can leak across sessions. **Test discipline — good, with two gaps.** No test re-encodes the constant under test: every radius assertion references the fake window's own input (`WorldRevealReadinessBarrierTests`, `WorldRevealDerivedWindowIntegrationTests`). No new `Skip=`. No test deleted — one was renamed (`OutdoorReveal_JoinsNearRenderTexturesAndTerrain` → `RequiredWindow_IsRereadOnEveryEvaluationWithoutReconstruction`) with its original assertions carried into a new `OutdoorReveal_JoinsRenderTexturesAndTerrainOverTheDerivedWindow`; coverage is preserved and the +36 arithmetic holds. Gaps: - No test exercises a demote. That is D-1. - `WorldRevealDerivedWindowIntegrationTests` advertises itself as end-to-end against "the REAL `StreamingController`, `GpuWorldState`, and `PhysicsEngine`", but constructs `new GpuWorldState()` with no spawn adapter, so `IsRenderReady` degenerates to `IsLoaded` via the `?? true` at `GpuWorldState.cs:181`. The single most load-bearing predicate in the whole change is stubbed out by a null in the test named after it. It also builds its own `revealWindow` lambda rather than the production one in `SessionPlayerComposition.cs:373-379`, so the `ApplyRevealRadiusOverride` wrapper is never exercised in composition. **Sabotage spot-checks — 4 of 9 families reproduced (asked for ≥ 3).** | Sabotage | Named test(s) | Result | |---|---|---| | `PhysicsEngine`: scratch → `new HashSet()` | `WarmedNeighborhoodQuery_AtTheFarRadius_AllocatesNothing` | FAIL, **"allocated 27,712,000 bytes"** — S5's claimed figure reproduces to the byte | | `StreamingController`: revert tier split to `!IsNearTier \|\| !IsRenderReady` | `TieredWindow_OuterRingFarTierMemberIsResident`, `TieredWindow_AbsentOuterRingMemberIsNotResident`, `TieredWindow_MapCornerDestinationConvergesAtAWideRadius`, `OutdoorReveal_HoldsUntilTheWholeDerivedWindowIsPublished` ×2, `LoginReveal_UsesTheSameWidenedGateAsPortalArrival` | 6 FAIL | | `RuntimeWorldTransitState`: shape → `== (isIndoor ? 0 : 1)` | `OutdoorReadinessShape_AcceptsAnyDerivedStreamingRadius` | 6 FAIL (all inline radii) | | `WorldRevealCoordinator`: neuter `ReconcileDestinationReservationRadius` | `MidHoldRadiusChange_ReopensTheReservationOnTheSameGeneration` | 1 FAIL | All four reverted; `git status` clean afterwards. No sign of the C5b-D3 class (a test that passes with its own change reverted) in the families checked. **Suite at HEAD `fafc0b65`, Release, verified by running it:** `11,179 passed / 4 skipped / 0 failed` — exactly the expected 11,178 + 1 Core settler test. Per project: UI 546, Content 124, Runtime 1217, Cli 4, Bake 15, App 4157/3 skips, Headless 89, Core.Net 764, Core 4263/1 skip. None of #302/#308/#321 surfaced in this run and none are conflated with the finding above. **Process rules — clean.** No suppression flag, grace period, retry loop, or symptom guard was introduced. `ACDREAM_PROBE_REVEAL_RADIUS` lives in a diagnostic owner per Code Structure Rule 5 and is correctly argued as a measurement probe rather than a shipped knob. `AD-2` was amended and `AP-149` filed in the same commit, satisfying the divergence-register rule. The `ACDREAM_STREAM_RADIUS` CLAUDE.md correction is accurate against `SessionPlayerComposition.ComposeCore:249-257`. --- ## Recommended disposition Do not ship the gate widening until D-1 is closed. The two shapes worth considering, in preference order: 1. **Make a demote re-register the Far tier.** The demote already leaves a fully valid Far-tier landblock behind; the registration it drops is the *Near* mesh set. `LandblockRetirementStage.MeshReferences` on a `NearLayer` ticket should release the Near references and then re-assert an empty Far registration, exactly as `PublishAsFar` does — i.e. the same `OnLandblockLoaded(landblock, empty)` call the Far publication makes. This makes `IsRenderReady` mean "drawable at its current tier", which is what both the render path and the gate already assume it means. 2. **Re-publish demoted members inside a destination reservation.** Weaker: it fixes the gate without fixing the predicate, and leaves the next caller of `IsRenderReady` holding the same trap. Whichever is chosen, it needs a test at the demote transition — `Near publish → demote → IsRenderNeighborhoodResident(dest, near, far)` — and the `WorldRevealDerivedWindowIntegrationTests` fixture should be given a real `LandblockSpawnAdapter` so its `IsRenderReady` stops being a tautology. R-1 and R-2 are one-line and one-decision respectively and can ride along.