Enable transactions for CreateObjectPool

This commit enables transactions for CreateObjectPool. One test
had to be changed as it now fails earlier in the call chain due to
invalid state and leads to a different error messsage. Another test
asserting lock semantics is skipped. RPCs won't see each other's
locks with transactions.

TestCreate is now getting the test commit created in a branch where
as it was previously unreferenced. Object pool creations are treated
as normal repository creations by transactions. As such, unreferenced
objects are not included in the write-ahead logged packs. Referencing
the object allows the later assertions for object existence to pass.
Overall, this shouldn't really make a big difference in behavior. If
nothing is referencing the objects anyway, the pool doesn't need them.
If they are later referenced, later fetches to the pool will retrieve
them. The bigger question is whether packing a repository in the single
pack that gets logged deteroriate performance meaningfully. If this
becomes a problem, we can later special case object pool creations and
write-ahead log them more efficiently.

4 files changed, 29 insertions, 7 deletions

diff --git a/internal/gitaly/service/objectpool/create.go b/internal/gitaly/service/objectpool/create.go
index c60398285..29691d41f 100644
--- a/internal/gitaly/service/objectpool/create.go
+++ b/internal/gitaly/service/objectpool/create.go
@@ -29,9 +29,19 @@ func (s *server) CreateObjectPool(ctx context.Context, in *gitalypb.CreateObject
 		return nil, errInvalidPoolDir
 	}
 
-	if err := repoutil.Create(ctx, s.logger, s.locator, s.gitCmdFactory, s.txManager, s.repositoryCounter, poolRepo, func(poolRepo *gitalypb.Repository) error {
+	// repoutil.Create creates the repositories in a temporary directory. This means the repository is not created in the location
+	// expected by the transaction manager. This makes sense without transactions, but with transactions, there's no real point in
+	// doing so given a failed transaction's state is anyway removed. Creating the repository in a temporary directory is problematic
+	// as the reference transcation hook is invoked for the repository from unexpected location, causing the transaction to fail to
+	// associate the reference updates with the repository.
+	//
+	// Run the repository creation without the transaction in the context. The transactions reads the created repository's state from
+	// the disk when committing it, so it's not necessary to capture the updates from the reference-transaction hook. This avoids the
+	// problem for now, and later with transactions enabled by default we can stop creating repositories in unexpected locations.
+	ctxWithoutTransaction := storage.ContextWithTransactionID(ctx, 0)
+	if err := repoutil.Create(ctxWithoutTransaction, s.logger, s.locator, s.gitCmdFactory, s.txManager, s.repositoryCounter, poolRepo, func(poolRepo *gitalypb.Repository) error {
 		if _, err := objectpool.Create(
-			ctx,
+			ctxWithoutTransaction,
 			s.logger,
 			s.locator,
 			s.gitCmdFactory,
diff --git a/internal/gitaly/service/objectpool/create_test.go b/internal/gitaly/service/objectpool/create_test.go
index f7449c8a8..4b507832d 100644
--- a/internal/gitaly/service/objectpool/create_test.go
+++ b/internal/gitaly/service/objectpool/create_test.go
@@ -17,6 +17,7 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v16/internal/git/objectpool"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/git/stats"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/config"
+	"gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/storage"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/transaction"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/helper/perm"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/structerr"
@@ -32,7 +33,7 @@ func TestCreate(t *testing.T) {
 	ctx := testhelper.Context(t)
 	logger := testhelper.NewLogger(t)
 	cfg, repo, repoPath, _, client := setup(t, ctx)
-	commitID := gittest.WriteCommit(t, cfg, repoPath)
+	commitID := gittest.WriteCommit(t, cfg, repoPath, gittest.WithBranch("main"))
 
 	txManager := transaction.NewManager(cfg, logger, nil)
 	catfileCache := catfile.NewCache(cfg)
@@ -149,6 +150,7 @@ func TestCreate_unsuccessful(t *testing.T) {
 		desc        string
 		request     *gitalypb.CreateObjectPoolRequest
 		expectedErr error
+		skipWithWAL string
 	}{
 		{
 			desc: "no origin repository",
@@ -167,7 +169,15 @@ func TestCreate_unsuccessful(t *testing.T) {
 			request: &gitalypb.CreateObjectPoolRequest{
 				Origin: repo,
 			},
-			expectedErr: errMissingPool,
+			expectedErr: func() error {
+				if testhelper.IsWALEnabled() {
+					// The transaction middleware is erroring out and returns the generic
+					// repository not set error.
+					return structerr.NewInvalidArgument("%w", storage.ErrRepositoryNotSet)
+				}
+
+				return errMissingPool
+			}(),
 		},
 		{
 			desc: "outside pools directory",
@@ -233,6 +243,7 @@ func TestCreate_unsuccessful(t *testing.T) {
 				},
 			},
 			expectedErr: structerr.NewInternal("creating object pool: locking repository: file already locked"),
+			skipWithWAL: `Transactions are isolated and won't see each other's locks.`,
 		},
 		{
 			desc: "pool exists",
@@ -244,6 +255,10 @@ func TestCreate_unsuccessful(t *testing.T) {
 		},
 	} {
 		t.Run(tc.desc, func(t *testing.T) {
+			if tc.skipWithWAL != "" {
+				testhelper.SkipWithWAL(t, tc.skipWithWAL)
+			}
+
 			_, err := client.CreateObjectPool(ctx, tc.request)
 			testhelper.RequireGrpcError(t, tc.expectedErr, err)
 		})
diff --git a/internal/gitaly/storage/storagemgr/middleware.go b/internal/gitaly/storage/storagemgr/middleware.go
index 694acbe17..1b50053a2 100644
--- a/internal/gitaly/storage/storagemgr/middleware.go
+++ b/internal/gitaly/storage/storagemgr/middleware.go
@@ -41,7 +41,6 @@ var NonTransactionalRPCs = map[string]struct{}{
 	"/gitaly.ServerService/ReadinessCheck": {},
 
 	// Object pools are not yet supported with WAL.
-	"/gitaly.ObjectPoolService/CreateObjectPool":           {},
 	"/gitaly.ObjectPoolService/LinkRepositoryToObjectPool": {},
 
 	// ReplicateRepository is replicating the attributes and config which the
diff --git a/internal/gitaly/storage/storagemgr/middleware_test.go b/internal/gitaly/storage/storagemgr/middleware_test.go
index d731e389b..866284ae2 100644
--- a/internal/gitaly/storage/storagemgr/middleware_test.go
+++ b/internal/gitaly/storage/storagemgr/middleware_test.go
@@ -102,10 +102,8 @@ messages and behavior by erroring out the requests before they even hit this int
 	// them transactionally. For now, make these RPCs transactional within this test only. There's no point
 	// switching to dependency injection with the list of non transactional RPCs as we're soon enabling
 	// transactions for the remaining non-transactional RPCs and remove the list entirely.
-	delete(storagemgr.NonTransactionalRPCs, "/gitaly.ObjectPoolService/CreateObjectPool")
 	delete(storagemgr.NonTransactionalRPCs, "/gitaly.ObjectPoolService/LinkRepositoryToObjectPool")
 	defer func() {
-		storagemgr.NonTransactionalRPCs["/gitaly.ObjectPoolService/CreateObjectPool"] = struct{}{}
 		storagemgr.NonTransactionalRPCs["/gitaly.ObjectPoolService/LinkRepositoryToObjectPool"] = struct{}{}
 	}()