Re-entrancy: user callbacks invoked while a lock is held

[YuanYuYuan]

One defect class, found six times by hand. This issue states it once, tracks every issue and PR that addresses it, and defines what "done" means — so the individual PRs can stop restating the background and say only what they do.

The class

P — at every point where control escapes into unknown code (a dyn Fn, an Arc<dyn Fn>, an extern "C" fn), the calling thread holds no lock.

Violating it deadlocks whenever the callback re-enters. It is not a race: one thread, one call stack, reproduces every time the path runs, and adding logging changes nothing. A test that merely reaches the call site is enough to expose it — once something is watching.

Every fix in this family is the same rewrite:

acq · callout · rel     ⟶     acq · rel · callout

Collect, release, call. That is why the six fixes look alike.

Methodology

Fixing six instances by hand does not close the class — the seventh is written next week. So the series is in two halves:

1. Fix the instances — one PR per defect, each with its own reproduction.
2. Make the class unrepresentable. A debug-time monitor (feat(hiroz): debug-time barrier against callbacks under a lock #255) counts the guards live on the thread. It asserts that count is zero before every callout. Coverage and a CI gate keep that true.

Issues and PRs

One issue per role; each states its thing once and references rather than restates.

Issue	Role	PR
this issue	meta — the class, the graph, the order, the gate	—
#283	closure — what actually closes this issue	(after the instances below)
#284	residual — what #283 explicitly does not cover	(after #283)
#285	decision — what a self-feeding callback should do, now that it recurses instead of hanging	—
#287	residual, introduced by the fix shape — a callback can outlive rmw_event_set_callback(.., null) and use freed user_data	—
#288	residual, pre-existing — rmw never unregisters entity callbacks; same UAF class as #287, reached differently	—
#289	residual, pre-existing — two event-API sharp edges reachable from shipped public API	—
#290	residual, pre-existing — notifier subscribers are exempt from the dispatcher on an unenforced claim	—
#291	residual, mechanism gap — the ffi feature is built by no crate on the PR gate; its re-entrancy detector was deleted	—
#296	residual, coverage — what #250's green CI does not prove: 14 reverts that should fail the suite and do not	—
#302	residual, introduced by the fix shape — one drain thread per callback subscriber; measured at baseline + N	(held — see below)
#292	sibling — RMW_EVENT_MESSAGE_LOST is plumbed and never raised	#294
#293	sibling — SUBSCRIPTION_/PUBLISHER_INCOMPATIBLE_TYPE declared supported and never raised	—

What the residuals have in common

Fixing the deadlock does not finish the job, and each residual is a distinct way of not being finished:

• Re-entrancy: the guard stops at hiroz's own locks — widen the callout rule to the library boundary #284 — the guard cannot see locks we do not own.
• Re-entrancy: decide what self-feeding callbacks should do — bound the depth #285 — removing a hang converts it into unbounded recursion; something still has to bound it.
• Event callback can outlive rmw_event_set_callback(.., null) and use freed user_data #287 — moving the callout out of the lock removed an accidental guarantee that the lock was providing. The Arc owns the closure; nobody owns the C pointer it captured.
• rmw never unregisters entity event callbacks: leak plus a stale user_data kept reachable #288 — a registration that is never revoked keeps a stale pointer reachable, with no detach to hang a fix on.
• Two event-API sharp edges: set_callback fires the backlog under the caller's lock; duplicate register + one unregister drops all #289 — the unsafe shape is still reachable from public API, and the tripwire cannot see it because the lock is caller-owned.
• Notifier subscribers are exempt from the dispatcher on an unenforced claim #290 — a path is exempted from the fix on a justification the code does not enforce; the exemption is documented, which is not the same as being true.
• The ffi feature is built by no crate on the PR gate, and its re-entrancy detector was deleted #291 — the guard is required on five publish paths and detected on two; a compile error in ffi code already reached the gate unnoticed.
• Re-entrancy: test-coverage gaps left open by #250 #296 — the fix is correct and its detectors are thin; reverting the headline change leaves the whole suite green.
• Re-entrancy: the fix shape costs one drain thread per callback subscriber #302 — moving the callout off the delivery thread needs a thread to move it to, and a push API has no executor to borrow one from. The cost is forced by the contract, not chosen.

Siblings — a different defect class, found by the same reading

#292 and #293 are not re-entrancy. They came out of auditing this family's event paths and are recorded here so the audit's coverage is visible, not because the class fix touches them.

Their shape is declared-but-never-raised: rmw_event_type_is_supported returns true, rmw_event_set_callback installs a callback, rmw_take_event fills the status — and nothing ever raises the event, so the callback cannot fire and the status is permanently zero.

Audit of all 11 event types, verified in both directions (raise sites counted with a multi-line-aware search after a same-line grep undercounted three times):

rmw event	declared supported	raised	rmw_zenoh_cpp raises
SUBSCRIPTION_MATCHED, PUBLICATION_MATCHED	✅	✅	✅
REQUESTED_QOS_INCOMPATIBLE, OFFERED_QOS_INCOMPATIBLE	✅	✅	❌ — hiroz ahead
MESSAGE_LOST	✅	❌	✅ — #292, the only place hiroz trailed
SUBSCRIPTION_INCOMPATIBLE_TYPE, PUBLISHER_INCOMPATIBLE_TYPE	✅	❌	❌ (upstream TODO) — #293
LIVELINESS_CHANGED/_LOST, both DEADLINE_MISSED	❌ (None)	—	— correctly unsupported, not defects

#292's fix depended on this family. It raises through update_shared_event_status, which #260 introduced — raising it on main beforehand would have meant a callout under the manager lock, i.e. planting a fresh instance of the very defect #282 tracks. Resolved: #260 merged, #294 is retargeted to main and boundary-rebased, and the full CI suite now runs on it for the first time (it had only ever run the title check while its base was a branch).

#287 is the residual #260 left behind. It is not a latent pre-existing bug; that PR's fix shape creates it, and the same PR closes the analogous window for guard conditions by taking owned Arcs — which the event path cannot do, because no Arc can own a C pointer.

Issue	Defect	PR	Role	Status
#254	(enhancement) debug-time guard against callbacks under a lock	#255	mechanism — keystone	✅ merged 2026-08-04
#249	Publishing from inside a subscriber callback hangs	#250	instance	open, targets main, merge-base ae4f0f8f. Head 8724f1ae; CI re-running after a docs-only commit. Reviewed 2026-08-06 — one open tradeoff, see PR. Residuals split out to #296 and #302
#252	Lifecycle node cannot answer get_state during its own transition callback	#253	instance	✅ merged 2026-08-05
#256	Parameter API from inside an on_set callback deadlocks	#257	instance + coverage	✅ merged 2026-08-05
#259	Graph/event callbacks invoked with three locks held, plus a lock-order hazard	#260	instance + coverage	✅ merged 2026-08-06 (ae4f0f8f)
#261	rmw executor callbacks invoked under a mutex on the ordinary startup path	#262	instance + coverage	open, targets main, ✅ 29 checks green on cfaa2725, 0 failed

Related but not this class, tracked here only to avoid confusion:

• fix(graph): resolve startup deadlock on liveliness query with many live tokens #226 — startup deadlock on liveliness query with many live tokens. A different mechanism; needs its own merge-or-close decision.

Dependency structure

#255 defined the tracked lock types the other three consume, so it was a genuine keystone — #257, #260 and #262 could not compile without it. It merged on 2026-08-04, so the graph has collapsed to:

  ✅ #255  ✅ #253  ✅ #257  ✅ #260   merged
  ⬜ #250                              open, already rebased onto ae4f0f8f
  ⬜ #262                              open, TWO BLOCKERS — see the PR
  ⬜ #294                              open, sibling of #292

#260 merged 2026-08-06 as ae4f0f8f. It re-staled #250 and #262; both have since been rebased onto it and their merge-base is ae4f0f8f.

⚠️ #294 needed a boundary rebase, not a plain one. It was based on #260's branch, and #260 was squash-merged — so a plain rebase would have replayed #260's seven commits onto the squashed result. git rebase --onto origin/main <260-tip> replayed only #294's own five. Its diff dropped from 1363 lines to 372, which is the check that the boundary was right. Same technique as the #255 case below.

#262 is no longer stacked — it targets main directly. The only remaining ordering is weak: #262 and #260 share crates/rmw-zenoh-rs/src/rmw.rs, so whichever lands second needs a rebase. There is no type dependency; #262 compiles on main without #260.

All three have now had an adversarial review. #250's (2026-08-06) left one open decision — the TransientLocal path replaces transport backpressure with an unbounded queue, which its breaking-change list described as bounded. Stated in the PR as an explicit ship-or-block rather than resolved.

⚠️ Every merge re-stales the rest. MERGEABLE means "no conflicts", never "up to date", and a green run against the previous base proves nothing. After each merge the remaining PRs need a rebase and a fresh full CI run. This is not pedantry. #250 silently reverted #271. Nobody could see it until the branch was rebased, because git computes a stale branch's diff against a merge base where the regression does not yet exist.

⚠️ #226 collides with #260 in crates/hiroz/src/graph.rs. It is not part of this family (a different startup-deadlock mechanism), but whichever of the two lands second needs a rebase.

⚠️ Merging the keystone auto-closed two of its children

Recorded because it cost real time and the recovery is not obvious.

pr/1-reentrancy-tripwire was deleted when #255 merged, and GitHub auto-closed every PR that targeted it — #257 and #260, unmerged. Two deadlock fixes silently left the tree: main gained reentrancy.rs but parameter/service.rs still had no tracked lock, and nothing reported the gap.

The rule: retarget stacked children to main BEFORE merging the parent, not after.

Recovery, if it happens again. GitHub refuses to retarget a closed PR. It also refuses to reopen a PR whose base branch is deleted. So restore the base first, from the permanent PR head ref:

git push origin refs/pull/<parent>/head:refs/heads/<deleted-base>
gh pr reopen <N> && gh pr edit <N> --base main
git push origin --delete <deleted-base>      # the PRs survive this

Rebasing them then needs a boundary, not a plain rebase. #255 was squash-merged, so main holds one commit where the branch held eight; git rebase origin/main replays all eight onto the squashed result and conflicts immediately on the keystone's own first commit. Replay only each PR's own commits:

git rebase --onto origin/main <keystone-branch-tip>

Both rebased with zero conflicts. Each PR's diff came back at exactly its pre-close size.

Run that size check after every rebase. A mismatch means the boundary was wrong and foreign commits came along.

(Resolved: #262 was retargeted to main and rebased; all three are green on af00f1ed.)

Verification gate — per PR, before merge

1. The issue's reproduction fails on main and passes on the branch. Green CI proves a change is harmless, not that it fixes the issue.
2. Full CI on a main-based branch — 27 checks, not 4.
3. No vacuous suites. Several re-entrancy tests are behind feature flags; without them the binaries are empty and report 0 passed. A skip is a failure unless explicitly justified.

Definition of done

Merging all six does not close this issue. The monitor only sees lock types that were converted, and today it would see six of the eleven files that both take a lock and call through an opaque callee. Five files holding 27 acquisitions remain uninstrumented.

Spot-checked, those five are currently correct — each scopes its guard so it drops before the callout. That is the point: correctness there rests on someone having remembered the braces, with nothing that fails when the braces are removed. Which is exactly the state main was in before these six defects were found by hand.

Closing this issue requires #283 — convert the remaining files, and add a CI gate that fails when a file becomes callback-reachable without being instrumented. Its acceptance criteria live there; they are not repeated here, because two copies of a checklist drift and both look maintained while they do.

#283 is blocked by the six PRs above — it conflicts with all of them.

#284, #285, #287, #288 and #289 do not block closing this issue. They are the honest remainder: what stays true after the class is made unrepresentable. Each is tracked separately so that closing this one does not quietly close them — which is the whole reason an instance fix is never allowed to close a meta issue.

Known limitation, stated up front

The monitor sees hiroz-owned locks. A callout made while holding a zenoh, std or other third-party lock still self-deadlocks and is invisible to it. This is not hypothetical: the equivalent open bug in the C++ implementation is a library-internal wait, where undeclaring an entity blocks on in-flight callbacks of the very callback that is running.

So the completed work should be read as: this class cannot recur on any instrumented path — not as full coverage.

Tracked as #284, which is the largest residual once #283 lands, and the one neither a runtime counter nor a compile-time encoding reaches: it is closed by widening the definition of the event being guarded — from "control escapes to unknown code we were handed" to "control escapes anywhere it may re-enter us, or block on a callback we are running" — not by strengthening the mechanism that guards it.