Re-entrancy: decide what self-feeding callbacks should do — bound the depth

[YuanYuYuan]

Part of #282. A decision, not a defect — the fixes in that family are correct; what is unresolved is what re-entrant self-feeding should do, and today the answer differs by subsystem.

What changed

Before #282's fixes, a callback that re-entered its own subsystem deadlocked. After them it does not — but the two subsystems terminate differently, and neither result is bounded:

	self-feeding callback	bounded by
parameters (#257)	recurses → fatal runtime error: stack overflow	nothing
pub/sub (#250)	iterates on the drain thread, indefinitely	nothing (samples drop at queue depth)

Both are improvements on an unkillable hang. Neither is a result the caller can read and act on.

Why they differ — this is forced, not sloppy

The fix shape acq · callout · rel → acq · rel · callout fixes the ordering. It says nothing about which thread runs the callout, and the two subsystems could not spend that freedom the same way:

• Parameters are synchronous by contract. set_parameter returns SetParametersResult, and that result is the callback's verdict — validate_and_apply reads cb_result.successful and fails the set on rejection. The callout must complete before the call returns, so a queue is not available. Collect-release-call was the only option, and nested re-entry is therefore a nested call.
• Pub/sub could not be synchronous. The offending lock belonged to zenoh-ext's AdvancedSubscriber, not to us, and you cannot apply collect-release-call to a lock you do not own — there is no guard in your code to drop. Routing around the dispatcher meant taking session-local delivery onto our own queue and drain thread. And it was permitted because publish() promises nothing about delivery, so nothing forced synchrony.

So each PR took the only shape its constraints allowed. What is left over is a user-facing inconsistency: "re-entrancy is safe now" resolves to a crash in one subsystem and an unbounded loop in the other, and a user will generalise from whichever they meet first.

This is outside the property the monitor enforces

Worth stating so nobody expects the tripwire to catch it: no lock is held during the recursion. The guard is dropped before the callback runs — that is the fix. So the guard-count monitor is silent here and always will be.

Fixing the deadlock converted a safety violation (a thread stuck forever holding a lock) into a termination problem. Termination is liveness, and liveness has no finite witness — the same reason "this cannot deadlock" is not monitorable.

A depth cap converts it back into a safety property. "Callback depth never exceeds N" has a finite witness and a runtime monitor can decide it. That is the same move this whole family is built on, applied a second time to the problem the first application exposed.

This mechanism already existed here, and was deliberately deleted

Read this before evaluating the proposal, because without it the proposal looks like re-adding something the team just removed.

#250's third listed change:

Remove MAX_CALLBACK_REENTRY_DEPTH. Re-entrancy is now structurally impossible rather than depth-bounded, so the interim cap is deleted rather than left as unreachable code that silently drops samples.

So pub/sub had a depth cap, and removing it was correct there: once session-local delivery moved onto a queue, re-entry stopped being recursion at all, and a cap that can never fire is unreachable code that silently drops samples.

Parameters cannot have that queue. set_parameter returns SetParametersResult, and that result is the callback's verdict — validate_and_apply reads cb_result.successful and fails the set on rejection. The callout must complete before the call returns, so the work cannot be deferred to a drain thread. The structural fix that made the cap unnecessary for pub/sub is unavailable here.

That is the argument this issue rests on: the cap is not being reintroduced, it is being kept where the thing that replaced it does not apply. If a reviewer's instinct is "we just deleted this", the answer is that we deleted it from the one subsystem that earned the right to.

Correction to an earlier framing

An earlier version of this issue said pub/sub went asynchronous because the offending lock belonged to zenoh-ext and could not be reordered. That is only half of it, and the weaker half. #250's own description gives the real driver:

Zenoh dispatches a same-session sample inline on the thread that called put, so with the lock gone a publishing callback recursed instead of iterating.

So pub/sub hit exactly this issue's problem — remove the deadlock, get recursion — and answered it with the queue. Parameters hit the same problem and have no answer yet. The two subsystems are not solving different problems; pub/sub is already downstream of this decision, and this issue is parameters catching up.

A consequence worth recording: fixing the zenoh-ext lock upstream would not let #250's async delivery be reverted. That lock caused the deadlock; zenoh core's inline same-session dispatch causes the recursion, and only the second motivates the queue.

Proposal

Parameters — bound the depth

A thread-local depth counter with an RAII token, the same pattern as the guard counter, counting callback depth instead of lock depth:

let _depth = CallbackDepth::enter()          // RAII: ++ on enter, -- on drop
    .ok_or_else(|| SetParametersResult::failure(
        "on_set callback re-entered set_parameter more than N levels deep; \
         the callback is feeding itself"))?;

fatal runtime error: stack overflow becomes a SetParametersResult::failure the caller can read. A cap of 8–16 permits every sane cascade (/a → /b → /c) while catching the cycle.

A blanket ban is wrong and worth ruling out explicitly: rejecting any set entered from inside an on_set callback would regress #257's own acceptance test, parameter_on_set_callback_setting_another_parameter_does_not_deadlock. Single-level re-entry is the legitimate case and the thing #256 was filed about.

Alternative, if a number feels arbitrary: track the parameter names currently being applied on this thread and reject on repeat. That gives cycle: /a → /b → /a and needs no constant, at the cost of a thread-local Vec. Better diagnostics; ship the counter first if only one lands.

Pub/sub — already decided; confirm rather than reopen

Given the above, pub/sub's unbounded iteration is not an oversight to correct — it is the deliberate replacement for the cap that #250 deleted, and its tests assert it.

What is still worth deciding is whether unbounded iteration is the intended end state, or whether a self-feeding loop should eventually be reported. Bounded stack and a livelock is more survivable than a crash, so leaving it is defensible; it should just be a recorded decision rather than a property nobody revisited.

#250 tests the unbounded loop as accepted behaviour (self_feeding_callback_loop_iterates_without_a_depth_cap, intra_closed_loop_runs_iteratively). That may well be right — bounded stack, and a livelock is more survivable than a crash. But it should be a decision recorded here, not a property that exists because of how the fix was shaped.

Options: leave as-is and document; or add an analogous cap that logs and drops when a sample's causal chain exceeds N.

Either way — say it once, publicly

Whatever is decided, the re-entrancy contract belongs in user-facing docs in one place: you may re-enter, provided you terminate, and here is what happens if you do not.

What CI can and cannot do

CI cannot catch a user's self-feeding callback — that code is not in this repository.

What it catches is the guard going missing: a test with a deliberately self-feeding callback asserting a clean failure result. Remove the cap and the test recurses, the process aborts, and the run goes red. Per this family's standing rule that a check which has never printed a failure is unvalidated, that test must be proven to fire by removing the cap once — not assumed.

Acceptance criteria

• A self-feeding on_set callback returns a readable failure rather than aborting the process
• Single-level and few-level re-entry still work — fix(parameter): stop deadlocking on re-entry from an on_set callback #257's existing tests unchanged and passing
• The cap's detector is demonstrated to fire when the cap is removed
• A decision is recorded for pub/sub, whichever way it goes
• The re-entrancy contract is documented in one user-facing place