Skip to content

fixes write close #96

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
abhinavcode wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fixes write close #96

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion registry/app/pkg/docker/controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -422,13 +422,13 @@ func (c *Controller) CancelBlobUpload(
errors = append(errors, e)
return responseHeaders, errors
}
defer blobCtx.Upload.Close()

responseHeaders = &commons.ResponseHeaders{
Headers: map[string]string{"Docker-Upload-UUID": blobCtx.UUID},
}

//nolint:contextcheck
// Note: Cancel() internally closes the writer, so no defer Close() needed
if err := blobCtx.Upload.Cancel(blobCtx); err != nil {
log.Ctx(ctx).Error().Stack().Err(err).Msgf("error encountered canceling upload: %v", err)
errors = append(errors, errcode.ErrCodeUnknown.WithDetail(err))
Expand Down
9 changes: 8 additions & 1 deletion registry/app/pkg/docker/local.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1094,7 +1094,13 @@ func (r *LocalRegistry) PushBlob(
return responseHeaders, errs
}

defer ctx.Upload.Close()
// Track if upload was cancelled to prevent double-close in defer
uploadCancelled := false
defer func() {
if !uploadCancelled {
ctx.Upload.Close()
}
}()
Comment on lines +1097 to +1103

@coderabbitai coderabbitai Bot Feb 24, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 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 5

Repository: 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 3

Repository: 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 10

Repository: 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 -20

Repository: 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 -n

Repository: 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.go

Repository: 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 -50

Repository: 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 -20

Repository: 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 -50

Repository: 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 -100

Repository: abhinavcode/harness

Length of output: 3981

🏁 Script executed:

# Read complete blobwriter.go Cancel method
cd registry && sed -n '120,135p' app/storage/blobwriter.go

Repository: abhinavcode/harness

Length of output: 565

🏁 Script executed:

# Read complete globalblobwriter.go Cancel method
cd registry && sed -n '173,195p' app/storage/globalblobwriter.go

Repository: 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.go

Repository: 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 -50

Repository: 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 -50

Repository: 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 -10

Repository: 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 -150

Repository: 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 -20

Repository: 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.go

Repository: 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.


if artInfo.Digest == "" {
// no digest? return error, but allow retry.
Expand Down Expand Up @@ -1165,6 +1171,7 @@ func (r *LocalRegistry) PushBlob(
}
//nolint:contextcheck
// Clean up the backend blob data if there was an error.
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)
Expand Down