Batch: thirteen audit findings — data loss, a security gap, two leaks, and UB (#493-#502, #505-#507)

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`),

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`

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

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. |

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

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<std::string> 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. |

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

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

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<std::shared_ptr<void>> 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<detail::HandlerBinding> 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;