fix(packages/container): data race when uploading container blobs concurrently

[noeljackson]

Fix data race when uploading container blobs concurrently

[noeljackson]

Have you ever seen the real panic? Or it is just your guess & AI hallucination?

Yes it happens to me. I build three packages at one time and two of them share the first packages base layer and this happens.

ERROR: failed to build: failed to solve: error writing layer blob: failed commit on ref "layer-sha256:899c661b5a286b4b2b60fcf067a3e84167c361a5dcede57a9b1fb8588847e4bf": unexpected status from PUT request to https://git.hidden.com/v2/package/processor-worker-dev/blobs/uploads/yklmcx18cy8ntatrhn3oawbxj?digest=sha256%3A899c661b5a286b4b2b60fcf067a3e84167c361a5dcede57a9b1fb8588847e4bf: 400 Bad Request
unknown
::error::buildx failed with: unknown
  ❌  Failure - Main Build and push

[wxiaoguang]

Have you ever seen the real panic? Or it is just your guess & AI hallucination?

Yes it happens to me. I build three packages at one time and two of them share the first packages base layer and this happens.

What is the panic? Only error log means nothing.

Is it really caused by such "race condition in BlobUploader"? BlobUploader is designed to be only used in one request, where is the "race condition"?

[wxiaoguang]

When multiple goroutines call
NewBlobUploader() with the same upload ID, they share the same instance.

Is it true? Every NewBlobUploader creates a new BlobUploader

[noeljackson]

When multiple goroutines call
NewBlobUploader() with the same upload ID, they share the same instance.

Is it true? Every NewBlobUploader creates a new BlobUploader

You're right that each NewBlobUploader creates a new instance. The race isn't between requests it's within a single request.

In PutBlobsUpload (container.go):

• Line 418: defer uploader.Close()
• Line 432: saveAsPackageBlob(ctx, uploader, ...) passes uploader to content store
• Line 454: _ = uploader.Close()

The comment at lines 451-453 explains: "Some SDK (e.g.: minio) will close the Reader if it is also a Closer after uploading."

So within ONE request, Close() can be called by:

1. The content store/SDK after reading
2. The explicit call at line 454
3. The deferred call at line 418

Without mutex protection, if the SDK closes the file in a goroutine or callback while the explicit Close() runs, we get a race on u.file. The mutex makes Close() idempotent by checking if u.file == nil before closing.

I don't have a panic stack trace because the issue manifests as 400 Bad Request from the client side Gitea doesn't panic, it returns an error. However, the fix eliminated the intermittent 400 errors during parallel container builds.

This fix works for me, without it, there is issue every time i build. If not good enough, I can rollback, trigger panics and improve the details, but this does fix my problem.

[wxiaoguang]

3.

Without mutex protection, if the SDK closes the file in a goroutine or callback while the explicit Close() runs, we get a race on u.file. The mutex makes Close() idempotent by checking if u.file == nil before closing.

Most (or all) io.ReadClosers are safe to be closed twice. The same to os.File, so it won't really cause any problem either.

---

if the SDK closes the file in a goroutine or callback while the explicit Close() runs,

Why such goroutine can exist? The MinIO SDK uploading should have stopped after saveAsPackageBlob

---

Since it isn't the real cause, I don't think it really fixes.

[wxiaoguang]

This fix works for me, without it, there is issue every time i build. If not good enough, I can rollback, trigger panics and improve the details, but this does fix my problem.

Yes, I think we need to figure out the root cause. We need a fix which is theoretically right and does fix a real cause.

[wxiaoguang]

Awesome, it looks right now. 👍

[wxiaoguang]

I will make some improvements to the tests. By the way, no need to rebase or force push then (https://github.com/go-gitea/gitea/blob/main/CONTRIBUTING.md#maintaining-open-prs)

[noeljackson]

I will make some improvements to the tests. By the way, no need to rebase or force push then (https://github.com/go-gitea/gitea/blob/main/CONTRIBUTING.md#maintaining-open-prs)

I am sorry my agent ignored instructions.

Thank you for helping to fix this. It's a big blocker for me.

[noeljackson]

Found the Real Root Cause

Thanks for pushing back on this. You were right that the BlobUploader mutex wasn't the fix. I reproduced the issue by reverting to upstream Gitea 1.25.2 and running parallel container builds. Here's what I found in the logs:

PUT /v2/sonica/processor-worker-dev/blobs/uploads/yumlyda1lavwk5mwqtlgrsnxo?digest=sha256:2535496781...
    panic @ container/container.go:400(container.PutBlobsUpload)
    err=runtime error: invalid memory address or nil pointer dereference

PUT /v2/sonica/processor-producer-dev/blobs/uploads/fzp5tduxkih9jc1hbhylmdgkv?digest=sha256:2535496781...
    panic @ container/container.go:400(container.PutBlobsUpload)
    err=runtime error: invalid memory address or nil pointer dereference

Notice that the same digest (sha256:2535496781...) was being pushed by three parallel builds at the same time. The websocket build succeeded with 201 Created, but the worker and producer builds both panicked.

The Actual Bug

The problem is in GetOrInsertBlob in models/packages/package_blob.go:

if _, err = e.Insert(pb); err != nil {
    return nil, false, err  // Returns nil for pb!
}

When multiple parallel requests try to insert the same blob, they all do a SELECT first and find nothing. Then one INSERT succeeds while the others fail with a duplicate key violation. The function returns nil for the PackageBlob in this case.

The caller in blob.go then tries to clean up on error:

if err != nil {
    if !exists {
        contentStore.Delete(packages_module.BlobHash256Key(pb.HashSHA256))  // pb is nil here!
    }
}

This causes the nil pointer panic.

The Fix

I've updated the PR to handle this race properly by retrying the SELECT after an INSERT fails:

if _, err = e.Insert(pb); err != nil {
    // Another request inserted the same blob between our SELECT and INSERT.
    // Retry the SELECT to get the existing blob.
    if has, _ = e.Where(hashCond).Get(existing); has {
        return existing, true, nil
    }
    return nil, false, err
}

This follows the same pattern used by TryInsertPackage and TryInsertFile which handle the same type of race condition.

I've reverted all the BlobUploader mutex changes since you correctly pointed out that each request creates its own instance.

[wxiaoguang]

Made some more changes, do the new changes look good to you?

[noeljackson]

Ah... I see. Yes, the changes look great! The global lock by hash is a cleaner solution. I should have seen this. Thank you.