Revert "Make MemoryCache thread-safety tests deterministic"

[MichalStrehovsky]

Resolves#130139

Reverts #129897

[dotnet-policy-service]

Tagging subscribers to this area: @dotnet/area-extensions-caching
See info in area-owners.md if you want to be subscribed.

[Copilot]

Pull request overview

This PR reverts prior changes that attempted to make MemoryCache thread-safety tests deterministic, in response to a reported CI failure. The net effect is to restore time-based/concurrency-loop style tests and add ActiveIssue skips for the failing scenarios.

Changes:

• Reworks several thread-safety tests from barrier + fixed-iteration loops back to cancellation/time-based loops.
• Adds ActiveIssue(72890) to additional tests to disable them while the issue is investigated.
• Includes a few incidental formatting changes in the test file.

[github-actions]

Copilot Code Review

Holistic Assessment

Motivation: PR #129897 ("Make MemoryCache thread-safety tests deterministic") introduced a CI-breaking failure in OvercapacityPurge_AreThreadSafe (tracked as #130139). Reverting to unblock CI is justified and appropriate.

Approach: This is a clean git revert — the diff is the exact inverse of #129897 (111 additions / 60 deletions vs. 60 / 111). All three affected test methods are disabled via [ActiveIssue("...72890")], which prevents CI flakiness from the known pre-existing test bugs. No production code is changed.

Summary: ✅ LGTM. A straightforward, clean revert that restores the previous known state with flaky tests properly disabled. No concerns — the pre-existing test issues (Random shared across threads, cts.Token causing TaskCanceledException, volatile-less cross-thread flags) are all tracked by #72890 and gated behind [ActiveIssue].

---

Detailed Findings

Detailed Findings

✅ Revert completeness — Clean inverse of #129897

Verified the diff is the exact inverse of PR #129897. Only one file is touched (MemoryCacheSetAndRemoveTests.cs), and the additions/deletions match exactly. All three test methods (GetAndSet_AreThreadSafe_AndUpdatesNeverLeavesNullValues, OvercapacityPurge_AreThreadSafe, AddAndReplaceEntries_AreThreadSafe) are restored to their pre-#129897 state.

✅ Tests correctly disabled — [ActiveIssue] annotations

All three reverted tests have [ActiveIssue("https://github.com/dotnet/runtime/issues/72890")], ensuring they are skipped in CI. GetAndSet_AreThreadSafe_AndUpdatesNeverLeavesNullValues additionally retains its original [ActiveIssue("...72879")] for the underlying cache bug. This matches the state before #129897.

✅ No production code impact

The revert is limited to test code. No changes to MemoryCache or any shipping library code.

💡 Pre-existing formatting quirks restored (no action needed)

The revert restores a few minor formatting oddities that existed before #129897 (e.g., => {} without space, null,out without space after comma, misaligned ;} on line 788). These are pre-existing and not introduced by this PR. They can be cleaned up in a future PR if desired.

Note

This review was created by GitHub Copilot.

Generated by Code Review for issue #130152 · ● 14M · ◷

[sami-daniel]

Thanks for the revert.

Sorry about the breakage.

I took another look at #130139, and I think I might have found what's causing the failure. I don't think it's related to the concurrency rewrite itself, but to the final assertion, AssertCacheSize(cache.Count, cache).

AssertCacheSize captures cache.Count once and then retries against cache.Size, while overcapacity compaction can still be running on a background ThreadPool thread after Task.WaitAll returns. That leaves a small window where Count and Size can be temporarily out of sync.

Do you happen to know where the original failure showed up in the tree? Was it on a particular pipeline? If I can reproduce the same environment locally, I might be able to confirm the theory....

I couldn't reproduce it locally, so it still looks timing dependent. I'll put together a follow-up PR to re land the #129897 by updating the assertion once this revert merges and I've verified the fix.

[MichalStrehovsky]

Sorry about the breakage.

No worries, this happens.

Do you happen to know where the original failure showed up in the tree? Was it on a particular pipeline? If I can reproduce the same environment locally, I might be able to confirm the theory....

#130139 has the legs where automation saw this. Linux arm32, Windows x86, ReadyToRun Windows x64. .NET Core and .NET Framework alike. Doesn't look to be environment specific, just non-deterministic. For these kinds of issues one has to run the entire test locally for hundreds of iterations and even that doesn't guarantee success. Make sure to run with -c Release.