Merge branch 'cron-clean-tempdir' into 'master'

Add automatic tempdir cleaner

Closes #918

See merge request gitlab-org/gitaly!540

5 files changed, 182 insertions, 2 deletions

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 0f01befee..d68277dd3 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -1,5 +1,10 @@
 # Gitaly changelog
 
+UNRELEASED
+
+- Add automatic tempdir cleaner
+  https://gitlab.com/gitlab-org/gitaly/merge_requests/540
+
 v0.73.0
 
 - Implement CreateBundle RPC
diff --git a/cmd/gitaly/main.go b/cmd/gitaly/main.go
index 79ed5a4df..79b2c8ba6 100644
--- a/cmd/gitaly/main.go
+++ b/cmd/gitaly/main.go
@@ -17,6 +17,7 @@ import (
 	"gitlab.com/gitlab-org/gitaly/internal/linguist"
 	"gitlab.com/gitlab-org/gitaly/internal/rubyserver"
 	"gitlab.com/gitlab-org/gitaly/internal/server"
+	"gitlab.com/gitlab-org/gitaly/internal/tempdir"
 	"gitlab.com/gitlab-org/gitaly/internal/version"
 
 	"github.com/prometheus/client_golang/prometheus"
@@ -105,6 +106,8 @@ func main() {
 	config.ConfigurePrometheus()
 	config.ConfigureConcurrencyLimits()
 
+	tempdir.StartCleaning()
+
 	var listeners []net.Listener
 
 	if socketPath := config.Config.SocketPath; socketPath != "" {
diff --git a/internal/tempdir/.gitignore b/internal/tempdir/.gitignore
new file mode 100644
index 000000000..ace1063ab
--- /dev/null
+++ b/internal/tempdir/.gitignore
@@ -0,0 +1 @@
+/testdata
diff --git a/internal/tempdir/tempdir.go b/internal/tempdir/tempdir.go
index 44e00570e..3632382a2 100644
--- a/internal/tempdir/tempdir.go
+++ b/internal/tempdir/tempdir.go
@@ -1,19 +1,26 @@
 package tempdir
 
 import (
+	"fmt"
 	"io/ioutil"
 	"os"
 	"path"
+	"path/filepath"
+	"strings"
+	"time"
 
 	pb "gitlab.com/gitlab-org/gitaly-proto/go"
+	"gitlab.com/gitlab-org/gitaly/internal/config"
 	"gitlab.com/gitlab-org/gitaly/internal/helper"
+
+	log "github.com/sirupsen/logrus"
 )
 
 const (
 	// We need to be careful that this path does not clash with any
 	// directory name that could be provided by a user. The '+' character is
 	// not allowed in GitLab namespaces or repositories.
-	tmpRoot = "+gitaly/tmp"
+	tmpRootPrefix = "+gitaly/tmp"
 )
 
 // New returns the path of a new temporary directory for use with the
@@ -24,10 +31,79 @@ func New(repo *pb.Repository) (string, error) {
 		return "", err
 	}
 
-	root := path.Join(storageDir, tmpRoot)
+	root := tmpRoot(storageDir)
 	if err := os.MkdirAll(root, 0700); err != nil {
 		return "", err
 	}
 
 	return ioutil.TempDir(root, "repo")
 }
+
+func tmpRoot(storageRoot string) string {
+	return path.Join(storageRoot, tmpRootPrefix)
+}
+
+// StartCleaning starts tempdir cleanup goroutines.
+func StartCleaning() {
+	for _, st := range config.Config.Storages {
+		go func(name string, dir string) {
+			start := time.Now()
+			err := clean(tmpRoot(dir))
+
+			entry := log.WithFields(log.Fields{
+				"time_ms": int(1000 * time.Since(start).Seconds()),
+				"storage": name,
+			})
+			if err != nil {
+				entry = entry.WithError(err)
+			}
+			entry.Info("finished tempdir cleaner walk")
+
+			time.Sleep(1 * time.Hour)
+		}(st.Name, st.Path)
+	}
+}
+
+func clean(dir string) error {
+	// If we start "cleaning up" the wrong directory we may delete user data
+	// which is Really Bad.
+	if !strings.HasSuffix(dir, tmpRootPrefix) {
+		log.Print(dir)
+		panic(invalidCleanRoot("invalid tempdir clean root: panicking to prevent data loss"))
+	}
+
+	return filepath.Walk(dir, cleanFunc)
+}
+
+const (
+	maxAge = 7 * 24 * time.Hour
+)
+
+type invalidCleanRoot string
+
+func cleanFunc(path string, info os.FileInfo, errIncoming error) error {
+	if errIncoming != nil && !os.IsNotExist(errIncoming) {
+		return fmt.Errorf("incoming %q: %v", path, errIncoming)
+	}
+
+	if info == nil {
+		return nil
+	}
+
+	if perm := info.Mode().Perm(); info.IsDir() && perm&0700 < 0700 {
+		// Fix directory read permissions
+		if err := os.Chmod(path, perm|0700); err != nil {
+			return err
+		}
+	}
+
+	if time.Since(info.ModTime()) < maxAge {
+		return nil
+	}
+
+	if err := os.Remove(path); err != nil && !info.IsDir() {
+		return err
+	}
+
+	return nil
+}
diff --git a/internal/tempdir/tempdir_test.go b/internal/tempdir/tempdir_test.go
index 3762a8883..a356474dc 100644
--- a/internal/tempdir/tempdir_test.go
+++ b/internal/tempdir/tempdir_test.go
@@ -5,6 +5,7 @@ import (
 	"os"
 	"path"
 	"testing"
+	"time"
 
 	pb "gitlab.com/gitlab-org/gitaly-proto/go"
 	"gitlab.com/gitlab-org/gitaly/internal/testhelper"
@@ -29,3 +30,97 @@ func TestNewFailStorageUnknown(t *testing.T) {
 	_, err := New(&pb.Repository{StorageName: "does-not-exist", RelativePath: "foobar.git"})
 	require.Error(t, err)
 }
+
+var cleanRoot = path.Join("testdata/clean", tmpRootPrefix)
+
+func TestCleanerSafety(t *testing.T) {
+	defer func() {
+		if p := recover(); p != nil {
+			if _, ok := p.(invalidCleanRoot); !ok {
+				t.Fatalf("expected invalidCleanRoot panic, got %v", p)
+			}
+		}
+	}()
+
+	//This directory is invalid because it does not end in '+gitaly/tmp'
+	invalidDir := "testdata/does-not-exist"
+	clean(invalidDir)
+
+	t.Fatal("expected panic")
+}
+
+func TestCleanSuccess(t *testing.T) {
+	if err := chmod("a", 0700); err != nil && !os.IsNotExist(err) {
+		t.Fatal(err)
+	}
+
+	require.NoError(t, os.RemoveAll(cleanRoot), "clean up test clean root")
+
+	old := time.Unix(0, 0)
+	recent := time.Now()
+
+	makeDir(t, "a", old)
+	makeDir(t, "c", recent)
+	makeDir(t, "f", old)
+
+	makeFile(t, "a/b", old)
+	makeFile(t, "c/d", old)
+	makeFile(t, "e", recent)
+
+	// This is really evil and even breaks 'rm -rf'
+	require.NoError(t, chmod("a", 0), "apply evil permissions to 'a'")
+
+	assertEntries(t, "a", "c", "e", "f")
+
+	require.NoError(t, clean(cleanRoot), "walk first pass")
+	// 'a' won't get removed because it's mtime is bumped when 'a/b' is deleted
+	assertEntries(t, "a", "c", "e")
+
+	info, err := stat("a")
+	require.NoError(t, err)
+	require.Equal(t, os.FileMode(0700), info.Mode().Perm(), "permissions of 'a' should have been fixed")
+
+	_, err = stat("a/b")
+	require.True(t, os.IsNotExist(err), "entry 'a/b' should be gone")
+
+	require.NoError(t, clean(cleanRoot), "walk second pass")
+	assertEntries(t, "a", "c", "e")
+}
+
+func chmod(p string, mode os.FileMode) error {
+	return os.Chmod(path.Join(cleanRoot, p), mode)
+}
+
+func stat(p string) (os.FileInfo, error) {
+	return os.Stat(path.Join(cleanRoot, p))
+}
+
+func assertEntries(t *testing.T, entries ...string) {
+	foundEntries, err := ioutil.ReadDir(cleanRoot)
+	require.NoError(t, err)
+
+	require.Len(t, foundEntries, len(entries))
+
+	for i, name := range entries {
+		require.Equal(t, name, foundEntries[i].Name())
+	}
+}
+
+func makeFile(t *testing.T, filePath string, mtime time.Time) {
+	fullPath := path.Join(cleanRoot, filePath)
+	require.NoError(t, ioutil.WriteFile(fullPath, nil, 0644))
+	require.NoError(t, os.Chtimes(fullPath, mtime, mtime))
+}
+
+func makeDir(t *testing.T, dirPath string, mtime time.Time) {
+	fullPath := path.Join(cleanRoot, dirPath)
+	require.NoError(t, os.MkdirAll(fullPath, 0700))
+	require.NoError(t, os.Chtimes(fullPath, mtime, mtime))
+}
+
+func TestCleanNoTmpExists(t *testing.T) {
+	// This directory is valid because it ends in the special prefix
+	dir := path.Join("testdata", "does-not-exist", tmpRootPrefix)
+
+	require.NoError(t, clean(dir))
+}