Skip to content

Commit d27114c

Browse files
committed
Merge remote-tracking branch 'giteaofficial/main'
* giteaofficial/main: fix(packages/container): data race when uploading container blobs concurrently (go-gitea#36524)
2 parents 88d5608 + 65d93d8 commit d27114c

File tree

4 files changed

+75
-8
lines changed

4 files changed

+75
-8
lines changed

models/packages/package_blob.go

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -43,20 +43,27 @@ func GetOrInsertBlob(ctx context.Context, pb *PackageBlob) (*PackageBlob, bool,
4343

4444
existing := &PackageBlob{}
4545

46-
has, err := e.Where(builder.Eq{
46+
hashCond := builder.Eq{
4747
"size": pb.Size,
4848
"hash_md5": pb.HashMD5,
4949
"hash_sha1": pb.HashSHA1,
5050
"hash_sha256": pb.HashSHA256,
5151
"hash_sha512": pb.HashSHA512,
52-
}).Get(existing)
52+
}
53+
54+
has, err := e.Where(hashCond).Get(existing)
5355
if err != nil {
5456
return nil, false, err
5557
}
5658
if has {
5759
return existing, true, nil
5860
}
5961
if _, err = e.Insert(pb); err != nil {
62+
// Handle race condition: another request may have inserted the same blob
63+
// between our SELECT and INSERT. Retry the SELECT to get the existing blob.
64+
if has, _ = e.Where(hashCond).Get(existing); has {
65+
return existing, true, nil
66+
}
6067
return nil, false, err
6168
}
6269
return pb, false, nil
Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,51 @@
1+
// Copyright 2026 The Gitea Authors. All rights reserved.
2+
// SPDX-License-Identifier: MIT
3+
4+
package packages
5+
6+
import (
7+
"testing"
8+
9+
"code.gitea.io/gitea/models/unittest"
10+
11+
"github.com/stretchr/testify/assert"
12+
"github.com/stretchr/testify/require"
13+
"golang.org/x/sync/errgroup"
14+
)
15+
16+
func TestGetOrInsertBlobConcurrent(t *testing.T) {
17+
require.NoError(t, unittest.PrepareTestDatabase())
18+
19+
testBlob := PackageBlob{
20+
Size: 123,
21+
HashMD5: "md5",
22+
HashSHA1: "sha1",
23+
HashSHA256: "sha256",
24+
HashSHA512: "sha512",
25+
}
26+
27+
const numGoroutines = 3
28+
var wg errgroup.Group
29+
results := make([]*PackageBlob, numGoroutines)
30+
existed := make([]bool, numGoroutines)
31+
for idx := range numGoroutines {
32+
wg.Go(func() error {
33+
blob := testBlob // Create a copy of the test blob for each goroutine
34+
var err error
35+
results[idx], existed[idx], err = GetOrInsertBlob(t.Context(), &blob)
36+
return err
37+
})
38+
}
39+
require.NoError(t, wg.Wait())
40+
41+
// then: all GetOrInsertBlob succeeds with the same blob ID, and only one indicates it did not exist before
42+
existedCount := 0
43+
assert.NotNil(t, results[0])
44+
for i := range numGoroutines {
45+
assert.Equal(t, results[0].ID, results[i].ID)
46+
if existed[i] {
47+
existedCount++
48+
}
49+
}
50+
assert.Equal(t, numGoroutines-1, existedCount)
51+
}

routers/api/packages/container/blob.go

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -26,9 +26,18 @@ import (
2626

2727
// saveAsPackageBlob creates a package blob from an upload
2828
// The uploaded blob gets stored in a special upload version to link them to the package/image
29-
func saveAsPackageBlob(ctx context.Context, hsr packages_module.HashedSizeReader, pci *packages_service.PackageCreationInfo) (*packages_model.PackageBlob, error) { //nolint:unparam // PackageBlob is never used
29+
// There will be concurrent uploading for the same blob, so it needs a global lock per blob hash
30+
func saveAsPackageBlob(ctx context.Context, hsr packages_module.HashedSizeReader, pci *packages_service.PackageCreationInfo) (*packages_model.PackageBlob, error) { //nolint:unparam //returned PackageBlob is never used
3031
pb := packages_service.NewPackageBlob(hsr)
32+
err := globallock.LockAndDo(ctx, "container-blob:"+pb.HashSHA256, func(ctx context.Context) error {
33+
var err error
34+
pb, err = saveAsPackageBlobInternal(ctx, hsr, pci, pb)
35+
return err
36+
})
37+
return pb, err
38+
}
3139

40+
func saveAsPackageBlobInternal(ctx context.Context, hsr packages_module.HashedSizeReader, pci *packages_service.PackageCreationInfo, pb *packages_model.PackageBlob) (*packages_model.PackageBlob, error) {
3241
exists := false
3342

3443
contentStore := packages_module.NewContentStore()
@@ -67,7 +76,7 @@ func saveAsPackageBlob(ctx context.Context, hsr packages_module.HashedSizeReader
6776
return createFileForBlob(ctx, uploadVersion, pb)
6877
})
6978
if err != nil {
70-
if !exists {
79+
if !exists && pb != nil { // pb can be nil if GetOrInsertBlob failed
7180
if err := contentStore.Delete(packages_module.BlobHash256Key(pb.HashSHA256)); err != nil {
7281
log.Error("Error deleting package blob from content store: %v", err)
7382
}

services/packages/container/blob_uploader.go

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -63,10 +63,10 @@ func NewBlobUploader(ctx context.Context, id string) (*BlobUploader, error) {
6363
}
6464

6565
return &BlobUploader{
66-
model,
67-
hash,
68-
f,
69-
false,
66+
PackageBlobUpload: model,
67+
MultiHasher: hash,
68+
file: f,
69+
reading: false,
7070
}, nil
7171
}
7272

0 commit comments

Comments
 (0)