Merge branch 'wc/rm-packed-refs-lock' into 'master'

storagemgr: Remove packed-refs lockfile on start

Closes #5313

See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/6238

Merged-by: karthik nayak <knayak@gitlab.com>
Approved-by: karthik nayak <knayak@gitlab.com>
Reviewed-by: Will Chandler <wchandler@gitlab.com>
Reviewed-by: Sami Hiltunen <shiltunen@gitlab.com>
Co-authored-by: Will Chandler <wchandler@gitlab.com>

2 files changed, 79 insertions, 0 deletions

diff --git a/internal/gitaly/storage/storagemgr/transaction_manager.go b/internal/gitaly/storage/storagemgr/transaction_manager.go
index 33f50f932..6ba849132 100644
--- a/internal/gitaly/storage/storagemgr/transaction_manager.go
+++ b/internal/gitaly/storage/storagemgr/transaction_manager.go
@@ -1004,6 +1004,11 @@ 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)
 	}
@@ -1120,6 +1125,27 @@ func (mgr *TransactionManager) createStateDirectory() error {
 	return nil
 }
 
+// removePackedRefsLocks removes any packed-refs.lock and packed-refs.new files present in the manager's
+// repository. No grace period for the locks is given as any lockfiles present must be stale and can be
+// safely removed immediately.
+func (mgr *TransactionManager) removePackedRefsLocks(ctx context.Context) error {
+	for _, lock := range []string{".new", ".lock"} {
+		lockPath := filepath.Join(mgr.repositoryPath, "packed-refs"+lock)
+
+		// We deliberately do not fsync this deletion. Should a crash occur before this is persisted
+		// to disk, the restarted transaction manager will simply remove them again.
+		if err := os.Remove(lockPath); err != nil {
+			if errors.Is(err, fs.ErrNotExist) {
+				continue
+			}
+
+			return fmt.Errorf("remove %v: %w", lockPath, err)
+		}
+	}
+
+	return nil
+}
+
 // removeStaleWALFiles removes files from the log directory that have no associated log entry.
 // Such files can be left around if transaction's files were moved in place successfully
 // but the manager was interrupted before successfully persisting the log entry itself.
diff --git a/internal/gitaly/storage/storagemgr/transaction_manager_test.go b/internal/gitaly/storage/storagemgr/transaction_manager_test.go
index ca4d5cf7a..86b9f5ba7 100644
--- a/internal/gitaly/storage/storagemgr/transaction_manager_test.go
+++ b/internal/gitaly/storage/storagemgr/transaction_manager_test.go
@@ -494,6 +494,59 @@ func TestTransactionManager(t *testing.T) {
 			},
 		},
 		{
+			desc: "delete packed reference with stale reference locks",
+			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")
+						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.
+						require.NoError(t, os.WriteFile(
+							filepath.Join(repoPath, "packed-refs.lock"),
+							[]byte{},
+							perm.PrivateFile,
+						))
+						require.NoError(t, os.WriteFile(
+							filepath.Join(repoPath, "packed-refs.new"),
+							[]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: "create a file-directory reference conflict different transaction",
 			steps: steps{
 				StartManager{},