From c21c49c40f25c764a0bc63710d401ed4ba753ebb Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Sun, 16 Aug 2026 12:17:09 +0300 Subject: [PATCH 1/4] tests: add morph::testkit::OomInjector, close bridge.hpp's 2 OOM-only catch(...) blocks (morph#108) Adds a test-only allocator seam (tests/oom_injector.hpp + .cpp) that overrides the process-wide operator new/delete for whichever test binary links it in, making the next operator new call whose size matches a threshold (optionally the Kth such match) throw std::bad_alloc on demand. Lives under tests/, not include/morph/core/: this replaces the allocator for the whole binary, so it must never link into the shipped morph library or any example app. A size (+ occurrence-count) predicate, not a raw allocation count: an earlier raw-counter design was rejected as fragile -- the exact allocation count before a target line varies by STL/compiler and would silently start missing the intended allocation on any upstream change. The predicate instead targets the allocation's shape (a long string's heap buffer, picked to defeat SSO on every supported STL), so unrelated smaller allocations elsewhere in a call chain never shift which occurrence is being counted. Self-tested first (test_oom_injector.cpp), matching this repo's established pattern for a checker nobody would otherwise trust (scripts/test_check_deprecated_markers.sh, test_check_test_type_names.sh). Uses the seam to add real tests for both catch (...) blocks attachHandlerAsync has guarded since #108 was filed -- the out-of-frame success callback and the in-frame claimHandoff success path, both of which only fail on a genuine std::bad_alloc from copying a long primary key into contextKey/primary. Both now throw for real and are asserted to surface through onDone/onError, leave the binding unattached (not half-published), and leave the handler reusable afterward. Full suite: 1053 test cases, 10062 assertions, all passing. Co-Authored-By: Claude Sonnet 5 --- tests/CMakeLists.txt | 2 + tests/oom_injector.cpp | 128 ++++++++++++++++++++++++++ tests/oom_injector.hpp | 127 ++++++++++++++++++++++++++ tests/test_async_registration.cpp | 145 ++++++++++++++++++++++++++++++ tests/test_oom_injector.cpp | 63 +++++++++++++ 5 files changed, 465 insertions(+) create mode 100644 tests/oom_injector.cpp create mode 100644 tests/oom_injector.hpp create mode 100644 tests/test_oom_injector.cpp diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 4897343f..94e5e8b7 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -5,6 +5,8 @@ morph_generate_pinned_facts_header( ) add_executable(morph_tests + oom_injector.cpp + test_oom_injector.cpp test_executor.cpp test_example.cpp test_executor_extra.cpp diff --git a/tests/oom_injector.cpp b/tests/oom_injector.cpp new file mode 100644 index 00000000..991eb854 --- /dev/null +++ b/tests/oom_injector.cpp @@ -0,0 +1,128 @@ +// SPDX-License-Identifier: Apache-2.0 + +#include "oom_injector.hpp" + +#include +#include +#include + +namespace { + +// injectorArmed off means no injector active on this thread. While armed, +// every operator new call with size >= minSizeToFail counts down +// remainingMatchesUntilFailure; it throws when a matching call brings that +// count to 0, then disarms (one-shot). Plain built-in types only -- these +// variables' own reads/writes must never themselves allocate, or arming the +// injector would recurse into itself the moment operator new next runs. +// thread_local, not a class member: operator new below is a free function +// with no `this` to hang state off, and every thread needs its own +// independent state (see the header's own @par Thread safety). +// NOLINTBEGIN(cppcoreguidelines-avoid-non-const-global-variables) -- this +// state *is* the seam; there is no non-global way to reach into a bare +// operator new call from outside. +thread_local std::size_t minSizeToFail = 0; +thread_local std::size_t remainingMatchesUntilFailure = 0; +thread_local bool injectorArmed = false; +// NOLINTEND(cppcoreguidelines-avoid-non-const-global-variables) + +} // namespace + +namespace morph::testkit { + +OomInjector::OomInjector(std::size_t minSize, std::size_t matchToFail) { + if (injectorArmed) { + throw std::logic_error("OomInjector: another instance is already active on this thread"); + } + injectorArmed = true; + minSizeToFail = minSize; + remainingMatchesUntilFailure = matchToFail == 0 ? 1 : matchToFail; +} + +OomInjector::~OomInjector() { + injectorArmed = false; + minSizeToFail = 0; + remainingMatchesUntilFailure = 0; +} + +} // namespace morph::testkit + +namespace { + +// Every operator new overload below funnels through this so the trigger +// logic lives in one place. Recursion guard: reading/writing the +// thread_local state above touches only built-ins, never the heap, so this +// cannot re-enter itself. +void* allocateOrInject(std::size_t size) { + if (injectorArmed && size >= minSizeToFail) { + if (remainingMatchesUntilFailure <= 1) { + injectorArmed = false; // one-shot: disarm before throwing, so + // the catch block itself (and anything + // else on this thread afterward) + // allocates normally. + remainingMatchesUntilFailure = 0; + throw std::bad_alloc{}; + } + --remainingMatchesUntilFailure; + } + // NOLINTNEXTLINE(cppcoreguidelines-no-malloc, cppcoreguidelines-owning-memory) -- + // this *is* the process-wide operator new/delete pair; std::malloc/free + // is what it has to be built from. + if (void* ptr = std::malloc(size == 0 ? 1 : size)) { + return ptr; + } + throw std::bad_alloc{}; +} + +} // namespace + +void* operator new(std::size_t size) { + return allocateOrInject(size); +} + +void* operator new[](std::size_t size) { + return allocateOrInject(size); +} + +void* operator new(std::size_t size, const std::nothrow_t& tag) noexcept { + (void)tag; + try { + return allocateOrInject(size); + } catch (...) { + return nullptr; + } +} + +void* operator new[](std::size_t size, const std::nothrow_t& tag) noexcept { + (void)tag; + try { + return allocateOrInject(size); + } catch (...) { + return nullptr; + } +} + +// NOLINTBEGIN(readability-inconsistent-declaration-parameter-name, +// cppcoreguidelines-no-malloc, cppcoreguidelines-owning-memory) -- these +// replace the library's own global operator delete overloads; the "sized" +// forms' declarations in don't name their second parameter, and +// std::free is what a hand-written operator delete has to call to release +// what allocateOrInject's std::malloc above returned. +void operator delete(void* ptr) noexcept { + std::free(ptr); +} + +void operator delete[](void* ptr) noexcept { + std::free(ptr); +} + +void operator delete(void* ptr, std::size_t size) noexcept { + (void)size; + std::free(ptr); +} + +void operator delete[](void* ptr, std::size_t size) noexcept { + (void)size; + std::free(ptr); +} +// NOLINTEND(readability-inconsistent-declaration-parameter-name, +// cppcoreguidelines-no-malloc, cppcoreguidelines-owning-memory) diff --git a/tests/oom_injector.hpp b/tests/oom_injector.hpp new file mode 100644 index 00000000..a3c172ce --- /dev/null +++ b/tests/oom_injector.hpp @@ -0,0 +1,127 @@ +// SPDX-License-Identifier: Apache-2.0 + +#pragma once +#include + +namespace morph::testkit { + +/// @brief RAII scope that makes the @c matchToFail -th `operator new` call +/// whose requested size is at least @c minSize throw `std::bad_alloc` +/// on the current thread, so a test can drive a real +/// allocation-failure path (a `catch (...)` around a +/// `std::string`/container assignment, for example) without +/// exhausting process memory or forking/mocking the standard +/// allocator. +/// +/// Test-only tooling, not part of the shipped library: `oom_injector.cpp` +/// defines `operator new`/`operator delete` for the whole binary it links +/// into, so this lives under `tests/` and is only ever linked into test +/// executables, never into `morph`'s own interface library or any example +/// app -- a real consumer's process must never get its allocator replaced +/// by test-only code. +/// +/// @par Why this exists +/// Several `catch (...)` blocks across the codebase (see +/// `LASTRADA-Software/morph#108`) only ever fire on `std::bad_alloc` from a +/// real allocation failure -- there is no other way into them. That is not +/// portably reachable from a unit test without a seam: this class overrides +/// the process-wide `operator new`/`operator new[]` (defined once in +/// `oom_injector.cpp`) to consult thread-local state before delegating to +/// the real allocator. +/// +/// @par Why a size predicate, not a raw allocation count +/// An earlier design counted "the Nth `operator new` call after arming" — +/// rejected before this shipped, because that count depends on exactly how +/// many heap allocations the standard library, the test's own setup code, +/// and even unrelated `std::string`/container growth perform first, which +/// varies across STL implementations and can shift with an unrelated +/// compiler/library upgrade. A test would silently start failing the wrong +/// allocation (or none at all) the moment that count changed, without +/// necessarily failing loudly. A size predicate instead targets what the +/// allocation actually *is*: e.g. "an allocation of at least 64 bytes" +/// reliably matches a specific long string's heap buffer (a +/// short-string-optimized `std::string` never calls `operator new` at all), +/// so a test picks a source string long enough to defeat SSO on every +/// supported standard library and lets the predicate find that allocation +/// regardless of how many smaller, unrelated allocations happen around it. +/// +/// @par Why @c matchToFail, not always the first match +/// Some call paths copy the same long value more than once before reaching +/// the statement a test actually wants to fail (e.g. a setup copy taken +/// before a dispatch call, followed by the copy a success callback makes +/// inside that same call stack, when the callback fires inline rather than +/// on a separate later call). @c matchToFail lets a test skip past known, +/// earlier same-shape copies to reach the one it means — still robust, +/// because it counts only allocations that already match @p minSize, not +/// every allocation the standard library happens to make; a copy of a +/// small, short-string-optimized helper value never shifts this count. +/// +/// @par Usage +/// ```cpp +/// { +/// // "expected" must be long enough that copying it heap-allocates on +/// // every supported STL (well past libstdc++/libc++/MSVC's ~15-23 +/// // byte SSO buffers). +/// std::string expected(128, 'x'); +/// morph::testkit::OomInjector inject{/*minSize=*/64}; +/// REQUIRE_THROWS_AS(codeThatCopiesExpectedSomewhere(expected), std::bad_alloc); +/// } // scope ends here: later allocations succeed normally again, even if +/// // the matching allocation never actually happened. +/// ``` +/// The @p matchToFail -th `operator new` call after construction whose +/// `size >= minSize` throws `std::bad_alloc` instead of allocating; every +/// smaller allocation, every earlier matching-size allocation before the +/// target occurrence, and every allocation of any size after it fires +/// (including inside the `catch` block itself) allocates normally: this is +/// a one-shot trigger, not a standing size filter. Only one `OomInjector` +/// may be active per thread at a time — nesting two throws +/// `std::logic_error` from the inner constructor, since both would +/// otherwise race over the same thread-local state. +/// +/// @par Thread safety +/// The injection state is `thread_local`: an `OomInjector` constructed on +/// one thread only affects `operator new` calls made from that same thread. +/// Other threads' allocations are never affected, so a test that spins up +/// worker threads around the code under test must construct the injector on +/// whichever thread actually performs the allocation it wants to fail. +/// +/// @par Why this isn't a `morph::core::FileIoOps`-shaped injectable parameter +/// `FileIoOps` works because `FileActionLog`/`FileOfflineQueue` call the raw +/// syscall directly and can take an injectable strategy object in their own +/// constructor. The `catch (...)` blocks this class targets guard a plain +/// `std::string` copy-assignment with no such seam to thread through -- the +/// allocation happens inside `std::string::operator=` itself, several layers +/// below any parameter `Bridge` could plausibly accept. Overriding the +/// global allocator is the standard technique for exactly this shape of gap. +class OomInjector { + public: + /// @brief Arms the injector so the @p matchToFail -th future `operator + /// new` call on this thread (counting from 1) whose requested + /// size is at least @p minSize throws `std::bad_alloc` instead of + /// allocating. Smaller allocations, and matching-size allocations + /// before the target occurrence, succeed normally and do not + /// otherwise affect the trigger. + /// @param minSize Minimum allocation size, in bytes, that counts toward + /// @p matchToFail. + /// @param matchToFail Which occurrence (counting from 1) of an + /// allocation `>= minSize` should fail. Defaults to 1: the first + /// one. + /// @throws std::logic_error if another `OomInjector` is already active + /// on this thread. + // NOLINTNEXTLINE(bugprone-easily-swappable-parameters) -- both are named + // for exactly what they mean at any call site (minSize=.../matchToFail=...), + // and matchToFail's default makes the common one-argument call unambiguous. + explicit OomInjector(std::size_t minSize, std::size_t matchToFail = 1); + + /// @brief Disarms the injector. Allocations on this thread after this + /// point succeed normally again, whether or not the matching + /// allocation ever actually happened. + ~OomInjector(); + + OomInjector(const OomInjector&) = delete; + OomInjector& operator=(const OomInjector&) = delete; + OomInjector(OomInjector&&) = delete; + OomInjector& operator=(OomInjector&&) = delete; +}; + +} // namespace morph::testkit diff --git a/tests/test_async_registration.cpp b/tests/test_async_registration.cpp index 6741dd8b..274ed45c 100644 --- a/tests/test_async_registration.cpp +++ b/tests/test_async_registration.cpp @@ -22,6 +22,7 @@ #include #include #include +#include #include #include #include @@ -31,6 +32,7 @@ #include #include +#include "oom_injector.hpp" #include "test_support.hpp" namespace { @@ -1822,3 +1824,146 @@ TEST_CASE("ensureBoundAsync's out-of-frame success callback is a genuine no-op o REQUIRE_NOTHROW(rawBackend->completeNext()); SUCCEED("completing a registerModelSharedAsync reply after the binding itself is gone did not crash"); } + +// --------------------------------------------------------------------------- +// Coverage for LASTRADA-Software/morph#108: attachHandlerAsync's two +// success-path `catch (...)` blocks (the out-of-frame callback below, and its +// in-frame claimHandoff counterpart) only ever fire on std::bad_alloc from a +// real allocation failure inside the strongBinding->contextKey/primary copy- +// assignment. morph::testkit::OomInjector (see oom_injector.hpp) makes that +// failure happen for real, on demand, instead of leaving both branches +// permanently undocumented-but-untested. +// +// AOmKeyModel below is std::string-keyed (PrimaryKeyOf == std::string) so its +// primary key is morph::model::keyToString's std::string pass-through, not a +// std::to_string of an integer -- the test picks a key long enough (256 +// bytes) to defeat every supported standard library's short-string +// optimization, so copying it genuinely allocates and OomInjector's size +// predicate has a real allocation to catch. +// NOLINTBEGIN(misc-use-internal-linkage) -- glaze's reflection needs these to +// be externally linked, not file-local, unlike the anonymous-namespace types +// above. +struct AOmTouch { + std::string key; + int amount = 0; +}; + +struct AOmKeyModel { + using PrimaryKey = std::string; + int value = 0; + int execute(const AOmTouch& act) { + value += act.amount; + return value; + } +}; + +BRIDGE_REGISTER_MODEL(AOmKeyModel, "AOm_KeyModel") +BRIDGE_REGISTER_ACTION(AOmKeyModel, AOmTouch, "AOm_Touch") +BRIDGE_KEY_FROM(AOmTouch, &AOmTouch::key); +// NOLINTEND(misc-use-internal-linkage) + +TEST_CASE( + "attachHandlerAsync's out-of-frame success callback surfaces a real allocation failure through onDone " + "(morph#108)", + "[bridge][registration][issue108]") { + SyncExec cbExec; + auto backend = std::make_shared(); + morph::bridge::Bridge bridge{std::make_unique(backend)}; + morph::bridge::BridgeHandler handler{bridge, &cbExec}; + + // 256 bytes defeats SSO on every supported standard library (libstdc++, + // libc++, and MSVC's implementations all inline at most ~23 bytes), so + // copying it into strongBinding->contextKey/primary genuinely allocates. + std::string const longKey(256, 'k'); + std::atomic result{-1}; + std::atomic failed{false}; + auto pending = handler.execute(AOmTouch{.key = longKey, .amount = 7}); + pending.then([&](int val) { result.store(val); }).onError([&](const std::exception_ptr&) { failed.store(true); }); + + REQUIRE(backend->pendingCount() == 1); + + // Arm right before the reply is completed: the attach's own dispatch + // machinery (registerModel/factory()) allocates plenty, but none of it + // copies `longKey` -- only the success callback's `strongBinding-> + // contextKey = primaryCopy` does, and it is >= 128 bytes, so the + // threshold only ever matches that copy, not the setup noise before it. + { + morph::testkit::OomInjector inject{/*minSize=*/128}; + backend->completeNext(); + } + + REQUIRE(morph::testing::waitUntil([&] { return result.load() != -1 || failed.load(); })); + CHECK(result.load() == -1); + CHECK(failed.load()); + // The failed publish left the handler unattached, exactly like any other + // attach failure -- not a half-published, corrupted binding. + CHECK_FALSE(handler.primary().has_value()); + + // The handler is still usable afterward: a fresh attach with a normal, + // short key succeeds, proving the injected failure left no corruption + // behind (the injector already disarmed itself after firing once). + std::atomic secondResult{-1}; + handler.execute(AOmTouch{.key = "short", .amount = 3}) + .then([&](int val) { secondResult.store(val); }) + .onError([&](const std::exception_ptr&) { failed.store(true); }); + REQUIRE(backend->pendingCount() == 1); + backend->completeNext(); + REQUIRE(morph::testing::waitUntil([&] { return secondResult.load() != -1; })); + CHECK(secondResult.load() == 3); + CHECK(handler.primary().value_or("") == "short"); +} + +TEST_CASE( + "attachHandlerAsync's in-frame claimHandoff success path surfaces a real allocation failure through onDone " + "(morph#108)", + "[bridge][registration][issue108]") { + SyncExec cbExec; + auto backend = std::make_unique(); + morph::bridge::Bridge bridge{std::move(backend)}; + morph::bridge::BridgeHandler handler{bridge, &cbExec}; + + std::string const longKey(256, 'k'); + std::atomic result{-1}; + std::atomic failed{false}; + + // InlineCompletingBackend answers attachModelAsync synchronously, inside + // execute()'s own dispatch frame -- this drives claimHandoff's success + // path (binding->contextKey = primaryCopy) rather than the out-of-frame + // callback the test above targets. Unlike that test, the whole call + // happens in one stack: attachHandlerAsync makes several of its own + // copies of `longKey` before ever reaching claimHandoff's try block (the + // by-value `primary` parameter, `auto primaryCopy = primary;`, the + // {.contextKey=, .primary=} aggregate's two fields, and the dispatch + // lambda's by-value capture) -- each is a >=260-byte allocation (272 + // bytes measured: a 256-byte string plus its heap-block header), the + // same shape as the target copy itself, so minSize alone cannot tell + // them apart from `binding->contextKey = primaryCopy` below. + // matchToFail=5 skips the first 4 (confirmed empirically against this + // exact call path: 5 total size>=260 allocations occur end to end, with + // the 5th being the target -- verified by temporarily setting + // matchToFail far beyond 5 and observing every match still succeeds + // normally, then dropping to exactly 5 and confirming it throws inside + // claimHandoff's try, not anywhere earlier). This is still not a raw + // process-wide allocation count (which was rejected as fragile): it only + // counts allocations that already match minSize=260, and every + // unrelated smaller allocation in the dispatch chain (confirmed up to + // 224 bytes for shared_ptr control blocks and similar) never shifts + // which occurrence is "the 5th". + { + morph::testkit::OomInjector inject{/*minSize=*/260, /*matchToFail=*/5}; + handler.execute(AOmTouch{.key = longKey, .amount = 7}) + .then([&](int val) { result.store(val); }) + .onError([&](const std::exception_ptr&) { failed.store(true); }); + } + + CHECK(result.load() == -1); + CHECK(failed.load()); + CHECK_FALSE(handler.primary().has_value()); + + std::atomic secondResult{-1}; + handler.execute(AOmTouch{.key = "short", .amount = 3}) + .then([&](int val) { secondResult.store(val); }) + .onError([&](const std::exception_ptr&) { failed.store(true); }); + CHECK(secondResult.load() == 3); + CHECK(handler.primary().value_or("") == "short"); +} diff --git a/tests/test_oom_injector.cpp b/tests/test_oom_injector.cpp new file mode 100644 index 00000000..1a6704bc --- /dev/null +++ b/tests/test_oom_injector.cpp @@ -0,0 +1,63 @@ +// SPDX-License-Identifier: Apache-2.0 +// +// Self-test for morph::testkit::OomInjector (oom_injector.hpp), the same way +// scripts/test_check_deprecated_markers.sh and +// scripts/test_check_test_type_names.sh self-test their own checkers before +// anything else relies on them: a fault-injection seam nobody tests reports +// "it works" whether or not it actually fires the failure it claims to. + +#include +#include +#include +#include + +#include "oom_injector.hpp" + +TEST_CASE("OomInjector: an allocation below the threshold succeeds normally", "[testkit][oom-injector]") { + morph::testkit::OomInjector inject{/*minSize=*/1024}; + // A short string is well within SSO on every supported STL; even if it + // weren't, its heap buffer is far below the 1024-byte threshold above. + std::string small = "short"; + CHECK(small == "short"); +} + +TEST_CASE("OomInjector: the first allocation at or above the threshold throws std::bad_alloc", + "[testkit][oom-injector]") { + // 256 bytes defeats SSO on every supported standard library (libstdc++, + // libc++, and MSVC's implementations all inline at most ~23 bytes). + std::string source(256, 'x'); + morph::testkit::OomInjector inject{/*minSize=*/128}; + std::string target; + REQUIRE_THROWS_AS(target = source, std::bad_alloc); +} + +TEST_CASE("OomInjector: is one-shot -- only the first matching allocation fails, not every later one", + "[testkit][oom-injector]") { + std::string source(256, 'x'); + morph::testkit::OomInjector inject{/*minSize=*/128}; + std::string first; + REQUIRE_THROWS_AS(first = source, std::bad_alloc); + // The injector fired once and disarmed itself; a second copy of the same + // large string must now succeed normally, in the same scope. + std::string second; + REQUIRE_NOTHROW(second = source); + CHECK(second == source); +} + +TEST_CASE("OomInjector: disarms when its scope ends, even if the matching allocation never happened", + "[testkit][oom-injector]") { + { + morph::testkit::OomInjector inject{/*minSize=*/128}; + // Deliberately do not perform any allocation >= 128 bytes here. + } + // The armed-but-never-triggered injector must not leak into later code. + std::string source(256, 'x'); + std::string target; + REQUIRE_NOTHROW(target = source); + CHECK(target == source); +} + +TEST_CASE("OomInjector: two instances on the same thread cannot be active at once", "[testkit][oom-injector]") { + morph::testkit::OomInjector outer{/*minSize=*/128}; + REQUIRE_THROWS_AS((morph::testkit::OomInjector{/*minSize=*/128}), std::logic_error); +} From 50b2e90dfba81aa53ddc6b1a470f541c1c1d195b Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Sun, 16 Aug 2026 12:54:30 +0300 Subject: [PATCH 2/4] tests: drop the OomInjector in-frame test, revert matchToFail (CI-confirmed cross-STL flakiness) The in-frame claimHandoff test's matchToFail=5 was tuned empirically against MSVC's allocator (the only local build available) and did not reproduce on clang/libstdc++ or gcc/libstdc++ in CI: the number of same-shaped copies of the primary key that attachHandlerAsync makes before reaching the target catch (...) block is itself an STL/compiler implementation detail, exactly the kind of fragility the size-predicate design was meant to avoid. Confirmed via CI logs on LASTRADA-Software/morph#110: every Linux leg (clang and gcc alike) failed with result == 7 instead of -1, meaning the injected failure never fired. Removes the in-frame test and the matchToFail parameter it was the sole safe use case for, rather than re-tuning per-platform magic numbers. The out-of-frame test (already portable -- uses minSize alone, with no occurrence count, and passed on every CI leg) still closes one of the two catch (...) blocks morph#108 named for real; the in-frame one is documented in-code as the same shape, deliberately not forced with a second, unportable test. Full suite: 1052 test cases, 10057 assertions, all passing. Co-Authored-By: Claude Sonnet 5 --- tests/oom_injector.cpp | 33 +++++------- tests/oom_injector.hpp | 85 +++++++++++++++---------------- tests/test_async_registration.cpp | 73 +++++++------------------- 3 files changed, 71 insertions(+), 120 deletions(-) diff --git a/tests/oom_injector.cpp b/tests/oom_injector.cpp index 991eb854..df98e22f 100644 --- a/tests/oom_injector.cpp +++ b/tests/oom_injector.cpp @@ -9,19 +9,17 @@ namespace { // injectorArmed off means no injector active on this thread. While armed, -// every operator new call with size >= minSizeToFail counts down -// remainingMatchesUntilFailure; it throws when a matching call brings that -// count to 0, then disarms (one-shot). Plain built-in types only -- these -// variables' own reads/writes must never themselves allocate, or arming the -// injector would recurse into itself the moment operator new next runs. -// thread_local, not a class member: operator new below is a free function -// with no `this` to hang state off, and every thread needs its own -// independent state (see the header's own @par Thread safety). +// the next operator new call with size >= minSizeToFail throws and disarms +// (one-shot). Plain built-in types only -- these variables' own +// reads/writes must never themselves allocate, or arming the injector would +// recurse into itself the moment operator new next runs. thread_local, not +// a class member: operator new below is a free function with no `this` to +// hang state off, and every thread needs its own independent state (see the +// header's own @par Thread safety). // NOLINTBEGIN(cppcoreguidelines-avoid-non-const-global-variables) -- this // state *is* the seam; there is no non-global way to reach into a bare // operator new call from outside. thread_local std::size_t minSizeToFail = 0; -thread_local std::size_t remainingMatchesUntilFailure = 0; thread_local bool injectorArmed = false; // NOLINTEND(cppcoreguidelines-avoid-non-const-global-variables) @@ -29,19 +27,17 @@ thread_local bool injectorArmed = false; namespace morph::testkit { -OomInjector::OomInjector(std::size_t minSize, std::size_t matchToFail) { +OomInjector::OomInjector(std::size_t minSize) { if (injectorArmed) { throw std::logic_error("OomInjector: another instance is already active on this thread"); } injectorArmed = true; minSizeToFail = minSize; - remainingMatchesUntilFailure = matchToFail == 0 ? 1 : matchToFail; } OomInjector::~OomInjector() { injectorArmed = false; minSizeToFail = 0; - remainingMatchesUntilFailure = 0; } } // namespace morph::testkit @@ -54,15 +50,10 @@ namespace { // cannot re-enter itself. void* allocateOrInject(std::size_t size) { if (injectorArmed && size >= minSizeToFail) { - if (remainingMatchesUntilFailure <= 1) { - injectorArmed = false; // one-shot: disarm before throwing, so - // the catch block itself (and anything - // else on this thread afterward) - // allocates normally. - remainingMatchesUntilFailure = 0; - throw std::bad_alloc{}; - } - --remainingMatchesUntilFailure; + injectorArmed = false; // one-shot: disarm before throwing, so the + // catch block itself (and anything else on + // this thread afterward) allocates normally. + throw std::bad_alloc{}; } // NOLINTNEXTLINE(cppcoreguidelines-no-malloc, cppcoreguidelines-owning-memory) -- // this *is* the process-wide operator new/delete pair; std::malloc/free diff --git a/tests/oom_injector.hpp b/tests/oom_injector.hpp index a3c172ce..36ba75ea 100644 --- a/tests/oom_injector.hpp +++ b/tests/oom_injector.hpp @@ -5,13 +5,12 @@ namespace morph::testkit { -/// @brief RAII scope that makes the @c matchToFail -th `operator new` call -/// whose requested size is at least @c minSize throw `std::bad_alloc` -/// on the current thread, so a test can drive a real -/// allocation-failure path (a `catch (...)` around a -/// `std::string`/container assignment, for example) without -/// exhausting process memory or forking/mocking the standard -/// allocator. +/// @brief RAII scope that makes the next `operator new` call whose +/// requested size is at least @c minSize throw `std::bad_alloc` on +/// the current thread, so a test can drive a real allocation-failure +/// path (a `catch (...)` around a `std::string`/container assignment, +/// for example) without exhausting process memory or forking/mocking +/// the standard allocator. /// /// Test-only tooling, not part of the shipped library: `oom_injector.cpp` /// defines `operator new`/`operator delete` for the whole binary it links @@ -26,8 +25,8 @@ namespace morph::testkit { /// real allocation failure -- there is no other way into them. That is not /// portably reachable from a unit test without a seam: this class overrides /// the process-wide `operator new`/`operator new[]` (defined once in -/// `oom_injector.cpp`) to consult thread-local state before delegating to -/// the real allocator. +/// `oom_injector.cpp`) to consult a thread-local size threshold before +/// delegating to the real allocator. /// /// @par Why a size predicate, not a raw allocation count /// An earlier design counted "the Nth `operator new` call after arming" — @@ -38,23 +37,28 @@ namespace morph::testkit { /// compiler/library upgrade. A test would silently start failing the wrong /// allocation (or none at all) the moment that count changed, without /// necessarily failing loudly. A size predicate instead targets what the -/// allocation actually *is*: e.g. "an allocation of at least 64 bytes" -/// reliably matches a specific long string's heap buffer (a +/// allocation actually *is*: e.g. "the next allocation of at least 64 +/// bytes" reliably matches a specific long string's heap buffer (a /// short-string-optimized `std::string` never calls `operator new` at all), /// so a test picks a source string long enough to defeat SSO on every /// supported standard library and lets the predicate find that allocation /// regardless of how many smaller, unrelated allocations happen around it. /// -/// @par Why @c matchToFail, not always the first match -/// Some call paths copy the same long value more than once before reaching -/// the statement a test actually wants to fail (e.g. a setup copy taken -/// before a dispatch call, followed by the copy a success callback makes -/// inside that same call stack, when the callback fires inline rather than -/// on a separate later call). @c matchToFail lets a test skip past known, -/// earlier same-shape copies to reach the one it means — still robust, -/// because it counts only allocations that already match @p minSize, not -/// every allocation the standard library happens to make; a copy of a -/// small, short-string-optimized helper value never shifts this count. +/// @par Why there is no occurrence-count parameter +/// A call path that copies the same long value more than once before +/// reaching the statement a test actually wants to fail (e.g. a setup copy +/// taken before a dispatch call, followed by another copy inside that same +/// call stack when a callback fires inline) cannot be disambiguated by size +/// alone. An earlier revision added a "skip the first K-1 matches" +/// occurrence count to handle exactly that case — and it was removed after +/// a value tuned against one STL's allocator (MSVC) failed to reproduce on +/// two others (libstdc++, libc++) in CI: the number of same-shaped copies +/// before a target line is itself an implementation detail, not something +/// this seam can portably parameterise around. A call path with that shape +/// is not a good fit for `OomInjector` — pick a different reproduction (a +/// path where the target copy is the *first* allocation of its size, as the +/// out-of-frame test in `test_async_registration.cpp` does) rather than +/// counting occurrences. /// /// @par Usage /// ```cpp @@ -68,15 +72,14 @@ namespace morph::testkit { /// } // scope ends here: later allocations succeed normally again, even if /// // the matching allocation never actually happened. /// ``` -/// The @p matchToFail -th `operator new` call after construction whose -/// `size >= minSize` throws `std::bad_alloc` instead of allocating; every -/// smaller allocation, every earlier matching-size allocation before the -/// target occurrence, and every allocation of any size after it fires -/// (including inside the `catch` block itself) allocates normally: this is -/// a one-shot trigger, not a standing size filter. Only one `OomInjector` -/// may be active per thread at a time — nesting two throws -/// `std::logic_error` from the inner constructor, since both would -/// otherwise race over the same thread-local state. +/// The first `operator new` call after construction whose `size >= minSize` +/// throws `std::bad_alloc` instead of allocating; every smaller allocation +/// before it (and every allocation, of any size, after it fires — including +/// inside the `catch` block itself) allocates normally: this is a one-shot +/// trigger, not a standing size filter. Only one `OomInjector` may be active +/// per thread at a time — nesting two throws `std::logic_error` from the +/// inner constructor, since both would otherwise race over the same +/// thread-local state. /// /// @par Thread safety /// The injection state is `thread_local`: an `OomInjector` constructed on @@ -95,23 +98,15 @@ namespace morph::testkit { /// global allocator is the standard technique for exactly this shape of gap. class OomInjector { public: - /// @brief Arms the injector so the @p matchToFail -th future `operator - /// new` call on this thread (counting from 1) whose requested - /// size is at least @p minSize throws `std::bad_alloc` instead of - /// allocating. Smaller allocations, and matching-size allocations - /// before the target occurrence, succeed normally and do not - /// otherwise affect the trigger. - /// @param minSize Minimum allocation size, in bytes, that counts toward - /// @p matchToFail. - /// @param matchToFail Which occurrence (counting from 1) of an - /// allocation `>= minSize` should fail. Defaults to 1: the first - /// one. + /// @brief Arms the injector so the first future `operator new` call on + /// this thread whose requested size is at least @p minSize + /// throws `std::bad_alloc` instead of allocating. Smaller + /// allocations before it succeed normally and do not count + /// against the trigger. + /// @param minSize Minimum allocation size, in bytes, that should fail. /// @throws std::logic_error if another `OomInjector` is already active /// on this thread. - // NOLINTNEXTLINE(bugprone-easily-swappable-parameters) -- both are named - // for exactly what they mean at any call site (minSize=.../matchToFail=...), - // and matchToFail's default makes the common one-argument call unambiguous. - explicit OomInjector(std::size_t minSize, std::size_t matchToFail = 1); + explicit OomInjector(std::size_t minSize); /// @brief Disarms the injector. Allocations on this thread after this /// point succeed normally again, whether or not the matching diff --git a/tests/test_async_registration.cpp b/tests/test_async_registration.cpp index 274ed45c..95d25240 100644 --- a/tests/test_async_registration.cpp +++ b/tests/test_async_registration.cpp @@ -1913,57 +1913,22 @@ TEST_CASE( CHECK(handler.primary().value_or("") == "short"); } -TEST_CASE( - "attachHandlerAsync's in-frame claimHandoff success path surfaces a real allocation failure through onDone " - "(morph#108)", - "[bridge][registration][issue108]") { - SyncExec cbExec; - auto backend = std::make_unique(); - morph::bridge::Bridge bridge{std::move(backend)}; - morph::bridge::BridgeHandler handler{bridge, &cbExec}; - - std::string const longKey(256, 'k'); - std::atomic result{-1}; - std::atomic failed{false}; - - // InlineCompletingBackend answers attachModelAsync synchronously, inside - // execute()'s own dispatch frame -- this drives claimHandoff's success - // path (binding->contextKey = primaryCopy) rather than the out-of-frame - // callback the test above targets. Unlike that test, the whole call - // happens in one stack: attachHandlerAsync makes several of its own - // copies of `longKey` before ever reaching claimHandoff's try block (the - // by-value `primary` parameter, `auto primaryCopy = primary;`, the - // {.contextKey=, .primary=} aggregate's two fields, and the dispatch - // lambda's by-value capture) -- each is a >=260-byte allocation (272 - // bytes measured: a 256-byte string plus its heap-block header), the - // same shape as the target copy itself, so minSize alone cannot tell - // them apart from `binding->contextKey = primaryCopy` below. - // matchToFail=5 skips the first 4 (confirmed empirically against this - // exact call path: 5 total size>=260 allocations occur end to end, with - // the 5th being the target -- verified by temporarily setting - // matchToFail far beyond 5 and observing every match still succeeds - // normally, then dropping to exactly 5 and confirming it throws inside - // claimHandoff's try, not anywhere earlier). This is still not a raw - // process-wide allocation count (which was rejected as fragile): it only - // counts allocations that already match minSize=260, and every - // unrelated smaller allocation in the dispatch chain (confirmed up to - // 224 bytes for shared_ptr control blocks and similar) never shifts - // which occurrence is "the 5th". - { - morph::testkit::OomInjector inject{/*minSize=*/260, /*matchToFail=*/5}; - handler.execute(AOmTouch{.key = longKey, .amount = 7}) - .then([&](int val) { result.store(val); }) - .onError([&](const std::exception_ptr&) { failed.store(true); }); - } - - CHECK(result.load() == -1); - CHECK(failed.load()); - CHECK_FALSE(handler.primary().has_value()); - - std::atomic secondResult{-1}; - handler.execute(AOmTouch{.key = "short", .amount = 3}) - .then([&](int val) { secondResult.store(val); }) - .onError([&](const std::exception_ptr&) { failed.store(true); }); - CHECK(secondResult.load() == 3); - CHECK(handler.primary().value_or("") == "short"); -} +// attachHandlerAsync's in-frame claimHandoff success path (binding-> +// contextKey = primaryCopy, reached when a backend's attachModelAsync +// completes synchronously -- see InlineCompletingBackend above) has the +// identical shape of catch (...) as the out-of-frame callback the test above +// targets, and is deliberately NOT given its own forced-OOM test: the whole +// call happens in one stack, so several of attachHandlerAsync's own earlier +// copies of the same primary key (the by-value `primary` parameter, `auto +// primaryCopy = primary;`, the {.contextKey=, .primary=} aggregate's two +// fields, the dispatch lambda's by-value capture) are the same >=SSO- +// defeating size as the target copy itself -- so a minSize-only OomInjector +// can't distinguish them, and the number of such copies before the target +// line is a real STL/compiler implementation detail (confirmed: an +// occurrence-count value tuned against MSVC's allocator did not reproduce on +// clang/libstdc++ or gcc/libstdc++ in CI, silently passing through instead +// of catching the target allocation). Forcing this specific occurrence +// portably would need either a structural change that gives the target copy +// a distinguishable allocation shape, or a seam finer-grained than a global +// allocator override can offer -- disproportionate machinery for one +// branch. Tracked by the same morph#108, not a second, separate ask. From 2b1f22d1ea081c31d6b5b8d6d69fbfefaffe4bfb Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Sun, 16 Aug 2026 13:32:29 +0300 Subject: [PATCH 3/4] ci: exclude OomInjector-based tests from ASan, TSan, and Valgrind Two real, structural incompatibilities surfaced in CI on PR #110, not tuning issues: - clang-asan/clang-tsan: ASan and TSan's own runtimes already interpose operator new/operator delete themselves. Linking oom_injector.cpp's own definitions alongside either sanitizer's runtime fails at link time with "multiple definition of `operator new(unsigned long)'" against libclang_rt.{asan,tsan}_cxx.a. - Valgrind (gcc-debug leg): memcheck intercepts allocations at a layer this override does not reach, so the injector silently never fires under it -- confirmed by its own self-tests failing there ("no exception was thrown where one was expected"). Excludes every test tagged [oom-injector]/[issue108] on exactly these three legs: `ctest -E "OomInjector|morph#108"` for clang-asan/ clang-tsan (matched against CTest's own discovered test names), and a Catch2 tag filter (`"~[oom-injector]" "~[issue108]"`) for the Valgrind leg's direct binary invocation. Every other CI leg (plain clang/gcc, Windows, ubsan, coverage) runs these tests normally -- confirmed locally: full suite still 1052 test cases / 10057 assertions passing with no exclusion applied. oom_injector.hpp documents both incompatibilities and the exclusion mechanism, so a future test using this seam knows to carry one of those two tags. Co-Authored-By: Claude Sonnet 5 --- .github/workflows/ci.yml | 30 +++++++++++++++++++++++++++--- tests/oom_injector.hpp | 13 +++++++++++++ 2 files changed, 40 insertions(+), 3 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 0e02b2fa..b29bfeca 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -255,10 +255,22 @@ jobs: # on a runner with no display. QT_QPA_PLATFORM: offscreen run: | + # morph::testkit::OomInjector (tests/oom_injector.cpp) overrides + # the process-wide operator new/delete to force std::bad_alloc on + # demand -- ASan and TSan's own runtimes already interpose + # operator new/delete themselves, and linking a second, competing + # definition fails with "multiple definition of `operator + # new(unsigned long)'" (confirmed in CI). Excluded by test name on + # exactly these two legs; every other CI leg (plain clang/gcc, + # Windows, ubsan, coverage) runs these tests normally. + EXCLUDE_ARGS=() + if [ "${{ matrix.preset }}" = "clang-asan" ] || [ "${{ matrix.preset }}" = "clang-tsan" ]; then + EXCLUDE_ARGS+=(-E "OomInjector|morph#108") + fi if [ "${{ matrix.preset }}" = "clang-coverage" ]; then - LLVM_PROFILE_FILE="build/clang-coverage/%p.profraw" ctest --preset clang-coverage + LLVM_PROFILE_FILE="build/clang-coverage/%p.profraw" ctest --preset clang-coverage "${EXCLUDE_ARGS[@]}" else - ctest --preset ${{ matrix.preset }} + ctest --preset ${{ matrix.preset }} "${EXCLUDE_ARGS[@]}" fi - name: Generate coverage report @@ -688,6 +700,18 @@ jobs: exit 1 fi echo "── memcheck: $suite ──" + # [oom-injector]/[issue108]: morph::testkit::OomInjector + # overrides the process-wide operator new/delete + # (tests/oom_injector.cpp) to force std::bad_alloc on demand -- + # Valgrind's own memcheck instrumentation intercepts allocations + # at a layer this override does not reach, so the injector + # silently never fires under Valgrind (confirmed: even its own + # self-tests fail here). Skipped for this leg only; every other + # CI leg (plain clang/gcc, Windows) runs these tests normally. + EXTRA_ARGS=() + if [ "$suite" = "tests/morph_tests" ]; then + EXTRA_ARGS=("~[oom-injector]" "~[issue108]") + fi valgrind \ --tool=memcheck \ --leak-check=full \ @@ -695,7 +719,7 @@ jobs: --errors-for-leak-kinds=definite,indirect \ --error-exitcode=1 \ --suppressions=cmake/valgrind.supp \ - "$binary" || status=1 + "$binary" "${EXTRA_ARGS[@]}" || status=1 done exit "$status" diff --git a/tests/oom_injector.hpp b/tests/oom_injector.hpp index 36ba75ea..fc902e44 100644 --- a/tests/oom_injector.hpp +++ b/tests/oom_injector.hpp @@ -19,6 +19,19 @@ namespace morph::testkit { /// app -- a real consumer's process must never get its allocator replaced /// by test-only code. /// +/// @par Incompatible with ASan, TSan, and Valgrind +/// ASan and TSan's own runtimes already interpose `operator new`/`operator +/// delete` themselves; linking this seam's own definitions alongside either +/// one fails at link time with "multiple definition of `operator +/// new(unsigned long)'" (confirmed in CI). Valgrind's memcheck intercepts +/// allocations at a layer this override does not reach, so the injector +/// silently never fires under it (confirmed: even its own self-tests fail +/// there). `.github/workflows/ci.yml` excludes every test tagged +/// `[oom-injector]`/`[issue108]` on the `clang-asan`/`clang-tsan` legs (by +/// `ctest -E`) and the Valgrind leg (by Catch2 tag filter) for exactly this +/// reason -- a test using this seam must carry one of those tags so it is +/// excluded consistently on every leg where the override cannot work. +/// /// @par Why this exists /// Several `catch (...)` blocks across the codebase (see /// `LASTRADA-Software/morph#108`) only ever fire on `std::bad_alloc` from a From 5a0fb5f901deaa34e8ff2f5b3bf686c76f984043 Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Sun, 16 Aug 2026 13:51:41 +0300 Subject: [PATCH 4/4] tests: actually compile out OomInjector's operator new/delete under ASan/TSan The previous fix only excluded OomInjector-based tests at the ctest level (ctest -E) on clang-asan/clang-tsan -- that doesn't help, because oom_injector.cpp's own operator new/delete overloads were still compiled and linked into the whole morph_tests binary regardless of which tests actually run. The link-time conflict against ASan/TSan's own operator-new interposition (libclang_rt.{asan,tsan}_cxx.a) happens before any test executes at all, so runtime test exclusion can never prevent it. Confirmed still failing on PR #110 after the first fix with the identical "multiple definition of `operator new(unsigned long)'" error. Actually compiles the overloads out via __SANITIZE_ADDRESS__/ __SANITIZE_THREAD__ (GCC and Clang both define these under -fsanitize=address/thread) plus a Clang-only __has_feature fallback, using nested #ifdef/#if blocks rather than one combined boolean expression -- MSVC's preprocessor doesn't define __has_feature and choked on `defined(__has_feature) && (__has_feature(...))` on a single line (C1012, unmatched parenthesis), so the detection is restructured to never evaluate __has_feature(...) except already inside an `#elif defined(__has_feature)` block. OomInjector's constructor now throws a clear std::logic_error under that configuration instead of silently doing nothing, as a correctness backstop -- the CI-level ctest -E exclusion (kept from the prior fix) remains the actual mechanism that prevents this from ever firing in practice. Verified directly: `clang++ -fsanitize=address -c tests/oom_injector.cpp` compiles clean with zero operator-new/delete symbols in the resulting object file (confirmed via llvm-nm); the same file compiled without -fsanitize=address still emits them. Full local suite (MSVC, non-sanitized): 1052 test cases, 10057 assertions, all passing. Co-Authored-By: Claude Sonnet 5 --- tests/oom_injector.cpp | 51 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 51 insertions(+) diff --git a/tests/oom_injector.cpp b/tests/oom_injector.cpp index df98e22f..ef0af605 100644 --- a/tests/oom_injector.cpp +++ b/tests/oom_injector.cpp @@ -6,6 +6,38 @@ #include #include +// ASan/TSan already define their own operator new/operator new[]/operator +// delete/operator delete[] inside their runtime (libclang_rt.{asan,tsan}_cxx.a) +// to track allocations for their own instrumentation. Defining this file's +// own overloads under either sanitizer fails at *link* time with "multiple +// definition of `operator new(unsigned long)'" against that runtime archive +// -- ctest-level test exclusion (see .github/workflows/ci.yml) cannot help +// here, since the conflict happens before any test ever runs. +// +// Detected via nested #ifdef/#if blocks (not one combined boolean +// expression): MSVC's preprocessor does not define __has_feature at all, and +// some preprocessors do not short-circuit `defined(__has_feature) && +// __has_feature(...)` on a single line the way C++ code would -- they can +// still try to macro-expand `__has_feature` as a bare identifier and choke +// on the unmatched parenthesis that follows (`__has_feature(address_ +// sanitizer)`) once `defined(__has_feature)` alone is false. Nesting avoids +// ever writing `__has_feature` on a line MSVC actually preprocesses. +// +// GCC and Clang both define __SANITIZE_ADDRESS__/__SANITIZE_THREAD__ +// whenever the corresponding -fsanitize=address/thread flag is active, which +// covers this repo's own clang-asan/clang-tsan presets without needing +// __has_feature at all; the __has_feature branch below only matters for a +// Clang invocation that enables a sanitizer through some other means. +#if defined(__SANITIZE_ADDRESS__) || defined(__SANITIZE_THREAD__) +#define MORPH_TESTKIT_UNDER_ASAN_OR_TSAN +#elif defined(__has_feature) +#if __has_feature(address_sanitizer) +#define MORPH_TESTKIT_UNDER_ASAN_OR_TSAN +#elif __has_feature(thread_sanitizer) +#define MORPH_TESTKIT_UNDER_ASAN_OR_TSAN +#endif +#endif + namespace { // injectorArmed off means no injector active on this thread. While armed, @@ -28,11 +60,26 @@ thread_local bool injectorArmed = false; namespace morph::testkit { OomInjector::OomInjector(std::size_t minSize) { +#ifdef MORPH_TESTKIT_UNDER_ASAN_OR_TSAN + // The operator new/delete overrides below are compiled out under this + // build (see the guard around them). Constructing an OomInjector here + // would silently do nothing, so this throws instead of letting a test + // misread that silence as "the injected failure never happened to + // trigger". In practice this never fires: every test using OomInjector + // is excluded from the clang-asan/clang-tsan CI legs by tag (see + // .github/workflows/ci.yml) -- this is a correctness backstop, not the + // primary mechanism. + (void)minSize; + throw std::logic_error( + "OomInjector: unusable under ASan/TSan (operator new/delete overrides are compiled out -- " + "see oom_injector.cpp)"); +#else if (injectorArmed) { throw std::logic_error("OomInjector: another instance is already active on this thread"); } injectorArmed = true; minSizeToFail = minSize; +#endif } OomInjector::~OomInjector() { @@ -42,6 +89,8 @@ OomInjector::~OomInjector() { } // namespace morph::testkit +#ifndef MORPH_TESTKIT_UNDER_ASAN_OR_TSAN + namespace { // Every operator new overload below funnels through this so the trigger @@ -117,3 +166,5 @@ void operator delete[](void* ptr, std::size_t size) noexcept { } // NOLINTEND(readability-inconsistent-declaration-parameter-name, // cppcoreguidelines-no-malloc, cppcoreguidelines-owning-memory) + +#endif // !MORPH_TESTKIT_UNDER_ASAN_OR_TSAN