fix(runtime): close the C5b re-review findings — Gate A narrowing filed, no-window payload gate, bisect hazard recorded
Both C5b re-reviews returned PASS on 02578441..ff100cf3. This lands the bookkeeping corrections they left, the one gate asymmetry both found independently, and one wrong retail fact neither of them caught. 1. AP-148 / #325 — Gate A's teleport test, wrong on primary source twice. The C5b contract stated retail's Gate A teleport term as "TELEPORT_TS equal" (and, in the trace block, as "must NOT be newer") and blessed acdream's `teleport == _timestamps[Teleport]` as retail-exact. Disassembly of the PDB-paired binary at SmartBox::HandleReceivedPosition 0x0045402B-0x00454054 says otherwise: the shortcut is taken iff the wire stamp is equal OR newer (wrap-safe) — `sbb eax,eax / neg eax` materialises the carry of the compare and the branch skips Gate A on CF, i.e. only when the wire stamp is strictly OLDER. It is CPhysicsObj::newer_event @0x00451B10's identical idiom with the operands swapped. Binary Ninja drops the flag test and renders it `if (-((eax_7 - eax_7)) == 0)`, always true — which is why two rounds of reading pseudo-C recorded it backwards. So acdream's ForcePosition disposition is a strict SUBSET of retail's Gate A set, and a local ForcePosition carrying a newer teleport stamp is misrouted into a full Apply: wire heading instead of preserved heading, unparent, possible placement frame, zeroed velocity, TELEPORT_TS advanced, and OfferTeleportDestination called for a packet retail never starts presentation for. PhysicsTimestampGate.cs is NOT changed. The predicate exists twice (also ValidAcceptedAuthority's PreviousTeleport == AcceptedTeleport), and the fix has to decide TELEPORT_TS's disposition on a Gate A path that has never seen a stale-but-equal pair. #325 records all of it and says explicitly that it is not a one-line comparison swap. C5b made this marginally better, not worse: clearParent was unconditionally true before C5b and is unchanged; installPlacementFrame moved toward retail's HasAnims gate. 2. Retail F2 / architecture L-A — the no-window route had no pre-merge payload validation. Root fix, not a documented asymmetry. The graphical route validates before the merge (OnPosition's payloadIsValid -> LiveEntityInboundAuthorityGate's !payloadIsValid return); despite its name CanAcceptPositionPayload is not projectile-scoped. The no-window route had no equivalent, and since D1 fed an unvalidated LandblockId into CommitWireCellRebucket — where 0 is the withdrawal shape, silently de-residencing the entity in the field every bot reads as CellId. RuntimeLiveEntitySessionController.OnPositionUpdated now applies the same rule at the same point, reusing RuntimeAuthoritativePositionRouteClassifier.IsValidCreateWirePosition plus the finite-velocity term — the exact pair TryApplyPosition already applies on its initial-residence branch. Chosen over documenting it because the fix is five lines and leaving it would have left two written claims falsified by the code. It is a behaviour change: headless now drops packets it merged. Against ACE the set is empty, and the graphical host has carried this gate since it was written; the argument is recorded in the contract's §15.2 rather than gated. Two test fixtures carried cell ids retail's own inbound_valid_cellid rejects (low words 0x41 and 0x51, above the 0x40 landcell ceiling). Their constants were corrected; their assertions were not. New test sabotage-verified in both directions: gate removed -> red at the withdrawal-shape assertion; gate moved to guard only the cell commit -> red at the pose assertion, which is what makes it a before-the-MERGE test rather than a before-the-commit test. 3. Register and doc corrections. - AD-64: "deliberately absent" was presented as the complete difference list and was not. Adds (a) the residence gate is weaker than the merge's own — both hosts' commits use TryGetCurrent while TryApplyPosition's FIFO branch uses TryGetTransaction, so the wire cell can commit ahead of the continuation that will replay it; (b) the two missile gates are two different expressions that agree today; (c) the payload gate, now present. Risk column records that (a) and (b) have no discriminating test on either side. - AP-147: amended for D1 — pre-D1 the no-window host published [Updated] alone and lost the Rebucketed, so a headless event log is now a real instance of the "consumer that snapshots a delta" the row warns about. - AD-60: "Matches retail exactly" scoped to the withhold, since the row's body documents two channels that do not. - CommitWireCellRebucket: notes the unreachable ThrowIfNull / EnsureNotDisposed precedence inversion. - TryCommitAcceptedWireCell: the discarded commit bool is explained rather than left bare — false means IsCurrent went stale, unreachable three statements after a synchronous TryGetActive. 4. Bisect hazard recorded in the C4 closeout handoff (the doc CLAUDE.md sends readers to before any C5 work) and in the contract's §15.3: commits 735f0a72..23aa62f2 contain a live headless defect — every remote's FullCellId frozen for the session — introduced by735f0a72and fixed only atff100cf3. Nothing throws and no test in the range fails. Gates: Release build 0 errors/0 warnings. Complete suite 11,142 passed / 4 skipped / 0 failed against the 11,141 / 4 / 0 baseline — net +1, exactly the one new test. No flake appeared (#302, #308, #321 all green). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
Erik
parent
ff100cf33f
commit
9ee9c1a1a6
7 changed files with 455 additions and 21 deletions
|
|
@ -2092,6 +2092,15 @@ public sealed class RuntimeEntityObjectLifetime : IDisposable
|
|||
uint spatialCellOrLandblockId,
|
||||
Action<RuntimeEntityRecord>? acknowledgeProjection = null)
|
||||
{
|
||||
// Noted at the C5b closeout, unreachable in production and left as
|
||||
// is: this null check runs BEFORE CommitRebucket's EnsureNotDisposed,
|
||||
// so a call that is both disposed AND null throws
|
||||
// ArgumentNullException where the same call straight to
|
||||
// CommitRebucket throws ObjectDisposedException. No caller can
|
||||
// produce that pair — both hosts' callers resolve `canonical` from a
|
||||
// live TryGetActive on the same thread — and reordering would put a
|
||||
// disposed-instance check in front of a derivation that touches no
|
||||
// instance state.
|
||||
ArgumentNullException.ThrowIfNull(canonical);
|
||||
uint committedFullCell =
|
||||
(spatialCellOrLandblockId & 0xFFFFu) != 0xFFFFu
|
||||
|
|
|
|||
|
|
@ -4,6 +4,7 @@ using AcDream.Core.Net.Messages;
|
|||
using AcDream.Core.Physics;
|
||||
using AcDream.Runtime.Entities;
|
||||
using AcDream.Runtime.Gameplay;
|
||||
using AcDream.Runtime.Physics;
|
||||
using AcDream.Runtime.World;
|
||||
|
||||
namespace AcDream.Runtime.Session;
|
||||
|
|
@ -223,6 +224,42 @@ public sealed class RuntimeLiveEntitySessionController
|
|||
private void OnPositionUpdated(
|
||||
WorldSession.EntityPositionUpdate update)
|
||||
{
|
||||
// C5b follow-up (2026-08-05), retail finding F2 / architecture
|
||||
// finding L-A, found independently by both re-reviewers. The
|
||||
// graphical route validates the wire payload BEFORE the merge —
|
||||
// LiveEntityNetworkUpdateController.OnPosition computes
|
||||
// `payloadIsValid` from ProjectileController.CanAcceptPositionPayload
|
||||
// (retail Position::IsValid @0x005A9480 composed with Frame::IsValid
|
||||
// @0x00534ED0, plus finite origin/velocity) and
|
||||
// LiveEntityInboundAuthorityGate.TryAcceptPosition returns false on
|
||||
// it, ahead of the timestamp gate and every wire-cell writer. Despite
|
||||
// its name that check is not projectile-scoped; it runs for every
|
||||
// guid. This route had no equivalent, so an invalid payload merged
|
||||
// here and then — since D1 — fed its unvalidated LandblockId into
|
||||
// CommitWireCellRebucket, whose own doc calls a 0 landblock "the
|
||||
// withdrawal shape": cell 0 + landblock 0, silently de-residencing
|
||||
// the entity in the exact field every bot reads as
|
||||
// RuntimeEntitySnapshot.CellId.
|
||||
//
|
||||
// The predicate is not re-derived here. It is
|
||||
// RuntimeAuthoritativePositionRouteClassifier.IsValidCreateWirePosition
|
||||
// plus the finite-velocity term — literally the pair
|
||||
// RuntimeEntityObjectLifetime.TryApplyPosition already applies on its
|
||||
// initial-residence branch, and the same composition the graphical
|
||||
// gate applies. Rejecting BEFORE the merge (rather than before the
|
||||
// cell commit alone) is what makes the two hosts genuinely
|
||||
// symmetric: neither one lets an invalid payload advance the
|
||||
// timestamp gate.
|
||||
if (!RuntimeAuthoritativePositionRouteClassifier
|
||||
.IsValidCreateWirePosition(update.Position)
|
||||
|| update.Velocity is { } wireVelocity
|
||||
&& !(float.IsFinite(wireVelocity.X)
|
||||
&& float.IsFinite(wireVelocity.Y)
|
||||
&& float.IsFinite(wireVelocity.Z)))
|
||||
{
|
||||
return;
|
||||
}
|
||||
|
||||
bool isLocal =
|
||||
update.Guid == _runtime.PlayerIdentity.ServerGuid;
|
||||
PlayerMovementController? localController =
|
||||
|
|
@ -358,9 +395,12 @@ public sealed class RuntimeLiveEntitySessionController
|
|||
|
||||
/// <summary>
|
||||
/// D1 (C5b architecture review): commits the accepted wire cell to
|
||||
/// canonical residency for a no-window host, under the same
|
||||
/// reachability rules the graphical <c>OnPosition</c> route applies to
|
||||
/// AD-60's W2. The committed VALUE is
|
||||
/// canonical residency for a no-window host, under reachability rules
|
||||
/// derived one by one from the graphical <c>OnPosition</c> route's own
|
||||
/// early returns for AD-60's W2. They are NOT identical, and AD-64
|
||||
/// enumerates every place they differ — the two absent gates, the
|
||||
/// residence gate's weaker predicate, and the missile gate's structural
|
||||
/// drift risk. The committed VALUE is
|
||||
/// <see cref="RuntimeEntityObjectLifetime.CommitWireCellRebucket"/>'s —
|
||||
/// one rule, shared by both hosts, including its landblock-vs-cell
|
||||
/// branch.
|
||||
|
|
@ -387,6 +427,27 @@ public sealed class RuntimeLiveEntitySessionController
|
|||
/// operation kind. Committing a wire cell for a projectile here would
|
||||
/// invent residency a placement route owns.
|
||||
/// </para>
|
||||
///
|
||||
/// <para>
|
||||
/// <b>Two known imprecisions, both host-symmetric and both pre-existing;
|
||||
/// filed at AD-64 rather than papered over here.</b> (1) The residence
|
||||
/// gate uses <c>TryGetInitialCreateResidence</c> (<c>TryGetCurrent</c>),
|
||||
/// while <c>RuntimeEntityObjectLifetime.TryApplyPosition</c>'s own FIFO
|
||||
/// branch uses the strictly WEAKER <c>TryGetPendingInitialResidence</c>
|
||||
/// (<c>TryGetTransaction</c> = current OR a completed-but-unretired
|
||||
/// lease). In that window the merge enqueues the packet as a
|
||||
/// continuation while this gate reads "no residence" and commits the
|
||||
/// wire cell ahead of the continuation that will replay it. The
|
||||
/// graphical route's <c>RebucketLiveEntity</c> reads the same weaker
|
||||
/// predicate, so both hosts have it identically. (2) The missile
|
||||
/// predicate below is the graphical route's FALLBACK conjunction; that
|
||||
/// route PREFERS <c>earlyRemoteRoute.OperationKind is
|
||||
/// ProjectileAuthoritative</c> and drops to the conjunction only when
|
||||
/// the classification is null. The two agree today — the conjunction is
|
||||
/// what the classifier's own projectile test is built from — but they
|
||||
/// are separate expressions and only one of them is reachable here,
|
||||
/// because this route classifies nothing for a remote.
|
||||
/// </para>
|
||||
/// </summary>
|
||||
private void TryCommitAcceptedWireCell(
|
||||
WorldSession.EntityPositionUpdate update)
|
||||
|
|
@ -400,6 +461,17 @@ public sealed class RuntimeLiveEntitySessionController
|
|||
return;
|
||||
}
|
||||
|
||||
// The bool is discarded, where the graphical caller
|
||||
// (LiveEntityRuntime.RebucketLiveEntity) treats false as
|
||||
// ThrowAfterCommittedProjectionChange. That is not a suppressed
|
||||
// failure: false means `Entities.IsCurrent(canonical)` went stale, and
|
||||
// TryGetActive above returned the CURRENT record synchronously three
|
||||
// statements earlier on the same thread, with nothing in between that
|
||||
// can retire it. The graphical caller needs the test because it has
|
||||
// already published spatial/presentation changes by that point and a
|
||||
// stale canonical would leave them orphaned; this route publishes
|
||||
// nothing ahead of the commit, so there is no half-applied state to
|
||||
// detect. Asserting on it would be asserting on an unreachable value.
|
||||
_ = Entities.CommitWireCellRebucket(
|
||||
canonical,
|
||||
update.Position.LandblockId);
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue