operations: Skip precursory update of target ref in UserMergeToRef

UserMergeToRef performs three steps when computing a merge:

    1. The target reference is force-overwritten with whatever the
       first-parent-ref is currently pointing to.
    2. The merge is computed.
    3. The target reference gets updated with the resulting merge
       commit.

This sequence is problematic with transactions: while it is an expected
failure mode that the second step fails, we have already updated a
reference and thus cast a vote before returning an error. Because we
will always create a replication job if there's been at least one vote
and the primary returned an error, we'll thus do so and cause excessive
replication to occur even though it shouldn't be needed.

It's questionable whether the first step is by design or whether it
serves as a means against races. It certainly feels unexpected to me
that an RPC which fails with PreconditionFailed because of conflicts
still modifies references, and this behaviour is not documented either.

This commit goes with the assumption that the purpose of the current
code is to protect against races because computing the merge can take a
rather long time, and we don't want to overwrite any reference which has
changed while we were computing the merge. This can be implemented in a
much more graceful manner though by first reading the target reference,
computing the merge and then doing a git-update-ref(1) with the revision
we've just read as expected old value. This avoids doing the precursory
update of the reference and thus also casting the vote, and as a result
we now don't trigger replication anymore.

Given that we're operating based on assumptions here, this change is
implemented behind a feature flag. This flag only acts as an escape
hatch though and is thus default-enabled.

Changelog: changed

3 files changed, 87 insertions, 13 deletions

diff --git a/internal/gitaly/service/operations/merge.go b/internal/gitaly/service/operations/merge.go
index 7ab270821..424dba8ec 100644
--- a/internal/gitaly/service/operations/merge.go
+++ b/internal/gitaly/service/operations/merge.go
@@ -11,6 +11,7 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v14/internal/git"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/git2go"
 	"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"
 )
 
@@ -265,9 +266,39 @@ func (s *Server) UserMergeToRef(ctx context.Context, request *gitalypb.UserMerge
 		}
 	}
 
-	// First, overwrite the reference with the target reference.
-	if err := repo.UpdateRef(ctx, git.ReferenceName(request.TargetRef), oid, ""); err != nil {
-		return nil, updateRefError{reference: string(request.TargetRef)}
+	var oldTargetOID git.ObjectID
+	if featureflag.IsEnabled(ctx, featureflag.UserMergeToRefSkipPrecursorRefUpdate) {
+		// Resolve the current state of the target reference. We do not care whether it
+		// exists or not, but what we do want to assert is that the target reference doesn't
+		// change while we compute the merge commit as a small protection against races.
+		targetRef, err := repo.GetReference(ctx, git.ReferenceName(request.TargetRef))
+		if err == nil {
+			if targetRef.IsSymbolic {
+				return nil, helper.ErrPreconditionFailedf("target reference is symbolic: %q", request.TargetRef)
+			}
+
+			oid, err := git.NewObjectIDFromHex(targetRef.Target)
+			if err != nil {
+				return nil, helper.ErrInternalf("invalid target revision: %v", err)
+			}
+
+			oldTargetOID = oid
+		} else if errors.Is(err, git.ErrReferenceNotFound) {
+			oldTargetOID = git.ZeroOID
+		} else {
+			return nil, helper.ErrInternalf("could not read target reference: %v", err)
+		}
+	} else {
+		// This is the old code path which always force-updated the target reference before
+		// computing the merge. As a result, even if the merge failed, we'd have updated the
+		// reference to point to the first parent ref. It does feel unexpected that the
+		// target reference changes even if the merge itself fails. Furthermore, this is
+		// causing problems with transactions: if the merge fails, we have already voted
+		// once to update the target reference and will thus trigger a replication job.
+		if err := repo.UpdateRef(ctx, git.ReferenceName(request.TargetRef), oid, ""); err != nil {
+			return nil, updateRefError{reference: string(request.TargetRef)}
+		}
+		oldTargetOID = oid
 	}
 
 	// Now, we create the merge commit...
@@ -296,7 +327,7 @@ func (s *Server) UserMergeToRef(ctx context.Context, request *gitalypb.UserMerge
 
 	// ... and move branch from target ref to the merge commit. The Ruby
 	// implementation doesn't invoke hooks, so we don't either.
-	if err := repo.UpdateRef(ctx, git.ReferenceName(request.TargetRef), mergeOID, oid); err != nil {
+	if err := repo.UpdateRef(ctx, git.ReferenceName(request.TargetRef), mergeOID, oldTargetOID); err != nil {
 		//nolint:stylecheck
 		return nil, helper.ErrPreconditionFailed(fmt.Errorf("Could not update %s. Please refresh and try again", string(request.TargetRef)))
 	}
diff --git a/internal/gitaly/service/operations/merge_test.go b/internal/gitaly/service/operations/merge_test.go
index 8ac2b81e9..bd001ee66 100644
--- a/internal/gitaly/service/operations/merge_test.go
+++ b/internal/gitaly/service/operations/merge_test.go
@@ -17,6 +17,7 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v14/internal/git/localrepo"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/config"
 	"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/proto/go/gitalypb"
 	"google.golang.org/grpc/codes"
@@ -697,10 +698,11 @@ func TestConflictsOnUserMergeToRefRequest(t *testing.T) {
 	ctx, cleanup := testhelper.Context()
 	defer cleanup()
 
-	ctx, cfg, repo, repoPath, client := setupOperationsService(t, ctx)
+	ctx, cfg, repoProto, repoPath, client := setupOperationsService(t, ctx)
+	repo := localrepo.NewTestRepo(t, cfg, repoProto)
 
 	t.Run("allow conflicts to be merged with markers when modified on both sides", func(t *testing.T) {
-		request := buildUserMergeToRefRequest(t, cfg, repo, repoPath, "1450cd639e0bc6721eb02800169e464f212cde06", "824be604a34828eb682305f0d963056cfac87b2d", "modified-both-sides-conflict")
+		request := buildUserMergeToRefRequest(t, cfg, repoProto, repoPath, "1450cd639e0bc6721eb02800169e464f212cde06", "824be604a34828eb682305f0d963056cfac87b2d", "modified-both-sides-conflict")
 		request.AllowConflicts = true
 
 		resp, err := client.UserMergeToRef(ctx, request)
@@ -713,7 +715,7 @@ func TestConflictsOnUserMergeToRefRequest(t *testing.T) {
 	})
 
 	t.Run("allow conflicts to be merged with markers when modified on source and removed on target", func(t *testing.T) {
-		request := buildUserMergeToRefRequest(t, cfg, repo, repoPath, "eb227b3e214624708c474bdab7bde7afc17cefcc", "92417abf83b75e67b8ace920bc8e83e1986da4ac", "modified-source-removed-target-conflict")
+		request := buildUserMergeToRefRequest(t, cfg, repoProto, repoPath, "eb227b3e214624708c474bdab7bde7afc17cefcc", "92417abf83b75e67b8ace920bc8e83e1986da4ac", "modified-source-removed-target-conflict")
 		request.AllowConflicts = true
 
 		resp, err := client.UserMergeToRef(ctx, request)
@@ -726,7 +728,7 @@ func TestConflictsOnUserMergeToRefRequest(t *testing.T) {
 	})
 
 	t.Run("allow conflicts to be merged with markers when removed on source and modified on target", func(t *testing.T) {
-		request := buildUserMergeToRefRequest(t, cfg, repo, repoPath, "92417abf83b75e67b8ace920bc8e83e1986da4ac", "eb227b3e214624708c474bdab7bde7afc17cefcc", "removed-source-modified-target-conflict")
+		request := buildUserMergeToRefRequest(t, cfg, repoProto, repoPath, "92417abf83b75e67b8ace920bc8e83e1986da4ac", "eb227b3e214624708c474bdab7bde7afc17cefcc", "removed-source-modified-target-conflict")
 		request.AllowConflicts = true
 
 		resp, err := client.UserMergeToRef(ctx, request)
@@ -739,7 +741,7 @@ func TestConflictsOnUserMergeToRefRequest(t *testing.T) {
 	})
 
 	t.Run("allow conflicts to be merged with markers when both source and target added the same file", func(t *testing.T) {
-		request := buildUserMergeToRefRequest(t, cfg, repo, repoPath, "f0f390655872bb2772c85a0128b2fbc2d88670cb", "5b4bb08538b9249995b94aa69121365ba9d28082", "source-target-added-same-file-conflict")
+		request := buildUserMergeToRefRequest(t, cfg, repoProto, repoPath, "f0f390655872bb2772c85a0128b2fbc2d88670cb", "5b4bb08538b9249995b94aa69121365ba9d28082", "source-target-added-same-file-conflict")
 		request.AllowConflicts = true
 
 		resp, err := client.UserMergeToRef(ctx, request)
@@ -757,7 +759,7 @@ func TestConflictsOnUserMergeToRefRequest(t *testing.T) {
 	// to show the `Their` side when we present the merge commit on the merge
 	// request diff.
 	t.Run("allow conflicts to be merged without markers when renamed on source and removed on target", func(t *testing.T) {
-		request := buildUserMergeToRefRequest(t, cfg, repo, repoPath, "aafecf84d791ec43dfa16e55eb0a0fbd9c72d3fb", "3ac7abfb7621914e596d5bf369be8234b9086052", "renamed-source-removed-target")
+		request := buildUserMergeToRefRequest(t, cfg, repoProto, repoPath, "aafecf84d791ec43dfa16e55eb0a0fbd9c72d3fb", "3ac7abfb7621914e596d5bf369be8234b9086052", "renamed-source-removed-target")
 		request.AllowConflicts = true
 
 		resp, err := client.UserMergeToRef(ctx, request)
@@ -769,7 +771,7 @@ func TestConflictsOnUserMergeToRefRequest(t *testing.T) {
 	})
 
 	t.Run("allow conflicts to be merged without markers when removed on source and renamed on target", func(t *testing.T) {
-		request := buildUserMergeToRefRequest(t, cfg, repo, repoPath, "3ac7abfb7621914e596d5bf369be8234b9086052", "aafecf84d791ec43dfa16e55eb0a0fbd9c72d3fb", "removed-source-renamed-target")
+		request := buildUserMergeToRefRequest(t, cfg, repoProto, repoPath, "3ac7abfb7621914e596d5bf369be8234b9086052", "aafecf84d791ec43dfa16e55eb0a0fbd9c72d3fb", "removed-source-renamed-target")
 		request.AllowConflicts = true
 
 		resp, err := client.UserMergeToRef(ctx, request)
@@ -781,7 +783,7 @@ func TestConflictsOnUserMergeToRefRequest(t *testing.T) {
 	})
 
 	t.Run("allow conflicts to be merged without markers when both source and target renamed the same file", func(t *testing.T) {
-		request := buildUserMergeToRefRequest(t, cfg, repo, repoPath, "aafecf84d791ec43dfa16e55eb0a0fbd9c72d3fb", "fe6d6ff5812e7fb292168851dc0edfc6a0171909", "source-target-renamed-same-file")
+		request := buildUserMergeToRefRequest(t, cfg, repoProto, repoPath, "aafecf84d791ec43dfa16e55eb0a0fbd9c72d3fb", "fe6d6ff5812e7fb292168851dc0edfc6a0171909", "source-target-renamed-same-file")
 		request.AllowConflicts = true
 
 		resp, err := client.UserMergeToRef(ctx, request)
@@ -793,12 +795,49 @@ func TestConflictsOnUserMergeToRefRequest(t *testing.T) {
 	})
 
 	t.Run("disallow conflicts to be merged", func(t *testing.T) {
-		request := buildUserMergeToRefRequest(t, cfg, repo, repoPath, "1450cd639e0bc6721eb02800169e464f212cde06", "824be604a34828eb682305f0d963056cfac87b2d", "disallowed-conflicts")
+		request := buildUserMergeToRefRequest(t, cfg, repoProto, repoPath, "1450cd639e0bc6721eb02800169e464f212cde06", "824be604a34828eb682305f0d963056cfac87b2d", "disallowed-conflicts")
 		request.AllowConflicts = false
 
 		_, err := client.UserMergeToRef(ctx, request)
 		require.Equal(t, status.Error(codes.FailedPrecondition, "Failed to create merge commit for source_sha 1450cd639e0bc6721eb02800169e464f212cde06 and target_sha 824be604a34828eb682305f0d963056cfac87b2d at refs/merge-requests/x/written"), err)
 	})
+
+	targetRef := git.Revision("refs/merge-requests/foo")
+
+	t.Run("failing merge does not update target reference if skipping precursor update-ref", func(t *testing.T) {
+		ctx := featureflag.OutgoingCtxWithFeatureFlags(ctx, featureflag.UserMergeToRefSkipPrecursorRefUpdate)
+
+		request := buildUserMergeToRefRequest(t, cfg, repoProto, repoPath, "1450cd639e0bc6721eb02800169e464f212cde06", "824be604a34828eb682305f0d963056cfac87b2d", t.Name())
+		request.TargetRef = []byte(targetRef)
+
+		_, err := client.UserMergeToRef(ctx, request)
+		testhelper.RequireGrpcError(t, err, codes.FailedPrecondition)
+
+		hasRevision, err := repo.HasRevision(ctx, targetRef)
+		require.NoError(t, err)
+		require.False(t, hasRevision, "branch should not have been created")
+	})
+
+	t.Run("failing merge does update target reference if not skipping precursor update-ref", func(t *testing.T) {
+		ctx := featureflag.OutgoingCtxWithDisabledFeatureFlags(ctx, featureflag.UserMergeToRefSkipPrecursorRefUpdate)
+
+		request := buildUserMergeToRefRequest(t, cfg, repoProto, repoPath,
+			"1450cd639e0bc6721eb02800169e464f212cde06", "824be604a34828eb682305f0d963056cfac87b2d", t.Name())
+		request.TargetRef = []byte(targetRef)
+
+		_, err := client.UserMergeToRef(ctx, request)
+		testhelper.RequireGrpcError(t, err, codes.FailedPrecondition)
+
+		firstParentRevision, err := repo.ResolveRevision(ctx, git.Revision(request.FirstParentRef))
+		require.NoError(t, err)
+
+		// The previous implementation of UserMergeToRef would've written the first parent
+		// ref into the target reference before computing the merge. As a result, it exists
+		// even if the merge itself failed.
+		targetRevision, err := repo.ResolveRevision(ctx, targetRef)
+		require.NoError(t, err)
+		require.Equal(t, firstParentRevision, targetRevision)
+	})
 }
 
 func buildUserMergeToRefRequest(t testing.TB, cfg config.Cfg, repo *gitalypb.Repository, repoPath string, sourceSha string, targetSha string, mergeBranchName string) *gitalypb.UserMergeToRefRequest {
diff --git a/internal/metadata/featureflag/feature_flags.go b/internal/metadata/featureflag/feature_flags.go
index 5f844558d..38b8eb878 100644
--- a/internal/metadata/featureflag/feature_flags.go
+++ b/internal/metadata/featureflag/feature_flags.go
@@ -19,6 +19,9 @@ var (
 	TxConfig = FeatureFlag{Name: "tx_config", OnByDefault: false}
 	// TxRemote enables transactional voting for AddRemote and DeleteRemote.
 	TxRemote = FeatureFlag{Name: "tx_remote", OnByDefault: false}
+	// UserMergeToRefSkipPrecursorRefUpdate causes UserMergeToRef to not update the
+	// target reference in case computing the merge fails.
+	UserMergeToRefSkipPrecursorRefUpdate = FeatureFlag{Name: "user_merge_to_ref_skip_precursor_ref_update", OnByDefault: true}
 )
 
 // All includes all feature flags.
@@ -28,4 +31,5 @@ var All = []FeatureFlag{
 	FetchInternalRemoteErrors,
 	TxConfig,
 	TxRemote,
+	UserMergeToRefSkipPrecursorRefUpdate,
 }