fix(review): close the C5b architecture-review findings (D2/D3/D4, L1-L5, S1)

Follow-up to C5b (735f0a72). The retail-conformance review passed, so no
production behaviour line moved: the flag truth table and the
refreshPosition:false withhold are untouched. This is blast radius, test
discrimination and documentation fidelity - plus two findings I could not
confirm and am rebutting rather than complying with.

D3 - THE PUBLISH-CONSERVATION TEST DID NOT DISCRIMINATE. The reviewer was
right and it was the worst finding here: proof obligation 3's test passed
identically with C5b reverted. Its only delta assertion FILTERED
(Assert.Single(deltas, Rebucketed && parentGuid)), so the pre-C5b stream
[Rebucketed] and the post-C5b stream [Updated, Rebucketed] both satisfied
it, and childSpatialBefore+1 held in both worlds because whichever site did
not move the cell propagated idempotently. It now asserts the complete
ordered parent stream plus each element's CellId and Position.ObjCellId.
Sabotage: restoring refreshPosition:acceptedPosition turns it red (it was
green before), together with the withhold test and the new L5 test.

That cardinality change was itself unfiled and is now AP-147: a
cell-changing accepted Position publishes TWO entity deltas where it
published one, and the intermediate Updated pairs the OLD CellId with the
NEW wire Position - a torn pair that did not exist pre-C5b, since both
halves used to move inside one publish. No production consumer reads a
delta's paired fields, but a recorder/plugin/bot event log would capture
it. The row states why suppressing the Updated is not available at that
layer (the merge cannot know whether its caller reaches W2).

D4 - THE PROJECTILE DOC COMMENT WAS FALSE AND ITS RETAIL ARGUMENT WAS
INVERTED. SyncPresentationFromResolvedBody claimed record.FullCellId is
"the WIRE cell ... stamped by the merge's RefreshDerivedState/SetFullCell,
before classification ever runs" and argued from retail's store_position
@0x00515CE2 that the destination cell is the right one. C5b falsified the
premise; the missile arm also returns before W2, so nothing stamps the wire
cell for a projectile at all. Rewritten. The honest conclusion, which the
old text would have called wrong: on a stored outcome presentation now
pairs the DESTINATION world position with the SOURCE cell. That is not a
choice this method can make differently - StoreAcceptedDestinationPose
writes only Position/Orientation, so record.FullCellId and
body.CellPosition.ObjCellId now hold the same source cell and reading
either yields the same value. The divergence is AP-138 item (1)'s
store-writes-pose-but-not-cell residual, retiring via #309, not a field
choice here. Projecting the wire cell instead would invent a residency the
placement declined - the AP-1 shape C5b closed.

L3/L4/L5 - PINNING GAPS, ALL THREE CONFIRMED AND CLOSED.
L3: the matrix's oracle passed HasAnimations as a literal, so the merge's
old.MotionTableId ?? old.Physics?.MotionTableId and
RuntimeAcceptedPositionRouteRequests.Build's canonical-snapshot twin were
textually identical and pinned by nothing. The oracle is now BUILT by the
production constructor.
L4: every fixture set both MotionTableId halves to the same value, so
deleting either operand of the ?? was undetectable while the production
comment said the mixed case is the real-world one. Six mixed rows added,
including the explicit-zero row (a present-but-zero top half is not null,
so ?? never reaches the physics half).
L5: the retained Rebucketed ternary had zero coverage through
TryApplyPosition - every restoreCancelledPark test called Forget directly.
Now driven through the real merge, with the wire cell deliberately the
SOURCE while the park's committed body cell is the DESTINATION, so the
restored residency can only have come from the rollback.
Sabotage (each red, each restored): merge ?? -> top half only, 1 red;
-> physics half only, 2 red; Build's ?? -> physics half only, 2 red;
ternary -> constant Updated, exactly the L5 test red.

L1/L2 - THE MISSING TEST IS ADDED; THE DEFECT IS NOT THERE. The reviewer
was right that C5b's "no fixture covers pickup at that layer" was
inaccurate - LiveEntityNetworkOnPositionCollapseMatrixTests drives the real
OnPosition at ~26 sites - and the end-to-end test is added: withdraw ->
accepted Position -> IsSpatiallyProjected && FullCellId == wireCell, both
guid classes.

But ChildUnparentDisposition.Pending is NOT a live defect, because it is
production-unreachable. The sole production _withdrawProjection binding
(LivePresentationComposition.cs:599) is
LiveEntityProjectionWithdrawalController.WithdrawExact, whose only Pending
mint is inside its catch block and therefore always carries a non-null
Failure - and AdvanceUnparentTransition rethrows at
EquippedChildRenderController.cs:1307 BEFORE the return Pending at :1309.
The named drop scenario does not reach it anyway (BeginDetachedRemoval has
already emptied the capture list) and would be correct if it did: a
previously-equipped child is LegacyImmediate, so the FullCellId != 0u gate
at DatLiveEntityProjectionMaterializer.cs:767 is never consulted and
re-projection uses the wire cell at LiveEntityRuntime.cs:824.

Measured while building that test, and NOT what C5b assumed: W2 and W3 are
REDUNDANT on the remote tail. Sabotaging W2 alone - adopting the committed
cell instead of the wire cell, OR skipping the rebucket outright - leaves
the whole file green, because W3's RemoteMotion.CellId write reads through
to canonical FullCellId via CommitCanonicalCell, whose CellCommitted
recovery re-installs the bucket. Only removing BOTH goes red, and then the
new test is the only red in the file. So it is named for what it pins, and
AD-60 is amended with the measurement: neither channel is individually
load-bearing, so a future retirement of one is caught by nothing else.

D2 - REBUTTED, WITH THE REAL GAP FILED INSTEAD. The reviewer's hypothesis
was that TryApplyInitialCreateCompletionPresentation's staleness guard lost
its ability to detect an intervening steady-state Position when C5b stopped
the merge stamping the wire cell, and asked for a PositionAuthorityVersion
term. I do not think that is right and did not add it.

The receipt's facts are the canonical BODY's pose and cell at publish
(PublishExecutorCompletion builds both from the record). Exactly two owners
can move them: a Runtime SetPosition commit/withdrawal, every one of which
calls AdvancePlacementCommit - the only caller family is
RuntimeSetPositionState - and a rebucket, which moves FullCellId. Both are
already covered by the two existing terms. An accepted steady-state
Position is neither, and C5b did not make it one: the merge refreshes the
snapshot and advances PositionAuthorityVersion but never wrote the body,
and the App generic tail writes the RENDER entity. The wire-cell half stays
covered because W2/W3 commit it in the same call; the paths that return
before them leave the record at the last committed cell, which IS the
receipt's own cell - correctly not a supersession.

Adding the term would decline receipts whose facts are still true, on the
entity's FIRST world-visible moment: the pose write and
RebucketLiveEntityPresentationOnly would be skipped while TryPublishPlace
still publishes, so a packet returning before the render write would leave
the sidecar visible at its materialized pose in a wrong bucket. That is the
handoff's own "removed the invariant failure while leaving the bug" shape.

There IS one supersession neither term covers, and it predates C5b:
RuntimeRemotePlacementDriveController.StoreAcceptedDestinationPose writes
body.Position/Orientation on the far-snap Refused/Contention arm with no
placement commit and no cell move. Filed as #323 with the FIFO-blocking
argument for why a receipt can still be pending when it lands, an explicit
"not established as reachable", and an explicit "do not fix it with
PositionAuthorityVersion". The guard's comment now carries the whole
argument instead of one sentence.

S1 - DANGLING POINTER CLOSED. InboundPhysicsStateController.cs:610 still
said the two-callers-one-rule debt was "tracked for the eventual cutover
unification ... See docs/ISSUES.md", which pointed at nothing after C5b
closed #275 without a successor. Filed #322, cited from both the comment
and #275's closure, including why widening TryApplyPosition's signature to
take a route would be the wrong unification.

AP-138 amended: C5b staled its round-3 measurement that "both
accepted-Position callers commit the accepted wire cell to
record.FullCellId before submitting". Route 2 submits from
TryExecuteAcceptedLocalPosition ahead of W2, so on a first submit
PlacementTouchesPrefix's CurrentCellId arm now names the SOURCE landblock,
not the destination. Confined to which prefix the quiescence pre-flight
matches, which that row already established is not the correctness
mechanism.

GATES. Release build 0 errors. Complete suite 11,134 passed / 4 skipped /
0 failed, from the 11,125 / 4 baseline at ed806997: net +9, all new tests,
no test deleted or weakened, no new skip. Runtime.Tests 1195 -> 1202 (+6
mixed-motion-table rows, +1 park-rollback fact); App.Tests 4132 -> 4134
(+2 guid rows). None of #302/#308/#321 appeared. Not connected-gated -
nothing here changes runtime behaviour.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
Erik 2026-08-05 22:08:46 +02:00
parent ed8069976c
commit 23aa62f292
9 changed files with 637 additions and 62 deletions

View file

@ -503,38 +503,62 @@ internal sealed class ProjectileController
/// before this is called.
///
/// <para>
/// <b>B1/B2 fix (round-2 review — reverting round 1's own R6 "fix",
/// which the retail reviewer retracted as factually wrong).</b>
/// <b>Which cell this projects — REWRITTEN 2026-08-05 (C5b review, D4).
/// The round-2 B1/B2 text this replaces was falsified by C5b and its
/// retail argument now points the other way; do not restore it.</b>
/// <c>ParentCellId</c> is read from <c>record</c>'s (the local resolved
/// by <c>TryGetCurrent</c> below, equal to
/// <paramref name="expectedRecord"/> once the currency check passes)
/// <c>FullCellId</c> — the WIRE cell — never the body's own
/// <c>CellPosition.ObjCellId</c>. On a committed outcome the two agree
/// (<c>CommitCanonical</c> writes both together), so the choice is a
/// no-op there. On a STORED outcome
/// (<c>Refused</c>/<c>Contention</c>/<c>RejectedPreparation</c>),
/// <c>StoreAcceptedDestinationPose</c> composes <c>body.Position</c>
/// from <c>accepted.PositionX + worldOffset(accepted.LandblockId)</c> —
/// the WIRE cell's world frame — while <c>record.FullCellId</c> is that
/// SAME wire cell (stamped by the merge's
/// <c>RefreshDerivedState</c>/<c>SetFullCell</c>, before classification
/// ever runs). The body's OWN <c>CellPosition.ObjCellId</c>, in
/// contrast, is never touched by the store fallback and is therefore
/// the SOURCE cell the body left — reading it here would pair the new
/// destination position with the cell the body is no longer in. Three
/// independent checks confirm <c>record.FullCellId</c> is the correct
/// source: (1) the sibling remote arm's
/// <c>TryApplyGenericRemoteRenderPose</c> pairs the wire world position
/// with the wire cell the same way; (2) Runtime's own
/// <c>SyncProjectilePresentation</c>, forty lines away in the SAME
/// packet, publishes the shadow row at <c>record.FullCellId</c> — so
/// reading the body's cell here would disagree with the shadow for the
/// SAME body in the SAME call; (3) retail's <c>store_position</c>
/// @0x00515CE2 writes the object's whole <c>Position</c> INCLUDING
/// <c>objcell_id</c>, so after a retail store the object's cell IS the
/// destination (the wire cell) — never the stale one. The body's own
/// stale cell after a store is the acdream-side residual (AP-138,
/// shared with the remote arms), not the truth to project.
/// <c>FullCellId</c> — since C5b (#275) that is the last COMMITTED cell,
/// never the wire cell. The steady-state merge withholds the derived
/// <c>FullCellId</c> write entirely
/// (<c>RuntimeEntityObjectLifetime.TryApplyPosition</c>'s
/// <c>RefreshSnapshot(..., refreshPosition: false)</c>, AD-60), and the
/// missile arm returns from <c>OnPosition</c> before the prologue
/// rebucket (W2) ever runs, so nothing between admission and this call
/// stamps the wire cell for a projectile.
/// </para>
///
/// <para>
/// <b>The choice of source is now a no-op on every outcome.</b> On a
/// COMMITTED outcome <c>CommitCanonical</c> writes
/// <c>record.FullCellId</c> and <c>body.CellPosition.ObjCellId</c>
/// together. On a STORED outcome
/// (<c>Refused</c>/<c>Contention</c>/<c>RejectedPreparation</c>)
/// <c>StoreAcceptedDestinationPose</c> writes <c>body.Position</c> and
/// <c>body.Orientation</c> and NOTHING else — it does not move the
/// body's <c>CellPosition.ObjCellId</c>, and post-C5b it does not move
/// the record's <c>FullCellId</c> either. Both therefore hold the same
/// last-committed (SOURCE) cell, so reading either field yields the
/// identical value. That is what
/// <c>MissileFarRefused_…ParentCellIdAgreesWithCommittedCell</c> pins:
/// an identity between the record and the body, not a wire-cell
/// constant. Runtime's own <c>SyncProjectilePresentation</c>, forty
/// lines away in the SAME packet, publishes the shadow row at
/// <c>record.FullCellId</c>, so this keeps presentation and shadow in
/// agreement for the same body in the same call.
/// </para>
///
/// <para>
/// <b>The pairing this leaves is genuinely torn, and is named rather
/// than papered over.</b> After a stored outcome the render entity gets
/// the DESTINATION world position (the store composed it from
/// <c>accepted.PositionX + worldOffset(accepted.LandblockId)</c>, the
/// wire cell's world frame) paired with the SOURCE cell. Retail cannot
/// produce that state twice over: it has no refusable placement at all
/// (AD-62), and its <c>store_position</c> @0x00515CE2 writes the whole
/// <c>Position</c> INCLUDING <c>objcell_id</c> before
/// <c>GotoLostCell</c> @0x00515CF2, so a retail store ends at the
/// destination cell AND the destination pose. The divergence is
/// acdream's store-writes-pose-but-not-cell residual — AP-138 item (1)'s
/// "at the destination, with no resolved cell", shared with both remote
/// arms — and its retirement is issue #309 (make the failure path open
/// retail's lost-cell registration), NOT reading a different field here.
/// Projecting the wire cell instead would invent a residency the
/// placement explicitly declined, which is the exact AP-1-shaped write
/// C5b closed. Reachability today is deterministic-test-only: ACE never
/// sends <c>UpdatePosition</c> for a missile (AP-141's evidence,
/// <c>WorldObject_Tick.cs:333-334</c>/<c>:265</c>).
/// </para>
///
/// <para>

View file

@ -1221,6 +1221,52 @@ public sealed class LiveEntityRuntime : ILiveEntityRadarSource
!= token.PlacementCommitVersion)
{
// A newer move superseded this receipt's facts after the drain.
//
// WHY THESE TWO TERMS AND NOT `PositionAuthorityVersion` (C5b
// review, finding D2 — examined and deliberately left alone;
// the sibling guard in
// TryGetRuntimePlacementProjectionRecord(requirePlacementVersions:
// true) DOES check it, and the asymmetry is intentional).
//
// This receipt's facts are the CANONICAL BODY's pose and cell at
// publish time (RuntimeSetPositionState.PublishExecutorCompletion
// builds the snapshot from `record.PhysicsBody` and
// `record.FullCellId`). There are exactly two owners that can move
// either: a Runtime SetPosition commit/withdrawal, every one of
// which calls `AdvancePlacementCommit` — the ONLY caller family is
// RuntimeSetPositionState itself — and a rebucket, which moves
// `FullCellId`. Both are covered above.
//
// An intervening accepted steady-state Position is NOT one of
// them, and specifically was not turned into one by C5b (#275).
// C5b made the merge withhold the `FullCellId` write, but the
// merge never wrote the canonical body either: it refreshes the
// snapshot and advances `PositionAuthorityVersion`, and the
// App-side generic tail writes the RENDER entity, not the body.
// The wire-cell half is still covered here because the OnPosition
// prologue rebucket (W2) commits it in the same call, so a
// cell-changing Position still trips the first term; the paths
// that return before W2 (the local force arm, the missile arm)
// leave the record exactly where the last commit put it, which is
// this receipt's own cell — correctly NOT a supersession.
//
// Adding `PositionAuthorityVersion` would therefore decline
// receipts whose facts are still true, on the entity's FIRST
// world-visible moment: the pose write and
// RebucketLiveEntityPresentationOnly below would be skipped while
// TryPublishPlace still publishes, so a packet that returned
// before the generic tail's render write would leave the sidecar
// visible at its materialized pose in a possibly-wrong bucket.
// That is a behaviour change in the wrong direction, not a
// restoration.
//
// The ONE supersession neither term covers is
// RuntimeRemotePlacementDriveController.StoreAcceptedDestinationPose:
// the far-snap Refused/Contention arm writes `body.Position` and
// `body.Orientation` with no placement commit and no cell move, so
// it can stale this receipt's pose silently. That is pre-existing
// (it predates C5b, which changed nothing about that arm) and is
// filed as docs/ISSUES.md #323 rather than papered over here.
return true;
}

View file

@ -607,11 +607,14 @@ public sealed class InboundPhysicsStateController
/// contact still comes solely from the retained wire packet's own
/// <c>IsGrounded</c> bit on both. That remaining structural difference
/// (two callers computing the same two flags from the same two inputs
/// rather than sharing one code path) is internal refactor debt tracked
/// for the eventual cutover unification - it is NOT a retail divergence
/// and does not belong in
/// docs/architecture/retail-divergence-register.md. See docs/ISSUES.md
/// for the tracked follow-up.
/// rather than sharing one code path) is internal refactor debt - it is
/// NOT a retail divergence and does not belong in
/// docs/architecture/retail-divergence-register.md. It is tracked as
/// docs/ISSUES.md issue <b>#322</b>, filed 2026-08-05 at the C5b review
/// (finding S1): #275 is CLOSED and this comment's former "tracked for
/// the eventual cutover unification / see docs/ISSUES.md" wording pointed
/// at nothing once it was. #322 also records why widening this method's
/// signature to take a whole route would be the wrong unification.
/// </summary>
public bool TryApplyPosition(
WorldSession.EntityPositionUpdate update,