From e28d25f2e70335f42e296a31810d39e1a1e616e6 Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Tue, 22 Sep 2026 21:57:15 +0200 Subject: [PATCH 1/3] docs: one page of Lightweight constraints, and two of the three turn out to be stale (fixes #695) Three plan documents each recorded the same Lightweight limits, because each rung rediscovered them and wrote them into its own plan, where the next rung could not find them. This collects them into one page under docs/, where a rung author looks, and gives every entry what a plan entry does not have: the pinned revision it was verified at, the file and line in Lightweight that decides it, the workaround actually used in this tree, and the condition that retires it. Checking them at the pin (bbb972a7, g++ 16.2.1, -fsyntax-only, non-reflection) is what the page is for, and it immediately earned itself. Of the three constraints the issue names, only one survives: - Where() cannot bind a binary value: CONFIRMED, and now as a compile diagnostic rather than by reading the variant. SqlVariant::InnerType has zero binary alternatives; the identical call with a std::string compiles, so the refusal is about the value type. Filed upstream as LASTRADA-Software/Lightweight#618 so the entry has a retirement condition somebody tracks. - "Fluent Query/Update refuse HasMany-bearing records": FALSE in both halves. DataMapper::Update on a HasMany-bearing record compiles; so does Query().Where(...).All(). What is true is narrower and is not about HasMany at all -- Query() has no Update() for ANY record, which the control probe on a HasMany-free record shows by failing identically. - "HasMany resolves a child FK by ordinal member index": RETIRED upstream. Record.hpp:242-255 now says in its own words that resolution is by relationship type, never by member position, and InverseBelongsToResolver carries three static_asserts turning every failure mode into a named compile error. Verified with the child's BelongsTo declared last, which positional resolution would get wrong. A fourth entry, found while checking the others and already worked around in-tree: the migration DSL has no predicate on any CreateIndex overload, so a partial unique index has no spelling. The page says outright that an entry is only as good as its last verification, because entries 2 and 3 are what that rule was written from. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01VptDWG2fKr2vBnLSJcgzgW --- docs/LIGHTWEIGHT-CONSTRAINTS.md | 212 ++++++++++++++++++++++++++++++++ examples/IMPLEMENTATION.md | 8 ++ 2 files changed, 220 insertions(+) create mode 100644 docs/LIGHTWEIGHT-CONSTRAINTS.md diff --git a/docs/LIGHTWEIGHT-CONSTRAINTS.md b/docs/LIGHTWEIGHT-CONSTRAINTS.md new file mode 100644 index 00000000..540596d3 --- /dev/null +++ b/docs/LIGHTWEIGHT-CONSTRAINTS.md @@ -0,0 +1,212 @@ +# Lightweight constraints an example author hits + +The ladder's persistence layer is the [LASTRADA Lightweight](https://github.com/LASTRADA-Software/Lightweight) +ORM, pinned to one commit SHA by `examples/common/CMakeLists.txt` and, in +lockstep, `examples/bank/CMakeLists.txt`. Some of what that revision can and +cannot do is not obvious from its headers, and every rung that +rediscovered a limit wrote it down in **its own plan document** — so the next +rung rediscovered it again. This page is the single place those belong. + +**Scope.** This is not morph's design (that lives in `docs/spec/`) and not a +plan (those live in `docs/superpowers/`). It is a record of *someone else's* +library's behaviour, kept here because example authors are the people who trip +over it. + +**What every entry carries**, and what the plan documents did not: + +- the constraint, stated as something you can act on; +- **the pinned revision it was verified at**, and how it was verified; +- the file and line in Lightweight that decides it; +- the workaround actually used in this tree; +- **what would retire it** — the condition under which the entry should be + deleted. + +**An entry is only as good as its last verification.** When the pin moves, +re-run the probes; two of the three entries below were folklore that the pin +had already retired, and nobody noticed because nothing re-checked them. + +Pinned revision at the time of writing: +`bbb972a78e1962b968a2c6ad93f7dade736eaa01`. + +--- + +## 1. `Where()` cannot bind a binary value — **live** + +**Constraint.** The fluent query builder's literal `Where()` overload cannot +take a `SqlBinary` or `SqlDynamicBinary`. A comparison against a binary +column has to be expressed some other way. + +**Verified at** `bbb972a78e1962b968a2c6ad93f7dade736eaa01`, by compiling it +(`g++ 16.2.1`, `-std=c++23 -fsyntax-only`, non-reflection build): + +```cpp +Lightweight::SqlDynamicBinary<32> blob; +mapper.Query().Where(Lightweight::FieldNameOf<&Child::label>, "=", blob).All(); +``` + +``` +/usr/include/c++/16/bits/alloc_traits.h:716:28: error: no matching function for call to + 'construct_at(Lightweight::SqlVariant*&, const Lightweight::SqlDynamicBinary<32>&)' +``` + +The identical call with `std::string{"x"}` in place of `blob` compiles, so the +refusal is about the value type and not about the call shape. + +**Why.** `Where()`'s literal path stores the bound value in a +`std::vector` (`src/Lightweight/SqlQuery/Core.hpp:137`), and +`SqlVariant::InnerType` (`src/Lightweight/DataBinder/SqlVariant.hpp:49`) has +**zero** binary alternatives — the variant runs `SqlNullType`, `SqlGuid`, +`bool`, the integer and floating types, the string types, `SqlText`, `SqlDate`, +`SqlTime`, `SqlDateTime`, and stops. `SqlDataBinder>` exists +and works everywhere else; `Where()` simply does not go through it. + +**Workaround used in this tree.** Store the value in a text column as +lower-case hex and compare against the hex string. +`examples/bank`'s offline queue does exactly that for +`OfflineQueueRecord::idempotencyKeyHex`, because enqueue-time dedup needs a +`WHERE` on that column. Note what it does *not* do: the adjacent `payload` +column stays a real `SqlDynamicBinary`, because nothing ever puts it in a +`Where()`. The constraint is on the *comparison*, not on binary storage, so hex +only where a `WHERE` reaches. Hex is NUL-free by construction, so the text +column carries an arbitrary byte string without truncating it or colliding two +keys that share a NUL prefix, and the conformance round-trip +(`tests/offline_queue_conformance.hpp`'s `checkNulPayloadRoundTrip`) passes +unedited. + +**What retires this.** A Lightweight release that either adds a binary +alternative to `SqlVariant::InnerType` or routes `Where()`'s literal path +through `SqlDataBinder<>`. Tracked upstream as +[LASTRADA-Software/Lightweight#618](https://github.com/LASTRADA-Software/Lightweight/issues/618). +When it lands, the hex column can become a plain `SqlDynamicBinary` again. + +--- + +## 2. `Query()` has no `Update()` — **live, but not the constraint it was recorded as** + +**Constraint.** The record-typed fluent builder +(`mapper.Query().Where(...)`) offers `All()`, `First()`, `Count()`, +`Delete()` and friends, but **no `Update()`**. To update, use +`DataMapper::Update(record)` on a fetched record, or drop to the untyped +builder on the connection. + +**Verified at** the pin, by compiling it: + +```cpp +mapper.Query().Where(Lightweight::FieldNameOf<&Parent::id>, "=", 1).Update(); +``` + +``` +error: 'class Lightweight::SqlAllFieldsQueryBuilder' has no member named 'Update' +``` + +**This corrects what the plan archive says.** Three plan documents record this +as *"fluent `Query`/`Update` refuse `HasMany`-bearing records in the +non-reflection build"* (`docs/superpowers/plans/2026-08-16-kanban-backend.md`, +`docs/superpowers/specs/2026-08-16-kanban-rung4-design.md`, +`docs/superpowers/plans/2026-08-19-ledger-rung5.md`). At the pinned revision +that is **false in both halves**, and the control probes say so: + +| probe | at the pin | +| --- | --- | +| `DataMapper::Update(parent)` where `Parent` has a `HasMany` member | **compiles** | +| `mapper.Query().Where(...).All()` where `Parent` has a `HasMany` member | **compiles** | +| `mapper.Query().Where(...).Update()` (`Parent` has `HasMany`) | fails | +| `mapper.Query().Where(...).Update()` (`Child` has **no** `HasMany`) | **fails identically** | + +The last row is the one that matters: the refusal has nothing to do with +`HasMany`. `DataMapper::Update` skips relations explicitly — *"Relations +(HasMany, HasManyThrough, HasOneThrough, ...) have no column of their own"*, +`src/Lightweight/DataMapper/DataMapper.hpp:2047` and three sibling sites — so a +`HasMany` member costs an update nothing. + +**Workaround.** `DataMapper::Update(record)`, which is what every ladder rung +already does. + +**What retires this.** A Lightweight release that adds `Update()` to +`SqlAllFieldsQueryBuilder`. Re-run the probe when the pin moves. + +--- + +## 3. `HasMany` resolves its foreign key by ordinal member index — **retired; do not believe this** + +**This constraint is no longer true**, and it is recorded here only because +three plan documents still assert it and a fourth rung would otherwise design +around a limit that no longer exists. + +**What the plans say.** That `HasMany` locates the child's foreign key by +matching **member position**, so reordering a record's members silently +repoints the relation. + +**What the pinned revision does.** Matches by relationship **type**. +`src/Lightweight/DataMapper/Record.hpp:242-255` says so in its own words — +*"This is how `HasMany`, `HasManyThrough` and `HasOneThrough` locate their +foreign key column: by matching the relationship type, never by member +position"* — and `detail::InverseBelongsToResolver` (`Record.hpp:213-238`) +carries three `static_assert`s that turn every failure mode into a named +compile error: no `BelongsTo` at all, a `BelongsTo` whose column name does not +match, and an ambiguous pair of them. + +**Verified at** the pin, with the child's `BelongsTo` declared **last**, which +positional resolution would get wrong: + +```cpp +static_assert(Lightweight::InverseBelongsToIndexOf == 2); +static_assert(Lightweight::InverseBelongsToFieldNameOf == std::string_view{"parent"}); +``` + +Both hold. + +**What would re-open this.** A `HasMany` relation resolving to the wrong column +at a pin where the `static_assert`s above are absent. If that happens, this +entry becomes live again and the pin's `Record.hpp` is the first thing to read. + +--- + +## 4. The migration DSL cannot express a partial index — **live** + +**Constraint.** `SqlMigrationQueryBuilder`'s index API takes a name, a table +and a column list, and nothing else. There is no predicate, so +`CREATE UNIQUE INDEX … WHERE ` — a partial index — has no spelling in the +DSL. + +**Verified at** the pin, by reading the API surface. Every overload, in +`src/Lightweight/SqlQuery/Migrate.hpp:553-582`: + +```cpp +CreateIndex(std::string indexName, std::string tableName, std::vector columns, bool unique = false); +CreateUniqueIndex(std::string indexName, std::string tableName, std::vector columns); +CreateIndex(std::string tableName, std::vector columns, IndexType type = IndexType::NonUnique); +``` + +Also `SqlCreateTableQueryBuilder::Unique()` / `::UniqueIndex()` +(`Migrate.hpp:74`, `:80`) and `SqlAlterTableQueryBuilder::AddUniqueIndex(column)` +(`:207`) — none takes a predicate. `MigrationPlan.cpp` renders a trailing +`WHERE` only for UPDATE and DELETE plans (`:56-71`), never for an index. + +**Why it bites.** Dedup on an optional key wants exactly a partial index: +`UNIQUE(key) WHERE key <> ''`. An *unconditional* unique index is not a +substitute — it makes the second empty-key row fail, which +`IOfflineQueue::enqueue` forbids. + +**Workaround used in this tree.** `examples/bank`'s offline queue does the +check as a `SELECT` under its own mutex instead, and says so in +`LightweightOfflineQueue`'s class comment: as strong as `FileOfflineQueue`'s +linear scan (single process), deliberately weaker than +`SqliteOfflineQueue`'s real index (any writer). The other option, not taken +there, is `SqlMigrationQueryBuilder::Native(callback)` (`Migrate.hpp:594`), +which hands the plan a raw SQL string — an escape hatch that gives up the DSL's +cross-dialect rendering for that one statement. + +**What retires this.** A `CreateIndex` overload taking a predicate expression, +or an equivalent in the plan renderer. + +--- + +## Adding an entry + +File the upstream issue first, so the entry has a retirement condition somebody +is tracking, then add a section here with the five bullets from the top of this +page. An entry with no verification revision is worth less than no entry: it +cannot be re-checked, and it will outlive the constraint it describes — which +is exactly how entries 2 and 3 above became folklore. diff --git a/examples/IMPLEMENTATION.md b/examples/IMPLEMENTATION.md index bff4964a..67428bef 100644 --- a/examples/IMPLEMENTATION.md +++ b/examples/IMPLEMENTATION.md @@ -10,6 +10,14 @@ could have provided is a defect in the stress test.** If the framework can't provide it, that inability is a *finding* — record it per [`FINDINGS.md`](FINDINGS.md), don't quietly code around it. +**Before you design around a Lightweight limitation, read +[`docs/LIGHTWEIGHT-CONSTRAINTS.md`](../docs/LIGHTWEIGHT-CONSTRAINTS.md)** — one +page, one entry per constraint, each with the pinned revision it was verified +at and what would retire it. It exists because three rungs in a row +rediscovered the same limits and recorded them in their own plan documents, +where the next rung could not find them and where two of them quietly went +stale. If you hit a new one, add it there rather than to your plan. + **The promotion rule (rule-of-three, from the round-7 review):** an app-built answer to a framework gap (the polling helper with its timeout, an op-id ledger, epoch tokens, a recursive validator, redaction-on-serve) From fa67ebfbb8d65f0d4e63d593cc8f27c2f635522a Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Tue, 22 Sep 2026 21:57:32 +0200 Subject: [PATCH 2/3] offline: give the attempt count its own type, so the transposed setAttempts does not compile (fixes #696) setAttempts(uint64_t itemId, uint32_t attempts) was two adjacent, mutually convertible integers. bugprone-easily-swappable-parameters said so at every implementation, which meant every implementor hand-wrote a suppression for a hazard they did not choose -- the framework exporting its lint bill to its consumers -- and the suppression left the hazard exactly where it was. The hazard is silent in both directions, which is what makes it worth a type rather than a NOLINT: setAttempts on an unknown id is a documented no-op, so a transposed call writes an id into a count, matches nothing, returns normally and throws nothing. The real item's count never advances, and the defect surfaces much later as a retry budget that never exhausts. morph::offline::Attempts is implicitly constructible from a narrow integer and deliberately NOT from a 64-bit one, which is what a QueueItem::id is. That is the whole mechanism: setAttempts(attempts, itemId) stops compiling, while every existing call site -- setAttempts(id, 3), setAttempts(id, counter) -- reads exactly as before. No test call site in the tree changed. A genuinely 64-bit count stays expressible with the narrowing visible at the call site. Proven rather than asserted, both directions: $ g++ -fsyntax-only ... -DTRANSPOSED=1 error: cannot convert 'const uint64_t' to 'morph::offline::Attempts' 8 | queue.setAttempts(attempts, itemId); $ g++ -fsyntax-only ... -DTRANSPOSED=0 exit=0 and the two static_asserts that hold the property in place were mutation tested -- weakening the constructor's constraint to sizeof(Count) <= sizeof(uint64_t) makes both of them fire, so neither is vacuous. They are written as a pair on purpose: an alias for uint32_t fails the first, an explicit constructor fails the second. QueueItem::attempts stays a plain uint32_t. It is a struct field reached by name with no adjacent same-typed field to transpose it with; the hazard was in the parameter list and that is where the type went. Deletes the hand-written NOLINT in examples/bank, which is the second data point the issue cites and the reason the tax was recurring rather than hypothetical. tests/test_sync_worker.cpp:406 is a recording test double whose override signature has to follow the interface; that one override is the only line touched in that file. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01VptDWG2fKr2vBnLSJcgzgW --- docs/spec/offline/offline.md | 41 +++++++++- .../offline/lightweight_offline_queue.hpp | 2 +- .../src/offline/lightweight_offline_queue.cpp | 5 +- include/morph/offline/file_offline_queue.hpp | 6 +- include/morph/offline/offline_queue.hpp | 80 ++++++++++++++++++- .../morph/offline/sqlite_offline_queue.hpp | 4 +- tests/test_sync_worker.cpp | 4 +- 7 files changed, 126 insertions(+), 16 deletions(-) diff --git a/docs/spec/offline/offline.md b/docs/spec/offline/offline.md index c76ddb96..9867310c 100644 --- a/docs/spec/offline/offline.md +++ b/docs/spec/offline/offline.md @@ -258,7 +258,7 @@ while offline; `SyncWorker` drains and replays them on reconnect. | `enqueue` | `uint64_t enqueue(std::string payload, std::string idempotencyKey)` | Appends payload carrying the dedup key (stored on `QueueItem::idempotencyKey`). Virtual with a default that delegates to the one-arg `enqueue` then stamps the key via the protected `setIdempotencyKey`, so existing implementations keep working; the key is dropped by an implementation with no per-item storage that overrides neither. | | `drain` | `std::vector drain()` | Returns all pending items in enqueue order, without removing them. Safe to call multiple times — items survive between `drain()` and the corresponding `markDone()`. | | `markDone` | `void markDone(uint64_t itemId)` | Removes the item identified by `itemId`. No-op if not found. | -| `setAttempts` | `void setAttempts(uint64_t itemId, uint32_t attempts)` | Persists an updated attempt count for an item. **Public** (unlike `setIdempotencyKey`) because `SyncWorker` calls it from outside the queue after every failed replay. Default no-op; `InMemoryOfflineQueue` overrides it to update the in-deque item. A queue that overrides it to store the count durably makes `SyncWorker`'s retry budget survive a process restart. | +| `setAttempts` | `void setAttempts(uint64_t itemId, Attempts attempts)` | Persists an updated attempt count for an item. **Public** (unlike `setIdempotencyKey`) because `SyncWorker` calls it from outside the queue after every failed replay. Default no-op; `InMemoryOfflineQueue` overrides it to update the in-deque item. A queue that overrides it to store the count durably makes `SyncWorker`'s retry budget survive a process restart. The count is an `Attempts`, not a bare `uint32_t` — see "`Attempts`: why the count has its own type" below. | | `size` | `std::size_t size() const` | Number of pending items, without removing them. Default calls `drain().size()` — correct but O(n) and allocates a full snapshot to answer a size query; every shipped implementation overrides it with a direct count. | | `maxDepth` | `std::optional maxDepth() const` | The capacity `enqueue()` enforces, or `std::nullopt` if unbounded. Default: `std::nullopt` — preserves current behavior for any `IOfflineQueue` subclass written before this method existed. | | `setIdempotencyKey` (protected) | `void setIdempotencyKey(uint64_t itemId, std::string key)` | Hook the default two-arg `enqueue` uses to stamp the key onto an already-enqueued item. Default no-op; `InMemoryOfflineQueue` records the key directly instead. **A conflicting non-empty key is skipped, never raised** — an implementation that deduplicates leaves the item unkeyed rather than failing, because the default `enqueue` has already inserted by the time it stamps, so the row exists either way and an exception could not undo it. That path therefore yields an extra *unkeyed* item, not a dedup hit; a caller wanting dedup uses the virtual two-arg `enqueue` (see below). | @@ -267,6 +267,45 @@ while offline; `SyncWorker` drains and replays them on reconnect. default can call it (and so can an application) without needing a non-`const` reference to the queue. +### `Attempts`: why the count has its own type + +`setAttempts(uint64_t itemId, uint32_t attempts)` was two adjacent, mutually +convertible unsigned integers, and `bugprone-easily-swappable-parameters` said +so at every implementation — which meant every implementor, in this tree and +out of it, hand-wrote a suppression for a hazard they did not choose. That is +the framework exporting its lint bill to its consumers, and the suppression +left the hazard in place. + +The hazard is not theoretical, and it is **silent in both directions**: +`setAttempts()` on an unknown id is a documented no-op +(`tests/test_offline_queue.cpp`, `tests/test_file_offline_queue.cpp`), so a +transposed call writes an attempt count into an id nothing matches, returns +normally, and throws nothing. The real item's count never advances. The defect +surfaces much later, as a `SyncWorker` retry budget that never exhausts and a +poison payload that replays forever. + +`morph::offline::Attempts` removes it rather than suppressing the warning +about it: + +- **Implicit from a narrow integer**, so `setAttempts(id, 3)` and + `setAttempts(id, counter)` read exactly as before and no call site in the + tree changed. +- **Not constructible from a 64-bit integer**, which is what a `QueueItem::id` + is. `setAttempts(attempts, itemId)` therefore does not compile. +- A genuinely 64-bit count is still expressible, with the narrowing visible at + the call site: `setAttempts(id, static_cast(count))`. + +`QueueItem::attempts` stays a plain `uint32_t`: it is a struct field, reached +by name, with no adjacent same-typed field to transpose it with. The hazard was +in the *parameter list*, and that is where the type went. + +Two `static_assert`s below `IOfflineQueue` hold the property in place, and they +are written as a pair on purpose: one asserts the transposed call is **not** +well-formed, the other that the ordinary call still is. Either alone would pass +against a degenerate definition — an alias for `uint32_t` fails the first, an +`explicit` constructor fails the second — so the pair is what makes the check +mean something. + **Enqueue order is the implementation's to keep; the id does not imply it.** `drain()` requires enqueue order, and `QueueItem::id` does not supply it. All three shipped implementations mint ids that increase with insertion — an diff --git a/examples/bank/include/bank/offline/lightweight_offline_queue.hpp b/examples/bank/include/bank/offline/lightweight_offline_queue.hpp index 9d0d8b50..2b64c8d4 100644 --- a/examples/bank/include/bank/offline/lightweight_offline_queue.hpp +++ b/examples/bank/include/bank/offline/lightweight_offline_queue.hpp @@ -170,7 +170,7 @@ class LightweightOfflineQueue final : public morph::offline::IOfflineQueue { /// after the item was marked done does not fail. /// @param itemId Id of the item whose attempt count changed. /// @param attempts New cumulative attempt count to persist. - void setAttempts(std::uint64_t itemId, std::uint32_t attempts) override; + void setAttempts(std::uint64_t itemId, morph::offline::Attempts attempts) override; private: mutable std::mutex _mtx; diff --git a/examples/bank/src/offline/lightweight_offline_queue.cpp b/examples/bank/src/offline/lightweight_offline_queue.cpp index 964767e6..dbce2adb 100644 --- a/examples/bank/src/offline/lightweight_offline_queue.cpp +++ b/examples/bank/src/offline/lightweight_offline_queue.cpp @@ -172,14 +172,13 @@ std::size_t LightweightOfflineQueue::size() const { std::optional LightweightOfflineQueue::maxDepth() const { return _maxDepth; } -// NOLINTNEXTLINE(bugprone-easily-swappable-parameters) -- the signature is IOfflineQueue's, not ours -void LightweightOfflineQueue::setAttempts(std::uint64_t itemId, std::uint32_t attempts) { +void LightweightOfflineQueue::setAttempts(std::uint64_t itemId, morph::offline::Attempts attempts) { const std::scoped_lock lock{_mtx}; auto record = _mapper.QuerySingle(itemId); if (!record.has_value()) { return; } - record->attempts = attempts; + record->attempts = attempts.value(); _mapper.Update(*record); } diff --git a/include/morph/offline/file_offline_queue.hpp b/include/morph/offline/file_offline_queue.hpp index 03140830..c1e0e7dd 100644 --- a/include/morph/offline/file_offline_queue.hpp +++ b/include/morph/offline/file_offline_queue.hpp @@ -283,7 +283,7 @@ class FileOfflineQueue : public IOfflineQueue { /// @brief Persists an updated attempt count for @p itemId. No-op if not found. /// @param itemId Id of the item whose count changed. /// @param attempts New cumulative attempt count to store. - void setAttempts(uint64_t itemId, uint32_t attempts) override { + void setAttempts(uint64_t itemId, Attempts attempts) override { std::scoped_lock const lock{_mtx}; auto iter = _items.find(itemId); if (iter == _items.end()) { @@ -294,9 +294,9 @@ class FileOfflineQueue : public IOfflineQueue { // disk. Write from a copy so `_items` is only updated once the record is // durable. auto updated = iter->second; - updated.attempts = attempts; + updated.attempts = attempts.value(); appendPut(updated); - iter->second.attempts = attempts; + iter->second.attempts = attempts.value(); } protected: diff --git a/include/morph/offline/offline_queue.hpp b/include/morph/offline/offline_queue.hpp index ce589c75..dc6aede6 100644 --- a/include/morph/offline/offline_queue.hpp +++ b/include/morph/offline/offline_queue.hpp @@ -2,18 +2,76 @@ #pragma once #include +#include #include #include #include #include #include #include +#include #include #include "../core/observability.hpp" namespace morph::offline { +/// @brief A replay attempt count, as a type distinct from a queue item id. +/// +/// `setAttempts(itemId, attempts)` takes two integers that mean entirely +/// different things, and before this type existed both were plain unsigned +/// integers, mutually convertible in either direction. Transposing them +/// compiled, and — because `setAttempts()` on an unknown id is a documented +/// no-op — it also *ran*: the real item's count never advanced, nothing threw, +/// and the defect surfaced much later as a retry budget that never exhausts. +/// +/// `Attempts` removes the hazard rather than suppressing the warning about it. +/// It is implicitly constructible from a narrow integer, so `setAttempts(id, 3)` +/// reads exactly as it did before, and **deliberately not constructible from a +/// 64-bit integer**, which is what a `QueueItem::id` is — so the transposed +/// call does not compile. A genuinely 64-bit count is still expressible, with +/// the narrowing spelled out at the call site: +/// `setAttempts(id, static_cast(count))`. +class Attempts { +public: + /// @brief Constructs a zero attempt count. + constexpr Attempts() noexcept = default; + + /// @brief Implicitly wraps a narrow integral attempt count. + /// + /// Constrained to integral types **strictly narrower than a queue item + /// id**, which is the whole point of the class: `uint64_t` (and any other + /// 64-bit integer) is rejected, so passing an item id where an attempt + /// count belongs is a compile error rather than a silent no-op. `bool` is + /// excluded because a boolean is never an attempt count. + /// + /// @tparam Count Integral type of the incoming count. + /// @param count The attempt count to wrap. + template + requires(std::integral && !std::same_as, bool> && + sizeof(Count) < sizeof(std::uint64_t)) + constexpr Attempts(Count count) noexcept : _value{static_cast(count)} {} + + /// @brief Returns the wrapped count. + /// @return The attempt count as a plain `uint32_t`. + [[nodiscard]] constexpr std::uint32_t value() const noexcept { return _value; } + + /// @brief Compares two attempt counts. + /// @param other The count to compare against. + /// @return `true` when both wrap the same value. + [[nodiscard]] constexpr bool operator==(const Attempts& other) const noexcept = default; + +private: + std::uint32_t _value{0}; +}; + +static_assert(!std::is_constructible_v, + "Attempts must not be constructible from a queue item id, or setAttempts()'s two " + "parameters become mutually convertible again and a transposed call compiles."); +static_assert(std::is_convertible_v, + "Attempts must stay implicitly constructible from a narrow count, so existing " + "setAttempts(id, n) call sites keep reading the way they did."); + /// @brief An item stored in the offline queue. /// /// The payload is an opaque string — the caller controls the serialisation @@ -234,8 +292,10 @@ struct IOfflineQueue { /// (`SyncWorker`'s own in-memory counter is then always authoritative, /// since `QueueItem::attempts` never advances). /// @param itemId Id of the item whose attempt count changed. - /// @param attempts New cumulative attempt count to persist. - virtual void setAttempts([[maybe_unused]] uint64_t itemId, [[maybe_unused]] uint32_t attempts) {} + /// @param attempts New cumulative attempt count to persist. Typed, not a + /// bare integer, so a transposed call does not compile — see + /// `Attempts` for what that silently did before. + virtual void setAttempts([[maybe_unused]] uint64_t itemId, [[maybe_unused]] Attempts attempts) {} protected: /// @brief Stamps an idempotency key onto an already-enqueued item. @@ -250,6 +310,18 @@ struct IOfflineQueue { }; // NOLINTEND(cppcoreguidelines-special-member-functions) +// The point of `Attempts`, stated as something the compiler checks rather than +// as a comment. The second assertion is what keeps the first from being +// vacuous: if `Attempts` were an alias for `uint32_t` the transposed call would +// compile and the first assertion would fail, and if it were made explicit the +// ordinary call would stop compiling and the second would fail. Both directions +// have to hold. +static_assert(!std::is_invocable_v, + "setAttempts's parameters must not be transposable: writing an item id into an attempt count is " + "silent, because setAttempts on an unknown id is a documented no-op."); +static_assert(std::is_invocable_v, + "setAttempts must still accept a plain count in the right order."); + // ── In-memory implementation ────────────────────────────────────────────────── /// @brief Thread-safe in-memory implementation of `IOfflineQueue`. @@ -329,11 +401,11 @@ class InMemoryOfflineQueue : public IOfflineQueue { /// to simulate cross-restart dead-lettering in tests. /// @param itemId Id of the item to update. /// @param attempts New attempt count to store. - void setAttempts(uint64_t itemId, uint32_t attempts) override { + void setAttempts(uint64_t itemId, Attempts attempts) override { std::scoped_lock const lock{_mtx}; auto iter = std::ranges::find_if(_items, [itemId](const QueueItem& item) { return item.id == itemId; }); if (iter != _items.end()) { - iter->attempts = attempts; + iter->attempts = attempts.value(); } } diff --git a/include/morph/offline/sqlite_offline_queue.hpp b/include/morph/offline/sqlite_offline_queue.hpp index 64ad196b..48ac6570 100644 --- a/include/morph/offline/sqlite_offline_queue.hpp +++ b/include/morph/offline/sqlite_offline_queue.hpp @@ -464,10 +464,10 @@ class SqliteOfflineQueue : public IOfflineQueue { /// @brief Persists an updated attempt count for @p itemId. No-op if absent. /// @param itemId Id of the item whose count changed. /// @param attempts New cumulative attempt count to store. - void setAttempts(uint64_t itemId, uint32_t attempts) override { + void setAttempts(uint64_t itemId, Attempts attempts) override { std::scoped_lock const lock{_mtx}; detail::StatementGuard const guard{prepare("UPDATE morph_offline_queue SET attempts = ? WHERE id = ?;")}; - bindInt64(guard.get(), 1, static_cast(attempts)); + bindInt64(guard.get(), 1, static_cast(attempts.value())); bindInt64(guard.get(), 2, static_cast(itemId)); stepOrThrow(guard.get(), "setAttempts"); } diff --git a/tests/test_sync_worker.cpp b/tests/test_sync_worker.cpp index 6459140e..147a9d53 100644 --- a/tests/test_sync_worker.cpp +++ b/tests/test_sync_worker.cpp @@ -403,8 +403,8 @@ namespace { /// Records every `setAttempts` write so a test can prove the *durable* count /// is left alone, not merely the in-memory one. struct AttemptRecordingQueue : morph::offline::InMemoryOfflineQueue { - void setAttempts(uint64_t itemId, uint32_t attempts) override { - writes.emplace_back(itemId, attempts); + void setAttempts(uint64_t itemId, morph::offline::Attempts attempts) override { + writes.emplace_back(itemId, attempts.value()); morph::offline::InMemoryOfflineQueue::setAttempts(itemId, attempts); } std::vector> writes; From 367569e492530fccc82203da5b60be5539d12980 Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Tue, 22 Sep 2026 21:57:47 +0200 Subject: [PATCH 3/3] net: measure gai_strerror's thread safety instead of leaving it unestablished (fixes #640) The triage asked for one of two readings -- glibc's ATTRIBUTES table or the POSIX text -- and neither man page is installed on this machine either. So the property was measured directly, which is stronger than reading a table because it tests the hazard rather than the documentation of it. glibc 2.44, gcc -O0: EAI_AGAIN ptr=0x7febd9bb62f2 "Temporary failure in name resolution" EAI_FAIL ptr=0x7febd9bb632e "Non-recoverable failure in name resolution" EAI_AGAIN ptr=0x7febd9bb62f2 (second call, same code) distinct pointers for distinct codes: YES first string intact after second call: YES dladdr -> /usr/lib/libc.so.6, anonymous (a literal in a data section) unknown code 12345 -> "Unknown error" (a constant, not a formatted buffer) 7fa71e99f000-7fa71ea15000 r--p 0019f000 00:1c 15085232 /usr/lib/libc.so.6 The r--p is the load-bearing part. std::strerror's defect, which morph#625 removed, is a shared MUTABLE buffer a second caller overwrites; a read-only mapping cannot be one. Distinct pointers per code rule out a rotating slot, and even the unrecognised-code path returns a constant, so there is no per-call buffer on any path. musl/emscripten, which the WASM build links, is a static const char msgs[] table by inspection. Winsock's documented-unsafe gai_strerrorA is never compiled: TcpSocket is POSIX-only, and CI's Windows leg cannot reach this file. So the call stays, and now for two reasons the issue asked to be kept apart: rc is an EAI_* code and not an errno, so morph#641's errnoMessage() would render a confidently wrong string -- and separately, it does not have the defect that would make replacing it worth the effort. Recorded at the call site and in docs/spec/security.md, whose existing bullet said gai_strerror was "deliberately untouched" only for the first reason. Both records state what was NOT established: no other libc was checked and POSIX's own text is still unread, so this is a measurement on the configurations morph builds, not a portability guarantee. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01VptDWG2fKr2vBnLSJcgzgW --- docs/spec/security.md | 31 +++++++++++++++++++++---- include/morph/net/detail/tcp_socket.hpp | 28 ++++++++++++++++++++++ 2 files changed, 55 insertions(+), 4 deletions(-) diff --git a/docs/spec/security.md b/docs/spec/security.md index 66842a86..93abe85a 100644 --- a/docs/spec/security.md +++ b/docs/spec/security.md @@ -633,10 +633,33 @@ transport above is not a matter of degree: configuration this project tests, the two spellings render an `errno` to identical bytes. The property gained is that the guarantee now holds by specification rather than by the implementation happening to be safe. - `TcpSocket::connect`'s `::gai_strerror` is deliberately untouched: it renders - `EAI_*` resolver codes, which are not `errno` values, so - `std::system_category()` cannot describe them and no drop-in substitution - exists. + `TcpSocket::connect`'s `::gai_strerror` is deliberately untouched, for two + separate reasons that morph#640 asked to be kept apart. The first is that no + substitution exists: it renders `EAI_*` resolver codes, which are not `errno` + values, so `std::system_category().message()` would describe them + confidently and wrongly. The second is that it does not have + `std::strerror`'s defect in the first place — **measured**, not assumed. On + glibc 2.44 it returns a pointer to a string literal in libc's own read-only + mapping, distinct per code and stable across calls: + + ``` + EAI_AGAIN ptr=0x7fa71e9b62f2 "Temporary failure in name resolution" + EAI_FAIL ptr=0x7fa71e9b632e "Non-recoverable failure in name resolution" + dladdr -> /usr/lib/libc.so.6, anonymous (a literal in a data section) + 7fa71e99f000-7fa71ea15000 r--p ... /usr/lib/libc.so.6 + ``` + + The `r--p` is the load-bearing part: a shared scratch buffer of the kind + `std::strerror` is permitted to return would have to be writable. Even an + unrecognised code yields a constant ("Unknown error") rather than a formatted + one, so there is no per-call buffer on any path. The musl implementation the + WASM build links is a `static const char msgs[]` table by inspection, with + the same property, and `TcpSocket` is POSIX-only, so Winsock's + documented-unsafe `gai_strerrorA` is never compiled. What is **not** + established: no other libc was checked, and POSIX itself is not read as + requiring this — so the entry is a measurement on the configurations morph + builds, not a portability guarantee. Re-check if a platform with a different + libc joins CI. ## Residual limitations & hardening checklist diff --git a/include/morph/net/detail/tcp_socket.hpp b/include/morph/net/detail/tcp_socket.hpp index 80c85594..ea184d2d 100644 --- a/include/morph/net/detail/tcp_socket.hpp +++ b/include/morph/net/detail/tcp_socket.hpp @@ -109,6 +109,34 @@ class TcpSocket { std::string const portStr = std::to_string(static_cast(port)); int const rc = ::getaddrinfo(host.c_str(), portStr.c_str(), &hints, &resolved); if (rc != 0 || resolved == nullptr) { + // `::gai_strerror`, and NOT `errnoMessage()`: `rc` is an `EAI_*` + // code, not an `errno`, so `std::system_category().message(rc)` + // would render a confidently wrong string (morph#641). + // + // It also stays here rather than going the way `std::strerror` + // went in morph#625, because it does not have `std::strerror`'s + // defect. This throw site runs on threads this subsystem spawns + // (see `errnoMessage` below), so the question was live; it was + // measured rather than assumed (morph#640). + // + // glibc 2.44, `gcc -O0`: `gai_strerror` returns a pointer to a + // string literal inside libc's own read-only data, distinct per + // code and stable across calls -- + // + // EAI_AGAIN ptr=0x7fa71e9b62f2 "Temporary failure in name resolution" + // EAI_FAIL ptr=0x7fa71e9b632e "Non-recoverable failure in name resolution" + // dladdr -> /usr/lib/libc.so.6, anonymous (a literal in a data section) + // 7fa71e99f000-7fa71ea15000 r--p ... /usr/lib/libc.so.6 + // + // The `r--p` mapping is the load-bearing part: a shared scratch + // buffer would have to be writable. Even an unrecognised code + // returns a constant ("Unknown error"), not a formatted one. The + // musl/emscripten implementation morph's WASM build uses is a + // `static const char msgs[]` table by inspection, same property. + // `TcpSocket` is POSIX-only (this file's own class comment), so + // Winsock's documented-unsafe `gai_strerrorA` never applies. + // + // Re-check if a platform with a different libc joins CI. throw std::runtime_error("TcpSocket::connect: getaddrinfo failed for " + host + ": " + ::gai_strerror(rc)); } struct AddrInfoGuard {