Revert "Merge branch 'po-diskcache-walker-fix' into 'master'"po-revert-mr-1721

This reverts commit 632f765dc7b42ee95af60065951e3a4948c7e65a, reversing
changes made to 57f5c06e85935eb33b0b8a8188e83237234eda7d.

6 files changed, 41 insertions, 130 deletions

diff --git a/changelogs/unreleased/po-diskcache-walker-fix.yml b/changelogs/unreleased/po-diskcache-walker-fix.yml
deleted file mode 100644
index 0f73f882b..000000000
--- a/changelogs/unreleased/po-diskcache-walker-fix.yml
+++ /dev/null
@@ -1,5 +0,0 @@
----
-title: File walker deletes empty directories
-merge_request: 1721
-author:
-type: other
diff --git a/internal/cache/prometheus.go b/internal/cache/prometheus.go
index d25405854..16ce368a9 100644
--- a/internal/cache/prometheus.go
+++ b/internal/cache/prometheus.go
@@ -52,12 +52,6 @@ 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() {
@@ -69,7 +63,6 @@ func init() {
 	prometheus.MustRegister(errTotal)
 	prometheus.MustRegister(walkerCheckTotal)
 	prometheus.MustRegister(walkerRemovalTotal)
-	prometheus.MustRegister(walkerErrorTotal)
 }
 
 func countErr(err error) error {
@@ -90,5 +83,4 @@ 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
new file mode 100644
index 000000000..bfe037ff4
--- /dev/null
+++ b/internal/cache/walk_test.go
@@ -0,0 +1,12 @@
+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 942d3b958..94737fa9f 100644
--- a/internal/cache/walker.go
+++ b/internal/cache/walker.go
@@ -14,72 +14,46 @@ 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 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
-
-	countWalkCheck()
-	entries, err := ioutil.ReadDir(path)
-	if err != nil {
-		if os.IsNotExist(err) {
-			logWalkErr(err, path, "unable to stat directory")
-			return nil
+func cleanWalk(walkPath string) error {
+	walkErr := filepath.Walk(walkPath, func(path string, info os.FileInfo, err error) error {
+		if err != nil {
+			return err
 		}
-		return err
-	}
-
-	for _, e := range entries {
-		ePath := filepath.Join(path, e.Name())
 
-		if e.IsDir() {
-			if err := cleanWalk(ePath); err != nil {
-				return err
-			}
-			continue
+		if info.IsDir() {
+			return nil
 		}
 
 		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
-	}
+		threshold := time.Now().Add(-1 * staleAge)
+		if info.ModTime().After(threshold) {
+			return nil
+		}
 
-	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 nil
+	return walkErr
 }
 
 const cleanWalkFrequency = 10 * time.Minute
diff --git a/internal/cache/walker_internal_test.go b/internal/cache/walker_internal_test.go
deleted file mode 100644
index bd5d3e768..000000000
--- a/internal/cache/walker_internal_test.go
+++ /dev/null
@@ -1,65 +0,0 @@
-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 d18bcf5c3..6433b3535 100644
--- a/internal/cache/walker_test.go
+++ b/internal/cache/walker_test.go
@@ -49,7 +49,8 @@ func TestDiskCacheObjectWalker(t *testing.T) {
 		}
 	}
 
-	expectRemovals := cache.ExportMockRemovalCounter.Count() + 4
+	expectChecks := cache.ExportMockCheckCounter.Count() + 4
+	expectRemovals := cache.ExportMockRemovalCounter.Count() + 2
 
 	// disable the initial move-and-clear function since we are only
 	// evaluating the walker
@@ -58,7 +59,7 @@ func TestDiskCacheObjectWalker(t *testing.T) {
 
 	require.NoError(t, config.Validate()) // triggers walker
 
-	pollCountersUntil(t, expectRemovals)
+	pollCountersUntil(t, expectChecks, expectRemovals)
 
 	for _, p := range shouldExist {
 		assert.FileExists(t, p)
@@ -147,20 +148,22 @@ func satisfyConfigValidation(tmpPath string) error {
 	return nil
 }
 
-func pollCountersUntil(t testing.TB, expectRemovals int) {
+func pollCountersUntil(t testing.TB, expectChecks, 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; removals: %d",
+				"timed out polling prometheus stats; checks: %d removals: %d",
+				cache.ExportMockCheckCounter.Count(),
 				cache.ExportMockRemovalCounter.Count(),
 			)
 		default:
 			// keep on truckin'
 		}
-		if cache.ExportMockRemovalCounter.Count() == expectRemovals {
+		if cache.ExportMockCheckCounter.Count() == expectChecks &&
+			cache.ExportMockRemovalCounter.Count() == expectRemovals {
 			break
 		}
 		time.Sleep(time.Millisecond)