objectpool: Revert error if object does not exist on fetch

In 3bfe68576 (objectpool: Raise error if object pool does not exist on
fetch, 2022-10-26), we have changed `FetchIntoObjectPool()` to raise an
error if the object pool didn't exist yet instead of silently creating
it. This fixes a bug in Praefect, which needs to know about all RPCs
which create a repository in order to record the new repository.

This change unfortunately breaks some tests in Rails. So let's revert it
for the time being so that we ca first amend these tests.

Changelog: fixed

4 files changed, 14 insertions, 105 deletions

diff --git a/internal/git/objectpool/fetch.go b/internal/git/objectpool/fetch.go
index 6cc0e19a7..7f3e72472 100644
--- a/internal/git/objectpool/fetch.go
+++ b/internal/git/objectpool/fetch.go
@@ -17,8 +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/helper"
-	"gitlab.com/gitlab-org/gitaly/v15/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/transaction/voting"
 )
 
@@ -26,14 +24,8 @@ var objectPoolRefspec = fmt.Sprintf("+refs/*:%s/*", git.ObjectPoolRefNamespace)
 
 // FetchFromOrigin initializes the pool and fetches the objects from its origin repository
 func (o *ObjectPool) FetchFromOrigin(ctx context.Context, origin *localrepo.Repo) error {
-	if featureflag.ObjectPoolDontInitOnFetch.IsDisabled(ctx) {
-		if err := o.Init(ctx); err != nil {
-			return fmt.Errorf("initializing object pool: %w", err)
-		}
-	} else {
-		if !o.Exists() {
-			return helper.ErrInvalidArgumentf("object pool does not exist")
-		}
+	if err := o.Init(ctx); err != nil {
+		return fmt.Errorf("initializing object pool: %w", err)
 	}
 
 	originPath, err := origin.Path()
diff --git a/internal/git/objectpool/fetch_test.go b/internal/git/objectpool/fetch_test.go
index c0c4f17ed..fe0a32d57 100644
--- a/internal/git/objectpool/fetch_test.go
+++ b/internal/git/objectpool/fetch_test.go
@@ -3,7 +3,6 @@
 package objectpool
 
 import (
-	"context"
 	"fmt"
 	"path/filepath"
 	"strconv"
@@ -14,23 +13,18 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git/gittest"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git/localrepo"
-	"gitlab.com/gitlab-org/gitaly/v15/internal/helper"
 	"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.ObjectPoolDontInitOnFetch).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)
 	repoPath, err := repo.Path()
+
 	require.NoError(t, err)
 
 	// Write some reachable objects into the object pool member and fetch them into the pool.
@@ -42,7 +36,6 @@ func testFetchFromOriginDangling(t *testing.T, ctx context.Context) {
 		gittest.WithTree(treeID),
 		gittest.WithBranch("master"),
 	)
-	require.NoError(t, pool.Init(ctx))
 	require.NoError(t, pool.FetchFromOrigin(ctx, repo))
 
 	// We now write a bunch of objects into the object pool that are not referenced by anything.
@@ -88,19 +81,14 @@ func testFetchFromOriginDangling(t *testing.T, ctx context.Context) {
 
 func TestFetchFromOrigin_fsck(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.ObjectPoolDontInitOnFetch).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, err := repo.Path()
 	require.NoError(t, err)
 
-	require.NoError(t, pool.Init(ctx))
 	require.NoError(t, pool.FetchFromOrigin(ctx, repo), "seed pool")
 
 	// We're creating a new commit which has a root tree with duplicate entries. git-mktree(1)
@@ -120,19 +108,14 @@ func testFetchFromOriginFsck(t *testing.T, ctx context.Context) {
 
 func TestFetchFromOrigin_deltaIslands(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.ObjectPoolDontInitOnFetch).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)
 	repoPath, err := repo.Path()
 	require.NoError(t, err)
 
-	require.NoError(t, pool.Init(ctx))
 	require.NoError(t, pool.FetchFromOrigin(ctx, repo), "seed pool")
 	require.NoError(t, pool.Link(ctx, repo))
 
@@ -146,12 +129,8 @@ func testFetchFromOriginDeltaIslands(t *testing.T, ctx context.Context) {
 
 func TestFetchFromOrigin_bitmapHashCache(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.ObjectPoolDontInitOnFetch).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)
@@ -159,7 +138,6 @@ func testFetchFromOriginBitmapHashCache(t *testing.T, ctx context.Context) {
 	require.NoError(t, err)
 	gittest.WriteCommit(t, cfg, repoPath, gittest.WithBranch("master"))
 
-	require.NoError(t, pool.Init(ctx))
 	require.NoError(t, pool.FetchFromOrigin(ctx, repo))
 
 	bitmaps, err := filepath.Glob(filepath.Join(pool.FullPath(), "objects", "pack", "*.bitmap"))
@@ -171,12 +149,8 @@ func testFetchFromOriginBitmapHashCache(t *testing.T, ctx context.Context) {
 
 func TestFetchFromOrigin_refUpdates(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.ObjectPoolDontInitOnFetch).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)
@@ -191,7 +165,6 @@ func testFetchFromOriginRefUpdates(t *testing.T, ctx context.Context) {
 	oldRefs["tags/v1.1.0"] = gittest.WriteTag(t, cfg, repoPath, "v1.1.0", oldRefs["heads/csv"].Revision())
 
 	// We now fetch that data into the object pool and verify that it exists as expected.
-	require.NoError(t, pool.Init(ctx))
 	require.NoError(t, pool.FetchFromOrigin(ctx, repo))
 	for ref, oid := range oldRefs {
 		require.Equal(t, oid, gittest.ResolveRevision(t, cfg, poolPath, "refs/remotes/origin/"+ref))
@@ -225,12 +198,8 @@ func testFetchFromOriginRefUpdates(t *testing.T, ctx context.Context) {
 
 func TestFetchFromOrigin_refs(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.ObjectPoolDontInitOnFetch).Run(t, testFetchFromOriginRefs)
-}
-
-func testFetchFromOriginRefs(t *testing.T, ctx context.Context) {
-	t.Parallel()
 
+	ctx := testhelper.Context(t)
 	cfg, pool, repoProto := setupObjectPool(t, ctx)
 
 	// Initialize the object pool and verify that it ain't yet got any references.
@@ -268,24 +237,3 @@ func testFetchFromOriginRefs(t *testing.T, ctx context.Context) {
 	// write out in Git.
 	require.NoFileExists(t, filepath.Join(poolPath, "FETCH_HEAD"))
 }
-
-func TestFetchFromOrigin_missingPool(t *testing.T) {
-	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.ObjectPoolDontInitOnFetch).Run(t, testFetchFromOriginMissingPool)
-}
-
-func testFetchFromOriginMissingPool(t *testing.T, ctx context.Context) {
-	t.Parallel()
-
-	cfg, pool, repoProto := setupObjectPool(t, ctx)
-	repo := localrepo.NewTestRepo(t, cfg, repoProto)
-
-	err := pool.FetchFromOrigin(ctx, repo)
-	if featureflag.ObjectPoolDontInitOnFetch.IsEnabled(ctx) {
-		require.Equal(t, helper.ErrInvalidArgumentf("object pool does not exist"), err)
-		require.False(t, pool.Exists())
-	} else {
-		require.NoError(t, err)
-		require.True(t, pool.Exists())
-	}
-}
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 2ac4d8208..217fee28e 100644
--- a/internal/gitaly/service/objectpool/fetch_into_object_pool_test.go
+++ b/internal/gitaly/service/objectpool/fetch_into_object_pool_test.go
@@ -24,7 +24,6 @@ import (
 	"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/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 +39,8 @@ import (
 
 func TestFetchIntoObjectPool_Success(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.ObjectPoolDontInitOnFetch).Run(t, testFetchIntoObjectPoolsuccess)
-}
-
-func testFetchIntoObjectPoolsuccess(t *testing.T, ctx context.Context) {
-	t.Parallel()
 
+	ctx := testhelper.Context(t)
 	cfg, repo, repoPath, _, client := setup(t, ctx)
 
 	parentID := gittest.WriteCommit(t, cfg, repoPath, gittest.WithBranch("main"))
@@ -102,11 +97,6 @@ func testFetchIntoObjectPoolsuccess(t *testing.T, ctx context.Context) {
 
 func TestFetchIntoObjectPool_transactional(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.ObjectPoolDontInitOnFetch).Run(t, testFetchIntoObjectPoolTransactional)
-}
-
-func testFetchIntoObjectPoolTransactional(t *testing.T, ctx context.Context) {
-	t.Parallel()
 
 	var votes []voting.Vote
 	var votesMutex sync.Mutex
@@ -119,6 +109,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),
@@ -230,11 +221,6 @@ func testFetchIntoObjectPoolTransactional(t *testing.T, ctx context.Context) {
 
 func TestFetchIntoObjectPool_CollectLogStatistics(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.ObjectPoolDontInitOnFetch).Run(t, testFetchIntoObjectPoolcollectLogStatistics)
-}
-
-func testFetchIntoObjectPoolcollectLogStatistics(t *testing.T, ctx context.Context) {
-	t.Parallel()
 
 	cfg := testcfg.Build(t)
 	testcfg.BuildGitalyHooks(t, cfg)
@@ -244,6 +230,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(t, ctx, cfg, gittest.CreateRepositoryConfig{
 		Seed: gittest.SeedGitLabTest,
@@ -280,11 +267,6 @@ func testFetchIntoObjectPoolcollectLogStatistics(t *testing.T, ctx context.Conte
 
 func TestFetchIntoObjectPool_Failure(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.ObjectPoolDontInitOnFetch).Run(t, testFetchIntoObjectPoolfailure)
-}
-
-func testFetchIntoObjectPoolfailure(t *testing.T, ctx context.Context) {
-	t.Parallel()
 
 	cfgBuilder := testcfg.NewGitalyCfgBuilder()
 	cfg, repos := cfgBuilder.BuildWithRepoAt(t, t.Name())
@@ -302,6 +284,7 @@ func testFetchIntoObjectPoolfailure(t *testing.T, ctx context.Context) {
 		txManager,
 		housekeeping.NewManager(cfg.Prometheus, txManager),
 	)
+	ctx := testhelper.Context(t)
 
 	pool := initObjectPool(t, cfg, cfg.Storages[0])
 
@@ -352,12 +335,8 @@ func testFetchIntoObjectPoolfailure(t *testing.T, ctx context.Context) {
 
 func TestFetchIntoObjectPool_dfConflict(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.ObjectPoolDontInitOnFetch).Run(t, testFetchIntoObjectPoolDfConflict)
-}
-
-func testFetchIntoObjectPoolDfConflict(t *testing.T, ctx context.Context) {
-	t.Parallel()
 
+	ctx := testhelper.Context(t)
 	cfg, repo, repoPath, _, client := setup(t, ctx)
 
 	pool := initObjectPool(t, cfg, cfg.Storages[0])
diff --git a/internal/metadata/featureflag/ff_object_pool_dont_init_on_fetch.go b/internal/metadata/featureflag/ff_object_pool_dont_init_on_fetch.go
deleted file mode 100644
index aa7bf57ca..000000000
--- a/internal/metadata/featureflag/ff_object_pool_dont_init_on_fetch.go
+++ /dev/null
@@ -1,10 +0,0 @@
-package featureflag
-
-// ObjectPoolDontInitOnFetch will cause FetchIntoObjectPool to not initialize the object pool when
-// it does not yet exist.
-var ObjectPoolDontInitOnFetch = NewFeatureFlag(
-	"object_pool_dont_init_on_fetch",
-	"v15.6.0",
-	"https://gitlab.com/gitlab-org/gitaly/-/issues/4560",
-	false,
-)