Fix flaky test_async_load_databases::test_materialized_views_replicated

[groeneai]

Changelog category (leave one):

• CI Fix or Improvement (changelog entry is not required)

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

...

Description

Fixes flaky test_async_load_databases::test_materialized_views_replicated. 13 distinct PR groups hit it in the last 30 days (most recent 2026-06-10); the dominant signature is an empty test_context_raw (the 900s pytest session timeout), and when the session is killed it takes the sibling test_materialized_views_cascaded_multiple down too (shared module fixture).

Root cause (two cooperating timing assumptions that break on slow sanitizer builds):

1. disconnect_event.wait(90) ignored its return value. When restart_clickhouse() takes longer than 90s (its stop+start budget alone is ~120s, and startup is much slower under TSan/ASan), the main thread fell through while the server was still restarting.
2. The INSERT loops used node1.query(...) with no per-query timeout. A query to a half-restarted server (TCP port open but not yet serving) makes clickhouse-client connect and then block on the response. With DEFAULT_QUERY_TIMEOUT=600s, one such query exhausts the 900s session timeout.

Fix (test-only, assertions unchanged):

• assert disconnect_event.wait(180) so a too-slow restart fails loudly instead of silently racing ahead (180s gives sanitizer builds headroom over the original 90s).
• timeout=60 on the INSERTs in both loops, so a connection to a not-yet-ready server fails fast and attributably instead of hanging up to 600s.
• the pre-restart guarded loop also catches QueryTimeoutExceedException (the new fast-fail path).

Reproduced and validated locally (native integration runner, debug build): unmodified test passes on a fast build (race doesn't trigger); with an injected slow restart, an unguarded query(timeout=None) against a half-up server blocks indefinitely while timeout=60 fails in seconds; the fixed test passes 3/3 repeated runs.

Version info

• Merged into: 26.6.1.955 (included in 26.6 and later)
• Backported to: 25.8.26.9

[groeneai]

Pre-PR validation gate

#	Question	Answer
a	Deterministic repro?	Yes. With the real clickhouse-client against a black-hole socket (accepts TCP, never completes the native handshake = a half-restarted server), node1.query(..., timeout=None) stays blocked indefinitely (still alive after 12s, would hit the 600s DEFAULT_QUERY_TIMEOUT); timeout=5 raises QueryTimeoutExceedException in 5.0s. Also reproduced in-context: with an injected slow restart, disconnect_event.wait(N) returns False and the unguarded loop runs against a not-yet-ready server.
b	Root cause explained?	On slow (sanitizer) builds restart_clickhouse() can exceed the main thread's wait(90). The bare wait(90) ignored its return value, so the main thread proceeded while the server was still restarting. The INSERT loops then used query(timeout=None), so a connection to a half-up server (port open, not serving) blocks up to DEFAULT_QUERY_TIMEOUT=600s, exhausting pytest's 900s session timeout, which also fails the sibling test_materialized_views_cascaded_multiple (shared module fixture). CIDB signature confirms it: empty test_context_raw (session-timeout), 13 distinct PR groups in 30 days, most recent 2026-06-10.
c	Fix matches root cause?	Yes. assert disconnect_event.wait(180) makes a too-slow restart fail loudly instead of silently racing ahead; timeout=60 on the INSERTs bounds the exact 600s hang that drives the session timeout. No bounds widened, no randomization disabled, no data reduced.
d	Test intent preserved / new tests added?	Preserved. The test's assertions (row-count propagation test_table_S == test_table_H through the MV) are unchanged. No new test needed (this is a test-reliability fix, not a product-code bug).
e	Both directions demonstrated?	Yes. Half-up hang: timeout=None blocks (would reach 600s) vs timeout=N fails in N s (proven with the real client + real CommandRequest). Fixed test passes the happy path (67s) and 3/3 repeated runs on a debug build via the native integration runner.
f	Fix is general, not a narrow patch?	Yes. The same query(timeout=None) hang pattern existed in BOTH insert loops (the guarded L408 loop and the unguarded L422 loop); the fix adds a per-query timeout to both and extends the guarded loop's except to also catch the new QueryTimeoutExceedException fast-fail path. No product code change, so no upstream-origin trace applies.

Note: a separate LOGICAL_ERROR crash ('Mapping for table with UUID ... doesn't exist') also appears in this test, but it is isolated to a single feature PR (#105499, drop_detached_tables) and is that PR's responsibility, not a trunk issue. It is unrelated to this timeout fix.

Session id: cron:clickhouse-worker-slot-30:20260610-235700

[groeneai]

cc @tavplubix could you review this? It fixes the flaky test_materialized_views_replicated (the dominant CIDB signature is the 900s session timeout, which also takes the sibling cascaded_multiple down). On slow sanitizer builds the restart outlasts the test's wait(90), and the unguarded node1.query(timeout=None) inserts then block up to 600s on a half-restarted server. Fix asserts the restart wait and adds timeout=60 to the insert loops so a hung query fails fast instead of killing the session.

[clickhouse-gh]

Workflow [PR], commit [e5479ff]

Summary: ✅

---

AI Review

Summary

This PR is a test-only reliability fix for test_async_load_databases::test_materialized_views_replicated: it bounds node1.query calls during restart, catches the resulting QueryTimeoutExceedException in the pre-start loop, and asserts that restart_clickhouse completes instead of silently racing past disconnect_event.wait. The earlier bot finding about per-query timeouts accumulating across the whole loop is addressed in the current code by checking disconnect_event and a phase deadline before each retry. I found no remaining code-review blockers or major issues.

Missing context / blind spots

• ⚠️ Most GitHub checks for current head e5479ff48697ac4f6a1177fecb7b790c398f26f7 were still queued or pending during this review; the direct Praktika PR report showed no failed tests so far, but completed CI would close the runtime-validation gap.

Final Verdict

Status: ✅ Approve

[clickhouse-gh]

LLVM Coverage Report

Metric	Baseline	Current	Δ
Lines	84.60%	84.60%	+0.00%
Functions	92.30%	92.30%	+0.00%
Branches	77.20%	77.30%	+0.10%

Changed lines: No C/C++ source files changed — skipping uncovered code analysis.

Newly covered by added/modified tests: 371 line(s), 22 function(s) across 148 file(s) · Details

Top files

• src/IO/S3/copyS3File.cpp: 10 line(s)
• src/Coordination/KeeperServer.cpp: 9 line(s), 1 function(s)
• src/Storages/StorageReplicatedMergeTree.cpp: 9 line(s)
• src/AggregateFunctions/AggregateFunctionQuantileExact.cpp: 8 line(s)
• src/Functions/array/FunctionArrayMapped.h: 8 line(s)

Full report

[groeneai]

CI finished for HEAD e5479ff48697. No PR-caused failures; all checks green. The only red is CH Inc sync (internal sync check that does not run on fork PRs). Ready for review.

[alexey-milovidov]

Good improvement.