feat(hiroz): debug-time barrier against callbacks under a lock by YuanYuYuan · Pull Request #255 · ZettaScaleLabs/hiroz

YuanYuYuan · 2026-07-27T20:09:13Z

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

Role in #282

The mechanism, and the keystone. This PR fixes no defect; it lands the detector. It is inert as merged — nothing in production code uses the new types on this branch — and becomes load-bearing as #257, #260 and #262 land, each converting its own lock and routing its own callout. Those three do not compile without it, so it merges first.

Issue

Closes #254 (enhancement).

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

What is demonstrated is that the detector detects, which matters more than usual here — a check that has never printed a failure is unvalidated. assert_fires_while_a_guard_is_live is a should_panic unit test that fires the assertion with a live tracked guard, so the tripwire cannot silently rot into a no-op.

What this PR does

crates/hiroz/src/reentrancy.rs (new). A thread-local count of live guards; an RAII GuardCount that increments on acquire and decrements on drop; TrackedMutex<T> / TrackedRwLock<T> whose guards embed it; assert_no_guards_held(site), which panics naming the call site; and invoke_user_callback!(site, call), which does both in one line. Guard field order is fixed so the inner guard releases before the counter falls — reversed, there is an instant where the count reads zero while the lock is held. All behind debug_assertions; zero cost in release.
A barrier that is not executed is not a barrier. hiroz-tests was linted with interop features but tested with none, and a crate-level #![cfg(feature = "ros-msgs")] that is unsatisfied compiles to an empty test binary reporting 0 passed — indistinguishable from green. Four re-entrancy tests had no coverage in the job that gates every PR. CI now runs hiroz-tests with ros-msgs,jazzy, and build.rs states the requirement package-wide so a narrower --test <name> cannot bypass it.
scripts/check-local.nu realigned with what CI actually runs: four missing checks, and cargo fmt --all — default-members is only hiroz and hiroz-codegen, so without it every other member is skipped, which is how the violation fixed in fix(ci): rustfmt hook silently ran stable, ignoring rustfmt.toml #286 reached main.

A cheaper option was tried and rejected on measurement: clippy::significant_drop_in_scrutinee reports zero hits on hiroz, with the lint confirmed live in the same run against its own documented trigger (a scrutinee-temporary guard fires; the bound-guard shape transcribed from parameter/service.rs:191 does not). The rule keys on guards left as unnamed scrutinee temporaries, and the shape here binds the guard. Nor could a bespoke lint do better: the defect is a dynamic guard lifetime spanning a dispatch through Arc<dyn Fn> or an opaque extern "C" fn.

Breaking changes

None to any published API, and zero cost in release builds.

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

Checklist

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

Rebased onto main after #286; cargo fmt --all --check, the pre-commit gate, the reentrancy suite and cargo doc all pass locally on the rebased branch.

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> {

hiroz has repeatedly shipped the same defect: a user callback invoked while a non-reentrant hiroz lock guard is still live on the calling thread. Any callback that re-enters hiroz there blocks on a lock its own thread already holds. It needs no race and is reachable from ordinary public API. Add `reentrancy`, a debug-only tripwire for the invariant "a user callback is never invoked while a hiroz lock guard is live". `TrackedMutex` and `TrackedRwLock` wrap the `std::sync` types and count live guards in a thread-local; `invoke_user_callback!(site, call)` asserts the count is zero and names the site before dispatching. Field order is fixed so the inner guard releases before the counter decrements. Everything is behind `debug_assertions` and compiles to nothing in release; tests and CI run in debug, which is where the assertion is wanted. A cheaper option was tried first and does not work. `clippy::significant_drop_in_scrutinee` reports zero hits on code known to contain the defect, because the shape is `if let Ok(cb) = holder.lock()`, which *binds* the guard rather than leaving it a scrutinee temporary. The defect is a dynamic guard lifetime spanning a dynamic dispatch through `Arc<dyn Fn>` or an `extern "C" fn` pointer; static analysis sees through neither, a counter sees both. The module carries a unit test that asserts the tripwire panics while a guard is live, so the detector cannot silently rot into a no-op. Two adjacent fixes are needed for this barrier to actually run: - `hiroz-tests` was linted under interop features but tested under none. Several of its suites are `#![cfg(feature = "ros-msgs")]`, and a crate-level cfg that is not satisfied compiles to an empty test binary reporting `0 passed` — indistinguishable from green. `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. `feature_gate.rs` is not gated, so a featureless build cannot compile it away, and it fails with a message naming the cause. - `check-local.nu` (the local gate) was missing four checks that `check-local.sh` (what CI runs) has: check-hu, test-shm, check-distro-features and the rustdoc intra-doc link check, and neither passed `--all` to `cargo fmt`, which skips `hiroz-py` and `rmw-zenoh-rs`. The rustdoc detector was validated by injecting a broken link: rc=0 clean, rc=1 broken, rc=0 restored. The five new intra-doc links in `reentrancy.rs` are fully qualified so they resolve under that gate. The three pre-existing unresolved links in `error.rs` are a separate defect on main and are fixed in their own change, not buried here.

… gate `GuardCount` was a fieldless unit struct, so any code able to name it could construct one -- and therefore drop one. `Drop` decrements the thread-local, so a stray `drop(GuardCount)` while a tracked guard was live took the count to zero, `assert_no_guards_held` then passed, and a genuine callback-under-lock went unreported. `saturating_sub` made that desync permanently silent. A private field means only this module can mint one, so the count moves only by acquiring and releasing a real guard. The module docs called it a unit struct; they now say zero-sized newtype. `check-local.nu`'s rustdoc gate matched only `unresolved link`, while `check-local.sh:95` -- what CI runs -- also matches `broken_intra_doc_links`. A diagnostic carrying the lint name but not that phrase passed locally and failed remotely, which is the specific way this gate has already wasted CI cycles on this branch series. Both patterns now, as the shell gate does. No behaviour change: the four `reentrancy::tests` still pass, including `assert_fires_while_a_guard_is_live`, which is the one that proves the counter still detects.

`tests/feature_gate.rs` only fails when Cargo happens to select that target. `cargo test -p hiroz-tests --test reentrant_service` with no features builds only that target, the crate-level `cfg` compiles it to an empty binary, `0 passed` is reported, and the guard never runs -- so the silent-skip mode it exists to close stayed reachable through the narrower invocation. A build script runs for every build of the package whatever targets are selected, so that is the one place the requirement holds everywhere. Verified both ways: `cargo check -p hiroz-tests --tests` exits 101 with the message, and with `--features ros-msgs,jazzy` exits 0. Note `cargo check -p hiroz-tests` *without* `--tests` proves nothing here -- the crate has no lib target, so cargo builds nothing and never runs the build script at all. That invocation reports success on a crate it did not compile. `scripts/test-pure-rust.nu`'s `shm_example` step named no features; it now does. `shm_example` itself is ungated, but the crate no longer has a supported featureless configuration, which `feature_gate.rs` already declared.

Two things the GitHub pre-commit gate caught that the local runner does not. `scripts/test-ros.nu` ran `cargo clippy --all-targets --workspace -F rmw`, which selects `hiroz-tests` with no features. Its suites are `#![cfg(feature = "ros-msgs")]`, so clippy was linting a set of empty files and reporting success -- the same hollow result the new build-script guard exists to refuse. That guard turned this from a silent no-op into a hard failure, which is the point. Exclude the crate from the workspace pass and lint it separately with `ros-interop,<distro>`, matching how the same script already builds and tests it. Also drops a stray blank line in `crates/hiroz-tests/build.rs` that the pinned stable rustfmt in the pre-commit hook rejects. Worth noting the local `cargo fmt --all --check` does not catch it: CI runs `nix build .#checks...pre-commit-check`, a different and stricter gate.

`run-tests` gained `--workspace --exclude hiroz-tests` so the gated suites would actually run. That also pulled in `rmw-zenoh-rs`, whose build script generates bindings from ROS C headers -- so the ROS-*independent* job now fails with `'rcutils/strdup.h' file not found` before running a single test. `rmw-zenoh-rs` is not in `default-members`, so it was never built by this job before `--workspace` was introduced. Excluding it restores the previous selection rather than dropping coverage; the ROS jobs build and lint it via `-F rmw`. This did not show up on the local runner because its dev shell patches the workspace member list to drop that crate. GitHub's runner does not, which is why the wider matrix caught it.

`--workspace` also pulled in `hiroz-msgs`, whose `shm_size_estimation` suite asks the OS for a POSIX shared-memory segment big enough for a PointCloud2. A GitHub runner's /dev/shm cannot provide one: the failure is `OS error 12` (ENOMEM) raised inside `zenoh-shm` before any hiroz code runs. Three tests fail for a property of the machine, not of the change under test. Skip that binary rather than exclude the whole crate, so the rest of `hiroz-msgs` keeps the coverage `--workspace` was added to gain. SHM itself is still covered by the dedicated `test-shm` step, which allocates segments a runner can serve. Like `rmw-zenoh-rs`, `hiroz-msgs` is not in `default-members`, so none of this ran in this job before `--workspace` was introduced -- these are newly-surfaced, not newly-broken.

…history Review found three false claims shipped in code, not just prose. 1. The panic message told developers to "see GraphEventManager:: trigger_graph_change for the pattern". On this branch that function holds trigger_guard_condition AND graph_guard_conditions across trigger(gc), an opaque extern "C" pointer -- it is a live instance of the defect the tripwire just caught. It only becomes the pattern after pr/4b rewrites it. The pattern is now stated inline instead of cross-referenced. 2. feature_gate.rs, build.rs and test-pure-rust.nu all cited reentrant_service.rs as the most important gated suite, and asserted "that is exactly how those four tests stopped running without anyone noticing". That file does not exist on main or on this branch -- it arrives with pr/3. Those tests never ran, so they cannot have stopped running. Now cites the ten suites that actually are gated here. 3. The "0 passed" story conflated two mechanisms. default-members excludes hiroz-tests, so a bare `cargo nextest run` never built the crate at all -- it was not built empty. The empty-compilation mode is real but reachable only when the crate IS selected without features. Both are now stated, and kept distinct.

saturating_sub prevents the wrap but absorbs the desync silently, which is the exact shape this module exists to detect: once the count is stuck below the number of live guards, assert_no_guards_held starts passing while a guard is held. Asserts non-zero before decrementing, suppressed while unwinding because a panic raised during a panic aborts and would replace the original failure. Adds the test that proves it fires.

The module doc claimed ten crate-level gated suites; there are eight. It also cited lifecycle.rs and parameter_tests.rs as examples, in both the doc and the panic message -- neither carries a crate-level cfg, so they never show the empty-binary symptom the message sends a reader to find. Name only verified crate-level suites, and drop the hardcoded count so it cannot drift again. Fold the two-bullet selection-vs-compilation section into one closing sentence and stop restating what build.rs and the panic text already say.

They had drifted into essay. Cut narrative, history and repetition; keep the mechanism and the non-obvious constraints. Two errors fixed while rewriting: test-pure-rust.nu repeated the wrong suite count and cited lifecycle.rs and parameter_tests.rs as crate-level gated, which they are not. reentrancy.rs claimed a clippy result for rmw-zenoh-rs that was never measured -- it now states the hiroz result, which was, along with the positive control proving the lint was live. Module-doc links are reference-style: reentrancy.rs's inner docs merge with the mod declaration in lib.rs and resolve in the crate root's scope, so bare shortcuts do not resolve.

The comment named hiroz-py and rmw-zenoh-rs as what plain cargo fmt skips. It skips every member outside default-members, and the one that actually mattered was hiroz-tests -- the violation #286 fixed.

YuanYuYuan force-pushed the pr/1-reentrancy-tripwire branch from ad3a4d7 to 9bc6dfd Compare July 28, 2026 06:35

YuanYuYuan requested a review from Copilot July 28, 2026 07:13

Copilot started reviewing on behalf of YuanYuYuan July 28, 2026 07:14 View session

Copilot AI reviewed Jul 28, 2026

View reviewed changes

Comment thread crates/hiroz/src/reentrancy.rs Outdated

Comment thread crates/hiroz/src/reentrancy.rs

Comment thread scripts/check-local.nu Outdated

Comment thread crates/hiroz-tests/tests/feature_gate.rs

YuanYuYuan requested a review from Copilot July 28, 2026 10:29

Copilot started reviewing on behalf of YuanYuYuan July 28, 2026 10:30 View session

Copilot AI reviewed Jul 28, 2026

View reviewed changes

YuanYuYuan force-pushed the pr/1-reentrancy-tripwire branch from 21c96f5 to 69326f0 Compare July 28, 2026 14:23

YuanYuYuan force-pushed the pr/1-reentrancy-tripwire branch from a70f733 to eee8c43 Compare August 4, 2026 11:06

YuanYuYuan added 10 commits August 4, 2026 20:35

YuanYuYuan force-pushed the pr/1-reentrancy-tripwire branch from 2db0ccd to cf4911a Compare August 4, 2026 12:36

docs: correct the cargo fmt --all rationale, drop a stray blank line

54d5072

The comment named hiroz-py and rmw-zenoh-rs as what plain cargo fmt skips. It skips every member outside default-members, and the one that actually mattered was hiroz-tests -- the violation #286 fixed.

YuanYuYuan merged commit 27128d0 into main Aug 4, 2026
27 checks passed

YuanYuYuan deleted the pr/1-reentrancy-tripwire branch August 4, 2026 18:43

YuanYuYuan restored the pr/1-reentrancy-tripwire branch August 4, 2026 18:52

YuanYuYuan deleted the pr/1-reentrancy-tripwire branch August 4, 2026 18:53

YuanYuYuan mentioned this pull request Aug 5, 2026

Event callback can outlive rmw_event_set_callback(.., null) and use freed user_data #287

Open

6 tasks

Navigation Menu

Uh oh!

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

feat(hiroz): debug-time barrier against callbacks under a lock#255
YuanYuYuan merged 11 commits into
mainfrom
pr/1-reentrancy-tripwire

YuanYuYuan commented Jul 27, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

YuanYuYuan commented Jul 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Role in #282

Issue

What this PR does

Breaking changes

Checklist

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

YuanYuYuan commented Jul 27, 2026 •

edited

Loading