diff --git a/Projects/Server/Events/AccountLoginEvent.cs b/Projects/Server/Events/AccountLoginEvent.cs index b631ac68a..cf1c0bd1a 100644 --- a/Projects/Server/Events/AccountLoginEvent.cs +++ b/Projects/Server/Events/AccountLoginEvent.cs @@ -39,9 +39,9 @@ public class AccountLoginEventArgs public ALRReason RejectReason { get; set; } /// - /// No verdict yet: a subscriber moved the password check off the game loop and will reply - /// itself once it lands. The packet handler must not send an accept or a reject when this is - /// set, or the client receives two answers to one login. + /// No verdict yet: a subscriber moved the password check off the game loop and replies itself + /// once it lands. The packet handler must send neither accept nor reject while this is set, or + /// the client gets two answers to one login. /// public bool Deferred { get; set; } } diff --git a/Projects/UOContent/Accounting/Account.cs b/Projects/UOContent/Accounting/Account.cs index 093153951..b583585ee 100644 --- a/Projects/UOContent/Accounting/Account.cs +++ b/Projects/UOContent/Accounting/Account.cs @@ -404,10 +404,9 @@ public partial class Account : IAccount, IComparable /// Applies a hash derived off the game loop. Distinct from the private UpgradePassword /// below, which adopts a legacy hash when loading pre-binary XML accounts. /// - /// Unguarded, because ordering is already total: dispatch happens on the loop, the worker takes - /// one job at a time in FIFO order, and results come back through the loop context in that same - /// order. Last dispatched is therefore last applied. A second worker thread would break that - /// and would need ordering reintroduced here. + /// Unguarded: dispatch is on the loop, one worker drains FIFO, and results return through the + /// loop context in that order, so last dispatched is last applied. A second worker would need + /// ordering reintroduced here. /// internal void ApplyPasswordWrite(string newEncrypted, PasswordProtectionAlgorithm algorithm) { diff --git a/Projects/UOContent/Accounting/AccountHandler.cs b/Projects/UOContent/Accounting/AccountHandler.cs index 4f32b3e62..9dfde1b74 100644 --- a/Projects/UOContent/Accounting/AccountHandler.cs +++ b/Projects/UOContent/Accounting/AccountHandler.cs @@ -71,7 +71,7 @@ public static class AccountHandler { EventSink.AccountLogin += EventSink_AccountLogin; - // Wired here because AssemblyHandler only discovers *public* static Configure/Initialize, + // Wired here because AssemblyHandler only discovers public static Configure/Initialize, // and PasswordWorker is internal. EventSink.Shutdown += PasswordWorker.Shutdown; EventSink.ServerCrashed += PasswordWorker.OnCrashed; @@ -145,8 +145,7 @@ public static class AccountHandler if (accessList[0].MatchClassC(ipAddress)) { - // Confirmed from the callback: off the loop the hash has not landed yet when - // this returns. + // Confirmed from the callback: off-loop the write has not landed yet here. PasswordWorker.SetPassword( acct, pass, @@ -325,9 +324,8 @@ public static class AccountHandler } /// - /// 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. + /// Separate from the caller's else-if chain because two outcomes are not verdicts: the off-loop + /// path has none yet, and a full queue must reject rather than fall through and verify. /// private static void HandlePasswordCheck(AccountLoginEventArgs e, Account acct, string pw) { @@ -341,7 +339,7 @@ public static class AccountHandler case PasswordCheckDispatch.Saturated: { // Reject rather than verify inline: steering work back onto the loop is what a - // flood would want. + // flood wants. logger.Warning( "Login: {NetState} Password verification queue full, rejecting '{Username}'", e.State, @@ -363,10 +361,8 @@ public static class AccountHandler ApplyVerifiedLogin(e, acct); } - /// - /// Everything after the password is known good. Shared so an off-loop verdict lands in exactly - /// the same state as an inline one. - /// + /// Everything after the password is known good, shared so an off-loop verdict lands + /// in the same state as an inline one. private static void ApplyVerifiedLogin(AccountLoginEventArgs e, Account acct) { if (acct.Banned) @@ -386,10 +382,10 @@ public static class AccountHandler private enum PasswordCheckDispatch { - /// Verify on the loop, as before. + /// Verify on the loop. Inline, - /// Handed to the verification thread; no verdict yet. + /// Handed to the worker; no verdict yet. Deferred, /// The queue is full. @@ -398,12 +394,10 @@ public static class AccountHandler /// /// Hands the password check to the worker, whatever algorithm it uses. Every protection is safe - /// to run off the loop, so there is no carve-out. - /// - /// Nor is a cheap digest worth carving out. AccountSecurity.Configure refuses anything - /// below SHA2 as the configured algorithm, so MD5 and SHA1 only ever appear as a stored hash - /// awaiting migration -- which makes NeedsPasswordUpgrade true, and the job carries the - /// upgrade hash that dominates it. The microsecond digest is never the whole job. + /// off the loop, so there is no carve-out, and a cheap digest does not need one either: + /// AccountSecurity.Configure refuses anything below SHA2 as the configured algorithm, so + /// MD5 and SHA1 only appear as a stored hash awaiting migration. That makes + /// NeedsPasswordUpgrade true, and the upgrade hash dominates the job. /// private static PasswordCheckDispatch DispatchPasswordCheck(AccountLoginEventArgs e, Account acct, string pw) { diff --git a/Projects/UOContent/Accounting/Security/PasswordWorker.cs b/Projects/UOContent/Accounting/Security/PasswordWorker.cs index 0ef3b956f..686872480 100644 --- a/Projects/UOContent/Accounting/Security/PasswordWorker.cs +++ b/Projects/UOContent/Accounting/Security/PasswordWorker.cs @@ -22,11 +22,10 @@ using Server.Network; namespace Server.Accounting.Security; /// -/// Work handed to the password thread. Strings and references it only carries: the worker reads no -/// game state and writes none. +/// Work handed to the password thread, which reads no game state and writes none. /// -/// Either half is optional, which is what lets one job type serve both callers. A login verifies -/// and may rehash; an explicit password change only hashes. +/// Verify and hash are independently optional: a login verifies and may rehash, an explicit change +/// only hashes. /// internal sealed class PasswordJob { @@ -39,8 +38,8 @@ internal sealed class PasswordJob /// Hash to verify against, with . public string StoredHash; - /// Algorithm was written with. Resolved on the loop, because - /// AccountSecurity.CurrentAlgorithm is mutable state the worker must not read. + /// Algorithm was written with. Both algorithms are resolved on + /// the loop; AccountSecurity.CurrentAlgorithm is mutable state the worker must not read. public PasswordProtectionAlgorithm StoredAlgorithm; /// Phrase to verify, or null to skip verification. @@ -71,14 +70,16 @@ internal readonly struct PasswordOutcome } /// -/// Runs Argon2 off the game loop, where a verify costs ~8.9 ms of frozen world per login attempt. +/// Runs password hashing off the game loop. An Argon2 verify costs ~8.9 ms of frozen world per +/// login attempt, successful or not. /// -/// 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. +/// Exactly one worker, and that is load-bearing three times over. It cannot cost the loop more than +/// an inline verify under any scheduling regime, because at worst it takes an equal share of one +/// core -- which is what lets the measurement hold on hardware we cannot inspect. It caps live +/// hashing arenas at one. And writes apply in dispatch order only because a single thread drains +/// FIFO, so a second would need ordering reintroduced. /// -/// ~110 verifies/sec, which is ample: login latency is not a concern, only loop time. +/// ~110 verifies/sec, which is ample: only loop time matters, not login latency. /// internal sealed class PasswordWorker { @@ -87,9 +88,8 @@ internal sealed class PasswordWorker /// /// Backstop, not a flood defense. 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 defense belongs at the connection layer. + /// that bound and can only trip if that invariant breaks. A cap low enough to blunt an attack + /// would reject real players first; flood defense belongs at the connection layer. /// private const int MaxPending = 4096; @@ -99,8 +99,8 @@ internal sealed class PasswordWorker private static PasswordWorker _instance; - // 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. + // Needs a spare core to move work to, which a 1-2 core host does not have. Off in DEBUG, where + // logins are rare and the inline path is easier to follow. internal static readonly bool Enabled = #if DEBUG false; @@ -120,7 +120,7 @@ internal sealed class PasswordWorker _thread = new Thread(Execute) { IsBackground = true, - Name = "Password Verification" + Name = "Password Worker" }; _thread.Start(); @@ -129,10 +129,7 @@ internal sealed class PasswordWorker // Created on first use, so a shard that never takes the off-loop path never allocates a thread. private static PasswordWorker Instance => _instance ??= new PasswordWorker(); - /// - /// Queues a job. False when the queue is full, in which case the caller must reject the login - /// without verifying. - /// + /// Queues a job. False when full, and the caller must then reject without verifying. internal static bool TryEnqueue(PasswordJob job) => Instance.TryEnqueueCore(job); private bool TryEnqueueCore(PasswordJob job) @@ -181,10 +178,9 @@ internal sealed class PasswordWorker Interlocked.Decrement(ref _pending); - // 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. A null State means the job is not tied to a connection at all, such - // as an admin password change, and must still run. + // Gone while it waited: skip it rather than hash for a verdict nobody receives. Running + // only goes true -> false, so a stale read wastes a hash but never skips a live one. A + // null State is a job with no connection to lose, and still runs. if (job.State?.Running == false) { continue; @@ -199,7 +195,7 @@ internal sealed class PasswordWorker catch (Exception ex) { // A verdict must still come back, or the connection never gets a reply. - logger.Error(ex, "Password verification failed for {Username}", job.Account?.Username); + logger.Error(ex, "Password work failed for {Username}", job.Account?.Username); outcome = new PasswordOutcome(false, null); } @@ -226,8 +222,7 @@ internal sealed class PasswordWorker private static void Apply(PasswordJob job, PasswordOutcome outcome) { - // Re-checked: a connection can drop while the result sits in the loop queue. Jobs with no - // connection attached, such as an admin password change, are unaffected. + // Re-checked: a connection can drop while the result sits in the loop queue. if (job.State?.Running == false) { return; @@ -242,12 +237,11 @@ internal sealed class PasswordWorker } /// - /// Sets a password, off the loop when that is available and inline otherwise, invoking - /// on the loop either way. Both branches claim a write slot first, so - /// the newest request wins however the work was routed. + /// Sets a password, off the loop where available and inline otherwise, invoking + /// on the loop either way. /// - /// The confirmation belongs in , not at the call site: off-loop it has - /// not happened yet when the call returns. + /// Confirm from , not the call site: off-loop the write has not + /// happened when this returns. /// internal static void SetPassword(Account account, string plainPassword, Action onDone) { @@ -268,8 +262,8 @@ internal sealed class PasswordWorker if (!TryEnqueue(job)) { - // Saturated. A password change is rare and must not be silently dropped, so this one - // pays the hash on the loop rather than failing. + // Saturated. Unlike a login, a password change must not be dropped, so it pays the + // hash on the loop instead. account.SetPassword(plainPassword); onDone?.Invoke(true); } @@ -279,11 +273,10 @@ internal sealed class PasswordWorker internal static PasswordOutcome ComputeInline(PasswordJob job) => Compute(job); /// - /// Normal shutdown. The loop has stopped but this runs on the game thread, so pending work can - /// be finished in place -- which is the only chance it gets, since nothing will pump the loop - /// context again. + /// Normal shutdown, on the game thread after the loop has stopped. Pending work gets no other + /// chance: nothing will pump the loop context again. /// - /// Only writes are finished. A verify decides a login, and every connection is closing. + /// Writes only. A verify decides a login, and every connection is closing. /// internal static void Shutdown() { @@ -296,7 +289,7 @@ internal sealed class PasswordWorker instance.StopThread(); - // Results posted before the thread stopped are still queued on a loop that has exited. + // Results posted before the thread stopped are queued on a loop that has exited. Core.LoopContext.ExecuteTasks(); while (instance._queue.TryDequeue(out var job)) @@ -318,9 +311,8 @@ internal sealed class PasswordWorker } /// - /// Crash. There is no usable game thread, so nothing may be applied -- stop the thread and let - /// whatever was pending go. Subscribed separately because HandleClosed skips - /// InvokeShutdown when the server crashed. + /// Crash. No usable game thread, so nothing may be applied and pending work is dropped. + /// Subscribed separately because HandleClosed skips InvokeShutdown when crashed. /// internal static void OnCrashed(ServerCrashedEventArgs e) => _instance?.StopThread(); diff --git a/dev-docs/claude-skills/modernuo-threading.md b/dev-docs/claude-skills/modernuo-threading.md index 3b2f370c7..3b6507af2 100644 --- a/dev-docs/claude-skills/modernuo-threading.md +++ b/dev-docs/claude-skills/modernuo-threading.md @@ -181,10 +181,10 @@ Timer.DelayCall(TimeSpan.Zero, TimeSpan.FromMilliseconds(50), () => | Worker | Justification | |---|---| -| `Accounting/Security/PasswordWorker.cs` | 8.9 ms/login on-loop; 3.5-8.9 ms measured saving | +| `Accounting/Security/PasswordWorker.cs` | 8.9 ms/login on-loop at Argon2; 3.5-8.9 ms measured saving | | `Engines/Advanced Search/AdvancedSearchGump.cs` | Admin-triggered full-world scan, saves disabled | -### The five rules +### The six 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. @@ -192,6 +192,9 @@ Timer.DelayCall(TimeSpan.Zero, TimeSpan.FromMilliseconds(50), () => 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. +6. Everything the worker calls must itself be thread-safe. A singleton is not automatically safe — + `HashAlgorithm.ComputeHash` carries state, `Utility`'s RNG is a shared `System.Random` and game + state. Prefer static one-shot APIs (`SHA256.HashData`, `RandomNumberGenerator.Fill`). ### Crossing the boundary diff --git a/dev-docs/threading-model.md b/dev-docs/threading-model.md index d625f3093..d89bc1c61 100644 --- a/dev-docs/threading-model.md +++ b/dev-docs/threading-model.md @@ -177,12 +177,12 @@ version of this -- it is a correctness bug. | Worker | Off-loop work | Justification | |---|---|---| -| `Accounting/Security/PasswordWorker.cs` | Argon2 verification and hashing | `docs/handoffs/2026-08-07-off-loop-argon2-hashing.md` -- 8.9 ms/login on-loop, measured 3.5--8.9 ms saved | +| `Accounting/Security/PasswordWorker.cs` | Password verification and hashing | `docs/handoffs/2026-08-07-off-loop-argon2-hashing.md` -- 8.9 ms/login on-loop at Argon2, 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 +#### The six 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 @@ -197,6 +197,12 @@ Adding to this table needs the same bar: a measurement, and all five rules below 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. +6. **Everything the worker calls must itself be safe off-thread.** A process-wide singleton is not + automatically safe -- look for instance state. `HashAlgorithm.ComputeHash` carries the running + digest across `HashCore`/`HashFinal`, so two threads sharing one corrupt each other. `Utility`'s + RNG is a shared `System.Random`, which is both thread-unsafe and game state. Prefer the static + one-shot forms (`SHA256.HashData`, `RandomNumberGenerator.Fill`), and if a dependency cannot be + made safe, fix it at the source rather than narrowing the worker around it. #### Handing work across the boundary