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/Server/Events/AccountLoginEvent.cs b/Projects/Server/Events/AccountLoginEvent.cs index 5c81af000..cf1c0bd1a 100644 --- a/Projects/Server/Events/AccountLoginEvent.cs +++ b/Projects/Server/Events/AccountLoginEvent.cs @@ -37,6 +37,13 @@ public class AccountLoginEventArgs public bool Accepted { get; set; } public ALRReason RejectReason { get; set; } + + /// + /// 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; } } public static partial class EventSink diff --git a/Projects/UOContent.Tests/Tests/Accounting/PasswordWorkerTests.cs b/Projects/UOContent.Tests/Tests/Accounting/PasswordWorkerTests.cs new file mode 100644 index 000000000..5972e5eb9 --- /dev/null +++ b/Projects/UOContent.Tests/Tests/Accounting/PasswordWorkerTests.cs @@ -0,0 +1,229 @@ +using System; +using System.Threading; +using Server.Accounting; +using Server.Accounting.Security; +using Xunit; + +namespace Server.Tests.Accounting; + +[Collection("Sequential UOContent Tests")] +public class PasswordWorkerTests : IDisposable +{ + private const string Password = "hunter2"; + + private readonly PasswordProtectionAlgorithm _originalAlgorithm = AccountSecurity.CurrentAlgorithm; + + public PasswordWorkerTests() => AccountSecurity.CurrentAlgorithm = PasswordProtectionAlgorithm.Argon2; + + public void Dispose() => AccountSecurity.CurrentAlgorithm = _originalAlgorithm; + + private static Account CreateAccount(string username) => + Accounts.GetAccount(username) as Account ?? new Account(username, Password); + + /// + /// Enqueues work, then pumps the loop context until or the deadline. + /// + /// The context pins itself to the thread that constructed it and refuses ExecuteTasks + /// from any other. The fixture's belongs to whichever thread built the fixture, and xUnit gives + /// no guarantee that a test method runs on that thread even inside a sequential collection -- + /// so this owns one for the duration and puts the original back. Pumping the fixture's context + /// passed locally and failed on CI. + /// + private static void PumpUntil(Action enqueue, Func complete, int timeoutSeconds = 20) + { + var original = Core.LoopContext; + var owned = new EventLoopContext(); + Core.LoopContext = owned; + + try + { + enqueue(); + + var deadline = DateTime.UtcNow.AddSeconds(timeoutSeconds); + + while (!complete() && DateTime.UtcNow < deadline) + { + owned.ExecuteTasks(); + Thread.Sleep(5); + } + + // Anything that landed between the last pump and the final check. + owned.ExecuteTasks(); + } + finally + { + Core.LoopContext = original; + } + } + + private static PasswordJob JobFor(Account account, string submitted) => + new() + { + Account = account, + StoredHash = account.Password, + VerifyPhrase = account.GetVerifyPhrase(submitted), + HashPhrase = account.NeedsPasswordUpgrade() ? account.GetRehashPhrase(submitted) : null, + StoredAlgorithm = account.PasswordAlgorithm, + TargetAlgorithm = AccountSecurity.CurrentAlgorithm + }; + + /// + /// Drives the real queue rather than ComputeInline. A job with no NetState attached -- an + /// admin password change -- was being dropped by the liveness check, which read a null State as + /// a dead connection, so the change silently never happened and its callback never fired. + /// + [Fact] + public void RunsAJobThatHasNoConnectionAttached() + { + var account = CreateAccount("offloop-no-netstate-user"); + var applied = false; + + var job = new PasswordJob + { + Account = account, + HashPhrase = account.GetRehashPhrase("a-queued-password"), + TargetAlgorithm = AccountSecurity.CurrentAlgorithm, + OnComplete = (_, outcome) => applied = outcome.Hash != null + }; + + PumpUntil(() => Assert.True(PasswordWorker.TryEnqueue(job)), () => applied); + + Assert.True(applied); + Assert.True(account.CheckPassword("a-queued-password")); + } + + [Fact] + public void VerifiesTheCorrectPassword() + { + var account = CreateAccount("offloop-correct-user"); + + var outcome = PasswordWorker.ComputeInline(JobFor(account, Password)); + + Assert.True(outcome.Verified); + } + + [Fact] + public void RejectsTheWrongPassword() + { + var account = CreateAccount("offloop-wrong-user"); + + var outcome = PasswordWorker.ComputeInline(JobFor(account, "not-the-password")); + + Assert.False(outcome.Verified); + Assert.Null(outcome.Hash); + } + + [Fact] + public void ProducesNoUpgradeWhenParametersAreCurrent() + { + var account = CreateAccount("offloop-current-user"); + + var outcome = PasswordWorker.ComputeInline(JobFor(account, Password)); + + Assert.True(outcome.Verified); + Assert.Null(outcome.Hash); + } + + [Fact] + public void ProducesAnUpgradeWhenParametersAreStale() + { + var account = CreateAccount("offloop-stale-user"); + + // The shipping default before #2562: Argon2i, m=8192, t=3, p=1. + account.Password = + "$argon2i$v=19$m=8192,t=3,p=1$LD1XJz7P3wQmIJ+Tu6ScgA$NO5hBABsHQ172C5nDO2X4gWnB4jDef3x6WhLdVE2LFw"; + + var outcome = PasswordWorker.ComputeInline(JobFor(account, Password)); + + Assert.True(outcome.Verified); + Assert.StartsWith("$argon2id$v=19$m=16384,t=1,p=1$", outcome.Hash); + } + + [Fact] + public void ProducesNoUpgradeWhenThePasswordIsWrong() + { + var account = CreateAccount("offloop-wrong-stale-user"); + account.Password = + "$argon2i$v=19$m=8192,t=3,p=1$LD1XJz7P3wQmIJ+Tu6ScgA$NO5hBABsHQ172C5nDO2X4gWnB4jDef3x6WhLdVE2LFw"; + + var outcome = PasswordWorker.ComputeInline(JobFor(account, "not-the-password")); + + Assert.False(outcome.Verified); + Assert.Null(outcome.Hash); + } + + [Fact] + public void AppliesAWrite() + { + var account = CreateAccount("offloop-apply-user"); + var upgraded = Argon2PasswordProtection.Instance.EncryptPassword(Password); + + account.ApplyPasswordWrite(upgraded, PasswordProtectionAlgorithm.Argon2); + + Assert.Equal(upgraded, account.Password); + Assert.True(account.CheckPassword(Password)); + } + + /// + /// Writes apply in dispatch order, which is what makes a guard unnecessary: dispatch is on the + /// loop, one worker drains FIFO, and results return through the loop context in that same order. + /// A second worker thread would break this and would need ordering reintroduced. + /// + [Fact] + public void WritesApplyInDispatchOrder() + { + var account = CreateAccount("offloop-two-writes-user"); + var done = 0; + + PumpUntil( + () => + { + for (var i = 1; i <= 2; i++) + { + Assert.True( + PasswordWorker.TryEnqueue( + new PasswordJob + { + Account = account, + HashPhrase = account.GetRehashPhrase($"password-{i}"), + StoredAlgorithm = account.PasswordAlgorithm, + TargetAlgorithm = AccountSecurity.CurrentAlgorithm, + OnComplete = (_, _) => done++ + } + ) + ); + } + }, + () => done >= 2 + ); + + Assert.Equal(2, done); + Assert.True(account.CheckPassword("password-2")); + Assert.False(account.CheckPassword("password-1")); + } + + [Theory] + [InlineData(PasswordProtectionAlgorithm.SHA1)] + [InlineData(PasswordProtectionAlgorithm.SHA2)] + public void UsesTheUsernameSaltedPhraseForShaAccounts(PasswordProtectionAlgorithm algorithm) + { + AccountSecurity.CurrentAlgorithm = algorithm; + var account = CreateAccount($"offloop-phrase-{algorithm}-user"); + + // Verification must use the algorithm the hash was stored under... + Assert.Equal($"{account.Username}{Password}", account.GetVerifyPhrase(Password)); + + // ...and a rehash the one it is moving to. Swapping these is the #2562 lockout. + AccountSecurity.CurrentAlgorithm = PasswordProtectionAlgorithm.Argon2; + Assert.Equal(Password, account.GetRehashPhrase(Password)); + } + + [Fact] + public void UsesTheBarePasswordForArgon2Accounts() + { + var account = CreateAccount("offloop-phrase-argon2-user"); + + Assert.Equal(Password, account.GetVerifyPhrase(Password)); + Assert.Equal(Password, account.GetRehashPhrase(Password)); + } +} diff --git a/Projects/UOContent.Tests/Tests/Accounting/Security/PasswordProtectionTest.cs b/Projects/UOContent.Tests/Tests/Accounting/Security/PasswordProtectionTest.cs index b86df418d..a0ef97a56 100644 --- a/Projects/UOContent.Tests/Tests/Accounting/Security/PasswordProtectionTest.cs +++ b/Projects/UOContent.Tests/Tests/Accounting/Security/PasswordProtectionTest.cs @@ -75,6 +75,32 @@ public class PasswordProtectionTest Assert.False(passwordProtection.ValidatePassword(encryptedPassword, "Not the same password")); } + /// + /// Literal digests of , so the stored format cannot drift. These are + /// compared as strings against what is already in every account database -- a casing or encoding + /// change would lock out every SHA and MD5 account on the shard at once. + /// + [Theory] + [InlineData("MD5", "52284053181040AC90DBDE74A0E7FF5E")] + [InlineData("SHA1", "9AC635509803AAE2D8312BA1879289259A50C5F0")] + [InlineData( + "SHA2", + "5A727BFF8F8E08A24BDF6B0CD5065F30A1F8E0060B857BB8AFD6955BE0ACBC489DA63F19B8F4CF08D73DE4069CF4B" + + "29D94B353F31513B2FB2D9382EFE15AE975" + )] + public void HashAlgorithm_StoredFormatIsStable(string algorithmType, string expected) + { + var protection = algorithmType switch + { + "SHA1" => HashAlgorithmPasswordProtection.SHA1Instance, + "SHA2" => HashAlgorithmPasswordProtection.SHA2Instance, + _ => HashAlgorithmPasswordProtection.MD5Instance, + }; + + Assert.Equal(expected, protection.EncryptPassword(plainPassword)); + Assert.True(protection.ValidatePassword(expected, plainPassword)); + } + // The shipping default before this change, as a literal so it cannot drift with the configured // defaults. Password: "hunter2". private const string LegacyArgon2iHash = diff --git a/Projects/UOContent/Accounting/Account.cs b/Projects/UOContent/Accounting/Account.cs index 45ede29bb..b583585ee 100644 --- a/Projects/UOContent/Accounting/Account.cs +++ b/Projects/UOContent/Accounting/Account.cs @@ -379,28 +379,53 @@ public partial class Account : IAccount, IComparable public void SetPassword(string plainPassword) { PasswordAlgorithm = AccountSecurity.CurrentAlgorithm; - var phrase = PasswordAlgorithm is PasswordProtectionAlgorithm.SHA1 or PasswordProtectionAlgorithm.SHA2 - ? $"{_username}{plainPassword}" - : plainPassword; + Password = AccountSecurity.CurrentPasswordProtection.EncryptPassword( + AccountSecurity.DerivePhrase(PasswordAlgorithm, _username, plainPassword) + ); + } - Password = AccountSecurity.CurrentPasswordProtection.EncryptPassword(phrase); + /// The phrase that verifies against the currently stored hash. + internal string GetVerifyPhrase(string plainPassword) => + AccountSecurity.DerivePhrase(_passwordAlgorithm, _username, plainPassword); + + /// The phrase a rehash to the configured algorithm would be derived from. + internal string GetRehashPhrase(string plainPassword) => + AccountSecurity.DerivePhrase(AccountSecurity.CurrentAlgorithm, _username, plainPassword); + + /// + /// Whether a successful login should rewrite the stored hash, because the algorithm changed or + /// its cost parameters moved. + /// + internal bool NeedsPasswordUpgrade() => + _passwordAlgorithm != AccountSecurity.CurrentAlgorithm || + AccountSecurity.CurrentPasswordProtection.NeedsRehash(Password); + + /// + /// 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: 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) + { + PasswordAlgorithm = algorithm; + Password = newEncrypted; } public bool CheckPassword(string plainPassword) { - var phrase = _passwordAlgorithm is PasswordProtectionAlgorithm.SHA1 or PasswordProtectionAlgorithm.SHA2 - ? $"{_username}{plainPassword}" - : plainPassword; + var ok = AccountSecurity.GetPasswordProtection(_passwordAlgorithm) + .ValidatePassword(Password, GetVerifyPhrase(plainPassword)); - var ok = AccountSecurity.GetPasswordProtection(_passwordAlgorithm).ValidatePassword(Password, phrase); if (!ok) { return false; } // Upgrade the password protection in case we change the algorithm - if (_passwordAlgorithm != AccountSecurity.CurrentAlgorithm || - AccountSecurity.CurrentPasswordProtection.NeedsRehash(Password)) + if (NeedsPasswordUpgrade()) { SetPassword(plainPassword); } diff --git a/Projects/UOContent/Accounting/AccountHandler.cs b/Projects/UOContent/Accounting/AccountHandler.cs index 542e16a40..eea0dc069 100644 --- a/Projects/UOContent/Accounting/AccountHandler.cs +++ b/Projects/UOContent/Accounting/AccountHandler.cs @@ -5,6 +5,7 @@ using System.Net; using System.Runtime.CompilerServices; using ModernUO.CodeGeneratedEvents; using Server.Accounting; +using Server.Accounting.Security; using Server.Engines.CharacterCreation; using Server.Engines.Help; using Server.Logging; @@ -69,6 +70,9 @@ public static class AccountHandler public static void Initialize() { EventSink.AccountLogin += EventSink_AccountLogin; + + EventSink.Shutdown += PasswordWorker.Stop; + EventSink.ServerCrashed += PasswordWorker.OnCrashed; } [Usage("Password ")] @@ -139,8 +143,12 @@ public static class AccountHandler if (accessList[0].MatchClassC(ipAddress)) { - acct.SetPassword(pass); - from.SendMessage("The password to your account has changed."); + // Confirmed from the callback: off-loop the write has not landed yet here. + PasswordWorker.SetPassword( + acct, + pass, + _ => from.SendMessage("The password to your account has changed.") + ); } else { @@ -307,25 +315,129 @@ public static class AccountHandler logger.Information("Login: {NetState} Access denied for '{Username}'", e.State, un); e.RejectReason = LockdownLevel > AccessLevel.Player ? ALRReason.BadComm : ALRReason.BadPass; } - else if (!acct.CheckPassword(pw)) + else { - logger.Information("Login: {NetState} Invalid password for '{Username}'", e.State, un); - e.RejectReason = ALRReason.BadPass; + HandlePasswordCheck(e, acct, pw); } - else if (acct.Banned) + } + + /// + /// 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) + { + switch (DispatchPasswordCheck(e, acct, pw)) { - logger.Information("Login: {NetState} Banned account '{Username}'", e.State, un); + case PasswordCheckDispatch.Deferred: + { + e.Deferred = true; + return; + } + case PasswordCheckDispatch.Saturated: + { + // Reject rather than verify inline: steering work back onto the loop is what a + // flood wants. + logger.Warning( + "Login: {NetState} Password verification queue full, rejecting '{Username}'", + e.State, + acct.Username + ); + + e.RejectReason = ALRReason.BadComm; + return; + } + } + + if (!acct.CheckPassword(pw)) + { + logger.Information("Login: {NetState} Invalid password for '{Username}'", e.State, acct.Username); + e.RejectReason = ALRReason.BadPass; + return; + } + + ApplyVerifiedLogin(e, acct); + } + + /// 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) + { + logger.Information("Login: {NetState} Banned account '{Username}'", e.State, acct.Username); e.RejectReason = ALRReason.Blocked; + return; + } + + logger.Information("Login: {NetState} Valid credentials for '{Username}'", e.State, acct.Username); + e.State.Account = acct; + e.Accepted = true; + + acct.LogAccess(e.State); + LoginAllowlist.RecordLogin(e.State?.Address); + } + + private enum PasswordCheckDispatch + { + /// Verify on the loop. + Inline, + + /// Handed to the worker; no verdict yet. + Deferred, + + /// The queue is full. + Saturated + } + + /// + /// Hands the password check to the worker, whatever algorithm it uses. Every protection is safe + /// 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) + { + if (!PasswordWorker.Enabled) + { + return PasswordCheckDispatch.Inline; + } + + var job = new PasswordJob + { + Account = acct, + State = e.State, + StoredHash = acct.Password, + StoredAlgorithm = acct.PasswordAlgorithm, + VerifyPhrase = acct.GetVerifyPhrase(pw), + HashPhrase = acct.NeedsPasswordUpgrade() ? acct.GetRehashPhrase(pw) : null, + TargetAlgorithm = AccountSecurity.CurrentAlgorithm, + OnComplete = static (j, outcome) => + CompleteDeferredAccountLogin(j.State, j.Account, outcome.Verified) + }; + + return PasswordWorker.TryEnqueue(job) + ? PasswordCheckDispatch.Deferred + : PasswordCheckDispatch.Saturated; + } + + /// Resumes a login whose password check ran on the verification thread. + internal static void CompleteDeferredAccountLogin(NetState state, Account acct, bool verified) + { + var e = new AccountLoginEventArgs(state, acct.Username, null); + + if (verified) + { + ApplyVerifiedLogin(e, acct); } else { - logger.Information("Login: {NetState} Valid credentials for '{Username}'", e.State, un); - e.State.Account = acct; - e.Accepted = true; - - acct.LogAccess(e.State); - LoginAllowlist.RecordLogin(e.State?.Address); + logger.Information("Login: {NetState} Invalid password for '{Username}'", state, acct.Username); + e.RejectReason = ALRReason.BadPass; } + + IncomingAccountPackets.CompleteAccountLogin(state, e.Accepted, e.RejectReason); } [OnEvent(nameof(GameServer.GameServerLoginEvent))] diff --git a/Projects/UOContent/Accounting/Security/AccountSecurity.cs b/Projects/UOContent/Accounting/Security/AccountSecurity.cs index 9f7faeafc..deae4c8e0 100644 --- a/Projects/UOContent/Accounting/Security/AccountSecurity.cs +++ b/Projects/UOContent/Accounting/Security/AccountSecurity.cs @@ -51,6 +51,17 @@ public static class AccountSecurity } } + /// + /// The string actually fed to the KDF. SHA1 and SHA2 salt by username; everything else hashes + /// the password alone. Verification must derive with the algorithm the stored hash was made + /// with, and a rehash with the one it is moving to -- deriving with the wrong one produces a + /// hash that verifies once and never again. + /// + public static string DerivePhrase(PasswordProtectionAlgorithm algorithm, string username, string plainPassword) + => algorithm is PasswordProtectionAlgorithm.SHA1 or PasswordProtectionAlgorithm.SHA2 + ? $"{username}{plainPassword}" + : plainPassword; + public static IPasswordProtection GetPasswordProtection(PasswordProtectionAlgorithm algorithm) { var passwordProtection = algorithm switch diff --git a/Projects/UOContent/Accounting/Security/HashAlgorithmPasswordProtection.cs b/Projects/UOContent/Accounting/Security/HashAlgorithmPasswordProtection.cs index 6d6e579cd..63d0ac7a8 100644 --- a/Projects/UOContent/Accounting/Security/HashAlgorithmPasswordProtection.cs +++ b/Projects/UOContent/Accounting/Security/HashAlgorithmPasswordProtection.cs @@ -19,19 +19,47 @@ using Server.Text; namespace Server.Accounting.Security; +/// +/// The obsolete unsalted digests, kept only so imported accounts can log in once and be upgraded. +/// +/// Hashing goes through the one-shot static APIs rather than a retained . +/// A instance carries the running digest across HashCore/HashFinal, so +/// two threads sharing one corrupt each other's result -- and these are process-wide singletons. +/// The static form has no such state, allocates nothing, and produces identical bytes. +/// public class HashAlgorithmPasswordProtection : IPasswordProtection { - public static IPasswordProtection MD5Instance = new HashAlgorithmPasswordProtection(MD5.Create()); - public static IPasswordProtection SHA1Instance = new HashAlgorithmPasswordProtection(SHA1.Create()); - public static IPasswordProtection SHA2Instance = new HashAlgorithmPasswordProtection(SHA512.Create()); - private readonly HashAlgorithm _hashAlgorithm; + private enum Kind + { + MD5, + SHA1, + SHA512 + } - public HashAlgorithmPasswordProtection(HashAlgorithm hashAlgorithm) => _hashAlgorithm = hashAlgorithm; + public static readonly IPasswordProtection MD5Instance = new HashAlgorithmPasswordProtection(Kind.MD5); + public static readonly IPasswordProtection SHA1Instance = new HashAlgorithmPasswordProtection(Kind.SHA1); + public static readonly IPasswordProtection SHA2Instance = new HashAlgorithmPasswordProtection(Kind.SHA512); + + private const int MaxDigestLength = 64; // SHA512, the largest of the three. + + private readonly Kind _kind; + + private HashAlgorithmPasswordProtection(Kind kind) => _kind = kind; public string EncryptPassword(string plainPassword) { var bytes = plainPassword.AsSpan(0, Math.Min(256, plainPassword.Length)).GetBytesAscii(); - return _hashAlgorithm.ComputeHash(bytes).ToHexString(); + + Span digest = stackalloc byte[MaxDigestLength]; + + var written = _kind switch + { + Kind.MD5 => MD5.HashData(bytes, digest), + Kind.SHA1 => SHA1.HashData(bytes, digest), + _ => SHA512.HashData(bytes, digest) + }; + + return digest[..written].ToHexString(); } public bool ValidatePassword(string encryptedPassword, string plainPassword) => diff --git a/Projects/UOContent/Accounting/Security/PBKDF2PasswordProtection.cs b/Projects/UOContent/Accounting/Security/PBKDF2PasswordProtection.cs index 7ae464021..96706c030 100644 --- a/Projects/UOContent/Accounting/Security/PBKDF2PasswordProtection.cs +++ b/Projects/UOContent/Accounting/Security/PBKDF2PasswordProtection.cs @@ -22,23 +22,24 @@ namespace Server.Accounting.Security; public class PBKDF2PasswordProtection : IPasswordProtection { - private const ushort m_MinIterations = 1024; - private const ushort m_MaxIterations = 1536; - private const int m_SaltSize = 8; - private const int m_HashSize = 32; - private const int m_OutputSize = 2 + m_SaltSize + m_HashSize; + private const ushort MinIterations = 1024; + private const ushort MaxIterations = 1536; + private const int SaltSize = 8; + private const int HashSize = 32; + private const int OutputSize = 2 + SaltSize + HashSize; public static readonly IPasswordProtection Instance = new PBKDF2PasswordProtection(); public string EncryptPassword(string plainPassword) { - Span output = stackalloc byte[m_OutputSize]; - var iterations = Utility.RandomMinMax(m_MinIterations, m_MaxIterations); + Span output = stackalloc byte[OutputSize]; + + var iterations = RandomNumberGenerator.GetInt32(MinIterations, MaxIterations + 1); BinaryPrimitives.WriteUInt16LittleEndian(output[..2], (ushort)iterations); - var salt = output.Slice(2, m_SaltSize); + var salt = output.Slice(2, SaltSize); RandomNumberGenerator.Fill(salt); - var hash = output.Slice(2 + m_SaltSize, m_HashSize); + var hash = output.Slice(2 + SaltSize, HashSize); Rfc2898DeriveBytes.Pbkdf2(plainPassword, salt, hash, iterations, HashAlgorithmName.SHA256); return output.ToHexString(); @@ -46,15 +47,15 @@ public class PBKDF2PasswordProtection : IPasswordProtection public bool ValidatePassword(string encryptedPassword, string plainPassword) { - Span encryptedBytes = stackalloc byte[m_OutputSize]; + Span encryptedBytes = stackalloc byte[OutputSize]; encryptedPassword.GetBytes(encryptedBytes); var iterations = BinaryPrimitives.ReadUInt16LittleEndian(encryptedBytes[..2]); - var salt = encryptedBytes.Slice(2, m_SaltSize); + var salt = encryptedBytes.Slice(2, SaltSize); - Span hash = stackalloc byte[m_HashSize]; + Span hash = stackalloc byte[HashSize]; Rfc2898DeriveBytes.Pbkdf2(plainPassword, salt, hash, iterations, HashAlgorithmName.SHA256); - return hash.SequenceEqual(encryptedBytes[(m_SaltSize + 2)..]); + return hash.SequenceEqual(encryptedBytes[(SaltSize + 2)..]); } } diff --git a/Projects/UOContent/Accounting/Security/PasswordWorker.cs b/Projects/UOContent/Accounting/Security/PasswordWorker.cs new file mode 100644 index 000000000..83e9207f1 --- /dev/null +++ b/Projects/UOContent/Accounting/Security/PasswordWorker.cs @@ -0,0 +1,293 @@ +/************************************************************************* + * ModernUO * + * Copyright 2019-2026 - ModernUO Development Team * + * Email: hi@modernuo.com * + * File: PasswordWorker.cs * + * * + * This program is free software: you can redistribute it and/or modify * + * it under the terms of the GNU General Public License as published by * + * the Free Software Foundation, either version 3 of the License, or * + * (at your option) any later version. * + * * + * You should have received a copy of the GNU General Public License * + * along with this program. If not, see . * + *************************************************************************/ + +using System; +using System.Collections.Concurrent; +using System.Threading; +using Server.Logging; +using Server.Network; + +namespace Server.Accounting.Security; + +/// +/// Work handed to the password thread, which reads no game state and writes none. +/// +/// Verify and hash are independently optional: a login verifies and may rehash, an explicit change +/// only hashes. +/// +internal sealed class PasswordJob +{ + public Account Account; + + /// Ties the job to a connection. Null when the work is not gated on one, such as a + /// password change by an admin. + public NetState State; + + /// Hash to verify against, with . + public string StoredHash; + + /// 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. + public string VerifyPhrase; + + /// Phrase to hash, or null when nothing needs writing. + public string HashPhrase; + + public PasswordProtectionAlgorithm TargetAlgorithm; + + /// Runs on the game loop with the result. Free to touch game state. + public Action OnComplete; +} + +internal readonly struct PasswordOutcome +{ + /// True when no verification was asked for, or it succeeded. + public readonly bool Verified; + + /// The derived hash, or null when nothing was hashed or verification failed. + public readonly string Hash; + + public PasswordOutcome(bool verified, string hash) + { + Verified = verified; + Hash = hash; + } +} + +/// +/// 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, 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: only loop time matters, not login latency. +/// +internal sealed class PasswordWorker +{ + private static readonly ILogger logger = LogFactory.GetLogger(typeof(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 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; + + // Nothing signals the worker when a save freeze ends, so it re-checks on this interval -- but + // only while a save is in progress, never in steady state. + private const int SaveGatePollMs = 50; + + private static PasswordWorker _instance; + + // 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; +#else + Environment.ProcessorCount >= 4; +#endif + + private readonly Thread _thread; + private readonly AutoResetEvent _work = new(false); + private readonly ConcurrentQueue _queue = []; + + private int _pending; + private volatile bool _exit; + + private PasswordWorker() + { + _thread = new Thread(Execute) + { + IsBackground = true, + Name = "Password Worker" + }; + + _thread.Start(); + } + + private static PasswordWorker Instance => _instance ??= new PasswordWorker(); + + /// 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) + { + if (Volatile.Read(ref _pending) >= MaxPending) + { + return false; + } + + Interlocked.Increment(ref _pending); + _queue.Enqueue(job); + _work.Set(); + + return true; + } + + /// + /// 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; + + private void Execute() + { + while (!_exit) + { + if (_queue.IsEmpty) + { + // A kernel block at zero CPU. Set() during a hash leaves the event signalled, so a + // wake arriving mid-job is not lost. + _work.WaitOne(); + continue; + } + + if (!CanRunNow()) + { + _work.WaitOne(SaveGatePollMs); + continue; + } + + if (!_queue.TryDequeue(out var job)) + { + continue; + } + + Interlocked.Decrement(ref _pending); + + // 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; + } + + PasswordOutcome outcome; + + try + { + outcome = Compute(job); + } + catch (Exception ex) + { + // A verdict must still come back, or the connection never gets a reply. + logger.Error(ex, "Password work failed for {Username}", job.Account?.Username); + outcome = new PasswordOutcome(false, null); + } + + Core.LoopContext.Post(() => Apply(job, outcome)); + } + } + + private static PasswordOutcome Compute(PasswordJob job) + { + if (job.VerifyPhrase != null && + !AccountSecurity.GetPasswordProtection(job.StoredAlgorithm) + .ValidatePassword(job.StoredHash, job.VerifyPhrase)) + { + return new PasswordOutcome(false, null); + } + + return new PasswordOutcome( + true, + job.HashPhrase == null + ? null : AccountSecurity.GetPasswordProtection(job.TargetAlgorithm).EncryptPassword(job.HashPhrase) + ); + } + + private static void Apply(PasswordJob job, PasswordOutcome outcome) + { + // Re-checked: a connection can drop while the result sits in the loop queue. + if (job.State?.Running == false) + { + return; + } + + if (outcome.Verified && outcome.Hash != null) + { + job.Account.ApplyPasswordWrite(outcome.Hash, job.TargetAlgorithm); + } + + job.OnComplete?.Invoke(job, outcome); + } + + /// + /// Sets a password, off the loop where available and inline otherwise, invoking + /// on the loop either way. + /// + /// 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) + { + if (!Enabled) + { + account.SetPassword(plainPassword); + onDone?.Invoke(true); + return; + } + + var job = new PasswordJob + { + Account = account, + HashPhrase = account.GetRehashPhrase(plainPassword), + TargetAlgorithm = AccountSecurity.CurrentAlgorithm, + OnComplete = (_, outcome) => onDone?.Invoke(outcome.Hash != null) + }; + + if (!TryEnqueue(job)) + { + // 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); + } + } + + /// Runs a job on the calling thread. The seam the tests drive. + internal static PasswordOutcome ComputeInline(PasswordJob job) => Compute(job); + + /// + /// Stops the worker on shutdown or crash. Pending jobs are dropped rather than finished: + /// nothing saves the world after this point, so a write applied here would reach no disk. + /// + /// Draining the loop context is not this type's business either. That belongs in the core + /// shutdown path, before subscriber events run -- a subscriber pumping the shared context would + /// execute other subscribers' work at an arbitrary point in the event order. + /// + internal static void Stop() => _instance?.StopThread(); + + // HandleClosed skips InvokeShutdown when the server crashed, so the crash path needs its own + // subscription. + internal static void OnCrashed(ServerCrashedEventArgs e) => Stop(); + + private void StopThread() + { + _exit = true; + _work.Set(); + _thread.Join(TimeSpan.FromSeconds(5)); + } +} diff --git a/Projects/UOContent/Engines/Advanced Search/AdvancedSearchUtilities.cs b/Projects/UOContent/Engines/Advanced Search/AdvancedSearchUtilities.cs index 2c349847d..6775dcaef 100644 --- a/Projects/UOContent/Engines/Advanced Search/AdvancedSearchUtilities.cs +++ b/Projects/UOContent/Engines/Advanced Search/AdvancedSearchUtilities.cs @@ -1,6 +1,5 @@ using System; using System.Buffers; -using System.Collections.Generic; using System.Globalization; using System.Numerics; using System.Runtime.CompilerServices; diff --git a/Projects/UOContent/Engines/Pathing/MovementPath.cs b/Projects/UOContent/Engines/Pathing/MovementPath.cs index 32aa8ba11..d1ef874fc 100644 --- a/Projects/UOContent/Engines/Pathing/MovementPath.cs +++ b/Projects/UOContent/Engines/Pathing/MovementPath.cs @@ -1,7 +1,5 @@ using System; using System.Diagnostics; -using Server.Engines.Pathing; -using Server.Engines.Pathing.Cache; using Server.Items; using Server.PathAlgorithms; using Server.Spells; diff --git a/Projects/UOContent/Gumps/AdminGump.cs b/Projects/UOContent/Gumps/AdminGump.cs index 1fe917b55..3a48bc5aa 100644 --- a/Projects/UOContent/Gumps/AdminGump.cs +++ b/Projects/UOContent/Gumps/AdminGump.cs @@ -5,6 +5,7 @@ using System.Net; using System.Runtime.InteropServices; using System.Threading; using Server.Accounting; +using Server.Accounting.Security; using Server.Collections; using Server.Commands; using Server.Maps; @@ -2903,7 +2904,7 @@ namespace Server.Gumps else { notice = "The password has been changed."; - a.SetPassword(password); + PasswordWorker.SetPassword(a, password, null); page = AdminGumpPage.AccountDetails_Information; CommandLogging.WriteLine( from, diff --git a/Projects/UOContent/Network/AutoDenylist/AutoDenylist.cs b/Projects/UOContent/Network/AutoDenylist/AutoDenylist.cs index 3ed36cfe5..b9b4dd7c1 100644 --- a/Projects/UOContent/Network/AutoDenylist/AutoDenylist.cs +++ b/Projects/UOContent/Network/AutoDenylist/AutoDenylist.cs @@ -28,7 +28,7 @@ namespace Server.Network; /// /// The local half of promotion. Contributing to CrowdSec only helps once an OS bouncer reacts; until then /// every reconnect costs a socket, a buffer and a NetState slot — and the verdicts that matter most -/// are reachable only after reading bytes, like a zero seed. It is also the whole defence on a shard running +/// are reachable only after reading bytes, like a zero seed. It is also the whole defense on a shard running /// no bouncer, which is the default. Not persisted, by design: a holding pen that survives restarts is a ban /// without a ban's review. Only verdicts are held. /// diff --git a/Projects/UOContent/Network/LoginAllowlist/LoginAllowlistConfiguration.cs b/Projects/UOContent/Network/LoginAllowlist/LoginAllowlistConfiguration.cs index b3242a85f..08f04ecf2 100644 --- a/Projects/UOContent/Network/LoginAllowlist/LoginAllowlistConfiguration.cs +++ b/Projects/UOContent/Network/LoginAllowlist/LoginAllowlistConfiguration.cs @@ -93,7 +93,7 @@ public record LoginAllowlistSettings /// this it escalates like anything else until it earns a new entry by logging in again. /// /// - /// Generous on purpose: local defences never stop applying, so a high threshold only delays the external + /// Generous on purpose: local defenses never stop applying, so a high threshold only delays the external /// ban. A bad line might trip a gate a few times an hour; a host being used to flood burns through this /// in seconds. Set to 0 to never revoke. /// diff --git a/Projects/UOContent/Network/Packets/IncomingAccountPackets.cs b/Projects/UOContent/Network/Packets/IncomingAccountPackets.cs index 888eeb6d7..792ed880b 100644 --- a/Projects/UOContent/Network/Packets/IncomingAccountPackets.cs +++ b/Projects/UOContent/Network/Packets/IncomingAccountPackets.cs @@ -564,7 +564,22 @@ public static class IncomingAccountPackets EventSink.InvokeAccountLogin(accountLoginEventArgs); - if (accountLoginEventArgs.Accepted) + // The password check moved off the loop; whoever took it replies when the verdict lands. + if (accountLoginEventArgs.Deferred) + { + return; + } + + CompleteAccountLogin(state, accountLoginEventArgs.Accepted, accountLoginEventArgs.RejectReason); + } + + /// + /// Replies to an account login. Split out so a verdict produced off the loop reaches the client + /// through exactly the same path as one produced inline. + /// + internal static void CompleteAccountLogin(NetState state, bool accepted, ALRReason rejectReason) + { + if (accepted) { var serverListEventArgs = new GatewayServer.ServerListEventArgs(state, state.Account); @@ -584,7 +599,7 @@ public static class IncomingAccountPackets else { state.Account = null; - AccountLogin_ReplyRej(state, accountLoginEventArgs.RejectReason); + AccountLogin_ReplyRej(state, rejectReason); } } diff --git a/Projects/UOContent/Utilities/Types.cs b/Projects/UOContent/Utilities/Types.cs index fa669d96a..ca5b9422d 100644 --- a/Projects/UOContent/Utilities/Types.cs +++ b/Projects/UOContent/Utilities/Types.cs @@ -1,6 +1,5 @@ using System; using System.Collections.Concurrent; -using System.Collections.Generic; using System.Globalization; using System.Reflection; using System.Runtime.CompilerServices; diff --git a/dev-docs/claude-skills/modernuo-threading.md b/dev-docs/claude-skills/modernuo-threading.md index 800bf77d2..3b6507af2 100644 --- a/dev-docs/claude-skills/modernuo-threading.md +++ b/dev-docs/claude-skills/modernuo-threading.md @@ -150,6 +150,78 @@ 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/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 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. +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. +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 + +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..d89bc1c61 100644 --- a/dev-docs/threading-model.md +++ b/dev-docs/threading-model.md @@ -130,6 +130,138 @@ 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/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 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 + 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. +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 + +**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