repository: Move cleanup of lockfiles into housekeeping

In order to not leave behind partially written files, git uses lockfiles
for many of its persistent data. Under erroneous circumstances it may
happen that those lockfiles are left behind without any cleanup, which
may cause us to fail when running any subsequent git commands. We thus
clean up some of those lockfiles after a grace period in our Cleanup
RPC.

Now that the Cleanup RPC calls our housekeeping tasks, this commit moves
this logic into them to make it more generally available. Most
importantly, housekeeping tasks are also run for object pools and as
part of the GarbageCollect RPC, both of which may benefit from this
cleanup.

4 files changed, 123 insertions, 29 deletions

diff --git a/internal/git/housekeeping/housekeeping.go b/internal/git/housekeeping/housekeeping.go
index e6b173c85..cec0bbe90 100644
--- a/internal/git/housekeeping/housekeeping.go
+++ b/internal/git/housekeeping/housekeeping.go
@@ -16,6 +16,15 @@ const (
 	deleteTempFilesOlderThanDuration = 7 * 24 * time.Hour
 	brokenRefsGracePeriod            = 24 * time.Hour
 	minimumDirPerm                   = 0700
+	lockfileGracePeriod              = 15 * time.Minute
+)
+
+var (
+	lockfiles = []string{
+		"config.lock",
+		"HEAD.lock",
+		"objects/info/commit-graphs/commit-graph-chain.lock",
+	}
 )
 
 type staleFileFinderFn func(context.Context, string) ([]string, error)
@@ -27,6 +36,7 @@ func Perform(ctx context.Context, repoPath string) error {
 
 	for field, staleFileFinder := range map[string]staleFileFinderFn{
 		"objects": findTemporaryObjects,
+		"locks":   findStaleLockfiles,
 		"refs":    findBrokenLooseReferences,
 	} {
 		staleFiles, err := staleFileFinder(ctx, repoPath)
@@ -58,6 +68,42 @@ func Perform(ctx context.Context, repoPath string) error {
 	return nil
 }
 
+// findStaleFiles determines whether any of the given files rooted at repoPath
+// are stale or not. A file is considered stale if it exists and if it has not
+// been modified during the gracePeriod. A nonexistent file is not considered
+// to be a stale file and will not cause an error.
+func findStaleFiles(repoPath string, gracePeriod time.Duration, files ...string) ([]string, error) {
+	var staleFiles []string
+
+	for _, file := range files {
+		path := filepath.Join(repoPath, file)
+
+		fileInfo, err := os.Stat(path)
+		if err != nil {
+			if os.IsNotExist(err) {
+				continue
+			}
+			return nil, err
+		}
+
+		if time.Since(fileInfo.ModTime()) < gracePeriod {
+			continue
+		}
+
+		staleFiles = append(staleFiles, path)
+	}
+
+	return staleFiles, nil
+}
+
+// findStaleLockfiles finds a subset of lockfiles which may be created by git
+// commands. We're quite conservative with what we're removing, we certaintly
+// don't just scan the repo for `*.lock` files. Instead, we only remove a known
+// set of lockfiles which have caused problems in the past.
+func findStaleLockfiles(ctx context.Context, repoPath string) ([]string, error) {
+	return findStaleFiles(repoPath, lockfileGracePeriod, lockfiles...)
+}
+
 func findTemporaryObjects(ctx context.Context, repoPath string) ([]string, error) {
 	var temporaryObjects []string
 
diff --git a/internal/git/housekeeping/housekeeping_test.go b/internal/git/housekeeping/housekeeping_test.go
index ffdef18e5..e753323e6 100644
--- a/internal/git/housekeeping/housekeeping_test.go
+++ b/internal/git/housekeeping/housekeeping_test.go
@@ -2,6 +2,7 @@ package housekeeping
 
 import (
 	"bytes"
+	"fmt"
 	"io/ioutil"
 	"os"
 	"path/filepath"
@@ -311,6 +312,77 @@ func TestPerform_references(t *testing.T) {
 	}
 }
 
+func TestPerform_withSpecificFile(t *testing.T) {
+	for file, finder := range map[string]staleFileFinderFn{
+		"HEAD.lock":   findStaleLockfiles,
+		"config.lock": findStaleLockfiles,
+	} {
+		testPerformWithSpecificFile(t, file, finder)
+	}
+}
+
+func testPerformWithSpecificFile(t *testing.T, file string, finder staleFileFinderFn) {
+	ctx, cancel := testhelper.Context()
+	defer cancel()
+
+	_, repoPath, cleanup := testhelper.NewTestRepo(t)
+	defer cleanup()
+
+	for _, tc := range []struct {
+		desc          string
+		entries       []entry
+		expectedFiles []string
+	}{
+		{
+			desc: fmt.Sprintf("fresh %s is kept", file),
+			entries: []entry{
+				f(file, 0700, 10*time.Minute, Keep),
+			},
+		},
+		{
+			desc: fmt.Sprintf("stale %s in subdir is kept", file),
+			entries: []entry{
+				d("subdir", 0700, 240*time.Hour, Keep, []entry{
+					f(file, 0700, 24*time.Hour, Keep),
+				}),
+			},
+		},
+		{
+			desc: fmt.Sprintf("stale %s is deleted", file),
+			entries: []entry{
+				f(file, 0700, 61*time.Minute, Delete),
+			},
+			expectedFiles: []string{
+				filepath.Join(repoPath, file),
+			},
+		},
+		{
+			desc: fmt.Sprintf("variations of %s are kept", file),
+			entries: []entry{
+				f(file[:len(file)-1], 0700, 61*time.Minute, Keep),
+				f("~"+file, 0700, 61*time.Minute, Keep),
+				f(file+"~", 0700, 61*time.Minute, Keep),
+			},
+		},
+	} {
+		t.Run(tc.desc, func(t *testing.T) {
+			for _, e := range tc.entries {
+				e.create(t, repoPath)
+			}
+
+			staleFiles, err := finder(ctx, repoPath)
+			require.NoError(t, err)
+			require.ElementsMatch(t, tc.expectedFiles, staleFiles)
+
+			require.NoError(t, Perform(ctx, repoPath))
+
+			for _, e := range tc.entries {
+				e.validate(t, repoPath)
+			}
+		})
+	}
+}
+
 func TestShouldRemoveTemporaryObject(t *testing.T) {
 	type args struct {
 		path    string
diff --git a/internal/gitaly/service/repository/cleanup.go b/internal/gitaly/service/repository/cleanup.go
index 465405cb2..d1981282f 100644
--- a/internal/gitaly/service/repository/cleanup.go
+++ b/internal/gitaly/service/repository/cleanup.go
@@ -15,8 +15,6 @@ import (
 	"google.golang.org/grpc/status"
 )
 
-var lockFiles = []string{"config.lock", "HEAD.lock", "objects/info/commit-graphs/commit-graph-chain.lock"}
-
 func (s *server) Cleanup(ctx context.Context, in *gitalypb.CleanupRequest) (*gitalypb.CleanupResponse, error) {
 	if err := s.cleanupRepo(ctx, in.GetRepository()); err != nil {
 		return nil, err
@@ -50,10 +48,6 @@ func (s *server) cleanupRepo(ctx context.Context, repo *gitalypb.Repository) err
 
 	older15min := time.Now().Add(-15 * time.Minute)
 
-	if err := cleanFileLocks(repoPath, older15min); err != nil {
-		return status.Errorf(codes.Internal, "Cleanup: cleanupConfigLock: %v", err)
-	}
-
 	if err := cleanPackedRefsNew(repoPath, older15min); err != nil {
 		return status.Errorf(codes.Internal, "Cleanup: cleanPackedRefsNew: %v", err)
 	}
@@ -168,28 +162,6 @@ func (s *server) cleanDisconnectedWorktrees(ctx context.Context, repo *gitalypb.
 	return cmd.Wait()
 }
 
-func cleanFileLocks(repoPath string, threshold time.Time) error {
-	for _, fileName := range lockFiles {
-		lockPath := filepath.Join(repoPath, fileName)
-
-		fi, err := os.Stat(lockPath)
-		if err != nil {
-			if os.IsNotExist(err) {
-				continue
-			}
-			return err
-		}
-
-		if fi.ModTime().Before(threshold) {
-			if err := os.Remove(lockPath); err != nil && !os.IsNotExist(err) {
-				return err
-			}
-		}
-	}
-
-	return nil
-}
-
 func cleanPackedRefsNew(repoPath string, threshold time.Time) error {
 	path := filepath.Join(repoPath, "packed-refs.new")
 
diff --git a/internal/gitaly/service/repository/cleanup_test.go b/internal/gitaly/service/repository/cleanup_test.go
index 1a7e11487..377deed05 100644
--- a/internal/gitaly/service/repository/cleanup_test.go
+++ b/internal/gitaly/service/repository/cleanup_test.go
@@ -261,7 +261,11 @@ func TestCleanupFileLocks(t *testing.T) {
 
 	req := &gitalypb.CleanupRequest{Repository: testRepo}
 
-	for _, fileName := range lockFiles {
+	for _, fileName := range []string{
+		"config.lock",
+		"HEAD.lock",
+		"objects/info/commit-graphs/commit-graph-chain.lock",
+	} {
 		lockPath := filepath.Join(testRepoPath, fileName)
 		// No file on the lock path
 		_, err := client.Cleanup(ctx, req)