diff --git a/docs/spec/core/backend.md b/docs/spec/core/backend.md index e919d797..d2df9a56 100644 --- a/docs/spec/core/backend.md +++ b/docs/spec/core/backend.md @@ -846,8 +846,8 @@ nothing for a caller that never uses it. ### Graceful shutdown (`beginShutdown()` / `drainedWithin()`) -`beginShutdown()` enters shutdown: every subsequent `register` and `execute` -envelope is rejected with `err "server shutting down"` (checked once, at the +`beginShutdown()` enters shutdown: every subsequent `register`, `attach` and +`execute` envelope is rejected with `err "server shutting down"` (checked once, at the top of `dispatchMessage`, before any other validation — including the shutdown check happening before authorization or registry lookups run); `deregister` (and any other envelope kind) is still served so clients can @@ -1661,7 +1661,7 @@ inside the class calls `close()` — no thread it joins can be waiting on it. | `setSupportedVersionRange(min, max)` | Sets the inclusive protocol-version range advertised on `hello`. Defaults to `{kProtocolVersion, kProtocolVersion}`. Throws `std::invalid_argument` if `min > max`. Thread-safe. | | `health()` | `[[nodiscard]] HealthStatus health() const` — snapshot of readiness/liveModels/inFlight. Cheap; safe from any thread. See [observability.md](observability.md). | | `setHealthHandler(handler)` | `void setHealthHandler(std::function)` — fires immediately with the current status, and again whenever readiness changes (currently only `beginShutdown()` triggers a change); `nullptr` clears without firing. | -| `beginShutdown()` | Enters shutdown: subsequent `register`/`execute` envelopes get `err "server shutting down"`; `deregister` still served. Idempotent, irreversible. Flips `health().ready` to `false` and re-invokes any installed health handler. | +| `beginShutdown()` | Enters shutdown: subsequent `register`/`attach`/`execute` envelopes get `err "server shutting down"`; `deregister` still served. A client therefore cannot re-attach to a shared instance during the drain window. Idempotent, irreversible. Flips `health().ready` to `false` and re-invokes any installed health handler. | | `drainedWithin(deadline)` | `[[nodiscard]] bool drainedWithin(std::chrono::milliseconds deadline)` — blocks (condition-variable wait, not a poll) until every in-flight `execute` has replied or `deadline` elapses. Returns `true`/`false` accordingly. | ### `SimulatedRemoteBackend` diff --git a/docs/spec/core/bridge.md b/docs/spec/core/bridge.md index 73726c0f..73759e82 100644 --- a/docs/spec/core/bridge.md +++ b/docs/spec/core/bridge.md @@ -85,10 +85,11 @@ the `ModelId` value the active backend assigned; 0 = unbound. The last three fields are the state behind [`isBound()` / `whenBound()`](#registration-readiness--isbound--whenbound). -`registrationInFlight` is `true` from the moment `registerHandlerImpl` hands -the binding's initial registration to `IBackend::registerModelAsync` (and that -call returns `true`) until the resulting `onRegistered`/`onError` callback -resolves; `registrationWaiters` holds the callbacks queued while it is. +`registrationInFlight` is `true` from just *before* `registerHandlerImpl` calls +`IBackend::registerModelAsync` until the resulting `onRegistered`/`onError` +callback resolves. It is set unconditionally on every path, the synchronous +fallback included — that fallback does not *leave* it set, because it resolves +the waiters and clears the flag before returning; `registrationWaiters` holds the callbacks queued while it is. Both are guarded by `registrationMtx` — deliberately a mutex of the binding's own, not `Bridge::_mtx` or `_attachMtx`, because a waiter may be queued or resolved from either the registering thread or the backend's reply-delivering diff --git a/docs/spec/core/shared_instances.md b/docs/spec/core/shared_instances.md index 55ab64f8..56ddb8a8 100644 --- a/docs/spec/core/shared_instances.md +++ b/docs/spec/core/shared_instances.md @@ -422,18 +422,15 @@ dispatch's completion instead of issuing its own; tracked as a follow-up. Until then, a caller should not fire the same keyed action twice back-to-back before the first settles. -**Not covered: the result-keyed *promote* step is still synchronous.** This -section made the **bind** half of a result-keyed action async -(`ensureBoundAsync` → `registerModelSharedAsync`). The **promote** half did -not change: `Bridge::assignHandlerPrimary` still calls the synchronous -`IBackend::assignPrimary`, which on `QtWebSocketBackend` is a `sendSync` — -a nested `QEventLoop`. There is no `assignPrimaryAsync`. So a **WASM client -dispatching a result-keyed creating action** (a `CreatePoll`-shaped action: -create the entity, adopt the key its result carries) still blocks, and still -aborts the page, at the promote step — after the bind step this section fixed -already succeeded. Payload-keyed actions (`OpenPoll{pollId}`-shaped, the -attach path) are fully covered and do not block. Giving `assignPrimary` an -async form is a separate follow-up. +**The result-keyed *promote* step has since been covered too.** This section +made the **bind** half of a result-keyed action async (`ensureBoundAsync` → +`registerModelSharedAsync`). At the time of writing the **promote** half still +called the synchronous `IBackend::assignPrimary` — a `sendSync`, and so a nested +`QEventLoop`, on `QtWebSocketBackend` — which blocked and aborted a WASM page. +That is no longer true: `IBackend::assignPrimaryAsync` exists +([backend.md](backend.md#promotion--assignprimaryasync)), +`QtWebSocketBackend` overrides it, and `Bridge::assignHandlerPrimary` prefers it, +falling back to the synchronous call only when a backend returns `false`. ## Ownership and authorization diff --git a/docs/spec/forms/forms.md b/docs/spec/forms/forms.md index 33ff09bc..338befec 100644 --- a/docs/spec/forms/forms.md +++ b/docs/spec/forms/forms.md @@ -221,7 +221,7 @@ output of `glz::write_json_schema()` to add seven annotation groups: | Annotation | Scope | Contents | |---|---|---| -| `required` | Top-level, and every nested-aggregate object schema (see [Nested aggregates (recursive, cycle-guarded)](#nested-aggregates-recursive-cycle-guarded)) | Array of field names that are **not** `std::optional<...>` and not listed in `A::optionalFields`. | +| `required` | Top-level, and every nested-aggregate object schema (see [Nested aggregates (recursive, cycle-guarded)](#nested-aggregates-recursive-cycle-guarded)) | Array of field names that are **not** `std::optional<...>` and not listed in `A::optionalFields`. Always written, overwriting whatever glaze produced: glaze never derives `required` from member types — it emits one only where a type declares `meta::required` (and for a tagged variant's discriminator) — so morph does not rely on its absence. | | `x-order` | Every property | The member's declaration index (0‑based), so a renderer lays fields out in declaration order regardless of JSON key ordering. | | `x-decimalPlaces` | `Quantity` properties | The field's declared precision (`Quantity::declaredDecimals`). | | `x-unitAlternatives` | `Quantity` properties | Convertible display/entry units derived from `UnitTraits::relations`, each with `{id, display, decimals, num, den}` — `id`/`display`/`decimals` come from the alternative unit's `UnitMeta`, and `num`/`den` are the exact alternative-to-canonical ratio. Omitted entirely when the field's unit declares no convertible units. | diff --git a/docs/spec/journal/journal.md b/docs/spec/journal/journal.md index e6491bc7..1edffde5 100644 --- a/docs/spec/journal/journal.md +++ b/docs/spec/journal/journal.md @@ -444,8 +444,16 @@ already happened. ## IActionLog — the storage interface A pure-virtual interface for durable, append-only storage of action entries. -Entries are never removed by the framework — this is a permanent record, unlike -`morph::offline::IOfflineQueue` (whose `markDone()` deletes items once retried). +No *entry-level* deletion API exists — there is nothing corresponding to +`morph::offline::IOfflineQueue::markDone()`, which deletes items once retried. +Two operations on the *shipped file implementation* — not on this interface — +do change what a subsequent `entries()` returns, and neither is an exception to +the append-only rule so much as a boundary of it: +`FileActionLog::`[`rotate()`](#rotation-and-retention), which seals the active +file and reopens an empty one, and `FileActionLog`'s private +`repairTornTail()`, which discards a truncated trailing record and runs only +from that class's constructor. An `IActionLog` implementation over another sink +owes neither. | Method | Signature | Purpose | |---|---|---| @@ -1138,7 +1146,7 @@ and `RemoteServer::setLogProvider(LogProvider)`, declared in `remote.hpp`. See |---|---|---| | `LogEntry` is a plain aggregate | **No `glz::meta`** | Same automatic reflection `BRIDGE_REGISTER_ACTION` uses; no manual schema maintenance. | | Error path sharing | **`detail::throwOnGlazeError` for both `toJson`/`fromJson`** | `fromJson`'s failure is easy to test (malformed input); `toJson`'s is structurally unreachable for `LogEntry`. Routing both through one non-template function means the same compiled branch covers both, so `toJson`'s error path is exercised by `fromJson`'s tests. | -| Entries are never removed | **Append-only, no deletion API** | Permanent audit trail — unlike `IOfflineQueue` whose `markDone()` deletes retried items. | +| No entry-level deletion | **Append-only, no per-entry deletion API** | Permanent audit trail — unlike `IOfflineQueue` whose `markDone()` deletes retried items. `FileActionLog`'s `rotate()` and private `repairTornTail()` operate on the file, not on entries, and are not part of `IActionLog`. | | Default log is a function-local static | **`detail::defaultActionLogState()` returns a `pair`** | Safe regardless of translation-unit init order, unlike a namespace-scope global. | | `SessionLog::checkpoint` advances the watermark *before* forwarding | **At-most-once / forward-only** | A checkpoint is a forward-only commit point, not a transaction to retry: the watermark advances first, so a throwing durable sink drops that batch permanently. (`IOfflineQueue`'s retry semantics do *not* carry over — the shared shape is superficial.) | | Checkpoint watermark is a committed-`seq` threshold, not an `_all` index | **Track committed state by entry identity** | `seq` is assigned once and never reused, so it stays a valid commit marker even as coalescing forwards fewer entries than it consumes and as `undoLast()` pops tail entries. A raw index into the mutable `_all` vector cannot: it silently shifts meaning when entries are removed, which is the root of the undo/coalescing incoherence this replaces. | diff --git a/docs/spec/offline/offline.md b/docs/spec/offline/offline.md index 8eb837f5..81169d64 100644 --- a/docs/spec/offline/offline.md +++ b/docs/spec/offline/offline.md @@ -124,8 +124,8 @@ The payload format is the caller's choice — JSON, binary-hex, plain text, etc. #### `idempotencyKey`: deduping against the journal -`QueueItem::id` is **queue-local** — a durable queue re-presents the same logical -op with a fresh `id` after a restart, and the journal's `seq` is journal-local, +`QueueItem::id` is **queue-local** — both shipped durable queues re-present the +*stored* `id` after a restart, and the journal's `seq` is journal-local, so the two subsystems share no identity. That is exactly the seam where an op can be **double-applied**: the offline queue and the journal can each replay the same logical operation with nothing to recognise it as already-applied. @@ -559,7 +559,7 @@ and calls a caller-supplied `ReplayFunction` for each item. | ctor | `SyncWorker(IOfflineQueue&, ReplayFunction, DeadLetterSink = nullptr)` | References the queue and the replay callable; the sink is an optional third argument. | | ctor | `SyncWorker(IOfflineQueue&, DetailedReplayFunction, DeadLetterSink = nullptr)` | Same, taking the three-outcome callable. The two overloads are unambiguous — `ReplayOutcome` is a scoped enum, so neither return type implicitly converts to the other. The boolean overload adapts into this one, so `run()` implements a single contract. | | `run()` | `SyncResult run()` | Drains the queue and replays each item. Concurrent calls are serialised by an internal mutex. Returns immediately if `stop()` was called before acquiring the lock. Emits the `queueDepth` metric once, with the drained item count, before replaying (see [observability.md](../core/observability.md)). | -| `stop()` | `void stop()` | Signals an in-progress `run()` to stop after the current item. One-shot — the flag resets at the start of the next `run()`. | +| `stop()` | `void stop()` | Signals an in-progress `run()` to stop after the current item. `run()` clears the flag at its start — but a `stop()` landing *during* a run leaves it set on return, so the next `run()` takes its early-out and drains nothing; work resumes on the run after that. | **Retry & dead-letter (hard-coded cap, durable count):** @@ -744,7 +744,7 @@ calling thread. | Enumerator | Meaning | |---|---| -| `Reconnected` | Backend reopened, made active, context bound, queue replay invoked. | +| `Reconnected` | Backend reopened, made active, context bound. Replay is invoked only if `shouldContinue()` still holds at that point — `Reconnected` can be returned without replaying. | | `GaveUp` | Exhausted `maxAttempts` without a successful reconnect; stayed offline. | | `Aborted` | `shouldContinue()` returned false before any reconnect attempt. | diff --git a/docs/spec/util/rational.md b/docs/spec/util/rational.md index 365fa021..8dc633d8 100644 --- a/docs/spec/util/rational.md +++ b/docs/spec/util/rational.md @@ -188,16 +188,21 @@ 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. -- **`canonicalise`** — flips sign for a negative denominator (`numerator = - -numerator`) and takes `absoluteNumerator = numerator < 0 ? -numerator : - numerator`; both negate `INT64_MIN`. This is the shared sink for every - constructor and operator, so any path that lets `INT64_MIN` reach - canonicalisation is unsafe. - -Only the wire codec (`setWire`) defends against this: it maps an `INT64_MIN` +- **`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 + unsigned arithmetic. There is no `absoluteNumerator` local any more. Since it + is the shared sink for every constructor and operator, a value that reaches it + is safe. + +The wire codec (`setWire`) also defends independently: it maps an `INT64_MIN` `num`/`den` to `-INT64_MAX` *before* constructing, so untrusted input never -negates the trap value. In-code call sites get no such guard — keep operands -well inside the envelope above. +reaches the trap value at all. + +The entry points that do **not** canonicalise are where the hazard remains — the +whole-integer `Rational{value, DecimalPlaces{n}}` constructor retains its +numerator verbatim, and `numerator` is a public member. See morph#496 for a +confirmed UB site reached that way. ### Checked arithmetic diff --git a/include/morph/core/bridge.hpp b/include/morph/core/bridge.hpp index 0182aead..98cf593f 100644 --- a/include/morph/core/bridge.hpp +++ b/include/morph/core/bridge.hpp @@ -206,12 +206,13 @@ struct HandlerBinding { /// @brief Registration-settled seam (see `Bridge::whenBound`). /// - /// `registrationInFlight` is `true` from the moment `registerHandlerImpl` - /// hands this binding's initial registration to - /// `IBackend::registerModelAsync` (and that call returns `true`) until its - /// `onRegistered`/`onError` callback resolves — the synchronous fallback - /// path never sets it, since that call has already returned bound (or - /// thrown) by the time anyone could observe it in flight. `whenBound()` + /// `registrationInFlight` is `true` from just *before* `registerHandlerImpl` + /// calls `IBackend::registerModelAsync` until that call's + /// `onRegistered`/`onError` callback resolves. It is set unconditionally on + /// every path, the synchronous fallback included — see the comment at the + /// assignment for why it must be set before the backend call rather than + /// after. The fallback does not *leave* it set: it resolves the waiters + /// (and clears the flag) before returning. `whenBound()` /// checks it to distinguish "an async reply is coming, queue a waiter" /// from "nothing is in flight, resolve false now". `registrationWaiters` /// holds callbacks queued by `whenBound()` while `registrationInFlight` is @@ -340,8 +341,11 @@ inline std::optional claimHandoff(AsyncDispatchHandoff& handoff) /// question. /// /// @par Why a blocking gate is acceptable here -/// `~Bridge` waits only for work that is provably non-blocking and bounded: -/// the sole guarded region is `~BridgeHandler`'s `deregisterHandler` call, and +/// `~Bridge` waits only for work that is provably non-blocking and bounded. +/// There are three guarded regions: `~BridgeHandler`'s `deregisterHandler` +/// call, `executeVia`'s `.then` continuation around `onResult`, and +/// `registerHandlerImpl`'s async `onRegistered` callback. The argument below is +/// about the first; each of the other two carries its own at its call site. /// `IBackend::deregisterModel` never blocks on another thread on any shipped /// backend — `LocalBackend` erases map entries under its own mutex, /// `SimulatedRemoteBackend` runs the envelope inline via @@ -368,8 +372,9 @@ struct BridgeLifetime { /// `switchBackend()` is called, enabling seamless local ↔ remote transitions. /// /// @par Thread safety -/// All public methods are thread-safe. `executeVia()` uses a lock-free snapshot -/// of the backend pointer so it does not block `switchBackend()`. +/// All public methods are thread-safe. `executeVia()` takes a short snapshot of +/// the backend `shared_ptr` under the dedicated `_backendMtx` (never `_mtx`), so +/// it does not block `switchBackend()`. class Bridge { public: /// @brief Constructs a bridge that dispatches through @p backend. @@ -607,10 +612,12 @@ class Bridge { } std::exception_ptr failure; { - // contextKey/primary are plain std::strings that five - // other sites read under `_attachMtx`; publishing them - // without it would be a data race, not just a stale - // read. + // 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.) std::scoped_lock const guard{_attachMtx}; auto pinned = weakBackend.lock(); if (!pinned || pinned != loadBackend()) { @@ -1363,8 +1370,9 @@ class Bridge { /// @brief Dispatches @p action against the model identified by @p binding. /// - /// Uses a lock-free snapshot of the backend and model id so the call does - /// not block `switchBackend()`. If a backend switch happens concurrently, + /// Snapshots the backend `shared_ptr` under `_backendMtx` and reads the + /// binding's `currentId` lock-free, so the call does not block + /// `switchBackend()`. If a backend switch happens concurrently, /// the old backend still exists (its `shared_ptr` refcount is > 0) and the /// call either succeeds or fails with "model not found" — both are safe. /// @@ -1675,11 +1683,13 @@ class Bridge { /// @brief Weak observer of this bridge's lifetime, handed to each handler. /// - /// A `BridgeHandler` checks this in its destructor: if the token is no longer - /// active the `Bridge` is already gone, so it skips deregistration instead of - /// dereferencing a dangling `Bridge&`. The bridge must still outlive its - /// handlers for normal `execute`/`set` calls; this only makes the *teardown* - /// order-independent so a mis-ordered destruction is defined behaviour. + /// Note `~BridgeHandler` does **not** use this: gating a *call into* the + /// bridge needs `lifetimeGate()`'s `detail::BridgeLifetime`, because a token + /// answers only advisorily and a member call made a few instructions after a + /// stale "active" runs on destroyed memory (morph#486). A token is the right + /// tool for *declining work*, not for keeping an object alive across a call. + /// The bridge must still outlive its handlers for normal `execute`/`set` + /// calls; the gate only makes *teardown* order-independent. /// /// The bridge is the framework's own first consumer of the primitive every /// caller now gets (docs/spec/core/callback_scope.md). It uses only the @@ -2000,7 +2010,7 @@ struct NoSharing {}; /// A shared handler that only ever runs *keyless* actions never attaches, and /// its `execute` fails fast with "handler not bound": there is no instance to /// run against and inventing a private one would silently defeat the sharing -/// the caller asked for. Attach first — see docs/planned/shared_model_instances.md. +/// the caller asked for. Attach first — see docs/spec/core/shared_instances.md. struct AllowShared {}; /// @brief RAII wrapper that binds a single model type to a `Bridge`. diff --git a/include/morph/core/model_key.hpp b/include/morph/core/model_key.hpp index cdd7166a..251e0ae6 100644 --- a/include/morph/core/model_key.hpp +++ b/include/morph/core/model_key.hpp @@ -27,7 +27,7 @@ /// /// Keys travel the wire as strings (`wire::Envelope::primary`) regardless of /// their C++ type, so the directory in `RemoteServer` needs exactly one map type -/// rather than one per key type. See docs/planned/shared_model_instances.md. +/// rather than one per key type. See docs/spec/core/shared_instances.md. namespace morph::model { @@ -84,9 +84,10 @@ using UnwrappedKeyOf = std::remove_cvref_t())>; /// @brief The key type of a model, when one has been declared *for* it. /// -/// Specialised by `BRIDGE_KEY_FROM`, which deduces the type from the action -/// member it is given — so a model does not have to say anything about keys -/// inside its own class. The primary template is deliberately empty: a model +/// Specialised by `BRIDGE_MODEL_KEY` (and `BRIDGE_MODEL_KEY_FROM_RESULT`); +/// `BRIDGE_KEY_FROM` emits only `ActionKeyTraits` and deliberately leaves this +/// alone, since the model's key type is already established by its one +/// `BRIDGE_MODEL_KEY` line. The primary template is deliberately empty: a model /// with neither this specialisation nor a nested alias is simply unkeyed. /// @tparam Model Concrete model type. template @@ -108,8 +109,9 @@ concept DeducedKey = /// @brief Satisfied by model types that have a primary key, however it was named. /// /// Two ways in, and neither requires touching the model's own class body beyond -/// the first: a nested `PrimaryKey` alias, or a `BRIDGE_KEY_FROM` declaration -/// that deduces the type from the action field carrying it. Following +/// the first: a nested `PrimaryKey` alias, or a `BRIDGE_MODEL_KEY` declaration, +/// which deduces the type from the model field it is given +/// (`MemberTypeOf`). Following /// `morph::forms`' standing rule — *infer by default, declare to override* — a /// nested alias wins when both are present, which is what lets a model whose /// key type differs from the field's type (an `int` column keyed as a diff --git a/include/morph/core/registry.hpp b/include/morph/core/registry.hpp index b55e5f31..28b50110 100644 --- a/include/morph/core/registry.hpp +++ b/include/morph/core/registry.hpp @@ -396,7 +396,7 @@ template // WARNING for whoever next measures this file's coverage: this line // can read as "covered" in a merged/aggregated coverage report even // though the branch above is genuinely never taken by any real test. - // tests/test_registry_schema_forgery.cpp's WireSchemasUnsatisfiable + // tests/test_wire_schemas.cpp's WireSchemasUnsatisfiable // fixture (a different, deliberately-closed finding) throws partway // through this function, after llvm-cov's per-function body-region // counter has already incremented for that instantiation -- llvm-cov diff --git a/include/morph/core/remote.hpp b/include/morph/core/remote.hpp index 13cd88a5..3fee060c 100644 --- a/include/morph/core/remote.hpp +++ b/include/morph/core/remote.hpp @@ -288,7 +288,10 @@ class RemoteServer : public std::enable_shared_from_this { /// to call from a thread that *is* the worker pool — for example, from a /// `BridgeHandler` constructor invoked from inside an action handler. /// - /// Only safe for control messages (`register`, `deregister`). An `execute` + /// Safe for every kind except `execute` — `register`, `deregister`, + /// `attach`, `assign`, `instances`, `schemas` and `hello` all route through + /// here (see `SimulatedRemoteBackend` below, which uses all seven). An + /// `execute` /// envelope posts to the strand and produces its reply asynchronously, after /// this synchronous call has already returned and destroyed the local reply /// buffer the deferred callback would write into. To keep that from becoming a @@ -334,7 +337,7 @@ class RemoteServer : public std::enable_shared_from_this { /// Peeks at @p msg's `kind`/`modelId` — a cheap, best-effort decode, thrown /// away immediately either way — and, for an `execute` naming a `modelId`, /// takes an execute-ordering ticket (see `_executeGate`'s own doc comment - /// on the class-private members above, and + /// on the class-private members, and /// `morph::backend::detail::ExecuteOrderGate::take`) *before* posting to /// `_pool`, so two same-model `execute`s /// posted back-to-back always take their tickets in call order — the @@ -397,10 +400,13 @@ class RemoteServer : public std::enable_shared_from_this { /// @brief Reclaims every model still registered under @p cid, then drops the scope. /// /// Call once the transport observes the connection is gone (disconnect, - /// close, error). Erases every surviving model in `cid`'s scope from the - /// registry exactly as an explicit `deregister` would (so a later - /// `execute` against one of those ids replies `err "model not found"`), - /// then drops the scope itself. + /// close, error). Releases exactly as many references as this connection + /// held, exactly as an explicit `deregister` would, then drops the scope + /// itself. A *private* instance (count 1, no directory entry) is erased + /// outright, so a later `execute` against its id replies + /// `err "model not found"`; a *shared* instance another connection is still + /// attached to survives, and an `execute` against it still succeeds — see + /// `releaseInstanceLocked`'s early return. /// /// Idempotent: `cid == 0`, an unknown `cid`, or a `cid` already closed is a /// no-op. Deliberately does **not** consult `IAuthorizer` — this is the @@ -443,8 +449,11 @@ class RemoteServer : public std::enable_shared_from_this { using LogProvider = std::function(std::string_view modelType, std::string_view contextKey)>; - /// @brief Installs @p provider, consulted on every `register` envelope whose - /// `contextKey` is non-empty. + /// @brief Installs @p provider, consulted whenever an instance is + /// constructed with a non-empty `contextKey` — every `register` + /// envelope, and every `attach` that misses the shared directory and + /// therefore creates the instance (both reach + /// `attachLogIfConfigured`). /// /// This is what closes the gap `IModelHolder::attachActionLog` leaves open /// for remote topologies: `RemoteServer` owns the actual model instances for @@ -546,9 +555,11 @@ class RemoteServer : public std::enable_shared_from_this { } } - /// @brief Enters shutdown: from now on, `register` and `execute` envelopes - /// are rejected with `err "server shutting down"`. `deregister` is - /// still served so clients can tear down cleanly. + /// @brief Enters shutdown: from now on, `register`, `attach` and `execute` + /// envelopes are rejected with `err "server shutting down"`. + /// `deregister` is still served so clients can tear down cleanly. + /// Note `attach` is refused too, so a client cannot re-attach to a + /// shared instance during the drain window. /// /// Idempotent — safe to call more than once, and safe to call while /// `handle()`/`handleInline()` calls are concurrently in flight on other @@ -1345,9 +1356,9 @@ class RemoteServer : public std::enable_shared_from_this { } } if (!holder) { - // The one path this whole mechanism exists to keep fast (finding - // 035, and the reverted first attempt this doc comment on the - // class-private members describes): a lookup against a modelId + // The one path this whole mechanism exists to keep fast (and the + // reverted first attempt the doc comment on the class-private + // members describes): a lookup against a modelId // that is not (or no longer) live must resolve immediately, // never waiting on some other, unrelated model's strand — this // ticket is released right here, before any wait could ever be @@ -1462,8 +1473,8 @@ class RemoteServer : public std::enable_shared_from_this { // script's own module docstring on its scan scope). This is // the single call site that produces this message, so the // typo-drift risk a shared constant guards against doesn't - // apply here the way it does for the consumer-side - // comparisons below. + // apply here the way it does for the consumer-side comparison + // in `core/detail/reply_router.hpp`. timeoutHandle = _timeoutScheduler->schedule(limits.executeTimeout, [complete, callId]() mutable { complete(::morph::wire::encode(::morph::wire::makeErr("timeout", callId))); }); @@ -1714,8 +1725,9 @@ class RemoteServer : public std::enable_shared_from_this { std::atomic _inFlightExecutes{0}; std::unique_ptr<::morph::async::detail::TimeoutScheduler> _timeoutScheduler; // Set once by beginShutdown() and never cleared — there is no - // un-shutdown. Checked at the top of dispatchMessage() for register and - // execute envelopes only; deregister and any other kind are unaffected. + // un-shutdown. Checked at the top of dispatchMessage() for register, + // attach and execute envelopes; deregister and any other kind are + // unaffected. std::atomic _shuttingDown{false}; // Readiness flag for health(). Flipped to false exactly once, by // beginShutdown() — there is no un-shutdown, so once false it stays false. diff --git a/include/morph/core/strand.hpp b/include/morph/core/strand.hpp index 14f08c7c..65930a23 100644 --- a/include/morph/core/strand.hpp +++ b/include/morph/core/strand.hpp @@ -94,8 +94,10 @@ class StrandExecutor { // out from under us (the erase needs _mapMtx too, and never fires // while pending is non-empty). // - // Lock order is _mapMtx → strand->mtx, matching scheduleNext's - // scoped_lock{_mapMtx, strand->mtx}. A freshly created strand's mtx + // Lock order is _mapMtx → strand->mtx, matching scheduleNext — + // which acquires the two sequentially rather than with one + // scoped_lock over both, precisely to keep this order (see its own + // comment). A freshly created strand's mtx // is uncontended; an existing strand's mtx can only be held // elsewhere under the same _mapMtx-first order, so no deadlock. std::scoped_lock const mapLock{_mapMtx}; diff --git a/include/morph/core/timeout_scheduler.hpp b/include/morph/core/timeout_scheduler.hpp index ff3509ee..550d3d6f 100644 --- a/include/morph/core/timeout_scheduler.hpp +++ b/include/morph/core/timeout_scheduler.hpp @@ -303,7 +303,8 @@ class TimeoutScheduler { /// @brief Held by `shared_ptr` so a browser timer that outlives this /// object detects that fact instead of writing to freed storage — - /// the same weak-token pattern as `morph::bridge::Bridge::_liveness`. + /// the same weak-token pattern as `morph::bridge::Bridge`'s + /// `_callbacks` `CallbackScope` (exposed as `Bridge::liveness()`). std::shared_ptr _state{std::make_shared()}; }; diff --git a/include/morph/core/wire.hpp b/include/morph/core/wire.hpp index dd3fbe10..461b0d4a 100644 --- a/include/morph/core/wire.hpp +++ b/include/morph/core/wire.hpp @@ -168,7 +168,7 @@ inline Envelope makeRegister(std::string typeId, std::string contextKey = {}) { /// is recorded with no owner principal, so `IAuthorizer::authorizeInstance`'s /// documented `ownerPrincipal == ctx.principal` policy does not lock the second /// client out of an instance the first created — see -/// docs/planned/shared_model_instances.md. +/// docs/spec/core/shared_instances.md. /// /// @param typeId Model type id to register or attach to. /// @param primary Canonical string encoding of the instance's primary key. @@ -402,8 +402,10 @@ struct EscapingWriteOpts : glz::opts { /// /// @p message may embed untrusted content (e.g. an unrecognized `Envelope::kind` /// or a caught exception's `what()`) — any raw control byte it contains is -/// replaced with a `\xHH` placeholder so the encoded envelope is always valid, -/// re-decodable JSON (see `detail::sanitizeControlChars`). +/// replaced with a `\xHH` placeholder. This is *output sanitization*, not JSON +/// validity: `encode`'s `detail::EscapingWriteOpts` already guarantees validity +/// for every field. What this stops is a raw control byte reaching a terminal or +/// log that interprets it (see `detail::sanitizeControlChars`). inline Envelope makeErr(std::string message, uint64_t callId = 0) { Envelope env; env.kind = "err"; @@ -414,7 +416,7 @@ inline Envelope makeErr(std::string message, uint64_t callId = 0) { /// @brief The `err` reply message `RemoteServer` sends when /// `LimitPolicy::executeTimeout` fires server-side (see -/// `RemoteServer::execute`'s `_timeoutScheduler` path). +/// `RemoteServer::dispatchExecute`'s `_timeoutScheduler` path). /// /// `"timeout"` exactly is the documented wire contract (`docs/spec/core/ /// backend.md`'s `executeTimeout` row, `docs/spec/core/completion.md`'s diff --git a/include/morph/forms/app.hpp b/include/morph/forms/app.hpp index d8daf8fc..cb91678f 100644 --- a/include/morph/forms/app.hpp +++ b/include/morph/forms/app.hpp @@ -76,8 +76,9 @@ struct WizardScreen { } }; -// A ViewScreen counterpart (kind: "view") belongs here once -// docs/planned/gui_collections_views.md's ViewTraits exists. appSchemaJson +// A ViewScreen counterpart (kind: "view") could be added here: +// `morph::views::ViewTraits` (forms/views.hpp) now exists, so nothing +// blocks it. appSchemaJson // below only requires each screen type to expose id()/kind()/ref(), so adding // it later needs no change to appSchemaJson itself — this reference demo // therefore only exercises "form" and "wizard" screens (see diff --git a/include/morph/forms/forms.hpp b/include/morph/forms/forms.hpp index 209deed9..a2f35ea5 100644 --- a/include/morph/forms/forms.hpp +++ b/include/morph/forms/forms.hpp @@ -11,15 +11,24 @@ /// auto-generated GUI. It builds on glaze's `write_json_schema` (which /// already contributes types, `$defs`, per-field metadata declared via /// `glz::json_schema`, and the `ExtUnits` stamped by -/// `morph::units::Quantity`) and closes the two gaps glaze leaves open: -/// -/// - **`required`** — glaze's schema writer emits no `required` array at -/// all. `schemaJson()` derives one: a member is *required* unless it is -/// a `std::optional<...>` or its name is listed in the action's opt-out -/// list (see below). -/// - **`x-decimalPlaces`** — for `Quantity` members, the unit's default -/// decimal count from `UnitTraits`, so a client knows the input step -/// without hardcoding unit knowledge. +/// `morph::units::Quantity`) and closes the gaps glaze leaves open. The list +/// below is the main surface, not an exhaustive enumeration of every key +/// emitted: +/// +/// - **`required`** — under the options `schemaJson()` uses, glaze derives no +/// `required` entries from member types: `glz::requires_key` only returns +/// `true` for a member when `meta::requires_key` says so or when +/// `Opts.error_on_missing_keys` is set, and morph sets neither. (A type +/// declaring `meta::required`, and a tagged variant's discriminator, do +/// still get one.) +/// `schemaJson()` always writes its own, overwriting whatever the schema +/// writer did or did not produce: a member is *required* unless it is a +/// `std::optional<...>` or its name is listed in the action's opt-out list +/// (see below). +/// - **`x-decimalPlaces`** — for `Quantity` members, the field's *declared* +/// precision (`Quantity::declaredDecimals`, which defaults to the +/// unit's `UnitTraits` default but is overridable per member), so a client +/// knows the input step without hardcoding unit knowledge. /// - **`x-order`** — the member's declaration index on every property, so a /// renderer can lay fields out in declaration order (JSON object key order /// is not reliable once schemas pass through DOMs/maps). @@ -666,7 +675,7 @@ template } /// @brief Constraint for the comparison rule/condition kinds (`greater`, -/// `greaterOrEqual`, `less`, `lessOrEqual`, added in a later task): an +/// `greaterOrEqual`, `less`, `lessOrEqual`): an /// `EmptyCapableField` whose engaged value (`operator*()`) is three-way /// comparable to itself — satisfied by `Quantity` (dereferences to /// `math::Rational`) and `morph::time::Timestamp` (dereferences to @@ -720,8 +729,9 @@ inline constexpr bool isLiteralString> = true; /// `has_value()`, not `hasValue()` (see forms.md's `allRequiredEngaged` /// "two exclusions" note). The cross-field rule vocabulary's engagement /// checks (`engaged`, `notEngaged`, `requiredWhen`, and the membership rules -/// added in a later task) accept either kind of field, since the planned -/// spec's own worked example ranges an `exactlyOneOf` over two plain +/// `exactlyOneOf`/`atLeastOneOf`/`mutuallyExclusive`) accept either kind of +/// field, since docs/spec/forms/forms.md's worked example ranges an +/// `exactlyOneOf` over two plain /// `std::optional` fields. template concept EngageableField = EmptyCapableField || detail::isStdOptional; @@ -1040,10 +1050,14 @@ template concept RuleLiteral = std::same_as || std::same_as || std::same_as || std::same_as || detail::isLiteralString; -/// @brief Largest magnitude an IEEE-754 double holds exactly: 2^53. +/// @brief The largest N such that *every* integer in `[0, N]` is exactly +/// representable as an IEEE-754 double: 2^53. /// -/// A JSON number beyond this cannot survive `JSON.parse` intact, so a bound -/// above it needs an exact companion the renderer can read instead. +/// Not "the largest value a double holds exactly" — 2^60 is exact too. What +/// stops at 2^53 is the *contiguous* range: past it, consecutive integers start +/// sharing a representation, so an integer bound above it cannot be relied on to +/// survive `JSON.parse` intact and needs an exact companion the renderer can +/// read instead. inline constexpr std::uint64_t kExactDoubleLimit = 9007199254740992ULL; /// @brief Signed spelling of `kExactDoubleLimit`, for the negative bound. @@ -1203,8 +1217,8 @@ struct RequiredWhen { /// @tparam A Action type (deduced). /// @tparam Cond Condition node type (deduced). /// @param field Pointer to the member that becomes conditionally required. -/// @param when The condition node (`engaged(...)`, `notEngaged(...)`, or — -/// starting a later task — a comparison or `equals(...)`). +/// @param when The condition node (`engaged(...)`, `notEngaged(...)`, a +/// comparison, or `equals(...)`). /// @return The rule node. template requires EngageableField @@ -1655,7 +1669,7 @@ concept HasExplicitSubmit = requires { namespace detail { /// @brief Evaluates @p rule against @p action, skipping presentation rules -/// (`VisibleWhen` / `ReadonlyWhen`, added in a later task) by construction — +/// (`VisibleWhen` / `ReadonlyWhen`) by construction — /// they never gate. template [[nodiscard]] constexpr bool evaluateGatingRule(const Rule& rule, const A& action) noexcept { @@ -2732,8 +2746,9 @@ constexpr void recomputeAll(A& action) { /// probe instance of its *own* containing type, a `fieldMetadata` array built /// from `describe<>()` cannot be a single in-class `static constexpr` /// initializer (the type is still incomplete at that point, and glaze's -/// reflection for it is not `constexpr` either — see this feature's plan for -/// the two compile errors this produces). Declare the member in the class +/// reflection for it is not `constexpr` either — see +/// docs/spec/forms/forms.md, "deriving the field name from the member", for the +/// two compile errors this produces). Declare the member in the class /// and define it just after the closing brace instead: /// @code{.cpp} /// struct RecordMeasurement { @@ -2980,7 +2995,8 @@ template /// /// glaze's `write_json_schema()` output, post-processed with: /// - a top-level `required` array (see file docs for the rule), -/// - `x-decimalPlaces` on every `Quantity` property (the unit's default), +/// - `x-decimalPlaces` on every `Quantity` property (the field's declared +/// precision — see `reconcileDeclaredPrecision`), /// - `x-order` (declaration index) on every property. /// /// The result is fixed per type, so it is computed once and cached. On any diff --git a/include/morph/journal/action_log.hpp b/include/morph/journal/action_log.hpp index ffc032d1..442df8c9 100644 --- a/include/morph/journal/action_log.hpp +++ b/include/morph/journal/action_log.hpp @@ -34,8 +34,10 @@ enum class Outcome : std::uint8_t { Succeeded, Failed }; /// @brief One recorded execution of an action against a model instance. /// -/// Produced automatically by `morph::model::detail::IModelHolder::recordIfAttached` -/// — application and model code never construct or append these directly. +/// Normally produced automatically by +/// `morph::model::detail::IModelHolder::recordIfAttached`. Application code may +/// also construct and append one directly — that is what an outbox row is, and +/// what `causalParentId` is set by; see `morph::journal::OutboxRelay`. struct LogEntry { /// @brief Monotonic order assigned by the sink on `append()`. Callers pass `0`. uint64_t seq = 0; diff --git a/include/morph/net/detail/ws_handshake.hpp b/include/morph/net/detail/ws_handshake.hpp index 15055f2c..07cb3054 100644 --- a/include/morph/net/detail/ws_handshake.hpp +++ b/include/morph/net/detail/ws_handshake.hpp @@ -174,7 +174,7 @@ struct ClientHandshakeRequest { /// @brief Parses a client's HTTP/1.1 Upgrade request (header block only, no /// trailing blank line). /// @param headerBlock The request's header lines, joined by `\r\n`, with no -/// trailing `\r\n\r\n` (see `readHttpHeaderBlock`, Task 6). +/// trailing `\r\n\r\n` (see `readHttpHeaderBlock`). /// @return The extracted key and path. /// @throws std::runtime_error if the request line is not a `GET`, or the /// `Sec-WebSocket-Key`/`Upgrade` headers are missing. @@ -222,7 +222,7 @@ inline ClientHandshakeRequest parseClientHandshakeRequest(std::string_view heade /// @brief Verifies the server's HTTP/1.1 101 response against the key the /// client sent. /// @param headerBlock The response's header lines, joined by `\r\n`, with no -/// trailing `\r\n\r\n` (see `readHttpHeaderBlock`, Task 6). +/// trailing `\r\n\r\n` (see `readHttpHeaderBlock`). /// @param clientKey The `Sec-WebSocket-Key` this client sent in its request. /// @throws std::runtime_error if the status line is not `101`, or /// `Sec-WebSocket-Accept` does not match `computeAcceptKey(clientKey)`. diff --git a/include/morph/offline/file_offline_queue.hpp b/include/morph/offline/file_offline_queue.hpp index 47f85c19..020edf27 100644 --- a/include/morph/offline/file_offline_queue.hpp +++ b/include/morph/offline/file_offline_queue.hpp @@ -29,8 +29,10 @@ namespace morph::offline { -/// @brief Thrown by `FileOfflineQueue` when its on-disk NDJSON cannot be -/// opened, or a non-trailing line is malformed. +/// @brief Thrown by `FileOfflineQueue` when a non-trailing line of its on-disk +/// NDJSON is malformed (via `detail::throwOnGlazeError`). Note the +/// "cannot be opened" paths throw plain `std::runtime_error`, not this +/// type — see the constructor's own exception documentation. struct FileOfflineQueueError : std::runtime_error { using std::runtime_error::runtime_error; }; @@ -70,8 +72,11 @@ inline void throwOnGlazeError(const glz::error_ctx& errCode, std::string_view co /// escaped `\` or `"`, glaze's chunked writer path silently rewrites the /// control byte as two 0x00 bytes, corrupting the payload before it ever /// reaches disk. Mirrors `morph::wire::detail::EscapingWriteOpts` (`core/wire.hpp`) -/// exactly; duplicated here (rather than shared) so this header stays free of -/// a `core/` dependency. Escaping is lossless, so any such byte still +/// exactly; duplicated here (rather than shared) so this header does not pull in +/// `core/wire.hpp`'s envelope machinery for a four-line options struct (it +/// already depends on `core/file_io_ops.hpp`, `core/logger.hpp` and +/// `core/observability.hpp`, so it is this one header that is being avoided, not +/// `core/` as such). Escaping is lossless, so any such byte still /// round-trips through `fromJson` unchanged. struct EscapingWriteOpts : glz::opts { /// @brief Emit control bytes as `\\uXXXX` rather than raw. diff --git a/include/morph/offline/offline_queue.hpp b/include/morph/offline/offline_queue.hpp index 1aff47d1..bf39fe6e 100644 --- a/include/morph/offline/offline_queue.hpp +++ b/include/morph/offline/offline_queue.hpp @@ -22,8 +22,8 @@ struct QueueItem { /// @brief Stable identifier assigned at enqueue time. /// /// Local to *this* queue instance; it is **not** a cross-subsystem - /// idempotency key (a durable queue re-presents the same logical op with a - /// fresh `id` after a restart, and the journal never sees it). Use + /// idempotency key (it is queue-local and the journal never sees it; both + /// shipped durable queues re-present the *stored* id after a restart). Use /// `idempotencyKey` to dedup a replay against already-applied ops. uint64_t id{}; diff --git a/include/morph/offline/reconnect_coordinator.hpp b/include/morph/offline/reconnect_coordinator.hpp index 090619e1..bab05e78 100644 --- a/include/morph/offline/reconnect_coordinator.hpp +++ b/include/morph/offline/reconnect_coordinator.hpp @@ -17,7 +17,10 @@ namespace morph::offline { /// @brief Outcome of a single `onOnline()` attempt sequence. enum class ReconnectOutcome : std::uint8_t { - Reconnected, ///< Backend reopened, made active, context bound, queue replay invoked. + Reconnected, ///< Backend reopened, made active, context bound. Replay is + ///< invoked only if `shouldContinue()` still holds at that + ///< point — see `onOnline()`; `Reconnected` can be returned + ///< without replaying. GaveUp, ///< Exhausted maxAttempts without a successful reconnect; stayed offline. Aborted, ///< shouldContinue() returned false before any reconnect (e.g. went offline again). }; diff --git a/include/morph/offline/sqlite_offline_queue.hpp b/include/morph/offline/sqlite_offline_queue.hpp index 931e24c6..874be31d 100644 --- a/include/morph/offline/sqlite_offline_queue.hpp +++ b/include/morph/offline/sqlite_offline_queue.hpp @@ -380,10 +380,11 @@ class SqliteOfflineQueue : public IOfflineQueue { } void stepOrThrow(sqlite3_stmt* stmt, const char* what) const { - // A busy/error code is treated the same as reaching the end -- a - // production consumer wanting to distinguish SQLITE_BUSY should retry - // instead, but a single in-process mutex around the whole connection - // makes SQLITE_BUSY practically unreachable for this reference queue. + // Anything but SQLITE_DONE throws, so SQLITE_BUSY is not distinguished + // from a genuine error -- a production consumer wanting to retry on busy + // would need to split them. A single in-process mutex around the whole + // connection makes SQLITE_BUSY practically unreachable for this + // reference queue, which is why the distinction is not made here. if (sqlite3_step(stmt) != SQLITE_DONE) { throw SqliteOfflineQueueError{std::string{"SqliteOfflineQueue: "} + what + " failed: " + sqlite3_errmsg(_db)}; diff --git a/include/morph/offline/sync_worker.hpp b/include/morph/offline/sync_worker.hpp index c0fbf164..f6d5899d 100644 --- a/include/morph/offline/sync_worker.hpp +++ b/include/morph/offline/sync_worker.hpp @@ -276,8 +276,10 @@ class SyncWorker { /// @brief Signals an in-progress `run()` to stop after the current item. /// - /// Thread-safe. The flag is automatically reset at the start of the next - /// `run()` call, so stopping is one-shot. + /// Thread-safe. `run()` clears the flag at its start, so stopping is + /// one-shot — but note a `stop()` that lands *during* a run leaves the flag + /// set on return, so the next `run()` takes its early-out and drains + /// nothing; work resumes only on the run after that. void stop() { _stopped.store(true); } private: diff --git a/include/morph/qt/qt_websocket_backend.hpp b/include/morph/qt/qt_websocket_backend.hpp index 803e1ec5..a940a5fd 100644 --- a/include/morph/qt/qt_websocket_backend.hpp +++ b/include/morph/qt/qt_websocket_backend.hpp @@ -227,7 +227,10 @@ class QtWebSocketBackend : public ::morph::backend::detail::IBackend { /// @param contextKey Stable identity of the new instance; travels in the wire envelope. /// @param onRegistered Invoked with the server-assigned `ModelId` on success. /// @param onError Invoked with a diagnostic message on failure or disconnect. - /// @return `true` always — this backend has an async path (`false` is never returned). + /// @return `true` when `Config::asyncRegistrationEnabled` is set (the + /// backend then owns the reply); `false` when it is not — which is + /// the default, so the caller falls back to the synchronous path + /// unless the embedder opted in. bool registerModelAsync(const std::string& typeId, std::function()> factory, std::string_view contextKey, diff --git a/include/morph/render/i18n.hpp b/include/morph/render/i18n.hpp index 9d8346ad..83b4ed37 100644 --- a/include/morph/render/i18n.hpp +++ b/include/morph/render/i18n.hpp @@ -7,8 +7,11 @@ /// message keys (`morph::forms::i18n`, `forms/i18n.hpp`). /// /// `morph::render` is client-side only and never appears on the wire — it is -/// the namespace the planned per-field widget-override registry -/// (`gui_renderer_toolkit.md`'s `SlotRegistry`) will eventually share. morph +/// the C++ side of the client-side rendering seam. The per-field +/// widget-override registry that pairs with it is *not* C++ at all: it is +/// `SlotRegistry`, a QML type in module `MorphForms` +/// (`src/qt/forms/qml/SlotRegistry.qml`, documented in +/// docs/spec/forms/forms.md), so do not look for it under this namespace. morph /// ships this seam and the resolution algorithm below; it defines **no** /// translation storage format. A host adapts whatever catalog it already /// owns (Qt `QTranslator`/`.qm`, a JSON bundle, a database) into the one diff --git a/include/morph/util/rational.hpp b/include/morph/util/rational.hpp index c25968e5..f0a3acc8 100644 --- a/include/morph/util/rational.hpp +++ b/include/morph/util/rational.hpp @@ -726,7 +726,8 @@ struct Rational { /// @brief Logs an `INT64_MIN` component clamped by `canonicalise`. /// /// Skipped during constant evaluation, and non-throwing, like - /// `reportOverflow` -- see that function for why the `catch` is there. + /// `reportOverflow` -- see that function for why no local try/catch is + /// needed (morph#158 moved that guarantee into the logging layer). static constexpr void reportClamp() noexcept { if (!std::is_constant_evaluated()) { ::morph::log::logError( @@ -794,8 +795,9 @@ struct Rational { /// forming that product, so a helper that returns the already-multiplied /// values would have to compute the very product being checked for. /// @param rhs The other operand. - /// @return `leftScaled` multiplies `this->denominator`/`numerator`; - /// `rightScaled` multiplies `rhs.numerator`. + /// @return `rightScaled` multiplies `this->numerator` and + /// `this->denominator`; `leftScaled` multiplies `rhs.numerator` + /// (see `addAssignUnchecked`, which applies them). [[nodiscard]] constexpr DenominatorScale scaleFactorsFor(const Rational& rhs) const noexcept { auto const denominatorGcd = std::gcd(denominator, rhs.denominator); return DenominatorScale{ @@ -1355,8 +1357,10 @@ template // the half-ulp llround adds: values in [2^63 - 0.5, 2^63) round *up* to // 2^63 and would overflow int64. Casting INT64_MAX instead would itself // round up to 2^63 where long double == double. The negative bound is - // asymmetric because INT64_MIN == -2^63 is a valid result and llround - // maps (-2^63 - 0.5, -2^63] onto it, hence `<` against -2^63 exactly. + // asymmetric because llround maps (-2^63 - 0.5, -2^63] onto INT64_MIN, + // hence `<` against -2^63 exactly. Note the accepted INT64_MIN does not + // survive as such: the constructor below canonicalises, and canonicalise() + // clamps it to -INT64_MAX with an error log (see reportClamp). constexpr auto twoPow63 = 0x1p63L; if (scaled >= twoPow63 - 0.5L || scaled < -twoPow63) { return std::unexpected(RationalError::Overflow); diff --git a/scripts/branch_partial_allowlist.json b/scripts/branch_partial_allowlist.json index 3a4dee42..2a6e3739 100644 --- a/scripts/branch_partial_allowlist.json +++ b/scripts/branch_partial_allowlist.json @@ -32,7 +32,7 @@ }, { "file": "include/morph/util/rational.hpp", - "line": 731, + "line": 732, "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." }, @@ -44,37 +44,37 @@ }, { "file": "include/morph/util/rational.hpp", - "line": 757, + "line": 758, "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 775 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 776 below." }, { "file": "include/morph/util/rational.hpp", - "line": 775, + "line": 776, "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 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." + "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." }, { "file": "include/morph/util/rational.hpp", - "line": 899, + "line": 901, "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": 1495, + "line": 1499, "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": 1404, + "line": 1408, "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." }, { "file": "include/morph/core/strand.hpp", - "line": 190, + "line": 192, "source": "if (iter != _strands.end() && iter->second == strand) {", "reason": "Unreachable by construction given this class's lock discipline (core audit finding ST1, resolved to (b) by a concurrency-focused review pass after an initial (a)/(b)-undecided pass). `_strands` has exactly two mutation sites: `post()`'s insert-if-absent (this file, `if (!slot) { slot = make_shared(); }`) and this exact block's own erase a few lines below, both under `_mapMtx`. At most one lambda per `Strand` runs at a time (`post()` only schedules when `!strand->running`, and re-arming happens only through this same lambda's own `more` branch), so dispatch for one `Strand` is strictly serial; and only a strand's own currently-running lambda can erase its map entry (the erase fires only in the `!more` branch for the entry this frame just found under `_mapMtx`, and a concurrent `post(key)` while this lambda runs can only push onto the existing `Strand`, never replace it). Together these force `_strands.find(key)` to yield this exact strand whenever this line runs, so `iter->second == strand` cannot be false. No stress test needed: one was considered, but given the strength of the lock-discipline argument it would spend CI time re-confirming an already-proven invariant rather than searching for an unknown one." }, @@ -104,31 +104,31 @@ }, { "file": "include/morph/core/remote.hpp", - "line": 1415, + "line": 1426, "source": "if (_inFlightExecutes.compare_exchange_weak(current, current + 1, std::memory_order_relaxed)) {", "reason": "Real but requires genuine thread contention to trigger -- accepted as documented rather than closed with a flaky test (core audit finding RM11). The false arm (the CAS lost the race and must retry) needs two threads to genuinely collide on the same atomic increment at the same instant; it is a real, reachable hazard the retry loop correctly handles, not dead code, but inherently non-deterministic to trigger from a test without exact thread-timing control. Same disposition class as `strand.hpp`'s ST1 above, and as core audit finding O1 (`observability.hpp`'s `endSpan`), whose entry left this file once a coverage run showed its arm taken: a stress test with many concurrent `execute()` calls against a tight `maxInFlightExecutes` limit would probably eventually hit it, but flakily." }, { "file": "include/morph/core/bridge.hpp", - "line": 1445, + "line": 1453, "source": "if (_executeDeadline.count() > 0 && _timeoutScheduler) {", "reason": "Unreachable by construction (core audit finding B6). `setExecuteDeadline` (this file) is the only writer of both `_executeDeadline` and `_timeoutScheduler`, and always creates `_timeoutScheduler` in the same call that sets `_executeDeadline` positive (`_executeDeadline = deadline; if (_executeDeadline.count() > 0 && !_timeoutScheduler) { _timeoutScheduler = std::make_shared<...>(); }`, both under `_executeDeadlineMtx`); nothing anywhere resets `_timeoutScheduler` back to null -- the class's own doc comment on `setExecuteDeadline` says so explicitly (\"setting the deadline back to 0 stops new calls from arming it but does not tear the thread down\"). So `_executeDeadline > 0 && !_timeoutScheduler` cannot happen at this line once any positive deadline has ever been set." }, { "file": "include/morph/core/bridge.hpp", - "line": 1563, + "line": 1571, "source": "if (deadlineHandle && schedulerRef) {", - "reason": "Unreachable by construction, same joint-assignment shape as B6 above (core audit finding B11, reclassified (a)->(b) on review). `deadlineHandle` and `schedulerRef` are assigned together, a few lines above this one in `executeVia`, only inside `if (_executeDeadline.count() > 0 && _timeoutScheduler) { schedulerRef = _timeoutScheduler; ... }` (see the bridge.hpp:1421 entry above) -- there is no path that sets `deadlineHandle` without also having set `schedulerRef` from the same non-null `_timeoutScheduler` in the same conditional. So `schedulerRef` null while `deadlineHandle` is non-null cannot occur; the only theoretically-open arm this compound condition has is structurally impossible." + "reason": "Unreachable by construction, same joint-assignment shape as B6 above (core audit finding B11, reclassified (a)->(b) on review). `deadlineHandle` and `schedulerRef` are assigned together, a few lines above this one in `executeVia`, only inside `if (_executeDeadline.count() > 0 && _timeoutScheduler) { schedulerRef = _timeoutScheduler; ... }` (see the bridge.hpp:1453 entry above) -- there is no path that sets `deadlineHandle` without also having set `schedulerRef` from the same non-null `_timeoutScheduler` in the same conditional. So `schedulerRef` null while `deadlineHandle` is non-null cannot occur; the only theoretically-open arm this compound condition has is structurally impossible." }, { "file": "include/morph/core/remote.hpp", - "line": 1456, + "line": 1467, "source": "if (_timeoutScheduler) {", "reason": "Unreachable by construction, mirrors `bridge.hpp`'s B6 (core audit finding RM10). `setLimitPolicy` (this file) is the only writer of both `_limits.executeTimeout` and `_timeoutScheduler`: `_limits = policy; if (_limits.executeTimeout.count() > 0 && !_timeoutScheduler) { _timeoutScheduler = std::make_unique<...>(); }`, both under `_limitsMtx` -- the same lock this line's enclosing block holds. Nothing anywhere nulls `_timeoutScheduler` afterward, so `dispatchExecute`'s `limits.executeTimeout.count() > 0` guard (this line's enclosing `if`, a few lines above) already guarantees `_timeoutScheduler` is non-null whenever this line runs." }, { "file": "include/morph/core/remote.hpp", - "line": 1341, + "line": 1352, "source": "if (auto ownerIter = _owners.find(mid); ownerIter != _owners.end()) {", "reason": "Unreachable by construction given the `_owners`/`_models` invariant (core audit finding RM16). `grep -n \"_owners\\[\" remote.hpp` shows `_owners` is created in lockstep with `_models` at both of its insertion sites (the shared path, `_owners[fresh] = std::string{}`, and the private-registration path, `_owners[mid] = std::move(env.session.principal)`) and erased together with `_models` at the single erasure site (`releaseInstanceLocked`: `_models.erase(mid); _owners.erase(mid);`). This line is reached only after `iter != _models.end()` (`mid` found in `_models`, a few lines above, same locked block) -- unlike RM1's `_attachCount`/`_sharedKeyOf` pair, which has a documented exception at a poisoned-eviction site, no such exception exists for `_owners`/`_models`: every write site keeps them lockstep, so `mid` being present in `_models` guarantees it is present in `_owners` too." },