Repository navigation
Fix HybridCache wildcard invalidation timestamp under frozen TimeProvider (#7849) - #7908
Manish-1kumar wants to merge 2 commits into
Conversation
🎉 Good job! The coverage increased 🎉
Full code coverage report: https://dev.azure.com/dnceng-public/public/_build/results?buildId=1660958&view=codecoverage-tab |
There was a problem hiding this comment.
🟡 Changes recommended
The regression tests cover only throwing backend reads, leaving the malformed nonempty invalidation-payload fallback unverified; add a frozen-clock test confirming that a newly set entry remains in L1.
1 open issue
What changed in this PR
This PR fixes HybridCache invalidation fallback timestamps so newly written L1 entries remain valid when the clock is frozen and the distributed cache is unavailable, addressing #7849.
Changes:
- Adjusts failed and malformed invalidation-read fallbacks to one tick before the current time, clamped to zero.
- Adds frozen-clock regression tests for retaining tagged and untagged entries with an offline distributed cache.
| File | Description |
|---|---|
| test/Libraries/Microsoft.Extensions.Caching.Hybrid.Tests/UnreliableL2Tests.cs | Adds frozen-clock retention tests and an offline cache stub. |
| src/Libraries/Microsoft.Extensions.Caching.Hybrid/Internal/DefaultHybridCache.L2.cs | Adjusts both invalidation-read fallback timestamps. |
Review effort: Balanced
| { | ||
| // not what we expected! assume invalid | ||
| timestamp = CurrentTimestamp(); | ||
| timestamp = Math.Max(0L, CurrentTimestamp() - 1); |
There was a problem hiding this comment.
I agree with Copilot, can you, please, add test for the unexpected buffer size case?
| { | ||
| // not what we expected! assume invalid | ||
| timestamp = CurrentTimestamp(); | ||
| timestamp = Math.Max(0L, CurrentTimestamp() - 1); |
There was a problem hiding this comment.
I think it would be a bit better if both changes were just CurrentTimestamp() - 1. That would mean that even when time is 0, we don't incorrectly invalidate current entries.
(We don't have to worry about overflow, since CurrentTimestamp() is always >= 0.)
| // Wait briefly for the background wildcard invalidation read task to complete | ||
| await Task.Delay(100); |
There was a problem hiding this comment.
Fixed delays (here and in the other test) should generally be avoided, they lead to flaky tests.
In this case, I think they can be removed, since the cache used is actually synchronous.
|
@Manish-1kumar Thank you for the PR. I have a couple of suggestions. |
Description
When
DefaultHybridCacheis initialized with anIDistributedCachebackend, it asynchronously prefetches the global wildcard invalidation timestamp (SafeReadTagInvalidationAsync(TagSet.WildcardTag)). If the backend distributed cache is offline or throws on startup,SafeReadTagInvalidationAsynccatches the failure and falls back toCurrentTimestamp().In unit and integration tests using a frozen$T_0$ ). When an application subsequently invokes $T_0 \le T_0$ ), which evaluates to
FakeTimeProvider,_globalInvalidateTimestampis initialized to the current time tick (SetAsync, the item is stored withCreationTimestamp = T_0. When the item is subsequently looked up viaGetOrCreateAsync,IsWildcardExpired(timestamp)comparestimestamp <= _globalInvalidateTimestamp.Result(true, immediately expiring and discarding the newly written entry and triggering a cache miss.This fix adjusts
SafeReadTagInvalidationAsyncon the error and unexpected length fallback paths to returnMath.Max(0L, CurrentTimestamp() - 1). Any cache entries created strictly prior to the failed read are still invalidated as intended, but entries written at or after that tick remain valid, decoupling behavior from clock resolution.Fixes #7849
Customer Impact
Consumers running integration tests with
FakeTimeProviderand disconnected or unavailable distributed cache backends (or fail-fast Redis configurations) will not have newly written L1 entries immediately discarded by wildcard expiration.Regression?
No.
Risk
Extremely low. Minimal two-line adjustment to fallback timestamp calculation on the failure paths of
SafeReadTagInvalidationAsync.Testing
OfflineL2_WithFrozenClock_RetainsSetEntriesandOfflineL2_WithFrozenClock_RetainsTaggedSetEntriestoUnreliableL2Tests.cs(demonstrating failure before fix, passing after fix).dotnet build src/Libraries/Microsoft.Extensions.Caching.Hybrid/Microsoft.Extensions.Caching.Hybrid.csproj -c Release(0 Warnings, 0 Errors across all 6 TFMs).dotnet test test/Libraries/Microsoft.Extensions.Caching.Hybrid.Tests/Microsoft.Extensions.Caching.Hybrid.Tests.csproj -c Release(284/284 passed on net10.0, 284/284 on net9.0, 279/279 on net8.0).git diff --checkpassed with 0 errors.Microsoft Reviewers: Open in CodeFlow