Fix ConcurrentQueue.TryDequeue spuriously reporting empty during a segment freeze

[kafka1991]

Fixes #132736

ConcurrentQueueSegment.EnsureFrozenForEnqueues published _frozenForEnqueues = true before bumping the Tail by FreezeOffset, while TryDequeue's empty check reads the flag first and the Tail second. In the window between the two stores a dequeuer can pair frozen == true with the pre-freeze Tail and subtract FreezeOffset from a Tail that was never bumped. A segment holds fewer than FreezeOffset items, so currentTail - FreezeOffset - currentHead <= 0 is then always true and TryDequeue reports the segment empty while it still holds committed items — a false return with no moment during the call at which the queue was empty. The Interlocked.Add full fence does not close the window; it only guarantees the stores become visible in exactly this order, and the freezing thread can stall between them (see the issue for the full interleaving and a reproducer that hits ~100 false-empties per 128M operations per round on current bits).

The fix bumps the Tail before publishing the flag. The reader's three possible pairings become:

1. frozen == true — the bump is necessarily visible, so the Tail read afterwards includes FreezeOffset and the frozen clause reports empty only when the segment is genuinely drained.
2. frozen == false with a bumped Tail (freeze landed between the two reads) — currentTail - currentHead is a large positive value, so the check reports "not empty", spins, and retries; the next iteration takes pairing 1. This is the bounded benign retry the existing comment in TryDequeue already describes.
3. frozen == false with an un-bumped Tail — pre-freeze fast path, unchanged.

Enqueuers never read the flag (a bumped Tail fails their sequence check and routes them to EnqueueSlow, unchanged), and EnsureFrozenForEnqueues only runs under the cross-segment lock, so the if (!_frozenForEnqueues) guard is unaffected by the reordering.

[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.

[VSadov]

nothing seems to guarantee that frozen is read before Tail in the following:

bool frozen = _frozenForEnqueues;                       // reads flag first
long currentTail = Volatile.Read(ref _headAndTail.Tail); // reads tail second

can there still be a problem for the ordering that we want, if frozen is read after Tail?

[VSadov]

I think we want to move the Volatile.Read from reading the Tail to the read of the frozen.
There is nothing that the read of Tail needs to be before - there are no more interesting reads/writes in this iteration of the loop and opaque SpinOnce will prevent any kind of out of loop hoisting.
However, it seems important for this fix that frozen needs to be read before the Tail

If this makes sense, the same would need to be applied to TryPeek. It is basically the same code.

[VSadov]

can a regression test be added for this?
how long the repro in the bug needs to run to hit a failure?

Maybe run the repro for 1 second as a test?
It does not need to fail always, just needs to fail once in a while.

[kafka1991]

@dotnet-policy-service agree

[AlanLiu90]

@VSadov

Hi, we encountered the same issue in production last Sunday, and I came up with a different fix.

My understanding is that currentTail - currentHead can never be negative, so I think changing
currentTail - FreezeOffset - currentHead <= 0
to
currentTail - FreezeOffset - currentHead == 0
may be sufficient.

I have my implementation here.

Could you take a look and see whether this approach would be preferable, or whether there is a case I'm missing where the implementation in this PR is necessary?

[kafka1991]

here

@VSadov

Hi, we encountered the same issue in production last Sunday, and I came up with a different fix.

My understanding is that currentTail - currentHead can never be negative, so I think changing currentTail - FreezeOffset - currentHead <= 0 to currentTail - FreezeOffset - currentHead == 0 may be sufficient.

I have my implementation here.

Could you take a look and see whether this approach would be preferable, or whether there is a case I'm missing where the implementation in this PR is necessary?

Your change would likely avoid the false-empty result, but the reasoning is quite implicit (at least for me).

I prefer fixing the publication protocol directly. frozen == true should guarantee that Tail already includes FreezeOffset.

[VSadov]

here

@VSadov
Hi, we encountered the same issue in production last Sunday, and I came up with a different fix.
My understanding is that currentTail - currentHead can never be negative, so I think changing currentTail - FreezeOffset - currentHead <= 0 to currentTail - FreezeOffset - currentHead == 0 may be sufficient.
I have my implementation here.
Could you take a look and see whether this approach would be preferable, or whether there is a case I'm missing where the implementation in this PR is necessary?

Your change would likely avoid the false-empty result, but the reasoning is quite implicit (at least for me).

I prefer fixing the publication protocol directly. frozen == true should guarantee that Tail already includes FreezeOffset.

I think the solution with == is slightly more preferable:

• it removes dependency on the order of reading _frozenForEnqueues and relies on another invariant that is there regardless. Maybe it was the original assumption that order does not matter, but then <= unintentionally introduced the dependency.
  Ordering dependencies are harder to reason about. If fewer is possible, it would be better.
• we can make read of Tail with just ordinary reads (not volatile) in the TryDequeue/TryPeek
• we would not need any changes in EnsureFrozenForEnqueues

@AlanLiu90 - can you make diff comments to this PR with exact proposed changes? - so that we could look at exact changes and have an option to accept, while keeping the rest of the changes.

My understanding is that currentTail - currentHead can never be negative

It would be useful to add an assert somewhere, if there is a convenient place near the places where we will rely on the invariant.

[AlanLiu90]

@VSadov
Hi, we encountered the same issue in production last Sunday, and I came up with a different fix.
My understanding is that currentTail - currentHead can never be negative, so I think changing currentTail - FreezeOffset - currentHead <= 0 to currentTail - FreezeOffset - currentHead == 0 may be sufficient.
I have my implementation here.
Could you take a look and see whether this approach would be preferable, or whether there is a case I'm missing where the implementation in this PR is necessary?

Your change would likely avoid the false-empty result, but the reasoning is quite implicit (at least for me).
I prefer fixing the publication protocol directly. frozen == true should guarantee that Tail already includes FreezeOffset.

I think the solution with == is slightly more preferable:

• it removes dependency on the order of reading _frozenForEnqueues and relies on another invariant that is there regardless. Maybe it was the original assumption that order does not matter, but then <= unintentionally introduced the dependency.
  Ordering dependencies are harder to reason about. If fewer is possible, it would be better.
• we can make read of Tail with just ordinary reads (not volatile) in the TryDequeue/TryPeek
• we would not need any changes in EnsureFrozenForEnqueues

@AlanLiu90 - can you make diff comments to this PR with exact proposed changes? - so that we could look at exact changes and have an option to accept, while keeping the rest of the changes.

My understanding is that currentTail - currentHead can never be negative

It would be useful to add an assert somewhere, if there is a convenient place near the places where we will rely on the invariant.

I did some additional testing today and found a race that my proposed fix doesn't cover:

1. Thread-1 is inside TryDequeue, in the diff < 0 branch, and has not yet read _frozenForEnqueues
2. Other threads enqueue and dequeue multiple items, so both Head and Tail advance. Assume Tail advances to a value 64 (FreezeOffset) greater than the Head that Thread-1 read, and the segment becomes full
3. Thread-2 tries to enqueue a new item, fails because the segment is full, and calls EnsureFrozenForEnqueues, which sets _frozenForEnqueues to true. It has not yet executed Interlocked.Add(ref _headAndTail.Tail, FreezeOffset)
4. Thread-1 reads _frozenForEnqueues and Tail. Since frozen && currentTail - FreezeOffset - currentHead == 0 is true, it returns false even though the segment still contains items

Apologies for the back and forth — @kafka1991's approach looks like the safer one to me. @VSadov Let me know if you'd still like the == change as diff suggestions on top of it.

[VSadov]

@VSadov
Hi, we encountered the same issue in production last Sunday, and I came up with a different fix.
My understanding is that currentTail - currentHead can never be negative, so I think changing currentTail - FreezeOffset - currentHead <= 0 to currentTail - FreezeOffset - currentHead == 0 may be sufficient.
I have my implementation here.
Could you take a look and see whether this approach would be preferable, or whether there is a case I'm missing where the implementation in this PR is necessary?

Your change would likely avoid the false-empty result, but the reasoning is quite implicit (at least for me).
I prefer fixing the publication protocol directly. frozen == true should guarantee that Tail already includes FreezeOffset.

I think the solution with == is slightly more preferable:

• it removes dependency on the order of reading _frozenForEnqueues and relies on another invariant that is there regardless. Maybe it was the original assumption that order does not matter, but then <= unintentionally introduced the dependency.
  Ordering dependencies are harder to reason about. If fewer is possible, it would be better.
• we can make read of Tail with just ordinary reads (not volatile) in the TryDequeue/TryPeek
• we would not need any changes in EnsureFrozenForEnqueues

@AlanLiu90 - can you make diff comments to this PR with exact proposed changes? - so that we could look at exact changes and have an option to accept, while keeping the rest of the changes.

My understanding is that currentTail - currentHead can never be negative

It would be useful to add an assert somewhere, if there is a convenient place near the places where we will rely on the invariant.

I did some additional testing today and found a race that my proposed fix doesn't cover:

1. Thread-1 is inside TryDequeue, in the diff < 0 branch, and has not yet read _frozenForEnqueues
2. Other threads enqueue and dequeue multiple items, so both Head and Tail advance. Assume Tail advances to a value 64 (FreezeOffset) greater than the Head that Thread-1 read, and the segment becomes full
3. Thread-2 tries to enqueue a new item, fails because the segment is full, and calls EnsureFrozenForEnqueues, which sets _frozenForEnqueues to true. It has not yet executed Interlocked.Add(ref _headAndTail.Tail, FreezeOffset)
4. Thread-1 reads _frozenForEnqueues and Tail. Since frozen && currentTail - FreezeOffset - currentHead == 0 is true, it returns false even though the segment still contains items

Apologies for the back and forth — @kafka1991's approach looks like the safer one to me. @VSadov Let me know if you'd still like the == change as diff suggestions on top of it.

Right. A stale Head can be EnsureFrozenForEnqueues behind the Tail. Somehow I assumed that this case will self-recover, but it can indeed return "no items".

We will go with the original fix then. Thanks everybody!

[VSadov]

/ba-g Test failures are unrelated (Cryptography, Quic, Http).

[snakefoot]

Should backport be considered for NET11 and NET10 ?

[VSadov]

Should backport be considered for NET11 and NET10 ?

yes. I'll backport this to both net11 and 10

[VSadov]

/backport to release/11.0

[github-actions]

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

[VSadov]

/backport to release/10.0

[github-actions]

Started backporting to release/10.0 (link to workflow run)

[github-actions]

@VSadov backporting to release/10.0 failed, the patch most likely resulted in conflicts. Please backport manually!

git am output

$ git cherry-pick 4ffd33b300c8dc20b930fc8f538d169d5698fdb4

Auto-merging src/libraries/System.Collections.Concurrent/tests/ConcurrentQueueTests.cs
CONFLICT (content): Merge conflict in src/libraries/System.Collections.Concurrent/tests/ConcurrentQueueTests.cs
error: could not apply 4ffd33b300c... Fix ConcurrentQueue.TryDequeue spuriously reporting empty during a segment freeze (#132737)
hint: After resolving the conflicts, mark them with
hint: "git add/rm <pathspec>", then run
hint: "git cherry-pick --continue".
hint: You can instead skip this commit with "git cherry-pick --skip".
hint: To abort and get back to the state before "git cherry-pick",
hint: run "git cherry-pick --abort".
hint: Disable this message with "git config set advice.mergeConflict false"


$ git am --3way --empty=keep --ignore-whitespace --keep-non-patch changes.patch

Applying: Fix ConcurrentQueue freeze/empty-check race
Applying: Order the frozen flag read before the Tail read
Using index info to reconstruct a base tree...
M	src/libraries/System.Collections.Concurrent/tests/ConcurrentQueueTests.cs
Falling back to patching base and 3-way merge...
Auto-merging src/libraries/System.Collections.Concurrent/tests/ConcurrentQueueTests.cs
CONFLICT (content): Merge conflict in src/libraries/System.Collections.Concurrent/tests/ConcurrentQueueTests.cs
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config set advice.mergeConflict false"
Patch failed at 0002 Order the frozen flag read before the Tail read
Error: The process '/usr/bin/git' failed with exit code 128

Link to workflow output

[AlanLiu90]

@VSadov Would you consider backporting this fix to .NET 8 as well? We're currently running .NET 8 in production, so having the fix included in a future .NET 8 release would be very helpful.

[snakefoot]

@VSadov /backport to release/10.0

Seems merge is failing for ConcurrentQueueTests.cs in the NET10 backport:

Auto-merging src/libraries/System.Collections.Concurrent/tests/ConcurrentQueueTests.cs
CONFLICT (content): Merge conflict in src/libraries/System.Collections.Concurrent/tests/ConcurrentQueueTests.cs