fix(lfs): failed upload deletes a concurrent upload's meta object (#38693)

UploadHandler creates the LFS meta object only as the last step of
uploadOrVerify, after the content is already in the store, so a request
that errors has never created a row of its own. The removal on the error
path was a real compensating action when it was added in #14726, where
the meta object was created before contentStore.Put, but #16865 moved
creation after the Put and left the removal behind.

Since then it can only ever delete a row created by a different request:
a stalled git-lfs PUT that fails after its own retry has already
succeeded wipes the winner's meta object, leaving the content in the
store unreachable and eligible for orphan cleanup.

Drop the removal and add a regression test asserting that a failing
upload keeps a pre-existing meta object.

Fixes: #38424
Assisted-by: Claude Code:claude-opus-5
Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>

modules/lfs/content_store.go

@@ -24,7 +24,7 @@ var (
 
 // ContentStore provides a simple file system based storage.
 type ContentStore struct {
-	storage.ObjectStorage
+	ObjectStorage storage.ObjectStorage
 }
 
 // NewContentStore creates the default ContentStore
@@ -36,7 +36,7 @@ func NewContentStore() *ContentStore {
 // Get takes a Meta object and retrieves the content from the store, returning
 // it as an io.ReadSeekCloser.
 func (s *ContentStore) Get(pointer Pointer) (storage.Object, error) {
-	f, err := s.Open(pointer.RelativePath())
+	f, err := s.ObjectStorage.Open(pointer.RelativePath())
 	if err != nil {
 		log.Error("Whilst trying to read LFS OID[%s]: Unable to open Error: %v", pointer.Oid, err)
 		return nil, err
@@ -53,7 +53,7 @@ func (s *ContentStore) Put(pointer Pointer, r io.Reader) error {
 
 	// now pass the wrapped reader to Save - if there is a size mismatch or hash mismatch then
 	// the errors returned by the newHashingReader should percolate up to here
-	written, err := s.Save(p, wrappedRd, pointer.Size)
+	written, err := s.ObjectStorage.Save(p, wrappedRd, pointer.Size)
 	if err != nil {
 		log.Error("Whilst putting LFS OID[%s]: Failed to copy to tmpPath: %s Error: %v", pointer.Oid, p, err)
 		return err
@@ -69,7 +69,7 @@ func (s *ContentStore) Put(pointer Pointer, r io.Reader) error {
 
 	// if the upload failed, try to delete the file
 	if err != nil {
-		if errDel := s.Delete(p); errDel != nil {
+		if errDel := s.ObjectStorage.Delete(p); errDel != nil {
 			log.Error("Cleaning the LFS OID[%s] failed: %v", pointer.Oid, errDel)
 		}
 	}
@@ -77,16 +77,18 @@ func (s *ContentStore) Put(pointer Pointer, r io.Reader) error {
 	return err
 }
 
-// Exists returns true if the object exists in the content store.
+func (s *ContentStore) Stat(pointer Pointer) (os.FileInfo, error) {
+	return s.ObjectStorage.Stat(pointer.RelativePath())
+}
+
 func (s *ContentStore) Exists(pointer Pointer) (bool, error) {
-	_, err := s.ObjectStorage.Stat(pointer.RelativePath())
-	if err != nil {
-		if os.IsNotExist(err) {
-			return false, nil
-		}
-		return false, err
+	_, err := s.Stat(pointer)
+	if os.IsNotExist(err) {
+		return false, nil
+	} else if err == nil {
+		return true, nil
 	}
-	return true, nil
+	return false, err
 }
 
 // Verify returns true if the object exists in the content store and size is correct.

services/lfs/server.go

@@ -11,6 +11,7 @@ import (
 	"errors"
 	"fmt"
 	"io"
+	"io/fs"
 	"net/http"
 	"net/url"
 	"regexp"
@@ -319,28 +320,25 @@ func UploadHandler(ctx *context.Context) {
 		return
 	}
 
-	contentStore := lfs_module.NewContentStore()
-	exists, err := contentStore.Exists(p)
-	if err != nil {
-		log.Error("Unable to check if LFS OID[%s] exist. Error: %v", p.Oid, err)
-		writeStatus(ctx, http.StatusInternalServerError)
-		return
-	}
-
 	uploadOrVerify := func() error {
-		if exists {
+		contentStore := lfs_module.NewContentStore()
+		stat, err := contentStore.Stat(p)
+		if stat != nil {
 			// The bytes already exist in the content store. Only skip proof of
 			// possession when the object is already linked to *this* repo; never
 			// trust cross-repo access (ctx.Doer is the repo owner for deploy keys),
 			// which would let a caller link an object it cannot produce.
 			meta, err := git_model.GetLFSMetaObjectByOid(ctx, repository.ID, p.Oid)
-			if err != nil && err != git_model.ErrLFSObjectNotExist {
+			if err != nil && !errors.Is(err, util.ErrNotExist) {
 				log.Error("Unable to get LFS MetaObject [%s]. Error: %v", p.Oid, err)
 				return err
 			}
 			if meta == nil {
-				// The file exists but is not linked to this repo.
+				// The file exists but is not linked to this repo, or the file is being uploaded.
 				// The upload gets verified by hashing and size comparison to prove access to it.
+				// Keep in mind: here the file might be incomplete due to concurrent uploading, so the verification might fail.
+				// ATTENTION: it's impossible to handle corrupted file on server-side at the moment,
+				// we don't know whether a file is really corrupted, or it is being uploaded.
 				hash := sha256.New()
 				written, err := io.Copy(hash, ctx.Req.Body)
 				if err != nil {
@@ -355,11 +353,17 @@ func UploadHandler(ctx *context.Context) {
 					return lfs_module.ErrHashMismatch
 				}
 			}
-		} else if err := contentStore.Put(p, ctx.Req.Body); err != nil {
-			log.Error("Error putting LFS MetaObject [%s] into content store. Error: %v", p.Oid, err)
+		} else if errors.Is(err, fs.ErrNotExist) {
+			// not exist, store it into the store
+			if err := contentStore.Put(p, ctx.Req.Body); err != nil {
+				log.Error("Error putting LFS MetaObject [%s] into content store. Error: %v", p.Oid, err)
+				return err
+			}
+		} else {
+			log.Error("Unable to check if LFS OID[%s] stat. Error: %v", p.Oid, err)
 			return err
 		}
-		_, err := git_model.NewLFSMetaObject(ctx, repository.ID, p)
+		_, err = git_model.NewLFSMetaObject(ctx, repository.ID, p)
 		return err
 	}
 
@@ -372,9 +376,12 @@ func UploadHandler(ctx *context.Context) {
 			log.Error("Error whilst uploadOrVerify LFS OID[%s]: %v", p.Oid, err)
 			writeStatus(ctx, http.StatusInternalServerError)
 		}
-		if _, err = git_model.RemoveLFSMetaObjectByOid(ctx, repository.ID, p.Oid); err != nil {
-			log.Error("Error whilst removing MetaObject for LFS OID[%s]: %v", p.Oid, err)
-		}
+		// Do not remove the LFS MetaObject here: this request only creates it after the content is verified and stored,
+		// an invalid request should not remove the existing correct record.
+		// If two requests are loading (the file is incomplete):
+		// * one will keep writing the file content
+		// * one will fail the verification because it reads an incomplete file, the failure should be just ignore
+		// In the end, the first one will complete the upload and insert a LFS MetaObject record.
 		return
 	}
 

services/repository/lfs.go

@@ -101,7 +101,7 @@ func GarbageCollectLFSMetaObjectsForRepo(ctx context.Context, repo *repo_model.R
 				return nil
 			}
 
-			if err := store.Delete(metaObject.RelativePath()); err != nil {
+			if err := store.ObjectStorage.Delete(metaObject.RelativePath()); err != nil {
 				log.Error("Unable to remove lfs metaobject %s from store: %v", metaObject.Oid, err)
 			}
 			deleted++

tests/integration/api_repo_lfs_test.go

@@ -5,9 +5,8 @@ package integration
 
 import (
 	"bytes"
+	"fmt"
 	"net/http"
-	"path"
-	"strconv"
 	"strings"
 	"testing"
 
@@ -59,6 +58,7 @@ func TestAPILFSMediaType(t *testing.T) {
 }
 
 func createLFSTestRepository(t *testing.T, repoName string) *repo_model.Repository {
+	t.Helper()
 	ctx := NewAPITestContext(t, "user2", repoName, auth_model.AccessTokenScopeWriteRepository, auth_model.AccessTokenScopeWriteUser)
 	t.Run("CreateRepo", doAPICreateRepository(ctx, false))
 
@@ -281,7 +281,7 @@ func TestAPILFSBatch(t *testing.T) {
 			assert.Equal(t, git_model.ErrLFSObjectNotExist, err)
 
 			// Cleanup
-			err = contentStore.Delete(p.RelativePath())
+			err = contentStore.ObjectStorage.Delete(p.RelativePath())
 			assert.NoError(t, err)
 		})
 
@@ -334,13 +334,11 @@ func TestAPILFSUpload(t *testing.T) {
 	defer test.MockVariableValue(&setting.LFS.StartServer, true)()
 
 	repo := createLFSTestRepository(t, "lfs-upload-repo")
-	oid := storeObjectInRepo(t, repo.ID, "dummy3")
-	defer git_model.RemoveLFSMetaObjectByOid(t.Context(), repo.ID, oid)
-
 	session := loginUser(t, "user2")
 
 	newRequest := func(t testing.TB, p lfs.Pointer, content string) *RequestWrapper {
-		return NewRequestWithBody(t, "PUT", path.Join("/user2/lfs-upload-repo.git/info/lfs/objects/", p.Oid, strconv.FormatInt(p.Size, 10)), strings.NewReader(content))
+		reqUrl := fmt.Sprintf("/user2/lfs-upload-repo.git/info/lfs/objects/%s/%d", p.Oid, p.Size)
+		return NewRequestWithBody(t, "PUT", reqUrl, strings.NewReader(content))
 	}
 
 	t.Run("InvalidPointer", func(t *testing.T) {
@@ -382,15 +380,14 @@ func TestAPILFSUpload(t *testing.T) {
 		})
 
 		// Cleanup
-		err = contentStore.Delete(p.RelativePath())
+		err = contentStore.ObjectStorage.Delete(p.RelativePath())
 		assert.NoError(t, err)
 	})
 
 	t.Run("MetaAlreadyExists", func(t *testing.T) {
 		defer tests.PrintCurrentTest(t)()
-
+		oid := storeObjectInRepo(t, repo.ID, "123456")
 		req := newRequest(t, lfs.Pointer{Oid: oid, Size: 6}, "")
-
 		session.MakeRequest(t, req, http.StatusOK)
 	})
 
@@ -410,6 +407,24 @@ func TestAPILFSUpload(t *testing.T) {
 		session.MakeRequest(t, req, http.StatusUnprocessableEntity)
 	})
 
+	t.Run("ConcurrentFailureKeepsExistingMeta", func(t *testing.T) {
+		defer tests.PrintCurrentTest(t)()
+		pointer, err := lfs.GeneratePointer(strings.NewReader("any-content"))
+		assert.NoError(t, err)
+
+		// mock a record in database: it should not happen in real world (no existing file in the store)
+		// !!for testing purpose only!! to verify a failed upload should not remove a valid record,
+		_, err = git_model.NewLFSMetaObject(t.Context(), repo.ID, pointer)
+		assert.NoError(t, err)
+
+		// make an invalid request, the existing lfs record should not be removed
+		req := newRequest(t, lfs.Pointer{Oid: pointer.Oid, Size: 1}, "invalid content")
+		session.MakeRequest(t, req, http.StatusUnprocessableEntity)
+		meta, err := git_model.GetLFSMetaObjectByOid(t.Context(), repo.ID, pointer.Oid)
+		assert.NoError(t, err)
+		assert.NotNil(t, meta)
+	})
+
 	t.Run("Success", func(t *testing.T) {
 		defer tests.PrintCurrentTest(t)()
 

tests/integration/lfs_getobject_test.go

@@ -26,18 +26,14 @@ import (
 )
 
 func storeObjectInRepo(t *testing.T, repositoryID int64, content string) string {
+	t.Helper()
 	pointer, err := lfs.GeneratePointer(strings.NewReader(content))
 	assert.NoError(t, err)
 
 	_, err = git_model.NewLFSMetaObject(t.Context(), repositoryID, pointer)
 	assert.NoError(t, err)
-	contentStore := lfs.NewContentStore()
-	exist, err := contentStore.Exists(pointer)
+	err = lfs.NewContentStore().Put(pointer, strings.NewReader(content))
 	assert.NoError(t, err)
-	if !exist {
-		err := contentStore.Put(pointer, strings.NewReader(content))
-		assert.NoError(t, err)
-	}
 	return pointer.Oid
 }