integration/rpctest: do not ignore exit errors and allow harness restart

Now, Harness TearDown procedure returns an error if the btcd process
stopped with error status code. Also, SkipCleanup option was added to
TearDown, so now we can write tests that restart the node while keeping
the state.

Author: allocz

integration/rpctest/node.go

@@ -5,6 +5,7 @@
 package rpctest
 
 import (
+	"errors"
 	"fmt"
 	"log"
 	"os"
@@ -179,12 +180,23 @@ func newNode(config *nodeConfig, dataDir string) (*node, error) {
 	}, nil
 }
 
+func (n *node) started() bool {
+	if n.pidFile != "" {
+		return true
+	}
+	return false
+}
+
 // start creates a new btcd process, and writes its pid in a file reserved for
 // recording the pid of the launched process. This file can be used to
 // terminate the process in case of a hang, or panic. In the case of a failing
 // test case, or panic, it is important that the process be stopped via stop(),
 // otherwise, it will persist unless explicitly killed.
 func (n *node) start() error {
+	if n.started() {
+		return fmt.Errorf("node already started")
+	}
+
 	if err := n.cmd.Start(); err != nil {
 		return err
 	}
@@ -208,31 +220,39 @@ func (n *node) start() error {
 
 // stop interrupts the running btcd process process, and waits until it exits
 // properly. On windows, interrupt is not supported, so a kill signal is used
-// instead
+// instead.
 func (n *node) stop() error {
+	var multiErr error
+
 	if n.cmd == nil || n.cmd.Process == nil {
-		// return if not properly initialized
-		// or error starting the process
+		// return if not properly initialized or error starting the
+		// process.
 		return nil
 	}
-	defer n.cmd.Wait()
+
 	if runtime.GOOS == "windows" {
-		return n.cmd.Process.Signal(os.Kill)
+		multiErr = errors.Join(n.cmd.Process.Signal(os.Kill), multiErr)
+	} else {
+		multiErr = errors.Join(n.cmd.Process.Signal(os.Interrupt),
+			multiErr)
 	}
-	return n.cmd.Process.Signal(os.Interrupt)
+
+	multiErr = errors.Join(n.cmd.Wait(), multiErr)
+
+	return multiErr
 }
 
 // cleanup cleanups process and args files. The file housing the pid of the
 // created process will be deleted, as well as any directories created by the
 // process.
 func (n *node) cleanup() error {
-	if n.pidFile != "" {
-		if err := os.Remove(n.pidFile); err != nil {
-			log.Printf("unable to remove file %s: %v", n.pidFile,
-				err)
-		}
+	if err := os.Remove(n.pidFile); err != nil {
+		log.Printf("unable to remove file %s: %v", n.pidFile, err)
 	}
 
+	n.pidFile = ""
+	n.cmd = n.config.command()
+
 	// Since the node's main data directory is passed in to the node config,
 	// it isn't our responsibility to clean it up. So we're done after
 	// removing the pid file.
@@ -242,13 +262,24 @@ func (n *node) cleanup() error {
 // shutdown terminates the running btcd process, and cleans up all
 // file/directories created by node.
 func (n *node) shutdown() error {
-	if err := n.stop(); err != nil {
-		return err
+	var multiErr error
+
+	if !n.started() {
+		return nil
 	}
+
+	exitErr := &exec.ExitError{}
+	if err := n.stop(); err != nil && errors.As(err, &exitErr) {
+		multiErr = errors.Join(err, multiErr)
+	} else if err != nil {
+		return errors.Join(err, multiErr)
+	}
+
 	if err := n.cleanup(); err != nil {
-		return err
+		return errors.Join(err, multiErr)
 	}
-	return nil
+
+	return multiErr
 }
 
 // genCertPair generates a key/cert pair to the paths provided.

integration/rpctest/rpc_harness.go

@@ -5,10 +5,12 @@
 package rpctest
 
 import (
+	"errors"
 	"fmt"
 	"math/rand/v2"
 	"net"
 	"os"
+	"os/exec"
 	"path/filepath"
 	"strconv"
 	"sync"
@@ -276,7 +278,7 @@ func New(opts ...HarnessOpts) (*Harness, error) {
 // goroutine as they are not concurrent safe.
 func (h *Harness) SetUp(createTestChain bool, numMatureOutputs uint32) error {
 	// Start the btcd node itself. This spawns a new process which will be
-	// managed
+	// managed.
 	if err := h.node.start(); err != nil {
 		return fmt.Errorf("error starting node: %w", err)
 	}
@@ -328,11 +330,13 @@ func (h *Harness) SetUp(createTestChain bool, numMatureOutputs uint32) error {
 	return nil
 }
 
-// tearDown stops the running rpc test instance.  All created processes are
-// killed, and temporary directories removed.
+// tearDown stops the running rpc test instance. All created processes are
+// killed, and temporary directories removed if skipCleanup is false.
 //
 // This function MUST be called with the harness state mutex held (for writes).
-func (h *Harness) tearDown() error {
+func (h *Harness) tearDown(skipCleanup bool) error {
+	var multiErr error
+
 	if h.Client != nil {
 		h.Client.Shutdown()
 		h.Client.WaitForShutdown()
@@ -343,29 +347,45 @@ func (h *Harness) tearDown() error {
 		h.BatchClient.WaitForShutdown()
 	}
 
-	if err := h.node.shutdown(); err != nil {
-		return err
+	// In the case of exit errors we still perform the cleanup, but also
+	// return the exit error to the caller.
+	exitErr := &exec.ExitError{}
+	if err := h.node.shutdown(); err != nil && errors.As(err, &exitErr) {
+		multiErr = errors.Join(err, multiErr)
+	} else if err != nil {
+		return errors.Join(err, multiErr)
 	}
 
-	if err := os.RemoveAll(h.testNodeDir); err != nil {
-		return err
+	if !skipCleanup {
+		if err := os.RemoveAll(h.testNodeDir); err != nil {
+			return errors.Join(err, multiErr)
+		}
+
+		delete(testInstances, h.testNodeDir)
 	}
 
-	delete(testInstances, h.testNodeDir)
+	return multiErr
+}
 
-	return nil
+type HTearDownOpts struct {
+	SkipCleanup bool
 }
 
 // TearDown stops the running rpc test instance. All created processes are
 // killed, and temporary directories removed.
 //
 // NOTE: This method and SetUp should always be called from the same goroutine
 // as they are not concurrent safe.
-func (h *Harness) TearDown() error {
+func (h *Harness) TearDown(opts ...HTearDownOpts) error {
 	harnessStateMtx.Lock()
 	defer harnessStateMtx.Unlock()
 
-	return h.tearDown()
+	var o HTearDownOpts
+	if len(opts) > 0 {
+		o = opts[0]
+	}
+
+	return h.tearDown(o.SkipCleanup)
 }
 
 // connectRPCClient attempts to establish an RPC connection to the created btcd

integration/rpctest/rpc_harness_test.go

@@ -546,6 +546,62 @@ func testMemWalletLockedOutputs(r *Harness, t *testing.T) {
 	}
 }
 
+func testSkipCleanup(_ *Harness, t *testing.T) {
+	h, err := New()
+	if err != nil {
+		t.Fatal(err)
+	}
+	if err := h.SetUp(true, 1); err != nil {
+		t.Fatal(err)
+	}
+	defer h.TearDown()
+	count, err := h.Client.GetBlockCount()
+	if err != nil {
+		t.Fatal(err)
+	}
+	if count != 101 {
+		t.Fatalf("unexpected blockcount %d", count)
+	}
+
+	// Verify that the state is kept between restarts.
+	for range 3 {
+		if err := h.TearDown(HTearDownOpts{SkipCleanup: true}); err != nil {
+
+			t.Fatal(err)
+		}
+		if err := h.SetUp(false, 0); err != nil {
+			t.Fatal(err)
+		}
+	}
+	count, err = h.Client.GetBlockCount()
+	if err != nil {
+		t.Fatal(err)
+	}
+	if count != 101 {
+		t.Fatalf("unexpected blockcount %d", count)
+	}
+}
+
+func testTearDownReturnsErrorWhenExitCodeIsNotZero(_ *Harness, t *testing.T) {
+	// The invalid flag should make the startup fail.
+	h, err := New(HarnessOpts{ExtraArgs: []string{"--invalidflag=0"}})
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	// Make sure we fail quick.
+	h.ConnectionRetryTimeout = time.Millisecond * 100
+	h.MaxConnRetries = 1
+	if err := h.SetUp(false, 0); err == nil {
+		t.Fatal(err)
+	}
+
+	// We expect the process to exit with non zero status code.
+	if err := h.TearDown(); err == nil {
+		t.Fatal(err)
+	}
+}
+
 var harnessTestCases = []HarnessTestCase{
 	testSendOutputs,
 	testConnectNode,
@@ -556,6 +612,8 @@ var harnessTestCases = []HarnessTestCase{
 	testGenerateAndSubmitBlockWithCustomCoinbaseOutputs,
 	testMemWalletReorg,
 	testMemWalletLockedOutputs,
+	testSkipCleanup,
+	testTearDownReturnsErrorWhenExitCodeIsNotZero,
 }
 
 var mainHarness *Harness

integration/rpctest/utils.go

@@ -140,7 +140,7 @@ func TearDownAll() error {
 	defer harnessStateMtx.Unlock()
 
 	for _, harness := range testInstances {
-		if err := harness.tearDown(); err != nil {
+		if err := harness.tearDown(false); err != nil {
 			return err
 		}
 	}