diff options
author | James Fargher <proglottis@gmail.com> | 2021-09-15 03:01:11 +0300 |
---|---|---|
committer | James Fargher <proglottis@gmail.com> | 2021-09-15 03:01:11 +0300 |
commit | ccd8ea3e1436d6464ac5f67e6bebd1ae317f4d50 (patch) | |
tree | a28ceef0015db2e2272dca7dc606672b434f017b | |
parent | 0a167388f2d382e43b354f318199fe2139c1de47 (diff) | |
parent | 4abf27024d6411d535d2e71526b9f682526e9c68 (diff) |
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
-rw-r--r-- | Makefile | 12 | ||||
-rw-r--r-- | _support/git-patches/0015-update-ref-fix-streaming-of-status-updates.patch | 124 | ||||
-rw-r--r-- | internal/git/updateref/update_with_hooks.go | 2 | ||||
-rw-r--r-- | internal/git/updateref/updateref.go | 47 | ||||
-rw-r--r-- | internal/git/updateref/updateref_test.go | 68 | ||||
-rw-r--r-- | internal/git/version.go | 57 | ||||
-rw-r--r-- | internal/git/version_test.go | 138 | ||||
-rw-r--r-- | internal/gitaly/server/auth_test.go | 20 | ||||
-rw-r--r-- | internal/gitaly/service/ref/delete_refs.go | 8 | ||||
-rw-r--r-- | internal/metadata/featureflag/ff_updateref_assert_locking.go | 4 |
10 files changed, 443 insertions, 37 deletions
@@ -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) |