fix(physics): restore a cancelled park instead of leaving the entity withdrawn
Shipped-code defect affecting committed route-2 code, found while reviewing route 4b-1. RuntimeSetPositionState.ParkDeferred withdraws an entity from the world: body.InWorld = false, TransientStateFlags.Active cleared, WithdrawCanonical, SuspendObjectClock. CancelCoreDeferred then removed the operation and rewrote the pending Withdraw into a Discard while restoring NONE of it. So cancelling a wakeable park was strictly worse than keeping one — the park is wakeable, the cancel destroys the only object that could ever wake it, and the entity is left invisible AND intangible with nothing to bring it back. Route 2's re-issue funnel masked this: re-issuing is correct for a one-shot ForcePosition ACE never repeats, and wrong for a repeated remote stream, so the hole was hidden rather than fixed. Retail's own answer is a working park, verified in the decomp rather than assumed: CPhysicsObj::SetPositionInternal @0x00515BD0, when AdjustPosition yields no cell @0x00515C1D, calls prepare_to_leave_visibility @0x00515CDA, store_position @0x00515CE2 (the DESTINATION pose is committed), GotoLostCell @0x00515CF2 registering at m_position.objcell_id read AFTER store_position (so the destination cell), clears transient 0x80 @0x00515CF7, and returns OK @0x00515D07. InitObjCell @0x00508260 drains the lost list on cell load and calls reenter_visibility @0x00516250, which re-places from the object's OWN m_position with flags 0x11. Two corrections to the direction I gave, both forced by evidence and both right: The pose must NOT be rolled back — only the withdrawal. Three shipped route-2 tests capture positionAtPark AFTER the park and assert it survives the cancel, and retail agrees: store_position commits the destination and nothing un-commits it. Restoring residency at the body's committed cell is therefore retail's own cell choice, not merely self-consistent. The gate defaults to FALSE with four explicit opt-ins, rather than defaulting true with opt-outs at the withdrawal callers. That keeps every one of the ~20 shipped Forget/ForgetExactPlacement sites at exactly its current behaviour instead of depending on having correctly enumerated the withdrawal transactions. Review had already found the broad version corrupting five of them (TryApplyPickup, CommitAcceptedParent, CommitAcceptedParentCellless, CommitWithdrawal, CommitPositionChannelUpdate): they hand-roll a partial re-withdrawal that undoes the clock and FullCellId but not InWorld or the _spatialRoots re-registration, leaving a picked-up item both in inventory and an InWorld cellless spatial root in the physics workset. ParkDeferred's restorableOnCancel is opt-in for exactly one of its four callers — the plain unplaceable-destination park. Every quiescence and retirement park is excluded deliberately: those entities are withdrawn because their world is going away, and restoring residency inside a quiescing prefix blocks its retirement. VerifyPositionChannelCancellation now asserts InWorld and IsSpatialRoot per channel — Position is a cancellation and must restore; Pickup and Parent are withdrawals and must not. It previously asserted only !IsDeferred and counts, which is why five green states hid this. Register row AP-136 measured against GotoLostCell/reenter_visibility rather than labelled "retail-shaped". Files #309 (the restore-on-cancel residual, with park-survives recorded as the retail-faithful target and its two blockers named: the NewerPositionPickupAndParentEachCancelExactLostOperation invariant and teardown convergence) and #310 (an unbounded retirement stall — a retained preparation retry pins its prefix through HasOldPrefixPlacementDebt forever, and TickLostCellDeadlines has no production caller so the 25 s timer never fires). This is a user-observable change to shipped paths: restorableOnCancel: true sits in SubmitPreparedPlacementCore, the shared core behind every production placement. AP-136 and #309 carry the proposed two-client check. Gates: complete Release solution 10,973 passed / 4 skipped / 0 failed (baseline 10,938). Every new test discrimination-verified by reverting the fix. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
parent
9e97be1896
commit
634bc5513a
9 changed files with 666 additions and 10 deletions
|
|
@ -319,6 +319,95 @@ public sealed partial class RuntimeCollisionPrefixQuiescenceTests
|
|||
submitted.Status);
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// C4 route 4b-1 review finding B2, pinned. The contract asked whether
|
||||
/// <c>ParkCollisionResidents</c>'s overlap throw stays unreachable under a
|
||||
/// design that lets remotes hold placement operations. It does, and for a
|
||||
/// STRUCTURAL reason rather than an incidental one:
|
||||
/// <c>TryAcquireCollisionPrefixMutationPermission</c> consults
|
||||
/// <c>HasOldPrefixPlacementDebt</c> FIRST and refuses permission outright,
|
||||
/// so <c>ParkCollisionResidents</c> is never entered while any affected
|
||||
/// root holds an operation. (Additionally, a parked record has
|
||||
/// <c>FullCellId == 0</c>, so it is not an affected resident at all.)
|
||||
///
|
||||
/// <para><b>The real hazard is not a throw, it is an unbounded streaming
|
||||
/// stall — and this test PINS it as a stall, not as a pass.</b> A retained
|
||||
/// preparation retry keeps its landblock prefix in placement debt, so the
|
||||
/// retirement is refused on EVERY poll and the landblock never retires.
|
||||
/// There is no bound: the retirement coordinator simply retries, and
|
||||
/// <c>TickLostCellDeadlines</c> — the only expiry that could break the
|
||||
/// cycle — has NO production caller, so its deadline never fires. The only
|
||||
/// thing that clears it is an inbound packet for that same entity, which
|
||||
/// is exactly what a <c>RetrySetupUnavailable</c> on an asset that never
|
||||
/// loads does not produce.</para>
|
||||
///
|
||||
/// <para>This is a pre-existing hazard independent of route 4b-1, filed as
|
||||
/// its own issue. 4b-1 does NOT bound it; it only avoids widening it, by
|
||||
/// declining to retain operations for destinations it cannot service.</para>
|
||||
/// </summary>
|
||||
[Fact]
|
||||
public void RetainedPreparationRetryStallsPrefixRetirementIndefinitely()
|
||||
{
|
||||
using var fixture = new Fixture();
|
||||
RuntimeEntityRecord record = fixture.Add(
|
||||
0x7000300Bu,
|
||||
1,
|
||||
CellP,
|
||||
new Vector3(10f, 28f, 7f));
|
||||
|
||||
// A retained preparation retry: begun, never prepared — the shape a
|
||||
// RetrySetupUnavailable on an asset that never resolves leaves behind.
|
||||
RuntimeEntityPlacementToken retained = fixture.Lifetime.Physics
|
||||
.SetPosition.BeginAcceptedPlacement(
|
||||
record,
|
||||
record.PositionAuthorityVersion,
|
||||
RuntimeSetPositionOperationKind.RemoteAuthoritative);
|
||||
Assert.True(retained.IsValid);
|
||||
|
||||
RuntimeCollisionPrefixQuiescenceToken token = fixture.Begin(2UL);
|
||||
|
||||
// The production retirement path, polled hard. Permission is refused
|
||||
// every single time; nothing in the system advances it.
|
||||
for (int poll = 0; poll < 1_000; poll++)
|
||||
{
|
||||
Assert.False(
|
||||
fixture.TryAcquire(token, out _),
|
||||
$"retirement unexpectedly acquired permission on poll {poll}; "
|
||||
+ "if this now succeeds the stall has been bounded and "
|
||||
+ "this test's pinned decision must be revisited");
|
||||
}
|
||||
|
||||
// ParkCollisionResidents was never entered, so its overlap throw could
|
||||
// not fire — the contract's item 6, proven structurally.
|
||||
Assert.True(fixture.Lifetime.Physics.IsSpatialRoot(record));
|
||||
Assert.False(fixture.Lifetime.Physics.SetPosition.TryPeekProjection(
|
||||
out _));
|
||||
|
||||
// And the discriminator: retiring the retained operation is what
|
||||
// releases the prefix. Once the debt is gone the ordinary two-phase
|
||||
// handshake proceeds — ParkCollisionResidents withdraws the residents
|
||||
// and permission follows the withdrawal acknowledgements — so drain
|
||||
// those exactly as the production host does.
|
||||
_ = fixture.Lifetime.Physics.SetPosition.ForgetExactPlacement(retained);
|
||||
bool acquired = false;
|
||||
for (int poll = 0; poll < 32 && !acquired; poll++)
|
||||
{
|
||||
acquired = fixture.TryAcquire(token, out _);
|
||||
if (acquired)
|
||||
break;
|
||||
if (fixture.Lifetime.Physics.SetPosition.TryPeekProjection(
|
||||
out RuntimePlacementProjectionSnapshot projection))
|
||||
{
|
||||
Assert.True(fixture.Lifetime.Physics.SetPosition
|
||||
.AcknowledgeProjection(projection.Token));
|
||||
}
|
||||
}
|
||||
Assert.True(
|
||||
acquired,
|
||||
"clearing the retained preparation retry must let the prefix "
|
||||
+ "retire; if it does not, the stall has a second cause");
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void QueriedNeighborPrefixHoldsResultWithoutRequestDependency()
|
||||
{
|
||||
|
|
|
|||
|
|
@ -1487,6 +1487,126 @@ public sealed class RuntimeSetPositionStateTests
|
|||
placed.Token));
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// C4 route 4b-1 review, the decisive case. <c>ParkDeferred</c> withdraws
|
||||
/// the entity from the world (<c>InWorld = false</c>, object clock
|
||||
/// suspended, canonical residency dropped). The merge-time
|
||||
/// <c>Forget</c> that every accepted Position performs
|
||||
/// (<c>RuntimeEntityObjectLifetime.TryApplyPosition</c>) then removes the
|
||||
/// operation and restores NONE of that, so the entity is left invisible
|
||||
/// AND intangible with nothing left that could ever wake it.
|
||||
///
|
||||
/// <para>Retail cannot reach that state: <c>CPhysicsObj::SetPositionInternal</c>
|
||||
/// @0x00515BD0 commits the destination pose via <c>store_position</c>
|
||||
/// @0x00515CE2 and registers the object with
|
||||
/// <c>CObjectMaint::GotoLostCell</c> @0x00515CF2 / @0x00508210, and the
|
||||
/// ONLY remover of that registration is
|
||||
/// <c>CObjectMaint::InitObjCell</c> @0x00508260, which drains the lost
|
||||
/// list on cell load and calls <c>CPhysicsObj::reenter_visibility</c>
|
||||
/// @0x00508296 / @0x00516250 for every object in it. A retail lost object
|
||||
/// is never silently un-registered.</para>
|
||||
/// </summary>
|
||||
[Fact]
|
||||
public void CancellingWakeableParkLeavesEntityWithdrawnWithNothingToWakeIt()
|
||||
{
|
||||
PhysicsEngine engine = FlatEngine(SourceLandblock, 0f);
|
||||
using var lifetime = new RuntimeEntityObjectLifetime(engine);
|
||||
RuntimeEntityRecord record = CreateRecord(lifetime, 0x7000411Bu, 1);
|
||||
PhysicsBody body = AttachBody(lifetime, record, SourceCell);
|
||||
|
||||
Assert.True(body.InWorld);
|
||||
Assert.True(record.ObjectClock.IsActive);
|
||||
Assert.Equal(SourceCell, record.FullCellId);
|
||||
|
||||
RuntimeSetPositionOutcome parked = lifetime.Physics.SetPosition.Apply(
|
||||
record,
|
||||
record.PositionAuthorityVersion,
|
||||
Command(CrossLandblockRequest()));
|
||||
|
||||
Assert.Equal(RuntimeSetPositionStatus.DeferredCell, parked.Status);
|
||||
Assert.True(lifetime.Physics.SetPosition.IsDeferred(record));
|
||||
Assert.False(body.InWorld);
|
||||
Assert.False(record.ObjectClock.IsActive);
|
||||
Assert.Equal(0u, record.FullCellId);
|
||||
Vector3 parkedPosition = body.Position;
|
||||
|
||||
// The merge-time cancel every accepted Position performs.
|
||||
_ = lifetime.Physics.SetPosition.Forget(
|
||||
record,
|
||||
restoreCancelledPark: true);
|
||||
|
||||
// The park is gone (the shipped newer-Position-cancels-the-park
|
||||
// invariant, pinned by
|
||||
// NewerPositionPickupAndParentEachCancelExactLostOperation) - so the
|
||||
// entity MUST have been rolled back, or nothing is left to wake it.
|
||||
Assert.False(lifetime.Physics.SetPosition.IsDeferred(record));
|
||||
Assert.True(
|
||||
body.InWorld,
|
||||
"cancelled park left the entity withdrawn: InWorld=false");
|
||||
Assert.True(
|
||||
record.ObjectClock.IsActive,
|
||||
"cancelled park left the object clock suspended");
|
||||
Assert.True(
|
||||
record.FullCellId != 0u,
|
||||
"cancelled park left the entity without canonical residency");
|
||||
Assert.True(
|
||||
lifetime.Physics.IsSpatialRoot(record),
|
||||
"cancelled park left the entity out of the physics workset");
|
||||
Assert.Equal(
|
||||
TransientStateFlags.Active,
|
||||
body.TransientState & TransientStateFlags.Active);
|
||||
|
||||
// The pose committed at park time STANDS - retail's store_position
|
||||
// @0x00515CE2 commits the destination on the lost-cell branch and
|
||||
// never un-commits it. Residency follows the body's committed cell so
|
||||
// the two cannot disagree.
|
||||
Assert.Equal(parkedPosition, body.Position);
|
||||
Assert.Equal(body.CellPosition.ObjCellId, record.FullCellId);
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// The sibling cancel entry point. Route 2's controller and route 4b's
|
||||
/// remote controller both cancel a <c>DeferredCell</c> park through
|
||||
/// <c>ForgetExactPlacement</c>, not through <c>Forget</c>, so the rollback
|
||||
/// has to live at the shared <c>CancelCore</c> layer or the identical
|
||||
/// stranded-entity hole stays open on exactly the routes that produce it.
|
||||
/// </summary>
|
||||
[Fact]
|
||||
public void CancellingWakeableParkByExactTokenAlsoRestoresTheEntity()
|
||||
{
|
||||
PhysicsEngine engine = FlatEngine(SourceLandblock, 0f);
|
||||
using var lifetime = new RuntimeEntityObjectLifetime(engine);
|
||||
RuntimeEntityRecord record = CreateRecord(lifetime, 0x7000411Cu, 1);
|
||||
PhysicsBody body = AttachBody(lifetime, record, SourceCell);
|
||||
|
||||
RuntimeEntityPlacementToken token = lifetime.Physics.SetPosition
|
||||
.BeginAcceptedPlacement(
|
||||
record,
|
||||
record.PositionAuthorityVersion,
|
||||
RuntimeSetPositionOperationKind.RemoteAuthoritative);
|
||||
Assert.True(token.IsValid);
|
||||
RuntimeSetPositionOutcome parked = lifetime.Physics.SetPosition
|
||||
.SubmitPreparedPlacement(token, Command(CrossLandblockRequest()));
|
||||
Assert.Equal(RuntimeSetPositionStatus.DeferredCell, parked.Status);
|
||||
Assert.False(body.InWorld);
|
||||
Assert.False(record.ObjectClock.IsActive);
|
||||
Assert.Equal(0u, record.FullCellId);
|
||||
|
||||
_ = lifetime.Physics.SetPosition.ForgetExactPlacement(
|
||||
token,
|
||||
restoreCancelledPark: true);
|
||||
|
||||
Assert.False(lifetime.Physics.SetPosition.IsDeferred(record));
|
||||
Assert.True(body.InWorld, "exact-token cancel left the entity withdrawn");
|
||||
Assert.True(
|
||||
record.ObjectClock.IsActive,
|
||||
"exact-token cancel left the object clock suspended");
|
||||
Assert.True(
|
||||
record.FullCellId != 0u,
|
||||
"exact-token cancel left the entity without canonical residency");
|
||||
Assert.True(lifetime.Physics.IsSpatialRoot(record));
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void ReentrantNewerPositionDuringPickupDiscardSuppressesStalePickupDelta()
|
||||
{
|
||||
|
|
@ -2406,6 +2526,7 @@ public sealed class RuntimeSetPositionStateTests
|
|||
"DormantLocalActivation",
|
||||
"ParkReason",
|
||||
"PreparedCommandAwaitingWithdrawalAck",
|
||||
"ParkWithdrawal",
|
||||
"InPool",
|
||||
];
|
||||
string[] expectedFieldNames = expectedPropertyNames
|
||||
|
|
@ -2985,6 +3106,38 @@ public sealed class RuntimeSetPositionStateTests
|
|||
.DeferredSetPositionCount);
|
||||
Assert.Equal(0, lifetime.Physics.CaptureOwnership()
|
||||
.LostCellDeadlineCount);
|
||||
|
||||
// Pickup and Parent are WITHDRAWAL transactions, not cancellations:
|
||||
// each hand-rolls its own partial re-withdrawal after the Forget,
|
||||
// undoing the object clock and FullCellId but NOTHING else. The
|
||||
// park-rollback the accepted-Position merge opts into must never fire
|
||||
// underneath them - it would set InWorld and re-add the entity to
|
||||
// _spatialRoots, neither of which their cleanup undoes, leaving a
|
||||
// picked-up item both in inventory AND an InWorld cell-less spatial
|
||||
// root inside the CPhysics::UseTime workset. The counts above cannot
|
||||
// see either field, so assert them directly.
|
||||
//
|
||||
// Position is the one CANCELLATION here: a newer accepted Position
|
||||
// supersedes the placement while the entity stays in the world, so it
|
||||
// must roll back rather than strand the body.
|
||||
if (channel is CancellationChannel.Position)
|
||||
{
|
||||
Assert.True(
|
||||
record.PhysicsBody?.InWorld ?? false,
|
||||
"the cancellation channel must roll the park back");
|
||||
Assert.True(
|
||||
lifetime.Physics.IsSpatialRoot(record),
|
||||
"the cancellation channel must restore the spatial root");
|
||||
}
|
||||
else
|
||||
{
|
||||
Assert.False(
|
||||
record.PhysicsBody?.InWorld ?? false,
|
||||
"a withdrawal channel must not leave the body InWorld");
|
||||
Assert.False(
|
||||
lifetime.Physics.IsSpatialRoot(record),
|
||||
"a withdrawal channel must not leave the entity a spatial root");
|
||||
}
|
||||
}
|
||||
|
||||
private static void CommitParent(
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue