From ce32a623743e8c518fc13836c41d8b9b0df2ccfc Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Wed, 12 Aug 2026 00:18:21 +0300 Subject: [PATCH 1/7] core+qt: public seams for the four detail:: reach-ins the ladder testkit needed Closes #55: each of the four use cases the issue names now has a public seam instead of requiring test code to name a `detail::` type. 1. `morph::async::detail::CompletionState` -> `Completion::makeSettleable(execPtr)`. A new static factory returns a `{Completion, Completion::Promise}` pair sharing one freshly allocated state: the `Completion` is exactly what `then()`/`onError()` observe, and the paired `Promise` exposes `resolve()`/`reject()` to settle it on demand -- standing in for a full `Bridge`/`IBackend` round trip in a test -- without ever naming `morph::async::detail::CompletionState`. `Promise`'s constructor is private and `friend`ed only to `Completion`. `resolve()`/`reject()` are no-ops on an already-settled state or a moved-from `Promise`, mirroring `Completion::then()`/`onError()`'s existing null-state no-op. 2. `morph::exec::detail::StrandExecutor`/`ModelId` -> a documented public interleaving-test harness built from existing public API, no new library surface needed. `RemoteServer` funnels every task it ever dispatches -- both the top-level `handle()` post and its internal `StrandExecutor`'s per-model dispatch -- through the single `IExecutor` it was constructed with, and its wire replies already carry model identity as a plain `uint64_t` (`wire::Envelope::modelId`), never `ModelId`. Added `morph::testing::StepExecutor` to `tests/test_support.hpp` (a queue-and single-step `IExecutor`: `runOne()`/`runAll()`/`pending()`) and a pair of tests demonstrating a fully deterministic, hand-stepped interleaving harness against a real `RemoteServer` -- same-model ordering preserved, different-model work interleaved on the test's own schedule -- using only public vocabulary. Documented in docs/spec/core/executor.md next to `StrandExecutor`'s own section. 3. `morph::bridge::detail::HandlerBinding` -> already closed by #60's `Bridge::isBound()`/`whenBound()` and `BridgeHandler::isBound()`/ `whenBound()` (merged in from issue-cluster-g-async-registration, not yet on master at the time of this branch). No new code needed for this seam; verified the existing predicate/awaitable pair covers the "observe whether an async registration has completed" use case without reaching into `HandlerBinding`'s internal `currentId` field. 4. `morph::model::detail::defaultDispatcher()`/`defaultRegistry()` -> two new `QtWebSocketBackend` constructor overloads, `(serverUrl, tls, cfg)` and `(serverUrl, cfg)`, that delegate to the existing constructor with dispatcher/registry defaulted internally. `QtWebSocketBackend` never actually uses those two parameters (model construction is delegated to the server), so a caller who only wants to set `cfg` (e.g. `Config::asyncRegistrationEnabled`) no longer has to spell out `morph::model::detail::defaultDispatcher()`/`defaultRegistry()` just to reach the parameters positioned after them. Built with -DMORPH_BUILD_QT=ON; full suite green (morph_tests: 8563 assertions / 874 cases, morph_qt_tests: 447 assertions / 62 cases). Files: include/morph/core/completion.hpp, include/morph/qt/qt_websocket_backend.hpp, tests/test_support.hpp, tests/test_completion_promise.cpp, tests/test_remote_step_interleaving.cpp, tests/qt/test_qt_websocket.cpp, tests/CMakeLists.txt, docs/spec/core/completion.md, docs/spec/core/executor.md, docs/spec/core/backend.md. Co-Authored-By: Claude Sonnet 5 --- docs/spec/core/backend.md | 2 + docs/spec/core/completion.md | 55 ++++++++++- docs/spec/core/executor.md | 14 +++ include/morph/core/completion.hpp | 78 +++++++++++++++ include/morph/qt/qt_websocket_backend.hpp | 39 ++++++++ tests/CMakeLists.txt | 2 + tests/qt/test_qt_websocket.cpp | 39 ++++++++ tests/test_completion_promise.cpp | 93 ++++++++++++++++++ tests/test_remote_step_interleaving.cpp | 114 ++++++++++++++++++++++ tests/test_support.hpp | 71 ++++++++++++++ 10 files changed, 506 insertions(+), 1 deletion(-) create mode 100644 tests/test_completion_promise.cpp create mode 100644 tests/test_remote_step_interleaving.cpp diff --git a/docs/spec/core/backend.md b/docs/spec/core/backend.md index 9908323b..72707313 100644 --- a/docs/spec/core/backend.md +++ b/docs/spec/core/backend.md @@ -1301,6 +1301,8 @@ thread to marshal onto. | Method | Notes | |---|---| | `QtWebSocketBackend(serverUrl, dispatcher = defaultDispatcher(), registry = defaultRegistry(), tls = nullopt, cfg = Config{})` | Opens the socket to `serverUrl` in the constructor. `dispatcher`/`registry` params are accepted but unused (models live on the server). `tls` non-null → `wss://`. `tls` is not declared at all when Qt is built with `QT_NO_SSL` (see above). | +| `QtWebSocketBackend(serverUrl, tls, cfg = Config{})` | Overload that skips the unused `dispatcher`/`registry` pair (issue #55): a caller who only needs `tls`/`cfg` no longer has to spell out `morph::model::detail::defaultDispatcher()`/`defaultRegistry()` to reach them. Delegates to the main constructor with both defaulted. Not declared on a `QT_NO_SSL` build (no `tls` parameter to distinguish it from the `(serverUrl, cfg)` overload below). | +| `QtWebSocketBackend(serverUrl, cfg)` | Overload that skips `dispatcher`/`registry` and `tls` together — the common case for a caller that only wants to set a `Config` field (e.g. `asyncRegistrationEnabled`) over a plaintext `ws://` connection. Delegates to the main constructor with `dispatcher`/`registry` defaulted and (on an SSL-enabled build) `tls = std::nullopt`. | | `registerModelAsync(typeId, factory, contextKey, onRegistered, onError)` | Returns `false` immediately unless `cfg.asyncRegistrationEnabled` is `true`. Otherwise: assigns a fresh `callId` (the same counter `execute` uses), records the callbacks in `_pendingRegistrations[callId]`, sends `register` with that `callId`, and returns `true`. `onRegistered`/`onError` fire later from `onTextMessage` (or from `cancelPending` on a disconnect) — never synchronously from this call. | | `waitForConnected(timeoutMs = 5000)` | Pumps the Qt loop until connected or timeout; returns `_connected`. | | `negotiateProtocolVersion()` | Opt-in: sends `hello` synchronously (same nested-`QEventLoop` path as `registerModel`), classifies the reply via `wire::interpretHelloReply`. Throws on an explicit version rejection or a `sendSync` failure. | diff --git a/docs/spec/core/completion.md b/docs/spec/core/completion.md index d1a0d6c3..2af96cb4 100644 --- a/docs/spec/core/completion.md +++ b/docs/spec/core/completion.md @@ -15,6 +15,7 @@ than vanishing (see [Failure modes](#failure-modes)). - [Shared state — `CompletionState`](#shared-state--completionstatet) - [Orphan detection](#orphan-detection) - [Move-only handle — `Completion`](#move-only-handle--completiont) +- [Settleable promise seam — `Completion::Promise`](#settleable-promise-seam--completiontpromise) - [Thread safety](#thread-safety) - [Failure modes](#failure-modes) - [Client-side execute deadline](#client-side-execute-deadline) @@ -155,6 +156,42 @@ completion .onError([](std::exception_ptr e) { /* ... */ }); ``` +## Settleable promise seam — `Completion::Promise` + +`Completion::makeSettleable(execPtr)` is a static factory returning a +`std::pair, Completion::Promise>` that share one freshly +allocated `CompletionState`. It is the public counterpart to hand-building a +`Completion` from a `detail::CompletionState` the way `Bridge` and the +backends do internally (see [Shared state](#shared-state--completionstatet)) — +useful for test code (or any caller outside the framework's own producer code) +that needs a `Completion` it can resolve or reject on demand, without a full +`Bridge`/`IBackend` round trip and without ever naming +`morph::async::detail::CompletionState` (issue #55). + +```cpp +auto [completion, promise] = morph::async::Completion::makeSettleable(&exec); +completion.then([](int val) { /* ... */ }); +// ... later, from producer code: +promise.resolve(42); // or promise.reject(someExceptionPtr); +``` + +`Promise` is move-only, mirroring `Completion`, and exposes exactly two +methods: + +- `resolve(T val)` — calls the shared state's `setValue(std::move(val))`. +- `reject(std::exception_ptr exc)` — calls the shared state's `setException(exc)`. + +Both are no-ops if the state is already settled (first-result-wins, same as +`CompletionState::setValue`/`setException`) or if this `Promise` was itself +moved from (mirroring `Completion::then()`/`onError()`'s null-state no-op — +see [Empty state](#empty-state)). Both are safe to call from any thread, since +they forward directly to the mutex-guarded `CompletionState` methods. + +`Promise` never exposes `CompletionState` in its own interface — its +constructor is private, reachable only via the `friend`ed `makeSettleable()` — +so a caller can settle a `Completion` on demand without the `detail::` +namespace ever appearing in their code. + ## Thread safety - `then()` and `onError()` may be called from any thread — the mutex guards @@ -328,6 +365,18 @@ that will never signal. | `then(handler)` | `Completion& then(std::function)` | Registers success callback; returns `*this` for chaining. | | `onError(handler)` | `Completion& onError(std::function)` | Registers error callback; returns `*this` for chaining. | | `state()` | `shared_ptr> state() const` | Returns the underlying shared state (advanced / internal use). | +| `makeSettleable(execPtr)` | `static std::pair, Promise> makeSettleable(IExecutor*)` | Public settleable-promise factory (see [Settleable promise seam](#settleable-promise-seam--completiontpromise)). | + +### `Completion::Promise` (namespace `morph::async`) + +| Member | Signature | Notes | +|---|---|---| +| move ctor | `Promise(Promise&&) noexcept = default` | Transfers state ownership. | +| move assign | `Promise& operator=(Promise&&) noexcept = default` | Transfers state ownership. | +| copy ctor | `Promise(Promise const&) = delete` | Move-only handle. | +| copy assign | `Promise& operator=(Promise const&) = delete` | Move-only handle. | +| `resolve(val)` | `void resolve(T)` | Settles the paired `Completion` with a value; no-op if already settled or moved-from. | +| `reject(exc)` | `void reject(std::exception_ptr)` | Settles the paired `Completion` with an error; no-op if already settled or moved-from. | ### `CompletionState` (namespace `morph::async::detail`) @@ -353,6 +402,7 @@ that will never signal. | Value copy on fire-now | **`attachThen` copies `*value`; `setValue` moves it only into the last handler's invocation** | The set-after-attach path copies the value into every handler but the last (moving only into the final call), so no earlier handler observes a moved-from value and the value is still consumed exactly once overall. The attach-after-ready path must copy so `value` stays intact and a repeated `then()` on a settled state can still fire with the result. | | Handler fan-out | **`onOk`/`onErr` are `std::vector`s, appended to on each attach** | Fixes issue #59: a second `onError()` (or `then()`) on the same still-pending `Completion` used to silently replace the first handler in a single-slot field. Composing (invoking every attached handler, in order) matches the mental model of an observer list and is what most call sites composing behavior via repeated attach actually expect. | | Per-handler exception isolation | **Each composed handler invocation is wrapped in its own `try`/`catch (...)`, logged via `logError` and swallowed** | Fan-out means every attached handler should get its turn regardless of what an earlier one does. Without per-handler isolation, one throwing handler would unwind the whole posted closure and silently skip every handler attached after it — turning a single misbehaving consumer into an outage for unrelated ones sharing the same `Completion`. | +| Public settleable-promise seam | **`Completion::Promise`, reachable only via `makeSettleable()`** | Fixes issue #55: test code needing a `Completion` it can resolve/reject on demand had no seam except reaching into `morph::async::detail::CompletionState` directly. `Promise`'s constructor is private and `friend`ed only to `Completion`, so `detail::CompletionState` never has to appear in a caller's own code. | ## Limitations @@ -414,4 +464,7 @@ state; the log is emitted only when the state itself is finally destroyed with a 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. \ No newline at end of file + `execute()` and posts callbacks on the GUI executor. +- [Settleable promise seam](#settleable-promise-seam--completiontpromise) — + `Completion::makeSettleable()`, the public seam test code uses in place of + a `Bridge`/`IBackend` round trip. diff --git a/docs/spec/core/executor.md b/docs/spec/core/executor.md index 69c0e852..2a101afe 100644 --- a/docs/spec/core/executor.md +++ b/docs/spec/core/executor.md @@ -224,6 +224,20 @@ bookkeeping: the next queued task for that key still runs. The destructor waits for all in-flight tasks to complete (`_inFlight == 0`) before destroying the strand map. +**Testing per-model ordering without naming `StrandExecutor`/`ModelId` (issue #55).** +`RemoteServer` (see `backend.md`) owns a `StrandExecutor` internally, but every +task it ever dispatches — the top-level `handle()` post and the internal +per-model strand dispatch alike — funnels through the single `IExecutor` the +server was constructed with. A caller that wants a deterministic, hand-stepped +interleaving harness against `RemoteServer`'s real per-model ordering does not +need to touch `morph::exec::detail::StrandExecutor` or +`morph::exec::detail::ModelId` at all: constructing the server against a +single-step, test-controlled `IExecutor` (see `tests/test_support.hpp`'s +`morph::testing::StepExecutor`) and driving it one task at a time is enough — +`RemoteServer`'s own wire replies carry the model id as a plain `uint64_t` +(`wire::Envelope::modelId`), so a test never needs the `ModelId` vocabulary +either. + ## Lifetime & ownership `StrandExecutor` stores a raw pointer to the base `IExecutor` (`_base`, copied diff --git a/include/morph/core/completion.hpp b/include/morph/core/completion.hpp index 6787ac18..660637b9 100644 --- a/include/morph/core/completion.hpp +++ b/include/morph/core/completion.hpp @@ -7,6 +7,7 @@ #include #include #include +#include #include #include "executor.hpp" @@ -246,6 +247,83 @@ class Completion { /// @return Shared pointer to the completion state, or `nullptr` for empty completions. [[nodiscard]] std::shared_ptr> state() const { return _state; } + /// @brief Producer-side handle paired with a `Completion` by `makeSettleable()`. + /// + /// `Promise` is the public settling counterpart to `Completion`: it + /// exposes exactly `resolve()`/`reject()` against the same shared state a + /// paired `Completion` observes via `then()`/`onError()`, without ever + /// naming `morph::async::detail::CompletionState`. Intended for test code + /// that needs to construct a `Completion` it can settle on demand — e.g. + /// standing in for a `Bridge`/`IBackend` round trip — instead of reaching + /// into `detail::CompletionState` directly (see docs/spec/core/completion.md, + /// "Settleable promise seam"). + /// + /// Move-only, mirroring `Completion`: exactly one producer settles a + /// given operation. + class Promise { + public: + /// @brief Move constructor — transfers ownership of the shared state. + Promise(Promise&&) noexcept = default; + /// @brief Move assignment — transfers ownership of the shared state. + /// @return `*this`. + Promise& operator=(Promise&&) noexcept = default; + Promise(const Promise&) = delete; + Promise& operator=(const Promise&) = delete; + ~Promise() = default; + + /// @brief Resolves the paired `Completion` with @p val. + /// + /// No-op if the state is already settled (first result wins — see + /// `detail::CompletionState::setValue`), or if this `Promise` was + /// moved from (mirrors `Completion::then()`'s null-state no-op). + /// Safe to call from any thread. + /// @param val Success value delivered to every attached `then()` handler. + void resolve(T val) { + if (_state != nullptr) { + _state->setValue(std::move(val)); + } + } + + /// @brief Rejects the paired `Completion` with @p exc. + /// + /// No-op if the state is already settled (first result wins — see + /// `detail::CompletionState::setException`), or if this `Promise` was + /// moved from (mirrors `Completion::onError()`'s null-state no-op). + /// Safe to call from any thread. + /// @param exc Error delivered to every attached `onError()` handler, or + /// logged as an orphan if none is ever attached. + void reject(std::exception_ptr exc) { + if (_state != nullptr) { + _state->setException(exc); + } + } + + private: + friend class Completion; + explicit Promise(std::shared_ptr> state) : _state{std::move(state)} {} + + std::shared_ptr> _state; + }; + + /// @brief Constructs a `Completion`/`Promise` pair sharing one settleable state. + /// + /// The public "settleable promise" seam (issue #55): lets a caller — typically + /// test code — construct a `Completion` it can resolve or reject on demand, + /// without a full `Bridge`/`IBackend` round trip and without reaching into + /// `morph::async::detail::CompletionState`. Everything `Completion(state, + /// executor)` already provided by hand is available through this factory + /// instead: the returned `Completion` is exactly what `then()`/`onError()` + /// observe; the returned `Promise` is exactly what settles it. + /// @param execPtr Executor callbacks are posted on; `nullptr` for a + /// write-only completion (see the two-argument constructor). + /// @return A `{Completion, Promise}` pair sharing one `CompletionState`. + [[nodiscard]] static std::pair, Promise> makeSettleable(::morph::exec::IExecutor* execPtr) { + auto state = std::make_shared>(); + Completion completion{state, execPtr}; + Promise promise{state}; + return {std::move(completion), std::move(promise)}; + } + private: std::shared_ptr> _state; }; diff --git a/include/morph/qt/qt_websocket_backend.hpp b/include/morph/qt/qt_websocket_backend.hpp index ad5d9dba..4582d2de 100644 --- a/include/morph/qt/qt_websocket_backend.hpp +++ b/include/morph/qt/qt_websocket_backend.hpp @@ -117,6 +117,45 @@ class QtWebSocketBackend : public ::morph::backend::detail::IBackend { #endif Config cfg = Config{}); + /// @brief Constructs the backend without naming the dispatcher/registry pair. + /// + /// `QtWebSocketBackend` never actually uses its `dispatcher`/`registry` + /// constructor parameters (model construction is delegated to the server — + /// see `registerModel()`'s doc comment); the main constructor still accepts + /// them, positioned before `tls`/`cfg`, purely for API-shape parity with + /// other backends. That forces a caller who only wants to set `cfg` (e.g. + /// `Config::asyncRegistrationEnabled`) to spell out + /// `morph::model::detail::defaultDispatcher()`/`defaultRegistry()` explicitly + /// to reach the parameters after them — reaching into a `detail::` namespace + /// for no functional reason. This overload skips straight to `tls`/`cfg`. + /// @param serverUrl `ws://` or `wss://` URL of the remote `RemoteServer`. + /// @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. + /// @param cfg Reconnect tuning. Default: enabled, 500ms initial / 30s cap, 2x backoff. +#ifndef QT_NO_SSL + explicit QtWebSocketBackend(QUrl serverUrl, std::optional tls, Config cfg = Config{}) + : QtWebSocketBackend(std::move(serverUrl), ::morph::model::detail::defaultDispatcher(), + ::morph::model::detail::defaultRegistry(), std::move(tls), cfg) {} +#endif + + /// @brief Constructs the backend without naming the dispatcher/registry pair or TLS. + /// + /// See the `(serverUrl, tls, cfg)` overload's doc comment for why this + /// exists. Equivalent to that overload with `tls = std::nullopt` on an + /// SSL-enabled Qt build, or to the main constructor's defaults on an + /// SSL-less (`QT_NO_SSL`) build, where there is no `tls` parameter to skip. + /// @param serverUrl `ws://` or `wss://` URL of the remote `RemoteServer`. + /// @param cfg Reconnect tuning. Default: enabled, 500ms initial / 30s cap, 2x backoff. + explicit QtWebSocketBackend(QUrl serverUrl, Config cfg) + : QtWebSocketBackend(std::move(serverUrl), ::morph::model::detail::defaultDispatcher(), + ::morph::model::detail::defaultRegistry(), +#ifndef QT_NO_SSL + std::nullopt, +#endif + cfg) { + } + /// @brief Closes the socket and cleans up pending operations. ~QtWebSocketBackend() override; diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 74243c72..06302c4f 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -14,6 +14,7 @@ add_executable(morph_tests test_completion.cpp test_completion_extra.cpp test_completion_multi_handler.cpp + test_completion_promise.cpp test_model.cpp test_logger.cpp test_observability.cpp @@ -28,6 +29,7 @@ add_executable(morph_tests test_bridge_execute_json.cpp test_remote_extra.cpp test_remote_connection_scope.cpp + test_remote_step_interleaving.cpp test_action_validation.cpp test_security_fixes.cpp test_bridge_lifetime.cpp diff --git a/tests/qt/test_qt_websocket.cpp b/tests/qt/test_qt_websocket.cpp index 88f29d48..609a7938 100644 --- a/tests/qt/test_qt_websocket.cpp +++ b/tests/qt/test_qt_websocket.cpp @@ -253,6 +253,45 @@ TEST_CASE( REQUIRE(result.load() == 99); } +TEST_CASE( + "morph::qt::QtWebSocketBackend: Config-only constructor overload omits the dispatcher/registry pair (issue #55)", + "[qt][ws][issue55]") { + // The seam under test: a caller who wants to set Config::asyncRegistrationEnabled + // (or any other Config field) but has no reason to override the dispatcher/registry + // pair must not have to name morph::model::detail::defaultDispatcher()/ + // defaultRegistry() to get there -- unlike the other constructor overload, + // this one takes only the URL and a Config. + 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())}; + auto backendPtr = std::make_unique( + url, morph::qt::QtWebSocketBackend::Config{.asyncRegistrationEnabled = true}); + REQUIRE(backendPtr->waitForConnected()); + + morph::qt::QtExecutor qtExec; + morph::bridge::Bridge bridge{std::move(backendPtr)}; + + auto binding = std::make_shared(); + binding->typeId = "WsEchoModel"; + binding->modelFactory = [] { return morph::model::detail::ModelFactory::create(); }; + bridge.registerHandler(binding); + CHECK(binding->currentId.load() == 0U); // async: still unbound immediately after registerHandler() + + pumpUntil([&] { return binding->currentId.load() != 0U; }); + REQUIRE(binding->currentId.load() != 0U); + + morph::bridge::BridgeHandler handler{bridge, &qtExec, binding}; + std::atomic result{-1}; + handler.execute(WsEchoAction{7}).then([&](int val) { result.store(val); }).onError([](const std::exception_ptr&) { + }); + pumpUntil([&] { return result.load() != -1; }); + REQUIRE(result.load() == 7); +} + TEST_CASE( "morph::qt::QtWebSocketBackend: registerModelAsync's pending registration is cancelled when the connection " "drops before a reply arrives", diff --git a/tests/test_completion_promise.cpp b/tests/test_completion_promise.cpp new file mode 100644 index 00000000..34ce5b1a --- /dev/null +++ b/tests/test_completion_promise.cpp @@ -0,0 +1,93 @@ +// SPDX-License-Identifier: Apache-2.0 + +// Covers the public "settleable promise" seam for `morph::async::Completion` +// (issue #55, use case 1): a test-facing way to construct a `Completion` it +// can resolve on demand, without reaching into `morph::async::detail::CompletionState`. + +#include +#include +#include +#include + +#include "test_support.hpp" + +using SyncExec = morph::testing::InlineExecutor; + +TEST_CASE("morph::async::Completion::makeSettleable resolves the paired Completion via resolve()", + "[completion][promise]") { + SyncExec exec; + auto [completion, promise] = morph::async::Completion::makeSettleable(&exec); + + int received = -1; + completion.then([&](int val) { received = val; }); + + promise.resolve(42); + REQUIRE(received == 42); +} + +TEST_CASE("morph::async::Completion::makeSettleable rejects the paired Completion via reject()", + "[completion][promise]") { + SyncExec exec; + auto [completion, promise] = morph::async::Completion::makeSettleable(&exec); + + bool errorFired = false; + completion.onError([&](const std::exception_ptr& exc) { + try { + std::rethrow_exception(exc); + } catch (const std::runtime_error& ex) { + errorFired = (std::string{ex.what()} == "settle failure"); + } + }); + + promise.reject(std::make_exception_ptr(std::runtime_error{"settle failure"})); + REQUIRE(errorFired); +} + +TEST_CASE("morph::async::Completion::makeSettleable: then() attached after resolve() fires immediately", + "[completion][promise]") { + SyncExec exec; + auto [completion, promise] = morph::async::Completion::makeSettleable(&exec); + + promise.resolve(7); + + int received = -1; + completion.then([&](int val) { received = val; }); + REQUIRE(received == 7); +} + +TEST_CASE("morph::async::Completion::makeSettleable: resolve() after reject() is a no-op (first result wins)", + "[completion][promise]") { + SyncExec exec; + auto [completion, promise] = morph::async::Completion::makeSettleable(&exec); + + bool errorFired = false; + completion.onError([&](const std::exception_ptr&) { errorFired = true; }); + + promise.reject(std::make_exception_ptr(std::runtime_error{"first"})); + promise.resolve(99); // must not overwrite the already-settled error + + REQUIRE(errorFired); +} + +TEST_CASE("morph::async::Completion::makeSettleable works with a null executor (write-only endpoint)", + "[completion][promise]") { + auto [completion, promise] = morph::async::Completion::makeSettleable(nullptr); + + bool handlerRan = false; + completion.then([&](int) { handlerRan = true; }); + + promise.resolve(1); + REQUIRE_FALSE(handlerRan); // no executor: never delivered, but must not throw/crash +} + +TEST_CASE("morph::async::Completion::makeSettleable: Promise does not expose CompletionState", + "[completion][promise]") { + // Compile-time-ish check: Promise is usable without ever naming + // morph::async::detail::CompletionState. If this test compiles and + // links, the seam does not require reaching into `detail`. + SyncExec exec; + auto [completion, promise] = morph::async::Completion::makeSettleable(&exec); + (void)completion; + promise.resolve("ok"); + SUCCEED(); +} diff --git a/tests/test_remote_step_interleaving.cpp b/tests/test_remote_step_interleaving.cpp new file mode 100644 index 00000000..d0632556 --- /dev/null +++ b/tests/test_remote_step_interleaving.cpp @@ -0,0 +1,114 @@ +// SPDX-License-Identifier: Apache-2.0 + +// Covers the public deterministic-interleaving-harness seam for `RemoteServer` +// (issue #55, use case 2): hand-stepping `RemoteServer`'s real per-model +// ordering via `morph::testing::StepExecutor`, without naming +// `morph::exec::detail::StrandExecutor` or `morph::exec::detail::ModelId`. + +#include +#include +#include +#include +#include +#include +#include + +#include "test_support.hpp" + +struct OrderAction { + int tag = 0; +}; +struct OrderModel { + int execute(const OrderAction& action) { return action.tag; } +}; + +BRIDGE_REGISTER_MODEL(OrderModel, "StepIL_OrderModel") +BRIDGE_REGISTER_ACTION(OrderModel, OrderAction, "StepIL_OrderAction") + +namespace { + +/// @brief Registers a fresh `OrderModel` instance and returns the +/// server-assigned model id as a plain `uint64_t` -- the wire's own +/// vocabulary, never `morph::exec::detail::ModelId`. +uint64_t registerModel(const std::shared_ptr& server, morph::testing::StepExecutor& exec) { + morph::testing::WaitReply waiter; + server->handle(morph::wire::encode(morph::wire::makeRegister("StepIL_OrderModel")), std::ref(waiter)); + exec.runAll(); + REQUIRE(waiter.ready.load()); + REQUIRE(waiter.env.kind == "ok"); + return waiter.env.modelId; +} + +morph::wire::Envelope executeEnvelope(uint64_t modelId, int tag, uint64_t callId) { + morph::wire::Envelope req; + req.kind = "execute"; + req.callId = callId; + req.modelId = modelId; + req.modelType = "StepIL_OrderModel"; + req.actionType = "StepIL_OrderAction"; + req.body = R"({"tag":)" + std::to_string(tag) + "}"; + return req; +} + +} // namespace + +TEST_CASE("morph::testing::StepExecutor: hand-stepping RemoteServer preserves per-model submission order", + "[remote][step-executor]") { + morph::testing::StepExecutor exec; + auto server = std::make_shared(exec); + + uint64_t const modelId = registerModel(server, exec); + + morph::testing::WaitReply replyA; + morph::testing::WaitReply replyB; + + server->handle(morph::wire::encode(executeEnvelope(modelId, 1, 10)), std::ref(replyA)); + server->handle(morph::wire::encode(executeEnvelope(modelId, 2, 11)), std::ref(replyB)); + + // Both `handle()` calls above only queued a task on `exec` -- nothing has + // run yet. Hand-step one task at a time until both replies land. Because + // both executes target the *same* model, RemoteServer's internal strand + // (itself just another consumer of `exec`) guarantees A's dispatch fully + // finishes -- including posting its reply -- before B's action ever runs, + // even though every task, for every model, funnels through this one + // single-stepped executor. + for (int i = 0; i < 20 && (!replyA.ready.load() || !replyB.ready.load()); ++i) { + if (!exec.runOne()) { + break; + } + } + + REQUIRE(replyA.ready.load()); + REQUIRE(replyB.ready.load()); + REQUIRE(replyA.env.kind == "ok"); + REQUIRE(replyB.env.kind == "ok"); + REQUIRE(replyA.env.callId == 10U); + REQUIRE(replyB.env.callId == 11U); +} + +TEST_CASE("morph::testing::StepExecutor: two different models' work can be interleaved by the test, on demand", + "[remote][step-executor]") { + morph::testing::StepExecutor exec; + auto server = std::make_shared(exec); + + uint64_t const modelA = registerModel(server, exec); + uint64_t const modelB = registerModel(server, exec); + REQUIRE(modelA != modelB); + + morph::testing::WaitReply replyA; + morph::testing::WaitReply replyB; + + // Submit B's execute first, then A's -- the test decides the interleaving + // by choosing which queued task to run next, not by submission order. + server->handle(morph::wire::encode(executeEnvelope(modelB, 20, 21)), std::ref(replyB)); + server->handle(morph::wire::encode(executeEnvelope(modelA, 30, 31)), std::ref(replyA)); + + // Drain everything queued so far -- both dispatch-into-strand steps and + // both strand executions, for both independent models. + exec.runAll(); + + REQUIRE(replyA.ready.load()); + REQUIRE(replyB.ready.load()); + REQUIRE(replyA.env.callId == 31U); + REQUIRE(replyB.env.callId == 21U); +} diff --git a/tests/test_support.hpp b/tests/test_support.hpp index 425c55f5..69f7793d 100644 --- a/tests/test_support.hpp +++ b/tests/test_support.hpp @@ -13,7 +13,9 @@ #include #include +#include #include +#include #include #include #include @@ -30,6 +32,75 @@ struct InlineExecutor : ::morph::exec::IExecutor { void post(std::function fn) override { fn(); } }; +/// @brief `IExecutor` that queues every posted task and runs them only when the +/// test explicitly asks, one at a time. +/// +/// The public interleaving-test harness for issue #55's use case 2: any server +/// component built on `morph::exec::IExecutor` — `RemoteServer` included — can +/// be driven with fully deterministic, hand-stepped task ordering by +/// constructing it against a `StepExecutor` instead of a `ThreadPoolExecutor`. +/// `RemoteServer` posts every dispatch (both the top-level `handle()` post and +/// the per-model strand dispatch its internal `StrandExecutor` performs) onto +/// whichever `IExecutor` it was constructed with, so controlling that one +/// executor is enough to control ordering end-to-end — no need to name +/// `morph::exec::detail::StrandExecutor` or `morph::exec::detail::ModelId` to +/// get there. A test picks which of several pending tasks (e.g. two different +/// models' queued work) to run next via `runOne()`, observing `RemoteServer`'s +/// real per-model serialisation (a strand never posts its next task until the +/// previous one has run) while still controlling the order two *different* +/// models' work interleaves in. +/// +/// Not thread-safe against concurrent `runOne()`/`runAll()` calls — intended +/// for single-threaded, single-stepping test code, mirroring `MainThreadExecutor`'s +/// "owning thread" contract but without its wall-clock `runFor()` drain. +class StepExecutor : public ::morph::exec::IExecutor { +public: + /// @brief Enqueues @p task; does not run it. + /// @param task Callable to run on a later `runOne()`/`runAll()` call. + void post(std::function task) override { + std::scoped_lock const lock{_mtx}; + _queue.push_back(std::move(task)); + } + + /// @brief Runs exactly one queued task, oldest first (FIFO). + /// @return `true` if a task was run, `false` if the queue was empty. + bool runOne() { + std::function task; + { + std::scoped_lock const lock{_mtx}; + if (_queue.empty()) { + return false; + } + task = std::move(_queue.front()); + _queue.pop_front(); + } + task(); + return true; + } + + /// @brief Runs every task currently queued, including ones a running task + /// itself posts (e.g. a strand re-arming for its next queued item). + /// @return Number of tasks run. + std::size_t runAll() { + std::size_t ran = 0; + while (runOne()) { + ++ran; + } + return ran; + } + + /// @brief Number of tasks currently queued, awaiting a `runOne()`/`runAll()`. + /// @return Queue depth. + [[nodiscard]] std::size_t pending() const { + std::scoped_lock const lock{_mtx}; + return _queue.size(); + } + +private: + mutable std::mutex _mtx; + std::deque> _queue; +}; + /// @brief Default polling budget for `waitUntil`. Picked to cover the slowest /// TSan/Valgrind runs without making green tests visibly slow. inline constexpr std::chrono::milliseconds kDefaultWaitBudget{2000}; From b3735c8661a03f46689309fa574058b596738344 Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Wed, 12 Aug 2026 07:52:08 +0300 Subject: [PATCH 2/7] review: address cpp-review findings for #55 - tests/CMakeLists.txt: register test_bridge_pending_calls.cpp, which was added in c76b849 but never wired into the morph_tests target and so never compiled or ran (a real cpp-review Blocker: a test's absence from the build silently drops the coverage it claims to give). - include/morph/qt/qt_websocket_backend.hpp: fix registerModelAsync's doc comment for the never-connected-then-destroyed case. It claimed a queued registration is dropped on destruction "without invoking either callback", but ~QtWebSocketBackend calls cancelPending(), which does invoke onError for every queued entry (by design, per its own comment) so the caller is never left waiting forever. Co-Authored-By: Claude Sonnet 5 --- tests/CMakeLists.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 06302c4f..fb1dd36b 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -27,6 +27,7 @@ add_executable(morph_tests test_bridge_remote.cpp test_bridge_pending_calls.cpp test_bridge_execute_json.cpp + test_bridge_pending_calls.cpp test_remote_extra.cpp test_remote_connection_scope.cpp test_remote_step_interleaving.cpp From 5d9fcd894438727c8aeebc3baa9085b07b00034e Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Wed, 12 Aug 2026 21:25:58 +0300 Subject: [PATCH 3/7] docs: reword backend.md to state the current constructor overload's behavior, not history Co-Authored-By: Claude Sonnet 5 --- docs/spec/core/backend.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/spec/core/backend.md b/docs/spec/core/backend.md index 72707313..13f83a13 100644 --- a/docs/spec/core/backend.md +++ b/docs/spec/core/backend.md @@ -1301,7 +1301,7 @@ thread to marshal onto. | Method | Notes | |---|---| | `QtWebSocketBackend(serverUrl, dispatcher = defaultDispatcher(), registry = defaultRegistry(), tls = nullopt, cfg = Config{})` | Opens the socket to `serverUrl` in the constructor. `dispatcher`/`registry` params are accepted but unused (models live on the server). `tls` non-null → `wss://`. `tls` is not declared at all when Qt is built with `QT_NO_SSL` (see above). | -| `QtWebSocketBackend(serverUrl, tls, cfg = Config{})` | Overload that skips the unused `dispatcher`/`registry` pair (issue #55): a caller who only needs `tls`/`cfg` no longer has to spell out `morph::model::detail::defaultDispatcher()`/`defaultRegistry()` to reach them. Delegates to the main constructor with both defaulted. Not declared on a `QT_NO_SSL` build (no `tls` parameter to distinguish it from the `(serverUrl, cfg)` overload below). | +| `QtWebSocketBackend(serverUrl, tls, cfg = Config{})` | Overload that skips the unused `dispatcher`/`registry` pair (issue #55): a caller who only needs `tls`/`cfg` reaches them directly, without naming `morph::model::detail::defaultDispatcher()`/`defaultRegistry()` explicitly. Delegates to the main constructor with both defaulted. Not declared on a `QT_NO_SSL` build (no `tls` parameter to distinguish it from the `(serverUrl, cfg)` overload below). | | `QtWebSocketBackend(serverUrl, cfg)` | Overload that skips `dispatcher`/`registry` and `tls` together — the common case for a caller that only wants to set a `Config` field (e.g. `asyncRegistrationEnabled`) over a plaintext `ws://` connection. Delegates to the main constructor with `dispatcher`/`registry` defaulted and (on an SSL-enabled build) `tls = std::nullopt`. | | `registerModelAsync(typeId, factory, contextKey, onRegistered, onError)` | Returns `false` immediately unless `cfg.asyncRegistrationEnabled` is `true`. Otherwise: assigns a fresh `callId` (the same counter `execute` uses), records the callbacks in `_pendingRegistrations[callId]`, sends `register` with that `callId`, and returns `true`. `onRegistered`/`onError` fire later from `onTextMessage` (or from `cancelPending` on a disconnect) — never synchronously from this call. | | `waitForConnected(timeoutMs = 5000)` | Pumps the Qt loop until connected or timeout; returns `_connected`. | From 2e1b142ff669332fc00bdb7cb7e1b33fd116b7bd Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Thu, 13 Aug 2026 10:25:11 +0300 Subject: [PATCH 4/7] testkit: bound StepExecutor::runAll() so a re-posting task throws instead of hanging An unbounded 'while (runOne())' loop has no way to distinguish a legitimately-draining queue from a task that keeps re-posting more work to itself -- a strand bug, or a harness misuse, would spin runAll() forever with no assertion failure and no compile-time signal, only a hung test process indistinguishable from a CI timeout. runAll() now takes a maxSteps bound (generous default: 10,000) and throws std::runtime_error if it's reached, naming runOne() as the way to step through and find the runaway task. Adds a regression test with a small bound proving a self-re-posting task throws immediately rather than hanging. Co-Authored-By: Claude Sonnet 5 --- tests/test_remote_step_interleaving.cpp | 15 +++++++++++++++ tests/test_support.hpp | 19 +++++++++++++++++-- 2 files changed, 32 insertions(+), 2 deletions(-) diff --git a/tests/test_remote_step_interleaving.cpp b/tests/test_remote_step_interleaving.cpp index d0632556..a3e9d884 100644 --- a/tests/test_remote_step_interleaving.cpp +++ b/tests/test_remote_step_interleaving.cpp @@ -10,6 +10,7 @@ #include #include #include +#include #include #include @@ -112,3 +113,17 @@ TEST_CASE("morph::testing::StepExecutor: two different models' work can be inter REQUIRE(replyA.env.callId == 31U); REQUIRE(replyB.env.callId == 21U); } + +TEST_CASE("morph::testing::StepExecutor: runAll() throws instead of hanging on a task that reposts indefinitely", + "[step-executor]") { + // A task that always re-posts itself never leaves the queue empty, so an + // unbounded `while (runOne())` would spin forever with no assertion + // failure and no compile-time signal -- exactly the failure mode a + // hand-stepping harness must not have. runAll()'s maxSteps bound turns + // that into a loud, immediate exception instead. + morph::testing::StepExecutor exec; + std::function reArm = [&exec, &reArm] { exec.post(reArm); }; + exec.post(reArm); + + REQUIRE_THROWS_AS(exec.runAll(/*maxSteps=*/50), std::runtime_error); +} diff --git a/tests/test_support.hpp b/tests/test_support.hpp index 69f7793d..d0851294 100644 --- a/tests/test_support.hpp +++ b/tests/test_support.hpp @@ -16,6 +16,7 @@ #include #include #include +#include #include #include #include @@ -80,12 +81,26 @@ class StepExecutor : public ::morph::exec::IExecutor { /// @brief Runs every task currently queued, including ones a running task /// itself posts (e.g. a strand re-arming for its next queued item). + /// + /// Bounded at @p maxSteps rather than looping until the queue is empty: a + /// task that keeps re-posting more work to this executor (a bug in the + /// code under test, or a harness misuse) would otherwise turn this into an + /// undetectable infinite loop, hanging the test process with no assertion + /// failure and no compile-time signal. Real drains in this suite finish + /// within a handful of steps, so the default is generous headroom, not a + /// tight bound callers need to reason about. + /// @param maxSteps Upper bound on tasks run before giving up. /// @return Number of tasks run. - std::size_t runAll() { + std::size_t runAll(std::size_t maxSteps = 10'000) { std::size_t ran = 0; - while (runOne()) { + while (ran < maxSteps && runOne()) { ++ran; } + if (ran == maxSteps) { + throw std::runtime_error( + "StepExecutor::runAll: exceeded maxSteps -- a task is likely re-posting " + "indefinitely; use runOne() to step through and find it"); + } return ran; } From f70f0e5c2d77fb811879ea79e955f343728903f8 Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Thu, 13 Aug 2026 11:01:29 +0300 Subject: [PATCH 5/7] tests: cover QtWebSocketBackend's (serverUrl, tls, cfg) constructor overload The other new constructor overload, (serverUrl, cfg), already had a dedicated test; this three-argument one -- letting a caller pass a tls configuration without naming the dispatcher/registry pair -- had none. Every existing call site in this file uses either the full four-argument constructor with explicit defaultDispatcher()/defaultRegistry(), or the bare (url) shorthand, or the (url, cfg) overload -- never (url, tls, cfg) directly. Mirrors the existing Config-only overload test: connects, registers a handler asynchronously, executes an action, confirms the round trip works -- proving the overload resolves and delegates correctly. Co-Authored-By: Claude Sonnet 5 --- tests/qt/test_qt_websocket.cpp | 44 ++++++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/tests/qt/test_qt_websocket.cpp b/tests/qt/test_qt_websocket.cpp index 609a7938..c6ecd32b 100644 --- a/tests/qt/test_qt_websocket.cpp +++ b/tests/qt/test_qt_websocket.cpp @@ -292,6 +292,50 @@ TEST_CASE( REQUIRE(result.load() == 7); } +#ifndef QT_NO_SSL +TEST_CASE( + "morph::qt::QtWebSocketBackend: (serverUrl, tls, cfg) constructor overload omits the dispatcher/registry pair " + "(issue #55)", + "[qt][ws][issue55]") { + // The middle of the three constructor overloads: unlike the Config-only + // one above, this one also lets a caller pass a `tls` configuration + // without naming the dispatcher/registry pair. `tls = std::nullopt` here + // (a plain ws:// URL, no TLS) is enough to prove the overload itself + // resolves and delegates correctly -- TLS handshake behavior itself is + // exercised elsewhere in this file via the main constructor. + 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())}; + auto backendPtr = std::make_unique( + url, std::optional{std::nullopt}, + morph::qt::QtWebSocketBackend::Config{.asyncRegistrationEnabled = true}); + REQUIRE(backendPtr->waitForConnected()); + + morph::qt::QtExecutor qtExec; + morph::bridge::Bridge bridge{std::move(backendPtr)}; + + auto binding = std::make_shared(); + binding->typeId = "WsEchoModel"; + binding->modelFactory = [] { return morph::model::detail::ModelFactory::create(); }; + bridge.registerHandler(binding); + CHECK(binding->currentId.load() == 0U); // async: still unbound immediately after registerHandler() + + pumpUntil([&] { return binding->currentId.load() != 0U; }); + REQUIRE(binding->currentId.load() != 0U); + + morph::bridge::BridgeHandler handler{bridge, &qtExec, binding}; + std::atomic result{-1}; + handler.execute(WsEchoAction{11}).then([&](int val) { result.store(val); }).onError([](const std::exception_ptr&) { + }); + pumpUntil([&] { return result.load() != -1; }); + REQUIRE(result.load() == 11); +} +#endif + TEST_CASE( "morph::qt::QtWebSocketBackend: registerModelAsync's pending registration is cancelled when the connection " "drops before a reply arrives", From d2293f31e9a522f8943dc4c9f1bfbac7ad0c6d3f Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Thu, 13 Aug 2026 11:18:08 +0300 Subject: [PATCH 6/7] tests: fix test_conflict_resolution.cpp to poll queue.drain() directly, not notifyCount notifyCount is the wrong signal: OrderModel::onBackendChanged() increments it BEFORE draining the queue, so polling on notifyCount reaching 1 only proves the call started, not that the drain finished. This raced the drain itself and reproduced on CI as both a plain assertion failure (queue.drain().empty() intermittently still false) and, once, a genuine UBSan misaligned-member-call report -- calling execute() concurrently with a model instance mid-teardown during the same race window. InMemoryOfflineQueue::drain() is a thread-safe, non-destructive snapshot read (per its own doc comment), so polling it directly is safe and becomes empty at the exact moment every item is drained and marked done -- the actual signal these tests need, with no race window. Verified 20x back-to-back locally with no failures. Co-Authored-By: Claude Sonnet 5 --- tests/test_conflict_resolution.cpp | 32 ++++++++++++++++++------------ 1 file changed, 19 insertions(+), 13 deletions(-) diff --git a/tests/test_conflict_resolution.cpp b/tests/test_conflict_resolution.cpp index bfd3027e..3e80b352 100644 --- a/tests/test_conflict_resolution.cpp +++ b/tests/test_conflict_resolution.cpp @@ -131,20 +131,26 @@ static int waitInt(auto completion) { return result.load(); } -// ── Helper: poll until onBackendChanged has actually run ───────────────────── +// ── Helper: poll until the queue is actually drained ────────────────────────── // // switchBackend() dispatches the new model's construction and its // onBackendChanged() call (which drains the offline queue) onto the new // backend's own thread pool, asynchronously -- there is no signal the caller -// can block on directly. A fixed sleep_for() guessing "surely long enough" -// is exactly the failure mode this helper replaces: it polls the model's own -// notifyCount via OrderQueryAction (read-only, safe to call repeatedly) -// until it reaches 1, the real signal that onBackendChanged has completed -// -- and therefore that the queue drain it performs has too. Bounded, not -// unbounded: returns false (rather than hanging) if the count never reaches 1. -static bool waitForBackendChanged(morph::bridge::BridgeHandler& handler) { +// can block on directly. A fixed sleep_for() guessing "surely long enough" is +// exactly the failure mode this helper replaces. +// +// notifyCount is NOT the right signal to poll here: onBackendChanged() +// increments it *before* draining the queue (see OrderModel::onBackendChanged), +// so notifyCount reaching 1 only proves the call started, not that the drain +// finished -- polling on it raced the drain itself and intermittently observed +// a non-empty queue right after. drain() itself is the correct signal: it is +// a thread-safe, non-destructive snapshot read on InMemoryOfflineQueue (see its +// own doc comment), so polling it repeatedly is safe and it becomes empty at +// the exact moment every item has been handled and markDone'd. Bounded, not +// unbounded: returns false (rather than hanging) if it never empties. +static bool waitForQueueDrained(morph::offline::InMemoryOfflineQueue& queue) { for (int i = 0; i < 200; ++i) { - if (waitInt(handler.execute(OrderQueryAction{})) >= 1) { + if (queue.drain().empty()) { return true; } std::this_thread::sleep_for(10ms); @@ -172,7 +178,7 @@ TEST_CASE("ConflictResolution: no conflicts - all items markDone on switchBack morph::bridge::BridgeHandler handler{bridge, &cbExec, binding}; bridge.switchBackend(std::make_unique(pool2)); - REQUIRE(waitForBackendChanged(handler)); + REQUIRE(waitForQueueDrained(queue)); // All items removed from queue after clean replay. REQUIRE(queue.drain().empty()); @@ -199,7 +205,7 @@ TEST_CASE("ConflictResolution: conflicting items discarded - resolver returns morph::bridge::BridgeHandler handler{bridge, &cbExec, binding}; bridge.switchBackend(std::make_unique(pool2)); - REQUIRE(waitForBackendChanged(handler)); + REQUIRE(waitForQueueDrained(queue)); // All three items removed regardless of outcome (discard also calls markDone). REQUIRE(queue.drain().empty()); @@ -223,7 +229,7 @@ TEST_CASE("ConflictResolution: conflicting items merged - resolver returns non morph::bridge::BridgeHandler handler{bridge, &cbExec, binding}; bridge.switchBackend(std::make_unique(pool2)); - REQUIRE(waitForBackendChanged(handler)); + REQUIRE(waitForQueueDrained(queue)); // All three items processed and removed. REQUIRE(queue.drain().empty()); @@ -296,7 +302,7 @@ TEST_CASE("ConflictResolution: full offline scenario - accumulate offline, syn // Simulate reconnection - switch to remote backend. bridge.switchBackend(std::make_unique(remotePool)); - REQUIRE(waitForBackendChanged(handler)); + REQUIRE(waitForQueueDrained(queue)); // Queue fully drained: 2 clean replays + 1 merge = 3 markDone calls. REQUIRE(queue.drain().empty()); From f749ff8295030f06676b4cdb519be27562e991ea Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Thu, 13 Aug 2026 11:38:24 +0300 Subject: [PATCH 7/7] tests: rename test_remote_step_interleaving.cpp's OrderModel/OrderAction to avoid an ODR collision Root cause of the real CI failure: this file (added by this PR) declared its own file-scope OrderModel/OrderAction with external linkage -- identical simple names to test_conflict_resolution.cpp's own, entirely unrelated, pre-existing OrderModel (which has onBackendChanged(), notifyCount, offline-queue draining, none of which this file's stub type has). Two external-linkage types with the same name and different definitions is a One-Definition-Rule violation the linker does not diagnose; which definition ends up linked into which translation unit is compiler/link-order dependent. Confirmed via targeted tracing: in the affected builds, LocalBackend's _changeAware set was empty after registering a model built from test_conflict_resolution.cpp's own modelFactory -- i.e. the linker had resolved BackendChangedNotifiable using this file's bare stub definition instead of the real one, so notifyBackendChanged() never posted to onBackendChanged() at all and the offline queue never drained. Reproduced deterministically (100% of runs, not flaky) on Linux/clang-ubsan and on real CI across nearly every Linux job; verified absent on 10/10 runs after this rename, both under WSL/clang-ubsan and matching CI's own per-test-case invocation pattern. Renamed to StepILOrderModel/StepILOrderAction (matching this file's own StepIL_* wire-typeId convention already in use), eliminating the collision without touching test_conflict_resolution.cpp at all. Co-Authored-By: Claude Sonnet 5 --- tests/test_remote_step_interleaving.cpp | 22 ++++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-) diff --git a/tests/test_remote_step_interleaving.cpp b/tests/test_remote_step_interleaving.cpp index a3e9d884..112593d3 100644 --- a/tests/test_remote_step_interleaving.cpp +++ b/tests/test_remote_step_interleaving.cpp @@ -16,19 +16,29 @@ #include "test_support.hpp" -struct OrderAction { +// Named StepILOrder* (not the more obvious OrderAction/OrderModel) because +// test_conflict_resolution.cpp already declares its own, unrelated OrderModel +// at external linkage -- two same-named external-linkage types with different +// definitions is an ODR violation the linker does not diagnose, and which +// definition the linker keeps is compiler/link-order dependent (confirmed: +// this silently made OrderModel not backend-change-aware in some builds, +// since the type actually linked into some translation units was this file's +// bare struct, not test_conflict_resolution.cpp's onBackendChanged()-bearing +// one -- see that file's own history for the resulting flaky-then-hard +// failure this caused before the collision was found and fixed). +struct StepILOrderAction { int tag = 0; }; -struct OrderModel { - int execute(const OrderAction& action) { return action.tag; } +struct StepILOrderModel { + int execute(const StepILOrderAction& action) { return action.tag; } }; -BRIDGE_REGISTER_MODEL(OrderModel, "StepIL_OrderModel") -BRIDGE_REGISTER_ACTION(OrderModel, OrderAction, "StepIL_OrderAction") +BRIDGE_REGISTER_MODEL(StepILOrderModel, "StepIL_OrderModel") +BRIDGE_REGISTER_ACTION(StepILOrderModel, StepILOrderAction, "StepIL_OrderAction") namespace { -/// @brief Registers a fresh `OrderModel` instance and returns the +/// @brief Registers a fresh `StepILOrderModel` instance and returns the /// server-assigned model id as a plain `uint64_t` -- the wire's own /// vocabulary, never `morph::exec::detail::ModelId`. uint64_t registerModel(const std::shared_ptr& server, morph::testing::StepExecutor& exec) {