Evict stale and excess lock snapshots [minor]

GitLfsCache.Tests/Configuration/GitLfsCacheOptionsValidatorTests.cs

@@ -295,6 +295,19 @@ public void Validate_NonPositiveMaxSnapshotLocks_Fails()
 		Assert.Contains("Locks:MaxSnapshotLocks", result.FailureMessage);
 	}
 
+	[TestMethod]
+	public void Validate_NonPositiveMaxSnapshots_Fails()
+	{
+		GitLfsCacheOptions options = Valid();
+		options.Locks.MaxSnapshots = 0;
+
+		ValidateOptionsResult result = Validate(options);
+
+		Assert.IsFalse(result.Succeeded);
+		Assert.IsNotNull(result.FailureMessage);
+		Assert.Contains("Locks:MaxSnapshots", result.FailureMessage);
+	}
+
 	[TestMethod]
 	public void Validate_UpstreamWithNoRepositories_FailsNamingTheWildcard()
 	{

GitLfsCache.Tests/Locks/LockSnapshotStoreTests.cs

@@ -0,0 +1,121 @@
+// Copyright (c) 2023-2026 ktsu-dev contributors
+
+namespace ktsu.GitLfsCache.Tests.Locks;
+
+using System.Globalization;
+using ktsu.GitLfsCache.Configuration;
+using ktsu.GitLfsCache.Locks;
+using Microsoft.Extensions.Options;
+using Microsoft.Extensions.Time.Testing;
+using Microsoft.VisualStudio.TestTools.UnitTesting;
+
+[TestClass]
+public class LockSnapshotStoreTests
+{
+	private const string Upstream = "github";
+	private const string Repository = "owner/repo.git/info/lfs";
+
+	private static readonly TimeSpan ListTtl = TimeSpan.FromSeconds(15);
+
+	private static (LockSnapshotStore Store, FakeTimeProvider Time) Build(int maxSnapshots = 1000)
+	{
+		GitLfsCacheOptions options = new()
+		{
+			Locks = new LocksOptions { ListTtl = ListTtl, MaxSnapshots = maxSnapshots },
+		};
+
+		FakeTimeProvider time = new(new DateTimeOffset(2026, 8, 19, 9, 47, 0, TimeSpan.Zero));
+
+		return (new LockSnapshotStore(Options.Create(options), time), time);
+	}
+
+	private static LockSnapshotKey Key(int branch) =>
+		new(Upstream, Repository, string.Create(CultureInfo.InvariantCulture, $"refs/heads/b{branch}"));
+
+	private static LockSnapshot Snapshot(FakeTimeProvider time) => new([], time.GetUtcNow());
+
+	[TestMethod]
+	public void Publish_AfterManyRefsHaveOutlivedTheListTtl_DropsEveryStaleSnapshot()
+	{
+		// The ref comes from the client's query string, so a client can name as many as it likes.
+		(LockSnapshotStore store, FakeTimeProvider time) = Build();
+
+		for (int branch = 0; branch < 200; branch++)
+		{
+			store.Publish(Key(branch), Snapshot(time));
+		}
+
+		Assert.AreEqual(200, store.Count);
+
+		time.Advance(ListTtl);
+		store.Publish(Key(200), Snapshot(time));
+
+		Assert.AreEqual(1, store.Count);
+		Assert.IsNull(store.Read(Key(0)));
+		Assert.IsNull(store.Read(Key(199)));
+		Assert.IsNotNull(store.Read(Key(200)));
+	}
+
+	[TestMethod]
+	public void Publish_BeyondMaxSnapshots_EvictsTheOldestFirst()
+	{
+		(LockSnapshotStore store, FakeTimeProvider time) = Build(maxSnapshots: 3);
+
+		for (int branch = 0; branch < 5; branch++)
+		{
+			store.Publish(Key(branch), Snapshot(time));
+			time.Advance(TimeSpan.FromSeconds(1));
+		}
+
+		Assert.AreEqual(3, store.Count);
+		Assert.IsNull(store.Read(Key(0)));
+		Assert.IsNull(store.Read(Key(1)));
+		Assert.IsNotNull(store.Read(Key(2)));
+		Assert.IsNotNull(store.Read(Key(3)));
+		Assert.IsNotNull(store.Read(Key(4)));
+	}
+
+	[TestMethod]
+	public void Publish_ASnapshotThatIsAlreadyOld_KeepsIt()
+	{
+		// A long walk can finish with a snapshot that is already past its lifetime. Evicting it on the
+		// way in would throw away the work that produced it.
+		(LockSnapshotStore store, FakeTimeProvider time) = Build(maxSnapshots: 1);
+		LockSnapshot old = Snapshot(time);
+		time.Advance(ListTtl * 2);
+
+		store.Publish(Key(0), old);
+
+		Assert.AreSame(old, store.Read(Key(0)));
+	}
+
+	[TestMethod]
+	public void Publish_RepublishingOneKey_HoldsOneSnapshot()
+	{
+		(LockSnapshotStore store, FakeTimeProvider time) = Build();
+
+		store.Publish(Key(0), Snapshot(time));
+		LockSnapshot latest = Snapshot(time);
+		store.Publish(Key(0), latest);
+
+		Assert.AreEqual(1, store.Count);
+		Assert.AreSame(latest, store.Read(Key(0)));
+	}
+
+	[TestMethod]
+	public void Invalidate_DropsEveryRefOfTheRepositoryOnly()
+	{
+		(LockSnapshotStore store, FakeTimeProvider time) = Build();
+		LockSnapshotKey other = new(Upstream, "owner/other.git/info/lfs", null);
+
+		store.Publish(Key(0), Snapshot(time));
+		store.Publish(Key(1), Snapshot(time));
+		store.Publish(other, Snapshot(time));
+
+		store.Invalidate(Upstream, Repository);
+
+		Assert.IsNull(store.Read(Key(0)));
+		Assert.IsNull(store.Read(Key(1)));
+		Assert.IsNotNull(store.Read(other));
+	}
+}

GitLfsCache/Configuration/GitLfsCacheOptionsValidator.cs

@@ -214,6 +214,11 @@ private static void ValidateLocks(LocksOptions locks, List<string> failures)
 			failures.Add($"{GitLfsCacheOptions.SectionName}:Locks:MaxSnapshotLocks must be greater than zero.");
 		}
 
+		if (locks.MaxSnapshots <= 0)
+		{
+			failures.Add($"{GitLfsCacheOptions.SectionName}:Locks:MaxSnapshots must be greater than zero.");
+		}
+
 		if (locks.MaxFanOutConcurrency <= 0)
 		{
 			failures.Add($"{GitLfsCacheOptions.SectionName}:Locks:MaxFanOutConcurrency must be greater than zero.");

GitLfsCache/Configuration/LocksOptions.cs

@@ -51,6 +51,17 @@ public sealed class LocksOptions
 	/// </remarks>
 	public int MaxSnapshotLocks { get; set; } = 100_000;
 
+	/// <summary>
+	/// Gets or sets the most lock snapshots held in memory at once.
+	/// </summary>
+	/// <remarks>
+	/// One snapshot is held per repository and ref a client has listed, and the ref comes from the
+	/// client, so without a ceiling the number of snapshots would grow with every branch ever asked
+	/// about. Snapshots older than <see cref="ListTtl"/> are dropped first, then the oldest of the
+	/// rest. Memory is bounded by this times <see cref="MaxSnapshotLocks"/>.
+	/// </remarks>
+	public int MaxSnapshots { get; set; } = 1000;
+
 	/// <summary>
 	/// Gets or sets how many lock calls may be in flight against one upstream at a time.
 	/// </summary>

GitLfsCache/Locks/LockSnapshotStore.cs

@@ -3,19 +3,39 @@
 namespace ktsu.GitLfsCache.Locks;
 
 using System.Collections.Concurrent;
+using ktsu.GitLfsCache.Configuration;
+using Microsoft.Extensions.Options;
 
 /// <summary>
-/// In-memory snapshot store, one entry per repository, replaced on publish.
+/// In-memory snapshot store, one entry per repository and ref, replaced on publish.
 /// </summary>
 /// <remarks>
 /// Replacement rather than mutation is what makes a snapshot safe to hand to many concurrent readers
 /// without a lock: a reader holds the instance it took, and a publish landing underneath it changes
 /// nothing that reader can see.
+/// <para>
+/// The ref in the key comes straight from the client's query string, so the number of keys is not
+/// bounded by anything the operator controls. Every publish therefore drops snapshots older than
+/// <see cref="LocksOptions.ListTtl"/>, which would be refetched before being served anyway, and then
+/// drops the oldest survivors until at most <see cref="LocksOptions.MaxSnapshots"/> remain.
+/// Eviction happens on publish rather than on a timer because a publish is the only thing that
+/// grows the store, and it follows a full upstream walk, so a scan of the store is noise beside it.
+/// </para>
 /// </remarks>
-public sealed class LockSnapshotStore : ILockSnapshotStore
+/// <param name="options">The configured options.</param>
+/// <param name="timeProvider">Clock, injected so expiry is testable.</param>
+public sealed class LockSnapshotStore(
+	IOptions<GitLfsCacheOptions> options,
+	TimeProvider timeProvider) : ILockSnapshotStore
 {
 	private readonly ConcurrentDictionary<LockSnapshotKey, LockSnapshot> _snapshots = new();
 
+	// Serializes eviction only. Reads never take it, and publishes are rare next to reads.
+	private readonly Lock _evictionGate = new();
+
+	/// <summary>Gets how many snapshots the store currently holds.</summary>
+	public int Count => _snapshots.Count;
+
 	/// <inheritdoc />
 	public LockSnapshot? Read(LockSnapshotKey key)
 	{
@@ -30,6 +50,8 @@ public void Publish(LockSnapshotKey key, LockSnapshot snapshot)
 		Ensure.NotNull(snapshot);
 
 		_snapshots[key] = snapshot;
+
+		Evict(key);
 	}
 
 	/// <inheritdoc />
@@ -45,4 +67,52 @@ public void Invalidate(string upstream, string repositoryPath)
 			_snapshots.TryRemove(key, out _);
 		}
 	}
+
+	private void Evict(LockSnapshotKey published)
+	{
+		LocksOptions locks = options.Value.Locks;
+		DateTimeOffset now = timeProvider.GetUtcNow();
+
+		lock (_evictionGate)
+		{
+			List<KeyValuePair<LockSnapshotKey, LockSnapshot>> survivors = [];
+
+			foreach (KeyValuePair<LockSnapshotKey, LockSnapshot> entry in _snapshots)
+			{
+				// The snapshot just published is never a candidate, even when its walk took long
+				// enough to make it look stale or old: evicting it would throw away the work that
+				// triggered this call.
+				if (entry.Key.Equals(published))
+				{
+					continue;
+				}
+
+				// Removal is conditional on the value, so a snapshot republished under the same key
+				// since the scan began is left alone.
+				if (entry.Value.IsStale(now, locks.ListTtl))
+				{
+					_snapshots.TryRemove(entry);
+				}
+				else
+				{
+					survivors.Add(entry);
+				}
+			}
+
+			// One slot is the snapshot just published.
+			int excess = survivors.Count + 1 - locks.MaxSnapshots;
+
+			if (excess <= 0)
+			{
+				return;
+			}
+
+			foreach (KeyValuePair<LockSnapshotKey, LockSnapshot> entry in survivors
+				.OrderBy(entry => entry.Value.TakenAt)
+				.Take(excess))
+			{
+				_snapshots.TryRemove(entry);
+			}
+		}
+	}
 }

README.md

@@ -139,6 +139,7 @@ The flags are a convenience over the same settings and win over all three, so `-
 | `Locks:AdmissionTtl` | How long an upstream authorization is trusted before it is proven again. Must be at least `ListTtl`, and startup refuses otherwise. |
 | `Locks:RefreshTimeout` | How long a request waits for another request's listing walk before walking itself. |
 | `Locks:MaxSnapshotLocks` | Above this many locks a repository is relayed rather than cached, so one enormous repository cannot consume memory without bound. |
+| `Locks:MaxSnapshots` | The most lock listings held in memory at once, one per repository and ref a client has listed. Listings older than `ListTtl` are dropped first, then the oldest of the rest. |
 | `Locks:MaxFanOutConcurrency` | How many lock calls may be in flight against one upstream at a time, across every request in the process. The right value per forge has to be found by measurement. |
 | `Locks:MaxFanOutPaths` | The most paths one batched request may carry. Beyond this the request is refused rather than accepted and throttled part way through. |
 | `Locks:MaxFanOutRetries` | How many times a throttled lock call is retried before it is reported as failed. |