fixes write close#96
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
📝 WalkthroughWalkthroughThe changes modify blob upload cleanup to prevent double-closing of the upload writer. The CancelBlobUpload function now delegates writer closure to the Cancel() method, while the local upload handler uses a guarded defer that conditionally closes the upload only if cancellation didn't occur. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 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.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
registry/app/pkg/docker/controller.go (1)
431-435:⚠️ Potential issue | 🟠 MajorThe resource leak concern is valid —
blobWriter.Cancel()does not callClose()whenfileWriter.Cancel()fails.The comment claiming "Cancel() internally closes the writer" is incorrect for error paths. Both
blobWriter.Cancel()andglobalBlobWriter.Cancel()implementations return early iffileWriter.Cancel()returns an error, skipping the subsequentClose()call entirely. By removing thedefer blobCtx.Upload.Close(), there is no recovery path ifCancel()fails, leaving the writer open.blobwriter.go (lines 122–132)
func (bw *blobWriter) Cancel(ctx context.Context) error { dcontext.GetLogger(ctx, log.Debug()).Msg("(*blobWriter).Cancel") if err := bw.fileWriter.Cancel(ctx); err != nil { return err // Early return, bw.Close() never called } if err := bw.Close(); err != nil { dcontext.GetLogger(ctx, log.Error()).Msgf("error closing blobwriter: %s", err) } return bw.removeResources(ctx) }Either restore the
defer Close()safety net or refactorCancel()to unconditionally callClose()on all code paths.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@registry/app/pkg/docker/controller.go` around lines 431 - 435, The comment shows Cancel() implementations (blobWriter.Cancel and globalBlobWriter.Cancel) return early when fileWriter.Cancel() errors, skipping Close(); restore a safety net by reintroducing a deferred Close() on the upload writer (e.g., call defer blobCtx.Upload.Close() immediately after obtaining blobCtx.Upload) or change blobWriter.Cancel/globalBlobWriter.Cancel so they always call bw.Close() even if fileWriter.Cancel() fails (ensure Close() is invoked in all error paths and log any Close() error); reference blobCtx.Upload.Cancel/blobCtx.Upload.Close and blobWriter.Cancel/globalBlobWriter.Cancel/fileWriter.Cancel when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@registry/app/pkg/docker/local.go`:
- Around line 1097-1103: The defer that closes ctx.Upload is being suppressed by
setting uploadCancelled = true before calling Cancel(), which leaks the writer
if blobWriter.Cancel() fails; in the code paths around the Upload/Cancel logic
(look for the uploadCancelled variable, the deferred ctx.Upload.Close(), and the
BlobWriter.Cancel()/blobWriter.Cancel() call sites) move the assignment
uploadCancelled = true so it is only set after Cancel() returns nil, and if
Cancel() returns an error ensure you do not set uploadCancelled (let defer run)
and propagate/log the error as appropriate so Close() still executes on failure.
---
Outside diff comments:
In `@registry/app/pkg/docker/controller.go`:
- Around line 431-435: The comment shows Cancel() implementations
(blobWriter.Cancel and globalBlobWriter.Cancel) return early when
fileWriter.Cancel() errors, skipping Close(); restore a safety net by
reintroducing a deferred Close() on the upload writer (e.g., call defer
blobCtx.Upload.Close() immediately after obtaining blobCtx.Upload) or change
blobWriter.Cancel/globalBlobWriter.Cancel so they always call bw.Close() even if
fileWriter.Cancel() fails (ensure Close() is invoked in all error paths and log
any Close() error); reference blobCtx.Upload.Cancel/blobCtx.Upload.Close and
blobWriter.Cancel/globalBlobWriter.Cancel/fileWriter.Cancel when making the
change.
| // Track if upload was cancelled to prevent double-close in defer | ||
| uploadCancelled := false | ||
| defer func() { | ||
| if !uploadCancelled { | ||
| ctx.Upload.Close() | ||
| } | ||
| }() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the code at lines 1097-1103 and around line 1174-1178
cd registry && cat -n app/pkg/docker/local.go | sed -n '1090,1180p'Repository: abhinavcode/harness
Length of output: 3068
🏁 Script executed:
# Search for Cancel() calls and the context around line 1174
rg -n "uploadCancelled\s*=" app/pkg/docker/local.go -B 5 -A 5Repository: abhinavcode/harness
Length of output: 126
🏁 Script executed:
# Look for the Cancel() method signature and any related code patterns
rg -n "Cancel\(" app/pkg/docker/local.go -B 3 -A 3Repository: abhinavcode/harness
Length of output: 126
🏁 Script executed:
# Find the type definition of ctx.Upload (likely BlobWriter)
cd registry && rg -n "type.*BlobWriter" app/pkg/docker/local.go -B 2 -A 10Repository: abhinavcode/harness
Length of output: 45
🏁 Script executed:
# Search for ctx.Upload assignment or type hints
cd registry && rg -n "ctx\.Upload\s*=" app/pkg/docker/local.go | head -20Repository: abhinavcode/harness
Length of output: 176
🏁 Script executed:
# Look for any imports of distribution library
cd registry && head -50 app/pkg/docker/local.go | grep -n "import\|distribution"Repository: abhinavcode/harness
Length of output: 74
🏁 Script executed:
# Read the imports section
cd registry && head -60 app/pkg/docker/local.go | cat -nRepository: abhinavcode/harness
Length of output: 2557
🏁 Script executed:
# Look at line 1448 and context to understand ctx.Upload assignment
cd registry && sed -n '1440,1460p' app/pkg/docker/local.goRepository: abhinavcode/harness
Length of output: 442
🏁 Script executed:
# Search for BlobWriter or Upload interface definition in the codebase
cd registry && rg -n "BlobWriter\|type Upload" --type go -A 5 | head -50Repository: abhinavcode/harness
Length of output: 45
🏁 Script executed:
# Search for BlobWriter in storage package
cd registry && find . -type f -name "*.go" -path "*/storage/*" -exec grep -l "BlobWriter\|Upload" {} \; | head -20Repository: abhinavcode/harness
Length of output: 355
🏁 Script executed:
# Look for the actual type/interface used
cd registry && rg -n "type.*\bUpload\b" --type go -A 3 | head -50Repository: abhinavcode/harness
Length of output: 45
🏁 Script executed:
# Search for Cancel method definition
cd registry && rg -n "func.*Cancel\(" --type go -B 2 -A 10 | head -100Repository: abhinavcode/harness
Length of output: 3981
🏁 Script executed:
# Read complete blobwriter.go Cancel method
cd registry && sed -n '120,135p' app/storage/blobwriter.goRepository: abhinavcode/harness
Length of output: 565
🏁 Script executed:
# Read complete globalblobwriter.go Cancel method
cd registry && sed -n '173,195p' app/storage/globalblobwriter.goRepository: abhinavcode/harness
Length of output: 783
🏁 Script executed:
# Check the fileWriter type to understand Cancel contract
cd registry && rg -n "fileWriter.*interface\|fileWriter\s*\w+\s*interface" --type go -B 2 -A 15 app/storage/blobwriter.goRepository: abhinavcode/harness
Length of output: 45
🏁 Script executed:
# Check what type ctx.Upload actually is by looking at the function signature
cd registry && sed -n '1000,1100p' app/storage/blobwriter.go | head -50Repository: abhinavcode/harness
Length of output: 45
🏁 Script executed:
# Check for interface definition of BlobWriter/Upload
cd registry && rg -n "type.*interface.*\{" app/storage/blobwriter.go -A 20 | head -50Repository: abhinavcode/harness
Length of output: 45
🏁 Script executed:
# Confirm which implementation is typically used
cd registry && rg -n "globalBlobWriter\|blobWriter" app/pkg/docker/local.go | head -10Repository: abhinavcode/harness
Length of output: 45
🏁 Script executed:
# Check the blobwriter.go file structure and Close method
cd registry && cat -n app/storage/blobwriter.go | head -150Repository: abhinavcode/harness
Length of output: 5296
🏁 Script executed:
# Look for the interface definition of BlobWriter
cd registry && rg -n "interface\s*\{" app/storage/ --type go -B 5 | grep -A 15 "Cancel\|Close"Repository: abhinavcode/harness
Length of output: 952
🏁 Script executed:
# Check what types implement the blob writer interface
cd registry && rg -n "ctx\.Upload\." app/pkg/docker/local.go | head -20Repository: abhinavcode/harness
Length of output: 235
🏁 Script executed:
# Check the BlobWriter interface definition to confirm Cancel contract
cd registry && sed -n '100,120p' app/storage/blobs.goRepository: abhinavcode/harness
Length of output: 1044
uploadCancelled = true set before Cancel() creates resource leak if Cancel() fails.
The BlobWriter.Cancel() interface contract states: "frees any associated resources". However, the blobWriter.Cancel() implementation (line 124-125) returns immediately on error without calling Close(), leaving the writer potentially unclosed and leaked.
Setting uploadCancelled = true before calling Cancel() (line 1174) suppresses the deferred Close() even if Cancel() fails and doesn't close the writer internally.
Move uploadCancelled = true to only after a successful Cancel():
Fix: only suppress defer on successful cancel
- uploadCancelled = true // Mark as cancelled to prevent defer Close() from running
if err := ctx.Upload.Cancel(ctx); err != nil {
// If the cleanup fails, all we can do is observe and report.
log.Ctx(ctx).Error().Msgf("error canceling upload after error: %v", err)
+ } else {
+ uploadCancelled = true // Cancel() closed the writer; suppress defer Close()
}Also applies to: 1174-1178
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@registry/app/pkg/docker/local.go` around lines 1097 - 1103, The defer that
closes ctx.Upload is being suppressed by setting uploadCancelled = true before
calling Cancel(), which leaks the writer if blobWriter.Cancel() fails; in the
code paths around the Upload/Cancel logic (look for the uploadCancelled
variable, the deferred ctx.Upload.Close(), and the
BlobWriter.Cancel()/blobWriter.Cancel() call sites) move the assignment
uploadCancelled = true so it is only set after Cancel() returns nil, and if
Cancel() returns an error ensure you do not set uploadCancelled (let defer run)
and propagate/log the error as appropriate so Close() still executes on failure.
1 participant
Summary by CodeRabbit
Bug Fixes