housekeeping: Make `CleanStaleData` configurable

In previous commits we introduced a configuration for `CleanStaleData`
via the struct `CleanStaleDataConfig`. In this commit, we make this
configuration an argument to `CleanStaleData`. This allow users to
supply their own configuration and only run the cleanup functions they
require.

We fix tests around this and also add a test to test a non-default grace
period for `findStaleReferenceLocks`.

5 files changed, 71 insertions, 21 deletions

diff --git a/internal/git/housekeeping/clean_stale_data.go b/internal/git/housekeeping/clean_stale_data.go
index c377fc133..a21b4367e 100644
--- a/internal/git/housekeeping/clean_stale_data.go
+++ b/internal/git/housekeeping/clean_stale_data.go
@@ -89,10 +89,8 @@ func DefaultStaleDataCleanup() CleanStaleDataConfig {
 	}
 }
 
-// CleanStaleData cleans up any stale data in the repository.
-func (m *RepositoryManager) CleanStaleData(ctx context.Context, repo *localrepo.Repo) error {
-	cfg := DefaultStaleDataCleanup()
-
+// CleanStaleData removes any stale data in the repository as per the provided configuration.
+func (m *RepositoryManager) CleanStaleData(ctx context.Context, repo *localrepo.Repo, cfg CleanStaleDataConfig) error {
 	span, ctx := tracing.StartSpanIfHasParent(ctx, "housekeeping.CleanStaleData", nil)
 	defer span.Finish()
 
diff --git a/internal/git/housekeeping/clean_stale_data_test.go b/internal/git/housekeeping/clean_stale_data_test.go
index 90bf8e4ea..f893e180e 100644
--- a/internal/git/housekeeping/clean_stale_data_test.go
+++ b/internal/git/housekeeping/clean_stale_data_test.go
@@ -203,6 +203,26 @@ func requireCleanStaleDataMetrics(t *testing.T, m *RepositoryManager, metrics cl
 	require.NoError(t, testutil.CollectAndCompare(m, strings.NewReader(builder.String()), "gitaly_housekeeping_pruned_files_total"))
 }
 
+func requireReferenceLockCleanupMetrics(t *testing.T, m *RepositoryManager, metrics cleanStaleDataMetrics) {
+	t.Helper()
+
+	var builder strings.Builder
+
+	_, err := builder.WriteString("# HELP gitaly_housekeeping_pruned_files_total Total number of files pruned\n")
+	require.NoError(t, err)
+	_, err = builder.WriteString("# TYPE gitaly_housekeeping_pruned_files_total counter\n")
+	require.NoError(t, err)
+
+	for metric, expectedValue := range map[string]int{
+		"reflocks": metrics.reflocks,
+	} {
+		_, err := builder.WriteString(fmt.Sprintf("gitaly_housekeeping_pruned_files_total{filetype=%q} %d\n", metric, expectedValue))
+		require.NoError(t, err)
+	}
+
+	require.NoError(t, testutil.CollectAndCompare(m, strings.NewReader(builder.String()), "gitaly_housekeeping_pruned_files_total"))
+}
+
 func TestRepositoryManager_CleanStaleData(t *testing.T) {
 	t.Parallel()
 	testcases := []struct {
@@ -381,7 +401,7 @@ func TestRepositoryManager_CleanStaleData(t *testing.T) {
 
 			mgr := NewManager(cfg.Prometheus, nil)
 
-			require.NoError(t, mgr.CleanStaleData(ctx, repo))
+			require.NoError(t, mgr.CleanStaleData(ctx, repo, DefaultStaleDataCleanup()))
 
 			for _, e := range tc.entries {
 				e.validate(t, repoPath)
@@ -486,7 +506,7 @@ func TestRepositoryManager_CleanStaleData_references(t *testing.T) {
 
 			mgr := NewManager(cfg.Prometheus, nil)
 
-			require.NoError(t, mgr.CleanStaleData(ctx, repo))
+			require.NoError(t, mgr.CleanStaleData(ctx, repo, DefaultStaleDataCleanup()))
 
 			var actual []string
 			require.NoError(t, filepath.Walk(filepath.Join(repoPath, "refs"), func(path string, info os.FileInfo, _ error) error {
@@ -613,7 +633,7 @@ func TestRepositoryManager_CleanStaleData_emptyRefDirs(t *testing.T) {
 
 			mgr := NewManager(cfg.Prometheus, nil)
 
-			require.NoError(t, mgr.CleanStaleData(ctx, repo))
+			require.NoError(t, mgr.CleanStaleData(ctx, repo, DefaultStaleDataCleanup()))
 
 			for _, e := range tc.entries {
 				e.validate(t, repoPath)
@@ -738,7 +758,7 @@ func TestRepositoryManager_CleanStaleData_withSpecificFile(t *testing.T) {
 			repo := localrepo.NewTestRepo(t, cfg, repoProto)
 			mgr := NewManager(cfg.Prometheus, nil)
 
-			require.NoError(t, mgr.CleanStaleData(ctx, repo))
+			require.NoError(t, mgr.CleanStaleData(ctx, repo, DefaultStaleDataCleanup()))
 			for _, subcase := range []struct {
 				desc          string
 				entry         entry
@@ -782,7 +802,7 @@ func TestRepositoryManager_CleanStaleData_withSpecificFile(t *testing.T) {
 					require.NoError(t, err)
 					require.ElementsMatch(t, subcase.expectedFiles, staleFiles)
 
-					require.NoError(t, mgr.CleanStaleData(ctx, repo))
+					require.NoError(t, mgr.CleanStaleData(ctx, repo, DefaultStaleDataCleanup()))
 
 					entry.validate(t, repoPath)
 				})
@@ -836,7 +856,7 @@ func TestRepositoryManager_CleanStaleData_serverInfo(t *testing.T) {
 
 	mgr := NewManager(cfg.Prometheus, nil)
 
-	require.NoError(t, mgr.CleanStaleData(ctx, repo))
+	require.NoError(t, mgr.CleanStaleData(ctx, repo, DefaultStaleDataCleanup()))
 
 	for _, entry := range entries {
 		entry.validate(t, repoPath)
@@ -856,6 +876,9 @@ func TestRepositoryManager_CleanStaleData_referenceLocks(t *testing.T) {
 		entries                []entry
 		expectedReferenceLocks []string
 		expectedMetrics        cleanStaleDataMetrics
+		gracePeriod            time.Duration
+		cfg                    CleanStaleDataConfig
+		metricsCompareFn       func(t *testing.T, m *RepositoryManager, metrics cleanStaleDataMetrics)
 	}{
 		{
 			desc: "fresh lock is kept",
@@ -865,6 +888,26 @@ func TestRepositoryManager_CleanStaleData_referenceLocks(t *testing.T) {
 					f("main.lock", withAge(10*time.Minute)),
 				}),
 			},
+			gracePeriod: referenceLockfileGracePeriod,
+			cfg:         DefaultStaleDataCleanup(),
+		},
+		{
+			desc: "fresh lock is deleted when grace period is low",
+			entries: []entry{
+				d("refs", []entry{
+					f("main", withAge(10*time.Minute)),
+					f("main.lock", withAge(10*time.Minute), expectDeletion),
+				}),
+			},
+			expectedReferenceLocks: []string{
+				"refs/main.lock",
+			},
+			expectedMetrics: cleanStaleDataMetrics{
+				reflocks: 1,
+			},
+			gracePeriod:      time.Second,
+			cfg:              OnlyStaleReferenceLockCleanup(time.Second),
+			metricsCompareFn: requireReferenceLockCleanupMetrics,
 		},
 		{
 			desc: "stale lock is deleted",
@@ -880,6 +923,8 @@ func TestRepositoryManager_CleanStaleData_referenceLocks(t *testing.T) {
 			expectedMetrics: cleanStaleDataMetrics{
 				reflocks: 1,
 			},
+			gracePeriod: referenceLockfileGracePeriod,
+			cfg:         DefaultStaleDataCleanup(),
 		},
 		{
 			desc: "nested reference locks are deleted",
@@ -907,6 +952,8 @@ func TestRepositoryManager_CleanStaleData_referenceLocks(t *testing.T) {
 			expectedMetrics: cleanStaleDataMetrics{
 				reflocks: 3,
 			},
+			gracePeriod: referenceLockfileGracePeriod,
+			cfg:         DefaultStaleDataCleanup(),
 		},
 	} {
 		tc := tc
@@ -933,19 +980,23 @@ func TestRepositoryManager_CleanStaleData_referenceLocks(t *testing.T) {
 				expectedReferenceLocks = append(expectedReferenceLocks, filepath.Join(repoPath, referenceLock))
 			}
 
-			staleLockfiles, err := findStaleReferenceLocks(referenceLockfileGracePeriod)(ctx, repoPath)
+			staleLockfiles, err := findStaleReferenceLocks(tc.gracePeriod)(ctx, repoPath)
 			require.NoError(t, err)
 			require.ElementsMatch(t, expectedReferenceLocks, staleLockfiles)
 
 			mgr := NewManager(cfg.Prometheus, nil)
 
-			require.NoError(t, mgr.CleanStaleData(ctx, repo))
+			require.NoError(t, mgr.CleanStaleData(ctx, repo, tc.cfg))
 
 			for _, e := range tc.entries {
 				e.validate(t, repoPath)
 			}
 
-			requireCleanStaleDataMetrics(t, mgr, tc.expectedMetrics)
+			if tc.metricsCompareFn != nil {
+				tc.metricsCompareFn(t, mgr, tc.expectedMetrics)
+			} else {
+				requireCleanStaleDataMetrics(t, mgr, tc.expectedMetrics)
+			}
 		})
 	}
 }
@@ -1048,7 +1099,7 @@ func TestRepositoryManager_CleanStaleData_missingRepo(t *testing.T) {
 
 	require.NoError(t, os.RemoveAll(repoPath))
 
-	require.NoError(t, NewManager(cfg.Prometheus, nil).CleanStaleData(ctx, repo))
+	require.NoError(t, NewManager(cfg.Prometheus, nil).CleanStaleData(ctx, repo, DefaultStaleDataCleanup()))
 }
 
 func TestRepositoryManager_CleanStaleData_unsetConfiguration(t *testing.T) {
@@ -1089,7 +1140,7 @@ func TestRepositoryManager_CleanStaleData_unsetConfiguration(t *testing.T) {
 
 	mgr := NewManager(cfg.Prometheus, nil)
 
-	require.NoError(t, mgr.CleanStaleData(ctx, repo))
+	require.NoError(t, mgr.CleanStaleData(ctx, repo, DefaultStaleDataCleanup()))
 	require.Equal(t,
 		`[core]
 	repositoryformatversion = 0
@@ -1128,7 +1179,7 @@ func TestRepositoryManager_CleanStaleData_unsetConfigurationTransactional(t *tes
 		AuthInfo: backchannel.WithID(nil, 1234),
 	})
 
-	require.NoError(t, NewManager(cfg.Prometheus, txManager).CleanStaleData(ctx, repo))
+	require.NoError(t, NewManager(cfg.Prometheus, txManager).CleanStaleData(ctx, repo, DefaultStaleDataCleanup()))
 	require.Equal(t, 2, len(txManager.Votes()))
 
 	configKeys := gittest.Exec(t, cfg, "-C", repoPath, "config", "--list", "--local", "--name-only")
@@ -1180,7 +1231,7 @@ func TestRepositoryManager_CleanStaleData_pruneEmptyConfigSections(t *testing.T)
 
 	mgr := NewManager(cfg.Prometheus, nil)
 
-	require.NoError(t, mgr.CleanStaleData(ctx, repo))
+	require.NoError(t, mgr.CleanStaleData(ctx, repo, DefaultStaleDataCleanup()))
 	require.Equal(t, `[core]
 	repositoryformatversion = 0
 	filemode = true
diff --git a/internal/git/housekeeping/manager.go b/internal/git/housekeeping/manager.go
index 36e26fb86..173ad06c2 100644
--- a/internal/git/housekeeping/manager.go
+++ b/internal/git/housekeeping/manager.go
@@ -14,8 +14,8 @@ import (
 // Manager is a housekeeping manager. It is supposed to handle housekeeping tasks for repositories
 // such as the cleanup of unneeded files and optimizations for the repository's data structures.
 type Manager interface {
-	// CleanStaleData removes any stale data in the repository.
-	CleanStaleData(context.Context, *localrepo.Repo) error
+	// CleanStaleData removes any stale data in the repository as per the provided configuration.
+	CleanStaleData(ctx context.Context, repo *localrepo.Repo, cfg CleanStaleDataConfig) error
 	// OptimizeRepository optimizes the repository's data structures such that it can be more
 	// efficiently served.
 	OptimizeRepository(context.Context, *localrepo.Repo, ...OptimizeRepositoryOption) error
diff --git a/internal/git/housekeeping/optimize_repository.go b/internal/git/housekeeping/optimize_repository.go
index 728457f4f..ec010d33a 100644
--- a/internal/git/housekeeping/optimize_repository.go
+++ b/internal/git/housekeeping/optimize_repository.go
@@ -158,7 +158,7 @@ func optimizeRepository(
 	}()
 
 	timer := prometheus.NewTimer(m.tasksLatency.WithLabelValues("clean-stale-data"))
-	if err := m.CleanStaleData(ctx, repo); err != nil {
+	if err := m.CleanStaleData(ctx, repo, DefaultStaleDataCleanup()); err != nil {
 		return fmt.Errorf("could not execute houskeeping: %w", err)
 	}
 	timer.ObserveDuration()
diff --git a/internal/git/objectpool/fetch.go b/internal/git/objectpool/fetch.go
index c4f40825c..36823dd38 100644
--- a/internal/git/objectpool/fetch.go
+++ b/internal/git/objectpool/fetch.go
@@ -11,6 +11,7 @@ import (
 	"github.com/grpc-ecosystem/go-grpc-middleware/logging/logrus/ctxlogrus"
 	"github.com/sirupsen/logrus"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/git"
+	"gitlab.com/gitlab-org/gitaly/v16/internal/git/housekeeping"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/git/localrepo"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/git/stats"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/git/updateref"
@@ -34,7 +35,7 @@ func (o *ObjectPool) FetchFromOrigin(ctx context.Context, origin *localrepo.Repo
 		return fmt.Errorf("computing origin repo's path: %w", err)
 	}
 
-	if err := o.housekeepingManager.CleanStaleData(ctx, o.Repo); err != nil {
+	if err := o.housekeepingManager.CleanStaleData(ctx, o.Repo, housekeeping.DefaultStaleDataCleanup()); err != nil {
 		return fmt.Errorf("cleaning stale data: %w", err)
 	}