From f3abd11884acdcad708e36b9b31a72219ecdba5c Mon Sep 17 00:00:00 2001
From: Seebs <seebs@molecula.com>
Date: Tue, 28 Feb 2023 12:43:27 -0600
Subject: [PATCH] don't use things that instantly exit in a code path tests hit

The testing package is full of subtle magic, and one of the most
subtle is this: t.Logf, etcetera, all write to a buffer which is
then displayed after the test is run. Which means that, if you
exit, the buffer is never displayed. This means that, if a test
case can fail in a way that causes an instant exit, you don't
hit defers, you don't get your log messages, you just get a mysterious
exit of the process.

We have two cases where backup commands were calling log.Fatal
instead of returning an error. The error in question is displayed
correctly and informatively if returned, so we return it.

We also have one case where we were using os.Exit to avoid a
deadlock. Instead, we make the thing that would deadlock
conditional on the test not having failed. In the event that
the test fails, we now print our failure message correctly,
then also report an unclosed cluster. That's fine.
---
 ctl/backup.go     |  3 +--
 ctl/backup_tar.go |  3 +--
 index_test.go     | 29 ++++++++++++++++++-----------
 3 files changed, 20 insertions(+), 15 deletions(-)

diff --git a/ctl/backup.go b/ctl/backup.go
index d12f91f11..248630759 100644
--- a/ctl/backup.go
+++ b/ctl/backup.go
@@ -10,7 +10,6 @@ import (
 	"fmt"
 	"io"
 	"io/fs"
-	"log"
 	"os"
 	"path/filepath"
 	"time"
@@ -549,7 +548,7 @@ func (cmd *BackupCommand) backupShardDataframe(ctx context.Context, indexName st
 	resp, err := client.GetDataframeShard(ctx, indexName, shard)
 	// no error if doesn't exist
 	if err != nil {
-		log.Fatal(err)
+		return fmt.Errorf("getting dataframe: %w", err)
 	}
 	defer resp.Body.Close()
 	if resp.StatusCode == 404 {
diff --git a/ctl/backup_tar.go b/ctl/backup_tar.go
index e8f65d90d..bd07ded09 100644
--- a/ctl/backup_tar.go
+++ b/ctl/backup_tar.go
@@ -8,7 +8,6 @@ import (
 	"encoding/json"
 	"fmt"
 	"io"
-	"log"
 	"os"
 	"path"
 	"path/filepath"
@@ -329,7 +328,7 @@ func (cmd *BackupTarCommand) backupTarShardDataframe(ctx context.Context, tw *ta
 	resp, err := client.GetDataframeShard(ctx, indexName, shard)
 	// no error if doesn't exist
 	if err != nil {
-		log.Fatal(err)
+		return fmt.Errorf("getting dataframe: %w", err)
 	}
 	defer resp.Body.Close()
 	if resp.StatusCode == 404 {
diff --git a/index_test.go b/index_test.go
index 15addd338..6ccc71f62 100644
--- a/index_test.go
+++ b/index_test.go
@@ -6,7 +6,6 @@ import (
 	"context"
 	"fmt"
 	"math/rand"
-	"os"
 	"reflect"
 	"testing"
 	"time"
@@ -258,7 +257,23 @@ func isNotFoundError(err error) bool {
 // For details, check out https://molecula.atlassian.net/browse/CORE-919
 func TestIndex_RecreateFieldOnRestart(t *testing.T) {
 	c := test.MustRunUnsharedCluster(t, 1)
-	defer c.Close()
+	defer func() {
+		// We anticipate a deadlock, and if we hit the deadlock,
+		// closing would ALSO deadlock, so we won't want to do that.
+		//
+		// The alternative, of trying to call os.Exit, prevents us
+		// from reporting anything at all, because testing doesn't
+		// actually display messages until it's done.
+		//
+		// So, if we're bailing because of a fatal error, we don't
+		// try to close the cluster, because Something Went Wrong and
+		// it may well have been a deadlock. We leak one cluster on
+		// a failed test, but we correctly report the test as failed
+		// before also reporting the unclosed resources.
+		if !t.Failed() {
+			c.Close()
+		}
+	}()
 
 	// create index
 	indexName := fmt.Sprintf("idx_%d", rand.Uint64())
@@ -310,15 +325,7 @@ func TestIndex_RecreateFieldOnRestart(t *testing.T) {
 	}()
 	select {
 	case <-time.After(10 * time.Second):
-		// We have to use os.Exit here instead of t.Fatal or panic since
-		// on panic, deferred statements are still ran. Given that
-		// we have deferred cluster.Close(), it deadlocks on the same
-		// issue this test is, well, is testing on.
-		// With os.Exit, the process exits at that point without running the
-		// deferred actions. This is more of a work-around fix to make the
-		// test meaningful on timeout.
-		t.Logf("recreating field took too long")
-		os.Exit(1)
+		t.Fatal("recreating field took too long")
 	case err := <-errCh:
 		if err != nil {
 			t.Fatal(err)