Always initialize staging directory for a transaction

So far we've been initializing the staging directory only when a
transaction is about to write objects. Since we're soon about to
set up a quarantine automatically when the transaction begins,
we'll need the staging directory to store it. Let's create the
staging directory thus unconditionally.

Tests had to be adapted as the assertions ensuring the staging
directory is always cleaned up were failing. Some of the tests were
not always finishing their transactions. This was fine as they didn't
write objects so the staging directory was not created and there was
no state to clean up. Now that we always create it, we need to finish
the transactions so they clean up after themselves.

If there is a crash, some transactions could be interrupted without
being able to clean up after themselves. This is handled in the
PartitionManager by always deleting and recreating the staging directory
on partition start to ensure it doesn't contain any stale data. Do
the same in the TransactionManager tests to clean up after crashes.

Previously read-only transactions would not set up a quarantine to avoid
wasteful work that is not needed. They'll do so for now but we'll later
fix this by differentiating read-only transactions from read-write
transactions.

2 files changed, 21 insertions, 22 deletions

diff --git a/internal/gitaly/storage/storagemgr/transaction_manager.go b/internal/gitaly/storage/storagemgr/transaction_manager.go
index eeb167a71..ef08b33f6 100644
--- a/internal/gitaly/storage/storagemgr/transaction_manager.go
+++ b/internal/gitaly/storage/storagemgr/transaction_manager.go
@@ -165,9 +165,6 @@ type Transaction struct {
 	// to finish where needed.
 	finished chan struct{}
 
-	// initStagingDirectory is called to lazily initialize the staging directory when it is
-	// needed.
-	initStagingDirectory func() error
 	// stagingDirectory is the directory where the transaction stages its files prior
 	// to them being logged. It is cleaned up when the transaction finishes.
 	stagingDirectory string
@@ -240,16 +237,6 @@ func (mgr *TransactionManager) Begin(ctx context.Context) (_ *Transaction, retur
 		close(readReady)
 	}
 
-	txn.initStagingDirectory = func() error {
-		stagingDirectory, err := os.MkdirTemp(mgr.stagingDirectory, "")
-		if err != nil {
-			return fmt.Errorf("mkdir temp: %w", err)
-		}
-
-		txn.stagingDirectory = stagingDirectory
-		return nil
-	}
-
 	txn.finish = func() error {
 		defer close(txn.finished)
 
@@ -284,6 +271,12 @@ func (mgr *TransactionManager) Begin(ctx context.Context) (_ *Transaction, retur
 			return nil, ErrRepositoryNotFound
 		}
 
+		var err error
+		txn.stagingDirectory, err = os.MkdirTemp(mgr.stagingDirectory, "")
+		if err != nil {
+			return nil, fmt.Errorf("mkdir temp: %w", err)
+		}
+
 		return txn, nil
 	}
 }
@@ -373,10 +366,6 @@ func (txn *Transaction) DeleteRepository() {
 // is a Git object directory where the new objects introduced in the transaction must be written. The quarantined
 // objects needed by the updated reference tips will be included in the transaction.
 func (txn *Transaction) QuarantineDirectory() (string, error) {
-	if err := txn.initStagingDirectory(); err != nil {
-		return "", fmt.Errorf("init staging directory: %w", err)
-	}
-
 	quarantineDirectory := filepath.Join(txn.stagingDirectory, "quarantine")
 	if err := os.MkdirAll(filepath.Join(quarantineDirectory, "pack"), perm.PrivateDir); err != nil {
 		return "", fmt.Errorf("create quarantine directory: %w", err)
@@ -595,10 +584,6 @@ func (mgr *TransactionManager) stageHooks(ctx context.Context, transaction *Tran
 		return nil
 	}
 
-	if err := transaction.initStagingDirectory(); err != nil {
-		return fmt.Errorf("init staging directory: %w", err)
-	}
-
 	if err := repoutil.ExtractHooks(
 		ctx,
 		bytes.NewReader(transaction.customHooksUpdate.CustomHooksTAR),
diff --git a/internal/gitaly/storage/storagemgr/transaction_manager_test.go b/internal/gitaly/storage/storagemgr/transaction_manager_test.go
index bbf4fb8b7..8b463f5cf 100644
--- a/internal/gitaly/storage/storagemgr/transaction_manager_test.go
+++ b/internal/gitaly/storage/storagemgr/transaction_manager_test.go
@@ -1256,6 +1256,9 @@ func TestTransactionManager(t *testing.T) {
 						CustomHookIndex: 2,
 					},
 				},
+				Rollback{
+					TransactionID: 4,
+				},
 			},
 			expectedState: StateAssertion{
 				Database: DatabaseState{
@@ -1965,6 +1968,12 @@ func TestTransactionManager(t *testing.T) {
 						CustomHookIndex: 3,
 					},
 				},
+				Rollback{
+					TransactionID: 5,
+				},
+				Rollback{
+					TransactionID: 6,
+				},
 			},
 			expectedState: StateAssertion{
 				DefaultBranch: "refs/heads/main",
@@ -3065,8 +3074,13 @@ func TestTransactionManager(t *testing.T) {
 					managerRunning = true
 					managerErr = make(chan error)
 
-					transactionManager = NewTransactionManager(database, storagePath, relativePath, stagingDir, setup.CommandFactory, housekeepingManager, setup.RepositoryFactory, noopTransactionFinalizer)
+					// The PartitionManager deletes and recreates the staging directory prior to starting a TransactionManager
+					// to clean up any stale state leftover by crashes. Do that here as well so the tests don't fail if we don't
+					// finish transactions after crash simulations.
+					require.NoError(t, os.RemoveAll(stagingDir))
+					require.NoError(t, os.Mkdir(stagingDir, perm.PrivateDir))
 
+					transactionManager = NewTransactionManager(database, storagePath, relativePath, stagingDir, setup.CommandFactory, housekeepingManager, setup.RepositoryFactory, noopTransactionFinalizer)
 					installHooks(t, transactionManager, database, hooks{
 						beforeReadLogEntry:  step.Hooks.BeforeApplyLogEntry,
 						beforeStoreLogEntry: step.Hooks.BeforeAppendLogEntry,