Remove only the expired cache entry TryGet observed, and saturate the TTL [patch]

CLAUDE.md

@@ -145,7 +145,7 @@ Tests use **MSTest.Sdk** targeting net10.0 only. The test project (`Essentials.T
 - `KeyedHashProviderTests.cs` - Tests all 3 HMAC keyed hash providers, `Verify`, and `FixedTimeComparison`
 - `RandomProviderTests.cs` - Contract tests over all 4 random providers (bounds, validation, shuffle and sampling invariants, uniformity of a range that does not divide 2^32), plus reference sequences for Xoshiro and Pcg produced by an independent transcription of each published algorithm. Those vectors make a seeded sequence part of the package contract: changing one is a breaking change and this is where it surfaces
 - `DistributionProviderTests.cs` - Tests all 10 distributions three ways: contract properties every distribution must have (CDF bounded and non-decreasing, quantile inverts it, density integrates to one, masses sum to one, survival function complements the CDF), reference values computed outside the codebase, and empirical checks that seeded samples match the analytic moments and deciles
-- `CacheProviderTests.cs` - Tests cache operations including expiration, and that a legitimately cached `null` is honoured rather than treated as a miss: `Get`/`GetAsync` return it instead of throwing, and `GetOrAdd`/`GetOrAddAsync` return it without re-invoking the factory. `TryGet`'s `bool` is the sole authority on presence — a null check on the value is not a proxy for it
+- `CacheProviderTests.cs` - Tests cache operations including expiration, and that a legitimately cached `null` is honoured rather than treated as a miss: `Get`/`GetAsync` return it instead of throwing, and `GetOrAdd`/`GetOrAddAsync` return it without re-invoking the factory. `TryGet`'s `bool` is the sole authority on presence — a null check on the value is not a proxy for it. It also pins that an expired entry's removal leaves a value set concurrently under the same key in place (a key type that runs the `Set` between `TryGet`'s read and its remove makes that interleaving deterministic), and that a `TimeSpan.MaxValue` or `TimeSpan.MinValue` time-to-live saturates rather than overflowing
 - `CommandExecutorTests.cs` - Tests command execution, including the synchronous path, cancellation before and during a run, a working directory that does not exist, that the synchronous and asynchronous paths capture the same bytes for a command whose output ends without a terminator, and that `ExecuteAndGetOutput` throws unwrapped. `ICommandExecutor`'s own synchronous defaults are reached through a test double that declares only the asynchronous members, since `NativeCommandExecutor` replaces them
 - `EncodingProviderTests.cs` - Tests Base64 and Hex encoding
 - `ObfuscationProviderTests.cs` - Tests all obfuscation providers via round-trip (obfuscate → deobfuscate)

Essentials.CacheProviders.InMemory/InMemoryCacheProvider.cs

@@ -5,6 +5,7 @@ namespace ktsu.Essentials.CacheProviders.InMemory;
 using ktsu.Essentials;
 using System;
 using System.Collections.Concurrent;
+using System.Collections.Generic;
 
 /// <summary>
 /// An in-memory cache provider that stores key-value pairs with optional expiration support.
@@ -31,8 +32,9 @@ public bool TryGet(TKey key, out TValue? value)
 				return true;
 			}
 
-			// Entry has expired, remove it
-			cache.TryRemove(key, out _);
+			// Entry has expired. Remove only the entry that was observed, so a fresh value another thread
+			// has just set under the same key is left in place.
+			RemoveEntry(key, entry);
 		}
 
 		value = default;
@@ -45,11 +47,8 @@ public bool TryGet(TKey key, out TValue? value)
 	/// <param name="key">The cache key.</param>
 	/// <param name="value">The value to cache.</param>
 	/// <param name="expiration">The optional time-to-live for the cached entry. If null, the entry does not expire.</param>
-	public void Set(TKey key, TValue value, TimeSpan? expiration = null)
-	{
-		DateTime? expirationTime = expiration.HasValue ? DateTime.UtcNow + expiration.Value : null;
-		cache[key] = new CacheEntry(value, expirationTime);
-	}
+	public void Set(TKey key, TValue value, TimeSpan? expiration = null) =>
+		cache[key] = new CacheEntry(value, ComputeExpiration(expiration));
 
 	/// <summary>
 	/// Removes a cached value by key.
@@ -63,6 +62,37 @@ public void Set(TKey key, TValue value, TimeSpan? expiration = null)
 	/// </summary>
 	public void Clear() => cache.Clear();
 
+	/// <summary>
+	/// Converts a time-to-live into an absolute expiration time, saturating rather than overflowing.
+	/// A time-to-live that reaches past <see cref="DateTime.MaxValue"/> never expires, and one that
+	/// reaches before <see cref="DateTime.MinValue"/> has already expired.
+	/// </summary>
+	private static DateTime? ComputeExpiration(TimeSpan? expiration)
+	{
+		if (expiration is null)
+		{
+			return null;
+		}
+
+		DateTime now = DateTime.UtcNow;
+		if (expiration.Value > DateTime.MaxValue - now)
+		{
+			return null;
+		}
+
+		return expiration.Value < DateTime.MinValue - now ? DateTime.MinValue : now + expiration.Value;
+	}
+
+	private void RemoveEntry(TKey key, CacheEntry entry)
+	{
+		KeyValuePair<TKey, CacheEntry> observed = new(key, entry);
+#if NET5_0_OR_GREATER
+		cache.TryRemove(observed);
+#else
+		((ICollection<KeyValuePair<TKey, CacheEntry>>)cache).Remove(observed);
+#endif
+	}
+
 	private sealed class CacheEntry(TValue value, DateTime? expiration)
 	{
 		public TValue Value { get; } = value;

Essentials.Tests/CacheProviderTests.cs

@@ -8,6 +8,7 @@ namespace ktsu.Essentials.Tests;
 using System.Threading.Tasks;
 using ktsu.Essentials;
 using ktsu.Essentials.All;
+using ktsu.Essentials.CacheProviders.InMemory;
 using Microsoft.Extensions.DependencyInjection;
 using Microsoft.VisualStudio.TestTools.UnitTesting;
 
@@ -202,4 +203,77 @@ public void Cache_No_Expiration_Persists()
 		Assert.IsTrue(found, "Should find entry without expiration");
 		Assert.AreEqual(42, value);
 	}
+
+	[TestMethod]
+	public void Cache_Expired_TryGet_Does_Not_Remove_A_Value_Set_Concurrently()
+	{
+		InMemoryCacheProvider<InterleavingKey, int> cache = new();
+		InterleavingKey key = new();
+
+		cache.Set(key, -1, TimeSpan.FromTicks(-1));
+
+		// TryGet hashes the key once to read the expired entry and again to remove it. Setting a fresh
+		// value between the two reproduces another thread's Set landing inside that window.
+		key.OnSecondHash(() => cache.Set(key, 2));
+		bool foundExpired = cache.TryGet(key, out _);
+
+		Assert.IsFalse(foundExpired, "The entry TryGet read had expired");
+		Assert.IsTrue(key.Interleaved, "The concurrent Set should have run between the read and the remove");
+		Assert.IsTrue(cache.TryGet(key, out int value), "The value set concurrently should survive the expired entry's removal");
+		Assert.AreEqual(2, value);
+	}
+
+	[TestMethod]
+	public void Cache_Set_With_MaxValue_Expiration_Never_Expires()
+	{
+		ICacheProvider<string, int> cache = CreateCache();
+
+		cache.Set("key", 42, TimeSpan.MaxValue);
+
+		Assert.IsTrue(cache.TryGet("key", out int value), "An entry with the largest time-to-live should be found");
+		Assert.AreEqual(42, value);
+	}
+
+	[TestMethod]
+	public void Cache_Set_With_MinValue_Expiration_Has_Already_Expired()
+	{
+		ICacheProvider<string, int> cache = CreateCache();
+
+		cache.Set("key", 42, TimeSpan.MinValue);
+
+		Assert.IsFalse(cache.TryGet("key", out _), "An entry with the most negative time-to-live has already expired");
+	}
+
+	/// <summary>
+	/// A key that runs an action when it is hashed for the second time after being armed, which lets a
+	/// test interleave work between the lookups a single cache call makes.
+	/// </summary>
+	private sealed class InterleavingKey
+	{
+		private Action? pending;
+		private int hashesUntilAction;
+
+		public bool Interleaved { get; private set; }
+
+		public void OnSecondHash(Action action)
+		{
+			pending = action;
+			hashesUntilAction = 2;
+		}
+
+		public override int GetHashCode()
+		{
+			if (pending is not null && --hashesUntilAction == 0)
+			{
+				Action action = pending;
+				pending = null;
+				action();
+				Interleaved = true;
+			}
+
+			return 17;
+		}
+
+		public override bool Equals(object? obj) => ReferenceEquals(this, obj);
+	}
 }