journal: OutboxRelay throws NullSinkError instead of crashing on a null sink (closes morph#95)

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
 

include/morph/journal/outbox.hpp

@@ -5,13 +5,25 @@
 #include <functional>
 #include <memory>
 #include <span>
+#include <stdexcept>
 #include <vector>
 
 #include "../core/logger.hpp"
 #include "action_log.hpp"
 
 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.
     }
 };
 

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<IActionLog>, 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<IActionLog> 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<std::string> 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<LogEntry>{makeEntry("OB_Model", "acct-1", "OB_Deposit", "row-1")}; };
+    relay.markRelayed = [](std::span<const LogEntry>) {};
+    // 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]") {