Make LFS http_client parallel within a batch.

Signed-off-by: Royce Remer <[email protected]>

Author: rremer

modules/lfs/http_client.go

@@ -17,6 +17,8 @@ import (
 	"code.gitea.io/gitea/modules/log"
 	"code.gitea.io/gitea/modules/proxy"
 	"code.gitea.io/gitea/modules/setting"
+
+	"golang.org/x/sync/errgroup"
 )
 
 // HTTPClient is used to communicate with the LFS server
@@ -113,6 +115,7 @@ func (c *HTTPClient) Upload(ctx context.Context, objects []Pointer, callback Upl
 	return c.performOperation(ctx, objects, nil, callback)
 }
 
+// performOperation takes a slice of LFS object pointers, batches them, and performs the upload/download operations concurrently in each batch
 func (c *HTTPClient) performOperation(ctx context.Context, objects []Pointer, dc DownloadCallback, uc UploadCallback) error {
 	if len(objects) == 0 {
 		return nil
@@ -133,71 +136,92 @@ func (c *HTTPClient) performOperation(ctx context.Context, objects []Pointer, dc
 		return fmt.Errorf("TransferAdapter not found: %s", result.Transfer)
 	}
 
+	errGroup, groupCtx := errgroup.WithContext(context.Background())
 	for _, object := range result.Objects {
-		if object.Error != nil {
-			log.Trace("Error on object %v: %v", object.Pointer, object.Error)
-			if uc != nil {
-				if _, err := uc(object.Pointer, object.Error); err != nil {
-					return err
-				}
-			} else {
-				if err := dc(object.Pointer, nil, object.Error); err != nil {
-					return err
-				}
-			}
-			continue
-		}
+		func(groupCtx context.Context, object *ObjectResponse, dc DownloadCallback, uc UploadCallback, transferAdapter TransferAdapter) {
+			errGroup.Go(func() error {
+				err := performSingleOperation(groupCtx, object, dc, uc, transferAdapter)
+				return err
+			})
+		}(groupCtx, object, dc, uc, transferAdapter)
+	}
 
-		if uc != nil {
-			if len(object.Actions) == 0 {
-				log.Trace("%v already present on server", object.Pointer)
-				continue
-			}
+	// only the first error is returned, preserving legacy behavior before concurrency
+	return errGroup.Wait()
+}
 
-			link, ok := object.Actions["upload"]
-			if !ok {
-				log.Debug("%+v", object)
-				return errors.New("missing action 'upload'")
-			}
+// performSingleOperation performs an LFS upload or download operation on a single object
+func performSingleOperation(ctx context.Context, object *ObjectResponse, dc DownloadCallback, uc UploadCallback, transferAdapter TransferAdapter) error {
+	// the response from an lfs batch api request for this specific object id contained an error
+	if object.Error != nil {
+		log.Trace("Error on object %v: %v", object.Pointer, object.Error)
 
-			content, err := uc(object.Pointer, nil)
-			if err != nil {
+		// this was an 'upload' request inside the batch request
+		if uc != nil {
+			if _, err := uc(object.Pointer, object.Error); err != nil {
 				return err
 			}
+		}
 
-			err = transferAdapter.Upload(ctx, link, object.Pointer, content)
-			if err != nil {
-				return err
-			}
+		// this was not an 'upload' request inside the batch request, meaning in must be a 'download' request
+		err := dc(object.Pointer, nil, object.Error)
+		if errors.Is(object.Error, ErrObjectNotExist) {
+			log.Warn("Ignoring missing upstream LFS object %-v: %v", object.Pointer, err)
+			return nil
+		}
 
-			link, ok = object.Actions["verify"]
-			if ok {
-				if err := transferAdapter.Verify(ctx, link, object.Pointer); err != nil {
-					return err
-				}
-			}
-		} else {
-			link, ok := object.Actions["download"]
-			if !ok {
-				// no actions block in response, try legacy response schema
-				link, ok = object.Links["download"]
-			}
-			if !ok {
-				log.Debug("%+v", object)
-				return errors.New("missing action 'download'")
-			}
+		// this was a 'download' request which was a legitimate error response from the batch api (not an http/404)
+		return err
+	}
 
-			content, err := transferAdapter.Download(ctx, link)
-			if err != nil {
-				return err
-			}
+	// the response from an lfs batch api request contained necessary upload/download fields to act upon
+	if uc != nil {
+		if len(object.Actions) == 0 {
+			log.Trace("%v already present on server", object.Pointer)
+			return nil
+		}
+
+		link, ok := object.Actions["upload"]
+		if !ok {
+			return errors.New("missing action 'upload'")
+		}
+
+		content, err := uc(object.Pointer, nil)
+		if err != nil {
+			return err
+		}
 
-			if err := dc(object.Pointer, content, nil); err != nil {
+		err = transferAdapter.Upload(ctx, link, object.Pointer, content)
+		if err != nil {
+			return err
+		}
+
+		link, ok = object.Actions["verify"]
+		if ok {
+			if err := transferAdapter.Verify(ctx, link, object.Pointer); err != nil {
 				return err
 			}
 		}
-	}
+	} else {
+		link, ok := object.Actions["download"]
+		if !ok {
+			// no actions block in response, try legacy response schema
+			link, ok = object.Links["download"]
+		}
+		if !ok {
+			log.Debug("%+v", object)
+			return errors.New("missing action 'download'")
+		}
 
+		content, err := transferAdapter.Download(ctx, link)
+		if err != nil {
+			return err
+		}
+
+		if err := dc(object.Pointer, content, nil); err != nil {
+			return err
+		}
+	}
 	return nil
 }
 

modules/lfs/http_client_test.go

@@ -211,36 +211,31 @@ func TestHTTPClientDownload(t *testing.T) {
 			expectederror: "TransferAdapter not found: ",
 		},
 		// case 5
-		{
-			endpoint:      "https://error-in-response-objects.io",
-			expectederror: "Object not found",
-		},
-		// case 6
 		{
 			endpoint:      "https://empty-actions-map.io",
 			expectederror: "missing action 'download'",
 		},
-		// case 7
+		// case 6
 		{
 			endpoint:      "https://download-actions-map.io",
 			expectederror: "",
 		},
-		// case 8
+		// case 7
 		{
 			endpoint:      "https://upload-actions-map.io",
 			expectederror: "missing action 'download'",
 		},
-		// case 9
+		// case 8
 		{
 			endpoint:      "https://verify-actions-map.io",
 			expectederror: "missing action 'download'",
 		},
-		// case 10
+		// case 9
 		{
 			endpoint:      "https://unknown-actions-map.io",
 			expectederror: "missing action 'download'",
 		},
-		// case 11
+		// case 10
 		{
 			endpoint:      "https://legacy-batch-request-download.io",
 			expectederror: "",

modules/repository/repo.go

@@ -5,7 +5,6 @@ package repository
 
 import (
 	"context"
-	"errors"
 	"fmt"
 	"io"
 	"strings"
@@ -182,10 +181,6 @@ func StoreMissingLfsObjectsInRepository(ctx context.Context, repo *repo_model.Re
 	downloadObjects := func(pointers []lfs.Pointer) error {
 		err := lfsClient.Download(ctx, pointers, func(p lfs.Pointer, content io.ReadCloser, objectError error) error {
 			if objectError != nil {
-				if errors.Is(objectError, lfs.ErrObjectNotExist) {
-					log.Warn("Repo[%-v]: Ignore missing LFS object %-v: %v", repo, p, objectError)
-					return nil
-				}
 				return objectError
 			}