Merge branch 'pks-updateref-fix-locking-race' into 'master'

updateref: Fix indeterminate state when locking refs

Closes #3761 and #3702

See merge request gitlab-org/gitaly!3856

10 files changed, 443 insertions, 37 deletions

diff --git a/Makefile b/Makefile
index 044949f5e..b84894484 100644
--- a/Makefile
+++ b/Makefile
@@ -142,13 +142,23 @@ ifeq ($(origin GIT_PATCHES),undefined)
     GIT_PATCHES += 0013-fetch-merge-fetching-and-consuming-refs.patch
     GIT_PATCHES += 0014-fetch-avoid-second-connectivity-check-if-we-already-.patch
 
+    # git-update-ref(1) allows us to explicitly manage transactional state via
+    # a set of verbs "start", "prepare" and "commit", which we use in the
+    # git/updateref package. Due to a missing flush in git, the confirmation
+    # message that the state change has been executed couldn't be read by us
+    # though and thus we were not in a position to verify the transition. This
+    # is fixed by the following patch, which adds the missing flushes. This has
+    # been merged into next via 4ae19a5f34 (Merge branch
+    # 'ps/update-ref-batch-flush' into next, 2021-09-10).
+    GIT_PATCHES += 0015-update-ref-fix-streaming-of-status-updates.patch
+
     # This extra version has two intentions: first, it allows us to detect
     # capabilities of the command at runtime. Second, it helps admins to
     # discover which version is currently in use. As such, this version must be
     # incremented whenever a new patch is added above. When no patches exist,
     # then this should be undefined. Otherwise, it must be set to at least
     # `gl1` given that `0` is the "default" GitLab patch level.
-    GIT_EXTRA_VERSION := gl2
+    GIT_EXTRA_VERSION := gl3
 endif
 
 ifeq ($(origin GIT_BUILD_OPTIONS),undefined)
diff --git a/_support/git-patches/0015-update-ref-fix-streaming-of-status-updates.patch b/_support/git-patches/0015-update-ref-fix-streaming-of-status-updates.patch
new file mode 100644
index 000000000..ecdca4675
--- /dev/null
+++ b/_support/git-patches/0015-update-ref-fix-streaming-of-status-updates.patch
@@ -0,0 +1,124 @@
+From efa3d64ce86a600563c8bf909c46dd0985ee6c11 Mon Sep 17 00:00:00 2001
+Message-Id: <efa3d64ce86a600563c8bf909c46dd0985ee6c11.1631522930.git.ps@pks.im>
+From: Patrick Steinhardt <ps@pks.im>
+Date: Fri, 3 Sep 2021 11:06:37 +0200
+Subject: [PATCH] update-ref: fix streaming of status updates
+
+When executing git-update-ref(1) with the `--stdin` flag, then the user
+can queue updates and, since e48cf33b61 (update-ref: implement
+interactive transaction handling, 2020-04-02), interactively drive the
+transaction's state via a set of transactional verbs. This interactivity
+is somewhat broken though: while the caller can use these verbs to drive
+the transaction's state, the status messages which confirm that a verb
+has been processed is not flushed. The caller may thus be left hanging
+waiting for the acknowledgement.
+
+Fix the bug by flushing stdout after writing the status update. Add a
+test which catches this bug.
+
+Signed-off-by: Patrick Steinhardt <ps@pks.im>
+Signed-off-by: Junio C Hamano <gitster@pobox.com>
+---
+ builtin/update-ref.c  | 14 ++++++++++----
+ t/t1400-update-ref.sh | 32 ++++++++++++++++++++++++++++++++
+ 2 files changed, 42 insertions(+), 4 deletions(-)
+
+diff --git a/builtin/update-ref.c b/builtin/update-ref.c
+index 6029a80544..a84e7b47a2 100644
+--- a/builtin/update-ref.c
++++ b/builtin/update-ref.c
+@@ -302,6 +302,12 @@ static void parse_cmd_verify(struct ref_transaction *transaction,
+ 	strbuf_release(&err);
+ }
+ 
++static void report_ok(const char *command)
++{
++	fprintf(stdout, "%s: ok\n", command);
++	fflush(stdout);
++}
++
+ static void parse_cmd_option(struct ref_transaction *transaction,
+ 			     const char *next, const char *end)
+ {
+@@ -317,7 +323,7 @@ static void parse_cmd_start(struct ref_transaction *transaction,
+ {
+ 	if (*next != line_termination)
+ 		die("start: extra input: %s", next);
+-	puts("start: ok");
++	report_ok("start");
+ }
+ 
+ static void parse_cmd_prepare(struct ref_transaction *transaction,
+@@ -328,7 +334,7 @@ static void parse_cmd_prepare(struct ref_transaction *transaction,
+ 		die("prepare: extra input: %s", next);
+ 	if (ref_transaction_prepare(transaction, &error))
+ 		die("prepare: %s", error.buf);
+-	puts("prepare: ok");
++	report_ok("prepare");
+ }
+ 
+ static void parse_cmd_abort(struct ref_transaction *transaction,
+@@ -339,7 +345,7 @@ static void parse_cmd_abort(struct ref_transaction *transaction,
+ 		die("abort: extra input: %s", next);
+ 	if (ref_transaction_abort(transaction, &error))
+ 		die("abort: %s", error.buf);
+-	puts("abort: ok");
++	report_ok("abort");
+ }
+ 
+ static void parse_cmd_commit(struct ref_transaction *transaction,
+@@ -350,7 +356,7 @@ static void parse_cmd_commit(struct ref_transaction *transaction,
+ 		die("commit: extra input: %s", next);
+ 	if (ref_transaction_commit(transaction, &error))
+ 		die("commit: %s", error.buf);
+-	puts("commit: ok");
++	report_ok("commit");
+ 	ref_transaction_free(transaction);
+ }
+ 
+diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
+index 4506cd435b..1e754e258f 100755
+--- a/t/t1400-update-ref.sh
++++ b/t/t1400-update-ref.sh
+@@ -1598,6 +1598,38 @@ test_expect_success 'transaction cannot restart ongoing transaction' '
+ 	test_must_fail git show-ref --verify refs/heads/restart
+ '
+ 
++test_expect_success PIPE 'transaction flushes status updates' '
++	mkfifo in out &&
++	(git update-ref --stdin <in >out &) &&
++
++	exec 9>in &&
++	test_when_finished "exec 9>&-" &&
++
++	echo "start" >&9 &&
++	echo "start: ok" >expected &&
++	read line <out &&
++	echo "$line" >actual &&
++	test_cmp expected actual &&
++
++	echo "create refs/heads/flush $A" >&9 &&
++
++	echo prepare >&9 &&
++	echo "prepare: ok" >expected &&
++	read line <out &&
++	echo "$line" >actual &&
++	test_cmp expected actual &&
++
++	# This must now fail given that we have locked the ref.
++	test_must_fail git update-ref refs/heads/flush $B 2>stderr &&
++	grep "fatal: update_ref failed for ref ${SQ}refs/heads/flush${SQ}: cannot lock ref" stderr &&
++
++	echo commit >&9 &&
++	echo "commit: ok" >expected &&
++	read line <out &&
++	echo "$line" >actual &&
++	test_cmp expected actual
++'
++
+ test_expect_success 'directory not created deleting packed ref' '
+ 	git branch d1/d2/r1 HEAD &&
+ 	git pack-refs --all &&
+-- 
+2.33.0
+
diff --git a/internal/git/updateref/update_with_hooks.go b/internal/git/updateref/update_with_hooks.go
index 22a943495..a1b04aa56 100644
--- a/internal/git/updateref/update_with_hooks.go
+++ b/internal/git/updateref/update_with_hooks.go
@@ -178,7 +178,7 @@ func (u *UpdaterWithHooks) UpdateReference(
 	// We need to lock the reference before executing the reference-transaction hook such that
 	// there cannot be any concurrent modification.
 	if err := updater.Prepare(); err != nil {
-		return fmt.Errorf("preparing ref update: %w", err)
+		return Error{reference: reference.String()}
 	}
 	// We need to explicitly cancel the update here such that we release the lock when this
 	// function exits if there is any error between locking and committing.
diff --git a/internal/git/updateref/updateref.go b/internal/git/updateref/updateref.go
index c3eca46bf..dde7b5b76 100644
--- a/internal/git/updateref/updateref.go
+++ b/internal/git/updateref/updateref.go
@@ -1,6 +1,7 @@
 package updateref
 
 import (
+	"bufio"
 	"bytes"
 	"context"
 	"fmt"
@@ -8,6 +9,7 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v14/internal/command"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/git"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/config"
+	"gitlab.com/gitlab-org/gitaly/v14/internal/metadata/featureflag"
 )
 
 // Updater wraps a `git update-ref --stdin` process, presenting an interface
@@ -16,7 +18,12 @@ import (
 type Updater struct {
 	repo   git.RepositoryExecutor
 	cmd    *command.Command
+	stdout *bufio.Reader
 	stderr *bytes.Buffer
+
+	// withStatusFlushing determines whether the Git version used supports proper flushing of
+	// status messages.
+	withStatusFlushing bool
 }
 
 // UpdaterOpt is a type representing options for the Updater.
@@ -65,10 +72,22 @@ func New(ctx context.Context, conf config.Cfg, repo git.RepositoryExecutor, opts
 		return nil, err
 	}
 
+	withStatusFlushing := false
+	if featureflag.UpdaterefVerifyStateChanges.IsEnabled(ctx) {
+		gitVersion, err := git.CurrentVersionForExecutor(ctx, repo)
+		if err != nil {
+			return nil, fmt.Errorf("determining git version: %w", err)
+		}
+
+		withStatusFlushing = gitVersion.FlushesUpdaterefStatus()
+	}
+
 	updater := &Updater{
-		repo:   repo,
-		cmd:    cmd,
-		stderr: &stderr,
+		repo:               repo,
+		cmd:                cmd,
+		stderr:             &stderr,
+		stdout:             bufio.NewReader(cmd),
+		withStatusFlushing: withStatusFlushing,
 	}
 
 	// By writing an explicit "start" to the command, we enable
@@ -134,16 +153,30 @@ func (u *Updater) Cancel() error {
 func (u *Updater) setState(state string) error {
 	_, err := fmt.Fprintf(u.cmd, "%s\x00", state)
 	if err != nil {
-		return fmt.Errorf("updating state to %s: %w", state, err)
+		return fmt.Errorf("updating state to %q: %w", state, err)
 	}
 
 	// For each state-changing command, git-update-ref(1) will report successful execution via
 	// "<command>: ok" lines printed to its stdout. Ideally, we should thus verify here whether
 	// the command was successfully executed by checking for exactly this line, otherwise we
 	// cannot be sure whether the command has correctly been processed by Git or if an error was
-	// raised. Unfortunately, Git won't flush the output though, and as such we would be left
-	// waiting for the message to appear here. This needs to be fixed upstream first, and after
-	// this we should adapt this function to assert commands.
+	// raised. Unfortunately, Git only knows to flush these reports either starting with v2.34.0
+	// or with our backported version v2.33.0.gl3.
+	if u.withStatusFlushing {
+		line, err := u.stdout.ReadString('\n')
+		if err != nil {
+			// We need to explicitly cancel the command here and wait for it to
+			// terminate such that we can retrieve the command's stderr in a race-free
+			// manner.
+			_ = u.Cancel()
+
+			return fmt.Errorf("state update to %q failed: %w, stderr: %q", state, err, u.stderr)
+		}
+
+		if line != fmt.Sprintf("%s: ok\n", state) {
+			return fmt.Errorf("state update to %q not successful: expected ok, got %q", state, line)
+		}
+	}
 
 	return nil
 }
diff --git a/internal/git/updateref/updateref_test.go b/internal/git/updateref/updateref_test.go
index 89ab269f4..aea0de5cb 100644
--- a/internal/git/updateref/updateref_test.go
+++ b/internal/git/updateref/updateref_test.go
@@ -12,6 +12,7 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v14/internal/git/hooks"
 	"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/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/testhelper"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/testhelper/testcfg"
 )
@@ -133,6 +134,46 @@ func TestUpdater_prepareLocksTransaction(t *testing.T) {
 	require.Contains(t, err.Error(), "fatal: prepared transactions can only be closed")
 }
 
+func TestUpdater_concurrentLocking(t *testing.T) {
+	testhelper.NewFeatureSets([]featureflag.FeatureFlag{
+		featureflag.UpdaterefVerifyStateChanges,
+	}).Run(t, testUpdaterConcurrentLocking)
+}
+
+func testUpdaterConcurrentLocking(t *testing.T, ctx context.Context) {
+	cfg, protoRepo, _ := testcfg.BuildWithRepo(t)
+	repo := localrepo.NewTestRepo(t, cfg, protoRepo)
+
+	commit, logErr := repo.ReadCommit(ctx, "refs/heads/master")
+	require.NoError(t, logErr)
+
+	firstUpdater, err := New(ctx, cfg, repo)
+	require.NoError(t, err)
+	require.NoError(t, firstUpdater.Update("refs/heads/master", "", commit.Id))
+	require.NoError(t, firstUpdater.Prepare())
+
+	secondUpdater, err := New(ctx, cfg, repo)
+	require.NoError(t, err)
+	require.NoError(t, secondUpdater.Update("refs/heads/master", "", commit.Id))
+
+	// With flushing, we're able to detect concurrent locking at prepare time already instead of
+	// at commit time.
+	if gitSupportsStatusFlushing(t, ctx, cfg) && featureflag.UpdaterefVerifyStateChanges.IsEnabled(ctx) {
+		err := secondUpdater.Prepare()
+		require.Error(t, err)
+		require.Contains(t, err.Error(), "state update to \"prepare\" failed: EOF, stderr: \"warning: update refs/heads/master: missing <newvalue>, treating as zero\\nfatal: prepare: cannot lock ref 'refs/heads/master'")
+
+		require.NoError(t, firstUpdater.Commit())
+	} else {
+		require.NoError(t, secondUpdater.Prepare())
+		require.NoError(t, firstUpdater.Commit())
+
+		err := secondUpdater.Commit()
+		require.Error(t, err)
+		require.Contains(t, err.Error(), "git update-ref: exit status 128, stderr: \"warning: update refs/heads/master: missing <newvalue>, treating as zero\\nfatal: prepare: cannot lock ref 'refs/heads/master'")
+	}
+}
+
 func TestBulkOperation(t *testing.T) {
 	ctx, cancel := testhelper.Context()
 	defer cancel()
@@ -235,10 +276,13 @@ func TestUpdater_closingStdinAbortsChanges(t *testing.T) {
 }
 
 func TestUpdater_capturesStderr(t *testing.T) {
-	ctx, cancel := testhelper.Context()
-	defer cancel()
+	testhelper.NewFeatureSets([]featureflag.FeatureFlag{
+		featureflag.UpdaterefVerifyStateChanges,
+	}).Run(t, testUpdaterCapturesStderr)
+}
 
-	_, _, updater := setupUpdater(t, ctx)
+func testUpdaterCapturesStderr(t *testing.T, ctx context.Context) {
+	cfg, _, updater := setupUpdater(t, ctx)
 
 	ref := "refs/heads/a"
 	newValue := strings.Repeat("1", 40)
@@ -246,11 +290,23 @@ func TestUpdater_capturesStderr(t *testing.T) {
 
 	require.NoError(t, updater.Update(git.ReferenceName(ref), newValue, oldValue))
 
-	expectedErr := fmt.Sprintf("git update-ref: exit status 128, stderr: "+
-		"\"fatal: commit: cannot update ref '%s': "+
-		"trying to write ref '%s' with nonexistent object %s\\n\"", ref, ref, newValue)
+	var expectedErr string
+	if gitSupportsStatusFlushing(t, ctx, cfg) && featureflag.UpdaterefVerifyStateChanges.IsEnabled(ctx) {
+		expectedErr = fmt.Sprintf("state update to \"commit\" failed: EOF, stderr: \"fatal: commit: cannot update ref '%s': "+
+			"trying to write ref '%s' with nonexistent object %s\\n\"", ref, ref, newValue)
+	} else {
+		expectedErr = fmt.Sprintf("git update-ref: exit status 128, stderr: "+
+			"\"fatal: commit: cannot update ref '%s': "+
+			"trying to write ref '%s' with nonexistent object %s\\n\"", ref, ref, newValue)
+	}
 
 	err := updater.Commit()
 	require.NotNil(t, err)
 	require.Equal(t, err.Error(), expectedErr)
 }
+
+func gitSupportsStatusFlushing(t *testing.T, ctx context.Context, cfg config.Cfg) bool {
+	version, err := git.CurrentVersion(ctx, git.NewExecCommandFactory(cfg))
+	require.NoError(t, err)
+	return version.FlushesUpdaterefStatus()
+}
diff --git a/internal/git/version.go b/internal/git/version.go
index c1f285767..dddf152ee 100644
--- a/internal/git/version.go
+++ b/internal/git/version.go
@@ -1,11 +1,13 @@
 package git
 
 import (
-	"bytes"
 	"context"
 	"fmt"
+	"io/ioutil"
 	"strconv"
 	"strings"
+
+	"gitlab.com/gitlab-org/gitaly/v14/internal/command"
 )
 
 // minimumVersion is the minimum required Git version. If updating this version, be sure to
@@ -39,22 +41,42 @@ type Version struct {
 
 // CurrentVersion returns the used git version.
 func CurrentVersion(ctx context.Context, gitCmdFactory CommandFactory) (Version, error) {
-	var buf bytes.Buffer
 	cmd, err := gitCmdFactory.NewWithoutRepo(ctx, SubCmd{
 		Name: "version",
-	}, WithStdout(&buf))
+	})
+	if err != nil {
+		return Version{}, fmt.Errorf("spawning version command: %w", err)
+	}
+
+	return parseVersionFromCommand(cmd)
+}
+
+// CurrentVersionForExecutor returns the git version used by the given executor.
+func CurrentVersionForExecutor(ctx context.Context, executor RepositoryExecutor) (Version, error) {
+	cmd, err := executor.Exec(ctx, SubCmd{
+		Name: "version",
+	})
 	if err != nil {
-		return Version{}, err
+		return Version{}, fmt.Errorf("spawning version command: %w", err)
 	}
 
-	if err = cmd.Wait(); err != nil {
-		return Version{}, err
+	return parseVersionFromCommand(cmd)
+}
+
+func parseVersionFromCommand(cmd *command.Command) (Version, error) {
+	versionOutput, err := ioutil.ReadAll(cmd)
+	if err != nil {
+		return Version{}, fmt.Errorf("reading version output: %w", err)
 	}
 
-	out := strings.Trim(buf.String(), " \n")
-	versionString := strings.SplitN(out, " ", 3)
+	if err := cmd.Wait(); err != nil {
+		return Version{}, fmt.Errorf("waiting for version command: %w", err)
+	}
+
+	trimmedVersionOutput := strings.Trim(string(versionOutput), " \n")
+	versionString := strings.SplitN(trimmedVersionOutput, " ", 3)
 	if len(versionString) != 3 {
-		return Version{}, fmt.Errorf("invalid version format: %q", buf.String())
+		return Version{}, fmt.Errorf("invalid version format: %q", string(versionOutput))
 	}
 
 	version, err := parseVersion(versionString[2])
@@ -82,6 +104,23 @@ func (v Version) SupportsObjectTypeFilter() bool {
 	return !v.LessThan(Version{major: 2, minor: 32, patch: 0})
 }
 
+// FlushesUpdaterefStatus determines whether the given Git version properly flushes status messages
+// in git-update-ref(1).
+func (v Version) FlushesUpdaterefStatus() bool {
+	// We need to be a bit more careful here given that this comes in via a custom patch. The
+	// fix will be released as part of v2.34, so it's either in v2.33 with at least patch level
+	// 3, or it's greater than or equal to v2.34.
+	switch {
+	case v.major == 2 && v.minor == 33 && v.gl >= 3:
+		return true
+	case v.major == 2 && v.minor >= 34:
+		return true
+	case v.major >= 3:
+		return true
+	}
+	return false
+}
+
 // LessThan determines whether the version is older than another version.
 func (v Version) LessThan(other Version) bool {
 	switch {
diff --git a/internal/git/version_test.go b/internal/git/version_test.go
index 681d8e03a..a452c0f50 100644
--- a/internal/git/version_test.go
+++ b/internal/git/version_test.go
@@ -1,12 +1,131 @@
 package git
 
 import (
+	"context"
 	"fmt"
+	"os/exec"
 	"testing"
 
 	"github.com/stretchr/testify/require"
+	"gitlab.com/gitlab-org/gitaly/v14/internal/command"
+	"gitlab.com/gitlab-org/gitaly/v14/internal/testhelper"
 )
 
+type versionGitCommandFactory struct {
+	CommandFactory
+
+	t       *testing.T
+	version string
+}
+
+func newVersionGitCommandFactory(t *testing.T, version string) *versionGitCommandFactory {
+	return &versionGitCommandFactory{
+		t:       t,
+		version: version,
+	}
+}
+
+func (f *versionGitCommandFactory) NewWithoutRepo(ctx context.Context, subcmd Cmd, opts ...CmdOpt) (*command.Command, error) {
+	f.t.Helper()
+
+	require.Equal(f.t, SubCmd{
+		Name: "version",
+	}, subcmd)
+	require.Len(f.t, opts, 0)
+
+	cmd, err := command.New(ctx, exec.Command("/usr/bin/env", "echo", f.version), nil, nil, nil)
+	require.NoError(f.t, err)
+
+	return cmd, nil
+}
+
+type versionRepositoryExecutor struct {
+	RepositoryExecutor
+
+	t       *testing.T
+	version string
+}
+
+func newVersionRepositoryExecutor(t *testing.T, version string) *versionRepositoryExecutor {
+	return &versionRepositoryExecutor{
+		t:       t,
+		version: version,
+	}
+}
+
+func (e *versionRepositoryExecutor) Exec(ctx context.Context, subcmd Cmd, opts ...CmdOpt) (*command.Command, error) {
+	e.t.Helper()
+
+	require.Equal(e.t, SubCmd{
+		Name: "version",
+	}, subcmd)
+	require.Len(e.t, opts, 0)
+
+	cmd, err := command.New(ctx, exec.Command("/usr/bin/env", "echo", e.version), nil, nil, nil)
+	require.NoError(e.t, err)
+
+	return cmd, nil
+}
+
+func TestCurrentVersion(t *testing.T) {
+	ctx, cancel := testhelper.Context()
+	defer cancel()
+
+	for _, tc := range []struct {
+		desc            string
+		versionString   string
+		expectedErr     string
+		expectedVersion Version
+	}{
+		{
+			desc:          "valid version",
+			versionString: "git version 2.33.1.gl1",
+			expectedVersion: Version{
+				versionString: "2.33.1.gl1", major: 2, minor: 33, patch: 1, gl: 1,
+			},
+		},
+		{
+			desc:          "valid version with trailing newline",
+			versionString: "git version 2.33.1.gl1\n",
+			expectedVersion: Version{
+				versionString: "2.33.1.gl1", major: 2, minor: 33, patch: 1, gl: 1,
+			},
+		},
+		{
+			desc:          "multi-line version",
+			versionString: "git version 2.33.1.gl1\nfoobar\n",
+			expectedErr:   "cannot parse git version: strconv.ParseUint: parsing \"1\\nfoobar\": invalid syntax",
+		},
+		{
+			desc:          "unexpected format",
+			versionString: "2.33.1\n",
+			expectedErr:   "invalid version format: \"2.33.1\\n\\n\"",
+		},
+	} {
+		t.Run(tc.desc, func(t *testing.T) {
+			t.Run("command factory", func(t *testing.T) {
+				actualVersion, err := CurrentVersion(ctx, newVersionGitCommandFactory(t, tc.versionString))
+				if tc.expectedErr == "" {
+					require.NoError(t, err)
+				} else {
+					require.EqualError(t, err, tc.expectedErr)
+				}
+				require.Equal(t, tc.expectedVersion, actualVersion)
+			})
+
+			t.Run("repository executor", func(t *testing.T) {
+				actualVersion, err := CurrentVersionForExecutor(ctx, newVersionRepositoryExecutor(t, tc.versionString))
+				if tc.expectedErr == "" {
+					require.NoError(t, err)
+				} else {
+					require.EqualError(t, err, tc.expectedErr)
+				}
+				require.Equal(t, tc.expectedVersion, actualVersion)
+			})
+		})
+	}
+}
+
 func TestVersion_LessThan(t *testing.T) {
 	for _, tc := range []struct {
 		smaller, larger string
@@ -128,6 +247,25 @@ func TestVersion_SupportsObjectTypeFilter(t *testing.T) {
 		version string
 		expect  bool
 	}{
+		{"2.32.0.gl3", false},
+		{"2.33.0.gl3", true},
+		{"2.33.1.gl3", true},
+		{"2.34.0", true},
+		{"3.0.0", true},
+	} {
+		t.Run(tc.version, func(t *testing.T) {
+			version, err := parseVersion(tc.version)
+			require.NoError(t, err)
+			require.Equal(t, tc.expect, version.FlushesUpdaterefStatus())
+		})
+	}
+}
+
+func TestVersion_FlushesUpdaterefStatus(t *testing.T) {
+	for _, tc := range []struct {
+		version string
+		expect  bool
+	}{
 		{"2.31.0", false},
 		{"2.31.0-rc0", false},
 		{"2.31.1", false},
diff --git a/internal/gitaly/server/auth_test.go b/internal/gitaly/server/auth_test.go
index 027436ab4..94f5e9ef5 100644
--- a/internal/gitaly/server/auth_test.go
+++ b/internal/gitaly/server/auth_test.go
@@ -328,17 +328,6 @@ func TestAuthBeforeLimit(t *testing.T) {
 	ctx, cancel := testhelper.Context()
 	defer cancel()
 
-	targetRevision := "c7fbe50c7c7419d9701eebe64b1fdacc3df5b9dd"
-	inputTagName := "to-be-créated-soon"
-
-	request := &gitalypb.UserCreateTagRequest{
-		Repository:     repo,
-		TagName:        []byte(inputTagName),
-		TargetRevision: []byte(targetRevision),
-		User:           gittest.TestUser,
-		Message:        []byte("a new tag!"),
-	}
-
 	defer func(d time.Duration) {
 		gitalyauth.SetTokenValidityDuration(d)
 	}(gitalyauth.TokenValidityDuration())
@@ -351,8 +340,15 @@ sleep %vs
 	errChan := make(chan error)
 
 	for i := 0; i < 2; i++ {
+		i := i
 		go func() {
-			_, err := client.UserCreateTag(ctx, request)
+			_, err := client.UserCreateTag(ctx, &gitalypb.UserCreateTagRequest{
+				Repository:     repo,
+				TagName:        []byte(fmt.Sprintf("tag-name-%d", i)),
+				TargetRevision: []byte("c7fbe50c7c7419d9701eebe64b1fdacc3df5b9dd"),
+				User:           gittest.TestUser,
+				Message:        []byte("a new tag!"),
+			})
 			errChan <- err
 		}()
 	}
diff --git a/internal/gitaly/service/ref/delete_refs.go b/internal/gitaly/service/ref/delete_refs.go
index e3720a7ae..715dfa47f 100644
--- a/internal/gitaly/service/ref/delete_refs.go
+++ b/internal/gitaly/service/ref/delete_refs.go
@@ -49,7 +49,9 @@ func (s *server) DeleteRefs(ctx context.Context, in *gitalypb.DeleteRefsRequest)
 	}
 
 	if err := updater.Prepare(); err != nil {
-		return nil, helper.ErrInternalf("could not prepare ref update: %v", err)
+		return &gitalypb.DeleteRefsResponse{
+			GitError: fmt.Sprintf("unable to delete refs: %s", err.Error()),
+		}, nil
 	}
 
 	vote, err := voteHash.Vote()
@@ -66,6 +68,10 @@ func (s *server) DeleteRefs(ctx context.Context, in *gitalypb.DeleteRefsRequest)
 	}
 
 	if err := updater.Commit(); err != nil {
+		// TODO: We should be able to easily drop this error here and return a real error as
+		// soon as we always use proper locking semantics in git-update-ref(1). All locking
+		// issues would be catched when `Prepare()`ing the changes already, so a fail
+		// afterwards would hint at a real unexpected error.
 		return &gitalypb.DeleteRefsResponse{GitError: fmt.Sprintf("unable to delete refs: %s", err.Error())}, nil
 	}
 
diff --git a/internal/metadata/featureflag/ff_updateref_assert_locking.go b/internal/metadata/featureflag/ff_updateref_assert_locking.go
new file mode 100644
index 000000000..e018fd1e6
--- /dev/null
+++ b/internal/metadata/featureflag/ff_updateref_assert_locking.go
@@ -0,0 +1,4 @@
+package featureflag
+
+// UpdaterefVerifyStateChanges causes us to assert that transactional state changes finish correctly.
+var UpdaterefVerifyStateChanges = NewFeatureFlag("updateref_verify_state_changes", true)