AdvancedSearchThreadWorker.Execute signaled _stopEvent before clearing
_pause and before reading the exit condition. Sleep() unblocks the instant
that signal fires, so the owning thread can start the next cycle while the
worker is still finishing the previous one. Two ways that goes wrong:
- Reuse hang. The next cycle's Wake/Push/Sleep writes _pause = true, then
the worker's stale `_pause = false` lands on top of it. The inner loop
never observes pauseRequested, the queue is already empty, and it spins
on Thread.Yield() forever -- so the owning thread's next Sleep() waits on
a _stopEvent that is never set again. A single search wakes each worker
once, so this only surfaces once the shared pool is reused.
- Orphaned Exit. Exit() sets _exit, Wake()s, then Sleep()s, the moment the
drain's Sleep() returns. Reading _exit after the signal, the worker can
observe that fresh _exit, return without ever consuming the Wake, and
leave Exit()'s Sleep() waiting on a signal nobody will send. The IsAlive
guard does not close this: the thread passes the check and returns
immediately after.
Both go away by ordering the handshake the way SerializationThreadWorker
already does -- sample the exit condition, clear _pause, then signal. That
worker is the reference for this pattern and is protected by its comment
alone, which is the precedent followed here.
Verified before merge with two throwaway timing tests (25k reuse cycles
and 2k drain-then-Exit cycles): both failed against the previous ordering
and passed with this one. They are not part of the change. Their
reproduction threshold is a property of one machine's scheduler -- at 2k
and 200 cycles the buggy build passed -- so as permanent tests they would
have cost ~560ms and 2000 thread creations per suite run for a guarantee
that may not hold on a CI runner.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
## Summary
Hardens the **Advanced Search** engine (`Projects/UOContent/Engines/Advanced Search/`) — the GM entity finder that fans searches across background worker threads. A code review surfaced 14 defects (A–N), including a shard-crasher reachable from a single admin typo and a path that silently disables autosave for the rest of the shard's uptime. Each behavioral fix ships with a test.
Full `UOContent.Tests` suite: **530/530 green** (21 new AdvancedSearch tests).
## Fixes
### Crash / data-loss
- **A — Shard crash on a malformed Property Test.** `AdvancedSearchThreadWorker.Execute` had no `try/catch` and the worker `Thread` is foreground, so a parse throw (`Hits>abc`, `Layer=onehanded` — `Enum.Parse` was case-sensitive, `Hits>1@` — empty sub-expression indexing) terminated the process. Now: `ParseValue`/`CompareValues` use `TryParse`/`Enum.TryParse(ignoreCase)` and return no-match instead of throwing; the per-entity filter is wrapped in `try/catch` (logs + skips); empty expressions are guarded.
- **C — Overlapping searches corrupt state + brick autosave.** `_threadWorkers`/`_threadId` were `static` but `DoSearch` is an instance method; a second search (double-click / two admins) stomped shared worker state and could leave a drain waiting forever on the shared `AutoResetEvent`, so `AutoSave.SavesEnabled` was never restored. Now: an `Interlocked` re-entrancy guard rejects concurrent searches.
- **G — Autosave restore not guaranteed.** The restore lived only in the success callback. Now it's in a `finally` (plus an outer `catch` covering the synchronous setup and a `catch` on the drain body), so autosave + the guard are always released.
### Wrong results
- **D — `@`/`|` operator precedence.** `a@b|c` evaluated as `a && (b || c)` instead of `(a && b) || c`. OR now binds looser than AND (`AdvancedSearchUtilities.EvaluateBoolean`, unit-tested).
- **E — Descending sort, partial last page rendered blank** (the index decreased in descending mode and the `break` early-out killed the loop). Now a bounded `VisibleCount`-driven loop renders the last page in both directions.
- **F — Deleted entities** were not skipped (ghost rows). Now `DoEntitySearch` skips `entity.Deleted`.
- **N — Reference-type comparisons** threw (`Comparer<T>.Default.Compare` on non-`IComparable`) and compared references to a string. Now equality is by value and ordering is guarded to `IComparable` (no throw).
### Worker perf / hardening
- **H** busy-spin → `Thread.Yield()` in the drain; **I** `GetProperties()` cached per `Type`; **J** `HandleValidInternal` moved behind the cheap map/range/region filters; **K** worker threads are `IsBackground` + `Exit()` tolerates an already-terminated worker; **L** `_filter == null` guard; **M** consistent `Volatile` access on `_pause`/`_exit`.
### Documented
- **B** — the residual worker/event-loop read race is documented on `AdvancedSearchThreadWorker`: workers read live entity state concurrently with the loop, so value-type reads may be stale-but-safe and getter exceptions are swallowed; fully eliminating it would require snapshotting entity fields on the main thread (deferred).
## Notes
- New test-only seams (`TryBeginSearch`/`EndSearch`/`IsSearchInProgress`/`VisibleCount`/`TryParseValue`/`EvaluateBoolean`) are `internal` via the existing `InternalsVisibleTo("UOContent.Tests")`.
- Dead `public ParseValue<T>` removed.
- `ConcurrentDictionary` for the reflection cache is intentional — these workers are genuinely parallel.
