From b0ded55cd6554bffa5745a3386eecd7589ea65ae Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Sat, 15 Aug 2026 22:52:33 +0300 Subject: [PATCH 1/3] feat(core): FileIoOps fault-injection seam; close FileActionLog's I/O gaps Adds morph::core::FileIoOps (include/morph/core/file_io_ops.hpp): the raw fwrite/fflush/fsync/fopen/file-open/resize_file calls FileActionLog and FileOfflineQueue both make, as an injectable strategy. Every member defaults to the real syscall it stands in for, so a default-constructed FileIoOps -- and every existing call site that never passes one -- is byte-for-byte what was called directly before this seam existed. No behavior change for any real caller. Wires it through FileActionLog (constructor takes an optional second FileIoOps parameter, default {}) and closes every branch LASTRADA-Software/morph#97 requested this exact seam for: - append()'s short-fwrite() throw - flush()'s failing fflush()/fsync() throws (and forgetting the unflushed idempotencyKeys on either) - rotate()'s pre-rotation failing fflush()/fsync() (nothing closed or renamed) and its failing reopen after a successful rename (leaves the log closed; requireOpen()'s throwing arm and the destructor's null check are both reachable from this one scenario) - repairTornTail()'s unreadable-path and failing resize_file() arms 8 new fault-injection tests in test_action_log_phase2.cpp, each overriding exactly one FileIoOps member and leaving the rest at their real defaults. Full morph_tests suite (1044 cases / 10034 assertions) passes; the whole-file behavior around each injected failure was verified to still work normally (a retry after clearing the injected failure actually succeeds and is durably recorded). docs/spec/journal/journal.md updated for the new constructor signature, per CLAUDE.md. --- docs/spec/journal/journal.md | 9 +- include/morph/core/file_io_ops.hpp | 99 +++++++++++ include/morph/journal/file_action_log.hpp | 82 ++------- tests/test_action_log_phase2.cpp | 197 ++++++++++++++++++++++ 4 files changed, 321 insertions(+), 66 deletions(-) create mode 100644 include/morph/core/file_io_ops.hpp diff --git a/docs/spec/journal/journal.md b/docs/spec/journal/journal.md index d80a2915..64fe1559 100644 --- a/docs/spec/journal/journal.md +++ b/docs/spec/journal/journal.md @@ -212,7 +212,12 @@ entry is one `toJson`-encoded line. `flush()` flushes the C stdio buffer and then issues a real `fsync` (POSIX `fsync` / Windows `_commit`), so a crash immediately after `flush()` returns cannot lose data. -Open (creating if necessary) via `FileActionLog(std::filesystem::path)`. +Open (creating if necessary) via `FileActionLog(std::filesystem::path, morph::core::FileIoOps = {})`. +The second parameter is a test-only fault-injection seam (`morph/core/ +file_io_ops.hpp`) — the raw `fwrite`/`fflush`/`fsync`/`fopen`/file-open/ +`resize_file` calls this class makes, as an injectable strategy defaulting to +the real syscalls, letting a test force the failure branches that otherwise +need a real OS-level I/O error to reach. A normal caller never passes one. Throws `std::runtime_error` if the file cannot be opened. Closes the file in the destructor. Copy and move are deleted. @@ -702,7 +707,7 @@ All symbols live in `namespace morph::journal`. |---|---|---| | `IActionLog` | abstract struct | `virtual ~IActionLog() = default`; `append(LogEntry)`, `flush()`, `entries(entityKey)`. | | `InMemoryActionLog` | class | `: IActionLog`. Thread-safe `std::vector`-backed. `flush()` no-op. | -| `FileActionLog` | class | `: IActionLog`. Newline-delimited JSON, fsync on `flush()`. `explicit FileActionLog(std::filesystem::path)`. `void rotate(const std::filesystem::path& sealedPath)` seals the active file and reopens a fresh one — see [Rotation and retention](#rotation-and-retention). Copy/move deleted. | +| `FileActionLog` | class | `: IActionLog`. Newline-delimited JSON, fsync on `flush()`. `explicit FileActionLog(std::filesystem::path, morph::core::FileIoOps = {})` — the `FileIoOps` is a test-only fault-injection seam, see above. `void rotate(const std::filesystem::path& sealedPath)` seals the active file and reopens a fresh one — see [Rotation and retention](#rotation-and-retention). Copy/move deleted. | | `SessionLog` | class | `: IActionLog`. Full-fidelity in-memory log + `undoLast()` + `checkpoint()`. | ### Process-wide default diff --git a/include/morph/core/file_io_ops.hpp b/include/morph/core/file_io_ops.hpp new file mode 100644 index 00000000..270e1d00 --- /dev/null +++ b/include/morph/core/file_io_ops.hpp @@ -0,0 +1,99 @@ +// SPDX-License-Identifier: Apache-2.0 + +#pragma once +#include +#include +#include +#include +#include +#include + +#ifdef _WIN32 +#include +#else +#include +#endif + +namespace morph::core { + +/// @brief The raw file-I/O primitives `morph::journal::FileActionLog` and +/// `morph::offline::FileOfflineQueue` both call, as an injectable +/// strategy. Every member defaults to the real syscall/stdlib call it +/// stands in for, so a default-constructed `FileIoOps` is byte-for-byte +/// what both classes called directly before this seam existed — no +/// behavior change for a normal caller. +/// +/// @par Why this exists +/// Both classes have several branch arms that only run when a real OS-level +/// file-I/O call fails partway through an otherwise-successful operation +/// (disk full, fd closed underneath, a permission change racing an exact +/// window). None of those are reachable from a portable unit test without +/// this seam — see `LASTRADA-Software/morph#97`, which requested exactly +/// this for `FileActionLog`; `FileOfflineQueue` has the identical gap. A +/// test constructs a `FileIoOps` whose relevant member fails on demand (or +/// on the Nth call, or forever) and passes it to the class under test; +/// every other member stays at its real default, so the rest of the class's +/// I/O behaves normally around the one injected failure. +/// +/// @par Thread safety +/// `FileIoOps` itself is a plain value type with no shared state — copying +/// or moving one has ordinary value semantics. Whether the *callbacks* +/// themselves are safe to call from multiple threads concurrently is up to +/// whatever a test installs; the real default callbacks are exactly the +/// real syscalls, which already have their own well-defined thread-safety. +struct FileIoOps { + /// @brief Writes @p size bytes from @p buffer to @p file. Mirrors `std::fwrite`. + /// @return The number of bytes actually written; short of @p size on failure. + std::function fwrite = + [](const void* buffer, std::size_t size, std::FILE* file) { + return std::fwrite(buffer, 1, size, file); + }; + + /// @brief Flushes @p file's stdio buffer. Mirrors `std::fflush`. + /// @return `0` on success, nonzero on failure. + std::function fflush = [](std::FILE* file) { return std::fflush(file); }; + + /// @brief Commits @p file's contents to durable storage. POSIX `fsync` / + /// Windows `_commit`, resolved from @p file via `fileno`/`_fileno`. + /// @return `0` on success, nonzero on failure. + std::function fsync = [](std::FILE* file) { +#ifdef _WIN32 + return _commit(_fileno(file)); +#else + return ::fsync(fileno(file)); +#endif + }; + + /// @brief Opens @p path in mode @p mode. Mirrors `std::fopen`. + /// @return The open file, or `nullptr` on failure. + // NOLINTNEXTLINE(cppcoreguidelines-owning-memory) — mirrors std::fopen's own raw-owning-pointer return + std::function fopen = + [](const std::string& path, const char* mode) { + // NOLINTNEXTLINE(cert-err33-c) — callers check the returned handle themselves + return std::fopen(path.c_str(), mode); + }; + + /// @brief Reports whether @p path can be opened for reading right now. + /// Stands in for `std::ifstream{path}`'s own open-succeeded check + /// (`repairTornTail()`'s `if (!input)`) — a fault-injection test has + /// no way to make a *real* `std::ifstream` construction fail + /// without actually breaking the filesystem, so this predicate is + /// consulted first; the real default performs the real open + /// `std::ifstream` itself would. + /// @return `true` if @p path is currently readable. + std::function canOpenForRead = + [](const std::filesystem::path& path) { return static_cast(std::ifstream{path}); }; + + /// @brief Truncates/extends @p path to @p newSize bytes. Mirrors + /// `std::filesystem::resize_file`. + /// @param path Path to resize. + /// @param newSize Target size, in bytes. + /// @param errorCode Set on failure, cleared on success — same contract as + /// `std::filesystem::resize_file`'s own `error_code` overload. + std::function + resizeFile = [](const std::filesystem::path& path, std::uintmax_t newSize, std::error_code& errorCode) { + std::filesystem::resize_file(path, newSize, errorCode); + }; +}; + +} // namespace morph::core diff --git a/include/morph/journal/file_action_log.hpp b/include/morph/journal/file_action_log.hpp index 3c88bad1..fdc1b5b9 100644 --- a/include/morph/journal/file_action_log.hpp +++ b/include/morph/journal/file_action_log.hpp @@ -12,6 +12,7 @@ #include #include +#include "../core/file_io_ops.hpp" #include "../core/logger.hpp" #include "action_log.hpp" @@ -57,11 +58,16 @@ class FileActionLog : public IActionLog { /// whatever is already on disk at @p path — an O(n) scan of the existing /// file's contents, paid once here, not on every `append()`. /// @param path File to append entries to. + /// @param ioOps Injectable file-I/O primitives; defaults to the real + /// syscalls. Test-only seam — see `morph::core::FileIoOps`'s own + /// docs — for forcing the failure branches that need a real + /// OS-level I/O error to reach. /// @throws std::runtime_error if the file cannot be opened. /// @throws SerializationError if an existing file at @p path has a malformed /// *interior* line (a malformed trailing line is tolerated — see /// `entries()`). - explicit FileActionLog(std::filesystem::path path) : _path{std::move(path)} { + explicit FileActionLog(std::filesystem::path path, ::morph::core::FileIoOps ioOps = {}) + : _path{std::move(path)}, _io{std::move(ioOps)} { // Discard a torn trailing record before anything else touches the file. // The file is opened "a", so the next append() would otherwise start // writing at the exact byte the truncated JSON stopped at, with no @@ -94,7 +100,7 @@ class FileActionLog : public IActionLog { _seenIdempotencyKeys.insert(existing.idempotencyKey); } } - _file = std::fopen(_path.string().c_str(), "a"); + _file = _io.fopen(_path.string(), "a"); if (_file == nullptr) { throw std::runtime_error("FileActionLog: failed to open " + _path.string()); } @@ -143,13 +149,7 @@ class FileActionLog : public IActionLog { entry.seq = ++_nextSeq; auto line = toJson(entry); line.push_back('\n'); - // Not covered by this file's own test suite: forcing a short fwrite() - // needs a real OS-level failure (disk full, fd closed underneath us) - // at this exact point, and the codebase has no fault-injection seam - // for file I/O to trigger that on demand (see - // LASTRADA-Software/morph#97, requesting one). Left uncovered rather - // than shipping a flaky test that fills the disk. - if (std::fwrite(line.data(), 1, line.size(), _file) != line.size()) { + if (_io.fwrite(line.data(), line.size(), _file) != line.size()) { throw std::runtime_error("FileActionLog::append: short write to " + _path.string()); } if (!entry.idempotencyKey.empty()) { @@ -176,22 +176,11 @@ class FileActionLog : public IActionLog { void flush() override { std::scoped_lock const lock{_mtx}; requireOpen("flush"); - // Not covered by this file's own test suite: forcing fflush()/fsync() - // to fail here needs a real OS-level failure (disk full, fd closed - // underneath us) at this exact point, and the codebase has no - // fault-injection seam for file I/O to trigger that on demand (see - // LASTRADA-Software/morph#97, requesting one). Left uncovered rather - // than shipping a flaky test that fills the disk. - if (std::fflush(_file) != 0) { + if (_io.fflush(_file) != 0) { _unflushedIdempotencyKeys.clear(); throw std::runtime_error("FileActionLog::flush: failed to flush " + _path.string()); } -#ifdef _WIN32 - int const syncResult = _commit(_fileno(_file)); -#else - int const syncResult = ::fsync(fileno(_file)); -#endif - if (syncResult != 0) { + if (_io.fsync(_file) != 0) { _unflushedIdempotencyKeys.clear(); throw std::runtime_error("FileActionLog::flush: failed to fsync " + _path.string()); } @@ -287,21 +276,10 @@ class FileActionLog : public IActionLog { // flush() itself takes _mtx and this is already under it. A failure is // raised before the file is closed and renamed, so a segment is never // sealed around entries that never reached the disk. - // - // Not covered by this file's own test suite: forcing fflush()/fsync() - // to fail here needs a real OS-level failure at this exact point, and - // the codebase has no fault-injection seam for file I/O to trigger - // that on demand (see LASTRADA-Software/morph#97, requesting one). - // Left uncovered rather than shipping a flaky test that fills the disk. - if (std::fflush(_file) != 0) { + if (_io.fflush(_file) != 0) { throw std::runtime_error("FileActionLog::rotate: failed to flush " + _path.string()); } -#ifdef _WIN32 - int const syncResult = _commit(_fileno(_file)); -#else - int const syncResult = ::fsync(fileno(_file)); -#endif - if (syncResult != 0) { + if (_io.fsync(_file) != 0) { throw std::runtime_error("FileActionLog::rotate: failed to fsync " + _path.string()); } // Everything buffered is now durable in the segment about to be sealed. @@ -320,18 +298,7 @@ class FileActionLog : public IActionLog { // success this creates a fresh empty file; on failure it reopens the // same pre-rotation file (still holding every prior entry), so a // failed rotation never leaves the log unusable. - // - // Not covered by this file's own test suite: this branch (and the - // `_file == nullptr` states it leaves behind -- the destructor's null - // check, requireOpen()'s throwing arm, and the "successful vs failed - // rename" wording just below) only runs when this fopen() fails right - // after the rename above just succeeded. Forcing that needs the - // original path's directory to become unwritable in the gap between - // two sequential library calls, which requires a real fault-injection - // seam this codebase does not have (see LASTRADA-Software/morph#97, - // requesting one). Left uncovered rather than racing a directory - // removal against this call. - _file = std::fopen(_path.string().c_str(), "a"); + _file = _io.fopen(_path.string(), "a"); if (_file == nullptr) { throw std::runtime_error("FileActionLog::rotate: failed to reopen " + _path.string() + " after " + (renameError ? "a failed" : "a successful") + " rename to " + @@ -370,17 +337,10 @@ class FileActionLog : public IActionLog { if (errorCode || size == 0) { return; // absent or empty: nothing to repair } - std::ifstream input{_path, std::ios::binary}; - // Not covered by this file's own test suite: forcing ifstream to fail - // here needs the path to become unreadable in the gap between the - // file_size() call just above and this open -- a permission change or - // removal raced against this exact window -- and the codebase has no - // fault-injection seam for file I/O to trigger that on demand (see - // LASTRADA-Software/morph#97, requesting one). Left uncovered rather - // than racing a permission change against this call. - if (!input) { + if (!_io.canOpenForRead(_path)) { return; } + std::ifstream input{_path, std::ios::binary}; std::uintmax_t intactEnd = 0; std::uintmax_t offset = 0; std::string line; @@ -395,14 +355,7 @@ class FileActionLog : public IActionLog { if (intactEnd == size) { return; } - std::filesystem::resize_file(_path, intactEnd, errorCode); - // Not covered by this file's own test suite: forcing resize_file() to - // fail here needs a real OS-level failure (permission revoked between - // the read pass above and this truncation, disk full, etc.) at this - // exact point, and the codebase has no fault-injection seam for file - // I/O to trigger that on demand (see LASTRADA-Software/morph#97, - // requesting one). Left uncovered rather than shipping a flaky test - // that races a permission change against this call. + _io.resizeFile(_path, intactEnd, errorCode); if (errorCode) { ::morph::log::logWarn("FileActionLog: could not truncate torn trailing record in " + _path.string() + ": " + errorCode.message()); @@ -413,6 +366,7 @@ class FileActionLog : public IActionLog { } std::filesystem::path _path; + ::morph::core::FileIoOps _io; std::FILE* _file = nullptr; mutable std::mutex _mtx; uint64_t _nextSeq{0}; diff --git a/tests/test_action_log_phase2.cpp b/tests/test_action_log_phase2.cpp index 48582882..4db5c6c8 100644 --- a/tests/test_action_log_phase2.cpp +++ b/tests/test_action_log_phase2.cpp @@ -14,6 +14,7 @@ #include #include #include +#include #include #include #include @@ -607,3 +608,199 @@ TEST_CASE("FileActionLog::rotate: promotes unflushed idempotencyKeys into durabl REQUIRE(sealedEntries.size() == 1); REQUIRE(sealedEntries[0].idempotencyKey == "row-1"); } + +// ── FileIoOps fault injection (LASTRADA-Software/morph#97) ───────────────── +// +// Every branch below only runs when a real OS-level file-I/O call fails +// partway through an otherwise-successful operation -- previously +// unreachable from a portable unit test (see each site's prior in-code +// comment, now removed since these tests close them for real). FileIoOps +// (morph/core/file_io_ops.hpp) makes each call injectable; every test here +// overrides exactly one member and leaves the rest at their real defaults, +// so the surrounding I/O still touches the real filesystem normally. + +TEST_CASE("FileActionLog::append: a short fwrite() throws and does not record the idempotencyKey as unflushed", + "[action_log][phase2][file][fault-injection]") { + TempFile const tmp{"file_fault_append_short_write"}; + morph::core::FileIoOps ioOps; + ioOps.fwrite = [](const void*, std::size_t size, std::FILE*) { return size - 1; }; // always short by one byte + + FileActionLog log{tmp.path, ioOps}; + auto entry = makeEntry("P2_Model", "acct-1", "P2_Deposit", "{}", "10"); + entry.idempotencyKey = "row-1"; + REQUIRE_THROWS_AS(log.append(entry), std::runtime_error); + + // The failed write must not have left the key recorded as unflushed -- + // a retry of the same row must not be silently deduplicated away. + morph::core::FileIoOps realOps; // the retry itself must actually succeed + FileActionLog log2{tmp.path, realOps}; + log2.append(entry); + log2.flush(); + REQUIRE(log2.entries().size() == 1); +} + +TEST_CASE("FileActionLog::flush: a failing fflush() throws and forgets the unflushed idempotencyKeys", + "[action_log][phase2][file][fault-injection]") { + TempFile const tmp{"file_fault_flush_fflush"}; + // `log` stores its own copy of ioOps by value (FileIoOps's whole point is + // to be a plain, copyable strategy) -- mutating this local `ioOps` after + // construction has no effect on what `log` already captured. A shared + // `shouldFail` flag the lambda itself reads is what lets one FileIoOps + // instance flip from failing to succeeding mid-test. + auto shouldFail = std::make_shared(true); + morph::core::FileIoOps ioOps; + ioOps.fflush = [shouldFail](std::FILE* file) { return *shouldFail ? -1 : std::fflush(file); }; + + FileActionLog log{tmp.path, ioOps}; + auto entry = makeEntry("P2_Model", "acct-1", "P2_Deposit", "{}", "10"); + entry.idempotencyKey = "row-1"; + log.append(entry); + REQUIRE_THROWS_AS(log.flush(), std::runtime_error); + + // Forgotten, not durably deduplicated: a retry must actually re-append. + *shouldFail = false; + log.append(entry); + log.flush(); + REQUIRE(log.entries().size() == 2); +} + +TEST_CASE("FileActionLog::flush: a failing fsync() throws and forgets the unflushed idempotencyKeys", + "[action_log][phase2][file][fault-injection]") { + TempFile const tmp{"file_fault_flush_fsync"}; + auto shouldFail = std::make_shared(true); + morph::core::FileIoOps ioOps; + morph::core::FileIoOps const realOps; // captures the real default fsync callback to fall back to + ioOps.fsync = [shouldFail, realOps](std::FILE* file) { return *shouldFail ? -1 : realOps.fsync(file); }; + + FileActionLog log{tmp.path, ioOps}; + auto entry = makeEntry("P2_Model", "acct-1", "P2_Deposit", "{}", "10"); + entry.idempotencyKey = "row-1"; + log.append(entry); + REQUIRE_THROWS_AS(log.flush(), std::runtime_error); + + *shouldFail = false; + log.append(entry); + log.flush(); + REQUIRE(log.entries().size() == 2); +} + +TEST_CASE("FileActionLog::rotate: a failing pre-rotation fflush() throws before anything is closed or renamed", + "[action_log][phase2][file][fault-injection]") { + TempFile const active{"file_fault_rotate_fflush_active"}; + TempFile const sealed{"file_fault_rotate_fflush_sealed"}; + auto shouldFail = std::make_shared(true); + morph::core::FileIoOps ioOps; + ioOps.fflush = [shouldFail](std::FILE* file) { return *shouldFail ? -1 : std::fflush(file); }; + + FileActionLog log{active.path, ioOps}; + log.append(makeEntry("P2_Model", "acct-1", "P2_Deposit", "{}", "10")); + REQUIRE_THROWS_AS(log.rotate(sealed.path), std::runtime_error); + + // Nothing was closed or renamed: the active file is exactly as it was, + // and no sealed file was ever created. + REQUIRE_FALSE(std::filesystem::exists(sealed.path)); + *shouldFail = false; + log.flush(); + REQUIRE(log.entries().size() == 1); +} + +TEST_CASE("FileActionLog::rotate: a failing pre-rotation fsync() throws before anything is closed or renamed", + "[action_log][phase2][file][fault-injection]") { + TempFile const active{"file_fault_rotate_fsync_active"}; + TempFile const sealed{"file_fault_rotate_fsync_sealed"}; + auto shouldFail = std::make_shared(true); + morph::core::FileIoOps ioOps; + morph::core::FileIoOps const realOps; + ioOps.fsync = [shouldFail, realOps](std::FILE* file) { return *shouldFail ? -1 : realOps.fsync(file); }; + + FileActionLog log{active.path, ioOps}; + log.append(makeEntry("P2_Model", "acct-1", "P2_Deposit", "{}", "10")); + REQUIRE_THROWS_AS(log.rotate(sealed.path), std::runtime_error); + + REQUIRE_FALSE(std::filesystem::exists(sealed.path)); + *shouldFail = false; + log.flush(); + REQUIRE(log.entries().size() == 1); +} + +TEST_CASE( + "FileActionLog::rotate: a failing reopen after a successful rename leaves the log closed, " + "requireOpen()'s throwing arm reachable, and the destructor's null check load-bearing", + "[action_log][phase2][file][fault-injection]") { + // The one scenario morph#97 called out as needing the *most* real-world + // contortion to reach without this seam: fopen() failing on the reopen + // right after the rename to sealedPath already succeeded. With FileIoOps, + // this is just "let the constructor's own fopen() through for real, then + // fail every fopen() call after it" -- a call counter, since rotator's + // own construction needs a real, valid handle before rotate() ever runs. + // A single FileActionLog instance for the whole test: a second instance + // on the same path would hold its own competing file handle open, which + // is exactly the kind of extra concurrency this test does not need. + TempFile const active{"file_fault_rotate_reopen_active"}; + TempFile const sealed{"file_fault_rotate_reopen_sealed"}; + + auto callCount = std::make_shared(0); + morph::core::FileIoOps ioOps; + morph::core::FileIoOps const realOps; + ioOps.fopen = [callCount, realOps](const std::string& path, const char* mode) -> std::FILE* { + return (*callCount)++ == 0 ? realOps.fopen(path, mode) : nullptr; + }; + FileActionLog rotator{active.path, ioOps}; + rotator.append(makeEntry("P2_Model", "acct-1", "P2_Deposit", "{}", "10")); + rotator.flush(); + + REQUIRE_THROWS_AS(rotator.rotate(sealed.path), std::runtime_error); + // The rename itself succeeded (fflush/fsync were untouched -- only fopen + // fails), so the sealed file now holds every entry that was on disk. + REQUIRE(std::filesystem::exists(sealed.path)); + + // The log is left with no open file: append()/flush()/a further rotate() + // must all throw via requireOpen(), not dereference a null handle. + REQUIRE_THROWS_AS(rotator.append(makeEntry("P2_Model", "acct-1", "P2_Deposit", "{}", "20")), std::runtime_error); + REQUIRE_THROWS_AS(rotator.flush(), std::runtime_error); + REQUIRE_THROWS_AS(rotator.rotate(sealed.path), std::runtime_error); + + // Destruction with _file == nullptr must be safe (the null check in the + // destructor is load-bearing, not defensive noise) -- rotator's own + // scope exit below exercises exactly that. +} + +TEST_CASE("FileActionLog: a torn trailing record whose path becomes unreadable is left untouched", + "[action_log][phase2][file][fault-injection]") { + TempFile const tmp{"file_fault_repair_unreadable"}; + { + std::ofstream out{tmp.path, std::ios::binary}; + out << R"({"seq":1,"modelType":"P2_Model","entityKey":"acct-1","actionType":"P2_Deposit","payload":"{}","result":"10","principal":"","timestampMs":0})" + << "\n"; + out << R"({"seq":2,"modelType":"P2_Model")"; // torn trailing record, no newline + } + auto const sizeBefore = std::filesystem::file_size(tmp.path); + + morph::core::FileIoOps ioOps; + ioOps.canOpenForRead = [](const std::filesystem::path&) { return false; }; + FileActionLog log{tmp.path, ioOps}; // repairTornTail() must skip the truncation + + REQUIRE(std::filesystem::file_size(tmp.path) == sizeBefore); +} + +TEST_CASE("FileActionLog: a torn trailing record whose resize_file() fails is logged, not silently swallowed", + "[action_log][phase2][file][fault-injection]") { + TempFile const tmp{"file_fault_repair_resize_fails"}; + { + std::ofstream out{tmp.path, std::ios::binary}; + out << R"({"seq":1,"modelType":"P2_Model","entityKey":"acct-1","actionType":"P2_Deposit","payload":"{}","result":"10","principal":"","timestampMs":0})" + << "\n"; + out << R"({"seq":2,"modelType":"P2_Model")"; // torn trailing record, no newline + } + auto const sizeBefore = std::filesystem::file_size(tmp.path); + + morph::core::FileIoOps ioOps; + ioOps.resizeFile = [](const std::filesystem::path&, std::uintmax_t, std::error_code& errorCode) { + errorCode = std::make_error_code(std::errc::permission_denied); + }; + FileActionLog log{tmp.path, ioOps}; // repairTornTail() logs a warning and returns, does not throw + + // The failed truncation left the file exactly as it was -- not repaired, + // but not corrupted further either. + REQUIRE(std::filesystem::file_size(tmp.path) == sizeBefore); +} From 9a3ed10c16b30f8a2d9a93f12cbf0289c84f870c Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Sat, 15 Aug 2026 22:52:42 +0300 Subject: [PATCH 2/3] tests(offline): wire FileOfflineQueue through FileIoOps; close its I/O gaps Same shared seam FileActionLog now uses (morph::core::FileIoOps) -- FileOfflineQueue has the identical class of gap: several branches only ran when a real OS-level file-I/O call failed partway through an otherwise-successful operation. Constructor takes an optional second FileIoOps parameter (default {}), and every fwrite/fflush/fsync/fopen call site (writeLine/syncFile, and compact()'s own temp-file write) now routes through it, syncFile changed from static to an instance method since it reads _io. 6 new fault-injection tests in test_file_offline_queue.cpp: the constructor's own append-mode fopen() failing, enqueue()'s short fwrite()/failing fflush()/failing fsync(), and construction-time compact()'s own short fwrite()/failing fflush(). Full morph_tests suite (1044 cases / 10034 assertions) passes. docs/spec/offline/offline.md updated for the new constructor signature, per CLAUDE.md. --- docs/spec/offline/offline.md | 7 ++ include/morph/offline/file_offline_queue.hpp | 28 ++--- tests/test_file_offline_queue.cpp | 101 +++++++++++++++++++ 3 files changed, 123 insertions(+), 13 deletions(-) diff --git a/docs/spec/offline/offline.md b/docs/spec/offline/offline.md index 86d3dd88..88969b06 100644 --- a/docs/spec/offline/offline.md +++ b/docs/spec/offline/offline.md @@ -222,6 +222,13 @@ queue depths; `SqliteOfflineQueue` is the index-backed alternative for high-volume keyed enqueues. Not safe for multiple processes to open the same path concurrently. +The constructor takes an optional second `morph::core::FileIoOps` parameter +(`FileOfflineQueue(std::filesystem::path, morph::core::FileIoOps = {})`) — the +same test-only fault-injection seam `FileActionLog` uses (see +`docs/spec/journal/journal.md`): the raw `fwrite`/`fflush`/`fsync`/`fopen` +calls this class makes, as an injectable strategy defaulting to the real +syscalls. A normal caller never passes one. + ### `SqliteOfflineQueue` Reference SQLite-backed `IOfflineQueue` (`sqlite_offline_queue.hpp`), built diff --git a/include/morph/offline/file_offline_queue.hpp b/include/morph/offline/file_offline_queue.hpp index 7b39fc66..dfdce716 100644 --- a/include/morph/offline/file_offline_queue.hpp +++ b/include/morph/offline/file_offline_queue.hpp @@ -15,6 +15,7 @@ #include #include +#include "../core/file_io_ops.hpp" #include "../core/logger.hpp" #include "offline_queue.hpp" @@ -135,13 +136,18 @@ class FileOfflineQueue : public IOfflineQueue { /// @brief Opens (or creates) the queue log at @p path, replaying and /// compacting whatever is already there. /// @param path NDJSON file to store queue state in. + /// @param ioOps Injectable file-I/O primitives; defaults to the real + /// syscalls. Test-only seam — see `morph::core::FileIoOps`'s own + /// docs — for forcing the failure branches that need a real + /// OS-level I/O error to reach. /// @throws FileOfflineQueueError if @p path exists but contains a /// malformed non-trailing line. /// @throws std::runtime_error if @p path cannot be opened/rewritten. - explicit FileOfflineQueue(std::filesystem::path path) : _path{std::move(path)} { + explicit FileOfflineQueue(std::filesystem::path path, ::morph::core::FileIoOps ioOps = {}) + : _path{std::move(path)}, _io{std::move(ioOps)} { load(); compact(); - _file = std::fopen(_path.string().c_str(), "a"); + _file = _io.fopen(_path.string(), "a"); if (_file == nullptr) { throw std::runtime_error("FileOfflineQueue: failed to open " + _path.string()); } @@ -263,22 +269,17 @@ class FileOfflineQueue : public IOfflineQueue { void writeLine(const std::string& json) { std::string line = json; line.push_back('\n'); - if (std::fwrite(line.data(), 1, line.size(), _file) != line.size()) { + if (_io.fwrite(line.data(), line.size(), _file) != line.size()) { throw std::runtime_error("FileOfflineQueue: short write to " + _path.string()); } syncFile(_file, _path.string()); } - static void syncFile(std::FILE* file, const std::string& what) { - if (std::fflush(file) != 0) { + void syncFile(std::FILE* file, const std::string& what) const { + if (_io.fflush(file) != 0) { throw std::runtime_error("FileOfflineQueue: failed to flush " + what); } -#ifdef _WIN32 - int const syncResult = _commit(_fileno(file)); -#else - int const syncResult = ::fsync(fileno(file)); -#endif - if (syncResult != 0) { + if (_io.fsync(file) != 0) { throw std::runtime_error("FileOfflineQueue: failed to fsync " + what); } } @@ -328,14 +329,14 @@ class FileOfflineQueue : public IOfflineQueue { /// append-mode `_file` handle is opened for new writes. void compact() { std::string const tmp = _path.string() + ".compact-tmp"; - std::FILE* out = std::fopen(tmp.c_str(), "w"); + std::FILE* out = _io.fopen(tmp, "w"); if (out == nullptr) { throw std::runtime_error("FileOfflineQueue: failed to open " + tmp + " for compaction"); } auto writeRecord = [&](const detail::FileQueueRecord& record) { std::string outLine = detail::toJson(record); outLine.push_back('\n'); - if (std::fwrite(outLine.data(), 1, outLine.size(), out) != outLine.size()) { + if (_io.fwrite(outLine.data(), outLine.size(), out) != outLine.size()) { // Closing on the way out of a throw; nothing to report a // close failure to. // NOLINTNEXTLINE(cert-err33-c, cppcoreguidelines-owning-memory) @@ -381,6 +382,7 @@ class FileOfflineQueue : public IOfflineQueue { } std::filesystem::path _path; + ::morph::core::FileIoOps _io; std::FILE* _file = nullptr; std::mutex _mtx; std::map _items; diff --git a/tests/test_file_offline_queue.cpp b/tests/test_file_offline_queue.cpp index 0d62dfb3..e101e3a7 100644 --- a/tests/test_file_offline_queue.cpp +++ b/tests/test_file_offline_queue.cpp @@ -5,6 +5,7 @@ #include #include #include +#include #include #include #include @@ -406,3 +407,103 @@ TEST_CASE("morph::offline::FileOfflineQueue: construction throws if the compacti auto const path = std::filesystem::path{"/no/such/directory/at/all/queue.ndjson"}; REQUIRE_THROWS_AS(morph::offline::FileOfflineQueue(path), std::runtime_error); } + +// ── FileIoOps fault injection (LASTRADA-Software/morph#97) ───────────────── +// +// Same seam FileActionLog's own fault-injection tests use (morph/core/ +// file_io_ops.hpp) -- FileOfflineQueue has the identical class of gap: +// several branches only run when a real OS-level file-I/O call fails +// partway through an otherwise-successful operation. + +TEST_CASE("morph::offline::FileOfflineQueue: the constructor's own append-mode fopen() failing throws", + "[file_queue][fault-injection]") { + auto path = tempQueuePath(); + std::filesystem::remove(path); + morph::core::FileIoOps ioOps; + ioOps.fopen = [](const std::string&, const char*) -> std::FILE* { return nullptr; }; + REQUIRE_THROWS_AS(morph::offline::FileOfflineQueue(path, ioOps), std::runtime_error); + std::filesystem::remove(path); +} + +TEST_CASE("morph::offline::FileOfflineQueue::enqueue: a short fwrite() to the append-mode file throws", + "[file_queue][fault-injection]") { + auto path = tempQueuePath(); + std::filesystem::remove(path); + auto shouldFail = std::make_shared(false); + morph::core::FileIoOps ioOps; + ioOps.fwrite = [shouldFail](const void* buffer, std::size_t size, std::FILE* file) { + return *shouldFail ? size - 1 : std::fwrite(buffer, 1, size, file); + }; + + { + morph::offline::FileOfflineQueue queue{path, ioOps}; + *shouldFail = true; + REQUIRE_THROWS_AS(queue.enqueue("payload"), std::runtime_error); + } // queue's own file handle must close before remove() -- Windows cannot delete an open file + std::filesystem::remove(path); +} + +TEST_CASE("morph::offline::FileOfflineQueue::enqueue: a failing fflush() on the append-mode file throws", + "[file_queue][fault-injection]") { + auto path = tempQueuePath(); + std::filesystem::remove(path); + auto shouldFail = std::make_shared(false); + morph::core::FileIoOps ioOps; + ioOps.fflush = [shouldFail](std::FILE* file) { return *shouldFail ? -1 : std::fflush(file); }; + + { + morph::offline::FileOfflineQueue queue{path, ioOps}; + *shouldFail = true; + REQUIRE_THROWS_AS(queue.enqueue("payload"), std::runtime_error); + } + std::filesystem::remove(path); +} + +TEST_CASE("morph::offline::FileOfflineQueue::enqueue: a failing fsync() on the append-mode file throws", + "[file_queue][fault-injection]") { + auto path = tempQueuePath(); + std::filesystem::remove(path); + auto shouldFail = std::make_shared(false); + morph::core::FileIoOps ioOps; + morph::core::FileIoOps const realOps; + ioOps.fsync = [shouldFail, realOps](std::FILE* file) { return *shouldFail ? -1 : realOps.fsync(file); }; + + { + morph::offline::FileOfflineQueue queue{path, ioOps}; + *shouldFail = true; + REQUIRE_THROWS_AS(queue.enqueue("payload"), std::runtime_error); + } + std::filesystem::remove(path); +} + +TEST_CASE( + "morph::offline::FileOfflineQueue: a short fwrite() during construction-time compaction throws before the " + "append-mode file is ever opened", + "[file_queue][fault-injection]") { + auto path = tempQueuePath(); + { + // Seed one surviving item so compact() has at least one "put" line to + // write -- an empty queue's compact() writes nothing and never calls + // fwrite at all. + std::ofstream out{path}; + out << R"({"op":"put","id":1,"payload":"seed","idempotencyKey":"","attempts":0})" << "\n"; + } + morph::core::FileIoOps ioOps; + ioOps.fwrite = [](const void*, std::size_t size, std::FILE*) { return size - 1; }; + REQUIRE_THROWS_AS(morph::offline::FileOfflineQueue(path, ioOps), std::runtime_error); + std::filesystem::remove(path); +} + +TEST_CASE( + "morph::offline::FileOfflineQueue: a failing fflush() during construction-time compaction throws", + "[file_queue][fault-injection]") { + auto path = tempQueuePath(); + { + std::ofstream out{path}; + out << R"({"op":"put","id":1,"payload":"seed","idempotencyKey":"","attempts":0})" << "\n"; + } + morph::core::FileIoOps ioOps; + ioOps.fflush = [](std::FILE*) { return -1; }; + REQUIRE_THROWS_AS(morph::offline::FileOfflineQueue(path, ioOps), std::runtime_error); + std::filesystem::remove(path); +} From 563697639b5fd13f1847000f3d381d807d1dcda1 Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Sat, 15 Aug 2026 23:10:29 +0300 Subject: [PATCH 3/3] docs(core): add design spec for FileIoOps Was missing from the previous commits -- the Header <-> spec sync gate correctly caught it (include/morph/core/file_io_ops.hpp is new, but no docs/spec/core/** file changed alongside it). One file per public type, per CLAUDE.md. --- docs/spec/core/file_io_ops.md | 100 ++++++++++++++++++++++++++++++++++ 1 file changed, 100 insertions(+) create mode 100644 docs/spec/core/file_io_ops.md diff --git a/docs/spec/core/file_io_ops.md b/docs/spec/core/file_io_ops.md new file mode 100644 index 00000000..be850dea --- /dev/null +++ b/docs/spec/core/file_io_ops.md @@ -0,0 +1,100 @@ +# `morph::core::FileIoOps` — design + +`morph::core::FileIoOps` (`include/morph/core/file_io_ops.hpp`) is an +injectable strategy for the raw file-I/O primitives `morph::journal:: +FileActionLog` and `morph::offline::FileOfflineQueue` both call: +`fwrite`, `fflush`, `fsync`/`_commit`, `fopen`, an ifstream-open probe, and +`std::filesystem::resize_file`. Every member is a `std::function` defaulting +to the real syscall/stdlib call it stands in for. + +## Contents + +- [Why it exists](#why-it-exists) +- [Shape](#shape) +- [Usage](#usage) +- [Thread safety](#thread-safety) +- [Cross-references](#cross-references) + +## Why it exists + +`FileActionLog`/`FileOfflineQueue` both have several branch arms that only +run when a real OS-level file-I/O call fails partway through an +otherwise-successful operation — disk full, a file descriptor closed +underneath, a permission change racing an exact window between two library +calls. None of those are reachable from a portable unit test without a way +to fail one specific call on demand (see `LASTRADA-Software/morph#97`, +which requested exactly this for `FileActionLog`; `FileOfflineQueue` has the +identical gap). + +`FileIoOps` is that seam. A test constructs one, overrides the one member it +wants to fail (optionally gated behind a `std::shared_ptr` or a call +counter so it only fails on a specific call, not every call for the rest of +the object's lifetime), and passes it to the class under test's constructor. +Every other member stays at its real default, so the rest of the class's I/O +behaves normally around the one injected failure. + +## Shape + +A plain aggregate of six `std::function` members, each mirroring one +underlying call: + +| Member | Mirrors | Signature | +|---|---|---| +| `fwrite` | `std::fwrite` | `size_t(const void*, size_t, FILE*)` | +| `fflush` | `std::fflush` | `int(FILE*)` | +| `fsync` | POSIX `fsync` / Windows `_commit` | `int(FILE*)` | +| `fopen` | `std::fopen` | `FILE*(const std::string&, const char*)` | +| `canOpenForRead` | `std::ifstream{path}`'s own open check | `bool(const std::filesystem::path&)` | +| `resizeFile` | `std::filesystem::resize_file` | `void(const std::filesystem::path&, uintmax_t, std::error_code&)` | + +`canOpenForRead` exists because a fault-injection test has no way to make a +*real* `std::ifstream` construction fail without actually breaking the +filesystem — so `repairTornTail()`'s "can I read this path right now" check +is factored out as its own predicate, consulted before the real +`std::ifstream` is constructed, rather than trying to intercept the stream +construction itself. + +## Usage + +Both classes take an optional second constructor parameter: + +```cpp +explicit FileActionLog(std::filesystem::path path, morph::core::FileIoOps ioOps = {}); +explicit FileOfflineQueue(std::filesystem::path path, morph::core::FileIoOps ioOps = {}); +``` + +A normal caller never passes one — the default-constructed `FileIoOps` is +byte-for-byte what both classes called directly before this seam existed, so +this is not a behavior change for any existing caller. Example, forcing a +short write: + +```cpp +morph::core::FileIoOps ioOps; +ioOps.fwrite = [](const void* buffer, std::size_t size, std::FILE* file) { + return size - 1; // always one byte short +}; +FileActionLog log{path, ioOps}; +REQUIRE_THROWS_AS(log.append(entry), std::runtime_error); +``` + +To fail only a *specific* call (e.g. "the reopen after rotate()'s rename, not +the constructor's own open"), capture a shared counter or flag and check it +inside the lambda — see `tests/test_action_log_phase2.cpp`'s and +`tests/test_file_offline_queue.cpp`'s own fault-injection test cases for the +established idiom. + +## Thread safety + +`FileIoOps` itself is a plain value type with no shared state — copying or +moving one has ordinary value semantics. Whether the *callbacks* themselves +are safe to call from multiple threads concurrently is up to whatever a test +installs; the real default callbacks are exactly the real syscalls, which +already have their own well-defined thread-safety. + +## Cross-references + +- [`docs/spec/journal/journal.md`](../journal/journal.md) — `FileActionLog`'s + own design, including the branches this seam closes. +- [`docs/spec/offline/offline.md`](../offline/offline.md) — `FileOfflineQueue`'s + own design, including the identical class of branch this seam closes. +- `LASTRADA-Software/morph#97` — the issue that requested this seam.