Fix crash caused by stale ZooKeeper session in UDF retry loop

[fm4v]

Linked issues

• fixes https://github.com/ClickHouse/clickhouse-core-incidents/issues/1645

The ZooKeeperRetriesControl retry loop introduced in #101891 reused the same expired ZooKeeper session on every retry iteration, without calling renewZooKeeper as every other retry loop in the codebase does. This caused repeated requests on a finalized session, leading to crashes on canary deploys (https://github.com/ClickHouse/clickhouse-core-incidents/issues/1644, https://github.com/ClickHouse/clickhouse-core-incidents/issues/1645).

Fix: renew the ZooKeeper session via zookeeper_getter on each retry iteration (matching the pattern used by backup coordination code), and move getObjectNamesAndSetWatch inside the retry loop so that the object list and watches are re-established on the fresh session.

Changelog category (leave one):

• Critical Bug Fix (crash, data loss, RBAC)

Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):

Fix crash in UDF refresh caused by ZooKeeperRetriesControl retrying on a stale (expired) ZooKeeper session without renewing it.

Documentation entry for user-facing changes

• Documentation is written (mandatory for new features)

[clickhouse-gh]

Workflow [PR], commit [b20f2d8]

Summary: ❌

job_name	test_name	status	info	comment
Stress test (arm_msan)		failure
	Server died	FAIL	cidb, issue	ISSUE EXISTS
	MemorySanitizer: use-of-uninitialized-value (STID: 4179-5154)	FAIL	cidb, issue	ISSUE EXISTS
Finish Workflow		failure
	python3 ./ci/jobs/scripts/workflow_hooks/new_tests_check.py	failure		IGNORED
Fast test (arm_darwin)		error		IGNORED

---

AI Review

Summary

This PR fixes a real reliability issue in UDF refresh: retries now renew ZooKeeper session via zookeeper_getter and re-fetch object names/watches on the fresh session before reloading objects. The core fix is correct and aligned with existing ZooKeeperRetriesControl usage patterns. I did not find new correctness, safety, concurrency, or performance blockers in the current diff.

Tests

⚠️ No regression test was added for the stale-session retry path in refreshObjects. This gap has already been raised in existing inline review discussion, so I am not posting a duplicate inline comment.

ClickHouse Rules

Item	Status	Notes
Deletion logging	➖
Serialization versioning	➖
Core-area scrutiny	✅
No test removal	✅
Experimental gate	➖
No magic constants	✅
Backward compatibility	✅
SettingsChangesHistory.cpp	➖
PR metadata quality	✅
Safe rollout	✅
Compilation time	✅
No large/binary files	✅

Final Verdict

Status: ⚠️ Request changes

Minimum required action:

• Add a focused regression test that reproduces a Keeper session-expiration/hardware-error condition during UDF refresh/retry and verifies recovery on a fresh session without leaving a partial UDF set.

[clickhouse-gh]

Please add a regression test for this path. The original issue was a production exception caused by retrying on an expired Keeper session; this change removes the local retry loop, so we need a test that forces a Keeper hardware error during refreshObjects and verifies recovery happens via processWatchQueue retry (fresh session) without leaving a partial UDF set.

[alexey-milovidov]

The 02346_text_index_bug101913 test failure is fixed by #102108 (already merged). Please rebase or merge master to pick up the fix.

[clickhouse-gh]

LLVM Coverage Report

Metric	Baseline	Current	Δ
Lines	84.10%	84.00%	-0.10%
Functions	90.90%	90.90%	+0.00%
Branches	76.60%	76.50%	-0.10%

Changed lines: 95.24% (20/21) | lost baseline coverage: 2 line(s) · Uncovered code

Full report · Diff report

[alexey-milovidov]

The MSan stress test failure (MemorySanitizer: use-of-uninitialized-value, STID 4179-5154 or 4148-3044) is a known pre-existing issue unrelated to this PR. Fix: #102158

[alexey-milovidov]

@fm4v, our CI checks that every bug fix is accompanied by a test. But you ignored it... Maybe there is still a chance to add a test?

[azat]

Other places issues a SYNC to keeper in case of new session, probably we also need it here? Since other keeper nodes may not see some objects that was visible from this session

Also, why this and #101891 has been merged w/o review?

[fm4v]

My bad, I tested it on a single customer replica before merging: https://github.com/ClickHouse/clickhouse-private/pull/54946

[tavplubix]

SYNC to keeper in case of new session

@fm4v please also check this:

ClickHouse/src/Common/ZooKeeper/ZooKeeper.h

Lines 604 to 618 in b95ea45

	/// Checks that our session was not killed, and allows to avoid applying a request from an old lost session.
	/// Imagine a "connection-loss-on-commit" situation like this:
	/// - We have written some write requests to the socket and immediately disconnected (e.g. due to "Operation timeout")
	/// - The requests were sent, but the destination [Zoo]Keeper host will receive it later (it doesn't know about our requests yet)
	/// - We don't know the status of our requests
	/// - We connect to another [Zoo]Keeper replica with a new session, and do some reads
	/// to find out the status of our requests. We see that they were not committed.
	/// - The packets from our old session finally arrive to the destination [Zoo]Keeper host. The requests get processed.
	/// - Changes are committed (although, we have already seen that they are not)
	///
	/// We need a way to reject requests from old sessions somehow.
	///
	/// So we update the version of /clickhouse/sessions/server_uuid node when starting a new session.
	/// And there's an option to check this version when committing something.
	void addCheckSessionOp(Coordination::Requests & requests) const;

[fm4v]

@alexey-milovidov I've added a test in the first PR #101891, but it was unreliable due to network fault injection timing. Tested manually on a custom replica of the affected customer instance