repository: Always lock gitconfig and gitattributes on write

We used to directly write to both the gitconfig and gitattributes files
without having proper locking semantics which assert that there was no
concurrent write to the same file. As a result, it could be that
multiple RPCs which modify the same file all succeeded, but overwrote
their respective changes.

This was fixed via a new locking file writer, which locks the file,
votes on the changes and then commits it into place only if all nodes
agree on the change and if there was no concurrent write to the same
file. Like this, we now have atomic guarantees on both the gitconfig and
gitattributes files.

Remove the feature flag which guards this code. It has been tested in
production without any observed issues.

Changelog: fixed

6 files changed, 30 insertions, 162 deletions

diff --git a/internal/gitaly/service/repository/apply_gitattributes.go b/internal/gitaly/service/repository/apply_gitattributes.go
index 38b517837..00d3c2bf7 100644
--- a/internal/gitaly/service/repository/apply_gitattributes.go
+++ b/internal/gitaly/service/repository/apply_gitattributes.go
@@ -8,12 +8,10 @@ import (
 	"os"
 	"path/filepath"
 
-	"github.com/grpc-ecosystem/go-grpc-middleware/logging/logrus/ctxlogrus"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/git"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/git/catfile"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/transaction"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/helper"
-	"gitlab.com/gitlab-org/gitaly/v14/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/safe"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/transaction/txinfo"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/transaction/voting"
@@ -65,57 +63,23 @@ func (s *server) applyGitattributes(ctx context.Context, c catfile.Batch, repoPa
 		return err
 	}
 
-	if featureflag.TxFileLocking.IsEnabled(ctx) {
-		writer, err := safe.NewLockingFileWriter(attributesPath, safe.LockingFileWriterConfig{
-			FileWriterConfig: safe.FileWriterConfig{FileMode: attributesFileMode},
-		})
-		if err != nil {
-			return fmt.Errorf("creating gitattributes writer: %w", err)
-		}
-		defer writer.Close()
-
-		if _, err := io.CopyN(writer, blobObj.Reader, blobInfo.Size); err != nil {
-			return err
-		}
-
-		if err := transaction.CommitLockedFile(ctx, s.txManager, writer); err != nil {
-			return fmt.Errorf("committing gitattributes: %w", err)
-		}
-
-		return nil
-	}
-
-	tempFile, err := os.CreateTemp(infoPath, "attributes")
+	writer, err := safe.NewLockingFileWriter(attributesPath, safe.LockingFileWriterConfig{
+		FileWriterConfig: safe.FileWriterConfig{FileMode: attributesFileMode},
+	})
 	if err != nil {
-		return helper.ErrInternalf("creating temporary gitattributes file: %w", err)
+		return fmt.Errorf("creating gitattributes writer: %w", err)
 	}
-	defer func() {
-		if err := os.Remove(tempFile.Name()); err != nil && !errors.Is(err, os.ErrNotExist) {
-			ctxlogrus.Extract(ctx).WithError(err).Errorf("failed to remove tmp file %q", tempFile.Name())
-		}
-	}()
+	defer writer.Close()
 
-	// Write attributes to temp file
-	if _, err := io.CopyN(tempFile, blobObj.Reader, blobInfo.Size); err != nil {
+	if _, err := io.CopyN(writer, blobObj.Reader, blobInfo.Size); err != nil {
 		return err
 	}
 
-	if err := tempFile.Close(); err != nil {
-		return err
+	if err := transaction.CommitLockedFile(ctx, s.txManager, writer); err != nil {
+		return fmt.Errorf("committing gitattributes: %w", err)
 	}
 
-	// Change the permission of tempFile as the permission of file attributesPath
-	if err := os.Chmod(tempFile.Name(), attributesFileMode); err != nil {
-		return err
-	}
-
-	// Vote on the contents of the newly written gitattributes file.
-	if err := s.vote(ctx, blobInfo.Oid); err != nil {
-		return fmt.Errorf("could not commit gitattributes: %w", err)
-	}
-
-	// Rename temp file and return the result
-	return os.Rename(tempFile.Name(), attributesPath)
+	return nil
 }
 
 func (s *server) vote(ctx context.Context, oid git.ObjectID) error {
diff --git a/internal/gitaly/service/repository/apply_gitattributes_test.go b/internal/gitaly/service/repository/apply_gitattributes_test.go
index b83c2fd09..f85b6f456 100644
--- a/internal/gitaly/service/repository/apply_gitattributes_test.go
+++ b/internal/gitaly/service/repository/apply_gitattributes_test.go
@@ -12,10 +12,8 @@ import (
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/backchannel"
-	"gitlab.com/gitlab-org/gitaly/v14/internal/git"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/service"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/metadata"
-	"gitlab.com/gitlab-org/gitaly/v14/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/testhelper"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/testhelper/testassert"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/testhelper/testcfg"
@@ -29,13 +27,11 @@ import (
 )
 
 func TestApplyGitattributesSuccess(t *testing.T) {
-	testhelper.NewFeatureSets([]featureflag.FeatureFlag{
-		featureflag.TxFileLocking,
-	}).Run(t, testApplyGitattributesSuccess)
-}
-
-func testApplyGitattributesSuccess(t *testing.T, ctx context.Context) {
 	t.Parallel()
+
+	ctx, cancel := testhelper.Context()
+	defer cancel()
+
 	cfg, repo, _, client := setupRepositoryService(t)
 
 	infoPath := filepath.Join(cfg.Storages[0].Path, repo.GetRelativePath(), "info")
@@ -93,13 +89,11 @@ func (s *testTransactionServer) VoteTransaction(ctx context.Context, in *gitalyp
 }
 
 func TestApplyGitattributesWithTransaction(t *testing.T) {
-	testhelper.NewFeatureSets([]featureflag.FeatureFlag{
-		featureflag.TxFileLocking,
-	}).Run(t, testApplyGitattributesWithTransaction)
-}
-
-func testApplyGitattributesWithTransaction(t *testing.T, ctx context.Context) {
 	t.Parallel()
+
+	ctx, cancel := testhelper.Context()
+	defer cancel()
+
 	cfg, repo, repoPath := testcfg.BuildWithRepo(t)
 
 	transactionServer := &testTransactionServer{}
@@ -138,16 +132,8 @@ func testApplyGitattributesWithTransaction(t *testing.T, ctx context.Context) {
 			desc:     "successful vote writes gitattributes",
 			revision: []byte("e63f41fe459e62e1228fcef60d7189127aeba95a"),
 			voteFn: func(t *testing.T, request *gitalypb.VoteTransactionRequest) (*gitalypb.VoteTransactionResponse, error) {
-				var expectedHash []byte
-				if featureflag.TxFileLocking.IsEnabled(ctx) {
-					vote := voting.VoteFromData([]byte("/custom-highlighting/*.gitlab-custom gitlab-language=ruby\n"))
-					expectedHash = vote.Bytes()
-				} else {
-					oid, err := git.NewObjectIDFromHex("36814a3da051159a1683479e7a1487120309db8f")
-					require.NoError(t, err)
-					expectedHash, err = oid.Bytes()
-					require.NoError(t, err)
-				}
+				vote := voting.VoteFromData([]byte("/custom-highlighting/*.gitlab-custom gitlab-language=ruby\n"))
+				expectedHash := vote.Bytes()
 
 				require.Equal(t, expectedHash, request.ReferenceUpdatesHash)
 				return &gitalypb.VoteTransactionResponse{
@@ -166,11 +152,7 @@ func testApplyGitattributesWithTransaction(t *testing.T, ctx context.Context) {
 			},
 			shouldExist: false,
 			expectedErr: func() error {
-				if featureflag.TxFileLocking.IsEnabled(ctx) {
-					return status.Error(codes.Unknown, "committing gitattributes: voting on locked file: preimage vote: transaction was aborted")
-				}
-
-				return status.Error(codes.Unknown, "could not commit gitattributes: vote failed: transaction was aborted")
+				return status.Error(codes.Unknown, "committing gitattributes: voting on locked file: preimage vote: transaction was aborted")
 			}(),
 		},
 		{
@@ -180,13 +162,8 @@ func testApplyGitattributesWithTransaction(t *testing.T, ctx context.Context) {
 				return nil, errors.New("foobar")
 			},
 			shouldExist: false,
-
 			expectedErr: func() error {
-				if featureflag.TxFileLocking.IsEnabled(ctx) {
-					return status.Error(codes.Unknown, "committing gitattributes: voting on locked file: preimage vote: rpc error: code = Unknown desc = foobar")
-				}
-
-				return status.Error(codes.Unknown, "could not commit gitattributes: vote failed: rpc error: code = Unknown desc = foobar")
+				return status.Error(codes.Unknown, "committing gitattributes: voting on locked file: preimage vote: rpc error: code = Unknown desc = foobar")
 			}(),
 		},
 		{
@@ -232,13 +209,11 @@ func testApplyGitattributesWithTransaction(t *testing.T, ctx context.Context) {
 }
 
 func TestApplyGitattributesFailure(t *testing.T) {
-	testhelper.NewFeatureSets([]featureflag.FeatureFlag{
-		featureflag.TxFileLocking,
-	}).Run(t, testApplyGitattributesFailure)
-}
-
-func testApplyGitattributesFailure(t *testing.T, ctx context.Context) {
 	t.Parallel()
+
+	ctx, cancel := testhelper.Context()
+	defer cancel()
+
 	_, repo, _, client := setupRepositoryService(t)
 
 	tests := []struct {
diff --git a/internal/gitaly/service/repository/config.go b/internal/gitaly/service/repository/config.go
index c650bc65b..a0a7f420c 100644
--- a/internal/gitaly/service/repository/config.go
+++ b/internal/gitaly/service/repository/config.go
@@ -1,16 +1,11 @@
 package repository
 
 import (
-	"context"
-	"fmt"
 	"io"
 	"os"
 	"path/filepath"
 
-	"gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/transaction"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/helper"
-	"gitlab.com/gitlab-org/gitaly/v14/internal/transaction/txinfo"
-	"gitlab.com/gitlab-org/gitaly/v14/internal/transaction/voting"
 	"gitlab.com/gitlab-org/gitaly/v14/proto/go/gitalypb"
 	"gitlab.com/gitlab-org/gitaly/v14/streamio"
 	"google.golang.org/grpc/codes"
@@ -49,38 +44,3 @@ func (s *server) GetConfig(
 
 	return nil
 }
-
-func (s *server) voteOnConfig(ctx context.Context, repo *gitalypb.Repository) error {
-	return transaction.RunOnContext(ctx, func(tx txinfo.Transaction) error {
-		repoPath, err := s.locator.GetPath(repo)
-		if err != nil {
-			return fmt.Errorf("get repo path: %w", err)
-		}
-
-		var vote voting.Vote
-
-		config, err := os.Open(filepath.Join(repoPath, "config"))
-		switch {
-		case err == nil:
-			hash := voting.NewVoteHash()
-			if _, err := io.Copy(hash, config); err != nil {
-				return fmt.Errorf("seeding vote: %w", err)
-			}
-
-			vote, err = hash.Vote()
-			if err != nil {
-				return fmt.Errorf("computing vote: %w", err)
-			}
-		case os.IsNotExist(err):
-			vote = voting.VoteFromData([]byte("notfound"))
-		default:
-			return fmt.Errorf("open repo config: %w", err)
-		}
-
-		if err := s.txManager.Vote(ctx, tx, vote); err != nil {
-			return fmt.Errorf("casting vote: %w", err)
-		}
-
-		return nil
-	})
-}
diff --git a/internal/gitaly/service/repository/fullpath.go b/internal/gitaly/service/repository/fullpath.go
index 0fbe27878..d2fdbe86d 100644
--- a/internal/gitaly/service/repository/fullpath.go
+++ b/internal/gitaly/service/repository/fullpath.go
@@ -4,7 +4,6 @@ import (
 	"context"
 
 	"gitlab.com/gitlab-org/gitaly/v14/internal/helper"
-	"gitlab.com/gitlab-org/gitaly/v14/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/v14/proto/go/gitalypb"
 )
 
@@ -26,24 +25,8 @@ func (s *server) SetFullPath(
 
 	repo := s.localrepo(request.GetRepository())
 
-	if featureflag.TxFileLocking.IsEnabled(ctx) {
-		if err := repo.SetConfig(ctx, fullPathKey, request.GetPath(), s.txManager); err != nil {
-			return nil, helper.ErrInternalf("setting config: %w", err)
-		}
-
-		return &gitalypb.SetFullPathResponse{}, nil
-	}
-
-	if err := s.voteOnConfig(ctx, request.GetRepository()); err != nil {
-		return nil, helper.ErrInternalf("preimage vote on config: %w", err)
-	}
-
-	if err := repo.Config().Set(ctx, fullPathKey, request.GetPath()); err != nil {
-		return nil, helper.ErrInternalf("writing config: %w", err)
-	}
-
-	if err := s.voteOnConfig(ctx, request.GetRepository()); err != nil {
-		return nil, helper.ErrInternalf("postimage vote on config: %w", err)
+	if err := repo.SetConfig(ctx, fullPathKey, request.GetPath(), s.txManager); err != nil {
+		return nil, helper.ErrInternalf("setting config: %w", err)
 	}
 
 	return &gitalypb.SetFullPathResponse{}, nil
diff --git a/internal/gitaly/service/repository/fullpath_test.go b/internal/gitaly/service/repository/fullpath_test.go
index 1224e30fd..dea6d131e 100644
--- a/internal/gitaly/service/repository/fullpath_test.go
+++ b/internal/gitaly/service/repository/fullpath_test.go
@@ -1,7 +1,6 @@
 package repository
 
 import (
-	"context"
 	"fmt"
 	"os"
 	"path/filepath"
@@ -12,7 +11,6 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/config"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/helper"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/helper/text"
-	"gitlab.com/gitlab-org/gitaly/v14/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/testhelper"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/testhelper/testassert"
 	"gitlab.com/gitlab-org/gitaly/v14/proto/go/gitalypb"
@@ -21,12 +19,9 @@ import (
 func TestSetFullPath(t *testing.T) {
 	t.Parallel()
 
-	testhelper.NewFeatureSets([]featureflag.FeatureFlag{
-		featureflag.TxFileLocking,
-	}).Run(t, testSetFullPath)
-}
+	ctx, cancel := testhelper.Context()
+	defer cancel()
 
-func testSetFullPath(t *testing.T, ctx context.Context) {
 	cfg, client := setupRepositoryServiceWithoutRepo(t)
 
 	t.Run("missing repository", func(t *testing.T) {
@@ -79,12 +74,8 @@ func testSetFullPath(t *testing.T, ctx context.Context) {
 
 		require.Nil(t, response)
 
-		expectedErr := fmt.Sprintf("rpc error: code = NotFound desc = writing config: rpc "+
+		expectedErr := fmt.Sprintf("rpc error: code = NotFound desc = setting config: rpc "+
 			"error: code = NotFound desc = GetRepoPath: not a git repository: %q", repoPath)
-		if featureflag.TxFileLocking.IsEnabled(ctx) {
-			expectedErr = fmt.Sprintf("rpc error: code = NotFound desc = setting config: rpc "+
-				"error: code = NotFound desc = GetRepoPath: not a git repository: %q", repoPath)
-		}
 		require.EqualError(t, err, expectedErr)
 	})
 
diff --git a/internal/metadata/featureflag/ff_tx_file_locking.go b/internal/metadata/featureflag/ff_tx_file_locking.go
deleted file mode 100644
index 08f1c9f6c..000000000
--- a/internal/metadata/featureflag/ff_tx_file_locking.go
+++ /dev/null
@@ -1,5 +0,0 @@
-package featureflag
-
-// TxFileLocking enables two-phase voting on files with proper locking semantics such that no races
-// can exist anymore.
-var TxFileLocking = NewFeatureFlag("tx_file_locking", false)