Skip to content

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

Description

@lzydmxy

From a deep parity audit of every ZK command between RaftKeeper and ClickHouse Keeper, four client-visible bugs (fixed on branch fix/robust-request-handling, PR follows):

  1. A Multi with an unsupported sub-op aborts the whole server. StoreRequestMultiTxn throws for unknown sub-opnums (GetACL, Sync, FilteredListWithStatsAndData, ListRecursive, ...) or read/write mixes at Raft apply time, where RequestProcessor::applyRequest catches any exception and calls ::abort() — a remotely triggerable DoS. ClickHouse accepts the same requests. Fixed: validation records the error instead of throwing, and the Multi replies with a clean ZBADARGUMENTS.

  2. TTL-mode create desyncs the wire stream. ZK 3.5+ PERSISTENT_WITH_TTL/PERSISTENT_SEQUENTIAL_WITH_TTL append a trailing ttl int64 that RaftKeeper never read: the node was silently created as plain persistent and the next request on the connection parsed garbage. Fixed: the ttl is consumed and the mode rejected with ZUNIMPLEMENTED; CONTAINER and unknown create modes are rejected with ZBADARGUMENTS (matches ClickHouse Keeper).

  3. Unknown opnums hang clients. Opnums like Reconfig(16)/Create2(15)/CreateTTL(21) threw while receiving; the request was silently dropped and the client hung until its timeout. Fixed: the server replies with a clean error response and the connection stays usable.

  4. SetWatches(101) drops child watches on reconnect. watch_nodes_info was built from data_watches only, so every re-established child watch was treated as "node missing": it spuriously fired NodeDeleted and was dropped, and exist watches never fired NodeCreated. Fixed: the node-info map now covers all three watch arrays. Also fixed two latent bugs the now-reachable firing path exposed: a watch_mutex self-deadlock in processRequestSetWatch (registration under lock, firing after), and CHILD events misdelivering to data watches instead of list watches.

Tests: 3 new unit tests + 3 new wire-level integration tests in test_session_fake_client (including one that proves the server survives the bad Multi by doing a quorum write afterwards). Local runs: 65/65 unit, 6/6 fake-client, 17/17 back-to-back.

Remaining tranches (planned after #398 merges, since they touch ops 503-507 introduced there):

  • CheckStat wire format incompatible with ClickHouse (path + 3×int32 vs path + int32 + full Stat)
  • Feature-flag bit positions diverge from bit 5 onward
  • TryRemove only swallows ZNONODE (ClickHouse also swallows ZBADVERSION/ZNOTEMPTY)
  • RemoveRecursive: ZNONODE on missing root (ClickHouse: ZOK); Delete ACL checked only on parent (ClickHouse: every node)
  • FilteredListWithStatsAndData / ListRecursive skip per-child / per-descendant ACL checks
  • /keeper/* system paths writable by clients

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't working

    Type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions