From 1913607eada671ba703450dc09f37fd6ac1f37b2 Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Wed, 9 Sep 2026 22:05:30 +0200 Subject: [PATCH 01/12] journal, offline: stop treating an unreadable file as an empty or torn one (#493, #494) Two file-backed stores scanned with an `ifstream` whose open they never checked, and both then committed the resulting empty read over the real file. `FileActionLog::repairTornTail()` (#493) probes with `_io.canOpenForRead` and then opens a *separate*, unchecked stream. When that open fails, `getline` runs zero times, `intactEnd` stays 0, and `resizeFile(_path, 0)` truncates the whole journal -- reported through the ordinary "discarded N byte(s) of a torn trailing record" warning, so it reads as a successful repair. Measured: three complete, fsynced entries (648 bytes) went to 0. The comment above the truncate argued the discard is safe because "whatever follows the final newline is by construction an incomplete record" -- true only of bytes the scan actually read, which is the assumption that was missing. `core/file_io_ops.hpp:76-78` already described the absent check as present ("stands in for repairTornTail()'s `if (!input)`"). Now: bail on a failed open, and bail on `input.bad()` -- a read error mid-scan leaves everything past `intactEnd` unread rather than established to be torn, and truncating there discards complete records too. `entries()` gets the same treatment, distinguishing "no journal yet" (absent, legitimately empty, which the constructor's dedup rebuild depends on) from "present but unreadable", which previously emptied the idempotencyKey dedup set OutboxRelay relies on and turned at-least-once-plus-dedup into duplicates with no diagnostic. `FileOfflineQueue::load()` (#494) is the same defect and needed no fault injection to reach: it bypasses the `FileIoOps` seam entirely with a raw `std::ifstream`, and the constructor calls `compact()` immediately after, which rewrites the file from the empty `_items`. Measured: three pending items (306 bytes) went to 0 with the constructor returning normally and the queue reporting an empty backlog. Both the failed-open and mid-read-error cases now throw, so compact() cannot run on a load that did not succeed. Also fixes the write ordering #494 reported alongside: `markDone()` erased from `_items` *before* `appendDone()`, so a throwing append (short write, failed fflush, failed fsync) left the item gone from memory with no tombstone on disk -- this process never replays it, and a restart resurrects and re-applies it. `enqueue()` already had the right order. `setAttempts()` had the same inversion. Now durable-first in both: a failure leaves the item live in both places, which replays once too often at worst, and `idempotencyKey` exists to absorb that. Regression tests for both, POSIX-only (making a file unreadable-but-writable is what reproduces the window; Windows maps permissions onto the read-only attribute alone) and skipped for a root euid. Verified they fail without the header changes and pass with them -- a test asserting only the happy path would have been green either way. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01SxkvtHvan2fWKDDaBpqkgv --- include/morph/journal/file_action_log.hpp | 27 ++++++++++ include/morph/offline/file_offline_queue.hpp | 33 +++++++++++- tests/test_action_log_phase2.cpp | 54 ++++++++++++++++++++ tests/test_file_offline_queue.cpp | 45 ++++++++++++++++ 4 files changed, 157 insertions(+), 2 deletions(-) diff --git a/include/morph/journal/file_action_log.hpp b/include/morph/journal/file_action_log.hpp index fdc1b5b9..68c43ebf 100644 --- a/include/morph/journal/file_action_log.hpp +++ b/include/morph/journal/file_action_log.hpp @@ -202,6 +202,14 @@ class FileActionLog : public IActionLog { [[nodiscard]] std::vector entries(std::string_view entityKey = {}) const override { std::scoped_lock const lock{_mtx}; std::ifstream in{_path}; + if (!in && std::filesystem::exists(_path)) { + // Distinguish "no journal yet" (absent: legitimately empty, and the + // constructor's dedup rebuild depends on that) from "journal present + // but unreadable". Returning {} for the second silently empties the + // idempotencyKey dedup set OutboxRelay relies on, turning + // at-least-once-plus-dedup into duplicates with no diagnostic. + throw std::runtime_error("FileActionLog: cannot read " + _path.string()); + } std::vector lines; std::string line; while (std::getline(in, line)) { @@ -341,6 +349,17 @@ class FileActionLog : public IActionLog { return; } std::ifstream input{_path, std::ios::binary}; + if (!input) { + // The probe above said readable and this open still failed (a + // permission change or fd exhaustion landing in between). Falling + // through would scan nothing, leave `intactEnd` at 0, and truncate + // the whole journal as if it were one torn record -- so bail + // instead. The safety argument below holds only for bytes this + // function actually read. + ::morph::log::logWarn("FileActionLog: could not read " + _path.string() + + " to check for a torn trailing record; leaving it untouched"); + return; + } std::uintmax_t intactEnd = 0; std::uintmax_t offset = 0; std::string line; @@ -352,6 +371,14 @@ class FileActionLog : public IActionLog { ++offset; // the '\n' getline consumed intactEnd = offset; } + if (input.bad()) { + // Terminated by an I/O error rather than by end-of-file, so + // everything past `intactEnd` is unread rather than established to + // be torn. Truncating here would discard complete, fsynced records. + ::morph::log::logWarn("FileActionLog: read error while checking " + _path.string() + + " for a torn trailing record; leaving it untouched"); + return; + } if (intactEnd == size) { return; } diff --git a/include/morph/offline/file_offline_queue.hpp b/include/morph/offline/file_offline_queue.hpp index 020edf27..fe56b2d6 100644 --- a/include/morph/offline/file_offline_queue.hpp +++ b/include/morph/offline/file_offline_queue.hpp @@ -240,10 +240,19 @@ class FileOfflineQueue : public IOfflineQueue { /// @param itemId Id returned by the corresponding `enqueue()` call. void markDone(uint64_t itemId) override { std::scoped_lock const lock{_mtx}; - if (_items.erase(itemId) == 0) { + auto iter = _items.find(itemId); + if (iter == _items.end()) { return; } + // Durable first, then in-memory -- the same order `enqueue()` uses, and + // for the mirror-image reason. `appendDone` throws on a short write, a + // failed fflush or a failed fsync; erasing before it means this process + // would never replay the item again while no tombstone reached disk, so + // a restart resurrects it and applies it a second time. Erasing after + // means a failure leaves the item live in both places, which replays + // once too often at worst -- and `idempotencyKey` exists to absorb that. appendDone(itemId); + _items.erase(iter); } /// @brief Persists an updated attempt count for @p itemId. No-op if not found. @@ -255,8 +264,14 @@ class FileOfflineQueue : public IOfflineQueue { if (iter == _items.end()) { return; } + // Durable first, for the same reason as `markDone` above: a throwing + // `appendPut` must not leave memory claiming a count that never reached + // disk. Write from a copy so `_items` is only updated once the record is + // durable. + auto updated = iter->second; + updated.attempts = attempts; + appendPut(updated); iter->second.attempts = attempts; - appendPut(iter->second); } protected: @@ -321,6 +336,15 @@ class FileOfflineQueue : public IOfflineQueue { return; } std::ifstream in{_path}; + if (!in) { + // The file exists (checked above) but cannot be read. Returning an + // empty `_items` here is not "an empty queue": the constructor calls + // compact() straight after load(), which would rewrite `_path` from + // that empty set and destroy every pending item. Throw so the caller + // learns the queue could not be opened, instead of being handed one + // that silently reports no work. + throw std::runtime_error("FileOfflineQueue: cannot read " + _path.string()); + } std::vector lines; std::string line; while (std::getline(in, line)) { @@ -329,6 +353,11 @@ class FileOfflineQueue : public IOfflineQueue { } } uint64_t highestId = 0; + if (in.bad()) { + // A read error mid-file, not end-of-file: `lines` is a prefix of the + // queue, and compact() would commit that prefix over the whole file. + throw std::runtime_error("FileOfflineQueue: read error on " + _path.string()); + } for (std::size_t i = 0; i < lines.size(); ++i) { detail::FileQueueRecord record; try { diff --git a/tests/test_action_log_phase2.cpp b/tests/test_action_log_phase2.cpp index 257f6c92..52317ca3 100644 --- a/tests/test_action_log_phase2.cpp +++ b/tests/test_action_log_phase2.cpp @@ -23,6 +23,9 @@ #include #include #include +#if !defined(_WIN32) +#include // geteuid, for the permission-based fault-injection cases below +#endif #include #include @@ -839,3 +842,54 @@ TEST_CASE("FileActionLog: a torn trailing record whose resize_file() fails is lo // but not corrupted further either. REQUIRE(std::filesystem::file_size(tmp.path) == sizeBefore); } + +// ── An unreadable journal must never be mistaken for a torn one (morph#493) ── +// +// repairTornTail() scans with an ifstream whose open it did not check, so a +// scan that never happened left `intactEnd` at 0 and truncated the whole file +// as one torn record -- reported through the ordinary "discarded N byte(s)" +// warning, so it looked like a successful repair. Measured before the fix: 3 +// complete, fsynced entries (648 bytes) went to 0. +// +// POSIX-only: making a file unreadable-but-writable is what reproduces the +// probe/open window, and Windows has no portable equivalent (std::filesystem +// maps permissions onto the read-only attribute alone). Skipped for a root +// euid, which ignores the permission bits entirely. +#if !defined(_WIN32) +TEST_CASE("FileActionLog: an unreadable journal is left intact, not truncated as a torn record", + "[action_log][phase2][file][fault-injection]") { + if (::geteuid() == 0) { + SUCCEED("running as root: permission bits are not enforced"); + return; + } + TempFile const tmp{"file_unreadable_not_torn"}; + std::uintmax_t sizeBefore = 0; + { + FileActionLog log{tmp.path}; + for (int i = 1; i <= 3; ++i) { + auto entry = makeEntry("P2_Model", "acct-" + std::to_string(i), "P2_Deposit", "{}", "10"); + log.append(entry); + } + log.flush(); + REQUIRE(log.entries().size() == 3); + sizeBefore = std::filesystem::file_size(tmp.path); + REQUIRE(sizeBefore > 0); + } + + // Write-only: the scan's open fails while resize_file would still succeed, + // which is precisely the combination that made the truncation possible. + std::filesystem::permissions(tmp.path, std::filesystem::perms::owner_write); + morph::core::FileIoOps ioOps; + ioOps.canOpenForRead = [](const std::filesystem::path&) { return true; }; // force the probe/open window + + // The constructor must fail loudly rather than hand back a log whose dedup + // set is silently empty. + REQUIRE_THROWS_AS((FileActionLog{tmp.path, ioOps}), std::runtime_error); + + std::filesystem::permissions(tmp.path, std::filesystem::perms::owner_all); + // The whole point: every byte still there. + REQUIRE(std::filesystem::file_size(tmp.path) == sizeBefore); + FileActionLog reopened{tmp.path}; + REQUIRE(reopened.entries().size() == 3); +} +#endif // !defined(_WIN32) diff --git a/tests/test_file_offline_queue.cpp b/tests/test_file_offline_queue.cpp index f9b1edd6..38805b99 100644 --- a/tests/test_file_offline_queue.cpp +++ b/tests/test_file_offline_queue.cpp @@ -13,6 +13,9 @@ #include #include +#if !defined(_WIN32) +#include // geteuid, for the permission-based fault-injection case below +#endif #include "offline_queue_conformance.hpp" namespace { @@ -692,3 +695,45 @@ TEST_CASE("morph::offline::FileOfflineQueue: the idempotency-key contract surviv [&path] { return std::make_unique(path); }); std::filesystem::remove(path); } + +// ── An unreadable queue file must not be committed away (morph#494) ── +// +// load() read with an unchecked ifstream and the constructor calls compact() +// straight after, so a failed read produced an empty `_items` that compact() +// then wrote over the real file. Measured before the fix: 3 pending items (306 +// bytes) went to 0, with the constructor returning normally and the queue +// reporting an empty backlog. Unlike FileActionLog's sibling defect this needed +// no fault injection at all -- load() bypasses the FileIoOps seam entirely. +// +// POSIX-only and non-root, for the same reasons as the FileActionLog case. +#if !defined(_WIN32) +TEST_CASE("FileOfflineQueue: an unreadable queue file is not silently compacted away", + "[offline][file][fault-injection]") { + if (::geteuid() == 0) { + SUCCEED("running as root: permission bits are not enforced"); + return; + } + const auto path = std::filesystem::temp_directory_path() / "morph_test_offline_unreadable.ndjson"; + std::filesystem::remove(path); + std::uintmax_t sizeBefore = 0; + { + morph::offline::FileOfflineQueue queue{path}; + (void)queue.enqueue(R"({"op":"transfer","amount":100})"); + (void)queue.enqueue(R"({"op":"transfer","amount":250})"); + (void)queue.enqueue(R"({"op":"transfer","amount":375})"); + REQUIRE(queue.drain().size() == 3); + sizeBefore = std::filesystem::file_size(path); + REQUIRE(sizeBefore > 0); + } + + std::filesystem::permissions(path, std::filesystem::perms::owner_write); + // Must throw rather than hand back a queue that reports no pending work. + REQUIRE_THROWS_AS(morph::offline::FileOfflineQueue{path}, std::runtime_error); + + std::filesystem::permissions(path, std::filesystem::perms::owner_all); + REQUIRE(std::filesystem::file_size(path) == sizeBefore); + morph::offline::FileOfflineQueue reopened{path}; + REQUIRE(reopened.drain().size() == 3); + std::filesystem::remove(path); +} +#endif // !defined(_WIN32) From 2b14c326d5b8ad3b4200429e0c9870ce12eca0f5 Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Wed, 9 Sep 2026 22:12:00 +0200 Subject: [PATCH 02/12] core, util, render: four defects whose comments already described the fix (#496, #497, #499, #501) **#496 -- `formatRationalDecimal` negated `INT64_MIN` in `int64_t`.** The comment above it said "widen before taking the absolute value so the magnitude is always representable"; the expression nested the casts so the unary `-` ran on `int64_t`, and the widening cast happened after the UB rather than before it. Confirmed by UBSan at quantity.hpp:101. `INT64_MIN` reaches this function because the whole-integer `Rational{value, DecimalPlaces{n}}` constructor does not canonicalise, so the clamp in `canonicalise()` never runs on that path, and `numerator` is public. Now uses `detail::absU64`, the shared helper that does exactly what the comment claimed, already reachable from this header. **#497 -- a sign after the decimal separator was accepted.** `sawAnyOutput` was set only at the bottom of the loop and the decimal-separator branch `continue`d past it, so after a separator the sign guard still believed nothing had been emitted: `normalizeLocaleNumber(",-5", ",", ".")` returned ".-5". The guard's own comment states the intent exactly ("a stripped group separator before the sign would otherwise make an injected sign look leading"). The QML mirror in DynamicForm.qml, documented as mirroring this function, has always rejected it -- so the two control edges disagreed on the same input, which is the more serious half. Deliberately *not* narrowed further: the header also promised output matching `-?[0-9]+(\.[0-9]+)?`, which "`.`", "`.5`" and "`5.`" do not satisfy, and I first tightened the final check to enforce it. That was wrong -- the QML mirror accepts all three, so tightening one edge alone puts them back out of step, and rejecting ".5" is a UX regression on ordinary input. The shape is documented instead; narrowing both edges together is a separate call for the maintainer. **#499 -- `CallbackScope::reset()` raced every other member.** `_state` was a plain `shared_ptr` written by `reset()` and read by `token()`, `guard()`, `requestStop()` and `stopRequested()`; the control block's atomic refcount protects the pointee, not the handle. Made `std::atomic>` rather than narrowing the documented contract, because the guarantee is deliberate: the class documents every member as concurrently safe, and `reset()`'s own doc turns on a token holder "that pinned it while racing this call". Measured: 18 ThreadSanitizer reports before, 0 after. The two `_state != nullptr` guards went with it -- provably unreachable (sole constructor make_shared's, class non-copyable and non-movable, reset() always assigns a fresh value), so two permanently-surviving mutants under the gate morph#408 just brought online. **#501 -- `MainThreadExecutor` let a non-`std::exception` escape the pump.** `ThreadPoolExecutor::loop` has caught `...` all along; this one did not, and three doc claims on the same class depended on the missing arm -- `runOnce()` promises to return `true` "whether or not that task threw" and did not return at all. Added the mirroring catch-all. Regression tests for all four. #497 and #501 fail without their fix directly; #496 and #499 are sanitizer-dependent by nature -- value-stable UB and a data race -- and are caught by the existing `clang-ubsan` and `clang-tsan` CI legs, verified by hand both ways rather than assumed. Full suite: 22011 assertions, no regressions. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01SxkvtHvan2fWKDDaBpqkgv --- include/morph/core/callback_scope.hpp | 29 +++++++++++++++------ include/morph/core/executor.hpp | 8 ++++++ include/morph/render/locale_format.hpp | 18 +++++++++++-- include/morph/util/quantity.hpp | 11 ++++---- tests/test_callback_scope.cpp | 36 ++++++++++++++++++++++++++ tests/test_executor.cpp | 20 ++++++++++++++ tests/test_quantity.cpp | 18 +++++++++++++ tests/test_render_locale_format.cpp | 34 ++++++++++++++++++++++++ 8 files changed, 159 insertions(+), 15 deletions(-) diff --git a/include/morph/core/callback_scope.hpp b/include/morph/core/callback_scope.hpp index f2c0d315..2e7e8406 100644 --- a/include/morph/core/callback_scope.hpp +++ b/include/morph/core/callback_scope.hpp @@ -221,10 +221,14 @@ class CallbackScope { /// Idempotent and safe from any thread. The owner remains alive, so tokens /// report `CallbackStatus::Stopped` rather than `Expired`. Undone only by /// `reset()`, which starts a new generation. - void requestStop() noexcept { - if (_state != nullptr) { - _state->stopped.store(true, std::memory_order_release); - } + void requestStop() const noexcept { + // One load, then act on that generation: re-reading `_state` between + // the null test and the store would be a second, possibly different + // generation. It is never null (the sole constructor make_shared's it, + // the class is non-copyable and non-movable, and `reset()` always + // assigns a fresh value), so the former `!= nullptr` guard was an + // unreachable branch and is gone -- morph#499. + _state.load(std::memory_order_acquire)->stopped.store(true, std::memory_order_release); } /// @brief Retires every token issued so far and starts a fresh, live generation. @@ -237,18 +241,20 @@ class CallbackScope { void reset() { auto fresh = std::make_shared(); requestStop(); - _state = std::move(fresh); + _state.store(std::move(fresh), std::memory_order_release); } /// @brief Whether this generation has been stopped. /// @return `true` after `requestStop()`, until the next `reset()`. [[nodiscard]] bool stopRequested() const noexcept { - return _state != nullptr && _state->stopped.load(std::memory_order_acquire); + return _state.load(std::memory_order_acquire)->stopped.load(std::memory_order_acquire); } /// @brief Issues a weak token for the current generation. /// @return A `CallbackToken` observing this scope; keeps nothing alive. - [[nodiscard]] CallbackToken token() const noexcept { return CallbackToken{_state}; } + [[nodiscard]] CallbackToken token() const noexcept { + return CallbackToken{_state.load(std::memory_order_acquire)}; + } /// @brief Wraps @p fn so it runs only while this scope is alive and un-stopped. /// @@ -267,7 +273,14 @@ class CallbackScope { } private: - std::shared_ptr _state; + /// Atomic because the class documents *every* member as safe to call + /// concurrently, and `reset()` writes this while `token()`, `guard()`, + /// `requestStop()` and `stopRequested()` read it. A plain `shared_ptr` made + /// that a data race on the pointer object itself -- the control block's + /// atomic refcount protects the pointee, not the handle (morph#499). The + /// guarantee is deliberate, not incidental: `reset()`'s own doc turns on a + /// token holder "that pinned it while racing this call". + std::atomic> _state; }; } // namespace morph::async diff --git a/include/morph/core/executor.hpp b/include/morph/core/executor.hpp index 7d82fbf1..f505a7be 100644 --- a/include/morph/core/executor.hpp +++ b/include/morph/core/executor.hpp @@ -213,6 +213,14 @@ class MainThreadExecutor : public IExecutor { task(); } catch (const std::exception& exc) { ::morph::log::logError("[main-thread] callback threw: " + std::string{exc.what()}); + } catch (...) { + // Mirrors ThreadPoolExecutor::loop's own catch-all. Without it a + // non-`std::exception` throw unwinds out of runFor()/runOnce()/ + // drain(), each of which documents that a throwing task is logged + // and the pump continues -- runOnce() promises to return `true` + // "whether or not that task threw", and would not return at all. + // morph#501. + ::morph::log::logError("[main-thread] callback threw unknown exception"); } } diff --git a/include/morph/render/locale_format.hpp b/include/morph/render/locale_format.hpp index e3dc0d97..3e4a1233 100644 --- a/include/morph/render/locale_format.hpp +++ b/include/morph/render/locale_format.hpp @@ -37,8 +37,17 @@ namespace morph::render { /// occurrence of @p decimalSeparator with `.`. Passing `decimalSeparator == /// "."` and an empty @p groupSeparator is the identity transform (the /// locale-free behavior). Malformed input (a second decimal separator, a -/// sign anywhere but the leading position, or any character that is not a -/// digit) yields `std::nullopt` rather than a best-effort guess. +/// sign anywhere but the leading position of the *output*, or any character that +/// is not a digit) yields `std::nullopt` rather than a best-effort guess. The +/// decimal point counts as output, so a sign placed straight after the separator +/// ("`,-5`" in a de-DE locale) is rejected -- matching the QML mirror in +/// `src/qt/forms/qml/DynamicForm.qml`, which has always rejected it (morph#497). +/// +/// The result is `.`-decimal and digit-only, but is **not** narrowed to +/// `-?[0-9]+(\.[0-9]+)?`: a bare "`.`", a leading "`.5`" and a trailing "`5.`" +/// are passed through, exactly as that same QML mirror passes them. Tightening +/// one side alone would put the two control edges back out of step, so the shape +/// is documented here rather than changed. /// /// Separators are matched as whole strings, so a multi-byte one (e.g. U+202F) /// works; matching them before the per-byte digit scan is what keeps their @@ -69,6 +78,11 @@ namespace morph::render { } sawDecimal = true; canonical += '.'; + // The decimal point *is* output: without this, the `chr == '-'` + // guard below still believes nothing has been emitted, and a sign + // placed straight after the separator ("`,-5`" in a de-DE locale) + // is accepted as if it were leading. morph#497. + sawAnyOutput = true; i += decimalSeparator.size(); continue; } diff --git a/include/morph/util/quantity.hpp b/include/morph/util/quantity.hpp index 58fd4cb7..83e41c4d 100644 --- a/include/morph/util/quantity.hpp +++ b/include/morph/util/quantity.hpp @@ -95,11 +95,12 @@ namespace detail { // Work on magnitudes; the sign is reattached at the end. The canonical // invariant guarantees denominator > 0, so only the numerator carries sign. bool const negative = value.numerator < 0; - // Negating INT64_MIN would overflow int64; widen before taking the absolute - // value so the magnitude is always representable. - auto const num = - negative ? static_cast(-static_cast(static_cast(value.numerator))) - : static_cast(value.numerator); + // Negate in unsigned arithmetic: `-INT64_MIN` is undefined as a signed + // operation, and INT64_MIN reaches here through the whole-integer + // `Rational{value, DecimalPlaces{n}}` constructor, which does not + // canonicalise (and `numerator` is public). `absU64` is the shared helper + // that gets this right -- see morph#496. + auto const num = ::morph::math::detail::absU64(value.numerator); auto const den = static_cast(value.denominator); auto const places = static_cast(value.decimalPlaces.value); diff --git a/tests/test_callback_scope.cpp b/tests/test_callback_scope.cpp index 04f7a80a..affa2e28 100644 --- a/tests/test_callback_scope.cpp +++ b/tests/test_callback_scope.cpp @@ -573,3 +573,39 @@ TEST_CASE("CallbackScope: destroying the scope under a concurrent dispatch loop REQUIRE(hits->load() == settled); } } + +// ── morph#499: reset() races every other member ── +// +// `_state` was a plain shared_ptr written by reset() and read by token(), +// guard(), requestStop() and stopRequested(). Concurrent read/write of the same +// shared_ptr object is a data race -- the control block's atomic refcount +// protects the pointee, not the handle. The class documents *every* member as +// concurrently safe, and reset()'s own doc turns on a token holder "that pinned +// it while racing this call", so the guarantee is deliberate. Now atomic. +// +// This case exists to be run under ThreadSanitizer; it is deliberately +// assertion-light, because what it proves is the absence of a report. +TEST_CASE("CallbackScope: reset() concurrent with token()/stopRequested() is race-free", + "[callback_scope][thread][morph499]") { + morph::async::CallbackScope scope; + std::atomic stop{false}; + std::atomic observed{0}; + + std::thread reader{[&] { + while (!stop.load(std::memory_order_acquire)) { + auto tok = scope.token(); + (void)tok.active(); + (void)scope.stopRequested(); + observed.fetch_add(1, std::memory_order_relaxed); + } + }}; + for (int i = 0; i < 2000; ++i) { + scope.reset(); + } + stop.store(true, std::memory_order_release); + reader.join(); + + REQUIRE(observed.load() > 0); + // A fresh generation is live after the last reset(). + REQUIRE(scope.token().active()); +} diff --git a/tests/test_executor.cpp b/tests/test_executor.cpp index a08dc2d8..03cc350d 100644 --- a/tests/test_executor.cpp +++ b/tests/test_executor.cpp @@ -129,3 +129,23 @@ TEST_CASE("morph::exec::MainThreadExecutor drain runs a bounded chain of tasks t exec.drain(); REQUIRE(count.load() == chainLength); } + +// ── morph#501: a non-std::exception must not escape the main-thread pump ── +// +// runTask() caught only `const std::exception&`, while ThreadPoolExecutor::loop +// has caught `...` as well all along. Three doc claims on this class depended on +// the missing arm: runFor "execution continues with the next task", runOnce +// "returns `true` whether or not that task threw" (it did not return at all), +// and drain, which left the queue undrained. +TEST_CASE("MainThreadExecutor: a task throwing a non-std::exception is contained", "[executor][morph501]") { + morph::exec::MainThreadExecutor exec; + bool ranAfter = false; + exec.post([] { throw 42; }); // not derived from std::exception + exec.post([&ranAfter] { ranAfter = true; }); + + // Before the fix this call propagated the `int` instead of returning. + REQUIRE(exec.runOnce()); + // And the queue must still be pumpable afterwards. + REQUIRE(exec.runOnce()); + REQUIRE(ranAfter); +} diff --git a/tests/test_quantity.cpp b/tests/test_quantity.cpp index 56f0ccd4..e84c52b5 100644 --- a/tests/test_quantity.cpp +++ b/tests/test_quantity.cpp @@ -4,6 +4,7 @@ #include #include #include +#include #include #include #include @@ -652,3 +653,20 @@ TEST_CASE("NamedQuantity slices to a plain Quantity", "[quantity]") { Tariff blank; CHECK_FALSE(blank.hasValue()); } + +// ── morph#496: rendering an un-canonicalised INT64_MIN numerator ── +// +// formatRationalDecimal negated the numerator with signed arithmetic, which is +// UB for INT64_MIN -- confirmed by UBSan at quantity.hpp:101 before the fix. +// INT64_MIN reaches it because the whole-integer `Rational{value, DecimalPlaces}` +// constructor does not canonicalise (and `numerator` is a public member), so the +// clamp in canonicalise() never runs on this path. +TEST_CASE("formatRationalDecimal: an un-canonicalised INT64_MIN numerator renders exactly", + "[quantity][rational][morph496]") { + morph::math::Rational const value{std::numeric_limits::min(), morph::math::DecimalPlaces{0}}; + // Precondition: this constructor really does keep the trap value. + REQUIRE(value.numerator == std::numeric_limits::min()); + // Under -fsanitize=undefined this line was the UB report; the magnitude must + // survive the unsigned negation intact rather than wrapping. + REQUIRE(morph::units::detail::formatRationalDecimal(value) == "-9223372036854775808"); +} diff --git a/tests/test_render_locale_format.cpp b/tests/test_render_locale_format.cpp index ba37e81c..cbf1898a 100644 --- a/tests/test_render_locale_format.cpp +++ b/tests/test_render_locale_format.cpp @@ -145,3 +145,37 @@ TEST_CASE("render::locale_format round-trips through a multi-byte separator", "[ auto const display = formatCanonicalNumber("1050.25", ",", kNarrowNbsp); CHECK(normalizeLocaleNumber(display, ",", kNarrowNbsp) == "1050.25"); } + +// ── morph#497: a sign after the decimal separator is not "leading" ── +// +// `sawAnyOutput` was only set at the bottom of the loop, and the +// decimal-separator branch `continue`d past it -- so after a separator the sign +// guard still believed nothing had been emitted and accepted an injected sign. +// The QML mirror (src/qt/forms/qml/DynamicForm.qml, documented as mirroring +// this function) always rejected these, so the two control edges disagreed. +TEST_CASE("normalizeLocaleNumber: a sign after the decimal separator is rejected", + "[render][locale][morph497]") { + // de-DE: comma decimal, dot grouping -- the reported shape. + REQUIRE_FALSE(morph::render::normalizeLocaleNumber(",-5", ",", ".").has_value()); + // en-US equivalent. + REQUIRE_FALSE(morph::render::normalizeLocaleNumber(".-5", ".", ",").has_value()); + // With a group separator stripped first, which is the case the guard's own + // comment is about. + REQUIRE_FALSE(morph::render::normalizeLocaleNumber("1.,-5", ",", ".").has_value()); + + // Control: the guard already worked once a digit had been emitted, and must + // keep working. + REQUIRE_FALSE(morph::render::normalizeLocaleNumber("1-2", ".", ",").has_value()); + // Control: a genuinely leading sign still parses. + REQUIRE(morph::render::normalizeLocaleNumber("-1,5", ",", ".") == "-1.5"); +} + +TEST_CASE("normalizeLocaleNumber: the loose shapes stay accepted, in step with the QML mirror", + "[render][locale][morph497]") { + // Deliberately NOT narrowed to `-?[0-9]+(\.[0-9]+)?`: DynamicForm.qml's + // normalizeLocaleNumber accepts all three, and tightening one edge alone + // would put them back out of step. Documented on the function. + REQUIRE(morph::render::normalizeLocaleNumber(".5", ".", ",") == ".5"); + REQUIRE(morph::render::normalizeLocaleNumber("5.", ".", ",") == "5."); + REQUIRE(morph::render::normalizeLocaleNumber(".", ".", ",") == "."); +} From 9dbe92f0988653dd4dda9202d6bdbbdccc2d49dd Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Wed, 9 Sep 2026 22:17:27 +0200 Subject: [PATCH 03/12] qt, session: stamp the session on the async control paths, and say where authorize is called (#495, #500) **#495 -- the three async control builders never stamped `env.session`.** `registerModelSharedAsync`, `attachModelAsync` and `assignPrimaryAsync` each built their envelope with `makeRegisterShared`/`makeAttach`/`makeAssign` and encoded it with no session assignment, while all three synchronous counterparts (`registerModelShared`, `attachModel`, `assignPrimary`) stamped it immediately before encode, as does every equivalent site in `net::SocketBackend`. So it was a Qt-side omission, not a protocol choice. `RemoteServer` authenticates and authorizes from `env.session` -- `stampVerifiedPrincipal`, and the register/attach/assign authorization sites -- so those three verbs arrived with a default-constructed session and could not be authenticated at all. The exposure is the async path, which is opt-in behind `asyncRegistrationEnabled` but is also the *only* path a WASM main thread can use, since the synchronous ones block on a nested QEventLoop. The regression test records what the **server** saw, not that the call succeeded -- the latter was already true before the fix. It asserts on `ctx.token` rather than `ctx.principal`, because `stampVerifiedPrincipal` deliberately clears a client-asserted principal that `authenticate()` cannot vouch for, so principal is "" either way and proves nothing. Measured: "" before, "tok-495" after. While in `assignPrimaryAsync`, also moved `wire::encode` above the `_pendingAssigns` insertion. `registerModelAsync` states that invariant at length and the other two async hooks point back at it and follow it; this one did the opposite, so a throwing encode would park its onRegistered/onError in the pending map forever with no message ever sent. **#500 -- `IAuthorizer::authorize`'s contract did not describe its own call sites.** It said "Called once per `execute` envelope". It is also called for `instances` and `schemas`, both passing an **empty** `actionType`. The server is right and deliberate -- the comments at those sites explain that gating the two read channels lets a deployer refuse enumeration or schema disclosure without refusing use, and `schemas` discloses field names, bounds, rules and the payload fingerprint of every action. The stale artefact was the interface doc, which is why this is a contract correction and not a behaviour change. An implementor matching on `actionType` would hit its default arm on exactly those two disclosure verbs, and whether that fails open or closed was being decided without being told the case exists. Now stated as a table on the interface, on `@param actionType`, on `SigningAuthorizer::Policy` (same gap), and in docs/spec/session/session.md. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01SxkvtHvan2fWKDDaBpqkgv --- docs/spec/session/session.md | 2 +- include/morph/session/session.hpp | 26 +++++++-- include/morph/session/session_auth.hpp | 3 ++ src/qt/qt_websocket_backend.cpp | 22 ++++++-- tests/qt/test_qt_websocket.cpp | 73 ++++++++++++++++++++++++++ 5 files changed, 118 insertions(+), 8 deletions(-) diff --git a/docs/spec/session/session.md b/docs/spec/session/session.md index 1e6d96d4..84450a20 100644 --- a/docs/spec/session/session.md +++ b/docs/spec/session/session.md @@ -413,7 +413,7 @@ above and [bridge.md](../core/bridge.md). | Member | Signature | Notes | |---|---|---| | `~IAuthorizer()` | `virtual ~IAuthorizer() = default` | Virtual destructor for polymorphic use. | -| `authorize` | `[[nodiscard]] virtual bool authorize(const Context&, std::string_view modelType, std::string_view actionType) const = 0` | Returns `true` to allow dispatch, `false` to reject. Called per `execute` envelope. Sees only type ids. | +| `authorize` | `[[nodiscard]] virtual bool authorize(const Context&, std::string_view modelType, std::string_view actionType) const = 0` | Returns `true` to allow dispatch, `false` to reject. Called on `execute`, `instances` and `schemas` envelopes -- the latter two pass an **empty** `actionType`, so an implementation matching on it must handle that case. Sees only type ids. | | `authenticate` | `[[nodiscard]] virtual std::optional authenticate(const Context&) const` | Optional. Default returns `nullopt`. Called after `authorize` succeeds; a returned value overwrites `Context::principal` (making it authoritative), and `nullopt` clears `Context::principal` so an unverified claim is never presented to the model. Also called at `register` time to record the instance's owner principal. | | `authorizeInstance` | `[[nodiscard]] virtual bool authorizeInstance(const Context&, std::string_view modelType, std::string_view actionType, std::uint64_t modelId, std::string_view ownerPrincipal) const` | Optional. Default returns `true` (allow). Consulted per `execute` and per `deregister` with the target instance id and its recorded owner. Override to enforce per-instance ownership; `modelType`/`actionType` are empty for `deregister`. | | `authorizeRegister` | `[[nodiscard]] virtual bool authorizeRegister(const Context&, std::string_view modelType) const` | Optional. Default returns `true` (allow). Consulted on every `register`, after authentication, before the instance is constructed. Override to bound *who may create* an instance. | diff --git a/include/morph/session/session.hpp b/include/morph/session/session.hpp index c6b6df63..016f6cc1 100644 --- a/include/morph/session/session.hpp +++ b/include/morph/session/session.hpp @@ -93,9 +93,25 @@ struct Principal { /// @brief Authorizes incoming actions on a `RemoteServer`. /// -/// Called once per `execute` envelope, before the action is dispatched. A `false` -/// return causes the server to reply with `err|unauthorized` (the client surfaces -/// the error through the `.onError(...)` callback). +/// Called before dispatch on **three** envelope kinds, not only `execute`. A +/// `false` return causes the server to reply with `err|unauthorized` (the client +/// surfaces the error through the `.onError(...)` callback): +/// +/// | Envelope | `modelType` | `actionType` | +/// |---|---|---| +/// | `execute` | `env.modelType` | `env.actionType` | +/// | `instances` | `env.typeId` | **empty** | +/// | `schemas` | `env.typeId` | **empty** | +/// +/// The two read channels are gated deliberately, so a deployer can refuse +/// enumeration or schema disclosure without refusing use -- `schemas` returns +/// field names, bounds, rules and the payload fingerprint of every action, so it +/// must not be reachable by a caller the server would not let execute. +/// +/// **An implementation that switches or matches on `actionType` must handle the +/// empty case explicitly**, or it will hit its default arm on exactly those two +/// disclosure verbs. Whether that fails open or closed is the implementation's +/// choice, but it has to be a choice (morph#500). /// /// Default implementation supplied by the framework is `AllowAllAuthorizer`. Real /// deployments install a custom subclass that checks principal claims, action @@ -108,7 +124,9 @@ struct IAuthorizer { /// /// @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. + /// @param actionType String id of the action being invoked, or **empty** for + /// the `instances` and `schemas` envelopes -- see the + /// table on this interface's own doc comment. /// @return `true` to allow dispatch, `false` to reject with `err|unauthorized`. [[nodiscard]] virtual bool authorize(const Context& ctx, // NOLINTNEXTLINE(bugprone-easily-swappable-parameters) diff --git a/include/morph/session/session_auth.hpp b/include/morph/session/session_auth.hpp index b077e8b0..c6c1cda5 100644 --- a/include/morph/session/session_auth.hpp +++ b/include/morph/session/session_auth.hpp @@ -521,6 +521,9 @@ class SigningAuthorizer : public IAuthorizer { /// @brief Optional per-request policy over verified claims. /// /// Receives the verified token plus the target ids; return `false` to deny. + /// The action id is **empty** for the `instances` and `schemas` envelopes -- + /// see `IAuthorizer::authorize`'s own doc comment for why, and handle that + /// case explicitly rather than letting it reach a default arm (morph#500). /// The default (empty) admits any validly-signed, unexpired token. using Policy = std::function; diff --git a/src/qt/qt_websocket_backend.cpp b/src/qt/qt_websocket_backend.cpp index a27b6f43..0eab82ad 100644 --- a/src/qt/qt_websocket_backend.cpp +++ b/src/qt/qt_websocket_backend.cpp @@ -242,6 +242,11 @@ bool QtWebSocketBackend::registerModelSharedAsync( auto env = ::morph::wire::makeRegisterShared(typeId, std::string{identity.primary}, std::string{identity.contextKey}); env.callId = callId; + // Same stamp the synchronous registerModelShared applies: RemoteServer + // authenticates and authorizes from env.session, so omitting it here reached + // the server as an unauthenticated principal on the async (WASM) path only. + // morph#495. + env.session = _session; // See registerModelAsync's identical comment: encoded before the map // insertion, so a throwing encode() cannot orphan a pending entry. auto const encoded = QString::fromStdString(::morph::wire::encode(env)); @@ -279,6 +284,8 @@ bool QtWebSocketBackend::attachModelAsync( auto env = ::morph::wire::makeAttach(typeId, std::string{identity.primary}, current.v, std::string{identity.contextKey}); env.callId = callId; + // See registerModelSharedAsync above: stamped for the same reason (morph#495). + env.session = _session; // See registerModelAsync's identical comment: encoded before the map // insertion, so a throwing encode() cannot orphan a pending entry. auto const encoded = QString::fromStdString(::morph::wire::encode(env)); @@ -371,13 +378,22 @@ bool QtWebSocketBackend::assignPrimaryAsync(::morph::exec::detail::ModelId mid, return true; } uint64_t const callId = ++_nextCallId; + auto env = ::morph::wire::makeAssign(typeId, std::string{primary}, mid.v); + env.callId = callId; + // Same stamp the synchronous assignPrimary applies -- RemoteServer authorizes + // from env.session, so an unstamped assign reached the server as an + // unauthenticated principal on the async (WASM) path only (morph#495). + env.session = _session; + // Encoded before the map insertion below, the invariant registerModelAsync + // states and the other two async hooks already follow: wire::encode() can + // throw, and a throw after inserting would park this callId's + // onRegistered/onError in _pendingAssigns forever with no message sent. + auto const encoded = QString::fromStdString(::morph::wire::encode(env)); { std::scoped_lock const lock{_pendingMtx}; _pendingAssigns[callId] = PendingAssign{std::move(onRegistered), std::move(onError)}; } - auto env = ::morph::wire::makeAssign(typeId, std::string{primary}, mid.v); - env.callId = callId; - _socket.sendTextMessage(QString::fromStdString(::morph::wire::encode(env))); + _socket.sendTextMessage(encoded); return true; } diff --git a/tests/qt/test_qt_websocket.cpp b/tests/qt/test_qt_websocket.cpp index 593bfe40..f8573618 100644 --- a/tests/qt/test_qt_websocket.cpp +++ b/tests/qt/test_qt_websocket.cpp @@ -2313,3 +2313,76 @@ int main(int argc, char* argv[]) { QCoreApplication::processEvents(QEventLoop::AllEvents); return result; } + +// ── morph#495: the async control paths must stamp the session too ── +// +// registerModelSharedAsync, attachModelAsync and assignPrimaryAsync each built +// their envelope and encoded it with no `env.session = _session`, while all +// three synchronous counterparts stamped it. RemoteServer authenticates and +// authorizes from env.session (remote.hpp: stampVerifiedPrincipal, and the +// register/attach/assign authorization sites), so a client using the async path +// -- which is the WASM path, and the only one a WASM main thread can use -- +// reached an authorizing server as an unauthenticated principal. +// +// A test asserting only "the async call succeeds" would have passed before the +// fix, so this records what the *server* saw. +namespace { +struct RecordingAuthorizer : morph::session::IAuthorizer { + mutable std::mutex mtx; + mutable std::vector registerTokens; + + [[nodiscard]] bool authorize(const morph::session::Context&, std::string_view, std::string_view) const override { + return true; + } + [[nodiscard]] bool authorizeRegister(const morph::session::Context& ctx, std::string_view) const override { + // `token`, not `principal`: stampVerifiedPrincipal clears the + // client-asserted principal whenever authenticate() cannot vouch for it + // (this authorizer does not override authenticate), so principal is "" + // either way and would prove nothing. The token rides the same + // `env.session` and is not rewritten, so it is the field that shows + // whether the envelope carried a session at all. + std::scoped_lock const lock{mtx}; + registerTokens.push_back(ctx.token); + return true; + } +}; +} // namespace + +TEST_CASE("morph::qt::QtWebSocketBackend: the async control envelopes carry the session", + "[qt][ws][morph495][security]") { + ensureApp(); + morph::exec::ThreadPoolExecutor serverPool{2}; + auto authorizer = std::make_shared(); + auto server = std::make_shared( + serverPool, authorizer, morph::model::detail::defaultDispatcher(), morph::model::detail::defaultRegistry()); + morph::qt::QtWebSocketServer wsServer{*server, 0}; + REQUIRE(wsServer.listen()); + + QUrl url{QString("ws://127.0.0.1:%1").arg(wsServer.port())}; + morph::qt::QtWebSocketBackend backend{url, morph::model::detail::defaultDispatcher(), + morph::model::detail::defaultRegistry(), std::nullopt, + morph::qt::QtWebSocketBackend::Config{.asyncRegistrationEnabled = true}}; + REQUIRE(backend.waitForConnected()); + + morph::session::Context session; + session.principal = "alice"; + session.token = "tok-495"; + backend.setSession(session); + + std::atomic registered{0}; + std::string failure; + REQUIRE(backend.registerModelSharedAsync( + "WsEchoModel", nullptr, {.contextKey = "acct-495", .primary = "acct-495"}, + [&](morph::exec::detail::ModelId mid) { registered.store(mid.v); }, + [&](const std::string& message) { failure = message; })); + pumpUntil([&] { return registered.load() != 0U || !failure.empty(); }); + CHECK(failure.empty()); + REQUIRE(registered.load() != 0U); + + std::scoped_lock const lock{authorizer->mtx}; + REQUIRE_FALSE(authorizer->registerTokens.empty()); + // Before the fix this was "" -- registerModelSharedAsync built its envelope + // with no `env.session = _session`, so the server received a + // default-constructed session and could not authenticate the caller at all. + CHECK(authorizer->registerTokens.back() == "tok-495"); +} From 46fdf2366c82c5e3e83a6482706516b333837481 Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Wed, 9 Sep 2026 22:29:06 +0200 Subject: [PATCH 04/12] core: release the in-flight slot on an unwind, and settle #505 the other way (#502, #505) **#502 -- both in-flight counters leaked permanently on a throw.** `RemoteServer::dispatchExecute` reserves a slot in `_inFlightExecutes` and then runs a stretch of non-`noexcept` code before anything can decrement it: `emitMetric`, two `make_shared`, `TimeoutScheduler::schedule`, `awaitTurn`'s mutex, and `_strand.post`. The reservation's own comment ("needs no unwind on the early-return paths above") is true of *returns* and silent about *throws*. One throw left the counter permanently over-counted, and `drainedWithin()` predicates on it reaching zero -- so graceful shutdown could never succeed again for that server, and with `maxInFlightExecutes` set a slot was gone for good. Fixed with an RAII reservation that claims the same `finished` flag `complete` uses. Claiming rather than replying is the point: `dispatchMessage`'s catch is what replies on that path, so replying here too would break `handle()`'s reply-exactly-once contract -- and claiming the flag also makes an already-armed timeout a no-op, closing the second half of the bug, where a throw after the timer was scheduled produced a second `err "timeout"` for the same callId. `finished` moved above the reservation so one guard covers the whole window. `Bridge::executeVia` had the same shape: `_pendingCalls` incremented and the client deadline armed before an untried `backend->execute(...)`, which is genuinely throwing code (`serializeAction()` runs user `toJson` and glaze; `wire::encode` runs glaze). Now undone on unwind before the exception continues. **#505 -- resolved the opposite way to the obvious one, on evidence.** The issue offered two readings: the `_attachMtx` invariant holds and `registerHandlerImpl` breaks it, or the invariant is overstated. I implemented the first -- copy `contextKey` out under the lock -- and it failed "Bridge: an in-flight shared attach does not block unrelated handler registration" (tests/test_shared_instances.cpp), whose own comment says the dedicated attach mutex exists so `registerHandler()` "no longer contends for the same lock as a slow shared attach". Acquiring `_attachMtx` during registration, even briefly, reproduces exactly the regression that test was written to catch. So reading 1 is ruled out by test, and the unlocked read stays. What was missing was never the lock -- it was the statement of why the read is safe without one: the writers all operate on an already-registered binding, while this read happens *during* registration, which puts a requirement on the caller of the pre-built `registerHandler(binding)` overload (set `contextKey` first; do not mutate it concurrently with that call). Now stated at the read, on the `_attachMtx` member as an explicit carve-out from an otherwise absolute rule, and in docs/spec/core/bridge.md. Regression test for the `Bridge` half of #502 (1 before, 0 after). The `RemoteServer` half is structural: reaching it needs a throw from inside the reserved window, which no in-tree backend does on demand. Full suite: 22015 assertions, no regressions. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01SxkvtHvan2fWKDDaBpqkgv --- docs/spec/core/bridge.md | 16 ++++++++ include/morph/core/bridge.hpp | 62 +++++++++++++++++++++++++---- include/morph/core/remote.hpp | 50 ++++++++++++++++++++++- tests/test_bridge_pending_calls.cpp | 38 ++++++++++++++++++ 4 files changed, 157 insertions(+), 9 deletions(-) diff --git a/docs/spec/core/bridge.md b/docs/spec/core/bridge.md index 73759e82..edc8e346 100644 --- a/docs/spec/core/bridge.md +++ b/docs/spec/core/bridge.md @@ -756,6 +756,22 @@ It is the same rule `registerHandlerImpl` already follows for `_mtx`. See [shared_instances.md](shared_instances.md), "Async register-or-attach and attach". +**`registerHandlerImpl` reads `contextKey` without `_attachMtx`, on purpose.** +`HandlerBinding::primary`/`contextKey` are otherwise mutated and read only under +`_attachMtx`. Registration is the one carve-out, and it is forced: acquiring +`_attachMtx` there makes `registerHandler()` contend with a slow shared attach, +which is exactly the regression *"Bridge: an in-flight shared attach does not +block unrelated handler registration"* (`tests/test_shared_instances.cpp`) +exists to catch — taking the lock there reproduces that failure. + +The read is made safe by ordering rather than locking: every writer +(`attachHandler`, `ensureBound`, `assignHandlerPrimary`) operates on an +already-registered binding, while this read happens *during* registration. The +pre-built-binding `registerHandler(binding)` overload hands the caller the +binding first, so the requirement falls on the caller: **set `contextKey` before +calling `registerHandler()`, and do not mutate it concurrently with that call.** +Afterwards the ordinary `_attachMtx` rule applies. See morph#505. + The guarantee is unconditional, including for a backend that completes its `attachModelAsync`/`registerModelSharedAsync` callback **inline** — from inside the dispatch call itself, while the dispatching frame still holds `_attachMtx` diff --git a/include/morph/core/bridge.hpp b/include/morph/core/bridge.hpp index 98cf593f..c04546ce 100644 --- a/include/morph/core/bridge.hpp +++ b/include/morph/core/bridge.hpp @@ -612,12 +612,12 @@ class Bridge { } std::exception_ptr failure; { - // contextKey/primary are plain std::strings that the - // other attach/assign sites read under `_attachMtx`; - // publishing them without it would be a data race, not - // just a stale read. (`registerHandlerImpl`'s two reads - // are the exception and hold neither lock -- see - // morph#505.) + // contextKey/primary are plain std::strings that every + // other site reads under `_attachMtx`; publishing them + // without it would be a data race, not just a stale + // read. (`registerHandlerImpl`'s read during + // registration is the one documented carve-out -- see + // its own comment, and morph#505.) std::scoped_lock const guard{_attachMtx}; auto pinned = weakBackend.lock(); if (!pinned || pinned != loadBackend()) { @@ -1549,7 +1549,25 @@ class Bridge { std::scoped_lock const lock{_sessionMtx}; call.session = _defaultSession; } - auto anyCompletion = backend->execute(::morph::exec::detail::ModelId{raw}, std::move(call), cbExec); + // `backend->execute` is not `noexcept` and genuinely throws: for + // `QtWebSocketBackend` it runs `call.serializeAction()` (user `toJson` + // and glaze) and `wire::encode(env)`. A throw here escaped + // `BridgeHandler::execute` with `_pendingCalls` already incremented and + // the deadline already armed, permanently inflating `pendingCalls()` -- + // which the class documents as a quiescence gate -- and stranding the + // timer entry. Undo both, then let the exception continue to the caller + // (morph#502). + ::morph::async::Completion> anyCompletion = [&] { + try { + return backend->execute(::morph::exec::detail::ModelId{raw}, std::move(call), cbExec); + } catch (...) { + _pendingCalls->fetch_sub(1, std::memory_order_relaxed); + if (deadlineHandle && schedulerRef) { + schedulerRef->cancel(*deadlineHandle); + } + throw; + } + }(); anyCompletion .then([typedState, onResult = std::move(onResult), raw, deadlineHandle, schedulerRef, pendingCalls = _pendingCalls, subscriptions = _subscriptions, @@ -1758,6 +1776,26 @@ class Bridge { std::weak_ptr<::morph::backend::detail::IBackend> const weakBackend{backend}; std::weak_ptr const weakBinding{binding}; + + // `binding->contextKey` is read here without `_attachMtx`, and that is a + // deliberate carve-out from the "read only under `_attachMtx`" rule the + // member comment states -- not an oversight. Taking the lock here is + // *ruled out by test*: "Bridge: an in-flight shared attach does not + // block unrelated handler registration" + // (tests/test_shared_instances.cpp) exists precisely because + // `attachHandler` holds `_attachMtx` across a full backend round trip, + // and having registration contend for the same lock is the regression + // that test was written to catch. Measured: acquiring it here, even + // briefly, fails that case. + // + // What makes the unlocked read safe is ordering, not locking: the + // writers (`attachHandler`, `ensureBound`, `assignHandlerPrimary`) all + // operate on a binding that is already registered, and this runs during + // registration. The pre-built-binding `registerHandler()` overload hands + // the caller the binding first, so the requirement is on the caller: + // **set `contextKey` before calling `registerHandler()`, and do not + // mutate it concurrently with that call.** After registration returns, + // every access goes under `_attachMtx` as documented. morph#505. bool const started = backend->registerModelAsync( binding->typeId, binding->modelFactory, binding->contextKey, [this, weakBackend, weakBinding, lifetime = _lifetime](::morph::exec::detail::ModelId newId) { @@ -1932,7 +1970,15 @@ class Bridge { // reply-delivering thread that itself needed `_mtx` could deadlock // against it. `HandlerBinding::primary`/`contextKey` are therefore // mutated (and must be read) only under `_attachMtx` — never under `_mtx` - // alone. `switchBackend()` and the reconnect handler, which also touch + // alone. **One carve-out**, and it is load-bearing rather than an + // oversight: `registerHandlerImpl` reads `contextKey` unlocked *during + // registration*, because acquiring `_attachMtx` there would make + // `registerHandler()` contend with a slow shared attach — the exact + // regression "Bridge: an in-flight shared attach does not block unrelated + // handler registration" (tests/test_shared_instances.cpp) was written to + // catch, and which taking the lock there demonstrably reproduces. That read + // is ordered rather than locked; see its own comment for the requirement + // that places on a caller of the pre-built-binding overload (morph#505). `switchBackend()` and the reconnect handler, which also touch // them alongside `_handlers`, take both mutexes together via // `std::scoped_lock{_mtx, _attachMtx}` (deadlock-safe regardless of // acquisition order, by `std::scoped_lock`'s own guarantee). diff --git a/include/morph/core/remote.hpp b/include/morph/core/remote.hpp index 3fee060c..fb8e1408 100644 --- a/include/morph/core/remote.hpp +++ b/include/morph/core/remote.hpp @@ -1413,6 +1413,13 @@ class RemoteServer : public std::enable_shared_from_this { // pool's width. That is exactly the burst the limit exists to prevent. // Reserving here, at the last point before the slot is genuinely taken, // needs no unwind on the early-return paths above. + // Created before the reservation below, not after it, so `reservation` + // can claim the same completion slot `complete` uses. Everything between + // the increment and the strand post is non-`noexcept` -- emitMetric, two + // make_shared, TimeoutScheduler::schedule, awaitTurn's mutex, the post + // itself -- and a throw there used to leak the slot permanently. + auto finished = std::make_shared(); + std::size_t inFlightAfterInc = 0; if (limits.maxInFlightExecutes != 0) { std::size_t current = _inFlightExecutes.load(std::memory_order_relaxed); @@ -1434,6 +1441,44 @@ class RemoteServer : public std::enable_shared_from_this { ::morph::observe::detail::emitMetric(::morph::observe::Metric::executeInFlight, static_cast(inFlightAfterInc)); auto self = shared_from_this(); + + // Releases the in-flight slot if this frame leaves by exception before + // the dispatch is handed off. It claims `finished` rather than replying: + // dispatchMessage's catch is what replies on that path, and replying here + // too would break handle()'s reply-exactly-once contract. Claiming the + // flag also makes an already-armed timeout a no-op, so the caller cannot + // receive a second `err "timeout"` for the same callId afterwards. + // + // Without this, one throw left `_inFlightExecutes` permanently + // over-counted: `drainedWithin()` predicates on it reaching zero, so + // graceful shutdown could never succeed again for that server, and with + // `maxInFlightExecutes` set a slot was lost for good (morph#502). + struct InFlightReservation { + RemoteServer* server; + std::shared_ptr finished; + bool handedOff = false; + + InFlightReservation(RemoteServer* owner, std::shared_ptr flag) + : server{owner}, finished{std::move(flag)} {} + ~InFlightReservation() { + if (handedOff || finished->test_and_set()) { + return; + } + auto const remaining = server->_inFlightExecutes.fetch_sub(1, std::memory_order_relaxed) - 1; + ::morph::observe::detail::emitMetric(::morph::observe::Metric::executeInFlight, + static_cast(remaining)); + if (remaining == 0) { + std::scoped_lock const drainLock{server->_drainMtx}; + server->_drainCv.notify_all(); + } + } + InFlightReservation(const InFlightReservation&) = delete; + InFlightReservation& operator=(const InFlightReservation&) = delete; + InFlightReservation(InFlightReservation&&) = delete; + InFlightReservation& operator=(InFlightReservation&&) = delete; + }; + InFlightReservation reservation{this, finished}; + std::uint64_t const callId = env.callId; // `finished` fires the caller's `reply` exactly once — whichever of the @@ -1441,7 +1486,6 @@ class RemoteServer : public std::enable_shared_from_this { // decrements the in-flight counter exactly once, regardless of which // path won. This preserves handle()'s reply-exactly-once contract even // though two independent paths can now race to resolve the same call. - auto finished = std::make_shared(); auto replySlot = std::make_shared>(std::move(reply)); auto complete = [self, finished, replySlot](std::string msg) { if (!finished->test_and_set()) { @@ -1575,6 +1619,10 @@ class RemoteServer : public std::enable_shared_from_this { // delay a *different* execute's own pre-strand work for no ordering // benefit. ticketGuard.release(); + // The strand task now owns `complete`, so the in-flight slot is its + // responsibility rather than this frame's. Anything that throws past + // here is on a path where `complete` will still run. + reservation.handedOff = true; } /// @brief Returns the next opaque model id. diff --git a/tests/test_bridge_pending_calls.cpp b/tests/test_bridge_pending_calls.cpp index 169cde33..55e01dc1 100644 --- a/tests/test_bridge_pending_calls.cpp +++ b/tests/test_bridge_pending_calls.cpp @@ -161,3 +161,41 @@ TEST_CASE("Bridge: pendingCalls() does not increment for a synchronously-failed REQUIRE(errorFired); REQUIRE(bridge.pendingCalls() == 0); } + +// ── morph#502: a throwing backend->execute() must not leak the slot ── +// +// executeVia() incremented `_pendingCalls` and armed the client deadline before +// calling `backend->execute(...)`, which was not wrapped in a try. That call is +// genuinely throwing code -- for QtWebSocketBackend it runs `serializeAction()` +// (user `toJson` and glaze) and `wire::encode(env)`. A throw escaped +// BridgeHandler::execute() with the counter permanently inflated, which breaks +// the quiescence gate this whole file exists to cover: pendingCalls() could +// never return to 0 again for that bridge. +namespace { +/// Wraps LocalBackend and throws from execute(), the way an encode failure does. +struct ThrowingExecuteBackend : morph::backend::LocalBackend { + using morph::backend::LocalBackend::LocalBackend; + + morph::async::Completion> execute(morph::exec::detail::ModelId, + morph::backend::detail::ActionCall, + morph::exec::IExecutor*) override { + throw std::runtime_error("serialize/encode failed"); + } +}; +} // namespace + +TEST_CASE("Bridge: a throwing backend execute() leaves pendingCalls() at zero", "[bridge][pending-calls][morph502]") { + morph::exec::ThreadPoolExecutor pool{2}; + SyncExecutor cbExec; + morph::bridge::Bridge bridge{std::make_unique(pool)}; + morph::bridge::BridgeHandler handler{bridge, &cbExec}; + + REQUIRE(bridge.pendingCalls() == 0); + REQUIRE_THROWS_AS(handler.execute(PCFastAction{.value = 21}), std::runtime_error); + // Before the fix this was 1, permanently, for the life of the bridge. + CHECK(bridge.pendingCalls() == 0); + + // And the bridge is still usable as a quiescence gate afterwards. + REQUIRE_THROWS_AS(handler.execute(PCFastAction{.value = 1}), std::runtime_error); + CHECK(bridge.pendingCalls() == 0); +} From 30497b04511c8c30544ef95a1fb04d20611faeb7 Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Wed, 9 Sep 2026 22:34:21 +0200 Subject: [PATCH 05/12] net: reap finished connections, unblock the destructor, bound connect (#498, #506, #507) **#498 -- SocketServer leaked an fd and a thread handle per connection ever accepted.** `_clients`/`_clientThreads` were only ever pushed to in `acceptLoop` and cleared in `close()`; nothing removed a connection whose `clientLoop` had returned. The surviving `shared_ptr` kept the `ClientConnection` -- and its `TcpSocket` -- alive, so the fd stayed open, and every `std::thread` stayed joinable. The accumulation was per connection *ever accepted*, not per live one. `ClientConnection` now carries a `finished` flag set on every exit path, and `acceptLoop` reaps before taking on the next connection. Threads are moved out and joined after `_clientsMtx` is released -- `clientLoop`'s own teardown takes that mutex through `sendText`, so joining under it would deadlock -- and the flag is raised only after the scope guard has reclaimed the connection's models, which is why `FinishedFlag` is declared *before* the guard so it destructs last. (I had it the other way round first; the comment said "destroyed first" while C++ destroys in reverse declaration order.) The class doc and docs/spec/core/backend.md both say destruction leaves no dangling threads, and both are true -- at teardown, which is why the leak was invisible. The regression test therefore samples `/proc/self/fd` **while the server is still running**: 25 connect/disconnect cycles took the count from 11 to 38 before the fix and leave it flat after. A test that opened N connections and then destroyed the server would have passed either way. **#506 -- `~SocketBackend` took `_socketMtx` around `shutdownBoth()`.** `sendFrame` holds that lock across `_socket.sendAll()`, which loops on a blocking `::send` with no timeout, so a thread stalled against a peer that stopped reading holds it indefinitely -- and the destructor then waited on the one lock whose release requires the `shutdownBoth()` it could not reach. `SocketServer::close()` documents this exact trap and deliberately does not take its own write mutex; this is the same trap on the client side. `shutdownBoth()` is documented safe from any thread, so the lock was unnecessary as well as harmful. Not reproduced -- a stalled-peer teardown needs a peer that accepts and never reads. **#507 -- `TcpSocket::connect` was unbounded and signal-fragile.** The `::poll` sat inside the candidate loop, so `connectTimeout` was applied *per resolved address*; with `ai_family = AF_UNSPEC` several candidates are the norm ("localhost" is both ::1 and 127.0.0.1), making the worst case N x timeout while two doc comments state it as a single bound. Now one deadline for the whole call. The same poll treated `EINTR` as connect failure, so an ordinary delivered signal abandoned a candidate -- every other blocking syscall in the file already retries, and the accept loop's own comment argues for exactly that. Also clamps the `int` millisecond conversion instead of truncating a large timeout into a garbage (possibly infinite) value, and checks `getsockopt`'s return, which on failure left `soErr` at 0 and handed back a broken socket as connected. Full net suite: 983 assertions. Main suite: 22016 assertions. No regressions. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01SxkvtHvan2fWKDDaBpqkgv --- include/morph/net/detail/tcp_socket.hpp | 44 +++++++++++++-- include/morph/net/socket_backend.hpp | 18 ++++-- include/morph/net/socket_server.hpp | 56 +++++++++++++++++++ tests/net/test_socket_server.cpp | 74 +++++++++++++++++++++++++ 4 files changed, 183 insertions(+), 9 deletions(-) diff --git a/include/morph/net/detail/tcp_socket.hpp b/include/morph/net/detail/tcp_socket.hpp index ffa01243..f5691a5e 100644 --- a/include/morph/net/detail/tcp_socket.hpp +++ b/include/morph/net/detail/tcp_socket.hpp @@ -8,10 +8,12 @@ #include #include +#include #include #include #include #include +#include #include #include #include @@ -90,7 +92,9 @@ class TcpSocket { /// @brief Connects to `host:port`, failing after @p timeout. /// @param host Numeric or resolvable hostname. /// @param port TCP port. - /// @param timeout Maximum time to wait for the connection to establish. + /// @param timeout Maximum time to wait for the connection to establish, + /// across *all* resolved addresses -- one deadline for the + /// whole call, not one per candidate. /// @return A connected `TcpSocket`. /// @throws std::runtime_error on resolution failure, connect failure, or timeout. static TcpSocket connect(const std::string& host, std::uint16_t port, std::chrono::milliseconds timeout) { @@ -111,6 +115,14 @@ class TcpSocket { ~AddrInfoGuard() { ::freeaddrinfo(p); } } guard{resolved}; + // One deadline for the whole call, not one timeout per candidate. + // `ai_family = AF_UNSPEC` makes several candidates the norm ("localhost" + // resolves to both ::1 and 127.0.0.1), and polling `timeout` inside the + // loop meant the worst case was N x timeout -- while two doc comments + // (here and SocketBackend's destructor note) state it as a single bound. + // morph#507. + auto const deadline = std::chrono::steady_clock::now() + timeout; + for (addrinfo* rp = resolved; rp != nullptr; rp = rp->ai_next) { int fd = ::socket(rp->ai_family, rp->ai_socktype, rp->ai_protocol); if (fd < 0) { @@ -130,15 +142,39 @@ class TcpSocket { pollfd pfd{}; pfd.fd = fd; pfd.events = POLLOUT; - int const pollRc = ::poll(&pfd, 1, static_cast(timeout.count())); + // Retry on EINTR rather than treating a delivered signal as a + // connect failure. Every other blocking syscall in this file already + // does (accept, tryAccept, recvSome, sendAll), and the accept loop's + // own comment makes the case: "any signal the host happens to + // deliver (a profiler's timer, SIGCHLD, SIGWINCH)" must not tear the + // operation down. This poll was the one that did not honour it. + int pollRc = 0; + for (;;) { + auto const remaining = + std::chrono::duration_cast(deadline - std::chrono::steady_clock::now()); + if (remaining.count() <= 0) { + pollRc = 0; // deadline reached: treat as timeout + break; + } + // Clamped: poll takes int milliseconds, and a multi-week timeout + // would otherwise truncate to a garbage (possibly negative, + // i.e. infinite) value. + auto const waitMs = static_cast( + std::min(remaining.count(), std::numeric_limits::max())); + pollRc = ::poll(&pfd, 1, waitMs); + if (pollRc >= 0 || errno != EINTR) { + break; + } + } if (pollRc <= 0) { ::close(fd); continue; } int soErr = 0; socklen_t soErrLen = sizeof(soErr); - ::getsockopt(fd, SOL_SOCKET, SO_ERROR, &soErr, &soErrLen); - if (soErr != 0) { + // Checked: a failing getsockopt leaves `soErr` at 0, which would + // otherwise hand back a broken socket as successfully connected. + if (::getsockopt(fd, SOL_SOCKET, SO_ERROR, &soErr, &soErrLen) != 0 || soErr != 0) { ::close(fd); continue; } diff --git a/include/morph/net/socket_backend.hpp b/include/morph/net/socket_backend.hpp index 2ed7b188..550d815c 100644 --- a/include/morph/net/socket_backend.hpp +++ b/include/morph/net/socket_backend.hpp @@ -86,11 +86,19 @@ class SocketBackend : public ::morph::backend::detail::IBackend { /// implementation. See `docs/spec/core/backend.md`'s `morph::net` section. ~SocketBackend() override { _shuttingDown.store(true); - { - std::scoped_lock lock{_socketMtx}; - if (_socket.valid()) { - _socket.shutdownBoth(); - } + // Deliberately NOT under `_socketMtx` -- this is the same trap + // `SocketServer::close()` documents avoiding, reached from the client + // side. `sendFrame` holds `_socketMtx` across `_socket.sendAll()`, which + // loops on a blocking `::send` with no timeout, so a thread stalled + // against a peer that has stopped reading holds that lock indefinitely. + // Waiting for it here would block the destructor on exactly the + // condition that only `shutdownBoth()` can clear -- and `shutdownBoth()` + // is documented safe from any thread (detail/tcp_socket.hpp), which is + // what makes taking the lock unnecessary as well as harmful. The I/O + // thread's own Pong/Close echo in `drainFrames` reaches `sendFrame` too, + // so the stuck holder need not even be an application thread. morph#506. + if (_socket.valid()) { + _socket.shutdownBoth(); } _reconnectCv.notify_all(); if (_ioThread.joinable()) { diff --git a/include/morph/net/socket_server.hpp b/include/morph/net/socket_server.hpp index 26d6af14..9eeefdb5 100644 --- a/include/morph/net/socket_server.hpp +++ b/include/morph/net/socket_server.hpp @@ -201,6 +201,10 @@ class SocketServer { ::morph::backend::ConnectionId cid{0}; std::mutex writeMtx; std::atomic closed{false}; + /// Set by `clientLoop` as its last act, so `acceptLoop` can tell a + /// finished connection from a live one and reclaim both its fd and its + /// thread handle. See `reapFinishedClients` (morph#498). + std::atomic finished{false}; void sendText(const std::string& payload) { std::scoped_lock lock{writeMtx}; @@ -252,6 +256,12 @@ class SocketServer { if (_closing.load()) { return; } + // Before taking on another one: nothing else ever removed a + // finished connection, so an fd and a joinable thread handle + // accumulated per connection *ever accepted*, not per live + // connection, until close() (morph#498). + reapFinishedClients(); + auto conn = std::make_shared(std::move(*clientSocket), _server.openConnection()); std::thread clientThread{[this, conn] { clientLoop(conn); }}; { @@ -262,7 +272,52 @@ class SocketServer { } } + /// @brief Drops connections whose `clientLoop` has returned, joining their + /// threads and releasing their sockets. + /// + /// Called from `acceptLoop` only, so it never runs concurrently with itself. + /// Threads are moved out and joined *after* `_clientsMtx` is released: a + /// join can block, and `clientLoop`'s own teardown takes that same mutex + /// through `sendText`, so joining under the lock would deadlock. Every + /// thread collected here has already set `finished`, so each join is + /// effectively immediate. + void reapFinishedClients() { + std::vector doneThreads; + { + std::scoped_lock lock{_clientsMtx}; + for (std::size_t i = _clients.size(); i-- > 0;) { + if (!_clients[i]->finished.load(std::memory_order_acquire)) { + continue; + } + doneThreads.push_back(std::move(_clientThreads[i])); + _clientThreads.erase(_clientThreads.begin() + static_cast(i)); + _clients.erase(_clients.begin() + static_cast(i)); + } + } + for (auto& t : doneThreads) { + if (t.joinable()) { + t.join(); + } + } + } + void clientLoop(const std::shared_ptr& conn) { + // Announces "this thread is done" on every exit path, so acceptLoop's + // reaper can release the fd and join the thread handle rather than + // holding both until close(). Declared *before* the scope guard below so + // it is destroyed last: the flag must not go up until the connection's + // models have actually been reclaimed. morph#498. + struct FinishedFlag { + explicit FinishedFlag(std::atomic& f MORPH_LIFETIMEBOUND) : flag{f} {} + ~FinishedFlag() { flag.store(true, std::memory_order_release); } + FinishedFlag(const FinishedFlag&) = delete; + FinishedFlag& operator=(const FinishedFlag&) = delete; + FinishedFlag(FinishedFlag&&) = delete; + FinishedFlag& operator=(FinishedFlag&&) = delete; + + std::atomic& flag; + } const finishedFlag{conn->finished}; + // Reclaim this connection's models however the loop exits — failed // handshake, peer close, read error, or shutdown via close(). Without // it every model registered over this transport outlived its connection @@ -284,6 +339,7 @@ class SocketServer { ::morph::backend::ConnectionId cid; } const guard{_server, conn->cid}; + std::string leftover; try { leftover = ::morph::net::detail::performServerHandshake(conn->socket); diff --git a/tests/net/test_socket_server.cpp b/tests/net/test_socket_server.cpp index 4c64965a..2c255978 100644 --- a/tests/net/test_socket_server.cpp +++ b/tests/net/test_socket_server.cpp @@ -9,6 +9,7 @@ #include #include #include +#include #include #include #include @@ -1336,3 +1337,76 @@ TEST_CASE("SocketServer: teardown racing a connecting client still finishes prom REQUIRE(elapsed < kBudget); } } + +// ── morph#498: finished connections must be reclaimed while the server runs ── +// +// `_clients` and `_clientThreads` were only ever pushed to in acceptLoop and +// cleared in close(); nothing removed a connection whose clientLoop had +// returned. The shared_ptr kept the ClientConnection -- and its TcpSocket -- +// alive, so the fd stayed open, and each std::thread stayed joinable. The leak +// was therefore per connection *ever accepted*, not per live connection. +// +// The class doc and docs/spec/core/backend.md both say destruction leaves no +// dangling threads, and both are true -- at teardown. That is exactly why this +// test samples the fd count *while the server is still running*: a test that +// opened N connections and then destroyed the server would have passed before +// the fix and proved nothing (invariant 7). +#if !defined(_WIN32) +namespace { +std::size_t openFdCount() { + std::size_t count = 0; + for (const auto& entry : std::filesystem::directory_iterator{"/proc/self/fd"}) { + (void)entry; + ++count; + } + return count; +} +} // namespace + +TEST_CASE("SocketServer: a finished connection's fd and thread are reclaimed before shutdown", + "[net][socket_server][morph498]") { + if (!std::filesystem::exists("/proc/self/fd")) { + SUCCEED("no /proc/self/fd on this platform"); + return; + } + morph::exec::ThreadPoolExecutor pool{2}; + auto server = std::make_shared(pool); + morph::net::SocketServer wsServer{*server, 0}; + REQUIRE(wsServer.listen()); + + // RawWsClient closes its socket on destruction, so each scope block below is + // one full connect/disconnect cycle. + // + // Warm up: the first few settle one-off allocations, so the baseline reflects + // steady state rather than start-up. + for (int i = 0; i < 4; ++i) { + RawWsClient const client{wsServer.port()}; + } + std::this_thread::sleep_for(std::chrono::milliseconds{50}); + + std::size_t const baseline = openFdCount(); + + // Each iteration opens and closes one connection. Before the fix every one + // of these left an fd behind, so the count grew monotonically with N. + constexpr int kRounds = 25; + for (int i = 0; i < kRounds; ++i) { + RawWsClient const client{wsServer.port()}; + } + // Reaping happens on accept, so the last couple of connections are still + // tracked; give the loop one more accept and a moment to settle. + { + RawWsClient const trigger{wsServer.port()}; + } + std::this_thread::sleep_for(std::chrono::milliseconds{100}); + { + RawWsClient const trigger2{wsServer.port()}; + } + std::this_thread::sleep_for(std::chrono::milliseconds{100}); + + std::size_t const after = openFdCount(); + INFO("baseline=" << baseline << " after=" << after << " rounds=" << kRounds); + // Allow generous slack for the two still-unreaped connections and any + // transient fds; what must NOT happen is growth proportional to kRounds. + CHECK(after < baseline + static_cast(kRounds) / 2); +} +#endif // !defined(_WIN32) From 05db71e1cde83958297771937e7053f38b0f000e Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Wed, 9 Sep 2026 22:39:11 +0200 Subject: [PATCH 06/12] Apply clang-format, and re-pin the branch-coverage allowlist Formatting: one comment I inserted joined onto the sentence that followed it, leaving an over-long line in bridge.hpp's `_attachMtx` block. Ran CI's exact command (`git ls-files -z '*.hpp' '*.cpp' | xargs -0 clang-format --dry-run -Werror`) over all 803 tracked files; clean. Allowlist, audited both ways (every `source` pin lands on its own text, every "line N" in `reason` prose resolves to a real entry): - **Dropped two entries** rather than re-pinning them: callback_scope.hpp's `if (_state != nullptr)` and `_state != nullptr && ...` branches no longer exist, having been removed with #499's atomic conversion. Re-pinning a vanished branch would have quietly kept a dead exemption alive. - Re-pinned eight that merely moved. - One needed care: this branch *adds* a second `if (deadlineHandle && schedulerRef)` to `executeVia` (the #502 unwind guard), placed ahead of the existing one, so nearest-occurrence matching pinned the entry to the new line. The entry's own `reason` describes the `.then` disarm, so it is pinned there (1589) instead. Left the new guard unlisted deliberately -- if the coverage job reports it as partial, it should earn its own entry with its own reasoning rather than inherit someone else's. All three suites after the reformat: main 22016 assertions, net 983, qt 550. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01SxkvtHvan2fWKDDaBpqkgv --- include/morph/core/bridge.hpp | 7 +++---- include/morph/net/socket_server.hpp | 1 - scripts/branch_partial_allowlist.json | 30 ++++++++------------------- tests/net/test_socket_server.cpp | 2 +- tests/test_executor.cpp | 2 +- tests/test_render_locale_format.cpp | 3 +-- 6 files changed, 15 insertions(+), 30 deletions(-) diff --git a/include/morph/core/bridge.hpp b/include/morph/core/bridge.hpp index c04546ce..7eac63cf 100644 --- a/include/morph/core/bridge.hpp +++ b/include/morph/core/bridge.hpp @@ -1978,10 +1978,9 @@ class Bridge { // handler registration" (tests/test_shared_instances.cpp) was written to // catch, and which taking the lock there demonstrably reproduces. That read // is ordered rather than locked; see its own comment for the requirement - // that places on a caller of the pre-built-binding overload (morph#505). `switchBackend()` and the reconnect handler, which also touch - // them alongside `_handlers`, take both mutexes together via - // `std::scoped_lock{_mtx, _attachMtx}` (deadlock-safe regardless of - // acquisition order, by `std::scoped_lock`'s own guarantee). + // that places on a caller of the pre-built-binding overload (morph#505). `switchBackend()` and the reconnect + // handler, which also touch them alongside `_handlers`, take both mutexes together via `std::scoped_lock{_mtx, + // _attachMtx}` (deadlock-safe regardless of acquisition order, by `std::scoped_lock`'s own guarantee). std::mutex _attachMtx; mutable std::mutex _sessionMtx; ::morph::session::Context _defaultSession; diff --git a/include/morph/net/socket_server.hpp b/include/morph/net/socket_server.hpp index 9eeefdb5..3136d8a5 100644 --- a/include/morph/net/socket_server.hpp +++ b/include/morph/net/socket_server.hpp @@ -339,7 +339,6 @@ class SocketServer { ::morph::backend::ConnectionId cid; } const guard{_server, conn->cid}; - std::string leftover; try { leftover = ::morph::net::detail::performServerHandshake(conn->socket); diff --git a/scripts/branch_partial_allowlist.json b/scripts/branch_partial_allowlist.json index 2a6e3739..824d88d9 100644 --- a/scripts/branch_partial_allowlist.json +++ b/scripts/branch_partial_allowlist.json @@ -78,18 +78,6 @@ "source": "if (iter != _strands.end() && iter->second == strand) {", "reason": "Unreachable by construction given this class's lock discipline (core audit finding ST1, resolved to (b) by a concurrency-focused review pass after an initial (a)/(b)-undecided pass). `_strands` has exactly two mutation sites: `post()`'s insert-if-absent (this file, `if (!slot) { slot = make_shared(); }`) and this exact block's own erase a few lines below, both under `_mapMtx`. At most one lambda per `Strand` runs at a time (`post()` only schedules when `!strand->running`, and re-arming happens only through this same lambda's own `more` branch), so dispatch for one `Strand` is strictly serial; and only a strand's own currently-running lambda can erase its map entry (the erase fires only in the `!more` branch for the entry this frame just found under `_mapMtx`, and a concurrent `post(key)` while this lambda runs can only push onto the existing `Strand`, never replace it). Together these force `_strands.find(key)` to yield this exact strand whenever this line runs, so `iter->second == strand` cannot be false. No stress test needed: one was considered, but given the strength of the lock-discipline argument it would spend CI time re-confirming an already-proven invariant rather than searching for an unknown one." }, - { - "file": "include/morph/core/callback_scope.hpp", - "line": 225, - "source": "if (_state != nullptr) {", - "reason": "`_state` (a `std::shared_ptr`) is unreachable-null by construction (core audit finding CS1). It is constructed non-null in `CallbackScope()`'s member-initializer (`_state{std::make_shared<...>()}`) and the only other write site is `reset()`, which always reassigns it to another fresh `make_shared` result, never to `nullptr`. `grep -n \"_state\" callback_scope.hpp` confirms these are the only two write sites in the file. Copy and move are both explicitly `= delete`d (\"Identity, not a value\"), so there is no moved-from state to null it either. Raw branch data confirms it empirically: the `!= nullptr` check's false arm shows exactly 0 hits across every recorded hit on this line. No test would close this -- it is a defensive belt-and-suspenders check against a state the class's own constructors and deleted copy/move already make impossible." - }, - { - "file": "include/morph/core/callback_scope.hpp", - "line": 246, - "source": "return _state != nullptr && _state->stopped.load(std::memory_order_acquire);", - "reason": "Same unreachable-null `_state` invariant as line 225 above (core audit finding CS1) applied to this line's leading sub-condition. Only the `_state != nullptr` conjunct is structurally dead; the line's other sub-condition, the actual `stopped` flag, is already exercised both ways by `stopRequested()`'s ordinary tests (both a not-yet-stopped and a stopped read are recorded), so this entry is scoped to the null-check conjunct specifically, not a claim that `stopRequested()`'s real logic is untested." - }, { "file": "include/morph/core/backend.hpp", "line": 777, @@ -104,7 +92,7 @@ }, { "file": "include/morph/core/remote.hpp", - "line": 1426, + "line": 1433, "source": "if (_inFlightExecutes.compare_exchange_weak(current, current + 1, std::memory_order_relaxed)) {", "reason": "Real but requires genuine thread contention to trigger -- accepted as documented rather than closed with a flaky test (core audit finding RM11). The false arm (the CAS lost the race and must retry) needs two threads to genuinely collide on the same atomic increment at the same instant; it is a real, reachable hazard the retry loop correctly handles, not dead code, but inherently non-deterministic to trigger from a test without exact thread-timing control. Same disposition class as `strand.hpp`'s ST1 above, and as core audit finding O1 (`observability.hpp`'s `endSpan`), whose entry left this file once a coverage run showed its arm taken: a stress test with many concurrent `execute()` calls against a tight `maxInFlightExecutes` limit would probably eventually hit it, but flakily." }, @@ -116,13 +104,13 @@ }, { "file": "include/morph/core/bridge.hpp", - "line": 1571, + "line": 1589, "source": "if (deadlineHandle && schedulerRef) {", "reason": "Unreachable by construction, same joint-assignment shape as B6 above (core audit finding B11, reclassified (a)->(b) on review). `deadlineHandle` and `schedulerRef` are assigned together, a few lines above this one in `executeVia`, only inside `if (_executeDeadline.count() > 0 && _timeoutScheduler) { schedulerRef = _timeoutScheduler; ... }` (see the bridge.hpp:1453 entry above) -- there is no path that sets `deadlineHandle` without also having set `schedulerRef` from the same non-null `_timeoutScheduler` in the same conditional. So `schedulerRef` null while `deadlineHandle` is non-null cannot occur; the only theoretically-open arm this compound condition has is structurally impossible." }, { "file": "include/morph/core/remote.hpp", - "line": 1467, + "line": 1511, "source": "if (_timeoutScheduler) {", "reason": "Unreachable by construction, mirrors `bridge.hpp`'s B6 (core audit finding RM10). `setLimitPolicy` (this file) is the only writer of both `_limits.executeTimeout` and `_timeoutScheduler`: `_limits = policy; if (_limits.executeTimeout.count() > 0 && !_timeoutScheduler) { _timeoutScheduler = std::make_unique<...>(); }`, both under `_limitsMtx` -- the same lock this line's enclosing block holds. Nothing anywhere nulls `_timeoutScheduler` afterward, so `dispatchExecute`'s `limits.executeTimeout.count() > 0` guard (this line's enclosing `if`, a few lines above) already guarantees `_timeoutScheduler` is non-null whenever this line runs." }, @@ -140,31 +128,31 @@ }, { "file": "include/morph/net/socket_server.hpp", - "line": 207, + "line": 211, "source": "if (closed.load() || !socket.valid()) {", "reason": "The `!socket.valid()` disjunct is unreachable by construction (net audit, `socket_server.hpp` finding #7). `ClientConnection::socket` is set once at construction and never moved from or reassigned anywhere in this file (`grep -n \"conn->socket\\|->socket\\.\"` finds only method calls on it, never an assignment or `std::move`). `sendText()` is only ever called while a `shared_ptr` keeps the connection alive, and the only place `TcpSocket::valid()` can become false is that socket's own destructor, which cannot run while such a `shared_ptr` is held. `closed.store(true)` (this same class's `close()` handling, `clientLoop`'s catches) is what every code path that could plausibly invalidate the socket sets first, so the `closed.load()` disjunct alone already accounts for every real teardown path this connection can take." }, { "file": "include/morph/net/socket_server.hpp", - "line": 249, + "line": 253, "source": "if (!clientSocket) {", "reason": "Real, reachable race (`tryAccept()` returning nullopt because the pending connection went away before it was taken), but accepted as documented rather than forced with a flaky test after extensive attempts (net audit, `socket_server.hpp` finding #10). Three different techniques were tried: a single real `TcpSocket::connect()` immediately followed by an abortive (`SO_LINGER{1,0}`) close (0/150 hits); a burst of many such attempts to build backlog depth (still 0 hits); and a burst of bare non-blocking `::connect()`+abort attempts skipping `TcpSocket::connect()`'s `getaddrinfo()`/poll overhead (960 attempts across 15 bursts, still 0 hits, with most connections resetting before the TCP handshake progressed far enough to make the listener readable at all, rather than after). No way was found, from outside the process, to reliably land in the specific narrow window this branch requires on this machine. Reported as attempted-and-left-open rather than forcing something flakier." }, { "file": "include/morph/net/socket_backend.hpp", - "line": 96, + "line": 104, "source": "if (_ioThread.joinable()) {", "reason": "Unreachable by construction (net audit, `socket_backend.hpp` extra finding #5). `_ioThread` is started unconditionally in the constructor and has exactly one join site: this line, in the destructor. Nothing else in the file resets, joins, or detaches it, so at destructor time it is always joinable." }, { "file": "include/morph/net/socket_backend.hpp", - "line": 108, + "line": 116, "source": "if (_handlerThread.joinable()) {", - "reason": "Unreachable by construction, same shape as `_ioThread`'s line 96 above (net audit, `socket_backend.hpp` extra finding #6). `_handlerThread` is started unconditionally in the constructor and has exactly one join site: this line, in the destructor. Nothing else resets, joins, or detaches it, so at destructor time it is always joinable." + "reason": "Unreachable by construction, same shape as `_ioThread`'s line 104 above (net audit, `socket_backend.hpp` extra finding #6). `_handlerThread` is started unconditionally in the constructor and has exactly one join site: this line, in the destructor. Nothing else resets, joins, or detaches it, so at destructor time it is always joinable." }, { "file": "include/morph/net/socket_backend.hpp", - "line": 547, + "line": 555, "source": "default:", "reason": "Unreachable except via the adjacent `Error` case it deliberately shares a body with (net audit, `socket_backend.hpp` extra finding #7). `detail::ExecuteReplyKind` is a closed 3-value enum (`Value`/`Timeout`/`Error`), all three handled explicitly above this label; `default:` exists only to satisfy this project's `-Wswitch-default`, per the source's own inline comment directly below this line. Reaching it via any value other than through the `Error` case falling through would require an out-of-range `static_cast` producing a value outside the enum's domain -- undefined behavior, not a legitimate test target." } diff --git a/tests/net/test_socket_server.cpp b/tests/net/test_socket_server.cpp index 2c255978..3f96d5b8 100644 --- a/tests/net/test_socket_server.cpp +++ b/tests/net/test_socket_server.cpp @@ -8,8 +8,8 @@ #include #include #include -#include #include +#include #include #include #include diff --git a/tests/test_executor.cpp b/tests/test_executor.cpp index 03cc350d..938ff81e 100644 --- a/tests/test_executor.cpp +++ b/tests/test_executor.cpp @@ -140,7 +140,7 @@ TEST_CASE("morph::exec::MainThreadExecutor drain runs a bounded chain of tasks t TEST_CASE("MainThreadExecutor: a task throwing a non-std::exception is contained", "[executor][morph501]") { morph::exec::MainThreadExecutor exec; bool ranAfter = false; - exec.post([] { throw 42; }); // not derived from std::exception + exec.post([] { throw 42; }); // not derived from std::exception exec.post([&ranAfter] { ranAfter = true; }); // Before the fix this call propagated the `int` instead of returning. diff --git a/tests/test_render_locale_format.cpp b/tests/test_render_locale_format.cpp index cbf1898a..cdd4c3d6 100644 --- a/tests/test_render_locale_format.cpp +++ b/tests/test_render_locale_format.cpp @@ -153,8 +153,7 @@ TEST_CASE("render::locale_format round-trips through a multi-byte separator", "[ // guard still believed nothing had been emitted and accepted an injected sign. // The QML mirror (src/qt/forms/qml/DynamicForm.qml, documented as mirroring // this function) always rejected these, so the two control edges disagreed. -TEST_CASE("normalizeLocaleNumber: a sign after the decimal separator is rejected", - "[render][locale][morph497]") { +TEST_CASE("normalizeLocaleNumber: a sign after the decimal separator is rejected", "[render][locale][morph497]") { // de-DE: comma decimal, dot grouping -- the reported shape. REQUIRE_FALSE(morph::render::normalizeLocaleNumber(",-5", ",", ".").has_value()); // en-US equivalent. From 619292c4543d6f68b93821f2ffe1fb335c08e92a Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Wed, 9 Sep 2026 22:48:05 +0200 Subject: [PATCH 07/12] docs: collapse a paragraph duplicated 17 times, and eight more stale comments (#504) **The "public macro surface" paragraph appeared 17 times** -- registry.hpp x8, model_key.hpp x4, quantity.hpp x2, and one each in views/app/flows -- six identical lines explaining why those macros carry `// clang-format off`. Well past the rule-of-three, and provably drifting: it claims the formatter "broke a token-paste invocation apart", and four of the six files contain no `##` at all. Now one canonical statement in CONTRIBUTING.md under Formatting/linting, with a one-line pointer at each site. Eight comments that assert something the code does not do: - `wire.hpp` -- the `Envelope` discriminator list is the declared authority ("see class docstring for valid values") and omitted `"hello"`, which `makeHello()` produces and `interpretHelloReply()` consumes; docs/spec/core/wire.md does list it. Also: `interpretHelloReply` documents requiring an `"err"` kind but matches on the message alone -- documented as deliberate rather than silently tightened, since a peer old enough not to know `hello` is one whose error shape we should not depend on. - `execute_order_gate.hpp` -- "Defensive; should not happen" over a branch the file's own contract lists as a first-class element ("tolerate a gate already erased") and which tests/test_execute_order_gate.cpp names in a test title. - `logger.hpp` -- called an unlocked read of `minLevel` a data race; it is a `std::atomic`. Only `sink` (a `std::function`) is. The real reason to take the lock is that the two must be captured as a *pair*, since `setLogger` and `setLogLevel` are separate calls. - `backend.hpp` -- `_changeAware` is inserted by `createAndTrack`, which `registerModelShared` calls directly without going through `registerModel`. - `reply_router.hpp` -- "allocated and stored under different locks" describes a relationship that does not exist; the call-id counter is a lock-free atomic read outside `_mtx`, deliberately. - `ws_handshake.hpp` -- the "64 KiB safety cap" is checked before each recv, so the real bound is one 4 KiB chunk higher, which the in-tree test already says. - `socket_server.hpp` -- replies are not "marshalled back onto the owning connection's own write path"; they are written inline on the worker-pool thread under the per-connection write mutex, as the spec correctly states. **One item was reported as a defect and turned out not to be.** #504 lists `model.hpp`'s `#include "strand.hpp"` as unused. It is unused *by model.hpp* -- but removing it broke tests/test_model.cpp, which reached `morph::exec::detail::ModelId` through it. For a header-only public library, dropping a transitive include is a source-breaking change for consumers and not worth the tidiness, so the include stays with a comment saying why. The `` gap that item flagged alongside it is real and is fixed: this file uses `std::same_as` and `concept` without including it. Suites unchanged: main 22015 assertions, net 983. Tree-wide clang-format clean. Allowlist re-pinned, audited both ways. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01SxkvtHvan2fWKDDaBpqkgv --- CONTRIBUTING.md | 10 ++++ include/morph/core/backend.hpp | 2 +- .../morph/core/detail/execute_order_gate.hpp | 7 ++- include/morph/core/detail/reply_router.hpp | 5 +- include/morph/core/logger.hpp | 9 ++- include/morph/core/model.hpp | 7 +++ include/morph/core/model_key.hpp | 28 ++-------- include/morph/core/registry.hpp | 56 +++---------------- include/morph/core/wire.hpp | 11 +++- include/morph/forms/app.hpp | 7 +-- include/morph/forms/flows.hpp | 7 +-- include/morph/forms/views.hpp | 7 +-- include/morph/net/detail/ws_handshake.hpp | 4 +- include/morph/net/socket_server.hpp | 5 +- include/morph/util/quantity.hpp | 14 +---- scripts/branch_partial_allowlist.json | 6 +- 16 files changed, 71 insertions(+), 114 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 65762426..46f5637e 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -93,6 +93,16 @@ serialising independent rungs behind one file. - **Formatting/linting:** `.clang-format` and `.clang-tidy` govern C++; markdown follows `.markdownlint.yaml` (119-column limit; code blocks and tables exempt). `pre-commit run --all-files` runs the configured hooks. + + **Public macro definitions are exempt, by `// clang-format off`.** They are + the framework's documented API and contributors read them as reference, so + the continuation backslashes are hand-aligned and the body stays legible as + a block; leaving them to the formatter means any unrelated edit nearby + re-wraps the whole definition, and in one case it broke a token-paste + (`##`) invocation apart. Freeze them, and realign by hand if a body changes. + The sites carry a one-line pointer back here rather than repeating this + paragraph — it used to be copy-pasted at all seventeen of them, and two of + those copies had drifted into describing code that was not there. - **Keep mechanical facts honest:** `docs/spec/pinned_facts.toml` pins the mechanical facts that recur across specs — enum cardinalities, key constants (`kMaxEnvelopeBytes`, `kMaxDecimalPlaces`, `kClockSkewMs`), diff --git a/include/morph/core/backend.hpp b/include/morph/core/backend.hpp index 9185208d..d13b16ab 100644 --- a/include/morph/core/backend.hpp +++ b/include/morph/core/backend.hpp @@ -742,7 +742,7 @@ class LocalBackend : public detail::IBackend { /// @brief Schedules `onBackendChanged()` on each change-aware model's strand. Thread-safe. /// - /// Only models recorded in `_changeAware` — maintained by `registerModel`/ + /// Only models recorded in `_changeAware` — maintained by `createAndTrack`/ /// `deregisterModel` from `IModelHolder::isBackendChangeAware()`, a /// compile-time answer per model type — are visited; there is no /// `dynamic_cast` and no scan of models that never opted in. Each such diff --git a/include/morph/core/detail/execute_order_gate.hpp b/include/morph/core/detail/execute_order_gate.hpp index 1ae43edf..65e69827 100644 --- a/include/morph/core/detail/execute_order_gate.hpp +++ b/include/morph/core/detail/execute_order_gate.hpp @@ -87,7 +87,12 @@ class ExecuteOrderGate { std::scoped_lock const lock{_mtx}; auto iter = _gates.find(mid); if (iter == _gates.end()) { - return; // Defensive; should not happen (this ticket's own take() created the entry). + // A supported call, not an impossibility: the file-level contract + // above lists "tolerate a gate already erased" as a first-class + // element, and tests/test_execute_order_gate.cpp names the case + // ("releasing a ticket for a model with no gate entry is a + // harmless no-op"). + return; } auto& gate = *iter->second; // `nextToRun` is the lowest ticket that has *not* released yet, which diff --git a/include/morph/core/detail/reply_router.hpp b/include/morph/core/detail/reply_router.hpp index d55c72af..16bb3936 100644 --- a/include/morph/core/detail/reply_router.hpp +++ b/include/morph/core/detail/reply_router.hpp @@ -92,7 +92,10 @@ enum class ExecuteReplyKind : std::uint8_t { /// `operator[]`, which value-initializes before assigning. /// /// Owns the call-id counter as well as the map, so the two cannot be -/// allocated and stored under different locks by accident. +/// allocated and stored by different owners by accident. Note the counter is a +/// lock-free `std::atomic` read *outside* `_mtx`, on purpose: allocating an id +/// and inserting its entry are deliberately not one atomic step -- `insertIf`'s +/// admit predicate is what makes the insert safe, not a shared lock. template class PendingCallTable { public: diff --git a/include/morph/core/logger.hpp b/include/morph/core/logger.hpp index e6e66a13..c87ff9e2 100644 --- a/include/morph/core/logger.hpp +++ b/include/morph/core/logger.hpp @@ -273,8 +273,13 @@ class ScopedLoggerOverride { /// `setLogger()` / `setLogLevel()` and just wants automatic restoration. ScopedLoggerOverride() { // NOLINTBEGIN(cppcoreguidelines-prefer-member-initializer) — these must be - // read while holding the lock; a member-initializer list would read the - // global state before the mutex is acquired (a data race). + // read while holding the lock, and a member-initializer list would read + // the global state before the mutex is acquired. For `sink` (a + // std::function) that would be a genuine data race; `minLevel` is a + // std::atomic and would merely be stale. The reason both are taken here + // is that they must be captured as one *pair*: setLogger and setLogLevel + // are separate calls, and a snapshot straddling them would restore a + // combination that never existed. std::scoped_lock const lock{detail::logState().mtx}; _savedSink = detail::logState().sink; _savedLevel = detail::logState().minLevel; diff --git a/include/morph/core/model.hpp b/include/morph/core/model.hpp index e231a533..38fe4676 100644 --- a/include/morph/core/model.hpp +++ b/include/morph/core/model.hpp @@ -2,11 +2,18 @@ #pragma once #include +#include #include #include #include #include +// strand.hpp defines no symbol this header uses. It is kept deliberately: +// consumers (tests/test_model.cpp among them) reach +// morph::exec::detail::ModelId through it, and for a header-only public library +// dropping a transitive include is a source-breaking change for its users -- +// not worth the tidiness. The include above is the real gap it was +// masking; this file uses std::same_as and `concept` without including it. #include "../journal/action_log.hpp" #include "../session/session.hpp" #include "strand.hpp" diff --git a/include/morph/core/model_key.hpp b/include/morph/core/model_key.hpp index 251e0ae6..145f5bd5 100644 --- a/include/morph/core/model_key.hpp +++ b/include/morph/core/model_key.hpp @@ -288,12 +288,7 @@ concept ResultKeyed = ActionKeyTraits::hasKey && ActionKeyTraits::fromResu /// which `[basic.def.odr]` permits in multiple translation units when the /// definitions are token-identical. See docs/spec/core/registry.md, "Header /// placement is legal". -// clang-format off -- public macro surface: hand-aligned on purpose. -// These definitions are the framework's documented API; contributors read them -// as reference, and the continuation backslashes line up so the body is legible -// as a block. Leaving them to the formatter means any unrelated edit nearby -// re-wraps the whole definition, and in one case it broke a token-paste -// invocation apart. Freeze them; realign by hand if a body changes. +// clang-format off -- public macro surface; see CONTRIBUTING.md, "Formatting/linting". #define BRIDGE_MODEL_KEY(M, A, MEMBER) \ template <> \ struct morph::model::ActionKeyTraits { \ @@ -318,12 +313,7 @@ concept ResultKeyed = ActionKeyTraits::hasKey && ActionKeyTraits::fromResu /// Must appear at global scope; a header included by several translation units /// is fine (only explicit specialisations are emitted — see /// docs/spec/core/registry.md, "Header placement is legal"). -// clang-format off -- public macro surface: hand-aligned on purpose. -// These definitions are the framework's documented API; contributors read them -// as reference, and the continuation backslashes line up so the body is legible -// as a block. Leaving them to the formatter means any unrelated edit nearby -// re-wraps the whole definition, and in one case it broke a token-paste -// invocation apart. Freeze them; realign by hand if a body changes. +// clang-format off -- public macro surface; see CONTRIBUTING.md, "Formatting/linting". #define BRIDGE_KEY_FROM(A, MEMBER) \ template <> \ struct morph::model::ActionKeyTraits { \ @@ -343,12 +333,7 @@ concept ResultKeyed = ActionKeyTraits::hasKey && ActionKeyTraits::fromResu /// Must appear at global scope; a header included by several translation units /// is fine (only explicit specialisations are emitted — see /// docs/spec/core/registry.md, "Header placement is legal"). -// clang-format off -- public macro surface: hand-aligned on purpose. -// These definitions are the framework's documented API; contributors read them -// as reference, and the continuation backslashes line up so the body is legible -// as a block. Leaving them to the formatter means any unrelated edit nearby -// re-wraps the whole definition, and in one case it broke a token-paste -// invocation apart. Freeze them; realign by hand if a body changes. +// clang-format off -- public macro surface; see CONTRIBUTING.md, "Formatting/linting". #define BRIDGE_MODEL_KEY_FROM_RESULT(M, A, MEMBER) \ template <> \ struct morph::model::ActionKeyTraits { \ @@ -373,12 +358,7 @@ concept ResultKeyed = ActionKeyTraits::hasKey && ActionKeyTraits::fromResu /// Must appear at global scope; a header included by several translation units /// is fine (only explicit specialisations are emitted — see /// docs/spec/core/registry.md, "Header placement is legal"). -// clang-format off -- public macro surface: hand-aligned on purpose. -// These definitions are the framework's documented API; contributors read them -// as reference, and the continuation backslashes line up so the body is legible -// as a block. Leaving them to the formatter means any unrelated edit nearby -// re-wraps the whole definition, and in one case it broke a token-paste -// invocation apart. Freeze them; realign by hand if a body changes. +// clang-format off -- public macro surface; see CONTRIBUTING.md, "Formatting/linting". #define BRIDGE_KEY_FROM_RESULT(A, MEMBER) \ template <> \ struct morph::model::ActionKeyTraits { \ diff --git a/include/morph/core/registry.hpp b/include/morph/core/registry.hpp index 28b50110..85dde4b9 100644 --- a/include/morph/core/registry.hpp +++ b/include/morph/core/registry.hpp @@ -902,12 +902,7 @@ bool registerActionExecutorOnce(std::string_view modelId, std::string_view actio /// /// @param M Concrete model type. /// @param NAME String literal used as the type-id. -// clang-format off -- public macro surface: hand-aligned on purpose. -// These definitions are the framework's documented API; contributors read them -// as reference, and the continuation backslashes line up so the body is legible -// as a block. Leaving them to the formatter means any unrelated edit nearby -// re-wraps the whole definition, and in one case it broke a token-paste -// invocation apart. Freeze them; realign by hand if a body changes. +// clang-format off -- public macro surface; see CONTRIBUTING.md, "Formatting/linting". #define BRIDGE_REGISTER_MODEL(M, NAME) \ template <> \ struct morph::model::ModelTraits { \ @@ -943,12 +938,7 @@ bool registerActionExecutorOnce(std::string_view modelId, std::string_view actio /// @param NAME String literal used as the action type-id. /// @param ... Optional: a `morph::model::Loggable` value (defaults to `Loggable::Yes`). // NOLINTBEGIN(cppcoreguidelines-macro-usage) — registration macros are the intended public API -// clang-format off -- public macro surface: hand-aligned on purpose. -// These definitions are the framework's documented API; contributors read them -// as reference, and the continuation backslashes line up so the body is legible -// as a block. Leaving them to the formatter means any unrelated edit nearby -// re-wraps the whole definition, and in one case it broke a token-paste -// invocation apart. Freeze them; realign by hand if a body changes. +// clang-format off -- public macro surface; see CONTRIBUTING.md, "Formatting/linting". #define BRIDGE_REGISTER_ACTION(...) \ BRIDGE_REGISTER_ACTION_PICK(__VA_ARGS__, BRIDGE_REGISTER_ACTION_4, BRIDGE_REGISTER_ACTION_3) \ (__VA_ARGS__) @@ -1003,12 +993,7 @@ bool registerActionExecutorOnce(std::string_view modelId, std::string_view actio /// @param RESULT The action's result type, named explicitly. /// @param NAME String literal used as the action type-id. /// @param ... Optional: a `morph::model::Loggable` value (defaults to `Loggable::Yes`). -// clang-format off -- public macro surface: hand-aligned on purpose. -// These definitions are the framework's documented API; contributors read them -// as reference, and the continuation backslashes line up so the body is legible -// as a block. Leaving them to the formatter means any unrelated edit nearby -// re-wraps the whole definition, and in one case it broke a token-paste -// invocation apart. Freeze them; realign by hand if a body changes. +// clang-format off -- public macro surface; see CONTRIBUTING.md, "Formatting/linting". #define BRIDGE_REGISTER_ACTION_FOR_CLIENT(...) \ BRIDGE_REGISTER_ACTION_FOR_CLIENT_PICK(__VA_ARGS__, BRIDGE_REGISTER_ACTION_FOR_CLIENT_5, \ BRIDGE_REGISTER_ACTION_FOR_CLIENT_4) \ @@ -1018,12 +1003,7 @@ bool registerActionExecutorOnce(std::string_view modelId, std::string_view actio /// @cond detail #define BRIDGE_REGISTER_ACTION_FOR_CLIENT_PICK(_1, _2, _3, _4, _5, NAME, ...) NAME -// clang-format off -- public macro surface: hand-aligned on purpose. -// These definitions are the framework's documented API; contributors read them -// as reference, and the continuation backslashes line up so the body is legible -// as a block. Leaving them to the formatter means any unrelated edit nearby -// re-wraps the whole definition, and in one case it broke a token-paste -// invocation apart. Freeze them; realign by hand if a body changes. +// clang-format off -- public macro surface; see CONTRIBUTING.md, "Formatting/linting". #define BRIDGE_REGISTER_ACTION_FOR_CLIENT_4(M, A, RESULT, NAME) \ BRIDGE_REGISTER_ACTION_FOR_CLIENT_5(M, A, RESULT, NAME, ::morph::model::Loggable::Yes) // clang-format on @@ -1047,12 +1027,7 @@ bool registerActionExecutorOnce(std::string_view modelId, std::string_view actio /// deduction from `M::execute(A)`. /// @param NAME String literal used as the action type-id. /// @param LOGGABLE A `morph::model::Loggable` value. -// clang-format off -- public macro surface: hand-aligned on purpose. -// These definitions are the framework's documented API; contributors read them -// as reference, and the continuation backslashes line up so the body is legible -// as a block. Leaving them to the formatter means any unrelated edit nearby -// re-wraps the whole definition, and in one case it broke a token-paste -// invocation apart. Freeze them; realign by hand if a body changes. +// clang-format off -- public macro surface; see CONTRIBUTING.md, "Formatting/linting". #define BRIDGE_DETAIL_ACTION_TRAITS_BODY(M, A, RESULT_ALIAS, NAME, LOGGABLE) \ template <> \ struct morph::model::ActionTraits { \ @@ -1124,12 +1099,7 @@ bool registerActionExecutorOnce(std::string_view modelId, std::string_view actio } // clang-format on -// clang-format off -- public macro surface: hand-aligned on purpose. -// These definitions are the framework's documented API; contributors read them -// as reference, and the continuation backslashes line up so the body is legible -// as a block. Leaving them to the formatter means any unrelated edit nearby -// re-wraps the whole definition, and in one case it broke a token-paste -// invocation apart. Freeze them; realign by hand if a body changes. +// clang-format off -- public macro surface; see CONTRIBUTING.md, "Formatting/linting". #define BRIDGE_REGISTER_ACTION_FOR_CLIENT_5(M, A, RESULT, NAME, LOGGABLE) \ BRIDGE_DETAIL_ACTION_TRAITS_BODY(M, A, RESULT, NAME, LOGGABLE) // clang-format on @@ -1140,12 +1110,7 @@ bool registerActionExecutorOnce(std::string_view modelId, std::string_view actio #define BRIDGE_REGISTER_ACTION_3(M, A, NAME) BRIDGE_REGISTER_ACTION_4(M, A, NAME, ::morph::model::Loggable::Yes) -// clang-format off -- public macro surface: hand-aligned on purpose. -// These definitions are the framework's documented API; contributors read them -// as reference, and the continuation backslashes line up so the body is legible -// as a block. Leaving them to the formatter means any unrelated edit nearby -// re-wraps the whole definition, and in one case it broke a token-paste -// invocation apart. Freeze them; realign by hand if a body changes. +// clang-format off -- public macro surface; see CONTRIBUTING.md, "Formatting/linting". #define BRIDGE_REGISTER_ACTION_4(M, A, NAME, LOGGABLE) \ BRIDGE_DETAIL_ACTION_TRAITS_BODY(M, A, decltype(std::declval().execute(std::declval())), NAME, LOGGABLE) // clang-format on @@ -1159,12 +1124,7 @@ bool registerActionExecutorOnce(std::string_view modelId, std::string_view actio /// /// @param A Concrete action type. /// @param FN Callable `bool(const A&)`. -// clang-format off -- public macro surface: hand-aligned on purpose. -// These definitions are the framework's documented API; contributors read them -// as reference, and the continuation backslashes line up so the body is legible -// as a block. Leaving them to the formatter means any unrelated edit nearby -// re-wraps the whole definition, and in one case it broke a token-paste -// invocation apart. Freeze them; realign by hand if a body changes. +// clang-format off -- public macro surface; see CONTRIBUTING.md, "Formatting/linting". #define BRIDGE_REGISTER_VALIDATOR(A, FN) \ template <> \ struct morph::model::ActionValidator { \ diff --git a/include/morph/core/wire.hpp b/include/morph/core/wire.hpp index 461b0d4a..556cb725 100644 --- a/include/morph/core/wire.hpp +++ b/include/morph/core/wire.hpp @@ -66,6 +66,11 @@ inline constexpr std::uint32_t kProtocolVersion = 1; /// the serialized result in `body`. For `register` replies, /// `modelId` carries the new id. /// - `"err"` — server failure reply. Uses `callId` if present and `message`. +/// - `"hello"` — client opens protocol-version negotiation, once per +/// connection before any other kind. Uses `protocolVersion`; +/// see `makeHello()` and `interpretHelloReply()`. A peer +/// predating negotiation answers `err "unknown envelope kind: +/// hello"`, which is how a legacy server is detected. struct Envelope { /// @brief Discriminator — see class docstring for valid values. std::string kind; @@ -568,7 +573,7 @@ enum class ProtocolNegotiationResult : std::uint8_t { /// @param reply Decoded reply envelope — the result of `decode()` on the /// response to a `"hello"` round-trip. /// @return `Negotiated` if @p reply's `kind` is `"ok"`; `LegacyPeer` if it is -/// an `"err"` whose `message` is exactly `"unknown envelope kind: +/// an envelope whose `message` is exactly `"unknown envelope kind: /// hello"` — the generic unrecognised-`kind` message a pre-negotiation /// `RemoteServer` produces for a `kind` it does not switch on. /// @throws std::runtime_error if @p reply is any other `"err"` (e.g. @@ -579,6 +584,10 @@ inline ProtocolNegotiationResult interpretHelloReply(const Envelope& reply) { if (reply.kind == "ok") { return ProtocolNegotiationResult::Negotiated; } + // Matched on `message` alone: `kind` is deliberately not inspected, because a + // peer old enough to not know `hello` is also a peer whose error shape we do + // not want to depend on. Any envelope carrying exactly this message is + // treated as a legacy peer. if (reply.message == "unknown envelope kind: hello") { return ProtocolNegotiationResult::LegacyPeer; } diff --git a/include/morph/forms/app.hpp b/include/morph/forms/app.hpp index cb91678f..23f25cdf 100644 --- a/include/morph/forms/app.hpp +++ b/include/morph/forms/app.hpp @@ -154,12 +154,7 @@ template /// reference are (see docs/spec/forms/workflows_navigation.md). /// @param A Concrete `morph::app::App<...>` type. /// @param NAME String literal used as the app's type-id. -// clang-format off -- public macro surface: hand-aligned on purpose. -// These definitions are the framework's documented API; contributors read them -// as reference, and the continuation backslashes line up so the body is legible -// as a block. Leaving them to the formatter means any unrelated edit nearby -// re-wraps the whole definition, and in one case it broke a token-paste -// invocation apart. Freeze them; realign by hand if a body changes. +// clang-format off -- public macro surface; see CONTRIBUTING.md, "Formatting/linting". #define BRIDGE_REGISTER_APP(A, NAME) \ template <> \ struct morph::app::AppTraits { \ diff --git a/include/morph/forms/flows.hpp b/include/morph/forms/flows.hpp index 5dd301cd..16995668 100644 --- a/include/morph/forms/flows.hpp +++ b/include/morph/forms/flows.hpp @@ -553,12 +553,7 @@ class FlowSession { /// docs/spec/forms/workflows_navigation.md). /// @param W Concrete `morph::flows::Wizard<...>` type. /// @param NAME String literal used as the wizard's type-id. -// clang-format off -- public macro surface: hand-aligned on purpose. -// These definitions are the framework's documented API; contributors read them -// as reference, and the continuation backslashes line up so the body is legible -// as a block. Leaving them to the formatter means any unrelated edit nearby -// re-wraps the whole definition, and in one case it broke a token-paste -// invocation apart. Freeze them; realign by hand if a body changes. +// clang-format off -- public macro surface; see CONTRIBUTING.md, "Formatting/linting". #define BRIDGE_REGISTER_WIZARD(W, NAME) \ template <> \ struct morph::flows::WizardTraits { \ diff --git a/include/morph/forms/views.hpp b/include/morph/forms/views.hpp index ebb5e724..f8e70ae6 100644 --- a/include/morph/forms/views.hpp +++ b/include/morph/forms/views.hpp @@ -484,12 +484,7 @@ inline bool registerViewOnce(std::string_view viewId) noexcept { /// `using ns::V;` first — this macro pastes `V` into an /// identifier, so it cannot be namespace-qualified). /// @param NAME String literal used as the view's type-id. -// clang-format off -- public macro surface: hand-aligned on purpose. -// These definitions are the framework's documented API; contributors read them -// as reference, and the continuation backslashes line up so the body is legible -// as a block. Leaving them to the formatter means any unrelated edit nearby -// re-wraps the whole definition, and in one case it broke a token-paste -// invocation apart. Freeze them; realign by hand if a body changes. +// clang-format off -- public macro surface; see CONTRIBUTING.md, "Formatting/linting". #define BRIDGE_REGISTER_VIEW(V, NAME) \ template <> \ struct morph::views::ViewTraits { \ diff --git a/include/morph/net/detail/ws_handshake.hpp b/include/morph/net/detail/ws_handshake.hpp index 07cb3054..d7c33465 100644 --- a/include/morph/net/detail/ws_handshake.hpp +++ b/include/morph/net/detail/ws_handshake.hpp @@ -267,7 +267,9 @@ struct HandshakeReadResult { /// @param socket Connected socket to read from. /// @return The header text and any leftover bytes read past the terminator. /// @throws std::runtime_error if the peer closes before completing the header, -/// or if the header exceeds a 64 KiB safety cap. +/// or if the header exceeds a ~64 KiB safety cap. The check runs before +/// each `recvSome`, so the true bound is 64 KiB rounded up to the next +/// read chunk (tests/net/test_handshake_over_socket.cpp says the same). inline HandshakeReadResult readHttpHeaderBlock(TcpSocket& socket) { std::string buf; char chunk[4096]; diff --git a/include/morph/net/socket_server.hpp b/include/morph/net/socket_server.hpp index 3136d8a5..9a82c691 100644 --- a/include/morph/net/socket_server.hpp +++ b/include/morph/net/socket_server.hpp @@ -41,8 +41,9 @@ struct SocketServerConfig { /// @par Threading /// Owns one accept thread plus one thread per accepted connection — there is /// no shared event loop. `RemoteServer::handle()` replies arrive on the -/// server's worker-pool thread and are marshalled back onto the owning -/// connection's own write path, serialized by a per-connection mutex. +/// server's worker-pool thread and are written back on that same thread, over +/// the owning connection's socket, serialized by a per-connection write mutex. +/// Nothing is handed to the connection's own reader thread. /// /// @par Lifetime /// Holds `RemoteServer& _server` by reference, exactly like diff --git a/include/morph/util/quantity.hpp b/include/morph/util/quantity.hpp index 83e41c4d..be671ea2 100644 --- a/include/morph/util/quantity.hpp +++ b/include/morph/util/quantity.hpp @@ -546,12 +546,7 @@ struct Context { /// @cond INTERNAL #if MORPH_QUANTITY_PROVENANCE #define MORPH_Q_NODE(quantity) (quantity)._ctx.node -// clang-format off -- public macro surface: hand-aligned on purpose. -// These definitions are the framework's documented API; contributors read them -// as reference, and the continuation backslashes line up so the body is legible -// as a block. Leaving them to the formatter means any unrelated edit nearby -// re-wraps the whole definition, and in one case it broke a token-paste -// invocation apart. Freeze them; realign by hand if a body changes. +// clang-format off -- public macro surface; see CONTRIBUTING.md, "Formatting/linting". #define MORPH_Q_BUILD(out, op, lhsValue, rhsValue, resultValue, leftNode, rightNode) \ do { \ auto morphProvNode = std::make_shared<::morph::units::detail::ASTNode>(); \ @@ -568,12 +563,7 @@ struct Context { #else #define MORPH_Q_NODE(quantity) nullptr -// clang-format off -- public macro surface: hand-aligned on purpose. -// These definitions are the framework's documented API; contributors read them -// as reference, and the continuation backslashes line up so the body is legible -// as a block. Leaving them to the formatter means any unrelated edit nearby -// re-wraps the whole definition, and in one case it broke a token-paste -// invocation apart. Freeze them; realign by hand if a body changes. +// clang-format off -- public macro surface; see CONTRIBUTING.md, "Formatting/linting". #define MORPH_Q_BUILD(out, op, lhsValue, rhsValue, resultValue, leftNode, rightNode) \ do { \ } while (0) diff --git a/scripts/branch_partial_allowlist.json b/scripts/branch_partial_allowlist.json index 824d88d9..596efbeb 100644 --- a/scripts/branch_partial_allowlist.json +++ b/scripts/branch_partial_allowlist.json @@ -122,19 +122,19 @@ }, { "file": "include/morph/net/socket_server.hpp", - "line": 188, + "line": 189, "source": "if (t.joinable()) {", "reason": "Unreachable by construction (net audit, `socket_server.hpp` finding #6, re-verified against the current `close()` after the morph#451 fix serialized its whole body under `_closeMtx`). `_clientThreads` has exactly one push site (`acceptLoop()`, always a freshly-constructed, running `std::thread`) and this loop is the only place any entry is ever joined or detached. With `close()`'s entire body now serialized by `_closeMtx`, only one caller's `close()` can ever reach this loop: a second, later call observes `wasAlreadyClosing == true` and `!_acceptThread.joinable()` (already joined by the winner) and takes the early return above, before ever reaching the client-thread swap-and-join section this line is in. So every `std::thread` this loop iterates over is a fresh entry pushed by `acceptLoop()` that nothing has touched yet -- `joinable()` cannot be false here." }, { "file": "include/morph/net/socket_server.hpp", - "line": 211, + "line": 212, "source": "if (closed.load() || !socket.valid()) {", "reason": "The `!socket.valid()` disjunct is unreachable by construction (net audit, `socket_server.hpp` finding #7). `ClientConnection::socket` is set once at construction and never moved from or reassigned anywhere in this file (`grep -n \"conn->socket\\|->socket\\.\"` finds only method calls on it, never an assignment or `std::move`). `sendText()` is only ever called while a `shared_ptr` keeps the connection alive, and the only place `TcpSocket::valid()` can become false is that socket's own destructor, which cannot run while such a `shared_ptr` is held. `closed.store(true)` (this same class's `close()` handling, `clientLoop`'s catches) is what every code path that could plausibly invalidate the socket sets first, so the `closed.load()` disjunct alone already accounts for every real teardown path this connection can take." }, { "file": "include/morph/net/socket_server.hpp", - "line": 253, + "line": 254, "source": "if (!clientSocket) {", "reason": "Real, reachable race (`tryAccept()` returning nullopt because the pending connection went away before it was taken), but accepted as documented rather than forced with a flaky test after extensive attempts (net audit, `socket_server.hpp` finding #10). Three different techniques were tried: a single real `TcpSocket::connect()` immediately followed by an abortive (`SO_LINGER{1,0}`) close (0/150 hits); a burst of many such attempts to build backlog depth (still 0 hits); and a burst of bare non-blocking `::connect()`+abort attempts skipping `TcpSocket::connect()`'s `getaddrinfo()`/poll overhead (960 attempts across 15 bursts, still 0 hits, with most connections resetting before the TCP handshake progressed far enough to make the listener readable at all, rather than after). No way was found, from outside the process, to reliably land in the specific narrow window this branch requires on this machine. Reported as attempted-and-left-open rather than forcing something flakier." }, From 604da9bca76db7ef6af5f2c21e18b1eda9b6b084 Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Wed, 9 Sep 2026 22:52:21 +0200 Subject: [PATCH 08/12] mutation: record the scheduled run, and close the gap its survivors exposed (#453) Item A asks for the scheduled campaign's survivors classified rather than counted. This does the part that can be done from evidence and says plainly how much is left. **Recorded run[2]** in scripts/mutation_survivors.json from the first scheduled-gate campaign (run 34349442137): 773 mutants, 574 killed, 199 survived, 74.26%, with per-file and per-mutator breakdowns. Not comparable to the 352-survivor figure above it -- that was measured over a larger population, before cxx_remove_void_call was excluded. **Classified 16 of 199, and closed a real gap.** Clustering by (mutator, source shape) rather than assessing them one at a time put sixteen bitwise survivors in one place, and they split two ways: - **Thirteen are provably equivalent.** Four sit inside `OpaqueIdGenerator::mix`, the Feistel round function -- and the Feistel construction is invertible for *any* round function, which `permute()`'s own doc comment already states. Mutating F preserves the only property the code claims. The other nine are hash-combine sites; nothing in the contract fixes a hash *value*, so killing them would mean asserting a hash constant, which is the "test with no reason to exist" this issue explicitly warns against. - **Three were a genuine hole, now closed.** They sit on the Feistel *structure* rather than inside it, and they survive because the existing bijection test drives counters 1..20000 -- every one of which has a **zero high half**. That test therefore passes whether or not `permute()` uses the top 32 bits at all. Simulated against the real round function over counters that do exercise the high half: `>>` -> `<<` collapses 20000 ids to **1**, and the Feistel `^` -> `|` collapses them to **1256**. Catastrophic collisions in opaque model ids -- which exist to stop a client guessing another client's id -- and invisible to the sample. New test drives counters strided by 2^32; it kills the first two. The third (`hi << 32` -> `>>`) still survives: the id degenerates to its low half, which stays distinct for these inputs, so killing it needs a probe on the id's high bits rather than a distinctness count. Recorded as such. **183 of 199 remain unclassified**, and the file says so rather than implying a finished pass. remote.hpp (82) and bridge.hpp (33) hold 58% of them and have not been assessed; the largest cluster is cxx_replace_scalar_call (92), concentrated on `.empty()`/`.size()` guards. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01SxkvtHvan2fWKDDaBpqkgv --- scripts/mutation_survivors.json | 99 +++++++++++++++++++++++++++++++++ tests/test_opaque_model_ids.cpp | 33 +++++++++++ 2 files changed, 132 insertions(+) diff --git a/scripts/mutation_survivors.json b/scripts/mutation_survivors.json index 281a3f03..a5bebdcf 100644 --- a/scripts/mutation_survivors.json +++ b/scripts/mutation_survivors.json @@ -234,6 +234,63 @@ "cxx_gt_to_ge": 8, "other": 37 } + }, + { + "label": "first scheduled-gate run, cxx_remove_void_call excluded", + "date": "2026-09-09", + "scope": "core-forms", + "driven_by": ".github/workflows/mutation.yml (workflow_dispatch, run 34349442137)", + "tool": "Mull 0.34.0 / LLVM 22", + "compiler": "clang-22 (ubuntu-26.04 runner)", + "mutators": "scripts/mutation.sh's list, with cxx_remove_void_call excluded (morph#434)", + "mutants": 773, + "killed": 574, + "survived": 199, + "mutation_score_percent": 74.26, + "wall_time": "116m4s campaign, 2h25m job", + "movement_against_baseline": "first run under this mutator set; not comparable to the 352-survivor figure above, which was measured over a larger population", + "survivors_by_file": { + "include/morph/core/remote.hpp": 82, + "include/morph/core/bridge.hpp": 33, + "include/morph/forms/forms.hpp": 13, + "include/morph/core/registry.hpp": 12, + "include/morph/core/payload_schema.hpp": 8, + "include/morph/core/wire.hpp": 8, + "include/morph/core/backend.hpp": 7, + "include/morph/forms/flows.hpp": 7, + "include/morph/forms/views.hpp": 6, + "include/morph/core/strand.hpp": 4, + "include/morph/core/completion.hpp": 3, + "include/morph/core/logger.hpp": 2, + "include/morph/core/model.hpp": 2, + "include/morph/core/model_key.hpp": 2, + "include/morph/core/observability.hpp": 2, + "include/morph/core/timeout_scheduler.hpp": 2, + "include/morph/forms/instance_constraints.hpp": 2, + "include/morph/core/detail/execute_order_gate.hpp": 1, + "include/morph/core/detail/subscription_registry.hpp": 1, + "include/morph/core/executor.hpp": 1 + }, + "survivors_by_mutator": { + "cxx_replace_scalar_call": 91, + "cxx_init_const": 27, + "cxx_assign_const": 25, + "cxx_add_to_sub": 13, + "cxx_gt_to_ge": 8, + "cxx_rshift_to_lshift": 6, + "cxx_eq_to_ne": 4, + "cxx_ne_to_eq": 4, + "cxx_xor_assign_to_or_assign": 4, + "cxx_xor_to_or": 3, + "cxx_lshift_to_rshift": 3, + "cxx_add_assign_to_sub_assign": 2, + "cxx_ge_to_gt": 2, + "cxx_lt_to_le": 2, + "cxx_sub_assign_to_add_assign": 1, + "cxx_gt_to_le": 1, + "cxx_pre_inc_to_pre_dec": 1, + "cxx_post_inc_to_post_dec": 1 + } } ], "false_positive_finding": { @@ -369,5 +426,47 @@ "wholesale (or the whole mutator, pending an upstream fix) rather than needing a", "per-site allowlist for it." ] + }, + "classification_2026_09_09": { + "_comment": [ + "Partial classification of run[2]'s 199 survivors (morph#453 item A).", + "Recorded so the next pass starts from evidence rather than from zero.", + "UNCLASSIFIED IS THE MAJORITY: 183 of 199 have not been assessed." + ], + "method": "Clustered by (mutator, source shape), then assessed cluster by cluster against what the code actually promises -- not by writing an assertion per mutant, which the issue's own constraint rules out.", + "equivalent": { + "count": 13, + "sites": [ + "core/remote.hpp:157,158,160,162 -- OpaqueIdGenerator::mix, the Feistel round function", + "core/bridge.hpp:121 -- HandlerKey hash-combine", + "core/registry.hpp:54 -- PairKeyHash hash-combine", + "core/payload_schema.hpp:218 -- fingerprint hash accumulation" + ], + "reason": [ + "The Feistel construction (L,R) -> (R, L ^ F(R)) is invertible for ANY round function F, which permute()'s own doc comment states ('this regardless of the round function'). Mutating inside mix() therefore preserves the only property the code claims -- that distinct counters yield distinct ids -- so no honest test can kill these.", + "The hash-combine sites are the same shape: nothing in the contract fixes a hash VALUE, only determinism and adequate distribution. A test that killed these would assert a specific hash constant, pinning an implementation detail -- exactly the 'test with no reason to exist' the issue warns against." + ] + }, + "real_gap_closed": { + "count": 3, + "sites": [ + "core/remote.hpp:142 -- static_cast(counter >> 32)", + "core/remote.hpp:145 -- lo = hi ^ mix(lo, roundKey)", + "core/remote.hpp:148 -- (static_cast(hi) << 32) | lo" + ], + "finding": "The existing bijection test drives counters 1..20000, all of which have a ZERO high half. It therefore passes whether or not permute() uses the top 32 bits at all -- it cannot distinguish a 64-bit permutation from one that ignores half its input.", + "measured": "Simulated against the real round function over 20000 counters that do exercise the high half: '>>' -> '<<' collapses 20000 ids to 1; the Feistel '^' -> '|' collapses them to 1256. Both are catastrophic collisions in opaque model ids, and both are invisible to the existing sample.", + "closed_by": "tests/test_opaque_model_ids.cpp, 'OpaqueIdGenerator is a bijection over counters that exercise the high half'. Kills the :142 and :145 mutants. The :148 mutant still survives -- 'hi >> 32' yields 0 so the id degenerates to its low half, which stays distinct for these inputs; killing it needs a probe on the id's high bits, not a distinctness count." + }, + "unclassified": { + "count": 183, + "largest_clusters": [ + "cxx_replace_scalar_call x92 -- replaces a scalar-returning call with 42; concentrated on .empty()/.size() guards", + "cxx_init_const x27 and cxx_assign_const x25", + "cxx_add_to_sub x13", + "cxx_gt_to_ge x8 -- the boundary family morph#434 confirmed mutates and kills correctly, so these should be read as real" + ], + "note": "remote.hpp (82) and bridge.hpp (33) hold 58% of all survivors. Neither has been assessed." + } } } diff --git a/tests/test_opaque_model_ids.cpp b/tests/test_opaque_model_ids.cpp index 0ede2e8d..f87ebb4b 100644 --- a/tests/test_opaque_model_ids.cpp +++ b/tests/test_opaque_model_ids.cpp @@ -93,6 +93,39 @@ TEST_CASE("OpaqueIdGenerator is a bijection: 20000 counters produce 20000 distin REQUIRE(seen.size() == n); } +// ── morph#453: the bijection above cannot see the high 32 bits ── +// +// Counters 1..20000 all have a zero high half, so the case above passes whether +// or not `permute` uses `counter >> 32` at all -- it cannot distinguish a +// 64-bit permutation from one that silently ignores the top half. That is the +// "would this still pass if the feature did nothing?" failure, and the mutation +// campaign found it: three survivors sit on the Feistel structure +// (`counter >> 32`, `hi ^ mix(...)`, `hi << 32`), none of which the sample can +// reach. +// +// Simulated against the real round function, over 20000 counters that *do* +// exercise the high half: replacing `>>` with `<<` collapses 20000 ids to **1**, +// and replacing the Feistel `^` with `|` collapses them to **1256**. Both are +// catastrophic id collisions -- for opaque model ids, a security-relevant one -- +// and both are invisible to the existing sample. +TEST_CASE("OpaqueIdGenerator is a bijection over counters that exercise the high half", + "[opaque_id][unit][morph453]") { + morph::backend::detail::OpaqueIdGenerator gen; + std::unordered_set seen; + constexpr uint64_t n = 20000; + // Stride by 2^32 so every counter has a distinct, non-zero high word; the + // +7 keeps the low half non-zero too, so neither half is degenerate. + for (uint64_t i = 0; i < n; ++i) { + seen.insert(gen.permute((i << 32U) + 7U)); + } + REQUIRE(seen.size() == n); + + // And mixing both halves: adjacent counters differing only in the high word + // must not map to the same id. + REQUIRE(gen.permute(1U) != gen.permute((1ULL << 32U) + 1U)); + REQUIRE(gen.permute(0U) != gen.permute(1ULL << 32U)); +} + TEST_CASE("OpaqueIdGenerator output is not sequential", "[opaque_id][unit]") { morph::backend::detail::OpaqueIdGenerator gen; const uint64_t first = gen.permute(1); From d1ba985e47a119ebd63c97bdee5b92a25db9d0c7 Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Wed, 9 Sep 2026 23:30:53 +0200 Subject: [PATCH 09/12] Fix the four CI failures, including one that invalidated #499's approach MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit **WASM build — `std::atomic>` is not portable here.** libstdc++ implements the C++20 partial specialisation; libc++ as shipped with emscripten does not, so it falls back to the primary template and hard-errors on `is_trivially_copyable`. Both WASM legs failed. This project builds for WASM, so that type is simply unavailable, and #499's fix had to change shape. The alternatives were a mutex — which costs `token()` and `stopRequested()` their `noexcept`, on a path the Bridge calls per registration — or narrowing the contract. Narrowed it: `token()`, `guard()`, `requestStop()`, `stopRequested()` and the destructor stay mutually concurrent; **`reset()` must be externally synchronised**, which matches the usage it exists for (`onNewQuery() { _callbacks.reset(); }`, the owner thread's supersede verb) and matches what every in-tree caller already does. The within-a-generation guarantee is unchanged. `_state`'s comment records that this is a portability constraint rather than a preference, so the next reader does not "fix" it back. The regression test was asserting the guarantee that just went away, so it now pins the one actually delivered: concurrent `token()`/`stopRequested()` against `requestStop()`, with `reset()` called unraced. Verified TSan-clean (0 reports); the old racing pattern still reports 26, which is now documented caller error rather than a defect. **Header ↔ spec sync** wanted four sub-domains. All four got real content, not filler: journal.md and offline.md document the unreadable-file behaviour these commits introduce (and offline.md the durable-first mutation ordering); rational.md records that `formatRationalDecimal` is no longer one of its listed `INT64_MIN` negation sites and why that path escaped the clamp; forms.md notes where the macro-formatting rationale now lives. **clang-tidy-diff** — nine findings on changed lines, all mine: a non-const `scoped_lock`, two unchecked `operator[]` (now iterator-based), three too-short identifiers, two `#if !defined(X)` that want `#ifndef`, and a missing parenthesis in `+`/`/`. Normalised the `_WIN32` guards across all three test files while there. Suites after: main 22021 assertions, net 983. Tree-wide clang-format clean. Allowlist 23 entries, 0 stale. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01SxkvtHvan2fWKDDaBpqkgv --- docs/spec/forms/forms.md | 5 ++- docs/spec/journal/journal.md | 1 + docs/spec/offline/offline.md | 15 ++++++- docs/spec/util/rational.md | 7 ++++ include/morph/core/callback_scope.hpp | 58 +++++++++++++++------------ include/morph/net/socket_server.hpp | 20 ++++----- tests/net/test_socket_server.cpp | 6 +-- tests/qt/test_qt_websocket.cpp | 2 +- tests/test_action_log_phase2.cpp | 6 +-- tests/test_callback_scope.cpp | 34 ++++++++++------ tests/test_file_offline_queue.cpp | 6 +-- 11 files changed, 101 insertions(+), 59 deletions(-) diff --git a/docs/spec/forms/forms.md b/docs/spec/forms/forms.md index 338befec..2b892020 100644 --- a/docs/spec/forms/forms.md +++ b/docs/spec/forms/forms.md @@ -22,7 +22,10 @@ development" with "flexible when the generated form isn't enough": 2. **Declare to override.** When inference is ambiguous or insufficient (a label, a layout group, a widget choice, a cross-field rule), the user adds a *typed, compile-time* declaration — a `static constexpr` member or a small - registration macro on the action. Never mandatory; absence falls back to a + registration macro on the action (these macros are hand-aligned behind + `// clang-format off`; the rationale lives once in `CONTRIBUTING.md` under + *Formatting/linting*, and each site carries a pointer rather than a copy). + Never mandatory; absence falls back to a sensible convention. 3. **Escape hatch always available.** The schema below is a documented, stable contract (see "Renderer contract"). Anything the generated GUI cannot diff --git a/docs/spec/journal/journal.md b/docs/spec/journal/journal.md index 1edffde5..6c0014fc 100644 --- a/docs/spec/journal/journal.md +++ b/docs/spec/journal/journal.md @@ -1155,6 +1155,7 @@ and `RemoteServer::setLogProvider(LogProvider)`, declared in `remote.hpp`. See | `FileActionLog::seq` is process-local | **Fresh per process, not resumed from disk** | `seq` is a monotonic order key within one process instance, not a cross-restart durable identifier. On-disk order is append order; `entries()` returns in that order regardless of `seq` gaps. | | `FileActionLog` uses C stdio + `fsync` | **`fopen`/`fwrite`/`fflush`/`fsync`** | `fwrite` is buffered; `flush()` calls `fflush` then `fsync` (or `_commit` on Windows) for real durability. POSIX `write`/`fsync` would bypass stdio buffering entirely; C stdio gives buffering by default with explicit flush control. | | `FileActionLog::entries` tolerates a torn trailing line | **Skip + warn on the last line only; re-throw mid-file** | A crash between `append`'s `fwrite` and the next flush can truncate the final line. Skipping it keeps the log readable after a crash; re-throwing on interior damage refuses to silently hide real corruption. | +| An **unreadable** journal is not an empty or torn one | **`repairTornTail()` leaves the file untouched; `entries()` throws** | Both scan with an `ifstream`. When that open fails — or a read errors mid-scan — nothing was read, so `repairTornTail()`'s safety argument ("whatever follows the final newline is by construction an incomplete record") does not hold, and truncating to the scan's `intactEnd` discarded the whole journal while logging it as a successful repair. `entries()` distinguishes *absent* (legitimately empty, which the constructor's dedup rebuild depends on) from *present but unreadable*: returning `{}` for the second silently emptied the `idempotencyKey` dedup set `OutboxRelay` relies on. See morph#493. | | `InMemoryActionLog`/`FileActionLog` dedup on `idempotencyKey` | **Non-empty key only; `SessionLog` excluded** | Makes both safe default choices for `OutboxRelay::sink` without changing behavior for callers that never set the key (empty key never dedups). `SessionLog` is excluded because its contract is full fidelity — nothing coalesced or dropped. | | Payload evolution is **detected**, not prevented | **Fingerprint stamped per entry; `replay()` refuses a mismatch** | The additive-only [data-at-rest contract](#data-at-rest-contract) was already published and already unenforced. Strict decode would reject the additive change the contract permits; a lint sees one commit while a journal outlives the deployment that wrote it. A derived fingerprint cannot be forgotten the way a hand-maintained version number can. | | A mismatch throws rather than warning | **`SchemaMismatchError` out of `replay()`** | The defect is confident wrongness. A suspect holder plus a log line reproduces it with extra steps, and puts the burden of noticing on the code path that demonstrably did not notice. | diff --git a/docs/spec/offline/offline.md b/docs/spec/offline/offline.md index 81169d64..52cd3b29 100644 --- a/docs/spec/offline/offline.md +++ b/docs/spec/offline/offline.md @@ -370,7 +370,20 @@ id on the second restart — enqueue 1 and 2, `markDone(2)`, restart (compacts t just id 1), restart again, and the next `enqueue()` reissued id 2, the id of a completed and acknowledged item. Mutations also raise rather than swallow I/O failures: a short write or a failed `fflush`/`fsync` throws, since every -mutation is documented as a committed transaction by the time the call returns. A +mutation is documented as a committed transaction by the time the call returns. +They are also ordered **durable-first**: `markDone()` appends the tombstone +before erasing from `_items`, and `setAttempts()` writes before updating memory. +The reverse order meant a throwing append left the item gone from memory with no +tombstone on disk, so this process never replayed it and a restart resurrected +and re-applied it; durable-first fails the other way, replaying once too often at +worst, which `idempotencyKey` exists to absorb (morph#494). + +An **unreadable** queue file is not an empty queue. `load()` reads with its own +`ifstream`, and the constructor calls `compact()` immediately after — which +rewrites the file from whatever `load()` produced. A failed open or a mid-file +read error therefore committed an empty set over the real backlog, with the +constructor returning normally and the queue reporting no pending work. Both now +throw, so `compact()` cannot run on a load that did not succeed (morph#494). A keyed `enqueue`'s dedup is a linear scan over pending items — fine at modest queue depths; `SqliteOfflineQueue` is the index-backed alternative for high-volume keyed enqueues. Not safe for multiple processes to open the same diff --git a/docs/spec/util/rational.md b/docs/spec/util/rational.md index 8dc633d8..edc284a0 100644 --- a/docs/spec/util/rational.md +++ b/docs/spec/util/rational.md @@ -188,6 +188,13 @@ site when that exact value reaches it: straight into the canonicalising constructor. - **`reciprocal`** — negates the numerator in the `numerator < 0` branch; `INT64_MIN` there overflows. +- **Rendering** (`morph::units::detail::formatRationalDecimal`) — *was* one of + these and no longer is. It negated the numerator in `int64_t` under a comment + claiming it widened first; UBSan confirmed the report. It now goes through + `detail::absU64`, which negates in unsigned arithmetic. This mattered because + the whole-integer `Rational{value, DecimalPlaces{n}}` constructor does not + canonicalise, so the clamp never ran on that path and `numerator` is public + (morph#496). - **`canonicalise`** — **no longer one of these.** It clamps an `INT64_MIN` numerator to `-INT64_MAX` (with an `error`-level log, `reportClamp`) *before* any sign flip, and computes the gcd through `detail::absU64`, which negates in diff --git a/include/morph/core/callback_scope.hpp b/include/morph/core/callback_scope.hpp index 2e7e8406..d74612b5 100644 --- a/include/morph/core/callback_scope.hpp +++ b/include/morph/core/callback_scope.hpp @@ -192,10 +192,21 @@ class CallbackToken { /// construction. /// /// @par Thread safety -/// All members are safe to call concurrently from any thread. `requestStop()` -/// and `stopRequested()` are lock-free atomic operations; `reset()` publishes a -/// fresh generation and retires the previous one (stopping it first, so a token -/// holder that raced the swap and pinned the old state still observes refusal). +/// `token()`, `guard()`, `requestStop()`, `stopRequested()` and the destructor +/// are safe to call concurrently from any thread; `requestStop()` and +/// `stopRequested()` are lock-free atomic operations on the current generation. +/// +/// **`reset()` is the exception and must be externally synchronised** against +/// the others — call it from the thread that owns the scope. It replaces the +/// `_state` handle itself, and reading a `shared_ptr` while another thread +/// assigns it is a data race on the handle: the control block's refcount is +/// atomic, the pointer object is not. This is a narrower promise than this +/// paragraph used to make, and the reason is a portability constraint, not +/// taste — see `_state`'s own comment. In the usage `reset()` exists for +/// (`void onNewQuery() { _callbacks.reset(); }`, the supersede verb) the owning +/// thread is the caller anyway. Within a single generation, the old guarantee +/// holds unchanged: `reset()` stops the outgoing generation before releasing +/// it, so a token holder that pinned it still observes refusal (morph#499). /// /// Identity, not a value: neither copyable nor movable. A moved-from scope would /// have to either strand or silently retarget tokens already captured in flight; @@ -222,13 +233,11 @@ class CallbackScope { /// report `CallbackStatus::Stopped` rather than `Expired`. Undone only by /// `reset()`, which starts a new generation. void requestStop() const noexcept { - // One load, then act on that generation: re-reading `_state` between - // the null test and the store would be a second, possibly different - // generation. It is never null (the sole constructor make_shared's it, - // the class is non-copyable and non-movable, and `reset()` always - // assigns a fresh value), so the former `!= nullptr` guard was an - // unreachable branch and is gone -- morph#499. - _state.load(std::memory_order_acquire)->stopped.store(true, std::memory_order_release); + // `_state` is never null: the sole constructor make_shared's it, the + // class is non-copyable and non-movable, and `reset()` always assigns a + // fresh value. The former `!= nullptr` guard was an unreachable branch + // and is gone (morph#499). + _state->stopped.store(true, std::memory_order_release); } /// @brief Retires every token issued so far and starts a fresh, live generation. @@ -241,20 +250,16 @@ class CallbackScope { void reset() { auto fresh = std::make_shared(); requestStop(); - _state.store(std::move(fresh), std::memory_order_release); + _state = std::move(fresh); } /// @brief Whether this generation has been stopped. /// @return `true` after `requestStop()`, until the next `reset()`. - [[nodiscard]] bool stopRequested() const noexcept { - return _state.load(std::memory_order_acquire)->stopped.load(std::memory_order_acquire); - } + [[nodiscard]] bool stopRequested() const noexcept { return _state->stopped.load(std::memory_order_acquire); } /// @brief Issues a weak token for the current generation. /// @return A `CallbackToken` observing this scope; keeps nothing alive. - [[nodiscard]] CallbackToken token() const noexcept { - return CallbackToken{_state.load(std::memory_order_acquire)}; - } + [[nodiscard]] CallbackToken token() const noexcept { return CallbackToken{_state}; } /// @brief Wraps @p fn so it runs only while this scope is alive and un-stopped. /// @@ -273,14 +278,15 @@ class CallbackScope { } private: - /// Atomic because the class documents *every* member as safe to call - /// concurrently, and `reset()` writes this while `token()`, `guard()`, - /// `requestStop()` and `stopRequested()` read it. A plain `shared_ptr` made - /// that a data race on the pointer object itself -- the control block's - /// atomic refcount protects the pointee, not the handle (morph#499). The - /// guarantee is deliberate, not incidental: `reset()`'s own doc turns on a - /// token holder "that pinned it while racing this call". - std::atomic> _state; + /// Written by `reset()` and read by every other member. **Not** atomic, and + /// that is a constraint rather than a preference: `std::atomic>` + /// is a C++20 library feature libstdc++ provides and libc++ (as shipped with + /// emscripten, which this project builds for) does not -- it falls back to + /// the primary template and hard-errors on `is_trivially_copyable`. The + /// alternative, a mutex, would cost `token()`/`stopRequested()` their + /// `noexcept`. So the concurrency contract is narrowed instead; see this + /// class's own Thread safety paragraph (morph#499). + std::shared_ptr _state; }; } // namespace morph::async diff --git a/include/morph/net/socket_server.hpp b/include/morph/net/socket_server.hpp index 9a82c691..7964b634 100644 --- a/include/morph/net/socket_server.hpp +++ b/include/morph/net/socket_server.hpp @@ -285,19 +285,21 @@ class SocketServer { void reapFinishedClients() { std::vector doneThreads; { - std::scoped_lock lock{_clientsMtx}; + std::scoped_lock const lock{_clientsMtx}; for (std::size_t i = _clients.size(); i-- > 0;) { - if (!_clients[i]->finished.load(std::memory_order_acquire)) { + auto const clientIt = _clients.begin() + static_cast(i); + auto const threadIt = _clientThreads.begin() + static_cast(i); + if (!(*clientIt)->finished.load(std::memory_order_acquire)) { continue; } - doneThreads.push_back(std::move(_clientThreads[i])); - _clientThreads.erase(_clientThreads.begin() + static_cast(i)); - _clients.erase(_clients.begin() + static_cast(i)); + doneThreads.push_back(std::move(*threadIt)); + _clientThreads.erase(threadIt); + _clients.erase(clientIt); } } - for (auto& t : doneThreads) { - if (t.joinable()) { - t.join(); + for (auto& done : doneThreads) { + if (done.joinable()) { + done.join(); } } } @@ -309,7 +311,7 @@ class SocketServer { // it is destroyed last: the flag must not go up until the connection's // models have actually been reclaimed. morph#498. struct FinishedFlag { - explicit FinishedFlag(std::atomic& f MORPH_LIFETIMEBOUND) : flag{f} {} + explicit FinishedFlag(std::atomic& target MORPH_LIFETIMEBOUND) : flag{target} {} ~FinishedFlag() { flag.store(true, std::memory_order_release); } FinishedFlag(const FinishedFlag&) = delete; FinishedFlag& operator=(const FinishedFlag&) = delete; diff --git a/tests/net/test_socket_server.cpp b/tests/net/test_socket_server.cpp index 3f96d5b8..02e87243 100644 --- a/tests/net/test_socket_server.cpp +++ b/tests/net/test_socket_server.cpp @@ -1351,7 +1351,7 @@ TEST_CASE("SocketServer: teardown racing a connecting client still finishes prom // test samples the fd count *while the server is still running*: a test that // opened N connections and then destroyed the server would have passed before // the fix and proved nothing (invariant 7). -#if !defined(_WIN32) +#ifndef _WIN32 namespace { std::size_t openFdCount() { std::size_t count = 0; @@ -1407,6 +1407,6 @@ TEST_CASE("SocketServer: a finished connection's fd and thread are reclaimed bef INFO("baseline=" << baseline << " after=" << after << " rounds=" << kRounds); // Allow generous slack for the two still-unreaped connections and any // transient fds; what must NOT happen is growth proportional to kRounds. - CHECK(after < baseline + static_cast(kRounds) / 2); + CHECK(after < baseline + (static_cast(kRounds) / 2)); } -#endif // !defined(_WIN32) +#endif // _WIN32 diff --git a/tests/qt/test_qt_websocket.cpp b/tests/qt/test_qt_websocket.cpp index f8573618..779f239b 100644 --- a/tests/qt/test_qt_websocket.cpp +++ b/tests/qt/test_qt_websocket.cpp @@ -2358,7 +2358,7 @@ TEST_CASE("morph::qt::QtWebSocketBackend: the async control envelopes carry the morph::qt::QtWebSocketServer wsServer{*server, 0}; REQUIRE(wsServer.listen()); - QUrl url{QString("ws://127.0.0.1:%1").arg(wsServer.port())}; + QUrl const url{QString("ws://127.0.0.1:%1").arg(wsServer.port())}; morph::qt::QtWebSocketBackend backend{url, morph::model::detail::defaultDispatcher(), morph::model::detail::defaultRegistry(), std::nullopt, morph::qt::QtWebSocketBackend::Config{.asyncRegistrationEnabled = true}}; diff --git a/tests/test_action_log_phase2.cpp b/tests/test_action_log_phase2.cpp index 52317ca3..71fa1e9c 100644 --- a/tests/test_action_log_phase2.cpp +++ b/tests/test_action_log_phase2.cpp @@ -23,7 +23,7 @@ #include #include #include -#if !defined(_WIN32) +#ifndef _WIN32 #include // geteuid, for the permission-based fault-injection cases below #endif #include @@ -855,7 +855,7 @@ TEST_CASE("FileActionLog: a torn trailing record whose resize_file() fails is lo // probe/open window, and Windows has no portable equivalent (std::filesystem // maps permissions onto the read-only attribute alone). Skipped for a root // euid, which ignores the permission bits entirely. -#if !defined(_WIN32) +#ifndef _WIN32 TEST_CASE("FileActionLog: an unreadable journal is left intact, not truncated as a torn record", "[action_log][phase2][file][fault-injection]") { if (::geteuid() == 0) { @@ -892,4 +892,4 @@ TEST_CASE("FileActionLog: an unreadable journal is left intact, not truncated as FileActionLog reopened{tmp.path}; REQUIRE(reopened.entries().size() == 3); } -#endif // !defined(_WIN32) +#endif // _WIN32 diff --git a/tests/test_callback_scope.cpp b/tests/test_callback_scope.cpp index affa2e28..513198be 100644 --- a/tests/test_callback_scope.cpp +++ b/tests/test_callback_scope.cpp @@ -574,18 +574,21 @@ TEST_CASE("CallbackScope: destroying the scope under a concurrent dispatch loop } } -// ── morph#499: reset() races every other member ── +// ── morph#499: what CallbackScope's concurrency contract actually covers ── // -// `_state` was a plain shared_ptr written by reset() and read by token(), -// guard(), requestStop() and stopRequested(). Concurrent read/write of the same -// shared_ptr object is a data race -- the control block's atomic refcount -// protects the pointee, not the handle. The class documents *every* member as -// concurrently safe, and reset()'s own doc turns on a token holder "that pinned -// it while racing this call", so the guarantee is deliberate. Now atomic. +// The class used to document *every* member as concurrently safe. It is not: +// `reset()` replaces the `_state` handle, and reading a shared_ptr while another +// thread assigns it races on the handle itself (the control block's refcount is +// atomic; the pointer object is not). Making `_state` a +// `std::atomic>` fixes it on libstdc++ and does not +// compile on libc++/emscripten, which this project targets -- so the contract +// was narrowed instead: `reset()` must be externally synchronised, everything +// else stays concurrent. // -// This case exists to be run under ThreadSanitizer; it is deliberately -// assertion-light, because what it proves is the absence of a report. -TEST_CASE("CallbackScope: reset() concurrent with token()/stopRequested() is race-free", +// This case pins the half that *is* promised, and is meant to be run under +// ThreadSanitizer -- deliberately assertion-light, because what it proves is the +// absence of a report. +TEST_CASE("CallbackScope: token()/requestStop()/stopRequested() are concurrent among themselves", "[callback_scope][thread][morph499]") { morph::async::CallbackScope scope; std::atomic stop{false}; @@ -599,13 +602,20 @@ TEST_CASE("CallbackScope: reset() concurrent with token()/stopRequested() is rac observed.fetch_add(1, std::memory_order_relaxed); } }}; + // requestStop() from this thread while the reader reads: both act on the + // *current* generation, which is what the narrowed contract still covers. for (int i = 0; i < 2000; ++i) { - scope.reset(); + scope.requestStop(); } stop.store(true, std::memory_order_release); reader.join(); REQUIRE(observed.load() > 0); - // A fresh generation is live after the last reset(). + REQUIRE(scope.stopRequested()); + + // reset() is the externally-synchronised member: called here with no reader + // running, which is the documented usage (the owner thread's supersede verb). + scope.reset(); REQUIRE(scope.token().active()); + REQUIRE_FALSE(scope.stopRequested()); } diff --git a/tests/test_file_offline_queue.cpp b/tests/test_file_offline_queue.cpp index 38805b99..92e3b05d 100644 --- a/tests/test_file_offline_queue.cpp +++ b/tests/test_file_offline_queue.cpp @@ -13,7 +13,7 @@ #include #include -#if !defined(_WIN32) +#ifndef _WIN32 #include // geteuid, for the permission-based fault-injection case below #endif #include "offline_queue_conformance.hpp" @@ -706,7 +706,7 @@ TEST_CASE("morph::offline::FileOfflineQueue: the idempotency-key contract surviv // no fault injection at all -- load() bypasses the FileIoOps seam entirely. // // POSIX-only and non-root, for the same reasons as the FileActionLog case. -#if !defined(_WIN32) +#ifndef _WIN32 TEST_CASE("FileOfflineQueue: an unreadable queue file is not silently compacted away", "[offline][file][fault-injection]") { if (::geteuid() == 0) { @@ -736,4 +736,4 @@ TEST_CASE("FileOfflineQueue: an unreadable queue file is not silently compacted REQUIRE(reopened.drain().size() == 3); std::filesystem::remove(path); } -#endif // !defined(_WIN32) +#endif // _WIN32 From e3947ac0f84cdaf9930fe15903d361a6d1fe2b3b Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Thu, 10 Sep 2026 02:30:56 +0200 Subject: [PATCH 10/12] Revert #506's fix -- it traded a hypothetical deadlock for a real data race CI's clang-tsan leg failed on "SocketBackend: action result delivered via then", and the report points straight at the change I made for morph#506: #0 TcpSocket::closeNow() tcp_socket.hpp:434 #1 TcpSocket::operator=(TcpSocket&&) tcp_socket.hpp:82 #2 SocketBackend::onDisconnected() socket_backend.hpp:494 vs #0 TcpSocket::valid() tcp_socket.hpp:395 #1 SocketBackend::~SocketBackend() socket_backend.hpp:100 My reasoning was wrong in a specific way worth recording. I argued the lock was unnecessary because `shutdownBoth()` is documented safe from any thread. That is true, and it is not what `_socketMtx` was protecting at this site: `onDisconnected()` **reassigns** `_socket` (`_socket = TcpSocket{}`, a move-assign that closes the old fd), so an unlocked `_socket.valid()` in the destructor races the I/O thread replacing the object, not merely using it. `SocketServer::close()`, which I cited as precedent, does not have this shape -- it never reassigns the socket it shuts down. Measured both ways locally under TSan, rather than taking CI's word for it: **25 race reports without the lock, 0 with it.** So the lock is restored and #506 stays open. Its underlying hazard is still real -- `sendFrame` holds `_socketMtx` across a blocking, un-timed `sendAll`, so a peer that stops reading can park the destructor -- but closing it needs a way to reach the fd without the mutex (an atomic fd shadowing `_socket`, with its own fd-reuse story), not simply dropping the lock. Both the code comment and the issue now say that. Also fixes the flaky test I added for #499, which failed on clang-debug, Windows cl-debug, clang-asan and Valgrind. Two separate defects in it, both mine: - `observed == 0`: a fixed 2000-iteration loop of relaxed stores finishes in microseconds, so the reader thread could still be starting when `stop` went up. Now driven by the reader's own progress against a deadline, and asserts the interleaving count so a clean TSan run means something rather than reporting success having measured nothing. - `stopRequested() == false`: the replacement used `while`, whose condition is checked *first* -- if the reader raced past the threshold before the first iteration, `requestStop()` was never called at all. `do`/`while` fixes it. This one only appeared under heavy oversubscription. Stress-verified: 192 runs at 48x oversubscription, 0 failures (the first fix failed 8 of 72 at 24x). Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01SxkvtHvan2fWKDDaBpqkgv --- include/morph/net/socket_backend.hpp | 31 ++++++++++++++++----------- scripts/branch_partial_allowlist.json | 8 +++---- tests/test_callback_scope.cpp | 24 ++++++++++++++++++--- 3 files changed, 43 insertions(+), 20 deletions(-) diff --git a/include/morph/net/socket_backend.hpp b/include/morph/net/socket_backend.hpp index 550d815c..dc133ab2 100644 --- a/include/morph/net/socket_backend.hpp +++ b/include/morph/net/socket_backend.hpp @@ -86,19 +86,24 @@ class SocketBackend : public ::morph::backend::detail::IBackend { /// implementation. See `docs/spec/core/backend.md`'s `morph::net` section. ~SocketBackend() override { _shuttingDown.store(true); - // Deliberately NOT under `_socketMtx` -- this is the same trap - // `SocketServer::close()` documents avoiding, reached from the client - // side. `sendFrame` holds `_socketMtx` across `_socket.sendAll()`, which - // loops on a blocking `::send` with no timeout, so a thread stalled - // against a peer that has stopped reading holds that lock indefinitely. - // Waiting for it here would block the destructor on exactly the - // condition that only `shutdownBoth()` can clear -- and `shutdownBoth()` - // is documented safe from any thread (detail/tcp_socket.hpp), which is - // what makes taking the lock unnecessary as well as harmful. The I/O - // thread's own Pong/Close echo in `drainFrames` reaches `sendFrame` too, - // so the stuck holder need not even be an application thread. morph#506. - if (_socket.valid()) { - _socket.shutdownBoth(); + // Under `_socketMtx`, and it has to be -- see morph#506, which proposed + // dropping it and was proved wrong by ThreadSanitizer. `shutdownBoth()` + // is indeed safe to call from any thread, but that is not what the lock + // is protecting here: `onDisconnected()` *reassigns* `_socket` + // (`_socket = TcpSocket{}`, a move-assign that closes the old fd), so an + // unlocked `_socket.valid()` here races the I/O thread replacing the + // object out from under it. + // + // The hazard #506 describes is real and remains open: `sendFrame` holds + // this mutex across a blocking, un-timed `sendAll`, so a peer that stops + // reading can park the destructor here. Closing that needs a way to + // reach the fd without the mutex (an atomic fd shadowing `_socket`, with + // its own fd-reuse story), not simply removing the lock. + { + std::scoped_lock const lock{_socketMtx}; + if (_socket.valid()) { + _socket.shutdownBoth(); + } } _reconnectCv.notify_all(); if (_ioThread.joinable()) { diff --git a/scripts/branch_partial_allowlist.json b/scripts/branch_partial_allowlist.json index 596efbeb..b814da1d 100644 --- a/scripts/branch_partial_allowlist.json +++ b/scripts/branch_partial_allowlist.json @@ -140,19 +140,19 @@ }, { "file": "include/morph/net/socket_backend.hpp", - "line": 104, + "line": 109, "source": "if (_ioThread.joinable()) {", "reason": "Unreachable by construction (net audit, `socket_backend.hpp` extra finding #5). `_ioThread` is started unconditionally in the constructor and has exactly one join site: this line, in the destructor. Nothing else in the file resets, joins, or detaches it, so at destructor time it is always joinable." }, { "file": "include/morph/net/socket_backend.hpp", - "line": 116, + "line": 121, "source": "if (_handlerThread.joinable()) {", - "reason": "Unreachable by construction, same shape as `_ioThread`'s line 104 above (net audit, `socket_backend.hpp` extra finding #6). `_handlerThread` is started unconditionally in the constructor and has exactly one join site: this line, in the destructor. Nothing else resets, joins, or detaches it, so at destructor time it is always joinable." + "reason": "Unreachable by construction, same shape as `_ioThread`'s line 109 above (net audit, `socket_backend.hpp` extra finding #6). `_handlerThread` is started unconditionally in the constructor and has exactly one join site: this line, in the destructor. Nothing else resets, joins, or detaches it, so at destructor time it is always joinable." }, { "file": "include/morph/net/socket_backend.hpp", - "line": 555, + "line": 560, "source": "default:", "reason": "Unreachable except via the adjacent `Error` case it deliberately shares a body with (net audit, `socket_backend.hpp` extra finding #7). `detail::ExecuteReplyKind` is a closed 3-value enum (`Value`/`Timeout`/`Error`), all three handled explicitly above this label; `default:` exists only to satisfy this project's `-Wswitch-default`, per the source's own inline comment directly below this line. Reaching it via any value other than through the `Error` case falling through would require an out-of-range `static_cast` producing a value outside the enum's domain -- undefined behavior, not a legitimate test target." } diff --git a/tests/test_callback_scope.cpp b/tests/test_callback_scope.cpp index 513198be..8b8301c1 100644 --- a/tests/test_callback_scope.cpp +++ b/tests/test_callback_scope.cpp @@ -13,6 +13,7 @@ #include #include +#include #include #include #include @@ -604,13 +605,30 @@ TEST_CASE("CallbackScope: token()/requestStop()/stopRequested() are concurrent a }}; // requestStop() from this thread while the reader reads: both act on the // *current* generation, which is what the narrowed contract still covers. - for (int i = 0; i < 2000; ++i) { + // + // Driven by the reader's own progress rather than a fixed iteration count. + // A fixed count races thread-start latency -- 2000 relaxed stores finish in + // microseconds, so on a loaded machine the reader could still be starting + // when `stop` went up, leaving `observed` at 0. That is what happened on + // CI's clang-debug leg while this test passed locally. + // do/while, not while: the condition is checked *after* the first call, so + // this cannot execute zero times. A plain `while` did, when the reader + // raced ahead of the first iteration and `observed` was already past the + // threshold -- leaving `requestStop()` never called and `stopRequested()` + // false. That showed up only under heavy oversubscription. + constexpr int kMinInterleavings = 100; + auto const deadline = std::chrono::steady_clock::now() + std::chrono::seconds{10}; + do { scope.requestStop(); - } + } while (observed.load(std::memory_order_relaxed) < kMinInterleavings && + std::chrono::steady_clock::now() < deadline); stop.store(true, std::memory_order_release); reader.join(); - REQUIRE(observed.load() > 0); + // Proves the two threads genuinely interleaved, so a clean TSan run means + // something. Without this the case could report success having measured + // nothing at all. + REQUIRE(observed.load() >= kMinInterleavings); REQUIRE(scope.stopRequested()); // reset() is the externally-synchronised member: called here with no reader From 8cf7701075232900d2a816838c8b8f27e01033a1 Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Thu, 10 Sep 2026 07:32:47 +0200 Subject: [PATCH 11/12] Clear the last three clang-tidy findings - `test_callback_scope.cpp` -- my own `do`/`while` (the fix for the zero-iteration bug) trips `cppcoreguidelines-avoid-do-while`. Same guarantee without it: one unconditional `requestStop()`, then the bounded progress loop. Re-stressed at 48x oversubscription, 0 failures. - `test_opaque_model_ids.cpp`, `test_file_offline_queue.cpp` -- two locals that can be `const` (`misc-const-correctness`). Checked the whole branch locally this time rather than discovering these one CI run at a time: ran clang-tidy over every changed .cpp against the repo's .clang-tidy and compile_commands.json, then filtered its 428 findings down to the 888 lines this branch actually touches. Zero remain, Qt and net sources included. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01SxkvtHvan2fWKDDaBpqkgv --- tests/test_callback_scope.cpp | 16 ++++++++-------- tests/test_file_offline_queue.cpp | 2 +- tests/test_opaque_model_ids.cpp | 2 +- 3 files changed, 10 insertions(+), 10 deletions(-) diff --git a/tests/test_callback_scope.cpp b/tests/test_callback_scope.cpp index 8b8301c1..1ac39b70 100644 --- a/tests/test_callback_scope.cpp +++ b/tests/test_callback_scope.cpp @@ -611,17 +611,17 @@ TEST_CASE("CallbackScope: token()/requestStop()/stopRequested() are concurrent a // microseconds, so on a loaded machine the reader could still be starting // when `stop` went up, leaving `observed` at 0. That is what happened on // CI's clang-debug leg while this test passed locally. - // do/while, not while: the condition is checked *after* the first call, so - // this cannot execute zero times. A plain `while` did, when the reader - // raced ahead of the first iteration and `observed` was already past the - // threshold -- leaving `requestStop()` never called and `stopRequested()` - // false. That showed up only under heavy oversubscription. + // The first call is unconditional, deliberately: a bare `while` whose + // condition is tested first executed zero times when the reader raced ahead + // of it, leaving `requestStop()` never called and `stopRequested()` false. + // That only appeared under heavy oversubscription. constexpr int kMinInterleavings = 100; auto const deadline = std::chrono::steady_clock::now() + std::chrono::seconds{10}; - do { + scope.requestStop(); + while (observed.load(std::memory_order_relaxed) < kMinInterleavings && + std::chrono::steady_clock::now() < deadline) { scope.requestStop(); - } while (observed.load(std::memory_order_relaxed) < kMinInterleavings && - std::chrono::steady_clock::now() < deadline); + } stop.store(true, std::memory_order_release); reader.join(); diff --git a/tests/test_file_offline_queue.cpp b/tests/test_file_offline_queue.cpp index 92e3b05d..90367ec2 100644 --- a/tests/test_file_offline_queue.cpp +++ b/tests/test_file_offline_queue.cpp @@ -732,7 +732,7 @@ TEST_CASE("FileOfflineQueue: an unreadable queue file is not silently compacted std::filesystem::permissions(path, std::filesystem::perms::owner_all); REQUIRE(std::filesystem::file_size(path) == sizeBefore); - morph::offline::FileOfflineQueue reopened{path}; + morph::offline::FileOfflineQueue const reopened{path}; REQUIRE(reopened.drain().size() == 3); std::filesystem::remove(path); } diff --git a/tests/test_opaque_model_ids.cpp b/tests/test_opaque_model_ids.cpp index f87ebb4b..9c8858e4 100644 --- a/tests/test_opaque_model_ids.cpp +++ b/tests/test_opaque_model_ids.cpp @@ -110,7 +110,7 @@ TEST_CASE("OpaqueIdGenerator is a bijection: 20000 counters produce 20000 distin // and both are invisible to the existing sample. TEST_CASE("OpaqueIdGenerator is a bijection over counters that exercise the high half", "[opaque_id][unit][morph453]") { - morph::backend::detail::OpaqueIdGenerator gen; + morph::backend::detail::OpaqueIdGenerator const gen; std::unordered_set seen; constexpr uint64_t n = 20000; // Stride by 2^32 so every counter has a distinct, non-zero high word; the From 05a7e8ea7a224080b104c5b55d8bc490dddcb220 Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Thu, 10 Sep 2026 09:40:34 +0200 Subject: [PATCH 12/12] Make the #499 concurrency test deterministic under a serialising scheduler Valgrind was the last failing leg, and it was this test again -- `observed == 0` after a full ten-second deadline. Not a memory finding: all three memcheck runs reported 0 errors and 0 bytes lost, and the step failed only because a Catch2 case did. Cause: Valgrind serialises threads, switching at syscalls and similar points. My main thread spun on `requestStop()`, which is a relaxed atomic store and offers the scheduler nothing to switch on, so the reader never ran at all. The 10s deadline expired with the two threads never having interleaved. Two changes, both about making the interleaving *happen* rather than hoping for it: - A `started` handshake, so the main thread does not begin until the reader is provably inside its loop. - `std::this_thread::yield()` in both loops. `sched_yield(2)` is a real syscall, which is exactly the switch point a serialising scheduler needs. Deadline raised to 30s, since under Valgrind everything is ~20-50x slower. This is the third distinct failure mode in one test -- thread-start latency, a zero-iteration `while`, and now scheduler starvation -- so it is verified across every environment that has bitten it rather than just the one that last failed: Valgrind 5/5 clean, 192 runs at 48x oversubscription with 0 failures, TSan clean with 0 race reports, clang-tidy clean on the new lines, full suite 22021 assertions. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01SxkvtHvan2fWKDDaBpqkgv --- tests/test_callback_scope.cpp | 36 +++++++++++++++++++++++------------ 1 file changed, 24 insertions(+), 12 deletions(-) diff --git a/tests/test_callback_scope.cpp b/tests/test_callback_scope.cpp index 1ac39b70..6f50ddd7 100644 --- a/tests/test_callback_scope.cpp +++ b/tests/test_callback_scope.cpp @@ -595,32 +595,44 @@ TEST_CASE("CallbackScope: token()/requestStop()/stopRequested() are concurrent a std::atomic stop{false}; std::atomic observed{0}; + // `started` is a handshake, not decoration: the main thread must not begin + // its side until the reader is provably inside its loop. Three separate CI + // failures came from getting this wrong -- a fixed iteration count that + // finished before the reader was scheduled, a `while` whose condition was + // tested first and so ran zero times, and finally Valgrind, which serialises + // threads and starved the reader for a full ten-second deadline while the + // main thread spun on atomics that never yield. + std::atomic started{false}; std::thread reader{[&] { + started.store(true, std::memory_order_release); while (!stop.load(std::memory_order_acquire)) { auto tok = scope.token(); (void)tok.active(); (void)scope.stopRequested(); observed.fetch_add(1, std::memory_order_relaxed); + // Yields inside the loop as well, so neither side can monopolise a + // serialised scheduler. + std::this_thread::yield(); } }}; + + constexpr int kMinInterleavings = 100; + auto const deadline = std::chrono::steady_clock::now() + std::chrono::seconds{30}; + while (!started.load(std::memory_order_acquire) && std::chrono::steady_clock::now() < deadline) { + std::this_thread::yield(); + } + // requestStop() from this thread while the reader reads: both act on the // *current* generation, which is what the narrowed contract still covers. - // - // Driven by the reader's own progress rather than a fixed iteration count. - // A fixed count races thread-start latency -- 2000 relaxed stores finish in - // microseconds, so on a loaded machine the reader could still be starting - // when `stop` went up, leaving `observed` at 0. That is what happened on - // CI's clang-debug leg while this test passed locally. - // The first call is unconditional, deliberately: a bare `while` whose - // condition is tested first executed zero times when the reader raced ahead - // of it, leaving `requestStop()` never called and `stopRequested()` false. - // That only appeared under heavy oversubscription. - constexpr int kMinInterleavings = 100; - auto const deadline = std::chrono::steady_clock::now() + std::chrono::seconds{10}; + // The first call is unconditional so this cannot run zero times. scope.requestStop(); while (observed.load(std::memory_order_relaxed) < kMinInterleavings && std::chrono::steady_clock::now() < deadline) { scope.requestStop(); + // sched_yield(2) is a real syscall, which is what gives Valgrind's + // serialising scheduler a point at which to switch to the reader. A + // pure atomic-store loop offers it none. + std::this_thread::yield(); } stop.store(true, std::memory_order_release); reader.join();