[Breaking] CacheClient ensure consistent return values for expires in. by niemyjski · Pull Request #432 · FoundatioFx/Foundatio

niemyjski · 2025-12-31T21:54:22Z

This PR introduces breaking changes to ensure consistent return values and expiration behavior across all ICacheClient implementations. The primary goal is to standardize how cache methods handle expiration parameters, particularly edge cases like zero, negative, and TimeSpan.MaxValue expirations.

Key Changes:

Standardized expiration handling with consistent behavior: null removes TTL, positive sets TTL, zero/negative removes key
Enhanced DateTimeExtensions.SafeAdd methods with proper overflow protection before arithmetic operations
Added comprehensive XML documentation to the ICacheClient interface describing all methods and their expiration behavior
Renamed and expanded test coverage for expiration scenarios across all cache client implementations
Updated documentation guide with detailed TTL behavior reference tables and examples

…ent-expires-in

Introduces comprehensive caching documentation, covering the ICacheClient interface, expiration behavior, implementations, common patterns, DI, and best practices. Defines the ICacheClient interface, providing a unified API for cache operations across different implementations, complete with detailed XML documentation.

Adds comprehensive tests to verify the correct expiration behavior for various cache operations (AddAsync, IncrementAsync, ListAddAsync, ReplaceAsync, SetAllAsync, SetAsync, SetExpirationAsync, SetIfHigherAsync, SetIfLowerAsync). These tests cover edge cases like past, zero, max, and null expirations, ensuring that the cache behaves consistently and predictably across different implementations. Additionally, this change addresses precision issues in IncrementAsync, SetIfHigherAsync, and SetIfLowerAsync when dealing with floating-point decimals.

Ensures that `DateTime` and `DateTimeOffset` additions do not result in overflows, returning `MaxValue` or `MinValue` when appropriate. Also adds tests to validate the date time extension methods.

Removes conditional logic that was preventing cache entries from being consistently updated with expiration times. Now the expiration is always set regardless of whether the expiresIn value is present. Also, ensures that GetExpirationAsync and GetAllExpirationAsync correctly handle cache entries with no explicit expiration by returning null consistently. It now checks for both null and DateTime.MaxValue expiration values.

Ensures consistent behavior for setting cache expiration. When expiresIn is zero or negative, the cache key is removed. The return value is also updated to consistently return 0.

Ensures that SetIfHigherAsync and SetIfLowerAsync return 0 when the item is expired. Previously the methods returned -1 when the key had an expired timespan.

Ensures that zero or negative expiration values passed to the AddAsync method will result in removing the cache key. This prevents potential issues where invalid expiration values could lead to unexpected cache behavior.

src/Foundatio.TestHarness/Caching/CacheClientTestsBase.cs

+            Assert.Equal(3, listValue.Value.Count);
+            expiration = await cache.GetExpirationAsync("list-normal-exp");
+            Assert.NotNull(expiration);
+            Assert.InRange(expiration.Value, TimeSpan.FromMinutes(59), TimeSpan.FromHours(1));


src/Foundatio.TestHarness/Caching/CacheClientTestsBase.cs

+            Assert.Equal("new-value", (await cache.GetAsync<string>("replace-normal-exp")).Value);
+            expiration = await cache.GetExpirationAsync("replace-normal-exp");
+            Assert.NotNull(expiration);
+            Assert.InRange(expiration.Value, TimeSpan.FromMinutes(59), TimeSpan.FromHours(1));


src/Foundatio.TestHarness/Caching/CacheClientTestsBase.cs

+            Assert.True(await cache.ExistsAsync("set-expiration-datetime"));
+            expiration = await cache.GetExpirationAsync("set-expiration-datetime");
+            Assert.NotNull(expiration);
+            Assert.InRange(expiration.Value, expirationDateTime - expirationDateTime.Subtract(TimeSpan.FromSeconds(5)),


src/Foundatio.TestHarness/Caching/CacheClientTestsBase.cs

+            Assert.True(await cache.ExistsAsync("set-expiration-timespan"));
+            expiration = await cache.GetExpirationAsync("set-expiration-timespan");
+            Assert.NotNull(expiration);
+            Assert.InRange(expiration.Value, TimeSpan.FromMinutes(59), TimeSpan.FromHours(1));


src/Foundatio.TestHarness/Caching/CacheClientTestsBase.cs

Ensures consistent calculation of expiration times for cache entries by using a single `ToExpiresIn` extension method. This prevents inconsistencies arising from different calculations across various methods.

Copilot

Pull request overview

This PR introduces breaking changes to ensure consistent return values and expiration behavior across all ICacheClient implementations. The primary goal is to standardize how cache methods handle expiration parameters, particularly edge cases like zero, negative, and TimeSpan.MaxValue expirations.

Key Changes:

Standardized expiration handling with consistent behavior: null removes TTL, positive sets TTL, zero/negative removes key
Enhanced DateTimeExtensions.SafeAdd methods with proper overflow protection before arithmetic operations
Added comprehensive XML documentation to the ICacheClient interface describing all methods and their expiration behavior
Renamed and expanded test coverage for expiration scenarios across all cache client implementations
Updated documentation guide with detailed TTL behavior reference tables and examples

Reviewed changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated 10 comments.

Show a summary per file

File	Description
src/Foundatio/Extensions/DateTimeExtensions.cs	Fixed overflow detection in SafeAdd methods to check before adding instead of after, preventing integer wraparound
src/Foundatio/Caching/InMemoryCacheClient.cs	Standardized expiration handling: added validation for zero/negative expiresIn, return 0 instead of -1 for SetIfHigher/SetIfLowerAsync, consistent TTL removal with null
src/Foundatio/Caching/HybridCacheClient.cs	Added expiration validation for AddAsync to handle zero/negative values consistently
src/Foundatio/Caching/ICacheClient.cs	Added comprehensive XML documentation for all interface methods with detailed expiration behavior, parameter descriptions, and return value specifications
docs/guide/caching.md	Added extensive TTL behavior documentation with reference tables, examples, and Azure Redis eviction policy warnings
tests/Foundatio.Tests/Extensions/DateTimeExtensionsTests.cs	New test file with comprehensive coverage for SafeAdd, SafeAddMilliseconds, Floor, Ceiling, and Unix timestamp conversion methods
src/Foundatio.TestHarness/Caching/CacheClientTestsBase.cs	Renamed expiration tests for consistency, added new tests for expiration edge cases, removed obsolete SetExpirationAsync tests, added floating-point precision tests
tests/Foundatio.Tests/Caching/*Tests.cs	Updated all cache client test implementations to call renamed and new expiration test methods from base class

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

docs/guide/caching.md

src/Foundatio/Caching/ICacheClient.cs

tests/Foundatio.Tests/Caching/InMemoryCacheClientTests.cs

src/Foundatio.TestHarness/Caching/CacheClientTestsBase.cs

src/Foundatio/Caching/ICacheClient.cs

docs/guide/caching.md

Standardizes the behavior of the SetIfHigherAsync and SetIfLowerAsync methods regarding TTL handling. Previously, these methods returned -1 when a zero or negative TimeSpan was provided, leading to inconsistent behavior. Now, they consistently return 0 and remove the key, aligning with other cache methods.

Adds tests to verify that hybrid cache invalidation works correctly across multiple cache instances for increment, removeIfEqual, replaceIfEqual and set operations. This ensures data consistency in distributed caching scenarios.

Copilot

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 3 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-01-01T14:52:04Z

src/Foundatio/Caching/InMemoryCacheClient.cs

            // Skip keys without expiration (consistent with GetExpirationAsync behavior)
-            if (existingEntry.ExpiresAt.HasValue)
+            // DateTime.MaxValue means no expiration
+            if (existingEntry.ExpiresAt.HasValue && existingEntry.ExpiresAt.Value != DateTime.MaxValue)
                result[key] = existingEntry.ExpiresAt.Value.Subtract(_timeProvider.GetUtcNow().UtcDateTime);


Inconsistent behavior between GetExpirationAsync and GetAllExpirationAsync for keys without expiration. GetExpirationAsync returns null for keys without expiration, but GetAllExpirationAsync completely omits such keys from the result dictionary. This creates an inconsistency: if a key exists with no TTL, GetExpirationAsync will return null (indicating the key exists with no expiration), but GetAllExpirationAsync won't include it in the results at all.

To maintain consistency, GetAllExpirationAsync should include keys without expiration in the result dictionary with a null value, matching GetExpirationAsync's behavior.

Copilot · 2026-01-01T14:52:04Z

src/Foundatio.TestHarness/Caching/CacheClientTestsBase.cs

+            // NOTE: The expiresIn parameter on ListRemoveAsync is currently NOT implemented
+            // in InMemoryCacheClient - it is a no-op. This test documents the expected behavior
+            // that SHOULD be implemented: expiration should be applied to remaining items after removal.
+            // For now, this test verifies the remove operation works, but expiration is ignored.
+
+            // Setup: add items to list
+            const string key = "list-remove-exp";
+            Assert.Equal(3, await cache.ListAddAsync(key, [1, 2, 3]));
+            Assert.True(await cache.ExistsAsync(key));
+
+            // Past expiration: currently the expiresIn is ignored, so remove still works
+            // Expected behavior (not implemented): past expiration should remove all remaining items
+            long removed = await cache.ListRemoveAsync(key, [1], TimeSpan.FromMilliseconds(-1));
+            Assert.Equal(1, removed);
+            // Key still exists because expiresIn is not implemented
+            Assert.True(await cache.ExistsAsync(key));
+            var listValue = await cache.GetListAsync<int>(key);
+            Assert.True(listValue.HasValue);
+            Assert.Equal(2, listValue.Value.Count);
+
+            // Normal expiration: currently ignored
+            removed = await cache.ListRemoveAsync(key, [2], TimeSpan.FromHours(1));
+            Assert.Equal(1, removed);
+            Assert.True(await cache.ExistsAsync(key));
+            listValue = await cache.GetListAsync<int>(key);
+            Assert.True(listValue.HasValue);
+            Assert.Single(listValue.Value);
+            Assert.Contains(3, listValue.Value);
+
+            // Max expiration: currently ignored
+            removed = await cache.ListRemoveAsync(key, [3], TimeSpan.MaxValue);
+            Assert.Equal(1, removed);
+            // After removing all items, key should not exist
+            Assert.False(await cache.ExistsAsync(key));
+        }
+    }


The documentation comment indicates that ListRemoveAsync's expiresIn parameter should set expiration on remaining items after removal, but the implementation doesn't actually apply the expiration. While the test documents this as a known limitation, it creates a misleading API signature where the parameter appears to do something but is completely ignored. Consider either implementing the feature or removing the parameter from the signature to avoid confusion.

docs/guide/caching.md

niemyjski added 10 commits December 26, 2025 15:14

Merge branch 'main' into bugfix/ensure-consistent-expires-in

5276559

Merge remote-tracking branch 'origin/main' into bugfix/ensure-consist…

59f50ca

…ent-expires-in

Merge remote-tracking branch 'origin/main' into bugfix/ensure-consist…

3cf1421

…ent-expires-in

Guarantees safe DateTime and DateTimeOffset additions

532971d

Ensures that `DateTime` and `DateTimeOffset` additions do not result in overflows, returning `MaxValue` or `MinValue` when appropriate. Also adds tests to validate the date time extension methods.

Handles zero or negative expiresIn values

3826b1d

Ensures consistent behavior for setting cache expiration. When expiresIn is zero or negative, the cache key is removed. The return value is also updated to consistently return 0.

Corrects SetIfHigher/Lower expiry return value

e5e8068

Ensures that SetIfHigherAsync and SetIfLowerAsync return 0 when the item is expired. Previously the methods returned -1 when the key had an expired timespan.

Handles zero or negative expiration values.

d074aa0

Ensures that zero or negative expiration values passed to the AddAsync method will result in removing the cache key. This prevents potential issues where invalid expiration values could lead to unexpected cache behavior.

niemyjski requested review from Copilot and ejsmith and removed request for Copilot December 31, 2025 21:54

niemyjski self-assigned this Dec 31, 2025

niemyjski added bug enhancement labels Dec 31, 2025

niemyjski requested a review from Copilot December 31, 2025 21:58

github-code-quality bot found potential problems Dec 31, 2025

View reviewed changes

Copilot started reviewing on behalf of niemyjski December 31, 2025 21:59 View session

Consolidates expiration handling for cache client

985f4ac

Ensures consistent calculation of expiration times for cache entries by using a single `ToExpiresIn` extension method. This prevents inconsistencies arising from different calculations across various methods.

Copilot AI reviewed Dec 31, 2025

View reviewed changes

niemyjski added 2 commits January 1, 2026 08:41

Adds hybrid cache invalidation tests

817ebed

Adds tests to verify that hybrid cache invalidation works correctly across multiple cache instances for increment, removeIfEqual, replaceIfEqual and set operations. This ensures data consistency in distributed caching scenarios.

niemyjski requested a review from Copilot January 1, 2026 14:48

Copilot started reviewing on behalf of niemyjski January 1, 2026 14:49 View session

Copilot AI reviewed Jan 1, 2026

View reviewed changes

pr feedback

22f14c5

niemyjski merged commit 4edbec8 into main Jan 1, 2026
3 checks passed

niemyjski deleted the bugfix/ensure-consistent-expires-in branch January 1, 2026 15:15

niemyjski mentioned this pull request Jan 1, 2026

[Breaking] CacheClient ensure consistent return values for expires in. FoundatioFx/Foundatio.Redis#128

Merged

Uh oh!

Conversation

niemyjski commented Dec 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Jan 1, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 1, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

niemyjski commented Dec 31, 2025 •

edited

Loading