Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 13 additions & 1 deletion services/packages/packages.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import (
"io"
"net/http"
"net/url"
"os"
"strings"

"gitea.dev/models/db"
Expand All @@ -25,6 +26,7 @@ import (
packages_module "gitea.dev/modules/packages"
"gitea.dev/modules/setting"
"gitea.dev/modules/storage"
"gitea.dev/modules/util"
notify_service "gitea.dev/services/notify"
)

Expand Down Expand Up @@ -280,8 +282,18 @@ func addFileToPackageVersionUnchecked(ctx context.Context, pv *packages_model.Pa
log.Error("Error inserting package blob: %v", err)
return nil, nil, false, err
}
// Check if the blob file actually exists in the content store, since the
// blob row could have been created while the file was lost (eg. after a
// partial migration), otherwise the file would never be restored.
// See issue #19586 for the same inconsistency in the container registry.
contentStore := packages_module.NewContentStore()
if exists {
if err := contentStore.Has(packages_module.BlobHash256Key(pb.HashSHA256)); err != nil && (errors.Is(err, util.ErrNotExist) || errors.Is(err, os.ErrNotExist)) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please review and fix this line by human.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the review. This line mirrors the check that already exists on the container registry path for the same inconsistency (the FIXME workaround for #19586 in routers/api/packages/container/blob.go):

err = contentStore.Has(packages_module.BlobHash256Key(pb.HashSHA256))
if err != nil && (errors.Is(err, util.ErrNotExist) || errors.Is(err, os.ErrNotExist)) {
    exists = false
}

The intent: GetOrInsertBlob returning exists=true only proves the row exists, not the file. When the file is missing (Has reports not-exist), treat the blob as missing so the Save below rewrites it — otherwise, after a "row present, file lost" storage inconsistency, every re-publish of the same content inserts new package_file rows and never restores the file, and downloads fail forever (the failure mode in #39215).

One deliberate choice to flag: if Has fails with any error other than not-exist (e.g. an I/O error), we keep exists=true and skip the Save. That matches the container path exactly — a transient storage error should not silently turn into "file missing" and risk dropping the blob from further consideration; and in the common case (local file system, S3) a read error on an existing file is far less likely than a plain not-exist.

Happy to adjust if you'd rather handle the error differently (e.g. propagate it, or treat any Has error as missing).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/go-gitea/gitea/blob/main/CONTRIBUTING.md#ai-contribution-policy

No time to read AI response.

Reply by human. Just tell people what's the right thing to do

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

contentStore.Has only returns os.ErrNotExist, it never returns util.ErrNotExist

Old code is not right either.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 4590db1 — you're right, the storage layer only ever returns os.ErrNotExist, so the util.ErrNotExist check was dead code. Dropped it, kept the os.ErrNotExist check.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you very much. I will try to refactor the legacy code together.

log.Debug("Package registry inconsistent: blob %v does not exist on storage", pb.HashSHA256)
exists = false
}
}
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
Expand Down
83 changes: 83 additions & 0 deletions services/packages/packages_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,11 @@
package packages

import (
"bytes"
"crypto/sha256"
"encoding/hex"
"io"
"net/http"
"testing"

"gitea.dev/models/db"
Expand All @@ -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"
Expand All @@ -22,6 +29,82 @@ func TestMain(m *testing.M) {
unittest.MainTest(m)
}

// TestCreatePackageAndAddFileRestoresMissingBlobFile reproduces the state from
// https://github.com/go-gitea/gitea/issues/39215: a blob row exists in the
// database but its file in the content store is missing (e.g. after the file
// was lost on the storage). Publishing a package that references the same
// content must restore the missing blob file, otherwise every re-publish is a
// silent no-op and the package is permanently undownloadable.
func TestCreatePackageAndAddFileRestoresMissingBlobFile(t *testing.T) {
assert.NoError(t, unittest.PrepareTestDatabase())
user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2})

// A nupkg that only contains a zero-byte "_._" placeholder entry in
// addition to the nuspec, mirroring the packages reported in the issue.
// The same bytes are uploaded twice (as two different packages) so that
// the second upload reuses the blob row from the first one.
nupkg := test.WriteZipArchive(map[string]string{
"package.nuspec": "<package><metadata><id>nuget.repro</id><version>1.0.0</version></metadata></package>",
"lib/netstandard2.0/_._": "",
}).Bytes()
nupkgSum := sha256.Sum256(nupkg)
key := packages_module.BlobHash256Key(hex.EncodeToString(nupkgSum[:]))
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", nupkg)
require.NoError(t, err)
assert.NoError(t, contentStore.Has(key))

// Simulate the storage inconsistency: the blob row survives but its file
// is missing (this is the state the issue reporter had, where deleting and
// re-publishing the package never restored the file).
require.NoError(t, contentStore.Delete(key))
assert.Error(t, contentStore.Has(key))

// 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", nupkg)
require.NoError(t, err)

// The blob file must be present and both packages must be downloadable
assert.NoError(t, contentStore.Has(key))
for _, pf := range []*packages_model.PackageFile{pf1, pf2} {
s, _, _, err := OpenFileForDownload(t.Context(), pf, http.MethodGet)
require.NoError(t, err)
data, err := io.ReadAll(s)
require.NoError(t, err)
assert.NoError(t, s.Close())
assert.Equal(t, nupkg, data)
}
}

func uploadPackage(t *testing.T, user *user_model.User, name, filename string, data []byte) (*packages_model.PackageFile, error) {
_, pf, err := CreatePackageAndAddFile(t.Context(),
&PackageCreationInfo{
PackageInfo: PackageInfo{
Owner: user,
PackageType: packages_model.TypeNuGet,
Name: name,
Version: "1.0.0",
},
SemverCompatible: true,
Creator: user,
},
&PackageFileCreationInfo{
PackageFileInfo: PackageFileInfo{Filename: filename},
Creator: user,
Data: mustHashedBuffer(t, data),
IsLead: true,
})
return pf, err
}

func mustHashedBuffer(t *testing.T, data []byte) *packages_module.HashedBuffer {
buf, err := packages_module.CreateHashedBufferFromReader(bytes.NewReader(data))
require.NoError(t, err)
return buf
}

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}
Expand Down
Loading