fix: clean errors instead of crashes/hangs on unsupported requests; fix SetWatches child-watch restore - #400
Open
lzydmxy wants to merge 5 commits into
Open
Conversation
|
Unit test report for commit d45dfb0. All test cases passed! Successful Test Cases
|
|
Integration test report for commit d45dfb0. All test cases passed! Successful Test Cases
|
…ix SetWatches child-watch restore Four fixes from a ClickHouse-Keeper parity audit: 1. A Multi containing an unsupported sub-op (GetACL, Sync, mixed read/write, ...) threw BAD_ARGUMENTS at Raft apply time, where RequestProcessor::applyRequest catches any exception and calls ::abort() on the whole server process — a remotely triggerable DoS. The validation now records the error instead of throwing, and the Multi answers with a clean ZBADARGUMENTS response. 2. ZK 3.5+ TTL create modes (PERSISTENT_WITH_TTL / PERSISTENT_SEQUENTIAL_ WITH_TTL) append an int64 ttl after the flags. RaftKeeper didn't read it, so the node was silently created as plain persistent AND the next request on the connection parsed garbage. The ttl is now consumed and the mode rejected with ZUNIMPLEMENTED (stream stays aligned); CONTAINER and unknown create modes are rejected with ZBADARGUMENTS, matching ClickHouse Keeper. 3. Unknown opnums (Reconfig 16, Create2 15, CreateTTL 21, ...) threw from getOpNum/toString inside the receive path and the request was silently dropped — the client hung until its timeout. The connection now replies with a clean error response and stays usable. 4. SetWatches(101) built its node-info map from data_watches only, so every re-established child watch was treated as "node missing": it spuriously fired DELETED and dropped the watch, and exist watches never fired CREATED. The map now covers all three watch arrays. Also fixes a latent deadlock the now-reachable firing path exposed (processRequestSetWatch held watch_mutex while calling processWatches, which locks it again), and makes CHILD events fire list watches instead of misdelivering to data watches. Tests: 3 new unit tests (Multi rejection without abort, TTL/container wire rejection without desync, SetWatches restore semantics) and 3 new wire-level integration tests in test_session_fake_client (unknown opnum error reply + connection survival, TTL create rejection + stream alignment, bad Multi error reply + server survival). Co-Authored-By: Claude <noreply@anthropic.com>
…g; SetWatches fires only the restoring session Code-review fixes on top of the tranche-1 changes: 1. MultiRead with a non-read sub-op threw BAD_ARGUMENTS at apply time; read-path exceptions produce no response, so the client hung until its timeout. It now answers with a clean per-subrequest ZBADARGUMENTS error at the offending position (the valid sub-ops still execute), matching the clean-error behavior of the Multi write path. Updated MultiReadRejectsWriteOps to assert the wire-visible behavior instead of the throw. 2. SetWatches restore triggers went through the global processWatches, which broadcasts events to every session watching the path and cascades to the parent path's list watches (e.g. an exist-watch restore firing CREATED also consumed the parent's children watch). ZooKeeper's DataTree.setWatches fires only the re-registering session's own watcher. Added a session-scoped triggerWatchForSession that fires and unregisters only that session's watch with no parent cascade; the unit test now also asserts fired events only ever go to the restoring session and that another session's watch survives. Co-Authored-By: Claude <noreply@anthropic.com>
…es restore Covers the review fixes with end-to-end wire tests in test_session_fake_client: - test_multiread_with_write_subop_returns_per_position_error: a MultiRead with [Create, Exists] sub-ops answers per-subrequest errors (ZBADARGUMENTS at the bad position, the valid Exists still runs) and the connection stays usable. - test_set_watches_restore_fires_correct_events: raw SetWatches restores fire CHILD (not the pre-fix spurious DELETED) when pzxid moved, silently re-register when unchanged, fire CREATED for exist watches on existing nodes, and only ever deliver to the re-registering session — no parent-path cascade, no cross-session broadcast (a second raw session on the same node watches the same path and must receive nothing). Also makes recv_reply buffered per socket: the server may coalesce several replies into one TCP segment, and the previous helper dropped the second frame. Co-Authored-By: Claude <noreply@anthropic.com>
lzydmxy
force-pushed
the
fix/robust-request-handling
branch
from
August 20, 2026 03:51
04a3bcf to
d4aa4d8
Compare
Contributor
Review of PR #400I did a full review of this PR: built it (clang-18 / Ninja / RelWithDebInfo), ran the complete unit test suite (72/72 pass, including the 3 new tests), ran all 5 new wire-level integration tests (all pass), and re-ran the existing watch-related suites ( Verified
Issue found — one fix requested1.
if (construction_error != Coordination::Error::ZOK)
{
response_typed.error = construction_error;
return {response, {}};
}The Suggested fix — reuse the existing rollback pattern: if (construction_error != Coordination::Error::ZOK)
{
for (size_t j = 0; j < response_typed.responses.size(); ++j)
{
response_typed.responses[j] = std::make_shared<Coordination::ZooKeeperErrorResponse>();
response_typed.responses[j]->error = construction_error;
}
response_typed.error = construction_error;
return {response, {}};
}Severity: medium — the crash is fixed (primary goal), but the wire response shape is wrong and may confuse strict clients. Minor / optional2. 3. 4. NIT: the module-level VerdictApprove with finding 1 requested before merge — it's a small, localized fix reusing an existing pattern in the same function. Everything else is solid: the commit messages are exemplary, the deadlock fix is real, the session-scoped watch semantics are correct, and the raw-wire integration tests are exactly the right level (they prove server survival and stream alignment, which unit tests can't). |
…e the per-op body JackyWoo review on JDRaftKeeper#400: the construction_error early-return set the top-level response error to the rejection code directly. ZooKeeperResponse:: writeNoCopy only serializes the per-subrequest body when the top-level error is ZOK, so a non-ZOK top-level error here silently suppressed the body entirely — the client would see an empty multi response instead of the per-op errors the wire format is supposed to carry (the reviewer's suggested snippet populated the sub-response array but kept the top-level error non-ZOK, which would still have been a no-op on the wire). Fixed to match the existing rollback path in the same function: leave the top-level error at its ZOK default and populate every sub-response with the rejection error, so writeImpl actually runs and the body carries the real per-op errors. Also confirmed this doesn't trip the write-path's watch-firing check, which gates on the *last* sub-response's error, not the top-level one. Updated the two unit tests and the raw-wire integration test that asserted the old (incorrect) top-level-error shape to instead check the per-op body, matching the sibling MultiRead test added in the same PR. Also drops the now-unused BAD_ARGUMENTS extern declaration (flagged by style-check). Co-Authored-By: Claude <noreply@anthropic.com>
Contributor
Author
|
@JackyWoo Finding 1 confirmed real, fixed in 36f2731 — but not with the exact snippet suggested, since that snippet wouldn't have changed anything on the wire. Traced the actual send path ( Coordination::write(error, out);
if (error == Error::ZOK)
writeImpl(out); // this is what serializes response_typed.responses[]
Fix instead leaves the top-level error at its ZOK default (matching the existing rollback loop a few lines below, which never touches Also verified this doesn't accidentally trigger watch-firing for the rejected multi: that check gates on Updated the two unit tests and the raw-wire integration test that had asserted the old top-level-error shape — they now check the per-op body, matching the sibling MultiRead test. Dropped the unused 72/72 unit, 35/35 integration (fake-client + back-to-back) after the fix. |
…s full-suite CI run Root-caused each rather than blanket-raising timeouts: - test_random_requests, test_concurrent_watches: inherently long-running (thousands of round trips / sleep-paced concurrent workload) and already minimized where possible; the 300s pytest-timeout default doesn't leave headroom under a loaded CI runner. Given explicit per-test overrides via @pytest.mark.timeout(600) instead of cutting coverage further. - test_node_replace: the write through zk_conn4 (a node that just joined via live 4-node reconfig) may need an extra forwarding hop while the new leader settles; the default 10s kazoo session timeout was too tight for that under load. Bumped to 30s for that connection only. - test_snapshot_and_load[True]: the final unguarded create() lands right after 1000 rapid creates across a freshly-restarted 3-node cluster: a transient timeout there is the same class of hiccup the preceding loop already tolerates via try/except. Gave it the same tolerance (a few retries) instead of leaving it as the one fragile call in the function. - test_between_servers, test_server_restart: 1000 children x (create + set + 3 reads) is 5000+ round trips per test. Reduced to 100, matching the existing precedent in test_random_requests — same assertions, same cross-server consistency coverage, far fewer round trips. Verified locally: all 6 tests pass individually, plus a full test_back_to_back regression run (27/27) since two of its tests changed. Co-Authored-By: Claude <noreply@anthropic.com>
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
2 participants
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.
Which issues of this PR fixes:
Fixes #399
Change log:
Four fixes from a ClickHouse-Keeper parity audit:
A Multi containing an unsupported sub-op no longer aborts the server.
StoreRequestMultiTxnthrew BAD_ARGUMENTS at Raft apply time, whereRequestProcessor::applyRequestcatches any exception and calls::abort()on the whole server process — a remotely triggerable DoS. The validation now records the error instead of throwing, and the Multi answers with a clean ZBADARGUMENTS response.TTL-mode creates no longer desync the wire stream. ZK 3.5+ TTL create modes (PERSISTENT_WITH_TTL / PERSISTENT_SEQUENTIAL_WITH_TTL) append an int64 ttl after the flags. RaftKeeper didn't read it, so the node was silently created as plain persistent AND the next request on the connection parsed garbage. The ttl is now consumed and the mode rejected with ZUNIMPLEMENTED (stream stays aligned); CONTAINER and unknown create modes are rejected with ZBADARGUMENTS, matching ClickHouse Keeper.
Unknown opnums get a clean error reply. Reconfig(16)/Create2(15)/CreateTTL(21) threw from getOpNum/toString inside the receive path and the request was silently dropped — the client hung until its timeout. The connection now replies with a clean error response and stays usable.
SetWatches(101) restores child and exist watches correctly. The node-info map was built from data_watches only, so every re-established child watch was treated as "node missing": it spuriously fired DELETED and dropped the watch, and exist watches never fired CREATED. The map now covers all three watch arrays. Also fixes a latent deadlock the now-reachable firing path exposed (processRequestSetWatch held watch_mutex while calling processWatches, which locks it again — registration and firing are now separated), and makes CHILD events fire list watches instead of misdelivering to data watches.
Tests:
test_session_fake_client: unknown opnum error reply + connection survival, TTL create rejection + stream alignment, bad Multi error reply + server survival (a quorum write right after proves the server didn't die).test_session_fake_client, 17/17test_back_to_back.