From 488266888f9ccc76955311897c2c03a328acfc8c Mon Sep 17 00:00:00 2001 From: Fabiana Cecin Date: Fri, 19 Jun 2026 12:09:36 -0300 Subject: [PATCH] fix: harden LogosDelivery facade lifecycle * MessagingClient.new: return a Result via a new newMessagingClient factory instead of quit(QuitFailure) on a recoverable SendService failure; no longer aborts the FFI host (restores master's behaviour). Callers (facade, FFI mount, tests) updated to the factory. * LogosDelivery.stop(): best-effort run-to-completion; stop and nil every component even if a prior one fails, aggregating errors (was: early return that leaked the messaging client and the node). * LogosDelivery.getNodeInfo(): nil-guard self.waku like the sibling getters (was: nil-deref before start / after stop). --- logos_delivery/logos_delivery.nim | 29 +++++++++++-------- logos_delivery/messaging/messaging_client.nim | 23 ++++++++++----- logos_delivery/waku/api/api.nim | 3 +- tests/node/test_wakunode_health_monitor.nim | 8 +++-- 4 files changed, 40 insertions(+), 23 deletions(-) diff --git a/logos_delivery/logos_delivery.nim b/logos_delivery/logos_delivery.nim index 5ddb2dec5..5e0dcbc07 100644 --- a/logos_delivery/logos_delivery.nim +++ b/logos_delivery/logos_delivery.nim @@ -12,6 +12,7 @@ import results, chronos import std/options # some()/Option for WakuNodeConf.mode import std/json # newJArray/newJObject/%*/pretty in getAvailableConfigs +import std/strutils import brokers/broker_context, brokers/broker_interface, brokers/broker_implement import logos_delivery/api/logos_delivery_interface as logosdelivery_iface @@ -48,10 +49,8 @@ proc createNode(conf: WakuNodeConf): Future[Result[Waku, string]] {.async.} = return ok(wakuRes) proc initMessagingClient(self: LogosDelivery): Result[MessagingClient, string] = - return ok( - MessagingClient.createUnderContext( - globalBrokerContext(), self.waku.conf.p2pReliability, self.waku.node - ) + newMessagingClient( + globalBrokerContext(), self.waku.conf.p2pReliability, self.waku.node ) proc initReliableChannelManager( @@ -147,32 +146,36 @@ BrokerImplement LogosDelivery of LogosDeliveryInterface: return err("initialize failed: " & e.msg) method stop(self: LogosDelivery): Future[Result[void, string]] {.async.} = + var errs: seq[string] + if not self.reliableChannelManager.isNil(): try: await self.reliableChannelManager.stop() self.reliableChannelManager.close() - self.reliableChannelManager = nil except CatchableError as e: - return err("ReliableChannelManager stop failed: " & e.msg) + errs.add("ReliableChannelManager stop failed: " & e.msg) + self.reliableChannelManager = nil if not self.messagingClient.isNil(): try: await self.messagingClient.stop() self.messagingClient.close() - self.messagingClient = nil except CatchableError as e: - return err("MessagingClient stop failed: " & e.msg) + errs.add("MessagingClient stop failed: " & e.msg) + self.messagingClient = nil if not self.waku.isNil(): try: - (await self.waku.stop()).isOkOr: - return err("Node stop failed: " & $error) # `Waku` is a plain ref object (not a BrokerImplement) — `stop()` is its # only teardown; there is no `close()`. - self.waku = nil + (await self.waku.stop()).isOkOr: + errs.add("Node stop failed: " & $error) except CatchableError as e: - return err("Node stop failed: " & e.msg) + errs.add("Node stop failed: " & e.msg) + self.waku = nil + if errs.len > 0: + return err(errs.join("; ")) return ok() method shutdown(self: LogosDelivery): Future[Result[void, string]] {.async.} = @@ -181,6 +184,8 @@ BrokerImplement LogosDelivery of LogosDeliveryInterface: method getNodeInfo( self: LogosDelivery, id: NodeInfoId ): Future[Result[string, string]] {.async.} = + if self.waku.isNil(): + return err("not initialized; call startAsNode or startAsClient first") return ok(self.waku.stateInfo.getNodeInfoItem(id)) method getAvailableConfigs( diff --git a/logos_delivery/messaging/messaging_client.nim b/logos_delivery/messaging/messaging_client.nim index aec1ca684..f826b0be1 100644 --- a/logos_delivery/messaging/messaging_client.nim +++ b/logos_delivery/messaging/messaging_client.nim @@ -1,6 +1,6 @@ import results, chronos import chronicles -import brokers/broker_implement +import brokers/broker_implement, brokers/broker_context import logos_delivery/api/messaging_client_interface import logos_delivery/waku/api/types, @@ -30,12 +30,12 @@ proc stop*(self: MessagingClient) {.async.} = self.started = false BrokerImplement MessagingClient of MessagingClientInterface: - proc new*(T: typedesc[MessagingClient], useP2PReliability: bool, node: WakuNode): T = - let sendService = SendService.new(useP2PReliability, node).valueOr: - error "Failed to initialize SendService", error = error - quit(QuitFailure) - - let recvService = RecvService.new(node) + proc new*( + T: typedesc[MessagingClient], + node: WakuNode, + sendService: SendService, + recvService: RecvService, + ): T = T(node: node, sendService: sendService, recvService: recvService, started: false) method subscribe( @@ -71,3 +71,12 @@ BrokerImplement MessagingClient of MessagingClientInterface: asyncSpawn self.sendService.send(deliveryTask) return ok(requestId) + +proc newMessagingClient*( + ctx: BrokerContext, useP2PReliability: bool, node: WakuNode +): Result[MessagingClient, string] = + let sendService = SendService.new(useP2PReliability, node).valueOr: + error "Failed to initialize SendService", error = error + return err("Failed to initialize SendService: " & error) + let recvService = RecvService.new(node) + ok(MessagingClient.createUnderContext(ctx, node, sendService, recvService)) diff --git a/logos_delivery/waku/api/api.nim b/logos_delivery/waku/api/api.nim index 025a65179..33a8707a4 100644 --- a/logos_delivery/waku/api/api.nim +++ b/logos_delivery/waku/api/api.nim @@ -44,8 +44,7 @@ proc mountMessagingClient*(w: Waku): Result[void, string] = if not w.messagingClient.isNil(): return ok() - w.messagingClient = - MessagingClient.createUnderContext(w.brokerCtx, w.conf.p2pReliability, w.node) + w.messagingClient = ?newMessagingClient(w.brokerCtx, w.conf.p2pReliability, w.node) return ok() # TODO workaround for legacy use. It will be removed as soon all usage goes through LogosDelivery. diff --git a/tests/node/test_wakunode_health_monitor.nim b/tests/node/test_wakunode_health_monitor.nim index d0e1441e2..46f50b782 100644 --- a/tests/node/test_wakunode_health_monitor.nim +++ b/tests/node/test_wakunode_health_monitor.nim @@ -228,7 +228,9 @@ suite "Health Monitor - events": nodeA.mountMetadata(1, @[0'u16]).expect("Node A failed to mount metadata") await nodeA.start() - let ds = MessagingClient.new(false, nodeA) + let ds = newMessagingClient(nodeA.brokerCtx, false, nodeA).expect( + "Failed to create MessagingClient" + ) ds.start().expect("Failed to start MessagingClient") let monitorA = NodeHealthMonitor.new(nodeA) @@ -331,7 +333,9 @@ suite "Health Monitor - events": nodeA.mountMetadata(1, @[0'u16]).expect("Node A failed to mount metadata") await nodeA.start() - let ds = MessagingClient.new(false, nodeA) + let ds = newMessagingClient(nodeA.brokerCtx, false, nodeA).expect( + "Failed to create MessagingClient" + ) ds.start().expect("Failed to start MessagingClient") let subMgr = nodeA.subscriptionManager