From 06102af0f488f662f2afa1ba554097d73d2c582d Mon Sep 17 00:00:00 2001 From: Kirill Blinichkin Date: Mon, 6 Jul 2026 08:09:41 -0500 Subject: [PATCH] fix: handle a failed Start() safely at the node and library layers (#1312) Signed-off-by: kblinichkin --- library/node.go | 9 +++++++-- library/node_start_test.go | 27 +++++++++++++++++++++++++++ waku/v2/node/stop_test.go | 34 ++++++++++++++++++++++++++++++++++ waku/v2/node/wakunode2.go | 6 ++++++ 4 files changed, 74 insertions(+), 2 deletions(-) create mode 100644 library/node_start_test.go create mode 100644 waku/v2/node/stop_test.go diff --git a/library/node.go b/library/node.go index 47eea0f0..91064659 100644 --- a/library/node.go +++ b/library/node.go @@ -261,12 +261,17 @@ func Start(instance *WakuInstance) error { return err } - instance.ctx, instance.cancel = context.WithCancel(context.Background()) + ctx, cancel := context.WithCancel(context.Background()) - if err := instance.node.Start(instance.ctx); err != nil { + if err := instance.node.Start(ctx); err != nil { + cancel() return err } + // Record started state only after node.Start() succeeds, so a failed Start() + // leaves IsStarted() false and Free() will not Stop() a node that never ran. + instance.ctx, instance.cancel = ctx, cancel + if instance.node.DiscV5() != nil { if err := instance.node.DiscV5().Start(context.Background()); err != nil { stop(instance) diff --git a/library/node_start_test.go b/library/node_start_test.go new file mode 100644 index 00000000..3a8a14e1 --- /dev/null +++ b/library/node_start_test.go @@ -0,0 +1,27 @@ +package library + +import ( + "fmt" + "net" + "testing" + + "github.com/stretchr/testify/require" +) + +// TestStartFailureLeavesInstanceStopped checks that a failed Start() leaves the +// instance not-started and Free() safe (here the listen port is already in use). +func TestStartFailureLeavesInstanceStopped(t *testing.T) { + ln, err := net.Listen("tcp", "127.0.0.1:0") + require.NoError(t, err) + defer ln.Close() + addr := ln.Addr().(*net.TCPAddr) + + instance := Init() + cfg := fmt.Sprintf(`{"host":"127.0.0.1","port":%d,"relay":false,"store":false,"discV5":false}`, addr.Port) + require.NoError(t, NewNode(instance, cfg)) + + err = Start(instance) + require.Error(t, err, "Start should fail when the port is in use") + require.False(t, IsStarted(instance), "instance must not report started after a failed Start") + require.NotPanics(t, func() { _ = Free(instance) }) +} diff --git a/waku/v2/node/stop_test.go b/waku/v2/node/stop_test.go new file mode 100644 index 00000000..217ecee9 --- /dev/null +++ b/waku/v2/node/stop_test.go @@ -0,0 +1,34 @@ +package node + +import ( + "context" + "net" + "testing" + + "github.com/ethereum/go-ethereum/crypto" + "github.com/stretchr/testify/require" + "github.com/waku-org/go-waku/tests" +) + +// TestStopAfterFailedStart checks that Stop() is safe when Start() failed before +// the host was created (here the listen port is already in use). +func TestStopAfterFailedStart(t *testing.T) { + // Occupy a TCP port so libp2p host creation fails to listen on it. + ln, err := net.Listen("tcp", "127.0.0.1:0") + require.NoError(t, err) + defer ln.Close() + addr := ln.Addr().(*net.TCPAddr) + + key, err := tests.RandomHex(32) + require.NoError(t, err) + prvKey, err := crypto.HexToECDSA(key) + require.NoError(t, err) + + wakuNode, err := New(WithPrivateKey(prvKey), WithHostAddress(addr)) + require.NoError(t, err) + + err = wakuNode.Start(context.Background()) + require.Error(t, err, "Start should fail when the port is in use") + + require.NotPanics(t, func() { wakuNode.Stop() }) +} diff --git a/waku/v2/node/wakunode2.go b/waku/v2/node/wakunode2.go index dc9a9c4f..40e69473 100644 --- a/waku/v2/node/wakunode2.go +++ b/waku/v2/node/wakunode2.go @@ -533,6 +533,12 @@ func (w *WakuNode) Stop() { w.stopWithTimeout(w.bcaster.Stop, "bcaster", 5*time.Second) + // Start() can fail before the host is created (e.g. port in use), leaving a + // partially-started node; guard so Stop() cannot nil-dereference w.host. + if w.host == nil { + return + } + defer w.connectionNotif.Close() defer w.addressChangesSub.Close()