Merge branch 'ps-clean-packed-refs-new' into 'master'

Stale packed-refs.new files prevents branches from being deleted

Closes #2627

See merge request gitlab-org/gitaly!2206

4 files changed, 110 insertions, 19 deletions

diff --git a/changelogs/unreleased/ps-clean-packed-refs-new.yml b/changelogs/unreleased/ps-clean-packed-refs-new.yml
new file mode 100644
index 000000000..2dd16f127
--- /dev/null
+++ b/changelogs/unreleased/ps-clean-packed-refs-new.yml
@@ -0,0 +1,5 @@
+---
+title: Stale packed-refs.new files prevents branches from being deleted
+merge_request: 2206
+author:
+type: fixed
diff --git a/internal/service/repository/cleanup.go b/internal/service/repository/cleanup.go
index 54469938a..e18e9f401 100644
--- a/internal/service/repository/cleanup.go
+++ b/internal/service/repository/cleanup.go
@@ -48,11 +48,16 @@ func cleanupRepo(ctx context.Context, repo *gitalypb.Repository) error {
 		return status.Errorf(codes.Internal, "Cleanup: cleanDisconnectedWorktrees: %v", err)
 	}
 
-	configLockThreshod := time.Now().Add(-15 * time.Minute)
-	if err := cleanFileLocks(repoPath, configLockThreshod); err != nil {
+	older15min := time.Now().Add(-15 * time.Minute)
+
+	if err := cleanFileLocks(repoPath, older15min); err != nil {
 		return status.Errorf(codes.Internal, "Cleanup: cleanupConfigLock: %v", err)
 	}
 
+	if err := cleanPackedRefsNew(repoPath, older15min); err != nil {
+		return status.Errorf(codes.Internal, "Cleanup: cleanPackedRefsNew: %v", err)
+	}
+
 	return nil
 }
 
@@ -166,3 +171,28 @@ func cleanFileLocks(repoPath string, threshold time.Time) error {
 
 	return nil
 }
+
+func cleanPackedRefsNew(repoPath string, threshold time.Time) error {
+	path := filepath.Join(repoPath, "packed-refs.new")
+
+	fileInfo, err := os.Stat(path)
+	if err != nil {
+		if os.IsNotExist(err) {
+			return nil // file is already gone, nothing to do!
+		}
+		return err
+	}
+
+	if fileInfo.ModTime().After(threshold) {
+		return nil // it is fresh enough
+	}
+
+	if err := os.Remove(path); err != nil {
+		if os.IsNotExist(err) {
+			return nil // file is already gone, nothing to do!
+		}
+		return err
+	}
+
+	return nil
+}
diff --git a/internal/service/repository/cleanup_test.go b/internal/service/repository/cleanup_test.go
index 6b36eae92..7b8256c17 100644
--- a/internal/service/repository/cleanup_test.go
+++ b/internal/service/repository/cleanup_test.go
@@ -31,17 +31,17 @@ func TestCleanupDeletesRefsLocks(t *testing.T) {
 	refsPath := filepath.Join(testRepoPath, "refs")
 
 	keepRefPath := filepath.Join(refsPath, "heads", "keepthis")
-	createFileWithTimes(keepRefPath, freshTime)
+	mustCreateFileWithTimes(t, keepRefPath, freshTime)
 	keepOldRefPath := filepath.Join(refsPath, "heads", "keepthisalso")
-	createFileWithTimes(keepOldRefPath, oldTime)
+	mustCreateFileWithTimes(t, keepOldRefPath, oldTime)
 	keepDeceitfulRef := filepath.Join(refsPath, "heads", " .lock.not-actually-a-lock.lock ")
-	createFileWithTimes(keepDeceitfulRef, oldTime)
+	mustCreateFileWithTimes(t, keepDeceitfulRef, oldTime)
 
 	keepLockPath := filepath.Join(refsPath, "heads", "keepthis.lock")
-	createFileWithTimes(keepLockPath, freshTime)
+	mustCreateFileWithTimes(t, keepLockPath, freshTime)
 
 	deleteLockPath := filepath.Join(refsPath, "heads", "deletethis.lock")
-	createFileWithTimes(deleteLockPath, oldTime)
+	mustCreateFileWithTimes(t, deleteLockPath, oldTime)
 
 	c, err := client.Cleanup(ctx, req)
 	assert.NoError(t, err)
@@ -100,7 +100,7 @@ func TestCleanupDeletesPackedRefsLock(t *testing.T) {
 			lockPath := filepath.Join(testRepoPath, "packed-refs.lock")
 
 			if tc.lockTime != nil {
-				createFileWithTimes(lockPath, *tc.lockTime)
+				mustCreateFileWithTimes(t, lockPath, *tc.lockTime)
 			}
 
 			ctx, cancel := testhelper.Context()
@@ -279,15 +279,71 @@ func TestCleanupFileLocks(t *testing.T) {
 		assert.NoError(t, err)
 
 		// Fresh lock should remain
-		createFileWithTimes(lockPath, freshTime)
+		mustCreateFileWithTimes(t, lockPath, freshTime)
 		_, err = client.Cleanup(ctx, req)
 		assert.NoError(t, err)
 		assert.FileExists(t, lockPath)
 
 		// Old lock should be removed
-		createFileWithTimes(lockPath, oldTime)
+		mustCreateFileWithTimes(t, lockPath, oldTime)
 		_, err = client.Cleanup(ctx, req)
 		assert.NoError(t, err)
 		testhelper.AssertPathNotExists(t, lockPath)
 	}
 }
+
+func TestCleanupDeletesPackedRefsNew(t *testing.T) {
+	serverSocketPath, stop := runRepoServer(t)
+	defer stop()
+
+	client, conn := newRepositoryClient(t, serverSocketPath)
+	defer conn.Close()
+
+	testCases := []struct {
+		desc        string
+		lockTime    *time.Time
+		shouldExist bool
+	}{
+		{
+			desc:        "created recently",
+			lockTime:    &freshTime,
+			shouldExist: true,
+		},
+		{
+			desc:        "exists for too long",
+			lockTime:    &oldTime,
+			shouldExist: false,
+		},
+		{
+			desc:        "nothing to clean up",
+			shouldExist: false,
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.desc, func(t *testing.T) {
+			testRepo, testRepoPath, cleanupFn := testhelper.NewTestRepo(t)
+			defer cleanupFn()
+
+			req := &gitalypb.CleanupRequest{Repository: testRepo}
+			packedRefsNewPath := filepath.Join(testRepoPath, "packed-refs.new")
+
+			if tc.lockTime != nil {
+				mustCreateFileWithTimes(t, packedRefsNewPath, *tc.lockTime)
+			}
+
+			ctx, cancel := testhelper.Context()
+			defer cancel()
+
+			c, err := client.Cleanup(ctx, req)
+			require.NotNil(t, c)
+			require.NoError(t, err)
+
+			if tc.shouldExist {
+				require.FileExists(t, packedRefsNewPath)
+			} else {
+				testhelper.AssertPathNotExists(t, packedRefsNewPath)
+			}
+		})
+	}
+}
diff --git a/internal/service/repository/gc_test.go b/internal/service/repository/gc_test.go
index 1295e3275..256a517fd 100644
--- a/internal/service/repository/gc_test.go
+++ b/internal/service/repository/gc_test.go
@@ -173,17 +173,17 @@ func TestGarbageCollectDeletesRefsLocks(t *testing.T) {
 	// deleted are all due to our *.lock cleanup step before gc runs (since
 	// `git gc` also deletes files from /refs when packing).
 	keepRefPath := filepath.Join(refsPath, "heads", "keepthis")
-	createFileWithTimes(keepRefPath, freshTime)
+	mustCreateFileWithTimes(t, keepRefPath, freshTime)
 	keepOldRefPath := filepath.Join(refsPath, "heads", "keepthisalso")
-	createFileWithTimes(keepOldRefPath, oldTime)
+	mustCreateFileWithTimes(t, keepOldRefPath, oldTime)
 	keepDeceitfulRef := filepath.Join(refsPath, "heads", " .lock.not-actually-a-lock.lock ")
-	createFileWithTimes(keepDeceitfulRef, oldTime)
+	mustCreateFileWithTimes(t, keepDeceitfulRef, oldTime)
 
 	keepLockPath := filepath.Join(refsPath, "heads", "keepthis.lock")
-	createFileWithTimes(keepLockPath, freshTime)
+	mustCreateFileWithTimes(t, keepLockPath, freshTime)
 
 	deleteLockPath := filepath.Join(refsPath, "heads", "deletethis.lock")
-	createFileWithTimes(deleteLockPath, oldTime)
+	mustCreateFileWithTimes(t, deleteLockPath, oldTime)
 
 	c, err := client.GarbageCollect(ctx, req)
 	testhelper.RequireGrpcError(t, err, codes.Internal)
@@ -322,10 +322,10 @@ func TestCleanupInvalidKeepAroundRefs(t *testing.T) {
 	}
 }
 
-func createFileWithTimes(path string, mTime time.Time) {
-	os.MkdirAll(filepath.Dir(path), 0755)
-	ioutil.WriteFile(path, nil, 0644)
-	os.Chtimes(path, mTime, mTime)
+func mustCreateFileWithTimes(t testing.TB, path string, mTime time.Time) {
+	require.NoError(t, os.MkdirAll(filepath.Dir(path), 0755))
+	require.NoError(t, ioutil.WriteFile(path, nil, 0644))
+	require.NoError(t, os.Chtimes(path, mTime, mTime))
 }
 
 func TestGarbageCollectDeltaIslands(t *testing.T) {