diff --git a/CLAUDE.md b/CLAUDE.md index 924255249..b1a90f6ea 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -19,7 +19,7 @@ Apply these when writing or reviewing `.cs` files under `Projects/`. 7. **`STArrayPool.Shared`** not `ArrayPool.Shared` — single-threaded optimized, no locks 8. **`PooledRefList`** not `new List()` 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). 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(() => …)`. Never touch game state off-thread; never let the scheduler decide where the heavy work runs → `dev-docs/threading-model.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` diff --git a/Projects/UOContent/Accounting/Account.cs b/Projects/UOContent/Accounting/Account.cs index 4a99cfaee..a81b75d1a 100644 --- a/Projects/UOContent/Accounting/Account.cs +++ b/Projects/UOContent/Accounting/Account.cs @@ -401,15 +401,13 @@ public partial class Account : IAccount, IComparable AccountSecurity.CurrentPasswordProtection.NeedsRehash(Password); /// - /// Applies a rehash derived off the game loop. Not to be confused with the private - /// UpgradePassword below, which adopts a legacy hash wholesale during RunUO/ServUO - /// import. + /// Applies a rehash derived off the game loop. Distinct from the private + /// UpgradePassword below, which adopts a legacy hash during RunUO/ServUO import. /// /// - /// The hash the verification started from. Milliseconds passed while it ran, and the password - /// may have been changed in that window by an admin or by the player. The newer value was - /// already written with current parameters and needs no upgrade, so this drops the stale one - /// rather than replacing a live credential with a hash of the previous password. + /// The hash verification started from. If the password changed while it ran, the newer value + /// was already written with current parameters, so the stale upgrade is dropped rather than + /// replacing a live credential with a hash of the password it superseded. /// internal void ApplyPasswordUpgrade( string expectedCurrent, string newEncrypted, PasswordProtectionAlgorithm algorithm diff --git a/Projects/UOContent/Accounting/AccountHandler.cs b/Projects/UOContent/Accounting/AccountHandler.cs index 8aea6cbe2..62fc9e8a6 100644 --- a/Projects/UOContent/Accounting/AccountHandler.cs +++ b/Projects/UOContent/Accounting/AccountHandler.cs @@ -315,9 +315,9 @@ public static class AccountHandler } /// - /// Decides where the password check runs, and produces the verdict when it runs here. An - /// else-if chain cannot express this: the off-loop path yields no verdict at all, and the - /// saturated path is a rejection rather than a reason to fall through and verify. + /// Not an else-if branch in the caller because two of the outcomes are not verdicts: the + /// off-loop path produces none yet, and a full queue must reject rather than fall through and + /// verify. /// private static void HandlePasswordCheck(AccountLoginEventArgs e, Account acct, string pw) { @@ -330,8 +330,8 @@ public static class AccountHandler } case PasswordCheckDispatch.Saturated: { - // Reject rather than verify inline: steering the work back onto the loop is - // what a flood would be trying to achieve. + // Reject rather than verify inline: steering work back onto the loop is what a + // flood would want. logger.Warning( "Login: {NetState} Password verification queue full, rejecting '{Username}'", e.State, @@ -389,12 +389,10 @@ public static class AccountHandler /// /// Hands an Argon2 verify to the verification thread. /// - /// Argon2-stored accounts only. A SHA/MD5 hash is verified in microseconds and its protection - /// holds a shared HashAlgorithm whose ComputeHash is not thread safe, so those - /// stay here. Their one-time rehash into Argon2 stays here too: it costs a migrating account a - /// single 8.9 ms login, exactly as it does today, and moving it would mean either making the - /// player wait on an upgrade that does not gate their verdict, or applying account state from a - /// callback with no login left to attach it to. + /// Argon2-stored accounts only: SHA/MD5 protections share a HashAlgorithm whose + /// ComputeHash is not thread safe, and cost microseconds anyway. Their one-time rehash + /// into Argon2 also stays here, costing a migrating account one 8.9 ms login exactly as today -- + /// moving it would mean waiting on an upgrade that does not gate the verdict. /// private static PasswordCheckDispatch DispatchPasswordCheck(AccountLoginEventArgs e, Account acct, string pw) { diff --git a/Projects/UOContent/Accounting/Security/PasswordVerificationWorker.cs b/Projects/UOContent/Accounting/Security/PasswordVerificationWorker.cs index 7e5e148bc..1f52d39b6 100644 --- a/Projects/UOContent/Accounting/Security/PasswordVerificationWorker.cs +++ b/Projects/UOContent/Accounting/Security/PasswordVerificationWorker.cs @@ -23,19 +23,18 @@ using Server.Network; namespace Server.Accounting.Security; /// -/// Work handed to the verification thread. Everything here is either an immutable string or a -/// reference the worker only carries -- the worker reads no game state and writes none. +/// Work handed to the verification thread. Strings and references it only carries: the worker +/// reads no game state and writes none. /// internal sealed class PasswordVerificationJob { public Account Account; public NetState State; - /// The stored hash at dispatch, used to verify and to guard the upgrade against a - /// password change that lands while this runs. + /// The hash at dispatch. Also guards the upgrade against a password change landing + /// while this runs. public string StoredHash; - /// Phrase to verify against . public string VerifyPhrase; /// Phrase to rehash from, or null when no upgrade is due. @@ -59,42 +58,25 @@ internal readonly struct PasswordVerificationOutcome } /// -/// Runs Argon2 off the game loop. +/// Runs Argon2 off the game loop, where a verify costs ~8.9 ms of frozen world per login attempt. /// -/// An Argon2 verify is ~8.9 ms, which is more than half a frame of frozen world for every login -/// attempt, successful or not. Measurement (docs/handoffs/2026-08-07-off-loop-argon2-hashing.md) -/// puts the on-loop saving at 3.5-8.9 ms per login: the hand-off costs ~220 ns, and the only real -/// residue is the loop's own work slowing while a memory-hard KDF evicts shared L3. +/// Exactly one worker. A single background hasher cannot cost the loop more than the inline verify +/// under any scheduling regime, because at worst it takes an equal share of one core; a pool breaks +/// that bound and is what would make the gain hardware-dependent. It also caps live Argon2 arenas +/// at one, and does the least total harm to the loop's own cache footprint. /// -/// One worker, deliberately, for three reasons that agree: -/// - the per-login contention tax falls with concurrency but total loop damage rises, so one -/// hasher does the least harm to the loop; -/// - a single background hasher cannot cost the loop more than the inline verify under any -/// scheduling regime, because at worst it takes an equal share of one core -- which is what -/// makes the measurement extrapolate to hardware we cannot inspect. A pool breaks that bound; -/// - exactly one 16 MiB Argon2 arena is live at a time whatever the login volume, which answers -/// memory-exhaustion without a separate cap. -/// -/// Throughput is ~110 verifies/sec. Wall-clock login latency is explicitly not a concern, so -/// head-of-line blocking during a rush costs nothing. +/// ~110 verifies/sec, which is ample: login latency is not a concern, only loop time. /// internal sealed class PasswordVerificationWorker { private static readonly ILogger logger = LogFactory.GetLogger(typeof(PasswordVerificationWorker)); /// - /// Backstop, not a policy. The queue is already bounded by construction: AccountLogin rejects a - /// second 0x80 on the same connection via SentFirstPacket, so a connection can hold at - /// most one pending verify, and the engine caps concurrent connections at 4096 - /// (NetState.Network.cs). This matches that bound so it can only trip if the - /// one-per-connection invariant is ever broken. - /// - /// Deliberately not a flood defence. Any cap low enough to blunt an attack rejects real players - /// first -- during a mass reconnect they are the queue -- and a cap high enough not to do that - /// blunts nothing. Refusing logins for the duration of an attack *is* the denial of service, - /// just self-inflicted. That defence belongs at the connection layer, where IP bans, the - /// blocklist and the accept gate already sit and where an attacker is stopped before costing a - /// hash at all. + /// Backstop, not a flood defence. SentFirstPacket holds a connection to one pending + /// verify and the engine caps connections at 4096 (NetState.Network.cs), so this matches + /// that bound and can only trip if the one-per-connection invariant breaks. A cap low enough to + /// blunt an attack would reject real players first -- during a mass reconnect they are the + /// queue. Flood defence belongs at the connection layer. /// internal const int MaxPending = 4096; @@ -104,11 +86,8 @@ internal sealed class PasswordVerificationWorker private static PasswordVerificationWorker _instance; - /// - /// Off-loop verification needs a spare core to move work to, which a 1-2 core host does not - /// have, and is pointless on a dev box or test shard where logins are rare and the simpler - /// path is easier to reason about. - /// + // Needs a spare core to move work to, which a 1-2 core host does not have. Off in DEBUG: + // dev boxes and test shards have few logins and are better served by the simpler path. internal static bool Enabled { get; } = #if DEBUG false; @@ -120,8 +99,8 @@ internal sealed class PasswordVerificationWorker private readonly AutoResetEvent _work = new(false); private readonly ConcurrentQueue _queue = new(); - // Its own Argon2, sharing no RNG with the loop's. Verification is static-backed and would be - // safe either way; hashing is not. + // Its own Argon2: verification is static-backed and safe to share, hashing draws from a + // per-instance RNG and is not. private readonly IPasswordProtection _argon2 = Argon2PasswordProtection.CreateIsolated(); private int _pending; @@ -164,10 +143,9 @@ internal sealed class PasswordVerificationWorker } /// - /// Argon2 only, and only outside the save freeze. The freeze runs on the loop, so nothing new - /// can be queued while it holds; checking before each job bounds the overlap to whichever hash - /// was already in flight. PendingSave counts too -- the serialization threads are awake and - /// spinning on an empty queue by then, which is the worst moment to add a competitor. + /// Checked before each job, which bounds a save overlap to whichever hash was already running: + /// the freeze holds the loop, so nothing new can be queued during it. PendingSave counts too -- + /// the serialization threads are already awake and spinning on an empty queue by then. /// private static bool CanRunNow() => World.WorldState is WorldState.Running or WorldState.WritingSave; @@ -197,11 +175,9 @@ internal sealed class PasswordVerificationWorker Interlocked.Decrement(ref _pending); - // Gone while it waited. Skipping here reclaims the slot without spending ~9 ms on a - // verdict nobody will receive, which matters most during exactly the reconnect rush - // that builds a queue in the first place. Running only ever goes true -> false, so a - // stale read costs a wasted hash that Apply discards -- it can never skip a live - // connection. + // Gone while it waited: skip it rather than spend ~9 ms on a verdict nobody receives. + // Running only goes true -> false, so a stale read wastes a hash but can never skip a + // live connection. if (job.State?.Running != true) { continue; @@ -215,7 +191,7 @@ internal sealed class PasswordVerificationWorker } catch (Exception ex) { - // A verdict must still come back, or the connection waits forever for a reply. + // A verdict must still come back, or the connection never gets a reply. logger.Error(ex, "Password verification failed for {Username}", job.Account?.Username); outcome = new PasswordVerificationOutcome(false, null); } @@ -241,8 +217,7 @@ internal sealed class PasswordVerificationWorker { var state = job.State; - // The connection may have gone while the hash ran. A dead NetState must not be revived, and - // nothing may be written on its behalf. + // Re-checked: the connection can also drop while the verdict sits in the loop queue. if (state?.Running != true) { return; @@ -256,8 +231,7 @@ internal sealed class PasswordVerificationWorker AccountHandler.CompleteDeferredAccountLogin(state, job.Account, outcome.Verified); } - /// Runs a job on the calling thread. The seam the tests drive, and the path taken when - /// off-loop verification is gated off. + /// Runs a job on the calling thread. The seam the tests drive. internal static PasswordVerificationOutcome ComputeInline(PasswordVerificationJob job) => Instance.Compute(job); diff --git a/dev-docs/claude-skills/modernuo-threading.md b/dev-docs/claude-skills/modernuo-threading.md index 800bf77d2..b16f1a0f5 100644 --- a/dev-docs/claude-skills/modernuo-threading.md +++ b/dev-docs/claude-skills/modernuo-threading.md @@ -150,6 +150,75 @@ These files MAY use threading (they're server infrastructure, not game logic): - `Projects/Server/Network/` - Network I/O - `Projects/Server/Timer/Timer.Pool.cs` - Pool refill +## Exceptions: Vetted Workers in UOContent + +**A background thread is a last resort.** The forbidden list is about game logic, which is never +threaded. A dedicated worker touching no game state is the sanctioned way off the loop, and +necessarily uses `new Thread`, `ConcurrentQueue`, `Interlocked`, `AutoResetEvent` and +`volatile` **at the thread boundary only**. + +### Prove the need first + +- Measure **on-loop time**, not wall-clock. Frozen world is the cost; player latency is not. +- Off-loading creates no CPU. On 1-2 cores there is no spare core — gate on `ProcessorCount`. +- Count what stays: dispatch, continuation, and the loop slowing while the worker evicts shared L3. +- Record the measurement, or nobody can re-justify the worker later. + +### Game logic stays on the loop — chunk it + +Work needing game state cannot be threaded at any core count. Too slow for one tick? Split across +ticks, bounded by count or elapsed time — never "until done". + +```csharp +Timer.DelayCall(TimeSpan.Zero, TimeSpan.FromMilliseconds(50), () => +{ + var budget = 0; + while (_cursor < _items.Count && budget++ < 100) { Process(_items[_cursor++]); } +}); +``` + +### Vetted workers + +| Worker | Justification | +|---|---| +| `Accounting/Security/PasswordVerificationWorker.cs` | 8.9 ms/login on-loop; 3.5-8.9 ms measured saving | +| `Engines/Advanced Search/AdvancedSearchGump.cs` | Admin-triggered full-world scan, saves disabled | + +### The five rules + +1. No game state read or written off-thread; dispatch immutable values captured on the loop. +2. Resolve policy (algorithm, salt, era branch) at dispatch — the worker holds none. +3. Park on a kernel wait, never spin. Spinning burns a core on shared hosts. +4. Run only while `WorldState is Running or WritingSave`. **Not** `World.Saving` — that misses + `PendingSave`, where serialization threads are already spinning. +5. Bounded queue, or a bound upstream named in a comment. + +### Crossing the boundary + +Dispatch captures what the continuation will need to re-validate: + +```csharp +var job = new Job { Target = state, Expected = account.Password, Input = DerivePhrase(...) }; +if (!Worker.TryEnqueue(job)) { /* reject — never fall back to running it inline */ } +``` + +Hand back one of two ways, and no other: + +```csharp +Core.LoopContext.Post(() => Apply(job, result)); // a result for a specific caller +Volatile.Write(ref _snapshot, newTable); // a shared table rebuilt periodically +``` + +The continuation re-validates, because time passed: + +```csharp +if (job.Target?.Running != true) { return; } // gone +if (account.Password != job.Expected) { return; } // changed underneath +``` + +Always post a result, including on failure — a worker that throws silently leaves its caller +waiting forever. Use `ConfigureAwait(false)` on every await inside off-loop work. + ## Anti-Patterns | Pattern | Problem | Solution | diff --git a/dev-docs/threading-model.md b/dev-docs/threading-model.md index 09b0ec8f4..64ace0275 100644 --- a/dev-docs/threading-model.md +++ b/dev-docs/threading-model.md @@ -130,6 +130,132 @@ These files in `Projects/Server/` MAY use threading because they handle I/O outs - `Timer/Timer.Pool.cs` -- Async pool refill - `EventLoopTasks.cs` -- The synchronization context itself +### Exceptions: Vetted Workers in `Projects/UOContent/` + +**Take great care here. A background thread is a last resort, not a tool of first choice.** + +The table above is about **game logic**, which is never threaded. A dedicated worker that touches +no game state is the sanctioned way to move CPU-heavy or I/O work off the loop, and it necessarily +uses primitives the table forbids -- `new Thread`, `ConcurrentQueue`, `Interlocked`, +`AutoResetEvent`, `volatile`. Those are legitimate **at the thread boundary**, and nowhere else. + +#### First: prove the need + +Do not add a worker because something "looks slow". Measure, and measure the right thing: + +- **Measure on-loop time, not wall-clock.** How long a player waits does not matter; how long the + world is frozen does. A change that improves latency but not loop time buys nothing. +- **Off-loading does not create CPU.** It converts "the loop is blocked for N ms" into "the loop + competes for cores for N ms". On a 1--2 core host there is no spare core and it buys nothing at + all -- gate on `Environment.ProcessorCount`. +- **Account for what stays behind.** Dispatch, the continuation, and the loop's own work slowing + down while the worker evicts shared L3. That last one is real and is usually the largest. +- **Write the benchmark down.** A worker with no recorded measurement cannot be re-justified later, + and will be removed by someone who cannot tell whether it earns its complexity. + +#### Game logic stays on the loop -- chunk it instead + +Work that **needs** game state cannot be threaded at any core count. If it is too slow for one +tick, split it across ticks rather than across threads: + +```csharp +// Bound the work per tick, resume where it left off. +Timer.DelayCall(TimeSpan.Zero, TimeSpan.FromMilliseconds(50), () => +{ + var budget = 0; + while (_cursor < _items.Count && budget++ < 100) + { + Process(_items[_cursor++]); + } +}); +``` + +Bound by count or elapsed time, never by "until done". Threading game state is not a faster +version of this -- it is a correctness bug. + +#### Vetted workers + +| Worker | Off-loop work | Justification | +|---|---|---| +| `Accounting/Security/PasswordVerificationWorker.cs` | Argon2 password verification | `docs/handoffs/2026-08-07-off-loop-argon2-hashing.md` -- 8.9 ms/login on-loop, measured 3.5--8.9 ms saved | +| `Engines/Advanced Search/AdvancedSearchGump.cs` | Parallel entity search | Admin-triggered full-world scan; saves disabled for its duration | + +Adding to this table needs the same bar: a measurement, and all five rules below. + +#### The five rules + +1. **No game state off-thread, read or written.** Hand the worker immutable values (strings, + structs) captured on the loop. Carrying a reference is fine only if the worker just passes it + back untouched. +2. **Decide policy on the loop, compute on the worker.** Anything rule-dependent -- which algorithm, + which salt, which era branch -- is resolved at dispatch, so the worker holds no policy it could + apply inconsistently. +3. **Park on a kernel wait; never spin.** `AutoResetEvent.WaitOne()` costs nothing while idle. + `SerializationThreadWorker` does spin, but only to await a producer mid-drain; absent that race, + spinning is a bug that burns a core on shared hosts. +4. **Yield to world saves.** Run only while `WorldState is Running or WritingSave`. `World.Saving` + is *not* the right check -- it covers only the freeze and misses `PendingSave`, where the + serialization threads are already awake and spinning on an empty queue. +5. **Bound the queue**, or rely on a bound upstream and say which one in a comment. + +#### Handing work across the boundary + +**Loop → worker (dispatch).** Snapshot everything needed into immutable values. Capture any value +you intend to overwrite later, so the continuation can tell whether it changed: + +```csharp +var job = new Job +{ + Target = state, // carried, never dereferenced off-thread + Expected = account.Password, // captured so the continuation can detect a change + Input = DerivePhrase(...) // policy resolved here, on the loop +}; + +if (!Worker.TryEnqueue(job)) +{ + // Full. Reject -- do not fall back to running it inline, or a flood steers the work + // straight back onto the loop. +} +``` + +**Worker → loop (hand back).** Two sanctioned routes, and no others: + +```csharp +// 1. Marshal the apply step. Preferred when a specific result belongs to a specific caller. +Core.LoopContext.Post(() => Apply(job, result)); + +// 2. Publish an immutable snapshot behind a single volatile reference, read lock-free by the loop. +// Preferred for a shared lookup table rebuilt periodically. +Volatile.Write(ref _snapshot, newTable); +``` + +**The continuation must re-validate.** Time passed, and the loop kept running: + +```csharp +private static void Apply(Job job, Result result) +{ + // Gone? Never revive a dead NetState or a deleted entity. + if (job.Target?.Running != true) + { + return; + } + + // Changed? Do not overwrite a newer value with one derived from an older one. + if (!string.Equals(account.Password, job.Expected, StringComparison.Ordinal)) + { + return; + } + + account.Apply(result); +} +``` + +**Always post a result, including on failure.** A worker that throws and posts nothing leaves +whatever awaited it waiting forever. Catch, log, and post a failure verdict. + +**Never** call into game state from the worker, and never `await` on the loop in a way that lets a +continuation resume heavy work there -- `ConfigureAwait(false)` on every await inside off-loop work. + ## Memory Pooling ### STArrayPool