From 975f89c8705987663f954ef5dc84d7825cec70f0 Mon Sep 17 00:00:00 2001 From: Erik Date: Mon, 22 Jun 2026 20:12:54 +0200 Subject: [PATCH] fix(D.2b): harden optimistic-move pending map (Opus review I1+I2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit I2 (real bug): Clear() (teleport/logoff) and Remove() (item destroyed) now drop the item's _pendingMoves entry — a stale snapshot on a recycled guid could otherwise mis-rollback a different future item. I1 (race): track an OUTSTANDING count per item so an early ConfirmMove (0x0022) for the first of several in-flight moves of the SAME item can't clear the snapshot while a later move is unconfirmed — a later reject can still roll back to the original instead of stranding the item at the rejected position. Both with regression tests; full suite green. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/AcDream.Core/Items/ClientObjectTable.cs | 31 ++++++++++++++----- .../Items/ClientObjectTableTests.cs | 26 ++++++++++++++++ 2 files changed, 49 insertions(+), 8 deletions(-) diff --git a/src/AcDream.Core/Items/ClientObjectTable.cs b/src/AcDream.Core/Items/ClientObjectTable.cs index 238f0eb7..73a40315 100644 --- a/src/AcDream.Core/Items/ClientObjectTable.cs +++ b/src/AcDream.Core/Items/ClientObjectTable.cs @@ -47,7 +47,7 @@ public sealed class ClientObjectTable // B-Drag: pre-move snapshots for optimistic inventory moves. itemId → (container, slot) BEFORE // the optimistic MoveItem; restored by RollbackMove on InventoryServerSaveFailed (0x00A0), // cleared by ConfirmMove on the InventoryPutObjInContainer (0x0022) echo. - private readonly Dictionary _pendingMoves = new(); + private readonly Dictionary _pendingMoves = new(); /// Fires when an object is first added to the session. public event Action? ObjectAdded; @@ -144,17 +144,30 @@ public sealed class ClientObjectTable public bool MoveItemOptimistic(uint itemId, uint newContainerId, int newSlot) { if (!_objects.TryGetValue(itemId, out var item)) return false; - if (!_pendingMoves.ContainsKey(itemId)) - _pendingMoves[itemId] = (item.ContainerId, item.ContainerSlot); // snapshot the ORIGINAL once + // Snapshot the ORIGINAL position once + count OUTSTANDING optimistic moves of this item. The + // count stops an early confirm (0x0022) for the first of several in-flight moves of the SAME + // item from clearing the snapshot while a later move is still unconfirmed — else a later + // reject would find nothing pending and strand the item at the rejected position. + if (_pendingMoves.TryGetValue(itemId, out var p)) + _pendingMoves[itemId] = (p.container, p.slot, p.outstanding + 1); + else + _pendingMoves[itemId] = (item.ContainerId, item.ContainerSlot, 1); return MoveItem(itemId, newContainerId, newSlot); } - /// The server confirmed the move (InventoryPutObjInContainer 0x0022 echo) — drop the - /// pending snapshot. No-op for a server-initiated move we never tracked. - public void ConfirmMove(uint itemId) => _pendingMoves.Remove(itemId); + /// The server confirmed a move (InventoryPutObjInContainer 0x0022 echo) — decrement the + /// outstanding count; drop the snapshot only once no optimistic moves of this item remain. No-op + /// for a server-initiated move we never tracked. + public void ConfirmMove(uint itemId) + { + if (!_pendingMoves.TryGetValue(itemId, out var p)) return; + if (p.outstanding <= 1) _pendingMoves.Remove(itemId); + else _pendingMoves[itemId] = (p.container, p.slot, p.outstanding - 1); + } - /// The server rejected the move (InventoryServerSaveFailed 0x00A0) — restore the item to - /// its pre-move (container, slot) and drop the snapshot. False if nothing was pending. + /// The server rejected a move (InventoryServerSaveFailed 0x00A0) — restore the item to its + /// pre-move (container, slot) and drop the snapshot entirely (the server's next snapshot reconciles + /// any still-outstanding moves). False if nothing was pending. public bool RollbackMove(uint itemId) { if (!_pendingMoves.TryGetValue(itemId, out var pre)) return false; @@ -171,6 +184,7 @@ public sealed class ClientObjectTable if (!_objects.TryRemove(itemId, out var item)) return false; if (item.ContainerId != 0 && _containerIndex.TryGetValue(item.ContainerId, out var l)) l.Remove(itemId); + _pendingMoves.Remove(itemId); // a destroyed item must not leave a snapshot that mis-rolls-back a recycled guid ObjectRemoved?.Invoke(item); return true; } @@ -430,5 +444,6 @@ public sealed class ClientObjectTable _objects.Clear(); _containers.Clear(); _containerIndex.Clear(); + _pendingMoves.Clear(); // B-Drag: drop in-flight optimistic snapshots (a recycled guid must not mis-rollback) } } diff --git a/tests/AcDream.Core.Tests/Items/ClientObjectTableTests.cs b/tests/AcDream.Core.Tests/Items/ClientObjectTableTests.cs index dc239fda..188370c6 100644 --- a/tests/AcDream.Core.Tests/Items/ClientObjectTableTests.cs +++ b/tests/AcDream.Core.Tests/Items/ClientObjectTableTests.cs @@ -396,6 +396,32 @@ public sealed class ClientObjectTableTests public void RollbackMove_unknownItem_false() => Assert.False(new ClientObjectTable().RollbackMove(0xDEADu)); + [Fact] + public void MoveItemOptimistic_twice_oneConfirm_stillRollsBack() // I1: an early confirm must not strand the item + { + var table = new ClientObjectTable(); + table.Ingest(FullWeenie(0x910u, container: 0xC0u)); table.MoveItem(0x910u, 0xC0u, 5); + table.MoveItemOptimistic(0x910u, 0xC1u, 0); // move 1 (outstanding 1, snapshot C0/5) + table.MoveItemOptimistic(0x910u, 0xC2u, 0); // move 2 (outstanding 2) + table.ConfirmMove(0x910u); // server confirms move 1 → outstanding 1, STILL pending + Assert.True(table.RollbackMove(0x910u)); // move 2 rejected → can still roll back (not stranded at C2) + Assert.Equal(0xC0u, table.Get(0x910u)!.ContainerId); + Assert.Equal(5, table.Get(0x910u)!.ContainerSlot); + } + + [Fact] + public void Clear_dropsPendingMoves_soRecycledGuidIsntMisRolledBack() // I2: pending must not survive a table flush + { + var table = new ClientObjectTable(); + table.Ingest(FullWeenie(0x911u, container: 0xC0u)); + table.MoveItemOptimistic(0x911u, 0xC1u, 0); // pending snapshot for 0x911 + table.Clear(); // teleport/logoff flushes the table + + table.Ingest(FullWeenie(0x911u, container: 0xC5u)); // a recycled guid reappears fresh + Assert.False(table.RollbackMove(0x911u)); // no stale pending → no mis-rollback + Assert.Equal(0xC5u, table.Get(0x911u)!.ContainerId); + } + [Fact] public void ContainerIndex_SlotChange_ResortsInPlace() // guards the Reindex same-container early-out {