Serve a Range request for a cached object as 206 instead of the whole object [patch]

GitLfsCache.Tests/Integration/ProxyFlowTests.cs

@@ -303,6 +303,83 @@
 		Assert.AreEqual("bytes=0-3", fixture.Upstream.Requests.Last().Range);
 	}
 
+	/// <summary>Stores an object through a whole download, then returns a fresh download href for it.</summary>
+	private static async Task<string> WarmAsync(ProxyFixture fixture, byte[] content, string oid)
+	{
+		fixture.Upstream.AddObject(oid, content);
+		JsonNode coldBatch = await PostBatchAsync(fixture, "download", oid, content.Length);
+		using HttpClient client = fixture.Client;
+		await client.GetByteArrayAsync(Relative(HrefOf(coldBatch, "download")));
+		Assert.IsTrue(fixture.Store.Exists("github", oid));
+
+		JsonNode warmBatch = await PostBatchAsync(fixture, "download", oid, content.Length);
+		return Relative(HrefOf(warmBatch, "download"));
+	}
+
+	[TestMethod]
+	[DataRow(10L, 19L, 10, 10, DisplayName = "Closed range")]
+	[DataRow(null, 5L, 31, 5, DisplayName = "Suffix range")]
+	[DataRow(30L, null, 30, 6, DisplayName = "Open range")]
+	public async Task Download_RangeRequestOnAHit_ReturnsPartialContentFromTheStore(
+		long? from,
+		long? to,
+		int expectedStart,
+		int expectedLength)
+	{
+		await using ProxyFixture fixture = await ProxyFixture.StartAsync();
+		(byte[] content, string oid) = Object("0123456789abcdefghijklmnopqrstuvwxyz");
+		string href = await WarmAsync(fixture, content, oid);
+		using HttpClient client = fixture.Client;
+
+		using HttpRequestMessage request = new(HttpMethod.Get, href);
+		request.Headers.Range = new System.Net.Http.Headers.RangeHeaderValue(from, to);
+
+		using HttpResponseMessage response = await client.SendAsync(request);
+
+		Assert.AreEqual(HttpStatusCode.PartialContent, response.StatusCode);
+		CollectionAssert.AreEqual(
+			content.Skip(expectedStart).Take(expectedLength).ToArray(),
+			await response.Content.ReadAsByteArrayAsync());
+		Assert.AreEqual(
+			$"bytes {expectedStart}-{expectedStart + expectedLength - 1}/{content.Length}",
+			response.Content.Headers.ContentRange?.ToString());
+		Assert.AreEqual(1, fixture.Upstream.FetchCount(oid), "A ranged hit must be served from the store");
+	}
+
+	[TestMethod]
+	public async Task Download_UnsatisfiableRangeOnAHit_Returns416()
+	{
+		await using ProxyFixture fixture = await ProxyFixture.StartAsync();
+		(byte[] content, string oid) = Object("too short for that range");
+		string href = await WarmAsync(fixture, content, oid);
+		using HttpClient client = fixture.Client;
+
+		using HttpRequestMessage request = new(HttpMethod.Get, href);
+		request.Headers.Range = new System.Net.Http.Headers.RangeHeaderValue(1000, 2000);
+
+		using HttpResponseMessage response = await client.SendAsync(request);
+
+		Assert.AreEqual(HttpStatusCode.RequestedRangeNotSatisfiable, response.StatusCode);
+		Assert.AreEqual($"bytes */{content.Length}", response.Content.Headers.ContentRange?.ToString());
+	}
+
+	[TestMethod]
+	public async Task Download_HitWithoutARange_ReturnsTheWholeObjectAndAdvertisesRanges()
+	{
+		await using ProxyFixture fixture = await ProxyFixture.StartAsync();
+		(byte[] content, string oid) = Object("whole object please");
+		string href = await WarmAsync(fixture, content, oid);
+		using HttpClient client = fixture.Client;
+
+		using HttpResponseMessage response = await client.GetAsync(href);
+
+		Assert.AreEqual(HttpStatusCode.OK, response.StatusCode);
+		CollectionAssert.AreEqual(content, await response.Content.ReadAsByteArrayAsync());
+		Assert.AreEqual(content.Length, response.Content.Headers.ContentLength);
+		Assert.AreEqual("application/octet-stream", response.Content.Headers.ContentType?.MediaType);
+		CollectionAssert.Contains(response.Headers.AcceptRanges.ToList(), "bytes");
+	}
+
 	[TestMethod]
 	public async Task Download_ObjectUpstreamDoesNotHave_ReportsTheErrorPerObject()
 	{
@@ -314,13 +391,13 @@
 
 		Assert.AreEqual(404, entry["error"]!["code"]!.GetValue<int>());
 		Assert.IsFalse(entry.AsObject().ContainsKey("actions"));
 	}
 
 	[TestMethod]
 	public async Task Upload_IsRelayedUpstreamAndStoredOnTheWayThrough()
 	{
 		await using ProxyFixture fixture = await ProxyFixture.StartAsync();
 		(byte[] content, string oid) = Object("pushed object");
 
 		JsonNode batch = await PostBatchAsync(fixture, "upload", oid, content.Length);
 		using HttpClient client = fixture.Client;

GitLfsCache/Endpoints/ObjectRouteHandler.cs

@@ -35,7 +35,7 @@
 /// <param name="metrics">Cache counters.</param>
 /// <param name="options">The configured options.</param>
 /// <param name="logger">Logger.</param>
 internal sealed class ObjectRouteHandler(
 	IUpstreamClient upstreamClient,
 	IHrefTokenCodec codec,
 	BatchRewriter rewriter,
@@ -151,7 +151,7 @@
 				metrics.RecordHit(route.Upstream, length);
 				EndpointLog.ServedFromCache(logger, token.Oid, route.Upstream);
 
-				await ServeFromStoreAsync(context, cached, length, cancellationToken).ConfigureAwait(false);
+				await ServeFromStoreAsync(context, cached).ConfigureAwait(false);
 			}
 
 			return;
@@ -179,8 +179,11 @@
 			// A follower released without the object, because the leader's client went away or the
 			// leader stalled, queues again: one of the released followers becomes the new leader and
 			// the rest wait for it, rather than every one of them fetching the same object at once.
-			for (int attempt = 1; !ticket.IsLeader; attempt++)
+			int attempt = 0;
+
+			while (!ticket.IsLeader)
 			{
+				attempt++;
 				EndpointLog.WaitingForLeader(logger, token.Oid, route.Upstream);
 
 				bool published = await ticket
@@ -198,8 +201,7 @@
 					{
 						store.Touch(route.Upstream, token.Oid);
 						metrics.RecordHit(route.Upstream, nowLength);
-						await ServeFromStoreAsync(context, nowCached, nowLength, cancellationToken)
-							.ConfigureAwait(false);
+						await ServeFromStoreAsync(context, nowCached).ConfigureAwait(false);
 					}
 
 					return;
@@ -243,7 +245,7 @@
 	/// server refuses part way through surfaces as a 500 from the upstream relay rather than a 413.
 	/// The limit is lifted here, in the library, so every host is covered, and only for an upload
 	/// whose token has already been checked, so batch and lock bodies keep the server's bound.
 	/// </remarks>
 	/// <param name="context">The request context.</param>
 	private static void LiftRequestBodyLimit(HttpContext context)
 	{
@@ -489,19 +491,11 @@
 		return true;
 	}
 
-	private static async Task ServeFromStoreAsync(
-		HttpContext context,
-		Stream cached,
-		long length,
-		CancellationToken cancellationToken)
-	{
-		context.Response.ContentType = OctetStream;
-		context.Response.ContentLength = length;
-
-		await StreamTee
-			.CopyAsync(cached, context.Response.Body, null, null, cancellationToken)
-			.ConfigureAwait(false);
-	}
+	// ASP.NET Core's range processing answers a Range with 206 and Content-Range, an unsatisfiable
+	// one with 416, and advertises Accept-Ranges: bytes. That is what lets git-lfs resume an
+	// interrupted download of a cached object instead of starting it over.
+	private static Task ServeFromStoreAsync(HttpContext context, Stream cached) =>
+		Results.Stream(cached, OctetStream, enableRangeProcessing: true).ExecuteAsync(context);
 
 	private static void CopyTransferHeaders(HttpResponseMessage response, HttpContext context)
 	{