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

Open
#116,690 1 comment 1 reaction 0 assignees View on GitHub
comp-keeper external potential bug
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.