From dc98cf3cfc84b68fd1b3be8b303693cb7ebf7c4f Mon Sep 17 00:00:00 2001 From: Kamron Batman <3953314+kamronbatman@users.noreply.github.com> Date: Sat, 25 Jul 2026 14:57:30 -0700 Subject: [PATCH] fix(advancedsearch): clear pause and sample exit before signaling the drain 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) --- .../Advanced Search/AdvancedSearchThreadWorker.cs | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/Projects/UOContent/Engines/Advanced Search/AdvancedSearchThreadWorker.cs b/Projects/UOContent/Engines/Advanced Search/AdvancedSearchThreadWorker.cs index a4a26c244..5206477c9 100644 --- a/Projects/UOContent/Engines/Advanced Search/AdvancedSearchThreadWorker.cs +++ b/Projects/UOContent/Engines/Advanced Search/AdvancedSearchThreadWorker.cs @@ -117,10 +117,15 @@ public class AdvancedSearchThreadWorker } } - worker._stopEvent.Set(); // Allow the main thread to continue now that we are finished + // The owning thread may start another cycle the moment _stopEvent is set (Exit does exactly + // that). Clear _pause and sample the exit condition before signaling, or the new cycle's + // pause request is clobbered / its Sleep orphaned. Matches SerializationThreadWorker. + var exiting = Core.Closing || Volatile.Read(ref worker._exit); Volatile.Write(ref worker._pause, false); - if (Core.Closing || Volatile.Read(ref worker._exit)) + worker._stopEvent.Set(); // Allow the main thread to continue now that we are finished + + if (exiting) { return; }