journal: record failed action attempts, not only successes - #33
Open
Yaraslaut wants to merge 2 commits into
Open
journal: record failed action attempts, not only successes#33Yaraslaut wants to merge 2 commits into
Yaraslaut wants to merge 2 commits into
Conversation
Both real Model::execute() call sites -- ActionDispatcher::registerAction's runner (registry.hpp, server/remote topologies) and Bridge::executeVia's localOp (bridge.hpp, LocalBackend) -- appended a journal LogEntry only after a successful execution. An action that threw (a validation failure, a lost connection, a rejected write) left no entry at all, so an audit trail could not answer "who tried X and was refused" -- it just looked quiet. Wrap both call sites in try/catch: on success, record outcome=Succeeded with the JSON result (unchanged from before); on a caught std::exception, record outcome=Failed with the exception's what() and no result, then rethrow unchanged -- callers see no behavior change, only the journal gains entries it previously lacked. Add LogEntry::outcome (a new Outcome enum: Succeeded/Failed, defaulting to Succeeded so pre-existing on-disk entries decode unchanged) and LogEntry::error (empty unless Failed). Outcome gets its own glz::meta so it serialises as the readable string "Succeeded"/"Failed" rather than a bare 0/1, keeping the NDJSON audit trail human-readable. Since a Failed entry can now appear in the same log stream replay()/ undoLast() read, and a failed attempt never mutated model state, journal::replay() now skips Outcome::Failed entries before dispatching -- otherwise reconstruction would re-run the rejected action and very likely hit the same exception again, aborting the reconstruction. Closes #23 Signed-off-by: Yaraslau Tamashevich <yaraslau.tamashevich@gmail.com>
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Windows cl-debug/cl-release failed under /W4 /WX: the catch parameter `exc` in ActionDispatcher::registerAction's runner (registry.hpp) and Bridge::executeVia's localOp (bridge.hpp) is only referenced inside an `if constexpr (... == Loggable::Yes)` branch. MSVC's C4101 fires when that branch is compiled out (e.g. lab::ListSamples, a Loggable::No action), because the discarded branch's use of exc.what() is never instantiated. GCC/Clang don't warn on this. Mark both parameters [[maybe_unused]]. Also documents the try/catch outcome-recording behavior in docs/spec/core/bridge.md and registry.md (the header <-> spec sync CI gate flagged include/morph/core/*.hpp changing without a matching docs/spec/core/** update). Signed-off-by: Yaraslau Tamashevich <yaraslau.tamashevich@gmail.com>
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
1 participant
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.
Summary
Model::execute()call sites (ActionDispatcher::registerAction's runner for remote/Qt topologies, andBridge::executeVia'slocalOpforLocalBackend) only appended a journalLogEntryafter a successful execution. A thrown action (validation failure, lost connection, rejected write) left the audit trail silent.try/catch (const std::exception&): success recordsoutcome=Succeededwith the result (unchanged), failure recordsoutcome=Failedwitherror=what()and emptyresult, then rethrows unchanged — no behavior change for callers, only the journal gains entries it previously lacked.LogEntry::outcome(newOutcomeenum:Succeeded/Failed, defaulting toSucceededso existing on-disk entries decode unchanged) andLogEntry::error.Outcomegets aglz::metaso it serialises as the readable string, not a bare0/1.Failedentry can now appear in the same log streamreplay()/undoLast()read, and a failed attempt never mutated model state,journal::replay()now skipsOutcome::Failedentries before dispatching — otherwise reconstruction would re-run the rejected action and very likely throw the same exception again, aborting reconstruction. Covered by a dedicated regression test.docs/spec/journal/journal.md) updated: field table, API reference, and thereplay()/Invariants sections that previously (correctly, at the time) stated "only successful actions are recorded."Test plan
tests/test_action_log.cpp: new tests for both execution sites recordingoutcome=Failed/erroron a thrown action and still rethrowing; existing success-path tests extended to assertoutcome=Succeeded; newreplay()test proving aFailedentry is skipped (not re-dispatched, no throw, correct reconstructed state)../build/tests/morph_tests— all 814 test cases / 8304 assertions pass.Closes #23
🤖 Generated with Claude Code