[importcpp]

Fixes #3787

Problem

Promotion and changing an existing upstream synchronously stop and join the replication thread. Today an idle event loop observes the stop flag only at its next 100 ms timer tick. That timer-phase wait unnecessarily extends the admin command and its exclusive command-guard hold time.

Change

• Add an explicit StopMode::kWakeEventLoop, used only by Server::AddMaster and Server::RemoveMaster when they stop an existing replication thread.
• Activate the existing timer with event_active; keep TimerCB responsible for cleanup on the replication thread.
• Protect the published timer pointer with a short mutex. Publication observes an earlier stop request, withdrawal precedes event/base destruction, and join never holds this mutex.
• Publish the stop mode and steady-clock timestamp before the atomic stop flag. Log the Stop-request-to-TimerCB interval, cleanup time, and total Stop time in microseconds. The cleanup interval includes the preceding stop-observation log, not only resource release.
• Keep no-argument Stop() on the timer path, including Server::Stop, which can be called by the existing signal handler.

No timer interval change, new polling loop, replication catch-up shortcut, early command-lock release, Controller change, or metadata-persistence optimization is included. Active wakeup removes an idle-loop wait; it cannot preempt a running callback, a blocking connect, full sync, or a RocksDB write. This patch does not redesign signal shutdown or command-lock ordering.

Regression Coverage

Four C++ tests exercise production Stop/TimerCB/publication logic with a controlled event loop: idle wakeup, stop before publication, no-argument Stop retaining the timer path, and a timer-expiry/wakeup race repeated 100 times. They check thread join and event/base withdrawal. The idle test uses a five-second fallback timer and a one-second latency bound; the early-publication case also checks that the event is active before dispatch starts. Test runners impose outer timeouts because a broken join cannot be bounded by an assertion in the same thread.

The Go integration test performs 10 promotion rounds with three nodes. Each round writes 128 unique values, waits for the target offset, promotes the target, reverses the old primary, changes a third replica's existing upstream, and verifies all accumulated values on all nodes. It waits for the old feed connection to be reaped before reattaching the old primary.

Reproduction and Results

Ubuntu 24.04.4 LTS / Linux 6.8.0-100-generic aarch64 VM, 8 vCPUs, 12 GiB RAM, Clang 18.1.3, Release, jemalloc disabled, redis-cli 7.0.15. Thirty promotions per build, using the same script in sequential runs with no background workload or Controller:

Build	Minimum	Median	Maximum
Unmodified 885ace6d0477cf41bc2a5c34df79940cdb9c7049	6.949 ms	61.088 ms	113.640 ms
Same base with this patch	1.337 ms	2.295 ms	4.086 ms

Both runs completed all 30 promotions with the expected role and marker value.

See the linked issue for the standalone two-node Python/redis-cli reproducer. The measurements include client process startup and connection setup; they are not a guaranteed failover bound or a GET/SET error-rate improvement claim.

Validation

Local configuration	Stop tests, 4 tests x 10 repetitions	Related C++ tests	Replication integration tests
macOS arm64, Release	Pass	22 passed	7 passed
Ubuntu 24.04 arm64, Clang 18, Release	Pass	22 passed	7 passed
Ubuntu 24.04 arm64, Clang 18, ASan + LSan	Pass	22 passed	7 passed
Ubuntu 24.04 arm64, Clang 18, TSan, LuaJIT ON	Pass	Failed during Lua initialization	Not run
Ubuntu 24.04 arm64, Clang 18, TSan, LuaJIT OFF	Pass	22 passed	7 passed

The first ASan integration run failed the new test's five-second wait for the old primary's feed connection to be reaped. The promotion had already completed, with Stop logged at 364 us, and there was no sanitizer report. After allowing ten seconds for that asynchronous, ping-driven connection cleanup, all seven selected integration tests passed. This changes a test wait only, not a production timeout. The initial failure is not counted as a pass.

Formatting (x.py check format, clang-format 18) and x.py check golangci-lint passed. Changed-line clang-tidy 21 passed, as did full-file tidy of the new C++ test. Full-file tidy of the production translation units still reports three existing diagnostics in untouched code: mask/omask initialization and vector capacity in fetchFiles; these are not changed by this patch.

Linux sanitizer builds use -DDISABLE_JEMALLOC=ON, -DCMAKE_BUILD_TYPE=RelWithDebInfo, and -O1 -gline-tables-only -DNDEBUG for both C and C++, with either -DENABLE_ASAN=ON or -DENABLE_TSAN=ON. The second TSan configuration additionally uses -DENABLE_LUAJIT=OFF; Release and ASan retain LuaJIT. Both kvrocks and unittest are built. The Go test executable was cross-compiled with Go 1.26.3 (GOOS=linux GOARCH=arm64 CGO_ENABLED=0) and executed inside the Linux VM; the server binaries are native Linux builds.

TSan/LuaJIT limitation: On this ARM64 VM, the full focused selection with LuaJIT enabled crashed in lua_pushcclosure -> lua::LoadLibraries -> lua::CreateState while constructing ServerWaitTest.EmptyMap, after the four new tests. The original 18 Cluster/ServerWait tests, without the new suite, passed 20 repetitions on both the unmodified base and patched binaries. A separate LuaJIT+TSan-only process returned NULL from luaL_newstate() in 4/50 fresh processes; the same source without TSan returned no NULLs in 50 runs. This reproduces an initialization failure without Kvrocks, Libevent, or this patch, but does not establish the exact cause of the full-suite crash or rule out test-order effects. The LuaJIT-enabled TSan matrix remains failed; the LuaJIT-disabled result is separate, not a fix for the original crash. No Lua changes or new suppression rules are included.

Independent LuaJIT initialization probe

LuaJIT revision: 1968b356c2ecbe729e3998e063b9813732644966, as pinned by this Kvrocks base. The probe links the same static library used by the initial LuaJIT-enabled TSan build.

#include <cstdio>
#include <cstdlib>
#include <thread>

#include <lua.hpp>

int main(int argc, char **argv) {
  const int count = argc > 1 ? std::atoi(argv[1]) : 0;
  for (int i = 0; i < count; ++i) std::thread([] {}).join();
  auto *state = luaL_newstate();
  std::printf("threads=%d, luaL_newstate=%p\n", count, static_cast<void *>(state));
  if (!state) return 2;
  lua_close(state);
}

Using the dependency directories from a LuaJIT-enabled TSan build:

BUILD_LUAJIT="$PWD/build-tsan-luajit"
clang++-18 -O1 -g -fsanitize=thread -pthread \
  -I"$BUILD_LUAJIT/_deps/luajit-build/include" luajit-smoke.cc \
  "$BUILD_LUAJIT/_deps/luajit-src/src/libluajit.a" -ldl -o luajit-smoke
for i in $(seq 1 50); do ./luajit-smoke 0 || echo "iteration=$i exit=$?"; done

Observed: NULL at iterations 3, 9, 42, and 45 (exit 2). Zero user-created threads in this run. Removing -fsanitize=thread and repeating produced 0/50 NULL results. These are local observations, not a portable failure-rate estimate.

Sanitizer runs use the repository's existing tests/lsan-suppressions and tests/tsan-suppressions without adding suppression rules. TSan's existing signal-handler suppression means these runs do not validate signal safety. They also do not prove that all possible thread interleavings are race-free.

Reproduce the focused test selection after building each configuration (set BUILD to that build directory):

ROOT="$PWD"
BUILD="$ROOT/build"
export LSAN_OPTIONS="suppressions=$ROOT/tests/lsan-suppressions"
export TSAN_OPTIONS="suppressions=$ROOT/tests/tsan-suppressions:halt_on_error=1"
export ASAN_OPTIONS="detect_leaks=1:halt_on_error=1:abort_on_error=1"
cd "$BUILD"
timeout 180 ./unittest --gtest_filter='ReplicationThreadTest.*' --gtest_repeat=10
timeout 180 ./unittest --gtest_filter='ReplicationThreadTest.*:*Cluster*:*ServerWait*'
cd "$ROOT/tests/gocase"
go test ./integration/replication -count=1 -timeout=540s -parallel=2 \
  -run '^(TestRepeatedPromotionAndUpstreamChange|TestClusterReplication|TestReplicationBasics|TestFullSyncReplication|TestReplicationLoading|TestReplicationContinueRunning|TestReplicationWithMultiSlaves)$' \
  -binPath="$BUILD/kvrocks" -workspace="$(mktemp -d)"

Review Focus and AI Assistance

OpenAI Codex assisted with diagnosis, implementation, regression tests, local validation, and drafting this PR. A separate Codex chat reviewed the code and this description; this is not a human maintainer review. Please pay particular attention to event lifetime, stop-before-publication, lock ordering around join, and preserving the default shutdown path.

The local focused matrix is not a substitute for the upstream full CI matrix and maintainer review.

[github-actions]

Hi @importcpp,

Thank you for your pull request. Please review our Contributing Guide.

Please make sure you understand your changes and explain your reasoning in this pull request. Low-quality pull requests may be closed.