diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 9e1710c2..0d74a8f2 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -276,9 +276,13 @@ jobs: - name: Run coverage run: | source /tmp/nix-dev-env.sh + # hiroz-tests needs its own features: several suites are + # `#![cfg(feature = "ros-msgs")]` and compile to empty binaries without + # them, 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..c1b0188a 100644 --- a/crates/hiroz-tests/build.rs +++ b/crates/hiroz-tests/build.rs @@ -1,6 +1,34 @@ use std::{env, path::PathBuf}; fn main() { + // Package-wide enforcement of the feature requirement. + // + // `tests/feature_gate.rs` fails loudly when the crate is built without + // `ros-msgs`, but only if Cargo happens to select *that* target. Ask for one + // gated suite on its own -- `cargo test -p hiroz-tests --test cache` with no + // features -- and Cargo builds only that target, the crate-level `cfg` + // compiles it to an empty binary, `0 passed` is reported, and the guard + // never runs. The silent-skip mode the guard exists to close is still + // reachable through the narrower invocation. + // + // A build script runs for every build of this package regardless of which + // targets are selected, so this is the only place the requirement can be + // stated once and hold everywhere. + // + // Consequence, already declared in `feature_gate.rs`: `hiroz-tests` has no + // supported featureless configuration. + 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..c3d7c2c3 --- /dev/null +++ b/crates/hiroz-tests/tests/feature_gate.rs @@ -0,0 +1,58 @@ +//! Fails loudly when `hiroz-tests` is built without the features its suites need. +//! +//! Ten suites in this crate are `#![cfg(feature = "ros-msgs")]` — `cache.rs`, +//! `lifecycle.rs`, `parameter_tests.rs`, `service_schema_discovery.rs`, +//! `subscriber_timeout.rs`, `type_description_integration.rs`, the `z_*_example` +//! suites, and others. A crate-level `cfg` that is not satisfied does not error +//! and does not warn: the file compiles to an empty test binary reporting +//! `0 passed`, which is indistinguishable from green in every runner and log. +//! +//! Two separate mechanisms could hide that, and they are worth keeping distinct: +//! +//! * **Selection.** `Cargo.toml` sets +//! `default-members = ["crates/hiroz", "crates/hiroz-codegen"]`, so a bare +//! `cargo nextest run` never selected `hiroz-tests` at all — the crate was not +//! built, rather than built empty. `scripts/test-pure-rust.nu` now names the +//! crate explicitly. +//! * **Empty compilation.** Ask for the crate without features and the gated +//! suites really do compile away to `0 passed`. That is what this file and the +//! build script guard against. +//! +//! This file is deliberately **not** gated, so it is the one thing in the crate +//! that a featureless build cannot compile away. If the features go missing +//! again, the run fails with a message naming the cause instead of quietly +//! shrinking. +//! +//! Consequence, stated so it is a decision rather than a surprise: `hiroz-tests` +//! has no supported featureless configuration. Run it as +//! `cargo test -p hiroz-tests --features ros-msgs,jazzy` (or with `ros-interop` +//! for the suites that also need a ROS installation). + +/// 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`, `lifecycle.rs`, \ + `parameter_tests.rs` 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..51660446 100644 --- a/crates/hiroz/src/lib.rs +++ b/crates/hiroz/src/lib.rs @@ -79,6 +79,9 @@ 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..0a7084e5 --- /dev/null +++ b/crates/hiroz/src/reentrancy.rs @@ -0,0 +1,313 @@ +//! Debug-time enforcement of hiroz's one hard rule about user callbacks: +//! +//! > **A user callback is never invoked while a hiroz lock guard is live.** +//! +//! Every re-entrancy deadlock hiroz has had was one violation of that sentence. +//! A callback invoked under a guard is user code running inside hiroz's own +//! critical section, and the first thing such code usually does is call back +//! into hiroz — re-acquiring a non-reentrant `Mutex`/`RwLock` on the thread that +//! already holds it. There is no race to lose; it is a deterministic hang. +//! +//! The rule was previously enforced by review. That does not scale, and it +//! demonstrably leaked: a mechanical sweep of this workspace found further +//! instances in `rmw-zenoh-rs` after five had already been fixed by hand. +//! +//! # Why a runtime tripwire and not a lint +//! +//! The obvious candidate, `clippy::significant_drop_in_scrutinee`, was tried and +//! **does not fire on this code at all**. It targets guards that are unnamed +//! temporaries in a `match`/`if let` scrutinee; hiroz's (and rmw's) shape is +//! `if let Ok(cb) = holder.lock()`, where the guard is *bound* to a name and so +//! is not a scrutinee temporary. Verified: `cargo clippy -p rmw-zenoh-rs -W +//! clippy::significant_drop_in_scrutinee` reports zero hits on code containing +//! five genuine instances of the defect. +//! +//! A bespoke lint fares no better. The defect is a *dynamic* guard lifetime +//! spanning a *dynamic* dispatch through `Arc` or an `extern "C" fn` +//! pointer. Static analysis cannot see through either, and the FFI pointers are +//! opaque by construction. +//! +//! A counter can see both, trivially. +//! +//! # Cost +//! +//! Zero in release. [`GuardCount`](crate::reentrancy::GuardCount) is a +//! zero-sized newtype whose constructor and `Drop` compile to nothing without +//! `debug_assertions`, and +//! [`assert_no_guards_held`](crate::reentrancy::assert_no_guards_held) +//! expands to nothing. Tests and CI run in debug, which is where the assertion +//! is wanted. +//! +//! # Using it +//! +//! Lock declarations on a user-callback-reachable path use +//! [`TrackedMutex`](crate::reentrancy::TrackedMutex) / +//! [`TrackedRwLock`](crate::reentrancy::TrackedRwLock) instead of the +//! `std::sync` originals. Every site that invokes user code calls +//! [`assert_no_guards_held`](crate::reentrancy::assert_no_guards_held) first — in practice via +//! [`invoke_user_callback!`], which does both in one line and names the site in +//! the panic message. + +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 is what makes the counter trustworthy. As a fieldless unit +/// struct this was constructible — and therefore *droppable* — by any code that +/// could name it, and `Drop` decrements the thread-local. A stray +/// `drop(GuardCount)` while a tracked guard was live would take the count to +/// zero, `assert_no_guards_held` would pass, and a genuine callback-under-lock +/// would go unreported. `saturating_sub` guaranteed that desync was silent. +/// Only this module can mint one now, so the count can only be moved by +/// acquiring and releasing a real guard. +#[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| n.set(n.get().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 builds only) if any tracked hiroz guard is live on this thread. +/// +/// Call immediately before invoking user code. `site` names the call site and is +/// reproduced in the panic message, because the useful information when this +/// fires is *which* callback was about to run, not the backtrace of the counter. +#[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} hiroz lock guard(s) still live on this thread. \ + A callback that re-enters hiroz here may deadlock, and will if it \ + touches a lock this thread already holds. The fix is always the \ + same shape: 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"); + } + + /// The detector must actually detect. Without this, a tripwire that never + /// fires is indistinguishable from a codebase with no defects. + #[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"); + // In release the assertion is compiled out, so the test must not be + // expected to panic there. + #[cfg(not(debug_assertions))] + assert_eq!(live_guards(), 0); + } + + #[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..d4b72d1b 100755 --- a/scripts/check-local.sh +++ b/scripts/check-local.sh @@ -35,7 +35,9 @@ run_check() { } # 1. Check formatting -run_check "Formatting (cargo fmt)" "cargo fmt --check" +# --all is required: without it cargo fmt only covers the default workspace +# members, silently skipping hiroz-py and rmw-zenoh-rs. +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 6e85db85..62998bcc 100755 --- a/scripts/test-pure-rust.nu +++ b/scripts/test-pure-rust.nu @@ -19,7 +19,31 @@ def run-tests [] { $env.RUSTFLAGS = "-D warnings" log-step "Run tests" - run-cmd "cargo nextest run --no-fail-fast" + # `hiroz-tests` is excluded here and run separately below *with* its features. + # Ten of its suites (`cache.rs`, `lifecycle.rs`, `parameter_tests.rs`, + # `service_schema_discovery.rs`, `subscriber_timeout.rs` and others) 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. + # Separately, the crate is not in `default-members`, so before this change a + # bare `cargo nextest run` did not build it at all -- naming it explicitly is + # what makes the features load-bearing rather than moot. + # None of those features need a ROS installation (hiroz-msgs bundles the + # definitions), so there is no reason for this job to run the crate blind. + # `rmw-zenoh-rs` is excluded too: its build script generates bindings from + # ROS C headers (`rcutils/strdup.h`), which this job by definition does not + # have. It is not in `default-members`, so it was never built here before + # `--workspace` was introduced -- excluding it restores that, rather than + # dropping coverage. The ROS jobs build and lint it via `-F rmw`. + # `shm_size_estimation` is skipped, not excluded for convenience: it asks the + # OS for a POSIX shared-memory segment large enough to hold a PointCloud2, + # and a GitHub runner's `/dev/shm` cannot provide it -- the failure is + # `OS error 12` (ENOMEM) from `zenoh-shm`, before any hiroz code runs. It is + # a property of the machine, not of the change under test. SHM is covered by + # the dedicated `test-shm` step, which uses segments a runner can allocate. + # `hiroz-msgs` is not in `default-members`, so these never ran in this job + # until `--workspace` was introduced here. + 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 [] { @@ -80,7 +104,10 @@ 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 -- its build script now + # enforces that -- so this names 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 a0ff8c26..2b1af6ed 100755 --- a/scripts/test-ros.nu +++ b/scripts/test-ros.nu @@ -22,7 +22,13 @@ def clippy-rmw [] { } log-step "Clippy (rmw feature)" - run-cmd "cargo clippy --all-targets --workspace -F rmw -- -D warnings" + # `hiroz-tests` is excluded and linted separately with its features. Under + # plain `--workspace` it is selected with none, and its suites are + # `#![cfg(feature = "ros-msgs")]` — so clippy would lint a set of empty + # files and report success. Its build script now refuses that configuration + # outright rather than let it look clean. + 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 [] {