diff --git a/modules/lfs/content_store.go b/modules/lfs/content_store.go index 04e1091022..06d427a2a6 100644 --- a/modules/lfs/content_store.go +++ b/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. diff --git a/services/lfs/server.go b/services/lfs/server.go index 8c7de2ccb6..285bc00eb2 100644 --- a/services/lfs/server.go +++ b/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 } diff --git a/services/repository/lfs.go b/services/repository/lfs.go index 52a1605da0..618cad1c99 100644 --- a/services/repository/lfs.go +++ b/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++ diff --git a/tests/integration/api_repo_lfs_test.go b/tests/integration/api_repo_lfs_test.go index 0f65606017..dea02814df 100644 --- a/tests/integration/api_repo_lfs_test.go +++ b/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)() diff --git a/tests/integration/lfs_getobject_test.go b/tests/integration/lfs_getobject_test.go index ddc23ccd05..ca3bbeb79c 100644 --- a/tests/integration/lfs_getobject_test.go +++ b/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 }