Repository navigation
RUST/AGENT: fix agent deadlocks and c-strings - #1367
Conversation
Fix a deadlock in fetch_remote_md due to locking of an already locked RwLock. Fix several places calling c++ api with non-null terminated strings. Also make test_etcd_metadata_exchange run on ci and more consistent with the c++ version of the same test.
|
👋 Hi tomerg-nvidia! Thank you for contributing to ai-dynamo/nixl. Your PR reviewers will review your contribution then trigger the CI to test your changes. 🚀 |
|
/build |
1 similar comment
|
/build |
|
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:
📝 WalkthroughWalkthroughAdds a reusable ETCD startup helper and replaces inline ETCD bootstraps in CI/test scripts; hardens Rust agent FFI by converting remote identifiers to CStrings and reusing lock guards; and updates Rust metadata tests to use cross-agent backends with polling-based synchronization and invalidation checks. Changes
Sequence Diagram(s)sequenceDiagram
participant CI as CI Script / Test Runner
participant ETCD as ETCD Server
participant Agent1 as Agent1 (backend1)
participant Agent2 as Agent2 (backend2)
CI->>ETCD: start_etcd_server("/nixl/..._ci")
ETCD-->>CI: ready
CI->>Agent1: launch agent1 (uses backend1)
CI->>Agent2: launch agent2 (uses backend2)
Agent1->>ETCD: publish metadata
Agent2->>ETCD: poll & fetch remote metadata
Agent2-->>Agent1: observe remote metadata
Agent2->>ETCD: request remote invalidation
ETCD-->>Agent1: propagate invalidation
Agent2->>ETCD: poll until invalidation visible
ETCD-->>Agent2: invalidation observed
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Tip CodeRabbit can generate a title for your PR based on the changes with custom instructions.Set the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.gitlab/test_rust.sh:
- Around line 58-60: The etcd invocation uses unquoted shell variable expansions
(NIXL_ETCD_ENDPOINTS, NIXL_ETCD_PEER_URLS) which can lead to
word-splitting/globbing; update the etcd command invocation to wrap each
variable expansion in double quotes (e.g. "--listen-client-urls
\"$NIXL_ETCD_ENDPOINTS\"" etc.) so each argument is passed intact and safe from
globbing when calling etcd.
In `@src/bindings/rust/tests/tests.rs`:
- Around line 724-735: The three fixed thread::sleep(Duration::from_secs(10))
calls are making the test slow and flaky; replace each sleep with a bounded
polling loop that checks the concrete observable condition (e.g., that agent1's
metadata is invalidated/visible in etcd after
agent1.invalidate_local_md(Some(&opt_args))? and that agent2's metadata is
invalidated after agent2.invalidate_remote_md("EtcdAgent1")?). Implement polling
using Instant::now() and a timeout (e.g., 10s) with a short sleep interval
(e.g., 50–200ms) between attempts, call the appropriate read/check method on the
agent or etcd client to verify the metadata state, break early on success, and
return an error or panic if the timeout elapses so tests fail fast instead of
waiting fixed sleeps. Ensure you replace each specific sleep that follows
invalidate_local_md and invalidate_remote_md with this polling pattern.
- Around line 705-715: The test is reusing a single OptArgs containing only
backend1 for both agents; create an agent-specific OptArgs for agent2 instead of
discarding backend2: after calling agent2.create_backend (the backend produced
by agent2.create_backend / the params from agent2.get_plugin_params), construct
a second OptArgs (via OptArgs::new) and add backend2 with
opt_args2.add_backend(&backend2), then use the first opt_args when invoking
agent1.send_local_md(...) and use opt_args2 when invoking
agent2.fetch_remote_md(...), matching the pattern used in
test_send_notification.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
.gitlab/test_rust.shsrc/bindings/rust/src/agent.rssrc/bindings/rust/tests/tests.rs
|
/build |
1 similar comment
|
/build |
Signed-off-by: Tomer Gilad <tgilad@nvidia.com>
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)
src/bindings/rust/src/agent.rs (1)
1112-1129: 🧹 Nitpick | 🔵 TrivialCString temporary lifetime is safe but consider explicit bindings for clarity.
The pattern
CString::new(remote_agent)?.as_ptr().cast()is technically safe because Rust temporaries live until the end of the enclosing statement. However, this relies on subtle lifetime semantics that can be error-prone during refactoring.The
Dropimplementation (lines 1144-1153) uses a clearer pattern with explicit binding. Consider applying the same style here for consistency and maintainability:♻️ Optional refactor for consistency
fn invalidate_remote_md(&mut self, remote_agent: &str) -> Result<(), NixlError> { unsafe { if self.remotes.remove(remote_agent) { + let c_remote = CString::new(remote_agent)?; nixl_capi_invalidate_remote_md( self.handle.as_ptr(), - CString::new(remote_agent)?.as_ptr().cast(), + c_remote.as_ptr().cast(), ); } else { return Err(NixlError::InvalidParam); } } Ok(()) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bindings/rust/src/agent.rs` around lines 1112 - 1129, In invalidate_all_remotes (and the earlier branch that calls nixl_capi_invalidate_remote_md with CString::new(...).as_ptr().cast()), avoid relying on the temporary CString's end-of-statement lifetime: bind the CString to a local variable (e.g., let c_remote = CString::new(remote)?;) and pass c_remote.as_ptr().cast() to nixl_capi_invalidate_remote_md so the CString remains alive for the call, matching the explicit binding style used in the Drop implementation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/bindings/rust/src/agent.rs`:
- Around line 1112-1129: In invalidate_all_remotes (and the earlier branch that
calls nixl_capi_invalidate_remote_md with CString::new(...).as_ptr().cast()),
avoid relying on the temporary CString's end-of-statement lifetime: bind the
CString to a local variable (e.g., let c_remote = CString::new(remote)?;) and
pass c_remote.as_ptr().cast() to nixl_capi_invalidate_remote_md so the CString
remains alive for the call, matching the explicit binding style used in the Drop
implementation.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
.ci/scripts/common.sh.gitlab/test_cpp.sh.gitlab/test_nixlbench.sh.gitlab/test_python.sh.gitlab/test_rust.shsrc/bindings/rust/src/agent.rssrc/bindings/rust/tests/tests.rs
|
/build |
Signed-off-by: Tomer Gilad <tgilad@nvidia.com>
|
/build |
1 similar comment
|
/build |
|
/build |
|
/ok to test 6af91bc |
|
/build |
|
/ok to test c958d15 |
|
/build |
|
/ok to test |
@tomerg-nvidia, there was an error processing your request: See the following link for more information: https://docs.gha-runners.nvidia.com/cpr/e/1/ |
|
/ok to test 392eb91 |
This branch had an error being deployed
5 participants
What?
Fix a deadlock in fetch_remote_md due to locking of an already locked RwLock.
Fix several places calling c++ api with non-null terminated strings.
Make test_etcd_metadata_exchange run on ci and more consistent with the c++ version of the same test.
Why?
test_etcd_metadata_exchange was not passing before that. It was also not running in CI due to the missing etcd config.
When running it manually it hung forever because of the deadlock.
Summary by CodeRabbit
Chores
Refactor
Tests