Re-entrancy: close the coverage gap so an unconverted lock cannot bypass the guard

[YuanYuYuan]

Part of #282. This is what closes that issue — merging the six fixes does not.

The gap

The debug-time monitor added in #255 counts guards only from converted lock types (TrackedMutex / TrackedRwLock). A guard from a plain std::sync::Mutex is invisible to it. So the detector's reach is exactly the set of locks somebody converted, and nothing reports the locks nobody did.

A file matters here if it is callback-reachable: it both (a) acquires a lock and (b) calls through a dyn Fn, an Arc<dyn Fn>, or an extern "C" fn pointer. On that criterion — measured 2026-07-31, re-derive before acting — there are 11 such files holding 146 acquisitions. The six PRs in #282 cover six of them. Five are untouched:

⚠️ "Covered by" below means the deadlock at that site is fixed, not that file's locks are tracked. Two corrections, measured 2026-08-05:

1. Nothing is tracked on main today. #255 merged inert — outside the monitor's own module there are zero TrackedMutex/TrackedRwLock fields in the whole crate. Every "covered" claim below is a statement about a PR that has not landed.

2. Coverage is per-field, not per-file, even on the branches. parameter/service.rs has two lock fields and #257 converts one (on_set_callback; store stays a plain RwLock). event.rs has four and #260 converts three. So the tripwire cannot see the unconverted fields, and a future change that moved a callout under one of them would fire nothing.

The consequence for this issue is that its scope is larger than the "five untouched files" framing suggests: closing the gap means auditing the converted files too, not just the ones the family never reached. The acquisition counts below are per-file totals and should not be read as "all of these are handled once the family merges."

acquisitions	file
12	crates/hiroz/src/ffi/action.rs
6	crates/hiroz/src/dynamic/type_description_service.rs
5	crates/hiroz/src/ffi/service.rs
3	crates/hiroz/src/action/server.rs
1	crates/hiroz/src/service.rs

This is not a list of five more bugs. Spot-checked, ffi/action.rs is currently correct — it scopes each acquisition in an explicit block so the guard drops before goal_callback and execute_callback run:

{
    let mut server = server_mutex_clone.lock().unwrap();
    server.send_goal_server.map.insert(key.clone(), query);
}                                     // ← guard dropped here

let accepted = goal_callback(user_data, goal_data.as_ptr(), goal_data.len());

That is exactly the problem. Correctness there rests on someone having remembered the braces, with no mechanism that fails when the braces are removed — which is the state main was in before the six defects in #282 were found by hand.

Deliverables

• Re-derive the callback-reachable set, then for each file identify which acquisitions are actually on a callout-reaching path. Not all 27 are; converting the rest inflates the diff without changing the property. Re-check queue.rs and cache.rs specifically — neither matched the criterion, but queue.rs is the subscriber-dispatch surface fix(pubsub): stop running subscriber callbacks on the publishing thread #250 exists to fix, so if the criterion excludes it, the criterion is wrong rather than the file being safe.
• Convert those acquisitions to tracked lock types and route every callout through invoke_user_callback!.
• Extend the tracked-lock API as needed — try_lock, try_read, try_write, get_mut, into_inner, is_poisoned are all missing today and the inner lock is private, so a site needing one cannot convert. Add them as required, each with the same counter discipline and guard field ordering; not speculatively.
• A CI gate that recomputes the criterion and fails when a file enters the callback-reachable set uninstrumented.
• An automated falsification check (below).
• Write the coverage boundary down, naming what is excluded.

Why a lint is the wrong gate

The obvious mechanism is a clippy.toml disallowed-types entry banning std::sync::Mutex/RwLock. It is the wrong check, for a reason that is the whole point of this issue.

disallowed-types catches someone reaching for a banned type in a module already known to be callback-reachable. It cannot catch the regression that actually happens: a file becoming callback-reachable — someone adds an Arc<dyn Fn> call to a file that already held plain locks. Compliant yesterday, a hazard today, and no type-based lint notices, because no banned type was introduced. It also costs a module-level #[allow] at roughly 180 legitimately-untracked acquisitions, since the lint is workspace-wide.

So make the criterion itself executable and run that:

for each production source file (excluding the monitor itself):
  reachable := acquires_a_lock(f) AND calls_through(dyn Fn | Arc<dyn Fn> | extern "C" fn ptr)
  if reachable and f has an untracked acquisition                -> FAIL
  if reachable and f has a callout not via invoke_user_callback! -> FAIL

Syntactic, over the file list — the criterion is already syntactic and a dyn Fn call is greppable. If it proves too coarse, tighten the criterion, not the mechanism.

Why the falsification must be automated

"Removing the scoping reintroduces a failing test" is the strongest acceptance criterion available here, and the one most likely to rot: it is eleven files of manual mutation, demonstrated once, in a review nobody repeats. Demonstrated-once decays to assumed-forever.

Make it a harness instead. Per file, record the mutation that reintroduces the defect — the scope to delete, or the invoke_user_callback! to unwrap — apply it to a scratch copy, run that file's test, assert it fails, restore, assert it passes. A run reporting "all mutations survived" is a red build, not a green one.

This is not over-engineering. A companion demo crate written specifically to be falsifiable still shipped three checks that could not fail — six examples that passed either way, and later two whose timeout verdict could not distinguish a blocked worker from a panicked one (recv_timeout(..).is_err() reads Disconnected as Timeout, because the sender drops on unwind). Each was found by running the mutation, never by reading the test. This work has eleven chances to make the same mistake.

Coverage boundary — say what is excluded

The monitor sees hiroz-owned locks. A callout made while holding a zenoh, std or other third-party lock still self-deadlocks and still passes the check.

Not a corner case: rmw-zenoh-rs exists to call into zenoh, and the equivalent open bug in the C++ implementation is a library-internal wait — undeclaring an entity blocks on in-flight callbacks of the very callback that is running. Put this in the monitor's module docs so completing this issue is not read as a stronger claim than it is.

Acceptance criteria

• Every callout in the callback-reachable set goes through invoke_user_callback!
• Every lock those callouts could be holding is a tracked type
• Removing any one scope reintroduces a failing test — demonstrated by a harness that re-runs, not by a one-time review
• CI recomputes the callback-reachable set and fails when a file enters it uninstrumented — verified by adding a dyn Fn call to an untracked file and watching the build go red
• The coverage boundary is written where a reader will find it, and names the third-party-lock exclusion

Ordering

Blocked by #282's six PRs — this conflicts with all of them, so it lands after they merge. It should be one PR, not one per file: it is a single mechanism applied uniformly plus one gate, and a partial gate cannot be green until the last file lands.