Forward the client's refspec on cached lock walks and probes

GitLfsCache.Tests/Integration/LockListCachingTests.cs

@@ -215,6 +215,41 @@ public async Task CreatingALock_InvalidatesTheSnapshot()
 		Assert.HasCount(2, (await ListLocksAsync(fixture))["locks"]!.AsArray());
 	}
 
+	[TestMethod]
+	[DataRow("?refspec=refs/heads/main", "refspec=refs%2Fheads%2Fmain", DisplayName = "With a refspec")]
+	[DataRow("", null, DisplayName = "Without a refspec")]
+	public async Task CachedListing_ForwardsTheClientsRefspecOnTheWalkAndTheProbe(string query, string? forwarded)
+	{
+		// The snapshot is keyed by ref because upstream may answer differently per ref, so what fills it
+		// and what admits a caller to it have to ask upstream about that same ref.
+		await using ProxyFixture fixture = await ProxyFixture.StartAsync();
+		fixture.Upstream.Locks.Add("a");
+
+		await ListLocksAsync(fixture, query, Credential);
+		await ListLocksAsync(fixture, query, OtherCredential);
+
+		StubUpstream.RecordedRequest[] listings =
+		[
+			.. fixture.Upstream.Requests.Where(request => request.Path.EndsWith("/locks", StringComparison.Ordinal)),
+		];
+
+		// The first caller's walk and the second caller's probe.
+		Assert.HasCount(2, listings);
+		Assert.Contains("limit=1", listings[1].Query, "The second request should be the one-page probe.");
+
+		foreach (StubUpstream.RecordedRequest listing in listings)
+		{
+			if (forwarded is null)
+			{
+				Assert.DoesNotContain("refspec", listing.Query);
+			}
+			else
+			{
+				Assert.Contains(forwarded, listing.Query);
+			}
+		}
+	}
+
 	[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")]

GitLfsCache.Tests/Integration/StubUpstream.cs

@@ -91,7 +91,8 @@ protected override async Task<HttpResponseMessage> SendAsync(
 				request.Headers.TryGetValues("Authorization", out IEnumerable<string>? authorization)
 					? string.Join(",", authorization)
 					: null,
-				request.Headers.Range?.ToString()));
+				request.Headers.Range?.ToString(),
+				request.RequestUri!.Query));
 		}
 
 		if (path.EndsWith("/unlock", StringComparison.Ordinal))
@@ -431,5 +432,6 @@ private HttpResponseMessage BuildUploadResponse(
 	/// <param name="Path">The absolute path.</param>
 	/// <param name="Authorization">The Authorization header, or null when absent.</param>
 	/// <param name="Range">The Range header, or null when absent.</param>
-	internal sealed record RecordedRequest(string Method, string Path, string? Authorization, string? Range);
+	/// <param name="Query">The query string, including its leading <c>?</c>, or empty when absent.</param>
+	internal sealed record RecordedRequest(string Method, string Path, string? Authorization, string? Range, string Query);
 }

GitLfsCache.Tests/Locks/CredentialAdmissionTests.cs

@@ -33,51 +33,51 @@ public void IsAdmitted_BeforeAnyUpstreamSuccess_IsFalse()
 		// The whole point: nothing is admitted until upstream actually said yes.
 		(CredentialAdmission admission, _) = Build();
 
-		Assert.IsFalse(admission.IsAdmitted("github", Repository, Credential));
+		Assert.IsFalse(admission.IsAdmitted("github", Repository, null, Credential));
 	}
 
 	[TestMethod]
 	public void IsAdmitted_AfterAdmit_IsTrue()
 	{
 		(CredentialAdmission admission, _) = Build();
 
-		admission.Admit("github", Repository, Credential);
+		admission.Admit("github", Repository, null, Credential);
 
-		Assert.IsTrue(admission.IsAdmitted("github", Repository, Credential));
+		Assert.IsTrue(admission.IsAdmitted("github", Repository, null, Credential));
 	}
 
 	[TestMethod]
 	public void IsAdmitted_AfterTheTtl_IsFalseAgain()
 	{
 		(CredentialAdmission admission, FakeTimeProvider time) = Build(TimeSpan.FromMinutes(1));
 
-		admission.Admit("github", Repository, Credential);
+		admission.Admit("github", Repository, null, Credential);
 		time.Advance(TimeSpan.FromMinutes(1));
 
 		// This is the window in which a credential revoked upstream still reads listings. It has to
 		// actually close.
-		Assert.IsFalse(admission.IsAdmitted("github", Repository, Credential));
+		Assert.IsFalse(admission.IsAdmitted("github", Repository, null, Credential));
 	}
 
 	[TestMethod]
 	public void IsAdmitted_JustBeforeTheTtl_IsStillTrue()
 	{
 		(CredentialAdmission admission, FakeTimeProvider time) = Build(TimeSpan.FromMinutes(1));
 
-		admission.Admit("github", Repository, Credential);
+		admission.Admit("github", Repository, null, Credential);
 		time.Advance(TimeSpan.FromSeconds(59));
 
-		Assert.IsTrue(admission.IsAdmitted("github", Repository, Credential));
+		Assert.IsTrue(admission.IsAdmitted("github", Repository, null, Credential));
 	}
 
 	[TestMethod]
 	public void IsAdmitted_ADifferentCredential_IsNotAdmitted()
 	{
 		(CredentialAdmission admission, _) = Build();
 
-		admission.Admit("github", Repository, Credential);
+		admission.Admit("github", Repository, null, Credential);
 
-		Assert.IsFalse(admission.IsAdmitted("github", Repository, "Basic c29tZW9uZTplbHNl"));
+		Assert.IsFalse(admission.IsAdmitted("github", Repository, null, "Basic c29tZW9uZTplbHNl"));
 	}
 
 	[TestMethod]
@@ -86,19 +86,35 @@ public void IsAdmitted_ADifferentRepository_IsNotAdmitted()
 		// Admission is per repository. Read access to one proves nothing about another.
 		(CredentialAdmission admission, _) = Build();
 
-		admission.Admit("github", Repository, Credential);
+		admission.Admit("github", Repository, null, Credential);
 
-		Assert.IsFalse(admission.IsAdmitted("github", "owner/other.git/info/lfs", Credential));
+		Assert.IsFalse(admission.IsAdmitted("github", "owner/other.git/info/lfs", null, Credential));
 	}
 
 	[TestMethod]
 	public void IsAdmitted_ADifferentUpstream_IsNotAdmitted()
 	{
 		(CredentialAdmission admission, _) = Build();
 
-		admission.Admit("github", Repository, Credential);
+		admission.Admit("github", Repository, null, Credential);
 
-		Assert.IsFalse(admission.IsAdmitted("ado", Repository, Credential));
+		Assert.IsFalse(admission.IsAdmitted("ado", Repository, null, Credential));
+	}
+
+	[TestMethod]
+	[DataRow("refs/heads/feature", DisplayName = "Another ref")]
+	[DataRow(null, DisplayName = "No ref")]
+	[DataRow("", DisplayName = "An empty ref")]
+	public void IsAdmitted_ADifferentRef_IsNotAdmitted(string? reference)
+	{
+		// The locking API treats the ref as an authentication input, so upstream accepting a
+		// credential under one ref says nothing about another.
+		(CredentialAdmission admission, _) = Build();
+
+		admission.Admit("github", Repository, "refs/heads/main", Credential);
+
+		Assert.IsTrue(admission.IsAdmitted("github", Repository, "refs/heads/main", Credential));
+		Assert.IsFalse(admission.IsAdmitted("github", Repository, reference, Credential));
 	}
 
 	[TestMethod]
@@ -108,9 +124,9 @@ public void Key_CannotBeConfusedAcrossFields()
 		// "b" would hash identically and one repository's admission would serve another's.
 		(CredentialAdmission admission, _) = Build();
 
-		admission.Admit("github", "a/b", Credential);
+		admission.Admit("github", "a/b", null, Credential);
 
-		Assert.IsFalse(admission.IsAdmitted("github/a", "b", Credential));
+		Assert.IsFalse(admission.IsAdmitted("github/a", "b", null, Credential));
 	}
 
 	[TestMethod]
@@ -121,21 +137,21 @@ public void IsAdmitted_NoCredential_IsNeverAdmitted(string? authorization)
 		// Admitting an anonymous caller would mean serving a listing to someone who proved nothing.
 		(CredentialAdmission admission, _) = Build();
 
-		admission.Admit("github", Repository, authorization);
+		admission.Admit("github", Repository, null, authorization);
 
-		Assert.IsFalse(admission.IsAdmitted("github", Repository, authorization));
+		Assert.IsFalse(admission.IsAdmitted("github", Repository, null, authorization));
 	}
 
 	[TestMethod]
 	public void Admit_Twice_ExtendsTheWindowFromTheSecondTime()
 	{
 		(CredentialAdmission admission, FakeTimeProvider time) = Build(TimeSpan.FromMinutes(1));
 
-		admission.Admit("github", Repository, Credential);
+		admission.Admit("github", Repository, null, Credential);
 		time.Advance(TimeSpan.FromSeconds(50));
-		admission.Admit("github", Repository, Credential);
+		admission.Admit("github", Repository, null, Credential);
 		time.Advance(TimeSpan.FromSeconds(50));
 
-		Assert.IsTrue(admission.IsAdmitted("github", Repository, Credential));
+		Assert.IsTrue(admission.IsAdmitted("github", Repository, null, Credential));
 	}
 }

GitLfsCache/Locks/CredentialAdmission.cs

@@ -3,6 +3,7 @@
 namespace ktsu.GitLfsCache.Locks;
 
 using System.Collections.Concurrent;
+using System.Globalization;
 using System.Security.Cryptography;
 using System.Text;
 using ktsu.GitLfsCache.Configuration;
@@ -42,7 +43,7 @@ public sealed class CredentialAdmission(
 	private readonly Lock _hashGate = new();
 
 	/// <inheritdoc />
-	public bool IsAdmitted(string upstream, string repositoryPath, string? authorization)
+	public bool IsAdmitted(string upstream, string repositoryPath, string? reference, string? authorization)
 	{
 		// An anonymous caller is never admitted. Upstream would refuse it, and admitting it here would
 		// mean a listing served to someone who never proved anything.
@@ -51,7 +52,7 @@ public bool IsAdmitted(string upstream, string repositoryPath, string? authoriza
 			return false;
 		}
 
-		string key = Key(upstream, repositoryPath, authorization);
+		string key = Key(upstream, repositoryPath, reference, authorization);
 
 		if (!_admitted.TryGetValue(key, out DateTimeOffset expiry))
 		{
@@ -70,14 +71,14 @@ public bool IsAdmitted(string upstream, string repositoryPath, string? authoriza
 	}
 
 	/// <inheritdoc />
-	public void Admit(string upstream, string repositoryPath, string? authorization)
+	public void Admit(string upstream, string repositoryPath, string? reference, string? authorization)
 	{
 		if (string.IsNullOrEmpty(authorization))
 		{
 			return;
 		}
 
-		_admitted[Key(upstream, repositoryPath, authorization)] =
+		_admitted[Key(upstream, repositoryPath, reference, authorization)] =
 			timeProvider.GetUtcNow() + options.Value.Locks.AdmissionTtl;
 
 		if (_admitted.Count > SweepThreshold)
@@ -106,13 +107,18 @@ private void Sweep()
 	/// Derives the entry key for one credential and repository.
 	/// </summary>
 	/// <remarks>
-	/// The three parts are separated by a character that cannot appear in an upstream key, so no two
-	/// different triples can produce the same input. Without that, an upstream and repository could be
+	/// The parts are separated by a character that cannot appear in an upstream key, so no two
+	/// different combinations can produce the same input. Without that, an upstream and repository could be
 	/// re-split to match a different pair.
 	/// </remarks>
-	private string Key(string upstream, string repositoryPath, string authorization)
+	private string Key(string upstream, string repositoryPath, string? reference, string authorization)
 	{
-		byte[] input = Encoding.UTF8.GetBytes($"{upstream}\n{repositoryPath}\n{authorization}");
+		// The ref goes last and length-prefixed, since unlike the other parts it comes straight from a
+		// query string and may contain the separator. -1 keeps "no ref" apart from an empty one.
+		string scope = reference is null
+			? "-1:"
+			: string.Create(CultureInfo.InvariantCulture, $"{reference.Length}:{reference}");
+		byte[] input = Encoding.UTF8.GetBytes($"{upstream}\n{repositoryPath}\n{authorization}\n{scope}");
 
 		// HMACSHA256 holds mutable state across ComputeHash, so one shared instance needs a gate. The
 		// alternative, an instance per call, allocates on a path taken on every lock request.

GitLfsCache/Locks/ICredentialAdmission.cs

@@ -23,9 +23,13 @@ public interface ICredentialAdmission
 	/// </summary>
 	/// <param name="upstream">The upstream key.</param>
 	/// <param name="repositoryPath">The repository the credential was accepted for.</param>
+	/// <param name="reference">
+	/// The ref the credential was presented with, or null for none. Upstream may accept a credential
+	/// for one ref and refuse it for another, so an admission for one does not carry over.
+	/// </param>
 	/// <param name="authorization">The client's Authorization header, exactly as sent.</param>
 	/// <returns><see langword="true"/> when an unexpired admission exists.</returns>
-	public bool IsAdmitted(string upstream, string repositoryPath, string? authorization);
+	public bool IsAdmitted(string upstream, string repositoryPath, string? reference, string? authorization);
 
 	/// <summary>
 	/// Records that upstream accepted this credential for this repository.
@@ -36,6 +40,10 @@ public interface ICredentialAdmission
 	/// </remarks>
 	/// <param name="upstream">The upstream key.</param>
 	/// <param name="repositoryPath">The repository the credential was accepted for.</param>
+	/// <param name="reference">
+	/// The ref the credential was presented with, or null for none. Upstream may accept a credential
+	/// for one ref and refuse it for another, so an admission for one does not carry over.
+	/// </param>
 	/// <param name="authorization">The client's Authorization header, exactly as sent.</param>
-	public void Admit(string upstream, string repositoryPath, string? authorization);
+	public void Admit(string upstream, string repositoryPath, string? reference, string? authorization);
 }

GitLfsCache/Locks/LockListRefresher.cs

@@ -52,6 +52,7 @@ public async Task<LockRefreshResult> ProbeAsync(
 		using HttpRequestMessage request = UpstreamRequests.BuildLockListRequest(
 			upstreamBase,
 			key.RepositoryPath,
+			key.Ref,
 			cursor: null,
 			limit: 1,
 			authorization);
@@ -83,6 +84,7 @@ public async Task<LockRefreshResult> RefreshAsync(
 			using HttpRequestMessage request = UpstreamRequests.BuildLockListRequest(
 				upstreamBase,
 				key.RepositoryPath,
+				key.Ref,
 				cursor,
 				limit: null,
 				authorization);

GitLfsCache/Locks/LockListService.cs

@@ -65,7 +65,7 @@ public async Task<LockListOutcome> ResolveAsync(
 
 		// A caller already admitted, with a snapshot still inside its lifetime, is the steady state and
 		// costs upstream nothing at all. This is the whole point of the subsystem.
-		if (usable && admission.IsAdmitted(key.Upstream, key.RepositoryPath, authorization))
+		if (usable && admission.IsAdmitted(key.Upstream, key.RepositoryPath, key.Ref, authorization))
 		{
 			metrics.RecordLockListHit(key.Upstream);
 			return LockListOutcome.Serve(current!);
@@ -110,7 +110,7 @@ private async Task<LockListOutcome> ProbeAsync(
 			return LockListOutcome.Relay();
 		}
 
-		admission.Admit(key.Upstream, key.RepositoryPath, authorization);
+		admission.Admit(key.Upstream, key.RepositoryPath, key.Ref, authorization);
 		return LockListOutcome.Serve(current);
 	}
 
@@ -155,7 +155,7 @@ private async Task<LockListOutcome> RefreshAsync(
 				snapshots.Publish(key, result.Snapshot!);
 
 				// The walk succeeding is itself upstream's answer that this caller may read these locks.
-				admission.Admit(key.Upstream, key.RepositoryPath, authorization);
+				admission.Admit(key.Upstream, key.RepositoryPath, key.Ref, authorization);
 				ticket.Complete(published: true);
 				return LockListOutcome.Serve(result.Snapshot!);
 

GitLfsCache/Upstreams/UpstreamRequests.cs

@@ -84,13 +84,19 @@ public static HttpRequestMessage BuildBatchRequest(
 	/// </remarks>
 	/// <param name="upstreamBase">The configured upstream base URL.</param>
 	/// <param name="repositoryPath">The path between the upstream key and <c>/locks</c>.</param>
+	/// <param name="refspec">
+	/// The ref the client listed under, forwarded as <c>refspec</c>, or null when it sent none. The
+	/// locking API treats the ref as an authentication input, so a walk or probe made on a client's
+	/// behalf has to present the same one the client did.
+	/// </param>
 	/// <param name="cursor">Upstream's cursor for the page to fetch, or null for the first.</param>
 	/// <param name="limit">A page size to request, or null to let upstream choose.</param>
 	/// <param name="authorization">The client's Authorization header, forwarded unchanged.</param>
 	/// <returns>The request to send upstream.</returns>
 	public static HttpRequestMessage BuildLockListRequest(
 		Uri upstreamBase,
 		string repositoryPath,
+		string? refspec,
 		string? cursor,
 		int? limit,
 		string? authorization)
@@ -100,6 +106,11 @@ public static HttpRequestMessage BuildLockListRequest(
 
 		List<string> query = [];
 
+		if (refspec is not null)
+		{
+			query.Add($"refspec={Uri.EscapeDataString(refspec)}");
+		}
+
 		if (!string.IsNullOrEmpty(cursor))
 		{
 			query.Add($"cursor={Uri.EscapeDataString(cursor)}");