Antalya-26.3 - Backport flaky-fix commits from upstream (2026-08-07) - #2117
Open
github-actions[bot] wants to merge 19 commits into
Open
Antalya-26.3 - Backport flaky-fix commits from upstream (2026-08-07)#2117github-actions[bot] wants to merge 19 commits into
github-actions[bot] wants to merge 19 commits into
Conversation
The test injects a 1200ms network delay and expects an S3 INSERT to time out and raise. It set no explicit S3 timeout, so raising relied on a default timeout being below 1200ms on the connection path actually taken. On a fresh connection the adaptive connect timeout (500ms first attempt, 1000ms on retries) trips reliably. But when the S3 request reuses a pooled keep-alive connection there is no fresh TCP connect, so the connect timeout does not apply; the only remaining gate is the send/receive idleness timeout, whose default (adaptive 3000ms / configured 30000ms) is far above 1200ms, so the write completes and pytest.raises sees no exception. That is the rare DID NOT RAISE flake (one occurrence in 40 days of CI, zero on master). Make the request timeout the single failure mechanism: disable adaptive timeouts, keep the connect timeout above the delay, and set s3_request_timeout_ms=500. The send/receive idleness timeout applies to every attempt on both fresh and reused connections, so the 1200ms delay always exceeds it and the write times out deterministically. Also assert the raised error is a timeout, so the test cannot pass on an unrelated error. (cherry picked from commit bf0f909)
The first query materialized a ~1.3 GB haystack column (numbers(1023), each row up to 2.66 MB) only to trip the per-row regexp_max_matches_per_row (default 1000) fast-throw TOO_LARGE_ARRAY_SIZE. The throw fires on the first row (2600 matches); the rest of the column is wasted allocation. Under the shared parallel-runner memory budget, building it could hit MEMORY_LIMIT_EXCEEDED before the expected throw, so the test intermittently failed (0 master failures, 27 unrelated PRs in 30 days). Use a single materialized 1001-character row, which crosses the same per-row limit at ~1 KB instead of 1.3 GB. materialize() keeps the runtime code path the throw lives in. A low max_memory_usage on the query also guards against regressing to a large fixture. The second query is unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> (cherry picked from commit b54dc15)
The per-format setup produced all messages (including the tail broken message) to the topic before the destination materialized views existed, then created the Kafka table, the data view and the errors view in sequence. Creating the first materialized view starts the streaming loop, so on slow builds the loop could consume and commit the broken message before the errors view was attached. The broken message is then past the committed offset and never reaches the errors view: the data view has its rows but the errors view stays empty, failing the test at "Error row for format X did not appear in kafka_errors_X_mv". Create both materialized views, detach/re-attach the Kafka table, and only then produce the messages, so nothing is consumable until both views exist. This mirrors the fix accepted for the sibling test test_kafka_engine_put_errors_to_stream in ClickHouse#107025. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> (cherry picked from commit 231c36c)
test_async_connect verifies async-connect preemption: the first replica of
each shard in test_cluster_connect is an unreachable address (129.0.0.1 /
129.0.0.2), so the in-progress connect must be preempted by the
hedged_connection_timeout_ms timer and switch to the good replica. That timer
is the only path counted by the HedgedRequestsChangeReplica event, which the
test asserts fired (>= 2, one per shard).
The connect to an unreachable 129.0.0.x address does not always stall.
Depending on the runner's routing it can fail fast (host unreachable) before
the 100 ms timer. A fast failure switches the replica through the
connection-failure path (ConnectionEstablisher), which increments
DistributedConnectionFailTry, not HedgedRequestsChangeReplica. When both shards
fail fast the event stays 0; system.events omits zero-valued events, so the
query returns an empty string and int('') raises "invalid literal for int()
with base 10: ''". Slow builds (ASan, MSan, coverage) hit the fast-fail path
often enough to flake while master stays green (15 failures across 7 unrelated
PRs over 90 days, first 2026-05-05, 0 on master).
Drop the initiator's outbound packets to 129.0.0.1/129.0.0.2 with
PartitionManager so the connect always stalls and is preempted by the hedged
timer, taking the counted path. This keeps the original assertion and the
async-connect-preemption intent, and makes it deterministic regardless of
routing. The engine is not changed: hedged failover already reaches the good
replica on both paths.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
(cherry picked from commit c1e3273)
github-actions
Bot
added
antalya
cicd
Author
The test checked that an unused scalar subquery and an unused `WITH` expression are not evaluated by measuring wall clock time: it ran `sleepEachRow(3)` inside them with `max_execution_time = 2`, so the query was expected to finish quickly instead of timing out. That makes the test depend on machine load rather than on the property under test. On a loaded sanitizer runner even the trivial `SELECT * FROM system.one` exceeds the limit: the reported timeouts were `elapsed 6086.177 ms` and `elapsed 13035.071488 ms` against `maximum: 2000 ms`, both far above the 3 second sleep, and one run failed with `MEMORY_LIMIT_EXCEEDED` instead. Raising the limit from 1 to 2 seconds in ClickHouse#91744 did not help. Check the same property with `throwIf` instead: if the unused expression is evaluated, the query fails with `FUNCTION_THROW_IF_VALUE_IS_NON_ZERO`, and otherwise it succeeds immediately, with no dependency on timing. Two queries that do use the value are added to make sure the check has teeth. The test now takes about 0.4 seconds instead of 7.7 seconds. https://s3.amazonaws.com/clickhouse-test-reports/json.html?REF=master&sha=f9189f48f9bfdf7d079878e800b6eb46beb8924a&name_0=MasterCI&name_1=Stateless%20tests%20%28amd_tsan%2C%20parallel%29 Related: ClickHouse#89202 Related: ClickHouse#91744 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> (cherry picked from commit 4ce52e5)
`test_kafka_flush_by_time` asserts `uniqExact` over the insert timestamp plus a row count of at least 15 after an 18 second window. The flush count, which the test is named for, is correct in every failure; the row count is what falls short. The produce loop sent one message per `k.kafka_produce` call, and that helper constructs a new `KafkaProducer` on every call, so every message paid a full client bootstrap including the `kafka-python` broker-version probe. In the failing CI run that probe took 2.1 s on one iteration (3.023 s for the whole iteration, against 0.107-0.113 s for the other 17), which cost about 3 messages out of a budget that had roughly half a message of slack. The result was 5 + 9 = 14 rows over 2 distinct flush timestamps, so the flush-by-time behaviour was correct and only the total was short. Both engines flushed on schedule (7841/7599 ms, versus 7537/7535 ms in the parametrization that passed on the same server 21 seconds earlier), so this is not a `StorageKafka` or `StorageKafka2` flush race. Hoist one producer out of the loop, reusing the existing `k.get_kafka_producer` and `k.producer_serializer` helpers that five other producers in this file already use, and close it after the producer thread is joined. Per-iteration cost drops from 0.107-3.02 s to a single send plus flush round trip. The assertion, the threshold of 15, the 18 second window and the 0.8 s message rate are unchanged. Measured after the change: producer constructions per run drop from 17 to 1; there are still exactly 2 in-window flushes in 25 of 25 runs and never 3, so the distinct-timestamp assertion stays live and the cadence stays on the `stream_flush_interval_ms` default at 7514-9020 ms; in-window row totals are 19 to 21 against the threshold of 15, instead of 14. 50 executions across both parametrizations pass. Verified in both directions by injecting the measured 2.9 s stall into the produce path: on master the test fails with the CI signature, with this change it passes even when the stall fires inside the loop, and reverting only the hoist under the same stall brings the failure back. (cherry picked from commit dce0ed2)
The test ends with
SELECT sum(rows) FROM system.parts
WHERE ... AND startsWith(name, 'patch')
and expects `200`, but that query was racing against the background cleanup
thread.
Once `OPTIMIZE TABLE ... FINAL` has applied the patches, the data version of the
merged part reaches the patches' max data version, so `clearUnusedPatchParts`
considers both patch parts unused and drops them. For `ReplicatedMergeTree` the
drop goes through a `DROP_PART` log entry, which removes the parts outright
rather than leaving them `Outdated`, so they disappear from `system.parts` and
the last query returns `0` - or `100`, when only one of them had been dropped
yet.
On a fast runner the test finishes long before the first iteration of the
cleanup thread, so the race is invisible. On a loaded runner it gets a turn
before the last query; accordingly, all observed failures are on `amd_tsan`,
`amd_msan` and flaky checks.
Pin `remove_unused_patch_parts = 0` on the table, the same way
`03100_lwu_01_basics` and other lightweight-update tests already do, so the
patch parts stay in place for the final check. The future-reads check itself is
unaffected.
Reproduced locally by making the cleanup thread run every second: without this
change the test fails with exactly the diff seen in CI (`-200` / `+0`), with it
the test passes.
CI report: https://s3.amazonaws.com/clickhouse-test-reports/json.html?REF=master&sha=107275200b9ab432ec9375a091aad62690484bfd&name_0=MasterCI&name_1=Stateless%20tests%20%28amd_tsan%2C%20parallel%29
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
(cherry picked from commit 9101372)
The statement `SELECT a FROM t_max_rows_to_read LIMIT 1` has no ORDER BY, so its result is unspecified: any row of the table is a legal answer. On a single reader the first granule is read first, so the answer happened to be 0 and the reference file has asserted that one particular unspecified answer since the test was written. Under parallel replicas the read is split into per-replica mark ranges and whichever replica answers first supplies the row that LIMIT 1 keeps, so the assertion is not stable and the test fails intermittently with a one-line reference diff of 32 instead of 0. 32 is not a random row. On the failing lane all parts are on object storage, so PartRangesReadInfo takes the remote branch where both merge_tree_min_rows_for_concurrent_read_for_remote_filesystem and its bytes counterpart default to 0, and minMarksForConcurrentRead collapses to merge_tree_min_read_task_size = 8 marks per task. With index_granularity = 4 the second task begins at mark 8, whose first row is 8 * 4 = 32. On a local disk the same formula yields a single task covering the whole part, which is why the failure only appears on the object-storage lanes and why earlier local-disk reproduction attempts could only ever observe 0. Project a constant instead, so every legal row selection produces the reference value. The reference file does not change, because it already asserts 0. What the line measures is unchanged, verified rather than assumed: EXPLAIN still shows ReadFromMergeTree and ReadFromRemoteParallelReplicas, read_rows and ProfileEvents['SelectedMarks'] are identical before and after (4 and 25), and lowering max_rows_to_read to 3, 2 or 1 still raises TOO_MANY_ROWS at exactly the old thresholds, on both analyzers. The read is not eliminated because chooseSmallestColumnToReadFromStorage injects the table's single column. max_rows_to_read = 20 and index_granularity = 4, which are what the test actually measures, are untouched, and no setting is pinned, so nothing leaks into the following statements. A no-parallel-replicas tag or a parallel_replicas_blacklist entry was deliberately avoided: either would remove the whole test from that lane, where it otherwise runs green thousands of times, while only one of its seven statements is affected. (cherry picked from commit cc956b9)
`03394_distributed_shuffle_join_with_in` and `03394_distributed_shuffle_join_with_aggregation` assert that `BuildRuntimeFilter` appears in the `EXPLAIN` plan, but neither pins the two settings that decide whether the runtime join filter is built at all. Reproduced locally with the exact randomized setting set from the CI report (https://s3.amazonaws.com/clickhouse-test-reports/json.html?PR=110321&sha=fbebd96fb7f9dcee3d3bdb40df67558ffeaa1b40&name_0=PR&name_1=Stateless%20tests%20%28amd_msan%2C%20WasmEdge%2C%20parallel%2C%201%2F2%29, PR ClickHouse#110321) and minimized to a two-setting combination: `query_plan_optimize_join_order_randomize` nonzero together with `join_runtime_filter_min_probe_rows` above the estimated probe size. Join-order randomization changes which side is the probe side, and `joinRuntimeFilter` then skips the filter because the probe side is estimated to produce at most `join_runtime_filter_min_probe_rows` rows, so the `BuildRuntimeFilter` step disappears from the plan and the plan diff fails. Neither setting reproduces the failure alone. Pin both, mirroring `03394_distributed_shuffle_join_with_prewhere`, which already pins them for the same reason. Verified: with the randomized setting set the plan output of both tests now matches the reference exactly (before the fix `BuildRuntimeFilter` was absent in both). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> (cherry picked from commit dbd998c)
A span reaches system.opentelemetry_span_log only when its holder is destroyed: TracingContextHolder::~TracingContextHolder enqueues it via shared_span_log->add(...) (src/Common/OpenTelemetryTraceContext.cpp:469-472), and SpanHolder::finish() does the same at :207-210. SYSTEM FLUSH LOGS opentelemetry_span_log flushes only what is already queued, so it cannot flush a span whose holder is still alive. The TCPHandler SERVER span is exactly such a span. Its holder is declared one scope out of the query block with the comment "Initialized later. It has to be destroyed after query_state is destroyed." (src/Server/TCPHandler.cpp :572-573), is constructed at :604-609, and is destroyed only at the end of the connection-loop iteration, that is after query_state->finalizeOut(out) at :1153 has already sent the client its answer. check_span read the count once, so a flush issued by the very next statement could observe a short count and report [operation_name like 'TCPHandler' ]=0, expected: 1 while the test's own dump showed every other span for that trace id. Retry the read while the numeric count is below the expectation and let the existing exact comparison decide. The assertion, the failure line and the span dump are unchanged. This mirrors the merged fixes for the two sibling tests with the identical race, 02417_opentelemetry_insert_on_distributed_table and 01455_opentelemetry_distributed. The sleep budget is test-wide rather than per call: the default path has 11 check_span calls with a positive expectation, so a per-call bound would cost up to about 165 seconds when many assertions are short, and Fast test runs this file with --timeout 60 (ci/jobs/fast_test.py:378), which would kill the run and lose the diagnostic. A count already above the expectation is reported on the first iteration, and a site expecting 0 reads exactly once. Also break out immediately on a non-numeric result and quote the comparison operands, which removes a real shell error seen in CI when the client failed and the count came back empty: 02423_ddl_for_opentelemetry.sh: line 52: [: =: unary operator expected (cherry picked from commit c29108d)
test_keeper_auth/test.py::test_partial_auth[get_genuine_zk] could fail in its finally cleanup with NoAuthError while deleting /test_partial_acl_delete/subnode, right after the test had granted ALL on the parent. Only the Apache ZooKeeper parametrization was affected; the ClickHouse Keeper one passed the identical sequence. ZooKeeper authorizes a delete against the PARENT's ACL, and its setACL publishes an outstanding ChangeRecord that still carries the old ACL: the new ACL goes only into the SetACLTxn, and duplicate() copies acl verbatim while updating only stat.aversion. getRecordForPath prefers that outstanding record, so a delete prepped after the client's setACL reply but before that txn has been applied is authorized against the pre-setACL ACL, which this test deliberately creates with delete=False. Keeper instead writes the new acl_id into uncommitted state and reads it back in the delete preprocess, i.e. it is read-your-own-writes on ACLs where ZooKeeper is not - hence the asymmetry. This is why the earlier remedy did not hold: it added the two setACL calls that already succeed immediately before the delete that still fails. The permission is not the problem, its visibility is. Retry that one cleanup delete, only on NoAuthError, bounded by a wall-clock deadline. The subsequent parent delete is authorized against "/", whose ACL this test never touches, so it is left un-retried and still fails loudly. Validated by opening the window on demand (issuing the child delete before the parent setACL is acknowledged) against a 3-node ZooKeeper 3.6.2 ensemble: 10 failures in 40 iterations. With the helper at timeout=0 the failure still occurs (2 of 32 opened windows); at the shipped timeout=30 it does not (0 of 85). The in-body pytest.raises(NoAuthError) oracle was confirmed non-vacuous. 50 runs of both parametrizations and the whole 26-test file pass. (cherry picked from commit bd24ffd)
The test intermittently failed with a per-query memory limit error while reading the wide `String` column `s`: Code: 241. Query memory limit exceeded: would use 5.00 GiB (attempt to allocate chunk of 512.00 MiB), maximum: 4.66 GiB: (while reading column s) ... While executing MergeTreeSelect. (in query: select sum(h = cityHash64(s)) from huge_strings) This is the per-query `max_memory_usage` limit, not the server-wide `(total)` one; the two share `Code: 241` but are different failure modes. The cause is aggregate read-stream concurrency rather than one oversized allocation. `tests/clickhouse-test` draws `max_threads = 32` with probability 0.03, and `max_streams_to_max_threads_ratio` is 1 and not randomized, so about 32 read streams are created. Each stream deserializes `s` into its own `ColumnString::chars` buffer, and with rows up to 9 MB it grows that buffer through the doubling realloc in `SerializationString`. `Allocator::realloc` charges the new size before freeing the old one, so the throwing allocation is tested against everything already live across all streams. Meanwhile `max_memory_usage` is a fixed suite default of 4.66 GiB and is not randomized. The margin is thin: the observed rows report `would use` 4.70 to 5.04 GiB, so 1 to 8 percent above the limit. `SerializationString` is the last straw, not the defect. Pin `max_threads = 3` on the two verification queries that materialize `s`. Three is the top of the range 97 percent of runs already draw, so it is not an invented constant, and it cuts the concurrent buffer footprint by roughly 6x against an overshoot of a few percent. `select count()` is left unpinned: it is answered from metadata, with a measured peak of 0 B, and where the trivial count is disabled the smallest compressed column is chosen, which can never be `s`. The insert loops, which are the behavior under test, are untouched, and the assertions and the reference file are unchanged. Validated on an ASAN+UBSAN build against a forced worst-case fixture of 60 Compact parts holding 5 GiB of `s`. Both arms ran on one dataset and one binary with only the pin differing, and both carried the same `--max_threads 32` client option: unpinned 12/12 failures reproducing the signature above at a 4.6 GiB peak, pinned 0/12 at 645 MiB. The `sum(l = length(s))` line behaves the same under its `optimize_functions_to_subcolumns = 0` draw, failing 6/6 unpinned. Running the fixed test through `clickhouse-test` with `--client-option max_threads=32` confirms the query-level clause wins over the client option, with `system.query_log` recording an effective `max_threads` of 3 for the pinned queries and 32 for the unpinned count. 50 randomized runs pass, 3 of which drew the 32-thread value, with no change in runtime. Note this signature has a single public CI hit in the last 365 days, so a green CI run on this pull request is not evidence about the mechanism either way. (cherry picked from commit 967a951)
…headroom
The test intermittently failed on pread_threadpool with only its 4th column
wrong (1 1 1 0): QueryLocalReadThrottlerSleepMicroseconds came out 0 while the
query was demonstrably slow and all bytes had passed the throttler.
The 4th assertion is a coupled oracle. Throttler::throttle increments the bytes
counter unconditionally, but the sleep counter only inside `if (block)`, and
block requires tokens_value < 0. The token bucket only goes negative when bytes
arrive faster than the cap, so once a loaded runner's read rate falls to about
the cap the throttler correctly does not sleep and the assertion fails. With a
1 MiB/s cap over an 8 MB payload the fixture had only 1.115x of margin between
the cap and the arrival rate at which the assertion breaks (measured), which a
contended sanitizer runner erases. The nominal required sleep was also
8e6/1048576-1 = 6.63 s, already below the 7 s the first column demands, so that
column had been passing on unrelated overhead; the in-file comment claiming
"(8-1)/1=7 seconds" was wrong because '1M' is 1048576, not 1e6.
The sleep time is not lost or misattributed to a pool thread:
ThrottlerSleepMicroseconds is 0 as well in the reproduced failure, so no thread
slept at all. Both counters are charged through the same
CurrentThread::getProfileEvents() a few lines apart, and the pread_threadpool
throttle call runs on the consuming pipeline thread in
AsynchronousReadBufferFromFileDescriptor::nextImpl, not on a pool thread -
ThreadPoolReader never throttles.
Co-reduce the payload (1e6 -> 2e5 rows) and the cap ('1M' -> 160000 B/s)
together, which raises the required sleep to 9 s at comparable wall clock, and
rescale the two byte thresholds by the same factor so they keep asserting that
the whole payload passed through the throttler. All four assertions, both time
thresholds and the reference file are unchanged. This is the same fix that was
merged for the sibling 04103_user_network_bandwidth_throttler (ClickHouse#103422).
Reproduced deterministically by capping the arrival rate with
max_execution_speed_bytes and max_threads=1: the unmodified test prints
1 1 1 0 on all three arms, and 1 1 1 1 with the injection absent. The measured
failure boundary moves from 1.115x of the cap to at most 1.006x, and the test
now also passes at arrival rates below its own cap. 50/50 clean local runs;
runtime 23.9 s -> 28.3 s.
No source change: the throttler behaved correctly at every arrival rate
measured, so there is no product defect here.
(cherry picked from commit a2a3a41)
test_same_credentials and test_no_credentials insert into one replica
and then assert the table contents on the other, with a fixed
time.sleep(1) as the only barrier.
ReplicatedMergeTreeSink commits the part's /log/log-N znode in the same
multi-op transaction that commits the part, so when the INSERT returns
the log entry is durably in ZooKeeper. The other replica, however,
learns of it only asynchronously: its queue_updating_task pulls the
log, a background pool task executes the resulting GET_PART, and the
part is fetched over the interserver HTTP endpoint and committed. None
of that chain is bounded by anything the test controls, so on a loaded
sanitizer runner it routinely exceeds one second and the assertion
reads a stale replica:
AssertionError: assert '111\n' == '111\n222\n'
That reached master at cfc1fd2. Over
the last 90 days CIDB has 7 occurrences of the lag signature - an
AssertionError at one of the four reads that query the replica which
did not receive the insert - across 4 distinct refs, spanning both
tests. Over the same 90 days CIDB records 220751 OK and 13 FAIL
results for these two tests, so the lag accounts for 7 of the 13
failures: a genuine low-rate race rather than a broken check.
Replace the barrier instead of the timing constant: before each
cross-replica assertion, the replica about to be read runs SYSTEM SYNC
REPLICA test_table with an explicit timeout. waitForProcessingQueue
first calls pullLogsToQueue(..., SYNC), so a pending GET_PART is
guaranteed visible before the wait set is computed, then triggers the
background assignee, then addSubscriber snapshots the queue's entry
ids under state_mutex while registering the callback, so the wait
cannot miss the entry it must wait for and returns as soon as the
fetch lands. That turns an unbounded asynchronous wait into a
deterministic barrier at no fixed cost. The file already uses this
idiom at four other places. Raising the sleep was rejected: it treats
the symptom and re-races on a slower runner.
Scope is deliberately two tests and four lines. In
test_different_credentials and test_credentials_and_no_credentials the
sleep guards a negative assertion across intentionally mismatched
interserver credentials, where replication must not happen; a sync
there can never complete and fails with QueryTimeoutExceedException,
which was measured rather than assumed. All eight assertions are left
byte-identical, so the change only strengthens the barrier.
Validated with the fetch stalled deliberately on the reading replica:
the assertion fails before this change with the exact CI signature and
passes after it on the same binary, and reverting only the new barrier
reddens it again. 200/200 green over 50 repeats of the whole file.
(cherry picked from commit 0e2550f)
The test asserts that a clickhouse-benchmark run logs 3 queries under one initial_query_id. On a loaded runner it read 0 instead of 3. The benchmark process never ran its query. Connection::connect uses handshake_timeout_ms (default 10000, Settings.cpp:410) as the socket receive timeout for the server Hello read, and clickhouse-test gives the benchmark no timeout overrides (shell_config.sh builds CLICKHOUSE_BENCHMARK_OPT0 from only --port, --database and --log_comment, while the client gets connect_timeout and receive_timeout from the runner). When the accept/handshake path stalls for longer than 10 s the benchmark exits with SOCKET_TIMEOUT before sending any query, so query_log has no rows for that query_id and the assertion reads 0. In the reported run the server accepted no TCP connection for 22.50 s (21:45:14.247 to 21:45:36.747 in the job's clickhouse-server.log, zero TCPHandlerFactory accepts in between, and executeQuery starts fell to 0 for the 11 s from 21:45:17 to 21:45:27). The benchmark's connection was accepted at the end of that window and the server logged "Client has gone away", the benchmark having already given up. The job's query_log.tsv confirms it: the failing instance has 0 rows with client_name = 'ClickHouse benchmark', while each of the 6 in-place reruns has exactly 3. Give connect and handshake a generous budget, matching the same fix already merged for 01600_benchmark_query (ClickHouse#108570) and present in 03630_benchmark_accept_invalid_certificate and 03636_benchmark_error_messages. The assertion is unchanged, so what the test verifies is unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> (cherry picked from commit 7199612)
The test counted every `Done processing connection.` message in the server log
and asserted that its own connection was the only one that closed. Two
unrelated connections break that count:
* The readiness probe of `cluster.start` connects to the TCP port and closes it
without sending any data. The port is bound and listening from `createServers`
onwards, so the kernel completes the handshake long before the server starts
accepting; the probe is served only once startup finishes, which can be after
the test has already sampled the initial count. This is what fails on MSan,
where startup takes seconds:
Application: Ready for connections.
TCPHandlerFactory: TCP Request. Address: 172.16.2.1:59612
TCPHandler: Client has not sent any data.
TCPHandler: Done processing connection. <-- counted, but not sampled
* `clickhouse-client` reconnects after the server drops it, so a single run of
the test can close two connections by itself. That is the shape of the earlier
failures, where both tests reported a delta of two.
Count the connections closed *because a limit was reached* instead - the server
logs `Closing connection due to limits` exactly once per such connection, and
the reason distinguishes the query-count limit from the time limit. Neither the
readiness probe nor a reconnecting client reaches a limit: `query_count` and
`connection_timer` are per-connection, so a fresh connection starts from zero.
With the count no longer polluted by startup connections, the `sleep` that tried
to wait them out is not needed.
https://s3.amazonaws.com/clickhouse-test-reports/json.html?REF=master&sha=7d22d13ab50ff1474fad83ba8e19db7b60c24412&name_0=MasterCI&name_1=Integration%20tests%20%28amd_msan%2C%203%2F8%29
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
(cherry picked from commit 2938a9d)
The fourth assertion in this test snapshotted `parts`, `active_parts` and `total_marks` from `system.tables` immediately after `ALTER TABLE ... DETACH PARTITION 1` and required exactly `2 1 2`. Two of those three columns are not well defined at that point. `parts` is `getAllPartsCount()` = `data_parts_by_info.size()` and `total_marks` sums `getMarksCount()` over the same container, so both count parts in *every* state, `Outdated` included. `DETACH PARTITION` does not erase the detached part: it covers it with an empty level+1 part, flipping the original to `Outdated`, and then makes a single best-effort synchronous reclamation pass. That pass may legitimately remove nothing. `grabOldParts` declines when it cannot immediately take `grab_old_parts_mutex` (the per-table cleanup thread calls the same function every second by default), and it skips any part whose `DataPartPtr` is still held elsewhere, for example by a concurrent read. In those cases the detached part remains in `data_parts_by_info` and both counters include it, so the test reads `3 1 4`. `active_parts` is the `total_active_size_parts` atomic, maintained under the parts lock alongside every state transition, and it is 1 in every state reachable here. The final query now asserts only that column. The first three assertions are unchanged and still check `parts` and `total_marks` exactly: no part is ever `Outdated` before the DETACH, and each of the two partitions holds a single part, so nothing is mergeable and the all-states and active-only counters coincide. Note that `SETTINGS old_parts_lifetime = 0` does not fix this. The ownership and lock-contention checks in `grabOldParts` are evaluated before the removal-time gate, so the race survives; it would only change which value the test expects. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> (cherry picked from commit 5af2fa9)
…nt before reads
The test's kazoo client is pinned to a single ensemble member, zoo1
(`cluster.get_kazoo_client("zoo1")` -> `helpers/cluster.py:4672` connects to that
one container), while the server's session spans zoo1/zoo2/zoo3
(`helpers/zookeeper_config.xml`). `CREATE NAMED COLLECTION` writes synchronously and
commits on leader+quorum before the query returns
(`NamedCollectionsMetadataStorage.cpp:313` -> `ZooKeeper.cpp:906`), so there is no
server-side durability gap. But `quorum_reads` defaults to false
(`src/Coordination/CoordinationSettings.cpp:62`, not overridden by any integration
keeper config), so a follower read is not linearizable: if zoo1 has not yet applied
the committed transaction to its in-memory state, its `get` answers from the
pre-write snapshot and raises `NoNodeError`.
The read is also effectively single-shot. `KazooClientWithImplicitRetries.get`
routes through `KazooRetry`, whose retry set is
`(ConnectionLoss, OperationTimeoutError, ForceRetryError)` plus
`SessionExpiredError`; `NoNodeError` is not a member, so `KazooRetry.__call__`
re-raises it on the first attempt.
`zk.sync(path)` forces the connected follower to catch up with the leader before
responding, eliminating the race rather than waiting it out. It is a real raft
barrier, not a hint: `ZooKeeperSyncRequest::isReadRequest()` returns false
(`src/Common/ZooKeeper/ZooKeeperCommon.h:124`) and `OpNum::Sync` is classified as a
write (`ZooKeeperConstants.cpp:132`), and kazoo's `sync` blocks until the response is
acknowledged.
This mirrors 9476b38, which fixed the same class in
the sibling module `tests/integration/test_named_collections/test.py` with eight
`zk.sync(ZK_PATH)` insertions, and follows the older pattern in
`tests/integration/test_drop_replica/test.py`.
The two sites changed here are the only unpolled external kazoo reads left in the
file; the reads at `wait_zk_child_exists`/`wait_zk_child_absent` are bounded polling
loops and are left alone. Fixing `check_encrypted` covers five tests
(test_zookeeper_encrypted_storage, test_encryption_persists_after_restart,
test_special_characters_and_unicode, test_many_keys, test_survives_restart); the
inline read in test_new_replica_encrypted_data_integrity needs its own line, and is
where a second occurrence of this failure was recorded.
Every existing assertion is unchanged, so a genuinely missing or unencrypted znode
still fails the test.
(cherry picked from commit e1857b6)
The test asserts that a read-only table's part stays on the local volume while a writable control table's part is moved to the remote volume in the background. It failed once on amd_tsan with both disk_name reads flipped to s3_disk, the writable control line in between still passing: the read-only table's part was already remote at the first observation, before the test had marked the table read-only at all. This is not a product defect. table_readonly is set after the INSERT, so no move guard was bypassed; the guard was simply never handed a local part to hold in place. The fixture encoded a wall-clock assumption instead. SYSTEM STOP MOVES cancels parts_mover.moves_blocker, which only the background mover reads, so it cannot gate the INSERT's own space reservation: when a part's move TTL is already expired at write time, tryReserveSpacePreferringTTLRules reserves directly on the TTL destination volume, because perform_ttl_move_on_insert defaults to true and the local_remote policy did not override it. The interval that must stay under the 5 second TTL is therefore inside the INSERT statement, from the now() constant fixed at query analysis time to the time(nullptr) read at reservation, and a loaded runner can exceed it. Disable perform_ttl_move_on_insert on that volume so a part always starts on the local volume however long the INSERT takes. MergeTreePartsMover never reads the flag, so the part stays move-eligible and the writable control table still moves, which is what keeps the test meaningful: reverting only the table_readonly guard in MergeTreeData::scheduleDataMovingJob makes the test fail again with "readonly disk after control moved: s3_disk". The assertion, the reference file and the test tags are unchanged. The signature has one occurrence in 180 days and none on master. The other two tests using this shared policy move parts with explicit ALTER ... MOVE and declare no TTL, so an insert-time TTL-move flag cannot reach them. (cherry picked from commit 767e8b4)
strtgbb
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Automated backport of upstream flaky-fix commits.
Applied
bf0f90973919Fix flaky test_s3_table_functions_timeouts (committed 2026-07-23T01:36:26Z)b54dc15ccf10Fix flaky test 01661_extract_all_groups_throw_fast under memory pressure (committed 2026-07-23T02:50:38Z)231c36ce98d6Fix flaky test_kafka_formats_with_broken_message (committed 2026-07-23T22:46:53Z)c1e3273140e6Fix flaky test_hedged_requests::test_async_connect (committed 2026-07-24T17:38:27Z)Skipped (cherry-pick conflict — manual backport needed)
2d531c66b031Fix flaky test04538_with_fill_max_execution_time(committed 2026-07-21T02:39:18Z)f2bf53742903Fix flaky test 04011_predicate_statistics_log under parallel replicas (committed 2026-07-23T12:07:23Z)9bcba1b43879Fix flaky test 04546_constant_join_shrink_stored_blocks (committed 2026-07-25T20:57:39Z)