tests: OOM-injection allocator seam, closes morph#108 - #110
Merged
Conversation
… 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 <noreply@anthropic.com>
…firmed 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 #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 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
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 <noreply@anthropic.com>
…San/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 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #108: adds a test-only allocator seam and
uses it to close both
catch (...)blocks inattachHandlerAsyncthatissue named (the out-of-frame success callback and the in-frame
claimHandoffcounterpart) -- both only ever fire on a genuinestd::bad_alloccopying a long primary key intocontextKey/primary.The seam:
morph::testkit::OomInjectortests/oom_injector.{hpp,cpp}overrides the process-wideoperator new/operator deletefor whichever test binary links it in(
morph_testsonly -- never the shippedmorphlibrary or any exampleapp). A test constructs one with a size threshold (and optionally which
occurrence of a matching-size allocation to fail); the next
operator newcall of at least that size throwsstd::bad_allocinstead ofallocating.
Lives under
tests/, notinclude/morph/core/: this is test-onlytooling with no production API surface, unlike
morph::core::FileIoOps(the #97 precedent) which is a real constructor parameter on real
library types. Overriding the global allocator is invasive by nature
(it affects every allocation in the binary), so it deliberately stays
out of the public, shipped headers.
Design note on why this is a size predicate, not a raw allocation
count: an earlier version counted "the Nth
operator newcall afterarming" and was rejected before it shipped -- that count depends on
exactly how many allocations the standard library and surrounding setup
code perform first, which shifts across STL implementations and even an
unrelated compiler/library upgrade, silently missing the intended
allocation. The shipped design instead matches on the allocation's
shape (size >= threshold), picking a source string long enough to
defeat SSO on every supported STL -- so unrelated smaller allocations
elsewhere in a call chain never affect which occurrence is being
counted. One test needed occurrence-counting too (see below); this
remains far more robust than a raw process-wide count because it only
counts allocations that already match the size predicate.
Self-tested first (
test_oom_injector.cpp) before anything else relieson it, matching this repo's established pattern
(
scripts/test_check_deprecated_markers.sh,scripts/test_check_test_type_names.sh): threshold behavior, one-shotfiring, RAII disarm-on-scope-exit, and the no-nesting guard.
The two tests it enables
AsyncRegisterBackend's deferredcompleteNext()reply drives the callback on a separate call from
execute()'s ownstack --
minSize=128alone reliably isolates the target copy here.InlineCompletingBackendanswers synchronously insideexecute()'s own call stack, so several ofattachHandlerAsync's ownsetup copies of the same long key happen first, before
claimHandoff'stryblock is ever reached.minSize=260(aboveevery incidental allocation in the chain, confirmed empirically) plus
matchToFail=5(the empirically-confirmed 5th and last matchingallocation, verified by first arming with a very high
matchToFailand observing exactly 5 total matches, all succeeding normally) lands
precisely on the target
binding->contextKey = primaryCopy.Both tests assert the failure surfaces through
onDone/onError, thebinding is left unattached rather than half-published, and the handler
is still usable afterward with a normal (short) key -- proving the
injected failure left no corruption behind.
Verification
Full
morph_testssuite: 1053 test cases, 10062 assertions, allpassing (up from 1046/10038 before this branch).
🤖 Generated with Claude Code