acdream/docs/research/2026-08-04-c4-route-7-architecture-review-round2.md

# C4 route 7 — ARCHITECTURE / ADVERSARIAL review, round 2 (DELTA) — 2026-08-04

**Verdict: PASS.**

All three round-1 blocking findings (A1, A2, A3) are properly remediated, and
each remediation is sabotage-sensitive in the direction that matters. Seven new
findings (B1–B7) are recorded below; none is a correctness defect in shipped
behaviour with a demonstrated failure, and each has a one-to-three-line fix.
**B1 and B3's logging clause are recommended before the connected gate**, but
neither blocks.

Scope: delta only. Round 1's accepted conclusions — the `SetFullCell` blast-radius
enumeration, the D2 chokepoint-completeness proof, and the D7 inertness
verification — are not re-litigated. Round-1 report:
[`2026-08-04-c4-route-7-architecture-review.md`](2026-08-04-c4-route-7-architecture-review.md).

Gates re-run:

- `dotnet build` Runtime.Tests + App.Tests — 0 warnings, 0 errors.
- `dotnet test AcDream.Runtime.Tests` — **1,157 passed / 0 failed** (was 1,156;
  +1 = the depth-cap test).
- `dotnet test AcDream.App.Tests --filter EquippedChild` — **37 passed / 0
  failed** (was 36; the round-1 D4 test was replaced by two).

---

## 1. Round-1 blockers — re-verified

### A1 (MAJOR, "the D4 test cannot fail") — **FIXED, and the fix is sound.**

`tests/AcDream.App.Tests/Rendering/EquippedChildProjectionWithdrawalTests.cs:321-393`
(`TickChild_D4_PresentationBucketMovesToTheDestinationLandblock`).

**Is it measuring the draw bucket or a proxy?** The draw bucket.
`fixture.Spatial` is a real `GpuWorldState` (`:1443`, `= new()`), and
`CopyLiveEntitiesNearLandblock` (`src/AcDream.App/Streaming/GpuWorldState.cs:493-523`)
reads `_loadedLiveByLandblock` — the per-landblock `HashSet<WorldEntity>` the
renderer's live-entity publication actually maintains. It is not a mirror field
and not a derived view. It also `destination.Clear()`s at `:500`, so the
four sequential queries in the test do not contaminate each other (without that
clear, the `DoesNotContain` assertions would be false-negatives; with it, they
are real).

**Can it pass with the demotion removed?** No, and I checked both sabotage
directions:

- *Delete the `RebucketEquippedChildPresentation` call at
  `EquippedChildRenderController.cs:411-425`* → the child never leaves
  `oldLandblock`, so the post-tick `Assert.Contains(newLandblock…)` fails. This
  is the failure the round-1 test could not produce.
- *Revert to the legacy `RebucketLiveEntity`* → the bucket moves (so the
  `Contains` passes) but `CommitRebucket` → `RuntimeEntityRecord.SetFullCell`
  bumps `SpatialAuthorityVersion` unconditionally (`RuntimeEntityRecord.cs:246-251`),
  so the final `Assert.Equal(childSpatialVersionBeforeTick, …)` fails.

Both halves of the transfer are therefore pinned in opposite directions. The
test also cannot pass **vacuously** on an empty query result: the pre-tick
`Assert.Contains(oldLandblock…)` and the post-tick `Assert.Contains(newLandblock…)`
are positive assertions on two different landblocks, and a
`!_availability.IsWorldAvailable` early-return (`GpuWorldState.cs:501`) would
fail the first one. The implementer's reported sabotage ("stubbing the demoted
call to claim success failed it — empty collection") is consistent with that
structure.

### A2 (MAJOR, "silent write-nothing outcome") — **FIXED. The fork is the right call; `NotAttached`-as-benign is safe.**

I judged the fork against the alternative I originally suggested, and the
implementer's choice is better. Reasoning, done by enumeration rather than by
accepting the stated rationale:

`NotAttached` is returned only when `!HasCommittedParent(serverGuid)` or when an
initial-create residence is active. The complete set of things that can remove
`_lastAcceptedByChild[child]` is `ParentAttachmentState.RemoveCommittedChild`,
reached from exactly six call sites:

| remover | is it an unwind edge? |
|---|---|
| `CommitProjection` (`:581`) | re-adds on the next line — transient, unobservable |
| `EndChildProjection` (`:753`) | yes — pickup / Position-unparent |
| `EndGeneration(child)` (`:709`) | yes — child replaced |
| `DeleteGeneration(child)` (`:734`) | yes — child deleted |
| `RemoveCommittedParentReferences(parent)` (`:865-873`, from `EndGeneration`/`DeleteGeneration` on the PARENT) | yes — parent deleted/replaced |
| `RemoveObject` / `RemoveChild` (`:660`, `:756`) | zero production callers |

`CommitProjection` is also the **only** writer, and
`PrepareAndTryRealize` always calls it before `TryRealize` installs the
`AttachedChild` (`EquippedChildRenderController.cs:861-891`), with the recovery
branch gated on `Relations.IsCommitted`. So `_attachedByChild` non-empty ⇒
committed, **unless one of the five genuine unwind edges has fired**. There is
no state in which a child is legitimately attached, rendering, and
`HasCommittedParent` is false. `NotAttached` therefore cannot swallow a real
pose-loss — it can only fire in a window where a teardown is already in flight
and owns the child's fate.

The parent-deleted case is worth naming because it is the longest window
(`_pendingOrphanRemovalByChild` defers the withdrawal across frames,
`EquippedChildRenderController.cs:342`/`:584`): during it, D3 has already zeroed
the child's canonical cell and the bucket is frozen at the parent's last
landblock. That is unchanged from pre-route-7 behaviour (the old
`RebucketLiveEntity` would have rebucketed to the same stale
`parent.ParentCellId`), so it is not a regression.

Routing `NoProjection` into `WithdrawForPoseLoss` is also correct for the
`_projections.TryGetCurrent` failure it is named for — see **B6** for the second
condition that shares the label.

### A3 (MEDIUM, "P8 enumerated the wrong interface") — **FIXED.**

`RuntimeEntityDirectory.cs`'s `PropagateFullCellToChildren` doc now carries a
dedicated `<para>` naming `GameRuntimeEventHub` as itself an
`IRuntimeEntityObjectObserver` that fans out to `IRuntimeEventObserver`, and
`RuntimeTraceRecorder.OnEntity` as the real non-stub consumer, and explicitly
retracts "no consumer at all". The decision (publish nothing) is unchanged and
its stated basis is now true. P4's write-up in the same comment also correctly
states the `ProjectionKind is World` reason for the no-spatial-root claim rather
than the round-1 relation-based reason.

### A4, A5, A7, A9, A10 — spot-checked, all correct.

- **A4** — `RuntimeEntityObjectLifetime.CommitAcceptedParentCellless` carries the
  payload-contract doc, and `Attach_ParentCelled_…` now subscribes a
  `RecordingEntityObserver` and asserts the `Withdrawn` delta's
  `Entity.CellId == parent.FullCellId` and `!= 0`
  (`RuntimeEntityChildCellPropagationTests.cs:34-57`). Sabotage-sensitive:
  moving D1's re-cell after the publish makes `withdrawn.Entity.CellId` zero and
  fails two assertions. This is a real pin, not a source-text pin.
- **A5** — `RuntimeInitialCreateContinuationExecutor.cs:2184-2190`: the dormant
  pickup replay now runs `EndChildProjection` before `LeaveWorld`, matching
  `TryApplyPickup`. Both pickup paths are one shape.
- **A7** — the skip is now keyed on the `(FullCellId, CanonicalLandblockId)`
  pair (`RuntimeEntityDirectory.cs:389-395`), with the reason stated. Cycle
  termination is unaffected: A→B→A still writes A, writes B, then finds A's
  **pair** already equal and stops (`PropagationChokepoint_TerminatesAHostileTwoCycle…`
  still green).
- **A9** — `RebucketEquippedChildPresentation` now carries the same
  `MaterializationResidence is AwaitRuntimePlacement && HasActiveInitialCreateResidence`
  gate `RebucketLiveEntity` honours (`LiveEntityRuntime.cs:1140-1145`).
- **A10** — the C3c-R1 F6 summary is back on
  `FirstEntryDriveServesOneRouteAtATimeAndScopesClearToTheOwner`
  (`RuntimeLiveEntitySessionControllerTests.cs:422-430`); no duplicate
  `<summary>` remains.

---

## 2. New findings

### B1 — MEDIUM. The `NotAttached` test's "the draw bucket did NOT move" assertion is non-discriminating.

**Where:** `tests/AcDream.App.Tests/Rendering/EquippedChildProjectionWithdrawalTests.cs:395-450`
(`TickChild_D4_NotAttachedDisposition_SkipsTheBucketMoveWithoutFailingTheTick`),
final assertion at `:445-448`.

Unlike its sibling, this test never moves the parent: both entities are spawned
at the fixture's default `Cell` and no `CommitRebucket` runs. So
`parent.WorldEntity.ParentCellId` still names the **same landblock the child's
bucket is already in** (`0x0101FFFF`). If the `HasCommittedParent` guard were
deleted outright, `RebucketLiveEntityPresentationOnly` would run and rebucket
the child to the landblock it is already in — and
`Assert.Contains(buffer, kv => kv.Value == child.WorldEntity)` on `oldLandblock`
would still pass.

**Concrete failure scenario:** delete the `HasCommittedParent` check at
`LiveEntityRuntime.cs:1137-1138`. The guard A2 exists to establish is gone; the
enum collapses to `Moved`/`NoProjection`; this test stays green.

The test's **load-bearing half is sound** — `LastFullPoseCompositionVisits` +
`AttachedEntityIds` genuinely pin "the tick was not torn down as a pose loss",
which is the fork judgement A2 asked for, and *that* half does fail if
`NotAttached` were routed to `return false`. Only the bucket clause is
decorative.

**Fix direction:** mirror the sibling — register the second landblock, drive
`CommitRebucket(parent, newCell, newLandblock)` before the tick, and assert the
child is **still in `oldLandblock`** afterwards. Then the assertion discriminates.

### B2 — MEDIUM. A6's justification comment contradicts the R6 gap comment two methods away, and the narrowing removed a (weak) recovery path.

**Where:** `src/AcDream.Runtime/Session/RuntimeLiveEntitySessionController.cs:414-433`
(`RetryChildrenWaitingForParent`'s A6 `<para>`) vs `:346-370`
(`ResolveAndCommitChildAttachment`'s R6 KNOWN-GAP `<para>`).

The A6 comment justifies scanning only `_unresolvedByChild` on the grounds that
"this drive never populates `_stagedByChild`/`_recoveryByChild` through any path
those two sweeps exist to catch". That is false: `ParentAttachmentState.Resolve`
sets `_stagedByChild[childGuid]` on accept (`:472-475`), and the R6 gap
documented two methods above is *precisely* a relation left staged-but-uncommitted
because `TryCommitParent`'s POSITION_TS gate was not satisfied during the child's
pending initial-create residence.

**Concrete failure scenario:** a headless `ParentEvent` arrives while the child's
initial residence is pending. `Resolve` stages it; `TryCommitParent` refuses; the
relation sits in `_stagedByChild` forever. Under round 1's
`ChildrenWaitingForParent` (which unioned all three tables) a later spawn naming
the same parent guid would have re-driven it; under `ChildrenUnresolvedForParent`
nothing will. Weak in practice — it needs a *new parent generation* to fire — but
the narrowing strictly reduced recovery, and the comment claims it did not.

The R6 gap itself is disclosed honestly and I accept it as an out-of-scope
residual (headless committed nothing on this path before D5 existed). The
finding is the contradiction, plus the consequence for the **contract §7 headless
gate**: its scenario is "the local player equips via the bot command surface and
crosses a boundary". If the equip lands during the item's own residence, the gate
produces no `cause=headless-attach` line and must be read as a **not-run**, not a
pass — the same rule the contract already states for `cause=propagate`.

**Fix direction:** correct the A6 `<para>` to say what is actually true (this
drive promotes unresolved relations; a staged-but-uncommitted relation is the R6
gap and is deliberately not retried here), and cross-reference the R6 paragraph.
No code change required.

### B3 — MEDIUM. The depth cap's failure mode contradicts AP-142 clause (a), is unrecoverable, and is invisible without the probe flag.

**Where:** `src/AcDream.Runtime/Entities/RuntimeEntityDirectory.cs:349-357`
(the constant) and `:433-441` (the early return).

Three separate points, in descending importance:

1. **The failure mode is the shape the model exists to avoid.** Past depth 64
   the tail keeps a **stale non-zero** `FullCellId`. Under acdream's single-field
   model that reads as "still resident at the old cell" at all 45+ predicate
   sites — exactly the `#184` invisible-but-solid shape that AP-142 **clause (a)**
   cites as the reason the removal path propagates zero rather than reproducing
   retail's stale-`objcell_id` residue. The cap therefore reintroduces, as its
   *chosen* failure mode, the residue the row rejects. Zeroing the truncated
   subtree instead ("we cannot maintain this, so it is not resident") is both
   consistent with the model and strictly safer.
2. **It is unrecoverable.** Every subsequent crossing truncates at the same
   depth, so a subtree that falls past the cap once never re-syncs. And because
   the skip is value-idempotent, a later same-value write prunes even earlier
   (the asymmetry AP-142 clause (b)'s R10 correction already records). A
   permanently-stale resident subtree is worse than a transiently-stale one.
3. **It is silent in production.** `[child-cell-depth-exceeded]` is emitted only
   under `PhysicsDiagnostics.ProbeChildCellEnabled`, i.e. only when someone has
   already set `ACDREAM_PROBE_CHILD_CELL=1`. A defensive guard whose premise is
   "this must never happen" should be unconditionally observable — a one-shot
   log or a counter on the ownership ledger — or nobody will ever learn it fired.

**Is 64 defensible?** Yes, as a number. Real chains are 2–3 deep
(player → weapon; quiver → arrow). 64 is ~20× any plausible depth and is well
inside a 1 MB stack for a two-frame-per-level recursion. **Is the cap the right
mechanism?** An iterative worklist over a pre-sized scratch would remove the
hazard entirely, keep 0 B after warmup, and need no cap, no divergence row, and
no failure mode — that is the answer I would take if this is revisited. But the
cap is a strict improvement over round 1's uncatchable process kill, so it is not
a blocker.

**Fix direction (cheapest first):** (i) make the past-cap log unconditional;
(ii) zero the truncated subtree rather than leaving it stale, and amend AP-142
clause (e) to say so; (iii) if revisited, replace the recursion with an
iterative worklist and retire clause (e).

### B4 — LOW. The reused scratch buffer is not re-entrancy-safe; round 1's fresh array was.

**Where:** `src/AcDream.Runtime/Session/RuntimeLiveEntitySessionController.cs:73`
(`_unresolvedChildrenScratch`), `:434-440` (filled, then iterated while calling
`ResolveAndCommitChildAttachment`).

`ChildrenUnresolvedForParent` **clears and refills the caller's buffer**
(`ParentAttachmentState.cs:657-660`). `RetryChildrenWaitingForParent` then
iterates that same instance while calling into
`ResolveAndCommitChildAttachment` → `TryCommitParent` /
`CommitAcceptedParentCellless` → `AcknowledgeProjectionAndPublish` → synchronous
observer fan-out through `GameRuntimeEventHub`.

**Concrete failure scenario:** an `IRuntimeEventObserver` synchronously feeds a
spawn back into the sink → nested `OnSpawned` → nested
`RetryChildrenWaitingForParent` → the shared buffer is cleared and refilled →
the outer loop's `waiting.Count` and indices now address the inner call's
contents, so children are skipped or retried twice. Requires an observer that
re-enters the sink; the shipped headless policies are empty stubs, so this is
latent, not live. The graphical sibling it mirrors
(`ChildrenWaitingForParent` → `.ToArray()`) is re-entrancy-safe by allocation,
and round 1's version inherited that safety; the A6 optimization traded it away
without a guard.

**Fix direction:** a `_retryInProgress` flag that falls back to a fresh `List`
when re-entered, or state the single-entry precondition in the doc and assert it.

### B5 — LOW. The recursion no longer routes children through the public `SetFullCell`, creating a silent divergence trap.

**Where:** `src/AcDream.Runtime/Entities/RuntimeEntityDirectory.cs:409-414` —
propagation now calls `child.SetFullCell(...)` (the **record** method) plus a
direct recursive call, where round 1 re-entered `RuntimeEntityDirectory.SetFullCell`.

The two are equivalent **today** — the public method is exactly
`EnsureKnown` + `record.SetFullCell` + propagate, and `EnsureKnown` is a
validation-only throw (`:812-820`) already subsumed by the preceding
`TryGetActive`. The change was needed to thread `depth`.

**Concrete failure scenario:** anyone later adding a side effect to
`RuntimeEntityDirectory.SetFullCell` — an index update, a delta publish, a
telemetry counter — gets it for the root and **silently not** for propagated
children, reintroducing exactly the "mapping written against one caller's
reachable set" class D2 was designed to eliminate. Nothing in the code says the
equivalence is load-bearing.

**Fix direction:** give the public method a private `depth`-carrying overload and
have the recursion call *that*, so there is one body; or add one comment line at
`:409` naming the equivalence as an invariant to maintain.

### B6 — LOW. `NoProjection` conflates two different outcomes, and one of them now triggers a teardown that round 1 did not.

**Where:** `src/AcDream.App/World/LiveEntityRuntime.cs:1131-1152`. The enum doc
(`:45-56`) defines `NoProjection` as "the graphical projection itself is gone",
but `:1147-1152` also returns it when `RebucketLiveEntityPresentationOnly`
returns `false` — which means the **projection operation was displaced by a
re-entrant callback** (`IsCurrentProjectionOperation` at `:1029`/`:1054`), i.e. a
*newer* rebucket took over. `TickChild` then returns `false` →
`Tick()`'s `failed` list → `WithdrawForPoseLoss`.

**Concrete failure scenario:** a re-entrant observer starts a newer rebucket for
the same child mid-`_spatial.RebucketLiveEntity`; the older tick concludes
"projection gone" and withdraws the child's projection that the newer operation
just legitimately re-established. Low reachability — the
`_presentationOnlySpatialMutationDepth` guard (`:3354-3360`) already suppresses
the most likely re-entrant rebucket source — but round 1 discarded the bool, so
this teardown is new behaviour introduced by the A2 fix.

**Fix direction:** either split the displaced case into its own disposition
(`Superseded`, treated as benign — the newer operation owns the bucket), or
document at `:1147` that displacement is deliberately treated as pose loss and
why.

### B7 — INFO. One other test derives a built-collection size from a live production constant.

Asked directly, so answered directly: I swept `tests/` for the shape that caused
the implementer's 64,000-node stack overflow (a test that both *reads* a
production constant and *sizes work* by it). Exactly one other match with real
blast radius:

`tests/AcDream.Core.Tests/World/LandblockLoaderTests.cs:210` —
`for (uint i = 0; i <= LandblockStaticEntityIdAllocator.MaxCounter + 1u; i++)`
builds one `Stab` per iteration. `MaxCounter = 0xFFF`
(`src/AcDream.Core/World/LandblockStaticEntityIdAllocator.cs:16`), so 4,097 cheap
allocations today. A sabotage raising it by orders of magnitude would OOM or hang
rather than stack-overflow, and the test's `Assert.Contains("4096-entry", …)`
literal pins the constant — but only *after* the build loop, so the pin does not
protect the loop. Milder than the route-7 case and pre-existing; noted, not
filed against this slice.

The other three matches are benign: `LightManagerTests` (`MaxGlobalLights = 128`,
+50 iterations of trivial work) and the two `CapacityTrimIdleFrames` frame loops.

The route-7 depth test's own remaining constant read
(`RuntimeEntityChildCellPropagationTests.cs:473`,
`for (i = 0; i <= MaxPropagationDepth; i++)`) is **correctly protected**: the
literal pin `Assert.Equal(64, RuntimeEntityDirectory.MaxPropagationDepth)` at
`:441` fires first, and the chain length at `:442` is a literal. The boundary is
pinned in both directions — a cap of 70 would give index 69 the new cell and fail
the tail assertion; a cap of 60 would leave indices 61–64 stale and fail the loop.

---

## 3. Direct answers to the coordinator's remaining questions

### The `WarmedSteadyContactRefreshDoesNotAllocate` flake — **#302 class. Not this slice.**

Stated definitely rather than hedged, because it is provable by reachability:

- The measured window
  (`RuntimeCollisionReportingStateTests.cs:2375-2379`) contains **only**
  `lifetime.Physics.HandleSetPositionCollisions(...)`. Entity registration and
  shadow setup happen before `GC.GetAllocatedBytesForCurrentThread()` is sampled.
- `src/AcDream.Runtime/Physics/RuntimeCollisionReportingState.cs` contains **zero**
  occurrences of `SetFullCell` and **zero** of `ParentAttachments` (grepped). Its
  only `_entities` interactions are read-only queries (`TryGetByLocalId`,
  `IsCurrent`, `SessionLifetimeVersion`, `CurrentLifetimeMutation`) plus
  `StopMissileAfterCollision`, which mutates `FinalPhysicsState`, not the cell.
  `StopMissileAfterCollision` is not in the complete `SetFullCell` caller set
  enumerated in round 1 §0 and re-verified against this diff.

So `PropagateFullCellToChildren` is not reachable from the measured call at all,
and the fixture has no committed parent relations in any case. A
`GC.GetAllocatedBytesForCurrentThread() == 0` assertion that passes in isolation
and on re-run but fails once under full-suite load is the documented **#302**
shape (tiered-JIT / first-touch allocation on the measuring thread), and should
be treated as such: re-run and name it, never chase it. The instinct to look
rather than auto-dismiss was right — the answer is that the new code is not on
that path.

### Point 6 — deliberately-not-done items: **my "not blocking alone" assessment holds.**

- **Writer-family coverage for `CommitCanonicalCell` / `RuntimeSetPositionState`.**
  Still covered *by construction*, and round 2 did not weaken that: both funnel
  through the identical `RuntimeEntityDirectory.SetFullCell:359-366` line that
  `CommitRebucket` (tested) and `RefreshSnapshot` (tested) use, and round 1's
  chokepoint-completeness proof (`RefreshDerivedState` has exactly two callers;
  `RuntimeEntityRecord.SetFullCell` exactly two) is unchanged by this diff. Two
  of the four writer families are tested end-to-end; the other two are the same
  three lines of code.
- **Fuller P2 across N crossings** — `NeverArmPartition_D8_…` already drives five
  crossings; the clock/suspension assertions live in the attach and pickup tests.
  Thin, not absent.
- **Dedicated P5/P6 tests** — still argued by construction, and the construction
  argument is still valid after round 2: `PropagateFullCellToChildren` remains
  field-writes + one dictionary probe + `TryGetActive`, with no callback, no
  LINQ, no closure, and `ChildrenAttachedToParent` still returning the stored
  `List<uint>` or the cached `Array.Empty<uint>()`. The round-2 restructuring
  (direct `child.SetFullCell` instead of re-entering the directory method) makes
  the path *cheaper*, not richer. **B4** is the one place round 2 introduced new
  re-entrancy surface, and it is in D5's retry, not the propagation.

---

## 4. Recommended before the connected gate (non-blocking)

1. **B1** — make the `NotAttached` test's bucket assertion discriminate (move the
   parent first). Two lines.
2. **B3 clause 3** — make the past-cap log unconditional. One line.
3. **B2** — correct the A6 justification comment so it does not contradict the R6
   gap paragraph, and carry the "no `cause=headless-attach` line ⇒ not-run"
   reading into the headless gate step. Comment only.

The **connected two-client gate remains unrun** and is still the acceptance test
for the D4 transfer, with the contract's rule intact: a session showing zero
`cause=propagate` lines during the landblock-crossing step is a not-run, not a
pass. The headless gate now carries the same caveat for `cause=headless-attach`
(B2).