fix(worker): avoid shared err race in multipart upload#443
Conversation
📝 WalkthroughWalkthroughMultipart part uploads now use a dedicated retrying helper that returns completed-part metadata. Concurrent upload handling delegates to this helper, and tests cover successful uploads, propagated failures, and per-part error isolation. ChangesMultipart upload isolation
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Comment |
8f291dc to
c2d83bc
Compare
Each part-upload goroutine wrote the shared multipart function-scope err instead of a goroutine-local one. Concurrent parts raced on err, and a failing part whose err was overwritten to nil by a peer could skip its error return and dereference a nil upload result, panicking the worker. Extract the per-part upload into uploadPart, which keeps the error and result local to each call, and add tests covering the success, failure, and concurrent paths under the race detector. Closes NVIDIA#442 Signed-off-by: mesutoezdil <mesudozdil@gmail.com>
c2d83bc to
126504f
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/compute-plane-services/worker-utils/worker/large.go (1)
251-253: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPreserve the failing part number in the returned error.
This new error boundary returns the raw SDK error, so
errgroup.Wait()cannot identify which part failed. Wrap it with%wand includepartInput.PartNumberwhile preservingerrors.Isbehavior.As per path instructions, Go changes under
src/**should follow subtree error-handling conventions and preserve error context with%w.Proposed fix
- return types.CompletedPart{}, err + return types.CompletedPart{}, fmt.Errorf("upload part %d: %w", aws.ToInt32(partInput.PartNumber), err)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/compute-plane-services/worker-utils/worker/large.go` around lines 251 - 253, Update the error return in the part-processing flow to wrap the SDK error with `%w` and include `partInput.PartNumber` in the message, preserving `errors.Is` compatibility while allowing `errgroup.Wait()` callers to identify the failed part.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/compute-plane-services/worker-utils/worker/large.go`:
- Around line 251-253: Update the error return in the part-processing flow to
wrap the SDK error with `%w` and include `partInput.PartNumber` in the message,
preserving `errors.Is` compatibility while allowing `errgroup.Wait()` callers to
identify the failed part.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: b35d84c1-0bf7-4299-ad81-a64761204e3d
📒 Files selected for processing (2)
src/compute-plane-services/worker-utils/worker/large.gosrc/compute-plane-services/worker-utils/worker/large_helpers_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- src/compute-plane-services/worker-utils/worker/large_helpers_test.go
TL;DR
In
multipartUpload, every part-upload goroutine wrote the shared function-scopeerrinstead of a goroutine-local one. Concurrent parts raced onerr(
go test -raceflags it), and a failing part whoseerrwas overwritten to nilby a peer could skip its error return and dereference a nil upload result,
panicking the worker on the large-response upload path. Extract the per-part
upload into
uploadPart, keeping the error and result local to each call.Issues
Closes #442
Testing
go test ./worker/ -racepasses. AddedTestUploadPartcovering success,failure (returns the error with no nil dereference), and a concurrent run where
one part fails and must not clobber the others. Verified the pre-fix shared-
errpattern reports a data race under
-race. No QA needed.Checklist
Summary by CodeRabbit