From d44c2d8d686dc1e5749da8ed46dfbc9021f82c46 Mon Sep 17 00:00:00 2001 From: Gabriel Cruz Date: Fri, 7 Aug 2026 12:46:06 -0300 Subject: [PATCH 1/4] chore(ffi): bump to 0.3.0-rc.3 --- library/README.md | 36 +++++++- library/liblogosdelivery.h | 11 ++- library/liblogosdelivery.nim | 1 + library/logos_delivery_api/node_api.nim | 27 ++++-- library/logos_delivery_api/sync_exports.nim | 6 ++ logos_delivery.nimble | 2 +- logos_delivery/logos_delivery.nim | 9 ++ nimble.lock | 4 +- nix/deps.nix | 4 +- nix/submodules.json | 2 +- tests/ffi/test_ffi_persistency_lifecycle.nim | 89 +++++++++++++++++--- 11 files changed, 164 insertions(+), 27 deletions(-) create mode 100644 library/logos_delivery_api/sync_exports.nim diff --git a/library/README.md b/library/README.md index 82e1a77c8f..ab845076c6 100644 --- a/library/README.md +++ b/library/README.md @@ -108,7 +108,8 @@ int logosdelivery_start_node(void *ctx, LogosDeliveryScalarRawFn callback, void ``` #### `logosdelivery_stop_node` -Stops the node. +Stops the node. It always removes the event listeners, and it stops the node +only while one still runs, so a second call is a no-op that reports `RET_OK`. ```c int logosdelivery_stop_node(void *ctx, LogosDeliveryScalarRawFn callback, void *userData); @@ -122,6 +123,39 @@ use `ctx` afterwards. int logosdelivery_destroy(void *ctx); ``` +If the node still runs, `logosdelivery_destroy` stops it first, so a host that +skips `logosdelivery_stop_node` no longer leaves a live node behind. The call +blocks its caller while the worker drains its in-flight handlers and stops the +node. The bound is `2 * ffiRecycleTimeoutMs + ffiTeardownTimeoutMs + 2 s`, which +is 15 s at the nim-ffi defaults. + +Still call `logosdelivery_stop_node` first in normal operation. Two limits apply +to the stop that `logosdelivery_destroy` runs: + +- The return code does not cover it. `logosdelivery_destroy` returns `RET_ERR` + for an invalid `ctx` or a failed context teardown; a failed node stop is only + logged, and you get `RET_OK`. +- nim-ffi cancels the stop after `ffiTeardownTimeoutMs` (10 s) and frees the + library anyway, which leaves the node half stopped. Only an explicit + `logosdelivery_stop_node` runs to completion and reports its result. + +### Context-free calls + +These take no `ctx` and no callback. The host `dlsym`s the symbol and reads the +return value straight across the C ABI. + +#### `logosdelivery_version` +Returns the version and git commit hash of the library. You can call it before +`logosdelivery_create_node`. The first call into the library starts the Nim +runtime, so it is cheap only after that. + +```c +const char *logosdelivery_version(void); +``` + +The buffer belongs to the calling thread and stays valid until that thread calls +`logosdelivery_version` again. Copy the bytes before you hand them on. + ### Messaging #### `logosdelivery_subscribe` diff --git a/library/liblogosdelivery.h b/library/liblogosdelivery.h index d701725290..509077619d 100644 --- a/library/liblogosdelivery.h +++ b/library/liblogosdelivery.h @@ -3,8 +3,9 @@ // The call surface is generated from the {.ffi.} annotations in library/*.nim // and written to generated/logosdelivery.h by `make liblogosdelivery`. That file // is a build artifact, not checked in, so build the library before you compile -// against this header. This file adds the event-listener ABI, which nim-ffi -// exports from declareLibrary but does not emit into the `abi = c` header. +// against this header. This file adds the entry points nim-ffi exports but does +// not emit into the `abi = c` header: the event-listener ABI from +// declareLibrary, and the synchronous `{.ffiExport.}` procs. #pragma once #ifndef __liblogosdelivery__ #define __liblogosdelivery__ @@ -32,6 +33,12 @@ extern "C" { #endif + // Version and git commit hash. Needs no ctx, so it is callable before + // logosdelivery_create_node, but the first call into the library starts the + // Nim runtime. The buffer belongs to the calling thread and lasts until that + // thread calls this again, so copy it before you hand it on. + const char *logosdelivery_version(void); + // Raw result-delivery callback used by the event API. `msg` is a byte run of // `len` bytes, not NUL-terminated, and is valid only for the duration of the // call. diff --git a/library/liblogosdelivery.nim b/library/liblogosdelivery.nim index 853311aef1..811135d598 100644 --- a/library/liblogosdelivery.nim +++ b/library/liblogosdelivery.nim @@ -18,6 +18,7 @@ include ./logos_delivery_api/node_api, ./logos_delivery_api/messaging_api, ./logos_delivery_api/debug_api, + ./logos_delivery_api/sync_exports, ./kernel_api/peer_manager_api, ./kernel_api/discovery_api, ./kernel_api/debug_node_api, diff --git a/library/logos_delivery_api/node_api.nim b/library/logos_delivery_api/node_api.nim index 277b0a5569..9b38086ba8 100644 --- a/library/logos_delivery_api/node_api.nim +++ b/library/logos_delivery_api/node_api.nim @@ -31,9 +31,6 @@ proc logosdelivery_create_node( return ok(lib) -proc logosdelivery_destroy(self: LogosDelivery) {.ffiDtor.} = - discard - proc registerFFIEventListeners(self: LogosDelivery): Result[void, string] = ## Bridges every broker event the library re-publishes onto the FFI event ## registry. Keep in step with `dropFFIEventListeners`. @@ -168,18 +165,36 @@ proc logosdelivery_start_node( return err(error) (await self.start()).isOkOr: + ## Drop what the line above registered, or a retry stacks a second set. + await self.dropFFIEventListeners() let errMsg = $error chronicles.error "START_NODE failed", err = errMsg return err("failed to start: " & errMsg) return ok("") +proc stopNode(self: LogosDelivery): Future[Result[void, string]] {.async.} = + ## The teardown `logosdelivery_stop_node` and `logosdelivery_destroy` share. + ## Listeners come off unconditionally: a start that failed after + ## `registerFFIEventListeners` leaves them on a node that never ran. + await self.dropFFIEventListeners() + + if not self.isRunning(): + return ok() + + await self.stop() + proc logosdelivery_stop_node( self: LogosDelivery ): Future[Result[string, string]] {.ffi.} = - await self.dropFFIEventListeners() - - (await self.stop()).isOkOr: + (await self.stopNode()).isOkOr: let errMsg = $error chronicles.error "STOP_NODE failed", err = errMsg return err("failed to stop: " & errMsg) return ok("") + +proc logosdelivery_destroy(self: LogosDelivery) {.ffiDtor.} = + ## Safety net for a host that skips `stop_node` (issue #4108): nim-ffi recycles + ## the worker instead of joining it, so an unstopped node runs on. The recycle + ## handler drops what this returns, so the host still reads RET_OK on failure. + (await self.stopNode()).isOkOr: + chronicles.error "DESTROY failed", err = error diff --git a/library/logos_delivery_api/sync_exports.nim b/library/logos_delivery_api/sync_exports.nim new file mode 100644 index 0000000000..8c4980cde5 --- /dev/null +++ b/library/logos_delivery_api/sync_exports.nim @@ -0,0 +1,6 @@ +## `{.ffiExport.}` entry points: no context, no callback, the return value +## crosses the C ABI directly. The `{.ffi.}` surface next door needs both. + +proc logosdelivery_version(): string {.ffiExport.} = + ## Same string `waku_version` answers over the context surface. + WakuNodeVersionString diff --git a/logos_delivery.nimble b/logos_delivery.nimble index cd216f6887..c0639b6336 100644 --- a/logos_delivery.nimble +++ b/logos_delivery.nimble @@ -61,7 +61,7 @@ requires "nim >= 2.2.4", # Packages not on nimble (use git URLs) -requires "https://github.com/logos-messaging/nim-ffi#53515de17af0ef3e88b2aec9675b8163dddc14ae" # v0.3.0-rc.2 +requires "https://github.com/logos-messaging/nim-ffi#b6c17dc822960b626d76d814de90208c0a40a44e" # v0.3.0-rc.3 requires "https://github.com/logos-messaging/nim-sds.git#b12f5ee07c5b764303b51fb948b32a4ade1de3b5" diff --git a/logos_delivery/logos_delivery.nim b/logos_delivery/logos_delivery.nim index 55867e5827..6b74e9b353 100644 --- a/logos_delivery/logos_delivery.nim +++ b/logos_delivery/logos_delivery.nim @@ -216,6 +216,15 @@ proc stop*(self: LogosDelivery): Future[Result[void, string]] {.async.} = return ok() +func isRunning*(self: LogosDelivery): bool = + ## True while a layer still holds what `stop` releases. The channel manager + ## needs no test: its `stop` empties the channel table, so a second call is + ## a no-op. + let transportUp = + not self.waku.isNil() and not self.waku.node.isNil() and self.waku.node.started + let messagingUp = not self.messagingClient.isNil() and self.messagingClient.started + transportUp or messagingUp + proc isOnline*(self: LogosDelivery): Future[Result[bool, string]] {.async.} = if self.waku.isNil(): return err("Waku node is not initialized") diff --git a/nimble.lock b/nimble.lock index 8d924d507f..e9696130aa 100644 --- a/nimble.lock +++ b/nimble.lock @@ -644,7 +644,7 @@ }, "ffi": { "version": "0.3.0", - "vcsRevision": "53515de17af0ef3e88b2aec9675b8163dddc14ae", + "vcsRevision": "b6c17dc822960b626d76d814de90208c0a40a44e", "url": "https://github.com/logos-messaging/nim-ffi", "downloadMethod": "git", "dependencies": [ @@ -655,7 +655,7 @@ "cbor_serialization" ], "checksums": { - "sha1": "1d84ceaf8594f4970c5a37f916003ffc0531dc4e" + "sha1": "74e796df3ef39d828e014df701127edfbee459e0" } }, "boringssl": { diff --git a/nix/deps.nix b/nix/deps.nix index 858d4b58b3..5cac5f24c5 100644 --- a/nix/deps.nix +++ b/nix/deps.nix @@ -285,8 +285,8 @@ ffi = pkgs.fetchgit { url = "https://github.com/logos-messaging/nim-ffi"; - rev = "53515de17af0ef3e88b2aec9675b8163dddc14ae"; - sha256 = "0ncf9j7fhgd3nswr4rh19jx77dl974sajphdl04cb602hshgj5ij"; + rev = "b6c17dc822960b626d76d814de90208c0a40a44e"; + sha256 = "1sjnax54j39igsxkig095grj4x3j9div4fimrmz609byhp4qdavh"; fetchSubmodules = true; }; diff --git a/nix/submodules.json b/nix/submodules.json index c9ac2d8148..34a537a78c 100644 --- a/nix/submodules.json +++ b/nix/submodules.json @@ -56,7 +56,7 @@ { "path": "vendor/nim-ffi", "url": "https://github.com/logos-messaging/nim-ffi", - "rev": "53515de17af0ef3e88b2aec9675b8163dddc14ae" + "rev": "b6c17dc822960b626d76d814de90208c0a40a44e" } , { diff --git a/tests/ffi/test_ffi_persistency_lifecycle.nim b/tests/ffi/test_ffi_persistency_lifecycle.nim index 99528c2f16..9b7e8d93d3 100644 --- a/tests/ffi/test_ffi_persistency_lifecycle.nim +++ b/tests/ffi/test_ffi_persistency_lifecycle.nim @@ -20,13 +20,17 @@ ## destroying ctx1 without stop must not corrupt ctx2's persistency. ## Historically a UB probe (stale global into a released heap -> ## SIGSEGV on Linux, zombie singleton on macOS/arm64); kept as a guard. -## The destroy-without-stop teardown gap itself is tracked separately -## (issue #4108). ## ## case different-storage-paths ## two contexts with different local-storage-paths must both start; ## the former singleton refused the second rootDir. ## +## case destroy-stops-node +## destroy without stop must tear the node down (issue #4108). nim-ffi +## 0.3.0-rc.3 runs the `{.ffiDtor.}` body on the recycle path, and +## `logosdelivery_destroy` stops the node there. The case also calls the +## context-free `logosdelivery_version` export before any node exists. +## ## Each case runs in a child process (the second one can fault) with its ## output captured to a file, so a crash is an exit code rather than a dead ## test binary. @@ -53,6 +57,7 @@ const CaseStopSteals = "--case-stop-steals-persistence" CaseDestroyOnly = "--case-destroy-without-stop" CaseTwoPaths = "--case-different-storage-paths" + CaseDestroyStops = "--case-destroy-stops-node" DisabledMarker = "SDS persistence disabled" ## Logged by `sdsPersistence()` when the singleton is unusable. @@ -79,6 +84,9 @@ type cdecl, gcsafe .} CtxFn = proc(ctx: pointer, cb: FfiCallback, userData: pointer): cint {.cdecl, gcsafe.} + DestroyFn = proc(ctx: pointer): cint {.cdecl, gcsafe.} + ## The `{.ffiDtor.}` export takes no callback: it blocks until teardown ends. + VersionFn = proc(): cstring {.cdecl, gcsafe.} ChannelCreateFn = proc( ctx: pointer, cb: FfiCallback, @@ -93,7 +101,8 @@ type createNode: CreateNodeFn startNode: CtxFn stopNode: CtxFn - destroy: CtxFn + destroy: DestroyFn + version: VersionFn channelCreate: ChannelCreateFn channelExists: ChannelExistsFn @@ -165,7 +174,8 @@ proc loadApi(): Api = createNode: cast[CreateNodeFn](lib.need("logosdelivery_create_node")), startNode: cast[CtxFn](lib.need("logosdelivery_start_node")), stopNode: cast[CtxFn](lib.need("logosdelivery_stop_node")), - destroy: cast[CtxFn](lib.need("logosdelivery_destroy")), + destroy: cast[DestroyFn](lib.need("logosdelivery_destroy")), + version: cast[VersionFn](lib.need("logosdelivery_version")), channelCreate: cast[ChannelCreateFn](lib.need("logosdelivery_channel_create")), channelExists: cast[ChannelExistsFn](lib.need("logosdelivery_channel_exists")), ) @@ -215,6 +225,20 @@ proc call(api: Api, s: ptr Slot, label: string, fn: CtxFn, ctx: pointer) = discard fn(ctx, onDone, s) expectOk(label, awaitSlot(s)) +proc destroyCtx(api: Api, label: string, ctx: pointer) = + ## `destroy` answers with its return code, not through the slot. + expectOk(label, (ok: true, ret: int(api.destroy(ctx)), msg: "")) + +proc expectVersion(api: Api) = + ## `{.ffiExport.}` procs never reach the generated header, so `loadApi`'s + ## symbol lookup plus this call are the only check that one is still exported. + ## Runs before any create_node: a context-free call is the point of the export. + let version = $api.version() + echo " [ctx-free logosdelivery_version] ", version + if not version.startsWith("version / git commit hash:"): + echo " FAIL: logosdelivery_version returned an unexpected string" + failed = true + proc createChannel(api: Api, s: ptr Slot, label: string, ctx: pointer, id: string) = armSlot(s) discard api.channelCreate( @@ -253,16 +277,17 @@ proc runStopSteals(api: Api, s: ptr Slot) = api.createChannel(s, "ctx2 channel_create (before ctx1 stop)", ctx2, "before") api.call(s, "ctx1 stop_node", api.stopNode, ctx1) - api.call(s, "ctx1 destroy", api.destroy, ctx1) + api.destroyCtx("ctx1 destroy", ctx1) api.createChannel(s, "ctx2 channel_create (after ctx1 stop)", ctx2, "after") api.call(s, "ctx2 stop_node", api.stopNode, ctx2) - api.call(s, "ctx2 destroy", api.destroy, ctx2) + api.destroyCtx("ctx2 destroy", ctx2) proc runDestroyOnly(api: Api, s: ptr Slot) = - ## Same, but ctx1 is destroyed without stop_node -- reset() never runs, so - ## the global keeps pointing into ctx1's released FFI-thread heap. + ## Same, but ctx1 is destroyed without stop_node. The dtor now reaches + ## reset() on that path too; the case stays as the guard for the era when it + ## did not, and the global kept pointing into ctx1's released FFI-thread heap. let root = caseRoot("shared") let ctx1 = createCtx(api, s, "ctx1", root, 60030, 60031) let ctx2 = createCtx(api, s, "ctx2", root, 60040, 60041) @@ -277,13 +302,13 @@ proc runDestroyOnly(api: Api, s: ptr Slot) = ## against released memory. api.createChannel(s, "ctx1 channel_create (opens the sds job)", ctx1, "owned-by-ctx1") - api.call(s, "ctx1 destroy (no stop_node)", api.destroy, ctx1) + api.destroyCtx("ctx1 destroy (no stop_node)", ctx1) api.churn(s, ctx2) api.createChannel(s, "ctx2 channel_create (after ctx1 destroy)", ctx2, "after") api.call(s, "ctx2 stop_node", api.stopNode, ctx2) - api.call(s, "ctx2 destroy", api.destroy, ctx2) + api.destroyCtx("ctx2 destroy", ctx2) proc runTwoPaths(api: Api, s: ptr Slot) = ## Two contexts, two storage paths. The singleton refuses to be re-targeted, @@ -299,8 +324,32 @@ proc runTwoPaths(api: Api, s: ptr Slot) = api.call(s, "ctx2 start_node (different local-storage-path)", api.startNode, ctx2) api.call(s, "ctx1 stop_node", api.stopNode, ctx1) - api.call(s, "ctx1 destroy", api.destroy, ctx1) - api.call(s, "ctx2 destroy", api.destroy, ctx2) + api.destroyCtx("ctx1 destroy", ctx1) + api.destroyCtx("ctx2 destroy", ctx2) + +proc runDestroyStopsNode(api: Api, s: ptr Slot) = + ## destroy without stop must tear the node down. The listening ports are the + ## observable: a node that still runs holds them, so the second bind fails. + ## ctx1 had to bind those same ports before the destroy, so a failure of the + ## second bind means ctx1 never let go, not that a stranger holds the port. + ## ctx2 also reuses the pooled worker thread ctx1 released, which is where a + ## surviving loop of ctx1's node would still be scheduled. + api.expectVersion() + + let ctx1 = createCtx(api, s, "ctx1", caseRoot("dtor_a"), 60070, 60071) + if failed: + return + + api.call(s, "ctx1 start_node", api.startNode, ctx1) + api.destroyCtx("ctx1 destroy (no stop_node)", ctx1) + + let ctx2 = createCtx(api, s, "ctx2 (same ports)", caseRoot("dtor_b"), 60070, 60071) + if failed: + return + + api.call(s, "ctx2 start_node (ports must be free)", api.startNode, ctx2) + api.call(s, "ctx2 stop_node", api.stopNode, ctx2) + api.destroyCtx("ctx2 destroy", ctx2) proc runChild(which: string) = let api = loadApi() @@ -313,6 +362,8 @@ proc runChild(which: string) = runDestroyOnly(api, s) of CaseTwoPaths: runTwoPaths(api, s) + of CaseDestroyStops: + runDestroyStopsNode(api, s) else: quit("unknown case " & which, 2) @@ -387,3 +438,17 @@ suite "FFI - persistency lifecycle across library contexts": removeDir(caseRoot("path_b")) check r.code == 0 + + test "destroy without stop_node must stop the node and free its ports": + if not fileExists(libPath()): + echo "skipped: no ", libPath() + skip() + else: + removeDir(caseRoot("dtor_a")) + removeDir(caseRoot("dtor_b")) + let r = runCase(CaseDestroyStops) + report(CaseDestroyStops, r) + removeDir(caseRoot("dtor_a")) + removeDir(caseRoot("dtor_b")) + + check r.code == 0 From 019a5ccbbfdea46f133f7cb8d5b8cd68de94658f Mon Sep 17 00:00:00 2001 From: Gabriel Cruz Date: Fri, 7 Aug 2026 13:42:17 -0300 Subject: [PATCH 2/4] chore: trim comments --- library/README.md | 35 ++++++++------------ library/liblogosdelivery.h | 11 +++--- library/logos_delivery_api/node_api.nim | 11 +++--- library/logos_delivery_api/sync_exports.nim | 4 +-- logos_delivery/logos_delivery.nim | 5 ++- tests/ffi/test_ffi_persistency_lifecycle.nim | 24 +++++--------- 6 files changed, 33 insertions(+), 57 deletions(-) diff --git a/library/README.md b/library/README.md index ab845076c6..d10e8f1d3e 100644 --- a/library/README.md +++ b/library/README.md @@ -108,8 +108,8 @@ int logosdelivery_start_node(void *ctx, LogosDeliveryScalarRawFn callback, void ``` #### `logosdelivery_stop_node` -Stops the node. It always removes the event listeners, and it stops the node -only while one still runs, so a second call is a no-op that reports `RET_OK`. +Stops the node and removes the event listeners. A second call is a no-op that +reports `RET_OK`. ```c int logosdelivery_stop_node(void *ctx, LogosDeliveryScalarRawFn callback, void *userData); @@ -123,38 +123,29 @@ use `ctx` afterwards. int logosdelivery_destroy(void *ctx); ``` -If the node still runs, `logosdelivery_destroy` stops it first, so a host that -skips `logosdelivery_stop_node` no longer leaves a live node behind. The call -blocks its caller while the worker drains its in-flight handlers and stops the -node. The bound is `2 * ffiRecycleTimeoutMs + ffiTeardownTimeoutMs + 2 s`, which -is 15 s at the nim-ffi defaults. +Stops the node first if it still runs, so skipping `logosdelivery_stop_node` no +longer leaves a live node behind. It blocks for up to 15 s at the nim-ffi +defaults (`2 * ffiRecycleTimeoutMs + ffiTeardownTimeoutMs + 2 s`). -Still call `logosdelivery_stop_node` first in normal operation. Two limits apply -to the stop that `logosdelivery_destroy` runs: - -- The return code does not cover it. `logosdelivery_destroy` returns `RET_ERR` - for an invalid `ctx` or a failed context teardown; a failed node stop is only - logged, and you get `RET_OK`. -- nim-ffi cancels the stop after `ffiTeardownTimeoutMs` (10 s) and frees the - library anyway, which leaves the node half stopped. Only an explicit - `logosdelivery_stop_node` runs to completion and reports its result. +Prefer an explicit `logosdelivery_stop_node`: a failed stop here is only logged +(`RET_ERR` covers an invalid `ctx` and a failed context teardown, nothing else), +and nim-ffi cancels the stop at `ffiTeardownTimeoutMs` (10 s), leaving the node +half stopped. ### Context-free calls -These take no `ctx` and no callback. The host `dlsym`s the symbol and reads the -return value straight across the C ABI. +No `ctx` and no callback: `dlsym` the symbol and read the return value. #### `logosdelivery_version` -Returns the version and git commit hash of the library. You can call it before -`logosdelivery_create_node`. The first call into the library starts the Nim -runtime, so it is cheap only after that. +Version and git commit hash. Callable before `logosdelivery_create_node`, though +the first call into the library starts the Nim runtime. ```c const char *logosdelivery_version(void); ``` The buffer belongs to the calling thread and stays valid until that thread calls -`logosdelivery_version` again. Copy the bytes before you hand them on. +`logosdelivery_version` again, so copy the bytes. ### Messaging diff --git a/library/liblogosdelivery.h b/library/liblogosdelivery.h index 509077619d..bf82b7f3b7 100644 --- a/library/liblogosdelivery.h +++ b/library/liblogosdelivery.h @@ -3,9 +3,8 @@ // The call surface is generated from the {.ffi.} annotations in library/*.nim // and written to generated/logosdelivery.h by `make liblogosdelivery`. That file // is a build artifact, not checked in, so build the library before you compile -// against this header. This file adds the entry points nim-ffi exports but does -// not emit into the `abi = c` header: the event-listener ABI from -// declareLibrary, and the synchronous `{.ffiExport.}` procs. +// against this header. This file adds what nim-ffi exports but leaves out of the +// `abi = c` header: the event-listener ABI, and the `{.ffiExport.}` procs. #pragma once #ifndef __liblogosdelivery__ #define __liblogosdelivery__ @@ -33,10 +32,8 @@ extern "C" { #endif - // Version and git commit hash. Needs no ctx, so it is callable before - // logosdelivery_create_node, but the first call into the library starts the - // Nim runtime. The buffer belongs to the calling thread and lasts until that - // thread calls this again, so copy it before you hand it on. + // Version and git commit hash. Needs no ctx. The buffer belongs to the calling + // thread and lasts until that thread calls this again, so copy it. const char *logosdelivery_version(void); // Raw result-delivery callback used by the event API. `msg` is a byte run of diff --git a/library/logos_delivery_api/node_api.nim b/library/logos_delivery_api/node_api.nim index 9b38086ba8..2988d39b20 100644 --- a/library/logos_delivery_api/node_api.nim +++ b/library/logos_delivery_api/node_api.nim @@ -165,7 +165,7 @@ proc logosdelivery_start_node( return err(error) (await self.start()).isOkOr: - ## Drop what the line above registered, or a retry stacks a second set. + ## A retry would stack a second set. await self.dropFFIEventListeners() let errMsg = $error chronicles.error "START_NODE failed", err = errMsg @@ -173,9 +173,7 @@ proc logosdelivery_start_node( return ok("") proc stopNode(self: LogosDelivery): Future[Result[void, string]] {.async.} = - ## The teardown `logosdelivery_stop_node` and `logosdelivery_destroy` share. - ## Listeners come off unconditionally: a start that failed after - ## `registerFFIEventListeners` leaves them on a node that never ran. + ## Listeners come off unconditionally: a failed start registers them anyway. await self.dropFFIEventListeners() if not self.isRunning(): @@ -193,8 +191,7 @@ proc logosdelivery_stop_node( return ok("") proc logosdelivery_destroy(self: LogosDelivery) {.ffiDtor.} = - ## Safety net for a host that skips `stop_node` (issue #4108): nim-ffi recycles - ## the worker instead of joining it, so an unstopped node runs on. The recycle - ## handler drops what this returns, so the host still reads RET_OK on failure. + ## Safety net for a host that skips `stop_node` (#4108): nim-ffi recycles the + ## worker rather than joining it, so an unstopped node keeps running. (await self.stopNode()).isOkOr: chronicles.error "DESTROY failed", err = error diff --git a/library/logos_delivery_api/sync_exports.nim b/library/logos_delivery_api/sync_exports.nim index 8c4980cde5..0434dfae97 100644 --- a/library/logos_delivery_api/sync_exports.nim +++ b/library/logos_delivery_api/sync_exports.nim @@ -1,5 +1,5 @@ -## `{.ffiExport.}` entry points: no context, no callback, the return value -## crosses the C ABI directly. The `{.ffi.}` surface next door needs both. +## `{.ffiExport.}` entry points: no context, no callback, the value crosses the +## C ABI directly. proc logosdelivery_version(): string {.ffiExport.} = ## Same string `waku_version` answers over the context surface. diff --git a/logos_delivery/logos_delivery.nim b/logos_delivery/logos_delivery.nim index 6b74e9b353..0e5767ed38 100644 --- a/logos_delivery/logos_delivery.nim +++ b/logos_delivery/logos_delivery.nim @@ -217,9 +217,8 @@ proc stop*(self: LogosDelivery): Future[Result[void, string]] {.async.} = return ok() func isRunning*(self: LogosDelivery): bool = - ## True while a layer still holds what `stop` releases. The channel manager - ## needs no test: its `stop` empties the channel table, so a second call is - ## a no-op. + ## True while a layer still holds what `stop` releases; the channel manager + ## needs no test, its `stop` is already a no-op when empty. let transportUp = not self.waku.isNil() and not self.waku.node.isNil() and self.waku.node.started let messagingUp = not self.messagingClient.isNil() and self.messagingClient.started diff --git a/tests/ffi/test_ffi_persistency_lifecycle.nim b/tests/ffi/test_ffi_persistency_lifecycle.nim index 9b7e8d93d3..3007bef0c7 100644 --- a/tests/ffi/test_ffi_persistency_lifecycle.nim +++ b/tests/ffi/test_ffi_persistency_lifecycle.nim @@ -26,10 +26,8 @@ ## the former singleton refused the second rootDir. ## ## case destroy-stops-node -## destroy without stop must tear the node down (issue #4108). nim-ffi -## 0.3.0-rc.3 runs the `{.ffiDtor.}` body on the recycle path, and -## `logosdelivery_destroy` stops the node there. The case also calls the -## context-free `logosdelivery_version` export before any node exists. +## destroy without stop must tear the node down (issue #4108), and the +## context-free `logosdelivery_version` export must answer with no node. ## ## Each case runs in a child process (the second one can fault) with its ## output captured to a file, so a crash is an exit code rather than a dead @@ -230,9 +228,8 @@ proc destroyCtx(api: Api, label: string, ctx: pointer) = expectOk(label, (ok: true, ret: int(api.destroy(ctx)), msg: "")) proc expectVersion(api: Api) = - ## `{.ffiExport.}` procs never reach the generated header, so `loadApi`'s - ## symbol lookup plus this call are the only check that one is still exported. - ## Runs before any create_node: a context-free call is the point of the export. + ## `{.ffiExport.}` procs never reach the generated header, so nothing but + ## `loadApi` and this call checks that one is still exported. let version = $api.version() echo " [ctx-free logosdelivery_version] ", version if not version.startsWith("version / git commit hash:"): @@ -285,9 +282,8 @@ proc runStopSteals(api: Api, s: ptr Slot) = api.destroyCtx("ctx2 destroy", ctx2) proc runDestroyOnly(api: Api, s: ptr Slot) = - ## Same, but ctx1 is destroyed without stop_node. The dtor now reaches - ## reset() on that path too; the case stays as the guard for the era when it - ## did not, and the global kept pointing into ctx1's released FFI-thread heap. + ## Same, but ctx1 is destroyed without stop_node. The dtor reaches reset() on + ## that path too now; the case guards the era when it did not. let root = caseRoot("shared") let ctx1 = createCtx(api, s, "ctx1", root, 60030, 60031) let ctx2 = createCtx(api, s, "ctx2", root, 60040, 60041) @@ -328,12 +324,8 @@ proc runTwoPaths(api: Api, s: ptr Slot) = api.destroyCtx("ctx2 destroy", ctx2) proc runDestroyStopsNode(api: Api, s: ptr Slot) = - ## destroy without stop must tear the node down. The listening ports are the - ## observable: a node that still runs holds them, so the second bind fails. - ## ctx1 had to bind those same ports before the destroy, so a failure of the - ## second bind means ctx1 never let go, not that a stranger holds the port. - ## ctx2 also reuses the pooled worker thread ctx1 released, which is where a - ## surviving loop of ctx1's node would still be scheduled. + ## The ports are the observable: ctx1 bound them before the destroy, so a + ## failed second bind means ctx1 never let go, not that a stranger holds them. api.expectVersion() let ctx1 = createCtx(api, s, "ctx1", caseRoot("dtor_a"), 60070, 60071) From e976aa9d49bb634adc3d3650020e06230c05b0f1 Mon Sep 17 00:00:00 2001 From: Gabriel Cruz Date: Mon, 10 Aug 2026 16:13:46 -0300 Subject: [PATCH 3/4] chore: use 0.3.0 version --- library/README.md | 5 +++ logos_delivery.nimble | 6 +-- tests/ffi/test_ffi_persistency_lifecycle.nim | 43 ++++++++++++++++++++ 3 files changed, 51 insertions(+), 3 deletions(-) diff --git a/library/README.md b/library/README.md index d10e8f1d3e..e12cb13775 100644 --- a/library/README.md +++ b/library/README.md @@ -69,6 +69,11 @@ void *logosdelivery_create_node( **Returns:** the context handle, or `NULL` on failure. Creation is asynchronous: wait for `onCreated` before you make any other call. +A context whose `onCreated` reported `RET_ERR` stays live but holds no node. +Every later call on it answers `RET_ERR` with `library is not initialized: the +constructor failed or has not run yet`, and `logosdelivery_destroy` still +releases it. + **Example configuration JSON:** ```json { diff --git a/logos_delivery.nimble b/logos_delivery.nimble index c0639b6336..62cc75d360 100644 --- a/logos_delivery.nimble +++ b/logos_delivery.nimble @@ -57,12 +57,12 @@ requires "nim >= 2.2.4", "zlib", # Debug & Testing "testutils", - "unittest2" + "unittest2", + # FFI + "ffi == 0.3.0" # Packages not on nimble (use git URLs) -requires "https://github.com/logos-messaging/nim-ffi#b6c17dc822960b626d76d814de90208c0a40a44e" # v0.3.0-rc.3 - requires "https://github.com/logos-messaging/nim-sds.git#b12f5ee07c5b764303b51fb948b32a4ade1de3b5" requires "https://github.com/NagyZoltanPeter/nim-brokers.git#v3.3.0" diff --git a/tests/ffi/test_ffi_persistency_lifecycle.nim b/tests/ffi/test_ffi_persistency_lifecycle.nim index 3007bef0c7..d9d70cbe3d 100644 --- a/tests/ffi/test_ffi_persistency_lifecycle.nim +++ b/tests/ffi/test_ffi_persistency_lifecycle.nim @@ -29,6 +29,10 @@ ## destroy without stop must tear the node down (issue #4108), and the ## context-free `logosdelivery_version` export must answer with no node. ## +## case call-after-failed-ctor +## a context whose constructor failed must reject a later call with an +## error instead of faulting on its nil library. +## ## Each case runs in a child process (the second one can fault) with its ## output captured to a file, so a crash is an exit code rather than a dead ## test binary. @@ -56,6 +60,7 @@ const CaseDestroyOnly = "--case-destroy-without-stop" CaseTwoPaths = "--case-different-storage-paths" CaseDestroyStops = "--case-destroy-stops-node" + CaseFailedCtor = "--case-call-after-failed-ctor" DisabledMarker = "SDS persistence disabled" ## Logged by `sdsPersistence()` when the singleton is unusable. @@ -191,6 +196,14 @@ proc expectOk(step: string, r: tuple[ok: bool, ret: int, msg: string]) = echo " FAIL: ", step, " expected RET_OK" failed = true +proc expectErr( + step: string, r: tuple[ok: bool, ret: int, msg: string], needle: string +) = + note(step, r) + if not r.ok or r.ret == RetOk or not r.msg.contains(needle): + echo " FAIL: ", step, " expected an error carrying '", needle, "'" + failed = true + proc nodeConfig(storagePath: string, tcpPort, discv5Port: int): string = $( %*{ @@ -343,6 +356,24 @@ proc runDestroyStopsNode(api: Api, s: ptr Slot) = api.call(s, "ctx2 stop_node", api.stopNode, ctx2) api.destroyCtx("ctx2 destroy", ctx2) +proc runCallAfterFailedCtor(api: Api, s: ptr Slot) = + ## `create_node` hands back a live context before the constructor runs, so a + ## host holds one even when the constructor fails. `LogosDelivery` is a `ref`, + ## and a call against it used to read the fields of a nil library. + armSlot(s) + let ctx = api.createNode("{ not a config }".cstring, onDone, s) + if ctx.isNil(): + echo " FAIL: create_node returned nil, the case needs a live context" + failed = true + return + expectErr("ctx create_node (invalid config)", awaitSlot(s), "parseLogosDeliveryConf") + + armSlot(s) + discard api.startNode(ctx, onDone, s) + expectErr("ctx start_node (constructor failed)", awaitSlot(s), "not initialized") + + api.destroyCtx("ctx destroy (constructor failed)", ctx) + proc runChild(which: string) = let api = loadApi() let s = createShared(Slot) @@ -356,6 +387,8 @@ proc runChild(which: string) = runTwoPaths(api, s) of CaseDestroyStops: runDestroyStopsNode(api, s) + of CaseFailedCtor: + runCallAfterFailedCtor(api, s) else: quit("unknown case " & which, 2) @@ -444,3 +477,13 @@ suite "FFI - persistency lifecycle across library contexts": removeDir(caseRoot("dtor_b")) check r.code == 0 + + test "a call against a context whose constructor failed must not fault": + if not fileExists(libPath()): + echo "skipped: no ", libPath() + skip() + else: + let r = runCase(CaseFailedCtor) + report(CaseFailedCtor, r) + + check r.code == 0 From 75b95de14f0b8506634f1a049677023a9db153c1 Mon Sep 17 00:00:00 2001 From: Gabriel Cruz Date: Wed, 12 Aug 2026 10:54:32 -0300 Subject: [PATCH 4/4] fix: failing ci --- tests/waku_relay/test_wakunode_relay.nim | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/tests/waku_relay/test_wakunode_relay.nim b/tests/waku_relay/test_wakunode_relay.nim index dda3c65ae7..3e624f8626 100644 --- a/tests/waku_relay/test_wakunode_relay.nim +++ b/tests/waku_relay/test_wakunode_relay.nim @@ -605,6 +605,18 @@ suite "WakuNode - Relay": check: nodes[i].wakuRelay.peerStats[nodes[0].switch.peerInfo.peerId].score == -249999.9 + proc badPeerIsolated(): bool = + nodes[0].peerManager.switch.connManager.getConnections().len == 0 and + toSeq(1 ..< 5).allIt( + nodes[it].peerManager.switch.connManager.getConnections().len == 3 + ) + + ## The disconnect trails the score drop, and the delay grows on a loaded + ## runner, so poll instead of reading the counts once. + let deadline = Moment.now() + 30.seconds + while Moment.now() < deadline and not badPeerIsolated(): + await sleepAsync(500.millis) + # nodes[0] was blacklisted from all other peers, no connections check: nodes[0].peerManager.switch.connManager.getConnections().len == 0