Merge branch 'bvl-cleanup-invalid-keeparounds' into 'master'

Clean up invalid keep-around refs when performing housekeeping

See merge request gitlab-org/gitaly!992

3 files changed, 185 insertions, 0 deletions

diff --git a/changelogs/unreleased/bvl-cleanup-invalid-keeparounds.yml b/changelogs/unreleased/bvl-cleanup-invalid-keeparounds.yml
new file mode 100644
index 000000000..d484b536a
--- /dev/null
+++ b/changelogs/unreleased/bvl-cleanup-invalid-keeparounds.yml
@@ -0,0 +1,5 @@
+---
+title: Clean up invalid keep-around refs when performing housekeeping
+merge_request: 992
+author:
+type: other
diff --git a/internal/service/repository/gc.go b/internal/service/repository/gc.go
index d1da2bc48..cd06bce0e 100644
--- a/internal/service/repository/gc.go
+++ b/internal/service/repository/gc.go
@@ -1,10 +1,17 @@
 package repository
 
 import (
+	"errors"
+	"fmt"
+	"io/ioutil"
+	"os"
+	"path/filepath"
+
 	grpc_logrus "github.com/grpc-ecosystem/go-grpc-middleware/logging/logrus"
 	log "github.com/sirupsen/logrus"
 	"gitlab.com/gitlab-org/gitaly-proto/go/gitalypb"
 	"gitlab.com/gitlab-org/gitaly/internal/git"
+	"gitlab.com/gitlab-org/gitaly/internal/git/catfile"
 	"gitlab.com/gitlab-org/gitaly/internal/helper"
 	"gitlab.com/gitlab-org/gitaly/internal/helper/housekeeping"
 	"golang.org/x/net/context"
@@ -27,6 +34,10 @@ func (server) GarbageCollect(ctx context.Context, in *gitalypb.GarbageCollectReq
 		return nil, err
 	}
 
+	if err := cleanupKeepArounds(ctx, in.GetRepository()); err != nil {
+		return nil, err
+	}
+
 	args := []string{"-c"}
 	if in.GetCreateBitmap() {
 		args = append(args, "repack.writeBitmaps=true")
@@ -54,3 +65,79 @@ func (server) GarbageCollect(ctx context.Context, in *gitalypb.GarbageCollectReq
 
 	return &gitalypb.GarbageCollectResponse{}, nil
 }
+
+func cleanupKeepArounds(ctx context.Context, repo *gitalypb.Repository) error {
+	repoPath, err := helper.GetRepoPath(repo)
+	if err != nil {
+		return nil
+	}
+
+	batch, err := catfile.New(ctx, repo)
+	if err != nil {
+		return nil
+	}
+
+	keepAroundsPrefix := "refs/keep-around"
+	keepAroundsPath := filepath.Join(repoPath, keepAroundsPrefix)
+
+	refInfos, err := ioutil.ReadDir(keepAroundsPath)
+	if os.IsNotExist(err) {
+		return nil
+	}
+	if err != nil {
+		return err
+	}
+
+	for _, info := range refInfos {
+		if info.IsDir() {
+			continue
+		}
+
+		refName := fmt.Sprintf("%s/%s", keepAroundsPrefix, info.Name())
+		path := filepath.Join(repoPath, keepAroundsPrefix, info.Name())
+
+		if err = checkRef(batch, refName, info); err == nil {
+			continue
+		}
+
+		if err := fixRef(ctx, repo, batch, path, refName, info.Name()); err != nil {
+			return err
+		}
+	}
+
+	return nil
+}
+
+func checkRef(batch *catfile.Batch, refName string, info os.FileInfo) error {
+	if info.Size() == 0 {
+		return errors.New("checkRef: Ref file is empty")
+	}
+
+	_, err := batch.Info(refName)
+	return err
+}
+
+func fixRef(ctx context.Context, repo *gitalypb.Repository, batch *catfile.Batch, refPath string, name string, sha string) error {
+	// So the ref is broken, let's get rid of it
+	if err := os.RemoveAll(refPath); err != nil {
+		return err
+	}
+
+	// If the sha is not in the the repository, we can't fix it
+	if _, err := batch.Info(sha); err != nil {
+		return nil
+	}
+
+	// The name is a valid sha, recreate the ref
+	updateRefArgs := []string{"update-ref", name, sha}
+	cmd, err := git.Command(ctx, repo, updateRefArgs...)
+	if err != nil {
+		return err
+	}
+
+	if err = cmd.Wait(); err != nil {
+		return err
+	}
+
+	return nil
+}
diff --git a/internal/service/repository/gc_test.go b/internal/service/repository/gc_test.go
index d718c3229..651a5dfea 100644
--- a/internal/service/repository/gc_test.go
+++ b/internal/service/repository/gc_test.go
@@ -7,6 +7,7 @@ import (
 	"os"
 	"path"
 	"path/filepath"
+	"strings"
 	"testing"
 	"time"
 
@@ -160,6 +161,98 @@ func TestGarbageCollectFailure(t *testing.T) {
 
 }
 
+func TestCleanupInvalidKeepAroundRefs(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()
+
+	// Make the directory, so we can create random reflike things in it
+	require.NoError(t, os.MkdirAll(filepath.Join(testRepoPath, "refs", "keep-around"), 0755))
+
+	testCases := []struct {
+		desc        string
+		refName     string
+		refContent  string
+		shouldExist bool
+	}{
+		{
+			desc:        "A valid ref",
+			refName:     "0b4bc9a49b562e85de7cc9e834518ea6828729b9",
+			refContent:  "0b4bc9a49b562e85de7cc9e834518ea6828729b9",
+			shouldExist: true,
+		},
+		{
+			desc:        "A ref that does not exist",
+			refName:     "bogus",
+			refContent:  "bogus",
+			shouldExist: false,
+		}, {
+			desc:        "Filled with the blank ref",
+			refName:     "0b4bc9a49b562e85de7cc9e834518ea6828729b9",
+			refContent:  "0000000000000000000000000000000000000000",
+			shouldExist: true,
+		}, {
+			desc:        "An existing ref with blank content",
+			refName:     "0b4bc9a49b562e85de7cc9e834518ea6828729b9",
+			refContent:  "",
+			shouldExist: true,
+		}, {
+			desc:        "A valid sha that does not exist in the repo",
+			refName:     "d669a6f1a70693058cf484318c1cee8526119938",
+			refContent:  "d669a6f1a70693058cf484318c1cee8526119938",
+			shouldExist: false,
+		},
+	}
+
+	for _, testcase := range testCases {
+		t.Run(testcase.desc, func(t *testing.T) {
+			ctx, cancel := testhelper.Context()
+			defer cancel()
+
+			// Create a proper keep-around loose ref
+			existingSha := "1e292f8fedd741b75372e19097c76d327140c312"
+			existingRefName := fmt.Sprintf("refs/keep-around/%s", existingSha)
+			testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "update-ref", existingRefName, existingSha)
+
+			// Create an invalid ref that should should be removed with the testcase
+			bogusSha := "b3f5e4adf6277b571b7943a4f0405a6dd7ee7e15"
+			bogusPath := filepath.Join(testRepoPath, fmt.Sprintf("refs/keep-around/%s", bogusSha))
+			require.NoError(t, ioutil.WriteFile(bogusPath, []byte(bogusSha), 0644))
+
+			// Creating the keeparound without using git so we can create invalid ones in testcases
+			refPath := filepath.Join(testRepoPath, fmt.Sprintf("refs/keep-around/%s", testcase.refName))
+			require.NoError(t, ioutil.WriteFile(refPath, []byte(testcase.refContent), 0644))
+
+			// Perform the request
+			req := &gitalypb.GarbageCollectRequest{Repository: testRepo}
+			_, err := client.GarbageCollect(ctx, req)
+			require.NoError(t, err)
+
+			// The existing keeparound still exists
+			commitSha := testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "rev-parse", existingRefName)
+			require.Equal(t, existingSha, strings.TrimSpace(string(commitSha)))
+
+			//The invalid one was removed
+			_, err = os.Stat(bogusPath)
+			require.True(t, os.IsNotExist(err), "expected 'does not exist' error, got %v", err)
+
+			if testcase.shouldExist {
+				keepAroundName := fmt.Sprintf("refs/keep-around/%s", testcase.refName)
+				commitSha := testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "rev-parse", keepAroundName)
+				require.Equal(t, testcase.refName, strings.TrimSpace(string(commitSha)))
+			} else {
+				_, err := os.Stat(refPath)
+				require.True(t, os.IsNotExist(err), "expected 'does not exist' error, got %v", err)
+			}
+		})
+	}
+}
+
 func createFileWithTimes(path string, mTime time.Time) {
 	ioutil.WriteFile(path, nil, 0644)
 	os.Chtimes(path, mTime, mTime)