Skip to content

worker multipart upload shares one err across goroutines, racing and risking a nil dereference #442

Description

@mesutoezdil

Description

In src/compute-plane-services/worker-utils/worker/large.go, multipartUpload
launches up to multipartUploadConcurrency part-upload goroutines. Each goroutine
wrote the shared function-scope err variable rather than a goroutine-local one:

var uploadResult *s3.UploadPartOutput
_, _, err = lo.AttemptWithDelay(3, 100*time.Millisecond, func(index int, duration time.Duration) error {
    var err error
    uploadResult, err = client.UploadPart(ctx, partInput)
    return err
})
if err != nil {
    return err
}

The err on the _, _, err = line is the multipartUpload function-scope err
(declared earlier and also reused by the main reader goroutine). Multiple upload
goroutines read and write it concurrently.

Impact

  • Data race on err (flagged by go test -race).
  • If one part's upload fails (its local uploadResult stays nil) but a peer
    goroutine overwrites the shared err to nil before the failing goroutine
    reaches its if err != nil check, the failing goroutine skips the return and
    dereferences uploadResult.ETag on a nil pointer, panicking the worker.
  • The mirror case makes a successful part inherit a peer's error and spuriously
    abort the whole upload.

This is on the large-response upload path (responses over the multipart
threshold).

Definition of Done

  • Each part upload uses a goroutine-local error and result.
  • A unit test exercises the success and failure paths and a concurrent run under
    the race detector, asserting a failing part does not clobber successful parts.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions