From 8ec3806562aa8be5b90a94c6bd5e49ef2dd29fda Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Thu, 10 Sep 2026 12:12:49 +0200 Subject: [PATCH 1/3] mutation: the survivor list is not trustworthy -- 5 of 6 samples are killed (#453, #510) morph#453 item A asks for run[2]'s 199 survivors classified. Before classifying them I sampled six, applying each reported mutation by hand and running the suite. Five are detected by the existing tests despite being reported as survived: remote.hpp:1031 cxx_replace_scalar_call 140 assertion failures remote.hpp handleInline 84 assertion failures bridge.hpp whenBound 6 assertion failures bridge.hpp:1683 cxx_assign_const suite hangs (a timeout is detected) forms.hpp:1794 cxx_init_const 2 assertion failures bridge.hpp:584 cxx_init_const unchanged -- correctly survived The first was measured on a worktree at adfe8e5f -- the exact commit the campaign ran -- with its own build, so the quoted source line matches the report character-for-character and the 140 failures are against that revision's own 21985-passing baseline. Ruled out before filing: not a test-scope mismatch (the core-forms scope runs tests/morph_tests, the same binary and full suite, no filter passed to mull-runner); not a revision mismatch (see above); not a wholesale mutant-selection failure (574 of 773 were killed in the same run). **The defect is per-mutant, not per-family.** My first draft of this blamed two whole mutator families and put "117 of 199, 59%" on it. Continuing to sample disproved that: cxx_init_const produced one genuine equivalent (bridge.hpp:584 -- the initialiser is dead, overwritten on the next statement) *and* one mis-report (forms.hpp:1794 -- the accumulator idiom, where the initial value is read). Same mutator, both outcomes. The percentage is withdrawn; six samples support only the narrow claim that the list cannot be worked from. Mechanism is NOT established. morph#434 confirmed its defect by disassembling the mutant; Mull is not installed on this workstation, so this is behavioural evidence only, and the file says so. What this does to item A: classifying 199 entries by hand against a list where 5 of 6 samples are false would be measuring Mull rather than the suite -- the "control that reports a result while measuring nothing" this repository keeps catching. Filed as morph#510; the tooling needs characterising first. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01SxkvtHvan2fWKDDaBpqkgv --- scripts/mutation_survivors.json | 76 +++++++++++++++++++++++++++++---- 1 file changed, 67 insertions(+), 9 deletions(-) diff --git a/scripts/mutation_survivors.json b/scripts/mutation_survivors.json index a5bebdcf..cb414756 100644 --- a/scripts/mutation_survivors.json +++ b/scripts/mutation_survivors.json @@ -429,9 +429,12 @@ }, "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." + "Classification of run[2]'s 199 survivors (morph#453 item A).", + "Updated 2026-09-10. The headline finding is NOT a classification: sampling", + "showed the survivor list itself is unreliable -- 5 of 6 sampled survivors", + "are killed by the existing suite when the same mutation is applied by hand.", + "See survivor_list_unreliable below and morph#510. Classifying the remaining", + "entries against this list would be measuring the tool, not the test suite." ], "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": { @@ -460,13 +463,68 @@ }, "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": "16 classified (13 equivalent, 3 real and closed). The rest are NOT classified, and per survivor_list_unreliable above, classifying them against this run is the wrong next step.", + "next_step": "Fix or characterise the tooling first (morph#510). A survivor list where 5 of 6 samples are false is not a worklist." + }, + "survivor_list_unreliable": { + "sampled": 6, + "detected_by_suite_despite_being_reported_survived": 5, + "issue": "morph#510", + "status": "Reproduced behaviourally, one of them on a worktree at the campaign's own revision (adfe8e5f) with its own build. Mechanism NOT established -- Mull is not installed here, so no disassembly, unlike morph#434.", + "samples": [ + { + "site": "remote.hpp:1031", + "mutator": "cxx_replace_scalar_call", + "mutation": "if (env.kind == \"register\") forced true", + "result": "140 assertion failures (baseline 21985 passing) -- at adfe8e5f", + "verdict": "detected" + }, + { + "site": "remote.hpp handleInline", + "mutator": "cxx_replace_scalar_call", + "mutation": "if (env.kind == \"execute\") forced true", + "result": "84 assertion failures", + "verdict": "detected" + }, + { + "site": "bridge.hpp whenBound", + "mutator": "cxx_replace_scalar_call", + "mutation": "if (isBound(binding)) forced true", + "result": "6 assertion failures", + "verdict": "detected" + }, + { + "site": "bridge.hpp:1683", + "mutator": "cxx_assign_const", + "mutation": "_lifetime->alive = false -> true in ~Bridge", + "result": "suite hangs; mull-runner's --timeout 60000 counts a timeout as detected", + "verdict": "detected" + }, + { + "site": "forms.hpp:1794", + "mutator": "cxx_init_const", + "mutation": "bool found = false -> true", + "result": "2 assertion failures", + "verdict": "detected" + }, + { + "site": "bridge.hpp:584", + "mutator": "cxx_init_const", + "mutation": "bool started = false -> true", + "result": "suite unchanged (21985 passing)", + "verdict": "genuinely equivalent -- the initialiser is dead, overwritten by `started = backend->attachModelAsync(...)` on the next statement" + } + ], + "important": [ + "The defect is per-mutant, not per-family: cxx_init_const produced one genuine equivalent AND one mis-report. An earlier draft of this entry blamed whole families and was wrong.", + "So the 199 cannot be bulk-reclassified by mutator. What the sampling establishes is that the list is not trustworthy enough to classify against by hand." + ], + "ruled_out": [ + "Test-scope mismatch: scripts/mutation.sh's core-forms scope runs tests/morph_tests, the same binary and full suite, with no filter passed to mull-runner.", + "Revision mismatch: the 140-failure measurement is on adfe8e5f itself and the quoted source line matches character-for-character.", + "Wholesale mutant-selection failure: 574 of 773 were killed in the same run, so the mechanism works in general." ], - "note": "remote.hpp (82) and bridge.hpp (33) hold 58% of all survivors. Neither has been assessed." + "consequence": "The 199-survivor baseline overstates real gaps by an unknown but large margin. It still works as a relative ratchet for morph#408's gate, since the artifacts are stable run to run." } } } From a64b10b4b443c409d95934a37b5f97a0321e1b06 Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Thu, 10 Sep 2026 12:32:02 +0200 Subject: [PATCH 2/3] net: bound a no-progress send (#506), and five more stale comments (#504) ## #506 -- closed from the other end The hazard is real: `sendFrame` holds `_socketMtx` across `sendAll`, which loops on a blocking `::send` with no timeout, so a peer that accepts and stops reading fills the kernel send buffer and parks `~SocketBackend` on that lock, with nothing able to release it. The obvious fix -- dropping the lock -- is wrong, and was reverted after CI proved it: it races `onDisconnected()` reassigning `_socket` (25 ThreadSanitizer reports against 0). The other candidate, an atomic fd shadowing `_socket` so the destructor can `::shutdown` without the mutex, carries an fd-reuse hazard of its own -- the fd can be closed and recycled between the load and the shutdown. So: `TcpSocket::setSendTimeout` (SO_SNDTIMEO), applied on connect from a new `SocketBackendConfig::sendTimeout` (30s default, zero disables). A send making no progress now returns EAGAIN, `sendAll` throws as it already does for every other send error, and `_socketMtx` is released. The lock stays exactly where it was, so the data race stays closed. The timeout is deliberately generous and is not a slow-link cutoff -- it bounds one `send` syscall making *no* progress. morph#506 had no reproduction. It has one now, written before the fix rather than after: a listener that accepts and never reads a byte, with the sender pushing until the buffers fill. Measured both ways -- without the timeout: exit 124 under an external `timeout 120` -- it hangs with the timeout: throws and completes, well inside the 20s bound ## #504 -- five more comments that describe something the code does not do - `remote.hpp` -- the `_executeGate` comment claimed the fast-reject path "never touches this gate at all" and then negated itself in the same parenthesis ("...except it does get a ticket"). `handleImpl` takes a ticket for any well-formed `execute` with a non-zero modelId, long before the registry lookup that finds the model gone, so the self-correction was the true half. - `callback_scope.hpp` -- "rejected at compile time" overstates it: the `static_assert` sits inside the returned wrapper's body, so it fires when the wrapper is *invoked*. A guard built from a value-returning callable and never called compiles cleanly. Corrected in the header and in docs/spec/core/callback_scope.md. - `backend.hpp` -- `@p typeId` and `@p mid` both marked as parameters of a function that has neither: `typeId` is unnamed in the signature, and the id is a local inside `createAndTrack`. - `quantity.hpp` -- referred to an `operator<<(std::format)` that does not exist, in a file that says so itself at :1161; the referent is `std::formatter`. Its sibling cross-reference pointed "above" at something ~90 lines below. - `rational.hpp` -- `` and `` are unused (no `std::atomic`, no `throw`, no `*_error`), while `` is used four times and came in only transitively. Removal verified by building the whole tree including the ladder, after the model.hpp lesson that an unused include can still be load-bearing for consumers. docs/spec/util/quantity_type.md also records that `formatRationalDecimal` takes its magnitude through `absU64`, and why that path escaped the clamp (morph#496). Local pre-flight before pushing, rather than discovering it in CI: tree-wide clang-format, `-Wdocumentation -Werror`, spec-citation lint, full build, both suites (22021 and 987 assertions), TSan on the SocketBackend cases (0 reports), and clang-tidy filtered to changed lines -- which caught two findings (`setSendTimeout` wanting `const`, an implicit widening in the test) that would otherwise have been a CI round trip. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01SxkvtHvan2fWKDDaBpqkgv --- docs/spec/core/callback_scope.md | 11 ++++-- docs/spec/util/quantity_type.md | 10 +++++- include/morph/core/backend.hpp | 8 +++-- include/morph/core/callback_scope.hpp | 6 ++-- include/morph/core/remote.hpp | 9 ++--- include/morph/net/detail/tcp_socket.hpp | 22 ++++++++++++ include/morph/net/socket_backend.hpp | 27 ++++++++++++--- include/morph/util/quantity.hpp | 4 +-- include/morph/util/rational.hpp | 3 +- scripts/branch_partial_allowlist.json | 32 ++++++++--------- tests/net/test_tcp_socket.cpp | 46 +++++++++++++++++++++++++ 11 files changed, 141 insertions(+), 37 deletions(-) diff --git a/docs/spec/core/callback_scope.md b/docs/spec/core/callback_scope.md index f001f05e..7e458704 100644 --- a/docs/spec/core/callback_scope.md +++ b/docs/spec/core/callback_scope.md @@ -148,8 +148,15 @@ QTimer::singleShot(0, _callbacks.guard([this] { tick(); })); ``` The returned callable forwards every argument and returns `void`. A -value-returning callable is rejected at compile time: there is no defensible -value to return when delivery is suppressed. +value-returning callable is rejected: there is no defensible value to return +when delivery is suppressed. + +The rejection is a `static_assert` **inside the returned wrapper's body**, so it +fires when the wrapper is *invoked*, not when `guard()` is called. A wrapper +built from a value-returning callable and then never called compiles cleanly — +worth knowing when the guarded callback is stored and its subscription torn down +before its first tick. Constraining `guard()` itself would make the rejection +unconditional; that has not been done. ### Gated overloads elsewhere diff --git a/docs/spec/util/quantity_type.md b/docs/spec/util/quantity_type.md index 04dc4204..38ede613 100644 --- a/docs/spec/util/quantity_type.md +++ b/docs/spec/util/quantity_type.md @@ -252,7 +252,15 @@ unit metadata travels only in the schema, never in the instance. Every printed form of a `Quantity` — `std::format`, and every number inside `equation()` — goes through **one** helper, `detail::formatRationalDecimal`, so a value reads identically everywhere. There is a single formatting path and -**no `operator<<`** (streaming is done by formatting to a `std::string` first). +**no `operator<<`** (streaming is done by formatting to a `std::string` first); +in-code references to one are references to `std::formatter`. + +`formatRationalDecimal` takes the numerator's magnitude through +`math::detail::absU64`, in unsigned arithmetic. It negated in `int64_t` until +morph#496, which is undefined for `INT64_MIN` — reachable because the +whole-integer `Rational{value, DecimalPlaces{n}}` constructor does not +canonicalise, so the clamp that would otherwise remove the trap value never +ran. **The decimal form.** `formatRationalDecimal` renders the exact `Rational` as a fixed decimal at its **runtime `DecimalPlaces`** and then trims trailing zeros diff --git a/include/morph/core/backend.hpp b/include/morph/core/backend.hpp index d13b16ab..ab17469b 100644 --- a/include/morph/core/backend.hpp +++ b/include/morph/core/backend.hpp @@ -611,9 +611,11 @@ class LocalBackend : public detail::IBackend { /// @brief Creates a model instance via @p factory and registers it. /// - /// @p typeId is accepted for interface compatibility but not used — the - /// concrete type is captured by the factory closure. If the new holder's - /// `isBackendChangeAware()` returns `true`, @p mid is also recorded in + /// The `typeId` parameter is accepted for interface compatibility but not + /// used — it is unnamed in the signature below, and the concrete type is + /// captured by the factory closure. If the new holder's + /// `isBackendChangeAware()` returns `true`, the new id (a local in + /// `createAndTrack`, not a parameter here) is also recorded in /// `_changeAware` so `notifyBackendChanged()` finds it without a /// `dynamic_cast` sweep. /// @param factory Callable that constructs the `IModelHolder`. diff --git a/include/morph/core/callback_scope.hpp b/include/morph/core/callback_scope.hpp index d74612b5..62a9380c 100644 --- a/include/morph/core/callback_scope.hpp +++ b/include/morph/core/callback_scope.hpp @@ -98,8 +98,10 @@ class CallbackToken { /// /// @tparam F Callable type to wrap. Must return `void` for every argument /// list it is invoked with; a value-returning callable has no - /// defensible answer for the suppressed case and is rejected at - /// compile time. + /// defensible answer for the suppressed case. Rejected by a + /// `static_assert` in the returned wrapper's body, so it fires + /// when the wrapper is *invoked* -- a wrapper that is created and + /// never called compiles either way. /// @param fn Callable to gate. Moved into the returned wrapper. /// @return A callable with @p fn's argument list and a `void` return. template diff --git a/include/morph/core/remote.hpp b/include/morph/core/remote.hpp index fb8e1408..c32a4f6e 100644 --- a/include/morph/core/remote.hpp +++ b/include/morph/core/remote.hpp @@ -1688,10 +1688,11 @@ class RemoteServer : public std::enable_shared_from_this { // (`test_remote_connection_scope.cpp`'s "an in-flight execute completes // safely across a disconnect" test — a lookup against a since-reclaimed // modelId must resolve without waiting on some other blocked model's - // strand — never touches this gate at all, since it never gets a ticket - // for a model that turns out to be gone... except it does get a ticket, - // and must release it immediately rather than hold up a live ticket - // behind it; see `ExecuteOrderGate::release`'s own doc comment). + // strand). That path *does* take a ticket: `handleImpl` takes one for any + // well-formed `execute` with a non-zero `modelId`, long before the registry + // lookup that discovers the model is gone. What keeps it fast is releasing + // that ticket immediately rather than holding up a live one behind it; see + // `ExecuteOrderGate::release`'s own doc comment. // // Keyed by ModelId internally, not held forever: a model with no // outstanding tickets has no entry in the gate's map at all (erased once diff --git a/include/morph/net/detail/tcp_socket.hpp b/include/morph/net/detail/tcp_socket.hpp index f5691a5e..3177c8ae 100644 --- a/include/morph/net/detail/tcp_socket.hpp +++ b/include/morph/net/detail/tcp_socket.hpp @@ -374,6 +374,28 @@ class TcpSocket { } } + /// @brief Bounds how long a single `::send` inside `sendAll()` may block. + /// + /// Without this a `sendAll` against a peer that has stopped reading blocks + /// forever once the kernel send buffer fills, and it does so while holding + /// whatever lock its caller took -- which is how `~SocketBackend` came to be + /// parkable behind `_socketMtx` (morph#506). With `SO_SNDTIMEO` set, the + /// blocked `send` returns `EAGAIN`/`EWOULDBLOCK` instead, `sendAll` throws + /// as it already does for any other send error, and the lock is released. + /// + /// A timeout is not a "slow link" cutoff: it bounds one `send` syscall that + /// is making *no* progress, so it should be set generously. Zero disables it + /// (the kernel default, block forever). + /// + /// @param timeout Per-`send` bound; zero to disable. + /// @return `true` if the option was applied. + [[nodiscard]] bool setSendTimeout(std::chrono::milliseconds timeout) const noexcept { + timeval tv{}; + tv.tv_sec = static_cast(timeout.count() / 1000); + tv.tv_usec = static_cast((timeout.count() % 1000) * 1000); + return ::setsockopt(_fd, SOL_SOCKET, SO_SNDTIMEO, &tv, sizeof(tv)) == 0; + } + /// @brief Shuts down both directions of the socket, unblocking a concurrent /// `recvSome`/`sendAll` on another thread. Safe to call from any thread. /// diff --git a/include/morph/net/socket_backend.hpp b/include/morph/net/socket_backend.hpp index dc133ab2..c8cd5884 100644 --- a/include/morph/net/socket_backend.hpp +++ b/include/morph/net/socket_backend.hpp @@ -36,6 +36,14 @@ struct SocketBackendConfig { double backoffMultiplier = 2.0; /// @brief Maximum time to wait for the initial TCP connect to complete. std::chrono::milliseconds connectTimeout{5000}; + /// @brief Bound on a single `::send` that is making no progress. + /// + /// Applied as `SO_SNDTIMEO`. Without it, a peer that stops reading fills the + /// kernel send buffer and parks `sendFrame` inside `sendAll` **while holding + /// `_socketMtx`** -- which parks `~SocketBackend` behind the same lock, with + /// nothing able to release it (morph#506). Generous on purpose: it bounds a + /// send making *no* progress, not a slow one. Zero disables it. + std::chrono::milliseconds sendTimeout{30000}; }; /// @brief `IBackend` implementation that communicates with a `RemoteServer` @@ -94,11 +102,13 @@ class SocketBackend : public ::morph::backend::detail::IBackend { // 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. + // The hazard #506 describes is closed from the other end: `sendAll` is + // no longer un-timed. `Config::sendTimeout` (SO_SNDTIMEO, 30s default) + // bounds any single send that makes no progress, so a peer that stops + // reading can hold `_socketMtx` for at most that long instead of + // forever, and this wait is bounded rather than open-ended. Fixing it + // that way rather than by reaching the fd without the mutex avoids the + // fd-reuse hazard an atomic shadow descriptor would carry. { std::scoped_lock const lock{_socketMtx}; if (_socket.valid()) { @@ -639,6 +649,13 @@ class SocketBackend : public ::morph::backend::detail::IBackend { bool connectedOk = false; try { auto socket = ::morph::net::detail::TcpSocket::connect(_url.host, _url.port, _cfg.connectTimeout); + if (_cfg.sendTimeout.count() > 0) { + // Before the handshake, so even that cannot park forever. + // Bounds any single send that makes no progress, which is + // what keeps ~SocketBackend from being parked behind + // _socketMtx by a peer that stopped reading (morph#506). + (void)socket.setSendTimeout(_cfg.sendTimeout); + } std::string leftover = ::morph::net::detail::performClientHandshake(socket, _url); { std::scoped_lock lock{_socketMtx}; diff --git a/include/morph/util/quantity.hpp b/include/morph/util/quantity.hpp index be671ea2..ed4feffa 100644 --- a/include/morph/util/quantity.hpp +++ b/include/morph/util/quantity.hpp @@ -723,11 +723,11 @@ struct Quantity { Quantity out; out.payload = adjusted; // Not `out._ctx = _ctx`: that would copy this quantity's derivation - // node verbatim, so equation() and operator<<(std::format) would + // node verbatim, so equation() and std::formatter would // disagree -- the node's own recorded `result` is still *this* // quantity's old payload/precision, but `out.payload` is the // retagged one. A fresh node (same convention as operator - // Quantity()'s unit conversion above) keeps the two consistent. + // Quantity()'s unit conversion, below) keeps the two consistent. MORPH_Q_BUILD(out, "retag decimal places", payload, std::nullopt, out.payload, MORPH_Q_NODE(*this), nullptr); return out; } diff --git a/include/morph/util/rational.hpp b/include/morph/util/rational.hpp index f0a3acc8..b0d1705c 100644 --- a/include/morph/util/rational.hpp +++ b/include/morph/util/rational.hpp @@ -95,7 +95,6 @@ /// (e.g. sums over large coprime denominators). Keep operands within /// the decimal-scaled ranges the precision tags imply. -#include #include #include #include @@ -108,8 +107,8 @@ #include #include #include -#include #include +#include #include #include diff --git a/scripts/branch_partial_allowlist.json b/scripts/branch_partial_allowlist.json index b814da1d..41617565 100644 --- a/scripts/branch_partial_allowlist.json +++ b/scripts/branch_partial_allowlist.json @@ -20,21 +20,21 @@ "entries": [ { "file": "include/morph/util/rational.hpp", - "line": 272, + "line": 271, "source": "assert(rawDecimalPlaces <= kMaxDecimalPlaces);", "reason": "assert()'s false arm calls abort(). Reaching it means violating the precondition and terminating the process, so no test in a Catch2 binary can observe it and still report; a death test would prove only that assert() works. The decision the assert guards is checked by the caller-side tests that stay inside kMaxDecimalPlaces." }, { "file": "include/morph/util/rational.hpp", - "line": 698, + "line": 697, "source": "if (!std::is_constant_evaluated()) {", "reason": "The false arm is the constant-evaluated one, and constant evaluation does not execute instrumented code -- no counter can increment during it. So in every run this branch is true, structurally, and its other arm is not a gap in the tests but a gap in what runtime instrumentation can see. reportOverflow's constexpr behaviour is exercised by the constexpr Rational arithmetic in tests/test_rational.cpp; what cannot be exercised is llvm-cov's counter." }, { "file": "include/morph/util/rational.hpp", - "line": 732, + "line": 731, "source": "if (!std::is_constant_evaluated()) {", - "reason": "reportClamp's copy of the same shape as line 698 above, and uncoverable for the same reason: the untaken arm is constant evaluation, which increments no counters." + "reason": "reportClamp's copy of the same shape as line 697 above, and uncoverable for the same reason: the untaken arm is constant evaluation, which increments no counters." }, { "file": "include/morph/util/datetime.hpp", @@ -44,31 +44,31 @@ }, { "file": "include/morph/util/rational.hpp", - "line": 758, + "line": 757, "source": "return std::is_gt(ordering) ? 1 : 0;", - "reason": "The false arm (a tie, returning 0) is unreachable. compareForSaturation has exactly two call sites, both from the saturating branch of operator+=/operator-=, and both are guarded by addWouldOverflow/subWouldOverflow having just returned true. A tie in compareForSaturation (*this == -rhs for +=, or *this == rhs for -=) requires -- since operator== compares canonical numerator/denominator pairs directly -- identical denominators. With equal denominators, addWouldOverflow/subWouldOverflow's own scaling factors collapse to rightScaled == leftScaled == 1 (gcd(D, D) == D), so the cross-multiplication checks reduce to numerator +/- rhs.numerator directly on already-canonical (so already in-range) int64_t values whose sum/difference is provably 0 -- never an overflow (and canonicalise() already forbids either component from being INT64_MIN, so negating one to check the other is always representable). So whenever the two operands could produce a tie, the overflow predicate that gates the call to compareForSaturation is always false, and the saturating path -- hence compareForSaturation's tie return -- is never reached with a tied pair. Shares this root cause with saturateToward's sign == 0 arm at line 776 below." + "reason": "The false arm (a tie, returning 0) is unreachable. compareForSaturation has exactly two call sites, both from the saturating branch of operator+=/operator-=, and both are guarded by addWouldOverflow/subWouldOverflow having just returned true. A tie in compareForSaturation (*this == -rhs for +=, or *this == rhs for -=) requires -- since operator== compares canonical numerator/denominator pairs directly -- identical denominators. With equal denominators, addWouldOverflow/subWouldOverflow's own scaling factors collapse to rightScaled == leftScaled == 1 (gcd(D, D) == D), so the cross-multiplication checks reduce to numerator +/- rhs.numerator directly on already-canonical (so already in-range) int64_t values whose sum/difference is provably 0 -- never an overflow (and canonicalise() already forbids either component from being INT64_MIN, so negating one to check the other is always representable). So whenever the two operands could produce a tie, the overflow predicate that gates the call to compareForSaturation is always false, and the saturating path -- hence compareForSaturation's tie return -- is never reached with a tied pair. Shares this root cause with saturateToward's sign == 0 arm at line 775 below." }, { "file": "include/morph/util/rational.hpp", - "line": 776, + "line": 775, "source": "numerator = sign == 0 ? 0 : (sign < 0 ? -maxValue : maxValue);", - "reason": "The sign == 0 arm is unreachable, for the same root cause as compareForSaturation's tie return at line 758 above: saturateToward is only ever called with the sign compareForSaturation returned, and compareForSaturation can only return 0 (a tie) for a pair whose overflow predicate is provably false -- so the saturating path that calls saturateToward is never entered with a tied pair, and sign is never 0 when this line runs." + "reason": "The sign == 0 arm is unreachable, for the same root cause as compareForSaturation's tie return at line 757 above: saturateToward is only ever called with the sign compareForSaturation returned, and compareForSaturation can only return 0 (a tie) for a pair whose overflow predicate is provably false -- so the saturating path that calls saturateToward is never entered with a tied pair, and sign is never 0 when this line runs." }, { "file": "include/morph/util/rational.hpp", - "line": 901, + "line": 900, "source": "if (crossDivisorOne == 0 || crossDivisorTwo == 0) {", "reason": "Both disjuncts are unreachable, and the body they guard (the zero-numerator short-circuit `return false;` right below) is correspondingly dead. crossDivisorOne = gcd(|numerator|, rhs.denominator), crossDivisorTwo = gcd(|rhs.numerator|, denominator). std::gcd(a, b) == 0 iff both a == 0 and b == 0. denominator/rhs.denominator can never be 0: every constructor path (Rational(Numerator, Denominator, DecimalPlaces)) calls canonicalise(), which clamps a 0 denominator to 1 before returning, and every mutating operation either recomputes the denominator as a product of positive denominators or calls canonicalise() again. So denominator > 0 (and rhs.denominator > 0) is a whole-class invariant, making crossDivisorOne/crossDivisorTwo == 0 impossible regardless of numerator/rhs.numerator." }, { "file": "include/morph/util/rational.hpp", - "line": 1499, + "line": 1498, "source": "if (ctx.begin() == ctx.end() || *ctx.begin() == '}') {", "reason": "The ctx.begin() == ctx.end() true arm is unreachable. This is the textbook cppreference-style custom-formatter parse() idiom. Empirically verified on this toolchain (libc++, via a standalone probe compiled with clang++ -std=c++23 -stdlib=libc++): for both std::format(\"{}\", x) and std::format(\"{:}\", x), ctx.end() always points past the terminating '}', so ctx.begin() == ctx.end() is false and the terminator is always reachable via *ctx.begin() == '}' -- matching the observed 0/14 split exactly. std::format's top-level parser (and std::vformat's, which performs the same replacement-field validation before dispatching to a type's parse()) rejects an unterminated '{...' before ever calling into formatter::parse, so this function is never invoked with an already-exhausted range. The first disjunct is defensive boilerplate that this standard library's implementation (and the standard's own guarantee about validated replacement fields) makes structurally unreachable." }, { "file": "include/morph/util/rational.hpp", - "line": 1408, + "line": 1407, "source": "if (!detail::addOverflows(whole, step)) {", "reason": "The false arm (the overflow-would-occur case, declining to step) is unreachable, contrary to Task 2's initial classification of this line as testable -- verified both mathematically and empirically (a standalone probe sweeping denominators/precisions found 476 cases where the scale-up saturated to whole == INT64_MAX, and every one had a fractional remainder of exactly 0, never >= 0.5) before writing this entry. A Rational's magnitude can never exceed INT64_MAX: its value is numerator/denominator with denominator >= 1 and numerator in [-INT64_MAX, INT64_MAX] (canonicalise() clamps INT64_MIN away), so |value| <= |numerator| <= INT64_MAX always. `whole` is trunc(scaled), so whole == INT64_MAX forces scaled == INT64_MAX/1 exactly -- there is no room for scaled to be in (INT64_MAX, INT64_MAX + 1) the way an unbounded rational could land. With scaled == whole exactly, `fraction` (scaled minus whole) is always 0, so `roundAway` (set from comparing fraction against 1/2) is always false when whole == INT64_MAX, and stepping never happens on that side. On the other side, step == -1 would need whole == INT64_MIN to overflow, but whole's range is [-INT64_MAX, INT64_MAX] (INT64_MIN is never a valid Rational magnitude), so that direction cannot overflow either. The guard is defensive: reachable only if a future change let a Rational's magnitude exceed INT64_MAX, which the type's invariants currently forbid everywhere else in this file." }, @@ -80,7 +80,7 @@ }, { "file": "include/morph/core/backend.hpp", - "line": 777, + "line": 779, "source": "if (iter != _models.end()) {", "reason": "Unreachable by construction given the `_changeAware`/`_models` invariant (core audit finding BK2). Every model id is inserted into `_changeAware` (when change-aware) in the same `_regMtx`-held critical section that inserts it into `_models` (`createAndTrack`, this file: `if (holder->isBackendChangeAware()) { _changeAware.insert(mid); } _models[mid] = std::move(holder);`), and both are erased together at the single erasure site (`deregisterModel`: `_models.erase(mid); _changeAware.erase(mid);`, also under `_regMtx`). `notifyBackendChanged()` (this function) holds the same `_regMtx` while iterating `_changeAware` and looking each id up in `_models` at this line, so every id it walks is guaranteed still present in `_models` -- the \"not found\" arm cannot occur without a code change that breaks this lockstep bookkeeping." }, @@ -140,19 +140,19 @@ }, { "file": "include/morph/net/socket_backend.hpp", - "line": 109, + "line": 119, "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": 121, + "line": 131, "source": "if (_handlerThread.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." + "reason": "Unreachable by construction, same shape as `_ioThread`'s line 119 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": 560, + "line": 570, "source": "default:", "reason": "Unreachable except via the adjacent `Error` case it deliberately shares a body with (net audit, `socket_backend.hpp` extra finding #7). `detail::ExecuteReplyKind` is a closed 3-value enum (`Value`/`Timeout`/`Error`), all three handled explicitly above this label; `default:` exists only to satisfy this project's `-Wswitch-default`, per the source's own inline comment directly below this line. Reaching it via any value other than through the `Error` case falling through would require an out-of-range `static_cast` producing a value outside the enum's domain -- undefined behavior, not a legitimate test target." } diff --git a/tests/net/test_tcp_socket.cpp b/tests/net/test_tcp_socket.cpp index 3906dbe0..08af5fb4 100644 --- a/tests/net/test_tcp_socket.cpp +++ b/tests/net/test_tcp_socket.cpp @@ -477,3 +477,49 @@ TEST_CASE("TcpSocket::shutdownBoth: a safe no-op on an empty socket", "[net][tcp empty.shutdownBoth(); // must not crash REQUIRE_FALSE(empty.valid()); } + +// ── morph#506: a peer that stops reading must not park the sender forever ── +// +// Once the kernel send buffer fills against a peer that never reads, a blocking +// `::send` never returns -- and `sendAll` loops on it. `SocketBackend::sendFrame` +// holds `_socketMtx` across that call, so `~SocketBackend` parks on the same +// lock with nothing able to release it. (Removing the lock is NOT the fix: it +// races `onDisconnected()` reassigning `_socket`, 25 ThreadSanitizer reports.) +// +// `SO_SNDTIMEO` bounds one no-progress send instead, so `sendAll` throws the way +// it already does for any other send error and the lock is released. +TEST_CASE("TcpSocket: setSendTimeout bounds a send against a peer that never reads", "[net][tcp][morph506]") { + auto listener = TcpSocket::listen(0); + std::uint16_t const port = listener.boundPort(); + + // Accepts and then does nothing at all -- never reads a byte. Held open for + // the duration of the test so the connection stays established. + TcpSocket serverSide; + std::thread acceptThread{[&] { serverSide = listener.accept(); }}; + auto clientSide = TcpSocket::connect("127.0.0.1", port, std::chrono::milliseconds{2000}); + acceptThread.join(); + REQUIRE(serverSide.valid()); + + constexpr auto kTimeout = std::chrono::milliseconds{300}; + REQUIRE(clientSide.setSendTimeout(kTimeout)); + + // Push until the buffers fill. Without the timeout this loop never returns; + // with it, sendAll throws once a single send makes no progress. + std::string const chunk(std::size_t{256} * 1024, 'x'); + auto const start = std::chrono::steady_clock::now(); + bool threw = false; + for (int i = 0; i < 400 && !threw; ++i) { + try { + clientSide.sendAll(chunk.data(), chunk.size()); + } catch (const std::runtime_error&) { + threw = true; + } + } + auto const elapsed = std::chrono::steady_clock::now() - start; + + REQUIRE(threw); + // The bound that matters: it gave up rather than blocking indefinitely. + // Generous multiple of the timeout, since each successful send before the + // buffers filled costs real time too. + CHECK(elapsed < std::chrono::seconds{20}); +} From dca2e7da813e9debd43881cb4829a88c018d80f3 Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Thu, 10 Sep 2026 13:06:49 +0200 Subject: [PATCH 3/3] Fix setSendTimeout for GCC and clang-tidy, and close two gaps in my own pre-flight Both CI failures were in the function this branch added, and both were things my local checks could not see: - **GCC's `-Werror=useless-cast`** rejected `static_cast(...)`: `milliseconds::rep` and `timeval`'s members are both `long` here, so the cast is an identity cast. clang has no such warning, and I had only built with clang -- while the Valgrind leg builds with GCC, which is why it failed there at 2m11s (a compile error, not a memory finding). Casts dropped. - **clang-tidy's `readability-identifier-length`** wanted three characters, so the conventional `tv` is now `timeoutVal`. The pre-flight gaps, both now closed: 1. I built only with clang. Added a GCC build (`build/gcc-net`, the compiler the Valgrind and gcc legs use) and ran the suite under it. 2. My clang-tidy filter diffed the *working tree* (`git diff HEAD`), which is empty once the work is committed -- so it filtered 428 findings against zero changed lines and reported clean regardless. It now diffs `origin/master...HEAD`, matching what CI's clang-tidy-diff compares. Against the branch's real 107 changed lines: clean. Verified: clang and GCC builds both clean, net suite 987 assertions under each, the new #506 case passing under Valgrind (the leg that failed), tree-wide clang-format clean. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01SxkvtHvan2fWKDDaBpqkgv --- include/morph/net/detail/tcp_socket.hpp | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/include/morph/net/detail/tcp_socket.hpp b/include/morph/net/detail/tcp_socket.hpp index 3177c8ae..0eac59eb 100644 --- a/include/morph/net/detail/tcp_socket.hpp +++ b/include/morph/net/detail/tcp_socket.hpp @@ -390,10 +390,15 @@ class TcpSocket { /// @param timeout Per-`send` bound; zero to disable. /// @return `true` if the option was applied. [[nodiscard]] bool setSendTimeout(std::chrono::milliseconds timeout) const noexcept { - timeval tv{}; - tv.tv_sec = static_cast(timeout.count() / 1000); - tv.tv_usec = static_cast((timeout.count() % 1000) * 1000); - return ::setsockopt(_fd, SOL_SOCKET, SO_SNDTIMEO, &tv, sizeof(tv)) == 0; + // Assigned without casts on purpose: `milliseconds::rep` and + // `timeval`'s members are both `long` on the platforms this builds for, + // so an explicit cast is an identity cast and GCC rejects it under + // -Werror=useless-cast. clang-tidy also wants names of three characters + // or more, hence `timeoutVal` rather than the conventional `tv`. + timeval timeoutVal{}; + timeoutVal.tv_sec = timeout.count() / 1000; + timeoutVal.tv_usec = (timeout.count() % 1000) * 1000; + return ::setsockopt(_fd, SOL_SOCKET, SO_SNDTIMEO, &timeoutVal, sizeof(timeoutVal)) == 0; } /// @brief Shuts down both directions of the socket, unblocking a concurrent