diff --git a/GitLfsCache.Tests/Configuration/GitLfsCacheOptionsValidatorTests.cs b/GitLfsCache.Tests/Configuration/GitLfsCacheOptionsValidatorTests.cs index 250db36..028a840 100644 --- a/GitLfsCache.Tests/Configuration/GitLfsCacheOptionsValidatorTests.cs +++ b/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() { diff --git a/GitLfsCache.Tests/Locks/LockSnapshotStoreTests.cs b/GitLfsCache.Tests/Locks/LockSnapshotStoreTests.cs new file mode 100644 index 0000000..c8f2cba --- /dev/null +++ b/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)); + } +} diff --git a/GitLfsCache/Configuration/GitLfsCacheOptionsValidator.cs b/GitLfsCache/Configuration/GitLfsCacheOptionsValidator.cs index a215a36..87f2b62 100644 --- a/GitLfsCache/Configuration/GitLfsCacheOptionsValidator.cs +++ b/GitLfsCache/Configuration/GitLfsCacheOptionsValidator.cs @@ -214,6 +214,11 @@ private static void ValidateLocks(LocksOptions locks, List 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."); diff --git a/GitLfsCache/Configuration/LocksOptions.cs b/GitLfsCache/Configuration/LocksOptions.cs index 1bf9677..d0596ba 100644 --- a/GitLfsCache/Configuration/LocksOptions.cs +++ b/GitLfsCache/Configuration/LocksOptions.cs @@ -51,6 +51,17 @@ public sealed class LocksOptions /// public int MaxSnapshotLocks { get; set; } = 100_000; + /// + /// Gets or sets the most lock snapshots held in memory at once. + /// + /// + /// 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 are dropped first, then the oldest of the + /// rest. Memory is bounded by this times . + /// + public int MaxSnapshots { get; set; } = 1000; + /// /// Gets or sets how many lock calls may be in flight against one upstream at a time. /// diff --git a/GitLfsCache/Locks/LockSnapshotStore.cs b/GitLfsCache/Locks/LockSnapshotStore.cs index f6fd868..d2f45f8 100644 --- a/GitLfsCache/Locks/LockSnapshotStore.cs +++ b/GitLfsCache/Locks/LockSnapshotStore.cs @@ -3,19 +3,39 @@ namespace ktsu.GitLfsCache.Locks; using System.Collections.Concurrent; +using ktsu.GitLfsCache.Configuration; +using Microsoft.Extensions.Options; /// -/// In-memory snapshot store, one entry per repository, replaced on publish. +/// In-memory snapshot store, one entry per repository and ref, replaced on publish. /// /// /// 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. +/// +/// 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 +/// , which would be refetched before being served anyway, and then +/// drops the oldest survivors until at most 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. +/// /// -public sealed class LockSnapshotStore : ILockSnapshotStore +/// The configured options. +/// Clock, injected so expiry is testable. +public sealed class LockSnapshotStore( + IOptions options, + TimeProvider timeProvider) : ILockSnapshotStore { private readonly ConcurrentDictionary _snapshots = new(); + // Serializes eviction only. Reads never take it, and publishes are rare next to reads. + private readonly Lock _evictionGate = new(); + + /// Gets how many snapshots the store currently holds. + public int Count => _snapshots.Count; + /// public LockSnapshot? Read(LockSnapshotKey key) { @@ -30,6 +50,8 @@ public void Publish(LockSnapshotKey key, LockSnapshot snapshot) Ensure.NotNull(snapshot); _snapshots[key] = snapshot; + + Evict(key); } /// @@ -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> survivors = []; + + foreach (KeyValuePair 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 entry in survivors + .OrderBy(entry => entry.Value.TakenAt) + .Take(excess)) + { + _snapshots.TryRemove(entry); + } + } + } } diff --git a/README.md b/README.md index 7547d25..8727a0e 100644 --- a/README.md +++ b/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. |