fix(event,graph): run event callbacks outside the locks they live under

[YuanYuYuan]

Part of #282 — the defect class, the shared fix shape and the merge order are stated there.

Role in #282

Instance fix plus coverage, and the widest one. It converts the graph and event registry locks. It is also why the debug-time monitor counts guards rather than setting a flag: the liveliness path runs with three tracked-or-untracked locks held, and a single bit cannot be cleared soundly on release.

Consumes the types added by #255 (TrackedMutex, invoke_user_callback!), which is merged. Targets main; the merge base is af00f1ed, the current tip of main.

Issue

Fixes #259 — three failures spanning two defect classes.

#	Failure	Class	Evidence
1	GraphEventManager invoked registered callbacks while holding the registries they live in. The hot path is the liveliness subscriber in Graph::new_with_pattern, which also held the GraphData mutex — so three locks (GraphData, event_callbacks, entity_topics) on every liveliness token, around user code the rmw layer hands to an rclcpp executor	re-entrancy	deterministic hang, reproduced by test
2	Lock-order hazard. The liveliness callback held GraphData across the acquisition of event_callbacks, so that order constrained every other path; add_local_entity and the rmw callers reach event_callbacks without GraphData. #259 classifies this as an ABBA inversion	lock ordering	established by reading, not by executing — no test reproduces it
3	EventsManager is shared as Arc<Mutex<..>>, so &mut self proves the caller holds it — and update_event_status fired the callback from there. All eight call sites in crates/rmw-zenoh-rs/src/rmw.rs had this shape	re-entrancy	deterministic hang, reproduced by test

Failure 2 is not covered by the "no callout under a lock" property. A guard count cannot see lock order.

What this PR does

#	Change	Why
1	trigger_event_with_policy, trigger_graph_change and the guard-condition sweep collect under the lock, drop every guard, then notify	the class fix
2	Graph::new_with_pattern stops holding GraphData across trigger_graph_change	the third lock of failure 1, and the GraphData-then-event_callbacks leg of failure 2
3	record_event_status_with_policy returns the callback (#[must_use]); update_shared_event_status[_with_policy] becomes the entry point holders of the shared manager use	failure 3
4	event_callbacks, entity_topics and graph_guard_conditions become TrackedMutex; the two GraphEventManager callouts route through invoke_user_callback!	a reintroduction panics naming the site instead of hanging
5	GuardConditionState::take_triggered (one swap) replaces read-then-reset in rmw_wait	independent bug: a trigger() landing between the two atomic ops was swallowed — that wake was reported and the next rmw_wait blocked until timeout
6	GraphGuardConditionTrigger removed; registrations become Arc<dyn GraphGuardCondition>, and NodeImpl keeps the same allocation so teardown can unregister it	triggering outside the lock made raw-pointer registrations a use-after-free — rmw_destroy_node unregisters and frees while a trigger is in flight
7	The event user_data pointer stops being laundered through usize; it moves in a newtype with explicit unsafe impl Send/Sync	the as usize round-trip silenced the Send/Sync check that was flagging a pointer crossing threads, and destroyed the pointer's provenance

Change 5 is not a re-entrancy defect — it was found while reading the same file. Its symptom is a latency spike rather than a hang, which is how it survived.

Kept whole deliberately. Splitting on the crate boundary was considered and rejected: the rmw-zenoh-rs half implements a trait the hiroz half defines, so an rmw-only PR would not compile until the hiroz-only PR merged — another stacked PR, which is what this series exists to remove.

Evidence

Six tests are added: five deadlock scenarios (three in reentrant_graph_event.rs, two in reentrant_event_status.rs) and one unit test in event.rs pinning that the guard-condition registry owns its registrations.

measurement	result	revision
the six new tests, plus fmt, clippy and doc build	all pass, all clean	e285962c (current tip)
revert event.rs and graph.rs, keep the three reentrant_graph_event tests	3 fail — two on the 30 s deadline, one because the re-entrant graph query returned 0 publishers	cbabd82c (earlier revision of this branch, before the rebase onto af00f1ed)
revert wholesale, keep the two reentrant_event_status tests	does not compile — both call an entry point this PR introduces	as above
reinstate the callout inside update_shared_event_status_with_policy and re-run	2 fail on the 30 s deadline	temporary revert commit on the same revision

The third row is why the event-status pair is necessary rather than a passenger: it does not detect the history (a wholesale revert stops the file compiling) but it does detect the property. Reinstate the callout and both tests hang. That is the regression still reachable, since the old entry point is gone.

The reinstated-callout run fails on the harness's timeout arm, not its panic arm, so it is classified as a deadlock rather than an assertion failure.

⚠️ The two revert directions were measured on the pre-rebase revision of this branch and have not been re-run at e285962c. The fix-present row was measured at the current tip.

Breaking changes

Three public items in hiroz, all source-breaking. rmw-zenoh-rs is consumed over the C ABI, so its changes are not part of this list.

#	Item	Before → After	Action	Effort
1	pub type EventCallback	Box<dyn Fn(i32) + Send + Sync> → Arc<…>	Box::new → Arc::new	mechanical
2	pub type GraphGuardConditionTrigger	removed; register_graph_guard_condition / unregister_graph_guard_condition now take Arc<dyn GraphGuardCondition>	define a type and impl GraphGuardCondition	not mechanical — a closure cannot be substituted
3	GraphEventManager::set_guard_condition_trigger	removed	register the guard condition itself; there is no separate trigger hook	small, but no drop-in

Neither of the first two is a rename:

• Box cannot be cloned, so it cannot be lifted out of a guard — and lifting the callback out is the fix.
• The closure form cannot express the ownership a guard condition needs: the registry must keep the target alive across a trigger performed with no lock held.

Additive, not breaking: EventsManager::install_callback and EventsManager::record_event_status_with_policy are new; update_shared_event_status[_with_policy] are new free functions.

Coverage this does not have

gap	detail
GraphData is untracked	it is a parking_lot::Mutex. Reintroduce the deleted guard at the top of the liveliness callback and assert_no_guards_held still reports 0. That hot path is caught by exactly one test — graph_change_callback_querying_the_graph_does_not_deadlock — which needs a router, is #[serial], and polls for propagation
nothing covers failure 2	lock ordering is invisible to a guard count
five callouts in event.rs bypass invoke_user_callback!	EventsManager::set_callback (backlog), EventsManager::update_event_status_with_policy, gc.trigger() inside trigger_graph_change, update_shared_event_status_with_policy, RmEventHandle::set_callback (backlog). Only the two GraphEventManager registry callouts are routed
change 5 has no detector — measured, not suspected	reverting take_triggered to the racy read-then-reset leaves the suite green: 93 integration tests and 288 library unit tests all passed. The lost-wake bug is guarded only by the reading that found it
changes 6 and 7 are believed to have no behavioural detector	stated as a belief; not probed

⚠️ Known residual this fix introduces

An event callback can fire after rmw_event_set_callback(event, nullptr, nullptr) returns, and invoke a stale user_data. Tracked in #287 with the design settled.

Trigger	Thread A clones the Arc out and releases the manager lock. Thread B (executor teardown) calls set_callback(nullptr), which returns; rclcpp frees the pointee. Thread A then calls the closure.
Why it is new	The invocation used to happen inside Mutex<EventsManager>, which RmEventHandle::set_callback also takes, so a detach could not return mid-invocation. That exclusion was accidental but load-bearing.
Why the Arc does not help	It owns the closure. The raw C pointer it captured is rclcpp's — which is why change 6's remedy cannot be reused here.
Not fixable by	re-locking (that is the deadlock this removes), or a flag checked in the closure (the free can land between check and call, and the code then looks defended)
What does fix it	detach blocks until any in-flight invocation returns, with a bypass when called from the invoking thread — the shape of std::stop_callback's destructor contract

This does not block merging, but it is a real trade, and it is not a gap shared with upstream. rmw_zenoh_cpp does not have this seam: EventsManager::event_set_callback (rmw_zenoh_cpp/src/detail/event.cpp:105) and EventsManager::trigger_event_callback (:133) both take event_mutex_ (:118, :143) and invoke the user callback while holding it (:126, :146). Upstream's detach therefore cannot return mid-invocation.

So upstream takes the other horn: it keeps the lock, has no use-after-free window, and does have the deadlock this PR removes. Closing #287 is hardening beyond what any peer implementation provides, not catching up to one.

How upstream compares

Read from rmw_zenoh_cpp source, not inferred. Paths below are relative to rmw_zenoh_cpp/src/detail/.

path	rmw_zenoh_cpp	this PR
graph discovery callbacks	collect, release, call — parse_put unlocks graph_mutex_ at graph_cache.cpp:428 before invoking, with the graph_mutex_ → discovery_mutex_ order documented at :205	same
QoS event callbacks	still under two locks: parse_put holds graph_mutex_ (graph_cache.cpp:367) through handle_matched_events_for_put (:180) into update_event_counters, which locks events_mutex_ (:1496) and invokes the callback at :1528	released
event set / trigger	invoked under the same mutex event_set_callback takes (event.cpp:118, :126, :143, :146)	released → #287

• The fix shape is independently confirmed. Upstream reached collect-release-call for discovery, with a comment giving the same reason (graph_cache.cpp:210-213).
• This PR is ahead on the event path. Upstream still holds two locks across the QoS event callout — that is failure 1, unfixed in C++.
• The Event callback can outlive rmw_event_set_callback(.., null) and use freed user_data #287 tradeoff is a genuine choice, not a regression against a peer. Upstream keeps the lock: no use-after-free, but the deadlock. This takes the other horn. Neither has both, which is the argument for the barrier rather than for either status quo.

Checklist

• Added tests and documentation
• Both directions measured — see the evidence table, including which revision each row was measured on
• Review findings addressed: the lost wake (change 5) and the immutable guard-condition borrow are fixed here, the teardown window is documented above, and Event callback can outlive rmw_event_set_callback(.., null) and use freed user_data #287, rmw never unregisters entity event callbacks: leak plus a stale user_data kept reachable #288 and Two event-API sharp edges: set_callback fires the backlog under the caller's lock; duplicate register + one unregister drops all #289 are filed
• Comments tightened — the &mut self rule is stated once on EventsManager and referenced, rather than restated at three call sites
• ./scripts/check-local.sh not re-run since the rebase; CI covers it

[Copilot]

Pull request overview

Moves graph and event callbacks outside mutex guards to prevent re-entrant deadlocks and lock-order inversion.

Changes:

• Snapshots callbacks before invocation and introduces shared event-status helpers.
• Releases graph-data locks before graph notifications.
• Adds re-entrancy regression tests.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
crates/hiroz/src/event.rs	Refactors callback storage, locking, and dispatch.
crates/hiroz/src/graph.rs	Narrows graph-data guard lifetimes.
crates/rmw-zenoh-rs/src/rmw.rs	Uses shared event-status helpers.
crates/rmw-zenoh-rs/src/context.rs	Constructs the trigger callback with Arc.
crates/hiroz-tests/tests/reentrant_graph_event.rs	Tests graph callback re-entrancy.
crates/hiroz-tests/tests/reentrant_event_status.rs	Tests event-status callback re-entrancy.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (6)

crates/hiroz/src/event.rs:81

• set_callback still invokes the backlog callback from a &mut self method. A public caller using the normal shared shape (manager.lock().unwrap().set_callback(...)) therefore keeps the outer Mutex<EventsManager> guard alive here, and a callback that calls take_event deterministically deadlocks. The install_callback split only avoids this through RmEventHandle; this wrapper needs the same shared-manager treatment, or it must be restricted/deprecated for exclusively owned managers. This invocation should also use the repository's invoke_user_callback! tripwire.

            callback(unread_count);

crates/hiroz/src/event.rs:457

• The shared-manager path correctly drops its mutex, but this is still invocation of the user callback and must use invoke_user_callback! under the rule in reentrancy.rs:43-49. Calling it directly leaves this newly introduced dispatch path outside the debug-time guard-lifetime detector.

    if let Some(callback) = callback {
        callback(change);
    }

crates/hiroz/src/event.rs:508

• This backlog notification is user code, so invoking it directly violates the explicit convention in reentrancy.rs:43-49 that every user-code call site uses invoke_user_callback!. Wrapping it preserves the lock-release fix while allowing debug builds to catch any tracked guard retained by future callers.

        if unread_count != 0 {
            callback(unread_count);
        }

crates/hiroz/src/event.rs:347

• GraphGuardCondition::trigger is an externally implemented callback (and the surrounding comment explicitly says it may re-enter the manager), but this dispatch bypasses invoke_user_callback!. Per reentrancy.rs:43-49, wrap this call so debug builds detect if any tracked hiroz guard remains live at this callback boundary.

        for gc in guard_conditions {
            gc.trigger();
        }

crates/hiroz/src/event.rs:203

• The PR's Breaking Changes section says GraphGuardConditionTrigger merely changes from Box to Arc, but this diff actually removes that public alias and set_guard_condition_trigger, introduces a new trait, and changes registration from a raw pointer to Arc<dyn GraphGuardCondition>. Downstream migration is therefore not the stated one-token Box::new → Arc::new update; update the compatibility design or document the full public API break and migration.

pub trait GraphGuardCondition: Send + Sync {
    /// Wake whatever is waiting on this guard condition.
    ///
    /// Called with no hiroz lock held, possibly concurrently, and possibly
    /// after the corresponding C handle has been destroyed.
    fn trigger(&self);

crates/hiroz/src/event.rs:123

• This is user callback dispatch, but it bypasses the mandatory re-entrancy tripwire. reentrancy.rs:43-49 requires every user-code invocation to go through invoke_user_callback!; without it, this public wrapper can silently invoke user code while another tracked hiroz guard is live.

This issue also appears in the following locations of the same file:

• line 345
• line 455
• line 506

        if let Some(callback) =
            self.record_event_status_with_policy(event_type, change, policy_kind)
        {
            callback(change);
        }

[Copilot]

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (7)

crates/hiroz/src/event.rs:203

• This public trait replaces GraphGuardConditionTrigger and the diff also removes set_guard_condition_trigger and changes both registration signatures. The PR description instead says the alias merely changes from Box to Arc and lists only two alias-shape breaks. Please update the breaking-change and migration notes (or preserve compatibility) so downstream impact is accurately disclosed.

pub trait GraphGuardCondition: Send + Sync {
    /// Wake whatever is waiting on this guard condition.
    ///
    /// Called with no hiroz lock held, possibly concurrently, and possibly
    /// after the corresponding C handle has been destroyed.
    fn trigger(&self);

crates/hiroz/src/event.rs:122

• This callback invocation bypasses the mandatory invoke_user_callback! check documented in crates/hiroz/src/reentrancy.rs:43-49. The direct-manager path can still be reached while another tracked hiroz guard is live, so use the tripwire here as well.

            callback(change);

crates/hiroz/src/event.rs:346

• GraphGuardCondition is a public, externally implementable trait, so trigger() is also an invocation of user-supplied code. Per the invariant in crates/hiroz/src/reentrancy.rs:43-49, dispatch it through invoke_user_callback! just like the endpoint callbacks below.

            gc.trigger();

crates/hiroz/src/event.rs:456

• The shared-manager callback is user code but this new dispatch bypasses the invoke_user_callback! invariant in crates/hiroz/src/reentrancy.rs:43-49. Use the macro after dropping the manager guard so upstream tracked-lock regressions are detected.

        callback(change);

crates/hiroz/src/event.rs:507

• This backlog dispatch is another user callback site that skips the required tripwire (crates/hiroz/src/reentrancy.rs:43-49). Wrapping it preserves the lock-free behavior while detecting callers that still hold a tracked hiroz guard.

            callback(unread_count);

crates/hiroz/src/event.rs:245

• Releasing event_callbacks before inserting entity_topics makes registration observable in a partial state: trigger_event can already invoke the new callback, while a concurrent trigger_graph_change can miss it because its topic is not present yet. Keep the callbacks guard until the topic insertion completes (the trigger path already takes these locks in the same order) so registration has one atomic visibility point.

        {
            let mut callbacks = self.event_callbacks.lock().unwrap();
            let entity_callbacks = callbacks.entry(entity_gid).or_default();
            entity_callbacks.insert(event_type, Arc::new(callback));
        }

crates/hiroz/src/event.rs:81

• This invokes externally supplied callback code directly, bypassing the repository's required re-entrancy tripwire (crates/hiroz/src/reentrancy.rs:43-49). Route it through invoke_user_callback! so a callback reached while any tracked guard is live fails diagnostically instead of hanging.

This issue also appears in the following locations of the same file:

• line 122
• line 346
• line 456
• line 507

            callback(unread_count);