backup: fix MakeBackupDestinationStores cleanup

Previously, `MakeBackupDestinationStores` would not properly cleanup in
the event that an error was encountered during the opening of the
stores. To make matters worse, if the user called `cleanup` in the event
of a non-nil error, we would run into a nil pointer exception.

Epic: None

Release note: None

Author: kev-cao

pkg/backup/backupdest/incrementals.go

@@ -198,25 +198,27 @@ func MakeBackupDestinationStores(
 	mkStore cloud.ExternalStorageFromURIFactory,
 	destinationDirs []string,
 ) ([]cloud.ExternalStorage, func() error, error) {
-	stores := make([]cloud.ExternalStorage, len(destinationDirs))
-	for i := range destinationDirs {
-		store, err := mkStore(ctx, destinationDirs[i], user)
-		if err != nil {
-			return nil, nil, errors.Wrapf(err, "failed to open backup storage location")
-		}
-		stores[i] = store
-	}
-
-	return stores, func() error {
-		// Close all the stores in the returned cleanup function.
+	stores := make([]cloud.ExternalStorage, 0, len(destinationDirs))
+	cleanup := func() error {
 		var combinedErr error
 		for _, store := range stores {
 			if err := store.Close(); err != nil {
 				combinedErr = errors.CombineErrors(combinedErr, err)
 			}
 		}
 		return combinedErr
-	}, nil
+	}
+
+	for _, dir := range destinationDirs {
+		store, err := mkStore(ctx, dir, user)
+		if err != nil {
+			err := errors.Wrapf(err, "failed to open backup storage location")
+			return nil, nil, errors.CombineErrors(err, cleanup())
+		}
+		stores = append(stores, store)
+	}
+
+	return stores, cleanup, nil
 }
 
 // ResolveIncrementalsBackupLocation returns the resolved locations of

pkg/backup/backupdest/incrementals_test.go

@@ -594,3 +594,69 @@ func TestLegacyFindPriorBackups(t *testing.T) {
 		})
 	}
 }
+
+func TestCleanupMakeBackupDestinationStores(t *testing.T) {
+	defer leaktest.AfterTest(t)()
+	// This test ensures that in the event that MakeBackupDestinationStores
+	// encounters an error either during store creation or during cleanup that all
+	// stores that were opened have Close called.
+	ctx := context.Background()
+
+	const maxStores = 10
+	failAfterN := rand.Intn(maxStores + 1)
+
+	stores := make([]*fakeStore, 0, maxStores)
+	mkStore := func(
+		_ context.Context,
+		_ string,
+		_ username.SQLUsername,
+		_ ...cloud.ExternalStorageOption,
+	) (cloud.ExternalStorage, error) {
+		if len(stores) == failAfterN {
+			return nil, fmt.Errorf("simulated failure after %d stores", failAfterN)
+		}
+		s := &fakeStore{closeErrProbability: 0.1}
+		stores = append(stores, s)
+		return s, nil
+	}
+
+	destinations := make([]string, maxStores)
+	_, cleanup, err := backupdest.MakeBackupDestinationStores(
+		ctx, username.RootUserName(), mkStore, destinations,
+	)
+
+	countOpenStores := func() int {
+		count := 0
+		for _, s := range stores {
+			if !s.calledClose {
+				count++
+			}
+		}
+		return count
+	}
+
+	if failAfterN < maxStores {
+		require.Error(t, err)
+		require.Equal(t, 0, countOpenStores())
+	} else {
+		require.NoError(t, err)
+		require.Equal(t, maxStores, countOpenStores())
+		_ = cleanup()
+		require.Equal(t, 0, countOpenStores())
+	}
+}
+
+type fakeStore struct {
+	cloud.ExternalStorage
+	calledClose         bool
+	closeErrProbability float32
+}
+
+// Close() simulates closing the store and probabilistically returns an error.
+func (s *fakeStore) Close() error {
+	s.calledClose = true
+	if rand.Float32() < s.closeErrProbability {
+		return fmt.Errorf("simulated close error")
+	}
+	return nil
+}