From 3a4053c1019668b6678869db27790443f204b610 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ernestas=20Luko=C5=A1evi=C4=8Dius?= Date: Sat, 5 Sep 2026 23:55:33 +0300 Subject: [PATCH] cache/s3: fix a panic, an off-by-one and a discarded abort in the touch path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When a blob is already in the bucket the exporter refreshes its timestamp instead of uploading it again. Three things are wrong on that path. HeadObject's ContentLength was dereferenced without a check. It is a *int64 and the SDK leaves it nil when the response carries no Content-Length header, which several S3-compatible endpoints do. That panicked the daemon rather than failing the export. Check it at the call site and pass the size by value, so touch cannot be handed a nil. buildCopySourceRange clamped with end > objectSize. A copy source range is inclusive, so the last byte it may name is objectSize-1, and an end equal to objectSize addresses one byte past the object. It is reachable when the remaining bytes are exactly one part short of the part size. The abort of a failed multipart upload ran on the context that had just failed, which is usually already cancelled, so the abort failed too. Its error was discarded, so nothing said so. An upload left incomplete keeps its parts, and the bucket keeps charging for them until a lifecycle rule removes them. Abort on a detached context with its own timeout and log a failure. Add a test for the copy source range. The one byte short case fails on the old bound. Signed-off-by: Ernestas Lukoševičius --- cache/remotecache/s3/s3.go | 37 +++++++++++++++---- cache/remotecache/s3/touch_test.go | 59 ++++++++++++++++++++++++++++++ 2 files changed, 88 insertions(+), 8 deletions(-) create mode 100644 cache/remotecache/s3/touch_test.go diff --git a/cache/remotecache/s3/s3.go b/cache/remotecache/s3/s3.go index 3868371eab26..5fb88d672d51 100644 --- a/cache/remotecache/s3/s3.go +++ b/cache/remotecache/s3/s3.go @@ -25,6 +25,7 @@ import ( cacheimporttypes "github.com/moby/buildkit/cache/remotecache/v1/types" "github.com/moby/buildkit/session" "github.com/moby/buildkit/solver" + "github.com/moby/buildkit/util/bklog" "github.com/moby/buildkit/util/compression" "github.com/moby/buildkit/util/progress" "github.com/moby/buildkit/worker" @@ -52,6 +53,10 @@ const ( attrRetryMode = "retry_mode" attrRetryMaxAttempts = "retry_max_attempts" maxCopyObjectSize = 5 * 1024 * 1024 * 1024 + // abortMultipartTimeout bounds the best-effort cleanup of a failed + // multipart upload, which runs on a context detached from the failed + // operation. + abortMultipartTimeout = 30 * time.Second ) type Config struct { @@ -283,7 +288,13 @@ func (e *exporter) Finalize(ctx context.Context) (map[string]string, error) { } if exists != nil { if time.Since(*exists) > e.config.TouchRefresh { - err = e.s3Client.touch(groupCtx, key, size) + // Some S3-compatible endpoints omit Content-Length from + // HeadObject responses, and dereferencing it + // unconditionally took the daemon down. + if size == nil { + return errors.Errorf("failed to touch %s: object has no content length", key) + } + err = e.s3Client.touch(groupCtx, key, *size) if err != nil { return errors.Wrapf(err, "failed to touch file") } @@ -562,7 +573,8 @@ func (s3Client *s3Client) exists(ctx context.Context, key string) (*time.Time, * func buildCopySourceRange(start int64, objectSize int64) string { end := start + maxCopyObjectSize - 1 - if end > objectSize { + // The range is inclusive, so the last addressable byte is objectSize-1. + if end >= objectSize { end = objectSize - 1 } startRange := strconv.FormatInt(start, 10) @@ -570,11 +582,11 @@ func buildCopySourceRange(start int64, objectSize int64) string { return "bytes=" + startRange + "-" + stopRange } -func (s3Client *s3Client) touch(ctx context.Context, key string, size *int64) (err error) { +func (s3Client *s3Client) touch(ctx context.Context, key string, size int64) (err error) { copySource := fmt.Sprintf("%s/%s", s3Client.bucket, key) // CopyObject does not support files > 5GB - if *size < maxCopyObjectSize { + if size < maxCopyObjectSize { cp := &s3.CopyObjectInput{ Bucket: &s3Client.bucket, CopySource: ©Source, @@ -598,13 +610,22 @@ func (s3Client *s3Client) touch(ctx context.Context, key string, size *int64) (e } defer func() { + if err == nil { + return + } abortIn := s3.AbortMultipartUploadInput{ Bucket: &s3Client.bucket, Key: &key, UploadId: output.UploadId, } - if err != nil { - s3Client.AbortMultipartUpload(ctx, &abortIn) + // By the time we get here the operation context is usually already + // cancelled, which would fail the abort and leave the upload to accrue + // storage charges. Abort on a detached context, and report a failure + // rather than discarding it. + abortCtx, cancel := context.WithTimeoutCause(context.WithoutCancel(ctx), abortMultipartTimeout, errors.WithStack(context.DeadlineExceeded)) + defer cancel() + if _, abortErr := s3Client.AbortMultipartUpload(abortCtx, &abortIn); abortErr != nil { + bklog.G(ctx).Errorf("failed to abort multipart upload for %s, it may keep incurring storage costs until a bucket lifecycle rule removes it: %v", key, abortErr) } }() @@ -612,8 +633,8 @@ func (s3Client *s3Client) touch(ctx context.Context, key string, size *int64) (e var currentPosition int64 var completedParts []s3types.CompletedPart - for currentPosition < *size { - copyRange := buildCopySourceRange(currentPosition, *size) + for currentPosition < size { + copyRange := buildCopySourceRange(currentPosition, size) partInput := s3.UploadPartCopyInput{ Bucket: &s3Client.bucket, CopySource: ©Source, diff --git a/cache/remotecache/s3/touch_test.go b/cache/remotecache/s3/touch_test.go new file mode 100644 index 000000000000..0fbc44288702 --- /dev/null +++ b/cache/remotecache/s3/touch_test.go @@ -0,0 +1,59 @@ +package s3 + +import ( + "fmt" + "testing" + + "github.com/stretchr/testify/require" +) + +// TestBuildCopySourceRange checks the ranges used to refresh an object larger +// than the CopyObject limit. A copy source range is inclusive, so the last +// byte it may name is objectSize-1. +func TestBuildCopySourceRange(t *testing.T) { + tests := []struct { + name string + start int64 + objectSize int64 + want string + }{ + { + name: "first part of a larger object", + start: 0, + objectSize: 3 * maxCopyObjectSize, + want: "bytes=0-5368709119", + }, + { + name: "final short part", + start: maxCopyObjectSize, + objectSize: maxCopyObjectSize + 100, + want: "bytes=5368709120-5368709219", + }, + { + name: "remainder ends exactly on the part size", + start: 0, + objectSize: maxCopyObjectSize, + want: "bytes=0-5368709119", + }, + { + name: "remainder one byte short of the part size", + start: 0, + objectSize: maxCopyObjectSize - 1, + want: "bytes=0-5368709118", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := buildCopySourceRange(tt.start, tt.objectSize) + require.Equal(t, tt.want, got) + + // The range must never address a byte past the end of the object. + var start, end int64 + _, err := fmt.Sscanf(got, "bytes=%d-%d", &start, &end) + require.NoError(t, err) + require.Less(t, end, tt.objectSize) + require.LessOrEqual(t, start, end) + }) + } +}