fix(vendor): Slice 6 review corrections — ownership-checked retire, live slider display, drag-proof shop rows, hardened buy reservation
Some checks are pending
Headless portability / portable-headless (ubuntu-latest) (push) Waiting to run
Headless portability / portable-headless (windows-latest) (push) Waiting to run
Headless portability / linux-graphical (push) Waiting to run
Headless portability / linux-vulkan (push) Waiting to run

All nine findings from the buy-arc review, at root:

F1 the materializer's retire pass re-checks ownership (guid->vendorId
map; remove only while the live object's ContainerId still equals the
recording vendor) — buying a player-sold UNIQUE no longer deletes the
item you just purchased; the discriminating reparent-then-refresh test
pins it. F2 the cost/name display subscribes to the live split state
and shares ONE quantity computation with Buy (retail re-renders per
slider tick: RecvNotice_StackSliderChanged 0x004C4500) — the sentence
and the charge can no longer disagree. F3 shop rows never mint drag
payloads (UiItemSlot.AllowDragSource gates both IsDragSource AND
GetDragPayload — the second gate was caught by this pass's own test).
F4 sendBuy reports whether anything was sent; a null-session buy
cancels the reservation instead of leaking BusyCount forever.
F5 the retire loop snapshots, isolates per-guid observer failures, and
clears its tracking in finally and Dispose — teardown convergence can
no longer wedge. F6 auto-select is retail's unconditional
first-filtered-item shape (pc:201180-201184; the survival-check was
our invention and the comment claiming otherwise is corrected).
F7 non-stack buys clamp to quantity 1 locally (BuySingleItem
pc:201669). F8 the Add button is hard-disabled until staging exists.
F9 AP-161/162/163 rewritten to the post-fix reality.

Clean-room complete solution: 11,378 passed / 4 skipped / 0 failed.
The #350 render-ledger overflow observed this session is under
separate investigation and is NOT addressed here.

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
This commit is contained in:
Erik 2026-08-07 23:12:50 +02:00
parent 97cf873870
commit 3c9fc57adb
11 changed files with 670 additions and 116 deletions

View file

@ -86,12 +86,40 @@ namespace AcDream.Runtime.Gameplay;
/// already was, which is safe by construction and never corrupts a real
/// object's ownership.
/// </para>
///
/// <para>
/// <b>Ownership re-check on retire (review finding F1).</b> Buying a
/// UNIQUE vendor item does not merely drop it from the next
/// <c>ApproachVendor</c> snapshot — ACE first re-containers the SAME guid
/// into the BUYER's own pack via <c>CreateObject</c>
/// (<c>Player_Commerce.cs:86-108</c>, A.2 of the Slice 6 research doc) and
/// only THEN sends the full-replace refresh that no longer lists it. If
/// the retire pass below removed every guid merely absent from the new
/// snapshot, it would delete the just-purchased item straight back out of
/// the buyer's own inventory the instant the post-buy refresh landed. Each
/// owned guid therefore remembers the vendor id it was registered under,
/// and the retire pass only calls <see cref="ClientObjectTable.Remove"/>
/// when the LIVE object's current <c>ContainerId</c> still equals that
/// recorded vendor id — i.e. nothing else has re-containered it since.
/// When it no longer matches (a purchase moved it to the buyer, or some
/// other owner claimed it), the tracking entry is dropped silently and the
/// object itself is left completely untouched, mirroring the collision
/// policy above.
/// </para>
/// </summary>
public sealed class VendorShopItemMaterializer : IDisposable
{
private readonly VendorState _vendor;
private readonly ClientObjectTable _objects;
private readonly HashSet<uint> _ownedGuids = new();
/// <summary>
/// Guids this materializer currently owns in <see cref="ClientObjectTable"/>,
/// mapped to the vendor id they were registered under. F1: the retire
/// pass re-checks the live object's <c>ContainerId</c> against this
/// recorded value before deleting anything — see the class doc's
/// "Ownership re-check on retire" section.
/// </summary>
private readonly Dictionary<uint, uint> _ownedGuids = new();
private bool _disposed;
public VendorShopItemMaterializer(VendorState vendor, ClientObjectTable objects)
@ -110,7 +138,7 @@ public sealed class VendorShopItemMaterializer : IDisposable
public int OwnedCount => _ownedGuids.Count;
/// <summary>True if <paramref name="guid"/> is a shop item this materializer put in the table.</summary>
public bool Owns(uint guid) => _ownedGuids.Contains(guid);
public bool Owns(uint guid) => _ownedGuids.ContainsKey(guid);
private void OnVendorTransition(VendorTransition transition)
{
@ -119,40 +147,78 @@ public sealed class VendorShopItemMaterializer : IDisposable
foreach (VendorShopItem item in currentItems)
stillListed.Add(item.ItemGuid);
// Retire every guid we own that fell out of the new snapshot (sold
// out, session closed/reset, or a different vendor superseded this
// one — in every one of those cases stillListed is missing it).
// Runs BEFORE the materialize loop below: "on REPLACE, the old
// vendor's items go before the new ones land."
foreach (uint guid in _ownedGuids)
var nextOwned = new Dictionary<uint, uint>(currentItems.Count);
try
{
if (!stillListed.Contains(guid))
_objects.Remove(guid);
}
var nextOwned = new HashSet<uint>(currentItems.Count);
foreach (VendorShopItem item in currentItems)
{
bool ownedAlready = _ownedGuids.Contains(item.ItemGuid);
if (!ownedAlready && _objects.Get(item.ItemGuid) is not null)
// Retire every guid we own that fell out of the new snapshot (sold
// out, session closed/reset, or a different vendor superseded this
// one — in every one of those cases stillListed is missing it).
// Runs BEFORE the materialize loop below: "on REPLACE, the old
// vendor's items go before the new ones land."
//
// Iterate a SNAPSHOT (F5): ClientObjectTable.Remove synchronously
// fires ObjectRemoved to every subscriber with no per-listener
// isolation (unlike VendorState's own Changed dispatch). A
// throwing external observer must not abort this loop midway and
// strand the remaining guids un-retired.
foreach (KeyValuePair<uint, uint> owned in new List<KeyValuePair<uint, uint>>(_ownedGuids))
{
// Collision guard — see class doc. Never take ownership of a
// guid this materializer did not itself add.
Console.Error.WriteLine(
"[VendorShopItemMaterializer] skipped guid=0x"
+ item.ItemGuid.ToString("X8")
+ " — already present in ClientObjectTable and not "
+ "owned by this vendor session.");
continue;
if (stillListed.Contains(owned.Key))
continue;
// F1 — re-check ownership before removing. A purchase can
// have already re-containered this guid into the buyer's
// pack (see class doc); only retire it if it is STILL the
// vendor's, i.e. the live object's ContainerId still equals
// the vendor id we registered it under. If it moved, drop
// the tracking entry silently and leave the (now
// someone-else's) object completely untouched.
ClientObject? live = _objects.Get(owned.Key);
if (live is null || live.ContainerId != owned.Value)
continue;
try
{
_objects.Remove(owned.Key);
}
catch (Exception error)
{
System.Diagnostics.Trace.TraceError(
"[VendorShopItemMaterializer] ObjectRemoved observer "
+ "threw retiring guid=0x{0}: {1}",
owned.Key.ToString("X8"),
error);
}
}
_objects.Ingest(ToWeenieData(item, transition.VendorId));
nextOwned.Add(item.ItemGuid);
}
foreach (VendorShopItem item in currentItems)
{
bool ownedAlready = _ownedGuids.ContainsKey(item.ItemGuid);
if (!ownedAlready && _objects.Get(item.ItemGuid) is not null)
{
// Collision guard — see class doc. Never take ownership of a
// guid this materializer did not itself add.
Console.Error.WriteLine(
"[VendorShopItemMaterializer] skipped guid=0x"
+ item.ItemGuid.ToString("X8")
+ " — already present in ClientObjectTable and not "
+ "owned by this vendor session.");
continue;
}
_ownedGuids.Clear();
foreach (uint guid in nextOwned)
_ownedGuids.Add(guid);
_objects.Ingest(ToWeenieData(item, transition.VendorId));
nextOwned[item.ItemGuid] = transition.VendorId;
}
}
finally
{
// F5: guaranteed to run even if the retire/materialize passes
// above throw somewhere this method doesn't already catch, so
// _ownedGuids never straddles two inconsistent generations.
_ownedGuids.Clear();
foreach (KeyValuePair<uint, uint> entry in nextOwned)
_ownedGuids[entry.Key] = entry.Value;
}
}
/// <summary>
@ -198,5 +264,12 @@ public sealed class VendorShopItemMaterializer : IDisposable
if (_disposed) return;
_disposed = true;
_vendor.Changed -= OnVendorTransition;
// F5 safety net: RuntimeInventoryState.Dispose() always runs
// Vendor.Reset() first (which drives OnVendorTransition's own retire
// pass down to zero), but a caller that disposes this class directly
// without a prior Reset()/Close() must not leave stale tracking
// entries behind — OwnedCount feeds
// RuntimeInventoryOwnershipSnapshot.IsConverged.
_ownedGuids.Clear();
}
}