Fix synchronous cancellation race in Channels

Project: dotnet/runtime | Language: C# | Concepts: concurrency, error-handling, testing-strategy | Difficulty: senior
Link: https://github.com/dotnet/runtime/pull/132230

The problem

The AsyncOperation class in System.Threading.Channels had a race condition when cancellation occurred synchronously during registration. If cancellation happened immediately, the callback could observe an uninitialized cancellation token, leading to the operation being completed without proper atomic reservation. This allowed a reader or waiter to be completed multiple times, causing assertion failures or silent data loss in channels.

Why it's non-trivial

A naive fix might only check the cancellation token at the time of registration, but this does not account for the window where cancellation and registration overlap. The cancellation callback could run before the registration is fully assigned, leading to inconsistent state and races. Fixing this requires careful ordering and immutable state to guarantee correctness even under concurrent or synchronous cancellation.

The approach

1. Introduce an immutable _isCancelable field

+        /// <summary>true if this operation was created with a cancelable token; otherwise, false.</summary>
+        private readonly bool _isCancelable;

Annotation:
A new boolean field _isCancelable is added and initialized before any registration or callback can run. This ensures that the "cancelable" status is available and immutable for the lifetime of the operation, avoiding races on token state.

2. Set _isCancelable before registration and use it for logic

-            Debug.Assert(!pooled || !cancellationToken.CanBeCanceled);
+            _isCancelable = cancellationToken.CanBeCanceled;
+            Debug.Assert(!pooled || !_isCancelable);

Annotation:
The _isCancelable field is set as the very first step in the constructor, before any other logic or registration. This guarantees that even if cancellation occurs synchronously, the callback will see the correct value.

3. Use _isCancelable instead of checking the token at runtime

-            if (cancellationToken.CanBeCanceled)
+            if (_isCancelable)

Annotation:
All logic that previously checked cancellationToken.CanBeCanceled is switched to use the precomputed _isCancelable. This removes any ambiguity or race about the token's state.

4. Update reservation and completion logic to use _isCancelable

-        public bool TryReserveCompletionIfCancelable() =>
-            !CancellationToken.CanBeCanceled ||
+        public bool TryReserveCompletionIfCancelable() =>
+            !_isCancelable ||

Annotation:
The reservation logic now uses the immutable field, ensuring correct atomic reservation even if cancellation is synchronous.

5. Fix linked-list removal race in ChannelUtilities.Remove

+            // If the list has already been detached, the operation's links may be cleared concurrently.
+            if (head is null)
+            {
+                return;
+            }
+
             Debug.Assert(op.Next is null == op.Previous is null);
 
-            // If the operation is known to not be in the list referenced by head, avoid further manipulating the instance.
-            if (head is null || op.Next is null)
+            // If the operation is known to not be in the list referenced by head, avoid further manipulating the instance.
+            if (op.Next is null)
             {
                 return;
             }

Annotation:
This change ensures that if the list head is already null (detached), the method returns early, avoiding races where the operation's links could be cleared concurrently.

6. Add deterministic test coverage for the race

+#if NET
+        [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsNotMonoRuntime))]
+        public async Task AsyncOperation_SynchronousCancellationDuringRegistration_ReservesCompletion()
+        {
+            using var cts = new CancellationTokenSource();
+            cts.Cancel();
+
+            object operation = AsyncOperationAccessors<int>.CreateBlockedReadAsyncOperation(
+                runContinuationsAsynchronously: true,
+                cts.Token,
+                pooled: false,
+                static (state, token) => Assert.True(AsyncOperationAccessors<int>.TrySetCanceled(state, token)));
+
+            var valueTask = new ValueTask<int>((IValueTaskSource<int>)operation, token: 0);
+            await AssertExtensions.CanceledAsync(cts.Token, async () => await valueTask);
+            Assert.False(AsyncOperationAccessors<int>.TryReserveCompletionIfCancelable(operation));
+        }
+#endif

Annotation:
A new test ensures that when cancellation occurs synchronously during registration, the operation correctly reserves completion, preventing double-completion or data loss.

From the review thread

PranavSenthilnathan:
The _isCancelable fix closes one path, but #129796’s exact assertion remains reachable independently.

Cancellation can queue removal while an operation is still linked. TryComplete then sets the live head to null under lock and clears the detached node’s Previous and Next outside the lock. Remove may observe those fields between writes because it asserts before checking the null head.

Since head == null means the operation no longer belongs to the live list, the fix is simply:

if (head is null)
{
    return;
}

Debug.Assert(op.Next is null == op.Previous is null);

I suggest including this fix in this PR because it closes #129796; otherwise the same Known Build Error remains possible. It does not change release behavior.

PranavSenthilnathan:
We should backport this. The assertion failure is benign, but the cancellation issue (#132094) is worth fixing. When this issue happens, a writer thinks it successfully sent and item while the reader thinks it was successfully cancelled so the item is lost entirely.

/cc @jeffhandley any concerns about the risk? It's a small straightforward change that prevents the race. There is a boolean field addition which increases the object size very slightly, but I think we can accept that.

Takeaways

• Immutable state should be established before any concurrent or asynchronous action can observe it. This prevents races where callbacks or other threads see partially-initialized objects.
• Guard against concurrent list mutations by checking for detachment before manipulating links. This avoids assertion failures and inconsistent state when elements can be removed concurrently.
• Test for races by forcing synchronous execution of cancellation or callbacks. Deterministic tests that simulate the race window are essential for validating concurrency fixes.