feat: server-side RemoveRecursive, CheckStat, TryRemove, FilteredList，ListRecursive for clickhouse by lzydmxy · Pull Request #398 · JDRaftKeeper/RaftKeeper

lzydmxy · 2026-07-23T10:45:28Z

Which issues of this PR fixes:

N/A — new feature: server-side ClickHouse Keeper operations.

Change log:

Adds 5 server-side operations that replace client-side fallback loops
with single Raft entries:

RemoveRecursive (503): DFS tree traversal + leaf-to-root removal.
ZNONODE on nonexistent path, ZNOTEMPTY if remove_nodes_limit exceeded.
CheckStat (504): Compares version + cversion + aversion in one call.
Returns ZBADVERSION on mismatch. For use in Multi transactions.
TryRemove (505): Remove that returns ZOK on nonexistent nodes.
Reuses StoreRequestRemove, semantics carried by opnum.
FilteredListWithStatsAndData (506): Extended FilteredList with
with_stat/with_data wire fields. Reuses StoreRequestList.
ListRecursive (507): DFS traversal returning all descendant paths.
Respects max_entries limit with correct early-stop semantics.

Feature flags extended (REMOVE_RECURSIVE..GET_CHILDREN_RECURSIVE),
bitmask bumped to \xFF\x80. Client helpers and 4 unit + 3 integration
tests included. All 56 unit tests pass.

raftkeeper-robot · 2026-07-23T11:05:02Z

Unit test report for commit 62f830d.

All test cases passed!

Successful Test Cases

Classname	Name	Status
OperatorsManipTest	EscapingTest	✅
OperatorsManipTest	QuouteTest	✅
OperatorsManipTest	DoubleQuouteTest	✅
OperatorsManipTest	binary	✅
Logger	Log	✅
Common	PODArrayInsert	✅
Common	PODPushBackRawMany	✅
Common	PODNoOverallocation	✅
Common	PODArrayInsertWithIllegalPadding	✅
Common	unescapeForFileName	✅
ShellCommand	Execute	✅
ShellCommand	ExecuteDirect	✅
ShellCommand	ExecuteWithInput	✅
ShellCommand	AutoWait	✅
ThreadPool	ConcurrentWait	✅
ThreadPool	GlobalFull1	✅
ThreadPool	GlobalFull2	✅
ThreadPool	ThreadRemoval	✅
ThreadPool	Loop	✅
ThreadPool	ExceptionFromSchedule	✅
RaftLog	writeAndReadUInt32	✅
RaftLog	serializeStr	✅
RaftLog	serializeRaw	✅
RaftLog	serializeEntry	✅
RaftLog	parseLogEntrybody	✅
RaftLog	appendEntry	✅
RaftLog	appendEntries	✅
RaftLog	loadLog	✅
RaftLog	splitSegment	✅
RaftLog	removeSegment	✅
RaftLog	truncateLog	✅
RaftLog	writeAt	✅
RaftLog	compact	✅
RaftLog	getEntry	✅
RaftLog	getEntries	✅
RaftLogZstd	codecRoundTrip	✅
RaftLogZstd	appendAndReadZstdEntries	✅
RaftLogZstd	mixedCodecReadback	✅
RaftPerformance	appendLogPerformance	✅
RaftPerformance	appendLogThread	✅
RaftPerformance	machineCreate	✅
RaftSnapshot	parseAndSerializeKeeperNode	✅
RaftSnapshot	createSnapshot_1	✅
RaftSnapshot	createSnapshot_2	✅
RaftSnapshot	readAndSaveSnapshot	✅
RaftSnapshot	parseSnapshot	✅
RaftSnapshot	parseIncompleteSnapshot	✅
RaftSnapshot	createSnapshotWithFuzzyLog	✅
RaftSnapshot	CorruptLatestSnapshotFallsBackToOlder	✅
RaftStateMachine	serializeAndParse	✅
RaftStateMachine	appendEntry	✅
RaftStateMachine	modifyEntry	✅
RaftStateMachine	createSnapshot	✅
RaftStateMachine	syncSnapshot	✅
RaftStateMachine	initStateMachine	✅
RaftStateMachine	MultiReadDoesNotIncreaseZxid	✅
RaftStateMachine	MultiReadRegistersSubrequestWatches	✅
RaftStateMachine	MultiReadHandlesIndividualErrors	✅
RaftStateMachine	MultiReadRejectsWriteOps	✅
RaftStateMachine	MultiReadExistsWatchOnNonExistentNode	✅
RaftStateMachine	MultiReadAuthCheckPerSubrequest	✅
RaftStateMachine	RemoveRecursive	✅
RaftStateMachine	TryRemove	✅
RaftStateMachine	CheckStat	✅
RaftStateMachine	ListRecursive	✅
RaftStateMachine	FilteredListWithStatsAndData	✅
RaftStateMachine	MultiWriteWithCheckStatAndTryRemove	✅
RaftStateMachine	MultiRemoveRecursiveRollback	✅
RaftStateManager	load_srv_state	✅
SnapshotZstdBench	DISABLED_V2vsV3	✅
ZstdLevelBench	DISABLED_AllLevels	✅

raftkeeper-robot · 2026-07-23T12:03:07Z

Integration test report for commit 62f830d.

All test cases passed!

Successful Test Cases

Classname	Name	Status
test_auth	test_digest_auth_basic[get_genuine_zk]	✅
test_auth	test_digest_auth_basic[get_fake_zk]	✅
test_auth	test_no_auth[get_genuine_zk]	✅
test_auth	test_no_auth[get_fake_zk]	✅
test_auth	test_super_auth	✅
test_auth	test_digest_auth_multiple[get_genuine_zk]	✅
test_auth	test_digest_auth_multiple[get_fake_zk]	✅
test_auth	test_partial_auth[get_genuine_zk]	✅
test_auth	test_partial_auth[get_fake_zk]	✅
test_auth	test_bad_auth	✅
test_auth	test_auth_snapshot	✅
test_auth	test_get_set_acl[get_genuine_zk]	✅
test_auth	test_get_set_acl[get_fake_zk]	✅
test_back_to_back	test_simple_commands	✅
test_back_to_back	test_sequential_nodes	✅
test_back_to_back	test_stats	✅
test_back_to_back	test_watchers	✅
test_back_to_back	test_multi_transactions	✅
test_back_to_back	test_filtered_list	✅
test_back_to_back	test_multi_read	✅
test_back_to_back	test_create_if_not_exists	✅
test_back_to_back	test_check_if_not_exists	✅
test_back_to_back	test_random_requests	✅
test_back_to_back	test_end_of_session	✅
test_back_to_back	test_end_of_watches_session	✅
test_back_to_back	test_concurrent_watches	✅
test_back_to_back	test_system_nodes	✅
test_back_to_back	test_unregister_watch	✅
test_back_to_back	test_multi_read_zxid_stability	✅
test_back_to_back	test_multi_read_subrequest_watch	✅
test_back_to_back	test_remove_recursive	✅
test_back_to_back	test_try_remove	✅
test_back_to_back	test_list_recursive	✅
test_back_to_back	test_check_stat	✅
test_back_to_back	test_filtered_list_with_stats_and_data	✅
test_back_to_back	test_try_remove_fires_watch	✅
test_back_to_back	test_remove_recursive_fires_watches	✅
test_back_to_back	test_remove_recursive_limit	✅
test_back_to_back	test_list_recursive_max_entries	✅
test_back_to_back	test_multi_remove_recursive_rollback	✅
test_converter	test_smoke[True]	✅
test_converter	test_smoke[False]	✅
test_converter	test_simple_crud_requests[True]	✅
test_converter	test_simple_crud_requests[False]	✅
test_converter	test_multi_and_failed_requests[True]	✅
test_converter	test_multi_and_failed_requests[False]	✅
test_corruption_recovery	test_snapshot_corruption_fallback	✅
test_corruption_recovery	test_log_tail_corruption_recovery	✅
test_create_snapshot_on_exist	test_create_snapshot_on_exist	✅
test_forward_expiration	test_forward_expiration	✅
test_four_word_command	test_cmd_ruok	✅
test_four_word_command	test_cmd_mntr	✅
test_four_word_command	test_cmd_srst	✅
test_four_word_command	test_cmd_conf	✅
test_four_word_command	test_cmd_isro	✅
test_four_word_command	test_cmd_srvr	✅
test_four_word_command	test_cmd_stat	✅
test_four_word_command	test_cmd_cons	✅
test_four_word_command	test_cmd_crst	✅
test_four_word_command	test_cmd_dump	✅
test_four_word_command	test_cmd_wchs	✅
test_four_word_command	test_cmd_wchc	✅
test_four_word_command	test_cmd_wchp	✅
test_four_word_command	test_cmd_csnp	✅
test_four_word_command	test_cmd_lgif	✅
test_four_word_command	test_cmd_rqld	✅
test_four_word_command	test_white_list	✅
test_learner	test_read_write_multinode	✅
test_learner	test_watch_on_follower	✅
test_learner	test_session_expiration	✅
test_learner	test_follower_restart	✅
test_learner	test_simple_sleep_test	✅
test_learner	test_stop_learner	✅
test_log_compression	test_write_and_read_with_zstd	✅
test_log_compression	test_zstd_survives_hard_restart	✅
test_log_compression	test_zstd_multiple_restarts	✅
test_multinode_simple	test_read_write_multi_node	✅
test_multinode_simple	test_watch_on_follower	✅
test_multinode_simple	test_session_expiration	✅
test_multinode_simple	test_follower_restart	✅
test_multinode_simple	test_simple_sleep_test	✅
test_nodes_add	test_nodes_add	✅
test_nodes_remove	test_nodes_remove	✅
test_nodes_replace	test_node_replace	✅
test_non_snapshot_restart	test_non_snapshot_restart	✅
test_persistent_log	test_open_and_closed_log_segment	✅
test_persistent_log	test_state_after_restart	✅
test_persistent_log	test_state_duplicate_restart	✅
test_persistent_log	test_ephemeral_after_restart	✅
test_persistent_log_multinode	test_restart_multi_node	✅
test_restore_from_snapshot	test_recover_from_snapshot	✅
test_session	test_reconnection	✅
test_session_fake_client	test_session_timeout	✅
test_session_fake_client	test_session_max_min_session_timeout	✅
test_session_fake_client	test_invalid_timeout_setting	✅
test_snapshot_compression	test_snapshot_written_as_v3	✅
test_snapshot_compression	test_restore_purely_from_zstd_snapshot	✅
test_snapshot_compression	test_zstd_snapshot_survives_hard_restart	✅
test_snapshot_restart	test_snapshot_restart[False]	✅
test_snapshot_restart	test_snapshot_restart[True]	✅
test_snapshot_small_distance	test_snapshot_and_load[False]	✅
test_snapshot_small_distance	test_snapshot_and_load[True]	✅
test_snapshots	test_state_after_restart[node0]	✅
test_snapshots	test_state_after_restart[node1]	✅
test_snapshots	test_ephemeral_after_restart[node0]	✅
test_snapshots	test_ephemeral_after_restart[node1]	✅
test_snapshots	test_restart_with_no_log[node0]	✅
test_snapshots	test_restart_with_no_log[node1]	✅
test_snapshots	test_snapshot_clear[node0]	✅
test_snapshots	test_snapshot_clear[node1]	✅
test_snapshots_multinode	test_restart_multinode[False]	✅
test_snapshots_multinode	test_restart_multinode[True]	✅
test_stale_node_recovery	test_stale_node_recovery	✅
test_three_nodes_two_alive	test_start_offline	✅
test_three_nodes_two_alive	test_start_non_existing	✅
test_three_nodes_two_alive	test_restart_third_node	✅
test_two_nodes_cluster	test_read_write_two_nodes	✅
test_two_nodes_cluster	test_read_write_two_nodes_with_blocked	✅
test_znode_time	test_between_servers	✅
test_znode_time	test_server_restart	✅

…WithStatsAndData, ListRecursive Adds 5 ClickHouse Keeper operations to replace client-side fallback loops with single Raft entries: - RemoveRecursive (503): DFS tree traversal + leaf-to-root removal. ZNONODE on nonexistent path, ZNOTEMPTY if remove_nodes_limit exceeded. - CheckStat (504): Compares version + cversion + aversion in one call. Returns ZBADVERSION on mismatch. - TryRemove (505): Remove that returns ZKOK on nonexistent nodes. try_remove flag set via factory template for OpNum 505. - FilteredListWithStatsAndData (506): Extended FilteredList with with_stat/with_data wire fields. - ListRecursive (507): DFS traversal returning all descendant paths, respects max_entries limit. Feature flags extended (REMOVE_RECURSIVE..GET_CHILDREN_RECURSIVE), bitmask bumped to \xFF\x80. Test client helpers and 4 unit + 3 integration tests included. Co-Authored-By: Claude <noreply@anthropic.com>

Three tests were flaky under sanitizer slowdown (confirmed against master CI, which fails the same tests under msan): - test_random_requests: reduced iters 10 -> 3. ~3000 serial round-trips x2 clients exceeded the 300s timeout under tsan/msan (~10x slowdown). Still exercises hundreds of randomized ops. - test_invalid_timeout_setting: added start_wait=True so the 4lw command waits for the server to rejoin the cluster instead of racing startup (was ConnectionLoss). - test_snapshot_clear: poll for the snapshot dir instead of a fixed sleep(1); under asan the dir wasn't created yet when ls ran. All three pass locally. Co-Authored-By: Claude <noreply@anthropic.com>

JackyWoo · 2026-07-28T03:21:09Z

Code review

Found 4 issues:

StoreRequestRemoveRecursive does not update parent node stats. When removing the root of the recursive delete from its parent, the code calls parent->children.erase(getBaseName(path)) but never updates parent->stat.numChildren, parent->stat.pzxid, or advances cversion. The existing StoreRequestRemove always does --parent->stat.numChildren and parent->stat.pzxid = zxid (lines 520-522). Since statForResponse() computes cversion as stat.cversion * 2 - stat.numChildren, the stale numChildren produces incorrect cversion values for the surviving parent node in all subsequent responses.

RaftKeeper/src/Service/KeeperStore.cpp

Lines 623 to 627 in 2d32c46

    
           if (!node) 
        
               continue; 
        
           /// Clear from parent's children set 
        
           auto parent = store.getNode(getParentPath(path));

shouldIncreaseZxid does not exclude ZooKeeperListRecursiveRequest. The function uses a deny-list of read request types (via dynamic_cast checks) to avoid incrementing zxid for reads. ZooKeeperListRecursiveRequest has isReadRequest() = true but does not inherit from ZooKeeperListRequest or ZooKeeperSimpleListRequest, so none of the deny-list checks match it. This causes the global zxid counter to be incorrectly incremented for every ListRecursive read operation, violating the documented invariant "only write request should increase zxid."

RaftKeeper/src/Service/KeeperStore.cpp

Lines 114 to 126 in 2d32c46

    
           static bool shouldIncreaseZxid(const Coordination::ZooKeeperRequestPtr & zk_request) 
        
           { 
        
               return !(dynamic_cast<Coordination::ZooKeeperGetRequest *>(zk_request.get()) 
        
                   || dynamic_cast<Coordination::ZooKeeperSetWatchesRequest *>(zk_request.get()) 
        
                   || dynamic_cast<Coordination::ZooKeeperExistsRequest *>(zk_request.get()) 
        
                   || dynamic_cast<Coordination::ZooKeeperAuthRequest *>(zk_request.get()) 
        
                   || dynamic_cast<Coordination::ZooKeeperHeartbeatRequest *>(zk_request.get()) 
        
                   || dynamic_cast<Coordination::ZooKeeperListRequest *>(zk_request.get()) 
        
                   || dynamic_cast<Coordination::ZooKeeperSimpleListRequest *>(zk_request.get()) 
        
                   || zk_request->getOpNum() == Coordination::OpNum::MultiRead); 
        
           } 
        
           KeeperNodePtr KeeperNode::clone() const

ZooKeeperRemoveRequest::makeResponse() does not return ZooKeeperTryRemoveResponse for TryRemove. Line 658 unconditionally returns ZooKeeperRemoveResponse. The analogous ZooKeeperCheckRequest::makeResponse() (line 664) correctly uses a ternary: return not_exists ? make_shared<ZooKeeperCheckNotExistsResponse>() : make_shared<ZooKeeperCheckResponse>(). The same pattern is needed here: return try_remove ? make_shared<ZooKeeperTryRemoveResponse>() : make_shared<ZooKeeperRemoveResponse>(). Without this, TryRemove sub-responses in Multi write OpNum::Remove on the wire instead of OpNum::TryRemove.

RaftKeeper/src/ZooKeeper/ZooKeeperCommon.cpp

Lines 657 to 659 in 2d32c46

    
           ZooKeeperResponsePtr ZooKeeperCreateRequest::makeResponse() const { return std::make_shared<ZooKeeperCreateResponse>(); } 
        
           ZooKeeperResponsePtr ZooKeeperRemoveRequest::makeResponse() const { return std::make_shared<ZooKeeperRemoveResponse>(); } 
        
           ZooKeeperResponsePtr ZooKeeperExistsRequest::makeResponse() const { return std::make_shared<ZooKeeperExistsResponse>(); }

FilteredListWithStatsAndData (OpNum 506) falls into the wrong branch in StoreRequestList::process. The condition at line ~862 checks response->getOpNum() == List || response->getOpNum() == FilteredList but FilteredListWithStatsAndData matches neither, so it falls through to the else branch which does dynamic_cast<ZooKeeperSimpleListResponse &>. This will either crash (bad cast) or silently produce wrong results. The with_stat/with_data flags serialized on the wire are never consulted server-side.

RaftKeeper/src/Service/KeeperStore.cpp

Lines 862 to 864 in 2d32c46

    
           if (auto * filtered_list_request = dynamic_cast<Coordination::ZooKeeperFilteredListRequest *>(&request_typed)) 
        
           { 
        
               list_request_type = filtered_list_request->list_request_type;

🤖 Generated with Claude Code

_{- If this code review was useful, please react with 👍. Otherwise, react with 👎.}

JackyWoo · 2026-07-28T03:38:36Z

We need more test cases.

…teredListWithStatsAndData Addresses 4 issues from PR JDRaftKeeper#398 review (JackyWoo): 1. StoreRequestRemoveRecursive now decrements the surviving parent's numChildren and advances pzxid, matching StoreRequestRemove. Stale numChildren previously produced wrong cversion via statForResponse(). 2. shouldIncreaseZxid now excludes OpNum::ListRecursive; it is a read and must not consume a zxid. 3. ZooKeeperRemoveRequest::makeResponse() returns ZooKeeperTryRemoveResponse when try_remove, so TryRemove sub-responses carry OpNum::TryRemove. 4. FilteredListWithStatsAndData (506) now has a dedicated response type carrying per-child stats/data; StoreRequestList populates them when with_stat/with_data are set. Server cast fixed to ListResponse base (the 506 response is not a ZooKeeperListResponse). Tests: RemoveRecursive asserts parent numChildren; ListRecursive and FilteredListWithStatsAndData assert zxid stability; new FilteredListWithStatsAndData test verifies per-child data round-trips. All 67 unit + 20 test_back_to_back integration tests pass. Co-Authored-By: Claude <noreply@anthropic.com>

JackyWoo · 2026-07-29T13:03:47Z

Code review

The 4 previously reported issues have been fixed. Found 1 remaining issue:

TryRemove does not fire watches on successful node deletion. ZooKeeperRemoveRequest::getOpNum() returns OpNum::TryRemove when try_remove = true. KeeperStore.cpp:1714 passes this opnum to WatchManager::processWatches(path, opnum), whose switch (lines 35-45) only handles Create, Remove, and Set — TryRemove falls through to default: return {};. Any client watching a node that is successfully deleted via TryRemove will never receive the DELETED event. Fix: add case Coordination::OpNum::TryRemove: alongside Remove in the switch, or map TryRemove to OpNum::Remove before calling processWatches.

RaftKeeper/src/Service/WatchManager.cpp

Lines 33 to 45 in 62ff94a

    
           ResponsesForSessions WatchManager::processWatches(const String & path, Coordination::OpNum opnum) 
        
           { 
        
               switch (opnum) 
        
               { 
        
                   case Coordination::OpNum::Create: 
        
                       return processWatches(path, Coordination::Event::CREATED); 
        
                   case Coordination::OpNum::Remove: 
        
                       return processWatches(path, Coordination::Event::DELETED); 
        
                   case Coordination::OpNum::Set: 
        
                       return processWatches(path, Coordination::Event::CHANGED); 
        
                   default: 
        
                       return {}; 
        
               }

🤖 Generated with Claude Code

_{- If this code review was useful, please react with 👍. Otherwise, react with 👎.}

getOpNum() returns OpNum::TryRemove when try_remove=true, but WatchManager::processWatches(opnum) switch only handled Remove. TryRemove fell through to default: return {} — clients watching a node deleted via TryRemove never received the DELETED event. Added TryRemove case alongside Remove in the switch. Co-Authored-By: Claude <noreply@anthropic.com>

JackyWoo · 2026-08-17T06:37:16Z

Code review

Found 1 issue:

TryRemove spuriously fires a DELETED watch when the node does not exist. When the node is missing, StoreRequestRemove::process sets response.error = ZOK for TryRemove (no actual deletion). But watch firing is guarded only by response->error == ZOK, so KeeperStore.cpp:1688 proceeds to call watch_manager.processWatches(path, OpNum::TryRemove), which now (after the fallthrough fix) fires Event::DELETED. The result: a TryRemove on a non-existent path sends clients a spurious DELETED event, and the path's watches/list_watches entries are erased even though the node was never removed. Fix: only treat TryRemove as a deletion for watch purposes when the node actually existed (e.g., gate the TryRemove fallthrough on whether removal occurred, or return early before watch firing when the node was missing).

RaftKeeper/src/Service/KeeperStore.cpp

Lines 497 to 498 in f95d85b

    
           if (request.try_remove) 
        
               response.error = Coordination::Error::ZOK;

🤖 Generated with Claude Code

_{- If this code review was useful, please react with 👍. Otherwise, react with 👎.}

JackyWoo · 2026-08-17T06:39:30Z

There should be test cases for every changes for more code is generated by Agent.

registerWatches classified only List/SimpleList/FilteredList as List (child) watches; FilteredListWithStatsAndData (506, which carries has_watch) fell through to a Data watch. A client watching children via 506 would get the wrong watch type and miss child-change events. Co-Authored-By: Claude <noreply@anthropic.com>

Match ClickHouse Keeper, which allows CheckStat and TryRemove as multi subrequests (its client dispatches by request class, so they share the Check/Remove classes). RaftKeeper dispatches by opnum, so StoreRequestMultiTxn threw 'Illegal command' for them despite advertising the CHECK_STAT/TRY_REMOVE feature flags — a ClickHouse client sending such a multi would be rejected. - TryRemove -> StoreRequestRemove (handles try_remove, has undo) - CheckStat -> StoreRequestCheckStat (read-only condition, no-op undo) RemoveRecursive/ListRecursive are intentionally still rejected in multi: StoreRequestRemoveRecursive has no undo, so it cannot roll back if a later subrequest fails. Adding it needs undo support first (follow-up). Test: MultiWriteWithCheckStatAndTryRemove verifies both in one write multi. 68 unit + 20 integration tests pass. Co-Authored-By: Claude <noreply@anthropic.com>

StoreRequestRemoveRecursive now snapshots every node it removes (clone() preserves each node's children set) and returns an Undo that re-adds them, restoring acl usage, ephemeral owners, and the surviving parent's children/numChildren/pzxid. This makes it safe as a multi subrequest, which is now enabled (matching ClickHouse Keeper). RaftKeeper already had closure-based rollback in StoreRequestMultiTxn; this just gives RemoveRecursive a working Undo instead of an empty one — no architectural change needed. Test: MultiRemoveRecursiveRollback puts RemoveRecursive + a failing Check in one multi and asserts the whole subtree (nodes, data, parent link, parent stat) is restored on rollback. 69 unit + 20 integration tests pass. Co-Authored-By: Claude <noreply@anthropic.com>

processRequest's watch-firing logic fired a watch for a single path (zk_request->getPath()), but RemoveRecursive deletes an entire subtree. Descendant nodes' watchers were never notified. Track the removed paths on StoreRequestRemoveRecursive and fire a DELETED watch for each. Also adds integration coverage for CheckStat, FilteredListWithStatsAndData, TryRemove/RemoveRecursive watch firing, RemoveRecursive/ListRecursive limits, and RemoveRecursive rollback inside multi transactions. Co-Authored-By: Claude <noreply@anthropic.com>

start(node) backgrounded node4's restart with start_wait=True, which makes start_raftkeeper() run its own wait_for_join_cluster(60) and kill -9 the process on timeout as a fallback. waiter.wait(timeout=50) doesn't cancel that thread, so it keeps running independently of the test's own later, separate node4.wait_for_join_cluster() call (issued only after nodes 1-3 are reconfigured). Under ASAN's slowdown the two checks landed close enough together that the background thread's stale timeout fired and killed node4's process right after the foreground had already validated it and started writing, producing a ConnectionLoss. test_three_nodes_two_alive already uses start_wait=False for this same background-start + explicit-wait_for_join_cluster idiom; apply the same here so the background thread only confirms the OS process launched and leaves the join check solely to the explicit call. Co-Authored-By: Claude <noreply@anthropic.com>

Session expiry needs up to ~1.5s beyond the negotiated timeout to be observable (500ms scan period + Raft commit round-trip), so under sanitizer slowdown the 1s-margin checks were flaky: the same test failed in master's own msan run with the identical assert 20 == 0. Adds 1s margin to each expiry check while keeping the ordering assertions (shorter-timeout sessions must expire before longer ones). Co-Authored-By: Claude <noreply@anthropic.com>

JackyWoo · 2026-08-18T09:09:49Z

Code review (re-review)

The new commits fix RemoveRecursive descendant watches (fires DELETED per removed path), FilteredListWithStatsAndData watch type registration (now treated as List), and add rollback + Multi-transaction support for RemoveRecursive/CheckStat/TryRemove. Good progress.

One issue from the previous round still stands:

TryRemove spuriously fires a DELETED watch when the node does not exist. When the node is missing, StoreRequestRemove::process sets response.error = ZOK for TryRemove (correct "best-effort" behavior — no actual deletion). But watch firing is guarded only by response->error == ZOK, so KeeperStore.cpp:1750 proceeds to call watch_manager.processWatches(path, OpNum::TryRemove), which falls through to fire Event::DELETED. The result: a TryRemove on a non-existent path sends clients a spurious DELETED event and erases the path's watches/list_watches entries even though the node was never removed. Fix: only fire the TryRemove DELETED watch when the node actually existed (e.g., track whether removal occurred and gate watch firing on that, rather than on response->error == ZOK).

RaftKeeper/src/Service/KeeperStore.cpp

Lines 496 to 499 in 47e4ba4

    
           { 
        
               if (request.try_remove) 
        
                   response.error = Coordination::Error::ZOK; 
        
               else

🤖 Generated with Claude Code

_{- If this code review was useful, please react with 👍. Otherwise, react with 👎.}

TryRemove returns ZOK even when the node does not exist, but watch firing was gated only on response->error == ZOK, so a TryRemove on a nonexistent path fired Event::DELETED and consumed the path's watches even though nothing was deleted (single-request path and Multi sub-requests alike). Track whether StoreRequestRemove actually removed a node (mirrors RemoveRecursive::removed_paths) and gate watch firing on it. Co-Authored-By: Claude <noreply@anthropic.com>

The tsan integration run aborted node1 with a ThreadSanitizer data race in NuRaft's commit-callback timeout path (result_code_ read outside commit_ret_elems_lock_ while the commit thread writes it under the lock). The race is only reachable when a commit exceeds client_req_timeout, and this test dir had it at 1s: under sanitizer slowdown session-establishment commits alone take ~1.3s, so the timeout path was hit routinely. With halt_on_error=1 the leader (only voter; nodes 2/3 are learners) died and 12 subsequent tests failed waiting for a cluster that could never elect a leader. NuRaft is a pinned upstream submodule, so fix on our side: raise the test configs' operation_timeout_ms (which also derives client_req_timeout_ms and the forwarder deadline) from 1s to 10s. Commits that used to trip the 1s timeout now stay on the normal path. test_cmd_conf asserts the configured value; updated to match. Co-Authored-By: Claude <noreply@anthropic.com>

JackyWoo · 2026-08-20T03:38:07Z

Code review (re-review)

No issues found. Checked for bugs and CLAUDE.md compliance.

The spurious DELETED watch for TryRemove-on-missing-node is fixed: a removed flag now gates watch firing on both the non-Multi path (gated on actually_removed) and the Multi path (per-sub-op sub_remove->removed, index-aligned with concrete_requests[i]/requests[i] since the else-branch throws). All previously reported issues across review rounds are resolved.

🤖 Generated with Claude Code

_{- If this code review was useful, please react with 👍. Otherwise, react with 👎.}

lzydmxy changed the title ~~feat: server-side RemoveRecursive, CheckStat, TryRemove, FilteredList…~~ feat: server-side RemoveRecursive, CheckStat, TryRemove, FilteredList，ListRecursive for clickhouse Jul 23, 2026

lzydmxy force-pushed the feat/server-side-ops branch 4 times, most recently from f31f564 to 5ff46a2 Compare July 25, 2026 18:26

lzydmxy force-pushed the feat/server-side-ops branch from 5ff46a2 to c180905 Compare July 26, 2026 11:34

JackyWoo self-assigned this Jul 29, 2026

JackyWoo added the feature New feature or request label Jul 29, 2026

JackyWoo added this to the Release v2.4.0 milestone Jul 29, 2026

undefined and others added 4 commits August 17, 2026 20:53

lzydmxy mentioned this pull request Aug 17, 2026

Fix: bad Multi aborts server, TTL create desyncs wire, unknown opnums hang, SetWatches drops child watches #399

Open

undefined and others added 2 commits August 18, 2026 12:01

undefined and others added 2 commits August 18, 2026 17:49

JackyWoo approved these changes Aug 20, 2026

View reviewed changes

JackyWoo merged commit 867c1a5 into JDRaftKeeper:master Aug 20, 2026
20 checks passed

Navigation Menu

Uh oh!

feat: server-side RemoveRecursive, CheckStat, TryRemove, FilteredList，ListRecursive for clickhouse - #398

feat: server-side RemoveRecursive, CheckStat, TryRemove, FilteredList，ListRecursive for clickhouse#398
JackyWoo merged 12 commits into
JDRaftKeeper:masterfrom
lzydmxy:feat/server-side-ops

lzydmxy commented Jul 23, 2026 •

edited

Loading

Uh oh!

raftkeeper-robot Bot commented Jul 23, 2026 •

edited

Loading

Uh oh!

raftkeeper-robot Bot commented Jul 23, 2026 •

edited

Loading

Uh oh!

JackyWoo commented Jul 28, 2026

Uh oh!

JackyWoo commented Jul 28, 2026

Uh oh!

JackyWoo commented Jul 29, 2026

Uh oh!

JackyWoo commented Aug 17, 2026

Uh oh!

JackyWoo commented Aug 17, 2026 •

edited

Loading

Uh oh!

JackyWoo commented Aug 18, 2026

Uh oh!

JackyWoo commented Aug 20, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

lzydmxy commented Jul 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issues of this PR fixes:

Change log:

Uh oh!

raftkeeper-robot Bot commented Jul 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

raftkeeper-robot Bot commented Jul 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JackyWoo commented Jul 28, 2026

Code review

Uh oh!

JackyWoo commented Jul 28, 2026

Uh oh!

JackyWoo commented Jul 29, 2026

Code review

Uh oh!

JackyWoo commented Aug 17, 2026

Code review

Uh oh!

JackyWoo commented Aug 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JackyWoo commented Aug 18, 2026

Code review (re-review)

Uh oh!

JackyWoo commented Aug 20, 2026

Code review (re-review)

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

lzydmxy commented Jul 23, 2026 •

edited

Loading

raftkeeper-robot Bot commented Jul 23, 2026 •

edited

Loading

raftkeeper-robot Bot commented Jul 23, 2026 •

edited

Loading

JackyWoo commented Aug 17, 2026 •

edited

Loading