erik/acdream
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions 1

fix(D.2b): harden optimistic-move pending map (Opus review I1+I2)

Browse source 
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) <noreply@anthropic.com>
This commit is contained in:
Erik 2026-06-22 20:12:54 +02:00
parent 8df395894e
commit 975f89c870
2 changed files with 49 additions and 8 deletions
Download patch file Download diff file Expand all files Collapse all files

31
src/AcDream.Core/Items/ClientObjectTable.cs
View file

@ -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<uint, (uint container, int slot)> _pendingMoves = new();
private readonly Dictionary<uint, (uint container, int slot, int outstanding)> _pendingMoves = new();
/// <summary>Fires when an object is first added to the session.</summary>
public event Action<ClientObject>? 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);
}
/// <summary>The server confirmed the move (InventoryPutObjInContainer 0x0022 echo) — drop the
/// pending snapshot. No-op for a server-initiated move we never tracked.</summary>
public void ConfirmMove(uint itemId) => _pendingMoves.Remove(itemId);
/// <summary>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.</summary>
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);
}
/// <summary>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.</summary>
/// <summary>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.</summary>
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)
}
}

26
tests/AcDream.Core.Tests/Items/ClientObjectTableTests.cs
View file

@ -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
{