bridge: installReconnectHandler's liveness-check-then-touch-`this` window (morph#486's shape), with no safe mechanical fix

[Yaraslaut]

Summary

Bridge gates five of its own callbacks on CallbackToken::active() and then
touches this. That is the same check-then-act shape as morph#486, which was a
use-after-free — and #486's fix (PR #488) deliberately covers only
~BridgeHandler, because that is the one site where a documented guarantee was
being broken. The other sites are untouched and are recorded here rather than
folded into that PR.

Verification status

Inferred from reading the code. Not reproduced, not measured. I did not
construct a case that lands in any of these windows, and I do not know their
rates. What is measured is that the identical shape one function away
(~BridgeHandler → Bridge::deregisterHandler) faulted 26 times in 200
unsanitized runs — see #486 and PR #488. That is evidence about the shape, not
about these five call sites.

Revision: include/morph/core/bridge.hpp at origin/master fdbfd65, line
numbers as of that revision.

The sites

Each does alive.active() / weakLiveness.active() and then dereferences
this. Between the two, another thread can complete ~Bridge.

1. executeVia's .then continuation (bridge.hpp:1495-1520). Reads
   bool const bridgeAlive = alive.active(); then this->_pendingCalls,
   onResult(...) (which for a result-keyed action reaches back into the bridge
   through a captured raw pointer), hasSubscribers() and publishResult(...).
   Runs on whichever thread the backend resolved on.
2. executeVia's .onError continuation (bridge.hpp:1555-1561). Same
   check, then this->_pendingCalls.fetch_sub(...).
3. registerHandlerImpl's onRegistered callback (bridge.hpp:1629-1646).
   Checks weakLiveness.active(), then takes _mtx and calls loadBackend() —
   both members of a possibly-destroyed Bridge.
4. installReconnectHandler's handler (bridge.hpp:1740-1775). Checks the
   token, then std::scoped_lock{_mtx, _attachMtx} and iterates _handlers.
   Explicitly documented as running "on the backend's transport thread".
5. switchBackend's and the whenBound paths were not audited beyond the
   above; treat the list as "at least these", not "exactly these".

The comments at sites 1 and 2 already reason carefully about whether the
check is needed and get that part right — the schedulerRef capture next to
them is a correct, pin-based solution to the same problem for the
TimeoutScheduler. What none of them addresses is that the check and the use
are two separate steps.

Why PR #488 did not extend the gate to these

detail::BridgeLifetime (added by #488) would close every one of these
mechanically. It was not applied here because it makes ~Bridge block, and
each site needs its own deadlock argument before that is safe:

• Site 4 is the clear counter-example. QtWebSocketBackend::registerModel
  blocks the calling thread on a nested QEventLoop, and the reconnect handler
  calls registerModelWithContext. Holding a gate across that, while ~Bridge
  on the Qt thread waits for it, is a deadlock by construction.
• Sites 1 and 2 call publishResult, which reaches subscription sinks. Blocking
  ~Bridge behind consumer-supplied code is a different risk from blocking it
  behind deregisterModel, which is provably non-blocking on every shipped
  backend.

So the honest position is: the gate is the right primitive, but "apply it
everywhere" is not a safe mechanical edit, and the alternative for these sites
may be pinning (as schedulerRef already does) rather than gating.

What would change the verdict

• Close it if someone shows these windows are unreachable — e.g. that every
  path into them is already ordered behind something that outlives ~Bridge.
• Raise it if any of them is reproduced. Site 4 is the most promising to
  attack: a QtWebSocketBackend reconnect firing on the transport thread while
  the owner destroys the Bridge.

Related

• Bridge::deregisterHandler racing ~ThreadPoolExecutor is a use-after-free (1 in 40 runs of bookmarks' fetch-timer test) #486 / PR bridge: make a handler's deregistration outlast the bridge's own destructor (#486) #488 — the same class, at ~BridgeHandler, reproduced and fixed.
• docs/spec/core/callback_scope.md, "Boundary of the guarantee" — why
  CallbackToken::active() cannot carry this weight.

🤖 Filed with Claude Code

https://claude.ai/code/session_01C6uB9zxdSG8qp3VNAAyFvF

[Yaraslaut]

Triage: triage: valid, with three corrections and a constrained fix

Read against origin/master fdbfd65 in a clean worktree, and against
docs/spec/core/bridge.md / concurrency_and_lifetimes.md as PR #488 leaves
them (gh pr diff 488), not as they are on master today.

Verdict: the five sites exist, the shape is as described, and the deadlock
argument that stops #488's gate being extended mechanically is real. Three
things in the issue need correcting before anyone works it, none of which
changes that verdict.

---

1. All five sites exist, and all five are check-then-touch-this

Confirmed by reading, at the line numbers given:

#	Site	Check	Then touches this
1	executeVia's .then	bridge.hpp:1508 bool const bridgeAlive = alive.active();	bridge.hpp:1515 this->_pendingCalls.fetch_sub(...), :1522 onResult(...), :1530 hasSubscribers(), :1531 publishResult(...)
2	executeVia's .onError	bridge.hpp:1559 if (alive.active())	bridge.hpp:1560 this->_pendingCalls.fetch_sub(...)
3	registerHandlerImpl's onRegistered	bridge.hpp:1631 bool const bridgeAlive = weakLiveness.active();	bridge.hpp:1634 std::scoped_lock const lock{_mtx};, :1636 loadBackend()
4	installReconnectHandler's handler	bridge.hpp:1741 if (!weakLiveness.active())	bridge.hpp:1748 std::scoped_lock const lock{_mtx, _attachMtx};, :1749 loadBackend(), :1752 _handlers

None is a superficially-similar read: every one dereferences this after the
check, and #488's gate is not handed to any of them (lifetimeGate() is copied
only into BridgeHandler::_bridgeGate).

The list is longer than five. grep -n '\.active()' include/morph/core/bridge.hpp
returns nine sites; 1904 is the one #488 fixes, 1349 is a comment. The remaining
three are the same shape and are not in the issue:

• attachHandlerAsync's attachModelAsync reply — bridge.hpp:536 check, then
  bridge.hpp:549 std::scoped_lock const guard{_attachMtx} + :551 loadBackend().
• ensureBoundAsync's registerModelSharedAsync reply — bridge.hpp:685
  check, then :698 _attachMtx + :700 loadBackend().
• assignHandlerPrimary's assignPrimaryAsync reply — bridge.hpp:821 check,
  then :828 _attachMtx + :830 loadBackend().

The issue's "at least these, not exactly these" hedge was the right call. It is
eight, plus whatever switchBackend/whenBound audit turns up.

2. Correction: sites 1 and 2 do not run "on whichever thread the backend resolved on"

Completion marshals invocation through cbExec. CompletionState::setValue
builds the fan-out closure and posts it (completion.hpp:78-79):

if (callback != nullptr && cbExec != nullptr) {
    cbExec->post(std::move(callback));
}

and executeVia passes the handler's GUI executor straight through as that
cbExec (bridge.hpp:1458 backend->execute(ModelId{raw}, std::move(call), cbExec),
where cbExec is BridgeHandler::_guiExec). So the .then/.onError bodies
run on the GUI executor's thread, not the resolving thread. Measured, not
inferred — this is a live TSan stack from ladder_bookmarks_tests on this
revision, showing the continuation arriving on the main thread through Qt:

  Read of size 8 at 0x720c00007808 by main thread:
    #1 CompletionState<shared_ptr<void>>::setValue(...)::'lambda'()   completion.hpp:63:62
    #6 morph::qt::QtExecutor::post(...)::'lambda'()::operator()()     qt_executor.hpp:89:17
    #8 QObject::event(QEvent*)                                       qobject.cpp:1480
  Previous write ... by thread T2:
    #5 CompletionState<shared_ptr<void>>::setValue(...)              completion.hpp:52:26
    #6 SimulatedRemoteBackend::execute(...)::'lambda'(...)           remote.hpp:2022:36
   #12 StrandExecutor::scheduleNext(...)::'lambda'()                 strand.hpp:156:17

That matters for reachability: in the ordinary single-GUI-thread wiring, sites 1
and 2 execute on the same thread that runs ~Bridge, so they cannot interleave
with it. They become concurrent only if the Bridge is destroyed off the
cbExec thread.

This is not academic after #488, and it is worth saying explicitly: #488's spec
change promotes the teardown contract from "order-independent" to
"order-independent on any thread" (bridge.md: "defined behaviour on any
thread"; concurrency_and_lifetimes.md: "neither does which thread each side
runs on"). A reader who takes that at face value and destroys a Bridge on a
worker thread lands exactly in sites 1–4. So #488 does not merely leave these
open — it slightly widens the set of callers entitled to reach them.

Relatedly, the contract as #488 leaves it says liveness() "still gates
delivery of the bridge's own callbacks, and that is the job it is right for:
a suppressed callback simply does not run, so a check that has gone stale costs
nothing". That is true of Completion's delivery gate; it is not true of
these eight sites, where the check is followed by member calls on this. The
spec sentence and the code disagree, and that disagreement is itself a finding
worth recording on the issue rather than being resolved silently.

3. Reachability, per site, against the four shipped backends

Bridge's constructor arms the reconnect path unconditionally
(bridge.hpp:337 installReconnectHandler(_backend);), and switchBackend
re-arms it (:1246).

• Sites 1 & 2 — need ~Bridge on a thread other than the handler's
  cbExec. Every backend can resolve off-thread (LocalBackend via
  StrandExecutor, backend.hpp:798/:842; SimulatedRemoteBackend via
  RemoteServer's pool; SocketBackend via its read loop), but the body
  still lands on cbExec. No shipped rung destroys its Bridge off the GUI
  thread — examples/common/gui/app_context.hpp and
  examples/kanban/src/headless/main.cpp both own the Bridge on the Qt loop
  thread. Inferred-from-reading, not reproduced. Note that the existing
  Bridge: hasSubscribers is not read once the bridge is destroyed (guard-page death test) in tests/test_bridge_lifetime.cpp already establishes the
  sequential half — that this continuation can run with this dangling, and
  that bridgeAlive is the only thing standing between it and a fault on a
  PROT_NONE page. What is unproven is the concurrent interleaving, not the
  reachability of the callback.

• Site 3, and the three unlisted siblings (536/685/821) — these fire only
  from the four *Async backend hooks, and QtWebSocketBackend is the only
  backend that overrides any of them (grep -rn 'bool registerModelAsync|bool registerModelSharedAsync|bool attachModelAsync|bool assignPrimaryAsync'
  returns exactly qt_websocket_backend.hpp and the backend.hpp defaults).
  The defaults return false without invoking either callback
  (backend.hpp:149-160), so on LocalBackend/SimulatedRemoteBackend/
  SocketBackend these closures are never invoked at all — the synchronous
  fallback runs instead. On QtWebSocketBackend they fire from onTextMessage
  on the Qt loop thread. So: same thread as a Qt-owned ~Bridge. Not
  concurrently reachable on any shipped backend in the ordinary wiring.

• Site 4 — the one with a genuinely concurrent shipped path.
  net::SocketBackend deliberately runs the reconnect handler on a dedicated
  thread (socket_backend.hpp:77 _handlerThread = std::thread{...},
  :444 handlerThreadMain()), and its own comment says why it must not run
  inline (socket_backend.hpp:425-437: the handler "goes through sendSync ->
  wait on _syncCv … Run inline, that wait blocks the one thread responsible
  for satisfying it"). That thread is not the owner's, so it is genuinely
  concurrent with ~Bridge. QtWebSocketBackend fires it on the Qt loop thread
  (src/qt/qt_websocket_backend.cpp:55-57, inside the connected lambda);
  LocalBackend/SimulatedRemoteBackend never fire it at all (backend.hpp:439
  default is store-and-ignore). In this tree the Bridge-over-SocketBackend
  wiring appears only in tests/testkit, but nothing in the framework restricts
  it, and Bridge arms the handler for whatever backend it is given.

4. The deadlock claim is correct — and stronger than stated

Verified, path exists:

installReconnectHandler's handler calls pinned->registerModelWithContext(...)
(bridge.hpp:1769; the shared branch registerModelShared at :1766). QtWebSocketBackend does not override
registerModelWithContext; the base default forwards to registerModel
(include/morph/core/backend.hpp:101-106):

virtual ::morph::exec::detail::ModelId registerModelWithContext(
    const std::string& typeId, std::function<...> factory, std::string_view contextKey) {
    (void)contextKey;
    return registerModel(typeId, std::move(factory));
}

QtWebSocketBackend::registerModel calls sendSync
(src/qt/qt_websocket_backend.cpp:146), and sendSync parks a nested event
loop (src/qt/qt_websocket_backend.cpp:113-137):

    QEventLoop loop;
    _syncLoop = &loop;
    loop.exec();

The shared branch is the same: registerModelShared → sendSync
(qt_websocket_backend.cpp:330). So the claim holds as written.

Two amplifications the issue understates:

1. That nested loop has no timeout. loop.exec() in sendSync is armed by
   nothing — only a reply or a disconnected signal quits it (contrast
   waitForConnected, which does QTimer::singleShot(timeoutMs, ...) at
   :107). Gating site 4 would therefore make ~Bridge block for an unbounded
   network round-trip, not merely a long one. bridge: make a handler's deregistration outlast the bridge's own destructor (#486) #488's own justification for a
   blocking ~Bridge is explicitly that the guarded region is provably
   non-blocking (deregisterModel, "fire-and-forget sends … precisely so that
   destruction never spins a nested event loop"). Site 4 is the exact inverse of
   that argument.
2. There is a same-thread self-deadlock too. The nested loop pumps the Qt
   queue while the gate is held shared. Anything it dispatches that reaches
   ~Bridge — a queued deleteLater on the owner — takes the same
   shared_mutex exclusively on a thread already holding it shared. That is
   precisely the rule bridge: make a handler's deregistration outlast the bridge's own destructor (#486) #488 adds to concurrency_and_lifetimes.md ("a
   ~BridgeHandler must not be reachable from the thread that is already inside
   ~Bridge … self-deadlock on the gate"), reached from the other end.

net::SocketBackend is no better: its reconnect handler also goes through
sendSync (its own comment says so), so gating it blocks ~Bridge behind a
round-trip on that backend too.

Conclusion: the fix is constrained, and the issue's own preference is the
right one. BridgeLifetime cannot be applied to site 4. Pinning — the
schedulerRef pattern already used one function away (bridge.hpp:1342-1357,
which spells out exactly this reasoning for TimeoutScheduler) — is the shape
that composes here, because it keeps the callee alive without making the
destructor wait on the caller.

What would still change the verdict

Unchanged from the issue, plus one:

• Close sites 3/536/685/821 if someone confirms the reading above — that the
  only backend implementing the *Async hooks fires them on the same thread
  that owns the Bridge — and records that as an invariant rather than an
  accident. They are currently unreachable-by-construction on every shipped
  backend, which is a much weaker claim than sites 1/2/4 carry.
• Raise site 4 by reproducing it: a Bridge over net::SocketBackend,
  reconnect firing on _handlerThread, owner destroying the Bridge. That is
  the one site with a shipped concurrent path and no same-thread argument.

Labels

triage: valid, area: core.

🤖 Generated with Claude Code

https://claude.ai/code/session_01C6uB9zxdSG8qp3VNAAyFvF

[Yaraslaut]

PR #491 fixes 3 of the 5 named sites — sites 1, 2 (executeVia's .then/.onError) and site 3 (registerHandlerImpl's async reply).

Sites 1 & 2: _pendingCalls and _subscriptions are now heap-allocated (shared_ptr, mirroring detail::BridgeLifetime's own pattern) and captured by value into the continuations instead of reached through this. Once pinned, neither needs a liveness check to touch safely — _pendingCalls is now decremented unconditionally, and SubscriptionRegistry::publishResult already snapshots its sinks under its own lock and invokes them outside it, so it can't deadlock against a re-entering subscriber. onResult is the one piece that's genuinely not pinnable (it calls assignHandlerPrimary, which needs the current bridge/backend) — that one call is gated on detail::BridgeLifetime, safe because assignHandlerPrimary's synchronous path is non-blocking by construction.

Site 3: gated on detail::BridgeLifetime across its whole touch of this (_mtx, loadBackend()) — safe to hold there since nothing in that span calls into consumer code or a blocking backend path.

Site 4 (installReconnectHandler) is deliberately left unfixed — this PR confirms your own analysis: gating it the mechanical way would let ~Bridge block behind registerModelWithContext/registerModelShared's nested-event-loop wait on QtWebSocketBackend, and if the reconnect and the destructor ever land on the same thread that's a self-deadlock, not a slow teardown. I looked for a safe alternative (snapshot the handler list under the gate, release before the blocking calls) but that reintroduces a real race against a concurrent switchBackend() that the current full-critical-section locking prevents — not a improvement worth the new correctness risk. No safe fix found; reasoning recorded in docs/spec/concurrency_and_lifetimes.md's new "The same check-then-call shape, elsewhere in Bridge" section.

Site 5 (switchBackend/whenBound) — not audited beyond what was already true when this issue was filed; still open.

Verified against the full local suite (21985 assertions, same behavior before/after) and against test_bridge_lifetime.cpp's existing dedicated regression cases for sites 1-2 (the onResult dangling-bridge case and the hasSubscribers guard-page death test), which pass unmodified — the fix changes how each site is made safe, not what those tests observe.

Leaving this open for sites 4/5.

[Yaraslaut]

Audit finished: site 5 is clean, the *Async sites are contract-safe — PR #492

This closes out the two items PR #491 left. Scope is now site 4 alone.

Site 5 — audited, does not have the shape

The issue listed switchBackend/whenBound as "not audited beyond the above; treat the list as at least these, not exactly these." Audited now: neither is a liveness-gated callback.

• switchBackend() (bridge.hpp:1230) is an ordinary synchronous member function called by the bridge's owner. It takes no CallbackToken and performs no active() check — the caller not calling it on a destroyed Bridge is the same ordinary requirement every member function carries, not the Bridge::deregisterHandler racing ~ThreadPoolExecutor is a use-after-free (1 in 40 runs of bookmarks' fetch-timer test) #486 shape.
• whenBound() (bridge.hpp:967) likewise, and its queued waiters capture only the CompletionState they resolve ([state](bool ok) / [state](std::exception_ptr err)) — never this. Nothing to dangle.

Recorded in the spec so the next reader does not redo it.

The three *Async reply sites — the real finding

The triage comment above listed these as "currently unreachable-by-construction on every shipped backend, which is a much weaker claim than sites 1/2/4 carry", and asked for it to be "recorded as an invariant rather than an accident". Confirmed, and done.

attachHandlerAsync (:601 post-#491), ensureBoundAsync (:758) and assignHandlerPrimary (:902) each still test weakLiveness.active() and then take _attachMtx and call loadBackend() — the two-step shape, present in the source.

The reason they are safe: QtWebSocketBackend is the only backend in the tree overriding any *Async hook (grep over include/, examples/, tests/ — the rest are test doubles). Its header requires it be used from the Qt event loop thread (qt_websocket_backend.hpp:93, "Must be used from the Qt event loop thread"), and it documents all four callbacks as firing on that thread from onTextMessage (:205, :327). Check and use are on the same thread that would run ~Bridge, so they cannot straddle a destructor.

That was true but written nowhere. A future backend replying on its own transport thread would reopen morph#486's use-after-free silently — no test failing, no comment objecting. PR #492 therefore states it as a contract on IBackend::registerModelAsync (with the other three hooks pointing at it) and names it at each of the three Bridge call sites, so an off-thread delivery is a documented contract break rather than a latent race to be rediscovered.

detail::BridgeLifetime is not the instrument here, for site 4's reason: gating would make ~Bridge block behind a backend's registration path.

Remaining scope

Site 4 (installReconnectHandler) only. #491's finding stands and I did not find a way past it: gating deadlocks against sendSync's unbounded nested QEventLoop (no timeout, unlike waitForConnected), and the snapshot-then-release alternative reintroduces a race against a concurrent switchBackend() that the current full-critical-section locking prevents. net::SocketBackend's reconnect handler goes through sendSync too, so it is no better. The reasoning lives in docs/spec/concurrency_and_lifetimes.md.

Raise it by reproducing: a Bridge over net::SocketBackend, reconnect firing on _handlerThread, owner destroying the Bridge. That is still the one site with a shipped concurrent path and no same-thread argument.

[Yaraslaut]

Site 4 status: unchanged, and deliberately so

Re-examined while resolving the rest of the audit batch. Nothing has changed the analysis, and I did not attempt a speculative fix.

The two independent assessments on record — PR #491's and this issue's own triage — both concluded there is no safe mechanical fix, for reasons that still hold:

• detail::BridgeLifetime cannot be applied: registerModelWithContext/registerModelShared block on sendSync's unbounded nested QEventLoop (no timeout, unlike waitForConnected), so gating would make ~Bridge wait on a network round trip — and if the reconnect and the destructor land on the same thread, that is a self-deadlock rather than a slow teardown.
• Snapshotting the handler list under the gate and releasing before the blocking calls reintroduces a race against a concurrent switchBackend() that the current full-critical-section locking prevents.
• net::SocketBackend's reconnect handler goes through sendSync too, so it is not a Qt-only constraint.

Worth adding one data point from this batch, because it is the same shape and it settled the analogous question by evidence rather than argument: for morph#505 I implemented the mechanical lock-taking fix and it failed an existing test ("Bridge: an in-flight shared attach does not block unrelated handler registration"), which is what established that reading as wrong. That is the standard site 4 would need to meet — and unlike #505, no test exists here that would tell us either way, because the window has never been reproduced.

So the honest position is unchanged: this is documented known behaviour, not an oversight, with the reasoning in docs/spec/concurrency_and_lifetimes.md. Raising it needs a reproduction — a Bridge over net::SocketBackend, reconnect firing on _handlerThread, owner destroying the Bridge — not another round of reasoning.

[Yaraslaut]

Closing at the maintainer's request.

Sites 1-3 were fixed by PR #491 and site 5 was audited clean (PR #492) — switchBackend() and whenBound() are ordinary synchronous member calls, not liveness-gated callbacks. What remained was site 4, installReconnectHandler, and it has no safe mechanical fix: detail::BridgeLifetime would let ~Bridge block behind sendSync's unbounded nested QEventLoop, and the snapshot-then-release alternative reintroduces a race against a concurrent switchBackend().

The reasoning is in docs/spec/concurrency_and_lifetimes.md, so the behaviour is documented rather than merely unfixed. Raising it again needs a reproduction — a Bridge over net::SocketBackend with reconnect firing on _handlerThread while the owner destroys the bridge — not more analysis.