392c1e22c1
A player-parented child never received a canonical cell. Its FullCellId stayed 0 for its whole attached lifetime, so it could not follow the player across a boundary. Scope was wider than the local player: every REMOTE player's equipment too. ROOT CAUSE. EquippedChildRenderController hardcoded ParentInstanceSequence: 0 for a parented CreateObject. Correct for creatures and statics, which really are sequence 0; wrong for players, whose ObjectInstance is Character.TotalLogins (ACE Player_Networking.cs:37). The relation filed under (playerGuid, 0) while the record carried TotalLogins, so both route-7 write sites — D1's attach re-cell and D2's propagation lookup — keyed on an incarnation that never matched. TryCommitParent did not validate the sequence, so the attach succeeded and printed normally. Silent. A ROUTE 7 REGRESSION (cd3129e9) that un-masked a latent bug: the TickChild call route 7 deleted was keyed on the child guid alone and was structurally immune to a wrong parent key. THE FIX IS TO STOP TREATING PLAYERS DIFFERENTLY, not to special-case them. Retail's attach path is guid-only end to end — PhysicsDesc::get_parent_id @0x00558a18 -> CObjectMaint::GetObjectA @0x00558a2d -> set_parent @0x00558a3e, with SetChildren @0x00509370 hash-walking by guid — and neither set_parent overload (@0x00515A90, @0x00515B50) nor enter_cell @0x00510ED0 contains any player test or instance-sequence read. Our player/non-player split was purely an artifact of keying relations by (guid, incarnation) against a wire message that carries no parent incarnation. Late-binding to whoever currently holds the guid is retail's own semantics. Fixed at BOTH producers: OnSpawn and OnCreateParentAccepted, the second carrying the byte-identical defect and not named in the contract's scope line. THE INVARIANT IS EQUALITY, NOT FRESHNESS. The contract rejected both framings I offered: every one of the 45 FullCellId liveness predicates excludes a committed child on a NON-cell clause first, so the child inherits only the parent record's existing staleness, which is already present today with no symptom. The key fix alone restores child-equals-parent for every parent class. TWO SITES GATED, inert only because the cell was zero and would have woken wrongly: the hydration candidate loop (a nonzero-cell child would take the legacy RebucketLiveEntity -> CommitRebucket, a second canonical writer — route 7's exact defect class) and RestoreShadow (would install a broadphase row for the weapon, the #184 shape, contradicting route 7's P4). Retail anchor: update_object's parent != 0 early-out @0x00515D40 — children are never independently re-placed. THREE MAJORS WERE FIXED BY DELETION. The first pass added a deferral queue for an unaddressable parent, carrying a missing child-freshness gate (A2), a sentinel-0 collision with the generation filters (A3), and unbounded accumulation (A5). Both reviewers then proved the deferred branch unreachable for BOTH producers — RegisterEntityCore defers the entire CreateObject one layer above, reading the same ?? chain, and CreateParentUpdate is produced only inside AcceptCreateCore, after that gate passes. The machinery was deleted rather than repaired, and the diff SHRANK to 76 added / 13 removed from 91/24 while gaining the A1 fix. Retail confirmed the deletion does not diverge: acdream's real port of retail's per-guid replay (QueueBlobForObject) is a different, untouched layer, and the deleted queue was a third redundant one downstream of it. THE GUARD MUST NOT TEAR WHAT IT PROTECTS. The first pass threw InvalidOperationException AFTER the canonical half had committed, so the one time it fired it left the child parented with no committed relation and a staged one blocking Resolve — a torn transaction, the exact outcome the contract pinned against. Now a pure CanCommitIncarnation precondition checked BEFORE the commit at both sites, with a logged refusal instead of a throw. Route 3's N3 principle (do not make a transient fatal on a host that must survive 30 sessions x 2 hours) reinforces it, but the tearing argument stands alone. TEST QUALITY, the recurring lesson in its most refined form. The A1 test initially passed sabotage FOR THE WRONG REASON: a mismatched ChildPositionSequence meant TryCommitParent's own gate refused in either ordering, so the three assertions carrying A1's meaning passed both ways and only an incidental staging assertion failed. It failed on stranding, not tearing. Corrected, the sabotage now names line 925 — Assert.Null(snapshot.ParentGuid), with the parent's guid in it — proving the canonical mutation happened before the catch. "Fails under sabotage" is necessary, not sufficient; WHICH assertion fails is the real question. The dual parent-class matrix (player 0x5… incarnation > 1 vs creature 0x8… incarnation 0, identical outcomes, sabotage-verified in both directions) is the structural fix for how this survived a full dual review and two connected sessions: every prior test and both captured gate logs used sequence-0 parents. Register: AP-142 clause (f); AP-132 amended to distinguish the two producers; new row AP-146 for the local player's coarse canonical cell (retail writes it per tick at SetPositionInternal @0x00515330 — which, per the retail review, ALSO walks this->children writing each child's objcell_id @0x005153AE-@0x005153D8, so retail's per-tick child propagation lives in the same function). That divergence had no row at all, a standing rule-1 violation now corrected. Follow-up #320 filed for making the player's cell track ordinary movement — deliberately excluded here: it touches the landblock-preserve contract, the Rebucketed cadence, route-2/4b-3 classification inputs AP-136/AP-138 spent four review rounds pinning, and the portal-space frozen-source-cell race. Two dual review rounds; 6 architecture MAJORs and 2 retail MAJORs closed. Diagnostic refusals are latched per child guid and the latch clears on Clear()/RemoveChild, so a recycled guid's next incarnation still logs rather than being silently suppressed. Complete Release suite MEASURED at 11,112 passed / 4 skipped / 0 failed (baseline 11,090 at52175aa1, +22). Neither known flake fired. STILL OWED: the connected gate, with the CORRECTED positive criterion — assert the equipped child's FullCellId EQUALS the parent's after a crossing (a zero is a failure, not a silence), run with BOTH a player and a creature parent, plus the new step carrying an armed creature across a landblock unload/reload. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
287 lines
15 KiB
Markdown
287 lines
15 KiB
Markdown
# Issue #319 — architecture / adversarial review, ROUND 2 (delta) — 2026-08-05
|
||
|
||
**Verdict: FAIL — one blocking finding, a one-line test change.**
|
||
|
||
Round 1: [`2026-08-05-issue-319-architecture-review.md`](2026-08-05-issue-319-architecture-review.md).
|
||
|
||
**Every production concern from round 1 is closed, and I verified each by
|
||
reading the code rather than accepting the summary.** The A1 ordering fix is
|
||
correct on both hosts, the A6 deletion argument is provable and all three of
|
||
its links check out, and A2/A3/A5 are gone by deletion rather than relocated.
|
||
The single blocker is evidentiary: the new test that exists specifically to
|
||
guard the A1 ordering property has three headline assertions that are
|
||
**vacuous in its own fixture**, so the guard against the exact regression it
|
||
was written for is one incidental assertion. That is fixable by changing one
|
||
constant.
|
||
|
||
Verification performed for this round:
|
||
|
||
- `dotnet build -c Release` — succeeded, 0 warnings, 0 errors.
|
||
- `AcDream.Runtime.Tests` (`ParentAttachmentStateTests`,
|
||
`RuntimeEntityChildCellPropagationTests`) — **38/38** (was 32).
|
||
- `AcDream.App.Tests` (`EquippedChildProjectionWithdrawalTests`,
|
||
`LiveEntityHydrationControllerTests`, `LiveEntityPresentationControllerTests`,
|
||
`LiveEntityRuntimeTests`) — **208/208** (was 206).
|
||
- Production delta measured independently: **76 non-comment lines added, 13
|
||
removed** — matches the stated 76/13, down from 91/24.
|
||
|
||
---
|
||
|
||
## B1 — MAJOR, BLOCKING — `PrepareAndTryRealize_MismatchedIncarnation_RefusesBeforeCanonicalCommit`'s three canonical assertions cannot fail, in either ordering
|
||
|
||
**`tests/AcDream.App.Tests/Rendering/EquippedChildProjectionWithdrawalTests.cs`**,
|
||
the new A1 test.
|
||
|
||
The test injects `wrongRelation` with `ChildPositionSequence: 1` against a
|
||
child created by `fixture.RegisterOnly(childGuid, generation: 1, hasPosition: true)`.
|
||
That helper builds the spawn from `ControllerFixture.SpawnData`, which sets
|
||
`Timestamps.Position: 0` and never sets the top-level `PositionSequence`
|
||
(`WorldSession.EntitySpawn`'s `ushort PositionSequence = 0` default,
|
||
`WorldSession.cs:163`). Unlike its sibling
|
||
`NoPositionCreateParent_CommitsAfterParentPartArrayValidation`, this test does
|
||
**not** call `TryApplyCreateParent`/`TryApplyParent` first, so nothing ever
|
||
advances the child's POSITION_TS.
|
||
|
||
The canonical half is
|
||
`CommitStagedParent` → `RuntimeEntityObjectLifetime.TryCommitParent:1406-1412`
|
||
→ `InboundPhysicsStateController.TryCommitParent:299-312`, whose gate is:
|
||
|
||
```csharp
|
||
if (!TryGet(childGuid, out gate, out child)
|
||
|| gate.PositionTimestamp != positionSequence // 0 != 1
|
||
|| child.PositionSequence != positionSequence) // 0 != 1
|
||
{ accepted = default; return false; }
|
||
```
|
||
|
||
`positionSequence` is `relation.ChildPositionSequence` = 1; both gate values
|
||
are 0. **The canonical commit refuses on POSITION_TS before the incarnation is
|
||
ever consulted — in either ordering.**
|
||
|
||
**Consequence.** Revert the fix (put `CanCommitIncarnation` back after
|
||
`CommitStagedParent`, or delete it entirely) and trace the test:
|
||
|
||
| assertion | reverted-order outcome |
|
||
|---|---|
|
||
| `Assert.Null(snapshot.ParentGuid)` | **still passes** — `TryCommitParent` refused, snapshot untouched |
|
||
| `Assert.NotNull(snapshot.Position)` | **still passes** — same reason |
|
||
| `Assert.False(TryGetCommittedParent(...))` | **still passes** — `CommitProjection` never reached |
|
||
| `Assert.False(TryGetStagedProjection(...))` | fails — `return default` leaves the relation staged |
|
||
|
||
So the test does fail under sabotage — the letter of "every new test must fail
|
||
against broken behaviour" is met — but it fails on the *stranding* assertion,
|
||
not the *tearing* one. The claim relayed to me ("asserts the child's snapshot
|
||
stays un-parented; sabotage-verified by reverting the order") is literally true
|
||
and materially misleading: the snapshot assertions never execute against a live
|
||
canonical commit, so they assert nothing.
|
||
|
||
**Why this blocks rather than being cosmetic.** The regression this guard
|
||
exists to catch is "someone lets the canonical half run before the incarnation
|
||
check." A plausible future variant — moving the check back inside
|
||
`CommitProjection` while *also* calling `RejectProjection` on refusal — would
|
||
re-tear the transaction and **pass this test on all four assertions**. The
|
||
guard does not cover its own finding.
|
||
|
||
**Fix (one line).** Change `wrongRelation`'s `ChildPositionSequence: 1` to `0`
|
||
so it matches the fixture's gate. Then under a reverted ordering
|
||
`TryCommitParent` succeeds, nulls `snapshot.Position`, writes `ParentGuid`, and
|
||
all three canonical assertions bite — while the fixed code still refuses at
|
||
`CanCommitIncarnation` (which reads only the parent incarnation and is
|
||
independent of POSITION_TS) and `ValidateParentProjection` still returns
|
||
`Ready`. Re-run the sabotage and confirm the failure message names one of the
|
||
three snapshot assertions, not the staged-projection one.
|
||
|
||
---
|
||
|
||
## What I verified as CLOSED
|
||
|
||
### A1 — ordering fix: torn-transaction window genuinely closed on both hosts ✓
|
||
|
||
- `ParentAttachmentState.CanCommitIncarnation:589-605` is genuinely pure: it
|
||
reads `resolveParentInstance` and writes only stderr. No table is touched.
|
||
- **Graphical** (`EquippedChildRenderController.cs:974-989`): the pre-check runs
|
||
before `CommitStagedParent`, refuses via `RejectProjection`, and returns
|
||
`CanAdvanceWireQueue: true`. Correct on three counts I checked separately:
|
||
(i) no canonical mutation precedes it; (ii) `RejectProjection` clears
|
||
`_stagedByChild`, so `Resolve`'s early return (`:457-458`) is unblocked —
|
||
the round-1 "stranded forever" outcome is gone; (iii) `CanAdvanceWireQueue: true`
|
||
lets `ResolveAndTryRealize`'s `while (true)` loop continue, and it still
|
||
terminates, because each iteration either breaks or consumes one relation from
|
||
the finite `_unresolvedByChild` queue, and `Resolve` can only re-stage on
|
||
exact incarnation equality — which is precisely the condition
|
||
`CanCommitIncarnation` accepts.
|
||
- **Headless** (`RuntimeLiveEntitySessionController.cs:408-417`): same
|
||
pre-check, before `TryCommitParent`, with `RejectProjection` + `return false`.
|
||
- `CommitProjection` now returns `false` instead of throwing, and its internal
|
||
check (`:628-629`) sits after the `_stagedByChild` lookup but before
|
||
`RemoveCommittedChild`/`_lastAcceptedByChild`, so it mutates nothing before
|
||
refusing.
|
||
- The residual window I looked for does not exist: between the pre-check and
|
||
`CommitProjection`, the only mutation is `TryCommitParent` writing the
|
||
**child's** snapshot; the parent's `_snapshots` entry — the tripwire's input —
|
||
is untouched, so the two reads cannot disagree.
|
||
|
||
Route 3's N3 principle is now honoured: the condition is refused loudly and
|
||
recoverably, never fatally.
|
||
|
||
### A6 — the deletion argument: all three links independently verified ✓
|
||
|
||
This was the justification for deleting rather than fixing, so I checked it
|
||
rather than accepting it.
|
||
|
||
1. **`RegisterEntityCore` defers the whole CreateObject.**
|
||
`RuntimeEntityObjectLifetime.cs:795-812`:
|
||
`uint parentGuid = incoming.ParentGuid ?? incoming.Physics?.Parent?.Guid ?? 0u;`
|
||
then `if (beginInitialResidence && parentGuid != 0u && !Entities.TryGetActive(parentGuid, out _))`
|
||
→ `EnqueueDeferredCreate` + `DeferredForParent: true`. Confirmed this is the
|
||
**earliest** branch in the method that can admit a create — it precedes
|
||
`PreviewCreateDisposition` (`:818`), the pending-residence early return, and
|
||
`AcceptCreate` (`:866-868`).
|
||
2. **`beginInitialResidence` is true for every graphical-host CreateObject.**
|
||
`LiveEntityRuntime.cs:583` is the sole App registration entry and uses
|
||
`RegisterEntityWithInitialResidence`. The three internal replay/drain sites
|
||
(`RuntimeEntityObjectLifetime.cs:234`, `:317`, `:400`) also use it, so the
|
||
gate re-applies on deferred-create replay rather than being bypassed by it.
|
||
3. **`CreateParentUpdate` cannot precede the gate.** It exists only inside
|
||
`InboundCreateResult.SameGenerationEvents`, produced by
|
||
`BuildSameGenerationEvents` in `InboundPhysicsStateController.AcceptCreate`
|
||
(`:87`) / `AcceptCreateDeferredSameGeneration`, both called at
|
||
`RuntimeEntityObjectLifetime.cs:866-868` — after the gate. I traced every
|
||
consumer and all three originate there:
|
||
`LiveEntityHydrationController.cs:340` (`result.SameGenerationEvents`),
|
||
`LiveEntityNetworkUpdateController.cs:400-401` (via
|
||
`ApplySameGeneration`/`LiveEntitySameGenerationUpdateRouter`), and
|
||
`RuntimeEntityObjectLifetime.cs:2544` (`AdmitSameGenerationCreate`, also
|
||
post-gate).
|
||
|
||
**The chain holds.** `AcceptLateBoundCreateObjectRelation`'s `else` branch is
|
||
structurally unreachable for both producers, and the deletion removes no real
|
||
path.
|
||
|
||
### A2 / A3 / A5 — gone by deletion, not relocated ✓
|
||
|
||
Repo-wide grep for `DeferCreateObjectRelation`, `LateBindParentInstance`, and
|
||
`lateBind` returns **zero** hits in `src/` and `tests/` (only unrelated
|
||
`InteractionUiLateBindings` matches). `ParentAttachmentState.Resolve` is
|
||
byte-identical to HEAD — the diff touches only `CanCommitIncarnation` and
|
||
`CommitProjection`, and `ParentAttachmentRelation` gains no field. So:
|
||
|
||
- **A2** (missing child POSITION_TS gate on a relation that skipped `accept`) —
|
||
no relation skips `accept` any more; `Resolve`'s single path is the original
|
||
ParentEvent one.
|
||
- **A3** (placeholder `ParentInstanceSequence = 0` misread by
|
||
`FilterParentCandidates`) — no relation is enqueued with a placeholder; the
|
||
only unresolved-queue writer is the pre-existing `Enqueue`.
|
||
- **A5** (unbounded accumulation of deferred CreateObject relations) — that
|
||
population no longer exists. The surviving `_unresolvedByChild` accumulation
|
||
question is pre-existing ParentEvent behaviour, untouched by this fix.
|
||
|
||
The replacement — a loud stderr line with **no state mutation** — is the right
|
||
trade: if the upstream invariant ever breaks, the failure mode is a
|
||
non-attached (cell-less, position-less) child plus a log line, not a resurrected
|
||
cross-generation relation.
|
||
|
||
### Test 9 — covers the shapes I named ✓
|
||
|
||
Three dual-class tests in `ParentAttachmentStateTests`. Checked against the A2/A5
|
||
shapes specifically:
|
||
|
||
- `LedgerConvergence_ChildRemoval_ZeroesEveryTable` asserts all four counters
|
||
(`CommittedRelationCount`, `RecoveryRelationCount`, `StagedRelationCount`,
|
||
`UnresolvedRelationCount`) **plus** the `_committedChildrenByParent` reverse
|
||
index via `ChildrenAttachedToParent` — that reverse index is the table
|
||
round 1's stranding scenarios would have leaked into, and asserting it
|
||
separately from `HasCommittedParent` is the right call.
|
||
- `LedgerConvergence_ParentRemoval_ZeroesEveryTable` covers `RemoveObject`'s
|
||
parent-reference sweep — the A2 "relation outlives its parent" half.
|
||
- `LedgerConvergence_Teardown_ZeroesEveryTableWithMixedPendingState` holds a
|
||
committed child and an unresolved ParentEvent simultaneously before `Clear()`
|
||
— the A5 mixed-state shape.
|
||
|
||
Each fails against broken behaviour non-vacuously (they establish nonzero
|
||
counts before the removal and assert zero after, so a no-op removal fails).
|
||
|
||
### A4 — sufficient ✓
|
||
|
||
The `RestoreShadow` comment now states the correction plainly (route 7's D1
|
||
already gives creature-parented children a nonzero cell, so the clause **is** a
|
||
live behaviour change for that class), and contract §7 Half B now carries the
|
||
explicit watch instruction (`:549`, `:554`). Given the population is bounded by
|
||
`ShadowObjectRegistry.UpdatePosition`'s unregistered-entity no-op
|
||
(`ShadowObjectRegistry.cs:696-697`), naming it in the connected gate is the
|
||
right resolution — a synthetic test cannot settle it, and the gate is where it
|
||
gets observed.
|
||
|
||
### A8 — confirmed non-blocking ✓
|
||
|
||
`AcceptCreateObjectRelation` remains public with an optional resolver. With A1's
|
||
pre-check now at both production sites, a future producer regression is caught
|
||
before any mutation, so the cost of tightening the API is not worth the ~12
|
||
test call sites.
|
||
|
||
---
|
||
|
||
## Non-blocking observations
|
||
|
||
**B2 — LOW — the gate and the producer test different oracles.** The upstream
|
||
gate (`RuntimeEntityObjectLifetime.cs:797`) uses `Entities.TryGetActive`
|
||
(active-record table); `AcceptLateBoundCreateObjectRelation` uses
|
||
`_liveEntities.TryGetSnapshot` (the `InboundPhysicsStateController._snapshots`
|
||
map). Active ⇒ snapshot holds because `AddActive` is fed from the snapshot
|
||
`AcceptCreate` wrote, but there is a narrow inversion inside `TryDeleteEntity`:
|
||
`TryDelete` removes `_snapshots[guid]` (`InboundPhysicsStateController.cs:101-102`)
|
||
several statements before `RemoveActive` (`RuntimeEntityObjectLifetime.cs:~2094`).
|
||
Reaching the `else` branch through it would need a re-entrant child CreateObject
|
||
inside that window — not reachable from a single-threaded pump. Worth one
|
||
sentence in the remark, since the remark's argument is phrased in the gate's
|
||
predicate but enforced with a different one.
|
||
|
||
**B3 — LOW — the structural invariant is host-scoped and the remark does not
|
||
say so.** `RuntimeLiveEntitySessionController.cs:144` calls
|
||
`Entities.RegisterEntity` (`beginInitialResidence: false`) when
|
||
`_worldProjection is null`, which bypasses the parent-deferral gate entirely.
|
||
That host has no `EquippedChildRenderController`, so the producer is
|
||
unreachable there — but the remark reads as an unconditional claim. One clause
|
||
("in the graphical host; the content-less direct host has no CreateObject-carried
|
||
relation producer at all") would make it audit-proof.
|
||
|
||
**B4 — INFO — the refusal is unconditionally destructive.** `RejectProjection`
|
||
discards the relation, where `Resolve`'s equivalent staleness rule
|
||
(`ParentAttachmentState.cs:484-498`) is conditional: a relation naming a
|
||
*newer*-than-live parent generation is retained until that generation arrives.
|
||
Unreachable today, because a staged relation can only have been staged on exact
|
||
equality — but if the staging rules ever loosen, discard is the wrong arm for
|
||
the packet-ahead case.
|
||
|
||
**B5 — INFO — `CommitProjection`'s XML doc slightly overclaims.** "non-tearing
|
||
on its own terms for ANY caller" is true of *this method's* tables, not of a
|
||
caller's canonical state: a caller that commits canonically first and then calls
|
||
`CommitProjection` still tears when the internal check refuses. That is exactly
|
||
the shape A1 removed, and both production sites now pre-check, so the guard is
|
||
genuine belt-and-braces — the sentence just shouldn't imply it protects callers
|
||
who get the ordering wrong.
|
||
|
||
**B6 — LOW — unbounded stderr on a per-packet path.** Both new
|
||
`Console.Error.WriteLine` sites (`AcceptLateBoundCreateObjectRelation`'s `else`,
|
||
`CanCommitIncarnation`'s refusal) log unconditionally. If the invariant ever
|
||
breaks for a repeating producer, this spams once per packet. A log-once-per-guid
|
||
latch would suit the "should be structurally unreachable — investigate if seen"
|
||
framing better.
|
||
|
||
**B7 — INFO — test 9's scope.** The three tests exercise `RemoveChild`,
|
||
`RemoveObject`, and `Clear` — not `DeleteGeneration`/`EndGeneration`, the two
|
||
paths carrying the `WaitOwner is Parent` retention rule
|
||
(`ParentAttachmentState.cs:819-821`, `:844-846`). That rule is pre-existing and
|
||
unchanged by this fix, so it is legitimately out of scope; the ledger claim
|
||
simply should not be read as covering generation boundaries.
|
||
|
||
---
|
||
|
||
## What a PASS needs
|
||
|
||
**One change:** `ChildPositionSequence: 1` → `0` in
|
||
`PrepareAndTryRealize_MismatchedIncarnation_RefusesBeforeCanonicalCommit`'s
|
||
`wrongRelation`, then re-run the ordering sabotage and confirm the failure names
|
||
a snapshot assertion rather than the staged-projection one.
|
||
|
||
Everything else in this round is PASS-quality. B2/B3 are one-clause comment
|
||
improvements; B4–B7 are informational.
|