diff --git a/docs/spec/core/backend.md b/docs/spec/core/backend.md index 9908323b..13f83a13 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` 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`. | | `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..fb1dd36b 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 @@ -26,8 +27,10 @@ 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 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..c6ecd32b 100644 --- a/tests/qt/test_qt_websocket.cpp +++ b/tests/qt/test_qt_websocket.cpp @@ -253,6 +253,89 @@ 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); +} + +#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", 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_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()); diff --git a/tests/test_remote_step_interleaving.cpp b/tests/test_remote_step_interleaving.cpp new file mode 100644 index 00000000..112593d3 --- /dev/null +++ b/tests/test_remote_step_interleaving.cpp @@ -0,0 +1,139 @@ +// 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 + +#include "test_support.hpp" + +// 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 StepILOrderModel { + int execute(const StepILOrderAction& action) { return action.tag; } +}; + +BRIDGE_REGISTER_MODEL(StepILOrderModel, "StepIL_OrderModel") +BRIDGE_REGISTER_ACTION(StepILOrderModel, StepILOrderAction, "StepIL_OrderAction") + +namespace { + +/// @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) { + 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); +} + +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 425c55f5..d0851294 100644 --- a/tests/test_support.hpp +++ b/tests/test_support.hpp @@ -13,7 +13,10 @@ #include #include +#include #include +#include +#include #include #include #include @@ -30,6 +33,89 @@ 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). + /// + /// 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 maxSteps = 10'000) { + std::size_t ran = 0; + 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; + } + + /// @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};