Batch: thirteen audit findings — data loss, a security gap, two leaks, and UB (#493-#502, #505-#507) - #509
Merged
Merged
Conversation
…n one (#493, #494) Two file-backed stores scanned with an `ifstream` whose open they never checked, and both then committed the resulting empty read over the real file. `FileActionLog::repairTornTail()` (#493) probes with `_io.canOpenForRead` and then opens a *separate*, unchecked stream. When that open fails, `getline` runs zero times, `intactEnd` stays 0, and `resizeFile(_path, 0)` truncates the whole journal -- reported through the ordinary "discarded N byte(s) of a torn trailing record" warning, so it reads as a successful repair. Measured: three complete, fsynced entries (648 bytes) went to 0. The comment above the truncate argued the discard is safe because "whatever follows the final newline is by construction an incomplete record" -- true only of bytes the scan actually read, which is the assumption that was missing. `core/file_io_ops.hpp:76-78` already described the absent check as present ("stands in for repairTornTail()'s `if (!input)`"). Now: bail on a failed open, and bail on `input.bad()` -- a read error mid-scan leaves everything past `intactEnd` unread rather than established to be torn, and truncating there discards complete records too. `entries()` gets the same treatment, distinguishing "no journal yet" (absent, legitimately empty, which the constructor's dedup rebuild depends on) from "present but unreadable", which previously emptied the idempotencyKey dedup set OutboxRelay relies on and turned at-least-once-plus-dedup into duplicates with no diagnostic. `FileOfflineQueue::load()` (#494) is the same defect and needed no fault injection to reach: it bypasses the `FileIoOps` seam entirely with a raw `std::ifstream`, and the constructor calls `compact()` immediately after, which rewrites the file from the empty `_items`. Measured: three pending items (306 bytes) went to 0 with the constructor returning normally and the queue reporting an empty backlog. Both the failed-open and mid-read-error cases now throw, so compact() cannot run on a load that did not succeed. Also fixes the write ordering #494 reported alongside: `markDone()` erased from `_items` *before* `appendDone()`, so a throwing append (short write, failed fflush, failed fsync) left the item gone from memory with no tombstone on disk -- this process never replays it, and a restart resurrects and re-applies it. `enqueue()` already had the right order. `setAttempts()` had the same inversion. Now durable-first in both: a failure leaves the item live in both places, which replays once too often at worst, and `idempotencyKey` exists to absorb that. Regression tests for both, POSIX-only (making a file unreadable-but-writable is what reproduces the window; Windows maps permissions onto the read-only attribute alone) and skipped for a root euid. Verified they fail without the header changes and pass with them -- a test asserting only the happy path would have been green either way. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SxkvtHvan2fWKDDaBpqkgv
… fix (#496, #497, #499, #501) **#496 -- `formatRationalDecimal` negated `INT64_MIN` in `int64_t`.** The comment above it said "widen before taking the absolute value so the magnitude is always representable"; the expression nested the casts so the unary `-` ran on `int64_t`, and the widening cast happened after the UB rather than before it. Confirmed by UBSan at quantity.hpp:101. `INT64_MIN` reaches this function because the whole-integer `Rational{value, DecimalPlaces{n}}` constructor does not canonicalise, so the clamp in `canonicalise()` never runs on that path, and `numerator` is public. Now uses `detail::absU64`, the shared helper that does exactly what the comment claimed, already reachable from this header. **#497 -- a sign after the decimal separator was accepted.** `sawAnyOutput` was set only at the bottom of the loop and the decimal-separator branch `continue`d past it, so after a separator the sign guard still believed nothing had been emitted: `normalizeLocaleNumber(",-5", ",", ".")` returned ".-5". The guard's own comment states the intent exactly ("a stripped group separator before the sign would otherwise make an injected sign look leading"). The QML mirror in DynamicForm.qml, documented as mirroring this function, has always rejected it -- so the two control edges disagreed on the same input, which is the more serious half. Deliberately *not* narrowed further: the header also promised output matching `-?[0-9]+(\.[0-9]+)?`, which "`.`", "`.5`" and "`5.`" do not satisfy, and I first tightened the final check to enforce it. That was wrong -- the QML mirror accepts all three, so tightening one edge alone puts them back out of step, and rejecting ".5" is a UX regression on ordinary input. The shape is documented instead; narrowing both edges together is a separate call for the maintainer. **#499 -- `CallbackScope::reset()` raced every other member.** `_state` was a plain `shared_ptr` written by `reset()` and read by `token()`, `guard()`, `requestStop()` and `stopRequested()`; the control block's atomic refcount protects the pointee, not the handle. Made `std::atomic<std::shared_ptr<...>>` rather than narrowing the documented contract, because the guarantee is deliberate: the class documents every member as concurrently safe, and `reset()`'s own doc turns on a token holder "that pinned it while racing this call". Measured: 18 ThreadSanitizer reports before, 0 after. The two `_state != nullptr` guards went with it -- provably unreachable (sole constructor make_shared's, class non-copyable and non-movable, reset() always assigns a fresh value), so two permanently-surviving mutants under the gate morph#408 just brought online. **#501 -- `MainThreadExecutor` let a non-`std::exception` escape the pump.** `ThreadPoolExecutor::loop` has caught `...` all along; this one did not, and three doc claims on the same class depended on the missing arm -- `runOnce()` promises to return `true` "whether or not that task threw" and did not return at all. Added the mirroring catch-all. Regression tests for all four. #497 and #501 fail without their fix directly; #496 and #499 are sanitizer-dependent by nature -- value-stable UB and a data race -- and are caught by the existing `clang-ubsan` and `clang-tsan` CI legs, verified by hand both ways rather than assumed. Full suite: 22011 assertions, no regressions. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SxkvtHvan2fWKDDaBpqkgv
…ere authorize is called (#495, #500) **#495 -- the three async control builders never stamped `env.session`.** `registerModelSharedAsync`, `attachModelAsync` and `assignPrimaryAsync` each built their envelope with `makeRegisterShared`/`makeAttach`/`makeAssign` and encoded it with no session assignment, while all three synchronous counterparts (`registerModelShared`, `attachModel`, `assignPrimary`) stamped it immediately before encode, as does every equivalent site in `net::SocketBackend`. So it was a Qt-side omission, not a protocol choice. `RemoteServer` authenticates and authorizes from `env.session` -- `stampVerifiedPrincipal`, and the register/attach/assign authorization sites -- so those three verbs arrived with a default-constructed session and could not be authenticated at all. The exposure is the async path, which is opt-in behind `asyncRegistrationEnabled` but is also the *only* path a WASM main thread can use, since the synchronous ones block on a nested QEventLoop. The regression test records what the **server** saw, not that the call succeeded -- the latter was already true before the fix. It asserts on `ctx.token` rather than `ctx.principal`, because `stampVerifiedPrincipal` deliberately clears a client-asserted principal that `authenticate()` cannot vouch for, so principal is "" either way and proves nothing. Measured: "" before, "tok-495" after. While in `assignPrimaryAsync`, also moved `wire::encode` above the `_pendingAssigns` insertion. `registerModelAsync` states that invariant at length and the other two async hooks point back at it and follow it; this one did the opposite, so a throwing encode would park its onRegistered/onError in the pending map forever with no message ever sent. **#500 -- `IAuthorizer::authorize`'s contract did not describe its own call sites.** It said "Called once per `execute` envelope". It is also called for `instances` and `schemas`, both passing an **empty** `actionType`. The server is right and deliberate -- the comments at those sites explain that gating the two read channels lets a deployer refuse enumeration or schema disclosure without refusing use, and `schemas` discloses field names, bounds, rules and the payload fingerprint of every action. The stale artefact was the interface doc, which is why this is a contract correction and not a behaviour change. An implementor matching on `actionType` would hit its default arm on exactly those two disclosure verbs, and whether that fails open or closed was being decided without being told the case exists. Now stated as a table on the interface, on `@param actionType`, on `SigningAuthorizer::Policy` (same gap), and in docs/spec/session/session.md. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SxkvtHvan2fWKDDaBpqkgv
…her way (#502, #505) **#502 -- both in-flight counters leaked permanently on a throw.** `RemoteServer::dispatchExecute` reserves a slot in `_inFlightExecutes` and then runs a stretch of non-`noexcept` code before anything can decrement it: `emitMetric`, two `make_shared`, `TimeoutScheduler::schedule`, `awaitTurn`'s mutex, and `_strand.post`. The reservation's own comment ("needs no unwind on the early-return paths above") is true of *returns* and silent about *throws*. One throw left the counter permanently over-counted, and `drainedWithin()` predicates on it reaching zero -- so graceful shutdown could never succeed again for that server, and with `maxInFlightExecutes` set a slot was gone for good. Fixed with an RAII reservation that claims the same `finished` flag `complete` uses. Claiming rather than replying is the point: `dispatchMessage`'s catch is what replies on that path, so replying here too would break `handle()`'s reply-exactly-once contract -- and claiming the flag also makes an already-armed timeout a no-op, closing the second half of the bug, where a throw after the timer was scheduled produced a second `err "timeout"` for the same callId. `finished` moved above the reservation so one guard covers the whole window. `Bridge::executeVia` had the same shape: `_pendingCalls` incremented and the client deadline armed before an untried `backend->execute(...)`, which is genuinely throwing code (`serializeAction()` runs user `toJson` and glaze; `wire::encode` runs glaze). Now undone on unwind before the exception continues. **#505 -- resolved the opposite way to the obvious one, on evidence.** The issue offered two readings: the `_attachMtx` invariant holds and `registerHandlerImpl` breaks it, or the invariant is overstated. I implemented the first -- copy `contextKey` out under the lock -- and it failed "Bridge: an in-flight shared attach does not block unrelated handler registration" (tests/test_shared_instances.cpp), whose own comment says the dedicated attach mutex exists so `registerHandler()` "no longer contends for the same lock as a slow shared attach". Acquiring `_attachMtx` during registration, even briefly, reproduces exactly the regression that test was written to catch. So reading 1 is ruled out by test, and the unlocked read stays. What was missing was never the lock -- it was the statement of why the read is safe without one: the writers all operate on an already-registered binding, while this read happens *during* registration, which puts a requirement on the caller of the pre-built `registerHandler(binding)` overload (set `contextKey` first; do not mutate it concurrently with that call). Now stated at the read, on the `_attachMtx` member as an explicit carve-out from an otherwise absolute rule, and in docs/spec/core/bridge.md. Regression test for the `Bridge` half of #502 (1 before, 0 after). The `RemoteServer` half is structural: reaching it needs a throw from inside the reserved window, which no in-tree backend does on demand. Full suite: 22015 assertions, no regressions. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SxkvtHvan2fWKDDaBpqkgv
…#498, #506, #507) **#498 -- SocketServer leaked an fd and a thread handle per connection ever accepted.** `_clients`/`_clientThreads` were only ever pushed to in `acceptLoop` and cleared in `close()`; nothing removed a connection whose `clientLoop` had returned. The surviving `shared_ptr` kept the `ClientConnection` -- and its `TcpSocket` -- alive, so the fd stayed open, and every `std::thread` stayed joinable. The accumulation was per connection *ever accepted*, not per live one. `ClientConnection` now carries a `finished` flag set on every exit path, and `acceptLoop` reaps before taking on the next connection. Threads are moved out and joined after `_clientsMtx` is released -- `clientLoop`'s own teardown takes that mutex through `sendText`, so joining under it would deadlock -- and the flag is raised only after the scope guard has reclaimed the connection's models, which is why `FinishedFlag` is declared *before* the guard so it destructs last. (I had it the other way round first; the comment said "destroyed first" while C++ destroys in reverse declaration order.) The class doc and docs/spec/core/backend.md both say destruction leaves no dangling threads, and both are true -- at teardown, which is why the leak was invisible. The regression test therefore samples `/proc/self/fd` **while the server is still running**: 25 connect/disconnect cycles took the count from 11 to 38 before the fix and leave it flat after. A test that opened N connections and then destroyed the server would have passed either way. **#506 -- `~SocketBackend` took `_socketMtx` around `shutdownBoth()`.** `sendFrame` holds that lock across `_socket.sendAll()`, which loops on a blocking `::send` with no timeout, so a thread stalled against a peer that stopped reading holds it indefinitely -- and the destructor then waited on the one lock whose release requires the `shutdownBoth()` it could not reach. `SocketServer::close()` documents this exact trap and deliberately does not take its own write mutex; this is the same trap on the client side. `shutdownBoth()` is documented safe from any thread, so the lock was unnecessary as well as harmful. Not reproduced -- a stalled-peer teardown needs a peer that accepts and never reads. **#507 -- `TcpSocket::connect` was unbounded and signal-fragile.** The `::poll` sat inside the candidate loop, so `connectTimeout` was applied *per resolved address*; with `ai_family = AF_UNSPEC` several candidates are the norm ("localhost" is both ::1 and 127.0.0.1), making the worst case N x timeout while two doc comments state it as a single bound. Now one deadline for the whole call. The same poll treated `EINTR` as connect failure, so an ordinary delivered signal abandoned a candidate -- every other blocking syscall in the file already retries, and the accept loop's own comment argues for exactly that. Also clamps the `int` millisecond conversion instead of truncating a large timeout into a garbage (possibly infinite) value, and checks `getsockopt`'s return, which on failure left `soErr` at 0 and handed back a broken socket as connected. Full net suite: 983 assertions. Main suite: 22016 assertions. No regressions. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SxkvtHvan2fWKDDaBpqkgv
Formatting: one comment I inserted joined onto the sentence that followed it, leaving an over-long line in bridge.hpp's `_attachMtx` block. Ran CI's exact command (`git ls-files -z '*.hpp' '*.cpp' | xargs -0 clang-format --dry-run -Werror`) over all 803 tracked files; clean. Allowlist, audited both ways (every `source` pin lands on its own text, every "line N" in `reason` prose resolves to a real entry): - **Dropped two entries** rather than re-pinning them: callback_scope.hpp's `if (_state != nullptr)` and `_state != nullptr && ...` branches no longer exist, having been removed with #499's atomic conversion. Re-pinning a vanished branch would have quietly kept a dead exemption alive. - Re-pinned eight that merely moved. - One needed care: this branch *adds* a second `if (deadlineHandle && schedulerRef)` to `executeVia` (the #502 unwind guard), placed ahead of the existing one, so nearest-occurrence matching pinned the entry to the new line. The entry's own `reason` describes the `.then` disarm, so it is pinned there (1589) instead. Left the new guard unlisted deliberately -- if the coverage job reports it as partial, it should earn its own entry with its own reasoning rather than inherit someone else's. All three suites after the reformat: main 22016 assertions, net 983, qt 550. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SxkvtHvan2fWKDDaBpqkgv
…comments (#504) **The "public macro surface" paragraph appeared 17 times** -- registry.hpp x8, model_key.hpp x4, quantity.hpp x2, and one each in views/app/flows -- six identical lines explaining why those macros carry `// clang-format off`. Well past the rule-of-three, and provably drifting: it claims the formatter "broke a token-paste invocation apart", and four of the six files contain no `##` at all. Now one canonical statement in CONTRIBUTING.md under Formatting/linting, with a one-line pointer at each site. Eight comments that assert something the code does not do: - `wire.hpp` -- the `Envelope` discriminator list is the declared authority ("see class docstring for valid values") and omitted `"hello"`, which `makeHello()` produces and `interpretHelloReply()` consumes; docs/spec/core/wire.md does list it. Also: `interpretHelloReply` documents requiring an `"err"` kind but matches on the message alone -- documented as deliberate rather than silently tightened, since a peer old enough not to know `hello` is one whose error shape we should not depend on. - `execute_order_gate.hpp` -- "Defensive; should not happen" over a branch the file's own contract lists as a first-class element ("tolerate a gate already erased") and which tests/test_execute_order_gate.cpp names in a test title. - `logger.hpp` -- called an unlocked read of `minLevel` a data race; it is a `std::atomic`. Only `sink` (a `std::function`) is. The real reason to take the lock is that the two must be captured as a *pair*, since `setLogger` and `setLogLevel` are separate calls. - `backend.hpp` -- `_changeAware` is inserted by `createAndTrack`, which `registerModelShared` calls directly without going through `registerModel`. - `reply_router.hpp` -- "allocated and stored under different locks" describes a relationship that does not exist; the call-id counter is a lock-free atomic read outside `_mtx`, deliberately. - `ws_handshake.hpp` -- the "64 KiB safety cap" is checked before each recv, so the real bound is one 4 KiB chunk higher, which the in-tree test already says. - `socket_server.hpp` -- replies are not "marshalled back onto the owning connection's own write path"; they are written inline on the worker-pool thread under the per-connection write mutex, as the spec correctly states. **One item was reported as a defect and turned out not to be.** #504 lists `model.hpp`'s `#include "strand.hpp"` as unused. It is unused *by model.hpp* -- but removing it broke tests/test_model.cpp, which reached `morph::exec::detail::ModelId` through it. For a header-only public library, dropping a transitive include is a source-breaking change for consumers and not worth the tidiness, so the include stays with a comment saying why. The `<concepts>` gap that item flagged alongside it is real and is fixed: this file uses `std::same_as` and `concept` without including it. Suites unchanged: main 22015 assertions, net 983. Tree-wide clang-format clean. Allowlist re-pinned, audited both ways. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SxkvtHvan2fWKDDaBpqkgv
…xposed (#453) Item A asks for the scheduled campaign's survivors classified rather than counted. This does the part that can be done from evidence and says plainly how much is left. **Recorded run[2]** in scripts/mutation_survivors.json from the first scheduled-gate campaign (run 34349442137): 773 mutants, 574 killed, 199 survived, 74.26%, with per-file and per-mutator breakdowns. Not comparable to the 352-survivor figure above it -- that was measured over a larger population, before cxx_remove_void_call was excluded. **Classified 16 of 199, and closed a real gap.** Clustering by (mutator, source shape) rather than assessing them one at a time put sixteen bitwise survivors in one place, and they split two ways: - **Thirteen are provably equivalent.** Four sit inside `OpaqueIdGenerator::mix`, the Feistel round function -- and the Feistel construction is invertible for *any* round function, which `permute()`'s own doc comment already states. Mutating F preserves the only property the code claims. The other nine are hash-combine sites; nothing in the contract fixes a hash *value*, so killing them would mean asserting a hash constant, which is the "test with no reason to exist" this issue explicitly warns against. - **Three were a genuine hole, now closed.** They sit on the Feistel *structure* rather than inside it, and they survive because the existing bijection test drives counters 1..20000 -- every one of which has a **zero high half**. That test therefore passes whether or not `permute()` uses the top 32 bits at all. Simulated against the real round function over counters that do exercise the high half: `>>` -> `<<` collapses 20000 ids to **1**, and the Feistel `^` -> `|` collapses them to **1256**. Catastrophic collisions in opaque model ids -- which exist to stop a client guessing another client's id -- and invisible to the sample. New test drives counters strided by 2^32; it kills the first two. The third (`hi << 32` -> `>>`) still survives: the id degenerates to its low half, which stays distinct for these inputs, so killing it needs a probe on the id's high bits rather than a distinctness count. Recorded as such. **183 of 199 remain unclassified**, and the file says so rather than implying a finished pass. remote.hpp (82) and bridge.hpp (33) hold 58% of them and have not been assessed; the largest cluster is cxx_replace_scalar_call (92), concentrated on `.empty()`/`.size()` guards. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SxkvtHvan2fWKDDaBpqkgv
**WASM build — `std::atomic<std::shared_ptr<T>>` is not portable here.** libstdc++ implements the C++20 partial specialisation; libc++ as shipped with emscripten does not, so it falls back to the primary template and hard-errors on `is_trivially_copyable`. Both WASM legs failed. This project builds for WASM, so that type is simply unavailable, and #499's fix had to change shape. The alternatives were a mutex — which costs `token()` and `stopRequested()` their `noexcept`, on a path the Bridge calls per registration — or narrowing the contract. Narrowed it: `token()`, `guard()`, `requestStop()`, `stopRequested()` and the destructor stay mutually concurrent; **`reset()` must be externally synchronised**, which matches the usage it exists for (`onNewQuery() { _callbacks.reset(); }`, the owner thread's supersede verb) and matches what every in-tree caller already does. The within-a-generation guarantee is unchanged. `_state`'s comment records that this is a portability constraint rather than a preference, so the next reader does not "fix" it back. The regression test was asserting the guarantee that just went away, so it now pins the one actually delivered: concurrent `token()`/`stopRequested()` against `requestStop()`, with `reset()` called unraced. Verified TSan-clean (0 reports); the old racing pattern still reports 26, which is now documented caller error rather than a defect. **Header ↔ spec sync** wanted four sub-domains. All four got real content, not filler: journal.md and offline.md document the unreadable-file behaviour these commits introduce (and offline.md the durable-first mutation ordering); rational.md records that `formatRationalDecimal` is no longer one of its listed `INT64_MIN` negation sites and why that path escaped the clamp; forms.md notes where the macro-formatting rationale now lives. **clang-tidy-diff** — nine findings on changed lines, all mine: a non-const `scoped_lock`, two unchecked `operator[]` (now iterator-based), three too-short identifiers, two `#if !defined(X)` that want `#ifndef`, and a missing parenthesis in `+`/`/`. Normalised the `_WIN32` guards across all three test files while there. Suites after: main 22021 assertions, net 983. Tree-wide clang-format clean. Allowlist 23 entries, 0 stale. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SxkvtHvan2fWKDDaBpqkgv
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
…a race
CI's clang-tsan leg failed on "SocketBackend: action result delivered via then",
and the report points straight at the change I made for morph#506:
#0 TcpSocket::closeNow() tcp_socket.hpp:434
#1 TcpSocket::operator=(TcpSocket&&) tcp_socket.hpp:82
#2 SocketBackend::onDisconnected() socket_backend.hpp:494
vs
#0 TcpSocket::valid() tcp_socket.hpp:395
#1 SocketBackend::~SocketBackend() socket_backend.hpp:100
My reasoning was wrong in a specific way worth recording. I argued the lock was
unnecessary because `shutdownBoth()` is documented safe from any thread. That is
true, and it is not what `_socketMtx` was protecting at this site:
`onDisconnected()` **reassigns** `_socket` (`_socket = TcpSocket{}`, a
move-assign that closes the old fd), so an unlocked `_socket.valid()` in the
destructor races the I/O thread replacing the object, not merely using it.
`SocketServer::close()`, which I cited as precedent, does not have this shape --
it never reassigns the socket it shuts down.
Measured both ways locally under TSan, rather than taking CI's word for it:
**25 race reports without the lock, 0 with it.**
So the lock is restored and #506 stays open. Its underlying hazard is still real
-- `sendFrame` holds `_socketMtx` across a blocking, un-timed `sendAll`, so a
peer that stops reading can park the destructor -- but closing it needs a way to
reach the fd without the mutex (an atomic fd shadowing `_socket`, with its own
fd-reuse story), not simply dropping the lock. Both the code comment and the
issue now say that.
Also fixes the flaky test I added for #499, which failed on clang-debug, Windows
cl-debug, clang-asan and Valgrind. Two separate defects in it, both mine:
- `observed == 0`: a fixed 2000-iteration loop of relaxed stores finishes in
microseconds, so the reader thread could still be starting when `stop` went
up. Now driven by the reader's own progress against a deadline, and asserts
the interleaving count so a clean TSan run means something rather than
reporting success having measured nothing.
- `stopRequested() == false`: the replacement used `while`, whose condition is
checked *first* -- if the reader raced past the threshold before the first
iteration, `requestStop()` was never called at all. `do`/`while` fixes it.
This one only appeared under heavy oversubscription.
Stress-verified: 192 runs at 48x oversubscription, 0 failures (the first fix
failed 8 of 72 at 24x).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SxkvtHvan2fWKDDaBpqkgv
- `test_callback_scope.cpp` -- my own `do`/`while` (the fix for the zero-iteration bug) trips `cppcoreguidelines-avoid-do-while`. Same guarantee without it: one unconditional `requestStop()`, then the bounded progress loop. Re-stressed at 48x oversubscription, 0 failures. - `test_opaque_model_ids.cpp`, `test_file_offline_queue.cpp` -- two locals that can be `const` (`misc-const-correctness`). Checked the whole branch locally this time rather than discovering these one CI run at a time: ran clang-tidy over every changed .cpp against the repo's .clang-tidy and compile_commands.json, then filtered its 428 findings down to the 888 lines this branch actually touches. Zero remain, Qt and net sources included. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SxkvtHvan2fWKDDaBpqkgv
…duler Valgrind was the last failing leg, and it was this test again -- `observed == 0` after a full ten-second deadline. Not a memory finding: all three memcheck runs reported 0 errors and 0 bytes lost, and the step failed only because a Catch2 case did. Cause: Valgrind serialises threads, switching at syscalls and similar points. My main thread spun on `requestStop()`, which is a relaxed atomic store and offers the scheduler nothing to switch on, so the reader never ran at all. The 10s deadline expired with the two threads never having interleaved. Two changes, both about making the interleaving *happen* rather than hoping for it: - A `started` handshake, so the main thread does not begin until the reader is provably inside its loop. - `std::this_thread::yield()` in both loops. `sched_yield(2)` is a real syscall, which is exactly the switch point a serialising scheduler needs. Deadline raised to 30s, since under Valgrind everything is ~20-50x slower. This is the third distinct failure mode in one test -- thread-start latency, a zero-iteration `while`, and now scheduler starvation -- so it is verified across every environment that has bitten it rather than just the one that last failed: Valgrind 5/5 clean, 192 runs at 48x oversubscription with 0 failures, TSan clean with 0 race reports, clang-tidy clean on the new lines, full suite 22021 assertions. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SxkvtHvan2fWKDDaBpqkgv
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.
Resolves thirteen issues from the framework audit in one PR, batched to keep CI runs down. One commit per coherent fix, so the history stays reviewable.
FileActionLog::repairTornTail()truncating the whole journal to zeroFileOfflineQueue's constructor compacting an unreadable file away, + two write-ordering inversionsINT64_MINnegation UB informatRationalDecimalnormalizeLocaleNumberaccepting a sign after the decimal separatorSocketServerleaking an fd + thread per connection ever acceptedCallbackScope::reset()racing every other memberIAuthorizer::authorize's contract not describing its own call sitesMainThreadExecutorletting a non-std::exceptionescape the pumpregisterHandlerImpl's unlockedcontextKeyread~SocketBackendtaking the lockSocketServer::close()documents as a deadlockTcpSocket::connect's per-address timeout andEINTRhandlingTwo results worth reading before the diff
#505 resolved the opposite way to the obvious one. The issue offered two readings: the
_attachMtxinvariant holds and the code breaks it, or the invariant is overstated. I implemented the first — copycontextKeyout under the lock — and it failed an existing test: "Bridge: an in-flight shared attach does not block unrelated handler registration", whose own comment says the dedicated attach mutex exists soregisterHandler()"no longer contends for the same lock as a slow shared attach". Taking the lock during registration reproduces exactly the regression that test was written to catch. So reading 1 is ruled out by evidence, the unlocked read stays, and what was actually missing — the ordering argument that makes it safe, and the requirement it places on callers of the pre-built-binding overload — is now stated at the read, on the member as an explicit carve-out, and in the spec.#497 was half-reverted after checking the other side. The issue reported both a sign-injection bug and a looser-than-documented output grammar. I fixed both, then checked
DynamicForm.qml— documented as mirroring this function — and found it accepts.5,5.and.. Tightening only the C++ would have created a new divergence, which is the very thing the issue is about, and rejecting.5is a UX regression on ordinary input. The sign fix stays (QML already rejected those); the grammar is documented rather than narrowed, and narrowing both edges together is left as a maintainer call.Verification
Every fix has a regression test, and each was checked to fail without its fix — invariant 7. Measured, not asserted:
load()bypasses theFileIoOpsseam entirely.""before,"tok-495"after. Asserts onctx.token, notctx.principal— the latter is deliberately cleared whenauthenticate()cannot vouch for it, so it would have proved nothing./proc/self/fdwent 11 → 38 over 25 connect/disconnect cycles before; flat after. Samples while the server runs, because the class doc and spec are both true at teardown, which is why the leak was invisible.pendingCalls()was1before,0after.quantity.hpp:101before, silent after.Suites: main 22016 assertions, net 983, qt 550.
clang-format --dry-run -Werrorover all 803 tracked files: clean.check_spec_citations.sh: clean. Branch-coverage allowlist audited both ways, with two now-vanished entries dropped rather than re-pinned.Not resolved here
#345 (upstream Lightweight, parked), #453 (needs a scheduled mutation run to classify against), #489 site 4 (no safe fix known), #504 (comment backlog).
🤖 Generated with Claude Code
https://claude.ai/code/session_01SxkvtHvan2fWKDDaBpqkgv
Also in this PR
#504 (comment backlog), partially. The "public macro surface" paragraph was copy-pasted 17 times across six headers — well past rule-of-three, and provably drifting: it claims the formatter "broke a token-paste invocation apart", and four of those six files contain no
##at all. Collapsed to one canonical statement in CONTRIBUTING.md plus a one-line pointer at each site. Eight further stale comments fixed (wire.hpp's discriminator list omitting"hello";execute_order_gate's "should not happen" over a branch its own contract and a named test both cover;logger.hppcalling an atomic read a data race; and others).One #504 item turned out not to be a defect:
model.hpp's#include "strand.hpp"is unused by that header, but removing it broketests/test_model.cpp, which reachedmorph::exec::detail::ModelIdthrough it. For a header-only public library that is a source-breaking change for consumers; the include stays, with a comment saying why. The<concepts>gap the same item flagged is real and is fixed.#453 item A, partially. Recorded the scheduled campaign's run (773 mutants, 574 killed, 199 survived, 74.26%) and classified 16 of the 199 — thirteen provably equivalent (mutations inside the Feistel round function, which is invertible for any F, and hash-combine sites where killing them would mean asserting a hash constant), and three that were a genuine hole.
That hole is worth calling out: the existing
OpaqueIdGeneratorbijection test drives counters 1..20000, all with a zero high half, so it passes whether or notpermute()uses the top 32 bits. Simulated against the real round function over counters that do exercise it,>>→<<collapses 20000 ids to 1 and the Feistel^→|collapses them to 1256 — catastrophic collisions in ids that exist to stop one client guessing another's. New test closes it. 183 of 199 remain unclassified and the file says so.Closes
Closes #493, closes #494, closes #495, closes #496, closes #497, closes #498, closes #499, closes #500, closes #501, closes #502, closes #505, closes #507.
#506 is deliberately NOT closed. I implemented its fix, and CI'''s clang-tsan leg proved it wrong: removing the destructor'''s
_socketMtxintroduced a real data race, becauseonDisconnected()reassigns_socketrather than merely using it — so the unlocked_socket.valid()races the I/O thread replacing the object. Measured locally both ways: 25 TSan reports without the lock, 0 with it. Reverted; the issue stays open with that evidence, since its underlying hazard is real but needs an atomic fd shadowing_socket, not a dropped lock.#453 and #504 stay open for the parts listed above as remaining.