From 87f10ad6cd8664a000acff85cc16693acf952e67 Mon Sep 17 00:00:00 2001 From: Matthew DeVenny Date: Mon, 6 Jul 2026 09:33:20 -0700 Subject: [PATCH 1/4] #49 Address unserializable entries Signed-off-by: Matthew DeVenny --- README.md | 14 +++++ .../CacheEntryBinarySerializer.cs | 8 ++- src/NatsDistributedCache/NatsCache.Log.cs | 7 +++ src/NatsDistributedCache/NatsCache.cs | 9 +++ .../Cache/UndeserializableEntryTests.cs | 63 +++++++++++++++++++ .../CacheEntryBinarySerializerTests.cs | 47 ++++++++++++++ 6 files changed, 147 insertions(+), 1 deletion(-) create mode 100644 test/IntegrationTests/Cache/UndeserializableEntryTests.cs diff --git a/README.md b/README.md index 0c8761e..cc07377 100644 --- a/README.md +++ b/README.md @@ -171,6 +171,20 @@ services.AddSingleton(new FakeTimeProvider()); services.AddNatsDistributedCache(options => options.BucketName = "cache"); ``` +## Cache Entry Format and Upgrades + +Cache entries are stored in a compact binary envelope. When an entry cannot be deserialized — because +it was written by an incompatible release (for example a pre-binary version that used a JSON envelope) +or is otherwise corrupt — the read is treated as a **cache miss** rather than an error, and logged at +`Debug`. Because a cache's source of truth lives elsewhere, no manual migration is required: + +- Entries with a TTL are reaped automatically by NATS once they expire. +- Entries without a TTL are left in place and re-populated the next time the key is written (a `Set` + overwrites the stored bytes unconditionally), which happens naturally under cache-aside usage. + +Upgrading is therefore seamless in a rolling deployment: a node never deletes an entry it cannot read, +so it cannot discard entries written by a newer node still being rolled out. + ## Additional Resources * [ASP.NET Core Hybrid Cache Documentation](https://learn.microsoft.com/en-us/aspnet/core/performance/caching/hybrid?view=aspnetcore-10.0) diff --git a/src/NatsDistributedCache/CacheEntryBinarySerializer.cs b/src/NatsDistributedCache/CacheEntryBinarySerializer.cs index d7f526b..4e6d4d4 100644 --- a/src/NatsDistributedCache/CacheEntryBinarySerializer.cs +++ b/src/NatsDistributedCache/CacheEntryBinarySerializer.cs @@ -113,8 +113,14 @@ public void Serialize(IBufferWriter bufferWriter, CacheEntry value) long? slidingExpirationTicks = null; if ((flags & HasSlidingExpiration) != 0) { - if (!reader.TryReadLittleEndian(out long slidingTicks)) + if (!reader.TryReadLittleEndian(out long slidingTicks) || + slidingTicks <= 0 || + slidingTicks > DateTimeOffset.MaxValue.Ticks) { + // Missing, non-positive, or absurdly large sliding ticks (corrupt entry): fail closed + // to a miss, mirroring the absolute-ticks bounds check above. A valid sliding window + // is always a positive TimeSpan, and no legitimate window approaches the full + // DateTimeOffset tick range, so anything outside (0, MaxValue] is treated as corrupt. return null; } diff --git a/src/NatsDistributedCache/NatsCache.Log.cs b/src/NatsDistributedCache/NatsCache.Log.cs index 19c51a0..cd27cb0 100644 --- a/src/NatsDistributedCache/NatsCache.Log.cs +++ b/src/NatsDistributedCache/NatsCache.Log.cs @@ -13,9 +13,16 @@ private void LogException(Exception exception) => private void LogSwallowedException(Exception exception) => _logger.LogWarning(EventIds.Exception, exception, "NATS cache read failed in TryGetAsync; returning a cache miss"); + private void LogUndeserializableEntry(string key) => + _logger.LogDebug( + EventIds.UndeserializableEntry, + "Cache entry for key {key} could not be deserialized (legacy or corrupt format); returning a cache miss", + key); + private static class EventIds { public static readonly EventId Connected = new(100, nameof(Connected)); public static readonly EventId Exception = new(101, nameof(Exception)); + public static readonly EventId UndeserializableEntry = new(102, nameof(UndeserializableEntry)); } } diff --git a/src/NatsDistributedCache/NatsCache.cs b/src/NatsDistributedCache/NatsCache.cs index a37d187..cd907a9 100644 --- a/src/NatsDistributedCache/NatsCache.cs +++ b/src/NatsDistributedCache/NatsCache.cs @@ -363,6 +363,15 @@ private Lazy> CreateLazyKvStore() => var kvEntry = natsResult.Value; if (kvEntry.Value == null) { + // Present entry whose bytes we cannot deserialize: a legacy JSON envelope from a + // pre-binary release, or genuine corruption. Intended behavior is to treat it as a + // cache miss and leave the entry in place. It self-heals when the key is next written + // (Set overwrites unconditionally), and any TTL'd entry is reaped by NATS. We + // deliberately do not evict it (an older node must not delete entries written in a + // newer format during a rolling deploy) nor throw (a cache should degrade to a miss, + // not fail the caller's operation). Logged at Debug to aid diagnosis without flooding + // logs during a JSON->binary migration, when every legacy key transiently lands here. + LogUndeserializableEntry(key); return null; } diff --git a/test/IntegrationTests/Cache/UndeserializableEntryTests.cs b/test/IntegrationTests/Cache/UndeserializableEntryTests.cs new file mode 100644 index 0000000..4e1fd66 --- /dev/null +++ b/test/IntegrationTests/Cache/UndeserializableEntryTests.cs @@ -0,0 +1,63 @@ +using System.Text; +using CodeCargo.Nats.DistributedCache.TestUtils.Services.Logging; +using Microsoft.Extensions.Caching.Distributed; +using Microsoft.Extensions.Logging; +using Microsoft.Extensions.Options; +using NATS.Net; + +namespace CodeCargo.Nats.DistributedCache.IntegrationTests.Cache; + +public class UndeserializableEntryTests(NatsIntegrationFixture fixture) : TestBase(fixture) +{ + // A legacy JSON envelope from a pre-binary release: the first byte is '{' (0x7B), which never + // matches the binary FormatVersion, so the serializer returns null. + private static readonly byte[] LegacyJsonEntry = + Encoding.UTF8.GetBytes("{\"absexp\":null,\"sldexp\":null,\"data\":\"AQID\"}"); + + [Fact] + public async Task PresentButUndeserializableEntryIsReadAsMissAndLoggedAtDebug() + { + var key = MethodKey(); + var logger = new RecordingLogger(); + var cache = new NatsCache(Options.Create(new NatsCacheOptions { BucketName = "cache" }), NatsConnection, logger); + await WriteRawEntryAsync(key, LegacyJsonEntry); + + // The undeserializable entry reads as a cache miss rather than throwing... + var result = await cache.GetAsync(key, TestContext.Current.CancellationToken); + Assert.Null(result); + + // ...and is surfaced once at Debug to aid diagnosis without failing the caller's operation. + var record = Assert.Single(logger.Records, r => r.EventId.Name == "UndeserializableEntry"); + Assert.Equal(LogLevel.Debug, record.LogLevel); + } + + [Fact] + public async Task UndeserializableEntryIsLeftInPlaceAndSelfHealsOnNextWrite() + { + var key = MethodKey(); + var cache = new NatsCache( + Options.Create(new NatsCacheOptions { BucketName = "cache" }), + NatsConnection, + new RecordingLogger()); + await WriteRawEntryAsync(key, LegacyJsonEntry); + + // The read leaves the entry untouched (no eviction), so it is still a miss on a second read. + Assert.Null(await cache.GetAsync(key, TestContext.Current.CancellationToken)); + + // Writing the key (a no-TTL entry, as in the migration case) overwrites the legacy bytes... + var value = Encoding.UTF8.GetBytes($"healed-{Guid.NewGuid()}"); + await cache.SetAsync(key, value, new DistributedCacheEntryOptions(), TestContext.Current.CancellationToken); + + // ...and the entry is now readable, confirming the documented "re-populated on next write" path. + Assert.Equal(value, await cache.GetAsync(key, TestContext.Current.CancellationToken)); + } + + // Writes raw bytes to the "cache" bucket at the key the cache reads, bypassing the binary + // serializer so the stored entry cannot be deserialized. + private async Task WriteRawEntryAsync(string key, byte[] raw) + { + var encodedKey = new NatsCacheKeyEncoder().Encode(key); + var kvStore = await NatsConnection.CreateKeyValueStoreContext().GetStoreAsync("cache"); + await kvStore.PutAsync(encodedKey, raw, cancellationToken: TestContext.Current.CancellationToken); + } +} diff --git a/test/UnitTests/Serialization/CacheEntryBinarySerializerTests.cs b/test/UnitTests/Serialization/CacheEntryBinarySerializerTests.cs index 13022a4..65164a6 100644 --- a/test/UnitTests/Serialization/CacheEntryBinarySerializerTests.cs +++ b/test/UnitTests/Serialization/CacheEntryBinarySerializerTests.cs @@ -159,6 +159,43 @@ public void Deserialize_OutOfRangeAbsoluteTicks_ReturnsNull() Assert.Null(Deserialize(bytes)); } + [Fact] + public void Deserialize_OutOfRangeSlidingTicks_ReturnsNull() + { + // flags = HasSlidingExpiration, followed by an absurd sliding tick value. Before validation + // this deserialized "successfully" and yielded a ~10,675-day TTL; it must now fail closed. + Assert.Null(Deserialize(SlidingOnly(long.MaxValue))); + } + + [Theory] + [InlineData(0L)] + [InlineData(-1L)] + [InlineData(long.MinValue)] + public void Deserialize_NonPositiveSlidingTicks_ReturnsNull(long slidingTicks) + { + // A valid sliding window is always a positive TimeSpan, so zero/negative ticks are corrupt. + Assert.Null(Deserialize(SlidingOnly(slidingTicks))); + } + + [Fact] + public void Deserialize_TruncatedSlidingField_ReturnsNull() + { + // Header claims a sliding expiration but only supplies 4 of the required 8 bytes. + byte[] truncated = [CacheEntryBinarySerializer.FormatVersion, 0b0000_0010, 1, 2, 3, 4]; + + Assert.Null(Deserialize(truncated)); + } + + [Fact] + public void Deserialize_MaxAcceptedSlidingTicks_RoundTrips() + { + // The upper bound (the full DateTimeOffset tick range) is inclusive and still deserializes. + var result = Deserialize(SlidingOnly(DateTimeOffset.MaxValue.Ticks)); + + Assert.NotNull(result); + Assert.Equal(DateTimeOffset.MaxValue.Ticks, result.SlidingExpirationTicks); + } + [Fact] public void Deserialize_MultiSegmentSequence_RoundTrips() { @@ -195,6 +232,16 @@ public void AbsoluteExpiration_RoundTripsAsUtcInstant() Assert.Equal(local.UtcDateTime, result.AbsoluteExpiration.Value.UtcDateTime); } + // Builds a sliding-expiration-only entry: [version][flags=HasSlidingExpiration][slidingTicks:8]. + private static byte[] SlidingOnly(long slidingTicks) + { + var bytes = new byte[2 + 8]; + bytes[0] = CacheEntryBinarySerializer.FormatVersion; + bytes[1] = 0b0000_0010; + BinaryPrimitives.WriteInt64LittleEndian(bytes.AsSpan(2), slidingTicks); + return bytes; + } + private static ReadOnlySequence CreateSegmented(byte[] data, int splitAt) { var first = new BufferSegment(data.AsMemory(0, splitAt)); From 46a03ceee18614842d72f60acffc4a5bf622638d Mon Sep 17 00:00:00 2001 From: Matthew DeVenny Date: Mon, 6 Jul 2026 09:54:31 -0700 Subject: [PATCH 2/4] ensure set and get are consistent Signed-off-by: Matthew DeVenny --- .../CacheEntryBinarySerializer.cs | 19 ++++++++++++++----- src/NatsDistributedCache/NatsCache.cs | 12 ++++++++++++ .../Cache/TimeExpirationUnitTests.cs | 19 +++++++++++++++++++ 3 files changed, 45 insertions(+), 5 deletions(-) diff --git a/src/NatsDistributedCache/CacheEntryBinarySerializer.cs b/src/NatsDistributedCache/CacheEntryBinarySerializer.cs index 4e6d4d4..50e0168 100644 --- a/src/NatsDistributedCache/CacheEntryBinarySerializer.cs +++ b/src/NatsDistributedCache/CacheEntryBinarySerializer.cs @@ -26,6 +26,15 @@ internal sealed class CacheEntryBinarySerializer : INatsSerialize, I /// internal const byte FormatVersion = 1; + /// + /// The largest accepted sliding-expiration window, matching the absolute-expiration ceiling: a + /// sliding window cannot exceed the full tick range. Enforced on both + /// the write path (NatsCache.GetTtl throws) and the read path here (fail closed) so that any + /// value which can be serialized can also be deserialized — no legitimately written entry becomes + /// an undeserializable miss. + /// + internal static readonly long MaxSlidingExpirationTicks = DateTimeOffset.MaxValue.Ticks; + private const byte HasAbsoluteExpiration = 0b0000_0001; private const byte HasSlidingExpiration = 0b0000_0010; private const byte KnownFlags = HasAbsoluteExpiration | HasSlidingExpiration; @@ -115,12 +124,12 @@ public void Serialize(IBufferWriter bufferWriter, CacheEntry value) { if (!reader.TryReadLittleEndian(out long slidingTicks) || slidingTicks <= 0 || - slidingTicks > DateTimeOffset.MaxValue.Ticks) + slidingTicks > MaxSlidingExpirationTicks) { - // Missing, non-positive, or absurdly large sliding ticks (corrupt entry): fail closed - // to a miss, mirroring the absolute-ticks bounds check above. A valid sliding window - // is always a positive TimeSpan, and no legitimate window approaches the full - // DateTimeOffset tick range, so anything outside (0, MaxValue] is treated as corrupt. + // Missing, non-positive, or out-of-range sliding ticks (corrupt entry): fail closed to + // a miss, mirroring the absolute-ticks bounds check above. A valid sliding window is + // always a positive TimeSpan within (0, MaxSlidingExpirationTicks]; the write path + // enforces the same ceiling, so any legitimately written value round-trips. return null; } diff --git a/src/NatsDistributedCache/NatsCache.cs b/src/NatsDistributedCache/NatsCache.cs index cd907a9..a91e029 100644 --- a/src/NatsDistributedCache/NatsCache.cs +++ b/src/NatsDistributedCache/NatsCache.cs @@ -231,6 +231,18 @@ public async ValueTask TryGetAsync( "The sliding expiration value must be positive."); } + // Reject sliding windows the serializer could not round-trip. The read path fails closed above + // MaxSlidingExpirationTicks, so accepting a larger value here would silently store an entry that + // later reads back as an undeserializable cache miss. + if (options.SlidingExpiration.HasValue && + options.SlidingExpiration.Value.Ticks > CacheEntryBinarySerializer.MaxSlidingExpirationTicks) + { + throw new ArgumentOutOfRangeException( + nameof(DistributedCacheEntryOptions.SlidingExpiration), + options.SlidingExpiration.Value, + "The sliding expiration value is too large."); + } + var absoluteExpiration = ResolveAbsoluteExpiration(options); if (!absoluteExpiration.HasValue) { diff --git a/test/UnitTests/Cache/TimeExpirationUnitTests.cs b/test/UnitTests/Cache/TimeExpirationUnitTests.cs index eae3313..956702a 100644 --- a/test/UnitTests/Cache/TimeExpirationUnitTests.cs +++ b/test/UnitTests/Cache/TimeExpirationUnitTests.cs @@ -91,4 +91,23 @@ public void ZeroSlidingExpirationThrows() "The sliding expiration value must be positive.", TimeSpan.Zero); } + + [Fact] + public void TooLargeSlidingExpirationThrows() + { + var key = MethodKey(); + var value = new byte[1]; + + // TimeSpan.MaxValue.Ticks == long.MaxValue, which exceeds the ceiling the serializer can + // round-trip; the write path must reject it rather than store an entry that later reads back + // as an undeserializable miss. + ExceptionAssert.ThrowsArgumentOutOfRange( + () => + { + Cache.Set(key, value, new DistributedCacheEntryOptions().SetSlidingExpiration(TimeSpan.MaxValue)); + }, + nameof(DistributedCacheEntryOptions.SlidingExpiration), + "The sliding expiration value is too large.", + TimeSpan.MaxValue); + } } From ee12ffa9a9c5df36651c9302fd6149771a9b73eb Mon Sep 17 00:00:00 2001 From: Matthew DeVenny Date: Mon, 6 Jul 2026 11:10:59 -0700 Subject: [PATCH 3/4] #49 Bound sliding and absolute expirations to the NATS TTL encoding limit NATS message TTLs are encoded as (int)ttl.TotalSeconds in ToTtlString, which overflows above int.MaxValue seconds (~68 years) and emits an invalid header. Cap both expiration kinds at that limit: - Rename the shared ceiling to MaxTtlTicks (= int.MaxValue seconds) and correct it from DateTimeOffset.MaxValue.Ticks (~10k years), which was well above what the encoding can represent. - GetTtl now rejects an absolute-only expiration whose window exceeds the ceiling (the absolute+sliding minimum is already bounded by sliding). - The deserializer still fails closed on stored sliding ticks above it. Also strengthen the no-eviction integration test with a direct KV-store assertion, and fix an inaccurate magnitude in a test comment. Co-Authored-By: Claude Opus 4.8 (1M context) Signed-off-by: Matthew DeVenny --- .../CacheEntryBinarySerializer.cs | 22 +++++----- src/NatsDistributedCache/NatsCache.cs | 37 +++++++++++++--- .../Cache/UndeserializableEntryTests.cs | 8 +++- .../Cache/TimeExpirationUnitTests.cs | 43 +++++++++++++++++-- .../Cache/TimeProviderExpirationUnitTests.cs | 24 +++++++++++ .../CacheEntryBinarySerializerTests.cs | 19 ++++---- 6 files changed, 124 insertions(+), 29 deletions(-) diff --git a/src/NatsDistributedCache/CacheEntryBinarySerializer.cs b/src/NatsDistributedCache/CacheEntryBinarySerializer.cs index 50e0168..1891cb9 100644 --- a/src/NatsDistributedCache/CacheEntryBinarySerializer.cs +++ b/src/NatsDistributedCache/CacheEntryBinarySerializer.cs @@ -27,13 +27,15 @@ internal sealed class CacheEntryBinarySerializer : INatsSerialize, I internal const byte FormatVersion = 1; /// - /// The largest accepted sliding-expiration window, matching the absolute-expiration ceiling: a - /// sliding window cannot exceed the full tick range. Enforced on both - /// the write path (NatsCache.GetTtl throws) and the read path here (fail closed) so that any - /// value which can be serialized can also be deserialized — no legitimately written entry becomes - /// an undeserializable miss. + /// The largest expiration window the cache can represent, in ticks. NATS message TTLs are encoded as + /// whole seconds in a 32-bit field ((int)ttl.TotalSeconds in NatsExtensions.ToTtlString), + /// so a window beyond seconds (~68 years) would overflow the cast and emit + /// an invalid TTL header. NatsCache.GetTtl rejects sliding and absolute expirations that exceed + /// this on write, and the deserializer fails closed on stored sliding ticks above it, so every + /// accepted value round-trips end to end and no legitimately written entry becomes an + /// undeserializable miss. /// - internal static readonly long MaxSlidingExpirationTicks = DateTimeOffset.MaxValue.Ticks; + internal const long MaxTtlTicks = int.MaxValue * TimeSpan.TicksPerSecond; private const byte HasAbsoluteExpiration = 0b0000_0001; private const byte HasSlidingExpiration = 0b0000_0010; @@ -124,12 +126,12 @@ public void Serialize(IBufferWriter bufferWriter, CacheEntry value) { if (!reader.TryReadLittleEndian(out long slidingTicks) || slidingTicks <= 0 || - slidingTicks > MaxSlidingExpirationTicks) + slidingTicks > MaxTtlTicks) { // Missing, non-positive, or out-of-range sliding ticks (corrupt entry): fail closed to - // a miss, mirroring the absolute-ticks bounds check above. A valid sliding window is - // always a positive TimeSpan within (0, MaxSlidingExpirationTicks]; the write path - // enforces the same ceiling, so any legitimately written value round-trips. + // a miss, like the absolute-ticks bounds check above. A valid sliding window is always + // a positive TimeSpan within (0, MaxTtlTicks]; the write path enforces the same ceiling, + // so any legitimately written value round-trips. return null; } diff --git a/src/NatsDistributedCache/NatsCache.cs b/src/NatsDistributedCache/NatsCache.cs index a91e029..44f0f83 100644 --- a/src/NatsDistributedCache/NatsCache.cs +++ b/src/NatsDistributedCache/NatsCache.cs @@ -231,11 +231,11 @@ public async ValueTask TryGetAsync( "The sliding expiration value must be positive."); } - // Reject sliding windows the serializer could not round-trip. The read path fails closed above - // MaxSlidingExpirationTicks, so accepting a larger value here would silently store an entry that + // Reject sliding windows the serializer/TTL encoding could not round-trip. The read path fails + // closed above MaxTtlTicks, so accepting a larger value here would silently store an entry that // later reads back as an undeserializable cache miss. if (options.SlidingExpiration.HasValue && - options.SlidingExpiration.Value.Ticks > CacheEntryBinarySerializer.MaxSlidingExpirationTicks) + options.SlidingExpiration.Value.Ticks > CacheEntryBinarySerializer.MaxTtlTicks) { throw new ArgumentOutOfRangeException( nameof(DistributedCacheEntryOptions.SlidingExpiration), @@ -256,10 +256,33 @@ public async ValueTask TryGetAsync( return TimeSpan.Zero; } - // If there's also a sliding expiration, use the minimum of the two - return options.SlidingExpiration.HasValue - ? TimeSpan.FromTicks(Math.Min(ttl.Ticks, options.SlidingExpiration.Value.Ticks)) - : ttl; + // If there's also a sliding expiration, use the minimum of the two. Sliding is bounded to + // MaxTtlTicks above, so the minimum is always within the encodable range. + if (options.SlidingExpiration.HasValue) + { + return TimeSpan.FromTicks(Math.Min(ttl.Ticks, options.SlidingExpiration.Value.Ticks)); + } + + // Absolute-only: the TTL spans the full window to the absolute instant. Reject windows the NATS + // TTL encoding cannot represent (see CacheEntryBinarySerializer.MaxTtlTicks) rather than emitting + // an overflowed header. + if (ttl.Ticks > CacheEntryBinarySerializer.MaxTtlTicks) + { + if (options.AbsoluteExpirationRelativeToNow.HasValue) + { + throw new ArgumentOutOfRangeException( + nameof(DistributedCacheEntryOptions.AbsoluteExpirationRelativeToNow), + options.AbsoluteExpirationRelativeToNow.Value, + "The relative expiration value is too large."); + } + + throw new ArgumentOutOfRangeException( + nameof(DistributedCacheEntryOptions.AbsoluteExpiration), + options.AbsoluteExpiration!.Value, + "The absolute expiration is too far in the future."); + } + + return ttl; } internal CacheEntry CreateCacheEntry(byte[] value, DistributedCacheEntryOptions options) => diff --git a/test/IntegrationTests/Cache/UndeserializableEntryTests.cs b/test/IntegrationTests/Cache/UndeserializableEntryTests.cs index 4e1fd66..719c904 100644 --- a/test/IntegrationTests/Cache/UndeserializableEntryTests.cs +++ b/test/IntegrationTests/Cache/UndeserializableEntryTests.cs @@ -41,8 +41,14 @@ public async Task UndeserializableEntryIsLeftInPlaceAndSelfHealsOnNextWrite() new RecordingLogger()); await WriteRawEntryAsync(key, LegacyJsonEntry); - // The read leaves the entry untouched (no eviction), so it is still a miss on a second read. + // The read leaves the entry untouched (no eviction): the original legacy bytes are still in + // the bucket afterwards, and it is still a miss on a second read. Assert.Null(await cache.GetAsync(key, TestContext.Current.CancellationToken)); + var kvStore = await NatsConnection.CreateKeyValueStoreContext().GetStoreAsync("cache"); + var storedEntry = await kvStore.GetEntryAsync( + new NatsCacheKeyEncoder().Encode(key), + cancellationToken: TestContext.Current.CancellationToken); + Assert.Equal(LegacyJsonEntry, storedEntry.Value); // Writing the key (a no-TTL entry, as in the migration case) overwrites the legacy bytes... var value = Encoding.UTF8.GetBytes($"healed-{Guid.NewGuid()}"); diff --git a/test/UnitTests/Cache/TimeExpirationUnitTests.cs b/test/UnitTests/Cache/TimeExpirationUnitTests.cs index 956702a..3b97e6b 100644 --- a/test/UnitTests/Cache/TimeExpirationUnitTests.cs +++ b/test/UnitTests/Cache/TimeExpirationUnitTests.cs @@ -98,9 +98,9 @@ public void TooLargeSlidingExpirationThrows() var key = MethodKey(); var value = new byte[1]; - // TimeSpan.MaxValue.Ticks == long.MaxValue, which exceeds the ceiling the serializer can - // round-trip; the write path must reject it rather than store an entry that later reads back - // as an undeserializable miss. + // TimeSpan.MaxValue is far beyond the ceiling the NATS TTL encoding supports (int.MaxValue + // seconds), so the write path must reject it rather than store an entry that later reads back + // as an undeserializable miss or emits an overflowed TTL header. ExceptionAssert.ThrowsArgumentOutOfRange( () => { @@ -110,4 +110,41 @@ public void TooLargeSlidingExpirationThrows() "The sliding expiration value is too large.", TimeSpan.MaxValue); } + + [Fact] + public void TooFarRelativeExpirationThrows() + { + var key = MethodKey(); + var value = new byte[1]; + + // A relative expiration beyond the NATS TTL encoding limit (int.MaxValue seconds, ~68 years) + // would overflow the (int) cast in ToTtlString, so it must be rejected on write. + var relative = TimeSpan.FromDays(365 * 100); + ExceptionAssert.ThrowsArgumentOutOfRange( + () => + { + Cache.Set(key, value, new DistributedCacheEntryOptions().SetAbsoluteExpiration(relative)); + }, + nameof(DistributedCacheEntryOptions.AbsoluteExpirationRelativeToNow), + "The relative expiration value is too large.", + relative); + } + + [Fact] + public void TooFarAbsoluteExpirationThrows() + { + var key = MethodKey(); + var value = new byte[1]; + + // Same limit for an absolute instant: more than ~68 years out overflows the TTL header. + var absolute = TimeProvider.GetUtcNow().AddYears(100); + ExceptionAssert.ThrowsArgumentOutOfRange( + () => + { + Cache.Set(key, value, new DistributedCacheEntryOptions().SetAbsoluteExpiration(absolute)); + }, + nameof(DistributedCacheEntryOptions.AbsoluteExpiration), + "The absolute expiration is too far in the future.", + absolute); + } } diff --git a/test/UnitTests/Cache/TimeProviderExpirationUnitTests.cs b/test/UnitTests/Cache/TimeProviderExpirationUnitTests.cs index 25c3cd1..5afa311 100644 --- a/test/UnitTests/Cache/TimeProviderExpirationUnitTests.cs +++ b/test/UnitTests/Cache/TimeProviderExpirationUnitTests.cs @@ -95,4 +95,28 @@ public void GetTtlReturnsMinimumOfSlidingAndAbsolute() Assert.Equal(TimeSpan.FromMinutes(2), ttl); } + + [Fact] + public void GetTtlAcceptsMaxSlidingExpiration() + { + // The ceiling itself is valid: it round-trips through both the serializer and the + // second-granularity NATS TTL encoding without overflowing. + var max = TimeSpan.FromTicks(CacheEntryBinarySerializer.MaxTtlTicks); + + var ttl = Cache.GetTtl(new DistributedCacheEntryOptions().SetSlidingExpiration(max)); + + Assert.Equal(max, ttl); + } + + [Fact] + public void GetTtlWithFarAbsoluteAndSlidingReturnsSliding() + { + // A far-future absolute paired with a small sliding must not be rejected: the effective TTL is + // the (encodable) sliding window, not the huge absolute one, so the ceiling check is not tripped. + var ttl = Cache.GetTtl(new DistributedCacheEntryOptions() + .SetAbsoluteExpiration(TimeSpan.FromDays(365 * 100)) + .SetSlidingExpiration(TimeSpan.FromMinutes(10))); + + Assert.Equal(TimeSpan.FromMinutes(10), ttl); + } } diff --git a/test/UnitTests/Serialization/CacheEntryBinarySerializerTests.cs b/test/UnitTests/Serialization/CacheEntryBinarySerializerTests.cs index 65164a6..6dc3beb 100644 --- a/test/UnitTests/Serialization/CacheEntryBinarySerializerTests.cs +++ b/test/UnitTests/Serialization/CacheEntryBinarySerializerTests.cs @@ -159,12 +159,14 @@ public void Deserialize_OutOfRangeAbsoluteTicks_ReturnsNull() Assert.Null(Deserialize(bytes)); } - [Fact] - public void Deserialize_OutOfRangeSlidingTicks_ReturnsNull() + [Theory] + [InlineData(long.MaxValue)] + [InlineData(CacheEntryBinarySerializer.MaxTtlTicks + 1)] + public void Deserialize_OutOfRangeSlidingTicks_ReturnsNull(long slidingTicks) { - // flags = HasSlidingExpiration, followed by an absurd sliding tick value. Before validation - // this deserialized "successfully" and yielded a ~10,675-day TTL; it must now fail closed. - Assert.Null(Deserialize(SlidingOnly(long.MaxValue))); + // Before validation an absurd value deserialized "successfully": TimeSpan.FromTicks(long.MaxValue) + // is ~10,675,199 days (~29,000 years). Anything above the ceiling must now fail closed. + Assert.Null(Deserialize(SlidingOnly(slidingTicks))); } [Theory] @@ -189,11 +191,12 @@ public void Deserialize_TruncatedSlidingField_ReturnsNull() [Fact] public void Deserialize_MaxAcceptedSlidingTicks_RoundTrips() { - // The upper bound (the full DateTimeOffset tick range) is inclusive and still deserializes. - var result = Deserialize(SlidingOnly(DateTimeOffset.MaxValue.Ticks)); + // The upper bound (int.MaxValue seconds, the largest window the NATS TTL encoding supports) is + // inclusive and still deserializes. + var result = Deserialize(SlidingOnly(CacheEntryBinarySerializer.MaxTtlTicks)); Assert.NotNull(result); - Assert.Equal(DateTimeOffset.MaxValue.Ticks, result.SlidingExpirationTicks); + Assert.Equal(CacheEntryBinarySerializer.MaxTtlTicks, result.SlidingExpirationTicks); } [Fact] From efea3977869bde8804ede6d75bc08d1df86ea670 Mon Sep 17 00:00:00 2001 From: Matthew DeVenny Date: Thu, 16 Jul 2026 14:04:17 -0700 Subject: [PATCH 4/4] #49 Support MaxValue "never expire" sentinels; polish range errors and logs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address review feedback from mtmk: - Treat DateTimeOffset.MaxValue / TimeSpan.MaxValue expirations as "cache forever" (no TTL) instead of rejecting them as out-of-range — a common "never expire" idiom, normalized to no expiration in GetTtl/CreateCacheEntry. - Include the maximum window in the range-check exception messages. - Use PascalCase structured log property names ({Key}). Co-Authored-By: Claude Opus 4.8 (1M context) Signed-off-by: Matthew DeVenny --- .../CacheEntryBinarySerializer.cs | 5 ++ src/NatsDistributedCache/NatsCache.Log.cs | 2 +- src/NatsDistributedCache/NatsCache.cs | 75 ++++++++++++------- .../Cache/TimeExpirationUnitTests.cs | 9 ++- .../Cache/TimeProviderExpirationUnitTests.cs | 35 +++++++++ 5 files changed, 94 insertions(+), 32 deletions(-) diff --git a/src/NatsDistributedCache/CacheEntryBinarySerializer.cs b/src/NatsDistributedCache/CacheEntryBinarySerializer.cs index 1891cb9..2a43c71 100644 --- a/src/NatsDistributedCache/CacheEntryBinarySerializer.cs +++ b/src/NatsDistributedCache/CacheEntryBinarySerializer.cs @@ -37,6 +37,11 @@ internal sealed class CacheEntryBinarySerializer : INatsSerialize, I /// internal const long MaxTtlTicks = int.MaxValue * TimeSpan.TicksPerSecond; + /// + /// as a , for range-check exception messages. + /// + internal static readonly TimeSpan MaxTtl = TimeSpan.FromTicks(MaxTtlTicks); + private const byte HasAbsoluteExpiration = 0b0000_0001; private const byte HasSlidingExpiration = 0b0000_0010; private const byte KnownFlags = HasAbsoluteExpiration | HasSlidingExpiration; diff --git a/src/NatsDistributedCache/NatsCache.Log.cs b/src/NatsDistributedCache/NatsCache.Log.cs index cd27cb0..5955068 100644 --- a/src/NatsDistributedCache/NatsCache.Log.cs +++ b/src/NatsDistributedCache/NatsCache.Log.cs @@ -16,7 +16,7 @@ private void LogSwallowedException(Exception exception) => private void LogUndeserializableEntry(string key) => _logger.LogDebug( EventIds.UndeserializableEntry, - "Cache entry for key {key} could not be deserialized (legacy or corrupt format); returning a cache miss", + "Cache entry for key {Key} could not be deserialized (legacy or corrupt format); returning a cache miss", key); private static class EventIds diff --git a/src/NatsDistributedCache/NatsCache.cs b/src/NatsDistributedCache/NatsCache.cs index 44f0f83..82e8d19 100644 --- a/src/NatsDistributedCache/NatsCache.cs +++ b/src/NatsDistributedCache/NatsCache.cs @@ -206,50 +206,56 @@ public async ValueTask TryGetAsync( internal TimeSpan? GetTtl(DistributedCacheEntryOptions options) { - if (options.AbsoluteExpiration.HasValue && options.AbsoluteExpiration.Value <= TimeProvider.GetUtcNow()) + // Maximum-value sentinels mean "never expire": normalize them to no expiration so they are not + // rejected as out-of-range below. Callers commonly use DateTimeOffset.MaxValue / TimeSpan.MaxValue + // as a "cache forever" idiom, which is equivalent to omitting expiration entirely. + var absoluteExpiration = EffectiveAbsoluteExpiration(options); + var relativeToNow = EffectiveRelativeToNow(options); + var slidingExpiration = EffectiveSlidingExpiration(options); + + if (absoluteExpiration.HasValue && absoluteExpiration.Value <= TimeProvider.GetUtcNow()) { throw new ArgumentOutOfRangeException( nameof(DistributedCacheEntryOptions.AbsoluteExpiration), - options.AbsoluteExpiration.Value, + absoluteExpiration.Value, "The absolute expiration value must be in the future."); } - if (options.AbsoluteExpirationRelativeToNow.HasValue && - options.AbsoluteExpirationRelativeToNow.Value <= TimeSpan.Zero) + if (relativeToNow.HasValue && relativeToNow.Value <= TimeSpan.Zero) { throw new ArgumentOutOfRangeException( nameof(DistributedCacheEntryOptions.AbsoluteExpirationRelativeToNow), - options.AbsoluteExpirationRelativeToNow.Value, + relativeToNow.Value, "The relative expiration value must be positive."); } - if (options.SlidingExpiration.HasValue && options.SlidingExpiration.Value <= TimeSpan.Zero) + if (slidingExpiration.HasValue && slidingExpiration.Value <= TimeSpan.Zero) { throw new ArgumentOutOfRangeException( nameof(DistributedCacheEntryOptions.SlidingExpiration), - options.SlidingExpiration.Value, + slidingExpiration.Value, "The sliding expiration value must be positive."); } // Reject sliding windows the serializer/TTL encoding could not round-trip. The read path fails // closed above MaxTtlTicks, so accepting a larger value here would silently store an entry that // later reads back as an undeserializable cache miss. - if (options.SlidingExpiration.HasValue && - options.SlidingExpiration.Value.Ticks > CacheEntryBinarySerializer.MaxTtlTicks) + if (slidingExpiration.HasValue && + slidingExpiration.Value.Ticks > CacheEntryBinarySerializer.MaxTtlTicks) { throw new ArgumentOutOfRangeException( nameof(DistributedCacheEntryOptions.SlidingExpiration), - options.SlidingExpiration.Value, - "The sliding expiration value is too large."); + slidingExpiration.Value, + $"The sliding expiration value is too large. The maximum is {CacheEntryBinarySerializer.MaxTtl}."); } - var absoluteExpiration = ResolveAbsoluteExpiration(options); - if (!absoluteExpiration.HasValue) + var resolvedAbsolute = ResolveAbsoluteExpiration(options); + if (!resolvedAbsolute.HasValue) { - return options.SlidingExpiration; + return slidingExpiration; } - var ttl = absoluteExpiration.Value - TimeProvider.GetUtcNow(); + var ttl = resolvedAbsolute.Value - TimeProvider.GetUtcNow(); if (ttl.TotalMilliseconds <= 0) { // Value is in the past, remove it @@ -258,9 +264,9 @@ public async ValueTask TryGetAsync( // If there's also a sliding expiration, use the minimum of the two. Sliding is bounded to // MaxTtlTicks above, so the minimum is always within the encodable range. - if (options.SlidingExpiration.HasValue) + if (slidingExpiration.HasValue) { - return TimeSpan.FromTicks(Math.Min(ttl.Ticks, options.SlidingExpiration.Value.Ticks)); + return TimeSpan.FromTicks(Math.Min(ttl.Ticks, slidingExpiration.Value.Ticks)); } // Absolute-only: the TTL spans the full window to the absolute instant. Reject windows the NATS @@ -268,18 +274,18 @@ public async ValueTask TryGetAsync( // an overflowed header. if (ttl.Ticks > CacheEntryBinarySerializer.MaxTtlTicks) { - if (options.AbsoluteExpirationRelativeToNow.HasValue) + if (relativeToNow.HasValue) { throw new ArgumentOutOfRangeException( nameof(DistributedCacheEntryOptions.AbsoluteExpirationRelativeToNow), - options.AbsoluteExpirationRelativeToNow.Value, - "The relative expiration value is too large."); + relativeToNow.Value, + $"The relative expiration value is too large. The maximum is {CacheEntryBinarySerializer.MaxTtl}."); } throw new ArgumentOutOfRangeException( nameof(DistributedCacheEntryOptions.AbsoluteExpiration), - options.AbsoluteExpiration!.Value, - "The absolute expiration is too far in the future."); + absoluteExpiration!.Value, + $"The absolute expiration is too far in the future. The maximum window is {CacheEntryBinarySerializer.MaxTtl}."); } return ttl; @@ -290,7 +296,7 @@ internal CacheEntry CreateCacheEntry(byte[] value, DistributedCacheEntryOptions { Data = value, AbsoluteExpiration = ResolveAbsoluteExpiration(options), - SlidingExpirationTicks = options.SlidingExpiration?.Ticks + SlidingExpirationTicks = EffectiveSlidingExpiration(options)?.Ticks }; // An entry is absolutely expired once the clock reaches its absolute expiration instant. The @@ -326,12 +332,27 @@ internal NatsKVConfig BuildBucketConfig() return config; } + // "Never expire" sentinels: a DateTimeOffset.MaxValue absolute instant or a TimeSpan.MaxValue window + // is normalized to no expiration, so it is not rejected as out-of-range and the entry lives forever. + private static DateTimeOffset? EffectiveAbsoluteExpiration(DistributedCacheEntryOptions options) => + options.AbsoluteExpiration == DateTimeOffset.MaxValue ? null : options.AbsoluteExpiration; + + private static TimeSpan? EffectiveRelativeToNow(DistributedCacheEntryOptions options) => + options.AbsoluteExpirationRelativeToNow == TimeSpan.MaxValue ? null : options.AbsoluteExpirationRelativeToNow; + + private static TimeSpan? EffectiveSlidingExpiration(DistributedCacheEntryOptions options) => + options.SlidingExpiration == TimeSpan.MaxValue ? null : options.SlidingExpiration; + // Resolves the effective absolute expiration instant: a relative expiration (offset from the // current clock) takes precedence over an explicit absolute expiration when both are set. - private DateTimeOffset? ResolveAbsoluteExpiration(DistributedCacheEntryOptions options) => - options.AbsoluteExpirationRelativeToNow.HasValue - ? TimeProvider.GetUtcNow().Add(options.AbsoluteExpirationRelativeToNow.Value) - : options.AbsoluteExpiration; + // Maximum-value sentinels are treated as "no expiration" (see the Effective* helpers). + private DateTimeOffset? ResolveAbsoluteExpiration(DistributedCacheEntryOptions options) + { + var relativeToNow = EffectiveRelativeToNow(options); + return relativeToNow.HasValue + ? TimeProvider.GetUtcNow().Add(relativeToNow.Value) + : EffectiveAbsoluteExpiration(options); + } private string GetEncodedKey(string key) => string.IsNullOrEmpty(_keyPrefix) diff --git a/test/UnitTests/Cache/TimeExpirationUnitTests.cs b/test/UnitTests/Cache/TimeExpirationUnitTests.cs index 3b97e6b..3c7dd9a 100644 --- a/test/UnitTests/Cache/TimeExpirationUnitTests.cs +++ b/test/UnitTests/Cache/TimeExpirationUnitTests.cs @@ -98,17 +98,18 @@ public void TooLargeSlidingExpirationThrows() var key = MethodKey(); var value = new byte[1]; - // TimeSpan.MaxValue is far beyond the ceiling the NATS TTL encoding supports (int.MaxValue - // seconds), so the write path must reject it rather than store an entry that later reads back + // A window beyond the NATS TTL encoding ceiling (~68 years) but short of the TimeSpan.MaxValue + // "never expire" sentinel must be rejected rather than stored as an entry that later reads back // as an undeserializable miss or emits an overflowed TTL header. + var sliding = TimeSpan.FromDays(365 * 100); ExceptionAssert.ThrowsArgumentOutOfRange( () => { - Cache.Set(key, value, new DistributedCacheEntryOptions().SetSlidingExpiration(TimeSpan.MaxValue)); + Cache.Set(key, value, new DistributedCacheEntryOptions().SetSlidingExpiration(sliding)); }, nameof(DistributedCacheEntryOptions.SlidingExpiration), "The sliding expiration value is too large.", - TimeSpan.MaxValue); + sliding); } [Fact] diff --git a/test/UnitTests/Cache/TimeProviderExpirationUnitTests.cs b/test/UnitTests/Cache/TimeProviderExpirationUnitTests.cs index 5afa311..a76856e 100644 --- a/test/UnitTests/Cache/TimeProviderExpirationUnitTests.cs +++ b/test/UnitTests/Cache/TimeProviderExpirationUnitTests.cs @@ -119,4 +119,39 @@ public void GetTtlWithFarAbsoluteAndSlidingReturnsSliding() Assert.Equal(TimeSpan.FromMinutes(10), ttl); } + + // DateTimeOffset.MaxValue / TimeSpan.MaxValue mean "cache forever": no TTL, not an out-of-range throw. + [Fact] + public void GetTtlTreatsMaxAbsoluteInstantAsNoExpiration() => + Assert.Null(Cache.GetTtl(new DistributedCacheEntryOptions().SetAbsoluteExpiration(DateTimeOffset.MaxValue))); + + [Fact] + public void GetTtlTreatsMaxRelativeExpirationAsNoExpiration() => + Assert.Null(Cache.GetTtl(new DistributedCacheEntryOptions().SetAbsoluteExpiration(TimeSpan.MaxValue))); + + [Fact] + public void GetTtlTreatsMaxSlidingExpirationAsNoExpiration() => + Assert.Null(Cache.GetTtl(new DistributedCacheEntryOptions().SetSlidingExpiration(TimeSpan.MaxValue))); + + [Fact] + public void CreateCacheEntryTreatsMaxValueSlidingAsNoExpiration() + { + var entry = Cache.CreateCacheEntry( + new byte[1], + new DistributedCacheEntryOptions().SetSlidingExpiration(TimeSpan.MaxValue)); + + Assert.Null(entry.SlidingExpirationTicks); + Assert.Null(entry.AbsoluteExpiration); + } + + [Fact] + public void CreateCacheEntryTreatsMaxValueAbsoluteAsNoExpiration() + { + var entry = Cache.CreateCacheEntry( + new byte[1], + new DistributedCacheEntryOptions().SetAbsoluteExpiration(DateTimeOffset.MaxValue)); + + Assert.Null(entry.AbsoluteExpiration); + Assert.Null(entry.SlidingExpirationTicks); + } }