feat(ffi): initialize with dynamic plugins#368
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds transactional dynamic plugin activation across the Rust core, FFI, and Go bindings, with discovered configuration layering, opaque ownership handles, explicit cleanup, native and worker support, and lifecycle validation. ChangesDynamic plugin activation
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant GoCaller
participant GoBinding
participant RustFFI
participant PluginHostActivation
participant DynamicPlugin
GoCaller->>GoBinding: ActivateDynamicPlugins(config, specs)
GoBinding->>RustFFI: send serialized activation request
RustFFI->>PluginHostActivation: activate parsed specifications
PluginHostActivation->>DynamicPlugin: load and start plugin
DynamicPlugin-->>PluginHostActivation: activation result
PluginHostActivation-->>RustFFI: activation handle and report
RustFFI-->>GoBinding: return handle and report JSON
GoCaller->>GoBinding: Close()
GoBinding->>RustFFI: clear and free activation
RustFFI->>DynamicPlugin: stop and unload plugin
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
github-actions
Bot
added
size:XL
License DiffCompared against Lockfile license changesLockfile License ChangesRustAdded
Removed
Updated/Changed
NodeAdded
Removed
Updated/Changed
PythonAdded
Removed
Updated/Changed
Status output |
39e39dd to
ef56a85
Compare
ef56a85 to
ad20605
Compare
ad20605 to
beb3d68
Compare
github-actions
Bot
added
size:XXL
bbednarski9
changed the title
beb3d68 to
e2e5e8c
Compare
github-actions
Bot
added
size:XL
bbednarski9
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
♻️ Duplicate comments (1)
crates/ffi/src/types/mod.rs (1)
296-320: 🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy liftDocs now warn about the clear/free race, but
free()still doesn't synchronize withclear()'s mutex — UAF risk remains.
nemo_relay_plugin_activation_clear(incrates/ffi/src/api/plugin.rs) holds aMutexGuardonactivation.0for the full teardown call.free()here drops the entireBox<FfiPluginActivation>(mutex included) without ever locking it first. This is the same critical race flagged in prior review rounds — only the doc comment changed this round; the implementation is untouched.As per path instructions, "Check for... FFI ownership/lifetime safety" for binding changes. Given the raw C ABI is meant for consumers beyond the Go binding, and the PR is gated on real-consumer validation, this residual UAF should be fixed with actual synchronization (e.g., shared
Arc<Mutex<...>>sofree()can fence on the same lock) rather than relying solely on caller-observed documentation.🔒 Sketch of a fence-before-drop fix
pub unsafe extern "C" fn nemo_relay_plugin_activation_free(ptr: *mut *mut FfiPluginActivation) { if ptr.is_null() { return; } let activation = unsafe { ptr.replace(std::ptr::null_mut()) }; if !activation.is_null() { + // Fence on the same mutex `clear()` locks so a live guard's unlock + // never runs against freed memory. + if let Ok(mut guard) = unsafe { &*activation }.0.lock() { + guard.take(); + } drop(unsafe { Box::from_raw(activation) }); } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/ffi/src/types/mod.rs` around lines 296 - 320, Synchronize nemo_relay_plugin_activation_free with nemo_relay_plugin_activation_clear before dropping the activation allocation. Make the activation state use a shared synchronization primitive that both functions lock, have free acquire the same lock and complete teardown fencing before releasing the Box, and preserve the existing nulling and repeated-call behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@crates/ffi/src/types/mod.rs`:
- Around line 296-320: Synchronize nemo_relay_plugin_activation_free with
nemo_relay_plugin_activation_clear before dropping the activation allocation.
Make the activation state use a shared synchronization primitive that both
functions lock, have free acquire the same lock and complete teardown fencing
before releasing the Box, and preserve the existing nulling and repeated-call
behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: a2f3593e-09d4-4ca8-a2d2-f390849e7788
📒 Files selected for processing (5)
crates/ffi/nemo_relay.hcrates/ffi/src/api/plugin.rscrates/ffi/src/types/mod.rsgo/nemo_relay/plugin.gogo/nemo_relay/plugin_activation_test.go
📜 Review details
🧰 Additional context used
📓 Path-based instructions (21)
crates/ffi/**
📄 CodeRabbit inference engine (.agents/skills/test-ffi-surface/SKILL.md)
Rebuild the FFI crate in release mode so the shared library and header stay in sync when making changes to crates/ffi
Files:
crates/ffi/nemo_relay.hcrates/ffi/src/types/mod.rscrates/ffi/src/api/plugin.rs
crates/ffi/nemo_relay.h
📄 CodeRabbit inference engine (.agents/skills/test-ffi-surface/SKILL.md)
Check the generated header diff when any exported symbol or type changed in the FFI surface
Update generated or generated-from-build surfaces such as
crates/ffi/nemo_relay.hthrough the proper build step.
Files:
crates/ffi/nemo_relay.h
**/*.{rs,py,go,js,ts,c,h}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Use language-appropriate naming conventions: Rust
snake_case, C FFI exports prefixednemo_relay_, GoPascalCase, Node.jscamelCase, and Pythonsnake_case.
Files:
crates/ffi/nemo_relay.hcrates/ffi/src/types/mod.rscrates/ffi/src/api/plugin.rsgo/nemo_relay/plugin.gogo/nemo_relay/plugin_activation_test.go
{crates/ffi/src/api/*.rs,crates/ffi/nemo_relay.h}
📄 CodeRabbit inference engine (.agents/skills/add-binding-feature/SKILL.md)
Add or update the shared C/FFI surface in the relevant
crates/ffi/src/api/*.rsmodule, re-export it throughcrates/ffi/src/api/mod.rs, and keep the generatedcrates/ffi/nemo_relay.hheader correct.
Files:
crates/ffi/nemo_relay.hcrates/ffi/src/api/plugin.rs
**/*
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
**/*: Format changed files with the language-native formatter before the final lint/test pass.
If dynamic plugin behavior changed, usemaintain-dynamic-pluginsand include the native SDK, worker protocol, Python SDK, docs, packaging, and Codecov surfaces in the validation plan.
If code changes alter APIs, bindings, commands, paths, packaging behavior, observability/adaptive semantics, or documented best practices, update any dependent maintainer or consumer skills in the same branch.
During iteration, preferuv run pre-commit run --files <changed files...>.
Before review or handoff, runuv run pre-commit run --all-files.
Files:
crates/ffi/nemo_relay.hcrates/ffi/src/types/mod.rscrates/ffi/src/api/plugin.rsgo/nemo_relay/plugin.gogo/nemo_relay/plugin_activation_test.go
crates/{python,ffi,node}/**/*
⚙️ CodeRabbit configuration file
crates/{python,ffi,node}/**/*: Treat binding changes as public API changes. Check for parity with the other language bindings, FFI ownership/lifetime safety,
callback error propagation, stable type conversion, and consistent async/stream semantics.
Flag changes that update one binding without corresponding tests or documentation for the same surface elsewhere.
Files:
crates/ffi/nemo_relay.hcrates/ffi/src/types/mod.rscrates/ffi/src/api/plugin.rs
**/*.rs
📄 CodeRabbit inference engine (.agents/skills/prepare-pr/SKILL.md)
**/*.rs: Any Rust change must runjust test-rust
Any Rust change must runcargo fmt --all
Any Rust change must runcargo clippy --workspace --all-targets -- -D warnings
**/*.rs: Runcargo fmt --allfor all FFI work since it is Rust work
Runjust test-rustto validate FFI changes
Runcargo clippy --workspace --all-targets -- -D warningsto enforce strict linting on FFI workWhen Rust files changed as part of Go work, also run
cargo fmt --all,just test-rust, andcargo clippy --workspace --all-targets -- -D warnings
**/*.rs: Runcargo fmt --allwhen Rust files are changed as part of Node work
Runcargo clippy --workspace --all-targets -- -D warningswhen Rust files are changed as part of Node work
Runjust test-rustwhen Rust files are changed as part of Node workWhen changing the core Rust runtime or Rust-facing API surface, format Rust code with
cargo fmt(rustfmt defaults), keepcargo clippy -- -D warningsclean, and satisfycargo deny checkperdeny.toml.
**/*.rs: If any Rust code changed, always runjust test-rust.
If any Rust code changed, also runcargo fmt --all.
If any Rust code changed, also runcargo clippy --workspace --all-targets -- -D warnings.
For Rust changes headed for review, runcargo fmt --allandcargo clippy --workspace --all-targets -- -D warningseven if relying on pre-commit.
Files:
crates/ffi/src/types/mod.rscrates/ffi/src/api/plugin.rs
crates/ffi/**/*.rs
📄 CodeRabbit inference engine (.agents/skills/test-go-binding/SKILL.md)
If the change touched
crates/ffi, also usetest-ffi-surfacefor validationUse C FFI export names prefixed with
nemo_relay_in the raw C FFI layer.
Files:
crates/ffi/src/types/mod.rscrates/ffi/src/api/plugin.rs
**/*.{rs,py}
📄 CodeRabbit inference engine (AGENTS.md)
Follow binding naming conventions in Rust and Python: use
snake_case.
Files:
crates/ffi/src/types/mod.rscrates/ffi/src/api/plugin.rs
**/*.{rs,py,js,mjs,cjs,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{rs,py,js,mjs,cjs,ts,tsx}: UseJson = serde_json::Valuein Rust-facing runtime APIs where the existing code expects JSON payloads.
UseResult<T>withFlowErrorin core runtime paths, and keep errors explicit and binding-appropriate at the wrapper layer.
Keep async behavior on the existing tokio-based model; bindings should preserve callback and future lifetimes rather than blocking or hiding async work unexpectedly.
Files:
crates/ffi/src/types/mod.rscrates/ffi/src/api/plugin.rs
**/*.{rs,go,js,ts}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Add the SPDX license header to all Rust, Go, JavaScript, and TypeScript source files using the corresponding
//comment form.
Files:
crates/ffi/src/types/mod.rscrates/ffi/src/api/plugin.rsgo/nemo_relay/plugin.gogo/nemo_relay/plugin_activation_test.go
{crates/**/src/**/*.rs,python/**/*.py}
📄 CodeRabbit inference engine (.agents/skills/maintain-dynamic-plugins/SKILL.md)
Do not add tests under
src; Rust tests belong in cratetests/trees, and Python SDK tests belong underpython/tests.
Files:
crates/ffi/src/types/mod.rscrates/ffi/src/api/plugin.rs
**/*.{rs,py,go,js,ts}
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
If a language surface changed, always run that language's test target even when Rust core did not change.
Files:
crates/ffi/src/types/mod.rscrates/ffi/src/api/plugin.rsgo/nemo_relay/plugin.gogo/nemo_relay/plugin_activation_test.go
go/nemo_relay/**/*.go
📄 CodeRabbit inference engine (.agents/skills/test-go-binding/SKILL.md)
go/nemo_relay/**/*.go: Format changed Go packages withcd go/nemo_relay && go fmt ./...
Run Go tests withjust test-goto build and test the NeMo Relay Go binding
Usejust build-gowhen you want an explicit build-only pass or need the artifact for other work
Usejust ci=true test-gowhen you need the CI-style coverage and JUnit path
On macOS, setDYLD_LIBRARY_PATHto the../../target/releasedirectory before running the rawgo testcommand directlyUse
PascalCasefor public Go APIs.
Files:
go/nemo_relay/plugin.gogo/nemo_relay/plugin_activation_test.go
**/*.go
📄 CodeRabbit inference engine (CONTRIBUTING.md)
When changing the experimental Go binding, format Go code with
gofmtand keepgo vet ./...passing.
Files:
go/nemo_relay/plugin.gogo/nemo_relay/plugin_activation_test.go
{crates/python/src/py_api/mod.rs,python/nemo_relay/**/*.py,python/nemo_relay/**/*.pyi,go/nemo_relay/**/*.go,crates/node/src/api/**/*.rs}
📄 CodeRabbit inference engine (.agents/skills/add-binding-feature/SKILL.md)
Update the language-native bindings for every exposed surface in Python, Go, and Node.js.
Files:
go/nemo_relay/plugin.gogo/nemo_relay/plugin_activation_test.go
{python/nemo_relay/**/*.py,python/nemo_relay/**/*.pyi,go/nemo_relay/**/*.go}
📄 CodeRabbit inference engine (.agents/skills/add-binding-feature/SKILL.md)
Update language wrapper helpers such as Python wrapper modules, Python type stubs, and Go shorthand packages when the new behavior belongs in those helper layers.
Files:
go/nemo_relay/plugin.gogo/nemo_relay/plugin_activation_test.go
**/*.{py,go,js,ts}
📄 CodeRabbit inference engine (.agents/skills/maintain-observability/SKILL.md)
Keep Python, Go, and Node.js config objects and subscriber/exporter methods aligned so all bindings expose the same logical knobs and semantics.
Files:
go/nemo_relay/plugin.gogo/nemo_relay/plugin_activation_test.go
go/nemo_relay/**
📄 CodeRabbit inference engine (.agents/skills/maintain-optimizer/SKILL.md)
Keep shared plugin helpers in
go/nemo_relayaligned with plugin registration, composition, and lifecycle behavior.
Files:
go/nemo_relay/plugin.gogo/nemo_relay/plugin_activation_test.go
go/nemo_relay/**/*
⚙️ CodeRabbit configuration file
go/nemo_relay/**/*: Review Go binding changes for cgo memory ownership, race safety, callback cleanup, idiomatic exported APIs, and parity with Rust/FFI behavior.
Any API change should include focused Go tests and consider race-test behavior.
Files:
go/nemo_relay/plugin.gogo/nemo_relay/plugin_activation_test.go
{crates/**/tests/**,python/tests/**,go/nemo_relay/**/*_test.go}
⚙️ CodeRabbit configuration file
{crates/**/tests/**,python/tests/**,go/nemo_relay/**/*_test.go}: Tests should cover the behavior promised by the changed API surface, including error paths and cross-request isolation where relevant.
Prefer assertions on lifecycle events, scope stacks, middleware ordering, and binding parity over shallow smoke tests.
Files:
go/nemo_relay/plugin_activation_test.go
🔇 Additional comments (6)
crates/ffi/nemo_relay.h (1)
1469-1481: LGTM!Also applies to: 2535-2543
crates/ffi/src/api/plugin.rs (1)
275-287: LGTM!go/nemo_relay/plugin.go (2)
118-154: LGTM!Also applies to: 171-180
447-452: LGTM!go/nemo_relay/plugin_activation_test.go (2)
13-13: LGTM!Also applies to: 30-31, 961-991, 993-1039
298-360: LGTM!
## Overview Adds a shared harness-native activation path that layers explicit plugin configuration over the default discovered plugins.toml files before loading dynamic plugins. The existing exact activation path remains available for hosts such as the Relay CLI that already resolved their configuration. ## Details - Reuse the same one-time config resolution used by static initialization. - Add PluginHostActivation::activate_with_discovered_config for language and FFI bindings. - Keep PluginHostActivation::activate exact and free of implicit discovery. - Validate that a file-configured static component and a dynamic component activate in one owned transaction. ## Validation - cargo test -p nemo-relay --test native_plugin_integration (28 passed) - cargo clippy -p nemo-relay --all-targets -- -D warnings - cargo fmt --all -- --check - git diff --check ## Related Issues Relates to #365, #366, and #368. ## Summary by CodeRabbit * **New Features** * Added a plugin activation entrypoint that layers discovered `plugins.toml` configuration with dynamically provided plugin components. * **Refactor** * Improved configuration resolution by centralizing how default/discovered configuration is merged with provided settings before activation. * **Bug Fixes** * Ensured activation cleanup correctly deregisters the discovered static-base plugin kind after clearing, without impacting dynamic registrations. * **Tests** * Added an end-to-end integration test covering combined discovered + dynamic activation and verifying post-clear deregistration. Authors: - Bryan Bednarski (https://github.com/bbednarski9) Approvers: - Will Killian (https://github.com/willkill07) URL: #418
Signed-off-by: Bryan Bednarski <bbednarski@nvidia.com>
Signed-off-by: Bryan Bednarski <bbednarski@nvidia.com>
Signed-off-by: Bryan Bednarski <bbednarski@nvidia.com>
Signed-off-by: Bryan Bednarski <bbednarski@nvidia.com>
Signed-off-by: Bryan Bednarski <bbednarski@nvidia.com>
Signed-off-by: Bryan Bednarski <bbednarski@nvidia.com>
Signed-off-by: Bryan Bednarski <bbednarski@nvidia.com>
Signed-off-by: Bryan Bednarski <bbednarski@nvidia.com>
Signed-off-by: Bryan Bednarski <bbednarski@nvidia.com>
Signed-off-by: Bryan Bednarski <bbednarski@nvidia.com>
bded6f5 to
ccf3991
Compare
Signed-off-by: Bryan Bednarski <bbednarski@nvidia.com>
bbednarski9
added
experimental
bbednarski9
changed the title
|
/ok to test d68108f |
|
/merge |
2 participants
Summary
This exposes the shared owned dynamic-plugin host through the C
nemo_relay_initialize_with_dynamic_pluginsand GoInitializeWithDynamicPluginsentry points for evaluation. Static-only harness-native applications continue usingnemo_relay_initialize_pluginsor GoInitializePlugins. Dynamic activation requires at least one specification.At startup, the binding discovers the standard
plugins.tomlfiles once, layers the explicit configuration over them, and then appends components loaded from the dynamic-plugin specifications. File-configured and explicitly configured static components initialize before dynamic components. Configuration files are not watched or reloaded.nemo_relay_initialize_with_dynamic_plugins, returning an opaque C ABI activation handleplugins.tomldiscovery with explicit values taking precedencePluginActivationwrapper with synchronous explicitCloseand asynchronous defensive finalizationInvalidArgwith actionable guidanceReviewer guidance
Validation
plugins.tomland an explicitly supplied dynamic plugin activate together and both callbacks executecargo test -p nemo-relay-ffi --all-features— 79 unit and 74 integration tests passedcargo clippy --workspace --all-targets -- -D warningsgo test -v ./...suite passed across all Go binding packagesgo vet ./..., formatting, generated-header synchronization, and pre-commit checks passedProduction gate
Before removing the DO NOT MERGE label, identify a real consumer and validate at minimum: host startup/shutdown integration, activation-handle ownership, in-flight callback teardown, failure diagnostics, packaging/linking, and repeated process lifecycle behavior.
Stack
plugins.tomldiscovery and layeringSummary by CodeRabbit
plugins.tomlconfiguration with explicitly provided activation specs.