Remove unnecessary cache freshness check#65659
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the OutputCaching middleware to allow serving cached entries even when the computed entry age is zero (same tick) or would otherwise be non-positive, addressing flakiness in timing-sensitive scenarios.
Changes:
- Removed the
CachedEntryAge <= TimeSpan.Zerofreshness invalidation check when serving cached responses. - Added tests covering scenarios where response time is equal to, after, or before the cache entry creation time.
- Retired the EventId 11 debug log message and removed the corresponding test helper constant.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/Middleware/OutputCaching/test/TestUtils.cs | Removes the LoggedMessage entry for the retired EventId 11 log. |
| src/Middleware/OutputCaching/test/OutputCacheMiddlewareTests.cs | Adds regression tests for cache serving when response time equals/comes after/comes before creation time. |
| src/Middleware/OutputCaching/src/OutputCacheMiddleware.cs | Removes the non-positive-age freshness failure path when serving cached responses. |
| src/Middleware/OutputCaching/src/LoggerExtensions.cs | Removes the EventId 11 logger message and leaves a note that the ID is retired. |
| default); | ||
| } | ||
|
|
||
| timeProvider.AdjustTime(timeProvider.GetUtcNow() - TimeSpan.FromSeconds(5)); |
There was a problem hiding this comment.
FakeTimeProvider from Microsoft.Extensions.Time.Testing is used elsewhere in this repo with SetUtcNow(DateTimeOffset)/Advance(TimeSpan); there doesn’t appear to be an AdjustTime(...) API (no other call sites in the repo). This line will likely fail to compile. Consider using timeProvider.SetUtcNow(timeProvider.GetUtcNow() - TimeSpan.FromSeconds(5)) (or similar) to move the clock backwards for this test.
| timeProvider.AdjustTime(timeProvider.GetUtcNow() - TimeSpan.FromSeconds(5)); | |
| timeProvider.SetUtcNow(timeProvider.GetUtcNow() - TimeSpan.FromSeconds(5)); |
Output caching tests could fail due to the second request happening too fast (the same tick) and failing the
timeDifference <= TimeSpan.Zerocheck. If a request is processed at the same time as a cache entry is added, then it should be able to use the cached entry. Similarly, if we change our time calculation in the future to occur earlier in the request processing pipeline and compare with the cached entry age it could be negative and would fail the previously mentioned check. Again, I don't see a reason we can't serve the cached response in that case.