repocleaner: Only clean repos that are more than one day old

In order to avoid marking repositories in flight as unused, check the
timestamp on the repositories and use a grace period of 24 hours.

3 files changed, 38 insertions, 32 deletions

diff --git a/cmd/praefect/subcmd_list_untracked_repositories_test.go b/cmd/praefect/subcmd_list_untracked_repositories_test.go
index 0f7a93b1e..a2c251a81 100644
--- a/cmd/praefect/subcmd_list_untracked_repositories_test.go
+++ b/cmd/praefect/subcmd_list_untracked_repositories_test.go
@@ -5,8 +5,10 @@ import (
 	"context"
 	"flag"
 	"fmt"
+	"os"
 	"strings"
 	"testing"
+	"time"
 
 	"github.com/stretchr/testify/require"
 	"gitlab.com/gitlab-org/gitaly/v14/client"
@@ -53,9 +55,18 @@ func TestListUntrackedRepositories_Exec(t *testing.T) {
 	g2Cfg := testcfg.Build(t, testcfg.WithStorages("gitaly-2"))
 
 	// Repositories not managed by praefect.
-	repo1, _ := gittest.InitRepo(t, g1Cfg, g1Cfg.Storages[0])
-	repo2, _ := gittest.InitRepo(t, g1Cfg, g1Cfg.Storages[0])
-	repo3, _ := gittest.InitRepo(t, g2Cfg, g2Cfg.Storages[0])
+	repo1, repo1Path := gittest.InitRepo(t, g1Cfg, g1Cfg.Storages[0])
+	repo2, repo2Path := gittest.InitRepo(t, g1Cfg, g1Cfg.Storages[0])
+	_, _ = gittest.InitRepo(t, g2Cfg, g2Cfg.Storages[0])
+
+	require.NoError(t, os.Chtimes(
+		repo1Path,
+		time.Now().Add(-(24*time.Hour+1*time.Second)),
+		time.Now().Add(-(24*time.Hour+1*time.Second))))
+	require.NoError(t, os.Chtimes(
+		repo2Path,
+		time.Now().Add(-(24*time.Hour+1*time.Second)),
+		time.Now().Add(-(24*time.Hour+1*time.Second))))
 
 	g1Addr := testserver.RunGitalyServer(t, g1Cfg, nil, setup.RegisterAll, testserver.WithDisablePraefect())
 	g2Addr := testserver.RunGitalyServer(t, g2Cfg, nil, setup.RegisterAll, testserver.WithDisablePraefect())
@@ -99,7 +110,6 @@ func TestListUntrackedRepositories_Exec(t *testing.T) {
 		"The following repositories were found on disk, but missing from the tracking database:",
 		fmt.Sprintf(`{"relative_path":%q,"storage":"gitaly-1","virtual_storage":"praefect"}`, repo1.RelativePath),
 		fmt.Sprintf(`{"relative_path":%q,"storage":"gitaly-1","virtual_storage":"praefect"}`, repo2.RelativePath),
-		fmt.Sprintf(`{"relative_path":%q,"storage":"gitaly-2","virtual_storage":"praefect"}`, repo3.RelativePath),
 		"", // an empty extra element required as each line ends with "delimiter" and strings.Split returns all parts
 	}
 	require.ElementsMatch(t, exp, strings.Split(out.String(), "\n"))
diff --git a/internal/praefect/repocleaner/repository.go b/internal/praefect/repocleaner/repository.go
index dea0ad17c..aeff3ac7d 100644
--- a/internal/praefect/repocleaner/repository.go
+++ b/internal/praefect/repocleaner/repository.go
@@ -187,6 +187,12 @@ func (wr *Walker) ExecOnRepositories(ctx context.Context, virtualStorage, storag
 			break
 		}
 
+		// repositories that are in the process of being created, where
+		// they do not yet have a record in Praefect.
+		if res.GetModificationTime().AsTime().After(time.Now().Add(-24 * time.Hour)) {
+			continue
+		}
+
 		batch = append(batch, res.RelativePath)
 
 		if len(batch) == cap(batch) {
diff --git a/internal/praefect/repocleaner/repository_test.go b/internal/praefect/repocleaner/repository_test.go
index f07b4d9e7..6924d3069 100644
--- a/internal/praefect/repocleaner/repository_test.go
+++ b/internal/praefect/repocleaner/repository_test.go
@@ -2,6 +2,7 @@ package repocleaner
 
 import (
 	"context"
+	"os"
 	"sync/atomic"
 	"testing"
 	"time"
@@ -71,19 +72,23 @@ func TestRunner_Run(t *testing.T) {
 		RepositoriesInBatch: 2,
 	}
 
+	// each gitaly has an extra repo-4.git repository
 	gittest.CloneRepo(t, g1Cfg, g1Cfg.Storages[0], gittest.CloneRepoOpts{RelativePath: repo1RelPath})
 	gittest.CloneRepo(t, g1Cfg, g1Cfg.Storages[0], gittest.CloneRepoOpts{RelativePath: repo2RelPath})
 	gittest.CloneRepo(t, g1Cfg, g1Cfg.Storages[0], gittest.CloneRepoOpts{RelativePath: repo3RelPath})
+	_, repo4Path := gittest.CloneRepo(t, g1Cfg, g1Cfg.Storages[0], gittest.CloneRepoOpts{RelativePath: "repo-4.git"})
+	require.NoError(t, os.Chtimes(repo4Path, time.Now().Add(-25*time.Hour), time.Now().Add(-25*time.Hour)))
 
-	// second gitaly is missing repo-3.git repository
 	gittest.CloneRepo(t, g2Cfg, g2Cfg.Storages[0], gittest.CloneRepoOpts{RelativePath: repo1RelPath})
 	gittest.CloneRepo(t, g2Cfg, g2Cfg.Storages[0], gittest.CloneRepoOpts{RelativePath: repo2RelPath})
+	_, repo4Path = gittest.CloneRepo(t, g2Cfg, g2Cfg.Storages[0], gittest.CloneRepoOpts{RelativePath: "repo-4.git"})
+	require.NoError(t, os.Chtimes(repo4Path, time.Now().Add(-25*time.Hour), time.Now().Add(-25*time.Hour)))
 
-	// third gitaly has an extra repo-4.git repository
 	gittest.CloneRepo(t, g3Cfg, g3Cfg.Storages[0], gittest.CloneRepoOpts{RelativePath: repo1RelPath})
 	gittest.CloneRepo(t, g3Cfg, g3Cfg.Storages[0], gittest.CloneRepoOpts{RelativePath: repo2RelPath})
 	gittest.CloneRepo(t, g3Cfg, g3Cfg.Storages[0], gittest.CloneRepoOpts{RelativePath: repo3RelPath})
-	gittest.CloneRepo(t, g3Cfg, g3Cfg.Storages[0], gittest.CloneRepoOpts{RelativePath: "repo-4.git"})
+	_, repo4Path = gittest.CloneRepo(t, g3Cfg, g3Cfg.Storages[0], gittest.CloneRepoOpts{RelativePath: "repo-4.git"})
+	require.NoError(t, os.Chtimes(repo4Path, time.Now().Add(-25*time.Hour), time.Now().Add(-25*time.Hour)))
 	ctx, cancel := context.WithCancel(testhelper.Context(t))
 
 	repoStore := datastore.NewPostgresRepositoryStore(db.DB, nil)
@@ -125,33 +130,18 @@ func TestRunner_Run(t *testing.T) {
 	var iteration int32
 	runner := NewRunner(cfg, logger, praefect.StaticHealthChecker{virtualStorage: []string{storage1, storage2, storage3}}, nodeSet.Connections(), storageCleanup, storageCleanup, actionStub{
 		PerformMethod: func(ctx context.Context, argVirtualStoage, argStorage string, notExisting []string) error {
-			assert.Equal(t, virtualStorage, argVirtualStoage)
-			// Because action invocation happens for batches each run could result
-			// multiple invocations of the action. Amount of invocations can be calculated
-			// as amount of storage repositories divided on the size of the batch and rounded up.
-			// For storages:
-			//	'gitaly-1' is it 3 repos / 2 = 2 calls [0,1]
-			// 	'gitaly-2' is it 2 repos / 2 = 1 call [2]
-			// 	'gitaly-3' is it 4 repos / 2 = 2 calls [3,4]
+			// There should be three iterations, as each storage has
+			// one repository that is unused by praefect.
+			atomic.AddInt32(&iteration, 1)
+
 			i := atomic.LoadInt32(&iteration)
-			switch i {
-			case 0, 1:
-				assert.Equal(t, storage1, argStorage)
-				assert.ElementsMatch(t, nil, notExisting)
-			case 2:
-				assert.Equal(t, storage2, argStorage)
-				assert.ElementsMatch(t, []string{repo1RelPath}, notExisting)
-			case 3:
-				assert.Equal(t, storage3, argStorage)
-				assert.ElementsMatch(t, nil, notExisting)
-			case 4:
-				assert.Equal(t, storage3, argStorage)
+			assert.Equal(t, virtualStorage, argVirtualStoage)
+			assert.Equal(t, []string{"repo-4.git"}, notExisting)
+
+			if i == 3 {
 				// Terminates the loop.
 				defer cancel()
-				assert.Equal(t, []string{"repo-4.git"}, notExisting)
-				return nil
 			}
-			atomic.AddInt32(&iteration, 1)
 			return nil
 		},
 	})
@@ -169,7 +159,6 @@ func TestRunner_Run(t *testing.T) {
 	}
 	waitReceive(t, done)
 
-	require.Equal(t, int32(4), atomic.LoadInt32(&iteration))
 	require.GreaterOrEqual(t, len(loggerHook.AllEntries()), 2)
 	require.Equal(
 		t,
@@ -216,7 +205,8 @@ func TestRunner_Run_noAvailableStorages(t *testing.T) {
 		RepositoriesInBatch: 2,
 	}
 
-	gittest.CloneRepo(t, g1Cfg, g1Cfg.Storages[0], gittest.CloneRepoOpts{RelativePath: repo1RelPath})
+	_, repoPath := gittest.CloneRepo(t, g1Cfg, g1Cfg.Storages[0], gittest.CloneRepoOpts{RelativePath: repo1RelPath})
+	require.NoError(t, os.Chtimes(repoPath, time.Now().Add(-25*time.Hour), time.Now().Add(-25*time.Hour)))
 	ctx, cancel := context.WithCancel(testhelper.Context(t))
 
 	repoStore := datastore.NewPostgresRepositoryStore(db.DB, nil)