From daef7c9835aca8a3af5409fc288793a987598e00 Mon Sep 17 00:00:00 2001 From: Erik Date: Tue, 4 Aug 2026 16:48:41 +0200 Subject: [PATCH] fix(inventory): reset a split result's movement timestamps so recovery cannot throw (#314) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Found by C4 route 6's integration tests (1b484937) and split out of that zero-production closure per the standing split-on-discovery rule. PendingSplitToWorldProjection.BuildSpawn zeroes the top-level MovementSequence/ServerControlSequence, but its Physics.Timestamps `with` block overrode only Position/Teleport/ForcePosition/Instance — leaving Timestamps.Movement and .ServerControlledMove at the SOURCE item's values. RuntimeEntityObjectLifetime.HasConsistentCreateIdentityAndParent requires the PhysicsDesc timestamps and their flattened projections to agree, so the synthetic spawn failed the predicate and TryRecoverUnknownPosition threw `CreateObject 0x… has inconsistent instance or parent projections` instead of completing the canonical create-placement transaction. Reachable in ordinary play: retail's per-object update_times channels are monotonic and do not reset when an item re-enters a container, so any item that ever had world presence — dropped once, picked back up, then split — carries nonzero values in exactly those two fields. The split pile then never appears. Fix is the honest value, not a placation of the predicate: a fresh split GUID has no movement history by construction, so both channels are zero in both projections. Deliberately NOT fixed by loosening HasConsistentCreateIdentityAndParent — the predicate was right and the producer was wrong. The route-6 test that documented the throw (SplitSourceWithRetainedMovementTimestamps_ThrowsInsteadOfRecovering) is renamed to …_StillRecovers and now pins the fix. It asserts more than "no throw": the result's movement channels must be ZERO in both projections, so the test cannot pass against a lenient-predicate workaround. Sabotage-verified in both directions — restoring the old BuildSpawn reproduces the exact original InvalidOperationException. Notable for the campaign record: this is a crash in the precise mechanism route 6's scoping cited as EVIDENCE that drops already converge on the canonical transaction. Reading the code said the path converges; driving it said it throws. The zero-production route was still correct — and building its tests anyway is what found this. Complete Release suite 11,020 passed / 4 skipped / 0 failed, unchanged from 1b484937 (the test flipped its assertion rather than being added). Neither known flake fired. Co-Authored-By: Claude Opus 5 --- .../InventoryWorldDropProjectionController.cs | 16 +++++ .../LiveEntityHydrationControllerTests.cs | 59 ++++++++++++++----- 2 files changed, 59 insertions(+), 16 deletions(-) diff --git a/src/AcDream.App/World/InventoryWorldDropProjectionController.cs b/src/AcDream.App/World/InventoryWorldDropProjectionController.cs index 8397fa02..40db242e 100644 --- a/src/AcDream.App/World/InventoryWorldDropProjectionController.cs +++ b/src/AcDream.App/World/InventoryWorldDropProjectionController.cs @@ -179,12 +179,28 @@ internal sealed class PendingSplitToWorldProjection Position = update.Position, Parent = null, Velocity = update.Velocity, + // #314: every timestamp this record projects to the flattened + // spawn fields below MUST be reset here in the same breath. + // HasConsistentCreateIdentityAndParent + // (RuntimeEntityObjectLifetime) rejects a create whose + // PhysicsDesc timestamps disagree with their flattened + // projections, and the `source with { … }` below zeroes + // MovementSequence/ServerControlSequence. Omitting the + // matching Movement/ServerControlledMove resets here left a + // split whose SOURCE carried nonzero movement stamps — any + // item dropped once, picked back up, and split again — + // failing that predicate and throwing instead of completing + // the canonical create-placement transaction. A fresh split + // GUID has no movement history by construction, so zero is + // the honest value, not a placation of the predicate. Timestamps = sourcePhysics.Timestamps with { Position = update.PositionSequence, Teleport = update.TeleportSequence, ForcePosition = update.ForcePositionSequence, Instance = update.InstanceSequence, + Movement = 0, + ServerControlledMove = 0, }, } : null; diff --git a/tests/AcDream.App.Tests/World/LiveEntityHydrationControllerTests.cs b/tests/AcDream.App.Tests/World/LiveEntityHydrationControllerTests.cs index ef3d063d..3d37fce9 100644 --- a/tests/AcDream.App.Tests/World/LiveEntityHydrationControllerTests.cs +++ b/tests/AcDream.App.Tests/World/LiveEntityHydrationControllerTests.cs @@ -2091,11 +2091,12 @@ public sealed class LiveEntityHydrationControllerTests } [Fact] - public void SplitSourceWithRetainedMovementTimestamps_ThrowsInsteadOfRecovering() + public void SplitSourceWithRetainedMovementTimestamps_StillRecovers() { - // DISCOVERED DEFECT (2026-08-04, C4 route 6 evidence-gathering; NOT - // fixed here -- the contract requires zero production lines and - // directs stop-and-report instead of a speculative fix). + // #314 REGRESSION TEST (defect found 2026-08-04 during C4 route 6's + // evidence-gathering, fixed in the commit immediately after route 6's + // zero-production closure). This test was originally written to + // DOCUMENT the throw; it now pins the fix. // // BuildSpawn resets the top-level MovementSequence / // ServerControlSequence to 0 (InventoryWorldDropProjectionController @@ -2109,13 +2110,19 @@ public sealed class LiveEntityHydrationControllerTests // ServerControlledMove wire update during an earlier stint with // world presence (e.g. dropped once before, picked back up, split // again) carries nonzero values in exactly those two Timestamps - // fields. The resulting spawn then fails - // RuntimeEntityObjectLifetime.HasConsistentCreateIdentityAndParent - // (:2321-2327), and split recovery THROWS instead of completing the - // canonical create-placement transaction -- which is exactly the - // property route 6's "enters the same canonical transaction" claim - // depends on holding unconditionally. See the final report for the - // recommended follow-up. + // fields. The resulting spawn then failed + // RuntimeEntityObjectLifetime.HasConsistentCreateIdentityAndParent, + // and split recovery THREW instead of completing the canonical + // create-placement transaction -- which is exactly the property + // route 6's "enters the same canonical transaction" claim depends on + // holding unconditionally. + // + // The fix resets Movement/ServerControlledMove inside BuildSpawn's + // Timestamps `with` block, alongside the top-level + // MovementSequence/ServerControlSequence zeroing that was already + // there. Zero is the honest value for a fresh split GUID, which has + // no movement history by construction -- not a placation of the + // predicate. using var fixture = new Fixture(originKnown: true); var drop = new DropHarness(fixture); const uint sourceGuid = 0x50000C40u; @@ -2138,11 +2145,31 @@ public sealed class LiveEntityHydrationControllerTests Assert.True(drop.DispatchSplit(sourceGuid, stackSize: 6, splitAmount: 1)); const uint splitResultGuid = 0x80000D01u; - InvalidOperationException exception = Assert.Throws(() => - drop.Projection.TryRecoverUnknownPosition( - DropPositionUpdate(splitResultGuid, x: 30f))); - Assert.Contains( - "inconsistent instance or parent projections", exception.Message); + bool recovered = drop.Projection.TryRecoverUnknownPosition( + DropPositionUpdate(splitResultGuid, x: 30f)); + + // Recovers rather than throwing, and reaches the same canonical + // create-placement transaction the zero-timestamp sibling + // (SplitToWorld_NewGuidPositionRecovery_EntersSameCanonicalTransaction + // AsDrop) asserts — the source's retained movement history must not + // change the outcome at all. + Assert.True(recovered); + Assert.True(fixture.Runtime.TryGetSnapshot( + splitResultGuid, out WorldSession.EntitySpawn spawn)); + Assert.Equal(1, spawn.StackSize); + Assert.Equal(0u, spawn.ContainerId); + Assert.True(fixture.Runtime.TryGetRecord( + splitResultGuid, out LiveEntityRecord record)); + Assert.NotNull(record.WorldEntity); + + // The positive half (#314's actual fix): the split result's movement + // channels are zeroed in BOTH projections, not merely consistent with + // each other. Asserting only "it didn't throw" would pass against a + // fix that made HasConsistentCreateIdentityAndParent lenient instead. + Assert.Equal(0, spawn.MovementSequence); + Assert.Equal(0, spawn.ServerControlSequence); + Assert.Equal(0u, spawn.Physics!.Value.Timestamps.Movement); + Assert.Equal(0u, spawn.Physics.Value.Timestamps.ServerControlledMove); } [Fact]