objectpool: Always prune refs on fetches to fix D/F conflictspks-remove-fetch-into-object-pool-prune-refs-ff

With 018958fb1 (objectpool: Fix conflicting references when fetching
into pools, 2022-07-27) we have started to prune references in objetc
pools that have been removed in their primary pool member. This is a
necessity to avoid conflicting references in the object pool and its
member, which would otherwise break updating such pools completely.

This code has been rolled out to production on August 11th without any
observed negative fallout, and the flag was subsequently enabled by
default. Remove the feature flag guarding it.

Changelog: changed

4 files changed, 37 insertions, 125 deletions

diff --git a/internal/git/objectpool/fetch.go b/internal/git/objectpool/fetch.go
index 0b399dc13..7f3e72472 100644
--- a/internal/git/objectpool/fetch.go
+++ b/internal/git/objectpool/fetch.go
@@ -17,7 +17,6 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git/localrepo"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git/updateref"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/transaction"
-	"gitlab.com/gitlab-org/gitaly/v15/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/transaction/voting"
 )
 
@@ -58,10 +57,8 @@ func (o *ObjectPool) FetchFromOrigin(ctx context.Context, origin *localrepo.Repo
 	// `--atomic` and `--prune` together then it still wouldn't be able to recover from the D/F
 	// conflict. So we first to a preliminary prune that only prunes refs without fetching
 	// objects yet to avoid that scenario.
-	if featureflag.FetchIntoObjectPoolPruneRefs.IsEnabled(ctx) {
-		if err := o.pruneReferences(ctx, origin); err != nil {
-			return fmt.Errorf("pruning references: %w", err)
-		}
+	if err := o.pruneReferences(ctx, origin); err != nil {
+		return fmt.Errorf("pruning references: %w", err)
 	}
 
 	var stderr bytes.Buffer
diff --git a/internal/git/objectpool/fetch_test.go b/internal/git/objectpool/fetch_test.go
index 0a03970ec..7305697f0 100644
--- a/internal/git/objectpool/fetch_test.go
+++ b/internal/git/objectpool/fetch_test.go
@@ -3,7 +3,6 @@
 package objectpool
 
 import (
-	"context"
 	"fmt"
 	"os"
 	"path/filepath"
@@ -17,18 +16,13 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git/localrepo"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/config"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/helper/text"
-	"gitlab.com/gitlab-org/gitaly/v15/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper"
 )
 
 func TestFetchFromOrigin_dangling(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.FetchIntoObjectPoolPruneRefs).Run(t, testFetchFromOriginDangling)
-}
-
-func testFetchFromOriginDangling(t *testing.T, ctx context.Context) {
-	t.Parallel()
 
+	ctx := testhelper.Context(t)
 	cfg, pool, repoProto := setupObjectPool(t, ctx)
 	repo := localrepo.NewTestRepo(t, cfg, repoProto)
 
@@ -98,12 +92,8 @@ func testFetchFromOriginDangling(t *testing.T, ctx context.Context) {
 
 func TestFetchFromOrigin_fsck(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.FetchIntoObjectPoolPruneRefs).Run(t, testFetchFromOriginFsck)
-}
-
-func testFetchFromOriginFsck(t *testing.T, ctx context.Context) {
-	t.Parallel()
 
+	ctx := testhelper.Context(t)
 	cfg, pool, repoProto := setupObjectPool(t, ctx)
 	repo := localrepo.NewTestRepo(t, cfg, repoProto)
 	repoPath := filepath.Join(cfg.Storages[0].Path, repo.GetRelativePath())
@@ -127,12 +117,8 @@ func testFetchFromOriginFsck(t *testing.T, ctx context.Context) {
 
 func TestFetchFromOrigin_deltaIslands(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.FetchIntoObjectPoolPruneRefs).Run(t, testFetchFromOriginDeltaIslands)
-}
-
-func testFetchFromOriginDeltaIslands(t *testing.T, ctx context.Context) {
-	t.Parallel()
 
+	ctx := testhelper.Context(t)
 	cfg, pool, repoProto := setupObjectPool(t, ctx)
 
 	repo := localrepo.NewTestRepo(t, cfg, repoProto)
@@ -152,12 +138,8 @@ func testFetchFromOriginDeltaIslands(t *testing.T, ctx context.Context) {
 
 func TestFetchFromOrigin_bitmapHashCache(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.FetchIntoObjectPoolPruneRefs).Run(t, testFetchFromOriginBitmapHashCache)
-}
-
-func testFetchFromOriginBitmapHashCache(t *testing.T, ctx context.Context) {
-	t.Parallel()
 
+	ctx := testhelper.Context(t)
 	cfg, pool, repoProto := setupObjectPool(t, ctx)
 	repo := localrepo.NewTestRepo(t, cfg, repoProto)
 
@@ -182,12 +164,8 @@ func testFetchFromOriginBitmapHashCache(t *testing.T, ctx context.Context) {
 
 func TestFetchFromOrigin_refUpdates(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.FetchIntoObjectPoolPruneRefs).Run(t, testFetchFromOriginRefUpdates)
-}
-
-func testFetchFromOriginRefUpdates(t *testing.T, ctx context.Context) {
-	t.Parallel()
 
+	ctx := testhelper.Context(t)
 	cfg, pool, repoProto := setupObjectPool(t, ctx)
 	repo := localrepo.NewTestRepo(t, cfg, repoProto)
 	repoPath := filepath.Join(cfg.Storages[0].Path, repo.GetRelativePath())
@@ -239,12 +217,8 @@ func testFetchFromOriginRefUpdates(t *testing.T, ctx context.Context) {
 
 func TestFetchFromOrigin_refs(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.FetchIntoObjectPoolPruneRefs).Run(t, testFetchFromOriginRefs)
-}
-
-func testFetchFromOriginRefs(t *testing.T, ctx context.Context) {
-	t.Parallel()
 
+	ctx := testhelper.Context(t)
 	cfg, pool, _ := setupObjectPool(t, ctx)
 	poolPath := pool.FullPath()
 
diff --git a/internal/gitaly/service/objectpool/fetch_into_object_pool_test.go b/internal/gitaly/service/objectpool/fetch_into_object_pool_test.go
index 03262537b..582e7180e 100644
--- a/internal/gitaly/service/objectpool/fetch_into_object_pool_test.go
+++ b/internal/gitaly/service/objectpool/fetch_into_object_pool_test.go
@@ -22,9 +22,7 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git/housekeeping"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/config"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/transaction"
-	"gitlab.com/gitlab-org/gitaly/v15/internal/helper"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/metadata"
-	"gitlab.com/gitlab-org/gitaly/v15/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper/testcfg"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper/testserver"
@@ -40,12 +38,8 @@ import (
 
 func TestFetchIntoObjectPool_Success(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.FetchIntoObjectPoolPruneRefs).Run(t, testFetchIntoObjectPoolSuccess)
-}
-
-func testFetchIntoObjectPoolSuccess(t *testing.T, ctx context.Context) {
-	t.Parallel()
 
+	ctx := testhelper.Context(t)
 	cfg, repo, repoPath, locator, client := setup(ctx, t)
 
 	repoCommit := gittest.WriteCommit(t, cfg, repoPath, gittest.WithBranch(t.Name()))
@@ -98,11 +92,6 @@ func testFetchIntoObjectPoolSuccess(t *testing.T, ctx context.Context) {
 
 func TestFetchIntoObjectPool_transactional(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.FetchIntoObjectPoolPruneRefs).Run(t, testFetchIntoObjectPoolTransactional)
-}
-
-func testFetchIntoObjectPoolTransactional(t *testing.T, ctx context.Context) {
-	t.Parallel()
 
 	var votes []voting.Vote
 	var votesMutex sync.Mutex
@@ -115,6 +104,7 @@ func testFetchIntoObjectPoolTransactional(t *testing.T, ctx context.Context) {
 		},
 	}
 
+	ctx := testhelper.Context(t)
 	cfg := testcfg.Build(t)
 	cfg.SocketPath = runObjectPoolServer(
 		t, cfg, config.NewLocator(cfg),
@@ -163,17 +153,13 @@ func testFetchIntoObjectPoolTransactional(t *testing.T, ctx context.Context) {
 		})
 		require.NoError(t, err)
 
-		if featureflag.FetchIntoObjectPoolPruneRefs.IsEnabled(ctx) {
-			require.Equal(t, []voting.Vote{
-				// We expect to see two votes that demonstrate we're voting on no deleted
-				// references.
-				voting.VoteFromData(nil), voting.VoteFromData(nil),
-				// It is a bug though that we don't have a vote on the unchanged references
-				// in git-fetch(1).
-			}, votes)
-		} else {
-			require.Nil(t, votes)
-		}
+		require.Equal(t, []voting.Vote{
+			// We expect to see two votes that demonstrate we're voting on no deleted
+			// references.
+			voting.VoteFromData(nil), voting.VoteFromData(nil),
+			// It is a bug though that we don't have a vote on the unchanged references
+			// in git-fetch(1).
+		}, votes)
 	})
 
 	t.Run("with a new reference", func(t *testing.T) {
@@ -192,17 +178,13 @@ func testFetchIntoObjectPoolTransactional(t *testing.T, ctx context.Context) {
 		vote := voting.VoteFromData([]byte(fmt.Sprintf(
 			"%s %s refs/remotes/origin/heads/new-branch\n", git.ObjectHashSHA1.ZeroOID, repoCommit,
 		)))
-		if featureflag.FetchIntoObjectPoolPruneRefs.IsEnabled(ctx) {
-			require.Equal(t, []voting.Vote{
-				// The first two votes stem from the fact that we're voting on no
-				// deleted references.
-				voting.VoteFromData(nil), voting.VoteFromData(nil),
-				// And the other two votes are from the new branch we pull in.
-				vote, vote,
-			}, votes)
-		} else {
-			require.Equal(t, []voting.Vote{vote, vote}, votes)
-		}
+		require.Equal(t, []voting.Vote{
+			// The first two votes stem from the fact that we're voting on no
+			// deleted references.
+			voting.VoteFromData(nil), voting.VoteFromData(nil),
+			// And the other two votes are from the new branch we pull in.
+			vote, vote,
+		}, votes)
 	})
 
 	t.Run("with a stale reference in pool", func(t *testing.T) {
@@ -220,29 +202,20 @@ func testFetchIntoObjectPoolTransactional(t *testing.T, ctx context.Context) {
 		})
 		require.NoError(t, err)
 
-		if featureflag.FetchIntoObjectPoolPruneRefs.IsEnabled(ctx) {
-			// We expect a single vote on the reference we have deleted.
-			vote := voting.VoteFromData([]byte(fmt.Sprintf(
-				"%[1]s %[1]s %s\n", git.ObjectHashSHA1.ZeroOID, reference,
-			)))
-			require.Equal(t, []voting.Vote{vote, vote}, votes)
-		} else {
-			require.Nil(t, votes)
-		}
+		// We expect a single vote on the reference we have deleted.
+		vote := voting.VoteFromData([]byte(fmt.Sprintf(
+			"%[1]s %[1]s %s\n", git.ObjectHashSHA1.ZeroOID, reference,
+		)))
+		require.Equal(t, []voting.Vote{vote, vote}, votes)
 
 		exists, err := pool.Repo.HasRevision(ctx, git.Revision(reference))
 		require.NoError(t, err)
-		require.Equal(t, exists, featureflag.FetchIntoObjectPoolPruneRefs.IsDisabled(ctx))
+		require.False(t, exists)
 	})
 }
 
 func TestFetchIntoObjectPool_CollectLogStatistics(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.FetchIntoObjectPoolPruneRefs).Run(t, testFetchIntoObjectPoolCollectLogStatistics)
-}
-
-func testFetchIntoObjectPoolCollectLogStatistics(t *testing.T, ctx context.Context) {
-	t.Parallel()
 
 	cfg := testcfg.Build(t)
 	testcfg.BuildGitalyHooks(t, cfg)
@@ -252,6 +225,7 @@ func testFetchIntoObjectPoolCollectLogStatistics(t *testing.T, ctx context.Conte
 	logger, hook := test.NewNullLogger()
 	cfg.SocketPath = runObjectPoolServer(t, cfg, locator, logger)
 
+	ctx := testhelper.Context(t)
 	ctx = ctxlogrus.ToContext(ctx, log.WithField("test", "logging"))
 	repo, _ := gittest.CreateRepository(ctx, t, cfg, gittest.CreateRepositoryConfig{
 		Seed: gittest.SeedGitLabTest,
@@ -371,12 +345,8 @@ func TestFetchIntoObjectPool_Failure(t *testing.T) {
 
 func TestFetchIntoObjectPool_dfConflict(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.FetchIntoObjectPoolPruneRefs).Run(t, testFetchIntoObjectPoolDfConflict)
-}
-
-func testFetchIntoObjectPoolDfConflict(t *testing.T, ctx context.Context) {
-	t.Parallel()
 
+	ctx := testhelper.Context(t)
 	cfg, repo, repoPath, _, client := setup(ctx, t)
 
 	pool := initObjectPool(t, cfg, cfg.Storages[0])
@@ -401,15 +371,6 @@ func testFetchIntoObjectPoolDfConflict(t *testing.T, ctx context.Context) {
 	gittest.Exec(t, cfg, "-C", repoPath, "update-ref", "-d", "refs/heads/branch")
 	gittest.WriteCommit(t, cfg, repoPath, gittest.WithBranch("branch/conflict"))
 
-	gitVersion, err := gittest.NewCommandFactory(t, cfg).GitVersion(ctx)
-	require.NoError(t, err)
-
-	// Due to a bug in old Git versions we may get an unexpected exit status.
-	expectedExitStatus := 254
-	if !gitVersion.FlushesUpdaterefStatus() {
-		expectedExitStatus = 1
-	}
-
 	// Verify that we can still fetch into the object pool regardless of the D/F conflict. While
 	// it is not possible to store both references at the same time due to the conflict, we
 	// should know to delete the old conflicting reference and replace it with the new one.
@@ -417,20 +378,11 @@ func testFetchIntoObjectPoolDfConflict(t *testing.T, ctx context.Context) {
 		ObjectPool: pool.ToProto(),
 		Origin:     repo,
 	})
-	if featureflag.FetchIntoObjectPoolPruneRefs.IsEnabled(ctx) {
-		require.NoError(t, err)
+	require.NoError(t, err)
 
-		poolPath, err := config.NewLocator(cfg).GetRepoPath(gittest.RewrittenRepository(ctx, t, cfg, pool.ToProto().GetRepository()))
-		require.NoError(t, err)
+	poolPath, err := config.NewLocator(cfg).GetRepoPath(gittest.RewrittenRepository(ctx, t, cfg, pool.ToProto().GetRepository()))
+	require.NoError(t, err)
 
-		// Verify that the conflicting reference exists now.
-		gittest.Exec(t, cfg, "-C", poolPath, "rev-parse", "refs/remotes/origin/heads/branch/conflict")
-	} else {
-		// But right now it fails due to a bug.
-		testhelper.RequireGrpcError(t, helper.ErrInternalf(
-			"fetch into object pool: exit status %d, stderr: %q",
-			expectedExitStatus,
-			"error: cannot lock ref 'refs/remotes/origin/heads/branch/conflict': 'refs/remotes/origin/heads/branch' exists; cannot create 'refs/remotes/origin/heads/branch/conflict'\n",
-		), err)
-	}
+	// Verify that the conflicting reference exists now.
+	gittest.Exec(t, cfg, "-C", poolPath, "rev-parse", "refs/remotes/origin/heads/branch/conflict")
 }
diff --git a/internal/metadata/featureflag/ff_fetch_into_object_pool_prune_refs.go b/internal/metadata/featureflag/ff_fetch_into_object_pool_prune_refs.go
deleted file mode 100644
index 4f0a2b41b..000000000
--- a/internal/metadata/featureflag/ff_fetch_into_object_pool_prune_refs.go
+++ /dev/null
@@ -1,11 +0,0 @@
-package featureflag
-
-// FetchIntoObjectPoolPruneRefs enables pruning of references in object pools. This is required in
-// order to fix cases where updating pools doesn't work anymore due to preexisting references
-// conflicting with new references in the pool member.
-var FetchIntoObjectPoolPruneRefs = NewFeatureFlag(
-	"fetch_into_object_pool_prune_refs",
-	"v15.3.0",
-	"https://gitlab.com/gitlab-org/gitaly/-/issues/4394",
-	true,
-)