warrentyexpired/ModernUO
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions
ModernUO/CLAUDE.md
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.
2026-08-09 00:13:34 -07:00

11 KiB
Raw Blame History

ModernUO

.NET 10 Ultima Online server emulator. Single-threaded game loop. All game logic runs on one thread.

  • Server engine: Projects/Server/ — do NOT modify without explicit request
  • Game content: Projects/UOContent/ — primary editing target
  • Build: dotnet build from repo root

Code Audit Rules

Apply these when writing or reviewing .cs files under Projects/.

  1. LINQ — Tier 1 (zero-cost patterns) free on hot paths; Tier 2 (low overhead) OK on warm paths; Tier 3 (allocating) forbidden on hot paths → dev-docs/code-standards.md
  2. No Console.WriteLine — use LogFactory.GetLogger(typeof(MyClass)) → logger.Information(...) (requires using Server.Logging;)
  3. Threading policy — game logic runs only on the main loop; never touch game state (World, mobiles, items, maps, timers) from a background thread. Heavy work that needs game state must be chunked across ticks, not threaded. Heavy work that does not need game state (large-file parse, external I/O) must run on a background thread and must yield to world saves (defer while World.Saving/WorldState.PendingSave). Publish results back to the loop as an immutable snapshot swapped via a single volatile reference — the only sanctioned volatile. No lock/Mutex/ConcurrentDictionary in game logic. Rule #10 covers how background work hands results back to the loop → dev-docs/threading-model.md
  4. No World.Mobiles/World.Items iteration — use spatial queries: map.GetMobilesInRange<T>(), map.GetItemsInRange<T>()
  5. Clean up refs in OnDelete()/OnAfterDelete() — null out Item/Mobile references
  6. Cancel timers in OnDelete()/OnAfterDelete() — call _token.Cancel() or _timer?.Stop()
  7. STArrayPool<T>.Shared not ArrayPool<T>.Shared — single-threaded optimized, no locks
  8. PooledRefList<T> not new List<T>() on hot paths — zero GC pressure, stack-allocated ref struct
  9. Serialization — class must be partial, constructor needs [Constructible], TimerExecutionToken must NOT have [SerializableField]. New classes: use [SerializationGenerator(version)] (omit encoded). When bumping versions, add MigrateFrom(VXContent) (X = previous version). Never modify Deserialize(reader, version) for version bumps — that method is only for pre-codegen legacy saves. When migrating from pre-codegen Serialize/Deserialize: pass false if old code used reader.ReadInt(), bump version +1, and keep old logic as private void Deserialize(IGenericReader reader, int version) → dev-docs/runuo-migration-docs/02-serialization.md
  10. No Task.Run/new Thread() for game logic (tandem with rule #3) — game logic is the single-threaded event loop. Backgrounding is allowed only for work that does not itself touch game state (external service calls, large-file parse). Prove the need before adding a thread: measure on-loop time, not wall-clock (frozen world is the cost, player latency is not), and gate on Environment.ProcessorCount — off-loading creates no CPU and buys nothing on 1–2 cores. New workers go in the vetted table in dev-docs/threading-model.md with their measurement. When such work must feed game logic: run the heavy/I/O part off-loop and ConfigureAwait(false) its awaits so a continuation never resumes on the loop and silently foregrounds heavy work; then hand the result back explicitly — publish an immutable snapshot swapped via a volatile reference (the loop reads it lock-free), or marshal the apply step with Core.LoopContext.Post(() => …), re-validating in the continuation whatever may have changed while it ran. Never touch game state off-thread; never let the scheduler decide where the heavy work runs → dev-docs/threading-model.md
  11. Never assume era — if code uses Core.AOS/Core.SE/etc., ask which expansion to target
  12. Naming — _camelCase private fields, PascalCase properties/methods/classes; don't flag legacy m_ but use _ for new code
  13. No empty gumps — every gump must produce visual elements. An empty gump leaks on client+server (no way to close it). Use static DisplayTo() to validate before constructing → dev-docs/gump-system.md
  14. PropertyList string literals must be holes — $"{"Map"}\t{value}" not $"Map\t{value}". The handler treats bare text as delimiters, {} holes as arguments. Only \t should be a bare literal → dev-docs/property-lists.md
  15. Braces required on all control flow — if, else, for, foreach, while, do, switch must always have braces, even for single-line bodies → dev-docs/code-standards.md
  16. Prefer switch expressions and switch-when — use switch expressions for value mapping and switch-when for pattern matching where they improve readability. Exception: skip if unreadable or cold path → dev-docs/code-standards.md
  17. No System.Text.StringBuilder — use ValueStringBuilder with stackalloc (bounded output) or ValueStringBuilder.Create() (unbounded). Supports $"..." interpolation directly. Always use using var for disposal. Use Reset() instead of reassigning → dev-docs/string-handling.md
  18. Interpolation anti-patterns on handler-aware APIs — Send*/Say/Emote/PublicOverhead*/IPropertyList.Add/gump AddLabel/AddHtml/Html.Center/SpanWriter.Write* all have ref RawInterpolatedStringHandler overloads that allocate zero strings, but only when the call-site argument is a $"..." literal directly. Avoid: ternaries with interpolated branches (Send(c ? $"a" : $"b")), switch expressions with interpolated arms, pre-built var s = $"..." locals (single-use), .ToString() / .String() / string.Format inside holes, string concat ({a + b}), LINQ string ops in holes. Use :L format spec for lowercase ({rank:L} not rank.ToString().ToLowerInvariant()) → dev-docs/string-handling.md § Interpolation Anti-Patterns
  19. No InvalidateProperties() from inside GetProperties — every property a GetProperties override reads must be a pure read. InvalidateProperties() rebuilds the list in place (Reset() + rebuild), and Reset() returns the pooled interpolation buffer — which the compiler rents for the whole $"..." expression, so every hole is evaluated while it is live — and rewinds the packet cursor. A getter that invalidates therefore throws ArgumentNullException (parameter "array") out of GetProperties from an unrelated-looking line, or silently corrupts the tooltip. The engine refuses and logs an error; DEBUG throws. Lazy recomputation in a getter is fine — the notification is not. Invalidate in the setter that changes the value, or defer with Timer.DelayCall(InvalidateProperties) → dev-docs/property-lists.md § Never Invalidate From Inside GetProperties

Dev-Docs Reference

Topic File
Code standards & LINQ tiers dev-docs/code-standards.md
Serialization system dev-docs/serialization.md
Content patterns (Items, Mobiles, Creatures) dev-docs/content-patterns.md
Era & expansion handling dev-docs/era-expansion.md
Timer system dev-docs/timers.md
Event scheduler (wall-clock/calendar) dev-docs/event-scheduler.md
Object property lists (tooltips) dev-docs/property-lists.md
Gump (UI dialog) system dev-docs/gump-system.md
Commands & targeting dev-docs/commands-targeting.md
Event system dev-docs/events.md
Threading model dev-docs/threading-model.md
Server lifecycle & bootstrap phases (Configure/ConfigurePrompts/Initialize) dev-docs/server-lifecycle.md
Platform prerequisites (ICU, tzdata, native libs per distro) dev-docs/platform-prerequisites.md
Configuration system dev-docs/configuration.md
Networking & packets dev-docs/networking-packets.md
IP bans, blocklists & allowlists (incl. unblocking a player) dev-docs/ip-bans-and-allowlists.md
Region system dev-docs/regions.md
String handling & ValueStringBuilder dev-docs/string-handling.md
RunUO migration (overview) dev-docs/runuo-migration-docs/00-overview.md
RunUO migration (all docs) dev-docs/runuo-migration-docs/

Claude Skills (Opt-In)

Detailed Claude Code skills live in dev-docs/claude-skills/. They are not auto-loaded — they must be copied to .claude/skills/ to activate.

When to offer: If the user is building complex content (new items, creatures, spells, gumps, quests, packets, serialization work, etc.), ask:

I have detailed Claude Code skills for this kind of work. Want me to enable them? I'll copy the relevant files from dev-docs/claude-skills/ to .claude/skills/.

Then copy only the relevant skill files based on the task:

Task Skills to enable
New Item or Mobile modernuo-content-patterns, modernuo-serialization, modernuo-property-lists
Creature / spawn modernuo-content-patterns, modernuo-serialization, modernuo-timers
Spell or ability modernuo-content-patterns, modernuo-serialization, modernuo-timers, modernuo-era-expansion
Gump / UI dialog modernuo-gump-system, modernuo-commands-targeting
Quest or event system modernuo-events, modernuo-content-patterns, modernuo-configuration
Scheduled / seasonal / holiday events modernuo-event-scheduler, modernuo-timers
Custom regions / dynamic areas modernuo-regions, modernuo-content-patterns
Packet / networking modernuo-networking, modernuo-threading
Commands modernuo-commands-targeting
Timer work modernuo-timers, modernuo-serialization
Config system modernuo-configuration
Era-conditional code modernuo-era-expansion
String building / formatting modernuo-string-handling
Code review / audit modernuo-code-audit
Any .cs file edit modernuo-code-audit (always offer for code changes)
RunUO Migration
Migrate any RunUO script migrate-from-runuo/migrate-foundation (always), plus system-specific skills below
Migrate Item/Mobile/Creature migrate-from-runuo/migrate-foundation, migrate-from-runuo/migrate-serialization, migrate-from-runuo/migrate-items-mobiles
Migrate serialization migrate-from-runuo/migrate-serialization
Migrate timers migrate-from-runuo/migrate-timers
Migrate gumps migrate-from-runuo/migrate-gumps
Migrate packets migrate-from-runuo/migrate-packets
Migrate property lists migrate-from-runuo/migrate-property-lists
Migrate events/commands migrate-from-runuo/migrate-commands-events
Migrate persistence (WorldSave) migrate-from-runuo/migrate-persistence
Migrate multi-file system migrate-from-runuo/migrate-systems

To enable a skill: cp dev-docs/claude-skills/<name>.md .claude/skills/

Migration skills reference the deep docs in dev-docs/runuo-migration-docs/ and point to existing ModernUO skills for best practices.

The modernuo-code-audit skill auto-triggers on .cs file edits and flags convention violations (warnings only, asks before fixing).