ledger: give rung 5 an App layer and move the report job's scheduling out of the model - #243
Open
Yaraslaut wants to merge 2 commits into
Open
ledger: give rung 5 an App layer and move the report job's scheduling out of the model#243Yaraslaut wants to merge 2 commits into
Yaraslaut wants to merge 2 commits into
Conversation
… out of the model `ledger::LedgerModel` owned a one-thread `ThreadPoolExecutor` and `execute(SubmitReport)` posted the report aggregation to it, making it the only ladder model that included `<morph/core/executor.hpp>` -- a model that had become framework-aware in order to schedule work. The cause was a missing layer, not a bad call: rung 5 had no `src/app/`, so the job had nowhere else to go (closes #160). Rung 5 now has one. `ledger::app::App` owns the worker pool and a periodic report runner that sweeps `ledger_report_jobs` for `Pending` rows and dispatches `RunReportJob` back at the model through an internal client bridge carrying a service principal -- the same shape `bookmarks::app::App`'s metadata worker already had. The aggregation itself did NOT move to the App. A monthly statement is business logic, and `examples/IMPLEMENTATION.md` rule 1 puts business logic in models; only the decision of when it runs is orchestration. So `execute(SubmitReport)` writes a `Pending` row and returns, and the new `execute(RunReportJob)` holds the computation the posted lambda used to. Consequences worth naming: * The run happens on the strand for its own ledger, so a report and a concurrent `StoreTransaction` against the same book serialise instead of racing. The WAL read snapshot stays: `BudgetModel` writes from a strand of its own. * A job is now a row, not a queued lambda, so it survives the process that accepted it -- a runner starting later picks it up. `SubmitReport::params` is persisted (new nullable `params_json` column) because the row is the only record of the request a later runner has. * `RunReportJob` is idempotent: an already-terminal job is left untouched and its status returned, which is what makes the runner's re-dispatch of a still-outstanding job harmless. * The App owns no `RemoteServer`. `RemoteServer` clears the session principal for any authorizer that does not authenticate, and this rung installs no authorizer, so every mutating action would arrive with an empty principal and be refused. The runner dispatches over a `LocalBackend` -- the backend this rung's shipped client already uses. Tests: `test_ledger_reports.cpp` loses its poll-and-sleep helper entirely -- the run is a synchronous action now, so there is nothing to wait for -- and gains cases for the Pending-until-run property, params round-tripping through the database, the principal gate, and re-run idempotency. New `test_app.cpp` covers the runner, including a job outliving the App that accepted it. `test_report_presenter.cpp` now runs a client and an App together, which is what the deployment actually is. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Added while raising this PR's patch coverage (65.69%, the lowest of the
eleven open PRs). The uncovered lines in src/app/app.cpp are its error arms
-- the `.then()` branch that logs a Failed result and the `.onError()`
branch with its `describe()` helper -- so the obvious test was a job whose
aggregation fails.
**It does not fail.** A job pointing at a ledger that no longer exists finds
no accounts, produces `[]`, and settles *Done*: measured `status=1 body=[]`.
That is indistinguishable from a successful report over a real ledger with no
activity, which for an auditable-records rung is the wrong default. Filed as
morph#250; this test asserts today's behaviour so it cannot change silently
while that is open, and deliberately pins the property that matters either
way -- the row reaches a *terminal* state, since a row left Pending is what a
poller spins on forever.
No test is added for the two error arms, because they are not reachable
through any existing seam and a test that executes a line without asserting
anything is worth less than the honest gap:
* a missing ledger yields an empty report, as above;
* malformed params_json does not throw -- decodeMonthlyParams returns
nullopt and the report falls back to all-time;
* DbFaultFixture contends on a *named* advisory lock and LedgerModel takes
none, so it cannot fault the aggregation.
Covering them needs a fault seam that does not exist, which is a design
decision rather than a missing test. Recorded in morph#250 alongside the
behaviour that led there.
Suite: 397 assertions in 80 cases, ladder_ledger_tests, clang -Weverything
-Werror.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #160.
Which of the two options, and why
Option 1: give ledger an App layer — with one correction to the issue's own suggested fix.
#160's step 2 says "the App layer picks up pending jobs, computes, and dispatches an action carrying the result". Taken literally that would put a monthly-statement aggregation in the App, which
examples/IMPLEMENTATION.mdrule 1 forbids: all business logic lives in models. The precedent the issue cites doesn't do that either —bookmarks::app::Appdoes only the non-domain half (fetch a URL) and dispatchesRecordMetadatafor the domain half.So the split here is: the App decides when, the model computes what.
execute(SubmitReport)writes aPendingrow carryingkindandparams, and returns. No executor, no thread.ledger_model.hppno longer includes<morph/core/executor.hpp>— it was the only ladder model that did.execute(RunReportJob)holds the aggregation the posted lambda used to, unchanged apart from where it gets its params.ledger::app::Appowns the worker pool and aQTimerthat sweepsledger_report_jobsforPendingrows and fire-and-forget dispatchesRunReportJobthrough an internal-clientBridgecarryingkReportRunnerPrincipal, with thestopBackgroundJobs()/reportsInFlight()settle seam bookmarks and pastebin both have.Option 2 ("something smaller") isn't actually available: the only smaller move is making the model's executor injectable, which is #222 and leaves the model owning it.
One thing that contradicts #160
The issue says "
bookmarks,pollsandkanbaneach have anApp…bookmarks::App::fetchMetadataOnce()is the working precedent, including its service-principal token minting for the re-entering dispatch." That precedent does not transfer, and the reason is worth reading before reviewingapp.hpp:RemoteServer::dispatchExecuteclears the session principal whenever the authorizer'sauthenticate()returnsnullopt— correct perdocs/spec/security.md, and the default forAllowAllAuthorizer. Rung 5 ships nosrc/auth/, noAuthModel, no token secret. So aRemoteServer-based App is not merely unauthenticated here; it is one on which no action can carry a principal, and every mutatingLedgerModelaction refuses an empty one.I hit this as a real failure, not by reading:
The runner therefore dispatches over a
LocalBackend— which is also exactly the backend this rung's shipped client uses (AppContext'sLocalmode) — andAppowns noRemoteServerand noserver()accessor. That is documented at length inapp.hpprather than left to be rediscovered. For the same reason this PR adds nosrc/server/main.cpp: an unauthenticated financial server is not a thing to ship. Rung 5's missing login/authorizer/server story is filed as #242, not folded in here.In-flight jobs at teardown
The most interesting consequence, and it is an improvement rather than a regression.
Before: a job was a lambda in a
ThreadPoolExecutorthe model owned.~ThreadPoolExecutordrains its queue before joining, so a job in flight when the model died did finish — but only inside that exact process. A process that was killed lost the job entirely, with nothing anywhere that could tell it had ever been submitted, and the row stayedPendingforever.After: the job is the row. A pass is "find the
Pendingrows, dispatch each". So:Pending, and the first pass of the next runner picks it up.test_app.cpp's "A job submitted before the App existed is picked up by its first pass" is exactly that scenario, with twoAppscopes standing in for two processes.SubmitReport::paramshad to become a persisted column (params_json, new nullableALTER TABLEmigration20260819000014): the params used to be decoded on the submitting thread and captured into the lambda, so they never needed to outlive it. A later runner has nothing but the row.Pending. What it is not safe for isApp's own completion callbacks, hencereportsInFlight()and the pastebin-style_reportExecutor-declared-first member ordering.app.hpp: a job whose aggregation crashes the process is retried on restart rather than abandoned. A job whose aggregation merely throws is recordedFailedand is not retried.There is no
src/server/main.cppin this rung to add a drain to, so thestopBackgroundJobs()-then-drain-then-destroy sequence is exercised bytest_app.cppand bytest_report_presenter.cppinstead. When rung 5 gets a server (#242), that main copies bookmarks'drainMetadataFetchesagainstreportsInFlight().Interaction with #222 — direct collision
#222 and this PR cannot both land as they stand. #222 adds
explicit LedgerModel(std::shared_ptr<IExecutor>)so a test can inject a deterministic executor for_reportExecutor. This PR deletes_reportExecutor, so that constructor has nothing left to inject and the<morph/core/executor.hpp>include it depends on is gone.Branched from
masteras instructed, not from #222. Concretely:examples/ledger/include/ledger/models/ledger_model.hpp/src/models/ledger_model.cpp— testkit: a worker-side executor double, and ledger's report tests driven by it #222 adds a constructor and its null check; this PR removes the member both exist for. Merging testkit: a worker-side executor double, and ledger's report tests driven by it #222 first leaves a constructor to delete here; merging this first leaves testkit: a worker-side executor double, and ledger's report tests driven by it #222 with nothing to add.examples/ledger/tests/test_ledger_reports.cpp— both rewrite it, toward different end states. testkit: a worker-side executor double, and ledger's report tests driven by it #222 keeps one deliberately-real-ThreadPoolExecutorcase and converts the rest toStepExecutor. Here there is no executor under the model at all: the run is a plain synchronous action, sopollUntilSettledis deleted outright — no sleep, no cap, no test double, andexamples/TESTING.md's ban onsleep_foroutsidepump.hppis satisfied by construction rather than by budget. testkit: a worker-side executor double, and ledger's report tests driven by it #222's careful justification for its one retained real-pool case ("the worker runs on a genuinely different thread, wheresession::current()is not the caller's") no longer describes anything: the worker is the caller's strand, and it has a session on purpose.examples/common/testkit/step_executor.hpp— testkit: a worker-side executor double, and ledger's report tests driven by it #222's other half, and it does not collide. It is a general testkit component, wanted independently (morph#161 names every ladder async-job test, not just ledger's). It just loses ledger as its first consumer.Suggested resolution, if the maintainer agrees: land
step_executor.hpp+ its owntest_step_executor.cpp+ theTESTING.mdentry from #222, and drop #222'sLedgerModelconstructor and itstest_ledger_reports.cppconversions. I have not touched #222's branch.Verification
Measured locally, on this branch, with
-DCMAKE_CXX_COMPILER=/opt/homebrew/opt/llvm/bin/clang++andQT_QPA_PLATFORM=offscreen:ladder_ledger_testsbuilds warning-free and passes: 392 assertions in 79 test cases.bash scripts/check_spec_citations.sh→Prose lint OK.Mutation check (
AGENTS.md, "ask whether the check would still pass if the feature did nothing"), two mutations, each built and run, each reverted:App::runPendingReportsOnce()returns immediately after its query, dispatching nothing → 7 failed (72 passed | 7 failed): all fivetest_app.cppcases, plusReportQmlBridge drives submit->poll to doneandThe App's report timer settles a job with no pass driven by hand.execute(RunReportJob)skips itsfinishReportJob(..., Done, ...)→ 12 failed (67 passed | 12 failed), spanning bothtest_app.cppandtest_ledger_reports.cpp(SubmitReport returns immediately…,Re-polling the same completed job…,Running an already-settled job recomputes nothing,SubmitReport stores its params on the job row…).Left to CI, not measured here: the
Application ladderjob's full-ladder build and every other rung's suite; the Docs (DoxygenFAIL_ON_WARNINGS) job over the new public symbols; sanitizer and coverage presets; every non-macOS platform. Only ledger's own targets were configured locally (-DMORPH_LADDER_RUNGS=ledger), deliberately — another agent was compiling on the same machine.Filed in passing, not folded in
RemoteServerwould clear one anyway. Pre-existing; it is what forced theLocalBackendchoice above.