gitaly: Cleanup stale reflocks in `prepareReferenceTransaction`

When running `prepareReferenceTransaction`, there is a possibility that
it fails due to the existence of stale reference locks in the repository.

Since TransactionManager is the only process apart from housekeeping
which writes into the repository, we should be okay with cleaning these
stale reference locks.

So in `prepareReferenceTransaction`, if we encounter a stale reference
lock, we add an inhibitor on housekeeping from running git-pack-refs(1).
This is to avoid new reference locks from being created. Then we clear
the existing stale reference locks before continuing further.

To enable this we expose the internal function `addPackRefsInhibitor` as
`AddPackRefsInhibitor` in the housekeeping package via the
`HousekeepingManager`.

We also add a new step type: `ModifyFiles` in `TestTransactionManager`
which allows us to create files in the repository.

3 files changed, 86 insertions, 12 deletions

diff --git a/internal/git/housekeeping/manager.go b/internal/git/housekeeping/manager.go
index 173ad06c2..854d6c8ad 100644
--- a/internal/git/housekeeping/manager.go
+++ b/internal/git/housekeeping/manager.go
@@ -19,6 +19,8 @@ type Manager interface {
 	// OptimizeRepository optimizes the repository's data structures such that it can be more
 	// efficiently served.
 	OptimizeRepository(context.Context, *localrepo.Repo, ...OptimizeRepositoryOption) error
+	// AddPackRefsInhibitor allows clients to block housekeeping from running git-pack-refs(1).
+	AddPackRefsInhibitor(ctx context.Context, repoPath string) (bool, func(), error)
 }
 
 // repositoryState holds the housekeeping state for individual repositories. This structure can be
@@ -311,3 +313,10 @@ func (m *RepositoryManager) Collect(metrics chan<- prometheus.Metric) {
 	m.dataStructureSize.Collect(metrics)
 	m.dataStructureTimeSinceLastOptimization.Collect(metrics)
 }
+
+// AddPackRefsInhibitor exposes the internal function addPackRefsInhibitor on the
+// RepositoryManager level. This can then be used by other clients to block housekeeping
+// from running git-pack-refs(1).
+func (m *RepositoryManager) AddPackRefsInhibitor(ctx context.Context, repoPath string) (successful bool, _ func(), err error) {
+	return m.repositoryStates.addPackRefsInhibitor(ctx, repoPath)
+}
diff --git a/internal/gitaly/transaction_manager.go b/internal/gitaly/transaction_manager.go
index 939cd4ca7..82f2d6983 100644
--- a/internal/gitaly/transaction_manager.go
+++ b/internal/gitaly/transaction_manager.go
@@ -1157,26 +1157,67 @@ func (mgr *TransactionManager) applyDefaultBranchUpdate(ctx context.Context, def
 // or aborting up to the caller. Either should be called to clean up the process. The process is cleaned up
 // if an error is returned.
 func (mgr *TransactionManager) prepareReferenceTransaction(ctx context.Context, referenceUpdates []*gitalypb.LogEntry_ReferenceUpdate, repository *localrepo.Repo) (*updateref.Updater, error) {
-	updater, err := updateref.New(ctx, repository, updateref.WithDisabledTransactions())
-	if err != nil {
-		return nil, fmt.Errorf("new: %w", err)
+	// This section runs git-update-ref(1), but could fail due to existing
+	// reference locks. So we create a function which can be called again
+	// post cleanup of stale reference locks.
+	updateFunc := func() (*updateref.Updater, error) {
+		updater, err := updateref.New(ctx, repository, updateref.WithDisabledTransactions())
+		if err != nil {
+			return nil, fmt.Errorf("new: %w", err)
+		}
+
+		if err := updater.Start(); err != nil {
+			return nil, fmt.Errorf("start: %w", err)
+		}
+
+		for _, referenceUpdate := range referenceUpdates {
+			if err := updater.Update(git.ReferenceName(referenceUpdate.ReferenceName), git.ObjectID(referenceUpdate.NewOid), ""); err != nil {
+				return nil, fmt.Errorf("update %q: %w", referenceUpdate.ReferenceName, err)
+			}
+		}
+
+		if err := updater.Prepare(); err != nil {
+			return nil, fmt.Errorf("prepare: %w", err)
+		}
+
+		return updater, nil
 	}
 
-	if err := updater.Start(); err != nil {
-		return nil, fmt.Errorf("start: %w", err)
+	// If git-update-ref(1) runs without issues, our work here is done.
+	updater, err := updateFunc()
+	if err == nil {
+		return updater, nil
 	}
 
-	for _, referenceUpdate := range referenceUpdates {
-		if err := updater.Update(git.ReferenceName(referenceUpdate.ReferenceName), git.ObjectID(referenceUpdate.NewOid), ""); err != nil {
-			return nil, fmt.Errorf("update %q: %w", referenceUpdate.ReferenceName, err)
+	// If we get an error due to existing stale reference locks, we should clear it up
+	// and retry running git-update-ref(1).
+	var updateRefError *updateref.AlreadyLockedError
+	if errors.As(err, &updateRefError) {
+		// Before clearing stale reference locks, we add should ensure that housekeeping doesn't
+		// run git-pack-refs(1), which could create new reference locks. So we add an inhibitor.
+		success, cleanup, err := mgr.housekeepingManager.AddPackRefsInhibitor(ctx, mgr.repositoryPath)
+		if !success {
+			return nil, fmt.Errorf("add pack-refs inhibitor: %w", err)
+		}
+		defer cleanup()
+
+		// We ask housekeeping to cleanup stale reference locks. We don't add a grace period, because
+		// transaction manager is the only process which writes into the repository, so it is safe
+		// to delete these locks.
+		if err := mgr.housekeepingManager.CleanStaleData(ctx, mgr.repository, housekeeping.OnlyStaleReferenceLockCleanup(0)); err != nil {
+			return nil, fmt.Errorf("running reflock cleanup: %w", err)
 		}
-	}
 
-	if err := updater.Prepare(); err != nil {
-		return nil, fmt.Errorf("prepare: %w", err)
+		// We try a second time, this should succeed. If not, there is something wrong and
+		// we return the error.
+		//
+		// Do note, that we've already added an inhibitor above, so git-pack-refs(1) won't run
+		// again until we return from this function so ideally this should work, but in case it
+		// doesn't we return the error.
+		return updateFunc()
 	}
 
-	return updater, nil
+	return nil, err
 }
 
 // appendLogEntry appends the transaction to the write-ahead log. References that failed verification are skipped and thus not
diff --git a/internal/gitaly/transaction_manager_test.go b/internal/gitaly/transaction_manager_test.go
index 83eae522b..f4817c3d2 100644
--- a/internal/gitaly/transaction_manager_test.go
+++ b/internal/gitaly/transaction_manager_test.go
@@ -423,6 +423,30 @@ func TestTransactionManager(t *testing.T) {
 			},
 		},
 		{
+			desc: "create reference with existing reference lock",
+			steps: steps{
+				StartManager{
+					ModifyRepository: func(_ testing.TB, repoPath string) {
+						err := os.WriteFile(fmt.Sprintf("%s/refs/heads/main.lock", repoPath), []byte{}, 0o666)
+						require.NoError(t, err)
+					},
+				},
+				Begin{},
+				Commit{
+					ReferenceUpdates: ReferenceUpdates{
+						"refs/heads/main": {OldOID: setup.ObjectHash.ZeroOID, NewOID: setup.Commits.First.OID},
+					},
+				},
+			},
+			expectedState: StateAssertion{
+				DefaultBranch: "refs/heads/main",
+				References:    []git.Reference{{Name: "refs/heads/main", Target: setup.Commits.First.OID.String()}},
+				Database: DatabaseState{
+					string(keyAppliedLogIndex(relativePath)): LogIndex(1).toProto(),
+				},
+			},
+		},
+		{
 			desc: "create a file-directory reference conflict different transaction",
 			steps: steps{
 				StartManager{},