feat(hiroz): debug-time barrier against callbacks under a lock

[YuanYuYuan]

Description

Closes #254

This PR fixes no defect. It lands a regression barrier — the enforcement mechanism for an invariant that has been violated five separate times in this repository, each time with the same shape: a user callback invoked while a non-reentrant hiroz lock guard is still live on the calling thread.

Scope, stated up front: this PR delivers the mechanism, not its call sites. Nothing in production code uses TrackedMutex, TrackedRwLock, assert_no_guards_held or invoke_user_callback! on this branch — every use is inside reentrancy.rs and its own unit tests. The five call sites arrive with the fixes that need them (#257 parameter, #260 event/graph, #262 rmw), each of which converts its lock and routes its callback through the macro. So the barrier is inert as merged and becomes load-bearing as those land. That ordering is deliberate: the type has to exist before three PRs can reference it, and bundling the conversions here would make this PR the whole series.

It should land first. Three of the fixes in this series will not compile without it.

Key Changes

• crates/hiroz/src/reentrancy.rs (new) — TrackedMutex<T> / TrackedRwLock<T> wrap the std::sync types; their guards carry a GuardCount that increments a thread-local on acquire and decrements on drop. Field order is fixed so the inner guard releases before the counter decrements. assert_no_guards_held(site) panics if any tracked guard is live, naming the call site; invoke_user_callback!(site, call) does both in one line. Everything is behind debug_assertions and compiles to nothing in release — tests and CI run in debug, which is where the assertion is wanted.

• crates/hiroz-tests/build.rs — the package-wide half of the feature enforcement. tests/feature_gate.rs fails loudly on a featureless build, but only if Cargo selects that target: cargo test -p hiroz-tests --test cache with no features builds only cache, the crate-level cfg compiles it to an empty binary, 0 passed is reported, and the guard never runs. A build script runs for every build of the package regardless of target selection, so it is the only place the requirement can be stated once and hold everywhere. Consequence, declared in both places: hiroz-tests has no supported featureless configuration.

• scripts/test-pure-rust.nu, .github/workflows/ci.yml, crates/hiroz-tests/tests/feature_gate.rs — a barrier that is not executed is not a barrier. hiroz-tests was linted under interop features but tested under none. Several suites are #![cfg(feature = "ros-msgs")], and a crate-level cfg that is not satisfied compiles to an empty test binary reporting 0 passed, which is indistinguishable from green. That is what CI saw: the clippy step compiled the files, the test step did not run them, and four re-entrancy tests had no coverage in the job that gates every PR. run-tests now runs the workspace excluding hiroz-tests, then hiroz-tests with ros-msgs,jazzy; coverage gets the same features for the same reason. The features need no ROS installation — hiroz-msgs bundles the definitions — so there was never a reason to run the crate blind.

• scripts/check-local.nu, scripts/check-local.sh — check-local.nu (the local gate) was missing four checks that check-local.sh (what CI runs) has: check-console, test-shm, check-distro-features and the rustdoc intra-doc link check. Neither passed --all to cargo fmt, which silently skips hiroz-py and rmw-zenoh-rs. Three of five failures in a recent CI run landed in exactly that blind spot, so no local run could have caught them.

What fails without this

No failing baseline, and that is the honest answer: this PR fixes no bug. The justification is that it makes an entire class of defect detectable rather than each instance individually.

What can be shown is that the detector detects, which matters more here than usual — a check that has never printed a failure is unvalidated. Two demonstrations, and they are not equally scoped:

1. Self-contained, reproducible on this branch. reentrancy.rs carries four unit tests of its own, including assert_fires_while_a_guard_is_live — a should_panic test that fires the assertion with a live tracked guard, so the tripwire cannot silently rot into a no-op. hiroz --lib goes 282 -> 286. This is the only evidence this PR alone can produce, because nothing else on this branch holds a tracked guard.

2. End-to-end, and it requires #257's wiring — not reproducible on this branch alone. Run against pr/1 + pr/3 combined, reintroducing the parameter-service violation (holding the read guard across the callback instead of cloning the Arc out) makes the barrier fire on test_parameter_validation_callback, an ordinary parameter test rather than 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.
A callback that re-enters hiroz here will deadlock. Collect what you need under the
lock, drop the guard, then call — see `GraphEventManager::trigger_graph_change` for
the pattern.

test result: FAILED. 10 passed; 3 failed

Removing the violation restores 13/13. That it fired on an ordinary test is the point: the previous barrier was a hand-written deadline test per known defect, which by construction cannot catch the next one. Read this as evidence about the mechanism's behaviour once wired, which is what #257/#260/#262 do — not as something a reviewer can reproduce from this PR's tree.

For the test-wiring half: crates/hiroz-tests/feature_gate.rs fails loudly with a featureless invocation and passes with --features ros-msgs,jazzy, and build.rs closes the narrower --test <name> path that would otherwise bypass it.

A cheaper option was tried first and rejected on evidence. clippy::significant_drop_in_scrutinee reports zero hits on code containing five genuine instances of the defect, because the shape is if let Ok(cb) = holder.lock() — which binds the guard, so it is not a scrutinee temporary. The defect is a dynamic guard lifetime spanning a dynamic dispatch through Arc<dyn Fn> or an opaque extern "C" fn pointer; static analysis sees through neither, a counter sees both.

Breaking Changes

None. Zero cost in release builds — the counter operations and the assertion are #[cfg(debug_assertions)].

hiroz-tests now requires --features ros-msgs to build at all. That is a change to how the test crate is invoked, not to any published API, and it is the point of the change rather than a side effect.

Notes

• The tripwire is wired into the guards behind the defects in this series, not all 191 lock acquisition sites in the workspace. Extending it to the rest of hiroz, plus a clippy.toml disallowed-types entry forcing new code through TrackedMutex/TrackedRwLock, would make the barrier total rather than targeted. Worth doing; out of scope here.
• cargo doc is red on main today for unrelated reasons (three unresolved intra-doc links in error.rs). This branch's own five new intra-doc links in reentrancy.rs are fully qualified and resolve cleanly; pr/0-rustdoc-links clears the pre-existing three. The two were deliberately not combined — the error.rs fix should not depend on this PR being accepted.

Checklist

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

[Copilot]

Pull request overview

Adds debug-time callback re-entrancy detection and strengthens test/local-check coverage.

Changes:

• Adds tracked synchronization primitives and callback assertions.
• Ensures feature-gated integration tests run in CI and coverage.
• Aligns local formatting and validation checks.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
crates/hiroz/src/reentrancy.rs	Implements the re-entrancy detector.
crates/hiroz/src/lib.rs	Exports the new module.
crates/hiroz-tests/tests/feature_gate.rs	Detects featureless test runs.
scripts/test-pure-rust.nu	Runs integration tests with required features.
scripts/check-local.sh	Formats all workspace members.
scripts/check-local.nu	Expands local validation checks.
.github/workflows/ci.yml	Enables test features during coverage.

---

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

[Copilot]

Pull request overview

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

Comments suppressed due to low confidence (8)

scripts/check-local.nu:83

• check-hu always builds the WASM plugins, but the default pureRust development shell intentionally omits the wasm32-wasip2 sysroot (flake.nix:469-472). Adding it unconditionally here makes the full local suite fail in the repository's default environment. Please either gate/split the WASM portion or provision/require pureRust-wasm before running this check.

        {name: "hu clippy (check-hu)", cmd: "nu scripts/test-pure-rust.nu check-hu"},

crates/hiroz-tests/tests/feature_gate.rs:34

• This failure message names reentrant_service.rs, but that test target does not exist, so users are told that nonexistent coverage was skipped. Describe the actual gated suites unless the missing target is added.

         The suites gated on it — `reentrant_service.rs` among them — have been \
         compiled to empty test binaries and will report `0 passed`, which reads \
         as green. That is not a pass; it is no coverage.\n\

crates/hiroz/src/reentrancy.rs:223

• This replacement guard loses the standard library guard's must_use diagnostic, allowing an acquired read lock to be immediately and silently discarded.

#[derive(Debug)]
pub struct TrackedReadGuard<'a, T> {

crates/hiroz/src/reentrancy.rs:236

• This replacement guard loses the standard library guard's must_use diagnostic, allowing an acquired write lock to be immediately and silently discarded.

#[derive(Debug)]
pub struct TrackedWriteGuard<'a, T> {

scripts/check-local.nu:82

• No workflow invokes check-local.sh; CI directly invokes selected test-pure-rust.nu subcommands, and the Rustdoc-link check is not present in the workflows. Please avoid claiming that remote CI runs the shell script.

This issue also appears on line 83 of the same file.

        # The four checks below exist in scripts/check-local.sh (which remote CI
        # runs) but were absent here, so they could only ever fail remotely.

crates/hiroz-tests/tests/feature_gate.rs:5

• reentrant_service.rs does not exist in this repository (the only references are the new comments/messages), while test_parameter_validation_callback is ungated at parameter_tests.rs:61. Consequently this gate does not protect the callback-under-lock test coverage claimed here and in the PR description. Please add the referenced test target in this PR or base the gate and rationale on coverage that actually exists.

This issue also appears on line 32 of the same file.

//! Several suites in this crate are `#![cfg(feature = "ros-msgs")]` — most
//! importantly `reentrant_service.rs`, which is the only coverage the
//! callback-under-lock deadlocks in the service and parameter paths have. A

scripts/test-pure-rust.nu:25

• The named reentrant_service.rs test target is absent from the repository, so this comment misstates which tests are enabled by the second command.

    # Several of its suites — `reentrant_service.rs` most importantly — are
    # `#![cfg(feature = "ros-msgs")]`, and a crate-level cfg that is not satisfied
    # compiles to an empty test binary reporting `0 passed`. That reads as green.

crates/hiroz/src/reentrancy.rs:168

• Unlike std::sync::MutexGuard, this public replacement is not must_use, so tracked.lock().unwrap(); silently drops the lock immediately. Preserve the standard guard diagnostic to prevent accidental no-op locking.

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

• line 222
• line 235

#[derive(Debug)]
pub struct TrackedMutexGuard<'a, T> {