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)