ClickHouse / ClickHouse/ClickHouse
Keeper silently hands out a new session in place of the old one on reconnect — a ZooKeeper client never learns that its session has expired
- Dominant language
- C++
- Stars
- 49.9k
- Forks
- 9k
- Avg merge
- 20h 33m
- Merged PRs (30d)
- 501
Description
### Company or project name
gmonit.ru
### Describe what's wrong
ClickHouse Keeper does not support session restore (the `previous_session_id` field of the handshake is [read off the wire and discarded](https://github.com/ClickHouse/ClickHouse/blob/4c1d9c8ad564ef2d7aa4c6ecfc7bf9b141219194/src/Server/KeeperTCPHandler.cpp#L364), [the comment in the code](https://github.com/ClickHouse/ClickHouse/blob/4c1d9c8ad564ef2d7aa4c6ecfc7bf9b141219194/src/Server/KeeperTCPHandler.cpp#L303)). The problem is that the client is never told about it.
As a result the client loses the objects tied to the session, such as ephemeral nodes and watches.
And the application code will assume they are still alive, because there was no SessionExpired notification.
This reproduces for at least 2 zookeeper clients: Zookeeper (Java) and kazoo (Python).
The ClickHouse client itself is not affected: in `ZooKeeper::sendHandshake` (`src/Common/ZooKeeper/ZooKeeperImpl.cpp`) `previous_session_id` is a hardcoded literal `0`, ClickHouse never asks to continue a session.
### Does it reproduce on the most recent release?
Yes
### How to reproduce
Checked on v26.9.1.195-testing.
[repro_keeper_session_restore.py](https://github.com/user-attachments/files/31511288/repro_keeper_session_restore.py)
```
mkdir -p keeper_config.d
printf '0.0.0.0' > keeper_config.d/listen.xml
docker run --rm -p 9181:9181 -v "$PWD/keeper_config.d:/etc/clickhouse-keeper/keeper_config.d:ro" \
clickhouse/clickhouse-keeper:head
docker run --rm -p 2181:2181 zookeeper:3.9
pip install kazoo
python3 repro_keeper_session_restore.py 127.0.0.1:9181
python3 repro_keeper_session_restore.py 127.0.0.1:2181
```
ClickHouse Keeper `v26.9.1.195-testing-6e6a2a508889e7e97149298d3490ac139f8ae320`:
```
holding the ephemeral node /repro_session_3e2d5bf8
before the drop: session id 0x0000000000000001, ephemeralOwner 0x0000000000000001, client state CONNECTED
breaking the socket: kazoo _CONNECTION_DROP
client state: SUSPENDED
client state: CONNECTED
after the reconnect: session id 0x0000000000000002, ephemeralOwner 0x0000000000000001, client state CONNECTED
waiting: up to 15s for the abandoned session to time out
after the timeout: session id 0x0000000000000002, ephemeralOwner , client state CONNECTED
```
Apache ZooKeeper:
```
holding the ephemeral node /repro_session_4597938a
before the drop: session id 0x01004d6be1d30007, ephemeralOwner 0x01004d6be1d30007, client state CONNECTED
breaking the socket: kazoo _CONNECTION_DROP
client state: SUSPENDED
client state: CONNECTED
after the reconnect: session id 0x01004d6be1d30007, ephemeralOwner 0x01004d6be1d30007, client state CONNECTED
waiting: up to 15s for the abandoned session to time out
after the timeout: session id 0x01004d6be1d30007, ephemeralOwner 0x01004d6be1d30007, client state CONNECTED
```
### Expected behavior
If a client asks to continue its session and the server cannot continue it, the server must say "your session has expired" instead of handing out a new one in place of the old one.
### Error message and/or stacktrace
_No response_
### Related issues and pull requests
Related: https://github.com/ClickHouse/ClickHouse/issues/59489
### Additional context
I am attaching a patch generated by Opus, so that you can see how small the change is.
[keeper-session-expired.patch](https://github.com/user-attachments/files/31511293/keeper-session-expired.patch)
Contributor guide
Research direction
Start in src/Server/KeeperTCPHandler.cpp at the handshake handling referenced in the issue, then read the attached repro_keeper_session_restore.py and the related session code. Reproduce the reconnect with kazoo against ClickHouse Keeper and Apache ZooKeeper for comparison. Done means a client that requests session continuation is told when its session cannot be restored, rather than receiving a new session silently.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- cpp, java, python
- Domain
- databases, distributed-systems
- Issue type
- Bug
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Activity status
- Active
- Clarity
- Clearly specified
- Newbie friendliness
- 72/100