diff --git a/Projects/Server.Tests/Tests/Items/DecayRegistrationTests.cs b/Projects/Server.Tests/Tests/Items/DecayRegistrationTests.cs new file mode 100644 index 000000000..0e48977ef --- /dev/null +++ b/Projects/Server.Tests/Tests/Items/DecayRegistrationTests.cs @@ -0,0 +1,232 @@ +using System; +using Server.Items; +using Xunit; + +namespace Server.Tests; + +[Collection("Sequential Server Tests")] +public class DecayRegistrationTests +{ + // The wheel is static and these tests move the clock, so start each from a known schedule. + public DecayRegistrationTests() => DecayScheduler.ResetForTests(); + + // Stands in for a region that refuses decay, e.g. HouseRegion over a locked down item. + private class RefusesDecayItem : Item + { + public RefusesDecayItem() : base(0x1234) + { + } + + public override bool OnDecay() => false; + } + + // The item is already dequeued, so a refusal that does not reschedule strands it forever. + [Fact] + public void ItemWhoseDecayIsRefused_StaysTrackedAndSurvives() + { + var start = Core._now; + + try + { + var item = new RefusesDecayItem(); + item.MoveToWorld(new Point3D(106, 100, 0), Map.Felucca); + + AdvanceDecay(start, item.DecayTime + TimeSpan.FromMinutes(2), item); + + Assert.False(item.Deleted, "A refused decay must not delete the item."); + Assert.True( + DecayScheduler.IsRegistered(item), + "A refused decay must leave the item tracked so it is retried." + ); + + item.Delete(); + } + finally + { + Core._now = start; + } + } + + // Drives the scheduler and the game clock without depending on the shared timer wheel. + private static void AdvanceDecay(DateTime start, TimeSpan duration, Item item) + { + var deadline = start + duration; + + for (var now = start; now <= deadline && !item.Deleted; now += TimeSpan.FromMilliseconds(256)) + { + Core._now = now; + DecayScheduler.ProcessTick(now); + } + } + + [Fact] + public void ItemOnGround_ActuallyDecaysAfterDecayTime() + { + var start = Core._now; + + try + { + var item = new Item(0x1234); + item.MoveToWorld(new Point3D(100, 100, 0), Map.Felucca); + + Assert.True(DecayScheduler.IsRegistered(item)); + + AdvanceDecay(start, item.DecayTime + TimeSpan.FromMinutes(2), item); + + Assert.True(item.Deleted, "Item on the ground must decay once DecayTime elapses."); + } + finally + { + Core._now = start; + } + } + + [Fact] + public void ItemOnGround_DoesNotDecayBeforeDecayTime() + { + var start = Core._now; + + try + { + var item = new Item(0x1234); + item.MoveToWorld(new Point3D(102, 100, 0), Map.Felucca); + + AdvanceDecay(start, item.DecayTime - TimeSpan.FromMinutes(2), item); + + Assert.False(item.Deleted, "Item must not decay before DecayTime elapses."); + + item.Delete(); + } + finally + { + Core._now = start; + } + } + + [Fact] + public void ItemPlacedInWorld_IsRegisteredForDecay() + { + var item = new Item(0x1234); + item.MoveToWorld(new Point3D(100, 100, 0), Map.Felucca); + + Assert.True(item.CanDecay()); + Assert.True(DecayScheduler.IsRegistered(item), "Item on the ground must be tracked for decay."); + + item.Delete(); + } + + // The real client path: Mobile.Lift internalizes, then Mobile.Drop calls MoveToWorld. + [Fact] + public void ItemLiftedThenDroppedToGround_IsRegisteredForDecay() + { + var item = new Item(0x1234); + item.MoveToWorld(new Point3D(100, 100, 0), Map.Felucca); + + // Player lifts the item to the cursor. + item.Internalize(); + Assert.Equal(Map.Internal, item.Map); + + // Player drops it back onto the ground. + item.MoveToWorld(new Point3D(101, 100, 0), Map.Felucca); + + Assert.True(item.CanDecay()); + Assert.True( + DecayScheduler.IsRegistered(item), + "Item dropped to the ground after being lifted must be tracked for decay." + ); + + item.Delete(); + } + + // Held on the cursor means Map.Internal, which must not be tracked. + [Fact] + public void ItemLiftedToCursor_IsNotRegisteredForDecay() + { + var item = new Item(0x1234); + item.MoveToWorld(new Point3D(100, 100, 0), Map.Felucca); + + item.Internalize(); + + Assert.False(item.CanDecay()); + Assert.False(DecayScheduler.IsRegistered(item), "Item held on the cursor must not be tracked for decay."); + + item.Delete(); + } + + // A new item is on Map.Internal, so construction must not touch the scheduler. + [Fact] + public void NewItem_IsNotTracked() + { + var item = new Item(0x1234); + + Assert.Equal(DecayScheduler.SlotNone, item.DecaySlot); + Assert.False(DecayScheduler.IsRegistered(item), "A newly constructed item must not be tracked."); + + item.Delete(); + } + + // Unregister() trusts DecaySlot; IsRegistered() scans the structures. Assert both to catch drift. + [Fact] + public void DecaySlot_AgreesWithStructures_AcrossLifecycle() + { + var item = new Item(0x1234); + + item.MoveToWorld(new Point3D(103, 100, 0), Map.Felucca); + Assert.NotEqual(DecayScheduler.SlotNone, item.DecaySlot); + Assert.True(DecayScheduler.IsRegistered(item)); + + // Lifted to the cursor: untracked, and no stale slot left behind. + item.Internalize(); + Assert.Equal(DecayScheduler.SlotNone, item.DecaySlot); + Assert.False(DecayScheduler.IsRegistered(item)); + + // Back to the ground: tracked again. + item.MoveToWorld(new Point3D(104, 100, 0), Map.Felucca); + Assert.NotEqual(DecayScheduler.SlotNone, item.DecaySlot); + Assert.True(DecayScheduler.IsRegistered(item)); + + item.Delete(); + Assert.False(DecayScheduler.IsRegistered(item), "A deleted item must leave no trace in the scheduler."); + } + + // Contents spilled onto the ground must start decaying. + [Fact] + public void ContainerDestroy_ContentsDroppedToGroundAreTracked() + { + var pack = new Container(0xE75); + pack.MoveToWorld(new Point3D(105, 100, 0), Map.Felucca); + + var item = new Item(0x1234); + pack.AddItem(item); + Assert.False(DecayScheduler.IsRegistered(item)); + + pack.Destroy(); + + Assert.True(item.CanDecay()); + Assert.True(DecayScheduler.IsRegistered(item), "Contents spilled by Container.Destroy must be tracked."); + + item.Delete(); + } + + // Dropping into a container must untrack; taking it back out to the ground must re-track. + [Fact] + public void ItemMovedIntoContainerThenBackToGround_IsRegisteredForDecay() + { + var pack = new Container(0xE75); + pack.MoveToWorld(new Point3D(100, 100, 0), Map.Felucca); + + var item = new Item(0x1234); + pack.AddItem(item); + + Assert.False(DecayScheduler.IsRegistered(item), "Item inside a container must not be tracked for decay."); + + item.Internalize(); + item.MoveToWorld(new Point3D(101, 100, 0), Map.Felucca); + + Assert.True(item.CanDecay()); + Assert.True(DecayScheduler.IsRegistered(item), "Item taken out of a container must be tracked for decay."); + + item.Delete(); + pack.Delete(); + } +} diff --git a/Projects/Server/Items/Container.cs b/Projects/Server/Items/Container.cs index 34dbbbbb1..f139da24b 100644 --- a/Projects/Server/Items/Container.cs +++ b/Projects/Server/Items/Container.cs @@ -437,9 +437,7 @@ public partial class Container : Item for (var i = items.Count - 1; i >= 0; --i) { - var item = items[i]; - item.SetLastMoved(); - item.MoveToWorld(loc, map); + items[i].MoveToWorld(loc, map); } Delete(); diff --git a/Projects/Server/Items/DecayScheduler.cs b/Projects/Server/Items/DecayScheduler.cs index 165c1c274..694d9b6b2 100644 --- a/Projects/Server/Items/DecayScheduler.cs +++ b/Projects/Server/Items/DecayScheduler.cs @@ -22,9 +22,24 @@ namespace Server.Items; /// Timer wheel-based decay scheduler with O(1) registration/unregistration. /// Uses coarse-grained HashSet buckets for time intervals, with a PriorityQueue /// for fine-grained processing of items due within the current window. +/// +/// An item that becomes decay eligible must be passed to +/// once its parent, map and location are final. The scheduler drops items that stop being eligible, +/// but nothing enrolls one that becomes eligible while untracked. +/// +/// +/// and overrides must be stable rather than +/// recomputed per read: is read repeatedly, and a value that +/// changes between reads re-buckets the item every tick and decays it at the wrong time. +/// /// public class DecayScheduler : Timer { + // Item.DecaySlot values; any non-negative value is a bucket index. + internal const sbyte SlotNone = -1; + internal const sbyte SlotOverflow = -2; + internal const sbyte SlotActive = -3; + private const int BucketCount = 12; private static int _maxItemsPerTick; private static TimeSpan _tickInterval; @@ -74,6 +89,65 @@ public class DecayScheduler : Timer { } + /// + /// Test hook: drops all tracked items and re-bases the rotation schedule on + /// , so deadlines left by a test that moved the clock do not suppress + /// rotations in the next one. + /// + internal static void ResetForTests() + { + Shared?.Stop(); + + _activeQueue.Clear(); + _overflowBucket.Clear(); + + for (var i = 0; i < BucketCount; i++) + { + _buckets[i].Clear(); + } + + _currentBucketIndex = 0; + + var now = Core.Now; + _nextBucketRotation = now + _bucketInterval; + _nextOverflowCheck = now + _totalBucketSpan; + } + + /// + /// Diagnostics: true if the item is currently tracked in any bucket, the overflow + /// bucket, or the active queue. + /// + internal static bool IsRegistered(Item item) + { + if (item == null) + { + return false; + } + + if (_overflowBucket.Contains(item)) + { + return true; + } + + for (var i = 0; i < BucketCount; i++) + { + if (_buckets[i].Contains(item)) + { + return true; + } + } + + foreach (var (queued, _) in _activeQueue.UnorderedItems) + { + if (queued == item) + { + return true; + } + } + + return false; + } + /// /// Checks if all decay tracking structures are empty. /// @@ -115,11 +189,13 @@ public class DecayScheduler : Timer if (timeUntilDecay <= _bucketInterval) { _activeQueue.Enqueue(item, decayTime); + item.DecaySlot = SlotActive; } // If beyond the total bucket span, add to overflow bucket else if (timeUntilDecay > _totalBucketSpan) { _overflowBucket.Add(item); + item.DecaySlot = SlotOverflow; } else { @@ -127,6 +203,7 @@ public class DecayScheduler : Timer var bucketOffset = GetBucketOffset(timeUntilDecay); var absoluteIndex = (_currentBucketIndex + bucketOffset) % BucketCount; _buckets[absoluteIndex].Add(item); + item.DecaySlot = (sbyte)absoluteIndex; } } @@ -135,27 +212,26 @@ public class DecayScheduler : Timer /// public static void Unregister(Item item) { - if (item?.Deleted != false) + if (item == null || item.DecaySlot == SlotNone) { return; } - // Check overflow bucket first - if (_overflowBucket.Remove(item)) - { - return; - } + var slot = item.DecaySlot; + item.DecaySlot = SlotNone; - // Check regular buckets - for (var i = 0; i < BucketCount; i++) + if (slot == SlotOverflow) { - if (_buckets[i].Remove(item)) - { - return; - } + _overflowBucket.Remove(item); + } + else if (slot == SlotActive) + { + _activeQueue.Remove(item, out _, out _); + } + else + { + _buckets[slot].Remove(item); } - - _activeQueue.Remove(item, out _, out _); } /// @@ -171,10 +247,11 @@ public class DecayScheduler : Timer return Math.Clamp(bucketOffset, 0, BucketCount - 1); } - protected override void OnTick() + /// + /// Advances the wheel for . Returns true when nothing is left to track. + /// + internal static bool ProcessTick(DateTime now) { - var now = Core.Now; - // Process items from active queue first (smaller queue = faster log(n) operations) ProcessActiveQueue(now); @@ -190,8 +267,13 @@ public class DecayScheduler : Timer ProcessOverflow(now); } + return IsEmpty(); + } + + protected override void OnTick() + { // Stop timer if nothing left to track - if (IsEmpty()) + if (ProcessTick(Core.Now)) { Stop(); return; @@ -220,7 +302,8 @@ public class DecayScheduler : Timer { if (item.Deleted || !item.CanDecay()) { - continue; // Lazy cleanup - item was picked up or deleted + item.DecaySlot = SlotNone; // Lazy cleanup - item was picked up or deleted + continue; } var decayTime = item.ScheduledDecayTime; @@ -233,6 +316,7 @@ public class DecayScheduler : Timer { // Extended beyond total span - move to overflow _overflowBucket.Add(item); + item.DecaySlot = SlotOverflow; } else { @@ -243,11 +327,13 @@ public class DecayScheduler : Timer if (actualIndex != _currentBucketIndex) { _buckets[actualIndex].Add(item); + item.DecaySlot = (sbyte)actualIndex; } else { // Still maps to current bucket - add to active queue _activeQueue.Enqueue(item, decayTime); + item.DecaySlot = SlotActive; } } continue; @@ -255,6 +341,7 @@ public class DecayScheduler : Timer // Due within next window - add to active queue _activeQueue.Enqueue(item, decayTime); + item.DecaySlot = SlotActive; } // Clear the processed bucket @@ -276,10 +363,12 @@ public class DecayScheduler : Timer { if (item.Deleted || !item.CanDecay()) { + item.DecaySlot = SlotNone; return true; // Remove invalid items } - var timeUntilDecay = item.ScheduledDecayTime - now; + var decayTime = item.ScheduledDecayTime; + var timeUntilDecay = decayTime - now; if (timeUntilDecay > _totalBucketSpan) { @@ -289,13 +378,15 @@ public class DecayScheduler : Timer // Now within regular bucket range - move to appropriate bucket if (timeUntilDecay <= _bucketInterval) { - _activeQueue.Enqueue(item, item.ScheduledDecayTime); + _activeQueue.Enqueue(item, decayTime); + item.DecaySlot = SlotActive; } else { var bucketOffset = GetBucketOffset(timeUntilDecay); var actualIndex = (_currentBucketIndex + bucketOffset) % BucketCount; _buckets[actualIndex].Add(item); + item.DecaySlot = (sbyte)actualIndex; } return true; // Remove from overflow @@ -319,6 +410,7 @@ public class DecayScheduler : Timer processed++; _activeQueue.Dequeue(); + item.DecaySlot = SlotNone; if (item.Deleted || !item.CanDecay()) { @@ -333,6 +425,12 @@ public class DecayScheduler : Timer { item.Delete(); } + else + { + // Refused by the region. Restart the clock rather than dropping the item, which has + // already left the queue; re-registering as-is would spin on a due time in the past. + item.SetLastMoved(); + } } } } diff --git a/Projects/Server/Items/Item.cs b/Projects/Server/Items/Item.cs index 3a8adafe1..ee066c657 100644 --- a/Projects/Server/Items/Item.cs +++ b/Projects/Server/Items/Item.cs @@ -208,6 +208,10 @@ public partial class Item : IHued, IComparable, ISpawnable, IObjectPropert private ItemDelta m_DeltaFlags; private Direction m_Direction; private ImplFlag m_Flags; + + // Where DecayScheduler tracks this item. Not serialized; PostDeserialize re-registers. + internal sbyte DecaySlot = DecayScheduler.SlotNone; + private int m_Hue; private int m_ItemID; private Layer m_Layer; @@ -225,12 +229,12 @@ public partial class Item : IHued, IComparable, ISpawnable, IObjectPropert m_ItemID = itemID; Serial = World.NewItem; - Visible = true; - Movable = true; + // Assigned directly: the Visible/Movable setters and SetLastMoved() would register the item + // for decay while m_Map is still null, then unregister it once it is Map.Internal. + m_Flags = ImplFlag.Visible | ImplFlag.Movable; Amount = 1; m_Map = Map.Internal; - - SetLastMoved(); + LastMoved = Core.Now; World.AddEntity(this); } @@ -1107,7 +1111,9 @@ public partial class Item : IHued, IComparable, ISpawnable, IObjectPropert var oldLocation = GetWorldLocation(); var oldRealLocation = m_Location; - SetLastMoved(); + // Register at the end instead: CanDecay() reads parent, map and location, none of which + // are final yet. + LastMoved = Core.Now; if (Parent is Mobile mobile) { @@ -1242,6 +1248,8 @@ public partial class Item : IHued, IComparable, ISpawnable, IObjectPropert Map = map; Location = location; } + + UpdateDecayRegistration(); } /// @@ -2601,7 +2609,9 @@ public partial class Item : IHued, IComparable, ISpawnable, IObjectPropert { var version = reader.ReadInt(); - SetLastMoved(); + // Default for versions without a saved value. Stamp only: map and parent are still unread, + // and PostDeserialize registers every item once its state is final. + LastMoved = Core.Now; switch (version) { diff --git a/Projects/UOContent.Tests/Tests/Items/TreasureChests/TreasureChestDecayTests.cs b/Projects/UOContent.Tests/Tests/Items/TreasureChests/TreasureChestDecayTests.cs new file mode 100644 index 000000000..31a839ca0 --- /dev/null +++ b/Projects/UOContent.Tests/Tests/Items/TreasureChests/TreasureChestDecayTests.cs @@ -0,0 +1,65 @@ +using System; +using Server; +using Server.Items; +using Server.Tests; +using Xunit; + +namespace UOContent.Tests; + +[Collection("Sequential UOContent Tests")] +public class TreasureChestDecayTests +{ + public static TheoryData ChestTypes => + [ + typeof(TreasureChestLevel1), + typeof(TreasureChestLevel2), + typeof(TreasureChestLevel3), + typeof(TreasureChestLevel4) + ]; + + // DecayScheduler reads ScheduledDecayTime repeatedly and needs the reads to agree, so the + // random interval must be rolled once per chest rather than per read. + [Theory] + [MemberData(nameof(ChestTypes))] + public void DecayTimeIsStableAcrossReads(Type chestType) + { + var chest = chestType.CreateInstance(); + + try + { + var expected = chest.DecayTime; + + for (var i = 0; i < 100; i++) + { + Assert.Equal(expected, chest.DecayTime); + } + } + finally + { + chest.Delete(); + } + } + + [Theory] + [MemberData(nameof(ChestTypes))] + public void ScheduledDecayTimeIsStableAcrossReads(Type chestType) + { + var chest = chestType.CreateInstance(); + + try + { + chest.MoveToWorld(new Point3D(150, 150, 0), Map.Felucca); + + var expected = chest.ScheduledDecayTime; + + for (var i = 0; i < 100; i++) + { + Assert.Equal(expected, chest.ScheduledDecayTime); + } + } + finally + { + chest.Delete(); + } + } +} diff --git a/Projects/UOContent/Items/Containers/Strongbox.cs b/Projects/UOContent/Items/Containers/Strongbox.cs index aa5e63925..7d461d715 100644 --- a/Projects/UOContent/Items/Containers/Strongbox.cs +++ b/Projects/UOContent/Items/Containers/Strongbox.cs @@ -32,10 +32,6 @@ public partial class StrongBox : BaseContainer, IChoppable public override int DefaultMaxWeight => 0; - public override bool Decays => _house == null || _owner?.Deleted != false || !_house.IsCoOwner(_owner); - - public override TimeSpan DecayTime => TimeSpan.FromMinutes(30.0); - public void OnChop(Mobile from) { if (_house?.Deleted != false || _owner?.Deleted != false || from == _owner || _house.IsOwner(from)) @@ -50,11 +46,13 @@ public partial class StrongBox : BaseContainer, IChoppable Timer.StartTimer(TimeSpan.FromSeconds(1.0), Validate); } + // A strongbox is only ever its owner's. Without a house, or without an owner still co-owning + // that house, it would be a free container anyone could loot, so it goes away instead. A deleted + // owner deserializes back as null, which IsCoOwner rejects. private void Validate() { - if (_owner != null && _house?.IsCoOwner(_owner) == false) + if (_house?.IsCoOwner(_owner) != true) { - Console.WriteLine("Warning: Destroying strongbox of {0}", _owner.Name); Destroy(); } } diff --git a/Projects/UOContent/Items/TreasureChests/TreasureChestLevel1.cs b/Projects/UOContent/Items/TreasureChests/TreasureChestLevel1.cs index 61dfd853d..929903171 100644 --- a/Projects/UOContent/Items/TreasureChests/TreasureChestLevel1.cs +++ b/Projects/UOContent/Items/TreasureChests/TreasureChestLevel1.cs @@ -72,7 +72,10 @@ public partial class TreasureChestLevel1 : LockableContainer public override bool IsDecoContainer => false; - public override TimeSpan DecayTime => TimeSpan.FromMinutes(Utility.Random(15, 60)); + // Rolled once: DecayScheduler reads ScheduledDecayTime repeatedly and needs a stable value. + private readonly TimeSpan _decayTime = TimeSpan.FromMinutes(Utility.Random(15, 60)); + + public override TimeSpan DecayTime => _decayTime; public override int DefaultGumpID => 0x44; diff --git a/Projects/UOContent/Items/TreasureChests/TreasureChestLevel2.cs b/Projects/UOContent/Items/TreasureChests/TreasureChestLevel2.cs index bc3fe877c..f212d3e71 100644 --- a/Projects/UOContent/Items/TreasureChests/TreasureChestLevel2.cs +++ b/Projects/UOContent/Items/TreasureChests/TreasureChestLevel2.cs @@ -69,7 +69,10 @@ public partial class TreasureChestLevel2 : LockableContainer public override bool IsDecoContainer => false; - public override TimeSpan DecayTime => TimeSpan.FromMinutes(Utility.Random(15, 60)); + // Rolled once: DecayScheduler reads ScheduledDecayTime repeatedly and needs a stable value. + private readonly TimeSpan _decayTime = TimeSpan.FromMinutes(Utility.Random(15, 60)); + + public override TimeSpan DecayTime => _decayTime; public override int DefaultGumpID => 0x42; diff --git a/Projects/UOContent/Items/TreasureChests/TreasureChestLevel3.cs b/Projects/UOContent/Items/TreasureChests/TreasureChestLevel3.cs index 9ec26c7bb..04b4577c9 100644 --- a/Projects/UOContent/Items/TreasureChests/TreasureChestLevel3.cs +++ b/Projects/UOContent/Items/TreasureChests/TreasureChestLevel3.cs @@ -114,7 +114,10 @@ public partial class TreasureChestLevel3 : LockableContainer public override bool IsDecoContainer => false; - public override TimeSpan DecayTime => TimeSpan.FromMinutes(Utility.Random(15, 60)); + // Rolled once: DecayScheduler reads ScheduledDecayTime repeatedly and needs a stable value. + private readonly TimeSpan _decayTime = TimeSpan.FromMinutes(Utility.Random(15, 60)); + + public override TimeSpan DecayTime => _decayTime; public override int DefaultGumpID => 0x42; diff --git a/Projects/UOContent/Items/TreasureChests/TreasureChestLevel4.cs b/Projects/UOContent/Items/TreasureChests/TreasureChestLevel4.cs index a4bbd17bc..12639638d 100644 --- a/Projects/UOContent/Items/TreasureChests/TreasureChestLevel4.cs +++ b/Projects/UOContent/Items/TreasureChests/TreasureChestLevel4.cs @@ -117,7 +117,10 @@ public partial class TreasureChestLevel4 : LockableContainer public override bool IsDecoContainer => false; - public override TimeSpan DecayTime => TimeSpan.FromMinutes(Utility.Random(15, 60)); + // Rolled once: DecayScheduler reads ScheduledDecayTime repeatedly and needs a stable value. + private readonly TimeSpan _decayTime = TimeSpan.FromMinutes(Utility.Random(15, 60)); + + public override TimeSpan DecayTime => _decayTime; public override int DefaultGumpID => 0x42;