Skip to content

fix: Structured API Layers PR review (suggested fixes)#3966

Draft
fcecin wants to merge 2 commits into
poc/structured-api-interfacefrom
fix/fcecin-review-structured-api
Draft

fix: Structured API Layers PR review (suggested fixes)#3966
fcecin wants to merge 2 commits into
poc/structured-api-interfacefrom
fix/fcecin-review-structured-api

Conversation

@fcecin

@fcecin fcecin commented Jun 19, 2026

Copy link
Copy Markdown
Contributor

Description

This PR implements some code suggestions for the Structured API Layers PR that I came up with while reviewing the PR.

Merging this PR is not needed. It just one way to address the points in my code review (see below).

Can also cherry-pick these, or do something different/custom in the code, or leave unaddressed and just close this.

PR Review:

  • MessagingClient.new kills the host process. It quit(QuitFailure)s on a recoverable SendService.new failure (a node with neither relay nor lightpush). Via the FFI start path (node_api.nim -> mountMessagingClient) that aborts the embedding app instead of returning an error, and it left the initMessagingClient(...).valueOr branch dead.
  • LogosDelivery.stop() abandons teardown on the first failure. An exception in one component early-returns, so e.g. a throwing channel-manager teardown leaves the messaging client and the libp2p node running (leak).
  • getNodeInfo() nil-derefs. Unlike kernel()/messaging()/channels() it doesn't guard self.waku.isNil() -> crash if called before start / after stop.

Changes

  • 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).

 * 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).
@github-actions

github-actions Bot commented Jun 19, 2026

Copy link
Copy Markdown

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:3966

Built from fe7122c

Comment thread logos_delivery/messaging/messaging_client.nim Outdated
Co-authored-by: Ivan FB <128452529+Ivansete-status@users.noreply.github.com>
@Ivansete-status

Ivansete-status commented Jun 24, 2026

Copy link
Copy Markdown
Collaborator

@fcecin - the changes looked correct to me, conceptually. However, given that we are not merging POC in the short term, we may need to submit a separate PR based on master, instead.

@fcecin

fcecin commented Jun 24, 2026

Copy link
Copy Markdown
Contributor Author

@fcecin - the changes looked correct to me, conceptually. However, given that we are not merging POC in the short term, we may need to submit a separate PR to based it on master, instead.

Yep this PR merges into Zoltan's PR, it's just a "review PR." He can drive it however he likes as part of the rewview, close etc. If he ever merges his PR this is closed regardless.

@fcecin

fcecin commented Jun 24, 2026

Copy link
Copy Markdown
Contributor Author

Actually, let me mark this as draft so it doesn't cause confusion.

@fcecin
fcecin marked this pull request as draft June 24, 2026 12:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants