From 9c28adc69481b27d861f294a493a83b8fcf033d0 Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Tue, 29 Sep 2026 15:43:42 +0200 Subject: [PATCH 1/4] forms(qml): read an optional enum's closed set one level down (#839) glaze emits std::optional for a glz::enumerate'd E as {"anyOf": [{"$ref": "#/$defs/E"}, {"type": "null"}]}, and $defs.E is the oneOf of const alternatives. enumChoices() gave up on the first non-null branch without a const, so every optional enum rendered as a free-text field. A non-null branch that, after resolveRef, is itself an all-const oneOf/anyOf now contributes its alternatives. Exactly one level; a set nested deeper still falls back. The field's kind becomes "enum", the value encodes as the JSON string, blank is omitted. tst_DynamicFormEnumChoice pins glaze's generated $ref shape verbatim, the issue's inlined nested shape, and the depth bound. Closes #839 Co-Authored-By: Claude Opus 5.5 --- docs/spec/forms/forms.md | 20 +++- src/qt/forms/qml/DynamicForm.qml | 41 +++++-- .../forms/tests/tst_DynamicFormEnumChoice.qml | 112 ++++++++++++++++++ 3 files changed, 164 insertions(+), 9 deletions(-) diff --git a/docs/spec/forms/forms.md b/docs/spec/forms/forms.md index 3b9331672..a134e6745 100644 --- a/docs/spec/forms/forms.md +++ b/docs/spec/forms/forms.md @@ -1259,12 +1259,30 @@ such a member as a `oneOf` of `const` alternatives, each carrying its own This is standard JSON-Schema vocabulary, not an `x-*` extension: no morph key declares it and none is needed. A renderer recognises the shape by the property holding a `oneOf`/`anyOf` in which **every** branch bar `{"type": "null"}` -carries a `const`. One branch without a `const` and it is not a closed set — +carries a `const` (or is itself such a set, for an optional member — below). +One branch that does neither and it is not a closed set — that is the nullability shape above, and a partial list would be worse than no list at all. The bare JSON-Schema `enum` keyword (`{"enum": ["a", "b"]}`), which glaze does not emit but a hand-written schema may, states the same thing and is read the same way. +An **optional** enum member (`std::optional`) reaches its set one level +down. glaze emits it as a nullable `anyOf` whose non-null branch is a `$ref` to +the enum's definition, and that definition is the `oneOf` of `const`s above: + +```json +"grade": {"anyOf": [{"$ref": "#/$defs/Grade"}, {"type": "null"}]}, +"$defs": {"Grade": {"type": "string", + "oneOf": [{"title": "Low", "const": "Low"}, + {"title": "High", "const": "High"}]}} +``` + +A non-null branch that, after following its `$ref`, is itself a +`oneOf`/`anyOf` of `const` alternatives therefore counts as those alternatives: +the member is the same closed set, and it is optional. Exactly one level is +read this way. A set nested deeper is not a shape any generator produces, and it +stays "not a closed set" rather than being guessed at. + Two obligations follow, and `DynamicForm.qml` meets both: - **Draw a selection control**, not a text field. The alternatives' `title`s are diff --git a/src/qt/forms/qml/DynamicForm.qml b/src/qt/forms/qml/DynamicForm.qml index 09442b2b5..1228552ec 100644 --- a/src/qt/forms/qml/DynamicForm.qml +++ b/src/qt/forms/qml/DynamicForm.qml @@ -461,10 +461,15 @@ Frame { // 2. The bare JSON Schema `enum` keyword, which glaze does not emit but // a hand-written or evolved schema may: `{"enum": ["a", "b"]}`. // + // An optional member wraps a shape-1 set in a nullable `anyOf`, one level + // down: `{"anyOf": [{"$ref": }, {"type": "null"}]}`. That inner + // set is read as the property's own (constBranchRows). + // // This is distinguishable from the nullable-`$ref` shape resolveProp - // collapses, whose branches carry no `const` at all: **one** - // branch without a `const` and the property is not a closed set, so the - // whole thing falls back rather than offering a partial list. + // collapses, whose branches carry no `const` at all: **one** branch that + // neither pins a value nor is itself a set of pinned values, and the + // property is not a closed set, so the whole thing falls back rather than + // offering a partial list. // // `valueJson` is the JSON literal of the value (a string alternative // therefore arrives quoted), matching the convention a server-fetched @@ -485,18 +490,38 @@ Frame { } return rows } + const rows = constBranchRows(p, true) + return rows === null ? [] : rows + } + + // The `const` alternatives of `p`'s `oneOf`/`anyOf`, or null when `p` has + // no such list or any non-null branch pins no value. + // + // With `nested`, a branch that is itself such a list contributes its + // alternatives. That one level is how glaze spells `std::optional` for + // an enumerated `E`: `{"anyOf": [{"$ref": "#/$defs/E"}, {"type": "null"}]}`, + // where `$defs.E` is the `oneOf` of `const`s. Deeper nesting is not a + // shape any generator produces, so it stays "not a closed set" rather than + // being guessed at. + function constBranchRows(p, nested) { const branches = Array.isArray(p.anyOf) ? p.anyOf : (Array.isArray(p.oneOf) ? p.oneOf : null) if (branches === null) - return [] + return null const rows = [] for (let i = 0; i < branches.length; ++i) { const branch = resolveRef(branches[i]) if (branch.type === "null") continue - if (branch["const"] === undefined) - return [] - rows.push({ label: String(opt(branch.title, branch["const"])), - valueJson: JSON.stringify(branch["const"]) }) + if (branch["const"] !== undefined) { + rows.push({ label: String(opt(branch.title, branch["const"])), + valueJson: JSON.stringify(branch["const"]) }) + continue + } + const inner = nested ? constBranchRows(branch, false) : null + if (inner === null || inner.length === 0) + return null + for (let j = 0; j < inner.length; ++j) + rows.push(inner[j]) } return rows } diff --git a/src/qt/forms/tests/tst_DynamicFormEnumChoice.qml b/src/qt/forms/tests/tst_DynamicFormEnumChoice.qml index dbe108000..fbff8b734 100644 --- a/src/qt/forms/tests/tst_DynamicFormEnumChoice.qml +++ b/src/qt/forms/tests/tst_DynamicFormEnumChoice.qml @@ -17,6 +17,9 @@ // - pastebin::CreatePaste (Visibility, Editability) -- a rung whose GUI // binds DynamicForm { actionType: "CreatePaste" } today // +// plus glaze's own output for an optional enum member (`optionalEnumSchema`), +// which no shipped action carries yet but any `std::optional` produces. +// // The `enum` keyword and the "not a closed set" fallbacks are hand-written: // glaze emits neither, and the point of both is what a renderer does with a // schema it did not generate. @@ -118,6 +121,42 @@ TestCase { "required": [] }) + // glaze's schema for `struct S { Grade req{}; std::optional grade; }` + // with `Grade` a `glz::enumerate`d `enum class { Low, High }`, verbatim. + // The optional member reaches its set only through a `$ref` inside a + // nullable `anyOf`, one level below the property. + property var optionalEnumSchema: ({ + "type": "object", + "properties": { + "grade": { "anyOf": [{ "$ref": "#/$defs/Grade" }, { "type": "null" }] }, + "req": { "$ref": "#/$defs/Grade" } + }, + "$defs": { + "Grade": { "type": "string", + "oneOf": [{ "title": "Low", "const": "Low" }, + { "title": "High", "const": "High" }] } + }, + "required": ["req"] + }) + + // Hand-written nestings of the same set: + // `inlined` -- the set written in place of glaze's `$ref`. + // `deeper` -- the set two levels down. No generator produces it, so it + // stays "not a closed set" rather than being guessed at. + property var nestedEnumSchema: ({ + "properties": { + "inlined": { "anyOf": [{ "oneOf": [{ "title": "Low", "const": "Low" }, + { "title": "High", "const": "High" }] }, + { "type": "null" }], + "x-order": 0 }, + "deeper": { "anyOf": [{ "anyOf": [{ "oneOf": [{ "title": "Low", "const": "Low" }, + { "title": "High", "const": "High" }] }] }, + { "type": "null" }], + "x-order": 1 } + }, + "required": [] + }) + // A server-fetched Choice, unchanged by any of this: its options are not // in the schema, so it must still fetch and must still not be gated on a // membership check this renderer cannot make. @@ -144,6 +183,16 @@ TestCase { DynamicForm { actionType: "HandWritten"; schema: testCase.handWrittenSchema; controller: null } } + Component { + id: optionalEnumForm + DynamicForm { actionType: "OptionalEnum"; schema: testCase.optionalEnumSchema; controller: null } + } + + Component { + id: nestedEnumForm + DynamicForm { actionType: "NestedEnum"; schema: testCase.nestedEnumSchema; controller: null } + } + Component { id: choiceForm DynamicForm { actionType: "Choice"; schema: testCase.choiceSchema; controller: mockController } @@ -206,6 +255,69 @@ TestCase { compare(nullable.enumOptions.map(function (r) { return r.label }), ["On", "Off"]) } + // ── an optional enum: the set one level down ───────────────────────────── + + function test_glazes_optional_enum_is_a_closed_set_of_kind_enum() { + var form = createTemporaryObject(optionalEnumForm, testCase) + verify(form !== null) + var grade = meta(form, "grade") + compare(grade.isEnum, true) + compare(grade.kind, "enum") + compare(grade.enumOptions.map(function (r) { return r.label }), ["Low", "High"]) + compare(grade.enumOptions.map(function (r) { return r.valueJson }), ['"Low"', '"High"']) + // The required sibling reaches the same set through a bare `$ref`. + compare(meta(form, "req").kind, "enum") + } + + function test_glazes_optional_enum_renders_a_selection_control() { + var form = createTemporaryObject(optionalEnumForm, testCase) + var control = findChild(form, "field_grade") + verify(control !== null) + verify(control.currentIndex !== undefined) + verify(control.placeholderText === undefined) + compare(control.count, 2) + compare(control.textAt(1), "High") + } + + function test_glazes_optional_enum_encodes_the_name_and_blank_is_omitted() { + var form = createTemporaryObject(optionalEnumForm, testCase) + var req = findChild(form, "field_req") + req.currentIndex = 0 + req.activated(0) + // `grade` is optional and untouched: no value, not an invalid one. + compare(form.ready, true) + compare(form.previewLine.indexOf("grade"), -1) + + var grade = findChild(form, "field_grade") + grade.currentIndex = 1 + grade.activated(1) + compare(form.ready, true) + verify(form.previewLine.indexOf('"grade":"High"') !== -1) + } + + function test_glazes_optional_enum_refuses_a_value_outside_the_set() { + var form = createTemporaryObject(optionalEnumForm, testCase) + var req = findChild(form, "field_req") + req.currentIndex = 0 + req.activated(0) + form.setFieldValue("grade", '"Medium"') + compare(form.ready, false) + } + + function test_an_inlined_nested_set_is_a_closed_set_too() { + var form = createTemporaryObject(nestedEnumForm, testCase) + var inlined = meta(form, "inlined") + compare(inlined.isEnum, true) + compare(inlined.kind, "enum") + compare(inlined.enumOptions.map(function (r) { return r.valueJson }), ['"Low"', '"High"']) + } + + function test_a_set_nested_two_levels_down_is_not_a_closed_set() { + var form = createTemporaryObject(nestedEnumForm, testCase) + compare(meta(form, "deeper").isEnum, false) + compare(meta(form, "deeper").enumOptions.length, 0) + } + // ── what is *not* a closed set keeps its old behaviour ─────────────────── function test_a_branch_without_a_const_makes_it_not_a_closed_set() { From 59976e550bf57cd00e61cbdedfdb940141a611f7 Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Tue, 29 Sep 2026 16:12:55 +0200 Subject: [PATCH 2/4] core: promote runPostCommitTail into morph::model (#789) Every rung that journals from inside execute() commits and then keeps working, and a throw from that post-commit work told the caller a durable write had failed. kanban hand-rolled the containment; no other rung had it. morph::model::runPostCommitTail (include/morph/core/model.hpp) is kanban's helper promoted as-is, with the value-returning overload's result type made a template parameter deduced from `committed`: run the tail, log a failure through morph::log::logError, never rethrow. The void overload is noexcept and rejects a value-returning tail at compile time. - kanban: local copy deleted; the ten call sites use the framework helper. - ledger: every logAction after a durable write (StoreTransaction and its rule cascade, SetCategory, CreateLedger, OpenAccount, UndoTransaction, ImportLedgerChunk, RunReportJob) now runs through it. - tests/test_post_commit_tail.cpp: framework-level coverage, including a model-shaped handler journalling to a refusing sink. - examples/ledger/tests/test_ledger_post_commit_tail.cpp: ledger shown using it; fails on master's unshielded code. - docs/spec/core/post_commit_tail.md: new spec. Closes #789 Co-Authored-By: Claude Opus 5.5 --- docs/spec/README.md | 1 + docs/spec/core/post_commit_tail.md | 154 ++++++++++++ .../include/kanban/models/board_model.hpp | 2 +- examples/kanban/src/models/board_model.cpp | 120 ++------- examples/ledger/src/models/ledger_model.cpp | 50 ++-- .../tests/test_ledger_post_commit_tail.cpp | 118 +++++++++ include/morph/core/model.hpp | 72 ++++++ tests/CMakeLists.txt | 1 + tests/test_post_commit_tail.cpp | 229 ++++++++++++++++++ 9 files changed, 629 insertions(+), 118 deletions(-) create mode 100644 docs/spec/core/post_commit_tail.md create mode 100644 examples/ledger/tests/test_ledger_post_commit_tail.cpp create mode 100644 tests/test_post_commit_tail.cpp diff --git a/docs/spec/README.md b/docs/spec/README.md index 6b0fbdd27..29ad02ce5 100644 --- a/docs/spec/README.md +++ b/docs/spec/README.md @@ -101,6 +101,7 @@ behavioural differences between the two, collected in one table, are in **When things go wrong** [`error_handling.md`](error_handling.md) · +[`core/post_commit_tail.md`](core/post_commit_tail.md) · [`core/logger.md`](core/logger.md) · [`core/observability.md`](core/observability.md) diff --git a/docs/spec/core/post_commit_tail.md b/docs/spec/core/post_commit_tail.md new file mode 100644 index 000000000..0e583acd7 --- /dev/null +++ b/docs/spec/core/post_commit_tail.md @@ -0,0 +1,154 @@ +# `runPostCommitTail` — design + +Design spec for `morph::model::runPostCommitTail` (`include/morph/core/model.hpp`): +the seam between an action handler's commit and the work that follows it. + +Read this before writing a mutating `execute()` that does anything after its +commit — journalling to a model-owned log, a rule cascade, rebuilding cached +state. + +## Contents + +- [The problem](#the-problem) +- [API surface](#api-surface) +- [Where the tail starts](#where-the-tail-starts) +- [Why a utility, not a dispatch-layer hook](#why-a-utility-not-a-dispatch-layer-hook) +- [Relation to the framework's own recording](#relation-to-the-frameworks-own-recording) +- [Out of scope](#out-of-scope) +- [Cross-references](#cross-references) + +## The problem + +A mutating handler validates, opens a transaction, mutates, **commits**, and +then often keeps working: it appends to an action log it owns, fires rules the +write triggered, or re-reads state for its return value. Everything after the +commit can still throw — a contended `SQLITE_BUSY` past the busy timeout, a +journal sink whose `append` refuses the entry (which +[`IActionLog::append`](../journal/journal.md)'s contract *requires* it to signal +by throwing). + +If that exception leaves `execute()`, the caller is told the action failed while +the write it made stands. A client that believes it will retry, and the retry +applies the mutation twice. That is the defect this helper closes: **a call must +not report failure for a mutation that already happened.** + +## API surface + +```cpp +namespace morph::model { + +template + requires std::invocable && std::is_void_v> +void runPostCommitTail(Tail&& tail, std::string_view what) noexcept; + +template + requires std::invocable && std::convertible_to, Result> +[[nodiscard]] Result runPostCommitTail(Tail&& tail, Result committed, std::string_view what) + noexcept(std::is_nothrow_move_constructible_v); + +} +``` + +Both overloads run `tail` once. If it throws, the exception is caught — a +`std::exception` and anything else alike — and reported through +`morph::log::logError`: + +``` + committed, but its post-commit tail failed: + committed, but its post-commit tail threw a non-std::exception +``` + +Nothing is rethrown. `what` names the handler; by convention it carries the +model's tag too (`"[kanban::BoardModel] CreateColumn"`), because the log line is +the only place the failure surfaces. + +- **The `void` overload** is for a tail whose result the caller does not need — + the common case, where the tail is journalling alone and the handler's return + value was computed before the commit. It is `noexcept`: `logError`'s + formatting overload is itself `noexcept`, so nothing on the path can escape. +- **The value-returning overload** is for a handler whose return value is + *refreshed* by the tail — a board state rebuilt after a rule cascade the move + fired. `committed` is the truthful answer the handler already holds from + before the tail began; if the tail throws, that is what is returned. `Result` + is deduced from `committed` alone, so the caller receives the handler's own + result type and the tail may yield anything convertible to it. It is + `noexcept` whenever returning `committed` cannot throw. + +The `void` overload rejects a value-returning tail at compile time rather than +discarding its value, because a tail that computes something is almost always a +tail whose value the caller was meant to get. + +## Where the tail starts + +Only work *after* the commit goes through the helper. An exception from before +the commit still means the mutation did not happen, and it must still reach the +caller: wrapping it would report success for a write that was rolled back. + +Anything the caller's **return value** depends on is best computed *before* the +commit, inside the transaction. A re-read that fails there rolls the write back, +so the caller's "this failed" is true. A re-read that fails after the commit +leaves nothing truthful to return unless the handler already holds an answer — +which is exactly the value-returning overload's precondition. The cost of +reading inside the transaction is that the write lock is held for the length of +the read; for SQLite under contention that is measurable, and the handler that +already needs the state inside the transaction (for an idempotency ledger row, +say) pays nothing extra. + +## Why a utility, not a dispatch-layer hook + +The seam is a function the handler calls on itself, at the point in its own +body where it knows the commit succeeded. The alternatives — a result type +carrying a deferred continuation, an `ActionTraits` after-commit hook the +dispatcher calls, a journal decorator that swallows `append` failures — all add +an extension point the framework calls into after `execute()` returns. None is +needed for this, and each costs more: + +- A hook called after `execute()` returns runs on a different frame, so it + cannot see what the handler computed inside the transaction unless that + travels in the result. A rule cascade built from rows inserted in the same + transaction is an ordinary local variable to a tail closure. +- A continuation-carrying result type would change every `execute()` signature + and have to be hidden from `resultToJson`, the wire and `journal::replay`. +- A swallowing journal decorator covers only journalling, and makes "the entry + is recorded" no longer a promise the outbox relay can rely on. + +It also keeps a model independent of the dispatch layer: there is no new +model-to-dispatcher contract, only a utility in the same category as +`morph::log::logError`, and a directly constructed model gets the same +guarantee as a registered one. + +The one shape this does not express is post-commit work that must run in a +**different execution context** from the one `execute()` returns on — genuinely +asynchronous follow-on work. The tail runs synchronously, before `execute()` +returns. A handler that needs the other shape needs one of the dispatch-level +designs above, alongside this helper rather than instead of it. + +## Relation to the framework's own recording + +For a registered model, the dispatch layer appends the action's own +`Succeeded` entry after `execute()` returns, and a sink failure there surfaces +as `morph::model::ActionRecordingError` — the caller is told the write happened +and its record did not (journal.md, "A refused recording is not an execution +failure"). That path has a channel back to the caller, because the dispatcher +owns the reply. + +A handler's own tail has no such channel without changing its result type, so +the helper's channel is the error log. The two are complementary: the framework +covers the entry it writes, the helper covers the work a handler does itself. + +## Out of scope + +- **Failures before the commit.** A non-domain exception thrown before the + commit is the action's failure, and a registered model's dispatch site records + it `Outcome::Failed` already. The helper deliberately does not touch that + path. +- **Retrying the tail.** A failed tail is logged, not retried. Whether a missed + journal entry or cascade should be recovered is the application's decision. + +## Cross-references + +| Spec | Why | +|---|---| +| [registry.md](registry.md) | `ActionDispatcher`, `IModelHolder` and the dispatch sites whose recording this complements. | +| [../journal/journal.md](../journal/journal.md) | `IActionLog::append`'s throwing contract; `ActionRecordingError`. | +| [logger.md](logger.md) | `morph::log::logError`, the helper's reporting channel. | diff --git a/examples/kanban/include/kanban/models/board_model.hpp b/examples/kanban/include/kanban/models/board_model.hpp index 200fd98d8..09b259cd3 100644 --- a/examples/kanban/include/kanban/models/board_model.hpp +++ b/examples/kanban/include/kanban/models/board_model.hpp @@ -393,7 +393,7 @@ class BoardModel { /// *replace* the failure being reported with the failure to /// report it: a less diagnosable exception, and on a destructor /// path a `std::terminate`. So it is contained here, the same way - /// `runPostCommitTail` contains the mirror case, and + /// `morph::model::runPostCommitTail` contains the mirror case, and /// the exception the caller sees is always the original one. /// /// **Precondition:** an exception is being handled. This is a diff --git a/examples/kanban/src/models/board_model.cpp b/examples/kanban/src/models/board_model.cpp index 0dac8da15..c09aa1ce6 100644 --- a/examples/kanban/src/models/board_model.cpp +++ b/examples/kanban/src/models/board_model.cpp @@ -9,6 +9,7 @@ #include #include #include +#include #include #include #include @@ -164,84 +165,6 @@ void requireProjectMatchesAttachedBoard(ProjectId projectId, std::uint64_t attac } } -/// @brief Runs @p tail, a handler's *post-commit* work, and contains any -/// exception it throws. -/// -/// A handler that has already called `SqlTransaction::Commit()` has made its -/// mutation durable. Whatever it does afterwards -- journalling, a rule -/// cascade, a re-read for the return value -- can still fail (a contended -/// `SQLITE_BUSY` past the busy timeout is the failure observed in CI), and -/// before this existed such a failure propagated out of `execute()`: the -/// caller was told the action had failed, while the row it wrote was -/// committed, and `execute()`'s own catch journalled an `Outcome::Failed` -/// entry for it. A call must not report failure for a mutation that already -/// happened. -/// -/// So the tail's exceptions stop here and @p committed -- the state the -/// handler did commit -- is returned instead. Only the post-commit tail goes -/// through this: an exception from *before* the commit still means the -/// mutation did not happen and must still reach the caller. -/// -/// @tparam Tail Nullary callable returning `GetBoardResult`. -/// @param tail The post-commit work to run. -/// @param committed The state to return if @p tail throws. -/// @param what A phrase naming the handler, for the log line. -/// @return @p tail's result, or @p committed if it threw. -template -[[nodiscard]] GetBoardResult runPostCommitTail(Tail&& tail, GetBoardResult committed, std::string_view what) { - try { - return std::forward(tail)(); - } catch (const std::exception& error) { - ::morph::log::logError(std::string{"[kanban::BoardModel] "} + std::string{what} + - " committed, but its post-commit tail failed: " + error.what()); - } catch (...) { - ::morph::log::logError(std::string{"[kanban::BoardModel] "} + std::string{what} + - " committed, but its post-commit tail threw a non-std::exception"); - } - return committed; -} - -/// @brief The same containment for a tail that produces nothing the caller -/// needs -- the shape every mutating handler other than -/// `MoveTaskPosition` has. -/// -/// Those handlers' tails are `logAction` alone: the value they return was -/// already computed, so there is no fallback to choose and no second value to -/// reconcile. Giving them the three-argument overload above would mean passing -/// a `committed` that the tail also returns unchanged -- a branch no input can -/// distinguish, which is worse than no branch. This overload is that case -/// written down. -/// -/// **What each handler must do to be eligible**: anything the *caller's return -/// value* depends on runs before `Commit()`, not after it. `CreateColumn` and -/// its three siblings therefore build their `GetBoardResult` inside the -/// transaction, which is where `MoveTaskPosition` builds its own (it needs one -/// for the applied-ops ledger row). That is not a workaround for this -/// overload's lack of a fallback -- it is the stronger ordering. A re-read that -/// fails *before* the commit rolls the write back, so the caller's "this -/// failed" is true; a re-read that fails *after* it leaves nothing truthful to -/// return, because the board state is the answer and there is no partial board -/// worth sending. The cost is that the write transaction now spans the read, so -/// it holds SQLite's write lock for longer under contention; -/// `MoveTaskPosition`, the heaviest handler in this file, has held it across -/// exactly that read since this rung was written. -/// -/// @tparam Tail Nullary callable returning `void`. -/// @param tail The post-commit work to run. -/// @param what A phrase naming the handler, for the log line. -template -void runPostCommitTail(Tail&& tail, std::string_view what) { - try { - std::forward(tail)(); - } catch (const std::exception& error) { - ::morph::log::logError(std::string{"[kanban::BoardModel] "} + std::string{what} + - " committed, but its post-commit tail failed: " + error.what()); - } catch (...) { - ::morph::log::logError(std::string{"[kanban::BoardModel] "} + std::string{what} + - " committed, but its post-commit tail threw a non-std::exception"); - } -} - [[nodiscard]] GetBoardResult buildState(::Lightweight::DataMapper& mapper, const db::ProjectRecord& project) { GetBoardResult result; result.projectId = ProjectId{static_cast(project.id.Value())}; @@ -586,8 +509,8 @@ GetBoardResult BoardModel::execute(const CreateColumn& action) { // Built before the commit, not after it. The board state is this call's // whole return value, so a re-read that fails must roll the write back // rather than leave a committed mutation with nothing truthful to - // report -- see `runPostCommitTail`'s void overload for the reasoning - // and its cost. `MoveTaskPosition` has always read here. + // report (`morph::model::runPostCommitTail`'s spec has the reasoning + // and its cost). `MoveTaskPosition` has always read here. auto result = buildState(mapper.Get(), project); transaction.Commit(); @@ -596,7 +519,7 @@ GetBoardResult BoardModel::execute(const CreateColumn& action) { // The row is durable from the line above; `logAction` is not, and // `_log->append`/`flush` can throw. A throw here must not tell the // caller the CreateColumn failed. - runPostCommitTail([&] { logAction(action, result); }, "CreateColumn"); + ::morph::model::runPostCommitTail([&] { logAction(action, result); }, "[kanban::BoardModel] CreateColumn"); return result; } catch (...) { logFailureForCurrentException(action); @@ -638,8 +561,8 @@ GetBoardResult BoardModel::execute(const CreateSwimlane& action) { // Built before the commit, not after it. The board state is this call's // whole return value, so a re-read that fails must roll the write back // rather than leave a committed mutation with nothing truthful to - // report -- see `runPostCommitTail`'s void overload for the reasoning - // and its cost. `MoveTaskPosition` has always read here. + // report (`morph::model::runPostCommitTail`'s spec has the reasoning + // and its cost). `MoveTaskPosition` has always read here. auto result = buildState(mapper.Get(), project); transaction.Commit(); @@ -648,7 +571,7 @@ GetBoardResult BoardModel::execute(const CreateSwimlane& action) { // The row is durable from the line above; `logAction` is not, and // `_log->append`/`flush` can throw. A throw here must not tell the // caller the CreateSwimlane failed. - runPostCommitTail([&] { logAction(action, result); }, "CreateSwimlane"); + ::morph::model::runPostCommitTail([&] { logAction(action, result); }, "[kanban::BoardModel] CreateSwimlane"); return result; } catch (...) { logFailureForCurrentException(action); @@ -705,8 +628,8 @@ GetBoardResult BoardModel::execute(const CreateTask& action) { // Built before the commit, not after it. The board state is this call's // whole return value, so a re-read that fails must roll the write back // rather than leave a committed mutation with nothing truthful to - // report -- see `runPostCommitTail`'s void overload for the reasoning - // and its cost. `MoveTaskPosition` has always read here. + // report (`morph::model::runPostCommitTail`'s spec has the reasoning + // and its cost). `MoveTaskPosition` has always read here. auto result = buildState(mapper.Get(), project); transaction.Commit(); @@ -715,7 +638,7 @@ GetBoardResult BoardModel::execute(const CreateTask& action) { // The row is durable from the line above; `logAction` is not, and // `_log->append`/`flush` can throw. A throw here must not tell the // caller the CreateTask failed. - runPostCommitTail([&] { logAction(action, result); }, "CreateTask"); + ::morph::model::runPostCommitTail([&] { logAction(action, result); }, "[kanban::BoardModel] CreateTask"); return result; } catch (...) { logFailureForCurrentException(action); @@ -760,8 +683,8 @@ GetBoardResult BoardModel::execute(const AddComment& action) { // Built before the commit, not after it. The board state is this call's // whole return value, so a re-read that fails must roll the write back // rather than leave a committed mutation with nothing truthful to - // report -- see `runPostCommitTail`'s void overload for the reasoning - // and its cost. `MoveTaskPosition` has always read here. + // report (`morph::model::runPostCommitTail`'s spec has the reasoning + // and its cost). `MoveTaskPosition` has always read here. auto result = buildState(mapper.Get(), project); transaction.Commit(); @@ -770,7 +693,7 @@ GetBoardResult BoardModel::execute(const AddComment& action) { // The row is durable from the line above; `logAction` is not, and // `_log->append`/`flush` can throw. A throw here must not tell the // caller the AddComment failed. - runPostCommitTail([&] { logAction(action, result); }, "AddComment"); + ::morph::model::runPostCommitTail([&] { logAction(action, result); }, "[kanban::BoardModel] AddComment"); return result; } catch (...) { logFailureForCurrentException(action); @@ -827,7 +750,7 @@ Ack BoardModel::execute(const AddAttachment& action) { // which is already true the moment the commit above returns. All the // tail does is journal, and a journal that refuses the entry does not // make the attachment un-added. - runPostCommitTail([&] { logAction(action, Ack{}); }, "AddAttachment"); + ::morph::model::runPostCommitTail([&] { logAction(action, Ack{}); }, "[kanban::BoardModel] AddAttachment"); return Ack{}; } catch (...) { logFailureForCurrentException(action); @@ -904,7 +827,7 @@ Ack BoardModel::execute(const RemoveAttachment& action) { transaction.Commit(); // ── post-commit tail ──────────────────────────────────────────── - runPostCommitTail([&] { logAction(action, Ack{}); }, "RemoveAttachment"); + ::morph::model::runPostCommitTail([&] { logAction(action, Ack{}); }, "[kanban::BoardModel] RemoveAttachment"); return Ack{}; } catch (...) { logFailureForCurrentException(action); @@ -965,7 +888,7 @@ CreateRuleResult BoardModel::execute(const CreateRule& action) { transaction.Commit(); // ── post-commit tail ──────────────────────────────────────────── - runPostCommitTail([&] { logAction(action, result); }, "CreateRule"); + ::morph::model::runPostCommitTail([&] { logAction(action, result); }, "[kanban::BoardModel] CreateRule"); return result; } catch (...) { logFailureForCurrentException(action); @@ -1041,7 +964,7 @@ Ack BoardModel::execute(const DeleteRule& action) { transaction.Commit(); // ── post-commit tail ──────────────────────────────────────────── - runPostCommitTail([&] { logAction(action, Ack{}); }, "DeleteRule"); + ::morph::model::runPostCommitTail([&] { logAction(action, Ack{}); }, "[kanban::BoardModel] DeleteRule"); return Ack{}; } catch (...) { logFailureForCurrentException(action); @@ -1076,7 +999,8 @@ ApplyTagMutationResult BoardModel::execute(const ApplyTagMutation& action) { // committed by the time it returns, so this `logAction` is post-commit // exactly as the other handlers' are, even though the `Commit()` is not // visible in this function. - runPostCommitTail([&] { logAction(action, ApplyTagMutationResult{}); }, "ApplyTagMutation"); + ::morph::model::runPostCommitTail([&] { logAction(action, ApplyTagMutationResult{}); }, + "[kanban::BoardModel] ApplyTagMutation"); return ApplyTagMutationResult{}; } catch (...) { logFailureForCurrentException(action); @@ -1330,10 +1254,10 @@ GetBoardResult BoardModel::execute(const MoveTaskPosition& action) { // It nonetheless runs against the same contended SQLite file: // `evaluateRules` queries (and may write), and `buildState` re-reads, // both of which can hit `SQLITE_BUSY` past the busy timeout under this - // rung's stress test. `runPostCommitTail` (this file, above) contains + // rung's stress test. `morph::model::runPostCommitTail` contains // that and explains why; the pre-cascade `result` is what a caller gets // if the tail fails. - return runPostCommitTail( + return ::morph::model::runPostCommitTail( [&] { logAction(action, result); @@ -1367,7 +1291,7 @@ GetBoardResult BoardModel::execute(const MoveTaskPosition& action) { // recorded. return buildState(mapper.Get(), project); }, - result, "MoveTaskPosition"); + result, "[kanban::BoardModel] MoveTaskPosition"); } catch (...) { logFailureForCurrentException(action); throw; diff --git a/examples/ledger/src/models/ledger_model.cpp b/examples/ledger/src/models/ledger_model.cpp index 8e5426c2e..a879cf1be 100644 --- a/examples/ledger/src/models/ledger_model.cpp +++ b/examples/ledger/src/models/ledger_model.cpp @@ -12,6 +12,7 @@ #include #include #include +#include #include #include #include @@ -683,7 +684,7 @@ CreateLedgerResult LedgerModel::execute(const CreateLedger& action) { ledgerRow.owner = Light::SqlAnsiString<64>{ctx->principal}; mapper.Create(ledgerRow); auto result = CreateLedgerResult{.id = LedgerId{static_cast(ledgerRow.id.Value())}}; - logAction(action, result); + ::morph::model::runPostCommitTail([&] { logAction(action, result); }, "[ledger::LedgerModel] CreateLedger"); return result; } catch (const LedgerError& error) { logFailure(action, error.what()); @@ -734,7 +735,7 @@ AccountInfo LedgerModel::execute(const OpenAccount& action) { // precision (0 for JPY/KRW, 2 for // USD/EUR), not a hardcoded 2 }; - logAction(action, result); + ::morph::model::runPostCommitTail([&] { logAction(action, result); }, "[ledger::LedgerModel] OpenAccount"); return result; } catch (const LedgerError& error) { logFailure(action, error.what()); @@ -1051,19 +1052,27 @@ GetLedgerResult LedgerModel::execute(const StoreTransaction& action) { sqlTxn.Commit(); - logAction(action, result); - - // Logged only now, after the trigger's own entry above, so the cascade - // always lands strictly after its trigger in the log's seq order. - // logAction is the *only* logger for each of these entries -- - // setCategoryImpl holds no logging of its own, and the public - // execute(SetCategory) overload (which also calls setCategoryImpl, then - // logs unconditionally with an empty causalParentId) is deliberately not - // called from here, to avoid double-logging the same firing. - const std::string triggerCausalId = "transactionJournal:" + std::to_string(journalRow.id.Value()); - for (const auto& cascadeAction : cascadesToLog) { - logAction(cascadeAction, SetCategoryResult{}, triggerCausalId); - } + // The transaction and its cascades are durable from the line above. + // Journalling them can still throw, and that must not tell the caller + // the StoreTransaction failed. + ::morph::model::runPostCommitTail( + [&] { + logAction(action, result); + + // Logged only now, after the trigger's own entry above, so the + // cascade always lands strictly after its trigger in the log's + // seq order. logAction is the *only* logger for each of these + // entries -- setCategoryImpl holds no logging of its own, and + // the public execute(SetCategory) overload (which also calls + // setCategoryImpl, then logs unconditionally with an empty + // causalParentId) is deliberately not called from here, to + // avoid double-logging the same firing. + const std::string triggerCausalId = "transactionJournal:" + std::to_string(journalRow.id.Value()); + for (const auto& cascadeAction : cascadesToLog) { + logAction(cascadeAction, SetCategoryResult{}, triggerCausalId); + } + }, + "[ledger::LedgerModel] StoreTransaction"); return result; } catch (const LedgerError& error) { @@ -1165,7 +1174,8 @@ GetLedgerResult LedgerModel::execute(const UndoTransaction& action) { "Reversal of: " + std::string{originalJournalRow.description.Value().ToStringView()}, morph::time::Timestamp::now(), reversalLegs, reversalLegAccounts, reversalCausalParentId); - logAction(action, result, reversalCausalParentId); + ::morph::model::runPostCommitTail([&] { logAction(action, result, reversalCausalParentId); }, + "[ledger::LedgerModel] UndoTransaction"); return result; } catch (const LedgerError& error) { logFailure(action, error.what()); @@ -1323,7 +1333,8 @@ ImportResult LedgerModel::execute(const ImportLedgerChunk& action) { } auto result = ImportResult{.imported = imported, .duplicates = duplicates}; - logAction(action, result); + ::morph::model::runPostCommitTail([&] { logAction(action, result); }, + "[ledger::LedgerModel] ImportLedgerChunk"); return result; } catch (const LedgerError& error) { logFailure(action, error.what()); @@ -1532,7 +1543,7 @@ RunReportJobResult LedgerModel::execute(const RunReportJob& action) { } auto result = RunReportJobResult{.status = ReportStatus::Done}; - logAction(action, result); + ::morph::model::runPostCommitTail([&] { logAction(action, result); }, "[ledger::LedgerModel] RunReportJob"); return result; } @@ -1573,7 +1584,8 @@ SetCategoryResult LedgerModel::execute(const SetCategory& action) { Lightweight::SqlTransaction sqlTxn{mapper.Connection(), Lightweight::SqlTransactionMode::ROLLBACK}; setCategoryImpl(mapper, action); sqlTxn.Commit(); - logAction(action, SetCategoryResult{}); + ::morph::model::runPostCommitTail([&] { logAction(action, SetCategoryResult{}); }, + "[ledger::LedgerModel] SetCategory"); return SetCategoryResult{}; } catch (const LedgerError& error) { logFailure(action, error.what()); diff --git a/examples/ledger/tests/test_ledger_post_commit_tail.cpp b/examples/ledger/tests/test_ledger_post_commit_tail.cpp new file mode 100644 index 000000000..ebf405550 --- /dev/null +++ b/examples/ledger/tests/test_ledger_post_commit_tail.cpp @@ -0,0 +1,118 @@ +// SPDX-License-Identifier: Apache-2.0 +// +// A ledger write whose journalling fails *after* the write is durable must not +// be reported to the caller as a failure. Each case attaches a sink that +// refuses every entry, asserts the call returns normally, that the sink really +// was asked (so the case cannot pass because nothing threw), and that the write +// is visible on a fresh read. + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include "ledger/core/money.hpp" +#include "ledger/db/ledger_entity.hpp" +#include "ledger/models/ledger_model.hpp" +#include "testkit/db_fixture.hpp" + +namespace { + +/// @brief See `test_ledger_model.cpp`'s identical `contextFor`. +[[nodiscard]] morph::session::Context contextFor(std::string principal) { + morph::session::Context ctx; + ctx.principal = std::move(principal); + return ctx; +} + +class ScopedPrincipal { +public: + explicit ScopedPrincipal(std::string principal) : _ctx{contextFor(std::move(principal))}, _scope{_ctx} {} + +private: + morph::session::Context _ctx; + morph::session::detail::ScopedContext _scope; +}; + +/// @brief An action log whose `append()` throws, as `IActionLog::append`'s +/// contract requires of a sink that could not record the entry. +class RefusingActionLog : public morph::journal::IActionLog { +public: + void append(morph::journal::LogEntry /*entry*/) override { + ++appendAttempts; + throw std::runtime_error{"RefusingActionLog: append refused"}; + } + + void flush() override {} + + [[nodiscard]] std::vector entries(std::string_view /*entityKey*/ = {}) const override { + return {}; + } + + int appendAttempts = 0; +}; + +} // namespace + +TEST_CASE("OpenAccount and StoreTransaction report success when only their journalling fails", + "[ledger][journal][post_commit_tail]") { + const morph::ladder::testkit::DbFixture fixture; + Lightweight::DataMapper mapper; + ledger::db::LedgerRecord ledgerRow; + ledgerRow.name = "Personal"; + mapper.Create(ledgerRow); + const auto ledgerId = ledger::LedgerId{static_cast(ledgerRow.id.Value())}; + + ledger::LedgerModel model; + const ScopedPrincipal principal{"alice"}; + auto log = std::make_shared(); + model.attachActionLog(log, std::to_string(*ledgerId)); + + ledger::AccountInfo checking; + ledger::AccountInfo groceries; + REQUIRE_NOTHROW(checking = model.execute(ledger::OpenAccount{.ledgerId = ledgerId, + .name = "Checking", + .kind = ledger::AccountKind::Asset, + .currency = ledger::Currency::USD})); + REQUIRE_NOTHROW(groceries = model.execute(ledger::OpenAccount{.ledgerId = ledgerId, + .name = "Groceries", + .kind = ledger::AccountKind::Expense, + .currency = ledger::Currency::USD})); + CHECK(log->appendAttempts == 2); + + using morph::math::DecimalPlaces; + using morph::math::Denominator; + using morph::math::Numerator; + ledger::GetLedgerResult stored; + REQUIRE_NOTHROW( + stored = model.execute(ledger::StoreTransaction{ + .ledgerId = ledgerId, + .description = "Weekly shop", + .date = morph::time::Timestamp::now(), + .legs = {ledger::TransactionLeg{ + .accountId = checking.id, + .amount = morph::math::Rational{Numerator{-5000}, Denominator{1}, DecimalPlaces{2}}}, + ledger::TransactionLeg{ + .accountId = groceries.id, + .amount = morph::math::Rational{Numerator{5000}, Denominator{1}, DecimalPlaces{2}}}}})); + CHECK(log->appendAttempts == 3); + CHECK(stored.accounts.size() == 2); + + // Read back through a model with no log attached: the write is durable, + // and matches what the caller was told. + ledger::LedgerModel reader; + const auto state = reader.execute(ledger::GetLedger{.ledgerId = ledgerId}); + const auto balanceOf = [&](ledger::AccountId id) { + const auto found = std::ranges::find_if(state.accounts, [&](const auto& a) { return a.id == id; }); + REQUIRE(found != state.accounts.end()); + return found->balance.numerator; + }; + CHECK(balanceOf(checking.id) == -5000); + CHECK(balanceOf(groceries.id) == 5000); +} diff --git a/include/morph/core/model.hpp b/include/morph/core/model.hpp index b39d324d4..a541af225 100644 --- a/include/morph/core/model.hpp +++ b/include/morph/core/model.hpp @@ -4,10 +4,15 @@ #include #include #include +#include +#include #include #include +#include +#include #include #include +#include // strand.hpp defines no symbol this header uses. It is kept deliberately: // consumers (tests/test_model.cpp among them) reach @@ -18,8 +23,75 @@ #include "../journal/action_log.hpp" #include "../session/session.hpp" #include "detail/task_handler.hpp" +#include "logger.hpp" #include "strand.hpp" +namespace morph::model { + +// ── Post-commit tail ────────────────────────────────────────────────────────── + +/// @brief Runs @p tail, an action handler's work *after* its commit, and +/// contains any exception it throws. +/// +/// Once a handler has committed, its mutation is durable. What it does next -- +/// journalling, a rule cascade, rebuilding cached state -- can still fail, and +/// if that failure leaves `execute()` the caller is told the action failed +/// while the write it made stands. This is the seam that keeps the two apart: +/// the tail's exception is logged through `morph::log::logError` and goes no +/// further. Only work that follows the commit belongs in @p tail; an exception +/// from before it still means the mutation did not happen and must still reach +/// the caller. +/// +/// Anything the caller's return value depends on is best computed before the +/// commit, where a failure rolls the write back. A handler that cannot do that +/// uses the value-returning overload instead. +/// +/// @tparam Tail Nullary callable returning `void`. +/// @param tail The post-commit work to run. +/// @param what Names the handler in the log line, e.g. `"[kanban::BoardModel] CreateColumn"`. +template + requires std::invocable && std::is_void_v> +void runPostCommitTail(Tail&& tail, std::string_view what) noexcept { + try { + std::invoke(std::forward(tail)); + } catch (const std::exception& error) { + ::morph::log::logError("{} committed, but its post-commit tail failed: {}", what, error.what()); + } catch (...) { + ::morph::log::logError("{} committed, but its post-commit tail threw a non-std::exception", what); + } +} + +/// @brief The same containment for a tail that produces the handler's result, +/// falling back to the state the handler committed if the tail throws. +/// +/// For a handler whose return value is refreshed by its post-commit work -- +/// a state rebuilt after a rule cascade, say -- and which already holds a +/// truthful answer from before the tail began. If the tail throws, that answer +/// is returned instead: it describes what the commit made durable, which is +/// the one thing the caller must not be misinformed about. +/// +/// @tparam Result The handler's result type; deduced from @p committed. +/// @tparam Tail Nullary callable whose result converts to @p Result. +/// @param tail The post-commit work to run. +/// @param committed The result to return if @p tail throws. +/// @param what Names the handler in the log line. +/// @return @p tail's result, or @p committed if it threw. +template + requires std::invocable && std::convertible_to, Result> +[[nodiscard]] Result runPostCommitTail(Tail&& tail, Result committed, + std::string_view what) noexcept(std::is_nothrow_move_constructible_v) { + try { + return std::invoke(std::forward(tail)); + } catch (const std::exception& error) { + ::morph::log::logError("{} committed, but its post-commit tail failed: {}", what, error.what()); + } catch (...) { + ::morph::log::logError("{} committed, but its post-commit tail threw a non-std::exception", what); + } + return committed; +} + +} // namespace morph::model + namespace morph::model::detail { template diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index cbe7e0020..c66d7d36a 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -31,6 +31,7 @@ add_executable(morph_tests test_coroutine_model.cpp test_async_delay.cpp test_model.cpp + test_post_commit_tail.cpp test_logger.cpp test_observability.cpp test_backend_extra.cpp diff --git a/tests/test_post_commit_tail.cpp b/tests/test_post_commit_tail.cpp new file mode 100644 index 000000000..78cddc6e5 --- /dev/null +++ b/tests/test_post_commit_tail.cpp @@ -0,0 +1,229 @@ +// SPDX-License-Identifier: Apache-2.0 +// +// Tests for `morph::model::runPostCommitTail`: the seam between a handler's +// commit and the work that follows it. +// +// Each case that asserts containment also asserts the tail really ran and +// really threw, so none of them can pass for the uninteresting reason that +// nothing failed. The model-shaped cases at the end drive the helper the way a +// handler uses it -- commit, then journal to a sink that refuses -- and check +// the caller is told the truth about the committed write. + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +namespace { + +using morph::log::LogLevel; +using morph::model::runPostCommitTail; + +/// @brief Collects every log line emitted while it is alive. +class CapturedLog { +public: + CapturedLog() + : _guard{ + [this](LogLevel level, std::string_view message) { lines.emplace_back(level, std::string{message}); }} {} + + std::vector> lines; + +private: + morph::log::ScopedLoggerOverride _guard; +}; + +struct NotAStdException {}; + +/// @brief A sink whose `append()` throws, as `IActionLog::append`'s contract +/// requires of a sink that could not record the entry. +class RefusingActionLog : public morph::journal::IActionLog { +public: + void append(morph::journal::LogEntry /*entry*/) override { + ++appendAttempts; + throw std::runtime_error{"journal sink unavailable"}; + } + + void flush() override {} + + [[nodiscard]] std::vector entries(std::string_view /*entityKey*/ = {}) const override { + return {}; + } + + int appendAttempts = 0; +}; + +/// @brief The handler shape the helper exists for: validate, mutate, commit, +/// then journal. +struct CounterModel { + std::shared_ptr log; + int committed = 0; + int cached = 0; + + void journal() const { + if (log) { + log->append(morph::journal::LogEntry{}); + } + } + + /// @brief Commits an increment, then journals it after the commit. + int increment(int by) { + if (by <= 0) { + throw std::invalid_argument{"increment must be positive"}; // before the commit + } + committed += by; // the commit + runPostCommitTail([&] { journal(); }, "CounterModel::increment"); + return committed; + } + + /// @brief Commits an increment, then refreshes a cached view of it after + /// the commit; the refreshed view is the result. + int incrementAndRefresh(int by) { + committed += by; // the commit + const int atCommit = committed; + return runPostCommitTail( + [&] { + journal(); + cached = committed * 10; + return cached; + }, + atCommit, "CounterModel::incrementAndRefresh"); + } +}; + +} // namespace + +// ── void overload ───────────────────────────────────────────────────────────── + +TEST_CASE("runPostCommitTail runs a tail that succeeds and logs nothing", "[model][post_commit_tail]") { + const CapturedLog captured; + int runs = 0; + runPostCommitTail([&] { ++runs; }, "Handler"); + CHECK(runs == 1); + CHECK(captured.lines.empty()); +} + +TEST_CASE("runPostCommitTail contains a std::exception and logs it as an error", "[model][post_commit_tail]") { + const CapturedLog captured; + bool reached = false; + REQUIRE_NOTHROW(runPostCommitTail( + [&] { + reached = true; + throw std::runtime_error{"disk full"}; + }, + "[demo::Model] Create")); + CHECK(reached); + REQUIRE(captured.lines.size() == 1); + CHECK(captured.lines.front().first == LogLevel::error); + CHECK(captured.lines.front().second == + "[demo::Model] Create committed, but its post-commit tail failed: disk full"); +} + +TEST_CASE("runPostCommitTail contains an exception of any type", "[model][post_commit_tail]") { + const CapturedLog captured; + bool reached = false; + REQUIRE_NOTHROW(runPostCommitTail( + [&] { + reached = true; + throw NotAStdException{}; + }, + "Handler")); + CHECK(reached); + REQUIRE(captured.lines.size() == 1); + CHECK(captured.lines.front().first == LogLevel::error); + CHECK(captured.lines.front().second == "Handler committed, but its post-commit tail threw a non-std::exception"); +} + +// ── value-returning overload ────────────────────────────────────────────────── + +TEST_CASE("runPostCommitTail returns the tail's result when the tail succeeds", "[model][post_commit_tail]") { + const CapturedLog captured; + const std::string result = + runPostCommitTail([] { return std::string{"refreshed"}; }, std::string{"committed"}, "Handler"); + CHECK(result == "refreshed"); + CHECK(captured.lines.empty()); +} + +TEST_CASE("runPostCommitTail returns the committed result when the tail throws", "[model][post_commit_tail]") { + const CapturedLog captured; + bool reached = false; + const std::string result = runPostCommitTail( + [&]() -> std::string { + reached = true; + throw std::runtime_error{"re-read failed"}; + }, + std::string{"committed"}, "Handler"); + CHECK(reached); + CHECK(result == "committed"); + REQUIRE(captured.lines.size() == 1); + CHECK(captured.lines.front().second == "Handler committed, but its post-commit tail failed: re-read failed"); +} + +TEST_CASE("runPostCommitTail converts the tail's result to the committed result's type", "[model][post_commit_tail]") { + // The tail yields a `const char*`; `Result` is deduced from `committed` + // alone, so the handler's own result type is what the caller receives. + const auto result = runPostCommitTail([] { return "refreshed"; }, std::string{"committed"}, "Handler"); + STATIC_REQUIRE(std::is_same_v, std::string>); + CHECK(result == "refreshed"); +} + +TEST_CASE("runPostCommitTail never throws on the caller's path", "[model][post_commit_tail]") { + // Tails that could throw, so the answer is the helper's own guarantee + // rather than a property of the tail. `what` is a `string_view` already: + // the conversion from a string literal is outside the helper's promise. + auto voidTail = [] { throw std::runtime_error{"tail"}; }; + auto intTail = []() -> int { throw std::runtime_error{"tail"}; }; + constexpr std::string_view what{"Handler"}; + STATIC_REQUIRE(noexcept(runPostCommitTail(voidTail, what))); + STATIC_REQUIRE(noexcept(runPostCommitTail(intTail, 0, what))); +} + +// ── the handler shape ───────────────────────────────────────────────────────── + +TEST_CASE("A handler whose journal refuses after the commit reports the committed write", + "[model][post_commit_tail]") { + const CapturedLog captured; + CounterModel model; + auto log = std::make_shared(); + model.log = log; + + int returned = 0; + REQUIRE_NOTHROW(returned = model.increment(3)); + + CHECK(log->appendAttempts == 1); // the tail really did fail + CHECK(returned == 3); + CHECK(model.committed == 3); + REQUIRE(captured.lines.size() == 1); + CHECK(captured.lines.front().second == + "CounterModel::increment committed, but its post-commit tail failed: journal sink unavailable"); +} + +TEST_CASE("A handler's failure before the commit still reaches the caller", "[model][post_commit_tail]") { + CounterModel model; + model.log = std::make_shared(); + CHECK_THROWS_AS(model.increment(0), std::invalid_argument); + CHECK(model.committed == 0); +} + +TEST_CASE("A value-returning handler falls back to its committed state when the tail fails", + "[model][post_commit_tail]") { + const CapturedLog captured; + CounterModel model; + + CHECK(model.incrementAndRefresh(2) == 20); // no log: the tail's result + + auto log = std::make_shared(); + model.log = log; + int returned = 0; + REQUIRE_NOTHROW(returned = model.incrementAndRefresh(5)); + CHECK(log->appendAttempts == 1); + CHECK(returned == 7); // the state at commit, not a stale or partial refresh + CHECK(model.committed == 7); + CHECK(captured.lines.size() == 1); +} From 19d2f2662ec33e64967c7bd542a89946dfbad27c Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Fri, 2 Oct 2026 06:48:49 +0200 Subject: [PATCH 3/4] tidy: satisfy clang-tidy on the post-commit tail changes Co-Authored-By: Claude Sonnet 5.5 Claude-Session: https://claude.ai/code/session_01Aed6W1C9xpio4Hq6bBsHsG --- examples/ledger/src/models/ledger_model.cpp | 4 +++ .../tests/test_ledger_post_commit_tail.cpp | 4 +-- tests/test_post_commit_tail.cpp | 31 ++++++++++--------- 3 files changed, 22 insertions(+), 17 deletions(-) diff --git a/examples/ledger/src/models/ledger_model.cpp b/examples/ledger/src/models/ledger_model.cpp index a879cf1be..36f1412c8 100644 --- a/examples/ledger/src/models/ledger_model.cpp +++ b/examples/ledger/src/models/ledger_model.cpp @@ -842,6 +842,10 @@ ListTransactionsResult LedgerModel::execute(const ListTransactions& action) { return result; } +// One handler runs the whole transaction, its trigger cascade and the post-commit +// journalling; the branching is the transaction's, and splitting it would scatter +// one atomic unit across helpers. +// NOLINTNEXTLINE(readability-function-cognitive-complexity) GetLedgerResult LedgerModel::execute(const StoreTransaction& action) { try { const auto* ctx = morph::session::current(); diff --git a/examples/ledger/tests/test_ledger_post_commit_tail.cpp b/examples/ledger/tests/test_ledger_post_commit_tail.cpp index ebf405550..466fcb783 100644 --- a/examples/ledger/tests/test_ledger_post_commit_tail.cpp +++ b/examples/ledger/tests/test_ledger_post_commit_tail.cpp @@ -108,8 +108,8 @@ TEST_CASE("OpenAccount and StoreTransaction report success when only their journ // and matches what the caller was told. ledger::LedgerModel reader; const auto state = reader.execute(ledger::GetLedger{.ledgerId = ledgerId}); - const auto balanceOf = [&](ledger::AccountId id) { - const auto found = std::ranges::find_if(state.accounts, [&](const auto& a) { return a.id == id; }); + const auto balanceOf = [&](ledger::AccountId accountId) { + const auto found = std::ranges::find_if(state.accounts, [&](const auto& account) { return account.id == accountId; }); REQUIRE(found != state.accounts.end()); return found->balance.numerator; }; diff --git a/tests/test_post_commit_tail.cpp b/tests/test_post_commit_tail.cpp index 78cddc6e5..168e1f710 100644 --- a/tests/test_post_commit_tail.cpp +++ b/tests/test_post_commit_tail.cpp @@ -31,11 +31,12 @@ class CapturedLog { public: CapturedLog() : _guard{ - [this](LogLevel level, std::string_view message) { lines.emplace_back(level, std::string{message}); }} {} + [this](LogLevel level, std::string_view message) { _lines.emplace_back(level, std::string{message}); }} {} - std::vector> lines; + [[nodiscard]] const std::vector>& lines() const noexcept { return _lines; } private: + std::vector> _lines; morph::log::ScopedLoggerOverride _guard; }; @@ -106,7 +107,7 @@ TEST_CASE("runPostCommitTail runs a tail that succeeds and logs nothing", "[mode int runs = 0; runPostCommitTail([&] { ++runs; }, "Handler"); CHECK(runs == 1); - CHECK(captured.lines.empty()); + CHECK(captured.lines().empty()); } TEST_CASE("runPostCommitTail contains a std::exception and logs it as an error", "[model][post_commit_tail]") { @@ -119,9 +120,9 @@ TEST_CASE("runPostCommitTail contains a std::exception and logs it as an error", }, "[demo::Model] Create")); CHECK(reached); - REQUIRE(captured.lines.size() == 1); - CHECK(captured.lines.front().first == LogLevel::error); - CHECK(captured.lines.front().second == + REQUIRE(captured.lines().size() == 1); + CHECK(captured.lines().front().first == LogLevel::error); + CHECK(captured.lines().front().second == "[demo::Model] Create committed, but its post-commit tail failed: disk full"); } @@ -135,9 +136,9 @@ TEST_CASE("runPostCommitTail contains an exception of any type", "[model][post_c }, "Handler")); CHECK(reached); - REQUIRE(captured.lines.size() == 1); - CHECK(captured.lines.front().first == LogLevel::error); - CHECK(captured.lines.front().second == "Handler committed, but its post-commit tail threw a non-std::exception"); + REQUIRE(captured.lines().size() == 1); + CHECK(captured.lines().front().first == LogLevel::error); + CHECK(captured.lines().front().second == "Handler committed, but its post-commit tail threw a non-std::exception"); } // ── value-returning overload ────────────────────────────────────────────────── @@ -147,7 +148,7 @@ TEST_CASE("runPostCommitTail returns the tail's result when the tail succeeds", const std::string result = runPostCommitTail([] { return std::string{"refreshed"}; }, std::string{"committed"}, "Handler"); CHECK(result == "refreshed"); - CHECK(captured.lines.empty()); + CHECK(captured.lines().empty()); } TEST_CASE("runPostCommitTail returns the committed result when the tail throws", "[model][post_commit_tail]") { @@ -161,8 +162,8 @@ TEST_CASE("runPostCommitTail returns the committed result when the tail throws", std::string{"committed"}, "Handler"); CHECK(reached); CHECK(result == "committed"); - REQUIRE(captured.lines.size() == 1); - CHECK(captured.lines.front().second == "Handler committed, but its post-commit tail failed: re-read failed"); + REQUIRE(captured.lines().size() == 1); + CHECK(captured.lines().front().second == "Handler committed, but its post-commit tail failed: re-read failed"); } TEST_CASE("runPostCommitTail converts the tail's result to the committed result's type", "[model][post_commit_tail]") { @@ -199,8 +200,8 @@ TEST_CASE("A handler whose journal refuses after the commit reports the committe CHECK(log->appendAttempts == 1); // the tail really did fail CHECK(returned == 3); CHECK(model.committed == 3); - REQUIRE(captured.lines.size() == 1); - CHECK(captured.lines.front().second == + REQUIRE(captured.lines().size() == 1); + CHECK(captured.lines().front().second == "CounterModel::increment committed, but its post-commit tail failed: journal sink unavailable"); } @@ -225,5 +226,5 @@ TEST_CASE("A value-returning handler falls back to its committed state when the CHECK(log->appendAttempts == 1); CHECK(returned == 7); // the state at commit, not a stale or partial refresh CHECK(model.committed == 7); - CHECK(captured.lines.size() == 1); + CHECK(captured.lines().size() == 1); } From 895885a6ac6efff38df2bd675584b798de8d0bd8 Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Fri, 2 Oct 2026 08:36:03 +0200 Subject: [PATCH 4/4] format: clang-format the tidy fixes Co-Authored-By: Claude Sonnet 5.5 Claude-Session: https://claude.ai/code/session_01Aed6W1C9xpio4Hq6bBsHsG --- examples/ledger/tests/test_ledger_post_commit_tail.cpp | 3 ++- tests/test_post_commit_tail.cpp | 5 +++-- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/examples/ledger/tests/test_ledger_post_commit_tail.cpp b/examples/ledger/tests/test_ledger_post_commit_tail.cpp index 466fcb783..fc6c6ceec 100644 --- a/examples/ledger/tests/test_ledger_post_commit_tail.cpp +++ b/examples/ledger/tests/test_ledger_post_commit_tail.cpp @@ -109,7 +109,8 @@ TEST_CASE("OpenAccount and StoreTransaction report success when only their journ ledger::LedgerModel reader; const auto state = reader.execute(ledger::GetLedger{.ledgerId = ledgerId}); const auto balanceOf = [&](ledger::AccountId accountId) { - const auto found = std::ranges::find_if(state.accounts, [&](const auto& account) { return account.id == accountId; }); + const auto found = + std::ranges::find_if(state.accounts, [&](const auto& account) { return account.id == accountId; }); REQUIRE(found != state.accounts.end()); return found->balance.numerator; }; diff --git a/tests/test_post_commit_tail.cpp b/tests/test_post_commit_tail.cpp index 168e1f710..e55d8f21a 100644 --- a/tests/test_post_commit_tail.cpp +++ b/tests/test_post_commit_tail.cpp @@ -30,8 +30,9 @@ using morph::model::runPostCommitTail; class CapturedLog { public: CapturedLog() - : _guard{ - [this](LogLevel level, std::string_view message) { _lines.emplace_back(level, std::string{message}); }} {} + : _guard{[this](LogLevel level, std::string_view message) { + _lines.emplace_back(level, std::string{message}); + }} {} [[nodiscard]] const std::vector>& lines() const noexcept { return _lines; }