From f9ef82a822c510723a2e880d9e17c7b519b4b68a Mon Sep 17 00:00:00 2001 From: Matt <47545907+SoundMatt@users.noreply.github.com> Date: Sat, 22 Aug 2026 16:16:40 -0700 Subject: [PATCH] fix(redundancy): make promote() CAS-based to close a concurrent-failover race Bug: RedundantRequestFn::send() reads active_ under a short lock, calls the RequestFn pointer outside the lock, and on ErrClosed/ErrTimeout calls promote() to fail over to the standby. promote() did a blind, unconditional toggle (active_ = active_==&primary_ ? &standby_ : &primary_), computed from whatever active_ happens to be *at the moment promote() runs* rather than from what the calling thread actually observed failing. Two send() calls that both start on the primary, both fail concurrently, and both call promote() therefore apply the toggle TWICE (serialized by the internal mutex): the first flips primary->standby, the second -- blind to what its caller actually saw fail -- flips it straight back standby->primary. Net effect: zero net toggles, active_ left pointing at the confirmed-bad primary for every subsequent caller, silently defeating the hot-standby failover mechanism and violating REQ-RED-006 ('Once RedundantRequestFn has promoted the standby, subsequent send() calls shall continue to be served by the standby without reverting to the primary on their own'). Fix: add a private, CAS-style promote_from(RequestFn* observed_active) that only flips active_ away from the specific pointer the caller observed failing (captured under send()'s lock before the call), and is a no-op if active_ has already moved on. send()'s auto-promote path now calls promote_from(active) instead of the public no-arg promote(). The public promote() itself is left untouched (still an unconditional manual toggle) to preserve REQ-RED-004 and existing manual-promote() API/behavior exactly as-is -- this is a targeted concurrency fix to the auto-promote path, not a redesign of the public surface. Test: tests/test_redundancy.cpp gains a new [thread] case (tagged REQ-RED-006) that drives 8 threads through a two-phase handshake (entered_cv/release_cv, matching this project's existing shmem concurrency-test pattern) so every thread deterministically observes active_ == &primary_ before any of them can reach promote_from(), then releases them all together to force the race, and asserts active_ ends up on the standby, never reverted to primary. Verification: - Full rebuild from scratch (cmake -DRCP_BUILD_TESTS=ON -DCMAKE_BUILD_TYPE=Debug + cmake --build): 0 errors, 0 warnings under -Wall -Wextra -Wpedantic. - Full ctest suite: 100% pass, 58/58 test binaries, including the new case. - New test run standalone 20x with --rng-seed time against the fix: 20/20 pass. - Mutation check: reverted only the promote_from() call back to the old blind promote() toggle, rebuilt clean, ran the new test standalone 20x: 20/20 reliably FAIL with 'REQUIRE_FALSE( rr.is_primary_active() )' == '!true', i.e. the test deterministically catches the exact bug. Reapplied the fix, rebuilt clean, ran the new test standalone 30x: 30/30 pass. Closes a finding from the cpp-RCP v3.0.0 deep audit (batch 2, redundancy/race). Signed-off-by: Matt <47545907+SoundMatt@users.noreply.github.com> --- include/rcp/redundancy.hpp | 27 +++++++++- tests/test_redundancy.cpp | 108 +++++++++++++++++++++++++++++++++++++ 2 files changed, 133 insertions(+), 2 deletions(-) diff --git a/include/rcp/redundancy.hpp b/include/rcp/redundancy.hpp index 4bc261c..4d83b17 100644 --- a/include/rcp/redundancy.hpp +++ b/include/rcp/redundancy.hpp @@ -72,9 +72,16 @@ class RedundantRequestFn { if (!cfg_.auto_promote) return ec; - // Promote standby on retriable failure. + // Promote standby on retriable failure. Pass along the exact + // RequestFn* this call observed active (captured under the lock + // above, before the call): if two send() calls race on the same + // failing active pointer, only the one whose promote_from() runs + // first actually flips active_; the other's observed pointer no + // longer matches active_ by the time it acquires the lock, so it is + // a no-op instead of an unconditional toggle that would flip + // active_ right back (see promote_from()). if (ec == ErrClosed || ec == ErrTimeout) { - promote(); + promote_from(active); for (int i = 0; i < cfg_.max_retries; ++i) { RequestFn* retry_active; { @@ -109,6 +116,22 @@ class RedundantRequestFn { } private: + // promote_from is send()'s internal, CAS-style counterpart to the public + // promote() above: it only flips active_ away from the specific pointer + // the caller observed failing. If active_ has already moved on (e.g. a + // concurrent send() on the same observed-failing pointer promoted first), + // this is a no-op rather than re-toggling — without this guard, two + // send() calls that both observe the primary failing concurrently would + // together apply the toggle twice (primary->standby, then straight back + // standby->primary), silently leaving active_ on the confirmed-bad + // primary for every later caller (REQ-RED-006). + void promote_from(RequestFn* observed_active) { + std::lock_guard lk(mu_); + if (active_ == observed_active) { + active_ = (active_ == &primary_) ? &standby_ : &primary_; + } + } + RequestFn primary_; RequestFn standby_; RequestFn* active_; diff --git a/tests/test_redundancy.cpp b/tests/test_redundancy.cpp index d7cffc0..4ed08ae 100644 --- a/tests/test_redundancy.cpp +++ b/tests/test_redundancy.cpp @@ -20,8 +20,12 @@ #include "rcp/mock.hpp" #include "rcp/redundancy.hpp" +#include #include #include +#include +#include +#include using namespace rcp; using namespace std::chrono_literals; @@ -150,3 +154,107 @@ TEST_CASE("redundancy: RedundantRequestFn is itself usable as an rcp::RequestFn REQUIRE_FALSE(ec); REQUIRE(resp.id == req.id); } + +// ── Concurrency regression: promote() must not be a blind toggle ─────────── +// +// send() reads active_ under a short lock, then calls the RequestFn pointer +// *outside* the lock. Two concurrent send() calls can therefore both observe +// active_ == &primary_, both have the primary fail, and both attempt to +// promote. An unconditional toggle (active_ = active_==&primary_ ? &standby_ +// : &primary_) would apply twice in that case -- the first promote flips +// primary->standby, the second (serialized behind the same mutex, but blind +// to what the caller actually observed) flips it straight back +// standby->primary -- silently reverting to the confirmed-bad primary and +// defeating failover for every later caller. This is a real regression risk +// for REQ-RED-006 ("subsequent send() calls shall continue to be served by +// the standby without reverting to the primary on their own"). +// +// The two send() calls are driven through an explicit two-phase handshake +// (entered_cv / release_cv), following this project's established pattern +// for deterministic concurrency tests (see e.g. shmem's +// "admits up to queue_capacity concurrent callers" case): both threads are +// held inside their (still-primary) RequestFn call -- i.e. both have already +// captured active_ == &primary_ under send()'s lock -- until both have +// entered, and are then released together so their promote attempts +// genuinely race on the *same* observed-primary pointer. This makes the race +// deterministic instead of depending on OS scheduling luck. +TEST_CASE("redundancy: concurrent send() failures on the same observed primary promote " + "exactly once and never revert to primary", + "[redundancy][thread][REQ-RED-006]") { + constexpr int kThreads = 8; + + std::mutex mu; + std::condition_variable entered_cv; + std::condition_variable release_cv; + int entered_count = 0; + bool may_release = false; + + // Always fails (like fail_fn), but first blocks every caller until all + // kThreads callers are simultaneously inside the primary call -- forcing + // every send() to have observed active_ == &primary_ before any of them + // can reach promote_from(). + RequestFn blocking_fail_fn = [&](const Context&, const acf::AcfMessageInfo&, + const std::vector&, acf::AcfMessageInfo&, + std::vector&) { + { + std::lock_guard lk(mu); + ++entered_count; + } + entered_cv.notify_all(); + + std::unique_lock lk(mu); + release_cv.wait(lk, [&] { return may_release; }); + return ErrClosed; + }; + + // The standby is a trivial, stateless always-succeeds fn rather than a + // mock::Server (which is not itself documented/guaranteed thread-safe + // for concurrent dispatch()) -- this test's own retries after promotion + // are deliberately concurrent, and must not introduce a second, unrelated + // race of their own on top of the one under test. + RequestFn always_ok_fn = [](const Context&, const acf::AcfMessageInfo&, + const std::vector&, acf::AcfMessageInfo&, + std::vector&) { return std::error_code{}; }; + + redundancy::RedundantRequestFn rr(blocking_fail_fn, always_ok_fn); + REQUIRE(rr.is_primary_active()); + + auto req = gpio_read_request(); + + std::vector threads; + threads.reserve(kThreads); + for (int i = 0; i < kThreads; ++i) { + threads.emplace_back([&] { + acf::AcfMessageInfo out; + std::vector out_payload; + rr.send(Context{}, req, {}, out, out_payload); + }); + } + + // Wait until every thread's send() is blocked inside the primary call -- + // each has already read active_ == &primary_ under the lock, before any + // of them can call promote_from(). + { + std::unique_lock lk(mu); + entered_cv.wait(lk, [&] { return entered_count == kThreads; }); + } + + // Release them all together: every thread's primary call now returns + // ErrClosed and races to promote_from(observed == &primary_) at + // (approximately) the same time, serialized only by RedundantRequestFn's + // internal mutex. + { + std::lock_guard lk(mu); + may_release = true; + } + release_cv.notify_all(); + + for (auto& th : threads) th.join(); + + // Exactly one net promotion must have occurred: active_ must be the + // standby, never reverted back to the primary that every caller observed + // failing (REQ-RED-006). With the old blind-toggle promote(), an even + // number of concurrent promote attempts on the same observed pointer + // cancel back out to &primary_, and this REQUIRE fails. + REQUIRE_FALSE(rr.is_primary_active()); +}