Cleanup RPC prunes disconnected work trees

7 files changed, 273 insertions, 21 deletions

diff --git a/changelogs/unreleased/po_stale_worktrees.yml b/changelogs/unreleased/po_stale_worktrees.yml
new file mode 100644
index 000000000..fcfb06aa0
--- /dev/null
+++ b/changelogs/unreleased/po_stale_worktrees.yml
@@ -0,0 +1,5 @@
+---
+title: Cleanup RPC prunes disconnected work trees
+merge_request: 1189
+author:
+type: fixed
diff --git a/internal/git/proto.go b/internal/git/proto.go
index bfbbcfcc2..dfe112311 100644
--- a/internal/git/proto.go
+++ b/internal/git/proto.go
@@ -62,25 +62,90 @@ func Version() (string, error) {
 	return ver[2], nil
 }
 
-// SupportsDeltaIslands checks if a version string (e.g. "2.20.0")
-// corresponds to a Git version that supports delta islands.
-func SupportsDeltaIslands(version string) (bool, error) {
-	versionSplit := strings.SplitN(version, ".", 3)
+// VersionLessThan returns true if the parsed version value of v1Str is less
+// than the parsed version value of v2Str. An error can be returned if the
+// strings cannot be parsed.
+// Note: this is an extremely simplified semver comparison algorithm
+func VersionLessThan(v1Str, v2Str string) (bool, error) {
+	var (
+		v1, v2 version
+		err    error
+	)
+
+	for _, v := range []struct {
+		string
+		*version
+	}{
+		{v1Str, &v1},
+		{v2Str, &v2},
+	} {
+		*v.version, err = parseVersion(v.string)
+		if err != nil {
+			return false, err
+		}
+	}
+
+	return versionLessThan(v1, v2), nil
+}
+
+func versionLessThan(v1, v2 version) bool {
+	switch {
+
+	case v1.major < v2.major:
+		return true
+	case v1.major > v2.major:
+		return false
+
+	case v1.minor < v2.minor:
+		return true
+	case v1.minor > v2.minor:
+		return false
+
+	case v1.patch < v2.patch:
+		return true
+	case v1.patch > v2.patch:
+		return false
+
+	default:
+		// this should only be reachable when versions are equal
+		return false
+
+	}
+}
+
+type version struct {
+	major, minor, patch uint32
+}
+
+func parseVersion(versionStr string) (version, error) {
+	versionSplit := strings.SplitN(versionStr, ".", 3)
 	if len(versionSplit) < 3 {
-		return false, fmt.Errorf("expected major.minor.patch in %q", version)
+		return version{}, fmt.Errorf("expected major.minor.patch in %q", versionStr)
 	}
 
-	var major, minor uint32
-	for i, v := range []*uint32{&major, &minor} {
+	var ver version
+
+	for i, v := range []*uint32{&ver.major, &ver.minor, &ver.patch} {
 		n64, err := strconv.ParseUint(versionSplit[i], 10, 32)
 		if err != nil {
-			return false, err
+			return version{}, err
 		}
 
 		*v = uint32(n64)
 	}
 
-	return major >= 2 && minor >= 20, nil
+	return ver, nil
+}
+
+// SupportsDeltaIslands checks if a version string (e.g. "2.20.0")
+// corresponds to a Git version that supports delta islands.
+func SupportsDeltaIslands(versionStr string) (bool, error) {
+	v, err := parseVersion(versionStr)
+	if err != nil {
+		return false, err
+	}
+
+	return !versionLessThan(v, version{2, 20, 0}), nil
 }
 
 // BuildGitOptions helps to generate options to the git command.
diff --git a/internal/git/proto_test.go b/internal/git/proto_test.go
new file mode 100644
index 000000000..3acb952ae
--- /dev/null
+++ b/internal/git/proto_test.go
@@ -0,0 +1,92 @@
+package git_test
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/require"
+	"gitlab.com/gitlab-org/gitaly/internal/git"
+)
+
+func TestVersionComparator(t *testing.T) {
+	for _, tc := range []struct {
+		v1, v2 string
+		expect bool
+	}{
+		// v1 < v2 == expect
+		{"0.0.0", "0.0.0", false},
+		{"0.0.0", "0.0.1", true},
+		{"0.0.0", "0.1.0", true},
+		{"0.0.0", "0.1.1", true},
+		{"0.0.0", "1.0.0", true},
+		{"0.0.0", "1.0.1", true},
+		{"0.0.0", "1.1.0", true},
+		{"0.0.0", "1.1.1", true},
+
+		{"0.0.1", "0.0.0", false},
+		{"0.0.1", "0.0.1", false},
+		{"0.0.1", "0.1.0", true},
+		{"0.0.1", "0.1.1", true},
+		{"0.0.1", "1.0.0", true},
+		{"0.0.1", "1.0.1", true},
+		{"0.0.1", "1.1.0", true},
+		{"0.0.1", "1.1.1", true},
+
+		{"0.1.0", "0.0.0", false},
+		{"0.1.0", "0.0.1", false},
+		{"0.1.0", "0.1.0", false},
+		{"0.1.0", "0.1.1", true},
+		{"0.1.0", "1.0.0", true},
+		{"0.1.0", "1.0.1", true},
+		{"0.1.0", "1.1.0", true},
+		{"0.1.0", "1.1.1", true},
+
+		{"0.1.1", "0.0.0", false},
+		{"0.1.1", "0.0.1", false},
+		{"0.1.1", "0.1.0", false},
+		{"0.1.1", "0.1.1", false},
+		{"0.1.1", "1.0.0", true},
+		{"0.1.1", "1.0.1", true},
+		{"0.1.1", "1.1.0", true},
+		{"0.1.1", "1.1.1", true},
+
+		{"1.0.0", "0.0.0", false},
+		{"1.0.0", "0.0.1", false},
+		{"1.0.0", "0.1.0", false},
+		{"1.0.0", "0.1.1", false},
+		{"1.0.0", "1.0.0", false},
+		{"1.0.0", "1.0.1", true},
+		{"1.0.0", "1.1.0", true},
+		{"1.0.0", "1.1.1", true},
+
+		{"1.0.1", "0.0.0", false},
+		{"1.0.1", "0.0.1", false},
+		{"1.0.1", "0.1.0", false},
+		{"1.0.1", "0.1.1", false},
+		{"1.0.1", "1.0.0", false},
+		{"1.0.1", "1.0.1", false},
+		{"1.0.1", "1.1.0", true},
+		{"1.0.1", "1.1.1", true},
+
+		{"1.1.0", "0.0.0", false},
+		{"1.1.0", "0.0.1", false},
+		{"1.1.0", "0.1.0", false},
+		{"1.1.0", "0.1.1", false},
+		{"1.1.0", "1.0.0", false},
+		{"1.1.0", "1.0.1", false},
+		{"1.1.0", "1.1.0", false},
+		{"1.1.0", "1.1.1", true},
+
+		{"1.1.1", "0.0.0", false},
+		{"1.1.1", "0.0.1", false},
+		{"1.1.1", "0.1.0", false},
+		{"1.1.1", "0.1.1", false},
+		{"1.1.1", "1.0.0", false},
+		{"1.1.1", "1.0.1", false},
+		{"1.1.1", "1.1.0", false},
+		{"1.1.1", "1.1.1", false},
+	} {
+		actual, err := git.VersionLessThan(tc.v1, tc.v2)
+		require.NoError(t, err)
+		require.Equal(t, tc.expect, actual)
+	}
+}
diff --git a/internal/service/repository/cleanup.go b/internal/service/repository/cleanup.go
index 9cae1580b..35e96f4bb 100644
--- a/internal/service/repository/cleanup.go
+++ b/internal/service/repository/cleanup.go
@@ -9,27 +9,29 @@ import (
 	"time"
 
 	"gitlab.com/gitlab-org/gitaly-proto/go/gitalypb"
+	"gitlab.com/gitlab-org/gitaly/internal/git"
 	"gitlab.com/gitlab-org/gitaly/internal/helper"
+
 	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/status"
 )
 
 var lockFiles = []string{"config.lock", "HEAD.lock"}
 
-func (server) Cleanup(_ctx context.Context, in *gitalypb.CleanupRequest) (*gitalypb.CleanupResponse, error) {
-	repoPath, err := helper.GetRepoPath(in.GetRepository())
-	if err != nil {
-		return nil, err
-	}
-
-	if err := cleanupRepo(repoPath); err != nil {
+func (server) Cleanup(ctx context.Context, in *gitalypb.CleanupRequest) (*gitalypb.CleanupResponse, error) {
+	if err := cleanupRepo(ctx, in.GetRepository()); err != nil {
 		return nil, err
 	}
 
 	return &gitalypb.CleanupResponse{}, nil
 }
 
-func cleanupRepo(repoPath string) error {
+func cleanupRepo(ctx context.Context, repo *gitalypb.Repository) error {
+	repoPath, err := helper.GetRepoPath(repo)
+	if err != nil {
+		return err
+	}
+
 	threshold := time.Now().Add(-1 * time.Hour)
 	if err := cleanRefsLocks(filepath.Join(repoPath, "refs"), threshold); err != nil {
 		return status.Errorf(codes.Internal, "Cleanup: cleanRefsLocks: %v", err)
@@ -43,6 +45,10 @@ func cleanupRepo(repoPath string) error {
 		return status.Errorf(codes.Internal, "Cleanup: cleanStaleWorktrees: %v", err)
 	}
 
+	if err := cleanDisconnectedWorktrees(ctx, repo); err != nil {
+		return status.Errorf(codes.Internal, "Cleanup: cleanDisconnectedWorktrees: %v", err)
+	}
+
 	configLockThreshod := time.Now().Add(-15 * time.Minute)
 	if err := cleanFileLocks(repoPath, configLockThreshod); err != nil {
 		return status.Errorf(codes.Internal, "Cleanup: cleanupConfigLock: %v", err)
@@ -128,6 +134,15 @@ func cleanStaleWorktrees(repoPath string, threshold time.Time) error {
 	return nil
 }
 
+func cleanDisconnectedWorktrees(ctx context.Context, repo *gitalypb.Repository) error {
+	cmd, err := git.Command(ctx, repo, "worktree", "prune")
+	if err != nil {
+		return err
+	}
+
+	return cmd.Wait()
+}
+
 func cleanFileLocks(repoPath string, threshold time.Time) error {
 	for _, fileName := range lockFiles {
 		lockPath := filepath.Join(repoPath, fileName)
diff --git a/internal/service/repository/cleanup_test.go b/internal/service/repository/cleanup_test.go
index da88cce37..cec59c4ec 100644
--- a/internal/service/repository/cleanup_test.go
+++ b/internal/service/repository/cleanup_test.go
@@ -2,6 +2,7 @@ package repository
 
 import (
 	"os"
+	"os/exec"
 	"path/filepath"
 	"testing"
 	"time"
@@ -9,6 +10,7 @@ import (
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"gitlab.com/gitlab-org/gitaly-proto/go/gitalypb"
+	"gitlab.com/gitlab-org/gitaly/internal/git"
 	"gitlab.com/gitlab-org/gitaly/internal/testhelper"
 )
 
@@ -185,6 +187,75 @@ func TestCleanupDeletesStaleWorktrees(t *testing.T) {
 	}
 }
 
+func TestCleanupDisconnectedWorktrees(t *testing.T) {
+	const (
+		worktreeName     = "test-worktree"
+		worktreeAdminDir = "worktrees"
+	)
+
+	addWorkTree := func(repoPath, worktree string) error {
+		return exec.Command(
+			"git",
+			testhelper.AddWorktreeArgs(repoPath, worktree)...,
+		).Run()
+	}
+
+	server, serverSocketPath := runRepoServer(t)
+	defer server.Stop()
+
+	client, conn := newRepositoryClient(t, serverSocketPath)
+	defer conn.Close()
+
+	testRepo, testRepoPath, cleanupFn := testhelper.NewTestRepo(t)
+	defer cleanupFn()
+
+	worktreePath := filepath.Join(testRepoPath, worktreeName)
+	worktreeAdminPath := filepath.Join(
+		testRepoPath, worktreeAdminDir, filepath.Base(worktreeName),
+	)
+
+	req := &gitalypb.CleanupRequest{Repository: testRepo}
+
+	testhelper.AddWorktree(t, testRepoPath, worktreeName)
+
+	ctx, cancel := testhelper.Context()
+	defer cancel()
+
+	// removing the work tree path but leaving the administrative files in
+	// $GIT_DIR/worktrees will result in the work tree being in a
+	// "disconnected" state
+	err := os.RemoveAll(worktreePath)
+	require.NoError(t, err,
+		"disconnecting worktree by removing work tree at %s should succeed", worktreePath,
+	)
+
+	// TODO: remove the following version checks when the lowest supported git
+	// version is 2.20.0 or higher. Refer to relevant gitlab-ce issue:
+	// https://gitlab.com/gitlab-org/gitlab-ce/issues/54255
+	version, err := git.Version()
+	require.NoError(t, err)
+
+	pre2_20_0, err := git.VersionLessThan(version, "2.20.0")
+	require.NoError(t, err)
+
+	if !pre2_20_0 {
+		err = addWorkTree(testRepoPath, worktreeName)
+		require.Error(t, err,
+			"creating a new work tree at the same path as a disconnected work tree should fail",
+		)
+	}
+
+	// cleanup should prune the disconnected worktree administrative files
+	_, err = client.Cleanup(ctx, req)
+	require.NoError(t, err)
+	testhelper.AssertFileNotExists(t, worktreeAdminPath)
+
+	// if the worktree administrative files are pruned, then we should be able
+	// to checkout another worktree at the same path
+	err = addWorkTree(testRepoPath, worktreeName)
+	require.NoError(t, err)
+}
+
 func TestCleanupFileLocks(t *testing.T) {
 	server, serverSocketPath := runRepoServer(t)
 	defer server.Stop()
diff --git a/internal/service/repository/gc.go b/internal/service/repository/gc.go
index 879059475..f16d7693f 100644
--- a/internal/service/repository/gc.go
+++ b/internal/service/repository/gc.go
@@ -30,7 +30,7 @@ func (server) GarbageCollect(ctx context.Context, in *gitalypb.GarbageCollectReq
 		return nil, err
 	}
 
-	if err := cleanupRepo(repoPath); err != nil {
+	if err := cleanupRepo(ctx, in.GetRepository()); err != nil {
 		return nil, err
 	}
 
diff --git a/internal/testhelper/testhelper.go b/internal/testhelper/testhelper.go
index 32fdfe418..acad406d7 100644
--- a/internal/testhelper/testhelper.go
+++ b/internal/testhelper/testhelper.go
@@ -421,11 +421,15 @@ func cloneTestRepo(t *testing.T, bare bool) (repo *gitalypb.Repository, repoPath
 	return repo, repoPath, func() { os.RemoveAll(repoPath) }
 }
 
+// AddWorktreeArgs returns git command arguments for adding a worktree at the
+// specified repo
+func AddWorktreeArgs(repoPath, worktreeName string) []string {
+	return []string{"-C", repoPath, "worktree", "add", "--detach", worktreeName}
+}
+
 // AddWorktree creates a worktree in the repository path for tests
 func AddWorktree(t *testing.T, repoPath string, worktreeName string) {
-	args := []string{"-C", repoPath, "worktree", "add", "--detach", worktreeName}
-
-	MustRunCommand(t, nil, "git", args...)
+	MustRunCommand(t, nil, "git", AddWorktreeArgs(repoPath, worktreeName)...)
 }
 
 // ConfigureGitalySSH configures the gitaly-ssh command for tests