diff --git a/include/rcp/alloc.hpp b/include/rcp/alloc.hpp new file mode 100644 index 0000000..ff2662e --- /dev/null +++ b/include/rcp/alloc.hpp @@ -0,0 +1,225 @@ +// Deterministic allocation-failure fault injection (Phase 17, cpp-RCP issue +// #129 — ROADMAP.md Phase 17 §2, "a fresh allocation seam (dependency- +// injected/std::pmr-style, not a global hook table) with fault-injection +// tests from day one"). +// +// ── Why this exists ────────────────────────────────────────────────────────── +// A prior audit this session found cpp-RCP had no equivalent at all to +// c-RCP's rcp_alloc_set_hooks() (include/rcp/alloc.h + src/alloc.c): no way +// to deterministically exercise an allocation-failure branch in a test, and +// (confirmed by grepping this tree) no try/catch(std::bad_alloc) or any +// other allocation-failure handling anywhere in this codebase at all — +// every allocating call site here (std::vector growth, std::make_unique, +// bare `new`) currently just lets std::bad_alloc propagate uncaught. +// +// ── Why this is NOT a port of c-RCP's alloc.h/alloc.c mechanism ───────────── +// c-RCP's rcp_alloc_set_hooks() is a single process-wide global function- +// pointer table that every rcp_malloc()/rcp_calloc()/rcp_realloc()/ +// rcp_free() call in that codebase routes through. That design is the +// *wrong* pattern to port literally here, and c-RCP's own history is the +// evidence: issue #600/PR #615 ([c-RCP-23b]) had to retrofit +// rcp_alloc_lock_hooks()/rcp_alloc_unlock_hooks() as a second mechanism +// bolted on top, purely to close an access-control gap the global table +// itself created — with no lock engaged, ANY caller anywhere in the same +// process (including unrelated QM-rated code) could call +// rcp_alloc_set_hooks() at any time and silently redirect the allocator +// backing c-RCP's own ASIL-B-rated e2e.c/watchdog.c call sites, with no +// detection and no attribution (see c-RCP's alloc.h header comment, "Locking +// the hook table" section). That is exactly the class of bug cpp-RCP's +// existing architecture already avoids by construction: every stateful +// primitive in this codebase (rcp/faultinject.hpp's Interceptor, rcp/ +// loan.hpp's BufferPool, rcp/watchdog.hpp's Manager, ...) is an +// instance-owned object a caller explicitly holds and passes by reference — +// there is no global, ambiently-reachable mutable state anywhere for an +// unrelated caller to silently repoint. Introducing a global operator-new +// override (the only way to literally intercept "every std::vector/new in +// the whole program", short of a hook table) would reintroduce precisely +// that problem, and would not even be fully portable across this project's +// target platforms (macOS's two-level namespace, MSVC's own linker model — +// the identical portability motivation c-RCP's own alloc.h header comment +// gives for why it built a function-pointer indirection in the first place, +// rather than relying on malloc-symbol interposition). +// +// ── Design ──────────────────────────────────────────────────────────────── +// FaultInjector below follows rcp/faultinject.hpp's own idiom exactly: a +// small, instance-owned, std::mutex-guarded class a caller constructs, holds +// (typically via reference or pointer, dependency-injected into whatever it +// is testing), and explicitly arms/disarms — never a singleton, never +// ambiently reachable. It does not attempt to intercept allocation +// transparently; instead, a specific, already-allocating call site opts in +// by calling FaultInjector::should_fail() immediately before it would +// otherwise allocate, and takes that as a signal to report a simulated +// failure through this codebase's own std::error_code convention (matching +// this codebase's established convention elsewhere — see rcp/request.hpp's +// RequestErrc, rcp/watchdog.hpp's WatchdogErrc, etc. — rather than throwing, +// since nothing in this codebase currently catches an allocation-related +// exception at all). +// +// ── Scope: Phase 1's bounded-capacity paths were audited, not wired in ────── +// The task guidance for this pass named four Phase 1 fixed-capacity modules +// as candidates for fault-injection integration: rcp/request.hpp's +// BoundedVector/RequestLedger, rcp/fragment.hpp's Reassembler, rcp/ +// respqueue.hpp's RespQueue, and rcp/loan.hpp's BufferPool's *free-list* +// bookkeeping. All four were already re-read for this pass (test_request.cpp +// "fixed-capacity" tests, test_fragment.cpp's "exceeding max_total_len"/ +// "exceeding this Reassembler's own fixed capacity" tests, test_respqueue.cpp +// capacity_octets/kMaxEntries tests, test_loan.cpp's "beyond the free list's +// fixed capacity" test) and found already directly, adequately tested — and, +// more fundamentally, each of those specific bounded-capacity-exceeded paths +// backs a compile-time-sized std::array (BoundedVector's std::array +// data_, Reassembler's own fixed segment buffer, RespQueue's kMaxEntries +// array, BufferPool's kPoolMaxEntries free-list array): reaching capacity is +// detected by a plain size_ >= Capacity comparison *before* touching memory, +// not by attempting and failing a heap allocation. There is no allocation on +// those specific paths for a fault injector to intercept — wiring one in +// would be inert by construction, exactly the "artificial integration into +// Phase 1's already-solid modules" the task guidance says to avoid in that +// case. +// +// What genuinely does allocate, and is not already fault-injectable, is +// rcp/loan.hpp's BufferPool::loan() *cache-miss* path — `new +// std::vector(size, 0)`, a real heap allocation whose failure this +// codebase currently cannot simulate at all. BufferPool now optionally +// accepts a FaultInjector* (default nullptr, fully backward compatible — +// see loan.hpp's own header comment) so that one genuine allocation call +// site is fault-injectable end to end, as a concrete, non-artificial +// demonstration that this seam actually composes with a real caller rather +// than existing only in isolation. FaultInjector itself is otherwise a +// general-purpose, ready-to-use utility for future phases' own allocating +// call sites (e.g. Phase 4's mock.hpp/regmap.hpp dispatch tables), matching +// this file's own "smaller, more general-purpose FaultInjector utility" +// fallback design the task guidance names explicitly. +// +// ── REQ-ALLOC-* transfer audit (c-RCP .fusa-reqs.json, 11 requirements) ───── +// c-RCP's REQ-ALLOC-001..011 describe rcp_alloc_set_hooks()'s specific +// mechanics (a global hook table, its NULL-means-reset convenience, its +// malloc/calloc/realloc/free passthrough shape, and REQ-ALLOC-007..011's +// rcp_alloc_lock_hooks()/_unlock_hooks()/_hooks_locked() access-control +// retrofit). None of the eleven transfer as written: REQ-ALLOC-001/002/010/ +// 011 (install/reset hooks, rejected-while-locked) describe a global +// mutable-table API this design deliberately does not have; REQ-ALLOC- +// 003/004/005/006 (malloc/calloc/realloc/free passthrough-or-hook +// semantics) describe C's own four-allocator-function surface, which C++ +// does not have as separate entry points (std::vector/std::make_unique/`new` +// each pick their own allocation strategy internally; there is no equivalent +// single indirection point to hook transparently without a global operator- +// new override — see this file's "Why this is NOT a port" section above); +// REQ-ALLOC-007/008/009 (lock/unlock/query-locked) exist purely to retrofit +// access control onto REQ-ALLOC-001/002's own global table, so they have no +// referent once that table doesn't exist — an instance-owned FaultInjector +// only affects the specific call sites a caller explicitly constructed it +// into, so there is no shared, ambiently-reachable state to protect from +// unrelated callers in the first place; the access-control problem those +// three requirements solve for c-RCP's design does not arise here by +// construction. The one *concept* that does transfer is REQ-ALLOC-003/004/ +// 006's underlying intent — "give the test suite a portable, deterministic +// way to force an allocation-failure branch without a real, +// practically-unreachable OOM condition or a non-portable malloc- +// interposition trick" — which is exactly what FaultInjector::should_fail() +// below provides, just via an opt-in call at the allocation site rather than +// a transparent global hook. +#pragma once + +#include +#include +#include +#include + +namespace rcp { +namespace alloc { + +// ── Errors ──────────────────────────────────────────────────────────────────── + +enum class AllocErrc : int { + simulated_allocation_failure = 1, // an armed FaultInjector reported this attempt should fail +}; + +inline const std::error_category& alloc_category() noexcept { + struct Cat : std::error_category { + const char* name() const noexcept override { return "rcp.alloc"; } + std::string message(int ev) const override { + switch (static_cast(ev)) { + case AllocErrc::simulated_allocation_failure: + return "rcp/alloc: FaultInjector simulated an allocation failure"; + default: + return "rcp/alloc: unknown error"; + } + } + }; + static Cat instance; + return instance; +} + +inline std::error_code make_error_code(AllocErrc e) noexcept { + return {static_cast(e), alloc_category()}; +} + +// ── FaultInjector ───────────────────────────────────────────────────────────── +// Instance-owned, std::mutex-guarded deterministic allocation-failure +// trigger — see this file's header comment for the full design rationale. +// Not a singleton: a caller constructs one, holds it (typically passing a +// pointer or reference into whatever it wants to fault-inject), and arms it +// explicitly. With nothing armed, should_fail() always returns false — a +// FaultInjector nobody has armed is a silent no-op, matching rcp/ +// faultinject.hpp's Interceptor's own "no active rule -> pass straight +// through" default. +class FaultInjector { +public: + // arm schedules should_fail() to report failure for the next `count` + // calls (default 1); count < 0 arms it indefinitely (every future + // should_fail() call reports failure until disarm()), matching + // rcp::faultinject::Rule::count's identical -1-means-forever + // convention. arm(0) disarms (equivalent to disarm()). + void arm(int count = 1) { + std::lock_guard lk(mu_); + remaining_ = count; + } + + void disarm() { + std::lock_guard lk(mu_); + remaining_ = 0; + } + + // armed reports whether the next should_fail() call would currently + // report failure. + bool armed() const { + std::lock_guard lk(mu_); + return remaining_ != 0; + } + + // should_fail is called by an opted-in allocation call site immediately + // before it would otherwise perform a real allocation. Returns true + // (and, unless armed indefinitely, consumes one count) exactly when + // that attempt should simulate failure instead of allocating; the + // caller is expected to translate that into its own failure signal + // (typically make_error_code(AllocErrc::simulated_allocation_failure), + // or a module-specific std::error_code of its own) rather than + // proceeding with the real allocation. + bool should_fail() noexcept { + std::lock_guard lk(mu_); + if (remaining_ == 0) return false; + if (remaining_ > 0) --remaining_; + return true; + } + + // remaining reports how many more should_fail() calls will currently + // report failure (-1 if armed indefinitely, 0 if disarmed). Test + // introspection, not part of the arm/should_fail contract itself. + int remaining() const { + std::lock_guard lk(mu_); + return remaining_; + } + +private: + mutable std::mutex mu_; + int remaining_ = 0; // 0 = disarmed; -1 = fires forever; >0 = fires N more times +}; + +} // namespace alloc +} // namespace rcp + +// Enable std::error_code construction from rcp::alloc::AllocErrc. +namespace std { +template <> +struct is_error_code_enum : true_type {}; +} // namespace std diff --git a/include/rcp/loan.hpp b/include/rcp/loan.hpp index 0328302..caa5fcc 100644 --- a/include/rcp/loan.hpp +++ b/include/rcp/loan.hpp @@ -64,8 +64,21 @@ // (see loan()'s own release closure below) instead of growing the free // list further — the same degradation c-RCP's own pool_append() documents, // never a leak or corruption, only a forfeited reuse opportunity. +// +// ── Phase 17 allocation fault-injection seam (cpp-RCP issue #129) ─────────── +// loan()'s cache-miss branch below (`new std::vector(size, 0)`) is +// a genuine heap allocation this codebase previously had no way to +// deterministically fail in a test -- unlike this file's own kPoolMaxEntries +// bound above, which is a size_ >= Capacity comparison with nothing to +// allocate at all. BufferPool now optionally accepts an rcp::alloc:: +// FaultInjector* (default nullptr -- fully backward compatible; every +// existing construction site, including new_buffer_pool(), is unaffected) +// so that call site can be exercised via fault injection. See rcp/ +// alloc.hpp's own header comment for why this is an opt-in, instance-owned +// seam rather than a global allocator hook. #pragma once +#include "alloc.hpp" #include "rcp.hpp" #include @@ -83,6 +96,16 @@ constexpr size_t kPoolMaxEntries = 64; class BufferPool { public: + BufferPool() = default; + + // fault_injector, when non-null, is consulted (should_fail()) before + // this pool's one genuine allocation call site — the cache-miss branch + // of loan() below — actually allocates. Optional and defaulted to + // nullptr so every pre-existing construction site (including + // new_buffer_pool()) is unaffected; see this file's own header comment. + // fault_injector is not owned by this pool and must outlive it. + explicit BufferPool(alloc::FaultInjector* fault_injector) : fault_injector_(fault_injector) {} + ~BufferPool() { close(); for (size_t i = 0; i < entries_len_; i++) delete entries_[i]; @@ -90,7 +113,13 @@ class BufferPool { // loan returns a zeroed buffer of exactly size bytes, drawn from the // pool if a same-or-larger buffer is available for reuse, or freshly - // allocated otherwise. + // allocated otherwise. Returns alloc::AllocErrc::simulated_allocation_ + // failure, unchanged, if a cache-miss allocation would be needed and + // this pool's fault_injector (if any) reports it should fail — + // the pool's own state is left exactly as it was before the call in + // that case (no partial buffer taken from the free list is lost: the + // free-list search below only removes an entry on a cache *hit*, which + // never reaches the fault-injection check at all). std::error_code loan(int size, std::unique_ptr& out) { if (closed_.load(std::memory_order_acquire)) return ErrClosed; if (size < 0) return std::make_error_code(std::errc::invalid_argument); @@ -111,6 +140,8 @@ class BufferPool { if (raw) { raw->assign(static_cast(size), 0); // re-zero: no stale data leaks across reuse } else { + if (fault_injector_ && fault_injector_->should_fail()) + return alloc::make_error_code(alloc::AllocErrc::simulated_allocation_failure); raw = new std::vector(static_cast(size), 0); } @@ -158,6 +189,8 @@ class BufferPool { // are not (matching c-RCP's own design exactly). std::array*, kPoolMaxEntries> entries_{}; size_t entries_len_ = 0; // always <= kPoolMaxEntries + + alloc::FaultInjector* fault_injector_ = nullptr; // not owned; see constructor doc comment }; inline std::unique_ptr new_buffer_pool() { diff --git a/include/rcp/sim.hpp b/include/rcp/sim.hpp index 76e3836..fefae90 100644 --- a/include/rcp/sim.hpp +++ b/include/rcp/sim.hpp @@ -86,11 +86,13 @@ class Simulator final { watchdog::Manager& watchdog() noexcept { return watchdog_mgr_; } // register_stream begins watchdog tracking for a request stream — - // forwards to rcp::watchdog::Manager::register_stream (v2.10.0). Call - // once per stream a scenario wants watchdog-miss detection for; a - // stream dispatch() is never called for simply never has its watchdog - // kicked or polled. - void register_stream(uint64_t stream_key) { watchdog_mgr_.register_stream(stream_key); } + // forwards to rcp::watchdog::Manager::register_stream (v2.10.0), + // including its fixed-capacity result (Phase 17 c-RCP-reference pass, + // cpp-RCP issue #129: Manager::kMaxStreams) rather than silently + // discarding it. Call once per stream a scenario wants watchdog-miss + // detection for; a stream dispatch() is never called for simply never + // has its watchdog kicked or polled. + std::error_code register_stream(uint64_t stream_key) { return watchdog_mgr_.register_stream(stream_key); } // fault injects `err` on every subsequent dispatch() call until // recover() is called — the same Fault/Recover scenario-testing diff --git a/include/rcp/watchdog.hpp b/include/rcp/watchdog.hpp index 3de601e..a5dd527 100644 --- a/include/rcp/watchdog.hpp +++ b/include/rcp/watchdog.hpp @@ -7,6 +7,86 @@ // fusa:req REQ-WDG-007 // fusa:req REQ-WDG-008 +// ── Phase 17 c-RCP-reference pass (cpp-RCP issue #129) ─────────────────────── +// c-RCP's include/rcp/watchdog.h + src/watchdog.c (this project's RC5-spec- +// conformant reference implementation) is a substantially different design +// from the one below: `rcp_watchdog_keeper_t` owns a background +// re-evaluation thread (default poll_interval_ms = 10, `rcp_watchdog_ +// default_config()`), is constructed once from a fixed array of streams +// (`rcp_watchdog_keeper_new(cfg, streams, n_streams)`), and reports state +// purely via a cached `rcp_e2e_wd_result_t` (`rcp_watchdog_keeper_status()`) +// updated by that thread, fired to subscribers on change. That shape is +// itself a *later* c-RCP redesign (issue #338/[c-RCP-17]) of an even +// earlier caller-driven c-RCP watchdog; this codebase's own StreamWatchdog/ +// Manager below is a third, independently-engineered design, derived +// directly from the TC18 extraction (§3.8) at v2.10.0 (ROADMAP.md milestone +// 54) rather than transliterated from either c-RCP shape — and already the +// one every other Phase 17 module in this tree depends on (rcp/sim.hpp's +// Simulator::register_stream/poll_watchdog, rcp/mdns.hpp's ServerInfo:: +// stream_key convention). Per this rewrite's own stated approach ("re-derive +// ..., translated into idiomatic C++, not transliterated C" — ROADMAP.md +// Phase 17), the audit below evaluates c-RCP's 12 REQ-WDG-* requirements +// (.fusa-reqs.json) against this file's own already-shipped, independently- +// numbered REQ-WDG-001..008 catalog (this project's own .fusa-reqs.json) for +// *behavioral* content gaps, not API-shape gaps: +// +// - c-RCP REQ-WDG-001/002 (re-run evaluate() at poll interval; elapsed = +// time since last kick) and REQ-WDG-003/004/005/008 (kick/status/notify/ +// disabled-never-overflows) are behaviorally already covered by this +// file's own REQ-WDG-001/002/004/005/008 — StreamWatchdog::check/ +// kick_from_request compute the identical "elapsed since last kick vs. +// rx_wd_timeout_interval, gated on rx_wd_enable" rule, just evaluated +// on caller-driven poll() rather than a thread's own timer. +// - c-RCP REQ-WDG-006 (event callback fires on result *change*) does NOT +// transfer as written: this file's Manager::poll fires subscribers on +// every HealthEvent poll() *produces* (REQ-WDG-008, below), which is a +// deliberately different, already-tested edge-triggered contract (an +// overflow event and a still-latched info-notification are both +// "produced", not deduplicated against a cached previous result) — c-RCP +// itself changed on essentially this exact axis across its own +// watchdog redesigns; no cached rcp_e2e_wd_result_t exists here to +// compare against for a "changed" test in the first place. +// - c-RCP REQ-WDG-007 (close() terminates the poll thread, idempotent) and +// REQ-WDG-011 (default_config poll_interval_ms) do not transfer: this +// header owns no clock or background thread of its own, same +// disclaimer as every other endpoint/lifecycle header in this codebase +// since v2.6.0 (see this file's own header comment below) — there is no +// thread to close and no poll interval to default. +// - c-RCP REQ-WDG-009 (initial status computed synchronously before +// rcp_watchdog_keeper_new() returns, so status() never observes a stale +// placeholder) does not transfer: this design has no cached "last +// computed result" at all — check()/poll() compute and return live, +// nothing to go stale. +// - c-RCP REQ-WDG-010 (the RC Server's request-reception path shall call +// kick() on every request received) is a cross-cutting dispatch-layer +// integration requirement, not watchdog-module behavior; this file's +// kick_from_request()/Manager::on_request_received() are the hook, and +// rcp/sim.hpp's Simulator::dispatch already wires it in exactly this +// shape (see sim.hpp's own header comment). +// - c-RCP REQ-WDG-012 (destroy(k) is a null-safe no-op, otherwise closes +// then frees) does not transfer: no manual destroy exists here — +// StreamWatchdog/Manager are plain value types with ordinary C++ +// destructors, RAII covers this by construction. +// - c-RCP's [c-RCP-17] fixed-capacity conversion of its stream table and +// callback list (`RCP_WATCHDOG_MAX_STREAMS`/`RCP_WATCHDOG_MAX_CALLBACKS`, +// both 16 — watchdog.h) DOES genuinely transfer as a content gap: prior +// to this pass, Manager below stored streams_/callbacks_ in an unbounded +// std::map/std::vector, unlike every other Phase 17 table in this +// codebase (rcp/loan.hpp's kPoolMaxEntries, rcp/respqueue.hpp's +// kMaxEntries, rcp/request.hpp's kMaxTrackedRequests) — exactly the "no +// fixed-capacity/no-dynamic-allocation architecture" gap ROADMAP.md's +// Phase 17 introduction cites as one of the reasons for this whole +// rewrite. Manager::kMaxStreams/kMaxCallbacks below port c-RCP's own +// RCP_WATCHDOG_MAX_STREAMS/MAX_CALLBACKS value (16) unchanged, matching +// loan.hpp's own "match c-RCP's chosen bound exactly, not inventing a +// stricter or looser one" precedent — see Manager's own doc comment. +// This is an engineering hardening tracked under issue #129, not a new +// numbered REQ-WDG-* behavior (c-RCP's own bound isn't tied to a +// REQ-WDG-* id in its catalog either — see [c-RCP-17]'s own citation +// style), so no new fusa:req/fusa:test tag is added for it, matching +// loan.hpp's and respqueue.hpp's own fixed-capacity tests (untagged +// Catch2 [watchdog] tests only). + // Per-request-stream watchdog driver — the embedding-application wiring // layer that turns "any inbound request accepted on a stream" into the // OPEN Alliance TC18 Remote Control Protocol Specification v0.5.1_RC's @@ -55,14 +135,13 @@ #include #include +#include #include #include #include -#include #include #include #include -#include namespace rcp { namespace watchdog { @@ -70,7 +149,13 @@ namespace watchdog { // ── Errors ──────────────────────────────────────────────────────────────────── enum class WatchdogErrc : int { - stream_not_registered = 1, // target stream key was never passed to Manager::register_stream + stream_not_registered = 1, // target stream key was never passed to Manager::register_stream + // [Phase 17 c-RCP-reference pass, cpp-RCP issue #129] Ported from + // c-RCP's RCP_WATCHDOG_MAX_STREAMS/MAX_CALLBACKS fixed-capacity bound + // (watchdog.h) — see Manager::kMaxStreams/kMaxCallbacks's own doc + // comment below. + stream_capacity_exceeded = 2, // Manager already tracks kMaxStreams distinct streams + callback_capacity_exceeded = 3, // Manager already holds kMaxCallbacks subscribers }; inline const std::error_category& watchdog_category() noexcept { @@ -80,6 +165,10 @@ inline const std::error_category& watchdog_category() noexcept { switch (static_cast(ev)) { case WatchdogErrc::stream_not_registered: return "rcp/watchdog: stream key was not registered with this Manager"; + case WatchdogErrc::stream_capacity_exceeded: + return "rcp/watchdog: Manager already tracks its fixed maximum number of streams"; + case WatchdogErrc::callback_capacity_exceeded: + return "rcp/watchdog: Manager already holds its fixed maximum number of subscribers"; default: return "rcp/watchdog: unknown error"; } @@ -178,28 +267,65 @@ class StreamWatchdog { // timeout detection itself. class Manager { public: - using HealthCallback = std::function; + // [Phase 17 c-RCP-reference pass, cpp-RCP issue #129] Ported from + // c-RCP's include/rcp/watchdog.h RCP_WATCHDOG_MAX_STREAMS/ + // RCP_WATCHDOG_MAX_CALLBACKS (both 16) — c-RCP's own doc comment there + // rationalizes 16 as "matches e2e.h's own + // RCP_E2E_STREAM_FAULT_TRACKER_MAX_STREAMS precedent... rather than + // inventing an unrelated number"; ported unchanged here for the same + // reason rcp/loan.hpp's kPoolMaxEntries matches c-RCP's own + // RCP_LOAN_POOL_MAX_ENTRIES exactly rather than this port choosing a + // stricter or looser bound of its own. Backs the fixed std::array + // storage below (streams_keys_/streams_, callbacks_), not + // std::map/std::vector growable without bound — this class previously + // had no capacity ceiling at all, unlike every other Phase 17 table in + // this codebase (see this file's own header comment). + static constexpr size_t kMaxStreams = 16; + static constexpr size_t kMaxCallbacks = 16; - // register_stream begins tracking a stream; a harmless no-op if - // stream_key is already registered (existing watchdog state, including - // any latched safe state, is left untouched). - void register_stream(uint64_t stream_key) { streams_.try_emplace(stream_key); } + using HealthCallback = std::function; - // unregister_stream stops tracking a stream and discards its state. - void unregister_stream(uint64_t stream_key) noexcept { streams_.erase(stream_key); } + // register_stream begins tracking a stream; a harmless no-op (returns + // no error) if stream_key is already registered (existing watchdog + // state, including any latched safe state, is left untouched). Returns + // stream_capacity_exceeded, unchanged, once kMaxStreams distinct + // streams are already tracked — see kMaxStreams's own doc comment + // above; the fixed-capacity table is not silently grown past it, + // matching c-RCP's own rcp_watchdog_keeper_new()/RCP_WATCHDOG_MAX_STREAMS + // "rejected, not silently truncated" contract. + std::error_code register_stream(uint64_t stream_key) { + if (find_index(stream_key) != kNotFound) return {}; + if (streams_len_ >= kMaxStreams) return make_error_code(WatchdogErrc::stream_capacity_exceeded); + streams_keys_[streams_len_] = stream_key; + streams_[streams_len_] = StreamWatchdog{}; + ++streams_len_; + return {}; + } - bool is_registered(uint64_t stream_key) const noexcept { - return streams_.find(stream_key) != streams_.end(); + // unregister_stream stops tracking a stream and discards its state — + // swap-with-last removal (same O(1) technique rcp/loan.hpp's + // BufferPool::loan() free-list release already uses): the vacated slot + // is filled from the table's last live entry, so streams_len_ shrinks + // by exactly one with no gap. A harmless no-op for an unregistered key. + void unregister_stream(uint64_t stream_key) noexcept { + size_t idx = find_index(stream_key); + if (idx == kNotFound) return; + size_t last = streams_len_ - 1; + streams_keys_[idx] = streams_keys_[last]; + streams_[idx] = std::move(streams_[last]); + --streams_len_; } + bool is_registered(uint64_t stream_key) const noexcept { return find_index(stream_key) != kNotFound; } + // on_request_received is the driver hook the embedding transport calls // once per accepted inbound request — see StreamWatchdog:: // kick_from_request. Returns stream_not_registered, unchanged, for a // key never passed to register_stream. std::error_code on_request_received(uint64_t stream_key, uint64_t now_ms) noexcept { - auto it = streams_.find(stream_key); - if (it == streams_.end()) return make_error_code(WatchdogErrc::stream_not_registered); - it->second.kick_from_request(now_ms); + size_t idx = find_index(stream_key); + if (idx == kNotFound) return make_error_code(WatchdogErrc::stream_not_registered); + streams_[idx].kick_from_request(now_ms); return {}; } @@ -210,34 +336,60 @@ class Manager { // unaffected by that failure. std::error_code poll(uint64_t stream_key, const regmap::RequestStreamConfig& cfg, request::RequestLedger& ledger, uint64_t now_ms) { - auto it = streams_.find(stream_key); - if (it == streams_.end()) return make_error_code(WatchdogErrc::stream_not_registered); - auto ev = it->second.check(stream_key, cfg, ledger, now_ms); + size_t idx = find_index(stream_key); + if (idx == kNotFound) return make_error_code(WatchdogErrc::stream_not_registered); + auto ev = streams_[idx].check(stream_key, cfg, ledger, now_ms); if (ev.has_value()) { - for (auto& cb : callbacks_) cb(*ev); + for (size_t i = 0; i < callbacks_len_; ++i) callbacks_[i](*ev); } return {}; } bool in_safe_state(uint64_t stream_key) const noexcept { - auto it = streams_.find(stream_key); - return it != streams_.end() && it->second.in_safe_state(); + size_t idx = find_index(stream_key); + return idx != kNotFound && streams_[idx].in_safe_state(); } std::error_code clear_safe_state(uint64_t stream_key) noexcept { - auto it = streams_.find(stream_key); - if (it == streams_.end()) return make_error_code(WatchdogErrc::stream_not_registered); - it->second.clear_safe_state(); + size_t idx = find_index(stream_key); + if (idx == kNotFound) return make_error_code(WatchdogErrc::stream_not_registered); + streams_[idx].clear_safe_state(); return {}; } // subscribe registers a callback fired, in registration order, on - // every HealthEvent poll() produces for any registered stream. - void subscribe(HealthCallback cb) { callbacks_.push_back(std::move(cb)); } + // every HealthEvent poll() produces for any registered stream. Returns + // callback_capacity_exceeded, unchanged, once kMaxCallbacks subscribers + // are already registered — c-RCP's own rcp_watchdog_keeper_subscribe() + // has the identical bound and the identical "rejected, not silently + // grown" contract (RCP_WATCHDOG_MAX_CALLBACKS, watchdog.h/.c). + std::error_code subscribe(HealthCallback cb) { + if (callbacks_len_ >= kMaxCallbacks) return make_error_code(WatchdogErrc::callback_capacity_exceeded); + callbacks_[callbacks_len_] = std::move(cb); + ++callbacks_len_; + return {}; + } + + // Introspection for tests, not part of the register/subscribe contract + // itself — always <= kMaxStreams/kMaxCallbacks, by construction. + size_t stream_count() const noexcept { return streams_len_; } + size_t callback_count() const noexcept { return callbacks_len_; } private: - std::map streams_; - std::vector callbacks_; + static constexpr size_t kNotFound = static_cast(-1); + + size_t find_index(uint64_t stream_key) const noexcept { + for (size_t i = 0; i < streams_len_; ++i) + if (streams_keys_[i] == stream_key) return i; + return kNotFound; + } + + std::array streams_keys_{}; + std::array streams_{}; + size_t streams_len_ = 0; // always <= kMaxStreams + + std::array callbacks_{}; + size_t callbacks_len_ = 0; // always <= kMaxCallbacks }; } // namespace watchdog diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index b394e36..e574fb6 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -118,6 +118,10 @@ add_executable(test_shmem test_shmem.cpp) target_link_libraries(test_shmem PRIVATE rcp::rcp Catch2::Catch2WithMain) add_test(NAME rcp_shmem COMMAND test_shmem) +add_executable(test_alloc test_alloc.cpp) +target_link_libraries(test_alloc PRIVATE rcp::rcp Catch2::Catch2WithMain) +add_test(NAME rcp_alloc COMMAND test_alloc) + # ── Milestones 19–40 ────────────────────────────────────────────────────────── add_executable(test_authz test_authz.cpp) diff --git a/tests/test_alloc.cpp b/tests/test_alloc.cpp new file mode 100644 index 0000000..0da1b30 --- /dev/null +++ b/tests/test_alloc.cpp @@ -0,0 +1,170 @@ +// Tests for rcp/alloc.hpp's FaultInjector — the deterministic +// allocation-failure fault-injection seam (Phase 17, cpp-RCP issue #129). +// See rcp/alloc.hpp's own header comment for the design rationale and the +// REQ-ALLOC-* transfer audit against c-RCP's alloc.h/alloc.c. + +#include + +#include "rcp/alloc.hpp" +#include "rcp/loan.hpp" + +using rcp::alloc::AllocErrc; +using rcp::alloc::FaultInjector; + +// ── FaultInjector — disarmed by default ────────────────────────────────────── + +TEST_CASE("FaultInjector::should_fail always returns false with nothing armed", "[alloc]") { + FaultInjector fi; + REQUIRE_FALSE(fi.armed()); + REQUIRE(fi.remaining() == 0); + REQUIRE_FALSE(fi.should_fail()); + REQUIRE_FALSE(fi.should_fail()); +} + +// ── arm(count) — fires exactly `count` times, then reverts to disarmed ────── + +TEST_CASE("FaultInjector::arm(1) fires exactly once, then reverts to disarmed", "[alloc]") { + FaultInjector fi; + fi.arm(); + REQUIRE(fi.armed()); + REQUIRE(fi.remaining() == 1); + + REQUIRE(fi.should_fail()); + REQUIRE_FALSE(fi.armed()); + REQUIRE(fi.remaining() == 0); + REQUIRE_FALSE(fi.should_fail()); // consumed; back to passthrough +} + +TEST_CASE("FaultInjector::arm(N) fires exactly N times", "[alloc]") { + FaultInjector fi; + fi.arm(3); + + REQUIRE(fi.should_fail()); + REQUIRE(fi.remaining() == 2); + REQUIRE(fi.should_fail()); + REQUIRE(fi.remaining() == 1); + REQUIRE(fi.should_fail()); + REQUIRE(fi.remaining() == 0); + + REQUIRE_FALSE(fi.should_fail()); // exhausted + REQUIRE_FALSE(fi.armed()); +} + +// ── arm(-1) — fires indefinitely until disarm() ────────────────────────────── + +TEST_CASE("FaultInjector::arm(-1) fires indefinitely until disarm()", "[alloc]") { + FaultInjector fi; + fi.arm(-1); + REQUIRE(fi.remaining() == -1); + + for (int i = 0; i < 25; ++i) { + REQUIRE(fi.should_fail()); + REQUIRE(fi.remaining() == -1); // never decrements while armed forever + } + + fi.disarm(); + REQUIRE_FALSE(fi.armed()); + REQUIRE_FALSE(fi.should_fail()); +} + +// ── disarm() / re-arm() ─────────────────────────────────────────────────────── + +TEST_CASE("FaultInjector::disarm before any should_fail call cancels a pending arm", "[alloc]") { + FaultInjector fi; + fi.arm(5); + fi.disarm(); + REQUIRE_FALSE(fi.should_fail()); +} + +TEST_CASE("FaultInjector can be re-armed after exhausting a prior arm", "[alloc]") { + FaultInjector fi; + fi.arm(1); + REQUIRE(fi.should_fail()); + REQUIRE_FALSE(fi.should_fail()); + + fi.arm(2); + REQUIRE(fi.should_fail()); + REQUIRE(fi.should_fail()); + REQUIRE_FALSE(fi.should_fail()); +} + +TEST_CASE("FaultInjector::arm(0) is equivalent to disarm", "[alloc]") { + FaultInjector fi; + fi.arm(4); + fi.arm(0); + REQUIRE_FALSE(fi.armed()); + REQUIRE_FALSE(fi.should_fail()); +} + +// ── Error code plumbing ─────────────────────────────────────────────────────── + +TEST_CASE("AllocErrc::simulated_allocation_failure round-trips through std::error_code", "[alloc]") { + std::error_code ec = rcp::alloc::make_error_code(AllocErrc::simulated_allocation_failure); + REQUIRE(ec == AllocErrc::simulated_allocation_failure); + REQUIRE(static_cast(ec)); + REQUIRE(std::string(ec.category().name()) == "rcp.alloc"); + REQUIRE_FALSE(ec.message().empty()); +} + +// ── Integration: rcp/loan.hpp's BufferPool cache-miss allocation ──────────── +// The one genuine (non-bounded-capacity) allocation call site this pass +// wired the seam into — see rcp/loan.hpp's own header comment. + +TEST_CASE("BufferPool without a FaultInjector allocates normally (backward compatible)", "[alloc][loan]") { + rcp::loan::BufferPool pool; // default construction, unaffected by this pass + std::unique_ptr l; + REQUIRE_FALSE(pool.loan(64, l)); + REQUIRE(l != nullptr); +} + +TEST_CASE("An armed FaultInjector makes BufferPool::loan's cache-miss allocation fail", "[alloc][loan]") { + FaultInjector fi; + rcp::loan::BufferPool pool(&fi); + + fi.arm(1); + std::unique_ptr l; + std::error_code ec = pool.loan(64, l); + REQUIRE(ec == AllocErrc::simulated_allocation_failure); + REQUIRE(l == nullptr); + REQUIRE(pool.pooled_count() == 0); // nothing was taken from (or added to) the free list +} + +TEST_CASE("After a simulated failure is consumed, the next BufferPool::loan call succeeds normally", + "[alloc][loan]") { + FaultInjector fi; + rcp::loan::BufferPool pool(&fi); + + fi.arm(1); + std::unique_ptr failed; + REQUIRE(pool.loan(64, failed) == AllocErrc::simulated_allocation_failure); + + std::unique_ptr ok; + REQUIRE_FALSE(pool.loan(64, ok)); // fault was one-shot; this attempt allocates for real + REQUIRE(ok != nullptr); +} + +TEST_CASE("A FaultInjector armed on a BufferPool does not affect a free-list cache hit", "[alloc][loan]") { + FaultInjector fi; + rcp::loan::BufferPool pool(&fi); + + std::unique_ptr first; + REQUIRE_FALSE(pool.loan(64, first)); // real allocation, injector not yet armed + first.reset(); // returned to the pool's free list + REQUIRE(pool.pooled_count() == 1); + + // Arm the injector *after* a reusable buffer is already pooled: the + // cache-hit branch never reaches the allocation call site at all, so + // an armed-but-unconsumed injector must not block a hit. + fi.arm(1); + std::unique_ptr second; + REQUIRE_FALSE(pool.loan(64, second)); // served from the free list, not a new allocation + REQUIRE(second != nullptr); + REQUIRE(fi.armed()); // the arm was never consumed -- confirms it was never consulted +} + +TEST_CASE("nullptr FaultInjector (the default) never blocks BufferPool allocation", "[alloc][loan]") { + rcp::loan::BufferPool pool(static_cast(nullptr)); + std::unique_ptr l; + REQUIRE_FALSE(pool.loan(8, l)); + REQUIRE(l != nullptr); +} diff --git a/tests/test_watchdog.cpp b/tests/test_watchdog.cpp index 126d728..91828a8 100644 --- a/tests/test_watchdog.cpp +++ b/tests/test_watchdog.cpp @@ -264,3 +264,71 @@ TEST_CASE("Manager::poll invokes every subscribed callback in registration order REQUIRE(order == std::vector{1, 2}); } + +// ── [Phase 17 c-RCP-reference pass, cpp-RCP issue #129] Fixed-capacity ────── +// Manager::streams_/callbacks_ (ported from c-RCP's watchdog.h +// RCP_WATCHDOG_MAX_STREAMS/RCP_WATCHDOG_MAX_CALLBACKS, both 16 — see +// Manager's own doc comment). Untagged, matching rcp/loan.hpp's/ +// rcp/respqueue.hpp's own fixed-capacity conversion tests: this bound is an +// engineering hardening tracked under issue #129, not itself a numbered +// REQ-WDG-* requirement in either c-RCP's or this project's own catalog. + +TEST_CASE("Manager::register_stream succeeds up to kMaxStreams, then rejects further streams", "[watchdog]") { + Manager mgr; + for (uint64_t i = 0; i < Manager::kMaxStreams; ++i) { + REQUIRE_FALSE(mgr.register_stream(i + 1)); + } + REQUIRE(mgr.stream_count() == Manager::kMaxStreams); + + // One more, at capacity: rejected, not silently grown. + REQUIRE(mgr.register_stream(Manager::kMaxStreams + 1) == + make_error_code(WatchdogErrc::stream_capacity_exceeded)); + REQUIRE(mgr.stream_count() == Manager::kMaxStreams); + REQUIRE_FALSE(mgr.is_registered(Manager::kMaxStreams + 1)); + + // Every stream registered before capacity was reached remains reachable + // — confirms the fixed array was fully populated, not silently + // truncated below capacity. + REQUIRE(mgr.is_registered(Manager::kMaxStreams)); +} + +TEST_CASE("Manager::register_stream is a harmless no-op for an already-registered stream, even at capacity", + "[watchdog]") { + Manager mgr; + for (uint64_t i = 0; i < Manager::kMaxStreams; ++i) { + REQUIRE_FALSE(mgr.register_stream(i + 1)); + } + // Re-registering an existing key never consults the capacity check. + REQUIRE_FALSE(mgr.register_stream(1)); + REQUIRE(mgr.stream_count() == Manager::kMaxStreams); +} + +TEST_CASE("Manager::unregister_stream frees a slot for a subsequent register_stream at capacity", + "[watchdog]") { + Manager mgr; + for (uint64_t i = 0; i < Manager::kMaxStreams; ++i) { + REQUIRE_FALSE(mgr.register_stream(i + 1)); + } + REQUIRE(mgr.register_stream(999) == make_error_code(WatchdogErrc::stream_capacity_exceeded)); + + mgr.unregister_stream(1); + REQUIRE(mgr.stream_count() == Manager::kMaxStreams - 1); + REQUIRE_FALSE(mgr.is_registered(1)); + + REQUIRE_FALSE(mgr.register_stream(999)); // room again + REQUIRE(mgr.is_registered(999)); + REQUIRE(mgr.stream_count() == Manager::kMaxStreams); +} + +TEST_CASE("Manager::subscribe succeeds up to kMaxCallbacks, then rejects further subscribers", "[watchdog]") { + Manager mgr; + for (size_t i = 0; i < Manager::kMaxCallbacks; ++i) { + REQUIRE_FALSE(mgr.subscribe([](const HealthEvent&) {})); + } + REQUIRE(mgr.callback_count() == Manager::kMaxCallbacks); + + // One more, at capacity: rejected, not silently grown. + REQUIRE(mgr.subscribe([](const HealthEvent&) {}) == + make_error_code(WatchdogErrc::callback_capacity_exceeded)); + REQUIRE(mgr.callback_count() == Manager::kMaxCallbacks); +}