From 2b9016b83888a05e34ae0bdd5afb6cdd69d92dff Mon Sep 17 00:00:00 2001
From: Karthik Nayak <knayak@gitlab.com>
Date: Fri, 2 Jun 2023 12:00:55 +0200
Subject: [PATCH] housekeeping: Allow context cancellation of git-pack-refs(1)

We can add inhibitors from running git-pack-refs(1) via the
`addPackRefsInhibitor()` function. But if there is a ongoing
git-pack-refs(1), we are blocked until it finishes.

We instead use a context and propagate cancellation via the context.
This allows the inhibitors to take priority and not be blocked over
git-pack-refs(1).
---
 internal/git/housekeeping/manager.go          | 19 ++++++++++++++++---
 .../git/housekeeping/optimize_repository.go   |  2 +-
 .../housekeeping/optimize_repository_test.go  |  6 +++++-
 3 files changed, 22 insertions(+), 5 deletions(-)

diff --git a/internal/git/housekeeping/manager.go b/internal/git/housekeeping/manager.go
index d73152182b..c7804432aa 100644
--- a/internal/git/housekeeping/manager.go
+++ b/internal/git/housekeeping/manager.go
@@ -32,6 +32,9 @@ type repositoryState struct {
 	// packRefsDone is a channel used to denote when the ongoing (if any) call to packRefsIfNeeded
 	// is completed.
 	packRefsDone chan struct{}
+	// packRefsCancel is the context cancellation function which can be used to cancel any
+	// running git-pack-refs(1).
+	packRefsCancel context.CancelFunc
 	// packRefsInhibitors keeps a count of the number of inhibitors on running packRefsIfNeeded.
 	packRefsInhibitors int32
 	// isRunning is used to indicate if housekeeping is running.
@@ -139,6 +142,12 @@ func (s *repositoryStates) addPackRefsInhibitor(ctx context.Context, repoPath st
 			break
 		}
 
+		// Instead of waiting for git-pack-refs to finish, we ask it to
+		// stop.
+		if state.packRefsCancel != nil {
+			state.packRefsCancel()
+		}
+
 		state.Unlock()
 
 		select {
@@ -174,7 +183,7 @@ func (s *repositoryStates) addPackRefsInhibitor(ctx context.Context, repoPath st
 // is at least one inhibitors then we return false. If there are no inhibitors, we setup the `packRefsDone`
 // channel to denote when `git-pack-refs(1)` finishes, this is handled when the caller calls the
 // cleanup function returned by this function.
-func (s *repositoryStates) tryRunningPackRefs(repoPath string) (successful bool, _ func()) {
+func (s *repositoryStates) tryRunningPackRefs(ctx context.Context, repoPath string) (successful bool, _ context.Context, _ func()) {
 	state, cleanup := s.getState(repoPath)
 	defer func() {
 		if !successful {
@@ -186,12 +195,15 @@ func (s *repositoryStates) tryRunningPackRefs(repoPath string) (successful bool,
 	defer state.Unlock()
 
 	if state.packRefsInhibitors > 0 || state.packRefsDone != nil {
-		return false, nil
+		return false, ctx, nil
 	}
 
 	state.packRefsDone = make(chan struct{})
 
-	return true, func() {
+	ctx, cancel := context.WithCancel(ctx)
+	state.packRefsCancel = cancel
+
+	return true, ctx, func() {
 		defer cleanup()
 
 		state.Lock()
@@ -199,6 +211,7 @@ func (s *repositoryStates) tryRunningPackRefs(repoPath string) (successful bool,
 
 		close(state.packRefsDone)
 		state.packRefsDone = nil
+		state.packRefsCancel = nil
 	}
 }
 
diff --git a/internal/git/housekeeping/optimize_repository.go b/internal/git/housekeeping/optimize_repository.go
index ec010d33a2..af732e1380 100644
--- a/internal/git/housekeeping/optimize_repository.go
+++ b/internal/git/housekeeping/optimize_repository.go
@@ -287,7 +287,7 @@ func (m *RepositoryManager) packRefsIfNeeded(ctx context.Context, repo *localrep
 	}
 
 	// If there are any inhibitors, we don't run git-pack-refs(1).
-	ok, cleanup := m.repositoryStates.tryRunningPackRefs(path)
+	ok, ctx, cleanup := m.repositoryStates.tryRunningPackRefs(ctx, path)
 	if !ok {
 		return false, nil
 	}
diff --git a/internal/git/housekeeping/optimize_repository_test.go b/internal/git/housekeeping/optimize_repository_test.go
index 419e039ba7..36815c73db 100644
--- a/internal/git/housekeeping/optimize_repository_test.go
+++ b/internal/git/housekeeping/optimize_repository_test.go
@@ -389,10 +389,14 @@ func TestPackRefsIfNeeded(t *testing.T) {
 				b.block["pack-refs"] = ch
 
 				return setupData{
-					refsShouldBePacked: true,
+					// addPackRefsInhibitor cancels the context of the git-pack-refs(1)
+					refsShouldBePacked: false,
 					shouldPackReferences: func(_ context.Context) bool {
 						return true
 					},
+					errExpected: fmt.Errorf("packing refs: %w, stderr: %q",
+						fmt.Errorf("getting Git version: %w",
+							fmt.Errorf("spawning version command: %w", context.Canceled)), ""),
 				}
 			},
 		},
-- 
GitLab