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/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/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/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) 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/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 { 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;