Merge branch 'pks-praefect-fix-remove-replication-events-flake' into 'master'

cmd/praefect: Fix flake when testing removal of replication events

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

Merged-by: Patrick Steinhardt <psteinhardt@gitlab.com>
Approved-by: Quang-Minh Nguyen <qmnguyen@gitlab.com>

1 files changed, 15 insertions, 6 deletions

diff --git a/cmd/praefect/subcmd_remove_repository_test.go b/cmd/praefect/subcmd_remove_repository_test.go
index 57c6d1bed..7878a965d 100644
--- a/cmd/praefect/subcmd_remove_repository_test.go
+++ b/cmd/praefect/subcmd_remove_repository_test.go
@@ -338,19 +338,21 @@ func TestRemoveRepository_removeReplicationEvents(t *testing.T) {
 	require.NoError(t, err)
 	require.Equal(t, []uint64{failedEvent.ID}, acknowledgedJobIDs)
 
+	resetCh := make(chan struct{})
 	ticker := helper.NewManualTicker()
-	defer ticker.Stop()
+	ticker.ResetFunc = func() {
+		resetCh <- struct{}{}
+	}
 
 	var wg sync.WaitGroup
 	wg.Add(1)
 	go func() {
 		defer wg.Done()
 
-		// Tick multiple times so that we know that at least one event must have been processed by
-		// the command.
-		ticker.Tick()
-		ticker.Tick()
-		ticker.Tick()
+		// Wait for the system-under-test to execute the `Reset()` function of the ticker
+		// for the first time. This means that the logic-under-test has executed once and
+		// that the processing loop is blocked until we call `Tick()`.
+		<-resetCh
 
 		// Verify that the database now only contains a single job, which is the "in_progress" one.
 		var jobIDs glsql.Uint64Provider
@@ -364,6 +366,13 @@ func TestRemoveRepository_removeReplicationEvents(t *testing.T) {
 		assert.NoError(t, err)
 		assert.Equal(t, []uint64{inProgressEvent.ID}, acknowledgedJobIDs)
 
+		// Trigger the ticker so that the processing loop becomes unblocked. This should
+		// cause us to prune the now-acknowledged job.
+		//
+		// Note that we explicitly don't close the reset channel or try to receive another
+		// message on it. This is done to ensure that we have now deterministically removed
+		// the replication event and that the loop indeed has terminated as expected without
+		// calling `Reset()` on the ticker again.
 		ticker.Tick()
 	}()