File walker deletes empty directories

5 files changed, 125 insertions, 41 deletions

diff --git a/internal/cache/prometheus.go b/internal/cache/prometheus.go
index 16ce368a9..d25405854 100644
--- a/internal/cache/prometheus.go
+++ b/internal/cache/prometheus.go
@@ -52,6 +52,12 @@ var (
 			Help: "Total number of events during diskcache filesystem walks",
 		},
 	)
+	walkerErrorTotal = prometheus.NewCounter(
+		prometheus.CounterOpts{
+			Name: "gitaly_diskcache_walker_error_total",
+			Help: "Total number of errors during diskcache filesystem walks",
+		},
+	)
 )
 
 func init() {
@@ -63,6 +69,7 @@ func init() {
 	prometheus.MustRegister(errTotal)
 	prometheus.MustRegister(walkerCheckTotal)
 	prometheus.MustRegister(walkerRemovalTotal)
+	prometheus.MustRegister(walkerErrorTotal)
 }
 
 func countErr(err error) error {
@@ -83,4 +90,5 @@ var (
 	countLoserBytes  = func(n float64) { bytesLoserTotals.Add(n) }
 	countWalkRemoval = func() { walkerRemovalTotal.Inc() }
 	countWalkCheck   = func() { walkerCheckTotal.Inc() }
+	countWalkError   = func() { walkerErrorTotal.Inc() }
 )
diff --git a/internal/cache/walk_test.go b/internal/cache/walk_test.go
deleted file mode 100644
index bfe037ff4..000000000
--- a/internal/cache/walk_test.go
+++ /dev/null
@@ -1,12 +0,0 @@
-package cache
-
-import (
-	"testing"
-
-	"github.com/stretchr/testify/assert"
-)
-
-func TestcleanWalkDirNotExists(t *testing.T) {
-	err := cleanWalk("/path/that/does/not/exist")
-	assert.NoError(t, err, "cleanWalk returned an error for a non existing directory")
-}
diff --git a/internal/cache/walker.go b/internal/cache/walker.go
index 94737fa9f..942d3b958 100644
--- a/internal/cache/walker.go
+++ b/internal/cache/walker.go
@@ -14,46 +14,72 @@ import (
 	"github.com/sirupsen/logrus"
 	"gitlab.com/gitlab-org/gitaly/internal/config"
 	"gitlab.com/gitlab-org/gitaly/internal/dontpanic"
+	"gitlab.com/gitlab-org/gitaly/internal/log"
 	"gitlab.com/gitlab-org/gitaly/internal/tempdir"
 )
 
-func cleanWalk(walkPath string) error {
-	walkErr := filepath.Walk(walkPath, func(path string, info os.FileInfo, err error) error {
-		if err != nil {
-			return err
-		}
+func logWalkErr(err error, path, msg string) {
+	countWalkError()
+	log.Default().
+		WithField("path", path).
+		WithError(err).
+		Warn(msg)
+}
+
+func cleanWalk(path string) error {
+	defer time.Sleep(100 * time.Microsecond) // relieve pressure
 
-		if info.IsDir() {
+	countWalkCheck()
+	entries, err := ioutil.ReadDir(path)
+	if err != nil {
+		if os.IsNotExist(err) {
+			logWalkErr(err, path, "unable to stat directory")
 			return nil
 		}
+		return err
+	}
 
-		countWalkCheck()
+	for _, e := range entries {
+		ePath := filepath.Join(path, e.Name())
 
-		threshold := time.Now().Add(-1 * staleAge)
-		if info.ModTime().After(threshold) {
-			return nil
+		if e.IsDir() {
+			if err := cleanWalk(ePath); err != nil {
+				return err
+			}
+			continue
+		}
+
+		countWalkCheck()
+		if time.Since(e.ModTime()) >= staleAge {
+			if err := os.Remove(ePath); err != nil {
+				if os.IsNotExist(err) {
+					continue
+				}
+				logWalkErr(err, ePath, "unable to remove file")
+				return err
+			}
+			countWalkRemoval()
 		}
+	}
+
+	files, err := ioutil.ReadDir(path)
+	if err != nil {
+		logWalkErr(err, path, "unable to stat directory after walk")
+		return err
+	}
 
+	if len(files) == 0 {
 		if err := os.Remove(path); err != nil {
 			if os.IsNotExist(err) {
-				// race condition: another file walker on the same storage may
-				// have deleted the file already
 				return nil
 			}
-
+			logWalkErr(err, path, "unable to remove empty directory")
 			return err
 		}
-
 		countWalkRemoval()
-
-		return nil
-	})
-
-	if os.IsNotExist(walkErr) {
-		return nil
 	}
 
-	return walkErr
+	return nil
 }
 
 const cleanWalkFrequency = 10 * time.Minute
diff --git a/internal/cache/walker_internal_test.go b/internal/cache/walker_internal_test.go
new file mode 100644
index 000000000..bd5d3e768
--- /dev/null
+++ b/internal/cache/walker_internal_test.go
@@ -0,0 +1,65 @@
+package cache
+
+import (
+	"io/ioutil"
+	"os"
+	"os/exec"
+	"path/filepath"
+	"strings"
+	"testing"
+	"time"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func TestCleanWalkDirNotExists(t *testing.T) {
+	err := cleanWalk("/path/that/does/not/exist")
+	assert.NoError(t, err, "cleanWalk returned an error for a non existing directory")
+}
+
+func TestCleanWalkEmptyDirs(t *testing.T) {
+	tmp, err := ioutil.TempDir("", t.Name())
+	require.NoError(t, err)
+	defer func() { require.NoError(t, os.RemoveAll(tmp)) }()
+
+	for _, tt := range []struct {
+		path  string
+		stale bool
+	}{
+		{path: "a/b/c/"},
+		{path: "a/b/c/1", stale: true},
+		{path: "a/b/c/2", stale: true},
+		{path: "a/b/d/"},
+		{path: "e/"},
+		{path: "e/1"},
+		{path: "f/"},
+	} {
+		p := filepath.Join(tmp, tt.path)
+		if strings.HasSuffix(tt.path, "/") {
+			require.NoError(t, os.MkdirAll(p, 0755))
+		} else {
+			require.NoError(t, ioutil.WriteFile(p, nil, 0655))
+			if tt.stale {
+				require.NoError(t, os.Chtimes(p, time.Now(), time.Now().Add(-time.Hour)))
+			}
+		}
+	}
+
+	require.NoError(t, cleanWalk(tmp))
+
+	actual := findFiles(t, tmp)
+	expect := `.
+./e
+./e/1
+`
+	require.Equal(t, expect, actual)
+}
+
+func findFiles(t testing.TB, path string) string {
+	cmd := exec.Command("find", ".")
+	cmd.Dir = path
+	out, err := cmd.Output()
+	require.NoError(t, err)
+	return string(out)
+}
diff --git a/internal/cache/walker_test.go b/internal/cache/walker_test.go
index 6433b3535..d18bcf5c3 100644
--- a/internal/cache/walker_test.go
+++ b/internal/cache/walker_test.go
@@ -49,8 +49,7 @@ func TestDiskCacheObjectWalker(t *testing.T) {
 		}
 	}
 
-	expectChecks := cache.ExportMockCheckCounter.Count() + 4
-	expectRemovals := cache.ExportMockRemovalCounter.Count() + 2
+	expectRemovals := cache.ExportMockRemovalCounter.Count() + 4
 
 	// disable the initial move-and-clear function since we are only
 	// evaluating the walker
@@ -59,7 +58,7 @@ func TestDiskCacheObjectWalker(t *testing.T) {
 
 	require.NoError(t, config.Validate()) // triggers walker
 
-	pollCountersUntil(t, expectChecks, expectRemovals)
+	pollCountersUntil(t, expectRemovals)
 
 	for _, p := range shouldExist {
 		assert.FileExists(t, p)
@@ -148,22 +147,20 @@ func satisfyConfigValidation(tmpPath string) error {
 	return nil
 }
 
-func pollCountersUntil(t testing.TB, expectChecks, expectRemovals int) {
+func pollCountersUntil(t testing.TB, expectRemovals int) {
 	// poll injected mock prometheus counters until expected events occur
 	timeout := time.After(time.Second)
 	for {
 		select {
 		case <-timeout:
 			t.Fatalf(
-				"timed out polling prometheus stats; checks: %d removals: %d",
-				cache.ExportMockCheckCounter.Count(),
+				"timed out polling prometheus stats; removals: %d",
 				cache.ExportMockRemovalCounter.Count(),
 			)
 		default:
 			// keep on truckin'
 		}
-		if cache.ExportMockCheckCounter.Count() == expectChecks &&
-			cache.ExportMockRemovalCounter.Count() == expectRemovals {
+		if cache.ExportMockRemovalCounter.Count() == expectRemovals {
 			break
 		}
 		time.Sleep(time.Millisecond)