fix: make Python plugin teardown asyncio-safe - #605
Conversation
Signed-off-by: Will Killian <wkillian@nvidia.com>
github-actions
Bot
added
size:M
WalkthroughThe PR adds coordinated asynchronous Python plugin cleanup backed by native teardown workers. Synchronous ChangesAsync plugin cleanup
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant PythonCaller
participant clear_async
participant NativeBinding
participant TeardownThread
participant PluginConfiguration
PythonCaller->>clear_async: await clear_async()
clear_async->>NativeBinding: await clear_plugin_configuration_async()
NativeBinding->>TeardownThread: start or join cleanup
TeardownThread->>PluginConfiguration: clear configuration
PluginConfiguration-->>TeardownThread: cleanup result
TeardownThread-->>NativeBinding: completion or mapped error
NativeBinding-->>clear_async: awaitable result
clear_async-->>PythonCaller: cleanup completed
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Signed-off-by: Will Killian <wkillian@nvidia.com>
Signed-off-by: Will Killian <wkillian@nvidia.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/python/src/py_plugin.rs (1)
1042-1044: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winRoute synchronous clearing through
PluginConfigurationClearState.
clear_plugin_configuration_pybypasses the shared state. The core lease prevents concurrent teardown, but a synchronous call duringclear_async()returns a conflict instead of waiting. Start or join the shared clear and block for its completion with the GIL detached. Add a cross-thread test.🤖 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/python/src/py_plugin.rs` around lines 1042 - 1044, Update clear_plugin_configuration_py to route synchronous clearing through PluginConfigurationClearState by starting or joining the shared clear operation, waiting for completion while the GIL is detached, and preserving conflict-free behavior when clear_async() is in progress. Add a cross-thread test covering a synchronous clear overlapping an asynchronous clear.
🤖 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.
Outside diff comments:
In `@crates/python/src/py_plugin.rs`:
- Around line 1042-1044: Update clear_plugin_configuration_py to route
synchronous clearing through PluginConfigurationClearState by starting or
joining the shared clear operation, waiting for completion while the GIL is
detached, and preserving conflict-free behavior when clear_async() is in
progress. Add a cross-thread test covering a synchronous clear overlapping an
asynchronous clear.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: a95095d6-a729-4e37-937e-3f5bdedc0b59
📒 Files selected for processing (2)
crates/python/src/py_plugin.rspython/tests/test_event_sanitizers.py
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: Check / Run
- GitHub Check: Preview docs
🧰 Additional context used
📓 Path-based instructions (14)
**/*.{rs,py}
📄 CodeRabbit inference engine (AGENTS.md)
Follow binding naming conventions in Rust and Python: use
snake_case.
Files:
python/tests/test_event_sanitizers.pycrates/python/src/py_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:
python/tests/test_event_sanitizers.pycrates/python/src/py_plugin.rs
**/*.py
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*.py: When changing the Python wrapper package, tests, or docs tooling, lint with Ruff (E,F,W,I), format with Ruff formatter (120-character lines, double quotes), and passtytype checking.
Add the SPDX license header to all Python source files using the#comment form.
Files:
python/tests/test_event_sanitizers.py
**/*.{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:
python/tests/test_event_sanitizers.pycrates/python/src/py_plugin.rs
{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:
python/tests/test_event_sanitizers.pycrates/python/src/py_plugin.rs
python/tests/**/*.py
📄 CodeRabbit inference engine (.agents/skills/test-python-binding/SKILL.md)
python/tests/**/*.py: Pytest is used to run tests.
Do not add@pytest.mark.asyncioto any test; async tests are automatically detected and run by the async runner.
Do not add a-> Nonereturn type annotation to test functions.
When mocking a class, do not define a new class; useunittest.mock.MagicMockorunittest.mock.AsyncMock, with thespecconstructor argument when necessary.
Name mocked classes with themockprefix, notfake.
Prefer pytest fixtures over helper methods.
Do not repeat fixtures; if a fixture is needed in multiple test files, place it in aconftest.pyfile.
When creating a fixture, use@pytest.fixture(name="<fixture_name>"[, scope="<scope>"])and define the fixture function asdef <fixture_name>_fixture() -> <return_type>:; only specifyscopewhen it is notfunction.
Preferpytest.mark.parametrizeover creating individual tests for different input types.
Files:
python/tests/test_event_sanitizers.py
**/*
📄 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:
python/tests/test_event_sanitizers.pycrates/python/src/py_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.
**/*.{rs,py,go,js,ts}: When observability configuration or lifecycle is exposed, keep FFI and Python, Go, and Node.js binding-native config objects and subscriber/exporter methods aligned in logical knobs and semantics.
Require every OpenTelemetry endpoint to have a type and nonblank destination; resolveheader_envvalues at activation and reject missing, blank, or duplicate headers.
Concatenate layered ATOF sink, ATIF storage, and OpenTelemetry endpoint lists with higher-precedence entries first.
Preserve correct handling of mark events, start/end events, orphan cases, and span or trajectory fields derived from intended event data.
Run affected Rust tests andjust test-rustwhen event fields change; runjust test-python,just test-go, andjust test-nodewhen binding-native configuration or lifecycle changes.
Files:
python/tests/test_event_sanitizers.pycrates/python/src/py_plugin.rs
**/*.{rs,py,js,ts,tsx,go,java,kt,swift}
📄 CodeRabbit inference engine (.agents/skills/add-middleware/SKILL.md)
Add tests covering registration and duplicate names, deregistration and missing names, priority ordering, callback failure policy, scope-local inheritance and cleanup, event payload semantics, immutable mark and scope fields, and parity across affected bindings.
Files:
python/tests/test_event_sanitizers.pycrates/python/src/py_plugin.rs
**/*.{md,mdx,py,go,js,ts}
📄 CodeRabbit inference engine (.agents/skills/maintain-observability/SKILL.md)
Examples and documentation must use each exporter's documented flush/deregister order before shutdown.
Files:
python/tests/test_event_sanitizers.py
{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:
python/tests/test_event_sanitizers.py
**/*.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/python/src/py_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/python/src/py_plugin.rs
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/python/src/py_plugin.rs
🔇 Additional comments (6)
python/tests/test_event_sanitizers.py (3)
511-515: No diff content is shown for this range beyond the change summary ("replaced synchronous plugin cleanup with awaitedplugin.clear_async()"). This matches the new API and the syncclear()RuntimeError-inside-a-running-loop change described elsewhere, so no concern to raise without the actual code.
549-549: No diff content is shown for this range beyond the change summary ("awaitplugin.clear_async()instead of calling synchronousplugin.clear()" in failure-rollback cleanup). Consistent with the new API; no concern to raise without the actual code.
8-10: LGTM!Also applies to: 34-116
crates/python/src/py_plugin.rs (3)
6-10: LGTM!Also applies to: 760-788, 795-798, 969-999, 1002-1004
1048-1057: 🩺 Stability & AvailabilityVerify whether Node.js binding needs equivalent non-blocking teardown.
This adds a Python-specific
clear_plugin_configuration_async_pyto avoid blocking a runningasyncioevent loop during native teardown. Node.js also runs a single-threaded event loop and could hit the same deadlock class if its plugin binding still exposes only a blockingclear(). The current stack outline scopes this cohort to Python files only. Confirm whether the Node.js binding already handles this, or whether a follow-up is needed for parity.As per path instructions, "Flag changes that update one binding without corresponding tests or documentation for the same surface elsewhere."
Source: Path instructions
885-966: 🩺 Stability & AvailabilityNo duplicate native teardown occurs across these paths.
PLUGIN_MUTATION_OWNERpreventsclear_plugin_configuration()from starting while a dynamic host owns the configuration, and the host retains its lease untilclear_inner()completes. Initialization also requires the same ownership state. The reset cannot replace an active clear state through the described sequence.> Likely an incorrect or invalid review comment.
|
/merge |
2 participants
Overview
Prevent
plugin.clear()from deadlocking a running Pythonasyncioloop when queued subscriber delivery depends on an event sanitizer coroutine.Details
plugin.clear_async()and run native teardown on the Rust blocking worker pool so the Python event loop remains available to pending sanitizer callbacks.plugin.clear()raise an actionableRuntimeErrorwhen called from a running event loop.clear_async()and update Python type stubs and documentation examples.Validation:
uv run pre-commit run --all-files: passed.cargo clippy --workspace --all-targets -- -D warnings: passed.just docs: passed.just test-python: 599 passed; 12 tests were blocked by an ignored local.nemo-relay/plugins.tomlusing unsupported observability config version 2.just test-rust: 996 passed; 88 tests were blocked by the same ignored local configuration.Where should the reviewer start?
Start with
python/nemo_relay/plugin.pyfor the public lifecycle contract, thencrates/python/src/py_plugin.rsfor the non-blocking native bridge andpython/tests/test_event_sanitizers.pyfor the deadlock regression.Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)
Summary by CodeRabbit
New Features
await plugin.clear_async(), allowing teardown without blocking Python event loops.Bug Fixes
clear()rejects calls from running event loops and directs callers to the async alternative.Documentation