fix: MultiRead zxid and watch registration by lzydmxy · Pull Request #397 · JDRaftKeeper/RaftKeeper

lzydmxy · 2026-07-23T08:42:17Z

Which issues of this PR fixes:

This PR try to fix #

Change log:

Two fixes for MultiRead (OpNum 22) processing:

zxid: MultiRead no longer consumes a zxid. Added OpNum::MultiRead to shouldIncreaseZxid() exclusion list. MultiRead is a read-only operation — only write requests should advance the global transaction ID.
Unbundle & watches: MultiRead subrequests are now processed individually instead of through StoreRequestMultiTxn. Each subrequest gets:
- Independent ACL check
- Per-subrequest watch registration (was silently dropped before — only the outer MultiRead wrapper's has_watch was checked, which is always false)
- Individual error codes preserved per subrequest
Added an allowlist guard rejecting write ops (Create/Set/Remove) inside
MultiRead — without this they would bypass Raft consensus.

Tests: 3 unit (zxid stability, watch registration, per-subrequest errors)
+ 2 integration (zxid via mntr, watches via wchc 4LW)

Two fixes for MultiRead (OpNum 22) processing: 1. zxid: MultiRead no longer consumes a zxid. Added OpNum::MultiRead to shouldIncreaseZxid() exclusion list. MultiRead is a read-only operation — only write requests should advance the global transaction ID. 2. Unbundle & watches: MultiRead subrequests are now processed individually instead of through StoreRequestMultiTxn. Each subrequest gets: - Independent ACL check - Per-subrequest watch registration (was silently dropped before — only the outer MultiRead wrapper's has_watch was checked, which is always false) - Individual error codes preserved per subrequest Added an allowlist guard rejecting write ops (Create/Set/Remove) inside MultiRead — without this they would bypass Raft consensus. Tests: 3 unit (zxid stability, watch registration, per-subrequest errors) + 2 integration (zxid via mntr, watches via wchc 4LW) Co-Authored-By: Claude <noreply@anthropic.com>

raftkeeper-robot · 2026-07-23T09:00:49Z

Unit test report for commit d8aa1fc.

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	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	✅
RaftStateManager	load_srv_state	✅
SnapshotZstdBench	DISABLED_V2vsV3	✅
ZstdLevelBench	DISABLED_AllLevels	✅
RaftPerformance	appendLogThread	✅

raftkeeper-robot · 2026-07-23T09:58:15Z

Integration test report for commit d8aa1fc.

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_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	✅

- test_multi_read_zxid_stability: switch from mntr to srvr 4LW command. mntr uses zk_key\tvalue format and doesn't include Zxid; srvr uses Key: value format including Zxid. - test_multi_read_subrequest_watch: add missing exists() method to MultiReadRequest and import Exists from kazoo.client. Co-Authored-By: Claude <noreply@anthropic.com>

MultiRead.deserialize() returns get_children as a bare list, not a tuple. results[2] is already ['a', 'b'] — indexing with [0] gave 'a' (a single char). Co-Authored-By: Claude <noreply@anthropic.com>

JackyWoo · 2026-07-24T03:55:55Z

Code review

Found 3 test coverage gaps:

No test for write-op allowlist guard — The new MultiRead branch throws Exception(BAD_ARGUMENTS, ...) when a write operation (Create/Set/Remove) is passed inside a MultiRead request. This is security-critical ("Writes routed here would bypass Raft consensus") but has no unit or integration test verifying the rejection.

RaftKeeper/src/Service/KeeperStore.cpp

Lines 1363 to 1375 in 78f5725

    
           auto sub_opnum = sub_zk_request->getOpNum(); 
        
           /// MultiRead only allows read operations. Writes routed here would bypass Raft consensus. 
        
           if (sub_opnum != Coordination::OpNum::Get 
        
               && sub_opnum != Coordination::OpNum::Exists 
        
               && sub_opnum != Coordination::OpNum::List 
        
               && sub_opnum != Coordination::OpNum::SimpleList 
        
               && sub_opnum != Coordination::OpNum::FilteredList 
        
               && sub_opnum != Coordination::OpNum::GetACL) 
        
           { 
        
               throw Exception( 
        
                   ErrorCodes::BAD_ARGUMENTS, 
        
                   "Illegal command {} as part of MultiRead request",

No test for per-subrequest ACL check — The code checks ACL individually per subrequest and returns ZNOAUTH for unauthorized ones. Neither the unit tests nor integration tests exercise this path (all tests pass check_acl=true but never set up an unauthorized session).

RaftKeeper/src/Service/KeeperStore.cpp

Lines 1379 to 1383 in 78f5725

    
           auto sub_store_request = StoreRequestFactory::instance().get(sub_zk_request); 
        
           Coordination::ZooKeeperResponsePtr sub_response; 
        
           if (check_acl && !sub_store_request->checkAuth(*this, session_id)) 
        
           {

No test for Exists watch registration on non-existent node — The code has a special branch that registers a watch even when error == ZNONODE for Exists requests. The existing MultiReadRegistersSubrequestWatches test uses Exists on /test_node_b which exists, so this conditional (sub_response->error == Coordination::Error::ZNONODE && sub_zk_request->getOpNum() == Coordination::OpNum::Exists) is never actually exercised.

RaftKeeper/src/Service/KeeperStore.cpp

Lines 1391 to 1394 in 78f5725

    
           sub_response->xid = sub_zk_request->xid; 
        
           sub_response->zxid = zxid.load();

Coverage summary:

Behavior	Unit Test	Integration Test
MultiRead doesn't bump zxid	covered	covered
Watch registration per subrequest	covered	covered
Individual error codes per subrequest	covered	-
Write-op allowlist guard (throws on illegal ops)	missing	-
ACL check per subrequest (ZNOAUTH path)	missing	-
Exists watch on ZNONODE (special conditional)	missing	-

🤖 Generated with Claude Code

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

…NONODE watch Covers three review findings: - MultiReadRejectsWriteOps: verifies Exception is thrown when Create/Set/Remove subrequests appear inside a MultiRead - MultiReadExistsWatchOnNonExistentNode: verifies Exists with watch=true on a nonexistent path still registers the watch (exercises the ZNONODE + Exists conditional branch) - MultiReadAuthCheckPerSubrequest: verifies per-subrequest ACL checks — a public node returns ZOK, a digest-only node returns ZNOAUTH Co-Authored-By: Claude <noreply@anthropic.com>

JackyWoo · 2026-07-24T05:11:38Z

Code review (follow-up)

All 3 previously flagged coverage gaps are now addressed:

MultiReadRejectsWriteOps — verifies the write-op allowlist guard throws on illegal Create inside MultiRead
MultiReadAuthCheckPerSubrequest — verifies per-subrequest ACL enforcement with mixed public/restricted nodes
MultiReadExistsWatchOnNonExistentNode — verifies watch registration on ZNONODE for Exists subrequests

Updated coverage:

Behavior	Unit Test	Integration Test
MultiRead doesn't bump zxid	covered	covered
Watch registration per subrequest	covered	covered
Individual error codes per subrequest	covered	-
Write-op allowlist guard	covered	-
ACL check per subrequest	covered	-
Exists watch on ZNONODE	covered	-

No further issues found. LGTM.

🤖 Generated with Claude Code

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

undefined and others added 2 commits July 23, 2026 19:43

fix: wrong get_children response index in zxid stability test

78f5725

MultiRead.deserialize() returns get_children as a bare list, not a tuple. results[2] is already ['a', 'b'] — indexing with [0] gave 'a' (a single char). Co-Authored-By: Claude <noreply@anthropic.com>

lzydmxy requested a review from JackyWoo July 24, 2026 02:52

JackyWoo approved these changes Jul 24, 2026

View reviewed changes

JackyWoo merged commit 67f173b into JDRaftKeeper:master Jul 24, 2026
20 checks passed

Navigation Menu

Uh oh!

fix: MultiRead zxid and watch registration - #397

fix: MultiRead zxid and watch registration#397
JackyWoo merged 4 commits into
JDRaftKeeper:masterfrom
lzydmxy:fix/multiread-unbundle-and-zxid

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 24, 2026

Uh oh!

JackyWoo commented Jul 24, 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 24, 2026

Code review

Uh oh!

JackyWoo commented Jul 24, 2026

Code review (follow-up)

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