fix(redundancy): make promote() CAS-based to close a concurrent-failover race - #175
Merged
Merged
Conversation
…ver 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>
17 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Bug (HIGH severity, cpp-RCP v3.0.0 deep audit finding, batch 2)
include/rcp/redundancy.hpp'sRedundantRequestFn::send()readsactive_under a short lock, calls theRequestFnpointer outside the lock, and onErrClosed/ErrTimeoutcallspromote().promote()did a blind, unconditional toggle:The toggle direction is computed from whatever
active_happens to be at the momentpromote()runs, not from what the calling thread actually observed failing. Twosend()calls that both start on the primary, both fail concurrently, and both callpromote()apply the toggle twice (serialized by the internal mutex): thread A flips primary→standby, thread B'spromote()— blind to what B actually saw fail — flips it right back standby→primary.Net effect: zero net toggles,
active_left pointing at the confirmed-bad primary for every subsequent caller — silently defeating the entire hot-standby failover mechanism, and violating the project's own accepted requirement REQ-RED-006 in.fusa-reqs.json:Fix
Added a private, CAS-style
promote_from(RequestFn* observed_active):send()now captures the pointer it actually read under its lock (already done, just wasn't being used) and passes it topromote_from(active)on the auto-promote path, instead of calling the no-argpromote(). Ifactive_has already moved on by the time a racing caller'spromote_from()acquires the lock, it's a no-op instead of a re-toggle.The public
promote()is untouched — it's still an unconditional manual toggle, preservingREQ-RED-004("promote() shall switch the active RequestFn between primary and standby") and all existing manual-promote()test behavior exactly as-is. This is a targeted fix to the auto-promote path insidesend(), not a redesign of the public API.No other files or unrelated code touched.
Test
tests/test_redundancy.cppgains one new[thread]-tagged case (taggedREQ-RED-006per the existing// fusa:test REQ-IDconvention):send()against a primaryRequestFnthat always fails, using a two-phase handshake (entered_cv/release_cvmutex+condition_variable gate) — the same deterministic-concurrency pattern already established in this codebase (seetests/test_shmem.cpp's "admits up to queue_capacity concurrent callers" case).active_ == &primary_undersend()'s lock before any of them can reachpromote_from()— making the race deterministic rather than relying on OS scheduling luck.promote_from().active_ends up on the standby, never reverted to primary.mock::Server), so the test doesn't introduce an unrelated race of its own from concurrentmock::Server::dispatch()calls, which isn't documented as thread-safe.Verification
rm -rf build && cmake -S . -B build -DRCP_BUILD_TESTS=ON -DCMAKE_BUILD_TYPE=Debug && cmake --build build -j— 0 errors, 0 warnings under-Wall -Wextra -Wpedantic.ctest --output-on-failure— 100% pass, 58/58 test binaries, including the new case.--rng-seed time, 30/30 passed (20 before the mutation check + 30 after reapplying the fix).promote_from(active)call back to the old blindpromote()toggle, rebuilt clean (still 0 warnings), and ran the new test standalone 20x — 20/20 reliably FAILED with:Closes a finding from the cpp-RCP v3.0.0 deep audit.