Re-entrancy: test-coverage gaps left open by #250

[YuanYuYuan]

Part of #282.

#250 fixes #249 and its CI is green. This issue records what that green does not prove.

Each item below is a revert that should fail the suite and does not. None of them is a defect in #250 — they mark the places where a future regression would pass unnoticed. Filed so the list survives the merge of #250, where it would otherwise stop being read.

This issue closes nothing. It is the honest remainder.

G1 — the gating decision is pinned; the wiring is not

Narrowed by #250 at 5aa7e383. Two unit tests now pin qos_needs_advanced in both directions: volatile_does_not_need_an_advanced_subscriber and transient_local_needs_an_advanced_subscriber. Forcing the function to return true unconditionally — the shape of #249 — fails the first and passes the second. Both directions were run, each asserting a non-zero test count.

What is still unpinned: no test asserts that the decision reaches the subscriber. ZSub holds its SubscriberHandle privately, so nothing can assert matches!(handle, SubscriberHandle::Plain { .. }) for a Volatile subscriber. A rewiring that ignored qos_needs_advanced and declared an AdvancedSubscriber anyway would still leave the suite green.

Fix: expose the handle, or add a test-only accessor, then assert the variant for both durabilities.

G1 was the highest-value item here while the decision itself was unpinned. It is not any longer. G2 now is: teardown discarding the backlog is unpinned, and it is the behaviour behind the one hazard #250 discloses on a public API.

Unpinned production changes

tag	Production change	Revert that stays green	Why nothing catches it
G2	Teardown discards the backlog	Pop pending before checking closed	No test drops a dispatcher with a non-empty queue, and none asserts on teardown duration
G3	destroy_subscriber releases the GIL before dropping	Restore the bare swap_remove	Its only test uses lambda _msg: None on topics no publisher writes to, so the drain thread never needs the GIL
G4	impl Drop for PyZNode releases the GIL	Delete the impl	No Python test drops a node or shuts the interpreter down with a callback in flight
G5	publish_raw releases the GIL	Delete py.allow_threads	No test calls publish_raw. (publish is pinned, by test_interpreter_stays_alive_during_reentrant_publish)
G6	LocalPublishGuard on publish_serialized and publish_sample	Delete either guard	Neither method is called by any test or example, so #249 stays reachable through two of the five public publish paths
G7	catch_unwind around the user callback	Call (*handler)(sample) directly	Nothing in the suite panics inside a callback. Without the guard the drain thread dies and the subscriber goes silent forever, with no log
G8	Teardown and self-drop on the plain/Volatile path	—	Both teardown tests are transient_local_*. On the plain path a remote sample runs the callback on an RX worker, so the join actually runs. This was checked by hand and does not deadlock — zenoh holds no lock across the callout — but nothing pins it. Volatile is the ROS 2 default
G9	Drop-order of SubscriberHandle's fields	Swap the two fields	No failure could be constructed: enqueue returns early once closed is set. Untested, not defective
G10	The escalating drop/backlog warn!	Remove either warning	No test asserts a warning is emitted. This is the only user-visible signal for the silent same-session sample loss #250 introduces

Test-quality gaps

tag	Gap	Effect
G11	transient_local_delivery_preserves_order is vacuous at one delivery — windows(2) yields nothing on a 1-element slice, and the only floor is !is_empty()	A run delivering 1 of 500 samples passes and reports ordering preserved, having observed no pair. A len() >= 2 floor restores the claim
G12	Both new harnesses treat recv_timeout(..).is_err() as "deadlocked"	A failing assert! drops the sender, so Disconnected is reported as a deadlock. #260 fixed this shape on main; these files reintroduce it. Wrong message, not a wrong verdict
G13	async_publish_delivers_off_the_publishing_thread asserts assert_ne! on thread ids where the name is available	If inline delivery stopped and the sample returned on an RX worker, it still passes. Asserting Some("hiroz-sub-drain") asserts the value rather than a difference
G14	The two new queue.rs tests pin BoundedQueue, not DispatchQueue	dispatch_capacity's doc cites zero_capacity_retains_one_sample as evidence about its own zero-depth behaviour. The test constructs the other type. Correct tests, wrong type for the claim

Detected, for contrast

Inverting either if runs_user_code branch is caught — one flip deadlocks transient_local_callback_republishing_on_same_topic_does_not_deadlock, the other kills self_feeding_callback_loop_iterates_without_a_depth_cap by stack overflow.

Genuinely undetected is only the narrower direction: making runs_user_code() return true for the queue variants too adds a needless thread and nothing fails.

Properties argued in comments, pinned by nothing

The items above are reverts that stay green. These are different in kind: each is a property #250 argues in a doc comment at its call site, and which its description once listed under a heading called "Evidence". Reading the code is not evidence.

tag	Property	Holds because of a dependency?	Test sketch
G15	No lost wakeup between enqueue and dequeue	no	Enqueue from a second thread with the drain thread already parked in wait. Assert the callback runs within a deadline. Repeat the race
G16	Drop does not hold the queue lock across join()	no	Park a callback on a barrier the test controls. Drop the subscriber from another thread. Release the barrier. Assert the drop returns
G17	No thread leaked when declare_subscriber fails	no	Force the declare to fail. Assert the thread count returns to baseline
G18	The plain path holds no zenoh lock when the callback runs inline	yes — zenoh	Publish from inside a callback fed by a remote sample. It must not deadlock
G19	No lock-order cycle with zenoh-ext's statesref	yes — zenoh-ext	Stress concurrent remote delivery and callback republication on a TransientLocal profile, under a deadline
G20	KeepLast(usize::MAX) is safe end to end	yes — zenoh-ext	Build such a subscriber. Assert it builds, delivers and drops
G21	The escalating warn counters cannot wrap	no	Set the counters near u64::MAX through a test-only constructor. Assert the warning still fires. Extends G10, which covers emission rather than arithmetic
G22	The action client's two dispatcher threads cannot hang teardown	no	Drop an action client with feedback in flight. Assert teardown returns within a deadline and the terminal status is observed

Important

G18, G19 and G20 hold because of how a dependency behaves today. A zenoh or zenoh-ext upgrade can break any of them with no change to hiroz, and nothing in the suite would notice. The other five break only when someone edits hiroz. Order the work accordingly.

Two further properties of this kind were already recorded above rather than given new tags: the panic guard is G7, and SubscriberHandle's field order is G9.

Suggested order

1. G18, G19, G20 — the three a dependency upgrade can break silently, with no hiroz change to review.
2. G11, G12 — the two gaps that let a test report a property it did not observe.
3. G2, G7, G10 — the silent-failure paths (teardown, panicking callback, lost warning).
4. G1's remainder — asserting the gating decision reaches SubscriberHandle::Plain.
5. The rest as convenient.

Related

• Notifier subscribers are exempt from the dispatcher on an unenforced claim #290 — notifier subscribers skip the dispatcher on an unenforced claim
• The ffi feature is built by no crate on the PR gate, and its re-entrancy detector was deleted #291 — the ffi feature is built on the gate but never linted or tested
• RMW_EVENT_MESSAGE_LOST is plumbed but never raised #292 — MESSAGE_LOST is never raised