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.

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).