Shutdown() drained pending writes and applied them. That accomplished
nothing: no save runs on shutdown -- World.Save() is reachable only from the
autosave timer and the console command, and HandleClosed merely waits for an
in-progress write before Environment.Exit(0) -- so an applied write lands in
an Account that is immediately discarded. It was justified as becoming
correct once a shutdown save exists, which is building for a fix that does
not.
It also called Core.LoopContext.ExecuteTasks(), which is not a subscriber's
to call. Pumping the shared context from inside a shutdown handler runs other
subscribers' posted work at an arbitrary point in the event order. That drain
belongs in the core, before the events, and is recorded as such in the
follow-up handoff along with the ordering it requires.
What is left is stopping the thread, which is the same on both paths, so
shutdown and crash now share one implementation.
Several comments still described an Argon2-only worker: the class summary,
the thread name, the log message, and the dev-docs entry. The worker runs
whichever protection an account stores.
Trimmed the rest to the fact a reader cannot recover from the code, and
dropped the narration around it.
Adds a sixth worker rule to the threading model, which is the one this branch
actually learned: everything a worker calls must itself be safe off-thread,
and a process-wide singleton is not automatically safe. HashAlgorithm carries
the running digest across HashCore/HashFinal, and Utility's RNG is a shared
System.Random and game state besides. Both were reasons the worker had been
narrowed to Argon2, and both were better fixed at the source.
The worker was Argon2-only and kept its own protection instance. Both are now
unnecessary, but not for the reason the code gave.
CreateIsolated() was justified by the RNG, which was wrong. Argon2's Verify
is static-backed and stackalloc throughout, and the salt RNG is a stateless
syscall wrapper -- neither has state to race over. The real blocker was
HashAlgorithmPasswordProtection, which retains a HashAlgorithm carrying the
running digest across HashCore/HashFinal, shared through process-wide
singletons. Two threads there corrupt each other.
That is fixed at the source: hashing now goes through the one-shot static
APIs, which have no such state, allocate nothing, and produce identical
bytes. Literal digests are pinned in a test first, because these are compared
as strings against every account database -- any drift would lock out every
SHA and MD5 account at once.
PBKDF2 drew its iteration count from Utility.RandomMinMax, a shared
System.Random that is both thread-unsafe and game state. It now uses the
cryptographic RNG, matching the salt beside it.
With all three safe, the worker no longer needs to know which algorithm it is
running, and the dispatch conditions collapse to "is off-loop available". A
cheap digest now pays a thread hop it does not need, which costs login
latency we have already decided not to care about, and saves loop time we do.
The sequence guarded against a reordering that cannot happen. Dispatch runs
on the game loop, so enqueue order is dispatch order; one worker drains the
queue FIFO and processes serially; results post to the loop context in that
same order and are drained in it. Last dispatched is last applied.
The case it was written for -- two changes dispatched before either landed,
second silently dropped -- was caused by the hash-comparison guard it
replaced, not by concurrency. Removing the guard fixes it; adding a more
elaborate one was the wrong move.
Checked every inline SetPassword that bypasses the queue: the constructor and
XML import cannot have a job in flight, CheckPassword's rehash is only
reached by non-Argon2 accounts which never queue, and the two fallbacks in
PasswordWorker.SetPassword only run when nothing is queued at all or the
queue holds 4096. So there is no reachable interleave to guard.
The single worker is now load-bearing for ordering as well as for cache,
scheduling and memory. Noted where it matters, and WritesApplyInDispatchOrder
pins it through the real queue so a second worker would fail a test rather
than silently reorder writes.
The liveness check in the worker loop read a null NetState as a dead one:
if (job.State?.Running != true) { continue; }
A password change carries no connection, so State is null, null != true, and
every off-loop SetPassword was silently dropped -- the hash never ran, the
account was never written, and the callback that confirms it to the player
never fired. Apply() had the null case right; the dequeue check did not.
Nothing caught it because the tests all drove ComputeInline, which bypasses
the queue entirely. Adds one that goes through TryEnqueue and pumps the loop
context, which fails against the old check.
Also drops the unused Pending property and narrows MaxPending to private.
Exit() was never wired, so it was dead code either way. Wiring it correctly
depends on which teardown path is running.
On a normal shutdown EventSink.Shutdown fires on the game thread after the
loop has stopped, which is the last chance pending work gets: results the
worker already posted are sitting on a loop context nothing will pump again.
So the thread is stopped, the context drained once, and queued writes are
computed and applied in place. Verifies are dropped instead -- they only
decide a login, and every connection is closing.
On a crash there is no usable game thread, so nothing may be applied and the
thread is simply stopped. Subscribed separately because HandleClosed skips
InvokeShutdown when _crashed is set, which would otherwise leave the crash
path unhandled entirely.
Wired from AccountHandler.Initialize rather than a Configure on the worker:
AssemblyHandler.AddMethods binds Static | Public, so a Configure on an
internal type is never discovered.
SetPassword derives a full Argon2 hash, so the [password command, the admin
gump, account creation and the XML import each cost ~8.9 ms of frozen world.
Only the login verify had been moved.
DRY-ing the two paths surfaced a correctness trap rather than just shared
code. ApplyPasswordUpgrade guarded by comparing the stored hash, which is
right for a login rehash -- do not clobber a newer password with a rehash of
the one it superseded -- but wrong for an explicit change: two changes
dispatched before either landed would drop the second and silently keep the
older password. Ordering is now a per-account dispatch sequence claimed on
the loop, which gives "newest wins" for both callers through one mechanism.
SetPassword bumps it as well, so an inline write also supersedes an in-flight
one.
One job type serves both: PasswordJob carries an optional verify phrase and
an optional hash phrase plus an OnComplete that runs on the loop, so a login
verifies and may rehash while a password change only hashes. The class is
PasswordWorker now, since verification no longer describes what it does.
PasswordWorker.SetPassword is the single entry point and falls back to
hashing inline when the gate is off or the queue is saturated -- unlike a
login, a password change must never be silently dropped, and it is rare
enough that the loop can absorb one.
The [password confirmation moves into the callback, because off the loop it
has not happened when the call returns. Account creation stays inline: it
gates the login flow, so deferring it restructures the accept path.
2026-08-08 12:25:49 -07:00
Renamed from Projects/UOContent/Accounting/Security/PasswordVerificationWorker.cs (Browse further)
