From 3003708818d15c80052324bd634b2c9cfd89268d Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Sun, 16 Aug 2026 12:34:36 +0300 Subject: [PATCH] journal: OutboxRelay::relay() throws NullSinkError instead of crashing on a null sink (morph#95) A null sink reaching relay()'s sink-using path (drainOutbox() reporting at least one row) used to be a null-pointer virtual dispatch through sink->append() -- real UB, confirmed as a process crash, not a catchable exception. A null drainOutbox/markRelayed already throws a catchable std::bad_function_call; this makes sink consistent with that instead of the odd one out. relay() now checks sink for null right after logIfAnyDepNull() (which already logged the null-sink warning) and before any row ever reaches sink->append(), throwing the new NullSinkError (a std::runtime_error, matching this codebase's established Error convention -- SerializationError, FileOfflineQueueError, ParseError, etc.). An empty drainOutbox() with a null sink is still a no-op, unchanged: nothing would touch sink either way. This is a deliberate, documented behavior change (per the issue's own explicit caution against silently patching this) -- docs/spec/journal/ journal.md's OutboxRelay contract is updated to match. The one previously-untestable branch (a null sink actually reaching sink->append) now has a real test instead of a comment explaining why it couldn't be tested. Full suite: 1047 test cases, 10040 assertions, all passing. Doxygen doc build (WARN_AS_ERROR=FAIL_ON_WARNINGS) passes clean for the new NullSinkError type. Co-Authored-By: Claude Sonnet 5 --- docs/spec/journal/journal.md | 13 ++++++++-- include/morph/journal/outbox.hpp | 31 ++++++++++++++++++----- tests/test_outbox.cpp | 43 +++++++++++++++++++------------- 3 files changed, 62 insertions(+), 25 deletions(-) diff --git a/docs/spec/journal/journal.md b/docs/spec/journal/journal.md index 64fe1559..8823eea2 100644 --- a/docs/spec/journal/journal.md +++ b/docs/spec/journal/journal.md @@ -633,6 +633,10 @@ sink gives no re-relay protection. ### `journal::OutboxRelay` ```cpp +struct NullSinkError : std::runtime_error { + using std::runtime_error::runtime_error; +}; + struct OutboxRelayResult { std::size_t relayed = 0; }; @@ -670,8 +674,13 @@ guarantee comes from the host's own transaction, not from `OutboxRelay`. Mirroring `ReconnectCoordinator::Deps`, a null `drainOutbox`/`markRelayed`/`sink` is logged (via `morph::log::logError`) at the start of every `relay()` call but -does not reject the call — invoking a null member still throws -(`std::bad_function_call`) or crashes as usual. +does not reject the call by itself — invoking a null `drainOutbox`/`markRelayed` +still throws `std::bad_function_call` as usual (a null `std::function` call). A +null `sink` throws `NullSinkError`, a catchable `std::runtime_error`, once +`drainOutbox()` reports at least one row to relay — thrown before `sink` is +ever dereferenced, so no row is lost or marked relayed. `sink` being null is +not itself rejected when there is nothing to relay: an empty outbox is still a +no-op regardless of `sink`, exactly as it is when `sink` is real. ### What this does not do diff --git a/include/morph/journal/outbox.hpp b/include/morph/journal/outbox.hpp index 6197ba19..c6f5a8db 100644 --- a/include/morph/journal/outbox.hpp +++ b/include/morph/journal/outbox.hpp @@ -5,6 +5,7 @@ #include #include #include +#include #include #include "../core/logger.hpp" @@ -12,6 +13,17 @@ namespace morph::journal { +/// @brief Thrown by `OutboxRelay::relay()` when `sink` is null. +/// +/// A null `drainOutbox`/`markRelayed` already throws a catchable +/// `std::bad_function_call` (invoking a null `std::function`); this makes a +/// null `sink` consistent with that instead of a raw null-`shared_ptr` +/// dereference (real undefined behavior, not portably catchable — see +/// `LASTRADA-Software/morph#95`). +struct NullSinkError : std::runtime_error { + using std::runtime_error::runtime_error; +}; + /// @brief Outcome of one `OutboxRelay::relay()` call. struct OutboxRelayResult { /// @brief Outbox rows drained, appended to `OutboxRelay::sink`, and marked @@ -82,6 +94,11 @@ struct OutboxRelay { /// nothing reached the sink, and nothing would ever surface the loss. /// /// @return The number of rows relayed in this call. + /// @throws NullSinkError if `sink` is null and `drainOutbox()` reports at + /// least one row — thrown before `sink` is ever dereferenced, so + /// no row is lost or marked relayed. A null `sink` with nothing + /// to relay is still a no-op, exactly like an empty outbox with a + /// real `sink`: nothing would touch `sink` either way. /// @throws std::exception propagated from `sink->append()` or `sink->flush()`; /// the batch is left unmarked and therefore retryable. OutboxRelayResult relay() { @@ -90,6 +107,9 @@ struct OutboxRelay { if (rows.empty()) { return {}; } + if (!sink) { + throw NullSinkError("journal::OutboxRelay::relay: sink is null"); + } for (const auto& row : rows) { sink->append(row); } @@ -114,12 +134,11 @@ struct OutboxRelay { ::morph::log::logError("[journal::OutboxRelay] null sink"); } // This branch itself is unit-tested (test_outbox.cpp asserts the - // warning fires). The crash relay() goes on to hit afterwards -- - // sink->append() dispatching through a null shared_ptr -- is real UB - // and not exercised: no seam here turns it into something a portable - // unit test can catch instead of taking down the process (see - // LASTRADA-Software/morph#95, requesting either a catchable exception - // for this case or an observability hook logIfAnyDepNull() could feed). + // warning fires). relay() itself throws NullSinkError right after + // this call if sink is null and there is at least one row to relay + // (see LASTRADA-Software/morph#95) -- a null drainOutbox/markRelayed + // still throws std::bad_function_call as usual, invoking a null + // std::function. } }; diff --git a/tests/test_outbox.cpp b/tests/test_outbox.cpp index 833bd4ed..9bf310f0 100644 --- a/tests/test_outbox.cpp +++ b/tests/test_outbox.cpp @@ -390,23 +390,32 @@ TEST_CASE("OutboxRelay::relay(): a null sink is logged, not rejected, at call ti REQUIRE(sawWarning); } -// A null `sink` reaching relay()'s *sink-using* path (i.e. with drainOutbox() -// returning rows) is deliberately NOT exercised here: unlike -// `drainOutbox`/`markRelayed` (null std::function -> catchable -// std::bad_function_call), `sink` is a std::shared_ptr, so -// `sink->append(row)` on a null sink is a null-pointer virtual dispatch -- -// real UB that crashes the process (confirmed: SIGSEGV under this build), not -// a C++ exception a REQUIRE_THROWS_AS could observe. This is the documented -// contract (see outbox.hpp's class doc and docs/spec/journal/journal.md: -// "invoking a null member still throws (std::bad_function_call) or crashes -// as usual"), not an oversight -- forcing it into a portable, deterministic -// unit test would need a signal/guard-page harness like -// test_bridge_lifetime.cpp's POSIX-only hasSubscribers() case, which is far -// more machinery than this one branch warrants and still would not run on -// Windows/MSVC, where this suite also builds. The logIfAnyDepNull() call's -// null-sink warning line itself is now covered by the empty-drainOutbox test -// above -- only the subsequent crash on a non-empty drain is left -// unexercised. Tracked as LASTRADA-Software/morph#95. +TEST_CASE("OutboxRelay::relay(): a null sink reaching a non-empty drain throws NullSinkError, not UB", + "[outbox][relay]") { + // Unlike the empty-drainOutbox case above (which stays on relay()'s + // early-return path and never reaches sink at all), this drains a real + // row with sink still null -- morph#95's actual gap: sink->append(row) + // on a null shared_ptr used to be a null-pointer virtual + // dispatch (real UB, a process crash, not a catchable exception). relay() + // now throws NullSinkError before ever dereferencing sink. + std::vector logged; + morph::log::ScopedLoggerOverride guard{ + [&](morph::log::LogLevel, std::string_view msg) { logged.emplace_back(msg); }, + morph::log::LogLevel::debug, + }; + + OutboxRelay relay; + relay.drainOutbox = [] { return std::vector{makeEntry("OB_Model", "acct-1", "OB_Deposit", "row-1")}; }; + relay.markRelayed = [](std::span) {}; + // relay.sink left null on purpose. + + REQUIRE_THROWS_AS(relay.relay(), morph::journal::NullSinkError); + + bool sawWarning = std::any_of(logged.begin(), logged.end(), [](const std::string& line) { + return line.find("null sink") != std::string::npos; + }); + REQUIRE(sawWarning); +} TEST_CASE("OutboxRelay + FileActionLog: re-relay after a simulated process restart dedups via the sink", "[outbox][relay][file_action_log]") {