Missing throw in generateUUIDForTable lets RESTORE silently reuse backup UUIDs

Inizia da src/Backups/RestoreCoordinationOnCluster.cpp:424 in generateUUIDForTable, poi leggi le chiamate da RestorerFromBackup::createTable e createDatabase. Esegui la riproduzione oppure aggiungi il caso di regressione descritto, eliminando il nodo Keeper table_uuids durante RESTORE. Il lavoro è completato quando RESTORE fallisce con un errore di coordinamento dovuto al nodo mancante, invece di riuscire con l’UUID del backup.

Describe what's wrong

• RESTORE ... ON CLUSTER reports RESTORED and reuses the backup's table UUID instead of failing when <restore>/table_uuids is missing in Keeper.
• Trigger is the ZooKeeper node table_uuids vanishing mid-restore, so tryCreate returns ZNONODE.
• Expected: the restore fails with a coordination error about the missing node; actual: it succeeds and silently keeps the old UUID.
• Once this happens, every later table and database on that host takes the same silent path, since the node is never recreated.

Root cause: In RestoreCoordinationOnCluster.cpp:424 inside generateUUIDForTable, the KeeperException built for ZNONODE is never thrown, so the lambda returns normally, retryLoop treats it as success, and the backup UUID is kept instead of regenerated.

Does it reproduce on most recent release?

Confirmed on master; not checked on a release.

How to reproduce

Reproducing requires continuously deleting the table_uuids Keeper node during the RESTORE, since the in-tree cleaner normally only removes it after all hosts finish.

import threading
import uuid

import pytest
from kazoo.exceptions import NoNodeError, NotEmptyError

from helpers.cluster import ClickHouseCluster

cluster = ClickHouseCluster(__file__)
node = cluster.add_instance(
    "node",
    main_configs=["configs/cluster.xml"],
    external_dirs=["/backups/"],
    with_zookeeper=True,
)


@pytest.fixture(scope="module", autouse=True)
def started_cluster():
    try:
        cluster.start()
        yield cluster
    finally:
        cluster.shutdown()


def test_restore_fails_if_table_uuids_node_is_missing():
    node.query("CREATE TABLE t (x UInt32) ENGINE = MergeTree ORDER BY x")
    node.query("INSERT INTO t VALUES (1), (2), (3)")
    node.query("BACKUP TABLE t TO Disk('backups', 'b1')")
    node.query("DROP TABLE t SYNC")

    restore_uuid = str(uuid.uuid4())
    path = f"/clickhouse/backups/restore-{restore_uuid}/table_uuids"
    zk = cluster.get_kazoo_client("zoo1")
    stop = threading.Event()

    def remove_table_uuids_node():
        while not stop.is_set():
            try:
                zk.delete(path)
            except (NoNodeError, NotEmptyError):
                pass

    thread = threading.Thread(target=remove_table_uuids_node)
    thread.start()
    try:
        error = node.query_and_get_error(
            f"RESTORE TABLE t ON CLUSTER 'cluster1' FROM Disk('backups', 'b1') "
            f"SETTINGS restore_uuid = '{restore_uuid}'"
        )
    finally:
        stop.set()
        thread.join()

    assert "No node" in error

Files next to test.py:

configs/cluster.xml:

<clickhouse>
    <remote_servers>
        <cluster1>
            <shard>
                <replica>
                    <host>node</host>
                    <port>9000</port>
                </replica>
            </shard>
        </cluster1>
    </remote_servers>
    <storage_configuration>
        <disks>
            <backups>
                <type>local</type>
                <path>/backups/</path>
            </backups>
        </disks>
    </storage_configuration>
    <backups>
        <allowed_disk>backups</allowed_disk>
    </backups>
</clickhouse>

The same steps without the integration-test harness, using the official Docker images:

#!/usr/bin/env bash
set -u
IMAGE=${IMAGE:-clickhouse/clickhouse-server:head}
NAME=ch126973
DIR=$(mktemp -d)
cat > "$DIR/repro.xml" <<'EOF'
<clickhouse>
    <keeper_server>
        <tcp_port>9181</tcp_port>
        <server_id>1</server_id>
        <log_storage_path>/var/lib/clickhouse/coordination/log</log_storage_path>
        <snapshot_storage_path>/var/lib/clickhouse/coordination/snapshots</snapshot_storage_path>
        <raft_configuration>
            <server><id>1</id><hostname>localhost</hostname><port>9234</port></server>
        </raft_configuration>
    </keeper_server>
    <zookeeper><node><host>localhost</host><port>9181</port></node></zookeeper>
    <remote_servers>
        <c1><shard><replica><host>localhost</host><port>9000</port></replica></shard></c1>
    </remote_servers>
    <storage_configuration>
        <disks><backups><type>local</type><path>/backups/</path></backups></disks>
    </storage_configuration>
    <backups><allowed_disk>backups</allowed_disk></backups>
</clickhouse>
EOF
docker rm -f $NAME >/dev/null 2>&1
docker run -d --name $NAME -v "$DIR/repro.xml:/etc/clickhouse-server/config.d/repro.xml" "$IMAGE" >/dev/null
Q() { docker exec $NAME clickhouse-client -q "$1"; }
for i in $(seq 60); do Q "SELECT 1" >/dev/null 2>&1 && break; sleep 1; done
Q "SELECT version()"
Q "CREATE TABLE t (x UInt32) ENGINE = MergeTree ORDER BY x"
Q "INSERT INTO t VALUES (1), (2), (3)"
echo "original uuid: $(Q "SELECT uuid FROM system.tables WHERE database = 'default' AND name = 't'")"
Q "BACKUP TABLE t TO Disk('backups', 'b1')" >/dev/null
Q "DROP TABLE t SYNC"
# Control: a normal RESTORE ON CLUSTER gives the table a new UUID.
Q "RESTORE TABLE t ON CLUSTER c1 FROM Disk('backups', 'b1')" >/dev/null
echo "control uuid:  $(Q "SELECT uuid FROM system.tables WHERE database = 'default' AND name = 't'")"
Q "DROP TABLE t SYNC"
# Same RESTORE while the coordination node <restore>/table_uuids keeps disappearing.
R=$(cat /proc/sys/kernel/random/uuid)
docker exec -d $NAME bash -c "timeout 20 bash -c \"yes 'rm /clickhouse/backups/restore-$R/table_uuids' | clickhouse keeper-client -p 9181 --tests-mode\" >/dev/null 2>&1"
sleep 1
Q "RESTORE TABLE t ON CLUSTER c1 FROM Disk('backups', 'b1') SETTINGS restore_uuid = '$R'"
echo "restored uuid: $(Q "SELECT uuid FROM system.tables WHERE database = 'default' AND name = 't'")"
Q "SELECT count() FROM t"
Q "SELECT name, value, last_error_message FROM system.errors WHERE name = 'KEEPER_EXCEPTION' AND last_error_message LIKE '%table_uuids%'"
docker rm -f $NAME >/dev/null
rm -rf "$DIR"

Expected behavior

RESTORE should fail with a coordination error like 'No node, path /clickhouse/backups/restore-/table_uuids/...'.

Expected output of the integration test above:

RESTORE fails with `Coordination error: No node, path /clickhouse/backups/restore-<uuid>/table_uuids/...` and the test passes. Without the deleter, the restored table gets a new UUID (control above: d04b4410-... != c8aeceab-...).

Error message and/or stacktrace

RESTORE reports RESTORED and the table ends up with the original backup UUID instead of a new one, with only a KEEPER_EXCEPTION row in system.errors as a trace.

Actual output of the integration test above:

Integration test, run 1:
E helpers.client.QueryRuntimeException: Client expected to be failed but succeeded! stdout: 29e48b75-b345-443e-9295-759094eca395	RESTORED
Integration test, run 2:
E helpers.client.QueryRuntimeException: Client expected to be failed but succeeded! stdout: 89ea8732-6be6-4f65-bc2a-b79f59cd0b5f	RESTORED

Standalone script on clickhouse/clickhouse-server:head (26.11.1.353):
ori

Additional context

Open risks:

• The proof test relies on a background thread deleting table_uuids between the internal host's startup and its first generateUUIDForTable. It was deterministic in 4 of 4 local runs, but it is timing-based.

Found during automated review of PR #126973; this finding was not executed, so nobody is tagged. The cited lines predate that PR: per git blame they were introduced by #109327 (Fix EmbeddedRocksDB backup to include table data). Severity P2 · Finding h_pr126973_001