Merge branch 'backup_goroutine_leak' into 'master'

Ensure backup.Sink get cleaned up properly

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

Merged-by: Quang-Minh Nguyen <qmnguyen@gitlab.com>
Approved-by: Quang-Minh Nguyen <qmnguyen@gitlab.com>
Reviewed-by: Quang-Minh Nguyen <qmnguyen@gitlab.com>
Reviewed-by: John Cai <jcai@gitlab.com>
Co-authored-by: James Fargher <jfargher@gitlab.com>

5 files changed, 21 insertions, 3 deletions

diff --git a/internal/backup/backup.go b/internal/backup/backup.go
index 78caa51f3..c06a17110 100644
--- a/internal/backup/backup.go
+++ b/internal/backup/backup.go
@@ -26,6 +26,7 @@ var (
 
 // Sink is an abstraction over the real storage used for storing/restoring backups.
 type Sink interface {
+	io.Closer
 	// GetWriter saves the written data to relativePath. It is the callers
 	// responsibility to call Close and check any subsequent errors.
 	GetWriter(ctx context.Context, relativePath string) (io.WriteCloser, error)
diff --git a/internal/backup/backup_test.go b/internal/backup/backup_test.go
index 70cea1550..81fb28d77 100644
--- a/internal/backup/backup_test.go
+++ b/internal/backup/backup_test.go
@@ -48,6 +48,8 @@ func TestManager_RemoveAllRepositories(t *testing.T) {
 
 	backupRoot := testhelper.TempDir(t)
 	sink := backup.NewFilesystemSink(backupRoot)
+	defer testhelper.MustClose(t, sink)
+
 	locator, err := backup.ResolveLocator("pointer", sink)
 	require.NoError(t, err)
 
@@ -169,6 +171,8 @@ func TestManager_Create(t *testing.T) {
 				customHooksPath := joinBackupPath(t, backupRoot, vanityRepo, backupID, "001.custom_hooks.tar")
 
 				sink := backup.NewFilesystemSink(backupRoot)
+				defer testhelper.MustClose(t, sink)
+
 				locator, err := backup.ResolveLocator("pointer", sink)
 				require.NoError(t, err)
 
@@ -335,6 +339,8 @@ func TestManager_Create_incremental(t *testing.T) {
 				bundlePath := joinBackupPath(t, backupRoot, repo, backupID, tc.expectedIncrement+".bundle")
 
 				sink := backup.NewFilesystemSink(backupRoot)
+				defer testhelper.MustClose(t, sink)
+
 				locator, err := backup.ResolveLocator("pointer", sink)
 				require.NoError(t, err)
 
@@ -587,6 +593,8 @@ func TestManager_Restore_latest(t *testing.T) {
 							repo, expectedChecksum := tc.setup(t)
 
 							sink := backup.NewFilesystemSink(backupRoot)
+							defer testhelper.MustClose(t, sink)
+
 							locator, err := backup.ResolveLocator(locatorName, sink)
 							require.NoError(t, err)
 
@@ -776,6 +784,8 @@ func TestManager_Restore_specific(t *testing.T) {
 					repo, expectedChecksum := tc.setup(t)
 
 					sink := backup.NewFilesystemSink(backupRoot)
+					defer testhelper.MustClose(t, sink)
+
 					locator, err := backup.ResolveLocator("pointer", sink)
 					require.NoError(t, err)
 
@@ -841,6 +851,8 @@ func TestManager_CreateRestore_contextServerInfo(t *testing.T) {
 	defer testhelper.MustClose(t, pool)
 
 	sink := backup.NewFilesystemSink(backupRoot)
+	defer testhelper.MustClose(t, sink)
+
 	locator, err := backup.ResolveLocator("pointer", sink)
 	require.NoError(t, err)
 
diff --git a/internal/backup/filesystem_sink.go b/internal/backup/filesystem_sink.go
index 6eb86c06d..6d68502db 100644
--- a/internal/backup/filesystem_sink.go
+++ b/internal/backup/filesystem_sink.go
@@ -54,3 +54,8 @@ func (fs *FilesystemSink) GetReader(ctx context.Context, relativePath string) (i
 	}
 	return f, nil
 }
+
+// Close is a no-op to implement the Sink interface
+func (fs *FilesystemSink) Close() error {
+	return nil
+}
diff --git a/internal/backup/sink_test.go b/internal/backup/sink_test.go
index 24ed35ddb..0c40ee6ff 100644
--- a/internal/backup/sink_test.go
+++ b/internal/backup/sink_test.go
@@ -104,6 +104,8 @@ func TestResolveSink(t *testing.T) {
 				require.EqualError(t, err, tc.errMsg)
 				return
 			}
+			defer testhelper.MustClose(t, sink)
+
 			tc.verify(t, sink)
 		})
 	}
diff --git a/internal/backup/testhelper_test.go b/internal/backup/testhelper_test.go
index 2ef254a27..53f50c4ae 100644
--- a/internal/backup/testhelper_test.go
+++ b/internal/backup/testhelper_test.go
@@ -7,7 +7,5 @@ import (
 )
 
 func TestMain(m *testing.M) {
-	// gocloud.dev/blob leaks the HTTP connection even if we make sure to close all buckets.
-	//nolint:staticcheck
-	testhelper.Run(m, testhelper.WithDisabledGoroutineChecker())
+	testhelper.Run(m)
 }