Delete old lock files before performing Garbage Collection

3 files changed, 193 insertions, 8 deletions

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 592b6ed03..98a9aa28e 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -2,9 +2,11 @@
 
 UNRELEASED
 
+- Delete old lock files before performing Garbage Collection
+  https://gitlab.com/gitlab-org/gitaly/merge_requests/587
 - Implement RepositoryService.IsSquashInProgress RPC
   https://gitlab.com/gitlab-org/gitaly/merge_requests/593
-- Added test to prevent wiki page duplication 
+- Added test to prevent wiki page duplication
   https://gitlab.com/gitlab-org/gitaly/merge_requests/539
 - Fixed bug in wiki_find_page method
   https://gitlab.com/gitlab-org/gitaly/merge_requests/590
diff --git a/internal/service/repository/gc.go b/internal/service/repository/gc.go
index 9f6724eae..a8f7961d1 100644
--- a/internal/service/repository/gc.go
+++ b/internal/service/repository/gc.go
@@ -1,6 +1,11 @@
 package repository
 
 import (
+	"os"
+	"path/filepath"
+	"strings"
+	"time"
+
 	"github.com/grpc-ecosystem/go-grpc-middleware/logging/logrus"
 	log "github.com/sirupsen/logrus"
 	"gitlab.com/gitlab-org/gitaly/internal/helper/housekeeping"
@@ -19,6 +24,19 @@ func (server) GarbageCollect(ctx context.Context, in *pb.GarbageCollectRequest)
 		"WriteBitmaps": in.GetCreateBitmap(),
 	}).Debug("GarbageCollect")
 
+	repoPath, err := helper.GetRepoPath(in.GetRepository())
+	if err != nil {
+		return nil, err
+	}
+
+	threshold := time.Now().Add(-1 * time.Hour)
+	if err := cleanRefsLocks(filepath.Join(repoPath, "refs"), threshold); err != nil {
+		return nil, status.Errorf(codes.Internal, "GarbageCollect: cleanRefsLocks: %v", err)
+	}
+	if err := cleanPackedRefsLock(repoPath, threshold); err != nil {
+		return nil, status.Errorf(codes.Internal, "GarbageCollect: cleanPackedRefsLock: %v", err)
+	}
+
 	args := []string{"-c"}
 	if in.GetCreateBitmap() {
 		args = append(args, "repack.writeBitmaps=true")
@@ -31,19 +49,14 @@ func (server) GarbageCollect(ctx context.Context, in *pb.GarbageCollectRequest)
 		if _, ok := status.FromError(err); ok {
 			return nil, err
 		}
-		return nil, status.Errorf(codes.Internal, err.Error())
+		return nil, status.Errorf(codes.Internal, "GarbageCollect: gitCommand: %v", err)
 	}
 
 	if err := cmd.Wait(); err != nil {
-		return nil, status.Errorf(codes.Internal, err.Error())
+		return nil, status.Errorf(codes.Internal, "GarbageCollect: cmd wait: %v", err)
 	}
 
 	// Perform housekeeping post GC
-	repoPath, err := helper.GetRepoPath(in.GetRepository())
-	if err != nil {
-		return nil, err
-	}
-
 	err = housekeeping.Perform(ctx, repoPath)
 	if err != nil {
 		ctxlogger.WithError(err).Warn("Post gc housekeeping failed")
@@ -51,3 +64,42 @@ func (server) GarbageCollect(ctx context.Context, in *pb.GarbageCollectRequest)
 
 	return &pb.GarbageCollectResponse{}, nil
 }
+
+func cleanRefsLocks(rootPath string, threshold time.Time) error {
+	return filepath.Walk(rootPath, func(path string, info os.FileInfo, err error) error {
+		if err != nil {
+			return err
+		}
+
+		if info.IsDir() {
+			return nil
+		}
+
+		if strings.HasSuffix(info.Name(), ".lock") && info.ModTime().Before(threshold) {
+			if err := os.Remove(path); err != nil && !os.IsNotExist(err) {
+				return err
+			}
+		}
+
+		return nil
+	})
+}
+
+func cleanPackedRefsLock(repoPath string, threshold time.Time) error {
+	path := filepath.Join(repoPath, "packed-refs.lock")
+	fileInfo, err := os.Stat(path)
+	if err != nil {
+		if os.IsNotExist(err) {
+			return nil
+		}
+		return err
+	}
+
+	if fileInfo.ModTime().Before(threshold) {
+		if err := os.Remove(path); err != nil && !os.IsNotExist(err) {
+			return err
+		}
+	}
+
+	return nil
+}
diff --git a/internal/service/repository/gc_test.go b/internal/service/repository/gc_test.go
index 8a8b77b87..5dcdfa9de 100644
--- a/internal/service/repository/gc_test.go
+++ b/internal/service/repository/gc_test.go
@@ -3,9 +3,12 @@ package repository
 import (
 	"context"
 	"fmt"
+	"io/ioutil"
+	"os"
 	"path"
 	"path/filepath"
 	"testing"
+	"time"
 
 	"google.golang.org/grpc/codes"
 
@@ -15,6 +18,11 @@ import (
 	pb "gitlab.com/gitlab-org/gitaly-proto/go"
 )
 
+var (
+	freshTime = time.Now()
+	oldTime   = freshTime.Add(-2 * time.Hour)
+)
+
 func TestGarbageCollectSuccess(t *testing.T) {
 	server, serverSocketPath := runRepoServer(t)
 	defer server.Stop()
@@ -73,6 +81,124 @@ func TestGarbageCollectSuccess(t *testing.T) {
 	}
 }
 
+func TestGarbageCollectDeletesRefsLocks(t *testing.T) {
+	server, serverSocketPath := runRepoServer(t)
+	defer server.Stop()
+
+	client, conn := newRepositoryClient(t, serverSocketPath)
+	defer conn.Close()
+
+	testRepo, testRepoPath, cleanupFn := testhelper.NewTestRepo(t)
+	defer cleanupFn()
+
+	ctx, cancel := testhelper.Context()
+	defer cancel()
+
+	req := &pb.GarbageCollectRequest{Repository: testRepo}
+	refsPath := filepath.Join(testRepoPath, "refs")
+
+	// Note: Creating refs this way makes `git gc` crash but this actually works
+	// in our favor for this test since we can ensure that the files kept and
+	// deleted are all due to our *.lock cleanup step before gc runs (since
+	// `git gc` also deletes files from /refs when packing).
+	keepRefPath := filepath.Join(refsPath, "heads", "keepthis")
+	createFileWithTimes(keepRefPath, freshTime)
+	keepOldRefPath := filepath.Join(refsPath, "heads", "keepthisalso")
+	createFileWithTimes(keepOldRefPath, oldTime)
+	keepDeceitfulRef := filepath.Join(refsPath, "heads", " .lock.not-actually-a-lock.lock ")
+	createFileWithTimes(keepDeceitfulRef, oldTime)
+
+	keepLockPath := filepath.Join(refsPath, "heads", "keepthis.lock")
+	createFileWithTimes(keepLockPath, freshTime)
+
+	deleteLockPath := filepath.Join(refsPath, "heads", "deletethis.lock")
+	createFileWithTimes(deleteLockPath, oldTime)
+
+	c, err := client.GarbageCollect(ctx, req)
+	testhelper.AssertGrpcError(t, err, codes.Internal, "GarbageCollect: cmd wait")
+	assert.Nil(t, c)
+
+	// Sanity checks
+	assert.FileExists(t, keepRefPath)
+	assert.FileExists(t, keepOldRefPath)
+	assert.FileExists(t, keepDeceitfulRef)
+
+	assert.FileExists(t, keepLockPath)
+
+	// There's assert.FileExists but no assert.NotFileExists ¯\_(ツ)_/¯
+	_, err = os.Stat(deleteLockPath)
+	assert.True(t, os.IsNotExist(err))
+}
+
+func TestGarbageCollectDeletesPackedRefsLock(t *testing.T) {
+	server, serverSocketPath := runRepoServer(t)
+	defer server.Stop()
+
+	client, conn := newRepositoryClient(t, serverSocketPath)
+	defer conn.Close()
+
+	testCases := []struct {
+		desc        string
+		lockTime    *time.Time
+		shouldExist bool
+	}{
+		{
+			desc:        "with a recent lock",
+			lockTime:    &freshTime,
+			shouldExist: true,
+		},
+		{
+			desc:        "with an old lock",
+			lockTime:    &oldTime,
+			shouldExist: false,
+		},
+		{
+			desc:        "with a non-existing lock",
+			lockTime:    nil,
+			shouldExist: false,
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.desc, func(t *testing.T) {
+			testRepo, testRepoPath, cleanupFn := testhelper.NewTestRepo(t)
+			defer cleanupFn()
+
+			// Force the packed-refs file to have an old time to test that even
+			// in that case it doesn't get deleted
+			packedRefsPath := filepath.Join(testRepoPath, "packed-refs")
+			os.Chtimes(packedRefsPath, oldTime, oldTime)
+
+			req := &pb.GarbageCollectRequest{Repository: testRepo}
+			lockPath := filepath.Join(testRepoPath, "packed-refs.lock")
+
+			if tc.lockTime != nil {
+				createFileWithTimes(lockPath, *tc.lockTime)
+			}
+
+			ctx, cancel := testhelper.Context()
+			defer cancel()
+
+			c, err := client.GarbageCollect(ctx, req)
+
+			// Sanity checks
+			assert.FileExists(t, filepath.Join(testRepoPath, "HEAD")) // For good measure
+			assert.FileExists(t, packedRefsPath)
+
+			if tc.shouldExist {
+				assert.FileExists(t, lockPath)
+			} else {
+				assert.NoError(t, err)
+				assert.NotNil(t, c)
+
+				// There's assert.FileExists but no assert.NotFileExists ¯\_(ツ)_/¯
+				_, err = os.Stat(lockPath)
+				assert.True(t, os.IsNotExist(err))
+			}
+		})
+	}
+}
+
 func TestGarbageCollectFailure(t *testing.T) {
 	server, serverSocketPath := runRepoServer(t)
 	defer server.Stop()
@@ -103,3 +229,8 @@ func TestGarbageCollectFailure(t *testing.T) {
 	}
 
 }
+
+func createFileWithTimes(path string, mTime time.Time) {
+	ioutil.WriteFile(path, nil, 0644)
+	os.Chtimes(path, mTime, mTime)
+}