diff --git a/GitLfsCache.Tests/Integration/LockListCachingTests.cs b/GitLfsCache.Tests/Integration/LockListCachingTests.cs index 01e30c6..a4987d8 100644 --- a/GitLfsCache.Tests/Integration/LockListCachingTests.cs +++ b/GitLfsCache.Tests/Integration/LockListCachingTests.cs @@ -215,6 +215,43 @@ public async Task CreatingALock_InvalidatesTheSnapshot() Assert.HasCount(2, (await ListLocksAsync(fixture))["locks"]!.AsArray()); } + [TestMethod] + [DataRow("/locks", "{\"path\":\"b\",\"ref\":{\"name\":\"refs/heads/main\"}}", 2, DisplayName = "Create")] + [DataRow("/locks/1/unlock", "{\"ref\":{\"name\":\"refs/heads/main\"}}", 0, DisplayName = "Unlock")] + public async Task ChangingALock_InvalidatesASnapshotListedUnderARefspec(string change, string body, int expected) + { + // git-lfs lists with ?refspec= but carries the ref of a create or unlock in the body, so the + // change never names the snapshot the listing was cached under. + await using ProxyFixture fixture = await ProxyFixture.StartAsync(); + fixture.Upstream.Locks.Add("a"); + const string Refspec = "?refspec=refs/heads/main"; + + Assert.HasCount(1, (await ListLocksAsync(fixture, Refspec))["locks"]!.AsArray()); + + if (expected > 1) + { + fixture.Upstream.Locks.Add("b"); + } + else + { + fixture.Upstream.Locks.Clear(); + } + + using (HttpClient client = fixture.Client) + { + using HttpRequestMessage request = new(HttpMethod.Post, $"{LfsPath}{change}") + { + Content = new StringContent(body, Encoding.UTF8, "application/vnd.git-lfs+json"), + }; + + request.Headers.TryAddWithoutValidation("Authorization", Credential); + using HttpResponseMessage response = await client.SendAsync(request); + Assert.IsTrue(response.IsSuccessStatusCode, $"{(int)response.StatusCode}"); + } + + Assert.HasCount(expected, (await ListLocksAsync(fixture, Refspec))["locks"]!.AsArray()); + } + [TestMethod] public async Task LocksDisabled_RelaysExactlyAsBefore() { diff --git a/GitLfsCache/Endpoints/LockRouteHandler.cs b/GitLfsCache/Endpoints/LockRouteHandler.cs index daefcbd..a6c8d06 100644 --- a/GitLfsCache/Endpoints/LockRouteHandler.cs +++ b/GitLfsCache/Endpoints/LockRouteHandler.cs @@ -231,10 +231,7 @@ private void InvalidateIfChanged(HttpContext context, LfsRoute route) { if (context.Response.StatusCode is >= 200 and < 300) { - lockSnapshots.Invalidate(new LockSnapshotKey( - route.Upstream, - route.RepositoryPath, - context.Request.Query["refspec"].FirstOrDefault())); + lockSnapshots.Invalidate(route.Upstream, route.RepositoryPath); } } } diff --git a/GitLfsCache/Locks/ILockSnapshotStore.cs b/GitLfsCache/Locks/ILockSnapshotStore.cs index cdb5ad1..d97b88c 100644 --- a/GitLfsCache/Locks/ILockSnapshotStore.cs +++ b/GitLfsCache/Locks/ILockSnapshotStore.cs @@ -26,12 +26,20 @@ public interface ILockSnapshotStore public void Publish(LockSnapshotKey key, LockSnapshot snapshot); /// - /// Drops the snapshot for a repository, so the next read refreshes. + /// Drops every snapshot for a repository, whatever ref it was listed under, so the next read of + /// any of them refreshes. /// /// /// Called after a lock creation or release the proxy relayed successfully. Locks changed outside /// the proxy are not seen here and are bounded only by the listing lifetime. + /// + /// Every ref goes, not only the one the change named. A listing carries its ref in the query + /// string while a create or unlock carries it in the body, and a client can list under one ref + /// and lock under another, so the change cannot reliably name the snapshot it made wrong. Dropping + /// the rest only costs a refetch. + /// /// - /// The repository whose snapshot is now known to be wrong. - public void Invalidate(LockSnapshotKey key); + /// The upstream the repository is served from. + /// The repository whose snapshots are now known to be wrong. + public void Invalidate(string upstream, string repositoryPath); } diff --git a/GitLfsCache/Locks/LockFanOut.cs b/GitLfsCache/Locks/LockFanOut.cs index 15b1f12..a5727bc 100644 --- a/GitLfsCache/Locks/LockFanOut.cs +++ b/GitLfsCache/Locks/LockFanOut.cs @@ -86,7 +86,7 @@ await Parallel.ForAsync( // is about to look at them. if (results.Any(result => JsonValues.Bool(result?["ok"]) == true)) { - snapshots.Invalidate(key); + snapshots.Invalidate(key.Upstream, key.RepositoryPath); } JsonArray array = []; diff --git a/GitLfsCache/Locks/LockSnapshotStore.cs b/GitLfsCache/Locks/LockSnapshotStore.cs index 03777fa..f6fd868 100644 --- a/GitLfsCache/Locks/LockSnapshotStore.cs +++ b/GitLfsCache/Locks/LockSnapshotStore.cs @@ -33,9 +33,16 @@ public void Publish(LockSnapshotKey key, LockSnapshot snapshot) } /// - public void Invalidate(LockSnapshotKey key) + public void Invalidate(string upstream, string repositoryPath) { - Ensure.NotNull(key); - _snapshots.TryRemove(key, out _); + Ensure.NotNull(upstream); + Ensure.NotNull(repositoryPath); + + foreach (LockSnapshotKey key in _snapshots.Keys.Where(key => + string.Equals(key.Upstream, upstream, StringComparison.Ordinal) + && string.Equals(key.RepositoryPath, repositoryPath, StringComparison.Ordinal))) + { + _snapshots.TryRemove(key, out _); + } } }