Clear packed-refs locks when encountering them

The TransactionManager is currently clearing packed-refs locks on
initialization. This is somewhat wasteful given there generally
wouldn't be any stale lock files. If a partition has multiple
repositories, this is increasingly wasteful as we'd have to check
every repository when initializing. Move the lock file clearing to
happen instead when we encounter locks while attempting to work
on the repository. This lazier method of clearing the locks works
better with many repositories.

The tests related to packed-refs lock clearing were split in two to
ensure both possible lock files are correctly detected.

2 files changed, 51 insertions, 11 deletions

diff --git a/internal/gitaly/storage/storagemgr/transaction_manager.go b/internal/gitaly/storage/storagemgr/transaction_manager.go
index d95d9fd78..6e480b692 100644
--- a/internal/gitaly/storage/storagemgr/transaction_manager.go
+++ b/internal/gitaly/storage/storagemgr/transaction_manager.go
@@ -1004,11 +1004,6 @@ func (mgr *TransactionManager) initialize(ctx context.Context) error {
 		mgr.applyNotifications[i] = make(chan struct{})
 	}
 
-	// Remove any packed-refs lockfiles found on startup.
-	if err := mgr.removePackedRefsLocks(mgr.ctx); err != nil {
-		return fmt.Errorf("remove stale packed-refs locks: %w", err)
-	}
-
 	if err := mgr.removeStaleWALFiles(mgr.ctx, mgr.appendedLogIndex); err != nil {
 		return fmt.Errorf("remove stale packs: %w", err)
 	}
@@ -1356,8 +1351,7 @@ func (mgr *TransactionManager) prepareReferenceTransaction(ctx context.Context,
 
 	// 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) {
+	if errors.Is(err, updateref.ErrPackedRefsLocked) || errors.As(err, &updateref.AlreadyLockedError{}) {
 		// 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)
@@ -1373,6 +1367,11 @@ func (mgr *TransactionManager) prepareReferenceTransaction(ctx context.Context,
 			return nil, fmt.Errorf("running reflock cleanup: %w", err)
 		}
 
+		// Remove possible locks and temporary files covering `packed-refs`.
+		if err := mgr.removePackedRefsLocks(mgr.ctx); err != nil {
+			return nil, fmt.Errorf("remove stale packed-refs locks: %w", err)
+		}
+
 		// We try a second time, this should succeed. If not, there is something wrong and
 		// we return the error.
 		//
diff --git a/internal/gitaly/storage/storagemgr/transaction_manager_test.go b/internal/gitaly/storage/storagemgr/transaction_manager_test.go
index 3d9897242..b36d74638 100644
--- a/internal/gitaly/storage/storagemgr/transaction_manager_test.go
+++ b/internal/gitaly/storage/storagemgr/transaction_manager_test.go
@@ -494,7 +494,7 @@ func TestTransactionManager(t *testing.T) {
 			},
 		},
 		{
-			desc: "delete packed reference with stale reference locks",
+			desc: "delete packed reference with existing packed-refs.lock",
 			steps: steps{
 				StartManager{},
 				Begin{
@@ -511,15 +511,56 @@ func TestTransactionManager(t *testing.T) {
 					ModifyRepository: func(tb testing.TB, cfg config.Cfg, repoPath string) {
 						// Pack the reference and create a stale lockfile for it.
 						gittest.Exec(tb, cfg, "-C", repoPath, "pack-refs", "--all")
-						require.NoError(t, os.WriteFile(fmt.Sprintf("%s/refs/heads/main.lock", repoPath), []byte{}, perm.PrivateFile))
 
-						// Add packed-refs lockfiles. The reference deletion will fail if these
-						// are not cleaned up.
+						// Add packed-refs.lock. The reference deletion will fail if this
+						// isn't cleaned up.
 						require.NoError(t, os.WriteFile(
 							filepath.Join(repoPath, "packed-refs.lock"),
 							[]byte{},
 							perm.PrivateFile,
 						))
+					},
+				},
+				Begin{
+					TransactionID: 2,
+					ExpectedSnapshot: Snapshot{
+						ReadIndex: 1,
+					},
+				},
+				Commit{
+					TransactionID: 2,
+					ReferenceUpdates: ReferenceUpdates{
+						"refs/heads/main": {OldOID: setup.Commits.First.OID, NewOID: setup.ObjectHash.ZeroOID},
+					},
+				},
+			},
+			expectedState: StateAssertion{
+				Database: DatabaseState{
+					string(keyAppliedLogIndex(relativePath)): LogIndex(2).toProto(),
+				},
+			},
+		},
+		{
+			desc: "delete packed reference with existing packed-refs.new",
+			steps: steps{
+				StartManager{},
+				Begin{
+					TransactionID: 1,
+				},
+				Commit{
+					TransactionID: 1,
+					ReferenceUpdates: ReferenceUpdates{
+						"refs/heads/main": {OldOID: setup.ObjectHash.ZeroOID, NewOID: setup.Commits.First.OID},
+					},
+				},
+				CloseManager{},
+				StartManager{
+					ModifyRepository: func(tb testing.TB, cfg config.Cfg, repoPath string) {
+						// Pack the reference and create a stale lockfile for it.
+						gittest.Exec(tb, cfg, "-C", repoPath, "pack-refs", "--all")
+
+						// Add packed-refs.new. The reference deletion will fail if this
+						// isn't cleaned up.
 						require.NoError(t, os.WriteFile(
 							filepath.Join(repoPath, "packed-refs.new"),
 							[]byte{},