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

[YuanYuYuan]

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

Role in #282

Instance fix + coverage. Fixes one defect and converts the lock behind it to a tracked type, so a reintroduction panics in debug naming the site instead of hanging. Depends on #255, which merged 2026-08-04.

Issue

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 blocks on a lock the same thread already holds. No race required.

Two paths reach it from ordinary public API:

from inside the callback	what it wants	why it cannot get it
on_set_parameters (re-register)	write()	the same thread still holds the read guard — deadlock, deterministic
set_parameter (set another)	read(), recursively	std::sync::RwLock does not guarantee recursive read; it deadlocks if a writer queues between the two

Evidence

Reverting only crates/hiroz/src/parameter/service.rs and keeping the tests flips exactly one test — which is what makes this evidence rather than a green run.

direction	result	measured on
fix present	4 passed in 5.15s	fb0b082a
production file reverted, tests kept	3 passed; 1 failed in 34.54s	fb0b082a

⚠️ Measured on fb0b082a, not the current tip. The branch has since been rebased onto b0efcbe4 and gained two documentation commits. Nothing since touched the transition path, so the result should still hold — but it has not been re-measured, and this note is here rather than a claim that it has.

What each test is evidence of

Four tests, one detector. The file now says this itself, so the distinction survives away from this description.

test	role
parameter_on_set_callback_reregistering_does_not_deadlock	detector — fails on its deadline without the fix
parameter_on_set_callback_setting_another_parameter_does_not_deadlock	guard, not detector: recursive read() usually succeeds unless a writer is queued, so against unfixed source it is a coin flip
two service scenarios	the audit's negative result — they pass with or without the fix, because it does not touch services

The service tests are not padding. #256 was found by auditing the subsystems the pub/sub fix did not cover — services, actions, parameters — and "services came back clean" is a claim this file substantiates rather than asserts.

One property worth noting separately

Reverting the guard-drop while keeping the tripwire makes the violation fire on test_parameter_validation_callback — an ordinary parameter test, not a deadlock test. That is the point of the barrier: any existing test that merely exercises a callback becomes a detector for the whole class.

What this PR does

• 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.
• Convert the lock to TrackedRwLock; both call sites dispatch through invoke_user_callback!.
• Four deadline-guarded scenarios in reentrant_service.rs. 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: ZServer::build_internal calls handler.handle(query) with no hiroz lock held, the action server's user handler is awaited with no guard on the stack, and the action client never runs user code on a zenoh thread.

Coverage limit: ParameterState has two lock fields and this converts one. store stays a plain RwLock, so the tripwire cannot see it — a future change moving the callout inside the store block would fire nothing. Tracked in #283.

Breaking changes

What changes	Who is affected	Before → After	Action
Re-entrant set_parameter from on_set recurses instead of blocking	a callback that sets another parameter unconditionally	silent hang → fatal runtime error: stack overflow	terminate the recursion

Not affected: a callback that guards its own re-entry — the ordinary case, and what the tests exercise.

Intended direction: an observable, stoppable crash beats an unkillable hang, and it is what any other ROS 2 stack gives. But it is a behaviour change, not "None" — the same trade #250 discloses for subscribers, and the reason #285 exists.

Checklist

• Added/updated tests/documentation (if applicable)
• Both directions measured — see the evidence table, including which commit
• Evidence not re-measured after the rebase onto b0efcbe4 and the two doc commits
• ./scripts/check-local.sh — passed on the original commits, not re-run since. CI covers it.

[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`.