Fix synchronous cancellation race in Channels

[steveisok]

Fixes#129796
Fixes#132094

AsyncOperation used _cancellationRegistration.Token to determine whether completion needed atomic reservation. If cancellation ran synchronously inside UnsafeRegister, the registration had not yet been assigned, so the callback observed a default non-cancelable token and completed without reserving the operation.

A reader or waiter could then be reserved and completed a second time. This caused linked-list assertion failures in RendezvousChannel and could silently lose items during a BoundedChannel direct handoff.

Use an immutable _isCancelable value initialized before registration can invoke the callback. This preserves the non-cancelable fast path, pooling behavior, and registration cleanup without retaining the full CancellationToken on modern .NET.

Adds deterministic coverage for:

• synchronous cancellation during registration
• skipping a canceled bounded-channel reader during direct handoff
• delayed rendezvous waiter removal after the list has been drained

The added field fits existing object padding; BlockedReadAsyncOperation<int> remains 96 bytes.

Validation

• System.Threading.Channels Debug tests: 1,599 passed, 2 existing stress tests skipped
• Debug and Release library builds: succeeded with no warnings
• Release 3,000,000-item repro: 3,000,000 written and read, no missing items

Note

This pull request description was generated with the assistance of GitHub Copilot.

[azure-pipelines]

Azure Pipelines:
Successfully started running 3 pipeline(s).
13 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

[Copilot]

Pull request overview

This PR fixes a race in System.Threading.Channels async operations where synchronous cancellation during token registration could cause completion to proceed without an atomic reservation, enabling double-completion and corrupting waiter/reader linked lists (with downstream assertion failures and potential item loss).

Changes:

• Make cancelability an immutable construction-time property (_isCancelable) and use it for completion reservation decisions, avoiding reliance on _cancellationRegistration.Token when registration may be a default value.
• Add test infrastructure to construct synchronously-canceled internal channel async operations and inspect/modify their linked-list state.
• Add deterministic regression tests for rendezvous waiter removal and bounded-channel direct handoff skipping canceled readers.

Show a summary per file

File	Description
src/libraries/System.Threading.Channels/src/System/Threading/Channels/AsyncOperation.cs	Introduces _isCancelable and switches completion reservation/assert logic to use it, addressing the synchronous-cancellation registration race.
src/libraries/System.Threading.Channels/tests/TestBase.cs	Adds reflection-based helpers to create/cancel internal async operations and manipulate their list links/heads for deterministic regression coverage.
src/libraries/System.Threading.Channels/tests/RendezvousChannelTests.cs	Adds a regression test validating delayed removal of a synchronously-canceled waiter doesn’t corrupt the waiting-reader list.
src/libraries/System.Threading.Channels/tests/BoundedChannelTests.cs	Adds regression tests for synchronous cancellation during registration and for skipping canceled readers during direct handoff.

Review details

• Files reviewed: 4/4 changed files
• Comments generated: 1
• Review effort level: Lite

[jkotas]

/azp run runtime-nativeaot-outerloop

[azure-pipelines]

Azure Pipelines:
Successfully started running 1 pipeline(s).

[tannergooding]

The change LGTM, but Jan's test feedback is applicable still

[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(headisnull){return;}Debug.Assert(op.Nextisnull==op.Previousisnull);

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.

Note

GitHub Copilot helped draft this comment.

[Copilot]

Review details

• Files reviewed: 2/2 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

Review details

Suppressed comments (1)

src/libraries/System.Threading.Channels/tests/BoundedChannelTests.cs:541

• The PR description claims deterministic coverage was added for additional scenarios (skipping a canceled bounded-channel reader during direct handoff, and delayed rendezvous waiter removal after the list has been drained), but the diff here only adds the synchronous-cancellation-during-registration test. Either add the missing tests (likely in BoundedChannelTests/RendezvousChannelTests) or update the PR description so it accurately reflects the changes in this PR.

#if NET
[Fact]
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));
}

• Files reviewed: 3/3 changed files
• Comments generated: 0 new
• Review effort level: Lite

[steveisok]

@PranavSenthilnathan can I get another review on this?

[PranavSenthilnathan]

The mono/wasm tests failures look valid: System.MissingMethodException : Could not find constructor. That test should probably be excluded from mono.

[Copilot]

Review details

• Files reviewed: 3/3 changed files
• Comments generated: 0 new
• Review effort level: Lite

[steveisok]

/backport to release/11.0-rc1

[github-actions]

Started backporting to release/11.0-rc1 (link to workflow run)