From d5b79594d328ef35ace946c5446cfcb2aafd11a9 Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Sat, 19 Sep 2026 10:11:36 +0200 Subject: [PATCH 01/11] core: name "deliver where the producer settled" as an executor `Bridge` is about to reach `IBackend::bindModel`/`promoteModel`, which take the executor their continuation is delivered on. `Bridge` owns no event loop, so the only executor it can honestly name is the one that reproduces what the four `*Async` verbs did: run the continuation on whichever thread the backend settled the reply on. That was previously a rule stated in a doc comment and checkable by nothing. `exec::detail::InlineExecutor` makes it a value a call site produces, which is the whole of what morph#567 moved -- not who is safe, but who decides. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01DbGrZGkr2WqcAs2DJyMym6 --- include/morph/core/executor.hpp | 51 +++++++++++++++++++++++++++++++++ 1 file changed, 51 insertions(+) diff --git a/include/morph/core/executor.hpp b/include/morph/core/executor.hpp index f505a7be9..b324efe11 100644 --- a/include/morph/core/executor.hpp +++ b/include/morph/core/executor.hpp @@ -229,4 +229,55 @@ class MainThreadExecutor : public IExecutor { std::queue> _q; }; +namespace detail { + +/// @brief Executor that runs each posted task on the posting thread, at once. +/// +/// "Deliver wherever the producer settled", expressed as an executor rather +/// than as a rule nobody can check. `Bridge` names it at the structural +/// registration surface (`IBackend::bindModel`/`promoteModel`, see +/// `docs/spec/core/backend.md`) because `Bridge` owns no thread of its own: it +/// has no event loop to post a registration continuation to, and the four +/// legacy `*Async` verbs it is replacing delivered their callbacks on exactly +/// this thread — whichever one the backend settled the reply on. Naming that +/// choice at the call site is the point of the change: the *caller* now decides +/// where a registration continuation runs, and can be changed to decide +/// differently without touching a single backend. +/// +/// @warning Not a general-purpose executor. Posting to it re-enters the caller, +/// so a handler that takes a lock the posting frame already holds +/// self-deadlocks. Every `Bridge` site that names it either releases +/// its locks first or parks the outcome through +/// `detail::AsyncDispatchHandoff` (see `Bridge::attachHandlerAsync`'s +/// `@par Locking`). Application code that wants "run it now" should +/// call the function instead of posting it. +// NOLINTNEXTLINE(cppcoreguidelines-special-member-functions) +class InlineExecutor : public IExecutor { +public: + /// @brief Runs @p task immediately, on the calling thread. + /// + /// An empty @p task is ignored rather than invoked, since `std::function`'s + /// empty state is a legal value to move around and calling it is undefined. + /// @param task Callable to execute. + void post(std::function task) override { + if (task) { + task(); + } + } +}; + +/// @brief The process-wide `InlineExecutor`. +/// +/// A function-local static, so it is constructed on first use and outlives every +/// `Completion` built against it — which is exactly what `Completion`'s "the +/// executor must outlive the completion" requirement asks of a caller that has +/// no executor of its own to name. +/// @return Reference to the shared inline executor. +inline IExecutor& inlineExecutor() { + static InlineExecutor executor; + return executor; +} + +} // namespace detail + } // namespace morph::exec From 721de997782886a67b741fb36a8b24f0e7433819 Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Sat, 19 Sep 2026 10:11:42 +0200 Subject: [PATCH 02/11] core: let Bridge's four dispatch sites fall back to bindModel, not a blocking verb Each of `registerHandlerImpl`, `attachHandlerAsync`, `ensureBoundAsync` and `assignHandlerPrimary` offered the backend a legacy `*Async` verb and, when it answered `false`, ran the synchronous verb inline. The second path is now `IBackend::bindModel`/`promoteModel` instead, so the path *count* is unchanged -- morph#571 deletes the first branch and leaves one. This is what makes morph#568 possible at all: `Bridge` is the only framework caller of the four verbs, so a backend that stops overriding them would otherwise be routed straight back into the blocking call the verbs existed to avoid. Behaviour is preserved deliberately, not incidentally: - The executor named is `inlineExecutor()`, so continuations still run on the thread the backend settles on. A blocking backend therefore settles inside the dispatch frame with `_attachMtx` held -- exactly what `AsyncDispatchHandoff` already existed for, so both branches park and the dispatching frame publishes under the lock it owns and reports once it is gone. - Failures carry the original `exception_ptr` to `onDone` rather than a `runtime_error` rebuilt from `what()`, which is what the synchronous fallback they replace did and what the legacy string channel could not do. - `registerHandlerImpl` parks a rejection that arrives inside its own frame and rethrows it: a `BridgeHandler` constructor that could not bind must not return as if it had. One named behaviour change: `assignHandlerPrimary`'s blocking branch used to let `assignPrimary` throw out of a `Completion` handler, where `CompletionState` swallowed and logged it. It now logs directly, because `promoteModel` reports through its `Completion` by contract. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01DbGrZGkr2WqcAs2DJyMym6 --- include/morph/core/bridge.hpp | 736 +++++++++++++++++++--------------- 1 file changed, 412 insertions(+), 324 deletions(-) diff --git a/include/morph/core/bridge.hpp b/include/morph/core/bridge.hpp index 7eac63cf3..12962c030 100644 --- a/include/morph/core/bridge.hpp +++ b/include/morph/core/bridge.hpp @@ -207,12 +207,12 @@ struct HandlerBinding { /// @brief Registration-settled seam (see `Bridge::whenBound`). /// /// `registrationInFlight` is `true` from just *before* `registerHandlerImpl` - /// calls `IBackend::registerModelAsync` until that call's - /// `onRegistered`/`onError` callback resolves. It is set unconditionally on - /// every path, the synchronous fallback included — see the comment at the - /// assignment for why it must be set before the backend call rather than - /// after. The fallback does not *leave* it set: it resolves the waiters - /// (and clears the flag) before returning. `whenBound()` + /// calls the backend until that call's continuation resolves. It is set + /// unconditionally on every path, the blocking one included — see the + /// comment at the assignment for why it must be set before the backend call + /// rather than after. A backend that settles inside the dispatch frame does + /// not *leave* it set: the continuation runs before `registerHandlerImpl` + /// returns and resolves the waiters (clearing the flag). `whenBound()` /// checks it to distinguish "an async reply is coming, queue a waiter" /// from "nothing is in flight, resolve false now". `registrationWaiters` /// holds callbacks queued by `whenBound()` while `registrationInFlight` is @@ -227,6 +227,28 @@ struct HandlerBinding { std::vector, std::function>> registrationWaiters; }; +/// @brief Renders @p failure as the diagnostic string a log line wants. +/// +/// The `Completion` surface carries an `exception_ptr`; the one remaining place +/// that needs text rather than a rethrowable failure is a log message. Kept +/// here rather than inline so "what does a rejected control call read like" +/// has one answer. +/// +/// @param failure Rejection to describe; may be null. +/// @return `what()` for a `std::exception`, or a generic stand-in otherwise. +inline std::string describeFailure(const std::exception_ptr& failure) { + if (!failure) { + return "unknown error"; + } + try { + std::rethrow_exception(failure); + } catch (const std::exception& exc) { + return exc.what(); + } catch (...) { + return "unknown error"; + } +} + /// @brief Outcome a backend's inline completion parked for its dispatcher. struct ParkedOutcome { /// @brief `true` when the parked outcome is a success. @@ -510,36 +532,39 @@ class Bridge { binding->currentId.store(newId.v); } - /// @brief Async counterpart to `attachHandler`: prefers the backend's - /// `attachModelAsync` when available, invoking @p onDone once - /// attached (or failed) instead of blocking. + /// @brief Async counterpart to `attachHandler`: dispatches the attach and + /// invokes @p onDone once attached (or failed), instead of blocking. /// - /// Falls back to the synchronous `attachHandler` body (and calls @p onDone - /// immediately, from this thread) when the backend offers no async - /// path — so a caller that always goes through this method behaves - /// identically to calling `attachHandler` directly, on every backend - /// that has not opted in to `attachModelAsync`. + /// Reaches the backend through `IBackend::bindModel` — the structural + /// registration surface — unless the backend still overrides the legacy + /// `attachModelAsync`, which is offered first and which morph#571 removes. + /// Either way there is exactly one dispatch and exactly one continuation: a + /// backend with no non-blocking attach settles inside the `bindModel` call, + /// having blocked for the same round trip the synchronous `attachHandler` + /// would have, and @p onDone is then invoked from this thread before this + /// method returns — so a caller that always goes through this method + /// behaves identically to calling `attachHandler` directly on such a + /// backend. /// /// @par Locking - /// `_attachMtx` is held around the guard check, the async branch's - /// *dispatch*, and the synchronous branch's own state mutation — matching + /// `_attachMtx` is held around the guard check and the *dispatch* — matching /// `attachHandler`'s existing lock scope — but is **released before /// @p onDone is ever invoked**, on every path, unconditionally. That is not /// a nicety: what `execute()` does from inside @p onDone is dispatch the /// action, and a result-keyed dispatch promotes its binding through /// `assignHandlerPrimary`, which takes `_attachMtx` itself. Invoking /// @p onDone under the lock therefore self-deadlocks the moment the - /// completion is delivered on the calling thread — which is exactly what - /// the synchronous fallback below does, and what an inline executor does - /// for every callback. This is `registerHandlerImpl`'s existing rule ("the - /// backend call must not run under `_mtx`") applied to `_attachMtx`. - /// - /// The guarantee holds even for a backend that completes its callback - /// *inline*, from inside `attachModelAsync` itself, while this frame still - /// holds the lock: such a callback parks its outcome in a - /// `detail::AsyncDispatchHandoff` and returns without acting, and this frame - /// applies it after the dispatch call has returned and the lock is gone. - /// See that struct's doc comment. + /// completion is delivered on the calling thread — which is exactly what a + /// blocking backend's `bindModel` does, since the executor this call site + /// names runs the continuation inline. This is `registerHandlerImpl`'s + /// existing rule ("the backend call must not run under `_mtx`") applied to + /// `_attachMtx`. + /// + /// The guarantee holds even for a backend that settles *inline*, from inside + /// the dispatch call itself, while this frame still holds the lock: such a + /// continuation parks its outcome in a `detail::AsyncDispatchHandoff` and + /// returns without acting, and this frame applies it after the dispatch call + /// has returned and the lock is gone. See that struct's doc comment. /// /// An out-of-frame success callback re-acquires `_attachMtx` for the two /// `std::string` fields it publishes (`contextKey`/`primary`, which @@ -581,83 +606,109 @@ class Bridge { } auto const previous = ::morph::exec::detail::ModelId{binding->currentId.load()}; auto backend = loadBackend(); - auto primaryCopy = primary; + auto primaryCopy = std::move(primary); std::weak_ptr<::morph::backend::detail::IBackend> const weakBackend{backend}; auto const weakLiveness = _callbacks.token(); std::weak_ptr const weakBinding{binding}; auto handoff = std::make_shared(); - bool started = false; + auto onAttached = [this, weakBackend, weakLiveness, weakBinding, primaryCopy, onDone, + handoff](::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 + // `IBackend::registerModelAsync`'s threading contract (see its + // doc comment): an overriding backend must deliver `*Async` + // replies on a thread that cannot run `~Bridge` concurrently. + // 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. + }; + auto onFailed = [onDone, handoff](const std::exception_ptr& failure) { + if (detail::parkIfInFrame(*handoff, false, {}, failure)) { + return; + } + onDone(failure); + }; try { - started = backend->attachModelAsync( + bool const started = backend->attachModelAsync( binding->typeId, binding->modelFactory, {.contextKey = primaryCopy, .primary = primaryCopy}, previous, - [this, weakBackend, weakLiveness, weakBinding, primaryCopy, onDone, - handoff](::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 - // `IBackend::registerModelAsync`'s threading contract (see its - // doc comment): an overriding backend must deliver `*Async` - // replies on a thread that cannot run `~Bridge` concurrently. - // 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, handoff](const std::string& message) { - auto failure = std::make_exception_ptr(std::runtime_error(message)); - if (detail::parkIfInFrame(*handoff, false, {}, failure)) { - return; - } - onDone(failure); + onAttached, [onFailed](const std::string& message) { + onFailed(std::make_exception_ptr(std::runtime_error(message))); }); + if (!started) { + // The structural surface (`IBackend::bindModel`). A backend + // with a genuinely non-blocking attach settles the returned + // `Completion` when its reply lands; one without settles it + // from inside this call, having blocked exactly as the + // synchronous `attachModel` this replaces did. Either way the + // continuation exists, so there is no third path below. + // + // `inlineExecutor()` because that is where the continuation ran + // before: on whichever thread the backend settled the reply on + // (`IBackend::registerModelAsync`'s prose contract). What is + // new is that this call site *names* it; see + // `exec::detail::InlineExecutor`. An inline settle therefore + // reaches `onAttached`/`onFailed` while `_attachMtx` is still + // held, which is precisely the case `handoff` exists for. + auto completion = + backend->bindModel(::morph::backend::detail::BindRequest{.typeId = binding->typeId, + .factory = binding->modelFactory, + .contextKey = primaryCopy, + .primary = primaryCopy, + .current = previous}, + ::morph::exec::detail::inlineExecutor()); + completion.then(onAttached).onError(onFailed); + } } catch (...) { - // The backend's own dispatch call can throw synchronously (e.g. - // QtWebSocketBackend::attachModelAsync's wire::encode() failing - // before send) -- report it like any other failure instead of - // letting it escape execute()'s documented never-throws contract. + // A backend's own dispatch call can throw synchronously (e.g. a + // `wire::encode()` failing before send) -- report it like any other + // failure instead of letting it escape execute()'s documented + // never-throws contract. `bindModel` rejects rather than throws, so + // this now guards only the legacy `attachModelAsync` branch. lock.unlock(); onDone(std::current_exception()); return; @@ -682,24 +733,8 @@ class Bridge { onDone(failure); return; } - if (started) { - return; - } - // No async path on this backend: run the identical synchronous attach - // `attachHandler` would have run, under the same lock, then report the - // outcome only once the lock is gone (see @par Locking above). - std::exception_ptr failure; - try { - auto newId = backend->attachModel(binding->typeId, binding->modelFactory, - {.contextKey = primary, .primary = primary}, previous); - binding->contextKey = primary; - binding->primary = std::move(primary); - binding->currentId.store(newId.v); - } catch (...) { - failure = std::current_exception(); - } - lock.unlock(); - onDone(failure); + // Nothing parked: the reply is still to come, and `onAttached`/ + // `onFailed` will deliver it themselves once it does. } /// @brief Gives @p binding an anonymous instance if it does not have one yet. @@ -746,60 +781,77 @@ class Bridge { auto const weakLiveness = _callbacks.token(); std::weak_ptr const weakBinding{binding}; auto handoff = std::make_shared(); - bool started = false; + auto onBound = [this, weakBackend, weakLiveness, weakBinding, onDone, + handoff](::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: the legacy + // `*Async` branch below leaves that to the backend (see + // `IBackend::registerModelAsync`'s doc comment), the `bindModel` + // branch names it as an executor. 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); + }; + auto onFailed = [onDone, handoff](const std::exception_ptr& failure) { + if (detail::parkIfInFrame(*handoff, false, {}, failure)) { + return; + } + onDone(failure); + }; try { - started = backend->registerModelSharedAsync( - binding->typeId, binding->modelFactory, {.contextKey = binding->contextKey, .primary = {}}, - [this, weakBackend, weakLiveness, weakBinding, onDone, handoff](::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 - // `IBackend::registerModelAsync`'s threading contract (see its - // doc comment): an overriding backend must deliver `*Async` - // replies on a thread that cannot run `~Bridge` concurrently. - // 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); - }, - [onDone, handoff](const std::string& message) { - auto failure = std::make_exception_ptr(std::runtime_error(message)); - if (detail::parkIfInFrame(*handoff, false, {}, failure)) { - return; - } - onDone(failure); + bool const started = backend->registerModelSharedAsync( + binding->typeId, binding->modelFactory, {.contextKey = binding->contextKey, .primary = {}}, onBound, + [onFailed](const std::string& message) { + onFailed(std::make_exception_ptr(std::runtime_error(message))); }); + if (!started) { + // The structural surface; see `attachHandlerAsync`'s identical + // branch for why `inlineExecutor()` is the executor this call + // site names. An empty `primary` with a zero `current` is the + // request shape that means `registerModelShared` -- the very + // verb the synchronous `ensureBound` calls. + auto completion = + backend->bindModel(::morph::backend::detail::BindRequest{.typeId = binding->typeId, + .factory = binding->modelFactory, + .contextKey = binding->contextKey, + .primary = {}, + .current = {}}, + ::morph::exec::detail::inlineExecutor()); + completion.then(onBound).onError(onFailed); + } } catch (...) { - // See attachHandlerAsync's identical guard: the backend's own + // See attachHandlerAsync's identical guard: a backend's own legacy // dispatch call can throw synchronously before send. lock.unlock(); onDone(std::current_exception()); @@ -815,23 +867,8 @@ class Bridge { onDone(parked->failure); return; } - if (started) { - return; - } - // No async path on this backend: run the identical synchronous - // registration `ensureBound` would have run, under the same lock, then - // report the outcome only once the lock is gone (see - // `attachHandlerAsync`'s "@par Locking"). - std::exception_ptr failure; - try { - auto newId = backend->registerModelShared(binding->typeId, binding->modelFactory, - {.contextKey = binding->contextKey, .primary = {}}); - binding->currentId.store(newId.v); - } catch (...) { - failure = std::current_exception(); - } - lock.unlock(); - onDone(failure); + // Nothing parked: the reply is still to come, and `onBound`/`onFailed` + // will deliver it themselves once it does. } /// @brief Files @p binding's current instance under @p primary, in place. @@ -855,24 +892,26 @@ class Bridge { /// `bool` return threaded across every `IBackend` implementation and, for /// wire backends, a reply field) — tracked as a follow-up, not fixed /// here. - /// Prefers the backend's `IBackend::assignPrimaryAsync` when it offers - /// one — the same "avoid a nested-event-loop block that aborts a WASM + /// Reaches the backend through `IBackend::promoteModel` — the structural + /// registration surface — unless it still overrides the legacy + /// `assignPrimaryAsync`, which is offered first and which morph#571 + /// removes. The same "avoid a nested-event-loop block that aborts a WASM /// main thread" rationale `Bridge::registerHandler()` follows for the - /// initial bind step (`backend.md`, "Asynchronous registration") applies - /// identically here: this method is invoked from inside the result - /// `Completion`'s callback chain (`BridgeHandler::execute`'s `onResult`), - /// not from the original call stack, so there is no caller left blocked - /// waiting on it either way — the async path simply avoids parking the - /// Qt event loop for the round trip. `binding->contextKey`/`primary` are - /// only published once the (possibly async) reply confirms the call, so - /// a caller reading `binding->primary()` never sees a promotion that the - /// backend has not actually completed. Falls back to the synchronous - /// `assignPrimary` when the backend offers no async path, publishing - /// immediately in that case. - /// - /// `_attachMtx` is released *before* calling `assignPrimaryAsync` — not - /// held across it — mirroring `registerHandlerImpl`'s discipline for - /// `registerModelAsync`: the success/error callback below re-acquires + /// initial bind step applies here: this method is invoked from inside the + /// result `Completion`'s callback chain (`BridgeHandler::execute`'s + /// `onResult`), not from the original call stack, so there is no caller + /// left blocked waiting on it either way — a non-blocking promote simply + /// avoids parking the Qt event loop for the round trip. + /// `binding->contextKey`/`primary` are only published once the reply + /// confirms the call, so a caller reading `binding->primary()` never sees a + /// promotion the backend has not actually completed. A backend with no + /// non-blocking promote settles inside the `promoteModel` call, publishing + /// before this method returns; a failure is logged rather than thrown, + /// since `promoteModel` reports through its `Completion` by contract. + /// + /// `_attachMtx` is released *before* the dispatch — not held across it — + /// mirroring `registerHandlerImpl`'s discipline: the success callback + /// below re-acquires /// `_attachMtx`, so a backend that ever invoked it synchronously (none /// documented here do, but nothing prevents one from doing so) would /// otherwise self-deadlock re-acquiring a mutex this same call stack @@ -895,57 +934,63 @@ class Bridge { auto const weakLiveness = _callbacks.token(); std::weak_ptr<::morph::backend::detail::IBackend> const weakBackend{backend}; std::weak_ptr const weakBinding{binding}; - bool const started = backend->assignPrimaryAsync( - ::morph::exec::detail::ModelId{raw}, binding->typeId, primary, - [this, weakLiveness, weakBackend, weakBinding, primary](::morph::exec::detail::ModelId) { - // This check and the `this` touch below it are two steps -- the - // morph#486 shape. Closed not by a gate but by - // `IBackend::registerModelAsync`'s threading contract (see its - // doc comment): an overriding backend must deliver `*Async` - // replies on a thread that cannot run `~Bridge` concurrently. - // 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; do not touch `this`. - } - auto strongBinding = weakBinding.lock(); - if (!strongBinding) { - return; // The BridgeHandler (and its binding) is gone. - } - std::scoped_lock const attachLock{_attachMtx}; - auto pinned = weakBackend.lock(); - if (!pinned || pinned != loadBackend()) { - // A switchBackend() already moved past this promotion; - // applying a stale reply now would overwrite whatever - // state the new backend's re-registration already - // established -- same reasoning as registerHandlerImpl's - // async callback. - return; - } - // A binding whose primary is already set (by a concurrent - // attach/assign that raced ahead of this async reply, or - // simply already promoted) must not be overwritten here. - if (!strongBinding->primary.empty()) { - return; - } - strongBinding->contextKey = primary; - strongBinding->primary = primary; - }, - [typeId = binding->typeId](const std::string& message) { - ::morph::log::logError("[assignHandlerPrimary] async promotion of '" + typeId + - "' failed: " + message); - }); - if (!started) { - backend->assignPrimary(::morph::exec::detail::ModelId{raw}, binding->typeId, primary); - std::scoped_lock const lock{_attachMtx}; - // Re-check under the lock: a concurrent attach/assign could have - // raced ahead while the (now-established-synchronous) backend - // call above ran without _attachMtx held. - if (binding->primary.empty()) { - binding->contextKey = primary; - binding->primary = std::move(primary); + auto onPromoted = [this, weakLiveness, weakBackend, weakBinding, primary](::morph::exec::detail::ModelId) { + // 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: the legacy + // `assignPrimaryAsync` branch below leaves that to the backend (see + // `IBackend::registerModelAsync`'s doc comment), the `promoteModel` + // branch names it as an executor. 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; do not touch `this`. + } + auto strongBinding = weakBinding.lock(); + if (!strongBinding) { + return; // The BridgeHandler (and its binding) is gone. } + std::scoped_lock const attachLock{_attachMtx}; + auto pinned = weakBackend.lock(); + if (!pinned || pinned != loadBackend()) { + // A switchBackend() already moved past this promotion; + // applying a stale reply now would overwrite whatever + // state the new backend's re-registration already + // established -- same reasoning as registerHandlerImpl's + // async callback. + return; + } + // A binding whose primary is already set (by a concurrent + // attach/assign that raced ahead of this async reply, or + // simply already promoted) must not be overwritten here. + if (!strongBinding->primary.empty()) { + return; + } + strongBinding->contextKey = primary; + strongBinding->primary = primary; + }; + auto onFailed = [typeId = binding->typeId](const std::string& message) { + ::morph::log::logError("[assignHandlerPrimary] async promotion of '" + typeId + "' failed: " + message); + }; + bool const started = backend->assignPrimaryAsync(::morph::exec::detail::ModelId{raw}, binding->typeId, primary, + onPromoted, onFailed); + if (!started) { + // 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 branch used to publish. + // Unlike that branch, 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. + 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)); + }); } } @@ -1740,21 +1785,27 @@ class Bridge { return _backend; } - /// @brief Shared body of both `registerHandler()` overloads: prefers the - /// backend's `registerModelAsync` path (see `IBackend::registerModelAsync`'s - /// doc comment for why — avoiding a nested-event-loop block that - /// aborts a WASM main thread) and falls back to the synchronous - /// `registerModelWithContext` when the backend offers no async path. - /// - /// @p binding is added to `_handlers` *before* the backend call — not - /// after, as the synchronous fallback below does internally — so a + /// @brief Shared body of both `registerHandler()` overloads: binds through + /// `IBackend::bindModel` — the structural registration surface — + /// unless the backend still overrides the legacy + /// `registerModelAsync`, which is offered first and which morph#571 + /// removes. + /// + /// A backend with a non-blocking bind returns an unsettled `Completion` and + /// the binding is returned unbound (see `IBackend::bindModel`'s doc comment + /// for why that matters — a nested-event-loop block aborts a WASM main + /// thread). One with only the blocking default settles inside the call, + /// having run exactly the `registerModelWithContext` this used to call + /// directly, so the binding is bound before this returns and a failure is + /// **rethrown** to the caller, as that call used to throw. + /// + /// @p binding is added to `_handlers` *before* the backend call, so a /// concurrently-running `switchBackend()`/reconnect can already see and /// re-register it even while this registration is still in flight (see - /// the async branch's comment for why that race is harmless). This also - /// means the backend call must not run under `_mtx`: a backend that (unlike - /// every backend documented here) invoked `onRegistered`/`onError` - /// synchronously from inside `registerModelAsync` would otherwise - /// self-deadlock re-acquiring `_mtx` in the callback below. + /// the continuation's comment for why that race is harmless). This also + /// means the backend call must not run under `_mtx`: a backend that settles + /// from inside the dispatch call would otherwise self-deadlock re-acquiring + /// `_mtx` in the continuation below. /// @param binding Binding to register; its `typeId`/`modelFactory`/`contextKey` must be set. void registerHandlerImpl(const std::shared_ptr& binding) { auto backend = loadBackend(); @@ -1796,73 +1847,110 @@ class Bridge { // **set `contextKey` before calling `registerHandler()`, and do not // mutate it concurrently with that call.** After registration returns, // every access goes under `_attachMtx` as documented. morph#505. - bool const started = backend->registerModelAsync( - binding->typeId, binding->modelFactory, binding->contextKey, - [this, weakBackend, weakBinding, lifetime = _lifetime](::morph::exec::detail::ModelId newId) { - auto strongBinding = weakBinding.lock(); - bool applied = false; - if (strongBinding) { - // `lifetime`'s gate held across the whole touch of `this` - // below (`_mtx`, `loadBackend()`), not just at entry -- - // `CallbackToken::active()` is advisory and cannot carry - // this weight (see `detail::BridgeLifetime`'s own doc - // comment). Safe to hold across this span, unlike - // `installReconnectHandler`'s reconnect callback - // (morph#489, site 4): nothing inside is a call into - // unbounded/consumer-supplied code, only a mutex and a - // backend-pointer comparison. - std::shared_lock const gate{lifetime->mtx}; - if (lifetime->alive) { - std::scoped_lock const lock{_mtx}; - auto pinned = weakBackend.lock(); - if (pinned && pinned == loadBackend()) { - // A switchBackend() already moved past this registration - // (see this backend's own doc comment on the class) and - // its own re-registration loop already gave `binding` a - // fresh id on the *new* backend -- applying this stale - // one now would overwrite that with a dangling id from a - // backend nothing uses any more. - strongBinding->currentId.store(newId.v); - applied = true; - } + auto onRegistered = [this, weakBackend, weakBinding, + lifetime = _lifetime](::morph::exec::detail::ModelId newId) { + auto strongBinding = weakBinding.lock(); + bool applied = false; + if (strongBinding) { + // `lifetime`'s gate held across the whole touch of `this` + // below (`_mtx`, `loadBackend()`), not just at entry -- + // `CallbackToken::active()` is advisory and cannot carry + // this weight (see `detail::BridgeLifetime`'s own doc + // comment). Safe to hold across this span, unlike + // `installReconnectHandler`'s reconnect callback + // (morph#489, site 4): nothing inside is a call into + // unbounded/consumer-supplied code, only a mutex and a + // backend-pointer comparison. + std::shared_lock const gate{lifetime->mtx}; + if (lifetime->alive) { + std::scoped_lock const lock{_mtx}; + auto pinned = weakBackend.lock(); + if (pinned && pinned == loadBackend()) { + // A switchBackend() already moved past this registration + // (see this backend's own doc comment on the class) and + // its own re-registration loop already gave `binding` a + // fresh id on the *new* backend -- applying this stale + // one now would overwrite that with a dangling id from a + // backend nothing uses any more. + strongBinding->currentId.store(newId.v); + applied = true; } } - // Resolve whenBound() waiters regardless of whether the id was - // actually applied above: either way this binding's initial - // registration attempt has settled (a stale reply ignored here - // means switchBackend's own synchronous re-registration already - // bound it), so nothing should still be described as "in - // flight". Runs even when the Bridge/binding is gone -- both - // weak locks above are only guards on touching `this`/the - // binding's other fields, not on this bookkeeping, which reads - // no Bridge state. - if (strongBinding) { - resolveRegistrationWaiters(*strongBinding, /*ok=*/applied || isBound(strongBinding), nullptr); + } + // Resolve whenBound() waiters regardless of whether the id was + // actually applied above: either way this binding's initial + // registration attempt has settled (a stale reply ignored here + // means switchBackend's own synchronous re-registration already + // bound it), so nothing should still be described as "in + // flight". Runs even when the Bridge/binding is gone -- both + // weak locks above are only guards on touching `this`/the + // binding's other fields, not on this bookkeeping, which reads + // no Bridge state. + if (strongBinding) { + resolveRegistrationWaiters(*strongBinding, /*ok=*/applied || isBound(strongBinding), nullptr); + } + }; + auto onFailed = [weakBinding, typeId = binding->typeId](const std::string& message) { + ::morph::log::logError("[registerHandler] async registration of '" + typeId + "' failed: " + message); + if (auto strongBinding = weakBinding.lock()) { + resolveRegistrationWaiters( + *strongBinding, /*ok=*/false, + std::make_exception_ptr(std::runtime_error("registration failed: " + message))); + } + }; + + bool const started = backend->registerModelAsync(binding->typeId, binding->modelFactory, binding->contextKey, + onRegistered, onFailed); + if (started) { + return; + } + + // The structural surface (`IBackend::bindModel`). An empty `primary` + // with a zero `current` is the request shape that means + // `registerModelWithContext` -- the verb this branch used to call + // directly -- so a backend with no non-blocking bind runs exactly that, + // blocks exactly as long, and settles before `bindModel` returns. + // + // `inlineExecutor()` because that is where this continuation ran + // before: on whichever thread the backend settled on, which for the + // blocking default is this one. See `exec::detail::InlineExecutor`. + // + // A synchronous backend that *fails* must still fail the way it used to + // -- `registerModelWithContext` threw out of `registerHandler()`, and a + // `BridgeHandler` constructor that cannot bind must not return as if it + // had. So a rejection arriving inside this frame is parked and + // rethrown, while one arriving later (a genuinely non-blocking backend, + // where no caller is left to throw at) goes to `onFailed`, which + // settles `whenBound()` waiters instead. + auto handoff = std::make_shared(); + auto completion = backend->bindModel(::morph::backend::detail::BindRequest{.typeId = binding->typeId, + .factory = binding->modelFactory, + .contextKey = binding->contextKey, + .primary = {}, + .current = {}}, + ::morph::exec::detail::inlineExecutor()); + completion + .then([onRegistered, handoff](::morph::exec::detail::ModelId newId) { + if (detail::parkIfInFrame(*handoff, true, newId, nullptr)) { + return; } - }, - [weakBinding, typeId = binding->typeId](const std::string& message) { - ::morph::log::logError("[registerHandler] async registration of '" + typeId + "' failed: " + message); - if (auto strongBinding = weakBinding.lock()) { - resolveRegistrationWaiters( - *strongBinding, /*ok=*/false, - std::make_exception_ptr(std::runtime_error("registration failed: " + message))); + onRegistered(newId); + }) + .onError([onFailed, handoff](const std::exception_ptr& failure) { + if (detail::parkIfInFrame(*handoff, false, {}, failure)) { + return; } + onFailed(detail::describeFailure(failure)); }); - - if (!started) { - binding->currentId.store( - backend->registerModelWithContext(binding->typeId, binding->modelFactory, binding->contextKey).v); - // Route through resolveRegistrationWaiters (not a bare flag - // clear): a whenBound() call from another thread that already - // holds this same binding (the pre-built-binding registerHandler() - // overload hands the caller the shared_ptr before this function - // is even called) could have raced in during the window above and - // queued a waiter while registrationInFlight was still true. That - // waiter's Completion must still be settled here, or it hangs - // forever -- registrationInFlight was never in flight for a - // backend with no async path, but whenBound() cannot tell that - // apart from "the reply just hasn't arrived yet" without this. - resolveRegistrationWaiters(*binding, /*ok=*/true, nullptr); + if (auto parked = detail::claimHandoff(*handoff)) { + if (!parked->succeeded) { + // Settles the waiters queued during the window above before + // unwinding, so a concurrent whenBound() is rejected rather + // than left hanging on a registration that will never complete. + resolveRegistrationWaiters(*binding, /*ok=*/false, parked->failure); + std::rethrow_exception(parked->failure); + } + onRegistered(parked->modelId); } } From 5a71a26a65021dc4ea611e6bb9c914001a282ba0 Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Sat, 19 Sep 2026 10:11:47 +0200 Subject: [PATCH 03/11] qt: move QtWebSocketBackend onto the structural surface, natively `QtWebSocketBackend` was the only backend in the tree overriding any of the four `*Async` verbs, so the entire prose threading contract on `IBackend` existed for exactly one class and the WASM special case it described lived here. It now overrides none of them, and implements `bindModel`/`promoteModel` directly -- not through `SynchronousBackendAdapter`, which moves a blocking call to another thread and a WASM main thread has no other thread to move it to. `Config::asyncRegistrationEnabled` stays, with its meaning narrowed to the one thing it actually decides: whether the transport blocks. Unset, `bindModel` defers to `IBackend`'s blocking default, which is the desktop behaviour every existing embedder relies on. It is no longer an opt-in to a second set of interface verbs, because the continuation now exists on both paths. Three duplications went with the verbs: - One send path (`sendControl`), so the "encode before recording the pending entry" invariant and morph#495's `env.session` stamp are stated once rather than four times. - One pending map: `register`, shared `register`, `attach` and `assign` replies were always matched identically, and the split had nothing left to represent. - `Completion::Promise` in place of two `std::function`s per entry, so a dropped socket now rejects with the `DisconnectedError` itself instead of a `runtime_error` rebuilt from its message -- the string channel could not carry a type. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01DbGrZGkr2WqcAs2DJyMym6 --- include/morph/qt/qt_websocket_backend.hpp | 356 ++++++++++------------ src/qt/qt_websocket_backend.cpp | 349 ++++++++------------- 2 files changed, 289 insertions(+), 416 deletions(-) diff --git a/include/morph/qt/qt_websocket_backend.hpp b/include/morph/qt/qt_websocket_backend.hpp index 780ce42d6..1081f29d3 100644 --- a/include/morph/qt/qt_websocket_backend.hpp +++ b/include/morph/qt/qt_websocket_backend.hpp @@ -39,34 +39,51 @@ struct QtWebSocketBackendConfig { /// @brief Multiplier applied to the delay after each failed attempt. double backoffMultiplier = 2.0; - /// @brief Opt in to `registerModelAsync` (see its doc comment on `IBackend`). - /// - /// Defaults to `false`: `Bridge::registerHandler()` then falls back to the - /// synchronous `registerModel`, exactly as before this feature existed — - /// every existing embedder (a desktop Qt client, this backend's own test - /// suite) keeps registering synchronously, immediately usable the line - /// after `BridgeHandler`'s constructor returns. - /// - /// Set `true` only for a build where that synchronous guarantee cannot - /// hold at all — a WASM main thread, where the nested `QEventLoop` - /// `registerModel` relies on aborts the page outright. Doing so is a - /// deliberate trade: the caller must then wait for registration to - /// complete (e.g. gate the UI on it) before firing an action through that - /// handler, since `executeVia` fails fast with "handler not bound" for an - /// unbound binding rather than queuing or blocking. + /// @brief Whether `bindModel`/`promoteModel` may return before the reply. + /// + /// Defaults to `false`: both settle their `Completion` from inside the call, + /// having blocked the Qt thread in a nested `QEventLoop` for the round trip + /// — `IBackend`'s own default behaviour, and what every existing embedder + /// (a desktop Qt client, this backend's own test suite) already relies on, + /// since it makes a handler usable on the line after `BridgeHandler`'s + /// constructor returns. + /// + /// Set `true` for a build where that blocking call cannot happen at all — + /// a WASM main thread, where Qt refuses to spin a nested loop and the + /// attempt aborts the page outright. `bindModel` then sends the request and + /// returns an unsettled `Completion`, which the reply settles later. That is + /// a deliberate trade, not a free improvement: the caller must wait for the + /// continuation (e.g. gate the UI on `BridgeHandler::whenBound()`) before + /// firing an action through that handler, since `executeVia` fails fast + /// with "handler not bound" for an unbound binding rather than queuing or + /// blocking. + /// + /// This flag chooses *whether the transport blocks*. It is no longer an + /// opt-in to a second set of interface verbs: the continuation exists on + /// both paths, because `bindModel` returns a `Completion` either way. bool asyncRegistrationEnabled = false; }; /// @brief `IBackend` implementation that communicates with a `RemoteServer` over WebSocket. /// -/// `registerModel()` is synchronous (blocks the calling thread via a nested -/// `QEventLoop` until the server replies) -- unusable on a WASM main thread, -/// which Qt refuses to spin a nested loop on at all. `registerModelAsync()` -/// is the non-blocking alternative `Bridge::registerHandler()` prefers when -/// available (see `IBackend::registerModelAsync`'s doc comment): it assigns a -/// call-id, sends the message, returns immediately, and invokes exactly one -/// of its `onRegistered`/`onError` callbacks once the matching reply arrives -/// -- the same call-id-matching mechanism `execute()` already uses. +/// Registration goes through the structural surface: `bindModel()` and +/// `promoteModel()` (`IBackend`, and `docs/spec/core/backend.md`'s "The +/// structural registration surface"). This is the one backend in the tree that +/// implements them *natively* rather than through the blocking defaults or +/// `SynchronousBackendAdapter` — with `Config::asyncRegistrationEnabled` set it +/// assigns a call-id, sends the message, and returns an unsettled `Completion` +/// that the matching reply settles, using the same call-id-matching mechanism +/// `execute()` already uses. Which thread the continuation then runs on is the +/// caller's choice, not this class's: `Completion` posts to the `IExecutor` +/// passed to `bindModel`/`promoteModel`, so a caller that needs the +/// continuation on its own thread names its own executor and gets it. +/// +/// The synchronous verbs (`registerModel()`, `registerModelShared()`, +/// `attachModel()`, `assignPrimary()`) remain, and still block the calling +/// thread in a nested `QEventLoop` until the server replies — unusable on a +/// WASM main thread, which Qt refuses to spin a nested loop on at all. They are +/// what the default (`asyncRegistrationEnabled == false`) `bindModel` runs, and +/// what `Bridge::switchBackend()` re-registers through. /// `deregisterModel()` is fire-and-forget (it sends the message without /// waiting, avoiding a nested event loop during destruction). `execute()` is /// asynchronous: it assigns a call-id, sends the message, and resolves the @@ -198,52 +215,57 @@ class QtWebSocketBackend : public ::morph::backend::detail::IBackend { const std::string& typeId, std::function()> factory) override; - /// @brief Sends a `register` message and returns without blocking; the - /// reply is matched later, asynchronously, by `callId`. - /// - /// The non-blocking counterpart to `registerModel`/`registerModelWithContext` - /// (see `IBackend::registerModelAsync`'s doc comment for why this exists): - /// `registerModel` blocks the calling thread in a nested `QEventLoop` via - /// `sendSync`, which a WASM main thread cannot do at all. This instead - /// assigns a fresh `callId` (the same counter `execute()` uses), sends the - /// `register` envelope, and returns `true` immediately; the reply is - /// matched via `_pendingRegistrations` when `onTextMessage` sees it (no - /// protocol change needed — the server already echoes `callId` on every - /// reply, `register` included). Exactly one of @p onRegistered / @p onError - /// fires, on the Qt event loop thread, once the reply arrives — or never, - /// if the socket disconnects first without ever reconnecting and - /// `cancelPending` is never called again for this id (a disconnect - /// *before* a reconnect calls `cancelPending`, which does invoke @p onError - /// — see `cancelPending`'s doc comment). - /// - /// If the socket has not finished connecting yet, the request is **queued** - /// rather than failed: this is exactly the ordering a single-threaded WASM - /// client must use, since it can never block waiting for the connection to - /// settle (a `BridgeHandler` constructed the moment the backend is wired - /// up, before the first `connected` signal). The queued request is sent — - /// with a call-id assigned at that point, not now — the moment `connected` - /// fires next (first connect included), in FIFO order. If the socket never - /// connects at all and the backend is torn down first, the queued entry is - /// still resolved: `~QtWebSocketBackend` calls `cancelPending`, which drains - /// `_queuedRegistrations` too and invokes @p onError exactly once for each — - /// no call-id was ever assigned, but the callback still fires, the same - /// guarantee an already-sent (call-id-bearing) registration gets. - /// - /// @param typeId String type-id of the model to instantiate. - /// @param factory Unused — this backend holds no local model to construct; - /// the server instantiates the model from `typeId`. - /// @param contextKey Stable identity of the new instance; travels in the wire envelope. - /// @param onRegistered Invoked with the server-assigned `ModelId` on success. - /// @param onError Invoked with a diagnostic message on failure or disconnect. - /// @return `true` when `Config::asyncRegistrationEnabled` is set (the - /// backend then owns the reply); `false` when it is not — which is - /// the default, so the caller falls back to the synchronous path - /// unless the embedder opted in. - bool registerModelAsync(const std::string& typeId, - std::function()> factory, - std::string_view contextKey, - std::function onRegistered, - std::function onError) override; + /// @brief Acquires a model instance over the wire, natively non-blocking + /// when `Config::asyncRegistrationEnabled` is set. + /// + /// The structural registration surface (`IBackend::bindModel`), implemented + /// here rather than inherited: this is the one backend in the tree with a + /// genuinely non-blocking acquire path, so it settles the returned + /// `Completion` when the server's reply arrives instead of blocking a + /// thread until then. + /// + /// With `Config::asyncRegistrationEnabled` unset (the default) this defers + /// to `IBackend::bindModel`, which runs the synchronous verb the request's + /// shape names and **blocks the Qt thread** in a nested `QEventLoop` for the + /// round trip. That is the desktop behaviour every existing embedder + /// relies on; see `QtWebSocketBackendConfig::asyncRegistrationEnabled`. + /// + /// With it set, the request's shape selects the envelope, mirroring + /// `IBackend::bindModelBlocking`'s routing of the same three shapes: + /// + /// | `primary` | `current` | Envelope sent | + /// |---|---|---| + /// | empty | `0` | `register` (private) | + /// | empty | non-zero | `deregister` of `current`, then `register` | + /// | non-empty | `0` | `register` with `shared` (register-or-attach) | + /// | non-empty | non-zero | `attach`, naming `current` | + /// + /// Each carries a fresh `callId` from the counter `execute()` uses, and the + /// reply is matched via `_pendingRegistrations` when `onTextMessage` sees + /// it — no protocol change, since the server already echoes `callId` on + /// every reply. + /// + /// A **private** registration issued before the socket has finished + /// connecting is queued rather than failed: exactly the ordering a + /// single-threaded WASM client must use, since it can never block waiting + /// for the connection to settle (a `BridgeHandler` constructed the moment + /// the backend is wired up, before the first `connected` signal). The queued + /// request is sent — with a call-id assigned at that point, not now — the + /// moment `connected` fires next, first connect included, in FIFO order. If + /// the socket never connects and the backend is torn down first, the queued + /// entry is still settled: `~QtWebSocketBackend` calls `cancelPending`, + /// which drains `_queuedRegistrations` too and rejects each exactly once. + /// A keyed bind on a disconnected socket rejects immediately instead. + /// + /// @param request Owning bind request; moved from. `request.factory` is + /// unused — this backend holds no local model to construct; + /// the server instantiates the model from `request.typeId`. + /// @param cbExec Executor the continuation is delivered on. Borrowed: it + /// must outlive the returned `Completion`. + /// @return A `Completion` resolved with the bound `ModelId`, or rejected + /// with the failure. Already settled on the blocking path. + ::morph::async::Completion<::morph::exec::detail::ModelId> bindModel(::morph::backend::detail::BindRequest request, + ::morph::exec::IExecutor& cbExec) override; /// @brief Sends a shared (register-or-attach) `register` and blocks for the reply. /// @@ -257,31 +279,6 @@ class QtWebSocketBackend : public ::morph::backend::detail::IBackend { const std::string& typeId, std::function()> factory, ::morph::backend::detail::InstanceIdentity identity) override; - /// @brief Sends a shared (register-or-attach) `register` and, if async - /// registration is enabled, returns without blocking. - /// - /// The non-blocking counterpart to `registerModelShared`, matching - /// `registerModelAsync`'s shape exactly (same `callId` counter, same - /// `_pendingRegistrations` map, same verb-agnostic reply routing in - /// `onTextMessage`). An empty `identity.primary` degrades to the private - /// path, i.e. to `registerModelAsync`, mirroring the synchronous - /// `registerModelShared`'s own degrade-to-private behaviour. - /// - /// @param typeId String type-id of the model. - /// @param factory Ignored — model construction is delegated to the server. - /// @param identity Entity key for the action log plus the directory primary key. - /// @param onRegistered Invoked with the assigned `ModelId` on success. - /// @param onError Invoked with a diagnostic message on failure. - /// @return `true` if `asyncRegistrationEnabled` is set (see - /// `QtWebSocketBackendConfig`) and the request was sent; - /// `false` otherwise, falling back to the synchronous - /// `registerModelShared`. - bool registerModelSharedAsync(const std::string& typeId, - std::function()> factory, - ::morph::backend::detail::InstanceIdentity identity, - std::function onRegistered, - std::function onError) override; - /// @brief Sends an `attach` and blocks for the reply, re-pointing from @p current. /// @param typeId String type-id of the model. /// @param factory Ignored — model construction is delegated to the server. @@ -293,30 +290,6 @@ class QtWebSocketBackend : public ::morph::backend::detail::IBackend { const std::string& typeId, std::function()> factory, ::morph::backend::detail::InstanceIdentity identity, ::morph::exec::detail::ModelId current) override; - /// @brief Sends an `attach` and, if async registration is enabled, - /// returns without blocking. - /// - /// The non-blocking counterpart to `attachModel`; see - /// `registerModelSharedAsync` immediately above for the shared shape. An - /// empty `identity.primary` releases @p current and degrades to a private - /// async registration, mirroring the synchronous `attachModel`'s own - /// empty-primary branch. - /// - /// @param typeId String type-id of the model. - /// @param factory Ignored — model construction is delegated to the server. - /// @param identity Entity key for the action log plus the directory primary key. - /// @param current Instance currently held, or `ModelId{0}` if none. - /// @param onRegistered Invoked with the `ModelId` now attached to, on success. - /// @param onError Invoked with a diagnostic message on failure. - /// @return `true` if `asyncRegistrationEnabled` is set and the request - /// was sent; `false` otherwise, falling back to the synchronous - /// `attachModel`. - bool attachModelAsync(const std::string& typeId, - std::function()> factory, - ::morph::backend::detail::InstanceIdentity identity, ::morph::exec::detail::ModelId current, - std::function onRegistered, - std::function onError) override; - /// @brief Files a live server-side instance under @p primary. /// @param mid Live instance to promote. /// @param typeId Model type id. @@ -324,30 +297,34 @@ class QtWebSocketBackend : public ::morph::backend::detail::IBackend { void assignPrimary(::morph::exec::detail::ModelId mid, const std::string& typeId, std::string_view primary) override; - /// @brief Sends an `assign` message and returns without blocking; the - /// reply is matched later, asynchronously, by `callId`. - /// - /// The non-blocking counterpart to `assignPrimary` (see - /// `IBackend::assignPrimaryAsync`'s doc comment): `assignPrimary` blocks - /// the calling thread in a nested `QEventLoop` via `sendSync`, the same - /// shape `registerModelAsync` exists to let a caller avoid for the bind - /// step. This instead assigns a fresh `callId` (the same counter - /// `execute()`/`registerModelAsync()` use), sends the `assign` envelope, - /// and returns `true` immediately; the reply is matched via - /// `_pendingAssigns` when `onTextMessage` sees it. Exactly one of - /// @p onRegistered / @p onError fires, on the Qt event loop thread, once - /// the reply arrives. - /// - /// @param mid Live instance to promote. - /// @param typeId Model type id. - /// @param primary Canonical string encoding of the key to file it under. - /// @param onRegistered Invoked with @p mid on success (including the - /// documented no-op cases -- see `IBackend::assignPrimaryAsync`). - /// @param onError Invoked with a diagnostic message on failure or disconnect. - /// @return `true` always -- this backend has an async path (`false` is never returned). - bool assignPrimaryAsync(::morph::exec::detail::ModelId mid, const std::string& typeId, std::string_view primary, - std::function onRegistered, - std::function onError) override; + /// @brief Files a live server-side instance under a key, without blocking. + /// + /// The structural counterpart of `assignPrimary` (`IBackend::promoteModel`), + /// implemented natively for the same reason `bindModel` above is: + /// `assignPrimary` blocks the Qt thread in a nested `QEventLoop` via + /// `sendSync`, which a WASM main thread cannot do. This assigns a fresh + /// `callId` from the counter `execute()`/`bindModel()` use, sends the + /// `assign` envelope, and returns an unsettled `Completion`; the reply is + /// matched via `_pendingAssigns` when `onTextMessage` sees it. + /// + /// Unlike `bindModel`, this does **not** consult + /// `Config::asyncRegistrationEnabled`: promotion happens from inside the + /// result `Completion`'s callback chain, where no caller is left blocked + /// waiting for it either way, so there is no synchronous guarantee to + /// preserve — which is why `assignPrimaryAsync`, the verb this replaces, + /// had no opt-in gate either. + /// + /// The documented no-op cases (empty `primary`, zero `mid`) resolve with + /// @p request's `mid` without sending anything, matching + /// `IBackend::promoteModel`. A disconnected socket rejects. + /// + /// @param request Owning promote request; moved from. + /// @param cbExec Executor the continuation is delivered on. Borrowed: it + /// must outlive the returned `Completion`. + /// @return A `Completion` resolved with the promoted `ModelId`, or rejected + /// with the failure. + ::morph::async::Completion<::morph::exec::detail::ModelId> promoteModel( + ::morph::backend::detail::PromoteRequest request, ::morph::exec::IExecutor& cbExec) override; /// @brief Asks the server for the live shared primary keys of @p typeId. /// @param typeId String type-id to enumerate. @@ -363,7 +340,7 @@ class QtWebSocketBackend : public ::morph::backend::detail::IBackend { /// model registered on the server indefinitely. /// /// Assigned a real, non-zero `callId` from the same counter/namespace - /// `execute()`/`registerModelAsync()` use (see issue #65): `callId == 0` + /// `execute()`/`bindModel()` use (see issue #65): `callId == 0` /// is reserved for a parked synchronous control call's reply, and a /// fire-and-forget `deregister` sharing that sentinel could otherwise have /// its own stray "ok" reply handed to an unrelated `registerModel`'s @@ -390,16 +367,14 @@ class QtWebSocketBackend : public ::morph::backend::detail::IBackend { /// @brief No-op — this backend holds no local model objects. void notifyBackendChanged() override {} - /// @brief Resolves every pending execute call's `Completion` with @p exc, - /// and fails every pending async registration's `onError`. + /// @brief Rejects every in-flight call's `Completion` with @p exc — + /// executes, binds (queued ones included) and promotes alike. /// /// Called by `Bridge::switchBackend()` on the outgoing backend, by `~Bridge`, /// and internally when the socket disconnects. Late replies arriving for - /// already-cancelled call ids (execute or register) are dropped silently. + /// already-cancelled call ids (execute, bind or promote) are dropped silently. /// - /// @param exc Exception delivered to every pending completion's error sink; - /// `exc.what()`-equivalent text is delivered to every pending - /// `registerModelAsync` call's `onError`. + /// @param exc Exception delivered to every pending completion's error sink. void cancelPending(const std::exception_ptr& exc) override; /// @brief Installs the handler `Bridge` uses to re-register handlers after a reconnect. @@ -451,15 +426,10 @@ class QtWebSocketBackend : public ::morph::backend::detail::IBackend { /// @return `true` if an execute was found and settled. bool tryRouteExecuteReply(const ::morph::wire::Envelope& env); - /// @brief Completes the async model registration filed under `env.callId`. + /// @brief Settles the bind/promote filed under `env.callId`, if one is pending. /// @param env Decoded reply envelope. - /// @return `true` if a pending registration was found and completed. - bool tryRouteRegistrationReply(const ::morph::wire::Envelope& env); - - /// @brief Completes the async primary-assignment filed under `env.callId`. - /// @param env Decoded reply envelope. - /// @return `true` if a pending assignment was found and completed. - bool tryRouteAssignReply(const ::morph::wire::Envelope& env); + /// @return `true` if a pending control call was found and settled. + bool tryRouteControlReply(const ::morph::wire::Envelope& env); /// @brief Drops the reply to a fire-and-forget deregister (issue #65). /// @param env Decoded reply envelope. @@ -472,16 +442,24 @@ class QtWebSocketBackend : public ::morph::backend::detail::IBackend { /// @brief Attempts to reopen the socket using the saved URL/TLS config. void attemptReconnect(); - /// @brief Assigns a call-id, records the pending registration, and sends - /// the `register` envelope. Shared by `registerModelAsync`'s - /// immediate path and the queued-request flush on `connected`. - void sendRegisterAsync(const std::string& typeId, std::string_view contextKey, - std::function onRegistered, - std::function onError); + /// @brief Assigns a call-id, records @p promise, and sends @p env. + /// + /// The one send path every non-blocking control call takes, so the ordering + /// invariant it enforces — encode before the map insertion, because a + /// throwing `wire::encode()` after inserting would park a promise for a + /// reply to a message that was never sent — is stated and tested once + /// rather than four times. + /// + /// @param env Control envelope to send; its `callId`/`session` are + /// stamped here. + /// @param promise Settled when the matching reply arrives, or by + /// `cancelPending`. + void sendControl(::morph::wire::Envelope env, + ::morph::async::Completion<::morph::exec::detail::ModelId>::Promise promise); - /// @brief Sends every request queued by `registerModelAsync` while the - /// socket was not yet connected, in FIFO order. Called from the - /// `connected` slot, before the reconnect handler fires. + /// @brief Sends every private bind queued while the socket was not yet + /// connected, in FIFO order. Called from the `connected` slot, + /// before the reconnect handler fires. void flushQueuedRegistrations(); QUrl _serverUrl; @@ -512,29 +490,29 @@ class QtWebSocketBackend : public ::morph::backend::detail::IBackend { std::unordered_map _pending; std::mutex _pendingMtx; - /// @brief One in-flight `registerModelAsync` call, keyed by `callId`. + /// @brief In-flight `bindModel`/`promoteModel` calls, keyed by `callId`. /// /// Kept separate from `PendingExecute`/`_pending` (a different `callId` /// namespace would be a protocol change; this shares the same namespace - /// and counter, just a different local map) because a register reply's - /// shape (`modelId`, no `deserialize` step) differs from an execute - /// reply's. - struct PendingRegistration { - std::function onRegistered; - std::function onError; - }; - std::unordered_map _pendingRegistrations; - - /// @brief One `registerModelAsync` call made before the socket had - /// finished connecting. No call-id is assigned until the request - /// is actually sent (from `flushQueuedRegistrations`), so a queued - /// entry that never gets to fire (backend destroyed first) needs no - /// cancellation bookkeeping. + /// and counter, just a different local map) because a control reply's shape + /// (`modelId`, no `deserialize` step) differs from an execute reply's. + /// + /// One map for both verbs, not two: `register`, `registerShared`, `attach` + /// and `assign` replies are all matched identically — a bare `modelId` + /// echoed against the `callId` — so the split the four `*Async` verbs used + /// to justify has nothing left to represent. + std::unordered_map::Promise> + _pendingRegistrations; + + /// @brief One private `bindModel` issued before the socket had finished + /// connecting. No call-id is assigned until the request is actually + /// sent (from `flushQueuedRegistrations`), so a queued entry that + /// never gets to fire (backend destroyed first) is drained by + /// `cancelPending` from here rather than by call-id. struct QueuedRegistration { std::string typeId; std::string contextKey; - std::function onRegistered; - std::function onError; + ::morph::async::Completion<::morph::exec::detail::ModelId>::Promise promise; }; std::vector _queuedRegistrations; @@ -549,18 +527,6 @@ class QtWebSocketBackend : public ::morph::backend::detail::IBackend { /// backend was cancelled/destroyed) is simply ignored — nothing to clean /// up either way. std::unordered_set _pendingDeregisters; - - /// @brief One in-flight `assignPrimaryAsync` call, keyed by `callId`. - /// - /// Mirrors `PendingRegistration`/`_pendingRegistrations`: same `callId` - /// namespace and counter, separate map because an `assign` reply is - /// matched the same way a `register`/`registerShared` reply is (a bare - /// `modelId`, echoing back the instance that was promoted). - struct PendingAssign { - std::function onRegistered; - std::function onError; - }; - std::unordered_map _pendingAssigns; }; } // namespace morph::qt diff --git a/src/qt/qt_websocket_backend.cpp b/src/qt/qt_websocket_backend.cpp index d24bad8bd..8bda27911 100644 --- a/src/qt/qt_websocket_backend.cpp +++ b/src/qt/qt_websocket_backend.cpp @@ -7,6 +7,7 @@ #include #include #include +#include #include #include @@ -45,10 +46,10 @@ QtWebSocketBackend::QtWebSocketBackend(QUrl serverUrl, ::morph::model::detail::A if (_connectHandler) { _connectHandler(); } - // Send every registerModelAsync request that arrived before this - // connect (the first connect included) -- see issue #54. Runs before - // the reconnect handler below so a caller that gates UI on - // onRegistered sees it fire promptly on first connect too. + // Send every bind request that arrived before this connect (the first + // connect included) -- see issue #54. Runs before the reconnect handler + // below so a caller that gates UI on the bind's continuation sees it + // fire promptly on first connect too. flushQueuedRegistrations(); // Fire the reconnect handler only on subsequent connects, never on the // first one — initial registration is handled by the BridgeHandler ctors. @@ -157,57 +158,97 @@ ::morph::exec::detail::ModelId QtWebSocketBackend::registerModel( throw std::runtime_error("register failed: " + reply.message); } -bool QtWebSocketBackend::registerModelAsync( - const std::string& typeId, std::function()> /*factory*/, - std::string_view contextKey, std::function onRegistered, - std::function onError) { +::morph::async::Completion<::morph::exec::detail::ModelId> QtWebSocketBackend::bindModel( + ::morph::backend::detail::BindRequest request, ::morph::exec::IExecutor& cbExec) { if (!_cfg.asyncRegistrationEnabled) { - // Opt-in only (see QtWebSocketBackendConfig::asyncRegistrationEnabled): - // returning false here makes Bridge::registerHandler() fall back to - // the synchronous registerModel(), preserving every existing - // embedder's behavior unless it explicitly asks for the async path. - return false; + // Blocking by request (see QtWebSocketBackendConfig::asyncRegistrationEnabled): + // IBackend's default runs the synchronous verb this request's shape + // names and settles from this thread, preserving every existing + // embedder's behaviour unless it explicitly asks for the non-blocking + // path. + return ::morph::backend::detail::IBackend::bindModel(std::move(request), cbExec); } - if (!_connected) { - // Queue rather than fail: this is exactly the ordering a - // single-threaded WASM client must use, since it can never block - // waiting for the connection to settle (see issue #54). The queued - // request is sent -- with a call-id assigned then, not now -- the - // moment `connected` fires next (first connect included), from - // flushQueuedRegistrations(). No call-id is assigned yet; if the - // backend is torn down (or the socket disconnects) before that - // happens, cancelPending() drains this queue too and still invokes - // onError exactly once. - std::scoped_lock const lock{_pendingMtx}; - _queuedRegistrations.push_back(QueuedRegistration{.typeId = typeId, - .contextKey = std::string{contextKey}, - .onRegistered = std::move(onRegistered), - .onError = std::move(onError)}); - return true; + + auto [completion, promise] = ::morph::async::Completion<::morph::exec::detail::ModelId>::makeSettleable(&cbExec); + + if (request.primary.empty()) { + // Mirrors the synchronous attachModel's empty-primary branch: release + // the instance currently held (fire-and-forget, as deregisterModel + // already is) and degrade to a private registration. + if (request.current.v != 0U) { + deregisterModel(request.current); + } + if (!_connected) { + // Queue rather than fail: this is exactly the ordering a + // single-threaded WASM client must use, since it can never block + // waiting for the connection to settle (see issue #54). The queued + // request is sent -- with a call-id assigned then, not now -- the + // moment `connected` fires next (first connect included), from + // flushQueuedRegistrations(). No call-id is assigned yet; if the + // backend is torn down (or the socket disconnects) before that + // happens, cancelPending() drains this queue too and still settles + // the promise exactly once. + std::scoped_lock const lock{_pendingMtx}; + _queuedRegistrations.push_back(QueuedRegistration{.typeId = std::move(request.typeId), + .contextKey = std::move(request.contextKey), + .promise = std::move(promise)}); + return std::move(completion); + } + sendControl(::morph::wire::makeRegister(request.typeId, request.contextKey), std::move(promise)); + return std::move(completion); } - sendRegisterAsync(typeId, contextKey, std::move(onRegistered), std::move(onError)); - return true; + + if (!_connected) { + // A keyed bind carries no queue: unlike a private registration it may + // be a re-point of a live instance, and replaying that against a + // connection that has since been re-established would attach from a + // `current` the new connection never issued. + promise.reject(std::make_exception_ptr(std::runtime_error{"disconnected"})); + return std::move(completion); + } + + // `registerShared` for a first bind, `attach` when re-pointing from a live + // instance -- the same two envelopes registerModelShared/attachModel build, + // selected by the same field of the request that IBackend::bindModelBlocking + // selects the synchronous verb by. + auto env = request.current.v == 0U + ? ::morph::wire::makeRegisterShared(request.typeId, request.primary, request.contextKey) + : ::morph::wire::makeAttach(request.typeId, request.primary, request.current.v, request.contextKey); + sendControl(std::move(env), std::move(promise)); + return std::move(completion); } -void QtWebSocketBackend::sendRegisterAsync(const std::string& typeId, std::string_view contextKey, - std::function onRegistered, - std::function onError) { +void QtWebSocketBackend::sendControl(::morph::wire::Envelope env, + ::morph::async::Completion<::morph::exec::detail::ModelId>::Promise promise) { uint64_t const callId = ++_nextCallId; - auto env = ::morph::wire::makeRegister(typeId, std::string{contextKey}); env.callId = callId; + // Stamped exactly as every synchronous control verb stamps it: RemoteServer + // authenticates and authorizes from env.session, so omitting it reached the + // server as an unauthenticated principal on the non-blocking (WASM) path + // only. morph#495 -- and now unmissable, because this is the only place a + // control envelope is sent from. env.session = _session; - // Encoded before the map insertion below: wire::encode() can throw on - // serialization failure, and a throw after inserting would leave this - // callId's onRegistered/onError parked in _pendingRegistrations forever, - // waiting for a reply to a message that was never sent -- nothing erases - // an entry whose send never happened. Encoding first means a throw here - // propagates to the caller (Bridge::registerHandlerImpl et al. already - // handle it) with nothing to clean up. - auto const encoded = QString::fromStdString(::morph::wire::encode(env)); + QString encoded; + try { + // Encoded before the map insertion below: wire::encode() can throw on + // serialization failure, and a throw after inserting would leave this + // callId's promise parked in _pendingRegistrations forever, waiting for + // a reply to a message that was never sent -- nothing erases an entry + // whose send never happened. + encoded = QString::fromStdString(::morph::wire::encode(env)); + } catch (...) { + // Rejected rather than rethrown: `bindModel`/`promoteModel` promise + // their caller exactly one failure channel, the returned `Completion` + // (`IBackend::bindModel`). Rethrowing would give a dispatch-time + // failure a second one, which is what the legacy `*Async` verbs did + // and what every call site then had to carry a `catch` for. Nothing + // has been recorded at this point, so there is nothing to unwind. + promise.reject(std::current_exception()); + return; + } { std::scoped_lock const lock{_pendingMtx}; - _pendingRegistrations[callId] = - PendingRegistration{.onRegistered = std::move(onRegistered), .onError = std::move(onError)}; + _pendingRegistrations.insert_or_assign(callId, std::move(promise)); } _socket.sendTextMessage(encoded); } @@ -219,86 +260,8 @@ void QtWebSocketBackend::flushQueuedRegistrations() { queued.swap(_queuedRegistrations); } for (auto& entry : queued) { - sendRegisterAsync(entry.typeId, entry.contextKey, std::move(entry.onRegistered), std::move(entry.onError)); - } -} - -bool QtWebSocketBackend::registerModelSharedAsync( - const std::string& typeId, std::function()> /*factory*/, - ::morph::backend::detail::InstanceIdentity identity, - std::function onRegistered, - std::function onError) { - if (!_cfg.asyncRegistrationEnabled) { - return false; - } - if (identity.primary.empty()) { - // Degrades to the private (non-shared) path, exactly like the - // synchronous registerModelShared below -- and that path already - // has an async form: this class's own registerModelAsync. - return registerModelAsync(typeId, nullptr, identity.contextKey, std::move(onRegistered), std::move(onError)); - } - if (!_connected) { - onError("disconnected"); - return true; - } - uint64_t const callId = ++_nextCallId; - auto env = - ::morph::wire::makeRegisterShared(typeId, std::string{identity.primary}, std::string{identity.contextKey}); - env.callId = callId; - // Same stamp the synchronous registerModelShared applies: RemoteServer - // authenticates and authorizes from env.session, so omitting it here reached - // the server as an unauthenticated principal on the async (WASM) path only. - // morph#495. - env.session = _session; - // See registerModelAsync's identical comment: encoded before the map - // insertion, so a throwing encode() cannot orphan a pending entry. - auto const encoded = QString::fromStdString(::morph::wire::encode(env)); - { - std::scoped_lock const lock{_pendingMtx}; - _pendingRegistrations[callId] = - PendingRegistration{.onRegistered = std::move(onRegistered), .onError = std::move(onError)}; - } - _socket.sendTextMessage(encoded); - return true; -} - -bool QtWebSocketBackend::attachModelAsync( - const std::string& typeId, std::function()> /*factory*/, - ::morph::backend::detail::InstanceIdentity identity, ::morph::exec::detail::ModelId current, - std::function onRegistered, - std::function onError) { - if (!_cfg.asyncRegistrationEnabled) { - return false; - } - if (identity.primary.empty()) { - // Mirrors the synchronous attachModel's empty-primary branch: release - // the current instance (fire-and-forget, as deregisterModel already - // is) and degrade to a private async registration. - if (current.v != 0U) { - deregisterModel(current); - } - return registerModelAsync(typeId, nullptr, identity.contextKey, std::move(onRegistered), std::move(onError)); + sendControl(::morph::wire::makeRegister(entry.typeId, entry.contextKey), std::move(entry.promise)); } - if (!_connected) { - onError("disconnected"); - return true; - } - uint64_t const callId = ++_nextCallId; - auto env = - ::morph::wire::makeAttach(typeId, std::string{identity.primary}, current.v, std::string{identity.contextKey}); - env.callId = callId; - // See registerModelSharedAsync above: stamped for the same reason (morph#495). - env.session = _session; - // See registerModelAsync's identical comment: encoded before the map - // insertion, so a throwing encode() cannot orphan a pending entry. - auto const encoded = QString::fromStdString(::morph::wire::encode(env)); - { - std::scoped_lock const lock{_pendingMtx}; - _pendingRegistrations[callId] = - PendingRegistration{.onRegistered = std::move(onRegistered), .onError = std::move(onError)}; - } - _socket.sendTextMessage(encoded); - return true; } ::morph::wire::ProtocolNegotiationResult QtWebSocketBackend::negotiateProtocolVersion() { @@ -365,40 +328,23 @@ void QtWebSocketBackend::assignPrimary(::morph::exec::detail::ModelId mid, const (void)modelIdFromReply(sendSync(::morph::wire::encode(env)), "assign"); } -bool QtWebSocketBackend::assignPrimaryAsync(::morph::exec::detail::ModelId mid, const std::string& typeId, - std::string_view primary, - std::function onRegistered, - std::function onError) { - if (primary.empty() || mid.v == 0U) { +::morph::async::Completion<::morph::exec::detail::ModelId> QtWebSocketBackend::promoteModel( + ::morph::backend::detail::PromoteRequest request, ::morph::exec::IExecutor& cbExec) { + auto [completion, promise] = ::morph::async::Completion<::morph::exec::detail::ModelId>::makeSettleable(&cbExec); + + if (request.primary.empty() || request.mid.v == 0U) { // Same no-op contract as the synchronous assignPrimary: nothing to - // promote. Resolve onRegistered as a no-op success, echoing mid back, - // rather than treating it as a failure. - onRegistered(mid); - return true; + // promote. Resolve, echoing mid back, rather than treating it as a + // failure. + promise.resolve(request.mid); + return std::move(completion); } if (!_connected) { - onError("disconnected"); - return true; - } - uint64_t const callId = ++_nextCallId; - auto env = ::morph::wire::makeAssign(typeId, std::string{primary}, mid.v); - env.callId = callId; - // Same stamp the synchronous assignPrimary applies -- RemoteServer authorizes - // from env.session, so an unstamped assign reached the server as an - // unauthenticated principal on the async (WASM) path only (morph#495). - env.session = _session; - // Encoded before the map insertion below, the invariant registerModelAsync - // states and the other two async hooks already follow: wire::encode() can - // throw, and a throw after inserting would park this callId's - // onRegistered/onError in _pendingAssigns forever with no message sent. - auto const encoded = QString::fromStdString(::morph::wire::encode(env)); - { - std::scoped_lock const lock{_pendingMtx}; - _pendingAssigns[callId] = - PendingAssign{.onRegistered = std::move(onRegistered), .onError = std::move(onError)}; + promise.reject(std::make_exception_ptr(std::runtime_error{"disconnected"})); + return std::move(completion); } - _socket.sendTextMessage(encoded); - return true; + sendControl(::morph::wire::makeAssign(request.typeId, request.primary, request.mid.v), std::move(promise)); + return std::move(completion); } std::vector QtWebSocketBackend::listInstances(const std::string& typeId) { @@ -472,15 +418,14 @@ ::morph::async::Completion> QtWebSocketBackend::execute( void QtWebSocketBackend::cancelPending(const std::exception_ptr& exc) { std::unordered_map drainedExecutes; - std::unordered_map drainedRegistrations; + std::unordered_map::Promise> + drainedRegistrations; std::vector drainedQueue; - std::unordered_map drainedAssigns; { std::scoped_lock const lock{_pendingMtx}; drainedExecutes.swap(_pending); drainedRegistrations.swap(_pendingRegistrations); drainedQueue.swap(_queuedRegistrations); - drainedAssigns.swap(_pendingAssigns); // _pendingDeregisters tracks fire-and-forget requests nobody awaits -- // just drop the bookkeeping, there is no callback to invoke. _pendingDeregisters.clear(); @@ -490,41 +435,22 @@ void QtWebSocketBackend::cancelPending(const std::exception_ptr& exc) { pending.state->setException(exc); } } - // Every caller passes a `make_exception_ptr`, so `rethrow_exception` always - // throws and one of the two handlers always assigns `message`. - std::string message; - try { - std::rethrow_exception(exc); - } catch (const std::exception& concrete) { - message = concrete.what(); - } catch (...) { - // A non-std::exception carries no portable message, so report the - // disconnect itself -- which is what a caller can act on anyway. Doing - // it here rather than as an initializer above keeps the handler from - // being lexically empty, which `bugprone-empty-catch` rejects however - // well the intent is commented (morph#514). - message = "disconnected"; - } - for (auto& [ignoredCallId, pending] : drainedRegistrations) { - if (pending.onError) { - pending.onError(message); - } - } - // A registerModelAsync request queued while the socket had never yet - // connected (issue #54) never got a call-id, so it cannot be found in - // _pendingRegistrations above -- drain it here instead, on the same - // cancelPending path that already handles a connection that goes away - // (or never comes up) before a queued reply, so its onError still fires - // exactly once rather than leaving the caller waiting forever. + for (auto& [ignoredCallId, promise] : drainedRegistrations) { + // The exception itself, not a message rebuilt from it: a control call + // rejected by a dropped socket now delivers the very + // `backend::DisconnectedError` an execute() call delivers, instead of + // the `runtime_error` the `*Async` verbs' string channel flattened it + // into. + promise.reject(exc); + } + // A private bind queued while the socket had never yet connected (issue + // #54) never got a call-id, so it cannot be found in _pendingRegistrations + // above -- drain it here instead, on the same cancelPending path that + // already handles a connection that goes away (or never comes up) before a + // queued reply, so its continuation still fires exactly once rather than + // leaving the caller waiting forever. for (auto& entry : drainedQueue) { - if (entry.onError) { - entry.onError(message); - } - } - for (auto& [ignoredCallId, pending] : drainedAssigns) { - if (pending.onError) { - pending.onError(message); - } + entry.promise.reject(exc); } } @@ -619,42 +545,26 @@ bool QtWebSocketBackend::tryRouteExecuteReply(const ::morph::wire::Envelope& env return true; } -bool QtWebSocketBackend::tryRouteRegistrationReply(const ::morph::wire::Envelope& env) { - PendingRegistration regPending; +bool QtWebSocketBackend::tryRouteControlReply(const ::morph::wire::Envelope& env) { + std::optional<::morph::async::Completion<::morph::exec::detail::ModelId>::Promise> promise; { std::scoped_lock const lock{_pendingMtx}; auto iter = _pendingRegistrations.find(env.callId); if (iter == _pendingRegistrations.end()) { return false; } - regPending = std::move(iter->second); + promise.emplace(std::move(iter->second)); _pendingRegistrations.erase(iter); } + // Settled outside `_pendingMtx`: settling runs the caller's continuation on + // whatever executor it named, and an executor that runs inline (which is + // what `Bridge` names) would then re-enter this backend under a lock this + // frame still holds. if (env.kind == "ok") { - regPending.onRegistered(::morph::exec::detail::ModelId{env.modelId}); + promise->resolve(::morph::exec::detail::ModelId{env.modelId}); } else { - regPending.onError(env.message); - } - return true; -} - -bool QtWebSocketBackend::tryRouteAssignReply(const ::morph::wire::Envelope& env) { - PendingAssign assignPending; - { - std::scoped_lock const lock{_pendingMtx}; - auto iter = _pendingAssigns.find(env.callId); - if (iter == _pendingAssigns.end()) { - return false; - } - assignPending = std::move(iter->second); - _pendingAssigns.erase(iter); - } - - if (env.kind == "ok") { - assignPending.onRegistered(::morph::exec::detail::ModelId{env.modelId}); - } else { - assignPending.onError(env.message); + promise->reject(std::make_exception_ptr(std::runtime_error{env.message})); } return true; } @@ -678,10 +588,7 @@ void QtWebSocketBackend::routeKeyedReply(const ::morph::wire::Envelope& env) { if (tryRouteExecuteReply(env)) { return; } - if (tryRouteRegistrationReply(env)) { - return; - } - if (tryRouteAssignReply(env)) { + if (tryRouteControlReply(env)) { return; } // No map matched (the deregister drop included): an already-resolved or @@ -700,7 +607,7 @@ void QtWebSocketBackend::onTextMessage(const QString& message) { return; } - // Async execute replies and async registerModelAsync replies both carry a + // Execute replies and non-blocking bind/promote replies both carry a // non-zero callId (the two share one counter/namespace, but land in // separate maps below since their reply shapes differ); sync replies // (registerModel/deregister/etc's sendSync calls) carry callId == 0 and From 77dfb31213bafbeb261e1f5a642c58993525378e Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Sat, 19 Sep 2026 10:11:53 +0200 Subject: [PATCH 04/11] tests: prove the surface carries a production backend, and the WASM path with it morph#567's "natively non-blocking backend" case exercised a test double settling its own promise, which pins the shape an implementor needs rather than what a real transport does. `QtWebSocketBackend` is the first real implementor, so this is the first test that the guarantee survives one. "bindModel delivers its continuation on the caller's executor, not the thread it settles on" uses an executor that counts posts without running them: the test pumps only the Qt loop until the reply lands and asserts the continuation has *not* run, then drains and asserts it has. Building the `Completion` against an inline executor instead of `cbExec` fails it (measured). The four direct `*Async` tests are rewritten against `bindModel` -- keyed register-or-attach, re-point, degrade-to-private, reject on a dead socket -- and `promoteModel` gains direct coverage including its documented no-op case. The morph#495 session-stamping test drives `bindModel`, where `sendControl` is now the single place that gap could reappear. `ThrowingSyncRegisterSharedBackend` now throws from `registerModelWithContext` too. `ensureBoundAsync` asks for an anonymous instance, and `BindRequest` names that shape (`primary` empty, `current` zero) as `registerModelWithContext` -- which `IBackend::registerModelShared` documents an empty primary as degrading to, and which every backend in the tree implements as its first statement. A double that threw only from `registerModelShared` would have stopped failing and the test would have passed by registering successfully. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01DbGrZGkr2WqcAs2DJyMym6 --- tests/qt/test_qt_websocket.cpp | 365 ++++++++++++++++++++++-------- tests/test_async_registration.cpp | 38 +++- 2 files changed, 295 insertions(+), 108 deletions(-) diff --git a/tests/qt/test_qt_websocket.cpp b/tests/qt/test_qt_websocket.cpp index f5b6169b0..42059ddde 100644 --- a/tests/qt/test_qt_websocket.cpp +++ b/tests/qt/test_qt_websocket.cpp @@ -105,6 +105,56 @@ static void pumpUntil(const std::function& done, int maxIterations = 50) QCoreApplication::sendPostedEvents(nullptr, QEvent::DeferredDelete); } +// ── Structural-registration-surface helpers ────────────────────────────────── +// +// The tests below drive `bindModel`/`promoteModel` directly. They need to +// distinguish "the continuation was handed to my executor" from "the +// continuation ran", which is precisely the distinction the surface exists to +// make: a backend that settled inline would collapse the two. +namespace { + +/// @brief `MainThreadExecutor` that counts what has been handed to it. +/// +/// `posted` rises when the backend settles; the queued task only runs on +/// `drain()`. A backend that ignored `cbExec` and invoked the continuation +/// itself would leave `posted` at zero. +struct CountingExecutor : morph::exec::MainThreadExecutor { + std::atomic posted{0}; + + void post(std::function task) override { + posted.fetch_add(1); + morph::exec::MainThreadExecutor::post(std::move(task)); + } +}; + +/// @brief Outcome of one `bindModel`/`promoteModel` call, as the test sees it. +struct ControlOutcome { + std::atomic modelId{0}; + std::string failure; + std::atomic settled{false}; +}; + +/// @brief Attaches @p outcome's handlers to @p completion. +void observe(morph::async::Completion& completion, ControlOutcome& outcome) { + completion + .then([&outcome](morph::exec::detail::ModelId mid) { + outcome.modelId.store(mid.v); + outcome.settled.store(true); + }) + .onError([&outcome](const std::exception_ptr& err) { + try { + std::rethrow_exception(err); + } catch (const std::exception& exc) { + outcome.failure = exc.what(); + } catch (...) { + outcome.failure = "unknown"; + } + outcome.settled.store(true); + }); +} + +} // namespace + // ── TLS config helpers ─────────────────────────────────────────────────────── static QSslConfiguration makeServerTlsConfig() { QFile certFile{QStringLiteral(TESTS_CERTS_DIR "/server.crt")}; @@ -233,7 +283,7 @@ TEST_CASE("morph::qt::QtWebSocketBackend::notifyBackendChanged is a documented n } TEST_CASE( - "morph::qt::QtWebSocketBackend: registerModelAsync (opt-in via Config::asyncRegistrationEnabled) registers " + "morph::qt::QtWebSocketBackend: bindModel (non-blocking via Config::asyncRegistrationEnabled) registers " "without blocking", "[qt][ws][issue26]") { ensureApp(); @@ -258,7 +308,7 @@ TEST_CASE( // Registration does not block: registerHandler() already returned above, // yet the binding is still unbound -- this is the whole point of the - // async path (see IBackend::registerModelAsync's doc comment). A real + // non-blocking path (see IBackend::bindModel's doc comment). A real // WASM caller would gate its UI on this instead of firing an action // immediately, since executeVia fails fast on an unbound binding. CHECK(binding->currentId.load() == 0U); @@ -358,7 +408,7 @@ TEST_CASE( #endif TEST_CASE( - "morph::qt::QtWebSocketBackend: registerModelAsync's pending registration is cancelled when the connection " + "morph::qt::QtWebSocketBackend: bindModel's pending registration is cancelled when the connection " "drops before a reply arrives", "[qt][ws][issue26]") { ensureApp(); @@ -391,13 +441,14 @@ TEST_CASE( CHECK(binding->currentId.load() == 0U); // onRegistered never fired; still safely unbound } -// ── The shared/keyed async wire methods ────────────────────────────────────── -// Same opt-in gate and same callId-keyed reply routing as registerModelAsync -// above; these drive them against a real RemoteServer, end to end. +// ── The keyed shapes of the structural surface ─────────────────────────────── +// `bindModel` with a non-empty `primary` is the register-or-attach the +// `registerModelShared`/`attachModel` pair used to spell as two verbs; these +// drive it against a real RemoteServer, end to end. // NOLINTNEXTLINE(readability-function-cognitive-complexity) -TEST_CASE("morph::qt::QtWebSocketBackend: registerModelSharedAsync registers-or-attaches without blocking", - "[qt][ws][issue26][shared-instances]") { +TEST_CASE("morph::qt::QtWebSocketBackend: bindModel with a primary registers-or-attaches without blocking", + "[qt][ws][issue26][shared-instances][morph568]") { ensureApp(); morph::exec::ThreadPoolExecutor serverPool{2}; auto server = std::make_shared(serverPool); @@ -410,19 +461,22 @@ TEST_CASE("morph::qt::QtWebSocketBackend: registerModelSharedAsync registers-or- morph::qt::QtWebSocketBackend::Config{.asyncRegistrationEnabled = true}}; REQUIRE(backend.waitForConnected()); - std::atomic registered{0}; - std::string failure; - REQUIRE(backend.registerModelSharedAsync( - "WsEchoModel", nullptr, {.contextKey = "acct-1", .primary = "acct-1"}, - [&](morph::exec::detail::ModelId mid) { registered.store(mid.v); }, - [&](const std::string& message) { failure = message; })); + morph::exec::MainThreadExecutor cbExec; + ControlOutcome first; + auto firstBind = backend.bindModel( + {.typeId = "WsEchoModel", .factory = nullptr, .contextKey = "acct-1", .primary = "acct-1", .current = {}}, + cbExec); + observe(firstBind, first); - // Returned true without waiting for the reply: nothing has arrived yet. - CHECK(registered.load() == 0U); + // Returned without waiting for the reply: nothing has arrived yet. + CHECK(first.modelId.load() == 0U); - pumpUntil([&] { return registered.load() != 0U || !failure.empty(); }); - CHECK(failure.empty()); - REQUIRE(registered.load() != 0U); + pumpUntil([&] { + cbExec.drain(); + return first.settled.load(); + }); + CHECK(first.failure.empty()); + REQUIRE(first.modelId.load() != 0U); // It really went out as a *shared* register, not a private one: the key is // now in the server's instance directory. @@ -430,21 +484,24 @@ TEST_CASE("morph::qt::QtWebSocketBackend: registerModelSharedAsync registers-or- REQUIRE(keys.size() == 1); CHECK(keys.front() == "acct-1"); - // A second shared register for the same key joins the same instance rather - // than creating a second one -- the register-or-attach half of the name. - std::atomic second{0}; - REQUIRE(backend.registerModelSharedAsync( - "WsEchoModel", nullptr, {.contextKey = "acct-1", .primary = "acct-1"}, - [&](morph::exec::detail::ModelId mid) { second.store(mid.v); }, - [&](const std::string& message) { failure = message; })); - pumpUntil([&] { return second.load() != 0U || !failure.empty(); }); - CHECK(failure.empty()); - CHECK(second.load() == registered.load()); + // A second bind for the same key joins the same instance rather than + // creating a second one -- the register-or-attach half of the shape. + ControlOutcome second; + auto secondBind = backend.bindModel( + {.typeId = "WsEchoModel", .factory = nullptr, .contextKey = "acct-1", .primary = "acct-1", .current = {}}, + cbExec); + observe(secondBind, second); + pumpUntil([&] { + cbExec.drain(); + return second.settled.load(); + }); + CHECK(second.failure.empty()); + CHECK(second.modelId.load() == first.modelId.load()); } // NOLINTNEXTLINE(readability-function-cognitive-complexity) -TEST_CASE("morph::qt::QtWebSocketBackend: attachModelAsync joins the existing shared instance without blocking", - "[qt][ws][issue26][shared-instances]") { +TEST_CASE("morph::qt::QtWebSocketBackend: bindModel re-points from a live instance without blocking", + "[qt][ws][issue26][shared-instances][morph568]") { ensureApp(); morph::exec::ThreadPoolExecutor serverPool{2}; auto server = std::make_shared(serverPool); @@ -457,40 +514,49 @@ TEST_CASE("morph::qt::QtWebSocketBackend: attachModelAsync joins the existing sh morph::qt::QtWebSocketBackend::Config{.asyncRegistrationEnabled = true}}; REQUIRE(backend.waitForConnected()); - // Seed the directory synchronously, so the async attach below has something - // to join and its reply can be compared against a known id. + // Seed the directory synchronously, so the bind below has something to join + // and its reply can be compared against a known id. auto const seeded = backend.registerModelShared("WsEchoModel", nullptr, {.contextKey = "acct-7", .primary = "acct-7"}); REQUIRE(seeded.v != 0U); - std::atomic attached{0}; - std::string failure; - REQUIRE(backend.attachModelAsync( - "WsEchoModel", nullptr, {.contextKey = "acct-7", .primary = "acct-7"}, morph::exec::detail::ModelId{0}, - [&](morph::exec::detail::ModelId mid) { attached.store(mid.v); }, - [&](const std::string& message) { failure = message; })); - CHECK(attached.load() == 0U); // the reply has not arrived yet - - pumpUntil([&] { return attached.load() != 0U || !failure.empty(); }); - CHECK(failure.empty()); - CHECK(attached.load() == seeded.v); - - // Re-pointing to a different key gets a different instance, still async. - std::atomic repointed{0}; - REQUIRE(backend.attachModelAsync( - "WsEchoModel", nullptr, {.contextKey = "acct-8", .primary = "acct-8"}, - morph::exec::detail::ModelId{attached.load()}, - [&](morph::exec::detail::ModelId mid) { repointed.store(mid.v); }, - [&](const std::string& message) { failure = message; })); - pumpUntil([&] { return repointed.load() != 0U || !failure.empty(); }); - CHECK(failure.empty()); - REQUIRE(repointed.load() != 0U); - CHECK(repointed.load() != seeded.v); + morph::exec::MainThreadExecutor cbExec; + ControlOutcome joined; + auto join = backend.bindModel( + {.typeId = "WsEchoModel", .factory = nullptr, .contextKey = "acct-7", .primary = "acct-7", .current = {}}, + cbExec); + observe(join, joined); + CHECK(joined.modelId.load() == 0U); // the reply has not arrived yet + + pumpUntil([&] { + cbExec.drain(); + return joined.settled.load(); + }); + CHECK(joined.failure.empty()); + CHECK(joined.modelId.load() == seeded.v); + + // Re-pointing to a different key gets a different instance -- the `attach` + // envelope, selected by a non-zero `current`. + ControlOutcome repointed; + auto repoint = backend.bindModel({.typeId = "WsEchoModel", + .factory = nullptr, + .contextKey = "acct-8", + .primary = "acct-8", + .current = morph::exec::detail::ModelId{joined.modelId.load()}}, + cbExec); + observe(repoint, repointed); + pumpUntil([&] { + cbExec.drain(); + return repointed.settled.load(); + }); + CHECK(repointed.failure.empty()); + REQUIRE(repointed.modelId.load() != 0U); + CHECK(repointed.modelId.load() != seeded.v); } // NOLINTNEXTLINE(readability-function-cognitive-complexity) -TEST_CASE("morph::qt::QtWebSocketBackend: attachModelAsync with an empty primary degrades to a private registration", - "[qt][ws][issue26][shared-instances]") { +TEST_CASE("morph::qt::QtWebSocketBackend: bindModel with an empty primary is a private registration", + "[qt][ws][issue26][shared-instances][morph568]") { ensureApp(); morph::exec::ThreadPoolExecutor serverPool{2}; auto server = std::make_shared(serverPool); @@ -503,23 +569,25 @@ TEST_CASE("morph::qt::QtWebSocketBackend: attachModelAsync with an empty primary morph::qt::QtWebSocketBackend::Config{.asyncRegistrationEnabled = true}}; REQUIRE(backend.waitForConnected()); - std::atomic registered{0}; - std::string failure; - REQUIRE(backend.attachModelAsync( - "WsEchoModel", nullptr, {.contextKey = "ctx", .primary = ""}, morph::exec::detail::ModelId{0}, - [&](morph::exec::detail::ModelId mid) { registered.store(mid.v); }, - [&](const std::string& message) { failure = message; })); - - pumpUntil([&] { return registered.load() != 0U || !failure.empty(); }); - CHECK(failure.empty()); - REQUIRE(registered.load() != 0U); + morph::exec::MainThreadExecutor cbExec; + ControlOutcome outcome; + auto bind = backend.bindModel( + {.typeId = "WsEchoModel", .factory = nullptr, .contextKey = "ctx", .primary = "", .current = {}}, cbExec); + observe(bind, outcome); + + pumpUntil([&] { + cbExec.drain(); + return outcome.settled.load(); + }); + CHECK(outcome.failure.empty()); + REQUIRE(outcome.modelId.load() != 0U); // Private, exactly like the synchronous attachModel's own empty-primary // branch: nothing was filed in the shared directory. CHECK(backend.listInstances("WsEchoModel").empty()); } -TEST_CASE("morph::qt::QtWebSocketBackend: registerModelSharedAsync on a never-connected socket reports onError", - "[qt][ws][issue26][shared-instances][disconnect]") { +TEST_CASE("morph::qt::QtWebSocketBackend: a keyed bindModel on a never-connected socket rejects", + "[qt][ws][issue26][shared-instances][disconnect][morph568]") { ensureApp(); // Port 1 is reserved and never listening — the socket never reaches Connected. QUrl url{QString("ws://127.0.0.1:1")}; @@ -528,21 +596,116 @@ TEST_CASE("morph::qt::QtWebSocketBackend: registerModelSharedAsync on a never-co morph::qt::QtWebSocketBackend::Config{.asyncRegistrationEnabled = true}}; REQUIRE_FALSE(backend.waitForConnected(200)); - std::string failure; - std::atomic registered{0}; - // Accepts the request (returns true) and reports the failure through - // onError rather than blocking or throwing. Bridge::ensureBoundAsync - // tolerates this firing inline, from inside the call itself. - REQUIRE(backend.registerModelSharedAsync( - "WsEchoModel", nullptr, {.contextKey = "acct-1", .primary = "acct-1"}, - [&](morph::exec::detail::ModelId mid) { registered.store(mid.v); }, - [&](const std::string& message) { failure = message; })); - CHECK(registered.load() == 0U); - CHECK(failure == "disconnected"); + morph::exec::MainThreadExecutor cbExec; + ControlOutcome outcome; + // Reports the failure through the Completion rather than blocking or + // throwing: one failure channel, which is what the structural surface + // promises its caller. + auto bind = backend.bindModel( + {.typeId = "WsEchoModel", .factory = nullptr, .contextKey = "acct-1", .primary = "acct-1", .current = {}}, + cbExec); + observe(bind, outcome); + cbExec.drain(); + CHECK(outcome.modelId.load() == 0U); + CHECK(outcome.failure == "disconnected"); +} + +// ── The delivery thread, which is what morph#567 made structural ───────────── +// +// This is the first *production* backend on the surface, so this is the first +// test that the guarantee survives a real transport rather than a test double +// settling its own promise. `QtWebSocketBackend` settles from `onTextMessage`, +// on the Qt event-loop thread; the caller here names a different executor and +// must be the one that decides when the continuation runs. +// +// Written so it fails if the backend delivered inline: `posted` rises when the +// reply lands, and the continuation must still not have run at that point. +TEST_CASE( + "morph::qt::QtWebSocketBackend: bindModel delivers its continuation on the caller's executor, not the " + "thread it settles on", + "[qt][ws][morph568][threading]") { + ensureApp(); + morph::exec::ThreadPoolExecutor serverPool{2}; + auto server = std::make_shared(serverPool); + morph::qt::QtWebSocketServer wsServer{*server, 0}; + REQUIRE(wsServer.listen()); + + QUrl url{QString("ws://127.0.0.1:%1").arg(wsServer.port())}; + morph::qt::QtWebSocketBackend backend{url, morph::model::detail::defaultDispatcher(), + morph::model::detail::defaultRegistry(), std::nullopt, + morph::qt::QtWebSocketBackend::Config{.asyncRegistrationEnabled = true}}; + REQUIRE(backend.waitForConnected()); + + CountingExecutor cbExec; + std::atomic ran{false}; + std::atomic seen{0}; + auto bind = backend.bindModel( + {.typeId = "WsEchoModel", .factory = nullptr, .contextKey = "ctx", .primary = "", .current = {}}, cbExec); + bind.then([&](morph::exec::detail::ModelId mid) { + seen.store(mid.v); + ran.store(true); + }) + .onError([&](const std::exception_ptr&) { ran.store(true); }); + + // Pump only the Qt loop: the reply arrives and the backend settles, which + // posts the continuation. Nothing drains `cbExec` here. + pumpUntil([&] { return cbExec.posted.load() != 0; }); + REQUIRE(cbExec.posted.load() != 0); // the reply really did land + CHECK_FALSE(ran.load()); // ... and the backend did not run it + + cbExec.drain(); + CHECK(ran.load()); + CHECK(seen.load() != 0U); +} + +// The promote half of the surface, direct rather than through Bridge. +TEST_CASE("morph::qt::QtWebSocketBackend: promoteModel files a live instance under a key without blocking", + "[qt][ws][shared-instances][morph568]") { + ensureApp(); + morph::exec::ThreadPoolExecutor serverPool{2}; + auto server = std::make_shared(serverPool); + morph::qt::QtWebSocketServer wsServer{*server, 0}; + REQUIRE(wsServer.listen()); + + QUrl url{QString("ws://127.0.0.1:%1").arg(wsServer.port())}; + morph::qt::QtWebSocketBackend backend{url, morph::model::detail::defaultDispatcher(), + morph::model::detail::defaultRegistry(), std::nullopt, + morph::qt::QtWebSocketBackend::Config{.asyncRegistrationEnabled = true}}; + REQUIRE(backend.waitForConnected()); + + auto const anonymous = backend.registerModel("WsEchoModel", nullptr); + REQUIRE(anonymous.v != 0U); + REQUIRE(backend.listInstances("WsEchoModel").empty()); + + morph::exec::MainThreadExecutor cbExec; + ControlOutcome outcome; + auto promote = backend.promoteModel({.mid = anonymous, .typeId = "WsEchoModel", .primary = "acct-9"}, cbExec); + observe(promote, outcome); + CHECK_FALSE(outcome.settled.load()); // not blocked on the round trip + + pumpUntil([&] { + cbExec.drain(); + return outcome.settled.load(); + }); + CHECK(outcome.failure.empty()); + CHECK(outcome.modelId.load() == anonymous.v); + + auto const keys = backend.listInstances("WsEchoModel"); + REQUIRE(keys.size() == 1); + CHECK(keys.front() == "acct-9"); + + // The documented no-op cases resolve rather than reject, and send nothing. + ControlOutcome noop; + auto emptyKey = backend.promoteModel({.mid = anonymous, .typeId = "WsEchoModel", .primary = ""}, cbExec); + observe(emptyKey, noop); + cbExec.drain(); + CHECK(noop.settled.load()); + CHECK(noop.failure.empty()); + CHECK(noop.modelId.load() == anonymous.v); } TEST_CASE( - "morph::qt::QtWebSocketBackend: registerModelAsync called before the socket connects queues and retries once " + "morph::qt::QtWebSocketBackend: bindModel called before the socket connects queues and retries once " "connected fires", "[qt][ws][issue54]") { ensureApp(); @@ -565,7 +728,7 @@ TEST_CASE( auto binding = std::make_shared(); binding->typeId = "WsEchoModel"; binding->modelFactory = [] { return morph::model::detail::ModelFactory::create(); }; - // registerModelAsync is called here, before the socket has finished its + // bindModel is called here, before the socket has finished its // handshake. Previously this called onError("disconnected") immediately // and never retried -- currentId would stay 0 forever even once the // socket connects moments later. It must instead queue the attempt and @@ -2321,7 +2484,7 @@ int main(int argc, char* argv[]) { return result; } -// ── morph#495: the async control paths must stamp the session too ── +// ── morph#495: the non-blocking control paths must stamp the session too ── // // registerModelSharedAsync, attachModelAsync and assignPrimaryAsync each built // their envelope and encoded it with no `env.session = _session`, while all @@ -2329,7 +2492,10 @@ int main(int argc, char* argv[]) { // authorizes from env.session (remote.hpp: stampVerifiedPrincipal, and the // register/attach/assign authorization sites), so a client using the async path // -- which is the WASM path, and the only one a WASM main thread can use -- -// reached an authorizing server as an unauthenticated principal. +// reached an authorizing server as an unauthenticated principal. Those three +// verbs are gone (morph#568); `sendControl` is now the single place a control +// envelope is built, so the gap has one place left to reappear in -- and this +// test still guards it. // // A test asserting only "the async call succeeds" would have passed before the // fix, so this records what the *server* saw. @@ -2355,7 +2521,7 @@ struct RecordingAuthorizer : morph::session::IAuthorizer { }; } // namespace -TEST_CASE("morph::qt::QtWebSocketBackend: the async control envelopes carry the session", +TEST_CASE("morph::qt::QtWebSocketBackend: the non-blocking control envelopes carry the session", "[qt][ws][morph495][security]") { ensureApp(); morph::exec::ThreadPoolExecutor serverPool{2}; @@ -2376,20 +2542,23 @@ TEST_CASE("morph::qt::QtWebSocketBackend: the async control envelopes carry the session.token = "tok-495"; backend.setSession(session); - std::atomic registered{0}; - std::string failure; - REQUIRE(backend.registerModelSharedAsync( - "WsEchoModel", nullptr, {.contextKey = "acct-495", .primary = "acct-495"}, - [&](morph::exec::detail::ModelId mid) { registered.store(mid.v); }, - [&](const std::string& message) { failure = message; })); - pumpUntil([&] { return registered.load() != 0U || !failure.empty(); }); - CHECK(failure.empty()); - REQUIRE(registered.load() != 0U); + morph::exec::MainThreadExecutor cbExec; + ControlOutcome outcome; + auto bind = backend.bindModel( + {.typeId = "WsEchoModel", .factory = nullptr, .contextKey = "acct-495", .primary = "acct-495", .current = {}}, + cbExec); + observe(bind, outcome); + pumpUntil([&] { + cbExec.drain(); + return outcome.settled.load(); + }); + CHECK(outcome.failure.empty()); + REQUIRE(outcome.modelId.load() != 0U); std::scoped_lock const lock{authorizer->mtx}; REQUIRE_FALSE(authorizer->registerTokens.empty()); - // Before the fix this was "" -- registerModelSharedAsync built its envelope - // with no `env.session = _session`, so the server received a - // default-constructed session and could not authenticate the caller at all. + // Before the fix this was "" -- the shared register built its envelope with + // no `env.session = _session`, so the server received a default-constructed + // session and could not authenticate the caller at all. CHECK(authorizer->registerTokens.back() == "tok-495"); } diff --git a/tests/test_async_registration.cpp b/tests/test_async_registration.cpp index ee9f10300..1ee1b69f6 100644 --- a/tests/test_async_registration.cpp +++ b/tests/test_async_registration.cpp @@ -534,10 +534,20 @@ class AsyncAssignPrimaryBackend : public morph::backend::LocalBackend { // A backend with no async registration path at all (registerModelSharedAsync // defaults to `return false`, so ensureBoundAsync always falls back to the -// synchronous path), whose synchronous registerModelShared throws -- -// exercises ensureBoundAsync's own synchronous-fallback `catch (...)` -// (Task 15a finding B2), the ensureBoundAsync counterpart of -// attachHandlerAsync's identical-shaped fallback catch. +// blocking path), whose synchronous register throws -- exercises +// ensureBoundAsync's own fallback failure path (Task 15a finding B2), the +// ensureBoundAsync counterpart of attachHandlerAsync's identical-shaped one. +// +// Both verbs throw, not just registerModelShared. `ensureBoundAsync` asks for an +// *anonymous* instance -- `registerModelShared` with an empty `primary` -- and +// `IBackend::registerModelShared` documents that case as degrading to +// `registerModelWithContext`; every backend in the tree implements the degrade +// as its first statement. Since morph#568 the fallback goes through +// `IBackend::bindModel`, whose `BindRequest` names that shape directly +// (`primary` empty, `current` zero) and therefore reaches +// `registerModelWithContext`, so a double that threw only from +// `registerModelShared` would quietly stop failing and this test would pass by +// registering successfully instead of by surfacing a throw. class ThrowingSyncRegisterSharedBackend : public morph::backend::detail::IBackend { public: morph::exec::detail::ModelId registerModel( @@ -553,6 +563,12 @@ class ThrowingSyncRegisterSharedBackend : public morph::backend::detail::IBacken void notifyBackendChanged() override {} void cancelPending(const std::exception_ptr&) override {} + morph::exec::detail::ModelId registerModelWithContext( + const std::string&, std::function()>, + std::string_view) override { + throw std::runtime_error("registerModelShared failed synchronously"); + } + morph::exec::detail::ModelId registerModelShared( const std::string&, std::function()>, morph::backend::detail::InstanceIdentity) override { @@ -1862,13 +1878,15 @@ TEST_CASE( "(Task 15a finding B2)", "[bridge][registration][issue26]") { // Distinct from the test above: ThrowingDispatchBackend's throw comes from - // the ASYNC dispatch entry point itself (registerModelSharedAsync), before - // any fallback is even considered. ThrowingSyncRegisterSharedBackend + // the legacy ASYNC dispatch entry point itself (registerModelSharedAsync), + // before any fallback is even considered, and is caught by + // ensureBoundAsync's `catch (...)`. ThrowingSyncRegisterSharedBackend // instead offers no async path at all (registerModelSharedAsync's default - // `return false`), so ensureBoundAsync falls through to running the - // synchronous registerModelShared body under the same lock -- and that - // synchronous call is what throws here, exercising the fallback's own - // `catch (...)` (bridge.hpp's ensureBoundAsync, ~line 751). + // `return false`), so ensureBoundAsync falls through to `bindModel`, whose + // default runs the blocking register from inside the call. That throw is + // turned into a rejection by `IBackend::bindModel` rather than propagating + // -- one failure channel, the returned `Completion` -- and must still reach + // @p onDone, with the original exception rather than a stringified one. auto binding = std::make_shared(); binding->typeId = "AR_Model"; binding->modelFactory = [] { return morph::model::detail::ModelFactory::create(); }; From 00377bff0e60b4e6568164863f78410ab1889f44 Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Sat, 19 Sep 2026 10:11:53 +0200 Subject: [PATCH 05/11] docs: record what morph#568 changed, and what it deliberately did not `backend.md` gains a migration-status table for the morph#522 set, marks the `*Async` sections as historical now that no backend overrides them, and describes `QtWebSocketBackend`'s native `bindModel`. Both specs are explicit that this does not close morph#486. `Bridge` names an inline executor because it owns no event loop, so the delivery thread is unchanged; what moved is the decision, from fifteen implementors to four call sites. Closing the window needs a `Bridge` executor bound to the thread that runs `~Bridge`, and nothing in the set currently owns that. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01DbGrZGkr2WqcAs2DJyMym6 --- docs/spec/concurrency_and_lifetimes.md | 38 ++-- docs/spec/core/backend.md | 265 ++++++++++++++++++++----- 2 files changed, 236 insertions(+), 67 deletions(-) diff --git a/docs/spec/concurrency_and_lifetimes.md b/docs/spec/concurrency_and_lifetimes.md index 2eff27a92..5d5edb1ba 100644 --- a/docs/spec/concurrency_and_lifetimes.md +++ b/docs/spec/concurrency_and_lifetimes.md @@ -298,13 +298,17 @@ that bounded wait into an unbounded one. Four dispositions, by site: `CallbackToken::active()` check and then takes `_attachMtx` and calls `loadBackend()`, so the two-step shape is present in the source. What closes the window is not a gate but a contract on the backend: - `IBackend::registerModelAsync`'s doc comment now states that a backend + `IBackend::registerModelAsync`'s doc comment states that a backend overriding any `*Async` hook must deliver its callbacks on a thread from - which `~Bridge` cannot run concurrently. `QtWebSocketBackend` — the only - backend in the tree that overrides them — satisfies this by construction - rather than by care: it must itself be used from the Qt event loop thread, - and fires all four callbacks from `onTextMessage` on that same thread, so the - check and the use cannot straddle a destructor. Gating these instead would + which `~Bridge` cannot run concurrently. Since morph#568 **no backend + overrides them**: each of the three sites now reaches + `IBackend::bindModel`/`promoteModel` instead, naming + `exec::detail::inlineExecutor()` as the delivery executor. That reproduces the + old delivery thread exactly — the continuation runs wherever the backend + settled — so the window is unchanged, and `QtWebSocketBackend` is still safe + for the same reason it 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 `_attachMtx`, which the synchronous `attachHandler` holds across a full `attachModel` round trip — the same shape of objection that rules a gate out for the reconnect handler. **The safety @@ -313,14 +317,20 @@ that bounded wait into an unbounded one. Four dispositions, by site: thread would reopen morph#486's use-after-free, and that is a contract break rather than a latent race to be rediscovered. - A structural alternative now exists alongside these four hooks and is what - replaces them: `IBackend::bindModel`/`promoteModel` take 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. That does not by itself close the window above; - it relocates the decision from a documented obligation on fifteen - implementors to a value one call site produces. Nothing in `Bridge` uses it - yet. See [core/backend.md](core/backend.md#the-structural-registration-surface--bindmodel-and-promotemodel) + 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 + `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. `switchBackend()` and `whenBound()` were audited for the same shape and do not diff --git a/docs/spec/core/backend.md b/docs/spec/core/backend.md index 0853aa4b9..7f807e7e0 100644 --- a/docs/spec/core/backend.md +++ b/docs/spec/core/backend.md @@ -30,6 +30,7 @@ and react to backend changes. - [Connect/disconnect notifications](#connectdisconnect-notifications) - [Asynchronous registration — `registerModelAsync`](#asynchronous-registration--registermodelasync) - [The structural registration surface — `bindModel` and `promoteModel`](#the-structural-registration-surface--bindmodel-and-promotemodel) + - [What a natively non-blocking backend does to `registerHandler`](#what-a-natively-non-blocking-backend-does-to-registerhandler) - [Error types](#error-types) - [`LocalBackend` — in-process execution](#localbackend--in-process-execution) - [`RemoteServer` — server-side message handler](#remoteserver--server-side-message-handler) @@ -76,7 +77,7 @@ holds a `unique_ptr` and delegates all model operations to it. |---|---| | `registerModel(typeId, factory)` | Registers a new model instance, returns its opaque `ModelId`. | | `registerModelWithContext(typeId, factory, contextKey)` | Same as `registerModel`, additionally passes a stable identity (e.g. account id). Default implementation drops `contextKey` and forwards to `registerModel` — correct for `LocalBackend` where the factory closure already captures identity. Every backend whose instances live behind a wire protocol overrides it to carry `contextKey` across: `SimulatedRemoteBackend` and `SocketBackend` both do. Not cosmetic — `RemoteServer::attachLogIfConfigured` skips the `LogProvider` lookup entirely on an empty `contextKey`, so a wire backend that drops the key leaves the instance with **no** action log rather than a log missing a field (morph#587). | -| `registerModelAsync(typeId, factory, contextKey, onRegistered, onError)` | Optional non-blocking counterpart to `registerModelWithContext`. Returns `false` by default (no async path); `Bridge::registerHandler()` prefers this when it returns `true` and falls back to the synchronous call otherwise. See [Asynchronous registration](#asynchronous-registration--registermodelasync). | +| `registerModelAsync(typeId, factory, contextKey, onRegistered, onError)` | Optional non-blocking counterpart to `registerModelWithContext`. Returns `false` by default, and since morph#568 no backend overrides it, so it always does; `Bridge::registerHandler()` then falls back to `bindModel`. Removed by morph#571. See [Asynchronous registration](#asynchronous-registration--registermodelasync). | | `bindModel(request, cbExec)` | Acquires a model instance and returns a `Completion` delivered on `cbExec`. One verb covering `registerModelWithContext`, `registerModelShared` and `attachModel`, selected by the request's shape. The preferred surface — see [The structural registration surface](#the-structural-registration-surface--bindmodel-and-promotemodel). | | `promoteModel(request, cbExec)` | Files an already-live instance under a key and returns a `Completion` delivered on `cbExec`. The structural counterpart of `assignPrimary`. | | `deregisterModel(mid)` | Removes the model identified by `mid`. | @@ -155,6 +156,15 @@ envelope, so there is nothing to stamp. ## Asynchronous registration — `registerModelAsync` +> **Status.** Since morph#568 **no backend in the tree overrides any of the four +> `*Async` verbs.** They still exist on `IBackend`, still default to `false`, +> and `Bridge` still offers them first — but every implementor now reaches +> `Bridge` through [the structural registration +> surface](#the-structural-registration-surface--bindmodel-and-promotemodel) +> below, which is what `Bridge` falls back to when a verb answers `false`. This +> section describes the older shape, which morph#571 removes; read it for what +> the `*Async` contract *was*, not for how registration works today. + `registerModel`/`registerModelWithContext` are synchronous: a backend whose registration requires a round-trip can only implement that by blocking the calling thread until the reply arrives — `QtWebSocketBackend` does this via a @@ -175,9 +185,10 @@ either callback. the pre-built-binding overload) prefers this path: it adds the binding to `_handlers` and calls `registerModelAsync` *before* acquiring `Bridge::_mtx` for the callback (a synchronous callback invocation would otherwise -self-deadlock re-acquiring the lock). If it returns `false`, `registerHandler` -falls back to the synchronous `registerModelWithContext`, exactly as before -this feature existed. If it returns `true`, the binding is returned **unbound** +self-deadlock re-acquiring the lock). If it returns `false` — which, since +morph#568, it always does — `registerHandler` falls back to `bindModel` with an +empty `primary` and a zero `current`, the request shape that means +`registerModelWithContext`. If it returns `true`, the binding is returned **unbound** (`currentId == 0`) — `executeVia` fails fast with "handler not bound" for any call made before `onRegistered` fires, so a caller using the async path must wait for registration (e.g. gate its UI on it) rather than fire an action @@ -200,11 +211,14 @@ and the reconnect handler perform after a backend swap remains synchronous; giving that an async path too is a larger change to `Bridge`'s locking model, left for a future issue if it proves necessary. -`QtWebSocketBackend` is the one backend that currently overrides this, gated -by `QtWebSocketBackendConfig::asyncRegistrationEnabled` (default `false` — see -its own section below). +`QtWebSocketBackend` was the one backend that ever overrode this; morph#568 +moved it to `bindModel`, so nothing overrides it now. Everything the two +paragraphs below describe still happens — it happens inside +`QtWebSocketBackend::bindModel`, still gated by +`QtWebSocketBackendConfig::asyncRegistrationEnabled` (default `false` — see its +own section below). -**Queueing before the first connect.** A `registerModelAsync` call made before +**Queueing before the first connect.** A non-blocking private bind made before the socket has finished connecting is **queued**, not failed — this is exactly the ordering a single-threaded WASM client must use, since it can never block waiting for the connection to settle (a `BridgeHandler` constructed the moment @@ -213,8 +227,8 @@ request is sent, in FIFO order, the moment `connected` fires next (the first connect included, before the reconnect handler runs) — no protocol change: a call-id is assigned only at send time, same as the immediate path. If the socket is torn down (destroyed, or disconnects) before ever connecting, the -queue is drained by `cancelPending`, which still invokes each queued request's -`onError` exactly once, exactly like an in-flight (already-sent) registration +queue is drained by `cancelPending`, which still rejects each queued request's +`Completion` exactly once, exactly like an in-flight (already-sent) registration would. See `QtWebSocketBackend`'s own section below. ### Shared/keyed registration — `registerModelSharedAsync` / `attachModelAsync` @@ -232,12 +246,13 @@ in: | `registerModelSharedAsync(typeId, factory, identity, onRegistered, onError)` | `registerModelShared` | `Bridge::ensureBoundAsync` | | `attachModelAsync(typeId, factory, identity, current, onRegistered, onError)` | `attachModel` | `Bridge::attachHandlerAsync` | -`QtWebSocketBackend` implements both behind the same -`asyncRegistrationEnabled` flag, reusing the same `callId`-keyed pending map -(reply routing is verb-agnostic — a `register`, a shared `register`, and an -`attach` all reply the same way). An empty `identity.primary` degrades to -`registerModelAsync`, mirroring the synchronous methods' degrade-to-private -behaviour. +`QtWebSocketBackend` used to implement both behind the same +`asyncRegistrationEnabled` flag. Since morph#568 it implements neither: both +shapes are reached through `bindModel`, which keeps the same +`asyncRegistrationEnabled` gate, the same `callId`-keyed pending map (reply +routing is verb-agnostic — a `register`, a shared `register`, an `attach` and an +`assign` all reply the same way) and the same degrade-to-private behaviour for +an empty `primary`. Unlike the synchronous `attachModel`'s default implementation, `attachModelAsync` does **not** release `current` itself: an overriding backend @@ -295,19 +310,27 @@ The other three cannot take that same gate. It makes `~Bridge` *block* for the gated span, and each span acquires `_attachMtx` — which the synchronous `Bridge::attachHandler` holds across a full `attachModel` round trip, unbounded on a wire backend. What closes the window instead is the delivery thread. -`QtWebSocketBackend` — the only backend in the tree overriding any of the four — -satisfies the contract by construction rather than by care: it must itself be -used from the Qt event loop thread, and every *reply-driven* callback fires from -`onTextMessage` on that same thread, so the check and the use cannot straddle a -destructor. Its two non-reply paths do not weaken this — a disconnected or no-op -dispatch invokes the callback inline, inside the caller's own frame (which -`Bridge::detail::parkIfInFrame` exists to handle), and `cancelPending` fires the -remainder from `~Bridge` itself, which is not a *concurrent* destructor. - -A backend that replies on its own transport thread therefore reopens #486's -use-after-free. That is a **contract break**, diagnosable from this page and -from `IBackend::registerModelAsync`'s doc comment — not a latent race to be -rediscovered by a sanitizer. + +**Where that stands after morph#568.** No backend overrides the four verbs any +more, so nothing is left for this contract to bind. What the delivery thread +*is*, however, has not changed, and neither has the window: `Bridge` reaches the +structural surface naming `exec::detail::inlineExecutor()`, so a continuation +still runs on whichever thread the backend settled on. For +`QtWebSocketBackend` that is `onTextMessage`, on the Qt event loop thread it +must itself be used from — the same by-construction safety it had before, now +arrived at by the same route the prose used to describe. 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. + +So morph#568 did not close #486's window, and does not claim to. It moved the +decision: which thread a registration continuation runs on is now a value +`Bridge` produces at four call sites rather than an obligation on every backend +author, and a `Bridge` that grows an executor of its own can change it in one +place. Until it does, a backend that replies on its own transport thread would +still reopen #486 — see [How the threading contract becomes +structural](#how-the-threading-contract-becomes-structural). ## The structural registration surface — `bindModel` and `promoteModel` @@ -452,19 +475,150 @@ the `Completion` when its reply arrives. There is no `bool`, no fallback verb and no inline-completion special case to declare: settling the `Completion` inside the dispatch call and settling it a second later from a transport thread are the same code at the call site, because delivery goes through `cbExec` -either way. That is the shape morph#568 moves `QtWebSocketBackend` onto, and -the shape morph#569 moves `SocketBackend` onto. +either way. + +Two backends in the tree take that route: `QtWebSocketBackend` (morph#568) and +`SocketBackend` (morph#569). `SocketBackend`'s case — why it is native rather +than wrapped, and what that does to its reconnect-handler hazard — is set out +under [The structural registration surface, +natively](#the-structural-registration-surface-natively). +`QtWebSocketBackend`'s follows here. + +Its `bindModel` reads: + +- `asyncRegistrationEnabled` unset → `IBackend::bindModel`, the blocking + default. Desktop behaviour, unchanged. +- set → build the envelope the request's shape names, assign a `callId`, send, + and return an unsettled `Completion` that `onTextMessage` settles. A private + bind made before the socket has connected is queued instead; a keyed one + rejects. + +The request-shape mapping is the one `SocketBackend` implements too — empty +`primary` with a zero `current` is a private `register`; empty `primary` with a +live `current` gives that instance up first and then binds privately; a +non-empty `primary` with a zero `current` is a shared `register`; with a live +one it is an `attach`. The two native implementations differ in transport and +in gating, not in what a `BindRequest` shape means. + +**They do differ in gating, and after morph#568 that difference is +observable.** `SocketBackend::bindModel` is unconditionally non-blocking; +`QtWebSocketBackend::bindModel` falls back to the blocking default unless +`asyncRegistrationEnabled` is set. Since morph#568 puts `Bridge` on this +surface, the gate now decides whether `Bridge::registerHandler` returns a +*bound* handler: a blocking `bindModel` settles inside the dispatch frame, so +the handler is bound on return, and a non-blocking one does not, so it is not. +See [What a natively non-blocking backend does to +`registerHandler`](#what-a-natively-non-blocking-backend-does-to-registerhandler) +below — that is a property of the surface, not of either transport. + +Two things that were four-way duplicated collapsed with the verbs. There is one +send path (`sendControl`), so the "encode before recording the pending entry" +invariant and the `env.session` stamp of morph#495 each exist once rather than +four times; and there is one pending map, because `register`, shared +`register`, `attach` and `assign` replies were always matched identically. +(`SocketBackend` reached the same conclusion independently and calls its own +`sendControlAsync`/`_pendingControl`; the Qt member keeps its older +`_pendingRegistrations` name.) + +The single-threaded WASM registration path — the case the four `*Async` verbs +were added for — therefore has **no special case left in the interface**. It is +the ordinary shape of `bindModel` on a backend whose transport is +non-blocking. `tests/qt/test_qt_websocket.cpp` pins it end to end against a real +`RemoteServer` (queue before connect, register-or-attach, re-point, +degrade-to-private, reject on a dead socket, promote), and +`examples/common/testkit/test_wasm_registration_path_native.cpp` pins the same +path through `Bridge`, where a fallback to the blocking verb would abort on the +nested `QEventLoop` `sendSync` needs. + +`QtWebSocketBackend` deliberately does **not** go through +`SynchronousBackendAdapter`: the adapter moves a blocking call to another +thread, and a WASM main thread has no other thread to move it to. + +### What a natively non-blocking backend does to `registerHandler` + +`Bridge::registerHandler` is synchronous and returns a `BridgeHandler`. What it +cannot do is make the *backend* synchronous. Before morph#568 the fallback under +`registerModelAsync` was the blocking `registerModelWithContext`, so on every +backend except a `QtWebSocketBackend` with `asyncRegistrationEnabled` set, the +handler was bound by the time the constructor returned. After morph#568 the +fallback is `bindModel`, so the rule is stated once, structurally: + +> **`registerHandler` returns a bound handler exactly when the backend's +> `bindModel` settles inside the call.** A backend that has not overridden +> `bindModel` gets `IBackend`'s default, which runs the legacy blocking verb and +> settles before returning — bound. A backend that overrode it natively settles +> when its reply lands — **unbound**, and the caller must gate on +> `Bridge::whenBound()` (or on the `onDone` of the async entry points) before +> issuing a call, exactly as the `*Async` path already required. + +`executeVia` fails fast with `"handler not bound"` for a call issued before the +reply arrives; it does not queue. That is unchanged — it is the same failure the +`*Async` path produced, now reachable through one surface instead of two. + +Consequences, as of morph#568: + +| Backend | `bindModel` | `registerHandler` returns | +|---|---|---| +| `LocalBackend`, `SimulatedRemoteBackend`, the eleven test doubles | default (blocking verb) | bound | +| A backend wrapped in `SynchronousBackendAdapter` | non-blocking, on the adapter's executor | **unbound** | +| `QtWebSocketBackend`, `asyncRegistrationEnabled` unset | default (blocking verb) | bound | +| `QtWebSocketBackend`, `asyncRegistrationEnabled` set | native, non-blocking | **unbound** (the WASM case, which is the whole point) | +| `SocketBackend` (morph#569) | native, non-blocking | **unbound** | + +The last row is a behaviour change that morph#568 causes and morph#569 does not: +morph#569 landed while `Bridge` still called the legacy verbs, so its native +`bindModel` had no caller and the row read "bound". It is the first time a +non-Qt embedder sees an unbound handler out of `registerHandler`, and it is why +`docs/spec/core/bridge.md`'s `whenBound()` section is the right place for a +`SocketBackend` caller to start. ### Migration status -No *caller* uses this surface yet. `Bridge` still calls the five synchronous -verbs and prefers the four `*Async` twins, every existing implementor still -compiles unchanged, and the default `bindModel`/`promoteModel` implementations -route to exactly the legacy verb each request shape names — so a backend that -has overridden nothing behaves identically through either surface. On the -implementor side, `SocketBackend` overrides both natively (morph#569) without -changing any of its legacy verbs. The four twins are removed, and the prose -threading contract retired, in morph#571. +| Step | What it does | State | +|---|---|---| +| morph#567 | Adds `bindModel`/`promoteModel` and `SynchronousBackendAdapter`. Nothing else changes. | Landed | +| morph#568 | `QtWebSocketBackend` implements the surface natively and drops all four `*Async` overrides; `Bridge`'s four dispatch sites fall back to it instead of to a synchronous verb. | Landed | +| morph#569 | `SocketBackend` implements the surface natively, keeping every legacy verb on `sendSync`. | Landed | +| morph#570 | The example GUIs and the WASM spike; `Bridge::installReconnectHandler` onto `bindModel`. | Open | +| morph#571 | Removes the four `*Async` verbs; migrates `LocalBackend`, `SimulatedRemoteBackend` and the test doubles. | Open | + +Every existing implementor still compiles unchanged, and the default +`bindModel`/`promoteModel` implementations route to exactly the legacy verb each +request shape names — so a backend that has overridden nothing behaves +identically through either surface. What changed with morph#568 is the *caller*: +after it, every `Bridge` dispatch site has the shape + +```cpp +bool const started = backend->Async(..., onOk, onErr); // removed by morph#571 +if (!started) { + auto completion = backend->bindModel(request, exec::detail::inlineExecutor()); + completion.then(onOk).onError(onErr); +} +``` + +so the path count did not grow: the structural call replaced the synchronous +fallback that used to sit there, and morph#571 deletes the first branch to leave +one. No backend in the tree overrides a `*Async` verb any more, so the second +branch is the one that always runs; the eleven test doubles in +`tests/test_async_registration.cpp` that do override them still exercise the +first, which is what keeps that suite meaningful until morph#571 rewrites it. + +`Bridge::installReconnectHandler` is the one dispatch site morph#568 did **not** +move: its handler still calls the blocking `registerModelShared`/ +`registerModelWithContext`. That is deliberate — moving it changes `Bridge`'s +locking model, not just a call — and it is the reason `SocketBackend`'s +reconnect-handler deadlock hazard survives morph#568 exactly as +[its own section](#the-structural-registration-surface-natively) says it +survives morph#569. + +The executor those call sites name is **`exec::detail::inlineExecutor()`**, which +runs the continuation on the thread that settled it. That is deliberately the +*old* delivery thread, so morph#568 changes no 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 the note under [Threading contract](#threading-contract--the-callbacks-delivery-thread) +and morph#588. ## Error types @@ -1278,17 +1432,18 @@ by `_pendingMtx`, because `cancelPending` can be called from `Bridge` / the parked loop returns with an empty `_pendingReply`, `sendSync` throws `"disconnected"`. See concurrency_and_lifetimes.md. - **`registerModelAsync` is the non-blocking alternative**, opt-in via - `QtWebSocketBackendConfig::asyncRegistrationEnabled` (default `false`, so - every existing embedder keeps `registerModel`'s synchronous behavior - unchanged). A call made before the socket has finished connecting is queued + **`bindModel` is the non-blocking alternative**, opt-in via + `QtWebSocketBackendConfig::asyncRegistrationEnabled` (default `false`, which + makes `bindModel` run `IBackend`'s blocking default, so every existing + embedder keeps `registerModel`'s synchronous behavior unchanged). A private + bind made before the socket has finished connecting is queued (`_queuedRegistrations`) rather than failed, and flushed — each entry assigned a call-id and sent, in FIFO order — from the `connected` slot, the first connect included, before `_connectHandler`'s reconnect-handler counterpart runs. If the backend is destroyed (or the socket disconnects) before that queue is ever flushed, `cancelPending` drains it and still - invokes each entry's `onError` exactly once. See [Asynchronous - registration](#asynchronous-registration--registermodelasync). + rejects each entry's `Completion` exactly once. See [Backends with a + genuinely non-blocking path](#backends-with-a-genuinely-non-blocking-path). - `deregisterModel` — **fire-and-forget**, not synchronous: if `_connected`, it sends a `deregister` envelope and returns immediately without waiting for the ack; if disconnected, it does nothing. This deliberately avoids a nested @@ -1311,11 +1466,14 @@ frame and routes it by `callId`: `deserialize(body)` into the completion's value (deserialisation exceptions become the completion's error), any other kind → `std::runtime_error(message)` into the completion's error. If not found there, `_pendingRegistrations` is - checked next (an async `registerModelAsync` reply — same `callId` - counter/namespace as `execute`, separate map because the reply shape differs): - `ok` → `onRegistered(ModelId{modelId})`, any other kind → `onError(message)`. - A `callId` matching **neither** map (e.g. a late reply for an - already-cancelled call) is dropped silently. + checked next (a non-blocking `bindModel`/`promoteModel` reply — same `callId` + counter/namespace as `execute`, separate map because the reply shape differs; + one map for both verbs, because a `register`, a shared `register`, an `attach` + and an `assign` reply are all matched identically): `ok` → + `resolve(ModelId{modelId})`, any other kind → `reject(runtime_error(message))`, + both **outside** `_pendingMtx`, since settling runs the caller's continuation + and `Bridge`'s executor runs it inline. A `callId` matching **neither** map + (e.g. a late reply for an already-cancelled call) is dropped silently. - A **`callId == 0`** frame is a synchronous control reply (`register` when `asyncRegistrationEnabled` is `false`, or `attach`/`assign`/`instances`); it is stored in `_pendingReply` and quits the parked nested `QEventLoop`. @@ -2049,7 +2207,7 @@ inside the class calls `close()` — no thread it joins can be waiting on it. | `initialReconnectDelay` | `std::chrono::milliseconds` | `500 ms` | | `maxReconnectDelay` | `std::chrono::milliseconds` | `30 s` | | `backoffMultiplier` | `double` | `2.0` | -| `asyncRegistrationEnabled` | `bool` | `false` — opts in to `registerModelAsync` (see [Asynchronous registration](#asynchronous-registration--registermodelasync)); `false` keeps every embedder on `registerModel`'s synchronous behavior. | +| `asyncRegistrationEnabled` | `bool` | `false` — whether `bindModel` may return before the reply. `false` defers to `IBackend::bindModel`, which blocks the Qt thread in a nested `QEventLoop`, keeping every existing embedder's behaviour. `true` is the WASM setting. Not an opt-in to a second set of verbs any more: the continuation exists either way. | ### `QtWebSocketBackend` (namespace `morph::qt`) @@ -2058,14 +2216,15 @@ inside the class calls `close()` — no thread it joins can be waiting on it. | `QtWebSocketBackend(serverUrl, dispatcher = defaultDispatcher(), registry = defaultRegistry(), tls = nullopt, cfg = Config{})` | Opens the socket to `serverUrl` in the constructor. `dispatcher`/`registry` params are accepted but unused (models live on the server). `tls` non-null → `wss://`. `tls` is not declared at all when Qt is built with `QT_NO_SSL` (see above). | | `QtWebSocketBackend(serverUrl, tls, cfg = Config{})` | Overload that skips the unused `dispatcher`/`registry` pair (issue #55): a caller who only needs `tls`/`cfg` reaches them directly, without naming `morph::model::detail::defaultDispatcher()`/`defaultRegistry()` explicitly. Delegates to the main constructor with both defaulted. Not declared on a `QT_NO_SSL` build (no `tls` parameter to distinguish it from the `(serverUrl, cfg)` overload below). | | `QtWebSocketBackend(serverUrl, cfg)` | Overload that skips `dispatcher`/`registry` and `tls` together — the common case for a caller that only wants to set a `Config` field (e.g. `asyncRegistrationEnabled`) over a plaintext `ws://` connection. Delegates to the main constructor with `dispatcher`/`registry` defaulted and (on an SSL-enabled build) `tls = std::nullopt`. | -| `registerModelAsync(typeId, factory, contextKey, onRegistered, onError)` | Returns `false` immediately unless `cfg.asyncRegistrationEnabled` is `true`. Otherwise: assigns a fresh `callId` (the same counter `execute` uses), records the callbacks in `_pendingRegistrations[callId]`, sends `register` with that `callId`, and returns `true`. `onRegistered`/`onError` fire later from `onTextMessage` (or from `cancelPending` on a disconnect) — never synchronously from this call. | +| `bindModel(request, cbExec)` | Defers to `IBackend::bindModel` (blocking) unless `cfg.asyncRegistrationEnabled` is `true`. Otherwise builds the envelope `request`'s shape names — `register`, shared `register`, or `attach` — assigns a fresh `callId` (the same counter `execute` uses), records the promise in `_pendingRegistrations[callId]` and sends. The `Completion` settles later from `onTextMessage` (or from `cancelPending` on a disconnect). A private bind on an unconnected socket is queued in `_queuedRegistrations` instead; a keyed one rejects with `"disconnected"`. | +| `promoteModel(request, cbExec)` | Always non-blocking, with no `Config` gate — `assignPrimary`'s caller is inside a `Completion` chain, so there is no synchronous guarantee to preserve. Sends `assign` through the same path. An empty `primary` or zero `mid` resolves with `request.mid` without sending. | | `waitForConnected(timeoutMs = 5000)` | Pumps the Qt loop until connected or timeout; returns `_connected`. | | `negotiateProtocolVersion()` | Opt-in: sends `hello` synchronously (same nested-`QEventLoop` path as `registerModel`), classifies the reply via `wire::interpretHelloReply`. Throws on an explicit version rejection or a `sendSync` failure. | | `registerModel(typeId, factory)` | Synchronous via nested `QEventLoop`; `factory` ignored. Throws on `err` reply. | | `deregisterModel(mid)` | **Fire-and-forget** — sends only if connected, does not wait for the ack. Carries a non-zero `callId` from the same counter `execute` uses, recorded in `_pendingDeregisters` so `onTextMessage` recognises the unwanted reply and drops it rather than handing it to a parked `sendSync` (issue #65). | | `execute(mid, call, cbExec)` | Assigns a `callId`, sends `execute`, returns a `Completion`. Immediate `DisconnectedError` if not connected. | | `notifyBackendChanged()` | No-op. | -| `cancelPending(exc)` | Drains `_pending` under `_pendingMtx`, delivers `exc` to each state. | +| `cancelPending(exc)` | Drains `_pending`, `_pendingRegistrations` and `_queuedRegistrations` under `_pendingMtx`, then delivers `exc` to each — the exception itself, so a control call rejected by a dropped socket carries the same `DisconnectedError` an `execute` does. | | `setReconnectHandler(handler)` | Stores the handler; invoked on the Qt thread after every *subsequent* connect. `nullptr` clears. | | `setConnectHandler(handler)` | Stores the handler; invoked on the Qt thread after every successful connect, first included. `nullptr` clears. | | `setDisconnectHandler(handler)` | Stores the handler; invoked on the Qt thread whenever the socket drops, before reconnect scheduling. `nullptr` clears. | From 379fb45a13abbcd774b78c854cfd59d699f4e499 Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Sat, 19 Sep 2026 10:46:41 +0200 Subject: [PATCH 06/11] test(qt): make the two local QUrl variables const clang-tidy-diff flagged both under misc-const-correctness: neither url is reassigned after construction. Co-Authored-By: Claude Sonnet 5 --- tests/qt/test_qt_websocket.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/qt/test_qt_websocket.cpp b/tests/qt/test_qt_websocket.cpp index 42059ddde..d092a8ec9 100644 --- a/tests/qt/test_qt_websocket.cpp +++ b/tests/qt/test_qt_websocket.cpp @@ -630,7 +630,7 @@ TEST_CASE( morph::qt::QtWebSocketServer wsServer{*server, 0}; REQUIRE(wsServer.listen()); - QUrl url{QString("ws://127.0.0.1:%1").arg(wsServer.port())}; + const QUrl url{QString("ws://127.0.0.1:%1").arg(wsServer.port())}; morph::qt::QtWebSocketBackend backend{url, morph::model::detail::defaultDispatcher(), morph::model::detail::defaultRegistry(), std::nullopt, morph::qt::QtWebSocketBackend::Config{.asyncRegistrationEnabled = true}}; @@ -667,7 +667,7 @@ TEST_CASE("morph::qt::QtWebSocketBackend: promoteModel files a live instance und morph::qt::QtWebSocketServer wsServer{*server, 0}; REQUIRE(wsServer.listen()); - QUrl url{QString("ws://127.0.0.1:%1").arg(wsServer.port())}; + const QUrl url{QString("ws://127.0.0.1:%1").arg(wsServer.port())}; morph::qt::QtWebSocketBackend backend{url, morph::model::detail::defaultDispatcher(), morph::model::detail::defaultRegistry(), std::nullopt, morph::qt::QtWebSocketBackend::Config{.asyncRegistrationEnabled = true}}; From 52310b9779903bc6a1e4de94d99ab46f09490b7f Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Sat, 19 Sep 2026 11:53:54 +0200 Subject: [PATCH 07/11] ci: repoint bridge.hpp's coverage-allowlist hints after #585's insertions Both entries drifted once the new bindModel/promoteModel machinery was inserted above them: - B6 (`_executeDeadline.count() > 0 && _timeoutScheduler`) moved from line 1453 to 1498, text unchanged. - B11 (`deadlineHandle && schedulerRef`) moved from 1589 to 1610, but that source line is not unique -- it also appears, unrelated, in the .then()/.onError() continuations further down (1634, 1723, present on master too). Repointed to 1610, the specific exception-path occurrence B11's reasoning actually describes, and said so in the entry so the ambiguity doesn't recur silently. Co-Authored-By: Claude Sonnet 5 --- 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 38b0257e5..427c5d848 100644 --- a/scripts/branch_partial_allowlist.json +++ b/scripts/branch_partial_allowlist.json @@ -98,15 +98,15 @@ }, { "file": "include/morph/core/bridge.hpp", - "line": 1453, + "line": 1498, "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": 1589, + "line": 1610, "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:1453 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." + "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:1498 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 1634, 1723) -- 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." }, { "file": "include/morph/core/remote.hpp", From 75e18a74663d4f476c5fcdeafc1e4124bbcccb56 Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Sun, 20 Sep 2026 10:00:08 +0200 Subject: [PATCH 08/11] core: let a backend say whether its caller may wait for a bind (fixes #593) `Bridge::registerHandlerImpl` has one call site for acquiring a model, and after morph#568 and morph#569 two shipped backends need opposite behaviour from it. Both return an unsettled `Completion` from `bindModel`, so from `Bridge`'s side they are indistinguishable: - `morph::net::SocketBackend` must be waited for. Its callers construct a `BridgeHandler` and use it on the next line, and `executeVia` fails fast on `currentId == 0`. - `QtWebSocketBackend` with `asyncRegistrationEnabled` must not be waited for. Its reply is delivered by the Qt event loop of the calling thread, so a wait is a deadlock -- on WASM, a page abort. That is the whole of morph#568. Adds `IBackend::bindWaitPolicy()`, returning `BindWait::kCallerMayBlock` by default and `kCallerMustNotBlock` for the two backends whose callers cannot usefully wait (`QtWebSocketBackend` under the flag, and `SynchronousBackendAdapter`, which exists precisely to move the blocking off the caller's thread and would deadlock against its own strand). Under `kCallerMayBlock`, `registerHandlerImpl` holds its inline window open until the completion settles (`detail::awaitHandoff`) and then publishes the outcome through the same code that already handles a backend settling inline -- so the two cases differ in how long the frame sits still, not in what the caller observes. This is not the `bool` morph#567 deliberately removed. That one chose *which verb to call*, so every call site carried two paths and a backend could be half-migrated. This one chooses nothing: one verb, called unconditionally, one continuation. It says only whether the thread that registered that continuation may stop and wait for it. Measured on this rebased tree (GCC 16.2.1, Debug, MORPH_BUILD_QT=ON MORPH_BUILD_NET=ON): - Before: 99% tests passed, 6 tests failed out of 1833 -- the six `tests/net/` cases morph#593 names. - After: 100% tests passed out of 1835. - Mutation, never wait (`false && ...` at the call site): 7 failures -- the same six, plus the new `kCallerMayBlock` case. - Mutation, always wait (`true || ...`): 6 failures -- the new `kCallerMustNotBlock` case, plus five `tests/qt/` cases that **time out** at 120 s rather than fail, which is the nested-`QEventLoop` deadlock in desktop clothing. Neither fixed setting of that call site is correct, which is the finding morph#593 recorded and this commit acts on. Also repoints `scripts/branch_partial_allowlist.json`'s two `bridge.hpp` line hints (1498 -> 1542, 1610 -> 1654) and the line numbers named inside the second entry's reasoning (1634/1723 -> 1678/1767), which this commit's insertions shifted. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01AbwhcguQFkhvVi2AH19sWk --- docs/spec/concurrency_and_lifetimes.md | 17 +++ docs/spec/core/backend.md | 117 ++++++++++++++++---- include/morph/core/backend.hpp | 94 ++++++++++++++++ include/morph/core/bridge.hpp | 86 ++++++++++++-- include/morph/qt/qt_websocket_backend.hpp | 28 +++++ scripts/branch_partial_allowlist.json | 6 +- tests/test_backend_registration_surface.cpp | 110 ++++++++++++++++++ 7 files changed, 424 insertions(+), 34 deletions(-) diff --git a/docs/spec/concurrency_and_lifetimes.md b/docs/spec/concurrency_and_lifetimes.md index 5d5edb1ba..011e1ac7e 100644 --- a/docs/spec/concurrency_and_lifetimes.md +++ b/docs/spec/concurrency_and_lifetimes.md @@ -281,6 +281,23 @@ that bounded wait into an unbounded one. Four dispositions, by site: `BridgeLifetime` across its whole touch of `this` (`_mtx`, `loadBackend()`). Safe to hold the gate here: nothing in that span calls into consumer code or a blocking backend path, only a mutex and a pointer comparison. + + Since morph#593 that callback may also run **on the registering thread + itself**: unless the backend answers `IBackend::BindWait::kCallerMustNotBlock`, + `registerHandlerImpl` waits for the bind completion and then delivers the + outcome from its own frame, so `registerHandler` returns a bound handler. That + reinstates, for one statement, exactly the blocking window every backend had + before morph#568, when the fallback was the synchronous + `registerModelWithContext`: a thread parked inside `registerHandler` is a + thread not running `~Bridge`, and a *different* thread destroying the `Bridge` + while `registerHandler` is still on this one was already a misuse then and is + no more possible now. What is new is only that the parking is visible in + `Bridge` rather than inside the backend verb. The two + `kCallerMustNotBlock` backends never park at all, which is the point: for + `QtWebSocketBackend` under `asyncRegistrationEnabled` the reply arrives on the + parked thread's own event loop, so parking would not be a slow teardown but a + deadlock — the same shape of objection that rules a gate out for the reconnect + handler below. - **`installReconnectHandler`'s reconnect callback.** Left as a `liveness()` check, deliberately not moved to `BridgeLifetime` — this is the case the first paragraph above warns about. The handler runs on the backend's diff --git a/docs/spec/core/backend.md b/docs/spec/core/backend.md index 7f807e7e0..f5a7d7a6a 100644 --- a/docs/spec/core/backend.md +++ b/docs/spec/core/backend.md @@ -28,6 +28,7 @@ and react to backend changes. - [The dispatch struct — `ActionCall`](#the-dispatch-struct--actioncall) - [The abstract interface — `IBackend`](#the-abstract-interface--ibackend) - [Connect/disconnect notifications](#connectdisconnect-notifications) +- [Waiting for a bind — `bindWaitPolicy`](#waiting-for-a-bind--bindwaitpolicy) - [Asynchronous registration — `registerModelAsync`](#asynchronous-registration--registermodelasync) - [The structural registration surface — `bindModel` and `promoteModel`](#the-structural-registration-surface--bindmodel-and-promotemodel) - [What a natively non-blocking backend does to `registerHandler`](#what-a-natively-non-blocking-backend-does-to-registerhandler) @@ -80,6 +81,7 @@ holds a `unique_ptr` and delegates all model operations to it. | `registerModelAsync(typeId, factory, contextKey, onRegistered, onError)` | Optional non-blocking counterpart to `registerModelWithContext`. Returns `false` by default, and since morph#568 no backend overrides it, so it always does; `Bridge::registerHandler()` then falls back to `bindModel`. Removed by morph#571. See [Asynchronous registration](#asynchronous-registration--registermodelasync). | | `bindModel(request, cbExec)` | Acquires a model instance and returns a `Completion` delivered on `cbExec`. One verb covering `registerModelWithContext`, `registerModelShared` and `attachModel`, selected by the request's shape. The preferred surface — see [The structural registration surface](#the-structural-registration-surface--bindmodel-and-promotemodel). | | `promoteModel(request, cbExec)` | Files an already-live instance under a key and returns a `Completion` delivered on `cbExec`. The structural counterpart of `assignPrimary`. | +| `bindWaitPolicy()` | Whether a caller may block its own thread until a `bindModel`/`promoteModel` completion settles. `BindWait::kCallerMayBlock` by default. The only framework caller is `Bridge::registerHandlerImpl`; see [Waiting for a bind — `bindWaitPolicy`](#waiting-for-a-bind--bindwaitpolicy). | | `deregisterModel(mid)` | Removes the model identified by `mid`. | | `execute(mid, call, cbExec)` | Dispatches `call` against the model identified by `mid`. Returns a `Completion>`. | | `notifyBackendChanged()` | Called by `Bridge::switchBackend()` after all handlers are re-registered. | @@ -447,6 +449,14 @@ natively](#the-structural-registration-surface-natively). executor, so the caller returns immediately with an unresolved `Completion`. A single-threaded WASM main thread has no such executor to offer, which is why morph#568 puts `QtWebSocketBackend` on the surface natively instead. +- **It answers `bindWaitPolicy()` itself rather than forwarding it**, with + `kCallerMustNotBlock`. `bindModel`/`promoteModel` are the two verbs it + reshapes, so the policy describing them describes the adapter and not what it + wraps. A caller that waited would pay back exactly the blocking cost the + adapter was interposed to move, and would deadlock outright if it happened to + be running on `blockingExec`. This is why a `Bridge` over an adapter returns + an unbound handler while a `Bridge` over `SocketBackend` — also non-blocking + — does not. - **The executor is required.** An adapter that ran the call inline when handed nothing would be a `bindModel` that blocks on some configurations and not others — contract by configuration, which is what is being removed. @@ -543,34 +553,91 @@ backend except a `QtWebSocketBackend` with `asyncRegistrationEnabled` set, the handler was bound by the time the constructor returned. After morph#568 the fallback is `bindModel`, so the rule is stated once, structurally: -> **`registerHandler` returns a bound handler exactly when the backend's -> `bindModel` settles inside the call.** A backend that has not overridden -> `bindModel` gets `IBackend`'s default, which runs the legacy blocking verb and -> settles before returning — bound. A backend that overrode it natively settles -> when its reply lands — **unbound**, and the caller must gate on -> `Bridge::whenBound()` (or on the `onDone` of the async entry points) before -> issuing a call, exactly as the `*Async` path already required. +> **`registerHandler` returns a bound handler unless the backend says the +> caller must not wait.** A backend that has not overridden `bindModel` gets +> `IBackend`'s default, which runs the legacy blocking verb and settles before +> returning — bound, with no wait. A backend that overrode `bindModel` natively +> settles when its reply lands, and `registerHandlerImpl` **waits for it** — +> still bound — unless that backend answers +> `BindWait::kCallerMustNotBlock`, in which case the handler is returned +> **unbound** and the caller must gate on `Bridge::whenBound()` (or on the +> `onDone` of the async entry points) before issuing a call, exactly as the +> `*Async` path already required. + +The wait is morph#593's correction to the rule as morph#568 first wrote it. The +rule then read "bound exactly when `bindModel` settles inside the call", which +made *how the backend is implemented* decide what a synchronous, public entry +point returns — and `SocketBackend` (morph#569) changed its implementation +without any intent to change that. See +[Waiting for a bind — `bindWaitPolicy`](#waiting-for-a-bind--bindwaitpolicy). `executeVia` fails fast with `"handler not bound"` for a call issued before the reply arrives; it does not queue. That is unchanged — it is the same failure the `*Async` path produced, now reachable through one surface instead of two. -Consequences, as of morph#568: +Consequences, as of morph#568 and morph#593: -| Backend | `bindModel` | `registerHandler` returns | -|---|---|---| -| `LocalBackend`, `SimulatedRemoteBackend`, the eleven test doubles | default (blocking verb) | bound | -| A backend wrapped in `SynchronousBackendAdapter` | non-blocking, on the adapter's executor | **unbound** | -| `QtWebSocketBackend`, `asyncRegistrationEnabled` unset | default (blocking verb) | bound | -| `QtWebSocketBackend`, `asyncRegistrationEnabled` set | native, non-blocking | **unbound** (the WASM case, which is the whole point) | -| `SocketBackend` (morph#569) | native, non-blocking | **unbound** | +| Backend | `bindModel` | `bindWaitPolicy()` | `registerHandler` returns | +|---|---|---|---| +| `LocalBackend`, `SimulatedRemoteBackend`, the eleven test doubles | default (blocking verb) | `kCallerMayBlock` (default) | bound; the wait finds the outcome already there | +| A backend wrapped in `SynchronousBackendAdapter` | non-blocking, on the adapter's executor | `kCallerMustNotBlock` | **unbound** | +| `QtWebSocketBackend`, `asyncRegistrationEnabled` unset | default (blocking verb) | `kCallerMayBlock` | bound | +| `QtWebSocketBackend`, `asyncRegistrationEnabled` set | native, non-blocking | `kCallerMustNotBlock` | **unbound** (the WASM case, which is the whole point) | +| `SocketBackend` (morph#569) | native, non-blocking | `kCallerMayBlock` (default) | bound, after waiting for the I/O thread's reply | + +### Waiting for a bind — `bindWaitPolicy` + +`Bridge::registerHandlerImpl` has exactly one call site for acquiring a model, +and after morph#568 and morph#569 two shipped backends needed opposite +behaviour from it: -The last row is a behaviour change that morph#568 causes and morph#569 does not: +| Backend | What `bindModel` does | What the call site must do | Why | +|---|---|---|---| +| `SocketBackend` | returns an unsettled `Completion`; the I/O thread settles it | **wait** | its callers construct a `BridgeHandler` and use it on the next line; `executeVia` fails fast on `currentId == 0` | +| `QtWebSocketBackend`, `asyncRegistrationEnabled` set | returns an unsettled `Completion` | **not wait** | the reply is delivered by the Qt event loop of the calling thread, so a wait is a deadlock — on WASM, a page abort | + +From the `Completion` alone the two are indistinguishable, and the surface +morph#567 introduced had removed the only signal that told them apart (the +`*Async` verbs' `bool` return). Measured, not argued: on morph#568's head, six +`tests/net/` cases failed with `"handler not bound"`, and forcing the *other* +answer at that call site instead hung five `tests/qt/` cases on the nested +`QEventLoop`. Neither fixed setting of the call site is correct. + +`IBackend::bindWaitPolicy()` restores exactly one bit: + +- `BindWait::kCallerMayBlock` (the default) — the completion settles without + the calling thread's participation, either inside the call or on a thread the + caller does not own. A backend that answers this **must** settle every + `Completion` it hands out exactly once without any further call from the + caller, including on transport failure and on `cancelPending`/destruction. +- `BindWait::kCallerMustNotBlock` — waiting is either impossible (the reply + needs the caller's own event loop) or pointless (`SynchronousBackendAdapter`, + which exists to move the blocking elsewhere, and would deadlock if the caller + happened to be running on its executor). + +It is deliberately not the `bool` morph#567 removed. That `bool` chose *which +verb to call*, so every call site carried two paths and a backend could be +half-migrated; this one chooses nothing. There is still exactly one verb, called +unconditionally, and exactly one continuation — the only question is whether the +thread that registered that continuation is allowed to stop and wait for it. +Only a synchronous entry point that must return a bound instance asks: +`registerHandlerImpl` is the sole framework caller, and the asynchronous entry +points (`attachHandlerAsync`, `ensureBoundAsync`, `assignHandlerPrimary`) never +wait and never consult it. + +The wait is unbounded by design. A timeout would make "is this handler bound +when `registerHandler` returns" depend on how fast the network was, which is the +non-determinism the synchronous contract exists to exclude; a backend that +breaks its own settle-exactly-once contract therefore hangs here rather than +silently handing back an unbound handler. + +`SocketBackend`'s row above is the one morph#568 changed and morph#569 did not: morph#569 landed while `Bridge` still called the legacy verbs, so its native -`bindModel` had no caller and the row read "bound". It is the first time a -non-Qt embedder sees an unbound handler out of `registerHandler`, and it is why -`docs/spec/core/bridge.md`'s `whenBound()` section is the right place for a -`SocketBackend` caller to start. +`bindModel` had no caller. morph#593 puts that row back to "bound" rather than +asking every existing non-Qt embedder to start gating on `whenBound()` — +`docs/spec/core/bridge.md`'s `whenBound()` section remains the right place for a +caller that wants to gate anyway, and is required for the two +`kCallerMustNotBlock` rows. ### Migration status @@ -579,6 +646,7 @@ non-Qt embedder sees an unbound handler out of `registerHandler`, and it is why | morph#567 | Adds `bindModel`/`promoteModel` and `SynchronousBackendAdapter`. Nothing else changes. | Landed | | morph#568 | `QtWebSocketBackend` implements the surface natively and drops all four `*Async` overrides; `Bridge`'s four dispatch sites fall back to it instead of to a synchronous verb. | Landed | | morph#569 | `SocketBackend` implements the surface natively, keeping every legacy verb on `sendSync`. | Landed | +| morph#593 | Adds `IBackend::bindWaitPolicy()`, the one signal morph#567's surface left the call site without. Fixes the `"handler not bound"` regression morph#568 caused in `SocketBackend`. | Landed | | morph#570 | The example GUIs and the WASM spike; `Bridge::installReconnectHandler` onto `bindModel`. | Open | | morph#571 | Removes the four `*Async` verbs; migrates `LocalBackend`, `SimulatedRemoteBackend` and the test doubles. | Open | @@ -2112,6 +2180,7 @@ inside the class calls `close()` — no thread it joins can be waiting on it. | `registerModelWithContext` | `virtual ModelId registerModelWithContext(const string&, function()>, string_view)` | Default: drops `contextKey`, calls `registerModel`. | | `bindModel` | `virtual Completion bindModel(BindRequest, IExecutor& cbExec)` | Default: runs `bindModelBlocking` inline and settles. See [The structural registration surface](#the-structural-registration-surface--bindmodel-and-promotemodel). | | `promoteModel` | `virtual Completion promoteModel(PromoteRequest, IExecutor& cbExec)` | Default: calls `assignPrimary` inline and settles with `request.mid`. | +| `bindWaitPolicy` | `virtual BindWait bindWaitPolicy() const noexcept` | Default: `BindWait::kCallerMayBlock`. Whether a caller may block until a `bindModel`/`promoteModel` completion settles. Read only by `Bridge::registerHandlerImpl`. | | `bindModelBlocking` | `ModelId bindModelBlocking(BindRequest)` | Non-virtual. Routes a `BindRequest` to `registerModelWithContext` / `registerModelShared` / `attachModel` by its shape; blocks. Shared by the default `bindModel` and by `SynchronousBackendAdapter`. | | `deregisterModel` | `virtual void deregisterModel(ModelId)` | Pure virtual. | | `execute` | `virtual Completion> execute(ModelId, ActionCall, IExecutor*)` | Pure virtual. | @@ -2135,6 +2204,13 @@ inside the class calls `close()` — no thread it joins can be waiting on it. | `PromoteRequest::typeId` | `std::string` | Model type id — the directory's first key component. | | `PromoteRequest::primary` | `std::string` | Key to file `mid` under. | +### `detail::BindWait` + +| Enumerator | Meaning | +|---|---| +| `kCallerMayBlock` | Default. The completion settles without the calling thread's participation; a caller may wait for it. Implies the backend settles every `Completion` exactly once, unprompted. | +| `kCallerMustNotBlock` | Waiting is impossible (the reply needs the caller's own event loop) or pointless (`SynchronousBackendAdapter`). The caller registers its continuation and returns. | + ### `SynchronousBackendAdapter` | Method | Notes | @@ -2142,6 +2218,7 @@ inside the class calls `close()` — no thread it joins can be waiting on it. | `SynchronousBackendAdapter(shared_ptr inner, IExecutor& blockingExec)` | Throws `std::invalid_argument` if `inner` is null. `blockingExec` is `MORPH_LIFETIMEBOUND` and must keep running tasks until the destructor's wait completes. | | `wrapped()` | The wrapped backend; never null. | | `bindModel(request, cbExec)` | Posts `inner->bindModelBlocking(request)` onto the control strand; settles the returned `Completion` on `cbExec`. Never blocks the caller. | +| `bindWaitPolicy()` | `BindWait::kCallerMustNotBlock`, always. Not forwarded: it describes the two verbs the adapter reshapes. | | `promoteModel(request, cbExec)` | Posts `inner->assignPrimary(...)` onto the control strand; resolves with `request.mid`. | | every other `IBackend` verb | Forwarded to `inner` unchanged, including the four `*Async` twins — wrapping a backend that has a non-blocking path must not take it away. | diff --git a/include/morph/core/backend.hpp b/include/morph/core/backend.hpp index 5eaf4a176..d2d3a485a 100644 --- a/include/morph/core/backend.hpp +++ b/include/morph/core/backend.hpp @@ -6,6 +6,7 @@ #include #include #include +#include #include #include #include @@ -133,6 +134,55 @@ struct PromoteRequest { std::string primary; }; +/// @brief Whether a caller holding an unsettled `bindModel`/`promoteModel` +/// `Completion` may block its own thread until that `Completion` +/// settles. +/// +/// This is the one thing `bindModel`'s signature cannot say, and morph#593 is +/// what happens when it is not said: two shipped backends both return an +/// unsettled `Completion` from `bindModel`, and `Bridge::registerHandlerImpl` +/// — a synchronous entry point that hands its caller a `BridgeHandler` usable +/// on the next line — must wait for one of them and must not wait for the +/// other. From the `Completion` alone the two are indistinguishable. +/// +/// It is deliberately **not** the `bool` the surface removed (see "The +/// structural registration surface" below, point 1). That `bool` chose +/// *which verb to call*, so every call site carried two paths and a backend +/// could be half-migrated. This one chooses nothing: there is still exactly +/// one verb, called unconditionally, and exactly one continuation. It says +/// only whether the thread that issued the call is allowed to stop and wait +/// for the continuation it already registered. +enum class BindWait : std::uint8_t { + /// @brief The completion settles without the calling thread's + /// participation — inside the call, or on a thread the caller does + /// not own. A caller may block until it settles. + /// + /// The default, and correct for every backend that has not overridden + /// `bindModel` (the default settles before it returns) as well as for + /// `SocketBackend`, whose I/O thread settles the completion. A backend + /// that returns this **must** settle every `Completion` it hands out + /// exactly once without further calls from the caller — including on + /// transport failure and on `cancelPending`/destruction — or a caller that + /// waits will wait forever. + kCallerMayBlock, + + /// @brief The completion cannot settle while the calling thread is blocked + /// in a wait, or blocking it would defeat the point of this + /// backend. A caller must register its continuation and return. + /// + /// Two backends say this, for two different reasons: + /// + /// - `QtWebSocketBackend` with `Config::asyncRegistrationEnabled` set: the + /// reply arrives through the Qt event loop of the thread that issued the + /// call, so waiting is a deadlock. On a WASM main thread it aborts the + /// page (morph#568), which is the case that surface exists for. + /// - `SynchronousBackendAdapter`: it exists precisely to move a blocking + /// call off the caller's thread, so a caller that then waits for it has + /// bought nothing — and if the caller happens to be running on the + /// adapter's own executor, has deadlocked. + kCallerMustNotBlock, +}; + /// @brief Abstract interface for execution backends (local, remote, …). /// /// A backend owns model instances and dispatches actions against them. @@ -494,6 +544,18 @@ struct IBackend { // implementations below, or without blocking the caller at all // through `SynchronousBackendAdapter`. // + // What morph#593 established is that removing *that* bool also + // removed something else the call site needed and that is not the + // same question: whether the thread that called `bindModel` is + // allowed to wait for the continuation it just registered. Two + // shipped backends return an unsettled `Completion` and give opposite + // answers (`SocketBackend`: yes, its I/O thread settles it; + // `QtWebSocketBackend` under `asyncRegistrationEnabled`: no, waiting + // deadlocks the event loop the reply arrives on). `bindWaitPolicy()` + // below restores exactly that one bit and nothing else — it never + // selects a verb, so the "second path" the removed bool created does + // not come back with it. + // // 2. **The delivery thread is a parameter.** `Completion` posts its // handlers to the executor it was built with, so the continuation runs // where @p cbExec says and nowhere else — the backend does not choose. @@ -579,6 +641,24 @@ struct IBackend { } // NOLINTEND(performance-unnecessary-value-param) + /// @brief Whether a caller may block until this backend's + /// `bindModel`/`promoteModel` completions settle. + /// + /// Answers the one question `Completion` cannot: an unsettled `Completion` + /// looks the same whether the reply is coming from a thread the caller does + /// not own or from the caller's own event loop. Only a synchronous entry + /// point that must hand back a *bound* instance asks it — + /// `Bridge::registerHandlerImpl` is the single caller in the framework; the + /// asynchronous entry points never wait and never consult it. + /// + /// A backend that returns `kCallerMayBlock` (the default) commits to + /// settling every `Completion` it returns exactly once without any further + /// call from the caller. Every backend that does not override `bindModel` + /// satisfies that trivially: the default settles before it returns. + /// + /// @return `BindWait::kCallerMayBlock` unless overridden. + [[nodiscard]] virtual BindWait bindWaitPolicy() const noexcept { return BindWait::kCallerMayBlock; } + /// @brief Runs @p request against the legacy synchronous verbs, blocking. /// /// Factored out of `bindModel`'s default implementation so that @@ -885,6 +965,20 @@ class SynchronousBackendAdapter : public detail::IBackend { }); } + /// @brief This adapter's whole purpose is that the caller does not block. + /// + /// Not forwarded to the wrapped backend, unlike everything below: + /// `bindModel`/`promoteModel` are the two verbs this adapter *reshapes*, so + /// the policy describing them describes this adapter, not what it wraps. A + /// caller that waited would pay exactly the blocking cost the adapter was + /// interposed to move elsewhere, and — if it happens to be running on + /// `blockingExec` — would deadlock against the strand it is waiting on. + /// + /// @return `BindWait::kCallerMustNotBlock`, always. + [[nodiscard]] detail::BindWait bindWaitPolicy() const noexcept override { + return detail::BindWait::kCallerMustNotBlock; + } + // ── Everything else is forwarded unchanged ─────────────────────────── // // A decorator has to forward every verb it does not reshape, including the diff --git a/include/morph/core/bridge.hpp b/include/morph/core/bridge.hpp index 12962c030..518dade79 100644 --- a/include/morph/core/bridge.hpp +++ b/include/morph/core/bridge.hpp @@ -6,6 +6,7 @@ #include #include #include +#include #include #include #include @@ -278,6 +279,12 @@ struct ParkedOutcome { struct AsyncDispatchHandoff { /// @brief Guards every other field; never held across `onDone` or `_attachMtx`. std::mutex mtx; + /// @brief Signalled once `fired` is set, for `awaitHandoff`'s benefit. + /// + /// Only a dispatcher that deliberately waits (`awaitHandoff`) ever blocks on + /// this; the three asynchronous dispatch sites never do, so for them the + /// `notify_all` in `parkIfInFrame` is an uncontended no-op. + std::condition_variable settled; /// @brief `true` while the backend's dispatch call is still on the caller's stack. bool inFrame = true; /// @brief Set once either callback has claimed the outcome. @@ -305,17 +312,25 @@ struct AsyncDispatchHandoff { /// `false` if the caller owns the outcome and should deliver it itself. inline bool parkIfInFrame(AsyncDispatchHandoff& handoff, bool succeeded, ::morph::exec::detail::ModelId modelId, std::exception_ptr failure) { - std::scoped_lock const guard{handoff.mtx}; - if (handoff.fired) { - // A backend is contractually allowed exactly one callback per dispatch; - // swallow a second one rather than reporting twice. - return true; + bool inFrame = false; + { + std::scoped_lock const guard{handoff.mtx}; + if (handoff.fired) { + // A backend is contractually allowed exactly one callback per dispatch; + // swallow a second one rather than reporting twice. + return true; + } + handoff.fired = true; + handoff.succeeded = succeeded; + handoff.modelId = modelId; + handoff.failure = std::move(failure); + inFrame = handoff.inFrame; } - handoff.fired = true; - handoff.succeeded = succeeded; - handoff.modelId = modelId; - handoff.failure = std::move(failure); - return handoff.inFrame; + // Outside the lock: a waiter in `awaitHandoff` re-acquires `mtx` the moment + // it wakes, and notifying while still holding it makes it wake only to block + // again. + handoff.settled.notify_all(); + return inFrame; } /// @brief Closes the inline window and takes whatever a callback parked. @@ -336,6 +351,35 @@ inline std::optional claimHandoff(AsyncDispatchHandoff& handoff) return ParkedOutcome{.succeeded = handoff.succeeded, .modelId = handoff.modelId, .failure = handoff.failure}; } +/// @brief Holds the inline window open until a callback parks an outcome in it, +/// then closes it and takes that outcome. +/// +/// `claimHandoff`'s blocking twin, and the whole of what +/// `BindWait::kCallerMayBlock` buys: the dispatching frame stops and waits, so a +/// reply that lands on the backend's own thread is still delivered *by this +/// frame*, on this thread, before the dispatching call returns — which is +/// exactly what the frame does for a backend that settled inline. The outcome +/// therefore takes the same path in both cases (published here, rethrown here), +/// instead of one path for "settled in frame" and another for "settled later". +/// +/// Only ever called on a backend that returned `BindWait::kCallerMayBlock`, +/// whose contract is that the completion settles without this thread doing +/// anything. There is deliberately no timeout: a bounded wait would make +/// "is the handler bound when `registerHandler` returns" depend on how fast the +/// network was, which is the non-determinism the synchronous contract exists to +/// exclude. A backend that violates its own settle-exactly-once contract hangs +/// here rather than silently returning an unbound handler, and a hang is the +/// diagnosable failure of those two. +/// +/// @param handoff Handoff slot created by the dispatching frame. +/// @return The outcome a callback parked; never `std::nullopt`. +inline std::optional awaitHandoff(AsyncDispatchHandoff& handoff) { + std::unique_lock guard{handoff.mtx}; + handoff.settled.wait(guard, [&handoff] { return handoff.fired; }); + handoff.inFrame = false; + return ParkedOutcome{.succeeded = handoff.succeeded, .modelId = handoff.modelId, .failure = handoff.failure}; +} + /// @brief The gate that makes "the `Bridge` is still there" and "call into it" /// a single, indivisible step. /// @@ -1942,7 +1986,27 @@ class Bridge { } onFailed(detail::describeFailure(failure)); }); - if (auto parked = detail::claimHandoff(*handoff)) { + // `registerHandler` is a synchronous entry point: its caller + // constructs a `BridgeHandler` and uses it on the next line, and + // `executeVia` fails fast on an unbound `currentId`. So unless the + // backend says its completion cannot settle while this thread waits, + // this frame waits for it -- and the outcome is then published and + // rethrown by exactly the code below that already handles a backend + // that settled inline. The two cases differ in how long this frame + // sits still, not in what the caller observes. + // + // `kCallerMustNotBlock` is the exception, and it is the *reason* it is + // an exception that matters: a `QtWebSocketBackend` with + // `asyncRegistrationEnabled` set delivers its reply through the Qt + // event loop of this very thread, so waiting here is a deadlock, not a + // delay -- on a WASM main thread it aborts the page (morph#568). Such a + // backend's caller gets an unbound handler and must gate on + // `whenBound()`, exactly as the `*Async` path already required of it. + // See `IBackend::bindWaitPolicy` and morph#593. + auto parked = backend->bindWaitPolicy() == ::morph::backend::detail::BindWait::kCallerMayBlock + ? detail::awaitHandoff(*handoff) + : detail::claimHandoff(*handoff); + if (parked) { if (!parked->succeeded) { // Settles the waiters queued during the window above before // unwinding, so a concurrent whenBound() is rejected rather diff --git a/include/morph/qt/qt_websocket_backend.hpp b/include/morph/qt/qt_websocket_backend.hpp index 1081f29d3..16dc9dff7 100644 --- a/include/morph/qt/qt_websocket_backend.hpp +++ b/include/morph/qt/qt_websocket_backend.hpp @@ -267,6 +267,34 @@ class QtWebSocketBackend : public ::morph::backend::detail::IBackend { ::morph::async::Completion<::morph::exec::detail::ModelId> bindModel(::morph::backend::detail::BindRequest request, ::morph::exec::IExecutor& cbExec) override; + /// @brief Whether a caller may block waiting for this backend's + /// `bindModel`/`promoteModel` completions. + /// + /// `kCallerMustNotBlock` exactly when `Config::asyncRegistrationEnabled` is + /// set, because that is exactly when a completion is settled by + /// `onTextMessage` — a Qt slot, delivered by the event loop of the thread + /// that called `bindModel`. A caller blocked in a wait is not running that + /// event loop, so the reply it is waiting for can never arrive: the + /// deadlock morph#568 exists to remove, which on a WASM main thread aborts + /// the page outright. + /// + /// With the flag unset this backend's `bindModel` is `IBackend`'s default, + /// which settles inside the call, so `kCallerMayBlock` is both true and + /// free: the caller's wait finds the outcome already parked and returns + /// without sleeping. + /// + /// Note which way round this reads. It does not say "registration is + /// asynchronous" — `SocketBackend`'s is too, and it answers + /// `kCallerMayBlock` because a separate I/O thread settles its completions. + /// It says only that *this* thread must not stop and wait. See morph#593. + /// + /// @return `kCallerMustNotBlock` when `Config::asyncRegistrationEnabled` is + /// set, `kCallerMayBlock` otherwise. + [[nodiscard]] ::morph::backend::detail::BindWait bindWaitPolicy() const noexcept override { + return _cfg.asyncRegistrationEnabled ? ::morph::backend::detail::BindWait::kCallerMustNotBlock + : ::morph::backend::detail::BindWait::kCallerMayBlock; + } + /// @brief Sends a shared (register-or-attach) `register` and blocks for the reply. /// /// An empty primary degrades to the private path. diff --git a/scripts/branch_partial_allowlist.json b/scripts/branch_partial_allowlist.json index 427c5d848..d05b1ce80 100644 --- a/scripts/branch_partial_allowlist.json +++ b/scripts/branch_partial_allowlist.json @@ -62,7 +62,7 @@ }, { "file": "include/morph/util/rational.hpp", - "line": 1498, + "line": 1542, "source": "if (ctx.begin() == ctx.end() || *ctx.begin() == '}') {", "reason": "The ctx.begin() == ctx.end() true arm is unreachable. This is the textbook cppreference-style custom-formatter parse() idiom. Empirically verified on this toolchain (libc++, via a standalone probe compiled with clang++ -std=c++23 -stdlib=libc++): for both std::format(\"{}\", x) and std::format(\"{:}\", x), ctx.end() always points past the terminating '}', so ctx.begin() == ctx.end() is false and the terminator is always reachable via *ctx.begin() == '}' -- matching the observed 0/14 split exactly. std::format's top-level parser (and std::vformat's, which performs the same replacement-field validation before dispatching to a type's parse()) rejects an unterminated '{...' before ever calling into formatter::parse, so this function is never invoked with an already-exhausted range. The first disjunct is defensive boilerplate that this standard library's implementation (and the standard's own guarantee about validated replacement fields) makes structurally unreachable." }, @@ -104,9 +104,9 @@ }, { "file": "include/morph/core/bridge.hpp", - "line": 1610, + "line": 1654, "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:1498 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 1634, 1723) -- 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 bridge.hpp:1542 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 1678, 1767) -- 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." }, { "file": "include/morph/core/remote.hpp", diff --git a/tests/test_backend_registration_surface.cpp b/tests/test_backend_registration_surface.cpp index 244d32d23..eba4df775 100644 --- a/tests/test_backend_registration_surface.cpp +++ b/tests/test_backend_registration_surface.cpp @@ -27,6 +27,7 @@ #include #include #include +#include #include #include #include @@ -569,3 +570,112 @@ TEST_CASE( "cancelPending", "setReconnectHandler", "setConnectHandler", "setDisconnectHandler", "setSession:pal"}); } + +// ── `bindWaitPolicy`: the one bit `Completion` cannot carry (morph#593) ────── +// +// Two backends both return an unsettled `Completion` from `bindModel`, and +// `Bridge::registerHandler` — a synchronous entry point whose caller uses the +// handler on the next line — must wait for one and must not wait for the other. +// The pair of cases below pins both answers at the one call site that asks. +// +// The 200 ms reply delay is deliberate and is what makes each case *fail* under +// the opposite policy: it is far longer than the microseconds a non-waiting +// `registerHandler` takes to return, so "bound on return" cannot happen by luck +// and "unbound on return" cannot be a lost race. Neither assertion's passing +// direction depends on the exact value — the waiting case waits however long the +// reply takes, and the non-waiting case polls until it arrives. + +namespace { + +constexpr auto kBindReplyDelay = std::chrono::milliseconds{200}; + +/// @brief A backend whose bind reply arrives, later, from a thread the caller +/// does not own — `morph::net::SocketBackend`'s shape. +/// +/// Derives from `RecordingBackend` for the verbs `Bridge` needs it to have, but +/// answers `false` to `registerModelAsync` (`RecordingBackend` answers `true`) +/// so that `Bridge::registerHandlerImpl` reaches the structural surface rather +/// than the legacy async verb morph#571 removes. +struct TransportThreadBackend : RecordingBackend { + /// @brief Policy this double reports; the thing under test. + morph::backend::detail::BindWait policy = morph::backend::detail::BindWait::kCallerMayBlock; + /// @brief The "transport" that settles the bind, asserted to be another thread. + std::thread transport; + /// @brief Thread `bindModel` was called on. + std::thread::id callerThread; + /// @brief `true` if the completion settled somewhere other than `callerThread`. + std::atomic settledOffCallerThread{false}; + + TransportThreadBackend() = default; + TransportThreadBackend(const TransportThreadBackend&) = delete; + TransportThreadBackend& operator=(const TransportThreadBackend&) = delete; + TransportThreadBackend(TransportThreadBackend&&) = delete; + TransportThreadBackend& operator=(TransportThreadBackend&&) = delete; + + ~TransportThreadBackend() override { + if (transport.joinable()) { + transport.join(); + } + } + + bool registerModelAsync(const std::string& /*typeId*/, + std::function()> /*factory*/, + std::string_view /*contextKey*/, std::function /*onRegistered*/, + std::function /*onError*/) override { + return false; + } + + ModelCompletion bindModel(BindRequest /*request*/, morph::exec::IExecutor& cbExec) override { + auto [completion, promise] = ModelCompletion::makeSettleable(&cbExec); + callerThread = std::this_thread::get_id(); + transport = std::thread{[this, kept = std::make_shared(std::move(promise))] { + std::this_thread::sleep_for(kBindReplyDelay); + settledOffCallerThread.store(std::this_thread::get_id() != callerThread); + kept->resolve(ModelId{99}); + }}; + return std::move(completion); + } + + [[nodiscard]] morph::backend::detail::BindWait bindWaitPolicy() const noexcept override { return policy; } +}; + +} // namespace + +TEST_CASE("morph::bridge::Bridge: registerHandler waits out a kCallerMayBlock backend's bind and returns bound", + "[backend][registration-surface][bridge]") { + auto owned = std::make_unique(); + auto* backend = owned.get(); + backend->policy = morph::backend::detail::BindWait::kCallerMayBlock; + + morph::bridge::Bridge bridge{std::move(owned)}; + auto binding = bridge.registerHandler(); + + // No polling, no drain: the constructor did not return until the reply + // landed. This is the contract every non-Qt embedder had before morph#568 + // and that morph#586 took away from `SocketBackend`. + REQUIRE(morph::bridge::Bridge::isBound(binding)); + REQUIRE(binding->currentId.load() == 99U); + // ...and it was a *wait*, not a synchronous backend: the value was produced + // on a thread `registerHandler` does not own. + REQUIRE(backend->settledOffCallerThread.load()); +} + +TEST_CASE("morph::bridge::Bridge: registerHandler does not wait for a kCallerMustNotBlock backend", + "[backend][registration-surface][bridge]") { + auto owned = std::make_unique(); + auto* backend = owned.get(); + backend->policy = morph::backend::detail::BindWait::kCallerMustNotBlock; + + morph::bridge::Bridge bridge{std::move(owned)}; + auto binding = bridge.registerHandler(); + + // Returned while the reply is still 200 ms away. For `QtWebSocketBackend` + // under `asyncRegistrationEnabled` this is not a preference: the reply is + // delivered by the Qt event loop of this very thread, so a `registerHandler` + // that waited here would never return (morph#568's WASM page abort). + REQUIRE_FALSE(morph::bridge::Bridge::isBound(binding)); + + REQUIRE(morph::testing::waitUntil([&] { return morph::bridge::Bridge::isBound(binding); })); + REQUIRE(binding->currentId.load() == 99U); + REQUIRE(backend->settledOffCallerThread.load()); +} From 9905aded6aed4ee247dc6266a0a5aaf042cc71e7 Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Sun, 20 Sep 2026 10:06:57 +0200 Subject: [PATCH 09/11] qt: carry contextKey on a blocking private registration (fixes #594) `QtWebSocketBackend` did not override `registerModelWithContext`, so it inherited `IBackend`'s default, which drops the key -- while its own `bindModel` non-blocking path carried it. The two disagreed, and which one ran was decided by `Config::asyncRegistrationEnabled`, whose documentation is entirely about blocking versus not blocking the Qt event loop. This is not an audit entry missing a field. `RemoteServer::attachLogIfConfigured` returns *before* consulting its `LogProvider` when the envelope's `contextKey` is empty, so a privately registered instance over this transport produced **no audit record at all**, silently, while `SimulatedRemoteBackend` and `morph::net::SocketBackend` (morph#587) produced one. Three call sites reached the dropping default: the blocking `bindModel`'s empty-`primary`/zero-`current` branch (every private registration with the flag at its `false` default), `Bridge::switchBackend`'s re-registration after a reconnect (whatever the flag was set to), and `registerModelShared`/`attachModel`'s empty-`primary` degradation. Fix is morph#587's two lines: override `registerModelWithContext` to build `makeRegister(typeId, contextKey)`, and have `registerModel` forward to it with an empty key, so one place builds this envelope rather than two. morph#594's verification status was "inferred from reading the code, not reproduced". It is now **reproduced**. The new case in `tests/qt/test_qt_websocket.cpp` stands up a `QtWebSocketServer` over a `RemoteServer` with a `LogProvider` installed and, before the fix, failed on the two sections that send a key: tests/qt/test_qt_websocket.cpp:2504: FAILED: CHECK( requestedFor == std::vector{"WsEchoModel:acct-594"} ) with expansion: { } == { "WsEchoModel:acct-594" } tests/qt/test_qt_websocket.cpp:2523: FAILED: CHECK( requestedFor == std::vector{"WsEchoModel:acct-bind"} ) with expansion: { } == { "WsEchoModel:acct-bind" } Per AGENTS.md's "would this still pass if the feature did nothing": the assertion is on the provider, not on the registration. Registration succeeded before the fix too -- all 15 assertions in the case passed except those two. The third section asserts the other direction: plain `registerModel` still sends no key, so the provider is *not* consulted. `registerModel`'s behaviour is unchanged. Full suite after: 100% tests passed out of 1836 (GCC 16.2.1, Debug, MORPH_BUILD_QT=ON MORPH_BUILD_NET=ON). Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01AbwhcguQFkhvVi2AH19sWk --- docs/spec/core/backend.md | 32 +++++++++-- include/morph/qt/qt_websocket_backend.hpp | 35 ++++++++++++ src/qt/qt_websocket_backend.cpp | 10 +++- tests/qt/test_qt_websocket.cpp | 69 +++++++++++++++++++++++ 4 files changed, 139 insertions(+), 7 deletions(-) diff --git a/docs/spec/core/backend.md b/docs/spec/core/backend.md index f5a7d7a6a..33f1115c8 100644 --- a/docs/spec/core/backend.md +++ b/docs/spec/core/backend.md @@ -77,7 +77,7 @@ holds a `unique_ptr` and delegates all model operations to it. | Method | Purpose | |---|---| | `registerModel(typeId, factory)` | Registers a new model instance, returns its opaque `ModelId`. | -| `registerModelWithContext(typeId, factory, contextKey)` | Same as `registerModel`, additionally passes a stable identity (e.g. account id). Default implementation drops `contextKey` and forwards to `registerModel` — correct for `LocalBackend` where the factory closure already captures identity. Every backend whose instances live behind a wire protocol overrides it to carry `contextKey` across: `SimulatedRemoteBackend` and `SocketBackend` both do. Not cosmetic — `RemoteServer::attachLogIfConfigured` skips the `LogProvider` lookup entirely on an empty `contextKey`, so a wire backend that drops the key leaves the instance with **no** action log rather than a log missing a field (morph#587). | +| `registerModelWithContext(typeId, factory, contextKey)` | Same as `registerModel`, additionally passes a stable identity (e.g. account id). Default implementation drops `contextKey` and forwards to `registerModel` — correct for `LocalBackend` where the factory closure already captures identity. Every backend whose instances live behind a wire protocol overrides it to carry `contextKey` across: `SimulatedRemoteBackend`, `SocketBackend` (morph#587) and `QtWebSocketBackend` (morph#594) all do. Not cosmetic — `RemoteServer::attachLogIfConfigured` skips the `LogProvider` lookup entirely on an empty `contextKey`, so a wire backend that drops the key leaves the instance with **no** action log rather than a log missing a field (morph#587). | | `registerModelAsync(typeId, factory, contextKey, onRegistered, onError)` | Optional non-blocking counterpart to `registerModelWithContext`. Returns `false` by default, and since morph#568 no backend overrides it, so it always does; `Bridge::registerHandler()` then falls back to `bindModel`. Removed by morph#571. See [Asynchronous registration](#asynchronous-registration--registermodelasync). | | `bindModel(request, cbExec)` | Acquires a model instance and returns a `Completion` delivered on `cbExec`. One verb covering `registerModelWithContext`, `registerModelShared` and `attachModel`, selected by the request's shape. The preferred surface — see [The structural registration surface](#the-structural-registration-surface--bindmodel-and-promotemodel). | | `promoteModel(request, cbExec)` | Files an already-live instance under a key and returns a `Completion` delivered on `cbExec`. The structural counterpart of `assignPrimary`. | @@ -1486,9 +1486,29 @@ by `_pendingMtx`, because `cancelPending` can be called from `Bridge` / `std::runtime_error("register failed: ")` (so a lost connection surfaces as `"register failed: disconnected"`) rather than propagating the raw error or hanging. The `factory` argument is ignored (model construction is delegated to - the server, as with all remote backends). `registerModelWithContext` is - **not** overridden — the default drops the `contextKey`, so this transport - does not carry a context key to the server's `LogProvider`. + the server, as with all remote backends). `registerModel` is a forward to + `registerModelWithContext` with an empty key, so there is one place that + builds this envelope rather than two. + +- `registerModelWithContext` — the same `register` round trip, carrying + `contextKey` on the envelope (`wire::makeRegister(typeId, contextKey)`). + Overridden since morph#594; before that it was `IBackend`'s default, which + drops the key. That is not a missing field: + `RemoteServer::attachLogIfConfigured` returns **before** consulting its + `LogProvider` when the envelope's `contextKey` is empty, so a privately + registered instance over this transport produced no action-log record at all + while `SimulatedRemoteBackend` and `SocketBackend` produced one. Three call + sites reached the dropping default: the blocking `bindModel`'s + empty-`primary`/zero-`current` branch (so, every private registration made + with `Config::asyncRegistrationEnabled` unset — its default), + `Bridge::switchBackend`'s re-registration after a reconnect (whatever that + flag was set to), and `registerModelShared`/`attachModel`'s own + empty-`primary` degradation. `bindModel`'s non-blocking path already carried + the key, which is what made the flag decide whether an instance was audited. + `tests/qt/test_qt_websocket.cpp`, "a private registration carries contextKey + to the server's log provider", pins all of it against a real + `QtWebSocketServer` and asserts on the provider rather than on the + registration succeeding — registration succeeded before the fix too. `sendSync` itself is hardened against a disconnect mid-call. Before parking the nested loop it checks `_connected` and throws `"disconnected"` up front if the @@ -2294,10 +2314,12 @@ inside the class calls `close()` — no thread it joins can be waiting on it. | `QtWebSocketBackend(serverUrl, tls, cfg = Config{})` | Overload that skips the unused `dispatcher`/`registry` pair (issue #55): a caller who only needs `tls`/`cfg` reaches them directly, without naming `morph::model::detail::defaultDispatcher()`/`defaultRegistry()` explicitly. Delegates to the main constructor with both defaulted. Not declared on a `QT_NO_SSL` build (no `tls` parameter to distinguish it from the `(serverUrl, cfg)` overload below). | | `QtWebSocketBackend(serverUrl, cfg)` | Overload that skips `dispatcher`/`registry` and `tls` together — the common case for a caller that only wants to set a `Config` field (e.g. `asyncRegistrationEnabled`) over a plaintext `ws://` connection. Delegates to the main constructor with `dispatcher`/`registry` defaulted and (on an SSL-enabled build) `tls = std::nullopt`. | | `bindModel(request, cbExec)` | Defers to `IBackend::bindModel` (blocking) unless `cfg.asyncRegistrationEnabled` is `true`. Otherwise builds the envelope `request`'s shape names — `register`, shared `register`, or `attach` — assigns a fresh `callId` (the same counter `execute` uses), records the promise in `_pendingRegistrations[callId]` and sends. The `Completion` settles later from `onTextMessage` (or from `cancelPending` on a disconnect). A private bind on an unconnected socket is queued in `_queuedRegistrations` instead; a keyed one rejects with `"disconnected"`. | +| `bindWaitPolicy()` | `kCallerMustNotBlock` exactly when `cfg.asyncRegistrationEnabled` is set — that is when completions are settled by `onTextMessage`, a Qt slot delivered by the calling thread's own event loop, so a caller that blocked waiting for one would never see it arrive. `kCallerMayBlock` otherwise, where `bindModel` settles inside the call. | | `promoteModel(request, cbExec)` | Always non-blocking, with no `Config` gate — `assignPrimary`'s caller is inside a `Completion` chain, so there is no synchronous guarantee to preserve. Sends `assign` through the same path. An empty `primary` or zero `mid` resolves with `request.mid` without sending. | | `waitForConnected(timeoutMs = 5000)` | Pumps the Qt loop until connected or timeout; returns `_connected`. | | `negotiateProtocolVersion()` | Opt-in: sends `hello` synchronously (same nested-`QEventLoop` path as `registerModel`), classifies the reply via `wire::interpretHelloReply`. Throws on an explicit version rejection or a `sendSync` failure. | -| `registerModel(typeId, factory)` | Synchronous via nested `QEventLoop`; `factory` ignored. Throws on `err` reply. | +| `registerModel(typeId, factory)` | Forwards to `registerModelWithContext` with an empty key. | +| `registerModelWithContext(typeId, factory, contextKey)` | Synchronous via nested `QEventLoop`; `factory` ignored. Sends `register` carrying `contextKey`, so a privately registered instance is journalled (morph#594). Throws on `err` reply, and wraps a `sendSync` failure as `"register failed: "`. | | `deregisterModel(mid)` | **Fire-and-forget** — sends only if connected, does not wait for the ack. Carries a non-zero `callId` from the same counter `execute` uses, recorded in `_pendingDeregisters` so `onTextMessage` recognises the unwanted reply and drops it rather than handing it to a parked `sendSync` (issue #65). | | `execute(mid, call, cbExec)` | Assigns a `callId`, sends `execute`, returns a `Completion`. Immediate `DisconnectedError` if not connected. | | `notifyBackendChanged()` | No-op. | diff --git a/include/morph/qt/qt_websocket_backend.hpp b/include/morph/qt/qt_websocket_backend.hpp index 16dc9dff7..8454745b1 100644 --- a/include/morph/qt/qt_websocket_backend.hpp +++ b/include/morph/qt/qt_websocket_backend.hpp @@ -215,6 +215,41 @@ class QtWebSocketBackend : public ::morph::backend::detail::IBackend { const std::string& typeId, std::function()> factory) override; + /// @brief Sends a `register` carrying @p contextKey and blocks until the reply arrives. + /// + /// `IBackend::registerModelWithContext`'s default drops @p contextKey, which + /// is right for `LocalBackend` — the caller's own factory closure already + /// captures the identity — but wrong for a backend whose instances live on + /// the far side of a wire protocol: the server constructs the holder itself, + /// so `contextKey` is the *only* channel by which the instance's identity + /// reaches it. `RemoteServer::attachLogIfConfigured` returns without + /// consulting its `LogProvider` at all when the envelope's `contextKey` is + /// empty, so dropping it here did not merely lose an entity key — it left + /// the instance **unjournalled** (morph#594). `SimulatedRemoteBackend` and + /// `morph::net::SocketBackend` (morph#587) override this for the same + /// reason; backends documented as interchangeable must not disagree about + /// whether a private registration is audited. + /// + /// This is also the verb the *blocking* `bindModel` path reaches for an + /// empty-`primary`, zero-`current` request, and the one + /// `Bridge::switchBackend` calls directly when it re-registers a handler + /// after a reconnect — so before morph#594 the key was dropped whatever + /// `Config::asyncRegistrationEnabled` was set to on a backend swap, and + /// dropped on every private registration when it was unset. `bindModel`'s + /// own non-blocking path already carried it. + /// + /// `registerModel` forwards here with an empty key, so there is one place + /// that builds this envelope rather than two that can drift apart. + /// + /// @param typeId String type-id of the model to register. + /// @param factory Ignored — model construction is delegated to the server. + /// @param contextKey Stable identity of the new instance; empty if none. + /// @return `ModelId` assigned by the server. + /// @throws std::runtime_error if the server replies with an error or the socket is not connected. + ::morph::exec::detail::ModelId registerModelWithContext( + const std::string& typeId, std::function()> factory, + std::string_view contextKey) override; + /// @brief Acquires a model instance over the wire, natively non-blocking /// when `Config::asyncRegistrationEnabled` is set. /// diff --git a/src/qt/qt_websocket_backend.cpp b/src/qt/qt_websocket_backend.cpp index 8bda27911..f213c587f 100644 --- a/src/qt/qt_websocket_backend.cpp +++ b/src/qt/qt_websocket_backend.cpp @@ -139,8 +139,14 @@ std::string QtWebSocketBackend::sendSync(const std::string& msg) { } ::morph::exec::detail::ModelId QtWebSocketBackend::registerModel( - const std::string& typeId, std::function()> /*factory*/) { - auto env = ::morph::wire::makeRegister(typeId); + const std::string& typeId, std::function()> factory) { + return registerModelWithContext(typeId, std::move(factory), {}); +} + +::morph::exec::detail::ModelId QtWebSocketBackend::registerModelWithContext( + const std::string& typeId, std::function()> /*factory*/, + std::string_view contextKey) { + auto env = ::morph::wire::makeRegister(typeId, std::string{contextKey}); env.session = _session; std::string replyJson; try { diff --git a/tests/qt/test_qt_websocket.cpp b/tests/qt/test_qt_websocket.cpp index d092a8ec9..0812bb478 100644 --- a/tests/qt/test_qt_websocket.cpp +++ b/tests/qt/test_qt_websocket.cpp @@ -23,6 +23,7 @@ #include #include #include +#include #include #include #include @@ -2461,6 +2462,74 @@ TEST_CASE("Process separation: TLS handshake works across processes", "[qt][wss] REQUIRE(runClient(url, {QStringLiteral("--tls")}) == 0); } +// Coverage for morph#594: a *private* registration over this transport must +// carry `contextKey` to the server, exactly as `SimulatedRemoteBackend` and +// `morph::net::SocketBackend` (morph#587) do. +// +// The assertion is deliberately on the provider, not on the registration: +// registration succeeded before this was fixed too. `RemoteServer:: +// attachLogIfConfigured` returns *before* consulting the `LogProvider` when the +// envelope's `contextKey` is empty, so a dropped key does not produce an audit +// record missing a field — it produces no audit record at all, silently. +TEST_CASE("morph::qt::QtWebSocketBackend: a private registration carries contextKey to the server's log provider", + "[qt][ws][action_log]") { + ensureApp(); + morph::exec::ThreadPoolExecutor serverPool{2}; + auto server = std::make_shared(serverPool); + + // The provider runs on the server's own strand and the assertions on this + // thread; the mutex is what makes that handoff an ordinary handoff rather + // than a data race TSan will flag. + std::mutex providerMtx; + std::vector requestedFor; + auto log = std::make_shared(); + server->setLogProvider([&](std::string_view modelType, std::string_view contextKey) { + std::scoped_lock const lock{providerMtx}; + requestedFor.emplace_back(std::string{modelType} + ":" + std::string{contextKey}); + return log; + }); + + morph::qt::QtWebSocketServer wsServer{*server, 0}; + REQUIRE(wsServer.listen()); + + QUrl const url{QString("ws://127.0.0.1:%1").arg(wsServer.port())}; + morph::qt::QtWebSocketBackend backend{url}; + REQUIRE(backend.waitForConnected()); + + SECTION("through the blocking registerModelWithContext path") { + auto const mid = backend.registerModelWithContext("WsEchoModel", nullptr, "acct-594"); + REQUIRE(mid.v != 0U); + std::scoped_lock const lock{providerMtx}; + CHECK(requestedFor == std::vector{"WsEchoModel:acct-594"}); + } + + SECTION("and through the default bindModel, which is what Bridge::registerHandler reaches here") { + // `asyncRegistrationEnabled` is unset, so this is `IBackend::bindModel`'s + // default dispatching the empty-`primary`/zero-`current` shape to + // `registerModelWithContext` — the path a `Bridge` over this backend + // takes, and the one morph#594 reported as dropping the key. + morph::exec::detail::ModelId bound{}; + backend + .bindModel(morph::backend::detail::BindRequest{.typeId = "WsEchoModel", + .factory = nullptr, + .contextKey = "acct-bind", + .primary = {}, + .current = {}}, + morph::exec::detail::inlineExecutor()) + .thenDetached([&](morph::exec::detail::ModelId mid) { bound = mid; }); + REQUIRE(bound.v != 0U); + std::scoped_lock const lock{providerMtx}; + CHECK(requestedFor == std::vector{"WsEchoModel:acct-bind"}); + } + + SECTION("while plain registerModel still sends no key, so the provider is not consulted") { + auto const mid = backend.registerModel("WsEchoModel", nullptr); + REQUIRE(mid.v != 0U); + std::scoped_lock const lock{providerMtx}; + CHECK(requestedFor.empty()); + } +} + // ── Custom main: own the QCoreApplication explicitly ───────────────────────── // // Without this, `QCoreApplication` was a static local in `ensureApp()` and so From bcbf0f51ce2a576716f832e35274e40ffbe567dc Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Sun, 20 Sep 2026 11:45:01 +0200 Subject: [PATCH 10/11] ci: repoint three coverage-allowlist hints the resolver rejected `check_branch_coverage.py`'s `resolve_allowlist_source_line()` failed the `Linux / clang-coverage` leg on three entries in `scripts/branch_partial_allowlist.json`: error: include/morph/util/rational.hpp:1542 has moved to line 1498. error: include/morph/core/backend.hpp:1230 has moved to line 1324. error: include/morph/core/bridge.hpp:1498 has moved to line 1542. Only one of the three is this branch's doing. Resolving each entry's `source` text against both revisions: rational.hpp hint 1542 -> master: 1498 this branch: 1498 bridge.hpp hint 1498 -> master: 1453 this branch: 1542 backend.hpp hint 1230 -> master: 1230 this branch: 1324 `rational.hpp` and `bridge.hpp` were **already stale on master** and this branch does not touch `rational.hpp` at all; `backend.hpp` drifted by the 94 lines this branch adds. All three are repointed here because the leg cannot go green otherwise, and every `source` text still matches uniquely, so no disposition changed -- only the coordinates. The pre-existing half is filed separately rather than folded in: the file that `resolve_allowlist_source_line()` exists to harden is itself carrying drift on master, which is the same rot class as morph#608. Verified: all 22 entries in the allowlist now resolve to the line they name on this branch. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01AbwhcguQFkhvVi2AH19sWk --- 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 d05b1ce80..db4d16423 100644 --- a/scripts/branch_partial_allowlist.json +++ b/scripts/branch_partial_allowlist.json @@ -62,7 +62,7 @@ }, { "file": "include/morph/util/rational.hpp", - "line": 1542, + "line": 1498, "source": "if (ctx.begin() == ctx.end() || *ctx.begin() == '}') {", "reason": "The ctx.begin() == ctx.end() true arm is unreachable. This is the textbook cppreference-style custom-formatter parse() idiom. Empirically verified on this toolchain (libc++, via a standalone probe compiled with clang++ -std=c++23 -stdlib=libc++): for both std::format(\"{}\", x) and std::format(\"{:}\", x), ctx.end() always points past the terminating '}', so ctx.begin() == ctx.end() is false and the terminator is always reachable via *ctx.begin() == '}' -- matching the observed 0/14 split exactly. std::format's top-level parser (and std::vformat's, which performs the same replacement-field validation before dispatching to a type's parse()) rejects an unterminated '{...' before ever calling into formatter::parse, so this function is never invoked with an already-exhausted range. The first disjunct is defensive boilerplate that this standard library's implementation (and the standard's own guarantee about validated replacement fields) makes structurally unreachable." }, @@ -80,7 +80,7 @@ }, { "file": "include/morph/core/backend.hpp", - "line": 1230, + "line": 1324, "source": "if (const auto* inst = _instances.find(modelId)) {", "reason": "Unreachable by construction given the `_changeAware`/`_instances` invariant (core audit finding BK2). `_changeAware` is an index over the instance directory: an id enters it in `createHolder` (this file, when the holder answers `isBackendChangeAware()`) in the same `_regMtx`-held critical section that files the instance, and leaves it in `deregisterModel` only when `InstanceDirectory::release` reports the instance actually destroyed. `notifyBackendChanged()` (this function) holds the same `_regMtx` while walking `_changeAware` and looking each id up at this line, so every id it walks is still live -- the null arm cannot occur without a code change that breaks that subset invariant. Formerly keyed on `_models`, the map morph#523 replaced with the directory; the invariant and its reason are unchanged." }, @@ -98,7 +98,7 @@ }, { "file": "include/morph/core/bridge.hpp", - "line": 1498, + "line": 1542, "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." }, From 691174ec70261ec97b82723c6485ba4d5a95743d Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Sun, 20 Sep 2026 11:58:15 +0200 Subject: [PATCH 11/11] ci: repoint two mutation-survivor citations this branch moved `scripts/check_mutation_survivors.py`, which landed on master in #614 after this branch was cut, resolves each triaged equivalent mutant's `source` text and fails when the `line` hint has drifted. Two entries name code this branch moves: include/morph/core/bridge.hpp:121 has moved to line 122 include/morph/core/backend.hpp:1228 has moved to line 1322 Both `source` texts still match uniquely, so neither disposition changed -- only the coordinates. `backend.hpp`'s 94-line delta is this branch's; the `bridge.hpp` one-line shift likewise. This is the gate working as designed, and was predicted before either branch was written: #614's PR body records that these two hints would go stale when this branch landed, and named the two files. Verified: `python3 scripts/check_mutation_survivors.py` reports `ok: 7 structured citation(s) ... resolve to the line they name`. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01AbwhcguQFkhvVi2AH19sWk --- scripts/mutation_survivors.json | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/scripts/mutation_survivors.json b/scripts/mutation_survivors.json index 0122e40ea..9d1a6a7cf 100644 --- a/scripts/mutation_survivors.json +++ b/scripts/mutation_survivors.json @@ -56,7 +56,7 @@ }, { "file": "include/morph/core/bridge.hpp", - "line": 121, + "line": 122, "mutants": 3, "mutator": "cxx_add_to_sub", "source": "return modelHash ^ (key.sharing.hash_code() + 0x9e3779b9U + (modelHash << 6) + (modelHash >> 2));", @@ -64,7 +64,7 @@ }, { "file": "include/morph/core/backend.hpp", - "line": 1228, + "line": 1322, "mutants": 1, "mutator": "cxx_replace_scalar_call", "source": "aware.reserve(_changeAware.size());",