From 1d90caeda8355bc3ee267b7365c88ca3da8ca9b1 Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Fri, 7 Aug 2026 01:07:28 +0300 Subject: [PATCH 01/20] core: escape control bytes in action and result JSON bodies MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit wire::encode has escaped ASCII control bytes since the fuzz-found envelope bug, but an execute envelope's `body` is not written by encode at all — it is produced separately by ActionTraits::toJson / resultToJson, which wrote with plain glz::write_json and so reproduced the identical gap for every string field of every action and result: invalid JSON that the peer's own reader rejects, and — alongside an escaped character — silent corruption into two 0x00 bytes by glaze's chunked write path. Found from the other end, by the application ladder's rung 1 replaying tests/fuzz/findings/ as paste content. Fixed with the same instrument one layer down: model::detail::EscapingWriteOpts, deliberately duplicating wire::detail::EscapingWriteOpts rather than making the model layer depend on the transport layer's header for a four-line option struct. Regression tests land as "Bug G" in test_wire_hardening.cpp, alongside the envelope-level cases they mirror; all four fail if the write options are reverted. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01HffixfknmXdAjnM5hSGRBb --- docs/spec/core/registry.md | 31 ++++++++++++- docs/spec/testing_strategy.md | 17 ++++++- include/morph/core/registry.hpp | 37 ++++++++++++++- tests/test_wire_hardening.cpp | 79 +++++++++++++++++++++++++++++++++ 4 files changed, 159 insertions(+), 5 deletions(-) diff --git a/docs/spec/core/registry.md b/docs/spec/core/registry.md index f815ba6f..164940a8 100644 --- a/docs/spec/core/registry.md +++ b/docs/spec/core/registry.md @@ -12,6 +12,7 @@ without knowing their concrete types. - [Customisation traits](#customisation-traits) - [ModelTraits](#modeltraits) - [ActionTraits](#actiontraits) + - [Control bytes in action and result bodies](#control-bytes-in-action-and-result-bodies) - [Validation and logging policy](#validation-and-logging-policy) - [ActionValidator](#actionvalidator) - [ValidationError](#validationerror) @@ -173,6 +174,32 @@ struct ActionTraits; // forward — specialize or use BRIDGE_REGISTER_ACTION All four JSON functions throw `detail::ParseError` (a `std::runtime_error` subclass) on glaze encode/decode failure. +#### Control bytes in action and result bodies + +`toJson`/`resultToJson` write with `detail::EscapingWriteOpts`, a `glz::opts` +refinement that turns on glaze's `escape_control_characters` — the same +treatment, and for the same two reasons, that `wire::encode` already applies +to the envelope (see wire.md, "Control bytes in string fields"). With the +option off, an ASCII control byte (U+0000–U+001F) in any caller-supplied +string field of an action or result: + +- **produces invalid output** — RFC 8259 requires those code points to be + escaped, and glaze's own reader enforces it, so the peer's `fromJson` throws + a `ParseError` on a body its own peer just wrote; and +- **can be silently corrupted** — with a `\` or `"` earlier in the same + string, glaze's chunked fast path writes such a byte out as two `0x00` + bytes, and the result still decodes. + +Action bodies are pure caller data (a paste's content, a chat message, a +filename), so this is at least as exposed as the envelope was. Escaping is +lossless in both directions; the read side needs no counterpart, since glaze's +reader already accepts `\uXXXX`. + +`morph::model::detail::EscapingWriteOpts` deliberately duplicates +`morph::wire::detail::EscapingWriteOpts` rather than reusing it: the action +codec belongs to the model layer and must not acquire a dependency on the +transport layer's header to share a four-line option struct. + ## Validation and logging policy ### `ActionValidator` @@ -583,7 +610,9 @@ Expands to: `static constexpr std::string_view typeId()` (no `noexcept`, unlike `ModelTraits::typeId()`), a `static constexpr Loggable loggable`, and four JSON codec functions (each throwing `detail::ParseError` on failure): `toJson`/ - `resultToJson` use `glz::write_json`; `fromJson`/`resultFromJson` use + `resultToJson` use `glz::write` (see + ["Control bytes in action and result bodies"](#control-bytes-in-action-and-result-bodies)); + `fromJson`/`resultFromJson` use `glz::read` — the same forward-compatibility convention `wire::decode` uses (see wire.md, "Action-evolution policy") — so an older-compiled action struct silently diff --git a/docs/spec/testing_strategy.md b/docs/spec/testing_strategy.md index de8a158d..2d7c7fd7 100644 --- a/docs/spec/testing_strategy.md +++ b/docs/spec/testing_strategy.md @@ -125,9 +125,22 @@ regression cases under `tests/fuzz/findings/`: log-bound text, and a raw `0x1B` in it would carry an ANSI escape into the reader's terminal. + That fix, in turn, covered the *envelope* only. An execute envelope's `body` + is not written by `wire::encode` at all — it is produced separately by + `ActionTraits::toJson` / `resultToJson` (registry.hpp's + `BRIDGE_REGISTER_ACTION` macro), which wrote with plain `glz::write_json` and + so reproduced the identical gap for every string field of every action and + result. Action bodies are pure caller data (a paste's content, a chat + message, a filename), so this is at least as exposed as the envelope was. + Found from the other end — by the application ladder's rung 1 (pastebin) + replaying `tests/fuzz/findings/` as *paste content*, which is the round trip + its README's "hostile content" requirement asks for — and fixed with the same + instrument one layer down, `model::detail::EscapingWriteOpts` (see + docs/spec/core/registry.md, "Control bytes in action and result bodies"). + These fixes are covered by dedicated regression tests in -`tests/test_wire_hardening.cpp` ("Bug C"/"Bug D"/"Bug E"/"Bug F") in addition to -the `fuzz_*_replay` findings above. +`tests/test_wire_hardening.cpp` ("Bug C"/"Bug D"/"Bug E"/"Bug F"/"Bug G") in +addition to the `fuzz_*_replay` findings above. ## Soak tests (`tests/soak/`) diff --git a/include/morph/core/registry.hpp b/include/morph/core/registry.hpp index 51ea3be0..85dec65c 100644 --- a/include/morph/core/registry.hpp +++ b/include/morph/core/registry.hpp @@ -191,6 +191,32 @@ struct ParseError : std::runtime_error { using std::runtime_error::runtime_error; }; +/// @brief Write options for an action's/result's own JSON body: identical to +/// `morph::wire::detail::EscapingWriteOpts`, applied one layer down. +/// +/// The envelope codec already escapes ASCII control bytes (see +/// `docs/spec/core/wire.md`, "Control bytes in string fields"); the action and +/// result bodies it carries are serialized here, separately, and need exactly +/// the same treatment for exactly the same two reasons. With the option off, +/// a raw `0x00`–`0x1F` in any caller-supplied string field of an action makes +/// the body invalid JSON that the peer's own reader rejects — and, when the +/// same string also contains an escaped character, glaze's chunked fast path +/// silently rewrites such a byte as two `0x00`s, destroying the payload in a +/// way that still decodes. Action bodies are pure caller data (a paste's +/// content, a message, a filename), so this is if anything more exposed than +/// the envelope was. +/// +/// Deliberately duplicated rather than reused from `morph::wire`: the action +/// codec belongs to the model layer and must not acquire a dependency on the +/// transport layer's header just to share a four-line option struct. +/// +/// Applies to writing only — glaze's reader already accepts `\\uXXXX`. +struct EscapingWriteOpts : glz::opts { + /// @brief Emit control bytes as `\\uXXXX` rather than raw. + // NOLINTNEXTLINE(readability-identifier-naming) — the name is glaze's, not ours; the option is matched by name. + bool escape_control_characters = true; +}; + // Forward declarations so instance() methods inside the classes can reference them. inline class ActionDispatcher& defaultDispatcher(); inline class ModelRegistryFactory& defaultRegistry(); @@ -698,7 +724,11 @@ bool registerActionExecutorOnce(std::string_view modelId, std::string_view actio static constexpr ::morph::model::Loggable loggable = (LOGGABLE); \ static std::string toJson(const A& action) { \ std::string out; \ - if (auto errCode = glz::write_json(action, out)) { \ + /* EscapingWriteOpts, not write_json: a raw control byte in any */ \ + /* caller-supplied string field would otherwise produce a body the */ \ + /* peer's reader rejects, or be silently mangled by glaze's chunked */ \ + /* fast path — see its doc comment in registry.hpp. */ \ + if (auto errCode = glz::write<::morph::model::detail::EscapingWriteOpts{}>(action, out)) { \ throw morph::model::detail::ParseError{glz::format_error(errCode, out)}; \ } \ return out; \ @@ -716,7 +746,10 @@ bool registerActionExecutorOnce(std::string_view modelId, std::string_view actio } \ static std::string resultToJson(const Result& result) { \ std::string out; \ - if (auto errCode = glz::write_json(result, out)) { \ + /* EscapingWriteOpts: see toJson() above — a result body carries */ \ + /* caller data back (a paste's content, a fetched record) and needs */ \ + /* the identical treatment. */ \ + if (auto errCode = glz::write<::morph::model::detail::EscapingWriteOpts{}>(result, out)) { \ throw morph::model::detail::ParseError{glz::format_error(errCode, out)}; \ } \ return out; \ diff --git a/tests/test_wire_hardening.cpp b/tests/test_wire_hardening.cpp index 41c45491..5f6b6f37 100644 --- a/tests/test_wire_hardening.cpp +++ b/tests/test_wire_hardening.cpp @@ -35,6 +35,17 @@ // for a different reason now (see Bug E): an err message is log-bound text, // and a raw 0x1B in it would carry an ANSI escape into the reader's terminal. // +// Bug G (control bytes in the action/result codec): the Bug E gap, one layer +// down. An execute envelope's `body` is not written by `wire::encode` at all +// — it is produced by `ActionTraits::toJson` / `resultToJson` +// (registry.hpp's BRIDGE_REGISTER_ACTION macro), which wrote with plain +// `glz::write_json` and so reproduced Bug E exactly for every string field +// of every action and result. Action bodies are pure caller data (a paste's +// content, a chat message, a filename), so this is at least as exposed as +// the envelope was. Found by pastebin (ladder rung 1) replaying +// tests/fuzz/findings/ as paste content; fixed with the same instrument, +// `model::detail::EscapingWriteOpts`. +// // Bug E (control bytes in the remaining string fields): the same writer gap // applies to every `Envelope` string, not just `message` — `body`, // `modelType`, `actionType`, `contextKey`, `typeId`, and the session's @@ -50,6 +61,7 @@ #include #include #include +#include #include #include #include @@ -289,3 +301,70 @@ TEST_CASE("wire::detail::peekCallId cannot be spoofed from an earlier string fie env.callId = 5U; CHECK(morph::wire::detail::peekCallId(encode(env)) == 5U); } + +// ── Bug G: the same writer gap, one layer down, in the action/result codec ─── +// +// `wire::encode` escapes control bytes in the *envelope*, but an execute +// envelope's `body` is produced separately, by `ActionTraits::toJson` / +// `resultToJson` (registry.hpp's BRIDGE_REGISTER_ACTION macro). Those wrote +// with plain `glz::write_json` and so reproduced Bug E exactly: an action +// carrying a raw control byte in any of its string fields serialized to a body +// its own peer's `fromJson` then rejected — and, alongside an escaped +// character, was silently rewritten into two 0x00 bytes. Action bodies are +// pure caller data (a paste's content, a chat message, a filename), so this is +// at least as exposed as the envelope was. Found by pastebin (ladder rung 1) +// replaying tests/fuzz/findings/ as paste content; fixed with the same +// instrument, `model::detail::EscapingWriteOpts`. See docs/spec/core/registry.md, +// "Control bytes in action and result bodies". + +// Namespace scope, not anonymous: glaze's reflection needs external linkage on +// the reflected type (see test_backend_rig.cpp's identical note). +struct WireCtlAction { + std::string text; +}; +struct WireCtlResult { + std::string text; +}; +struct WireCtlModel { + WireCtlResult execute(WireCtlAction action) { return WireCtlResult{.text = action.text}; } +}; + +BRIDGE_REGISTER_MODEL(WireCtlModel, "WireCtlModel") +BRIDGE_REGISTER_ACTION(WireCtlModel, WireCtlAction, "WireCtlAction") + +TEST_CASE("ActionTraits::toJson escapes control bytes so the action body re-decodes", "[wire][hardening]") { + const std::string payload = ctl(); + const auto json = morph::model::ActionTraits::toJson(WireCtlAction{.text = payload}); + CHECK(morph::model::ActionTraits::fromJson(json).text == payload); +} + +TEST_CASE("ActionTraits::resultToJson escapes control bytes so the result body re-decodes", + "[wire][hardening]") { + const std::string payload = ctl(); + const auto json = morph::model::ActionTraits::resultToJson(WireCtlResult{.text = payload}); + CHECK(morph::model::ActionTraits::resultFromJson(json).text == payload); +} + +TEST_CASE("ActionTraits preserves the whole control range byte-for-byte", "[wire][hardening]") { + std::string all; + for (int byte = 0x00; byte < 0x20; ++byte) { + all.push_back(static_cast(byte)); + } + const auto json = morph::model::ActionTraits::toJson(WireCtlAction{.text = all}); + const auto back = morph::model::ActionTraits::fromJson(json); + CHECK(back.text == all); + CHECK(back.text.size() == 32U); +} + +TEST_CASE("ActionTraits survives a control byte alongside an escaped character", "[wire][hardening]") { + // The corrupting half of the failure mode, not merely the invalid-output + // half — see the identical sweep for `encode` above. The value comparison, + // not the absence of a throw, is the assertion that matters. + for (std::size_t pad = 0; pad <= 40; ++pad) { + std::string payload = "\\" + std::string(pad, 'x'); + payload.push_back(static_cast(0x0B)); + payload += "\"tail"; + const auto json = morph::model::ActionTraits::toJson(WireCtlAction{.text = payload}); + CHECK(morph::model::ActionTraits::fromJson(json).text == payload); + } +} From f60c83cec6372c2c77d4ce086fd05974f271b7c1 Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Sat, 8 Aug 2026 00:58:29 +0300 Subject: [PATCH 02/20] core: add a client-side execute deadline (Bridge::setExecuteDeadline) Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01HffixfknmXdAjnM5hSGRBb --- docs/spec/core/backend.md | 18 +- docs/spec/core/bridge.md | 17 +- docs/spec/core/completion.md | 73 +++++++ include/morph/core/backend.hpp | 16 ++ include/morph/core/bridge.hpp | 90 ++++++++- include/morph/core/remote.hpp | 116 +---------- include/morph/core/timeout_scheduler.hpp | 130 ++++++++++++ tests/CMakeLists.txt | 1 + tests/test_client_execute_deadline.cpp | 241 +++++++++++++++++++++++ 9 files changed, 582 insertions(+), 120 deletions(-) create mode 100644 include/morph/core/timeout_scheduler.hpp create mode 100644 tests/test_client_execute_deadline.cpp diff --git a/docs/spec/core/backend.md b/docs/spec/core/backend.md index 614f977c..4aeee4d7 100644 --- a/docs/spec/core/backend.md +++ b/docs/spec/core/backend.md @@ -211,7 +211,9 @@ would. See `QtWebSocketBackend`'s own section below. ## Error types -Four exception types are thrown into in-flight `Completion`s: +Five exception types are thrown into in-flight `Completion`s. The first four are +raised by a backend; `ClientTimeoutError` is raised by `Bridge` itself, but is +declared alongside them so callers catch every dispatch failure from one header: | Type | Trigger | Purpose | |---|---|---| @@ -219,6 +221,7 @@ Four exception types are thrown into in-flight `Completion`s: | `BridgeDestroyedError` | `Bridge` is destroyed | In-flight completions are cancelled because the bridge is gone. | | `DisconnectedError` | Transport drops mid-call (e.g. WebSocket disconnect) | Framework retries the call on reconnect if the backend supports it; otherwise the GUI's `.onError(...)` runs. | | `TimeoutError` | Server-side `LimitPolicy::executeTimeout` elapses | Distinguishes a bounded-wait timeout from any other `err` reply, so callers can retry or surface a specific "request timed out" message. | +| `ClientTimeoutError` | Client-side `Bridge::setExecuteDeadline` elapses with *no* reply of any kind | Bounds the caller's wait when nothing comes back at all (a dropped frame, a hung server). Unlike `TimeoutError` it carries no evidence the server ever saw the request — see [`completion.md`](completion.md), "Client-side execute deadline". | ## `LocalBackend` — in-process execution @@ -430,11 +433,15 @@ A server-side execute timeout surfaces to a caller as `morph::backend::TimeoutEr than a generic `std::runtime_error`, on both `SimulatedRemoteBackend` and `QtWebSocketBackend`. -The background timer that enforces `executeTimeout` is `detail::TimeoutScheduler` — -a single dedicated thread per `RemoteServer` (mirroring `NetworkMonitor`'s +The background timer that enforces `executeTimeout` is +`morph::async::detail::TimeoutScheduler` (`include/morph/core/timeout_scheduler.hpp`) +— a single dedicated thread per `RemoteServer` (mirroring `NetworkMonitor`'s condition-variable wait loop), lazily started by `setLimitPolicy` the first time `executeTimeout` is configured, so a server that never uses the feature pays no -extra thread. +extra thread. The class lives in `morph::async::detail` rather than +`morph::backend::detail` because `Bridge` uses the same primitive for the +*client*-side `setExecuteDeadline` — see [`completion.md`](completion.md), +"Client-side execute deadline". ### Connection scopes @@ -1202,6 +1209,7 @@ thread to marshal onto. | `BridgeDestroyedError` | `std::runtime_error` | `"bridge destroyed before completion resolved"` | | `DisconnectedError` | `std::runtime_error` | `"transport disconnected before completion resolved"` | | `TimeoutError` | `std::runtime_error` | `"execute timed out on the server"` | +| `ClientTimeoutError` | `std::runtime_error` | `"execute timed out waiting for any reply"` | ### `LocalBackend` @@ -1361,7 +1369,7 @@ not a behavior change to the existing loopback-only default. | Reconnect handler skipped on first connect | Fired only when `_everConnected` was already true | The initial handler registration is driven by `BridgeHandler` constructors; firing the reconnect handler on the very first connect would double-register. | | No reconnect for never-connected sockets | `disconnected` schedules a retry only if `_everConnected` | A socket that never reached the server (bad URL / refused) fails fast via `waitForConnected` returning false, rather than backing off forever. | | Server reply marshalled to the Qt thread | `QMetaObject::invokeMethod(..., QueuedConnection)` with a `QPointer` | `RemoteServer::handle` produces the reply on a pool thread, but `QWebSocket::sendTextMessage` must run on the Qt thread; the weak `QPointer` drops the reply cleanly if the client disconnected meanwhile. | -| `executeTimeout` implementation | A dedicated, lazily-started background thread (`detail::TimeoutScheduler`) per `RemoteServer`, not a per-call thread | `IExecutor` has no delayed-post primitive and `RemoteServer` is transport-agnostic (cannot assume Qt's `QTimer`). One thread amortizes across every timed call; it is only started the first time `executeTimeout` is actually configured, so a server that never uses the feature pays no cost. | +| `executeTimeout` implementation | A dedicated, lazily-started background thread (`morph::async::detail::TimeoutScheduler`) per `RemoteServer`, not a per-call thread | `IExecutor` has no delayed-post primitive and `RemoteServer` is transport-agnostic (cannot assume Qt's `QTimer`). One thread amortizes across every timed call; it is only started the first time `executeTimeout` is actually configured, so a server that never uses the feature pays no cost. | | `messagesPerSecond` algorithm | Per-connection token bucket, capacity = rate, continuous refill, drop (not close) on empty | Simplest correct rate limiter; allows a legitimate one-second burst without penalizing an otherwise well-behaved client. Dropping (vs. closing) keeps a transient burst from taking down the connection — pair with `LimitPolicy::executeTimeout` if bounded caller-side waiting is also needed. | | Graceful shutdown drains via a shared in-flight counter, not a new `IExecutor::waitIdle` | `RemoteServer` counts its own accepted-but-unreplied executes rather than adding a general drain API to `IExecutor`/`StrandExecutor` | The drain condition morph can define precisely — "every accepted execute has replied" — lives at the server layer, where the work is counted; executor.md's "no graceful drain / `waitIdle`" limitation is deliberately left as-is for raw executor users. | | Backend-change-awareness captured at registration | `IModelHolder::isBackendChangeAware()` (compile-time answer per model type) + `LocalBackend::_changeAware`, maintained by `registerModel`/`deregisterModel` | Replaces a per-`notifyBackendChanged`-call `dynamic_cast` sweep over every live model with a virtual query done once at registration, and a lookup restricted to the models that actually opted in. No RTTI dependency; cost is O(change-aware models) instead of O(all models) under `_regMtx`. No change to the model-facing contract (`IBackendChangedSink`, `BackendChangedMixin`) or to when/where `onBackendChanged()` runs. | diff --git a/docs/spec/core/bridge.md b/docs/spec/core/bridge.md index ba312e44..f0363166 100644 --- a/docs/spec/core/bridge.md +++ b/docs/spec/core/bridge.md @@ -258,6 +258,19 @@ envelopes. The constructor does the same with the (typically empty) initial session. See [session.md](../session/session.md#how-a-context-originates-and-flows) and [backend.md](backend.md#session-propagation-to-control-envelopes). +**`setExecuteDeadline(deadline)`** / **`executeDeadline()`** installs an +opt-in, client-side wall-clock bound on how long any subsequent `executeVia()` +waits for a reply. Defaults to `std::chrono::milliseconds{0}` (disabled — the +pre-existing behavior, and no extra thread). When enabled, each dispatch races +the real reply against a `morph::async::detail::TimeoutScheduler` timer that +resolves the pending `Completion` with `morph::backend::ClientTimeoutError`; +whichever settles first wins, and the loser is discarded by +`CompletionState`'s first-result-wins rule. The on-time reply path disarms the +timer as the first statement of its completion callback. Thread-safe, its own +mutex (`_executeDeadlineMtx`). Full semantics — including how +`ClientTimeoutError` differs from the server-reported `TimeoutError` — in +[completion.md](completion.md#client-side-execute-deadline). + **`setPrincipal(principal)`** / **`currentPrincipal()`** installs and reads back a `morph::session::Principal` — the verified identity + roles, readable *outside* a dispatch (unlike `session::current()`, which only exists during @@ -576,9 +589,11 @@ make teardown order-independent.) | `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. Atomic: stages all re-registrations on the new backend, commits (publishes new ids + swaps) only if all succeed, else rolls back and rethrows leaving old backend + `currentId`s intact. Cancels old backend's pending ops with `BackendChangedError`. Holds both `_mtx` and `_attachMtx` for its duration. 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. | -| `executeVia` | `Completion executeVia(const shared_ptr&, Action, IExecutor*)` | Lock-free dispatch. Attaches default session. On `LocalBackend`, rejects an action whose `ActionValidator::ready` returns `false` with `morph::model::ValidationError` via `onError`, before `Model::execute` runs. Records a journal `LogEntry` for loggable actions on both success (`Outcome::Succeeded`) and a throwing `Model::execute` (`Outcome::Failed`, rethrown unchanged). Value-forwarding into the typed `Completion` is `try`/`catch`-guarded — a throwing result move/copy resolves the completion via `onError` instead of hanging or terminating. The bridge-touching side effects (`onResult`, `hasSubscribers()`/`publishResult`, and the `pendingCalls()` decrement) are gated on the `_liveness` token, checked before any runs, so a completion resolving after `~Bridge()` skips them instead of touching the dangling `Bridge`. Increments `pendingCalls()` once per call before dispatch (never for the synchronous "handler not bound" early return); decrements it exactly once, from whichever of the two mutually-exclusive resolution continuations actually fires. | +| `executeVia` | `Completion executeVia(const shared_ptr&, Action, IExecutor*)` | Lock-free dispatch. Attaches default session. On `LocalBackend`, rejects an action whose `ActionValidator::ready` returns `false` with `morph::model::ValidationError` via `onError`, before `Model::execute` runs. Records a journal `LogEntry` for loggable actions on both success (`Outcome::Succeeded`) and a throwing `Model::execute` (`Outcome::Failed`, rethrown unchanged). Value-forwarding into the typed `Completion` is `try`/`catch`-guarded — a throwing result move/copy resolves the completion via `onError` instead of hanging or terminating. The bridge-touching side effects (`onResult`, `hasSubscribers()`/`publishResult`, the `pendingCalls()` decrement, and the execute-deadline disarm) are gated on the `_liveness` token, checked before any runs, so a completion resolving after `~Bridge()` skips them instead of touching the dangling `Bridge`. Increments `pendingCalls()` once per call before dispatch (never for the synchronous "handler not bound" early return); decrements it exactly once, from whichever of the two mutually-exclusive resolution continuations actually fires. Arms the client-side execute deadline when one is installed (see `setExecuteDeadline`); the fast-fail "handler not bound" path returns before that and arms nothing. | | `setDefaultSession` | `void setDefaultSession(session::Context)` | Installs default session context; also pushes it to the active backend via `IBackend::setSession` so control envelopes (register/attach/assign/deregister) carry it too, not only `execute`. | | `defaultSession` | `session::Context defaultSession() const` | Returns snapshot of default session. | +| `setExecuteDeadline` | `void setExecuteDeadline(std::chrono::milliseconds)` | Opt-in client-side execute deadline; `0` (the default) disables it. Lazily creates the backing `TimeoutScheduler` thread on first enable. | +| `executeDeadline` | `std::chrono::milliseconds executeDeadline() const` | Returns the installed deadline; `0` when disabled. | | `setPrincipal` | `void setPrincipal(session::Principal)` | Installs the verified `Principal`, readable outside a dispatch. Pass `Principal{}` to clear (sign-out). | | `currentPrincipal` | `session::Principal currentPrincipal() const` | Returns a snapshot of the installed `Principal`; default-constructed if none was ever set. | | `pendingCalls` | `[[nodiscard]] size_t pendingCalls() const noexcept` | Count of `executeVia()` dispatches not yet resolved. Relaxed atomic load; see the `Bridge` section above. | diff --git a/docs/spec/core/completion.md b/docs/spec/core/completion.md index 53540e0f..77fca413 100644 --- a/docs/spec/core/completion.md +++ b/docs/spec/core/completion.md @@ -17,6 +17,7 @@ than vanishing (see [Failure modes](#failure-modes)). - [Move-only handle — `Completion`](#move-only-handle--completiont) - [Thread safety](#thread-safety) - [Failure modes](#failure-modes) +- [Client-side execute deadline](#client-side-execute-deadline) - [Empty state](#empty-state) - [API reference](#api-reference) - [Design decisions](#design-decisions) @@ -220,6 +221,70 @@ throw — they are silent by construction. `value` — since `attachThen`'s fire-now path reads `*value` directly, not from the (already-emptied) handler vector. +## Client-side execute deadline + +Nothing in `Completion` itself imposes a time limit: a state that no producer +ever settles simply stays pending forever, and its handle's callbacks never +fire. For an in-process `LocalBackend` that is unreachable, but across a wire a +request can genuinely disappear — a frame silently discarded by +`QtWebSocketServerConfig::messagesPerSecond`'s rate limiter, a connection that +dropped between send and reply, or a server that hangs. In every one of those +cases *no reply of any kind* comes back, so no layer below the caller has +anything to resolve the `Completion` with. + +`Bridge::setExecuteDeadline(std::chrono::milliseconds)` closes that hole. + +**Opt-in, default disabled.** The deadline defaults to +`std::chrono::milliseconds{0}`, which means "no deadline" and reproduces the +pre-existing behavior exactly — a `Bridge` that never calls the setter behaves +as it always did, and spawns no extra thread. The current value is readable via +`Bridge::executeDeadline()`. + +**Mechanics.** Every `executeVia()` call made while a non-zero deadline is +installed arms a timer on a `Bridge`-owned +`morph::async::detail::TimeoutScheduler` (a single background thread, created +lazily on the first call that enables a deadline and torn down with the +`Bridge`; the same class `RemoteServer` uses for its server-side +`LimitPolicy::executeTimeout`). The timer's callback captures only the typed +`CompletionState` — never the `Bridge` — and resolves it with +`morph::backend::ClientTimeoutError`. The real reply and the timer therefore +race, and **whichever settles the state first wins**, because `setValue` / +`setException` are no-ops once the state is `ready` (see +[Failure modes](#failure-modes) and the *first-result-wins* row in +[Design decisions](#design-decisions)). A real reply that arrives after the +deadline already fired is silently discarded — it is an ordinary late write to +an already-resolved state, not an error condition. Conversely, a reply that +arrives first disarms the timer as the *first* statement of the completion +callback, before any `onResult` / `publishResult` fan-out work, so a slow +subscriber cannot open a window for the timer to fire against a result already +in hand. + +The deadline is armed only for real dispatches. `executeVia()`'s fast-fail path +for an unbound handler resolves its `Completion` synchronously before the timer +block is reached, so no timer is created for it. + +The disarm is guarded on the same `Bridge` liveness token the rest of the +completion callback uses: the callback can in principle run after `~Bridge()` +(the backend may be co-owned and outlive the `Bridge`). Skipping the disarm in +that case is harmless — `~TimeoutScheduler` drops still-pending entries without +firing them. + +**`ClientTimeoutError` vs. `TimeoutError`.** Both live in `morph::backend` and +both derive from `std::runtime_error`, but they report different facts: + +| Type | Raised by | Means | +|---|---|---| +| `TimeoutError` | The **server**, as an explicit `err "timeout"` reply when `LimitPolicy::executeTimeout` elapses | The request *was* received and the action *is* running (morph never interrupts an in-flight `Model::execute`); the server chose to stop making the caller wait. | +| `ClientTimeoutError` | The **client**, when `Bridge::setExecuteDeadline`'s duration elapses | Nothing came back at all. Whether the server ever received the request, is still processing it, or replied over a connection that had already dropped is **unknown**. | + +The practical consequence for callers: `TimeoutError` confirms the action is +in flight server-side, so a blind retry risks a duplicate. `ClientTimeoutError` +confirms nothing, so a retry must be idempotent (or reconciled) either way. + +A deadline bounds the *caller's wait*, never the work. It does not cancel the +request — see [Limitations](#limitations), "No cancellation". The server-side +counterpart is documented in [`backend.md`](backend.md) under `LimitPolicy`. + ## Empty state A default-constructed `Completion` has a null `_state` pointer. `then()` and @@ -283,6 +348,10 @@ future/promise or a monadic async type. Its scope is narrow by design: promise/awaiter machinery. Consumption is callback-only. - **No cancellation.** There is no handle to cancel an outstanding operation; once started, it runs to completion (or is abandoned). + `Bridge::setExecuteDeadline` (see + [Client-side execute deadline](#client-side-execute-deadline)) is not an + exception to this: it bounds how long the *caller* waits by resolving the + state early, and does nothing to the work still in flight underneath. - **Single consumer handle, but multiple handlers per outcome.** The `Completion` handle itself is move-only — only one owner at a time — but each state's `onOk`/`onErr` are vectors, so repeated `then()`/`onError()` @@ -315,6 +384,10 @@ state; the log is emitted only when the state itself is finally destroyed with a is the executor on which every callback is posted. - [`logger.md`](logger.md) — `morph::log::logError`, the error-handling sink used by orphan detection when an error is abandoned. +- [`backend.md`](backend.md) — `morph::backend::LimitPolicy::executeTimeout`, + the *server-side* counterpart to + [the client-side execute deadline](#client-side-execute-deadline), and + `TimeoutError` / `ClientTimeoutError`. - [`error_handling.md`](../error_handling.md) — the framework-wide error-propagation story; the orphan-logging contract detailed in this file is summarised there alongside the executor and backend error paths. diff --git a/include/morph/core/backend.hpp b/include/morph/core/backend.hpp index 65a731a0..1b80b125 100644 --- a/include/morph/core/backend.hpp +++ b/include/morph/core/backend.hpp @@ -459,6 +459,22 @@ struct TimeoutError : std::runtime_error { TimeoutError() : std::runtime_error{"execute timed out on the server"} {} }; +/// @brief Thrown to a pending `Completion` when `Bridge::setExecuteDeadline`'s +/// duration elapses before any reply arrives — a frame silently +/// dropped by `QtWebSocketServerConfig::messagesPerSecond`, or a +/// genuinely hung server, either way. +/// +/// Distinct from `TimeoutError`: that type means the *server* explicitly +/// replied that it hit `LimitPolicy::executeTimeout` while the action was +/// still running. `ClientTimeoutError` means the client gave up waiting — +/// no reply of any kind arrived, so whether the server ever received the +/// request, is still processing it, or replied to a connection that had +/// already dropped is unknown. See `docs/spec/core/completion.md`. +struct ClientTimeoutError : std::runtime_error { + /// @brief Constructs the error with a canned diagnostic message. + ClientTimeoutError() : std::runtime_error{"execute timed out waiting for any reply"} {} +}; + /// @brief In-process backend that executes model actions on a thread pool strand. /// /// Each model instance gets its own strand so actions are serialised per-model diff --git a/include/morph/core/bridge.hpp b/include/morph/core/bridge.hpp index c3c06035..9172b042 100644 --- a/include/morph/core/bridge.hpp +++ b/include/morph/core/bridge.hpp @@ -4,6 +4,7 @@ #include #include #include +#include #include #include #include @@ -24,6 +25,7 @@ #include "completion.hpp" #include "model_key.hpp" #include "registry.hpp" +#include "timeout_scheduler.hpp" namespace morph::bridge { @@ -688,6 +690,43 @@ class Bridge { } } + /// @brief Sets (or disables) the client-side execute deadline. + /// + /// Every `executeVia()` call after this point races the real reply against + /// @p deadline; whichever settles first wins (`CompletionState::setValue`/ + /// `setException` are idempotent — see `completion.hpp`). If @p deadline + /// elapses first, the pending `Completion` fails with + /// `::morph::backend::ClientTimeoutError`; the real reply, if it arrives + /// later, is silently discarded exactly like any other late write to an + /// already-resolved `CompletionState`. + /// + /// Disabled (`std::chrono::milliseconds{0}`, the default) reproduces + /// today's exact behavior: a dropped frame or a hung server leaves the + /// `Completion` pending forever, same as before this method existed. + /// + /// The backing `TimeoutScheduler` (and its one background thread) is + /// created lazily on the first call that enables a deadline, so a `Bridge` + /// that never opts in spawns no extra thread. Once created it lives until + /// `~Bridge()`; setting the deadline back to `0` stops new calls from + /// arming it but does not tear the thread down. Thread-safe. + /// + /// @param deadline Maximum time to wait for any reply. `0` disables the + /// deadline. + void setExecuteDeadline(std::chrono::milliseconds deadline) { + std::scoped_lock const lock{_executeDeadlineMtx}; + _executeDeadline = deadline; + if (_executeDeadline.count() > 0 && !_timeoutScheduler) { + _timeoutScheduler = std::make_unique<::morph::async::detail::TimeoutScheduler>(); + } + } + + /// @brief Returns the currently installed client-side execute deadline. + /// @return The deadline; `std::chrono::milliseconds{0}` when disabled. + [[nodiscard]] std::chrono::milliseconds executeDeadline() const { + std::scoped_lock const lock{_executeDeadlineMtx}; + return _executeDeadline; + } + /// @brief Returns a copy of the currently installed default session. Thread-safe. /// @return Snapshot of the default `Context`. [[nodiscard]] ::morph::session::Context defaultSession() const { @@ -946,6 +985,22 @@ class Bridge { // each of which decrements exactly once (setValue/setException are // first-result-wins, so only one of the two ever actually fires). _pendingCalls.fetch_add(1, std::memory_order_relaxed); + // Arm the client-side deadline (setExecuteDeadline) only for real + // dispatches -- the fast-failed "handler not bound" completion above is + // already resolved and needs no timer. Reading the deadline and arming + // it happen under one lock so a concurrent setExecuteDeadline() cannot + // interleave between the two. + std::optional<::morph::async::detail::TimeoutScheduler::Handle> deadlineHandle; + { + std::scoped_lock const lock{_executeDeadlineMtx}; + if (_executeDeadline.count() > 0 && _timeoutScheduler) { + // The callback captures `typedState` alone -- never `this` -- so + // it stays safe to fire even while ~Bridge() is running. + deadlineHandle = _timeoutScheduler->schedule(_executeDeadline, [typedState] { + typedState->setException(std::make_exception_ptr(::morph::backend::ClientTimeoutError{})); + }); + } + } ::morph::backend::detail::ActionCall call; call.modelTypeId = std::string{::morph::model::ModelTraits::typeId()}; call.actionTypeId = std::string{::morph::model::ActionTraits::typeId()}; @@ -1051,8 +1106,22 @@ class Bridge { } auto anyCompletion = backend->execute(::morph::exec::detail::ModelId{raw}, std::move(call), cbExec); anyCompletion - .then([typedState, onResult = std::move(onResult), this, raw, + .then([typedState, onResult = std::move(onResult), this, raw, deadlineHandle, alive = liveness()](const std::shared_ptr& vAny) { + // Disarm the client-side deadline first, before any of the + // forwarding work below: a slow onResult/publishResult callback + // must not give the timer a window to fire concurrently and + // resolve this completion with ClientTimeoutError while the real + // result is already in hand. Guarded on the same liveness token + // the rest of this lambda uses -- `_timeoutScheduler` and + // `_executeDeadlineMtx` are Bridge members, and this callback can + // in principle run after ~Bridge(). Leaving the entry armed in + // that case is harmless: ~TimeoutScheduler drops pending entries + // without firing them. + if (deadlineHandle && !alive.expired()) { + std::scoped_lock const lock{_executeDeadlineMtx}; + _timeoutScheduler->cancel(*deadlineHandle); + } // Guard the value-forwarding: if R's move/copy throws (or the cast // is somehow wrong), route the exception to the typed completion's // error sink instead of letting it escape the callback executor — @@ -1106,12 +1175,19 @@ class Bridge { typedState->setException(std::current_exception()); } }) - .onError([typedState, this, alive = liveness()](const std::exception_ptr& err) { + .onError([typedState, this, deadlineHandle, alive = liveness()](const std::exception_ptr& err) { // The other of the two mutually-exclusive resolution paths -- // see the .then continuation above. Same liveness guard: this // touches `this` and must not run once the Bridge might be gone. + // Same disarm-first reasoning as the success branch above: a + // real error reply settles the completion, so the deadline + // must not also fire. if (!alive.expired()) { this->_pendingCalls.fetch_sub(1, std::memory_order_relaxed); + if (deadlineHandle) { + std::scoped_lock const lock{_executeDeadlineMtx}; + _timeoutScheduler->cancel(*deadlineHandle); + } } typedState->setException(err); }); @@ -1325,6 +1401,16 @@ class Bridge { ::morph::session::Context _defaultSession; mutable std::mutex _principalMtx; ::morph::session::Principal _principal; + // Client-side execute deadline (see setExecuteDeadline). Both the duration + // and the lazily-created scheduler live under one mutex, so a concurrent + // setExecuteDeadline() can never let executeVia() observe a non-zero + // deadline before the scheduler backing it exists. Declared ahead of + // `_liveness` (the last member, and therefore the first destroyed) so the + // liveness token an in-flight completion callback checks before touching + // these has already expired by the time they are torn down. + mutable std::mutex _executeDeadlineMtx; + std::chrono::milliseconds _executeDeadline{0}; + std::unique_ptr<::morph::async::detail::TimeoutScheduler> _timeoutScheduler; // Instance subscriptions. Held against the binding rather than a fixed // instance id so a re-pointed handler keeps its subscriptions; matched at // publish time by comparing the binding's current instance. diff --git a/include/morph/core/remote.hpp b/include/morph/core/remote.hpp index b283c080..6d46d86a 100644 --- a/include/morph/core/remote.hpp +++ b/include/morph/core/remote.hpp @@ -26,6 +26,7 @@ #include "backend.hpp" #include "logger.hpp" #include "observability.hpp" +#include "timeout_scheduler.hpp" #include "wire.hpp" namespace morph::backend { @@ -57,115 +58,6 @@ struct LimitPolicy { namespace detail { -/// @brief Background scheduler that invokes a callback once after a delay, unless cancelled first. -/// -/// `RemoteServer` is transport-agnostic and its `IExecutor` has no delayed-post -/// primitive, so a single dedicated thread per instance tracks pending -/// deadlines and fires callbacks when they elapse. Used to enforce -/// `LimitPolicy::executeTimeout` — see `docs/spec/core/backend.md`. -class TimeoutScheduler { -public: - /// @brief Opaque identifier for one scheduled callback. - using Handle = std::uint64_t; - - /// @brief Starts the background thread. - TimeoutScheduler() : _thread{[this] { run(); }} {} - - /// @brief Stops the background thread and joins it. - ~TimeoutScheduler() { - { - std::scoped_lock const lock{_mtx}; - _stop = true; - } - _cv.notify_all(); - _thread.join(); - } - - TimeoutScheduler(const TimeoutScheduler&) = delete; - TimeoutScheduler& operator=(const TimeoutScheduler&) = delete; - TimeoutScheduler(TimeoutScheduler&&) = delete; - TimeoutScheduler& operator=(TimeoutScheduler&&) = delete; - - /// @brief Schedules @p callback to run after @p delay on the scheduler's - /// background thread, unless cancelled first via `cancel()`. - /// @param delay Time to wait before firing. - /// @param callback Invoked on the scheduler thread if not cancelled in time. - /// Exceptions it throws are logged and swallowed. - /// @return Handle usable with `cancel()`. - Handle schedule(std::chrono::milliseconds delay, std::function callback) { - auto const deadline = std::chrono::steady_clock::now() + delay; - std::scoped_lock const lock{_mtx}; - Handle const handle = ++_nextHandle; - auto iter = _entries.emplace(deadline, Entry{handle, std::move(callback)}); - _index[handle] = iter; - _cv.notify_all(); - return handle; - } - - /// @brief Cancels a previously scheduled callback immediately. - /// - /// If @p handle has not fired yet, its entry (and anything its callback - /// captured) is erased right away — the caller does not have to wait for - /// the original deadline for that memory to be released. A no-op if - /// @p handle already fired or was already cancelled. - /// @param handle Handle returned by a prior `schedule()` call. - void cancel(Handle handle) { - std::scoped_lock const lock{_mtx}; - auto found = _index.find(handle); - if (found == _index.end()) { - return; - } - _entries.erase(found->second); - _index.erase(found); - } - -private: - struct Entry { - Handle handle; - std::function callback; - }; - - void run() { - std::unique_lock lock{_mtx}; - while (!_stop) { - if (_entries.empty()) { - _cv.wait(lock); - continue; - } - auto const nextDeadline = _entries.begin()->first; - _cv.wait_until(lock, nextDeadline); - if (_stop) { - break; - } - auto now = std::chrono::steady_clock::now(); - while (!_entries.empty() && _entries.begin()->first <= now) { - auto iter = _entries.begin(); - Entry entry = std::move(iter->second); - _index.erase(entry.handle); - _entries.erase(iter); - lock.unlock(); - try { - entry.callback(); - } catch (const std::exception& exc) { - ::morph::log::logError("[timeout-scheduler] callback threw: " + std::string{exc.what()}); - } catch (...) { - ::morph::log::logError("[timeout-scheduler] callback threw unknown exception"); - } - lock.lock(); - now = std::chrono::steady_clock::now(); - } - } - } - - std::mutex _mtx; - std::condition_variable _cv; - std::multimap _entries; - std::unordered_map::iterator> _index; - Handle _nextHandle{0}; - bool _stop{false}; - std::thread _thread; -}; - /// @brief Keyed 64-bit bijection that turns a monotonic counter into an /// unguessable, non-sequential id. /// @@ -487,7 +379,7 @@ class RemoteServer : public std::enable_shared_from_this { std::scoped_lock const lock{_limitsMtx}; _limits = policy; if (_limits.executeTimeout.count() > 0 && !_timeoutScheduler) { - _timeoutScheduler = std::make_unique(); + _timeoutScheduler = std::make_unique<::morph::async::detail::TimeoutScheduler>(); } } @@ -1290,7 +1182,7 @@ class RemoteServer : public std::enable_shared_from_this { } }; - detail::TimeoutScheduler::Handle timeoutHandle{}; + ::morph::async::detail::TimeoutScheduler::Handle timeoutHandle{}; if (limits.executeTimeout.count() > 0) { std::scoped_lock const lock{_limitsMtx}; if (_timeoutScheduler) { @@ -1452,7 +1344,7 @@ class RemoteServer : public std::enable_shared_from_this { // firing first. Shared by the executeInFlight metric, health()'s inFlight // field, and drainedWithin(): one counter, never double-counted. std::atomic _inFlightExecutes{0}; - std::unique_ptr _timeoutScheduler; + std::unique_ptr<::morph::async::detail::TimeoutScheduler> _timeoutScheduler; // Set once by beginShutdown() and never cleared — there is no // un-shutdown. Checked at the top of dispatchMessage() for register and // execute envelopes only; deregister and any other kind are unaffected. diff --git a/include/morph/core/timeout_scheduler.hpp b/include/morph/core/timeout_scheduler.hpp new file mode 100644 index 00000000..8f8c8e0e --- /dev/null +++ b/include/morph/core/timeout_scheduler.hpp @@ -0,0 +1,130 @@ +// SPDX-License-Identifier: Apache-2.0 + +#pragma once +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include "logger.hpp" + +namespace morph::async::detail { + +/// @brief Background scheduler that invokes a callback once after a delay, unless cancelled first. +/// +/// Neither `Bridge` nor `RemoteServer` is bound to a specific `IExecutor` +/// with a delayed-post primitive, so a single dedicated thread per instance +/// tracks pending deadlines and fires callbacks when they elapse. Used by +/// `RemoteServer` to enforce `LimitPolicy::executeTimeout` (server-side — +/// see `docs/spec/core/backend.md`) and by `Bridge::setExecuteDeadline` +/// (client-side — see `docs/spec/core/completion.md`). +class TimeoutScheduler { +public: + /// @brief Opaque identifier for one scheduled callback. + using Handle = std::uint64_t; + + /// @brief Starts the background thread. + TimeoutScheduler() : _thread{[this] { run(); }} {} + + /// @brief Stops the background thread and joins it. + ~TimeoutScheduler() { + { + std::scoped_lock const lock{_mtx}; + _stop = true; + } + _cv.notify_all(); + _thread.join(); + } + + TimeoutScheduler(const TimeoutScheduler&) = delete; + TimeoutScheduler& operator=(const TimeoutScheduler&) = delete; + TimeoutScheduler(TimeoutScheduler&&) = delete; + TimeoutScheduler& operator=(TimeoutScheduler&&) = delete; + + /// @brief Schedules @p callback to run after @p delay on the scheduler's + /// background thread, unless cancelled first via `cancel()`. + /// @param delay Time to wait before firing. + /// @param callback Invoked on the scheduler thread if not cancelled in time. + /// Exceptions it throws are logged and swallowed. + /// @return Handle usable with `cancel()`. + Handle schedule(std::chrono::milliseconds delay, std::function callback) { + auto const deadline = std::chrono::steady_clock::now() + delay; + std::scoped_lock const lock{_mtx}; + Handle const handle = ++_nextHandle; + auto iter = _entries.emplace(deadline, Entry{handle, std::move(callback)}); + _index[handle] = iter; + _cv.notify_all(); + return handle; + } + + /// @brief Cancels a previously scheduled callback immediately. + /// + /// If @p handle has not fired yet, its entry (and anything its callback + /// captured) is erased right away — the caller does not have to wait for + /// the original deadline for that memory to be released. A no-op if + /// @p handle already fired or was already cancelled. + /// @param handle Handle returned by a prior `schedule()` call. + void cancel(Handle handle) { + std::scoped_lock const lock{_mtx}; + auto found = _index.find(handle); + if (found == _index.end()) { + return; + } + _entries.erase(found->second); + _index.erase(found); + } + +private: + struct Entry { + Handle handle; + std::function callback; + }; + + void run() { + std::unique_lock lock{_mtx}; + while (!_stop) { + if (_entries.empty()) { + _cv.wait(lock); + continue; + } + auto const nextDeadline = _entries.begin()->first; + _cv.wait_until(lock, nextDeadline); + if (_stop) { + break; + } + auto now = std::chrono::steady_clock::now(); + while (!_entries.empty() && _entries.begin()->first <= now) { + auto iter = _entries.begin(); + Entry entry = std::move(iter->second); + _index.erase(entry.handle); + _entries.erase(iter); + lock.unlock(); + try { + entry.callback(); + } catch (const std::exception& exc) { + ::morph::log::logError("[timeout-scheduler] callback threw: " + std::string{exc.what()}); + } catch (...) { + ::morph::log::logError("[timeout-scheduler] callback threw unknown exception"); + } + lock.lock(); + now = std::chrono::steady_clock::now(); + } + } + } + + std::mutex _mtx; + std::condition_variable _cv; + std::multimap _entries; + std::unordered_map::iterator> _index; + Handle _nextHandle{0}; + bool _stop{false}; + std::thread _thread; +}; + +} // namespace morph::async::detail diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 17e22017..c790cb2d 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -31,6 +31,7 @@ add_executable(morph_tests test_action_validation.cpp test_security_fixes.cpp test_bridge_lifetime.cpp + test_client_execute_deadline.cpp test_dispatch_di.cpp test_handler_binding.cpp test_switch_backend.cpp diff --git a/tests/test_client_execute_deadline.cpp b/tests/test_client_execute_deadline.cpp new file mode 100644 index 00000000..c04662fe --- /dev/null +++ b/tests/test_client_execute_deadline.cpp @@ -0,0 +1,241 @@ +// SPDX-License-Identifier: Apache-2.0 +// +// Coverage for the client-side execute deadline (examples/LADDER.md's +// "Framework prerequisites" #2): Bridge::setExecuteDeadline races the real +// reply against a client-owned timeout, so a frame silently dropped by +// QtWebSocketServerConfig::messagesPerSecond, or a genuinely hung server, +// no longer blocks the calling Completion forever. + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include "test_support.hpp" + +namespace { + +struct DeadlineCount { + int x = 0; +}; + +struct DeadlineModel { + int execute(const DeadlineCount& a) { return a.x; } +}; + +// A backend whose execute() never resolves its Completion, simulating a frame +// the server dropped -- no reply, ever, on this path -- or a hung server. +class NeverRepliesBackend : public morph::backend::detail::IBackend { +public: + morph::exec::detail::ModelId registerModel( + const std::string&, std::function()>) override { + return morph::exec::detail::ModelId{1}; + } + void deregisterModel(morph::exec::detail::ModelId) override {} + morph::async::Completion> execute(morph::exec::detail::ModelId, + morph::backend::detail::ActionCall, + morph::exec::IExecutor* cbExec) override { + auto state = std::make_shared>>(); + ++liveCompletions; + return morph::async::Completion>{state, cbExec}; + // `state` is intentionally dropped here with no setValue/setException + // ever called -- the Completion this returns never settles on its + // own, matching a dropped frame or a server that never replies. + } + void notifyBackendChanged() override {} + // Deliberately a no-op: a real backend resolves its outstanding states + // here, which is exactly the "something eventually settles it" behaviour + // these tests must not rely on. + void cancelPending(const std::exception_ptr&) override {} + + std::atomic liveCompletions{0}; +}; + +// A backend that holds every state it hands out and only settles it when the +// test says so -- a server whose reply arrives *after* the client already gave +// up. Lets the "a late real reply is silently discarded" guarantee be asserted +// deterministically rather than by racing wall-clock timers. +class LateReplyBackend : public morph::backend::detail::IBackend { +public: + morph::exec::detail::ModelId registerModel( + const std::string&, std::function()>) override { + return morph::exec::detail::ModelId{1}; + } + void deregisterModel(morph::exec::detail::ModelId) override {} + morph::async::Completion> execute(morph::exec::detail::ModelId, + morph::backend::detail::ActionCall, + morph::exec::IExecutor* cbExec) override { + auto state = std::make_shared>>(); + { + std::scoped_lock const lock{_mtx}; + _pending.push_back(state); + } + return morph::async::Completion>{state, cbExec}; + } + void notifyBackendChanged() override {} + void cancelPending(const std::exception_ptr&) override {} + + /// Settles every outstanding request with @p value, as a server reply that + /// finally turned up would. + void replyLate(int value) { + std::vector>>> pending; + { + std::scoped_lock const lock{_mtx}; + pending.swap(_pending); + } + for (auto& state : pending) { + state->setValue(std::static_pointer_cast(std::make_shared(value))); + } + } + +private: + std::mutex _mtx; + std::vector>>> _pending; +}; + +} // namespace + +template <> +struct morph::model::ActionTraits { + using Result = int; + static constexpr std::string_view typeId() { return "Deadline_Count"; } + static std::string toJson(const DeadlineCount& a) { return R"({"x":)" + std::to_string(a.x) + "}"; } + static DeadlineCount fromJson(std::string_view) { return {}; } + static std::string resultToJson(const int& r) { return std::to_string(r); } + static int resultFromJson(std::string_view s) { return std::stoi(std::string{s}); } +}; +template <> +struct morph::model::ModelTraits { + static constexpr std::string_view typeId() { return "Deadline_Model"; } +}; + +TEST_CASE("Bridge::setExecuteDeadline(0) (the default) never fires -- a call that never replies " + "stays pending, matching pre-existing behavior", + "[core][bridge][client-deadline]") { + morph::exec::MainThreadExecutor exec; + morph::bridge::Bridge bridge{std::make_unique()}; + CHECK(bridge.executeDeadline() == std::chrono::milliseconds{0}); + morph::bridge::BridgeHandler handler{bridge, &exec}; + + bool resolved = false; + handler.execute(DeadlineCount{.x = 1}) + .then([&resolved](int) { resolved = true; }) + .onError([&resolved](const std::exception_ptr&) { resolved = true; }); + exec.runFor(std::chrono::milliseconds{200}); + CHECK_FALSE(resolved); +} + +TEST_CASE("Bridge::setExecuteDeadline fires ClientTimeoutError when no reply arrives in time", + "[core][bridge][client-deadline]") { + morph::exec::MainThreadExecutor exec; + morph::bridge::Bridge bridge{std::make_unique()}; + bridge.setExecuteDeadline(std::chrono::milliseconds{50}); + CHECK(bridge.executeDeadline() == std::chrono::milliseconds{50}); + morph::bridge::BridgeHandler handler{bridge, &exec}; + + bool failed = false; + bool threwClientTimeout = false; + handler.execute(DeadlineCount{.x = 1}).onError([&](const std::exception_ptr& err) { + failed = true; + try { + std::rethrow_exception(err); + } catch (const morph::backend::ClientTimeoutError&) { + threwClientTimeout = true; + } catch (...) { + } + }); + // Poll rather than a single runFor(): the deadline fires on the + // TimeoutScheduler's own background thread, which posts to `exec` -- + // give it real wall-clock slack, matching this codebase's other + // cross-thread test patterns. + for (int i = 0; i < 50 && !failed; ++i) { + exec.runFor(std::chrono::milliseconds{20}); + } + REQUIRE(failed); + CHECK(threwClientTimeout); +} + +TEST_CASE("A deadline that is cancelled by a real, on-time reply does not also fire", + "[core][bridge][client-deadline]") { + // Uses the ordinary in-process LocalBackend, which always replies quickly. + // Exercises the disarm path (the `.then`/`.onError` cancel-before-settle + // lines) and pins the happy path: enabling a deadline must not perturb a + // call that replies in time. What a *missing* disarm would look like from + // the outside is covered by the next test instead -- because + // CompletionState is first-result-wins, a deadline that fires after an + // on-time reply is a no-op, so it cannot be observed here. + morph::exec::ThreadPoolExecutor workerPool{2}; + morph::exec::MainThreadExecutor guiExec; + morph::bridge::Bridge bridge{std::make_unique(workerPool)}; + bridge.setExecuteDeadline(std::chrono::milliseconds{2000}); // generous; must not fire + morph::bridge::BridgeHandler handler{bridge, &guiExec}; + + int result = -1; + bool failed = false; + handler.execute(DeadlineCount{.x = 7}) + .then([&result](int r) { result = r; }) + .onError([&failed](const std::exception_ptr&) { failed = true; }); + for (int i = 0; i < 50 && result == -1 && !failed; ++i) { + guiExec.runFor(std::chrono::milliseconds{10}); + } + CHECK(result == 7); + CHECK_FALSE(failed); + // If the disarm did not work, the 2000ms deadline would still be pending on + // the scheduler's background thread when the Bridge goes out of scope here. + // That must not hang the test process: ~TimeoutScheduler drops pending + // entries without firing them and joins its thread unconditionally, so a + // leaked entry costs nothing at teardown. Noted rather than asserted -- + // there is no public handle to observe it through. +} + +TEST_CASE("A real reply that arrives after the deadline already fired is silently discarded", + "[core][bridge][client-deadline]") { + // The idempotency half of the race documented in docs/spec/core/completion.md: + // once the deadline resolved the Completion with ClientTimeoutError, the + // server's eventual reply must not resurrect it with a value. Driven + // explicitly by the test rather than by wall-clock luck. + morph::exec::MainThreadExecutor exec; + auto backendOwner = std::make_unique(); + auto* const backend = backendOwner.get(); + morph::bridge::Bridge bridge{std::move(backendOwner)}; + bridge.setExecuteDeadline(std::chrono::milliseconds{50}); + morph::bridge::BridgeHandler handler{bridge, &exec}; + + int settleCount = 0; + int value = -1; + bool threwClientTimeout = false; + handler.execute(DeadlineCount{.x = 1}) + .then([&](int r) { + ++settleCount; + value = r; + }) + .onError([&](const std::exception_ptr& err) { + ++settleCount; + try { + std::rethrow_exception(err); + } catch (const morph::backend::ClientTimeoutError&) { + threwClientTimeout = true; + } catch (...) { + } + }); + + for (int i = 0; i < 50 && settleCount == 0; ++i) { + exec.runFor(std::chrono::milliseconds{20}); + } + REQUIRE(settleCount == 1); + REQUIRE(threwClientTimeout); + + // The server finally replies. Nothing may change. + backend->replyLate(99); + exec.runFor(std::chrono::milliseconds{200}); + CHECK(settleCount == 1); + CHECK(value == -1); +} From ecf27269b72d9b2ad306457fa378e68d6e19f93e Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Sat, 8 Aug 2026 01:33:09 +0300 Subject: [PATCH 03/20] core: cover the execute deadline's disarm and verify its header standalone Review follow-ups on the client-side execute deadline: - Add include/morph/core/timeout_scheduler.hpp to the morph target's FILE_SET HEADERS list, so VERIFY_INTERFACE_HEADER_SETS compiles it standalone like every other public header. - Add a test that the disarm actually releases the scheduler entry. CompletionState is first-write-wins, so a stray timer firing after an on-time reply is invisible at the value level; what the disarm buys is lifetime. The new case watches the CompletionState through a weak_ptr and fails if the pending timer still pins it. Correct the third case's trailing comment, which claimed a coverage that did not exist. - Drop NeverRepliesBackend::liveCompletions (written, never read). - Drop / from remote.hpp, dead since TimeoutScheduler moved. - Merge the duplicated backend.md entry in completion.md's cross-references. - Note in setExecuteDeadline's docs that the clock starts before the backend's own execute() dispatch. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01HffixfknmXdAjnM5hSGRBb --- CMakeLists.txt | 1 + docs/spec/core/completion.md | 9 +++--- include/morph/core/bridge.hpp | 5 +++ include/morph/core/remote.hpp | 2 -- tests/test_client_execute_deadline.cpp | 45 ++++++++++++++++++++------ 5 files changed, 45 insertions(+), 17 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 958f841c..c8f016ac 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -160,6 +160,7 @@ target_sources(morph include/morph/core/executor.hpp include/morph/core/strand.hpp include/morph/core/completion.hpp + include/morph/core/timeout_scheduler.hpp include/morph/core/model.hpp include/morph/core/registry.hpp include/morph/core/backend.hpp diff --git a/docs/spec/core/completion.md b/docs/spec/core/completion.md index 77fca413..ce08dcf9 100644 --- a/docs/spec/core/completion.md +++ b/docs/spec/core/completion.md @@ -384,14 +384,13 @@ state; the log is emitted only when the state itself is finally destroyed with a is the executor on which every callback is posted. - [`logger.md`](logger.md) — `morph::log::logError`, the error-handling sink used by orphan detection when an error is abandoned. -- [`backend.md`](backend.md) — `morph::backend::LimitPolicy::executeTimeout`, - the *server-side* counterpart to +- [`backend.md`](backend.md) — backends resolve the pending `Completion` when a + response arrives; also `morph::backend::LimitPolicy::executeTimeout`, the + *server-side* counterpart to [the client-side execute deadline](#client-side-execute-deadline), and `TimeoutError` / `ClientTimeoutError`. - [`error_handling.md`](../error_handling.md) — the framework-wide error-propagation story; the orphan-logging contract detailed in this file is summarised there alongside the executor and backend error paths. - [`bridge.md`](bridge.md) — `BridgeHandler` produces `Completion` from - `execute()` and posts callbacks on the GUI executor. -- [`backend.md`](backend.md) — backends resolve the pending `Completion` when a - response arrives. \ No newline at end of file + `execute()` and posts callbacks on the GUI executor. \ No newline at end of file diff --git a/include/morph/core/bridge.hpp b/include/morph/core/bridge.hpp index 9172b042..c28844be 100644 --- a/include/morph/core/bridge.hpp +++ b/include/morph/core/bridge.hpp @@ -700,6 +700,11 @@ class Bridge { /// later, is silently discarded exactly like any other late write to an /// already-resolved `CompletionState`. /// + /// The clock starts inside `executeVia()`, immediately before the backend's + /// own `execute()` is dispatched, so @p deadline covers the whole round trip + /// — serialisation, transport, server-side work, and the reply's journey + /// back — not just the time spent waiting after dispatch. + /// /// Disabled (`std::chrono::milliseconds{0}`, the default) reproduces /// today's exact behavior: a dropped frame or a hung server leaves the /// `Completion` pending forever, same as before this method existed. diff --git a/include/morph/core/remote.hpp b/include/morph/core/remote.hpp index 6d46d86a..31c05432 100644 --- a/include/morph/core/remote.hpp +++ b/include/morph/core/remote.hpp @@ -8,14 +8,12 @@ #include #include #include -#include #include #include #include #include #include #include -#include #include #include #include diff --git a/tests/test_client_execute_deadline.cpp b/tests/test_client_execute_deadline.cpp index c04662fe..80c8b98c 100644 --- a/tests/test_client_execute_deadline.cpp +++ b/tests/test_client_execute_deadline.cpp @@ -6,7 +6,6 @@ // QtWebSocketServerConfig::messagesPerSecond, or a genuinely hung server, // no longer blocks the calling Completion forever. -#include #include #include #include @@ -44,7 +43,6 @@ class NeverRepliesBackend : public morph::backend::detail::IBackend { morph::backend::detail::ActionCall, morph::exec::IExecutor* cbExec) override { auto state = std::make_shared>>(); - ++liveCompletions; return morph::async::Completion>{state, cbExec}; // `state` is intentionally dropped here with no setValue/setException // ever called -- the Completion this returns never settles on its @@ -55,8 +53,6 @@ class NeverRepliesBackend : public morph::backend::detail::IBackend { // here, which is exactly the "something eventually settles it" behaviour // these tests must not rely on. void cancelPending(const std::exception_ptr&) override {} - - std::atomic liveCompletions{0}; }; // A backend that holds every state it hands out and only settles it when the @@ -166,12 +162,13 @@ TEST_CASE("Bridge::setExecuteDeadline fires ClientTimeoutError when no reply arr TEST_CASE("A deadline that is cancelled by a real, on-time reply does not also fire", "[core][bridge][client-deadline]") { // Uses the ordinary in-process LocalBackend, which always replies quickly. - // Exercises the disarm path (the `.then`/`.onError` cancel-before-settle - // lines) and pins the happy path: enabling a deadline must not perturb a - // call that replies in time. What a *missing* disarm would look like from - // the outside is covered by the next test instead -- because - // CompletionState is first-result-wins, a deadline that fires after an - // on-time reply is a no-op, so it cannot be observed here. + // Pins the happy path: enabling a deadline must not perturb a call that + // replies in time. It runs *through* the disarm path but cannot detect its + // absence -- because CompletionState is first-result-wins, a stray timer + // firing after an on-time reply is a silent no-op at the value level, so + // deleting the disarm entirely would leave this case green. The disarm's + // actual, observable effect is *lifetime*, and that is what the next test + // ("An on-time reply releases the deadline's scheduler entry") covers. morph::exec::ThreadPoolExecutor workerPool{2}; morph::exec::MainThreadExecutor guiExec; morph::bridge::Bridge bridge{std::make_unique(workerPool)}; @@ -196,6 +193,34 @@ TEST_CASE("A deadline that is cancelled by a real, on-time reply does not also f // there is no public handle to observe it through. } +TEST_CASE("An on-time reply releases the deadline's scheduler entry (and the state it pins)", + "[core][bridge][client-deadline]") { + // What the disarm actually buys: TimeoutScheduler::cancel() erases the + // pending entry and with it the std::function that captures a + // shared_ptr>. Without the disarm that entry -- and the + // completed state it keeps alive -- would be pinned for the full deadline. + morph::exec::ThreadPoolExecutor workerPool{2}; + morph::exec::MainThreadExecutor guiExec; + morph::bridge::Bridge bridge{std::make_unique(workerPool)}; + bridge.setExecuteDeadline(std::chrono::milliseconds{5000}); // long enough that only + morph::bridge::BridgeHandler handler{bridge, &guiExec}; // an actual cancel frees it + + int result = -1; + std::weak_ptr stateWatch; + { + auto completion = handler.execute(DeadlineCount{.x = 7}); + stateWatch = completion.state(); // Completion::state(), completion.hpp:208 + completion.then([&result](int r) { result = r; }); + } + for (int i = 0; i < 50 && result == -1; ++i) { + guiExec.runFor(std::chrono::milliseconds{10}); + } + REQUIRE(result == 7); + guiExec.runFor(std::chrono::milliseconds{100}); + // Without the disarm, the timer entry still owns the state for 5 s. + CHECK(stateWatch.expired()); +} + TEST_CASE("A real reply that arrives after the deadline already fired is silently discarded", "[core][bridge][client-deadline]") { // The idempotency half of the race documented in docs/spec/core/completion.md: From 7d88a8eeb1ef66984d458d77f600252342e9ad02 Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Sat, 8 Aug 2026 02:42:46 +0300 Subject: [PATCH 04/20] core: add an async register-or-attach/attach path for shared/keyed models Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01HffixfknmXdAjnM5hSGRBb --- docs/spec/core/backend.md | 42 ++- docs/spec/core/bridge.md | 11 + docs/spec/core/shared_instances.md | 64 ++++- include/morph/core/backend.hpp | 88 +++++- include/morph/core/bridge.hpp | 224 +++++++++++++--- include/morph/qt/qt_websocket_backend.hpp | 49 ++++ src/qt/qt_websocket_backend.cpp | 65 +++++ tests/test_async_registration.cpp | 312 ++++++++++++++++++++++ 8 files changed, 812 insertions(+), 43 deletions(-) diff --git a/docs/spec/core/backend.md b/docs/spec/core/backend.md index 4aeee4d7..9908323b 100644 --- a/docs/spec/core/backend.md +++ b/docs/spec/core/backend.md @@ -186,11 +186,10 @@ 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. `registerModelShared`/`attachModel` -(shared/keyed handlers) and the re-registration `switchBackend()`/the -reconnect handler perform after a backend swap remain synchronous; giving -those an async path too is a larger change to `Bridge`'s locking model, left -for a future issue if it proves necessary. +`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` is the one backend that currently overrides this, gated by `QtWebSocketBackendConfig::asyncRegistrationEnabled` (default `false` — see @@ -209,6 +208,39 @@ queue is drained by `cancelPending`, which still invokes each queued request's `onError` 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` implements both behind the same +`asyncRegistrationEnabled` flag, reusing the same `callId`-keyed pending map +(reply routing is verb-agnostic — a `register`, a shared `register`, and an +`attach` all reply the same way). An empty `identity.primary` degrades to +`registerModelAsync`, mirroring the synchronous methods' degrade-to-private +behaviour. + +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. + ## Error types Five exception types are thrown into in-flight `Completion`s. The first four are diff --git a/docs/spec/core/bridge.md b/docs/spec/core/bridge.md index f0363166..e5011418 100644 --- a/docs/spec/core/bridge.md +++ b/docs/spec/core/bridge.md @@ -532,6 +532,17 @@ slow remote round-trip on one handler's attach never blocks another handler's construction, destruction, or a `switchBackend()` call on the same `Bridge`). +`attachHandlerAsync`/`ensureBoundAsync` — the non-blocking counterparts +`BridgeHandler::execute()` routes its keyed dispatches through — take +`_attachMtx` over the same scope their synchronous twins do, but **release it +before invoking their `onDone` callback**, on every path including the +synchronous fallback. That is a hard requirement, not a style choice: +`onDone` is where the action itself is dispatched, and a result-keyed dispatch +promotes its binding via `assignHandlerPrimary`, which re-takes `_attachMtx`. +It is the same rule `registerHandlerImpl` already follows for `_mtx`. See +[shared_instances.md](shared_instances.md), "Async register-or-attach and +attach". + `subscribe`/`unsubscribe` mutate the bridge's subscription registry under `_subMtx`. Callbacks never run under that mutex: `publishResult` snapshots the matching sinks under the lock and invokes them outside it, marshalled to the diff --git a/docs/spec/core/shared_instances.md b/docs/spec/core/shared_instances.md index 74f2c836..98221eeb 100644 --- a/docs/spec/core/shared_instances.md +++ b/docs/spec/core/shared_instances.md @@ -11,6 +11,7 @@ - [The instance directory](#the-instance-directory) - [Enumerating live instances](#enumerating-live-instances) - [Wire protocol changes](#wire-protocol-changes) +- [Async register-or-attach and attach](#async-register-or-attach-and-attach) - [Ownership and authorization](#ownership-and-authorization) - [Lifetime and the A7 connection-scope change](#lifetime-and-the-a7-connection-scope-change) - [API reference](#api-reference) @@ -271,6 +272,65 @@ its primary so journal entries carry the entity key — but conflating them woul silently change behaviour for anyone already setting `contextKey` for journal 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"): + +| Virtual | Synchronous counterpart | Preferred 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. + +**Why this exists.** `registerModelShared`/`attachModel` are synchronous, so on +a wire backend they block in a nested `QEventLoop`, which a WASM main thread +cannot spin at all. Before this, the *first* payload-keyed action a WASM client +executed — the very shape a keyed screen is built on — aborted the page. See +`examples/LADDER.md`, "Framework prerequisites" #1, for the rung-3 (`polls`) +scenario that motivated closing this. + +**What callers see.** Nothing, by design. `BridgeHandler::execute()`'s +signature and its documented contract are unchanged, including the promise that +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. + +**`attach()` stays synchronous.** The standalone `handler.attach(key)` is a +`void` call with no `Completion` to route a failure through, so it still throws +and still blocks. That is deliberate, and its own doc comment already named the +escape hatch: *a caller that wants the failure delivered asynchronously should +attach via a payload-keyed action's `execute()` instead.* This section is what +makes that escape hatch real. Giving `attach()` itself an async form would mean +changing its return type, which is a separate, breaking decision. + +**Locking.** `Bridge::attachHandlerAsync`/`ensureBoundAsync` hold `_attachMtx` +across the guard check, the async dispatch, and the synchronous fallback's own +state mutation — but never across the `onDone` callback. This is load-bearing, +not stylistic: what `execute()` does from inside `onDone` is dispatch the +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`. + ## Ownership and authorization `RemoteServer` records an `ownerPrincipal` for each instance at register time @@ -341,9 +401,11 @@ strictly reduces pressure on it. | `BRIDGE_KEY_FROM(A, &A::field)` | macro | Declares that a further action `A` also carries the key. | | `BRIDGE_MODEL_KEY_FROM_RESULT(M, A, &R::field)` | macro | As `BRIDGE_MODEL_KEY`, but the key comes from `A`'s *result*. | | `BRIDGE_KEY_FROM_RESULT(A, &R::field)` | macro | A further creating action whose result establishes the key. | -| `handler.attach(key)` | `void` | Attaches (or re-points) without executing an action. | +| `handler.attach(key)` | `void` | Attaches (or re-points) without executing an action. Synchronous and throwing, by design — see [Async register-or-attach and attach](#async-register-or-attach-and-attach). | | `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 (payload-keyed) or bind-and-promote (result-keyed) step takes the backend's async path when one exists, so the call no longer blocks on a round-trip — visible only as *not aborting a WASM main thread*. 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. | ## Design decisions diff --git a/include/morph/core/backend.hpp b/include/morph/core/backend.hpp index 1b80b125..833c1704 100644 --- a/include/morph/core/backend.hpp +++ b/include/morph/core/backend.hpp @@ -131,10 +131,12 @@ struct IBackend { /// /// @note Scope: only `Bridge::registerHandler()`'s plain (non-shared) /// registration path — a `BridgeHandler`'s initial construction — - /// uses this. Shared/keyed registration (`registerModelShared`, - /// `attachModel`) and the re-registration `switchBackend()`/the - /// reconnect handler perform after a backend swap remain - /// synchronous; see docs/spec/core/backend.md. + /// 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. @@ -155,6 +157,46 @@ struct IBackend { 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). + /// + /// 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, + /// matching every caller's behavior before this method existed. + /// + /// @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 @@ -218,6 +260,44 @@ 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. + /// + /// @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 diff --git a/include/morph/core/bridge.hpp b/include/morph/core/bridge.hpp index c28844be..9d79ec82 100644 --- a/include/morph/core/bridge.hpp +++ b/include/morph/core/bridge.hpp @@ -358,6 +358,91 @@ class Bridge { binding->currentId.store(newId.v); } + /// @brief Async counterpart to `attachHandler`: prefers the backend's + /// `attachModelAsync` when available, invoking @p onDone once + /// attached (or failed) instead of blocking. + /// + /// Falls back to the synchronous `attachHandler` body (and calls @p onDone + /// immediately, from this thread) when the backend offers no async + /// path — so a caller that always goes through this method behaves + /// identically to calling `attachHandler` directly, on every backend + /// that has not opted in to `attachModelAsync`. + /// + /// @par Locking + /// `_attachMtx` is held around the guard check, the async branch's + /// *dispatch*, and the synchronous branch's own state mutation — matching + /// `attachHandler`'s existing lock scope — but is **released before + /// @p onDone is ever invoked**, on every path. That is not a nicety: what + /// `execute()` does from inside @p onDone is dispatch the action, and a + /// result-keyed dispatch promotes its binding through + /// `assignHandlerPrimary`, which takes `_attachMtx` itself. Invoking + /// @p onDone under the lock therefore self-deadlocks the moment the + /// completion is delivered on the calling thread — which is exactly what + /// the synchronous fallback below does, and what an inline executor does + /// for every callback. This is `registerHandlerImpl`'s existing rule ("the + /// backend call must not run under `_mtx`") applied to `_attachMtx`. + /// + /// The completion callbacks below likewise do not take `_attachMtx`: per + /// `IBackend::attachModelAsync`'s contract they run once the reply + /// arrives, i.e. after this frame has returned and released it. + /// + /// @tparam Model Concrete model type. + /// @param binding Shared binding, as returned by `registerSharedHandler()`. + /// @param primary Canonical string encoding of the primary key to attach to. + /// @param onDone Invoked with `nullptr` on success, or a non-null + /// `exception_ptr` on failure — always exactly once, + /// synchronously if the fallback path is taken. + template + void attachHandlerAsync(const std::shared_ptr& binding, std::string primary, + const std::function& onDone) { + std::unique_lock lock{_attachMtx}; + if (binding->primary == primary && binding->currentId.load() != 0U) { + lock.unlock(); + onDone(nullptr); + return; + } + auto const previous = ::morph::exec::detail::ModelId{binding->currentId.load()}; + auto backend = loadBackend(); + auto primaryCopy = primary; + std::weak_ptr const weakLiveness{_liveness}; + std::weak_ptr const weakBinding{binding}; + bool const started = backend->attachModelAsync( + binding->typeId, binding->modelFactory, {.contextKey = primaryCopy, .primary = primaryCopy}, previous, + [weakLiveness, weakBinding, primaryCopy, onDone](::morph::exec::detail::ModelId newId) { + auto aliveToken = weakLiveness.lock(); + if (!aliveToken) { + return; // The Bridge is gone; publishing this id would be pointless. + } + auto strongBinding = weakBinding.lock(); + if (!strongBinding) { + return; // The BridgeHandler (and its binding) is gone. + } + strongBinding->contextKey = primaryCopy; + strongBinding->primary = primaryCopy; + strongBinding->currentId.store(newId.v); + onDone(nullptr); + }, + [onDone](const std::string& message) { onDone(std::make_exception_ptr(std::runtime_error(message))); }); + if (started) { + return; + } + // No async path on this backend: run the identical synchronous attach + // `attachHandler` would have run, under the same lock, then report the + // outcome only once the lock is gone (see @par Locking above). + std::exception_ptr failure; + try { + auto newId = backend->attachModel(binding->typeId, binding->modelFactory, + {.contextKey = primary, .primary = primary}, previous); + binding->contextKey = primary; + binding->primary = std::move(primary); + binding->currentId.store(newId.v); + } catch (...) { + failure = std::current_exception(); + } + lock.unlock(); + onDone(failure); + } + /// @brief Gives @p binding an anonymous instance if it does not have one yet. /// /// Used before a result-keyed action: such an action generates the key it @@ -375,6 +460,56 @@ class Bridge { binding->currentId.store(newId.v); } + /// @brief Async counterpart to `ensureBound`. See `attachHandlerAsync`'s + /// doc comment for the fallback and locking contract. + /// @param binding Shared binding to bind. + /// @param onDone Invoked exactly once: `nullptr` on success, or a + /// non-null `exception_ptr` on failure. + void ensureBoundAsync(const std::shared_ptr& binding, + const std::function& onDone) { + std::unique_lock lock{_attachMtx}; + if (binding->currentId.load() != 0U) { + lock.unlock(); + onDone(nullptr); + return; + } + auto backend = loadBackend(); + std::weak_ptr const weakLiveness{_liveness}; + std::weak_ptr const weakBinding{binding}; + bool const started = backend->registerModelSharedAsync( + binding->typeId, binding->modelFactory, {.contextKey = binding->contextKey, .primary = {}}, + [weakLiveness, weakBinding, onDone](::morph::exec::detail::ModelId newId) { + auto aliveToken = weakLiveness.lock(); + if (!aliveToken) { + return; // The Bridge is gone; publishing this id would be pointless. + } + auto strongBinding = weakBinding.lock(); + if (!strongBinding) { + return; // The BridgeHandler (and its binding) is gone. + } + strongBinding->currentId.store(newId.v); + onDone(nullptr); + }, + [onDone](const std::string& message) { onDone(std::make_exception_ptr(std::runtime_error(message))); }); + if (started) { + return; + } + // No async path on this backend: run the identical synchronous + // registration `ensureBound` would have run, under the same lock, then + // report the outcome only once the lock is gone (see + // `attachHandlerAsync`'s "@par Locking"). + std::exception_ptr failure; + try { + auto newId = backend->registerModelShared(binding->typeId, binding->modelFactory, + {.contextKey = binding->contextKey, .primary = {}}); + binding->currentId.store(newId.v); + } catch (...) { + failure = std::current_exception(); + } + lock.unlock(); + onDone(failure); + } + /// @brief Files @p binding's current instance under @p primary, in place. /// /// The instance keeps everything the creating action just did — nothing is @@ -1543,6 +1678,16 @@ class BridgeHandler { template ::morph::async::Completion::Result> execute(Action action) { using R = ::morph::model::ActionTraits::Result; + // Three mutually exclusive routes, chained rather than sequential: an + // action is payload-keyed or result-keyed or neither (`PayloadKeyed` + // and `ResultKeyed` differ only in `fromResult`, so no action can + // satisfy both), and an unkeyed action — or any action at all on a + // `NoSharing` handler — always lands in the final `else`. Chaining the + // `if constexpr`s (rather than leaving the payload-keyed branch to + // fall through to that `else`, as it did while the attach step was + // synchronous) is what lets the keyed branches own their dispatch: the + // attach now completes asynchronously, so the dispatch it precedes has + // to happen from inside its completion callback, not on this stack. if constexpr (kShared && ::morph::model::detail::PayloadKeyed) { // The action names its instance: attach (or re-point) before // dispatching, so the call lands on the instance it asked for. A @@ -1550,13 +1695,31 @@ class BridgeHandler { // transport error, unauthorized) must surface through the // returned Completion's onError, exactly like every other // dispatch failure — not as a synchronous throw out of execute(). - try { - _bridge.template attachHandler(_binding, ::morph::model::ActionKeyTraits::key(action)); - } catch (...) { - return failedCompletion(std::current_exception()); - } - } - if constexpr (kShared && ::morph::model::detail::ResultKeyed) { + // + // 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 that has not opted in behaves + // exactly as it did before this path existed. + auto state = std::make_shared<::morph::async::detail::CompletionState>(); + ::morph::async::Completion pending{state, _guiExec}; + auto* const bridgePtr = &_bridge; + auto binding = _binding; + auto key = ::morph::model::ActionKeyTraits::key(action); + auto sharedAction = std::make_shared(std::move(action)); + bridgePtr->template attachHandlerAsync( + binding, std::move(key), + [bridgePtr, binding, sharedAction, state, guiExec = _guiExec](std::exception_ptr err) { + if (err) { + state->setException(err); + return; + } + bridgePtr->template executeVia(binding, std::move(*sharedAction), guiExec) + .then([state](R value) { state->setValue(std::move(value)); }) + .onError([state](std::exception_ptr exc) { state->setException(exc); }); + }); + return pending; + } else if constexpr (kShared && ::morph::model::detail::ResultKeyed) { // The action *creates* the instance and its result carries the // generated key, exactly as a database insert returns its primary // key. Adopt it before any user callback observes the result, so a @@ -1566,18 +1729,30 @@ class BridgeHandler { // exists. Give the handler an anonymous instance to run on, then // promote *that* instance once the reply names it — re-pointing to a // fresh one instead would strand whatever the create just did. - try { - _bridge.ensureBound(_binding); - } catch (...) { - return failedCompletion(std::current_exception()); - } + // Same async/fallback contract as the payload-keyed branch above, + // via Bridge::ensureBoundAsync. + auto state = std::make_shared<::morph::async::detail::CompletionState>(); + ::morph::async::Completion pending{state, _guiExec}; auto* const bridgePtr = &_bridge; auto binding = _binding; - return _bridge.template executeVia( - _binding, std::move(action), _guiExec, [bridgePtr, binding](const R& result) { - bridgePtr->template assignHandlerPrimary( - binding, ::morph::model::ActionKeyTraits::template keyOfResult(result)); + auto sharedAction = std::make_shared(std::move(action)); + bridgePtr->ensureBoundAsync( + binding, [bridgePtr, binding, sharedAction, state, guiExec = _guiExec](std::exception_ptr err) { + if (err) { + state->setException(err); + return; + } + bridgePtr + ->template executeVia( + binding, std::move(*sharedAction), guiExec, + [bridgePtr, binding](const R& result) { + bridgePtr->template assignHandlerPrimary( + binding, ::morph::model::ActionKeyTraits::template keyOfResult(result)); + }) + .then([state](R value) { state->setValue(std::move(value)); }) + .onError([state](std::exception_ptr exc) { state->setException(exc); }); }); + return pending; } else { return _bridge.template executeVia(_binding, std::move(action), _guiExec); } @@ -1755,23 +1930,6 @@ class BridgeHandler { [[nodiscard]] const std::shared_ptr& binding() const { return _binding; } private: - /// @brief Builds an already-failed `Completion`, resolved via `.onError(...)`. - /// - /// Used to turn a synchronous exception from the attach/promote step of - /// `execute()` into the same asynchronous failure shape every other - /// dispatch error takes, instead of letting it escape `execute()` as a - /// thrown exception. - /// @tparam R Result type of the action that failed to attach/promote. - /// @param exc Exception to deliver through `.onError(...)`. - /// @return A `Completion` already resolved with @p exc. - template - ::morph::async::Completion failedCompletion(std::exception_ptr exc) { - auto state = std::make_shared<::morph::async::detail::CompletionState>(); - ::morph::async::Completion comp{state, _guiExec}; - state->setException(std::move(exc)); - return comp; - } - Bridge& _bridge; std::weak_ptr _bridgeAlive; // expires when _bridge is destroyed ::morph::exec::IExecutor* _guiExec; diff --git a/include/morph/qt/qt_websocket_backend.hpp b/include/morph/qt/qt_websocket_backend.hpp index e5bdfab5..e50ddae0 100644 --- a/include/morph/qt/qt_websocket_backend.hpp +++ b/include/morph/qt/qt_websocket_backend.hpp @@ -204,6 +204,31 @@ class QtWebSocketBackend : public ::morph::backend::detail::IBackend { const std::string& typeId, std::function()> factory, ::morph::backend::detail::InstanceIdentity identity) override; + /// @brief Sends a shared (register-or-attach) `register` and, if async + /// registration is enabled, returns without blocking. + /// + /// The non-blocking counterpart to `registerModelShared`, matching + /// `registerModelAsync`'s shape exactly (same `callId` counter, same + /// `_pendingRegistrations` map, same verb-agnostic reply routing in + /// `onTextMessage`). An empty `identity.primary` degrades to the private + /// path, i.e. to `registerModelAsync`, mirroring the synchronous + /// `registerModelShared`'s own degrade-to-private behaviour. + /// + /// @param typeId String type-id of the model. + /// @param factory Ignored — model construction is delegated to the server. + /// @param identity Entity key for the action log plus the directory primary key. + /// @param onRegistered Invoked with the assigned `ModelId` on success. + /// @param onError Invoked with a diagnostic message on failure. + /// @return `true` if `asyncRegistrationEnabled` is set (see + /// `QtWebSocketBackendConfig`) and the request was sent; + /// `false` otherwise, falling back to the synchronous + /// `registerModelShared`. + bool registerModelSharedAsync(const std::string& typeId, + std::function()> factory, + ::morph::backend::detail::InstanceIdentity identity, + std::function onRegistered, + std::function onError) override; + /// @brief Sends an `attach` and blocks for the reply, re-pointing from @p current. /// @param typeId String type-id of the model. /// @param factory Ignored — model construction is delegated to the server. @@ -215,6 +240,30 @@ class QtWebSocketBackend : public ::morph::backend::detail::IBackend { const std::string& typeId, std::function()> factory, ::morph::backend::detail::InstanceIdentity identity, ::morph::exec::detail::ModelId current) override; + /// @brief Sends an `attach` and, if async registration is enabled, + /// returns without blocking. + /// + /// The non-blocking counterpart to `attachModel`; see + /// `registerModelSharedAsync` immediately above for the shared shape. An + /// empty `identity.primary` releases @p current and degrades to a private + /// async registration, mirroring the synchronous `attachModel`'s own + /// empty-primary branch. + /// + /// @param typeId String type-id of the model. + /// @param factory Ignored — model construction is delegated to the server. + /// @param identity Entity key for the action log plus the directory primary key. + /// @param current Instance currently held, or `ModelId{0}` if none. + /// @param onRegistered Invoked with the `ModelId` now attached to, on success. + /// @param onError Invoked with a diagnostic message on failure. + /// @return `true` if `asyncRegistrationEnabled` is set and the request + /// was sent; `false` otherwise, falling back to the synchronous + /// `attachModel`. + bool attachModelAsync(const std::string& typeId, + std::function()> factory, + ::morph::backend::detail::InstanceIdentity identity, ::morph::exec::detail::ModelId current, + std::function onRegistered, + std::function onError) override; + /// @brief Files a live server-side instance under @p primary. /// @param mid Live instance to promote. /// @param typeId Model type id. diff --git a/src/qt/qt_websocket_backend.cpp b/src/qt/qt_websocket_backend.cpp index 0fa25abc..f4fd67f3 100644 --- a/src/qt/qt_websocket_backend.cpp +++ b/src/qt/qt_websocket_backend.cpp @@ -211,6 +211,71 @@ void QtWebSocketBackend::flushQueuedRegistrations() { } } +bool QtWebSocketBackend::registerModelSharedAsync( + const std::string& typeId, std::function()> /*factory*/, + ::morph::backend::detail::InstanceIdentity identity, + std::function onRegistered, + std::function onError) { + if (!_cfg.asyncRegistrationEnabled) { + return false; + } + if (identity.primary.empty()) { + // Degrades to the private (non-shared) path, exactly like the + // synchronous registerModelShared below -- and that path already + // has an async form: this class's own registerModelAsync. + return registerModelAsync(typeId, nullptr, identity.contextKey, std::move(onRegistered), std::move(onError)); + } + if (!_connected) { + onError("disconnected"); + return true; + } + uint64_t const callId = ++_nextCallId; + { + std::scoped_lock const lock{_pendingMtx}; + _pendingRegistrations[callId] = + PendingRegistration{.onRegistered = std::move(onRegistered), .onError = std::move(onError)}; + } + auto env = + ::morph::wire::makeRegisterShared(typeId, std::string{identity.primary}, std::string{identity.contextKey}); + env.callId = callId; + _socket.sendTextMessage(QString::fromStdString(::morph::wire::encode(env))); + return true; +} + +bool QtWebSocketBackend::attachModelAsync( + const std::string& typeId, std::function()> /*factory*/, + ::morph::backend::detail::InstanceIdentity identity, ::morph::exec::detail::ModelId current, + std::function onRegistered, + std::function onError) { + if (!_cfg.asyncRegistrationEnabled) { + return false; + } + if (identity.primary.empty()) { + // Mirrors the synchronous attachModel's empty-primary branch: release + // the current instance (fire-and-forget, as deregisterModel already + // is) and degrade to a private async registration. + if (current.v != 0U) { + deregisterModel(current); + } + return registerModelAsync(typeId, nullptr, identity.contextKey, std::move(onRegistered), std::move(onError)); + } + if (!_connected) { + onError("disconnected"); + return true; + } + uint64_t const callId = ++_nextCallId; + { + std::scoped_lock const lock{_pendingMtx}; + _pendingRegistrations[callId] = + PendingRegistration{.onRegistered = std::move(onRegistered), .onError = std::move(onError)}; + } + auto env = + ::morph::wire::makeAttach(typeId, std::string{identity.primary}, current.v, std::string{identity.contextKey}); + env.callId = callId; + _socket.sendTextMessage(QString::fromStdString(::morph::wire::encode(env))); + return true; +} + ::morph::wire::ProtocolNegotiationResult QtWebSocketBackend::negotiateProtocolVersion() { std::string replyJson; try { diff --git a/tests/test_async_registration.cpp b/tests/test_async_registration.cpp index d4bc10b9..62ea723f 100644 --- a/tests/test_async_registration.cpp +++ b/tests/test_async_registration.cpp @@ -28,6 +28,7 @@ #include #include #include +#include #include #include "test_support.hpp" @@ -42,6 +43,42 @@ 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). + +/// Names the instance it wants in the action payload -> payload-keyed, so +/// executing it attaches the handler first (Bridge::attachHandlerAsync). +struct ARTouch { + std::int64_t id = 0; + int amount = 0; +}; + +/// Result of the creating action below; its `id` establishes the key. +struct ARKeyedCreated { + std::int64_t id = 0; + int value = 0; +}; + +/// Creates the entity, so its key can only come back in the reply -> +/// result-keyed, and executing it binds the handler first +/// (Bridge::ensureBoundAsync) and promotes it once the reply names the key. +struct ARKeyedCreate { + int initial = 0; +}; + +struct ARKeyedModel { + int value = 0; + int execute(const ARTouch& act) { + value += act.amount; + return value; + } + ARKeyedCreated execute(const ARKeyedCreate& act) { + value = act.initial; + return {.id = 4242, .value = value}; + } +}; + } // namespace template <> @@ -58,6 +95,38 @@ struct morph::model::ModelTraits { static constexpr std::string_view typeId() { return "AR_Model"; } }; +template <> +struct morph::model::ActionTraits { + using Result = int; + static constexpr std::string_view typeId() { return "AR_Touch"; } + static std::string toJson(const ARTouch& act) { + return R"({"id":)" + std::to_string(act.id) + R"(,"amount":)" + std::to_string(act.amount) + "}"; + } + static ARTouch fromJson(std::string_view /*json*/) { return {}; } + static std::string resultToJson(const int& res) { return std::to_string(res); } + static int resultFromJson(std::string_view text) { return std::stoi(std::string{text}); } +}; +template <> +struct morph::model::ActionTraits { + using Result = ARKeyedCreated; + static constexpr std::string_view typeId() { return "AR_KeyedCreate"; } + static std::string toJson(const ARKeyedCreate& act) { + return R"({"initial":)" + std::to_string(act.initial) + "}"; + } + static ARKeyedCreate fromJson(std::string_view /*json*/) { return {}; } + static std::string resultToJson(const ARKeyedCreated& res) { + return R"({"id":)" + std::to_string(res.id) + R"(,"value":)" + std::to_string(res.value) + "}"; + } + static ARKeyedCreated resultFromJson(std::string_view /*json*/) { return {}; } +}; +template <> +struct morph::model::ModelTraits { + static constexpr std::string_view typeId() { return "AR_KeyedModel"; } +}; + +BRIDGE_MODEL_KEY(ARKeyedModel, ARTouch, &ARTouch::id); +BRIDGE_KEY_FROM_RESULT(ARKeyedCreate, &ARKeyedCreated::id); + // ── Issue #67: assignHandlerPrimary prefers IBackend::assignPrimaryAsync ──── // // A model whose result-keyed action (BRIDGE_KEY_FROM_RESULT) drives @@ -136,6 +205,49 @@ class AsyncRegisterBackend : public morph::backend::detail::IBackend { 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; + } + + 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; + } + + void assignPrimary(morph::exec::detail::ModelId mid, const std::string& /*typeId*/, + std::string_view primary) override { + std::scoped_lock const lock{_regMtx}; + _assigned.emplace_back(mid.v, std::string{primary}); + } + + /// The (modelId, primary) pairs assignPrimary was asked to file, in order. + [[nodiscard]] std::vector> assignments() const { + std::scoped_lock const lock{_regMtx}; + return _assigned; + } + // Test hooks: settle the oldest still-pending async registration. void completeNext() { Pending pending; @@ -175,6 +287,7 @@ class AsyncRegisterBackend : public morph::backend::detail::IBackend { mutable std::mutex _regMtx; std::unordered_map> _models; + std::vector> _assigned; uint64_t _nextId{100}; }; @@ -898,3 +1011,202 @@ TEST_CASE("Bridge::whenBound: concurrent callers racing the exact moment registr REQUIRE(resolvedTrue.load() == kWaitersPerTrial); } } + +// --------------------------------------------------------------------------- +// Shared/keyed registration: registerModelSharedAsync + attachModelAsync. +// +// Same opt-in/fallback contract as registerModelAsync 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 +// is unchanged: the attach/promote step never throws out of the call, it +// resolves the returned Completion. +// --------------------------------------------------------------------------- + +using morph::bridge::AllowShared; + +// NOLINTNEXTLINE(readability-function-cognitive-complexity) +TEST_CASE("Bridge prefers attachModelAsync over the synchronous attachModel when the backend offers it", + "[bridge][registration][shared-instances][issue26]") { + SyncExec cbExec; + auto backend = std::make_unique(); + auto* rawBackend = backend.get(); + morph::bridge::Bridge bridge{std::move(backend)}; + + // An AllowShared handler registers nothing at construction -- it acquires + // an instance only when a keyed action names one. + morph::bridge::BridgeHandler handler{bridge, &cbExec}; + REQUIRE(rawBackend->pendingCount() == 0); + + std::atomic result{-1}; + std::atomic failed{false}; + auto pending = handler.execute(ARTouch{.id = 42, .amount = 5}); + pending.then([&](int val) { result.store(val); }).onError([&](const std::exception_ptr&) { failed.store(true); }); + + // The attach was dispatched but has not replied: execute() returned a + // still-pending Completion rather than blocking in a nested wait, which is + // the entire point on a WASM main thread. + REQUIRE(rawBackend->pendingCount() == 1); + CHECK(result.load() == -1); + CHECK_FALSE(failed.load()); + + rawBackend->completeNext(); + REQUIRE(morph::testing::waitUntil([&] { return result.load() != -1; })); + CHECK(result.load() == 5); + CHECK_FALSE(failed.load()); + CHECK(handler.primary().value_or(-1) == 42); +} + +// NOLINTNEXTLINE(readability-function-cognitive-complexity) +TEST_CASE("A backend with no async attach path falls back to 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. + morph::exec::ThreadPoolExecutor pool{2}; + SyncExec cbExec; + morph::bridge::Bridge bridge{std::make_unique(pool)}; + morph::bridge::BridgeHandler handler{bridge, &cbExec}; + + std::atomic result{-1}; + std::atomic failed{false}; + handler.execute(ARTouch{.id = 7, .amount = 3}) + .then([&](int val) { result.store(val); }) + .onError([&](const std::exception_ptr&) { failed.store(true); }); + + REQUIRE(morph::testing::waitUntil([&] { return result.load() != -1 || failed.load(); })); + CHECK_FALSE(failed.load()); + CHECK(result.load() == 3); + CHECK(handler.primary().value_or(-1) == 7); + + // A second keyed action on the same key is the idempotent-attach path, and + // lands on the same instance (3 + 4), proving the fallback kept the + // binding, not just the first reply. + std::atomic second{-1}; + handler.execute(ARTouch{.id = 7, .amount = 4}) + .then([&](int val) { second.store(val); }) + .onError([&](const std::exception_ptr&) { failed.store(true); }); + REQUIRE(morph::testing::waitUntil([&] { return second.load() != -1 || failed.load(); })); + CHECK_FALSE(failed.load()); + CHECK(second.load() == 7); +} + +// NOLINTNEXTLINE(readability-function-cognitive-complexity) +TEST_CASE( + "attachModelAsync's onError path surfaces through the returned Completion's onError, matching the synchronous " + "path's documented contract", + "[bridge][registration][shared-instances][issue26]") { + SyncExec cbExec; + auto backend = std::make_unique(); + auto* rawBackend = backend.get(); + morph::bridge::Bridge bridge{std::move(backend)}; + morph::bridge::BridgeHandler handler{bridge, &cbExec}; + + // execute() itself must not throw, whatever the attach does -- the failure + // is a Completion outcome, not a synchronous exception. + std::optional> pending; + REQUIRE_NOTHROW(pending.emplace(handler.execute(ARTouch{.id = 99, .amount = 1}))); + + std::string message; + std::atomic succeeded{false}; + pending->then([&](int) { succeeded.store(true); }).onError([&](const std::exception_ptr& err) { + try { + std::rethrow_exception(err); + } catch (const std::exception& exc) { + message = exc.what(); + } + }); + + REQUIRE(rawBackend->pendingCount() == 1); + REQUIRE_NOTHROW(rawBackend->failNext("attach refused")); + + REQUIRE(morph::testing::waitUntil([&] { return !message.empty(); })); + CHECK(message == "attach refused"); + CHECK_FALSE(succeeded.load()); + // The failed attach left the handler unattached, exactly as the + // synchronous path's throwing attach does. + CHECK_FALSE(handler.primary().has_value()); +} + +// 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") { + SyncExec cbExec; + auto backend = std::make_unique(); + auto* rawBackend = backend.get(); + morph::bridge::Bridge bridge{std::move(backend)}; + morph::bridge::BridgeHandler handler{bridge, &cbExec}; + REQUIRE(rawBackend->pendingCount() == 0); + + std::atomic value{-1}; + std::atomic failed{false}; + auto pending = handler.execute(ARKeyedCreate{.initial = 11}); + pending.then([&](ARKeyedCreated res) { value.store(res.value); }).onError([&](const std::exception_ptr&) { + failed.store(true); + }); + + // Bound asynchronously: still nothing resolved, nothing blocked. + REQUIRE(rawBackend->pendingCount() == 1); + CHECK(value.load() == -1); + + rawBackend->completeNext(); + REQUIRE(morph::testing::waitUntil([&] { return value.load() != -1; })); + CHECK(value.load() == 11); + CHECK_FALSE(failed.load()); + // The result-sourced key was adopted in place before the caller's + // .then() saw the result. + CHECK(handler.primary().value_or(-1) == 4242); + auto const assigned = rawBackend->assignments(); + REQUIRE(assigned.size() == 1); + CHECK(assigned.front().second == "4242"); + } + + SECTION("falls back to the synchronous registerModelShared when it does not") { + morph::exec::ThreadPoolExecutor pool{2}; + SyncExec cbExec; + morph::bridge::Bridge bridge{std::make_unique(pool)}; + morph::bridge::BridgeHandler handler{bridge, &cbExec}; + + std::atomic value{-1}; + std::atomic failed{false}; + handler.execute(ARKeyedCreate{.initial = 23}) + .then([&](ARKeyedCreated res) { value.store(res.value); }) + .onError([&](const std::exception_ptr&) { failed.store(true); }); + + REQUIRE(morph::testing::waitUntil([&] { return value.load() != -1 || failed.load(); })); + CHECK_FALSE(failed.load()); + CHECK(value.load() == 23); + CHECK(handler.primary().value_or(-1) == 4242); + } + + SECTION("surfaces registerModelSharedAsync's onError through the returned Completion") { + SyncExec cbExec; + auto backend = std::make_unique(); + auto* rawBackend = backend.get(); + morph::bridge::Bridge bridge{std::move(backend)}; + morph::bridge::BridgeHandler handler{bridge, &cbExec}; + + std::optional> pending; + REQUIRE_NOTHROW(pending.emplace(handler.execute(ARKeyedCreate{.initial = 5}))); + + std::string message; + std::atomic succeeded{false}; + pending->then([&](ARKeyedCreated) { succeeded.store(true); }).onError([&](const std::exception_ptr& err) { + try { + std::rethrow_exception(err); + } catch (const std::exception& exc) { + message = exc.what(); + } + }); + + REQUIRE(rawBackend->pendingCount() == 1); + REQUIRE_NOTHROW(rawBackend->failNext("no capacity")); + + REQUIRE(morph::testing::waitUntil([&] { return !message.empty(); })); + CHECK(message == "no capacity"); + CHECK_FALSE(succeeded.load()); + CHECK_FALSE(handler.primary().has_value()); + } +} From 8502d1e44e860d0ddbbbedd228083848a7993d8b Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Sat, 8 Aug 2026 03:37:41 +0300 Subject: [PATCH 05/20] core: never invoke an async attach's onDone from inside the dispatch frame `Bridge::attachHandlerAsync`/`ensureBoundAsync` promise to release `_attachMtx` before calling `onDone`. That held for every path they control, but not for a backend that completes its callback *inline*, from inside `attachModelAsync`/`registerModelSharedAsync` itself, while the dispatching frame is still holding the lock around the dispatch call -- which is exactly what `QtWebSocketBackend` does on its `!_connected` branch. An inline callback now parks its outcome in a `detail::AsyncDispatchHandoff` and returns without acting; the dispatching frame claims it once the dispatch call has returned, publishes it under the lock it already owns, releases the lock, and only then reports. A small mutex in the handoff makes the window race-free even against a backend that replies from another thread while its dispatch call is still on this stack, and keeps `onDone` invoked exactly once on every interleaving. With the inline case structurally excluded, `attachHandlerAsync`'s out-of-frame success callback can now take `_attachMtx` to publish `HandlerBinding::contextKey`/`primary` -- two plain `std::string`s that five other sites read under that lock, and that this callback previously wrote unsynchronized (a data race, not merely a stale read). `ensureBoundAsync`'s callback publishes only the atomic `currentId` and needs no lock. Also documents the in-flight-attach dedup gap on both methods (concurrent same-key calls before the first reply are not coalesced and can leak one bounded, connection-scoped server-side attach reference), and routes a throwing key extraction in `execute()`'s payload-keyed branch through the returned `Completion` instead of out of the call. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01HffixfknmXdAjnM5hSGRBb --- docs/spec/core/bridge.md | 17 +++ include/morph/core/bridge.hpp | 214 +++++++++++++++++++++++++++--- tests/test_async_registration.cpp | 135 +++++++++++++++++++ 3 files changed, 350 insertions(+), 16 deletions(-) diff --git a/docs/spec/core/bridge.md b/docs/spec/core/bridge.md index e5011418..54cb7061 100644 --- a/docs/spec/core/bridge.md +++ b/docs/spec/core/bridge.md @@ -543,6 +543,23 @@ It is the same rule `registerHandlerImpl` already follows for `_mtx`. See [shared_instances.md](shared_instances.md), "Async register-or-attach and attach". +The guarantee is unconditional, including for a backend that completes its +`attachModelAsync`/`registerModelSharedAsync` callback **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 +`detail::AsyncDispatchHandoff` and returns, and the dispatching frame applies +the outcome once its own dispatch call has returned — publishing under the lock +it already holds, then releasing it, then calling `onDone`. A tiny mutex inside +the handoff makes the window race-free even against a backend that replies from +another thread while its dispatch call is still on this stack, and keeps +`onDone` invoked exactly once on every interleaving. Because the inline case can +never reach the callback body, `attachHandlerAsync`'s out-of-frame success +callback is free to re-acquire `_attachMtx` for the two `std::string` fields it +publishes (`HandlerBinding::contextKey`/`primary`, which every other reader +takes that lock for); `ensureBoundAsync`'s publishes only the atomic +`currentId` and needs no lock at all. + `subscribe`/`unsubscribe` mutate the bridge's subscription registry under `_subMtx`. Callbacks never run under that mutex: `publishResult` snapshots the matching sinks under the lock and invokes them outside it, marshalled to the diff --git a/include/morph/core/bridge.hpp b/include/morph/core/bridge.hpp index 9d79ec82..7175dd93 100644 --- a/include/morph/core/bridge.hpp +++ b/include/morph/core/bridge.hpp @@ -6,6 +6,7 @@ #include #include #include +#include #include #include #include @@ -222,6 +223,93 @@ struct HandlerBinding { std::vector, std::function>> registrationWaiters; }; +/// @brief Outcome a backend's inline completion parked for its dispatcher. +struct ParkedOutcome { + /// @brief `true` when the parked outcome is a success. + bool succeeded = false; + /// @brief Instance id the success callback reported. + ::morph::exec::detail::ModelId modelId{}; + /// @brief Diagnostic the error callback reported; null on success. + std::exception_ptr failure; +}; + +/// @brief Handoff slot between an async attach/bind dispatch and its callback. +/// +/// `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. +/// +/// 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 +/// under the lock it already holds, releases the lock, and only then reports. +/// `mtx` makes the handover race-free even for a backend that replies from +/// another thread *while* its own dispatch call is still on this stack, and the +/// `fired` flag keeps `onDone` invoked exactly once on every interleaving. +struct AsyncDispatchHandoff { + /// @brief Guards every other field; never held across `onDone` or `_attachMtx`. + std::mutex mtx; + /// @brief `true` while the backend's dispatch call is still on the caller's stack. + bool inFrame = true; + /// @brief Set once either callback has claimed the outcome. + bool fired = false; + /// @brief `true` when the parked outcome is a success, `false` for a failure. + bool succeeded = false; + /// @brief Instance id the success callback reported. + ::morph::exec::detail::ModelId modelId{}; + /// @brief Diagnostic the error callback reported; null on success. + std::exception_ptr failure; +}; + +/// @brief Records a backend callback's outcome in @p handoff. +/// +/// Called first thing by both completion callbacks of an async attach/bind +/// dispatch. +/// +/// @param handoff Handoff slot created by the dispatching frame. +/// @param succeeded `true` for the success callback, `false` for the error one. +/// @param modelId Instance id, for the success callback; ignored otherwise. +/// @param failure Diagnostic, for the error callback; null otherwise. +/// @return `true` if the caller must **not** act — either because the dispatch +/// call is still on the dispatcher's stack (which owns the outcome from +/// here on) or because another callback already claimed this dispatch. +/// `false` if the caller owns the outcome and should deliver it itself. +inline bool parkIfInFrame(AsyncDispatchHandoff& handoff, bool succeeded, ::morph::exec::detail::ModelId modelId, + std::exception_ptr failure) { + std::scoped_lock const guard{handoff.mtx}; + if (handoff.fired) { + // A backend is contractually allowed exactly one callback per dispatch; + // swallow a second one rather than reporting twice. + return true; + } + handoff.fired = true; + handoff.succeeded = succeeded; + handoff.modelId = modelId; + handoff.failure = std::move(failure); + return handoff.inFrame; +} + +/// @brief Closes the inline window and takes whatever a callback parked. +/// +/// Called by the dispatching frame immediately after the backend's dispatch call +/// returns. After this, a callback that has not yet run delivers its own outcome. +/// +/// @param handoff Handoff slot created by the dispatching frame. +/// @return The parked outcome if the backend completed inline (or concurrently, +/// before this frame closed the window); `std::nullopt` if the frame won +/// the race and the reply, if any, is still to come. +inline std::optional claimHandoff(AsyncDispatchHandoff& handoff) { + std::scoped_lock const guard{handoff.mtx}; + handoff.inFrame = false; + if (!handoff.fired) { + return std::nullopt; + } + return ParkedOutcome{.succeeded = handoff.succeeded, .modelId = handoff.modelId, .failure = handoff.failure}; +} + } // namespace detail /// @brief Central dispatcher that routes typed actions to an `IBackend`. @@ -372,9 +460,9 @@ class Bridge { /// `_attachMtx` is held around the guard check, the async branch's /// *dispatch*, and the synchronous branch's own state mutation — matching /// `attachHandler`'s existing lock scope — but is **released before - /// @p onDone is ever invoked**, on every path. That is not a nicety: what - /// `execute()` does from inside @p onDone is dispatch the action, and a - /// result-keyed dispatch promotes its binding through + /// @p onDone is ever invoked**, on every path, unconditionally. That is not + /// a nicety: what `execute()` does from inside @p onDone is dispatch the + /// action, and a result-keyed dispatch promotes its binding through /// `assignHandlerPrimary`, which takes `_attachMtx` itself. Invoking /// @p onDone under the lock therefore self-deadlocks the moment the /// completion is delivered on the calling thread — which is exactly what @@ -382,9 +470,35 @@ class Bridge { /// for every callback. This is `registerHandlerImpl`'s existing rule ("the /// backend call must not run under `_mtx`") applied to `_attachMtx`. /// - /// The completion callbacks below likewise do not take `_attachMtx`: per - /// `IBackend::attachModelAsync`'s contract they run once the reply - /// arrives, i.e. after this frame has returned and released it. + /// The guarantee holds even for a backend that completes its callback + /// *inline*, from inside `attachModelAsync` itself, while this frame still + /// holds the lock: such a callback parks its outcome in a + /// `detail::AsyncDispatchHandoff` and returns without acting, and this frame + /// applies it after the dispatch call has returned and the lock is gone. + /// See that struct's doc comment. + /// + /// An out-of-frame success callback re-acquires `_attachMtx` for the two + /// `std::string` fields it publishes (`contextKey`/`primary`, which + /// `HandlerBinding` documents as readable only under that lock) and drops it + /// again before calling @p onDone. That re-acquisition is safe precisely + /// because the inline case never reaches it. + /// + /// @par Known gap + /// Two calls for the same key issued before the first one's reply arrives + /// are **not** deduplicated: the guard below reads `binding->primary`/ + /// `currentId`, neither of which is updated until the reply lands, so both + /// calls pass it and both dispatch an `attach`. This is a real behaviour + /// difference from the synchronous `attachHandler` it replaces, not merely + /// something inherent to asynchrony — `attachHandler` held `_attachMtx` + /// across the whole blocking round trip, which serialised concurrent + /// callers for free. It needs no second thread to hit: two `execute()` + /// calls in one event-loop turn are enough. The server answers both with + /// the same `ModelId` but counts two attachments, so one attach reference + /// leaks; the leak is bounded, not unbounded — the connection scope + /// releases every reference it holds when it closes. Closing this properly + /// needs in-flight tracking on the binding (coalescing the second caller + /// onto the first dispatch's completion); tracked as a follow-up, not fixed + /// here. Same gap, same reasoning, on `ensureBoundAsync`. /// /// @tparam Model Concrete model type. /// @param binding Shared binding, as returned by `registerSharedHandler()`. @@ -406,9 +520,13 @@ class Bridge { auto primaryCopy = primary; std::weak_ptr const weakLiveness{_liveness}; std::weak_ptr const weakBinding{binding}; + auto handoff = std::make_shared(); bool const started = backend->attachModelAsync( binding->typeId, binding->modelFactory, {.contextKey = primaryCopy, .primary = primaryCopy}, previous, - [weakLiveness, weakBinding, primaryCopy, onDone](::morph::exec::detail::ModelId newId) { + [this, weakLiveness, weakBinding, primaryCopy, onDone, handoff](::morph::exec::detail::ModelId newId) { + if (detail::parkIfInFrame(*handoff, true, newId, nullptr)) { + return; // Completed inline: the dispatching frame will finish this. + } auto aliveToken = weakLiveness.lock(); if (!aliveToken) { return; // The Bridge is gone; publishing this id would be pointless. @@ -417,12 +535,37 @@ class Bridge { if (!strongBinding) { return; // The BridgeHandler (and its binding) is gone. } - strongBinding->contextKey = primaryCopy; - strongBinding->primary = primaryCopy; - strongBinding->currentId.store(newId.v); - onDone(nullptr); + { + // contextKey/primary are plain std::strings that five other + // sites read under `_attachMtx`; publishing them without it + // would be a data race, not just a stale read. + std::scoped_lock const guard{_attachMtx}; + strongBinding->contextKey = primaryCopy; + strongBinding->primary = primaryCopy; + strongBinding->currentId.store(newId.v); + } + onDone(nullptr); // Outside the lock -- see @par Locking. }, - [onDone](const std::string& message) { onDone(std::make_exception_ptr(std::runtime_error(message))); }); + [onDone, handoff](const std::string& message) { + auto failure = std::make_exception_ptr(std::runtime_error(message)); + if (detail::parkIfInFrame(*handoff, false, {}, failure)) { + return; + } + onDone(failure); + }); + if (auto parked = detail::claimHandoff(*handoff)) { + // The backend answered on this very stack, with `_attachMtx` still + // held. Publish under the lock we already own, then release it and + // report -- @p onDone never runs inside the dispatch frame. + if (parked->succeeded) { + binding->contextKey = primaryCopy; + binding->primary = std::move(primaryCopy); + binding->currentId.store(parked->modelId.v); + } + lock.unlock(); + onDone(parked->failure); + return; + } if (started) { return; } @@ -461,7 +604,16 @@ class Bridge { } /// @brief Async counterpart to `ensureBound`. See `attachHandlerAsync`'s - /// doc comment for the fallback and locking contract. + /// doc comment for the fallback and locking contract, including the + /// inline-completion handling and the in-flight dedup gap, both of + /// which apply here identically (two result-keyed `execute()` calls + /// on the same still-unbound handler each bind their own anonymous + /// instance; the first is then stranded until the connection scope + /// closes). + /// + /// The one difference: this method's success callback publishes only + /// `currentId`, which is a `std::atomic`, so — unlike `attachHandlerAsync`'s + /// — it needs no `_attachMtx` of its own to do it. /// @param binding Shared binding to bind. /// @param onDone Invoked exactly once: `nullptr` on success, or a /// non-null `exception_ptr` on failure. @@ -476,9 +628,13 @@ class Bridge { auto backend = loadBackend(); std::weak_ptr const weakLiveness{_liveness}; std::weak_ptr const weakBinding{binding}; + auto handoff = std::make_shared(); bool const started = backend->registerModelSharedAsync( binding->typeId, binding->modelFactory, {.contextKey = binding->contextKey, .primary = {}}, - [weakLiveness, weakBinding, onDone](::morph::exec::detail::ModelId newId) { + [weakLiveness, weakBinding, onDone, handoff](::morph::exec::detail::ModelId newId) { + if (detail::parkIfInFrame(*handoff, true, newId, nullptr)) { + return; // Completed inline: the dispatching frame will finish this. + } auto aliveToken = weakLiveness.lock(); if (!aliveToken) { return; // The Bridge is gone; publishing this id would be pointless. @@ -490,7 +646,23 @@ class Bridge { strongBinding->currentId.store(newId.v); onDone(nullptr); }, - [onDone](const std::string& message) { onDone(std::make_exception_ptr(std::runtime_error(message))); }); + [onDone, handoff](const std::string& message) { + auto failure = std::make_exception_ptr(std::runtime_error(message)); + if (detail::parkIfInFrame(*handoff, false, {}, failure)) { + return; + } + onDone(failure); + }); + if (auto parked = detail::claimHandoff(*handoff)) { + // Completed on this stack, under `_attachMtx`: publish here, then + // release the lock before reporting (see `attachHandlerAsync`). + if (parked->succeeded) { + binding->currentId.store(parked->modelId.v); + } + lock.unlock(); + onDone(parked->failure); + return; + } if (started) { return; } @@ -1705,7 +1877,17 @@ class BridgeHandler { ::morph::async::Completion pending{state, _guiExec}; auto* const bridgePtr = &_bridge; auto binding = _binding; - auto key = ::morph::model::ActionKeyTraits::key(action); + // Key extraction is user code (ActionKeyTraits + keyToString), so it + // is inside the same no-throw-out-of-execute() promise the attach + // itself makes: a throw here resolves the Completion, it does not + // escape. + std::string key; + try { + key = ::morph::model::ActionKeyTraits::key(action); + } catch (...) { + state->setException(std::current_exception()); + return pending; + } auto sharedAction = std::make_shared(std::move(action)); bridgePtr->template attachHandlerAsync( binding, std::move(key), diff --git a/tests/test_async_registration.cpp b/tests/test_async_registration.cpp index 62ea723f..934d534d 100644 --- a/tests/test_async_registration.cpp +++ b/tests/test_async_registration.cpp @@ -291,6 +291,56 @@ 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. +class InlineCompletingBackend : public AsyncRegisterBackend { +public: + /// @param failInline When set, both methods report this message via onError + /// 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) { + if (_failInline) { + onError(*_failInline); + return; + } + onRegistered(registerModel(typeId, std::move(factory))); + } + + std::optional _failInline; +}; + // Shim so a Bridge (which takes ownership of a unique_ptr) can hold a backend // the test also keeps a shared_ptr to -- making it co-owned / able to outlive // the Bridge (see test_bridge_lifetime.cpp's identical BackendShim). Also lets @@ -1129,6 +1179,91 @@ TEST_CASE( CHECK_FALSE(handler.primary().has_value()); } +// NOLINTNEXTLINE(readability-function-cognitive-complexity) +TEST_CASE("A backend that completes attachModelAsync 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 + // lock to publish contextKey/primary. A callback that fires inline would + // therefore re-enter a mutex this very frame holds. The dispatch frame must + // park such an outcome and apply it after the lock is released instead. + SyncExec cbExec; + auto backend = std::make_unique(); + morph::bridge::Bridge bridge{std::move(backend)}; + morph::bridge::BridgeHandler handler{bridge, &cbExec}; + + std::atomic result{-1}; + std::atomic failed{false}; + // If the frame deadlocked, execute() never returns and this test hangs. + handler.execute(ARTouch{.id = 8, .amount = 6}) + .then([&](int val) { result.store(val); }) + .onError([&](const std::exception_ptr&) { failed.store(true); }); + + CHECK_FALSE(failed.load()); + CHECK(result.load() == 6); + // The inline outcome was published exactly as an out-of-frame one would be: + // primary() reads binding->primary under _attachMtx, which is also proof + // the lock was released rather than left held by the dispatch frame. + CHECK(handler.primary().value_or(-1) == 8); +} + +// NOLINTNEXTLINE(readability-function-cognitive-complexity) +TEST_CASE("A backend that completes registerModelSharedAsync 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 + // dispatch's onResult calls assignHandlerPrimary, which takes _attachMtx. + SyncExec cbExec; + auto backend = std::make_unique(); + auto* rawBackend = backend.get(); + morph::bridge::Bridge bridge{std::move(backend)}; + morph::bridge::BridgeHandler handler{bridge, &cbExec}; + + std::atomic value{-1}; + std::atomic failed{false}; + handler.execute(ARKeyedCreate{.initial = 17}) + .then([&](ARKeyedCreated res) { value.store(res.value); }) + .onError([&](const std::exception_ptr&) { failed.store(true); }); + + CHECK_FALSE(failed.load()); + CHECK(value.load() == 17); + CHECK(handler.primary().value_or(-1) == 4242); + auto const assigned = rawBackend->assignments(); + REQUIRE(assigned.size() == 1); + CHECK(assigned.front().second == "4242"); +} + +// 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. + SyncExec cbExec; + auto backend = std::make_unique(std::optional{"disconnected"}); + morph::bridge::Bridge bridge{std::move(backend)}; + morph::bridge::BridgeHandler handler{bridge, &cbExec}; + + std::optional> pending; + REQUIRE_NOTHROW(pending.emplace(handler.execute(ARTouch{.id = 3, .amount = 1}))); + + std::string message; + int errorCount = 0; + std::atomic succeeded{false}; + pending->then([&](int) { succeeded.store(true); }).onError([&](const std::exception_ptr& err) { + ++errorCount; + try { + std::rethrow_exception(err); + } catch (const std::exception& exc) { + message = exc.what(); + } + }); + + CHECK(message == "disconnected"); + CHECK(errorCount == 1); + CHECK_FALSE(succeeded.load()); + CHECK_FALSE(handler.primary().has_value()); +} + // NOLINTNEXTLINE(readability-function-cognitive-complexity) TEST_CASE("ensureBoundAsync mirrors the same three cases for a result-keyed (creating) action", "[bridge][registration][shared-instances][issue26]") { From 21a5ca2b08c88371a9a8eb060cc3116ee292360b Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Sat, 8 Aug 2026 11:01:21 +0300 Subject: [PATCH 06/20] core: give TimeoutScheduler a browser-timer build for single-threaded WASM The final whole-branch review's I1, confirmed real by reading the chain it cites: an `EventPoller`'s constructor calling `Bridge::setExecuteDeadline` unconditionally lazily constructs a `TimeoutScheduler` (bridge.hpp), and that constructor spawned a `std::thread` -- while single-threaded Emscripten builds (no `-pthread`) have no working `pthread_create`. Emscripten's non-pthread `pthread_create` stub fails, so libc++ throws `std::system_error` from that constructor on every use. Fixed rather than degraded: under `__EMSCRIPTEN__ && !__EMSCRIPTEN_PTHREADS__` the same public API (`schedule`/`cancel`/`Handle`) is built on `emscripten_async_call` -- the browser's `setTimeout` -- and fires on the main thread. Deadlines still fire; `ClientTimeoutError` still races the real reply. Two documented behavioural differences: callbacks are never concurrent with the caller, and `cancel()` releases the callback immediately but lets the underlying browser timer elapse harmlessly instead of clearing it. Pending timers hold a `weak_ptr` to the scheduler's state, so one that outlives its scheduler returns without touching freed storage. Honest scope: no Emscripten toolchain exists in this repository, so neither the original hazard nor this fix has been observed on a real WASM build -- stated as such in the header and in docs/spec/core/completion.md. What *was* verified locally: the browser branch compiles warning-free under a stubbed `emscripten.h` with `-D__EMSCRIPTEN__`, and against a queued stub it fires once, honours `cancel()`, and drops pending timers safely when the scheduler is destroyed. This cherry-pick from application-ladder drops the original commit's examples/common/gui/event_poller.hpp hunk (the actual EventPoller consumer that motivated this fix) -- that file is ladder-only and doesn't exist on this framework-only branch. Only the general-purpose TimeoutScheduler change and its spec update are included here; the ladder branch retains its own EventPoller fix built on top of this. Co-Authored-By: Claude Sonnet 5 --- docs/spec/core/completion.md | 29 +++- include/morph/core/timeout_scheduler.hpp | 181 ++++++++++++++++++++++- 2 files changed, 202 insertions(+), 8 deletions(-) diff --git a/docs/spec/core/completion.md b/docs/spec/core/completion.md index ce08dcf9..d1a0d6c3 100644 --- a/docs/spec/core/completion.md +++ b/docs/spec/core/completion.md @@ -240,12 +240,33 @@ pre-existing behavior exactly — a `Bridge` that never calls the setter behaves as it always did, and spawns no extra thread. The current value is readable via `Bridge::executeDeadline()`. +**Single-threaded WebAssembly.** `TimeoutScheduler` has a second build, +selected by `#if defined(__EMSCRIPTEN__) && !defined(__EMSCRIPTEN_PTHREADS__)`, +that uses the browser's own `setTimeout` (`emscripten_async_call`) instead of a +thread and fires its callbacks on the main thread — the same thread the Qt +event loop and every `QtExecutor`-posted completion callback already run on. +This is not a degradation switch: deadlines still fire, with the same +first-result-wins race and the same `ClientTimeoutError`. It exists because a +`wasm_singlethread` Qt build (what `.github/workflows/wasm-ladder.yml` installs +and what `cmake/morph_add_rung.cmake` builds against, with no `-pthread`) links +Emscripten's non-pthread `pthread_create` stub, so constructing a `std::thread` +throws `std::system_error` at runtime — which would have made +`setExecuteDeadline` unusable from a browser tab, and with it +`examples/common/gui/event_poller.hpp`, whose constructor calls it +unconditionally. Two behavioural differences, both documented in +`timeout_scheduler.hpp`'s own `@file` comment: callbacks are never concurrent +with the caller, and `cancel()` releases the callback immediately but leaves the +underlying browser timer to elapse harmlessly rather than clearing it. **This +build has never been compiled or run in this repository** — no Emscripten +toolchain is available here; its only verification is the `ladder-wasm` CI +compile gate. + **Mechanics.** Every `executeVia()` call made while a non-zero deadline is installed arms a timer on a `Bridge`-owned -`morph::async::detail::TimeoutScheduler` (a single background thread, created -lazily on the first call that enables a deadline and torn down with the -`Bridge`; the same class `RemoteServer` uses for its server-side -`LimitPolicy::executeTimeout`). The timer's callback captures only the typed +`morph::async::detail::TimeoutScheduler` (a single background thread — or, in a +single-threaded WASM build, a browser timer; see above — created lazily on the +first call that enables a deadline and torn down with the `Bridge`; the same +class `RemoteServer` uses for its server-side `LimitPolicy::executeTimeout`). The timer's callback captures only the typed `CompletionState` — never the `Bridge` — and resolves it with `morph::backend::ClientTimeoutError`. The real reply and the timer therefore race, and **whichever settles the state first wins**, because `setValue` / diff --git a/include/morph/core/timeout_scheduler.hpp b/include/morph/core/timeout_scheduler.hpp index 8f8c8e0e..cb18cea2 100644 --- a/include/morph/core/timeout_scheduler.hpp +++ b/include/morph/core/timeout_scheduler.hpp @@ -2,20 +2,78 @@ #pragma once #include -#include #include #include #include +#include +#include + +/// @file +/// `TimeoutScheduler` — "run this callback once, in N milliseconds, unless +/// cancelled first" — in two builds of the same public API. +/// +/// @par Why two builds +/// The ordinary build owns a dedicated `std::thread`. A **single-threaded +/// Emscripten** build cannot: Qt for WebAssembly is installed here as +/// `wasm_singlethread` (`.github/workflows/wasm-ladder.yml`) and +/// `cmake/morph_add_rung.cmake` passes no `-pthread`, so Emscripten's +/// non-pthread `pthread_create` stub fails and `std::thread`'s constructor +/// throws `std::system_error` ("thread constructor failed") — from inside +/// whatever completion callback happened to enable the deadline. Every WASM +/// client in this repository is Qt-event-loop driven and would hit this the +/// moment it called `Bridge::setExecuteDeadline` (which +/// `examples/common/gui/event_poller.hpp`'s constructor does +/// unconditionally, on every poll open). +/// +/// So under `__EMSCRIPTEN__` without `__EMSCRIPTEN_PTHREADS__` this class is +/// built on `emscripten_async_call` — the browser's own `setTimeout` — and +/// fires its callbacks on the single main thread, i.e. on the same thread the +/// Qt event loop and every `QtExecutor`-posted completion callback already +/// run on. Deadlines still fire; nothing is silently disabled. +/// +/// @par What differs between the two builds +/// - **Callback thread.** Threaded build: a private background thread, so a +/// callback must be prepared to run concurrently with the caller (the one +/// real callback in this codebase, `executeVia`'s, only touches a +/// `CompletionState`, which is itself mutex-guarded). Browser build: the +/// main thread, never concurrently with anything. +/// - **Cancellation.** Threaded build: the entry, its callback and everything +/// the callback captured are erased immediately. Browser build: identical +/// for the callback and its captures (the map entry is erased at once), but +/// the underlying browser timer is not itself cleared — it still fires at +/// its original deadline and finds nothing to do. Only a small ticket +/// allocation outlives `cancel()`, until that point. +/// - **Destruction.** Threaded build: the destructor joins its thread, so no +/// callback can be in flight afterwards. Browser build: nothing to join; +/// pending browser timers observe an expired `std::weak_ptr` to the +/// scheduler's state and return without invoking anything. +/// +/// @warning The browser build has never been compiled or run in this +/// repository — no Emscripten toolchain is available where it was written. +/// Its only verification is the `ladder-wasm` CI compile gate. Stated plainly +/// here rather than smoothed over, exactly like `examples/TESTING.md`'s note +/// on the WASM clients themselves. + +#if defined(__EMSCRIPTEN__) && !defined(__EMSCRIPTEN_PTHREADS__) +#define MORPH_TIMEOUT_SCHEDULER_BROWSER_TIMERS 1 +#include + +#include +#include +#include +#else +#include #include #include -#include #include -#include +#endif #include "logger.hpp" namespace morph::async::detail { +#ifndef MORPH_TIMEOUT_SCHEDULER_BROWSER_TIMERS + /// @brief Background scheduler that invokes a callback once after a delay, unless cancelled first. /// /// Neither `Bridge` nor `RemoteServer` is bound to a specific `IExecutor` @@ -23,7 +81,8 @@ namespace morph::async::detail { /// tracks pending deadlines and fires callbacks when they elapse. Used by /// `RemoteServer` to enforce `LimitPolicy::executeTimeout` (server-side — /// see `docs/spec/core/backend.md`) and by `Bridge::setExecuteDeadline` -/// (client-side — see `docs/spec/core/completion.md`). +/// (client-side — see `docs/spec/core/completion.md`). See this file's `@file` +/// comment for the single-threaded-WASM build of the same API. class TimeoutScheduler { public: /// @brief Opaque identifier for one scheduled callback. @@ -127,4 +186,118 @@ class TimeoutScheduler { std::thread _thread; }; +#else + +/// @brief Single-threaded-Emscripten build of the same API, backed by the +/// browser's `setTimeout` (`emscripten_async_call`) instead of a +/// thread. See this file's `@file` comment for why it exists and +/// exactly how its behaviour differs. +class TimeoutScheduler { +public: + /// @brief Opaque identifier for one scheduled callback. + using Handle = std::uint64_t; + + /// @brief Creates the scheduler. Starts no thread — there is none to start. + TimeoutScheduler() = default; + + /// @brief Drops every still-pending callback without firing it. + /// + /// Browser timers already queued outlive this object; each holds only a + /// `std::weak_ptr` to `_state` and returns immediately once it expires, + /// which is precisely at this destructor. Matches the threaded build's + /// "`~TimeoutScheduler` drops pending entries without firing them". + ~TimeoutScheduler() = default; + + TimeoutScheduler(const TimeoutScheduler&) = delete; + TimeoutScheduler& operator=(const TimeoutScheduler&) = delete; + TimeoutScheduler(TimeoutScheduler&&) = delete; + TimeoutScheduler& operator=(TimeoutScheduler&&) = delete; + + /// @brief Schedules @p callback to run after @p delay on the main + /// (browser) thread, unless cancelled first via `cancel()`. + /// @param delay Time to wait before firing. + /// @param callback Invoked on the main thread if not cancelled in time. + /// Exceptions it throws are logged and swallowed. + /// @return Handle usable with `cancel()`. + Handle schedule(std::chrono::milliseconds delay, std::function callback) { + Handle const handle = ++_state->nextHandle; + _state->pending.emplace(handle, std::move(callback)); + // Owned by the browser timer, deleted by `fire` below whether or not + // the entry is still live by then. A raw `new` rather than a + // `unique_ptr` because the ownership genuinely crosses a C callback + // boundary that cannot carry a smart pointer. + auto* ticket = new Ticket{_state, handle}; + ::emscripten_async_call(&TimeoutScheduler::fire, ticket, clampMillis(delay)); + return handle; + } + + /// @brief Cancels a previously scheduled callback immediately. + /// + /// If @p handle has not fired yet, its callback (and anything that + /// callback captured) is released right away, exactly like the threaded + /// build. The browser timer itself is left to elapse and find nothing — + /// see the `@file` comment. A no-op if @p handle already fired or was + /// already cancelled. + /// @param handle Handle returned by a prior `schedule()` call. + void cancel(Handle handle) { _state->pending.erase(handle); } + +private: + struct State { + std::unordered_map> pending; + Handle nextHandle{0}; + }; + + struct Ticket { + std::weak_ptr state; + Handle handle; + }; + + /// @brief @p delay as the `int` milliseconds `emscripten_async_call` + /// takes, saturating rather than wrapping (a `std::chrono` + /// duration can hold far more than an `int` can). + /// @param delay The requested delay. + /// @return A non-negative millisecond count that fits in an `int`. + [[nodiscard]] static int clampMillis(std::chrono::milliseconds delay) noexcept { + auto const count = delay.count(); + if (count <= 0) { + return 0; + } + if (count > static_cast(std::numeric_limits::max())) { + return std::numeric_limits::max(); + } + return static_cast(count); + } + + /// @brief The C callback the browser timer invokes. + /// @param arg The `Ticket*` handed to `emscripten_async_call`; always + /// deleted here, whether or not its entry is still live. + static void fire(void* arg) { + std::unique_ptr const ticket{static_cast(arg)}; + auto state = ticket->state.lock(); + if (!state) { + return; + } + auto found = state->pending.find(ticket->handle); + if (found == state->pending.end()) { + return; // cancelled before this timer elapsed + } + std::function callback = std::move(found->second); + state->pending.erase(found); + try { + callback(); + } catch (const std::exception& exc) { + ::morph::log::logError("[timeout-scheduler] callback threw: " + std::string{exc.what()}); + } catch (...) { + ::morph::log::logError("[timeout-scheduler] callback threw unknown exception"); + } + } + + /// @brief Held by `shared_ptr` so a browser timer that outlives this + /// object detects that fact instead of writing to freed storage — + /// the same weak-token pattern as `morph::bridge::Bridge::_liveness`. + std::shared_ptr _state{std::make_shared()}; +}; + +#endif + } // namespace morph::async::detail From ee04da76e3d450fb77fc3d0c8daea59fec6ee741 Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Mon, 10 Aug 2026 20:53:19 +0300 Subject: [PATCH 07/20] core: close a switchBackend staleness race and two exception-safety gaps in async keyed/shared attach Findings from a /code-review max pass over the application-ladder branch, fixed one round: attachHandlerAsync's and ensureBoundAsync's out-of-frame success callbacks published an async reply into HandlerBinding state (currentId/primary/ contextKey) with no check that the backend which produced it was still Bridge's active backend. registerHandlerImpl already has the right guard (weakBackend + pinned != loadBackend()) for its own synchronous-registration callback; the two async-attach paths added later never got the same treatment. A switchBackend() racing an in-flight attach could silently overwrite a binding with an id only meaningful on the backend nothing uses any more. Fixed by applying the identical guard to both callbacks -- but unlike registerHandlerImpl's fire-and-forget re-registration, a real execute() call is synchronously waiting on attachHandlerAsync/ ensureBoundAsync's onDone, so a stale reply now reports failure through it instead of being silently dropped (which would otherwise hang the caller forever). Two new regression tests in test_async_registration.cpp exercise this against attachHandlerAsync (empirically confirmed to fail without the fix: the stale reply corrupted handler.primary() and the subsequent action against the new backend never re-dispatched) and ensureBoundAsync. The backend dispatch call itself (attachModelAsync/registerModelSharedAsync) had no try/catch, unlike the synchronous fallback a few lines below -- QtWebSocketBackend's real implementation calls wire::encode() directly, documented to throw on serialization failure, which would otherwise escape BridgeHandler::execute() as a raw exception and break its documented never-throws contract. Fixed with a try/catch converting the exception into onDone's failure path. Separately, those same three QtWebSocketBackend methods (registerModelAsync/registerModelSharedAsync/attachModelAsync) inserted their pending-registration bookkeeping *before* calling encode(), so a throw there would orphan that entry forever, waiting for a reply to a message never sent -- fixed by encoding first and only inserting once encoding succeeds. attachHandlerAsync's success-callback string writes (contextKey/primary) were unguarded too, unlike the synchronous fallback that wraps the same writes so onDone still fires on a throwing assignment. Fixed the same way, in both the out-of-frame callback and the inline-claimed-outcome branch. Bridge::executeVia's client-side-deadline cancel() call was guarded only by a check-then-use `!alive.expired()` test -- proving Bridge's own liveness token hadn't been destroyed at that instant, not that ~Bridge() (which can run concurrently on another thread per docs/spec/core/bridge.md) wouldn't complete ~TimeoutScheduler()'s thread-join between the check and the cancel() call a few instructions later. ~Bridge()'s own body never acquires _executeDeadlineMtx, so that lock doesn't close the gap either. Fixed by changing _timeoutScheduler from unique_ptr to shared_ptr and having the completion callbacks hold their own shared_ptr copy (captured under the same lock as schedule()), so cancel() is always called on a scheduler that is provably still alive by construction, not by timing luck; also wrapped the call in try/catch, matching the pattern already used for the value- forwarding code beside it. timeout_scheduler.hpp: documented (not "fixed" -- emscripten_async_call's int parameter is a hard platform constraint) that the WASM build's delay saturates at ~24.85 days where the threaded build honors the full std::chrono::milliseconds range. Verified: full ladder 311/311 (was 307 before this branch's own unrelated test additions), morph_tests 868/868, morph_qt_tests 63/63 (one transient waitForConnected(2000) flake observed and reproduced as pre-existing/ unrelated -- 4/5 clean reruns, a real-network reconnect test untouched by any of these changes). Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01HffixfknmXdAjnM5hSGRBb --- include/morph/core/bridge.hpp | 275 ++++++++++++++++------- include/morph/core/timeout_scheduler.hpp | 9 + src/qt/qt_websocket_backend.cpp | 38 +++- tests/test_async_registration.cpp | 103 +++++++++ 4 files changed, 330 insertions(+), 95 deletions(-) diff --git a/include/morph/core/bridge.hpp b/include/morph/core/bridge.hpp index 7175dd93..6f392a73 100644 --- a/include/morph/core/bridge.hpp +++ b/include/morph/core/bridge.hpp @@ -518,52 +518,96 @@ class Bridge { auto const previous = ::morph::exec::detail::ModelId{binding->currentId.load()}; auto backend = loadBackend(); auto primaryCopy = primary; + std::weak_ptr<::morph::backend::detail::IBackend> const weakBackend{backend}; std::weak_ptr const weakLiveness{_liveness}; std::weak_ptr const weakBinding{binding}; auto handoff = std::make_shared(); - bool const started = backend->attachModelAsync( - binding->typeId, binding->modelFactory, {.contextKey = primaryCopy, .primary = primaryCopy}, previous, - [this, weakLiveness, weakBinding, primaryCopy, onDone, handoff](::morph::exec::detail::ModelId newId) { - if (detail::parkIfInFrame(*handoff, true, newId, nullptr)) { - return; // Completed inline: the dispatching frame will finish this. - } - auto aliveToken = weakLiveness.lock(); - if (!aliveToken) { - return; // The Bridge is gone; publishing this id would be pointless. - } - auto strongBinding = weakBinding.lock(); - if (!strongBinding) { - return; // The BridgeHandler (and its binding) is gone. - } - { - // contextKey/primary are plain std::strings that five other - // sites read under `_attachMtx`; publishing them without it - // would be a data race, not just a stale read. - std::scoped_lock const guard{_attachMtx}; - strongBinding->contextKey = primaryCopy; - strongBinding->primary = primaryCopy; - strongBinding->currentId.store(newId.v); - } - onDone(nullptr); // Outside the lock -- see @par Locking. - }, - [onDone, handoff](const std::string& message) { - auto failure = std::make_exception_ptr(std::runtime_error(message)); - if (detail::parkIfInFrame(*handoff, false, {}, failure)) { - return; - } - onDone(failure); - }); + bool started = false; + try { + started = backend->attachModelAsync( + binding->typeId, binding->modelFactory, {.contextKey = primaryCopy, .primary = primaryCopy}, + previous, + [this, weakBackend, weakLiveness, weakBinding, primaryCopy, + onDone, handoff](::morph::exec::detail::ModelId newId) { + if (detail::parkIfInFrame(*handoff, true, newId, nullptr)) { + return; // Completed inline: the dispatching frame will finish this. + } + auto aliveToken = weakLiveness.lock(); + if (!aliveToken) { + return; // The Bridge is gone; publishing this id would be pointless. + } + auto strongBinding = weakBinding.lock(); + if (!strongBinding) { + return; // The BridgeHandler (and its binding) is gone. + } + std::exception_ptr failure; + { + // contextKey/primary are plain std::strings that five + // other sites read under `_attachMtx`; publishing them + // without it would be a data race, not just a stale + // read. + std::scoped_lock const guard{_attachMtx}; + auto pinned = weakBackend.lock(); + if (!pinned || pinned != loadBackend()) { + // A switchBackend() already moved past this attach + // (see registerHandlerImpl's identical guard) and + // its own re-registration loop already handled + // `binding` on the *new* backend -- applying this + // stale reply now would overwrite that with a + // dangling id from a backend nothing uses any + // more. Unlike registerHandlerImpl's fire-and- + // forget re-registration, a real execute() call is + // synchronously waiting on `onDone` here, so the + // stale reply must still be reported -- silently + // dropping it would hang that caller forever. + failure = std::make_exception_ptr(std::runtime_error( + "attach reply arrived from a backend switchBackend() already replaced")); + } else { + try { + strongBinding->contextKey = primaryCopy; + strongBinding->primary = primaryCopy; + strongBinding->currentId.store(newId.v); + } catch (...) { + failure = std::current_exception(); + } + } + } + onDone(failure); // Outside the lock -- see @par Locking. + }, + [onDone, handoff](const std::string& message) { + auto failure = std::make_exception_ptr(std::runtime_error(message)); + if (detail::parkIfInFrame(*handoff, false, {}, failure)) { + return; + } + onDone(failure); + }); + } catch (...) { + // The backend's own dispatch call can throw synchronously (e.g. + // QtWebSocketBackend::attachModelAsync's wire::encode() failing + // before send) -- report it like any other failure instead of + // letting it escape execute()'s documented never-throws contract. + lock.unlock(); + onDone(std::current_exception()); + return; + } if (auto parked = detail::claimHandoff(*handoff)) { // The backend answered on this very stack, with `_attachMtx` still - // held. Publish under the lock we already own, then release it and - // report -- @p onDone never runs inside the dispatch frame. + // held -- switchBackend() cannot have run concurrently (it takes + // the same lock), so no staleness check is needed here. Publish + // under the lock we already own, then release it and report -- + // @p onDone never runs inside the dispatch frame. + std::exception_ptr failure = parked->failure; if (parked->succeeded) { - binding->contextKey = primaryCopy; - binding->primary = std::move(primaryCopy); - binding->currentId.store(parked->modelId.v); + try { + binding->contextKey = primaryCopy; + binding->primary = std::move(primaryCopy); + binding->currentId.store(parked->modelId.v); + } catch (...) { + failure = std::current_exception(); + } } lock.unlock(); - onDone(parked->failure); + onDone(failure); return; } if (started) { @@ -626,33 +670,63 @@ class Bridge { return; } auto backend = loadBackend(); + std::weak_ptr<::morph::backend::detail::IBackend> const weakBackend{backend}; std::weak_ptr const weakLiveness{_liveness}; std::weak_ptr const weakBinding{binding}; auto handoff = std::make_shared(); - bool const started = backend->registerModelSharedAsync( - binding->typeId, binding->modelFactory, {.contextKey = binding->contextKey, .primary = {}}, - [weakLiveness, weakBinding, onDone, handoff](::morph::exec::detail::ModelId newId) { - if (detail::parkIfInFrame(*handoff, true, newId, nullptr)) { - return; // Completed inline: the dispatching frame will finish this. - } - auto aliveToken = weakLiveness.lock(); - if (!aliveToken) { - return; // The Bridge is gone; publishing this id would be pointless. - } - auto strongBinding = weakBinding.lock(); - if (!strongBinding) { - return; // The BridgeHandler (and its binding) is gone. - } - strongBinding->currentId.store(newId.v); - onDone(nullptr); - }, - [onDone, handoff](const std::string& message) { - auto failure = std::make_exception_ptr(std::runtime_error(message)); - if (detail::parkIfInFrame(*handoff, false, {}, failure)) { - return; - } - onDone(failure); - }); + bool started = false; + try { + started = backend->registerModelSharedAsync( + binding->typeId, binding->modelFactory, {.contextKey = binding->contextKey, .primary = {}}, + [this, weakBackend, weakLiveness, weakBinding, onDone, + handoff](::morph::exec::detail::ModelId newId) { + if (detail::parkIfInFrame(*handoff, true, newId, nullptr)) { + return; // Completed inline: the dispatching frame will finish this. + } + auto aliveToken = weakLiveness.lock(); + if (!aliveToken) { + return; // The Bridge is gone; publishing this id would be pointless. + } + auto strongBinding = weakBinding.lock(); + if (!strongBinding) { + return; // The BridgeHandler (and its binding) is gone. + } + std::exception_ptr failure; + { + // Brief `_attachMtx` window purely to serialise this + // check against a concurrent switchBackend() (which + // takes the same lock) -- `currentId` itself is an + // atomic and needs no lock to store. + std::scoped_lock const guard{_attachMtx}; + auto pinned = weakBackend.lock(); + if (!pinned || pinned != loadBackend()) { + // See attachHandlerAsync's identical guard: a + // stale reply from a backend switchBackend() + // already replaced must still resolve `onDone` + // (a real execute() call is waiting), not be + // silently dropped. + failure = std::make_exception_ptr(std::runtime_error( + "attach reply arrived from a backend switchBackend() already replaced")); + } else { + strongBinding->currentId.store(newId.v); + } + } + onDone(failure); + }, + [onDone, handoff](const std::string& message) { + auto failure = std::make_exception_ptr(std::runtime_error(message)); + if (detail::parkIfInFrame(*handoff, false, {}, failure)) { + return; + } + onDone(failure); + }); + } catch (...) { + // See attachHandlerAsync's identical guard: the backend's own + // dispatch call can throw synchronously before send. + lock.unlock(); + onDone(std::current_exception()); + return; + } if (auto parked = detail::claimHandoff(*handoff)) { // Completed on this stack, under `_attachMtx`: publish here, then // release the lock before reporting (see `attachHandlerAsync`). @@ -1028,7 +1102,7 @@ class Bridge { std::scoped_lock const lock{_executeDeadlineMtx}; _executeDeadline = deadline; if (_executeDeadline.count() > 0 && !_timeoutScheduler) { - _timeoutScheduler = std::make_unique<::morph::async::detail::TimeoutScheduler>(); + _timeoutScheduler = std::make_shared<::morph::async::detail::TimeoutScheduler>(); } } @@ -1303,12 +1377,28 @@ class Bridge { // it happen under one lock so a concurrent setExecuteDeadline() cannot // interleave between the two. std::optional<::morph::async::detail::TimeoutScheduler::Handle> deadlineHandle; + // A private shared_ptr copy, obtained under the same lock as the + // schedule() call -- not `this->_timeoutScheduler`, and not gated on + // `alive` -- so the two `.then()`/`.onError()` callbacks below can + // call `cancel()` on a scheduler that is provably still alive, + // regardless of whether ~Bridge() has run or is running concurrently + // on another thread. `_executeDeadlineMtx` alone does not establish + // that: ~Bridge()'s own body never acquires it, so a plain + // `!alive.expired()` check followed by `_timeoutScheduler->cancel()` + // a few instructions later is a check-then-use race against + // ~Bridge()'s implicit member destruction (which joins + // TimeoutScheduler's thread). Holding a shared_ptr for the + // callback's own lifetime turns that into a non-issue by + // construction: while any copy of it is alive, ~TimeoutScheduler() + // cannot run at all. + std::shared_ptr<::morph::async::detail::TimeoutScheduler> schedulerRef; { std::scoped_lock const lock{_executeDeadlineMtx}; if (_executeDeadline.count() > 0 && _timeoutScheduler) { + schedulerRef = _timeoutScheduler; // The callback captures `typedState` alone -- never `this` -- so // it stays safe to fire even while ~Bridge() is running. - deadlineHandle = _timeoutScheduler->schedule(_executeDeadline, [typedState] { + deadlineHandle = schedulerRef->schedule(_executeDeadline, [typedState] { typedState->setException(std::make_exception_ptr(::morph::backend::ClientTimeoutError{})); }); } @@ -1418,21 +1508,32 @@ class Bridge { } auto anyCompletion = backend->execute(::morph::exec::detail::ModelId{raw}, std::move(call), cbExec); anyCompletion - .then([typedState, onResult = std::move(onResult), this, raw, deadlineHandle, + .then([typedState, onResult = std::move(onResult), this, raw, deadlineHandle, schedulerRef, alive = liveness()](const std::shared_ptr& vAny) { // Disarm the client-side deadline first, before any of the // forwarding work below: a slow onResult/publishResult callback // must not give the timer a window to fire concurrently and // resolve this completion with ClientTimeoutError while the real - // result is already in hand. Guarded on the same liveness token - // the rest of this lambda uses -- `_timeoutScheduler` and - // `_executeDeadlineMtx` are Bridge members, and this callback can - // in principle run after ~Bridge(). Leaving the entry armed in - // that case is harmless: ~TimeoutScheduler drops pending entries - // without firing them. - if (deadlineHandle && !alive.expired()) { - std::scoped_lock const lock{_executeDeadlineMtx}; - _timeoutScheduler->cancel(*deadlineHandle); + // result is already in hand. Uses the `schedulerRef` copy + // captured above, not `this->_timeoutScheduler` -- see that + // capture's own comment for why: this callback can in principle + // run after ~Bridge(), and `schedulerRef` (not `alive`) is what + // makes `cancel()` safe in that case, by keeping the scheduler + // alive for exactly as long as this callback needs it, not by + // racing a liveness check against ~Bridge()'s teardown. Leaving + // the entry armed if it were never cancelled would be harmless + // (~TimeoutScheduler drops pending entries without firing them), + // but a thrown cancel() must not prevent the real result from + // resolving the completion below either. + if (deadlineHandle && schedulerRef) { + try { + schedulerRef->cancel(*deadlineHandle); + } catch (...) { + // Best-effort: a failed cancel leaves the deadline's own + // entry to fire later and find nothing (setValue/ + // setException below are idempotent), which is exactly + // what an uncancelled entry already does. + } } // Guard the value-forwarding: if R's move/copy throws (or the cast // is somehow wrong), route the exception to the typed completion's @@ -1487,19 +1588,27 @@ class Bridge { typedState->setException(std::current_exception()); } }) - .onError([typedState, this, deadlineHandle, alive = liveness()](const std::exception_ptr& err) { + .onError([typedState, this, deadlineHandle, schedulerRef, + alive = liveness()](const std::exception_ptr& err) { + // Same disarm-first reasoning (and the same schedulerRef-based + // safety, not a liveness-then-use race) as the success branch + // above: a real error reply settles the completion, so the + // deadline must not also fire. + if (deadlineHandle && schedulerRef) { + try { + schedulerRef->cancel(*deadlineHandle); + } catch (...) { + // Best-effort: a failed cancel leaves the deadline's own + // entry to fire later and find nothing (setValue/ + // setException below are idempotent), which is exactly + // what an uncancelled entry already does. + } + } // The other of the two mutually-exclusive resolution paths -- // see the .then continuation above. Same liveness guard: this // touches `this` and must not run once the Bridge might be gone. - // Same disarm-first reasoning as the success branch above: a - // real error reply settles the completion, so the deadline - // must not also fire. if (!alive.expired()) { this->_pendingCalls.fetch_sub(1, std::memory_order_relaxed); - if (deadlineHandle) { - std::scoped_lock const lock{_executeDeadlineMtx}; - _timeoutScheduler->cancel(*deadlineHandle); - } } typedState->setException(err); }); @@ -1722,7 +1831,7 @@ class Bridge { // these has already expired by the time they are torn down. mutable std::mutex _executeDeadlineMtx; std::chrono::milliseconds _executeDeadline{0}; - std::unique_ptr<::morph::async::detail::TimeoutScheduler> _timeoutScheduler; + std::shared_ptr<::morph::async::detail::TimeoutScheduler> _timeoutScheduler; // Instance subscriptions. Held against the binding rather than a fixed // instance id so a re-pointed handler keeps its subscriptions; matched at // publish time by comparing the binding's current instance. diff --git a/include/morph/core/timeout_scheduler.hpp b/include/morph/core/timeout_scheduler.hpp index cb18cea2..35cef6d9 100644 --- a/include/morph/core/timeout_scheduler.hpp +++ b/include/morph/core/timeout_scheduler.hpp @@ -255,6 +255,15 @@ class TimeoutScheduler { /// @brief @p delay as the `int` milliseconds `emscripten_async_call` /// takes, saturating rather than wrapping (a `std::chrono` /// duration can hold far more than an `int` can). + /// + /// @note This is a real, documented behavioural asymmetry from the + /// threaded build, which honours the full `std::chrono::milliseconds` + /// range unconditionally: a delay beyond `INT_MAX` ms (~24.85 days) fires + /// at ~24.85 days here instead of at its true, much later requested time. + /// `emscripten_async_call`'s `int` parameter is a hard platform + /// constraint with no larger-range alternative to fall back to, so this + /// is accepted rather than worked around. No caller in this codebase + /// currently requests a deadline anywhere near that range. /// @param delay The requested delay. /// @return A non-negative millisecond count that fits in an `int`. [[nodiscard]] static int clampMillis(std::chrono::milliseconds delay) noexcept { diff --git a/src/qt/qt_websocket_backend.cpp b/src/qt/qt_websocket_backend.cpp index f4fd67f3..850222e5 100644 --- a/src/qt/qt_websocket_backend.cpp +++ b/src/qt/qt_websocket_backend.cpp @@ -190,14 +190,22 @@ void QtWebSocketBackend::sendRegisterAsync(const std::string& typeId, std::strin std::function onRegistered, std::function onError) { uint64_t const callId = ++_nextCallId; + auto env = ::morph::wire::makeRegister(typeId, std::string{contextKey}); + env.callId = callId; + env.session = _session; + // Encoded before the map insertion below: wire::encode() can throw on + // serialization failure, and a throw after inserting would leave this + // callId's onRegistered/onError parked in _pendingRegistrations forever, + // waiting for a reply to a message that was never sent -- nothing erases + // an entry whose send never happened. Encoding first means a throw here + // propagates to the caller (Bridge::registerHandlerImpl et al. already + // handle it) with nothing to clean up. + auto const encoded = QString::fromStdString(::morph::wire::encode(env)); { std::scoped_lock const lock{_pendingMtx}; _pendingRegistrations[callId] = PendingRegistration{std::move(onRegistered), std::move(onError)}; } - auto env = ::morph::wire::makeRegister(typeId, std::string{contextKey}); - env.callId = callId; - env.session = _session; - _socket.sendTextMessage(QString::fromStdString(::morph::wire::encode(env))); + _socket.sendTextMessage(encoded); } void QtWebSocketBackend::flushQueuedRegistrations() { @@ -230,15 +238,18 @@ bool QtWebSocketBackend::registerModelSharedAsync( return true; } uint64_t const callId = ++_nextCallId; + auto env = + ::morph::wire::makeRegisterShared(typeId, std::string{identity.primary}, std::string{identity.contextKey}); + env.callId = callId; + // See registerModelAsync's identical comment: encoded before the map + // insertion, so a throwing encode() cannot orphan a pending entry. + auto const encoded = QString::fromStdString(::morph::wire::encode(env)); { std::scoped_lock const lock{_pendingMtx}; _pendingRegistrations[callId] = PendingRegistration{.onRegistered = std::move(onRegistered), .onError = std::move(onError)}; } - auto env = - ::morph::wire::makeRegisterShared(typeId, std::string{identity.primary}, std::string{identity.contextKey}); - env.callId = callId; - _socket.sendTextMessage(QString::fromStdString(::morph::wire::encode(env))); + _socket.sendTextMessage(encoded); return true; } @@ -264,15 +275,18 @@ bool QtWebSocketBackend::attachModelAsync( return true; } uint64_t const callId = ++_nextCallId; + auto env = + ::morph::wire::makeAttach(typeId, std::string{identity.primary}, current.v, std::string{identity.contextKey}); + env.callId = callId; + // See registerModelAsync's identical comment: encoded before the map + // insertion, so a throwing encode() cannot orphan a pending entry. + auto const encoded = QString::fromStdString(::morph::wire::encode(env)); { std::scoped_lock const lock{_pendingMtx}; _pendingRegistrations[callId] = PendingRegistration{.onRegistered = std::move(onRegistered), .onError = std::move(onError)}; } - auto env = - ::morph::wire::makeAttach(typeId, std::string{identity.primary}, current.v, std::string{identity.contextKey}); - env.callId = callId; - _socket.sendTextMessage(QString::fromStdString(::morph::wire::encode(env))); + _socket.sendTextMessage(encoded); return true; } diff --git a/tests/test_async_registration.cpp b/tests/test_async_registration.cpp index 934d534d..1d576928 100644 --- a/tests/test_async_registration.cpp +++ b/tests/test_async_registration.cpp @@ -370,6 +370,22 @@ class AsyncBackendShim : public morph::backend::detail::IBackend { 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)); + } + 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)); + } private: std::shared_ptr _target; @@ -527,6 +543,93 @@ TEST_CASE("Bridge::registerHandler: a stale async reply after switchBackend() do CHECK(binding->currentId.load() == idAfterSwitch); } +// NOLINTNEXTLINE(readability-function-cognitive-complexity) +TEST_CASE( + "Bridge::attachHandlerAsync: a stale async attach reply after switchBackend() does not clobber the new " + "binding, and still resolves the caller's Completion", + "[bridge][registration][shared-instances][issue26]") { + SyncExec cbExec; + auto asyncBackendA = std::make_shared(); + morph::bridge::Bridge bridge{std::make_unique(asyncBackendA)}; + morph::bridge::BridgeHandler handler{bridge, &cbExec}; + + std::atomic result{-1}; + std::atomic failed{false}; + auto pending = handler.execute(ARTouch{.id = 42, .amount = 5}); + pending.then([&](int val) { result.store(val); }).onError([&](const std::exception_ptr&) { failed.store(true); }); + + // The attach was dispatched but has not replied yet. + REQUIRE(asyncBackendA->pendingCount() == 1); + CHECK(result.load() == -1); + CHECK_FALSE(failed.load()); + + // Switch away WHILE the attach on asyncBackendA is still outstanding. The + // handler never attached (its primary is still empty), so switchBackend's + // re-registration loop leaves it live-but-unbound on the new backend -- + // matching the `binding->shared && binding->primary.empty()` carry-over + // path. + auto secondBackend = std::make_unique(); + auto* rawSecond = secondBackend.get(); + bridge.switchBackend(std::move(secondBackend)); + + // The original (now-stale) attach reply from asyncBackendA finally + // arrives. It must not be published into the binding as if it were a + // valid id on the now-active backend -- and, unlike a fire-and-forget + // re-registration, this caller's Completion is genuinely waiting on + // `onDone`, so the stale reply must still resolve it (with an error) + // rather than leaving it hanging forever. + asyncBackendA->completeNext(); + REQUIRE(morph::testing::waitUntil([&] { return result.load() != -1 || failed.load(); })); + CHECK(result.load() == -1); + CHECK(failed.load()); + CHECK_FALSE(handler.primary().has_value()); + + // The handler is still usable against the now-active backend: a fresh + // attach succeeds normally, proving the stale reply left no corruption + // behind. + std::atomic secondResult{-1}; + handler.execute(ARTouch{.id = 42, .amount = 9}) + .then([&](int val) { secondResult.store(val); }) + .onError([&](const std::exception_ptr&) { failed.store(true); }); + REQUIRE(rawSecond->pendingCount() == 1); + rawSecond->completeNext(); + REQUIRE(morph::testing::waitUntil([&] { return secondResult.load() != -1; })); + CHECK(secondResult.load() == 9); + CHECK(handler.primary().value_or(-1) == 42); +} + +// NOLINTNEXTLINE(readability-function-cognitive-complexity) +TEST_CASE( + "Bridge::ensureBoundAsync: a stale async bind reply after switchBackend() does not clobber the new binding, " + "and still resolves the caller's Completion", + "[bridge][registration][shared-instances][issue26]") { + SyncExec cbExec; + auto asyncBackendA = std::make_shared(); + morph::bridge::Bridge bridge{std::make_unique(asyncBackendA)}; + morph::bridge::BridgeHandler handler{bridge, &cbExec}; + + std::atomic value{-1}; + std::atomic failed{false}; + auto pending = handler.execute(ARKeyedCreate{.initial = 11}); + pending.then([&](ARKeyedCreated res) { value.store(res.value); }).onError([&](const std::exception_ptr&) { + failed.store(true); + }); + + REQUIRE(asyncBackendA->pendingCount() == 1); + CHECK(value.load() == -1); + CHECK_FALSE(failed.load()); + + auto secondBackend = std::make_unique(); + bridge.switchBackend(std::move(secondBackend)); + + // The stale bind reply must not publish `currentId` from a backend + // nothing uses any more, and must still resolve the waiting Completion. + asyncBackendA->completeNext(); + REQUIRE(morph::testing::waitUntil([&] { return value.load() != -1 || failed.load(); })); + CHECK(value.load() == -1); + CHECK(failed.load()); +} + TEST_CASE("Bridge::registerHandler: an async reply arriving after ~Bridge() is a safe no-op", "[bridge][registration][issue26]") { morph::exec::ThreadPoolExecutor pool{2}; From 18022ad00b2980c846a485bf28fe216ed40b00d0 Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Sat, 8 Aug 2026 03:37:50 +0300 Subject: [PATCH 08/20] qt: cover the shared/keyed async wire methods against a real server `registerModelSharedAsync`/`attachModelAsync` had no direct test. Four round-trip cases in the existing RemoteServer rig: a shared register that lands in the instance directory and whose repeat joins the same instance, an attach that joins a seeded instance and then re-points to another, the empty-primary degrade-to-private branch, and the `!_connected` branch that reports `onError` inline. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01HffixfknmXdAjnM5hSGRBb --- tests/qt/test_qt_websocket.cpp | 150 +++++++++++++++++++++++++++++++++ 1 file changed, 150 insertions(+) diff --git a/tests/qt/test_qt_websocket.cpp b/tests/qt/test_qt_websocket.cpp index 717f86d5..88f29d48 100644 --- a/tests/qt/test_qt_websocket.cpp +++ b/tests/qt/test_qt_websocket.cpp @@ -287,6 +287,156 @@ TEST_CASE( CHECK(binding->currentId.load() == 0U); // onRegistered never fired; still safely unbound } +// ── The shared/keyed async wire methods ────────────────────────────────────── +// Same opt-in gate and same callId-keyed reply routing as registerModelAsync +// above; these drive them against a real RemoteServer, end to end. + +// NOLINTNEXTLINE(readability-function-cognitive-complexity) +TEST_CASE("morph::qt::QtWebSocketBackend: registerModelSharedAsync registers-or-attaches without blocking", + "[qt][ws][issue26][shared-instances]") { + ensureApp(); + morph::exec::ThreadPoolExecutor serverPool{2}; + auto server = std::make_shared(serverPool); + morph::qt::QtWebSocketServer wsServer{*server, 0}; + REQUIRE(wsServer.listen()); + + QUrl url{QString("ws://127.0.0.1:%1").arg(wsServer.port())}; + morph::qt::QtWebSocketBackend backend{url, morph::model::detail::defaultDispatcher(), + morph::model::detail::defaultRegistry(), std::nullopt, + morph::qt::QtWebSocketBackend::Config{.asyncRegistrationEnabled = true}}; + REQUIRE(backend.waitForConnected()); + + std::atomic registered{0}; + std::string failure; + REQUIRE(backend.registerModelSharedAsync( + "WsEchoModel", nullptr, {.contextKey = "acct-1", .primary = "acct-1"}, + [&](morph::exec::detail::ModelId mid) { registered.store(mid.v); }, + [&](const std::string& message) { failure = message; })); + + // Returned true without waiting for the reply: nothing has arrived yet. + CHECK(registered.load() == 0U); + + pumpUntil([&] { return registered.load() != 0U || !failure.empty(); }); + CHECK(failure.empty()); + REQUIRE(registered.load() != 0U); + + // It really went out as a *shared* register, not a private one: the key is + // now in the server's instance directory. + auto const keys = backend.listInstances("WsEchoModel"); + REQUIRE(keys.size() == 1); + CHECK(keys.front() == "acct-1"); + + // A second shared register for the same key joins the same instance rather + // than creating a second one -- the register-or-attach half of the name. + std::atomic second{0}; + REQUIRE(backend.registerModelSharedAsync( + "WsEchoModel", nullptr, {.contextKey = "acct-1", .primary = "acct-1"}, + [&](morph::exec::detail::ModelId mid) { second.store(mid.v); }, + [&](const std::string& message) { failure = message; })); + pumpUntil([&] { return second.load() != 0U || !failure.empty(); }); + CHECK(failure.empty()); + CHECK(second.load() == registered.load()); +} + +// NOLINTNEXTLINE(readability-function-cognitive-complexity) +TEST_CASE("morph::qt::QtWebSocketBackend: attachModelAsync joins the existing shared instance without blocking", + "[qt][ws][issue26][shared-instances]") { + ensureApp(); + morph::exec::ThreadPoolExecutor serverPool{2}; + auto server = std::make_shared(serverPool); + morph::qt::QtWebSocketServer wsServer{*server, 0}; + REQUIRE(wsServer.listen()); + + QUrl url{QString("ws://127.0.0.1:%1").arg(wsServer.port())}; + morph::qt::QtWebSocketBackend backend{url, morph::model::detail::defaultDispatcher(), + morph::model::detail::defaultRegistry(), std::nullopt, + morph::qt::QtWebSocketBackend::Config{.asyncRegistrationEnabled = true}}; + REQUIRE(backend.waitForConnected()); + + // Seed the directory synchronously, so the async attach below has something + // to join and its reply can be compared against a known id. + auto const seeded = + backend.registerModelShared("WsEchoModel", nullptr, {.contextKey = "acct-7", .primary = "acct-7"}); + REQUIRE(seeded.v != 0U); + + std::atomic attached{0}; + std::string failure; + REQUIRE(backend.attachModelAsync( + "WsEchoModel", nullptr, {.contextKey = "acct-7", .primary = "acct-7"}, morph::exec::detail::ModelId{0}, + [&](morph::exec::detail::ModelId mid) { attached.store(mid.v); }, + [&](const std::string& message) { failure = message; })); + CHECK(attached.load() == 0U); // the reply has not arrived yet + + pumpUntil([&] { return attached.load() != 0U || !failure.empty(); }); + CHECK(failure.empty()); + CHECK(attached.load() == seeded.v); + + // Re-pointing to a different key gets a different instance, still async. + std::atomic repointed{0}; + REQUIRE(backend.attachModelAsync( + "WsEchoModel", nullptr, {.contextKey = "acct-8", .primary = "acct-8"}, + morph::exec::detail::ModelId{attached.load()}, + [&](morph::exec::detail::ModelId mid) { repointed.store(mid.v); }, + [&](const std::string& message) { failure = message; })); + pumpUntil([&] { return repointed.load() != 0U || !failure.empty(); }); + CHECK(failure.empty()); + REQUIRE(repointed.load() != 0U); + CHECK(repointed.load() != seeded.v); +} + +// NOLINTNEXTLINE(readability-function-cognitive-complexity) +TEST_CASE("morph::qt::QtWebSocketBackend: attachModelAsync with an empty primary degrades to a private registration", + "[qt][ws][issue26][shared-instances]") { + ensureApp(); + morph::exec::ThreadPoolExecutor serverPool{2}; + auto server = std::make_shared(serverPool); + morph::qt::QtWebSocketServer wsServer{*server, 0}; + REQUIRE(wsServer.listen()); + + QUrl url{QString("ws://127.0.0.1:%1").arg(wsServer.port())}; + morph::qt::QtWebSocketBackend backend{url, morph::model::detail::defaultDispatcher(), + morph::model::detail::defaultRegistry(), std::nullopt, + morph::qt::QtWebSocketBackend::Config{.asyncRegistrationEnabled = true}}; + REQUIRE(backend.waitForConnected()); + + std::atomic registered{0}; + std::string failure; + REQUIRE(backend.attachModelAsync( + "WsEchoModel", nullptr, {.contextKey = "ctx", .primary = ""}, morph::exec::detail::ModelId{0}, + [&](morph::exec::detail::ModelId mid) { registered.store(mid.v); }, + [&](const std::string& message) { failure = message; })); + + pumpUntil([&] { return registered.load() != 0U || !failure.empty(); }); + CHECK(failure.empty()); + REQUIRE(registered.load() != 0U); + // Private, exactly like the synchronous attachModel's own empty-primary + // branch: nothing was filed in the shared directory. + CHECK(backend.listInstances("WsEchoModel").empty()); +} + +TEST_CASE("morph::qt::QtWebSocketBackend: registerModelSharedAsync on a never-connected socket reports onError", + "[qt][ws][issue26][shared-instances][disconnect]") { + ensureApp(); + // Port 1 is reserved and never listening — the socket never reaches Connected. + QUrl url{QString("ws://127.0.0.1:1")}; + morph::qt::QtWebSocketBackend backend{url, morph::model::detail::defaultDispatcher(), + morph::model::detail::defaultRegistry(), std::nullopt, + morph::qt::QtWebSocketBackend::Config{.asyncRegistrationEnabled = true}}; + REQUIRE_FALSE(backend.waitForConnected(200)); + + std::string failure; + std::atomic registered{0}; + // Accepts the request (returns true) and reports the failure through + // onError rather than blocking or throwing. Bridge::ensureBoundAsync + // tolerates this firing inline, from inside the call itself. + REQUIRE(backend.registerModelSharedAsync( + "WsEchoModel", nullptr, {.contextKey = "acct-1", .primary = "acct-1"}, + [&](morph::exec::detail::ModelId mid) { registered.store(mid.v); }, + [&](const std::string& message) { failure = message; })); + CHECK(registered.load() == 0U); + CHECK(failure == "disconnected"); +} + TEST_CASE( "morph::qt::QtWebSocketBackend: registerModelAsync called before the socket connects queues and retries once " "connected fires", From 5e684c3e2a36930a3ae9dc6c04174e86662cc7f7 Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Tue, 11 Aug 2026 21:40:54 +0300 Subject: [PATCH 09/20] tests: add regression coverage for finding 023 (Completion::onError single-slot) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds a direct test that CompletionState::attachOnError keeps only the most recently attached handler: attaching .onError() twice to the same pending Completion and asserting only the second handler fires. This documents the single-slot mechanism at its source in the core test suite. Extracted from application-ladder's 46c08f3, which paired this with a ladder-specific test (examples/common/testkit/test_presenter.cpp, exercising Presenter::track()'s onErr forwarding) — that half stays on the ladder branch since Presenter doesn't exist here. Co-Authored-By: Claude Sonnet 5 --- tests/test_completion.cpp | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/tests/test_completion.cpp b/tests/test_completion.cpp index 4b00b72e..20f24dc1 100644 --- a/tests/test_completion.cpp +++ b/tests/test_completion.cpp @@ -77,6 +77,31 @@ TEST_CASE("morph::async::Completion on_error does not fire on value", "[completi REQUIRE_FALSE(errorFired); } +TEST_CASE("morph::async::Completion onError composes every attached handler, in attachment order", + "[completion]") { + // CompletionState::attachOnError appends to a vector of handlers, so a + // second .onError() call on the same still-pending Completion — even + // via the separate Completion& returned from the first call, since + // then()/onError() both return *this — fires alongside the first rather + // than replacing it (see docs/spec/core/completion.md, "Handler + // fan-out"). This is the mechanism behind Presenter::track()'s onErr + // parameter (examples/common/gui/presenter.hpp), documented here at its + // source. + SyncExecutor exec; + auto state = std::make_shared>(); + morph::async::Completion comp{state, &exec}; + + bool handlerAFired = false; + bool handlerBFired = false; + comp.onError([&](const std::exception_ptr&) { handlerAFired = true; }); + comp.onError([&](const std::exception_ptr&) { handlerBFired = true; }); + + state->setException(std::make_exception_ptr(std::runtime_error{"test error"})); + + REQUIRE(handlerAFired); + REQUIRE(handlerBFired); +} + TEST_CASE("morph::async::Completion callback is posted through executor", "[completion]") { struct CountingExecutor : morph::exec::IExecutor { std::atomic count{0}; From 43a4b9a1937c4eb15d51933fe5674a21aa13f1be Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Tue, 11 Aug 2026 21:41:30 +0300 Subject: [PATCH 10/20] util: add morph::units::toString() for Quantity, work around a libc++ auto-NTTP formattability gap Emscripten's bundled libc++ has a known limitation recognising a std::formatter partial specialization parameterized over an `auto` non-type template parameter (the unit enumerator U here) for std::format's compile-time formattability check: `std::format("{}", someQuantity)` fails to compile there with "the supplied type is not formattable", even though the specialization is valid and the identical call compiles and runs correctly on every other toolchain (native Windows/Linux). Adds morph::units::toString(Quantity) -- the formatter's own rendering logic, exposed as a plain function -- and has the formatter delegate to it instead of duplicating the logic. A caller that hits the libc++ gap can call toString() directly, bypassing std::format's buggy compile-time trait check entirely. Produces byte-identical output to the std::formatter path, so this is a non-behavioral refactor for every existing caller. Updates docs/spec/util/quantity_type.md to document the new symbol and the delegation, closing the header/spec sync gate this trips. Extracted from application-ladder's 2ddbb85 + cc2ae7b, which paired this with ladder call sites (pastebin/bookmarks/polls QML bridges) switching from std::format to toString() -- those stay on the ladder branch, which is where the WASM build that actually exercises this gap lives. Co-Authored-By: Claude Sonnet 5 --- docs/spec/util/quantity_type.md | 5 +++-- include/morph/util/quantity.hpp | 38 +++++++++++++++++++++++++++------ 2 files changed, 35 insertions(+), 8 deletions(-) diff --git a/docs/spec/util/quantity_type.md b/docs/spec/util/quantity_type.md index 5480ebe7..19adec20 100644 --- a/docs/spec/util/quantity_type.md +++ b/docs/spec/util/quantity_type.md @@ -824,7 +824,8 @@ yields empty. | Symbol | Kind | Notes | |---|---|---| | `NamedQuantity` | class template | `Quantity` (declared precision defaulted) that names itself `Name` on construction; slices losslessly to a plain `Quantity`. `Name` is a `detail::FixedString` NTTP. Constructors: default (empty), `optional`, and from a plain `Quantity` — each names the value after building it; plus `static fromDouble(double)`. The name lives in the shared history, not as extra data. | -| `std::formatter>` | specialisation | Renders value + unit (`5.2kW`, `N/A%`). No `operator<<`. | +| `toString(Quantity)` | free function | Renders value + unit (`5.2kW`, `N/A%`) — the identical text `std::format("{}", q)` produces, via the identical logic; the `std::formatter` specialisation below delegates to it. Exists solely so a caller can render a `Quantity` without going through `std::format`'s own trait machinery: Emscripten's bundled libc++ has a known gap recognising a `std::formatter` partial specialisation parameterised over an `auto` non-type template parameter, so `std::format("{}", quantity)` fails to compile there outright, even though the specialisation is valid and works on every other toolchain this project targets. `pastebin::gui::readsText`/`bookmarks::gui::countText`/`polls::gui::countText` (the three QML-bridge call sites that render a `Quantity`) call it directly for this reason. | +| `std::formatter>` | specialisation | Renders value + unit (`5.2kW`, `N/A%`) by delegating to `toString`. No `operator<<`. | | `std::formatter>` | specialisation | Forwards to the `Quantity` formatter. | On the wire, `glz::meta` reduces the instance to its nullable @@ -847,7 +848,7 @@ and `unitAlternatives()`. | History structure | **Shared, unit-erased DAG (`ASTUnit` / `ASTNode` / `Context`)** | `ASTNode` (step + optional name + `shared_ptr` children) linked into a DAG; each `Quantity` holds a `Context` (a `shared_ptr` root). Cheap copies; reused subexpressions deduped by node identity; shareable across units. | | Placeholders | **Reuse, not leaf-vs-computed, mints a `cN`** | A value used once inlines (leaf → its number, computed → its expression); only a value reused across the expression earns one shared placeholder, so shared work is written once. | | Precision | **Actual = max of engaged operands; declared from `UnitTraits`** | Max-propagation keeps a result no less precise than its widest input; the declared tag stays a field property (`fromDouble` origin, `atDeclaredPrecision` to reset). | -| Formatting | **`std::formatter` only, delegating to the shared `formatRationalDecimal` renderer** | Single formatting path; no `operator<<`; the runtime `DecimalPlaces` tag is the sole authority on printed decimals. | +| Formatting | **`std::formatter`, delegating to `toString`, which delegates to the shared `formatRationalDecimal` renderer** | One rendering implementation (`toString`); `std::formatter` is a thin wrapper over it, not a second implementation. `toString` itself exists as a direct call target only because `std::format`'s compile-time formattability check fails to see this formatter's `auto`-NTTP specialisation on Emscripten's bundled libc++ — see the symbol table above. No `operator<<`; the runtime `DecimalPlaces` tag is the sole authority on printed decimals. | | Wire | **Payload only** | Units and history never travel; the wire stays a nullable `Rational`. | | Empty ordering | **Throws, not a compile error** | Emptiness is a runtime `optional` state, so ordering an empty operand throws `std::logic_error` (a testable defined diagnostic); `==` stays total. | | Conversion | **`UnitRelation` entries + auto-generated constrained-template `convert`; `UnitTraits::convert` static override** | Application declares exact peer-to-peer ratios in `UnitTraits::relations`; framework auto-generates a constrained `convert(From, To&)` template and records the provenance step. A `UnitTraits::convert` static wins (`if constexpr`) for non-ratio conversions (C↔F, currency) — a static, not an ADL free function, because the unit is a non-type template arg so ADL can't reach the enum's namespace. Chaining composes ratio edges over the relation graph. | diff --git a/include/morph/util/quantity.hpp b/include/morph/util/quantity.hpp index e49f883b..9ad0ee0d 100644 --- a/include/morph/util/quantity.hpp +++ b/include/morph/util/quantity.hpp @@ -1030,6 +1030,36 @@ struct NamedQuantity : Quantity { [[nodiscard]] static NamedQuantity fromDouble(double raw) { return NamedQuantity{Base::fromDouble(raw)}; } }; +/// @brief Renders @p quantity as value + unit (`5.2kW`, `N/A%`) — the same +/// text `std::format("{}", quantity)` produces via the `std::formatter` +/// specialization just below, exposed as a plain function so a caller +/// can render a `Quantity` without going through `std::format` itself. +/// +/// Exists because Emscripten's bundled libc++ (older than the Linux/Windows +/// standard library this project otherwise builds against — see +/// `.github/workflows/wasm-ladder.yml`'s `EMSDK_VERSION`) has a known +/// limitation recognising `std::formatter` partial specializations +/// parameterized over an `auto` non-type template parameter (`U` here) for +/// `std::format`'s compile-time formattability check — `std::format("{}", +/// someQuantity)` fails to compile there with "the supplied type is not +/// formattable" even though the specialization is valid and the identical +/// call compiles and runs correctly on every other toolchain this project +/// targets. `toString()` bypasses that check entirely: it calls the same +/// underlying logic directly instead of through `std::format`'s trait +/// machinery, so it works identically everywhere, WASM included. +/// @tparam U Unit enumerator. +/// @tparam Dec Declared decimals. +/// @param quantity The value to render. +/// @return The formatted text. +template +[[nodiscard]] inline std::string toString(const Quantity& quantity) { + constexpr auto display = UnitTraits::meta(U).display; + if (quantity.value()) { + return detail::formatRationalDecimal(*quantity.value()) + std::string{display}; + } + return "N/A" + std::string{display}; +} + } // namespace morph::units #if MORPH_QUANTITY_PROVENANCE @@ -1037,6 +1067,7 @@ struct NamedQuantity : Quantity { #endif /// @brief Renders value + unit (`5.2kW`, `N/A%`); no `operator<<` is provided. +/// Delegates to `morph::units::toString` so the two never drift. /// @tparam U Unit enumerator. /// @tparam Dec Declared decimals. template @@ -1051,12 +1082,7 @@ struct std::formatter> { /// @param ctx Format context. /// @return Output iterator past the written text. auto format(const morph::units::Quantity& quantity, std::format_context& ctx) const { - constexpr auto display = morph::units::UnitTraits::meta(U).display; - if (quantity.value()) { - return std::format_to(ctx.out(), "{}{}", morph::units::detail::formatRationalDecimal(*quantity.value()), - display); - } - return std::format_to(ctx.out(), "N/A{}", display); + return std::format_to(ctx.out(), "{}", morph::units::toString(quantity)); } }; From dc2f01f3a49720c27cc2be20f20f8a11246cf8f0 Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Tue, 11 Aug 2026 14:28:54 +0300 Subject: [PATCH 11/20] build: probe three -Weverything suppressions instead of assuming clang has them Emscripten's bundled clang (pinned by EMSDK_VERSION in the WASM workflows) is older than the Linux/Windows clang this project otherwise builds with, and rejects -Wno-nrvo, -Wno-unsafe-buffer-usage-in-libc-call, and -Wno-c2y-extensions outright with "unknown warning option" -- turning three suppression flags into the exact -Werror failures they exist to silence, the moment a WASM configure actually compiles a target that reaches apply_warnings() (morph_ladder_gui, the first one in build order, once the earlier morph_add_rung.cmake and wasm_spike fixes let the build get this far). check_cxx_compiler_flag() gates each of the three behind a genex now, probed once per configure and cached like every other CMake compiler check -- self-maintaining across future clang releases on either toolchain, unlike a hard-coded version cutoff. Co-Authored-By: Claude Sonnet 5 --- cmake/compiler_options.cmake | 24 +++++++++++++++++++++--- 1 file changed, 21 insertions(+), 3 deletions(-) diff --git a/cmake/compiler_options.cmake b/cmake/compiler_options.cmake index d9f9b1c8..ed5ffb11 100644 --- a/cmake/compiler_options.cmake +++ b/cmake/compiler_options.cmake @@ -1,3 +1,21 @@ +include(CheckCXXCompilerFlag) + +# Probed once per configure (CMake caches each check_cxx_compiler_flag() result +# in CMakeCache.txt keyed by the result variable, regardless of how many +# targets call apply_warnings()) rather than unconditionally listed below: +# these three are recent enough additions to Clang's -Weverything set that +# Emscripten's bundled clang (pinned to an older release than the Linux/ +# Windows Clang this project otherwise builds with — see +# .github/workflows/wasm-ladder.yml's EMSDK_VERSION) rejects them outright +# under -Werror with "unknown warning option", turning a *suppression* flag +# into the very error it exists to silence. A version-number cutoff would be +# equally correct but more fragile (would need updating every time either +# toolchain's version changes); probing the actual compiler is the standard, +# self-maintaining way to make -Weverything portable across Clang releases. +check_cxx_compiler_flag(-Wno-nrvo MORPH_CLANG_HAS_WNO_NRVO) +check_cxx_compiler_flag(-Wno-unsafe-buffer-usage-in-libc-call MORPH_CLANG_HAS_WNO_UNSAFE_BUFFER_USAGE_IN_LIBC_CALL) +check_cxx_compiler_flag(-Wno-c2y-extensions MORPH_CLANG_HAS_WNO_C2Y_EXTENSIONS) + function(apply_warnings target) target_compile_options(${target} PRIVATE # ── MSVC ────────────────────────────────────────────────────────────── @@ -74,16 +92,16 @@ function(apply_warnings target) -Wno-global-constructors # non-trivial namespace-scope initializers # (c) Stylistic / opinionated noise, not defects. -Wno-missing-noreturn - -Wno-nrvo # not eliding a trivial-type copy on return + $<$:-Wno-nrvo> # not eliding a trivial-type copy on return -Wno-shadow-uncaptured-local # lambda param shadowing an uncaptured local -Wno-documentation-unknown-command -Wno-unsafe-buffer-usage # flags all pointer arithmetic; needs a hardened API - -Wno-unsafe-buffer-usage-in-libc-call + $<$:-Wno-unsafe-buffer-usage-in-libc-call> -Wno-float-equal # exact == is intentional in the value/rational tests # (d) Conflicts with a warning we deliberately keep. -Wno-covered-switch-default # collides with -Wswitch-enum + -Wswitch-default # (e) Third-party test macros. - -Wno-c2y-extensions # Catch2 TEST_CASE expands __COUNTER__ + $<$:-Wno-c2y-extensions> # Catch2 TEST_CASE expands __COUNTER__ -Wno-unused-member-function # Catch2/test-fixture helper members -Wno-unneeded-member-function # (f) Clang 22 (Homebrew, macOS libc++) added thread-safety-analysis From 23bd24c22521919e6b15be26954d72a8d1c06e2d Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Tue, 11 Aug 2026 14:36:53 +0300 Subject: [PATCH 12/20] build: suppress -Wc++20-compat and -Wdisabled-macro-expansion Two more Clang -Weverything diagnostics the WASM leg's Emscripten- bundled clang fires that the Linux/Windows clang builds never have, now that the previous three fixes let morph_ladder_gui's actual source files reach the compiler: - -Wc++20-compat: the narrower sibling of the already-suppressed -Wpre-c++20-compat, covering consteval and implicit `typename` in alias templates specifically. Same "we target C++23" rationale as every other -Wno-pre-c++*-compat entry already in this list -- model_key.hpp, quantity.hpp, forms.hpp and bridge.hpp all use both constructs deliberately. - -Wdisabled-macro-expansion: Emscripten's sysroot stdio.h defines `#define stderr (stderr)` (a legal self-referential macro), which logger.hpp's `std::println(stderr, ...)` trips on. The macro is the platform's, not this codebase's -- nothing to fix in logger.hpp. Both are long-standing, stable Clang flags (unlike the three from the previous commit), so added unconditionally rather than behind a check_cxx_compiler_flag() probe. Co-Authored-By: Claude Sonnet 5 --- cmake/compiler_options.cmake | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/cmake/compiler_options.cmake b/cmake/compiler_options.cmake index ed5ffb11..81d203a4 100644 --- a/cmake/compiler_options.cmake +++ b/cmake/compiler_options.cmake @@ -84,12 +84,31 @@ function(apply_warnings target) -Wno-pre-c++17-compat-pedantic -Wno-pre-c++20-compat -Wno-pre-c++20-compat-pedantic + # -Wc++20-compat is the sibling of -Wpre-c++20-compat for a + # narrower set of syntax (consteval, implicit `typename` in alias + # templates) that only some Clang builds separate out from the + # pre-c++20-compat umbrella above — same "we target C++23" + # rationale, added once the WASM leg's Emscripten-bundled clang + # (older than the Linux/Windows clang this project otherwise + # builds with) was the first to actually split it out and fire it + # on model_key.hpp/quantity.hpp/forms.hpp/bridge.hpp. + -Wno-c++20-compat # (b) Inherent to a header-only, templated library. -Wno-weak-vtables # vtable emitted per TU for inline-virtual classes -Wno-ctad-maybe-unsupported # CTAD on types without explicit deduction guides -Wno-padded # struct tail/inter-member padding -Wno-exit-time-destructors # function-local statics with non-trivial dtors -Wno-global-constructors # non-trivial namespace-scope initializers + # Emscripten's sysroot stdio.h defines `#define stderr (stderr)` + # (a legal, intentional self-referential object-like macro used + # to make `stderr` a valid preprocessor token while still + # resolving to the libc symbol) -- logger.hpp's + # std::println(stderr, ...) call trips -Wdisabled-macro-expansion + # on that expansion. Not fixable in logger.hpp itself: the macro + # is the *platform's*, not this codebase's, and every other + # target's libc either doesn't define stderr as a macro at all or + # doesn't self-reference it this way. + -Wno-disabled-macro-expansion # (c) Stylistic / opinionated noise, not defects. -Wno-missing-noreturn $<$:-Wno-nrvo> # not eliding a trivial-type copy on return From f863cf986c717d65324be1fbd6d8dda8e8f5a02d Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Tue, 11 Aug 2026 14:48:20 +0300 Subject: [PATCH 13/20] build+docs: fix three more WASM-leg-only Clang diagnostics Reached morph_ladder_gui/morph_ladder_app's actual source files for the first time (steps 30-52/87) once the previous suppressions cleared; three new -Werror failures surfaced there: - forms.hpp: `@tparam N` on isLiteralString>'s variable-template partial specialization trips -Wdocumentation -- this Clang doesn't recognize @tparam as attached to a specialization the way it does the primary template just above it. Folded the parameter description into @brief prose instead of documenting a tag Clang won't accept, no information lost. - qt_websocket_backend.hpp: `@param tls` documented a constructor parameter that is #ifndef QT_NO_SSL-only (a WASM build is always QT_NO_SSL) -- true doc/declaration mismatch on this specific compile. Wrapped the doc line in the identical #ifndef QT_NO_SSL guard as the parameter itself. - compiler_options.cmake: -Wmissing-designated-field-initializers flags morph::session::Context{.principal = principal} for not also setting .token -- exactly this codebase's normal way to construct a DTO/config-style aggregate with everything else left at its member default (55+ occurrences of the same pattern across the ladder rungs). Added as a fifth check_cxx_compiler_flag()-gated suppression, same rationale as the four already added for this WASM leg. Verified locally: ladder_pastebin_tests, ladder_bookmarks_tests, ladder_polls_tests, ladder_common_tests all rebuild clean and pass (bookmarks' one failure is the known pre-existing Windows temp-file- lock flake in test_app.cpp). Co-Authored-By: Claude Sonnet 5 --- cmake/compiler_options.cmake | 9 +++++++++ include/morph/forms/forms.hpp | 4 ++-- include/morph/qt/qt_websocket_backend.hpp | 2 ++ 3 files changed, 13 insertions(+), 2 deletions(-) diff --git a/cmake/compiler_options.cmake b/cmake/compiler_options.cmake index 81d203a4..c9d071d6 100644 --- a/cmake/compiler_options.cmake +++ b/cmake/compiler_options.cmake @@ -15,6 +15,7 @@ include(CheckCXXCompilerFlag) check_cxx_compiler_flag(-Wno-nrvo MORPH_CLANG_HAS_WNO_NRVO) check_cxx_compiler_flag(-Wno-unsafe-buffer-usage-in-libc-call MORPH_CLANG_HAS_WNO_UNSAFE_BUFFER_USAGE_IN_LIBC_CALL) check_cxx_compiler_flag(-Wno-c2y-extensions MORPH_CLANG_HAS_WNO_C2Y_EXTENSIONS) +check_cxx_compiler_flag(-Wno-missing-designated-field-initializers MORPH_CLANG_HAS_WNO_MISSING_DESIGNATED_FIELD_INITIALIZERS) function(apply_warnings target) target_compile_options(${target} PRIVATE @@ -111,6 +112,14 @@ function(apply_warnings target) -Wno-disabled-macro-expansion # (c) Stylistic / opinionated noise, not defects. -Wno-missing-noreturn + # Deliberately-partial designated initialization of DTO/config- + # style aggregates (morph::session::Context{.principal = ...} + # and its many siblings across the ladder rungs) is this + # codebase's normal way to construct one with everything else + # left at its member default -- not an oversight this warning + # should flag. Probed like the other recent-Clang-only flags + # above: not every Clang release has this diagnostic yet. + $<$:-Wno-missing-designated-field-initializers> $<$:-Wno-nrvo> # not eliding a trivial-type copy on return -Wno-shadow-uncaptured-local # lambda param shadowing an uncaptured local -Wno-documentation-unknown-command diff --git a/include/morph/forms/forms.hpp b/include/morph/forms/forms.hpp index 2feaedb3..c8905981 100644 --- a/include/morph/forms/forms.hpp +++ b/include/morph/forms/forms.hpp @@ -621,8 +621,8 @@ using LiteralString = ::morph::detail::FixedString; template inline constexpr bool isLiteralString = false; -/// @brief `isLiteralString` specialization recognising `LiteralString`. -/// @tparam N Literal length of the recognised `LiteralString`. +/// @brief `isLiteralString` specialization recognising `LiteralString`, +/// where `N` is the recognised literal's length. template inline constexpr bool isLiteralString> = true; diff --git a/include/morph/qt/qt_websocket_backend.hpp b/include/morph/qt/qt_websocket_backend.hpp index e50ddae0..ad5d9dba 100644 --- a/include/morph/qt/qt_websocket_backend.hpp +++ b/include/morph/qt/qt_websocket_backend.hpp @@ -102,9 +102,11 @@ class QtWebSocketBackend : public ::morph::backend::detail::IBackend { /// @param serverUrl `ws://` or `wss://` URL of the remote `RemoteServer`. /// @param dispatcher Action dispatcher (defaults to the process-level singleton). /// @param registry Model registry (defaults to the process-level singleton). +#ifndef QT_NO_SSL /// @param tls If non-null, enables TLS and applies this configuration. Not /// declared at all on an SSL-less Qt build (`QT_NO_SSL`) — see /// the class doc comment's "SSL-less Qt builds" section. +#endif /// @param cfg Reconnect tuning. Default: enabled, 500ms initial / 30s cap, 2x backoff. explicit QtWebSocketBackend( QUrl serverUrl, From 60cc1e1d7f874753f0716e3b774420fbd7364c7a Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Tue, 11 Aug 2026 21:42:15 +0300 Subject: [PATCH 14/20] build: suppress GCC's -Wmissing-field-initializers for designated-init DTOs GCC's -Wextra implies -Wmissing-field-initializers, which -- unlike Clang's narrower -Wmissing-designated-field-initializers, already suppressed for the identical reason -- fires on every field a designated initializer leaves unset, flagging this codebase's normal way to construct a DTO/config-style aggregate with everything else left at its member default (morph::session::Context{.principal = ...} and its many siblings) as a defect, one diagnostic per omitted field. Extracted from application-ladder's dcd411c, which paired this with ladder-only fixes (bookmarks TokenIssuer call sites, a ci.yml Qt version gap) that stay on the ladder branch. Co-Authored-By: Claude Sonnet 5 --- cmake/compiler_options.cmake | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/cmake/compiler_options.cmake b/cmake/compiler_options.cmake index c9d071d6..a8a9de7e 100644 --- a/cmake/compiler_options.cmake +++ b/cmake/compiler_options.cmake @@ -54,6 +54,14 @@ function(apply_warnings target) $<$: -Wall -Wextra + # GCC's -Wextra implies -Wmissing-field-initializers, which (unlike + # Clang's narrower -Wmissing-designated-field-initializers, already + # suppressed above for the identical reason) fires on every field a + # designated initializer leaves unset -- flagging the same + # deliberately-partial DTO/config-style construction + # (morph::session::Context{.principal = ...} and its many + # siblings) as a defect, one diagnostic per omitted field. + -Wno-missing-field-initializers -Wpedantic -Wshadow -Wnon-virtual-dtor From 1b102e663f6031e508c7ae67ae3bb2999bb3f104 Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Tue, 11 Aug 2026 21:43:15 +0300 Subject: [PATCH 15/20] build+tests: fix three MSVC/Windows portability gaps - Define NOMINMAX/WIN32_LEAN_AND_MEAN build-wide, before any subdirectory is added, so any future -including dependency's min/max macros can't leak into a translation unit that later calls std::min/std::max/std::numeric_limits::min(). - tests/qt/qt_test_server_main.cpp: use _fileno instead of fileno for the Windows CRT (POSIX fileno isn't declared there). - tests/qt/test_qt_websocket.cpp: disambiguate 0-literal overload resolution under MSVC for QtWebSocketServer's port parameter (quint16{0} instead of a bare 0, which MSVC can resolve to the wrong overload). Extracted from application-ladder's 1043b7c, which paired these with ladder-only fixes (Lightweight ORM fetch config, vcpkg.json additions, [[nodiscard]] casts and 0-literal disambiguation in example test files) that stay on the ladder branch. Co-Authored-By: Claude Sonnet 5 --- CMakeLists.txt | 12 ++++++++++++ tests/qt/qt_test_server_main.cpp | 8 ++++---- 2 files changed, 16 insertions(+), 4 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index c8f016ac..1fc6cc16 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -10,6 +10,18 @@ set(CMAKE_CXX_STANDARD 23) set(CMAKE_CXX_STANDARD_REQUIRED ON) set(CMAKE_CXX_EXTENSIONS OFF) +# Global, build-wide: the Lightweight ORM (fetched into examples/common's +# ladder testkit and examples/bank) includes from SqlStatement.hpp +# with no NOMINMAX/WIN32_LEAN_AND_MEAN guard of its own, so its `min`/`max` +# macros leak into every translation unit that (transitively) includes it and +# break any later std::min/std::max/std::numeric_limits::min() call in the +# same TU (e.g. examples/common/clock.hpp). Defined here, before any +# subdirectory is added, so it reaches every target that could end up in such +# a TU, not just the ones that call apply_warnings(). +if(WIN32) + add_compile_definitions(NOMINMAX WIN32_LEAN_AND_MEAN) +endif() + option(MORPH_BUILD_TESTS "Build tests" ON) option(MORPH_BUILD_EXAMPLES "Build examples" ON) option(MORPH_BUILD_BANK_EXAMPLE "Build the SQLite/Lightweight bank example (heavy deps)" OFF) diff --git a/tests/qt/qt_test_server_main.cpp b/tests/qt/qt_test_server_main.cpp index bfaf334a..eaedda36 100644 --- a/tests/qt/qt_test_server_main.cpp +++ b/tests/qt/qt_test_server_main.cpp @@ -70,10 +70,10 @@ int main(int argc, char* argv[]) { std::cout.flush(); // Watch stdin for a "quit" command so the test can shut us down cleanly. -#ifdef _MSC_VER - // MSVC flags the POSIX `fileno` name itself (C4996) even though the - // symbol it names is the one actually deprecated; `_fileno` is its - // ISO-conformant replacement on this toolchain only. +#ifdef _WIN32 + // MSVC (and other Windows toolchains) flag the POSIX `fileno` name itself + // (C4996) even though the symbol it names is the one actually deprecated; + // `_fileno` is the ISO-conformant replacement on Windows. auto* stdinWatcher = new QSocketNotifier(_fileno(stdin), QSocketNotifier::Read, &app); #else auto* stdinWatcher = new QSocketNotifier(fileno(stdin), QSocketNotifier::Read, &app); From 2475c77df5f74caf9a4697d01181fc01576ec798 Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Thu, 13 Aug 2026 09:19:33 +0300 Subject: [PATCH 16/20] tests: add direct TimeoutScheduler unit coverage for the throw-swallowing paths Bridge::executeVia and RemoteServer only ever schedule callbacks that don't throw, so run()'s catch(std::exception&)/catch(...) arms around entry.callback() had zero hit count. test_timeout_scheduler.cpp exercises TimeoutScheduler directly: a callback throwing std::exception, a callback throwing a non-std::exception, a throwing callback not blocking a later callback from firing, cancel() on an unknown handle, and cancel() before the deadline elapses. Co-Authored-By: Claude Sonnet 5 --- tests/CMakeLists.txt | 1 + tests/test_timeout_scheduler.cpp | 102 +++++++++++++++++++++++++++++++ 2 files changed, 103 insertions(+) create mode 100644 tests/test_timeout_scheduler.cpp diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index c790cb2d..74243c72 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -32,6 +32,7 @@ add_executable(morph_tests test_security_fixes.cpp test_bridge_lifetime.cpp test_client_execute_deadline.cpp + test_timeout_scheduler.cpp test_dispatch_di.cpp test_handler_binding.cpp test_switch_backend.cpp diff --git a/tests/test_timeout_scheduler.cpp b/tests/test_timeout_scheduler.cpp new file mode 100644 index 00000000..801738c9 --- /dev/null +++ b/tests/test_timeout_scheduler.cpp @@ -0,0 +1,102 @@ +// SPDX-License-Identifier: Apache-2.0 +// +// Direct unit coverage for morph::async::detail::TimeoutScheduler (the +// threaded build, compiled whenever __EMSCRIPTEN__ without +// __EMSCRIPTEN_PTHREADS__ is not defined -- see timeout_scheduler.hpp's @file +// comment for the single-threaded browser build, which this file's own +// target never compiles and cannot exercise). Bridge::executeVia and +// RemoteServer only ever call schedule()/cancel() with callbacks that don't +// throw, so this file covers the case they don't: a scheduled callback that +// throws is logged and swallowed rather than propagating out of the +// scheduler's background thread. + +#include +#include +#include +#include +#include +#include + +using morph::async::detail::TimeoutScheduler; +using namespace std::chrono_literals; + +namespace { + +// Polls until predicate is true or the deadline elapses, to avoid a +// fixed-sleep race between the test thread and the scheduler's background +// thread invoking the callback. +template +bool waitFor(Predicate predicate, std::chrono::milliseconds timeout = 2s) { + auto const deadline = std::chrono::steady_clock::now() + timeout; + while (std::chrono::steady_clock::now() < deadline) { + if (predicate()) { + return true; + } + std::this_thread::sleep_for(1ms); + } + return predicate(); +} + +} // namespace + +TEST_CASE("TimeoutScheduler: a callback throwing std::exception is logged and swallowed", + "[timeout_scheduler]") { + TimeoutScheduler scheduler; + std::atomic fired{false}; + + scheduler.schedule(1ms, [&fired] { + fired = true; + throw std::runtime_error{"boom"}; + }); + + // The scheduler thread must survive the throw: the callback still runs + // (fired becomes true) and the destructor below still joins cleanly + // instead of std::terminate-ing on an escaped exception. + REQUIRE(waitFor([&] { return fired.load(); })); +} + +TEST_CASE("TimeoutScheduler: a callback throwing a non-std::exception is logged and swallowed", + "[timeout_scheduler]") { + TimeoutScheduler scheduler; + std::atomic fired{false}; + + scheduler.schedule(1ms, [&fired] { + fired = true; + throw 42; // NOLINT(hicpp-exception-baseclass) -- exercises the catch(...) arm deliberately. + }); + + REQUIRE(waitFor([&] { return fired.load(); })); +} + +TEST_CASE("TimeoutScheduler: a callback that throws does not stop later callbacks from firing", + "[timeout_scheduler]") { + TimeoutScheduler scheduler; + std::atomic secondFired{false}; + + scheduler.schedule(1ms, [] { throw std::runtime_error{"first callback throws"}; }); + scheduler.schedule(5ms, [&secondFired] { secondFired = true; }); + + REQUIRE(waitFor([&] { return secondFired.load(); })); +} + +TEST_CASE("TimeoutScheduler: cancel() on an unknown handle is a no-op", "[timeout_scheduler]") { + TimeoutScheduler scheduler; + // Never returned by schedule() on this instance -- exercises the + // not-found arm of cancel() without racing a real entry. + scheduler.cancel(TimeoutScheduler::Handle{999999}); + SUCCEED("cancel() on an unknown handle returned without firing or throwing"); +} + +TEST_CASE("TimeoutScheduler: cancel() before the deadline prevents the callback from firing", + "[timeout_scheduler]") { + TimeoutScheduler scheduler; + std::atomic fired{false}; + + auto const handle = scheduler.schedule(50ms, [&fired] { fired = true; }); + scheduler.cancel(handle); + + // Give the (cancelled) deadline time to have elapsed, then confirm the + // callback never ran. + std::this_thread::sleep_for(80ms); + REQUIRE_FALSE(fired.load()); +} From 9c2d63add1a121fce632b8cef1763db430132de7 Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Thu, 13 Aug 2026 09:44:50 +0300 Subject: [PATCH 17/20] tests: cover attachHandlerAsync/ensureBoundAsync's throwing-dispatch and destroyed-Bridge/binding paths codecov flagged 20 uncovered lines in bridge.hpp's patch diff. Six of those are a genuine tool-rendering artifact (the first statement after a block containing a conditional early-return shows a 0 hit count though the next line, executing exactly as often, shows the correct count -- the same pattern already confirmed for PR #71's bridge.hpp weak_ptr line). The rest are real: no test exercised a backend whose attachModelAsync/registerModelSharedAsync throws synchronously, or an async attach/bind reply arriving after the Bridge or its binding is already gone. Adds ThrowingDispatchBackend (throws from both dispatch entry points) and four lifetime tests: attachHandlerAsync/ensureBoundAsync's out-of-frame success callback is a no-op once the Bridge is gone, and once only the binding is gone. The two Bridge-gone cases co-own the backend via the existing AsyncBackendShim so completeNext() can still run after the Bridge itself is destroyed. Co-Authored-By: Claude Sonnet 5 --- tests/test_async_registration.cpp | 151 ++++++++++++++++++++++++++++++ 1 file changed, 151 insertions(+) diff --git a/tests/test_async_registration.cpp b/tests/test_async_registration.cpp index 1d576928..768fbb43 100644 --- a/tests/test_async_registration.cpp +++ b/tests/test_async_registration.cpp @@ -341,6 +341,29 @@ class InlineCompletingBackend : public AsyncRegisterBackend { 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. +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"); + } +}; + // Shim so a Bridge (which takes ownership of a unique_ptr) can hold a backend // the test also keeps a shared_ptr to -- making it co-owned / able to outlive // the Bridge (see test_bridge_lifetime.cpp's identical BackendShim). Also lets @@ -1448,3 +1471,131 @@ TEST_CASE("ensureBoundAsync mirrors the same three cases for a result-keyed (cre CHECK_FALSE(handler.primary().has_value()); } } + +TEST_CASE("attachHandlerAsync reports a synchronously-throwing dispatch call through onDone", + "[bridge][registration][shared-instances][issue26]") { + SyncExec cbExec; + morph::bridge::Bridge bridge{std::make_unique()}; + morph::bridge::BridgeHandler handler{bridge, &cbExec}; + + std::string message; + std::atomic succeeded{false}; + REQUIRE_NOTHROW(handler.execute(ARTouch{.id = 9, .amount = 1}) + .then([&](int) { succeeded.store(true); }) + .onError([&](const std::exception_ptr& err) { + try { + std::rethrow_exception(err); + } catch (const std::exception& exc) { + message = exc.what(); + } + })); + + CHECK(message == "attachModelAsync dispatch failed"); + CHECK_FALSE(succeeded.load()); + CHECK_FALSE(handler.primary().has_value()); +} + +TEST_CASE("ensureBoundAsync reports a synchronously-throwing dispatch call through onDone", + "[bridge][registration][shared-instances][issue26]") { + SyncExec cbExec; + morph::bridge::Bridge bridge{std::make_unique()}; + morph::bridge::BridgeHandler handler{bridge, &cbExec}; + + std::string message; + std::atomic succeeded{false}; + REQUIRE_NOTHROW(handler.execute(ARKeyedCreate{.initial = 3}) + .then([&](ARKeyedCreated) { succeeded.store(true); }) + .onError([&](const std::exception_ptr& err) { + try { + std::rethrow_exception(err); + } catch (const std::exception& exc) { + message = exc.what(); + } + })); + + CHECK(message == "registerModelSharedAsync dispatch failed"); + CHECK_FALSE(succeeded.load()); + CHECK_FALSE(handler.primary().has_value()); +} + +TEST_CASE("attachHandlerAsync's out-of-frame success callback is a no-op once the Bridge is gone", + "[bridge][registration][shared-instances][issue26]") { + SyncExec cbExec; + // The backend must outlive the Bridge for this test to complete the reply + // after destroying it, so it is co-owned via AsyncBackendShim (the same + // pattern test_bridge_lifetime.cpp uses) rather than owned solely by the + // Bridge's unique_ptr. + auto sharedBackend = std::make_shared(); + auto bridge = std::make_unique(std::make_unique(sharedBackend)); + auto handler = std::make_unique>(*bridge, &cbExec); + + REQUIRE_NOTHROW(handler->execute(ARTouch{.id = 11, .amount = 4})); + REQUIRE(sharedBackend->pendingCount() == 1); + + // Destroy the handler and the Bridge itself before the deferred reply + // lands: attachHandlerAsync's success callback holds only weak references + // to both, so completing it now must be a quiet no-op rather than + // dereferencing freed memory. + handler.reset(); + bridge.reset(); + + REQUIRE_NOTHROW(sharedBackend->completeNext()); + SUCCEED("completing an attach reply after the Bridge and handler are both gone did not crash"); +} + +TEST_CASE("attachHandlerAsync's out-of-frame success callback is a no-op once only the binding is gone", + "[bridge][registration][shared-instances][issue26]") { + SyncExec cbExec; + auto backend = std::make_unique(); + auto* rawBackend = backend.get(); + morph::bridge::Bridge bridge{std::move(backend)}; + auto handler = std::make_unique>(bridge, &cbExec); + + REQUIRE_NOTHROW(handler->execute(ARTouch{.id = 12, .amount = 4})); + REQUIRE(rawBackend->pendingCount() == 1); + + // Destroy only the handler (and, with it, the binding's last strong + // reference); the Bridge itself stays alive. attachHandlerAsync's success + // callback holds only a weak_ptr to the binding, so completing the reply + // now must be a no-op rather than dereferencing a freed HandlerBinding. + handler.reset(); + + REQUIRE_NOTHROW(rawBackend->completeNext()); + SUCCEED("completing an attach reply after only the binding is gone did not crash"); +} + +TEST_CASE("ensureBoundAsync's out-of-frame success callback is a no-op once the Bridge is gone", + "[bridge][registration][shared-instances][issue26]") { + SyncExec cbExec; + // See the identical attachHandlerAsync test above: the backend must + // outlive the Bridge, so it is co-owned via AsyncBackendShim. + auto sharedBackend = std::make_shared(); + auto bridge = std::make_unique(std::make_unique(sharedBackend)); + auto handler = std::make_unique>(*bridge, &cbExec); + + REQUIRE_NOTHROW(handler->execute(ARKeyedCreate{.initial = 6})); + REQUIRE(sharedBackend->pendingCount() == 1); + + handler.reset(); + bridge.reset(); + + REQUIRE_NOTHROW(sharedBackend->completeNext()); + SUCCEED("completing a registerModelSharedAsync reply after the Bridge and handler are both gone did not crash"); +} + +TEST_CASE("ensureBoundAsync's out-of-frame success callback is a no-op once only the binding is gone", + "[bridge][registration][shared-instances][issue26]") { + SyncExec cbExec; + auto backend = std::make_unique(); + auto* rawBackend = backend.get(); + morph::bridge::Bridge bridge{std::move(backend)}; + auto handler = std::make_unique>(bridge, &cbExec); + + REQUIRE_NOTHROW(handler->execute(ARKeyedCreate{.initial = 7})); + REQUIRE(rawBackend->pendingCount() == 1); + + handler.reset(); + + REQUIRE_NOTHROW(rawBackend->completeNext()); + SUCCEED("completing a registerModelSharedAsync reply after only the binding is gone did not crash"); +} From 505d7e593fe5a503422558d7e7b12636b022d30f Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Thu, 13 Aug 2026 09:48:29 +0300 Subject: [PATCH 18/20] tests: cover ensureBound's already-bound no-op and ensureBoundAsync's inline-onError path Two more of codecov's flagged lines were real gaps: ensureBound (the synchronous counterpart to ensureBoundAsync) had no test calling it twice on the same binding to exercise its already-bound early return, and ensureBoundAsync's error callback's parkIfInFrame no-op was untested even though attachHandlerAsync's identical branch already was. Also rewords the two 'binding is gone' test names/comments added in the previous commit: reading BridgeHandler::execute's dispatch closely shows the binding itself is pinned by the pending onDone closure independent of the BridgeHandler, so destroying just the handler never actually drops the binding's last strong reference through this path -- the tests still verify a real case (dropping the handler while an attach is in flight must not crash), just not the specific 'binding is gone' branch their original names claimed. Co-Authored-By: Claude Sonnet 5 --- tests/test_async_registration.cpp | 77 +++++++++++++++++++++++++++---- 1 file changed, 69 insertions(+), 8 deletions(-) diff --git a/tests/test_async_registration.cpp b/tests/test_async_registration.cpp index 768fbb43..828bb903 100644 --- a/tests/test_async_registration.cpp +++ b/tests/test_async_registration.cpp @@ -1543,7 +1543,7 @@ TEST_CASE("attachHandlerAsync's out-of-frame success callback is a no-op once th SUCCEED("completing an attach reply after the Bridge and handler are both gone did not crash"); } -TEST_CASE("attachHandlerAsync's out-of-frame success callback is a no-op once only the binding is gone", +TEST_CASE("attachHandlerAsync's out-of-frame success callback tolerates the BridgeHandler being gone", "[bridge][registration][shared-instances][issue26]") { SyncExec cbExec; auto backend = std::make_unique(); @@ -1554,14 +1554,17 @@ TEST_CASE("attachHandlerAsync's out-of-frame success callback is a no-op once on REQUIRE_NOTHROW(handler->execute(ARTouch{.id = 12, .amount = 4})); REQUIRE(rawBackend->pendingCount() == 1); - // Destroy only the handler (and, with it, the binding's last strong - // reference); the Bridge itself stays alive. attachHandlerAsync's success - // callback holds only a weak_ptr to the binding, so completing the reply - // now must be a no-op rather than dereferencing a freed HandlerBinding. + // Destroy only the handler; the Bridge itself stays alive. Note that the + // binding itself does *not* actually go away here: execute()'s dispatch + // copies `_binding` into a local held by this very completion's own + // onDone closure (see BridgeHandler::execute), so the pending dispatch + // keeps it alive independent of the BridgeHandler. This still exercises a + // real case worth having a test for -- a caller that drops its handler + // while a keyed attach is in flight must not crash when the reply lands. handler.reset(); REQUIRE_NOTHROW(rawBackend->completeNext()); - SUCCEED("completing an attach reply after only the binding is gone did not crash"); + SUCCEED("completing an attach reply after the BridgeHandler is gone did not crash"); } TEST_CASE("ensureBoundAsync's out-of-frame success callback is a no-op once the Bridge is gone", @@ -1583,7 +1586,7 @@ TEST_CASE("ensureBoundAsync's out-of-frame success callback is a no-op once the SUCCEED("completing a registerModelSharedAsync reply after the Bridge and handler are both gone did not crash"); } -TEST_CASE("ensureBoundAsync's out-of-frame success callback is a no-op once only the binding is gone", +TEST_CASE("ensureBoundAsync's out-of-frame success callback tolerates the BridgeHandler being gone", "[bridge][registration][shared-instances][issue26]") { SyncExec cbExec; auto backend = std::make_unique(); @@ -1594,8 +1597,66 @@ TEST_CASE("ensureBoundAsync's out-of-frame success callback is a no-op once only REQUIRE_NOTHROW(handler->execute(ARKeyedCreate{.initial = 7})); REQUIRE(rawBackend->pendingCount() == 1); + // See attachHandlerAsync's identical test above: the binding itself stays + // alive here (pinned by the pending dispatch's own onDone closure), but + // dropping the handler while the reply is still in flight is still a real + // case worth covering. handler.reset(); REQUIRE_NOTHROW(rawBackend->completeNext()); - SUCCEED("completing a registerModelSharedAsync reply after only the binding is gone did not crash"); + SUCCEED("completing a registerModelSharedAsync reply after the BridgeHandler is gone did not crash"); +} + +TEST_CASE("ensureBound is a no-op when the binding already has an instance", + "[bridge][registration][shared-instances][issue26]") { + // Bridge::ensureBound is the synchronous counterpart to ensureBoundAsync, + // used directly (not through BridgeHandler::execute) when a caller wants + // to force-bind an anonymous instance ahead of time. Calling it twice on + // the same binding exercises its already-bound early-return: the second + // call must not register a second instance. + morph::exec::ThreadPoolExecutor pool{2}; + morph::bridge::Bridge bridge{std::make_unique(pool)}; + + auto binding = std::make_shared(); + binding->typeId = "AR_KeyedModel"; + binding->modelFactory = [] { return morph::model::detail::ModelFactory::create(); }; + + REQUIRE_NOTHROW(bridge.ensureBound(binding)); + auto const firstId = binding->currentId.load(); + REQUIRE(firstId != 0U); + + REQUIRE_NOTHROW(bridge.ensureBound(binding)); + CHECK(binding->currentId.load() == firstId); +} + +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 + // dispatch call (which then returns true) exercises the parkIfInFrame + // no-op inside ensureBoundAsync's error callback, not just its success one. + SyncExec cbExec; + auto backend = std::make_unique(std::optional{"disconnected"}); + morph::bridge::Bridge bridge{std::move(backend)}; + morph::bridge::BridgeHandler handler{bridge, &cbExec}; + + std::optional> pending; + REQUIRE_NOTHROW(pending.emplace(handler.execute(ARKeyedCreate{.initial = 8}))); + + std::string message; + int errorCount = 0; + std::atomic succeeded{false}; + pending->then([&](ARKeyedCreated) { succeeded.store(true); }).onError([&](const std::exception_ptr& err) { + ++errorCount; + try { + std::rethrow_exception(err); + } catch (const std::exception& exc) { + message = exc.what(); + } + }); + + CHECK(message == "disconnected"); + CHECK(errorCount == 1); + CHECK_FALSE(succeeded.load()); + CHECK_FALSE(handler.primary().has_value()); } From 19dedbbd8c9835d997b66f004b176ab3c6b19c2b Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Thu, 13 Aug 2026 10:11:49 +0300 Subject: [PATCH 19/20] tests: cover execute()'s throwing-key path and parkIfInFrame's double-fire guard Two more of codecov's flagged lines were real: execute()'s try/catch around ActionKeyTraits::key(action) had no test whose key() throws, and detail::parkIfInFrame's 'handoff.fired' branch (a backend violating its documented one-callback-per-dispatch contract) had no test backend that actually violates it. Adds ARThrowingKeyTouch (a payload-keyed action whose ActionKeyTraits specialization throws from key(), written directly rather than via BRIDGE_KEY_FROM since that macro's generated body can't be made to throw) and DoubleFiringBackend (calls attachModelAsync's onRegistered twice inline, on purpose), plus one test each confirming execute()/attachHandlerAsync still resolve exactly once through onError/onDone rather than letting the second call through or escaping the no-throw contract. Co-Authored-By: Claude Sonnet 5 --- tests/test_async_registration.cpp | 104 ++++++++++++++++++++++++++++++ 1 file changed, 104 insertions(+) diff --git a/tests/test_async_registration.cpp b/tests/test_async_registration.cpp index 828bb903..6697e221 100644 --- a/tests/test_async_registration.cpp +++ b/tests/test_async_registration.cpp @@ -67,6 +67,13 @@ struct ARKeyedCreate { int initial = 0; }; +/// Payload-keyed, like ARTouch, but its ActionKeyTraits::key() below throws -- +/// exercises execute()'s "key extraction is user code" guard (the try/catch +/// around ActionKeyTraits::key(action), ahead of the attach dispatch). +struct ARThrowingKeyTouch { + int amount = 0; +}; + struct ARKeyedModel { int value = 0; int execute(const ARTouch& act) { @@ -77,6 +84,10 @@ struct ARKeyedModel { value = act.initial; return {.id = 4242, .value = value}; } + int execute(const ARThrowingKeyTouch& act) { + value += act.amount; + return value; + } }; } // namespace @@ -124,6 +135,27 @@ struct morph::model::ModelTraits { static constexpr std::string_view typeId() { return "AR_KeyedModel"; } }; +template <> +struct morph::model::ActionTraits { + using Result = int; + static constexpr std::string_view typeId() { return "AR_ThrowingKeyTouch"; } + static std::string toJson(const ARThrowingKeyTouch& act) { return R"({"amount":)" + std::to_string(act.amount) + "}"; } + static ARThrowingKeyTouch fromJson(std::string_view /*json*/) { return {}; } + static std::string resultToJson(const int& res) { return std::to_string(res); } + static int resultFromJson(std::string_view text) { return std::stoi(std::string{text}); } +}; + +// Written directly rather than via BRIDGE_KEY_FROM: that macro's generated +// key() body cannot be made to throw, which is the entire point of this type. +template <> +struct morph::model::ActionKeyTraits { + static constexpr bool hasKey = true; + static constexpr bool fromResult = false; + static std::string key(const ARThrowingKeyTouch&) { + throw std::runtime_error("key extraction failed"); + } +}; + BRIDGE_MODEL_KEY(ARKeyedModel, ARTouch, &ARTouch::id); BRIDGE_KEY_FROM_RESULT(ARKeyedCreate, &ARKeyedCreated::id); @@ -364,6 +396,26 @@ class ThrowingDispatchBackend : public AsyncRegisterBackend { } }; +// 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. +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; + } +}; + // Shim so a Bridge (which takes ownership of a unique_ptr) can hold a backend // the test also keeps a shared_ptr to -- making it co-owned / able to outlive // the Bridge (see test_bridge_lifetime.cpp's identical BackendShim). Also lets @@ -1660,3 +1712,55 @@ TEST_CASE("ensureBoundAsync's onError path is a no-op once the dispatching frame CHECK_FALSE(succeeded.load()); CHECK_FALSE(handler.primary().has_value()); } + +TEST_CASE("execute() surfaces a throwing ActionKeyTraits::key() through onError instead of escaping", + "[bridge][registration][shared-instances][issue26]") { + // Key extraction runs ahead of the attach dispatch, on execute()'s own + // stack -- a throwing key() must resolve the returned Completion's + // onError, matching every other keyed-dispatch failure, rather than + // throwing out of execute() itself. + SyncExec cbExec; + morph::bridge::Bridge bridge{std::make_unique()}; + morph::bridge::BridgeHandler handler{bridge, &cbExec}; + + std::string message; + std::atomic succeeded{false}; + REQUIRE_NOTHROW(handler.execute(ARThrowingKeyTouch{.amount = 1}) + .then([&](int) { succeeded.store(true); }) + .onError([&](const std::exception_ptr& err) { + try { + std::rethrow_exception(err); + } catch (const std::exception& exc) { + message = exc.what(); + } + })); + + CHECK(message == "key extraction failed"); + CHECK_FALSE(succeeded.load()); + CHECK_FALSE(handler.primary().has_value()); +} + +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 + // 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) + // twice for a single dispatch. + SyncExec cbExec; + morph::bridge::Bridge bridge{std::make_unique()}; + morph::bridge::BridgeHandler handler{bridge, &cbExec}; + + int completions = 0; + std::atomic result{-1}; + REQUIRE_NOTHROW(handler.execute(ARTouch{.id = 21, .amount = 6}) + .then([&](int val) { + ++completions; + result.store(val); + }) + .onError([&](const std::exception_ptr&) { ++completions; })); + + CHECK(completions == 1); + CHECK(result.load() == 6); + CHECK(handler.primary().value_or(-1) == 21); +} From 18c16f22193ec87f7e5b49d43a1621417ff692a9 Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Thu, 13 Aug 2026 10:16:03 +0300 Subject: [PATCH 20/20] tests: genuinely reach attachHandlerAsync/ensureBoundAsync's 'binding is gone' branch The earlier 'binding is gone' tests went through BridgeHandler::execute, whose own dispatch closure captures the binding by value -- so the binding never actually died while that dispatch was in flight (already noted in their comments). Calling attachHandlerAsync/ensureBoundAsync directly with an onDone that captures nothing binding-related removes that hidden strong reference, so dropping the test's own shared_ptr before completing the reply now genuinely exercises weakBinding.lock() failing. Co-Authored-By: Claude Sonnet 5 --- tests/test_async_registration.cpp | 58 +++++++++++++++++++++++++++++++ 1 file changed, 58 insertions(+) diff --git a/tests/test_async_registration.cpp b/tests/test_async_registration.cpp index 6697e221..6741dd8b 100644 --- a/tests/test_async_registration.cpp +++ b/tests/test_async_registration.cpp @@ -1764,3 +1764,61 @@ TEST_CASE("attachHandlerAsync reports exactly once even when the backend fires i CHECK(result.load() == 6); CHECK(handler.primary().value_or(-1) == 21); } + +TEST_CASE("attachHandlerAsync's out-of-frame success callback is a genuine no-op once the binding itself is gone", + "[bridge][registration][shared-instances][issue26]") { + // The other attachHandlerAsync/ensureBoundAsync "binding is gone" tests + // above go through BridgeHandler::execute, whose own dispatch closure + // captures the binding by value -- so the binding never actually dies + // while that dispatch is in flight (see those tests' comments). Calling + // attachHandlerAsync directly, with an onDone that captures nothing + // binding-related, removes that hidden strong reference: dropping the + // test's own shared_ptr before completing the reply is what actually + // exercises weakBinding.lock() failing. + auto backend = std::make_unique(); + auto* rawBackend = backend.get(); + morph::bridge::Bridge bridge{std::move(backend)}; + + std::weak_ptr weakBinding; + { + auto binding = std::make_shared(); + binding->typeId = "AR_KeyedModel"; + binding->modelFactory = [] { return morph::model::detail::ModelFactory::create(); }; + weakBinding = binding; + + std::atomic onDoneFired{false}; + bridge.template attachHandlerAsync(binding, "13", + [&onDoneFired](std::exception_ptr) { onDoneFired.store(true); }); + REQUIRE(rawBackend->pendingCount() == 1); + // `binding` (the only remaining strong reference, now that onDone + // captures none) goes out of scope at the end of this block. + } + REQUIRE(weakBinding.expired()); + + REQUIRE_NOTHROW(rawBackend->completeNext()); + SUCCEED("completing an attach reply after the binding itself is gone did not crash"); +} + +TEST_CASE("ensureBoundAsync's out-of-frame success callback is a genuine no-op once the binding itself is gone", + "[bridge][registration][shared-instances][issue26]") { + // Mirrors attachHandlerAsync's identical direct-call test above. + auto backend = std::make_unique(); + auto* rawBackend = backend.get(); + morph::bridge::Bridge bridge{std::move(backend)}; + + std::weak_ptr weakBinding; + { + auto binding = std::make_shared(); + binding->typeId = "AR_KeyedModel"; + binding->modelFactory = [] { return morph::model::detail::ModelFactory::create(); }; + weakBinding = binding; + + std::atomic onDoneFired{false}; + bridge.ensureBoundAsync(binding, [&onDoneFired](std::exception_ptr) { onDoneFired.store(true); }); + REQUIRE(rawBackend->pendingCount() == 1); + } + REQUIRE(weakBinding.expired()); + + REQUIRE_NOTHROW(rawBackend->completeNext()); + SUCCEED("completing a registerModelSharedAsync reply after the binding itself is gone did not crash"); +}