diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index cb624193..ec9e9711 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -297,9 +297,12 @@ jobs: - name: Run coverage run: | source /tmp/nix-dev-env.sh + # Without its features hiroz-tests compiles to empty binaries, so + # coverage taken blind under-reports the paths they exercise. cargo llvm-cov \ -p hiroz -p hiroz-codegen -p hiroz-cdr -p hiroz-protocol -p hiroz-schema \ -p hiroz-tests \ + --features hiroz-tests/ros-msgs,hiroz-tests/jazzy \ -j4 \ --lcov --output-path lcov.info shell: bash diff --git a/crates/hiroz-tests/build.rs b/crates/hiroz-tests/build.rs index 159e0b9f..229fda4c 100644 --- a/crates/hiroz-tests/build.rs +++ b/crates/hiroz-tests/build.rs @@ -1,6 +1,26 @@ use std::{env, path::PathBuf}; fn main() { + // Package-wide enforcement of the feature requirement. + // + // `tests/feature_gate.rs` only fires if Cargo selects that target. + // `cargo test -p hiroz-tests --test cache` with no features builds only + // `cache`, which the crate-level `cfg` compiles to an empty binary -- + // `0 passed`, guard never run. A build script runs for every build of the + // package regardless of target selection, so this is the one place the + // requirement holds everywhere. + if std::env::var_os("CARGO_FEATURE_ROS_MSGS").is_none() { + panic!( + "\n\nhiroz-tests requires the `ros-msgs` feature.\n\n\ + Without it the suites gated on it compile to empty test binaries \n\ + that report `0 passed`, which reads as green but is no coverage.\n\n\ + Build it as:\n\n \ + cargo test -p hiroz-tests --features ros-msgs,jazzy\n\n\ + Suites that drive a real ROS 2 installation need \n \ + --features ros-interop, instead.\n" + ); + } + // Declare custom cfg for ROS version detection println!("cargo::rustc-check-cfg=cfg(ros_humble)"); diff --git a/crates/hiroz-tests/tests/feature_gate.rs b/crates/hiroz-tests/tests/feature_gate.rs new file mode 100644 index 00000000..753ec1c8 --- /dev/null +++ b/crates/hiroz-tests/tests/feature_gate.rs @@ -0,0 +1,46 @@ +//! Fails loudly when `hiroz-tests` is built without the features its suites need. +//! +//! Several suites carry a crate-level `#![cfg(feature = "ros-msgs")]`. An +//! unsatisfied crate-level `cfg` neither errors nor warns — the file compiles to +//! an empty test binary reporting `0 passed`, indistinguishable from green. +//! +//! This file is deliberately ungated, so a featureless build cannot compile it +//! away. `build.rs` covers the narrower `--test ` invocations that never +//! select this target. +//! +//! `hiroz-tests` therefore has no supported featureless configuration: run it as +//! `cargo test -p hiroz-tests --features ros-msgs,jazzy`, or with +//! `ros-interop,` for suites that drive a real ROS installation. +//! +//! Selection is a separate failure mode from compilation: this crate is not in +//! `default-members`, so a bare `cargo nextest run` skips it rather than +//! building it empty. `scripts/test-pure-rust.nu` names it explicitly. + +/// Without `ros-msgs`, the gated suites are silently absent — fail instead. +#[test] +#[cfg(not(feature = "ros-msgs"))] +fn ros_msgs_gated_suites_must_not_be_silently_skipped() { + panic!( + "hiroz-tests was built without the `ros-msgs` feature.\n\ + \n\ + The suites gated on it — `cache.rs`, `subscriber_timeout.rs`, \ + `service_schema_discovery.rs`, the `z_*_example` suites and others — \ + 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\ + \n\ + Build the crate with its features:\n\ + \n\ + cargo test -p hiroz-tests --features ros-msgs,jazzy\n\ + \n\ + Suites that additionally drive a real ROS 2 installation need \ + `--features ros-interop,` instead." + ); +} + +/// With `ros-msgs`, record that the gate was satisfied. +/// +/// Present so the guard is visible in the test list in *both* configurations — +/// a check whose only evidence is the absence of a failure is not a check. +#[test] +#[cfg(feature = "ros-msgs")] +fn ros_msgs_gated_suites_are_compiled_in() {} diff --git a/crates/hiroz/src/lib.rs b/crates/hiroz/src/lib.rs index 71d67191..7cf63104 100644 --- a/crates/hiroz/src/lib.rs +++ b/crates/hiroz/src/lib.rs @@ -79,6 +79,8 @@ pub mod python_bridge; pub mod qos; /// Internal message queues. pub mod queue; +/// Debug-time enforcement of "no user callback under a hiroz lock guard". +pub mod reentrancy; /// Message type metadata traits (`WithTypeInfo`, etc.). pub mod ros_msg; /// ROS 2 service client and server. diff --git a/crates/hiroz/src/reentrancy.rs b/crates/hiroz/src/reentrancy.rs new file mode 100644 index 00000000..0127262a --- /dev/null +++ b/crates/hiroz/src/reentrancy.rs @@ -0,0 +1,307 @@ +//! Debug-time enforcement of: **a user callback is never invoked while a hiroz +//! lock guard is live.** +//! +//! A callback invoked under a guard runs user code inside hiroz's critical +//! section. If it re-enters hiroz it re-acquires a non-reentrant lock on the +//! thread already holding it — a deterministic hang, not a race. +//! +//! No lint catches this. `clippy::significant_drop_in_scrutinee` targets guards +//! that are unnamed scrutinee temporaries; the shape here is +//! `if let Ok(cb) = holder.lock()`, which *binds* the guard. Measured on this +//! crate: zero hits, with the lint confirmed live against its own documented +//! trigger. Nor could a bespoke one do better — the guard lifetime spans a +//! dynamic dispatch through `Arc` or an opaque `extern "C" fn`. +//! +//! Zero cost in release: [`GuardCount`] is zero-sized, and it and +//! [`assert_no_guards_held`] compile to nothing without `debug_assertions`. +//! Tests and CI run in debug. +//! +//! Usage: declare locks on callback-reachable paths as [`TrackedMutex`] / +//! [`TrackedRwLock`], and route every user-code invocation through +//! [`invoke_user_callback!`]. +//! +//! [`GuardCount`]: crate::reentrancy::GuardCount +//! [`assert_no_guards_held`]: crate::reentrancy::assert_no_guards_held +//! [`TrackedMutex`]: crate::reentrancy::TrackedMutex +//! [`TrackedRwLock`]: crate::reentrancy::TrackedRwLock + +use std::sync::{LockResult, Mutex, MutexGuard, RwLock, RwLockReadGuard, RwLockWriteGuard}; + +#[cfg(debug_assertions)] +thread_local! { + /// How many tracked hiroz guards are live on this thread right now. + static LIVE_GUARDS: std::cell::Cell = const { std::cell::Cell::new(0) }; +} + +/// RAII counter embedded in every tracked guard. +/// +/// The private field makes it unforgeable: as a fieldless unit struct, any code +/// naming it could `drop` one, decrementing the count to zero while a guard was +/// live and silently disarming [`assert_no_guards_held`]. +/// +/// `Drop` asserts non-zero before decrementing. `saturating_sub` alone prevents +/// the wrap but hides the desync, which is the failure mode this module exists +/// to remove. +#[derive(Debug)] +pub struct GuardCount(()); + +impl GuardCount { + #[inline(always)] + fn new() -> Self { + #[cfg(debug_assertions)] + LIVE_GUARDS.with(|n| n.set(n.get() + 1)); + Self(()) + } +} + +impl Drop for GuardCount { + #[inline(always)] + fn drop(&mut self) { + #[cfg(debug_assertions)] + LIVE_GUARDS.with(|n| { + let live = n.get(); + // `|| panicking()`: panicking while unwinding aborts, which would + // replace someone else's failure with this one. + debug_assert!( + live > 0 || std::thread::panicking(), + "hiroz GuardCount underflow: guard released with the live count \ + already 0. The counter has desynced from the guards it tracks, \ + so `assert_no_guards_held` can no longer detect a callback \ + invoked under a lock." + ); + n.set(live.saturating_sub(1)); + }); + } +} + +/// Number of tracked hiroz guards live on this thread. Always 0 in release. +#[inline(always)] +pub fn live_guards() -> usize { + #[cfg(debug_assertions)] + { + LIVE_GUARDS.with(|n| n.get()) + } + #[cfg(not(debug_assertions))] + { + 0 + } +} + +/// Panics (debug only) if any tracked guard is live on this thread. +/// +/// Call immediately before invoking user code. `site` is reproduced in the panic +/// message — when this fires, *which* callback was about to run is the useful +/// information, not the counter's backtrace. +#[inline(always)] +pub fn assert_no_guards_held(site: &str) { + #[cfg(debug_assertions)] + { + let live = live_guards(); + assert!( + live == 0, + "hiroz re-entrancy rule violated at `{site}`: about to invoke a user \ + callback with {live} lock guard(s) live on this thread. A callback \ + that re-enters hiroz will deadlock if it touches a lock this thread \ + holds. Fix: collect what you need into an owned value, drop every \ + guard, then invoke the callback." + ); + } + #[cfg(not(debug_assertions))] + let _ = site; +} + +/// Assert the re-entrancy rule, then invoke user code. +/// +/// ```ignore +/// invoke_user_callback!("EventsManager::set_callback backlog", callback(count)); +/// ``` +#[macro_export] +macro_rules! invoke_user_callback { + ($site:expr, $call:expr) => {{ + $crate::reentrancy::assert_no_guards_held($site); + $call + }}; +} + +// --------------------------------------------------------------------------- +// Tracked lock types +// --------------------------------------------------------------------------- + +/// A `std::sync::Mutex` whose guards are counted by [`live_guards`]. +#[derive(Debug, Default)] +pub struct TrackedMutex(Mutex); + +impl TrackedMutex { + pub fn new(value: T) -> Self { + Self(Mutex::new(value)) + } + + pub fn lock(&self) -> LockResult> { + match self.0.lock() { + Ok(inner) => Ok(TrackedMutexGuard { + inner, + _count: GuardCount::new(), + }), + Err(poisoned) => Err(std::sync::PoisonError::new(TrackedMutexGuard { + inner: poisoned.into_inner(), + _count: GuardCount::new(), + })), + } + } +} + +/// Guard for [`TrackedMutex`]. Field order matters: `inner` is declared first so +/// it is released before the counter decrements, never the other way round. +#[derive(Debug)] +pub struct TrackedMutexGuard<'a, T> { + inner: MutexGuard<'a, T>, + _count: GuardCount, +} + +impl std::ops::Deref for TrackedMutexGuard<'_, T> { + type Target = T; + fn deref(&self) -> &T { + &self.inner + } +} + +impl std::ops::DerefMut for TrackedMutexGuard<'_, T> { + fn deref_mut(&mut self) -> &mut T { + &mut self.inner + } +} + +/// A `std::sync::RwLock` whose guards are counted by [`live_guards`]. +#[derive(Debug, Default)] +pub struct TrackedRwLock(RwLock); + +impl TrackedRwLock { + pub fn new(value: T) -> Self { + Self(RwLock::new(value)) + } + + pub fn read(&self) -> LockResult> { + match self.0.read() { + Ok(inner) => Ok(TrackedReadGuard { + inner, + _count: GuardCount::new(), + }), + Err(p) => Err(std::sync::PoisonError::new(TrackedReadGuard { + inner: p.into_inner(), + _count: GuardCount::new(), + })), + } + } + + pub fn write(&self) -> LockResult> { + match self.0.write() { + Ok(inner) => Ok(TrackedWriteGuard { + inner, + _count: GuardCount::new(), + }), + Err(p) => Err(std::sync::PoisonError::new(TrackedWriteGuard { + inner: p.into_inner(), + _count: GuardCount::new(), + })), + } + } +} + +#[derive(Debug)] +pub struct TrackedReadGuard<'a, T> { + inner: RwLockReadGuard<'a, T>, + _count: GuardCount, +} + +impl std::ops::Deref for TrackedReadGuard<'_, T> { + type Target = T; + fn deref(&self) -> &T { + &self.inner + } +} + +#[derive(Debug)] +pub struct TrackedWriteGuard<'a, T> { + inner: RwLockWriteGuard<'a, T>, + _count: GuardCount, +} + +impl std::ops::Deref for TrackedWriteGuard<'_, T> { + type Target = T; + fn deref(&self) -> &T { + &self.inner + } +} + +impl std::ops::DerefMut for TrackedWriteGuard<'_, T> { + fn deref_mut(&mut self) -> &mut T { + &mut self.inner + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn guards_are_counted_and_released() { + let m = TrackedMutex::new(1u32); + assert_eq!(live_guards(), 0); + { + let g = m.lock().unwrap(); + assert_eq!(*g, 1); + assert_eq!(live_guards(), 1); + { + let rw = TrackedRwLock::new(2u32); + let _r = rw.read().unwrap(); + assert_eq!(live_guards(), 2); + } + assert_eq!(live_guards(), 1); + } + assert_eq!(live_guards(), 0); + } + + #[test] + fn assert_passes_with_no_guards() { + assert_no_guards_held("test"); + } + + /// A tripwire that never fires is indistinguishable from a clean codebase. + #[test] + #[cfg_attr(debug_assertions, should_panic(expected = "re-entrancy rule violated"))] + fn assert_fires_while_a_guard_is_live() { + let m = TrackedMutex::new(0u32); + let _g = m.lock().unwrap(); + assert_no_guards_held("deliberate violation"); + // Compiled out in release, so no panic is expected there. + #[cfg(not(debug_assertions))] + assert_eq!(live_guards(), 0); + } + + /// The underflow assertion needs the same proof the tripwire gets. Only this + /// module can mint a bare `GuardCount`, so only here can it be tested. + #[test] + #[cfg_attr(debug_assertions, should_panic(expected = "GuardCount underflow"))] + fn underflow_is_not_silent() { + assert_eq!(live_guards(), 0); + drop(GuardCount(())); + } + + #[test] + fn a_poisoned_guard_is_still_counted() { + let m = std::sync::Arc::new(TrackedMutex::new(0u32)); + let m2 = m.clone(); + let _ = std::thread::spawn(move || { + let _g = m2.lock().unwrap(); + panic!("poison it"); + }) + .join(); + + let guard = m.lock(); + assert!(guard.is_err(), "expected the mutex to be poisoned"); + let recovered = guard.unwrap_or_else(|e| e.into_inner()); + assert_eq!(live_guards(), 1, "a recovered poisoned guard must count"); + drop(recovered); + assert_eq!(live_guards(), 0); + } +} diff --git a/scripts/check-local.nu b/scripts/check-local.nu index 7aa68984..1d80e0da 100755 --- a/scripts/check-local.nu +++ b/scripts/check-local.nu @@ -56,7 +56,7 @@ def main [--suite: string = "full"] { if $suite == "lint" { log-header "Running hiroz Lint Checks" let checks = [ - {name: "Formatting (cargo fmt)", cmd: "cargo fmt --check"}, + {name: "Formatting (cargo fmt)", cmd: "cargo fmt --all --check"}, {name: "Clippy (all targets)", cmd: "cargo clippy --all-targets -- -D warnings"}, ] let results = $checks | enumerate | each {|item| @@ -70,7 +70,7 @@ def main [--suite: string = "full"] { log-header "Running hiroz Pre-Submission Checks" let checks = [ - {name: "Formatting (cargo fmt)", cmd: "cargo fmt --check"}, + {name: "Formatting (cargo fmt)", cmd: "cargo fmt --all --check"}, {name: "Clippy (all targets)", cmd: "cargo clippy --all-targets -- -D warnings"}, {name: "Build (cargo build)", cmd: "cargo build --examples"}, {name: "Tests", cmd: (if (which cargo-nextest | is-not-empty) { @@ -78,6 +78,12 @@ def main [--suite: string = "full"] { } else { "cargo test --lib --tests" })}, + # 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. + {name: "hu clippy (check-hu)", cmd: "nu scripts/test-pure-rust.nu check-hu"}, + {name: "SHM tests (test-shm)", cmd: "nu scripts/test-pure-rust.nu test-shm"}, + {name: "Distro feature flags (check-distro-features)", cmd: "nu scripts/test-pure-rust.nu check-distro-features"}, + {name: "Rustdoc links (cargo doc)", cmd: "let r = (^cargo doc --no-deps -p hiroz --quiet | complete); let w = ($r.stderr | lines | where {|it| ($it =~ 'unresolved link') or ($it =~ 'broken_intra_doc_links')}); if ($w | is-not-empty) { print ($w | str join (char newline)); error make {msg: 'rustdoc: unresolved intra-doc links'} }"}, ] let results = $checks | enumerate | each {|item| diff --git a/scripts/check-local.sh b/scripts/check-local.sh index a580f1b0..4fe4481d 100755 --- a/scripts/check-local.sh +++ b/scripts/check-local.sh @@ -35,7 +35,10 @@ run_check() { } # 1. Check formatting -run_check "Formatting (cargo fmt)" "cargo fmt --check" +# --all is required: default-members is just hiroz and hiroz-codegen, so without +# it every other member is skipped -- which is how the hiroz-tests violation +# fixed in #286 reached main. +run_check "Formatting (cargo fmt)" "cargo fmt --all --check" # 2. Clippy lints run_check "Clippy (all targets)" "cargo clippy --all-targets -- -D warnings" diff --git a/scripts/test-pure-rust.nu b/scripts/test-pure-rust.nu index 8f069c39..3f710ab8 100755 --- a/scripts/test-pure-rust.nu +++ b/scripts/test-pure-rust.nu @@ -19,7 +19,21 @@ def run-tests [] { $env.RUSTFLAGS = "-D warnings" log-step "Run tests" - run-cmd "cargo nextest run --no-fail-fast" + # Three exclusions, each run or covered elsewhere: + # + # - `hiroz-tests`: run separately below with `ros-msgs,jazzy`. Its gated + # suites are `#![cfg(feature = "ros-msgs")]` and compile to empty binaries + # reporting `0 passed` without them. No ROS install needed -- hiroz-msgs + # bundles the definitions. + # - `rmw-zenoh-rs`: its build script generates bindings from ROS C headers, + # which this job does not have. The ROS jobs lint it via `-F rmw`. + # - `shm_size_estimation`: needs a `/dev/shm` segment sized for a + # PointCloud2; a runner cannot allocate it and `zenoh-shm` fails with + # ENOMEM before any hiroz code runs. Covered by the `test-shm` step. + # + # None were in `default-members`, so none ran here before `--workspace`. + run-cmd "cargo nextest run --no-fail-fast --workspace --exclude hiroz-tests --exclude rmw-zenoh-rs -E 'not binary(shm_size_estimation)'" + run-cmd "cargo nextest run --no-fail-fast -p hiroz-tests --features ros-msgs,jazzy" } def check-bundled-msgs [] { @@ -135,7 +149,9 @@ def test-shm [] { # Integration-style unit tests (pub/sub with SHM) run-cmd "cargo test --package hiroz --test shm" # Integration tests (validate shm_pointcloud2 example) - run-cmd "cargo test --package hiroz-tests --test shm_example" + # `hiroz-tests` has no featureless configuration (enforced by its build + # script), so name the features even though shm_example itself is ungated. + run-cmd "cargo test --package hiroz-tests --test shm_example --features ros-msgs,jazzy" } # ============================================================================ diff --git a/scripts/test-ros.nu b/scripts/test-ros.nu index dc0e8484..f1ad2bc5 100755 --- a/scripts/test-ros.nu +++ b/scripts/test-ros.nu @@ -22,7 +22,11 @@ def clippy-rmw [] { } log-step "Clippy (rmw feature)" - run-cmd "cargo clippy --all-targets --workspace -F rmw -- -D warnings" + # `hiroz-tests` is linted separately with its features: under plain + # `--workspace` it is selected with none, so clippy would lint a set of + # empty files and report success. + run-cmd "cargo clippy --all-targets --workspace --exclude hiroz-tests -F rmw -- -D warnings" + run-cmd $"cargo clippy --all-targets -p hiroz-tests --features ros-interop,($distro) -- -D warnings" } def run-ros-interop [] {