fix(parameter): stop deadlocking on re-entry from an on_set callback

[YuanYuYuan]

Description

Fixes #256

ParameterState::validate_and_apply invoked the user's on_set callback while holding the RwLock read guard the callback is registered under. Re-entering the parameter API from that callback — re-registering it, or setting another parameter — blocks on a lock the same thread already holds. No race required.

Depends on pr/1-reentrancy-tripwire, which supplies TrackedRwLock and invoke_user_callback!. Branched off it accordingly.

Stacked PR — this PR's base is pr/1-reentrancy-tripwire, not main. The diff therefore shows only this branch's own commit. Merge pr/1 first; GitHub will retarget this one to main automatically when it does.

Key Changes

• crates/hiroz/src/parameter/service.rs — clone the Arc<SetCallback> out and let the read guard drop before invoking. SetCallback is already an Arc, so this is one refcount bump and no structural change. The store guard in the same function was already correctly scoped and is untouched.
• The lock becomes a TrackedRwLock and both call sites dispatch through invoke_user_callback!, so a reintroduction panics in debug naming the site instead of hanging.
• crates/hiroz-tests/tests/reentrant_service.rs — four deadline-guarded scenarios: a service handler publishing on its own session, a service handler calling a second service on its own session, an on_set callback setting another parameter, and an on_set callback re-registering itself. The harness distinguishes a worker panic from a timeout, so an ordinary assertion failure is not misreported as a deadlock.

Services and actions were swept for the same shape and are clean, demonstrably rather than by assumption: ZServer::build_internal declares a plain zenoh queryable and calls handler.handle(query) with no hiroz lock held, and zenoh core clones queryable callbacks and drops its state before invoking them, exactly as it does for subscribers; the action server's user handler is awaited in run_driver_loop with no guard on the stack, SafeGoalManager::modify is only ever handed crate-internal closures, and the action client never runs user code on a zenoh thread at all — feedback and status are forwarded into channels and consumed on the user's own task.

What fails without this

Detector proven in both directions.

Against the unfixed source, parameter_on_set_callback_reregistering_does_not_deadlock fails on its 30 s deadline:

3 passed; 1 failed   (35.59s)

With the fix, all four pass in 6.59 s.

Separately, reverting only the guard-drop and keeping the tripwire makes the violation fire on test_parameter_validation_callback — an ordinary parameter test, not a deadlock test:

thread 'test_parameter_validation_callback' panicked at crates/hiroz/src/reentrancy.rs:103:9:
hiroz re-entrancy rule violated at `ParameterState::validate_and_apply (per-parameter)`:
about to invoke a user callback with 1 hiroz lock guard(s) still live on this thread.

test result: FAILED. 10 passed; 3 failed

Removing the violation restores 13/13. That property is the point of the barrier: any existing test that merely exercises a callback becomes a detector for the whole class.

No regression: parameter_tests 13/13, hiroz --lib 286/286, reentrant_service 4/4.

Rebased onto pr/1's current tip (a70f7336) and re-verified there. The rebase replayed this branch's two commits with no conflicts, and its own diff is byte-identical before and after — it adds only reentrant_service.rs and parameter/service.rs.

Measured on the rebased head:

clippy -p hiroz --all-targets --features jazzy -- -D warnings              clean
clippy -p hiroz-tests --all-targets --features ros-msgs,jazzy -- -D warnings   clean
hiroz --lib                                                     286 passed
reentrant_service                                                 4 passed
parameter_tests                                                  13 passed
cargo test -p hiroz-tests --test feature_gate  (no features)     refuses to build:
    "hiroz-tests requires the `ros-msgs` feature"

The last line matters after a rebase: pr/1 enforces that requirement from build.rs, and this confirms picking up that commit did not defeat it.

None of this ran on a GitHub runner, and it cannot until the base moves: ci.yml triggers only on pull requests targeting main, so a PR based on pr/1 gets 3 checks instead of 27. Re-verify after the retarget to main.

Breaking Changes

One, and it is the same trade pr/2 discloses for subscribers.

An on_set callback that unconditionally sets another parameter used to deadlock — the second set blocked on a lock the same thread held. It now recurses: the guard is dropped before the callback runs, so the nested set acquires the lock, invokes the callback again, and repeats until the stack is exhausted. The failure mode changes from a silent hang to a fatal runtime error: stack overflow.

This is the intended direction — an observable, stoppable crash beats an unkillable hang, and it is the failure any other ROS 2 stack would give — but it is a behaviour change, not "None". A callback that guards its own re-entry (the ordinary case, and what the new tests do) is unaffected.

Notes

cargo doc is red on main today for reasons unrelated to this PR (three unresolved intra-doc links in error.rs), so this branch inherits a failing rustdoc check. It adds no doc links of its own; pr/0-rustdoc-links clears the gate for every branch in this series.

Run with --features ros-msgs,jazzy, which is what crates/hiroz-tests/build.rs requires package-wide on pr/1. Verified: cargo check -p hiroz-tests --tests --features ros-msgs,jazzy succeeds on this branch.

Checklist

• Ran ./scripts/check-local.sh successfully
• Added/updated tests/documentation (if applicable)

[Copilot]

Pull request overview

Fixes parameter callback re-entry deadlocks by releasing the callback lock before invoking user code.

Changes:

• Clones the callback outside its lock guard.
• Adds debug-time re-entrancy detection.
• Adds deadline-guarded service and parameter regression tests.

Reviewed changes

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

File	Description
crates/hiroz/src/parameter/service.rs	Prevents callback-under-lock deadlocks.
crates/hiroz-tests/tests/reentrant_service.rs	Adds re-entrancy regression coverage.

Comments suppressed due to low confidence (2)

crates/hiroz-tests/tests/reentrant_service.rs:311

• This comment says validate_and_apply still holds the read lock across the callback, which the production change specifically prevents. Describe this as the former defect so future readers do not infer that the regression remains.

/// This is the deterministic form of the same defect. `validate_and_apply` holds
/// `on_set_callback.read()` across the user callback; `on_set_parameters` takes
/// `on_set_callback.write()`. A callback that re-registers therefore asks the
/// same thread for a write lock while it still holds a read lock on the same
/// `std::sync::RwLock` — a guaranteed self-deadlock, no race required.

crates/hiroz-tests/tests/reentrant_service.rs:337

• At this point the callback no longer holds the read lock, so this inline comment is stale. Make clear that it describes the pre-fix failure mode rather than the current execution.

                // Re-register from inside the callback: write lock requested
                // while this thread still holds the read lock.

---

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

[Copilot]

Pull request overview

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

Comments suppressed due to low confidence (1)

crates/hiroz-tests/tests/reentrant_service.rs:16

• This points readers to reentrant_publish.rs, but no such file exists in the repository. Please remove the stale reference (or name the actual test/helper if one was intended).

//! Every scenario runs on a dedicated thread behind a hard deadline, so a
//! re-entrancy deadlock fails the test instead of wedging the suite — the same
//! shape as `reentrant_publish.rs`.