953 commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
Kamron Batman
|
24bcfee554
|
feat(network): lean base pools (#2641)
Some checks failed
Build / Build (MacOS 15) (push) Has been cancelled
Build / Build (MacOS 26) (push) Has been cancelled
Build / Build (AlmaLinux 10) (push) Has been cancelled
Build / Build (Debian 12) (push) Has been cancelled
Build / Build (Debian 13) (push) Has been cancelled
Build / Build (Fedora 44) (push) Has been cancelled
Build / Build (CentOS 10 Stream) (push) Has been cancelled
Build / Build (CentOS 9 Stream) (push) Has been cancelled
Build / Build (Ubuntu 26) (push) Has been cancelled
Build / Build (Ubuntu 22) (push) Has been cancelled
Build / Build (Ubuntu 24) (push) Has been cancelled
**Follow-on to #2639 (merged). References a local IORingGroup `1.0.13-preview.11` pack until 1.0.13 (modernuo/IORingGroup#15) is published; do not merge before that switch.** ## Summary Consumes IORingGroup's lean base pools (modernuo/IORingGroup#15): both network pools now start with one slab, grow a slab at a time with the population, and trim idle slabs back after quiet periods. - Fixes the transport's send-pool cap: previously only 1024 of the 4096 connections could get a send buffer; connection 1025 was closed at accept. - Network memory at boot drops from about 96 MB to about 10 MB at the defaults; a full 4096 logged-in connections is about 1.25 GB of base buffers plus the growth budget. - New settings: `network.initialBufferSlabs` (default 1; slabs of each pool held from boot and the trim floor) and `network.maxBufferSlabs` (default 128; divides the connection maximum into slabs, 32 connections per slab). Both are coerced with a warning; the same value feeds the ring table and the manager so they cannot drift. - The Debug-only maintenance line includes base-pool capacity and releases. - `dev-docs/server-requirements.md` rewrites the network memory story and adds the two settings. ## Pre-auth buffers Every connection starts on the transport's platform-minimum buffers (4 KB receive, 4 KB send) instead of the base pools. It is promoted to full-size buffers (64 KB receive, `network.sendBufferSize` send) when the game server verifies its account — the point where `NetState.Account` is assigned in the `GameServer_AwaitingGameServerLogin` or `GameServer_LoggedIn` state, so a verdict that lands after the parser has moved on still promotes. Nothing ever moves back. The login-server pass stays on the small buffers for its whole lifetime. Before credentials verify, nothing promotes: the 4 KB send ring is the entire pre-auth send budget, and a connection that overruns it is dropped as exhausted, exactly as the receive side drops a packet header declaring more bytes than the receive buffer can hold (new guard in `HandlePacket`; it also closes the old 65535-byte edge on 64 KB buffers). The stock login sequence sends under 2 KB. After credentials verify, the send path promotes on demand if it ever needs to (unbudgeted, outside the memory ceiling and the shrink bookkeeping — promotion is not growth), and the oversize-packet guard waits on a pending receive promotion or retries a stalled one once for a verified account before disconnecting. A completion that fills the receive buffer arms no receive, so `HandleReceive` now calls `RingSocket.ResumeReceive()` after the parse loop — at 4 KB a burst of small packets fills the buffer in one completion. Net effect: a flood of unauthenticated connections tops out at about 32 MB across the full 4096-connection cap where the platform minimum is 4 KB (the transport's retained slabs and the base pools used by logged-in players are separate), and never allocates a base-pool slab. Platform note: on Windows Server 2012 R2 / 2016 the transport's legacy mapping path floors at 64 KB: the pre-auth receive pool is off there (its base is 64 KB), while the pre-auth send buffer starts at 64 KB under the 256 KB base. The server logs the effective sizes at startup. ## Testing Server.Tests (905) and UOContent.Tests green on the preview pack (one pre-existing `FamiliarAITests` failure from #2644 reproduces on `main`, tracked separately). New tests cover both coercions, that the ring's registration table equals `RequiredRegisteredBuffers` for the configured values, promotion on game-server auth and on a late account, no promotion on the login server, pre-credential overrun ending in exhaustion, post-credential on-demand promotion, the oversize-packet guard through loopback (error, wait, retry with an account, and a promotion made pending mid-parse), and receiving again after a burst fills the initial buffer. |
||
|
Kamron Batman
|
31cd19b05b
|
feat(network): grow the send buffer on demand instead of disconnecting (#2639)
## Problem A connection's send buffer is a fixed 256 KB. A burst of world traffic (a crowded area, a mass spawn, a war) that outruns the client's acknowledgements fills it, `NetState.Send()` reports "send buffer exhausted", and the player is disconnected. Raising the size for everyone multiplies the per-connection footprint (4096 × 256 KB is already 1 GB at full occupancy, page-locked on Windows). ## What changes - **Growth.** When a packet does not fit (the write span is too small, the packet is larger than the span, or compression returns 0), `Send()` asks the transport to grow the buffer to the next power-of-two tier and retries, up to `network.sendBufferMaxSize` (2 MB). Compression retries once per tier since its output size is not known in advance, including when the buffer is completely full. Only when growth is refused does the existing exhaustion disconnect run. The success path is unchanged. - **Memory ceiling.** Growth is refused (with a once-a-minute warning) when the process working set exceeds `network.memoryCeilingPercent` (80) of the memory available to the process (container-aware; `0` turns the check off). The figure is sampled at startup and refreshed each maintenance tick. - **Shrink.** A grown socket returns to the base buffer once it is drained and 30 s have passed since its last growth, attempted from the `DataSent` handler and from the 5 s alive sweep. - **Retention.** Every minute a timer calls the transport's `Maintain()`, which trims idle tier slabs down to the peak concurrent usage of the last 15 minutes, so recurring bursts reuse buffers without allocation while rare ones give the memory back. The line logs at Debug, and only when capacity, usage, or the floor changed or a growth was refused (budget, at max, or ceiling), so an idle shard logs nothing. - **Budget.** `network.sendBufferGrowthBudget` (256 MB) caps the tier pools' capacity; a positive value below one tier slab is raised with a warning, a negative one is clamped to 0 (growth off). Worst case is base × connections plus the budget. - Settings are coerced with accurate warnings (power of two, minimum, 256 MB transport ceiling). `[dumpnetstates` gains the send buffer size. `dev-docs/server-requirements.md` describes the new memory story. ## Tests `NetStateSendBufferTests` (real loopback sockets): growth instead of disconnect with a byte-exact stream, compressed growth against the compressor's own output, the grow-then-copy path, growth with a send genuinely in flight, refusal past the maximum, refusal under the ceiling, refusal on a closing socket, shrink after the hold (direct and through the alive sweep), and the setting coercions. Server.Tests 891 passed, UOContent.Tests 1052 passed against the published 1.0.12. Reviewed per task, whole-branch, and adversarially by a second model (twice, the second time jointly with the transport branch); all findings addressed. |
||
|
Kamron Batman
|
d16166591c
|
fix(network): stop sending once a disconnect is handed to the socket (#2637)
## Problem
When a client's send buffer fills, the `send buffer exhausted` warning repeats for every packet, every tick, until the NetState is finally disposed. Before the io_uring transport an overflow produced one message plus the disconnect line.
## Root cause
Since #2315, `NetState.Disconnect()` only queues. The NetState keeps running, the Mobile stays attached, and in `Slice()` the queued disconnect becomes `RingSocket.Disconnect()`, which sees buffered or in-flight sends and merely sets `DisconnectPending` while the transport drains. Nothing stopped game logic from writing into that buffer afterwards, so:
- every broadcast to that player still reached `Send()`, hit the full buffer, and re-reported exhaustion (made visible by #2551);
- refills kept `ReadableBytes` above zero, so the graceful drain could never finish, and `DataSent` completions kept pushing the alive check out. A slow-but-acking client could keep a "disconnected" session attached indefinitely.
Two more sources of the same warning surfaced during the analysis: `Dispose()` sets `_running = false` before nulling `Mobile.NetState`, and the setter's bank-close / target-cancel packets then reported `0 writable`; and when the socket takes the immediate-close branch (nothing in flight) `Connected` drops without `DisconnectPending`, leaving a one-tick window that also re-reported.
## Changes
- `CannotSendPackets()` refuses once the socket is `DisconnectPending` or no longer `Connected`. Sends between `Disconnect()` and the `Slice()` handoff are still delivered (kicks with a message, the play-server ack).
- `SendBufferExhausted()` reports once per disconnect and keeps the first reason.
- `Send()` is silent while closing instead of reporting exhaustion for a socket that is going away.
- `_nextAliveCheck` is seeded from a real tick (the zero default suppressed the alive sweep on hosts whose counter starts negative).
- `CancelAllTrades()` had its null guard inverted since
|
||
|
Kamron Batman
|
309fcfeb27
|
feat(skills): SkillEvents.SkillUsed for cross-assembly subscribers; InternalsVisibleTo ModernSpawner.Tests (#2636)
## Summary
Two small additive changes that an external content assembly (ModernSpawner) needs, as separable commits.
**1. `SkillEvents.SkillUsed`** (`Projects/UOContent/Skills/SkillEvents.cs`, namespace `Server.Misc`): a plain C# event `Action<Mobile, Skill, bool success>` raised once per skill attempt from each of the four `Mobile_SkillCheck*` handlers, with the handler's own result. Attempts the handler resolves without a roll (too difficult, no challenge) raise too, so a grandmaster's trivial success and a guaranteed combat roll are observable. Not raised when the mobile lacks the skill. Each handler keeps its logic in a private core method and raises on the way out, so there is exactly one raise per attempt and `CheckSkill` itself is unchanged.
- **Why a plain event and not a `[GeneratedEvent]`:** generated events are compile-time static dispatch inside the UOContent compilation, so a subscriber in another assembly cannot use `[OnEvent]`. Shape follows `HelpEvents`.
- **Why "used", not "gained":** this is the XmlSpawner skill-trigger semantic (it wrapped the same four handlers and passed their result as `success`; its grammar was `Skill[+/-]` for success-only or failure-only). Gains are already observable through the existing skill-change notification on `Mobile`.
- **Cost:** one delegate null-check per attempt when nothing is subscribed; no boxing, no closure, no allocation. The handlers sit on the combat swing path.
- **Exception contract:** subscriber exceptions propagate, matching `EventSink`/`HelpEvents`; no try/catch by design.
**2. `InternalsVisibleTo("ModernSpawner.Tests")`** on `Server.csproj`, beside the existing `Server.Tests`/`UOContent.Tests` entries, so an external test host can seed `Core._now` the way the engine's own test initializers do. Separable; a public test seam on `Core` would serve the same need without naming a downstream assembly.
## Open question
The payload is the `Skill` object plus a positional `bool`. A `readonly struct` args type passed `in` would leave room to add `chance` or the target later without breaking subscribers. Happy to change before merge.
## Test plan
- [x] `UOContent.Tests`: 4 tests — a rolled attempt raises once with the returned outcome; each short-circuit path (no challenge, too difficult, on both the direct and value-window handlers) raises with the handler's result; a direct `CheckSkill` call does not raise; no subscriber does not throw. Full suite green.
- [x] `Server` and `UOContent` build clean with `TreatWarningsAsErrors`.
- [ ] CI
|
||
|
Kamron Batman
|
7deb8b082e
|
fix: Bumps dependencies (#2638) | ||
|
Kamron Batman
|
c9831b9680
|
fix: TextDefinition was uneditable in the props gump, and where parsed constants its own way (#2624)
## Pressing `>` on a TextDefinition did nothing useful
`#1217` moved `TextDefinition` into the Server project for serialization support and added `[PropertyObject]` along the way. `PropsGump` checks that attribute **before** its parsable fallback, so from that commit on the button either drilled into a read-only `Number`/`String` page (both get-only since `#1221`, so no `>` buttons — a dead end) or, when the value was null, re-sent the same page and looked inert.
Worth being clear that `#765` — which replaced the hand-maintained type list with a generic `IsParsable` branch — was **not** the regression. At that commit `TextDefinition` was `[Parsable]` only, fell through to the new branch, and the editor worked. Only the later attribute hijacked the routing.
`TextDefinition` is now caught ahead of the `[PropertyObject]` branch and routed to `SetGump`, which has had `TextDefinition`-specific code at line 35 all along.
## `0` and `"0"` could not be told apart
`Commands.Split` strips quotes before any parser runs, so this was never fixable in `TextDefinition.Parse` alone — the information is gone two layers earlier. The markers now travel inside the value:
| Input | Result |
|---|---|
| `1060847` | cliloc (unchanged) |
| `#1060847` | cliloc, explicit — the form `ToString()` already writes |
| `@"1060847"` | the literal string |
| `hello` | string (unchanged) |
| `(-null-)` | null (unchanged) |
`GetValue()` quotes a string that would not survive the trip back. That check is a real round trip through the parser rather than a pattern match, so it cannot drift from it. `Types.TryParse` decodes the same escape for plain strings — which is what `[get` has always written for the literal `"null"` without `[set` ever reading it back.
**Saved data is untouched.** Serialization reads a discriminated flag plus int/string (`IGenericReader.cs:175`) and the JSON converter switches on token type; neither calls `Parse`, so a stored string `"1060847"` stays a string and spawner JSON is unchanged. The new syntax stays in the text and command layer, where it reaches `[set`, `[add`, spawner props, the props gump and Advanced Search.
## `where` still parsed its constants its own way
`#2625` carried the hand-rolled constant parser across to `PropertyExpressions.Parse` unchanged. It looks for a static `Parse` overload on the property type and gives up if there is none — and `Type` and `IEntity` have neither:
```
where Subject Kind = Static -> Unable to convert string "Static" into type 'System.Type'.
where Subject Owner = 0x1 -> Unable to convert string "0x1" into type 'Server.Mobile'.
```
Both resolve fine for every other command. Routing through `Types.TryParse` picks up type-name lookup, entity resolution by serial, and the `@"..."` literal convention, so a value that works in one place now works everywhere. It also deletes the duplicate parser (−56 lines in `PropertyExpressions`).
Two things are load-bearing and preserved. `where` has always spelled a null constant as a bare `null`, where `[set` uses `(-null-)`; the shared parser reads a bare `null` as the text, so the null case is handled ahead of it and `= null` keeps meaning null. And nullable targets still unwrap first, so `where <int? prop> = 5` is unaffected.
Bare and hex integers, enums, bools, strings, `Map`, and TextDefinition's `#` / `@"..."` markers all resolve exactly as before — 13 of the 15 tests in `WhereConstantParsingTests` pin that and passed before the change as well as after.
## Documentation
The generic command system had no documentation anywhere in the repo: scopes, `where` and its operators, dot notation, `order by` / `distinct` / `limit`, the value and quoting syntax, `[interface` and `[batch` were all learn-by-reading-the-source. `dev-docs/generic-commands.md` covers them, linked from `CLAUDE.md` and `commands-targeting.md`, and points at <https://muo.gg/commands> for the per-command list rather than duplicating it.
Two behaviours it records that were news to me while writing it:
- `Contained` honours conditions on the normal command path but never sets `SupportsConditionals`, and `[batch` is the only place that reads the flag — so the same condition works typed directly and is refused under batch.
- `Multi`, `Single`, `Self` and `Serial` do not parse modifiers at all, so a `where` clause there is passed to the command as ordinary arguments rather than rejected.
Comments in the changed code were trimmed to what is not evident from the code itself; the rationale they carried is either in the doc now or in the commit that introduced it.
## Behavior changes worth a reviewer's attention
Two things go beyond strict bug-fixing, both deliberate:
- **Hex now parses into a TextDefinition.** Consolidating the three `Parse` overloads onto one span codec means `[set Message 0x102CE7` is cliloc 1060847 rather than the string. Previously only the 1-arg `Parse(string)` did hex and nothing called it. This makes the props gump's own `1060847 (0x102CE7)` display typeable.
- **`@"..."` decodes generally for strings**, not only the exact token `@"null"`. So `[set Name @"hello"` sets `hello`. A half-working escape seemed more surprising than a general one, and the `where` path already decoded it — but narrowing it back is a one-line change if preferred.
## Testing
40 tests, each written first and watched fail for the right reason. They cover the props gump routing (populated and null), cliloc/string/quoted parsing and `GetValue` round trips, `where` constant resolution for Type- and entity-valued properties, and the bare-`null` vs `@"null"` distinction that must not drift.
One sort case is added that #2625 left uncovered: ordering a value-typed chain whose intermediate is null, which reads as `default(int)` rather than throwing. Two other tests from the pre-rebase branch were dropped as duplicates of `ConditionalCompilerEdgeTests`.
Rebased onto `
|
||
|
Kamron Batman
|
93e46a88b2
|
fix: OPL revision hash collided on reordered and repeated properties (#2615)
## The bug
`ObjectPropertyList.AddHash` folded each cliloc and a Marvin hash of each argument together with XOR, which is both commutative and self-inverse. Any value mixed in an even number of times cancelled outright, so two properties sharing one argument hashed identically no matter what that argument was:
```
_hash = 31659021
AddHash(1063752); AddHash(hash("10"))
AddHash(1063737); AddHash(hash("10")) // the argument cancels here
AddHash(1063740) // -> 32714560, for ANY argument
```
The 10% and 5% variants of the same item therefore produced the same revision. The client caches the tooltip by revision and only re-requests when it changes, so it kept rendering the stale percentage.
Same root cause, three more shapes:
| | old | new |
|---|---|---|
| Two properties sharing an argument | collides | distinct |
| Properties reordered | collides | distinct |
| Two properties trading arguments | collides | distinct |
| Argument-less property added twice | cancels to 0 | distinct |
## The fix
Hash the finished property block in `Terminate()` instead of accumulating per property. Those bytes — cliloc, length prefix and UTF-16 text, in emission order — are the exact content the client renders, so anything that changes the tooltip changes the hash. The length prefix also removes the concatenation ambiguity the old scheme had.
`AddHash` and both `string.GetHashCode` calls are gone.
## Why 26 bits, and why not 64
The client never recomputes the hash — it stores what we send in 0xD6 and compares it for equality against the 0xDC revision (`ObjectPropertiesListManager.IsRevisionEquals`). So the algorithm is ours to choose, but the width is not:
- Both packets carry a **4-byte** revision, so 64 bits is not available. A wider internal value would be worse than useless: the server would see a change and send a 0xDC whose truncated 32 bits are identical, and the client still wouldn't refresh.
- `Terminate` writes the bare hash into 0xD6 while `SendOPLInfo` writes `Hash` with bit 30 set. The client recovers one from the other by masking off `0x40000000`, which only holds while the hash stays below that bit. Hence 26 bits, unchanged from before.
- `Hash => 0x40000000 + _hash` keeps the revision non-zero; the client parks 0 as its "nothing cached" sentinel.
## Collision behaviour
Enumerated real small-OPL spaces and counted 26-bit collisions against the birthday expectation for a uniform hash:
| Scenario | block | n | collisions | expected |
|---|---|---|---|---|
| 1 cliloc, no arg (every cliloc 1.0M–3.2M) | 6 B | 2,200,000 | 35,591 | ~36,061 |
| 1 cliloc + numeric arg 0–199,999 | 8–12 B | 200,000 | 300 | ~298 |
| 2 clilocs, both with numeric args | 12–24 B | 202,500 | 288 | ~306 |
| 1 cliloc + 1-char arg | 8 B | 4,000,000 | 116,291 | ~119,209 |
Every case lands on the random-model line, and per-bit P(1) across all 26 bits is 0.4962–0.5024 — XXH3's short-input paths still avalanche fully, so a 6-byte block behaves like a uniform 26-bit draw. Masking the low 26 bits versus folding all 64 down was a wash (35,591 vs 35,558).
The metric that matters is narrower, since the client compares a revision only against the previous revision **for the same serial**: 2^-26 = 1.5e-8 per genuine tooltip change. Consecutive-value transitions (charges 50 -> 49) collided 0 times in 200,000.
Row 3 is the old scheme quantified: it collapsed 202,500 inputs into 100,954 distinct hashes, a 50% collision rate — systematic, not probabilistic.
One honest regression: in row 1 the old scheme had zero collisions, because for a single argument-less cliloc under 2^26 the hash was the identity function. An item whose whole tooltip is one argument-less cliloc changing to a different one goes from never colliding to 1.5e-8.
## Performance
Cheaper, not just correct — one xxHash3 pass replaces a Marvin hash per string property over those same bytes. A 12-property, 302-byte tooltip, steady state:
```
old (XOR + Marvin per property) 70.5 ns
xxHash3 (streaming, HashUtility) 27.0 ns
```
`XxHash3.HashToUInt64` one-shot is a further ~7ns faster and produces byte-identical output, but taking it would mean editing `HashUtility.ComputeHash64`, whose values are baked into save files via `AssemblyHandler.GetTypeHash`. Not worth it.
## Second commit: plant old-client property list
Found while tracing the `Hash` read sites. `PlantItem.OldClientPropertyList` called `InitializePropertyList` on every access instead of only when the list was null, and without a `Reset`. Every read appended another copy of every property to the same buffer. `SendOPLPacketTo` and `SendPropertiesTo` both go through the getter, so a pre-7.0.12 client looking at a plant grew the buffer without bound and moved the revision on reads alone.
Now builds once, matching `Item.PropertyList`. `InvalidateProperties` already `Reset`s before rebuilding.
## Testing
`Server.Tests` 869 passed, `UOContent.Tests` 779 passed.
New regression tests, each confirmed failing against the old code first:
- `RepeatedArgument_DoesNotCancelOut` — the reported case
- `PropertyOrder_ChangesHash`, `SwappedArguments_ChangeHash`, `DuplicateProperty_ChangesHash`
- `ShortNumericArguments_ConsecutiveValuesDiffer`, `SmallPropertyBlocks_StayWellDistributed` — short-input avalanche, bounded loosely enough to hold for any seed
- `Hash_StaysWithinTheRevisionMask`, `EmptyList_IsNonZeroAndDistinctFromPopulated` — wire constraints
- `PlantItemPropertyListTests.OldClientPropertyList_BuildsOnceAndIsStableAcrossReads`
|
||
|
Kamron Batman
|
003491472f
|
fix: world save failure handling — no partial snapshots, no worker crash, staged publication (#2612)
Three pre-existing world-save failure-handling bugs, found while reviewing the delta-saves engine (#2610), split out so they land and get adopted on their own before the larger change. #2610 will be rebased on top once this merges. Tracked in project Delta Saves (#7). ## 1. A snapshot missing a segment was published `GenericEntityPersistence.WriteSnapshot` logged a segment (or self-payload) write error and carried on. The save was published without those records, and every entity in the failed segment was deleted at the next load. The error now propagates: `WriteFiles` never moves a partial snapshot over `Saves/`, the previous save stays authoritative, and staff are told. ## 2. A serializer exception killed the process, or published a partial freeze An entity whose `Serialize` threw on a worker thread was an unhandled exception on that thread, which terminates the process. On the inline (main-thread) drain it was caught, but the snapshot was still queued for writing with whatever the workers had produced. Workers now record the first exception and keep draining so the queue empties and the wake/pause handshake completes; after every worker paused, `Snapshot` treats a recorded error (or a failure of the drain itself) as a failed save: nothing is written, the previous save stays, staff are told, and the world resumes. A `WorldSave` handler throwing is logged but does not invalidate the serialized snapshot. ## 3. Publication was not transactional Publishing moved the previous `Saves/` away (`AutoArchive` in `WorldSavePostSnapshot`) and only then moved the new files in. A subscriber throwing after the archive, or the final move failing, left nothing at `Saves/` until the next save; a crash in that window lost the newest save at the next boot. The snapshot now moves to `Saves.next` as soon as its files are complete; only then do subscribers archive the previous save (same event, same `OldSavePath`, so `AutoArchive` and shard subscribers are unchanged), and the staged directory is renamed into place, which is atomic on one volume (file-by-file move across volumes). A staged directory is by construction a complete save newer than `Saves/`, so an interrupted publish is finished at the next boot (before load) or before the next save: whatever sits at `Saves/` is set aside as `Saves.previous-<timestamp>` (never deleted) and the staged save is published, with a warning naming the directory to archive or delete by hand. ## Tests Server.Tests 860 / UOContent.Tests green. New: a throwing serializer is recorded on the worker and the next drain starts clean; a segment that cannot be indexed fails `WriteSnapshot` instead of dropping records; staged-save recovery with and without an existing `Saves/`, and as a no-op. |
||
|
Kamron Batman
|
84153fba58
|
fix: Fixes dirty-tracking gaps in generated content: setters, sub-object owners, BaseVendor (#2609)
## Why Delta world saves re-serialize an entity only when it has been marked dirty. An audit of the generated classes found three ways serialized state changes without a mark; this PR closes the ones that do not need a new generator package. ## What - **Custom `[SerializableProperty]` setters mark dirty.** Twelve hand-written setters assigned their backing field without `this.MarkDirty()`. `PlagueBeastLord.OpenedBy` was an auto-property carrying the attribute with no backing field at all; it is now a generated field with the same name, order and command-property exposure. - **Generated sub-objects are linked to their owner.** Without a `[DirtyTrackingEntity]` member the generator emits setters that mark nothing. Eight entity-owned sub-objects now carry the link and receive the owner through the constructor the generator calls: `BOBFilter` (owner is `IEntity`: a `PlayerMobile` or a `BulkOrderBook`), `BOBLargeSubEntry`, `PuzzleChestSolution` and `PuzzleChestSolutionAndTime`, `TalismanAttribute` (the random factories now take the talisman), `VendorItem`, `PlayerBBMessage`, `RaffleEntry`, `ShardPollOption`. Migration schemas were regenerated with the pinned tool; the only change is the value rule argument becoming `DeserializationRequiresParent`. - **BaseVendor uses the generator; restock amounts are no longer persisted.** The only state it wrote was which buy entries had grown restock amounts, packed by index into the live `SBInfos` tables. Restock is transient now and rebuilds on load; version 1 records are read and discarded through the legacy path. Every vendor subclass now serializes through a generated chain. ## Generator 4.1.0 This PR adopts SerializationGenerator 4.1.0 (modernuo/SerializationGenerator#55): SG3019/SG3020 diagnostics, `[VolatileSerializedState]`, `StopXxx()` timer helpers, and owner-constructor preference for sub-objects (so dictionary values are constructed with their owner; the 4.0.0 relink fallback is gone). `PlayerVendor.v3.json` gains `DeserializationRequiresParent` for its `VendorItem` values so the migration content struct constructs them with their vendor. SG3019 is an error under `TreatWarningsAsErrors` and generator diagnostics ignore pragmas, so the five contexts that live in whole-file player-keyed persistences (`ChampionTitle`, `ChampionTitleContext`, `MurderContext`, `VirtueContext`, `JailRecord`) now carry a `[DirtyTrackingEntity]` link to their `PlayerMobile`, which is where they will live once those blobs move onto the player record. `JailSystem.EmptyRecord` keeps a null player (`[CanBeNull]`). `ChampionTitle` needs both a context and a player constructor because the generator resolves the rule against the containing type but emits the call with the parent field; a comment in the file records this. ## Wire format Unchanged. No version bumps except `BaseVendor` 1 to 2 (which now writes nothing of its own). Schema diffs are rule-argument only (`DeserializationRequiresParent`). ## Testing Server.Tests 848 passed, UOContent.Tests 759 passed. |
||
|
Kamron Batman
|
a2c232f4a8
|
fix: Removes side effects from locked down items, decaying houses, bracelet of binding, and builds (#2608)
## Why Delta world saves (only changed entities re-serialized during the freeze) need two things from content: `Serialize` must be pure, and an unchanged entity must produce identical bytes on every save. An audit of the tree found the places that break this today. This PR fixes them and adds a diagnostic that measures byte stability in a running world over real time. No wire format changes, no version bumps. ## What - **Guild**: `Serialize` ran the daily guildmaster recalculation (dictionary-order tie breaks, random picks, mutates another entity), war expiry, and the alliance leadership check on every save. That work now runs from `GuildMaintenanceTimer` (the former `WarTimer`, now started for both guild systems; the war and alliance checks are no-ops without the new guild system). - **BaseHouse**: the `DecayLevel` getter wrote `LastRefreshed` and advanced the dynamic decay stage on every read, so any undecayable house changed its persisted bytes whenever a sign, gump, or `[Props` looked at it. It is now a pure read; `UpdateDecay()` on the decay tick advances the stage and refreshes a house that leaves the undecayable state. - **Locked-down container contents decay through `DecayScheduler`** instead of a sweep inside `BaseHouse.Serialize`. `Item.CanDecay` now accepts a parent container whose new `Container.ContentsDecay` is true (locked down and not secure) when the content item is not itself locked down or secured; the `IsLockedDown`/`IsSecure` setters re-evaluate a container and its direct contents; `OnDecay` resolves the region from the world location. `BaseBoard`, `Aquarium` and `FishBowl` override `ContentsDecay` to false, matching the exclusions of the old sweep. Items already inside such containers are picked up at load, where every item re-evaluates its registration. The per-save O(all locked-down items) walk is gone; the work is now event driven. - **BraceletOfBinding**: the `Bound` getter nulled its backing field for a deleted target; it now returns null for a missing or deleted target without writing. - **`[SaveStability [delaySeconds=60] [sampleStride=1]`** (Administrator): hashes the serialized bytes of every entity in every registered entity persistence (`Persistence.EntityPersistences`, new), waits the delay in real time, hashes again, and reports per type how many records changed. Both passes are chunked under a 20 ms per-tick budget. Serializing twice inside one tick cannot see drift derived from `Core.Now`, which is frozen within a tick; the real-time wait is the point. On an idle shard the only legitimate churn is NPC movement and regeneration, so a static type with a high changed fraction is a serialization bug. `[SaveStability cancel` stops a run. Engine additions: `Container.ContentsDecay` / `UpdateContentsDecayRegistration()`, the `CanDecay`/`OnDecay` change above, and diagnostic-facing `IGenericEntityPersistence.Name`, `EntityCount`, `EnumerateEntities()`, and `Persistence.EntityPersistences`. ## Behaviour notes - Fealty recalculation now happens on the minute timer for both guild systems instead of at save time. - A house that becomes decayable is refreshed on its next decay tick rather than at the exact moment of transition (at most one minute later). - Contents of locked-down containers now decay on their own schedule (same `LastMoved + DecayTime` rule) instead of at the next world save after becoming due. ## Testing - New tests: `GuildSerializePurityTests`, `HouseDecayPurityTests`, `SaveStabilityTests` (stable entities hash identically after the clock advances; a clock-stamped record is reported by type only after the clock advances; the snapshot covers Items, Mobiles and Guilds). - `LockedDownContainerDecayTests` (Server.Tests): registration on drop, lock/secure/release transitions, excluded containers, nested containers, and decay on schedule through the scheduler. - Full suites: Server.Tests 855 passed, UOContent.Tests 765 passed. |
||
|
Kamron Batman
|
e52d54b7da
|
perf: keep damage entries in an inline intrusive list (#2605)
## Summary `Mobile.DamageEntries` was a `List<DamageEntry>` allocated for every mobile, including the ~99% that never take damage. It is now an inline `ValueLinkList<DamageEntry>` (24 bytes in the `Mobile` object, no separate allocation) ordered least recent → most recent. - `DamageEntry` implements `IValueLinkListNode<DamageEntry>`. - `RegisterDamage` moves the entry to the tail in O(1) instead of `Remove` + `Add` on a list. - Expired entries are always a head prefix, so pruning walks from the head and stops at the first live entry. The `DamageEntries` getter prunes on access. - `DamageEntries` is exposed as `ref readonly`; enumerate with `foreach` or `.ByDescending()`. Mutation goes through `RegisterDamage` / `ClearDamageEntries`. - `BaseCreature.GetLootingRights` and `BaseCreature.ComputeBonusDamage` take `in ValueLinkList<DamageEntry>`; all callers compile unchanged. Files that `foreach` over `DamageEntries` need `using Server.Collections;` for the enumerator extension. - RunUO migration docs (`dev-docs/runuo-migration-docs/09`, `11`) and the `migrate-items-mobiles` skill document the change. Saves one object and 16 bytes per mobile (~8 MB and 500k gen2 objects on a 500k world). Second of three PRs from the lazy per-mobile collections design (first: #2604). Branched from `main`; the two diffs touch disjoint hunks of `Mobile.cs`. ## Breaking change - `Mobile.DamageEntries` is no longer a `List<DamageEntry>`. Indexing, `.Clear()`, `.Add()`, `.Remove()` no longer compile; use `foreach`, `.ByDescending()`, `.Count`, `ClearDamageEntries()`, and `RegisterDamage`. Calling a `ValueLinkList` mutator on the `ref readonly` property compiles but operates on a copy while still unlinking the real nodes; do not. - `BaseCreature.GetLootingRights` and `BaseCreature.ComputeBonusDamage` signatures changed to `(in ValueLinkList<DamageEntry>, …)`. Save format is untouched: damage entries are not serialized. ## Behavior Recency order, `allowSelf`, tie-breaking in `FindMostTotal`/`FindLeastTotal` (most recent wins), `Responsible` accounting, and loot-rights ordering are unchanged and covered by the new `DamageEntryTests` and `LootingRightsTests`. ## Testing - `dotnet build -c Release` clean. - New `DamageEntryTests` and `LootingRightsTests` plus full `Server.Tests` and `UOContent.Tests`. |
||
|
Kamron Batman
|
708a354337
|
perf: stop allocating stat/skill mod lists for every mobile (#2604)
## Summary `_statMods` and `_skillMods` are created lazily by `AddStatMod` / `AddSkillMod` and nulled when they empty, and every reader already null-checks. The eager `new List<T>()` in `DefaultMobileInit` and `Deserialize` therefore allocated two dead 32-byte objects for every mobile. On a ~500k-mobile world that is ~32 MB and 1M gen2 objects that hold nothing. - Removes the four eager allocations. - Removes the `StatMods` accessor (no references). - Documents `SkillMods` as `null` when no mods are active (its one caller in `Skills.cs` already checks). First of three PRs from the lazy per-mobile collections design; `DamageEntries` and `Aggressors`/`Aggressed` follow separately. ## Breaking change - `Mobile.SkillMods` may now be `null` (it was never null after construction before). External callers that enumerate it or read `.Count` must null-check. - `Mobile.StatMods` is removed. Use `GetStatMod(name)` / `AddStatMod` / `RemoveStatMod`. Save format is untouched: neither list is serialized. ## Testing - `dotnet build -c Release` clean. - New `MobileLazyModListTests` plus full `Server.Tests` (840) and `UOContent.Tests` (756). |
||
|
Kamron Batman
|
547c2ea0fa
|
fix: Fixes tick count wrap-around in movement throttle, and eliminates more allocations in NetState (#2603)
## Summary Removes the per-tick allocation in the movement throttle, fixes tick-count wrap-around bugs in the throttle and RTT probe state, and trims per-connection allocations and dead fields in `NetState`. ## Movement throttle - **No more per-tick `List<NetState>` snapshot.** `ProcessAllQueues()` iterates the `HashSet` directly and removes drained or disconnected states in place. `HashSet<T>.Remove` does not invalidate enumerators on .NET Core 3.0+ (verified on 10.0.11); only inserting a *new* member does, and the only `Add` is in the packet handler, which never nests with `Slice()`. The eager `Remove` calls in `RejectAndReset`, `ClearQueue`, and `ProcessMovementQueue` are gone; membership is reconciled once per tick from `_hasQueuedMovements`. - **Debug logging** is now gated solely by the per-connection `NetState.MovementLogging` flag. The global `movementThrottle.debugLogging` setting is removed. - **New settings**: `movementThrottle.maxRttBonus`, `movementThrottle.maxChainGap`, and `movementThrottle.speedHackNotificationCooldown` were fields with no config binding. ## Tick-count wrap-around All comparisons are now in subtraction form and no tick field uses zero as a sentinel: - `now < _nextMovementTime` in the queue drain loop → `now - _nextMovementTime < 0`. - `_lastMovementRecordTime > 0`, `_lastSpeedHackNotification`, `_rttProbeTime > 0`, and `_nextRttProbe == 0` sentinels replaced with `_hasMovementRecord`, `_speedHackNotified`, `_rttProbePending`, and a seeded `_nextRttProbe`. - `_lastQueueDepthCheck` and `_movementWindowStart` are seeded from `Core.TickCount` at construction and on reset instead of zero. User-visible effects of the old code: on hosts with pass-through counters (GCP) movement history never recorded and speed hack detection was silently off; on every host, staff speed hack notifications were suppressed until `Core.TickCount` exceeded the five-minute cooldown. ## NetState - `Instances` returns `HashSet<NetState>` again so engine-internal `foreach` uses the struct enumerator instead of boxing through `IReadOnlySet<T>`. - Removed `_sustainedQueueDepth` (declared and zeroed since #2266, never read), `_lastRtt` (now derived as `LastRtt` from the newest history slot), and `_rttProbeTimestampHiRes` (only fed one debug log line). 20 bytes per connection. - `HuePickers`, `Menus`, and `Trades` are lazily created instead of allocating three lists per connection, including every login-server connection that dies on shard select. `Trades` is released when it empties. All helpers and the `HuePickerResponse` / `MenuResponse` handlers are null-tolerant; the trade cancel loops keep their `i < Count` guards because `SecureTrade.Cancel()` runs virtual item hooks that can re-enter the same list. ## Testing - `dotnet build -c Release` clean. - All MovementThrottle tests pass (27), plus the Trade / Menu / HuePicker / NetState tests (32). |
||
|
Kamron Batman
|
2935eafe24
|
feat: convert all delta-time serialization to anchored time (#2589)
## Summary Phase 3 of the anchored-time work: **every actively-written delta-time value in the engine now stores an anchored timestamp** — absolute on the wire, shifted forward by the downtime at load. Remaining time survives restarts (as delta did), and unlike delta, the bytes do not change on every save, so an idle world serializes identically save after save. The answer to "is it possible everywhere": **yes** — including the one case that looked impossible. ## The GenericPersistence problem, solved `GenericPersistence` bins (`Virtues.bin`, `StealableArtifacts.bin`, …) are raw payloads with no idx header, so they have no anchor of their own — anchored reads there would silently apply zero shift. But the anchor is a property of the **save**, not the file: every file in one save shares one `World.SaveStartTime`, and `Persistence.Load` reads **all** entity indexes (phase 1) before **any** persistence payload (phase 2). So the idx v5 header stamps a save-wide `World.LoadTimeShift`, and generic persistence readers inherit it. No file-format change, no per-bin header, old bins unaffected. ## Converted - **Item v10 → v11**: `LastMoved` — previously whole-minute delta, rewritten every save for every item, the single largest source of idle-save churn — and `DecayResetTime` (retiring the TODO from #2583). **Mobile v37 → v38**: the three stat-gain stamps. **BaseCreature v20 → v21**: `SummonEnd`. - **17 code-generated classes** (`[DeltaDateTime]` → `[AnchoredDateTime]`, version bump + `MigrateFrom` each): the five field spells, TransientItem, VirtueContext (×7 fields), PuzzleChestSolutionAndTime, BaseCamp, BaseBoat, RentedVendor, PlayerVendor, Ethics Player, Sheep, StarRoomGate, ChampionSpawn (×3), Corpse (`TimeOfDeath`, v19). The `MigrateFrom` bodies were generated from each class's current migration schema and are compiler-verified; VirtueContext's save-flagged nullables fall back to the same defaults the old deserialize left in place. Corpse's six migrations moved to a new `Corpse.Migrations.cs`. - **Hand-written sites**: StealableArtifacts (v2), VendorInventory (v1), ML quest objectives (persistence v3) — each gated on its own version. **Not converted, deliberately**: the ~25 read-only `ReadDeltaTime` sites in legacy version fallbacks and migration replays — they decode existing old bytes and must never change. `[DeltaDateTime]`/`WriteDeltaTime` remain available for them. ## Verification - Build 0 errors / 0 warnings; **837 + 708 tests green**. - Schema regeneration produced exactly the 17 expected new `vN.json` files (all `AnchoredTime` rule args), nothing else touched. - **New acceptance tests** pin the point of the whole effort: serializing the same item at two save times **5 hours apart produces byte-identical output**, and `LastMoved`/`DecayResetTime` round-trip **exactly** at sub-minute precision (the old minutes encoding destroyed both properties). ## Notes for review - `LastMoved` grows from a 1–3 byte encoded minutes value to 8-byte ticks per item — the price of byte-stability; it repays itself in incremental-save behavior since unchanged items now produce unchanged bytes. - BaseEscortable-style semantics are unchanged: anchored shift preserves *remaining* time exactly, the same contract delta provided, so no gameplay-visible behavior changes — deadlines simply stop being consumed by downtime that delta already protected against, now with stable bytes. ## Enforcement `WriteDeltaTime` is now `[Obsolete]` (interface + implementation). With the repo's warnings-as-errors, any new delta-time write — hand-written or emitted by a still-unconverted `[DeltaDateTime]` field — fails the build, with the migration instructions in the message. That the full solution still builds with **zero warnings** is itself the proof no active delta writer survived the conversion. `ReadDeltaTime` deliberately stays un-attributed: its remaining callers decode existing old bytes and are correct forever; its XML docs now state the legacy-decode-only contract. |
||
|
Kamron Batman
|
73f9688083
|
feat: adopt serialization generator v4 (field-side linkage, anchored timers) (#2586)
## Summary Adopts ModernUO.Serialization 4.0.0 across the engine. Three commits, reviewable independently: 1. **Package + tool bump to 4.0.0** (`Server.csproj`, `UOContent.csproj`, `dotnet-tools.json`). 2. **Timers → `[DeserializeTimer]`** — the 8 drifting timers (BaseLight, TreasureMapChest, MarkContainer, FillableContainer, DeathRobe, DecayedCorpse, Corpse, BaseEscortable) now store their next tick as **anchored time**: server downtime no longer consumes the remaining delay, and idle-world saves are byte-stable. This changes their wire format, so each class bumps its serialization version with a `MigrateFrom` that replays the old delta-time read through the migration schema (the new `vN.json` files carry `@AnchoredTimer`; the old ones keep `@TimerDrift`, which the generator reads forever). The 2 wall-clock timers (Aquarium, FountainOfLife) keep their exact format via `wallClock: true` — no bump. Restart methods drop their `TimeSpan.MinValue` sentinel checks: v4 invokes them **only when a timer was actually running at save**. 3. **Linkage → field-side declarations** — 175 conversions across 25 files: `[SerializableFieldSaveFlag(order)]`/`[SerializableFieldDefault(order)]` become `[SaveFlag(nameof(...), nameof(...))]` on the field, and `[SerializableFieldChanged(order)]` becomes the `fieldChanged:` argument of `[SerializableField]`. **Wire-neutral: zero migration schemas changed.** ## Verification - Solution builds with **0 errors, 0 warnings**; all three 4.0.0 packages verified indexed on nuget.org (no local feed needed). - **835 + 708 tests green.** - Generated output inspected: old-version content structs replay `ReadDeltaTime` (e.g. `V3Content.DecayTimerNext = reader.ReadDeltaTime()`), current versions write/read anchored time with the gated restart, and the wall-clock classes emit byte-identical `Write`/`ReadDateTime` framing. - Schema tool run is committed (CI's `git diff --exit-code` schema check passes): exactly the 8 expected new `vN.json` files, nothing else touched. - The conversion was scripted with a class-scoped resolver (order → same-class `[SerializableField(order)]`/`[SerializableProperty(order)]`); it planned 175/175 with zero ambiguities before applying. ## Notes - New `MigrateFrom`s use the content structs' provided `XxxDelay` property, matching the pre-existing idiom in Corpse's and TreasureMapChest's older migrations. - Follow-up candidate (separate PR, wire-neutral, any time): fold the ~150 eligible hand-written `[SerializableProperty]` setters (clamps, post-change side effects) down to `[SerializableField]` with `allowFieldChange`/`fieldChanged` hooks. |
||
|
Kamron Batman
|
126a10ce53
|
feat: anchored-time infrastructure with a save-start anchor in idx v5 (#2585)
## Summary The save-stability infrastructure consumed by generator v3's `[AnchoredDateTime]`: anchored timestamps are written as **absolute values** and re-based once at load by the elapsed time since the save started — so downtime doesn't age them, and an unchanged entity serializes to identical bytes (the prerequisite for replacing delta-time encodings, which rewrite every entity on every save). ## Design - **`WriteAnchoredTime` / `ReadAnchoredTime`** on `IGenericWriter`/`IGenericReader`. The read side applies the reader's `AnchoredTimeShift`; `Min/MaxValue` sentinels pass through unshifted, and shifts saturate instead of overflowing. - **`World.SaveStartTime`** is stamped the moment the world freezes for a snapshot — one anchor for the entire save, no per-persistence skew. - **idx v5**: the anchor ticks sit in the header right after the version. The anchor travels with the file it re-anchors, so a single idx+bin pair restored from a backup is self-describing, and anchor presence is guaranteed by the same version gate as the record format — there is no separate anchor file to lose. - **The shift rides the reader instance** (`BufferReader`, `UnmanagedDataReader`, `BinaryFileReader` delegating), not a static — parallel per-persistence loads and ad-hoc restores each see their own file's anchor. idx v4 and older read with a zero shift. ## Scope Behavior-neutral: nothing serializes anchored values yet (`Item.DecayResetTime` and the `[DeltaDateTime]` field migrations come separately, with their own version bumps). Saves written from this branch are idx v5; loading v4/v3 saves is unchanged and remains pinned by the existing hand-written-header tests. ## Testing - Unit round-trips: exact with zero shift, shifted read, sentinel passthrough, saturation, Local→UTC normalization. - End-to-end through the real worker/segment-log pipeline: an anchored timestamp re-bases across a simulated two-hour downtime via the idx v5 header. - Full suites green: Server.Tests 835/835, UOContent.Tests 708/708 (including the existing v4/v3 idx loading tests). |
||
|
Kamron Batman
|
541dbc5ac5
|
feat: Bumps dependencies. Introduces Serialization Generator v3 (#2584)
### Summary * Upgrades Serialization Generator to v3. This contains numerous bug fixes and a significant performance improvement. * Bumps other dependencies. |
||
|
Kamron Batman
|
971d7b6a77
|
fix: stop items from insta-decaying when decay eligibility is restored without a move (#2583)
## Summary A GM flipping `Movable` back on for a long-frozen item made it vanish within one scheduler tick. The setter registered the item with a deadline computed from its stale `LastMoved`, so `ProcessActiveQueue` deleted it almost immediately. The pre-#2311 save-time sweep had the same semantics, just hidden behind the save cadence. The same failure existed for `Visible` and `Spawner` transitions. `LastMoved` is deliberately left meaning actual movement — it feeds vendor inventory expiry and house moving-crate checks — so the fix does not rewrite it for state changes. ## Changes - **`DecayResetTime`** (CompactInfo-backed): the decay countdown runs from the later of `LastMoved` and this stamp. `RestartDecay()` stamps it only when the item can decay and the stamp extends the current deadline, so hot paths with a fresh `LastMoved` allocate nothing. - **`Movable`/`Visible`/`Spawner` setters** call `RestartDecay()` instead of registering a stale deadline. - **Region-refusal retry** in `DecayScheduler` uses `RestartDecay()` instead of rewriting `LastMoved`. - **Persistence**: the stamp survives save/load as a `WriteDeltaTime` delta under `SaveFlag.DecayReset` (to become `WriteAnchoredTime` once the save-time anchor is ported) (Item serialization v10), so a restart mid-window no longer deletes the item. - **`LastMoved` setter** drops a superseded stamp so the `CompactInfo` can collapse instead of being held (~40 bytes) forever. - **Raw `Map` setter** now counts as a move for parentless items: it stamps `LastMoved` and updates decay registration, closing the gap where an item moved out of `Map.Internal` via the setter never decayed. - **`LiftItemDupe`**: the remainder of a partially lifted *ground* stack was placed via raw `Location`/`Map` assignments and never enrolled for decay (lingering-trash leak since #2311) — now enrolled via the Map setter. Parented remainders get their map from `AddItem` (parent first, then map), so container splits never transit the scheduler. |
||
|
Kamron Batman
|
be3a08513f
|
fix: Fixes PlayerConstructed stacking/BODs (#2579)
### Summary * Removes player constructed as a requirement for BODs. * When two items stack and they don't match player constructed flags, the resulting stack loses the flag. |
||
|
Kamron Batman
|
240118340e
|
fix: stop the idle-sleep backoff tripping on healthy hosts (#2572)
## Problem The late-wake detector added in #2559 suspends idle sleeping on perfectly healthy hosts. The visible symptom is this Warning firing periodically on stable machines: > This host returned a 2ms idle wait at least 8ms late 2 time(s) in the last second; idle sleeping suspended for 5000ms Demoting it to Debug would hide the symptom but not the cost: every one of those lines means the shard dropped idle sleeping for 5s and burned a full core for no reason. The detector is what was mis-tuned. ## Cause 1 — lateness was a count, not a rate An idle loop performs **~400–500 sleeps per second** (2ms each, bounded by the 8ms wheel tick). The trip condition was `late > 1` across two consecutive one-second samples — a **0.4% tail-outlier rate**. A co-tenant burst, a page fault, or another process changing the system timer resolution clears that bar on a healthy host. A host that genuinely cannot schedule the process — throttled burstable vCPU — returns *most* of its waits late. Signal and noise were two orders of magnitude apart, and the check sat in the noise. Now gated on the proportion, with the absolute count kept as a floor: ```csharp if (late <= _lateWakeThreshold) { _consecutiveBadSamples = 0; return; } // floor if (late * 100 < sleeps * _lateWakePercent) { _consecutiveBadSamples = 0; return; } // rate ``` New `server.lateWakePercent` (default `10`). The floor is what keeps a window with only a handful of sleeps from tripping on a meaningless percentage; `server.lateWakeThreshold` keeps its existing meaning. ## Cause 2 — GC pauses were charged to the host `dev-docs/debugging-event-loop.md` already documents that the GC collects preferentially **during idle sleeps** — that is the natural pause point it looks for. So the detector was systematically measuring the GC's chosen pause point and billing it to the host's scheduler. Not an occasional coincidence; a designed-in one. ```csharp var collections = GC.CollectionCount(1); NetState.WaitForCompletion(requested); ... if (elapsed - requested >= Timer.TickRate && GC.CollectionCount(1) == collections) ``` Gen1 (which counts gen2 with it) rather than gen0 — gen0 pauses don't approach the 8ms `TickRate` bar anyway, and gating on them would discard useful samples. The second read short-circuits behind the overshoot test, so the common path costs **one** `GC.CollectionCount` per sleep: an internal counter read, single-digit nanoseconds, ~500/sec. ## Cause 3 — every backoff logged at Warning Tiered to the escalation that already existed, since a single suspension is recoverable and not something an operator can act on: | Backoff | Level | |---|---| | 1–2 | `Debug` | | 3–5 | `Warning` (now includes the sleep count and "for the Nth time running") | | ceiling | `Error`, unchanged | | recovery | `Information` (new) | Each backoff doubles the suspension, so every line is already a distinct escalation step — no further rate limiting needed. ## Drive-by The `BackoffResetAfterCleanMs` reset only ran on the path to a *new* backoff, making it unreachable for a host that recovered for good — such a host never cleared its escalation or re-armed `_loggedBackoffCeiling`. It now runs on every health sample, which is also what makes the new recovery line reachable. ## Testing Full solution builds clean, 0 warnings. No tests added: the state is private static in `Core` coupled to `_tickCount` with no injection point, and nothing covered it before — adding a seam purely to test it seemed worse than the gap. Happy to add one if reviewers disagree. |
||
|
Kamron Batman
|
9b35b39d0d
|
fix: stop stack merges and splits from laundering PlayerConstructed (#2576)
## Why `PlayerConstructed` is per-instance provenance, and #2574 put it on every crafted item — including potions, arrows and other stackables. Stack operations were written when no item carried provenance of any kind, so they treated two piles of the same graphic as interchangeable. **Merging** keeps the receiving stack's value. Dropping bought potions onto a crafted stack made the whole pile count as crafted; the reverse order erased it. Which one happened was decided by drag direction alone. **Splitting** rebuilds one half in `Mobile.LiftItemDupe`, which copies a fixed list of fields rather than going through `Dupe`/`CopyProperties`. `PlayerConstructed` was not on that list, so dragging part of a pile off stripped the new half. Worth calling out: `[IgnoreDupe]` does **not** govern this path — it only applies to `Dupe()`. Reasoning "the field isn't `[IgnoreDupe]`, so it copies" is wrong here. ## Changes - `Item.CanStackWith` compares `PlayerConstructed`, so crafted and non-crafted never merge into one indistinguishable pile. - `Mobile.LiftItemDupe` copies `PlayerConstructed` onto the remainder, so a split cannot produce halves that disagree about what they are. Refusing to merge is the whole fix. A stack has nowhere to record provenance, so the only coherent behaviour is to keep the two piles apart rather than pick a winner. ## What this deliberately does not do Paths that genuinely **virtualize** an item — pouring from a `PotionKeg`, for one — rebuild it without the flag, and the result is simply treated as not crafted. That is accepted rather than worked around; the alternative is threading provenance through every count-based container, which buys little. The keg stores a `Held` int rather than a stack, so nothing there depends on merging and nothing breaks. `CommodityDeed` is unaffected — it holds the real `Commodity` item rather than a count, so the flag rides along. ## Player-visible effect Crafted potions and arrows will no longer stack with bought or looted ones. That is the intended invariant, and it is the reason the flag can be trusted at all. ## Tests 7 new tests in `Server.Tests`: both merge directions, the matching-provenance case, split copying, and the split/re-merge round trip. `Server.Tests` **822 passing**, `UOContent.Tests` **701 passing**, build clean with 0 warnings. |
||
|
Kamron Batman
|
bd79cb7759
|
fix: Consolidate PlayerConstructed onto Item, stamped by the craft system (#2574)
Follow-up to #2573. That change made `SmallBOD.EndCombine` require a player-crafted item, but it could only read provenance off `BaseArmor`, `BaseWeapon` and `BaseClothing`, because those are the only three classes that track it — hence the hand-enumerated `armor?.PlayerConstructed ?? clothing?.PlayerConstructed ?? weapon?.PlayerConstructed ?? false`. The gap is structural rather than cosmetic. `PlayerConstructed` is set inside each base's `OnCraft`, so it can only ever reach types implementing `ICraftable`. Most craftables do not — the tinkering catalogue alone is largely plain `Item` subclasses — so any rule keyed on "was this actually crafted" has nothing to key on for those types. ## What changed Provenance moves to `Item` and is stamped centrally in `CraftItem`, immediately after the item is constructed and before the `ICraftable` dispatch, covering both the AOS and T2A craft paths. The three `OnCraft` overrides drop their now-redundant assignment and inherit `Item`'s property, so no call site outside them changes — `Resmelt` and `SalvageBag` still read `armor.PlayerConstructed` and still compile unchanged. `SmallBOD`'s three-way null-coalescing chain collapses to `item.PlayerConstructed`. `OnCraft` is only ever invoked from `CraftItem` (the other three call sites are `base.OnCraft` chaining), so removing those assignments has no other reachable effect. ## Storage cost: none `Item`'s `SaveFlag` word is written as a fixed-width `int`, not an encoded one, so occupying bit `0x08000000` changes no record lengths. Items that are not player-constructed serialize byte for byte as before, and crafted ones differ by a single bit in a field already being written. `Item` itself needs no version bump: a bare `SaveFlag` bit is self-describing, so records written before it existed lack it and read `false`. ## Version bumps The three content classes do need one, since removing a serialized field changes their layout: | Class | Version | Field removed | |---|---|---| | `BaseArmor` | 9 → 10 | 24 (was last, nothing renumbered) | | `BaseClothing` | 7 → 8 | 7 (fields 8–10 shift down) | | `BaseWeapon` | 10 → 11 | 26 (fields 27–30 shift down) | Each gets a `MigrateFrom` for its previous version that assigns the old bool to the inherited property, so existing crafted armour, weapons and clothing keep their provenance across the upgrade. `Item.Deserialize` runs first and reads the absent bit as `false`, then the migration overwrites it — the generated `Deserialize` calls `base.Deserialize` before dispatching, so the ordering holds. `BaseWeapon` had no migrations file and gains one. The renumbering is not stylistic: the generator requires contiguous field ordering and rejects a hole with `SG3005: Expected field 'Crafter' with order 7 but found 8`. New schema JSONs (`BaseArmor.v10`, `BaseClothing.v8`, `BaseWeapon.v11`) are generated by `ModernUOSchemaGenerator` and committed alongside. ## One thing worth a second opinion The new property is a plain auto-property on `Item`, so it does not call `this.MarkDirty()` the way the codegen setters it replaces did. `MarkDirty` is currently a no-op (`// TODO: Add dirty tracking back`) and no property in `Item.cs` calls it, so this matches the file as it stands — but it is worth noting if dirty tracking comes back. ## Verification Full solution builds in Release with 0 errors and 0 warnings; 1516 tests pass (815 `Server.Tests`, 701 `UOContent.Tests`). |
||
|
Kamron Batman
|
0628902644
|
fix: harden idle-sleep scheduling against bad config and misattributed saves (#2567)
Follow-ups to #2559, from a review of the ported idle-sleep/scheduler-health changes. ### Fixes - **`NetState.IsIdle` omitted `_pendingDisconnects`** — `Slice()` drains five queues; the property checked four. The other deferred work (`_connectingQueue`, alive checks, movement throttle) is time-gated and correctly excluded; the disconnect queue was the only ready-work omission. Impact was bounded (≤ one idle wait of delay), but the property's contract is "sleeping cannot strand pending work". - **Neither new setting was clamped** (`Main.cs`): - `server.lateWakeThreshold: -1` made `late <= threshold` false for every sample even at zero late wakes, so from the second sample on, sleeping was re-suspended every second, forever — a permanent full-core spin whose only trace was a nonsense warning ("… at least 8ms late 0 time(s)"). - `server.eventLoopIdleWaitMs: -1` disabled sleeping while the admin gump reported **Healthy** (it tested `== 0`). - Both now clamp to `>= 0` and log a warning naming the configured value. `-1` is a natural thing to reach for given the sibling key's doc says "set very high to disable". - **World snapshots were misattributed to `StolenMs`** — `World.Snapshot` ran outside all five profiler phases, so a 3-second save inside a sample read as ~75% stolen, and `debugging-event-loop.md` teaches stolen = "the host ran something else". The diagnostic pointed operators at buying dedicated CPU for their own largest loop-thread stall. Saves now land in a new `WorldSnapshot` phase; `[LoopStats` iterates `PhaseCount` generically, so the report and CSV pick it up with no changes. - **Admin gump conflated host-forced spin with configured spin** — when the startup probe finds no high-resolution wait support it zeroes the idle wait, after which the gump said "Spinning (configured)" and the operator's config said 2. New `Core.IdleSleepUnsupported` property; the gump now shows "Spinning - host cannot honor short waits" as a distinct fourth verdict. A genuinely configured 0 still reads "configured" (the probe only runs when the configured value was > 0). - **The backoff-ceiling `Error` logged once per process lifetime** — `_loggedBackoffCeiling` never reset, and at the ceiling the method returns before the `Warning`, so a host that recovered (>60s clean streak) and later degraded back to the ceiling never re-logged the one operator-actionable message. The flag now resets with the clean-streak escalation reset. - **Removed the unreachable "already suspended, extend" branch** — no sleeps occur while suspended, so `_lateWakes` stays 0 and every suspended sample early-returns before reaching it; with the threshold clamped it can never fire. If sleep gating ever changes, the normal path handles the case by counting a fresh episode. `dev-docs/debugging-event-loop.md` updated to match (phase list + gump verdict table). ### Verification - `dotnet build` clean (0 warnings) both normally and with `-p:EventLoopProfiling=true` (the snapshot phase only becomes live IL under the profiling flag). |
||
|
Kamron Batman
|
6d846b11e5
|
perf: Sleep the event loop when idle. Fixes networking micro-stalls. Adds event loop instrumentation. (#2559)
## Problem
`RunEventLoop` span through its body regardless of whether there was anything to do — ~10% of a desktop core for an empty shard, and ~70% of a core on a 3 vCPU VPS. A process that never idles is exactly what burstable vCPU plans throttle, which is how this surfaced: lag spikes that went away when the operator bought more cores. The spin also denied the GC its natural pause points, so memory climbed until a world save forced a collection — alarming in task manager, harmless in practice, and a recurring source of "is my server leaking?" reports.
## Result
Windows desktop, real world of **190,728 items / 33,158 mobiles**, no players, saves and prebake off, three consecutive runs:
| | Legacy spin | Idle sleeping |
|---|---|---|
| **CPU** | 10.42 – 10.50% of one core | **0.78 – 1.00%** |
| **Tick lag** (peak/15s) | 4–10 ms | 5–11 ms |
**~10× less CPU with tick lag unchanged** — the CPU came free rather than being traded for latency. Slower hosts gain proportionally more. Spin mode (`server.eventLoopIdleWaitMs=0`) independently gained **7× the iterations per core** (1.19M → 8.3M cycles/sec) from the ring's AcceptEx rework.
## How
The loop blocks in `NetState.WaitForCompletion` whenever every queue it drains is empty (all the drains are bounded, so leftovers keep it awake). Receive completions, new connections, and cross-thread `LoopContext.Post` (via the ring's sticky `Wake()`) are all in the wait set, so sleeping adds no latency to any of them. Only timer-driven logic sees wheel lag, bounded by the idle wait.
**Health is measured at the only place sleeping can cause harm.** A sleep is bounded by the time to the next wheel turn, so a correctly honoured sleep can never miss a deadline — the only failure mode is the host returning the wait late. That overshoot is measured on every sleep (one extra timestamp read; production's entire accounting cost), and an escalating backoff suspends sleeping when it persists. By construction, server work — saves, heavy staff commands, deep timer callbacks — cannot trip it, so the warning means exactly one thing: *the host is not scheduling the process promptly*, with two known remedies (dedicated CPU, or `=0`). Hosts with no high-resolution wait mechanism at all are detected once at startup and spin instead.
**CPS is removed.** `Core.CyclesPerSecond`/`AverageCPS` measured nothing actionable before and became actively misleading once the loop sleeps (the rate is set by the sleep, not by shard health). The admin gump's Performance page now shows the verdict instead: `Healthy` / `Sleep suspended (host)` / `Spinning (configured)`.
## Configuration
| Setting | Default | Meaning |
|---|---|---|
| `server.eventLoopIdleWaitMs` | `2` | Longest idle block. Measured across 1/2/4/8 ms, 2 is where the trade stops being free. `0` = never sleep: ~98% of a core, zero scheduling overhead — for large shards on dedicated CPU. |
| `server.lateWakeThreshold` | `1` | Idle waits the host may return a full tick late, per second, before sleeping backs off. Raise for jittery hosts; very high disables the backoff. |
## Diagnostics (compiled out by default)
`dotnet build -p:EventLoopProfiling=true` compiles in `EventLoopProfiler` — every hook is `[Conditional("EVENT_LOOP_PROFILING")]`, so normal builds contain zero profiling IL. The profiling build decomposes each second of wall time into **work (per loop phase) / sleep / GC pause / stolen residual**, keeps ~15 minutes of history in a ring buffer, and the `[LoopStats` command prints the last minute and dumps the full history to CSV. `dev-docs/debugging-event-loop.md` is the diagnosis guide (for humans and AI): what production already tells you, when to flip the profiling build, the signature table for host-steal vs deep-processing vs GC vs wake bugs, why dotnet-trace comes last, and the GC/RAM "leak" misconception.
## Verification
- 815 Server.Tests green; both build configurations compile.
- Docker echo harness green on epoll and io_uring (ping-pong mode); kqueue verified manually on an M1 Max.
- A/B measurements and per-change numbers: `measure/event-loop` branch.
## Notes
The full measurement harness and vendored ring sources used to develop this live on the [`measure/event-loop`](https://github.com/modernuo/ModernUO/tree/measure/event-loop) branch, kept for future loop work.
|
||
|
Kamron Batman
|
a7e65aab01
|
perf(login): run password hashing on a parked worker thread (#2566)
## Why An Argon2 verify is **~8.9 ms of frozen world per login attempt** — more than half a 16 ms frame. Failed attempts cost exactly the same as successful ones, by design, so a credential-stuffing flood is a full-cost stall per packet without needing valid credentials. `SetPassword` derives a hash too, so `[password`, the admin gump and account creation each pay the same. ## What the measurement says Off-loading does not delete the cost, it relocates it. Three things stay on the loop: | Component | Measured | |---|---:| | Inline verify (today) | **8.92 ms** | | Dispatch to the worker | 210 ns | | Drain the continuation off `LoopContext` | 13 ns | | Loop's own work slowed by shared-L3 eviction | **0.05 – 5.44 ms** | Net gain **3.5 – 8.9 ms** of on-loop time per login. Harness in `ModernUO-Benchmarks` (`Benchmarks/Argon2OffLoop/`): it models the loop as a dependent-load pointer chase swept across working-set sizes, which is an upper bound on cache-latency sensitivity, and copies `EventLoopContext` so the hand-off cost is the real one. Two results shaped the design: - **The contention tax peaks in the middle of the working-set range**, not at the top — 5.44 ms at 8 MiB (a quarter of this chip's L3), but 0.76 ms at 30 MiB and 0.10 ms at 256 KiB. A tiny hot set has nothing in L3 to lose; a huge one is already DRAM-bound. - **Per-login tax falls as concurrency rises** (5.44 → 2.56 → 1.60 ms at 1/2/4 hashers) while *total* loop damage rises. Contention is shared, not additive, so a login rush is not the disaster case — a single login is. ## Why exactly one worker It is load-bearing three times over, which is also why it must not quietly become a pool: - **Cost bound.** Off-loop loses to inline only if a hash steals ~82% of the loop's throughput. One hasher contending for one core leaves the loop ~50%. **A single background hasher cannot cost the loop more than the inline verify under any scheduling regime**, which is what lets the measurement hold on hardware we cannot inspect — AMD, VPS, oversubscribed VM. Four hashers drop the loop to ~20% and break it. - **Memory.** Exactly one hashing arena is live at a time whatever the login volume. - **Ordering.** Writes apply in dispatch order *only* because a single thread drains FIFO. A second worker would need ordering reintroduced; `WritesApplyInDispatchOrder` fails if that happens. Throughput is ~110 verifies/sec. Only loop time matters, not login latency, so head-of-line blocking during a rush costs nothing. ## Making every protection safe off-thread The worker was initially Argon2-only. That was the right call for the wrong reason — it was blamed on Argon2's salt RNG, which is a stateless syscall wrapper and was never a problem. The real blockers were elsewhere, and both are fixed at the source: | Protection | Was | Now | |---|---|---| | MD5/SHA1/SHA2 | shared `HashAlgorithm.ComputeHash`, which carries the running digest across `HashCore`/`HashFinal` through process-wide singletons | static `HashData` into a `stackalloc` span — no state, no allocation, identical bytes | | PBKDF2 | `Utility.RandomMinMax` → shared `System.Random`, thread-unsafe *and* game state | `RandomNumberGenerator.GetInt32`, matching the salt beside it | | Argon2 | already safe (`Verify` is static + stackalloc) | unchanged, singleton reused | Literal digests are pinned in a test **before** the change and still pass after it. These are compared as strings against every account database, so any casing or encoding drift would lock out every SHA and MD5 account at once. With all three safe, the worker no longer knows which algorithm it runs and the dispatch conditions collapse to "is off-loop available". ## Correctness - **Phrase derivation** moves to `AccountSecurity.DerivePhrase`, so verification (stored algorithm's rule) and rehash (target algorithm's rule) cannot disagree. Deriving with the wrong one is the shape of the lockout fixed in #2562. - **Liveness** is checked at dequeue *and* at apply — a connection can drop while queued or while the result sits in the loop queue. A job with no connection attached, such as an admin password change, runs regardless. - **Queue overflow rejects** a login rather than verifying inline; steering work back onto the loop is what a flood wants. A password change instead falls back to hashing inline, because unlike a login it must not be dropped. - **Shutdown and crash** both just stop the thread, and pending jobs are dropped. No save is initiated once shutdown begins — saving is the operator's choice up front, via the admin gump's save/no-save variants, and `WaitForWriteCompletion` honours one already in flight — so a write applied during teardown would reach no disk. The crash path needs its own subscription because `HandleClosed` skips `InvokeShutdown` when crashed. ## Bounding `MaxPending` is 4096 — a backstop, not a flood defense. `SentFirstPacket` holds a connection to one pending verify and the engine caps connections at 4096, so the queue is already bounded by construction and this can only trip if that invariant breaks. A cap low enough to blunt an attack would reject real players first; during a mass reconnect they *are* the queue. Flood defense belongs at the connection layer. The real DoS improvement is elsewhere: today every attempt stalls the world, and after this a flood occupies one core while the loop keeps ticking. ## Gate Release builds on 4+ cores. Below that there is no spare core to move work to, so off-loading buys nothing by construction; `DEBUG` is excluded because dev boxes and test shards have few logins. Both modes call the same code — the gate only chooses where it runs. ## Engine change One property, `AccountLoginEventArgs.Deferred`, so a subscriber can say "no verdict yet". `EventSink.AccountLogin` is `Action<...>` with no continuation, and the packet handler replies in the same call. Approved separately since it touches `Projects/Server/`. ## Docs `dev-docs/threading-model.md` and the threading skill gain a vetted-workers section. The forbidden-patterns table bans `new Thread`, `ConcurrentQueue<T>`, `Interlocked` and `volatile` in `UOContent`, and its exceptions covered only `Projects/Server/` — the existing Advanced Search fan-out already sat outside it. The new section leads with proving the need (measure on-loop time, not wall-clock; gate on core count; record the measurement), keeps game logic on the loop via chunking, and documents the hand-off protocol in both directions. ## Testing 698 UOContent tests, 810 Server tests, Release build clean. Covered: verify and rehash outcomes, phrase rules for SHA1/SHA2 vs Argon2, stored-format stability for MD5/SHA1/SHA2, jobs with no connection attached, and dispatch ordering through the real queue. The liveness and ordering guards are mutation-verified. |
||
|
Kamron Batman
|
cce035f1c3
|
fix: Removes unnecessary dictionary removal guards (#2565)
## What `Dictionary<K,V>.Remove` and `HashSet<T>.Remove` do not bump the collection's version, so removing an entry during a `foreach` does not invalidate the enumerator. A number of loops were still paying for a `PooledRefQueue`/`PooledRefList` to collect keys and drain them in a second pass. This drops those guards. ## Why it's safe Verified against .NET 10.0.10 rather than taken on trust, since the documented guarantee covers only `Dictionary<TKey,TValue>.Remove` while several of these call sites are `HashSet<T>` or enumerate `.Keys`/`.Values`: | Case | Result | |---|---| | `Dictionary` foreach + `Remove` | safe, all entries visited | | `Dictionary.Keys` / `.Values` foreach + `Remove` | safe, all entries visited | | `HashSet` foreach + `Remove` | safe, all entries visited | | `Dictionary` foreach + `Remove` **then `Add`** | throws `InvalidOperationException` | Reflection on `_version` confirms the mechanism: neither `Dictionary.Remove` nor `HashSet.Remove` touches it. Because `Remove` never bumps the version, the `Keys` and `Values` enumerators are just as safe as the dictionary's own, even though only `Dictionary.Remove` documents the behaviour. No entries were skipped in any case. The `HashSet` half is confirmed by [stephentoub on dotnet/dotnet-api-docs#8177](https://github.com/dotnet/dotnet-api-docs/issues/8177#issuecomment-1167251052): *"Both HashSet and Dictionary have been improved to support removal during enumeration. The docs may just benefit from updating."* The gap is in the documentation, not the runtime. `Remove` followed by `Add` in the same enumeration still throws. That is the line this PR does not cross. ## Guards removed `VisibilityList`, `ChampionTitleSystem`, `Channel`, `BombingRun`, `Ruleset`, `PuzzleChest`, `RaceChangeGump`, `StepCache`, `PlayerMurderSystem`, `VirtueSystem`, `ProjectedItem`, `StaminaSystem`, `AIGroupMovement`, `PromotedGuard`, `AutoDenylist`, `LoginAllowlist`, `AntiMacroSystem`, `DetectHidden`. Both collection kinds are covered: `Dictionary` (including loops over `.Keys` and `.Values`) and `HashSet` (`ProjectedItem._active`, `PlayerMurderSystem._contextTerms`, `StaminaSystem._resetHash`). In `StaminaSystem.ResetTimer` the `Count == queue.Count → Clear()` branch goes away with the queue — it only existed to avoid paying for N individual removes. Where the collection supports it, `Contains` + `Remove` and `TryGetValue` + `Remove` also collapse into a single lookup (`if (list.Remove(x))`, `if (m_Pending.Remove(ns, out var state))`). `Utility.Tidy<K,V>` keeps its two branches: when `K` is serializable the value is not inspected, otherwise the value is. Only the serializable side may be cast, so `Dictionary<Mobile, int>` and `Dictionary<Mobile, string>` stay valid. ## Deliberately unchanged **`BaseCreature.LoyaltyTimer.OnTick`** keeps its deferred-delete queue. Removing from `World.Mobiles` while enumerating it is safe, but `Mobile.Delete()` is not a `Remove` — it runs `OnDelete`/`OnAfterDelete`, the `OnParentDeleted` cascade over the creature's pack, `DropHolding()`, and region and guild callbacks. Anything in that surface that constructs a `Mobile` is an `Add` into the dictionary being enumerated, which does invalidate it. `BaseHire.PayTimer.OnTick` has the same shape and is likewise untouched. **Spatial-query buffers** — `GuardedRegion.CallGuards`, `Thunderstorm`, `Exorcism`, `LeverPuzzleController`, `BaseCreature.TeleportPets` — are a different hazard. They buffer the result of a range query because the drain moves or harms mobiles, which mutates sectors mid-enumeration. **Re-entrant drains.** The `_users` sets in `Firebomb` and the explosion, conflagration and confusion-blast potions look like this pattern but are not: the loop collects, `Clear()`s, and only then runs `Target.Cancel` on each, which can re-enter. `AnimalTrainer` enumerates `pm.Stabled` and drains through `RemoveStabled`, which nulls the `Stabled` field once it empties — safe for an in-flight enumerator, which holds the set reference rather than the field, but subtle enough not to be worth inlining on a cold path. ## Verification `dotnet build` clean with 0 warnings; 810 Server and 684 UOContent tests pass. |
||
|
Kamron Batman
|
23dc6649a0
|
fix: Require only runtime packages on Linux, and check ICU and tzdata the way the runtime does (#2561)
## Why ModernUO mandated `-dev` packages on production servers for exactly one reason: `DllImport` never asks for a versioned SONAME, so `libdeflate.so.0` and `libargon2.so.1` sitting in `/usr/lib` went unfound, and the `-dev` package's unversioned symlink was the only thing making resolution work. The `-dev` packages ship no library of their own — operators were installing headers and a static lib on machines that compile nothing. Fixed in the binding packages (modernuo/LibDeflate.Bindings#4, modernuo/Argon2.Bindings#13), so this picks them up and stops asking. ``` LibDeflate.Bindings 1.0.3 -> 1.0.4 Argon2.Bindings 1.17.0 -> 1.19.0 ``` ## zstd is dropped too, on every platform ZstdNet bundles `libzstd` for `linux-x64`, `linux-arm64`, `osx-x64`, `osx-arm64` and win, and nothing shells out to the CLI. Verified: the 15 `ManagedArchive` round-trip tests pass in a container with no `zstd` package installed and `which zstd` empty. Removed from the README, the macOS `brew install`, and CI — so the macOS runners now prove it rather than us assuming it. ## NativeLibraryChecker asks a different question It asked *"is package X installed"* via `dpkg -l` / `rpm -q`. That is what forced `-dev`, and no hardcoded name works for ICU anyway — its apt package is release-specific (`libicu70` on Ubuntu 22.04, `libicu76` on Debian 13). It now asks *"will the loader find this"*: `NativeLibrary.TryLoad` on the unversioned name, then `libfoo.so.N` descending through the range the runtime accepts. It deliberately does not consult a package database or `ldconfig -p`. Both answer a different question than "will `dlopen` succeed" — see the ICU section below for how that bit. ## What was wrong with the ICU check `libicuuc` was **inherited, not derived**. It came from translating the old package-name check into a library probe, without establishing which library that should be. Reviewing it turned up three defects, all of which could report ICU present on a host where the runtime then refuses to start: - **`libicui18n` was never probed.** The only ICU names in `libSystem.Globalization.Native.so` are `libicuuc` and `libicui18n`. `libicudata` arrives as a dependency of `libicuuc`, and `libicuio`/`libicutu`/`libicutest` are never referenced — so that is the complete list, and both are checked now. - **No version floor.** The runtime's `MinICUVersion` is 60, but the probe accepted down to `.so.0`. RHEL/CentOS 7 ships ICU 50, which passed and then aborted at startup. - **The `ldconfig` fast path bypassed the range.** A cache line for `libicuuc.so.50` still matches a `libicuuc.so` prefix test, so the floor was unenforceable through it. It also trusts a stale cache — observed reporting a deleted `libdeflate` as present. Removed in favour of asking the loader directly, which reads the same cache but answers the real question, and which also deletes the musl special-case (`ldconfig -p` exits 0 on musl while producing nothing usable). Worth knowing when this goes wrong in the field: **missing ICU does not throw, it `FailFast`s** — SIGABRT, exit 134, uncatchable. The process starts cleanly and dies later at whatever line first touches a culture, so the stack rarely implicates ICU. ## tzdata is a separate prerequisite, and nothing was checking it The event scheduler resolves configured zone IDs through `TimeZoneInfo`, which reads `/usr/share/zoneinfo`. It is data rather than a library, so no loader probe finds it, and slim container images routinely omit it. Without it every lookup except `UTC` throws `TimeZoneNotFoundException` and `GetSystemTimeZones()` returns 1 entry instead of ~419. There is no per-zone packaging to opt into — it is ~2 MB for the whole set. The one split that does exist is a trap rather than an optimization: Debian 12 and Ubuntu 24.04 move the legacy aliases into `tzdata-legacy`, so plain `tzdata` has `America/New_York` and `EST5EDT` but is **missing `US/Eastern` and `Asia/Calcutta`**. A shard configured with a legacy alias throws even though tzdata is installed. Documented, with both fixes. ## Why `InvariantGlobalization` stays false Dropping ICU entirely by turning on invariant mode looks tempting and is not safe. Because `Directory.Build.props` also sets `PredefinedCulturesOnly=false`, invariant mode does **not** throw `CultureNotFoundException` — it silently hands back invariant data. Measured on .NET 10: | Behaviour | With ICU | Invariant mode | |---|---|---| | `new CultureInfo("de-DE")` | real culture | succeeds, returns invariant data | | de-DE decimal separator | `,` | `.` | | `1234.5` as de-DE | `1.234,5` | `1,234.5` | | `string.Compare("a", "B", InvariantCulture)` | `-1` (linguistic) | `31` (ordinal) | | sort `[b, A, a, B]` | `a, A, b, B` | `A, B, a, b` | | `FindSystemTimeZoneById("Eastern Standard Time")` on Linux | resolves | `TimeZoneNotFoundException` | | UTF-8 round-trip of non-ASCII | unaffected | unaffected | Number parsing and formatting produce wrong values with no error, and culture-sensitive sort order silently becomes ordinal. Encoding is not the mechanism — UTF-8 round-trips fine either way. ## Documentation The rationale now lives in `dev-docs/platform-prerequisites.md` rather than in comments, so it is discoverable without reading the build tool: what each dependency is for, what breaks without it, per-distro package names, the ICU floor, the `tzdata-legacy` split, and why the check asks the loader instead of the package manager. README drops `libicu-dev`. Matching the runtime package by pattern (`'^libicu[0-9]+$'`) is version-independent without pulling in headers, so **no `-dev` package is required on any supported distribution** — which was the point of the whole change. ## CI now proves the claim instead of contradicting it The dnf job already installed runtime packages only. The apt job installed `libicu-dev`, which ships the unversioned `libicuuc.so` symlink — so every probe succeeded on the first attempt and the versioned-SONAME fallback this PR depends on was never exercised. Switched to the pattern match, verified to resolve exactly one package on jammy (70), bookworm (72), noble (74) and trixie (76). Added an assertion that the unversioned symlinks are absent. Without it the suite silently stops testing anything the moment a base image starts shipping one. Verified against all eight matrix distributions — none ship them — and confirmed the step fails as intended when a symlink is planted. ## Audit of every other native entry point Checked whether anything else has the same hazard. It does not: | Import | Verdict | |---|---| | `ws2_32.dll` — `SocketHelper` | Always present on Windows | | `libc` — `SocketHelper` | **Verified safe**, see below | | ZstdNet → `libzstd` | Bundled for every RID | | IORingGroup | No native library; raw syscalls | | ICU | Loaded by the .NET runtime itself, which probes versioned suffixes | `libc` deserved a hard look, because `libc.so` *is* a `libc6-dev` linker script while the real library is `libc.so.6` — the same shape as the bug being fixed. It is not affected. Measured in a container with no `libc6-dev`: ``` /usr/lib/x86_64-linux-gnu/libc.so ABSENT /lib/x86_64-linux-gnu/libc.so.6 present TryLoad("libc") LOADED <- resolves where "libdeflate" would not TryLoad("libc.so") not found getpid() -> DllImport("libc") WORKS ``` Confirmed on Alpine/musl as well. No code in this repo registers a `DllImportResolver`, and nothing else P/Invokes. ## `--check-prereqs` New flag. `Program.cs` only ran the SDK check in non-interactive mode — `NativeLibraryChecker` was reachable only through the Spectre-driven guided flow, so there was no way to verify a deployment target from a script or a container. It is what made the container verification below possible, and it prints the exact ICU package for the running release via `apt-cache`. It renders through the same `PrerequisiteChecker` the guided menu uses, rather than a second hand-rolled table that could drift from it. Spectre drops ANSI styling on its own when stdout is not a terminal, so redirected output stays clean; the console width is widened in that case so the install hints, which are shell commands meant to be copied, do not gain a newline mid-command. ``` ╭───────────────────────────╮ │ Checking native libraries │ ╰───────────────────────────╯ ✔ libicuuc (Found) ✔ libicui18n (Found) ❌ libdeflate (Not found) ❌ tzdata (Not found — every zone except UTC will throw) ⚠️ Install the missing dependencies. The -dev/-devel packages are not required: sudo apt-get install -y libicu74 libdeflate0 tzdata ``` Exit code carries the machine-readable half: 0 when everything resolves, 1 when anything is missing. ## Verification Against 1.0.4 and 1.19.0: build plus **810 Server.Tests and 642 UOContent.Tests**, on Windows and on Linux with **only** `libdeflate0` and `libargon2-1` installed — with the absence of the unversioned symlink asserted first so the run could not pass for the wrong reason. `--check-prereqs` verified in containers on Debian and Alpine across every state that matters: all present, each dependency removed individually, tzdata removed, a deliberately stale `ldconfig` cache, and ICU downgraded to `.so.50` to confirm the floor rejects it. Package resolution and the absence of unversioned symlinks checked on all eight CI distributions. |
||
|
Kamron Batman
|
6d81077772
|
perf(network): consume IORingGroup 1.0.9 to drop the per-iteration 6 KiB memset (#2558)
## Problem `IORingGroup`'s `WindowsManagedRIOGroup.DequeueRioCompletions` stackallocs `RIORESULT[256]` (6144 bytes) and runs **once per game-loop iteration** — `NetState.Slice` → `RingSocketManager.ProcessCompletions` → `PeekCompletions` → `DequeueRioCompletions`. The 1.0.8 package was compiled with the `.locals init` IL flag set, so every one of those calls memset the full 6 KiB before `RIODequeueCompletion` overwrote the entries it actually filled. An EventPipe profile of a near-idle shard (3 vCPU VPS, world saves off, one player logging in and moving around) put `System.Buffer.ZeroMemoryInternal` — called directly from `DequeueRioCompletions` — at **~2.8% of main-thread samples**, and it was the dominant frame in several 60–127 ms game-loop stalls. ## Why our existing attribute didn't cover it `Projects/Server/Module.cs` and `Projects/UOContent/Module.cs` already declare `[module: SkipLocalsInit]`. That attribute is a **compile-time** directive: it clears the flag in the IL of the assembly being compiled, and does not cross assembly boundaries. It never applied to the package. Verified by reading the shipped IL (`MethodBodyBlock.LocalVariablesInitialized`): | Assembly | attribute | methods with `.locals init` | |---|---|---| | `Server.dll` | present | 0 of 5439 | | `IORingGroup` 1.0.8 | **absent** | **158** | | `IORingGroup` 1.0.9 | present | **0 of 389** | ## Testing Built and tested against the locally-built 1.0.9 package (temporary local feed, not committed): - `dotnet build -c Release` — **0 warnings, 0 errors** - `Server.Tests` — **810 passed, 0 failed** - `UOContent.Tests` — **637 passed, 0 failed** - Confirmed the `IORingGroup.dll` deployed to `Distribution/` is the fixed build (0 of 389 methods zeroing) Only the `<PackageReference>` version changes; no source changes on this side. |
||
|
Kamron Batman
|
aae173a797
|
feat(network): allowlist false-positive IPs, escalate on behavior (#2556)
## Why The shard owner, on a Starlink CGNAT address, was blocked by the imported reputation blocklist. The cause was not CrowdSec. The address was a literal line in `ip-blocklist.txt`, so `BlocklistFilter` denied it at accept and then promoted it — and clearing the CrowdSec decision could not fix it either, because the file entry re-reports within `promoteSuppression` of every reconnect attempt. This is structural, not a one-off. Reputation feeds list shared consumer address space constantly: on CGNAT one public address fronts many subscribers **at the same time**, so a single abusive customer gets the address listed and everyone else behind it is blocked with them. Where leases rotate, a listing says little about whoever holds the address now. Around 1,000 Starlink addresses sit in the current list. So exemptions go where they cost nothing, and escalation is driven by what a connection actually does. ## Generator — `tools/Export-IpBlocklist.ps1` `-AllowlistFile` takes multiple paths, subtracted from the merged set before the output is written. Defaults to every `ip-allowlist*.txt` beside the output, merged into one allow set: - `ip-allowlist.txt` — operator exemptions, created once and **never rewritten** - `ip-allowlist-<name>.txt` — a carve-out you built, regenerable and copyable between shards **Subtraction is range-correct.** An allowlisted address inside a blocked CIDR splits that CIDR around the hole rather than being silently ignored. This also fixes `-ExcludeAnonymizers`, which parsed CIDR entries into `$anonCidr` and then only ever subtracted singles. **No carve-out ships.** A carve-out names a real network, and which ones a shard should exempt depends on where its players actually are — so publishing one would make that policy call for every shard and put a specific provider's address space in the repo. The script builds them on request instead: ```powershell .\Export-IpBlocklist.ps1 -AddCarveout starlink -Asn 14593 ``` Carve-outs are **discovered, not configured**: every `ip-allowlist*.txt` beside the output is subtracted, by the generator and by the shard, so a file an admin adds needs no config edit and no code change. Each carries an `asn=` marker in its header, which is how `-RefreshCarveouts` rebuilds it without the script keeping a list of anyone's networks; a hand-written allowlist has no marker and is never rewritten. Prefixes come from **announcements, not ownership records**, because registry data disagrees with what is actually routed and silently caps result sets: ARIN whois returns at most 256 rows and gives per-customer /24s, and `206.83.96.0/19` reads as APNIC in RDAP even though `206.83.96/21` is announced by Starlink. Editing an allowlist bypasses `-MinInterval`, so a just-added exemption isn't indistinguishable from the allowlist not working. A Starlink carve-out, if you build one, costs **~4,300 IPs + ~144 CIDRs of 4.2M (0.10%)**. ## Allowlists **`FileAllowlist`** reads the same files the generator subtracts, so an operator entry means "leave this address alone" for real. Subtraction alone only covers being *blocked*; behavioural detections never consult the blocklist, so without this a carve-out was quietly routed around — one scanner behind a shared address was enough to get everyone behind it contributed and firewalled, with nothing in the shard's own config explaining why. Reading the files also means an entry applies on the next reload rather than the next regeneration, which is what matters when someone is complaining now. **`LoginAllowlist`** is earned by authenticating, with a 90-day TTL because an address that logged in years ago is a stranger. Its own store rather than `Account.LoginIPs`, which has no timestamps and cannot be backfilled. An entry is evidence rather than a licence: 10 suppressed contributions in an hour revokes it, and a fresh login forgives the tally. Both are consulted **only after the blocklist has already matched**, so a normal accept pays nothing for them and the accept gate stays allowlist-free. `BanExemptions` combines them behind `BanChannel.IsExempt` and suppresses escalation only — every local defence still applies. Two limits, both deliberate and documented in the class: `LoginAllowlist` **cannot bootstrap** (an entry is only earned by getting in, so it never repairs an existing false positive), and it is weakest on rotating CGNAT. That is why `FileAllowlist` is the fix for those, and why it is manual. ## Behavioural detection | Reason | Trigger | |---|---| | `silent-connect` | Reaped after 5s having sent **zero bytes** | | `invalid-seed` | Opened with a zero seed | | `foreign-protocol` | Positively identified as HTTP, TLS or SSH | **`ForeignProtocol` inverts the test.** Asking "is this a good UO client?" cannot work: `LoginEncryption.ClientDecrypt` is a byte-for-byte stream XOR, so a legitimate client with encryption enabled when the shard expects none sends a structurally perfect connection whose payload is noise. "Speaks HTTP" is safe where "unreadable" is not — however misconfigured a UO client is, it never sends `GET / HTTP/1.1`. Nothing assumes arrival framing. TCP has no message boundaries, so a rule of the form "these bytes must arrive together" is broken by construction and drops real players on poor links. A prefix match with too few bytes to confirm waits for more. A four-byte seed can legitimately spell `GET ` (the address 71.69.84.32) or `0x16 0x03 0x0?` (22.3.x.x), so confirmation requires the request line to continue in printable ASCII or an actual ClientHello inside a plausible record — a real client's fifth byte is a packet id (`0x80`, `0x91`, `0xEF`), none of them printable, so those collisions fall through. Everything is keyed on **bytes-received rather than elapsed time**. A connection that sent something and ran out of time is far more likely a slow link than an attack, and banning those produces the worst failure mode available: the player retries, trips the rate limiter, and compounds a bad connection into hours of being firewalled off. ## `AutoDenylist` A short-lived local hold (15m) on behavioural detections, as `IConnectionFilter` + `IBanReporter` over one store so the engine detection sites never reach into content. This closes the gap where a flood pays for a socket, buffer and `NetState` slot per connection while waiting for the OS bouncer — the verdicts that matter most are reachable only *after* reading bytes — and it is the entire defence on a shard running no bouncer, which is the default config. Not persisted: a holding pen that survives restarts is a ban without a ban's review. Cost: one dictionary lookup on a usually-empty dict per accept. ## `BanReasons` Centralises the reason slugs. `IsBehavioral` is an **opt-in** set, not "everything except manual", so a future reason escalates normally instead of silently inheriting an exemption or entering a local denylist. This caught a real bug during review: the first cut of the exemption swallowed `manual` admin bans (`Commands.cs`, three sites in `AdminGump`) for any allowlisted address. ## Fixes found in review - **`BanConfiguration.Settings` was null until `Configure()` ran**, while the reap path dereferences it every `Slice()`. A harness driving `NetState.Slice()` directly hit an NRE that presented as flaky because it depended on whether an earlier test had already called `Configure()` — which is why it failed on some CI platforms and not others. Now starts at the record's defaults, with idempotency tracked by a flag; this also removes the same latent NRE from the pre-existing rate-limit path. - **`-AllowlistFile` was typed `[string]`** while documented and used as a list, so passing two paths would have collapsed them into one string. ## Layout and docs Content network code moves out of `Misc/` into `UOContent/Network/`, one concern per folder — `AutoDenylist/`, `Blocklist/`, `CrowdSec/`, `Firewall/`, `LoginAllowlist/`, `Packets/`. **Namespaces are untouched**, so these are pure file moves (git tracks all 16 as renames). `dev-docs/ip-bans-and-allowlists.md` documents the subsystem, leading with the operator process for unblocking a player — including the three things that look sufficient and are not: deleting the CrowdSec decision alone, editing `ip-blocklist.txt` by hand, and `cscli allowlists` alone. `.gitignore` covers the new config files. ## Testing Build clean. **Server.Tests 810 passed**, **UOContent.Tests 637 passed**, zero warnings. This branch adds 38 tests; the rest of the delta is main's, since this is rebased on current `main`. New coverage: TTL boundary and renewal, private-address exclusion, manual-ban-never-exempt, unopted-reason-never-exempt, strike revocation, quiet-window reset, login forgiveness, file-allowlist CIDR coverage, file-allowlist not spending the earned list's strikes, denylist expiry-on-read, cap enforcement, lapsed-entry reclaim, HTTP/TLS/SSH identification, seed-collision fall-through, and encrypted-login-is-not-foreign. Generator verified end-to-end against live feeds: a clean run ships no carve-out, `-AddCarveout starlink -Asn 14593` fetches and collapses 213 prefixes to 115 ranges in 0.1s over 4.2M entries, `-RefreshCarveouts` rediscovers it by its `asn=` marker, a hand-written allowlist is left untouched, and deleting a carve-out drops it rather than having it rewritten. CIDR splitting verified exhaustively: a single-IP hole in a /24 leaves exactly 255 of 256 addresses blocked. ## Operator note Existing installs are unaffected until the generator next runs, which creates `ip-allowlist.txt` and nothing else. To unblock someone: add the address to that file and delete any live CrowdSec decision — the existing ban outlives the config change. The shard picks the entry up on its next reload, so re-running the generator is optional. A shard whose players are on CGNAT (satellite, mobile, or an ISP short on IPv4) will likely also want `-AddCarveout`; see `dev-docs/ip-bans-and-allowlists.md`. ## Also included: a latent CI failure this PR surfaced `fix(tests): serialize test classes that rent through STArrayPool` touches a property-list test file that has nothing to do with this feature. It is here because it was failing macOS CI, and it is trivially cherry-pickable out if you would rather it went to `main` on its own — **which may be the better call, since it is failing `main` today.** CI has since gone green with it applied. `STArrayPool` is single-threaded by design and its bucket cache is a plain `static`, not `[ThreadStatic]`, with a check-then-act initialize in `Return()`: ```csharp var cacheBuckets = _cacheBuckets ?? InitializeBuckets(); ``` Two threads both see null, both initialize, and the loser trips `Debug.Assert(_cacheBuckets is null)`. Anything renting from it has to stay off parallel test threads — which is what the `DisableParallelization` collections are for. - `ObjectPropertyListReentrancyTests` and `ObjectPropertyListNestedBuildTests` (added in #2555) build property lists, which rent the interpolation buffer, but were not in the sequential collection — unlike `PropertyListInvalidationDuringBuildTests` in the same file. This is a **latent failure already on `main`**; it is timing-dependent, so it shows on some platforms and not others. - `AutoDenylistTests` (added here) has the same exposure: its cap tests reach `AutoDenylist.Sweep`, which rents a `PooledRefList` without `mt`. The blocklist tests need no marking because `BlocklistSnapshot.Build` asks for the `mt` pool explicitly. No production change — `STArrayPool` is the right pool on the game loop, where both `Sweep` and the property list actually run. ## Deliberately not included Waiting for a fragmented four-byte seed at `AwaitingSeed`. It looked like a bug but the disconnect is a deliberate defence: only pre-0xEF clients reach it (0xEF goes through `HandlePacket`, which already waits for its 21 bytes), and waiting converts an instant drop into a full 5s slot hold for a client sending one or two bytes, or a loris dribbling a byte every few seconds. Against a fixed 4096-entry `MaxConnections` table that trades capacity that matters for a fragmentation case a reconnect already fixes. |
||
|
Kamron Batman
|
b8d3fec59a
|
fix(opl): refuse property list invalidation raised from inside GetProperties (#2555)
## 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<char>.Shared` from the **finalizer thread**, and that pool is single-threaded by design. Left alone as a separate concern.
|
||
|
Kamron Batman
|
294dcd94a0
|
fix: Fixes send-path backpressure: consume IORingGroup 1.0.8, stop dropping packets silently (#2551)
## Summary Two related fixes on the outbound path: 1. Consume **IORingGroup 1.0.8**, which allows more than one send in flight per socket, and expose the two settings that go with it. 2. Stop `NetState.Send` silently discarding packets when the send buffer fills — including an out-of-bounds write reachable in that state. ## 1. Send-path stall (RIO) RIO reports send completion on **acknowledgement**, not on copy, so a completion cannot arrive sooner than one round trip. With one send in flight, `PostSend` refused to post again until the previous completion arrived — capping a connection at **one send per RTT** whenever it had data queued. Measured on a 50ms-RTT production shard: | | before | after | |---|---|---| | in-game latency, data flowing | **101–146 ms** | **48–51 ms** | | p95 | ~135 ms | 52.8 ms | | samples > 70 ms | 20 | **0** | The control that confirms the mechanism: server-side post→completion was **unchanged** at median 92ms across both runs. The ACK-binding is inherent to RIO and did not move; only its propagation into application latency did. Two things worth recording, because they explain why this went unnoticed: - As little as **6 bytes** of queued data held the gate shut, so it reproduced in empty areas, not just crowded ones. - The same measurement at loopback RTT is **microseconds**, so local testing could never surface it. New settings, both restart-time: - **`network.maxOutstandingSends`** (default 32) — sends in flight per connection. Honoured by RIO only; other backends complete sends on copy and report 1. Costs a request-queue and completion-queue slot per send, **not another buffer**, since every outstanding send addresses a different range of the same registered buffer. Worst-case added latency is roughly `completion RTT / value`. - **`network.sendBufferSize`** (default 256KB) — per-connection send buffer, coerced to a power of two of at least the platform allocation granularity. This is the lever for the disconnects below, and the per-connection memory ceiling. ## 2. Send buffer full `NetState.Send` had three failure modes once the buffer filled, none of them visible: | writable | behaviour | |---|---| | `0` | `GetSendBuffer` returned false → **packet dropped**, no log, no disconnect | | `4 … needed-1` | `Compress` returned 0 → `CommitWrite(0)` → **packet dropped** the same way | | `1 … 3` | `safeOutputLength = (nuint)output.Length - 4` **underflows** → hot-loop bounds check never trips → **writes past the span** | The first two leave a client connected while quietly missing game state, which is undiagnosable from either end. The third corrupts the in-flight region of the ring buffer, and is reachable precisely when a connection is congested, since callers only check for non-zero space. `Compress` now refuses an output too small to bound, and `Send` reports exhaustion instead of dropping — logging and disconnecting with **needed / writable / unacked / capacity**. Those numbers separate a slow client holding the buffer from a buffer genuinely too small for the shard, which is the case that warrants raising `network.sendBufferSize`. ## Testing `NetworkCompressionBoundsTests` covers the underflow using sentinel bytes around the output window. **Verified to fail without the guard** (4 failures from overwritten sentinels), confirming the out-of-bounds writes were real rather than theoretical. Full suites green: **788 Server.Tests**, **597 UOContent.Tests**, Release build clean against the published 1.0.8. ## Notes for reviewers - Upstream change: modernuo/IORingGroup#9. - The buffer-full path is now *loud* where it used to be silent. If a shard has been quietly dropping packets under load, this will surface as disconnects — that is the intended outcome, and the log line says which setting to raise. - Follow-up under discussion: promoting a connection to a larger buffer instead of disconnecting, which looks feasible on a live connection since buffers are referenced per-operation rather than bound to the request queue. |
||
|
Kamron Batman
|
c909ed1f2f
|
fix: Streamlines insurance. Insurance only executes when enabled. (#2550)
### Summary
Moves inventory insurance out of `Mobile`/`PlayerMobile` into its own system at `Projects/UOContent/Engines/Insurance/`, wires it into the feature flag system, and makes disabling it actually disable it everywhere.
### Changes
**New `Server.Engines.Insurance.Insurance` system**
* Owns its own `Configure()`, seeding from the existing `insurance.enable` setting (default `Core.AOS`), so no config migration is needed. `Mobile.InsuranceEnabled` is gone, along with its line in `ExpansionConfiguration`.
* `CanInsure`, `GetInsuranceCost`, `ToggleItemInsurance`, `AutoRenewInventoryInsurance`, `CancelRenewInventoryInsurance` and `OpenItemInsuranceMenu` move here from `PlayerMobile`, which keeps four one-line shims for the context-menu callbacks.
* Every entry point is gated on `Insurance.Enabled`, and the death-time state is only allocated when insurance is on — a shard without insurance pays nothing for it.
**Feature flag integration**
Insurance is now a first-class feature flag: `ServerFeatureFlags.InsuranceEnabled`, registered under the `insurance` key in `FeatureFlagManager.SyncStaticFlag`, so it can be inspected and toggled through the normal flag command/gump rather than only at boot. `Insurance.Enabled` reads through to the flag, so there is one source of truth for every consumer.
**Fixes a memory leak from PvP**
`PlayerMobile.m_InsuranceAward` was a `Mobile` field assigned on every death and never cleared, so every player permanently pinned a strong reference to the last player who killed them. Killers were kept alive by their victims indefinitely.
Death-time insurance state now lives in a `Dictionary<Mobile, InsuranceContext>` owned by the insurance system: the entry is created in `OnBeforeDeath` and removed in `OnDeath`, so nothing outlives the death that created it.
**Removes insurance fields from every PlayerMobile**
`m_InsuranceAward`, `m_InsuranceBonus` and `m_NonAutoreinsuredItems` were carried by every `PlayerMobile` whether or not the shard ran insurance. All three are gone; the equivalent state is allocated per-death, only for players who actually die with insured items, only when insurance is enabled.
**Stale `Insured` flags are inert when insurance is off**
`Item.Insured` is a persisted flag, so items stay marked after a shard turns insurance off. Every read path now checks the flag first, so those items behave exactly as if they were never insured:
* `Item.CheckBlessed` / `Item.IsStandardLoot` — they drop again instead of acting blessed
* `Item.AddLootTypeProperty` — no more phantom "insured" tooltip
* `PlayerMobile.FindItems_Callback` — not yanked out of nested bags on death
* `DestroyEquipment` — no longer immune
* `ClothingBlessDeed` — no longer reports "that item is already blessed"
**Gumps promoted out of `PlayerMobile`**
`ItemInsuranceMenuGump`, `ItemInsuranceMenuConfirmGump` and `CancelRenewInventoryInsuranceGump` were private nested classes reaching into `PlayerMobile` privates. They are now public types in `Engines/Insurance/Gumps/`, talking to the insurance system through its public API. `ItemInsuranceMenuGump.ToggleSelected()` replaces the confirm gump's reach-in to the parent's `_items`/`_insure` arrays.
### Behavior changes
* The per-item "You lack the funds to purchase the insurance" message on failed auto-renewal is no longer sent during death; players get the single 1061115 summary instead. Marked with a TODO pending a decision on whether the per-item message should spam.
* The killer's insurance bonus is deposited once at the end of death processing rather than 300 gold at a time per insured item, and the "gold has been deposited" message is now conditional on the deposit succeeding. Same total.
### Drive-by cleanups
`PoisonImpl.IncreaseLevel` -> `Poison.IncreaseLevel`, a redundant `is NetState { } ns` pattern, `new List<Item>(Items)` -> collection expression, alignment of the `SyncStaticFlag` switch arms, and some comment/formatting fixes in `PlayerMobile`.
|
||
|
Kamron Batman
|
c39454137e
|
feat(network): pluggable connection filters; file blocklist + contribute-first CrowdSec (#2542)
Reshapes IP banning around one idea: **core owns the question, content owns every answer.**
Core gains a single accept-path seam — `IConnectionFilter` — and loses everything that used to implement one. The firewall moves to UOContent, a new file-backed blocklist joins it there, and CrowdSec is repositioned from an in-app enforcer to a contribute-first reporter.
## The seam
```csharp
public interface IConnectionFilter
{
string Name { get; }
void Configure();
void Start(CancellationToken token);
void Stop();
bool ShouldDeny(IPAddress address);
}
```
The accept path went from hardcoded branches to one question:
```csharp
else if (ConnectionFilters.ShouldDeny(remoteIP, out var deniedBy))
{
logger.Debug("{Address} denied by connection filter '{Filter}'", remoteIP, deniedBy);
}
```
Filters register during the Configure sweep. The registry is a plain array walked by an indexed loop — no enumerator, no closure, no allocation — and the first denial short-circuits. An interface dispatch is noise next to the `accept()` syscall, so pluggability costs nothing measurable on the path that has to survive a DDoS.
Whatever a hit implies — persisting, promoting to an OS bouncer, contributing to the ban channel — is the filter's business, not the accept path's.
A filter that throws is **unregistered and the connection fails open**. A filter that faults once faults for every subsequent connection, so leaving it registered means an exception and a log line per accept — exactly the amplification an attacker wants — and a broken filter must not be able to deny everyone either.
This deliberately does **not** reuse `EventSink.InvokeSocketConnect`: that fires later and allocates a `SocketConnectEventArgs` per connection, which is what the accept path avoids for rejected traffic.
## What ships behind it
**`firewall`** (UOContent) — the existing admin-curated set. Collapsed from `Firewall` + `AdminFirewall` + a threaded enforcer into one single-threaded store with **zero concurrency primitives**: the accept path, admin gump, TTL expiry and boot load all run on the game loop. Persists to `Configuration/firewall.json` with automatic migration from the legacy `firewall.cfg`. No behavior change for operators — same namespace, same gump, same commands.
**`blocklist`** (UOContent) — new. Holds a millions-strong list in-app and **demand-pages** hits up to CrowdSec, which promotes them to the OS firewall.
The motivation is concrete: CrowdSec's Windows bouncer cannot load the ~3.9M IPs that 91 community feeds produce, but it handles ~100k fine. So the millions live in-process behind a binary search, and only addresses that *actually connect* get promoted. A `PromotedGuard` suppresses re-reporting an address until the bouncer picks it up.
The list is parsed straight from UTF-8 file bytes with no per-line string allocation, off the game loop, and published as an immutable snapshot swapped through a single `volatile` reference. Reloads yield to world saves.
**`tools/Export-IpBlocklist.ps1`** — the producer. Requires PowerShell 7 and runs on Windows, Linux and macOS; Windows PowerShell 5.1 is refused up front via `#requires`. Merges a thin, non-overlapping feed set into one de-duplicated, bogon-filtered file. Parsing runs in a compiled `Add-Type` hot loop (~1s for ~4M lines instead of minutes). Written to a `.tmp` sibling and swapped with `File.Replace`, so the shard never reads a half-written list, and a total feed outage refuses to overwrite a good list with an empty one. Re-running is idempotent — it exits without downloading anything while the list on disk is younger than `-MinInterval` (default 2h, the anchor feed's own refresh period), so a misconfigured scheduler can't hammer upstream.
## CrowdSec: contribute-first
`IBanReporter` + `BanChannel` fan locally-decided bans out to external systems. `CrowdSecReporter` (UOContent) posts to LAPI `POST /v1/alerts` and retracts via `DELETE /v1/decisions`.
Reporting is **enqueue-only** on the accept path: a bounded, coalescing channel drained off-loop with bounded retry, counted drops on overflow, and a flush on shutdown. Under a DDoS the accept path never does synchronous or lock-contending per-IP work.
### Why not pull decisions from CrowdSec?
The original design streamed decisions into an in-app snapshot and enforced them at the accept gate. That's the wrong layer: by the time the shard sees the connection, the TCP handshake and socket setup are already paid for. `cs-firewall-bouncer` drops the same traffic **at the kernel**, and it's what CrowdSec is built to do. So the shard now contributes what it uniquely knows (rate-limit trips, blocklist hits from real connection attempts) and lets the OS enforce.
The one thing the OS can't do — hold millions of entries on Windows — is exactly what the in-app blocklist covers, and it feeds the same pipeline.
## Threading policy
`CLAUDE.md` rule #3 is rewritten as an explicit three-part policy, with rule #10 restated in tandem:
- Anything touching game state runs **only** on the main loop.
- Heavy work that *needs* game state must be **chunked** across ticks, never threaded.
- Heavy work that does *not* need game state (large-file parse, external I/O) **must** run off-loop **and must yield to world saves**.
Results come back via an immutable snapshot swapped through a single `volatile` reference, or `Core.LoopContext.Post` — never by letting the scheduler decide where heavy work runs. Both new subsystems follow it.
## Shared primitives
`SortedRangeIndex<T> where T : IBinaryInteger<T>` — coalesced disjoint interval arrays plus a binary search. The firewall, the blocklist, and (as of this PR) core's reserved-network tables all use it.
Coalescing is a correctness requirement, not an optimization: multi-feed lists nest CIDRs (`/24` containing a `/32`), and a search that inspects only the rightmost run whose minimum is ≤ the value is sound **only** over disjoint runs. That bug was caught in review and is covered by regression tests.
`IPAddressUtility` collects the allocation-free `IPAddress` ↔ `UInt128` conversions and CIDR parsing that were previously scattered or duplicated.
## Config
| File | Owner | Keys |
|---|---|---|
| `Configuration/bans.json` | core | `reportRateLimitTrips`, `autoBanDuration` |
| `Configuration/blocklist.json` | content | `file`, `reloadInterval`, `reportHits`, `banDuration`, `promoteSuppression` |
| `Configuration/crowdsec.json` | content | `lapiUrl`, `machineId`, `password`, `origin`, `manualBanDuration`, `flushInterval`, `maxQueue` |
| `Configuration/firewall.json` | content | persisted firewall entries (migrated from `firewall.cfg`) |
Everything is inert by default. CrowdSec self-disables without credentials; the blocklist self-disables until its file exists. A shard that changes nothing sees no behavior change.
## Notes for review
- **Core no longer references `Firewall` or `IFirewallEntry` anywhere.** `NetworkUtilities` used to build its reserved-network tables out of `CidrFirewallEntry`, which coupled core to the firewall for something unrelated to banning; those are now a `SortedRangeIndex<UInt128>`, same semantics and public API.
- **`BanChannel.Stop()` no longer persists the firewall** — a contribution coordinator has no business saving an enforcement store. That's the firewall filter's `Stop()`.
- **A dead `whitelisted` parameter was dropped** from the blocklist gate: it was hardcoded `false` at its only call site, and no whitelist concept exists in core.
- **The blocklist filter is an instance, not a static.** The static version forced its tests onto the sequential collection with a reset hook; they now run in parallel.
- `dev-docs/networking-packets.md` documents the seam for content authors, plus a known wart in the `IPAddress` ↔ `UInt128` normalization flagged for a follow-up PR.
- The generator was verified on Linux, macOS and Windows under a temporary CI matrix (since removed). It caught two portability bugs — a Windows-only path separator, and a culture-sensitive duration parse that read `2.5` as `25` on comma-decimal locales and *silently* turned a 2.5h cooldown into 25h — plus a third that made the script unparseable on Windows PowerShell 5.1. The source is ASCII-only for that last reason: `#requires` is only honored once a file parses, so non-ASCII in a BOM-less script produces parse errors instead of the version message.
## Tests
**1344 pass** (782 `Server.Tests`, 562 `UOContent.Tests`). New coverage: filter registry (registration, short-circuit, fault-disable), blocklist parsing/CIDR/coalescing, snapshot reload markers, promote-guard TTL, ban-channel fan-out, CrowdSec alert building/dedup/flush-on-stop, and the generator's output-format contract pinned against the reader.
|
||
|
Kamron Batman
|
858c1d18bc
|
fix(opl): only apply the ':#' cliloc marker to integer values (#2540)
`ObjectPropertyList.AppendFormatted<T>(value, format)` treated **any** `{value:#}` as the cliloc marker (emitting `#<value>`). But cliloc numbers are integers — a `float`/`double`/`decimal` formatted with `#` is the standard custom-numeric (`#` = digit placeholder) format, not a cliloc reference, so those were being mis-marked.
Gate the marker on an integer value type:
```csharp
if (format == "#" && value is int or uint or long or ulong or short or ushort or byte or sbyte)
```
Now `{someFloat:#}` formats normally (passes `#` through to `TryFormat`); the marker/standard-format ambiguity narrows to the harmless `{0:#}` **integer** case (`#0`). Existing `AddLocalized(int)` / `{value:#}` (all `int`) are unaffected.
Adds `ObjectPropertyListSpanAddTests.HashFormat_OnlyMarksIntegers`: `int {value:#}` → `#<value>`; `double {value:#}` → `42.0.ToString("#")` (`"42"`, no `#`).
|
||
|
Kamron Batman
|
e12cc5dd83
|
perf(saves): eliminate world-save freeze bottlenecks (~9.5x faster freeze) (#2525)
Reduces the world-save freeze window from ~740ms to ~78ms (measured on a synthetic 10M-entity / 1.7GB world, 24 cores, through the real pipeline classes) by removing the per-entity handoff between the game loop and the serialization workers, fixing how large indivisible payloads are scheduled, rewriting the BufferWriter hot path, removing per-entity placement state entirely, and finally replacing the global serialized-types tracking with a per-file type table (idx v4) that also shrinks idx files by ~21% and speeds the background write phase. The pipeline has also been validated end-to-end on live-copy worlds in the multi-million-entity range, where the freeze is drain-bound (real `Serialize()` costs far more CPU per byte than synthetic writes) — the same structural wins hold, and entity/file round-trips are byte-clean across both load paths.
## The problem
The freeze window is `max(main-thread handoff, slowest worker drain)`:
1. **The producer was the bottleneck.** The main thread round-robined every entity through per-worker `ConcurrentQueue`s — two interlocked ops per entity, ~740ms of freeze floor at 10M entities before any serialization happened.
2. **Round-robin distributes count, not cost.** "Deep" systems (50MB generic persistence blobs) and "thick" entities (100K-item storage keys) landed on arbitrary workers, producing lopsided drain times on large worlds.
3. **Worst-case scheduling.** `GenericEntityPersistence.Serialize` pushed its self-payload *after* all entities, and generic persistences sort last in the registry — so the biggest indivisible blobs started serializing at the very end, extending the freeze by their entire duration.
## The fix
**Commit 1 — chunked handoff + LPT scheduling + heap pre-sizing:**
- Pooled 4096-entity chunks published to one shared queue; workers pull chunks and load-balance dynamically (a worker busy with a thick entity simply takes fewer chunks).
- `Persistence.SerializeAll` pushes systems largest-first (LPT) using the previous save's payload size (or loaded file size on first boot); self-payloads get dedicated single-entity chunks so they overlap the entity stream instead of ending it.
- Worker heaps pre-size from the loaded save's `.bin` totals, eliminating copy-on-grow inside the first save's freeze.
- `SpinWait` backoff in the drain loop (never `Sleep(1)`), per-worker balance stats logged in debug builds (the call site is compiled out of Release), 1MB snapshot write buffer.
**Commit 2 — workers iterate the dictionaries directly + main thread joins the drain:**
- `GenericEntityPersistence` publishes 4096-slot ranges over its dictionary's backing entries array; workers serialize occupied slots (`value != null`) directly through a `ShadowEntry<TValue>` struct mirroring the runtime's private `Entry` layout. Safe because the dictionary is frozen during `Saving` (mutations divert to the pending safety queues).
- The layout is **proven at startup before any code reads through it**: validation measures the true `Entry` stride via precise allocation accounting (guaranteeing all shadow reads are in-bounds), then verifies every key/value of a churned, resized, freelist-exercised dictionary — reading value slots as raw pointer bits only, never materializing a managed reference until the layout is proven. If a future runtime changes `Dictionary` internals, validation fails with a logged warning and saves fall back to the (fully maintained) enumerate-and-push path.
- The main thread joins the drain via an inline worker after publishing, instead of idling — worth a full worker share, proportionally more on low-core hosts.
**Commit 3 — 2.2x faster BufferWriter write path, single-pass short strings:**
- PGO already devirtualizes and inlines every `IGenericWriter.Write` callsite (interface vs concrete measured identical) — the real per-write cost was the non-inlinable `Index` setter (range-check throw path + per-write high-water tracking) plus span bounds checks. Writes now reserve capacity once, then do an unaligned store through a ref with a raw index increment; the high-water mark folds at Seek/Resize instead of per write.
- Class-level implementations of the hottest default interface methods keep nested writes inlined (a DIM re-dispatches on `this` even at a devirtualized callsite).
- Strings of 85 chars or fewer encode once into a stack scratch instead of walking the string twice (`GetByteCount` + `GetBytes`). Byte output is identical.
- Measured: 34.4 → 15.7 ns/entity on a generated-style write mix; end-to-end freeze ~99ms → ~74-82ms.
**Commit 4 — branch-free fallback push loop:**
- A bare `foreach { PushToCache(entity); }` runs at 2.3ns/entity; the same loop carrying a per-entity heavy-check runs 2.3x slower — the cost is the fatter loop body defeating tight-loop codegen. Entity-level >1MB payloads are rare enough to ride in shared chunks; system self-payloads (the large ones) are still explicitly scheduled largest-first.
**Commit 5 — drop the 9-byte per-entity placement state; snapshots write from worker segment logs:**
- Every `ISerializable` carried `SerializedThread/SerializedPosition/SerializedLength` so `WriteSnapshot` could gather each entity's bytes from the worker heaps in dictionary order. But the idx records absolute positions — bin order is free — so the snapshot is now written in worker-heap order and the join inverts: workers log segments (owner, slot range, heap start) plus one length per record as they serialize; positions are implicit because a worker's writes are contiguous, and identity comes from re-walking the same snapshot slots in the same order (stable until `PostWorldSave`).
- Chunks are persistence-homogeneous (the partial chunk publishes at each `SerializeAll` boundary) so segments route to files by owner with zero per-entity state. Self-payloads keep placement as three private fields on the handful of persistence instances.
- Net: 9 bytes (plus padding) of resident state removed from every item, mobile, guild, and account on every shard; three interface-property stores per entity leave the drain hot path (stamping dirtied one cache line per entity mid-freeze — the lengths log is one sequential stream); each segment's bytes hit the bin as a single span write instead of one copy per entity, speeding the background write phase; and `IGenericSerializable` shrinks to just `Serialize(IGenericWriter)`. Transient cost: ~4 bytes per entity in pooled per-worker logs, released after each write. The save format was unchanged at this point (idx v3, same loader); the v4 bump comes later in the branch.
**Commit 6 — staged file writes replace memory-mapped snapshot writing:**
- `MemoryMapFileWriter` is removed. `FileBufferWriter` composes through the full `BufferWriter` raw write path into a pooled staging block that drains to the file as large sequential positional writes (`RandomAccess.Write`); seeks flush the block and move the file offset, so backwards patches (the idx entity count) become small positional writes.
- Memory-mapped composition paid a soft page fault on every composed page plus unpredictable dirty-section teardown stalls at dispose — measured ~4x slower end-to-end than staged writes at snapshot sizes.
**Commits 7–11 — idx v4: per-file type table replaces SerializedTypes.db and all runtime type tracking:**
- Previously every `Write(Type)` from every worker enqueued into a shared `ConcurrentQueue<Type>` during the freeze (interlocked writes on a shared cache line, millions of mostly-duplicate entries), the background phase drained and deduped it all into a `HashSet`, and the snapshot recomputed `xxHash64(Type.FullName)` once per entity record (~5.4M redundant hashes per save on a large world) to write 9-byte tag+hash idx records plus a global `SerializedTypes.db`.
- The db's only real job was diagnostics: the string name behind "Type `<X>` was not found. Delete all of those types?" during idx loading. That map now lives in the idx itself: each `GenericEntityPersistence<T>` keeps an insertion-ordered `Type -> ushort` table, hydrated at `AddEntity` and on every deserialize path — one dictionary `TryAdd` per entity add on the game thread, amortized across gameplay, and provably immutable while the background writer reads it (adds divert to the pending queues during saves).
- idx v4 layout: the table (names only) is written before the records; records reference it by 2-byte index, shrinking from 33 to 26 bytes (−21%). The loader resolves each table name **once** (`FindTypeByHash(ComputeHash64(name))` — semantically identical to v3 resolution, `TypeAlias` included) into a constructor array, and each record becomes an array index instead of an 8-byte hash read plus dictionary probe. The unresolved-type prompt now surfaces once per type, with the name.
- Deleted outright: `World.SerializedTypes`, the drain/dedupe pass in `WriteFiles`, `BufferWriter`'s type tracking (its `Write(Type)` is now pure — payload format unchanged: tag byte + xxHash64), `FileBufferWriter`'s typeSet parameter, `Persistence.WriteSerializedTypesSnapshot`, and the adhoc db write. SerializedTypes.db is no longer produced.
- **Backward compatibility:** v0–v3 saves (including their SerializedTypes.db and legacy tdb files) load exactly as before, and every legacy load path hydrates the new table so the first v4 save after an upgrade is complete. Stale db files in existing save folders are simply ignored. Verified live: a v3 save boots, saves as v4 (Items.idx −20.2% on a dev world), and reloads with identical entity counts.
## Measured (synthetic 10M entities / 1.7GB, 64+64+32MB system blobs, 24x 2MB thick entities, dense write profile, 24 cores)
| Metric | Before | After |
|---|---|---|
| Steady-state freeze | ~740 ms | **~78 ms** |
| Main-thread publish cost | ~740 ms | **~0.1 ms** |
| Steady-state allocations | 0 | 0 (by iter 2) |
| Worker byte-load spread | 2x | ~1.15x |
The freeze is now bound by pure serialize throughput (payload / cores).
## Tests
- 779 Server.Tests + 501 UOContent.Tests pass.
- New across the branch: chunk fill/flush/owner-boundary tests, pool reuse/clear tests, an end-to-end multi-worker drain through the real wake/push/flush/pause protocol, a 50K-entry churn equivalence test for the shadow iteration re-walk (the exact pairing the snapshot writer relies on), byte-level BufferWriter output/position pins, `RuntimeLayoutIsSupported` so a silent fallback on a future runtime upgrade fails loudly in CI, and a full snapshot **round-trip test** that serializes 25K entities plus a self-payload through real workers, writes the idx/bin from the segment logs, and reloads them through the standard loader (now in v4 format).
- For idx v4 specifically: `FileBufferWriter` staging/drain/seek-patch and oversized-item tests, type-table registration tests, a hand-written v4 fixture proving an unresolvable type name skips only its own records through the console confirmation flow, and a hand-written **legacy v3 fixture** proving old saves still load and hydrate the type table for their next save.
## Trade-offs
- Chunk scheduling is nondeterministic, so worker heaps ratchet to each worker's max-ever draw rather than a fixed share. With slot ranges the balance is tight (~1.15x), so the effect is small; a shared slab pool remains an option if production shows retention creep.
- Entity-level heavy items inside slot ranges are serialized wherever they're encountered (no LPT for them); worst-case tail is one thick entity's serialize time (~ms). System self-payloads — the large ones — are still explicitly scheduled largest-first.
- Snapshot-write error granularity is per segment rather than per entity (heap-bounds bugs were the only thing the per-entity catch ever caught; idx metadata reads keep per-record granularity).
- idx v4 is a save-format version bump: old saves load unchanged through the preserved legacy paths, but saves written by this branch require this loader. Per-persistence type tables cap at 65,535 distinct entity types per boot (hard throw, orders of magnitude of headroom), and a type's table slot persists until restart even if its last entity is deleted — a few stale name entries per file, by design.
|
||
|
Kamron Batman
|
7434ed7ee1
|
fix(console): stop headless servers from pegging a CPU core (#2535)
## Problem On headless Linux deployments (systemd service, Docker without a TTY, `nohup`), the ModernUO process pegs a full CPU core even when idle. It does not reproduce on Windows because that runs with an interactive console. ## Root cause `ConsoleInputHandler` runs a background thread (named "Console Input Handler") that loops on `Console.ReadLine()`. When stdin is **not** an interactive terminal, `Console.ReadLine()` returns `null` at end-of-stream **immediately** on every call, so the loop `continue`s in a tight spin — one core at 100%. Reproduced in a container running the actual distribution: the "Console Input Handler" thread sat at ~90% CPU on a headless boot; with a blocking stdin it dropped to idle. ## Fix 1. **Detect headless once at startup:** `Core.Headless = Console.IsInputRedirected`. 2. **Extract a testable `ConsoleInputPump`** that owns the input stream: per line read, it *atomically* (under one lock) either delivers the line to a waiting prompt or dispatches a console command, and it **ends on EOF instead of spinning**. Cleanup runs unconditionally in a `finally`, so a pending prompt is always released (never hangs). Replaces the old `async void` loop and the fragile `_expectUserInput` / two-`AutoResetEvent` / `_input` handshake. 3. **`ConsoleInputHandler` becomes a thin headless-aware facade** over the pump. Headless: the reader thread never starts (`Console input disabled (headless: stdin is not a TTY).`), and `ReadLine()` throws a fatal `HeadlessConsoleInputException`. 4. **Data-gating and first-boot prompts** (deserialization "delete bad types? y/n", save-conflict, config/expansion setup) now route through `ConsoleInputHandler.ReadLine()`, so a headless server crashes fatal with a clear message instead of reading `null` (previously an NRE or a silent wrong branch). Design decision (model b): headless servers are expected to be supplied with configuration/save data (including the owner account); interactive prompts when headless are fatal by design. ## Testing - New `ConsoleInputPumpTests` (5 tests): EOF ends the loop without spinning; command dispatch; a pending prompt receives the next line; EOF while a prompt is pending completes it with `null` (no hang); a throwing command lookup does not hang a pending prompt. The tests synchronize on real pump state (no `Thread.Sleep`), so they are deterministic on slow CI. - Full `Server.Tests`: no new failures introduced. ## End-to-end verification (Docker, real distribution) | | Console Input Handler thread | Container CPU | |---|---|---| | Before fix (headless boot) | ~90% | ~199% (2 cores) | | After fix (headless boot) | **not started** | **~11%** | After the fix, a headless boot logs `Console input disabled (headless: stdin is not a TTY).`, loads the world normally, and idles instead of spinning. |
||
|
Kamron Batman
|
f4a771c19d
|
fix: Items dropped on the ground never decay (#2536)
## Problem Items dropped on the ground never decay. Corpses do, which makes the breakage look selective — but corpses are unaffected only because `Corpse.BeginDecay` runs its own `InternalTimer` and never touches `DecayScheduler`. Ordinary items are the only things that depend on the scheduler. ## Root cause `Item.MoveToWorld` called `SetLastMoved()` — which triggers `UpdateDecayRegistration()` — at the *top* of the method, before detaching the item from its parent and before assigning the new map. `CanDecay()` reads `Decays`, `Parent`, **and** `Map`, so registration was evaluated against the item's *pre-move* state. Because the `Item` constructor sets `m_Map = Map.Internal`, and `Mobile.Lift` calls `item.Internalize()` to put an item on the cursor, registration was consistently one step behind: | State | Tracked for decay? | | |---|---|---| | Item on the ground | **No** | never decays | | Item held on the cursor | **Yes** | backwards | Nothing corrected it afterwards: the later `m_Map = map` assigns the field directly, bypassing the `Map` property setter, and that setter does not refresh registration either. With no parent, `RemoveItem` (which *does* re-register) never runs. World load masked this — `ItemPersistence.PostDeserialize` re-registers every item against its final state, so decay appears to work for items that survive a restart. Only freshly dropped items are affected. **Fix:** stamp `LastMoved` up front so decay math stays correct, then call `UpdateDecayRegistration()` once the parent, map, and location are final. ## Audit of the rest of the call sites All 16 `SetLastMoved()` call sites were reviewed. `SetLastMoved()` must keep refreshing registration — `LastMoved` feeds `ScheduledDecayTime` and therefore which bucket an item belongs in — but it may only run once parent/map are final. The vendor, house, lift and drop sites already satisfy that. The rest of this PR fixes the ones that did not, plus what the audit turned up: - **`Item.Deserialize`** stamped via `SetLastMoved()` before the version data was read, registering against an unread `Map`/`Parent`. Safe only by accident (the `Item(Serial)` ctor leaves flags at 0, so `Decays` is false), and it cost an unregister per item per world load. Now stamps only; `PostDeserialize` does the registration. - **`Item` constructor** registered then immediately unregistered every item — the `Movable` setter saw `m_Map` still null, so `CanDecay()` was true. Also removes a `Configure`-order landmine: constructing an `Item` before `DecayScheduler.Configure()` would have thrown in `Shared.Start()`. - **`Container.Destroy`** stamped `LastMoved` immediately before `MoveToWorld`, which now stamps it itself. - **`Unregister` was documented O(1)** but scanned twelve buckets and did a linear `PriorityQueue.Remove`, on every construction, deserialize and move. Items now record where they are tracked in `Item.DecaySlot` (1 byte), so untracked items — the common case — leave in O(1). - **A refused decay silently dropped the item.** `ProcessActiveQueue` deleted on `OnDecay() == true` but did nothing when a region refused, leaving the item dequeued, untracked and on the ground forever. It now restarts the decay clock; re-registering as-is would spin, since `ScheduledDecayTime` is already past. ## Two content bugs of the same class The decay system replaced a polling sweep. Under polling, a `Decays`/`DecayTime` override could read live state every pass. Under a registration model it cannot — the scheduler drops items that stop being eligible, but nothing enrols one that becomes eligible while untracked. - **`TreasureChestLevel1-4`** overrode `DecayTime` as `Utility.Random(15, 60)` — a fresh roll on every read. `ScheduledDecayTime` is read repeatedly (to bucket, to re-bucket on rotation, to test whether due), so those reads disagreed: the chest re-bucketed every tick and decayed early instead of after its intended interval. Now rolled once per chest. Distribution unchanged — `Utility.Random(from, count)` is RunUO's `from + Next(count)`, so this is 15–74 minutes, as before. - **`StrongBox`** overrode `Decays` with a live check on `_house`, `_owner.Deleted` and `IsCoOwner`. Nothing notifies the box when any of those change, so it was never enrolled and the override never decayed anything — and it could not have: `HouseRegion.OnDecay` refuses a secured item inside a standing house, and the box is in `Secures`. Decay was never the mechanism here. - 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 `Validate()` destroys it. The old check missed exactly those two cases — it required a non-null owner and treated a null house as valid. A deleted owner deserializes back as null, which `IsCoOwner` rejects. The now meaningless `Decays`/`DecayTime` overrides and an unhelpful `Console.WriteLine` are gone. `DecayScheduler` now documents both constraints. ## Tests `DecayRegistrationTests` (Server) covers world placement, lift/drop, container round-trip, cursor-held (must *not* track), `Container.Destroy` spill, `DecaySlot`/structure agreement, refused decay, and a full decay lifecycle driven through the scheduler. `TreasureChestDecayTests` (UOContent) locks `DecayTime`/`ScheduledDecayTime` stability across all four chest levels. To make the lifecycle testable deterministically, `DecayScheduler` gains `internal` members (visible only via existing `InternalsVisibleTo`): `IsRegistered()`, `ProcessTick(now)` — extracted from `OnTick()` with no behaviour change — and `ResetForTests()`. Red/green verified. Without the `MoveToWorld` fix, 5 of 6 of the original tests fail, including `ItemOnGround_ActuallyDecaysAfterDecayTime`, which shows a ground item never decays even after a full simulated hour. Without the chest fix, 8 of 8 chest tests fail. Without the refused-decay fix, that test fails. Server.Tests 737/737 and UOContent.Tests 509/509 pass. |
||
|
Kamron Batman
|
7470cfd43c
|
fix: Bumps dependencies. (#2531) | ||
|
Kamron Batman
|
8b9bab20fd
|
fix(network): restore huffman code for symbol 0x19 (#2528)
## The bug #2522 rewrote the outgoing huffman table in `NetworkCompression.cs` and transposed symbol `0x19`'s code from `0x1CE` to `0x12E` (both 9 bits, so the length distribution — and the Kraft sum — stayed valid, which is why nothing obvious tripped). The real damage is that it broke prefix-freeness. `0x12E` is `100101110`, and symbol `0x0D`'s 8-bit code is `10010111` — a proper prefix of it. The client's decoder walks the tree bit by bit, so it hit a valid leaf at `0x0D` after 8 bits, emitted the wrong byte, and then reframed every subsequent code. That is exactly what the reporter's capture shows. Server sends `BF 00 0C 00 19 02 00 00 00 01 00 00`; the client's post-decompression stream reads `BF 00 0C 00 0D 55 00 00 01 00 00` — the literal `0D` is the mis-decoded `0x19`, and the packet is now one byte short, so framing desyncs from there on. ## Impact Any outgoing packet with byte `0x19` anywhere in its body (serials, coordinates, hues, lengths, text) corrupted the stream. Because the desync is in framing rather than a single field, the client silently stops applying server updates while still being able to send — no disconnect, no error. `StatLockInfo` (`0xBF` subcommand `0x19`) is sent during login, so it reproduces on essentially every connection. This is also #2526: "can only walk a few steps, then the client stops responding" is the same desync, not a VPS sizing problem. ## Fix One entry, restored to the canonical value: ```diff - 0x9, 0x191, 0x9, 0x12E, 0x7, 0x03F, ... + 0x9, 0x191, 0x9, 0x1CE, 0x7, 0x03F, ... ``` ## Validation of the whole table Rather than eyeball 257 entries, I diffed the current table against **every revision of it in this repo's history** — all 34, back through the renames to the original import. All 34 agree with each other, and `0x19` is the sole disagreement with #2522's rewrite. No other entry has ever changed. I also validated the table structurally: all 257 lengths in `[2,11]`, every value fits its declared bit-length, Kraft–McMillan sum exactly 1, and no code is a prefix of any other. It passes on all counts now, and the prefix check is what located the bug in the first place. Both checks were one-off validation scripts, not committed — see below. ## Test A single known-answer test (`~10ms`) that compresses all 256 symbols and asserts the exact output bytes. The expected bytes were generated from the canonical table, *not* from the implementation, so the test isn't circular. Any single wrong table entry changes the output, so it pins all 256 entries plus the terminal code, and it exercises the encoder end to end. A round-trip test would **not** catch this class of bug — encoder and decoder built from the same table agree with each other even when the table is wrong. The contract being violated is with the client's hard-coded tree, so the expected bytes have to come from outside the implementation. The structural prefix-free check and a second `StatLockInfo` vector were deliberately dropped after they'd served their purpose: the table is now verified and effectively frozen, so the structural check was guarding a constant, and the `StatLockInfo` vector is a strict subset of the all-symbols one. What remains covers the risk that's still live — `Compress` is a hand-unrolled bit-packing loop that will get optimized again, and this is the guard against that rewrite silently corrupting the wire format, which is precisely what happened here. Verified the test fails when the bug is reintroduced and passes when fixed. Full `Server.Tests` suite green: 727 passed. |
||
|
Kamron Batman
|
e035768ef8
|
fix: Optimizes outgoing packet encoding. (#2522)
### Summary * Fixes a regression in ModernUO huffman encoding compared to RunUO & ServUO. * Optimizes the encoding by 1.6x-2x using. ### Benchmarks ```cs | Method | Categories | Mean | Error | StdDev | Ratio | |----------------------- |----------- |---------:|--------:|--------:|------:| | BenchmarkSUOAcctPacket | AcctPacket | 757.1 ns | 3.83 ns | 3.58 ns | 1.00 | | BenchmarkMUOAcctPacket | AcctPacket | 941.4 ns | 3.27 ns | 2.73 ns | 1.24 | | BenchmarkOptAcctPacket | AcctPacket | 530.7 ns | 1.24 ns | 1.10 ns | 0.70 | | | | | | | | | BenchmarkSUOGump | GumpPacket | 526.8 ns | 2.10 ns | 1.86 ns | 1.00 | | BenchmarkMUOGump | GumpPacket | 657.7 ns | 1.25 ns | 1.11 ns | 1.25 | | BenchmarkOptGump | GumpPacket | 285.0 ns | 0.77 ns | 0.68 ns | 0.54 | ``` |
||
|
Kamron Batman
|
d7668df5ee
|
feat(opl): OplTextBlock multi-line tooltip builder + AddChunked (#2507)
## At a glance
```csharp
public override void GetProperties(IPropertyList list)
{
base.GetProperties(list);
// Accumulate any number of free-text lines. On dispose the block flushes via
// AddChunked, splitting across as many OPL properties as needed so none can
// overflow the legacy 2D client's per-property buffer (which would crash it).
using var block = list.TextBlock();
if (luck > 0)
{
block.Add($"Luck Bonus: +{luck}%"); // zero-alloc interpolation
}
block.Add("Cannot be repaired".AsSpan()); // plain text, no string allocation
// Already holding a '\n'-joined string? Skip the builder and chunk directly:
// list.AddChunked(description);
}
```
## What
Adds a safe path for emitting **variable-length, free-form (non-cliloc) tooltip text**:
- **`ObjectPropertyList.Add(ReadOnlySpan<char>)`** overloads — append raw text with no string allocation. Makes the old single-arg `Add(string)` redundant (a `string` binds to the span overload implicitly), so it's dropped.
- **`AddChunked(ReadOnlySpan<char>)`** on `IPropertyList` — splits newline-joined text at `\n` boundaries across as many passthrough-cliloc properties as needed, so no single property exceeds the cap.
- **`OplTextBlock`** (`ref struct`) + **`IPropertyList.TextBlock()`** — an ergonomic builder that accumulates `\n`-joined lines (with a zero-alloc interpolated `Add($"...")` overload) and flushes via `AddChunked` on dispose. Usage: `using var block = list.TextBlock();`.
- **`MaxArgumentLength` (504)** — per-property cap with a hard backstop that clamps + logs anything that slips through.
## Why
The legacy 2D client copies each OPL property's text into a fixed ~512-char (1024-byte) buffer. A single property longer than that smashes an adjacent world object's vtable on the client heap and crashes the client. `AddChunked`/`OplTextBlock` keep multi-line content safely under the cap instead of risking one oversized `Add`.
## Docs
- `dev-docs/property-lists.md` — new "Multi-Line Free Text" deep-dive section; corrected the stale `IPropertyList` listing.
- `dev-docs/claude-skills/modernuo-property-lists.md` — condensed pattern + anti-pattern.
## Tests
9 tests pass (`OplTextBlockTests`, `ObjectPropertyListSpanAddTests`): line joining, empty-line skipping, no-line no-op, zero-alloc interpolation, and long-content chunking staying under `MaxArgumentLength`. Full `UOContent` build is green, confirming dropping `Add(string)` breaks no call sites.
|
||
|
Kamron Batman
|
a038655541
|
fix: Bumps dependencies. Adds Server 2012/2016 support. (#2509)
### Summary * Bumps dependencies * Bumps IORingGroup to add epoll support and backward compatibility for Server 2012/2016. Closes #2508 |
||
|
Kamron Batman
|
a28a32f46d
|
fix(json): Rectangle3DConverter loses a z-level on write (#2506)
## Summary
`Rectangle3DConverter.Write` corrupts a rectangle's Z range for certain bounds. The omit-z guard was:
```csharp
var writeZ = value.Start.Z is > sbyte.MinValue and < sbyte.MaxValue
|| value.End.Z is > sbyte.MinValue and < sbyte.MaxValue;
```
This drops `z1`/`z2` whenever **both** Z bounds sit at/outside the sbyte extremes — but `Read` reconstructs absent Z as exactly `z1 = -128, z2 = 127` (depth 255). So any rectangle that trips the omit condition without *being* that sentinel round-trips to depth 255 and is corrupted.
The concrete case: a **homeRange-style** bound `Start.Z = -128, End.Z = 128` (depth 256, the full vertical range used by spawners) gets `z1`/`z2` omitted on write, then reads back as depth **255** — silently losing the top z-level on every re-serialize.
(Spotted while working on #2505; spawners now prefer the `homeRange` form so most square bounds avoid this path, but any non-square `spawnBounds` or other `Rectangle3D` JSON is affected.)
## Fix
Omit Z only for the exact sentinel `Read` produces (`z1 == -128 && z2 == 127`); write it for anything else:
```csharp
var writeZ = value.Start.Z != sbyte.MinValue || value.End.Z != sbyte.MaxValue;
```
`Read` is unchanged, so existing `regions.json` rectangles that omit Z keep loading identically.
## Tests
New `Rectangle3DConverterTests`: round-trips the homeRange depth-256 case, the depth-255 sentinel, and ordinary/edge z values; asserts the sentinel omits Z while homeRange bounds write it. Server.Tests 717/717, UOContent.Tests 487/487.
|
||
|
Kamron Batman
|
d8a64f3316
|
refactor(spawners): replace DynamicJson with typed SpawnerDto records (#2505)
## Summary Removes the dated `DynamicJson` JSON helper and migrates spawner JSON (de)serialization to a polymorphic `record SpawnerDto` hierarchy. `DynamicJson` was the last remaining consumer (regions moved off it in #1400). The key correctness improvement: **System.Text.Json deserializes plain DTO records, never a live `Item`.** Previously, deserializing directly into an `Item` meant STJ constructed world-registered objects *before* the data was validated — a malformed/hand-edited spawn file could leave orphaned spawner Items in the world save. Now a parse failure is GC-only; `dto.ToSpawner()` constructs the spawner only from a fully-validated DTO and self-cleans on failure. ## What changed - **New:** `SpawnerDto` (abstract) + `SpawnerDataDto` / `RegionSpawnerDto` / `ProximitySpawnerDto`, each marked with a reusable `[JsonDiscoverableType]` opt-in attribute. Auto-discovered at the `Configure` phase — no manual registration list (avoids the regions `Register<T>()` footgun), open to custom spawner subtypes. - **Symmetric mapping:** `BaseSpawner.ToDto()` (export) ⇄ `SpawnerDto.ToSpawner()` (import). `ToSpawner()` deletes-and-rethrows on any failure, so the importer can never orphan an Item. - **`SpawnerJsonSerializer`** wires `$type` polymorphism on the `SpawnerDto` root with loud collision/constructibility validation. - **Import/export commands** rewired to the typed DTO path (reflection `FindTypeByName`/`CreateInstance` removed). - **Data migration:** the 109 `Distribution/Data/Spawns/**` files moved from `"type"`→`"$type"` and legacy `homeRange`→`spawnBounds`. The runtime still *reads* legacy `homeRange` for external files. The `homeRange→spawnBounds` formula is proven equivalent to the runtime conversion (`BoundsEquivalenceTests`, hr=0/1/3/7). - **Deleted:** `Projects/Server/Json/DynamicJson.cs`. Sparse export output matches the legacy `ToJson` (nullable DTO properties + `WhenWritingNull`). Binary world-save serialization is untouched. ## Tests - DTO round-trip per spawner type; sparse-default omission; legacy `homeRange` read; export/import file round-trip. - `Import_MalformedFile_LeaksNoWorldItems` — proves a mid-array parse failure constructs zero world Items. - `AllSpawnFilesLoadTests` — every migrated spawn file deserializes and builds. - Duplicate-discriminator validation. - UOContent.Tests 485/485, Server.Tests 710/710, build clean. ## Follow-up (not in this PR) `Rectangle3DConverter.Write` (in `Projects/Server/`) omits `z1/z2` when `Start.Z == -128`, so a future server-side export of a `homeRange`-style spawner round-trips depth 256→255. Pre-existing and out of scope here (Server change); the migrated data reads correctly. Worth a separate small converter PR. |
||
|
Kamron Batman
|
73d1ee3874
|
fix(network): don't leak duped-layer equipment via EquipUpdate/OPL (#2502)
## Problem A creature equipped with two items that resolve to the **same layer** can crash the legacy EA 2D client (use-after-free). Equipment `Layer` comes from tiledata (`Layer = (Layer)ItemData.Quality`), so a **two-handed weapon and a shield both resolve to `Layer.TwoHanded`**. `Mobile.FindItemOnLayer` even documents the invariant: *"We only allow 1 item per layer. Its an implicit contract."* ## Root cause - `SendMobileIncoming` (0x78) **dedupes by layer** (the `layers` span) and sends only the first item per slot — so a static creature is fine. - But the per-item **`SendEquipUpdate` (0x2E)** and the item **OPL** sends do **not** dedupe. On any equip/property delta (`Item.ProcessDelta`) they fire per item and leak the second same-layer item on its own. - The client then holds two items on one equipment slot; when that slot is torn down (e.g. a large group of such creatures and the player runs out of range → mass remove) the legacy 2D client frees one and dereferences it → UAF. ClassicUO bounds-checks and is unaffected, but the server is emitting an invalid, self-contradictory stream either way. ## Fix Make per-item equip/OPL sends honor the same first-item-per-layer rule `SendMobileIncoming` already uses: - `Item.IsDupedEquipLayer()` — `m_Parent is Mobile m && m.FindItemOnLayer(m_Layer) != this` (reuses the existing helper; true when an earlier item already holds this items layer). - `Item.ProcessDelta`: early-return before the per-client loop for a duped equipped item (skips the EquipUpdate and OPL to everyone). - `Mobile` lift-reject re-show: skip the EquipUpdate and OPL for the dupe. No behavioral change for valid equipment (distinct layers → never duped). For the invalid duped-layer case the second item was already omitted by the 0x78 packet; this just stops it leaking back via the per-item paths. |
||
|
Kamron Batman
|
1c2e114b21
|
feat(pathfinding): multi-aware mask synthesizer + warm interior cache for house/boat cells (#2479)
## Problem Houses and boats (multis) were pathed correctly only by **delegation to the slow path**: `StepCache.TryGetMask` returns `Fallthrough_Multi` for any multi-covered cell, and `GetSuccessors` ran `CheckMovement` **8× per cell** (each re-resolving the tile stack via `GetStaticAndMultiTiles`) — a sustained per-step cost near every house/boat. There was also no automated test pinning multi pathfinding. This branch is the full multi-pathfinding effort in phases on one branch. ## Phase 1 — characterization tests (the oracle) Implementation-agnostic invariants: a cache-on≡cache-off whole-path invariant, a per-cell sweep vs `CheckMovement` over footprint+halo (incl. destination Z), hand-verified routing (around walls, demolish-reopens, foundation-redesign-honored), classic-house / foundation / boat fixtures, non-vacuity guards. These gate every later phase byte-for-byte. ## Phase 2 — live single-pass synthesizer `StepProbe.ComputeMultiMaskAt` synthesizes a covered cell's full 8-direction `StepMask` in one pass (the existing surface/step logic over `GetStaticAndMultiTiles` instead of 8× `CheckMovement`). `GetSuccessors` routes `Fallthrough_Multi` cells through it. No new cache, no `.swb` change. **~1.5×**, zero added allocations. ## Phase 3 / 3.1 — warm per-`multiID` interior cache (airtight) `MultiMaskCache` caches each fixed multi's local-frame `StepMask` for **interior** cells (cell + all 8 neighbours covered → terrain-neighbour-free → position-invariant), keyed by `multiID & 0x3FFF`, built lazily from the MCL. Interior cells become ~20 ns lookups. The cache is gated on a **per-instance footprint-clean flag** (`BaseMulti.PathInteriorCacheState`): an instance whose whole footprint terrain is below its floor (`maxTerrain < minFloor`) serves from the cache; a **dirty** instance (terrain intrudes — a contrived/GM placement) **degrades to live-synth, never a wrong mask**. This closes a cross-instance soundness gap (the cached mask depends on neighbour terrain too) found in a holistic review. The gate resets whenever the footprint's world-terrain relationship can change — **location, map, or ItemID** (a boat's heading swaps the MCL). **Boats are cached too.** Their per-`multiID` deck masks are movement-invariant (built once per heading), so a sailing boat never rebuilds them; only the cheap clean-flag rescan repeats per move (and only when pathed near). Narrow existing boats have little interior; wide galleons (`multi.mul`) would gain Castle-class. `HouseFoundation` (per-instance runtime `DesignState`) is the one type that stays on the live path. ## Verification - `UOContent.Tests` **454/454**, `Server.Tests` **708/708**, 0 failures. - The Phase-1 oracle (`MultiPathInvariantTests`, cache-on ≡ cache-off) stays **byte-identical** with the synthesizer + interior cache active. - Tests pin: footprint-cleanliness (clean vs sunk), dirty/cluttered placement degrades to live-synth while still pathing, clean placement serves, and the gate resets on move/ItemID change. ## Performance (modernuo/ModernUO-Benchmarks#8, full-fixture) Houses at **Green Acres** (flat staff region → clean footprints, the legit-placement case): | Route | Slow path | Phase 3.1 (interior cache) | Speedup | |-------|----------:|---------------------------:|--------:| | `around_a` (29 steps) | 238.3 µs | **49.1 µs** | **4.85×** | | `around_b` (29 steps) | 224.3 µs | **49.5 µs** | **4.53×** | ~130 of ~167 multi cells/route serve from the cache (~20 ns) vs 37 live-synth. Per-cell, the slow path's 8× `CheckMovement` grows with multi complexity (GuildHouse ~857 ns → Castle ~1,194 ns), the synthesizer is a flat ~780 ns, and the cache serve is ~20 ns — so big/tall multis (and wide galleons) gain most. Identical allocations throughout. |
||
|
Marcelo Paez Sequeira
|
92a540b2d0
|
feat(server): add Mobile.SayTo localized overload with explicit hue (#2481)
## Summary
`Mobile.SayTo(Mobile to, int number, string args = "")` always sends the localized message using `SpeechHue`. This adds a parallel overload that accepts an explicit `hue`:
```csharp
public void SayTo(Mobile to, int number, int hue, string args = "") =>
to.NetState.SendMessageLocalized(Serial, Body, MessageType.Regular, hue, 3, number, Name, args);
```
It mirrors the existing localized overload exactly, only substituting the caller-provided `hue` for `SpeechHue`, so content can send a cliloc message to a single mobile in a chosen color without dropping down to `NetState.SendMessageLocalized` directly. This restores the hued-cliloc `SayTo` that RunUO/ServUO content commonly relied on (e.g. `SayTo(from, 1042205, 0x3B2)`).
## Notes
- Purely additive; no behavior change to existing call sites.
- No overload ambiguity: `SayTo(m, num)` and `SayTo(m, num, "args")` still bind to the existing overload; `SayTo(m, num, hue)` binds to the new one (the third positional arg is `int` vs `string`).
- Null-safe to the same degree as the existing overload (`SendMessageLocalized` guards via `CannotSendPackets()`).
## Test Plan
- [x] `dotnet build Projects/Server` — 0 warnings, 0 errors.
- One-line additive overload mirroring an existing (untested) method; no existing `Mobile.SayTo` unit tests to extend. Happy to add coverage if preferred.
|
||
|
Kamron Batman
|
9d26a44a28
|
fix: Fixes pathfinding prebake and pathfinding multi-fallthrough. (#2478)
## Summary Fixes the pathfinding step-cache (`.swb`) prebake so it bakes **once** and skips when a valid cache already exists, instead of re-baking on every boot. The root cause was the staleness fingerprint hashing mutable in-memory tile data rather than the on-disk files. This PR makes the fingerprint a pure function of the client data files and separates dynamic multis (houses/boats) from the static cache. > This branch builds on the `ConfigurePrompts` first-boot-prompt unification (commit `5df8d0bd`, also included here) — that commit accounts for the `ServerConfiguration.cs` and `dev-docs/server-lifecycle.md` changes in the diff. ## The bug With `pathfinding.prebakeMaps` set, the cache re-baked on **every** boot. The `.swb` staleness fingerprint hashed the live `TileData.LandTable`/`ItemTable` flags, which the server patches at runtime (`ItemFixes`, `LOSBlocker`, `PotionKeg`, `CTF`) at nondeterministic lifecycle points (Initialize-phase methods share a priority; static ctors fire lazily). So a fingerprint stamped at runtime (`[PathBake`) never matched the one recomputed during startup `Initialize()`, and the cache rebaked every time. ## Changes **1. Fingerprint the files, not the in-memory tables** (`fix`) Hash `tiledata.mul` (cached, computed once) plus the per-map `.mul`/`.uop` files — never the runtime-mutated `TileData` tables. The fingerprint is now lifecycle-stable. Existing `.swb` files rebake once after deploy, then stay stable. **2. Compute the fingerprint once per boot** (`refactor`) `Configure()`'s `AutoLoadAtStartup()` already opens and fingerprint-validates a reader for every up-to-date `.swb`. `Initialize()` now skips baking any map that already has an open reader (`StepCache.HasLazyReader`) instead of recomputing the fingerprint a second time. **3. Bake static-only; route multis to the live path** (`refactor`) Multis (houses/boats) are dynamic, so they're no longer baked into the static chunk cache — they were tagged with `BuiltMultisVersion`, a non-persisted session counter, which made persisting them unsafe (false matches / wasted re-bakes). - Chunks bake land + `statics.mul` only. - At query time, any cell whose sector (or its 1-cell halo) contains a multi routes to `Fallthrough_Multi` → the existing live, multi-aware `CheckMovement` path. The halo prevents a cell proposing a walkable edge into a neighbouring wall; interior (multi-free) cells pay one sector lookup. - Adds `Sector.HasMultis` (one engine accessor); `.swb` format → v9 (rejects old multi-baked files); new `Fallthrough_Multi` telemetry. - Behaviour-preserving: multis use the same live path the engine used before the cache existed. **4. Comment polish** (`style`) — no behaviour change. ## Testing All green: **92** pathfinding (incl. a new fingerprint-stability test and a multi-halo fallthrough test), **423** UOContent, **708** Server. ## Follow-ups (not in this PR) - **Background bake worker** — make `[PathBake` and the boot prebake non-blocking (game thread serves tile reads to an off-thread worker). - **Per-multi MCL cache** — cache walkability in each multi's own frame (keyed by multiID, movement-invariant) so houses/boats get a fast path instead of the live fallback. - **House-pathfinding equivalence tests** — the one area not yet covered by a dedicated automated test; multi pathing is currently correct by delegation to the live path. |
||
|
Kamron Batman
|
eec37edd67
|
refactor(server): unify first-boot prompts into the ConfigurePrompts phase (#2477)
Stacked on #2475 (the `ConfigurePrompts` phase). Base will switch to `main` once #2475 merges. ## What Move the engine's own first-boot prompts — data directories, listeners, server name, expansion + map selection — out of `ServerConfiguration.Load` and into **`ServerConfiguration.ConfigurePrompts()`** (`[CallPriority(0)]`), so **all** first-boot prompting (engine and content) runs through the single `AssemblyHandler.Invoke("ConfigurePrompts")` phase. `Load` now only reads/creates the config file. ## Why it's safe - **Assembly loading uses `AssemblyDirectories` (default `./Assemblies`), not `DataDirectories`** — so assemblies load fine before the now-later data-dir prompt. This is the linchpin that makes the move possible. - **`UOClient.Load()`** (client-file discovery via `Core.FindDataFile`) needs `DataDirectories`, so it moved *with* the data-dir prompt into `ConfigurePrompts`. - **`Core.Expansion`** is now assigned in `ConfigurePrompts` (every non-mocked boot). Nothing between `LoadAssemblies` and that phase reads it — type initializers run lazily on first use, not during `LoadAssemblies`. - **`[CallPriority(0)]`** keeps the engine prompts (including map selection) ahead of content prompts such as the pathfinding pre-bake (priority 50), preserving "after map selection". - `Main.cs` already invokes the phase — **no startup-ordering edit** here. ## Tests `Server.Tests` **708/708**, `UOContent.Tests` **418/418**, build clean. Fixtures are unaffected: they call `Load(true)` (now just reads config) and set expansion/data dirs directly; `ConfigurePrompts` is gated on `m_Mocked`. ## ⚠️ Needs first-boot runtime verification `Main.cs` startup ordering is **not** covered by the fixture-based suite (the fixtures bypass `Main`). Please boot once with a fresh `modernuo.json` to confirm the first-boot prompt sequence (data dirs → … → expansion/maps → pathfinding pre-bake) and that `Core.Expansion` resolves correctly. Docs updated in `dev-docs/server-lifecycle.md`. |
||
|
Kamron Batman
|
16bf3016fb
|
feat: Pre-Publish 14 Crafting (supersedes #2181, #2381) (#2476)
## Summary
Adds authentic **T2A-era (pre-UO:Third-Dawn) packet-based crafting menus**, enabled via the **`t2aCraftMenus` server setting** (read once at startup; default **`!Core.UOTD`**, so a pre-UO:TD shard gets them automatically). When enabled, double-clicking a crafting tool opens the classic `0x7C`/`0x7D` item-list menu — skill- and material-filtered — instead of the modern gump, covering all 8 tool/skill crafts (blacksmithy, tailoring, tinkering, carpentry, alchemy, bowcraft/fletching, inscription, cartography). It is **not** a runtime/admin-flippable feature flag.
This is the **definitive, reconciled** branch and **supersedes**:
- **#2181** (Delphi — `T2A_CraftingMenus`): the original effort.
- **#2381** (Jack/UOLL — `t2a_crafting_menus`): the research-grounded superset (Delphi's base + 12 corrections), rebased onto current `main`.
Original authorship is preserved across the cherry-picked history: foundation commit **@Delphi79**, mechanic fixes **@jackuoll (Jack Ward)**, reconciliation/fixes/docs mine.
## How it was built
1. Cherry-picked Jack's 13 commits onto current `main` (superset of Delphi's; only 2 trivial FeatureFlags conflicts).
2. Applied targeted fixes (below) with tests.
3. Full convention audit, build, and test pass.
Grounded in independent historical research plus Jack's deep dive. Maintainer reference: `dev-docs/t2a-crafting.md`.
## Mechanics (highlights)
- Double-click tool → target resource → skill/material-filtered menu → craft. Resource pre-selection per skill; make-last by targeting the tool.
- **Stacked-gem jewelry:** target a gem stack → the **full stack** is consumed and the piece is named by count ("a 1000 diamond ring"); count persists (`BaseJewel` serialization **v4 → v5**, new `_gemCount`).
- **Tool-less inscription & cartography** (skill-list invoked; no pen/sextant); inscription consumes reagents+scroll on success and failure, mana only on success.
- **Tailoring matching-hue consumption:** targeting hued cloth/leather consumes only that hue. Crafted items take color from their **`CraftResource`** (not the dyed hue), so dyed leather/cloth don't tint the product; in T2A only colored ingots/ore color items (metal armor/shields).
- **Half-resources on failed non-scroll crafts** (pre-UO:TD).
- **Maker's mark** always prompted for exceptional items, via the shared `QueryMakersMarkGump`.
- Server-side menu infra changes are additive (`ItemListEntry.CraftIndex`, `Entries` setter, `HasSent`).
## Notable changes on top of the cherry-pick
- **Toggle is a startup server setting, not a feature flag.** Removed `ContentFeatureFlags.T2ACraftMenus` (and its admin-flippable plumbing); the value is read once via `ServerConfiguration.GetSetting("t2aCraftMenus", !Core.UOTD)` into `T2ACraftSystem.Enabled`. Since the default tracks the era and it can't be flipped at runtime, there's no incoherent "menus-on / UO:TD-era" state.
- **Stacked-gem consumption (B3a/B3):** consume the full `PendingGemCount` (was deliberately consuming 1 while naming by the stack), null-safe gem type, plain-piece fallback + message. New `T2AJewelGemCraftTests`.
- **Convention audit:** `new List<Item>()` → `PooledRefList<Item>` on the hue-aware consume path; removed dead code.
## Decisions & deviations
- `make-last` kept as **QoL** (post-T2A gump-era feature).
- `half-on-failure` (non-scroll) kept as a **reconstruction** (not OSI-confirmed).
- **Stacked-gem** behavior set per shard authority (overrides the "single gem" reconstruction).
- **Cooking** out of scope (no T2A crafting menu existed for it).
- **No colored items from dyed materials:** crafted color comes from the `CraftResource` type. Pre-AOS leather has no colored variant, so leather is always uncolored; weapons retain resource color only in AOS+ (unchanged, intended).
## Test plan
- Automated: `dotnet build ModernUO.slnx -c Debug` clean; `dotnet test Projects/UOContent.Tests` → **421 passed** (incl. 3 new jewelry tests).
- Manual (needs a running T2A shard + client):
- [ ] Each of the 8 skills opens the correct menu; empty-menu guard fires.
- [ ] Make-last repeats the last craft (jewelry re-prompts gem).
- [ ] Jewelry consumes the full targeted gem stack and names by count.
- [ ] Cartography consumes blank maps only with T2A enabled / maps+scrolls when disabled.
- [ ] Tailoring consumes only the targeted-hue material; crafted items are not tinted by dyed cloth/leather.
- [ ] Maker's-mark prompt on exceptional.
- [ ] Failed non-scroll craft consumes half resources.
- [ ] Inscription: reagents+scroll on success/failure, mana only on success.
- [ ] T2A disabled: gump crafting unchanged.
## Credits
Co-authored-by: @Delphi79
Co-authored-by: @jackuoll
|