fix(physics): bind a parented child to the parent's live incarnation (#319)

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 at 52175aa1, +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>
This commit is contained in:
Erik 2026-08-05 11:56:31 +02:00
parent af828a8a2a
commit 392c1e22c1
19 changed files with 3076 additions and 20 deletions

View file

@ -24,6 +24,79 @@ What does NOT go here:
- Every session: scan OPEN issues at start; promote/close anything we touched during the session before ending.
- Promoting to a Phase: mark as `DONE (promoted to Phase X)` + commit SHA where the Phase entry landed.
## #320 — The local player's canonical cell does not track ordinary movement (follow-up from #319)
**Status:** OPEN
**Severity:** LOW today (no observed symptom — see below); the correctness
question is real and unresolved
**Filed:** 2026-08-05, filed in the #319 fix commit per that contract's §2.2/§4/§9
**Component:** physics / entity lifetime / local player canonical cell
**Description.** Retail writes the local player's cell on EVERY physics tick
(`CPhysicsObj::SetPositionInternal` @0x00515330, unconditional). acdream's
canonical `FullCellId` for the LOCAL player is written only at three edges:
login activation (`RuntimeSetPositionState.cs:2741-2745`), an accepted inbound
Position/ForcePosition (`RuntimeEntityDirectory.RefreshSnapshot`
`RuntimeEntityRecord.cs:234`), and a teleport/portal placement commit
(`RuntimeSetPositionState.cs:5001-5007`; `LocalPlayerTeleportController.cs:255`).
Ordinary WASD movement passes a LANDBLOCK id, not an exact cell
(`LocalPlayerProjectionController.Project`, low 16 bits forced to `0xFFFF` in
both branches), and `LiveEntityRuntime.cs:935-938` explicitly PRESERVES the
prior canonical cell for that shape. So the player's canonical cell is coarse
and mostly-frozen between teleports — see the register row this issue's fix
commit files (AP-146) for the full citation and the argument that this is
currently safe for every EXISTING consumer.
**Why this is not #319's blast radius.** #319's fix makes a player-parented
equipped child inherit the parent's (the player's) canonical cell EXACTLY —
an equality invariant, not a freshness one. The child is stale-but-equal
wherever the player's own record already is; #319's fix does not touch the
player's own cell-writing paths at all. See
[`docs/research/2026-08-05-issue-319-contract.md`](research/2026-08-05-issue-319-contract.md)
§2.2 for the full argument that folding this into #319 would have put that
slice at route-3 scale (~418 lines) and was the wrong bundling regardless.
**What this issue must resolve before implementation, not after (§2.2's
enumeration + §9 item 2):**
1. Making the player's canonical cell exact-track movement touches the
deliberate landblock-preserve contract at `LiveEntityRuntime.cs:935-938`
a generic rebucket rule, not player-specific, so changing its input
population changes it for the one caller that relies on it.
2. `Rebucketed` entity-delta publication cadence: today the player NEVER
publishes a `Rebucketed` delta during WASD (the preserve path early-outs
at `CommitRebucket`'s `previous == fullCellId`,
`RuntimeEntityObjectLifetime.cs:1965-1972`); an exact-cell commit would
publish per EnvCell crossing — audit every consumer before shipping.
3. The accepted-Position classification inputs for the LOCAL player (route 2
and the 4b-3 `PreMergeCommittedCellId` measurement at
`TryApplyPosition:1801-1814`): a fresh committed cell changes the
pre-merge population on live correction paths AP-136/AP-138 spent four
review rounds pinning.
4. The portal-space freeze interaction
(`LocalPlayerProjectionController.Project:100-103` — the teleport owner
alone projects the destination while the local controller deliberately
retains its frozen source cell): a canonical exact-cell writer must not
race the teleport owner.
5. The `isOrdinaryRoot` family (`LiveEntityRuntime.cs:915-918`, `:3213`,
`:3323`) and the animation-scheduler local-player exclusion
(`LiveEntityAnimationScheduler.cs:183-227`).
6. **First verification step, unresolved by the #319 investigation or its
fix:** whether the local player is a `RuntimePhysicsState` spatial root,
and if so whether a stale-cell landblock retirement (a player WASD-ing
beyond the streaming radius from its last teleport, with no intervening
teleport or inbound Position) can sweep the player into
`ParkCollisionResidents`. The connected routes exercised so far all
teleport between stops, which refreshes the cell and may be masking this.
Establish this BEFORE deciding whether exact-cell tracking is even
optional — if the player can already be swept today, that is a separate,
more urgent bug independent of this issue's scope.
**Do not implement without a fresh retail-conformance argument** — this is a
design call (which of the two cells is the source of truth for a
client-authoritative parent), not a bug fix, per the #319 contract's §2
verdict.
## #318 — C4 route 3 §8 items 8/9/10 residual: no end-to-end composition test, no local-player shadow assertion, no T8 ordering
**Status:** OPEN
@ -13263,11 +13336,60 @@ by `LandingPacket_PlayerGuid_QueueClearedNoShadowPublish_316Preserved` /
## #319 — A player-parented child never receives a canonical cell (ParentInstanceSequence hardcoded 0)
**Status:** OPEN
**Status:** FIX IMPLEMENTED, awaiting the connected acceptance gate (§7 of the
contract) and commit — NOT YET COMMITTED in this worktree. Do not mark DONE
until the gate runs and the change lands.
**Severity:** LOW for the user (no observable symptom — verified, not assumed),
HIGH for process (it defeats route 7's own connected gate; see below)
**Filed:** 2026-08-05
**Component:** physics / entity lifetime / equipped children
**Fix summary (implementation, revised after the dual review — retail PASS,
architecture FAIL/6 MAJORs, both 2026-08-05).** Both CreateObject-carried
producers (`EquippedChildRenderController.OnSpawn` for a raw CreateObject and
`OnCreateParentAccepted` for the same-generation `CreateParentUpdate`
envelope — the contract named only `OnSpawn`; `OnCreateParentAccepted` has the
identical structural defect and was fixed alongside it) now route through
`AcceptLateBoundCreateObjectRelation`: if the parent's live snapshot is known
at accept time (always true in production — see A6 below), stage the relation
with the parent's live `InstanceSequence`; otherwise log a loud refusal with
no state mutation (a deferred-relation queue was tried here and REMOVED after
the review; see A6). `ParentAttachmentState.CanCommitIncarnation` is a pure,
side-effect-free precondition checked BEFORE either half of a parent-attach
commit mutates anything (moved there by architecture review finding A1: the
original shape checked from inside `CommitProjection`, reached only AFTER the
canonical commit had already landed, so a mismatch tore the transaction —
canonically parented, no committed relation, a staged relation blocking
`Resolve` forever). It logs and returns `false` rather than throwing (Route
3's N3 principle: a possibly-transient condition must not be fatal on a host
that must survive long endurance sessions) — wired from both the App producer
and the headless `RuntimeLiveEntitySessionController`, both now checking it
BEFORE their canonical commit. Two structural gates
(`LiveEntityHydrationController.OnLandblockLoaded`,
`LiveEntityPresentationController.RestoreShadow`) now refuse a record with a
committed parent, closing the two call sites the contract's §3.1/§3.2 flagged
as inert-only-because-the-cell-is-zero (§3.2's premise was corrected by
architecture finding A4: the gate is a live behavior change for
CREATURE-parented children, route 7's D1 already re-cells them nonzero — see
the connected gate's Half B watch item). **A6 — the deferred-relation
question, decided:** an initial revision queued a CreateObject-carried
relation whose parent was not yet addressable and adopted the parent's live
incarnation once it arrived. Both independent reviews proved this queue was
structurally unreachable in production for BOTH producers —
`RuntimeEntityObjectLifetime.RegisterEntityCore`'s `EnqueueDeferredCreate`
gate defers the ENTIRE CreateObject (both wire shapes) before either producer
ever runs — while it carried three latent defects of its own (a missing child
POSITION_TS gate, a placeholder-incarnation collision with the generation
filters, unbounded mid-session accumulation), exercised only by a test that
bypassed production routing. Deleted rather than fixed in place: dead code
carrying three defects is a worse trade than a loud refusal for a case the
layer above already guarantees cannot happen. New ledger-convergence tests
(dual-parent-class: child removal, parent removal, full teardown) close the
gap this decision would otherwise have left untested. Full contract:
[`docs/research/2026-08-05-issue-319-contract.md`](research/2026-08-05-issue-319-contract.md).
Follow-up filed as #320 (the local player's canonical cell does not track
ordinary movement — deliberately NOT bundled into this fix). Register:
AP-142 clause (f), AP-132 clarifying sentence, new row AP-146.
**Regressed by:** `cd3129e9` (C4 route 7), which un-masked a pre-existing latent
bug rather than creating it.