perf(backend): parallelize retained PATCH copies - #821
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughMultipart PATCH uploads now batch retained-part copy tasks and execute them with bounded concurrency. Copy failures cancel remaining work and trigger detached, timeout-bound multipart cleanup. Tests cover unit, backend, and end-to-end success, failure, and cancellation paths. ChangesMultipart patch copy
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant PatchRequest
participant patchcopy.CopyOrAbort
participant S3
participant MultipartUpload
PatchRequest->>patchcopy.CopyOrAbort: retained part tasks and concurrency limit
patchcopy.CopyOrAbort->>S3: bounded UploadPartCopy calls
S3-->>patchcopy.CopyOrAbort: copy results or PartError
patchcopy.CopyOrAbort->>MultipartUpload: abort after copy failure
MultipartUpload-->>PatchRequest: cleanup result
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pkg/backend/internal/patchcopy/copy.go (1)
59-64: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDefine package-level sentinels for the two static parameter-validation errors. Both checks build a fixed message with
fmt.Errorf(no format verbs), so callers cannot match them witherrors.Is, and per the guideline sentinel errors belong at package level viaerrors.New.
pkg/backend/internal/patchcopy/copy.go#L59-L64: return a package-levelErrInvalidConcurrencyinstead offmt.Errorf("patch copy concurrency must be positive").pkg/backend/internal/patchcopy/copy.go#L146-L154: return a package-levelErrInvalidAbortTimeoutinstead offmt.Errorf("patch abort timeout must be positive")at Line 149.As per coding guidelines, "Use the
ErrFoopattern for sentinel errors and define them at package level witherrors.New".♻️ Proposed sentinels
+// ErrInvalidConcurrency reports a non-positive copy concurrency limit. +var ErrInvalidConcurrency = errors.New("patch copy concurrency must be positive") + +// ErrInvalidAbortTimeout reports a non-positive multipart abort timeout. +var ErrInvalidAbortTimeout = errors.New("patch abort timeout must be positive")🤖 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 `@pkg/backend/internal/patchcopy/copy.go` around lines 59 - 64, Define package-level ErrInvalidConcurrency and ErrInvalidAbortTimeout sentinels with errors.New in pkg/backend/internal/patchcopy/copy.go, then return them directly from the concurrency validation at lines 59-64 and abort-timeout validation at lines 146-154 instead of constructing fixed fmt.Errorf messages.Source: Coding guidelines
pkg/backend/patch.go (1)
338-338: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider logging discarded abort errors.
All five
patchcopy.Abortcall sites drop the error, so a failed cleanup leaves an orphaned multipart upload with no signal. Alogger.Warnon non-nil abort error would make these leaks visible.Also applies to: 352-352, 361-361, 407-407
🤖 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 `@pkg/backend/patch.go` at line 338, Update all five patchcopy.Abort call sites in the relevant patch handling flows to capture the returned error and emit a logger.Warn when cleanup fails, including enough context to identify the multipart upload or S3 key. Preserve the existing abort behavior and avoid logging when Abort succeeds.
🤖 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 `@pkg/backend/internal/patchcopy/copy.go`:
- Around line 59-64: Define package-level ErrInvalidConcurrency and
ErrInvalidAbortTimeout sentinels with errors.New in
pkg/backend/internal/patchcopy/copy.go, then return them directly from the
concurrency validation at lines 59-64 and abort-timeout validation at lines
146-154 instead of constructing fixed fmt.Errorf messages.
In `@pkg/backend/patch.go`:
- Line 338: Update all five patchcopy.Abort call sites in the relevant patch
handling flows to capture the returned error and emit a logger.Warn when cleanup
fails, including enough context to identify the multipart upload or S3 key.
Preserve the existing abort behavior and avoid logging when Abort succeeds.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: eac74c60-d537-4b22-b576-d0a36ca09a7c
📒 Files selected for processing (4)
pkg/backend/internal/patchcopy/copy.gopkg/backend/internal/patchcopy/copy_test.gopkg/backend/patch.gopkg/backend/patch_test.go
qiffang
left a comment
qiffang
left a comment
There was a problem hiding this comment.
Dev-Lead review (dat9-dev1) — exact head 63fb4c28ef9863aff71e69f2b58fa16c8fdaba65
Reviewed the tip commit 63fb4c2 (4 files, +897/-14: new pkg/backend/internal/patchcopy/{copy.go,copy_test.go} + patch.go wiring + patch_test.go). All Option-A gates pass on my axis. GREEN.
✅ Bounded concurrency
patchcopy.Copy: workerCount = min(maxConcurrency, len(tasks)), unbuffered jobs channel, fixed worker pool. patch.go sets patchPartCopyConcurrency = 8. No unbounded fan-out.
✅ First-error cancel → drain → exactly-once abort
failureOnce sync.Oncerecords only the FIRSTPartErrorand callscancel(); workers observecopyCtx.Err()and stop; the feed loopbreak feeds oncopyCtx.Done(), thenclose(jobs)+workers.Wait()— all workers drain before Copy returns.CopyOrAbortrunsCopy(fully drained) thenAbortonce → abort happens after every worker stopped, exactly once.TestCopyOrAbortWaitsAndAbortsExactlyOnce/TestCopyOrAbortDoesNotAbortAfterSuccesscover both.
✅ Detached abort — ctx-cancel cannot skip cleanup
Abort uses context.WithoutCancel(ctx) + timeout, so caller cancellation still runs the AbortMultipartUpload. TestCopyOrAbortUsesDetachedContextAfterParentCancellation / TestAbortUsesDetachedContext cover it.
✅ Part/ETag ordering — safe by design (not fragile to out-of-order completion)
patchcopy.Copy discards the UploadPartCopy ETag, and the pre-#821 code did too — no contract change. Copied-part ETags are collected at confirm-time via ListParts (upload.go:951 → CompleteMultipartUpload at 1010), which returns parts by number from S3. So concurrent out-of-order copy completion cannot corrupt the final assembly — the ordering is decoupled to the ListParts step. TestPatchUploadCopiesRetainedPartsConcurrentlyAndCompletesInPartOrder asserts this.
✅ No metadata written before copy success
In patch.go, CopyOrAbort (line 307) returns an error before any reservation/metadata persist (return nil, "copy retained parts: %w" at 324); the plan/reservation is created only after copies succeed. On copy failure: abort inside CopyOrAbort + return, no metadata. TestPatchUploadCopyFailureWaitsThenAbortsWithoutMetadata covers it.
✅ Option B not mixed in
Only the 4 copy-path files; no session state-machine / async changes. Scope-clean per adversary-1's constraint.
Tests
pkg/backend/internal/patchcopy passes with -race locally (11 discriminating tests: bounded concurrency, first-failure cancel, parent-cancel, exactly-once abort, no-abort-after-success, detached-context, invalid-concurrency/timeout). The patch_test.go integration tests (...ConcurrentlyAndCompletesInPartOrder, ...CopyFailureWaitsThenAbortsWithoutMetadata) require testcontainers → run on hosted CI (locally they hit the known rootless Docker not found TestMain gate, unrelated). Please confirm hosted CI green before merge.
Net: my correctness axis on the parallelization is GREEN. Awaiting adversary-1/adversary-2 exact-head concurrence + hosted CI. drive9 is qiffang-owned → merge on qiffang's「合」.
adversary-2 — GREEN @
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 63fb4c28ef
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ); err != nil { | ||
| failureOnce.Do(func() { | ||
| failure = &PartError{PartNumber: task.PartNumber, Err: err} | ||
| cancel() |
There was a problem hiding this comment.
When one retained copy fails while other AWS UploadPartCopy requests are in flight, this cancellation can make their client calls return before S3 has necessarily stopped the server-side copies; workers.Wait therefore does not establish that those uploads have settled. CopyOrAbort subsequently aborts exactly once, but S3's AbortMultipartUpload contract warns that in-progress uploads may finish after an abort and that repeated aborts can be necessary, so this failure path can leave orphaned, billable multipart parts. Either let active copies settle without canceling their request contexts or verify and retry the abort cleanup.
Useful? React with 👍 / 👎.
qiffang
left a comment
qiffang
left a comment
There was a problem hiding this comment.
GREEN at exact head 63fb4c2.
No blocking findings. I attempted a formal APPROVE, but GitHub rejected it because this connector is authenticated as the PR author; treating this comment as my review verdict.
Reviewed gates:
- Scope stays on Option A only: bounded in-request retained-part copy; no PATCH session-state-machine/async Option B changes.
patchcopy.Copyuses a bounded worker pool (min(maxConcurrency, len(tasks)), wired as 8), records first failure withsync.Once, cancels queued work, and waits for workers before returning.CopyOrAbortperforms cleanup after copy workers drain;Abortusescontext.WithoutCancelplus a bounded timeout, so caller cancellation does not skip MPU cleanup.patch.gocallsCopyOrAbortbefore reservation/upload metadata; failure returns beforereserveUploadOnServer/InsertUpload.- Part/ETag ordering is safe: copied part ETags are not consumed for completion; confirm lists parts from S3 and completes from S3's part-number order.
CopiedPartsis assembled from original task order. - Existing later abort paths now use the same detached bounded abort helper, which is a correctness improvement.
Validation checked:
git diff --check 41503f74493abd4c48ffe3085d7890dcc31a056c...HEAD- gofmt check on changed Go files
go test ./pkg/backend/internal/patchcopy -count=100go test -race ./pkg/backend/internal/patchcopy -count=20go test ./pkg/backend/internal/patchcopy -run '^$' -bench '^BenchmarkCopy$' -benchtime=10x -count=1(~6.7x local 36-part speedup at concurrency 8 vs 1)go test -c ./pkg/backendgo test -c ./pkg/servergo vet ./pkg/backend/internal/patchcopy ./pkg/backend ./pkg/server- Hosted checks are green at this head:
ci,drive9-server-local e2e smoke, CodeRabbit.
Local DB-backed pkg/backend runtime regressions remain blocked on this machine by the known rootless Docker/testcontainers requirement, but hosted e2e/ci covered the runtime gate.
|
Exact-head E2E evidence for
The server E2E tests ran against a temporary isolated Homebrew MySQL 8.4 datadir/database via Backend/server compile, server race compile, vet, gofmt, and |
adversary-2 — GREEN @
|
|
GREEN from adversary-1 for exact head Formal GitHub APPROVE failed because this auth is treated as the PR author, so this comment is my review verdict. Scope checked: delta from prior reviewed head New e2e coverage is sufficiently discriminating: it exercises retained parts > concurrency cap (12 total, 9 copied, 8 in-flight), forces out-of-order copy completion, verifies copied/dirty plan coverage exactly once, checks retained byte ranges including non-full tail, uploads non-contiguous dirty parts, completes the patch, then compares full file bytes and SHA-256 against an independently synthesized expected buffer. Failure and request-cancel tests assert exactly one abort after copy workers drain, no active upload metadata, MPU not completable after abort, and original file bytes unchanged. Validation observed locally: No blockers found. COS live smoke remains tracked separately in #820 and is not a PR #821 merge blocker. |
srstack
left a comment
srstack
left a comment
There was a problem hiding this comment.
Review at 89e37b2 — LGTM ✅
Reviewed the full diff, the new patchcopy package, all three test files, and the patch.go integration. Ran go test ./pkg/backend/internal/patchcopy -count=50, -race -count=10, go vet, go test -c on pkg/backend + pkg/server, gofmt -l, git diff --check — all clean. Also ran two independent adversarial passes (concurrency-correctness and test-quality).
Concurrency correctness — proven sound. The core safety invariant holds on every interleaving:
Copyreturningnil⟺ every task'sUploadPartCopysucceeded.
A task can only be dropped when copyCtx is Done, which requires either the parent ctx was cancelled (→ ctx.Err() != nil, non-nil return) or a worker called cancel() inside failureOnce.Do (→ failure != nil first, non-nil return). So a nil return can never coexist with a dropped/failed task. No deadlock (feeder's select on copyCtx.Done() pairs with every early worker exit), no goroutine leak (close(jobs) unconditional, defer workers.Done() on all paths), no data race on failure (single writer via sync.Once, HB edge through workers.Wait()).
Design highlights worth calling out:
Abortdetaching viacontext.WithoutCancel(ctx)— cleanup survives request cancellation, which is exactly the leak this PR set out to close. Directly asserted (abortCtxErr == nilafter parent cancel).activeAtAbort == 0invariant proves all workers drain before abort touches the MPU, so abort never races an in-flight copy.CopiedPartsrebuilt from the ascendingcopyTasksslice afterCopyOrAbortreturns — ordering is structurally guaranteed by the caller, independent of completion order.
Tests — strong. Bounded concurrency is asserted in both directions (peak maxActive == N and a non-blocking select proving no extra worker started); the E2E test forces genuine out-of-order completion (part 1 gated to finish last, still lands first in CopiedParts) through the real HTTP→backend→LocalS3 path with full SHA-256 integrity verification; abort-exactly-once is counter-asserted; both failure and cancellation paths verify the original object is untouched. No time.Sleep/rand/t.Parallel flakiness. No tautological tests — each would fail against a plausibly broken impl.
Non-blocking follow-ups (none gate merge):
[MINOR]patch_test.go:117 TestPatchUploadCopiesRetainedPartsConcurrentlyAndCompletesInPartOrder— the name promises out-of-order completion, but it releases all workers simultaneously, so it doesn't actually force reorder; it's effectively a concurrency + part-set check. The E2E test covers real reordering, so coverage isn't lost — suggest renaming (or add gates) to match intent.[MINOR]Coverage gap: the presign-failure abort path (patch.go:266-272, also routed throughpatchcopy.Abort) isn't exercised by the new tests — only copy-failure and cancel-failure abort paths are.[NIT]copy.go:116prefers parentctx.Err()over aPartErrorafterWait(). If the parent is cancelled in the narrow window betweenWait()returning and this check while all copies actually succeeded,Copyreports a spurious error. Harmless in practice — the request is already tearing down andCopyOrAbortcorrectly aborts the MPU — but a real copyPartErrorcould likewise be masked by a concurrent deadline, costing diagnostic detail.[NIT]AbortwithabortTimeout <= 0skips cleanup entirely (returns the guard error). Unreachable today (callers passpostS3UploadFinalizeTimeout= 2m), just noting the contract.
Clean, well-scoped, measurably faster (~7.2x at 36 parts), and the leak-safety story is airtight. Approving.
2 participants
Summary
UploadPartCopyloop with a request-scoped, bounded 8-worker poolCorrectness
CopiedPartsis assembled from the original ordered task list, not completion orderListParts, whose implementations return part-number-sorted ETag mappingsSlowDownresponsesValidation
go test ./pkg/backend/internal/patchcopy -count=100go test -race ./pkg/backend/internal/patchcopy -count=20go test ./pkg/backend/internal/patchcopy -run '^$' -bench '^BenchmarkCopy$' -benchtime=10x -count=1go test -c ./pkg/backendgo test -c ./pkg/servergo vet ./pkg/backend/internal/patchcopy ./pkg/backend ./pkg/servergit diff --checkThe two DB-backed LocalS3 integration regressions compile locally. Runtime execution is deferred to hosted CI because this machine has no rootless Docker and
pkg/backendTestMain requires testcontainers MySQL.Fixes #820
Summary by CodeRabbit
New Features
Bug Fixes
Tests