From 5d6a5158d9695ea65af8ff5d2b284e7ceb60c0e6 Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Mon, 21 Sep 2026 01:24:56 +0200 Subject: [PATCH 1/5] examples: describe the structural registration surface, not the removed async twins (fixes #570) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The ticket is scoped to "move the example GUIs and the WASM spike onto the structural registration surface". They were already on it: nothing under `examples/` ever called `registerModelAsync`, `registerModelSharedAsync`, `attachModelAsync` or `assignPrimaryAsync` — application code reaches a backend only through `Bridge`, and morph#568 moved `Bridge`'s four dispatch sites. Verified rather than assumed: every one of the 25 hits under `examples/` on `c55ea5b7` was a `//` comment, a `///` doc comment, a CMake comment or a line of Markdown. So what this commit moves is the prose, which named verbs that morph#571 deletes and that nothing had implemented since morph#568. - The GUI clients and `AppContext` now name `QtWebSocketBackend::bindModel()` as what queues a private bind issued before the socket connects. The behaviour they describe is unchanged and still true: `bindModel` queues a request with an empty `primary` and sends it on the next `connected`, and rejects a keyed one with `"disconnected"` (`src/qt/qt_websocket_backend.cpp`, the `!_connected` arms of `bindModel`). - `AppContext`'s readiness contract gains the half it was missing: with `asyncRegistrationEnabled` the backend answers `BindWait::kCallerMustNotBlock`, so `registerHandler` hands back an *unbound* handler and a caller must gate on `whenBound()`/`isBound()`. That was true before this commit too; the doc comment only described the queue. - `examples/polls/README.md`'s struck `assignPrimaryAsync` claim keeps its correction but states it against `IBackend::promoteModel`, which no backend can decline — a stronger version of the same rebuttal. - `LADDER.md`, `TESTING.md` and `polls/README.md` keep their "shipped" history: each says what shipped then and that morph#567–morph#571 replaced it, rather than deleting the record. `examples/common/testkit/test_wasm_registration_path_native.cpp` still pins the single-threaded registration path through `Bridge` — only its comment changed, and its citation of `tests/qt/test_qt_websocket.cpp` now names that file's current test ("bindModel called before the socket connects queues and retries once connected fires"). Verification: `cmake --build build` green with `-DMORPH_BUILD_LADDER=ON -DMORPH_LADDER_RUNGS=all -DMORPH_BUILD_QT=ON -DMORPH_BUILD_NET=ON`; `examples/common/ladder_common_tests` 587 assertions in 153 cases, all passing. **Not verified: anything WASM.** No Emscripten toolchain was available, so neither `morph_ladder_wasm_spike` nor any `*_gui_wasm` target was configured, let alone built or run — those targets exist only in an Emscripten configure. The acceptance criterion "the WASM ladder clients build, and the WASM demo deploys" is therefore unmet by local measurement and rests entirely on the `wasm-ladder` and `wasm-demo` CI legs. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01VptDWG2fKr2vBnLSJcgzgW --- examples/LADDER.md | 16 +++--- examples/TESTING.md | 20 ++++---- examples/bookmarks/gui/main.cpp | 7 +-- examples/bookmarks/gui_wasm/main_wasm.cpp | 6 +-- examples/common/gui/app_context.hpp | 20 +++++--- examples/common/testkit/test_presenter.cpp | 7 +-- .../test_wasm_registration_path_native.cpp | 11 ++-- examples/common/wasm_spike/CMakeLists.txt | 2 +- examples/common/wasm_spike/main_wasm.cpp | 7 +-- examples/pastebin/gui/main.cpp | 7 +-- examples/pastebin/gui_wasm/main_wasm.cpp | 6 +-- examples/polls/README.md | 51 ++++++++++--------- examples/polls/gui_wasm/main_wasm.cpp | 10 ++-- 13 files changed, 96 insertions(+), 74 deletions(-) diff --git a/examples/LADDER.md b/examples/LADDER.md index b5d27d209..bfce2779e 100644 --- a/examples/LADDER.md +++ b/examples/LADDER.md @@ -252,12 +252,16 @@ check rather than take this section's word for it: 1. **Async shared/keyed attach for WASM** (before rung 3's WASM story) — `registerModelShared`/`attachModel` were synchronous and nested an event - loop, which aborts the page on the WASM main thread, and - `registerModelAsync` covered only the plain path. - **Shipped:** `IBackend::registerModelSharedAsync` and - `IBackend::attachModelAsync` (`include/morph/core/backend.hpp`), consumed by - `Bridge::ensureBoundAsync`/`attachHandlerAsync` and implemented by - `QtWebSocketBackend`. + loop, which aborts the page on the WASM main thread, and the non-blocking + counterpart that existed at the time covered only the plain path. + **Shipped:** `IBackend::bindModel` (`include/morph/core/backend.hpp`) — + one non-blocking acquire verb whose *request shape* selects private + registration, register-or-attach or re-point — consumed by + `Bridge::ensureBoundAsync`/`attachHandlerAsync` and implemented natively by + `QtWebSocketBackend`. It first shipped as a pair of optional non-blocking + twins beside the synchronous verbs; morph#567–morph#571 removed the twins + in favour of the single surface above, under which the synchronous verbs + survive only as what `IBackend`'s *default* `bindModel` dispatches to. 2. **Client-side execute deadline** (before rung 3's polling helper) — no timeout existed on a `Completion`, so a black-holed server hung the client forever. **Shipped:** `Bridge::setExecuteDeadline` diff --git a/examples/TESTING.md b/examples/TESTING.md index b912d8ff4..2c5c4da7b 100644 --- a/examples/TESTING.md +++ b/examples/TESTING.md @@ -63,10 +63,10 @@ QProcess client harness, the Qt-owning Catch2 `main()`), the pump helpers in themselves.** `Remote` is asynchronously connected and exposes `ready()`/`onReady(cb)`: presenters (which build `BridgeHandler`s, and a `BridgeHandler` constructor registers) **must** be constructed from inside - `onReady`. `QtWebSocketBackend::registerModelAsync()` queues a - registration issued before the socket connects and retries it once the - connection comes up (`docs/spec/core/backend.md`, "Asynchronous - registration"), so this is no longer the correctness hazard it once was + `onReady`. `QtWebSocketBackend::bindModel()` queues a private bind + issued before the socket connects and sends it once the + connection comes up (`docs/spec/core/backend.md`, "The structural + registration surface"), so this is no longer the correctness hazard it was — but building presenters/`BridgeHandler`s from inside `onReady` stays the simpler ordering to reason about, and is what every rung does. `Local` is ready on construction and runs `onReady` inline, so mode-blind @@ -550,11 +550,13 @@ Open framework facts every rung must respect (verified): (`registerModelShared`/`attachModel`) nests an event loop that aborts the page on WASM** — that part still holds, and a WASM client must not call it. What has changed is the remedy: async attach is **no longer a missing - framework prerequisite**. `IBackend::registerModelSharedAsync` and - `IBackend::attachModelAsync` (`include/morph/core/backend.hpp`) ship the - non-blocking counterparts, `Bridge::ensureBoundAsync`/`attachHandlerAsync` - dispatch to them, and `QtWebSocketBackend` implements both. A rung's WASM - story uses those rather than waiting on the framework. (The rung-1 coupling + framework prerequisite**. `IBackend::bindModel` + (`include/morph/core/backend.hpp`) is the one non-blocking acquire verb — + a request with a non-empty `primary` is the register-or-attach case, one + that also carries a non-zero `current` is the re-point — + `Bridge::ensureBoundAsync`/`attachHandlerAsync` dispatch to it, and + `QtWebSocketBackend` implements it natively. A rung's WASM + story uses that rather than waiting on the framework. (The rung-1 coupling the pastebin README calls out — burn atomicity via a shared keyed instance — is likewise no longer gated on this.) diff --git a/examples/bookmarks/gui/main.cpp b/examples/bookmarks/gui/main.cpp index 81df2a2b8..e305b510e 100644 --- a/examples/bookmarks/gui/main.cpp +++ b/examples/bookmarks/gui/main.cpp @@ -95,9 +95,10 @@ int main(int argc, char** argv) { // Mirrors AppContext's own doc-comment construction pattern: pick the // mode, then build every handler from inside onReady(). `Remote` mode // builds its backend with `asyncRegistrationEnabled`, and - // `QtWebSocketBackend::registerModelAsync()` queues a registration issued - // before the socket finishes connecting and retries it once the connection - // comes up (`docs/spec/core/backend.md`, "Asynchronous registration"), so + // `QtWebSocketBackend::bindModel()` queues a private bind issued before + // the socket finishes connecting and sends it once the connection comes + // up (`docs/spec/core/backend.md`, "The structural registration + // surface"), so // this ordering is no longer load-bearing for correctness — it is simply // the one shape that reads the same in both modes (`Local` is ready on // construction and runs onReady() inline). See diff --git a/examples/bookmarks/gui_wasm/main_wasm.cpp b/examples/bookmarks/gui_wasm/main_wasm.cpp index 958b1611e..dbf3171c2 100644 --- a/examples/bookmarks/gui_wasm/main_wasm.cpp +++ b/examples/bookmarks/gui_wasm/main_wasm.cpp @@ -84,9 +84,9 @@ int main(int argc, char** argv) { // Every handler is built from inside onReady(), never before it. A // registration issued before the socket is up does not fail permanently: // `Remote` mode sets `asyncRegistrationEnabled`, and - // `QtWebSocketBackend::registerModelAsync()` queues such a registration and - // retries it once the connection comes up (`docs/spec/core/backend.md`, - // "Asynchronous registration"). Deferring to onReady() is kept because it + // `QtWebSocketBackend::bindModel()` queues such a private bind and sends + // it once the connection comes up (`docs/spec/core/backend.md`, "The + // structural registration surface"). Deferring to onReady() is kept because it // is simpler to reason about than the pre-connect queue -- see // `examples/common/gui/app_context.hpp`'s "Readiness contract". What *is* // still a hard WASM constraint is how readiness is detected: AppContext diff --git a/examples/common/gui/app_context.hpp b/examples/common/gui/app_context.hpp index 18cf5ef01..aae699970 100644 --- a/examples/common/gui/app_context.hpp +++ b/examples/common/gui/app_context.hpp @@ -58,12 +58,20 @@ struct Remote { /// `Remote` mode builds its `QtWebSocketBackend` with /// `Config{.asyncRegistrationEnabled = true}` (the plain synchronous /// `registerModel` nests a `QEventLoop` and aborts a WASM page — -/// examples/TESTING.md, "WASM reality"). `QtWebSocketBackend:: -/// registerModelAsync()` queues a registration issued before the socket -/// has finished connecting and retries it once the connection comes up -/// (`docs/spec/core/backend.md`, "Asynchronous registration"), so -/// building a `BridgeHandler` immediately after this constructor returns is -/// no longer the correctness hazard it once was. +/// examples/TESTING.md, "WASM reality"). +/// `QtWebSocketBackend::bindModel()` queues a *private* bind issued before +/// the socket has finished connecting and sends it once the connection +/// comes up (`docs/spec/core/backend.md`, "The structural registration +/// surface"), so building a `BridgeHandler` immediately after this +/// constructor returns is no longer the correctness hazard it once was. +/// +/// What it does not do is make that handler *bound*. With +/// `asyncRegistrationEnabled` the backend answers +/// `BindWait::kCallerMustNotBlock`, so `Bridge::registerHandler` returns an +/// unbound handler and a call issued through it before the reply lands +/// fails "handler not bound" — gate on +/// `BridgeHandler::whenBound()`/`isBound()` (`docs/spec/core/bridge.md`, +/// "Registration readiness"). /// /// This class still detects readiness with `setConnectHandler` — not /// `waitForConnected()`, which nests an event loop and hangs a WASM page — diff --git a/examples/common/testkit/test_presenter.cpp b/examples/common/testkit/test_presenter.cpp index fc6b53242..58092c116 100644 --- a/examples/common/testkit/test_presenter.cpp +++ b/examples/common/testkit/test_presenter.cpp @@ -311,9 +311,10 @@ TEST_CASE("AppContext{Remote} defers readiness to the first connect", // Not ready the line after construction: QWebSocket::open() is // asynchronous and no event-loop turn has run yet. A BridgeHandler - // constructed here would queue its registration and retry once the - // socket connects (registerModelAsync's queueing, docs/spec/core/ - // backend.md), rather than failing -- but ctx.ready() still reflects + // constructed here would queue its bind and send it once the socket + // connects (`QtWebSocketBackend::bindModel`'s pre-connect queue, + // docs/spec/core/backend.md), rather than failing -- but ctx.ready() + // still reflects // socket-connect timing, not registration settlement, so it is false // regardless. REQUIRE_FALSE(ctx.ready()); diff --git a/examples/common/testkit/test_wasm_registration_path_native.cpp b/examples/common/testkit/test_wasm_registration_path_native.cpp index cbeb5cabe..683e8db9a 100644 --- a/examples/common/testkit/test_wasm_registration_path_native.cpp +++ b/examples/common/testkit/test_wasm_registration_path_native.cpp @@ -36,11 +36,12 @@ BRIDGE_REGISTER_ACTION(WasmSpikeProbeModel, WasmSpikeProbeAction, "WasmSpikeProb // constructed a `BridgeHandler` unconditionally, immediately after // constructing the Bridge -- before any Qt event-loop turn had a chance to // run, so the QWebSocket was guaranteed to still be unconnected at that -// point. `QtWebSocketBackend::registerModelAsync()` now queues a -// pre-connect registration and retries it once the socket connects (see -// tests/qt/test_qt_websocket.cpp's "registerModelAsync called before the -// socket connects queues and retries once connected fires", -// docs/spec/core/backend.md's "Asynchronous registration") -- so this call +// point. `QtWebSocketBackend::bindModel()` queues a pre-connect private +// bind and sends it once the socket connects (see +// tests/qt/test_qt_websocket.cpp's "bindModel called before the socket +// connects queues and retries once connected fires", +// docs/spec/core/backend.md's "The structural registration surface") -- so +// this call // sequence now resolves natively, with no need for the deferred-construction // workaround the test below demonstrates (which remains a valid, // simpler-still sequence, just no longer the only correct one). diff --git a/examples/common/wasm_spike/CMakeLists.txt b/examples/common/wasm_spike/CMakeLists.txt index 1893440f3..9909d3b2c 100644 --- a/examples/common/wasm_spike/CMakeLists.txt +++ b/examples/common/wasm_spike/CMakeLists.txt @@ -18,7 +18,7 @@ qt_standard_project_setup(REQUIRES 6.5) qt_add_executable(morph_ladder_wasm_spike main_wasm.cpp) # morph::qt is header-only (INTERFACE); the compiled QtWebSocketBackend -# constructor/registerModelAsync/setConnectHandler bodies live in +# constructor/bindModel/setConnectHandler bodies live in # morph_qt_impl (see ../../../CMakeLists.txt's `add_library(morph_qt_impl # STATIC ...)`). main_wasm.cpp constructs a QtWebSocketBackend directly, so # without this the WASM link fails on undefined symbols -- every other real diff --git a/examples/common/wasm_spike/main_wasm.cpp b/examples/common/wasm_spike/main_wasm.cpp index 9e6df1143..be4614b22 100644 --- a/examples/common/wasm_spike/main_wasm.cpp +++ b/examples/common/wasm_spike/main_wasm.cpp @@ -14,9 +14,10 @@ // directory's README.md for how the nightly Playwright smoke wires that up). // // IMPORTANT ordering constraint discovered while building this spike: -// QtWebSocketBackend::registerModelAsync() now queues a registration issued -// before the socket has connected and retries it once the connection comes -// up (docs/spec/core/backend.md, "Asynchronous registration") -- but the +// QtWebSocketBackend::bindModel() queues a private bind issued before the +// socket has connected and sends it once the connection comes up +// (docs/spec/core/backend.md, "The structural registration surface") -- but +// the // *reconnect* handler Bridge installs only fires on a *subsequent* // reconnect, never on the first connect, so this spike still defers to // setConnectHandler rather than relying on the pre-connect queue. The diff --git a/examples/pastebin/gui/main.cpp b/examples/pastebin/gui/main.cpp index 3a10e47a9..79c82ed43 100644 --- a/examples/pastebin/gui/main.cpp +++ b/examples/pastebin/gui/main.cpp @@ -75,9 +75,10 @@ int main(int argc, char** argv) { // Mirrors AppContext's own doc-comment construction pattern: pick the // mode, then build every handler from inside onReady(). `Remote` mode // builds its backend with `asyncRegistrationEnabled`, and - // `QtWebSocketBackend::registerModelAsync()` queues a registration issued - // before the socket finishes connecting and retries it once the connection - // comes up (`docs/spec/core/backend.md`, "Asynchronous registration"), so + // `QtWebSocketBackend::bindModel()` queues a private bind issued before + // the socket finishes connecting and sends it once the connection comes + // up (`docs/spec/core/backend.md`, "The structural registration + // surface"), so // this ordering is no longer load-bearing for correctness — it is simply // the one shape that reads the same in both modes (`Local` is ready on // construction and runs onReady() inline). See diff --git a/examples/pastebin/gui_wasm/main_wasm.cpp b/examples/pastebin/gui_wasm/main_wasm.cpp index d30d66057..515135149 100644 --- a/examples/pastebin/gui_wasm/main_wasm.cpp +++ b/examples/pastebin/gui_wasm/main_wasm.cpp @@ -74,9 +74,9 @@ int main(int argc, char** argv) { // Every handler is built from inside onReady(), never before it. A // registration issued before the socket is up no longer fails permanently: // `Remote` mode sets `asyncRegistrationEnabled`, and - // `QtWebSocketBackend::registerModelAsync()` queues such a registration and - // retries it once the connection comes up (`docs/spec/core/backend.md`, - // "Asynchronous registration"). Deferring to onReady() is kept because it + // `QtWebSocketBackend::bindModel()` queues such a private bind and sends + // it once the connection comes up (`docs/spec/core/backend.md`, "The + // structural registration surface"). Deferring to onReady() is kept because it // is simpler to reason about than the pre-connect queue — see // `examples/common/gui/app_context.hpp`'s "Readiness contract". What *is* // still a hard WASM constraint is how readiness is detected: AppContext diff --git a/examples/polls/README.md b/examples/polls/README.md index 06d960a1a..0b5b20d2a 100644 --- a/examples/polls/README.md +++ b/examples/polls/README.md @@ -138,17 +138,19 @@ runs on. in `gui/qml/Main.qml` still points here. The struck claim was: `Bridge::assignHandlerPrimary`'s promote step has - no async path (`IBackend::assignPrimary` being a synchronous `sendSync` - on `QtWebSocketBackend`, "with no `assignPrimaryAsync` anywhere in the - tree"), so a WASM tab dispatching `CreatePoll` would abort the page at - the promote step. Both premises are false against the tree as it stands: - - - **`assignPrimaryAsync` exists**, at every layer the claim named: - `IBackend::assignPrimaryAsync` (`include/morph/core/backend.hpp`), - `QtWebSocketBackend::assignPrimaryAsync` + no non-blocking path (`IBackend::assignPrimary` being a synchronous + `sendSync` on `QtWebSocketBackend`, with nothing beside it), so a WASM tab + dispatching `CreatePoll` would abort the page at the promote step. Both + premises are false against the tree as it stands: + + - **A non-blocking promote exists**, at every layer the claim named: + `IBackend::promoteModel` (`include/morph/core/backend.hpp`), + `QtWebSocketBackend::promoteModel` (`src/qt/qt_websocket_backend.cpp`), and `assignHandlerPrimary` itself, - which prefers it and falls back to the synchronous call only for a - backend that offers none (`include/morph/core/bridge.hpp`). + which calls it unconditionally (`include/morph/core/bridge.hpp`). When + the claim was written that layer was an optional non-blocking twin a + backend could decline; morph#567–morph#571 replaced it with + `promoteModel`, which no backend can decline. - **The promote step never runs for this rung anyway.** `assignHandlerPrimary` is reached from exactly one branch of `BridgeHandler::execute`, guarded by `kShared && @@ -219,19 +221,20 @@ since shipped** — this rung built them, as the heading above says. They are kept here because they explain the rung's task order, with a pointer to where each now lives: -- **Async shared/keyed attach.** *Shipped:* - `IBackend::registerModelSharedAsync` and `IBackend::attachModelAsync` +- **Async shared/keyed attach.** *Shipped:* `IBackend::bindModel` (`include/morph/core/backend.hpp`), dispatched to by - `Bridge::ensureBoundAsync`/`attachHandlerAsync` and implemented by - `QtWebSocketBackend`. The synchronous `registerModelShared`/`attachModel` - still nest a `QEventLoop` and still abort the page on the WASM main thread, - so a WASM client must use the async pair — but it exists, and the very first + `Bridge::ensureBoundAsync`/`attachHandlerAsync` and implemented natively by + `QtWebSocketBackend`. The blocking `sendSync` path a wire backend used to + take for a keyed acquire nests a `QEventLoop` and aborts the page on the + WASM main thread, so a WASM client must not reach it — and with + `Config::asyncRegistrationEnabled` set it does not, so the very first `OpenPoll` a WASM tab makes is no longer blocked on the framework. Built as - this rung's first framework-level task, mirroring - `registerModelAsync`'s existing opt-in/fallback shape (backend returns - `true` and later invokes exactly one callback, or returns `false` and the - caller falls back to the synchronous path unaffected) so every backend - that has not opted in keeps its current behavior. + this rung's first framework-level task, and built in the shape that existed + then: a pair of optional non-blocking twins a backend returned `true` or + `false` from, with the caller falling back to the synchronous verb on + `false`. morph#567–morph#571 removed the twins in favour of the one verb + above, so there is no opt-in left to decline and no fallback path; the + synchronous verbs survive only as what the *default* `bindModel` runs. - **Client-side execute deadline.** *Shipped:* `Bridge::setExecuteDeadline` (`include/morph/core/bridge.hpp`), specified in `docs/spec/core/completion.md`. Without it a genuinely hung server blocked @@ -347,10 +350,10 @@ log table above. (`registerModelShared`/`attachModel`) nests an event loop and **aborts the page on the WASM main thread**, so a WASM tab's very first `OpenPoll` must not go through it. This is no longer a framework prerequisite: - `registerModelSharedAsync`/`attachModelAsync` ship the non-blocking pair + `IBackend::bindModel` carries the keyed acquire without blocking (see § Framework prerequisites above). Still worth running the "several WASM - tabs" demo literally — the async path's *behaviour in a browser* has never - been observed, only its compilation. + tabs" demo literally — the non-blocking path's *behaviour in a browser* has + never been observed, only its compilation. - **The polling helper must own a client-side timeout**: an unwrapped poll call against a hung server hangs its completion forever, which is what `Bridge::setExecuteDeadline` now exists to bound. A rate-limited server no diff --git a/examples/polls/gui_wasm/main_wasm.cpp b/examples/polls/gui_wasm/main_wasm.cpp index 224cf43b6..89b4d8c63 100644 --- a/examples/polls/gui_wasm/main_wasm.cpp +++ b/examples/polls/gui_wasm/main_wasm.cpp @@ -133,7 +133,7 @@ /// papers over a real gap: /// - Pastebin's/bookmarks' plain (`NoSharing`) handlers each call /// `Bridge::registerHandler(binding)` at construction, which — via -/// `registerHandlerImpl` — issues a real `registerModelAsync` round trip +/// `registerHandlerImpl` — issues a real `IBackend::bindModel` round trip /// to the backend. Until that reply lands, any call through the handler /// fails "handler not bound"; that window is exactly why those two /// rungs' bridges expose a `bound` signal their QML gates the first @@ -249,10 +249,10 @@ int main(int argc, char** argv) { // contract, `examples/common/gui/app_context.hpp`). The two halves are not // symmetric: a plain *registration* issued before the socket is up is // queued and sent once it connects, but a keyed *attach* is not — - // `QtWebSocketBackend::attachModelAsync()` fails one immediately with - // "disconnected" and never retries it - // (`docs/spec/core/backend.md`, "Asynchronous registration", - // "Shared/keyed registration"). Identical to bookmarks'/pastebin's own + // `QtWebSocketBackend::bindModel()` rejects a bind carrying a non-empty + // `primary` immediately with "disconnected" and never retries it + // (`docs/spec/core/backend.md`, "The structural registration surface"). + // Identical to bookmarks'/pastebin's own // Remote clients, and — per this file's header comment — load-bearing for // a second, distinct reason: `PollBridge`'s handler's *first* network // call is `OpenPoll`'s async attach itself, with no prior "registration" From eefc9e6f1fe449c4c0fc5a5c2012c03b713adca9 Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Mon, 21 Sep 2026 01:25:18 +0200 Subject: [PATCH 2/5] core: remove IBackend's four async twins and relocate the prose threading contract (fixes #571) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `registerModelAsync`, `registerModelSharedAsync`, `attachModelAsync` and `assignPrimaryAsync` are gone from `IBackend` and from `SynchronousBackendAdapter`, and `Bridge`'s four "offer the twin first, fall back to `bindModel`" branches are now one unconditional dispatch each. $ grep -rn 'registerModelAsync\|registerModelSharedAsync\|attachModelAsync\|assignPrimaryAsync' include src tests examples $ echo $? 1 **`IBackend`'s virtual count, and #522's prediction.** Measured on `origin/master` (`c55ea5b7`) and on this commit, excluding the destructor: | | virtuals | pure | defaulted | |---|---|---|---| | before | 21 | 5 | 16 | | after | 17 | 5 | 12 | #522 says "18 virtuals with 14 defaulted" and predicts the interface "roughly halves". **The prediction was wrong on both counts.** The count it quotes was already stale when it was written (there were 21, not 18, once `bindModel`/`promoteModel`/`bindWaitPolicy` landed), and removing four of twenty-one is a 19% reduction, not a halving. What the set actually delivered is the thing #522 is about — there is now exactly one acquire surface and one promote surface, and no call site carries a second path — but the arithmetic in the ticket should not be quoted as if it came true. **What the twins carried that the surface does not.** One thing, named rather than left to be found. A `bool` twin handed `Bridge` two raw `std::function`s, so a backend that violated "exactly one callback" by firing twice reached `detail::parkIfInFrame`'s own double-claim guard. A `Completion` cannot be settled twice — `CompletionState` drops the second settle before any `Bridge` code sees it — so `DoubleFiringBackend` in `tests/test_async_registration.cpp` now pins the observable contract ("exactly one `onDone`") while that guard is no longer reachable *from a backend*. The guard is kept because `parkIfInFrame` is also called from the dispatching frame. This is recorded in the double's own comment and in `docs/spec/core/backend.md`. **The prose threading contract is relocated, not deleted.** The `@note` block shared by the four twins asked every backend author to deliver `onRegistered`/`onError` from a thread on which `~Bridge` could not run concurrently — morph#486's use-after-free. That reasoning is still true and now lives in `docs/spec/core/backend.md`, "What was wrong with the old shape" (what the contract was, and why `registerHandlerImpl` was the one site that did not depend on it) and "How the threading contract becomes structural" (what replaced it), plus `docs/spec/concurrency_and_lifetimes.md`'s bind/promote-continuation bullet. Per the runner decision recorded on #571, the claim is stated narrowly in all three places: the guarantee is structural **for backends**, and `Bridge`'s four dispatch sites still name `exec::detail::inlineExecutor()`, which reproduces the old delivery thread exactly. **The morph#486 window is unchanged, not closed.** Closing it means giving `Bridge` an executor of its own, which is morph#588 and is not in this commit. **The ticket's "Also in scope" list was stale, and is not acted on.** It asks for `LocalBackend`, `SimulatedRemoteBackend` and "the 11 test doubles" across `tests/test_switch_backend.cpp`, `tests/test_bridge_lifetime.cpp` and `tests/test_client_execute_deadline.cpp` to be migrated, "candidates for `SynchronousBackendAdapter` rather than hand-editing". Checked: none of those three files contains a single reference to any of the four verbs, and neither does `LocalBackend` or `SimulatedRemoteBackend`. They need no migration — `IBackend`'s default `bindModel` runs `bindModelBlocking`, which dispatches to exactly the synchronous verb each request shape names, so they behave identically through the surface. Wrapping them in `SynchronousBackendAdapter` would be a *regression*: the adapter answers `kCallerMustNotBlock`, which would turn every `registerHandler` against a `LocalBackend` into an unbound handler. The only doubles that did override the twins are the ones in `tests/test_async_registration.cpp`, and they are migrated here. **Migrated doubles** (`tests/test_async_registration.cpp`): `AsyncRegisterBackend` now overrides `bindModel` (one verb for all three acquire shapes, deferred into the same `completeNext()`/`failNext()` queue) and answers `BindWait::kCallerMustNotBlock`, which is what reproduces "dispatch and return without waiting" — the observable behaviour the `true` return used to produce. `InlineCompletingBackend`, `ThrowingDispatchBackend`, `DoubleFiringBackend` and `AsyncBackendShim` follow; `AsyncAssignPrimaryBackend` and `SelfFiringAssignPrimaryBackend` move to `promoteModel`. `ThrowingDispatchBackend` tells its two arms apart by `request.primary` rather than `request.current`: `attachHandlerAsync`'s *first* attach carries a zero `current`, so `current` would have put both tests on the same arm — a real detail of the surface the twins hid, found by the test failing. `tests/test_backend_registration_surface.cpp`'s `RecordingBackend` drops the four twins and the four forwarding assertions with them; the adapter's forwarding test still pins every synchronous verb. **What this does and does not establish.** The suite passes, and for a deletion that proves only that nothing referenced the deleted thing. Beyond compilation: every test that previously drove the twin branch now drives the `bindModel` branch with the same double and asserts the same outcomes, including the inline settle, the inline failure, the synchronously-throwing dispatch, the double-settle, the three staleness guards and the two ~Bridge/~binding teardown races — so the migration is checked by tests that were written against the twins' behaviour, not by new ones written against the replacement. The one case where that is *not* true is `parkIfInFrame`'s double-claim guard, named above. No behavioural claim beyond that is made. Verification, on this commit, Linux/GCC 16.2.1, Debug: tests/morph_tests 22931 assertions, 1556 cases (1 failed as expected) tests/qt/morph_qt_tests 578 assertions, 79 cases tests/net/morph_net_tests 1112 assertions, 191 cases tests/net_qt_interop/..._tests 9 assertions, 2 cases examples/common/ladder_common_tests 587 assertions, 153 cases $ bash scripts/check_spec_citations.sh Prose lint OK: every pinned fact is still cited; no banned terminology found; ... $ bash scripts/test_check_spec_citations.sh scripts/check_spec_citations.sh detects every section-citation and forms-vocabulary drift it claims to. Not verified: any WASM configuration (no Emscripten toolchain), clang-tidy, and any sanitizer build. `docs/spec/core/backend.md`'s "Asynchronous registration" section is replaced by "Why registration needs a non-blocking path", which keeps the live content (the nested-`QEventLoop`/WASM rationale, the `asyncRegistrationEnabled` gate, the pre-connect queue, and what an unbound handler means) and hands the history to "What was wrong with the old shape". Every citation of the old heading is repointed — `locality.md`, `bridge.md`, `shared_instances.md`, `include/morph/core/bridge.hpp` (three sites, to `bridge.md`'s "Registration readiness" or `backend.md`'s "Waiting for a bind"), and three `examples/` files. `scripts/test_check_spec_citations.sh`'s rename fixture is repointed to the new heading, and its self-test still catches all sixteen drifts it claims to. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01VptDWG2fKr2vBnLSJcgzgW --- docs/spec/concurrency_and_lifetimes.md | 56 +-- docs/spec/core/backend.md | 380 +++++++--------- docs/spec/core/bridge.md | 22 +- docs/spec/core/locality.md | 2 +- docs/spec/core/shared_instances.md | 78 ++-- examples/common/gui/app_context.cpp | 6 +- examples/common/gui/presenter.hpp | 2 +- examples/kanban/gui/main.cpp | 2 +- include/morph/core/backend.hpp | 326 ++------------ include/morph/core/bridge.hpp | 239 +++++----- include/morph/qt/qt_websocket_backend.hpp | 4 +- scripts/test_check_spec_citations.sh | 4 +- .../client_only_facade_no_model_header.cpp | 7 +- tests/qt/test_qt_websocket.cpp | 7 +- tests/test_async_registration.cpp | 425 +++++++++--------- tests/test_backend_registration_surface.cpp | 71 +-- 16 files changed, 604 insertions(+), 1027 deletions(-) diff --git a/docs/spec/concurrency_and_lifetimes.md b/docs/spec/concurrency_and_lifetimes.md index da6fffb83..4382b5b55 100644 --- a/docs/spec/concurrency_and_lifetimes.md +++ b/docs/spec/concurrency_and_lifetimes.md @@ -318,31 +318,37 @@ that bounded wait into an unbounded one. Four dispositions, by site: contract, but still a span a `BridgeLifetime` gate must not cover. So the site stays on `liveness()`, and the residual scope of issue #489 stays open. -- **The `*Async` reply callbacks** — `attachHandlerAsync`, `ensureBoundAsync` - and `assignHandlerPrimary`, three of the four `IBackend` async hooks. (The - fourth, `registerHandlerImpl`, is covered by the `BridgeLifetime` bullet - above and is not one of these.) Each of the three keeps a - `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 states that a backend - overriding any `*Async` hook must deliver its callbacks on a thread from - 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 - here is therefore conditional on a documented contract, not on `Bridge` - alone**: a future backend delivering these replies on its own transport - thread would reopen morph#486's use-after-free, and that is a contract break - rather than a latent race to be rediscovered. +- **The bind/promote reply continuations** — `attachHandlerAsync`, + `ensureBoundAsync` and `assignHandlerPrimary`. (The fourth registration site, + `registerHandlerImpl`, is covered by the `BridgeLifetime` bullet above and is + not one of these.) Each of the three keeps a `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 the thread + the continuation is delivered on. + + Until morph#571 that thread was a **contract on the backend**, stated in the + `*Async` twins' doc comments: a backend overriding one had to deliver its + callbacks from a thread on which `~Bridge` could not run concurrently. + morph#568 moved every site onto `IBackend::bindModel`/`promoteModel` and + morph#571 deleted the twins, so there is no such contract left to state — but + the three sites name `exec::detail::inlineExecutor()` as the delivery + executor, which reproduces the old delivery thread exactly: the continuation + runs wherever the backend settled. **The window is therefore unchanged, not + closed.** `QtWebSocketBackend` is still safe for the reason it always was: it + 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. + + **What changed with the removal is who could get it wrong, not whether it can + be wrong.** A backend that settles a `bindModel` completion on its own + transport thread would still reopen morph#486's use-after-free here; the + difference is that the delivery thread is now a value one call site produces + rather than an obligation on fifteen backend authors, so closing it is a + change in one place. That change — giving `Bridge` an executor of its own — + is morph#588 and has not been made. The structural surface that replaces these four hooks — `IBackend::bindModel`/`promoteModel` — takes the executor the continuation is diff --git a/docs/spec/core/backend.md b/docs/spec/core/backend.md index 57b4c5778..1543959d1 100644 --- a/docs/spec/core/backend.md +++ b/docs/spec/core/backend.md @@ -29,7 +29,7 @@ and react to backend changes. - [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) +- [Why registration needs a non-blocking path](#why-registration-needs-a-non-blocking-path) - [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) @@ -78,7 +78,6 @@ 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`, `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`. | | `bindWaitPolicy()` | Whether a caller may block its own thread until a `bindModel`/`promoteModel` completion settles. `BindWait::kCallerMayBlock` by default. The framework callers are `Bridge::registerHandlerImpl`, `Bridge::switchBackend`'s phase 1 and `Bridge::installReconnectHandler`'s handler; see [Waiting for a bind — `bindWaitPolicy`](#waiting-for-a-bind--bindwaitpolicy). | @@ -156,183 +155,61 @@ carries the current session. A wire-backed backend that overrides `setSession`: the local path never serialises a `Context` onto a wire 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 -nested `QEventLoop` in `sendSync`. On a WASM main thread, Qt refuses to spin a -nested loop at all (`WaitForMoreEvents is not supported on the main thread -without asyncify`), so that blocking call aborts the page — the very first -`registerModel` a WASM client makes. - -`IBackend::registerModelAsync(typeId, factory, contextKey, onRegistered, -onError)` is the optional non-blocking counterpart. A backend that offers one -sends the request and returns `true` immediately, then invokes exactly one of -`onRegistered(ModelId)` / `onError(message)` once the reply arrives, on the -backend's own thread — unless the backend is destroyed first, in which case -neither fires. The default implementation returns `false` without calling -either callback. - -`Bridge::registerHandler()` (both overloads — the default-factory template and -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` — 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 -immediately after constructing the handler. `BridgeHandler::isBound()` and -`whenBound()` are the public seams for doing that without polling the binding's -`currentId` directly — see `bridge.md`, "Registration readiness". - -**Staleness guard.** The success callback captures a `weak_ptr` -pinned to the backend the request was issued against, plus the Bridge's -`liveness()` token (the same pattern `installReconnectHandler` uses). Before -applying the received `ModelId`, it checks the liveness token (skip if the -Bridge is gone) and compares the pinned backend against `loadBackend()` (skip -if a `switchBackend()` already moved past this registration — that call's own -re-registration loop already gave the binding a fresh id on the new backend, -which a stale reply must not overwrite). - -**Scope.** Only the plain (non-shared) registration path uses this — a -`BridgeHandler`'s initial construction. The re-registration `switchBackend()` -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` 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 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 -the backend is wired up, before the first `connected` signal). The queued -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 +## Why registration needs a non-blocking path + +`registerModel`/`registerModelWithContext`, and the keyed +`registerModelShared`/`attachModel` beside them, are **synchronous**: a backend +whose registration requires a round trip can only implement them by blocking +the calling thread until the reply arrives. `QtWebSocketBackend` does that via +a nested `QEventLoop` in `sendSync`. + +On a WASM main thread Qt refuses to spin a nested loop at all +(`WaitForMoreEvents is not supported on the main thread without asyncify`), so +that blocking call **aborts the page** — on the very first `registerModel` a +WASM client makes, and again on the first payload-keyed `execute()` a keyed +screen makes, which attaches. That is the whole reason a non-blocking +registration path exists; everything below this heading is a consequence of it. + +The path is [`bindModel`/`promoteModel`](#the-structural-registration-surface--bindmodel-and-promotemodel). +Between morph#26 and morph#571 it was instead four optional `*Async` twins +beside the synchronous verbs, each returning a `bool` meaning "I accepted the +request and will call exactly one callback later" or "I have no such path, +call the synchronous verb instead". They are gone; what they were for, and +what was wrong with the shape, is +[What was wrong with the old shape](#what-was-wrong-with-the-old-shape) below. +Two consequences of that history are still load-bearing and are recorded here +rather than left to be rediscovered: + +**The gate is `asyncRegistrationEnabled`, not the surface.** `bindModel` is +non-blocking *as a signature* on every backend, but only a backend that +overrides it is non-blocking *in fact*. `QtWebSocketBackend`'s override is +gated behind `QtWebSocketBackendConfig::asyncRegistrationEnabled`, which is +**off by default**: with defaults, its `bindModel` is `IBackend`'s, which runs +the blocking verb. A WASM client must set the flag. See +[`QtWebSocketBackend`](#qtwebsocketbackend--client-side-websocket-transport). + +**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 the backend is wired up, before the first `connected` signal). The +queued 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 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` - -`registerModelShared` and `attachModel` have the same problem for the same -reason, reached by a different route: a keyed screen's first payload-keyed -`execute()` attaches, and on a wire backend that attach blocks in `sendSync`, -which aborts a WASM main thread. Both therefore have an optional non-blocking -counterpart with `registerModelAsync`'s exact shape and contract — -`false` by default, `true` plus exactly one later callback when a backend opts -in: - -| Virtual | Synchronous counterpart | Preferred by | -|---|---|---| -| `registerModelSharedAsync(typeId, factory, identity, onRegistered, onError)` | `registerModelShared` | `Bridge::ensureBoundAsync` | -| `attachModelAsync(typeId, factory, identity, current, onRegistered, onError)` | `attachModel` | `Bridge::attachHandlerAsync` | - -`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 -is behind a wire protocol whose single `attach` request re-points server-side, -leaving nothing to deregister — the same division of responsibility -`QtWebSocketBackend::attachModel` already follows for a non-empty primary. - -`BridgeHandler::execute()`'s public signature and contract are unchanged; see -[shared_instances.md](shared_instances.md), "Async register-or-attach and -attach", for the caller-visible story and the `_attachMtx` locking rule these -two `Bridge` methods must obey. - -A backend may invoke either callback **inline**, from inside the dispatch call -itself — `QtWebSocketBackend`'s `!_connected` branch does exactly that, and -this pair's contract does not forbid it on the success path either. -`Bridge::attachHandlerAsync`/`ensureBoundAsync` handle that case explicitly -(they defer the outcome out of the dispatch frame rather than acting on it -under `_attachMtx`), so an inline completion is legal, not merely tolerated. - -### Promotion — `assignPrimaryAsync` - -`assignPrimary` — the *promote* half of a result-keyed action — has the same -optional non-blocking counterpart, `assignPrimaryAsync`, preferred by -`Bridge::assignHandlerPrimary` and falling back to the synchronous -`assignPrimary` when a backend returns `false`. Its `onRegistered` echoes the -`ModelId` back for symmetry with `registerModelAsync`'s callback shape, and -fires for the no-op cases `assignPrimary` documents (empty primary, dead `mid`, -key already taken, `mid` already keyed differently, `mid` created for a key it -has since been evicted from) — those are not backend -failures, so they resolve `onRegistered` exactly as the synchronous path -returns normally for them. `onError` is for a genuine backend or transport -failure only. - -### Threading contract — the callback's delivery thread - -**All four `*Async` hooks share one requirement: a backend must not deliver -`onRegistered`/`onError` on a thread from which `~Bridge` can run -concurrently.** This is a contract on the backend, not an implementation detail -of `Bridge`. - -The reason is on `Bridge`'s side. Three of the four continuations behind these -hooks — `ensureBoundAsync`, `attachHandlerAsync` and `assignHandlerPrimary` in -`core/bridge.hpp` — test `CallbackToken::active()` and then dereference `this` -(each takes `_attachMtx` and calls `loadBackend()`). Those are two steps, so a -`~Bridge` completing between them is the use-after-free of issue #486 — the same -check-then-act shape -[concurrency_and_lifetimes.md](../concurrency_and_lifetimes.md) describes. - -`registerHandlerImpl`'s callback is the exception and does **not** rely on this -contract: it holds `detail::BridgeLifetime` across its whole touch of `this` -(`_mtx`, `loadBackend()`), which is safe there because nothing inside that span -calls into consumer code or a blocking backend path. - -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. - -**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). +`Completion` exactly once, exactly like an in-flight registration would. A +*keyed* bind carries no queue: it is rejected with `"disconnected"` +immediately. See `QtWebSocketBackend`'s own section below. + +**A queued or in-flight bind means an unbound handler.** `registerHandler` +returns before the reply lands whenever the backend answers +`BindWait::kCallerMustNotBlock`, and `executeVia` then fails fast with +`"handler not bound"`. The seams for gating on it without polling are +`BridgeHandler::isBound()` and `whenBound()` — see +[bridge.md](bridge.md), "Registration readiness", and +[Waiting for a bind — `bindWaitPolicy`](#waiting-for-a-bind--bindwaitpolicy) +below for which backends answer which way. ## The structural registration surface — `bindModel` and `promoteModel` @@ -344,23 +221,42 @@ five-step set replaces it. This section describes what replaces it and why. ### What was wrong with the old shape -Not the duplication. Two things that are properties of the *signatures*: - -1. **The continuation is optional.** Each `*Async` verb returns `bool`: `true` - means "I accepted the request and will call exactly one callback later", - `false` means "I have no async path, call the synchronous verb instead". - Every call site therefore carries two paths, and no backend can be partially - migrated without the caller knowing about it. `Bridge::attachHandlerAsync`, - `ensureBoundAsync` and `assignHandlerPrimary` each carry that second path, - plus the `detail::AsyncDispatchHandoff` machinery needed because the "async" - verb may also answer inline. - -2. **The delivery thread is prose.** [Threading contract — the callback's - delivery thread](#threading-contract--the-callbacks-delivery-thread) above - states the requirement precisely, and nothing can check it: a backend that - replies on its own transport thread compiles, passes, and reopens - morph#486's use-after-free. The contract is stated in a comment because the - signature has nowhere to put it. +The shape being replaced was four optional `*Async` twins on `IBackend` — +`registerModelAsync`, `registerModelSharedAsync`, `attachModelAsync` and +`assignPrimaryAsync` — each sitting beside a synchronous verb and each +returning `bool`. morph#571 removed them; this section is kept because what +was wrong with them is what the replacement is shaped by. + +What was wrong was not the duplication. It was two things that are properties +of the *signatures*: + +1. **The continuation was optional.** `true` meant "I accepted the request and + will call exactly one callback later", `false` meant "I have no async path, + call the synchronous verb instead". Every call site therefore carried two + paths, and no backend could be partially migrated without the caller knowing + about it. `Bridge::attachHandlerAsync`, `ensureBoundAsync` and + `assignHandlerPrimary` each carried that second path, plus the + `detail::AsyncDispatchHandoff` machinery needed because the "async" verb + might also answer inline. + +2. **The delivery thread was prose.** All four twins shared one requirement: a + backend must not deliver `onRegistered`/`onError` on a thread from which + `~Bridge` can run concurrently. That was a contract on the *backend*, stated + in a `@note`, and nothing could check it — a backend that replied on its own + transport thread compiled, passed, and reopened morph#486's use-after-free. + The reason it mattered is on `Bridge`'s side and is unchanged: three of the + four continuations (`ensureBoundAsync`, `attachHandlerAsync` and + `assignHandlerPrimary`) test `CallbackToken::active()` and then dereference + `this`, and a `~Bridge` completing between those two steps is issue #486 — + the same check-then-act shape + [concurrency_and_lifetimes.md](../concurrency_and_lifetimes.md) describes. + `registerHandlerImpl`'s callback was the exception: it holds + `detail::BridgeLifetime` across its whole touch of `this`, which is safe + there because nothing inside that span calls into consumer code or a + blocking backend path. 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 replaces it @@ -369,8 +265,8 @@ carry between them: | Verb | Signature | Replaces | |---|---|---| -| `bindModel` | `virtual Completion bindModel(BindRequest, IExecutor& cbExec)` | `registerModel`, `registerModelWithContext`, `registerModelShared`, `attachModel` — and their `*Async` twins. | -| `promoteModel` | `virtual Completion promoteModel(PromoteRequest, IExecutor& cbExec)` | `assignPrimary` and `assignPrimaryAsync`. | +| `bindModel` | `virtual Completion bindModel(BindRequest, IExecutor& cbExec)` | `registerModel`, `registerModelWithContext`, `registerModelShared`, `attachModel` — and the `*Async` twins beside them. | +| `promoteModel` | `virtual Completion promoteModel(PromoteRequest, IExecutor& cbExec)` | `assignPrimary`, and the `*Async` twin beside it. | `BindRequest` carries the union of the three acquire verbs' parameters, and its *shape* — not the verb name — selects the behaviour: @@ -388,9 +284,9 @@ empty `primary` *is* `registerModelWithContext`; `attachModel` with a zero distinction three times. Both request types own their strings. `InstanceIdentity` holds `string_view`s, -which is safe for a synchronous call and safe for the `*Async` verbs only -because their one implementor copies into its envelope before returning. A -request that may outlive the frame that issued it cannot rest on that. +which is safe for a synchronous call because the call does not return until the +backend has finished with them. A request that may outlive the frame that +issued it cannot rest on that. ### How the threading contract becomes structural @@ -415,6 +311,26 @@ event-loop thread passes that thread's executor and the two-step check-then-dereference can no longer straddle a destructor, by construction rather than by the backend author having read a `@note`. +**And `Bridge` is not yet that caller.** Its four dispatch sites all name +`exec::detail::inlineExecutor()`, which runs the continuation on whichever +thread the backend settled on — deliberately the *old* delivery thread, so +morph#568 and morph#571 change no observable threading. `Bridge` owns no event +loop and has no thread of its own to name. So the window morph#486 describes is +**unchanged, not closed**: the decision moved, the value did not. For +`QtWebSocketBackend` the safety is the same by-construction safety it always +had — it must itself be used from the Qt event loop thread and settles every +reply from `onTextMessage` on that same thread, so the check and the use cannot +straddle a destructor. Its two non-reply paths do not weaken this either: a +disconnected or no-op bind settles inline, inside the caller's own frame (which +`Bridge::detail::parkIfInFrame` exists to handle), and `cancelPending` settles +the remainder from `~Bridge` itself, which is not a *concurrent* destructor. A +future backend that replied on its own transport thread would still reopen +morph#486. Giving `Bridge` an executor of its own — which would change the +window rather than merely move the decision — is **morph#588**, and is +deliberately not part of morph#571: the guarantee this surface makes structural +is a guarantee about *backends*, and for `Bridge`-mediated calls the delivery +thread remains what the backend chose. + `tests/test_backend_registration_surface.cpp` pins this: the backend settles from a thread that is asserted to be *not* the caller's, the caller's executor is a `MainThreadExecutor` that runs nothing until the test thread drains it, and @@ -556,8 +472,9 @@ four times; and there is one pending map, because `register`, shared `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 single-threaded WASM registration path — the case the four removed +`*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, @@ -574,10 +491,11 @@ thread, and a WASM main thread has no other thread to move it to. `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: +the removed non-blocking twin was the blocking +`registerModelWithContext`, so on every backend except a `QtWebSocketBackend` +with `asyncRegistrationEnabled` set, the handler was bound by the time the +constructor returned. Since morph#568 there is only `bindModel`, so the rule is +stated once, structurally: > **`registerHandler` returns a bound handler unless the backend says the > caller must not wait.** A backend that has not overridden `bindModel` gets @@ -599,7 +517,8 @@ without any intent to change that. See `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. +removed `*Async` path produced, now reachable through one surface instead of +two. Consequences, as of morph#568 and morph#593: @@ -681,15 +600,15 @@ caller that wants to gate anyway, and is required for the two | 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. | Open | +| morph#570 | The example GUIs and the WASM spike. | Landed | | morph#615 | `Bridge::switchBackend`'s phase 1 and `Bridge::installReconnectHandler`'s handler onto `bindModel`, both consulting `bindWaitPolicy()`; `switchBackend`'s rollback keys on a rejected `Completion`. Also settles who owned the reconnect half: this table used to assign it to morph#570, whose own body scopes itself to `examples/` and never mentions `bridge.hpp`. | Landed | -| morph#571 | Removes the four `*Async` verbs; migrates `LocalBackend`, `SimulatedRemoteBackend` and the test doubles. | Open | +| morph#571 | Removes the four `*Async` verbs from `IBackend` and from `SynchronousBackendAdapter`, and drops `Bridge`'s four offer-the-twin-first branches. `LocalBackend`, `SimulatedRemoteBackend` and the test doubles in `tests/test_switch_backend.cpp`, `tests/test_bridge_lifetime.cpp` and `tests/test_client_execute_deadline.cpp` needed no migration: none overrode a twin, so all reach `bindModel`'s default unchanged. | Landed | 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 +`bindModel`/`promoteModel` implementations route to exactly the synchronous verb +each request shape names — so a backend that has overridden nothing behaves +identically to how it did before the surface existed. What changed with +morph#568 was the *caller*: every `Bridge` dispatch site took the shape ```cpp bool const started = backend->Async(..., onOk, onErr); // removed by morph#571 @@ -700,11 +619,23 @@ if (!started) { ``` 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. +fallback that used to sit there. morph#571 deleted the first branch, leaving +one. No backend in the tree had overridden a `*Async` verb since morph#568, so +the second branch was already the one that always ran; the doubles in +`tests/test_async_registration.cpp` that did override them now override +`bindModel`/`promoteModel` instead and answer +`BindWait::kCallerMustNotBlock`, which is what reproduces "dispatch and return +without waiting" — the observable behaviour the `true` return used to produce. + +One thing the removal did lose, named rather than left to be found: a `bool` +twin handed `Bridge` two raw `std::function`s, so a backend that violated the +one-callback contract by firing twice reached `detail::parkIfInFrame`'s own +double-claim guard. A `Completion` cannot be settled twice — `CompletionState` +drops the second settle before any `Bridge` code sees it — so +`tests/test_async_registration.cpp`'s `DoubleFiringBackend` now pins the +observable contract ("exactly one `onDone`") while that guard inside +`parkIfInFrame` is no longer reachable from a backend at all. The guard is kept +because `parkIfInFrame` is also called from the dispatching frame. `Bridge::installReconnectHandler` and `Bridge::switchBackend`'s phase 1 were the two dispatch sites morph#568 did **not** move: both still called the @@ -726,11 +657,12 @@ chooses, which is the half morph#569 owns. 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) +*old* delivery thread, so neither morph#568 nor morph#571 changes observable +*threading*: `Bridge` owns no event loop and has no thread of its own to name. +Making it name a real one is the step that would turn the structural guarantee +into a behaviour change, and it belongs to whichever ticket gives `Bridge` such +an executor — see +[How the threading contract becomes structural](#how-the-threading-contract-becomes-structural) and morph#588. ## Error types @@ -2286,7 +2218,7 @@ inside the class calls `close()` — no thread it joins can be waiting on it. | `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`. | | `cancelPending(exc)` | Rejects the adapter's own still-unsettled `bindModel`/`promoteModel` promises with `exc`, **then** forwards to `inner`. Not a plain forward: those promises are settled from `_control` tasks the wrapped backend has never heard of (morph#619). | -| 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. | +| every other `IBackend` verb | Forwarded to `inner` unchanged. Since morph#571 those are the synchronous verbs only: the one verb that could carry a non-blocking path is `bindModel`, which this adapter reshapes. | ### Error types diff --git a/docs/spec/core/bridge.md b/docs/spec/core/bridge.md index 70fc19f94..8ef21f889 100644 --- a/docs/spec/core/bridge.md +++ b/docs/spec/core/bridge.md @@ -86,7 +86,7 @@ the `ModelId` value the active backend assigned; 0 = unbound. The last three fields are the state behind [`isBound()` / `whenBound()`](#registration-readiness--isbound--whenbound). `registrationInFlight` is `true` from just *before* `registerHandlerImpl` calls -`IBackend::registerModelAsync` until the resulting `onRegistered`/`onError` +`IBackend::bindModel` until the resulting `Completion` callback resolves. It is set unconditionally on every path, the synchronous fallback included — that fallback does not *leave* it set, because it resolves the waiters and clears the flag before returning; `registrationWaiters` holds the callbacks queued while it is. @@ -138,8 +138,10 @@ abort the page. Three consequences: backend. Returns the `shared_ptr`. An overload accepts a pre-built binding (for dependency injection, custom `contextKey`, or custom factory captures). Both funnel through a shared `registerHandlerImpl`, which -prefers the backend's `IBackend::registerModelAsync` when it offers one (see -`backend.md`, "Asynchronous registration") and falls back to the synchronous +dispatches `IBackend::bindModel` (see +`backend.md`, "The structural registration surface") and waits for it only when +the backend says it may — otherwise it returns unbound, exactly as the removed +non-blocking twin's caller did. It no longer falls back to the synchronous `registerModelWithContext` otherwise — the returned binding may therefore come back **unbound** (`currentId == 0`) if the backend registered asynchronously and the reply has not arrived yet. @@ -526,10 +528,10 @@ subscription. ## Registration readiness — `isBound()` / `whenBound()` -A handler built over a backend that offers `registerModelAsync` comes back +A handler built over a backend that answers `BindWait::kCallerMustNotBlock` comes back **unbound**: `registerHandlerImpl` returns as soon as the request is sent, and `currentId` stays `0` until the reply arrives (`backend.md`, -["Asynchronous registration"](backend.md#asynchronous-registration--registermodelasync)). +["Why registration needs a non-blocking path"](backend.md#why-registration-needs-a-non-blocking-path)). `executeVia` fails fast with `"handler not bound"` for anything dispatched in that window. The window is unavoidable — it is a network round trip — so the contract this pair provides is not that it can be closed, but that a caller can @@ -580,14 +582,14 @@ implementation would be wrong: - **`registrationInFlight` is set *before* the backend call, not after.** No backend documented here invokes `onRegistered` synchronously from inside - `registerModelAsync`, but one could; setting the flag afterwards would leave + a non-blocking bind, but one could; setting the flag afterwards would leave a window in which the registration has already resolved while a concurrent `whenBound()` still reads "nothing in flight" and answers `false`. - **`whenBound()` re-checks `isBound()` under `registrationMtx` after its lock-free check.** The resolving callback binds the id and settles the waiters as two steps; a caller landing between them would otherwise queue a waiter onto a list that has already been drained, and wait forever. -- **The synchronous fallback settles waiters too.** When `registerModelAsync` +- **A bind that settles inline settles waiters too.** When `bindModel` returns `false` and `registerHandlerImpl` falls back to `registerModelWithContext`, it routes through the same `resolveRegistrationWaiters` rather than clearing the flag directly — a @@ -823,7 +825,7 @@ calling `registerHandler()`, and do not mutate it concurrently with that call.** Afterwards the ordinary `_attachMtx` rule applies. See morph#505. The guarantee is unconditional, including for a backend that completes its -`attachModelAsync`/`registerModelSharedAsync` callback **inline** — from inside +`bindModel` completion **inline** — from inside the dispatch call itself, while the dispatching frame still holds `_attachMtx` (`QtWebSocketBackend` does exactly this on its `!_connected` error branch). Such a callback does not act: it parks its outcome in a @@ -948,7 +950,7 @@ make teardown order-independent.) |---|---|---| | ctor | `explicit Bridge(unique_ptr)` | Installs reconnect handler on the backend, then pushes the (initially empty) default session via `setSession`. | | dtor | `~Bridge()` | Clears the active backend's reconnect handler, then cancels all pending completions with `BridgeDestroyedError`. | -| `registerHandler` | `shared_ptr registerHandler()` | Default factory. Prefers `IBackend::registerModelAsync`; see `backend.md`. | +| `registerHandler` | `shared_ptr registerHandler()` | Default factory. Dispatches `IBackend::bindModel`; see `backend.md`. | | `registerHandler(binding)` | `void registerHandler(const shared_ptr&)` | Pre-built binding. Same async-preferring behavior. | | `switchBackend` | `void switchBackend(unique_ptr)` / `void switchBackend(shared_ptr)` | Pushes the current default session onto the new backend via `setSession` before staging. Stages all re-registrations through `bindModel` on the new backend, commits (publishes new ids + swaps) only if all succeed, else rolls back and rethrows leaving old backend + `currentId`s intact. Atomic exactly when the new backend answers `kCallerMayBlock`; a `kCallerMustNotBlock` backend's binds are deferred and the switch is not all-or-nothing (see above). Cancels old backend's pending ops with `BackendChangedError`. Holds both `_mtx` and `_attachMtx` for its staging and commit, and resolves `whenBound()` waiters after releasing them. The `unique_ptr` overload is a template on the concrete backend type and delegates to the `shared_ptr` one — see below. | | `deregisterHandler` | `void deregisterHandler(const shared_ptr&)` | Deregisters from active backend (if bound), resets `currentId` to 0, removes from tracking. | @@ -993,7 +995,7 @@ make teardown order-independent.) | `contextKey` | `string` | Stable identity for remote backends (optional, empty by default). | | `currentId` | `atomic` | Backend-assigned model id; 0 = unbound. Read lock-free by `isBound()`. | | `registrationMtx` | `mutex` | Guards the two fields below. The binding's own lock, not `Bridge::_mtx`/`_attachMtx`: it is taken from the backend's reply-delivering thread as well as the registering one. | -| `registrationInFlight` | `bool` | `true` from just before `registerModelAsync` is called until its callback settles. The synchronous fallback path never leaves it set. | +| `registrationInFlight` | `bool` | `true` from just before `bindModel` is dispatched until its completion settles. A bind that settles inside the call never leaves it set. | | `registrationWaiters` | `vector, function>>` | `whenBound()` callbacks queued while a registration is in flight; invoked and cleared exactly once, by the same call that clears `registrationInFlight`. | ## Design decisions diff --git a/docs/spec/core/locality.md b/docs/spec/core/locality.md index 4c73c01de..063fbb0db 100644 --- a/docs/spec/core/locality.md +++ b/docs/spec/core/locality.md @@ -26,7 +26,7 @@ only tabulates the *differences*, and links there for depth. | 6 | **Computed fields and validators** | `recomputeAll` and `ActionValidator::ready` **do** run — this is deliberately *not* a locality difference. | Same. | [forms.md](../forms/forms.md) | | 7 | **Payload reference semantics** | The model receives the caller's own object. | The payload is a JSON round-trip, so the model receives a reconstruction. Anything not part of the serialised shape does not survive. | Implied by the wire model | | 8 | **Failure set** | Cannot produce transport failures. | Adds failures with no local analogue — `DisconnectedError` (raised only from a transport backend), `TimeoutError`, and the server's `err "server busy"`. | [backend.md](backend.md) | -| 9 | **Registration cost** | A map insert. | A blocking round-trip (a nested `QEventLoop`, or a condvar park). | [backend.md](backend.md), "Asynchronous registration" | +| 9 | **Registration cost** | A map insert. | A blocking round-trip (a nested `QEventLoop`, or a condvar park). | [backend.md](backend.md), "Why registration needs a non-blocking path" | | 10 | **Instance lifetime** | Reclaimed only when the handler goes out of scope. | Additionally reclaimed by connection close: a `register` is attributed to a connection scope, and `closeConnection(cid)` reclaims everything in it. | [backend.md](backend.md), "Connection scopes" | | 11 | **Subscriptions** | `subscribe` fans out to subscribers on the same `Bridge`. | Does not cross the wire — the string `subscribe` does not occur in `remote.hpp` at all. A remote peer's mutation notifies nobody locally. | [bridge.md](bridge.md) | | 12 | **Thread context and latency** | One strand hop. | Differs per transport; `backend.md` carries three separate tables. | [backend.md](backend.md), "Thread context" | diff --git a/docs/spec/core/shared_instances.md b/docs/spec/core/shared_instances.md index fc50e12cf..061aa1c5c 100644 --- a/docs/spec/core/shared_instances.md +++ b/docs/spec/core/shared_instances.md @@ -352,26 +352,30 @@ purposes, which the framework's opt-in discipline forbids. ## Async register-or-attach and attach No wire change: the three requests above are unchanged. What changed is that a -backend may now answer them *without blocking the caller*, through two opt-in -`IBackend` virtuals that mirror `registerModelAsync`'s established shape -(see [backend.md](backend.md), "Asynchronous registration"): +backend may now answer them *without blocking the caller*, through the one +structural acquire verb (see [backend.md](backend.md), "The structural +registration surface"): -| Virtual | Synchronous counterpart | Preferred by | +| `BindRequest` shape | Synchronous counterpart | Dispatched by | |---|---|---| -| `registerModelSharedAsync(typeId, factory, identity, onRegistered, onError)` | `registerModelShared` | `Bridge::ensureBoundAsync` | -| `attachModelAsync(typeId, factory, identity, current, onRegistered, onError)` | `attachModel` | `Bridge::attachHandlerAsync` | - -Both default to returning `false` without calling either callback; a backend -that opts in sends the request, returns `true` immediately, and later invokes -exactly one of `onRegistered(ModelId)` / `onError(message)` on its own thread. -`QtWebSocketBackend` implements both, gated behind the *same* -`QtWebSocketBackendConfig::asyncRegistrationEnabled` flag `registerModelAsync` -already uses — there is no second knob. Their replies route through the -existing `callId`-keyed pending-registration map, which is verb-agnostic: -`register` (shared or not) and `attach` all reply `ok` with a `modelId`, or -`err`. An empty `identity.primary` degrades to the private async path -(`registerModelAsync`), mirroring the synchronous methods' own -degrade-to-private behaviour rather than inventing new semantics. +| `primary` non-empty, `current` zero | `registerModelShared` | `Bridge::ensureBoundAsync` | +| `primary` non-empty, `current` non-zero | `attachModel` | `Bridge::attachHandlerAsync` | + +`IBackend::bindModel`'s default runs exactly the synchronous verb the shape +names and settles before returning, so a backend that overrides nothing behaves +as it always did. `QtWebSocketBackend` overrides it, gated behind +`QtWebSocketBackendConfig::asyncRegistrationEnabled` — one knob for every +shape. Replies route through the existing `callId`-keyed pending map, which is +verb-agnostic: `register` (shared or not) and `attach` all reply `ok` with a +`modelId`, or `err`. An empty `primary` degrades to a private bind, mirroring +the synchronous methods' own degrade-to-private behaviour rather than inventing +new semantics. + +Until morph#571 this was expressed as two *optional* `IBackend` virtuals +(`registerModelSharedAsync`/`attachModelAsync`) returning `bool`, beside two +more for the private bind and the promote. They are gone; the reasoning that +survived them is in [backend.md](backend.md), "What was wrong with the old +shape". **Why this exists.** `registerModelShared`/`attachModel` are synchronous, so on a wire backend they block in a nested `QEventLoop`, which a WASM main thread @@ -386,11 +390,12 @@ a payload- or result-keyed action's attach/promote step never throws out of the call but resolves the returned `Completion`'s `.onError(...)` instead. Only *how* that promise is kept changed: `execute()` now routes its keyed dispatch through `Bridge::attachHandlerAsync` / `Bridge::ensureBoundAsync`, which use the -async virtuals when the backend has them and otherwise run the identical -synchronous attach inline and call back before returning. A backend that has not -opted in behaves byte-for-byte as it did before. The one observable difference on -a backend that *has* opted in is that the dispatch happens after the attach's -reply arrives rather than on the calling stack — which is the point. +dispatch `IBackend::bindModel`: a backend with a genuinely non-blocking bind +settles it later, one without settles it inline, having run the identical +synchronous attach before returning. A backend that has not overridden +`bindModel` behaves byte-for-byte as it did before. The one observable +difference on a backend that *has* is that the dispatch happens after the +attach's reply arrives rather than on the calling stack — which is the point. **`attach()` stays synchronous.** The standalone `handler.attach(key)` is a `void` call with no `Completion` to route a failure through, so it still throws @@ -408,12 +413,13 @@ action, and a result-keyed dispatch promotes its binding through `assignHandlerPrimary`, which takes `_attachMtx` itself. It is the same rule `registerHandlerImpl` already follows for `_mtx`. -The rule holds unconditionally, including for a backend that completes its -callback **inline** — synchronously, from inside `attachModelAsync` / -`registerModelSharedAsync`, while the dispatching frame still holds the lock. -`QtWebSocketBackend` does this today on its `!_connected` branch (it reports -`onError("disconnected")` and returns `true`), and nothing in `IBackend` -forbids a backend from doing it on the *success* path too. An inline callback +The rule holds unconditionally, including for a backend that settles its +completion **inline** — synchronously, from inside `bindModel`, while the +dispatching frame still holds the lock (the executor those call sites name is +`exec::detail::inlineExecutor()`, so an inline settle runs the continuation +right there). `QtWebSocketBackend` does this today on its `!_connected` branch +(it rejects with `"disconnected"`), and nothing in `IBackend` forbids a backend +from doing it on the *success* path too. An inline callback therefore parks its outcome instead of acting on it, and the dispatching frame applies it after its own dispatch call returns: publish under the lock it already holds, release, then report. See @@ -438,14 +444,14 @@ Until then, a caller should not fire the same keyed action twice back-to-back before the first settles. **The result-keyed *promote* step has since been covered too.** This section -made the **bind** half of a result-keyed action async (`ensureBoundAsync` → -`registerModelSharedAsync`). At the time of writing the **promote** half still +made the **bind** half of a result-keyed action non-blocking +(`ensureBoundAsync`). At the time of writing the **promote** half still called the synchronous `IBackend::assignPrimary` — a `sendSync`, and so a nested `QEventLoop`, on `QtWebSocketBackend` — which blocked and aborted a WASM page. -That is no longer true: `IBackend::assignPrimaryAsync` exists -([backend.md](backend.md#promotion--assignprimaryasync)), -`QtWebSocketBackend` overrides it, and `Bridge::assignHandlerPrimary` prefers it, -falling back to the synchronous call only when a backend returns `false`. +That is no longer true: `IBackend::promoteModel` is its structural counterpart +([backend.md](backend.md#the-structural-registration-surface--bindmodel-and-promotemodel)), +`QtWebSocketBackend` overrides it, and `Bridge::assignHandlerPrimary` calls it +unconditionally — there is no synchronous branch left to fall back to. ## Ownership and authorization @@ -523,7 +529,7 @@ strictly reduces pressure on it. | `handler.primary()` | `std::optional` | The handler's current primary; empty if unattached. | | `handler.instances()` | `Completion>` | Snapshot of live shared keys for this model type. | | `handler.execute(keyedAction)` | `Completion` | Unchanged signature and contract. Its **attach** step (payload-keyed) and the **bind** step of the result-keyed path take the backend's async path when one exists, so neither blocks on a round-trip — visible only as *not aborting a WASM main thread*. The result-keyed path's **promote** step (`assignPrimary`) is still synchronous and still blocks. See [Async register-or-attach and attach](#async-register-or-attach-and-attach). | -| `IBackend::registerModelSharedAsync` / `attachModelAsync` | `bool` | Opt-in non-blocking counterparts to `registerModelShared`/`attachModel`; `false` by default, and callers then fall back to the synchronous method unchanged. | +| `IBackend::bindModel` | `Completion` | The one non-blocking acquire verb; its `BindRequest`'s shape selects private / register-or-attach / re-point. The default runs the synchronous verb that shape names and settles before returning, so a backend that overrides nothing is unchanged. | ## Design decisions diff --git a/examples/common/gui/app_context.cpp b/examples/common/gui/app_context.cpp index 655009bf9..ebfc1c6d3 100644 --- a/examples/common/gui/app_context.cpp +++ b/examples/common/gui/app_context.cpp @@ -24,9 +24,9 @@ AppContext::AppContext(Mode mode) { auto& remote = std::get(mode); // asyncRegistrationEnabled: the synchronous registerModel path nests a // QEventLoop, which aborts a WASM page outright (`examples/TESTING.md`, - // "WASM reality"). Registering before the socket connects now queues and - // retries once it does (`docs/spec/core/backend.md`, - // "Asynchronous registration"), but this class still defers via + // "WASM reality"). A private bind issued before the socket connects is + // queued and sent once it does (`docs/spec/core/backend.md`, + // "Why registration needs a non-blocking path"), but this class still defers via // setConnectHandler below rather than registering immediately — simpler // to reason about than relying on the queue, and what makes the // readiness contract in this class's doc comment necessary. diff --git a/examples/common/gui/presenter.hpp b/examples/common/gui/presenter.hpp index 5e36a4849..2b7d1abfc 100644 --- a/examples/common/gui/presenter.hpp +++ b/examples/common/gui/presenter.hpp @@ -36,7 +36,7 @@ class Presenter : public QObject { /// settles — i.e. once `Bridge::whenBound()`'s `Completion` /// resolves, however it resolves. `Remote` mode's registration is /// a round trip (`docs/spec/core/backend.md`, - /// "Asynchronous registration"): a `BridgeHandler` built the + /// "Why registration needs a non-blocking path"): a `BridgeHandler` built the /// instant the socket connects is handed back *unbound* /// (`currentId == 0`) and rejects every dispatch with "handler /// not bound" until the register reply lands. A subclass that diff --git a/examples/kanban/gui/main.cpp b/examples/kanban/gui/main.cpp index af6e79b59..73b217bdf 100644 --- a/examples/kanban/gui/main.cpp +++ b/examples/kanban/gui/main.cpp @@ -131,7 +131,7 @@ int main(int argc, char** argv) { // readiness contract, gui/app_context.hpp): a registration issued before // the socket connects is queued, and the handler it belongs to stays // unbound until the register reply lands - // (`docs/spec/core/backend.md`, "Asynchronous registration"). + // (`docs/spec/core/backend.md`, "Why registration needs a non-blocking path"). ::morph::ladder::gui::AppContext ctx{ serverUrl ? ::morph::ladder::gui::AppContext::Mode{::morph::ladder::gui::Remote{.url = *serverUrl}} : ::morph::ladder::gui::AppContext::Mode{::morph::ladder::gui::Local{.workers = 4}}}; diff --git a/include/morph/core/backend.hpp b/include/morph/core/backend.hpp index dea0ed6cf..6523d06f4 100644 --- a/include/morph/core/backend.hpp +++ b/include/morph/core/backend.hpp @@ -93,8 +93,8 @@ struct InstanceIdentity { /// style preference: a bind may outlive the frame that issued it, so a /// `string_view` into the caller's stack is a dangling read waiting for a /// backend that copies its envelope after the dispatch call returns rather -/// than before it. The legacy `*Async` verbs take views and are safe only -/// because their one implementor happens to encode before returning. +/// than before it. The synchronous verbs below take views and are safe only +/// because they do not return until the backend has finished with them. struct BindRequest { /// @brief String type-id of the model to instantiate (from `ModelTraits`). std::string typeId; @@ -218,133 +218,6 @@ struct IBackend { return registerModel(typeId, std::move(factory)); } - /// @brief Optional non-blocking counterpart to `registerModelWithContext`. - /// - /// `registerModelWithContext`/`registerModel` are synchronous: a backend - /// whose registration requires a round-trip (a socket backend) can only - /// implement that by blocking the calling thread until the reply arrives — - /// `QtWebSocketBackend` does this via a nested `QEventLoop`. On a WASM main - /// thread, Qt refuses to spin a nested loop at all - /// (`WaitForMoreEvents is not supported on the main thread without asyncify`), - /// so that blocking call aborts the page — the very first `registerModel` - /// a WASM client makes. - /// - /// Overriding this lets such a backend register without blocking: send the - /// request and return `true` immediately, then invoke exactly one of - /// @p onRegistered / @p onError once the reply arrives, on the backend's own - /// thread (unless the backend is destroyed first, in which case neither - /// fires). `Bridge::registerHandler()` prefers this path when it is - /// available and falls back to the synchronous `registerModelWithContext` - /// otherwise, so every backend that has not opted in (every backend as of - /// this writing, other than `QtWebSocketBackend`) is unaffected. - /// - /// The default implementation offers no async path and returns `false` - /// without calling either callback — the caller (`Bridge::registerHandler`) - /// falls back to `registerModelWithContext` in that case, so a backend - /// with no override behaves synchronously. - /// - /// @note **Threading contract, shared by all four `*Async` hooks: the - /// callback's thread must not be able to run `~Bridge` concurrently.** - /// Three of the four `Bridge` continuations behind these hooks — - /// `attachHandlerAsync`, `ensureBoundAsync` and `assignHandlerPrimary` - /// in `core/bridge.hpp` — test `CallbackToken::active()` and then - /// dereference `this`. Those are two steps, so a `~Bridge` that - /// completes between them is morph#486's use-after-free. - /// (`registerHandlerImpl`'s callback is the exception: it holds - /// `detail::BridgeLifetime` across its whole touch of `this`, so it - /// does not depend on this contract.) - /// - /// Those 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 delivery on the thread that owns the - /// `Bridge`. `QtWebSocketBackend` — the only backend in the tree - /// overriding any of these — satisfies that by construction: it must - /// itself be used from the Qt event loop thread - /// (`qt/qt_websocket_backend.hpp`), and every *reply-driven* callback - /// fires from `onTextMessage` on that same thread, so check and use - /// cannot straddle a destructor. Its two non-reply paths do not weaken - /// this: a disconnected or no-op dispatch invokes the callback inline, - /// still inside the caller's own frame (which `detail::parkIfInFrame` - /// exists to handle), and `cancelPending` fires the remainder from - /// `~Bridge` itself — which is not a *concurrent* destructor. **A backend that delivers these callbacks on - /// a thread the `Bridge`'s owner does not control breaks this contract - /// and reopens that use-after-free** — it is a contract break, not a - /// latent race to be discovered. See morph#489 and - /// docs/spec/concurrency_and_lifetimes.md. - /// - /// @note Scope: only `Bridge::registerHandler()`'s plain (non-shared) - /// registration path — a `BridgeHandler`'s initial construction — - /// uses this. Shared/keyed registration has its own opt-in async - /// pair, `registerModelSharedAsync`/`attachModelAsync` below, - /// preferred by `Bridge::ensureBoundAsync`/`attachHandlerAsync`. - /// The re-registration `switchBackend()`/the reconnect handler - /// perform after a backend swap remains synchronous; see - /// docs/spec/core/backend.md. - /// @param typeId String type-id of the model to instantiate. - /// @param factory Callable that constructs the `IModelHolder` (local path only). - /// @param contextKey Stable identity of the new instance; empty if none. - /// @param onRegistered Invoked with the assigned `ModelId` on success. - /// @param onError Invoked with a diagnostic message on failure. - /// @return `true` if this backend accepted the request and will invoke - /// exactly one callback later; `false` if it has no async path - /// (neither callback is invoked in that case). - virtual bool registerModelAsync(const std::string& typeId, - std::function()> factory, - std::string_view contextKey, - std::function onRegistered, - std::function onError) { - (void)typeId; - (void)factory; - (void)contextKey; - (void)onRegistered; - (void)onError; - return false; - } - - /// @brief Optional non-blocking counterpart to `registerModelShared`. - /// - /// Same rationale and shape as `registerModelAsync` (see its doc comment - /// immediately above): `registerModelShared`'s synchronous default - /// implementations block the calling thread until a reply arrives, which - /// aborts a WASM main thread the moment a shared/keyed handler makes its - /// first attach. A backend that overrides this sends the request and - /// returns `true` immediately, then invokes exactly one of - /// @p onRegistered / @p onError once the reply arrives, on the backend's - /// own thread (unless the backend is destroyed first, in which case - /// neither fires) — subject to `registerModelAsync`'s threading contract, - /// which applies here unchanged: that thread must not be able to run - /// `~Bridge` concurrently. - /// - /// The default implementation offers no async path and returns `false` - /// without calling either callback — the caller (`Bridge::ensureBoundAsync`) - /// falls back to the synchronous `registerModelShared` in that case, so a - /// backend with no override behaves synchronously. - /// - /// @param typeId String type-id of the model. - /// @param factory Callable that constructs the `IModelHolder` (local path only). - /// @param identity Entity key for the action log plus the directory primary key. - /// @param onRegistered Invoked with the assigned/attached `ModelId` on success. - /// @param onError Invoked with a diagnostic message on failure. - /// @return `true` if this backend accepted the request and will invoke - /// exactly one callback later; `false` if it has no async path. - // NOLINTBEGIN(performance-unnecessary-value-param) — by-value matches - // registerModelAsync's signature exactly; overriding backends move the - // callbacks into their pending-reply map. - virtual bool registerModelSharedAsync( - const std::string& typeId, std::function()> factory, - InstanceIdentity identity, std::function onRegistered, - std::function onError) { - (void)typeId; - (void)factory; - (void)identity; - (void)onRegistered; - (void)onError; - return false; - } - // NOLINTEND(performance-unnecessary-value-param) - /// @brief Registers or attaches to the shared instance holding @p primary. /// /// A *register-or-attach*: if an instance for `(typeId, primary)` is already @@ -408,45 +281,6 @@ struct IBackend { return next; } - /// @brief Optional non-blocking counterpart to `attachModel`. - /// - /// Same rationale and shape as `registerModelSharedAsync` immediately - /// above (itself mirroring `registerModelAsync`) — see that doc comment - /// for the full opt-in/fallback contract, and `registerModelAsync`'s for - /// the threading contract the callback's delivery thread must satisfy. - /// - /// @note Unlike the synchronous `attachModel` default above, this method - /// does *not* release @p current itself: an overriding backend is - /// behind a wire protocol, whose single `attach` request re-points - /// server-side and therefore leaves nothing to deregister — exactly - /// the division of responsibility `QtWebSocketBackend::attachModel` - /// already follows for a non-empty `identity.primary`. @p current is - /// passed so that request can name what it is re-pointing from. - /// - /// @param typeId String type-id of the model. - /// @param factory Callable that constructs the `IModelHolder` (local path only). - /// @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 this backend accepted the request and will invoke - /// exactly one callback later; `false` if it has no async path. - // NOLINTBEGIN(performance-unnecessary-value-param) — see registerModelSharedAsync above. - virtual bool attachModelAsync(const std::string& typeId, - std::function()> factory, - InstanceIdentity identity, ::morph::exec::detail::ModelId current, - std::function onRegistered, - std::function onError) { - (void)typeId; - (void)factory; - (void)identity; - (void)current; - (void)onRegistered; - (void)onError; - return false; - } - // NOLINTEND(performance-unnecessary-value-param) - /// @brief Enters an already-live instance into the directory under @p primary. /// /// The *promotion* half of keyed instances, and what makes a result-sourced @@ -477,60 +311,6 @@ struct IBackend { (void)primary; } - /// @brief Optional non-blocking counterpart to `assignPrimary`. - /// - /// `assignPrimary` is synchronous: a backend whose promote step requires a - /// round-trip (a socket backend) can only implement that by blocking the - /// calling thread until the reply arrives — `QtWebSocketBackend` does this - /// via a nested `QEventLoop`, exactly the blocking shape - /// `registerModelAsync` exists to let a caller avoid for the bind step. - /// The promote step (the second half of a shared handler's result-keyed - /// `execute()`, after `Bridge::ensureBound`) reaches the same nested loop - /// on a single-threaded WASM build, which cannot spin one at all. - /// - /// Overriding this lets such a backend promote without blocking: send the - /// request and return `true` immediately, then invoke exactly one of - /// @p onRegistered / @p onError once the reply arrives, on the backend's - /// own thread (unless the backend is destroyed first, in which case - /// neither fires) — subject to `registerModelAsync`'s threading contract, - /// which applies here unchanged: that thread must not be able to run - /// `~Bridge` concurrently. `Bridge::assignHandlerPrimary` prefers this path when - /// it is available and falls back to the synchronous `assignPrimary` - /// otherwise, so every backend that has not opted in (every backend as of - /// this writing, other than `QtWebSocketBackend`) is unaffected. - /// - /// The default implementation offers no async path and returns `false` - /// without calling either callback — the caller (`Bridge::assignHandlerPrimary`) - /// falls back to `assignPrimary` in that case, so a backend with no - /// override behaves synchronously. - /// - /// @param mid Live instance to promote. - /// @param typeId Model type id — the directory's first key component. - /// @param primary Canonical string encoding of the key to file it under. - /// @param onRegistered Invoked with @p mid (echoed back, for symmetry with - /// `registerModelAsync`'s callback shape) on success — - /// including the no-op cases `assignPrimary` documents - /// (empty primary, dead `mid`, key already taken, `mid` - /// already keyed differently): those are not backend - /// failures, so they resolve `onRegistered` exactly as - /// the synchronous path returns normally for them. - /// @param onError Invoked with a diagnostic message on a genuine - /// backend/transport failure. - /// @return `true` if this backend accepted the request and will invoke - /// exactly one callback later; `false` if it has no async path - /// (neither callback is invoked in that case). - virtual bool assignPrimaryAsync(::morph::exec::detail::ModelId mid, const std::string& typeId, - std::string_view primary, - std::function onRegistered, - std::function onError) { - (void)mid; - (void)typeId; - (void)primary; - (void)onRegistered; - (void)onError; - return false; - } - // ── The structural registration surface ────────────────────────────── // // `bindModel`/`promoteModel` are what the five verbs above become once the @@ -559,9 +339,12 @@ struct IBackend { // 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. - // That is the whole of the threading contract the four `*Async` verbs - // could only state in prose (see `registerModelAsync`'s `@note`, and - // docs/spec/core/backend.md, "The structural registration surface"). + // That is the whole of the threading contract the four `*Async` + // twins morph#571 removed could only state in prose: they asked + // every backend author to deliver on a thread from which `~Bridge` + // could not run concurrently, and nothing could check it. See + // docs/spec/core/backend.md, "The threading contract, and the half + // the surface does not close". // It does not by itself make a `~Bridge` race impossible: it moves the // choice of delivery thread from fifteen backend implementors, none of // which knows what the caller's teardown looks like, to the one caller @@ -569,8 +352,13 @@ struct IBackend { // nowhere" cannot be expressed — a null executor would silently drop // every continuation (see `Completion`'s constructor). // - // Retiring the five older verbs is morph#571; until then both surfaces - // exist and nothing in the tree has moved off the old one. + // morph#571 retired the four optional `*Async` twins. The synchronous + // verbs above remain, but no longer as a surface any caller chooses: + // they are what `bindModelBlocking` — and therefore the *default* + // `bindModel` — dispatches to, one request shape at a time. Nothing in + // the tree calls them directly any more, which is why a backend that + // overrides only `registerModel` still works through `bindModel` + // unchanged. /// @brief Acquires a model instance: the structural counterpart of /// `registerModelWithContext` / `registerModelShared` / `attachModel`. @@ -612,11 +400,11 @@ struct IBackend { /// counterpart of `assignPrimary`. /// /// The default implementation calls `assignPrimary` and settles the - /// returned `Completion` from this thread, echoing @p request's `mid` back - /// exactly as `assignPrimaryAsync` documents — including for every - /// documented no-op case (empty primary, dead `mid`, key already taken, - /// `mid` already keyed differently), which are not backend failures and - /// therefore resolve rather than reject. + /// returned `Completion` from this thread, echoing @p request's `mid` + /// back — including for every no-op case `assignPrimary` documents + /// (empty primary, dead `mid`, key already taken, `mid` already keyed + /// differently), which are not backend failures and therefore resolve + /// rather than reject. /// /// @param request Owning promote request. By value, matching `bindModel`, /// so an overriding backend can move it into its pending @@ -991,9 +779,12 @@ class SynchronousBackendAdapter : public detail::IBackend { // ── Everything else is forwarded unchanged ─────────────────────────── // - // A decorator has to forward every verb it does not reshape, including the - // four legacy `*Async` twins: wrapping a backend that *does* have a - // non-blocking path must not quietly take it away. + // A decorator has to forward every verb it does not reshape. Since + // morph#571 those are the synchronous verbs only: a wrapped backend has + // no non-blocking path of its own left to forward, because the one verb + // that could carry one — `bindModel` — is the verb this adapter + // reshapes, and a backend that already has a non-blocking `bindModel` + // has no reason to be wrapped. /// @brief Forwards to the wrapped backend. /// @param typeId String type-id of the model to instantiate. @@ -1039,58 +830,6 @@ class SynchronousBackendAdapter : public detail::IBackend { return _inner->attachModel(typeId, std::move(factory), identity, current); } - /// @brief Forwards to the wrapped backend. - /// @param typeId String type-id of the model to instantiate. - /// @param factory Callable that constructs the `IModelHolder`. - /// @param contextKey Stable identity of the new instance; empty if none. - /// @param onRegistered Invoked with the assigned `ModelId` on success. - /// @param onError Invoked with a diagnostic message on failure. - /// @return Whatever the wrapped backend returned. - bool registerModelAsync(const std::string& typeId, - std::function()> factory, - std::string_view contextKey, - std::function onRegistered, - std::function onError) override { - return _inner->registerModelAsync(typeId, std::move(factory), contextKey, std::move(onRegistered), - std::move(onError)); - } - - // NOLINTBEGIN(performance-unnecessary-value-param) — the overridden - // signatures take these by value; see `IBackend::registerModelSharedAsync`. - /// @brief Forwards to the wrapped backend. - /// @param typeId String type-id of the model. - /// @param factory Callable that constructs the `IModelHolder`. - /// @param identity Entity key for the action log plus the directory primary key. - /// @param onRegistered Invoked with the assigned/attached `ModelId` on success. - /// @param onError Invoked with a diagnostic message on failure. - /// @return Whatever the wrapped backend returned. - bool registerModelSharedAsync(const std::string& typeId, - std::function()> factory, - detail::InstanceIdentity identity, - std::function onRegistered, - std::function onError) override { - return _inner->registerModelSharedAsync(typeId, std::move(factory), identity, std::move(onRegistered), - std::move(onError)); - } - - /// @brief Forwards to the wrapped backend. - /// @param typeId String type-id of the model. - /// @param factory Callable that constructs the `IModelHolder`. - /// @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 Whatever the wrapped backend returned. - bool attachModelAsync(const std::string& typeId, - std::function()> factory, - detail::InstanceIdentity identity, ::morph::exec::detail::ModelId current, - std::function onRegistered, - std::function onError) override { - return _inner->attachModelAsync(typeId, std::move(factory), identity, current, std::move(onRegistered), - std::move(onError)); - } - // NOLINTEND(performance-unnecessary-value-param) - /// @brief Forwards to the wrapped backend. /// @param mid Live instance to promote. /// @param typeId Model type id — the directory's first key component. @@ -1100,19 +839,6 @@ class SynchronousBackendAdapter : public detail::IBackend { _inner->assignPrimary(mid, typeId, primary); } - /// @brief Forwards to the wrapped backend. - /// @param mid Live instance to promote. - /// @param typeId Model type id — the directory's first key component. - /// @param primary Canonical string encoding of the key to file it under. - /// @param onRegistered Invoked with @p mid on success. - /// @param onError Invoked with a diagnostic message on failure. - /// @return Whatever the wrapped backend returned. - bool assignPrimaryAsync(::morph::exec::detail::ModelId mid, const std::string& typeId, std::string_view primary, - std::function onRegistered, - std::function onError) override { - return _inner->assignPrimaryAsync(mid, typeId, primary, std::move(onRegistered), std::move(onError)); - } - /// @brief Forwards to the wrapped backend. /// @param typeId String type-id to enumerate. /// @return The wrapped backend's snapshot of live shared instance keys. diff --git a/include/morph/core/bridge.hpp b/include/morph/core/bridge.hpp index 86a73aec2..6ae75aa74 100644 --- a/include/morph/core/bridge.hpp +++ b/include/morph/core/bridge.hpp @@ -288,11 +288,12 @@ struct StagedRebinds { /// /// `Bridge::attachHandlerAsync`/`ensureBoundAsync` dispatch to the backend while /// holding `_attachMtx`, and both promise to release it before invoking their -/// `onDone`. A backend whose `attachModelAsync`/`registerModelSharedAsync` -/// completed its callback *inline* — synchronously, before the dispatch call -/// returned, as `QtWebSocketBackend` does on its `!_connected` error branch — -/// would otherwise break that promise from inside the dispatch frame, with the -/// lock still held. +/// `onDone`. A backend whose `bindModel` settles its `Completion` *inline* — +/// synchronously, before the dispatch call returned, as `QtWebSocketBackend` +/// does on its `!_connected` error branch — would otherwise break that promise +/// from inside the dispatch frame, with the lock still held (the executor +/// those call sites name is `inlineExecutor()`, so an inline settle runs the +/// continuation right there). /// /// So instead of acting, such a callback parks its outcome here and returns; the /// dispatching frame picks it up after the dispatch call returns, publishes it @@ -604,9 +605,8 @@ class Bridge { /// invokes @p onDone once attached (or failed), instead of blocking. /// /// 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 + /// registration surface, and since morph#571 the only one. 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 @@ -685,13 +685,14 @@ class Bridge { 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. + // morph#486 shape. Closed not by a gate but by the thread the + // continuation is delivered on. Before morph#571 that was a + // prose contract on every backend author; now it is the executor + // this call site names -- today `inlineExecutor()`, which keeps + // the pre-morph#568 delivery thread, so the window is unchanged + // rather than closed (morph#588). Gating instead would block + // `~Bridge` behind `_attachMtx`, which `attachHandler` holds + // across a full `attachModel` round trip. See morph#489. if (!weakLiveness.active()) { return; // The Bridge is gone; publishing this id would be pointless. } @@ -742,41 +743,35 @@ class Bridge { onDone(failure); }; try { - bool const started = backend->attachModelAsync( - binding->typeId, binding->modelFactory, {.contextKey = primaryCopy, .primary = primaryCopy}, previous, - 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); - } + // 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 second path here. + // + // `inlineExecutor()` because that is where the continuation ran + // before morph#568: on whichever thread the backend settled the + // reply on, which the removed `*Async` twins could only ask for in + // prose. 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 (...) { - // 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. + // `IBackend::bindModel`'s own default converts a throwing + // synchronous verb into a rejection, and every backend in the tree + // does the same. This guard is for an out-of-tree override that + // throws out of the dispatch call itself (e.g. a `wire::encode()` + // failing before send): report it like any other failure rather + // than let it escape execute()'s documented never-throws contract. lock.unlock(); onDone(std::current_exception()); return; @@ -856,12 +851,14 @@ class Bridge { } // 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. + // thread that cannot run `~Bridge` concurrently. Before morph#571 + // that thread was the backend's choice, asked for in prose; now + // the `bindModel` call below names it as an executor -- today + // `inlineExecutor()`, which keeps the same delivery thread, so + // the window is unchanged rather than closed (morph#588). Gating + // instead would block `~Bridge` behind `_attachMtx`, which + // `attachHandler` holds across a full `attachModel` round trip. + // See morph#489. if (!weakLiveness.active()) { return; // The Bridge is gone; publishing this id would be pointless. } @@ -898,29 +895,22 @@ class Bridge { onDone(failure); }; try { - 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); - } + // The structural surface; see `attachHandlerAsync`'s identical + // dispatch 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: a backend's own legacy - // dispatch call can throw synchronously before send. + // See attachHandlerAsync's identical guard: an out-of-tree + // `bindModel` override can throw out of the dispatch call itself. lock.unlock(); onDone(std::current_exception()); return; @@ -961,9 +951,8 @@ class Bridge { /// wire backends, a reply field) — tracked as a follow-up, not fixed /// here. /// 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 + /// registration surface, and since morph#571 the only one. + /// The same "avoid a nested-event-loop block that aborts a WASM /// main thread" rationale `Bridge::registerHandler()` follows for the /// initial bind step applies here: this method is invoked from inside the /// result `Completion`'s callback chain (`BridgeHandler::execute`'s @@ -1005,12 +994,14 @@ class Bridge { 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. + // thread that cannot run `~Bridge` concurrently. Before morph#571 + // that thread was the backend's choice, asked for in prose; now + // the `promoteModel` call below names it as an executor -- today + // `inlineExecutor()`, which keeps the same delivery thread, so the + // window is unchanged rather than closed (morph#588). Gating + // instead would block `~Bridge` behind `_attachMtx`, which + // `attachHandler` holds across a full `attachModel` round trip. + // See morph#489. if (!weakLiveness.active()) { return; // The Bridge is gone; do not touch `this`. } @@ -1040,26 +1031,22 @@ class Bridge { 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)); - }); - } + // The structural surface (`IBackend::promoteModel`). A backend with + // no non-blocking promote settles the returned `Completion` from + // inside this call, having run the same synchronous `assignPrimary` + // this replaces; `inlineExecutor()` then delivers `onPromoted` on + // this thread, exactly where the synchronous call used to publish. + // Unlike that call, a failure is logged rather than thrown: this runs + // inside the result `Completion`'s callback chain, where an escaping + // exception is swallowed by `CompletionState` anyway, and + // `promoteModel` reports through the `Completion` by contract. + 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)); + }); } /// @brief Returns @p binding's current primary key, or empty if unattached. @@ -1072,8 +1059,9 @@ class Bridge { /// @brief Whether @p binding currently has a live `ModelId`. /// - /// A binding constructed via the async registration path (see - /// `backend.md`, "Asynchronous registration") starts unbound and becomes + /// A binding constructed against a backend that answers + /// `BindWait::kCallerMustNotBlock` (see `backend.md`, "Waiting for a + /// bind") starts unbound and becomes /// bound only once `onRegistered` fires — this is the synchronous, /// point-in-time check; `whenBound()` below is the awaitable counterpart. /// @param binding Binding to inspect. @@ -1085,8 +1073,9 @@ class Bridge { /// @brief Resolves once @p binding's initial registration settles. /// /// Closes the gap `executeVia`'s fast-fail leaves for a caller that - /// constructs a `BridgeHandler` through the async registration path (see - /// `backend.md`, "Asynchronous registration") and wants to dispatch the + /// constructs a `BridgeHandler` against a backend that answers + /// `BindWait::kCallerMustNotBlock` (see `backend.md`, "Waiting for a + /// bind") and wants to dispatch the /// moment registration completes, rather than failing fast with "handler /// not bound" or polling `isBound()` in a loop of its own devising. /// @@ -1849,10 +1838,8 @@ class Bridge { } /// @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. + /// `IBackend::bindModel`, the structural registration surface, and + /// since morph#571 the only one. /// /// A backend with a non-blocking bind returns an unsettled `Completion` and /// the binding is returned unbound (see `IBackend::bindModel`'s doc comment @@ -1916,12 +1903,6 @@ class Bridge { // registration attempt is over. auto [onRegistered, onFailed] = makeBindCallbacks(binding, backend, "[registerHandler]"); - 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 @@ -1974,8 +1955,8 @@ class Bridge { // 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. + // `whenBound()`, exactly as the removed `*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); @@ -2613,10 +2594,11 @@ class BridgeHandler { // returned Completion's onError, exactly like every other // dispatch failure — not as a synchronous throw out of execute(). // - // The attach goes through Bridge::attachHandlerAsync, which uses - // the backend's `attachModelAsync` when it has one and otherwise - // runs the identical synchronous attach inline and calls back - // before returning — so a backend with no async attach path + // The attach goes through Bridge::attachHandlerAsync, which + // dispatches `IBackend::bindModel`: a backend with a genuinely + // non-blocking attach settles later, one without settles inline, + // having run the identical synchronous attach and called back + // before returning — so a backend with no non-blocking path // still behaves synchronously here, just through the async // interface. auto state = std::make_shared<::morph::async::detail::CompletionState>(); @@ -2802,8 +2784,9 @@ class BridgeHandler { /// @brief Whether this handler currently has a live backend instance. /// - /// A handler constructed through the async registration path (see - /// `backend.md`, "Asynchronous registration") starts unbound and only + /// A handler constructed against a backend that answers + /// `BindWait::kCallerMustNotBlock` (see `backend.md`, "Waiting for a + /// bind") starts unbound and only /// becomes bound once the deferred `onRegistered` fires; `execute()` /// fails fast with "handler not bound" for any call issued before that. /// This is the synchronous, point-in-time check; `whenBound()` below is diff --git a/include/morph/qt/qt_websocket_backend.hpp b/include/morph/qt/qt_websocket_backend.hpp index 8454745b1..95556924f 100644 --- a/include/morph/qt/qt_websocket_backend.hpp +++ b/include/morph/qt/qt_websocket_backend.hpp @@ -374,8 +374,8 @@ class QtWebSocketBackend : public ::morph::backend::detail::IBackend { /// `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. + /// preserve — which is why the optional non-blocking promote this + /// replaces (removed by morph#571) 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 diff --git a/scripts/test_check_spec_citations.sh b/scripts/test_check_spec_citations.sh index 014033cb6..66bd5c115 100755 --- a/scripts/test_check_spec_citations.sh +++ b/scripts/test_check_spec_citations.sh @@ -153,8 +153,8 @@ expect_caught "a citation naming a markdown file that does not resolve" \ # *spec* renames the section, exactly as a spec edit would, and every citation # of it must go dangling. expect_caught "a spec renaming a section its code comments cite" \ - "edit docs/spec/core/backend.md -e 's/^## Asynchronous registration .*\$/## Async registration — \`registerModelAsync\`/'" \ - 'has no section "Asynchronous registration"' + "edit docs/spec/core/backend.md -e 's/^## Why registration needs a non-blocking path\$/## Why registration needs a nonblocking path/'" \ + 'has no section "Why registration needs a non-blocking path"' # Vacuity guard on the scan itself. A stale glob here would leave the check # reporting green having verified nothing -- the exact failure mode that let diff --git a/tests/compile_checks/client_only_facade_no_model_header.cpp b/tests/compile_checks/client_only_facade_no_model_header.cpp index 694d89739..99c089a9d 100644 --- a/tests/compile_checks/client_only_facade_no_model_header.cpp +++ b/tests/compile_checks/client_only_facade_no_model_header.cpp @@ -89,9 +89,10 @@ struct InlineExecutor : morph::exec::IExecutor { // all -- deliberately NOT morph::model::detail::ModelHolder, // which would require ClientOnlyFacadeModel complete (it stores one by // value). LocalBackend::registerModel calls its factory SYNCHRONOUSLY inside -// Bridge::registerHandler(binding) (registerModelWithContext has no async -// path for LocalBackend -- see IBackend::registerModelAsync's doc comment), -// so the factory must succeed and return a real holder rather than throw; +// Bridge::registerHandler(binding) (LocalBackend does not override bindModel, +// so IBackend's default runs registerModelWithContext inline -- see +// IBackend::bindModel's doc comment), so the factory must succeed and return +// a real holder rather than throw; // throwing here would escape BridgeHandler's pre-built-binding constructor // itself, uncaught, in this probe's main() -- not the MORPH_CLIENT_ONLY // dispatch-time throw this probe means to observe. diff --git a/tests/qt/test_qt_websocket.cpp b/tests/qt/test_qt_websocket.cpp index 0812bb478..c6386d6bf 100644 --- a/tests/qt/test_qt_websocket.cpp +++ b/tests/qt/test_qt_websocket.cpp @@ -2555,9 +2555,10 @@ int main(int argc, char* argv[]) { // ── 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 -// three synchronous counterparts stamped it. RemoteServer authenticates and +// The three optional non-blocking control verbs that existed then (the +// register-or-attach, re-point and promote twins, all removed by morph#571) +// each built their envelope and encoded it with no `env.session = _session`, +// while all three synchronous counterparts stamped it. RemoteServer authenticates and // 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 -- diff --git a/tests/test_async_registration.cpp b/tests/test_async_registration.cpp index 1ee1b69f6..d81917907 100644 --- a/tests/test_async_registration.cpp +++ b/tests/test_async_registration.cpp @@ -1,14 +1,24 @@ // SPDX-License-Identifier: Apache-2.0 // -// Coverage for issue #26: Bridge::registerHandler() prefers -// IBackend::registerModelAsync when a backend offers one, falling back to the -// synchronous registerModelWithContext otherwise. AsyncRegisterBackend below -// is a minimal test double whose registerModelAsync defers its reply until -// the test explicitly completes it -- simulating a socket backend whose reply -// arrives later on its own thread (what QtWebSocketBackend's async path does -// against a real server), instead of blocking the calling thread via a nested -// event loop (what registerModel does today -- the pattern this issue is -// about, since Qt refuses to spin a nested loop on a WASM main thread at all). +// Coverage for issue #26: Bridge::registerHandler() and the keyed +// attach/promote entry points reach the backend through the structural +// registration surface (IBackend::bindModel / promoteModel), and a backend +// whose reply arrives later must not block the caller. +// +// AsyncRegisterBackend below is a minimal test double whose bindModel returns +// an unsettled Completion and defers the reply until the test explicitly +// completes it -- simulating a socket backend whose reply arrives later on its +// own thread (what QtWebSocketBackend does against a real server), instead of +// blocking the calling thread via a nested event loop (what the synchronous +// registerModel does -- the pattern this issue is about, since Qt refuses to +// spin a nested loop on a WASM main thread at all). +// +// Until morph#571 that shape was expressed by overriding four optional +// `*Async` twins that returned `bool`; the doubles here now express it by +// overriding bindModel/promoteModel and answering +// BindWait::kCallerMustNotBlock, which is what makes registerHandlerImpl +// return without waiting -- the same observable behaviour the `true` return +// used to produce. #include #include @@ -45,9 +55,8 @@ struct ARModel { int execute(const ARCount& a) { return a.x; } }; -// --- Keyed/shared coverage: the same deferred-reply idea applied to -// --- registerModelSharedAsync/attachModelAsync (the register-or-attach and -// --- attach counterparts of registerModelAsync). +// --- Keyed/shared coverage: the same deferred-reply idea applied to the +// --- register-or-attach and re-point shapes of the same bindModel request. /// Names the instance it wants in the action payload -> payload-keyed, so /// executing it attaches the handler first (Bridge::attachHandlerAsync). @@ -161,7 +170,7 @@ struct morph::model::ActionKeyTraits { BRIDGE_MODEL_KEY(ARKeyedModel, ARTouch, &ARTouch::id); BRIDGE_KEY_FROM_RESULT(ARKeyedCreate, &ARKeyedCreated::id); -// ── Issue #67: assignHandlerPrimary prefers IBackend::assignPrimaryAsync ──── +// ── Issue #67: assignHandlerPrimary goes through IBackend::promoteModel ──── // // A model whose result-keyed action (BRIDGE_KEY_FROM_RESULT) drives // Bridge::assignHandlerPrimary. Needs **external** linkage (not an anonymous @@ -194,9 +203,11 @@ BRIDGE_MODEL_KEY_FROM_RESULT(ARCreateModel, ARCreate, &ARCreated::id); namespace { -// Offers an async registration path that does not complete until the test -// calls completeNext()/failNext() -- simulating a backend whose registration -// reply arrives later, asynchronously, instead of blocking the caller. +using ModelCompletion = morph::async::Completion; + +// Offers a non-blocking bind that does not complete until the test calls +// completeNext()/failNext() -- simulating a backend whose registration reply +// arrives later, asynchronously, instead of blocking the caller. class AsyncRegisterBackend : public morph::backend::detail::IBackend { public: morph::exec::detail::ModelId registerModel( @@ -229,45 +240,20 @@ class AsyncRegisterBackend : public morph::backend::detail::IBackend { void cancelPending(const std::exception_ptr&) override {} void setReconnectHandler(const std::function&) override {} - bool registerModelAsync(const std::string& typeId, - std::function()> factory, - std::string_view /*contextKey*/, - std::function onRegistered, - std::function onError) override { - std::scoped_lock const lock{_pendingMtx}; - _pending.push_back(Pending{typeId, std::move(factory), std::move(onRegistered), std::move(onError)}); - return true; - } - - // The shared/keyed counterparts, deferred exactly the same way: the reply - // lands in the same queue completeNext()/failNext() drain, so a keyed - // attach is observably non-blocking for the same reason a plain - // registration is. - bool registerModelSharedAsync(const std::string& typeId, - std::function()> factory, - morph::backend::detail::InstanceIdentity /*identity*/, - std::function onRegistered, - std::function onError) override { - std::scoped_lock const lock{_pendingMtx}; - _pending.push_back(Pending{.typeId = typeId, - .factory = std::move(factory), - .onRegistered = std::move(onRegistered), - .onError = std::move(onError)}); - return true; + // One verb for all three acquire shapes (private, register-or-attach, + // re-point): each is deferred the same way, so the reply lands in the same + // queue completeNext()/failNext() drain and a keyed attach is observably + // non-blocking for the same reason a plain registration is. + ModelCompletion bindModel(morph::backend::detail::BindRequest request, morph::exec::IExecutor& cbExec) override { + auto [completion, promise] = ModelCompletion::makeSettleable(&cbExec); + queue(request.typeId, std::move(request.factory), std::move(promise)); + return std::move(completion); } - 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 { - std::scoped_lock const lock{_pendingMtx}; - _pending.push_back(Pending{.typeId = typeId, - .factory = std::move(factory), - .onRegistered = std::move(onRegistered), - .onError = std::move(onError)}); - return true; + // The point of the double: the caller must not stop and wait, because + // nothing settles until the test says so. + [[nodiscard]] morph::backend::detail::BindWait bindWaitPolicy() const noexcept override { + return morph::backend::detail::BindWait::kCallerMustNotBlock; } void assignPrimary(morph::exec::detail::ModelId mid, const std::string& /*typeId*/, @@ -309,6 +295,23 @@ class AsyncRegisterBackend : public morph::backend::detail::IBackend { return _pending.size(); } +protected: + /// @brief Parks one bind request until completeNext()/failNext() settles it. + void queue(std::string typeId, std::function()> factory, + ModelCompletion::Promise promise) { + auto kept = std::make_shared(std::move(promise)); + std::scoped_lock const lock{_pendingMtx}; + _pending.push_back(Pending{ + .typeId = std::move(typeId), + .factory = std::move(factory), + .onRegistered = [kept](morph::exec::detail::ModelId mid) { kept->resolve(mid); }, + .onError = + [kept](const std::string& message) { + kept->reject(std::make_exception_ptr(std::runtime_error(message))); + }, + }); + } + private: struct Pending { std::string typeId; @@ -325,96 +328,79 @@ class AsyncRegisterBackend : public morph::backend::detail::IBackend { uint64_t _nextId{100}; }; -// Completes its async attach/bind callbacks *inline* -- synchronously, from -// inside attachModelAsync/registerModelSharedAsync itself, before the dispatch -// call returns. This is legal (nothing in IBackend forbids it) and it is what -// QtWebSocketBackend already does on its !_connected error branch, so -// Bridge::attachHandlerAsync/ensureBoundAsync must survive it: at that moment -// the Bridge is still holding _attachMtx around the dispatch, and anything the -// callback does that re-enters the Bridge under that lock -- publishing the -// binding's primary, or a result-keyed dispatch's assignHandlerPrimary -- -// self-deadlocks unless the outcome is deferred out of the dispatch frame. +// Settles its bind *inline* -- synchronously, from inside bindModel itself, +// before the dispatch call returns. This is legal (nothing in IBackend forbids +// it) and it is what QtWebSocketBackend already does on its !_connected error +// branch, so Bridge::attachHandlerAsync/ensureBoundAsync must survive it: at +// that moment the Bridge is still holding _attachMtx around the dispatch, and +// anything the callback does that re-enters the Bridge under that lock -- +// publishing the binding's primary, or a result-keyed dispatch's +// assignHandlerPrimary -- self-deadlocks unless the outcome is deferred out of +// the dispatch frame. +// +// The executor those call sites name is inlineExecutor(), so resolving here +// runs the continuation on this very stack. class InlineCompletingBackend : public AsyncRegisterBackend { public: - /// @param failInline When set, both methods report this message via onError + /// @param failInline When set, the bind is rejected with this message /// inline instead of succeeding. explicit InlineCompletingBackend(std::optional failInline = std::nullopt) : _failInline{std::move(failInline)} {} - bool registerModelSharedAsync(const std::string& typeId, - std::function()> factory, - morph::backend::detail::InstanceIdentity /*identity*/, - std::function onRegistered, - std::function onError) override { - completeInline(typeId, std::move(factory), onRegistered, onError); - return true; - } - - 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 { - completeInline(typeId, std::move(factory), onRegistered, onError); - return true; - } - -private: - void completeInline(const std::string& typeId, - std::function()> factory, - const std::function& onRegistered, - const std::function& onError) { + ModelCompletion bindModel(morph::backend::detail::BindRequest request, morph::exec::IExecutor& cbExec) override { + auto [completion, promise] = ModelCompletion::makeSettleable(&cbExec); if (_failInline) { - onError(*_failInline); - return; + promise.reject(std::make_exception_ptr(std::runtime_error(*_failInline))); + } else { + promise.resolve(registerModel(request.typeId, std::move(request.factory))); } - onRegistered(registerModel(typeId, std::move(factory))); + return std::move(completion); } +private: std::optional _failInline; }; -// A backend whose async dispatch call itself throws synchronously, before -// returning -- e.g. QtWebSocketBackend::attachModelAsync's wire::encode() -// failing before send. Bridge::attachHandlerAsync/ensureBoundAsync must -// report this through onDone (matching execute()'s documented never-throws -// contract) rather than letting it escape. +// A backend whose dispatch call itself throws synchronously, before returning +// -- e.g. a wire::encode() failing before send. `IBackend::bindModel`'s own +// default converts a throwing synchronous verb into a rejection, so only an +// override can produce this shape; Bridge::attachHandlerAsync/ensureBoundAsync +// must still report it through onDone (matching execute()'s documented +// never-throws contract) rather than letting it escape. +// +// The two arms are told apart by the request's shape, exactly as the surface +// intends. `primary`, not `current`, is what separates them here: +// attachHandlerAsync names the key it wants (non-empty `primary`, and a zero +// `current` on a first attach), while ensureBoundAsync asks for an anonymous +// instance (`primary` empty). class ThrowingDispatchBackend : public AsyncRegisterBackend { public: - bool attachModelAsync(const std::string&, std::function()>, - morph::backend::detail::InstanceIdentity, morph::exec::detail::ModelId, - std::function, - std::function) override { - throw std::runtime_error("attachModelAsync dispatch failed"); - } - - bool registerModelSharedAsync(const std::string&, - std::function()>, - morph::backend::detail::InstanceIdentity, - std::function, - std::function) override { - throw std::runtime_error("registerModelSharedAsync dispatch failed"); + ModelCompletion bindModel(morph::backend::detail::BindRequest request, + morph::exec::IExecutor& /*cbExec*/) override { + if (!request.primary.empty()) { + throw std::runtime_error("bindModel keyed dispatch failed"); + } + throw std::runtime_error("bindModel anonymous dispatch failed"); } }; -// Violates IBackend's documented "exactly one callback per dispatch" -// contract by invoking onRegistered twice, inline, from inside -// attachModelAsync itself. Exercises detail::parkIfInFrame's own guard -// against a second callback claiming an outcome that inline dispatch already -// parked -- Bridge::attachHandlerAsync must still report exactly once. +// Tries to settle the same bind twice, inline, from inside bindModel itself. +// Bridge::attachHandlerAsync must still report exactly once. +// +// Note what moved with morph#571: the twins handed the Bridge two raw +// std::functions, so a second call reached detail::parkIfInFrame's own guard. +// A Completion cannot be settled twice -- CompletionState drops the second +// settle before any Bridge code sees it -- so this double now pins the +// *observable* contract ("exactly one onDone") while the guard inside +// parkIfInFrame is no longer reachable from a backend. See the PR for morph#571. class DoubleFiringBackend : public AsyncRegisterBackend { public: - 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 { - auto mid = registerModel(typeId, std::move(factory)); - onRegistered(mid); - onRegistered(mid); // Contract violation: fires a second time inline. - return true; + ModelCompletion bindModel(morph::backend::detail::BindRequest request, morph::exec::IExecutor& cbExec) override { + auto [completion, promise] = ModelCompletion::makeSettleable(&cbExec); + auto mid = registerModel(request.typeId, std::move(request.factory)); + promise.resolve(mid); + promise.resolve(mid); // Contract violation: settles a second time inline. + return std::move(completion); } }; @@ -440,60 +426,57 @@ class AsyncBackendShim : public morph::backend::detail::IBackend { void notifyBackendChanged() override { _target->notifyBackendChanged(); } void cancelPending(const std::exception_ptr& exc) override { _target->cancelPending(exc); } void setReconnectHandler(const std::function& handler) override { _target->setReconnectHandler(handler); } - bool registerModelAsync(const std::string& typeId, - std::function()> factory, - std::string_view contextKey, - std::function onRegistered, - std::function onError) override { - return _target->registerModelAsync(typeId, std::move(factory), contextKey, std::move(onRegistered), - std::move(onError)); - } - bool registerModelSharedAsync(const std::string& typeId, - std::function()> factory, - morph::backend::detail::InstanceIdentity identity, - std::function onRegistered, - std::function onError) override { - return _target->registerModelSharedAsync(typeId, std::move(factory), identity, std::move(onRegistered), - std::move(onError)); + ModelCompletion bindModel(morph::backend::detail::BindRequest request, morph::exec::IExecutor& cbExec) override { + return _target->bindModel(std::move(request), cbExec); } - 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 { - return _target->attachModelAsync(typeId, std::move(factory), identity, current, std::move(onRegistered), - std::move(onError)); + // Forwarded, not inherited: the wrapped backend defers every reply, so a + // shim that answered the default kCallerMayBlock would park + // registerHandlerImpl (and switchBackend's phase 1) on a completion + // nothing but the test can settle. + [[nodiscard]] morph::backend::detail::BindWait bindWaitPolicy() const noexcept override { + return _target->bindWaitPolicy(); } private: std::shared_ptr _target; }; -// Offers an async assignPrimary path that does not complete until the test -// calls completeNext()/failNext() -- the assignHandlerPrimary counterpart of +// Offers a non-blocking promote that does not complete until the test calls +// completeNext()/failNext() -- the assignHandlerPrimary counterpart of // AsyncRegisterBackend above, simulating a backend (QtWebSocketBackend is the // one real example) whose promote-in-place reply arrives later, on its own -// thread, instead of Bridge::assignHandlerPrimary falling back to the -// synchronous assignPrimary. Everything else (registration, execute) is -// delegated to a real LocalBackend so a result-keyed action's ensureBound() -// step behaves normally; only the promotion step is deferred. +// thread, instead of settling inside the promoteModel call. Everything else +// (registration, execute) is delegated to a real LocalBackend so a +// result-keyed action's ensureBound() step behaves normally; only the +// promotion step is deferred. class AsyncAssignPrimaryBackend : public morph::backend::LocalBackend { public: explicit AsyncAssignPrimaryBackend(morph::exec::IExecutor& pool) : LocalBackend{pool} {} - bool assignPrimaryAsync(morph::exec::detail::ModelId mid, const std::string& typeId, std::string_view primary, - std::function onRegistered, - std::function onError) override { + ModelCompletion promoteModel(morph::backend::detail::PromoteRequest request, + morph::exec::IExecutor& cbExec) override { + auto [completion, promise] = ModelCompletion::makeSettleable(&cbExec); + auto kept = std::make_shared(std::move(promise)); std::scoped_lock const lock{_pendingMtx}; - _pending.push_back(Pending{mid, typeId, std::string{primary}, std::move(onRegistered), std::move(onError)}); - return true; + _pending.push_back(Pending{ + .mid = request.mid, + .typeId = request.typeId, + .primary = request.primary, + .onRegistered = [kept](morph::exec::detail::ModelId mid) { kept->resolve(mid); }, + .onError = + [kept](const std::string& message) { + kept->reject(std::make_exception_ptr(std::runtime_error(message))); + }, + }); + return std::move(completion); } - // Test hooks: settle the oldest still-pending async promotion. Unlike + // Test hooks: settle the oldest still-pending promotion. Unlike // AsyncRegisterBackend::completeNext(), this does not also call the real - // (synchronous) assignPrimary -- assignPrimaryAsync's contract is that the - // backend performs the promotion itself and merely reports back, so the - // test double's completion is the promotion. + // (synchronous) assignPrimary from the reply path by accident: + // promoteModel's contract is that the backend performs the promotion + // itself and merely reports back, so the test double's completion is the + // promotion. void completeNext() { Pending pending; { @@ -532,18 +515,18 @@ class AsyncAssignPrimaryBackend : public morph::backend::LocalBackend { std::vector _pending; }; -// A backend with no async registration path at all (registerModelSharedAsync -// defaults to `return false`, so ensureBoundAsync always falls back to the -// 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. +// A backend with no `bindModel` override at all, so `IBackend`'s default runs +// the synchronous verb the request shape names -- and that verb throws. +// Exercises ensureBoundAsync's own failure path (Task 15a finding B2), the +// ensureBoundAsync counterpart of attachHandlerAsync's identical-shaped one, +// and with it `bindModel`'s default promise of turning a throwing synchronous +// verb into a rejection rather than a throw. // // 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 +// as its first statement. `bindModelBlocking` 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 @@ -576,9 +559,9 @@ class ThrowingSyncRegisterSharedBackend : public morph::backend::detail::IBacken } }; -// A backend whose assignPrimaryAsync defers exactly like AsyncAssignPrimaryBackend +// A backend whose promoteModel defers exactly like AsyncAssignPrimaryBackend // above, but which -- instead of relying on a test-driven completeNext() -- -// fires its one still-pending completion synchronously from inside its own +// settles its one still-pending completion synchronously from inside its own // destructor. This models a backend torn down (e.g. by switchBackend()) while // a promotion reply is still in flight, self-reporting success as a last act // of teardown: exactly the shape needed to exercise assignHandlerPrimary's @@ -600,28 +583,22 @@ class SelfFiringAssignPrimaryBackend : public morph::backend::LocalBackend { if (_pending) { auto pending = std::move(*_pending); _pending.reset(); - pending.onRegistered(pending.mid); + pending.promise->resolve(pending.mid); } } - bool assignPrimaryAsync(morph::exec::detail::ModelId mid, const std::string& typeId, std::string_view primary, - std::function onRegistered, - std::function onError) override { - _pending = Pending{.mid = mid, - .typeId = typeId, - .primary = std::string{primary}, - .onRegistered = std::move(onRegistered), - .onError = std::move(onError)}; - return true; + ModelCompletion promoteModel(morph::backend::detail::PromoteRequest request, + morph::exec::IExecutor& cbExec) override { + auto [completion, promise] = ModelCompletion::makeSettleable(&cbExec); + _pending = + Pending{.mid = request.mid, .promise = std::make_shared(std::move(promise))}; + return std::move(completion); } private: struct Pending { morph::exec::detail::ModelId mid; - std::string typeId; - std::string primary; - std::function onRegistered; - std::function onError; + std::shared_ptr promise; }; std::optional _pending; }; @@ -1032,11 +1009,11 @@ TEST_CASE( CHECK(message == "registration did not complete: the reply was discarded and 'AR_Model' is still unbound"); } -TEST_CASE("Bridge::registerHandler: falls back to the synchronous path for a backend with no async support", +TEST_CASE("Bridge::registerHandler: binds inline for a backend with no non-blocking path", "[bridge][registration][issue26]") { - // LocalBackend does not override registerModelAsync, so the default - // (returns false) applies and registerHandler falls back to - // registerModelWithContext -- binding is bound immediately, exactly as + // LocalBackend does not override bindModel, so IBackend's default runs + // registerModelWithContext -- the verb the request's shape names -- and + // settles before returning. The binding is bound immediately, exactly as // before this feature existed. morph::exec::ThreadPoolExecutor pool{2}; morph::bridge::Bridge bridge{std::make_unique(pool)}; @@ -1164,11 +1141,12 @@ TEST_CASE("Bridge::whenBound: multiple waiters on the same in-flight registratio CHECK(resolvedCount == 3); } -// ── Issue #67: assignHandlerPrimary prefers IBackend::assignPrimaryAsync ──── +// ── Issue #67: assignHandlerPrimary goes through IBackend::promoteModel ──── // // A result-keyed action's execute() calls ensureBound() then, once the reply -// names the key, assignHandlerPrimary(). When the backend offers -// assignPrimaryAsync, Bridge::assignHandlerPrimary must send the request and +// names the key, assignHandlerPrimary(). When the backend settles its +// promoteModel completion later, Bridge::assignHandlerPrimary must send the +// request and // return without blocking, publish binding->primary/contextKey only once the // (possibly deferred) reply confirms it, and guard a stale reply the same way // registerHandlerImpl's async callback does (Bridge/binding gone, or a @@ -1195,7 +1173,7 @@ TEST_CASE("Bridge::assignHandlerPrimary: uses the async path when the backend of // The promotion reply has not arrived yet: the handler already has an // anonymous instance (ensureBound ran synchronously against LocalBackend), // so the action itself has already executed and resolved -- but the - // promotion is what assignPrimaryAsync defers, not the execute() call. + // promotion is what promoteModel defers, not the execute() call. REQUIRE(morph::testing::waitUntil([&] { return done.load(); })); REQUIRE(created.has_value()); REQUIRE(rawBackend->pendingCount() == 1); @@ -1347,7 +1325,7 @@ TEST_CASE("Bridge::assignHandlerPrimary: a never-attached binding (currentId sti // arm of the early-return guard (raw == 0U || primary.empty() || // !binding->primary.empty()) is otherwise never driven true. A handler // that has never attached anything yet has no instance to promote a key - // onto, so this must return immediately: no assignPrimaryAsync dispatch, + // onto, so this must return immediately: no promoteModel dispatch, // no pendingCount bump. morph::exec::ThreadPoolExecutor pool{2}; auto backend = std::make_unique(pool); @@ -1544,9 +1522,10 @@ TEST_CASE("Bridge::whenBound: concurrent callers racing the exact moment registr } // --------------------------------------------------------------------------- -// Shared/keyed registration: registerModelSharedAsync + attachModelAsync. +// Shared/keyed registration: the register-or-attach and re-point request +// shapes of bindModel. // -// Same opt-in/fallback contract as registerModelAsync above, reached through +// Same deferred-reply contract as the private bind above, reached through // Bridge::attachHandlerAsync (payload-keyed actions) and // Bridge::ensureBoundAsync (result-keyed ones), both of which BridgeHandler's // execute() now routes its keyed dispatches through. execute()'s own contract @@ -1557,7 +1536,7 @@ TEST_CASE("Bridge::whenBound: concurrent callers racing the exact moment registr using morph::bridge::AllowShared; // NOLINTNEXTLINE(readability-function-cognitive-complexity) -TEST_CASE("Bridge prefers attachModelAsync over the synchronous attachModel when the backend offers it", +TEST_CASE("Bridge dispatches a re-point bindModel rather than the synchronous attachModel", "[bridge][registration][shared-instances][issue26]") { SyncExec cbExec; auto backend = std::make_unique(); @@ -1589,12 +1568,11 @@ TEST_CASE("Bridge prefers attachModelAsync over the synchronous attachModel when } // NOLINTNEXTLINE(readability-function-cognitive-complexity) -TEST_CASE("A backend with no async attach path falls back to the synchronous attachModel unchanged", +TEST_CASE("A backend with no non-blocking bind runs the synchronous attachModel unchanged", "[bridge][registration][shared-instances][issue26]") { - // LocalBackend overrides neither attachModelAsync nor - // registerModelSharedAsync, so IBackend's defaults (returning false) apply - // and the keyed execute() runs the identical synchronous attach it always - // has -- bound before the dispatch, on this thread. + // LocalBackend does not override bindModel, so IBackend's default routes + // the re-point request shape to the identical synchronous attach it always + // has -- bound before the dispatch returns, on this thread. morph::exec::ThreadPoolExecutor pool{2}; SyncExec cbExec; morph::bridge::Bridge bridge{std::make_unique(pool)}; @@ -1625,7 +1603,7 @@ TEST_CASE("A backend with no async attach path falls back to the synchronous att // NOLINTNEXTLINE(readability-function-cognitive-complexity) TEST_CASE( - "attachModelAsync's onError path surfaces through the returned Completion's onError, matching the synchronous " + "a rejected re-point bindModel surfaces through the returned Completion's onError, matching the synchronous " "path's documented contract", "[bridge][registration][shared-instances][issue26]") { SyncExec cbExec; @@ -1661,7 +1639,7 @@ TEST_CASE( } // NOLINTNEXTLINE(readability-function-cognitive-complexity) -TEST_CASE("A backend that completes attachModelAsync inline does not deadlock and resolves normally", +TEST_CASE("A backend that settles a re-point bindModel inline does not deadlock and resolves normally", "[bridge][registration][shared-instances][issue26]") { // Regression guard for the inline-completion hole: attachHandlerAsync // dispatches under _attachMtx, and its success callback re-acquires that @@ -1689,7 +1667,7 @@ TEST_CASE("A backend that completes attachModelAsync inline does not deadlock an } // NOLINTNEXTLINE(readability-function-cognitive-complexity) -TEST_CASE("A backend that completes registerModelSharedAsync inline still promotes a result-keyed action", +TEST_CASE("A backend that settles a register-or-attach bindModel inline still promotes a result-keyed action", "[bridge][registration][shared-instances][issue26]") { // The sharpest form of the same hole: an inline bind runs onDone -- i.e. // the whole dispatch -- inside ensureBoundAsync's frame, and a result-keyed @@ -1717,8 +1695,8 @@ TEST_CASE("A backend that completes registerModelSharedAsync inline still promot // NOLINTNEXTLINE(readability-function-cognitive-complexity) TEST_CASE("A backend that reports its async attach failure inline surfaces it through onError, exactly once", "[bridge][registration][shared-instances][issue26]") { - // QtWebSocketBackend's !_connected branch, in miniature: onError invoked - // synchronously from inside attachModelAsync, which then returns true. + // QtWebSocketBackend's !_connected branch, in miniature: the completion + // rejected synchronously from inside bindModel itself. SyncExec cbExec; auto backend = std::make_unique(std::optional{"disconnected"}); morph::bridge::Bridge bridge{std::move(backend)}; @@ -1748,7 +1726,7 @@ TEST_CASE("A backend that reports its async attach failure inline surfaces it th // NOLINTNEXTLINE(readability-function-cognitive-complexity) TEST_CASE("ensureBoundAsync mirrors the same three cases for a result-keyed (creating) action", "[bridge][registration][shared-instances][issue26]") { - SECTION("prefers registerModelSharedAsync when the backend offers it") { + SECTION("dispatches a register-or-attach bindModel that settles later") { SyncExec cbExec; auto backend = std::make_unique(); auto* rawBackend = backend.get(); @@ -1797,7 +1775,7 @@ TEST_CASE("ensureBoundAsync mirrors the same three cases for a result-keyed (cre CHECK(handler.primary().value_or(-1) == 4242); } - SECTION("surfaces registerModelSharedAsync's onError through the returned Completion") { + SECTION("surfaces a rejected register-or-attach bindModel through the returned Completion") { SyncExec cbExec; auto backend = std::make_unique(); auto* rawBackend = backend.get(); @@ -1845,7 +1823,7 @@ TEST_CASE("attachHandlerAsync reports a synchronously-throwing dispatch call thr } })); - CHECK(message == "attachModelAsync dispatch failed"); + CHECK(message == "bindModel keyed dispatch failed"); CHECK_FALSE(succeeded.load()); CHECK_FALSE(handler.primary().has_value()); } @@ -1868,7 +1846,7 @@ TEST_CASE("ensureBoundAsync reports a synchronously-throwing dispatch call throu } })); - CHECK(message == "registerModelSharedAsync dispatch failed"); + CHECK(message == "bindModel anonymous dispatch failed"); CHECK_FALSE(succeeded.load()); CHECK_FALSE(handler.primary().has_value()); } @@ -1877,13 +1855,12 @@ TEST_CASE( "ensureBoundAsync's synchronous fallback surfaces a real registerModelShared throw through onDone " "(Task 15a finding B2)", "[bridge][registration][issue26]") { - // Distinct from the test above: ThrowingDispatchBackend's throw comes from - // 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 `bindModel`, whose - // default runs the blocking register from inside the call. That throw is + // Distinct from the test above: ThrowingDispatchBackend's throw comes out + // of the `bindModel` *dispatch call* itself, before any completion exists, + // and is caught by ensureBoundAsync's `catch (...)`. + // ThrowingSyncRegisterSharedBackend instead does not override `bindModel` + // at all, so `IBackend`'s 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. @@ -1977,7 +1954,7 @@ TEST_CASE("ensureBoundAsync's out-of-frame success callback is a no-op once the bridge.reset(); REQUIRE_NOTHROW(sharedBackend->completeNext()); - SUCCEED("completing a registerModelSharedAsync reply after the Bridge and handler are both gone did not crash"); + SUCCEED("completing a register-or-attach bind reply after the Bridge and handler are both gone did not crash"); } TEST_CASE("ensureBoundAsync's out-of-frame success callback tolerates the BridgeHandler being gone", @@ -1998,7 +1975,7 @@ TEST_CASE("ensureBoundAsync's out-of-frame success callback tolerates the Bridge handler.reset(); REQUIRE_NOTHROW(rawBackend->completeNext()); - SUCCEED("completing a registerModelSharedAsync reply after the BridgeHandler is gone did not crash"); + SUCCEED("completing a register-or-attach bind reply after the BridgeHandler is gone did not crash"); } TEST_CASE("ensureBound is a no-op when the binding already has an instance", @@ -2078,8 +2055,8 @@ TEST_CASE( TEST_CASE("ensureBoundAsync's onError path is a no-op once the dispatching frame already claimed the outcome", "[bridge][registration][shared-instances][issue26]") { - // Mirrors attachModelAsync's identical inline-failure test above, for - // registerModelSharedAsync: onError invoked synchronously from inside the + // Mirrors the re-point shape's identical inline-failure test above, for + // the register-or-attach shape: the completion rejected from inside the // dispatch call (which then returns true) exercises the parkIfInFrame // no-op inside ensureBoundAsync's error callback, not just its success one. SyncExec cbExec; @@ -2137,7 +2114,7 @@ TEST_CASE("execute() surfaces a throwing ActionKeyTraits::key() through onError TEST_CASE("attachHandlerAsync reports exactly once even when the backend fires its callback twice inline", "[bridge][registration][shared-instances][issue26]") { - // DoubleFiringBackend violates attachModelAsync's documented one-callback + // DoubleFiringBackend violates bindModel's documented one-settle // contract on purpose: detail::parkIfInFrame's `handoff.fired` guard must // swallow the second, already-claimed callback rather than letting // attachHandlerAsync invoke onDone (and, downstream, publish the binding) @@ -2215,7 +2192,7 @@ TEST_CASE("ensureBoundAsync's out-of-frame success callback is a genuine no-op o REQUIRE(weakBinding.expired()); REQUIRE_NOTHROW(rawBackend->completeNext()); - SUCCEED("completing a registerModelSharedAsync reply after the binding itself is gone did not crash"); + SUCCEED("completing a register-or-attach bind reply after the binding itself is gone did not crash"); } // --------------------------------------------------------------------------- @@ -2307,8 +2284,8 @@ TEST_CASE( } // attachHandlerAsync's in-frame claimHandoff success path (binding-> -// contextKey = primaryCopy, reached when a backend's attachModelAsync -// completes synchronously -- see InlineCompletingBackend above) has the +// contextKey = primaryCopy, reached when a backend settles its re-point +// bindModel synchronously -- see InlineCompletingBackend above) has the // identical shape of catch (...) as the out-of-frame callback the test above // targets, and is deliberately NOT given its own forced-OOM test: the whole // call happens in one stack, so several of attachHandlerAsync's own earlier diff --git a/tests/test_backend_registration_surface.cpp b/tests/test_backend_registration_surface.cpp index 4085ba4e2..8e88d5c13 100644 --- a/tests/test_backend_registration_surface.cpp +++ b/tests/test_backend_registration_surface.cpp @@ -101,43 +101,6 @@ struct RecordingBackend : IBackend { calls.emplace_back("assignPrimary:" + std::to_string(mid.v) + ":" + std::string{primary}); } - // The four legacy `*Async` twins return `true` here — the opposite of - // `IBackend`'s default — so that a forwarding assertion cannot pass - // vacuously: if `SynchronousBackendAdapter` stopped forwarding one, the - // inherited default would answer `false` and the test would fail. - bool registerModelAsync(const std::string& /*typeId*/, - std::function()> /*factory*/, - std::string_view contextKey, std::function /*onRegistered*/, - std::function /*onError*/) override { - calls.emplace_back("registerModelAsync:" + std::string{contextKey}); - return true; - } - - // NOLINTBEGIN(performance-unnecessary-value-param) — the overridden signatures take these by value. - bool registerModelSharedAsync(const std::string& /*typeId*/, - std::function()> /*factory*/, - InstanceIdentity identity, std::function /*onRegistered*/, - std::function /*onError*/) override { - calls.emplace_back("registerModelSharedAsync:" + std::string{identity.primary}); - return true; - } - - bool attachModelAsync(const std::string& /*typeId*/, - std::function()> /*factory*/, - InstanceIdentity identity, ModelId current, std::function /*onRegistered*/, - std::function /*onError*/) override { - calls.emplace_back("attachModelAsync:" + std::string{identity.primary} + ":" + std::to_string(current.v)); - return true; - } - // NOLINTEND(performance-unnecessary-value-param) - - bool assignPrimaryAsync(ModelId mid, const std::string& /*typeId*/, std::string_view primary, - std::function /*onRegistered*/, - std::function /*onError*/) override { - calls.emplace_back("assignPrimaryAsync:" + std::to_string(mid.v) + ":" + std::string{primary}); - return true; - } - std::vector listInstances(const std::string& /*typeId*/) override { calls.emplace_back("listInstances"); return {"listed"}; @@ -540,17 +503,6 @@ TEST_CASE( REQUIRE(adapter.attachModel(std::string{kTypeId}, makeHolder, {.contextKey = "ck", .primary = "pk"}, ModelId{5}) == ModelId{4}); - // The legacy `*Async` twins are forwarded, not swallowed: a wrapped backend - // that has a non-blocking path keeps it. `RecordingBackend` answers `true` - // where `IBackend`'s default answers `false`, so each of these would fail if - // the adapter stopped overriding the verb and inherited that default. - REQUIRE(adapter.registerModelAsync(std::string{kTypeId}, makeHolder, "ck", nullptr, nullptr)); - REQUIRE(adapter.registerModelSharedAsync(std::string{kTypeId}, makeHolder, {.contextKey = "ck", .primary = "pk"}, - nullptr, nullptr)); - REQUIRE(adapter.attachModelAsync(std::string{kTypeId}, makeHolder, {.contextKey = "ck", .primary = "pk"}, - ModelId{5}, nullptr, nullptr)); - REQUIRE(adapter.assignPrimaryAsync(ModelId{1}, std::string{kTypeId}, "pk", nullptr, nullptr)); - adapter.assignPrimary(ModelId{6}, std::string{kTypeId}, "pk"); REQUIRE(adapter.listInstances(std::string{kTypeId}) == std::vector{"listed"}); adapter.deregisterModel(ModelId{7}); @@ -564,11 +516,9 @@ TEST_CASE( REQUIRE(recording->calls == std::vector{"registerModel", "registerModelWithContext:ck", "registerModelShared:pk", - "attachModel:pk:5", "registerModelAsync:ck", "registerModelSharedAsync:pk", - "attachModelAsync:pk:5", "assignPrimaryAsync:1:pk", "assignPrimary:6:pk", - "listInstances", "deregisterModel:7", "execute:8", "notifyBackendChanged", - "cancelPending", "setReconnectHandler", "setConnectHandler", - "setDisconnectHandler", "setSession:pal"}); + "attachModel:pk:5", "assignPrimary:6:pk", "listInstances", "deregisterModel:7", + "execute:8", "notifyBackendChanged", "cancelPending", "setReconnectHandler", + "setConnectHandler", "setDisconnectHandler", "setSession:pal"}); } // ── `bindWaitPolicy`: the one bit `Completion` cannot carry (morph#593) ────── @@ -592,10 +542,10 @@ 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. +/// Derives from `RecordingBackend` for the verbs `Bridge` needs it to have, +/// and overrides `bindModel` so that `Bridge::registerHandlerImpl` gets an +/// unsettled `Completion` instead of `RecordingBackend`'s inherited default, +/// which settles inline off the synchronous verbs. struct TransportThreadBackend : RecordingBackend { /// @brief Policy this double reports; the thing under test. morph::backend::detail::BindWait policy = morph::backend::detail::BindWait::kCallerMayBlock; @@ -618,13 +568,6 @@ struct TransportThreadBackend : RecordingBackend { } } - 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(); From c1bbdcc8227d36187d2c805c07c1f8460be830c8 Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Mon, 21 Sep 2026 01:25:29 +0200 Subject: [PATCH 3/5] gates: repoint the two line-hint allowlists at the lines the removal moved MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Mechanical follow-up to the two commits above, kept separate so that neither ticket's diff contains a number nobody read the code for. No behaviour change. `scripts/mutation_survivors.json` — three `backend.hpp` citations. The gate reported one moved line and two "appears 2 times … not decidable", exactly as those two entries' own `reason` text predicted it would: include/morph/core/backend.hpp:1412 has moved to line 1138. ... include/morph/core/backend.hpp:1305 is allowlisted by a source line that appears 2 times (lines [1031, 1050]), and none of them is 1305, so which one is meant is not decidable. ... include/morph/core/backend.hpp:1469 is allowlisted by a source line that appears 2 times (lines [1195, 1240]), and none of them is 1469, so which one is meant is not decidable. ... Each ambiguity was resolved by reading both candidates, not by taking the first: - `registerCount` → **1031**, inside `LocalBackend::registerModel`. The other candidate, 1050, is the `registerModelShared` arm, which the entry's own reason names as the one it is *not*. - `executeInFlight` → **1195**, the statement immediately after `inFlightCounter->fetch_add(...)`. The other candidate, 1240, follows `fetch_sub` inside the posted task; the entry's reason says "the increment side of an execute". - `aware.reserve(...)` → 1138, unambiguous. `scripts/branch_partial_allowlist.json` — three entries, same shape: `backend.hpp:1414 → 1140`, `bridge.hpp:1551 → 1540`, and `bridge.hpp:1673 → 1662`. The last is the ambiguous one: three textually identical `if (deadlineHandle && schedulerRef) {` guards exist, now at 1662, 1686 and 1775. 1662 is the one in `executeVia`'s `catch (...)` block, which is what that entry's reason describes ("specifically the exception-path use of the guard"); the other two are the `.then()`/`.onError()` continuations the same reason explicitly excludes, and its parenthetical naming their old lines is updated with them. One further hit was **not** edited: `mutation_survivors.json`'s `classification_2026_09_09` sample citing `bridge.hpp:584` and `started = backend->attachModelAsync(...)`. That is a dated record of what a sampling run measured on that revision, not a description of the current tree, and rewriting it to match today's code would destroy the measurement. A `note` saying so is added beside the verdict instead. It carries no verbatim `source` and so is not audited by the gate either way. Verification: $ python3 scripts/check_mutation_survivors.py ok: 15 structured citation(s) in scripts/mutation_survivors.json resolve to the line they name. note: 20 further citation(s) in this file are free text inside prose strings, which carry no verbatim `source` and so are NOT audited here (morph#613). `scripts/check_branch_coverage.py` and `scripts/check_error_path_coverage.py` both need `build/clang-coverage/coverage.lcov`, which needs a full `scripts/coverage.sh` run and was not produced here. Their line-hint half was audited directly instead, by calling `check_branch_coverage.resolve_allowlist_source_line` over every entry of both allowlists: 0 failures for each. **Their coverage half — whether each allowlisted arm is still partial — is therefore not verified locally and rests on the CI coverage leg.** Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01VptDWG2fKr2vBnLSJcgzgW --- scripts/branch_partial_allowlist.json | 8 ++++---- scripts/mutation_survivors.json | 9 +++++---- 2 files changed, 9 insertions(+), 8 deletions(-) diff --git a/scripts/branch_partial_allowlist.json b/scripts/branch_partial_allowlist.json index 1d48d1048..4b58f03c8 100644 --- a/scripts/branch_partial_allowlist.json +++ b/scripts/branch_partial_allowlist.json @@ -80,7 +80,7 @@ }, { "file": "include/morph/core/backend.hpp", - "line": 1414, + "line": 1140, "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,15 +98,15 @@ }, { "file": "include/morph/core/bridge.hpp", - "line": 1551, + "line": 1540, "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": 1673, + "line": 1662, "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:1551 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 1697, 1786) -- 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:1540 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 1686, 1775) -- 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/scripts/mutation_survivors.json b/scripts/mutation_survivors.json index e659f1d77..bf79e936c 100644 --- a/scripts/mutation_survivors.json +++ b/scripts/mutation_survivors.json @@ -81,7 +81,7 @@ }, { "file": "include/morph/core/backend.hpp", - "line": 1412, + "line": 1138, "mutants": 1, "mutator": "cxx_replace_scalar_call", "source": "aware.reserve(_changeAware.size());", @@ -147,13 +147,13 @@ "representative_sites": [ { "file": "include/morph/core/backend.hpp", - "line": 1305, + "line": 1031, "source": "::morph::observe::detail::emitMetric(::morph::observe::Metric::registerCount, 1.0);", "reason": "The registerCount emission on LocalBackend::registerModel. Note for whoever refreshes this hint: the same statement appears character-for-character on the registerModelShared arm as well, so if this `line` ever drifts the gate will report the citation as ambiguous rather than printing a corrected line. That is the right outcome -- which of the two arms is meant is a question for a reader, not for a resolver -- and it is recorded here so the message is not a surprise." }, { "file": "include/morph/core/backend.hpp", - "line": 1469, + "line": 1195, "source": "::morph::observe::detail::emitMetric(::morph::observe::Metric::executeInFlight,", "reason": "The executeInFlight emission on the increment side of an execute. Its decrement twin inside the posted task is the same text after stripping, so the ambiguity note on the registerCount entry above applies here too." } @@ -597,7 +597,8 @@ "mutator": "cxx_init_const", "mutation": "bool started = false -> true", "result": "suite unchanged (21985 passing)", - "verdict": "genuinely equivalent -- the initialiser is dead, overwritten by `started = backend->attachModelAsync(...)` on the next statement" + "verdict": "genuinely equivalent -- the initialiser is dead, overwritten by `started = backend->attachModelAsync(...)` on the next statement", + "note": "Recorded 2026-09-09 and kept verbatim. morph#571 removed that verb and the `bool started` initialiser with it, so neither the line nor the statement exists in the tree any more. This is a record of what a sampling run measured on that revision, not a description of today's code; rewriting it to match would destroy the measurement." } ], "important": [ From a7f8aba920a3d693bd7841776630263af1f94379 Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Mon, 21 Sep 2026 03:05:57 +0200 Subject: [PATCH 4/5] tests: settle the self-firing double's promotion without letting an exception out of its destructor `~SelfFiringAssignPrimaryBackend` resolves its one still-pending `ModelCompletion` as a last act of teardown, which is the only way to reach `assignHandlerPrimary`'s `!pinned` arm. Settling is not a non-throwing operation, and a destructor is implicitly `noexcept`, so an escape there is `std::terminate` with no attribution rather than a failed assertion. Reproduced, not inferred -- clang-tidy 22.1.8 (CI's pinned major) driven by `clang-tidy-diff.py` with this job's own configure and flags, on the branch before this commit: tests/test_async_registration.cpp:582:5: error: an exception may be thrown in function '~SelfFiringAssignPrimaryBackend' which should not throw exceptions [bugprone-exception-escape,-warnings-as-errors] note: frame #0: unhandled exception of type 'bad_weak_ptr' may be thrown in function '__throw_bad_weak_ptr' here note: frame #5: function 'setValue' calls function 'shared_from_this' here include/morph/core/completion.hpp:108 note: frame #6: function 'resolve' calls function 'setValue' here note: frame #7: function '~SelfFiringAssignPrimaryBackend' calls function 'resolve' here The named `bad_weak_ptr` is a static over-approximation: the `Promise` holds a `shared_ptr` to the state, so `shared_from_this()` cannot fail here. The destructor's obligation is not, because `setValue` also runs the continuation `Bridge::assignHandlerPrimary` attached, inline, on this thread -- that is ordinary caller code and nothing makes it non-throwing. So the body is wrapped in `try`/`catch (...)`, and the catch *records* into an `std::exception_ptr` the test owns and checks after the backend is gone. A `NOLINT` would have kept the terminate; a bare `catch (...) {}` would have turned it into silence. The new `CHECK` is what keeps the arm from being a swallow. Measured after: the same clang-tidy run reports nothing in this file's changed lines, and re-introducing the unguarded destructor makes it report the finding again at the moved line (609). `morph_tests`: 22932 assertions in 1556 cases, 1 failed as expected -- the branch's baseline plus this commit's one `CHECK`. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01VptDWG2fKr2vBnLSJcgzgW --- tests/test_async_registration.cpp | 54 +++++++++++++++++++++++++++---- 1 file changed, 48 insertions(+), 6 deletions(-) diff --git a/tests/test_async_registration.cpp b/tests/test_async_registration.cpp index d81917907..ed1ddffc2 100644 --- a/tests/test_async_registration.cpp +++ b/tests/test_async_registration.cpp @@ -24,6 +24,7 @@ #include #include #include +#include #include #include #include @@ -572,18 +573,48 @@ class ThrowingSyncRegisterSharedBackend : public morph::backend::detail::IBacken // in this file self-fires this way. class SelfFiringAssignPrimaryBackend : public morph::backend::LocalBackend { public: - explicit SelfFiringAssignPrimaryBackend(morph::exec::IExecutor& pool) : LocalBackend{pool} {} + /// @p escapeSink must outlive the Bridge that owns this backend: the + /// destructor writes to it, and the destructor runs from inside + /// `switchBackend()`/`~Bridge`. + SelfFiringAssignPrimaryBackend(morph::exec::IExecutor& pool, std::exception_ptr& escapeSink) + : LocalBackend{pool}, _escapeSink{&escapeSink} {} SelfFiringAssignPrimaryBackend(const SelfFiringAssignPrimaryBackend&) = delete; SelfFiringAssignPrimaryBackend& operator=(const SelfFiringAssignPrimaryBackend&) = delete; SelfFiringAssignPrimaryBackend(SelfFiringAssignPrimaryBackend&&) = delete; SelfFiringAssignPrimaryBackend& operator=(SelfFiringAssignPrimaryBackend&&) = delete; + // Settling a Completion is not a non-throwing operation, and a destructor + // is implicitly noexcept -- so an escape here is std::terminate, taking + // the whole test binary down with no attribution, rather than a failed + // assertion. That is not hypothetical reasoning about + // `bugprone-exception-escape`; the check names the concrete path, and it + // is inside the framework rather than inside this double: + // + // completion.hpp:108 CompletionState::setValue takes + // `this->shared_from_this()` to hand the saved + // continuations to the executor, and + // `shared_from_this()` throws std::bad_weak_ptr + // when the control block is already gone. + // + // Whether that particular throw is reachable is a separate question -- + // the Promise holds a shared_ptr to the state, so in this test it is not. + // The destructor still must not let *anything* out, including whatever a + // continuation attached by `Bridge::assignHandlerPrimary` throws, which is + // ordinary caller code that setValue runs inline on this thread. + // + // So: catch, and record rather than swallow. The test reads the sink after + // the backend is gone, which turns "terminate, no message" into a failed + // CHECK naming the exception. A NOLINT would have turned it into silence. ~SelfFiringAssignPrimaryBackend() override { - if (_pending) { - auto pending = std::move(*_pending); - _pending.reset(); - pending.promise->resolve(pending.mid); + try { + if (_pending) { + auto pending = std::move(*_pending); + _pending.reset(); + pending.promise->resolve(pending.mid); + } + } catch (...) { + *_escapeSink = std::current_exception(); } } @@ -601,6 +632,7 @@ class SelfFiringAssignPrimaryBackend : public morph::backend::LocalBackend { std::shared_ptr promise; }; std::optional _pending; + std::exception_ptr* _escapeSink; }; } // namespace @@ -1256,7 +1288,11 @@ TEST_CASE( // ever arrive while the backend that produced it is still alive to // deliver it. morph::exec::ThreadPoolExecutor pool{2}; - morph::bridge::Bridge bridge{std::make_unique(pool)}; + // Declared before `bridge`, so it outlives the backend whose destructor + // writes to it -- see SelfFiringAssignPrimaryBackend's own comment on why + // that destructor cannot be allowed to let an exception out. + std::exception_ptr escapedFromTeardown; + morph::bridge::Bridge bridge{std::make_unique(pool, escapedFromTeardown)}; SyncExec cbExec; morph::bridge::BridgeHandler handler{bridge, &cbExec}; @@ -1276,6 +1312,12 @@ TEST_CASE( // The self-fired reply must be discarded exactly like any other stale // reply -- not promote the binding using a backend that no longer exists. CHECK_FALSE(handler.primary().has_value()); + + // ...and settling it must not have thrown. Without this the destructor's + // catch would be a swallow: the arm exists so an escape is a named test + // failure instead of a std::terminate, and nothing proves it stays quiet + // unless the test looks. + CHECK(escapedFromTeardown == nullptr); } TEST_CASE("Bridge::assignHandlerPrimary: an async reply for an already-promoted binding does not overwrite it", From 3c6dd24f7952363f8fc184f5843792a86a9cc002 Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Mon, 21 Sep 2026 03:06:09 +0200 Subject: [PATCH 5/5] ci: stop clang-tidy-diff analysing changed sources this configure never builds Third instance of one structural problem, after #624 (a build-time generated header) and #650 (tests/lint/ text fixtures): `clang-tidy-diff.py` analyses every changed C/C++ line, a changed *comment* line is a changed line, and clang tooling does not skip a file it has no compile command for -- it interpolates a neighbouring entry's command. What comes back is a `clang-diagnostic-error` about this job's configure, not a finding about the diff, and `WarningsAsErrors: "*"` makes it a failed job. Reproduced on this branch with the `clang-tidy` job's own configure flags and clang-tidy 22.1.8: **21 `clang-diagnostic-error`s, all from five changed sources absent from `compile_commands.json`**, none of them naming a changed line's defect: examples/bookmarks/gui_wasm/main_wasm.cpp:58:10: error: 'QGuiApplication' file not found examples/common/wasm_spike/main_wasm.cpp:47:29: error: expected ')' examples/pastebin/gui_wasm/main_wasm.cpp:50:10: error: 'QGuiApplication' file not found examples/polls/gui_wasm/main_wasm.cpp:178:10: error: 'emscripten/emscripten.h' file not found include/morph/core/bridge.hpp:1610:43: error: no matching member function for call to 'into' include/morph/core/bridge.hpp:2667:37: error: no matching member function for call to 'executeVia' (+ 15 more in registry.hpp/model.hpp, all attributed upward from the same TU) The four WASM mains need an Emscripten/Qt-WASM toolchain this job does not configure. The `bridge.hpp`/`registry.hpp`/`model.hpp` errors are not those headers': the note chain resolves every one to `tests/compile_checks/client_only_facade_no_model_header.cpp`, which is built by a configure-time `try_run()` with `-DMORPH_CLIENT_ONLY` and *deliberately* omits `model.hpp`, so its model type is incomplete by design. Reverting the comment edits is not a general answer, and here was not even a local one. Measured: the four WASM comments cite a `docs/spec/core/backend.md` section that the same branch renames, so leaving them at their old text turns the spec-citation gate red instead -- ::error file=examples/bookmarks/gui_wasm/main_wasm.cpp,line=88::dangling section citation: docs/spec/core/backend.md has no section "Asynchronous registration" ::error file=examples/pastebin/gui_wasm/main_wasm.cpp,line=78::dangling section citation: ... ::error file=examples/polls/gui_wasm/main_wasm.cpp,line=254::dangling section citation: ... (x2) So the diff is filtered before `clang-tidy-diff.py` sees it. A changed *source* with no entry in `compile_commands.json` is dropped and named in the log with a `::warning::`; a changed *header* is never dropped, because a header is never a translation unit and discarding headers is what `-only-check-in-db` does -- #479's own defect one directory over. Sections are kept or dropped whole, never hunk by hunk: `clang-tidy-diff.py` attributes `@@` lines to the last `+++` it saw. The filter is only as honest as the database it consults, so it refuses to run unless that database is still the wide one the Configure step builds: at least 600 in-workspace sources and at least 200 under `examples/`. Counting only paths that resolve *inside* the workspace means a resolution mismatch trips the floor too, rather than silently skipping everything. Measured here: 703 entries naming 695 distinct in-workspace sources, 270 under `examples/`; CI measured 690/276 at #481's revision. The regression the floors exist to catch -- `MORPH_BUILD_LADDER` back to its OFF default -- takes `examples/` to 16. Verified, not asserted: * with the filter, `clang-tidy-diff.py` over this branch's diff exits 0 and reports nothing; 26 of 31 changed file sections analysed, 5 skipped and each named; * the filtered gate can still fail: re-introducing the unguarded `~SelfFiringAssignPrimaryBackend` makes it exit 1 with `bugprone-exception-escape` on the changed line; * the Python embedded in the workflow was extracted back out of the parsed YAML and produces a byte-identical filtered diff to the version tested standalone; * `check_workflow_job_banners.py`, `check_ci_clang_pin.sh` and `check_workflow_option_coverage.py` all pass on the edited file. Not verified: no Emscripten toolchain was available, so nothing was built for the four WASM mains -- the claim is only that this job cannot analyse them, which is what their absence from `compile_commands.json` shows. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01VptDWG2fKr2vBnLSJcgzgW --- .github/workflows/ci.yml | 138 ++++++++++++++++++++++++++++++++++++++- 1 file changed, 136 insertions(+), 2 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 7390f4c90..2d1cd4985 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -2458,8 +2458,141 @@ jobs: # raised is the file not being there, not the directory. That is what # the AUTOMOC step above generates, and why this job now builds two # targets (morph#624). - if ! git diff -U0 "$BASE_SHA" | \ - python3 "$CLANG_TIDY_DIFF" \ + # And it is not sufficient for a source this configure does not + # build at all. That is the third instance of one structural + # problem, after morph#624 (a generated header) and morph#650 + # (tests/lint/ text fixtures): clang-tidy-diff.py analyses every + # changed C/C++ line, a changed *comment* line is a changed line, + # and clang tooling does not skip a file it has no compile command + # for -- it interpolates a neighbouring entry's command. What comes + # back is a clang-diagnostic-error about this job's configure, not a + # lint finding about the diff, and WarningsAsErrors:"*" makes it a + # failed job. Measured on morph#649's branch, which edits five such + # sources in comments only: 21 clang-diagnostic-errors from the four + # WASM mains (no Emscripten/Qt-WASM toolchain here) and from + # tests/compile_checks/client_only_facade_no_model_header.cpp (built + # by a configure-time try_run() with -DMORPH_CLIENT_ONLY, and + # deliberately incomplete without it). + # + # Reverting the comment edits is not a general answer and in that + # case was not even a local one: those four comments cite a + # docs/spec/core/backend.md section the same branch renames, so + # leaving them alone turns the spec-citation gate red instead + # (reproduced -- four "dangling section citation" errors). + # + # So the diff is filtered first: a changed *source* with no entry in + # compile_commands.json is dropped, and every drop is printed. A + # changed *header* is never dropped -- a header is never a + # translation unit, so -only-check-in-db's literal membership test + # would silently discard all of include/morph/**, which is + # morph#479's own defect one directory over (see above). The filter + # refuses to run at all unless the database is still the wide one + # the Configure step builds, because a filter consulting a narrowed + # database would quietly stop analysing everything the narrowing + # dropped. + cat > /tmp/filter-unbuilt-sources.py <<'PY' + import json + import pathlib + import re + import sys + + # Only *sources* are filtered out. A header is never a translation + # unit, so it is never in compile_commands.json. + SOURCE_SUFFIXES = {".c", ".cc", ".cpp", ".cxx", ".c++", ".cl", ".m", ".mm"} + + # Floors, not equalities. This filter is only trustworthy while the + # database it consults is the wide one the Configure step builds: if + # that configure regresses -- MORPH_BUILD_LADDER back to its OFF + # default is the concrete way, morph#481 -- every examples/ source + # would silently become "not built by this configure" and stop being + # analysed, and this gate would go green having read 40% less code. + # The floor is what makes that loud. Counted over entries whose file + # resolves *inside the workspace*, so a resolution mismatch (a + # symlinked checkout, a path this script fails to normalise the same + # way) trips it too instead of silently skipping everything. + # + # Measured with this job's own configure flags and clang 22: 703 + # entries naming 695 distinct in-workspace sources, 270 of them under + # examples/. CI measured 690/276 at morph#481's revision. Both floors + # sit under both pairs with room for ordinary churn, and the + # regression they exist to catch takes examples/ to 16, not to 199. + MIN_ENTRIES = 600 + MIN_EXAMPLES_ENTRIES = 200 + + root = pathlib.Path.cwd().resolve() + database = json.loads(pathlib.Path(sys.argv[1]).read_text()) + compiled = {pathlib.Path(entry["file"]).resolve() for entry in database} + inside = {f for f in compiled if f.is_relative_to(root)} + examples = {f for f in inside if f.is_relative_to(root / "examples")} + + if len(inside) < MIN_ENTRIES or len(examples) < MIN_EXAMPLES_ENTRIES: + print( + f"::error::compile_commands.json names {len(inside)} " + f"in-workspace source(s) ({len(examples)} under examples/), " + f"below the floors {MIN_ENTRIES}/{MIN_EXAMPLES_ENTRIES}. This " + f"configure has stopped covering what the changed-source " + f"filter assumes it covers -- fix the configure, do not lower " + f"the floor." + ) + sys.exit(1) + + sections = [] + current = None + for line in pathlib.Path(sys.argv[2]).read_text(errors="replace").splitlines(keepends=True): + if line.startswith("diff --git "): + current = [] + sections.append(current) + if current is None: + continue + current.append(line) + + kept = [] + skipped = [] + for section in sections: + name = None + for line in section: + match = re.match(r'^\+\+\+ (?:"?b/)?([^\t\n"]*)', line) + if match: + name = match.group(1) + break + # A whole section is kept or dropped together, never just its + # hunks: clang-tidy-diff.py attributes `@@` lines to the last + # `+++` it saw, so dropping a `+++` while keeping its hunks would + # charge them to the previous file. + # + # No destination path (a pure deletion) leaves nothing to + # analyse, and clang-tidy-diff.py's extension filter drops it. + if name is None or name == "/dev/null": + kept.append(section) + continue + path = root / name + if path.suffix.lower() in SOURCE_SUFFIXES and path.resolve() not in compiled: + skipped.append(name) + continue + kept.append(section) + + pathlib.Path(sys.argv[3]).write_text("".join(line for section in kept for line in section)) + + for name in skipped: + print( + f"::warning file={name}::not analysed by clang-tidy-diff: this " + f"configure builds no translation unit for it, so clang-tidy " + f"would interpolate a neighbouring entry's compile command and " + f"report only clang-diagnostic-errors about that." + ) + print( + f"ok: compile database names {len(inside)} in-workspace source(s) " + f"({len(examples)} under examples/); {len(kept)} of {len(sections)} " + f"changed file section(s) analysed, {len(skipped)} source(s) " + f"skipped as unbuilt here" + ) + PY + + git diff -U0 "$BASE_SHA" > /tmp/changed.diff + python3 /tmp/filter-unbuilt-sources.py \ + build/clang-debug/compile_commands.json /tmp/changed.diff /tmp/analysed.diff + + if ! python3 "$CLANG_TIDY_DIFF" \ -path build/clang-debug \ -clang-tidy-binary clang-tidy-${{ env.CLANG_VERSION }} \ -p1 \ @@ -2467,6 +2600,7 @@ jobs: -extra-arg=-std=c++23 \ -extra-arg=-Wno-missing-include-dirs \ -quiet \ + < /tmp/analysed.diff \ 2>&1 | tee clang-tidy-report.txt; then echo "::error::clang-tidy reported findings on changed lines -- see the log above and the clang-tidy-report artifact" exit 1