From f0f4be90c9ed77a7c105e13a0982f0d5e33cf8ad Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Mon, 21 Sep 2026 09:24:03 +0200 Subject: [PATCH 1/5] core: give Bridge an executor for the registration replies that outlive their dispatch frame (fixes #588) `Bridge` issues registrations on its own behalf, and until now it was the only caller in the framework that could not say where their completions are delivered: its five dispatch sites name `exec::detail::inlineExecutor()`, so a reply is published on whichever thread the backend settled it on. That is the thread morph#486's use-after-free is about -- each of those callbacks asks "is the `Bridge` still alive" and then touches it. `Bridge`'s constructor now takes an optional `IExecutor* bridgeExec`, and `detail::deliverLate` routes a reply to it. Null -- the default -- runs the reply inline, which is byte for byte the previous behaviour, so nothing that does not ask for the new argument changes at all. The ticket asked for the five `inlineExecutor()` arguments to be replaced. They are deliberately **not**, and that is the one place this deviates from what #588 says. Naming a posting executor on the `bindModel`/`promoteModel` call breaks two things that are not negotiable: * `registerHandler()` stops being synchronous for every backend that binds inline. The settle becomes a queued task, `claimHandoff` finds nothing parked, and the caller gets an unbound handler from a call that has always returned a bound one. * a `kCallerMayBlock` backend deadlocks outright when the dispatching thread is the executor's thread -- `awaitHandoff` waits for a task only that same thread could run. A GUI embedder passing its GUI executor is exactly that case. An inline settle has to reach `parkIfInFrame` inside the dispatch frame, and the dispatching frame then publishes it on the dispatching thread. Only the *late* reply has a delivery thread left to choose, and that is the one the new executor gets. `assignHandlerPrimary` grew an `AsyncDispatchHandoff` for that reason: it had no way to tell the two cases apart, and posting both would have made an inline promote asynchronous. What this buys, stated as narrowly as it is true: the morph#486 window is closed for an embedder whose executor runs on a thread that cannot run `~Bridge` concurrently -- the callback and the destructor are then two tasks on one thread. An executor on an unrelated thread satisfies the type and closes nothing; it makes nothing worse either, since the existing `CallbackToken`/`BridgeLifetime` gates are untouched. That, rather than an unqualified "the guarantee is now structural", is what the specs now say. Rejected alternatives, both from #588's own triage thread: giving `Bridge` a thread of its own contradicts `concurrency_and_lifetimes.md`'s "teardown is order-independent, on any thread" and cannot exist in an Emscripten build with no pthreads; capturing the constructing thread names a thread nobody promised would be the destroying one. Verified: `morph_tests` 1561 cases, 22972 assertions, all pass (one deliberate expected failure). The three new cases in `tests/test_async_registration.cpp` were mutation-checked -- making `deliverLate` ignore its executor and always run inline fails the late-reply case (`binding->currentId` 100 != 0, `queued()` 0 != 1), which is the check AGENTS.md asks for. `deliverLate` is a template rather than a `std::function` parameter so the null path type-erases nothing and allocates nothing: taking a `std::function` put an allocation on the default path and made morph#108's OOM-injection case catch the wrong allocation. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01VptDWG2fKr2vBnLSJcgzgW --- docs/spec/concurrency_and_lifetimes.md | 49 ++- docs/spec/core/backend.md | 45 ++- docs/spec/core/bridge.md | 59 +++- include/morph/core/bridge.hpp | 414 ++++++++++++++++++------- tests/test_async_registration.cpp | 123 ++++++++ 5 files changed, 539 insertions(+), 151 deletions(-) diff --git a/docs/spec/concurrency_and_lifetimes.md b/docs/spec/concurrency_and_lifetimes.md index 4382b5b55..21447c16a 100644 --- a/docs/spec/concurrency_and_lifetimes.md +++ b/docs/spec/concurrency_and_lifetimes.md @@ -333,8 +333,20 @@ that bounded wait into an unbounded one. Four dispositions, by site: morph#571 deleted the twins, so there is no such contract left to state — but the three sites name `exec::detail::inlineExecutor()` as the delivery executor, which reproduces the old delivery thread exactly: the continuation - runs wherever the backend settled. **The window is therefore unchanged, not - closed.** `QtWebSocketBackend` is still safe for the reason it always was: it + runs wherever the backend settled. + + **Since morph#588 the window is closed for a `Bridge` that was given an + executor, and unchanged for one that was not.** The `bindModel`/ + `promoteModel` argument is still `inlineExecutor()` — deliberately, because a + reply that settles inside the dispatch frame must reach `parkIfInFrame` + there, or `registerHandler()` stops being synchronous and `awaitHandoff` + deadlocks against its own executor's thread. What moved is the *late* reply, + the only one that has a thread left to choose: `detail::deliverLate` posts it + to the `bridgeExec` the constructor was given, so for an embedder whose + executor runs on the thread that also runs `~Bridge`, the check and the + destructor are two tasks on one thread and cannot interleave at all. With the + default null executor the delivery is inline and the window is exactly what + it was. `QtWebSocketBackend` is still safe for the reason it always was: it must itself be used from the Qt event loop thread and settles every reply from `onTextMessage` on that same thread, so the check and the use cannot straddle a destructor. Gating these instead would make `~Bridge` block behind @@ -344,27 +356,32 @@ that bounded wait into an unbounded one. Four dispositions, by site: **What changed with the removal is who could get it wrong, not whether it can be wrong.** A backend that settles a `bindModel` completion on its own - transport thread would still reopen morph#486's use-after-free here; the - difference is that the delivery thread is now a value one call site produces - rather than an obligation on fifteen backend authors, so closing it is a - change in one place. That change — giving `Bridge` an executor of its own — - is morph#588 and has not been made. + transport thread reopens morph#486's use-after-free here for a bridge with no + `bridgeExec`; the difference morph#571 made is that the delivery thread is a + value one call site produces rather than an obligation on fifteen backend + authors, so closing it was a change in one place. morph#588 made it: the + choice is a constructor argument, and the residual exposure is the embedder's + own — supplying an executor on a thread unrelated to teardown satisfies the + type and closes nothing, which is stated where the argument is documented + rather than left to be discovered. The structural surface that replaces these four hooks — `IBackend::bindModel`/`promoteModel` — takes the executor the continuation is delivered on as an argument, so the delivery thread is chosen by the caller, which knows what its own teardown looks like, instead of by the backend, which - does not. `Bridge` now reaches it at all four sites (morph#568). **That does - not close the window above, and morph#568 does not claim it does**: `Bridge` - owns no event loop, so the executor it names is + does not. `Bridge` now reaches it at all five sites (morph#568, morph#615). + **That does not close the window above, and morph#568 does not claim it + does**: `Bridge` owns no event loop, so the executor it names is `exec::detail::inlineExecutor()` — "deliver wherever you settled", which is what the prose contract already required. What changed is where the decision - lives: one value produced at four `Bridge` call sites, rather than a - documented obligation on every `IBackend` implementor. Closing the window - means giving `Bridge` an executor bound to the thread that runs `~Bridge` and - naming that instead; nothing in the morph#522 set does that. See - [core/backend.md](core/backend.md#the-structural-registration-surface--bindmodel-and-promotemodel) - and morph#522. + lives: one value produced at five `Bridge` call sites, rather than a + documented obligation on every `IBackend` implementor. morph#588 then gave + `Bridge` an executor of its own and used it for the late replies — not in + place of the `inlineExecutor()` argument, which the in-frame settle needs, so + the two cases are now told apart by the handoff rather than by the executor. + See + [core/backend.md](core/backend.md#the-structural-registration-surface--bindmodel-and-promotemodel), + [core/bridge.md](core/bridge.md) and morph#522. `switchBackend()` and `whenBound()` were audited for the same shape and do not have it. Both are ordinary synchronous member functions called by the bridge's diff --git a/docs/spec/core/backend.md b/docs/spec/core/backend.md index 723381cca..ea15ce9b6 100644 --- a/docs/spec/core/backend.md +++ b/docs/spec/core/backend.md @@ -311,12 +311,23 @@ event-loop thread passes that thread's executor and the two-step check-then-dereference can no longer straddle a destructor, by construction rather than by the backend author having read a `@note`. -**And `Bridge` is not yet that caller.** Its four dispatch sites all name -`exec::detail::inlineExecutor()`, which runs the continuation on whichever -thread the backend settled on — deliberately the *old* delivery thread, so -morph#568 and morph#571 change no observable threading. `Bridge` owns no event -loop and has no thread of its own to name. So the window morph#486 describes is -**unchanged, not closed**: the decision moved, the value did not. For +**`Bridge` is that caller for half of it, since morph#588.** Its five dispatch +sites still name `exec::detail::inlineExecutor()` on the `bindModel`/ +`promoteModel` call itself, and that is now a decision rather than an absence: +an inline settle has to reach `Bridge::detail::parkIfInFrame` *inside* the +dispatch frame, because that is what keeps `registerHandler()` synchronous for +a backend that binds inline and what stops `detail::awaitHandoff` waiting on a +task only the waiting thread could run. What morph#588 added is an executor for +the other case — a reply that arrives after the dispatch frame has gone, which +is the only one with a thread left to choose. `Bridge`'s constructor takes an +optional `bridgeExec`, `detail::deliverLate` routes exactly those replies to +it, and a null one (the default) runs them inline, where they ran before. + +So the window morph#486 describes is **closed for an embedder that supplies an +executor whose thread also runs `~Bridge`** — the continuation and the +destructor are then two tasks on one thread and cannot interleave — and +**unchanged for one that does not**, which is every caller that has not been +updated. What is no longer true is that `Bridge` has nothing to name. For `QtWebSocketBackend` the safety is the same by-construction safety it always had — it must itself be used from the Qt event loop thread and settles every reply from `onTextMessage` on that same thread, so the check and the use cannot @@ -324,12 +335,13 @@ straddle a destructor. Its two non-reply paths do not weaken this either: a disconnected or no-op bind settles inline, inside the caller's own frame (which `Bridge::detail::parkIfInFrame` exists to handle), and `cancelPending` settles the remainder from `~Bridge` itself, which is not a *concurrent* destructor. A -future backend that replied on its own transport thread would still reopen -morph#486. Giving `Bridge` an executor of its own — which would change the -window rather than merely move the decision — is **morph#588**, and is -deliberately not part of morph#571: the guarantee this surface makes structural -is a guarantee about *backends*, and for `Bridge`-mediated calls the delivery -thread remains what the backend chose. +future backend that replied on its own transport thread reopens morph#486 for +a `Bridge` constructed without a `bridgeExec`, and does not for one constructed +with a suitable one. That split is the whole of what morph#588 claims: the +guarantee this surface makes structural is a guarantee about *backends*, and +for `Bridge`-mediated calls the delivery thread is now the embedder's choice +rather than the backend's — a contract one embedder can satisfy, instead of one +every backend author must remember. `tests/test_backend_registration_surface.cpp` pins this: the backend settles from a thread that is asserted to be *not* the caller's, the caller's executor @@ -692,11 +704,12 @@ The executor those call sites name is **`exec::detail::inlineExecutor()`**, whic runs the continuation on the thread that settled it. That is deliberately the *old* delivery thread, so neither morph#568 nor morph#571 changes observable *threading*: `Bridge` owns no event loop and has no thread of its own to name. -Making it name a real one is the step that would turn the structural guarantee -into a behaviour change, and it belongs to whichever ticket gives `Bridge` such -an executor — see +morph#588 left that argument alone — an inline settle must still be delivered +inline, or `registerHandler()` stops being synchronous — and gave `Bridge` an +optional executor for the replies that arrive *after* the dispatch frame +instead. See [How the threading contract becomes structural](#how-the-threading-contract-becomes-structural) -and morph#588. +and [bridge.md](bridge.md), "The bridge's own executor". ## Error types diff --git a/docs/spec/core/bridge.md b/docs/spec/core/bridge.md index 8ef21f889..16717dca6 100644 --- a/docs/spec/core/bridge.md +++ b/docs/spec/core/bridge.md @@ -12,6 +12,7 @@ that only know action names at runtime. - [Architecture overview](#architecture-overview) - [`HandlerBinding`](#handlerbinding) - [`Bridge`](#bridge) + - [The bridge's own executor](#the-bridges-own-executor) - [`BridgeHandler`](#bridgehandlermodel) - [Registration readiness — `isBound()` / `whenBound()`](#registration-readiness--isbound--whenbound) - [`ActionExecuteRegistry`](#actionexecuteregistry) @@ -425,6 +426,62 @@ now gets (see [callback_scope.md](callback_scope.md)); it uses only the liveness half — it never calls `requestStop()`, so its tokens go inactive only when the `Bridge` is destroyed. +### The bridge's own executor + +`Bridge`'s constructor takes an optional second argument, `IExecutor* +bridgeExec` (morph#588). Every other completion in the framework is delivered +on an executor its caller named — `BridgeHandler` supplies `guiExec`, +`executeVia` takes a `cbExec`. The registrations the bridge issues *on its own +behalf* had no such executor, and its five dispatch sites +(`registerHandlerImpl`, `attachHandlerAsync`, `ensureBoundAsync`, +`assignHandlerPrimary`, `rebindThroughSurface`) named +`exec::detail::inlineExecutor()` instead: "deliver wherever the backend +settled", written as a value rather than as a sentence in a doc comment. + +**What it is used for, and what it is not.** Exactly one thing: a registration +reply that arrives *after* its dispatching frame has closed the +`detail::AsyncDispatchHandoff` window (`detail::deliverLate`). A reply that +settles while the dispatch call is still on the stack is parked by +`detail::parkIfInFrame` and published by the dispatching frame itself, on the +dispatching thread, whatever `bridgeExec` says. + +The `bindModel`/`promoteModel` calls therefore keep naming `inlineExecutor()`. +That is a decision, not an omission left over from morph#568, and two things +break if it is changed: + +- **`registerHandler()` stops being synchronous.** For every backend that binds + inline — `LocalBackend`, `SimulatedRemoteBackend`, any `kCallerMayBlock` + backend — the settle would become a task queued on `bridgeExec` rather than a + callback on this stack, so `claimHandoff` would find nothing parked and the + caller would get an unbound handler from a call that has always returned a + bound one. +- **A `kCallerMayBlock` backend deadlocks.** `detail::awaitHandoff` stops the + dispatching thread until the reply is parked. If the reply is instead a task + on `bridgeExec` and the dispatching thread *is* the executor's thread — a GUI + embedder passing its GUI executor, which is the intended use — nothing will + ever run that task. + +**What the caller must guarantee.** `bridgeExec` is borrowed: it must outlive +the bridge *and* every registration still in flight when the bridge is +destroyed, because a late reply can land after `~Bridge` (the same requirement +`BridgeHandler`'s `guiExec` already carries). + +**What it buys, stated exactly.** The morph#486 window in these callbacks is +"check `CallbackToken::active()`, then touch the bridge". It is closed only if +`bridgeExec` runs its tasks on a thread that cannot run `~Bridge` concurrently +— for a Qt embedder, the GUI thread that both owns the `Bridge` and pumps the +executor; the callback and the destructor are then two tasks on one thread and +cannot interleave. An executor on some *other* thread satisfies the type and +closes nothing. It makes nothing worse either: the callbacks' existing +`CallbackToken`/`detail::BridgeLifetime` gates are unchanged, and with the null +default the delivery is inline, byte for byte the pre-morph#588 behaviour. + +`tests/test_async_registration.cpp` pins all three halves: a late reply is +queued on the executor and publishes nothing until it is drained (restoring +inline delivery fails that case), an inline bind and a keyed attach still +publish before `registerHandler`/`attachHandler` returns even when the executor +never runs, and a bridge constructed without one behaves as it always did. + ## `BridgeHandler` RAII handle. Registers a `HandlerBinding` on construction, deregisters on @@ -948,7 +1005,7 @@ make teardown order-independent.) | Member | Signature | Notes | |---|---|---| -| ctor | `explicit Bridge(unique_ptr)` | Installs reconnect handler on the backend, then pushes the (initially empty) default session via `setSession`. | +| ctor | `explicit Bridge(unique_ptr, IExecutor* bridgeExec = nullptr)` | Installs reconnect handler on the backend, then pushes the (initially empty) default session via `setSession`. `bridgeExec` is where a registration reply that arrived after its dispatch frame is delivered; null (the default) delivers it inline, exactly as before morph#588. See [The bridge's own executor](#the-bridges-own-executor). | | dtor | `~Bridge()` | Clears the active backend's reconnect handler, then cancels all pending completions with `BridgeDestroyedError`. | | `registerHandler` | `shared_ptr registerHandler()` | Default factory. Dispatches `IBackend::bindModel`; see `backend.md`. | | `registerHandler(binding)` | `void registerHandler(const shared_ptr&)` | Pre-built binding. Same async-preferring behavior. | diff --git a/include/morph/core/bridge.hpp b/include/morph/core/bridge.hpp index 70839555e..61e9b8565 100644 --- a/include/morph/core/bridge.hpp +++ b/include/morph/core/bridge.hpp @@ -422,6 +422,54 @@ inline std::optional awaitHandoff(AsyncDispatchHandoff& handoff) return ParkedOutcome{.succeeded = handoff.succeeded, .modelId = handoff.modelId, .failure = handoff.failure}; } +/// @brief Delivers a registration continuation that arrived *after* its +/// dispatching frame closed the handoff window. +/// +/// The counterpart to `parkIfInFrame` returning `false`: nobody is left on the +/// dispatching stack to publish this outcome, so the callback publishes it +/// itself — and, until morph#588, published it on whichever thread the backend +/// happened to settle the `Completion` on, because the executor every dispatch +/// site names is `exec::detail::inlineExecutor()`. That is the thread the +/// morph#486 use-after-free is about: each of these callbacks asks "is the +/// `Bridge` still alive" and then touches it, and a `~Bridge` running +/// concurrently on another thread can land between the two steps. +/// +/// Routing only the late delivery through an executor the `Bridge` was given +/// closes that window structurally for an embedder whose executor runs tasks +/// on the thread that also runs `~Bridge`: the continuation and the destructor +/// are then two tasks on one thread and cannot interleave at all. The in-frame +/// case deliberately does not go through @p exec — see `Bridge`'s +/// `bridgeExec` constructor parameter for why posting it would turn +/// synchronous registration asynchronous and deadlock a caller that waits on +/// `awaitHandoff` from the executor's own thread. +/// +/// A template rather than a `std::function` parameter, and that is not +/// incidental: with a null @p exec the callable is invoked **in place**, so the +/// default path type-erases nothing and allocates nothing. Taking a +/// `std::function` would have put a heap allocation — sometimes a large one, +/// since these closures carry a primary key — on the path that existed before +/// morph#588, which `tests/test_async_registration.cpp`'s morph#108 +/// allocation-failure case detects by catching the wrong allocation. +/// +/// @tparam Action Callable of no arguments; convertible to `std::function` only +/// on the posting path. +/// @param exec Executor to deliver on. Borrowed, and may be null, which is +/// the default a `Bridge` constructed without one carries: a +/// null @p exec runs @p action inline, exactly where it ran +/// before morph#588. Non-null, it must outlive every in-flight +/// registration, because a reply can land after `~Bridge`. +/// @param action Work to run. Must be safe to run after `~Bridge` — every +/// caller here gates on a `CallbackToken` or a +/// `detail::BridgeLifetime` before touching the bridge. +template +void deliverLate(::morph::exec::IExecutor* exec, Action&& action) { + if (exec == nullptr) { + std::forward(action)(); + return; + } + exec->post(std::function{std::forward(action)}); +} + /// @brief The gate that makes "the `Bridge` is still there" and "call into it" /// a single, indivisible step. /// @@ -491,9 +539,53 @@ class Bridge { /// (e.g. `QtWebSocketBackend`) can ask the bridge to re-register every live /// handler against the freshly reconnected peer. /// + /// @par The bridge's own executor (morph#588) + /// Every other completion in the framework is delivered on an executor its + /// caller named — `BridgeHandler` supplies `guiExec`, `executeVia` takes a + /// `cbExec`. The registrations the bridge issues *on its own behalf* had no + /// such executor: their five dispatch sites name + /// `exec::detail::inlineExecutor()`, which is "deliver wherever the backend + /// settled" written as a value rather than as a sentence in a doc comment. + /// @p bridgeExec is where that decision now lives. + /// + /// It is used for **one** thing: a registration reply that arrives after + /// its dispatching frame has gone (`detail::deliverLate`). That is the only + /// case with a thread to choose — a reply that settles while the dispatch + /// call is still on the stack is parked in a + /// `detail::AsyncDispatchHandoff` and published by the dispatching frame + /// itself, on the dispatching thread, whatever @p bridgeExec says. + /// + /// The `bindModel`/`promoteModel` calls therefore keep naming + /// `inlineExecutor()`, and that is deliberate rather than an omission. + /// Naming a posting executor there would break two things at once: + /// `registerHandler()` would stop being synchronous for every backend that + /// binds inline (the settle would become a queued task, so the handler + /// would be unbound on return), and a `kCallerMayBlock` backend would + /// deadlock outright whenever the dispatching thread *is* the executor's + /// thread — `detail::awaitHandoff` would sit waiting for a task only that + /// same thread can run. A GUI embedder passing its GUI executor is exactly + /// that case. + /// + /// **What the caller must guarantee, and what it buys.** @p bridgeExec must + /// outlive this bridge and every registration still in flight when it is + /// destroyed, because a late reply can land after `~Bridge` (the same + /// requirement `BridgeHandler`'s `guiExec` already carries). The window + /// morph#486 describes is closed only if @p bridgeExec runs its tasks on a + /// thread that cannot run `~Bridge` concurrently — for a Qt embedder, the + /// GUI thread that both owns the `Bridge` and pumps the executor. Supplying + /// an executor on some *other* thread satisfies the type and does not close + /// the window; it is not made worse than the default either, since the + /// callbacks' existing `CallbackToken`/`detail::BridgeLifetime` gates are + /// unchanged. Left null — the default — delivery is inline and behaviour is + /// byte-for-byte what it was before morph#588. + /// /// @param backend Initial backend. Ownership is transferred. - explicit Bridge(std::unique_ptr<::morph::backend::detail::IBackend> backend) - : _backend{std::shared_ptr<::morph::backend::detail::IBackend>(std::move(backend))} { + /// @param bridgeExec Executor for late registration continuations, or null + /// (the default) to keep delivering them inline. + /// Borrowed: it must outlive this bridge. + explicit Bridge(std::unique_ptr<::morph::backend::detail::IBackend> backend, + ::morph::exec::IExecutor* bridgeExec MORPH_LIFETIMEBOUND = nullptr) + : _backend{std::shared_ptr<::morph::backend::detail::IBackend>(std::move(backend))}, _bridgeExec{bridgeExec} { installReconnectHandler(_backend); // A null initial backend is tolerated (see installReconnectHandler's own // null check just above) — there is nothing yet to stamp a session onto. @@ -696,68 +788,91 @@ class Bridge { auto const weakLiveness = _callbacks.token(); std::weak_ptr const weakBinding{binding}; auto handoff = std::make_shared(); - auto onAttached = [this, weakBackend, weakLiveness, weakBinding, primaryCopy, onDone, - handoff](::morph::exec::detail::ModelId newId) { + auto* const bridgeExec = _bridgeExec; + auto onAttached = [this, weakBackend, weakLiveness, weakBinding, primaryCopy, onDone, handoff, + bridgeExec](::morph::exec::detail::ModelId newId) mutable { if (detail::parkIfInFrame(*handoff, true, newId, nullptr)) { return; // Completed inline: the dispatching frame will finish this. } - // This check and the `this` touch below it are two steps -- the - // morph#486 shape. Closed not by a gate but by the thread the - // continuation is delivered on. Before morph#571 that was a - // prose contract on every backend author; now it is the executor - // this call site names -- today `inlineExecutor()`, which keeps - // the pre-morph#568 delivery thread, so the window is unchanged - // rather than closed (morph#588). Gating instead would block - // `~Bridge` behind `_attachMtx`, which `attachHandler` holds - // across a full `attachModel` round trip. See morph#489. - if (!weakLiveness.active()) { - return; // The Bridge is gone; publishing this id would be pointless. - } - auto strongBinding = weakBinding.lock(); - if (!strongBinding) { - return; // The BridgeHandler (and its binding) is gone. - } - std::exception_ptr failure; - { - // contextKey/primary are plain std::strings that every - // other site reads under `_attachMtx`; publishing them - // without it would be a data race, not just a stale - // read. (`registerHandlerImpl`'s read during - // registration is the one documented carve-out -- see - // its own comment, and morph#505.) - std::scoped_lock const guard{_attachMtx}; - auto pinned = weakBackend.lock(); - if (!pinned || pinned != loadBackend()) { - // A switchBackend() already moved past this attach - // (see registerHandlerImpl's identical guard) and - // its own re-registration loop already handled - // `binding` on the *new* backend -- applying this - // stale reply now would overwrite that with a - // dangling id from a backend nothing uses any - // more. Unlike registerHandlerImpl's fire-and- - // forget re-registration, a real execute() call is - // synchronously waiting on `onDone` here, so the - // stale reply must still be reported -- silently - // dropping it would hang that caller forever. - failure = std::make_exception_ptr( - std::runtime_error("attach reply arrived from a backend switchBackend() already replaced")); - } else { - try { - strongBinding->contextKey = primaryCopy; - strongBinding->primary = primaryCopy; - strongBinding->currentId.store(newId.v); - } catch (...) { - failure = std::current_exception(); + // Nobody is left on the dispatching stack, so this callback + // publishes the outcome itself -- on `bridgeExec` if the embedder + // named one, inline otherwise. See `detail::deliverLate`. + // + // `mutable`, so `primaryCopy` is *moved* into the body's closure + // rather than copied: this callback runs exactly once (one + // `Completion`, settled once), and copying the primary key here + // would put a fresh allocation on a path that had none before + // morph#588 -- which is both a cost and, for + // `tests/test_async_registration.cpp`'s morph#108 case, the wrong + // allocation for its injector to catch. `onDone` cannot be moved + // the same way: it is captured from a `const` reference parameter, + // so the capture itself is const. + detail::deliverLate(bridgeExec, [this, weakBackend, weakLiveness, weakBinding, + primaryCopy = std::move(primaryCopy), onDone, newId] { + // This check and the `this` touch below it are two steps -- + // the morph#486 shape. Closed not by a gate but by the thread + // this body runs on. Before morph#571 that was a prose + // contract on every backend author; morph#568 made it the + // executor the dispatch site names, which is + // `inlineExecutor()` and so left the window unchanged; + // morph#588 moved the choice here, where a non-null + // `bridgeExec` running `~Bridge`'s own thread closes it, and + // the null default keeps the pre-morph#588 thread exactly. + // Gating instead would block `~Bridge` behind `_attachMtx`, + // which `attachHandler` holds across a full `attachModel` + // round trip. See morph#489. + if (!weakLiveness.active()) { + return; // The Bridge is gone; publishing this id would be pointless. + } + auto strongBinding = weakBinding.lock(); + if (!strongBinding) { + return; // The BridgeHandler (and its binding) is gone. + } + std::exception_ptr failure; + { + // contextKey/primary are plain std::strings that every + // other site reads under `_attachMtx`; publishing them + // without it would be a data race, not just a stale + // read. (`registerHandlerImpl`'s read during + // registration is the one documented carve-out -- see + // its own comment, and morph#505.) + std::scoped_lock const guard{_attachMtx}; + auto pinned = weakBackend.lock(); + if (!pinned || pinned != loadBackend()) { + // A switchBackend() already moved past this attach + // (see registerHandlerImpl's identical guard) and + // its own re-registration loop already handled + // `binding` on the *new* backend -- applying this + // stale reply now would overwrite that with a + // dangling id from a backend nothing uses any + // more. Unlike registerHandlerImpl's fire-and- + // forget re-registration, a real execute() call is + // synchronously waiting on `onDone` here, so the + // stale reply must still be reported -- silently + // dropping it would hang that caller forever. + failure = std::make_exception_ptr(std::runtime_error( + "attach reply arrived from a backend switchBackend() already replaced")); + } else { + try { + strongBinding->contextKey = primaryCopy; + strongBinding->primary = primaryCopy; + strongBinding->currentId.store(newId.v); + } catch (...) { + failure = std::current_exception(); + } } } - } - onDone(failure); // Outside the lock -- see @par Locking. + onDone(failure); // Outside the lock -- see @par Locking. + }); }; - auto onFailed = [onDone, handoff](const std::exception_ptr& failure) { + auto onFailed = [onDone, handoff, bridgeExec](const std::exception_ptr& failure) { if (detail::parkIfInFrame(*handoff, false, {}, failure)) { return; } - onDone(failure); + // Reported on the bridge's executor for the same reason the + // success path is: `onDone` is the caller's continuation, and a + // bridge that named an executor wants it delivered there. + detail::deliverLate(bridgeExec, [onDone, failure] { onDone(failure); }); }; try { // The structural surface (`IBackend::bindModel`). A backend with a @@ -861,55 +976,60 @@ class Bridge { auto const weakLiveness = _callbacks.token(); std::weak_ptr const weakBinding{binding}; auto handoff = std::make_shared(); - auto onBound = [this, weakBackend, weakLiveness, weakBinding, onDone, - handoff](::morph::exec::detail::ModelId newId) { + auto* const bridgeExec = _bridgeExec; + auto onBound = [this, weakBackend, weakLiveness, weakBinding, onDone, handoff, + bridgeExec](::morph::exec::detail::ModelId newId) { if (detail::parkIfInFrame(*handoff, true, newId, nullptr)) { return; // Completed inline: the dispatching frame will finish this. } - // This check and the `this` touch below it are two steps -- the - // morph#486 shape. Closed not by a gate but by delivering on a - // thread that cannot run `~Bridge` concurrently. Before morph#571 - // that thread was the backend's choice, asked for in prose; now - // the `bindModel` call below names it as an executor -- today - // `inlineExecutor()`, which keeps the same delivery thread, so - // the window is unchanged rather than closed (morph#588). Gating - // instead would block `~Bridge` behind `_attachMtx`, which - // `attachHandler` holds across a full `attachModel` round trip. - // See morph#489. - if (!weakLiveness.active()) { - return; // The Bridge is gone; publishing this id would be pointless. - } - auto strongBinding = weakBinding.lock(); - if (!strongBinding) { - return; // The BridgeHandler (and its binding) is gone. - } - std::exception_ptr failure; - { - // Brief `_attachMtx` window purely to serialise this - // check against a concurrent switchBackend() (which - // takes the same lock) -- `currentId` itself is an - // atomic and needs no lock to store. - std::scoped_lock const guard{_attachMtx}; - auto pinned = weakBackend.lock(); - if (!pinned || pinned != loadBackend()) { - // See attachHandlerAsync's identical guard: a - // stale reply from a backend switchBackend() - // already replaced must still resolve `onDone` - // (a real execute() call is waiting), not be - // silently dropped. - failure = std::make_exception_ptr( - std::runtime_error("attach reply arrived from a backend switchBackend() already replaced")); - } else { - strongBinding->currentId.store(newId.v); + // Late: published by this callback, on the bridge's executor when + // it has one. `mutable` plus moved captures for the reason + // `attachHandlerAsync`'s identical shape spells out. See + // `detail::deliverLate`. + detail::deliverLate(bridgeExec, [this, weakBackend, weakLiveness, weakBinding, onDone, newId] { + // This check and the `this` touch below it are two steps -- + // the morph#486 shape. Closed not by a gate but by the thread + // this body runs on, which morph#588 made the bridge's own + // choice; the null default is the pre-morph#588 thread, + // whichever one the backend settled on, so the window is + // unchanged there rather than closed. Gating instead would + // block `~Bridge` behind `_attachMtx`, which `attachHandler` + // holds across a full `attachModel` round trip. See morph#489. + if (!weakLiveness.active()) { + return; // The Bridge is gone; publishing this id would be pointless. } - } - onDone(failure); + auto strongBinding = weakBinding.lock(); + if (!strongBinding) { + return; // The BridgeHandler (and its binding) is gone. + } + std::exception_ptr failure; + { + // Brief `_attachMtx` window purely to serialise this + // check against a concurrent switchBackend() (which + // takes the same lock) -- `currentId` itself is an + // atomic and needs no lock to store. + std::scoped_lock const guard{_attachMtx}; + auto pinned = weakBackend.lock(); + if (!pinned || pinned != loadBackend()) { + // See attachHandlerAsync's identical guard: a + // stale reply from a backend switchBackend() + // already replaced must still resolve `onDone` + // (a real execute() call is waiting), not be + // silently dropped. + failure = std::make_exception_ptr(std::runtime_error( + "attach reply arrived from a backend switchBackend() already replaced")); + } else { + strongBinding->currentId.store(newId.v); + } + } + onDone(failure); + }); }; - auto onFailed = [onDone, handoff](const std::exception_ptr& failure) { + auto onFailed = [onDone, handoff, bridgeExec](const std::exception_ptr& failure) { if (detail::parkIfInFrame(*handoff, false, {}, failure)) { return; } - onDone(failure); + detail::deliverLate(bridgeExec, [onDone, failure] { onDone(failure); }); }; try { // The structural surface; see `attachHandlerAsync`'s identical @@ -1008,15 +1128,16 @@ class Bridge { auto const weakLiveness = _callbacks.token(); std::weak_ptr<::morph::backend::detail::IBackend> const weakBackend{backend}; std::weak_ptr const weakBinding{binding}; - auto onPromoted = [this, weakLiveness, weakBackend, weakBinding, primary](::morph::exec::detail::ModelId) { + auto onPromoted = [this, weakLiveness, weakBackend, weakBinding, primary] { // This check and the `this` touch below it are two steps -- the - // morph#486 shape. Closed not by a gate but by delivering on a - // thread that cannot run `~Bridge` concurrently. Before morph#571 - // that thread was the backend's choice, asked for in prose; now - // the `promoteModel` call below names it as an executor -- today - // `inlineExecutor()`, which keeps the same delivery thread, so the - // window is unchanged rather than closed (morph#588). Gating - // instead would block `~Bridge` behind `_attachMtx`, which + // morph#486 shape. Closed not by a gate but by the thread this + // body runs on. Before morph#571 that thread was the backend's + // choice, asked for in prose; morph#568 made it the executor the + // `promoteModel` call names, which is `inlineExecutor()` and so + // left the window unchanged; morph#588 moved the choice to the + // bridge's own executor for a reply that arrives after this + // frame, and left the in-frame reply published by this frame. + // Gating instead would block `~Bridge` behind `_attachMtx`, which // `attachHandler` holds across a full `attachModel` round trip. // See morph#489. if (!weakLiveness.active()) { @@ -1051,19 +1172,50 @@ class Bridge { // The structural surface (`IBackend::promoteModel`). A backend with // no non-blocking promote settles the returned `Completion` from // inside this call, having run the same synchronous `assignPrimary` - // this replaces; `inlineExecutor()` then delivers `onPromoted` on - // this thread, exactly where the synchronous call used to publish. + // this replaces; `inlineExecutor()` then delivers the reply on this + // thread, and the `claimHandoff` below publishes it before this + // method returns -- exactly where the synchronous call used to + // publish, which `BridgeHandler::execute`'s `onResult` relies on ("the + // binding is already promoted by the time user code sees the result"). // Unlike that call, a failure is logged rather than thrown: this runs // inside the result `Completion`'s callback chain, where an escaping // exception is swallowed by `CompletionState` anyway, and // `promoteModel` reports through the `Completion` by contract. + // + // The handoff is what separates the two cases morph#588 treats + // differently, and is why this site has one at all: a reply that + // lands in this frame is published by this frame, while one that + // lands later goes to the bridge's executor. Without it the callback + // could not tell the two apart, and posting *both* would make an + // inline promote asynchronous. + auto handoff = std::make_shared(); + auto* const bridgeExec = _bridgeExec; auto completion = backend->promoteModel( ::morph::backend::detail::PromoteRequest{ .mid = ::morph::exec::detail::ModelId{raw}, .typeId = binding->typeId, .primary = primary}, ::morph::exec::detail::inlineExecutor()); - completion.then(onPromoted).onError([onFailed](const std::exception_ptr& failure) { - onFailed(detail::describeFailure(failure)); - }); + completion + .then([onPromoted, handoff, bridgeExec](::morph::exec::detail::ModelId newId) { + if (detail::parkIfInFrame(*handoff, true, newId, nullptr)) { + return; // Settled inline: the dispatching frame owns the outcome. + } + detail::deliverLate(bridgeExec, onPromoted); + }) + .onError([onFailed, handoff, bridgeExec](const std::exception_ptr& failure) mutable { + if (detail::parkIfInFrame(*handoff, false, {}, failure)) { + return; + } + detail::deliverLate(bridgeExec, [onFailed = std::move(onFailed), failure] { + onFailed(detail::describeFailure(failure)); + }); + }); + if (auto parked = detail::claimHandoff(*handoff)) { + if (parked->succeeded) { + onPromoted(); + } else { + onFailed(detail::describeFailure(parked->failure)); + } + } } /// @brief Returns @p binding's current primary key, or empty if unattached. @@ -1622,7 +1774,8 @@ class Bridge { // reaching this point means it was anyway. static_cast(holder); throw std::logic_error( - "Bridge::executeVia: localOp invoked in a MORPH_CLIENT_ONLY build -- LocalBackend must not be used"); + "Bridge::executeVia: localOp invoked in a MORPH_CLIENT_ONLY build -- LocalBackend must not be " + "used"); #else auto& model = holder.template into(); // Local mode has no client/server split, so this is the same execution @@ -1944,18 +2097,27 @@ class Bridge { .primary = {}, .current = {}}, ::morph::exec::detail::inlineExecutor()); + auto* const bridgeExec = _bridgeExec; completion - .then([onRegistered, handoff](::morph::exec::detail::ModelId newId) { + .then([onRegistered, handoff, bridgeExec](::morph::exec::detail::ModelId newId) mutable { if (detail::parkIfInFrame(*handoff, true, newId, nullptr)) { return; } - onRegistered(newId); + // Only reachable for a `kCallerMustNotBlock` backend, whose + // reply lands after this frame gave up waiting: exactly the + // delivery morph#588 gives the bridge's executor. The + // in-frame outcome below is published by this frame instead, + // on this thread, whatever executor the bridge holds. + detail::deliverLate(bridgeExec, + [onRegistered = std::move(onRegistered), newId] { onRegistered(newId); }); }) - .onError([onFailed, handoff](const std::exception_ptr& failure) { + .onError([onFailed, handoff, bridgeExec](const std::exception_ptr& failure) mutable { if (detail::parkIfInFrame(*handoff, false, {}, failure)) { return; } - onFailed(detail::describeFailure(failure)); + detail::deliverLate(bridgeExec, [onFailed = std::move(onFailed), failure] { + onFailed(detail::describeFailure(failure)); + }); }); // `registerHandler` is a synchronous entry point: its caller // constructs a `BridgeHandler` and uses it on the next line, and @@ -2122,18 +2284,25 @@ class Bridge { .primary = binding->shared ? binding->primary : std::string{}, .current = {}}, ::morph::exec::detail::inlineExecutor()); + auto* const bridgeExec = _bridgeExec; completion - .then([onBound, handoff](::morph::exec::detail::ModelId newId) { + .then([onBound, handoff, bridgeExec](::morph::exec::detail::ModelId newId) mutable { if (detail::parkIfInFrame(*handoff, true, newId, nullptr)) { return; // Settled inline: the dispatching frame owns the outcome. } - onBound(newId); + // A deferred reply, so both callers have long since released + // `_mtx`/`_attachMtx` that this continuation re-takes; it is + // also the delivery whose thread morph#588 lets the bridge + // choose. See `detail::deliverLate`. + detail::deliverLate(bridgeExec, [onBound = std::move(onBound), newId] { onBound(newId); }); }) - .onError([onFailed, handoff](const std::exception_ptr& failure) { + .onError([onFailed, handoff, bridgeExec](const std::exception_ptr& failure) mutable { if (detail::parkIfInFrame(*handoff, false, {}, failure)) { return; } - onFailed(detail::describeFailure(failure)); + detail::deliverLate(bridgeExec, [onFailed = std::move(onFailed), failure] { + onFailed(detail::describeFailure(failure)); + }); }); return mayBlock ? detail::awaitHandoff(*handoff) : detail::claimHandoff(*handoff); } @@ -2395,6 +2564,15 @@ class Bridge { mutable std::mutex _backendMtx; std::shared_ptr<::morph::backend::detail::IBackend> _backend; + // Where a registration reply that missed its dispatching frame is + // delivered; null means "inline, on the settling thread", which is what + // every site did before morph#588. Read once per dispatch and captured by + // value into the continuation, never read *from* the callback: the + // callback can run after `~Bridge`, and `this->_bridgeExec` would then be + // a read of destroyed memory ahead of the very gate that exists to + // prevent one. Borrowed for the bridge's whole life plus the lifetime of + // anything still in flight -- see the constructor's `bridgeExec`. + ::morph::exec::IExecutor* _bridgeExec = nullptr; std::mutex _mtx; std::vector> _handlers; // Guards the shared-handler attach/register/assign path (ensureBound, diff --git a/tests/test_async_registration.cpp b/tests/test_async_registration.cpp index b3e0dc1fc..67c0b4658 100644 --- a/tests/test_async_registration.cpp +++ b/tests/test_async_registration.cpp @@ -2410,3 +2410,126 @@ TEST_CASE( CHECK(binding->currentId.load() != 0U); CHECK(binding->primary.empty()); } + +// ── morph#588: the bridge's own executor for late registration replies ────── +// +// Every Bridge dispatch site names `inlineExecutor()` on the `bindModel`/ +// `promoteModel` call, so a reply that arrives after the dispatching frame has +// gone used to be published on whichever thread the backend settled it on -- +// the morph#486 thread. `Bridge`'s optional `bridgeExec` constructor argument +// is where that decision lives now. The three cases below pin the three halves +// of the contract: a late reply goes through the executor, an in-frame reply +// does not, and no executor means exactly the old behaviour. + +namespace { + +// Queues everything posted to it and runs nothing until drain() is called. +// Deliberately never runs a task inside post(): a test that drains explicitly +// can tell "the bridge posted this" from "the bridge ran it inline", which an +// executor that ran tasks eagerly could not. +class QueuedExecutor : public morph::exec::IExecutor { +public: + void post(std::function task) override { + std::scoped_lock const lock{_mtx}; + _queued.push_back(std::move(task)); + } + + [[nodiscard]] std::size_t queued() const { + std::scoped_lock const lock{_mtx}; + return _queued.size(); + } + + // Runs every queued task on the calling thread, outside the mutex: a task + // is free to post another one. + std::size_t drain() { + std::vector> ready; + { + std::scoped_lock const lock{_mtx}; + ready.swap(_queued); + } + for (auto& task : ready) { + task(); + } + return ready.size(); + } + +private: + mutable std::mutex _mtx; + std::vector> _queued; +}; + +} // namespace + +TEST_CASE("Bridge(bridgeExec): a registration reply that misses its dispatch frame is published on that executor", + "[bridge][registration][issue588]") { + QueuedExecutor bridgeExec; + auto backend = std::make_unique(); + auto* rawBackend = backend.get(); + morph::bridge::Bridge bridge{std::move(backend), &bridgeExec}; + + auto binding = std::make_shared(); + binding->typeId = "AR_Model"; + binding->modelFactory = [] { return morph::model::detail::ModelFactory::create(); }; + bridge.registerHandler(binding); + REQUIRE(rawBackend->pendingCount() == 1); + REQUIRE(binding->currentId.load() == 0U); + REQUIRE(bridgeExec.queued() == 0); + + // The reply lands. Before morph#588 this published the id right here, on + // completeNext()'s own thread; now it is a task on the bridge's executor + // and nothing is published until that executor runs it. Restoring inline + // delivery makes the next two lines fail rather than merely not-prove. + rawBackend->completeNext(); + CHECK(binding->currentId.load() == 0U); + REQUIRE(bridgeExec.queued() == 1); + + CHECK(bridgeExec.drain() == 1); + CHECK(binding->currentId.load() != 0U); +} + +TEST_CASE("Bridge(bridgeExec): a bind that settles inside the dispatch frame is still published by that frame", + "[bridge][registration][issue588]") { + // The other half of the contract, and the reason the `bindModel` call + // keeps naming `inlineExecutor()`: `registerHandler` is synchronous for a + // backend that binds inline, and must stay so even when the bridge holds + // an executor that will never run (a GUI executor whose loop is not + // pumping, or -- for `awaitHandoff` -- the very thread doing the waiting). + // This executor never drains, so anything routed through it is lost. + QueuedExecutor neverDrained; + morph::exec::ThreadPoolExecutor pool{2}; + morph::bridge::Bridge bridge{std::make_unique(pool), &neverDrained}; + + auto binding = std::make_shared(); + binding->typeId = "AR_KeyedModel"; + binding->modelFactory = [] { return morph::model::detail::ModelFactory::create(); }; + bridge.registerHandler(binding); + + CHECK(binding->currentId.load() != 0U); + CHECK(neverDrained.queued() == 0); + + // Same for the keyed attach path, which publishes the primary before it + // returns so a caller reading `bindingPrimary()` on the next line sees it. + bridge.template attachHandler(binding, "k-588"); + CHECK(bridge.bindingPrimary(binding) == "k-588"); + CHECK(neverDrained.queued() == 0); +} + +TEST_CASE("Bridge(): with no executor, a late registration reply is delivered inline, as before morph#588", + "[bridge][registration][issue588]") { + // The default. `Bridge`'s new argument must compose (framework invariant + // 2), so omitting it has to leave the pre-morph#588 behaviour byte for + // byte: the reply publishes on completeNext()'s own thread, with no + // executor anywhere in the path. + auto backend = std::make_unique(); + auto* rawBackend = backend.get(); + morph::bridge::Bridge bridge{std::move(backend)}; + + auto binding = std::make_shared(); + binding->typeId = "AR_Model"; + binding->modelFactory = [] { return morph::model::detail::ModelFactory::create(); }; + bridge.registerHandler(binding); + REQUIRE(binding->currentId.load() == 0U); + + rawBackend->completeNext(); + CHECK(binding->currentId.load() != 0U); +} From da9deb526bb94701cb0539517db280f9fa0356cb Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Mon, 21 Sep 2026 09:29:42 +0200 Subject: [PATCH 2/5] tests/bench: re-measure what one local dispatch allocates, and commit the instrument that measured it (refs #572) morph#572's scope is a number -- "19 allocations, two CompletionStates, two strings per dispatch", taken on master @ 4017228d. morph#639, morph#649 and morph#654 then rewrote the dispatch path, so the first thing the ticket needs is not a fix but a re-measurement. This commit is that, plus the program that produced it, so the next person re-checks the figure in one command instead of rebuilding it from a description. Measured on f24e225a (this branch's base), x86-64 Linux, GCC 16.2.1 / libstdc++, -O2 -DNDEBUG, `Ping{int} -> Pong{int}` through `LocalBackend` on a one-thread pool, 50 warm-up calls excluded, 200 counted round trips: local execute round-trips : 200 heap allocations total : 4165 (20.82 per call) bytes allocated total : 398912 (1994.6 per call) allocations in one steady-state call: 21 The total held: 20.8 against 19.2, and 1995 bytes against 1983. The attribution in the ticket did not, and the fix it proposes is scoped by the attribution rather than by the total -- which is why this lands as a measurement and not as a fix. The per-line breakdown (backtrace-attributed at -O2 -g, symbolised with addr2line) and what it does to the ticket's "2-3 allocations per call" target are on morph#572; the ticket is handed back rather than built against a number this commit disproves in detail. `morph_bench_alloc` is an instrument, not a control. It is not registered with ctest and asserts nothing unless `--budget=` is passed: an allocation count is standard-library and allocator specific, so a ceiling that is right on libstdc++ is wrong on libc++, and a gate nobody can satisfy everywhere is worse than no gate. Both arms of the optional budget were exercised -- `--budget=5` exits 1 and prints the overrun, `--budget=40` exits 0 -- so the one assertion it can make is known to be able to fail. It is a separate binary from `morph_bench` deliberately: it replaces the global `operator new`/`delete`, which is process-wide and would perturb every other measurement in a shared binary. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01VptDWG2fKr2vBnLSJcgzgW --- docs/spec/testing_strategy.md | 28 +++ tests/bench/CMakeLists.txt | 24 ++ tests/bench/bench_dispatch_allocations.cpp | 246 +++++++++++++++++++++ 3 files changed, 298 insertions(+) create mode 100644 tests/bench/bench_dispatch_allocations.cpp diff --git a/docs/spec/testing_strategy.md b/docs/spec/testing_strategy.md index 26771ee14..9fa099885 100644 --- a/docs/spec/testing_strategy.md +++ b/docs/spec/testing_strategy.md @@ -221,6 +221,34 @@ file scope, not inside the file's anonymous namespace with its other local helpers — Glaze's reflection needs external linkage to mangle the type name, the same requirement `tests/fuzz/`'s harness fixtures document. +### Allocation census (`bench_dispatch_allocations.cpp`, target `morph_bench_alloc`) + +A second binary under the same option, and deliberately **not** a second case +in `morph_bench`: it replaces the global `operator new`/`delete`, which is +process-wide and would perturb any other measurement sharing the binary. It +counts the heap allocations one `Ping -> Pong` round trip costs through +`LocalBackend` — 50 warm-up calls excluded, every call waited out, no JSON and +no socket — and prints the total, the bytes, and (with `--attribute`) the size +of every allocation in one steady-state call. + +It exists because morph#572 is scoped by a number that three later pull +requests invalidated, and re-deriving such a number from a prose description of +how it was once taken is how a fix ends up built against a figure nobody +re-checked. + +**It is an instrument, not a control, and the distinction is the point here.** +It is not registered with ctest and asserts nothing unless `--budget=` is +passed: an allocation count is standard-library and allocator specific, so a +ceiling that holds on libstdc++ would be wrong on libc++ or MSVC, and a gate +that cannot be satisfied everywhere is worse than none. A green run of it +proves nothing; the number it prints is the output. Turning it into a CI gate +means giving it a per-toolchain budget first. + +Measured with it on `f24e225a`, x86-64 Linux, GCC 16.2.1 / libstdc++, `-O2 +-DNDEBUG`: **20.9 allocations and 1995 bytes per local round trip**, 21 +allocations in the recorded steady-state call. See morph#572 for the +per-line attribution and what it says about that ticket's scope. + ## Adversarial cross-socket run (`tests/qt/test_qt_websocket_adversarial.cpp`) Built under the existing `MORPH_BUILD_QT=ON` option (no new option — it's one diff --git a/tests/bench/CMakeLists.txt b/tests/bench/CMakeLists.txt index 92fc0e06b..fee40126b 100644 --- a/tests/bench/CMakeLists.txt +++ b/tests/bench/CMakeLists.txt @@ -19,3 +19,27 @@ endif() include(Catch) catch_discover_tests(morph_bench DISCOVERY_MODE PRE_TEST PROPERTIES TIMEOUT 60 LABELS "bench") + +# Allocation census for one local dispatch (morph#572). A binary of its own, +# not a case in morph_bench, because it replaces the global operator new: the +# replacement is process-wide and would perturb every other measurement in the +# same binary. Deliberately **not** registered with ctest -- it asserts nothing +# by default (an allocation count is toolchain-specific, so no ceiling is right +# everywhere), and a test that cannot fail is noise in a suite. Run it by hand: +# ./build/tests/bench/morph_bench_alloc [--attribute] +# See the file's own header comment for the method, and +# docs/spec/testing_strategy.md. +add_executable(morph_bench_alloc bench_dispatch_allocations.cpp) +target_link_libraries(morph_bench_alloc PRIVATE morph::morph) +apply_warnings(morph_bench_alloc) +# -Wmismatched-new-delete pairs a `new` the compiler inlined with the +# `std::free` inside the *replaced* `operator delete` and calls it a mismatch. +# It is not one: replacing the global allocation functions is this program's +# entire purpose, and forwarding them to malloc/free is how such a replacement +# is written. Scoped to this one target rather than relaxed anywhere else. +if(CMAKE_CXX_COMPILER_ID MATCHES "GNU|Clang") + target_compile_options(morph_bench_alloc PRIVATE -Wno-mismatched-new-delete) +endif() +if(DEFINED AF_SANITIZER) + apply_sanitizers(morph_bench_alloc ${AF_SANITIZER}) +endif() diff --git a/tests/bench/bench_dispatch_allocations.cpp b/tests/bench/bench_dispatch_allocations.cpp new file mode 100644 index 000000000..4b67bfd6b --- /dev/null +++ b/tests/bench/bench_dispatch_allocations.cpp @@ -0,0 +1,246 @@ +// SPDX-License-Identifier: Apache-2.0 + +// Allocation census for one local `execute` round trip, for morph#572. +// +// morph#572 is a performance ticket whose scope is set by a number -- "19 +// allocations, two CompletionStates, two strings per dispatch", measured on +// master @ 4017228d. Three pull requests then rewrote the dispatch path +// (morph#639, morph#649, morph#654), which is exactly the situation where a +// fix gets built against a figure nobody has re-checked. This program exists +// so the figure can be re-checked in one command instead of being rebuilt from +// a description of how it was once obtained. +// +// **It is an instrument, not a gate.** It counts and prints; it fails nothing +// unless `--budget=` is passed, and it is not registered with ctest. An +// allocation count is standard-library, allocator and platform specific -- a +// ceiling that is right here would be wrong on libc++ or MSVC -- so turning it +// into a CI control needs a per-toolchain budget that nothing currently has. +// Do not cite a green run of this as evidence of anything: read the number it +// prints. +// +// Method, so a later run is comparable with an earlier one: +// +// * Global `operator new`/`delete` are replaced. Counting is off until the +// warm-up is done, so process start-up, model registration and the first +// 50 dispatches are excluded. +// * The workload is the smallest one there is: `Ping{int}` -> `Pong{int}` +// through `LocalBackend` on a one-thread pool, with an inline callback +// executor. No JSON, no socket. What is left is framework overhead. +// * Every call is waited out before the next one starts, so the count is per +// completed round trip rather than per queued dispatch. +// * `--attribute` additionally prints the size of every allocation made +// during one steady-state call. For per-*line* attribution, build with +// `-g -rdynamic` and add a `backtrace()` to `note()`; that is how the +// breakdown in morph#572's re-measurement comment was produced. +// +// Build: `-DMORPH_BUILD_LOAD_TESTS=ON`, target `morph_bench_alloc`. See +// docs/spec/testing_strategy.md. + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +namespace { + +// Per-allocation sizes recorded for the one call `--attribute` inspects. A +// fixed array rather than a vector because the recorder runs *inside* +// `operator new` and must not allocate. +constexpr std::size_t kMaxRecorded = 512; + +// The counters live in a function-local static rather than at namespace scope: +// mutable globals are a finding, and a first-use-constructed aggregate of +// atomics allocates nothing, which the allocation hook below requires. +struct Census { + std::atomic counting{false}; + std::atomic recording{false}; + std::atomic allocations{0}; + std::atomic bytes{0}; + std::atomic recorded{0}; + std::array recordedSizes{}; +}; + +Census& census() { + static Census state; + return state; +} + +void note(std::size_t size) { + Census& state = census(); + if (!state.counting.load(std::memory_order_relaxed)) { + return; + } + state.allocations.fetch_add(1, std::memory_order_relaxed); + state.bytes.fetch_add(size, std::memory_order_relaxed); + if (state.recording.load(std::memory_order_relaxed)) { + std::size_t const slot = state.recorded.fetch_add(1, std::memory_order_relaxed); + if (slot < kMaxRecorded) { + state.recordedSizes.at(slot) = size; + } + } +} + +} // namespace + +// Replacing the global allocation functions is the whole measurement, and it +// is why this is a binary of its own rather than a case in `morph_bench`: the +// replacement is process-wide, so it would perturb any other benchmark sharing +// the binary. +// NOLINTNEXTLINE(misc-new-delete-overloads) +void* operator new(std::size_t size) { + std::size_t const request = size == 0 ? 1 : size; + note(request); + // NOLINTNEXTLINE(cppcoreguidelines-no-malloc,cppcoreguidelines-owning-memory) + void* block = std::malloc(request); + if (block == nullptr) { + throw std::bad_alloc{}; + } + return block; +} + +// NOLINTNEXTLINE(misc-new-delete-overloads) +void* operator new[](std::size_t size) { return ::operator new(size); } + +void operator delete(void* block) noexcept { + // NOLINTNEXTLINE(cppcoreguidelines-no-malloc,cppcoreguidelines-owning-memory) + std::free(block); +} +void operator delete(void* block, std::size_t /*size*/) noexcept { ::operator delete(block); } +void operator delete[](void* block) noexcept { ::operator delete(block); } +void operator delete[](void* block, std::size_t /*size*/) noexcept { ::operator delete(block); } + +// External linkage so glaze's reflection can mangle the type names -- the same +// convention every other morph benchmark fixture model follows. +struct BenchAllocPing { + int x = 0; +}; +struct BenchAllocPong { + int y = 0; +}; +struct BenchAllocModel { + BenchAllocPong execute(const BenchAllocPing& action) { return BenchAllocPong{.y = action.x * 2}; } +}; + +BRIDGE_REGISTER_MODEL(BenchAllocModel, "BenchAlloc_Model") +BRIDGE_REGISTER_ACTION(BenchAllocModel, BenchAllocPing, "BenchAlloc_Ping") + +namespace { + +// Runs each task on the calling thread, so the callback executor contributes +// no allocations of its own and what is counted is the dispatch path. +class InlineCallbackExecutor : public ::morph::exec::IExecutor { +public: + void post(std::function task) override { + if (task) { + task(); + } + } +}; + +constexpr int kWarmup = 50; +constexpr int kCalls = 200; + +int run(bool attribute, double budget) { + Census& state = census(); + ::morph::exec::ThreadPoolExecutor pool{1}; + InlineCallbackExecutor callbackExec; + ::morph::bridge::Bridge bridge{std::make_unique<::morph::backend::LocalBackend>(pool)}; + ::morph::bridge::BridgeHandler handler{bridge, &callbackExec}; + + std::atomic settled{0}; + auto roundTrip = [&handler, &settled](int value) { + int const target = settled.load(std::memory_order_relaxed) + 1; + handler.execute(BenchAllocPing{.x = value}) + .then([&settled](const BenchAllocPong&) { settled.fetch_add(1, std::memory_order_relaxed); }) + .onError([&settled](const std::exception_ptr&) { settled.fetch_add(1, std::memory_order_relaxed); }); + while (settled.load(std::memory_order_acquire) < target) { + std::this_thread::yield(); + } + }; + + for (int i = 0; i < kWarmup; ++i) { + roundTrip(i); + } + + state.counting.store(true, std::memory_order_relaxed); + for (int i = 0; i < kCalls; ++i) { + // The recorded call is one steady-state call from the middle of the + // run, not the first: the first still pays for whatever the warm-up + // left cold. + state.recording.store(attribute && i == kCalls / 2, std::memory_order_relaxed); + roundTrip(i); + } + state.recording.store(false, std::memory_order_relaxed); + state.counting.store(false, std::memory_order_relaxed); + + auto const totalAllocations = state.allocations.load(); + auto const totalBytes = state.bytes.load(); + double const perCall = static_cast(totalAllocations) / kCalls; + std::cout << std::format("local execute round-trips : {}\n", kCalls) + << std::format("heap allocations total : {} ({:.2f} per call)\n", totalAllocations, perCall) + << std::format("bytes allocated total : {} ({:.1f} per call)\n", totalBytes, + static_cast(totalBytes) / kCalls); + + if (attribute) { + auto const count = state.recorded.load(); + std::cout << std::format("\nallocations in one steady-state call: {}\n", count); + for (std::size_t i = 0; i < count && i < kMaxRecorded; ++i) { + std::cout << std::format(" #{:2} {} bytes\n", i, state.recordedSizes.at(i)); + } + } + + if (budget > 0.0) { + if (perCall > budget) { + std::cout << std::format("FAIL: {:.2f} allocations per call exceeds --budget={:.2f}\n", perCall, budget); + return 1; + } + std::cout << std::format("ok: {:.2f} allocations per call within --budget={:.2f}\n", perCall, budget); + } + return 0; +} + +} // namespace + +int main(int argc, char** argv) { + // Everything is inside the handler, argument parsing included: `main` must + // not let an exception escape, and both `std::stod` and `std::format` can + // throw. A benchmark that aborts on a typo'd flag would be a poor + // instrument. + try { + bool attribute = false; + double budget = 0.0; + std::span const args{argv, static_cast(argc)}; + for (std::size_t i = 1; i < args.size(); ++i) { + std::string_view const arg{args[i]}; + if (arg == "--attribute") { + attribute = true; + } else if (arg.starts_with("--budget=")) { + budget = std::stod(std::string{arg.substr(std::string_view{"--budget="}.size())}); + } else { + std::cout << "usage: morph_bench_alloc [--attribute] [--budget=]\n"; + return 2; + } + } + return run(attribute, budget); + } catch (const std::exception& exc) { + std::cout << "FAIL: " << exc.what() << "\n"; + return 1; + } catch (...) { + std::cout << "FAIL: unknown exception\n"; + return 1; + } +} From b64c1e9a12c5cd6d6630c35a057e3b692e50e5f0 Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Mon, 21 Sep 2026 11:00:18 +0200 Subject: [PATCH 3/5] core: rename the four init-captures that shadow the structured binding they move from The WASM leg compiles `-Weverything -Werror` and rejected morph#588's four new init-captures: include/morph/core/bridge.hpp:2112:38: error: declaration shadows a structured binding [-Werror,-Wshadow] (and 2118, 2297, 2303). All four are `[onX = std::move(onX), ...]` inside a lambda that captured `onX` from `auto [onRegistered, onFailed] = makeBindCallbacks(...)`. Renamed to `registered`/`bound`/`failed`. ## The rename cannot change which object is moved from An init-capture's initializer is looked up in the *enclosing* scope, not in the capture being declared ([expr.prim.lambda.capture]/6), so `[x = std::move(x)]` and `[y = std::move(x)]` both move from the enclosing `x`. Confirmed rather than asserted, with a tracked type on clang 22.1.8 -- the two forms print byte-identical traces: A: shadowing form [f = std::move(f)] B: renamed form [g = std::move(f)] copy of OUTER(gen1) copy of OUTER(gen1) move -> gen1 move -> gen1 after move, enclosing f.tag= after move, enclosing f.tag= call tag=OUTER gen=1 call tag=OUTER gen=1 So no use-after-move is introduced: in both forms the inner callable holds the live value and the outer lambda's copy is left moved-from, unused thereafter. The fifth `[onFailed = std::move(onFailed), ...]` (line 1208, in `assignHandlerPrimary`) is deliberately left alone: its `onFailed` is a plain local, not a structured binding, and neither toolchain diagnoses it. ## Why the GCC-only local build was clean, and why a clang build is too This is not a GCC/clang split. Local clang 22.1.8 files this diagnostic under `-Wshadow-uncaptured-local`, which `cmake/compiler_options.cmake`'s Clang branch suppresses tree-wide; emsdk 3.1.56's older clang files the identical construct under plain `-Wshadow`, which nothing suppresses. Same flag list on both legs -- both pass `-Wno-shadow-uncaptured-local` -- so the WASM leg is the only one in CI that enforces this diagnostic class. Filed separately rather than folded in. Verified by dropping that one suppression from the project's own flags for a TU that includes bridge.hpp (clang 22.1.8, everything else identical): before: shadows a local variable: 18 shadows a structured binding: 4 after: shadows a local variable: 18 The same 18 pre-existing local-variable shadows (backend.hpp, completion.hpp, registry.hpp, callback_scope.hpp) before and after, all four structured-binding shadows gone, none introduced. Not verified: no Emscripten toolchain is available here, so the WASM leg itself was not run. The above is a local proxy for the same diagnostic on a different clang, not a build of that leg. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01VptDWG2fKr2vBnLSJcgzgW --- include/morph/core/bridge.hpp | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/include/morph/core/bridge.hpp b/include/morph/core/bridge.hpp index 61e9b8565..fc48ca9bb 100644 --- a/include/morph/core/bridge.hpp +++ b/include/morph/core/bridge.hpp @@ -2108,16 +2108,14 @@ class Bridge { // delivery morph#588 gives the bridge's executor. The // in-frame outcome below is published by this frame instead, // on this thread, whatever executor the bridge holds. - detail::deliverLate(bridgeExec, - [onRegistered = std::move(onRegistered), newId] { onRegistered(newId); }); + detail::deliverLate(bridgeExec, [registered = std::move(onRegistered), newId] { registered(newId); }); }) .onError([onFailed, handoff, bridgeExec](const std::exception_ptr& failure) mutable { if (detail::parkIfInFrame(*handoff, false, {}, failure)) { return; } - detail::deliverLate(bridgeExec, [onFailed = std::move(onFailed), failure] { - onFailed(detail::describeFailure(failure)); - }); + detail::deliverLate( + bridgeExec, [failed = std::move(onFailed), failure] { failed(detail::describeFailure(failure)); }); }); // `registerHandler` is a synchronous entry point: its caller // constructs a `BridgeHandler` and uses it on the next line, and @@ -2294,15 +2292,14 @@ class Bridge { // `_mtx`/`_attachMtx` that this continuation re-takes; it is // also the delivery whose thread morph#588 lets the bridge // choose. See `detail::deliverLate`. - detail::deliverLate(bridgeExec, [onBound = std::move(onBound), newId] { onBound(newId); }); + detail::deliverLate(bridgeExec, [bound = std::move(onBound), newId] { bound(newId); }); }) .onError([onFailed, handoff, bridgeExec](const std::exception_ptr& failure) mutable { if (detail::parkIfInFrame(*handoff, false, {}, failure)) { return; } - detail::deliverLate(bridgeExec, [onFailed = std::move(onFailed), failure] { - onFailed(detail::describeFailure(failure)); - }); + detail::deliverLate( + bridgeExec, [failed = std::move(onFailed), failure] { failed(detail::describeFailure(failure)); }); }); return mayBlock ? detail::awaitHandoff(*handoff) : detail::claimHandoff(*handoff); } From 194c2d12dace5ec27e6fdfbfe049f78b20137a82 Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Mon, 21 Sep 2026 11:00:55 +0200 Subject: [PATCH 4/5] core: collapse the two late bind-reply bodies into one helper, clearing attachHandlerAsync's complexity gate clang-tidy rejected morph#588's executor branch: include/morph/core/bridge.hpp:776:10: error: function 'attachHandlerAsync' has cognitive complexity of 28 (threshold 25) [readability-function-cognitive-complexity,-warnings-as-errors] Extraction, following morph#615's remedy in this same file rather than raising the threshold or adding a NOLINT -- the number is a gate, and widening a gate to fit the change is the failure mode AGENTS.md names. ## Why this split rather than a narrower one The cheapest fix would lift `attachHandlerAsync`'s deferred body alone. What is extracted instead is the shape it *shares* with `ensureBoundAsync`, which is morph#615's own move (three continuation pairs -> one `makeBindCallbacks`). Both functions dispatch through `IBackend::bindModel`, both park an in-frame reply, and when the reply instead lands out of frame both run the same four steps: is the `Bridge` alive, is the binding alive, is the answering backend still the active one, and report through `onDone` either way. They differed only in what a successful reply publishes. That is now `publishLateBindReply`'s `Publish` parameter, and two ~30-line bodies -- including two copies of the stale-backend reasoning and of its error string -- became one. `assignHandlerPrimary`'s continuation is deliberately *not* routed through it. It has no `onDone`: a stale reply there is dropped silently because no caller is waiting on it. Folding it in would mean giving the helper a second mode, and the two behaviours differ on purpose. `Publish` is a template parameter, not a `std::function`, so the late path type-erases and allocates nothing -- the same constraint morph#108's OOM-injection case already imposed on `deliverLate`. ## Measured, on this revision, with clang-tidy 22.1.8 (CI's pinned major) before after attachHandlerAsync 28 13 (threshold 25) ensureBoundAsync 20 9 publishLateBindReply - 7 assignHandlerPrimary 19 19 (untouched) `ensureBoundAsync` at 20 was five under the gate and next in line; this takes both off it. The command that reported the 28 reports nothing on bridge.hpp now, and `clang-tidy-diff` against the base finds nothing on any changed line. ## Behaviour Unchanged on both paths, with one narrow exception worth stating: the helper wraps the publish in try/catch for `attachHandlerAsync`'s two `std::string` assignments, so `ensureBoundAsync` -- which previously had none -- now stores its atomic `currentId` inside one. An integral `store` is `noexcept`, so the handler is unreachable there. Lock scope, lock ordering and the "onDone outside the lock" contract are the same at both sites. Verified: clang 22.1.8 `-Weverything -Werror` build of `morph_tests` and `morph_bench_alloc` clean; suite green (1561 cases, 22971 assertions, 1 deliberate expected failure); Doxygen `WARN_AS_ERROR=FAIL_ON_WARNINGS` clean; `clang-format --dry-run -Werror` clean on both touched files. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01VptDWG2fKr2vBnLSJcgzgW --- docs/spec/core/bridge.md | 12 +- include/morph/core/bridge.hpp | 204 +++++++++++++++++++--------------- 2 files changed, 125 insertions(+), 91 deletions(-) diff --git a/docs/spec/core/bridge.md b/docs/spec/core/bridge.md index 16717dca6..5901527b7 100644 --- a/docs/spec/core/bridge.md +++ b/docs/spec/core/bridge.md @@ -896,7 +896,17 @@ never reach the callback body, `attachHandlerAsync`'s out-of-frame success callback is free to re-acquire `_attachMtx` for the two `std::string` fields it publishes (`HandlerBinding::contextKey`/`primary`, which every other reader takes that lock for); `ensureBoundAsync`'s publishes only the atomic -`currentId` and needs no lock at all. +`currentId` and needs no lock to store it. + +Both out-of-frame callbacks reach those publishes through one private helper, +`publishLateBindReply`, which holds the four steps they share — the liveness +check, the binding lock, the stale-backend comparison under `_attachMtx`, and +the single `onDone` outside it — and takes what to publish as a callable. It is +a template rather than a `std::function` parameter so the late path +type-erases and allocates nothing. `assignHandlerPrimary`'s continuation +deliberately does not use it: having no `onDone`, it drops a stale reply +silently instead of reporting it, and that difference is intended rather than +incidental. `whenBound()` synchronises on the *binding's* `registrationMtx`, never on a `Bridge` mutex, and never holds it across a callback: the resolver swaps the diff --git a/include/morph/core/bridge.hpp b/include/morph/core/bridge.hpp index fc48ca9bb..8e70bb948 100644 --- a/include/morph/core/bridge.hpp +++ b/include/morph/core/bridge.hpp @@ -710,6 +710,100 @@ class Bridge { binding->currentId.store(newId.v); } + /// @brief Applies an out-of-frame bind reply to a binding and reports the + /// outcome, under the guards every such reply needs. + /// + /// The one shape `attachHandlerAsync` and `ensureBoundAsync` share, and + /// the reason it is here rather than written twice: both dispatch through + /// `IBackend::bindModel`, both park an in-frame reply, and both are left + /// with the same four-step job when the reply instead arrives *after* the + /// dispatching frame has gone -- is the `Bridge` still there, is the + /// binding still there, is the backend that answered still the active one, + /// and report through `onDone` either way. They differ only in what a + /// successful reply publishes, which is what @p publish is. + /// + /// `assignHandlerPrimary`'s continuation deliberately does **not** go + /// through here. It has no `onDone` -- a stale reply there is dropped + /// silently, because no caller is waiting on it -- so routing it through a + /// helper whose contract is "report exactly once" would mean giving that + /// helper a second mode, and the two behaviours are different on purpose. + /// + /// @par The liveness check and the `this` touch are two steps + /// That is the morph#486 shape, and it is closed not by a gate but by the + /// thread this body runs on. Before morph#571 that was a prose contract on + /// every backend author; morph#568 made it the executor the dispatch site + /// names, which is `inlineExecutor()` and so left the window unchanged; + /// morph#588 moved the choice to the bridge's own executor, where a + /// non-null one running `~Bridge`'s thread closes it, and the null default + /// keeps the pre-morph#588 thread exactly. Gating instead would block + /// `~Bridge` behind `_attachMtx`, which `attachHandler` holds across a full + /// `attachModel` round trip. See morph#489. + /// + /// @par Locking + /// @p publish runs under `_attachMtx`, because `contextKey`/`primary` are + /// plain `std::string`s that every other site reads under that lock -- + /// publishing them without it would be a data race, not merely a stale + /// read. (`registerHandlerImpl`'s read during registration is the one + /// documented carve-out; see its own comment, and morph#505.) @p onDone is + /// invoked **after** the lock is released, on every path that invokes it at + /// all: what a caller does from inside it is dispatch the action, which can + /// re-enter `_attachMtx` through `assignHandlerPrimary`. + /// + /// @tparam Publish Callable taking `detail::HandlerBinding&`. Runs under + /// `_attachMtx` on success only. May throw: the exception + /// is caught and reported through @p onDone rather than + /// escaping onto an executor's thread. + /// @param liveness Token for the `Bridge`; nothing is touched once it + /// reports inactive. + /// @param weakBinding Binding the reply belongs to. A expired one means the + /// `BridgeHandler` is gone and there is nothing to + /// publish to, so @p onDone is not called. + /// @param weakBackend Backend the reply came from, compared against the + /// active one before anything is published. + /// @param onDone Caller's continuation: `nullptr` on success, a + /// non-null `exception_ptr` on failure. Invoked at most + /// once, outside `_attachMtx`. + /// @param publish Applies the reply to the binding. See @p Publish. + template + void publishLateBindReply(const ::morph::async::CallbackToken& liveness, + const std::weak_ptr& weakBinding, + const std::weak_ptr<::morph::backend::detail::IBackend>& weakBackend, + const std::function& onDone, Publish&& publish) { + if (!liveness.active()) { + return; // The Bridge is gone; publishing this id would be pointless. + } + auto strongBinding = weakBinding.lock(); + if (!strongBinding) { + return; // The BridgeHandler (and its binding) is gone. + } + std::exception_ptr failure; + { + std::scoped_lock const guard{_attachMtx}; + auto pinned = weakBackend.lock(); + if (!pinned || pinned != loadBackend()) { + // A switchBackend() already moved past this reply (see + // registerHandlerImpl's identical guard) and its own + // re-registration loop already handled this binding on the + // *new* backend -- applying the stale reply now would + // overwrite that with a dangling id from a backend nothing + // uses any more. Unlike registerHandlerImpl's fire-and-forget + // re-registration, a real execute() call is synchronously + // waiting on `onDone` here, so the stale reply must still be + // reported -- silently dropping it would hang that caller + // forever. + failure = std::make_exception_ptr( + std::runtime_error("attach reply arrived from a backend switchBackend() already replaced")); + } else { + try { + std::forward(publish)(*strongBinding); + } catch (...) { + failure = std::current_exception(); + } + } + } + onDone(failure); // Outside the lock -- see @par Locking. + } + /// @brief Async counterpart to `attachHandler`: dispatches the attach and /// invokes @p onDone once attached (or failed), instead of blocking. /// @@ -809,60 +903,17 @@ class Bridge { // so the capture itself is const. detail::deliverLate(bridgeExec, [this, weakBackend, weakLiveness, weakBinding, primaryCopy = std::move(primaryCopy), onDone, newId] { - // This check and the `this` touch below it are two steps -- - // the morph#486 shape. Closed not by a gate but by the thread - // this body runs on. Before morph#571 that was a prose - // contract on every backend author; morph#568 made it the - // executor the dispatch site names, which is - // `inlineExecutor()` and so left the window unchanged; - // morph#588 moved the choice here, where a non-null - // `bridgeExec` running `~Bridge`'s own thread closes it, and - // the null default keeps the pre-morph#588 thread exactly. - // Gating instead would block `~Bridge` behind `_attachMtx`, - // which `attachHandler` holds across a full `attachModel` - // round trip. See morph#489. - if (!weakLiveness.active()) { - return; // The Bridge is gone; publishing this id would be pointless. - } - auto strongBinding = weakBinding.lock(); - if (!strongBinding) { - return; // The BridgeHandler (and its binding) is gone. - } - std::exception_ptr failure; - { - // contextKey/primary are plain std::strings that every - // other site reads under `_attachMtx`; publishing them - // without it would be a data race, not just a stale - // read. (`registerHandlerImpl`'s read during - // registration is the one documented carve-out -- see - // its own comment, and morph#505.) - std::scoped_lock const guard{_attachMtx}; - auto pinned = weakBackend.lock(); - if (!pinned || pinned != loadBackend()) { - // A switchBackend() already moved past this attach - // (see registerHandlerImpl's identical guard) and - // its own re-registration loop already handled - // `binding` on the *new* backend -- applying this - // stale reply now would overwrite that with a - // dangling id from a backend nothing uses any - // more. Unlike registerHandlerImpl's fire-and- - // forget re-registration, a real execute() call is - // synchronously waiting on `onDone` here, so the - // stale reply must still be reported -- silently - // dropping it would hang that caller forever. - failure = std::make_exception_ptr(std::runtime_error( - "attach reply arrived from a backend switchBackend() already replaced")); - } else { - try { - strongBinding->contextKey = primaryCopy; - strongBinding->primary = primaryCopy; - strongBinding->currentId.store(newId.v); - } catch (...) { - failure = std::current_exception(); - } - } - } - onDone(failure); // Outside the lock -- see @par Locking. + // Guards, locking and the morph#486 reasoning all live in + // `publishLateBindReply`, which `ensureBoundAsync` shares. + // What is this site's own is the three fields a successful + // attach publishes -- and that they can throw, which is why + // the helper wraps this in a try/catch. + publishLateBindReply(weakLiveness, weakBinding, weakBackend, onDone, + [&primaryCopy, newId](detail::HandlerBinding& target) { + target.contextKey = primaryCopy; + target.primary = primaryCopy; + target.currentId.store(newId.v); + }); }); }; auto onFailed = [onDone, handoff, bridgeExec](const std::exception_ptr& failure) { @@ -987,42 +1038,15 @@ class Bridge { // `attachHandlerAsync`'s identical shape spells out. See // `detail::deliverLate`. detail::deliverLate(bridgeExec, [this, weakBackend, weakLiveness, weakBinding, onDone, newId] { - // This check and the `this` touch below it are two steps -- - // the morph#486 shape. Closed not by a gate but by the thread - // this body runs on, which morph#588 made the bridge's own - // choice; the null default is the pre-morph#588 thread, - // whichever one the backend settled on, so the window is - // unchanged there rather than closed. Gating instead would - // block `~Bridge` behind `_attachMtx`, which `attachHandler` - // holds across a full `attachModel` round trip. See morph#489. - if (!weakLiveness.active()) { - return; // The Bridge is gone; publishing this id would be pointless. - } - auto strongBinding = weakBinding.lock(); - if (!strongBinding) { - return; // The BridgeHandler (and its binding) is gone. - } - std::exception_ptr failure; - { - // Brief `_attachMtx` window purely to serialise this - // check against a concurrent switchBackend() (which - // takes the same lock) -- `currentId` itself is an - // atomic and needs no lock to store. - std::scoped_lock const guard{_attachMtx}; - auto pinned = weakBackend.lock(); - if (!pinned || pinned != loadBackend()) { - // See attachHandlerAsync's identical guard: a - // stale reply from a backend switchBackend() - // already replaced must still resolve `onDone` - // (a real execute() call is waiting), not be - // silently dropped. - failure = std::make_exception_ptr(std::runtime_error( - "attach reply arrived from a backend switchBackend() already replaced")); - } else { - strongBinding->currentId.store(newId.v); - } - } - onDone(failure); + // Same helper, same guards, as `attachHandlerAsync`. This + // site's difference is the whole of what it publishes: + // `currentId` alone, which is a `std::atomic` and so needs + // neither `_attachMtx` of its own nor the helper's try/catch + // (an integral `store` is `noexcept`). It still runs under the + // lock the helper takes, which is what serialises it against a + // concurrent `switchBackend()`. + publishLateBindReply(weakLiveness, weakBinding, weakBackend, onDone, + [newId](detail::HandlerBinding& target) { target.currentId.store(newId.v); }); }); }; auto onFailed = [onDone, handoff, bridgeExec](const std::exception_ptr& failure) { From 7b7e052a535a3e6b72e583e3d08ed514e11a35a3 Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Mon, 21 Sep 2026 11:01:06 +0200 Subject: [PATCH 5/5] scripts: repoint the two bridge.hpp branch-allowlist citations this branch moved The `Linux / clang-coverage` leg was red, and not for the reason its 40m57s runtime suggested. All 2926 tests passed and no floor was breached (`include/morph/core` 94.90% against a 94% floor). The whole failure was `scripts/check_branch_coverage.py`'s allowlist audit: error: include/morph/core/bridge.hpp:1557 has moved to line 1709. The text still matches, so nothing is wrong with the disposition -- update the `line` hint. error: include/morph/core/bridge.hpp:1679 is allowlisted by a source line that appears 3 times (lines [1832, 1856, 1945]), and none of them is 1679, so which one is meant is not decidable. Make the entry unambiguous. Both dispositions are still correct; only the `line` hints had drifted, because this branch inserted lines above them. Repointed against this branch's final tree (after the two commits before this one, which moved them again): 1557 -> 1733, 1679 -> 1856. ## Resolving the ambiguous one by reading, not by taking the first match `if (deadlineHandle && schedulerRef) {` occurs three times, all in `executeVia`: the `catch` block that undoes `_pendingCalls` before rethrowing, the `.then()` disarm, and the `.onError()` disarm. Three independent lines of evidence agree on the first: 1. The entry's own reason says so in as many words -- "This entry is specifically the exception-path use of the guard (the `catch` block that undoes `_pendingCalls` and cancels the deadline before rethrowing)" -- and then explicitly disclaims the other two as "a different guard on a different, reachable arm". 2. Read directly: line 1856 is that catch block. 3. On master (7ab4c7a9), where the gate is green, hint 1679 is an exact match on the first of the three occurrences, and the other two sit at the offsets the reason names. Note the resolver accepts any hint that matches one of the occurrences, so "all 22 entries resolve" is *not* evidence that the right one was picked -- hence the three checks above. The gate's other direction (the resolved line must still be partial in the LCOV report) would catch a wrong pick, and that half needs a coverage run this change did not make. Two bare line numbers inside the reason prose (`bridge.hpp:1557`, and "lines 1703, 1792" for the other two occurrences) were already stale and are removed rather than re-stated: the file's own header comment says a comment citing a bare line number is the defect this repository has found three times, and the structural naming beside them ("the entry above", "the `.then()`/`.onError()` continuations") identifies them unambiguously without rotting. Verified: `check_branch_coverage.py`'s own `resolve_allowlist_source_line` resolves all 22 entries cleanly against this tree. Not verified: no local coverage build was run, so the partial-line half of the audit, and the subsystem floors on this branch's code, are unmeasured here. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01VptDWG2fKr2vBnLSJcgzgW --- scripts/branch_partial_allowlist.json | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/scripts/branch_partial_allowlist.json b/scripts/branch_partial_allowlist.json index c4912ee36..c8db3ae2f 100644 --- a/scripts/branch_partial_allowlist.json +++ b/scripts/branch_partial_allowlist.json @@ -98,15 +98,15 @@ }, { "file": "include/morph/core/bridge.hpp", - "line": 1557, + "line": 1733, "source": "if (_executeDeadline.count() > 0 && _timeoutScheduler) {", "reason": "Unreachable by construction (core audit finding B6). `setExecuteDeadline` (this file) is the only writer of both `_executeDeadline` and `_timeoutScheduler`, and always creates `_timeoutScheduler` in the same call that sets `_executeDeadline` positive (`_executeDeadline = deadline; if (_executeDeadline.count() > 0 && !_timeoutScheduler) { _timeoutScheduler = std::make_shared<...>(); }`, both under `_executeDeadlineMtx`); nothing anywhere resets `_timeoutScheduler` back to null -- the class's own doc comment on `setExecuteDeadline` says so explicitly (\"setting the deadline back to 0 stops new calls from arming it but does not tear the thread down\"). So `_executeDeadline > 0 && !_timeoutScheduler` cannot happen at this line once any positive deadline has ever been set." }, { "file": "include/morph/core/bridge.hpp", - "line": 1679, + "line": 1856, "source": "if (deadlineHandle && schedulerRef) {", - "reason": "Unreachable by construction, same joint-assignment shape as B6 above (core audit finding B11, reclassified (a)->(b) on review). `deadlineHandle` and `schedulerRef` are assigned together, a few lines above this one in `executeVia`, only inside `if (_executeDeadline.count() > 0 && _timeoutScheduler) { schedulerRef = _timeoutScheduler; ... }` (see the bridge.hpp:1557 entry above) -- there is no path that sets `deadlineHandle` without also having set `schedulerRef` from the same non-null `_timeoutScheduler` in the same conditional. So `schedulerRef` null while `deadlineHandle` is non-null cannot occur; the only theoretically-open arm this compound condition has is structurally impossible. This entry is specifically the exception-path use of the guard (the `catch` block that undoes `_pendingCalls` and cancels the deadline before rethrowing). Two more textually-identical `if (deadlineHandle && schedulerRef)` guards exist further down, in the `.then()`/`.onError()` continuations (lines 1703, 1792) -- present on master too, this PR only shifted their line numbers -- which are a different guard on a different, reachable arm (schedulerRef going null between capture and a callback that can run after `~Bridge()`) and are not covered by this disposition." + "reason": "Unreachable by construction, same joint-assignment shape as B6 above (core audit finding B11, reclassified (a)->(b) on review). `deadlineHandle` and `schedulerRef` are assigned together, a few lines above this one in `executeVia`, only inside `if (_executeDeadline.count() > 0 && _timeoutScheduler) { schedulerRef = _timeoutScheduler; ... }` (see the entry above) -- there is no path that sets `deadlineHandle` without also having set `schedulerRef` from the same non-null `_timeoutScheduler` in the same conditional. So `schedulerRef` null while `deadlineHandle` is non-null cannot occur; the only theoretically-open arm this compound condition has is structurally impossible. This entry is specifically the exception-path use of the guard (the `catch` block that undoes `_pendingCalls` and cancels the deadline before rethrowing). Two more textually-identical `if (deadlineHandle && schedulerRef)` guards exist further down, in the `.then()`/`.onError()` continuations -- present on master too -- which are a different guard on a different, reachable arm (schedulerRef going null between capture and a callback that can run after `~Bridge()`) and are not covered by this disposition." }, { "file": "include/morph/core/remote.hpp",