fix: restore missing blob file when re-publishing a package (#39239)

Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>

modules/packages/content_store.go

@@ -4,11 +4,14 @@
 package packages
 
 import (
+	"errors"
 	"io"
+	"io/fs"
 	"net/url"
 	"path"
 	"strings"
 
+	"gitea.dev/modules/optional"
 	"gitea.dev/modules/setting"
 	"gitea.dev/modules/storage"
 	"gitea.dev/modules/util"
@@ -40,11 +43,15 @@ func (s *ContentStore) GetServeDirectURL(key BlobHash256Key, filename, method st
 	return s.store.ServeDirectURL(KeyToRelativePath(key), filename, method, reqParams)
 }
 
-// FIXME: Workaround to be removed in v1.20
-// https://github.com/go-gitea/gitea/issues/19586
-func (s *ContentStore) Has(key BlobHash256Key) error {
-	_, err := s.store.Stat(KeyToRelativePath(key))
-	return err
+func (s *ContentStore) OptionalSize(key BlobHash256Key) (sz optional.Option[int64], _ error) {
+	st, err := s.store.Stat(KeyToRelativePath(key))
+	if errors.Is(err, fs.ErrNotExist) {
+		return sz, nil
+	}
+	if err != nil {
+		return sz, err
+	}
+	return optional.Some(st.Size()), nil
 }
 
 // Save stores a package blob

modules/storage/azureblob.go

@@ -8,6 +8,7 @@ import (
 	"errors"
 	"fmt"
 	"io"
+	"io/fs"
 	"net/http"
 	"net/url"
 	"os"
@@ -108,7 +109,7 @@ func convertAzureBlobErr(err error) error {
 	}
 
 	if bloberror.HasCode(err, bloberror.BlobNotFound) {
-		return os.ErrNotExist
+		return fs.ErrNotExist
 	}
 	var respErr *azcore.ResponseError
 	if !errors.As(err, &respErr) {

modules/storage/local.go

@@ -8,6 +8,7 @@ import (
 	"errors"
 	"fmt"
 	"io"
+	"io/fs"
 	"net/url"
 	"os"
 	"path/filepath"
@@ -138,7 +139,7 @@ func (l *LocalStorage) ServeDirectURL(path, name, _ string, reqParams *ServeDire
 }
 
 func (l *LocalStorage) normalizeWalkError(err error) error {
-	if errors.Is(err, os.ErrNotExist) {
+	if errors.Is(err, fs.ErrNotExist) {
 		// ignore it because the file may be deleted during the walk, and we don't care about it
 		return nil
 	}

modules/storage/local_test.go

@@ -4,6 +4,7 @@
 package storage
 
 import (
+	"io/fs"
 	"os"
 	"strings"
 	"testing"
@@ -72,7 +73,7 @@ func TestLocalStorageDelete(t *testing.T) {
 		if exists {
 			require.NoError(t, err)
 		} else {
-			require.ErrorIs(t, err, os.ErrNotExist)
+			require.ErrorIs(t, err, fs.ErrNotExist)
 		}
 	}
 

modules/storage/minio.go

@@ -9,6 +9,7 @@ import (
 	"errors"
 	"fmt"
 	"io"
+	"io/fs"
 	"net/http"
 	"net/url"
 	"os"
@@ -87,9 +88,9 @@ func convertMinioErr(err error, optMsg ...string) error {
 	// Convert two responses to standard analogues
 	switch errResp.Code {
 	case "NoSuchKey":
-		return wrapErr(os.ErrNotExist)
+		return wrapErr(fs.ErrNotExist)
 	case "AccessDenied":
-		return wrapErr(os.ErrPermission)
+		return wrapErr(fs.ErrPermission)
 	}
 
 	return wrapErr(err)

routers/api/packages/container/blob.go

@@ -8,7 +8,6 @@ import (
 	"encoding/hex"
 	"errors"
 	"fmt"
-	"os"
 	"strings"
 
 	"gitea.dev/models/db"
@@ -18,7 +17,6 @@ import (
 	"gitea.dev/modules/log"
 	packages_module "gitea.dev/modules/packages"
 	container_module "gitea.dev/modules/packages/container"
-	"gitea.dev/modules/util"
 	packages_service "gitea.dev/services/packages"
 
 	"github.com/opencontainers/go-digest"
@@ -51,28 +49,10 @@ func saveAsPackageBlobInternal(ctx context.Context, hsr packages_module.HashedSi
 		if err := packages_service.CheckSizeQuotaExceeded(ctx, pci.Creator, pci.Owner, packages_model.TypeContainer, hsr.Size()); err != nil {
 			return err
 		}
-
-		pb, exists, err = packages_model.GetOrInsertBlob(ctx, pb)
+		pb, exists, err = packages_service.GetOrSavePackageBlob(ctx, contentStore, pb, hsr)
 		if err != nil {
-			log.Error("Error inserting package blob: %v", err)
 			return err
 		}
-		// FIXME: Workaround to be removed in v1.20
-		// https://github.com/go-gitea/gitea/issues/19586
-		if exists {
-			err = contentStore.Has(packages_module.BlobHash256Key(pb.HashSHA256))
-			if err != nil && (errors.Is(err, util.ErrNotExist) || errors.Is(err, os.ErrNotExist)) {
-				log.Debug("Package registry inconsistent: blob %s does not exist on file system", pb.HashSHA256)
-				exists = false
-			}
-		}
-		if !exists {
-			if err := contentStore.Save(packages_module.BlobHash256Key(pb.HashSHA256), hsr, hsr.Size()); err != nil {
-				log.Error("Error saving package blob in content store: %v", err)
-				return err
-			}
-		}
-
 		return createFileForBlob(ctx, uploadVersion, pb)
 	})
 	if err != nil {

routers/api/packages/container/container.go

@@ -9,7 +9,6 @@ import (
 	"io"
 	"net/http"
 	"net/url"
-	"os"
 	"regexp"
 	"strconv"
 	"strings"
@@ -28,7 +27,6 @@ import (
 	"gitea.dev/modules/setting"
 	"gitea.dev/modules/storage"
 	"gitea.dev/modules/structs"
-	"gitea.dev/modules/util"
 	"gitea.dev/routers/api/packages/helper"
 	auth_service "gitea.dev/services/auth"
 	"gitea.dev/services/context"
@@ -250,7 +248,7 @@ func PostBlobsUploads(ctx *context.Context) {
 	mount := ctx.FormTrim("mount")
 	from := ctx.FormTrim("from")
 	if mount != "" {
-		blob, _ := workaroundGetContainerBlob(ctx, &container_model.BlobSearchOptions{
+		blob, _ := getExistingContainerBlob(ctx, &container_model.BlobSearchOptions{
 			Repository: from,
 			Digest:     mount,
 		})
@@ -506,7 +504,7 @@ func getBlobFromContext(ctx *context.Context) (*packages_model.PackageFileDescri
 		return nil, container_model.ErrContainerBlobNotExist
 	}
 
-	return workaroundGetContainerBlob(ctx, &container_model.BlobSearchOptions{
+	return getExistingContainerBlob(ctx, &container_model.BlobSearchOptions{
 		OwnerID: ctx.Package.Owner.ID,
 		Image:   ctx.PathParam("image"),
 		Digest:  string(d),
@@ -643,7 +641,7 @@ func getManifestFromContext(ctx *context.Context) (*packages_model.PackageFileDe
 		return nil, err
 	}
 
-	return workaroundGetContainerBlob(ctx, opts)
+	return getExistingContainerBlob(ctx, opts)
 }
 
 // https://github.com/opencontainers/distribution-spec/blob/main/spec.md#checking-if-content-exists-in-the-registry
@@ -789,23 +787,20 @@ func GetTagsList(ctx *context.Context) {
 	})
 }
 
-// FIXME: Workaround to be removed in v1.20.
-// Update maybe we should never really remote it, as long as there is legacy data?
-// https://github.com/go-gitea/gitea/issues/19586
-func workaroundGetContainerBlob(ctx *context.Context, opts *container_model.BlobSearchOptions) (*packages_model.PackageFileDescriptor, error) {
+// getExistingContainerBlob checks if the blob exists in the database and content store, and returns the package file descriptor if it does.
+func getExistingContainerBlob(ctx *context.Context, opts *container_model.BlobSearchOptions) (*packages_model.PackageFileDescriptor, error) {
 	blob, err := container_model.GetContainerBlob(ctx, opts)
 	if err != nil {
 		return nil, err
 	}
 
-	err = packages_module.NewContentStore().Has(packages_module.BlobHash256Key(blob.Blob.HashSHA256))
+	storeKey := packages_module.BlobHash256Key(blob.Blob.HashSHA256)
+	sz, err := packages_module.NewContentStore().OptionalSize(storeKey)
 	if err != nil {
-		if errors.Is(err, util.ErrNotExist) || errors.Is(err, os.ErrNotExist) {
-			log.Debug("Package registry inconsistent: blob %s does not exist on file system", blob.Blob.HashSHA256)
-			return nil, container_model.ErrContainerBlobNotExist
-		}
-		return nil, err
+		return nil, fmt.Errorf("unable to check object size in content store: %w", err)
+	}
+	if sz.ValueOrDefault(-1) != blob.Blob.Size {
+		return nil, container_model.ErrContainerBlobNotExist
 	}
-
 	return blob, nil
 }

routers/api/packages/container/manifest.go

@@ -8,7 +8,6 @@ import (
 	"errors"
 	"fmt"
 	"io"
-	"os"
 	"strings"
 
 	"gitea.dev/models/db"
@@ -20,7 +19,6 @@ import (
 	"gitea.dev/modules/log"
 	packages_module "gitea.dev/modules/packages"
 	container_module "gitea.dev/modules/packages/container"
-	"gitea.dev/modules/util"
 	notify_service "gitea.dev/services/notify"
 	packages_service "gitea.dev/services/packages"
 	container_service "gitea.dev/services/packages/container"
@@ -379,26 +377,10 @@ func createFileFromBlobReference(ctx context.Context, pv, uploadVersion *package
 }
 
 func createManifestBlob(ctx context.Context, contentStore *packages_module.ContentStore, mci *manifestCreationInfo, pv *packages_model.PackageVersion, buf *packages_module.HashedBuffer) (_ *packages_model.PackageBlob, created bool, manifestDigest string, _ error) {
-	pb, exists, err := packages_model.GetOrInsertBlob(ctx, packages_service.NewPackageBlob(buf))
+	pb, exists, err := packages_service.GetOrSavePackageBlob(ctx, contentStore, packages_service.NewPackageBlob(buf), buf)
 	if err != nil {
-		log.Error("Error inserting package blob: %v", err)
 		return nil, false, "", err
 	}
-	// FIXME: Workaround to be removed in v1.20
-	// https://github.com/go-gitea/gitea/issues/19586
-	if exists {
-		err = contentStore.Has(packages_module.BlobHash256Key(pb.HashSHA256))
-		if err != nil && (errors.Is(err, util.ErrNotExist) || errors.Is(err, os.ErrNotExist)) {
-			log.Debug("Package registry inconsistent: blob %s does not exist on file system", pb.HashSHA256)
-			exists = false
-		}
-	}
-	if !exists {
-		if err := contentStore.Save(packages_module.BlobHash256Key(pb.HashSHA256), buf, buf.Size()); err != nil {
-			log.Error("Error saving package blob in content store: %v", err)
-			return nil, false, "", err
-		}
-	}
 
 	manifestDigest = digestFromHashSummer(buf)
 	pf, err := createFileFromBlobReference(ctx, pv, nil, &blobReference{

services/packages/packages.go

@@ -264,6 +264,42 @@ func NewPackageBlob(hsr packages_module.HashedSizeReader) *packages_model.Packag
 	}
 }
 
+func GetOrSavePackageBlob(ctx context.Context, contentStore *packages_module.ContentStore, blob *packages_model.PackageBlob, data packages_module.HashedSizeReader) (_ *packages_model.PackageBlob, _ bool, retErr error) {
+	if blob.Size != data.Size() {
+		return nil, false, fmt.Errorf("size mismatch: blob size %d, data size %d", blob.Size, data.Size())
+	}
+	pb, existsInDatabase, err := packages_model.GetOrInsertBlob(ctx, blob)
+	if err != nil {
+		return nil, false, fmt.Errorf("unable to get or insert blob: %w", err)
+	}
+
+	defer func() {
+		if retErr != nil && !existsInDatabase {
+			if errDelete := packages_model.DeleteBlobByID(ctx, pb.ID); errDelete != nil {
+				log.Error("unable to delete blob from database after failed save in content store: %v", errDelete)
+			}
+		}
+	}()
+
+	var objSize optional.Option[int64]
+	storeKey := packages_module.BlobHash256Key(pb.HashSHA256)
+	if existsInDatabase {
+		// check if the blob file actually is valid in the content store
+		objSize, err = contentStore.OptionalSize(storeKey)
+		if err != nil {
+			return nil, false, fmt.Errorf("unable to check object size in content store: %w", err)
+		}
+	}
+	if objSize.ValueOrDefault(-1) != blob.Size {
+		if err := contentStore.Save(storeKey, data, data.Size()); err != nil {
+			return nil, false, fmt.Errorf("unable to save object in content store: %w", err)
+		}
+	}
+	// existsInDatabase controls the "roll back", if other errors happen later,
+	// the "non-existing (newly created)" blob will be deleted from the content store, but not if it already existed in the database.
+	return pb, existsInDatabase, nil
+}
+
 func addFileToPackageVersion(ctx context.Context, pv *packages_model.PackageVersion, pvi *PackageInfo, pfci *PackageFileCreationInfo) (*packages_model.PackageFile, *packages_model.PackageBlob, bool, error) {
 	if err := CheckSizeQuotaExceeded(ctx, pfci.Creator, pvi.Owner, pvi.PackageType, pfci.Data.Size()); err != nil {
 		return nil, nil, false, err
@@ -272,21 +308,13 @@ func addFileToPackageVersion(ctx context.Context, pv *packages_model.PackageVers
 	return addFileToPackageVersionUnchecked(ctx, pv, pfci)
 }
 
-func addFileToPackageVersionUnchecked(ctx context.Context, pv *packages_model.PackageVersion, pfci *PackageFileCreationInfo) (*packages_model.PackageFile, *packages_model.PackageBlob, bool, error) {
+func addFileToPackageVersionUnchecked(ctx context.Context, pv *packages_model.PackageVersion, pfci *PackageFileCreationInfo) (_ *packages_model.PackageFile, _ *packages_model.PackageBlob, created bool, _ error) {
 	log.Trace("Adding package file: %v, %s", pv.ID, pfci.Filename)
 
-	pb, exists, err := packages_model.GetOrInsertBlob(ctx, NewPackageBlob(pfci.Data))
+	pb, exists, err := GetOrSavePackageBlob(ctx, packages_module.NewContentStore(), NewPackageBlob(pfci.Data), pfci.Data)
 	if err != nil {
-		log.Error("Error inserting package blob: %v", err)
 		return nil, nil, false, err
 	}
-	if !exists {
-		contentStore := packages_module.NewContentStore()
-		if err := contentStore.Save(packages_module.BlobHash256Key(pb.HashSHA256), pfci.Data, pfci.Data.Size()); err != nil {
-			log.Error("Error saving package blob in content store: %v", err)
-			return nil, nil, false, err
-		}
-	}
 
 	if pfci.OverwriteExisting {
 		pf, err := packages_model.GetFileForVersionByName(ctx, pv.ID, pfci.Filename, pfci.CompositeKey)

services/packages/packages_test.go

@@ -4,6 +4,11 @@
 package packages
 
 import (
+	"bytes"
+	"crypto/sha256"
+	"encoding/hex"
+	"io"
+	"net/http"
 	"testing"
 
 	"gitea.dev/models/db"
@@ -13,6 +18,8 @@ import (
 	unit_model "gitea.dev/models/unit"
 	"gitea.dev/models/unittest"
 	user_model "gitea.dev/models/user"
+	packages_module "gitea.dev/modules/packages"
+	"gitea.dev/modules/test"
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
@@ -22,6 +29,72 @@ func TestMain(m *testing.M) {
 	unittest.MainTest(m)
 }
 
+func TestCreatePackageAndAddFileRestoresMissingBlobFile(t *testing.T) {
+	assert.NoError(t, unittest.PrepareTestDatabase())
+	user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2})
+
+	uploadPackage := func(t *testing.T, user *user_model.User, name, filename string, data []byte) (*packages_model.PackageFile, error) {
+		buf, err := packages_module.CreateHashedBufferFromReader(bytes.NewReader(data))
+		require.NoError(t, err)
+		_, pf, err := CreatePackageAndAddFile(t.Context(),
+			&PackageCreationInfo{
+				Owner:            user,
+				PackageType:      packages_model.TypeNuGet,
+				Name:             name,
+				Version:          "1.0.0",
+				SemverCompatible: true,
+				Creator:          user,
+			},
+			&PackageFileCreationInfo{
+				Filename: filename,
+				Creator:  user,
+				Data:     buf,
+				IsLead:   true,
+			})
+		return pf, err
+	}
+
+	// This test data is from https://github.com/go-gitea/gitea/issues/39215, it doesn't really matter, actually.
+	// The key point is that if the blob object is missing in the content storage, it must be restored when uploaded again.
+	pkgData := test.WriteZipArchive(map[string]string{
+		"package.nuspec":         "<package><metadata><id>nuget.repro</id><version>1.0.0</version></metadata></package>",
+		"lib/netstandard2.0/_._": "",
+	}).Bytes()
+	pkgDataSum := sha256.Sum256(pkgData)
+	key := packages_module.BlobHash256Key(hex.EncodeToString(pkgDataSum[:]))
+	contentStore := packages_module.NewContentStore()
+
+	// The initial upload writes the blob row and its file
+	pf1, err := uploadPackage(t, user, "nuget.repro", "nuget.repro.1.0.0.nupkg", pkgData)
+	require.NoError(t, err)
+	sz, err := contentStore.OptionalSize(key)
+	assert.NoError(t, err)
+	assert.EqualValues(t, len(pkgData), sz.ValueOrDefault(-1))
+
+	// Simulate the storage inconsistency: the blob row survives but its file is missing
+	require.NoError(t, contentStore.Delete(key))
+	sz, err = contentStore.OptionalSize(key)
+	assert.NoError(t, err)
+	assert.EqualValues(t, -1, sz.ValueOrDefault(-1))
+
+	// Publishing a package with identical content must restore the blob file
+	pf2, err := uploadPackage(t, user, "nuget.repro-copy", "nuget.repro-copy.1.0.0.nupkg", pkgData)
+	require.NoError(t, err)
+	sz, err = contentStore.OptionalSize(key)
+	assert.NoError(t, err)
+	assert.EqualValues(t, len(pkgData), sz.ValueOrDefault(-1))
+
+	// The blob file must be present and both packages must be downloadable
+	for _, pf := range []*packages_model.PackageFile{pf1, pf2} {
+		s, _, _, err := OpenFileForDownload(t.Context(), pf, http.MethodGet)
+		require.NoError(t, err)
+		respData, err := io.ReadAll(s)
+		require.NoError(t, err)
+		assert.NoError(t, s.Close())
+		assert.Equal(t, pkgData, respData)
+	}
+}
+
 func TestUnlinkFromRepositoryRequiresTargetRepoAdmin(t *testing.T) {
 	assert.NoError(t, unittest.PrepareTestDatabase())
 	repo := &repo_model.Repository{OwnerID: 3, OwnerName: "org3", Name: "package-repo", LowerName: "package-repo", IsPrivate: true}