Close #506 with a reproduction, and establish that the mutation survivor list is not trustworthy (#453, #504) - #511
Merged
Merged
Conversation
…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 adfe8e5 -- 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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SxkvtHvan2fWKDDaBpqkgv
) ## #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<Quantity>`. Its sibling cross-reference pointed "above" at something ~90 lines below. - `rational.hpp` -- `<atomic>` and `<stdexcept>` are unused (no `std::atomic`, no `throw`, no `*_error`), while `<string_view>` 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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SxkvtHvan2fWKDDaBpqkgv
…wn 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<decltype(tv.tv_sec)>(...)`: `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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SxkvtHvan2fWKDDaBpqkgv
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Second pass at the remaining audit issues. Closes #506, and advances #453 and #504 with a finding that changes what item A is worth doing at all.
#506 — fixed, with the reproduction it never had
The hazard is real:
sendFrameholds_socketMtxacrosssendAll, which loops on a blocking::sendwith no timeout, so a peer that accepts and stops reading parks~SocketBackendon that lock.Two candidate fixes were rejected on evidence, not taste:
onDisconnected()reassigning_socket. 25 TSan reports vs 0._socket, so the destructor can::shutdownwithout the mutex — carries an fd-reuse hazard, since the fd can be closed and recycled between the load and the shutdown.So it is closed from the other end:
SO_SNDTIMEOvia a newSocketBackendConfig::sendTimeout(30s default). A send making no progress returnsEAGAIN,sendAllthrows as it already does for any other send error, and the lock is released. The lock stays where it is, so the data race stays closed.The issue had no reproduction. It has one now, written before the fix:
timeout 120— it hangs#453 — the finding that matters more than the classification
Item A asks for the campaign's 199 survivors classified. Before classifying, I sampled six by applying each reported mutation by hand. Five are killed by the existing suite despite being reported as survived:
remote.hpp:1031if (env.kind == "register")forced trueremote.hpphandleInlineif (env.kind == "execute")forced truebridge.hppwhenBoundif (isBound(binding))forced truebridge.hpp:1683_lifetime->alive = false→trueforms.hpp:1794bool found = false→truebridge.hpp:584bool started = false→trueThe first was measured on a worktree at
adfe8e5f— the campaign's own commit, with its own build — against that revision's 21985-passing baseline.Ruled out before filing: not a test-scope mismatch (
core-formsrunstests/morph_tests, same binary, no filter), not a revision mismatch, not a wholesale mutant-selection failure (574 of 773 were killed in the same run).I corrected myself mid-investigation. My first draft blamed two whole mutator families and put "117 of 199, 59%" on it. Continuing to sample disproved that:
cxx_init_constproduced one genuine equivalent and one mis-report. The defect is per-mutant, so the percentage is withdrawn. Filed as #510.Consequence for item A: classifying 199 entries by hand against a list where 5 of 6 samples are false measures Mull, not the suite — the "control that reports a result while measuring nothing" this repo keeps catching.
scripts/mutation_survivors.jsonrecords the six samples and this reasoning instead of a classification.#504 — five more stale comments
_executeGate's comment that negated itself mid-parenthesis;guard()'s "rejected at compile time" (thestatic_assertis in the wrapper body, so it fires on invocation); two@ptags naming things that are not parameters; a reference to anoperator<<the same file says does not exist; andrational.hpp's unused<atomic>/<stdexcept>alongside a<string_view>that was only ever transitive — removal verified against the whole tree including the ladder, after themodel.hpplesson.Verification
Run locally before pushing this time, which caught two clang-tidy findings that would otherwise have cost a CI round trip: 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.Closes #506.
🤖 Generated with Claude Code
https://claude.ai/code/session_01SxkvtHvan2fWKDDaBpqkgv