From 54f5428678554efdcea7f67f39ab695ff373e07c Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Mon, 25 May 2026 21:48:23 +0200 Subject: [PATCH] [morph] Fix documentation and CI --- .github/workflows/ci.yml | 5 +- CMakeLists.txt | 7 --- CMakePresets.json | 17 ------- cmake/compiler_options.cmake | 12 ----- cmake/msan.supp | 23 --------- include/morph/backend.hpp | 4 ++ include/morph/bridge.hpp | 59 ++++++++++++++--------- include/morph/qt/qt_websocket_backend.hpp | 4 ++ include/morph/registry.hpp | 3 ++ include/morph/remote.hpp | 5 ++ include/morph/session.hpp | 18 ++++++- tests/CMakeLists.txt | 11 +---- tests/test_support.hpp | 2 +- 13 files changed, 74 insertions(+), 96 deletions(-) delete mode 100644 cmake/msan.supp diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 0a82a1e..f73cd5b 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -122,7 +122,7 @@ jobs: strategy: fail-fast: false matrix: - preset: [clang-asan, clang-tsan, clang-ubsan, clang-msan, clang-coverage] + preset: [clang-asan, clang-tsan, clang-ubsan, clang-coverage] steps: - uses: actions/checkout@v4 @@ -133,12 +133,11 @@ jobs: key: apt-sanitizers-${{ hashFiles('.github/workflows/ci.yml') }} restore-keys: apt-sanitizers- - - name: Install Clang ${{ env.CLANG_VERSION }} and libc++ from apt.llvm.org + - name: Install Clang ${{ env.CLANG_VERSION }} from apt.llvm.org run: | sudo apt-get update -q sudo apt-get install -y ninja-build catch2 wget -qO- https://apt.llvm.org/llvm.sh | sudo bash -s -- ${{ env.CLANG_VERSION }} - sudo apt-get install -y libc++-${{ env.CLANG_VERSION }}-dev libc++abi-${{ env.CLANG_VERSION }}-dev - name: Cache sccache uses: actions/cache@v4 diff --git a/CMakeLists.txt b/CMakeLists.txt index f5e9e89..86329ef 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -91,15 +91,8 @@ endif() # ── Tests ──────────────────────────────────────────────────────────────────── if(MORPH_BUILD_TESTS) - # Under MSan every library Catch2 touches must also be MSan-instrumented. - # The system apt package is not, so we always build from source in that mode. - set(_catch2_use_fetch OFF) find_package(Catch2 CONFIG QUIET) if(NOT Catch2_FOUND) - set(_catch2_use_fetch ON) - endif() - - if(_catch2_use_fetch) include(FetchContent) FetchContent_Declare( Catch2 diff --git a/CMakePresets.json b/CMakePresets.json index 70fb3a4..09e4867 100644 --- a/CMakePresets.json +++ b/CMakePresets.json @@ -153,16 +153,6 @@ "AF_SANITIZER": "ubsan" } }, - { - "name": "clang-msan", - "displayName": "MSan (Clang, Linux — requires instrumented libc++)", - "inherits": "base-clang-linux", - "binaryDir": "${sourceDir}/build/clang-msan", - "cacheVariables": { - "CMAKE_BUILD_TYPE": "Debug", - "AF_SANITIZER": "msan" - } - }, { "name": "clang-coverage", "displayName": "Coverage (Clang source-based, Linux)", @@ -188,7 +178,6 @@ { "name": "clang-asan", "configurePreset": "clang-asan", "jobs": 0 }, { "name": "clang-tsan", "configurePreset": "clang-tsan", "jobs": 0 }, { "name": "clang-ubsan", "configurePreset": "clang-ubsan", "jobs": 0 }, - { "name": "clang-msan", "configurePreset": "clang-msan", "jobs": 0 }, { "name": "clang-coverage", "configurePreset": "clang-coverage", "jobs": 0 } ], "testPresets": [ @@ -246,12 +235,6 @@ "output": { "outputOnFailure": true }, "execution": { "noTestsAction": "error", "stopOnFailure": true } }, - { - "name": "clang-msan", - "configurePreset": "clang-msan", - "output": { "outputOnFailure": true }, - "execution": { "noTestsAction": "error", "stopOnFailure": true } - }, { "name": "clang-coverage", "configurePreset": "clang-coverage", diff --git a/cmake/compiler_options.cmake b/cmake/compiler_options.cmake index 0d788b5..2a74266 100644 --- a/cmake/compiler_options.cmake +++ b/cmake/compiler_options.cmake @@ -73,18 +73,6 @@ function(apply_sanitizers target mode) -fsanitize=undefined -fno-omit-frame-pointer -g) target_link_options(${target} PRIVATE -fsanitize=undefined) - elseif(mode STREQUAL "msan") - # Instrument only our own targets. Catch2 is built with -stdlib=libc++ (set - # globally for ABI compatibility) but without -fsanitize=memory to avoid its - # known false positives. The ignorelist covers morph:: SSO hash false positives. - target_compile_options(${target} PRIVATE - -fsanitize=memory -fsanitize-memory-track-origins - -fno-omit-frame-pointer -g - -stdlib=libc++ - "-fsanitize-ignorelist=${CMAKE_SOURCE_DIR}/cmake/msan.supp") - target_link_options(${target} PRIVATE - -fsanitize=memory - -stdlib=libc++) endif() endfunction() diff --git a/cmake/msan.supp b/cmake/msan.supp deleted file mode 100644 index dded18a..0000000 --- a/cmake/msan.supp +++ /dev/null @@ -1,23 +0,0 @@ -# LLVM sanitizer ignorelist for MSan. -# Syntax: https://clang.llvm.org/docs/SanitizerSpecialCaseList.html -# -# morph::ModelRegistryFactory::registerModel and morph::ActionDispatcher::registerAction -# trigger a libc++ SSO false positive: std::hash reads the full -# internal SSO buffer (including padding bytes) via a bulk hash. The padding is never -# explicitly initialized by the std::string constructor, which MSan flags. This is a -# known libc++ + MSan interaction, not a bug in user code. -# -# All Catch2 entries are false positives from Catch2's own internals (SSO padding -# in ReporterRegistry, Clara option parser, and test registration machinery). -# fun: patterns match against mangled symbol names using glob syntax. -[memory] -fun:*morph*ModelRegistryFactory*registerModel* -fun:*morph*ActionDispatcher*registerAction* -fun:*ReporterRegistry* -fun:*RegistryHub* -fun:*Singleton*getMutable* -fun:*AutoReg* -fun:*ParserRefImpl* -fun:*Clara*Parser* -fun:*makeCommandLineParser* -fun:*Session*Session* diff --git a/include/morph/backend.hpp b/include/morph/backend.hpp index 93bc96f..6b74e1a 100644 --- a/include/morph/backend.hpp +++ b/include/morph/backend.hpp @@ -100,11 +100,13 @@ struct IBackend { /// or surface a "backend changed" message — there is no public cancel API on /// `Completion` itself. struct BackendChangedError : std::runtime_error { + /// @brief Constructs the error with a canned diagnostic message. BackendChangedError() : std::runtime_error{"backend changed before completion resolved"} {} }; /// @brief Thrown to in-flight `Completion`s when `Bridge` is destroyed. struct BridgeDestroyedError : std::runtime_error { + /// @brief Constructs the error with a canned diagnostic message. BridgeDestroyedError() : std::runtime_error{"bridge destroyed before completion resolved"} {} }; @@ -112,6 +114,7 @@ struct BridgeDestroyedError : std::runtime_error { /// Qt WebSocket disconnect). The framework retries the call on reconnect if /// the backend supports it; otherwise the GUI's `.onError(...)` runs. struct DisconnectedError : std::runtime_error { + /// @brief Constructs the error with a canned diagnostic message. DisconnectedError() : std::runtime_error{"transport disconnected before completion resolved"} {} }; @@ -201,6 +204,7 @@ class LocalBackend : public detail::IBackend { } /// @brief Resolves every still-pending completion this backend produced with @p exc. + /// @param exc Exception delivered to every pending completion's error sink. void cancelPending(const std::exception_ptr& exc) override { std::vector>>> snapshot; { diff --git a/include/morph/bridge.hpp b/include/morph/bridge.hpp index d2c895e..04593ba 100644 --- a/include/morph/bridge.hpp +++ b/include/morph/bridge.hpp @@ -80,7 +80,7 @@ class Bridge { /// @param backend Initial backend. Ownership is transferred. explicit Bridge(std::unique_ptr<::morph::backend::detail::IBackend> backend) : _backend{std::shared_ptr<::morph::backend::detail::IBackend>(std::move(backend))} { - installReconnectHandler(_backend.load()); + installReconnectHandler(_backend); } /// @brief Cancels every still-pending completion on the active backend. @@ -89,7 +89,7 @@ class Bridge { /// server replies that arrive after destruction are no-ops because each /// `CompletionState::setValue`/`setException` is idempotent. ~Bridge() { - if (auto active = _backend.load()) { + if (auto active = loadBackend()) { active->cancelPending(std::make_exception_ptr(::morph::backend::BridgeDestroyedError{})); } } @@ -110,7 +110,7 @@ class Bridge { binding->typeId = std::string{::morph::model::ModelTraits::typeId()}; binding->modelFactory = [] { return ::morph::model::detail::ModelFactory::create(); }; std::scoped_lock lock{_mtx}; - binding->currentId.store(_backend.load()->registerModel(binding->typeId, binding->modelFactory).v); + binding->currentId.store(loadBackend()->registerModel(binding->typeId, binding->modelFactory).v); _handlers.push_back(binding); return binding; } @@ -122,23 +122,10 @@ class Bridge { /// @param binding Pre-constructed binding. Its `typeId` and `modelFactory` must be set. void registerHandler(const std::shared_ptr& binding) { std::scoped_lock lock{_mtx}; - binding->currentId.store(_backend.load()->registerModel(binding->typeId, binding->modelFactory).v); + binding->currentId.store(loadBackend()->registerModel(binding->typeId, binding->modelFactory).v); _handlers.push_back(binding); } - /// @brief Atomically replaces the active backend with @p newBackend. - /// - /// All live bindings are re-registered on the new backend and their - /// `currentId` values are updated. The old backend is released after the - /// swap. Any in-flight `Completion` objects targeting the old backend will - /// fail naturally. - /// - /// @note Lock ordering: `Bridge::_mtx` is acquired before - /// `LocalBackend::_regMtx`. `onBackendChanged()` implementations must - /// **not** call `registerHandler()` or `deregisterHandler()` — those - /// also acquire `_mtx` and would deadlock. - /// - /// @param newBackend Replacement backend. Ownership is transferred. /// @brief Installs a default session context applied to every call that does /// not provide one explicitly via `BridgeHandler::executeWith(...)`. /// @@ -153,11 +140,25 @@ class Bridge { } /// @brief Returns a copy of the currently installed default session. Thread-safe. + /// @return Snapshot of the default `Context`. [[nodiscard]] ::morph::session::Context defaultSession() const { std::scoped_lock lock{_sessionMtx}; return _defaultSession; } + /// @brief Atomically replaces the active backend with @p newBackend. + /// + /// All live bindings are re-registered on the new backend and their + /// `currentId` values are updated. The old backend is released after the + /// swap. Any in-flight `Completion` objects targeting the old backend will + /// fail naturally. + /// + /// @note Lock ordering: `Bridge::_mtx` is acquired before + /// `LocalBackend::_regMtx`. `onBackendChanged()` implementations must + /// **not** call `registerHandler()` or `deregisterHandler()` — those + /// also acquire `_mtx` and would deadlock. + /// + /// @param newBackend Replacement backend. Ownership is transferred. void switchBackend(std::unique_ptr<::morph::backend::detail::IBackend> newBackend) { auto newShared = std::shared_ptr<::morph::backend::detail::IBackend>(std::move(newBackend)); std::shared_ptr<::morph::backend::detail::IBackend> previous; @@ -176,7 +177,7 @@ class Bridge { } _handlers = std::move(live); - previous = _backend.exchange(newShared); + previous = exchangeBackend(newShared); newShared->notifyBackendChanged(); } installReconnectHandler(newShared); @@ -200,7 +201,7 @@ class Bridge { std::scoped_lock lock{_mtx}; uint64_t raw = binding->currentId.load(); if (raw != 0U) { - _backend.load()->deregisterModel(::morph::exec::detail::ModelId{raw}); + loadBackend()->deregisterModel(::morph::exec::detail::ModelId{raw}); } auto iter = std::ranges::find_if(_handlers, [&](auto& weak) { auto sptr = weak.lock(); @@ -230,7 +231,7 @@ class Bridge { ::morph::exec::IExecutor* cbExec) { using R = ::morph::model::ActionTraits::Result; - auto backend = _backend.load(); + auto backend = loadBackend(); uint64_t raw = binding->currentId.load(); auto typedState = std::make_shared<::morph::async::detail::CompletionState>(); @@ -265,6 +266,19 @@ class Bridge { } private: + std::shared_ptr<::morph::backend::detail::IBackend> loadBackend() const { + std::scoped_lock lock{_backendMtx}; + return _backend; + } + + std::shared_ptr<::morph::backend::detail::IBackend> exchangeBackend( + std::shared_ptr<::morph::backend::detail::IBackend> next) { + std::scoped_lock lock{_backendMtx}; + auto previous = std::move(_backend); + _backend = std::move(next); + return previous; + } + void installReconnectHandler(const std::shared_ptr<::morph::backend::detail::IBackend>& backend) { if (!backend) { return; @@ -276,7 +290,7 @@ class Bridge { backend->setReconnectHandler([this, weakBackend] { auto pinned = weakBackend.lock(); std::scoped_lock lock{_mtx}; - if (!pinned || pinned != _backend.load()) { + if (!pinned || pinned != loadBackend()) { return; // We've moved on to a different backend; ignore. } for (auto& weak : _handlers) { @@ -290,7 +304,8 @@ class Bridge { }); } - std::atomic> _backend; + mutable std::mutex _backendMtx; + std::shared_ptr<::morph::backend::detail::IBackend> _backend; std::mutex _mtx; std::vector> _handlers; mutable std::mutex _sessionMtx; diff --git a/include/morph/qt/qt_websocket_backend.hpp b/include/morph/qt/qt_websocket_backend.hpp index 68331d1..3cf7a99 100644 --- a/include/morph/qt/qt_websocket_backend.hpp +++ b/include/morph/qt/qt_websocket_backend.hpp @@ -116,9 +116,13 @@ class QtWebSocketBackend : public ::morph::backend::detail::IBackend { /// Called by `Bridge::switchBackend()` on the outgoing backend, by `~Bridge`, /// and internally when the socket disconnects. Late replies arriving for /// already-cancelled call ids are dropped silently. + /// + /// @param exc Exception delivered to every pending completion's error sink. void cancelPending(const std::exception_ptr& exc) override; /// @brief Installs the handler `Bridge` uses to re-register handlers after a reconnect. + /// @param handler Callable invoked on the Qt thread after every successful reconnect. + /// Pass `nullptr` to clear. void setReconnectHandler(std::function handler) override; private: diff --git a/include/morph/registry.hpp b/include/morph/registry.hpp index 8e8c725..4941c93 100644 --- a/include/morph/registry.hpp +++ b/include/morph/registry.hpp @@ -80,6 +80,9 @@ struct ActionValidator { /// /// Auto-detects a `bool validate() const` member on @p action via the /// `morph::model::detail::HasValidate` concept. Falls back to `true`. + /// + /// @param action Draft action whose readiness is being checked. + /// @return `true` if the action should fire, `false` to keep collecting fields. static constexpr bool ready(const Action& action) { if constexpr (detail::HasValidate) { return action.validate(); diff --git a/include/morph/remote.hpp b/include/morph/remote.hpp index 7acafe8..01043cd 100644 --- a/include/morph/remote.hpp +++ b/include/morph/remote.hpp @@ -35,6 +35,10 @@ namespace morph::backend { class RemoteServer : public std::enable_shared_from_this { public: /// @brief Constructs a server backed by @p workerPool with allow-all authorization. + /// + /// @param workerPool Pool used to process messages asynchronously. + /// @param dispatcher Action dispatcher; defaults to the process-level singleton. + /// @param registry Model factory registry; defaults to the process-level singleton. explicit RemoteServer( ::morph::exec::IExecutor& workerPool, ::morph::model::detail::ActionDispatcher& dispatcher = ::morph::model::detail::defaultDispatcher(), @@ -267,6 +271,7 @@ class SimulatedRemoteBackend : public detail::IBackend { void notifyBackendChanged() override {} /// @brief Resolves every still-pending completion this backend produced with @p exc. + /// @param exc Exception delivered to every pending completion's error sink. void cancelPending(const std::exception_ptr& exc) override { std::vector>>> snapshot; { diff --git a/include/morph/session.hpp b/include/morph/session.hpp index da3ba57..f7b4fa3 100644 --- a/include/morph/session.hpp +++ b/include/morph/session.hpp @@ -46,6 +46,11 @@ struct IAuthorizer { virtual ~IAuthorizer() = default; /// @brief Returns `true` if @p ctx is allowed to invoke @p actionType on @p modelType. + /// + /// @param ctx Per-call session attached by the client. + /// @param modelType String id of the target model type. + /// @param actionType String id of the action being invoked. + /// @return `true` to allow dispatch, `false` to reject with `err|unauthorized`. [[nodiscard]] virtual bool authorize(const Context& ctx, std::string_view modelType, std::string_view actionType) const = 0; }; @@ -55,8 +60,14 @@ struct IAuthorizer { /// Wire it explicitly via `RemoteServer(pool, dispatcher, registry, allowAll)` /// for documentation, or rely on the server's default (which uses this type). struct AllowAllAuthorizer : IAuthorizer { - [[nodiscard]] bool authorize(const Context& /*ctx*/, std::string_view /*modelType*/, - std::string_view /*actionType*/) const override { + /// @brief Permits every call. + /// @param ctx Ignored. + /// @param modelType Ignored. + /// @param actionType Ignored. + /// @return Always `true`. + [[nodiscard]] bool authorize([[maybe_unused]] const Context& ctx, + [[maybe_unused]] std::string_view modelType, + [[maybe_unused]] std::string_view actionType) const override { return true; } }; @@ -81,7 +92,10 @@ inline const Context*& tlsCurrent() { /// @brief RAII helper that sets the thread-local `Context` for its scope. class ScopedContext { public: + /// @brief Installs @p ctx as the thread-local context until the scope exits. + /// @param ctx Context whose address is stored; must outlive this object. explicit ScopedContext(const Context& ctx) : _prev{tlsCurrent()} { tlsCurrent() = &ctx; } + /// @brief Restores the previously active thread-local context. ~ScopedContext() { tlsCurrent() = _prev; } ScopedContext(const ScopedContext&) = delete; ScopedContext& operator=(const ScopedContext&) = delete; diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 7be072f..04d7e5e 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -45,12 +45,5 @@ if(AF_COVERAGE) apply_coverage(morph_tests) endif() -if(AF_SANITIZER STREQUAL "msan") - # catch_discover_tests runs the binary for enumeration which triggers MSan - # before any suppressions can be applied. Use a single all-up test instead, - # with the suppression file to silence Catch2's known false positive. - add_test(NAME morph_tests COMMAND morph_tests) -else() - include(Catch) - catch_discover_tests(morph_tests DISCOVERY_MODE PRE_TEST) -endif() +include(Catch) +catch_discover_tests(morph_tests DISCOVERY_MODE PRE_TEST) diff --git a/tests/test_support.hpp b/tests/test_support.hpp index 232b71b..f27c538 100644 --- a/tests/test_support.hpp +++ b/tests/test_support.hpp @@ -31,7 +31,7 @@ struct InlineExecutor : ::morph::exec::IExecutor { }; /// @brief Default polling budget for `waitUntil`. Picked to cover the slowest -/// TSan/MSan runs without making green tests visibly slow. +/// TSan/Valgrind runs without making green tests visibly slow. inline constexpr std::chrono::milliseconds kDefaultWaitBudget{2000}; /// @brief Default polling step for `waitUntil`.