Remove orphaned worktree directories

When cleanup encounters a stale worktree directory it attempts to remove
it using `git worktree remove`. This however fails when there is a
directory that git does not know about.

It is not entirely clear how this situation arises but since this
directory is gitaly controlled and `CreateBundle` calls cleanup and this
is required by backups, we should attempt to remove any such orphaned
directories instead of erroring.

Changelog: fixed

2 files changed, 135 insertions, 11 deletions

diff --git a/internal/gitaly/service/repository/cleanup.go b/internal/gitaly/service/repository/cleanup.go
index d3b3dbfbd..3b44f2563 100644
--- a/internal/gitaly/service/repository/cleanup.go
+++ b/internal/gitaly/service/repository/cleanup.go
@@ -4,14 +4,18 @@ package repository
 import (
 	"bytes"
 	"context"
+	"errors"
 	"fmt"
 	"io/ioutil"
 	"os"
 	"path/filepath"
+	"strings"
 	"time"
 
+	"gitlab.com/gitlab-org/gitaly/v14/internal/command"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/git"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/git/localrepo"
+	"gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/config"
 	"gitlab.com/gitlab-org/gitaly/v14/proto/go/gitalypb"
 	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/status"
@@ -75,17 +79,15 @@ func (s *server) cleanStaleWorktrees(ctx context.Context, repo *localrepo.Repo,
 		}
 
 		if info.ModTime().Before(threshold) {
-			var stderr bytes.Buffer
-			if err := repo.ExecAndWait(ctx, git.SubSubCmd{
-				Name:   "worktree",
-				Action: "remove",
-				Flags:  []git.Option{git.Flag{Name: "--force"}},
-				Args:   []string{info.Name()},
-			},
-				git.WithRefTxHook(ctx, repo, s.cfg),
-				git.WithStderr(&stderr),
-			); err != nil {
-				return fmt.Errorf("worktree remove: %w, stderr: %q", err, stderr.String())
+			err := removeWorktree(ctx, s.cfg, repo, info.Name())
+			switch {
+			case errors.Is(err, errUnknownWorktree):
+				// if git doesn't recognise the worktree then we can safely remove it
+				if err := os.RemoveAll(filepath.Join(worktreePath, info.Name())); err != nil {
+					return fmt.Errorf("worktree remove dir: %w", err)
+				}
+			case err != nil:
+				return err
 			}
 		}
 	}
@@ -93,6 +95,38 @@ func (s *server) cleanStaleWorktrees(ctx context.Context, repo *localrepo.Repo,
 	return nil
 }
 
+// errUnknownWorktree indicates that git does not recognise the worktree
+var errUnknownWorktree = errors.New("unknown worktree")
+
+func removeWorktree(ctx context.Context, cfg config.Cfg, repo *localrepo.Repo, name string) error {
+	var stderr bytes.Buffer
+	err := repo.ExecAndWait(ctx, git.SubSubCmd{
+		Name:   "worktree",
+		Action: "remove",
+		Flags:  []git.Option{git.Flag{Name: "--force"}},
+		Args:   []string{name},
+	},
+		git.WithRefTxHook(ctx, repo, cfg),
+		git.WithStderr(&stderr),
+	)
+	if isExitWithCode(err, 128) && strings.HasPrefix(stderr.String(), "fatal: '"+name+"' is not a working tree") {
+		return errUnknownWorktree
+	} else if err != nil {
+		return fmt.Errorf("remove worktree: %w, stderr: %q", err, stderr.String())
+	}
+
+	return nil
+}
+
+func isExitWithCode(err error, code int) bool {
+	actual, ok := command.ExitStatus(err)
+	if !ok {
+		return false
+	}
+
+	return code == actual
+}
+
 func (s *server) cleanDisconnectedWorktrees(ctx context.Context, repo *localrepo.Repo) error {
 	return repo.ExecAndWait(ctx, git.SubSubCmd{
 		Name:   "worktree",
diff --git a/internal/gitaly/service/repository/cleanup_test.go b/internal/gitaly/service/repository/cleanup_test.go
index dac386497..feb3de6c4 100644
--- a/internal/gitaly/service/repository/cleanup_test.go
+++ b/internal/gitaly/service/repository/cleanup_test.go
@@ -9,8 +9,12 @@ import (
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
+	"gitlab.com/gitlab-org/gitaly/v14/internal/git"
+	"gitlab.com/gitlab-org/gitaly/v14/internal/git/catfile"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/git/gittest"
+	"gitlab.com/gitlab-org/gitaly/v14/internal/git/localrepo"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/testhelper"
+	"gitlab.com/gitlab-org/gitaly/v14/internal/testhelper/testcfg"
 	"gitlab.com/gitlab-org/gitaly/v14/proto/go/gitalypb"
 )
 
@@ -77,6 +81,29 @@ func TestCleanupDeletesStaleWorktrees(t *testing.T) {
 	}
 }
 
+func TestCleanupDeletesOrphanedWorktrees(t *testing.T) {
+	t.Parallel()
+
+	_, repo, repoPath, client := setupRepositoryService(t)
+
+	worktreeCheckoutPath := filepath.Join(repoPath, worktreePrefix, "test-worktree")
+	basePath := filepath.Join(repoPath, "worktrees")
+	worktreePath := filepath.Join(basePath, "test-worktree")
+
+	require.NoError(t, os.MkdirAll(worktreeCheckoutPath, os.ModePerm))
+	require.NoError(t, os.Chtimes(worktreeCheckoutPath, oldTreeTime, oldTreeTime))
+
+	ctx, cancel := testhelper.Context()
+	defer cancel()
+
+	c, err := client.Cleanup(ctx, &gitalypb.CleanupRequest{Repository: repo})
+	assert.NoError(t, err)
+	assert.NotNil(t, c)
+
+	require.NoDirExists(t, worktreeCheckoutPath)
+	require.NoDirExists(t, worktreePath)
+}
+
 // TODO: replace emulated rebase RPC with actual
 // https://gitlab.com/gitlab-org/gitaly/issues/1750
 func TestCleanupDisconnectedWorktrees(t *testing.T) {
@@ -120,3 +147,66 @@ func TestCleanupDisconnectedWorktrees(t *testing.T) {
 	// to checkout another worktree at the same path
 	gittest.AddWorktree(t, cfg, repoPath, worktreePath)
 }
+
+func TestRemoveWorktree(t *testing.T) {
+	t.Parallel()
+
+	cfg, repoProto, repoPath := testcfg.BuildWithRepo(t)
+	gitCmdFactory := git.NewExecCommandFactory(cfg)
+	repo := localrepo.New(gitCmdFactory, catfile.NewCache(cfg), repoProto, cfg)
+
+	existingWorktreePath := filepath.Join(repoPath, worktreePrefix, "existing")
+	gittest.AddWorktree(t, cfg, repoPath, existingWorktreePath)
+
+	disconnectedWorktreePath := filepath.Join(repoPath, worktreePrefix, "disconnected")
+	gittest.AddWorktree(t, cfg, repoPath, disconnectedWorktreePath)
+	require.NoError(t, os.RemoveAll(disconnectedWorktreePath))
+
+	orphanedWorktreePath := filepath.Join(repoPath, worktreePrefix, "orphaned")
+	require.NoError(t, os.MkdirAll(orphanedWorktreePath, os.ModePerm))
+
+	for _, tc := range []struct {
+		worktree     string
+		errorIs      error
+		expectExists bool
+	}{
+		{
+			worktree:     "existing",
+			expectExists: false,
+		},
+		{
+			worktree:     "disconnected",
+			expectExists: false,
+		},
+		{
+			worktree:     "unknown",
+			errorIs:      errUnknownWorktree,
+			expectExists: false,
+		},
+		{
+			worktree:     "orphaned",
+			errorIs:      errUnknownWorktree,
+			expectExists: true,
+		},
+	} {
+		t.Run(tc.worktree, func(t *testing.T) {
+			ctx, cancel := testhelper.Context()
+			defer cancel()
+
+			worktreePath := filepath.Join(repoPath, worktreePrefix, tc.worktree)
+
+			err := removeWorktree(ctx, cfg, repo, tc.worktree)
+			if tc.errorIs == nil {
+				require.NoError(t, err)
+			} else {
+				require.ErrorIs(t, err, tc.errorIs)
+			}
+
+			if tc.expectExists {
+				require.DirExists(t, worktreePath)
+			} else {
+				require.NoDirExists(t, worktreePath)
+			}
+		})
+	}
+}