diff --git a/src/AcDream.Runtime/Physics/RuntimeSetPositionState.cs b/src/AcDream.Runtime/Physics/RuntimeSetPositionState.cs index 68efb27f..7a285b24 100644 --- a/src/AcDream.Runtime/Physics/RuntimeSetPositionState.cs +++ b/src/AcDream.Runtime/Physics/RuntimeSetPositionState.cs @@ -1,4 +1,5 @@ using System.Collections.Immutable; +using System.Diagnostics.CodeAnalysis; using System.Numerics; using AcDream.Content; using AcDream.Core.Items; @@ -265,7 +266,8 @@ internal readonly record struct RuntimeSetPositionOwnershipSnapshot( int PlacementCompletionWatchCount, int AcknowledgedPlacementCompletionCount, int CollisionPrefixQuiescenceCount, - int PendingQuiescenceProjectionCount) + int PendingQuiescenceProjectionCount, + int PooledOperationCount) { internal bool IndexesConsistent => LostDeadlineCount == LostDeadlineNodeCount @@ -275,6 +277,16 @@ internal readonly record struct RuntimeSetPositionOwnershipSnapshot( && UnboundDeferredCellCount == UnboundDeferredCellOrderCount && MoverPreparationAuthorityCount <= ActiveOperationCount; + /// + /// F2: deliberately EXCLUDED from . + /// is retained, idle Operation + /// capacity in the C2 object pool - legitimate to hold mid-session (that + /// is the entire point of pooling), so it must not make an otherwise + /// fully-drained, healthy session read as "not converged". Reset/dispose + /// tests assert it separately drops to zero + /// (OperationPoolClearsOnResetSession / + /// OperationPoolClearsOnDispose) instead of folding it into this gate. + /// internal bool IsConverged => ActiveOperationCount == 0 && AwaitingPreparationCount == 0 && DeferredCellCount == 0 @@ -334,19 +346,33 @@ internal sealed class RuntimeSetPositionState : IDisposable bool Prepared, RuntimeSetPositionCommand PreparedCommand); + /// + /// C2: settable (not init/required) so + /// / can recycle instances via + /// instead of allocating a fresh + /// object on every accepted placement. This was the single largest + /// contributor to the C2 allocation budget finding (see + /// docs/research/2026-07-31-canonical-set-position.md). Every + /// construction site (BeginAcceptedPlacementCore, + /// CreateWithdrawalOperation, ParkDeferred) still sets + /// every field it always set before; the only behavior change is that a + /// field left unset by a given site now comes from an explicit reset + /// instead of the CLR's implicit new-object default - the two are + /// identical in value. + /// private sealed class Operation { - internal required RuntimeEntityRecord Record { get; init; } + internal RuntimeEntityRecord Record { get; set; } = null!; internal PhysicsBody? Body { get; set; } - internal required RuntimeEntityPlacementToken Token { get; init; } - internal required RuntimeEntityKey Key { get; init; } - internal required ulong PositionAuthorityVersion { get; init; } - internal required ulong SessionLifetimeVersion { get; init; } - internal required ulong SourceSpatialAuthorityVersion { get; init; } - internal required ulong SourceVelocityAuthorityVersion { get; set; } - internal required bool PreviousContact { get; set; } - internal required bool PreviousOnWalkable { get; set; } - internal required RuntimeSetPositionCommand Command { get; set; } + internal RuntimeEntityPlacementToken Token { get; set; } + internal RuntimeEntityKey Key { get; set; } + internal ulong PositionAuthorityVersion { get; set; } + internal ulong SessionLifetimeVersion { get; set; } + internal ulong SourceSpatialAuthorityVersion { get; set; } + internal ulong SourceVelocityAuthorityVersion { get; set; } + internal bool PreviousContact { get; set; } + internal bool PreviousOnWalkable { get; set; } + internal RuntimeSetPositionCommand Command { get; set; } internal PhysicsSetPositionResult Result { get; set; } internal ulong SpatialAuthorityVersion { get; set; } internal ulong PlacementCommitVersion { get; set; } @@ -359,8 +385,8 @@ internal sealed class RuntimeSetPositionState : IDisposable internal ulong ProjectionSequence { get; set; } internal bool WakeableLostCell { get; set; } internal RuntimeEntityPlacementStage Stage { get; set; } - internal RuntimeSetPositionOperationKind Kind { get; init; } - internal RuntimePortalPlacementAuthority Portal { get; init; } + internal RuntimeSetPositionOperationKind Kind { get; set; } + internal RuntimePortalPlacementAuthority Portal { get; set; } internal bool RequiresPreparation { get; set; } internal bool Expired { get; set; } internal List? LostFamilyKeys { get; set; } @@ -372,6 +398,69 @@ internal sealed class RuntimeSetPositionState : IDisposable get; set; } + + /// + /// F3: pool-membership guard, not operation data. Set true by + /// right before pushing; cleared + /// here (called from right after + /// popping). Lets RetireOperationToPool detect and throw on a + /// double-retire - two retire calls for the same instance without an + /// intervening rent would otherwise silently duplicate it in the + /// pool stack. + /// + internal bool InPool { get; set; } + + /// + /// C2/F4: the ONLY place every field is set to its inert default - + /// this is the completeness net that `required`/`init` used to + /// provide before pooling needed plain settable properties. Called + /// exactly once when an Operation is retired to the pool + /// (), before it can be handed + /// back out by . is + /// left null! only for the instant the instance sits in the pool - + /// every rent site immediately overwrites it before any other + /// member is read. A reflection-based test + /// (OperationResetAllFieldsToDefaultTouchesEveryDeclaredField) pins + /// that this method assigns every declared instance field of this + /// class by name - a newly added field fails that test until both + /// the reset and the test's expected-field list are updated. + /// + internal void ResetAllFieldsToDefault() + { + Record = null!; + Body = null; + Token = default; + Key = default; + PositionAuthorityVersion = 0UL; + SessionLifetimeVersion = 0UL; + SourceSpatialAuthorityVersion = 0UL; + SourceVelocityAuthorityVersion = 0UL; + PreviousContact = false; + PreviousOnWalkable = false; + Command = default; + Result = default; + SpatialAuthorityVersion = 0UL; + PlacementCommitVersion = 0UL; + ExactCellId = 0u; + CollisionGeneration = 0UL; + CollisionPrefix = 0u; + WithdrawalAcknowledged = false; + CollisionGenerationReady = false; + CollisionQuiescenceHeld = false; + ProjectionSequence = 0UL; + WakeableLostCell = false; + Stage = default; + Kind = default; + Portal = default; + RequiresPreparation = false; + Expired = false; + LostFamilyKeys = null; + InheritedLostDeadline = false; + EnteringWorldFromCelllessResidence = false; + DormantLocalActivation = false; + PreparedCommandAwaitingWithdrawalAck = null; + InPool = false; + } } private sealed class CollisionPrefixQuiescence @@ -393,9 +482,24 @@ internal sealed class RuntimeSetPositionState : IDisposable internal bool ReleaseGenerationReady { get; set; } } + /// + /// F1: captures the operation's PositionAuthorityVersion/ + /// SpatialAuthorityVersion as PLAIN VALUES at construction time instead + /// of holding an reference and reading its + /// fields lazily inside . IsCurrent can run + /// DURING the ground-edge HitGround/LeaveGround callbacks this guard is + /// built for - a synchronous cancel-then-begin (or begin-twice) chain + /// for the SAME entity can retire this exact Operation instance to the + /// pool and rent it right back out (LIFO) for a different logical + /// operation before this guard is asked whether it is still current. A + /// live Operation reference would then silently read the WRONG + /// operation's authority versions; these captured scalars cannot be + /// repurposed out from under it. + /// private sealed class ContactCommitGuard( RuntimeSetPositionState owner, - Operation operation, + ulong positionAuthorityVersion, + ulong spatialAuthorityVersion, RuntimeEntityRecord record, PhysicsBody body, ulong placementCommitVersion, @@ -403,7 +507,8 @@ internal sealed class RuntimeSetPositionState : IDisposable { internal bool IsCurrent() => owner.IsCanonicalPlacementCommitCurrent( - operation, + positionAuthorityVersion, + spatialAuthorityVersion, record, body, placementCommitVersion, @@ -452,12 +557,163 @@ internal sealed class RuntimeSetPositionState : IDisposable double Deadline, ulong Sequence); + /// + /// C2: everything reads + /// to invoke + /// on behalf of the in-flight PhysicsEngine.SetPosition call. A + /// plain per-call lambda closing over the live + /// (and, at one call site, a local canonicalCommand) allocated a + /// fresh display class every accepted placement; this struct is pushed by + /// value onto instead so the ONE + /// cached delegate below never needs a new closure. A stack (not a single + /// field) survives any theoretical nested/re-entrant SetPosition call at + /// the PhysicsEngine layer - TransitionScratchArena.ActiveDepth/ + /// Capacity implies nesting is possible there even though + /// itself + /// never calls back into this class. + /// + private readonly record struct CollisionCallbackContext( + RuntimeEntityRecord Record, + ulong PositionAuthorityVersion, + ulong SpatialAuthorityVersion, + ulong VelocityAuthorityVersion, + double GameTime, + bool PreviousContact, + bool PreviousOnWalkable); + + private readonly Stack _collisionCallbackContexts + = new(4); + private readonly Func + _handleSetPositionCollisionsCallback; + + // C2: retired Operation instances wait here for reuse by + // BeginAcceptedPlacementCore instead of a fresh `new Operation` every + // accepted placement. Bounded so a pathological retirement/rent + // imbalance (e.g. many entities despawning with none spawning) cannot + // grow this into an unbounded retained cache - beyond the cap we simply + // let the retired instance become garbage, exactly like before pooling + // existed. + private const int MaxPooledOperations = 64; + private readonly Stack _operationPool = new(); + internal RuntimeSetPositionState( RuntimePhysicsState physics, RuntimeEntityDirectory entities) { _physics = physics ?? throw new ArgumentNullException(nameof(physics)); _entities = entities ?? throw new ArgumentNullException(nameof(entities)); + _handleSetPositionCollisionsCallback = + HandleSetPositionCollisionsCallback; + } + + /// + /// C2: the single cached delegate every accepted-placement SetPosition + /// call passes to PhysicsEngine.SetPosition in place of a fresh + /// per-call closure. Reads the innermost pushed + /// rather than capturing state + /// directly, so this delegate instance (created once in the + /// constructor) is reused for the lifetime of this owner. + /// + private bool HandleSetPositionCollisionsCallback( + PhysicsSetPositionCollisionReport report) + { + CollisionCallbackContext context = _collisionCallbackContexts.Peek(); + return _physics.HandleSetPositionCollisions( + context.Record, + context.PositionAuthorityVersion, + context.SpatialAuthorityVersion, + context.VelocityAuthorityVersion, + context.GameTime, + context.PreviousContact, + context.PreviousOnWalkable, + report); + } + + /// + /// C2: returns a pooled instance reset to inert defaults right here (NOT + /// at retirement - see for why that + /// ordering matters), or allocates a fresh instance exactly as + /// BeginAcceptedPlacementCore did before pooling existed. Callers + /// MUST set every field they previously set via object-initializer + /// syntax - a rented instance's unset fields are the SAME defaults a + /// brand-new instance would have (see + /// ), never leftover + /// state from a prior use. + /// + private Operation RentOperation() + { + if (_operationPool.Count == 0) + return new Operation(); + Operation pooled = _operationPool.Pop(); + pooled.ResetAllFieldsToDefault(); + return pooled; + } + + /// + /// C2: the only place an Operation is retired to the pool - called the + /// instant one is removed from for good. + /// Deliberately does NOT reset the instance here (that happens in + /// instead, right before reuse): a reentrant + /// callback chain can retire the operation the OUTER frame is still + /// executing inside of and needs to keep reading. + /// + /// Round 3 correction: an earlier revision of this comment claimed + /// IsCanonicalPlacementCommitCurrent additionally compared the + /// operation's Token to detect exactly this recycling. That claim was + /// wrong and the check it described was reverted - retail's + /// SetPositionInternal settle is UNCONDITIONAL, so an in-flight + /// ground-edge commit for an entity a reentrant cancel-then-begin has + /// since displaced must still complete its physical settle (contact + /// transition, collision reports, shadow sync) - see + /// ReentrantGroundEdgePlacementCannotBeCancelledByDisplacedOperation. A + /// Token/identity gate on the settle path itself would incorrectly abort + /// that commit the moment the entity is displaced. + /// + /// The ACTUAL safety mechanism, class-wide, is captured-token-vs- + /// fresh-lookup at every frame that holds an Operation reference across + /// a reentrancy point (a synchronous publish, a collision-report + /// dispatch that can reach an arbitrary + /// IRuntimeCollisionReportObserver, or a ground-edge HitGround/ + /// LeaveGround callback): capture operation.Token (globally + /// unique - checked(++_nextOperationId) is never reissued) into a + /// local BEFORE the reentrancy point, then after it re-resolve via a + /// fresh _operations.TryGetValue and compare + /// current.Token == capturedToken - never + /// ReferenceEquals/IsCurrent(Operation) against the + /// original reference, which becomes a tautology once pooling can hand + /// that SAME physical instance back out for a different logical + /// operation. This identity gate belongs at publication/cancellation/ + /// ownership decisions (see the token comparisons in + /// BeginAcceptedPlacementCore, SubmitPreparedPlacementCore, + /// RetryDeferred, and CommitCanonical's own bookkeeping- + /// write gate), never inside the settle/currency layer itself. + /// + /// F3 reorders BeginAcceptedPlacementCore to rent only AFTER this method + /// retires the displaced operation - every field this class still needs + /// from a displaced operation is captured into a local before that + /// retire point (never read from the possibly-the-same, freshly-reset + /// instance after), so it is safe, and more efficient, for a single- + /// entity churn cycle to hand the SAME instance right back out as the + /// next operation (LIFO). guards the one + /// invariant that ordering depends on: this method must never be called + /// twice for the same instance without an intervening + /// in between - that would silently + /// duplicate the instance in , so it throws + /// instead. + /// + private void RetireOperationToPool(Operation operation) + { + if (operation.InPool) + { + throw new InvalidOperationException( + "Operation was already retired to the pool - a double-retire " + + "without an intervening rent would duplicate it in the pool " + + "stack."); + } + if (_operationPool.Count >= MaxPooledOperations) + return; + operation.InPool = true; + _operationPool.Push(operation); } internal RuntimeSetPositionOwnershipSnapshot CaptureOwnership() @@ -501,7 +757,8 @@ internal sealed class RuntimeSetPositionState : IDisposable _placementCompletionWatches.Count, _acknowledgedPlacementCompletions.Count, _collisionPrefixQuiescence.Count, - pendingQuiescenceProjections); + pendingQuiescenceProjections, + _operationPool.Count); } internal int PendingProjectionCount => _pendingProjection.Count; @@ -922,6 +1179,8 @@ internal sealed class RuntimeSetPositionState : IDisposable in RuntimeEntityPlacementToken token) { EnsureNotDisposed(); + // Round 3 audit: safe - fresh lookup + Token check on the SAME line + // immediately precede IsCurrent, nothing reentrant in between. return token.IsValid && _operations.TryGetValue(token.Entity, out Operation? operation) && operation.Token == token @@ -998,7 +1257,11 @@ internal sealed class RuntimeSetPositionState : IDisposable { return default; } - return CancelCore(operation); + // Round 3: `token` is this call's own parameter, verified fresh + // against `_operations` immediately above with nothing reentrant in + // between - passing it straight through is equivalent to (and safer + // than) re-deriving it from `operation`. + return CancelCore(token.Entity, token); } internal RuntimeEntityPlacementToken TryBeginExclusiveAuthoredPlacement( @@ -1031,6 +1294,8 @@ internal sealed class RuntimeSetPositionState : IDisposable EnsureNotDisposed(); ArgumentNullException.ThrowIfNull(record); ArgumentNullException.ThrowIfNull(body); + // Round 3 audit: safe - fresh lookup + Token check earlier in this + // SAME guard clause precede IsCurrent, nothing reentrant in between. if (!token.IsValid || record.Key != token.Entity || !_operations.TryGetValue(token.Entity, out Operation? operation) @@ -1088,28 +1353,19 @@ internal sealed class RuntimeSetPositionState : IDisposable captureMoverPreparationAuthority ? RuntimeEntityPlacementPreparationKind.AuthoredMover : RuntimeEntityPlacementPreparationKind.LegacyDirect); - var replacement = new Operation - { - Record = record, - Token = token, - Key = key, - PositionAuthorityVersion = expectedPositionAuthorityVersion, - SessionLifetimeVersion = _entities.SessionLifetimeVersion, - SourceSpatialAuthorityVersion = record.SpatialAuthorityVersion, - SourceVelocityAuthorityVersion = record.VelocityAuthorityVersion, - PreviousContact = record.PhysicsBody?.InContact ?? false, - PreviousOnWalkable = record.PhysicsBody?.OnWalkable ?? false, - Command = default, - Result = default, - SpatialAuthorityVersion = record.SpatialAuthorityVersion, - PlacementCommitVersion = record.PlacementCommitVersion, - Stage = RuntimeEntityPlacementStage.AwaitingPreparation, - Kind = kind, - Portal = portal, - }; List? inheritedLostFamily = null; RuntimePlacementProjectionSnapshot? inheritedWithdrawal = null; bool inheritedWithdrawalAcknowledged = false; + // F3: captured into locals (rather than re-read from `displaced` + // after CancelCoreDeferred below) because `replacement` is + // deliberately rented AFTER that retire - see the no-self-aliasing + // note below and RetireOperationToPool's doc comment. Reading every + // scalar this method still needs before the retire point keeps this + // correct regardless of which physical instance `replacement` ends + // up being. + PhysicsSetPositionResult inheritedResult = default; + uint inheritedExactCellId = 0u; + ulong inheritedCollisionGeneration = 0UL; if (_operations.TryGetValue(key, out Operation? displaced) && (displaced.WakeableLostCell || displaced.InheritedLostDeadline)) @@ -1118,6 +1374,9 @@ internal sealed class RuntimeSetPositionState : IDisposable displaced.LostFamilyKeys = null; inheritedWithdrawalAcknowledged = displaced.WithdrawalAcknowledged; + inheritedResult = displaced.Result; + inheritedExactCellId = displaced.ExactCellId; + inheritedCollisionGeneration = displaced.CollisionGeneration; if (displaced.ProjectionSequence != 0UL && _pendingProjection.TryGetValue( displaced.ProjectionSequence, @@ -1134,6 +1393,36 @@ internal sealed class RuntimeSetPositionState : IDisposable cancelLostFamily: false, preserveLostFamily: inheritedLostFamily is not null, out RuntimePlacementProjectionSnapshot? discard); + + // F3: rent only AFTER the retire above (not before, as the C2 + // landing originally had it). Every field this method needs from a + // displaced operation was already captured into the locals above, + // so it is both SAFE and efficient for a single-entity churn cycle + // to hand that exact instance right back out here (the pool is + // LIFO) instead of allocating or drawing a different pooled + // instance - a fresh RentOperation() call always starts from + // `ResetAllFieldsToDefault`'s inert state regardless of which + // instance it returns. + Operation replacement = RentOperation(); + replacement.Record = record; + replacement.Token = token; + replacement.Key = key; + replacement.PositionAuthorityVersion = expectedPositionAuthorityVersion; + replacement.SessionLifetimeVersion = _entities.SessionLifetimeVersion; + replacement.SourceSpatialAuthorityVersion = + record.SpatialAuthorityVersion; + replacement.SourceVelocityAuthorityVersion = + record.VelocityAuthorityVersion; + replacement.PreviousContact = record.PhysicsBody?.InContact ?? false; + replacement.PreviousOnWalkable = + record.PhysicsBody?.OnWalkable ?? false; + replacement.Command = default; + replacement.Result = default; + replacement.SpatialAuthorityVersion = record.SpatialAuthorityVersion; + replacement.PlacementCommitVersion = record.PlacementCommitVersion; + replacement.Stage = RuntimeEntityPlacementStage.AwaitingPreparation; + replacement.Kind = kind; + replacement.Portal = portal; replacement.LostFamilyKeys = inheritedLostFamily; replacement.InheritedLostDeadline = inheritedLostFamily is not null; replacement.WithdrawalAcknowledged = inheritedWithdrawalAcknowledged; @@ -1141,10 +1430,9 @@ internal sealed class RuntimeSetPositionState : IDisposable { replacement.ProjectionSequence = retainedWithdrawal.Token.Sequence; - replacement.Result = displaced!.Result; - replacement.ExactCellId = displaced.ExactCellId; - replacement.CollisionGeneration = - displaced.CollisionGeneration; + replacement.Result = inheritedResult; + replacement.ExactCellId = inheritedExactCellId; + replacement.CollisionGeneration = inheritedCollisionGeneration; } _operations[key] = replacement; if (captureMoverPreparationAuthority) @@ -1156,7 +1444,26 @@ internal sealed class RuntimeSetPositionState : IDisposable } if (discard is { } cancelled) PublishPlacement(cancelled); - return IsCurrent(replacement) ? token : default; + // F3: deliberately does NOT use `IsCurrent(replacement)` here. + // `PublishPlacement` above can synchronously notify a subscriber + // that reentrantly calls BeginAcceptedPlacement for the SAME entity + // (see ReentrantBeginDuringDiscardCannotBeOverwrittenByOuterBegin) - + // that reentrant call retires `replacement` and, per this method's + // rent-after-retire ordering, can rent it right back out (LIFO) for + // the INNER operation. `replacement` (the physical object) would + // then read as the inner operation's Key/Token, and comparing it + // against itself via `_operations.TryGetValue(replacement.Key, ...) + // && ReferenceEquals(current, replacement)` is a tautology - it + // would report "still current" even though THIS (outer) Begin + // invocation was clearly superseded. `token` was captured fresh at + // the top of this call, before any reentrancy could touch it, so + // comparing it against whatever now actually owns the key correctly + // answers "is my own invocation still canonical" regardless of + // what happened to `replacement` in between. + return _operations.TryGetValue(key, out Operation? currentOperation) + && currentOperation.Token == token + ? token + : default; } private bool HasRetainedCompletion(RuntimeEntityKey key) @@ -1177,6 +1484,8 @@ internal sealed class RuntimeSetPositionState : IDisposable { EnsureNotDisposed(); command = default; + // Round 3 audit: safe - fresh lookup + Token check earlier in this + // SAME guard clause precede IsCurrent, nothing reentrant in between. if (!token.IsValid || !_operations.TryGetValue(token.Entity, out Operation? operation) || operation.Token != token @@ -1322,6 +1631,9 @@ internal sealed class RuntimeSetPositionState : IDisposable { EnsureNotDisposed(); ArgumentNullException.ThrowIfNull(record); + // Round 3 audit: safe - fresh lookup + Token check earlier in this + // SAME return expression precede IsCurrent, nothing reentrant in + // between. return token.IsValid && token.Entity == record.Key && _operations.TryGetValue(token.Entity, out Operation? operation) @@ -1396,6 +1708,13 @@ internal sealed class RuntimeSetPositionState : IDisposable ulong objectTableBindingAuthority = _physics.ObjectTableBindingAuthority; ulong objectTableAuthority = objectTable?.MutationRevision ?? 0UL; + // Round 3 audit: safe - `handleCollisions: null` means this call + // never invokes HandleSetPositionCollisions/HandleReports/ + // ReportEnvironment (no collision-report observer can be reached), + // and this dormant-activation path never calls CommitCanonical (so + // no ground-edge HitGround/LeaveGround dispatch either) - there is + // no reentrancy point between `operation` being obtained above and + // the ReferenceEquals check below. PhysicsSetPositionResult result = _physics.Engine.SetPosition( canonicalRequest, handleCollisions: null); @@ -1836,6 +2155,12 @@ internal sealed class RuntimeSetPositionState : IDisposable SetPositionCollisionBatchDispatchResult dispatch = _physics .CollisionReports.DispatchSetPositionBatchResult(receipt.Collision); bool reported = dispatch.Reported; + // Round 3 audit: safe - fresh lookup + Token.OperationId check + // earlier in this SAME guard clause precede IsCurrent, nothing + // reentrant in between. This dormant-activation family has zero + // production callers and never routes through CommitCanonical's + // ground-edge callback or the live collision-report dispatch that + // can reach an arbitrary observer. if (receipt.Status is RuntimeDormantSetPositionCommitStatus.RejectedPlacement && _operations.TryGetValue(receipt.Entity, out Operation? operation) @@ -1860,6 +2185,10 @@ internal sealed class RuntimeSetPositionState : IDisposable PhysicsBody body, in RuntimeDormantSetPositionCommitReceipt receipt) { + // Round 3 audit: safe - fresh lookup + Token.OperationId check + // earlier in this SAME return expression precede IsCurrent, nothing + // reentrant in between; this dormant family has zero production + // callers. return receipt.Status is RuntimeDormantSetPositionCommitStatus .AwaitingFinalShadowPreparation && _operations.TryGetValue(receipt.Entity, out Operation? operation) @@ -1898,6 +2227,10 @@ internal sealed class RuntimeSetPositionState : IDisposable if (receipt.Status is RuntimeDormantSetPositionCommitStatus .AwaitingFinalShadowPreparation) return IsDormantLocalActivationPrephaseCurrent(record, body, receipt); + // Round 3 audit: safe - fresh lookup + Token.OperationId check + // earlier in this SAME return expression precede IsCurrent, nothing + // reentrant in between; this dormant family has zero production + // callers. return receipt.Status is RuntimeDormantSetPositionCommitStatus .RejectedPlacement && _operations.TryGetValue(receipt.Entity, out Operation? operation) @@ -1938,6 +2271,15 @@ internal sealed class RuntimeSetPositionState : IDisposable PhysicsBody body, in RuntimeDormantSetPositionCommitReceipt receipt) { + // Round 3 audit: safe - fresh lookup + Token.OperationId check + // immediately precede IsCurrent, nothing reentrant in between; this + // dormant family has zero production callers. Nothing between here + // and the final `IsCurrent(operation)` below invokes anything + // reentrant either (IsVelocityCurrent/HandleAllCollisions/ + // CommitStationaryBits are pure PhysicsObjUpdate calls on `body`, + // and the nested IsDormantLocalActivationPrephaseCurrent call is + // itself a fresh-lookup check), so the SAME verified `operation` + // reference remains valid through the final check. if (!_operations.TryGetValue(receipt.Entity, out Operation? operation) || operation.Token.OperationId != receipt.OperationId || !IsCurrent(operation) @@ -2115,7 +2457,10 @@ internal sealed class RuntimeSetPositionState : IDisposable if (_operations.TryGetValue(receipt.Entity, out Operation? operation) && operation.Token.OperationId == receipt.OperationId) { - _ = CancelCore(operation); + // Round 3: `operation.Token` is read from the fresh lookup + // immediately above, with nothing reentrant in between - safe + // to pass straight through as the captured token. + _ = CancelCore(receipt.Entity, operation.Token); } } @@ -2131,7 +2476,10 @@ internal sealed class RuntimeSetPositionState : IDisposable { return; } - _ = CancelCore(operation); + // Round 3: `token` is this call's own parameter, verified fresh + // against `_operations` immediately above with nothing reentrant in + // between. + _ = CancelCore(token.Entity, token); } internal bool IsDormantLocalActivationCommitCurrent( @@ -2139,6 +2487,9 @@ internal sealed class RuntimeSetPositionState : IDisposable PhysicsBody body, in RuntimeDormantSetPositionCommitReceipt receipt) { + // Round 3 audit: safe - fresh lookup + Stage/ProjectionSequence + // check earlier in this SAME guard clause precede IsCurrent, + // nothing reentrant in between. if (!receipt.IsCommitted || !receipt.Projection.Token.IsValid || record.Key != receipt.Projection.Token.Entity @@ -2246,6 +2597,11 @@ internal sealed class RuntimeSetPositionState : IDisposable token.Entity, out exactAuthority) && exactAuthority.OperationId == token.OperationId; + // Round 3 audit: safe - `ownsToken` (fresh lookup + Token check) + // was just computed above with nothing reentrant in between; this + // is the function's own entry validation, before any of its + // internal reentrancy points (the SetPosition/CommitCanonical calls + // further down, which ARE converted to IsCurrentByToken). if (!ownsToken || operation is null || operation.Stage @@ -2399,19 +2755,34 @@ internal sealed class RuntimeSetPositionState : IDisposable quiescence.Token.LandblockPrefix); } - PhysicsSetPositionResult result = - _physics.Engine.SetPosition( + PhysicsSetPositionResult result; + _collisionCallbackContexts.Push(new CollisionCallbackContext( + operation.Record, + operation.PositionAuthorityVersion, + operation.SourceSpatialAuthorityVersion, + operation.SourceVelocityAuthorityVersion, + canonicalCommand.GameTime, + operation.PreviousContact, + operation.PreviousOnWalkable)); + try + { + result = _physics.Engine.SetPosition( canonicalRequest, - report => _physics.HandleSetPositionCollisions( - operation.Record, - operation.PositionAuthorityVersion, - operation.SourceSpatialAuthorityVersion, - operation.SourceVelocityAuthorityVersion, - canonicalCommand.GameTime, - operation.PreviousContact, - operation.PreviousOnWalkable, - report)); - if (!IsCurrent(operation)) + _handleSetPositionCollisionsCallback); + } + finally + { + _collisionCallbackContexts.Pop(); + } + // Round 3: `_physics.Engine.SetPosition` above can reenter this class + // - its collision-report callback can reach an arbitrary + // IRuntimeCollisionReportObserver subscriber that calls back into + // Begin/Cancel for this (or any) entity, which can retire-then-rent + // (LIFO) this exact `operation` instance for a different logical + // operation. `token` (the function parameter, captured before any + // of this ran) proves identity by value instead of trusting + // `operation`'s live fields. + if (!IsCurrentByToken(token.Entity, token, out operation)) return Outcome(RuntimeSetPositionStatus.Cancelled, result, default); if (result.IsSuccessful && TryGetBlockingQuiescence( @@ -2442,7 +2813,10 @@ internal sealed class RuntimeSetPositionState : IDisposable return Outcome(RuntimeSetPositionStatus.Rejected, result, default); } - if (!IsCurrent(operation)) + // Round 3: same reentrancy hazard as the check right after the + // SetPosition call above - re-verify by token rather than trusting + // `operation` across the collision-report dispatch. + if (!IsCurrentByToken(token.Entity, token, out operation)) return Outcome(RuntimeSetPositionStatus.Cancelled, result, default); operation.RequiresPreparation = false; operation.ExactCellId = result.CellId; @@ -2453,7 +2827,29 @@ internal sealed class RuntimeSetPositionState : IDisposable if (!CommitCanonical(operation, result)) { - PublishCancellation(CancelCore(operation)); + // Round 3: CommitCanonical's own ground-edge callback and + // collision-report dispatch can reenter this class, so + // `operation` may already be stale here even though it was + // just re-verified before the call. `token` (this function's + // own parameter, untouched since entry) is the safe capture. + PublishCancellation(CancelCore(token.Entity, token)); + return Outcome(RuntimeSetPositionStatus.Cancelled, result, default); + } + + // F1: CommitCanonical can succeed (fully applying the physical + // settle - contact transition, collision reports, shadow sync) for + // an operation a reentrant ground-edge callback has since displaced + // - retail lets that physical commit land regardless (see + // ReentrantGroundEdgePlacementCannotBeCancelledByDisplacedOperation). + // But THIS caller's own operation is no longer canonical, so it must + // still report Cancelled rather than publish a Place projection + // nothing will ever acknowledge again. `token` (the parameter, + // captured before any of this ran) is compared against a fresh + // lookup instead of trusting `operation`'s live fields, which may + // already reflect whatever displaced it. + if (!_operations.TryGetValue(token.Entity, out Operation? stillOwns) + || stillOwns.Token != token) + { return Outcome(RuntimeSetPositionStatus.Cancelled, result, default); } @@ -2469,6 +2865,38 @@ internal sealed class RuntimeSetPositionState : IDisposable projection); } + /// + /// C2: zero-allocation replacement for the LINQ + /// _pendingProjection.First() pattern used at every call site + /// below. Enumerable.First<TSource> takes an + /// IEnumerable<TSource> parameter, so calling it on a + /// dispatches through the + /// interface-typed IEnumerable<KeyValuePair<TKey, + /// TValue>>.GetEnumerator(), which BOXES the dictionary's + /// normally-struct Enumerator - this was the entire measured C2 + /// acknowledgement-path residual (120 B/op). A plain foreach on + /// the concrete field type + /// resolves to its public non-interface, struct-returning + /// GetEnumerator() instead and never boxes. Every call site below + /// already checks _pendingProjection.Count != 0 immediately + /// before calling this (short-circuiting `||`/`&&`), exactly + /// mirroring the precondition LINQ's First() relied on - the + /// throw path is unreachable in current usage, kept only so a future + /// caller that skips the guard fails loudly instead of silently, same as + /// LINQ's own contract would have. + /// + private KeyValuePair + FirstPendingProjection() + { + foreach (KeyValuePair entry + in _pendingProjection) + { + return entry; + } + throw new InvalidOperationException( + "FirstPendingProjection requires at least one pending entry."); + } + internal bool TryPeekProjection( out RuntimePlacementProjectionSnapshot projection) { @@ -2478,7 +2906,7 @@ internal sealed class RuntimeSetPositionState : IDisposable projection = default; return false; } - projection = _pendingProjection.First().Value; + projection = FirstPendingProjection().Value; return true; } @@ -2488,7 +2916,7 @@ internal sealed class RuntimeSetPositionState : IDisposable EnsureNotDisposed(); if (!token.IsValid || _pendingProjection.Count == 0 - || _pendingProjection.First().Key != token.Sequence + || FirstPendingProjection().Key != token.Sequence || !_pendingProjection.TryGetValue( token.Sequence, out RuntimePlacementProjectionSnapshot pending) @@ -2516,6 +2944,9 @@ internal sealed class RuntimeSetPositionState : IDisposable } return true; } + // Round 3 audit: safe - fresh lookup + ProjectionSequence check + // immediately precede IsCurrent, nothing reentrant in between (this + // is AcknowledgeProjection's own entry validation). if (!_operations.TryGetValue( token.Entity, out Operation? operation) @@ -2540,13 +2971,19 @@ internal sealed class RuntimeSetPositionState : IDisposable } operation.Stage = RuntimeEntityPlacementStage.AwaitingCommitAcknowledgement; _moverPreparationAuthorities.Remove(operation.Key); - return _operations.Remove(operation.Key); + bool removed = _operations.Remove(operation.Key); + if (removed) + RetireOperationToPool(operation); + return removed; } if (!operation.WakeableLostCell && !operation.InheritedLostDeadline) { _moverPreparationAuthorities.Remove(operation.Key); - return _operations.Remove(operation.Key); + bool removed = _operations.Remove(operation.Key); + if (removed) + RetireOperationToPool(operation); + return removed; } operation.WithdrawalAcknowledged = true; @@ -2649,9 +3086,21 @@ internal sealed class RuntimeSetPositionState : IDisposable } var operation = CreateWithdrawalOperation(record, key); _operations[key] = operation; + // Round 3: `operation.Token` is captured HERE, before + // PublishPlacement below can synchronously reenter this class - a + // subscriber reacting to the withdrawal-of-the-old-operation + // notification can call Begin/Cancel for this SAME entity, which + // (per RetireOperationToPool's doc comment) can retire-then-rent + // (LIFO) this exact `operation` instance for a brand-new logical + // operation before the checks below run. Comparing + // `ReferenceEquals`/`IsCurrent(operation)` against the stale + // reference afterward would be a tautology - it would report "still + // current" and then publish a Withdraw projection carrying the + // NEWER operation's state under the OLD operation's identity. + RuntimeEntityPlacementToken capturedToken = operation.Token; if (discard is { } cancelledOld) PublishPlacement(cancelledOld); - if (!IsCurrent(operation)) + if (!IsCurrentByToken(key, capturedToken, out operation)) return true; _ = PublishProjection( operation, @@ -2821,33 +3270,39 @@ internal sealed class RuntimeSetPositionState : IDisposable RuntimeSetPositionOperationKind.RemoteAuthoritative, body.LastUpdateTime, record.VelocityAuthorityVersion); - var operation = new Operation - { - Record = record, - Body = body, - Token = new RuntimeEntityPlacementToken( - _entities.SessionLifetimeVersion, - key, - record.PositionAuthorityVersion, - checked(++_nextOperationId), - RuntimeEntityPlacementPreparationKind.AuthoredMover), - Key = key, - PositionAuthorityVersion = record.PositionAuthorityVersion, - SessionLifetimeVersion = _entities.SessionLifetimeVersion, - SourceSpatialAuthorityVersion = record.SpatialAuthorityVersion, - SourceVelocityAuthorityVersion = record.VelocityAuthorityVersion, - PreviousContact = body.InContact, - PreviousOnWalkable = body.OnWalkable, - Command = command, - Result = result, - SpatialAuthorityVersion = record.SpatialAuthorityVersion, - PlacementCommitVersion = record.PlacementCommitVersion, - ExactCellId = cellId, - Stage = RuntimeEntityPlacementStage.AwaitingPreparation, - Kind = RuntimeSetPositionOperationKind.RemoteAuthoritative, - Portal = default, - RequiresPreparation = !hasPrepared, - }; + // F4: routed through RentOperation() (rather than a fresh + // `new Operation { ... }`) so every construction flows one path + // - see ResetAllFieldsToDefault's completeness-net doc comment. + // No key/displaced-operation collision is possible here: the + // loop above already skips this record when + // `_operations.ContainsKey(key)`. + Operation operation = RentOperation(); + operation.Record = record; + operation.Body = body; + operation.Token = new RuntimeEntityPlacementToken( + _entities.SessionLifetimeVersion, + key, + record.PositionAuthorityVersion, + checked(++_nextOperationId), + RuntimeEntityPlacementPreparationKind.AuthoredMover); + operation.Key = key; + operation.PositionAuthorityVersion = record.PositionAuthorityVersion; + operation.SessionLifetimeVersion = _entities.SessionLifetimeVersion; + operation.SourceSpatialAuthorityVersion = + record.SpatialAuthorityVersion; + operation.SourceVelocityAuthorityVersion = + record.VelocityAuthorityVersion; + operation.PreviousContact = body.InContact; + operation.PreviousOnWalkable = body.OnWalkable; + operation.Command = command; + operation.Result = result; + operation.SpatialAuthorityVersion = record.SpatialAuthorityVersion; + operation.PlacementCommitVersion = record.PlacementCommitVersion; + operation.ExactCellId = cellId; + operation.Stage = RuntimeEntityPlacementStage.AwaitingPreparation; + operation.Kind = RuntimeSetPositionOperationKind.RemoteAuthoritative; + operation.Portal = default; + operation.RequiresPreparation = !hasPrepared; _operations.Add(key, operation); _moverPreparationAuthorities[key] = CapturePreparationAuthority( operation, @@ -2970,7 +3425,7 @@ internal sealed class RuntimeSetPositionState : IDisposable private bool HasPendingProjectionThrough(ulong barrierSequence) => barrierSequence != 0UL && _pendingProjection.Count != 0 - && _pendingProjection.First().Key <= barrierSequence; + && FirstPendingProjection().Key <= barrierSequence; private bool HasCollisionDispatchDebt() { @@ -3153,8 +3608,27 @@ internal sealed class RuntimeSetPositionState : IDisposable bool ready, bool releaseUnavailable = false) { - foreach (Operation operation in _operations.Values.ToArray()) + // Round 3: snapshot (Key, Token) PAIRS, not Operation references. + // This loop's own RetryDeferred call below can reenter this class + // (CommitCanonical's ground-edge callbacks, or an arbitrary + // collision-report observer reentrantly calling Begin/Cancel) and + // retire-then-rent (LIFO) a LATER array entry's physical instance + // for a brand-new operation before this loop ever reaches it - a + // stale `Operation` reference captured once at the top (the + // pre-round-3 `.ToArray()` shape) would then silently read/mutate + // the wrong logical operation. Re-resolving by captured Token at + // the top of every iteration (before touching any field) detects + // that instead. + var snapshot = + new List<(RuntimeEntityKey Key, RuntimeEntityPlacementToken Token)>( + _operations.Count); + foreach (Operation existing in _operations.Values) + snapshot.Add((existing.Key, existing.Token)); + foreach ((RuntimeEntityKey key, RuntimeEntityPlacementToken capturedToken) + in snapshot) { + if (!IsCurrentByToken(key, capturedToken, out Operation? operation)) + continue; bool unavailableAfterReadyCommit = ready && operation.CollisionQuiescenceHeld && operation.CollisionGeneration == 0UL @@ -3333,6 +3807,18 @@ internal sealed class RuntimeSetPositionState : IDisposable continue; } RemoveDeferredBucket(cell); + // Round 3 audit: safe - `exact` snapshots KEYS (RuntimeEntityKey + // values), never Operation references, so nothing here can go + // stale the way a snapshotted-reference loop + // (RebindQuiescedDeferredOperations, before its round-3 fix) + // could. Every iteration re-resolves `operation` via a fresh + // `_operations.TryGetValue` and re-validates the exact + // WakeableLostCell/ExactCellId/CollisionPrefix/ + // CollisionGeneration shape before acting - a reentrant + // cancel-then-begin from an earlier iteration's RetryDeferred + // call (itself converted to IsCurrentByToken) is either not + // found at all or correctly rejected by this shape check for + // any later iteration touching the same or a different entity. foreach (RuntimeEntityKey entity in exact) { if (!_operations.TryGetValue( @@ -3379,6 +3865,16 @@ internal sealed class RuntimeSetPositionState : IDisposable _pendingProjection.Clear(); _expiredLostCells.Clear(); _expiredLostCellNodes.Clear(); + // F2: the C2 object pool retains full previous-generation entity + // graphs (Record -> Snapshot/PhysicsBody/clock/host references) + // through every pooled instance until it is rented and reset. Both + // callers of this method (ResetSession and Dispose) end the session + // those graphs belonged to, so nothing can still be mid-rent across + // this clear - unlike RetireOperationToPool/RentOperation, which + // must worry about a reentrant frame still executing inside a + // ground-edge callback, a session clear cannot be reentered from + // inside itself. + _operationPool.Clear(); } private RuntimeSetPositionOutcome ParkDeferred( @@ -3472,6 +3968,21 @@ internal sealed class RuntimeSetPositionState : IDisposable projection); } + /// + /// Round 3 audit: the entry check + /// below is safe (not converted to ) + /// because every one of this method's 4 call sites passes an + /// reference obtained via a fresh + /// _operations lookup (or an call) + /// with NOTHING reentrant executed between that lookup and this call - + /// AcknowledgeProjection and SubmitPreparedPlacementCore + /// call it immediately after their own entry validation, the + /// exact-cell-ready loop re-resolves by key every iteration and rejects + /// a repurposed operation via its WakeableLostCell/ExactCellId/ + /// CollisionPrefix/CollisionGeneration shape, and + /// RebindQuiescedDeferredOperations was converted in round 3 to + /// call immediately before this method. + /// private void RetryDeferred(Operation operation) { // The local-player activation lease owns its dormant body/controller @@ -3494,6 +4005,14 @@ internal sealed class RuntimeSetPositionState : IDisposable if (!IsDeferredWakePreparationCurrent(operation)) return; + // F1: hoisted before the ground-edge-callback-bearing + // CommitCanonical call below (same reasoning as + // SubmitPreparedPlacementCore's `token` parameter) - lets the + // post-CommitCanonical check confirm THIS operation is still + // canonical without trusting `operation`'s live fields, which a + // reentrant cancel-then-begin during the callback may have already + // repurposed. + RuntimeEntityPlacementToken operationToken = operation.Token; RuntimeCollisionPrefixQuiescenceToken restoringQuiescence = default; if (TryGetBlockingQuiescence( operation.Command.Physics, @@ -3531,23 +4050,49 @@ internal sealed class RuntimeSetPositionState : IDisposable operation.Command.GameTime), }; RebindPreparedCommand(operation); - PhysicsSetPositionResult result = IsStructurallyValid( - operation.Command.Physics) - ? _physics.Engine.SetPosition( - operation.Command.Physics, - report => _physics.HandleSetPositionCollisions( - operation.Record, - operation.PositionAuthorityVersion, - operation.SpatialAuthorityVersion, - operation.SourceVelocityAuthorityVersion, - operation.Command.GameTime, - operation.PreviousContact, - operation.PreviousOnWalkable, - report)) - : InvalidResult(operation.Command.Physics); + PhysicsSetPositionResult result; + if (IsStructurallyValid(operation.Command.Physics)) + { + _collisionCallbackContexts.Push(new CollisionCallbackContext( + operation.Record, + operation.PositionAuthorityVersion, + operation.SpatialAuthorityVersion, + operation.SourceVelocityAuthorityVersion, + operation.Command.GameTime, + operation.PreviousContact, + operation.PreviousOnWalkable)); + try + { + result = _physics.Engine.SetPosition( + operation.Command.Physics, + _handleSetPositionCollisionsCallback); + } + finally + { + _collisionCallbackContexts.Pop(); + } + } + else + { + result = InvalidResult(operation.Command.Physics); + } operation.CollisionGenerationReady = false; - if (!IsCurrent(operation)) + // Round 3: same reentrancy hazard as SubmitPreparedPlacementCore's + // post-SetPosition checks - the collision-report callback above can + // reach an arbitrary observer that calls back into Begin/Cancel and + // retires-then-rents (LIFO) this exact instance. `operationToken` + // was hoisted before the SetPosition call. (`operation` is a plain + // non-nullable parameter here, not a nullable local like + // SubmitPreparedPlacementCore's - route through a temporary so the + // NotNullWhen-proven reference can be assigned back to it.) + if (!IsCurrentByToken( + operationToken.Entity, + operationToken, + out Operation? refreshed)) + { return; + } + operation = refreshed; if (result.IsSuccessful && TryGetBlockingQuiescence( result, @@ -3605,12 +4150,47 @@ internal sealed class RuntimeSetPositionState : IDisposable } return; } - if (!IsCurrent(operation)) + // Round 3: nothing reentrant runs between here and the previous + // token check (only quiescence/deferred bookkeeping branches, all + // of which return before reaching this point) - re-verifying by + // token again anyway keeps this in lockstep with the same pattern + // used everywhere else in this method, and `operation` is already + // the freshly-verified reference from that check. + if (!IsCurrentByToken( + operationToken.Entity, + operationToken, + out Operation? stillCurrent)) + { return; + } + operation = stillCurrent; _preparedMovers[operation.Key] = operation.Command.Physics; if (!CommitCanonical(operation, result)) { - PublishCancellation(CancelCore(operation)); + // Round 3: CommitCanonical's own ground-edge callback and + // collision-report dispatch can reenter this class, so + // `operation` may already be stale here even though it was + // just re-verified before the call. + PublishCancellation(CancelCore(operationToken.Entity, operationToken)); + return; + } + + // F1: CommitCanonical can succeed (fully applying the physical + // settle) for an operation a reentrant ground-edge callback has + // since displaced - retail lets that physical commit land + // regardless (see + // ReentrantGroundEdgePlacementCannotBeCancelledByDisplacedOperation). + // But if THIS operation is no longer canonical, publishing a Place + // projection for it is wrong - nothing will ever acknowledge it + // again, and `operation`'s own fields may already reflect whatever + // displaced it. `operationToken` (hoisted before the callback) is + // compared against a fresh lookup rather than trusting `operation` + // itself. + if (!_operations.TryGetValue( + operationToken.Entity, + out Operation? stillOwns) + || stillOwns.Token != operationToken) + { return; } @@ -3626,10 +4206,49 @@ internal sealed class RuntimeSetPositionState : IDisposable Operation operation, in PhysicsSetPositionResult result) { + // Round 3 audit: safe - this is the function's own entry + // validation; every caller (SubmitPreparedPlacementCore, + // RetryDeferred) passes an `operation` obtained via + // IsCurrentByToken/IsCurrent immediately before calling + // CommitCanonical, with nothing reentrant in between. if (!result.IsCommitted || !IsCurrent(operation)) return false; RuntimeEntityRecord record = operation.Record; PhysicsBody body = operation.Body!; + // F1: every operation-derived scalar this method still needs AFTER + // invoking the ground-edge HitGround/LeaveGround callbacks below is + // captured into a local HERE, before those callbacks run - retail's + // own savedTransientState pattern (pseudo-C 283952 stacks the exact + // same bits before handle_all_collisions). A synchronous ground-edge + // chain that cancels then begins (or begins twice) for the SAME + // entity can retire this exact `operation` instance to the pool and + // rent it right back out (LIFO) for a DIFFERENT logical operation - + // reading `operation`'s live fields after that point would silently + // observe the wrong operation's state. The SETTLE below (contact + // transition, collision reports, shadow sync, and the currency + // checks gating them) deliberately does NOT hoist/compare the + // operation's Token: retail intentionally lets an in-flight + // ground-edge commit for a DISPLACED operation still complete (see + // IsCanonicalPlacementCommitCurrent's doc comment and + // ReentrantGroundEdgePlacementCannotBeCancelledByDisplacedOperation). + // `operationToken` is hoisted anyway - not for the settle, but for + // the Runtime-owned BOOKKEEPING writes at the very end of this + // method (Round 3 addendum A1), which must land on the operation + // THIS frame actually started with, never a nested cancel-then- + // begin's freshly-begun operation that happens to share the key and + // pass the record-state checks (a bare Begin never advances + // PlacementCommitVersion, so the settle-layer checks cannot detect + // that repurposing - only Token identity can). + RuntimeEntityPlacementToken operationToken = operation.Token; + RuntimeEntityKey operationKey = operation.Key; + ulong positionAuthorityVersion = operation.PositionAuthorityVersion; + ulong sourceVelocityAuthorityVersion = + operation.SourceVelocityAuthorityVersion; + double commandGameTime = operation.Command.GameTime; + bool previousContact = operation.PreviousContact; + bool previousOnWalkable = operation.PreviousOnWalkable; + float shadowWorldOffsetX = operation.Command.ShadowWorldOffsetX; + float shadowWorldOffsetY = operation.Command.ShadowWorldOffsetY; body.Orientation = result.Orientation; body.SnapToCell( result.CellId, @@ -3638,7 +4257,7 @@ internal sealed class RuntimeSetPositionState : IDisposable bool isStatic = (record.FinalPhysicsState & PhysicsStateFlags.Static) != 0; if (operation.EnteringWorldFromCelllessResidence) { - body.LastUpdateTime = operation.Command.GameTime; + body.LastUpdateTime = commandGameTime; _entities.ResetObjectClockForEnterWorld(record, isStatic); } if (operation.EnteringWorldFromCelllessResidence && !isStatic) @@ -3664,6 +4283,7 @@ internal sealed class RuntimeSetPositionState : IDisposable (result.CellId & 0xFFFF0000u) | 0xFFFFu); } operation.SpatialAuthorityVersion = record.SpatialAuthorityVersion; + ulong spatialAuthorityVersion = record.SpatialAuthorityVersion; _entities.AdvancePlacementCommit(record); operation.PlacementCommitVersion = record.PlacementCommitVersion; ulong canonicalCommitVersion = record.PlacementCommitVersion; @@ -3681,7 +4301,8 @@ internal sealed class RuntimeSetPositionState : IDisposable System.Collections.Immutable.ImmutableArray collidedObjectIds = result.CollidedObjectIds; if (!IsCanonicalPlacementCommitCurrent( - operation, + positionAuthorityVersion, + spatialAuthorityVersion, record, body, canonicalCommitVersion, @@ -3695,13 +4316,14 @@ internal sealed class RuntimeSetPositionState : IDisposable body, result.InContact, result.OnWalkable, - operation.PreviousOnWalkable); + previousOnWalkable); } else { var guard = new ContactCommitGuard( this, - operation, + positionAuthorityVersion, + spatialAuthorityVersion, record, body, canonicalCommitVersion, @@ -3710,14 +4332,15 @@ internal sealed class RuntimeSetPositionState : IDisposable body, result.InContact, result.OnWalkable, - operation.PreviousOnWalkable, + previousOnWalkable, remote.HitGround, remote.LeaveGround, guard.IsCurrent); } if (!contactCommitted || !IsCanonicalPlacementCommitCurrent( - operation, + positionAuthorityVersion, + spatialAuthorityVersion, record, body, canonicalCommitVersion, @@ -3729,17 +4352,18 @@ internal sealed class RuntimeSetPositionState : IDisposable bool reportingCurrent = !IsCollisionReportingEligible(record, body) || _physics.HandleSetPositionCollisionReports( record, - operation.PositionAuthorityVersion, - operation.SpatialAuthorityVersion, - operation.Command.GameTime, - operation.PreviousContact, - operation.PreviousOnWalkable, + positionAuthorityVersion, + spatialAuthorityVersion, + commandGameTime, + previousContact, + previousOnWalkable, collidedWithEnvironment, collidedObjectIds, out _); if (!reportingCurrent || !IsCanonicalPlacementCommitCurrent( - operation, + positionAuthorityVersion, + spatialAuthorityVersion, record, body, canonicalCommitVersion, @@ -3747,14 +4371,14 @@ internal sealed class RuntimeSetPositionState : IDisposable requireSpatialRoot: false)) return false; body.FramesStationaryFall = result.FramesStationaryFall; - if (IsVelocityCurrent(operation)) + if (IsVelocityCurrent(sourceVelocityAuthorityVersion, record)) { PhysicsObjUpdate.HandleAllCollisions( body, result.CollisionNormalValid, result.CollisionNormal, - operation.PreviousContact, - operation.PreviousOnWalkable, + previousContact, + previousOnWalkable, body.OnWalkable); } body.TransientState &= ~(TransientStateFlags.StationaryFall @@ -3770,7 +4394,8 @@ internal sealed class RuntimeSetPositionState : IDisposable if (remote is not null) remote.Airborne = !body.OnWalkable; if (!IsCanonicalPlacementCommitCurrent( - operation, + positionAuthorityVersion, + spatialAuthorityVersion, record, body, canonicalCommitVersion, @@ -3779,33 +4404,72 @@ internal sealed class RuntimeSetPositionState : IDisposable return false; _physics.Engine.ShadowObjects.CommitSetPosition( - operation.Key.LocalEntityId, + operationKey.LocalEntityId, result.Position, result.Orientation, result.CellId, - operation.Command.ShadowWorldOffsetX, - operation.Command.ShadowWorldOffsetY, + shadowWorldOffsetX, + shadowWorldOffsetY, result.ShadowAction, result.CrossCellIds); _physics.AcknowledgeSpatialProjection(record, spatial: true); - operation.ExactCellId = result.CellId; - operation.Result = result; - operation.WakeableLostCell = false; - operation.EnteringWorldFromCelllessResidence = false; - CancelLostFamilyDeadlines(operation); - return IsCurrent(operation) - && IsCanonicalPlacementCommitCurrent( - operation, - record, - body, - canonicalCommitVersion, - committedCellId, - requireSpatialRoot: true); + // A1 (round 3 addendum): unlike the settle above, these are + // Runtime's OWN bookkeeping for which logical operation this settle + // belongs to - a nested cancel-then-begin (reachable via either the + // ground-edge callbacks above OR an arbitrary collision-report + // observer reentrantly calling Begin/Cancel from inside + // HandleSetPositionCollisionReports) can recycle `operation` for a + // brand-new operation that stages at AwaitingPreparation without + // ever advancing PlacementCommitVersion - invisible to the + // record-state checks above. Re-resolve by the token captured + // before any callback ran and only write if it is still the exact + // instance; skipping the writes (rather than failing the whole + // commit) matches retail's already-unconditional physical settle - + // only the ownership bookkeeping is conditional. + if (_operations.TryGetValue(operationKey, out Operation? currentOperation) + && currentOperation.Token == operationToken) + { + currentOperation.ExactCellId = result.CellId; + currentOperation.Result = result; + currentOperation.WakeableLostCell = false; + currentOperation.EnteringWorldFromCelllessResidence = false; + CancelLostFamilyDeadlines(currentOperation); + } + + return IsCanonicalPlacementCommitCurrent( + positionAuthorityVersion, + spatialAuthorityVersion, + record, + body, + canonicalCommitVersion, + committedCellId, + requireSpatialRoot: true); } + /// + /// F1: takes the operation's authority versions as explicit VALUES + /// (captured by the caller before invoking the ground-edge HitGround/ + /// LeaveGround callbacks) rather than reading them live off an + /// reference that a reentrant cancel-then-begin + /// (or begin-twice) chain may have already retired and repurposed for a + /// DIFFERENT logical operation. Deliberately does NOT also compare the + /// operation's Token/identity: retail intentionally lets an in-flight + /// ground-edge commit for a DISPLACED operation still complete (its + /// physical contact-transition/shadow-sync settle even though a newer + /// operation now owns future placement authority for this entity - see + /// ReentrantGroundEdgePlacementCannotBeCancelledByDisplacedOperation). + /// Adding an identity check here would incorrectly abort that commit the + /// moment the entity is displaced, which is exactly the behavior that + /// test pins as wrong. The self-aliasing hazard a Token comparison would + /// otherwise guard against is real, but belongs at the call sites that + /// need "is this SPECIFIC Begin invocation still canonical" (see the + /// local-token comparison at the end of + /// BeginAcceptedPlacementCore), not here. + /// private bool IsCanonicalPlacementCommitCurrent( - Operation operation, + ulong positionAuthorityVersion, + ulong spatialAuthorityVersion, RuntimeEntityRecord record, PhysicsBody body, ulong placementCommitVersion, @@ -3813,10 +4477,8 @@ internal sealed class RuntimeSetPositionState : IDisposable bool requireSpatialRoot) => _entities.IsCurrent(record) && ReferenceEquals(record.PhysicsBody, body) - && record.PositionAuthorityVersion - == operation.PositionAuthorityVersion - && record.SpatialAuthorityVersion - == operation.SpatialAuthorityVersion + && record.PositionAuthorityVersion == positionAuthorityVersion + && record.SpatialAuthorityVersion == spatialAuthorityVersion && record.PlacementCommitVersion == placementCommitVersion && record.FullCellId == fullCellId && (!requireSpatialRoot || _physics.IsSpatialRoot(record)); @@ -3928,46 +4590,75 @@ internal sealed class RuntimeSetPositionState : IDisposable body.CellPosition.Frame.Origin, CrossCellIds: ImmutableArray.Empty, CollidedObjectIds: ImmutableArray.Empty); - return new Operation - { - Record = record, - Body = body, - Token = new RuntimeEntityPlacementToken( - _entities.SessionLifetimeVersion, - key, - record.PositionAuthorityVersion, - checked(++_nextOperationId), - RuntimeEntityPlacementPreparationKind.LegacyDirect), - Key = key, - PositionAuthorityVersion = record.PositionAuthorityVersion, - SessionLifetimeVersion = _entities.SessionLifetimeVersion, - SourceSpatialAuthorityVersion = record.SpatialAuthorityVersion, - SourceVelocityAuthorityVersion = record.VelocityAuthorityVersion, - PreviousContact = body.InContact, - PreviousOnWalkable = body.OnWalkable, - Command = new RuntimeSetPositionCommand( - physics, - RuntimeSetPositionOperationKind.RemoteAuthoritative, - body.LastUpdateTime, - record.VelocityAuthorityVersion, - ShadowWorldOffsetX: 0f, - ShadowWorldOffsetY: 0f), - Result = result, - SpatialAuthorityVersion = record.SpatialAuthorityVersion, - PlacementCommitVersion = record.PlacementCommitVersion, - ExactCellId = result.CellId, - WakeableLostCell = false, - Stage = RuntimeEntityPlacementStage - .AwaitingWithdrawalAcknowledgement, - Kind = RuntimeSetPositionOperationKind.RemoteAuthoritative, - Portal = default, - }; + // F4: routed through RentOperation() (rather than a fresh + // `new Operation { ... }`) so every construction flows one path - + // see ResetAllFieldsToDefault's completeness-net doc comment. The + // sole caller (Cancel) always runs CancelCoreDeferred against this + // same key first, so there is no displaced-operation state left to + // read here. + Operation operation = RentOperation(); + operation.Record = record; + operation.Body = body; + operation.Token = new RuntimeEntityPlacementToken( + _entities.SessionLifetimeVersion, + key, + record.PositionAuthorityVersion, + checked(++_nextOperationId), + RuntimeEntityPlacementPreparationKind.LegacyDirect); + operation.Key = key; + operation.PositionAuthorityVersion = record.PositionAuthorityVersion; + operation.SessionLifetimeVersion = _entities.SessionLifetimeVersion; + operation.SourceSpatialAuthorityVersion = record.SpatialAuthorityVersion; + operation.SourceVelocityAuthorityVersion = record.VelocityAuthorityVersion; + operation.PreviousContact = body.InContact; + operation.PreviousOnWalkable = body.OnWalkable; + operation.Command = new RuntimeSetPositionCommand( + physics, + RuntimeSetPositionOperationKind.RemoteAuthoritative, + body.LastUpdateTime, + record.VelocityAuthorityVersion, + ShadowWorldOffsetX: 0f, + ShadowWorldOffsetY: 0f); + operation.Result = result; + operation.SpatialAuthorityVersion = record.SpatialAuthorityVersion; + operation.PlacementCommitVersion = record.PlacementCommitVersion; + operation.ExactCellId = result.CellId; + operation.WakeableLostCell = false; + operation.Stage = RuntimeEntityPlacementStage + .AwaitingWithdrawalAcknowledgement; + operation.Kind = RuntimeSetPositionOperationKind.RemoteAuthoritative; + operation.Portal = default; + return operation; } + /// + /// Reference-identity currency check. SAFE ONLY when no reentrancy + /// point (a synchronous publish, a collision-report dispatch reaching + /// an arbitrary IRuntimeCollisionReportObserver, or a + /// ground-edge HitGround/LeaveGround callback) has intervened between + /// `operation` being obtained/last verified and this call - every + /// call site is audited and either (a) has no such reentrancy point in + /// between (commented at the call site), or (b) has been converted to + /// instead. See + /// 's doc comment for why + /// ReferenceEquals becomes a tautology once pooling can hand + /// the SAME physical instance back out for a different logical + /// operation at the same key. + /// private bool IsCurrent(Operation operation) => _operations.TryGetValue(operation.Key, out Operation? current) && ReferenceEquals(current, operation) - && _entities.SessionLifetimeVersion == operation.SessionLifetimeVersion + && IsOperationStateConsistent(operation); + + /// + /// Round 3: the consistency half of , + /// factored out so can reuse the exact + /// same checks after establishing identity via Token (safe under + /// pooling) instead of ReferenceEquals (unsafe under pooling - + /// see 's doc comment). + /// + private bool IsOperationStateConsistent(Operation operation) => + _entities.SessionLifetimeVersion == operation.SessionLifetimeVersion && _entities.IsCurrent(operation.Record) && operation.Record.Key == operation.Key && (operation.Body is null @@ -3979,6 +4670,40 @@ internal sealed class RuntimeSetPositionState : IDisposable && operation.Record.PlacementCommitVersion == operation.PlacementCommitVersion; + /// + /// Round 3: the class-wide replacement for + /// ReferenceEquals/ at any frame + /// that holds an Operation reference across a reentrancy point (a + /// synchronous publish, a collision-report dispatch that can reach an + /// arbitrary IRuntimeCollisionReportObserver, or a ground-edge + /// HitGround/LeaveGround callback). + /// MUST be read from the operation (or already be the caller's own + /// token parameter) BEFORE that reentrancy point - reading it fresh off + /// a possibly-already-repurposed reference here would be exactly as + /// tautological as the ReferenceEquals check this replaces (see + /// 's doc comment for the full + /// hazard: a reentrant cancel-then-begin can retire-then-rent the SAME + /// physical instance for a DIFFERENT logical operation, LIFO). On + /// success, returns the FRESHLY resolved Operation so the caller + /// continues with a reference proven current at THIS instant, never + /// the possibly-stale one it held before the reentrancy point. + /// + private bool IsCurrentByToken( + RuntimeEntityKey key, + in RuntimeEntityPlacementToken capturedToken, + [NotNullWhen(true)] out Operation? operation) + { + if (_operations.TryGetValue(key, out Operation? current) + && current.Token == capturedToken + && IsOperationStateConsistent(current)) + { + operation = current; + return true; + } + operation = null; + return false; + } + private bool IsExactDormantLocalActivationCurrent( RuntimeEntityRecord record, PhysicsBody body, @@ -3989,6 +4714,9 @@ internal sealed class RuntimeSetPositionState : IDisposable bool allowDeferredLease = false) { operation = null; + // Round 3 audit: safe - fresh lookup + Token check earlier in this + // SAME guard clause (below) precede IsCurrent, nothing reentrant in + // between; this dormant family has zero production callers. if (!token.IsValid || token.Entity != record.Key || token.PreparationKind @@ -4052,9 +4780,21 @@ internal sealed class RuntimeSetPositionState : IDisposable } private bool IsVelocityCurrent(Operation operation) => - operation.SourceVelocityAuthorityVersion == 0UL - || operation.Record.VelocityAuthorityVersion - == operation.SourceVelocityAuthorityVersion; + IsVelocityCurrent( + operation.SourceVelocityAuthorityVersion, + operation.Record); + + /// + /// F1: overload taking the hoisted scalar directly, for callers (like + /// CommitCanonical) that must not re-read a live + /// after a ground-edge callback may have retired + /// and repurposed it. + /// + private static bool IsVelocityCurrent( + ulong sourceVelocityAuthorityVersion, + RuntimeEntityRecord record) => + sourceVelocityAuthorityVersion == 0UL + || record.VelocityAuthorityVersion == sourceVelocityAuthorityVersion; private static MoverPreparationAuthority CapturePreparationAuthority( Operation operation, @@ -4242,20 +4982,43 @@ internal sealed class RuntimeSetPositionState : IDisposable .CancelledAwaitingAcknowledgement; discard = cancelled; } + // C2: `operation` was just removed from `_operations` above - see + // RetireOperationToPool's doc comment for why every caller (this one + // included) is safe to hand it back here, even the discard-pending + // case (AcknowledgeProjection's Discard branch never looks the + // operation back up by key). + RetireOperationToPool(operation); return true; } + /// + /// Round 3: takes the entity key plus a Token CAPTURED BY THE CALLER + /// (before any reentrancy point the caller passed through) instead of + /// an reference. The ReferenceEquals + /// check this replaced was exactly the shape the round 3 architecture + /// review flagged: safe before pooling (an Operation instance was never + /// reused), silently tautological after (a reentrant cancel-then-begin + /// can retire-then-rent the SAME instance for a DIFFERENT logical + /// operation, so `expected` and the freshly-looked-up `current` could be + /// the same reference while representing different operations) - with + /// the worse outcome that this method would then cancel the NEWER + /// operation instead of correctly no-op'ing. See + /// 's doc comment for the full + /// hazard and for the equivalent + /// conversion applied to plain currency checks. + /// private RuntimePlacementCancellationReceipt CancelCore( - Operation expected, + RuntimeEntityKey key, + in RuntimeEntityPlacementToken expectedToken, bool preserveLostFamily = false) { - if (!_operations.TryGetValue(expected.Key, out Operation? current) - || !ReferenceEquals(current, expected)) + if (!_operations.TryGetValue(key, out Operation? current) + || current.Token != expectedToken) { return default; } _ = CancelCoreDeferred( - expected.Key, + key, cancelLostFamily: false, preserveLostFamily, out RuntimePlacementProjectionSnapshot? discard); diff --git a/tests/AcDream.Runtime.Tests/Physics/RuntimeSetPositionStateTests.cs b/tests/AcDream.Runtime.Tests/Physics/RuntimeSetPositionStateTests.cs index 7b685664..d1e4f973 100644 --- a/tests/AcDream.Runtime.Tests/Physics/RuntimeSetPositionStateTests.cs +++ b/tests/AcDream.Runtime.Tests/Physics/RuntimeSetPositionStateTests.cs @@ -1,5 +1,6 @@ using System.Collections.Immutable; using System.Numerics; +using System.Reflection; using AcDream.Content; using AcDream.Content.Pak; using AcDream.Core.Net; @@ -288,10 +289,28 @@ public sealed class RuntimeSetPositionStateTests } long allocated = GC.GetAllocatedBytesForCurrentThread() - before; - // The dormant 4B1 owner still allocates its operation/projection - // envelope. Pin the measured Release ceiling so 4B2 cannot activate - // the route without making this cost explicit or reducing it. - Assert.InRange(allocated / iterations, 1L, 2_048L); + // C2: root-caused and fixed the dormant 4B1 owner's per-operation + // allocations instead of raising this cap. Measured exactly 944 B/op + // (stable across 5+ repeat runs), down from the pre-fix 2,032 B/op - + // pooling `Operation` instances (BeginAcceptedPlacementCore no + // longer allocates a fresh instance every accepted placement), + // caching the `PhysicsEngine.SetPosition` collision-report delegate + // instead of a per-call closure, and replacing the LINQ + // `_pendingProjection.First()` pattern (which boxed + // SortedDictionary's struct Enumerator through IEnumerable every + // acknowledgement) with a non-boxing foreach. See + // docs/research/2026-07-31-canonical-set-position.md for the C2 + // finding this closes and the accepted residual floor: ~520 B/op + // lives inside AcDream.Core's PhysicsEngine.SetPosition (transition + // init/inner solve/query-footprint materialization - shared physics + // infrastructure, out of this Runtime-only slice's scope) and + // ~208 B/op is SortedDictionary's inherent per-Add tree-node + // allocation for `_pendingProjection` (replacing that ordered + // structure to chase the last ~230 B/op was judged too invasive/ + // risky for the remaining headroom under this cap). 1,536 keeps + // roughly 60% headroom over the measured value for JIT/environment + // variance without re-opening the door to unbounded per-op growth. + Assert.InRange(allocated / iterations, 1L, 1_536L); } [Fact] @@ -1081,6 +1100,243 @@ public sealed class RuntimeSetPositionStateTests Assert.True(lifetime.Physics.SetPosition.CaptureOwnership().IsConverged); } + /// + /// F1 regression: from within the ground-edge HitGround callback, drive + /// an explicit cancel-then-begin for the SAME entity. Because the + /// Operation pool is LIFO, this retires the outer, still-executing + /// operation and immediately rents that EXACT instance back out for the + /// new ("recycled") operation - a strictly more adversarial recycle than + /// ReentrantGroundEdgePlacementCannotBeCancelledByDisplacedOperation's + /// begin-only chain (there, nothing had retired the outer operation to + /// the pool by the time the reentrant Begin ran, so it always drew a + /// different or fresh instance). CommitCanonical must still feed the + /// ORIGINAL (pre-callback) PreviousContact/PreviousOnWalkable into + /// HandleSetPositionCollisionReports, not whatever the recycled instance + /// now holds for the unrelated new operation. This is observable: the + /// environment-collision report only fires at all when + /// (!previousOnWalkable && body.OnWalkable) - the recycled + /// instance's PreviousOnWalkable (captured from the body's ALREADY- + /// landed post-transition state) would read true instead of the + /// original false, silently suppressing the report if the operation's + /// live (potentially repurposed) fields were read instead of hoisted + /// locals. + /// + [Fact] + public void ReentrantCancelThenBeginRecyclesInstanceButCollisionReportUsesPreCallbackValues() + { + PhysicsEngine engine = FlatEngine(SourceLandblock, 0f); + engine.TransitionCellCollisionTestHook = + (transition, phase, _, observed) => + { + if (phase == TransitionCellCollisionPhase.Environment) + { + transition.CollisionInfo.SetContactPlane( + new Plane(Vector3.UnitZ, 0f), + SourceCell); + } + return observed; + }; + using var lifetime = new RuntimeEntityObjectLifetime(engine); + RuntimeEntityRecord record = CreateRecord(lifetime, 0x70001046u, 1); + PhysicsBody body = AttachBody( + lifetime, + record, + SourceCell, + PhysicsStateFlags.Gravity | PhysicsStateFlags.ReportCollisions); + var collisionObserver = new CollisionReportObserver(); + using IDisposable collisionSubscription = lifetime.Physics + .CollisionReports.Subscribe(collisionObserver); + + RuntimeEntityPlacementToken recycled = default; + var remote = new ReentrantRemotePlacement(body) + { + CellId = SourceCell, + OnHitGround = () => + { + lifetime.Physics.SetPosition.Cancel( + record, + publishWithdrawal: false); + recycled = lifetime.Physics.SetPosition.BeginAcceptedPlacement( + record, + record.PositionAuthorityVersion, + RuntimeSetPositionOperationKind.RemoteAuthoritative); + }, + }; + lifetime.Entities.SetRemoteMotion(record, remote); + + _ = lifetime.Physics.SetPosition.Apply( + record, + record.PositionAuthorityVersion, + Command(Request(SourceCell, new Vector3(13f, 18f, 7f)))); + + Assert.True(recycled.IsValid); + Assert.True(body.OnWalkable); + RuntimeCollisionReport report = Assert.Single(collisionObserver.Reports); + Assert.Equal(RuntimeCollisionReportKind.EnvironmentCollision, report.Kind); + Assert.False(report.RecipientWasInContact); + } + + /// + /// Round 3 pinned regression: the coordinator's precise reachability + /// trace for the Cancel-path recycle hazard. Cancel(record, bool) + /// creates a withdrawal operation and installs it at this entity's key, + /// then calls PublishPlacement(cancelledOld) to notify observers + /// that the still-pending Place projection has been superseded by a + /// Discard - a synchronous dispatch that can reach an arbitrary + /// subscriber, which here + /// reentrantly calls BeginAcceptedPlacement for the SAME entity. + /// Because the Operation pool is LIFO and + /// BeginAcceptedPlacementCore rents only after retiring, that + /// reentrant Begin retires the withdrawal operation Cancel is + /// mid-way through publishing for and rents the exact same physical + /// instance right back out for a brand-new "inner" operation. Before + /// round 3's captured-token-vs-fresh-lookup conversion, Cancel's + /// post-publish check read IsCurrent(operation) off that live + /// (now-repurposed) reference - a tautology that would have let the + /// outer withdrawal proceed to publish a SECOND (Withdraw) projection + /// stamped with the inner operation's state, corrupting a Begin-only + /// operation that never asked for a pending projection. The fix + /// captures operation.Token into a local BEFORE + /// PublishPlacement runs and re-verifies it via a fresh + /// IsCurrentByToken lookup afterward, so the stale reference is + /// detected and the outer call returns without publishing anything + /// beyond the Discard. + /// + [Fact] + public void ReentrantBeginDuringCancelPublishCannotBeOverwrittenByOuterWithdraw() + { + PhysicsEngine engine = FlatEngine(SourceLandblock, 0f); + using var lifetime = new RuntimeEntityObjectLifetime(engine); + RuntimeEntityRecord record = CreateRecord(lifetime, 0x70001250u, 1); + _ = AttachBody(lifetime, record, SourceCell); + RuntimeSetPositionOutcome pending = lifetime.Physics.SetPosition.Apply( + record, + record.PositionAuthorityVersion, + Command(Request(SourceCell, new Vector3(18f, 18f, 7f)))); + Assert.Equal( + RuntimeSetPositionStatus.CommittedHostAcknowledgementPending, + pending.Status); + + RuntimeEntityPlacementToken inner = default; + var observer = new PlacementObserver(delta => + { + if (delta.Placement.Kind is RuntimePlacementProjectionKind.Discard) + { + inner = lifetime.Physics.SetPosition.BeginAcceptedPlacement( + record, + record.PositionAuthorityVersion, + RuntimeSetPositionOperationKind.RemoteAuthoritative); + } + }); + using IDisposable subscription = lifetime.Events.SubscribePlacement(observer); + + bool removed = lifetime.Physics.SetPosition.Cancel( + record, + publishWithdrawal: true); + + Assert.True(removed); + Assert.True(inner.IsValid); + // Only the Discard (for the pending Place this Cancel superseded) + // should have published - a stale-reference bug would have added a + // second (Withdraw) delta stamped with `inner`'s state. + RuntimePlacementDelta delta = Assert.Single(observer.Deltas); + Assert.Equal(RuntimePlacementProjectionKind.Discard, delta.Placement.Kind); + // `inner` must still be exactly what BeginAcceptedPlacement handed + // back - untouched by the outer Cancel's withdrawal publish. + Assert.True(lifetime.Physics.SetPosition.IsPlacementCurrent(inner)); + RuntimeSetPositionOwnershipSnapshot ownership = + lifetime.Physics.SetPosition.CaptureOwnership(); + Assert.Equal(1, ownership.ActiveOperationCount); + Assert.Equal(1, ownership.AwaitingPreparationCount); + } + + /// + /// Round 3 CancelCore-shape regression: from within the ground-edge + /// HitGround callback, recycle the SAME operation instance + /// (cancel-then-begin, LIFO pool) for a brand-new "inner" operation on + /// the same entity, AND advance the record's PlacementCommitVersion a + /// second time (exactly what a completed nested commit would also have + /// produced) so CommitCanonical's post-callback + /// IsCanonicalPlacementCommitCurrent check fails for the OUTER commit. + /// SubmitPreparedPlacementCore's own failure branch then calls + /// PublishCancellation(CancelCore(token.Entity, token)) using + /// `token` - the outer caller's own (now-stale) captured token. Before + /// round 3, CancelCore(Operation expected, ...) compared + /// `ReferenceEquals(current, expected)`; with the physical instance + /// recycled for `inner`, that check would have found the SAME instance + /// at this key and retired/cancelled it out from under the still-active + /// inner operation - the "worse outcome: cancelling the newer + /// operation" the reviewer flagged. The Token-keyed + /// CancelCore(key, expectedToken) must instead see + /// `current.Token != expectedToken` and no-op, leaving `inner` + /// untouched. + /// + [Fact] + public void ReentrantCancelThenBeginDuringCommitFailureLeavesInnerOperationUncancelled() + { + PhysicsEngine engine = FlatEngine(SourceLandblock, 0f); + engine.TransitionCellCollisionTestHook = + (transition, phase, _, observed) => + { + if (phase == TransitionCellCollisionPhase.Environment) + { + transition.CollisionInfo.SetContactPlane( + new Plane(Vector3.UnitZ, 0f), + SourceCell); + } + return observed; + }; + using var lifetime = new RuntimeEntityObjectLifetime(engine); + RuntimeEntityRecord record = CreateRecord(lifetime, 0x70001251u, 1); + PhysicsBody body = AttachBody( + lifetime, + record, + SourceCell, + PhysicsStateFlags.Gravity | PhysicsStateFlags.ReportCollisions); + + RuntimeEntityPlacementToken inner = default; + var remote = new ReentrantRemotePlacement(body) + { + CellId = SourceCell, + OnHitGround = () => + { + lifetime.Physics.SetPosition.Cancel( + record, + publishWithdrawal: false); + // Simulate a nested commit completing for `inner` (without + // needing a full nested SetPosition round-trip): advance + // PlacementCommitVersion BEFORE `inner`'s operation is + // created so `inner` snapshots the already-advanced value + // and stays internally self-consistent, while the OUTER + // commit's `canonicalCommitVersion` (captured before this + // callback ran) now mismatches the record. + lifetime.Entities.AdvancePlacementCommit(record); + inner = lifetime.Physics.SetPosition.BeginAcceptedPlacement( + record, + record.PositionAuthorityVersion, + RuntimeSetPositionOperationKind.RemoteAuthoritative); + }, + }; + lifetime.Entities.SetRemoteMotion(record, remote); + + RuntimeSetPositionOutcome outcome = lifetime.Physics.SetPosition.Apply( + record, + record.PositionAuthorityVersion, + Command(Request(SourceCell, new Vector3(13f, 18f, 7f)))); + + Assert.Equal(RuntimeSetPositionStatus.Cancelled, outcome.Status); + Assert.True(inner.IsValid); + // The bug this pins: a reference-based CancelCore would retire the + // recycled instance (now serving `inner`) out from under it. + // Confirm `inner` is still the live, current operation for this + // entity. + Assert.True(lifetime.Physics.SetPosition.IsPlacementCurrent(inner)); + RuntimeSetPositionOwnershipSnapshot ownership = + lifetime.Physics.SetPosition.CaptureOwnership(); + Assert.Equal(1, ownership.ActiveOperationCount); + Assert.Equal(1, ownership.AwaitingPreparationCount); + } + [Fact] public void RetrySnapshotPreservesOrderWhenObserverAcknowledgesTwoPendingTokens() { @@ -2012,6 +2268,158 @@ public sealed class RuntimeSetPositionStateTests Assert.True(lifetime.Physics.CaptureOwnership().IsConverged); } + /// + /// F2 regression: the C2 Operation pool retains full previous-generation + /// entity graphs (Record -> Snapshot/PhysicsBody/clock/host references) + /// through every pooled instance until it is rented and reset - a + /// session reset must not leave that behind uncleared, even though + /// PooledOperationCount is deliberately excluded from + /// (see + /// its doc comment). + /// + [Fact] + public void OperationPoolClearsOnResetSession() + { + PhysicsEngine engine = FlatEngine(SourceLandblock, 0f); + using var lifetime = new RuntimeEntityObjectLifetime(engine); + RuntimeEntityRecord record = CreateRecord(lifetime, 0x70001048u, 1); + _ = AttachBody(lifetime, record, SourceCell); + + RuntimeSetPositionOutcome outcome = lifetime.Physics.SetPosition.Apply( + record, + record.PositionAuthorityVersion, + Command(Request(SourceCell, new Vector3(11f, 18f, 7f)))); + Assert.True(lifetime.Physics.SetPosition.AcknowledgeProjection( + outcome.Projection)); + + RuntimeSetPositionOwnershipSnapshot beforeReset = + lifetime.Physics.SetPosition.CaptureOwnership(); + Assert.True(beforeReset.PooledOperationCount >= 1); + + lifetime.Physics.SetPosition.ResetSession(); + + Assert.Equal( + 0, + lifetime.Physics.SetPosition.CaptureOwnership().PooledOperationCount); + } + + /// + /// F2 regression: same guarantee as + /// , via Dispose instead + /// of ResetSession - both callers of ClearOwnedState must clear the + /// pool. + /// + [Fact] + public void OperationPoolClearsOnDispose() + { + PhysicsEngine engine = FlatEngine(SourceLandblock, 0f); + var lifetime = new RuntimeEntityObjectLifetime(engine); + RuntimeEntityRecord record = CreateRecord(lifetime, 0x70001049u, 1); + _ = AttachBody(lifetime, record, SourceCell); + + RuntimeSetPositionOutcome outcome = lifetime.Physics.SetPosition.Apply( + record, + record.PositionAuthorityVersion, + Command(Request(SourceCell, new Vector3(11f, 18f, 7f)))); + Assert.True(lifetime.Physics.SetPosition.AcknowledgeProjection( + outcome.Projection)); + + RuntimeSetPositionOwnershipSnapshot beforeDispose = + lifetime.Physics.SetPosition.CaptureOwnership(); + Assert.True(beforeDispose.PooledOperationCount >= 1); + + lifetime.Dispose(); + + Assert.Equal( + 0, + lifetime.Physics.SetPosition.CaptureOwnership().PooledOperationCount); + } + + /// + /// F4 regression: converting Operation's properties from + /// required { get; init; } to plain { get; set; } (so + /// pooling could recycle instances) dropped the compiler's completeness + /// net - nothing any longer forces every construction site to set every + /// field. This reflection-based test is the runtime replacement: it + /// pins the exact set of backing fields Operation declares (private, so + /// only reflection can reach it from a test) against a hardcoded, + /// maintained list. Adding a new auto-property to Operation changes its + /// backing-field set and fails this test immediately - the failure + /// message is the prompt to update BOTH this list and + /// Operation.ResetAllFieldsToDefault in the same change, exactly + /// mirroring what a missing `required` member assignment used to force + /// at compile time. + /// + [Fact] + public void OperationResetAllFieldsToDefaultTouchesEveryDeclaredField() + { + Type? operationType = typeof(RuntimeSetPositionState).GetNestedType( + "Operation", + BindingFlags.NonPublic); + Assert.NotNull(operationType); + + FieldInfo[] actualFields = operationType!.GetFields( + BindingFlags.Instance + | BindingFlags.NonPublic + | BindingFlags.Public); + string[] actualFieldNames = actualFields + .Select(field => field.Name) + .OrderBy(name => name, StringComparer.Ordinal) + .ToArray(); + + // The maintained list: one entry per auto-property Operation + // declares. Kept as property names (not the `k__BackingField` + // form the compiler actually emits) so a reviewer can read it + // directly against the property list in the source file. + string[] expectedPropertyNames = + [ + "Record", + "Body", + "Token", + "Key", + "PositionAuthorityVersion", + "SessionLifetimeVersion", + "SourceSpatialAuthorityVersion", + "SourceVelocityAuthorityVersion", + "PreviousContact", + "PreviousOnWalkable", + "Command", + "Result", + "SpatialAuthorityVersion", + "PlacementCommitVersion", + "ExactCellId", + "CollisionGeneration", + "CollisionPrefix", + "WithdrawalAcknowledged", + "CollisionGenerationReady", + "CollisionQuiescenceHeld", + "ProjectionSequence", + "WakeableLostCell", + "Stage", + "Kind", + "Portal", + "RequiresPreparation", + "Expired", + "LostFamilyKeys", + "InheritedLostDeadline", + "EnteringWorldFromCelllessResidence", + "DormantLocalActivation", + "PreparedCommandAwaitingWithdrawalAck", + "InPool", + ]; + string[] expectedFieldNames = expectedPropertyNames + .Select(name => $"<{name}>k__BackingField") + .OrderBy(name => name, StringComparer.Ordinal) + .ToArray(); + + Assert.Equal(expectedFieldNames, actualFieldNames); + + MethodInfo? resetMethod = operationType.GetMethod( + "ResetAllFieldsToDefault", + BindingFlags.Instance | BindingFlags.NonPublic); + Assert.NotNull(resetMethod); + } + [Fact] public void CommittedParentDoesNotLeakAcrossChildGuidReuse() {