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/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/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/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/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/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/bridge.hpp b/include/morph/core/bridge.hpp index 98cf593f..7eac63cf 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,10 +1970,17 @@ 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 - // 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). + // 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). std::mutex _attachMtx; mutable std::mutex _sessionMtx; ::morph::session::Context _defaultSession; diff --git a/include/morph/core/callback_scope.hpp b/include/morph/core/callback_scope.hpp index f2c0d315..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; @@ -221,10 +232,12 @@ 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 { + // `_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. @@ -242,9 +255,7 @@ class CallbackScope { /// @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); - } + [[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. @@ -267,6 +278,14 @@ class CallbackScope { } private: + /// 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; }; 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/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/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/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/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/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/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/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_backend.hpp b/include/morph/net/socket_backend.hpp index 2ed7b188..dc133ab2 100644 --- a/include/morph/net/socket_backend.hpp +++ b/include/morph/net/socket_backend.hpp @@ -86,8 +86,21 @@ class SocketBackend : public ::morph::backend::detail::IBackend { /// implementation. See `docs/spec/core/backend.md`'s `morph::net` section. ~SocketBackend() override { _shuttingDown.store(true); + // 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 lock{_socketMtx}; + std::scoped_lock const lock{_socketMtx}; if (_socket.valid()) { _socket.shutdownBoth(); } diff --git a/include/morph/net/socket_server.hpp b/include/morph/net/socket_server.hpp index 26d6af14..7964b634 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 @@ -201,6 +202,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 +257,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 +273,54 @@ 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 const lock{_clientsMtx}; + for (std::size_t i = _clients.size(); i-- > 0;) { + 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(*threadIt)); + _clientThreads.erase(threadIt); + _clients.erase(clientIt); + } + } + for (auto& done : doneThreads) { + if (done.joinable()) { + done.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& target MORPH_LIFETIMEBOUND) : flag{target} {} + ~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 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/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/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/include/morph/util/quantity.hpp b/include/morph/util/quantity.hpp index 58fd4cb7..be671ea2 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); @@ -545,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>(); \ @@ -567,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 2a6e3739..b814da1d 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." }, @@ -134,37 +122,37 @@ }, { "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": 207, + "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": 249, + "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." }, { "file": "include/morph/net/socket_backend.hpp", - "line": 96, + "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": 108, + "line": 121, "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 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": 547, + "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/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/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/net/test_socket_server.cpp b/tests/net/test_socket_server.cpp index 4c64965a..02e87243 100644 --- a/tests/net/test_socket_server.cpp +++ b/tests/net/test_socket_server.cpp @@ -8,6 +8,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). +#ifndef _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 // _WIN32 diff --git a/tests/qt/test_qt_websocket.cpp b/tests/qt/test_qt_websocket.cpp index 593bfe40..779f239b 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 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}}; + 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"); +} diff --git a/tests/test_action_log_phase2.cpp b/tests/test_action_log_phase2.cpp index 257f6c92..71fa1e9c 100644 --- a/tests/test_action_log_phase2.cpp +++ b/tests/test_action_log_phase2.cpp @@ -23,6 +23,9 @@ #include #include #include +#ifndef _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. +#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) { + 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 // _WIN32 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); +} diff --git a/tests/test_callback_scope.cpp b/tests/test_callback_scope.cpp index 04f7a80a..6f50ddd7 100644 --- a/tests/test_callback_scope.cpp +++ b/tests/test_callback_scope.cpp @@ -13,6 +13,7 @@ #include #include +#include #include #include #include @@ -573,3 +574,78 @@ TEST_CASE("CallbackScope: destroying the scope under a concurrent dispatch loop REQUIRE(hits->load() == settled); } } + +// ── morph#499: what CallbackScope's concurrency contract actually covers ── +// +// 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 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}; + 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. + // 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(); + + // 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 + // 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_executor.cpp b/tests/test_executor.cpp index a08dc2d8..938ff81e 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_file_offline_queue.cpp b/tests/test_file_offline_queue.cpp index f9b1edd6..90367ec2 100644 --- a/tests/test_file_offline_queue.cpp +++ b/tests/test_file_offline_queue.cpp @@ -13,6 +13,9 @@ #include #include +#ifndef _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. +#ifndef _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 const reopened{path}; + REQUIRE(reopened.drain().size() == 3); + std::filesystem::remove(path); +} +#endif // _WIN32 diff --git a/tests/test_opaque_model_ids.cpp b/tests/test_opaque_model_ids.cpp index 0ede2e8d..9c8858e4 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 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 + // +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); 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..cdd4c3d6 100644 --- a/tests/test_render_locale_format.cpp +++ b/tests/test_render_locale_format.cpp @@ -145,3 +145,36 @@ 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(".", ".", ",") == "."); +}