AwaitableEvent: concurrency bugs (lost wakeup, double-activation, shared wait event) affecting Threading & Caching patterns

[gfraiteur]

Summary

AwaitableEvent (Patterns/Common/PostSharp.Patterns.Common/Threading/Primitives/AwaitableEvent.cs) — the lock‑free wait primitive behind PostSharp.Patterns.Caching.BackgroundTaskScheduler — has several concurrency defects. The most severe is a lost wakeup in the manual‑reset path that can hang BackgroundTaskScheduler.WhenBackgroundTasksCompleted (and therefore caching‑backend dispose) forever.

These were found while working on the ported copy in Metalama.Patterns; the same code and the same bugs exist in this repository. Line numbers below refer to the PostSharp source.

Bugs

1. Lost wakeup: missing StoreLoad fence in the manual‑reset Set/wait handshake (highest severity)

SetManualReset() publishes the signal with a plain volatile store (this.SignalState = SIGNALED) and then drains the operation queue. The async waiter (ScheduleContinuationInner, manual‑reset branch) enqueues its operation and then reads the signal with a plain volatile read (if (this.SignalState == SIGNALED)).

This is a Dekker‑style handshake with no full fence on either side, so both sides can miss each other: a waiter that reads NOT_SIGNALED parks a WAITING operation that the concurrent drain never observes (it sees a stale, empty queue). The operation is then stranded in WAITING forever, even though SignalState == SIGNALED.

The auto‑reset path does not have this bug because it re‑reads the signal with Interlocked.CompareExchange (a full fence).

Symptom: WhenBackgroundTasksCompleted never completes. Reproduced by hammering "enqueue one background task, then await WhenBackgroundTasksCompleted" — it hangs within seconds (~1 run in 3), and reflecting into the event shows taskCount = 0, SignalState = SIGNALED, one operation stuck in WAITING, stable for 60+ s. Under CPU saturation it reproduces even more readily.

Fix: add a full Interlocked.MemoryBarrier() on both sides of the manual‑reset handshake — after the SIGNALED store in SetManualReset, and after Operations.Enqueue(op) in ScheduleContinuationInner and WaitManualReset.

2. Double activation: Activate() is not idempotent

WaitOperationAsync.Activate() / WaitOperationAsync<TData>.Activate() (≈ lines 960, 1037) fall through and re‑schedule the continuation when the operation is already SUCCESS (the "Operation already in SUCCESS state." branch does not return). ScheduleContinuationInner also calls op.Activate() in the branch where another thread already won the transition (Set()'s drain). The result is the async continuation being scheduled twice → the awaiting state machine runs to completion twice → InvalidOperationException: "An attempt was made to transition a task to a final state when it had already completed." on a thread‑pool thread.

Fix: schedule the continuation at most once (e.g. an Interlocked "already scheduled" guard around the dispatch).

3. Manual‑reset async self‑activation CAS uses the wrong expected state

In ScheduleContinuationInner, manual‑reset branch (≈ line 868), after the operation has been moved CREATED → WAITING, the re‑check does:

if (CREATED == Interlocked.CompareExchange(ref op.State, SUCCESS, CREATED))

The operation is WAITING at this point, so the CAS always fails and self‑activation is skipped, leaving activation to a future Set() drain that (per bug #1) may never observe it. The auto‑reset branch (≈ line 795) correctly compares against WAITING.

Fix: compare against WAITING, matching the auto‑reset branch.

4. The shared [ThreadStatic] wait event lets one wait corrupt another

GetThreadLocalEvent() (≈ line 296) hands every blocking wait on a thread the same ManualResetEventSlim, and WaitOperationSync.Activate() (≈ line 931) unconditionally does this.Event.Set(); return true; — it never CASes the state and can never "lose". Together these are unsafe:

• Spurious wakeup. An operation left in the queue by a timeout or cancellation still references the shared thread‑static event. A later Set() that drains the queue calls Activate() on that stale operation and sets the shared event — which the thread may by then be reusing for a completely different wait. That wait returns without a signal (e.g. Dispose returning while background tasks are still running).
• Lost signal (auto‑reset). SetAutoReset treats the unconditional true as "signal delivered" and stops draining, consuming the signal for a dead operation.
• Cancelling a blocked synchronous Wait(cancellationToken) throws out of op.Event.Wait(...) and abandons the operation in WAITING in the queue (never withdrawn), which feeds both problems above.

This path is reachable through BackgroundTaskScheduler.Dispose(CancellationToken).

Fix (three parts):

1. Give each wait operation its own event instead of a shared thread‑static one. This is the root fix: Activate() cannot transition the state and set the event atomically, so an activating thread preempted between the two can deliver its Set() after the waiter has already observed SUCCESS, returned, and begun an unrelated wait. With a private event, a late Set() lands on an object nobody waits on and is harmless. Do not pool the events — pooling reintroduces the reuse and the bug. Do not dispose them either: a late Set() must not hit a disposed object. (The thread‑static was only an allocation optimisation on the blocking‑wait path, which in practice runs on dispose.)
2. Make WaitOperationSync.Activate() CAS CREATED/WAITING → SUCCESS, set the event only when it wins, and return false when it loses (the Set() drain retry loops already handle false). This stops auto‑reset from consuming a signal for a dead operation.
3. On cancellation, CAS WAITING → TIMEOUT before rethrowing, restoring the signal via Set() for auto‑reset if the CAS loses.

Minor

• Reset() has two identical branches and an unresolved // TODO: does this work correctly with Set()?. Reset() is not safe to call concurrently with Set() (a Reset() racing SetManualReset's drain aborts it) — this should be documented, not just TODO'd.
• Operations that time out / complete early stay in Operations until a future Set() drains past them (unbounded retention if Set() is never called again).

Affected areas

• PostSharp.Patterns.Common — the AwaitableEvent primitive itself.
• PostSharp.Patterns.Caching — BackgroundTaskScheduler (WhenBackgroundTasksCompleted, Dispose), i.e. distributed/caching‑backend background‑task completion and dispose.

Notes

All four defects have been fixed and verified in the Metalama.Patterns port (deterministic single‑shot repros via the ConcurrencyTestingApi trace hooks for the double‑activation, plus load tests that reproduced the lost wakeup and now pass, including under CPU saturation). The same fixes apply verbatim here.

— Claude for @gfraiteur

[gfraiteur]

Related: #44 (BackgroundTaskScheduler task-counter leak → unbounded dispose wait). Both surface as the same symptom — caching-backend dispose hanging in WhenBackgroundTasksCompleted — but they are independent root causes: #44 is the counter never reaching zero, whereas bug #1 here is the event being signalled (SignalState == SIGNALED, counter already 0) while a waiter stays stranded in WAITING because the drain never observed it. Fixing either alone still leaves the other able to hang dispose.

— Claude for @gfraiteur