localrepo: Unlock symbolic refs when the update fails

We use a `safe.LockingFileWriter` to get transactional semantics when
updating symbolic references. This mechanism will create a lock for the
file that is about to be updated, votes on the contents of the file and
then finally unlocks it if everything went alright. In case something
goes wrong though the caller must make sure to close the writer, or
otherwise we may leak the lockfile and thus also obstruct all subsequent
writes to that symbolic reference. We don't do this correctly though.

Fix this bug by always unlocking the locking file writer.

Changelog: fixed

2 files changed, 7 insertions, 3 deletions

diff --git a/internal/git/localrepo/refs.go b/internal/git/localrepo/refs.go
index 769a833a4..30cc93000 100644
--- a/internal/git/localrepo/refs.go
+++ b/internal/git/localrepo/refs.go
@@ -10,6 +10,7 @@ import (
 	"path/filepath"
 	"strings"
 
+	"github.com/grpc-ecosystem/go-grpc-middleware/logging/logrus/ctxlogrus"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/command"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/git"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/transaction"
@@ -205,6 +206,11 @@ func (repo *Repo) setDefaultBranchWithTransaction(ctx context.Context, txManager
 	if err != nil {
 		return fmt.Errorf("getting file writer: %w", err)
 	}
+	defer func() {
+		if err := lockingFileWriter.Close(); err != nil {
+			ctxlogrus.Extract(ctx).WithError(err).Error("closing locked HEAD: %w", err)
+		}
+	}()
 
 	if _, err := lockingFileWriter.Write(newHeadContent); err != nil {
 		return fmt.Errorf("writing temporary HEAD: %w", err)
diff --git a/internal/git/localrepo/refs_test.go b/internal/git/localrepo/refs_test.go
index 6bbdea77a..df5c1c7ec 100644
--- a/internal/git/localrepo/refs_test.go
+++ b/internal/git/localrepo/refs_test.go
@@ -620,9 +620,7 @@ func TestRepo_SetDefaultBranch_errors(t *testing.T) {
 		err = repo.SetDefaultBranch(ctx, failingTxManager, "refs/heads/branch")
 		require.Error(t, err)
 		require.Equal(t, "committing temporary HEAD: voting on locked file: preimage vote: injected error", err.Error())
-
-		// This is a bug: the lockfile does not get cleaned up correctly.
-		require.FileExists(t, filepath.Join(repoPath, "HEAD.lock"))
+		require.NoFileExists(t, filepath.Join(repoPath, "HEAD.lock"))
 	})
 }