From b8d3fec59a61e5cfe4e8a8f25b54117276b5175b Mon Sep 17 00:00:00 2001 From: Kamron Batman <3953314+kamronbatman@users.noreply.github.com> Date: Tue, 28 Jul 2026 21:29:03 -0700 Subject: [PATCH] fix(opl): refuse property list invalidation raised from inside GetProperties (#2555) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## The bug Any property getter reached from `GetProperties` that calls `InvalidateProperties` takes the tooltip build down with it: ``` System.ArgumentNullException: Value cannot be null. (Parameter 'array') at Server.ObjectPropertyList.AppendStringDirect(String value) at Server.Mobiles.PlayerMobile.GetProperties(IPropertyList list) ``` `InvalidateProperties` rebuilds **in place** — `Reset()`, then `GetProperties()` again on the same instance — and `Reset()` does two destructive things to a build already in flight: 1. **It returns the pooled interpolation buffer.** The compiler rents it in the handler ctor and returns it in the closing `Add`, so *every hole is evaluated while it is live*: ```csharp var handler = new InterpolatedStringHandler(1, 2, list); // InitializeInterpolation() RENTS handler.AppendFormatted(pl.Rank.Title); // <-- getter runs HERE handler.AppendLiteral("\t"); handler.AppendFormatted(faction.Definition.PropName); list.Add(1060776, ref handler); // consumes span, RETURNS ``` ``` GetProperties(list) ├─ InitializeInterpolation() -> _arrayToReturnToPool = Rent(256) buffer LIVE ├─ « hole 1: pl.Rank.Title » │ └─ PlayerState.Rank.get (lazy recompute) │ └─ Invalidate() -> InvalidateProperties() -> m_PropertyList.Reset() │ └─ Dispose(): Return(buf); _arrayToReturnToPool = null buffer GONE └─ handler.AppendFormatted("Knight") └─ _arrayToReturnToPool.AsSpan(_pos..) └─ ArgumentNullException (Parameter 'array') ``` It surfaces as `ArgumentNullException` rather than `NullReferenceException` because the `Range` overload of `AsSpan` must read `array.Length`, so the BCL null-checks and names the parameter `array`. 2. **It rewinds the packet cursor**, so properties already written are overwritten by the nested pass — a silently corrupted tooltip even where the buffer survives. ## The fix: refuse, don't recover There is no correct recovery, and retrying the build would only hide the defect. A nested invalidation now logs an error with a stack trace, **throws in `DEBUG`** so it gets found and fixed, and in `RELEASE` returns without touching the list — a possibly stale tooltip, but no crash, no corrupted packet, and nothing leaked back to the pool. Getters that genuinely must invalidate should defer: ```csharp Timer.DelayCall(InvalidateProperties); ``` The guard flag lives on the `ObjectPropertyList`, not the entity: it is that list's own lifecycle, it costs nothing (both `Item` and `ObjectPropertyList` absorb it in existing padding, and the list is allocated lazily), and it stays correct when builds for different entities nest. Base instance sizes are unchanged from `main`: Item 128 B, Mobile 792 B, ObjectPropertyList 72 B, PlayerMobile 1216 B. `PropertyList` also publishes the list into `m_PropertyList` **before** building it rather than assigning through `??=` afterwards, so a nested `InvalidateProperties` sees the build in progress instead of recursing into a second throwaway list whose work is discarded. `ObjectPropertyList` re-rents its scratch buffer instead of spanning a null array, so a stray `Reset()` from any other caller degrades rather than aborting `GetProperties`. ## Factions `PlayerState`: maintained, not lazily computed The getter that surfaced this is now a plain field read — the whole `if (m_InvalidateRank)` block and the flag itself are gone: ```csharp public RankDefinition Rank => m_Rank; ``` `UpdateRank()` recomputes at each point an input actually changes: | Site | Why | |---|---| | `RankIndex` setter | this player's index changed | | end of `KillPoints` setter | two paths write `m_RankIndex` directly, bypassing the setter; runs once the swap bookkeeping and `ZeroRankOffset` have settled | | `Faction.AddMember` | *after* the insert — the member count is not settled during the ctor | | `FactionState` load | once ordering and `ZeroRankOffset` are final | Supporting fixes this forced out: - **Both ctors seed the lowest rank.** Nothing recomputes on read any more, so `Rank` has to be usable immediately — including for members that never get a `RankIndex` assigned, which is *every member with no kill points*. Without this, `Rank.Title` NREs. - **`Rank` always resolves.** Ranks are ordered by `Required` descending ending at `0`, so a *negative* percent (`RankIndex` out of sync with `ZeroRankOffset`) matched nothing and left `m_Rank` null. It no longer divides by a zero `ZeroRankOffset` either. - **A pre-existing staleness bug.** The `KillPoints` setter writes `m_RankIndex` directly in two places, so the cached rank was never refreshed when a player crossed zero kill points. All six readers of `Rank` were checked; none relied on the old side effect. One behaviour change worth flagging: rank refreshes are now **eager** where they used to be lazy, so a `KillPoints` change invalidates each swapped player as it happens. The swap loops break as soon as ordering is satisfied — typically 0–2 swaps — but it is on the path that runs on every faction kill. ## Documentation The rule is written down so it is enforceable rather than folklore: - **CLAUDE.md** audit rule 19 - **`dev-docs/property-lists.md`** — new "Never Invalidate From Inside `GetProperties`" section with the failing/passing pattern - **`dev-docs/claude-skills/modernuo-property-lists.md`** — key rule + anti-pattern - **`dev-docs/claude-skills/modernuo-code-audit.md`** — rule 19, ERROR severity ## Tests - `ObjectPropertyListReentrancyTests` — `Reset()` and `Dispose()` re-entered mid-hole (both red against `main` with the exact exception above), nesting behaviour, and the new contract: `DEBUG` throws, `RELEASE` survives, and the build is never retried into a loop. - `FactionRankTests` — `Rank` is populated before anything reads it, tracks `RankIndex` without a read, is stable across reads, and still resolves when `RankIndex` is out of sync with `ZeroRankOffset`. Red-verified: removing the ctor seed fails the first one. 793/793 `Server.Tests` and 608/608 `UOContent.Tests` pass. ## Noted, not addressed here `~ObjectPropertyList()` returns the rented array to `STArrayPool.Shared` from the **finalizer thread**, and that pool is single-threaded by design. Left alone as a separate concern. --- CLAUDE.md | 1 + .../ObjectPropertyListReentrancyTests.cs | 142 ++++++++++++++++++ Projects/Server/Items/Item.cs | 54 ++++++- Projects/Server/Mobiles/Mobile.cs | 52 ++++++- .../Server/PropertyList/ObjectPropertyList.cs | 31 ++++ .../Engines/Factions/FactionRankTests.cs | 100 ++++++++++++ .../Engines/Factions/Core/Faction.cs | 6 +- .../Engines/Factions/Core/FactionState.cs | 7 + .../Engines/Factions/Core/PlayerState.cs | 104 ++++++++----- dev-docs/claude-skills/modernuo-code-audit.md | 11 +- .../claude-skills/modernuo-property-lists.md | 7 + dev-docs/property-lists.md | 60 ++++++++ 12 files changed, 530 insertions(+), 45 deletions(-) create mode 100644 Projects/Server.Tests/Tests/PropertyList/ObjectPropertyListReentrancyTests.cs create mode 100644 Projects/UOContent.Tests/Tests/Engines/Factions/FactionRankTests.cs diff --git a/CLAUDE.md b/CLAUDE.md index 1b52b6816..0c41d2087 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -28,6 +28,7 @@ Apply these when writing or reviewing `.cs` files under `Projects/`. 16. **Prefer switch expressions and switch-when** — use switch expressions for value mapping and switch-when for pattern matching where they improve readability. Exception: skip if unreadable or cold path → `dev-docs/code-standards.md` 17. **No `System.Text.StringBuilder`** — use `ValueStringBuilder` with `stackalloc` (bounded output) or `ValueStringBuilder.Create()` (unbounded). Supports `$"..."` interpolation directly. Always use `using var` for disposal. Use `Reset()` instead of reassigning → `dev-docs/string-handling.md` 18. **Interpolation anti-patterns on handler-aware APIs** — `Send*`/`Say`/`Emote`/`PublicOverhead*`/`IPropertyList.Add`/gump `AddLabel`/`AddHtml`/`Html.Center`/`SpanWriter.Write*` all have `ref RawInterpolatedStringHandler` overloads that allocate zero strings, but only when the call-site argument is a `$"..."` literal directly. Avoid: ternaries with interpolated branches (`Send(c ? $"a" : $"b")`), switch expressions with interpolated arms, pre-built `var s = $"..."` locals (single-use), `.ToString()` / `.String()` / `string.Format` inside holes, string concat (`{a + b}`), LINQ string ops in holes. Use `:L` format spec for lowercase (`{rank:L}` not `rank.ToString().ToLowerInvariant()`) → `dev-docs/string-handling.md` § Interpolation Anti-Patterns +19. **No `InvalidateProperties()` from inside `GetProperties`** — every property a `GetProperties` override reads must be a pure read. `InvalidateProperties()` rebuilds the list in place (`Reset()` + rebuild), and `Reset()` returns the pooled interpolation buffer — which the compiler rents for the whole `$"..."` expression, so every hole is evaluated while it is live — and rewinds the packet cursor. A getter that invalidates therefore throws `ArgumentNullException` (parameter `"array"`) out of `GetProperties` from an unrelated-looking line, or silently corrupts the tooltip. The engine refuses and logs an error; `DEBUG` throws. Lazy recomputation in a getter is fine — the *notification* is not. Invalidate in the setter that changes the value, or defer with `Timer.DelayCall(InvalidateProperties)` → `dev-docs/property-lists.md` § Never Invalidate From Inside `GetProperties` ## Dev-Docs Reference diff --git a/Projects/Server.Tests/Tests/PropertyList/ObjectPropertyListReentrancyTests.cs b/Projects/Server.Tests/Tests/PropertyList/ObjectPropertyListReentrancyTests.cs new file mode 100644 index 000000000..8da5f748a --- /dev/null +++ b/Projects/Server.Tests/Tests/PropertyList/ObjectPropertyListReentrancyTests.cs @@ -0,0 +1,142 @@ +using System; +using Xunit; + +namespace Server.Tests; + +/// +/// The interpolation buffer is rented by the handler ctor and returned by the closing Add, so every +/// hole is evaluated while it is live. A Reset()/Dispose() landing in that window used to leave the +/// next Append* spanning a null array: ArgumentNullException, parameter "array". +/// +public class ObjectPropertyListReentrancyTests +{ + // Stands in for a property getter that invalidates while its own tooltip is being built. + private static string ResettingHole(ObjectPropertyList list, string value) + { + list.Reset(); + return value; + } + + private static string DisposingHole(ObjectPropertyList list, string value) + { + list.Dispose(); + return value; + } + + [Fact] + public void InterpolatedAdd_ResetMidHole_DoesNotThrow() + { + var opl = new ObjectPropertyList(null); + + var ex = Record.Exception( + () => opl.Add(1060776, $"{ResettingHole(opl, "Knight")}\t{"Council of Mages"}") + ); + + Assert.Null(ex); + } + + [Fact] + public void InterpolatedAdd_DisposeMidHole_DoesNotThrow() + { + var opl = new ObjectPropertyList(null); + + var ex = Record.Exception( + () => opl.Add(1060776, $"{DisposingHole(opl, "Knight")}\t{"Council of Mages"}") + ); + + Assert.Null(ex); + } +} + +/// +/// The guard is per-list, so nested builds (a GetProperties override that reads another entity's +/// PropertyList) cannot unguard the outer one the way a single shared slot would. +/// +public class ObjectPropertyListNestedBuildTests +{ + [Fact] + public void NestedBuild_DoesNotUnguardTheOuterList() + { + var outer = new ObjectPropertyList(null); + var inner = new ObjectPropertyList(null); + + outer.IsBuilding = true; + inner.IsBuilding = true; // another entity starts building, and finishes + inner.IsBuilding = false; + + Assert.True(outer.IsBuilding); + } + + [Fact] + public void Reset_MidInterpolation_LeavesTheListUsable() + { + var opl = new ObjectPropertyList(null); + + opl.Add(1060776, $"{Reset(opl, "Knight")}\t{"Council of Mages"}"); + opl.Add(1042971, "still working"); + opl.Terminate(); + + Assert.NotNull(opl.Buffer); + } + + private static string Reset(ObjectPropertyList list, string value) + { + list.Reset(); + return value; + } +} + + +/// +/// Invalidating from inside GetProperties is a defect in the getter, not a case to recover from: +/// DEBUG throws, RELEASE keeps a possibly stale tooltip without crashing or leaking. +/// +[Collection("Sequential Server Tests")] +public class PropertyListInvalidationDuringBuildTests +{ + private class SelfInvalidatingMobile : Mobile + { + public int Builds; + + public override void GetProperties(IPropertyList list) + { + Builds++; + base.GetProperties(list); + InvalidateProperties(); + list.Add(1060776, $"{"Knight"}\t{"Council of Mages"}"); + } + } + + private static SelfInvalidatingMobile Place(int x) + { + var m = new SelfInvalidatingMobile(); + m.MoveToWorld(new Point3D(x, 1000, 0), Map.Felucca); + return m; + } + + [Fact] + public void InvalidatingFromGetProperties_FailsLoudlyWithoutTearingDownTheBuild() + { + var wasEnabled = ObjectPropertyList.Enabled; + ObjectPropertyList.Enabled = true; + + try + { + var m = Place(1000); + +#if DEBUG + Assert.Throws(() => _ = m.PropertyList); +#else + Assert.Null(Record.Exception(() => _ = m.PropertyList)); +#endif + + // Refused, not retried. + Assert.Equal(1, m.Builds); + m.Delete(); + } + finally + { + ObjectPropertyList.Enabled = wasEnabled; + } + } +} diff --git a/Projects/Server/Items/Item.cs b/Projects/Server/Items/Item.cs index 4d5ee13b3..186437400 100644 --- a/Projects/Server/Items/Item.cs +++ b/Projects/Server/Items/Item.cs @@ -14,6 +14,7 @@ *************************************************************************/ using System; +using System.Diagnostics; using System.Collections.Generic; using System.Reflection; using System.Runtime.CompilerServices; @@ -798,7 +799,22 @@ public partial class Item : IHued, IComparable, ISpawnable, IObjectPropert public virtual int HuedItemID => m_ItemID; - public ObjectPropertyList PropertyList => m_PropertyList ??= InitializePropertyList(new ObjectPropertyList(this)); + public ObjectPropertyList PropertyList + { + get + { + if (m_PropertyList == null) + { + // Publish the list before building it so a nested InvalidateProperties can see the + // build in progress and defer instead of recursing into a second throwaway list. + var list = new ObjectPropertyList(this); + m_PropertyList = list; + InitializePropertyList(list); + } + + return m_PropertyList; + } + } /// /// Overridable. Fills an with everything applicable. By default, this invokes @@ -2429,9 +2445,19 @@ public partial class Item : IHued, IComparable, ISpawnable, IObjectPropert private ObjectPropertyList InitializePropertyList(ObjectPropertyList list) { - GetProperties(list); - AppendChildProperties(list); - list.Terminate(); + list.IsBuilding = true; + + try + { + GetProperties(list); + AppendChildProperties(list); + list.Terminate(); + } + finally + { + list.IsBuilding = false; + } + return list; } @@ -2448,6 +2474,26 @@ public partial class Item : IHued, IComparable, ISpawnable, IObjectPropert return; } + // Always a bug in the property getter, and there is no correct recovery: refuse rather than + // hide it. RELEASE keeps a possibly stale tooltip, DEBUG throws. + // See dev-docs/property-lists.md "Never Invalidate From Inside GetProperties". + if (m_PropertyList?.IsBuilding == true) + { + logger.Error( + "{Entity} called InvalidateProperties() while its property list was being built. Remove the side effect from the property getter, or defer it with Timer.DelayCall.\n{StackTrace}", + this, + new StackTrace() + ); + +#if DEBUG + throw new InvalidOperationException( + $"{this} invalidated its property list from inside GetProperties. Remove the side effect from the property getter." + ); +#else + return; +#endif + } + if (m_Map != null && m_Map != Map.Internal && !World.Loading) { int? oldHash; diff --git a/Projects/Server/Mobiles/Mobile.cs b/Projects/Server/Mobiles/Mobile.cs index 34daaf796..f22c9a939 100644 --- a/Projects/Server/Mobiles/Mobile.cs +++ b/Projects/Server/Mobiles/Mobile.cs @@ -26,6 +26,7 @@ using Server.Network; using Server.Prompts; using Server.Targeting; using System; +using System.Diagnostics; using System.Collections.Generic; using System.Runtime.CompilerServices; using Server.Buffers; @@ -2291,7 +2292,22 @@ public partial class Mobile : IHued, IComparable, ISpawnable, IObjectPro public int CompareTo(Mobile other) => other == null ? -1 : Serial.CompareTo(other.Serial); public virtual int HuedItemID => m_Female ? 0x2107 : 0x2106; - public ObjectPropertyList PropertyList => m_PropertyList ??= InitializePropertyList(new ObjectPropertyList(this)); + public ObjectPropertyList PropertyList + { + get + { + if (m_PropertyList == null) + { + // Publish the list before building it so a nested InvalidateProperties can see the + // build in progress and defer instead of recursing into a second throwaway list. + var list = new ObjectPropertyList(this); + m_PropertyList = list; + InitializePropertyList(list); + } + + return m_PropertyList; + } + } public virtual void GetProperties(IPropertyList list) { @@ -7225,8 +7241,18 @@ public partial class Mobile : IHued, IComparable, ISpawnable, IObjectPro private ObjectPropertyList InitializePropertyList(ObjectPropertyList list) { - GetProperties(list); - list.Terminate(); + list.IsBuilding = true; + + try + { + GetProperties(list); + list.Terminate(); + } + finally + { + list.IsBuilding = false; + } + return list; } @@ -7243,6 +7269,26 @@ public partial class Mobile : IHued, IComparable, ISpawnable, IObjectPro return; } + // Always a bug in the property getter, and there is no correct recovery: refuse rather than + // hide it. RELEASE keeps a possibly stale tooltip, DEBUG throws. + // See dev-docs/property-lists.md "Never Invalidate From Inside GetProperties". + if (m_PropertyList?.IsBuilding == true) + { + logger.Error( + "{Entity} called InvalidateProperties() while its property list was being built. Remove the side effect from the property getter, or defer it with Timer.DelayCall.\n{StackTrace}", + this, + new StackTrace() + ); + +#if DEBUG + throw new InvalidOperationException( + $"{this} invalidated its property list from inside GetProperties. Remove the side effect from the property getter." + ); +#else + return; +#endif + } + if (m_Map != null && m_Map != Map.Internal && !World.Loading) { int? oldHash; diff --git a/Projects/Server/PropertyList/ObjectPropertyList.cs b/Projects/Server/PropertyList/ObjectPropertyList.cs index 6404eccbd..58b247990 100644 --- a/Projects/Server/PropertyList/ObjectPropertyList.cs +++ b/Projects/Server/PropertyList/ObjectPropertyList.cs @@ -55,6 +55,12 @@ public sealed class ObjectPropertyList : IPropertyList, IDisposable private int _pos; private char[]? _arrayToReturnToPool; + /// + /// True while GetProperties is populating this list. Set by the owning entity so a nested + /// InvalidateProperties can be refused instead of Reset()ing a build already in flight. + /// + internal bool IsBuilding { get; set; } + public ObjectPropertyList(IEntity? e) { Entity = e; @@ -319,8 +325,23 @@ public sealed class ObjectPropertyList : IPropertyList, IDisposable private static int GetDefaultLength(int literalLength, int formattedCount) => Math.Max(256, literalLength + formattedCount * 11); + // Reset()/Dispose() return the scratch buffer to the pool. If either lands while a `$"..."` + // handler is still appending, re-rent rather than spanning a null array and throwing out of + // GetProperties. Mobile/Item hold the primary guard; this covers any other caller. + [MethodImpl(MethodImplOptions.AggressiveInlining)] + private void EnsureInterpolationBuffer() + { + if (_arrayToReturnToPool == null) + { + _arrayToReturnToPool = STArrayPool.Shared.Rent(256); + _pos = 0; + } + } + public void AppendLiteral(string value) { + EnsureInterpolationBuffer(); + if (value.Length == 1) { var chars = _arrayToReturnToPool.AsSpan(); @@ -354,6 +375,8 @@ public sealed class ObjectPropertyList : IPropertyList, IDisposable public void AppendFormatted(T value) { + EnsureInterpolationBuffer(); + string? s; if (value is IFormattable) { @@ -384,6 +407,8 @@ public sealed class ObjectPropertyList : IPropertyList, IDisposable public void AppendFormatted(T value, string? format) { + EnsureInterpolationBuffer(); + // '#' marks an integer argument as a cliloc ("#"). Integers only -- a float/double/decimal // '#' is the standard numeric format, not a cliloc marker. if (format == "#" && value is int or uint or long or ulong or short or ushort or byte or sbyte) @@ -442,6 +467,8 @@ public sealed class ObjectPropertyList : IPropertyList, IDisposable public void AppendFormatted(ReadOnlySpan value) { + EnsureInterpolationBuffer(); + if (value.TryCopyTo(_arrayToReturnToPool.AsSpan(_pos..))) { _pos += value.Length; @@ -454,6 +481,8 @@ public sealed class ObjectPropertyList : IPropertyList, IDisposable public void AppendFormatted(ReadOnlySpan value, int alignment = 0, string? format = null) { + EnsureInterpolationBuffer(); + var leftAlign = false; if (alignment < 0) { @@ -488,6 +517,8 @@ public sealed class ObjectPropertyList : IPropertyList, IDisposable public void AppendFormatted(string? value) { + EnsureInterpolationBuffer(); + if (value?.TryCopyTo(_arrayToReturnToPool.AsSpan(_pos..)) == true) { _pos += value.Length; diff --git a/Projects/UOContent.Tests/Tests/Engines/Factions/FactionRankTests.cs b/Projects/UOContent.Tests/Tests/Engines/Factions/FactionRankTests.cs new file mode 100644 index 000000000..2416055b1 --- /dev/null +++ b/Projects/UOContent.Tests/Tests/Engines/Factions/FactionRankTests.cs @@ -0,0 +1,100 @@ +using System.Collections.Generic; +using Server; +using Server.Factions; +using Xunit; + +namespace UOContent.Tests; + +/// +/// PlayerState.Rank is read from GetProperties, so it must stay a plain field read. These pin what +/// that requires: the rank is never null, and it is correct without anyone having read it first. +/// +[Collection("Sequential UOContent Tests")] +public class FactionRankTests +{ + // The faction ctor builds its own Definition, so no world state is needed. + private static Faction NewFaction() => new CouncilOfMages(); + + private static PlayerState AddMember(Faction faction, List owner) + { + var state = new PlayerState(new Mobile(), faction, owner); + owner.Add(state); + return state; + } + + [Fact] + public void Rank_IsPopulatedBeforeAnythingReadsIt() + { + var faction = NewFaction(); + var state = new PlayerState(new Mobile(), faction, []); + + // Nothing recomputes on read, so the ctor must leave a usable value or Rank.Title NREs. + Assert.NotNull(state.Rank); + Assert.NotNull(state.Rank.Title); + } + + [Fact] + public void Rank_IsTheLowestRank_ForAnUnrankedMember() + { + var faction = NewFaction(); + var owner = new List(); + var a = AddMember(faction, owner); + var b = AddMember(faction, owner); + + a.UpdateRank(); + b.UpdateRank(); + + var lowest = faction.Definition.Ranks[^1]; + + Assert.Equal(lowest.Rank, a.Rank.Rank); + Assert.Equal(lowest.Rank, b.Rank.Rank); + } + + [Fact] + public void SettingRankIndex_UpdatesRankWithoutAnyoneReadingIt() + { + var faction = NewFaction(); + var owner = new List(); + var top = AddMember(faction, owner); + var bottom = AddMember(faction, owner); + + faction.ZeroRankOffset = 2; + + top.RankIndex = 0; + bottom.RankIndex = 1; + + // No read triggered these, yet the ordering is reflected. + Assert.True(top.Rank.Rank > bottom.Rank.Rank); + } + + [Fact] + public void ReadingRank_IsStableAndSideEffectFree() + { + var faction = NewFaction(); + var owner = new List(); + var state = AddMember(faction, owner); + + faction.ZeroRankOffset = 1; + state.RankIndex = 0; + + var first = state.Rank; + var second = state.Rank; + + Assert.Same(first, second); + } + + [Fact] + public void RankIndexOutOfSyncWithZeroRankOffset_StillResolvesARank() + { + var faction = NewFaction(); + var owner = new List(); + var a = AddMember(faction, owner); + AddMember(faction, owner); + + // A negative percent used to match no rank at all, leaving Rank null. + faction.ZeroRankOffset = 1; + a.RankIndex = 5; + + Assert.NotNull(a.Rank); + } +} diff --git a/Projects/UOContent/Engines/Factions/Core/Faction.cs b/Projects/UOContent/Engines/Factions/Core/Faction.cs index 33166f5a0..6c440584b 100644 --- a/Projects/UOContent/Engines/Factions/Core/Faction.cs +++ b/Projects/UOContent/Engines/Factions/Core/Faction.cs @@ -242,7 +242,11 @@ public abstract class Faction : IComparable, ISpanParsable public virtual void AddMember(Mobile mob) { - Members.Insert(ZeroRankOffset, new PlayerState(mob, this, Members)); + var state = new PlayerState(mob, this, Members); + Members.Insert(ZeroRankOffset, state); + + // Ranked after the insert: the ctor ran while Owner was still short a member. + state.UpdateRank(); mob.AddToBackpack(FactionItem.Imbue(new Robe(), this, false, Definition.HuePrimary)); mob.SendLocalizedMessage(1010374); // You have been granted a robe which signifies your faction diff --git a/Projects/UOContent/Engines/Factions/Core/FactionState.cs b/Projects/UOContent/Engines/Factions/Core/FactionState.cs index 0170c718c..b8eba5091 100644 --- a/Projects/UOContent/Engines/Factions/Core/FactionState.cs +++ b/Projects/UOContent/Engines/Factions/Core/FactionState.cs @@ -114,6 +114,13 @@ public class FactionState } } + // The loop above only assigns RankIndex to members with kill points, and nothing + // computes rank on read, so rank everyone now that the ordering has settled. + foreach (var player in Members) + { + player.UpdateRank(); + } + FactionItems = []; if (version >= 2) diff --git a/Projects/UOContent/Engines/Factions/Core/PlayerState.cs b/Projects/UOContent/Engines/Factions/Core/PlayerState.cs index 792d2b4e3..e420836ce 100644 --- a/Projects/UOContent/Engines/Factions/Core/PlayerState.cs +++ b/Projects/UOContent/Engines/Factions/Core/PlayerState.cs @@ -8,7 +8,6 @@ public class PlayerState : IComparable { private Town m_Finance; - private bool m_InvalidateRank = true; private int m_KillPoints; private MerchantTitle m_MerchantTitle; private RankDefinition m_Rank; @@ -22,6 +21,10 @@ public class PlayerState : IComparable Faction = faction; Owner = owner; + // Owner does not contain this state yet, so the count is short by one; the caller ranks it + // after inserting. + SeedLowestRank(); + Attach(); Invalidate(); } @@ -54,6 +57,9 @@ public class PlayerState : IComparable } } + // Members are still being read; FactionState ranks everyone once the ordering settles. + SeedLowestRank(); + Attach(); } @@ -116,6 +122,8 @@ public class PlayerState : IComparable Owner.Remove(this); Owner.Insert(Faction.ZeroRankOffset, this); + // Direct, not through RankIndex: ZeroRankOffset is mid-update. The + // UpdateRank() at the end of this setter covers it. m_RankIndex = Faction.ZeroRankOffset; Faction.ZeroRankOffset++; } @@ -180,6 +188,7 @@ public class PlayerState : IComparable } m_KillPoints = value; + UpdateRank(); Invalidate(); } } @@ -193,49 +202,72 @@ public class PlayerState : IComparable if (m_RankIndex != value) { m_RankIndex = value; - m_InvalidateRank = true; + + UpdateRank(); + Invalidate(); } } } - public RankDefinition Rank + /// + /// Read from PlayerMobile.GetProperties, so it must stay a plain field read -- recomputing or + /// invalidating here re-enters the property list build. Maintained by . + /// + public RankDefinition Rank => m_Rank; + + // Lowest rank (Required 0): correct for an unranked member, and never null, so Rank.Title + // cannot NRE before the first UpdateRank(). + private void SeedLowestRank() { - get + var ranks = Faction.Definition.Ranks; + + if (ranks.Length > 0) { - if (m_InvalidateRank) + m_Rank = ranks[^1]; + } + } + + /// + /// Recomputes the cached rank. Call whenever , the faction's + /// ZeroRankOffset, or the member count changes -- and only once they have settled. + /// + public void UpdateRank() + { + var ranks = Faction.Definition.Ranks; + + if (ranks.Length == 0) + { + return; + } + + int percent; + + if (Owner.Count == 1) + { + percent = 1000; + } + else if (m_RankIndex == -1 || Faction.ZeroRankOffset <= 0) + { + percent = 0; + } + else + { + percent = (Faction.ZeroRankOffset - m_RankIndex) * 1000 / Faction.ZeroRankOffset; + } + + // Ranks run Required-descending ending at 0, so anything >= 0 matches below. A negative + // percent (RankIndex out of sync with ZeroRankOffset) would otherwise leave it null. + m_Rank = ranks[^1]; + + for (var i = 0; i < ranks.Length; i++) + { + var check = ranks[i]; + + if (percent >= check.Required) { - var ranks = Faction.Definition.Ranks; - int percent; - - if (Owner.Count == 1) - { - percent = 1000; - } - else if (m_RankIndex == -1) - { - percent = 0; - } - else - { - percent = (Faction.ZeroRankOffset - m_RankIndex) * 1000 / Faction.ZeroRankOffset; - } - - for (var i = 0; i < ranks.Length; i++) - { - var check = ranks[i]; - - if (percent >= check.Required) - { - m_Rank = check; - m_InvalidateRank = false; - break; - } - } - - Invalidate(); + m_Rank = check; + break; } - - return m_Rank; } } diff --git a/dev-docs/claude-skills/modernuo-code-audit.md b/dev-docs/claude-skills/modernuo-code-audit.md index 39a8d4983..5d5af15bb 100644 --- a/dev-docs/claude-skills/modernuo-code-audit.md +++ b/dev-docs/claude-skills/modernuo-code-audit.md @@ -191,8 +191,17 @@ mob.SendMessage($"You earned a {rank:L} trophy!"); // "gold" not "Gold" **See**: `dev-docs/string-handling.md` § "Interpolation Anti-Patterns" for the full reference with detailed before/after examples. +### 19. No InvalidateProperties From Inside GetProperties +**Check**: Any property read by a `GetProperties` override — including through helpers — must be a pure read. Flag getters that call `InvalidateProperties()` (or a wrapper like `Invalidate()`) as a side effect. +**Bad**: a `Rank` getter that lazily recomputes and then calls `Invalidate()`; reading it from `GetProperties` re-enters the build. +**Good**: invalidate in the setter that actually changes the value, or defer with `Timer.DelayCall(InvalidateProperties)`. +**Why**: `InvalidateProperties()` rebuilds the list in place (`Reset()` + rebuild). `Reset()` returns the pooled interpolation buffer — which the compiler rents for the whole `$"..."` expression, so every hole is evaluated while it is live — and rewinds the packet cursor. Re-entering mid-build throws `ArgumentNullException` (parameter `"array"`) out of `GetProperties` from a line unrelated to the offending getter, or silently corrupts the tooltip. The engine refuses and logs an error, and `DEBUG` throws, so this shows up as a crash in development. +**Note**: Lazy recomputation inside a getter is fine. It is the notification that must not happen there. + +**See**: `dev-docs/property-lists.md` § "Never Invalidate From Inside `GetProperties`". + ## Severity Levels -- **ERROR**: Rules 3, 9, 10, 13 (will cause bugs, build failures, or client-side leaks) +- **ERROR**: Rules 3, 9, 10, 13, 19 (will cause bugs, build failures, or client-side leaks) - **WARNING**: Rules 1 (Tier 3 LINQ), 2, 4, 5, 6, 7, 8, 12, 14, 15, 17 (performance/convention issues) - **INFO**: Rules 1 (Tier 2 LINQ on warm paths — note it but don't flag as violation), 16 (switch patterns — suggest but don't flag) - **ASK**: Rule 11 (need user input) diff --git a/dev-docs/claude-skills/modernuo-property-lists.md b/dev-docs/claude-skills/modernuo-property-lists.md index 4440c63db..51e47ff35 100644 --- a/dev-docs/claude-skills/modernuo-property-lists.md +++ b/dev-docs/claude-skills/modernuo-property-lists.md @@ -20,6 +20,12 @@ description: > 3. **String interpolation** works with `IPropertyList` -- use `$"..."` syntax 4. **`[InvalidateProperties]`** on `[SerializableField]` auto-refreshes tooltip on change 5. **Call `InvalidateProperties()`** manually when non-serialized state changes tooltip +6. **Never invalidate from inside `GetProperties`** -- every property a `GetProperties` override + reads must be a pure read. `InvalidateProperties()` rebuilds in place (`Reset()` + rebuild), so a + getter with that side effect tears down the list mid-build: it returns the pooled interpolation + buffer under an in-flight `$"..."` handler (`ArgumentNullException`, parameter `"array"`) and + rewinds the packet cursor. The engine refuses and logs an error; `DEBUG` throws. Defer instead: + `Timer.DelayCall(InvalidateProperties)` ## IPropertyList Interface @@ -235,6 +241,7 @@ block.Add("Cannot be repaired".AsSpan()); // plain span, no string alloc - **Excessive rebuilds**: Don't call `InvalidateProperties()` in tight loops - **Assuming tooltip support**: Check `ObjectPropertyList.Enabled` if needed - **One giant `Add()` for multi-line text**: A property over ~512 chars crashes the legacy 2D client. Use `AddChunked`/`OplTextBlock` for variable-length free text +- **Side-effecting property getters**: A getter reached from `GetProperties` that calls `InvalidateProperties()` (directly or via a helper like `Invalidate()`) re-enters the build and is refused — error logged, `DEBUG` throws. Lazy recomputation in a getter is fine; the *notification* is not. Invalidate where the value changes, or `Timer.DelayCall(InvalidateProperties)` ## Real Examples - Item properties: `Projects/Server/Items/Item.cs` (AddNameProperties, GetProperties) diff --git a/dev-docs/property-lists.md b/dev-docs/property-lists.md index 98e26efc1..929429bcd 100644 --- a/dev-docs/property-lists.md +++ b/dev-docs/property-lists.md @@ -301,6 +301,66 @@ public void UseCharge() } ``` +### Never Invalidate From Inside `GetProperties` (CRITICAL) + +`InvalidateProperties()` rebuilds the list **in place** — `Reset()`, then `GetProperties()` again on +the same instance. Calling it from a property getter that the build itself reaches is therefore +re-entrant, and `Reset()` does two destructive things to the build in flight: + +1. It returns the pooled interpolation scratch buffer. The compiler rents that buffer in the + interpolated-string handler's constructor and returns it in the closing `Add`, so **every hole is + evaluated while the buffer is live**. Pulling it out mid-append makes the next `Append*` span a + null array — `ArgumentNullException: Value cannot be null. (Parameter 'array')` thrown out of + `GetProperties`, from a line that looks unrelated to the getter that caused it. +2. It rewinds the packet cursor, so properties already written are overwritten by the nested pass. + +This is always a defect in the property getter, so the engine refuses rather than trying to recover: +a nested call logs an error with a stack trace, throws in `DEBUG`, and in `RELEASE` returns without +touching the list — leaving a possibly stale tooltip, but never a crash, a corrupted packet, or a +leaked pool buffer. Retrying the build would only hide the bug. + +```csharp +// BAD -- a getter with a side effect. Reading it from GetProperties re-enters the build. +public RankDefinition Rank +{ + get + { + if (_invalidateRank) + { + _rank = Recompute(); + _invalidateRank = false; + Invalidate(); // -> InvalidateProperties() -> Reset() on the list being built + } + + return _rank; + } +} + +// GOOD -- getters stay side-effect free; invalidate where the value actually changes. +public int RankIndex +{ + get => _rankIndex; + set + { + if (_rankIndex != value) + { + _rankIndex = value; + _invalidateRank = true; + Invalidate(); + } + } +} +``` + +Lazy recomputation inside a getter is fine — it is the *notification* that must not happen there. If +something genuinely must invalidate in response to a read, defer it off the build: + +```csharp +Timer.DelayCall(InvalidateProperties); +``` + +**Check when writing a `GetProperties` override**: every property it reads must be a pure read. + ## ObjectPropertyList Internals Defined in `Projects/Server/PropertyList/ObjectPropertyList.cs`: