Clean up invalid keep-around refsbvl-cleanup-invalid-keeparounds

We used to corrupt `keep-around` refs when creating them using
rugged. This ensures we clean up any refs git won't understand
repairing them if possible.

When the ref contents is blank, invalid or a "blank"-sha, we remove
the ref and recreate it based on it's filename if the filename was a
valid SHA.

This is run as part of the `GarbageCollect` RPC

3 files changed, 166 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 b6ac14e17..850304a74 100644
--- a/internal/service/repository/gc.go
+++ b/internal/service/repository/gc.go
@@ -1,10 +1,15 @@
 package repository
 
 import (
+	"fmt"
+	"os"
+	"path/filepath"
+
 	"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 +32,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 +63,75 @@ 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)
+
+	f, err := os.Open(keepAroundsPath)
+	if os.IsNotExist(err) {
+		return nil
+	}
+	if err != nil {
+		return err
+	}
+	defer f.Close()
+
+	// Passing -1 causes us to get the entire directory. This should be ok
+	// since this is only containing loose refs
+	refInfos, err := f.Readdir(-1)
+	if err != nil {
+		return err
+	}
+
+	for _, info := range refInfos {
+		if info.IsDir() {
+			return nil
+		}
+
+		refName := fmt.Sprintf("%s/%s", keepAroundsPrefix, info.Name())
+		path := filepath.Join(repoPath, keepAroundsPrefix, info.Name())
+
+		if info.Size() == 0 {
+			// if the file is empty, we already know we need to fix it
+			return fixRef(ctx, repo, batch, path, refName, info.Name())
+		}
+
+		if _, err = batch.Info(refName); err != nil {
+			return fixRef(ctx, repo, batch, path, refName, info.Name())
+		}
+	}
+
+	return nil
+}
+
+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 name was a valid sha, recreate the ref
+	if _, err := batch.Info(sha); err == nil {
+		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..0add84473 100644
--- a/internal/service/repository/gc_test.go
+++ b/internal/service/repository/gc_test.go
@@ -160,6 +160,86 @@ 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()
+
+			// Creating the keeparound without using git so we can create invalid ones
+			refPath := filepath.Join(testRepoPath, fmt.Sprintf("refs/keep-around/%s", testcase.refName))
+			err := ioutil.WriteFile(refPath, []byte(testcase.refContent), 0644)
+			require.NoError(t, err)
+
+			req := &gitalypb.GarbageCollectRequest{Repository: testRepo}
+			response, err := client.GarbageCollect(ctx, req)
+
+			require.NoError(t, err)
+			assert.NotNil(t, response)
+
+			if testcase.shouldExist {
+				checkValidKeepAroundRef(t, testRepoPath, testcase.refName)
+			} else {
+				_, err := os.Stat(refPath)
+				assert.Equal(t, true, os.IsNotExist(err))
+			}
+
+		})
+	}
+}
+
+func checkValidKeepAroundRef(t *testing.T, repoPath string, refname string) {
+	keepAroundName := fmt.Sprintf("refs/keep-around/%s", refname)
+	testhelper.MustRunCommand(t, nil, "git", "-C", repoPath, "show", keepAroundName)
+}
+
 func createFileWithTimes(path string, mTime time.Time) {
 	ioutil.WriteFile(path, nil, 0644)
 	os.Chtimes(path, mTime, mTime)