Skip to content
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.

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

Do not return an error uploading to remote cache #212

Merged
merged 4 commits into from
Mar 17, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
12 changes: 7 additions & 5 deletions pkg/leeway/build.go
Original file line number Diff line number Diff line change
Expand Up @@ -533,6 +533,13 @@ func Build(pkg *Package, opts ...BuildOption) (err error) {

buildErr := pkg.build(ctx)

// Check for build errors immediately and return if there are any
if buildErr != nil {
// We deliberately swallow the target package build error as that will have already been reported using the reporter.
return xerrors.Errorf("build failed")
}

// Only proceed with cache upload if build succeeded
pkgsToUpload := ctx.GetNewPackagesForCache()
// Convert []*Package to []cache.Package
pkgsToUploadCache := make([]cache.Package, len(pkgsToUpload))
Expand All @@ -541,11 +548,6 @@ func Build(pkg *Package, opts ...BuildOption) (err error) {
}

cacheErr := ctx.RemoteCache.Upload(context.Background(), ctx.LocalCache, pkgsToUploadCache)

if buildErr != nil {
// We deliberately swallow the target pacakge build error as that will have already been reported using the reporter.
return xerrors.Errorf("build failed")
}
if cacheErr != nil {
return cacheErr
}
Expand Down
49 changes: 43 additions & 6 deletions pkg/leeway/cache/remote/s3.go
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,9 @@ func (s *S3Cache) processPackages(ctx context.Context, pkgs []cache.Package, fn
}

if len(errs) > 0 {
// For upload operations, we want to log errors but not fail the entire build
// This is determined by the caller (Upload vs Download vs ExistingPackages)
log.WithField("errorCount", len(errs)).Debug("Some packages had errors during processing")
return fmt.Errorf("multiple errors occurred: %v", errs)
}

Expand Down Expand Up @@ -326,19 +329,42 @@ func (s *S3Cache) Download(ctx context.Context, dst cache.LocalCache, pkgs []cac

// Upload implements RemoteCache
func (s *S3Cache) Upload(ctx context.Context, src cache.LocalCache, pkgs []cache.Package) error {
return s.processPackages(ctx, pkgs, func(ctx context.Context, p cache.Package) error {
var uploadErrors []error

err := s.processPackages(ctx, pkgs, func(ctx context.Context, p cache.Package) error {
localPath, exists := src.Location(p)
if !exists {
return fmt.Errorf("package %s not found in local cache", p.FullName())
log.WithField("package", p.FullName()).Warn("package not found in local cache - skipping upload")
return nil // Skip but don't fail everything
}

key := filepath.Base(localPath)
if err := s.storage.UploadObject(ctx, key, localPath); err != nil {
return fmt.Errorf("failed to upload package %s: %w", p.FullName(), err)
log.WithError(err).WithFields(log.Fields{
"package": p.FullName(),
"key": key,
}).Warn("failed to upload package to remote cache - continuing")
uploadErrors = append(uploadErrors, fmt.Errorf("package %s: %w", p.FullName(), err))
return nil // Don't fail the entire operation
}

log.WithFields(log.Fields{
"package": p.FullName(),
"key": key,
}).Debug("successfully uploaded package to remote cache")
return nil
})

if err != nil {
log.WithError(err).Warn("errors occurred during upload to remote cache - continuing")
// Don't return the error to allow the build to continue
}

if len(uploadErrors) > 0 {
log.WithField("errorCount", len(uploadErrors)).Warn("some packages failed to upload to remote cache - continuing with build")
}

return nil // Always return nil to allow the build to continue
}

// s3ClientAPI is a subset of the S3 client interface we need
Expand Down Expand Up @@ -473,6 +499,7 @@ func (s *S3Storage) GetObject(ctx context.Context, key string, dest string) (int
func (s *S3Storage) UploadObject(ctx context.Context, key string, src string) error {
file, err := os.Open(src)
if err != nil {
log.WithError(err).WithField("key", key).Warn("failed to open source file for upload")
return fmt.Errorf("failed to open source file: %w", err)
}
defer file.Close()
Expand All @@ -490,10 +517,20 @@ func (s *S3Storage) UploadObject(ctx context.Context, key string, src string) er
_, err = uploader.Upload(ctx, input)
if err != nil {
var apiErr smithy.APIError
if errors.As(err, &apiErr) && apiErr.ErrorCode() == "Forbidden" {
log.WithError(err).Warnf("permission denied while uploading object %s to S3 - continuing", key)
return nil
if errors.As(err, &apiErr) {
if apiErr.ErrorCode() == "Forbidden" {
log.WithError(err).Warnf("permission denied while uploading object %s to S3 - continuing", key)
return nil
}
// Handle other API errors as warnings too
log.WithError(err).WithFields(log.Fields{
"key": key,
"errorCode": apiErr.ErrorCode(),
}).Warn("S3 API error while uploading object - continuing")
return fmt.Errorf("S3 API error: %w", err)
}
// Handle non-API errors
log.WithError(err).WithField("key", key).Warn("failed to upload object - continuing")
return fmt.Errorf("failed to upload object: %w", err)
}

Expand Down
10 changes: 0 additions & 10 deletions pkg/leeway/cache/remote/s3_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -368,16 +368,6 @@ func TestS3Cache_Upload(t *testing.T) {
},
},
},
{
name: "package not in local cache",
packages: []cache.Package{
&mockPackage{version: "v1"},
},
localCache: &mockLocalCache{
locations: map[string]string{},
},
expectError: true,
},
{
name: "403 forbidden error should not fail",
packages: []cache.Package{
Expand Down
2 changes: 1 addition & 1 deletion pkg/leeway/reporter.go
Original file line number Diff line number Diff line change
Expand Up @@ -178,7 +178,7 @@ func (r *ConsoleReporter) BuildFinished(pkg *Package, err error) {
return
}

color.Println("\n<green>build succeded</>")
color.Println("\n<green>build succeeded</>")
}

// PackageBuildStarted is called when a package build actually gets underway.
Expand Down