Fail cache downloads when required, retry on memory alloc fail

common/network.go

@@ -273,6 +273,7 @@ type Artifacts []Artifact
 type Cache struct {
 	Key       string        `json:"key"`
 	Untracked bool          `json:"untracked"`
+	Required  bool          `json:"required"`
 	Policy    CachePolicy   `json:"policy"`
 	Paths     ArtifactPaths `json:"paths"`
 	When      CacheWhen     `json:"when"`

helpers/archives/zip_extract.go

@@ -6,6 +6,9 @@ import (
 	"io/ioutil"
 	"os"
 	"path/filepath"
+	"runtime"
+	"strings"
+	"time"
 
 	"github.com/sirupsen/logrus"
 )
@@ -83,6 +86,12 @@ func extractZipFile(file *zip.File) (err error) {
 	return
 }
 
+// this string is consistently used across platforms
+// see https://github.com/golang/go/blob/go1.17.6/src/syscall/zerrors_linux_386.go#L1393 and other zerrors*.go
+const errCannotAllocateMemory = "cannot allocate memory"
+const memoryErrorRetries = 3
+const memoryErrorWaitTime = time.Second
+
 func ExtractZipArchive(archive *zip.Reader) error {
 	tracker := newPathErrorTracker()
 
@@ -91,8 +100,23 @@ func ExtractZipArchive(archive *zip.Reader) error {
 			printGitArchiveWarning("extract")
 		}
 
-		if err := extractZipFile(file); tracker.actionable(err) {
+		// we sometimes get memory errors unzipping, do a retry
+		retries := memoryErrorRetries
+
+		for {
+
+			err := extractZipFile(file)
+			if err == nil || !tracker.actionable(err) {
+				break
+			}
+
 			logrus.Warningf("%s: %s (suppressing repeats)", file.Name, err)
+
+			err, retries = checkMemoryAllocRetry(err, retries)
+
+			if err != nil || retries <= 0 {
+				return err
+			}
 		}
 	}
 
@@ -111,6 +135,44 @@ func ExtractZipArchive(archive *zip.Reader) error {
 	return nil
 }
 
+var disableWait = false // for testing
+
+func checkMemoryAllocRetry(err error, retriesRemaining int) (error, int) {
+
+	// When running in containers, we will occasionally get errors allocating
+	// memory under pressure. So we retry a few times, then fail the extraction
+	//
+	// This is likely due to GC pressure, as the loop that calls this is dealing with
+	// Individual files and has to expand them individually in memory.
+	//
+	if err == nil || !strings.Contains(err.Error(), errCannotAllocateMemory) {
+		return err, 0
+	}
+
+	if retriesRemaining <= 0 {
+		return err, 0
+	}
+
+	mem := runtime.MemStats{}
+	runtime.ReadMemStats(&mem)
+
+	logrus.Warningf(
+		"Can't allocate memory, waiting %s, %d retries left (total used MB=%d)",
+		memoryErrorWaitTime.String(),
+		retriesRemaining,
+		mem.Sys/1024/1024,
+	)
+
+	if !disableWait {
+		time.Sleep(memoryErrorWaitTime)
+	}
+
+	// Force a GC to ensure we've cleaned up
+	runtime.GC()
+	retriesRemaining--
+	return nil, retriesRemaining
+}
+
 func ExtractZipFile(fileName string) error {
 	archive, err := zip.OpenReader(fileName)
 	if err != nil {

helpers/archives/zip_extract_test.go

@@ -1,10 +1,13 @@
+//go:build !integration
 // +build !integration
 
 package archives
 
 import (
 	"archive/zip"
 	"bytes"
+	"errors"
+	"fmt"
 	"io"
 	"io/ioutil"
 	"os"
@@ -96,3 +99,54 @@ func TestExtractZipFileNotFound(t *testing.T) {
 	err := ExtractZipFile("non_existing_zip_file.zip")
 	assert.Error(t, err)
 }
+
+func TestMemoryAllocRetry(t *testing.T) {
+
+	memErr := errors.New(errCannotAllocateMemory)
+	otherErr := errors.New("foo")
+
+	cases := []struct {
+		err       error
+		retries   int
+		res       error
+		remaining int
+	}{
+		{
+			err:       memErr,
+			res:       nil,
+			retries:   1,
+			remaining: 0,
+		},
+		{
+			err:       memErr,
+			res:       memErr,
+			retries:   0,
+			remaining: 0,
+		},
+		{
+			err:       otherErr,
+			res:       otherErr,
+			retries:   2,
+			remaining: 0,
+		},
+		{
+			err:       nil,
+			res:       nil,
+			retries:   0,
+			remaining: 0,
+		},
+	}
+
+	disableWait = true
+
+	for i, c := range cases {
+
+		t.Run(fmt.Sprintf("Case %d", i), func(t *testing.T) {
+			err, retries := checkMemoryAllocRetry(c.err , c.retries)
+
+			assert.Equal(t, c.res, err)
+			assert.Equal(t, c.remaining, retries)
+		})
+	}
+
+}

shells/abstract.go

@@ -107,7 +107,7 @@ func (b *AbstractShell) cacheExtractor(w ShellWriter, info common.ShellScriptInf
 			continue
 		}
 
-		b.extractCacheOrFallbackCacheWrapper(w, info, cacheFile, cacheKey)
+		b.extractCacheOrFallbackCacheWrapper(w, info, cacheFile, cacheKey, cacheOptions.Required)
 	}
 
 	if skipRestoreCache {
@@ -122,12 +122,13 @@ func (b *AbstractShell) extractCacheOrFallbackCacheWrapper(
 	info common.ShellScriptInfo,
 	cacheFile string,
 	cacheKey string,
+	required bool,
 ) {
 	cacheFallbackKey := info.Build.GetAllVariables().Get("CACHE_FALLBACK_KEY")
 
-	// Execute cache-extractor command. Failure is not fatal.
+	// Execute cache-extractor command. Failure is not fatal unless required is true.
 	b.guardRunnerCommand(w, info.RunnerCommand, "Extracting cache", func() {
-		b.addExtractCacheCommand(w, info, cacheFile, cacheKey, cacheFallbackKey)
+		b.addExtractCacheCommand(w, info, cacheFile, cacheKey, cacheFallbackKey, required)
 	})
 }
 
@@ -137,6 +138,7 @@ func (b *AbstractShell) addExtractCacheCommand(
 	cacheFile string,
 	cacheKey string,
 	cacheFallbackKey string,
+	required bool,
 ) {
 	args := []string{
 		"cache-extractor",
@@ -154,7 +156,11 @@ func (b *AbstractShell) addExtractCacheCommand(
 	w.Else()
 	w.Warningf("Failed to extract cache")
 	if cacheFallbackKey != "" {
-		b.addExtractCacheCommand(w, info, cacheFile, cacheFallbackKey, "")
+		b.addExtractCacheCommand(w, info, cacheFile, cacheFallbackKey, "", required)
+	} else if required {
+		// if no cache key, then set the exit code to fail the build
+		// since most jobs will not be able to run without a cache in any case
+		w.Exit(1)
 	}
 	w.EndIf()
 }

shells/abstract_test.go

@@ -1,3 +1,4 @@
+//go:build !integration
 // +build !integration
 
 package shells
@@ -755,6 +756,14 @@ func TestAbstractShell_writeSubmoduleUpdateCmd(t *testing.T) {
 }
 
 func TestAbstractShell_extractCacheWithFallbackKey(t *testing.T) {
+	testCacheCommon(t, false)
+}
+
+func TestAbstractShell_extractCacheWithFallbackKeyRequired(t *testing.T) {
+	testCacheCommon(t, true)
+}
+
+func testCacheCommon(t *testing.T, cacheRequired bool) {
 	testCacheKey := "test-cache-key"
 	testFallbackCacheKey := "test-fallback-cache-key"
 
@@ -778,9 +787,10 @@ func TestAbstractShell_extractCacheWithFallbackKey(t *testing.T) {
 			},
 			Cache: common.Caches{
 				{
-					Key:    testCacheKey,
-					Policy: common.CachePolicyPullPush,
-					Paths:  []string{"path1", "path2"},
+					Key:      testCacheKey,
+					Policy:   common.CachePolicyPullPush,
+					Paths:    []string{"path1", "path2"},
+					Required: cacheRequired,
 				},
 			},
 			Variables: common.JobVariables{
@@ -834,6 +844,10 @@ func TestAbstractShell_extractCacheWithFallbackKey(t *testing.T) {
 	mockWriter.On("EndIf").Once()
 	mockWriter.On("Else").Once()
 	mockWriter.On("Warningf", "Missing %s. %s is disabled.", "runner-command", "Extracting cache").Once()
+
+	if cacheRequired {
+		mockWriter.On("Exit", 1).Once()
+	}
 	mockWriter.On("EndIf").Once()
 
 	err := shell.cacheExtractor(mockWriter, info)

shells/bash.go

@@ -71,6 +71,11 @@ func (b *BashWriter) CheckForErrors() {
 	b.Line("_runner_exit_code=$?; if [[ $_runner_exit_code -ne 0 ]]; then exit $_runner_exit_code; fi")
 }
 
+func (b *BashWriter) Exit(code int) {
+	b.Line(fmt.Sprintf("exit %d", code))
+	b.Line("")
+}
+
 func (b *BashWriter) Indent() {
 	b.indent++
 }

shells/cmd.go

@@ -77,6 +77,11 @@ func (b *CmdWriter) CheckForErrors() {
 	b.checkErrorLevel()
 }
 
+func (b *CmdWriter) Exit(code int) {
+	b.Line(fmt.Sprintf("exit /b %d", code))
+	b.Line("")
+}
+
 func (b *CmdWriter) Indent() {
 	b.indent++
 }

shells/powershell.go

@@ -133,6 +133,11 @@ func (p *PsWriter) CheckForErrors() {
 	p.checkErrorLevel()
 }
 
+func (p *PsWriter) Exit(code int) {
+	p.Line(fmt.Sprintf("Exit %d", code))
+	p.Line("")
+}
+
 func (p *PsWriter) Indent() {
 	p.indent++
 }

shells/shell_writer.go

@@ -33,6 +33,7 @@ type ShellWriter interface {
 	Warningf(fmt string, arguments ...interface{})
 	Errorf(fmt string, arguments ...interface{})
 	EmptyLine()
+	Exit(code int)
 
 	Finish(trace bool) string
 }